Skip to content

fix(guardian): prevent approval review stack overflows - #26663

Closed
fcoury-oai wants to merge 1 commit into
mainfrom
fcoury/guardian-approval-stack-fix
Closed

fix(guardian): prevent approval review stack overflows#26663
fcoury-oai wants to merge 1 commit into
mainfrom
fcoury/guardian-approval-stack-fix

Conversation

@fcoury-oai

@fcoury-oai fcoury-oai commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Why

Guardian approval review can abort the entire Codex process with a stack overflow while opening a permission request.

This was reproduced on macOS through the multi-directory/worktree flow:

  1. Start Codex in an existing worktree.
  2. Ask Codex to create a sibling worktree and continue working there.
  3. The git worktree add succeeds.
  4. Codex requests permission to switch the session working directory into the new sibling worktree.
  5. While the TUI displays Reviewing approval request, the process aborts:
Reviewing approval request
  permission request: switch this session's working directory to `.../codex-rs`

error: Recipe `codex` was terminated on line 17 by signal 6

The crash happens while Guardian materializes and loads its nested review session, before the permission request can complete. A Rust stack overflow aborts the process rather than returning a recoverable Guardian denial, so the user loses the active Codex process even though the worktree itself was created successfully.

Why the production stack configuration does not prevent this

Codex already gives its normal production execution paths larger stacks:

Rust currently gives ordinary spawned threads a 2 MiB default stack on Tier-1 platforms unless RUST_MIN_STACK or std::thread::Builder::stack_size overrides it. A newly spawned Guardian thread does not inherit the 16 MiB allocation of codex-main or the Tokio worker that created it. Release mode and installation through the npm wrapper therefore do not remove this failure mode.

The Guardian thread also builds a current-thread Tokio runtime and drives the complete nested approval-review future on that same OS stack. That makes this thread boundary the appropriate place to define the stack requirement explicitly.

What Changed

  • Replace std::thread::spawn with std::thread::Builder for Guardian approval reviews.
  • Give the review thread an explicit 4 MiB stack.
  • Name it guardian-approval-request-review so panic and native-thread diagnostics identify the failing subsystem.
  • Preserve the current-thread Tokio runtime and the existing review/cancellation behavior.
  • Preserve fail-closed behavior:
    • if runtime construction fails, Guardian sends ReviewDecision::Denied as before;
    • if the OS thread cannot be spawned, dropping the sender closes the oneshot channel, which callers already treat as denial.

This does not change Guardian policy, permission semantics, session reuse, approval decisions, workspace mutation behavior, thread count, or the npm launcher. It only changes the stack budget and diagnostic name of the existing per-review thread.

Why 4 MiB

Four MiB doubles Rust's current Tier-1 spawned-thread default while keeping the change local to the thread that has the demonstrated production requirement. It also matches the stack size already used by the Guardian parallel-review regression harness.

Using Builder::stack_size is preferable to relying on RUST_MIN_STACK here because it documents the requirement at the ownership boundary and does not increase the stack allocation of every Rust-created thread in the process. It also behaves consistently for source builds and packaged native binaries.

The existing code already creates one OS thread per approval review. This PR does not increase concurrency or thread lifetime; it only raises that thread's stack reservation from the platform/Rust default to 4 MiB.

Prior Art

  • #17846 was merged and approved after the Guardian parallel-review test overflowed its stack. It reduced stack pressure by boxing wrapper futures and gave the affected test an explicitly sized thread. Reviewers questioned whether the test-only stack carveout could be removed after boxing; CI still reproduced without it. That change established that this Guardian path has substantial stack pressure, but it did not size the production approval-review thread addressed here.
  • #19067 was merged and approved to set RUST_MIN_STACK=8388608 for Cargo, nextest, and Bazel test entry points after localized async-future and per-test fixes kept recurring. That protects test-runner threads, not production Guardian threads.
  • #25847 was merged and approved to run codex-main on the same 16 MiB stack budget as Tokio workers after /clear could overflow the smaller process-main stack. It is the closest production precedent for making a stack-heavy execution boundary explicit. The Guardian review thread is created below that boundary and therefore needs its own setting.
  • #25177 is an open draft addressing another production SIGABRT class by moving config loading off a deep TUI caller stack. It concerns a different call path and does not cover Guardian approval review.
  • #19597 was closed without merge after dedicated 4 MiB wrappers around two TUI tests exposed additional overflowing tests. That approach was abandoned because it was a narrow test-only workaround for a broader test-runner problem. This PR instead targets a single existing production thread boundary where all approval reviews enter Guardian.

How to Test

Manual regression path

  1. Start Codex with Guardian approval review enabled.
  2. Ask Codex to create a sibling Git worktree outside the current writable workspace root and continue working from its codex-rs directory.
  3. Allow the git worktree add operation.
  4. Wait for the permission request to switch the session working directory into the sibling worktree.
  5. Confirm Guardian review completes and the permission request can be answered without a stack-overflow SIGABRT.
  6. Confirm the session continues from the new worktree after approval.

Focused non-happy path

  1. Trigger the same workspace-switch permission request.
  2. Deny or cancel the request.
  3. Confirm Codex remains in the original workspace and continues running; the larger stack must not change denial or cancellation semantics.

Automated validation

  • just test -p codex-core guardian_parallel_reviews_fork_from_last_committed_trunk_history
  • just fix -p codex-core
  • The PR's Cargo, Bazel test, Bazel Clippy, release-build, formatting, argument-comment-lint, and platform jobs are passing on macOS, Linux, and Windows.

No external documentation update is needed because this is an internal runtime reliability fix with no user-facing API or configuration change.

@fcoury-oai fcoury-oai changed the title fix(guardian): increase approval review thread stack fix(guardian): prevent approval review stack overflows Jun 5, 2026
@fcoury-oai
fcoury-oai marked this pull request as ready for review June 5, 2026 19:50
@fcoury-oai
fcoury-oai requested a review from a team as a code owner June 5, 2026 19:50
@github-actions

Copy link
Copy Markdown
Contributor

Closing this pull request because it has had no updates for more than 14 days. If you plan to continue working on it, feel free to reopen or open a new PR.

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.

1 participant