Fix flaky SessionConfigLive static-exchange tests (#62)

[Segfaultd]

Fixes #62.

Root cause — test defect, not a library bug

In static mode, RakPeer::SendSessionConfigResponse() releases the server's withheld ID_NEW_INCOMING_CONNECTION in the same step that it sends ID_SESSION_CONFIG to the client. The client's ID_CONNECTION_REQUEST_ACCEPTED is only produced once that message has crossed the wire and been parsed, so the server's connection packet is always enqueued first.

StaticExchangeDeliversBothPayloads waited on the client first with PumpUntil(), and that helper's alsoPump drain discards every packet the other peer has queued. Whenever the scheduler lands in the gap between the two enqueues, ID_NEW_INCOMING_CONNECTION is thrown away, and the following wait then burns its full 15s deadline — exactly the reported symptom (line 280, ~15.2s).

This is the hazard PumpUntilBoth() was added for and already documents; these two tests were simply never converted.

Evidence

Instrumented PumpUntil's discard path and ran the test under 8-way parallel load:

6 [probe] PumpUntil(want=16) DISCARDED id=19 from alsoPump

6 discards, 6 failures — want=16 is ID_CONNECTION_REQUEST_ACCEPTED, id=19 is ID_NEW_INCOMING_CONNECTION. One-to-one.

Fix

Both StaticExchangeDeliversBothPayloads and EmptyPayloadsStillConnect (same latent defect — the handshake runs with empty payloads too) now use PumpUntilBoth(). No library change; nothing was masked by a retry or a longer deadline.

The interactive-mode tests are unaffected: they wait on the server's ID_NEW_INCOMING_CONNECTION first, which is the safe order given the enqueue ordering above.

Verification (macOS arm64)

Check	Before	After
Target test, 8× parallel × 15 repeats	5 failures / 115 runs	240/240 pass (both tests)
SessionConfigLive.* --gtest_repeat=10	—	21 tests × 10, pass
Full ctest	—	276/276 pass

Summary by CodeRabbit

• Tests
  • Improved integration test reliability for session connections, including cases where payloads are present or empty. Tests now verify that both client and server connection events are received before checking payload delivery.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 7ba8b456-65af-4042-9256-1f95a6dd8235

📥 Commits

Reviewing files that changed from the base of the PR and between 8a99a6d and 3fd604c.

📒 Files selected for processing (1)

• Tests/Integration/SessionConfigLiveTests.cpp

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

Walkthrough

The two session configuration integration tests now wait for both peers’ connection notifications together. They retain both packets and GUIDs before checking payload delivery or length.

Changes

Session connection notification handling

Layer / File(s)	Summary
Collect both connection notifications Tests/Integration/SessionConfigLiveTests.cpp	Both tests use PumpUntilBoth to collect the client and server connection notifications. They deallocate both packets before checking payload results.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 3fd60

The changed tests collect both connection notifications before checking payloads. No merge-blocking issue was found in the inspected paths.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: fixing flaky static-exchange tests in SessionConfigLive.
Linked Issues check	✅ Passed	Issue #62 required reproducing the timeout, identifying the missing packet, deciding whether the defect was in the test or library, and fixing the defect at the correct layer. The PR reports packet in…
Out of Scope Changes check	✅ Passed	The reviewed change is limited to Tests/Integration/SessionConfigLiveTests.cpp. It updates the two static-mode tests and uses the existing PumpUntilBoth() test helper to preserve both handshake no…
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit waits as packets arrive,
Two little notices hop in alive.
Both GUIDs are gathered, then payloads checked,
The empty one passes, the other’s intact.
I nibble a carrot and bound away.}

_{Comment @coderabbitai help to get the list of available commands.}