Repository navigation
test(watcher): pin the shutdown sentinel's position in cleanup() - #3051
Conversation
#3025 moved the `$DISPATCH_DIR/shutting-down` write to the top of `cleanup()` so the drain and queue guards observe it before any release, kill or sweep can still promote a worker (#2934). It shipped as a shell-ordering change with no test, so the property can regress silently — `shutting-down` appears nowhere under tests/. Adds the ordering assertions to the suite that already owns this script's shutdown behaviour, rather than a new file. The checks are guarded against passing for the wrong reason: an empty cleanup() extraction is asserted non-empty first (otherwise every ordering check below it is vacuous), and each anchor's presence is asserted before it is compared, because `[ "" -lt 5 ]` is an error rather than a pass. Falsified: moving the sentinel write to just after `sentinel_release_if_owner` fails the suite with FAIL the shutting-down sentinel precedes sentinel_release_if_owner: expected 'yes', got 'no' Restored, all checks pass. review-checks PASS, shellcheck clean, bash -n ok.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed the diff (test-only, +29). The ordering pins are exactly the #3025 invariant — sentinel-first in cleanup() — and the anchor-existence guards make the ordering checks non-vacuous: a renamed anchor fails its own check instead of silently passing the comparison. awk extraction is comment-stripped so the match can't land on prose. Safe; no live path touched.
|
The red From the job log (run 32055212191, job 95464473586) — the step never reached tsc or the tests:
Verified it is not a content failure, using main's scanner rather than this branch's copy: And the discriminator that it is not repo-wide: the same check is This is the failure mode #3050 fixes — a fetch failure that lands as a content-gate failure. Worth noting it now costs a full CI cycle per occurrence. One incidental finding, not from this PR: run main's scanner over |
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Approving — this adds real coverage where there was none, and the vacuity guards are the part I want to praise before I criticise the approach.
Verified, not assumed: the ordering the test asserts is genuinely true at main's cleanup() — sentinel write at relative line 7, sentinel_release_if_owner 16, first kill -TERM 18, fallback_outstanding_handlers 20. So it passes today for the right reason.
I did not run the file. Its earlier sections spawn watchers and send SIGTERM, and this host has a live task watcher doing real work; a shutdown test against that is not worth the outage. I exercised the new block's logic against the real source by hand instead. Flagging so you know which claim is measured and which is not: the ordering is measured, "the suite still passes end to end" is not — CI is mid-run on a2c71e82 as I write.
The nit, and it is a real one: this pins source ORDER, not behaviour. line_of greps the extracted cleanup() text and compares line numbers, so it passes if the sentinel merely appears first, and fails on a refactor that preserves semantics — extract the kill into a helper and the anchor vanishes even though the guarantee is intact. CLAUDE.md is direct about this shape ("Concurrency tests must call the production writer, not a copied recipe or source-regex surrogate"), and unlike a docs test this one is about a concurrency guarantee (#2934's worker promotion).
Two concrete ways it can go quietly wrong:
grep -vE '^[[:space:]]*#'strips only whole-line comments. A trailing comment mentioningshutting-downabove the real write would become the anchor and the ordering check would compare the wrong line. Not present today; nothing prevents it.line_of 'kill -TERM'takes the first match.cleanup()also haskill -TERM 0later; today the order makes that harmless, but the anchor is positional, not semantic.
I am not blocking on it. The behavioural form — run the watcher, SIGTERM it, assert the sentinel exists before any promotion can observe it — is a genuinely hard race to stage, and a guarded source-order check beats no check. Worth a follow-up rather than a rewrite here, and the existence guards you added are exactly what keeps this from being the usual vacuous version of this pattern.
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
|
@cla-assistant check |
What
#3025moved the$DISPATCH_DIR/shutting-downwrite to the top ofcleanup(), so the drain and queue guards observe it before any release, kill or sweep can still promote a worker (the#2934shape). It shipped as a pure shell-ordering change with no test, and the sentinel name appears nowhere undertests/— so the property can regress silently, and the failure mode is a lost task rather than a red build.This adds the ordering assertions to
tests/watch-tasks-stream-shutdown-emit.test.sh, which already owns this script's shutdown behaviour and already carries the#2934lineage, rather than starting a new file.Before / after
At the parent commit the sentinel is untested — the suite passes whether or not the write is first:
At this head, with the write moved to just after
sentinel_release_if_owner:Restored:
Guarded against passing for the wrong reason
Both failure modes this file already worries about apply here, so both are closed explicitly:
cleanup()extraction would make every ordering comparison vacuously true, so the extraction is asserted non-empty first;[ "" -lt 5 ]is an error, not a pass — so each anchor's presence is asserted before it is compared.Scope
Test-only;
src/untouched.review-checksPASS,shellcheck -S errorclean,bash -nok.This is a follow-up closing a gap in my own merged PR, not new behaviour — reviewers should feel free to treat it as low priority against the rest of the queue.