Skip to content

fix(proxy): retry websocket EOF before visible output - #1388

Open
Komzpa wants to merge 5 commits into
mainfrom
fix/websocket-disconnect-transparent-retry-20260717
Open

fix(proxy): retry websocket EOF before visible output#1388
Komzpa wants to merge 5 commits into
mainfrom
fix/websocket-disconnect-transparent-retry-20260717

Conversation

@Komzpa

@Komzpa Komzpa commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds bounded transparent retry for Codex websocket upstream EOFs that happen after response.created but before any downstream-visible text/tool output. This keeps transient no close frame received or sent upstream disconnects inside codex-lb instead of making Codex CLI spend a long reconnect loop on every affected thread.

Type of change

  • fix: — bug fix (no behavior change beyond the bug)

Linked issue: n/a

OpenSpec

  • Not applicable — bug fix that matches the existing spec
  • This PR touches a codex-faithful path (image pipeline, request/response shape, SSE framing, OAuth flow) and preserves upstream-equivalent behavior

Change directory: n/a

Changes

  • Allow up to three transparent websocket close replays before visible output instead of only one.
  • Add jittered backoff before reconnecting the upstream websocket.
  • Preserve the original downstream response id across repeated transparent replays.
  • Cover repeated created-only upstream EOFs and retry-budget exhaustion in websocket integration tests.

Simplicity

  • New feature defaults to off or works with zero config
  • No new required setup step (or maintainer approval via simplicity-budget-approved label)
  • New setting(s) and why each can't be a default: none
  • README sections / .env.example / dashboard nav within budget (or simplicity-budget-approved label requested)

Test plan

uv run pytest -q tests/integration/test_proxy_websocket_responses.py -k 'retries_repeated_created_only_upstream_eof or emits_response_failed_before_close_on_upstream_eof or closes_before_replaying_exposed_sequence' --tb=short
# 3 passed, 71 deselected, 1 warning in 5.55s

uvx ruff check app/modules/proxy/_service/support.py app/modules/proxy/_service/websocket/helpers.py app/modules/proxy/_service/websocket/mixin.py tests/integration/test_proxy_websocket_responses.py
# All checks passed!

uvx ruff format --check app/modules/proxy/_service/support.py app/modules/proxy/_service/websocket/helpers.py app/modules/proxy/_service/websocket/mixin.py tests/integration/test_proxy_websocket_responses.py
# 4 files already formatted

Screenshots / output

Proxy behavior: repeated upstream websocket EOFs before visible output are retried internally; the downstream sees the original response.created followed by the final terminal event using the original response id.

Checklist

  • Title is in Conventional Commits format (<type>(<scope>)?: <subject>).
  • Linked the related issue / discussion above.
  • Added or updated tests covering the change.
  • Ran uv run pre-commit run local-ci --hook-stage manual --all-files or the relevant make <target> subset locally.
  • If touching specs: openspec validate --specs passes and /opsx:verify is clean.
  • Simplicity gates reviewed: the five simplicity rules (PRINCIPLES.md P1-P5).
  • CHANGELOG is not edited by hand (release-please handles it).

@Komzpa

Komzpa commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0e25161d93

ℹ️ 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".

Comment thread app/modules/proxy/_service/websocket/mixin.py Outdated
@Komzpa Komzpa added the 🤖 codex: needs work [@codex review] raised an issue label Jul 17, 2026
@Komzpa
Komzpa force-pushed the fix/websocket-disconnect-transparent-retry-20260717 branch 2 times, most recently from c99b7b6 to ce3c23f Compare July 17, 2026 10:17
@Komzpa Komzpa removed the 🤖 codex: needs work [@codex review] raised an issue label Jul 17, 2026
@Komzpa

Komzpa commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ce3c23f6a3

ℹ️ 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".

Comment thread app/modules/proxy/_service/websocket/mixin.py Outdated
Comment thread app/modules/proxy/_service/websocket/mixin.py Outdated
@Komzpa Komzpa added 🤖 codex: needs work [@codex review] raised an issue and removed 🤖 codex: needs work [@codex review] raised an issue labels Jul 17, 2026
@Komzpa

Komzpa commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 49abb6d490

ℹ️ 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".

Comment thread app/modules/proxy/_service/support.py Outdated
@Komzpa Komzpa added 🤖 codex: needs work [@codex review] raised an issue and removed 🤖 codex: needs work [@codex review] raised an issue labels Jul 17, 2026
@Komzpa

Komzpa commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: 248eee7b67

ℹ️ 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".

@Komzpa Komzpa added the 🤖 codex: ok [@codex review] says no issues found. label Jul 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has had no activity for 7 days.

It will be closed in 23 more days unless there is new activity.

If this is still relevant, please:

  • Rebase or push an update if the branch drifted
  • Address pending review feedback if there is any
  • Leave a short comment confirming it is still being worked on

Thanks for the contribution 🙏

@github-actions github-actions Bot added the stale No response from reporter; scheduled for close label Jul 25, 2026
@Soju06

Soju06 commented Jul 28, 2026

Copy link
Copy Markdown
Owner

This PR is currently conflicting with main, so it can't be picked up in merge triage as-is. Could you rebase onto the latest main and resolve the conflicts? Once CI is green again it will be included in the next triage round. Thanks!

@Komzpa
Komzpa force-pushed the fix/websocket-disconnect-transparent-retry-20260717 branch from 248eee7 to 40c6c6e Compare July 28, 2026 06:13
@github-actions github-actions Bot removed the stale No response from reporter; scheduled for close label Jul 28, 2026
@Komzpa
Komzpa force-pushed the fix/websocket-disconnect-transparent-retry-20260717 branch from 40c6c6e to 0eb65c4 Compare July 28, 2026 09:18
@Soju06

Soju06 commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Thanks for the rebase — CI is green on the new head and the earlier codex P1/P2 findings look genuinely addressed (backoff now sleeps only after the replay is re-registered in pending_requests with its heartbeat, mixin.py:887-904, and the deferred-message paths release the prepared request's reservation before re-queuing, mixin.py:1092-1094 / 1259-1266). The OpenSpec delta keeping numeric-sequence exposure as the hard no-replay boundary also reads correctly, and the extended EOF regression asserts the budget boundary at the external websocket surface (final settle on resp_ws_eof_retry_3), so it fails under the old single-replay budget.

Three items before this is mergeable:

  1. Stale codex review on the current head. The clean @codex review pass (2026-07-17) reviewed 248eee7b67. Head 0eb65c4c81 ("fix(proxy): retire websocket upstream before next create", 2026-07-28) is a substantive restructuring of reader settlement (~113 lines in app/modules/proxy/_service/websocket/mixin.py), not just a rebase. Please request a fresh @codex review so the review gate covers what would actually merge.

  2. Unbounded reader await while draining (new in 0eb65c4c81). settle_current_upstream_reader_if_ready() (mixin.py:812-836) awaits upstream_reader without a timeout whenever upstream_control.reconnect_requested is set, even if the reader is not done (mixin.py:816-821), and it now runs unconditionally at the top of every downstream loop iteration (mixin.py:840). The settlement path that retires an account on health error sets reconnect_requested = True together with retire_after_drain = True (mixin.py:4502-4503) while other pending sequenced responses keep draining on that upstream — the reader deliberately keeps reading in that state (mixin.py:3489-3495). During such a drain the downstream receive loop is fully blocked: new response.create frames and even websocket.disconnect are not read by the owner loop until the drain finishes. On main the equivalent unbounded await only fires after a fresh create has been prepared (main mixin.py:1068), so keepalive-timeout iterations never blocked. The reader does emit downstream keepalives and detects client disconnects itself, so this may be acceptable — but please either gate the not-done await on the replay case it was added for, or explain why blocking the owner loop for a full retire-after-drain is intended.

  3. Release-train classification. Raising the transparent created-only replay budget from 1 to 3 plus jittered reconnect backoff is a behavior change with a MODIFIED Requirements spec delta, not a regression fix (no linked issue — the "Fixes #N" line is n/a). Under the current soak policy this looks like it should queue behind the stable cut (chore(main): release 1.23.0 #1472) rather than merge as a soak-exempt bug fix — maintainer call.

Minor: this PR, #1402, and #1384 all touch websocket/mixin.py / support.py, so whichever merges first will force rebases on the others — worth agreeing on an order.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0eb65c4c81

ℹ️ 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".

Comment thread app/modules/proxy/_service/websocket/mixin.py Outdated
Comment thread app/modules/proxy/_service/websocket/mixin.py Outdated
@Komzpa Komzpa added 🤖 codex: needs work [@codex review] raised an issue needs rebase Needs rebase or conflict repair against current main and removed 🤖 codex: ok [@codex review] says no issues found. labels Jul 30, 2026
@Soju06

Soju06 commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Maintainer decision on the round-4 classification question: the replay-budget increase (1→3 + jittered backoff) is accepted as a behavior change that will ride the normal beta soak after merge — it will NOT be held behind the stable cut. So the remaining blockers are only: (1) the two codex P2s on websocket/mixin.py (owner loop awaiting a not-done reader during retire-after-drain; blanket handler swallowing the owner's CancelledError and leaking reservations), (2) rebase across #1562/#1394. Fix those and this proceeds to merge.

@Komzpa
Komzpa force-pushed the fix/websocket-disconnect-transparent-retry-20260717 branch from 0eb65c4 to 8eef569 Compare August 4, 2026 21:41
@Komzpa Komzpa removed 🤖 codex: needs work [@codex review] raised an issue needs rebase Needs rebase or conflict repair against current main labels Aug 4, 2026
@Komzpa
Komzpa force-pushed the fix/websocket-disconnect-transparent-retry-20260717 branch from 8eef569 to 3140874 Compare August 4, 2026 21:50
@Komzpa

Komzpa commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@jaekwonhong

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: 3140874176

ℹ️ 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".

@Soju06

Soju06 commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Triage re-verification passed today: both codex P2s are addressed at 3140874176, the rebase over #1562 checks out, and per the maintainer decision this merges to start the beta soak. It was in today's merge queue, but #1520 (WS shutdown drain) landed first and this went CONFLICTING — both touch websocket/mixin.py. Please rebase; it merges immediately on a conflict-resolution-only update, no re-review needed.

@Komzpa Komzpa added the needs rebase Needs rebase or conflict repair against current main label Aug 6, 2026
@Komzpa
Komzpa force-pushed the fix/websocket-disconnect-transparent-retry-20260717 branch from 3140874 to 8a7fe6e Compare August 6, 2026 23:16
@Komzpa
Komzpa force-pushed the fix/websocket-disconnect-transparent-retry-20260717 branch from bf8cdd9 to 1676031 Compare August 7, 2026 00:20
@Komzpa Komzpa removed the needs rebase Needs rebase or conflict repair against current main label Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants