Skip to content

fix(quota): release reservations on header failure - #1559

Open
mastertyko wants to merge 11 commits into
Soju06:mainfrom
mastertyko:fix/release-quota-reservations-on-header-failure
Open

fix(quota): release reservations on header failure#1559
mastertyko wants to merge 11 commits into
Soju06:mainfrom
mastertyko:fix/release-quota-reservations-on-header-failure

Conversation

@mastertyko

@mastertyko mastertyko commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Close the ownership gap after API-key quota admission commits a reservation but before route-specific settlement owns cleanup. If rate-limit response-header calculation fails, the proxy now releases only the locally owned reservation through cancellation, propagates the original header failure, and does not start upstream work.

This PR is intentionally quota-only. The HTTP bridge lifecycle work has been removed and will be handled separately. The post-completed-event leak tracked in #1594 is a different lifecycle stage and remains open.

Type of change

  • fix: — bug fix (no behavior change beyond the bug)
  • feat: — new user-facing feature or capability
  • refactor: — internal refactor (no behavior change, no API change)
  • docs: — documentation only
  • chore: / ci: / build: — tooling, CI, packaging
  • test: — test-only change
  • Breaking change

Linked issue: None found in the bounded upstream issue/PR search. #1594 is related but explicitly outside this PR's lifecycle stage and remains open.

OpenSpec

  • This PR includes / updates an OpenSpec change
  • Not applicable — bug fix that matches the existing spec
  • Not applicable — docs / CI / chore only
  • This PR touches a codex-faithful path and preserves upstream-equivalent behavior

Change directory: openspec/changes/release-quota-reservations-on-header-failure/

The change is quota-only and defines admission-to-header reservation cleanup across stream, collect, compact, and subscription-backed transcription requests. It introduces no API, schema, setting, dashboard, default, or successful-response contract change.

Changes

  • Keep locally owned reservation cleanup active through rate-limit header preparation for stream, collect, compact, and subscription-backed transcription requests.
  • Shield that release from request cancellation; if release persistence itself fails, log it, preserve the original header failure, and leave the existing stale-recovery path unchanged.
  • Preserve borrowed-reservation ownership and ensure no upstream call begins when header preparation fails.
  • Add seven focused unit regressions and four parameterized route/persistence regressions requiring exactly one release, restored quota, and no upstream call across all four surfaces.
  • Run the route regression in the required PostgreSQL target as well as the default SQLite integration suite.

Test plan

Focused quota failure-injection unit regressions: 7 passed
Parameterized route/persistence regressions: 4 passed
Ruff check + format: passed
ty: passed
Proxy architecture checker: passed
openspec validate release-quota-reservations-on-header-failure --strict: passed (9/9 tasks)
openspec validate --specs: 48 passed, 0 failed
Synthetic merge with upstream/main 0095b6ea: conflict-free; focused 7+4 tests, ty, and diff check passed
git diff --check: passed

No local PostgreSQL server was used, so the asyncpg variant was not run locally. The parameterized regression remains included in POSTGRES_PYTEST_TARGETS for the required PostgreSQL CI job.

Screenshots / output

Not applicable: this changes an internal proxy failure path and has no dashboard or successful-response surface change. Route-level regressions cover the externally relevant seam.

Checklist

  • Title is in Conventional Commits format (<type>(<scope>)?: <subject>).
  • Related work is identified above; no exact issue was found to close.
  • Added or updated tests covering the change.
  • Ran the relevant local CI subset.
  • If touching specs: openspec validate --specs passes and the scoped change verifies cleanly.
  • Simplicity gates reviewed: no setting, setup step, README section, dashboard nav, or default changes.
  • CHANGELOG is not edited by hand.

@mastertyko

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

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

Reviewed commit: fca0d7f979

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

@mastertyko

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: de477242d3

ℹ️ 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/http_bridge/mixin.py Outdated
@mastertyko

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: d090cf450f

ℹ️ 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. and removed 🤖 codex: ok [@codex review] says no issues found. labels Jul 31, 2026
@Soju06

Soju06 commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Now CONFLICTING with main, but the conflicts are confined to the bridge-lifecycle commits: http_bridge/mixin.py, request_submit.py, streaming.py all collide with #1562 (durable capability lineage) and #1394 (silent/clean-close recovery, merged 08-04), both of which landed after this branch's base (3fe0d6f). The quota half — app/modules/proxy/api.py, the api-keys spec delta, tests/integration/test_api_keys_api.py, tests/unit/test_proxy_api_responses_contract.py, Makefile — merges cleanly today and I verified the gap it closes on current main: all four routes call _rate_limit_headers_for_request after _enforce_request_limits commits the reservation with no cleanup owner in between (e.g. api.py:4892 stream, and the collect/compact/transcribe sites).

Suggest splitting: land the quota fix as-is, and rebase the six bridge close-ownership commits separately. That rebase is not mechanical — #1394 reworked reader retirement/handoff and clean-close reuse on the same paths, so the exact-once upstream_close_attempted claim needs re-validation against reader handoff (a claimed-but-failed close permanently blocks every other close path; verify #1394's handoff flows never need to re-close a session another path already claimed).

One scope note so nobody assumes broader coverage: this releases the reservation for failures in the admission→header window only. The post-completed-event leak in #1594 (aborted completed bookkeeping strands the reservation heartbeat because _detach_http_bridge_request sees the request already popped) is a different lifecycle stage and remains open after this merges.

@mastertyko

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: 7b578fa138

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

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