Fix action hang when descendants keep stdio open - #151
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
…pletes) (#797) Reverts the v1.12 bump from #791. Since v1.12, heavy review runs on Linux never return after the codex turn completes: the action passes the runner's stdout/stderr descriptors to the whole codex process tree, and a surviving descendant keeps them open, so the step idles until timeout-minutes kills the job with the verdict already written (openai/codex-action#150). The CLI-side mitigation (codex 0.150.x) did not resolve it for this repo's drop-sudo runs; the action-side fix (openai/codex-action#151) is unmerged. Both attempts of the PR #796 review died this way (attempt 1 killed at the 60-minute timeout, attempt 2 stuck identically after the analysis finished). Also adds a dependabot ignore for openai/codex-action 1.12 so the broken version is not re-proposed; drop the ignore once the fixed release ships.
|
Issue #150 has been fixed upstream in Codex CLI 0.150.0, so this PR is no longer strictly necessary. Either way, thanks for tracking this down and getting the CLI fix out so quickly. |
|
Quick update: based on the reports in #169, the hang still reproduces even with Codex CLI 0.150.1, so this wrapper-level fix still seems necessary. |
|
I can confirm the same issue it hangs with v1.12. It's not related to the CLI version as I tried different ones. I'd to pin the Codex Action to v1.11 |
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Switching completion to exit fixes the descendant-FD hang, but destroying stdout/stderr in the exit handler can lose output. exit can fire before the child pipes have drained, so a larger final write may still be buffered when closeOutputStreams() unpipes and destroys them. The current regression only writes tiny log lines. Could we drain already-buffered output after the direct child exits without waiting for descendant-held EOF, and add a large final stdout/stderr regression?
|
I pushed an update based on your feedback.The action now waits briefly after the direct child exits to let buffered stdout/stderr drain, without waiting for EOF from descendants that may still hold the pipes open. I also added a regression test with large final stdout/stderr output, including the descendant-held-stdio case. pnpm run check and the full test suite are passing. Could you take another look? |
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The process-completion boundary looks sound. Using the direct child exit event avoids coupling completion to descendant-held descriptors, while the bounded drain preserves already-buffered stdout/stderr without reintroducing an indefinite wait. The regression coverage exercises both inherited stdio and large final output. No blocking issues found.
|
Thanks for the review. The PR is ready from my side — please merge when convenient. |
…base#6538) ## What kind of change does this PR introduce? CI reliability fix. ## What is the current behavior? AI Review's Codex jobs (`codex-review`, `adjudicate`) intermittently hang and get killed on job timeout, discarding a completed review. Most recently: [run 34323125644](https://github.com/supabase/cli/actions/runs/34323125644) on supabase#6535 died at ~55 minutes against a 45-minute timeout. This is a regression. supabase#6380 deliberately pinned `openai/codex-action` to v1.11 because v1.12 had a confirmed hang bug (openai/codex-action#150). supabase#6484 — a Dependabot `actions-major` group bump bundling 6 unrelated action updates — silently reverted that pin back to v1.12; the workflow's own comments (still saying "pinned to v1.11, NOT v1.12") went stale rather than catching the drift, since nothing diffed them against the actual `uses:` line. Checking upstream today turned up two separate, still-open v1.12 regressions, both matching this workflow's exact config (`safety-strategy: drop-sudo`, `sandbox: read-only`, `output-schema-file`): - openai/codex-action#151 — the v1.12 wrapper waits on the child process's `close` event with inherited stdio; a lingering descendant keeps the step alive forever after Codex has already written its output and finished. - openai/codex-action#160 — v1.12's `drop-sudo` rewrite chmods root-owned `/run` service sockets, breaking `systemd-resolved` on the GitHub-hosted runner itself, which kills the job 52-65 minutes in regardless of the job's own timeout — matching our job's 55-minute death exactly. Neither has a released fix. Both are action-level bugs independent of the pinned Codex CLI version (reproduced across CLI versions 0.147.0-0.150.1 in the upstream threads). This is not a diff-size or token-limit problem: supabase#6535's diff was only ~2200 lines, and v1.11 has cleanly handled 130k-270k-token diffs in under 15 minutes per the upstream reports and our own prior testing. ## What is the new behavior? - Re-pin `openai/codex-action` to v1.11 (`52fe01ec70a42f454c9d2ebd47598f9fd6893d56`) in both Codex jobs — verified it's a safe drop-in, since v1.11's `action.yml` supports every input this workflow uses. - Refresh the stale inline comments to cite the actual issues (#151, #160) instead of just the original #150. - Add `openai/codex-action` to `.github/dependabot.yml`'s `ignore` list (no `update-types` restriction, so it blocks all automated bumps) so a grouped bump can't silently regress this pin again. Any future bump now requires a deliberate PR that checks the upstream changelog/issue tracker first.
…ase#6542) ## What kind of change does this PR introduce? CI reliability fix (follow-up to supabase#6538). ## What is the current behavior? supabase#6538 re-pinned `openai/codex-action` to v1.11 and added a Dependabot ignore entry scoped to `versions: ["1.12.x"]`, intending to still let Dependabot propose v1.13+ once the upstream hang bugs (openai/codex-action#151, supabase#160) are fixed, while blocking the known-bad v1.12 line specifically. That scoping never actually worked. 9 minutes after supabase#6538 merged, Dependabot opened supabase#6541 proposing the exact v1.12 bump we were trying to block — config propagation wasn't the issue; the `versions` syntax was. The `github-actions` ecosystem's `ignore.versions` strings are parsed as Ruby `Gem::Requirement` (RubyGems comparator syntax: `>= x`, `~> x`, etc.), not npm-style semver ranges. `"1.12.x"` isn't a wildcard in that grammar — it parses as a literal version string with an implicit `=` operator, which never equals the real dependency version (`"1.12"`), so the ignore condition silently never matched anything. Verified directly against the actual parsing logic dependabot-core uses (`Dependabot::GithubActions::Requirement`, a thin wrapper around `Gem::Requirement`): ``` GithubActionsRequirement.new("1.12.x").satisfied_by?(Gem::Version.new("1.12")) # => false (bug) GithubActionsRequirement.new(">= 1.12, < 1.13").satisfied_by?(Gem::Version.new("1.12")) # => true GithubActionsRequirement.new(">= 1.12, < 1.13").satisfied_by?(Gem::Version.new("1.12.5")) # => true GithubActionsRequirement.new(">= 1.12, < 1.13").satisfied_by?(Gem::Version.new("1.11")) # => false GithubActionsRequirement.new(">= 1.12, < 1.13").satisfied_by?(Gem::Version.new("1.13")) # => false ``` ## What is the new behavior? Replace `versions: ["1.12.x"]` with `versions: [">= 1.12, < 1.13"]` — a real Gem::Requirement comparator range, confirmed to correctly match the 1.12 line (including any 1.12.x patch) while excluding v1.11 and v1.13+. supabase#6541 should be closed as superseded once this merges.
nicolas-bourdeau
left a comment
There was a problem hiding this comment.
🔴 Blocking: the new regression test fails in the GitHub Actions environment because GITHUB_OUTPUT is inherited by the fake Codex runner (test/runCodexExec.test.mjs:113-119). @actions/core.setOutput("final-message", ...) therefore writes to that file, not to result.stdout, but the test asserts /fake final message/ in stdout at test/runCodexExec.test.mjs:142. Reproduced with GITHUB_OUTPUT="$(mktemp)" node --test test/runCodexExec.test.mjs: the first test fails while the large-output test passes. Please either clear GITHUB_OUTPUT for this subprocess or assert the final-message output from a temporary output file; then rerun the CI-equivalent suite.
Summary
runCodexExecfrom the direct child process exit and close the private pipes so surviving descendants cannot keep the runner transport openThe Linux
sudo -> sh -> setpriv -> envprivilege-isolation chain is unchanged. The fix only changes the boundary between that process tree and the action runner's log streams.Fixes #150.
Root cause
runCodexExecpassed the runner's stdout and stderr descriptors directly to the entire Codex process tree. A descendant that survived the direct Codex process could retain those descriptors after the result had been written, preventing the GitHub Actions runner from observing the end of its log transport.Waiting for Node's
closeevent also couples command completion to inherited stream closure. The action now forwards private child pipes and uses the direct child'sexitevent as the command result, then tears down those private pipes.Test plan
corepack pnpm run checkcorepack pnpm test(145 tests: 142 passed, 3 platform-dependent skips)ETIMEDOUTfinal-messagegit diff --check