Skip to content

fix(relay): drain the relay endpoint before dismantling its tunnel - #138

Open
grumbach wants to merge 2 commits into
mainfrom
fix/relay-teardown-transport-error
Open

fix(relay): drain the relay endpoint before dismantling its tunnel#138
grumbach wants to merge 2 commits into
mainfrom
fix/relay-teardown-transport-error

Conversation

@grumbach

@grumbach grumbach commented Aug 14, 2026

Copy link
Copy Markdown
Member

Linear issue

https://linear.app/autonominetwork/issue/V2-986/relay-recv-stream-closed-transport-errors-up-100x-fleet-wide-after-the

Risk tier

  • T0 — docs / tooling / CI / pure UX-output. Repo CI only.
  • T1 — client-only, no network-facing behavior change. CI + prod compat smoke.
  • T2 — node/client logic with behavioral surface, no protocol/format/economics change. Dev testnet + ADR.
  • T3 — protocol / storage format / payments / routing. T2 evidence + adversarial testing.

Compatibility

  • Wire: no frame, transport-parameter or format change. Two things a peer or relay
    server can observe differently:
    • Connections on a discarded relay endpoint now normally receive the
      CONNECTION_CLOSE that close() queues, because the tunnel is no longer
      destroyed out from under them first. This is best-effort, not guaranteed. The
      unmodified 0.36.0 already decodes and surfaces it as ApplicationClosed, so
      no peer version is surprised by it.
    • Teardown holds the outer CONNECT-UDP stream, and so the relay server's
      capacity slot, for up to the one-second drain budget longer.
      NatTraversalEndpoint::shutdown inherits the same bound before it closes
      ordinary connections.
  • Storage: none.
  • API: no public signature changes. The teardown path the health monitor uses now
    marks the tunnel failed first and closes with the reason relay tunnel unhealthy instead of relay allocation aborted. That string travels in the
    application CONNECTION_CLOSE to relayed peers and to the relay server, under
    the same error code as before. Nothing in tree parses it.

Semver impact

  • breaking
  • feature
  • fix

Summary

A node that uses a relay holds two coupled things: a MASQUE tunnel to the relay
server, and a second QUIC endpoint that sends and receives through that tunnel.
When the node gave up a relay allocation it tore them down in the wrong order,
destroying the tunnel first and closing the endpoint second. Two things followed.

We logged a failure for something we did on purpose. Destroying the tunnel
drops the channel the endpoint reads from, so the endpoint driver's next
poll_recv returned BrokenPipe, EndpointDriver resolved to Err, and the
spawn wrapper logged ERROR I/O error: relay recv stream closed. Fleet-wide that
was 2,338 identical lines in 48 h across 73 hosts after 0.36.0 rolled out on
2026-08-12, about 100x the previous rate.

Peers using that relay were usually not told it was gone. Endpoint::close
only queues a ConnectionEvent::Close per connection. The frames still have to
travel out through the tunnel. When the driver died it cleared
connections.senders, so the connection driver handled the queued close, hit
channel EOF straight away, and exited before drive_transmit. The close frame was
built and never sent, and the peer waited out its own idle timeout. It is a
scheduling race rather than a certainty, so the claim is "not reliably told", not
"never told". The connection's local close reason was overwritten by the internal
transport error for the same reason.

That second one is why this is worth fixing. The log line is the symptom that
found it.

Three changes:

  1. Order. close(), then a bounded 1 s wait_idle(), then
    tunnel.shutdown(). A pure statement reorder. The close frames now leave
    through a tunnel that is still alive, and wait_idle measures a real drain
    instead of the empty sender map the dying driver had just cleared.

  2. Classification. RelayTunnelControl carries a TunnelCause of Live,
    Failed or ShutdownRequested in an AtomicU8 shared with the socket,
    settled by compare-exchange so the first cause wins. poll_recv parks instead
    of returning BrokenPipe when the teardown was requested, and still reports
    the error when the tunnel broke. The tunnel-death watcher stays quiet for a
    teardown we asked for.

    First-cause matters because cleanup routinely arrives after a tunnel has
    already broken. For the same reason the relay health monitor now has its own
    abort path that marks the tunnel failed before tearing it down. Without it,
    is_relay_healthy can condemn a relay from the state of the outer relay
    session while the tunnel's own cause is still Live, and the teardown would
    file that genuine failure as an intentional one.

    That verdict now also names the relay it was reached about. Checking health
    and then fetching the published handle are two separate awaits, so a
    replacement could publish in between and the monitor would tear down the
    healthy replacement on the strength of a verdict about its predecessor. That
    one is pre-existing, but marking the tunnel failed would have made it worse,
    so it is fixed here rather than inherited.

  3. Releasing what the crash used to release. The driver crashing was what
    freed a connection parked waiting for send capacity. It no longer crashes, so
    shutdown wakes send_capacity_freed itself and the poller consults the
    recorded writer state rather than the send channel, which only closes after the
    asynchronous abort lands. Whenever the poller answers "writable",
    enqueue_outbound must not answer WouldBlock, because Quinn retries that
    immediately without yielding. So a full queue whose writer has stopped drops
    the datagram.

Parking poll_recv does not strand the driver. Nothing can arrive on a torn-down
tunnel, and it keeps its other wakers: the endpoint-event channel, a connection's
Drained event, and the explicit wake EndpointRef::drop issues at refcount
zero. So it still retires with Ok(()).

The cause, the writer state and both Notifys live in one TunnelState held
strongly by the control, the socket and the tunnel tasks. That matters for the
dial-through path in p2p_endpoint, which drops its control as soon as the dial
completes while the socket and its tasks live on. A writer that recorded its exit
through a Weak to the control would be a no-op there and leave a parked poller
waiting forever.

What this does not claim

The fix silences only teardowns we requested. Of the 3,150 errors in the 48 h
window, 1,773 (56%) are matched by a completed "Proactive relay torn down". The
rest are unattributed, and some are genuine tunnel failures that this deliberately
keeps loud. ≥56% reduction is the defensible claim, not 100%.

The spike itself has already subsided. It collapsed at 2026-08-13 21:00 UTC when
every relay-lifecycle counter and the restart count fell together, most likely the
rolling upgrade finishing. This has not shipped, so it should not be credited with
that. What it fixes is a defect that recurs whenever relays churn.

CONNECTION_CLOSE delivery is improved, not guaranteed. Endpoint::close uses
try_send and drops the event if a connection mailbox is full, wait_idle waits
for the connection map to empty rather than for the MASQUE send queue to flush,
and 1 s need not cover Quinn's 3 × PTO drain.

Test evidence

  • cargo fmt --all -- --check: clean.
  • cargo clippy --all-targets --all-features -- -D warnings: clean.
  • cargo test --lib: 1,502 passed / 0 failed / 3 ignored.
  • Changed modules: masque:: 152 passed, nat_traversal_api:: 32 passed.

Six tests carry the change, and each was ablated to confirm it fails when the
part it guards is reverted:

Test Reverting this makes it fail
real_relay_teardown_closes_the_peer_and_releases_capacity the ordering, and independently the classification
proactive_relay_teardown_reports_no_transport_error the classification
unexpected_tunnel_loss_still_reports_a_transport_error scoping the quiet path to requested teardowns
shutdown_releases_a_poller_parked_on_a_full_send_queue the poller's writer-state check, and the wake itself
the_health_monitors_abort_is_recorded_as_a_failure mark_failed on the health-monitor abort
a_writer_exit_is_recorded_even_after_its_control_is_dropped sharing the tunnel state strongly rather than through a Weak

real_relay_teardown_closes_the_peer_and_releases_capacity is the one that closes
the network-facing claim. It uses a real relay service, a real CONNECT-UDP
session, the real run_stream_forwarding_loop data plane, the real
prepare_proactive_relay / abort_proactive_relay lifecycle, and a real peer
dialling the relay-allocated address. It asserts the peer observes
ApplicationClosed(RELAY_TUNNEL_LOST_CODE), that no transport error is logged,
and that the relay server gets its capacity slot back. With the ordering reverted
the peer is told nothing and it fails on a five-second timeout. With the
classification reverted it captures the fleet's exact line.

Mixed-version interop was exercised on an earlier revision of this branch by
running the three roles as separate processes linked against either unpatched
a68f1c8c or this head, across all six role combinations. Every patched-client
case delivered the close regardless of whether the relay server and peer were
patched, and every unpatched-client case timed out exactly as it does today. The
same harness over an address-translating lossy path delivered the close 20/20 on a
clean path and 15/20 at 30% loss, against 0/20 unpatched. That harness was a
one-off experiment and is not part of this PR.

Still open, and stated rather than glossed:

  • Dev testnet: not done. T2 requires it. Nothing has run outside cargo test.
  • NAT / canary on real links: not done. Real residential, mobile and CGNAT
    boxes, real conntrack expiry and bursty loss are not reachable in simulation.
  • Rollback: reasoned through, never rehearsed.
  • Observability: the signal this removes is replaced by info! lifecycle logs
    that are already in prod telemetry, but nothing is wired to watch them. After
    deploy, relay recv stream closed should fall to the residual and decouple from
    Proactive relay torn down, where today the two track 1:1. The relay prepared,
    torn down and canary-published rates should be unchanged.

New dependency

none. One lockfile-only bump rides along: h2 0.4.15 to 0.4.16 for
RUSTSEC-2026-0258, which landed in the RustSec database on 2026-08-19 and fails
cargo audit on main too, so it blocks every open PR until the lockfile moves.
Only the h2 stanza's version and checksum change, so no other dependency edge is
re-resolved, and cargo audit then exits clean with the three warnings already
allowed on main. Happy to split it into its own PR if preferred.

ADR

https://github.com/WithAutonomi/saorsa-transport/blob/fix/relay-teardown-transport-error/docs/adr/ADR-013-relay-tunnel-teardown-ordering.md

Mitigation / rollback

Revert and deploy through the normal rolling restart. No wire, stored format or
public API changes, so no coordinated downgrade or simultaneous fleet restart is
needed. Reverting restores the previous behaviour exactly, including the ERROR
line for requested teardowns and the lost CONNECTION_CLOSE.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

@github-actions

Copy link
Copy Markdown
Contributor

Benchmark Results

Performance Comparison

Benchmark Baseline Current Change Status

Summary

Configuration

  • Regression threshold: >10% slower
  • Improvement threshold: >10% faster
  • Measurements: Mean execution time

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

6 similar comments
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

@grumbach grumbach changed the title fix(relay): stop reporting requested tunnel teardown as a socket failure fix(relay): stop reporting requested tunnel teardown as a socket failure [not fleet-ready — see release gates] Aug 14, 2026
@grumbach
grumbach marked this pull request as draft August 14, 2026 06:14
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

7 similar comments
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

@grumbach grumbach changed the title fix(relay): stop reporting requested tunnel teardown as a socket failure [not fleet-ready — see release gates] fix(relay): drain the relay endpoint before dismantling its tunnel Aug 19, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Reminder: saorsa-transport is an independent project, not a fork of Quinn. We do not contribute changes back to quinn-rs/quinn.

A proactive relay allocation owns two coupled resources: a MASQUE tunnel
to the relay server, and a Quinn endpoint that uses that tunnel as its
socket. `ProactiveRelay::teardown` destroyed the tunnel while the
endpoint was still running on it, and two things followed.

The endpoint driver's next `poll_recv` returned `BrokenPipe`, so a
teardown we asked for was reported as a transport failure and logged at
ERROR. Fleet-wide this was 2,338 occurrences of `relay recv stream
closed` in 48 h across 73 hosts after the 0.36.0 rollout, about 100x the
previous rate.

Ending the driver that way also discarded the CONNECTION_CLOSE frames
`close()` had just queued. `EndpointDriver::drop` clears
`connections.senders`, so the connection driver handled the queued close,
hit channel EOF, and exited before `drive_transmit`. Peers reached
through the relay were told nothing and waited out their own idle
timeout. The connection's local close reason was overwritten by the
internal transport error for the same reason.

Teardown now closes the endpoint, drains it for up to one second, and
only then shuts the tunnel down, so the close frames leave through a
tunnel that is still alive and `wait_idle` measures a real drain rather
than the sender map the dying driver had just cleared.

A `TunnelState`, shared strongly by the control, the socket and the
tunnel tasks, records why the tunnel ended: `Live`, `Failed` or
`ShutdownRequested`, settled by compare-exchange so the first cause wins.
`poll_recv` parks for a teardown we requested and still reports
`BrokenPipe` for a tunnel that broke, and the tunnel-death watcher stays
quiet for the former. First cause matters because cleanup routinely
arrives after a tunnel has already broken; last writer wins would let it
silence the fault that triggered it. For the same reason the relay health
monitor gets its own abort path, which marks the tunnel failed before
tearing it down and names the exact allocation its verdict was about.

Because the driver no longer crashes, shutdown has to release what the
crash used to release. A `WriterExit` guard held by the writer future
records the writer's exit on every path that ends it, including an abort
that lands before the future's first poll, and wakes anything parked on
send capacity. Whenever the poller answers "writable", `enqueue_outbound`
must not answer `WouldBlock`, which Quinn retries immediately without
yielding, so a full queue whose writer has stopped drops the datagram.

Parking `poll_recv` does not strand the driver: nothing can arrive on a
torn-down tunnel, and it keeps its other wakers, so it still retires with
`Ok(())` once the endpoint is dropped.

Documented in ADR-013.
@grumbach
grumbach force-pushed the fix/relay-teardown-transport-error branch from f2d36c1 to e3f16d4 Compare August 19, 2026 07:30
@grumbach
grumbach marked this pull request as ready for review August 19, 2026 07:32
`cargo audit` fails on `h2` 0.4.15, which accepts unbounded empty HTTP/2
DATA frames. The advisory landed in the RustSec database on 2026-08-19
and fails on `main` as well, so it blocks every open PR in the repo until
the lockfile moves.

Lockfile only. No manifest or source file changes, and the h2 stanza's
version and checksum are the only lines touched, so no other dependency
edge is re-resolved. `h2` is reachable here through reqwest and hyper on
the UPnP path; the node's own protocol is QUIC.

`cargo audit` now exits clean with the three warnings already allowed on
`main`.

@dirvine dirvine left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

APPROVE — reviewed exact head f6d30aee00c4a926c293bebb6b86df539ca69198.

No material blockers found. Teardown now closes and drains the endpoint before shutting down the relay tunnel; tunnel-cause ownership, task aborts, and waiter wake-up paths remain bounded and cancellation-safe. CI is green. Local verification: formatting and all focused teardown tests passed, including real peer closure and capacity release.

This should land before #140 so that branch can inherit the h2 0.4.16 audit fix.

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.

2 participants