feat(api): add service-account API key expiration fields - #3802
feat(api): add service-account API key expiration fields#3802jbeckwith-oai wants to merge 1 commit into
Conversation
Castiron-Internal-PR: openai/openai-python-internal#84 Castiron-Source-SHA: 6eefcc34b79ca8c3087a3f6ee6fbbaf5b8ac7510 Castiron-Public-Base-SHA: 2a98f6a
Castiron custom code✅ No new custom-code files detected. 36 mixed files remain; 0 existing customizations changed; 5 generated baselines changed. Compared
31 existing customizations unchanged
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 33934453127 --repo openai/openai-python \
--name castiron-custom-code-33934453127-1 --dir /tmp/castiron-custom-code-33934453127-1
git apply --stat /tmp/castiron-custom-code-33934453127-1/custom-code.patch
cat /tmp/castiron-custom-code-33934453127-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 2a98f6a1dee448c6410531c89c2de0af4383c6a7 7863fba58838380e273caf8e320d26b25b261f77
python3 scripts/castiron/custom_code_report.py report \
--base 2a98f6a1dee448c6410531c89c2de0af4383c6a7 \
--head 7863fba58838380e273caf8e320d26b25b261f77 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-7863fba58838
cat /tmp/castiron-custom-code-7863fba58838/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
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. |
![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: 7863fba588
ℹ️ 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".
| if isinstance(event_type, str) and event_type and event_type != "error": | ||
| self._reconnect_attempt = 0 |
There was a problem hiding this comment.
When a successful reconnect is followed by a normal request-level error event, this condition leaves the previous attempt count active even though RealtimeErrorEvent explicitly documents that most errors are recoverable and the session remains open. If that healthy session later experiences an unrelated network disconnect before producing a non-error event, reconnection starts from the stale count and can eventually be refused after several independent incidents. Distinguish admission failures from recoverable application errors instead of treating every type == "error" event as a failed reconnect; the same condition is duplicated in the sync and Responses connection implementations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I checked the actual sync/async Realtime, Responses, and beta Responses connection classes. For Realtime, session.created is the first event on an established connection; it resets the count before later recoverable errors. A probe starting at the retry limit, receiving that event and then a request error, reconnects at attempt 1 after a later network close.
For a Responses connection that has received only errors since reconnecting, your observation about retaining the count is correct. This guard currently requires a non-error application event (such as response.created) to replenish the budget. Error events remain available to the caller and do not themselves close the connection or consume another retry. Resetting on a subset of errors needs a reliable distinction from errors followed by admission failure/closure, so I am preserving the current behavior pending maintainer agreement on that policy rather than adding an unverified classification. Leaving this thread open for that decision.
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The reconnect counter is reset only in recv() after the parsed event is known to be non-error. The byte-oriented receive path still calls recv_bytes() directly, so a caller using that public API can successfully receive frames after a reconnect without ever clearing _reconnect_attempt. Separate transient disconnects would then consume one shared retry budget across otherwise healthy periods until a later reconnect is refused. Is the raw receive path intentionally excluded from the new retry semantics? If not, it would be useful to cover reconnect -> successful raw frame -> disconnect again and make sure the budget resets without treating an application error event as success.
2 participants
Summary
Adds optional expiration fields for service-account API keys, clarifies the Responses store option, and honors WebSocket retry limits across repeated reconnects.
Changes