Test/frozen reader flood#8
Merged
Merged
Conversation
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
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.
tests: prove frozen-reader isolation under real backpressure (T6)
Summary
Adds three robustness tests covering the "frozen reader" scenario — a client that
stops reading its socket while a channel it belongs to is flooded by another
member — and fixes two harness defects uncovered while writing them.
The headline is not the three new tests. It is that the first version of them
passed without testing anything, and that finding out took measurement rather
than inspection. Details below, because they are the reason this branch touches
test_main.cppandMakefileas well.What's in here
tests: ignore SIGPIPE in test_runner to match ircserv's signal dispositiontests: add build target that compiles without running the suitetests/Makefiletests: prove frozen-reader isolation and survival during the flood, not afterdocs: record the harness SIGPIPE trap and the self-terminating flood patternCLAUDE.mdSuite: 148/148 locally and in
docker build --no-cache --target test.scripts/audit.shPASSED on all three tiers.scripts/normalize.sh --checkclean.The tests
ThirdClientUnaffectedByFrozenReaderFlood— an unrelated client C keepsgetting PONGs while A is frozen and B floods their shared channel.
ServerSurvivesFloodAgainstFrozenReader— a brand-new client D can stillcomplete registration (001) while the flood is in flight.
FrozenReaderEventuallyDisconnectedOnSendQ— A is eventually disconnectedonce the backlog exceeds real OS +
MAX_SENDQbackpressure.No test asserts an exact byte or line count. The SendQ-disconnect boundary is an
OS socket-buffer artifact (~1.88 MB measured here, not the 64 KiB
MAX_SENDQ), so anything keyed to it would be pinned to one machine.Problems found along the way
1. The tests passed vacuously (the whole point of the branch)
Tests 1 and 2 originally flooded 1000 lines (~57 KB). Measured: that drains in
~1 ms. By the time C was probed or D registered, the flood was long over —
both tests would have passed identically with no flood at all. They asserted
isolation after backpressure, not under it.
2. Raising the volume was not enough — it was still a race
The first fix raised the volume to 200 000 lines and moved the flood into a
std::thread. Green locally, 30/30 across repeats. Red in the CI container,where the same flood drained before D finished registering.
That is the real lesson of this branch: a fixed flood volume makes the overlap
probabilistic. It has to happen to outlast the probe on every machine the suite
will ever run on. Tuning the number upward would have bought green today and a
flaky test later.
Both tests now use a self-terminating flood: the flood thread runs until the
test tells it to stop (
stopFlood), bounded only by aFLOOD_CAPsafety netthat exists to keep the test finite if the condition never occurs. The overlap is
now structural — D cannot register outside an in-flight flood, and C cannot be
probed outside one — rather than a timing coincidence. Consequently Test 1 no
longer needs its
MIN_PROBESguard: the vacuous-pass case it watched for is nowimpossible by construction.
3. The test binary had a different signal disposition than the shipped one
tests/does not linkmain.cpp, so thesignal(SIGPIPE, SIG_IGN)thatircservinstalls in production never ran intest_runner. A server-sidesend()to a socket a test had alreadyclose()d — while a large SendQ wasstill pending for it — killed the whole process with SIGPIPE (exit 141).
No test had ever exposed this, because none left a large SendQ and then closed
abruptly. The suite had been silently exercising a process that behaves
differently from the delivered binary. Now installed in
tests/test_main.cpp.Verified it masks nothing: all 145 pre-existing tests behave identically.
Adversarial review
A separate review pass confirmed the three tests are deterministic (45/45 under
--gtest_repeat=15), that nostd::threadcan be skipped past itsjoin()on anearly return, and that Test 3 observes a genuine SendQ cut rather than a
disconnect assisted by the test draining A — its wait-loop
recv()only startsafter the server has already closed the socket, and drains the ~1.88 MB of OS
backlog in under a millisecond.
Two review findings are fixed in this PR: a comment in Test 3 that falsely claimed
fdAis never read again, and aCLAUDE.mdentry that documented the now-deletedMIN_PROBESguard.Follow-up, not in this PR
vendor/PostMan.hppPM_MAX_ROWS = 256silently discards results. Under--gtest_repeat=3over the full suite, 444 tests actually ran and passed, but thereport printed
All 256 assertions passed— the remaining ~188 rows were droppedwith no warning (
PostMan.cpp:154). The exit code is unaffected (it comes fromRUN_ALL_TESTS()), so CI is not lying, but the human-readable report is. Thisundermines
--gtest_repeatas a verification technique for this repo and deservesits own task; it lives in a submodule and is out of scope here.
Also still open: deferred teardown (T4), the valgrind harness (P1), and
Server.cpp:~365labelling SendQ disconnects as "Ping timeout" instead of"SendQ exceeded".