[kixelated]

Problem

Backoff::timeout documents 0 as unlimited retries, but that only worked through --backoff-timeout 0 or MOQ_BACKOFF_TIMEOUT=0. On a config the command line built, a value set in code had no effect:

• usage builds a default_value_t by formatting it and parsing the string back, so CliDuration::fallback(10s) reaches the struct as an explicit 10s, and CliDuration::resolve returns it over the typed field.
• Even if the fallback survived, a parse leaves the #[usage(skip)] field at Duration::ZERO, which resolve reads as "unset". A zero set afterwards looked the same as no value at all.

So config.backoff.timeout = Duration::ZERO on a parsed (or settings-merged) config still gave up after 10s. A probe against main resolved 10s for a parsed config set to zero, and 10s for a merged config set to either 3s or zero. In-tree, Cluster::lan_client sets zero on the relay's merged connect config.

Approach

• Backoff::timeout becomes Option<Duration>. None means unset, and Some(Duration::ZERO) means unlimited. Backoff::default() still holds Some(10s).
• --backoff-timeout has no parser default, so it is present only when actually given (flag or env). The window in effect is the flag, then the typed field, then 10s.
• Help no longer prints [default: 10s] for the flag. The doc comment still says it.

Impact

• Public API (breaking): moq_tokio::Backoff::timeout goes from Duration to Option<Duration>. In-tree callers updated: moq-c, moq-ffi, moq-gst, moq-relay, tests.
• C ABI, FFI records, wire: unchanged. moq_client_defaults still reports 10s with has_backoff_timeout set.
• Merge note: rebased onto fix(moq-tokio): stop reconnecting on terminal setup errors #5237, whose three new connection.rs tests set backoff.timeout to a bare Duration; they now wrap it in Some(...). fix(moq-tokio): the retry window no longer cuts a dial short #5352 and fix(moq-tokio): a restarting peer's GOAWAYs do not exhaust the retry window #5351 wrap theirs when they rebase onto this.

Validation

• connection::tests::a_timeout_set_in_code_beats_the_parser_default covers a parsed config, a merged config (zero and nonzero), an explicit flag still winning, and a config built in code. Restoring the parser default fails it: a parsed config, left: 10s, right: 0ns.
• nix develop --command just check passed: 3,806 Rust tests (13 skipped) plus the moq-c, moq-gst, FFI and Python suites.

Follow-up

The same parser behavior affects every other CliDuration-backed field: connect timeout, race, resolution delay, GOAWAY handover, QUIC idle timeout and keep-alive, listen timeout, WebSocket delay, and backoff initial and max. On a parsed or merged config, a value set in code is ignored, because fallback never survives the parse and every *_arg comes out explicit. Keep-alive has the same zero ambiguity, since zero disables it. This is planned in quest/m1/cli-duration-defaults.md (#5387): move every one of these fields to the shape used here.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-12T14:58:36.653530Z	5c296e0	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 5c296e0

No actionable bugs found in the changed code and inspected call sites.

Direction: distinguishing unset from explicit zero with Option, removing the parser default, and centralizing flag → typed value → 10s resolution is a focused fix. The documented Rust API break is reflected in the in-tree callers, while the C/FFI representations remain unchanged. Keeping the broader duration-parser cleanup separate is reasonable.

Verification: inspected all 11 changed files, the settings merge/serialization path, retry-window consumption, and native adapters. Regression tests cover parsed and round-tripped configs plus explicit flag precedence. This was a GitHub-only static review; I did not run builds or tests or independently verify the reported test results.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 29d8cb70-2b5e-4c86-828e-67cca6d1f929

📥 Commits

Reviewing files that changed from the base of the PR and between 5c296e0 and 8dd376e.

📒 Files selected for processing (1)

• rs/moq-tokio/src/connection.rs

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

Walkthrough

Reconnect backoff timeouts now use an optional duration. The effective retry window is selected from the CLI value, the configured value, or the default. The change adds Serde support for optional durations and updates C, FFI, GStreamer, and relay configuration callers to use the optional value. Tests cover timeout defaults, zero and nonzero values, precedence, and TOML round-tripping.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 8dd37

No actionable issue is established for this change; it is mergeable after normal checks.

Pre-merge checks · 4 · 1

Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 73.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 11 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks (4 passed)

Check name	Status	Explanation
Description check		The description clearly explains the backoff timeout bug, the Option-based fix, precedence behavior, API impact, updated callers, and validation results. It is directly related to the changeset.
Title check		The title concisely identifies the main fix: a backoff timeout set in code takes precedence over the parser default. The breaking API marker is also appropriate for the documented type change.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

✨ Simplify code

• Commit to this branch
• Create a new PR

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

Warning

Some tools did not complete. Review the errors below.

🔧 Clippy (1.98.1)

Clippy execution timed out

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[kixelated]

Merging. Rebased onto main after #5335 and #5284. The only change beyond the rebase: #5237's three new connection.rs tests set backoff.timeout to a bare Duration, so they now wrap it in Some(...). Every in-tree caller (moq-c, moq-ffi, moq-gst, moq-relay, tests) uses the Option; the C ABI, FFI records, and docs are unchanged. Codex and the OpenAI review were clean on the original head, and just check passes on this one.

The follow-up for the other CliDuration fields is already planned in quest/m1/cli-duration-defaults.md (#5387).

(Written by Claude Opus 5.5)