Skip to content

fix(proxy): stop abandoning an unresolved inflight session-creation future - #1644

Merged
Soju06 merged 15 commits into
mainfrom
fix-inflight-future-abandoned
Aug 16, 2026
Merged

fix(proxy): stop abandoning an unresolved inflight session-creation future#1644
Soju06 merged 15 commits into
mainfrom
fix-inflight-future-abandoned

Conversation

@Komzpa

@Komzpa Komzpa commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

On the previous-response reuse path, the HTTP bridge set session_to_return_after_close without clearing inflight_future, so a later create-chain arm published a session-creation future and then returned an existing session — leaving the future registered and never resolved. The janitor only evicts future.done() entries (pinned by an existing test), so the anchor stays wedged forever: http_bridge_restart_blocking pins True and the next request on that anchor gets a permanent 502. This was unconditional on that path, not a race.

Fix: the reuse path no longer publishes an inflight creation future. Regression fails on origin/main (permanent 502 / restart_blocking stuck) and passes with the fix; the existing janitor test still passes; 471 unit + 120 integration green (the 2 pre-existing reconnect failures reproduce unchanged on origin/main).

@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: 0787a078a9

ℹ️ 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 bughunt_tests/test_hz_bridge_inflight_leak.py Outdated
Comment thread openspec/changes/fix-inflight-future-abandoned/tasks.md Outdated
@Komzpa
Komzpa force-pushed the fix-inflight-future-abandoned branch from 0787a07 to 495ad8d Compare August 6, 2026 14:17
@github-actions github-actions Bot added the db migration PR changes Alembic database migrations; maintainer must coordinate merge order label Aug 6, 2026
@Komzpa
Komzpa force-pushed the fix-inflight-future-abandoned branch 2 times, most recently from df1107f to acc3726 Compare August 6, 2026 20:00
@Komzpa Komzpa added the 🤖 codex: needs work [@codex review] raised an issue label Aug 6, 2026
@Komzpa
Komzpa force-pushed the fix-inflight-future-abandoned branch from acc3726 to 603506b Compare August 6, 2026 21:47
@Komzpa Komzpa removed the 🤖 codex: needs work [@codex review] raised an issue label Aug 7, 2026
@Komzpa

Komzpa commented Aug 7, 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: 7e077ce587

ℹ️ 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/api_keys/repository.py
@Komzpa Komzpa added the 🤖 codex: needs work [@codex review] raised an issue label Aug 8, 2026
@Soju06

Soju06 commented Aug 10, 2026

Copy link
Copy Markdown
Owner

The core fix looks good: the one-line guard in app/modules/proxy/_service/http_bridge/mixin.py (elif session_to_return_after_close is None and inflight_future is None:) is the minimal correct invariant for the reuse path, and moving the regression onto the CI-collected public Responses route in tests/integration/test_http_responses_bridge.py addressed both earlier P1s. Thanks for the fast turnaround on those.

One remaining blocker before this can merge: please drop commit 7e077ce (fix(api-keys): retain durability compatibility hook, app/modules/api_keys/repository.py:380). That commit papers over a breakage that only existed at your branch's base (410bfae): at that point main's test_usage_reservation_creation_and_settlement_relax_commit_durability still called ApiKeysRepository.update_last_used, which the coalescing change (#1627) had already removed. Main has since fixed this properly — #1656 restored the touch as a stopgap, and #1665 rewrote the test to full durability through ApiKeyLastUsedCoalescer and removed both the call and the method again. On current main there are zero callers of update_last_used, so merging this PR as-is would reintroduce a dead synchronous write API that contradicts the completed coalescing OpenSpec change — which is exactly what the unresolved Codex P2 on this PR flags.

Suggested path: rebase onto current main (which makes the compatibility hook unnecessary), drop 7e077ce, and re-run the suites. While rebasing, also double-check whether the **_kwargs additions to the fake_create_http_bridge_session stubs in commit beb52e7 are still needed against current main — they were likewise compensating for base-vs-main signature drift. The mixin fix, the integration regression, and the OpenSpec artifacts can stay exactly as they are; once the api-keys commit is gone and the Codex thread resolves, this is merge-ready.

@Soju06

Soju06 commented Aug 12, 2026

Copy link
Copy Markdown
Owner

@Komzpa this PR is first in the merge order for your stack and is one thread away from green: CI is passing on 7e077ce and it's mergeable, but the Codex P2 about the bundled last-used compatibility hook is still open. Could you drop that hook from this PR (it's the same bundling issue flagged across the stack) or reply on the thread if you believe it's load-bearing here? Once that's resolved I can merge this and move down the stack (#1648#1649 → ...). Saw the ty fix and green CI on #1645 — nice turnaround; the remaining scope there is the three non-outdated codex P1s on the cancellation-settlement surface.

@Komzpa
Komzpa force-pushed the fix-inflight-future-abandoned branch from 80d966d to 795ed62 Compare August 14, 2026 18:03
@Komzpa
Komzpa force-pushed the fix-inflight-future-abandoned branch from 795ed62 to 0e3246d Compare August 14, 2026 18:06
@Komzpa

Komzpa commented Aug 14, 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: 932dc97326

ℹ️ 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/helpers.py
Comment thread tests/unit/test_proxy_security_work.py
Comment thread openspec/changes/fix-inflight-future-abandoned/tasks.md
@Komzpa Komzpa added the 🤖 codex: needs work [@codex review] raised an issue label Aug 14, 2026
@Komzpa Komzpa removed the 🤖 codex: needs work [@codex review] raised an issue label Aug 15, 2026
@Komzpa

Komzpa commented Aug 15, 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: b1cc7a3666

ℹ️ 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 tests/integration/test_http_responses_bridge.py Outdated
@Komzpa Komzpa added the 🤖 codex: needs work [@codex review] raised an issue label Aug 15, 2026
@Komzpa Komzpa removed the 🤖 codex: needs work [@codex review] raised an issue label Aug 15, 2026
@Komzpa

Komzpa commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: 09879d2d4d

ℹ️ 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 needs rebase Needs rebase or conflict repair against current main label Aug 15, 2026
@Soju06

Soju06 commented Aug 16, 2026

Copy link
Copy Markdown
Owner

Thanks — confirmed the compatibility-hook commit is gone (no api_keys changes remain in the diff), the reuse-path guard matches what was approved, Codex is clean on 09879d2, and CI Required is green. One last mechanical step: #1680 (goal-restart recovery across reconnects) merged on 08-15 and now conflicts with this branch in app/modules/proxy/_service/http_bridge/mixin.py — in the same region as the reuse-path guard. Please rebase onto current main and re-verify the session_to_return_after_close is None and inflight_future is None invariant still composes correctly with the goal-restart changes (re-run the anchored-reuse regressions). Once the rebase is green this is ready to merge first in the stack.

Resolve mixin.py conflict with #1680: keep the PR's reuse-path guard
(session_to_return_after_close is None) on the capacity/creation elif
while adopting #1680's LRU capacity-close planning body.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 16, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@Soju06, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 41 minutes

Limit details: You’ve used all 3 included reviews currently available under your plan.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 96fd8b9f-ffe2-42b0-bbac-15476d97fc43

📥 Commits

Reviewing files that changed from the base of the PR and between ff89bc4 and a9b991b.

📒 Files selected for processing (14)
  • app/modules/proxy/_service/http_bridge/mixin.py
  • app/modules/proxy/_service/websocket/helpers.py
  • openspec/changes/fix-inflight-future-abandoned/.openspec.yaml
  • openspec/changes/fix-inflight-future-abandoned/design.md
  • openspec/changes/fix-inflight-future-abandoned/proposal.md
  • openspec/changes/fix-inflight-future-abandoned/specs/responses-api-compat/spec.md
  • openspec/changes/fix-inflight-future-abandoned/tasks.md
  • openspec/changes/fix-websocket-response-create-lease-cancellation/design.md
  • openspec/changes/fix-websocket-response-create-lease-cancellation/proposal.md
  • openspec/changes/fix-websocket-response-create-lease-cancellation/specs/proxy-admission-control/spec.md
  • openspec/changes/fix-websocket-response-create-lease-cancellation/tasks.md
  • tests/integration/test_http_responses_bridge.py
  • tests/unit/test_proxy_security_work.py
  • tests/unit/test_proxy_utils.py

Comment @coderabbitai help to get the list of available commands.

@Soju06
Soju06 merged commit 57618c8 into main Aug 16, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

db migration PR changes Alembic database migrations; maintainer must coordinate merge order needs rebase Needs rebase or conflict repair against current main

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants