feat: fail-fast retries: jittered backoff bounded by time, not error type - #2647
Conversation
![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: 4e3538c1b6
ℹ️ 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".
| // The catalog exists but this build can't read it, or the session is gone. Waiting | ||
| // changes neither, and a broadcast with no servable renditions is what an empty | ||
| // rendition set already means. | ||
| Err(err) if !err.is_retryable() => { |
There was a problem hiding this comment.
When the HLS exporter starts after the broadcast is announced but before catalog.json has been created, catalog::Consumer::new can return moq_net::Error::NotFound from broadcast.track(...) rather than a transport error. This new terminal guard closes renditions and returns, so the broadcaster stays permanently empty even after the publisher writes the catalog a moment later; the old loop retried exactly this startup race. Handle the missing-catalog case separately from malformed/unsupported catalog errors.
Useful? React with 👍 / 👎.
| // Deliberately not decomposed. A rejected certificate arrives as a closed connection | ||
| // here and is retried until the give-up budget expires, which is the right call while | ||
| // certificates rotate underneath a long-lived publisher. | ||
| Self::Connection(_) | Self::Establish(_) | Self::Client(_) | Self::Server(_) | Self::RecvRequest(_) => true, |
There was a problem hiding this comment.
For the native WebTransport backends, the helper below only peels HTTP 401/403 into ConnectRejected; statuses like 400/404/405 remain inside Self::Client(_) (and the same blanket classification exists in noq/quiche). With this line marking every client CONNECT failure retryable, a bad endpoint path or unsupported method now waits until the reconnect timeout instead of surfacing immediately, even though only 408/429/502/503/504 should be retried. Preserve/classify the actual status before falling into the generic client-error arm. CLAUDE.mdL104-L106
Useful? React with 👍 / 👎.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change standardizes retry behavior across JavaScript and Rust components. It adds jittered exponential backoff, retry deadlines and budgets, stable-session resets, and protocol-based terminal status classification. Media capture and reconnect tests now cover delayed retries, exhaustion, and recovery. Relay peers, HLS flows, audio playback, RTMP accepts, and systemd services use bounded or indefinite retry policies. Documentation records the updated behavior. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
js/net/src/retry.ts (1)
107-133: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClamp
multiplierso the window cannot shrink.
#multiplieris accepted without validation.ReloadDelay.multiplierreaches this constructor from caller props injs/net/src/connection/reload.ts(line 224). A value below 1 shrinks#windowon every failure, so the loop retries faster and faster until the timeout budget expires. The Rust twin already guards this withself.config.multiplier.max(1)inrs/moq-net/src/retry.rs(line 145). Align the two implementations.♻️ Proposed clamp
constructor(props?: BackoffProps) { this.#initial = props?.initial ?? DEFAULT_INITIAL; - this.#multiplier = props?.multiplier ?? DEFAULT_MULTIPLIER; + // Below 1 the window would shrink per failure, turning the escalation into a tight loop. + this.#multiplier = Math.max(props?.multiplier ?? DEFAULT_MULTIPLIER, 1); this.#max = props?.max ?? DEFAULT_MAX;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/net/src/retry.ts` around lines 107 - 133, Clamp the multiplier assigned in the Backoff constructor to a minimum of 1, while preserving the existing default when no value is provided. Update the `#multiplier` initialization so delay() can never shrink `#window`, matching the Rust retry implementation.js/publish/src/source/retry.test.ts (1)
124-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the polling timing constants.
10000,5, and100define the polling timeout, interval, and quiet margin. Name them besideSPENT_TIMEOUTso test timing policy has one clear definition. As per coding guidelines, “Avoid using magic numbers; use named constants instead.”Proposed refactor
+const WAIT_TIMEOUT = 10_000; +const POLL_INTERVAL = 5; +const QUIET_MARGIN = 100; const SPENT_TIMEOUT = 30000; async function waitUntil(pred: () => boolean): Promise<void> { - const deadline = Date.now() + 10000; + const deadline = Date.now() + WAIT_TIMEOUT; while (!pred()) { if (Date.now() > deadline) throw new Error("timed out waiting for condition"); - await new Promise((resolve) => setTimeout(resolve, 5)); + await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL)); } } async function waitSpent(media: FakeMediaDevices): Promise<void> { - const quiet = (Retry.DELAY.max ?? 0) + 100; + const quiet = (Retry.DELAY.max ?? 0) + QUIET_MARGIN;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/publish/src/source/retry.test.ts` around lines 124 - 151, The waitUntil and waitSpent helpers contain unnamed polling timing values. Define named constants for the 10-second polling timeout, 5-millisecond polling interval, and 100-millisecond quiet margin beside SPENT_TIMEOUT, then update both helpers to use those constants.Source: Coding guidelines
rs/moq-mux/src/error.rs (1)
149-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider an exhaustive match instead of
_ => false.Every sibling classifier in this PR enumerates all variants, and
rs/moq-native/src/error.rsdocuments why: "The match is exhaustive so a new variant is a decision rather than an accident." Here the catch-all removes that compiler prompt.Errorcarries transport-adjacent variants already (Json,Loc), so a future variant that should be transient would silently become terminal.The current behavior is correct, so this is a durability improvement rather than a bug fix. Group the terminal variants into named arms with the same style as the other files.
As per coding guidelines: "For Rust retry classification, use exhaustive error matching and retry only explicitly retryable failures."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-mux/src/error.rs` around lines 149 - 155, Update Error::is_retryable to use an exhaustive match over every Error variant instead of the `_ => false` catch-all. Keep Moq and Io delegated to their existing retry classifiers, and group all explicitly terminal variants into named arms returning false, so newly added variants require an explicit retry decision.Source: Coding guidelines
rs/moq-native/src/reconnect.rs (1)
72-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a struct literal instead of reassigning fields after
default().The four assignments cover every field of
moq_net::retry::Config, so thedefault()value is discarded. Clippy'sfield_reassign_with_defaulttargets this shape. A functional-update literal states the intent and stays correct ifConfiggains a field.♻️ Proposed refactor
impl From<&Backoff> for moq_net::retry::Config { fn from(backoff: &Backoff) -> Self { - let mut config = Self::default(); - config.initial = backoff.initial; - config.multiplier = backoff.multiplier; - config.max = backoff.max; - config.timeout = backoff.timeout; - config + Self { + initial: backoff.initial, + multiplier: backoff.multiplier, + max: backoff.max, + timeout: backoff.timeout, + ..Self::default() + } } }The same pattern appears in
retry_backoff()inrs/moq-audio/src/playback/driver.rs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-native/src/reconnect.rs` around lines 72 - 81, Replace the default-then-field-reassignment pattern in From<&Backoff> for moq_net::retry::Config with a struct literal using the four Backoff fields and a functional update from Config::default(). Apply the same refactor in retry_backoff() in the playback driver, preserving all existing values and defaults.rs/moq-native/src/websocket.rs (1)
233-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCall
moq_net::retry::status_retryable(status)here.The HTTP upgrade status list duplicates
moq_net::retry::status_retryable, andqmux::Error::Http(status)currently matches theu16overload. Route this through the shared retry helper to keep the retry policy in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-native/src/websocket.rs` around lines 233 - 236, Update the qmux::Error::Http(status) branch in the connection retry matching logic to call the shared moq_net::retry::status_retryable(status) helper instead of maintaining an inline status list. Leave the other error branches unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/CLAUDE.md`:
- Line 83: Update the retry API references from next() to delay() in
js/CLAUDE.md lines 83-83 and rs/CLAUDE.md lines 112-112. In both `@moq/net`
Retry.Backoff and moq_net::retry guidance, keep the surrounding behavior
unchanged while documenting the valid delay() method.
In `@js/net/src/connection/reload.ts`:
- Around line 202-232: Update the eagerly created closed promise in Reload’s
constructor to attach a no-op rejection handler while preserving the original
promise for consumers awaiting closed. Extend the closed documentation to
mention rejection from terminal connection failures, including unsupported
protocols or unavailable transports, in addition to close() and retry timeout.
In `@js/net/src/retry.test.ts`:
- Around line 37-47: Update the “the budget is a deadline over the whole
sequence” test to use a timeout that the test can deterministically exceed
rather than relying on sub-millisecond performance.now() resolution. Make the
test callback async and await a suitable scheduling delay between the first and
second backoff.delay() calls, preserving the existing expectations and reset
behavior.
In `@js/publish/src/source/retry.test.ts`:
- Around line 302-315: Update the retry test around Microphone constraint
changes so it either makes FakeMediaDevices.getUserMedia reject the
intentionally invalid channelCount values, allowing the test to verify
constraint-failure recovery, or renames the test and comments to describe
media.missing/settings-change recovery instead. Ensure comments document only
the behavior actually exercised by the fake.
In `@rs/moq-native/src/quinn.rs`:
- Around line 436-445: Update the retryability match around the Client arm to
inspect Self::connect_error() and return false when the error is_auth(), before
the unconditional Client transport cases. Preserve retry behavior for
non-authentication Client errors and the existing ConnectRejected handling.
In `@rs/moq-net/src/lib.rs`:
- Line 87: Update Backoff::jitter to avoid narrowing half.as_nanos() to u64
before random_range, preserving the full duration-derived span and preventing a
zero-width range for oversized retry windows. Add a regression test covering
Duration::new(36_893_488_147, 419_103_232), including the sleep path, while
preserving existing jitter behavior for normal windows.
In `@rs/moq-relay/src/cluster.rs`:
- Around line 936-944: Update the retryable error branch in the
run_remote_session result match so that when elapsed >= stable_threshold, it
resets backoff before logging and retrying; retain the existing non-retryable
error return and warning behavior. Add a regression test covering a stable
session that closes with a transport error and verifies the backoff is reset.
In `@rs/moq-rtmp/src/server.rs`:
- Around line 336-342: Update the listener error handling around the accept loop
and self.pending so backoff does not stop polling pending handshakes: race or
otherwise concurrently drive the backoff sleep with pending-handshake progress,
returning a completed handshake as soon as it is ready while retaining the delay
for repeated accept errors. Add a regression test that completes a pending
handshake during accept_backoff and verifies it is served before the backoff
expires.
---
Nitpick comments:
In `@js/net/src/retry.ts`:
- Around line 107-133: Clamp the multiplier assigned in the Backoff constructor
to a minimum of 1, while preserving the existing default when no value is
provided. Update the `#multiplier` initialization so delay() can never shrink
`#window`, matching the Rust retry implementation.
In `@js/publish/src/source/retry.test.ts`:
- Around line 124-151: The waitUntil and waitSpent helpers contain unnamed
polling timing values. Define named constants for the 10-second polling timeout,
5-millisecond polling interval, and 100-millisecond quiet margin beside
SPENT_TIMEOUT, then update both helpers to use those constants.
In `@rs/moq-mux/src/error.rs`:
- Around line 149-155: Update Error::is_retryable to use an exhaustive match
over every Error variant instead of the `_ => false` catch-all. Keep Moq and Io
delegated to their existing retry classifiers, and group all explicitly terminal
variants into named arms returning false, so newly added variants require an
explicit retry decision.
In `@rs/moq-native/src/reconnect.rs`:
- Around line 72-81: Replace the default-then-field-reassignment pattern in
From<&Backoff> for moq_net::retry::Config with a struct literal using the four
Backoff fields and a functional update from Config::default(). Apply the same
refactor in retry_backoff() in the playback driver, preserving all existing
values and defaults.
In `@rs/moq-native/src/websocket.rs`:
- Around line 233-236: Update the qmux::Error::Http(status) branch in the
connection retry matching logic to call the shared
moq_net::retry::status_retryable(status) helper instead of maintaining an inline
status list. Leave the other error branches unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d8c74983-3366-48a4-beae-ada9a8437925
📒 Files selected for processing (39)
CLAUDE.mddoc/bin/gstreamer.mddoc/bin/relay/cluster.mdjs/CLAUDE.mdjs/net/src/connection/connect.tsjs/net/src/connection/handshake.tsjs/net/src/connection/reload.test.tsjs/net/src/connection/reload.tsjs/net/src/index.tsjs/net/src/retry.test.tsjs/net/src/retry.tsjs/publish/src/source/camera.tsjs/publish/src/source/microphone.tsjs/publish/src/source/retry.test.tsjs/publish/src/source/retry.tsnix/modules/moq-relay.nixpackaging/moq-relay/moq-relay.servicers/CLAUDE.mdrs/moq-audio/src/playback/driver.rsrs/moq-gst/src/sink/session.rsrs/moq-hls/src/error.rsrs/moq-hls/src/export/mod.rsrs/moq-hls/src/import.rsrs/moq-mux/src/error.rsrs/moq-native/src/error.rsrs/moq-native/src/iroh.rsrs/moq-native/src/noq.rsrs/moq-native/src/quiche.rsrs/moq-native/src/quinn.rsrs/moq-native/src/reconnect.rsrs/moq-native/src/tcp.rsrs/moq-native/src/unix.rsrs/moq-native/src/websocket.rsrs/moq-native/tests/reconnect.rsrs/moq-net/src/error.rsrs/moq-net/src/lib.rsrs/moq-net/src/retry.rsrs/moq-relay/src/cluster.rsrs/moq-rtmp/src/server.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-hls/src/export/mod.rs (1)
34-46: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd a retry deadline or attempt budget.
catalog_backoff()has no timeout, and this loop has no attempt limit. An open broadcast that never creates a catalog keeps the watcher retrying forever.Set a finite budget owned by
watch_catalog. When the budget expires, closerenditionsand return.As per coding guidelines, retries require "a deadline or attempt budget".
Also applies to: 203-220
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-hls/src/export/mod.rs` around lines 34 - 46, Update catalog_backoff and the watch_catalog retry loop to use a finite deadline or attempt budget owned by watch_catalog instead of retrying indefinitely. When the budget expires before a catalog is available, close renditions and return; preserve the existing retry behavior while the budget remains active.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rs/moq-hls/src/export/mod.rs`:
- Around line 34-46: Update catalog_backoff and the watch_catalog retry loop to
use a finite deadline or attempt budget owned by watch_catalog instead of
retrying indefinitely. When the budget expires before a catalog is available,
close renditions and return; preserve the existing retry behavior while the
budget remains active.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a3ec6cb-e39f-447f-8309-25326caecc93
📒 Files selected for processing (4)
rs/moq-hls/src/export/mod.rsrs/moq-native/src/noq.rsrs/moq-native/src/quiche.rsrs/moq-native/src/quinn.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rs/moq-native/src/quinn.rs
|
Pushed Two I'm declining, with reasons:
A give-up budget on the HLS export catalog wait. This one is deliberate and the comment now says so. The loop isn't repeating a failed request; it's waiting for a publisher to create its catalog track, and the broadcast closing is what ends the wait. A relay-side broadcast outlives its publisher's session, so any deadline here becomes a window in which a publisher outage leaves the broadcaster permanently empty with nothing to recover it. That's the same failure mode as the earlier Two fixes ship without a regression test, both because the loop has no seam to drive from a test: forcing (Written by Claude Opus 5) |
kixelated
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rs/moq-relay/src/cluster.rs (1)
932-943: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftReset only after an established session is stable.
Line 932 starts timing before
run_remote_once, and Line 985 performsclient.connect. If a blackholed peer fails after ten seconds, Line 941 resets the backoff even though no session was established. The nextbackoff.sleep()then uses the initial delay, so retry escalation never occurs.Return a session-established outcome or session duration from
run_remote_session. Reset only when an established session exceedsstable_threshold. Add a regression test or a focused test seam for a slow dial failure.As per coding guidelines, reset retry backoff only after an outcome that proves earlier failures no longer describe reality, such as a healthy session, and add a regression test for a bug fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-relay/src/cluster.rs` around lines 932 - 943, Update the retry flow around run_remote_once and run_remote_session so backoff.reset() occurs only when a session was successfully established and remained active for at least stable_threshold; do not use total dial duration for this decision. Propagate an established-session indicator or session duration from run_remote_session through run_remote_once, and add a focused regression test or test seam covering a slow connection failure that never establishes a session.Source: Coding guidelines
js/net/src/retry.ts (1)
82-97: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCap each delay to the remaining deadline.
Line 87 creates the deadline, but Line 94 returns a value from the full retry window. With
initial: 100andtimeout: 5, the first wait can be 50 to 100 milliseconds. The next retry then occurs after the five-millisecond budget.Calculate the remaining budget and cap the returned jittered delay to it. Add a regression test that uses an initial delay larger than the timeout.
As per coding guidelines, reproduce bugs at the lower layer and add a regression test for the fix.
Proposed fix
delay(): DOMHighResTimeStamp | undefined { + let remaining = Infinity; if (this.#timeout > 0) { const now = performance.now(); - if (this.#deadline === undefined) { - this.#deadline = now + this.#timeout; - } else if (now >= this.#deadline) { + const deadline = this.#deadline ?? now + this.#timeout; + this.#deadline = deadline; + remaining = deadline - now; + if (remaining <= 0) { return undefined; } } const delay = this.#window / 2 + Math.random() * (this.#window / 2); this.#window = Math.min(this.#window * this.#multiplier, this.#max); - return delay; + return Math.min(delay, remaining); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/net/src/retry.ts` around lines 82 - 97, Update the retry delay calculation in the timeout path around the retry method containing `#deadline` so the jittered delay is capped at the remaining deadline budget before being returned. Preserve the existing deadline initialization and expiry behavior, and add a lower-layer regression test using an initial delay greater than the timeout to verify the first wait does not exceed the configured timeout.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@js/net/src/retry.ts`:
- Around line 82-97: Update the retry delay calculation in the timeout path
around the retry method containing `#deadline` so the jittered delay is capped at
the remaining deadline budget before being returned. Preserve the existing
deadline initialization and expiry behavior, and add a lower-layer regression
test using an initial delay greater than the timeout to verify the first wait
does not exceed the configured timeout.
In `@rs/moq-relay/src/cluster.rs`:
- Around line 932-943: Update the retry flow around run_remote_once and
run_remote_session so backoff.reset() occurs only when a session was
successfully established and remained active for at least stable_threshold; do
not use total dial duration for this decision. Propagate an established-session
indicator or session duration from run_remote_session through run_remote_once,
and add a focused regression test or test seam covering a slow connection
failure that never establishes a session.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f81315d-8715-4dbf-924b-9f9c4d70b85f
📒 Files selected for processing (21)
CLAUDE.mdjs/CLAUDE.mdjs/net/src/connection/reload.tsjs/net/src/retry.test.tsjs/net/src/retry.tsjs/publish/src/source/retry.test.tsrs/CLAUDE.mdrs/moq-gst/src/sink/session.rsrs/moq-hls/src/error.rsrs/moq-hls/src/export/mod.rsrs/moq-hls/src/import.rsrs/moq-native/src/error.rsrs/moq-native/src/noq.rsrs/moq-native/src/quiche.rsrs/moq-native/src/quinn.rsrs/moq-native/src/reconnect.rsrs/moq-native/src/websocket.rsrs/moq-native/tests/reconnect.rsrs/moq-net/src/retry.rsrs/moq-relay/src/cluster.rsrs/moq-rtmp/src/server.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- js/CLAUDE.md
- rs/moq-rtmp/src/server.rs
- rs/moq-native/src/reconnect.rs
- rs/moq-hls/src/import.rs
- rs/moq-native/src/noq.rs
- rs/moq-gst/src/sink/session.rs
- rs/moq-net/src/retry.rs
- js/publish/src/source/retry.test.ts
- js/net/src/connection/reload.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e8adb54e7
ℹ️ 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".
| Self::Quiche(err) => err.status(), | ||
| #[cfg(feature = "websocket")] | ||
| Self::WebSocket(err) => err.status(), | ||
| _ => None, |
There was a problem hiding this comment.
When WebSocket fallback is enabled and both race arms get a settled HTTP response, this top-level status() drops the statuses because TransportRace falls into _ => None. Fresh evidence in this version is that the backend errors now expose status(), but the aggregate still discards it, so a bad path that returns 404/405 over both QUIC and WebSocket is treated as a transient transport failure and waits until the reconnect timeout instead of failing immediately. Consider propagating a peer status from the race, at least when both recorded answers are non-retryable.
Useful? React with 👍 / 👎.
| Ok(outcome) => { | ||
| backoff.reset(); | ||
| outcome |
There was a problem hiding this comment.
For already-discovered tracks, step(OnError::Warn) logs per-rendition fetch/import errors and still returns Ok, even if every rendition failed. Resetting the backoff here treats that as a successful pass, so a single-rendition import whose media playlist or segment starts returning 404/503 never reaches the non-retryable-status or give-up-budget arms below and can spin at the normal refresh cadence while publishing nothing. Carry whether any rendition succeeded or failed in StepOutcome, and only reset after a pass that actually made progress or had no rendition errors.
Useful? React with 👍 / 👎.
|
Pushed
No regression tests for the cluster or RTMP accept fixes: neither loop has a seam to drive a slow-dial failure or a failed Context for anyone reading the diff cold: the retryable/non-retryable classification that earlier reviews were built around is gone as of (Written by Claude Opus 5) |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-hls/src/import.rs`:
- Around line 99-104: Update the rendition-failure tracking around the step loop
and the logic at `failed` assignment near the rendition import handling so an
observed non-retryable error is never overwritten by a later retryable error;
only replace the retained error while the current failure is non-terminal.
Ensure the all-renditions-failed path returns the retained terminal response
instead of retrying, and add an inline regression test covering a `404` followed
by a retryable rendition error with no segments written.
In `@rs/moq-net/src/retry.rs`:
- Around line 126-131: Update the deadline handling in the retry flow to compute
remaining time with Instant::checked_duration_since(now) instead of direct
subtraction after the existing now >= deadline guard. Preserve the current None
return for expired deadlines and the resulting remaining assignment for valid
deadlines.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dbdac9dd-81bd-43b6-b48d-6fc3d5f63e8b
📒 Files selected for processing (6)
js/net/src/retry.test.tsjs/net/src/retry.tsrs/moq-hls/src/import.rsrs/moq-native/src/error.rsrs/moq-net/src/retry.rsrs/moq-relay/src/cluster.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- js/net/src/retry.ts
- js/net/src/retry.test.ts
- rs/moq-relay/src/cluster.rs
| if let Some(deadline) = deadline { | ||
| // Started already: stop once the budget is gone. | ||
| if now >= deadline { | ||
| return None; | ||
| } | ||
| remaining = Some(deadline - now); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching retry.rs:"
fd -a 'retry\.rs$' . | sed 's#^\./##'
echo
echo "Candidate file outline:"
ast-grep outline rs/moq-net/src/retry.rs --view expanded || true
echo
echo "Relevant source lines 1-180:"
sed -n '1,180p' rs/moq-net/src/retry.rs | nl -ba
echo
echo "Dependency/toolchain declarations mentioning web_async/time/dependencies:"
rg -n "web_async|web-time|time|tokio|Instant|dep:" Cargo.toml Cargo.lock rust-toolchain.toml rust-toolchain 2>/dev/null || true
echo
echo "WebAsync Instant definitions/usages:"
rg -n "struct Instant|type Instant|pub.*Instant|checked_duration_since|duration_since|Instant::now|use .*time" . --glob '*.rs' --glob 'Cargo.toml' --glob 'Cargo.lock' || trueRepository: moq-dev/moq
Length of output: 1439
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant source lines 1-180:"
sed -n '1,180p' rs/moq-net/src/retry.rs
echo
echo "Dependency toolchain declarations:"
rg -n "web_async|web-time|time|tokio|Instant|dep:" Cargo.toml Cargo.lock rust-toolchain.toml rust-toolchain 2>/dev/null || true
echo
echo "Instant definitions/usages:"
rg -n "struct Instant|type Instant|pub.*Instant|checked_duration_since|duration_since|Instant::now|use .*time" . --glob '*.rs' --glob 'Cargo.toml' --glob 'Cargo.lock' || true
echo
echo "Remaining lines with relevant retry/delay logic:"
sed -n '180,330p' rs/moq-net/src/retry.rsRepository: moq-dev/moq
Length of output: 41978
🌐 Web query:
web_async time Instant checked_duration_since docs
💡 Result:
In the Rust ecosystem, checked_duration_since is a standard method available on std::time::Instant [1][2] and tokio::time::Instant [3] used to safely calculate the time elapsed between two instants. The method checked_duration_since(earlier: Instant) returns an Option<Duration>: it returns Some(duration) if the specified earlier instant is indeed before the current instant, and None if it is later [1][3][2]. This is primarily used to avoid panics that can occur when performing subtraction on instants, particularly in cases of non-monotonic clock behavior [1][2]. Regarding "web_async," this refers to a portable library (often associated with the web-rs ecosystem) that provides a cross-platform facade for asynchronous tasks and time primitives [4][5][6]. Because std::time::Instant and tokio::time::Instant may panic or behave unexpectedly on certain browser WebAssembly (WASM) targets, web_async::time offers a portable Instant implementation [4][6]. This facade ensures that code using these time primitives—including the checked_duration_since method—remains compatible across native, WASI, and browser environments by routing calls to appropriate underlying implementations (such as wasmtimer for WASM) [4][6]. In summary: - Method: checked_duration_since(earlier: Instant) -> Option<Duration> [1][3][2] - Purpose: Safely compute duration without panicking if the order is incorrect or monotonicity is violated [1][2][7]. - Web_async Context: A portable wrapper that provides a consistent Instant API across different platforms, including browsers, where standard time implementations might otherwise fail or panic [4][6].
Citations:
- 1: https://doc.rust-lang.org/std/time/struct.Instant.html
- 2: https://dev-doc.rust-lang.org/stable/src/std/time.rs.html
- 3: https://docs.rs/tokio/latest/tokio/time/struct.Instant.html
- 4: feat(web-async): add portable time module kixelated/web-rs#38
- 5: [codex] export wasm time helpers kixelated/web-rs#41
- 6: kixelated/web-rs@fa8b513
- 7: Tracking issue for 'checked_duration_since' feature rust-lang/rust#58402
Use checked timestamp subtraction.
rs/moq-net/src/retry.rs imports kio::time::Instant, which exposes checked_duration_since. Replace deadline - now with checked_duration_since(now) so the checked property follows the if now >= deadline guard.
Proposed change
- remaining = Some(deadline - now);
+ remaining = deadline.checked_duration_since(now);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(deadline) = deadline { | |
| // Started already: stop once the budget is gone. | |
| if now >= deadline { | |
| return None; | |
| } | |
| remaining = Some(deadline - now); | |
| if let Some(deadline) = deadline { | |
| // Started already: stop once the budget is gone. | |
| if now >= deadline { | |
| return None; | |
| } | |
| remaining = deadline.checked_duration_since(now); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rs/moq-net/src/retry.rs` around lines 126 - 131, Update the deadline handling
in the retry flow to compute remaining time with
Instant::checked_duration_since(now) instead of direct subtraction after the
existing now >= deadline guard. Preserve the current None return for expired
deadlines and the resulting remaining assignment for valid deadlines.
Source: Coding guidelines
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c36f991a14
ℹ️ 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".
| tokio::time::sleep(delay).await; | ||
| delay = std::cmp::min(delay * backoff.multiplier, backoff.max); | ||
| tracing::warn!(%url, "reconnecting after backoff"); | ||
| if !retry.sleep().await { |
There was a problem hiding this comment.
When reconnect attempts hang until ClientConfig::timeout (30s by default), this check only spends the new Backoff budget during sleeps because the deadline is created inside Backoff::sleep() after client.connect() returns. Client::consume() still sets broadcast linger to backoff.timeout + 1s, assuming Reconnect::closed() fires first, so enough slow failed dials can let the origin tear down broadcasts while the reconnect loop is still retrying. Keep the reconnect deadline around the whole attempt+sleep sequence, or extend linger by the possible connect time.
Useful? React with 👍 / 👎.
| restarting cluster doesn't reconnect in lockstep. A peer that *rejects* us is not: a bad token, an | ||
| ALPN neither side speaks, or a URL this build can't dial produces the same failure on every dial, so | ||
| the relay logs `cluster peer rejected us` and gives up on that peer rather than hiding the cause |
There was a problem hiding this comment.
Fresh evidence here is the cluster loop's separate Attempt::failed path: a bad token or non-WebTransport path from client.connect() is wrapped into Attempt, then run_remote only logs cluster peer error; will retry and sleeps with timeout = ZERO, so it never emits the documented rejection message or exits the peer task. This new paragraph will lead operators to wait for a leave/re-enter/restart recovery path that cannot happen unless terminal handling is implemented in run_remote.
Useful? React with 👍 / 👎.
|
Pushed Three findings from this round, all valid:
Also took (Written by Claude Opus 5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cac2ce177a
ℹ️ 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".
| Ok(StepOutcome { | ||
| wrote_segments: 0, | ||
| failed: Some(err), | ||
| .. | ||
| }) => Err(err), |
There was a problem hiding this comment.
When a multi-rendition HLS import has one permanently missing rendition (for example a variant playlist returning 404) and the other renditions are healthy but simply have no new segments in this polling pass, this conversion turns the whole step into Err; the non-retryable-status arm below then returns and ends the entire import. That contradicts the per-rendition error handling in step(OnError::Warn), where one dead variant is supposed to be tolerated while the rest can still serve. Track whether any rendition successfully ingested, and only promote the stored failure to a whole-step error when no rendition succeeded, not just when no segments were written.
Useful? React with 👍 / 👎.
|
Pushed Also fixed Codex's finding on the same push, which was a real one and sharper than the version I'd already addressed. The import promoted a step to a failure when no segments were written — but a healthy live playlist writes none between segments. So a two-rendition import with one permanently-404 variant and one healthy-but-quiet variant would end outright, which is precisely what That's the second bug in three commits in this one conversion, both in the same direction (treating a quiet pass as a dead one). Worth a careful look from a human at (Written by Claude Opus 5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 223cbf995b
ℹ️ 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".
| `failed` only on something a retry cannot clear, such as a rejected token, unusable TLS material, or | ||
| a URL no compiled-in backend can dial. |
There was a problem hiding this comment.
When the sink is given a URL scheme that this moq-gst build cannot dial, the reconnect loop does not transition to failed: rs/moq-gst/src/sink/session.rs sets config.backoff.timeout = Duration::ZERO, and Reconnect::run only exits on err.is_auth() or a non-retryable HTTP err.status(). Errors such as NoBackend/unsupported scheme have neither, so they remain disconnected and retry forever; remove this example from the docs or make those local configuration errors terminal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aec493188e
ℹ️ 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".
| Self::Client(err) => client_status(err), | ||
| // One address answering is not the set answering, so a raced dial reports nothing | ||
| // rather than letting a single response speak for the rest. | ||
| Self::Failover(_) => None, |
There was a problem hiding this comment.
Fresh evidence here is that the new Failover arm drops status-bearing attempts: when DNS returns multiple addresses and they all reach the same relay but reject CONNECT with a settled status such as 404/405, failover::race wraps those backend errors in Failover, status() returns None, and Reconnect burns the backoff timeout instead of failing immediately. Please fold the failures and surface a non-retryable status when every attempted address reported one; the same pattern is needed for the noq/quiche copies.
Useful? React with 👍 / 👎.
|
Pushed
Caveat unchanged: quiche's copy is uncompiled here ( (Written by Claude Opus 5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 438a7355f1
ℹ️ 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".
| Self::Quiche(err) => err.status(), | ||
| #[cfg(feature = "websocket")] | ||
| Self::WebSocket(err) => err.status(), | ||
| _ => None, |
There was a problem hiding this comment.
When a backend sees a 401/403 CONNECT response, the From<crate::{quinn,noq,quiche,websocket}::Error> conversions collapse it into Error::Connect, but this new accessor falls through to None. That makes the public Error::status() API drop a real server response exactly for auth rejections, so any caller following the new status-based contract for diagnostics or retry policy cannot distinguish those from failures with no HTTP answer; map Self::Connect(Unauthorized/Forbidden) back to 401/403 here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
rs/moq-hls/src/error.rs (2)
21-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace bare HTTP status numbers with named constants.
The
matches!expression embeds five protocol values as magic numbers. Use named constants or HTTP status constants so the retry policy remains self-documenting.As per coding guidelines, avoid magic numbers and use named constants instead.
Proposed refactor
+const HTTP_STATUS_REQUEST_TIMEOUT: u16 = 408; +const HTTP_STATUS_TOO_MANY_REQUESTS: u16 = 429; +const HTTP_STATUS_BAD_GATEWAY: u16 = 502; +const HTTP_STATUS_SERVICE_UNAVAILABLE: u16 = 503; +const HTTP_STATUS_GATEWAY_TIMEOUT: u16 = 504; + pub(crate) fn status_retryable(status: u16) -> bool { - matches!(status, 408 | 429 | 502 | 503 | 504) + matches!( + status, + HTTP_STATUS_REQUEST_TIMEOUT + | HTTP_STATUS_TOO_MANY_REQUESTS + | HTTP_STATUS_BAD_GATEWAY + | HTTP_STATUS_SERVICE_UNAVAILABLE + | HTTP_STATUS_GATEWAY_TIMEOUT + ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-hls/src/error.rs` around lines 21 - 29, Update status_retryable to replace the literal HTTP status values in the matches! expression with descriptive named HTTP status constants, preserving the existing retry policy for 408, 429, 502, 503, and 504.Source: Coding guidelines
170-183: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd positive coverage for
Error::status().The current tests only cover responses that do not carry a status. Add reqwest-backed cases for a settled status such as
404and a retryable status such as503, so a regression that loses response statuses cannot pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-hls/src/error.rs` around lines 170 - 183, Add positive coverage to the an_http_failure_reports_its_status test for Error::status(), constructing reqwest-backed errors that preserve settled status 404 and retryable status 503, and assert each returns its corresponding status. Keep the existing no-status assertions unchanged.rs/moq-audio/Cargo.toml (1)
63-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare
randin the workspace dependencies.
randis declared three times in package manifests, while the workspace dependency section exists and should hold shared Rust dependencies. Addrandunder[workspace.dependencies], then reference it fromrs/moq-audio,rs/moq-hls, andrs/moq-relaywithrand = { workspace = true }.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-audio/Cargo.toml` at line 63, Move the shared rand version declaration into [workspace.dependencies] in the workspace Cargo.toml, then replace the direct rand declarations with rand = { workspace = true } in rs/moq-audio/Cargo.toml (63-63), rs/moq-hls/Cargo.toml (33-33), and rs/moq-relay/Cargo.toml (59-59).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 104-105: Update the retry guidance in the “When does it stop?”
section to document enforcing both termination caps: before sleeping, limit the
jittered delay to the remaining deadline, and when an attempt budget is used,
count the current attempt and skip sleeping after the final allowed attempt.
Keep the existing delay escalation and unlimited-supervisor guidance unchanged.
In `@doc/bin/gstreamer.md`:
- Around line 44-53: Update the documentation near the status explanation to
replace the idiomatic phrases “ridden out” and “waiting out an outage” with
direct wording: state that the sink continues retrying for the pipeline
lifetime, and that remaining disconnected from the first attempt may indicate
misconfiguration. Preserve the existing retry and failure behavior details.
In `@js/CLAUDE.md`:
- Line 83: The retry guidance in js/CLAUDE.md at lines 83-83 must state that the
local delay grows exponentially, is capped at max, and uses jitter; require the
loop to stop via a deadline or attempt budget. Update the corresponding guidance
in rs/CLAUDE.md at lines 112-112 to state that Duration grows exponentially, is
capped at MAX, and uses jitter; when a deadline applies, require the loop to
stop at that deadline.
In `@rs/moq-native/src/reconnect.rs`:
- Around line 91-96: Update Reconnect::run to check the deadline at each loop
iteration before starting or continuing connection work, and bound the existing
client.connect attempt by the same deadline so it cannot outlive
Backoff::timeout. Prevent clamped sleeps from launching another attempt once the
deadline is reached, while preserving the attempt-budget behavior. Add a
paused-time regression test covering a pending connection that remains active
until the deadline and verifies the reconnect loop stops.
- Around line 206-210: Update the reconnect backoff flow around the delay
initialization and escalation logic to cap delays safely before multiplication
and before each reconnect wait. Replace overflowing Duration scalar
multiplication with
Delay.saturating_mul(backoff.multiplier.max(1)).min(backoff.max), and apply the
same cap to initial delay assignments and resets so an initial delay above
backoff.max is bounded on the first wait. Add regression tests covering overflow
protection and initial-delay capping.
---
Nitpick comments:
In `@rs/moq-audio/Cargo.toml`:
- Line 63: Move the shared rand version declaration into
[workspace.dependencies] in the workspace Cargo.toml, then replace the direct
rand declarations with rand = { workspace = true } in rs/moq-audio/Cargo.toml
(63-63), rs/moq-hls/Cargo.toml (33-33), and rs/moq-relay/Cargo.toml (59-59).
In `@rs/moq-hls/src/error.rs`:
- Around line 21-29: Update status_retryable to replace the literal HTTP status
values in the matches! expression with descriptive named HTTP status constants,
preserving the existing retry policy for 408, 429, 502, 503, and 504.
- Around line 170-183: Add positive coverage to the
an_http_failure_reports_its_status test for Error::status(), constructing
reqwest-backed errors that preserve settled status 404 and retryable status 503,
and assert each returns its corresponding status. Keep the existing no-status
assertions unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6193704e-f753-4b6b-b0e1-f5e5ef675878
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
CLAUDE.mddoc/bin/gstreamer.mddoc/bin/relay/cluster.mdjs/CLAUDE.mdjs/net/src/connection/reload.tsjs/publish/src/source/retry.tsrs/CLAUDE.mdrs/moq-audio/Cargo.tomlrs/moq-audio/src/playback/driver.rsrs/moq-hls/Cargo.tomlrs/moq-hls/src/error.rsrs/moq-hls/src/export/mod.rsrs/moq-hls/src/import.rsrs/moq-hls/src/lib.rsrs/moq-native/src/error.rsrs/moq-native/src/noq.rsrs/moq-native/src/quiche.rsrs/moq-native/src/quinn.rsrs/moq-native/src/reconnect.rsrs/moq-relay/Cargo.tomlrs/moq-relay/src/cluster.rsrs/moq-rtmp/src/server.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- rs/moq-native/src/quiche.rs
- rs/moq-hls/src/export/mod.rs
- rs/moq-native/src/noq.rs
- rs/moq-native/src/error.rs
- js/net/src/connection/reload.ts
- rs/moq-hls/src/import.rs
- rs/moq-native/src/quinn.rs
- rs/moq-rtmp/src/server.rs
| - **How long between attempts?** Capped exponential backoff with jitter, never a fixed delay. Three lines at the call site: draw the wait from the top half of the current window (`delay.mul_f64(0.5 + rand::rng().random::<f64>() / 2.0)`), sleep it, then `delay = (delay * 2).min(MAX)`. There is deliberately no shared `Backoff` type. Each loop wants a different subset (most want no budget at all), the escalation is smaller than the abstraction over it, and a general one has to accept an arbitrary `max` it then has to defend against. | ||
| - **When does it stop?** A deadline or an attempt budget, and that budget is what ends the loop. Unlimited retries belong only to a supervisor whose job is to outlive an outage (a reconnecting publisher, a cluster peer, a listener), where the escalating delay is what keeps a permanently-dead target cheap. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the deadline and attempt caps.
The guidance says that a deadline or attempt budget ends the loop, but the three-line recipe caps only delay at MAX. When a deadline applies, cap the wait by the remaining deadline before sleeping. When an attempt budget applies, count the current attempt and do not sleep after the final permitted attempt. Otherwise, implementations can overshoot the deadline or perform an extra wait.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLAUDE.md` around lines 104 - 105, Update the retry guidance in the “When
does it stop?” section to document enforcing both termination caps: before
sleeping, limit the jittered delay to the remaining deadline, and when an
attempt budget is used, count the current attempt and skip sleeping after the
final allowed attempt. Keep the existing delay escalation and
unlimited-supervisor guidance unchanged.
|
|
||
| ## Conventions | ||
|
|
||
| - **Retry loops inline their backoff** (root Retries explains the why). Escalate a local delay toward a `max`, jitter each wait (`delay * (0.5 + Math.random() / 2)`), and hand it to `effect.timer`. There is no shared `Backoff` export, and don't try to classify which thrown values are worth retrying: the platform hands back `WebTransportError`, `DOMException`, `AggregateError`, and bare `Error`s interchangeably, so a budget or an attempt count is what stops the loop. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
State the required backoff shape explicitly in both retry guides.
Both lines require local jitter and a maximum, but neither states that the delay must grow exponentially. The JavaScript rule also does not clearly state that a time-bounded loop stops at its deadline.
js/CLAUDE.md#L83-L83: state that the delay grows exponentially and is capped atmax; require a deadline or attempt budget to stop the loop.rs/CLAUDE.md#L112-L112: state that theDurationgrows exponentially and is capped atMAX; require the loop to stop at its deadline when a deadline applies.
📍 Affects 2 files
js/CLAUDE.md#L83-L83(this comment)rs/CLAUDE.md#L112-L112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/CLAUDE.md` at line 83, The retry guidance in js/CLAUDE.md at lines 83-83
must state that the local delay grows exponentially, is capped at max, and uses
jitter; require the loop to stop via a deadline or attempt budget. Update the
corresponding guidance in rs/CLAUDE.md at lines 112-112 to state that Duration
grows exponentially, is capped at MAX, and uses jitter; when a deadline applies,
require the loop to stop at that deadline.
Source: Coding guidelines
Adopt one retry policy across the workspace: retry what a retry could plausibly clear, escalate the wait, bound the sequence, and let exactly one layer own the budget. Two shared primitives, one per language. `moq_net::retry` provides `Backoff` (capped exponential, equal jitter, optional give-up deadline) plus `io_retryable`/`status_retryable`; `@moq/net`'s `Retry` provides the same `Backoff` and a `Terminal` error class. Retryability is classified by `is_retryable()` on `moq_net`, `moq_native` (and each backend error enum), `moq_mux`, and `moq_hls` errors, always as an exhaustive `match` so a new variant has to be classified rather than defaulting to retryable. TypeScript inverts the default on purpose: the browser throws untyped platform errors, and treating those as terminal would strand connections a retry would have recovered, so `Terminal` marks what is settled and everything else is retried. Every retry site the audit flagged now either becomes terminal on a deterministic failure or uses the shared schedule: the native reconnect loop, cluster peer dials, HLS import and export, the RTMP accept loop, the audio playback driver, the JS reconnect loop, and browser capture reopen. The systemd units escalate their restart delay instead of restarting every five seconds forever. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two findings from review. A WebTransport CONNECT the server actually answered is its settled response, but only 401/403 were being peeled into `ConnectRejected`; everything else stayed inside the generic client-error arm, which the new classification marked retryable. A wrong path (404) or an endpoint that doesn't speak WebTransport (405) therefore burned the whole reconnect budget instead of surfacing. Each backend now extracts the status once, and both consumers read it: the auth classification and `is_retryable`. The HLS exporter's catalog wait was using `is_retryable`, which reads `NotFound` as needing an external change before another attempt can help. That is true in general and wrong here: the external change is the publisher writing the catalog track, which is exactly what the loop waits for. An exporter that subscribed between the announcement and the first catalog write would give up and leave the broadcaster permanently empty. Waiting is now keyed on the failure being moq-level at all, with the broadcast closing still ending the wait. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- The RTMP accept backoff was slept on inline, so a handshake that completed during it waited out the whole delay. It is a deadline now, and the loop keeps polling pending handshakes through the pause. - A cluster peer session that ran healthy and then dropped never reset its backoff: `run_remote_session` reports even a clean close as an error, so the reset keyed on `Ok(())` could not fire and a peer up for hours redialed on a stale five-minute window. Key it on how long the session lasted instead. - `Backoff::jitter` narrowed the sample span to `u64`, which a `max` past ~584 years truncates to an empty range and panics on. `max` comes from a caller-supplied humantime string, so saturate. - `Reload.closed` rejects unprompted on a terminal failure, and a caller is free to never await it. Mark the rejection handled so it doesn't surface as an `unhandledrejection`, and document the new cause. - The JS backoff now clamps its multiplier to 1, matching the Rust twin; below that the window shrinks per failure into a tight loop. - `moq_mux::Error::is_retryable` is exhaustive, and the WebSocket backend reuses `status_retryable` rather than its own status list. - Docs and tests: the guides said `next()` after the rename to `delay()`; the JS budget test no longer depends on sub-millisecond clock resolution; the capture test constants are named and the constraint test says what the fake actually exercises. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The retry loops no longer ask an error whether it is worth repeating. Deciding that means guessing, the guess has to stay correct as every wrapped error type evolves, and getting it wrong either strands a connection a retry would have recovered or hammers a dead one. The backoff budget already bounds the damage; classification only bought a faster path to surfacing a configuration error. Removes `is_retryable` from `moq_net::Error`, `moq_native::Error` and all seven backend error enums, `moq_mux::Error`, and `moq_hls::Error`, along with `retry::io_retryable`, the `Terminal` marker class in `@moq/net`, and the `catalog_pending` and `peer_is_retryable` helpers. What survives is an answer a peer actually gave, where the protocol defines the meaning rather than us inferring it. `retry::status_retryable` stays, and `moq_native::Error::status` / `moq_hls::Error::status` report the HTTP status a server sent so a caller can consult it: the reconnect loop still stops on a CONNECT the relay answered with a `404`, and the HLS import still stops on a `404` playlist. The reconnect loop also keeps its pre-existing `is_auth` guard. Everything else now ends on its budget: the cluster peer loop, the HLS export catalog wait, and the JS reconnect loop retry whatever they get. The schedules, jitter, budgets, and the RTMP and cluster fixes from earlier in this branch are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- The cluster loop timed the whole dial, not the session, so a peer that blackholed until the connect timeout looked "stable" and reset the backoff on every attempt. The escalation therefore never happened. `run_remote_session` now reports when the session actually came up, and the reset keys on the session's own lifetime. - A `Backoff` could sleep past its deadline by a whole window, since the jittered delay ignored what was left of the budget. An `initial` larger than `timeout` meant the first wait alone blew through it. Both the Rust and JS twins now cap the delay to the remaining budget, so the sequence lands on the deadline instead of overshooting it. - `moq_native::Error::status` dropped the status when a transport race lost both halves, so a `404` answered over both QUIC and WebSocket read as a transient failure. It now reports one, but only when both halves were answered and neither answer invites a retry. - The HLS import reset its backoff on any `Ok` step, and `step` returns `Ok` even when every rendition failed (it swallows per-rendition errors so one bad variant doesn't drop the rest). A source returning 404 forever would spin at the refresh cadence publishing nothing. `StepOutcome` now carries the failure, and a pass that imported nothing while a rendition was failing is treated as the failure it is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A backoff schedule is a utility, not part of the wire layer's published API. It now lives in `kio::time` next to `Deadline`, where the crates that need it (moq-native, moq-relay, moq-hls, moq-rtmp, moq-audio) already reach it. `moq_net::retry` is gone. `status_retryable` moves with its callers rather than being shared: moq-native and moq-hls each keep a private copy of the five-status list, so nothing about retry policy is exported from either. Three review findings folded in: - The budget now runs from construction or the last `reset` rather than from the first delay, so it covers the attempts as well as the waits between them. A reconnect whose every dial hung until the connect timeout could previously outlive its budget by that much, which also put it past the linger window `Client::consume` sizes from it. - When several HLS renditions fail in one pass, the step keeps the failure another pass could clear rather than whichever came last, so a single permanently-dead variant doesn't end an import the others could still serve. Keeping the last one made the outcome depend on rendition order. - `doc/bin/relay/cluster.md` still promised that a rejected cluster peer is given up on, which stopped being true when the classification came out. It documents the redial-forever behavior now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`cargo sort` failed CI: the `kio` dependency went in at a convenient anchor rather than its alphabetical slot in moq-audio, moq-relay, and moq-rtmp. The HLS import promoted a step to a failure when no *segments* were written, but a healthy live playlist writes none between segments. A multi-rendition import with one permanently-404 variant and one healthy but quiet one therefore ended outright, which is exactly what `OnError::Warn` exists to prevent. It now keys on renditions that ingested without error, so a step is a failure only when nothing ingested at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`Error::status` is public on both `moq_native` and `moq_hls`, but its doc comment linked `status_retryable`, which became private when the backoff moved to kio. `cargo doc -D warnings` rejects that. The rule is spelled out in prose instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`Backoff` had no natural home. It went into `moq_net` (the wire layer's published surface), then into `kio::time` next to `Deadline` despite having no poll surface, no `Waiter`, and nothing else kio-shaped about it. Both placements were about reach rather than about what the thing is. It isn't much of a thing. The escalation is three lines, and it's now those three lines at each of the six call sites: draw the wait from the top half of the current window, sleep it, double toward a local `const MAX`. The two loops that want a give-up budget (`Reconnect`, the HLS import) track a deadline next to their delay; the other four never wanted one, which is most of what the shared type was carrying. Generality was also the source of its worst bug: the caller-supplied `max` is what let jitter overflow a `u64` and panic. Six sites with fixed 4s/5s/10s/30s/300s ceilings can't reach it. `kio` is byte-identical to main again, `@moq/net` exports no `Retry`, and the four crates that took a `kio` dependency for this drop it. What remains of the PR's public API is one method: `Error::status`, on moq-native and moq-hls. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`status()` folded a transport race but not an address failover, so a hostname whose addresses all reach a relay that rejects the path with a settled status reported `None` and burned the whole reconnect budget. It now reports one, but only when every raced address answered and none of the answers invites a retry. `doc/bin/gstreamer.md` promised the sink reaches `failed` on "a URL no compiled-in backend can dial". It doesn't: with no classification and `timeout = 0`, only an auth rejection or a settled CONNECT status ends the loop, and a local configuration error retries forever. The page says what actually happens, and what to watch for instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Drop the Error::status()/status_retryable classification from moq-native and moq-hls, along with all the status plumbing through the QUIC backends. Every failure now rides the same jittered backoff, and a short budget is what stops the loop: an ephemeral failure is one that clears within it, so the budget classifies without anyone maintaining a status list. The only remaining short-circuit is the pre-existing is_auth guard. Shorten the budgets to match. Reconnect gives up after 10s (was 5m) with a 5s delay ceiling (was 30s), in Rust and JS. The HLS import budget drops from 300s to 10s. Supervisor loops that have nobody to return an error to (cluster peers, device reopen, RTMP accept) still retry forever, but the cluster ceiling drops from 300s to 10s so a dead peer is loudly broken and a returning one is picked up within seconds. Trim the CLAUDE.md retry guidance to one paragraph stating the policy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
438a735 to
580b1cb
Compare
kixelated
changed the title
|
Reshaped per kixelated's direction: retries are now bounded by time, not by error type. The (Written by Claude Fable 5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 580b1cb77e
ℹ️ 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".
| tokio::time::sleep(wait).await; | ||
| continue; |
There was a problem hiding this comment.
When failures persist until a jittered sleep is clipped to deadline, this sleep returns at the deadline and continue immediately starts another self.step(...) without rechecking the budget. Each HTTP fetch in that extra step can take up to REQUEST_TIMEOUT (30 seconds), and multiple renditions are processed sequentially, so the advertised 10-second retry budget can be exceeded substantially before the last error surfaces. Recheck the deadline before beginning the next step or bound the step itself by the remaining time.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
An HTTP status is an answer the origin actually sent, so it short-circuits the backoff: a 404 fails immediately instead of burning the budget, while 408/429 and the gateway statuses ride it. This restores Error::status() and status_retryable on moq-native and moq-hls (and the status plumbing through the QUIC backends) on top of the fail-fast defaults. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e1ead18e4
ℹ️ 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 server = qmux::Server::new() | ||
| .with_protocols(alpns.iter().map(|&a| (a, qmux_versions_for(a)))) | ||
| .with_keep_alive(qmux::KeepAlive::default()); | ||
| let server = qmux::Server::new().with_protocols(alpns.iter().map(|&a| (a, qmux_versions_for(a)))); |
There was a problem hiding this comment.
When a WebSocket peer disappears without sending a FIN, constructing qmux::Server without with_keep_alive(...) leaves the accepted session unaware of the disconnect until the operating system's TCP probes eventually fail. Broadcasts and other session state announced by that peer can therefore remain stale for hours, and the reconnect logic never gets a close event. Restore the explicit server keep-alive configuration that this line removes. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L89-L89
Useful? React with 👍 / 👎.
| const now = performance.now(); | ||
| const timeout = this.delay.timeout ?? DEFAULT_TIMEOUT; | ||
| this.#delay ??= this.delay.initial; | ||
| this.#deadline ??= timeout > 0 ? now + timeout : Number.POSITIVE_INFINITY; |
There was a problem hiding this comment.
When WebTransport.ready or the HTTP fingerprint fetch never settles, the configured 10-second timeout never fires because #deadline is initialized here only after connect() has already returned an error. The existing pending-WebTransport scenario can therefore leave Reload.closed pending and the status at connecting indefinitely. Arm the deadline before dialing and abort or race each connection attempt against the remaining budget. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
| // timeout means no deadline at all: retry for as long as the process lives. | ||
| let mut delay = backoff.initial; | ||
| let mut retry_start = tokio::time::Instant::now(); | ||
| let mut deadline = deadline_from(&backoff); |
There was a problem hiding this comment.
When a dial stalls, recording the deadline here does not bound client.connect(url.clone()).await; the default per-connect timeout is 30 seconds while the new reconnect budget is 10 seconds, and a sleep clipped to the deadline can also fall through into another full dial. Fresh evidence after the prior timeout finding is that the deadline now starts before the first attempt, but the attempt itself is still not raced against the remaining time. Bound each dial by that remainder so closed() surfaces the last failure near the promised budget. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
main's fail-fast retry rewrite (#2647) landed in the same part of `#retry` this branch touches. Took its jittered, deadline-bounded backoff wholesale and re-applied the two changes here on top: the `reload: false` early return, and routing the give-up path through `#finish` so it tears down the effect scope before settling `closed`. `ReloadDelay` lives in connect.ts on this branch, so main's updated docs and its 5s/10s defaults moved with it, and `#finish` now coerces via the shared `error()` helper main introduced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Addresses the
moq-dev/moqhalf of moq-dev/moq.pro#921 with a fail-fast policy: retries are bounded by time, with one narrow exception for HTTP statuses. The moq.pro half is untouched.The policy
Every retry loop is bounded by time, not by guessing at error types. An ephemeral failure is, by definition, one that clears within a few seconds; so each loop retries everything indistinguishably with capped exponential backoff and jitter, and gives up after a short budget (~10s), surfacing the last real error rather than a synthetic timeout.
The one exception is an answer a peer actually gave, where the protocol defines the meaning:
Error::status()on moq-native and moq-hls reports the HTTP status a server sent, and a status that isn't an invitation to retry (404,405, ...) fails immediately instead of burning the budget, while408,429, and the gateway/overload statuses ride the backoff. That is reading a response, not inferring intent from a failure; nothing without a status is classified.Concretely:
moq-native,js/netReload): default give-up budget drops from 5 minutes to 10 seconds, max delay from 30s to 5s. A dead relay or a typo'd URL now errors in ~10s instead of silently reconnecting for 5 minutes. Both remain configurable (Backoff/ReloadDelay), andtimeout = 0still means retry forever for supervised publishers (the gst sink keeps using it).404playlist ends it immediately.Bug fixes
pendingthrough the pause.run_remote_sessionreports even a clean close as an error, so the reset keyed onOk(())could never fire. Keying it on elapsed time then counted a slow dial as a healthy session. It now keys on the session's own lifetime.step(OnError::Warn)returnsOkeven when every rendition failed. Promoting that to a failure by segment count would break multi-rendition imports (a healthy live playlist writes no segments between passes), so it keys on renditions that ingested without error.Client::consumesizes from it.All six fixed-delay retry loops now escalate with jitter (native reconnect, cluster dials, HLS import/export, RTMP accept, audio device reopen, JS reconnect and capture reopen).
nix/modules/moq-relay.nixandpackaging/moq-relay/moq-relay.servicegainRestartSteps/RestartMaxDelaySec(systemd 254+; older versions warn and keep the flatRestartSec).Public API changes
One method, so this targets
main:Error::status() -> Option<u16>onmoq-nativeandmoq-hls. Nothing removed or signature-changed. Default values changed onmoq_native::Backoff(max 30s -> 5s, timeout 5m -> 10s) andjs/netReloadDelay(max 30000 -> 5000, timeout 300000 -> 10000).moq-relay,moq-audio, andmoq-hlsgain aranddependency (already in the lockfile) for jitter.No wire-format change, so no
drafts/update. Nomoq-ffichange, so no binding/wrapper sync. CLAUDE.md gains a short Retries section stating the policy.Test plan
just fix+just check: clean (after rebasing onto main; the branch had fallen behind fix(moq-video): reject unrepresentable sizes, and unbreak the macOS build #2648's macOS lint fixes).just rs testfor moq-native, moq-hls, moq-relay, moq-rtmp, moq-audio: 672 tests green.bun testfor@moq/net+@moq/publish: green.rs/moq-native/tests/reconnect.rsasserts the budget ends the loop and the give-up error names the underlying cause;rs/moq-native/src/quinn.rstests assert a rejected CONNECT carries its status through.Not verified locally
The
quichebackend never compiles on this host (boring-sysneeds cmake/nasm), nor doesmoq-gst(no GStreamer). CI covers both.🤖 Generated with Claude Code
(Written by Claude Fable 5)