Skip to content

fix(task-notifier): drop a queue head whose task file was archived without a result - #5200

Merged
sonichi merged 10 commits into
mainfrom
fix/notifier-drop-archived-head
Oct 21, 2026
Merged

sonichi merged 10 commits into
mainfrom
fix/notifier-drop-archived-head

Conversation

@sonichi

@sonichi sonichi commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner

Closes #4563. P1 id: rmummbsx5i56il242.

What changed, and why?

process_announced_queue() in both standby notifiers only removed the queue head once has_result found a result. If the task file was archived out of the tasks dir and no result could be found, the head never resolved. It was retried on every poll (~30s), typed into the core (each Claude submit_task can wait up to COMPLETION_TIMEOUT), and every newer owner task waited behind it.

The decision lives in one place, head-abandoned in src/delivery/task_dispatch.py (the head's payload, task_payload "$filename", is gone and no result exists). Both src/agent/claude/cli/task-notifier.sh and src/agent/codex/cli/task-notifier.sh call it after the healthy/idle wait and before submit_task, so a file archived during that wait is never typed. A dropped head is logged (dropped <file> from the queue: its task file is gone and no result was found; the log line is the only visible signal) and the loop continues to the next task.

On a drop each notifier also finishes what a delivered head would have finished:

  • Claude: clears the head's in-flight marker (inflight-clear), as has_result does. A marker left by an earlier attempt that timed out would otherwise linger.
  • Codex: calls mark_worker_stage "$filename" done, as every other terminal branch of that script does.

The composer-blocked retry path (#4677) is the same loop, so the one check covers it.

Files to look at first

  • src/delivery/task_dispatch.py: head_abandoned and its CLI entry
  • process_announced_queue() in both task-notifier.sh scripts (+7 lines each)
  • tests/task-notifier-abandoned-head.test.py: helper contract, plus the shipped queue functions of both notifiers (drop untyped, next task proceeds, no stale in-flight marker, Codex stage done)
  • tests/claude-task-notifier-archived-head.test.py: real main loop on the fake-tmux harness

What user behavior / bug does this prove?

The loop test queues task-gone.txt while the composer is busy, archives it (no result), queues task-next.txt, then frees the composer. task-next.txt must be typed; task-gone.txt must never be typed and the drop must be logged. The shipped-function test also asserts that the dropped head leaves no in-flight marker (Claude) and is marked done (Codex). With the two one-line additions removed, that assertion fails for both runtimes (mutation-checked).

Tests

Run on the rebased head plus the new commit; results are in the PR comments / CI. scripts/review-checks.sh on the diff against main: PASS.

Live round trip

Not run. A live end-to-end round trip against a real core is planned as a separate witness.

Edge cases / non-happy paths

  • A head that has a result still takes the existing has_result path, which runs first.
  • An unhealthy or busy core still returns without dropping, because the check sits after the wait.
  • Resolver-backed (worker deliveries) heads are covered: task_payload reads the stored payload path.
  • A file archived while submit_task is already in flight can still be typed once; the next pass then drops the head.
  • No prompts or typing behavior changed.

Migrations / config / rollback risks

None. No new config. Reverting restores the old behavior.

Known gaps / follow-up

🤖 Generated with Claude Code

@sonichi
sonichi requested a review from qingyun-wu as a code owner October 7, 2026 14:13
@sonichi

sonichi commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

CI shard "python standalone tests (6/7)" failed on tests/watch-tasks-stream-readiness-window-unreadable-config.test.py, which is not in this PR's changed files (only src/agent/claude/cli/task-notifier.sh, tests/claude-task-notifier-archived-head.test.py, tests/task-notifier-resolved-announcement.test.py are). Re-ran it directly against this same tree: PASS, all 13 subtests green, no flakes. Pre-existing CI-runner flake, not a regression from this diff; a rerun should clear it.

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 93%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

  • src/delivery/task_dispatch.py (100%)

Summary

  • Total: 7 lines
  • Missing: 0 lines
  • Coverage: 100%

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at head 82f6e8a.

Verdict: code looks correct and the test is a real reproduction; not approving yet. One thing changes it: a live round trip (below). No code blockers.

What I checked:

  • Bug exists on main: at parent 95fbd56 process_announced_queue only removes the head on has_result, so an archived head with no result is retried forever.
  • Test discriminates: claude-task-notifier-archived-head.test.py at this head Ran 1 test ... OK (21s); the same test against the parent notifier FAILED (failures=1). It drives the real notifier main loop and the real watcher/fswatch, with only tmux faked.
  • Placement is right: the existence check sits after wait_for_core_healthy, so an unhealthy core never drops anything, and a file archived during the healthy wait is not typed. task_payload covers resolver-backed payloads as well as $TASKS_DIR. Removal uses the same paired rm -f as the other two sites, so queue marker and payload cannot diverge.
  • The changed assertion in task-notifier-resolved-announcement.test.py is a structural pin (every queue-marker removal also removes its payload), which REVIEW.md lesson 14 allows; it would fire on a bare rm -f "$queue_dir/$filename". That suite passes locally (13 tests).
  • review-checks.sh on the merge-base diff: PASS. New comment is 2 lines, no references.
  • CI: all required checks green at this head. The earlier red was tests/watch-tasks-stream-readiness-window-unreadable-config.test.py in shard 6/7, a file this PR does not touch; the rerun passed.

Gate (REVIEW.md lesson 15): this is the standby delivery loop, and the new branch deletes queue state. Worst case is a live owner task silently dropped if task_payload ever names a path that is absent while the task is still pending. Before approval I'd like one real round trip on a restarted notifier: a task archived without a result is logged as dropped, and the next task is then delivered and answered.

Non-blocking:

  1. The same head-of-line bug remains in src/agent/codex/cli/task-notifier.sh, and the two notifiers now diverge on this policy. Please file the follow-up issue and link it here; better still, move the "can this head still be delivered" decision into the shared task_dispatch.py both notifiers already call, so it cannot drift again.
  2. The drop is only a stderr log line. A task that disappears without a result is unusual enough that a counter or a line in the notifier's health output would make it visible.

@sonichi

sonichi commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

Addressed the Codex-notifier nit: the abandoned-head rule now lives in src/delivery/task_dispatch.py (head-abandoned), which both the Claude and Codex notifiers call after their core wait. The bash copy is removed, so the rule can't drift between them. New tests/task-notifier-abandoned-head.test.py (7 tests) runs both notifiers' real queue loops: on origin/main both type the archived task (failures=4, errors=4); at 6222cd9 all 7 pass. The notifier and dispatch suites pass.

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head 30829f97. The code is correct and I found no blockers. I am not marking it ready because it has no substantive expert approval at this head. qingyun-wu's APPROVE has an empty body, and bassilkhilo-ag2's review is a COMMENT on 82f6e8af, from before the refactor into task_dispatch.py. One substantive expert approval at this head would make it ready.

What I ran (fresh worktree, env -i):

  • task-notifier-abandoned-head 8 OK, claude-task-notifier-archived-head 1 OK, task-notifier-resolved-announcement 13 OK, task-dispatch-contract 104 OK, claude-task-notifier 17 OK.
  • Failure mode: I removed the head-abandoned check from both notifiers. claude-task-notifier-archived-head fails ("the task behind an archived head was never delivered"), and the shipped-function test fails for both Claude and Codex. With the files restored, all pass.
  • The tests run each notifier's real process_announced_queue, queue_head, has_result and task_payload, with only the pane calls stubbed. The source checks are delegation pins (REVIEW.md lesson 14, second exception).
  • bassilkhilo-ag2's main non-blocking ask is addressed: the decision now lives in task_dispatch.head_abandoned and covers the Codex notifier too.
  • scripts/review-checks.sh --diff: PASS.

Gates: CI is green (32 success), license/cla is success, the PR is mergeable and has no unresolved threads.

Still owed (named, not blocking): a live round trip on a restarted notifier, where an archived head is logged as dropped and the next task is delivered and answered.

Nits:

  • On the Claude drop path, rm -f runs without inflight-clear, so an inflight marker for a dropped head that was already submitted stays behind. On the Codex drop path, mark_worker_stage is not called. Both are harmless leftovers keyed to a filename that will not return. Clean them up if they are easy.
  • bassilkhilo-ag2's second note still applies: a drop is visible only as a stderr line.

— Sutando (susans-MacBook-Pro)

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment at e3e38b3, not an approval yet: one of this PR's own tests fails deterministically here after the main merge, and I can't tell from CI whether it's environment or a real interaction.

The content is unchanged by the merge. The only commit after 30829f9 is the 2026-10-15 merge of main, and the PR's net diff against its merge base is byte-identical before and after it. Susan's review at 30829f9 (code correct, mutation-checked) still describes the change.

But tests/claude-task-notifier-archived-head.test.py fails 3 of 3 runs at this head on this host:

FAIL: test_an_archived_head_is_dropped_and_the_next_task_proceeds
AssertionError: 'dropped task-gone.txt from the queue' not found in
'task-notifier: standby armed for …/workspace/tasks (standby watcher pid …)
 watch-tasks-stream: role=standby inbox=…/workspace/tasks pid=…
 task-notifier: delivering task-next.txt as the standby: no session-role watcher holds …/workspace/tasks'
 : the drop must be logged

The notifier now takes main's standby-delivery path (no session-role watcher holds the test's temporary inbox) and delivers the next task without passing through the archived-head drop, so the drop is never logged. That path arrived with the main merge, which is why the test passed at 30829f9.

Not established: CI reports green at this head, so either CI doesn't run this file, or the standby path isn't taken there. Could you check which? If the standby path really bypasses the drop, the fix belongs in whichever function now picks the head, so an archived head is dropped on both paths, and the test should pin that.

Also run here: task-notifier-abandoned-head, task-notifier-resolved-announcement and task-dispatch-contract pass. claude-task-notifier errors at this head but is flaky on this host on main too (it passed, then errored, on consecutive runs at main), so I'm not counting it.

@sonichi

sonichi commented Oct 16, 2026

Copy link
Copy Markdown
Owner Author

@qingyun-wu thanks for the clear failure output. This is a timing race in the test, not an interaction with main. Fixed in 8059a24 (test-only).

Root cause. The "delivering … as the standby" line isn't a new path. task-notifier.sh is byte-identical across the main merge (git diff 30829f97 e3e38b31 -- src/agent/claude/cli/task-notifier.sh is empty), and submit_task_grown logs that line on every delivery. Your stderr has no line for task-gone.txt at all, so it was never queued. The test wrote task-gone.txt, slept a fixed 1.5s, then unlinked it. When the standby watcher announces slower than 1.5s, the file is gone by the time the event is handled. handle_event only dispatches [ -f "$path" ], so it is skipped and there is nothing to drop. CI (Linux, inotify) runs it and passes in 22s, and so does my host. A slower announce on a loaded host loses the race. The ambient thing is announce latency, not an env var or state file. env -i and full-env runs behave the same.

Repro. I put a shim fswatch on PATH that delays each event line by 3s. At e3e38b3 it reproduces your output exactly:

AssertionError: 'dropped task-gone.txt from the queue' not found in 'task-notifier: standby armed for …
watch-tasks-stream: role=standby inbox=… pid=72343
task-notifier: delivering task-next.txt as the standby: no session-role watcher holds …' : the drop must be logged
FAILED (failures=1)

Fix. The test now waits on the notifier instead of a clock. Before it unlinks task-gone.txt, it waits for the notifier's composer not empty for task-gone.txt attempt. Before it frees the composer, it waits for the same line for task-next.txt. The loop is serial, so the second line means the gone head was already settled. This also replaces the 4.5s sleep.

After (8059a24):

  • 3s-delayed fswatch shim: OK ×3 (38–48s)
  • env -i scrubbed: OK ×3 (~16s). Ambient env: OK
  • Mutation: with the head-abandoned check disabled in the Claude notifier, it fails with task-next.txt was never queued and attempted. So it still discriminates.
  • task-notifier-abandoned-head 8 OK, task-notifier-resolved-announcement 13 OK, task-dispatch-contract 104 OK
  • scripts/review-checks.sh --diff: PASS

Not done here: the live round trip on a restarted notifier is still owed.

@sonichi

sonichi commented Oct 16, 2026

Copy link
Copy Markdown
Owner Author

@qingyun-wu thanks for rerunning. 8059a24 still assumed the standby's fswatch was subscribed once it showed up in ps. On a loaded host it isn't, so task-gone.txt raised no event and the 30s wait ran out with nothing done, which matches your empty send-keys and stderr. Fixed in a022d21 (test only).

Readiness. A standby watcher has no subscription signal. The round-trip probe in watch-tasks-stream.sh runs only for --role session. The pid sentinel is stamped after launch_fswatch, which only checks that the process is alive, so waiting on it would not have helped. The test now gets readiness from things that do hold:

  • task-gone.txt is written before the notifier starts. The standby runs startup_sweep before it subscribes, so the head is announced without any fswatch event.
  • After the unlink, task-next.txt is rewritten every 2s until the notifier logs its busy-composer attempt. This is the same probe-rewrite the session watcher uses. A re-announce is a no-op: enqueue_announced_task returns early when the task is already queued.
  • Every wait polls up to 120s and returns as soon as its condition holds. The ps-based fswatch-attach wait is gone.

Repro. The shim fswatch on PATH sleeps 35s before starting the real fswatch, then delays each event by 3s.

Before, at 8059a24 with the shim, 3 runs:

AssertionError: False is not true : task-gone.txt was never queued and attempted
Ran 1 test in 30.581s / 31.386s / 31.250s
FAILED (failures=1)  x3

After, at a022d21:

  • Shim: OK x3 (53.5s, 53.3s, 53.4s)
  • env -i without the shim: OK x3 (15.7s, 14.7s, 20.1s). Ambient env: OK (15.7s)
  • Mutation: head-abandoned check disabled in the Claude notifier, with the shim: task-next.txt was never queued and attempted, FAILED (failures=1). So it still discriminates.
  • scripts/review-checks.sh --diff: PASS

Not covered: the live round trip on a restarted notifier is still owed.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at a022d21 (code owner). This is a substantive approval at the current head, replacing my COMMENT at e3e38b3.

  • The PR's code is unchanged and right. Every commit since 30829f9 is either the main merge (the PR's net diff is byte-identical across it) or a test-only change. Susan's review at 30829f9 still describes the code: task_dispatch.head_abandoned is the one decision both notifiers use, and disabling it fails the tests.
  • The test is now much more robust. It writes task-gone.txt before the notifier starts (so the standby's startup scan sees it), rewrites task-next.txt until an attempt is logged, and polls each condition for up to 120 s.

Measured on this host at a022d21: 9 of 11 runs pass. Both failures are the same shape: the standby is armed, but task-next.txt is never attempted within the full 120 s, with empty send-keys. That's an event the watcher never received rather than a timing race, and it matches this macOS host's fswatch dropping events under load (the fault #5170's line relay addressed). So I don't read it as a defect in this PR, but the test will still flake occasionally on a loaded Mac until fswatch delivery is fixed.

CI is green (34 pass).

@sonichi
sonichi enabled auto-merge (squash) October 16, 2026 22:34

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST_CHANGES at a022d21. This is for REVIEW.md §15 only; the code is correct.

It replaces this login's APPROVE of 2026-10-16. That approval rested on harness tests alone, and #5200 changes a live intake path: both notifiers' queue heads. §15 doesn't allow approving such a path without a post-restart witness, and Bassil and Susan both list the witness as still owed.

Code: correct, no blockers.

  • One owner for the policy: the drop decision is a single function, src/delivery/task_dispatch.py:137-144 (head_abandoned). The Claude and Codex notifiers only call it, after the core wait (claude/cli/task-notifier.sh:962-967, codex/cli/task-notifier.sh:522-527).
  • Ordering: has_result runs first, and head_abandoned re-checks for a result, live or archived.
  • Nothing real is lost: dropping removes only the notifier's queue marker. A late result still goes out through the results/ path.
  • Fails safe: a helper failure means "keep", which is the old behaviour.

Verified in a fresh worktree (offline env -i):

  • at this head: task-notifier-abandoned-head 8, task-notifier-resolved-announcement 13, task-dispatch-contract 104 and claude-task-notifier-archived-head 1 all pass;
  • src rolled back to the merge base: the tests fail;
  • mutations: removing the payload check (5 failures), the result check, or the Codex call each fails a test.
  • It's 99 commits behind main but merges cleanly, and the tests pass on the merged tree. Main has no competing fix.

Nits (Susan raised them too):

  • On the Claude side, a head that was already typed and is then dropped leaves its in-flight marker behind.
  • On the Codex side, the drop skips mark_worker_stage, so a stale pending stage marker stays.

Still needed for §15: on a restarted notifier, an archived-without-result head is logged as dropped, then the next owner task is delivered and answered once.

Reviewed at a022d21

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST_CHANGES at a022d21. I found no logic blockers.

Verified

  • The bug is still on main. In both notifiers, process_announced_queue drops the head only on has_result. Both now delegate to task_dispatch.head_abandoned.

  • The tests catch it:

    • Against the base notifiers, the shipped-queue test fails for both runtimes, because the archived head gets typed.
    • The e2e test fails at base with "task-next.txt was never queued and attempted".
    • At this head everything passes.
  • CI is green at this sha.

  • The drop scope is right. These heads are all kept:

    • claimed heads (CLAIMS_DIR), with the task file still present;
    • worker-held heads (deliveries/ sentinel);
    • heads whose result is archived under results/archive/YYYY-MM/ or epoch-suffixed (via iter_result_candidates);
    • heads whose payload is still live.

    Any non-zero exit from the helper also keeps the head.

Needed before approval

  1. Merge main. It now conflicts: CONFLICTING/DIRTY since #5792 landed in src/delivery/task_dispatch.py's _USAGE block. Keeping both sides resolves it, and on that merge these suites pass: abandoned-head (8), resolved-announcement (13), dispatch-contract (120), and the archived-head e2e.
  2. REVIEW.md §15. I need one live round trip on a restarted notifier: a task archived without a result is logged as dropped, and then the next task is delivered and answered. This has been open since the 10-07 review, and it matches Qingyun's current hold.

Non-blocking

  • Claude drop branch (src/agent/claude/cli/task-notifier.sh:963). Please check that the in-flight marker is cleared when a head is dropped. If a head was typed on an earlier pass and then archived, it could leave task-notifier-inflight/<file> behind. A later file restored under the same name would then be treated as already submitted. I traced only part of this, so please confirm.
  • Codex drop branch (src/agent/codex/cli/task-notifier.sh:~523). Consider mark_worker_stage "$filename" done, as the other removal sites do.
  • PR body. It says the Codex notifier is unchanged and describes a bash check, but this head changes Codex and the rule now lives in task_dispatch.py.

Review by Rui's Claude core (agent-assisted).

sonichi and others added 6 commits October 20, 2026 22:54
…thout a result

process_announced_queue() only dropped its head once has_result found a
result, so a task archived out of tasks/ without one stayed at the head
forever: retried every poll, typed into the core, and every newer task
waited behind it. Drop the head when its task file is gone, checked after
the healthy wait and before submit_task, and log one line.

Closes #4563

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d", not the site count

test_the_payload_record_goes_with_its_queue_marker counted exactly 2 paired
`rm -f "$queue_dir/$filename" "$PAYLOAD_DIR/$filename"` lines, so the third
drop site (archived head) failed it although it is paired. Assert instead that
every `rm -f "$queue_dir/` is the paired form; this also catches an unpaired
variant (e.g. `rm -f "$queue_dir/$filename" || true`) the old assertNotIn missed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tch; Codex gets the fix

The "task file gone and no result -> drop the queue head" decision was a bash
check in the Claude notifier only; the Codex notifier had the same stuck-head
bug (#4563). It is now task_dispatch.py's `head-abandoned` (head_abandoned()),
the shared owner both notifiers already call for has-result. Each notifier
calls it after its own core wait and keeps only its queue I/O.

tests/task-notifier-abandoned-head.test.py pins the helper's contract and runs
each notifier's shipped process_announced_queue (pane calls stubbed): an
archived head is dropped untyped and the next task is submitted, and both
scripts delegate the decision rather than re-deriving it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The diff-coverage gate missed task_dispatch.py's head-abandoned branch
(lines 468-471): it was only reached through a subprocess, which the gate
does not measure. Exercise _main directly for drop (0), keep (1) and both
malformed arities (2, usage on stderr).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ixed sleep

The archived-head test unlinked task-gone.txt 1.5s after writing it. When the
standby watcher announces slower than that, the file is gone before it is
queued, the watcher skips it, and there is no head to drop: the test failed on
its own precondition, not on the notifier. Wait on the notifier's busy-composer
attempt for each task instead of a clock.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…er startup latency

On a loaded host the standby's fswatch can subscribe tens of seconds after it
is visible in ps, so a task written then raises no event and the 30s wait on
the notifier expired with nothing done. The head is now written before the
notifier starts, so the standby's startup sweep announces it with no fswatch
event. The next task is rewritten until the notifier attempts it, the
round-trip a standby otherwise lacks. Every wait polls up to 120s and returns
as soon as its condition holds.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sonichi
sonichi force-pushed the fix/notifier-drop-archived-head branch from a022d21 to db3df07 Compare October 21, 2026 05:55
…stage when a head is dropped

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST_CHANGES at c411a98, for REVIEW.md §15 only. The code is clear.

Resolved since a022d21

  • The merge conflict is gone. The PR shows MERGEABLE against current main, and db3df07 is rebased on it.
  • The Claude drop branch (src/agent/claude/cli/task-notifier.sh:965) now calls inflight-clear, so a head that timed out and was then archived no longer leaves a stale in-flight marker.
  • The Codex drop branch (src/agent/codex/cli/task-notifier.sh:547) now calls mark_worker_stage "$filename" done, the same as its other terminal branches.
  • tests/task-notifier-abandoned-head.test.py is extended to cover both. The author reports it fails for both runtimes without these lines.

The only change since db3df07 is those two source lines plus the test.

Still needed: one live round trip on a restarted notifier at this head (§15). A task archived without a result should be logged as dropped, and the next task should then be delivered and answered. You've said Pro's install will run it.

I'm not repeating the earlier findings, which are unchanged. Some CI checks were still pending when I reviewed.

Before I approve, I'll also have my owner's Codex core verify Codex-core compatibility at the witnessed head.

Review by Rui's Claude core (agent-assisted).

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST_CHANGES at c411a98, for REVIEW.md §15 only. The code is clear.

Since a022d21:

  • The rebase onto main is clean, and task_dispatch.py is unchanged.
  • Claude drop branch: now runs task_dispatch.py inflight-clear, the same call the other terminal branches use, so a head that timed out and was then archived no longer leaves a stale in-flight marker.
  • Codex drop branch: now calls mark_worker_stage "$filename" done, like every other terminal branch.
  • Test: test_a_dropped_head_leaves_no_inflight_marker_and_codex_finishes_its_stage pins both.

Verified at this head: task-notifier-abandoned-head.test.py passes. Each new line was mutated on its own (the Codex one targeted at the drop branch, since the same three-line pattern appears in 6 places), and each fails only its own runtime's subtest.

Still needed for §15 (Pro's install is set to run it): on a restarted notifier, an archived-without-result head is logged as dropped, then the next owner task is delivered and answered once.

Reviewed at c411a98

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at c411a98. No blocking code findings; filing as COMMENT because the approve bar is not met at this head.

The new head adds the two nits from the earlier reviews, one line each, plus a test extension:

  • src/agent/claude/cli/task-notifier.sh: the abandoned-head drop now clears the in-flight marker (inflight-clear), so a head that timed out and was then archived no longer keeps a stale marker.
  • src/agent/codex/cli/task-notifier.sh:547: the drop branch now calls mark_worker_stage "$filename" done, like every other terminal branch.
  • tests/task-notifier-abandoned-head.test.py: 9 tests, OK at this head. Removing either new line fails it (the Claude one via the in-flight assertion; the Codex one via test_a_dropped_head_leaves_no_inflight_marker_and_codex_finishes_its_stage).

The core fix, reviewed at a022d21 with product code unchanged since apart from the two lines above: head_abandoned (payload gone and no ready result, live or archived) runs after the core idle wait and before submit, in both notifiers. A head with a live or archived result is never dropped, and a drop removes only the queue marker and the payload pointer, never an answer. Mutations of the keep/drop decision each failed a test at that review. Non-blocking, unchanged: a drop is visible only as a notifier log line.

CI: every check passes (Windows skipped), license/cla success. No conflict with main.

Approval bar: not met. There are no approvals at this head; qingyun-wu and john-the-dev hold CHANGES_REQUESTED here for the REVIEW.md 15 live post-restart round trip only. For my part the live round trip is named, not blocking: once one expert approves this head, I will approve.

— Sutando (susans-MacBook-Pro)

@sonichi

sonichi commented Oct 21, 2026

Copy link
Copy Markdown
Owner Author

REVIEW.md §15 live witness at c411a98

Owner's go. A scratch workspace and a private tmux socket. The notifier is this head's src/agent/claude/cli/task-notifier.sh (staged with git archive c411a983, identical to git show). The core pane is a scratch Claude with a minimal "answer the task file, write the result" instruction. No live core, live socket or live workspace was used. The test identity's relay was pointed at the scratch inbox only for the owner turn. Paths are by role.

1. In-run: an archived head is dropped, not re-announced, not blocking

standby armed for <scratch>/tasks (standby watcher pid …)
# task A written while the pane was not a ready Claude → announced, queued
mv tasks/task-w5200-a-….txt tasks/archive/      # no result for A
# task B written behind A; then the pane becomes a ready Claude
06:52:11Z dropped task-w5200-a-….txt from the queue: its task file is gone and no result was found
06:52:13Z composer not empty for task-w5200-b-….txt; leaving it queued (failing closed, not typing over a draft)
06:52:43Z delivering task-w5200-b-….txt as the standby …
  • capture-pane -J -S -5000: A appears 0 times; the "Sutando task ready: task-w5200-b…" prompt appears 1 time. results/ holds only B ("BRAVO"), one file.
  • In-flight marker (new at this head): the drop branch now runs task_dispatch.py inflight-clear. A real marker cannot exist for A, because the notifier writes one only after typing a task, and A is never typed. To exercise the clear, a marker for A was seeded with the production writer (task_dispatch.py inflight-mark …, labelled synthetic in the log). It was present before the drop, and the in-flight dir had 0 entries after it.

2. A real notifier restart

The queue lives in a per-run mktemp dir that is removed on exit, so a queued head cannot survive a restart. That is why the drop is shown in-run.

kill -TERM <notifier>          # verified by ps; its standby watcher and fswatch exited with it
standby armed for <scratch>/tasks (standby watcher pid …)        # restarted from c411a983
watcher startup sweep: already answered, not dispatching again: task-w5200-b-….txt

After 40 s there were no new lines, and the new queue was empty. In the pane, A still appears 0 times and the B prompt 1 time.

3. A real owner turn

The owner posted "witness 5200 task c: reply PONG and the current UTC time" in the test room. The test relay wrote it to the scratch inbox, the notifier typed it into the scratch core, and the core answered:

delivering task-de1ae5d7a32f19db73.txt as the standby …
result: "PONG — current UTC time: … (witness 5200, task c)"
[remote-gateway-bridge] result task-de1ae5d7a32f19db73 delivered …      # exactly 1 line

That is one prompt in the pane and one delivery. A and B were re-typed 0 times.

Witness setup note. On the first attempt the scratch Claude was running in accept-edits mode. The pane gate correctly does not read that footer as idle: it reported "unknown (no idle affordance)", the notifier logged "core did not become healthy within 300s", and the task stayed queued and was never typed. I relaunched the scratch Claude in bypass mode, as the live core runs, with only the Read and Write tools, and started the notifier again. Its startup sweep announced the still-unanswered owner task, which was delivered as above. This was setup on my side, not the PR.

Seen outside this PR

  • Pane gate during Claude's startup dialogs. In part 1 the gate read Claude's startup screen (folder-trust, then Chrome dialogs) as healthy. The B attempt failed closed ("composer not empty … not typing over a draft"), retried 30 s later once the dialogs were dismissed, and delivered once. Nothing was typed into a dialog, but healthy-during-dialog looks wrong.
  • Leftover temp dir on exit. The notifier's exit cleanup leaves the old run's mktemp dir behind with an empty payload/ subdir: cleanup_notifier removes queue/ and events but not payload/, so its rmdir fails.

Teardown. kill -9 of the scratch Claude (verified), TERM to the notifier (verified), and the private tmux server killed through its own socket. No scratch process is left, and the test relay is restored. The live session-state file was untouched by this witness: its only change happened hours before the teardown, from the live core.

Result: PASS (parts 1, 2, 3).

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE at c411a98. This replaces my §15-only hold.

The live witness (#5200 (comment)) shows this head's notifier with a scratch core:

  1. The drop. An archived-without-result head A is logged as dropped and typed 0 times. Task B behind it is delivered and answered once. A seeded in-flight marker for A is cleared by the new drop branch.
  2. A real notifier restart, by verified PID. The startup sweep sees B as already answered, and nothing is re-announced or re-typed.
  3. An owner task posted after the restart, delivered and answered exactly once.

The drop is shown before the restart rather than after it. The queue lives in a per-run mktemp dir that's removed on exit, so a queued head can't survive a restart; that's sound. The post-restart owner turn is the round trip §15 asks for.

The code was already clear (my c411a98 REQUEST_CHANGES has the mutation results).

Follow-ups from the witness, outside this PR:

  • The pane gate reads Claude's startup dialogs (folder-trust, Chrome) as healthy. It failed closed here, but it shouldn't call that state healthy.
  • cleanup_notifier leaves the run's temp dir behind with an empty payload/.

Reviewed at c411a98

@john-the-dev
john-the-dev dismissed their stale review October 21, 2026 15:09

§15 witness posted at c411a98 (issuecomment-6319339848): drop logged, B delivered once, restart re-announced nothing, owner turn answered once. My only blocking item is met; approval follows once Rui's Codex core verifies this head.

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST_CHANGES at exact head c411a9834cc7309dc423e5cafdca67ded4a22289.

The new §15 live witness is sufficient: it exercises the exact-head Claude notifier through drop, queue progression, a PID-level restart, no replay, and one fresh owner round trip. That resolves the former live-path hold. I found a separate blocker in the newly activated Codex-worker cleanup path.

Blocker — an abandoned task is recorded as a published completion.

src/agent/codex/cli/task-notifier.sh:545-548 reaches this branch only after head-abandoned establishes that the payload is gone and no ready result exists, but it then calls:

mark_worker_stage "$filename" done

That contradicts the writer contract in src/delivery/worker-stage.sh:28-30 (a published result already exists by done). In the production writer, done calls mark_done(..., published=True) and creates a durable .flag; residue() treats that flag alone as finished, because real completed results may already have been drained. abandon is the no-result transition: it clears the pending hold and retires the worker sentinel without creating completion evidence. The analogous no-result branch in src/watch-tasks-stream.sh:759-763 already uses abandon.

I reproduced the state transition with the exact-head production writer, an accepted worker sentinel, an archived payload, and no result:

stage=done + prune-spent
{"retired":["task-a"],"stale":[],"kept":[]}
residue=finished
state/workers/worker-3/done/task-a.flag

stage=abandon
residue=clean
(no done flag or worker sentinel)

This leaves false durable completion/attribution state and can suppress recovery of the same task ID even though no result was ever published. The current regression does not exercise the writer: tests/task-notifier-abandoned-head.test.py:105-108 stubs mark_worker_stage, then lines 137-148 explicitly assert the incorrect literal done argument.

Please use the production abandon transition for this branch, handle an abandon write failure according to the fail-closed contract, and cover the real writer state: no .flag, no pending record/sentinel, and residue == clean.

Verification: all current CI and CLA checks are green, there are no unresolved threads, and the head is mergeable. On a clean synthetic merge with current origin/main, review checks and git diff --check passed; focused tests also passed (9 abandoned-head, 120 dispatch-contract, 13 resolved-announcement, 1 archived-head loop). Those tests do not cover the production state transition above.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@qingyun-wu

Copy link
Copy Markdown
Collaborator

Re-review at a2afd3b: the code is clear, but I'm holding approval on one §15 point.

Code. The Codex drop branch now records abandon through the real writer (worker-stage.sh → pool_delivery.py mark-done --stage abandon, which withdraws a pending hold). A failed write keeps the head and payload and retries on the next idle cycle. On the live core (no SUTANDO_INSTANCE_ID), write_worker_stage returns 0, so the core can't stall on this.

Verified at this head:

  • abandoned-head 11 OK; archived-head 1 OK;
  • kill control: restoring done fails both new tests (test_codex_drop_withdraws_the_hold_through_the_production_writer and test_codex_keeps_the_head_when_the_abandon_write_fails).

§15. The live witness at c411a98 ran the Claude notifier, which is unchanged at this head. This commit changes the Codex notifier's live drop branch: its stage semantics and a new keep-the-head path. That branch has never run live; at c411a98 it was a one-line done call, and I let it through on tests then, which I shouldn't have. Either of these closes it:

  1. A short Codex-notifier witness on a Codex core in a scratch workspace, the same three parts as the Claude one: an archived head dropped and its hold withdrawn, a restart with nothing re-typed, and one owner task delivered once.
  2. Or Qingyun or Chi explicitly accept test-only coverage for the Codex branch.

Non-blocking: on a worker whose writer is permanently broken, the keep-the-head retry repeats every idle cycle and blocks that worker's queue. That fails closed, but the log line repeats each cycle; consider a backoff or a single escalation.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE at a2afd3b.

  • Code: clear (see my comment at this head). The Codex drop branch records abandon through the real writer and keeps the head when that write fails. The live core is unaffected (write_worker_stage returns 0 without SUTANDO_INSTANCE_ID). Restoring done fails both new tests.
  • §15, Claude notifier (unchanged at this head): live-witnessed at c411a98 with an in-run drop, a real restart, and an owner task delivered once.
  • §15, Codex notifier drop branch: test-only coverage, accepted by Chi in his own words in the PR triage room (as @Chi:ag2.space, 2026-10-21 ~15:50Z, replying "accept" to the hold).

Not live-verified: the Codex notifier's drop and abandon on a real Codex core. A live witness is owed when a Codex-core run is next convenient.

Reviewed at a2afd3b

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST_CHANGES at exact head a2afd3b4abcbe8044ae95573a28b80aec7714847.

The production blocker from my prior review is fixed. src/agent/codex/cli/task-notifier.sh:545-553 now records abandon only after the shared no-payload/no-ready-result decision, and a failed durable write returns before the queue/payload deletion and before later work can be submitted. The production-writer test also proves the success state is clean, with no done flag, pending record, or sentinel.

One narrow blocking regression gap remains in tests/task-notifier-abandoned-head.test.py:187-193: the failed-writer test claims to prove that the head and cached payload pointer survive, but _run() creates only the PAYLOAD_DIR directory and never creates PAYLOAD_DIR/task-gone.txt. It therefore checks queue retention and no later submission, but cannot observe payload-pointer loss.

Positive mutation control at this exact head, keeping the tests unchanged:

 if head-abandoned ...; then
+  rm -f "$PAYLOAD_DIR/$filename"
   if ! mark_worker_stage "$filename" abandon; then
Ran 11 tests in 8.401s
OK
PAYLOAD_DELETE_MUTATION_SURVIVED=YES

That pointer is load-bearing for resolver-backed workers: the next retry must re-check the same payload path, not silently fall back to the core inbox path. Please seed the missing cached payload record in the failed-writer setup, assert that it remains unchanged when abandon cannot be recorded, and show the payload-deletion mutation failing. Pinning its removal on the successful-abandon path would also make the paired cleanup explicit.

The original abandon -> done kill control is good: it makes both new tests fail (finished != clean; the next task is wrongly submitted). Independent QA also seeded a real cached pointer and confirmed the current production code retains it, so this is a regression-test blocker, not a second production-code defect. No other blocker was found.

Verification at this head: abandoned-head 11/11, dispatch contract 120/120, resolved-announcement 13/13, archived-head loop 1/1; review checks and git diff --check pass; a synthetic merge with current origin/main is clean and the changed suites pass. Current CI and CLA are green, the head is mergeable, and there are no unresolved threads. The prior §15 witness remains sufficient for the earlier live-path change; I am not requesting another witness for this narrowly scoped state correction, and none should be needed for the requested test-only follow-up.

Worst case with the production fix is a permanently broken worker-state writer holding that worker's queue head and repeating the retry log. That is deliberate fail-closed behavior: later work is not typed out of order and no false completion is published.

…cached payload pointer

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE at 956f62b. It's test-only since my a2afd3b approval: git diff a2afd3b4 956f62be touches only tests/task-notifier-abandoned-head.test.py.

Verified here:

  • abandoned-head: 11 OK.
  • Kewei's mutation (rm -f "$PAYLOAD_DIR/$filename" before the abandon write) now fails test_codex_keeps_the_head_when_the_abandon_write_fails with "the held head keeps its cached payload pointer".

My a2afd3b review still applies:

  • the Claude notifier is live-witnessed;
  • the Codex branch has test-only coverage, accepted by Chi;
  • a Codex-core live witness is still owed when convenient.

Reviewed at 956f62b

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE at exact head 956f62be1d3f774f9e9e321818d9ffd02b24dea7. This replaces my prior REQUEST_CHANGES.

The follow-up is test-only relative to production head a2afd3b4. It closes the remaining regression gap:

  • the successful-abandon setup now seeds a real cached pointer to the vanished payload and proves paired cleanup removes it;
  • the failed-writer setup seeds the same pointer and proves it survives byte-identical while the queue head remains held and nothing behind it is submitted;
  • all 11 named abandoned-head tests run and pass.

Both independent mutation controls discriminate:

delete $PAYLOAD_DIR/$filename before the failed abandon write
-> test_codex_keeps_the_head_when_the_abandon_write_fails FAIL
   "the held head keeps its cached payload pointer"

restore abandon -> done
-> test_codex_drop_withdraws_the_hold_through_the_production_writer FAIL
   residue: finished != clean
-> test_codex_keeps_the_head_when_the_abandon_write_fails FAIL
   task-next.txt was submitted

Independent QA reproduced the same results in a separate clean checkout. Local verification: abandoned-head 11/11, resolved-announcement 13/13, archived-head loop 1/1; review checks and git diff --check pass; a synthetic merge with current origin/main is clean and the changed suite remains 11/11. The prior production-path review and §15 disposition still apply because this commit changes only the test file.

Verdict: the code is review-ready with no remaining code/test blocker. Worst-case behavior is unchanged: a permanently broken worker-stage writer holds that worker's queue head and retries noisily, but fails closed—later tasks are not typed out of order and no false completion is published.

External merge gate still missing: license/cla is ABSENT at this head (commit status: state=pending total=0 contexts=), not failing or pending-with-a-context. All 34 Actions checks completed green/skipped as expected, the PR is mergeable, Qingyun's approval is current, both approvers return collaborator membership 204, and there are no unresolved threads. Per CONTRIBUTING.md, close+reopen the PR and re-check the head status; do not merge until license/cla=success appears.

@sonichi sonichi closed this Oct 21, 2026
auto-merge was automatically disabled October 21, 2026 17:36

Pull request was closed

@sonichi sonichi reopened this Oct 21, 2026
@sonichi

sonichi commented Oct 21, 2026

Copy link
Copy Markdown
Owner Author

@cla-assistant check

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

@cla-assistant check

@sonichi sonichi closed this Oct 21, 2026
@sonichi sonichi reopened this Oct 21, 2026
@sonichi
sonichi merged commit 9ad5126 into main Oct 21, 2026
84 of 86 checks passed
@sonichi
sonichi deleted the fix/notifier-drop-archived-head branch October 21, 2026 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

claude task-notifier loops on already-archived tasks and blocks new owner tasks indefinitely on 'composer not empty'

6 participants