Skip to content

fix: keep rescue Bash attached to Codex - #738

Open
fscfede-beep wants to merge 10 commits into
openai:mainfrom
fscfede-beep:fix/rescue-foreground-bash-432
Open

fix: keep rescue Bash attached to Codex#738
fscfede-beep wants to merge 10 commits into
openai:mainfrom
fscfede-beep:fix/rescue-foreground-bash-432

Conversation

@fscfede-beep

@fscfede-beep fscfede-beep commented Sep 5, 2026

Copy link
Copy Markdown

Summary

Fixes #432 and #324.

This PR makes background /codex:rescue durable without requiring one Bash invocation to remain attached for the whole Codex run. The outer rescue layer owns the user-facing execution choice; a background rescue delegates to the companion's detached task runtime and then acts as a bounded waiter/reader.

Current behavior:

  • background rescue launches task --background --json and captures a durable jobId;
  • waits are bounded with status <jobId> --wait --timeout-ms 60000 --json;
  • terminal output is retrieved with result <jobId> --raw, preserving Codex stdout verbatim;
  • queued job/request state is persisted before the detached worker is spawned;
  • task-worker runs only if the stored job is still queued, preventing a pre-start cancellation from being revived;
  • once a background jobId exists, interruption/cancellation of the waiting Agent does not implicitly cancel the detached Codex job; it remains recoverable through status/result until completion or an explicit /codex:cancel;
  • after spawnDetachedTaskWorker, the parent performs no mutable job-file, state.json, or PID-sidecar read/write. This avoids racing a worker that may truncate/rewrite its job JSON while starting;
  • the worker is the sole post-spawn PID-sidecar writer and publishes its own process.pid before its first queued-state read;
  • /codex:cancel claims terminal cancelled state before PID discovery, then terminates/removes the worker PID and reasserts the same terminal record afterward, closing the startup interleavings where cancellation could otherwise report success while a worker continued;
  • early worker exits remove the PID sidecar, tracked execution removes it in finally, cancellation removes it, and state pruning removes orphaned sidecars for dropped jobs.

This incorporates the lifecycle/race issues found during review rather than preserving the original foreground-only inner-Bash design.

Validation

Fresh deterministic validation at head 69484a7:

  • commands / render / state: 13 passed, 0 failed;
  • affected runtime set: 9 passed, 0 failed covering persist-before-spawn, no parent mutable-state access after spawn, worker PID-before-queued-read, cancel-terminal-state-before-PID ordering, cancelled-worker no-revival, detached E2E completion, normal result, raw result, and active cancellation;
  • isolated detached background E2E: PASS;
  • node --check plugins/codex/scripts/codex-companion.mjs: PASS;
  • git diff --check: PASS.

An earlier combined affected-set run on the previous head hit the E2E's 15-second status window under host load; later isolated and full deterministic runs passed. That failed timing run is not represented as PASS.

The upstream Pull Request CI for this branch is checked per head. If the current head reports action_required with 0 jobs created, that is treated as an external workflow-approval/action gate, not a CI test result.

Current scope

GitHub currently reports 8 changed files. The changes cover rescue routing/contracts, durable task runtime/state handling, result rendering behavior, documentation, and regression coverage.

@fscfede-beep
fscfede-beep requested a review from a team September 5, 2026 01:33

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 19810b45ba

ℹ️ 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".

Comment thread plugins/codex/agents/codex-rescue.md Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 76f876af2d

ℹ️ 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".

Comment thread plugins/codex/agents/codex-rescue.md Outdated

@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 updated ownership model makes sense now that background rescue has to survive the host Agent lifecycle: durability belongs to the companion job, while the Agent becomes a waiter/reader instead of trying to keep the Bash process alive. The PDEATHSIG regression is much stronger than a mocked child-process assertion.

One lifecycle edge I would still pin explicitly is cancellation or interruption of the waiting Agent after the detached job has been created. Does that intentionally leave the Codex job running for a later status / result, or should it cancel it? Either behavior can be valid, but it matters because this design deliberately decouples worker lifetime from the Agent.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3e5d8e786b

ℹ️ 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".

Comment thread plugins/codex/commands/rescue.md

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 82279bbe5b

ℹ️ 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".

Comment thread plugins/codex/scripts/codex-companion.mjs

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8574b097fb

ℹ️ 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".

Comment thread plugins/codex/scripts/codex-companion.mjs Outdated

Copy link
Copy Markdown
Author

Lifecycle contract clarification requested in review: once a background rescue has created a durable jobId, interruption/cancellation of the waiting Agent does not implicitly cancel the Codex job. The detached worker remains authoritative and recoverable via status / result; termination requires explicit /codex:cancel. That contract is now documented and regression-pinned in 66c480d. Subsequent race fixes culminate in d1c63b0, where queued request state is persisted before spawn and the startup PID is stored in a separate sidecar rather than rewriting state.json after spawn. Fresh current-head validation: commands/render/state 13/13 PASS; affected background/result/cancel runtime set 7/7 PASS; PID-sidecar smoke and diff check PASS. Upstream PR CI is currently action_required with 0 jobs created.

Copy link
Copy Markdown
Author

@codex review

Please review the current head d1c63b0. The review-driven lifecycle/race findings are resolved on this head: queued request persists before spawn, background output uses result --raw, cancellation cannot revive a pre-cancelled worker, and the startup PID is isolated in a sidecar so the parent does not race state.json after spawn. Fresh deterministic validation recorded in the PR body: commands/render/state 13/13 PASS; affected runtime 7/7 PASS; PID-sidecar smoke PASS; git diff --check PASS. Upstream CI is still action_required with 0 jobs, so this request is specifically for current-head code review.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d1c63b00fe

ℹ️ 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".

Comment thread plugins/codex/scripts/codex-companion.mjs

Copy link
Copy Markdown
Author

@codex review

Please review current head 54cd28d. This head closes the startup/cancel race found on d1c63b0: task-worker now publishes its own PID sidecar before the first queued-state read, so cancellation either leaves a cancelled record the worker observes or can terminate the worker by PID. Fresh deterministic validation: commands/render/state 13/13 PASS; affected runtime set 8/8 PASS; node --check and git diff --check PASS. Upstream CI remains action_required with 0 jobs, so this request is specifically for current-head code review.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 54cd28d96d

ℹ️ 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".

Comment thread plugins/codex/scripts/codex-companion.mjs

Copy link
Copy Markdown
Author

@codex review

Please review current head 35de31d. This head closes the remaining cancel/startup interleaving found on 54cd28d: /codex:cancel now persists terminal cancelled before PID discovery, then terminates/removes the worker PID and reasserts the terminal record afterward. Fresh validation: commands/render/state 13/13 PASS; affected runtime 9/9 PASS; isolated background E2E PASS; node --check and git diff --check PASS. Upstream PR CI run 33998182457 is still action_required with 0 jobs, so this request is specifically for current-head code review.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 35de31d58d

ℹ️ 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".

Comment thread plugins/codex/scripts/codex-companion.mjs Outdated

Copy link
Copy Markdown
Author

@codex review

Please review current head 69484a7. This head closes the post-spawn job-file parsing race found on 35de31d: after spawning the detached worker, the parent no longer reads or writes mutable job JSON/index/PID-sidecar state. The worker is the sole post-spawn PID-sidecar writer, and cancel already claims terminal state before PID discovery. Fresh validation: commands/render/state 13/13 PASS; affected runtime 9/9 PASS; isolated detached background E2E PASS; node --check and git diff --check PASS. Please review this exact head.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: 69484a7439

ℹ️ 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".

fscfede-beep commented Sep 6, 2026

Copy link
Copy Markdown
Author

@openai/developer-experience — contributor-side work on this PR is complete at exact head 69484a7439dd90d49ff10bb1c16a08a37ef09c88: the known lifecycle/race findings are resolved and current-head local validation is documented in the PR body. I verified that the active Main ruleset requires at least one formal approval from the Developer Experience team for *. The repository's classic required-status-check protection is currently off; Actions is still action_required with 0 jobs, so CI has not executed, but I am not representing that as a proven merge requirement here. When convenient, could someone from Developer Experience review the current head? No maintainer approval or CI result is being claimed yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants