feat(archive): resume a recording on restart - #4114
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
MERGEPositive improvement? Yes. Restarting a recorder used to hit Worth the complexity? Yes. The hard parts are isolated where they belong: Different approach? The rejected alternatives (fail on first group below floor; auto-start a new recording; seed via a pristine-only Notes (non-blocking):
Tests cover resume + sequence continuity, DVR orphan cleanup, failed recovery deleting nothing, and windows longer than one checkpoint. This is an automated review, not the maintainer's decision |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6362dc5112
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let (checkpoint, sequence) = replay(store, timeline, first..=last, complete).await?; | ||
| (Some(checkpoint), sequence) |
There was a problem hiding this comment.
When the segment keys are contiguous but the newest object's decoded window ends anywhere other than last + 1, recovery still succeeds as long as each retained record matches its window index. For example, if segments/5 is damaged but validly restates only records 0 through 4, resume sets the next segment to 5; the next commit uploads its media objects and then collides with the existing segments/5, leaving new orphans and stopping the writer. Check that checkpoint.range.end == last + 1 here so this malformed prefix is rejected before accepting new work.
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
Writer::new now recovers a prefix that already holds a recording instead of refusing it: it replays the retained timeline from a checkpoint, continues at the next segment through the new timeline::Producer::resume, refuses groups at or below each track's largest stored group, and a DVR deletes unreferenced group objects one grace period after a successful recovery. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6362dc5 to
1aad278
Compare
|
Rebased onto The writer conflict kept both sides: enrollment still starts from each track's recovered floor, and the malformed-group timestamp check stays. Also rejected a recovered window that does not end at the next segment after the newest timeline object. Resuming there would write the next segment on top of one that already exists. Covered by (Written by Grok 4.7) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1aad278834
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| let last = objects.back().and_then(|object| object.groups.last()); | ||
| let sequence = last.map_or(Ok(0), |group| group.sequence.checked_add(1).ok_or(Error::Overflow))?; |
There was a problem hiding this comment.
When retained timeline objects have valid checkpoint payloads but a later object's group sequence is lower than an earlier object's, recovery accepts them and derives the resumed sequence solely from the newest object. The resumed writer can therefore reuse an existing group ID; Reader::replay treats that collision as moq_net::Error::Duplicate and skips the newly committed timeline group (rs/moq-archive/src/reader/mod.rs:200-203), potentially hiding the final resumed segments until a later checkpoint appears. Validate group sequences across the replayed objects before deriving the next value. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
1 participant
Problem
moq_archive::Writer::newrefused any prefix that already held a timeline, so a restarted recorder could not continue its own recording. A crash also left DVR orphans behind: expired objects whose deletion was interrupted, and uploads whose segment never committed.Approach
Writer::newnow recovers the prefix instead of refusing it (new privaters/moq-archive/src/recover.rs):segments/and every track'sgroups/. The segment IDs must be contiguous.timeline::Producer::resume. Stored timeline group sequences continue past the last stored one, so a reader's replayed timeline track keeps increasing..infoand timeline objects are never candidates.Writer::newbefore anything is scheduled, so a failed or incomplete recovery deletes nothing.Overlap decision: each track's floor is the largest group stored under it, orphans included. Groups at or below the floor are refused through the existing duplicate path. A source replaying its cache after a restart just skips what the recording already holds. A source whose group sequences restarted can't continue the recording and needs a new prefix. Alternatives considered:
Tests
Regression tests are in
writer.rs, run against the in-memory store:.infoand timeline objects.Unit tests also cover the moq-json and moq-mux seeding.
Impact
Public API (all additive except the removed archive error variant):
moq_json::window::Checkpoint<T> { range: Range<u64>, records: Vec<T> },Encoder::resume(config, &Checkpoint<T>) -> Result<Self>,Producer::resume(track, config, &Checkpoint<T>) -> Result<Self>.moq_mux::timeline::Producer::resume(broadcast, config, &Checkpoint<Record>) -> Result<Self>. A timeline's window index is its segment number, so the next record is segmentrange.end. A mismatched record fails with the newmoq_mux::Error::TimelineCheckpoint(index).moq_archive::Error::Occupiedis removed (0.0.x crate).Writer::newnow resumes instead of returning it.Wire: none. The draft's Retention and Bootstrap and Recovery sections already specify this behavior.
Alternatives
Checkpointmirrors the window header (offset/start/records), not a separaterange+ suffix. I keptrangeto matchEncoder::range()andwindow().Producerthrough a pristine-onlyseed()method instead of a constructor. Rejected because the constructor makes seeding a used window unrepresentable.Follow-ups
groups/from a lexical offset (asQuery::groups_fromdoes), plus the timeline keys, would make it proportional to what changed.Closes nothing. Deletes
quest/m1/archive/recovery.md, which unblocksquest/m1/archive/dvr.md.🤖 Generated with Claude Code
(written by Claude Opus 5.5)