fix(openai): close owned Agents SDK client - #4508

@abhinavkr26104 Thanks for the PR! It doesn't reference an issue yet.

We require an issue for every PR, so the work can be prioritized before it's reviewed. Add one to the description:

• Closes #123 if this PR finishes the issue. That links it, gives your PR the issue's priority, and closes the issue when this merges. You can also link it from the Development section of the sidebar.
• Part of #123 if this is one step towards it. Related to, Towards, and Refs work the same way, and leave the issue open.

No issue exists for this yet? Open one first, then reference it. That's how we track what's worth doing, and it's usually quicker than it sounds. Note a reference has to point at an issue: naming another PR doesn't count.

The only exceptions are changes with no user-visible behaviour: pure Refactor / chore, Docs, or Test / CI work. If that's genuinely what this is, check that box under Type of change. Anything that fixes a bug, adds a feature, or changes the UI needs an issue, even when it also touches docs or tests.

See CONTRIBUTING.md for the full policy.

No action is taken beyond this comment.

@abhinavkr26104 This PR is a Bug fix, Feature, or UI / frontend change but the Demo section is missing or only contains a placeholder.

These change types require a screenshot or screen recording so reviewers can see the new behaviour without checking out the branch. Please update the Demo section with:

• A screenshot or screen recording of the change, or
• A link to a hosted video or GIF showing the new behaviour.

Use N/A only when the change has no user-visible effect whatsoever (e.g. a pure refactor or test-only change). If that's the case, uncheck the relevant type box and check Refactor / chore or Test / CI instead.

/review

Polly AI Review

Blocking issues

None. The change is correct:

• _owns_client = client is None accurately captures ownership — an auto-created client (via _get_openai_async_client) is owned and closed; an injected client is left to the caller.
• close() closes the right object. Even on the use_responses=False path where self._client is _wrap_client_for_reasoning_models(raw_client), that wrapper mutates the client in place (object.__setattr__(client, "chat", ...)) and returns the same object, so await self._client.close() reaches the real client's close(). No wrapper shadows or drops it.
• The new close() overrides the base no-op and is invoked by the adapter shutdown path (_executor_adapter.on_shutdown → close_session then close), so the leak described in [Bug] OpenAI Agents executor does not close its provider HTTP client on shutdown #4504 is actually reached.
• The regression test exercises both ownership paths and asserts close-count 1 vs 0. It passes locally (1 passed).

Security vulnerabilities

None. No new inputs, deserialization, network, or auth surface; this only tightens resource cleanup.

Non-blocking notes

• No super().close() call. Sibling executors (e.g. hermes_executor, claude_sdk_executor) call await super().close() at the end of their override. The base is currently a no-op so this is harmless today, but calling it would be more future-proof if the base close() ever gains behavior. Consider adding await super().close().
• close() isn't best-effort. If self._client.close() raises, it propagates through on_shutdown and could abort teardown before other cleanup. Other executors treat interrupt/close as best-effort (try/except + log). A stray exception here is unlikely for the OpenAI async client, but wrapping the close would harden shutdown.
• _session_states.clear() vs close_session per key. close() clears the dict directly rather than iterating close_session() (as codex/claude do). That's fine here since close_session for this executor only pops from the same dict and sessions hold no separate resources — just noting the divergence in pattern.

Summary

A small, correct, well-scoped bug fix that closes the executor-owned Agents SDK HTTP client on shutdown while leaving injected clients to their caller, with focused regression coverage for both paths. The ownership flag, the wrapped-client close path, and the adapter shutdown wiring all check out, and the new test passes. The only suggestions are optional hardening (call super().close(), make the close best-effort); none block merge. Visual demonstration is not applicable — this is a backend-only resource-cleanup change.

_{Automated review by Polly · workflow run}

Polly AI Review

Summary

This PR correctly closes a real resource leak: OpenAIAgentsSDKExecutor created its own AsyncOpenAI client (via _get_openai_async_client) but inherited the base Executor.close() no-op, so the client's HTTP connection pool was never released. The fix tracks ownership (self._owns_client = client is None), overrides close() to clear session state and close only executor-owned clients, and leaves injected clients to their caller. The logic and the ownership boundary are correct, and the test covers both paths.

Blocking issues

None.

Security vulnerabilities

None. No credentials, network, or trust-boundary changes.

Non-blocking notes

• close() is not idempotent-safe against exceptions. If self._client.close() raises, session state is already cleared but a second close() call would attempt to close the client again. AsyncOpenAI.close() is effectively idempotent, so this is minor — but wrapping the close in a try/finally or contextlib.suppress (as codex_executor / claude_sdk_executor do for their teardown) would make shutdown more robust if the underlying transport is already torn down.
• Test client is a structural stub, not the real type. _ClosableClient has no base_url, which is fine because use_responses=True (default) skips _wrap_client_for_reasoning_models and _is_databricks_openai_client tolerates the missing attribute. Worth noting the injected-client path in the test therefore doesn't exercise the wrapping branch (use_responses=False), where self._client is the wrapped-but-same object — ownership still holds there, so no bug, just untested. Optional to add a use_responses=False case.
• The close() docstring says "the client created by this executor" — accurate; no action needed.

Overall: a correct, minimal, and appropriately tested fix for the described leak. No changes required to merge; the notes above are optional hardening. No visual demonstration is warranted (pure backend resource-management change).

_{Automated review by Polly · workflow run}

🏷️ Doc impact: no-doc-update

Internal lifecycle change adding a close() method to release an executor-owned OpenAI client, with tests — no user-facing surface, integration config, or documented default changed.

_{Auto-classified on merge. Set the label manually before merging to override. · run}