PumpUntilBoth reports which side timed out; fix 85 mislabelled message ids (#65)

[Segfaultd]

Fixes #65, and fixes a library bug that writing #65 exposed.

1. The requested fix

PumpUntilBoth returned a bare bool and nulled both out-params, so a timeout at any of its 9 call sites said only "something didn't arrive". #62 was diagnosable precisely because the sequential code it replaced named the side.

It now returns a ::testing::AssertionResult. Every call site already wraps it in ASSERT_TRUE, which prints the message — so no call site changed:

Actual: false (timed out after 1500 ms waiting for ID_CONNECTION_REQUEST_ACCEPTED (16) on the
first peer and ID_SESSION_CONFIG_ABANDONED (132) on the second: ID_CONNECTION_REQUEST_ACCEPTED (16)
arrived, ID_SESSION_CONFIG_ABANDONED (132) did not)

2. The library bug it exposed

Naming the ids required PacketLogger::BaseIDTOString, and that message came back with the wrong name — ID_RESERVED_8 for ID_SESSION_CONFIG_ABANDONED.

The table was missing ID_RAKVOICE_RELAY_DATA (id 48). It is positional, so every id from 48 to 132 reported the next id's name:

id	reported	should be
128	ID_SESSION_CONFIG	ID_SESSION_CONFIG_REQUEST
132	ID_RESERVED_8	ID_SESSION_CONFIG_ABANDONED
133	ID_USER_PACKET_ENUM	ID_RESERVED_9

85 ids mislabelled, the entire session-config range among them. A stray trailing ID_USER_PACKET_ENUM sentinel kept the array at the expected length, so no length check could catch it. Fix is one line.

Diagnostic-only severity — but it misleads exactly when someone is reading a handshake log to debug a handshake, which is how the #62 investigation was run. I fixed it here rather than filing it, because shipping #65's diagnostics on top of a table that prints wrong names would make the feature actively misleading.

3. Test

Tests/Unit/PacketLoggerIdNamesTests.cpp (7 cases, Unit per CLAUDE.md — pure logic, no networking). Pins the table's ends and both sides of the break. Pinning the last entry is the load-bearing case: a name dropped anywhere earlier pushes it off its own slot.

Verified by reverting the one-line fix — 3 of 7 cases fail, naming the shifted values. Worth noting the blanket sweep alone does not catch it, since shifted names are still well-formed ID_* strings; that's why the pinned assertions exist.

Test and integration helper are guarded on _RAKNET_SUPPORT_PacketLogger, so MAFIANET_MINIMAL + MAFIANET_BUILD_TESTS still builds (the linux-minimal job doesn't build tests, so CI wouldn't have caught a regression there).

Verification (macOS arm64)

Full ctest: 283/283 (276 + 7 new).

Unrelated flake, for the record

PingTests.PingStatisticsAndOccasionalPing failed once during this work. It is not from this branch — it asserts hard wall-clock thresholds on loopback ping (average ping 12 exceeded threshold 10) and binds fixed port 60000, both of which CLAUDE.md warns against. Measured: 40/40 pass idle on this branch, 7/12 under artificial CPU load; master behaves the same. Not touched here — happy to file it separately.

Summary by CodeRabbit

• Bug Fixes
  • Packet logs now display a readable name for voice relay data packets instead of an unrecognized identifier.
  • Session configuration test timeouts now indicate which expected packets were not received, making failures easier to understand.

[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: a6d521f6-bdb1-44d0-aced-744fcb3f33bc

📥 Commits

Reviewing files that changed from the base of the PR and between 6f7f149 and cb38779.

📒 Files selected for processing (3)

• Source/src/packet_logger.cpp
• Tests/Integration/SessionConfigLiveTests.cpp
• Tests/Unit/PacketLoggerIdNamesTests.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 packet name table now includes ID_RAKVOICE_RELAY_DATA, with unit tests covering built-in and unknown IDs. Integration tests format known IDs by name. PumpUntilBoth returns an assertion result that identifies missing packets on timeout and retains packet cleanup.

Changes

Packet ID names and timeout diagnostics

Layer / File(s)	Summary
Add and verify built-in packet ID names Source/src/packet_logger.cpp, Tests/Unit/PacketLoggerIdNamesTests.cpp	The name table now includes ID_RAKVOICE_RELAY_DATA. Unit tests check selected packet names, the built-in ID range, and unknown IDs.
Report missing packets in integration tests Tests/Integration/SessionConfigLiveTests.cpp	IdName formats known IDs by name when packet logging is enabled. PumpUntilBoth returns an assertion result with details about missing packets and deallocates and clears any received packets on timeout.

Priority: ⬇️ Low

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

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to cb387

The packet-name correction and timeout diagnostics appear ready to merge after normal checks.

🚥 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 identifies both main changes: improved PumpUntilBoth timeout reporting and correction of mislabelled packet IDs.
Linked Issues check	✅ Passed	Issue #65 requires timeout diagnostics that identify the missing side at all nine PumpUntilBoth call sites. SessionConfigLiveTests.cpp now returns ::testing::AssertionResult, records whether eac…
Out of Scope Changes check	✅ Passed	The PacketLogger::BaseIDTOString table entry and its unit tests are directly connected to issue #65. The new timeout diagnostics use this table to name packet IDs. The missing entry caused those dia…
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 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 checks the packet names,
And finds the relay ID in place.
Two packets hop toward their marks,
A timeout tells which one is late.
The burrow tests each name with care.}

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