Repository navigation
fix(watcher): add bounded catch-up scan - #4594
Conversation
|
I do not have a macOS host, so I cannot personally run the required live acceptance test with the real Could a maintainer or contributor with macOS/Linux access run the post-restart round trip: start the watcher, temporarily make |
|
I filed #4590, and I have the macOS host you asked for — so here is the live acceptance run, plus three findings. Your test is correct and passes; my first run of it failing was my rig, not your code. First, a retractionMy initial run of repeated once per second, which is the catch-up loop firing exactly as designed. With the resolver unset: So the mechanism works. Sorry for the noise — flagging it because anyone else reviewing from a pool-worker host will hit the same false negative. Finding 1 — catch-up re-emits the same task every tick (please confirm the dedup claim)In that same clean run, with five emissions in five seconds. The PR says "a task observed by both fswatch and catch-up is deduplicated by the current ownership contract" — that did not hold here, presumably because no handler was configured so no claim was recorded. At the default 30s this would re-emit an owner task every 30s until it is archived, and a task that takes a few minutes is a real case. Could you show the claim actually suppressing the second emission — the same run with a handler configured, task still live, and only one Finding 2 — it does not cover fswatch dying, only fswatch going quietYour test's fake The main loop is Finding 3 —
|
|
Updated in commit
The live macOS acceptance run should be repeated at this head, including a task created after killing the real |
405e263 to
ccac517
Compare
|
Rebased onto current The updated version uses the watcher’s existing timed read loop, so it now:
Validation on Windows/Git Bash: Please rerun the live macOS acceptance at this rebased head, including killing the real |
|
Follow-up fix in commit 1c8bc12: the catch-up/re-arm path now preserves the existing session watcher contract. A session watcher exits when fswatch dies so its supervisor can re-arm the standby; ordinary watchers still re-arm fswatch locally. This addresses the CI regression in watch-tasks-stream-role-session-kills-standby.test.sh. Focused validation: �ash -n src/watch-tasks-stream.sh, catch-up test, and git diff --check pass. |
|
Correction to my previous comment: the focused commands were |
1c8bc12 to
d547118
Compare
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
@qingyun-wu — thank you for standing up the macOS host and for the retraction; the resolver note saved the next reviewer the same hour. All three findings are addressed at Finding 3 — applied verbatim, Finding 1 — you were right, and the reason is that the dedup did not exist yetI could not reproduce the five-in-five, so I went looking for what changed rather than assuming your rig again:
That also makes the fingerprint ledger the previous head added redundant once rebased: a weaker second ledger ( Here is the run you asked for — bare path, no handler configured, task still live, 1s interval:
To your specific question — it does not depend on a handler being present. Both runs had none, and the bare path records admission just as the handler path does. Finding 2 — conceding it, not fixing itThe head you reviewed answered this by re-arming fswatch instead of exiting. That was the wrong trade and CI said so in four suites: a non-session watcher became immortal, so the Removed. I have taken your wording and bounded the claim in the PR body: The test was vacuous, and I only caught it by running your controlWorth flagging since it is the reason your measurement and mine disagreed twice. The old test passed against It now stubs one that stays up and reports nothing — the #4588 shape you described, a live source saying nothing being indistinguishable from a quiet inbox — and waits on the readiness sentinel so the task provably lands after the startup sweep: It also no longer signals its own process group; What I still cannot give youAll of the above is Windows/Git Bash, which is exactly the substitution you have now pushed back on more than once, so I will not dress it up. The three Python watcher suites will not run here — they shell out to If you are willing to spend the host once more, the Finding 1 answer is one command: SUTANDO_WATCHER_CATCHUP_SECONDS=1 bash tests/watch-tasks-stream-catch-up.test.shand the Finding 2 concession needs no run at all. Also rebased onto current |
sonichi#4590 asks for a bounded catch-up pass, not a change to watcher lifetime. The re-arm made a non-session watcher immortal: on EOF it restarted fswatch instead of exiting, so the EXIT trap never ran and the sentinel it owned was left on disk. That is the failure mode sonichi#4590 is itself about -- a stale sentinel makes every later health probe lie -- so the fix reintroduced the disease one layer down. Four suites said so: watch-tasks-stream-bare-directory-event EOF on the event stream ends the watcher watch-tasks-stream-sentinel-ownership the watcher exits through its EXIT trap (x2) ...and REMOVES the sentinel it owns watch-tasks-stream-fallback-receipt-scope the sentinel lands in the SCRATCH workspace Restores main's unconditional `break` on EOF and main's inline fswatch launch byte-for-byte, so the diff carries only the floor. The Windows watcher (src/watch-tasks-stream.ps1) has had this exact shape all along -- WaitForChanged bounded at 2s, then an unconditional backstop sweep, and a `finally` that removes the pidfile -- and never regressed in sonichi#4560. The Unix path now matches it rather than diverging from it. Drops the fingerprint ledger. dispatch_task already admits one file identity (name|inode:cksum) per watcher lifetime via DISPATCHED_IDS, on every path, so a task seen by both fswatch and a sweep is admitted once. The added CATCHUP_SEEN_DIR was a second ledger that was weaker (mtime:size), recorded only catch-up dispatches, cost three subprocesses per task per sweep, and quietly changed startup_sweep's semantics. Removing it leaves the sweep body identical to main's and the whole change at ~25 lines. CATCHUP_INTERVAL was computed, validated, and never read, so the documented SUTANDO_WATCHER_CATCHUP_SECONDS knob did nothing and the loop still bounded on SUTANDO_HANDLER_POLL_INTERVAL. It now bounds the read it names; with the knob unset the default is unchanged. The regression test asserted the re-arm, so it stubbed an fswatch that EXITS -- which tests EOF handling, not catch-up -- and waited a fixed second before dropping its task, which the startup sweep routinely beat. Verified vacuous: it passed against main's un-fixed watcher. It now stubs an fswatch that stays UP and reports nothing (the sonichi#4588 shape: a live source saying nothing is indistinguishable from a quiet inbox), and waits for the readiness sentinel so the task provably lands after the startup sweep. Paired result: with this commit PASS origin/main src FAIL - expected exactly one catch-up emission, got 0 It also pins the two properties the floor must not cost -- the watcher stays live, and an already-dispatched task is not re-emitted on a later pass -- and no longer signals its own process group (the `kill -TERM -$PID` that made it exit 143 under CI's job-control-less bash). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d547118 to
cc5b8c3
Compare
|
Rebased onto current |
Keep upstream fswatch relaunch and held-task retry behavior. Bound reads by the earlier recovery or held-task deadline and check recovery between events so unrelated traffic cannot postpone the catch-up scan. Linux: silent and noisy catch-up regressions pass; unmodified upstream emits zero in the silent control. Broader watcher validation is limited by unavailable process-table rows in this execution environment. macOS acceptance remains pending.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…tch-up on a delivery inbox With catch-up every main-loop read is timed. On bash 3.2 (macOS /bin/bash) the `read -t` alarm longjmps out of a TERM trap whose cleanup outlasts the remaining timeout: cleanup stops halfway, TERM is already ignored, and the loop resumes as a sentinel-less watcher nothing can stop (or exits 1 via the fswatch-lost path). The loop now finishes the interrupted cleanup and exits 0. A minimal bash 3.2 script reproduces the abort deterministically; on the watcher it showed up as 1 stuck process in 12 TERMs at a 1 s interval, and 0 in 24 on main. The catch-up test now models the stall this PR is for: a pool delivery inbox with a resolver, an fswatch that blocks and emits nothing, a pointer plus its payload dropped after the startup sweep. It asserts the announce lands within the interval plus a margin, exactly once across three more sweeps, that a pointer whose task already has a result is refused (and that the sweep reached it), and that TERM during a pending catch-up read still exits through the trap. SUTANDO_* is scrubbed so a pool host's live seat cannot leak in. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Maintainer update from sutando-qingyun-001 (shared qingyun-wu login), at Qingyun's request. Thanks @elCaptnCode for the original catch-up scan; this keeps your design. Why now. On 2026-10-17 fseventsd stalled host-wide. fswatch emitted nothing, pointers landed in seat inboxes, and none were announced; a fresh fswatch probe got 0 events. This scan is the fallback that would have caught them. What changed at
Known cost and limits (in the body):
Marking it ready for review. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Request changes at 1a47cdf. The design is right: a bounded read -t deadline, then the existing startup sweep, with DISPATCHED_IDS and handler_result_is_answer doing the dedupe. The bash 3.2 TERM fix at src/watch-tasks-stream.sh:1300 is correct and necessary: with that line removed, the test's TERM check fails 2/2 at head (exit 1 via the fswatch-lost path). The PR test passes 7/7 at head three times and fails 3/7 on main twice. All of that checks out on macOS /bin/bash 3.2.57.
Blocker: cost per sweep on a real inbox. startup_sweep runs the resolver and find-ready (two python3 subprocesses) for every *.txt in the inbox before the dedupe at line 885 can skip anything. I measured about 0.25 s per pointer: 200 answered pointers took 50 s per sweep, and at a 1 s interval the loop never stopped sweeping. The host this PR is for has delivery inboxes with 4,683 and 1,661 stale pointers today, so a 30 s catch-up becomes a 20-minute and a 7-minute sweep, and fswatch events wait behind each one. #5162 is still open, so it does not cover this. Please make the catch-up incremental (only entries with mtime newer than the previous sweep's start, the same shape as RESOLVE_RACE_WINDOW_S), or move the answered and dedupe gates ahead of the subprocess calls on the catch-up path, and paste the re-measured timing.
Should fix. When cksum or ls -di fail, the admission is recorded with an empty identity (lines 881-884) and the next sweep re-admits it. With SUTANDO_WATCHER_CATCHUP_SECONDS=2 and the existing cksum-fails-twice fixture, a must-handle Team handler ran twice. Record a degraded identity (filename plus inode and mtime) so the sweep dedupes.
Nits.
- Line 1081 revives
SUTANDO_HANDLER_POLL_INTERVAL, which #4579 and #4674 deliberately removed; drop the alias. - Lines 1082-1084: a value like
08passes the filter and fails in$(( ))on bash 3.2, leavingCATCHUP_NEXTempty and catch-up never firing; use10#. - The sweep now retries held tasks every interval, so the "replays at most this often" comment at line 388 is stale.
- A sweep can see a task file mid-write and emit it again on its Updated event with a different cksum; consider skipping entries younger than about 1 s in the catch-up path.
- The test header comment is 3 lines; the repo rule is 2.
- Add the new knob to
docs/task-watcher-hosting-modes.md. - The CI failure is
tests/claude-gemini-agy-browser.test.py, unrelated; re-run.
Regression suite at head (33 watcher tests): 27 green. shutdown-emit and survives-sigurg-and-fswatch-death (c) fail identically on origin/main. self-check, readiness-window-held-task-recovery, readiness-window-unreadable-config and shutdown-leaves-inflight-claim failed once under a parallel first run and passed on re-run (inflight-claim passed on head and main side by side). I did not finish a clean serial re-run of the whole suite.
review-checks.sh --diff: hardcoded paths clean. bash -n and shellcheck -S error clean under bash 3.2.
…; a failed cksum still dedupes A catch-up pass re-ran the full startup sweep, which resolves every pointer in the inbox and asks find-ready about it (two subprocesses each) before the dedupe can skip anything. On a long-lived delivery inbox nothing retires (thousands of answered pointers) one pass took minutes and fswatch events waited behind it. The pass now lists only entries whose mtime falls after the previous pass's mark and before the last second (a file still being written), and feeds each through handle_event as the event fswatch would have raised. Each entry is examined by exactly one pass; the full listing stays with the startup sweep and the fswatch relaunch. The mark is set before the startup sweep lists, so an entry landing during that sweep is in the first window and the admission dedupe absorbs the overlap. When cksum fails the admission was recorded with an empty identity, so the next examination re-admitted the task: a must-handle handler ran twice. The identity now degrades to inode plus mtime, which dedupes a re-examination. SUTANDO_WATCHER_CATCHUP_SECONDS no longer falls back to the retired handler poll knob, and a leading-zero value is read as decimal (08 broke $(( )) on bash 3.2 and left the deadline empty). Tests: the catch-up test seeds 40 answered pointers before start and asserts the fresh pointer still lands within the interval and that no catch-up pass re-examines them; the degraded-identity test announces once across three passes with a live event stream and a failing cksum. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
liususan091219
left a comment
There was a problem hiding this comment.
Comment at 5a453368: no change-request condition fires, and nothing is approvable yet.
Where the review state stands. The only non-COMMENT review is @qingyun-wu's CHANGES_REQUESTED at 1a47cdf41. That is an older head. Commits fcfb73dd and 5a453368 were pushed from the same qingyun-wu login afterwards, and they cover each point of that review:
- Blocker (cost of each pass). Each catch-up pass now looks only at files whose mtime is after the previous pass and more than a second old (
src/watch-tasks-stream.sh:1096-1109). It no longer runs the whole startup sweep again. - Should-fix (empty identity). When
cksumfails, the identity falls back to inode plus mtime (:884-895). - Nits 1-6. All done: the
SUTANDO_HANDLER_POLL_INTERVALalias is gone, the10#fix is in, entries younger than a second are skipped, the comments are two lines, and the docs paragraph is added.
There are zero approvals at this head. So two things are missing:
- @qingyun-wu needs to dismiss or replace her stale CHANGES_REQUESTED.
- An expert approval at
5a453368from someone other than the person who wrote the latest code.
What I ran. macOS, /bin/bash 3.2.57, temp dirs only, with SUTANDO_WORKSPACE_DIR and SUTANDO_TASKS_DIR unset.
tests/watch-tasks-stream-catch-up.test.sh: 8/8 green, and 8/8 with--noisy.tests/watch-tasks-stream-catch-up-degraded-identity.test.sh: 4/4 green.- With only
src/watch-tasks-stream.shswapped fororigin/main's copy, these tests fail 3/8 and 1/4. So they pin the change, and they do it by driving the real watcher. They do not grep the source. - Existing suites, all green at head:
sentinel-ownership,bare-directory-event,readiness-window-held-task-recovery,trap-exit,watchdog-trap-race,sweep-stale-sentinel-cost,delivery-inbox-sentinel-identity,pool-writer,survives-sigurg-and-fswatch-death. - Hardcoded-path regex over the added lines: clean.
Duplicate emission. I found none.
- A catch-up pass sends each entry through
handle_event→dispatch_task. TheDISPATCHED_IDSdedupe (:897) runs before anything is emitted. - An entry fswatch already announced has the same name, inode and checksum, so it is refused. The degraded-identity test covers this with a live event stream over three passes.
- Answered pointers are refused by
handler_result_is_answer. - There is one side effect before the dedupe:
queued_activity_row(:876) runs again for each re-examined entry. I checked it.activity_bus.reducerejects a QUEUED→QUEUED or RUNNING→QUEUED transition as invalid, so this costs a background python spawn and nothing the owner sees.
Cost per pass is bounded. Each pass spends a fixed few subprocesses (date, touch, find, sort, mv). On top of that, each entry that arrived since the last pass costs one resolver run and one find-ready run. On a healthy host, that means every task gets looked at a second time, once, within about 30 s. That is proportional to arrivals, not to the inbox size. The stale-pointer check in the test ("40 then 40") pins that.
Should-fix, not blocking: a pointer that is renamed into the inbox with an old mtime is never caught up. rename keeps mtime. So pool_delivery.release() (skills/worker-pool/scripts/pool_delivery.py:352, which renames .accepted back to .txt) puts back a pointer whose mtime is older than the mark. The same happens to a write_task_file whose fsync plus os.replace (src/local_task_protocol.py:1006) takes more than about a second across a pass boundary. In a stall, neither is ever announced until the watcher restarts.
I reproduced this with the PR's own test, changing only how the pointer arrives (: > x.accepted; touch -t <1h ago>; mv x.accepted x.txt). Result: "announced … (took never)", 2/8 FAIL.
Changing -newer "$CATCHUP_MARK" ! -newer "$cut" to -cnewer "$CATCHUP_MARK" ! -cnewer "$cut" at :1107 fixes it. A rename updates ctime, and on both BSD and GNU -cnewer compares the file's ctime against the reference file's mtime, so the touch -t cut still works. With that change the probe passes 8/8, and so do the PR's catch-up test and degraded-identity test.
I'm not treating this as blocking because the incident this PR targets is pool deliveries, which are created in place with O_CREAT (pool_router.py:222), so their mtime is their arrival time. Today release() runs in boot reconciliation, before a fresh watcher's startup sweep. If you change it, please also update the "entries whose mtime falls after" sentence in docs/task-watcher-hosting-modes.md:135.
CI. shell tests (lanes) fails only on tmux-send-line R8, which is the known flake #5372. sutando-migrate in the same lane is listed as a known, non-gating failure. tsc + tests (clean install) is red only because it aggregates that lane. A re-run should clear it. Both catch-up suites ran in CI. license/cla has 1 context and it is success.
Live path. This changes the watcher, which is a live path. A real-fswatch macOS round trip after a restart is still owed: one task delivered during a stall, announced once, with relaunch and cleanup behaving as before. The PR body says the same. Per the local rule, this is named here and does not block.
What I did not verify.
- The bash 3.2 TERM fix (
:1330) beyond the PR's own check, which passed. - A full serial run of all ~41 watcher suites.
- Linux inotify behaviour. This host's FSEvents coalescing cannot reproduce event-shape flakes.
— Sutando (susans-MacBook-Pro)
A pointer renamed into the inbox keeps its old mtime, so the mtime-windowed catch-up listing never announced it. ctime is reset by the rename, and `find -cnewer` (BSD and GNU alike) compares it against the reference file's mtime, so the cut file keeps working. cleanup() removed only the events FIFO before rmdir, so the catch-up mark kept one $TMPDIR/sutando-task-watch.* directory alive per run. The pass runs every N seconds, not after N seconds of silence; the comment and the docs now say so. The catch-up test gains a rename probe (an hour-old pointer moved in after the mark must be announced within the interval) and asserts the runtime directory is gone after the watcher exits. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
qingyun-wu
left a comment
There was a problem hiding this comment.
Approve at 773b2c8. This is an agent-assisted review (Claude Code, qingyun-wu's agent).
Since my 5a45336 review. The three items I raised are fixed in one push:
- The catch-up window selects by ctime.
catchup_sweepuses-cnewer "$CATCHUP_MARK" ! -cnewer "$cut", so a pointer renamed into the inbox with an old mtime is picked up. The reference is still the mark file's mtime, so thetouch -tcut keeps working. - cleanup() removes
catchup-markandcatchup-mark.cutbeforermdir. The runtime directory no longer leaks. - The comment and docs say "every N seconds, events or not".
Verified locally at head (temp worktree; SUTANDO_WORKSPACE_DIR and SUTANDO_TASKS_DIR unset):
watch-tasks-stream-catch-up.test.sh: PASS, 10 checks, including the new rename probe and "runtime dir is gone after exit (left: 0)".watch-tasks-stream-catch-up-degraded-identity.test.sh: PASS, 4 checks.
The diff since 5a45336 touches only src/watch-tasks-stream.sh (±6 lines), the catch-up test and the hosting-modes doc.
Non-blocking: the remaining real-fswatch round trip after a restart is still worth taking once this is deployed.
liususan091219
left a comment
There was a problem hiding this comment.
Comment at 773b2c83: no change-request condition fires. The code looks ready, but the approval bar is not met yet.
My should-fix from 5a453368 landed. catchup_sweep now lists entries with -cnewer "$CATCHUP_MARK" ! -cnewer "$cut" (src/watch-tasks-stream.sh:1107). cleanup() removes catchup-mark and catchup-mark.cut before rmdir (:1049). The docs sentence now says ctime.
What I ran at this head (macOS, /bin/bash 3.2.57, env -i, temp dirs, no SUTANDO_* set)
tests/watch-tasks-stream-catch-up.test.sh: PASS, 10/10. That includes the hour-old-mtime rename probe and "runtime dir is gone after exit (left: 0)".tests/watch-tasks-stream-catch-up-degraded-identity.test.sh: PASS, 4/4.- Control: I made only
catchup_sweepreturn at once and kept everything else at head. The catch-up test then fails 4/10: the unreported pointer, the renamed pointer, the exactly-once count and the refused-by-sweep check all fail. Subscription, liveness, TERM-exit and runtime-dir cleanup still pass. So the test drives the real watcher and isolates the new pass. It does not grep the source. - Added lines contain no hardcoded workspace paths. The two
SUTANDO_WORKSPACE_DIRhits are test-local temp dirs.review-checks.sh --diffreports hardcoded-paths and root-artifacts clean. - CI at this head: all 33 check runs are success or skipped (Windows).
license/clais success with 1 context.mergeStateStatusis CLEAN. No review threads are unresolved.
Non-blocking note. Every read is now timed (t is always at least 1), so the untimed read branch and its sleep 0.1 (:1327, :1338) can no longer be reached. If fswatch is mid-exit but not yet a zombie, the loop retries the read with no pause until it is reaped. That window is short and the fix is optional. Removing the dead branch would also shrink the diff.
What is missing for approval
- An expert approval from someone who did not write this code. @qingyun-wu's APPROVED at this head reviews
773b2c83, which her account committed. Her account also committed seven of the eleven commits on this branch, four of them non-merge, including the catch-up rewrite, the TERM-trap fix and the ctime change. By §6's distinct-party rule, that is a co-author's approval, not an independent one. This is the same gap I named at5a453368. An approval at this head from another watcher reviewer would meet the bar. - Live path (named, not blocking). A real-fswatch macOS round trip after a restart is still owed: one task delivered during a stall, announced once, with relaunch and cleanup behaving as before.
Not verified: Linux inotify behaviour (this host's FSEvents coalescing cannot reproduce event-shape flakes), and a full serial run of the ~41 watcher suites.
— Sutando (susans-MacBook-Pro)
liususan091219
left a comment
There was a problem hiding this comment.
Comment at 773b2c83: no change-request condition fires; still not approvable, and the branch now conflicts with main.
What moved since my 2026-10-20 comment at this same head. Nothing on the branch. main did: three watcher changes landed that touch src/watch-tasks-stream.sh (#5792, #5801, #5803), and #5801 also touches docs/task-watcher-hosting-modes.md. GitHub now reports mergeable: CONFLICTING / mergeStateStatus: DIRTY. main has no catch-up pass of its own (no catchup/CATCHUP in main's copy of the watcher), so this PR is not superseded; it needs a rebase or merge of main.
Unchanged from 2026-10-20.
- The only APPROVED review is @qingyun-wu's at this head, and her account committed most of this branch, including the catch-up rewrite. By the distinct-party rule that is a co-author's approval, not an independent expert one.
license/claat this head: 1 context,success. No unresolved review threads.- Live path (named, not blocking): a real-fswatch macOS round trip after a restart is still owed.
What is missing for approval
- Resolve the conflict with
main, keeping #5801's owner-exit and #5803's checker-sleep changes intact alongsidecatchup_sweep, then re-run the two catch-up suites. - An approval at the new head from a watcher reviewer who did not write this code.
I did not re-run the suites: the head is unchanged, and the results in my 2026-10-20 comment still describe it. They will need re-running after the rebase.
— Sutando (susans-MacBook-Pro)
…-watcher-catch-up # Conflicts: # src/watch-tasks-stream.sh
liususan091219
left a comment
There was a problem hiding this comment.
Comment at 773b2c83: no change-request condition fires. Still not approvable: it conflicts with main, and it has no independent approval.
Re-checked today. The head has not moved since my 2026-10-24 comment. main's src/watch-tasks-stream.sh still has no catch-up pass, so this is not superseded. Since that comment, two more watcher changes have landed on main and touch the same file:
- #6026, a standby never takes a live session watcher's sentinel
- #6172, a foreign-inbox watcher never takes a live watcher's sentinel
The branch is now 267 commits behind, and mergeable is CONFLICTING.
What holds at this head:
- CI: 33 pass, 1 skipped.
license/clais success (1 context).- No unresolved threads.
- The catch-up tests drive the real watcher. My 2026-10-20 control showed that stubbing
catchup_sweepfails 4 of 10, so they are not source greps. - No hardcoded workspace paths are added.
What is missing for approval
- Rebase or merge
main. Keep #5801, #5803, #6026 and #6172 intact alongsidecatchup_sweep, then re-run both catch-up suites and CI. - An approval at the new head from a watcher reviewer who did not write this code. The one APPROVED review (qingyun-wu) comes from an account that committed most of the branch, including the catch-up rewrite.
- Live path (named, not blocking): a real-fswatch macOS round trip after a restart. One task should be delivered during a stall and announced once, and relaunch and cleanup should behave as before.
— Sutando (susans-MacBook-Pro)
…shipped default The core seat's own case from sonichi#4590: fswatch answers the readiness probe and then goes silent; a task dropped after the startup sweep is still announced once within the 30 s default. Fails with main's watcher (never announced). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@cla-assistant check |
|
@elCaptnCode, thank you for this. It turned out to be exactly the fix we needed. On our own host today, a core session watcher sat on owner tasks for up to half an hour: its fswatch stayed alive but stopped reporting inbox events, and the task was only found by the next watcher's startup sweep. That's #4590 as you described it. Since "allow edits by maintainers" is on, I pushed two commits on top of yours (normal push, no force; your commits and authorship are unchanged):
The macOS acceptance your description asked for. I ran it with real fswatch 1.18.3 and the watcher on
Also green here under
Re-requesting review at 71b4e2a. |
|
A correction to my comment above. I attributed our host's missed tasks to #4590, an fswatch that stays alive but stops reporting. Further evidence says that was wrong for those cases.
This PR's catch-up is still a sound guard against the fseventsd stall it targets, and the regression test still stands. But it wasn't the fix for what we saw, and I didn't want my earlier comment to suggest otherwise. Thanks again, @elCaptnCode. |
Problem and change
Refs #4590 and #4588. A live fswatch that emits no event can leave a newly delivered task undiscovered after the startup sweep. This adds a bounded recovery scan using the existing startup sweep and dispatch admission logic.
At head
ac1e3245906ec33d0b3114410f0a16e2ae628a59, upstream5e5e87dd1b7a81d68930933291a89634b731b91cis merged without rewriting the previous PR commits. The diff against current main remains two files.Review
src/watch-tasks-stream.shfirst:SUTANDO_WATCHER_CATCHUP_SECONDSdefaults toSUTANDO_HANDLER_POLL_INTERVAL, then 30 seconds. Empty, nonnumeric, and zero values fall back to 30. A BashSECONDSdeadline bounds the existing FIFO read by the earlier of the catch-up deadline and held-task retry deadline. Checking the catch-up deadline between events also prevents unrelated event traffic from postponing recovery indefinitely. No second scanner, timer process, or admission ledger is added.Upstream's fswatch relaunch budget/backoff, held-task retries, claim handling, and sentinel cleanup are retained. The earlier PR body's description of unconditional exit on fswatch death no longer describes current upstream. This PR does not add a new dead-source policy or claim to resolve all of #4590. Windows' PowerShell watcher is unchanged.
Before/after evidence
Linux, current source tree, stub fswatch; the task is created only after the readiness sentinel exists. The regression checks exactly one announcement, continued watcher liveness, and no duplicate on subsequent passes.
With only the watcher source replaced by upstream
5e5e87dd:At the repaired tree (published tree SHA
eb08ca26a9f4ea04bb2defeebd2ef6cd312ec69a):--noisyemits unrelated paths every 100 ms while withholding the actual task event. Its final message is shared with the silent case; the assertions are identical.Validation and limits
bash -non both changed files: passed.git diff --checkagainst current upstream for both changed files: passed.Live acceptance
No real-fswatch macOS acceptance has been performed at this head. Linux stub results do not replace it. Required before readiness: green relevant CI and a macOS run confirming catch-up delivers one task without duplicate dispatch while preserving upstream retry/relaunch and cleanup behavior.
No dependent PR or merge order; existing #4590 remains open. No maintainer approval or live completion is claimed.
Update (maintainer)
Why now. On 2026-10-17 a host-wide fseventsd stall on a macOS seat host left every watcher's fswatch alive but silent for hours. New task pointers kept landing in seat inboxes and were never announced, because main has no periodic rescan. This is the #4588/#4590 shape, caused by the event source rather than a startup stall.
What changed at
1a47cdf4(author's commits kept, no rewrite):Merged current
origin/main(93179d8c). No conflicts; the catch-up hunks apply unchanged on top of the fswatch relaunch, handler config and standby/session roles.A bash 3.2 TERM-trap fix that catch-up makes necessary. With catch-up, every main-loop read is
read -t. On bash 3.2 (macOS/bin/bash), the read's alarm longjmps out of a TERM trap whose cleanup takes longer than the time left on the read. Cleanup stops partway, TERM is already ignored, and the loop resumes. The result is a watcher with no sentinel that nothing can stop, or one that exits 1 through the fswatch-lost path. A 10-line bash 3.2 script reproduces this deterministically. On the watcher itself, 1 of 12 TERMs left a stuck process at a 1 s interval, against 0 of 24 on main. On main this was latent: only the held-task path used a timed read. The fix is one line after the read: ifCLEANING_UPis set, finishcleanupandexit 0. bash 5 never hits this, and the check is a no-op there.The test now models the stall. It uses a pool delivery inbox with a resolver and an fswatch stub that blocks and emits nothing (
exec tail -f /dev/null), and drops a pointer plus its payload after the startup sweep, withSUTANDO_WATCHER_CATCHUP_SECONDS=2. It asserts:TASK_FILE:across three more sweeps;pgrepstub makes cleanup outlast the read, so this check is deterministic.The test scrubs all
SUTANDO_*so a pool host's live seat cannot leak in, and--noisyis kept. The design is unchanged: a bounded read deadline plus a re-run ofstartup_sweep/dispatch_task. Already-emitted pointers are deduped by the per-lifetimeDISPATCHED_IDSadmission, and answered ones byhandler_result_is_answer. No second scanner, timer process or ledger is added.Test:
tests/watch-tasks-stream-catch-up.test.sh(macOS, bash 3.2.57, temp dirs only)Existing watcher suites at the branch head (
tests/watch-tasks-stream*,watch-tasks-dispatch-activity,watcher-*; 40 suites,env -i, temp dirs): 36 pass. 4 fail, and all 4 also fail on an unmodifiedorigin/mainarchive on the same host with the same or a superset of FAIL lines:bare-directory-event(second-EOF budget),self-check(--force-restart(b)/(k)),shutdown-emit(the "leaving to the next watcher" lines) andshutdown-leaves-inflight-claim(hang-before). None of them is introduced here.bash scripts/review-checks.sh --diffreports hardcoded-paths and root-artifacts clean (PARTIAL: no .py in scope).shellcheck -S errorandbash -nare clean.Known cost, not changed here. Every sweep re-resolves each pointer still in the inbox. Answered pointers are refused each time, which costs one resolver call and one
find-readycall per pointer per interval (default 30 s) and logs one stderr line each. #5162, which retires answered pointers in the archive step, is the complement that keeps this floor cheap on a long-lived inbox. Still out of scope: a dead fswatch past the relaunch budget, and a session watcher started during the stall (from reading the code, not tested: its readiness probe needs an fswatch event, so it exits 1 before catch-up ever runs).Update (maintainer, review at 1a47cdf)
Head
5a453368answers the request-changes review. Author commits are untouched;origin/mainis merged again (e36e6fb7).Blocker, cost per sweep. The catch-up pass no longer re-runs the full startup sweep.
catchup_sweeplists only entries whose mtime falls after the previous pass's mark and before the last second (find -newer mark ! -newer cut, three cheap subprocesses per pass, none per entry), and feeds each throughhandle_event, as the one fswatch event it stood for. Each entry is examined by exactly one pass; the mark predates the startup sweep's listing so nothing falls between them, and the existingDISPATCHED_IDSdedupe absorbs the overlap. The full listing stays with the startup sweep and the fswatch relaunch. This also answers nit 4 (an entry younger than 1 s is left to the next pass) and nit 3 (the sweep no longer re-examines held tasks, so theHELD_RETRY_INTERVALcomment is accurate as it stands).Measured on this host (macOS, bash 3.2.57, 2,000 answered pointers aged 1 h in a temp delivery inbox with an sh resolver, fswatch stub silent,
SUTANDO_WATCHER_CATCHUP_SECONDS=2; script in the review thread's worktree, not committed):The startup sweep still pays for the stale inbox once on both sides (that is
main's cost, #5162's problem). Before, a pointer dropped after the sentinel waited a whole 2,000-entry pass (596 s) and every pass re-refused all 2,000 (310 refusals per minute, continuously). After, it is announced on the next pass (3 s) and the stale pointers cost nothing further.Should-fix, empty identity. When
cksumfails, the identity now degrades tofilename|inode:m<mtime>instead of empty, so a re-examination is deduped; only a missing inode still dispatches without dedupe, logged as before. New testtests/watch-tasks-stream-catch-up-degraded-identity.test.sh: a live (polling) event stream, acksumthat always fails, one task, three catch-up passes. At1a47cdf4it was announced 4 times; now once.Nits. (1)
SUTANDO_HANDLER_POLL_INTERVALalias dropped. (2)10#normalisation;08,00and0now read as 8, 30 and 30. (3) See above. (4) See above. (5) Test headers are two lines. (6)docs/task-watcher-hosting-modes.mdgained a paragraph on the knob under "Known issues and gaps". (7) Nothing to change; CI will re-run on this push.Tests. The catch-up test now also seeds 40 answered pointers before start and asserts that a fresh pointer still lands within the interval and that no catch-up pass re-examines them.
Previous head
1a47cdf4with the new tests:This head:
Existing watcher suites, serial,
env -i, temp dirs (41 files:tests/watch-tasks-stream*,watch-tasks-dispatch-activity,watcher-*): 39 pass.shutdown-emitandshutdown-leaves-inflight-claimfail; both also fail on an unmodifiedorigin/mainarchive on this host (same FAIL lines).bare-directory-eventandself-check, which failed under the earlier parallel run, pass serially.review-checks.sh --diff,git diff --check,bash -nandshellcheck -S errorare clean.Update (maintainer, review at 5a45336)
Head
773b2c83answers the three items from the review at5a453368, in one push on top of it.ctime, not mtime. The catch-up listing used
find -newer mark ! -newer cut, which compares each entry's mtime. A pointer written elsewhere and renamed into the inbox keeps its old mtime, so one older than the previous pass was never listed, and never announced. The listing now uses-cnewer: a rename resets the entry's ctime, and both BSD (macOS) and GNUfindcompare-cneweragainst the reference file's mtime, so thetouch -tcut file keeps bounding the window as before. The in-place write case is unchanged (a fresh write sets both stamps).The catch-up test gains the probe: after the first catch-up announce, a pointer is written outside the inbox,
touch -tan hour in the past,mv-ed in, and must be announced within the interval plus margin. Eight serial runs each side on this host (macOS, bash 3.2.57, temp dirs, the new test against an unmodified archive of5a453368and against this head):Before: the renamed pointer was never announced in 8/8 runs (the reviewer saw 2/8 failing; here the hour-old mtime misses the window every time). After: announced 8/8, within 2-4 s of the move. Run 1 before also failed a third check, which the filtered log did not capture.
Leak.
cleanup()removed only the events FIFO beforermdir, so the catch-up mark kept every$TMPDIR/sutando-task-watch.*directory alive, one per watcher run (this host had hundreds).cleanup()now also removescatchup-markandcatchup-mark.cut. The test launches the watcher withTMPDIRunder its own temp dir and asserts nosutando-task-watch.*remains after the TERM exit (theleft: 1→left: 0lines above).Wording. The pass runs every
CATCHUP_INTERVALseconds whether or not events arrived (CATCHUP_NEXTis re-armed unconditionally at the loop head), not "after N seconds of silence". The comment insrc/watch-tasks-stream.shand the paragraph indocs/task-watcher-hosting-modes.mdsay so now, and the docs sentence says ctime.This head:
Existing watcher suites, serial,
env -i, temp dirs (41 files:tests/watch-tasks-stream*,watch-tasks-dispatch-activity,watcher-*): 39 pass on the first pass.shutdown-emitfails with the same threeleaving to the next watcherFAIL lines as on an unmodifiedorigin/mainarchive on this host, as before.shutdown-leaves-inflight-claimpassed this time.readiness-window-decision-instantfailed once under load (57 s,results=[]) and passes on rerun at this head (27 s) and at the5a453368archive (30 s); not introduced here.bash -n,shellcheck -S error,git diff --checkandgit diff | scripts/review-checks.sh(hardcoded-paths + root-artifacts clean; PARTIAL only because no.pyis in scope) are clean;tests/docs-watcher-launch-is-tagged.test.pypasses.🤖 Generated with Claude Code