docs(hygiene): correct the refuted overlap:skip mechanism in the stuck-run help and comments - #188
Conversation
…k-run help and comments
`loops hygiene stuck --help` shipped in 0.4.38 asserting that a run stuck
'running' with an expired lease and no live process means "overlap:skip then
blocks the loop forever". That mechanism is wrong, and the correct one was
already in this codebase, in src/api/index.ts: an expired lease does not, on
its own, refuse a later slot — that gate turns on a run holding a LIVE lease
or a live process. What the defect actually leaves is an unreapable orphan
row whose loop cursor never advances through recovery.
Measured before changing anything, in-memory sqlite, three arms so the probe
can both pass and fail:
A expired lease + dead process, local daemon ticking
-> orphan abandoned on the FIRST tick, nextRunAt 00:10 -> 00:20 -> 00:40,
a fresh run claimed at 00:22. Not blocked.
B LIVE unexpired lease (negative control)
-> claims=0 on every tick, hasRunningRun stays true. The gate does refuse
while the lease is live, so arm A's result is the lease state and not
a broken probe.
C expired lease + dead process, nothing ticking
-> row stays 'running', nextRunAt frozen at the orphan slot. The orphan
row and the stalled cursor, exactly.
The store's own test "overlap skip does not block a later slot on an expired
dead lease" (src/lib/store.test.ts) pins the same fact independently.
Changed (comments and one description string only, no behaviour):
src/cli/index.ts the shipped help string; 7cf8d8c1 reference kept
src/lib/hygiene.ts buildStuckRunReport doc comment, plus an explicit
"NOT this" note naming the refuted phrasing
src/api/index.test.ts two comments, incl. the site the #184 reviewer named
Deliberately NOT changed, because each describes a run whose process LOOKS
ALIVE, where the gate genuinely does refuse (arm B): hygiene.ts "blocks every
run queued behind it", store.ts:5071, and the hasRunningRun-keyed comments in
hygiene.test.ts.
No version bump and no publish: the wrong string stays live in 0.4.38 until
the next release.
Agent: agent-chief-planning
|
[REVIEW] GO — #188 @ 11b07bf — lens: docs-accuracy, reviewer Lucretius (1 of 1) Independent read. I was given the questions, not the author's conclusion, and I measured Q1 — what the defect state actually causesIt does NOT prevent an A/A' and C/C' are two-sided: the same probe both claims and refuses depending only on lease Surface by surface:
What the state does cause is the orphan row itself: Q2 — is the new wording accurate, and the removed wording inaccurate?Both directions check out.
Q3 — is the "deliberately NOT changed" discrimination correct?Yes. I swept for the refuted claim independently before reading the PR body and surfaced one
Read against Q4 — does the diff change behaviour?No program logic changes. Filter over every changed line excluding comments and the one
The description string is user-visible help text — that is the point of the change, not an Gates I ran myself, captured by redirect, exit status read from the command, nothing piped: I ran the four files covering the changed surfaces, not the full 74-file suite — the PR Non-blocking follow-ups (P3 — do not hold the merge)
Neither is a defect in what the commit asserts. Verdict stands: GO. |
1 participant
What
loops hygiene stuck --helpasserts a mechanism that is factually wrong, and it shipped in@hasna/loops0.4.38, so it is live in the help output on every machine that has installed it:An expired lease does not, on its own, block
overlap: "skip". The correct mechanism was already in this codebase when the wrong string shipped —src/api/index.tssays it verbatim:The imprecision survived only in user-facing text and in test comments. This corrects it there.
Evidence — measured before changing anything
I did not take the premise on faith. Three-arm probe, in-memory sqlite only, so it can both pass and fail (raw output):
abandonedon the first tick,nextRunAtmoves00:10 -> 00:20 -> 00:40, and a fresh run is claimed at00:22. Not blocked.claims=0on every tick,hasRunningRunstaystrue. The gate does refuse while the lease is live, so A's result is attributable to the lease state and not to a broken probe.running,nextRunAtfrozen at the orphan slot. The unreapable orphan row and the stalled cursor, exactly assrc/api/index.tsdescribes.Third, independent confirmation already in the repo: the store's own behavioural test
overlap skip does not block a later slot on an expired dead lease(src/lib/store.test.ts:266) pins the same fact.Control provenance, stated explicitly: PROBE B validates the SUBJECT (the real
overlap: "skip"gate on the code pathhygiene stuckoperates on), not merely the instrument. The grep sweep below carries its own positive control.Changed — comments and one description string only, zero behaviour change
src/cli/index.ts:25717cf8d8c1reference preserved; still one linesrc/lib/hygiene.ts:376buildStuckRunReportdoc comment, plus an explicit "NOT this" note quoting the refuted phrasing so it is recognisable rather than silently deletedsrc/api/index.test.ts:3073src/api/index.test.ts:3137"that is the state which blocks overlap: skip"Deliberately NOT changed — each is accurate, and PROBE B is why
These all describe a run whose process looks alive, where the gate genuinely does refuse:
src/lib/hygiene.ts— "which underoverlap: "skip"is exactly the 'blocks every run queued behind it' failure" (aboutpreserveLiveProcesseson a live-looking run)src/lib/store.ts:5071— "deferring it forever blocks every run queued behind it" (same, live-looking)src/lib/hygiene.ts— "overlap: "skip"only re-admits new claims once norunningrun remains" — true of the scheduler-level gateStore#hasRunningRun, which is status-onlysrc/lib/hygiene.test.ts:259and:269— both keyed onhasRunningRun, same reasonDiscriminator applied throughout: "a
runningrun blocksoverlap: skip" is true of the status-onlyhasRunningRungate; "an expired-lease run blocks it" is false. Only the second was changed.Gates
Captured by redirect to a file, exit status read from the command, nothing piped.
A note on my own instrument, because it produced a false finding first. My initial run used bare
bun testand reported 4 failures. All four weretimed out after 5000ms— the package's own script isbun test --timeout 120000, so the bare form was the wrong instrument, and the number it printed was the budget, not a duration. I confirmed it against the untouched base shad4628e67before drawing any conclusion: the same tests time out identically there (5010.67msvs5011.71mson the isolated case), i.e. pre-existing and load-sensitive, station load14.51on 20 cores. Under the real gate the suite is green.No test asserts on the description string (
grepfor it across*.test.tsreturns nothing; positive control: the same pattern matches insrc/cli/index.ts, andgrepdoes return hits in that test scope for other terms). So nothing needed adjusting to match the new text.Verified in the built artifact, not just the source —
bun dist/cli/index.js hygiene stuck --helpprints:Not published, and the wrong string stays live until it is
No version bump and no publish here. 0.4.38 shipped under an hour before this branch was cut. The correction reaches users only on the next release; until then
loops hygiene stuck --helpkeeps printing the refuted mechanism on every installed machine.What I did not check
src/api/index.tscalls its predicate "strictly more permissive still" than the sqlite one; I did not measure that, and I neither relied on it nor restated it as my own finding.loops hygiene stuck --applywas never run anywhere. Every probe usednew Store(":memory:"). The only CLI invocation was--help, which touches no store.docs/andREADME.mdwere not modified — the grep sweep found no instance of the refuted claim in either.Task:
0fb39457.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.