Skip to content

GO-7556: recover from stale connections after sleep/wake - #802

Merged
requilence merged 7 commits into
mainfrom
go-7556-conn-recovery
Oct 2, 2026
Merged

requilence merged 7 commits into
mainfrom
go-7556-conn-recovery

Conversation

@requilence

@requilence requilence commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Supersedes #801. The pool part is redone: instead of per-peer generation stamps, Flush swaps in fresh caches. That is simpler, and it fixes a case #801 missed: a Get issued after Flush used to join a dial that started before the flush and wait it out on a dead connection.

After sleep/wake, clients kept reusing dead pre-sleep connections, so handshakes and RPCs hung for 10–30s or longer.

Changes

  • pool:
    • Flush (new on pool.Pool/Service) atomically publishes a fresh incoming/outgoing cache pair. The Flush that replaced a pair walks it once after the swap and reports Closed for its peers before returning; watchers skip peers already reported, so each instance is reported exactly once, also with concurrent flushes. The old pair is torn down in the background, with at most 256 peer closes running at a time.
    • Lookups retry until the result comes from the current pair. The outgoing loader never dials for a replaced pair and dials under the pair's ctx. Get dials outgoing only on a real incoming miss, and waits out a busy incoming entry (for example a GC TryClose) instead of dialing a duplicate.
    • AddPeer returns ErrClosed during shutdown and waits for a mid-close entry before retrying. Retries are bounded, and flush swaps don't count against the bound. Accept bounds AddPeer at 10s.
    • Fast path via the new ocache.Peeker (Peek → miss/busy/hit, WaitClosing): cached lookups and misses allocate nothing. ocache.OCache is unchanged. ocache.PrometheusCollectors lets both cache pairs share one set of metrics; names and counting are unchanged.
    • peer.Peer implementations don't need to be comparable.
    • Incompatible-version backoff survives Flush. Close waits for pending teardowns, with a time limit.
  • peer:
    • Sub-conn closes that used to block the caller (release after ctx cancel, failed handshake, gc) run in the background (closeAsync). Release and failed-handshake closes count toward the open limiter; gc closes don't. A waiting AcquireDrpcConn keeps one deadline across wake-ups.
    • Error change visible to callers: an RPC or stream (Invoke, NewStream, MsgSend/MsgRecv/CloseSend) cut short because its sub conn closed or the connection died now returns transport.ErrConnClosed (matches net.ErrClosed) instead of context.Canceled or drpc's "manager closed". A caller's own cancellation stays context.Canceled. Errors carrying a drpc code (server replies), application errors and a stream's io.EOF are never rewritten, so err == io.EOF still works.
  • transport: yamux Open honours ctx. A dead connection is reported as ErrConnClosed via the new transport.NewConnClosedError, which keeps the original cause reachable. yamux reads keep plain io.EOF. A yamux write timeout counts only once the session has closed, because on a live session a slow uplink can cause one.
  • peerservice: a dial cancelled by its ctx (for example by Flush) tries no further addresses and records no QUIC demotion outcome.
  • handshake:
    • New OutgoingProtoHandshakeWithCloser hands the conn to a closer instead of closing it inline. OutgoingProtoHandshake behaves as before.
    • HandshakeError now unwraps, so errors.Is matches its cause (for example io.EOF or ctx errors).

Performance

Hit and miss paths in net/pool/pool_bench_test.go allocate nothing; on main, outgoing Get did 4 allocations. On hit paths the geomean is about −40% vs main. yamux Open with a cancellable ctx costs about 3µs and 4 allocs more per open, once per pooled sub conn. Servers never call Flush, so for them this branch brings faster lookups, bounded parallel closes on shutdown, and closes moved off caller paths.

Test plan

  • go test -race ./..., with stress runs of the changed packages (-count=20, -cpu=1,4)
  • Deterministic tests for the Flush/lookup/AddPeer/Close, observer, demotion, limiter and error-mapping races, checked against mutants
  • anytype-heart and any-sync-node/coordinator/filenode/consensusnode build against this branch; heart's device/payments/peerstatus/recovery/space/filestorage tests pass
  • Windows sleep/wake integration together with GO-7556: recover connections after sleep/wake; membership never stuck anytype-heart#3304

Linear: GO-7556

After sleep/wake, clients kept reusing dead pre-sleep connections: new
streams hung in the handshake or RPCs waited for replies for 10-30s+,
and pool.Flush could leave pre-flush peers usable.

net/pool:
- Flush swaps in a fresh incoming/outgoing cache pair and closes the
  old pair in the background: peers in parallel, outgoing first, so
  pre-flush dials are cancelled even if an incoming teardown hangs.
- Lookups merge the pair ctx and retry until their result comes from
  the pair that is still current, so a Get after Flush never returns or
  waits out a pre-flush dial; ErrClosed surfaces only when the pool is
  closed. A non-blocking fast path (ocache.Peeker) keeps cached lookups
  allocation-free and faster than before.
- AddPeer retries on a flushed pair or a transient entry.
  Incompatible-version verdicts survive Flush. Flush/Close are
  serialized; Close waits, bounded, for pending teardowns. Prometheus
  collectors are registered once and shared across pairs.

net/peer:
- Lazy per-peer cleanup owner moves synchronous sub-conn closes
  (release, failed handshake, gc) off the caller path. In-flight closes
  still count toward the open limiter; the MultiConn is closed only on
  stall evidence. Conns doomed by gc are never reused.

net/transport:
- yamux Open honours ctx.
- QUIC and yamux stream Read/Write report a dead connection or session
  as ErrConnClosed (wrapped).
- Optional WriteTimeouter.

net/secureservice/handshake:
- OutgoingProtoHandshakeWithCloser: a cancelled handshake hands the conn
  to a closer exactly once; the legacy function keeps its contract.
- HandshakeError unwraps to the underlying error.
Each sub-conn close is bounded by the transport (yamux write/close
timeouts, non-blocking QUIC/iroh/webtransport close), and in-flight
closes count toward the peer's open limiter, so a backlog throttles new
opens instead of needing a stall detector that kills the whole
multiconn. Remove the escalation, transport.WriteTimeouter and the yamux
writeTimeout plumbing; keep lazy capped workers, the pending list and
inFlight. Log once when a single close runs past a minute.
@requilence
requilence requested a review from cheggaaa October 2, 2026 11:01
@github-actions

github-actions Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

New Coverage 63.2% of statements
Patch Coverage 93.7% of changed statements (569/607)

Coverage provided by https://github.com/seriousben/go-patch-cover-action

pool:
- AddPeer returns ErrClosed during shutdown, bounds retries, waits for a
  mid-close entry (ocache Peeker.WaitClosing) and never waits on a
  flushed pair.
- Get restarts from incoming after evicting a dead outgoing peer, so a
  live incoming connection is used instead of a second dial.
- Flush reports Closed for the old pair's peers before it returns;
  watchers skip peers Flush already reported (once per instance).
- Old caches close concurrently; Peek counts no metrics, hits counted once.

peer:
- Replace the cleanup owner with closeAsync + an in-flight counter.
- One throttling deadline per AcquireDrpcConn call (no waiter starvation).
- Release and failed-handshake closes count toward the open limiter;
  gc closes don't.
- OutgoingProtoHandshake closes synchronously again; only the closer
  variant hands the close off.

transport: quic and yamux share transport.NewConnClosedError.
- peerservice: a dial cancelled by its ctx (e.g. Flush) tries no more
  addresses and records no QUIC demotion outcome
- pool: Get probes incoming via Peek (never loads into the incoming
  cache); the redial loop stops when the pair is replaced or the pool
  shuts down
- pool: Flush swaps don't consume AddPeer retries; pick leaves eviction
  to the watcher; drop keepOnFlush
- pool: only the Flush that replaced a pair walks it, once, after the
  swap, so concurrent Flushes report each peer exactly once
- yamux: a write timeout is ErrConnClosed only once the session closed
@requilence

Copy link
Copy Markdown
Contributor Author

Thanks for the recheck. Addressed in b672339.

Should fix

  1. A dial cancelled by its ctx (for example by Flush) now stops at the current address and records no demotion outcome, so it never sets FallbackFailed. A real QUIC timeout followed by a successful yamux dial is still recorded. Covered by the new dialoutcome_test.go subtests: Flush mid-dial, plain cancel, and a later real fallback failure.
  2. AddPeer: swaps no longer consume attempts, and the bound is attempts > retries, so it is reachable. Tests cover 3 and 5 swaps, and a two-duplicate replacement storm that ends with ErrExists after 3 evictions.

Worth raising

  • yamux write timeout: kept unwrapped on a live session. In yamux v0.1.2, ErrConnectionWriteTimeout doesn't close the session. Its timer covers the wait on the shared send queue, so a slow but healthy uplink can trip it while other streams keep working (a test reproduces that). Mapping it to ErrConnClosed would mark healthy connections as dead; headsync for example matches net.ErrClosed. A session that is really stalled fails its keepalive, shuts down, and its streams already return ErrConnClosed. So it is wrapped only once the session is closed, and the comment says so.
  • Get ctx: the redial loop now stops when its pair is replaced or the pool shuts down. Checking the caller ctx there turned out to be unreachable, because AfterFunc cancels the merged ctx asynchronously. Before the fix, the test saw dials into the old pair.
  • Spin during Close: fixed by the same check, which returns ErrClosed.
  • pick: no longer discards; eviction is left to the watcher.
  • Incoming probe: Get uses Peek, so nothing loads into the incoming cache. waitClosing and the retries stay, because a mid-close or replaced entry still needs them.

Minor

  • closeCaches now blocks, keepOnFlush is gone, abandoned() moved into export_test.go, and the comments are updated.
  • Flush now walks the incoming cache once and the outgoing cache twice. Verdicts have to be read under swapMu before the fresh pair is published, while marking has to happen after the swap. That ordering also fixes a lost/duplicate Closed with concurrent Flushes: only the Flush that replaced a pair walks it.

go test -race ./... passes.

pool:
- Get dials outgoing only on a real incoming miss; the outgoing loader
  refuses to dial for a replaced pair and dials under the pair's ctx
- Peek reports miss/busy/hit; Get waits out a busy incoming entry (e.g.
  a GC TryClose that declines) instead of dialing a duplicate
- no comparability requirement on peer.Peer (value-level check, by-id
  fallback)
- per-peer close concurrency capped at 256
- invariants documented at the top of pool.go

peer:
- an RPC error the caller didn't cause becomes ErrConnClosed when the
  sub conn closed, was doomed, or the session died (drpc otherwise
  reports a dead yamux session as context.Canceled)
- a conn handed to a waiter is claimed under the peer lock
- OutgoingProtoHandshake shares the closer variant's body (sync close)

transport/peerservice:
- yamux reads keep plain io.EOF; Open and closed-session errors are
  ErrConnClosed with the original cause kept
- cancellation is recorded per dial attempt; Accept bounds AddPeer at 10s
peer:
- never treat an RPC error carrying a drpc code as a lost connection;
  the ErrConnClosed wrapper also exposes Code()
- streams from NewStream report a dead sub conn on MsgSend/MsgRecv/
  CloseSend as ErrConnClosed (caller cancellation stays Canceled)
- NewConnClosedError doesn't double-wrap; acceptLoop uses errors.Is

pool:
- Pick and getIfActive return not-found at once when both caches miss
  (0 allocs, as on main)
- non-comparable peers are held behind a pool-owned pointer, so each
  pooled instance has exact identity (no by-id fallback)
- Flush/Close before Init are no-ops
- connLost rewrites only cancellation, drpc closed errors and
  closed-network errors on a closed/doomed sub conn; io.EOF (exact),
  coded and application errors pass through, so `err == io.EOF` works
- fast() returns the peer, never the pool's pooledPeer holder
- TestPool_FlushStorm: flush count follows the work done, not a
  background ticker that -race could starve
@requilence
requilence merged commit 0508d87 into main Oct 2, 2026
4 checks passed
@requilence
requilence deleted the go-7556-conn-recovery branch October 2, 2026 22:02
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 2, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants