fix(harness): bound the no-loss excusal by run size, not connection count - #17
Merged
Conversation
…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
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.
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.
test_load_runner.py::test_run_load_end_to_end_no_lossintermittently reds the requiredtest (windows-2025, py3.14)leg, which blocks auto-merge on unrelated PRs (it held #14). It reproduces identically onmain.The bug is the model, not the number
The reconcile excuses an unconfirmed send — in flight at a connection close, no ACK seen — because
sentis counted at write-buffer time, so the frame may never have left the socket. That excusal is capped, correctly, sotimeouts == sentcan't degrade the intake bound toread >= 0and 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:
_inflightis 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:
Nothing was lost. The neighbouring
acked >= sent // 4assertion 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 // 2stays 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_shortfallasserted 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.messagefoundry/messagefoundry_webconsole, notharness/.budget = unconfirmed_budgetfails 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 sotimeoutsstays ~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