fix(macos): recover the take a dead writer or a killed helper left on disk - #662
Merged
Merged
Conversation
… disk When a macOS stop fails — the writer died, or the helper was killed — the file on disk is a fragmented MP4 that ffmpeg, libavformat, Chromium and the editor all open as it is. The take was still reported as lost. This keeps it. electron/recording/nativeMacCaptureSalvage.ts walks the file's top-level boxes by their own offsets and counts the video frames whose bytes are actually in the file: the flat sample table the moov holds for the first second, then every fragment's trun, with tfhd/trex defaults. It does not use mp4box. PR #571 did, in 1 MiB chunks, and AVAssetWriter's last mdat carries a size field of 0, so mp4box aborted whenever a chunk boundary fell inside it and a take was kept or lost depending on its byte length; it also accepted a file cut inside a moof, which nothing can open. A torn tail — a moof, or a header, cut short — is truncated to where it starts and the file inspected again, so success always means the file opens as it is on disk. A final mdat cut short is the fragment still being written and is left alone. The stop handler only salvages when the helper has exited (a running helper may still be writing), then takes the normal save path: cursor telemetry, session manifest and media links are written, the result carries `recovered`, and the warning names how much was kept ("Recording stopped after 0:35: Disk Full. The part recorded until then was saved."). The warning reaches the editor and the CLI through the session hand-off from #661. Fixtures are five real takes from the helper with every mdat payload zeroed and gzipped (3-44 KB): two writer deaths, two killed helpers, one clean flat take. The frame counts the tests expect are ffmpeg's on the original files, and the torn cuts reproduce layouts ffmpeg refuses (cut inside a moof) and accepts once cut back.
…y what failed Review of the salvage found two defects. A recovered take most often follows a disk that filled up, and the cursor telemetry and session manifest are written to the same volume right after. Either write throwing ENOSPC reached the stop handler's catch, so a take whose video had just been recovered was reported lost with a raw errno. For a recovered take those writes are now secondary: a failure is logged and its partial file removed, and the stop still succeeds with the warning. The warning replaced the helper's sentence with the NSError's localized description. For the -16364 writer death that description is AVFoundation's generic "The operation could not be completed", so the user was never told the video file could not be written. The sentence is now kept, and a description is added only when it says something: "Recording stopped after 0:35: the video file could not be written (Disk Full)." The tests use the helper's verbatim messages instead of an invented one, and the length is rounded rather than floored (a 35 s take's frame durations sum to 34.98 s, which printed "0:34"). Also: whether to salvage is `nativeMacSalvageTarget`, tested, instead of an inline condition in the handler — salvage truncates, and the helper-exited check is what keeps it off a file still being written. The module doc no longer says every torn box breaks a file: a torn moof does; a half-written trailing header is cut as a precaution.
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 ignored due to path filters (5)
📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughChangesNative macOS recording recovery
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant NativeMacRecorder
participant StopHandler
participant CaptureSalvage
participant RecordingFiles
NativeMacRecorder->>StopHandler: stop recording
StopHandler->>CaptureSalvage: inspect failed capture
CaptureSalvage-->>StopHandler: repaired capture and duration
StopHandler->>RecordingFiles: write recovered sidecar files
StopHandler-->>NativeMacRecorder: recovered status and warning
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The recovery path correctly preserves fragment offsets and contains sidecar cleanup failures, with no identified issue blocking merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
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
When a macOS stop fails, the file left on disk is still a playable fragmented MP4. The stop fails when the writer died (#655 then stops the take) or when the helper was killed or crashed. The take was reported as lost; it is now recovered and opens in the editor.
What a failed take leaves on disk. A clean stop rewrites the file flat (
ftyp mdat moov). A failed one keeps AVAssetWriter's fragmented shape:mdatcomes before themoofthat indexes it (absolutetfhdbase offset). The finalmdatis the fragment that was still open, and it may declare size 0.Changes
electron/recording/nativeMacCaptureSalvage.ts(new).moovsample table for the first second, then every fragment'strunwithtfhd/trexdefaults.moof.moofor header cut short) makes a file that opens nowhere. It is truncated to where it starts and inspected again, so success always means the file on disk opens as it is.recovered: true. The warning keeps the helper's sentence and says how much was kept, e.g. "Recording stopped after 0:35: the video file could not be written (Disk Full). The part recorded until then was saved." It reaches the editor and the CLI through the hand-off from fix(macos): settle the native stop on the helper's exit, not its first error #661.Related issue
Refs #621. Replaces the salvage half of #571 (closed).
Type of change
Release impact
Desktop impact
Screenshots / video
None. The only UI change is the existing warning toast, now also shown for a recovered take.
Testing
End to end with the real helper. Mac mini M1, macOS 26.5. The helper was built from
main, recording 3840×2160@60 at 76.5 Mbps with system audio. The take went through the real stop and salvage modules with the stop handler's glue.SIGKILL12 s inhelper-failed, exitedhelper-failed, exitedThe one-frame gap in the disk-full take is the last frame. The last
moofindexes 12 samples in the tailmdatthat the full disk cut short, and the 57th sample's bytes are only partly on disk. ffmpeg lists that packet; the salvage counts only complete frames.Fixtures. Five real takes from the helper with every
mdatpayload zeroed and gzipped (3–44 KB), kept inelectron/recording/__fixtures__/macos-fmp4/:moof;Zeroing moves no box. The frame counts the tests expect are ffmpeg's packet counts on the original files.
Unit tests (
nativeMacCaptureSalvage.test.ts). The inspection matches ffmpeg on all five takes, and every case below comes from a real file:mdatmdatcut shortmoofmoofmoovmoovMore unit tests.
Suite. Full
vitestpasses (2,825 tests); bothtscprojects and Biome are clean.Adversarial review. A four-lens, read-only review ran on the first commit (box parsing, truncation safety, stop integration, tests and claims), with two independent skeptics per finding.
Not tested:
moofproduced by a real crash (only synthetic cuts of real files);Summary by CodeRabbit