Conversation
…not lost waitUntilCapturing() installed its resolver only when called, while its sibling waitUntilSourceSelected() latches the answer it already has. The start handler arms the helper and awaits capturing afterwards, so the helper's capture-started can already have been parsed — both events arrive in one stdout chunk and NdjsonLineReader dispatches them back to back within a single data callback, leaving no point at which a caller could install its handler. When that happens the resolve call finds startedResolve null, the answer is dropped, and the promise never settles: the recording is running and the file is filling while start-native-linux-recording waits forever, with no timeout by design. Latch it the same way source-selected is latched.
A helper that died after its first frame has stopped recording, so the latch must not answer for it: order the process check first and pin the boundary with a test that fails when the latch is read first.
…pture waits Adversarial verification of the capture-started latch: the undeferred session, a second wait after the event already answered the first, and a non-fatal error arriving after the first frame all reach the latch and had no test. Each fails on base as a 15s hang. The control pins the latch's initial false so the three above cannot pass for the wrong reason.
VerificationIndependent adversarial verification at head Production source is byte-identical to the reviewed head A/B for every test on the branchBase = $ npx vitest --run electron/native-bridge/capture/linuxNativeCaptureSession.test.ts # BASE 520f6e5e
✓ sends no restore token to the helper 9ms
✓ reports the source kind the portal granted 5ms
✓ distinguishes a granted monitor from a granted window 5ms
✓ leaves the granted kind unknown when the portal does not report one 5ms
✓ resolves the source selection before any frame is captured 3ms
✓ asks the helper to defer, and arms it only when told to 3ms
✓ arms at most once, so a caller need not track whether it prepared 1ms
✓ does not ask the helper to defer unless it was configured to 1ms
✓ rejects a pending source selection when the helper dies 5ms
✓ resolves the source selection immediately once it has already arrived 3ms
✓ knows the granted kind by the time the capture is confirmed running 1ms
× resolves the capture wait immediately once the first frame already landed 15009ms
× resolves the capture wait when arming and the first frame share a stdout chunk 15006ms
✓ still rejects the capture wait when the helper died before any frame 3ms
✓ rejects the capture wait when the helper died after its first frame 2ms
× resolves the capture wait on an undeferred session whose first frame already landed 15001ms
× resolves a second capture wait after the first was answered by the event 15005ms
× resolves the capture wait after a non-fatal error follows the first frame 15003ms
✓ (control) leaves the capture wait pending until the first frame lands 6ms
⎯⎯⎯⎯⎯⎯⎯ Failed Tests 5 ⎯⎯⎯⎯⎯⎯⎯
Error: Test timed out in 15000ms.
Tests 5 failed | 14 passed (19)
Duration 75.95s
$ npx vitest --run electron/native-bridge/capture/linuxNativeCaptureSession.test.ts --reporter=verbose # HEAD b57cbec2
✓ ... resolves the capture wait immediately once the first frame already landed 2ms
✓ ... resolves the capture wait when arming and the first frame share a stdout chunk 2ms
✓ ... still rejects the capture wait when the helper died before any frame 2ms
✓ ... rejects the capture wait when the helper died after its first frame 3ms
✓ ... resolves the capture wait on an undeferred session whose first frame already landed 2ms
✓ ... resolves a second capture wait after the first was answered by the event 2ms
✓ ... resolves the capture wait after a non-fatal error follows the first frame 2ms
✓ ... (control) leaves the capture wait pending until the first frame lands 4ms
Test Files 1 passed (1)
Tests 19 passed (19)
Duration 1.08sAll 5 base failures are 15s timeouts, not assertion failures — the promise never settles. That is the hang itself, and it is why the base arm takes 76s against 1.08s fixed. Tests added by this verification (rows the ledger had unpinned)Each was run against base individually before being committed:
Mutation arms — each control fails inside what it claims to protect$ # ARM A — `if (this.capturing)` hoisted ABOVE `if (!this.process)` (the ordering commit 3fc1210d reversed)
× rejects the capture wait when the helper died after its first frame 28ms
AssertionError: promise resolved "undefined" instead of rejecting
Tests 1 failed | 3 passed | 11 skipped (19)
$ # ARM B — `this.capturing = true;` DELETED from the capture-started case, field and read kept
$ grep -c "this.capturing" electron/native-bridge/capture/linuxNativeCaptureSession.ts
1
× resolves the capture wait immediately once the first frame already landed 15021ms
× resolves the capture wait when arming and the first frame share a stdout chunk 15008ms
✓ still rejects the capture wait when the helper died before any frame 8ms
✓ rejects the capture wait when the helper died after its first frame 3ms
Tests 2 failed | 2 passed | 11 skipped (19)
$ # ARM C — `private capturing = true` (latch initialised open)
× (control) leaves the capture wait pending until the first frame lands 28ms
AssertionError: expected 'settled' to be 'pending' // Object.is equality
Tests 1 failed | 18 skipped (19)Arm A confirms the ordering commit is load-bearing and that test 4 is a real control, independently of the transcript the body quoted. Arm B is the one that matters most: with the latch write removed the discriminating tests hang again, so they pin the fix rather than merely the existence of the read. Arm C confirms the new control is not decoration. Behaviour outside the stated bug
Toolchain at this head$ npx biome check electron/native-bridge/capture/linuxNativeCaptureSession.ts electron/native-bridge/capture/linuxNativeCaptureSession.test.ts
Checked 2 files in 130ms. No fixes applied.
$ npx tsc --noEmit
rc=0
$ npx tsc -p tsconfig.test.json --noEmit
rc=0
Verdict: verified. The claim holds under every arm attempted, the ledger is complete at 16 rows with one documented-unreachable row, and the PR body has been reconciled to this head. |
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: APPROVE — ready for the operator to submit; no blocking issues found.
I reviewed the 220-line diff, independently traced the base failure through waitUntilCapturing() and its production caller, and checked the candidate evidence, boundary ledger, policy excerpts, commit messages, and upstream prior-art searches. On base, electron/native-bridge/capture/linuxNativeCaptureSession.ts:228-235 always installs a fresh resolver after the liveness check, while the event handler's this.startedResolve?.() drops an already-arrived event when no waiter exists. The changed lines correctly preserve that arrival as state:
electron/native-bridge/capture/linuxNativeCaptureSession.ts:79—private capturing = false;
electron/native-bridge/capture/linuxNativeCaptureSession.ts:230-239— the existing!this.processrejection remains beforeif (this.capturing) { return Promise.resolve(); }.
electron/native-bridge/capture/linuxNativeCaptureSession.ts:432—this.capturing = true;
That ordering is important and is covered by the added post-frame-exit control: a dead helper still rejects rather than reporting a stale recording as live. The added tests also distinguish base from head for the early event, same-stdout-chunk, non-deferred, repeated-wait, and post-error paths.
What's good: the patch is narrowly scoped to the dropped-event invariant, adds no dependency or unrelated refactor, and the PR body supplies reproducible before/after evidence and a substantive boundary ledger. Current fork CI is green for test, lint, both TypeScript checks, build, and platform bundle jobs. The sole failed Validate PR title (semantic) check is expected for the fork-only [oss-candidate] marker; the stated suggested upstream title follows the upstream convention. My repeated upstream searches found no open PR duplicating this latch fix.
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).
Verdict: no blocking correctness or maintainer-readiness issues found; this is a small, well-pinned fix for a reachable dropped-event race.
Maintainer's-eye read
I independently confirmed the base-code failure mechanism. waitUntilCapturing() previously installed startedResolve only when called, while the capture-started handler had no durable state. An event emitted after arm() but before the wait is installed is therefore dropped, and the later promise cannot settle. The changed code fixes that at electron/native-bridge/capture/linuxNativeCaptureSession.ts:230-240:
if (!this.process) { return Promise.reject(new Error("The Linux capture helper is not running.")); } ... if (this.capturing) { return Promise.resolve(); }
The liveness-before-latch ordering is the right deliberate asymmetry with waitUntilSourceSelected(): after capture-started, the exit handler clears process, and reporting a still-running capture then would be false. The changed test at electron/native-bridge/capture/linuxNativeCaptureSession.test.ts:358-380 pins that case. The latch starts closed at linuxNativeCaptureSession.ts:79 and is set only on the changed capture-started path at linuxNativeCaptureSession.ts:429; the pending-before-first-frame control at test lines 478-501 prevents an initially-open or unconditional shortcut.
I rebuilt the changed-condition ledger: initial capturing=false, live/dead process, and capturing false/true at the waiter all have defined results. The reachable cases are covered: event-before-wait, same-stdout-chunk delivery, waiter-before-event, repeat waiter, deferred and non-deferred sessions, non-fatal error, and helper exit before/after the first frame. The new assertions exercise waitUntilCapturing() rather than directly testing a field or helper, and would fail if the latch read or write were removed.
The implementation follows a local established idiom: waitUntilSourceSelected() already retains an arrived event for late observers. Recent upstream history for this module is likewise narrow, behavior-focused capture work (for example 0dfa6510, fix(recording): record the window Linux users actually pick), which supports this one-state, no-dependency scope. I also re-ran prior-art searches for waitUntilCapturing and capture-started; neither surfaced a competing implementation or issue. The [oss-candidate] prefix is a staging marker rather than upstream title style and should be removed for operator submission, not treated as a fork-PR code finding.
The manual computer-use E2E noted in the PR is not a blocker for this main-process event-latching change: deterministic unit coverage and the fork's Test, Lint, Type Check, Typecheck (tests), Build, and platform bundle CI jobs are passing. The only non-green check is the expected semantic-title validation caused by the staging prefix.
What's good: the patch is tightly scoped, reuses the sibling latch idiom, retains the necessary post-event failure behavior, and includes discriminating race tests plus meaningful controls.
SECOND READ: READY
Upstream evidence
- Module history:
getopenscreen/openscreencommit0dfa6510(fix(recording): record the window Linux users actually pick) demonstrates the maintainers' existing narrow, evidence-led capture-fix style. - Recent merged capture PRs include getopenscreen#508 (
fix(capture): zero-copy dmabuf→VAAPI capture) and getopenscreen#517 (fix(macos): exclude recording controls from display captures), likewise scoped by platform and failure mode. - Prior-art searches re-run:
gh search prs --repo getopenscreen/openscreen "waitUntilCapturing"andgh search issues --repo getopenscreen/openscreen "capture-started"; neither returned a competing latch fix.
SECOND READ: READY
|
Submitted upstream for review. |
|
Submitted upstream for review. |
3 participants
Summary
LinuxNativeCaptureSession.waitUntilCapturing()installed its resolver only at call time and had no "already arrived" latch, while its siblingwaitUntilSourceSelected()has exactly that latch. Thecapture-startedhandler resolves through an optional chain (this.startedResolve?.()), so when nothing is waiting yet the answer is silently dropped.electron/ipc/handlers.ts:2497-2498) doessession.arm(); await session.waitUntilCapturing();— the await lands on a later microtask, and the helper'ssource-selected+capture-startedarrive in one stdout chunk thatNdjsonLineReaderdispatches back to back inside a singledatacallback. There is no point at which the caller can install its handler in between.start-native-linux-recordingnever returns, and this path deliberately has no timeout ("No timeout, on purpose").capturingfield, a latch read inwaitUntilCapturing()placed after the liveness check, and setting the latch in thecapture-startedcase. No new dependencies, no behaviour change on any other path.The failure mode on base is a hang, not an assertion: the promise never settles, so vitest kills each test at its 15s timeout. That is the bug reproduced exactly. Five 15s timeouts are why the base arm takes 76s and the fixed arm takes 1s.
Upstream
getopenscreen/openscreenmain520f6e5e2afd025b17a3e32e8bc3bef213fc2676fix/linux-capture-started-latch, headb57cbec205e61b94bb58c6434d99d727a583f60eelectron/native-bridge/capture/linuxNativeCaptureSession.tswaitUntilCapturing()(:229)case "capture-started"arm ofhandleEvent()(:428)electron/native-bridge/capture/linuxNativeCaptureSession.test.ts(19 tests, 8 added by this PR)electron/ipc/handlers.ts:2497-2498Production diff is 9 added lines in one file. Commits:
433f73ef(the latch),3fc1210d(ordering — liveness check ahead of the latch),b57cbec2(verification tests only, no production change).Bug
Trigger. On Linux,
start-native-linux-recordingarms the PipeWire helper and then awaitswaitUntilCapturing().arm()is synchronous (it writesrecord\nto the helper's stdin); theawaitruns on a later microtask. The helper emitssource-selected(main.rs:980) and thencapture-started(main.rs:907) as soon as the first frame stages. Node delivers whatever has accumulated on the pipe as onedatachunk, andNdjsonLineReader.push()loops over every complete line in that chunk, callinghandleEventsynchronously for each. Socapture-startedcan be processed before the caller'sawaitinstallsstartedResolve.Wrong outcome.
case "capture-started"callsthis.startedResolve?.(). With no resolver installed the optional chain makes this a no-op and the arrival is never recorded anywhere. The subsequentwaitUntilCapturing()then constructs a fresh promise and waits for an event that has already come and gone. Nothing else ever setsstartedResolve, and this path has no timeout by design (the class doc: a human may be reading the portal dialog, so a timeout would cancel legitimate recordings). The promise never settles.Blast radius. Linux/PipeWire recording only; the Windows and macOS sessions have their own classes. The user sees the app hang at the moment recording starts — while the helper is in fact recording and writing to the MP4, and the compositor's screen-sharing indicator is lit. The recording can only be ended by killing the app, which leaves the file without its
moovtrailer. It is a race, so it reproduces intermittently in the field and depends on how the kernel happens to chunk the helper's stdout.The same asymmetry was already recognised and fixed for the sibling wait:
waitUntilSourceSelected()carriesif (this.sourceSelected) return Promise.resolve();with the comment "Callers must not have to race the event to observe it."waitUntilCapturing()simply never got the same treatment.Repro
Run the package's own test file with the 8 added regression tests against the unmodified production source:
The second failing test is the production interleaving: both events written into one
stdoutwrite, exactly asNdjsonLineReaderreceives them from a real helper.Fix
Why this is minimal and correct: it records the fact that already exists in the event stream, in the same shape the sibling wait already uses, and changes no other path.
capturingis written in exactly one place and read in exactly one place.The ordering is load-bearing, and it differs deliberately from the sibling.
waitUntilSourceSelected()checks its latch before the liveness check, which is right there: a selection that already happened stays true even if the helper later dies, and the caller's next step is to readgrantedSourceKind. For capturing the opposite holds — a helper that died after its first frame has stopped recording, so answering "capturing" from a stale latch would report a recording that no longer exists and convert a hang into silent data loss. Putting the liveness check first preserves the existing rejection. This is pinned by a test that fails if the two are swapped (see Test evidence).Alternatives rejected.
waitUntilCapturing(). Rejected: the absence of a timeout is deliberate and documented (the portal picker has no upper bound), and a timeout would abort legitimate slow starts while still not delivering the event that did arrive.arm()install the resolver before writingrecord. Rejected: it only narrows the window. The non-deferred path (handlers.ts:2494) awaitswaitUntilSourceSelected()first, socapture-startedcan still land before the laterawait, andarm()is a no-op-able idempotent call that should not own promise state.handleEventby constructing the promise in the constructor. Rejected: larger change, and it would make a never-awaited rejection an unhandled rejection at process level.capture-started. Rejected: it is the Rust side's correct behaviour to report the first frame immediately, and a sleep there would be a race fix by timing.Test evidence
All 8 tests live in the existing
electron/native-bridge/capture/linuxNativeCaptureSession.test.ts, using that file's existingFakeHelper/newSession/startReady/flushStdoutfixtures and its doc-comment convention. Tests 1-4 were added with the fix; tests 5-8 were added by an independent adversarial verification run atb57cbec2, which changed no production code (diffagainst a pre-arm copy of the source: byte-identical to3fc1210d).false)Full-file transcripts for both arms are quoted verbatim under
## Summaryand## Repro; the per-test timings above are copied from those same two runs.The three controls each fail under a mutation of the thing they protect — none is decoration:
Test 3 controls for the latch not swallowing a pre-frame failure; test 4 controls the ordering (arm A above); test 8 controls the latch's initial value (arm C above).
The discriminating tests pin the fix, not merely the read. Deleting the latch write while keeping the field and the read restores the hang:
Tooling required by AGENTS.md, all run at head
b57cbec2:Both typecheck jobs were run because AGENTS.md requires it: "test files are invisible to the root config, and a type error in a
*.test.tsfails CI while the root check stays green." The repo's husky + lint-staged hook also ranbiome checkon every commit and passed.The full suite (
npm run test, ~1670 tests) was not run locally — per AGENTS.md's own guidance, the affected file is run while working. The fork's CI runs the full suite; see## Verification method.Verification method
executed, in a Linux container: Node v22 (repo pins 22.22.1 viaengines),npm ci --no-audit --no-fund(exit 0, 478 top-level entries), vitest 4.1.10. Both arms were executed by checking the base copy of the production file into the worktree (git checkout 520f6e5e -- <file>), running, then restoring — the test file identical in both arms. Every arm was guarded withgrep -c "this.capturing"(0 on base, 2 on head, 1 on arm B) so no arm can be misattributed, and the production source wasdiffed against a pre-arm copy after each restore to confirm it returned byte-identical.The bug is platform-conditional in the sense that the code path only runs on Linux, but the test is platform-independent: the session class is driven through a faked child process, which is how the existing tests in this file work and how CI (Linux-only) exercises it.
Independent adversarial verification ran at
b57cbec2and re-derived the boundary ledger from the diff rather than from this document. It rebuilt all three mutation arms itself instead of trusting the transcripts above, added tests 5-8, and touched no production code.The upstream's own CI ran on the fork. At the previous head
3fc1210d, run 35296580633 was green acrossTest(the FULLnpm run testsuite, ~1670 tests),Lint,Type Check,Typecheck (tests),Build,Docs,AppStream metadata, and the Swift/macOS/Windows bundle jobs. At the current headb57cbec2, run 35297720566,gh pr checks 1 --repo askalf/openscreen:Testhere is again the fullnpm run testsuite and it passed (2m9s), as didLint,Type Check,Typecheck (tests),Buildand the Swift/macOS/Windows bundle jobs. The threeRust *jobs were still pending when checked and were not waited on; they build the compositor crate, which this diff does not touch.One non-green job, and it does not concern the change:
Validate PR title (semantic)fails on the[oss-candidate]prefix in this fork PR's title, which is a staging marker and is not part of the upstream submission. The suggested upstream title at the bottom of this document is already conventional-commit clean and passes that rule.What was not verifiable here: nothing in this diff. The unbuildable part of this repo (see
## Prior art) is the Rust helper, which this change does not touch.Prior art
Git history of the touched file and its siblings:
No open or closed PR touches
waitUntilCapturingor this latch. The closest prior work is the sibling latch added in the same file forsource-selected, which this change mirrors.This PR is not a fix for issue getopenscreen#615 and does not claim to be. getopenscreen#615 reports malformed H.264 access units from the encoder/muxer; that lives in the Rust helper. It is cited here only as the issue that led to reading this pipeline.
Policy
CONTRIBUTING.mdandAGENTS.mdare the only policy files in the repo.AI_POLICY.md,.github/AI_POLICY.md,AI.md,AGENT_POLICY.md,.github/CONTRIBUTING.md,.github/PULL_REQUEST_TEMPLATE*andCODE_OF_CONDUCT.mdall return 404. No AI/LLM/agent restriction of any kind, and no CLA.AGENTS.mdis explicitly addressed to agents:Tooling lines complied with:
CONTRIBUTING.mdasks for a closing keyword only when a PR fully resolves an issue; this PR resolves no filed issue, so it references none.Not applicable / not done: no changelog or changeset is required by either file; no DCO sign-off is requested;
npm run i18n:checkis irrelevant (no locale strings touched); the manual computer-use E2E pass in AGENTS.md is required "after any change to native capture, preview or export" — this change is in the Electron main-process session wrapper, not native capture, and the container has no computer-use MCP and no display, so it was not run. Worth flagging to the maintainer as the one check a reviewer may want.Disclosure facts for the operator
Plain facts about what the AI did, for you to write your own disclosure:
waitUntilCapturing()against its siblingwaitUntilSourceSelected().true). Every transcript in this document is copy-pasted terminal output.biome check,tsc --noEmitandtsc -p tsconfig.test.json --noEmitwere run by AI and passed.npm run testsuite was not run locally, but it ran green in CI on the fork at the previous head (~1670 tests).Boundaries
Every predicate, guard and assignment the diff adds or changes. Rebuilt from the diff by the verification run; rows 14-16 are ones that pass added after the first ledger, and rows 6, 9 and 10 had their pins upgraded from pre-existing tests (which pass on both arms and so cannot pin anything) to tests that discriminate.
if (!this.process)inwaitUntilCapturing(unchanged, now first)process === nullbeforestart()process === nullafter helper exit, no frame seenprocess === nullafter helper exit, frame already seenif (this.capturing)— new latch readcapturing === false, process alive, event not yet arrivedcapturing === true, process alive (event already arrived)startedResolvewas nulled by the handlerthis.capturing = true— new assignmentcapture-startedcursor.rebasecapture-started(helper never emits one:main.rs:856let first = !capture.started()latches, and:906guardsif first && capture.started())true; no observable changemain.rs:856,906, not pinned by a testcapture-startedfor a non-deferred session (arm()is a no-op verb the helper ignores)deferStartcapturingfield initial valuefalse— no promise resolves earlystartedResolve?.()(unchanged)startedReject?.()(unchanged)capturingafterstop()/discard()waitUntilCapturing()calledprocessis null by then, so row 1 rejects first — the latch is never reachederrorevent path (cross-axis)errorarrives aftercapture-started; helper still alivecase "error"clears onlysourceSelectedResolve/Reject, never the latch, and the capture is genuinely still runningerrorevent path, reverse ordererrorarrives before any frame, helper then exitsfalse, and row 2 appliessourceSelectedlatch (sibling, untouched)Row 8 is the only row with no test, and it is unreachable from the shipping helper: the Rust side latches
firstbefore the frame stages and only emits when bothfirstandcapture.started()hold. Rows 1-3, 12, 13 and 15 are why the latch sits after the liveness check.The diff changes no behaviour outside
waitUntilCapturing()and thecapture-startedcase:capturingis written in one place and read in one place, and a whole-class grep for the other waiter state (sourceSelected,startedResolve,startedReject,stoppedResolve) confirms no other path reads or clears it.Suggested upstream PR title
fix(capture-linux): latch capture-started so an early first frame is not lost