fix(voice): clean up tasks when streams close early

[GautamSharma99]

This pull request fixes voice background-task and tracing cleanup when a consumer explicitly closes or cancels a voice async iterator.

Closes #4051.

Problem

StreamedAudioResult.stream() and OpenAISTTTranscriptionSession.transcribe_turns() previously ran cleanup only after normal iterator exhaustion or from cancellation handling around a queue read. Calling aclose() while either concrete async generator was suspended at a successful yield skipped trailing cleanup.

The cleanup helpers also requested task cancellation without draining owned work. Callers could therefore return before producer, dispatcher, synthesis, websocket, listener, or STT processing tasks had run their finalizers.

Two additional ordering hazards existed:

• STT cleanup finished the active span before awaiting WebSocket close, allowing event processing to start a replacement span during that await.
• StreamedAudioResult.stream() recorded session_ended only after yielding it. Immediate aclose() injected GeneratorExit before the flag changed, so cleanup cancelled the producer rather than allowing normal trace and session finalization.

Finally, attaching a secondary cleanup exception to a warning could render its implicit context, including the primary consumer traceback, when diagnostic logging was enabled.

Changes

• Run iterator cleanup from one finalization boundary for normal exhaustion, consumer cancellation, and explicit aclose().
• Record terminal session_ended delivery before yielding the event.
• During finalization after terminal delivery, shield the pipeline producer until it completes normally, then drain remaining owned tasks.
• Convert cleanup helpers to async cancel-and-drain operations and exclude the current cleanup task.
• Close the STT transport, cancel and drain session-owned work, and only then finish the final active transcription span.
• Preserve output-queue task_done() accounting when the STT iterator closes at a yield.
• Preserve an active consumer cancellation or exception when finalization fails.
• Emit only a fixed operational warning for secondary cleanup failure, without attaching the cleanup exception, arguments, traceback, or implicit consumer context.

Regression coverage

Controlled asyncio.Event interleavings assert that:

• explicit close after a regular yielded event cancels and settles all owned tasks;
• explicit close immediately after receiving session_ended remains pending until the producer completes normally and does not cancel it;
• a transcription completion processed while WebSocket close is suspended cannot leave a replacement span unfinished;
• cleanup failure cannot replace CancelledError or an exception injected at a successful yield;
• cleanup-failure warnings remain fixed static records even with diagnostic model logging enabled; and
• STT websocket, task, tracing, and output-queue ownership remains deterministic.

Scope

The deterministic cleanup guarantee covers normal exhaustion, cancellation, and explicit aclose(). A bare async for break does not synchronously await async-generator close, so this pull request does not claim synchronous cleanup at the point of a bare break.

Compatibility

This is an internal lifecycle fix against the v0.19.1 behavior boundary. It does not change public signatures, event formats, provider payloads, configuration, or persisted state.

Validation

• make format
• make lint
• make typecheck
• make tests
• Focused voice and STT suite: 53 passed
• Sensitive-logging collector tests: 10 passed

[seratch]

Thanks for the contribution. The underlying lifecycle bug is real, and wrapping the iterator lifecycles in try/finally while draining owned tasks is the right direction.

Before we can merge this, please make the STT shutdown ordering ownership-safe. close() currently finishes the active span before awaiting WebSocket close; while that await is suspended, _handle_events() can process a queued transcription completion and start a new span that remains unfinished when the tasks are cancelled. Please stop and drain the session-owned work before the final span cleanup, and add a controlled interleaving test for this ordering.

Please also preserve the original consumer cancellation or exception if cleanup fails, with a negative-path regression test. Scope the guarantee to explicit aclose(), cancellation, and normal exhaustion; a bare async for break does not synchronously await generator close.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5a4bcfac3f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[GautamSharma99]

Addressed the first review feedback in d340172:

• STT shutdown now closes the transport, drains all session-owned tasks, and only then performs final span cleanup.
• Added a controlled interleaving regression where _handle_events() starts a replacement span while WebSocket close is suspended.
• Cleanup and cleanup-warning failures no longer replace an active consumer cancellation or exception, with negative-path and redaction coverage.
• The PR scope now explicitly covers normal exhaustion, cancellation, and explicit aclose(), and does not claim synchronous cleanup for a bare async for break.

The focused voice/STT suite passes (55 tests), and the full format, lint, typecheck, and test stack passes locally.

[GautamSharma99]

@seratch
Thanks for the detailed review. I’ve addressed the requested changes in d340172:

• STT shutdown now closes the transport, stops and drains all session-owned tasks, and only then performs final span cleanup.
• Added a controlled interleaving test where _handle_events() starts a replacement span while WebSocket close is suspended.
• Cleanup failures no longer replace the original consumer cancellation or exception.
• Added negative-path coverage for cleanup and logging failures, including redacted and diagnostic logging modes.
• Clarified that the cleanup guarantee covers normal exhaustion, cancellation, and explicit aclose(), but not synchronous cleanup after a bare async for break.

The focused voice/STT suite passes with 55 tests, and the full format, lint, typecheck, and test suite passes locally. Could you please take another look?

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d340172737

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[seratch]

Thanks for the update. The STT shutdown ordering, replacement-span interleaving coverage, exception preservation, and clarified aclose() scope now address the previous review.

One lifecycle regression remains. StreamedAudioResult.stream() records session_ended only after yielding it. If the consumer immediately calls aclose(), GeneratorExit enters the exception cleanup path before saw_session_end is set, so the producer is cancelled instead of being allowed to finish normally. Please record terminal delivery before the yield, preserve graceful producer completion during finalization, and add a controlled regression test for this exact ordering.

Also, when cleanup fails while a primary consumer exception is active, please log only a static warning without attaching the cleanup exception. Its implicit __context__ otherwise causes verbose logging to include the primary consumer traceback. Keep this as one simplified finalization path rather than adding another logging mode.

[GautamSharma99]

Thanks for catching these remaining cases. Addressed in a1f77082:

• session_ended is now recorded before the generator yields it.
• Result finalization shields the producer after terminal delivery, then drains owned tasks from the same finally boundary.
• Added a controlled regression that consumes session_ended, immediately calls aclose(), verifies close waits for the producer, and verifies the producer completes without cancellation.
• Secondary cleanup failures now emit only a fixed logger.warning(...) message. No cleanup exception, arguments, exc_info, traceback, or implicit consumer context is attached, even when diagnostic model logging is enabled.
• Simplified both iterator implementations to one cleanup path while retaining primary cancellation and exception precedence.

The focused voice and STT suite passes with 52 tests, the sensitive-logging collector tests pass, and the full format, lint, typecheck, and test stack passes locally.

[seratch]

Can you resolve the conflicts as well?

[GautamSharma99]

Merged the current upstream main into this branch and resolved the test overlap with #4061 by retaining both its idle-dispatcher regression and this PR’s lifecycle coverage. Post-merge validation is clean: the focused voice/STT suite passes all 53 tests, and the full formatting, lint, type-check, and test stack passes.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a4a009a097

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[seratch]

Can you fix all the review comments by codex first?

[GautamSharma99]

Thanks for the review — both Codex comments are fixed in 876e263.

Root cause. Both cleanup paths (StreamedAudioResult.stream() and OpenAISTTTranscriptionSession.transcribe_turns()) run cleanup in a finally that swallows secondary exceptions to preserve the primary consumer exception. That also swallowed asyncio.CancelledError: when a caller cancels aclose() (or wraps it in asyncio.wait_for) while cleanup is still awaiting — shielding the producer after session_ended, or closing the STT websocket/draining tasks — the injected GeneratorExit had already set primary_exception_active = True, so the later CancelledError was treated as secondary and dropped. The caller then observed a successful close even though cleanup was cancelled.

Fix. The cleanup handler now always re-raises CancelledError, while still swallowing non-cancellation cleanup errors to preserve the primary exception:

except BaseException as cleanup_exception:
    if isinstance(cleanup_exception, asyncio.CancelledError) or (
        not primary_exception_active
    ):
        raise
    # log + swallow non-cancellation cleanup error

Tests. Added a regression for each path that cancels cleanup with a primary exception active and asserts the cancellation propagates. Both fail before this change (the STT one surfaced ValueError instead of CancelledError) and pass now.

make format, make lint, make typecheck, and make tests all pass.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 876e263a1a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Surface terminal producer errors on close

When a consumer stops as soon as it receives session_ended and calls aclose()/breaks, this treats the injected GeneratorExit as the primary exception. If the producer or a TTS task has just queued session_ended and then fails, the post-finally _check_errors() is skipped and any shielded producer exception is suppressed as a cleanup failure, so the caller observes a successful close even though stream() is the public terminal error boundary. GeneratorExit after a delivered terminal event should not hide those producer/task errors.

AGENTS.md reference: AGENTS.md:L126-L126

Useful? React with 👍 / 👎.

[seratch]

Can you resolve the conflicts and remaining review comments?

[AAliKKhan]

Hi @GautamSharma99 and @seratch,

I completed the remaining work for this PR:

1. Codex P2 "Surface terminal producer errors on close" is fixed. When a consumer closes the stream right after session_ended, the injected GeneratorExit no longer hides a producer/task error that surfaces during finalization: stream() now tracks the primary exception object and re-raises the cleanup exception when the primary is GeneratorExit on the terminal-delivery path, so aclose() reports the real producer outcome. A controlled regression covers this.

2. Merged current upstream/main and resolved the src/agents/voice/result.py conflict (kept this PR's stream() terminal-error handling; adopted upstream's _cleanup_tasks() finalization ordering). tests/voice/test_pipeline.py merged cleanly.

Verification: tests/voice/test_pipeline.py + tests/voice/test_openai_stt.py = 57 passed; ruff check/format and mypy (voice) clean. The new regression fails without the fix and passes with it.

I don't have push access to this fork, so the changes are in a PR against fix/voice-stream-cleanup:

GautamSharma99#1

If you merge that into fix/voice-stream-cleanup, this PR will pick up the completed work and the remaining review items (terminal-error surfacing, cleanup ordering, conflicts) will be resolved here.

[seratch]

Thanks again for working on this; it is merged as #4131