Repository navigation
Conversation
The Windows job already compiled MafiaNet's targets with /W4 but non-fatal. The 25 warnings in our own code are fixed: - five 64-bit Time -> 32-bit TimeMS assignments in peer.cpp made explicit (the value is compared with GetTimeMS(), so 32-bit is intended); - a local in ReliabilityLayer shadowing the splitPacketId member, renamed; - test-side message-id-to-char truncations made explicit, and two shadowed names renamed. C4251 (std::atomic members of exported classes have no DLL interface) is disabled for our targets: expected for a C++ DLL used with the same toolset. Warnings from fetched dependencies and samples are not ours and stay at their own levels.
Connection lifecycle, reliability, MTU negotiation, ping, offline messages,
disconnect reasons and the session-config suite tested protocol behaviour
only, so they now run as deterministic unit tests on SimulatedNetwork
instead of real UDP with wall-clock waits. Every assertion of the old
integration files is covered, and several are stronger:
- ping statistics are exact (fixed link delay) instead of a latency
threshold that flaked on loaded machines;
- session-config gating is checked after every 1 ms update instead of
sampled racily from another thread;
- negative checks ("never connected", "never delivered") cover the whole
scenario, not a window after a sleep;
- MTU negotiation is also tested under a path ceiling, ReliableOrdered
under loss and jitter, and connection limits on every tick.
The 12 integration files are deleted; 34 integration tests remain for what
needs real sockets or threads (socket layer and batching, binding, local
addresses, threaded dispatch, plugins over loopback, stress). The unit
suite (322 tests) runs in under a second.
SimulatedNetwork gains StartConnect (start an attempt without stepping).
The manual-update Shutdown flush sends nothing when the clock has not moved
since the last update; documented on SetManualUpdate (test mode only). The
TSan suppression for the deleted loopback SlotInspector is gone.
The new fuzz-debug preset builds the harnesses in Debug, so a RakAssert a remote can reach through crafted input aborts and is reported. Two did, in ReliabilityLayer, reachable by any connected peer against a Debug build: - "Couldn't read all the data": a packet claiming more data than its datagram carries; - "Hole count too high": a peer jumping its message numbering far ahead. Both inputs were already dropped in Release; now Debug drops them too. Three reproducers join the peer_datagram corpus. The libFuzzer targets now mirror the library's _DEBUG definition like the replay targets do; without it, a Debug harness stack-allocated a ReliabilityLayer with the wrong layout. The nightly fuzz workflow runs both presets.
…king it A connected peer sending ID_CONNECTION_ATTEMPT_FAILED (generated locally only) hit RakAssert(0) in Debug. In Release the receive loop zeroed its size and continued without freeing the message, leaking it and leaving the rest of that peer's queue for the next update. It is now freed and the loop moves on. Found by the peer_datagram fuzzer with asserts enabled; covered by a SimulatedPeer test and a corpus reproducer.
- PingTests.OccasionalPingIsObservableAsPingTraffic counted keep-alive pings too in Release: the keep-alive fires every half timeout, and the default timeout is 30 s in Debug but 10 s in Release. Pin the timeout. - Brace single-assertion if bodies: gtest macros expand to if/else, and GCC's -Wdangling-else (fatal under -Werror) flags them.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe pull request changes peer receive handling, adds simulated-network test coverage, removes several live integration test files, and updates compiler-warning and fuzzing CI configuration. ChangesPeer runtime handling
Simulated-network test coverage
Build and fuzz CI
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 45.59% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 136 functions across 24 files. (7 skipped… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title accurately summarizes the three main changes: MSVC /WX handling, removing assertions for malformed network input, and migrating 12 loopback suites to SimulatedNetwork. The wording is concise… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title accurately summarizes the three main changes: MSVC /WX handling, removing assertions for malformed network input, and migrating 12 loopback suites to SimulatedNetwork. The wording is concise and specific.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 45.59% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 136 functions across 24 files. (7 skipped: 7 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · 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.
I’m a rabbit with a test-suite grin,
I hop through packets, out and in.
Debug fuzz seeds now take the stage,
Simulated peers test every page.
I thump my feet: the builds are green!
And nibble carrots in between.
Comment @coderabbitai help to get the list of available commands.
This branch has not been deployed
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.
Follow-ups to #76. This covers three of the four open points; clang-format comes next as its own PR.
1. MSVC: zero
/W4warnings,/WXin CIThe Windows job already built with
/W4but treated warnings as non-fatal. The 25 warnings in MafiaNet's own code are fixed:Time/TimeMSassignments inpeer.cppmade explicit;ReliabilityLayermember, renamed;C4251 (std::atomic members of exported classes) is disabled for our targets only. Windows now builds with
MAFIANET_WARNINGS_AS_ERRORS, so every CI job except the oldest-toolchain Docker one treats warnings as errors.2. Asserts reachable from the network
A new
fuzz-debugpreset builds the harnesses in Debug, so aRakAssertreached through crafted input is reported as a crash. The nightly workflow now runs bothfuzzandfuzz-debug. Running it found three asserts that any connected peer could trigger against a Debug build. The first two inputs were already dropped in Release:The third was also a bug in Release: an internal-only message id (
ID_CONNECTION_ATTEMPT_FAILED) sent by a remote hitRakAssert(0)in Debug. In Release it leaked the message and stopped processing the rest of that peer's queue until the next update. Now it's freed and skipped.All three are now dropped without asserting. They're covered by corpus reproducers and a
SimulatedPeertest.The libFuzzer targets now define
_DEBUGthe same way the library does. Without that, a Debug harness put aReliabilityLayerwith the wrong layout on its stack, and ASan reported a stack overflow on every input.3. Loopback scenarios moved onto the simulated network
12 integration files that tested protocol behaviour only are now deterministic unit tests on
SimulatedNetwork:Every old assertion is covered, and several are stronger:
34 integration tests remain, for what genuinely needs real sockets or threads: the socket layer and batching, binding, local addresses, threaded dispatch, plugins over loopback, and stress. The unit suite has 323 tests and runs in under a second.
SimulatedNetworkgainsStartConnect.One test-mode quirk is documented on
SetManualUpdate. In manual-update mode,Shutdown()flushes with one update, and that update sends nothing if the clock hasn't moved since the previous one. A real clock always moves.Verification
-Werror: 357/357 each./WX: green on a workflow_dispatch run of this branch.Summary by CodeRabbit
Bug Fixes
Documentation
Tests