fix(run): cancel sibling work when a concurrent run-path task fails by PranavMishra28 · Pull Request #4142 · openai/openai-agents-python

PranavMishra28 · 2026-08-03T06:02:24Z

Summary

#4005 established that a failing branch of a concurrent fan-out must not leave its siblings running, and shipped gather_with_cancel (util/_asyncio_tasks.py) for it. That fix was applied to enablement checks. The run path still used bare asyncio.gather, which propagates the first exception while the siblings keep executing, detached, after the run has already unwound.

The sharpest instance is _execute_tool_plan (run_internal/tool_planning.py), which gathers six tool-execution categories with no surrounding try:

) = await asyncio.gather(
    execute_function_tool_calls(...),
    execute_computer_actions(...),
    execute_custom_tool_calls(...),
    execute_shell_calls(...),
    execute_apply_patch_calls(...),
    execute_local_shell_calls(...),
)

If execute_computer_actions raises — e.g. UserError("Computer tool safety check was not acknowledged") — the shell and apply-patch arms are not cancelled, so commands run and patches are written to disk after the run has failed. It also bypasses _FunctionToolBatchExecutor._raise_failure_after_draining_siblings, which exists to prevent exactly this one level down, because the failure originates in a sibling gather arm rather than inside the batch.

The inconsistency is concrete for one specific pair of user-supplied callables: realtime/openai_realtime.py gathers agent.get_system_prompt(...) with gather_with_cancel, while both run-path turn functions (run_single_turn and run_single_turn_streamed) gathered the same call with a bare asyncio.gather. Separately, run.py hand-rolls cancel-and-drain inline for the guardrail/model race, with a comment describing the invariant. So the rule is applied in three places in the codebase and is absent from run_internal/.

This switches the 25 run_internal/ gathers that lack return_exceptions=True to the existing helper — the tool-execution fan-out, the two prompt-resolution pairs, and the lifecycle-hook pairs. Mechanical call-site change, no new abstraction, no behavior change on the success path. execute_mcp_approval_requests wraps the result in list() because gather_with_cancel returns a tuple and the function declares -> list[RunItem].

Deliberately unchanged: run.py's guardrail/model race (already cancels and drains, and its tripwire branch cancels conditionally — a mechanical swap would change semantics); the 7 voice/ and sandbox/ gathers (separate subsystems); and every gather(..., return_exceptions=True) call, which already drains rather than orphaning.

Test plan

Two regression tests in tests/test_agent_prompt.py, one per loop, following the pattern #4005 established in tests/test_handoff_tool.py (three asyncio.Events: slow_started / slow_cancelled / slow_finished).

An agent is given an async instructions callable that blocks forever and records whether it was cancelled, plus an async prompt callable that waits for the first to start and then raises. Since get_system_prompt and get_prompt are gathered concurrently, the failing prompt must cancel the in-flight instructions callable:

test_run_cancels_sibling_instructions_when_prompt_resolution_fails — non-streamed loop
test_run_streamed_cancels_sibling_instructions_when_prompt_resolution_fails — streamed loop

Both assert slow_cancelled.is_set() and slow_finished.is_set(). On main both fail with assert False on slow_cancelled — the callable is still running when the run raises. With this change both pass.

Verification (mandatory local run order from AGENTS.md):

make format     # 847 files left unchanged
make lint       # All checks passed!
make typecheck  # mypy + pyright, exit 0
make tests      # 6223 passed, 4 skipped  /  38 passed, 5 skipped (serial)

Issue number

N/A — searched open and closed issues and PRs for gather_with_cancel, asyncio.gather, noop_coroutine, sibling, and on_tool_start; the only related hits are #4005 (merged, established the helper) and #3406 (adds an on_tool_progress hook, which introduces another instance of the pattern rather than changing it). No duplicate found.

Checks

I've added new tests, if relevant
I've run .agents/skills/code-change-verification/scripts/run.sh
I've confirmed all verification steps pass
If using Codex, I've run /review before submitting this PR

Developed with Claude Code; reviewed and tested by Pranav before marking ready for review.

openai#4005 established that a failing branch of a concurrent fan-out must not leave its siblings running, and shipped `gather_with_cancel` for it. That fix was applied to enablement checks; the run path still used bare `asyncio.gather`, which propagates the first exception while siblings keep executing detached. The sharpest case is `_execute_tool_plan`, which gathers six tool-execution categories with no surrounding try. When one arm raises — for example `execute_computer_actions` on an unacknowledged safety check — the shell and apply-patch arms keep running, so commands execute and patches are written after the run has already failed. This also bypasses the batch executor's own cancel-and-drain logic, because the failure originates in a sibling arm rather than inside the batch. The same pair of user-supplied callables makes the inconsistency concrete: `realtime/openai_realtime.py` gathers `agent.get_system_prompt(...)` with `gather_with_cancel`, while both run-path turn functions gathered the same call with a bare `asyncio.gather`. `run.py` hand-rolls cancel-and-drain inline for the guardrail/model race, so the invariant is applied in three places and absent from `run_internal/`. Switch the 25 `run_internal/` gathers that lack `return_exceptions=True` to the existing helper. No new abstraction, no behavior change on the success path; `execute_mcp_approval_requests` wraps the result in `list()` to keep its declared return type. Deliberately unchanged: `run.py`'s guardrail/model race, which already cancels and drains and whose tripwire branch has distinct semantics; and the `voice/` and `sandbox/` gathers, which are separate subsystems.

seratch

Thanks for the contribution. The cancel-and-drain direction and the use of the existing helper look sound.

Before we merge this, please add one controlled cross-category regression test at the _execute_tool_plan boundary. Have an async shell or apply-patch operation start and block, then make another category fail, for example through a rejected computer safety check. The test should verify that the original exception is preserved, the sibling receives cancellation, and its finally block completes before the run returns.

The current prompt tests cover the duplicated prompt-resolution paths, but they do not lock down the side-effecting tool boundary that motivates the PR. One test at the shared planning boundary is sufficient; a per-call-site matrix is not needed.

seratch · 2026-08-03T06:20:27Z

also, please fix the errors of make typecheck

PranavMishra28 · 2026-08-03T06:22:08Z

yep on it rn just saw the failed ci as well

…oads Review feedback from openai#4142. `gather_with_cancel` only declared overloads for two and three awaitables, so the six-argument call in `_execute_tool_plan` matched the variadic `*awaitables: Awaitable[T]` overload and every unpacked element came back as the union of all six result types instead of its own. That is the `make typecheck` failure at tool_planning.py:677. Add overloads for four, five and six, matching the shape typeshed uses for `asyncio.gather`. Add one regression test at the shared planning boundary rather than per call site. A shell executor sets `shell_started`, blocks, records cancellation and sets `shell_finished` in its `finally`; the computer tool's `on_safety_check` waits on `shell_started` before rejecting, so the safety-check failure is guaranteed to land while the shell arm is in flight instead of racing it. The test asserts the original `UserError` propagates and that the sibling was cancelled and finished unwinding by the time `_execute_tool_plan` returns.

PranavMishra28 · 2026-08-03T06:48:55Z

both pushed.

typecheck was my fault, and the cause is a bit non obvious: gather_with_cancel only declares overloads for 2 and 3 awaitables, so the 6 arg call in _execute_tool_plan fell through to the variadic *awaitables: Awaitable[T] overload and every unpacked element came back as the union of all six result types instead of its own. that is the return type mismatch at tool_planning.py:677. added 4/5/6 overloads so it matches the shape typeshed uses for asyncio.gather.

for the test, test_execute_tool_plan_cancels_sibling_category_on_failure in test_run_step_execution.py. shell executor sets shell_started, blocks, records cancellation, sets shell_finished in its finally. the computer tool on_safety_check awaits shell_started before returning False, so the rejection is guaranteed to land while the shell arm is genuinely in flight instead of racing it. asserts the original UserError propagates and that both events are set by the time _execute_tool_plan returns.

it fails on main with "sibling shell command was not cancelled", so it pins the planning boundary rather than just the helper. kept it to the one test, no per call site matrix.

green locally: lint, typecheck (mypy + pyright, 835 files), and 6232 + 38 tests.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 67346bee48

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

PranavMishra28 · 2026-08-03T07:08:46Z

One scope question while you are in it, since it borders on what you asked me to trim. There are 7 remaining bare asyncio.gather calls without return_exceptions=True outside run_internal/: 4 under sandbox/ and 2 under voice/, plus run.py where the guardrail/model race already hand-rolls cancel-and-drain inline. I left all of those alone deliberately to keep this to the run path. Happy to leave them, or to open a separate PR for the sandbox/ and voice/ ones if you think the same invariant should hold there.

seratch · 2026-08-03T08:24:19Z

Thanks for checking. Please leave those outside this PR, and no follow-up PR is needed based on the call-site count alone.

run.py already has intentional cancel-and-drain semantics, and the remaining bare gather in sandbox/materialization.py is covered by its outer cancellation-and-drain block. The other sandbox and voice call sites have different ownership and terminal-event semantics, so we should only change them if we can demonstrate a concrete failure and cover it with a focused regression test.

Let's keep this PR scoped to run_internal/.

_FunctionToolBatchExecutor spawns a task per function-tool call. On external cancellation it cancelled those tasks, attached loop-level result callbacks and returned immediately. That is correct for parent cancellation, which must not wait on tool cleanup, but it is wrong for the cross-category case: the _execute_tool_plan fan-out treats the arm's return as "this category is drained", so a handler was still mid-unwind (and still landing side effects) after the run had already raised. gather_with_cancel now records whether it is the one cancelling, so an arm can tell a failing sibling from an ancestor going away, and the executor waits a bounded 0.25s for its cancelled handlers only in the sibling-failure case. Tests: the new drain test fails without the executor change; the negative test pins that the flag stays False outside a failing gather, which is what keeps test_parent_cancellation_does_not_wait_for_tool_cleanup passing.

PranavMishra28 · 2026-08-03T14:47:37Z

you're right, it wasn't. i had read that finding as already covered by the sibling cancel and it isn't, so thanks for holding it. reproduced it before touching anything:

fn_cancelled: True fn_finished: False

that's a mixed plan with one function tool and one computer action whose safety check rejects once the function tool is in flight. the function tool observes cancellation but has not finished unwinding by the time _execute_tool_plan raises.

the mechanism is the one you flagged. _FunctionToolBatchExecutor.execute catches the external CancelledError, calls _cancel_pending_tasks_for_parent_cancellation (tool_execution.py:1519), and that method cancels the per-call tasks, attaches the loop-level result callbacks, and returns without awaiting them. so the arm coroutine finishes while its handlers are still running, and gather_with_cancel's drain sees a completed arm and re-raises.

what made this more than a one-liner: my first attempt just awaited the cancelled tasks there, and that broke test_parent_cancellation_does_not_wait_for_tool_cleanup. that test pins the opposite requirement, parent cancellation must return within 0.1s while cleanup is still blocked, and it's right to. the two cases arrive identically as a CancelledError from outside, so there was nothing at the catch site to branch on.

so gather_with_cancel now says which one it is. it puts a small mutable scope in a contextvar before creating the arm tasks, so the arms inherit a reference to the same object, and sets cancelling = True only when the exception that ended the gather is not itself a CancelledError:

except BaseException as error:
    scope.cancelling = not isinstance(error, asyncio.CancelledError)

a real sibling failure is a non-cancellation exception, so it drains. an ancestor cancelling the gather, or an arm being cancelled directly, leaves the flag False and keeps today's prompt teardown. the executor waits a bounded 0.25s and only in the sibling-failure case; the callbacks are still attached first so anything outliving the window is still reported rather than dropped.

two tests. test_execute_tool_plan_drains_function_tools_on_sibling_failure is the repro above and fails without the executor change. test_sibling_failure_drain_does_not_apply_to_parent_cancellation pins the flag directly, False while running, True once a failing sibling is the cause, False outside any gather, which is the property the existing parent-cancellation test depends on.

on the other finding in that review, the terminal-event one on any_llm_model.py, that's #4143's file and i'll answer it there rather than split it across two threads.

full gate green locally, both mypy and pyright, 6237 tests. and understood on scope, i've left sandbox/ and voice/ alone.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c10e0ad87e

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

PranavMishra28 · 2026-08-03T14:56:19Z

one thing that needs you rather than me: the Tests workflow on both this and #4143 is sitting at action_required, so the new pushes have no check runs at all yet. earlier pushes on both PRs ran fine and went 9/9, so this looks like the outside-contributor approval gate re-arming rather than anything wrong with the commits. whenever you approve the runs the results should show up on their own, no push needed from me.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1f79f1da9b

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

…pe, and keep post-invoke work Two follow-ups on the previous commit. The `not isinstance(error, CancelledError)` discriminator was wrong for an arm that raises CancelledError itself, which a shell, computer or custom executor does to report tool-local cancellation. The caller is not going away in that case, so the other arms still owe the fan-out a drained result, but the flag stayed false and they took the prompt teardown path. gather_with_cancel now waits on FIRST_COMPLETED and decides from which arm finished: an arm that ends badly comes back as a finished task, which proves this frame is alive and the cancellation originates here, while an ancestor cancelling us raises out of the wait itself. FIRST_EXCEPTION cannot be used because a cancelled task does not satisfy it, which is what let the old classification through. The cross-category drain also cancelled tasks that had already left the handler and entered their post-invoke phase (output guardrails, custom output extraction, on_tool_end). That lands the tool's side effect while skipping the lifecycle work the tool contract requires after it. It now uses the same _partition_pending_tasks split the in-batch sibling-failure path uses, and only for sibling failure; a parent cancellation is tearing the run down, so there is no lifecycle left to protect. Tests: four new cases. The arm-raised CancelledError case and the post-invoke case both fail without their respective changes; the other two pin the negative (ancestor cancellation stays classified as such) and positional result ordering through the new wait loop.

…ang the run gather_with_cancel drained cancelled arms with an unbounded await, so a shell, custom, computer, apply-patch or local-shell executor that catches CancelledError and keeps cleaning up stopped the failure that triggered the cancellation from ever propagating. Reproduced directly: the run hung and the sibling's RuntimeError never surfaced. The drain is now bounded by _ARM_DRAIN_SECONDS, chosen above the function-tool teardown window so an arm can still finish its own bounded cleanup. Arms that outlive the window are reported through a done callback rather than waited on, and results of arms that finished inside it are retrieved so a teardown failure is not logged as an exception that was never retrieved. Test uses the real drain window rather than a patched one, so on an unbounded drain it fails by timing out, which is the property being pinned.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d9b404fa86

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

chatgpt-codex-connector · 2026-08-03T15:28:09Z

+    error = task.exception()
+    if error is not None:
+        logger.warning("Cancelled concurrent task failed after teardown: %s", error)


Redact late concurrent-task exceptions

When a cancellation-resistant arm—such as a shell/custom tool executor or prompt/model callback—finishes after the drain window with an exception containing user data, this callback stores and renders that exception as a logging argument even though the model/tool data logging flags default to redacted. This can expose secrets in the resulting LogRecord; use the redaction-aware mixed model/tool logging helper and cover both redacted and diagnostic modes.

AGENTS.md reference: AGENTS.md:L95-L95

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-03T15:28:09Z

+                        await _cancel_and_drain(tasks)
+                        task.result()  # re-raises the arm's exception
+        except BaseException:
+            await _cancel_and_drain(tasks)


Avoid cancelling sibling cleanup twice

When a sibling catches the first CancelledError to perform cleanup lasting longer than _ARM_DRAIN_SECONDS, the first drain returns with that task pending, then task.result() raises into this handler and invokes _cancel_and_drain() again. That second call sends another cancellation into the cleanup coroutine, commonly interrupting resource release rather than detaching it with the promised late-result callback. Fresh evidence after the earlier unbounded-drain comment is that the new bounded helper is called both before task.result() and again by the surrounding exception handler; re-raise an already-drained arm without entering the second drain.

AGENTS.md reference: AGENTS.md:L128-L128

Useful? React with 👍 / 👎.

PranavMishra28 added 2 commits August 2, 2026 23:01

Merge branch 'main' into fix/cancel-siblings-run-path

6c6f7d7

PranavMishra28 marked this pull request as ready for review August 3, 2026 06:05

seratch requested changes Aug 3, 2026

View reviewed changes

seratch added the feature:core label Aug 3, 2026

seratch added this to the 0.19.x milestone Aug 3, 2026

PranavMishra28 added 2 commits August 2, 2026 23:41

Merge branch 'main' into fix/cancel-siblings-run-path

b178654

chatgpt-codex-connector Bot reviewed Aug 3, 2026

View reviewed changes

Comment thread src/agents/run_internal/tool_planning.py

PranavMishra28 requested a review from seratch August 3, 2026 07:17

PranavMishra28 mentioned this pull request Aug 3, 2026

fix(models): record model-call failures on the provider's own span #4143

Merged

4 tasks

chatgpt-codex-connector Bot reviewed Aug 3, 2026

View reviewed changes

Comment thread src/agents/util/_asyncio_tasks.py Outdated

Comment thread src/agents/run_internal/tool_execution.py Outdated

Merge branch 'main' into fix/cancel-siblings-run-path

1f79f1d

chatgpt-codex-connector Bot reviewed Aug 3, 2026

View reviewed changes

Comment thread src/agents/run_internal/tool_planning.py

PranavMishra28 added 2 commits August 3, 2026 08:13

chatgpt-codex-connector Bot reviewed Aug 3, 2026

View reviewed changes

Uh oh!

Conversation

PranavMishra28 commented Aug 3, 2026

Summary

Test plan

Issue number

Checks

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

seratch commented Aug 3, 2026

Uh oh!

PranavMishra28 commented Aug 3, 2026

Uh oh!

PranavMishra28 commented Aug 3, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

PranavMishra28 commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seratch commented Aug 3, 2026

Uh oh!

PranavMishra28 commented Aug 3, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

PranavMishra28 commented Aug 3, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

PranavMishra28 commented Aug 3, 2026 •

edited

Loading