refactor(net, relay): reshape the GOAWAY API and move migration into Reconnect

[kixelated]

Summary

An API reshape of #2490, built on @ksletmoe-aws's five commits so the GOAWAY
implementation, the codec work, and the test suite keep their authorship. Their
commits are unmodified apart from a rebase onto current main. #2490 is now
closed in favor of this, and its remaining piece (draining-route
deprioritization) is ported here.

What changed

moq-net: Drain/Draining become goaway::Producer/Consumer

session.send_goaway() -> Option<goaway::Producer>   // taken once per session
session.recv_goaway() -> goaway::Consumer           // peek / poll / recv

Every call site in #2490 was the same one-liner, drain().start*(uri).complete().await,
so the three-type typestate plus a claim released on drop was machinery with no
users. Producer::send consumes the handle, making "one GOAWAY per session" a
move rather than an atomic claim. The consumer matches bandwidth::Consumer on
the same struct, giving a sync peek, a composable poll, and a cloneable
handle the reconnect loop can hold.

The driver now owns the force-close deadline, so Session::closed() is the one
completion signal. That drops Draining entirely, along with the footgun where
it held a Session clone alive while waiting for a peer that might never leave.

Also gone: Session::is_going_away (now recv_goaway().peek()), public
Version::has_goaway, start vs start_with_timeout, and the Duration::ZERO
= "no deadline" magic value. Goaway::new() / Goaway::redirect(uri) /
.with_timeout(d) construct the message. URIs are String: there is at most one
GOAWAY per session, so Arc<str> was optimizing a once-per-connection allocation
while costing a conversion at every FFI boundary.

moq-net: two conformance fixes

Both are MUST-level in moq-transport draft-19 sect 10.4:

• A client naming a redirect URI is refused with ProtocolViolation. Clients
  cannot instruct servers to open connections, and a conformant server MUST close
  the session over it. feat(net, relay): GOAWAY with graceful drain and cluster migration #2490's API let a client send one and get killed for it.
• A duplicate GOAWAY closes the session instead of being logged and discarded.
  feat(net, relay): GOAWAY with graceful drain and cluster migration #2490's keep-first reasoning was sound (an observer may already be acting on the
  first URI) but leaves that observer acting on a URI the peer has replaced, with
  no way to tell.

send consumes the handle even when it returns Err: naming a URI as a client is
a bug to fix, not a runtime condition to recover from.

moq-native: Reconnect performs the migration

The lifecycle belongs in the reconnect loop, not in each application. Reconnect
already owns connect/backoff, and everything native reaches it (libmoq and
therefore py/swift/kt/go, moq-gst, moq-video), so they all get migration for free.

On a GOAWAY from a healthy session it dials the replacement while the old session
keeps serving, so old routes stay attached and live tracks hand over at a group
boundary (#2241). No backoff sleep: a handover is not a failure.

A session ending within backoff.initial counts as a failed attempt however it
ended, GOAWAY included. That one rule replaces #2490's CHURN_WINDOW/CHURN_LIMIT:
an A-redirects-to-B-redirects-to-A loop escalates through backoff and gives up at
backoff.timeout. The residual slow ping-pong (each leg healthy for seconds) is
left unguarded deliberately, since a long-lived session may legitimately migrate
many times and any fixed cap would eventually be wrong.

Redirects are sticky for the life of the loop. draft-19 scopes its MUST to
"the new session" and is silent beyond it, but a redirect is an assignment, not a
detour: reverting to the configured URL would redial the node that just told us
to leave. A fresh Reconnect starts from the configured URL again, which is the
right granularity and needs no extra bookkeeping.

Redirect is the trust policy (--client-goaway-redirect), defaulting to
Follow like every HTTP client. Beyond #2490's scheme-downgrade check it refuses
redirects that widen reachability: a public endpoint pointing at loopback, a
private range, or IPC. #2490 ranked unix in the top security tier, so an
authenticated upstream could aim the relay at a local socket and have it count as
an upgrade.

The old session drains inside the loop rather than in a detached task, so dropping
the Reconnect handle tears it down too.

moq-relay

run_remote_once is now client.reconnect(url), deleting the inline retry loop,
hardcoded backoff, churn constants, and the relay's own redirect resolver.

Connection becomes a builder, matching the Web::with_shutdown #2490 added
alongside it. This keeps the change additive and resolves #2490's open question
about main vs dev on the merits rather than by blast radius.

--drain-timeout takes a humantime duration ("10s", "500ms") like its neighbour
--cluster-linger. --cluster-drain-timeout is gone: the upstream handover
window is --client-goaway-handover, on the loop that actually performs it.

moq-native: the handover window is our deadline, not the peer's

--goaway-handover is a cap, applied whether or not the GOAWAY carries a
deadline of its own: whichever comes first wins. The peer's deadline is a
promise about when it force-closes, so waiting past it just holds a dead
session; ours is the cap on how long we keep one around at all, and a peer
naming an hour should not be able to talk us into honoring it.

An earlier revision let the peer's deadline win outright, which left the flag
applying only to a GOAWAY that named none. GoawayConfig::handover now takes
the peer's deadline and returns the effective window, so the policy lives in one
place instead of in a caller that has to remember to apply it.

moq-net: a draining session's routes are deprioritized

Ported from #2490 (@ksletmoe-aws's design), reshaped. This was the piece missing
from the earlier revision, and it is the one the original review asked for.

route_order ranks on (announce, cost, hop count, hash), so leaving a draining
route at its original cost let it stay primary through the hop/hash tie-break and
serve new groups right up until the session closed. Resume is group-granular, so
whatever group was in flight at the force-close was lost. The handover was meant
to happen before that, not at it.

Routes fed by a session now move to broadcast::DRAIN_COST when its peer drains.
Deprioritized, not withdrawn: a broadcast reachable only over the draining
session keeps being served, which is what makes the sender's deadline a handover
window rather than a cutoff. The two mechanisms compose, they don't overlap.

Three things differ from #2490's version:

• It hangs off the per-source task, not the announce loop. feat(net, relay): GOAWAY with graceful drain and cluster migration #2490 raced the
  announce decode against the GOAWAY signal on moq-lite, which means cancelling
  a partially-buffered transport read whenever the signal wins; whether that
  loses bytes depends on per-backend read cancel-safety we don't control. On
  IETF it only checked the flag when a message arrived, so an idle announce
  stream never deprioritized at all. Both wires now do it in the per-source task
  they already run, whose kio::wait races only in-memory pollables. Symmetric,
  and a peer that drains then goes quiet is handled on both.
• DRAIN_COST is the largest varint, not u64::MAX. A draining route is
  still announced downstream, and lite-06 carries the cost as a QUIC varint,
  which caps at 2^62-1. u64::MAX would fail to encode on the way out.
• It lives on broadcast::Dynamic. That is the handle each wire's per-source
  task already holds, so nothing keeps a second producer clone alive (and past
  its close-on-last-drop) just to reach the route.

moq-net: a pre-existing route-cost overflow

Found while sizing DRAIN_COST, and independent of GOAWAY. RouteCost::charged
saturated at u64::MAX, but the sum is re-encoded as a varint when forwarded. A
peer may legally advertise the largest varint there is, so adding this link's
price pushed the total out of range and the downstream encode failed with
BoundsExceeded, breaking the announce stream. Reachable on main today by any
lite-06 peer. Now saturates at the ceiling the wire can carry.

moq-native: Redirect::SameHost compared the authority

SameHost is documented (and named) as allowing a move between ports or schemes
but not to another host. It compared Url::authority(), which includes the port,
so it refused exactly the handoff it advertises. Surfaced by the redirect tests
below.

drafts

moq-lite specified the receiver-side "validate against local policy" rule but not
duplicate handling or whether a client may name a URI, so an implementer had no
way to be conformant. Added both, plus sender-side scheme continuity (matching
draft-19) and the sticky-redirect recommendation. The drain rule above is in as a
SHOULD, and the cost-saturation ceiling is now spelled out (the draft said
"saturating" without naming the cap, which is the bug above).

Resolved: the diamond failover regression

An earlier revision lost six groups across the handover in
cluster_diamond_goaway_seamless_failover. Root-caused and fixed, not silenced.

The "a session ending within backoff.initial counts as a failed attempt" rule
was applied to the handover as well as to the backoff bookkeeping.
backoff.initial is 1s, and a cluster peer that GOAWAYs shortly after the
handshake falls inside that window, so migration took the failure path: the old
session was dropped rather than kept alive, and the loop slept a second before
dialing the replacement. Nothing served the subscription across that gap.

The two concerns are now split. The handover is unconditional, so the old session
keeps serving until it closes or overstays its window. Only the bookkeeping
branches: an early GOAWAY still records a failed attempt and sleeps the backoff,
so two peers bouncing us between them still escalate and give up. A redirect loop
costs time, not data.

Worth noting for anyone reviewing the earlier revision's reasoning: my first
hypothesis (a pre-existing "replacement far ahead of the group cache" gap) was
wrong. The control experiment settled it — @ksletmoe-aws's original commits pass
this test on current main, so the regression was mine.

I also claimed at one point that #2473 conflicted behaviorally here, having
merged onto it and seen all three GOAWAY cluster tests hang. That was my own bug,
not an interaction: the experiment ran against the pre-fix code, whose early-GOAWAY
path tore the old session down, and #2473's stricter route selection turned the
resulting gap into a hang instead of a skip. Now that #2473 has landed, this
branch is rebased on it and all three pass.

Test plan

• just check clean (workspace tests, clippy, cargo doc -D warnings, taplo,
  remark); just drafts check parses.
• just rs loom 5/5, run because this touches model/.
• New: goaway_drains_routes_{moq_lite_04,moq_transport_19} (the peer sends
  nothing after the GOAWAY, so these only pass if the subscriber acted on the
  signal itself), drain_migrates_best_route, plus drain unit tests and the
  redirect-policy tests.
• Every new regression test was mutation-checked: reverting each fix fails the
  test that covers it, so none of them are vacuous.
• Not yet run locally: the JS suite and just test smoke-full. The js/net codec
  changes from feat(net, relay): GOAWAY with graceful drain and cluster migration #2490 are decode-only and untouched here.

Still open

• The relay keeps its outer backoff loop in run_remote on top of Reconnect's
  own. Functional but redundant; collapsing it means giving cluster peers an
  unlimited backoff.timeout.
• Status::Migrating does not cross the libmoq FFI boundary (OnStatus only
  reports Connected). Additive in Rust; needs a decision on whether C consumers
  should see it, which would touch moq.h, cpp/obs, and doc/lib/*.
• JS session-level GOAWAY is still deferred, and is an implementation rather than
  a mirror since @moq/net shares no code with moq-net.

(Written by Claude Opus 5)

[ksletmoe-aws]

@kixelated proposing we just go with this one as the path forward: I close #2490, and port over the one piece that is not here yet as a separate commit.

That piece is the draining-route deprioritization from your original review note. Right now goaway-api keeps the old routes attached at their normal cost and lets tracks hand over at a group boundary, which works when the replacement is genuinely cheaper. On equal cost though, route_order falls through to hop count and then hash, so the draining connection can stay primary. That is the case you wanted costed as infinite.

What the commit does: on GOAWAY receipt the subscriber bumps its own routes to broadcast::DRAIN_COST (u64::MAX), so best_route moves live subscribers off the dying connection. It sits at the session layer rather than in Reconnect, since that is where the broadcast producers actually live, so it needs no new public API and both lite and IETF get it. Tests cover the cost ordering and an origin-level migration between two competing routes.

I can rebase it onto goaway-api and send it your way, or you can fold it in yourself, or we drop it if you would rather solve the handover a different way. Let me know which and I will get out of your hair.

(Written by claude-opus-4.8)

[sourcery-ai]

Sorry @kixelated, your pull request is larger than the review limit of 150000 diff characters

[coderabbitai]

Walkthrough

Implemented GOAWAY support across protocol encoders, transport sessions, reconnect handling, and relay shutdown. Added URI validation, draft-specific timeout and request-ID processing, session APIs, subscription gating, redirect policies, predecessor draining, and relay-wide shutdown coordination. Added documentation plus unit, integration, broadcast, cluster migration, failover, timeout, and mock transport tests.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes cover #2490's GOAWAY, shutdown, migration, route-drain, codec, and regression-test requirements.
Out of Scope Changes check	✅ Passed	The diff stays focused on GOAWAY, shutdown, migration, and related tests/docs, with no clear unrelated feature work.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title clearly summarizes the main GOAWAY API reshape and Reconnect-based migration change.
Description check	✅ Passed	The description is directly related to the PR and accurately summarizes the GOAWAY, reconnect, and relay shutdown changes.

✨ Finishing Touches

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch goaway-api

---

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.}

[coderabbitai]

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

rs/moq-net/src/lite/goaway.rs (1)

24-48: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Enforce the URI cap on encode as well.

decode_msg rejects oversized values, but encode_msg writes them. A local send_goaway() with a URI over 8,192 bytes therefore succeeds locally and makes a compliant peer terminate the session for a protocol violation. Validate the byte length before encoding and share a named cap constant with the decoder and tests.

🤖 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/lite/goaway.rs` around lines 24 - 48, Update the GoAway
message encoding flow in encode_msg to reject URIs whose byte length exceeds
8,192 before writing them, returning the existing invalid-value encode error.
Define a named URI-cap constant and reuse it in both decode_msg and encode_msg,
ensuring tests reference the same constant rather than duplicating the literal.

🧹 Nitpick comments (8)

rs/moq-native/tests/broadcast.rs (1)

2475-2498: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

No coverage for the new Reconnect migration path.

These tests exercise raw session-level GOAWAY only. The behavior this layer adds (Redirect::resolve policy, Status::Migrating, sticky redirect URL, Draining handover, the healthy-vs-immediate backoff split) is untested here. A test that drives Client::reconnect against a server that GOAWAYs with a redirect to a second server would cover the interesting part. Want me to draft one?

🤖 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/tests/broadcast.rs` around lines 2475 - 2498, Add coverage for
the Reconnect migration path alongside the existing GOAWAY tests by driving
Client::reconnect against a server that sends GOAWAY with a redirect to a second
server. Assert Redirect::resolve policy, Status::Migrating, sticky redirect URL,
Draining handover, and distinct healthy-versus-immediate backoff behavior.

rs/moq-native/src/reconnect.rs (1)

114-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add inline tests for Redirect::resolve / scheme_tier.

These are the security boundary for peer-supplied URIs (scheme downgrade, reachability widening, SameHost), and the branches are cheap to pin down. Note one gap worth a case: when current has an unclassified scheme (tier 0, e.g. unix:), any target passes the downgrade check.

As per coding guidelines: "Keep Rust tests inline as #[cfg(test)] mod tests".

🤖 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 114 - 162, Add an inline
#[cfg(test)] mod tests beside Redirect::resolve and scheme_tier, covering empty
or ignored redirects, malformed URIs, scheme downgrades, reachability widening,
SameHost mismatches, and accepted redirects. Include a case where current uses
an unclassified tier-0 scheme such as unix: and any target passes the downgrade
check, while asserting scheme_tier rankings directly where useful.

Source: Coding guidelines

rs/moq-relay/src/shutdown.rs (1)

27-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider a getter instead of a public drain_timeout field.

A public field lets any holder of a cloned handle mutate its own drain window, silently diverging from the value main configured. A private field plus an accessor keeps the surface hard to misuse.

🤖 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/shutdown.rs` around lines 27 - 31, Make
Shutdown::drain_timeout private and add a read-only accessor method for
retrieving the configured Duration. Update existing consumers to call the
accessor instead of accessing the field directly, preserving the value
configured by main across cloned handles.

Source: Coding guidelines

rs/moq-relay/src/web.rs (1)

160-166: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

with_shutdown panics if any state clone exists.

routes() clones self.state into the router, so web.routes() followed by web.with_shutdown(...) aborts the process for an embedder. A panicking builder method is easy to misuse; consider holding shutdown on Web and constructing WebState lazily in routes()/serve(), so ordering never matters.

🤖 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/web.rs` around lines 160 - 166, Refactor the Web builder
around with_shutdown so it no longer relies on Arc::get_mut or panics after
routes() has cloned state. Store the shutdown value directly on Web, then apply
it when constructing WebState lazily in routes() or serve(), preserving shutdown
behavior regardless of whether with_shutdown is called before or after routes().

rs/moq-relay/tests/goaway_cluster.rs (1)

267-270: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the orphaned drain comment.

"Short drain so the test observes teardown quickly." no longer describes any code here (the drain configuration was dropped), leaving a misleading dangling comment.

♻️ Proposed cleanup

 	let mut cluster_config = ClusterConfig::default();
 	cluster_config.connect = vec![upstream_url.to_string()];
-	// Short drain so the test observes teardown quickly.
-
 	let mut client_config = moq_native::ClientConfig::default();

As per coding guidelines: "Comments and documentation must describe the current behavior, not historical migration context or obsolete behavior."

🤖 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/tests/goaway_cluster.rs` around lines 267 - 270, Remove the
obsolete “Short drain” comment immediately after the cluster_config.connect
assignment in the test setup, leaving the active ClusterConfig initialization
and connection configuration unchanged.

Source: Coding guidelines

rs/moq-relay/src/config.rs (1)

62-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a config-merge regression test for drain-timeout.

The TOML tests cover similar Option<Duration> fields, but rs/moq-relay/src/config.rs has no regression for the new --drain-timeout / MOQ_DRAIN_TIMEOUT flag leaving drain_timeout intact when the CLI flag is absent, or being overridden when present.

🤖 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/config.rs` around lines 62 - 73, The config merge tests
around the relay configuration must add regression coverage for drain_timeout:
verify a TOML-configured value remains intact when --drain-timeout is absent,
and verify the CLI value overrides it when provided. Reuse the existing
Option<Duration> TOML merge test pattern and reference the drain_timeout field
and its CLI/environment configuration.

Source: Coding guidelines

js/net/src/lite/goaway.ts (1)

31-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider a shared named constant for the 8,192-byte cap.

The same literal and near-identical check now live in js/net/src/ietf/goaway.ts (and in the Rust decoders). A shared exported constant (e.g. MAX_NEW_SESSION_URI_BYTES) keeps the limit and the error wording in one place.

As per coding guidelines: "Avoid using magic numbers; use named constants instead."

🤖 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/lite/goaway.ts` around lines 31 - 37, Replace the hard-coded
8,192-byte limit in the Goaway decoder with a shared exported named constant,
such as MAX_NEW_SESSION_URI_BYTES, and reuse that constant in both the lite and
IETF goaway implementations. Update the associated error wording to reference
the shared limit while preserving the existing byte-length validation behavior.

Source: Coding guidelines

rs/moq-native/src/lib.rs (1)

47-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider #[non_exhaustive] on the newly public GoawayConfig and Redirect.

Both become part of the published surface here. Redirect is a policy enum likely to gain variants, and GoawayConfig is an additive config struct with public fields and a Default. Existing exports (Backoff) share this shape, so this is a consistency call rather than a blocker.

As per coding guidelines: "For Rust config structs with public fields, use #[non_exhaustive] plus Default or a constructor; mark public enums that may gain variants as #[non_exhaustive]."

🤖 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/lib.rs` at line 47, Mark the newly public GoawayConfig
struct and Redirect enum as #[non_exhaustive] in their definitions within the
reconnect module. Preserve GoawayConfig’s existing Default implementation and
public API behavior, and leave the re-export in lib.rs 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 `@doc/concept/layer/moq-lite.md`:
- Around line 143-153: The lifecycle documentation should use the current GOAWAY
API instead of the removed drain flow: replace the sending-side references to
session.drain(), Drain, start, and Draining with session.send_goaway(),
Producer::send(Goaway), and Session::closed(), and update the receiving-side
flow to use session.recv_goaway() and Session::closed(). Preserve the documented
behavior for rejecting new requests, retaining existing subscriptions, and
reconnecting through a replacement session.

In `@rs/moq-native/src/client.rs`:
- Around line 63-67: Update the doc comment for Client::goaway to advertise the
actual flattened CLI flag names defined by GoawayConfig, replacing the
inaccurate --client-goaway-* pattern once the finalized flag names are
confirmed.

In `@rs/moq-native/src/reconnect.rs`:
- Around line 90-93: Update the documentation for Reconnect::status to describe
all current connection states, including Migrating, instead of claiming the loop
only alternates between Connected and Disconnected. Keep the documentation
aligned with the status behavior introduced by the Migrating variant.
- Around line 337-378: Ensure the reconnect loop continues polling an existing
Draining predecessor while connection attempts and backoff sleeps are pending,
so its handover deadline is enforced even when reconnects fail. Update the
immediate-redirect retry path around run_session and client.connect to race or
otherwise service draining alongside connect/backoff waits. Before assigning a
new Draining in the Ended::Goaway branch, retire the existing predecessor
instead of silently overwriting it.
- Around line 176-202: The GoawayConfig CLI fields register incorrect names and
scalar defaults that overwrite TOML values. In rs/moq-native/src/reconnect.rs
lines 176-202, set explicit client-goaway long names, use the
MOQ_CLIENT_GOAWAY_* environment variables, change redirect and handover to
Option<T> without default_value, apply fallback defaults at the use sites around
lines 348 and 359, and add a regression test confirming CLI defaults do not
override TOML configuration. In rs/moq-native/src/client.rs lines 63-67, update
the documentation to advertise the --client-goaway-* flags.
- Around line 165-173: Update is_local to classify IPv6 link-local addresses,
IPv4-mapped IPv6 addresses, and the unspecified IPv4 address 0.0.0.0 as local.
Preserve the existing domain, Unix-socket, loopback, private, and link-local
checks, and apply the IPv4 classification to mapped addresses before returning
the IPv6 result.

In `@rs/moq-net/src/goaway.rs`:
- Around line 90-99: The GOAWAY send path must reject oversized URIs and prevent
message-length truncation. In rs/moq-net/src/goaway.rs lines 90-99, update
Producer::send to return Error::ProtocolViolation when goaway.uri exceeds 8,192
bytes; in rs/moq-net/src/ietf/adapter.rs lines 545-576, update send_goaway to
convert body.len() with try_into() instead of an unchecked u16 cast, handling
conversion failure as an error.

In `@rs/moq-net/src/ietf/session.rs`:
- Around line 512-538: Update the post-GOAWAY drain loop in the SETUP stream
handler to preserve duplicate-GOAWAY enforcement: when the decoded message ID is
ietf::GoAway::ID, record it through goaway.record(...) and propagate its error
instead of only logging and ignoring it. Keep the existing discard-and-log
behavior for other message types.

In `@rs/moq-net/src/ietf/subscriber.rs`:
- Around line 90-92: Update run_subscribe_namespace to check the shared
going_away state before allocating a request ID or opening the
SUBSCRIBE_NAMESPACE stream; return Error::GoingAway when the peer has sent
GOAWAY, while preserving the existing request flow otherwise.

In `@rs/moq-net/tests/support/mock.rs`:
- Around line 286-307: Update accept_uni and accept_bi to create the
close_notify.notified() future before awaiting rx.recv(), then check the close
signal after the channel returns None using the same notified-before-check
pattern as closed(). Preserve returning the received stream or pair immediately,
and return close_error() once shutdown is confirmed without introducing a
post-close notification race.

---

Outside diff comments:
In `@rs/moq-net/src/lite/goaway.rs`:
- Around line 24-48: Update the GoAway message encoding flow in encode_msg to
reject URIs whose byte length exceeds 8,192 before writing them, returning the
existing invalid-value encode error. Define a named URI-cap constant and reuse
it in both decode_msg and encode_msg, ensuring tests reference the same constant
rather than duplicating the literal.

---

Nitpick comments:
In `@js/net/src/lite/goaway.ts`:
- Around line 31-37: Replace the hard-coded 8,192-byte limit in the Goaway
decoder with a shared exported named constant, such as
MAX_NEW_SESSION_URI_BYTES, and reuse that constant in both the lite and IETF
goaway implementations. Update the associated error wording to reference the
shared limit while preserving the existing byte-length validation behavior.

In `@rs/moq-native/src/lib.rs`:
- Line 47: Mark the newly public GoawayConfig struct and Redirect enum as
#[non_exhaustive] in their definitions within the reconnect module. Preserve
GoawayConfig’s existing Default implementation and public API behavior, and
leave the re-export in lib.rs unchanged.

In `@rs/moq-native/src/reconnect.rs`:
- Around line 114-162: Add an inline #[cfg(test)] mod tests beside
Redirect::resolve and scheme_tier, covering empty or ignored redirects,
malformed URIs, scheme downgrades, reachability widening, SameHost mismatches,
and accepted redirects. Include a case where current uses an unclassified tier-0
scheme such as unix: and any target passes the downgrade check, while asserting
scheme_tier rankings directly where useful.

In `@rs/moq-native/tests/broadcast.rs`:
- Around line 2475-2498: Add coverage for the Reconnect migration path alongside
the existing GOAWAY tests by driving Client::reconnect against a server that
sends GOAWAY with a redirect to a second server. Assert Redirect::resolve
policy, Status::Migrating, sticky redirect URL, Draining handover, and distinct
healthy-versus-immediate backoff behavior.

In `@rs/moq-relay/src/config.rs`:
- Around line 62-73: The config merge tests around the relay configuration must
add regression coverage for drain_timeout: verify a TOML-configured value
remains intact when --drain-timeout is absent, and verify the CLI value
overrides it when provided. Reuse the existing Option<Duration> TOML merge test
pattern and reference the drain_timeout field and its CLI/environment
configuration.

In `@rs/moq-relay/src/shutdown.rs`:
- Around line 27-31: Make Shutdown::drain_timeout private and add a read-only
accessor method for retrieving the configured Duration. Update existing
consumers to call the accessor instead of accessing the field directly,
preserving the value configured by main across cloned handles.

In `@rs/moq-relay/src/web.rs`:
- Around line 160-166: Refactor the Web builder around with_shutdown so it no
longer relies on Arc::get_mut or panics after routes() has cloned state. Store
the shutdown value directly on Web, then apply it when constructing WebState
lazily in routes() or serve(), preserving shutdown behavior regardless of
whether with_shutdown is called before or after routes().

In `@rs/moq-relay/tests/goaway_cluster.rs`:
- Around line 267-270: Remove the obsolete “Short drain” comment immediately
after the cluster_config.connect assignment in the test setup, leaving the
active ClusterConfig initialization and connection configuration unchanged.

🪄 Autofix (Beta)

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: 4a3c3cf2-7026-4894-b97a-26515ede72b7

📥 Commits

Reviewing files that changed from the base of the PR and between da45f8a and da4d6f6.

📒 Files selected for processing (41)

• doc/bin/relay/config.md
• doc/concept/layer/moq-lite.md
• drafts/draft-lcurley-moq-lite.md
• js/net/src/ietf/goaway.ts
• js/net/src/ietf/ietf.test.ts
• js/net/src/lite/goaway.test.ts
• js/net/src/lite/goaway.ts
• rs/moq-native/src/client.rs
• rs/moq-native/src/lib.rs
• rs/moq-native/src/reconnect.rs
• rs/moq-native/tests/broadcast.rs
• rs/moq-net/src/client.rs
• rs/moq-net/src/error.rs
• rs/moq-net/src/goaway.rs
• rs/moq-net/src/ietf/adapter.rs
• rs/moq-net/src/ietf/goaway.rs
• rs/moq-net/src/ietf/session.rs
• rs/moq-net/src/ietf/subscriber.rs
• rs/moq-net/src/lib.rs
• rs/moq-net/src/lite/goaway.rs
• rs/moq-net/src/lite/publisher.rs
• rs/moq-net/src/lite/session.rs
• rs/moq-net/src/lite/subscriber.rs
• rs/moq-net/src/lite/version.rs
• rs/moq-net/src/server.rs
• rs/moq-net/src/session.rs
• rs/moq-net/src/version.rs
• rs/moq-net/tests/goaway.rs
• rs/moq-net/tests/support/harness.rs
• rs/moq-net/tests/support/mock.rs
• rs/moq-net/tests/support/mod.rs
• rs/moq-relay/src/cluster.rs
• rs/moq-relay/src/config.rs
• rs/moq-relay/src/connection.rs
• rs/moq-relay/src/lib.rs
• rs/moq-relay/src/main.rs
• rs/moq-relay/src/shutdown.rs
• rs/moq-relay/src/web.rs
• rs/moq-relay/src/websocket.rs
• rs/moq-relay/tests/goaway_cluster.rs
• rs/moq-relay/tests/smoke.rs

[coderabbitai]

Actionable comments posted: 1

🤖 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-net/src/model/broadcast.rs`:
- Around line 52-58: Enforce MAX_COST in every public route construction path,
including Route::new and Route::with_cost, so no Route::cost can exceed the wire
ceiling; clamp or reject oversized values consistently with the existing API
contract. Add a regression test covering Route::new().with_cost(u64::MAX) and
verify the resulting route cannot carry a cost above MAX_COST.

🪄 Autofix (Beta)

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: 34855a26-0d98-493c-b47b-faf44b62d867

📥 Commits

Reviewing files that changed from the base of the PR and between da4d6f6 and c3e9f04.

📒 Files selected for processing (16)

• doc/bin/relay/config.md
• doc/concept/layer/moq-lite.md
• drafts/draft-lcurley-moq-lite.md
• rs/moq-native/src/client.rs
• rs/moq-native/src/reconnect.rs
• rs/moq-net/src/goaway.rs
• rs/moq-net/src/ietf/adapter.rs
• rs/moq-net/src/ietf/session.rs
• rs/moq-net/src/ietf/subscriber.rs
• rs/moq-net/src/lite/announce.rs
• rs/moq-net/src/lite/subscriber.rs
• rs/moq-net/src/model/broadcast.rs
• rs/moq-net/src/model/origin.rs
• rs/moq-net/tests/goaway.rs
• rs/moq-net/tests/support/mock.rs
• rs/moq-relay/tests/goaway_cluster.rs

🚧 Files skipped from review as they are similar to previous changes (10)

• doc/concept/layer/moq-lite.md
• rs/moq-native/src/client.rs
• rs/moq-net/tests/goaway.rs
• rs/moq-relay/tests/goaway_cluster.rs
• rs/moq-net/src/ietf/adapter.rs
• rs/moq-net/src/lite/subscriber.rs
• rs/moq-net/src/goaway.rs
• rs/moq-net/tests/support/mock.rs
• rs/moq-net/src/ietf/session.rs
• rs/moq-native/src/reconnect.rs

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Enforce MAX_COST at every public route entry point.

Route::cost is public and Route::with_cost accepts any u64, so callers can still create a cost above this ceiling. Such a route can reach Lite-06 forwarding and fail varint encoding, contradicting the new “capped” contract. Clamp or reject oversized costs when routes enter the model and add a regression test using Route::new().with_cost(u64::MAX).

Also applies to: 87-87

🤖 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/model/broadcast.rs` around lines 52 - 58, Enforce MAX_COST in
every public route construction path, including Route::new and Route::with_cost,
so no Route::cost can exceed the wire ceiling; clamp or reject oversized values
consistently with the existing API contract. Add a regression test covering
Route::new().with_cost(u64::MAX) and verify the resulting route cannot carry a
cost above MAX_COST.

[kixelated]

@kixelated proposing we just go with this one as the path forward: I close #2490, and port over the one piece that is not here yet as a separate commit.

That piece is the draining-route deprioritization from your original review note. Right now goaway-api keeps the old routes attached at their normal cost and lets tracks hand over at a group boundary, which works when the replacement is genuinely cheaper. On equal cost though, route_order falls through to hop count and then hash, so the draining connection can stay primary. That is the case you wanted costed as infinite.

What the commit does: on GOAWAY receipt the subscriber bumps its own routes to broadcast::DRAIN_COST (u64::MAX), so best_route moves live subscribers off the dying connection. It sits at the session layer rather than in Reconnect, since that is where the broadcast producers actually live, so it needs no new public API and both lite and IETF get it. Tests cover the cost ordering and an origin-level migration between two competing routes.

I can rebase it onto goaway-api and send it your way, or you can fold it in yourself, or we drop it if you would rather solve the handover a different way. Let me know which and I will get out of your hair.

(Written by claude-opus-4.8)

Yeah I'm working on it. We might not even need the DRAIN_COST thing if we switch to preferring the newer route, which is generally the correct one.