Skip to content

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
entelecheia merged 1 commit into
mainfrom
feat/40-s4a-followups
Oct 24, 2026
Merged

entelecheia merged 1 commit into
mainfrom
feat/40-s4a-followups

Conversation

@entelecheia

@entelecheia entelecheia commented Oct 24, 2026 •

Copy link
Copy Markdown
Member

What

agent-mem#40 F15, F16: the follow-ups claude left from the #424 review (task #435).

  • Doctor's scratch copy never outlives an interrupted doctor. Ctrl-C, SIGTERM, SIGHUP and SIGQUIT remove it and end doctor with that signal (re-raised after the cleanup, so the shell sees the right status); an exit handler covers a throw. The guard is installed before mkdtemp and 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).
  • The next doctor sweeps what a killed run left. The directory is now named 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.
  • A person's own stop of sync --if-due is not a failure. stopSignal().cause() says person (the four signals) or deadline, whichever came first, and a person's stop is classed local whatever 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.
  • CHANGELOG note for the cross-repo effect. A caller that passes signal to syncMemory now gets git in its own process group. agent-hub's daemon (src/hub/daemon.ts syncStop) aborts on SIGINT and SIGTERM through daemon.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 in dev/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, and test/doctor.test.ts stalls 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 (other awaits turn the loop too), so they are redundant on purpose. The remaining limit is stated in docs/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-run 83 files, bun test --timeout 20000 1389 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 after mkdtemp (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

Copilot AI balanced review requested due to automatic review settings October 24, 2026 16:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-24T16:29:40.876200Z a3e74d9 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

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
entelecheia force-pushed the feat/40-s4a-followups branch from a3e74d9 to 4c2ee5c Compare October 24, 2026 16:48
@entelecheia
entelecheia merged commit f654574 into main Oct 24, 2026
3 of 4 checks passed
@entelecheia
entelecheia deleted the feat/40-s4a-followups branch October 24, 2026 17:25
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.

2 participants