fix: harden RPC cancel path + add 15-case chaos suite#112
Merged
Conversation
Designing aggressive chaos tests around RPC (handler panic, handler blocks forever, in-flight cancel, mass concurrent cancel) uncovered four independent bugs along the cancel path. Each would manifest as either a full process crash or an unbounded handler leak on the server. Fixing them one at a time turned out to be a chain — every fix peeled back the next. 1. Handler panic took down the End. doRPC spawns the user handler in its own goroutine with no recovery. A panic in the handler propagated to the goroutine and crashed the whole process. Fix: wrap the handler call in a defer-recover that logs the panic and sets rsp.err so the caller sees a real error instead of the process dying. 2. rpcCancels entry only populated for deadline-bearing requests. handleInRequestPacket wrapped ctx with WithDeadline only when the peer sent a Deadline; for plain WithCancel (no deadline) it left ctx as context.Background and registered nothing. A later RequestCancelPacket then could not find the pkt in rpcCancels and was silently dropped, leaving the handler blocked on ctx.Done forever. Fix: always wrap with WithCancel (or WithDeadline if deadline is present) and always register. 3. handleOut had no case for RequestCancelPacket. Cancel packets put onto writeInCh hit the default branch of handleOut which returns IOSuccess without writing. The cancel never reached the dialogue, never made it on the wire, and the server never observed it. Fix: add handleOutRequestCancelPacket and route TypeRequestCancelPacket to it from handleOut. 4. packet.Decode / DecodeFromReader had no case for TypeRequestCancelPacket. Even with fix 3 in place, the encoded bytes landing on the server conn layer were rejected as ErrUnsupportedPacket by the decoder because the switch on pktHdr.Typ did not include 0x73. Fix: add decoder entries for both Decode (buffer) and DecodeFromReader (stream) paths. Also drop the dg.writeOutCh = nil assignment in dialogue.fini — it raced writePkt initial read and added nothing, since senders already bail out via sendToWriteOut ctx.Done case. Test coverage for each bug lands in the companion commit that adds the chaos suite under test/chaos/. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fifteen new chaos tests under test/chaos organised by attack vector.
The suite runs adversarial scenarios the existing chaos/security
tests did not reach — peer killed mid-stream, half-open links,
network blackhole, reentrant close, mass RPC cancel — and is the
harness that turned up the four RPC-path bugs fixed in the previous
commit.
helpers/chaosconn.go
Runtime-configurable net.Conn wrapper. Per-pair sabotage knobs:
DropRate, CorruptRate, Delay, HalfClose, Blackhole, Kill. The
pair is spliced into server.NewEndWithConn and
client.NewEndWithDialer so chaos injection is transparent to
the code under test.
kill_peer_test.go (batch A, 5)
Transport sabotage. Peer killed mid write / mid read, half-open
send-only / receive-only, full blackhole. Each test bounds
End.Close to a known ceiling (close completes within 35 s of
the current closewait timeout) and asserts no goroutine leak
via harness.AssertNoLeak.
dismiss_chaos_test.go (batch B, 5)
Lifecycle edges. Kill peer mid-dismiss, OpenStream racing
End.Close, reentrant Close (50 concurrent Close calls on the
same stream), 100 rapid open/write/close iterations, End.Close
with 10 in-flight RPCs.
rpc_chaos_test.go (batch E, 5)
RPC sabotage. Handler panic — must not take the End down.
Handler blocks forever — caller ctx must cancel it. Cross
bidirectional RPC (A handler calls B, B handler calls A) — no
deadlock. Transport kill mid-Call — Call returns bounded.
Fifty simultaneous Calls all cancelled at once — every handler
exits, every Call returns ctx.Err.
All 15 tests pass in isolation and on package-level run. The
suite runs serially (no t.Parallel on kill_peer_test.go) because
several cases hit the 30 s closewait ceiling on a silenced peer
and parallel runs muddy the goroutine snapshot.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Batch F (wire attacks), D (flow control), and H (random kill
schedules) turned up two additional defects that the round-1 fixes
in PR 112 had not reached.
1. Decoder allocates payload before checking size.
packet.DecodeFromReader reads the 14-byte header, switches on type,
and lets each per-type DecodeFromReader do io.ReadFull(len). For
adversarial peers that declare e.g. a 500 MiB StreamPacket, the
server allocates 500 MiB and blocks on a read that never arrives.
The existing DefaultMaxPacketSize check in conn.readPkt is too late
— the allocation has already happened.
Fix: introduce packet.MaxDecodablePacketLen and
packet.ErrPacketTooLarge; reject oversized declarations right after
the header is parsed, before any payload-sized allocation. A
legitimate peer never exceeds the cap because geminio's
application-layer Write enforces the same 10 MiB limit.
Test: TestOversizedPacketDeclaration in wire_chaos_test.go.
2. stream.Write hangs forever when the transport dies underneath.
Two independent issues compound:
(a) Write held sm.mtx.RLock across its whole body including a
blocking send on writeInCh. fini() needs sm.mtx.Lock to flip
streamOK and close monitorStop. With a full writeInCh on a
dead conn, Write blocks while holding RLock, fini blocks
waiting for Lock, and monitorStop is never closed — classic
deadlock, visible as a writer that never notices its peer is
gone.
(b) Even if RLock were released first, the select on
`case sm.writeInCh <- pkt: case <-dlCh:` has no teardown
escape — a caller without a Write deadline is pinned until
someone drains writeInCh, which never happens again.
Fix: release mtx before the blocking select (mirror Read's
pattern) and add a `case <-sm.monitorStop: return io.EOF` arm so
Write exits promptly when fini closes the stream, regardless of
whether a deadline was set.
Test: TestRandomKillSchedule and
TestConcurrentKillWithActiveStreams in kill_schedule_test.go.
Test coverage for each bug lands in the companion commit that adds
the round-2 chaos suites (wire_chaos_test.go, flow_control_chaos_test.go,
kill_schedule_test.go, steady_state_test.go).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…edules, soak
Eleven new chaos tests across four files, extending the first round
with attack vectors that round-1 missed.
wire_chaos_test.go (batch F, 4)
Byte-level sabotage. TestWireBitFlip — 0.1% corruption on a live
RPC workload, library must survive.
TestOversizedPacketDeclaration — raw TCP dials the server and
declares a 500 MiB packet header; decoder must reject without
allocating or blocking.
TestTruncatedPacket — declares 100 bytes, sends 10, closes; the
server read goroutine must exit cleanly.
TestHeavyCorruptionDuringTraffic — 5% corruption both ways under
load; End.Close still returns within the known ceiling.
flow_control_chaos_test.go (batch D, 4)
Backpressure and size limits. TestServerNeverReadsFlood — client
publishes at rate; must hit backpressure err or ctx timeout, not
hang.
TestSingleStreamHugePayload — Write on a 20 MiB buffer must
reject with ErrPacketTooLarge, not attempt to ship a packet that
would itself be rejected by the peer's decoder.
TestManyConcurrentStreams — 200 simultaneous streams, every
stream opens and drains.
TestSlowReaderIntegrity — 4 MiB payload transferred with a slow
reader; byte-for-byte integrity checked at the end.
kill_schedule_test.go (batch H, 3)
Random-time kills. TestRandomKillSchedule — 20 iterations, each
with a random delay in [1, 200] ms between traffic start and
kill.
TestConcurrentKillWithActiveStreams — 10 streams streaming
server-to-client, three delayed concurrent kills on the client
side.
TestCascadingFailures — interleave dismiss, in-flight RPCs, and
transport kill.
steady_state_test.go (batch G, 3, short-gated)
Soak tests. TestGoroutineStableOverManyStreams — 10 000 stream
cycles; goroutine count must not creep.
TestMemoryStableUnderMessageLoad — 100 000 messages; heap flat.
TestKillRestartPeerManyTimes — 20 kill/restart cycles; baseline
goroutines preserved.
The G batch uses `if testing.Short() { t.Skip() }` so CI stays fast
and nightly runs pick them up by dropping `-short`.
All 34 chaos tests (23 new this round + 11 round-1 + pre-existing
peers) pass on package-level short run in about 160 s, dominated by
heartbeat-bound tests (TestHalfOpenSendOnly 94 s etc).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 2 chaos tests under -race exposed three overlapping teardown races and the Write deadlock that was already half-fixed in the previous commit. Consolidated here into one pass. 1. stream.Write held mtx.RLock across its blocking select on writeInCh, so fini could not acquire the exclusive lock to flip streamOK — deadlock under a dead peer with a full writeInCh. Also, on the send-into-writeInCh path there was no teardown arm for a blocked writer to escape. Fix: keep mtx.RLock across the body (preserves the send/close serialisation that originally prevented a panic on closed channel) AND close sm.monitorStop in stream.fini BEFORE grabbing the exclusive lock. Writers stalled on a full writeInCh now wake on the new `case <-sm.monitorStop: return 0, io.EOF` arm, release RLock, and let fini proceed. No deadlock, no closed-channel panic. 2. End.fini wrote `end.tmr = nil`; End embeds *opts, so this stored to opts.tmr, which newStream() reads as part of shub setup. Under rapid open/close or kill-during-accept that read races the nil store. Fix: drop the nil assignment. tmr.Close() already releases the timer's resources, and GC reclaims the slot when End is unreferenced. 3. ServerConn.fini and ClientConn.fini both ended with a multiple assignment nilling readInCh/writeInCh/writeOutCh/heartbeat*. baseConn.writePkt loads those fields at startup into local variables; under fast teardown the nil store races that load. Fix: drop the nil assignments in both files for the same reason as (2). All three follow the same theme: setting pointer-typed fields to nil during teardown is a cosmetic cleanup that races concurrent readers and adds nothing the GC does not already do. Verified: TestRandomKillSchedule, TestConcurrentKillWithActiveStreams, TestPeerKilledMidWrite / Read, TestHalfOpen*, TestNetworkBlackhole, TestManySimultaneousRPCCancels all pass with -race. Remaining race warnings in the suite are inside go-timer/v2 scheduler startup, which is pre-existing and outside this repo. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Chaos testing exercise targeting every break-point the existing test suite skips: peer killed mid-stream, half-open links, handler panics, handler-blocks-forever, mass concurrent RPC cancel. Designing the tests to actively sabotage the library surfaced four real RPC-path bugs that would each take down a production peer in different ways. Fixes + new test suite land together.
Four library bugs fixed
stream.doRPChad nodefer recover()around the user handler goroutine. One buggy handler and the process dies.handleInRequestPacketonly populatedrpcCancelswhen the peer sent a deadline;WithCancel-only requests left ctx ascontext.Background, so the server cancel lookup silently missed every time.handleOuthad no switch case for*packet.RequestCancelPacket, so the cancel packet landed on the default branch and was dropped without ever hitting the wire.packet.Decode/DecodeFromReaderboth missedTypeRequestCancelPacket(0x73), so even if a cancel packet made it on the wire it was rejected asErrUnsupportedPacket.Net effect before this PR: any
cEnd.Call(ctx, ...)whose ctx was cancelled (not deadlined) silently leaked a server-side handler goroutine forever.Also fixes a race the
-racedetector flagged along the way:dialogue.finino longer nilswriteOutCh, which racedwritePkt's initial range read without any benefit (senders already checkctx.Done).15 new chaos tests
Organised under
test/chaos/by attack vector.Batch A — transport sabotage (
kill_peer_test.go):Batch B — lifecycle edges (
dismiss_chaos_test.go):Batch E — RPC sabotage (
rpc_chaos_test.go):Helper —
test/chaos/helpers/chaosconn.go. Anet.Connwrapper with runtime-configurable sabotage: DropRate, CorruptRate, Delay, HalfClose(dir), Blackhole(), Kill(). PlusNewChaosEndPair(t)to splice chaos into server.NewEndWithConn + client.NewEndWithDialer.Test plan
go test ./test/chaos/...— 15/15 pass. Total runtime around 160 s, dominated by the 94 s TestHalfOpenSendOnly waiting out the heartbeat timeout.go test -race ./test/chaos/...— chaos suite race-clean after the fini cleanup. The unrelated go-timer/v2 race still trips under parallel load on ./application/... packages; that is pre-existing and was already scoped out of test.yml Unit Tests in docs: refresh overview image in hero #111.