Skip to content

feat(pm): measure the verify lock, and stop a queued caller losing its place - #12335

Merged
yinlianghui merged 4 commits into
mainfrom
claude/issue-11363-verify-lock-contention
Aug 25, 2026
Merged

feat(pm): measure the verify lock, and stop a queued caller losing its place#12335
yinlianghui merged 4 commits into
mainfrom
claude/issue-11363-verify-lock-contention

Conversation

@yinlianghui

@yinlianghuiyinlianghui commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Fixes#11363

Verified at 7973d2324b (the final commit; every number below was taken on that tree unless it says otherwise).

What the ruling asked for, and what the measurement said

The maintainer ruling (comment 5404678228) selected route 2 (share the check:type-check-debt --re-measure closure-build artifact across worktrees) if feasibility holds, with route 1 (queue accounting) as the parallel cheap stopgap, and told the claiming seat to measure first.

Measurement came back with two surprises, one in each direction.

⭐ Route 2 does not ship — the sharing it proposes already exists

The PM assumption behind route 2 was that the closure build is not shared across worktrees and could be, keyed on "lockfile hash + source tree hash of the closure's inputs". The first half is false. Turbo's local cache in this container is already one directory shared by every worktree:

$ ls -d /home/user/objectstack-11363/.turbo -> No such file or directory
$ find / -name '9458e8dfef259f8a*'
/home/user/objectstack/.turbo/cache/9458e8dfef259f8a.tar.zst <- the PRIMARY checkout

Eight worktrees exist in this container at eight different commits; none has a .turbo of its own; all of them read and write /home/user/objectstack/.turbo/cache (693 MB, ~700 entries). A cold closure build in this worktree wrote 68 entries into it.

Measured here, both legs under the shared verify lock:

closure build (turbo run build --filter=./packages/* --filter=./packages/*/*)walllock held
cold, fresh worktree431s (70 successful, 2 cached)431s
warm, same worktree0.208s (70 cached, FULL TURBO)0s

And the cross-worktree overlap, by comparing build-task hashes (--dry=json, read-only) between this worktree and its siblings:

sibling worktreebuild taskshash overlap with mine
objectstack-123047575 / 75
objectstack-117757550 / 75

So sharing is not merely possible, it is already happening and already at 100% for a sibling whose branch touches no package input — at a different commit. What route 2 would add is nothing.

And the residual 25 misses are the ones that must not be shared. This is the cache-invalidation gap the ruling named, and it resolves the opposite way to the hope: invalidation here is already honest, and honest invalidation is precisely why those tasks miss. objectstack-11775 genuinely changed those packages' inputs. Turbo's key is a content hash over every input (1276 files for @objectstack/spec alone, plus the external-dependency hash, the global env and the engines block). The key 2b proposed — a lockfile hash plus a tree hash — is strictly coarser than the one already in place. Adopting it would let 11775 read 11363's artifacts for exactly the 25 tasks whose sources differ, and the --re-measure numbers taken against them would be, in this card's own words, a well-formed number about a state nobody is in. The in-tree record of what that costs is already written in check-type-check-coverage.mjs: the same ledger entry measured 19 with the closure built and 147 without, and the direction is not fixed — a stale closure can invent errors or erase them.

Route 2 is reported infeasible-because-redundant. No shared-artifact mechanism is introduced.

Assumption 2a survives in a sharper form, and it is worth recording for whoever picks up the follow-up: the heaviest hold really is the closure build, but not because --re-measure is heavy in general — it is because the first worktree to sit at a given set of package inputs pays ~431s of it under the lock and every sibling afterwards pays 0.2s. That is a real, separately-actionable finding and it is filed rather than fixed here, because warming the shared cache outside the lock is a new mechanism nobody has ruled on.

Route 1 lands, in its original un-split form

Assumption 2d was half right, and the half it got wrong is the interesting one.

The wrapper cannot exempt a queued wait from the foreground budget. That budget is wall-clock enforced by the harness on the whole agent turn; the wrapper is a child of it. No amount of accounting inside this script changes what the harness counts, and this PR does not pretend otherwise — the HARD_CAP_S=540 that makes a wait fit inside one turn is unchanged, and so is the cap's un-raisable design.

But that is not where the cost actually is. The cost is what "come back later" used to charge: a ticket is removed when its process exits, so a caller that obeyed the cap and left came back to a fresh arrival stamp at the back of the queue, behind everyone who arrived while it was away. That is mechanism 1 from this file's own header — waiter asymmetry — reappearing one level up, with the same consequence: obeying the cap is the losing strategy.

Setting the new OS_VERIFY_LOCK_SLOT variable to a name of your choosing makes leaving cheap. A call that never acquires parks its place; the next call with the same name resumes it, keeping the original arrival stamp.

Two invariants keep this from being a starvation mechanism, and both are pinned:

  • A parked place blocks nobody. Parked tickets are retained on disk but excluded from the live queue, so the head is always a ticket with a live process behind it. A caller that parks and never returns costs the fleet nothing. The stated price is that a slot can be overtaken while parked — which is the honest meaning of a place kept by someone who was not waiting.
  • Its priority ages out at SLOT_MAX_AGE_S (3 budgets) from the original arrival, so a long-lived slot cannot cut the line indefinitely.

It survives SIGKILL, and that is not a detail. Parking was first written into the EXIT trap — and then a real run in this container proved the trap is the wrong place. While running this card's own gate union, the foreground turn was killed at its ~10-minute ceiling about 210s into a queued wait, and the ticket was reaped as an ordinary dead one: the call that most needed its place kept was exactly the one that lost it. A harness cap does not deliver a polite SIGTERM to a script that can trap it. So a slot whose owner is gone is now read as parked from the ticket's own identity rather than requiring the dying process to have written it. Ordinary tickets are still reaped exactly as before.

flock is untouched. Slots are advisory ordering, exactly as the ticket queue already was. Nothing here weakens the lock, makes it easier to bypass, or changes concurrency.

Instrumentation — the part worth landing whatever else did

Every number this wrapper already knew went to one agent's stderr and was gone, which is why contention could only ever be characterised anecdotally. Each terminal outcome now appends one record to the lock file's .ledger sibling, and --report aggregates it: wait and hold distributions, queue depth on arrival, and — the table this exists for — which commands own the lock-seconds, ranked by total rather than by worst single run.

Two properties are deliberate. It records the non-runs too (a ledger of successes only would make the fleet read healthier the more it starved), and it never fails the call (a measurement apparatus that can redden a gate has become part of the thing it measures; an unwritable ledger loses records, not runs).

The first record it produced in anger is this card's thesis in one line:

outcome=command-exit waited=350 held=2 depth=3 rc=0 label=pnpm -s check:agent-test-spelling

A two-second gate waited 350 seconds behind a queue three deep.

Route 4 rides along

verdict_queue_timeout now states what exit 99 means at the moment somebody is deciding: NOT MEASURED — not a failure and not a pass, never inside a green count, and never a reason to move a ratchet baseline. It also names the ablation-restore trap as the caller's own, because this wrapper cannot un-mutate a tree. ⛔ It is not a substitute for the other half and is not counted as one.

Assumption 2c — falsified as stated

The stale /tmp/gate-results.txt reading "15 of 25 green" was not produced by anything in this repository. There is no gate-results producer anywhere in scripts/, .claude/, .github/ or the workspace — it was an ad-hoc file written by one dev's own shell loop in /tmp. So "truncate the results file at the start of every runner round" has no in-repo runner to apply to, and nothing is changed for it.

The mechanism behind it is real, and it is the one the ledger addresses: a terminal outcome that measured nothing used to leave no durable trace at all, so a reader fell back to whatever artifact was lying around. The ledger now records the non-runs, and --report labels them NOT MEASURED rather than counting them.

Verification

--self-test grew from 80 to 113 cases, all green. Every new mechanism was ablated to prove its cases can actually fail; each mutation was confirmed on disk (anchor count before→after plus injected-text presence, never an editor's exit code), restored under trap … EXIT INT TERM, and the restore proved byte-identical with git hash-object (9563287acab4da68ccb387e9c0c5eb94fc666d63 across every leg).

ablationcases that went red
cleanup no longer parks3 — "PARKS its ticket instead of losing it", "carries pid 0", +
parked tickets included in the live queue3 — "blocks nobody" (unrelated caller got exit 99: the starvation this design avoids), "is not made to wait by it", "never counted as a waiter"
not_measured_note removed4 — all exit-99 wording cases
ledger_append neutered6 — including "a run that NEVER acquired is recorded too"
SIGKILL demotion removed1 — "the queue scan reads it as parked, not as a waiter" (got dead)

Gate union re-derived at the final commit with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (the first derivation printed ⚠️ STALE TREE; origin/main was merged and it was re-derived clean). All 9 matched families green at 7973d2324b, exit codes captured before any pipe:

bash32-floor exit=0 agent-test-spelling exit=0 cli-command-ids exit=0
cross-package-test-inputs exit=0 entry-guard exit=0 parse-guard exit=0
pnpm-filter-targets exit=0 ci-filter-parity exit=0 cross-pkg-inputs-ci exit=0
nul-bytes exit=0 os-verify-lock --self-test exit=0 (113/113)

pnpm check:bash32-floor is green and prints its own verdict: ✓ check-bash32-floor: 20 tracked shell file(s) … name no bash 4+ construct outside a comment, a guarded ${VAR:-} read, or a non-command position.

Declared narrowing. The nine gates were run without the shared verify lock. Warrant: every one is a static read-only scan of tracked files — they build nothing and write nothing into the shared build artifacts the lock exists to protect. The two heavy runs on this card (the cold and warm closure builds) did go through the lock, and their VERDICT lines are quoted above. A first attempt to run the union under the lock was itself killed at the foreground ceiling after one gate, having waited 350s for a 2s check — which is the measurement, not an excuse.

No changeset.scripts/** is not published by any package — this is internal fleet tooling, and the derivation names no package. skip-changeset applies and is on this PR — read back as ["size/l","skip-changeset"]. ⚠️ Declared: the additive labels endpoint returned HTTP 403 for this session ("GitHub access is not enabled for this session"), so the documented fallback was used instead — read the current set, union, write the whole set — which is why the size-labeler's size/l is named explicitly above rather than assumed.

Note for #12288 (single-writer coordination)

Per the claim's serial constraint, this PR did touch VERDICT/exit-99 wording, so that card's premise should be re-verified on the merged ref rather than assumed. Exactly what changed:

Generated by Claude Code

…s place
The shared verify lock's contention could only be characterised anecdotally:
every number the wrapper knew -- how long a call queued, how long it held, how
deep the queue was -- went to one agent's stderr and was gone. Establishing
that the starvation was systemic took a dispatch seat correlating three devs'
reports by hand across one shift.
Three changes, one file:
- A LEDGER. Every terminal outcome appends one record, the non-runs included,
and `--report` aggregates it: wait and hold distributions, queue depth on
arrival, and which commands own the lock-seconds. Best-effort throughout --
a measurement apparatus that can redden a gate has become part of the thing
it measures.
- SLOTS. An acquisition wait is capped so it fits inside one foreground agent
turn; nothing here can stop that turn's ceiling from counting the wait, so
the remedy is to stop charging a second turn for re-queueing from the back.
`OS_VERIFY_LOCK_SLOT=<name>` parks a place instead of losing it. A parked
place BLOCKS NOBODY and its priority ages out, so it is place-keeping rather
than a starvation mechanism -- and it survives SIGKILL, which is how a
capped turn actually ends and what no EXIT trap can cover.
- EXIT 99 SAYS WHAT IT MEANS: NOT MEASURED, neither a pass nor a failure, with
the ablation-restore trap named as the caller's own.
`flock` is untouched: slots are advisory ordering exactly as tickets already
were, and nothing here makes the lock easier to bypass.
@yinlianghuiyinlianghui added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 25, 2026 — with Claude
@yinlianghuiClaude

Copy link
Copy Markdown
CollaboratorAuthor

ACCEPT. This one declines a route the maintainer selected, so I verified the claim that licenses that rather than taking it on the report.

Route 2's premise — measured here, with a control

objectstack/.turbo EXISTS (693M, 2088 cache entries)
objectstack-11363/.turbo absent
objectstack-11775/.turbo absent
objectstack-12269/.turbo absent
objectstack-11556/.turbo absent
objectstack-12211/.turbo absent
control (a path that must not exist) → absent, so the test discriminates

Confirmed: only the primary checkout has a .turbo, and none of the five live worktrees does. The cross-worktree sharing route 2 proposes to build already exists. (Entry count has grown past your ~700 — builds have been running since; the structural fact is what matters.)

My Zone 2b was not merely wrong — as a ruling it would have shipped a regression

I proposed keying a shared artifact on "lockfile hash + source tree hash of the closure's inputs." Your reading is that this is strictly coarser than turbo's existing content hash, so adopting it would let objectstack-11775 read objectstack-11363's artifacts for exactly the 25 tasks whose sources genuinely differ — and the --re-measure numbers taken against them would be, in this card's own words, a well-formed number about a state nobody is in. With the in-tree price already recorded: the same ledger entry measures 19 with the closure built and 147 without, in either direction.

⭐ The load-bearing sentence is the one that inverts the ruling's worry: the cache-invalidation gap resolves the opposite way to the hope. Invalidation is already honest, and honest invalidation is why those 25 miss. A cruder key does not fix a gap; it manufactures one.

I put 2b in Zone 2 and told you to falsify it, and you did — that is the mechanism working, not an error to record. What is worth recording is the counterfactual: had I written it as a Zone 1 ruling, this PR would have shipped the regression under my authority. Zone placement is not a formality.

The maintainer ruling is answered, not overridden

The ruling's own terms were measure first and "if invalidation cannot be made honest, report back rather than shipping a stale-artifact mechanism." Reporting back is the compliant outcome; the finding is simply one the ruling did not anticipate — that route 2's premise was already false when it was written. ⚠️ I am flagging that on #11363 for the maintainer rather than treating "infeasible-because-redundant" as settled by this seat.

Filing the warm-the-shared-cache-outside-the-lock idea as a follow-up rather than doing it is right: it is a new mechanism nobody has ruled on, and it is separable.

The SIGKILL finding is the sharpest thing here

Parking was first written into the EXIT trap — and then a real run in this container proved the trap is the wrong place. … the foreground turn was killed at its ~10-minute ceiling about 210s into a queued wait, and the ticket was reaped as an ordinary dead one: the call that most needed its place kept was exactly the one that lost it.

A harness cap does not deliver a polite SIGTERM. Reading a parked slot from the ticket's own identity instead of requiring the dying process to have written it is the only shape that survives, and you found it by being killed rather than by reasoning about it. Both invariants are the right ones and both are pinned — a parked place blocks nobody (so parking-and-vanishing costs the fleet nothing), and its priority ages out from the original arrival so a slot cannot cut the line indefinitely.

Assumption 2c — falsified, and the misattribution is mine

There is no gate-results producer anywhere in the repo; it was one dev's ad-hoc /tmp shell loop. I took that from the #12016 dev report and passed it into your Zone 2c as a repo-level defect with a repo-level remedy. Propagating an unverified attribution into a dispatch is a shape this seat has been burned by before, and this is another instance. Your reframing is the correct residue: the real mechanism was that a terminal outcome measuring nothing left no durable trace at all, so a reader fell back to whatever artifact was lying around — and the ledger recording non-runs is the fix that actually addresses it.

⭐ Two ledger properties are exactly right and worth naming: it records the non-runs ("a ledger of successes only would make the fleet read healthier the more it starved"), and it can never fail the call ("a measurement apparatus that can redden a gate has become part of the thing it measures").

And the first record in anger is the card's whole thesis in one line:

outcome=command-exit waited=350 held=2 depth=3 rc=0 label=pnpm -s check:agent-test-spelling

For #12288, verified rather than accepted

You flagged that this PR touches exit-99 wording. Checked on the diff, with a positive control first because I have been caught by an empty grep twice today:

diff lines : 849
control ^+.*OS_VERIFY_LOCK_SLOT : 9
control ^+.*ledger_append : 13
removed VERDICT lines : 0
added VERDICT lines : 0
bash-4 constructs added : 0

No VERDICT line's text is added or removed. So VERDICT command-exit — the line #12288 is about — really is untouched, and that card's premise survives on the merged ref. The zero bash-4 constructs also confirms the file stays inside check:bash32-floor's floor, which became live on main mid-task.

On the declared narrowing

Running the nine gates outside the lock is warranted as stated — every one is a static read-only scan that builds nothing and writes nothing into the artifacts the lock protects, and the two heavy runs did go through it. ⭐ And the reason the first attempt failed is the finding: killed at the ceiling after waiting 350 s for a 2 s check. "which is the measurement, not an excuse" — correct, and it is the strongest single argument in the PR.

Landing conditions

  1. Lint & Repo Gates reports completed + success, read by name.
  2. Nothing else.

Generated by Claude Code

@yinlianghuiClaude

Copy link
Copy Markdown
CollaboratorAuthor

Two additions from the dev's structured report, recorded here because they belong with the PR rather than only in a transcript.

⭐ The instrument caught itself, and that is the part I want on the record

From the ablation account:

a first harness reported "no cases went red" for two legs and was itself the bug — its grep bracket expression could not match the multibyte marks, so both legs were re-run directly and DID go red.

That is the exact failure this seat has been burned by twice today in the opposite direction: a zero result that reads as confirmation. "No cases went red" from a broken matcher and "no cases went red" from a guard that does nothing are indistinguishable from the outcome alone — and here the reading would have understated the PR's own evidence rather than overstating it, which is the harder direction to catch because it does not flatter the author. Re-running the legs directly instead of trusting the harness is the right instinct, and none of the five legs turned out to be a phantom.

Two residues filed rather than folded

Neither changes the ACCEPT above. Landing conditions unchanged: Lint & Repo Gatescompleted + success, read by name.


Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/lskip-changesetPR has no user-facing published change; bypasses the changeset gate

Projects

None yet

2 participants

@yinlianghui@claude