Repository navigation
Conversation
Quest-Claim: 50dcfde0d8f48542
export mp4 holds an exclusive file lock on its output while it records, and repair mp4 refuses a file it cannot lock. Neither runs on a filesystem without locks. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 36 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No blocking issues. One Windows-specific rough edge is noted inline.
Reviewed changes
I reviewed the advisory lock that moq export mp4 now holds on its output, and the matching lock check in moq repair mp4.
- Recorder lock:
run_mp4takesFile::try_lockright aftercreate_newand keeps the lock until the file closes. If it can't lock, it deletes the new file and fails. - Repair refusal:
Mp4::runlocks before it scans.WouldBlockgets a clear "still open by a recorder" message, and any other lock error stops the repair instead of letting it run unlocked. - Tests: a new fast test holds the lock from the test process. The nightly
a_killed_mp4_recording_is_repairedtest now tries a repair mid-recording, and a helpermoq_repairreplaces the inline command it used before. - Docs:
doc/bin/cli.mddescribes the lock and the refusal.
claude-opus-5-5 ｜ 𝕏
| // Held until the file closes, which `moq repair mp4` reads as a recorder still writing. | ||
| // The OS drops it with a killed recorder, so that recording stays repairable. A file that | ||
| // can't be locked could never be repaired safely, so it isn't recorded. | ||
| if let Err(err) = file.try_lock() { |
There was a problem hiding this comment.
On Windows, File::try_lock calls LockFileEx with LOCKFILE_EXCLUSIVE_LOCK, and that lock is mandatory. While a recording runs, every other process gets ERROR_LOCK_VIOLATION when it reads the file, so copying it, previewing the partial fragmented MP4, or letting a sync or backup client read it all fail. Unix keeps working because flock is only advisory. moq ships for Windows through winget, and export.rs is #![cfg(unix)], so no test would catch this.
Technical details
# Exclusive recorder lock blocks readers on Windows
## Affected sites
- rs/moq-cli/src/subscribe.rs:397 — `file.try_lock()` is exclusive and mandatory on Windows
- doc/bin/cli.md:165 — says "advisory" in effect; on Windows the lock also blocks reads
## Required outcome
- Other processes can still read an in-progress recording on Windows, or the docs state that they can't.
## Suggested approach (optional)
- Have the recorder take `try_lock_shared()` and have repair take the exclusive `try_lock()`. A shared lock still makes repair's exclusive attempt fail with `WouldBlock`, and the OS still drops it when the recorder is killed. On Windows a shared `LockFileEx` lock allows reads by other processes and blocks only writes, which the recorder doesn't need from anyone else. Two concurrent repairs still refuse each other.
- Note: on Windows a shared lock also blocks writes through the locking handle itself. Verify that the recorder's own writes still succeed, or keep the exclusive lock and document the Windows behavior instead.
## Open questions for the human (optional)
- Does reading a recording while it is still being written matter for Windows users?…error Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
moq export mp4 --outputnow holds an exclusive advisory file lock on its output while it records, andmoq repair mp4refuses a file it can't lock, with a message saying a recorder still has it open. Finishes quest/m1/mp4-repair-lock.md (deleted here), found by moq-dev/moq#7560.Summary
rs/moq-cli/src/subscribe.rs): aftercreate_new, takesFile::try_lockand holds it until the file closes. If it can't lock (no lock support), it removes the file it just created and fails: a recording that could never be repaired safely isn't started. The OS drops the lock with a killed recorder, so a SIGKILLed recording stays repairable.rs/moq-cli/src/repair.rs): takes the same lock non-blocking before scanning and holds it through the rewrite. Anytry_lockfailure, held or unsupported, refuses with that error (its Display says whether the file is in use), without repairing unlocked.moq-mux::repairis untouched (stillimpl Read + Seek).doc/bin/cli.md.Public API
None: CLI behavior only. Wire: none.
Validation
export::repair_refuses_a_locked_recording(fast, no relay, runs in the normal gate): the test process holds the lock, the builtmoq repair mp4binary exits non-zero with the lock message and leaves the file byte-identical; after release it proceeds to scan.export::a_killed_mp4_recording_is_repaired(just rs cli-export, ran locally, all 16 pass): repair mid-recording against the realexport mp4process refuses and does not rewrite the file; after SIGKILL the same repair succeeds (the lock died with the process).TryLockError::Errorarm and the recorder's remove-and-fail.just checkpasses.Choices for review
repairrefuses on anytry_lockfailure and reports the error itself, so held and unsupported share one message ("failed to lock FILE, so it may still be open by a recorder").repairtakes the exclusive lock too, so two concurrent repairs also refuse each other .repairin that instant sees a just-created empty file and fails to scan it rather than corrupting anything. Not worth closing (would need creating under a lock).Follow-ups
None.
Review
Rewritten by
quest reviewafter each round.Head: 563dbaf
Base: origin/main at 250a13a
Risk: medium,
the base has no RISK.md to name what is always high or customer-facing, so every change is mediumClassified: medium by default
Reviewer: Codex, gpt-6.1-sol, medium effort
Session:
01a1df3b-92ed-7330-b42b-1ccfa6b567e0, round 2Score: 9/10,
The delta preserves lock refusal while simplifying error handling and updating assertions consistently; source review found no actionable defects, and builds and tests were not run as instructed.Rounds: 2; first round 0; findings 0: fixed 0, refuted 0, open 0
Findings: none
Models
Size: XS
Implementer: Claude Code, claude-sonnet-5-5, unknown effort (arm A)