Skip to content

fix: close model providers created by Runner - #4785

Open
Excelius-Wang wants to merge 2 commits into
openai:mainfrom
Excelius-Wang:fix/close-runner-owned-model-provider
Open

fix: close model providers created by Runner#4785
Excelius-Wang wants to merge 2 commits into
openai:mainfrom
Excelius-Wang:fix/close-runner-owned-model-provider

Conversation

@Excelius-Wang

Copy link
Copy Markdown
Contributor

Summary

This pull request fixes a resource leak when Runner creates the default model provider for a
single run. Direct Responses WebSocket runs could finish successfully while the provider retained
its cached connection because the existing provider aclose() chain was never called.

Runner now closes only providers it creates implicitly after async, sync, or streamed execution
settles. A caller-supplied RunConfig or model_provider remains caller-owned and reusable, and
cleanup preserves the primary result, exception, and cancellation behavior.

Test plan

  • Added lifecycle coverage for implicit and explicit provider ownership, sync and async runs,
    streamed completion, cancellation, repeated cancellation, cancel-before-start, cleanup failure,
    sandbox wrapping, and a real loopback Responses WebSocket connection.
  • Ran .agents/skills/code-change-verification/scripts/run.sh: format, lint, mypy, pyright, and the
    full test suite passed (9301 passed, 29 skipped).

Issue number

N/A

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

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-08-31T03:24:21.727741Z 99853c0 New commits
🔒 Security Review Completed 2026-08-31T03:25:41.414262Z 99853c0 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@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: 55c0e3874f

ℹ️ 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 src/agents/run.py Outdated
Comment on lines +550 to +552
def _normalize_run_config_for_runner(
value: RunConfig | dict[str, Any] | None,
) -> tuple[RunConfig, bool]:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Move provider cleanup policy into run_internal

Move the new ownership normalization and cancellation-resistant provider cleanup helpers into src/agents/run_internal/ and leave only their orchestration calls here. These functions introduce substantive resource-lifecycle policy directly into the public runtime entrypoint, contrary to the repository requirement that new runtime logic live under run_internal; keeping them here further concentrates lifecycle behavior in the already-large run.py.

AGENTS.md reference: AGENTS.md:L190-L191

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — moved the run-config ownership normalization and cancellation-resistant provider cleanup helpers into src/agents/run_internal/model_provider_lifecycle.py. src/agents/run.py now retains only the orchestration imports and call sites, with behavior and coverage unchanged.

@seratch seratch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the ownership normalization and cancellation-resistant provider cleanup policy out of run.py and into run_internal/, leaving only orchestration calls in the public runtime entrypoint.

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the current head after the lifecycle policy was moved under run_internal. I traced implicit provider ownership through AgentRunner.run() and run_streamed(), then through streamed completion/cancellation in RunResultStreaming: non-streaming cleanup is protected by finally, streamed cleanup is registered against run-loop completion and awaited through the same one-shot cleanup task, and caller-supplied RunConfig / model_provider instances remain caller-owned. _close_runner_owned_model_provider() also preserves caller cancellation while preventing provider-close failures from replacing the primary run result/error. The focused coverage exercises normal async/sync completion, explicit ownership, cancellation/repeated cancellation, cancel-before-start, cleanup failure, and a real Responses WebSocket close. I don't see a blocking lifecycle issue in the current revision.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants