Skip to content

Fix action hang when descendants keep stdio open - #151

Open
Ce1ling wants to merge 2 commits into
openai:mainfrom
Ce1ling:fix/issue-150-process-completion
Open

Fix action hang when descendants keep stdio open#151
Ce1ling wants to merge 2 commits into
openai:mainfrom
Ce1ling:fix/issue-150-process-completion

Conversation

@Ce1ling

@Ce1ling Ce1ling commented Aug 22, 2026

Copy link
Copy Markdown

Summary

  • isolate the Codex process tree behind action-owned stdout and stderr pipes while preserving live log forwarding
  • complete runCodexExec from the direct child process exit and close the private pipes so surviving descendants cannot keep the runner transport open
  • add a regression test where fake Codex writes its final result, exits successfully, and leaves a descendant holding inherited stdio

The Linux sudo -> sh -> setpriv -> env privilege-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

runCodexExec passed 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 close event also couples command completion to inherited stream closure. The action now forwards private child pipes and uses the direct child's exit event as the command result, then tears down those private pipes.

Test plan

  • corepack pnpm run check
  • corepack pnpm test (145 tests: 142 passed, 3 platform-dependent skips)
  • regression proof with the production fix temporarily removed: the focused test fails after 2 seconds with ETIMEDOUT
  • focused test with the fix restored: passes in about 0.5-0.8 seconds and preserves stdout, stderr, and final-message
  • git diff --check

@github-actions

github-actions Bot commented Aug 22, 2026

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@Ce1ling

Ce1ling commented Aug 22, 2026

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

github-actions Bot added a commit that referenced this pull request Aug 22, 2026
igerber added a commit to igerber/diff-diff that referenced this pull request Aug 28, 2026
…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.
@Ce1ling

Ce1ling commented Aug 31, 2026

Copy link
Copy Markdown
Author

Issue #150 has been fixed upstream in Codex CLI 0.150.0, so this PR is no longer strictly necessary.
It may still be useful as a defensive fix, but it may also make sense to close it. I’ll leave that decision to the maintainers.

Either way, thanks for tracking this down and getting the CLI fix out so quickly.

@Ce1ling

Ce1ling commented Sep 2, 2026

Copy link
Copy Markdown
Author

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.
Could you please take another look and let me know whether this should be merged, or if any changes are needed?

@johangp

johangp commented Sep 2, 2026

Copy link
Copy Markdown

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 sylvesterkaczmarek 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.

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?

@Ce1ling

Ce1ling commented Sep 3, 2026

Copy link
Copy Markdown
Author

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.
The drain is bounded to 1 second so a stuck descendant or runner backpressure cannot hang the action indefinitely.

pnpm run check and the full test suite are passing. Could you take another look?

@sylvesterkaczmarek sylvesterkaczmarek 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.

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.

@Ce1ling

Ce1ling commented Sep 9, 2026

Copy link
Copy Markdown
Author

Thanks for the review. The PR is ready from my side — please merge when convenient.

pull Bot pushed a commit to oogalieboogalie/cli that referenced this pull request Sep 9, 2026
…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.
pull Bot pushed a commit to chizee/cli that referenced this pull request Sep 9, 2026
…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 nicolas-bourdeau 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.

🔴 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.

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.

v1.12: Linux run never returns after the turn completes; job dies on timeout with the output file already written

4 participants