diff --git a/crates/recording/src/recovery.rs b/crates/recording/src/recovery.rs index 9eddbcbeb2e..6098bf90047 100644 --- a/crates/recording/src/recovery.rs +++ b/crates/recording/src/recovery.rs @@ -368,18 +368,10 @@ 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_track_fragments(&segment_path, "display", "display.mp4"); + let display_fragments = display_info.fragments; + let display_init_segment = display_info.init_segment; if display_fragments.is_empty() { debug!( @@ -389,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")); @@ -617,6 +604,56 @@ impl RecoveryManager { } } + /// 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(single_file)) { + Some(fragment) => FragmentsInfo { + fragments: vec![fragment], + init_segment: None, + }, + None => 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 { + 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>, @@ -1318,17 +1355,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(()) } @@ -5197,6 +5254,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::*;