Repository navigation
feat(realtime): add opt-in bounded connection recovery - #2968
Conversation
Castiron custom codeEvaluated main: Trusted main policy: ✅ No new custom-code files detected. 47 mixed files remain; 0 existing customizations changed. Compared 47 existing customizations unchanged
7 more in the full report. A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 39107274745 --repo openai/openai-node \
--name castiron-custom-code-39107274745-1 --dir /tmp/castiron-custom-code-39107274745-1
git apply --stat /tmp/castiron-custom-code-39107274745-1/custom-code.patch
cat /tmp/castiron-custom-code-39107274745-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 3a3b8521a9278d68120c9256a0de4075acb82d80 b44832cf963afdea3c8fbda95485005c39f86dd6
python3 scripts/castiron/custom_code_report.py report \
--base 3a3b8521a9278d68120c9256a0de4075acb82d80 \
--head b44832cf963afdea3c8fbda95485005c39f86dd6 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-b44832cf963a
cat /tmp/castiron-custom-code-b44832cf963a/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved at c79d6ff. Two independent static reviews covered all ten changed files and supporting controller, queue, adapter, and credential code. No actionable findings. Recovery remains opt-in, refreshes credentials per attempt, retains never-attempted events for explicit flushing, and preserves legacy defaults. Reviewed close cancellation, stale socket ownership, retry bounds, authentication rejection, and browser/Node wiring. Existing feedback is addressed. Hosted checks inspected pass, including prior Node 22/24/26 results; a newer Node 22 run remains pending. No candidate code or tests executed locally.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c79d6ff5f8
ℹ️ 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".
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Revising my earlier approval after independently validating newly posted feedback in two static passes. Request changes for the existing undefined-serialization finding; no duplicate inline comment added. An otherwise typed event with toJSON() returning undefined is admitted as zero bytes, then draining throws after permanentClosed is set and before close notifications fire. Reject undefined serialization at admission and cover close/takePending with this input. Hosted CI, including the Node 22 rerun and external OkTest, now passes. No candidate code or tests executed locally.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved after two independent static re-reviews of the six-file delta and supporting queue/controller paths. Undefined JSON serialization is rejected before admission, addressing the prior close-notification failure. Both wrappers now use the captured reconnect URL, lifetime attempt reporting is consistent, and delay limits fit the timer range (jitter only reduces the capped delay). Regression coverage exercises both wrappers. No new actionable findings. Hosted Node 26 tests and lint pass; other tests/ecosystem checks remain pending. No candidate code or tests executed locally.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c1fb728e4
ℹ️ 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".
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Changing my decision to request changes after independently validating newly posted feedback with a second static reviewer. The previously fixed serialization/URL/attempt/timer issues remain fixed, but two lifecycle defects remain:
- Initial handshake queue: with recovery and a nonzero queue budget, CONNECTING sends are retained, initial open skips manual flushing, and no reconnected event tells callers to flush. Reject initial-handshake admission or define and implement an initial-open drain path.
- Close exception cleanup: invalid close arguments can leave the physical socket open while the controller announces final close and rejects sends. Ensure fallback transport disposal/detachment before finalizing.
Also reconcile the canonical docs/realtime.md recovery/queue statements with the new opt-in API (existing documentation feedback). These references avoid duplicate inline findings. Hosted SDK tests/OkTest pass; remaining checks are pending. No candidate code or tests executed.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Two independent static re-reviews confirm the three prior findings are fixed: initial-handshake sends are rejected without queue retention; invalid close falls back to transport termination/close and blocks post-close event dispatch; and the canonical guide now documents opt-in recovery and explicit state restoration/queue handling. The additional TLS/header snapshot and browser binary-mode fixes are consistent with the existing adapters. No new actionable findings in the incremental diff and relevant lifecycle context.
Hosted checks currently pass or are skipped. No candidate code or tests executed locally.
There was a problem hiding this comment.
💡 Codex Review
openai-node/src/realtime/ws.ts
Line 227 in 9750c0d
Rebuild custom URLs for async-factory reconnects
When a function-based credential requires OpenAIRealtimeWS.create(), this replacement discards the caller's buildRealtimeURL callback after the initial connection and installs a closure that returns the originally captured URL forever. Consequently, a builder that supplies a rotating endpoint or short-lived signed URL is reevaluated for reconnects made through the constructor or Azure factory but not through create(), and every replacement can retry the expired first URL despite refreshing its credential. Preserve the original builder for subsequent attempts while passing the captured URL only to the initial socket; the native create() factory has the same issue.
AGENTS.md reference: AGENTS.md:L52-L57
ℹ️ 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".
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Revising my earlier approval after independently validating two newly posted findings with a second reviewer. The previous three fixes remain verified.
- Use the deferred credential commit result. The base credential hook intentionally reads the cached accessor-transformed key after assigning a provider token. Initial creation uses that result, but deferred recovery captures the raw token and discards the potentially different value returned by commit(). Use the commit return value when opening the replacement in both wrappers.
- Preserve terminal handshake causes. Permanent HTTP status and TLS errors are reduced to WeakSet membership while replacement errors are suppressed. The final generic 1006 abort loses the detail callers need to fix authentication or certificate configuration. Retain and surface the terminal cause.
Referencing the existing findings without duplicate inline comments. Hosted checks pass or are skipped; no candidate code or tests executed locally.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Two independent static reviews of the complete three-file incremental diff confirm both outstanding findings are fixed. Both reconnect constructors use the value returned by credential.commit(), preserving transformed cached credentials. The ws adapter retains terminal HTTP/TLS diagnostics per socket and supplies the cause to the existing final error report before clearing it on close; transient replacement errors remain suppressed. Tests cover committed credentials in both adapters and a single terminal diagnostic for authentication/TLS failures. No new actionable findings.
Hosted CI is still running. No candidate code or tests executed locally.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6039f103b7
ℹ️ 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".
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Superseding my earlier approval after validating newly posted feedback at the same head. The two previously requested fixes remain correct.
Request changes for the existing credential refresh diagnostic finding: both wrappers await credential preparation, but a rejection enters the unbound catch in ws-connection-controller.ts and is discarded. Once retries are exhausted, onError receives only the generic original-close-code message. Retain the last preparation failure and supply it as the final SDK error cause; cover repeated provider failure for both wrappers while preserving close cancellation and transient retry suppression. Referencing the existing inline finding without duplicating it.
I do not adopt the separate claim that initial TLS failures emit no SDK error: the controller initial-socket error handler forwards err.message and err while not reconnecting, despite the wrapper handler returning.
Two independent static traces confirm the credential-refresh gap. Hosted CI is now complete with all checks passing or skipped. No candidate code or tests executed locally.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Two independent static reviews of the complete three-file incremental diff confirm the remaining credential-refresh diagnostic finding is fixed. The retry loop retains the latest preparation/open failure and supplies it on exhaustion or abort for the opt-in Realtime path; intentional close still suppresses failure reporting and legacy automatic-queue callers retain their error contract. The Node adapter also prefers a recorded terminal HTTP cause over the generic termination error on the existing error-reporting path, preserving the initial 401 without duplicate events. Regression coverage exercises both public wrappers with distinct successive credential failures and initial HTTP rejection. No new actionable findings.
Hosted Node 22 CI/test matrix remains pending; other substantive checks passed. No candidate code or tests executed locally.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Two independent static reviews of the complete incremental diff found no actionable issues. Reconnection now flushes pending automatic-queue commands and invokes the public reconnected hook before notifying stream observers; closing from that hook terminates the iterator without a later reconnected event. Socket listener rebinding remains before notification. The regression covers that ordering; wire-test synchronization remains meaningful. The shortened README points to the canonical guide, which retains the detailed recovery contract. Previous credential/error-diagnostic fixes remain intact.
Hosted CI remains in progress on this head. No candidate code or tests executed locally.
Automated Release PR --- ## [7.39.0](openai/openai-node@v7.38.0...v7.39.0) (2026-10-18) ### Features * **realtime:** add opt-in bounded connection recovery ([openai#2968](openai#2968)) ([64872e5](openai@64872e5)) ### Chores * **api:** use neutral moderation examples ([openai#2969](openai#2969)) ([3a3b852](openai@3a3b852)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: openai-sdks[bot] <284451331+openai-sdks[bot]@users.noreply.github.com>
2 participants
Why
A temporary Realtime connection failure currently requires the application to reconnect. Add an option for the SDK to reconnect while keeping the current behavior by default.
High-level description
Reuse the shared WebSocket controller for bounded reconnection in the
wsand native WebSocket clients. Each attempt refreshes credentials and keeps the same connection object. A replacement socket starts a new server session; it does not restore state or resend attempted commands.Reconnection stays off by default. Model sessions can enable
reconnect: true; transcription sessions and connections to an existing call reject this option. Wait for the firstopenbefore sending. During recovery, callers can set a byte budget to retain unattempted commands, restore state, then explicitly flush or take those commands.Preserve transport options, binary mode, redirect protection and cleanup. Use the final committed credential and preserve terminal handshake errors. The README and Realtime guide explain defaults, retry limits and state restoration. Existing Responses, Translation and Live behavior stays unchanged.
Validation
1735c1704de7bb5ab670cd1f4c900faaa345c472. All review threads are resolved; re-review is requested.Hosted service tests remain on hold. No package publication or merge is part of this PR.