Skip to content

Test/frozen reader flood#8

Merged
vjan-nie merged 9 commits into
Univers42:mainfrom
vjan-nie:test/frozen-reader-flood
Jul 12, 2026
Merged

Test/frozen reader flood#8
vjan-nie merged 9 commits into
Univers42:mainfrom
vjan-nie:test/frozen-reader-flood

Conversation

@vjan-nie

Copy link
Copy Markdown
Collaborator

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.cpp and Makefile as well.

What's in here

Commit Scope
tests: ignore SIGPIPE in test_runner to match ircserv's signal disposition harness (affects the whole suite)
tests: add build target that compiles without running the suite tests/Makefile
tests: prove frozen-reader isolation and survival during the flood, not after the three T6 tests
docs: record the harness SIGPIPE trap and the self-terminating flood pattern CLAUDE.md

Suite: 148/148 locally and in docker build --no-cache --target test.
scripts/audit.sh PASSED on all three tiers. scripts/normalize.sh --check clean.

The tests

  1. ThirdClientUnaffectedByFrozenReaderFlood — an unrelated client C keeps
    getting PONGs while A is frozen and B floods their shared channel.
  2. ServerSurvivesFloodAgainstFrozenReader — a brand-new client D can still
    complete registration (001) while the flood is in flight.
  3. FrozenReaderEventuallyDisconnectedOnSendQ — A is eventually disconnected
    once the backlog exceeds real OS + MAX_SENDQ backpressure.

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 a FLOOD_CAP safety net
that 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_PROBES guard: the vacuous-pass case it watched for is now
impossible by construction.

3. The test binary had a different signal disposition than the shipped one

tests/ does not link main.cpp, so the signal(SIGPIPE, SIG_IGN) that
ircserv installs in production never ran in test_runner. A server-side
send() to a socket a test had already close()d — while a large SendQ was
still 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 no std::thread can be skipped past its join() on an
early 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 starts
after 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
fdA is never read again, and a CLAUDE.md entry that documented the now-deleted
MIN_PROBES guard.

Follow-up, not in this PR

vendor/PostMan.hpp PM_MAX_ROWS = 256 silently discards results. Under
--gtest_repeat=3 over the full suite, 444 tests actually ran and passed, but the
report printed All 256 assertions passed — the remaining ~188 rows were dropped
with no warning (PostMan.cpp:154). The exit code is unaffected (it comes from
RUN_ALL_TESTS()), so CI is not lying, but the human-readable report is. This
undermines --gtest_repeat as a verification technique for this repo and deserves
its 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:~365 labelling SendQ disconnects as "Ping timeout" instead of
"SendQ exceeded".

@vjan-nie
vjan-nie merged commit 05d8a13 into Univers42:main Jul 12, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant