[mogul]

🤖: AI agent (First Mate), AI-assisted; human review required.

Closes #655

Context

Every acq invocation waited about one second at exit for the kit-fetch guardian. Root cause (reproduced): the guardian subshell runs trap '' HUP INT TERM, so its sleep 1 tick inherits an ignored SIGTERM. A non-interactive shell cannot reset a signal ignored on entry, so sh -c 'trap - TERM; exec sleep 1' does not help (measured: still ~0.68 s). On USR1 the guardian's EXIT trap ran kill "$tick" (ignored) and then wait "$tick", which blocked until the tick ended by itself.

The diagnosis in the issue is correct. One clarification: the guardian's tick is the only thing waited on; ps/tr in the loop are not involved.

Plan / change

• acq: the guardian's EXIT trap uses kill -s KILL "$tick" (one word) plus a comment explaining why KILL is required.
• test/bats/104-kit-fetch-exit-latency.bats + kit-fetch-exit-latency.py (new files; kit-fetch-lifecycle.py untouched, so no conflict with perf(test): shorten hostport and cli-kits test tail #654).

User-visible behavior change

Faster: every acq command no longer pays up to ~1 s at exit.

	before	after
acq version (real, sbx backend selected)	1.07 / 1.05 / 1.04 s	0.31 / 0.14 / 0.17 s*
acq --help	1.03 / 1.05 / 1.03 s	0.08 / 0.06 / 0.05 s

*The "after" run overlapped other load on the box (first-run/cache effect); isolated runs earlier measured 0.04-0.10 s for both. Timing without a backend selected (acq version exits 1 with no backend here) was 0.01 s both before and after because it exits before the guardian wait matters; the numbers above use ACQ_BACKEND=sbx.

Observable differences:

• stderr: none. kill -s KILL on the tick prints no job-control message in dash, bash 5.3, bash 5.3 --posix, bash 3.2.57, bash 3.2.57 --posix (checked; tests assert empty stderr).
• exit codes: unchanged.
• Timing of tmp-dir removal on normal exit / SIGHUP / SIGINT / SIGTERM: the guardian is signalled with USR1 directly and now finishes in milliseconds (was up to 1 s), so the root is removed sooner. The foreground shell's own exit is no longer held up.
• Ctrl-C: foreground shell behavior is unchanged; cleanup completes sooner.
• Parent SIGKILLed (no EXIT trap): unchanged. The guardian still polls ps -o ppid once per second and removes the root when the owner is gone. Residual window before and after: up to ~1 s plus the poll (measured 1.0-1.03 s). Not made faster; out of scope.
• Replaced/cleared EXIT trap, or acq exec-ing a long-running child (sbx/msb session): unchanged. The guardian still outlives the exec'd program's start and removes the root within ~1 s of the owner no longer being the guardian's parent (measured ~1.0 s before and after). During a long exec'd session the guardian keeps polling exactly as before.

Does not change: $$, stdin, terminal signals, exec semantics, the USR1 handshake, the ready handshake, orphan detection, and what is removed (rm -rf -- "$root" of the private acq-kit-fetch.* root only).

Verification transcript (Linux aarch64, dash /bin/sh, bash 5.3)

• Root cause timings (faithful standalone guardian, 0.3 s after start): kill TERM 0.69 s (dash) / 0.66 s (bash) -> kill -s KILL 0.006 s / 0.012 s. Under real bash 3.2.57: 0.707 s -> 0.004 s; bash --posix: 0.704 s -> 0.005 s. trap - TERM; exec sleep 1 alternative: still ~0.68 s.
• Test files, 3 runs each (ACQ_BATS_JOBS=1, bats wall time, includes load noise from concurrent runs):

file	before	after
60-help	56.6 / 55.7 / 56.4 s	10.8 / 8.5 / 9.4 s
113-host-port-override	80.7 / 84.6 / 81.8 s	65.1 / 58.6 / 58.8 s
103-kit-fetch-lifecycle	11.7 / 11.7 / 11.6 s	5.6 / 5.8 / 5.6 s

(An earlier un-selected-backend run of 60-help showed 6.7 s -> 5.5 s; absolute numbers vary heavily with host load.)

• test/bats/103-kit-fetch-lifecycle.bats: pass. New 104-kit-fetch-exit-latency.bats: pass (exit, SIGTERM/INT/HUP each < 0.5 s with root, guardian and tick gone; replaced trap, exec, parent SIGKILL: removed within bound, ~1 s).
• Real bash 3.2.57 (fetched with scripts/fetch-bash32-oci.py from feat/632-local-bash32-runner, nothing pushed to that branch): the new test and the full kit-fetch-lifecycle.py (21 cases, with $BASH pointed at 3.2.57) pass.
• Mutation checks (each fails the tests): guardian's rm -rf removed (103 and 104 fail); orphan check || exit 0 -> || : (103 and 104 fail); USR1 handler disabled (103 and 104 fail); kill -s KILL reverted to kill (104 fails: ('exit', 0, 1.02)); tick kill+wait removed (104 fails: leaked tick); while true loop mutation passes, expected because the loop condition is not a guarantee the tests exercise beyond what the orphan check covers.
• Test bounds: latency 0.5 s (healthy ~0.01 s, stuck tick ~1.0 s), settle 5 s. 20 runs unloaded and 12 under CPU load had no flakes (reviewer).
• Adversarial review sub-agent (races, signals, leaks): no blockers. One should-fix (test tolerated an orphaned tick that ended on its own) fixed in 7e90ac8. KILL vs reused-pid: the tick's pid is held (running or unreaped) until wait returns, so no new pid-reuse risk beyond what TERM already had.
• scripts/run-shellcheck acq test/bats/104-kit-fetch-exit-latency.bats: no new findings (existing SC2034 warning is in another file's context, unchanged).

CI bash 3.2 job duration: to be added once CI reports.

Rollback

Revert this commit (one-word change in acq; tests are additive).

Security impact

None widened. Only the guardian's own child sleep (a pid it spawned and still holds unreaped until wait) receives SIGKILL instead of SIGTERM. The only deleted path remains the mktemp-created private root (rm -rf -- "$root", unchanged). No new network, credential, privilege or file-access behavior. A pid-reuse hazard cannot arise from the changed line any more than with SIGTERM.

[mogul]

🤖: AI agent (First Mate), AI-assisted; human review required.

CI at head 7e90ac8 is green: pre-commit (8m34s), Conventional Commit Title, and the bash 3.2 offline suite (build job 15m52s).

The bash 3.2 job duration did not visibly drop. Other recent runs of the same job took 11m38s (chore/649) and 13m56s (fix/648), so run-to-run noise (runner, bash build/cache) is larger than the saving. The measured, controlled gains are in the local table above.

Re-measured on the pushed head, 5 runs each with ACQ_BACKEND=sbx: acq --help 0.059-0.067 s, acq version 0.14-0.24 s. The residual acq version time is its own work and varies with host load; it is no longer waiting on the guardian.

[wz-gsa]

Fixed-head adversarial review of 7e90ac882ecc7d562de8597afebda435a052f91d (complete three-file diff inspected; all required checks pass). Major — acq:737-745: the guardian keeps tick=$! after wait "$tick" has already reaped the child and only clears it on the next line. If USR1 arrives in that window, the EXIT trap sends SIGKILL to a numeric PID it no longer owns; a reused PID could be an unrelated same-user process. Bind cleanup to an actual live child/reap state rather than sending an unqualified SIGKILL to a stale saved PID, and keep the fast exit guarantee. Test cleanup — test/bats/kit-fetch-exit-latency.py:93-102,131-137: Case.__init__ starts a child before parsing READY and finding a tick, but failures there precede callers’ try/finally; the test can leak the acq owner/guardian/root. close() also kills stored guardian/tick PIDs based on existence rather than identity. Make construction exception-safe and reap the owned Popen child; do not kill a saved PID after identity is lost. The timing/cleanup scenarios are useful, but green CI does not close these windows. Static inspection only, no PR code run on the host. AI-assisted review (OpenCode); rereview a corrected head.

[mogul]

🤖: AI agent (First Mate), AI-assisted; human review required.

Addressed both review items on new head 42fb94d (two commits on top of 7e90ac8). CI is green: pre-commit, Conventional Commit Title, and the bash 3.2 offline suite (build and run). Please re-review.

(A) Stale tick PID could be SIGKILLed

The window is real, not just theoretical. Under a bash 3.2-style wait, the saved pid is already reaped when the next trap can run, and USR1 can be handled there.

• A standalone guardian loop with a shadow kill showed USR1 handlers seeing tick set after the child was gone: 33 of 182 handler runs on dash and 32 of 32 on bash 5.3.
• The test harness injects USR1 immediately after wait "$tick" returns, which reproduces it deterministically. The 7e90ac8 guardian then SIGKILLs an unrelated sleep bystander (guardian killed an unrelated process, -9). Both window-bystander-spared and window-non-sleep-child-spared fail against it.

Fix (design ii). stop_tick runs ps -o ppid= -o comm= -p "$tick" and sends KILL only if the parent is the guardian's own $$ and the command is sleep or */sleep. It then waits on the tick.

• Only the guardian reaps its own children, so a PID that ps reports as its child cannot be reused before the kill.
• A stale or foreign PID is never signalled.
• It uses POSIX ps options only. No GNU-only tools are added.
• If ps fails, nothing is killed and the guardian waits out the rest of the tick, so the worst case is 1 s, never a wrong kill.
• I considered restructuring the loop (i) and a different wait primitive (iii). I rejected (iii) because a non-interactive shell cannot un-ignore TERM, and job-spec kill (kill %%) fails in both dash and bash.

Second finding, fixed in 42fb94d. The adversarial reviewer found that the ps call widened the window in which a repeated USR1 could abort the EXIT trap before rm -rf "$root". stop_tick now starts with trap "" USR1 and set -f.

Behaviour is unchanged, so I left the PR body's behaviour section as is. Exit still takes a few ms.

(B) Test cleanup

Case.__init__ is now exception-safe: any failure during startup calls close().

• Each process the case may kill is recorded with its identity (command plus start time) when it is discovered. The guardian and tick are recorded, and close() also records the live children of the owner (a recursive walk) before it kills the owner.
• close() reaps the owner Popen, closes its pipes, removes a per-case TMPDIR, and uses kill_known, which re-checks identity before any kill. A PID whose identity is unknown or has changed is never killed.

Tests added to kit-fetch-exit-latency.py

Test	What it checks
window-bystander-spared	tick points at an unrelated live sleep that is not the guardian's child, and it must survive.
window-stale-pid-clean	The real stale PID case.
window-double-usr1-clean	USR1 stream plus slow ps.
window-non-sleep-child-spared	tick is a live guardian child that is not sleep.

Evidence

Mutations (each run against both kit-fetch-lifecycle.py and kit-fetch-exit-latency.py):

Mutation	Result
guardian rm -rf removed	killed by both
orphan check disabled	killed by both
USR1 handler disabled	killed by 104 (root left behind)
kill -s KILL reverted to kill	killed by 104 (exit 1.03 s)
tick kill removed	killed by 104 (exit 1.03 s)
ppid/identity guard removed	killed by 104 (killed an unrelated process)
ppid check only removed	killed by 104 (killed an unrelated process)
sleep comm check removed	killed by 104 (killed a non-sleep child)
trap "" USR1 removed	killed by 104 (root left behind)

The USR1 handler mutant survives the lifecycle test; the exit-latency test kills it.

Timings. ACQ_BACKEND=sbx, 5 runs, in ms:

	acq --help	acq version
before the PR (ef24965)	1037 1057 1042 1052 1036	1034 1041 1038 1037 1041
7e90ac8	89 81 97 131 99	282 271 272 259 415
42fb94d	87–90	213–388

The extra ps fork is not measurable against the ~1 s saved.

Other shells.

• Real bash 3.2.57: kit-fetch-exit-latency.py passes all 11 cases. In kit-fetch-lifecycle.py the nested case fails identically on 7e90ac8 and on the new head, because the relocatable bash wrapper cannot re-exec $BASH. With that case stubbed out it passes up to foreground SIGINT group, and the later failure is also the same on 7e90ac8 and the new head. Both look like fixture artifacts of the relocatable wrapper, not regressions. The CI bash 3.2 job is the authoritative run and passed.
• The reviewer also ran the guardian under dash, bash 5.3 (--posix) and bash 3.2.57 (--posix): all window tests pass.
• scripts/run-shellcheck shows no new findings.

Reviews. Two blocking adversarial sub-agent reviews, covering races, signals, PID reuse and leaks.

• Review 1: no blockers, one should-fix (the repeated-USR1 case), now fixed.
• Review 2: no blockers or should-fixes. It left a theoretical NIT: a USR1 burst of 8 before trap "" USR1 has run can still re-enter exit 0. Nothing in acq can produce that, because the owner sends exactly one USR1.