fix(harness): assert empty claims per MESSAGE, not per second (BACKLOG #1101) - #292
Merged
Conversation
…G #1101) The empty_claims_monotonic SLO carried wall clock in its denominator, so anything that slowed a run collapsed it with no engine change. Measured on one commit, one box: four contended replicates spread 0.451 to 2.49 against a 0.75 floor. It fired five times in one day on PRs whose content cannot reach the engine -- a docs and link-checker diff, and a mocha dev-dependency bump. The asserted metric is now empty_claims_per_msg. Both inputs are deltas over the SAME first-to-last in-hold samples, so dividing them cancels the span exactly and leaves Δempty_claims / Δread. The wall clock disappears algebraically rather than by assumption, which is the property that makes it survive contention: slowing the run scales numerator and denominator identically. This is also the quantity wall #3 actually means. profile.py documents the mode as measuring the per-commit herd size, which is a per-message quantity; the per-second form was a proxy for it that the runner could move. The per-second numbers are retained in the report as the operator-facing figures. They are simply no longer what gates a merge, and the JSON now carries total_per_msg alongside them so a triager can see both. Returns None, never 0.0, when the window absorbed no messages. The ratio is genuinely undefined there and a fabricated zero would chain through the monotonicity comparison as if it were a reading. ALSO FIXES THE LATENT GROUPING DEFECT, in the same pass because it is the same function. _monotonic_slo grouped by sweep_mode ALONE and chained prev_val across the sorted group, so a profile setting claim_modes = ["per_lane", "pooled"] would have compared pooled against per_lane -- and compare.py states pooled's rate SHOULD be materially lower, meaning a CORRECT engine would have failed it. No shipped profile combines them, which is the only reason it never fired. Now groups by (sweep_mode, claim_mode). VERIFIED AGAINST THE FAILURE MODE, NOT JUST FOR GREEN. The risk in this change is replacing a gate that fails at random with one that never fails at all, and a correction is the easiest place to skip measuring because it feels like it has already paid its dues. So the invariance property and the still-detects-a-real-regression property are pinned together, and both were shown to go RED under mutation before being trusted: grouping reverted to sweep_mode only 3 failed per-second metric restored 2 failed restored 8 passed A metric that never fires would have passed a stability test on its own. Not done, deliberately: gating reload_seconds directly. The item raises it as a conditional -- "if that cost is worth gating" -- which is a separate judgement. Verified: ruff format and check pass; 162 connscale tests pass (3 skipped); backlog hygiene 19 passed; the repo-wide link gate reports 5,359 links across 347 files all resolving. harness/ is not under CI mypy (it checks messagefoundry and messagefoundry_webconsole only).
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.
Fixes BACKLOG #1101, and the latent grouping defect recorded in the same item.
The defect
empty_claims_monotonicreadempty_claims_per_s, which carries wall clock in its denominator. Anything that slowed a run — CPU contention on a shared runner, or the O(N) reload probe firing mid-hold and stalling commits — collapsed the numerator while the denominator kept ticking. The gate went red with no engine change.Measured on one commit, one box: four contended replicates spread 0.451 to 2.49 against a 0.75 floor. It fired five times in one day, on PRs whose content cannot reach the engine — a docs-and-link-checker diff (#281) and a mocha dev-dependency bump (#285, twice, in two different sweep modes).
The fix
The asserted metric is now
empty_claims_per_msg. Both inputs are deltas over the same first-to-last in-hold samples, so dividing them cancels the span exactly:The wall clock disappears algebraically rather than by assumption — which is the property that makes it survive contention: slowing the run scales numerator and denominator identically.
It is also the quantity wall #3 actually means.
profile.pydocuments the mode as measuring the per-commit herd size, which is a per-message quantity; the per-second form was a proxy the runner could move.Returns
None, never0.0, when the window absorbed no messages — the ratio is genuinely undefined there, and a fabricated zero would chain through the comparison as if it were a reading.The per-second numbers stay in the report as the operator-facing figures. They are simply no longer what gates a merge, and the JSON now carries
total_per_msgbeside them so a triager sees both.The latent defect, fixed in the same pass
_monotonic_slogrouped bysweep_modealone and chainedprev_valacross the sorted group. A profile settingclaim_modes = ["per_lane", "pooled"]would have compared pooled against per_lane — andcompare.pystates pooled's rate should be materially lower, so a correct engine would have failed it. No shipped profile combines them, which is the only reason it never fired. Now groups by(sweep_mode, claim_mode).Verified against the failure mode, not just for green
The real risk here is replacing a gate that fails at random with one that never fails at all — and a correction is the easiest place to skip measuring, because it feels like it has already paid its dues. So the invariance property and the still-detects-a-real-regression property are pinned together, and both were shown to go RED under mutation before being trusted:
sweep_modeonlyA metric that never fires would have passed a stability test on its own.
Not done, deliberately
Gating
reload_secondsdirectly. #1101 raises it as a conditional — "if that cost is worth gating" — which is a separate judgement, not part of this fix.Checks
ruff format+ruff checkpass. 162 connscale tests pass (3 skipped). Backlog hygiene 19 passed. Repo-wide link gate: 5,359 links / 347 files, all resolve.harness/is not under CI mypy, which checksmessagefoundryandmessagefoundry_webconsoleonly.BACKLOG #1101 claimed via
claim.ps1before the code commit, and flipped to shipped in this PR.