-
Notifications
You must be signed in to change notification settings - Fork 415
fix(proxy): stop abandoning an unresolved inflight session-creation future #1644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
4ef3d4b
fix(http-bridge): avoid abandoned inflight session future
Komzpa 9d4cdba
test(http-bridge): cover inflight cleanup through responses route
Komzpa 8bd18c7
test(http-bridge): collect inflight reuse regression
Komzpa 39b2471
test(http-bridge): accept optional session creation kwargs
Komzpa e660489
fix(api-keys): retain durability compatibility hook
Komzpa 0e3246d
fix(api-keys): drop stale last-used durability hook
Komzpa 459f51c
fix(websocket): shield response-create lease release
Komzpa d7ca8f8
fix(websocket): finish shielded lease cleanup
Komzpa 7c77d1f
fix(websocket): collect cancellation cleanup coverage
Komzpa 2272adf
fix(websocket): satisfy lease cleanup type checks
Komzpa 458e859
fix: preserve websocket gate cancellation cleanup
Komzpa 932dc97
fix: drop stale durability drift from folded codex-lb branch
Komzpa b1cc7a3
test(http-bridge): prove anchored inflight reuse
Komzpa 09879d2
test(http-bridge): force registered-anchor reuse path
Komzpa a9b991b
Merge origin/main into fix-inflight-future-abandoned
Soju06 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 2 additions & 0 deletions
2
openspec/changes/fix-inflight-future-abandoned/.openspec.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| schema: spec-driven | ||
| created: 2026-08-06 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| ## Context | ||
|
|
||
| The bridge lookup loop first resolves a reusable previous-response session and records it in `session_to_return_after_close`. The generic create arm is selected independently by `inflight_future is None`, so it can register a new pending future for the already-resolved key before the function returns the reused session. The existing cleanup/janitor intentionally removes only completed futures and is covered by a unit test. | ||
|
|
||
| ## Goals / Non-Goals | ||
|
|
||
| **Goals:** | ||
|
|
||
| - Make the reuse decision terminal for session creation in that loop. | ||
| - Leave no unresolved future registered for a key whose existing session is returned. | ||
| - Preserve all create, waiter, handoff, timeout, and janitor behavior for paths that do not reuse a previous-response session. | ||
|
|
||
| **Non-Goals:** | ||
|
|
||
| - Do not change janitor eligibility or restart-blocking semantics for genuinely live creation futures. | ||
| - Do not redesign durable ownership, session closing, or response routing. | ||
|
|
||
| ## Decisions | ||
|
|
||
| Guard the generic `inflight_future is None` creation arm with `session_to_return_after_close is None`. This is the smallest local invariant: once reuse has selected a session, the loop may still close detached sessions, then returns the selected session without publishing a creation future. An early `continue` or future resolution would add lifecycle behavior without benefit and could interfere with the existing create-chain arms. | ||
|
|
||
| The regression uses the real `_get_or_create_http_bridge_session` previous-response lookup path and asserts both registry state and a second successful reuse. The existing janitor test remains unchanged as a negative control. | ||
|
|
||
| ## Risks / Trade-offs | ||
|
|
||
| - [Risk] A future branch might set `session_to_return_after_close` for a case that still needs creation. → Mitigation: the symbol has one assignment, in the validated live-session reuse arm; all other arms leave it `None` and retain the original create condition. | ||
| - [Risk] A future remains from an earlier concurrent creator. → Mitigation: reuse already requires the canonical previous-key inflight lookup to be empty; waiter behavior remains guarded by `inflight_future is not None`. |
25 changes: 25 additions & 0 deletions
25
openspec/changes/fix-inflight-future-abandoned/proposal.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| ## Why | ||
|
|
||
| The HTTP bridge's previous-response reuse path can return an already-live session after first publishing a new, unresolved session-creation future for that same anchor. That orphaned future permanently marks the bridge as restart-blocking and makes later requests fail with a continuity 502, so the create chain must not run when reuse has already selected a session. | ||
|
|
||
| ## What Changes | ||
|
|
||
| - Prevent the generic HTTP bridge session-creation arm from publishing an inflight future when the previous-response path has selected an existing session for return. | ||
| - Preserve the existing done-future janitor contract and all other session-creation and handoff arms. | ||
| - Keep regression coverage for both registry cleanup and a successful second request on the same previous-response anchor. | ||
|
|
||
| ## Capabilities | ||
|
|
||
| ### New Capabilities | ||
|
|
||
| None. | ||
|
|
||
| ### Modified Capabilities | ||
|
|
||
| - `responses-api-compat`: HTTP bridge previous-response reuse must not leave an unresolved session-creation future registered for the reused anchor. | ||
|
|
||
| ## Impact | ||
|
|
||
| - Affected code: `app/modules/proxy/_service/http_bridge/mixin.py` session lookup/create chain. | ||
| - Affected tests: focused HTTP bridge bughunt regression and existing unit/integration bridge suites. | ||
| - No API schema, persistence, or janitor behavior changes. |
51 changes: 51 additions & 0 deletions
51
openspec/changes/fix-inflight-future-abandoned/specs/responses-api-compat/spec.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| ## MODIFIED Requirements | ||
|
|
||
| ### Requirement: Continuity-dependent Responses follow-ups fail closed with retryable errors | ||
| When a Responses follow-up depends on previously established continuity state, the service MUST return a retryable continuity error if that continuity cannot be reconstructed safely. The service MUST NOT expose raw `previous_response_not_found` for bridge-local metadata loss or similar internal continuity gaps. When forwarding a turn-state-anchored follow-up to its bridge owner fails with `bridge_owner_unreachable` and a fresh durable lookup shows the owner no longer holds an active lease (released, expired, or the row is missing or CLOSED), the service MUST recover the follow-up locally through durable takeover instead of returning the retryable error. The fresh durable lookup MUST use the same resolution semantics as request routing, including the latest-turn-state fallback, so a row originally resolved without a registered alias remains takeover-eligible. When the durable lease is still actively held by another instance — including DRAINING rows whose lease has not been released or expired — the service MUST keep failing closed with the retryable error. | ||
|
|
||
| #### Scenario: HTTP bridge loses local continuity metadata for a follow-up request | ||
| - **WHEN** an HTTP `/v1/responses` or `/backend-api/codex/responses` follow-up request depends on `previous_response_id` or a hard continuity turn-state | ||
| - **AND** the bridge cannot reconstruct the matching live continuity state from local or durable metadata | ||
| - **THEN** the service returns a retryable OpenAI-format error | ||
| - **AND** the error code is not `previous_response_not_found` | ||
|
|
||
| #### Scenario: in-flight bridge follower loses continuity while waiting on the same canonical session | ||
| - **WHEN** a follow-up request waits on an in-flight HTTP bridge session for the same hard continuity key | ||
| - **AND** the bridge still cannot reconstruct safe continuity state once the leader finishes | ||
| - **THEN** the service returns a retryable OpenAI-format error | ||
| - **AND** the error code is not `previous_response_not_found` | ||
|
|
||
| #### Scenario: multiplexed follow-ups fail closed only for the matching continuity anchor | ||
| - **WHEN** a websocket or HTTP bridge session has multiple pending follow-up requests with different `previous_response_id` anchors | ||
| - **AND** continuity loss is detected for exactly one of those anchors | ||
| - **THEN** the service applies the retryable fail-closed continuity error only to the matching follow-up request | ||
| - **AND** it does not expose raw `previous_response_not_found` | ||
| - **AND** unrelated pending requests continue on their own response lifecycle | ||
|
|
||
| #### Scenario: multiplexed follow-ups sharing one anchor fail closed together without leaking raw continuity errors | ||
| - **WHEN** a websocket or HTTP bridge session has multiple pending follow-up requests that share the same `previous_response_id` anchor | ||
| - **AND** upstream emits an anonymous continuity loss event such as `previous_response_not_found` for that shared anchor | ||
| - **THEN** the service rewrites each affected follow-up into a retryable continuity error | ||
| - **AND** no affected follow-up exposes raw `previous_response_not_found` | ||
| - **AND** the run remains usable for subsequent requests after the rewritten failures | ||
|
|
||
| #### Scenario: single pre-created follow-up still fails closed when continuity loss omits explicit response id in message | ||
| - **WHEN** a websocket follow-up request is pending with `previous_response_id` and has not received a stable upstream `response.id` yet | ||
| - **AND** upstream emits `previous_response_not_found` with `param=previous_response_id` | ||
| - **AND** the upstream error message omits the literal previous response identifier | ||
| - **THEN** the service still maps that continuity loss to the pending follow-up | ||
| - **AND** it rewrites the downstream terminal event to a retryable continuity error | ||
| - **AND** it does not surface raw `previous_response_not_found` to the client | ||
|
|
||
| #### Scenario: turn-state follow-up recovers locally after the owner released its lease | ||
| - **WHEN** a turn-state-anchored follow-up without `previous_response_id` is forwarded to its bridge owner during the post-shutdown ring grace window | ||
| - **AND** the forward fails with `bridge_owner_unreachable` | ||
| - **AND** a fresh durable lookup using the request-routing resolution semantics (registered alias or latest-turn-state fallback) shows the lease is released or expired | ||
| - **THEN** the service retries the follow-up locally through durable takeover instead of returning the retryable 503 | ||
| - **AND** the takeover retry carries the fresh durable lookup as its continuity anchor even when the turn-state alias registration was lost | ||
| - **AND** a fresh durable lookup showing a live lease held by another instance — even for a DRAINING row — still fails closed with the retryable `bridge_owner_unreachable` error | ||
|
|
||
| #### Scenario: previous-response reuse does not register an abandoned creation future | ||
| - **WHEN** an HTTP bridge request resolves a live compatible session through `previous_response_id` | ||
| - **THEN** the bridge returns that existing session without registering an unresolved inflight session-creation future for its canonical key | ||
| - **AND** a subsequent request on the same previous-response anchor can reuse the session successfully |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| ## 1. Implementation | ||
|
|
||
| - [x] 1.1 Guard the generic HTTP bridge session-creation arm so a previous-response reuse selection cannot publish an inflight future. | ||
| - [x] 1.2 Keep the existing janitor and all non-reuse create-chain arms unchanged. | ||
|
|
||
| ## 2. Verification | ||
|
|
||
| - [x] 2.1 Run the F1 bughunt regression and confirm it fails on the baseline and passes after the fix, including the second-request reuse assertion. | ||
|
Komzpa marked this conversation as resolved.
|
||
| - [x] 2.2 Run the HTTP bridge unit and integration suites, including the existing live-inflight janitor test. | ||
| - [x] 2.3 Validate OpenSpec artifacts and inspect the final diff/status before committing. | ||
7 changes: 7 additions & 0 deletions
7
openspec/changes/fix-websocket-response-create-lease-cancellation/design.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # Design | ||
|
|
||
| `_release_websocket_response_create_gate` keeps its existing state-clearing and | ||
| gate-release ordering, but awaits the captured account lease release through | ||
| `asyncio.shield`. The release operation therefore continues after cancellation | ||
| of the surrounding WebSocket task, returning the account slot without changing | ||
| the existing response-create gate semantics. |
13 changes: 13 additions & 0 deletions
13
openspec/changes/fix-websocket-response-create-lease-cancellation/proposal.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| # Change: Make WebSocket response-create lease cleanup cancellation-safe | ||
|
|
||
| ## Why | ||
|
|
||
| WebSocket terminal cleanup clears the request state's account response-create | ||
| lease before awaiting its asynchronous release. Cancellation at that await can | ||
| leave the account slot counted until stale-lease reclamation. | ||
|
|
||
| ## What Changes | ||
|
|
||
| - Shield the account response-create lease release in WebSocket gate cleanup. | ||
| - Add regression coverage for cancellation under load-balancer runtime-lock | ||
| contention and retain coverage for genuine stale-lease reclamation. |
16 changes: 16 additions & 0 deletions
16
...socket-response-create-lease-cancellation/specs/proxy-admission-control/spec.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| ## ADDED Requirements | ||
|
|
||
| ### Requirement: WebSocket response-create lease cleanup is cancellation-safe | ||
|
|
||
| When WebSocket terminal cleanup has captured an account response-create lease, it MUST complete the asynchronous lease release even if the surrounding task is cancelled while waiting for the load-balancer runtime lock. Cleanup MUST retain the existing response-create gate release semantics. | ||
|
|
||
| #### Scenario: Cancellation under lease-release contention returns the account slot | ||
|
|
||
| - **GIVEN** a WebSocket request owns an account response-create lease and its | ||
| response-create gate | ||
| - **AND** the load-balancer runtime lock is held by another task | ||
| - **WHEN** terminal cleanup is cancelled while releasing the account lease | ||
| - **THEN** the account response-create slot MUST be returned after the lock is | ||
| freed | ||
| - **AND** the request state does not retain the released lease | ||
| - **AND** the response-create gate cleanup semantics remain unchanged |
6 changes: 6 additions & 0 deletions
6
openspec/changes/fix-websocket-response-create-lease-cancellation/tasks.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| # Tasks | ||
|
|
||
| - [x] Make WebSocket response-create lease release cancellation-safe. | ||
| - [x] Add cancellation and stale-reclaim regression coverage. | ||
| - [x] Run targeted WebSocket, HTTP bridge, and load-balancer lease tests. | ||
| - [x] Validate the OpenSpec documents. |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.