fix: stabilize Windows recording startup#141
Conversation
📝 WalkthroughWalkthroughFFmpeg session startup now cleans up uncommitted processes, preview stderr draining is bounded, Windows FFmpeg paths and encoder selection are adjusted, and smoke scripts gain platform-aware binaries, preview layout setup, dynamic timing, and shared reply parsing. ChangesFFmpeg capture compatibility
Platform-aware smoke validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SmokeScript
participant StudioWebSocket
participant Compositor
participant NativePreview
SmokeScript->>StudioWebSocket: open studio tab
SmokeScript->>StudioWebSocket: issue scene.layout.apply_preview
StudioWebSocket->>Compositor: apply screen-only layout intent
Compositor-->>StudioWebSocket: commit screen-only preset
SmokeScript->>NativePreview: start packaged native preview flow
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/videorc-backend/src/recording.rs (1)
1787-1799: 🩺 Stability & Availability | 🔵 TrivialRun the Rust and recording-studio gates before merging.
cargo fmt --check --all,cargo test -p videorc-backend,cargo clippy -p videorc-backend -- -D warnings,pnpm audit:rust, andpnpm smoke:recording-studio.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/videorc-backend/src/recording.rs` around lines 1787 - 1799, Before merging the recording process changes around UncommittedCaptureProcess::new and child_mut, run all required validation gates: cargo fmt --check --all, cargo test -p videorc-backend, cargo clippy -p videorc-backend -- -D warnings, pnpm audit:rust, and pnpm smoke:recording-studio. Resolve any failures before proceeding.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/videorc-backend/src/recording.rs`:
- Around line 10452-10459: Remove the command.kill_on_drop(true) configuration
from uncommitted_capture_process_terminates_child_on_drop so the test relies
exclusively on UncommittedCaptureProcess::drop to terminate the child, while
preserving the existing child spawn and guard-drop assertions.
- Line 1943: Move the uncommitted_capture_process.commit() call out of the early
startup path and keep the guard active through all subsequent asynchronous
setup. Transfer the child directly to monitor_session immediately after
installing ActiveRecording, so ownership is committed only once monitor_session
can take responsibility for cleanup.
- Around line 3027-3035: Update collect_preview_stderr to return the collected
bytes together with an explicit success/failure status or diagnostic; preserve
stderr on successful collection, but include a clear JoinError diagnostic for
Ok(Err(_)) and the timeout diagnostic before aborting stderr_task, so callers
and logs can distinguish empty stderr from drain failure.
- Around line 9614-9621: Update escape_tee_target() to replace each backslash
with the tee syntax’s two-character escaped form, rather than doubling it twice.
Preserve the existing escaping for pipes and brackets, and add regression
coverage verifying correct drive-letter and UNC Windows path handling.
---
Nitpick comments:
In `@crates/videorc-backend/src/recording.rs`:
- Around line 1787-1799: Before merging the recording process changes around
UncommittedCaptureProcess::new and child_mut, run all required validation gates:
cargo fmt --check --all, cargo test -p videorc-backend, cargo clippy -p
videorc-backend -- -D warnings, pnpm audit:rust, and pnpm
smoke:recording-studio. Resolve any failures before proceeding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2b68fb9-6658-4177-a5c1-2a81ee13ce79
📒 Files selected for processing (3)
crates/videorc-backend/src/recording.rsscripts/smoke-captions-contract.mjsscripts/smoke-session-ops.mjs
| async fn collect_preview_stderr(stderr_task: &mut tokio::task::JoinHandle<Vec<u8>>) -> Vec<u8> { | ||
| match timeout(PREVIEW_STDERR_DRAIN_TIMEOUT, &mut *stderr_task).await { | ||
| Ok(Ok(stderr)) => stderr, | ||
| Ok(Err(_)) => Vec::new(), | ||
| Err(_) => { | ||
| stderr_task.abort(); | ||
| Vec::new() | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Preserve a diagnostic when stderr collection fails or times out.
Both failure branches silently return genuine-looking empty stderr. Return a status/error alongside the bytes so the timeout message or logs distinguish “no stderr” from “stderr drain failed.”
As per coding guidelines, “Prefer explicit status and diagnostics over silent fallbacks.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/videorc-backend/src/recording.rs` around lines 3027 - 3035, Update
collect_preview_stderr to return the collected bytes together with an explicit
success/failure status or diagnostic; preserve stderr on successful collection,
but include a clear JoinError diagnostic for Ok(Err(_)) and the timeout
diagnostic before aborting stderr_task, so callers and logs can distinguish
empty stderr from drain failure.
Source: Coding guidelines
| fn escape_tee_target(target: &str) -> String { | ||
| // The tee muxer uses `|` to separate slave outputs and `[]` for per-output | ||
| // options; backslash-escape those so URLs/paths cannot break the filtergraph. | ||
| target | ||
| .replace('\\', "\\\\") | ||
| .replace('\\', "\\\\\\\\") | ||
| .replace('|', "\\|") | ||
| .replace('[', "\\[") | ||
| .replace(']', "\\]") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'escape_tee_target|tee_output_args' crates/videorc-backend/src/recording.rs
pnpm smoke:recording-studioRepository: TheOrcDev/videorc
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helper and its call sites with line numbers.
sed -n '9588,9650p' crates/videorc-backend/src/recording.rs
printf '\n--- usages ---\n'
rg -n -C2 'escape_tee_target\(|tee_output_args\(' crates/videorc-backend/src/recording.rs
printf '\n--- nearby tests mentioning tee / ffmpeg paths ---\n'
rg -n -C2 'tee|ffmpeg_file_path|escape_tee_target|UNC|Windows|\\\\\\\\' crates/videorc-backend/src/recording.rsRepository: TheOrcDev/videorc
Length of output: 42547
🌐 Web query:
FFmpeg tee muxer av_get_token backslash escaping slave filename Windows path UNC double backslash
💡 Result:
When using the FFmpeg tee muxer on Windows, special characters—including backslashes used in UNC paths (e.g., \server\share)—often require multiple levels of escaping because the tee muxer parses the slave string and then passes individual segments to subsequent layers [1][2][3]. The function av_get_token is used internally by the tee muxer to split slave outputs based on delimiters like '|' [2][4]. Because this process consumes backslashes as escape characters, a simple backslash in a path is frequently interpreted as an escape sequence rather than a literal file path separator [2][5]. To successfully use a Windows UNC path or any path containing backslashes within the tee muxer string: 1. Escape Backslashes: You must escape each backslash with an additional backslash. For example, a UNC path like \server\path would typically need to be represented as \\server\path within the tee string [6]. 2. Account for Multi-Level Escaping: Because the tee muxer often involves two levels of parsing—one for the tee muxer itself and another for the underlying muxer or filter (especially when options are included in brackets)—you may need to increase the number of backslashes (e.g., using four or more) if the path resides inside an option block [1][6][7]. 3. Alternative: Use Forward Slashes: FFmpeg generally handles forward slashes (/) as directory separators on Windows as well [8]. Converting your path to use forward slashes (e.g., //server/path) is often the most reliable way to avoid backslash escaping issues entirely, as it bypasses the need for the complex escape sequences [8]. If you are struggling with a specific command, verify if the path is being misinterpreted by checking the verbose logs (add -v verbose to your command), which will show how the tee muxer is splitting the slave strings [2][6]. If you see the path being truncated or mangled, it is a definitive sign that the escaping level is incorrect [6][9].
Citations:
- 1: https://ffmpeg.org/pipermail/ffmpeg-cvslog/2018-April/113555.html
- 2: https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/tee.c
- 3: http://mplayerhq.hu/pipermail/ffmpeg-cvslog/2013-February/060528.html
- 4: https://www.ffmpeg.org/doxygen/8.0/tee_8c_source.html
- 5: https://www.ffmpeg.org/doxygen/5.1/tee_8c_source.html
- 6: https://ffmpeg.org/pipermail/ffmpeg-user/2017-July/036675.html
- 7: https://ffmpeg.org/pipermail/ffmpeg-devel/2013-December/152017.html
- 8: https://ffmpeg.org/pipermail/ffmpeg-devel/2021-January/274244.html
- 9: https://ffmpeg.org/pipermail/ffmpeg-user/2019-November/045973.html
Avoid over-escaping tee targets on Windows. escape_tee_target() should map \ to \\, not \\\\; tee only tokenizes this spec once, so the current code inflates drive-letter and UNC paths. Add regression coverage for both path forms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/videorc-backend/src/recording.rs` around lines 9614 - 9621, Update
escape_tee_target() to replace each backslash with the tee syntax’s
two-character escaped form, rather than doubling it twice. Preserve the existing
escaping for pipes and brackets, and add regression coverage verifying correct
drive-letter and UNC Windows path handling.
| #[tokio::test] | ||
| async fn uncommitted_capture_process_terminates_child_on_drop() { | ||
| let mut command = long_running_capture_command(); | ||
| command.kill_on_drop(true); | ||
| let child = spawn_owned_tokio(&mut command).expect("spawn capture child"); | ||
| let pid = child.id().expect("capture child pid"); | ||
|
|
||
| drop(UncommittedCaptureProcess::new(child, Vec::new())); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove kill_on_drop(true) so this test exercises the guard.
Line 10455 independently terminates the child when its handle drops, so the test still passes if UncommittedCaptureProcess::drop stops killing children. Test the guard without this fallback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/videorc-backend/src/recording.rs` around lines 10452 - 10459, Remove
the command.kill_on_drop(true) configuration from
uncommitted_capture_process_terminates_child_on_drop so the test relies
exclusively on UncommittedCaptureProcess::drop to terminate the child, while
preserving the existing child spawn and guard-drop assertions.
…windows-recording-ffmpeg bring in changes from main
|
Pushed a6026aa to address both failed Windows jobs:
Local checks completed:
The targeted Rust test build was interrupted locally before completion; the refreshed Windows CI run will verify it. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/probe-live-audio-controls.mjs`:
- Around line 29-33: Update the process timeout calculation alongside
probeDurationSeconds so processTimeoutMs is derived from the computed probe
duration rather than hardcoded to 30 seconds. Ensure the resulting timeout
includes sufficient margin to exceed probeDurationSeconds as liveCommands grows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa2b3414-483c-4f8c-976c-03d68ae3937e
📒 Files selected for processing (6)
apps/desktop/src/main/smoke-command-security.test.tsapps/desktop/src/main/smoke-command-security.tscrates/videorc-backend/src/recording.rsscripts/lib/smoke-command-callers.test.mjsscripts/probe-live-audio-controls.mjsscripts/smoke-recording-native-preview-app.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/videorc-backend/src/recording.rs
| const probeDurationSeconds = Math.ceil( | ||
| ((liveCommands.length + 1) * productionReplyTimeoutMs) / 1000 + | ||
| artifactAnalysisWindowSeconds | ||
| ) | ||
| const processTimeoutMs = 30000 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Scale processTimeoutMs dynamically with probeDurationSeconds.
Since probeDurationSeconds is now dynamically computed based on the length of liveCommands, leaving processTimeoutMs hardcoded to 30 seconds introduces a risk of the process timing out prematurely if more commands are added in the future, or if runner scheduling overhead consumes the remaining margin.
Scale the process timeout dynamically to ensure it exceeds the computed probe duration.
🐛 Proposed fix
const probeDurationSeconds = Math.ceil(
((liveCommands.length + 1) * productionReplyTimeoutMs) / 1000 +
artifactAnalysisWindowSeconds
)
-const processTimeoutMs = 30000
+const processTimeoutMs = Math.max(30000, probeDurationSeconds * 1000 + 5000)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const probeDurationSeconds = Math.ceil( | |
| ((liveCommands.length + 1) * productionReplyTimeoutMs) / 1000 + | |
| artifactAnalysisWindowSeconds | |
| ) | |
| const processTimeoutMs = 30000 | |
| const probeDurationSeconds = Math.ceil( | |
| ((liveCommands.length + 1) * productionReplyTimeoutMs) / 1000 + | |
| artifactAnalysisWindowSeconds | |
| ) | |
| const processTimeoutMs = Math.max(30000, probeDurationSeconds * 1000 + 5000) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/probe-live-audio-controls.mjs` around lines 29 - 33, Update the
process timeout calculation alongside probeDurationSeconds so processTimeoutMs
is derived from the computed probe duration rather than hardcoded to 30 seconds.
Ensure the resulting timeout includes sufficient margin to exceed
probeDurationSeconds as liveCommands grows.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/lib/live-audio-control-protocol.mjs`:
- Around line 1-2: Update successfulLiveCommandReplies so the separator between
“ret:0” and “res:” matches only horizontal whitespace, never carriage returns or
newlines. Add a regression test covering intervening line content such as
“ret:0\nprogress=continue\nres:” and verify it is not counted as a successful
reply.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9afb5bb-dc96-49cf-b3d1-8b1193c060bd
📒 Files selected for processing (5)
scripts/lib/live-audio-control-protocol.mjsscripts/lib/live-audio-control-protocol.test.mjsscripts/lib/smoke-command-callers.test.mjsscripts/probe-live-audio-controls.mjsscripts/smoke-recording-native-preview-app.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/probe-live-audio-controls.mjs
| export function successfulLiveCommandReplies(output) { | ||
| return output.match(/\bret:0\s+res:/g) ?? [] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent the matcher from crossing line boundaries.
JavaScript \s+ matches \r and \n, so output such as ret:0\nprogress=continue\nres: is counted as a successful reply. Since scripts/probe-live-audio-controls.mjs uses this count to schedule subsequent commands, a false match can advance the protocol early. Restrict the separator to horizontal whitespace and add a regression test.
Proposed fix
export function successfulLiveCommandReplies(output) {
- return output.match(/\bret:0\s+res:/g) ?? []
+ return output.match(/\bret:0[^\S\r\n]+res:/g) ?? []
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function successfulLiveCommandReplies(output) { | |
| return output.match(/\bret:0\s+res:/g) ?? [] | |
| export function successfulLiveCommandReplies(output) { | |
| return output.match(/\bret:0[^\S\r\n]+res:/g) ?? [] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/live-audio-control-protocol.mjs` around lines 1 - 2, Update
successfulLiveCommandReplies so the separator between “ret:0” and “res:” matches
only horizontal whitespace, never carriage returns or newlines. Add a regression
test covering intervening line content such as “ret:0\nprogress=continue\nres:”
and verify it is not counted as a successful reply.
2 participants
Summary
Validation
Summary by CodeRabbit
Bug Fixes
Improvements