Repository navigation
fix(recording): ignore empty orphan segment directories during recovery - #2412
Conversation
An interrupted Studio resume can leave a segment directory behind that holds
no media at all: `SegmentPipelineFactory::create_next` allocates the
directory, and can then fail while resolving the capture target, e.g.
Studio resume failed; retaining paused recording
error=target_display_crop: Display not found
The failure path returns the error without removing the directory it created.
`require_recoverable_tracks` then iterated every `segment-N` directory on
disk and rejected any index that `analyze_incomplete` had not collected.
An empty directory is deliberately skipped during collection ("No display
fragments found for segment N", logged at debug level), so the two sides
disagreed and the entire recording was reported as
Unrecoverable display segment N
even though the segments that did hold footage were complete and otherwise
recoverable. One empty directory could cost the whole recording.
Share the display-media probe between collection and validation
(`segment_display_fragments`) so the two can no longer drift apart, and have
validation ignore an orphan directory only when it holds no recoverable
display media. A directory that still has footage but was not collected
stays fatal, because recovering around it would silently drop that footage.
Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for the detailed write-up, this is a real bug. Before review we need Greptile at 5/5 and green CI. Please only skip a segment directory when it holds no recoverable media at all (not just no display), fix the guard test so it actually reaches the populated-directory check, then request a Greptile re-review. |
…rd test Greptile P1: the skip only looked at display media, so a segment directory holding recoverable microphone, system-audio or camera media but no decodable display fragment was treated as an empty orphan. Recovery then left it out of the published metadata and silently omitted that footage. Skip a directory only when it holds no recoverable media of any kind. `segment_display_fragments` grows into `segment_track_fragments`, which the camera arm of segment collection now shares as well, and `segment_holds_recoverable_media` composes it with the audio probe. Greptile P1: the guard test declared two segments in metadata and then dropped segment 1 from the recoverable set, so validation returned "Missing display segment 1" before reaching the directory-level check it was meant to cover and its assertion could never pass. Declare a single segment so the guard is actually reached, and add a companion test for a directory holding only a non-display track. Co-Authored-By: Claude <noreply@anthropic.com>
|
Both P1s are addressed in 615e467: Skip condition widened to every track. Guard test now reaches the guard. It declared two segments in metadata and then dropped segment 1 from the recoverable set, so validation returned |
|
@iwillwin-wcy thanks for the follow-up, the widened "no recoverable media of any kind" check is exactly what we wanted. This now conflicts with main in crates/recording/src/recovery.rs: the segment collection on main uses find_studio_fragments_with_init (with the display.mp4 / camera.mp4 single-file fallbacks) and respawn_directories was reworked, so segment_track_fragments (recovery.rs:610 on your branch) needs to be rebuilt on top of find_studio_fragments_with_init rather than find_complete_fragments_with_init. Could you rebase onto main, keep main's behaviour for the display and camera collection, re-run the three new tests and resolve the two Greptile threads once done? Then we'll run the recovery suites on it. |
# Conflicts: # crates/recording/src/recovery.rs
|
@greptileai review |
|
@richiemcilroy is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
richiemcilroy
left a comment
There was a problem hiding this comment.
Verified for 0.6.2: Greptile 5/5 on this head, full CI green, and the full test suites pass on macOS with this PR combined with the other 0.6.2 changes.
What
A resume that fails after it has allocated the segment directory leaves an empty
segment-Ndirectory behind. That empty directory then makes recovery reject the entire recording:The segments that do hold footage are complete and intact. One empty directory costs the whole recording.
Why
SegmentPipelineFactory::create_nextallocates the segment directory, then can fail while resolving the capture target:That path returns the error without removing the directory it just created.
discard_resume_pipeline, which does clean up, is only reached from the Linuxcancel_resumepath.Recovery then contradicts itself.
analyze_incompletedeliberately skips a segment that has no display fragments:but
require_recoverable_trackswalked the same directory listing and rejected any index the collector had not collected:So the very segment the collector had just skipped became the reason to abandon everything.
Fix
segment_display_fragmentsand call it from both collection and validation, so the two can no longer drift apart about which segments are recoverable.Tests
Two regression tests in
crates/recording/src/recovery.rs:empty_orphan_segment_directory_does_not_block_recovery— reproduces the real layout (one segment with encoded fragments plus an emptysegment-1) and assertsrequire_recoverable_trackssucceeds. Fails withUnrecoverable display segment 1before this change.segment_directory_with_footage_outside_the_recoverable_set_is_still_fatal— pins the guard so the fix stays narrow.Context
This came out of a real 8m48s recording that was given up for lost: the user paused, minimized the captured window, hit resume, got
Display not found, then pressed stop and the whole recording was written off. The footage was complete on disk — 23791 frames — with only this empty directory standing in the way.Worth fixing rather than just documenting because the trigger is easy to hit in normal use:
is_window_valid_for_enumerationfilters out any window whereIsIconicis true, so minimizing the captured window during a pause is enough.Verification
rustfmt --edition 2024 --checkpasses on the changed file. I could not run the tests locally — my machine has no FFmpeg/vcpkg toolchain, so CI is the first compile. Flagging it so a CI failure isn't a surprise.

Confidence Score: 5/5
The PR appears safe to merge; both previous findings are addressed and no new blocking issue was found.Summary
Recovery now ignores orphan segment directories that contain no recoverable media.
Both previous findings are addressed. Tests were not run during this review.
Reviews (3) · Last reviewed commit: "test(recording): recover a recording pas..." · Reviewed by Greptile