fix(pm): head --report's hold bucket by the filter it actually applies - #12870
Merged
Conversation
#12823) `mode_report` builds all three distributions with one shared filter, `v > 0`. That filter fits the `waited` heading exactly -- a recorded `waited` of 0 IS "did not wait at all" -- and it does not fit an acquisition. `held` is written by every terminal outcome, so `held=0` names two disjoint things: a run that never acquired at all, and a run that acquired and released inside one clock tick. The heading claimed the bucket held the second kind while the filter drops it -- measured, 3 of 69 rows on the ledger this was filed from, one of which had waited 78s for the lock before holding it briefly. The dropped rows are the FASTEST holds, so p50 and p90 read high, and they read higher the better the fleet gets at holding the lock briefly. Presentation only. The filter is NOT changed: admitting held=0 would move n, p50 and p90 for every ledger past and future, so a figure quoted from an older report would stop matching a re-run over the same rows. That is a change of definition and is carved out to its own card. Every number this report prints is unchanged by this commit. Two things measured while writing it, which shaped the wording: - `held` is a difference of two whole-second clock reads (`now_s`), so the obvious spelling -- "over runs that held it for at least a second" -- is itself false: a 200ms hold straddling a tick records 1 and a 900ms hold inside one records 0. The heading says RECORDED. - `run_unlocked` writes `ledger_append unlocked 0 "$ran" ...`, so on a host with no usable flock the command's RUNTIME lands in the `held` field although the lock was never taken. Those rows are IN this bucket. Annotated, not filtered out. The `n` gap the report never explained now has a line of its own, and `waited`'s heading -- already true of its filter -- is untouched, with a self-test case pinning it present so the likeliest wrong repair (making the three headings "consistent") reds. Six --self-test cases: the new heading present, the old one GONE rather than annotated, the three explanatory lines, and the `waited` heading still there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CPrUz21stTFhJRUirdc4yw
This was referenced Aug 28, 2026
os-elon
marked this pull request as ready for review
August 28, 2026 06:34
os-elon
enabled auto-merge
August 28, 2026 06:34
This was referenced Aug 28, 2026
This was referenced Aug 28, 2026
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 #12823
mode_reportbuilds all three distributions from one loop with one shared filter:That filter fits the
waitedheading exactly — a recordedwaitedof 0 is "did not waitat all". It does not fit an acquisition.
heldis written by every terminal outcome, soheld=0names two disjoint things at once: a run that never acquired, and a run that acquiredand let go inside one clock tick. The heading claimed the bucket held the second kind while
the filter drops it.
The direction is the awkward one: the dropped rows are the fastest holds, so
p50andp90read high — and read higher the better the fleet gets at holding the lock briefly.Before / after
Both rendered over the same six-row fixture ledger. Every number is identical.
That fixture is worth reading twice:
records: 6againstn=3, and thep50=63is theunlockedrow — a run on which the lock was never taken. Two rows that did acquire(one after waiting 78s) are absent. The old heading described none of that.
⛔ The filter is NOT changed, and that is the ruling, not a preference
Admitting
held = 0would moven,p50andp90for every ledger, past and future: afigure quoted from an older report would stop matching a re-run over the very same rows, with
nothing in either report saying why. That is a change of definition, not a repair, and it
is carved out to its own card (filed; linked from the report comment). This branch changes no
number the report prints.
⛔
waited's heading is untouched — and pinned untouchedover runs that waited at allis already true of its filter. The likeliest way to get thisrepair wrong is to make the three headings "consistent" and break the one that was already
right, so a self-test case asserts that heading is still present, not merely that the new
one is.
Two measurements that changed the wording the card floated
The card suggested "over runs that held it for at least a second". Reasonable, and
measurably false — so it is not what shipped:
heldis a difference of two whole-second clock reads.held=$(($(now_s) - acquired_at)), andnow_sisEPOCHSECONDSordate +%s. A 200ms hold straddling a tickrecords 1; a 900ms hold inside one records 0. Promoting the recorded value to a
claim about true duration would rebuild this card's own defect one line further down. The
heading says RECORDED, and a
⇒line says what that is worth.run_unlockedwritesledger_append unlocked 0 "$ran" ...— on a host with no usableflockthe command's runtime lands in theheldfield although the lock was nevertaken. So the bucket is wrong in the other direction too: it admits rows that did not
acquire at all. Annotated here, because removing them is a filter change and belongs with
the carve-out above.
Tests
bash scripts/pm/os-verify-lock.sh --self-test— 186 cases, 0 failures, run under theshared verify lock on final head
193340a3(baseline before this branch: 180).Ablation: the six new cases were shown able to fail, and the direction was predicted
before the run. Mutation = the new heading reverted to the old string and the three
⇒blocks deleted, leaving the self-test cases in place.
waitedpin stays green — it was already true pre-fix⇒
✗ os-verify-lock self-test: 5 case(s) failed.Exactly five, exactly the predicted five.There is no build leg to state: this is a shell script executed from source, with no
dist/between the edit and the instrument. The mutation was proved to have reached the executed
surface by reading the mutated
--reportoutput itself (it printed the old heading), and therestore was proved by blob hash equal to
HEADplus an emptygit diff HEAD— not by an exitcode. An earlier ablation attempt died mid-mutation on a quoting error in the harness; its
trap ... EXITrestored the tree, verified the same two ways before continuing.Gates — the family was re-derived from the real change set with
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstackafter the final commit, and it added nothing. All green,each read from its own printed verdict line rather than a bare
$?:check:bash32-floor: 22 tracked shell file(s) ... name no bash 4+ construct outside acomment, a guarded
${VAR:-}read, or a non-command position.pnpm lintwas not narrowed — its population and this diff are disjoint, measured.isPathIgnored('scripts/pm/os-verify-lock.sh')returns true andcalculateConfigForFilereturns nothing for it, while the positive control
scripts/check-nul-bytes.mjsreturns areal config — so the instrument can return both answers.
eslint.config.mjsdeclares fileglobs only for
{ts,tsx,mts,cts,js,jsx,mjs,cjs}and mentions.shzero times, and it enablesno type-aware linting anywhere (its own comment at line 328: no
parserOptions.project, notyped rules, for ANY file), so no untouched file's verdict can move as a function of this
diff.
No changeset
Root
scripts/tooling publishes nothing. Consistent with the last twelve commits touchingscripts/pm/, which carry zero changesets between them — including PR #12827, thedirectly comparable repair to this same function, which carried
skip-changeset. Anempty-frontmatter changeset is not the alternative;
check-empty-changeset.mjsreds on it.Scope
The heading half only, per the ruling on the card. Related work, not addressed here and
still open: #12828 (the
/tmpephemerality source comment) and #12795 (routing gate runsthrough the lock). #12782 and #12783 are the two sibling repairs to this same function whose
pattern this one follows.
Generated by Claude Code