From c6461d8de1633eb8f78b03a0934cfbadfe181639 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 27 Jul 2026 16:36:07 -0500 Subject: [PATCH] fix(harness): bound the no-loss excusal by run size, not connection count The no-loss reconcile excuses an unconfirmed send (in flight at a connection close, no ACK seen) because `sent` is counted at write-buffer time -- the frame may never have left the socket. That excusal is capped so `timeouts == sent` cannot degrade the intake bound to `read >= 0` and let a total ACK-path regression pass as zero-loss. The cap modelled legitimate stranding as "~one in-flight frame per connection". That model is wrong for this sender: `_inflight` is an UNBOUNDED deque and an open-loop phase paces sends by the offered rate, not by an ACK slot, so what is genuinely in flight at a teardown is ~rate x ACK-latency -- a share of the run, unrelated to the connection count. It false-failed a demonstrably zero-loss run: 14 of 90 sends stranded (~16%) against a budget of 4, while the engine read every send that was not stranded and delivered all of them (engine_written == sink_received == 168, backlog 0). It reproduced identically on main, so it reds the REQUIRED windows-2025 leg and blocks auto-merge on unrelated PRs. Cap at max(connections, half the run), in all three copies of the accounting (load/report.py, connscale, estate -- deliberately kept in step). That keeps `read >= sent // 2` ALWAYS required, so a dead ACK path still fails loudly while ordinary teardown weather does not. ~3x the worst stranding observed. Test changes worth reviewing, because one verdict is deliberately flipped: the connscale flood scenario (6 of 36) is ordinary stranding under the new bound and would have passed for the wrong reason, so it is re-scenario'd to a genuine flood (30 of 36); a new test pins that the 6-of-36 shape now reconciles CLEAN. Two further tests pin that the excusal stops at exactly half the run and that loss beyond a large excusal still fails -- the widened bound must not blunt real detection. Mutation-verified: restoring the old bound fails all three new tests. --- harness/load/connscale/runner.py | 19 ++++---- harness/load/estate/runner.py | 11 +++-- harness/load/report.py | 26 +++++++---- tests/test_harness_reconcile.py | 77 ++++++++++++++++++++++++++++---- 4 files changed, 104 insertions(+), 29 deletions(-) diff --git a/harness/load/connscale/runner.py b/harness/load/connscale/runner.py index b25405d..a0b1cb1 100644 --- a/harness/load/connscale/runner.py +++ b/harness/load/connscale/runner.py @@ -815,14 +815,17 @@ def _reconcile( # unconfirmed sends as unconfirmed while ANY FURTHER shortfall is a real, confirmed-then-lost # message and still fails. With timeouts == 0 (every healthy run) it is exactly read >= sent. # - # BUT the excusal is BOUNDED by `unconfirmed_budget` (the caller's connection count — at most ~one - # stranded in-flight frame per connection is a plausible teardown artifact). Past the budget the - # timeout count is a SYSTEMIC no-ACK fault (mass resets, or the engine accepting frames and never - # ACKing — possibly accepted-and-dropped, the exact class the count-and-log invariant forbids), so - # NOTHING is excused and the reconcile fails loudly. Without the cap, `timeouts == sent` would - # degrade the intake bound to `read >= 0` and a total ACK-path regression would pass zero_loss. + # BUT the excusal must never go VACUOUS: with `timeouts == sent` an unbounded excusal degrades the + # intake bound to `read >= 0` and a total ACK-path regression would pass zero_loss. That cap used + # to be `unconfirmed_budget` alone, modelled as "~one stranded in-flight frame per connection" — + # a model this sender breaks (`_inflight` is an UNBOUNDED deque; open-loop sends are paced by the + # offered rate, not an ACK slot), so genuine teardown stranding scales with rate x ACK-latency, + # not the connection count. Bound it as a FRACTION instead — at most half the run, floored by the + # connection count — which keeps `read >= sent // 2` ALWAYS required. See harness/load/report.py's + # copy for the full rationale; the three copies are kept in step deliberately. unconfirmed = c.timeouts - over_budget = unconfirmed > unconfirmed_budget + budget = max(unconfirmed_budget, sent // 2) + over_budget = unconfirmed > budget excused = 0 if over_budget else unconfirmed read_short = sent - excused - read deliver_short = written - sink_received @@ -842,7 +845,7 @@ def _reconcile( if over_budget: parts.append( f"{unconfirmed} unconfirmed sends exceed the stranding budget " - f"({unconfirmed_budget} ≈ one in-flight per connection) — systemic no-ACK fault " + f"({budget} = max(connections, half the run)) — systemic no-ACK fault " f"(possible accepted-and-dropped); nothing excused" ) elif unconfirmed > 0 and read < sent: diff --git a/harness/load/estate/runner.py b/harness/load/estate/runner.py index 8e63007..c1ae832 100644 --- a/harness/load/estate/runner.py +++ b/harness/load/estate/runner.py @@ -428,10 +428,13 @@ def _reconcile( read = final.read - base.read written = final.written - base.written backlog = final.backlog - # An unconfirmed send (in-flight at a connection close, no ACK seen) is bounded-excused (at most ~one - # per connection); past the budget it is a systemic no-ACK fault. Mirrors the connscale reconcile. + # An unconfirmed send (in-flight at a connection close, no ACK seen) is bounded-excused; past the + # budget it is a systemic no-ACK fault. The bound is a FRACTION of the run (at most half), floored + # by the connection count — NOT "~one per connection", a model this sender breaks because + # `_inflight` is an unbounded deque. Mirrors the connscale/report reconciles; see report.py. unconfirmed = c.timeouts - over_budget = unconfirmed > unconfirmed_budget + budget = max(unconfirmed_budget, sent // 2) + over_budget = unconfirmed > budget excused = 0 if over_budget else unconfirmed read_short = sent - excused - read deliver_short = written - sink_received @@ -451,7 +454,7 @@ def _reconcile( if over_budget: parts.append( f"{unconfirmed} unconfirmed sends exceed the stranding budget " - f"({unconfirmed_budget} ≈ one in-flight per connection) — systemic no-ACK fault" + f"({budget} = max(connections, half the run)) — systemic no-ACK fault" ) detail = "; ".join(parts) if parts else "read>=sent, sink_received>=written, backlog drained" return NoLoss(ok, sent, read, written, sink_received, backlog, detail) diff --git a/harness/load/report.py b/harness/load/report.py index 00cee21..32e5fca 100644 --- a/harness/load/report.py +++ b/harness/load/report.py @@ -574,14 +574,24 @@ def _reconcile( # ANY FURTHER shortfall is a real, confirmed-then-lost message and still fails. With timeouts == 0 # (every healthy run) this is exactly as strict as read >= sent. # - # BUT the excusal is BOUNDED by `unconfirmed_budget` (the run's connection count — at most ~one - # stranded in-flight frame per connection is a plausible teardown artifact). Past the budget the - # timeout count is a SYSTEMIC no-ACK fault (mass resets, or the engine accepting frames and never - # ACKing — possibly accepted-and-dropped, the exact class the count-and-log invariant forbids), so - # NOTHING is excused and the reconcile fails loudly. Without the cap, `timeouts == sent` would - # degrade the intake bound to `read >= 0` and a total ACK-path regression would pass zero_loss. + # BUT the excusal must never go VACUOUS: with `timeouts == sent` an unbounded excusal degrades the + # intake bound to `read >= 0`, and a total ACK-path regression would pass as zero-loss. + # + # That cap used to be `unconfirmed_budget` alone, modelled as "~one stranded in-flight frame per + # connection". That model does not hold for THIS sender: `MllpConnection._inflight` is an UNBOUNDED + # deque — an open-loop phase paces sends by the offered rate, not by an ACK slot — so what is + # genuinely in flight at a teardown is ~rate x ACK-latency, which is a share of the run and has + # nothing to do with the connection count. On a contended runner that mismodelling false-failed a + # zero-loss run (14 stranded of 90 against a budget of 4) and red the required windows-2025 leg. + # + # So bound it as a FRACTION of the run, floored by the caller's connection count for tiny runs: + # at most half the sends may be excused, which keeps `read >= sent // 2` ALWAYS required. A dead + # ACK path (`timeouts == sent`) still blows the cap and fails loudly, and a run that genuinely + # loses more than half its sends at intake is still caught. Observed teardown stranding is ~16%, + # so this is ~3x the worst seen — wide enough to stop flaking, far from vacuous. unconfirmed = counters.timeouts - over_budget = unconfirmed > unconfirmed_budget + budget = max(unconfirmed_budget, sent // 2) + over_budget = unconfirmed > budget excused = 0 if over_budget else unconfirmed read_short = sent - excused - read deliver_short = written - sink_received @@ -603,7 +613,7 @@ def _reconcile( if over_budget: parts.append( f"{unconfirmed} unconfirmed sends exceed the stranding budget " - f"({unconfirmed_budget} ≈ one in-flight per connection) — systemic no-ACK fault " + f"({budget} = max(connections, half the run)) — systemic no-ACK fault " f"(possible accepted-and-dropped); nothing excused" ) elif unconfirmed > 0 and read < sent: diff --git a/tests/test_harness_reconcile.py b/tests/test_harness_reconcile.py index 5fd3b4a..ed339f2 100644 --- a/tests/test_harness_reconcile.py +++ b/tests/test_harness_reconcile.py @@ -5,9 +5,17 @@ The reconcile invariant under test (both the load runner's and the connscale runner's copies): a ``timeouts``-counted message (in-flight at a connection close with no ACK seen) is UNCONFIRMED — the frame may never have left the closed socket — so ``read >= sent - timeouts`` is the honest intake -bound, BUT the excusal is bounded by ``unconfirmed_budget`` (~one stranded in-flight per connection): -past it the timeout count is a systemic no-ACK fault and NOTHING is excused. With ``timeouts == 0`` -(every healthy run) the check is exactly as strict as ``read >= sent``. +bound, BUT the excusal is BOUNDED: past the bound the timeout count is a systemic no-ACK fault and +NOTHING is excused. With ``timeouts == 0`` (every healthy run) the check is exactly as strict as +``read >= sent``. + +The bound is ``max(unconfirmed_budget, sent // 2)``. It was ``unconfirmed_budget`` alone — "~one +stranded in-flight frame per connection" — until that model was found to be wrong for this sender: +``_inflight`` is an unbounded deque and open-loop sends are paced by the offered rate, not by an ACK +slot, so genuine teardown stranding scales with rate x ACK-latency rather than the connection count. +It false-failed a zero-loss run (14 stranded of 90 against a budget of 4) and red the required +windows-2025 leg. Capping at half the run keeps ``read >= sent // 2`` always required, so a dead ACK +path still fails loudly while ordinary teardown weather does not. These tests pin every edge the tolerance could silently widen through (the harness has caught real store bugs with this check — mf-load-test-harness — and that detection must survive the de-flake): @@ -93,17 +101,28 @@ def test_connscale_reconcile_timeout_flood_fails_even_without_shortfall() -> Non # A systemic no-ACK fault: timeouts past the budget fail EVEN when every frame demonstrably # arrived (read == sent) — an engine that ingests but never ACKs is broken, and excusing an # unbounded count would let `timeouts == sent` degrade the intake bound to `read >= 0`. - c = Counters(sent=36, acked=30, timeouts=6, sink_received=36) + # + # NB the flood must now be a REAL one. The budget is max(connections, half the run), so the old + # 6-of-36 scenario is ordinary teardown stranding under the current bound, not a fault — it would + # pass here for the wrong reason. 30 of 36 unconfirmed is unambiguously a dead ACK path. + c = Counters(sent=36, acked=6, timeouts=30, sink_received=36) result = connscale_reconcile(c, _BASE, _sample(read=36, written=36), unconfirmed_budget=_BUDGET) assert not result.ok assert "stranding budget" in result.detail # And with the flood masking a real shortfall, nothing is excused: the loss is reported too. - c2 = Counters(sent=36, acked=30, timeouts=6, sink_received=30) - result2 = connscale_reconcile( - c2, _BASE, _sample(read=30, written=30), unconfirmed_budget=_BUDGET - ) + c2 = Counters(sent=36, acked=6, timeouts=30, sink_received=6) + result2 = connscale_reconcile(c2, _BASE, _sample(read=6, written=6), unconfirmed_budget=_BUDGET) assert not result2.ok - assert "lost 6 on intake" in result2.detail + assert "lost 30 on intake" in result2.detail + + +def test_connscale_reconcile_teardown_stranding_is_not_a_flood() -> None: + # The other side of the same boundary: ordinary teardown stranding (6 of 36, ~17% — the shape the + # old ~one-per-connection cap false-failed) reconciles clean when the engine read everything that + # was not stranded. This is the case the previous test used to assert FAILED. + c = Counters(sent=36, acked=30, timeouts=6, sink_received=30) + result = connscale_reconcile(c, _BASE, _sample(read=30, written=30), unconfirmed_budget=_BUDGET) + assert result.ok, result.detail def test_connscale_reconcile_tolerance_is_intake_only() -> None: @@ -171,6 +190,46 @@ def test_load_reconcile_timeout_flood_fails_even_without_shortfall() -> None: assert "stranding budget" in result.detail +def test_load_reconcile_ci_teardown_stranding_is_not_a_flood() -> None: + # Regression for the red windows-2025 leg (2026-07-27): a demonstrably zero-loss run whose + # teardown left 14 of 90 sends unconfirmed (~16%) against the old connection-count budget of 4. + # The engine read every send that was not stranded (84 == 90 - 6 never-left-the-socket) and + # delivered all of them twice (fan-out 2), so this MUST reconcile clean. + c = Counters(sent=90, acked=76, timeouts=14, sink_received=168) + result = load_reconcile( + c, _poller(_sample(read=84, written=168)), 1.0, tolerance=0, unconfirmed_budget=4 + ) + assert result.ok, result.detail + + +def test_load_reconcile_excusal_is_capped_at_half_the_run() -> None: + # Mutation pin on the NEW bound. Half the run is the most the reconcile will ever forgive, so + # `read >= sent // 2` is always required and the intake bound can never go vacuous. Exactly at + # the cap passes; one over flips to a systemic fault with nothing excused. + at_cap = Counters(sent=90, acked=45, timeouts=45, sink_received=90) + assert load_reconcile( + at_cap, _poller(_sample(read=45, written=90)), 1.0, tolerance=0, unconfirmed_budget=4 + ).ok + over = Counters(sent=90, acked=44, timeouts=46, sink_received=88) + result = load_reconcile( + over, _poller(_sample(read=44, written=88)), 1.0, tolerance=0, unconfirmed_budget=4 + ) + assert not result.ok + assert "stranding budget" in result.detail + + +def test_load_reconcile_loss_beyond_a_large_excusal_still_fails() -> None: + # The widened bound must not blunt real detection: with 14 legitimately excused, the 15th absent + # message is still confirmed-then-lost and still fails. This is the assertion that would break if + # the cap were ever raised to "excuse everything". + c = Counters(sent=90, acked=76, timeouts=14, sink_received=150) + result = load_reconcile( + c, _poller(_sample(read=75, written=150)), 1.0, tolerance=0, unconfirmed_budget=4 + ) + assert not result.ok + assert "lost 1 on intake" in result.detail + + def test_load_reconcile_tolerance_is_intake_only() -> None: # Mutation pin (load copy): delivery and backlog shortfalls EXACTLY EQUAL to the timeout count # must fail — this copy had zero delivery-shortfall coverage anywhere in the suite before this