Repository navigation
fix(doctor): remove the scratch copy on interrupt, sweep stale ones, and do not count a person's stop as a sync failure (Refs #40 F15, F16) - #54
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…and do not count a person's stop as a sync failure (Refs #40 F15, F16)
entelecheia
force-pushed
the
feat/40-s4a-followups
branch
from
October 24, 2026 16:48
a3e74d9 to
4c2ee5c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
agent-mem#40 F15, F16: the follow-ups claude left from the #424 review (task #435).
exithandler covers a throw. The guard is installed beforemkdtempand told the directory's name afterwards: installed after, a signal in the few microseconds between them took its default action and stranded the directory (measured: 4 of 60 runs when a signal lands the instant the directory appears).amem-doctor-<pid>-<random>. A directory goes only when it is named exactly so, is a plain directory (not a link) this user owns, and its owner pid is gone; a live or undecidable pid keeps its directory; an older unnamed one goes only after a day. SIGKILL cannot be handled by anyone, which is what the sweep is for.sync --if-dueis not a failure.stopSignal().cause()saysperson(the four signals) ordeadline, whichever came first, and a person's stop is classedlocalwhatever step it hit (the same Ctrl-C reads "fetch failed" or "stopped" depending on the step, so the cause decides, not the text). Only the deadline running out, or a real transport failure, starts the backoff. This reverses the round 3 assertion that a Ctrl-C stamps a failure, on purpose.signaltosyncMemorynow gets git in its own process group. agent-hub's daemon (src/hub/daemon.tssyncStop) aborts on SIGINT and SIGTERM throughdaemon.stop()but not on SIGHUP or SIGQUIT, so a daemon ended by one of those relies on the exit handler. The agent-hub pin-bump PR should check that. (Read indev/agent-hub, not run.)A finding about Bun, found while testing and now in the contracts
Bun runs a signal handler only when the event loop turns, and a git lookup is a synchronous
spawnSync. A signal that reaches doctor ALONE during a stalled call waits until the call returns, and by the end of doctor the handler is gone. Doctor now yields to the loop between its stages, andtest/doctor.test.tsstalls every git call it makes, one at a time, and signals doctor during each. Measured: with every yield removed a stalled call loses the signal; removing any ONE yield does not (otherawaits turn the loop too), so they are redundant on purpose. The remaining limit is stated indocs/contracts.md: a signal that arrives before a call that then HANGS is handled only after it returns (a terminal's group signal also reaches a git already running, which is the case covered).Ubuntu CI on the first head (a3e74d9), found and reproduced on Linux
Ubuntu failed two of my new doctor tests while macOS passed. Cause: SIGQUIT's default action is a core dump, and Bun honours it. Doctor re-raised the signal after cleaning up; on Linux with core dumps on that wrote a core file of the whole process image. I reproduced it with the same Bun (1.4.2, the official release, run only inside a network-less container): the process takes 354 ms instead of 60 ms and leaves a 5.9 GB core file in a container with
ulimit -c unlimited, and the shell waits for it. Doctor now exits with the status that death would have had (131) for SIGQUIT, at once, and still re-raises the other three so a parent sees the signal itself. All four signals on Linux Bun: exit 130, 143, 129, 131, 53 to 70 ms, no scratch left, no core file. The test now states the SIGQUIT expectation (exit 131, no signalCode) and a mutant that re-raises it fails. I deleted the downloaded binary and the core file afterwards.The Ubuntu run also showed one process test at 14.8 s against the gate's 20 s default, so the process-spawning tests now have their own 90 s timeout and wider waits (12 clean repeats at the gate's default after that).
Flakiness, found by repeating, not by luck
The new process tests failed intermittently when I repeated the two files. Three causes, each fixed: the guard-before-mkdtemp race above; the test signalling before doctor had started the git it stalls on; and the deadline test using 400 ms, shorter than git needs to start ssh on a loaded machine (2 s now, and it asserts the short deadline, not the 30 s one, ended the run). Last run: both files 25 times in a row, 0 failures, no stray process.
Check
tsc clean, boundary OK (75 files),
npm pack --dry-run83 files,bun test --timeout 200001389 pass / 0 fail / 2 skip (on main f861bc5), hooks budget pass. Mutants that each fail a test: no signal guard, handler not re-raising, sweep removing live or young directories, sweep ignoring type, owner or links, doctor never sweeping, guard installed aftermkdtemp(the old order), every yield removed, a person's stop stamped as a failure, a later person signal overriding the deadline, the cause not passed on the throw path, the deadline seam ignored. One survived at first (the sweep's owner and link rules, because my fixtures did not exercise them); I fixed the fixtures and the uid is now injectable.Refs #40