Skip to content

fix(reconnect): a first session refused before it is healthy fails fast - #8817

Open
kixelated wants to merge 5 commits into
mainfrom
quest/m1/first-dial-refusal
Open

kixelated wants to merge 5 commits into
mainfrom
quest/m1/first-dial-refusal

Conversation

@kixelated

@kixelated kixelated commented Nov 15, 2026 •

Copy link
Copy Markdown
Collaborator

Completes quest/m1/first-dial-refusal.md.

Problem

A client whose first session is refused right after ALPN (a rejected token on moq-lite 03 or later, which has no server SETUP to refuse in) redialed forever: Reconnect marked the policy established as soon as the transport connected, so the relay's MalformedAuthToken close went through closed() and backed off under Backoff::timeout: None indefinitely. moq-dev/moq#8694 saw one refusal a second in the relay log and changed the interop test instead. The gst docs promised failed on a rejected token, which had been false since moq-dev/moq#8618.

Approach

A session establishes the policy only once it stays up past Backoff::healthy; no error kind is read.

  • rs/moq-net/src/reconnect.rs: connected() no longer sets established; end() sets it from the session's tenure (so a long session that ends in a redirect loop still counts) and probed() from the fallback's settle. closed() ends the policy with the close reason while not established, as walk_failed already did for a failed walk.
  • js/net/src/connection/reload.ts: #establishedHref is set by #settle at each session's end (close, GOAWAY, fallback probe, and the run's teardown on a page hide) once it lived past healthy; a close before then gives up the URL with that error.
  • Handover.tla: established is a variable set where the code sets it, EndClosed ends the policy until then, and FirstWalkFailsFast now also covers closes (a GOAWAY's backoff, which the peer asked for, is tracked as drained and exempt). All three configs pass (1.77M, 389k, 401k distinct states); reverting EndClosed to always back off violates FirstWalkFailsFast. Skill table, counts, and sync commit updated.
  • Docs: doc/bin/gstreamer.md and rs/moq-gst/src/sink/session.rs now promise failed for a first session that fails or is refused before it is healthy; the stale "refusal of since-replaced tokens redials" claims in moq-ffi and the verify-reconnect skill are gone (no code did that since feat(reconnect)!: the reconnect loop stops judging errors by kind #8618); moq-tokio, moq-c, moq-ffi, and Python Backoff docs say when retrying starts. Dart bindings regenerated for the ffi doc change.
  • Tests that relied on an immediate close being redialed now hold the first session past the healthy bar (on the simulated or paused clock where the test already used one, a real 1 s or FAST wait where it runs real sockets). test/drill/mutations/reconnect-stops-after-session-loss.patch retargeted to the new context.

Decisions

Taken while the maintainer was away; each is reversible in review.

  • A refused first session ends even when the tokens were replaced since its dial. a_first_settled_answer_fails_fast already settled that for a refused dial, and keeping an exception only for the after-ALPN path would make the same refusal behave differently by version. tokens::a_refused_credential_redials_with_its_replacement and the JS counterpart became tests of the fail-fast instead. Alternative: retry once when the connection credential changed since the dial (a small Tokens::replaced check), for both paths.
  • A GOAWAY before the healthy bar still migrates or backs off as before; only failures (a failed walk, a close) fail fast. So a first session drained by a restarting relay is followed, but if its replacement then closes before it is healthy, the policy ends, dropping a predecessor that may still serve. Alternative: count a GOAWAY as establishing.
  • The Verify skill's sync commit moves to this branch's base on main, as feat(moq-relay): restart beside the draining relay without dropping sessions #8596 did.

Impact

  • Public API: none. Wire: none.
  • Behavior: a client (Rust Reconnect/moq_tokio::Connection, JS Connection/Reload, and every binding through moq-ffi) whose first session closes before healthy (default 1 s) ends with that close's error instead of redialing. After a healthy session, every failure retries as before.

Validation

  • just test interop --all: passes, including the browser refused session case, now asserting a WebSocket Connection stops after exactly one refused session against a real relay process with real auth (it failed with "gave up after 1 session(s) instead of redialing" before the case was updated, which is the new behavior on the real surface).
  • New unit tests fail without the fix: reconnect::tests::a_first_session_closed_before_healthy_fails_fast deadlocks redialing, and reload.test.ts "a first session refused before it is healthy stops the URL" times out.
  • Integration: moq-tokio reconnect_ends_on_a_first_session_rejection (real QUIC server rejecting with each of Unauthorized, MalformedAuthToken, Expired: one accept, then the loop ends) and reconnect_retries_a_rejection_after_a_healthy_session; moq-relay websocket_unauthorized_ends_a_first_session; moq-c server_rejects_a_session (terminal snapshot carries the rejection).
  • TLA+ as above, including the mutation check.
  • just check passes except one py/moq-rs/tests/test_socket.py::test_abort_and_endpoint_close_are_errors run (unroutable instead of Closed); the socket model does not use the reconnect policy, and a rerun passed. Follow-up below.
  • Gate cost: about ten real-time tests gained a 1 s (or 50 ms) wait to age their first session; they run in parallel lanes, so the gate's wall time moves by a second or two.

Caused by

  • moq-dev/moq#8618 - removed the kind-based stop in closed() while connected() still marked the policy established, so a refused first session redialed forever (found by reviewer; the feature review).

Follow-ups

Models

Size: S
Implementer: Claude, claude-opus-5-5, unknown effort (arm A)

🤖 Generated with Claude Code

Review

Rewritten by quest review after each round.

  • Head: 7e93c67

  • Base: origin/main at ca88fc1

  • Risk: medium, the base has no RISK.md to name what is always high or customer-facing, so every change is medium

  • Classified: medium by default

  • Reviewer: Codex, gpt-6.1-sol, medium effort

  • Session: 01a1dfae-be9b-7371-b057-98b45fcd8185, round 1

  • Score: 8/10, The healthy-session threshold is applied consistently in Rust and JS with corresponding model, documentation, and test updates; no actionable defects were found by source inspection, but CI confirmation remains pending.

  • Rounds: 1; first round 0; findings 0: fixed 0, refuted 0, open 0

  • Findings: none

kixelated and others added 5 commits November 15, 2026 04:26
A session now establishes the reconnect policy only once it stays up past
Backoff::healthy. Until then a closed session ends the policy like a failed
walk, so a relay refusing a token right after ALPN (moq-lite 03+) reports the
refusal instead of redialing forever. Rust Reconnect and js/net Reload agree;
Handover.tla models the new rule.

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

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Also adds a follow-up quest for a socket close flake seen in the py tests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title A first session refused at SETUP fails fast fix(reconnect): a first session refused before it is healthy fails fast Nov 15, 2026
@kixelated
kixelated marked this pull request as ready for review November 15, 2026 14:50
@coderabbitai

coderabbitai Bot commented Nov 15, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 12 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 55a23d3a-42a6-4b69-8f8a-419a10db1cd9
Commits

Reviewing files that changed from the base of the PR and between fabfe5e and 7e93c67.

Files selected for processing: 36

  • .claude/skills/verify-reconnect/SKILL.md
  • dart/moq_ffi/lib/src/moq.dart
  • dart/moq_ffi/lib/src/uniffi_runtime.dart
  • doc/bin/gstreamer.md
  • go/wrapper/reconnect_test.go
  • js/net/src/connection/migrate.test.ts
  • js/net/src/connection/pool.test.ts
  • js/net/src/connection/reload.test.ts
  • js/net/src/connection/reload.ts
  • js/net/src/integration.test.ts
  • py/moq-rs/moq/_records.py
  • py/moq-rs/tests/test_server.py
  • quest/m1/first-dial-refusal.md
  • quest/m1/socket-close-flake.md
  • rs/moq-c/src/api.rs
  • rs/moq-c/src/test.rs
  • rs/moq-ffi/src/session.rs
  • rs/moq-ffi/src/test.rs
  • rs/moq-gst/src/sink/session.rs
  • rs/moq-gst/src/source/imp.rs
  • rs/moq-net/src/reconnect.rs
  • rs/moq-net/src/reconnect/tests.rs
  • rs/moq-net/verify/reconnect/Handover.tla
  • rs/moq-relay/tests/integration/auth_lifetime.rs
  • rs/moq-relay/tests/integration/handoff.rs
  • rs/moq-tokio/src/connection.rs
  • rs/moq-tokio/tests/integration/broadcast.rs
  • rs/moq-tokio/tests/integration/fresh_port.rs
  • rs/moq-tokio/tests/integration/reconnect.rs
  • rs/moq-tokio/tests/integration/sim_client.rs
  • rs/moq-tokio/tests/integration/tokens.rs
  • test/drill/mutations/reconnect-stops-after-session-loss.patch
  • test/interop/README.md
  • test/interop/clients/js/close.ts
  • test/interop/clients/js/src/close.ts
  • test/interop/interop.sh
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@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 gap across the bindings.

Reviewed changes

I reviewed the full diff at 7e93c67: the Reconnect and Reload fail-fast change, the Handover.tla update, and the test and doc updates across Rust, JS, Go, Python, C, FFI and GStreamer.

  • Rust Reconnect: established now comes from session tenure in end() and from settle in probed(), not from connected(). closed() ends the policy while the policy is not yet established, and it does this before the predecessor migrate-back.
  • JS Reload: #establishedHref is set only by #settle when a session ends. That covers a close, a GOAWAY, a probe, and run teardown on a page hide or disable. connected moved to the run scope so the cleanup can read it. A close before the healthy bar now calls #giveUp.
  • Parity checked: a GOAWAY before the healthy bar still migrates or backs off on both sides, and the failure after it then ends the policy on both sides. Under Restart::Wait/Handoff, a session drained before it is healthy keeps serving until the peer closes it. Its tenure includes that wait, so it still establishes the policy unless the drain deadline is under 1 s.
  • Model: established and drained are now real variables, and FirstWalkFailsFast allows only a GOAWAY's backoff. The retargeted drill mutation patch still applies cleanly.
  • Tests: tests that relied on an immediate redial now wait past the healthy bar first. The real-clock waits begin after the client reports Connected, so tenure >= healthy holds when the close lands.

ℹ️ The Go, Swift, Kotlin and Dart wrappers still describe the old fail-fast rule

The PR updates the Backoff docs for Python (_records.py), moq-ffi, moq-tokio and moq-c to say retrying starts only after a session stays up for 1 s. The matching Backoff and connect docs in the other bindings, and the user-facing doc/lib pages, still say only "a first dial that reaches no session ends the connection". An app developer reading those docs would expect a token refused after connecting to be retried, which is the behavior this PR removes. AGENTS.md asks for the same functionality across all languages and wrappers, and doc/ is what end users read.

Technical details
# Wrapper and user docs still describe the pre-#8817 fail-fast rule

## Affected sites
- go/wrapper/client.go:209 — `Backoff` doc: "applies only once a session has been established: a first dial that reaches no session…"
- go/wrapper/client.go:275-276 — `Connect` doc: "If that first dial reaches no session… only an established session reconnects"
- swift/Sources/Moq/Aliases.swift:93-94 — `Backoff` doc, same wording
- swift/Sources/Moq/Client.swift:68 — connect doc
- kt/moq/src/jvmAndAndroidMain/kotlin/dev/moq/Aliases.kt:389 — `Backoff` doc
- dart/moq/lib/src/aliases.dart:276-277 — `Backoff` doc
- dart/moq/lib/src/client.dart:64 — connect doc
- doc/lib/index.md:69 — "a relay that never answered with a session ends the connection"
- doc/lib/{go,kt,swift,dart,cpp,py}/index.md — Stats bullets: "A first dial that reaches no session ends the session with that error rather than retrying."
- py/moq-rs/README.md:163 — same sentence

## Required outcome
- Each binding's `Backoff`/connect docs match the wording already used in `py/moq-rs/moq/_records.py` and `rs/moq-ffi/src/session.rs`: until a session has stayed up past 1 s, a failed dial or a closed session (such as a refused credential) ends the connection with its error rather than retrying.
- The `doc/lib` pages state the same rule.

Pullfrog  | Fix it ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

This branch has not been deployed

No deployments
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