Skip to content

fix(kio)!: Instant has no panicking Duration arithmetic - #8220

Merged
kixelated merged 18 commits into
mainfrom
quest/m0/peer-arithmetic
Nov 10, 2026
Merged

kixelated merged 18 commits into
mainfrom
quest/m0/peer-arithmetic

Conversation

@kixelated

@kixelated kixelated commented Nov 8, 2026 •

Copy link
Copy Markdown
Collaborator

Completes quest/m0/peer-arithmetic.md, settling its last item, group u64::MAX accounting, per the maintainer decisions below, and deletes the quest.

Summary

kio::clock::Instant no longer has Add, AddAssign, Sub, or SubAssign with Duration, which panicked past the clock's range. It gains saturating_add and saturating_sub to go with checked_add and checked_sub. Instant - Instant stays: it already returns a saturating Duration.

Roughly 950 call sites across 160 files now use one of these. A compiler-driven rewrite turned each a ± d into a.saturating_*(d), and I reviewed every production site by hand:

  • Config and timer deadlines (reconnect, idle and keep-alive, drain, QUIC timers, pacers, shaper) saturate to the clock's last instant, centuries out, so a huge configured or negotiated duration means "effectively never" and cannot panic.
  • Lookback cutoffs (now - window) saturate to the clock's origin, which keeps everything.
  • Tests use saturating_*, since overflow never happens there.

Peer-reachable sites get explicit handling and a test at the extreme value:

  • IETF REQUEST_ERROR retry interval (ietf/publisher.rs refusal): an interval past the clock's range is Refused::Never, the same as 0, instead of panicking the publisher. Test: a_refusal_past_the_clock_s_range_is_not_retried (retry interval 2^62-1).
  • Joining FETCH save_join: largest.object + 1 becomes checked_add(1).unwrap_or(0). End Object 0 reads the whole group, which is exactly the prefix through Object ID u64::MAX. The test also caught a second overflow on the same input: model/group.rs Prefetch::fill_log iterated (index..), which overflows at u64::MAX, so it now iterates index..=u64::MAX. Test: a_joining_fetch_through_the_last_object_id_reads_its_whole_group.
  • Router seqno request (route/mod.rs requests): a route starved at seqno MAX sends no request, because its origin rotates its identity instead. Test: a_route_starved_at_the_last_seqno_requests_nothing.
  • lite-05/06 datagram sequence ≥ 2^62 (lite/publisher.rs DatagramSend::send): send now returns the encode error. A datagram-only lite-05/06 subscription ends with that error, the lite-07 Datagrams stream does the same (unreachable, since its varints carry any u64), and a datagram track beside a stream track stops its datagrams while the subscription continues. Before, these were dropped silently. Test: a_datagram_past_the_wire_s_sequences_fails.

Docs: doc/lib/rs/kio.md explains the missing operators and fixes its example.

Choices made without asking

  • I added saturating_add/saturating_sub to Instant, matching Wall::saturating_add. Without them, about 900 sites would need checked_add(..).unwrap_or(..).
  • Config deadlines saturate rather than use checked_add and turn into "no deadline". A saturated deadline is a real deadline at the clock's last instant (the_last_instant_is_reachable), roughly 450 years past a runtime's start, so in practice it means the same thing with less code.
  • live_edge's next still uses largest.object.saturating_add(1). At u64::MAX, a Next Object subscription starts at that object rather than past it. That is at most one duplicate object, and it never wraps.
  • Wall's own +/- with Duration are out of scope and unchanged.

Decisions

The maintainer's, 2026-11-07, replacing the earlier ## Waiting item on how group u64::MAX is accounted (option (a)).

  • Group u64::MAX is reserved, and ranges are half-open everywhere. create_group and append_group refuse it with Error::BoundsExceeded (js/net: RangeError from GroupProducer), and the bindings surface that error through their existing mapping. A received Group Stream naming it is refused as a malformed track, the way the model already refuses invalid input. The draft states it once, in the Group section.

    • Reversed tests: end_at_maximum_group_is_unbounded is now the_reserved_sequence_is_refused; a_missing_head_at_the_maximum_sequence_does_not_panic is replaced by a_group_at_the_reserved_sequence_is_refused on the receive path.
    • Removed last-group special cases: the reader cursor's next_sequence and skip_ordered floor are plain u64 instead of Option (where None meant past u64::MAX), Delivered::floor returns u64, the resume reader no longer inserts u64::MAX by hand, and the publisher, track, and miss judge drop their checked_add(1) guards on group sequences.
    • js/net: main's group sequences stop at 2^64-1, as a u64 does (from moq-dev/moq#8222) created group 2^64-1; it is folded into the reserved sequence is refused, which keeps its 2^64 and skip-bound assertions and covers createGroup, appendGroup, and the receive hook at the reserved sequence.
    • Kept: the subscription window's handling of a user-supplied inclusive bound at u64::MAX (still a valid way to say "unbounded"), and fetch_group(u64::MAX) failing at once.
  • lite-07 SUBSCRIBE_DROP and SUBSCRIBE_SKIP encode Group Start + Group Count. Group Count = 0 is empty, and Start + Count above 2^64-1 is a PROTOCOL_VIOLATION. Earlier versions keep the inclusive Group Start/Group End. In Rust both messages hold groups: Range<u64>; in JS start/end are now half-open. The quest's bug is gone by construction: no range can end at or past the reserved group.

    • Precondition checked: lite-07 is unpublished. Its ALPN is moq-lite-07-wip, offered only on explicit request, in this checkout and on release (moq-net v0.3.21, @moq/net 0.4.7); AGENTS.md lets a -wip version change freely.
    • Not chosen: inclusive ranges internally; refusing only at decode; leaving the edge.
  • ✅ The quest file stays, rewritten to the JS half (maintainer, /quest-complete, 2026-11-09; rejected: converting the JS call sites in this PR; dropping it). Main added to quest/m0/peer-arithmetic.md that @moq/clock mirrors kio (Instant drops add/sub for checked and saturating forms). That is about 160 JS call sites, and JS Instant.add throws a RangeError rather than panicking, so it stays the [M] remainder of the quest, listed in quest/m0/README.md.

Impact

  • Public API (breaking, kio): Instant no longer implements Add<Duration>, AddAssign<Duration>, Sub<Duration>, or SubAssign<Duration>. New: Instant::saturating_add, Instant::saturating_sub.
    • Migration: replace at + d with at.checked_add(d) (None past the clock's range) or at.saturating_add(d). Replace at - d with at.checked_sub(d) or at.saturating_sub(d). at - earlier (Instant - Instant) is unchanged.
  • Public API (breaking, moq-net): lite::SubscribeDrop and lite::SubscribeSkip replace start/end (inclusive) with groups: Range<u64>; SubscribeDrop::new takes a Range. track::Producer::create_group refuses sequence u64::MAX, and append_group refuses to reach it, with Error::BoundsExceeded. js/net: SubscribeDrop/SubscribeSkip end is exclusive, SubscribeSkip.encode/decode take the version, and createGroup/appendGroup throw RangeError at 2^64-1. skipGroups, dropGroups, and refuseGroups throw RangeError for an end above 2^64-1 (it was 2^64), so a range never names the reserved group.
    • Migration: SubscribeDrop { start, end, .. } becomes SubscribeDrop { groups: start..end + 1, .. }; a producer that used group u64::MAX stops one short.
  • Wire (lite-07 only, unpublished -wip): SUBSCRIBE_DROP and SUBSCRIBE_SKIP carry Group Start + Group Count instead of an inclusive Group End. Group 2^64-1 is reserved: a Group Stream naming it is refused as MALFORMED_TRACK. Earlier versions are unchanged on the wire. Other behavior changes only at extreme values: a lite-05/06 datagram subscription ends with an encode error at a sequence ≥ 2^62, and an IETF retry interval past the clock's range is treated as "never retry".

Caused by

No single PR. The panicking operators are as old as kio's Instant, which mirrored std's.

Suggested follow-ups

  • js/net/src/connection/bandwidth.test.ts ("a connection samples the transport only while a reservation reads the estimate") fails in this worktree whenever it runs after ticker.test.ts, even with every js/ file restored to origin/main, and passes in a clean origin/main checkout. So the cause is environmental, not this PR's code. Recommend planning it: the test's sample count depends on something outside the source, which a test should not.

  • Wall's Add/Sub with Duration panic the same way (past 2554 or before 1970). Recommend dropping it: nothing peer-reachable feeds Wall arithmetic today.

Checks

At head d44db1c840 (main merged three more times, last at 66f4f16af0). The merges resolved: moq-mux's catalog test (main's Wall::MOQ_EPOCH with this PR's saturating_sub); main's new reconnect probe and deadline sites and their tests (saturating_add); the route test a_route_starved_at_the_last_seqno_requests_nothing, which now announces its origin since routes reach only interested nodes; main's Finished handle from finish(), keeping this PR's plain max + 1 because group u64::MAX is reserved, and deleting main's finish_past_the_last_sequence_aborts_with_the_cause, whose group the PR now refuses; main's mocked-clock rewrites of three relay/ffi tests; and moq-video v4l2's drain loops.

  • just check: pass at d44db1c840
  • just test interop --all: pass at d6699fbb9a (the last merge changed no wire code). Two flakes on reruns that passed: rust-swapped -> js-native-* audio gaps, and finish rust -> * (the Rust finish publisher never printed finish acknowledged; every reader ended clean).

Review coverage

The whole diff was reviewed by Codex gpt-6.1-sol at xhigh in two parts:

  • Part 1, quest review round 1 (session 01a1b9c2-24c3-74b1-839a-b4f5d2e79f7b): it diffed kio, doc, quest, drafts, js, moq-net's lite, ietf, route, and model, plus moq-mux, moq-congestion, moq-quic connection, moq-shaper, moq-webrtc, moq-srt, moq-audio, moq-video, moq-cli play, and moq-net's reconnect, ledger, goaway, and server, and scripted a pass over every non-test hunk under rs/.
  • Part 2, a separate read-only codex exec at the same model and effort, covering the other 85 files (test, bench, and integration files, and the remaining production files in other crates): it read all 2546 lines of their patch. Findings: none.

Models

Size: M
Implementer: Claude Code, claude-opus-5-5, high effort
Implementer: Claude Code, claude-opus-5-5, medium effort (iterate, M)
Implementer: Claude Code, claude-opus-5-5, low effort (iterate, S)

(Written by Claude Opus 5.5 with high)

🤖 Generated with Claude Code

Review

Rewritten by quest review after each round.

  • Reviewer: Codex, gpt-6.1-sol, medium effort
  • Session: 01a1c298-d934-7ab3-9930-29b75467e878, round 2
  • Head: d44db1c
  • Base: origin/main at 66f4f16
  • Score: 9/10, No actionable regressions were found in the merge resolutions or their interactions with the reviewed changes; builds and tests were not run, as instructed.
  • Findings: none

bcb0763 adds only the reviewer-suggested quest/m0/README.md bullet (Pullfrog, #8220 (comment)) on top of the reviewed head, so it needs no new round.

kixelated and others added 2 commits November 7, 2026 16:05
kio's Instant drops Add, AddAssign, Sub, and SubAssign with Duration, which
panicked past the clock's range. It gains saturating_add and saturating_sub,
and every site picks checked or saturating.

Peer-reachable sites get explicit handling and a test at the extreme value:
- An IETF REQUEST_ERROR retry interval past the clock's range is a refusal
  that is never retried, instead of panicking the publisher.
- A joining FETCH after an upstream's Largest Object of u64::MAX asks for
  the whole group, and the group cursor no longer overflows at that index.
- A router starved at the last seqno sends no request, since its origin
  rotates instead.
- A lite-05/06 datagram sequence of 2^62 or more ends the datagram
  subscription instead of dropping silently.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title kio's Instant has no panicking arithmetic, and peer values never wrap fix(kio)!: Instant has no panicking Duration arithmetic Nov 8, 2026
…ount

Group sequence 2^64-1 is never valid: create_group and append_group refuse it
in moq-net and js/net, and a received Group Stream naming it is refused as a
malformed track. Every group then fits a half-open range, so SUBSCRIBE_DROP
and SUBSCRIBE_SKIP hold half-open ranges and lite-07 encodes them as Group
Start + Group Count. A drop or skip ending at the last group no longer misses
it. The last-group special cases in the cursor, delivered set, and publisher
are gone.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated and others added 5 commits November 7, 2026 21:36
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts:
#	js/net/src/group.ts
#	rs/moq-congestion/src/gcc/tests.rs
#	rs/moq-congestion/src/pacer.rs
#	rs/moq-net/src/lite/publisher.rs
#	rs/qmux/src/driver.rs
@kixelated

Copy link
Copy Markdown
Collaborator Author

Design change: the maintainer settled how group u64::MAX is accounted (2026-11-07).

  1. Ranges are half-open everywhere, and the top sequence is reserved. Group 2^64-1 is never valid: create_group and append_group refuse it in moq-net (Error::BoundsExceeded) and js/net (RangeError), the bindings surface the same error, and a received Group Stream naming it is refused as a malformed track. The draft states it once, in the Group section. The two tests that created that group are reversed, and the last-group special cases they needed (the cursor's Option floors, Delivered::floor, the publisher's checked_add(1) guards) are removed.
  2. lite-07 SUBSCRIBE_DROP and SUBSCRIBE_SKIP encode Group Start + Group Count. Group Count = 0 is empty, and Start + Count above 2^64-1 is a PROTOCOL_VIOLATION. Earlier versions keep the inclusive encoding. lite-07 is still unpublished: it is offered only as moq-lite-07-wip, on explicit request, here and on release.

Not chosen: inclusive ranges internally; refusing only at decode; leaving the edge.

(Written by Claude Opus 5.5 with high)

@kixelated
kixelated marked this pull request as ready for review November 8, 2026 06:37

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed 74684ec.

No actionable findings. Instant's saturating methods reuse checked arithmetic and bound overflow at MAX/ZERO; the migrated deadline and lookback expressions preserve their normal-range behavior. IETF retry intervals beyond the clock range explicitly become Never rather than a panicking deadline.

The reserved group sequence and half-open range changes are consistent across the inspected Rust/JS model and lite paths. Lite-07 checks Start + Count, while earlier versions retain inclusive wire ends. The joining-fetch fix preserves the whole-group sentinel at the last Object ID, and exhausted route seqnos no longer request a wrapped successor. The documented decision to end an unencodable legacy datagram lane is retained.

Direction: explicit checked/saturating clock operations and a reserved group sentinel make these boundaries easier to maintain than per-call overflow exceptions. Keep the legacy-wire and extreme-value regressions with the changes.

Verification limits: GitHub-only static review of the clock implementation, broad arithmetic migration, protocol/model changes and relevant regression diffs. No builds, tests, feature-matrix, browser/native, interop or timing runs performed; reported validation and the bandwidth-test diagnosis were not independently reproduced. Rechecked open/non-draft state, full head and published reviews immediately before this COMMENT.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Minor suggestions only.

Reviewed changes

  • kio::clock::Instant: drops Add/AddAssign/Sub/SubAssign<Duration> and gains saturating_add/saturating_sub, so the ~950 rewritten call sites each state whether a duration past the clock saturates or is refused.
  • Reserved group u64::MAX: create_group/append_group refuse it, a received Group Stream naming it resets as MalformedTrack, and the reader cursor, Delivered, skip_ordered, and the miss judge drop their "past u64::MAX" sentinels in favor of the reserved sequence.
  • lite-07 wire: SUBSCRIBE_DROP/SKIP carry Group Start + Group Count with half-open Range<u64> in Rust and exclusive end in JS, while earlier versions keep the inclusive wire form; the draft states the rule once.
  • Peer-reachable extremes: IETF retry interval past the clock, joining FETCH through Object u64::MAX, router seqno at MAX, and lite-05/06 datagram sequences ≥ 2^62 each get explicit handling and a test.
  • Docs and cleanup: doc/lib/rs/kio.md explains the missing operators, and quest/m0/peer-arithmetic.md plus its milestone link are deleted.

The full diff was read end to end, including the mechanical hunks. The saturating rewrites preserve the original expressions, and the removed min(u64::MAX - 1) clamps in track/miss.rs stay safe because target() clips runs to window sequences, so u64::MAX is never an inclusive run end there. The sentinel conversions (next_sequence, skip_ordered, Delivered::floor) are consistent with the reservation, and Delivered::insert's sequence + 1 is unreachable at MAX because only real group sequences reach it.

ℹ️ The JS reserved-sequence guard is untested

Group.Producer's constructor now refuses 2^64-1 and both createGroup and the receive hook funnel through it, so the behavior is there. But nothing in js/net asserts it, while the Rust side has the_reserved_sequence_is_refused and a_group_at_the_reserved_sequence_is_refused, and this PR added a JS codec test that explicitly mirrors a Rust one. A regression that removes the constructor check would keep the whole JS suite green.

Technical details
# Add JS coverage for the reserved group sequence

## Affected sites
- `js/net/src/group.ts:246` — `if (checkSequence(sequence) >= RESERVED) throw ...` is the only enforcement; no test exercises it.
- `js/net/src/track.test.ts` / `js/net/src/group.test.ts` — no case at `RESERVED` or `RESERVED + 1n`.

## Required outcome
- A test pinning `new Producer(RESERVED)` (and `createGroup`/`appendGroup` reaching it) to `RangeError`, mirroring the Rust names so a reader can match the halves.
- Ideally one on the receive path too, asserting the group stream is refused while the track lives on, as the Rust test does.

## Suggested approach (optional)
- Reuse the pattern of the new `lite-07 skip ranges count up to the reserved group` test, which cites the Rust test it mirrors.

Pullfrog  | Fix it ➔ | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

@kixelated

Copy link
Copy Markdown
Collaborator Author

Maintainer decision (2026-11-08, /quest-complete): continue from this branch's head (ae4c12fb6c), including the lite-07 drop/skip count. The narrower alternative on quest-iterate/peer-arithmetic-max-group is not chosen and will be deleted. Still open before this can land: the Medium review finding that forwarding stops after a datagram whose sequence can't be encoded (rs/moq-net/src/lite/publisher.rs:2968).

(Written by Claude Opus 5.5 with high)

kixelated and others added 3 commits November 8, 2026 21:01
Rewrites the Instant ± Duration sites main added (moq-mux sync, moq-cli play,
moq-audio capture, moq-video v4l2, moq-webrtc ice, kio tests) to saturating_*,
and adds the JS reserved-sequence test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e test; fmt

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t it

skipGroups, dropGroups, and refuseGroups accepted an end of 2^64, which the
lite-07 encoder refuses once forwarded, ending the subscription.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Pullfrog's finding (no JS test for the reserved group sequence) is fixed in 1cb1d83: the reserved sequence is refused in js/net/src/track.test.ts mirrors the Rust tests, covering createGroup, appendGroup, and the receive hook at 2^64-1, and the range bounds of skipGroups.

(Written by Claude Opus 5.5 with medium)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed 1cb1d83 incrementally against the latest published review, accounting for merged main changes.

No actionable findings in the incremental update. The JS exclusive group-range end now stops at the reserved sequence (2^64-1), allowing the last valid group while refusing ranges that include the reserved group. The consolidated regression checks creation, append exhaustion and both range-end boundaries. Also checked the merged clock arithmetic adaptations; the remaining follow-up formatting changes do not change their behavior.

Verification limits: GitHub-only static source/diff review. No builds or tests were run; author-reported validation was not independently reproduced.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Merged newest origin/main and applied the Instant arithmetic migration to the code main added since the prior review: moq-mux sync tests and bench, moq-cli play, moq-audio capture tests, moq-video v4l2 Phase state, moq-webrtc ICE Phase, and kio's installed/sim clock tests. The resolutions keep main's new lifecycle APIs with saturating_* in place of every ±, and keep the reserved-group check beside Outcome::Sealed in moq-net's model/track.rs.
  • Tightened checkEnd so an exclusive group end above RESERVED (2^64-1) is refused, matching encodeGroups/decodeGroups. skipGroups, dropGroups, and refuseGroups can no longer record a range that the lite-07 Group Start + Group Count encoder would refuse later, and the bound is correct for the inclusive pre-lite-07 ends too (max group 2^64-2).
  • Folded main's group-bound test into the reserved sequence is refused, which now covers createGroup at 2^64 and at RESERVED, the receive hook at RESERVED, appendGroup reaching it, and skipGroups accepting RESERVED while rejecting 2^64. This closes the only finding from the prior review, the untested JS reserved-sequence guard.

Verified checkEnd's tightening against the internal callers that pass a fill's end straight through (ietf/subscriber.ts refuseGroups(fill.start, fill.end, …)): fills are capped by the feed edge in track.ts #askFills, so they never carry UNBOUNDED, and the IETF range ends (this.last + 1n) stay finite. Rust's Range<u64> bound (end <= u64::MAX) is now mirrored exactly.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated added this pull request to the merge queue Nov 9, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Nov 9, 2026
@moq-bot

moq-bot Bot commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

quest land watch: the merge queue removed this pull request at 33998fc because it conflicts with main (merge_conflict).

The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs.

(written by quest land watch)

@moq-bot

moq-bot Bot commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

codex, gpt-6.1-sol, medium effort land fixer note:

The failure log says:

Auto-merging rs/moq-mux/src/catalog/producer.rs
CONFLICT (content): Merge conflict in rs/moq-mux/src/catalog/producer.rs

Local merge commit 772fd14b1674d154283694b50d5d6934c389e1c3 on pr-8220 preserves both base’s Wall::MOQ_EPOCH constant and the PR’s saturating_sub call. The only manual merge resolution is in that test.

Ran just ci base. Rust lint/type checks and all 13,497 Rust tests passed; Python checks and 126 tests, C/C++ checks, OBS compilation, Biome, JavaScript checks/tests, and audio-quality checks passed. The drafts check attempted bibliography fetches from bib.ietf.org and datatracker.ietf.org, outside this task’s allowed network destinations. I stopped the gate; remaining checks were not completed. The commit is preserved locally for review, but cannot be reported as validated.

(written by codex, gpt-6.1-sol, medium effort)

@moq-bot

moq-bot Bot commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

codex, gpt-6.1-sol, medium effort land fixer: Conflict re·solved locally, but just ci base could not complete within the network restriction: ".refcache/reference.RFC.6838.xml: fetching from https://bib.ietf.org/public/rfc/bibxml/reference.RFC.683. Recommendation: Review local merge commit 772fd14b1674d154283694b50d5d6934c389e1c3 and run just ci base to completion in an environment authorized to fetch the draft bibliography.

Auto-merging rs/moq-mux/src/catalog/producer.rs
CONFLICT (content): Merge conflict in rs/moq-mux/src/catalog/producer.rs

(written by codex, gpt-6.1-sol, medium effort)

kixelated and others added 2 commits November 9, 2026 09:09
# Conflicts:
#	rs/moq-mux/src/catalog/producer.rs
… arithmetic

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed 0c48cd0 incrementally since 1cb1d83, comparing both PR deltas against their respective merge bases to exclude merged-main changes.

No actionable findings in this update. The new fallback-probe, handshake and handover deadlines consistently use saturating arithmetic while preserving optional timeouts and normal-range behavior. The routing regression now originates with the root announcement required by main's updated route eligibility, so it still exercises the exhausted-seqno case. The catalog merge resolution retains both MOQ_EPOCH and saturating_sub; the removed HLS arithmetic hunk is accounted for by main's timeout-helper rewrite.

Direction: extending the existing checked/saturating clock API to newly merged callers keeps overflow policy explicit without adding special-case state.

Verification limits: GitHub-only static review of the incremental changes, relevant implementations and regression tests. No builds, tests, interop or timing runs were performed; author-reported checks were not independently reproduced. Rechecked open/non-draft state, full head and published reviews immediately before this COMMENT.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

The delta since the prior pullfrog review is one commit on top of a merge of origin/main (29bc0c95d); the range-diff touches only main-superseded hunks plus the reconnect migration below.

  • Migrated main's new reconnect deadlines: Probe::new, Probe::failed, the Dialing connect deadline, the goaway handover Predecessor deadline, and probe()'s timeout now use Instant::saturating_add, so main's new code obeys this PR's no-panicking-arithmetic contract and keeps the established "effectively never" meaning at the clock's last instant.
  • Adapted two tests to main's rewrites: a_late_poll_past_the_window_gives_up compares with saturating_add (identity at real-clock values), and a_route_starved_at_the_last_seqno_requests_nothing originates through the new root-claim originate helper that main's interest-scoped routing requires.
  • Dropped merge-superseded hunks: main's test rewrites in moq-mux's catalog producer (Wall::MOQ_EPOCH), moq-net's Sim::originate/SessionInfo, and the deleted moq-webrtc rate test leave no PR-side edit behind; Config::timeout is now Option<Duration>, so Some(ZERO) correctly means an immediate deadline instead of the removed !is_zero() filter.
  • PR behavior intact after the merge: create_group/append_group still refuse the reserved sequence with BoundsExceeded, lite-07 decode_groups/encode_groups still carry Group Start + Count with the half-open Range<u64>, and the route seqno request still stops at MAX.

Verified with cargo check -p moq-net --all-targets --features async (passes) and the two adapted tests (a_late_poll_past_the_window_gives_up, a_route_starved_at_the_last_seqno_requests_nothing, both pass). A tree-wide static sweep finds no remaining kio::clock::Instant ± Duration expression; the only ± Duration left is Wall (documented out of scope) and std::time::Instant.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

# Conflicts:
#	quest/m0/peer-arithmetic.md
#	rs/moq-ffi/src/test.rs
#	rs/moq-relay/tests/integration/auth_lifetime.rs
#	rs/moq-relay/tests/integration/session_revalidate.rs
# Conflicts:
#	rs/moq-video/src/decode/backend/v4l2.rs
#	rs/moq-video/src/encode/backend/v4l2.rs

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed d6699fb incrementally since 0c48cd0, comparing both PR deltas against their respective merge bases to separate merged-main changes.

No actionable findings in this update. The track merge preserves main's consuming finish() -> Finished API while computing the end in finish_shared; rejecting group u64::MAX at creation/append makes the removed overflow-abort branch unreachable. The boundary regressions retain the returned handle while checking the last valid group and ordered cursor. The removed FFI/relay deadline hunks are accounted for by main's event-driven/manual-clock test rewrites. The remaining JS clock arithmetic work is retained in the follow-up plan.

Direction: keep the reserved-group invariant at creation and the existing finished-handle lifecycle, rather than adding a second overflow-specific terminal path.

Verification limits: GitHub-only static review of the incremental diff and relevant source/test paths. No builds, tests, interop or timing checks were run; reported validation was not independently reproduced. Rechecked open/non-draft state, full head and published reviews before this COMMENT. GitHub currently reports merge conflicts; this review does not establish merge readiness.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one documentation inconsistency from the merge, inline.

Reviewed changes

The delta since the prior pullfrog review (0c48cd005) is two merges of origin/main; comparing both PR patches line by line isolates its effect to five files.

  • Kept the quest for the JS half: quest/m0/peer-arithmetic.md is no longer deleted but retitled to the @moq/clock mirroring work, crediting this PR's Rust half and leaving the remaining JS call sites as the follow-up. quest check still reports no findings.
  • Simplified finish_shared: with group u64::MAX reserved, max_sequence + 1 cannot overflow, so main's abort-with-BoundsExceeded branch and its finish_past_the_last_sequence_aborts_with_the_cause test (which created a group at MAX) are removed, while finish() keeps main's consuming Finished return.
  • Adopted main's rewrites of merged test suites: rs/moq-ffi/src/test.rs, auth_lifetime, and session_revalidate now match main's manual-clock/event-driven versions, so their PR hunks left the patch.
  • Re-applied saturating_* to main's newer Instant ± Duration sites in moq-rtmp, the relay drain tests, the IETF publisher, and the gst tests.

Verified TrackState::advance is the only max_sequence writer and that the fill paths reject MAX before reaching it, so the removed overflow branch is genuinely unreachable. cargo check --workspace --all-targets --exclude moq-gst and cargo check -p moq-net --all-targets --features async pass (moq-gst cannot build in this sandbox: glib-sys lacks system headers, and its hunks are test-only); bun test src/track.test.ts (225 pass) and bun test src/lite/ (747 pass, 5 pre-existing skips) pass.

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

Comment thread quest/m0/peer-arithmetic.md Resolved
Pullfrog review: the quest stays for its JS half, so its milestone entry stays too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated added this pull request to the merge queue Nov 10, 2026

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed bcb0763 incrementally since d6699fb, accounting for merged-main changes.

No actionable findings. The only new PR-specific edit restores quest/m0/README.md's link to the retained JS arithmetic quest, with its current title and scope. This resolves the reported index inconsistency. The source-code edits remain equivalent against their respective merge bases.

Direction: keeping the remaining JS work listed separately makes the Rust completion and outstanding API-parity work clear.

Verification limits: GitHub-only static comparison and inspection of the index and linked quest. No builds or tests run. Rechecked open/non-draft state, full head and published reviews immediately before this COMMENT.

Merged via the queue into main with commit 16a6730 Nov 10, 2026
9 checks passed
@kixelated
kixelated deleted the quest/m0/peer-arithmetic branch November 10, 2026 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant