This repository was archived by the owner on Jun 17, 2026. It is now read-only.
fix(recording): stream native-capture webcam to disk to prevent OOM crash on stop#687
Open
codigoyi wants to merge 5 commits into
Open
fix(recording): stream native-capture webcam to disk to prevent OOM crash on stop#687codigoyi wants to merge 5 commits into
codigoyi wants to merge 5 commits into
Conversation
…rash on stop Native macOS/Windows capture recorded the webcam sidecar with an in-memory MediaRecorder (no fileName), buffering the whole clip in renderer memory and materializing it into one Blob/ArrayBuffer on stop. Long recordings (e.g. ~20 min at 18 Mbps, multi-GB) overflowed the renderer heap and crashed it with EXC_BREAKPOINT/SIGTRAP before the file was ever written, losing the entire take. The screen path already streams chunks to disk (siddharthvaddem#616); this extends the same mechanism to the native webcam sidecar: - Pass a fileName to the native webcam createRecorderHandle (mac + windows) so chunks are written to disk as they arrive instead of buffering in memory. - Thread the webcam fileName through the native recording state so finalize targets the exact file the recorder streamed to, independent of the native recordingId echoed back by the helper. - Windows finalize routes a streamed webcam through store-recorded-session with an empty buffer + durationMs (it already patches the WebM duration on disk). - macOS attach-native-mac-webcam-recording finalizes the streamed file via the recording-stream registry and patches its duration on disk, falling back to the in-memory buffer when not streamed. - Discard partial webcam files/streams on cancel, failure, or mid-stream write error so nothing is orphaned. The main process decides streamed-vs-memory by whether a disk stream is open (registry.finalize), so sending an empty buffer when streaming is safe and the in-memory fallback is preserved when the stream fails to open. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds streamed webcam sidecar support and deterministic sidecar filenames; exposes optional durationMs through preload and IPC; RecordingStreamRegistry is registered earlier; attach handler finalizes streamed files and can patch on-disk WebM duration; native start/finalize and recorder discard paths updated for safe streaming and cleanup. ChangesNative Webcam Streaming and Duration Patching
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8ea848118
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/hooks/useScreenRecorder.ts (2)
463-473:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDiscard the streamed webcam on every Windows early-exit.
These returns stop the native flow, but they never call
activeWebcamRecorder.discard(). For a streamed sidecar that's the only cleanup path that closes the open recording stream and deletes the partial-webcam.webm, so cancel/error exits still leak files. the mac path already does this; Windows should mirror it here too.Also applies to: 535-541
🤖 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 `@src/hooks/useScreenRecorder.ts` around lines 463 - 473, The Windows stop flow in useScreenRecorder (around the stopNativeWindowsRecording result handling) returns early on discard or error without calling activeWebcamRecorder.discard(), leaking the streamed webcam file; update both early-exit paths (the discard branch and the !result.success branch near the stopNativeWindowsRecording call, and the analogous block referenced at 535-541) to first check for activeWebcamRecorder and call/await activeWebcamRecorder.discard() (or invoke it safely if it may be undefined) before clearing native state or returning, and mirror the same sequence used in the mac path (ensure activeNativeRecording.finalizing is handled consistently and clearNativeRecordingState() is still called afterwards).
926-933:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStartup aborts still leak the streamed sidecar.
createRecorderHandle(...)starts immediately, so by the time these branches run the webcam sidecar may already be open on disk. Stopping the recorder withoutdiscard()leaves the partial file/stream behind on countdown cancel or native-start failure. kinda cursed because these are exactly the “bail out early” paths users hit when setup goes sideways.Also applies to: 1026-1031, 1075-1086
🤖 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 `@src/hooks/useScreenRecorder.ts` around lines 926 - 933, The startup-abort branches leak the sidecar because they stop the recorder without discarding the partial stream; update the bailout paths in createRecorderHandle and the branches using browserWebcamRecorder to call recorder.discard() (await if it returns a promise) before calling recorder.stop() or throwing, and guard the call with a null/hasMethod check (e.g., browserWebcamRecorder?.recorder?.discard). Ensure the same discard-before-stop logic is applied to the other referenced locations (around the blocks at the 1026-1031 and 1075-1086 equivalents) so any partially written sidecar is removed on countdown cancel or native-start failure.electron/ipc/handlers.ts (1)
2188-2238:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRollback the finalized webcam file when attach fails.
After
recordingStreams.finalize(...)returns true, the sidecar is already kept on disk. Any later exception here just returns{ success: false }, which leaves a stray webcam file with no session pointing at it. lowkey risky on the exact failure path this PR says it cleans up — please unlinkwebcamVideoPathwhen the streamed attach path fails after finalize.🤖 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/ipc/handlers.ts` around lines 2188 - 2238, When recordingStreams.finalize(payload.webcam.fileName) returns true the webcam sidecar is left on disk; if any subsequent steps in the attach flow (e.g., isValidDurationMs + patchWebmDurationOnDisk or writing the session manifest) throw, the file is orphaned—so detect that the attach was for a streamed webcam (webcamStreamed true) and in the catch block unlink/remove webcamVideoPath (using fs.unlink or fs.rm, swallowing any unlink error) before returning the failure response; reference recordingStreams.finalize, webcamVideoPath, patchWebmDurationOnDisk, and the existing catch block to implement this cleanup.
🤖 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 `@src/hooks/useScreenRecorder.ts`:
- Around line 475-513: The code currently reads the entire native screen file
into renderer memory via readBinaryFile(nativeScreenPath) and passes its bytes
to storeRecordedSession, which can OOM for multi-GB recordings; stop reading the
file in the renderer and instead send the native path to the main process and
let the main process attach/patch the webcam sidecar. Concretely, remove the
readBinaryFile(nativeScreenPath) call and change the call to
window.electronAPI.storeRecordedSession (or add a new IPC like
attachWebcamToScreenRecording) to accept screenPath/nativeScreenFileName and
webcam metadata (webcamFileName, whether webcam is streamed, and if not streamed
a small duration-fixed webcam blob buffer), update callers around
nativeScreenPath, storedSession, activeNativeRecording and fixWebmDuration usage
so only webcam bytes (if any) are marshalled; implement the corresponding
main-process handler to read the large screen file from disk, finalize/patch the
WebM if needed, and own the disk streams.
---
Outside diff comments:
In `@electron/ipc/handlers.ts`:
- Around line 2188-2238: When recordingStreams.finalize(payload.webcam.fileName)
returns true the webcam sidecar is left on disk; if any subsequent steps in the
attach flow (e.g., isValidDurationMs + patchWebmDurationOnDisk or writing the
session manifest) throw, the file is orphaned—so detect that the attach was for
a streamed webcam (webcamStreamed true) and in the catch block unlink/remove
webcamVideoPath (using fs.unlink or fs.rm, swallowing any unlink error) before
returning the failure response; reference recordingStreams.finalize,
webcamVideoPath, patchWebmDurationOnDisk, and the existing catch block to
implement this cleanup.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 463-473: The Windows stop flow in useScreenRecorder (around the
stopNativeWindowsRecording result handling) returns early on discard or error
without calling activeWebcamRecorder.discard(), leaking the streamed webcam
file; update both early-exit paths (the discard branch and the !result.success
branch near the stopNativeWindowsRecording call, and the analogous block
referenced at 535-541) to first check for activeWebcamRecorder and call/await
activeWebcamRecorder.discard() (or invoke it safely if it may be undefined)
before clearing native state or returning, and mirror the same sequence used in
the mac path (ensure activeNativeRecording.finalizing is handled consistently
and clearNativeRecordingState() is still called afterwards).
- Around line 926-933: The startup-abort branches leak the sidecar because they
stop the recorder without discarding the partial stream; update the bailout
paths in createRecorderHandle and the branches using browserWebcamRecorder to
call recorder.discard() (await if it returns a promise) before calling
recorder.stop() or throwing, and guard the call with a null/hasMethod check
(e.g., browserWebcamRecorder?.recorder?.discard). Ensure the same
discard-before-stop logic is applied to the other referenced locations (around
the blocks at the 1026-1031 and 1075-1086 equivalents) so any partially written
sidecar is removed on countdown cancel or native-start failure.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da5ea51d-b2e5-49d4-9760-0416168c246b
📒 Files selected for processing (4)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/preload.tssrc/hooks/useScreenRecorder.ts
…tream-to-disk # Conflicts: # electron/ipc/handlers.ts
…d roll back orphan on attach failure Addresses review feedback on the native webcam disk-streaming change: now that the native webcam sidecar opens a main-process write stream, every path that bails out must close that stream and delete its partial *-webcam.webm, and the macOS attach handler must not leave a finalized file behind when it later fails. - Windows finalize: discard the webcam recorder on the discard/cancel branch and the stop-failure branch (mirrors the macOS finalize cleanup). - Native start aborts (Windows start failure; macOS countdown-cancel, start failure, and post-start cancel): discard the sidecar before bailing, since createRecorderHandle has already begun streaming to disk by then. - attach-native-mac-webcam-recording: track the finalized streamed file and unlink it in the catch block so a failure after finalize doesn't orphan a *-webcam.webm with no session pointing at it. Validated: tsc --noEmit clean, biome check clean, vitest 225/225 passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useScreenRecorder.ts (1)
488-514:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t hand off webcam ownership before the store actually succeeds.
storeOwnsWebcamflips beforefixWebmDuration()/storeRecordedSession(). If either throws, we jump to the outercatchand never discard the streamed sidecar, so the file handle / partial webcam can stick around. lowkey risky, especially since this PR is trying to close that exact leak class.A safer shape is: only mark ownership after
storeRecordedSession()returns success, and keep afinallycleanup for the “not definitely handed off” path.nit: cleaner ownership handoff
let storeOwnsWebcam = false; if (canStore && screenRead.data) { - storeOwnsWebcam = true; const nativeScreenFileName = nativeScreenPath.split(/[\\/]/).pop() ?? `${RECORDING_FILE_PREFIX}${activeNativeRecording.recordingId}.mp4`; const webcamFileName = activeNativeRecording.webcamFileName ?? `${RECORDING_FILE_PREFIX}${activeNativeRecording.recordingId}${WEBCAM_FILE_SUFFIX}${VIDEO_FILE_EXTENSION}`; const webcamVideoData = webcamStreamed ? new ArrayBuffer(0) : await (await fixWebmDuration(webcamBlob as Blob, duration)).arrayBuffer(); const stored = await window.electronAPI.storeRecordedSession({ screen: { videoData: screenRead.data, fileName: nativeScreenFileName, }, webcam: { videoData: webcamVideoData, fileName: webcamFileName, }, createdAt: activeNativeRecording.recordingId, cursorCaptureMode, durationMs: duration, }); + storeOwnsWebcam = stored.success; if (stored.success && stored.session) { storedSession = stored.session; } }Also applies to: 536-542
🤖 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 `@src/hooks/useScreenRecorder.ts` around lines 488 - 514, The code marks storeOwnsWebcam = true before awaiting fixWebmDuration() and window.electronAPI.storeRecordedSession(), which can leak the streamed webcam sidecar if either call throws; move the assignment so storeOwnsWebcam is set to true only after storeRecordedSession() resolves successfully, and add a finally block that checks if storeOwnsWebcam is false to perform the cleanup/discard of the streamed sidecar (mirror the existing cleanup logic used elsewhere); apply the same change to the other occurrence referenced (the block around lines 536-542) and ensure you still use webcamStreamed when choosing whether to call fixWebmDuration().
🤖 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 `@src/hooks/useScreenRecorder.ts`:
- Around line 933-935: The discard() call can race with a pending
openRecordingStream(): if openRecordingStream() later sets streamOpened=true and
closes the stream, an earlier discard() may no-op and leave an orphaned file;
update RecorderHandle.discard() to wait for or cancel a pending open before
returning (e.g., check and await the internal open promise or signal
cancellation to it), ensure openRecordingStream() exposes/uses a cancellable
promise or a shared "openPending" promise, and make
createRecorderHandle()/RecorderHandle methods (streamOpened, open promise,
discard) coordinate so callers of discard() (and sites using
browserWebcamRecorder?.discard()) cannot return before the stream open is
resolved or aborted.
---
Outside diff comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 488-514: The code marks storeOwnsWebcam = true before awaiting
fixWebmDuration() and window.electronAPI.storeRecordedSession(), which can leak
the streamed webcam sidecar if either call throws; move the assignment so
storeOwnsWebcam is set to true only after storeRecordedSession() resolves
successfully, and add a finally block that checks if storeOwnsWebcam is false to
perform the cleanup/discard of the streamed sidecar (mirror the existing cleanup
logic used elsewhere); apply the same change to the other occurrence referenced
(the block around lines 536-542) and ensure you still use webcamStreamed when
choosing whether to call fixWebmDuration().
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e84ed9b-6b31-48a8-b1e0-bb287d614c3b
📒 Files selected for processing (2)
electron/ipc/handlers.tssrc/hooks/useScreenRecorder.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/ipc/handlers.ts
…e Windows ownership on store success Second round of review feedback on the native webcam disk-streaming change: - recorderHandle.discard() now awaits the pending openRecordingStream() (and drains the write chain) before deciding whether to close. Previously an early discard() on a cancel/failed-start could no-op while the open was still pending, then the open would resolve, flip streamOpened=true and flush chunks to a file nothing would ever clean up. This is the root fix for every discard()-on-early-exit site added in the prior commit. - Native Windows finalize: only mark storeOwnsWebcam after storeRecordedSession() resolves successfully, and move the "not handed off" discard into a `finally`, so a throw in fixWebmDuration()/storeRecordedSession() still drops the partial streamed sidecar instead of leaking it. Validated: tsc --noEmit clean, biome check clean, vitest 225/225 passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/hooks/useScreenRecorder.ts (1)
600-606: 💤 Low valuenit: potential double-discard if webcamAssetPromise errors and outer code also discards
if the catch block here discards the webcam recorder, and then the outer code (lines 623, 630, 654) also calls discard on error paths, we might call
closeRecordingStreamtwice for the same file. lowkey depends on the main process being idempotent.probably fine since the
.catch(() => undefined)swallows any errors, but a cleaner pattern would be to add adiscardedflag on RecorderHandle to make discard() no-op after the first call.🤖 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 `@src/hooks/useScreenRecorder.ts` around lines 600 - 606, The catch block is discarding activeWebcamRecorder which can cause duplicate discard/close calls if outer error paths also call discard; make RecorderHandle.discard idempotent by adding an internal flag (e.g., a "discarded" boolean) on the RecorderHandle implementation and have RecorderHandle.discard() return immediately when already discarded, and ensure activeWebcamRecorder.discard() uses that new no-op behavior; update any callers (e.g., the catch here and outer error handlers) to keep calling discard() but rely on the idempotent implementation to avoid double-close of the recording stream.
🤖 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.
Nitpick comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 600-606: The catch block is discarding activeWebcamRecorder which
can cause duplicate discard/close calls if outer error paths also call discard;
make RecorderHandle.discard idempotent by adding an internal flag (e.g., a
"discarded" boolean) on the RecorderHandle implementation and have
RecorderHandle.discard() return immediately when already discarded, and ensure
activeWebcamRecorder.discard() uses that new no-op behavior; update any callers
(e.g., the catch here and outer error handlers) to keep calling discard() but
rely on the idempotent implementation to avoid double-close of the recording
stream.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 890d138e-169a-47eb-9e06-b82ae8701be5
📒 Files selected for processing (2)
src/hooks/recorderHandle.tssrc/hooks/useScreenRecorder.ts
Several early-exit paths can each call discard() for the same recorder (e.g. the webcamAssetPromise catch plus an outer error handler), which would invoke closeRecordingStream twice for one file. The main process already tolerates this (unlink swallows ENOENT), but guard discard() with a synchronous `discarded` flag so repeated/concurrent calls are a clean no-op after the first. Validated: tsc --noEmit clean, biome check clean, vitest 225/225 passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author
|
Addressed the double-discard nitpick in f910a39: |
This was referenced Jun 11, 2026
This was referenced Jun 11, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
Native macOS/Windows capture records the webcam sidecar with an in-memory
MediaRecorder(created without afileName), so the entire webcam clip is buffered in renderer memory and only materialized into a singleBlob/ArrayBufferwhen recording stops. For long recordings this overflows the renderer heap and crashes the renderer on stop — before the file is ever written — losing the whole take.This PR makes the native webcam sidecar stream its chunks to disk during recording, reusing the exact mechanism the screen path already uses (#616), so memory stays flat regardless of length.
Motivation
A user recorded ~20 min with webcam on macOS (ScreenCaptureKit native path) and pressed Stop; the app crashed and the recording was unrecoverable. The crash report shows the renderer dying with
EXC_BREAKPOINT/SIGTRAPin the V8cppgcheap at stop time — an out-of-memory abort while assembling the in-memory webcam blob.At
BITRATE_BASE(18 Mbps) a 20-minute webcam is on the order of multiple GB held in the renderer; assembling/arrayBuffer()-ing it is what tips it over. The screen track was already fixed in #616 by streaming to disk, but the native webcam sidecar still buffered in memory. This closes that gap.Type of Change
What changed
fileNameto the native webcamcreateRecorderHandleon both macOS and Windows, so chunks are appended to disk as they arrive instead of buffering in renderer memory.webcamFileName), so finalize targets the exact file the recorder streamed to — independent of therecordingIdthe native helper echoes back.store-recorded-sessionwith an empty buffer +durationMs; that handler already finalizes the on-disk file and patches the WebM duration.attach-native-mac-webcam-recordingfinalizes the streamed file via the recording-stream registry and patches its duration on disk, falling back to the in-memory buffer when not streamed.The main process decides streamed vs. in-memory by whether a disk stream is actually open (
registry.finalize()), so sending an empty buffer when streaming is safe, and the in-memory fallback is fully preserved when a stream fails to open (parity with the existing screen path).Related Issue(s)
Follow-up to #616 (which streamed the screen track to disk); this extends the same fix to the native webcam sidecar.
Testing
Static + unit checks (all green):
The existing
recorderHandle.test.tsandrecordingStream.test.tscover the streaming primitives this reuses (chunk ordering, open-failure fallback, finalize/discard).Checklist
Summary by CodeRabbit
New Features
Bug Fixes