Skip to content

fix: IPC reactor delivered stale responses to recycled client slots and died of SIGPIPE - #25150

Open
charlielye wants to merge 1 commit into
nextfrom
cl/ipc-reactor-disconnect-cleanup
Open

fix: IPC reactor delivered stale responses to recycled client slots and died of SIGPIPE#25150
charlielye wants to merge 1 commit into
nextfrom
cl/ipc-reactor-disconnect-cleanup

Conversation

@charlielye

@charlielye charlielye commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

The bug

ipc::IpcServer::run_reactor() keeps per-connection response-ordering state (arrival-order sequence counter + reorder stash) keyed by the transport's client slot id. SocketServer recycles slot ids (find_free_slot() returns the lowest freed slot) and nothing cleared the reactor's state when a connection ended. A connection dying with requests still in flight — routine in production: bb-avm-sim processes are killed on cancellation and teardown while their wsdb requests are outstanding — leaves "zombie" sequence entries on its slot, with two timing-dependent failure modes:

  1. Response misdelivery. If a new connection is accepted onto the freed slot before the zombie completes, the new connection's first response is stashed behind the zombie's sequence number, and the zombie's late completion is released to the new connection as its first frame. TS clients correlate responses positionally (no request-id envelope — the reactor's FIFO release is the correctness contract), so a single leaked frame shifts every subsequent response onto the wrong caller: wrong-type decodes or right-type values for the wrong parameters.
  2. Whole-server SIGPIPE death. If the zombie completes while the slot's fd is closed but not yet recycled, the reactor writes the response to the dead peer. send() passed no MSG_NOSIGNAL and nothing ignored SIGPIPE, so the write killed the entire server process — taking down every other client, including ones that never disconnected. Deterministically reproducible: any client that pipelines reads over UDS to aztec-wsdb and destroys its connection mid-flight killed the server within milliseconds (aztec-wsdb exited unexpectedly (code=null, signal=SIGPIPE)).

The fix

The slot table was an SHM-ism that had leaked into the socket transport: MPSC-SHM ids are physical ring indices and must recycle, but nothing about UDS wants recycled ids — idiomatic socket servers tie connection state to an identity that dies with the connection. So rather than guarding the recycled-id hazard, the socket transport now removes it:

  • SocketServer client ids are monotonic and never reused (next_client_id_++; id-keyed maps replace the dense slot vector, find_free_slot() is gone). A connection's identity cannot be inherited by a later connection, so a late respond() targets an id that no longer exists anywhere.
  • IpcServer::drain_disconnected_clients() (new hook): transports report client ids whose connection ended; the reactor erases their reorder state. With never-reused ids this is garbage collection, not a correctness-ordering mechanism — and respond() now find()s instead of creating, so a completion for an erased connection is dropped rather than written to a dead fd. (The hook's doc notes that a transport with recycled ids — SHM — must not adopt it as-is; that's the known SHM follow-up.)
  • MSG_NOSIGNAL on SocketServer::send() (Linux) and SO_NOSIGPIPE on accepted fds (macOS), plus SIGPIPE → SIG_IGN in install_default_signal_handlers(): a write that races a disconnect yields EPIPE (already handled — the send loop disconnects the client), never a process-killing signal.
  • UdsIpcClient retries ECONNRESET on connect (TS): under connection churn a connect can race the server's accept loop and get reset; previously only ECONNREFUSED/ENOENT/ETIMEDOUT/EAGAIN were retried, so a transient reset surfaced as a hard connect failure.

Tests

C++ (ipc-runtime, socket.test.cpp):

  • ReactorDropsStaleResponsesAndNeverReusesIds: the failure-mode-1 choreography (a connection dies with a gated response in flight; a new connection arrives; the zombie completes) — asserts the new connection receives exactly its own frame and a fresh client id. The recycled-slot version of this scenario was RED 20/20 before the fix (first frame carried the dead connection's payload + an extra leaked frame); GREEN now, 10× repeats.
  • ReactorSequentialConnectionsAreIndependent: clean-handover control, also pinning the never-reused-id invariant.
  • ReactorSurvivesResponseToDeadClient: reactor drops/fails responses to a dead client without dying. Note in-process writes to a just-closed peer can be absorbed by kernel buffering, so this alone cannot prove SIGPIPE immunity — hence the cross-process test below.

TS (yarn-project/world-state) — run against the rebuilt aztec-wsdb:

  • wsdb_sigpipe_death.test.ts: cross-process guard for failure mode 2. A long-lived monitor connection must keep reading correct answers through 20 rounds of an unrelated peer pipelining ~400 reads and destroying its connection mid-flight. Deterministically RED against the unfixed binary (server dead in ~12 ms, monitor gets read ECONNRESET); GREEN with the fix.
  • ipc_churn_correlation.test.ts: correlation load test where every response must prove it belongs to its own request by value, not just by type. Each connection plants a private fork with leaves derived from its own seed — connections share no observable state, so a cross-connection swap of same-type responses (invisible to the existing shared-state tests, where identical requests have identical answers) fails on wrong index/root/size. Legs: C sequential sanity; A single-connection pipelined reads + per-fork writes (reorder-stash pressure, no disconnects); D multi-connection read/write soak with no churn — readers assert recorded roots and exact leaf indices, writers pipeline append→read-after-write with exact-index asserts; B = D's workload plus a rotating mid-flight destroy + replace (slot recycling). WSDB_SOAK_MS extends D/B for grinding sessions (default ~4s for CI; 30s soak run clean).

Leg D is deliberately independent of this PR's fixes: it passes against the unfixed binary too (while Leg B fails there in ~56 ms), so it discriminates the disconnect-cleanup bug class from any other IPC/reorder/scheduler defect — a failure in D on any binary is a distinct bug.

Full ipc_runtime_tests suite passes (19/19); existing ipc_pipelined_read_correlation.test.ts passes against the rebuilt binary.

Context and follow-ups

Found while investigating a flaky noir-contracts TXE failure (Expected size in TreeStateReference deserialization — a positionally-mispaired wsdb response). These fixes remove the only proven server-side sources of stale/mispaired frames; whether that flake's exact delivery path is fully explained is still under investigation. Known follow-ups, deliberately out of scope here:

  • The MPSC-SHM transport has the analogous slot-reuse gap (per-slot response rings are not reset when a client detaches and its slot is reclaimed). Slot claim there is client-driven, so it needs a generation/handshake mechanism rather than the socket transport's reactor-thread disconnect list.
  • TXE server output is discarded unless the process exits non-zero (dump_fail in start_txes), which is why the original flake left no server-side trace; teeing it to a persistent log would make the next occurrence diagnosable.
  • The TS clients' "response with no pending caller" path still only console.warns; upgrading it to fail loudly is a small separate change to the ipc-codegen templates.

@charlielye
charlielye force-pushed the cl/ipc-reactor-disconnect-cleanup branch 3 times, most recently from e871afb to 6951252 Compare August 11, 2026 12:17
@charlielye
charlielye force-pushed the cl/ipc-reactor-disconnect-cleanup branch from 6951252 to 31b85c2 Compare August 11, 2026 12:23
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