Repository navigation
fix(mux,audio,json): a failed finish is final - #8210
Conversation
Quest-Claim: 04025ca601968295
Importers (MKV, fMP4, TS, TS Programs) hold one `Input` state instead of a `finished` flag and a separate failure slot. A finish that fails ends every track and reports its cause to every later call; input after a successful finish is refused with `Error::Finished`. The audio encode producer keeps its codec in a `Codec` enum, so a failed finish refuses later writes with its cause instead of feeding input-rate PCM to the codec. moq-json's window and snapshot producers abort the track when finishing fails and report an abort's cause, matching snapshot's design. container::Producer resets a group's state before the fallible close and aborts the group or track when closing fails; transcode keeps a pending cut until it applies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
… finish owns its outcome FLV and TS passthrough imports take the shared Input state. moq-json's window and snapshot producers record their own outcome in a State enum, and finishing closes their group even when another handle already ended the track. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 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 23 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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head 4cdc050.
No actionable findings. Recording terminal outcomes after fallible teardown, ending the remaining importer tracks, and preserving the first failure is a sound direction. The separate Refused state retains the existing contract that accepted media can still be finished after a decode refusal. The JSON producers now distinguish their own terminal state from an end declared by another track handle, and the audio producer cannot reuse its codec after a failed drain. Keeping a pending transcode cut until it succeeds also preserves the owed boundary.
Verification limits: GitHub-only static review of all 26 changed files and surrounding importer, group-close, audio-drain and JSON lifecycle code. No builds, tests, media playback, or failure-injection runs performed; reported runtime validation remains author-provided. Rechecked open/non-draft state, full head/base and published reviews immediately before posting.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The full diff at 4cdc05069, including all three fix commits and the two prior review rounds' fixes, plus the changed files in context. Ran the moq-json, moq-audio, and moq-transcode unit suites and the moq-mux library suite: everything passes except 22 moq-mux tests that shell out to ffmpeg/ffprobe, which are absent from this environment (recovery_test.rs:220, the export ffprobe helpers). Both old error variants are confirmed absent from release, as the Impact section claims.
- Shared importer state —
container::Input { Open, Refused, Failed, Finished }replaces thefinishedflag plusfailed: Optionin MKV, fMP4, FLV, TS import, TSPrograms, and TS passthrough;check()runs before every decode/seek/cut entry point, andFailedis terminal across all six. - Failed finishes end their tracks — fMP4/MKV/FLV end every track with
result.and(track.end())and keep the first error; TS routes a failedend()throughfail(), aborting streams, sections, SI, and releasing reservations; TSProgramsdoes the same per program. container::Producer— group state is taken before the fallible close, a close failure aborts the group and clears the buffer, and a failed last cut aborts the track rather than leaving it open; fMP4's passthrough track mirrors this.- Audio and JSON producers — the codec/state now records the terminal outcome (
Codec::Failed,State::Aborted), so a failed finish fails the track and every later write/finish returns its cause instead of feeding the codec or republishing; a finished producer's own state now gates edits even below an end another handle declared. - Transcode rung —
pending_cutis owed before the old sink drains and cleared only once applied, so a failed drain or cut is retried.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
Completes quest/m1/explicit-state/finish-final.md (deleted here) from the explicit-state epic.
Summary
A finish that fails now leaves the object terminally failed, with every track ended, and reports the cause to later calls. Every container importer refuses input after finish the same way.
mkv::Import,fmp4::Import,flv::Import,ts::Import,ts::Programs,ts::Passthrough) hold one crate-privatecontainer::Input { Open, Refused(Error), Failed(Error), Finished }instead of afinishedflag beside afailed: Option.Refused: a decode was refused. Later input gets that error, andfinishstill ends the tracks with what was accepted, as before.Failed:finishfailed. Every track has ended, either finished or aborted with the cause. Every later call returns the cause.Finished: later input getsError::Finished, andfinishagain does nothing.finishused to setfinishedbefore its fallible steps, so a section or private PES left without a media timestamp left its streams open, andDropnever retried. Now a failure aborts every stream, section, and SI track and releases the reservations.decodechecks the state first.Programs::finishdoes the same for every program.initial_reservation == Nonenow means only "catalog published": the oldTracksAfterFinishis renamedLateTracks, and finish is tracked byInput.container::Producer:close()resets the group's state before its fallible steps and aborts the group if closing fails. A finish whose last cut fails aborts the track instead of leaving it open. The fMP4 passthrough track does the same.moq-audio): the codec now lives inenum Codec { Live(Encoder), Finished, Failed(Error) }. A failedfinishfails the track, so subscribers see an error after the published packets. After that,writeandfinishreturn the cause instead of sending input-rate PCM to the codec or republishing terminal packets.moq-json): both now record the producer's own outcome inenum State { Open, Finished, Aborted(Error) }, alongside the openness each reads from the track. Another handle may end the track first, so the track alone can't say what this producer did.abort, edits report the cause instead ofClosed.Choices made without asking
finish(self)not adopted. The quest offered a consuming finish as an m2 option. These types keep&mut selfon purpose:ts::Programs::finishdocuments holding the importer (withLinger) to keep finished tracks servable, and the audio and JSON producers document thatabortor reads still work after finish. With the explicit state,&mut selfcan no longer reach a half-finished object, so the breaking move would buy little. I dropped it and deleted the quest instead of moving it to m2.finishafter a refuseddecodeto end the accepted media and returnOk. I kept that contract with the separateRefusedvariant rather than makingfinishreturn the refusal.moq_mux::Error::Finished. It replacesfmp4::Error::MoovAfterFinishand MKV's finish meaning ofTracksAfterFinish. Neither variant is onrelease.anyhow, which isn'tClone. The first failing call returns the original error. Later calls return amoq_mux::Error::Othercarrying the full{:#}chain. Subscribers seeStreamError::Internal, asError::reasongives for non-transport errors.StreamError::Internal, so the packets already published stay readable.Impact
Public API (moq-mux, moq-audio, moq-json). None of the removed or renamed items is on the
releasebranch, so this is not breaking:moq_mux::Error::Finished.fmp4::Error::MoovAfterFinish, which is nowmoq_mux::Error::Finished.mkv::Error::TracksAfterFinishtoLateTracks. It now only means a TrackEntry after the catalog was published. Input after finish ismoq_mux::Error::Finished.cut/seek/discontinuity, MKV and FLVseek, and FLV/TS/Programs/passthroughdecodenow refuse after finish.decode(&[])after finish now errors.ts::Programs::finishcalled again returns the same broadcasts.finishreturnsOk.Wire: none.
Caused by
finishsetfinishedbefore its fallible steps.finishedflag was set beforetrack.finish(), andensure_openhid an abort's cause.encoderonly afterend()'s fallible steps, while moq-dev/moq#2992 takes the resampler before them.Validation
main:ts::import::test::a_failed_finish_aborts_every_track: the section track used to stay open, and a retried finish returnedOk.ts::import::test::input_after_finish_is_refusedmkv::import::tests::a_failed_finish_reports_its_causeencode::producer::tests::a_failed_finish_refuses_later_calls: a write after a failed finish used to returnOk.window::test::a_failed_finish_is_finalwindow::test::edits_after_an_abort_report_its_causeflv::import_test::input_after_finish_is_refusedts::passthrough_test::input_after_finish_is_refusedwindow::test::finish_closes_the_group_on_a_track_already_endedsnapshot::test::a_finished_producer_refuses_edits_below_a_declared_endjust checkpasses on the head: 5470 tests, plus clippy and fmt.Suggested follow-ups
finish(self)across producers and importers (m2): recommend dropping it. The explicit state already prevents half-finished objects, and holding a finished importer or producer is a documented use (see Choices).catalog::Producer::finishmarksclosedbefore its fallible track finishes (rs/moq-mux/src/catalog/producer.rs), which is shape 2 outside this quest's list. Recommend planning it under the explicit-state epic.Models
Size: M
Implementer: Claude Code, claude-opus-5-5, high effort
🤖 Generated with Claude Code
(Written by Claude Opus 5.5 with high effort)
Review
Rewritten by
quest reviewafter each round.01a1b857-ffc8-7141-ada7-e7773c9c2ac1, round 3The remaining lifecycle finding is fixed, with no additional issues found in the incremental source review; builds and tests were not run as instructed.rs/moq-json/src/snapshot/producer.rs:408:Enforce the snapshot producer's terminal state before updating;rs/moq-json/src/snapshot/producer.rs:358 now calls open() before encoding or writing, so the producer's Finished state rejects updates even below a future track boundary.