Skip to content

fix(harness): bound the no-loss excusal by run size, not connection count - #17

Merged
wshallwshall merged 1 commit into
mainfrom
noloss-budget
Jul 27, 2026
Merged

fix(harness): bound the no-loss excusal by run size, not connection count#17
wshallwshall merged 1 commit into
mainfrom
noloss-budget

Conversation

@wshallwshall

Copy link
Copy Markdown
Collaborator

test_load_runner.py::test_run_load_end_to_end_no_loss intermittently reds the required test (windows-2025, py3.14) leg, which blocks auto-merge on unrelated PRs (it held #14). It reproduces identically on main.

The bug is the model, not the number

The reconcile excuses an unconfirmed send — in flight at a connection close, no ACK seen — because sent is counted at write-buffer time, so the frame may never have left the socket. That excusal is capped, correctly, so timeouts == sent can't 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 does not hold for this sender: _inflight is an unbounded deque, and an open-loop phase paces sends by the offered rate rather than by an ACK slot. What is genuinely in flight at teardown is therefore ~rate × ACK-latency — a share of the run, unrelated to the connection count.

The observed failure is a demonstrably zero-loss run:

sent 90
stranded at teardown 14 (~16%) — against a budget of 4
engine_read 84 = 90 − 6 that never left the socket
engine_written / sink_received 168 / 168 (fan-out 2)
backlog 0

Nothing was lost. The neighbouring acked >= sent // 4 assertion in the test was already deliberately loosened for exactly this teardown behaviour; no_loss's own budget never got the same treatment. That asymmetry is the defect.

The fix

Cap at max(connections, half the run), in all three copies of the accounting (load/report.py, connscale, estate — kept in step deliberately). read >= sent // 2 stays always required, so a dead ACK path still fails loudly while teardown weather does not. It is ~3× the worst stranding observed, and far from vacuous.

Reviewer note — one verdict is deliberately flipped

test_connscale_reconcile_timeout_flood_fails_even_without_shortfall asserted that 6 of 36 unconfirmed is a systemic fault. Under the new bound that is ordinary teardown stranding, so the test would have passed for the wrong reason. It is re-scenario'd to a genuine flood (30 of 36), and a new test pins that the 6-of-36 shape now reconciles clean — the behaviour change, made explicit rather than left implicit.

Two further tests pin that the widened bound has not blunted detection: the excusal stops at exactly half the run (one over flips to a systemic fault with nothing excused), and loss beyond a large excusal still fails.

Verification

  • tests/test_harness_reconcile.py: 17 passed. tests/test_load_runner.py: 5 passed.
  • Full reconcile-adjacent sweep (43 files: load, connscale, estate, multishard, harness): 668 passed, 3 skipped.
  • ruff format/check clean. CI's mypy covers messagefoundry/messagefoundry_webconsole, not harness/.
  • Mutation-verified: restoring the old budget = unconfirmed_budget fails all three new tests.

What this trades away, stated plainly

A widened excusal is a weaker intake gate: an engine dropping frames at intake produces no ACK, so those sends count as unconfirmed and are excused. Previously anything past ~1/connection failed; now up to half the run can be excused. The half-cap is what keeps that bounded, and the delivery-side checks (sink_received >= written, backlog == 0) are untouched. The alternative — making the sender wait out its in-flight window so timeouts stays ~0 and the strict bound holds — is the better long-term fix but cannot be validated locally, since the flake only appears under windows-2025 contention.

🤖 Generated with Claude Code

…ount

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.
@wshallwshall
wshallwshall enabled auto-merge (squash) July 27, 2026 21:37
wshallwshall added a commit that referenced this pull request Jul 27, 2026
…, and record the fix (#16)

(Subject deliberately avoids the literal "BACKLOG #N" token: the claim gate reads that in a
code-touching commit's SUBJECT as "this commit implements item N" and demands the number be claimed
first. This commit REMOVES a wrong citation rather than implementing anything, so claiming would be
the wrong signal. The body below names the item freely.)

BACKLOG #17 is the py3.11 pytest/aiosqlite cancellation deadlock, closed OBSOLETE at the 3.14-only
migration. It is cited correctly in tee/relay.py, the webconsole conftest and test_smart_backend.py.

It is NOT the harness message-list livelock. A pre-existing comment on the flaky monitor test blamed
"#17" for that, and while fixing the livelock I copied the wrong citation into four more places --
two test docstrings, an assertion message and the push-guard header -- plus the commit messages and PR
bodies that went with them. Anyone following it lands on a closed item about an unrelated bug, which is
a slower and more confusing dead end than no citation at all.

Removed from the livelock sites; each now says plainly what the defect was, and notes what the wrong
citation used to claim so the next person does not re-derive it. The legitimate #17 references are
untouched.

Also records the livelock itself in CHANGELOG 0.3.1, where it shipped: PR #5 merged before the tag was
cut, but no changelog line went with it, so the release notes are silent on a user-visible fix to the
harness message list. Filed under Fixed with the measurement that identified it (391 reads served, 0
rendered) rather than a vague "fixed a refresh bug".

And corrects the push guard's rationale for leaving enforce_admins off. It cited the "known flaky test"
as the reason an admin override must stay available; that failure was a livelock, not a flake, and it
is fixed -- so the argument is weaker now than when it was written. Said so rather than leaving a stale
justification standing for a security-relevant setting.
@wshallwshall
wshallwshall merged commit ba324ba into main Jul 27, 2026
31 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