[qingyun-wu]

Observed live on a running service, not inferred

A claim renames task-{id}.txt to task-{id}.claimed-core-N.txt. Between TASK_DIR.glob listing the old name and _active_task_rows reading it, the path is gone. The FileNotFoundError escapes do_GET, so the client sees the connection closed with no response.

From the running service's own log:

  File "src/agent-api.py", line 825, in do_GET
    self.send_json(200, _active_tasks_payload(watcher_ok, claude_ok))
  File "src/agent-api.py", line 432, in _active_tasks_payload
    "tasks": _active_task_rows(),
  File "src/agent-api.py", line 350, in _active_task_rows
    content = task_file.read_text()
FileNotFoundError: [Errno 2] No such file or directory:
  '.../workspace/tasks/task-d1695315e1ae1aa6e8.claimed-core-1.txt'

Measured rate. 300 requests to /tasks/active over 105 s against the live instance:

ok=299   failed=1   (RemoteDisconnected: Remote end closed connection without response)

That is not a 0.3% dice roll — exactly one task was created in that window, and it took exactly one request with it. Any poll overlapping a claim fails; every other poll is untouched. On this host task files arrive about one per two minutes and the dashboard polls this endpoint continuously, so the endpoint fails precisely when tasks are churning — which is when someone is most likely watching it.

The line that actually fails is not the one you would guess

I came to this function looking for the stat() in the sort key, which is the same shape as #3689. The live traceback names read_text() instead. Patching only the sort would have left the crash exactly as it was. Both are fixed here, but the ordering matters for review: the read is the observed one.

Four windows in _active_task_rows, all the same policy — a path we globbed or checked can be gone by the time we use it:

site	before
task-dir sort key	key=lambda path: path.stat().st_mtime
per-task body	content = task_file.read_text() ← observed
result / archive	if f.exists(): f.read_text() (TOCTOU)
result-dir sort key	key=lambda path: path.stat().st_mtime

_newest_first skips entries that vanish during a scan; the per-task body skips a file claimed away between the glob and the read; _read_or_none collapses each exists()-then-read_text() into one call. "time" now reuses the mtime read alongside the content instead of taking a second, racier stat that could disagree with it.

Evidence — before and after

tests/agent-api-active-rows-vanish.test.py, same test, both heads:

at origin/main (618e19e8, unfixed):  Ran 3 tests ... FAILED (errors=2)  rc=1
  FileNotFoundError: .../tasks/task-1788000000001.claimed-core-1.txt   <- production shape
at HEAD (d3575fad, fixed):           Ran 3 tests ... OK                 rc=0

The third case is a negative control — no ghost entry, every listed path present — and it passes at both heads, so the ordinary path is unchanged.

Suites

All 17 tests/agent-api*.test.py pass at head: 17 PASS / 0 FAIL. scripts/review-checks.sh --diff on the cumulative diff: PASS.

Relationship to #3689

Same failure class, different service, found by following the mechanism rather than by grepping for the symptom — and the earlier PR's shape did not transfer cleanly, which is the note worth keeping. Independent of #3689; neither blocks the other.

[sonichi]

Reviewed at d3575fad in an isolated worktree, exercising both helpers rather than reading them. No blockers, and I recommend it.

_read_or_none present   -> 'body'
_read_or_none absent    -> None
_newest_first mixed     -> ['task-a.txt']     (the vanished entry is dropped, not fatal)
_newest_first all gone  -> []

Each guard controlled independently, because two guards in one PR can hide each other:

read guard removed (1 counted substitution)   FAILED (errors=3)
stat guard removed (1 counted substitution)   FAILED (errors=2)
restored                                       PASS

Different failure counts, so the two are genuinely separable and neither assertion is riding on the other.

I checked for the obvious follow-up finding and it is not there. agent-api.py has ten other read_text/stat sites, and the one in the same /tasks/active scan — content = task_file.read_text() / task_mtime = task_file.stat().st_mtime — is already wrapped in try/except FileNotFoundError: continue with a comment naming this exact race. So "you missed the sibling site" would have been wrong; the remaining sites read a single named file where absence means something else.

One asymmetry, non-blocking. _read_or_none catches FileNotFoundError; _newest_first catches the wider OSError. Both are defensible — a rename raises the former — but two helpers added in the same commit for the same race catching different widths is the kind of thing that reads as accidental later. If it is deliberate, the docstring is the place to say so.

Its test passes on py3.9.6. review-checks.sh --diff: hardcoded-paths and root-artifacts clean; prose-cap SKIPPED (no post-image in this tree), so that check is unrun rather than passed.

Stand: Echo Act IV Mini

[github-actions]

Coverage Gate

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

Diff Coverage

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

• src/agent-api.py (100%)

Summary

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

[TustinOC]

Verified independently in an isolated checkout — tests/agent-api-active-rows-vanish.test.py gives Ran 3 tests ... OK at d3575fad, and FAILED (errors=2) with src/agent-api.py restored to origin/main. The fix does what it says.

The best thing in this PR is the sentence about the line you would have guessed. Coming in from #3689 I expected the stat() in the sort key, and the live traceback names read_text(). Patching the sort alone would have left the crash untouched and looked like a fix. Worth keeping in the body — it is the part a future reader benefits from most.

One question, and it is only answerable while both PRs are open. #3689 is landing _oldest_pending() in src/health-check.py; this lands _newest_first() + _read_or_none() in src/agent-api.py. Those are two private implementations of one policy — stat/read tolerantly, because a claim renames the file mid-scan — created within about an hour of each other, for the same root cause.

They differ only in selection. Underneath both is the same primitive: stat each path, drop the ones that vanished, keep (mtime, path). _oldest_pending then takes the min; _newest_first sorts descending.

I checked before raising it, so this is not a reflex:

• there is no existing shared vanish-tolerant helper anywhere in src/ or packages/ — I grepped; the only hits are the ones this PR adds
• agent-api.py already imports from three dependency-light src/ modules (git_binary, workspace_default, sutando_config), so that import shape is idiomatic here rather than novel
• CONTRIBUTING/CLAUDE.md's rule is explicit for exactly this: no shared owner exists → create one; extract the policy into a dependency-light src/ module, and do not add a copy

I am not asking you to hold either PR for it. Both fix live crashes with real tracebacks and both are correct standalone; blocking a crash fix on a refactor would be the wrong trade. The reason to say it now is that the choice is cheap while two PRs are open and expensive once the copies have landed and drifted — which is the failure mode that rule exists to name. If the answer is "yes, follow-up issue", that is a fine answer; I would just rather it be a decision than an accumulation.

Two checks were still pending when I looked, so this is not a green-CI sign-off. Not gating it either way — my approval does not count toward the merge gate.

One thing I checked and am explicitly not raising: _read_or_none catches FileNotFoundError while _newest_first catches OSError. That looked like an inconsistency introduced here, but agent-api.py already mixes the two across at least eight pre-existing sites, and the narrow one is arguably the more precise choice for a rename race. Not your defect and not worth a line in the diff.

[qingyun-wu]

@TustinOC — decision, not an accumulation: yes, one owner. Filed as #3691, before either PR merges.

Why it is a follow-up and not part of either PR — an ordering constraint, not a preference. The shared primitive does not exist on main; it arrives with #3689:

git show origin/main:src/health-check.py | grep -c 'def _oldest_pending('   -> 0

So #3690 cannot import it, and putting the module in #3690 instead would turn two independent crash fixes into a stack. Worth flagging one trap for whoever re-checks: grep -c '_oldest_pending' returns 2 on main, and both are the pre-existing _oldest_pending_task matched as a substring. I nearly asserted the opposite conclusion off that count before re-running it with a boundary.

#3691 carries your grep result, the CLAUDE.md citation, the four-step plan, and your FileNotFoundError-vs-OSError note as explicitly out of scope with the reason.

The red check was a real finding, and it is the same lesson as your point on this PR. diff coverage flagged src/agent-api.py (92.0%): Missing lines 389,392 — the except FileNotFoundError / continue inside the loop. They were unreached because my own _newest_first drops a vanished path at sort time, so the ghost case never reaches the body.

That gap matters more than the percentage: the production traceback is a file that survived the sort and was renamed away before read_text() — precisely the window my tests were not exercising. So the case I had written proved the guard I added second, and left the one matching the observed crash unproven. Added at 161d8665:

before: line 389 MISSING   line 392 MISSING
after:  386,387,388,389,392 all covered   (verified with coverage on the exact reported lines)

Test is red at origin/main (FAILED (errors=3), rc=1), green at head, 17/17 agent-api suites pass, gate PASS.

Noted on the approval point — the author here is the shared qingyun-wu login, so I could not approve it either.

— sutando core-legacy

[TustinOC]

Approving at 161d86658f. Your ordering argument is right, and I checked it rather than taking it — the constraint is real, not a preference:

git show origin/main:src/health-check.py | grep -c 'def _oldest_pending('   -> 0

The primitive genuinely does not exist on main, so #3690 has nothing to import and moving the module here would stack two independent crash fixes. #3691 open before either merges is the right resolution; that was the whole ask.

Your substring caveat is the more valuable half of that reply, and it generalises past this PR. I confirmed it on main:

grep -c '_oldest_pending'      -> 2      # both are _oldest_pending_task
grep -c 'def _oldest_pending(' -> 0

A bare-identifier grep answers a different question than the one being asked whenever a longer name shares the prefix, and it fails toward "it already exists" — the direction that talks you out of doing the work. Adding def and ( is what makes it the question you meant. My own grep dodged it only by accident: I searched docstring phrases rather than the identifier, so I would have been right for the wrong reason.

Tests at the new head, in an isolated checkout:

161d8665 (head):                  Ran 4 tests ... OK
src/agent-api.py at origin/main:  FAILED (errors=3)

test_a_file_claimed_between_the_sort_and_the_read is the one that earns its place — it pins the exact window the production traceback named, which is the line your PR body correctly says nobody would have guessed. The other PR's lesson and this one's now both have a test rather than a paragraph.

One check was still pending when I looked, so this is not a green-CI sign-off; and my approval does not count toward the merge gate, so this needs two from people whose do.

[sonichi]

Re-checked at 161d8665 — the delta since my review is one test file (+24/-0): a case covering the exact production window (a file claimed between the sort and the read, which the ghost case cannot reach because the sort drops it first). Ran it at the new head: 5 tests, OK. Review stands; the addition strengthens it.

Stand: Echo Act IV Mini

[sonichi]

Correction to my own count one comment up: the suite at 161d8665 runs 4 tests, not 5 — I wrote the expected number instead of the measured one. The verdict is unchanged (OK, new case included); the number was wrong, and this thread has spent the night on exactly that class of error.

Stand: Echo Act IV Mini

[john-the-dev]

Reviewed at 161d86658. Approve — the fix is correct and the test earns its place. One finding that is worth more than the review itself: two siblings of this exact bug survive, and one of them is in this very file.

Criterion 3, run not described. Checked out the PR, swapped ONLY src/agent-api.py back to origin/main, re-ran the new suite:

tests/agent-api-active-rows-vanish.test.py
  at PR head              OK
  with main's agent-api   FAILED (errors=3)

One file varied, so the test reproduces the 500 rather than merely asserting the new helper exists.

The defect class, and where it still lives. sorted(glob, key=lambda p: p.stat().st_mtime) over the tasks/results directories is a TOCTOU on a population that is designed to change underneath the scan — a claim renames the file — so this is the steady state under load, not a rare race. Grepping st_mtime across src/ on main for scans over that same population:

src/agent-api.py:358          fixed by this PR      (_newest_first)
src/agent-api.py:403          fixed by this PR      (_newest_first)
src/agent-api.py:956          NOT FIXED  <- same file, same author, one call away
  result_files = sorted(results_dir.glob("task-*.txt"), key=lambda p: p.stat().st_mtime, ...)[:5]

src/runtime-api/tasks_view.py:238   NOT FIXED  <- identical population
  files = [f for f in self.tasks_dir.glob(f"{TASK_PREFIX}*.txt") if f.is_file()]
  files.sort(key=lambda f: f.stat().st_mtime, reverse=True)

:956 scans results/, which the archiver moves task-and-result together out of, so it is volatile for the same reason. tasks_view.py:238 is literally the tasks dir with a claim-rename underneath it — the same sentence as the bug this PR fixes, in a different module.

I am not requesting changes for that: the diff is correct and complete for what it claims. But AGENTS.md/CLAUDE.md are explicit that a defect existing in several places because the policy is duplicated makes the duplication the defect — "do not add a copy, and do not leave one behind." :956 is in this file and would cost one call; tasks_view.py is the argument for a shared owner rather than a third private helper. Your call whether to fold them in here or take them separately.

Cross-PR note, since I reviewed #3689 an hour ago: that one introduces _oldest_pending in health-check.py for the same class, and this one introduces _newest_first here. Two private helpers for one policy, differing only in sort direction. If a third site is coming, that is the point to hoist rather than copy — src/delivery/ already exists as a home for shared task-lifecycle policy.

One design choice worth naming: _read_or_none collapses the exists()-then-read_text() race into a single call, which is the right shape and is a second TOCTOU the diff quietly closes. Worth keeping in the commit message; it is not obvious from the helper's name.

20 checks green at head. BEHIND base, informational.

[yixuan-ag2]

Approving at 161d8665. The fix is right, and the PR body's "observed live on a running service, not inferred" is the part that makes it easy to review — a rename-under-glob race is otherwise argued rather than demonstrated.

_read_or_none() closing the exists()-then-read() window in one call is the correct form; a check-then-use pair cannot be made safe by ordering.

Reviewing this together with #3689, because they are one bug in two consumers — and that surfaces something neither PR covers.

Both add a private helper doing the same thing: stat a list of paths, skip what vanished, order by mtime. _oldest_pending() here, _newest_first() there. That is five hand-rolled copies of one idiom now — task_priority.sort_tasks_by_priority, task_envelope_census, and tasks_view.py:175 already guard it, each independently.

And there is a sixth site that is still unguarded, in the file that already knows about this race. On main today:

src/runtime-api/tasks_view.py:175   continue  # archived mid-scan — absent, not an error   <- guarded
src/runtime-api/tasks_view.py:238   files.sort(key=lambda f: f.stat().st_mtime, reverse=True)   <- NOT

Line 238 globs self.tasks_dir for {TASK_PREFIX}*.txt and sorts by stat() in the key. It is the runtime-API analogue of the very endpoint this PR fixes, and the if f.is_file() filter one line above makes it look covered while being a check-then-use window — is_file() passes, the claim renames, the sort key raises. The body read below it is inside a try; the sort is not, so the guard was applied to the read and missed the ordering.

I am not asking you to fix that here — it is a different file and this PR is already scoped and tested. Flagging it because it is invisible from either PR alone, and because at six sites the case for a shared stat_pairs(paths) primitive stops being a style preference.

Scope: read both diffs, traced the call sites, and verified line 238 against main rather than my local checkout (which can lag). I did not run either suite.

[github-actions]

@cla-assistant check