fix(capture): stop losing the recording when finalization is interrupted - #798
fix(capture): stop losing the recording when finalization is interrupted#798iOSDevSK wants to merge 1 commit into
Conversation
`finishCapture()` appended a tail frame — one extra copy of the last buffer so the final frame gets its full duration — without checking that the input could accept it. `-[AVAssetWriterInput appendSampleBuffer:]` raises an NSInvalidArgumentException when the encoder queue is still backed up, and Swift cannot catch an Objective-C exception, so the helper hit abort() in the middle of finalization: no `finishWriting()`, an `mdat` still carrying its size-0 placeholder and no `moov` index. The whole recording became unplayable. Three of twelve recordings on one machine today died this way, all of them long captures at 5120x2830 where the encoder is most likely to be behind at stop time; the short ones finalized fine. Crash reports point at finishCapture() -> appendSampleBuffer -> objc_exception_throw -> abort. The capture callback already guarded `isReadyForMoreMediaData` before appending; finalization did not. Now the tail frame waits up to a second for the queue to drain and is skipped if it does not — the file is never worth one cosmetic frame. `endSession`, `markAsFinished` and `finishWriting` raise the same way on a writer that failed mid-capture, so they are gated on `.writing`, and the callback appends now check writer status too, which closes the same crash for a mid-recording failure such as a full disk. A writer that does not reach `.completed` is reported as an error instead of a success. The main process made it worse: when the helper died, the stop handler's fallback checked only that the output file existed and handed it to the editor, which rendered a smear of undecodable bytes. `isUnfinalizedMp4()` detects the interrupted-writer signature (open-ended `mdat`, no `moov`) and reports a failed recording instead. The check is deliberately conservative — anything it cannot positively identify keeps the previous behaviour. Rebuilding an index for already-broken files is left as follow-up; the capture data in those files is intact and recoverable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
📝 WalkthroughWalkthroughThe recorder now checks writer readiness during capture finalization. New MP4 utilities classify incomplete files. macOS recovery rejects unfinalized fallback captures before audio muxing or video finalization. ChangesMP4 Capture Recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ScreenCaptureKitRecorder
participant MP4File
participant NativeRecovery
participant isUnfinalizedMp4
ScreenCaptureKitRecorder->>MP4File: finalize ready writers
NativeRecovery->>MP4File: locate fallback capture
NativeRecovery->>isUnfinalizedMp4: validate MP4 metadata
isUnfinalizedMp4-->>NativeRecovery: return layout result
NativeRecovery-->>NativeRecovery: reject unfinalized output
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/recording/mac.ts`:
- Around line 287-290: Update the unfinalized-file branch in the recording
recovery helper around isUnfinalizedMp4 to return { success: false, message,
unfinalizedPath: candidatePath } instead of null. Preserve the existing console
error and ensure this failure result propagates to both
stop-native-screen-recording and the dedicated recovery IPC handler without
muxing or editor processing.
In `@electron/native/ScreenCaptureKitRecorder.swift`:
- Around line 408-418: Update the finalization logic around systemAudioWriter,
microphoneOnlyWriter, and finalizeFailure so every enabled writer must have
status .completed; return an error derived from microphoneOnlyWriter.error or
unfinalizedWriterError when microphoneOnlyWriter is not completed, even if
assetWriter completed. Ensure microphone-only writer failures are reported as
finalization failures rather than allowing success.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ea586b5-d4db-49df-93c6-6de935b522f9
📒 Files selected for processing (7)
electron/ipc/recording/mac.tselectron/ipc/recording/mp4Integrity.test.tselectron/ipc/recording/mp4Integrity.tselectron/ipc/register/recording.tselectron/native/ScreenCaptureKitRecorder.swiftelectron/native/bin/darwin-arm64/recordly-screencapturekit-helperelectron/native/bin/darwin-x64/recordly-screencapturekit-helper
| if (await isUnfinalizedMp4(candidatePath)) { | ||
| console.error("[mac-recover] Capture file was never finalized:", candidatePath); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return the interrupted-recording failure to the caller.
This branch returns null, which makes stop-native-screen-recording report “No native screen recording is active” when the helper already produced an unfinalized file. The dedicated recovery IPC handler similarly reports that no recoverable output exists.
Return { success: false, message, unfinalizedPath: candidatePath } from this branch. This preserves the failed-recording state and the raw path without passing the file to muxing or the editor.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import type { ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/recording/mac.ts` around lines 287 - 290, Update the
unfinalized-file branch in the recording recovery helper around isUnfinalizedMp4
to return { success: false, message, unfinalizedPath: candidatePath } instead of
null. Preserve the existing console error and ensure this failure result
propagates to both stop-native-screen-recording and the dedicated recovery IPC
handler without muxing or editor processing.
| if let systemAudioWriter, systemAudioWriter.status == .writing { | ||
| systemAudioInput?.markAsFinished() | ||
| await systemAudioWriter.finishWriting() | ||
| } | ||
|
|
||
| microphoneOnlyInput?.markAsFinished() | ||
| await microphoneOnlyWriter?.finishWriting() | ||
| if let microphoneOnlyWriter, microphoneOnlyWriter.status == .writing { | ||
| microphoneOnlyInput?.markAsFinished() | ||
| await microphoneOnlyWriter.finishWriting() | ||
| } | ||
|
|
||
| let finalizeFailure: Error? = assetWriter.flatMap { $0.status == .completed ? nil : ($0.error ?? unfinalizedWriterError(status: $0.status)) } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Report failed microphone writer completion.
When both audio sources are enabled, Lines 345-351 write microphone audio only to microphoneOnlyWriter. If that writer fails, Lines 413-416 skip finalization and Line 418 still returns success when the video writer completed. The later mux failure is non-fatal, so the completed recording can silently omit requested microphone audio.
Include every enabled writer in the completion check. Return a finalization error when microphoneOnlyWriter.status is not .completed.
🤖 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/ScreenCaptureKitRecorder.swift` around lines 408 - 418,
Update the finalization logic around systemAudioWriter, microphoneOnlyWriter,
and finalizeFailure so every enabled writer must have status .completed; return
an error derived from microphoneOnlyWriter.error or unfinalizedWriterError when
microphoneOnlyWriter is not completed, even if assetWriter completed. Ensure
microphone-only writer failures are reported as finalization failures rather
than allowing success.
1 participant
Problem
The macOS capture helper crashes at stop time and takes the whole recording with it.
ScreenCaptureRecorder.finishCapture()appends a tail frame — one extra copy of the last buffer, so the final frame gets its full duration — without checking that the input can accept it:-[AVAssetWriterInput appendSampleBuffer:]raisesNSInvalidArgumentExceptionwhen the encoder queue is still backed up, and Swift cannot catch an Objective-C exception, so the helper hitsabort()in the middle of finalization: nofinishWriting(), anmdatstill carrying its size-0 placeholder, nomoovindex. The file is unplayable.Crash report backtrace:
Three of twelve recordings on one machine in a single session died this way — 45 MB, 45 MB and 69 MB, all long captures at 5120×2830. The short ones finalized fine, which is consistent with encoder backpressure at stop being the trigger. The three broken files carry the exact interrupted-writer layout (
ftyp+wide+mdatsize 0, nomoov) and none of them got the.cursor.jsonsidecar that a successful stop writes.The main process then made it worse. When the helper died, the stop handler's fallback checked only that the output file existed and passed it to the editor, which rendered a smear of undecodable bytes instead of reporting a failed recording.
recoverNativeMacCaptureOutput()— reachable independently from the renderer's recovery IPC — did the same.Fix
Helper (
ScreenCaptureKitRecorder.swift)endSession,markAsFinishedandfinishWritingraise the same uncatchable way on a writer that is no longer.writing, so all three are gated on writer status.isReadyForMoreMediaDatabut not writer status — they do now, which closes the same crash for a mid-recording failure such as a full disk..completedis reported as an error instead of a success.Main process
isUnfinalizedMp4()(electron/ipc/recording/mp4Integrity.ts) walks the top-level box table — 8 bytes per header, so a multi-gigabyte capture costs a handful of reads — and identifies the interrupted-writer signature. Both recovery paths now refuse such a file and report a failed recording. The classifier is deliberately conservative: only that exact layout isunfinalized, anything unfamiliar isunknownand keeps the previous behaviour.The renderer already surfaces a stop failure to the user, so this produces an error message rather than a garbled editor.
Testing
npm test— 1004 passed, including 8 new cases for the box-table walk and classifier.moov.tsc --noEmitandbiome checkclean on the changed files.Not reproduced: the backed-up-queue state itself at stop. The guard is a code-level guarantee —
appendis now unreachable unless the input is ready and the writer is.writing, the two documented raise conditions — backed by the crash report.Note on existing broken files
The capture data in an interrupted file is intact: the
mdatparses to the exact byte as AVCC NAL units, and rebuilding an index produces a clean, watchable video. An in-app "recover interrupted recording" path is left as follow-up.🤖 Generated with Claude Code
Summary by CodeRabbit