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.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe Linux capture session now records when ChangesCapture-start latch
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The capture-start latch addresses the reported pending wait race, with no remaining concrete merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
The test comments name the invariant each case checks instead of the history behind it; the pending-wait case observes settlement with a flag and a stdout flush rather than racing a timer; the latch is set after the telemetry rebase so that comment keeps describing the rebase; and the liveness comment says only that an earlier capture-started event does not bypass the check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
Decisions
The fix records the fact that already exists in the event stream (a
capturingboolean), in the same shape the siblingwaitUntilSourceSelected()latch already uses, and changes no other path.capturingis written in exactly one place and read in exactly one place.The ordering is deliberate and differs from the sibling:
waitUntilSourceSelected()checks its latch before the liveness check (a selection that already happened stays true even if the helper later dies). 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.Alternatives rejected:
waitUntilCapturing(). The absence of a timeout is deliberate and documented (the portal picker has no upper bound); a timeout would abort legitimate slow starts while still not delivering the event that did arrive.arm()install the resolver before writingrecord. Only narrows the window — the non-deferred path awaitswaitUntilSourceSelected()first, socapture-startedcan still land before the laterawait, andarm()should not own promise state.handleEventby constructing the promise in the constructor. Larger change, and would make a never-awaited rejection an unhandled rejection at process level.capture-started. It is the Rust side's correct behaviour to report the first frame immediately; a sleep there would be a race fix by timing.Not run: the manual computer-use E2E pass required "after any change to native capture, preview or export" — this change is in the Electron main-process session wrapper, not native capture, and there was no display/computer-use environment available to run it.
AI assistance: this bug was found and the fix and tests were drafted with AI tooling in my workflow; the tests and checks above were executed as pasted. I'm responsible for the change and will handle review feedback.
Summary by CodeRabbit