Skip to content

fix(websocket): shield the response-create account-lease release against cancellation - #1651

Closed
Komzpa wants to merge 5 commits into
mainfrom
fix-create-gate-lease-leak
Closed

fix(websocket): shield the response-create account-lease release against cancellation#1651
Komzpa wants to merge 5 commits into
mainfrom
fix-create-gate-lease-leak

Conversation

@Komzpa

@Komzpa Komzpa commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

_release_websocket_response_create_gate nulled the lease fields on request_state and THEN awaited the release. A cancellation at that await (reachable under _runtime_lock contention) left the lease neither released nor recoverable from request_state — a leaked response_create slot self-healing only after the 900s TTL; 4 leaks on one account inside a TTL produces 429 account_response_create_cap.

Fix: the lease release is shielded so a cancellation cannot orphan it. The stale-lease reclaim path still works for genuinely stale leases.

Regression fails pre-fix (inflight_create == 1 leaked) and passes post-fix (7 probes); 964 named-suite tests green; openspec change validates strictly.

@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: 714d2086d2

ℹ️ 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_hz01_05_gate_lease_release_ordering.py Outdated
Comment thread app/modules/proxy/_service/websocket/helpers.py Outdated
@Komzpa
Komzpa force-pushed the fix-create-gate-lease-leak branch 3 times, most recently from 30750a1 to d0779a2 Compare August 6, 2026 20:03
@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-create-gate-lease-leak branch from d0779a2 to e1663f5 Compare August 6, 2026 22:13
@Komzpa

Komzpa commented Aug 8, 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: 5f91c1754b

ℹ️ 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 Outdated
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

Thanks for the quick turnaround on the first review round — the tracked-cleanup-task approach is the right direction. However, the second Codex review (commit 5f91c17) raised two findings that both check out on inspection, and they're blocking merge:

  1. P1 — caller cancellation is swallowed, not deferred (app/modules/proxy/_service/websocket/helpers.py:1603). In _await_cleanup_deferring_cancellation, when the caller is cancelled while the cleanup task is still running, the loop catches CancelledError, re-awaits asyncio.shield(task), and once the task finishes it returns normally — so the caller's cancellation is permanently discarded rather than re-raised after cleanup, contradicting the docstring ("before propagating cancellation"). A cancelled WebSocket/bridge terminal path then continues as if never cancelled. The new regression in tests/unit/test_proxy_security_work.py actually encodes this: after task.cancel() it does a bare await task expecting normal completion. Suggested fix: remember whether a CancelledError was caught while waiting, and after the cleanup task completes, re-raise it (e.g. raise caught_exc or raise asyncio.CancelledError), and update the test to pytest.raises(asyncio.CancelledError) around await task while still asserting the lease was released and the gate recovered.

  2. P2 — update_last_used re-adds a spec-retired API (app/modules/api_keys/repository.py:381). This method no longer exists on main; it was removed by openspec/changes/coalesce-api-key-last-used-writes in favor of the write-behind, greatest-wins coalescer, and the unconditional UPDATE ... SET last_used_at = utcnow() can move the timestamp backwards under out-of-order writes. It also has no production caller — only tests/integration/test_db_commit_durability.py on this branch. Please drop this hunk and update the durability test to exercise the coalesced contract instead.

Both are small, contained changes; once pushed, re-request @codex review to clear the needs work label. Also FYI: the db migration label on this PR appears to be a mislabel — the diff contains no Alembic files.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create an environment for this repo.

@Komzpa
Komzpa force-pushed the fix-create-gate-lease-leak branch from 31aa627 to d6b08fa Compare August 14, 2026 18:07
@Komzpa

Komzpa commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Folded into #1644: cancellation-safe response-create lease release joins the inflight-future cleanup carrier (they collided on the same hunks all day); history preserved via merge.

@Komzpa Komzpa closed this Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖 codex: needs work [@codex review] raised an issue db migration PR changes Alembic database migrations; maintainer must coordinate merge order

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants