From 548ca76205a7fda8bedef8bac661f20e6625167e Mon Sep 17 00:00:00 2001 From: iwillwin-wcy <13968663655@163.com> Date: Fri, 2 Oct 2026 17:11:57 +0800 Subject: [PATCH 1/2] fix(recording): ignore empty orphan segment directories during recovery 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 --- crates/recording/src/recovery.rs | 209 ++++++++++++++++++++++++++++--- 1 file changed, 192 insertions(+), 17 deletions(-) diff --git a/crates/recording/src/recovery.rs b/crates/recording/src/recovery.rs index 9eddbcbeb2e..0f9300fe92e 100644 --- a/crates/recording/src/recovery.rs +++ b/crates/recording/src/recovery.rs @@ -368,18 +368,9 @@ impl RecoveryManager { .and_then(|s| s.parse().ok()) .unwrap_or(0); - let display_dir = segment_path.join("display"); - let display_info = Self::find_complete_fragments_with_init(&display_dir); - let mut display_fragments = display_info.fragments; - let mut display_init_segment = display_info.init_segment; - - if display_fragments.is_empty() - && let Some(display_mp4) = - Self::probe_single_file(&segment_path.join("display.mp4")) - { - display_fragments = vec![display_mp4]; - display_init_segment = None; - } + let display_info = Self::segment_display_fragments(&segment_path); + let display_fragments = display_info.fragments; + let display_init_segment = display_info.init_segment; if display_fragments.is_empty() { debug!( @@ -617,6 +608,29 @@ impl RecoveryManager { } } + /// The display media a single segment directory holds, or no fragments when + /// the segment has nothing recoverable. Shared by segment collection and by + /// recovery validation so neither can disagree with the other about which + /// segments count as recoverable. + fn segment_display_fragments(segment_path: &Path) -> FragmentsInfo { + let info = Self::find_complete_fragments_with_init(&segment_path.join("display")); + + if !info.fragments.is_empty() { + return info; + } + + match Self::probe_single_file(&segment_path.join("display.mp4")) { + Some(fragment) => FragmentsInfo { + fragments: vec![fragment], + init_segment: None, + }, + None => FragmentsInfo { + fragments: Vec::new(), + init_segment: None, + }, + } + } + fn collect_respawn_groups( dir: &Path, health_tx: Option<&HealthSender>, @@ -1318,17 +1332,39 @@ impl RecoveryManager { } for entry in std::fs::read_dir(recording.project_path.join("content/segments"))? { let entry = entry?; - if let Some(index) = entry + let Some(index) = entry .file_name() .to_str() .and_then(|name| name.strip_prefix("segment-")) .and_then(|value| value.parse::().ok()) - && !indexes.contains(&index) + else { + continue; + }; + + if indexes.contains(&index) { + continue; + } + + // An interrupted resume, a crash or a power loss can leave a segment + // directory behind that holds no recoverable display media. It + // contributes nothing to the recording, so ignoring it is what lets + // the segments that do hold footage be recovered. Only a directory + // that still has display media but was not collected is fatal. + if Self::segment_display_fragments(&entry.path()) + .fragments + .is_empty() { - return Err(RecoveryError::Validation(format!( - "Unrecoverable display segment {index}" - ))); + warn!( + segment = index, + path = %entry.path().display(), + "Ignoring segment directory without recoverable display media" + ); + continue; } + + return Err(RecoveryError::Validation(format!( + "Unrecoverable display segment {index}" + ))); } Ok(()) } @@ -5197,6 +5233,145 @@ mod tests { } } +#[cfg(test)] +mod empty_orphan_segment_tests { + use super::*; + use cap_enc_ffmpeg::segmented_stream::{SegmentedVideoEncoder, SegmentedVideoEncoderConfig}; + + /// Encode a short fragmented display track so the segment passes the same + /// media probes a real recording does. + fn write_fragmented_display(segment_path: &Path) { + ffmpeg::init().unwrap(); + std::fs::create_dir_all(segment_path).unwrap(); + + let mut encoder = SegmentedVideoEncoder::init( + segment_path.join("display"), + cap_media_info::VideoInfo { + pixel_format: cap_media_info::Pixel::NV12, + width: 320, + height: 240, + time_base: ffmpeg::Rational(1, 1_000_000), + frame_rate: ffmpeg::Rational(30, 1), + }, + SegmentedVideoEncoderConfig { + segment_duration: Duration::from_secs(1), + ..Default::default() + }, + ) + .unwrap(); + + for index in 0..60 { + let mut frame = ffmpeg::frame::Video::new(ffmpeg::format::Pixel::NV12, 320, 240); + frame.data_mut(0).fill(32 + index as u8); + frame.data_mut(1).fill(128); + encoder + .queue_frame(frame, Duration::from_micros(index * 1_000_000 / 30)) + .unwrap(); + } + + encoder.finish().unwrap(); + assert!(!encoder.completed_segments().is_empty()); + } + + fn write_recording_meta(project: &Path, segment_count: usize) { + let segments = (0..segment_count) + .map(|index| MultipleSegment { + display: VideoMeta { + path: RelativePathBuf::from(format!( + "content/segments/segment-{index}/display.mp4" + )), + fps: 30, + start_time: None, + device_id: None, + }, + camera: None, + mic: None, + system_audio: None, + cursor: None, + keyboard: None, + display_notch: None, + }) + .collect(); + + let meta = RecordingMeta { + platform: None, + project_path: project.to_path_buf(), + pretty_name: "Test Recording".to_string(), + sharing: None, + upload: None, + inner: RecordingMetaInner::Studio(Box::new(StudioRecordingMeta::MultipleSegments { + inner: MultipleSegments { + segments, + cursors: Cursors::default(), + status: Some(StudioRecordingStatus::NeedsRemux), + }, + })), + }; + + std::fs::write( + project.join("recording-meta.json"), + serde_json::to_string_pretty(&meta).unwrap(), + ) + .unwrap(); + } + + /// A resume that fails after `create_next` has allocated the segment + /// directory but before it has written any frames leaves that directory + /// behind, empty. Recovery has to ignore it, otherwise the segment that + /// does hold footage is thrown away with it. + #[test] + fn empty_orphan_segment_directory_does_not_block_recovery() { + let directory = tempfile::tempdir().unwrap(); + let project = directory.path(); + + write_fragmented_display(&project.join("content/segments/segment-0")); + std::fs::create_dir_all(project.join("content/segments/segment-1")).unwrap(); + write_recording_meta(project, 1); + + let incomplete = RecoveryManager::inspect_recording(project) + .expect("a recording with one recoverable segment must be detected"); + assert_eq!( + incomplete.recoverable_segments.len(), + 1, + "the empty segment directory must not be collected" + ); + + RecoveryManager::require_recoverable_tracks(&incomplete) + .expect("an empty orphan segment directory must not make the recording unrecoverable"); + } + + /// The guard the fix must not disarm: a segment directory that still holds + /// footage but never made it into the recoverable set is a real + /// inconsistency, and recovering around it would silently drop that + /// footage from the recording. + #[test] + fn segment_directory_with_footage_outside_the_recoverable_set_is_still_fatal() { + let directory = tempfile::tempdir().unwrap(); + let project = directory.path(); + + write_fragmented_display(&project.join("content/segments/segment-0")); + write_fragmented_display(&project.join("content/segments/segment-1")); + write_recording_meta(project, 2); + + let mut incomplete = RecoveryManager::inspect_recording(project) + .expect("both populated segments must be detected"); + assert_eq!(incomplete.recoverable_segments.len(), 2); + + incomplete + .recoverable_segments + .retain(|segment| segment.index == 0); + + let error = RecoveryManager::require_recoverable_tracks(&incomplete).unwrap_err(); + + assert!( + error + .to_string() + .contains("Unrecoverable display segment 1"), + "a populated but uncollected segment must still fail validation, got: {error}" + ); + } +} + #[cfg(test)] mod required_track_failure_recovery_tests { use super::*; From 615e4677fcc94b84eed3435c98b50699dfd3822e Mon Sep 17 00:00:00 2001 From: iwillwin-wcy <13968663655@163.com> Date: Thu, 8 Oct 2026 19:15:25 +0800 Subject: [PATCH 2/2] fix(recording): widen the orphan check to every track, repair the guard 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 --- crates/recording/src/recovery.rs | 125 ++++++++++++++++++++++--------- 1 file changed, 90 insertions(+), 35 deletions(-) diff --git a/crates/recording/src/recovery.rs b/crates/recording/src/recovery.rs index 0f9300fe92e..6098bf90047 100644 --- a/crates/recording/src/recovery.rs +++ b/crates/recording/src/recovery.rs @@ -368,7 +368,8 @@ impl RecoveryManager { .and_then(|s| s.parse().ok()) .unwrap_or(0); - let display_info = Self::segment_display_fragments(&segment_path); + let display_info = + Self::segment_track_fragments(&segment_path, "display", "display.mp4"); let display_fragments = display_info.fragments; let display_init_segment = display_info.init_segment; @@ -380,17 +381,12 @@ impl RecoveryManager { continue; } - let camera_dir = segment_path.join("camera"); - let (camera_fragments, camera_init_segment) = { - let camera_info = Self::find_complete_fragments_with_init(&camera_dir); - if camera_info.fragments.is_empty() { - ( - Self::probe_single_file(&segment_path.join("camera.mp4")).map(|p| vec![p]), - None, - ) - } else { - (Some(camera_info.fragments), camera_info.init_segment) - } + let camera_info = Self::segment_track_fragments(&segment_path, "camera", "camera.mp4"); + let camera_init_segment = camera_info.init_segment; + let camera_fragments = if camera_info.fragments.is_empty() { + None + } else { + Some(camera_info.fragments) }; let mic_fragments = Self::find_audio_fragments(&segment_path.join("audio-input")); @@ -608,18 +604,21 @@ impl RecoveryManager { } } - /// The display media a single segment directory holds, or no fragments when - /// the segment has nothing recoverable. Shared by segment collection and by - /// recovery validation so neither can disagree with the other about which - /// segments count as recoverable. - fn segment_display_fragments(segment_path: &Path) -> FragmentsInfo { - let info = Self::find_complete_fragments_with_init(&segment_path.join("display")); + /// The fragmented media a single track of a segment directory holds, falling + /// back to the equivalent single file. Shared by segment collection and by + /// recovery validation so neither can disagree about what a segment holds. + fn segment_track_fragments( + segment_path: &Path, + track: &str, + single_file: &str, + ) -> FragmentsInfo { + let info = Self::find_complete_fragments_with_init(&segment_path.join(track)); if !info.fragments.is_empty() { return info; } - match Self::probe_single_file(&segment_path.join("display.mp4")) { + match Self::probe_single_file(&segment_path.join(single_file)) { Some(fragment) => FragmentsInfo { fragments: vec![fragment], init_segment: None, @@ -631,6 +630,30 @@ impl RecoveryManager { } } + /// Whether a segment directory holds any recoverable media at all. + /// + /// A directory with nothing in it is the residue of a resume that failed + /// before it wrote a frame, and recovery can ignore it. One holding any + /// track at all is a real inconsistency: skipping it would leave that + /// footage out of the recovered recording, so it has to be surfaced rather + /// than waved through. + fn segment_holds_recoverable_media(segment_path: &Path) -> bool { + for track in ["display", "camera"] { + let single_file = format!("{track}.mp4"); + + if !Self::segment_track_fragments(segment_path, track, &single_file) + .fragments + .is_empty() + { + return true; + } + } + + ["audio-input", "system_audio"] + .iter() + .any(|track| Self::find_audio_fragments(&segment_path.join(track)).is_some()) + } + fn collect_respawn_groups( dir: &Path, health_tx: Option<&HealthSender>, @@ -1346,18 +1369,16 @@ impl RecoveryManager { } // An interrupted resume, a crash or a power loss can leave a segment - // directory behind that holds no recoverable display media. It - // contributes nothing to the recording, so ignoring it is what lets - // the segments that do hold footage be recovered. Only a directory - // that still has display media but was not collected is fatal. - if Self::segment_display_fragments(&entry.path()) - .fragments - .is_empty() - { + // directory behind that holds no media at all. It contributes + // nothing to the recording, so ignoring it is what lets the segments + // that do hold footage be recovered. A directory holding any + // recoverable track is not that, and stays fatal: recovering around + // it would silently drop that footage from the recording. + if !Self::segment_holds_recoverable_media(&entry.path()) { warn!( segment = index, path = %entry.path().display(), - "Ignoring segment directory without recoverable display media" + "Ignoring segment directory without recoverable media" ); continue; } @@ -5238,14 +5259,14 @@ mod empty_orphan_segment_tests { use super::*; use cap_enc_ffmpeg::segmented_stream::{SegmentedVideoEncoder, SegmentedVideoEncoderConfig}; - /// Encode a short fragmented display track so the segment passes the same + /// Encode a short fragmented video track so the segment passes the same /// media probes a real recording does. - fn write_fragmented_display(segment_path: &Path) { + fn write_fragmented_track(segment_path: &Path, track: &str) { ffmpeg::init().unwrap(); std::fs::create_dir_all(segment_path).unwrap(); let mut encoder = SegmentedVideoEncoder::init( - segment_path.join("display"), + segment_path.join(track), cap_media_info::VideoInfo { pixel_format: cap_media_info::Pixel::NV12, width: 320, @@ -5324,7 +5345,7 @@ mod empty_orphan_segment_tests { let directory = tempfile::tempdir().unwrap(); let project = directory.path(); - write_fragmented_display(&project.join("content/segments/segment-0")); + write_fragmented_track(&project.join("content/segments/segment-0"), "display"); std::fs::create_dir_all(project.join("content/segments/segment-1")).unwrap(); write_recording_meta(project, 1); @@ -5344,14 +5365,18 @@ mod empty_orphan_segment_tests { /// footage but never made it into the recoverable set is a real /// inconsistency, and recovering around it would silently drop that /// footage from the recording. + /// + /// The metadata deliberately declares a single segment so that validation + /// reaches the directory-level guard. Declaring two would fail earlier with + /// "Missing display segment 1" and never exercise the check under test. #[test] fn segment_directory_with_footage_outside_the_recoverable_set_is_still_fatal() { let directory = tempfile::tempdir().unwrap(); let project = directory.path(); - write_fragmented_display(&project.join("content/segments/segment-0")); - write_fragmented_display(&project.join("content/segments/segment-1")); - write_recording_meta(project, 2); + write_fragmented_track(&project.join("content/segments/segment-0"), "display"); + write_fragmented_track(&project.join("content/segments/segment-1"), "display"); + write_recording_meta(project, 1); let mut incomplete = RecoveryManager::inspect_recording(project) .expect("both populated segments must be detected"); @@ -5370,6 +5395,36 @@ mod empty_orphan_segment_tests { "a populated but uncollected segment must still fail validation, got: {error}" ); } + + /// A directory holding only a track other than the display is not an empty + /// orphan. Skipping it would let recovery publish a recording that silently + /// omits that footage, so it stays fatal. + #[test] + fn segment_directory_holding_only_non_display_media_is_still_fatal() { + let directory = tempfile::tempdir().unwrap(); + let project = directory.path(); + + write_fragmented_track(&project.join("content/segments/segment-0"), "display"); + write_fragmented_track(&project.join("content/segments/segment-1"), "camera"); + write_recording_meta(project, 1); + + let incomplete = + RecoveryManager::inspect_recording(project).expect("the display segment must be found"); + assert_eq!( + incomplete.recoverable_segments.len(), + 1, + "a camera-only segment has no display fragments to collect" + ); + + let error = RecoveryManager::require_recoverable_tracks(&incomplete).unwrap_err(); + + assert!( + error + .to_string() + .contains("Unrecoverable display segment 1"), + "a segment holding camera media must still fail validation, got: {error}" + ); + } } #[cfg(test)]