fix: stream recording chunks to disk to support recordings longer than 10 minutes#617
fix: stream recording chunks to disk to support recordings longer than 10 minutes#617Amanuel2x wants to merge 1 commit into
Conversation
Recordings longer than ~10 minutes would silently fail to save. The entire recording was held in memory as a Blob[] array, then on stop fixWebmDuration() and arrayBuffer() created additional copies before the data was sent over IPC to be written. For an 18+ minute 1080p recording this meant 400-600MB being duplicated 3-4x in the renderer process, exceeding Electron's memory limits and causing a silent crash with no file written to disk. Fix: open an fs.WriteStream on the main process as soon as recording starts, and send each 1-second ondataavailable chunk directly to disk via IPC instead of accumulating chunks in RAM. On stop, the stream is closed and the session manifest is written — no in-memory blob needed. A full in-memory fallback is preserved for environments where the IPC stream is unavailable.
📝 WalkthroughWalkthroughThis PR implements disk-backed recording streaming to prevent out-of-memory crashes for long recordings. Instead of buffering the entire WebM blob in renderer memory, chunks are now streamed directly to disk via IPC as they arrive, keeping memory usage constant regardless of recording duration. ChangesRecording Stream Persistence
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 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. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 3
🧹 Nitpick comments (1)
src/hooks/useScreenRecorder.ts (1)
117-119: 💤 Low valuenit: chunk append errors are silently ignored.
if
appendRecordingChunkfails mid-recording (disk full, stream error), the promise rejection is swallowed byvoid. the recording continues but data is lost with no feedback.lowkey risky for long recordings — at minimum consider logging failures so they're debuggable. could also set a flag to trigger fallback behavior.
Also applies to: 142-143
🤖 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 117 - 119, The code currently uses "void window.electronAPI.appendRecordingChunk(recordingId, chunk)" inside the pendingChunks loop (and similarly at lines ~142-143), which swallows promise rejections and hides failures; update the logic in useScreenRecorder.ts to await or handle appendRecordingChunk's promise and surface errors: wrap the per-chunk calls (or the Promise.all of them) in try/catch or attach .catch handlers that log the error (e.g., console.error or an existing logger) and set a recording error flag/state (e.g., recordingFailed or setRecordingError) so the UI or fallback behavior can react when appendRecordingChunk(recordingId, chunk) fails; ensure you update both the pendingChunks loop and the later occurrence that uses appendRecordingChunk.
🤖 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 2081-2089: The handler that opens a write stream (using
resolveRecordingOutputPath, createWriteStream and activeWriteStreams with
recordingId) can fail with ENOENT if the recordings directory doesn't exist;
before creating the write stream call fs.mkdir (or fs.promises.mkdir) on the
parent directory of resolveRecordingOutputPath(fileName) with { recursive: true
} (await if using promises) inside the try block, then create the stream and set
activeWriteStreams; keep the existing try/catch and return shape but ensure the
directory creation is awaited and errors propagate into the existing error
return.
- Around line 2130-2141: The finalize path leaks file streams when a recording
is discarded: update finalizeScreenRecording (and/or cancelRecording flow) to
explicitly close and cleanup any activeWriteStreams entry for the recordingId
(use the same logic as in the existing write/close block that calls screenWs.end
and activeWriteStreams.delete) before bailing out, and also remove any partial
screenVideoPath (or ensure stream end is awaited) so open handles and orphaned
.webm files are cleaned up; reference activeWriteStreams,
finalizeScreenRecording, cancelRecording, discardRecordingId.current,
storeRecordedSession and discardCursorTelemetry when adding this cleanup or
creating a dedicated IPC abortRecording handler.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 104-166: The returned streaming flag is captured before
streamOpenPromise resolves, causing a race and data loss; fix by determining
streaming only when recording stops and exposing that final value (or including
it with the resolved blob). Concretely, update recorder.onstop to set a
finalStreaming boolean (based on streamFailed) before resolving
recordedBlobPromise, and change the hook's return value to either provide a
getter/function for streaming or have recordedBlobPromise resolve to an object
like { blob, streaming }; then update finalizeRecording to consume the final
streaming state from that getter/object instead of reading the early-captured
streaming variable. Ensure you touch streamOpenPromise,
streamFailed/streamReady, recorder.onstop, recordedBlobPromise, and the place
that checks activeScreenRecorder.streaming in finalizeRecording.
---
Nitpick comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 117-119: The code currently uses "void
window.electronAPI.appendRecordingChunk(recordingId, chunk)" inside the
pendingChunks loop (and similarly at lines ~142-143), which swallows promise
rejections and hides failures; update the logic in useScreenRecorder.ts to await
or handle appendRecordingChunk's promise and surface errors: wrap the per-chunk
calls (or the Promise.all of them) in try/catch or attach .catch handlers that
log the error (e.g., console.error or an existing logger) and set a recording
error flag/state (e.g., recordingFailed or setRecordingError) so the UI or
fallback behavior can react when appendRecordingChunk(recordingId, chunk) fails;
ensure you update both the pendingChunks loop and the later occurrence that uses
appendRecordingChunk.
🪄 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: 1f44ef88-365b-4fb5-b52e-82313de502a5
📒 Files selected for processing (4)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/preload.tssrc/hooks/useScreenRecorder.ts
| try { | ||
| const filePath = resolveRecordingOutputPath(fileName); | ||
| const ws = createWriteStream(filePath, { flags: "w" }); | ||
| activeWriteStreams.set(recordingId, ws); | ||
| return { success: true }; | ||
| } catch (error) { | ||
| return { success: false, error: String(error) }; | ||
| } | ||
| }, |
There was a problem hiding this comment.
Missing directory creation before opening write stream.
createWriteStream will throw ENOENT if RECORDINGS_DIR doesn't exist. Native recording flows call fs.mkdir(RECORDINGS_DIR, { recursive: true }) before starting, but the browser-based recording flow doesn't — it relies on this handler being called first.
🐛 Proposed fix
ipcMain.handle(
"open-recording-stream",
async (
_,
recordingId: number,
fileName: string,
): Promise<{ success: boolean; error?: string }> => {
try {
+ await fs.mkdir(RECORDINGS_DIR, { recursive: true });
const filePath = resolveRecordingOutputPath(fileName);
const ws = createWriteStream(filePath, { flags: "w" });
activeWriteStreams.set(recordingId, ws);
return { success: true };
} catch (error) {
return { success: false, error: String(error) };
}
},
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const filePath = resolveRecordingOutputPath(fileName); | |
| const ws = createWriteStream(filePath, { flags: "w" }); | |
| activeWriteStreams.set(recordingId, ws); | |
| return { success: true }; | |
| } catch (error) { | |
| return { success: false, error: String(error) }; | |
| } | |
| }, | |
| try { | |
| await fs.mkdir(RECORDINGS_DIR, { recursive: true }); | |
| const filePath = resolveRecordingOutputPath(fileName); | |
| const ws = createWriteStream(filePath, { flags: "w" }); | |
| activeWriteStreams.set(recordingId, ws); | |
| return { success: true }; | |
| } catch (error) { | |
| return { success: false, error: String(error) }; | |
| } | |
| }, |
🤖 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 2081 - 2089, The handler that opens a
write stream (using resolveRecordingOutputPath, createWriteStream and
activeWriteStreams with recordingId) can fail with ENOENT if the recordings
directory doesn't exist; before creating the write stream call fs.mkdir (or
fs.promises.mkdir) on the parent directory of
resolveRecordingOutputPath(fileName) with { recursive: true } (await if using
promises) inside the try block, then create the stream and set
activeWriteStreams; keep the existing try/catch and return shape but ensure the
directory creation is awaited and errors propagate into the existing error
return.
|
|
||
| // Close the streaming write stream if one was used; otherwise fall back to | ||
| // writing the full buffer (short recordings that never opened a stream). | ||
| const screenWs = activeWriteStreams.get(createdAt); | ||
| if (screenWs) { | ||
| await new Promise<void>((resolve, reject) => | ||
| screenWs.end((err?: Error | null) => (err ? reject(err) : resolve())), | ||
| ); | ||
| activeWriteStreams.delete(createdAt); | ||
| } else if (payload.screen.videoData && payload.screen.videoData.byteLength > 0) { | ||
| await fs.writeFile(screenVideoPath, Buffer.from(payload.screen.videoData)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if discardCursorTelemetry is the only cleanup called on discard
rg -n "discardCursorTelemetry" --type ts -C 3Repository: siddharthvaddem/openscreen
Length of output: 1541
🏁 Script executed:
#!/bin/bash
# Find the discard-cursor-telemetry handler in electron handlers
rg -n "discard-cursor-telemetry" --type ts -A 15 electron/Repository: siddharthvaddem/openscreen
Length of output: 1152
🏁 Script executed:
#!/bin/bash
# Also check what happens with activeWriteStreams when recording is discarded
rg -n "activeWriteStreams" --type ts -B 3 -A 3Repository: siddharthvaddem/openscreen
Length of output: 3504
🏁 Script executed:
#!/bin/bash
# Check if there's a cancel/cleanup handler for recordings
rg -n "cancelRecording\|abort.*[Rr]ecord" --type ts -A 5Repository: siddharthvaddem/openscreen
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Search for the discard-cursor-telemetry handler definition
rg -n '"discard-cursor-telemetry"' electron/ipc/handlers.ts -A 20Repository: siddharthvaddem/openscreen
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Also check the structure around the storeRecordedSessionFiles handler to understand the flow better
rg -n '"store-recorded-session"' electron/ipc/handlers.ts -B 5 -A 3Repository: siddharthvaddem/openscreen
Length of output: 313
🏁 Script executed:
#!/bin/bash
# Search for discard-cursor-telemetry in all files
rg -n '"discard-cursor-telemetry"' . --type tsRepository: siddharthvaddem/openscreen
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Check the useScreenRecorder hook to see full flow on discard
rg -n "discardRecordingId.current" src/hooks/useScreenRecorder.ts -B 5 -A 10Repository: siddharthvaddem/openscreen
Length of output: 5815
Streams definitely leak when recording is cancelled/discarded.
When cancelRecording is called, it sets discardRecordingId.current and triggers finalization. In finalizeScreenRecording, if the recording is being discarded (line 416-418 in useScreenRecorder.ts), it bails out early without calling storeRecordedSession. That handler is the only place activeWriteStreams gets cleaned up—so the write streams stay open indefinitely, and partial .webm files are orphaned on disk.
The discardCursorTelemetry call looks like an incomplete attempt at cleanup; the handler doesn't even exist in the codebase.
You'll need either:
- A dedicated IPC to abort/cleanup a recording stream by
recordingId, or - Close streams directly when discard happens (maybe in a new handler or as part of the same finalize flow)
Without this, cancelled recordings will accumulate open file handles and disk clutter.
🤖 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 2130 - 2141, The finalize path leaks
file streams when a recording is discarded: update finalizeScreenRecording
(and/or cancelRecording flow) to explicitly close and cleanup any
activeWriteStreams entry for the recordingId (use the same logic as in the
existing write/close block that calls screenWs.end and
activeWriteStreams.delete) before bailing out, and also remove any partial
screenVideoPath (or ensure stream end is awaited) so open handles and orphaned
.webm files are cleaned up; reference activeWriteStreams,
finalizeScreenRecording, cancelRecording, discardRecordingId.current,
storeRecordedSession and discardCursorTelemetry when adding this cleanup or
creating a dedicated IPC abortRecording handler.
|
|
||
| // Try to open a disk-backed stream. Falls back to in-memory if IPC unavailable. | ||
| const streamOpenPromise = | ||
| window.electronAPI?.openRecordingStream?.(recordingId, fileName) ?? | ||
| Promise.resolve({ success: false }); | ||
|
|
||
| const pendingChunks: ArrayBuffer[] = []; | ||
| let streamReady = false; | ||
| let streamFailed = false; | ||
|
|
||
| streamOpenPromise.then((result) => { | ||
| if (result.success) { | ||
| streamReady = true; | ||
| for (const chunk of pendingChunks) { | ||
| void window.electronAPI.appendRecordingChunk(recordingId, chunk); | ||
| } | ||
| pendingChunks.length = 0; | ||
| } else { | ||
| streamFailed = true; | ||
| } | ||
| }); | ||
|
|
||
| const fallbackChunks: Blob[] = []; | ||
|
|
||
| const recordedBlobPromise = new Promise<Blob>((resolve, reject) => { | ||
| recorder.ondataavailable = (event: BlobEvent) => { | ||
| if (event.data && event.data.size > 0) { | ||
| chunks.push(event.data); | ||
| if (!event.data || event.data.size === 0) return; | ||
|
|
||
| if (streamFailed) { | ||
| fallbackChunks.push(event.data); | ||
| return; | ||
| } | ||
|
|
||
| void event.data.arrayBuffer().then((buf) => { | ||
| if (streamFailed) { | ||
| fallbackChunks.push(new Blob([buf], { type: mimeType })); | ||
| return; | ||
| } | ||
| if (streamReady) { | ||
| void window.electronAPI.appendRecordingChunk(recordingId, buf); | ||
| } else { | ||
| pendingChunks.push(buf); | ||
| } | ||
| }); | ||
| }; | ||
|
|
||
| recorder.onerror = () => { | ||
| reject(new Error("Recording failed")); | ||
| }; | ||
|
|
||
| recorder.onstop = () => { | ||
| resolve(new Blob(chunks, { type: mimeType })); | ||
| if (streamFailed) { | ||
| // Streaming failed — return full in-memory blob as fallback. | ||
| resolve(new Blob(fallbackChunks, { type: mimeType })); | ||
| } else { | ||
| // Streaming succeeded — main process already has the data on disk. | ||
| resolve(new Blob([], { type: mimeType })); | ||
| } | ||
| }; | ||
| }); | ||
|
|
||
| recorder.start(RECORDER_TIMESLICE_MS); | ||
| return { recorder, recordedBlobPromise }; | ||
| return { recorder, recordedBlobPromise, streaming: !streamFailed }; |
There was a problem hiding this comment.
Race condition: streaming flag doesn't reflect actual outcome.
The streaming flag is captured at line 166 as !streamFailed, but at that point the streamOpenPromise hasn't resolved yet — so streamFailed is always false and streaming is always true initially.
If the IPC stream fails to open (permissions, disk error, etc.), the fallback logic in ondataavailable correctly buffers to fallbackChunks, and onstop correctly returns the fallback blob. But then in finalizeRecording, the check at line 430:
if (!activeScreenRecorder.streaming && screenBlob.size > 0) {...is false because streaming was captured as true. The in-memory data gets thrown away and an empty ArrayBuffer(0) is sent instead, resulting in data loss.
kinda cursed timing issue — maybe make streaming a getter that checks the final state, or resolve the promise before returning.
🐛 One possible fix: defer flag evaluation to onstop
- const fallbackChunks: Blob[] = [];
+ let usedFallback = false;
+ const fallbackChunks: Blob[] = [];
const recordedBlobPromise = new Promise<Blob>((resolve, reject) => {
recorder.ondataavailable = (event: BlobEvent) => {
if (!event.data || event.data.size === 0) return;
if (streamFailed) {
fallbackChunks.push(event.data);
+ usedFallback = true;
return;
}
void event.data.arrayBuffer().then((buf) => {
if (streamFailed) {
fallbackChunks.push(new Blob([buf], { type: mimeType }));
+ usedFallback = true;
return;
}
// ... rest unchanged
});
};
recorder.onstop = () => {
- if (streamFailed) {
+ if (usedFallback) {
resolve(new Blob(fallbackChunks, { type: mimeType }));
} else {
resolve(new Blob([], { type: mimeType }));
}
};
});
recorder.start(RECORDER_TIMESLICE_MS);
- return { recorder, recordedBlobPromise, streaming: !streamFailed };
+ // streaming is determined by whether we fell back to in-memory
+ // We need to expose this info; simplest: check blob size in finalizeRecording
+ // OR return a promise/getter. For now, returning true and relying on blob.size check.
+ return { recorder, recordedBlobPromise, streaming: true };Then update finalizeRecording to check blob size regardless of streaming flag:
-if (!activeScreenRecorder.streaming && screenBlob.size === 0) {
+if (screenBlob.size === 0 && !activeScreenRecorder.streaming) {
return;
}
+// If blob has data, always process it (fallback case)
+let screenVideoData: ArrayBuffer = new ArrayBuffer(0);
+if (screenBlob.size > 0) {
+ const fixedScreenBlob = await fixWebmDuration(screenBlob, duration);
+ screenVideoData = await fixedScreenBlob.arrayBuffer();
+}🤖 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 104 - 166, The returned
streaming flag is captured before streamOpenPromise resolves, causing a race and
data loss; fix by determining streaming only when recording stops and exposing
that final value (or including it with the resolved blob). Concretely, update
recorder.onstop to set a finalStreaming boolean (based on streamFailed) before
resolving recordedBlobPromise, and change the hook's return value to either
provide a getter/function for streaming or have recordedBlobPromise resolve to
an object like { blob, streaming }; then update finalizeRecording to consume the
final streaming state from that getter/object instead of reading the
early-captured streaming variable. Ensure you touch streamOpenPromise,
streamFailed/streamReady, recorder.onstop, recordedBlobPromise, and the place
that checks activeScreenRecorder.streaming in finalizeRecording.
neurot1cal
left a comment
neurot1cal
left a comment
There was a problem hiding this comment.
Code review — streaming chunks to disk
Great diagnosis and the right architecture (timeslice → disk, renderer holds one chunk). Left inline comments on the specific lines. The short version: the change trades the current loud failure for a few silent ones, including a metadata regression on the exact long recordings it's meant to save. I'd treat the three inline items marked blocking as merge-blockers.
Good parts: correct root cause; in-memory fallback preserved when the IPC stream fails to open; types added on both sides; tsc --noEmit clean.
Testing this locally
tsc + vitest don't cover the risk surface here (ordering, duration, cleanup). Suggested manual checks:
- Duration regression: record ~12–15 min @ 1080p, stop, then
ffprobe -v error -show_entries format=duration -of csv=p=0 <out>.webm— expect a real number; today a streamed file will likely showN/A/wrong. Scrub the seek bar to confirm. - Ordering: record a long high-motion clip and play end-to-end looking for freezes; better, add a
seqtoappend-recording-chunkand assert monotonic arrival. - Abandon/leak: start a recording then quit/cancel mid-way — confirm no half-file remains and
activeWriteStreams.size === 0;lsof -p <main-pid> | grep .webmshows no lingering handle. - Disk-full: point output at a tiny volume and record past capacity — expect a surfaced error, not a silent truncated file.
- Fallback path: force
openRecordingStreamto return{ success: false }and confirm the in-memory path still yields a correct, duration-fixed file.
🤖 Thorough code-quality pass via Claude Code.
|
|
||
| // Only fix duration / convert to ArrayBuffer if we have in-memory data. | ||
| let screenVideoData: ArrayBuffer = new ArrayBuffer(0); | ||
| if (!activeScreenRecorder.streaming && screenBlob.size > 0) { |
There was a problem hiding this comment.
[blocking] fixWebmDuration is skipped for every streamed recording → broken duration metadata.
Raw MediaRecorder WebM has no usable duration header — that's why fixWebmDuration exists. Streamed files now bypass it, so the long recordings this PR fixes save with N/A/infinite duration: no reliable seek bar, wrong length downstream. The crash is gone, but the file looks corrupt.
Fix: repair duration on the file after the stream closes, in the main process where the bytes now live. handlers.ts already imports spawn and the app ships ffmpeg, so a -c copy remux / duration-rewrite on close is the natural home (the renderer can't do it anymore — it no longer holds the blob).
| return; | ||
| } | ||
|
|
||
| void event.data.arrayBuffer().then((buf) => { |
There was a problem hiding this comment.
[blocking] Chunk write order isn't guaranteed → intermittent corruption.
Blob.arrayBuffer() resolution order across separate calls isn't spec-guaranteed, and the append is fire-and-forget (void). Chunk N+1 can resolve and append before chunk N — for a sequential container that's an unplayable file, intermittently, with no error.
Fix: make ordering explicit — serialize through one queue (await chunk K's arrayBuffer() + append before K+1), or pass a monotonic seq and have the main side assert/order on it.
Related: the three-state handling here (streamFailed / streamReady / pendingChunks, two booleans + an array + a nested streamFailed re-check) is a small state machine bolted into the callback. A tiny ChunkSink (accept(chunk) that buffers-until-ready / appends / falls back) would collapse the branching and is the natural home for the ordering fix.
| // Streaming chunk writers — keyed by recordingId. Chunks are appended directly | ||
| // to disk as they arrive from ondataavailable so the renderer never holds the | ||
| // full video in memory. | ||
| const activeWriteStreams = new Map<number, WriteStream>(); |
There was a problem hiding this comment.
[blocking] WriteStream lifecycle leaks.
activeWriteStreams is only .deleted inside storeRecordedSessionFiles. Any abandon path — onerror, user discard, early return, crash before stop — leaves the stream open (leaked fd) with a half-written file on disk. Opening a second stream for the same id would also orphan the first.
Fix: wrap this in one owner (RecordingStreams with open / append / close / abort) that closes on every terminal path, and guard against double-open.
|
|
||
| // Close the streaming write stream if one was used; otherwise fall back to | ||
| // writing the full buffer (short recordings that never opened a stream). | ||
| const screenWs = activeWriteStreams.get(createdAt); |
There was a problem hiding this comment.
[blocking] Open/close keys can diverge → empty file + leaked stream.
Streams are opened keyed by recordingId but closed here by createdAt, which falls back to Date.now() above. If payload.createdAt is ever falsy, createdAt becomes a fresh timestamp, this get misses, and you fall through to fs.writeFile(path, Buffer.from(emptyArrayBuffer)) — an empty file, with the real stream never closed. The renderer passes them equal today, but that's an undocumented cross-call invariant.
Fix: key open and close by the same single id and drop the Date.now() fallback for the streaming key.
| const ws = activeWriteStreams.get(recordingId); | ||
| if (!ws) return { success: false, error: "No active stream for recordingId " + recordingId }; | ||
| return new Promise((resolve) => { | ||
| ws.write(Buffer.from(chunk), (err) => { |
There was a problem hiding this comment.
Disk-write failures are swallowed → silent truncation.
This returns { success: false } on a ws.write error, but the renderer voids the call and never reads it. Disk-full mid-recording silently drops every later chunk and the recording "succeeds" truncated — the same silent-loss mode this PR is removing, relocated.
Fix: propagate the first append failure back to the recorder (stop + user-visible error, or fall back to in-memory if still viable). Also consider backpressure — ws.write's return value is ignored here, so a high-bitrate recording can balloon the main process buffer; respect write() === false / drain.
| if (payload.webcam) { | ||
| webcamVideoPath = resolveRecordingOutputPath(payload.webcam.fileName); | ||
| await fs.writeFile(webcamVideoPath, Buffer.from(payload.webcam.videoData)); | ||
| const webcamWs = activeWriteStreams.get(createdAt + 1); // webcam stream keyed as recordingId+1 |
There was a problem hiding this comment.
recordingId + 1 as the webcam key is brittle magic.
This + 1 convention is undocumented numeric coupling between renderer (activeRecordingId + 1) and main (createdAt + 1). Prefer explicit keys like ${id}:screen / ${id}:webcam, or return a real stream handle from open so callers never compute keys.
Testing follow-up — 16-min recording + duration-patch fixBuilt What worked
Confirmed: finding #2 is real
User-felt failure mode: pre-PR they got nothing on stop; post-PR they get a 557 MB file that loads in the preview but is uneditable. Fix (diff below)Patch the WebM Duration header on disk in the main process after Verified locally: same 16-min recording with the patch applied → duration-patch diff (3 files, applies on
|
|
@Amanuel2x can you address above findings and concerns raised? |
Recordings longer than ~10 minutes silently fail to save (#616). The renderer buffers the whole WebM as a Blob[], then on stop makes several in-memory copies (fixWebmDuration -> arrayBuffer -> Buffer.from) before writing. A long 1080p recording duplicates hundreds of MB several times in the renderer, exceeds Electron's memory limit, and the renderer crashes silently with no file saved. Two changes: 1. Stream chunks to disk (originally @Amanuel2x's contribution in #617). Open an fs.WriteStream in the main process at recording start and send each ~1s ondataavailable chunk straight to disk over two new IPC calls (open-recording-stream, append-recording-chunk), so the renderer never holds more than a single chunk. A full in-memory fallback is preserved for environments where the IPC stream cannot open. 2. Patch the WebM Duration header on disk after the stream closes. Browser MediaRecorder writes WebM with no Duration element, so streamed files save with duration=N/A and the editor's seek bar, timeline, and any scrub/trim break. A new electron/recording/webm-duration.ts module rewrites the Duration element, writing to a temp file and renaming in place so a crash mid-write cannot corrupt the recording. Streaming is opt-in: the screen recorder and the browser-only webcam recorder stream to disk; native-capture webcam sidecars (Windows, macOS) keep buffering in-memory, since their finalize path reads the recorder blob directly to attach the webcam track. Verified: tsc --noEmit clean; biome clean; vitest 166/166. Closes #616 Supersedes #617 Co-Authored-By: Amanuel <amanuel@localboostnetworking.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@Amanuel2x closing this in favour or #658 - credits given by author. |
3 participants
Fixes #616
Problem
Recordings longer than ~10 minutes silently fail to save. The entire recording was buffered in the renderer as a
Blob[]array, then on stopfixWebmDuration()+.arrayBuffer()+Buffer.from()created 3–4 additional in-memory copies before the data was written to disk. For an 18-minute 1080p recording this exceeds 400–600 MB in the renderer process, causing a silent crash with no file saved and no error shown.Solution
Open an
fs.WriteStreamon the main process the moment recording starts, and pipe each 1-secondondataavailablechunk directly to disk via two new IPC calls (open-recording-stream,append-recording-chunk). On stop the stream is closed and the session manifest is written — the renderer never holds more than a single 1-second chunk in memory.A full in-memory fallback is preserved for cases where the IPC stream fails to open.
Changes
src/hooks/useScreenRecorder.ts—createRecorderHandlestreams chunks to disk via IPC instead of accumulating them inBlob[]; stop path skipsarrayBuffer()when streaming succeededelectron/ipc/handlers.ts— addsopen-recording-streamandappend-recording-chunkhandlers;storeRecordedSessionFilescloses the write stream instead of callingwriteFilewhen streaming was usedelectron/preload.ts— exposes the two new IPC callselectron/electron-env.d.ts— type declarations for the two new IPC callsTesting
tsc --noEmit— no type errorsvitest --run— 151/151 tests passingSummary by CodeRabbit
Release Notes