Skip to content

fix(voice): finish transcription iterators when sessions close during setup - #4995

Merged
jbeckwith-oai merged 1 commit into
openai:mainfrom
rioyu123:fix/voice-stt-close-completion
Sep 25, 2026
Merged

jbeckwith-oai merged 1 commit into
openai:mainfrom
rioyu123:fix/voice-stt-close-completion

Conversation

@rioyu123

@rioyu123 rioyu123 commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This pull request fixes a hang when an application closes a streamed transcription session during setup and then waits for its transcript consumer to finish.

After the WebSocket connects, the SDK sends session.update and waits for the response. Calling close() at this point stops the connection and background tasks, but transcribe_turns() can remain blocked because the event processor that normally signals completion has not started yet.

Signal completion from the close path as well as normal event processing, using one shared guard to avoid duplicate completion markers. Task cleanup, transcript delivery, and error selection remain in their existing paths. Public APIs and WebSocket messages are unchanged.

This addresses application-initiated close(), not the server-initiated setup disconnect discussed in #4585. It does not include that PR's setup error propagation changes.

Test plan

  • Added real loopback WebSocket regressions that wait until the server receives session.update, then close the session before session.updated. Both tracing modes reproduce the hang on the base and finish with the fix.
  • Covered repeated close, active-session close, server close, provider errors, and consumer cancellation, including transcript delivery, owned-task completion, and tracing cleanup.
  • Full Voice suite: 230 passed on Windows and Linux. The STT tests also passed with four parallel workers: 32 passed.
  • Full repository verification passed on Linux / Python 3.14.4: formatting, lint, Mypy, Pyright, and 9,693 tests; 33 skipped. No live OpenAI speech calls were made.

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

@seratch
seratch marked this pull request as ready for review September 14, 2026 00:26
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 14, 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-09-14T00:30:19.408039Z e81f829 Draft marked ready
🔒 Security Review ✅ Completed 2026-09-14T00:31:01.697404Z e81f829 Draft marked ready
ℹ️ 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.

@github-actions

Copy link
Copy Markdown
Contributor

This PR is stale because it has been open for 10 days with no activity.

@github-actions github-actions Bot added the stale label Sep 24, 2026
@rioyu123

Copy link
Copy Markdown
Contributor Author

Still relevant: current main still doesn't signal completion on the close path. I reran tests/voice/test_openai_stt.py (32 tests) against this PR's head and all pass. The change remains scoped to application-initiated close during setup. Could this stay open for review?

@github-actions github-actions Bot removed the stale label Sep 25, 2026

@jbeckwith-oai jbeckwith-oai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the complete diff and the setup/close lifecycle. Closing during setup can leave the transcript consumer waiting after its producer tasks stop; the guarded completion signal after cleanup fixes that path without changing the public API or protocol handling. The controlled setup regression and terminal-path coverage fit the scope. No blocking findings. Tests were inspected, not executed locally; the current CI checks are green.

@jbeckwith-oai
jbeckwith-oai merged commit 60ed116 into openai:main Sep 25, 2026
21 checks passed
@github-actions github-actions Bot mentioned this pull request Sep 28, 2026
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