fix(mux): include timestamp rewind boundary details

[kixelated]

Problem

A refused timestamp rewind names neither the frame timestamp nor the bound it crossed, leaving reports such as #4635 without enough detail to diagnose. MoQ does not support rewinds; this error refuses them, so the message should explain itself.

Approach

Carry the refused timestamp and the floor (the previous group's start, the lowest allowed timestamp) in every producer and consumer TimestampRewind. Display both in microseconds without changing the group-start monotonicity rule. Cover a FLAC import::Track cut per frame with a one-microsecond backward step and both consumer error paths; remove the completed quest.

Impact

• Breaking Rust API on main: container::TimestampRewind changes from a unit struct to a struct with public timestamp and floor fields, both moq_net::Timestamp. The name floor matches the producer's and consumer's existing internal bound.
• Error messages now include both values in microseconds; FFI callers already receive the message.
• Wire encoding and accepted timestamp sequences are unchanged.

Alternatives

• Reporting only the live content edge would misdescribe overlapping groups; report the actual previous-group-start bound used for rejection.
• Naming the field edge or group_start: floor names its role and matches existing code.

Validation

The new FLAC regression failed with the old diagnostic, then passed with the fix. Nix just check passes.

Follow-ups

• Stereo FLAC and Opus issues #4635 remains open: its underlying timestamp sequence has not been reproduced.
• moq-cli::export a_clean_finish_exits_zero_once_the_linger_expires is a pre-existing timing flake (measures the linger after publisher exit); unrelated to this PR.

🤖 Generated with Claude Code

(Written by GPT-6, updated by Claude Opus 5.5)

[kixelated]

Completed the timestamp rewind diagnostics quest. The FLAC cut-per-frame regression reproduced the missing timestamp values before the fix; Nix just check now passes. Producer and both consumer paths report the refused timestamp and the previous group’s start in microseconds, preserving the current grouping rule.

Public API: breaking Rust change, so this draft targets dev; TimestampRewind gains typed timestamp and edge fields. Wire: unchanged. #4635 remains open pending the reporter’s underlying sequence.

Recommended next action: review this draft PR, then run /quest-merge once review and CI pass. No new follow-up quest is needed.

(Written by GPT-6)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 08a1151

No actionable introduced bugs found. The producer (rs/moq-mux/src/container/producer.rs:411–422) and both consumer paths (consumer.rs:238–241,495–498) preserve their previous comparisons and attach the exact timestamp and bound used for rejection. Reporting the previous group’s start is appropriate for dev’s overlapping-group semantics. The typed fields retain the original timestamps, while Display consistently converts both to microseconds; the existing FFI error conversion preserves that message.

Direction is sound and focused: enrich the error at its source without changing grouping or wire behavior. The public struct change correctly targets dev. The FLAC cut-per-frame regression exercises a one-microsecond rewind at a large timestamp, and the strengthened consumer tests check both fields and messages. Keeping #4635 open is correct because this improves diagnosis without reproducing its original failure.

Verification: static review of the complete diff, producer/consumer state, regression coverage, error conversions, and the original issue. Exact-head Check and Platform CI were still running when inspected. I did not run local checks or reproduce #4635; the reported local test results are author-provided.

(Written by review (OpenAI))

[kixelated]

CI investigation: the failed export assertion is an unrelated baseline timing race.

• Failed CI job: the export entered its 1 s linger at 15:56:54.586175 and reported expiry at 15:56:55.653815, a 1.067640 s wait. It exited successfully; only the test's lower-bound timing assertion failed.
• The test records Instant::now() after awaiting the publisher process exit. The export starts lingering when the broadcast finishes, before publisher shutdown completes. A shutdown or scheduling delay consumes the interval before the test starts measuring.
• rs/moq-cli/tests/export.rs and the linger implementation in rs/moq-cli/src/subscribe.rs are unchanged from the exact PR base 012fc8904.
• The unchanged base test passed once locally. On that same base, a scratch-only experiment emulated a delayed test task between publisher exit and Instant::now(); the same assertion failed (nextest exit 100), with the export still exiting successfully. No production timeouts or sleeps were changed. The experiment is not part of this PR.
• Searched open PRs and quests on dev and origin/main. test(cli): run fetch and completion operations on paused time #4660 and /quest/m1/test-flakes-2/cli-paused-clock.md cover fetch/completion, not this export assertion. No existing fix or owner was found for this race.

This diagnostics PR remains blocked by CI; no flaky rerun was requested. Recommended separate fix: measure from before closing publisher stdin, which triggers the broadcast finish, so publisher shutdown cannot consume the asserted linger interval. Keep the current linger and assertion duration. No change to this PR's diagnostics scope.

(Written by GPT-6)

[kixelated]

Code review (head 08a1151de1aa2ac6f9819d2ca2a0f0bc67a10a96)

MERGE

TimestampRewind grows timestamp / edge and Display names both in µs. Producer write and both consumer refusal paths (poll_read floor check, poll_malformed min-timestamp check) keep the same comparisons and attach the exact values used for rejection. In-repo matches stay compatible (Error::TimestampRewind(_) in moq-gst); consumer + FLAC cut-per-frame tests lock fields and message text. Breaking unit→struct on dev is called out; wire and acceptance rules unchanged.

Non-blocking

• CI Test red, unrelated: moq-cli::export a_clean_finish_exits_zero_once_the_linger_expires failed on a linger lower-bound timing race (export.rs / subscribe linger unchanged vs base 012fc8904). Check / Android / macOS / Windows are green. Does not block this diagnostics change.
• Producer rewind tests still only matches!(… TimestampRewind(_)); consumer + import cover the new fields/message. Optional tighten later.

No concrete bugs in the diagnostic payload or call sites. Keeping #4635 open is right.

This is an automated review, not the maintainer's decision
(Written by Grok)

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 7ae1ca89-3969-43ee-b98d-6c5e6ed4607f

📥 Commits

Reviewing files that changed from the base of the PR and between 08a1151 and 48b2cad.

📒 Files selected for processing (7)

• doc/lib/rs/moq-mux.md
• quest/m1/README.md
• quest/m1/timestamp-rewind-detail.md
• rs/moq-mux/src/container/consumer.rs
• rs/moq-mux/src/container/mod.rs
• rs/moq-mux/src/container/producer.rs
• rs/moq-mux/src/import/track.rs

💤 Files with no reviewable changes (2)

• quest/m1/timestamp-rewind-detail.md
• quest/m1/README.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

---

Walkthrough

TimestampRewind now stores the refused frame timestamp and previous group start, and its error message reports both in microseconds. Producer and consumer paths populate these fields. Tests check error values and messages, including a FLAC importer rewind case. Documentation describes the error fields and rewind behavior. The m1 quest plan and its detail document were removed.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 48b2c

The rewind errors now provide the refused timestamp and comparison floor. No actionable issue identified here prevents merging.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 48b2c

The reviewed change improves diagnostics without changing timestamp rejection rules or introducing new access or authority. The Rust API change is intentional; callers constructing or matching the former unit struct must update.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The inspected diagnostic exposure is bounded to timing information already held by the affected producer or subscribed consumer track. No other-track lookup or new privileged sink is introduced in these paths. Production distribution of error messages was not inspected.

Trust Boundaries and Controls

• observed — Publisher-controlled timestamps continue through the existing strict-below-floor rejection checks. The new error payload neither changes those controls nor provides a new path to group creation, frame delivery, or authority acquisition.

Resilience and Maintainability Implications

• observed — Producer rewind rejection remains before cutting or opening a group. Consumer error construction adds no cursor, start, floor, live-edge, or cleanup mutation. Direct-path frame consumption before rejection and pending-path buffering already existed; repeated polling and interruption introduce no new recovery transition in this delta.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files. (1 skipped: 1 u…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: adding boundary details to timestamp rewind errors.
Description check	✅ Passed	The description explains the problem, implementation, API impact, validation, and unchanged wire behavior. It is directly related to the changeset.

✨ Finishing Touches

✨ Simplify code

• Commit to this branch
• Create a new PR

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

Autopilot is currently an internal CodeRabbit preview.

---

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

_{Comment @coderabbitai help to get the list of available commands.}

[kixelated]

Maintainer decision (recorded during /quest-merge):

Q: Field naming for the bound

• ✅ Rename edge to floor (name by role: the lowest allowed timestamp, matching the consumer's and producer's existing floor)
• Rename to group_start
• Land as-is
• Drop it

Context confirmed: MoQ does not support rewinds; this error refuses them, and the detail exists so reports like #4635 diagnose themselves.

Applied in 48b2cad.

(Written by Claude Opus 5.5)

[kixelated]

Code review (head 48b2cad85a1ae2dd070a33cb2d76d44b77dd591b)

MERGE — re-review after push from 08a1151de1aa2ac6f9819d2ca2a0f0bc67a10a96

Push renames TimestampRewind::edge → floor (struct field, Display, producer/consumer construction, docs, consumer + import tests) and merges origin/main (quest README conflict only; timestamp-rewind-detail stays deleted). Comparisons and payloads are unchanged; moq-gst still matches TimestampRewind(_) so it does not care about the field name.

Prior MERGE had no open findings to clear. Same optional NB remains: producer rewind tests still only matches!(… TimestampRewind(_)). CI still in progress on this head.

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 48b2cad

No actionable introduced bugs found in the update since 08a1151. After separating inherited main changes, the substantive API delta is TimestampRewind::edge becoming floor (rs/moq-mux/src/container/mod.rs:126-138). Display, producer construction, both consumer paths, tests, and documentation use the new field consistently; the rejected timestamps, comparisons, and error text remain unchanged.

Direction remains sound: floor matches the bound's role without adding complexity. The breaking struct change belongs on the current main trunk; wire behavior is unchanged. This independently agrees with the existing Grok confirmation, and #4635's underlying sequence remains unverified.

Verification: static comparison of the previous reviewed patch, current PR diff, and rename commit. Exact-head Android CI succeeded; Check and Platform were still running when inspected. No local tests were run.

(Written by review (OpenAI))

[kixelated]

Merge summary

• Merged main in (no rebase). Conflict was only quest/m1/README.md; the completed quest stays deleted, and main's Subscriber max-delay entry is kept.
• Renamed TimestampRewind::edge to floor per the decision above: struct field, doc comment, Display, producer and both consumer paths, tests, and doc/lib/rs/moq-mux.md. moq-gst matches TimestampRewind(_) and moq-ffi forwards the message, so neither needed changes.
• PR body: dev wording updated to main, floor reflected in Impact.
• CI: the earlier Test failure was moq-cli::export a_clean_finish_exits_zero_once_the_linger_expires, a pre-existing timing flake unrelated to this PR (the @moq/net lines in that log were noise). All checks pass on 48b2cad85.
• Reviews on the final head: no findings. The optional nit (producer rewind tests only match the variant) is left as is; consumer and import tests already check the fields.

Public API: breaking, TimestampRewind gains timestamp and floor. Wire: unchanged.

(Written by Claude Opus 5.5)