Repository navigation
fix(task-orphan-check): a task a worker holds is not the core's orphan - #4425
Conversation
…to report `scripts/unanswered-tasks.py` listed every `tasks/task-*.txt` without a result as unanswered, with no notion of delegation. `src/check-pending-tasks.sh` has skipped worker-held tasks since #4281/#4338, so the two guards disagreed about the same file: measured live, the hook skipped a task a worker held while this script flagged it. The proactive loop gates idle on this script (`unanswered-tasks && core-status idle`), so the core reported "running" for as long as any worker held anything — a false busy signal, and the mirror of the false-empty class #4394 fixed. There was no shared owner for the question, only the shell hook, so this adds one rather than a third copy: `src/worker_delivery.py:holder_of()` reads the router's `deliveries/<recipient>/<task-id>{.txt,.accepted,.claimed}` contract, and the script delegates to it. An unreadable deliveries/ propagates instead of answering "nobody holds it" — that reading is what hands a worker's task back. A held task is skipped from the verdict but still printed to stderr, so a genuinely stuck holder stays visible rather than becoming invisible to both guards. tests/worker-delivery-matches-the-hook.test.py: 14 checks, including a parity check that reads the hook's own `sentinel_task_id()` suffixes and pins them to the module's. Mutation controls: removing the skip fails exactly the three behavioural cases; dropping `.accepted` from the module fails exactly the parity case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot "owed"
holder_of propagates on an unreadable deliveries/ rather than reading it as
"nobody holds it" — that reading is what hands a worker's task to the core. The
script had no arm for it, so the PermissionError escaped as a traceback and rc 1:
the same code as "a task got no result". The caller chains
unanswered-tasks.py --workspace "$WS" && core-status.sh idle
so rc 1 there reads as owed work and re-creates the false BUSY signal this
change set exists to remove — for as long as the permission error lasts, which
on a host with EPERM bursts is not a moment.
main() now catches OSError, says which directory it could not read, and exits 2
(cannot decide). The module still propagates; the CLI owns its exit contract,
the same way the missing-result_markers path already exits 2.
Three distinct states, each pinned:
rc 0 a worker holds it
rc 1 nobody holds it and the room heard nothing
rc 2 deliveries/ unreadable — cannot decide
With the readable-again control in the same test, so the 2 is attributable to
the mode bits rather than to the tree.
Found by the worker seat this defect hurt, reviewing the PR. My own first probe
of its report was inert — a fake workspace whose task was younger than
--min-age-sec, so every state returned "none", including a positive control that
should have failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The module claimed a fail-closed contract and did not implement it. Path.is_dir() and Path.exists() report a FAILED stat as False, so an unreadable recipient directory or sentinel read as "nobody holds it" — the exact reading holder_of exists to prevent, inside holder_of. Reproduced against the previous commit, unreadable recipient directory: before (de1c3f3) returned None <- the core would take a worker's task after PermissionError errno 13 <- the caller decides os.stat/os.lstat with only FileNotFoundError treated as absence. lstat for the sentinel, not stat: a dangling symlink still names a delegation, so it must read as held rather than as gone. Both arms are pinned, each by the case the other cannot reach: sentinel probe -> Path.exists() fails the unreadable-recipient regression recipient probe -> Path.is_dir() fails the symlink-loop regression The second is not hypothetical. pathlib ignores ELOOP as well as ENOENT, so a symlink-loop recipient reads as "not a directory" and disappears from the scan entirely; chmod cannot discriminate there, because the stat fails before is_dir()'s swallow matters. Also adds in-process calls to unanswered(), because the held-task branch was only ever reached through a subprocess and no coverage tracer follows one — which is why the hosted diff-coverage gate reported those lines unhit while the suite passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI went red on exactly one case, the one I added last push: ELOOP is 62 on darwin and 40 on linux, so `exc.errno == 62` passed here and failed on the runner. The rest of the file passed on both. The assertion is about the CLASS of failure — a stat that fails for a reason that is not ENOENT — so it must name the constant, never a number read off one platform. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rcise The hosted diff-coverage gate reported `scripts/unanswered-tasks.py:103,106,108` and `src/worker_delivery.py:36` unhit while the suite passed. Both are reached only through a subprocess, and no coverage tracer follows one. Adds in-process calls for each: `main()` with a patched argv against an unreadable deliveries/ (the cannot-decide arm), and `_is_dir()` on a simply absent path (the ENOENT branch). The subprocess cases stay — they are what pin the CLI's exit code as a caller sees it. The in-process `main()` catches OSError rather than letting it propagate: this is a bare-script suite, so an exception aborts the run and hides every case below it. With the guard removed the mutation now reports 5 failures including these two, instead of stopping at the first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@john-the-dev @yixuan-ag2 — stacked child of #4413 (same reviewer set). The ruling step for a review is the before/after block: the parent commit returns |
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 89%. Diff CoverageDiff: origin/fix/unanswered-tasks-honours-worker-sentinels...HEAD, staged and unstaged changes
Summary
|
The owner's rule: keep this change inside the skill — code outside skills carries side effects for every install, and the delivery sentinel is the worker pool's contract, not the core's. src/ is untouched; the hook that already reads deliveries/ (src/check-pending-tasks.sh) stays as it is. scripts/unanswered-tasks.py treats the module as an optional capability: a missing skill means no router, so nobody holds a task (holder None); any other stat failure propagates, because an unreadable skill is not an absent one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7941162 to
5a3a945
Compare
|
Rebased onto the re-homed parent ( |
…lled once worker_delivery re-declared the three stages pool_delivery already names (PENDING_SUFFIX, ACCEPTED_SUFFIX, LEGACY_ACCEPTED_SUFFIX) — with the hook's bash copy, three spellings of one vocabulary. Derive the tuple from the owner; the module==hook pin test still holds. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The classifier consulted results/ and age only. On the first boot after a core restart, four owner DMs (6-26 min old) that the pool router had already handed to two worker seats — deliveries/<recipient>/<id> sentinel present, no result yet because the workers were mid-answer — classified `orphan`, were archived out from under their workers, and were reported to the owner as unanswered in the recovery DM. Ask the shared predicate, src/worker_delivery.py:holder_of (the same one the Stop hook and scripts/unanswered-tasks.py use), right after the completion-marker check: a held task is `worker-held` — left in tasks/, never archived, never listed in the DM; a non-ENOENT failure reading deliveries/ is `unknown` — left in place and listed, because an unreadable deliveries/ read as empty is what hands a worker's task to the core. Stacked on #4413 (introduces src/worker_delivery.py); merge order: #4413, then this. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
5a3a945 to
43fe64d
Compare
|
Changes are required on
Please preserve the existing import recovery instructions while adding the worker-held/unknown branches, rerun the full standalone suite, and get the required checks green. Reviewed by Qingyun's Personal Codex. |
…f one spelling Adding `worker-held` and `unknown` changed the reconciliation formula and the not-moved list, and the two literal assertions that pinned the old strings went red — correctly: they are what noticed. Pin the property instead, so the next verdict is caught rather than the next rename: every letter the summary defines must appear in the formula, and the not-moved set must still name the import holds. Verified it is stronger than what it replaces — dropping W and X from the formula now fails with "summary defines ['W', 'X'] that the formula never adds", where the literal only said the string was absent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cla-assistant check |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approving ac4f9a4d. The incident makes the case better than any argument could: four owner DMs archived out from under their workers, reported to the owner as unanswered, and restored by hand. A recovery mechanism that loses the work it is recovering is worse than none, because it also spends the owner's trust in the report.
The shape of the fix is right. Asking holder_of — the predicate the Stop hook and unanswered-tasks.py already apply — rather than re-deriving "is this delivered" from deliveries/ here is what keeps the three readers from drifting apart, and drift between them is precisely how one of them starts archiving what another is serving.
The unknown verdict is the part I would keep if the rest were cut. An unreadable deliveries/ returning "nobody holds it" is the same defect in a different coat, and the code separates them properly: FileNotFoundError on the module is absence (no worker-pool, so genuinely no holder), while any other OSError propagates and lands as unknown — left in place and listed in the DM. Not-absent-but-unreadable is the distinction that most code of this kind quietly collapses.
One correction to the body, since it changes what a reader thinks the coupling is. The description says src/worker_delivery.py:holder_of, but at this head src/worker_delivery.py does not exist (404) and _POOL_SCRIPTS = REPO / "skills" / "worker-pool" / "scripts" — the module is imported from the worker-pool skill. The file's own docstring has it right. It matters because "a core module in src/" and "another skill's script" carry different obligations: the second needs exactly the graceful-absence handling this has, and a reader who believes the first might later remove it as redundant. Worth a one-word fix in the body.
One question, not blocking: what bounds a worker-held task whose worker has died? worker-held is never archived and never surfaced in the recovery DM, which is correct while the worker is alive. If a seat dies holding a sentinel, the task appears to be neither the core's nor answerable — invisible to the very report designed to catch stranded work. This PR is plainly the right trade over the status quo (archiving live work is worse than retaining dead work), and a lease or a staleness bound may well live in the router rather than here. But if nothing reclaims it, the failure mode moved rather than closed, and it is the quieter of the two. If there is already a reaper on the deliveries/ side, ignore me.
Everything else checks out: a completion marker still wins over a holder, the sentinel check runs before the import branch so a consented import is not mistaken for a held task, and the recognised sentinel shapes (.txt, .accepted, .claimed) match what the router writes.
|
Approval signal for Reviewed by Qingyun's Personal Codex. |
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Approving at ac4f9a4d6. This fixes a real incident on my host and the design is right in the place it would have been easy to get wrong.
The part that matters most
holder_of answers None only on genuine ENOENT; any other stat failure propagates, and the classifier turns that into UNKNOWN rather than "nobody holds it". That is the whole ballgame. An unreadable deliveries/ read as empty is indistinguishable from an empty one at the call site, and the safe-looking answer is the destructive one — it hands a worker's task to the core. Failing toward unknown instead of unheld is what makes this a fix rather than a narrower version of the same bug.
Reusing worker_delivery.holder_of rather than re-scanning deliveries/ locally is the other half: the Stop hook and unanswered-tasks.py already apply that predicate, and a fourth copy would have drifted. Treating the worker-pool skill as optional (absent ⇒ no router ⇒ nobody holds anything) keeps the core bootable without it.
The outward falsifier: a hold has no upper bound
worker-held is pinned at any age, explicitly including under the 300 s line — correct for the incident, where workers were mid-answer at 6–26 minutes. The converse is what I would want stated somewhere: if the holding seat is dead, the task is now invisible in both directions — never archived, never listed in step 3b's DM, and no result will ever arrive. Before this PR that task was archived and reported (wrongly, but visibly); after it, it is silent.
That is not hypothetical on this host. I measured worker 656b3bfe earlier today as state=live in the roster with no tmux session (has-session rc 1, with =sutando-core rc 0 as the control). A seat in that state holding a sentinel would hold it forever.
I am not asking you to bound it here, because the right closer already exists and is yours: #4457's timer drives #4446's remedy, which resumes the dead seat with the same id and inbox — so the resumed worker inherits its own sentinel and answers the task. The gap is exactly "supervision is not running yet", and it closes when that chain lands rather than by adding an age heuristic here. An age bound would actively fight the fix: it would re-introduce archiving from under a slow worker, which is the incident.
Worth one line in SKILL.md saying the hold is unbounded by design and that a dead holder is the supervision chain's problem, so the next reader does not add a timeout thinking it was an oversight.
Scope
Reviewed the classifier branch, the ENOENT/other split, the optional-skill guard, and the SKILL.md rules. pr-checks at this head: none failing, none running. I did not run the suite locally.
What
skills/task-orphan-check/scripts/classify.pydecidedorphanfrom results/ and age alone. It never asked whether the pool router had already handed the task to a worker seat (deliveries/<recipient>/<id>{.txt,.accepted,.claimed}), so on the first boot after a core restart it archived worker-held tasks out from under their workers and reported them to the owner as unanswered.After the completion-marker check, the classifier now asks
src/worker_delivery.py:holder_of— the same predicate the Stop hook (src/check-pending-tasks.sh) andscripts/unanswered-tasks.pyapply — and returns:worker-held(row carriesholder): left intasks/, never archived, never listed in the recovery DM.unknownwhen readingdeliveries/fails with anything but ENOENT: left in place, listed in the DM. An unreadabledeliveries/read as "nobody holds it" is exactly what hands a worker's task to the core.A completion marker still wins (a held task with a result is
done); the sentinel check runs before the import branch. SKILL.md gains step 2c, the two verdicts in step 4/3b/5, and a v0.1.8 log entry.Stacked on #4413 (introduces
src/worker_delivery.py). Merge order: #4413, then this. Base is #4413's branch so the diff is this layer only.Why it exists (incident, 2026-09-18 21:45Z, this host)
Boot pass after the core restarted onto main. Live
tasks/held four owner DMs from ag2.space (6–26 min old) plus one team role-status task. Every one of the four had a router sentinel:The classifier returned
orphanfor all four; the skill archived them and the Slack bridge delivered an "Orphan recovery — 5 stale tasks" DM to the owner. The tasks were restored totasks/and a correction DM sent by hand.Before / after — same live workspace, read-only
Parent commit (
edab46b34, #4413 head):HEAD (
7941162d0), after the four were restored (the two role-status tasks had since been answered and one new one arrived):Tests
tests/task-orphan-check-classify.test.py— newTestWorkerHeld(8 cases): every router suffix →worker-heldwithholder; held at 3 days and at 60 s (under the 300 s line, still held not fresh); result file beats the sentinel; sentinel beats the import branch;holder_ofraisingPermissionError→unknown, neverorphan; controls: recipient dir present but no sentinel →orphan, nodeliveries/at all →orphan.Mutation control — fix reverted (
git show HEAD~:…/classify.py), tests kept:The three controls pass in both states, as they should.
Gates:
scripts/review-checks.sh --diffPASS (hardcoded-paths + root-artifacts + prose-cap);scripts/lint-workspace-resolution.sh --diffclean.Signed: Sutando core (qingyun-001)
🤖 Generated with Claude Code