Repository navigation
No static-mode test catches a server that fails to withhold ID_NEW_INCOMING_CONNECTION #64
Description
Found while root-causing #62 (fixed in #63). Not a regression from that PR — the gap predates it. Filing separately because the fix there was test-only and deliberately did not widen scope.
What the mutation testing showed
To prove #63 had not weakened SessionConfigLive.StaticExchangeDeliversBothPayloads, I mutated the library and checked the test still failed. One mutant is not detected by that test, either before or after #63:
Mutant B — the server reports the connection instead of withholding it. In RakPeer::RunUpdateCycle's ID_NEW_INCOMING_CONNECTION branch (Source/src/peer.cpp), right after the packet bytes are stashed:
memcpy(remoteSystem->withheldConnectionPacketData, data, byteSize);
remoteSystem->withheldConnectionPacketLength = byteSize;
}
/* MUTANT B: report the connection now instead of withholding it */
ProduceWithheldConnectionPacket(remoteSystem, (MessageID)ID_NEW_INCOMING_CONNECTION);This breaks the documented load-bearing contract: ID_NEW_INCOMING_CONNECTION is supposed to mean "the client's payload is in hand". Under the mutant it surfaces before ID_SESSION_CONFIG_REQUEST has even arrived.
Results, macOS arm64:
| Test | vs Mutant B |
|---|---|
StaticExchangeDeliversBothPayloads (pre-#63) |
passes 9/10 — the 1 failure is the #62 flake (line 280, 15218 ms), not detection |
StaticExchangeDeliversBothPayloads (post-#63) |
passes 10/10 |
InteractiveAcceptGatesBothConnectionPackets |
passes |
InteractiveRejectFailsTheConnectionAttempt |
fails (3185 ms) |
ClientLeavingMidDecisionIsReportedAsAbandoned |
fails (1410 ms) |
Why this matters
- The property is covered at suite level, but only incidentally, by two tests whose stated purpose is rejection and client-departure behaviour. Nothing in their names or comments says they are the guard for withholding, so either could be rewritten in good faith and silently take the coverage with it.
InteractiveAcceptGatesBothConnectionPacketsis commented as "The load-bearing assertion" and does not catch it. Worth understanding why before trusting that comment — it asserts no connection is reported untilAcceptSession, which the mutant ought to violate.- The static path has no gating assertion at all.
StaticExchangeDeliversBothPayloadsonly asserts the payload is readable once the packet surfaces, which the mutant satisfies by the time the test looks.
Suggested work
- Add a static-mode assertion that the connection packet is genuinely withheld — e.g. the server must not report
ID_NEW_INCOMING_CONNECTIONwithin a window while the client'sID_SESSION_CONFIG_REQUESTis deliberately withheld or delayed. Per CLAUDE.md this is a candidate for the hermetic two-layer fake-socket harness rather than loopback timing, since it needs control over message arrival order. - Investigate why
InteractiveAcceptGatesBothConnectionPacketsdoes not detect the mutant, and fix or re-comment it. - Consider mutation-testing the rest of this suite; the exercise was cheap and found this in one pass.
Activity
Segfaultd commentedon Oct 8, 2026
Investigated the open question in this issue (why
InteractiveAcceptGatesBothConnectionPacketsdoes not catch mutant B). Answer, plus a scope correction.Cause:
PumpUntil()deallocates non-matching packets on the waited-on peer, not just onalsoPump. The test's first action waits forID_SESSION_CONFIG_REQUESTon the server, which drains and frees an earlyID_NEW_INCOMING_CONNECTIONbefore the negative assertion runs. Confirmed by probe:PumpUntil(want=128) DISCARDED id=19 from WANTED peer, test still passing. The test was structurally blind to the violation it exists to catch.Same test had a second defect: its two negative assertions were successive
SawWithin()probes, which swallow each other's evidence — the hazardCollectIds()'s comment already forbids.Both fixed in #66 (test-only). Against the mutant the test now fails 5/5 deterministically in ~630 ms instead of passing 10/10.
Scope correction to this issue: I audited the suite expecting the helper hazard to be widespread. It is not — exactly 2 at-risk negative assertions, both in this one test. Every other negative assertion already uses the collect-once pattern. Point 1 above ("covered only incidentally by two tests about rejection and client departure") is now resolved: the property is covered by a gating test whose stated purpose is gating.
What remains open here: the static-mode gating assertion. Static mode answers the request as soon as it arrives, so loopback has no window in which to observe withholding — it needs control over arrival order, i.e. the hermetic two-layer fake-socket harness per CLAUDE.md. That is the remaining work on this issue.
test(sessionconfig): stop waits from destroying evidence a later asse…
test(sessionconfig): cover static-mode withholding of the connection …
Segfaultd commentedon Oct 8, 2026
Done, in two parts.
#66 fixed the gating test that was structurally blind:
PumpUntildiscarded non-matching packets on the waited-on peer, so the wait forID_SESSION_CONFIG_REQUESTate the early connection packet before the negative assertion ran. That resolved points 1 and 2 of this issue — the property is now covered by a test whose stated purpose is gating, andInteractiveAcceptGatesBothConnectionPacketsdetects the violation (5/5, ~630 ms) instead of passing 10/10.#69 closed the remaining static-mode item. Worth recording that the gap was real: before writing anything I tested whether #66 had already covered it, since the withholding mechanism is shared. A mutant releasing the connection packet at transport-up in static mode only, leaving interactive gated, passed all 23 tests.
It is not observable through received packets — static mode answers the request the instant it arrives, so early release and correct code look identical to any poll. The test asserts the slot invariant (never reported while the remote's payload is missing) sampled off the server's own
RemoteSystemStruct, with a 48 KB client payload to widen the gated window. Fails 20/20 against the mutant with ~4.8M sampled violations; 30/30 and 32/32 under parallel load when clean.One correction to this issue's text: I suggested the hermetic two-layer fake-socket harness. That turned out not to apply —
ReliabilityLayerBlackHoleTestsdrivesReliabilityLayerobjects directly and never constructs aRakPeer, so it cannot reach the session handshake, andRakNetSocket2Allocator::AllocRNS2is a static factory with no injection seam. The slot-invariant approach gets deterministic coverage without that infrastructure.Also filed #68 for an unrelated pre-existing flake noticed along the way.