fix(cli): cleanup no longer killed by its own status message - #1417
Conversation
`go-code runs | head` left harnessd running. The orphan holds the workspace lock, so the next go-code in that project died ~11s later with "callback workspace is already owned" — and the wrapper's error hint suggested a port conflict, which cannot be the cause, because the lock is workspace-scoped rather than port-scoped. The script runs under `set -euo pipefail`, and stop_server's first statement was an info line. With stdout closed that write fails, `set -e` aborts the function, and the kill on the next line never runs. Cleanup was killed by its own status message. `|| true` on the printf is not sufficient on its own: a SIGPIPE taken while the EXIT trap is running terminates the shell instead of returning control to the trap. So stop_server now ignores PIPE outright, making cleanup immune to writing rather than merely tolerant of it. The EXIT trap is also armed the moment the daemon is spawned rather than at mode dispatch, which closes the window in between and collapses three per-mode arms into one owner of the contract. Closes #1416 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJGxhoFhA8JjkwZFcLGdS5
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 751defaeaf
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| ### Troubleshooting: a stale `harnessd` left behind | ||
|
|
||
| If a `harnessd` the wrapper started is ever left running after `go-code` exits — for example after a crash rather than a normal exit — the symptom on your next `go-code` invocation in that project is a failure like `callback workspace is already owned: resource temporarily unavailable`, because the leftover daemon still holds the workspace's callback-recovery lock. Changing `HARNESS_ADDR` or the port does not help: the lock is scoped to the workspace, not the port. The remedy is to stop the stray process directly, for example `pkill -f harnessd`, then run `go-code` again. |
There was a problem hiding this comment.
When a user has multiple projects or an intentional go-code --server instance running, pkill -f harnessd terminates every matching daemon rather than only the stale process holding this workspace lock, potentially interrupting unrelated active runs. The wrapper already records the owned daemon PID in ${TMPDIR:-/tmp}/harnessd.<wrapper-pid>.pid; direct users to identify and stop that specific PID instead of recommending a process-wide kill.
Useful? React with 👍 / 👎.
TestGoCodeScriptStopsHarnessdOnInterrupt called cmd.Process.Wait() twice — once in the deferred cleanup, once in the goroutine feeding the timeout select. os.Process.Wait is not safe to call twice on the same process. That is the one defect here that is proven; it was introduced in #1417. The stub daemon now takes its sleep child with it on SIGTERM instead of orphaning it, and the exit budget is widened to 30s as headroom for a loaded runner. Two earlier diagnoses were measured and disproven: the wrapper exits 0.21s after SIGINT rather than taking the 5s force-kill path, and the stub does genuinely survive SIGINT (POSIX inherits an ignored disposition across exec, so the sleep ignores it too). The Linux CI failure is not reproduced on macOS and its root cause stays unconfirmed. #1422 stays open until CI is green without a re-run. Refs #1422 Claude-Session: https://claude.ai/code/session_01WJGxhoFhA8JjkwZFcLGdS5 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #1416
The bug
go-code runs | head -5— or piping into a pager you quit — leftharnessdrunning. The orphan holds the workspace lock, so the nextgo-codein that project died about 11 seconds later:The wrapper's own hint made it worse by pointing at a port conflict. That advice cannot work: the lock is workspace-scoped, not port-scoped, so relocating the port fails the same way.
Cause
Found by
bash -x, not by reasoning:The script runs under
set -euo pipefail(line 2), andstop_server's first statement after its guards is thatinfoline. With stdout closed the write fails,set -eaborts the function, andkill "$pid"on the next line never runs. Cleanup was killed by its own status message.Fix
trap '' PIPEat the top ofstop_server. This was necessary on its own —|| trueon the printf did not fix it, verified by re-running the trace with only that guard and still leaking. A SIGPIPE taken while the EXIT trap is running terminates the shell rather than returning control to the trap, so cleanup has to be immune to writing, not merely tolerant of it.|| truewas then added toinfo,warn,die, andshow_harnessd_logso a status line can never abort its caller underset -eanywhere else.The EXIT trap is now armed in
start_serverimmediately after the daemon is spawned, closing the window between spawning and mode dispatch, and collapsing three per-mode arms into one owner of the contract.stop_serverchecksSTARTED_BY_USitself, so this cannot touch a daemon the wrapper did not start, and--serverstill deliberately detaches.Two corrections
I got this wrong twice, and the issue has been updated rather than quietly closed under a false premise.
It is not Ctrl+C. The issue was filed claiming SIGINT orphaned the daemon. That evidence was an artifact: I signalled the wrapper alone rather than its process group, so bash deferred its trap while a foreground child was running, and I checked liveness while the wrapper was still alive. Ctrl+C works correctly, and there is now a test proving it.
It is not a missing signal handler. An earlier draft added INT/TERM/HUP/PIPE traps and an
on_signalhelper. All removed — written against the wrong mechanism, and not needed once the real cause was found.Verification
Red first, for the real trigger:
TestGoCodeScriptStopsHarnessdOnInterruptcovers two cases — a wrapper-started daemon is stopped, a pre-existing daemon is left alone — signalling the whole process group as a real Ctrl+C does. It passes today; it is a regression guard against the fix over-reaching, and is labelled that way rather than presented as TDD.All seven wrapper tests green under
-race.Real path with real binaries, not stubs:
Follow-up, not in scope
callback workspace is already ownedshould name the owning PID and suggest a remedy. Worth its own issue.🤖 Generated with Claude Code
https://claude.ai/code/session_01WJGxhoFhA8JjkwZFcLGdS5