Make the Codex probe's timeout actually bound doctor - #629
Merged
Conversation
Two holes in what shipped, both the same shape as the drift the check was written for. It globbed *.yml only. Every workflow here is .yml today so nothing was skipped, but GitHub honours .yaml just as well, and a .yaml workflow running a linter this check never opened is exactly the invisible drift it exists to catch. Scan both, and fail on an empty scan rather than passing a check that opened no files. It enforced agreement, not currency. Setting all three workflows to v2.9.0 passes cleanly and reproduces the release failure that motivated the check in the first place. So: a floor, next to WORKFLOW_DIR, moved in the same commit as the pins it constrains. Compare with sort -V. As strings v2.9.0 sorts above v2.11.1, because 9 > 1, so a lexical test would wave through the precise version that broke the release tag — and [ -gt ] parses neither. aur-publish.yml's pkgrel guard carries the same note for the same reason.
`basecamp doctor` hung for ten minutes on a machine where `codex` is a wrapper script. queryCodexPlugin has had a 5s context timeout since it was written, and the timeout was doing nothing. Cancelling the context kills the child. It does not close output pipes a *grandchild* inherited, and Wait blocks on those copies until they close — so `codex` shipped as a launcher that shells out (an npm exec wrapper, a mise shim) leaves Wait parked on a pipe held open by a process the kill never reached. The deadline expires on schedule and the call returns whenever the grandchild feels like it. cmd.WaitDelay is the part that was missing: it bounds Wait itself, not just the process. A codex that will not answer inside the deadline is now a failed check, which is what a diagnostic probe should report. The existing tests all stub runCodexCommand, so none of them touched the real one. The new test does, against a script that backgrounds a longer-lived child — it fails at 30s without WaitDelay and passes in about a second with it. Both durations are named constants now; the timeout and the grace period after the kill belong next to each other.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a8b47c316
ℹ️ 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".
The test needs a grandchild that outlives the deadline by a wide margin, or it passes on the sleep ending rather than on WaitDelay working — so the 120s is load-bearing and cannot just be shortened. That makes reaping the test's job. WaitDelay closes the inherited pipe; it does not kill the process, which is reparented to init and sat there for two minutes afterwards, one orphan per `bin/ci`. The shell now records the background PID and t.Cleanup kills it, including on the failure path. Also runs the `sh` that LookPath resolved rather than assuming /bin/sh, since the skip already depends on that lookup.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6995cd6b29
ℹ️ 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".
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.
Found while cutting v0.9.0:
make release VERSION=0.9.0 DRY_RUN=1failedrace-testwithNot a flake, and not a test bug.
basecamp doctorreally does hang.What happens
queryCodexPluginhas had a 5s context timeout since it was written. The timeout was doing nothing.Canceling the context kills the child. It does not close output pipes a grandchild inherited, and
Waitblocks on those copies until they close. The stack shows exactly that — one goroutine parked inio.Copyon the pipe, another inexec.(*Cmd).awaitGoroutines, both nine minutes in:On the machine that caught it,
codexis a wrapper script rather than a binary. The processes were still there afterwards:The kill reached the wrapper.
npm execkept the inherited stdout pipe open, soWaitwaited on it. The deadline expired on schedule and the call returned whenever the grandchild felt like it.This is not specific to that launcher — an npm-exec shim, a mise shim, anything that shells out has the shape. It never reproduces in CI, where
codexis absent andFindCodexBinaryreturns early.The fix
cmd.WaitDelayboundsWaititself, not just the process — the one piece that was missing. A codex that will not answer inside the deadline is now a failed check, which is what a diagnostic probe should report.Both durations are named constants now; the timeout and the grace period after the kill belong next to each other.
internal/harness/codex.go:30is the onlyexecon the doctor's probe path — the Claude probe reads files — so this is the whole exposure there. Nothing else in the repo setsWaitDelay; the other 24 call sites are interactive setup and upgrade paths, and widening to them is a separate change with its own tests, not something to fold into a release fix.Verification
Every existing test in this file stubs
runCodexCommand, so none of them ever touched the real one. The new test does, against a script that backgrounds a longer-lived child:WaitDelayWaitDelayVerified by reverting the one line and re-running, not by inspection.
And the test that actually hung, re-run on the same machine with the same wrapper still installed:
9m47s and a timeout, to 2s.
bin/cigreen (exit 0).Summary by cubic
Fixes a hang in
basecamp doctorby making the Codex probe truly time out usingcmd.WaitDelay, and strengthens the linter lockstep check to prevent stale or missed pins.cmd.WaitDelay(1s) and usecodexQueryTimeout(5s) soWaitcan’t block on a grandchild’s stdout pipe.WaitDelayand passes with it. The test now records the grandchild PID and reaps it int.Cleanup, and usesshfound viaexec.LookPath.scripts/check-lint-lockstep.shto scan both.ymland.yamlworkflows, fail on empty scans, and enforce a minimumgolangci-lintversion using version-aware comparison.Written for commit 6995cd6. Summary will update on new commits.