Skip to content

Conversation

@richiemcilroy
Copy link
Member

@richiemcilroy richiemcilroy commented Dec 29, 2025

Lots of different optimisations to the recording pipeline, with added fail-safes.

Greptile Summary

This PR significantly improves the recording pipeline's resilience and recoverability through defensive error handling and enhanced metadata tracking.

Key improvements:

  • Timestamp anomaly handling: Replaced hard errors with graceful fallbacks for clock skew issues across all output pipelines (macOS/Windows). When timestamp arithmetic fails (backward movement, overflow, underflow), the system now logs warnings and uses safe defaults (Duration::ZERO or Duration::MAX) instead of crashing.
  • Mutex poisoning resilience: Encoder mutex operations now explicitly handle poisoned mutex states, logging warnings and skipping frames rather than propagating panics.
  • Enhanced recovery metadata: Manifest format upgraded to v5 with codec info (dimensions, frame rate, time base, pixel format) embedded for better recovery capabilities.
  • Init segment validation: Added validate_init_segment() to detect missing/corrupt init.mp4 files that would make segments unplayable, with critical logging.
  • Comprehensive test coverage: Added 784 lines of recovery tests covering status transitions, manifest parsing, segment validation, and edge cases.

Technical notes:

  • The timestamp handling changes follow Rust best practices using saturating_sub and checked arithmetic with explicit fallbacks
  • Mutex error handling properly distinguishes between lock errors and poisoned state
  • Auto-generated TypeScript bindings reflect diagnostic API simplification

Confidence Score: 4/5

  • Safe to merge with minor validation needed for init segment error handling
  • The changes are well-structured defensive improvements that gracefully handle edge cases. Confidence is 4/5 rather than 5/5 due to one concern: the init segment validation logs critical errors but doesn't prevent segment writing, which could create unplayable segments. However, the overall architecture improvements significantly increase recording reliability.
  • Pay attention to crates/enc-ffmpeg/src/mux/segmented_stream.rs - validate_init_segment() error handling behavior

Important Files Changed

Filename Overview
crates/enc-ffmpeg/src/mux/segmented_stream.rs Added codec info to manifest v5 and init segment validation; improved recovery reliability
crates/recording/src/output_pipeline/core.rs Replaced timestamp errors with graceful fallbacks for clock skew anomalies; improved robustness
crates/recording/src/output_pipeline/macos_fragmented_m4s.rs Improved encoder mutex error handling with explicit poisoned mutex warnings
crates/recording/src/output_pipeline/win_fragmented_m4s.rs Improved encoder mutex error handling with explicit poisoned mutex warnings
crates/recording/tests/recovery.rs Comprehensive new test suite (784 lines) covering recovery scenarios, manifest parsing, and edge cases

Sequence Diagram

sequenceDiagram
    participant App as Recording App
    participant Pipeline as Output Pipeline
    participant PauseTracker as Pause Tracker
    participant Encoder as Segmented Encoder
    participant FS as File System
    participant Recovery as Recovery Manager

    Note over App,Recovery: Recording Start
    App->>Pipeline: Start recording with config
    Pipeline->>Encoder: Initialize encoder (base_path, video_config)
    Encoder->>FS: Create base directory
    Encoder->>FS: Write init.mp4 header
    Encoder->>Encoder: Store codec_info (width, height, fps, etc)
    Encoder->>FS: Write manifest.json (v5, codec_info included)

    Note over App,Recovery: Recording Frames
    loop For each frame
        App->>Pipeline: Process frame (timestamp)
        Pipeline->>PauseTracker: adjust(timestamp)
        alt Paused
            PauseTracker-->>Pipeline: None (skip frame)
        else Clock skew detected
            PauseTracker->>PauseTracker: Handle timestamp anomaly
            PauseTracker->>PauseTracker: Log warning, use fallback
            PauseTracker-->>Pipeline: adjusted_timestamp
        else Normal
            PauseTracker-->>Pipeline: adjusted_timestamp
        end
        
        alt Has adjusted timestamp
            Pipeline->>Encoder: queue_frame(frame, adjusted_timestamp)
            alt Encoder mutex poisoned
                Encoder->>Encoder: Detect poisoned mutex
                Encoder->>Encoder: Log warning
                Encoder-->>Pipeline: Skip frame gracefully
            else Normal encoding
                Encoder->>Encoder: Encode frame
                Encoder->>FS: Write to segment file
            end
        end
    end

    Note over App,Recovery: Segment Completion
    Encoder->>Encoder: Detect segment duration threshold
    Encoder->>FS: Finalize current segment
    Encoder->>Encoder: validate_init_segment()
    alt Init segment missing/corrupt
        Encoder->>Encoder: Log CRITICAL error
    end
    Encoder->>FS: Update manifest.json
    Encoder->>FS: Write in-progress manifest

    Note over App,Recovery: Recording Stop / Crash
    alt Normal stop
        App->>Pipeline: Stop recording
        Pipeline->>Encoder: finish_with_timestamp()
        Encoder->>FS: Finalize all segments
        Encoder->>FS: Write final manifest (is_complete: true)
    else Crash/Interrupt
        Note over Pipeline,FS: Incomplete segments left on disk
    end

    Note over App,Recovery: Recovery Flow
    Recovery->>FS: Scan for incomplete recordings
    Recovery->>FS: Load recording-meta.json
    alt Status requires recovery
        Recovery->>FS: Read manifest.json (v5 supported)
        Recovery->>Recovery: Parse codec_info from manifest
        Recovery->>FS: Validate init.mp4 exists
        Recovery->>FS: Check segment completeness
        Recovery->>Recovery: Build recoverable video
        Recovery->>FS: Update recording status
    end
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines 319 to 322

if let Err(e) = self.validate_init_segment() {
tracing::error!("{e}");
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: validation logs critical error but doesn't stop segment writing

If init.mp4 is missing/corrupt, subsequent segments will be unplayable. Consider returning Result from this function and handling the error in the caller to prevent creating unusable segments.

Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/enc-ffmpeg/src/mux/segmented_stream.rs
Line: 319:322

Comment:
**logic:** validation logs critical error but doesn't stop segment writing

If `init.mp4` is missing/corrupt, subsequent segments will be unplayable. Consider returning `Result` from this function and handling the error in the caller to prevent creating unusable segments.

How can I resolve this? If you propose a fix, please make it concise.

@richiemcilroy richiemcilroy merged commit 43280d1 into main Dec 29, 2025
14 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants