diff --git a/crates/recording/src/recovery.rs b/crates/recording/src/recovery.rs index a2efd2da32..4aac5550a4 100644 --- a/crates/recording/src/recovery.rs +++ b/crates/recording/src/recovery.rs @@ -398,28 +398,9 @@ impl RecoveryManager { .and_then(|s| s.parse().ok()) .unwrap_or(0); - let display_dir = segment_path.join("display"); - let display_info = Self::find_studio_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; - } - - if display_fragments.is_empty() - && let Some(info) = Self::respawn_directories(&display_dir) - .into_iter() - .map(|(_, directory)| Self::find_studio_fragments_with_init(&directory)) - .find(|info| !info.fragments.is_empty()) - { - display_fragments = info.fragments; - display_init_segment = info.init_segment; - } + let display_info = Self::segment_track_fragments(&segment_path, "display", true); + let display_fragments = display_info.fragments; + let display_init_segment = display_info.init_segment; if display_fragments.is_empty() { debug!( @@ -429,18 +410,10 @@ impl RecoveryManager { continue; } - let camera_dir = segment_path.join("camera"); - let (camera_fragments, camera_init_segment) = { - let camera_info = Self::find_studio_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", false); + let camera_init_segment = camera_info.init_segment; + let camera_fragments = + (!camera_info.fragments.is_empty()).then_some(camera_info.fragments); let mic_fragments = Self::find_audio_fragments(&segment_path.join("audio-input")); let system_audio_fragments = @@ -717,6 +690,56 @@ impl RecoveryManager { } } + /// One track of a segment directory: its fragments, else the single-file + /// fallback, else (when `respawn` is set) the first respawned muxer + /// directory holding fragments. 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, respawn: bool) -> FragmentsInfo { + let dir = segment_path.join(track); + let info = Self::find_studio_fragments_with_init(&dir); + if !info.fragments.is_empty() { + return info; + } + + if let Some(file) = Self::probe_single_file(&segment_path.join(format!("{track}.mp4"))) { + return FragmentsInfo { + fragments: vec![file], + init_segment: None, + }; + } + + if respawn + && let Some(info) = Self::respawn_directories(&dir) + .into_iter() + .map(|(_, directory)| Self::find_studio_fragments_with_init(&directory)) + .find(|info| !info.fragments.is_empty()) + { + return info; + } + + FragmentsInfo { + fragments: Vec::new(), + init_segment: None, + } + } + + /// 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 { + ["display", "camera"].iter().any(|track| { + !Self::segment_track_fragments(segment_path, track, true) + .fragments + .is_empty() + }) || ["audio-input", "system_audio"] + .iter() + .any(|track| Self::find_audio_fragments(&segment_path.join(track)).is_some()) + } + fn respawn_directories(dir: &Path) -> Vec<(u32, PathBuf)> { let Ok(entries) = std::fs::read_dir(dir) else { return Vec::new(); @@ -1539,17 +1562,37 @@ 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) - { - return Err(RecoveryError::Validation(format!( - "Unrecoverable display segment {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 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 media" + ); + continue; } + + return Err(RecoveryError::Validation(format!( + "Unrecoverable display segment {index}" + ))); } Ok(()) } @@ -4701,6 +4744,28 @@ mod clean_studio_snapshot_tests { assert!(!project.join(OUTPUT).exists()); } + #[test] + fn empty_orphan_segment_directory_recovers_every_real_frame() { + let directory = playable_studio_project(StudioRecordingStatus::InProgress); + let project = directory.path(); + fs::create_dir_all(project.join("content/segments/segment-1")).unwrap(); + let recording = RecoveryManager::inspect_recording(project).unwrap(); + assert_eq!(recording.recoverable_segments.len(), 1); + let recovered = RecoveryManager::recover(&recording).unwrap(); + validate_recovered_track(&project.join(OUTPUT), ffmpeg::media::Type::Video).unwrap(); + assert_eq!( + ffmpeg::format::input(&project.join(OUTPUT)) + .unwrap() + .packets() + .count(), + 60 + ); + let StudioRecordingMeta::MultipleSegments { inner } = recovered.meta else { + panic!("Expected segmented Studio metadata"); + }; + assert_eq!(inner.segments.len(), 1); + } + fn completed_preparing_fixture(project: &Path) -> CompletedRecording { use crate::studio_recording::CleanStoppedStudio; @@ -6088,6 +6153,179 @@ mod tests { } } +#[cfg(test)] +mod empty_orphan_segment_tests { + use super::*; + use cap_enc_ffmpeg::segmented_stream::{SegmentedVideoEncoder, SegmentedVideoEncoderConfig}; + + /// Encode a short fragmented video track so the segment passes the same + /// media probes a real recording does. + 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(track), + 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_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); + + 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. + /// + /// 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_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"); + 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}" + ); + } + + /// 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)] mod required_track_failure_recovery_tests { use super::*;