Skip to content

No static-mode test catches a server that fails to withhold ID_NEW_INCOMING_CONNECTION #64

Closed
Closed
No static-mode test catches a server that fails to withhold ID_NEW_INCOMING_CONNECTION#64

Description

@Segfaultd
Segfaultd
opened on Oct 8, 2026

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

  1. 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.
  2. InteractiveAcceptGatesBothConnectionPackets is commented as "The load-bearing assertion" and does not catch it. Worth understanding why before trusting that comment — it asserts no connection is reported until AcceptSession, which the mutant ought to violate.
  3. The static path has no gating assertion at all. StaticExchangeDeliversBothPayloads only 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_CONNECTION within a window while the client's ID_SESSION_CONFIG_REQUEST is 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 InteractiveAcceptGatesBothConnectionPackets does 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

  1. Segfaultd commented on Oct 8, 2026

    @Segfaultd
    Segfaultd
    on Oct 8, 2026
    MemberAuthor

    Investigated the open question in this issue (why InteractiveAcceptGatesBothConnectionPackets does not catch mutant B). Answer, plus a scope correction.

    Cause: PumpUntil() deallocates non-matching packets on the waited-on peer, not just on alsoPump. The test's first action waits for ID_SESSION_CONFIG_REQUEST on the server, which drains and frees an early ID_NEW_INCOMING_CONNECTION before 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 hazard CollectIds()'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.

  2. Segfaultd
    added 2 commits that reference this issue on Oct 8, 2026

    test(sessionconfig): stop waits from destroying evidence a later asse…

    6f7f149

    test(sessionconfig): cover static-mode withholding of the connection …

    7ba9841

  3. Segfaultd commented on Oct 8, 2026

    @Segfaultd
    Segfaultd
    on Oct 8, 2026
    MemberAuthor

    Done, in two parts.

    #66 fixed the gating test that was structurally blind: PumpUntil discarded non-matching packets on the waited-on peer, so the wait for ID_SESSION_CONFIG_REQUEST ate 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, and InteractiveAcceptGatesBothConnectionPackets detects 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 — ReliabilityLayerBlackHoleTests drives ReliabilityLayer objects directly and never constructs a RakPeer, so it cannot reach the session handshake, and RakNetSocket2Allocator::AllocRNS2 is 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.

  4. Segfaultd
    closed this as completedon Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions