[kixelated]

Finishes quest/m1/explicit-state/catalog-finish.md, following moq-dev/moq#8210.

Summary

moq_mux::catalog::Producer::finish stored a clean Closed before finishing either track, so a failed hang finish left the MSF track open while later calls reported Closed. Now it finishes both tracks under the state lock, and on the first failure aborts both catalog tracks with that error and stores it as the terminal outcome. A handle created after the failure (video(..., None) and its modify(), set, etc.) reports that retained error instead of NotPublished, per the plan update in moq-dev/moq#8214. New test a_failed_finish_is_final_and_ends_both_tracks pins the MSF abort cause and covers a late handle.

Public API: none. Wire: none.

Choices made without asking

• Finish runs while holding the state lock, so a concurrent modify cannot slip a publish in between the check and the outcome.

Models

• Size: XS
• Implementer: Claude Code, claude-opus-5-5, low effort
• Implementer: Claude Code, claude-opus-5-5, low effort (iterate, XS)

Review

• Reviewer: Codex, gpt-6.1-sol, medium effort
• Session: 01a1b9a1-8824-7031-a8ac-aa77780a2006, round 1
• Head: d31cc18
• Base: origin/main at be1c32f
• Score: 9/10, Source review found no actionable defects and supports landing, but builds and tests were not run as instructed.
• Findings: none

🤖 Generated with Claude Code

(Written by Claude Opus 5.5 with low)

[coderabbitai]

Warning

Review limit reached

You'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 16 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 57299474-f5ab-4329-8cf5-8877f4088a36

Commits

Reviewing files that changed from the base of the PR and between 4a86602 and d31cc18.

Files selected for processing: 4

• quest/m1/explicit-state/README.md
• quest/m1/explicit-state/catalog-finish.md
• rs/moq-mux/src/catalog/producer.rs
• rs/moq-mux/src/catalog/tracks.rs

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[kixelated]

Automated review by review (OpenAI)

Reviewed 35b4bdb.

No actionable findings. Finishing under the shared catalog lock prevents a concurrent modification between the openness check and terminal outcome. The failure path ends the remaining catalog track and preserves the first error for subsequent finish/modify calls, rather than recording a clean Closed before fallible work. Reusing the existing Outputs::abort path keeps the fix local and consistent with publish failures.

The paused-clock regression exercises a failed hang finish, checks that the MSF reader ends with an error, and verifies that later operations retain the failure. This is the right direction for the catalog's shared lifecycle.

Verification limits: GitHub-only static review of the focused diff, catalog state/guard/output paths, and snapshot/track finish and abort implementations. No builds, tests, stress runs or interop checks performed. Rechecked open/non-draft state, full head and published reviews immediately before this COMMENT.

[pullfrog]

ℹ️ No critical issues — one minor test suggestion inline.

Reviewed changes

• finish records its terminal state after the work: catalog::Producer::finish now finishes the hang and MSF tracks under the state lock, stores Closed only on success, and on the first failure aborts both catalog tracks with that cause and stores it, so later finish/modify calls report the failure instead of a clean Closed.
• Regression test: a_failed_finish_is_final_and_ends_both_tracks makes the hang finish fail, then checks the MSF consumer ends with an error and both later calls repeat the cause.
• Quest cleanup: deletes quest/m1/explicit-state/catalog-finish.md and its README.md entry, with no dangling references left behind.

Beyond the diff I checked the surrounding shape this joins: abort, Guard's drop-time publish failure, and flush_if_ready already abort both tracks and store the cause, so finish is now consistent with them. Holding the state lock across finish adds no deadlock — the existing order is catalog state lock → moq_json producer lock (Guard::publish → Outputs::emit), and the JSON guard is never held while taking the catalog lock. moq_json::snapshot::Inner::finish already aborts its own track on a partial failure, so the follow-up Outputs::abort is idempotent for the hang track. A successful finish still stores Closed, so rs/moq-ffi's second finish keeps returning MoqError::Closed.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes

Reviewed d31cc1829, the follow-up that makes a handle created after a failed finish report that failure.

• Producer::closed() exposes the terminal outcome: a pub(crate) accessor that clones State::closed, giving the rendition layer the reason the catalog ended.
• Unpublished handles report the retained cause: Rendition::unpublished() returns the catalog's terminal error, else NotPublished, and both config() and stage() now use it, so a video(..., None) handle made after a failed finish fails with the finish cause from modify(), set_bitrate, and replace instead of NotPublished.
• Test pins the abort cause and the late handle: the vacuous Closed assertion is gone, the MSF drain breaks only on Unauthorized (panicking on a clean end or any other cause), and a late video(..., None) handle is asserted to repeat the cause.

I checked the two seams this touches. The new closed() lock is never taken with the catalog state lock already held — every caller of Rendition::config()/stage() (container::Producer::modify, reconfigure, set, estimate, supply_bitrate, replace) runs outside it. reconfigure's Err(crate::Error::NotPublished) => self.set(config) sentinel stays sound: when the catalog is closed, set would return the same terminal error, so the branch is only taken while the catalog is still open. A clean finish now makes an unpublished handle report Closed rather than NotPublished, which matches Producer::modify. The Outputs::abort cause mapping the test pins is real — a failed hang.finish() surfaces as Error::Json(Net(Unauthorized)), and Error::reason() forwards the inner transport error.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}

[kixelated]

Automated review by review (OpenAI)

Reviewed d31cc18, focusing on the delta since 35b4bdb and its interaction with the base.

No actionable findings. The direction is sound: unpublished renditions now consult the existing shared terminal outcome, preserving the finish failure while retaining NotPublished for an open catalog. The accessor adds no separate lifecycle state or nested lock in the affected call paths. The regression now pins the MSF abort cause to Unauthorized and covers a late handle's modify failure, addressing the earlier test suggestion.

Verification limits: GitHub-only static review of the delta, catalog and rendition callers, and snapshot finish/abort and error-mapping paths. No builds, tests, stress runs, or interop checks performed. Rechecked open/non-draft state, full head, and published reviews immediately before this COMMENT.