Skip to content

fix(proxy): retry missing response.created once - #1543

Closed
mhooooo wants to merge 7 commits into
Soju06:mainfrom
mhooooo:fix/retry-missing-response-created-once
Closed

fix(proxy): retry missing response.created once#1543
mhooooo wants to merge 7 commits into
Soju06:mainfrom
mhooooo:fix/retry-missing-response-created-once

Conversation

@mhooooo

@mhooooo mhooooo commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Recover Codex Responses streams without exposing the reconnect loop when either:

  • an HTTP bridge response.create receives no response.created; or
  • a reused HTTP/direct-WebSocket upstream is already closed before the next send begins.

Every replay stays bounded to one attempt, preserves the original request budget and required account, and remains fail-closed whenever dispatch may have begun.

Supersedes the closed draft #1483. Refs #1393.

OpenSpec

  • This PR includes / updates an OpenSpec change
  • This PR touches a codex-faithful path and preserves upstream-equivalent request and response framing

Change directory: openspec/changes/retry-missing-response-created-once/

Changes

  • Cap the eventless pre-response.created acknowledgement window at 30 seconds instead of 240 seconds.
  • Cancel the stale HTTP receive and invoke the existing safe pre-created replay path only for a sole, pre-visible request.
  • Add a sealed transport result proving a reused socket was closed before the send primitive was invoked.
  • Reconnect and resend the exact HTTP or direct-WebSocket request once on the same required account when that proof exists, including compacted continuations.
  • Keep mid-send failures terminal because their dispatch state is ambiguous.
  • Refuse direct-WebSocket replay if the retired reader does not confirm cancellation; terminally settle the turn, close downstream, and retain cleanup ownership.

Root cause and impact

The existing bridge watchdog always failed after a missing acknowledgement, even when its guarded replay path could safely recover. Separately, both HTTP and direct-WebSocket clients could reuse a normally closed warm upstream and surface a client reconnect before the next send.

The proxy now distinguishes a provably undispatched request from an ambiguous send. Only the former receives one exact same-account resend. Existing lifecycle, visibility, sibling-request, continuation, file-ownership, admission, and one-replay guards remain authoritative.

Simplicity

  • Works with zero configuration
  • Adds no setting or required setup step
  • Adds no dependency, endpoint, schema, migration, dashboard surface, or README section

Validation

Focused HTTP bridge + direct WebSocket suite
631 passed

Ruff check and format
passed (849 files)

Ty type checking
passed

Proxy architecture fitness checks
passed

Strict OpenSpec validation
48 specs passed
71 changes passed

Local Codex review at current head
No actionable correctness regressions identified

Live exact-revision verification also completed authenticated /models, HTTP Responses, and direct-WebSocket Responses requests through the locally deployed combined image.

Checklist

  • Title uses Conventional Commits format
  • Related issue referenced
  • Externally failing HTTP and direct-WebSocket paths have regressions
  • Relevant local gates passed
  • Strict OpenSpec validation passed
  • Simplicity gates reviewed
  • CHANGELOG.md is unchanged

@mhooooo
mhooooo marked this pull request as ready for review July 30, 2026 09:30
@mhooooo

mhooooo commented Jul 30, 2026

Copy link
Copy Markdown
Contributor 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: a8d5dff9c6

ℹ️ 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
@mhooooo

mhooooo commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

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

Reviewed commit: 56df72af4f

ℹ️ 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 🤖 codex: ok [@codex review] says no issues found. 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

#1394 (a66f793, merged 08-04 after a ~20h production soak) landed this PR's core behavior on main: the eventless missing-response.created watchdog is now capped at 60s (app/modules/proxy/_service/http_bridge/helpers.py:201) and the upstream reader already routes the timeout into the bounded pre-created replay (_retry_http_bridge_precreated_request, app/modules/proxy/_service/http_bridge/upstream_events.py:~887), backed by durable retry-circuit state and recovery-attempt fencing that this in-memory implementation predates. Combined with the #1562 connect-path rewrite, every app/ hunk here conflicts, and the 30s cap would regress the soaked 60s value.

One piece main still lacks: pre-send dispatch-absent classification. UpstreamWebSocketSendNotDispatchedError from the adapters, the exact same-account resend that can safely carry previous_response_id, and the direct-WS retained account/stream-lease handoff into _connect_proxy_websocket have no equivalent on main — recovery there is receive-side only, so a warm socket that closes between reuse-check and send still fails closed. If you want to keep that, please refile it as a narrow PR against the #1394 architecture: the replay should register through the durable recovery-attempt fence rather than a request-state field, and the retained-lease handoff needs checking against the in-flight stream-admission work (#1536). Closing this one as superseded otherwise.

@Soju06

Soju06 commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Closing per maintainer decision: superseded by #1394 (merged 08-04, ~20h prod soak) — see the supersession analysis above. The pre-send dispatch-absent classification + retained-lease handoff is still wanted as a narrow refile against the durable recovery-attempt fence; happy to review that as a fresh PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs rebase Needs rebase or conflict repair against current main

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants