fix(recording): stop the WGC helper from hanging on stop - #254
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesWindows capture shutdown
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Recorder
participant ElectronIPC
participant NativeHelper
participant WgcSession
participant Encoders
Recorder->>ElectronIPC: request native stop
ElectronIPC->>NativeHelper: write stop and close stdin
NativeHelper->>WgcSession: quiesce capture callbacks
WgcSession-->>NativeHelper: callback drain result
NativeHelper->>Encoders: stop and finalize encoders
Encoders-->>NativeHelper: finalization result
NativeHelper-->>ElectronIPC: completion or failure diagnostics
ElectronIPC-->>Recorder: stop result and recording state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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)
electron/native/wgc-capture/src/main.cpp (1)
1103-1105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the
quiesceCapture()result at thewgc-quiescestep.
quiesceCapture()returnsfalsewhen a frame callback did not drain inside its 5000 ms default. The return value is discarded here, so the only evidence is theWARNINGline printed insideWgcSession::quiesceCapture. The laterwgc-session-closestep then silently skips the device release, becausestop()callsquiesceCapture()again and returns early. Record the drain outcome in the stop-timing log so a bug report shows which of the two shutdown shapes occurred.♻️ Proposed change
beginStopStep("wgc-quiesce", stepBudgetMs); - session.quiesceCapture(); - logStopStep("wgc-quiesce"); + const bool wgcQuiesced = session.quiesceCapture(); + std::cerr << "[stop-timing] step=wgc-quiesce elapsed_ms=" << stopElapsedMs() + << " drained=" << (wgcQuiesced ? "true" : "false") << std::endl;Note that
logStopStepkeeps the required leadingstep=<name> elapsed_ms=<n>shape, so the addeddrained=field stays additive.🤖 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 `@electron/native/wgc-capture/src/main.cpp` around lines 1103 - 1105, Capture the boolean result returned by WgcSession::quiesceCapture() in the wgc-quiesce shutdown step and pass it to logStopStep so the timing entry includes an additive drained= field while preserving the required step and elapsed_ms fields.
🤖 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 `@electron/ipc/handlers.ts`:
- Around line 2751-2759: Clear pendingCursorRecordingData in the failed-stop
cleanup path immediately after stopCursorRecording(), matching the existing
discard path behavior, so stale cursor samples cannot be written by the next
editable-overlay recording through writePendingCursorTelemetry.
In `@electron/native/wgc-capture/src/main.cpp`:
- Around line 1129-1137: Adjust the shutdown sequence around encoder.finalize()
and webcamEncoder.finalize() so the screen finalize cannot consume the entire
global shutdown budget before the webcam finalize starts. Reserve sufficient
remaining time for the optional webcam finalize, or emit the screen completion
result before initiating the webcam step, while preserving the existing success
and failure reporting.
In `@scripts/test-windows-wgc-helper.mjs`:
- Around line 48-51: Update scripts/test-windows-wgc-helper.mjs at lines 48-51
by setting STOP_HANG_LIMIT_MS above the helper’s 50-second ceiling and pinning
OPENSCREEN_WGC_STOP_BUDGET_MS in the child environment created by runHelper.
Update technical-documentation/architecture/recording.md at line 71 to document
the 50-second OPENSCREEN_WGC_STOP_BUDGET_MS ceiling and identify
OPENSCREEN_WGC_STEP_BUDGET_MS as the normal 8-second per-step bound.
In `@technical-documentation/architecture/recording.md`:
- Line 71: Update the shutdown-budget description in the Windows helper
documentation to state that OPENSCREEN_WGC_STOP_BUDGET_MS defaults to 50 seconds
and that the separate per-step OPENSCREEN_WGC_STEP_BUDGET_MS defaults to 8
seconds and normally terminates a wedged step. Preserve the existing discussion
of shutdown behavior and outstanding fixes.
---
Nitpick comments:
In `@electron/native/wgc-capture/src/main.cpp`:
- Around line 1103-1105: Capture the boolean result returned by
WgcSession::quiesceCapture() in the wgc-quiesce shutdown step and pass it to
logStopStep so the timing entry includes an additive drained= field while
preserving the required step and elapsed_ms fields.
🪄 Autofix
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 Plus
Run ID: cc610f8a-87ef-45d5-aaeb-6c4224e94f66
📒 Files selected for processing (10)
electron/ipc/handlers.tselectron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/wgc_session.cppelectron/native/wgc-capture/src/wgc_session.helectron/recording/nativeWindowsCaptureStop.test.tselectron/recording/nativeWindowsCaptureStop.tsscripts/test-windows-wgc-helper.mjssrc/hooks/useScreenRecorder.nativeStopFailure.test.tsxsrc/hooks/useScreenRecorder.tstechnical-documentation/architecture/recording.md
The helper's stop wait was gated on the frame mutex:
std::unique_lock lock(mutex);
control.cv.wait(lock, [&]{ return control.stopRequested.load(); });
`stopRequested` is an atomic with no relationship to what that mutex
protects, but `condition_variable::wait` has to re-acquire it before it
can return -- and that mutex is held across uninterruptible D3D11 work:
the WGC frame callback's CopyResource, and the video writer's
Map(D3D11_MAP_READ) readback. One stalled driver call and the main thread
never came back, before the first [stop-timing] line was ever printed.
That is why issue #252 arrived with an empty diagnostic log.
Give stop its own mutex/CV pair that no frame thread ever touches, route
all nine stop sites through requestStop(), and bound the wait. Then bound
the shutdown itself: every step after the wait calls into a driver or
joins a thread that does, so decoupling the wait alone would only have
moved the hang. Each step gets a deadline under a global ceiling, and a
watchdog force-exits the process naming the step it died in.
Report success as soon as the MP4 index is written rather than at the end
of the process's life, so a wedged GPU teardown no longer costs a
recording that is already complete on disk. Check what finalize()
returns while doing it.
On the app side: keep a listener on the helper for the whole recording so
its diagnostics reach the bug report instead of being dropped between
start and stop, short-circuit a helper that already exited rather than
burning the timeout on a 'close' that can never arrive, and let discard
escape a wedged helper immediately.
The follow-on "Native Windows capture is not running." was separate: the
main process releases its helper handle unconditionally, the renderer did
not, so the next Record click sent a second stop. The same gap existed on
macOS and Linux.
The underlying stall is untouched -- the readback still runs inside the
frame lock, and the D3D adapter is still whichever one Windows hands us
on a four-adapter machine. What changes is that neither can hang the app.
Refs #252
…ture Review follow-up. The single `Recording stopped. Output path:` line was gated on `screenFinalized && webcamFinalized`, and the app treats that line as the only proof a recording is worth keeping. So an optional second file could veto a complete one: `webcamEncoder.finalize()` returning false discarded a perfectly indexed screen MP4, which was a regression against the previous behaviour where its result was ignored entirely. The same gate had a second way to fire. Both finalize steps clamp their deadline to the shared global ceiling, so a screen finalize that spent most of it left the webcam step already past its deadline, and the shutdown watchdog killed the process before the announcement ran. Announce after the screen finalize and before the webcam's, gated on the screen file alone. A failed webcam finalize is now an ERROR on stderr and a non-zero exit, which is what it always should have been -- not a lost recording. Also from the review: - Log the `quiesceCapture()` drain outcome at `wgc-quiesce`. It decides whether `wgc-session-close` releases the device or skips it, so a report that omits it cannot be read. - Keep `phase=` when parsing `[stop-timing]`. The diagnostic tool discarded it, which is the one field naming the step that hung, and the summary listed every step twice because begin and end lines both matched. Same double-count in the helper harness. - Pin `OPENSCREEN_WGC_STOP_BUDGET_MS` into the harness's child env and derive its hang limit from it. The limit was 30s against a 50s ceiling, so a long software-encoder finalize -- the case issue #34 exists for -- would have been killed and reported as the #252 hang. - Correct the shutdown budgets in the architecture doc: 8s per step, 50s overall, not 10s. - Clear `pendingCursorRecordingData` on the failed-stop path, matching the discard path.
The job filtered on `main`, so retargeting this PR at `release/v1.9.0` removed the artifact its own testing steps tell a reviewer to download. A recording fix aimed at a release is precisely when someone needs the compiled helper without a local MSVC toolchain.
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Review — with Windows 11 verification
I ran the two gates the PR body says are outstanding, on real Windows 11 hardware, using the helper built by this PR's own Diagnostic artifact job (verified at the binary level that it carries OPENSCREEN_WGC_TEST_STALL_READBACK_MS, phase=abandoned and stop-timeout).
Short version: the analysis holds up and both gates pass. One behaviour regression worth fixing before merge, plus two one-liners.
Windows 11 results
| Test | Result |
|---|---|
node scripts/test-windows-wgc-helper.mjs |
✅ stop 305 ms, 1.24 MB playable 5.03 s H.264 |
--system-audio |
✅ 227 ms, 2 streams |
--software-encoder |
✅ 194 ms |
--capture-cursor |
✅ 196 ms |
--stall-readback (the #252 gate) |
✅ helper self-exits in 13.1 s, phase=abandoned step=video-writer-join |
Real .exe × real waitForNativeWindowsCaptureStop, success path |
✅ {ok:true} in 185 ms, 1.77 MB |
Real .exe × real module, wedged frame lock |
✅ {ok:false, reason:"stop-timeout", message:"The recorder stalled while shutting down (video-writer-join)."} in 13.07 s, exited:true |
| New suites (20 tests) | ✅ 20/20 |
Full vitest --run |
1637 passed, 6 failed, 3 skipped |
tsc --noEmit ×2, biome check |
✅ clean |
Notes on that table:
- The 6 failures are pre-existing. All in
electron/recording/webm-seek-index.test.ts, and they fail identically at the merge-base (1749d849) on Windows. Unrelated to this PR. - The 13.1 s is exactly the designed budget: ~5 s
quiesceCapturedrain timeout + 8 svideo-writer-joinstep budget. Not a coincidence, which is a good sign the model in the PR body is the real one. - Beyond the harness, I wired the real compiled helper to the real
waitForNativeWindowsCaptureStop(the seam the unit tests fake) for both the success and wedged cases — that's the third and fourth rows from the bottom. The wedged run leaves a 0-byte stub, which the newonlyIfUnusablesize gate deletes. - I checked the "verified to fail pre-fix" claim independently: grafting
useScreenRecorder.nativeStopFailure.test.tsxonto the merge-base gives 2/3 failures, withrecordingstayingtrueafter a failed stop. Real guard, not vacuous.
Not covered: the app UI round trip (testing items 4 and 5). My screenshot tooling applies SetWindowDisplayAffinity(WDA_EXCLUDEFROMCAPTURE) to the dev Electron window and re-applies it on every capture, so I could not see the window to drive it. That is the one remaining gap — though the renderer half is covered by the test above, which renders the real hook.
Findings
1. A webcam finalize failure now discards the screen recording too — Medium, worth fixing before merge
electron/native/wgc-capture/src/main.cpp:1150
if (!encodeFailed && screenFinalized && webcamFinalized) { … "Recording stopped. Output path: " … }Before this PR, webcamEncoder.finalize()'s return was ignored and the line always printed. Now a webcam-only finalize failure suppresses the single line the app treats as proof of a good recording. settleFromOutput then returns helper-failed, the user is told the recording failed, and the complete, indexed screen MP4 survives on disk only because it happens to clear the 64 KB size gate — with no way to reach it from the UI.
CodeRabbit found the same defect from the other end (both finalize steps clamp to the same absolute ceiling, so the webcam step can start already past its deadline). That is the rarer trigger; a plain false return is the likely one. Fix once, at the reporting site: emit the screen path on screenFinalized, and degrade a webcam finalize failure to a WARNING: with webcamPath omitted.
2. The diagnostic tool drops phase= — Minor, one line
scripts/diagnostic-tool/diagnostic.mjs:152 matches /\[stop-timing\]\s+step=(\S+)\s+elapsed_ms=(\d+)/.
phase=abandoned naming the culprit step is the headline new signal, and the structured stopTiming array in the bug-report JSON discards it. Raw helperStderr still carries it, but the summary printer at :305 iterates stopTiming — so the console summary now prints every step twice (begin + completion) and never says which one died. (?:\s+phase=(\S+))? fixes it.
Same root cause in the harness: readStopTimingSteps doesn't filter phase=begin, so stopTimingSteps in the success JSON lists each step twice. Visible in my run:
"stopTimingSteps": ["command-received","wgc-quiesce","wgc-quiesce","microphone","microphone", …]
3. Harness budget mismatch — CodeRabbit is right
scripts/test-windows-wgc-helper.mjs:48-51 says "The helper's own shutdown budget is 10s" and sets STOP_HANG_LIMIT_MS = 30_000; the actual default is OPENSCREEN_WGC_STOP_BUDGET_MS = 50000. encoder-finalize is the one step given the whole ceiling — precisely the slow-software-encoder case issue #34 exists for. A long --software-encoder recording could be killed by the harness at 30 s and reported as a #252 regression. Either pin OPENSCREEN_WGC_STOP_BUDGET_MS in the child env or raise the limit above 50 s. Same stale "10s" in technical-documentation/architecture/recording.md:71.
4. Minor
reasonis returned on the stop IPC result but the renderer only readsresult.error. Dead field on the surface unless something else is meant to consume it.- Pre-existing, but worth a comment while you're in here:
requestStop()notifiescontrol.cvwithout holdingmutex, and the 10 s first-frame wait checksstopRequestedundermutex. A notify landing between the predicate check and the enqueue is lost — bounded to a 10 s delay bywait_for, unchanged from before, but this file now reasons carefully about exactly this class of thing. - CodeRabbit's
quiesceCapture()return-value logging nit is worth taking. It is the difference between "drained" and "leaked the device", and today only aWARNING:line distinguishes them.
5. CodeRabbit's pendingCursorRecordingData finding — not reachable
Its stale-samples scenario needs a subsequent recording that doesn't clear the buffer, but every start does: startCursorRecording nulls it at electron/ipc/handlers.ts:1042, and the non-overlay branch nulls it at :2401. The only real cost is holding samples in memory between recordings. Harmless to add for symmetry — just not a data-integrity bug.
Things that are right and easy to miss
session.stop()moved afterencoder.finalize(). The old order released the D3D device before finalizing. Strictly safer now.hasExited()before registering the'close'listener. Node never re-emitsclose; without this you burn the whole 60 s timeout waiting for an event that cannot arrive.InFlightGuardoutlivingframe.Close()— the ordering the comment claims is what the language actually does, and it is the difference between a clean quiesce and closing a frame pool under a live handler.- Lock order is one-directional (frame mutex →
stopMutex), andwaitForStop()never touches the frame lock. No deadlock introduced. \rstripping inreadCaptureCommands— the one command that must never be silently dropped.- The
!firstFrameArrivedearly return is safe:startVideoWriter()runs after that wait, sovideoWriterThreadisn't joinable there andreturn 1can't hitstd::terminate.
Risks
- Discard now kills instead of finalizing. Right call, but if the helper survives
taskkill /T /F,fs.rmgets EBUSY and the stray file stays (warned, not fatal). Same for the orphan keeping the "screen is being shared" indicator up until app exit — thebefore-quithook you deferred is the right place for that. - Finding 1 is the only real behaviour regression I found on the "does a normal stop still work" axis, and it only bites recordings with a separate webcam file. Everything else I could exercise on hardware behaved as described.
Verdict
Approve after finding 1. The root-cause analysis holds up, the containment is measured and works on real Windows 11 hardware, and the new tests are genuine regression guards. Finding 1 is a ~5-line change to where the success line is emitted; 2 and 3 are one-liners.
The PR's "Medium until item 2 passes that a normal stop is unaffected" can be upgraded: five configurations stop in 194–305 ms with playable output, on Windows 11.
🤖 Reviewed with Claude Code
6842c03 to
50715df
Compare
Rebased on
|
6aca90ba |
the original fix, unchanged |
50715df6 |
review fixes |
7272ebdf |
CI: build the diagnostic bundle for release branches |
What changed in 50715df6
The one that mattered — a webcam finalize could discard the screen recording. The Recording stopped. Output path: line was gated on screenFinalized && webcamFinalized, and the app treats that line as the only proof a recording is worth keeping. webcamEncoder.finalize() returning false therefore threw away a complete, indexed screen MP4 — a regression against the pre-PR behaviour, where the webcam result was ignored entirely and the line always printed. CodeRabbit found the same gate from the budget side (both finalize steps clamp to the same absolute ceiling, so a slow screen finalize can leave the webcam step already past its deadline and get the process killed before the announcement runs).
Reserving a budget margin only fixes the second trigger, so the announcement moved instead: it now runs after the screen finalize and before the webcam's, gated on the screen file alone. A failed webcam finalize is an ERROR: on stderr and a non-zero exit — the app has already read the success line and keeps the recording, which is the right trade for an optional second file. Pinned by keeps the screen recording when only the webcam failed to finalize.
The rest, all from the review:
quiesceCapture()'s drain outcome is logged atwgc-quiesce(the nitpick). It decides whetherwgc-session-closereleases the device or skips it, so a report that omits it can't be read. Nowdrained=true|false, additive afterelapsed_ms=so the leading shape the diagnostic tool matches is untouched.phase=survives parsing.scripts/diagnostic-tool/diagnostic.mjsdropped it — the single field naming the step that hung, in the tool whose whole job is producing a [Bug]: v1.8.0 native Windows recorder still hangs on stop; next attempt says capture is not running #252 bug report. Its summary also printed every step twice, because thephase=beginand completion lines both matched. Same double-count in the helper harness'sreadStopTimingSteps; both now keep completions only (andphase=abandoned, which is an ending, just a bad one).OPENSCREEN_WGC_STOP_BUDGET_MSis pinned into the harness's child env andSTOP_HANG_LIMIT_MSderived from it. It was 30 s against a 50 s ceiling, andencoder-finalizeis the one step allowed to spend that ceiling in full — so a long software-encoder finalize, the exact case issue [Bug]: I need help pls on windows 10 #34 exists for, would have been killed and reported as the [Bug]: v1.8.0 native Windows recorder still hangs on stop; next attempt says capture is not running #252 hang.- Shutdown budgets corrected in the architecture doc: 8 s per step (the bound that normally fires), 50 s overall, with the issue [Bug]: I need help pls on windows 10 #34 reason for the finalize allowance attached so the number isn't mistaken for arbitrary.
pendingCursorRecordingDatacleared on the failed-stop path. Worth doing, but the stated reason doesn't hold — see the reply on that thread: every recording start already clears the buffer, so the samples were never reachable by a later recording. What it actually buys is not holding a lost take's telemetry in memory until then, plus symmetry with the discard path. The comment says so, to stop someone later removing thestartCursorRecordingclear thinking this one covers it.
7272ebdf
diagnostic-artifact.yml filtered on main, so retargeting this PR at release/v1.9.0 silently removed the artifact the testing section tells a reviewer to download. Now [main, "release/**"] — the bundle is back on this PR's Checks tab, built from 50715df6.
Re-verified on Windows 11 against the rebuilt helper
CI-built from 50715df6, not the earlier binary:
| normal stop | ✅ 2.38 MB playable 5.03 s H.264 |
--software-encoder |
✅ 352 ms |
--webcam |
✅ 660 ms, both streams 4.93 s, webcam-encoder-finalize present — the reordered path |
--stall-readback (#252 gate) |
✅ self-exits 13.1 s, phase=abandoned step=video-writer-join |
stopTimingSteps |
✅ each step once, no more entry/exit pairs |
wgc-quiesce |
✅ drained=true, whole shutdown 59 ms |
Locally: tsc --noEmit ×2, biome check (0 findings on changed files), docs:check, i18n:check, and 60/60 across electron/recording + src/hooks. All 16 PR checks green, including the Windows diagnostic bundle.
One note: CodeRabbit reports "Review skipped: reviews are disabled for this base branch", so the replies above won't get a bot follow-up on release/v1.9.0.
The gap from my earlier review is unchanged: the in-app Record → Stop round trip (testing items 4 and 5) still hasn't been driven by hand.
🤖 Generated with Claude Code
Every RC of a line shipped the same release body. The notes start tag was derived from the stable version, so v1.9.0-rc.1 and v1.9.0-rc.2 both spanned v1.8.0..<tag> — rc.2 just repeated rc.1's list plus its own few entries, and v1.8.0-rc.8 and rc.9 came out byte-identical. Testers had no way to see what a re-cut actually changed, which is the one question an RC body has to answer. Resolve the previous RC of the same line instead, walking down from the current rc number so a skipped or failed RC doesn't break the chain. rc.1 still falls back to the previous stable, and stable releases are untouched. Build the RC body from `git log` rather than --generate-notes. GitHub's generator lists only the PRs it manages to associate and silently drops real ones: #254 and #261 were merged into release/v1.9.0 yet never appeared in v1.9.0-rc.2's body, so an RC could omit the very fix it was cut for. The commit range is the actual diff. Stable releases keep --generate-notes — they are the public-facing ones and want the PR links and the New Contributors section. Needs fetch-depth: 0 on the publish job's checkout for the tags and history.
Every RC of a line shipped the same release body. The notes start tag was derived from the stable version, so v1.9.0-rc.1 and v1.9.0-rc.2 both spanned v1.8.0..<tag> — rc.2 just repeated rc.1's list plus its own few entries, and v1.8.0-rc.8 and rc.9 came out byte-identical. Testers had no way to see what a re-cut actually changed, which is the one question an RC body has to answer. Resolve the previous RC of the same line instead, walking down from the current rc number so a skipped or failed RC doesn't break the chain. rc.1 still falls back to the previous stable, and stable releases are untouched. Build the RC body from `git log` rather than --generate-notes. GitHub's generator lists only the PRs it manages to associate and silently drops real ones: #254 and #261 were merged into release/v1.9.0 yet never appeared in v1.9.0-rc.2's body, so an RC could omit the very fix it was cut for. The commit range is the actual diff. Stable releases keep --generate-notes — they are the public-facing ones and want the PR links and the New Contributors section. Needs fetch-depth: 0 on the publish job's checkout for the tags and history.
Every RC of a line shipped the same release body. The notes start tag was derived from the stable version, so v1.9.0-rc.1 and v1.9.0-rc.2 both spanned v1.8.0..<tag> — rc.2 just repeated rc.1's list plus its own few entries, and v1.8.0-rc.8 and rc.9 came out byte-identical. Testers had no way to see what a re-cut actually changed, which is the one question an RC body has to answer. Resolve the previous RC of the same line instead, walking down from the current rc number so a skipped or failed RC doesn't break the chain. rc.1 still falls back to the previous stable, and stable releases are untouched. Build the RC body from `git log` rather than --generate-notes. GitHub's generator lists only the PRs it manages to associate and silently drops real ones: #254 and #261 were merged into release/v1.9.0 yet never appeared in v1.9.0-rc.2's body, so an RC could omit the very fix it was cut for. The commit range is the actual diff. Stable releases keep --generate-notes — they are the public-facing ones and want the PR links and the New Contributors section. Needs fetch-depth: 0 on the publish job's checkout for the tags and history.
1 participant
Summary
Stopping a native Windows recording could hang forever. The helper's stop wait was gated on the frame mutex:
stopRequestedis an atomic with no relationship to what that mutex protects, butcondition_variable::waitmust re-acquire it before it can return — and that mutex is held across uninterruptible D3D11 work (the WGC callback'sCopyResource, the video writer'sMap(D3D11_MAP_READ)). One stalled driver call and the main thread never returns, before the first[stop-timing]line is printed. That is why #252 arrived with an empty diagnostic log.This PR:
requestStop(), and bounds the wait.OPENSCREEN_WGC_STOP_BUDGET_MS, 50s), and a watchdog force-exits naming the step it died in.finalize()returns while doing it.What this does not fix
The underlying stall is untouched: the GPU readback still runs inside the frame lock (no
D3D11_MAP_FLAG_DO_NOT_WAIT, no timeout), andD3D11CreateDevice(nullptr, ...)still takes whatever adapter Windows hands us — on the reporter's four-adapter machine that may well be a virtual display driver. What changes is that neither can hang the app. A stall now produces a fast, named failure instead of a 60s freeze and a 0-byte MP4.Prior art worth knowing: commit
190fdc9f(PR #121) was this exact structural fix, closed unmerged 25 seconds after #123 merged. #119's stated premise was the software encoder, which #252's log explicitly rules out ("video":"default","preferSoftwareEncoder":false).Related issue
Refs #252
Deliberately not
Fixes— see Confidence below. #115 was closed by a fix that did not hold.Type of change
Release impact
Desktop impact
macOS/Linux are touched only by the renderer state fix, which they shared byte for byte.
Testing
Already done (Linux CI-equivalent)
npm run test— 1645 passing, including 20 new tests across two new suites.npx tsc --noEmitandnpx tsc -p tsconfig.test.json --noEmit.npm run lint,npm run docs:check,npm run i18n:check.CaptureControlsource was extracted verbatim and compiled withg++ -std=c++20 -Wall -Wextra, then run: with the frame lock held forever, the new design observes the stop in 300 ms; the old design never returns even with the flag set. That demonstrates the root cause and the fix, but it is not a substitute for building the real helper.Needed on Windows — this is the gate
The C++ has never been compiled: it was written on Linux with no MSVC, no Windows SDK, no WinRT. The reporter is on Windows 10 22H2 with an AMD iGPU + RTX 5070 Ti + two virtual display adapters (Todesk, GameViewer). Windows 11 is a good proxy for the code paths but not for the driver stall — the only version-sensitive APIs here are pre-existing and already guarded (
IsBorderRequiredis Win11-only and wrapped;IsCursorCaptureEnabledneeds Win10 2004+;CreateFreeThreadedsince 1809). Everything this PR adds is plain STL + core Win32.1. It compiles. ✅ Already confirmed — the
Windows x64 diagnostic bundlejob built the helper with MSVC onwindows-latestin 2m16s.No Visual Studio needed to test. That same job publishes the compiled helper as an artifact, so a Windows 11 machine can test without a toolchain. From this PR's Checks tab → Diagnostic artifact → the run summary → Artifacts →
openscreen-diagnostic-windows-x64. Unzip it and point the harness athelpers/win32-x64/wgc-capture.exe. (Take the artifact from the newest run if more commits land here.)Everything below then runs with just Node. (
--stall-readbackneeds nothing else; the full run also wantsffmpeg/ffprobeonPATHfor the stream probe.) Building locally withnpm run build:native:winworks too if the toolchain is there.2. No regression on a normal stop. The highest-risk part of this change: the shutdown was reordered, the success line moved earlier, a lock was removed.
Expect: exit 0, a playable MP4, and in the JSON summary
stopLatencyMswell under a second plus astopTimingStepsarray containingcommand-received,wgc-quiesce,video-writer-join,encoder-finalize,wgc-session-close. The run now fails loudly if the helper never acknowledges the stop, skips a shutdown step, or exceeds a 15s stop budget.Worth repeating with
--window,--webcam,--system-audio,--microphone,--software-encoder— the webcam path adds a second finalize step, and the software encoder is the slow-finalize case the budgets are sized for.3. The hang can no longer happen. This is the #252 regression test, and it needs no exotic hardware — a new env-gated injector holds the frame mutex exactly the way a wedged readback does:
Expect: the helper exits on its own within ~8s, stderr carries
[stop-timing] step=<name> elapsed_ms=<n> phase=abandoned, and stdout carries{"event":"stop-timeout","schemaVersion":2,"step":"..."}.--stall-readbackflag are both introduced by this PR, so onmainthe flag is simply ignored and the test passes normally — that proves nothing. To get a genuine A/B on Windows, graft the injector ontomainand rebuild: add thereadEnvInthelper and theOPENSCREEN_WGC_TEST_STALL_READBACK_MSread near the top ofmain(), plus the guardedstd::this_thread::sleep_forimmediately beforeif (latestFrameTexture)insidewriteVideoFrames' lock scope (three small hunks, all visible in this diff). The old helper then stays alive afterstopwith zero[stop-timing]output — the exact signature in #252.Without that, the "before" behaviour rests on the design argument above plus the Linux
g++demonstration, not on a Windows measurement. Worth knowing before reading a green run as a proof.4. The follow-on error is gone. In the app, with the injector set so the stop fails:
Record → Stop. Expect a prompt error toast (not a 60s freeze), then the record button starts a new recording rather than answering "Native Windows capture is not running." Also check the 0-byte MP4 is not left behind in the recordings folder, and that Cancel/Restart return immediately instead of blocking.
5. Sanity on the other platforms. Record → stop → editor opens, on macOS and Linux. The renderer change touches their failure paths.
If it still reproduces for the reporter
That is now a useful outcome. Ask them for the
[stop-timing]lines:step=command-receivedabsent → the helper never received the stop; look at the IPC, not the helper.command-receivedpresent, nothing after → the stop wait itself is still blocked, which would refute the analysis in this PR.phase=abandoned step=X→ X is the culprit, named for the first time.Their original report could not distinguish any of these, which is why this survived #119 and #123.
Follow-ups deliberately left out
Each is real, and each is wider than the reported bug:
IDXGIAdapter::GetDesc().Descriptionat startup. ~10 lines, no behaviour change, and it would immediately settle whether adapter 0 is a Todesk/GameViewer virtual display on the reporter's machine.nullptr. The strongest candidate for the actual root cause, but unproven and needs Windows validation.D3D11_MAP_FLAG_DO_NOT_WAITwith a bounded retry.before-quithook to kill orphaned helpers.Confidence
Honest split. High that the hang cannot recur and that the "not running" error is gone (the latter is covered by tests and is OS-independent React + IPC). Medium until item 2 above passes that a normal stop is unaffected. Low that the reporter ends up with a working recording — this converts an unbounded hang into a bounded, diagnosed failure, and if their driver stalls every time they will still lose the capture, just quickly and with an explanation.
We have proof the main thread never returned from the wait. We have no observation of which thread held the lock or why; the blocking
Mapis the leading candidate by elimination, not by measurement.🤖 Generated with Claude Code