Skip to content

fix(proxy): sequence websocket health after settlement - #1558

Merged
Soju06 merged 5 commits into
Soju06:mainfrom
mastertyko:fix/websocket-health-settlement-order
Aug 4, 2026
Merged

fix(proxy): sequence websocket health after settlement#1558
Soju06 merged 5 commits into
Soju06:mainfrom
mastertyko:fix/websocket-health-settlement-order

Conversation

@mastertyko

@mastertyko mastertyko commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Ensure ordering-sensitive stream callers observe whether API-key usage settlement actually committed before writing account health. A failed primary settlement now completes fallback release under the same tracked task once started; pre-start cancellation transfers release to the tracked callback instead. Cancellation during started work reports an unconfirmed outcome only after the release attempt completes. If neither operation confirms settlement, the related WebSocket or deferred retry health write remains unapplied.

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: Related to #1314

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 request/response behavior

Change directory: openspec/changes/sequence-websocket-health-after-settlement/

Changes

  • Propagate the real settlement/fallback result to ordering-sensitive callers while preserving the ordinary detached settlement path.
  • Keep tracked persistence ownership through failed or cancelled ordering-sensitive settlement and fallback release so graceful shutdown drains every phase, including pre-start cancellation.
  • Gate WebSocket and existing retry-deferred account-health writes on confirmed settlement, without coupling reconnect or retirement safety to persistence.
  • Add deterministic coverage for blocked fallback, failed fallback, cancellation before startup and during fallback, health persistence failure, and an unconfirmed retry settlement.

Related work

Test plan

python -m pytest tests/unit/test_proxy_utils.py -q
# 912 passed

python -m pytest tests/integration/test_detached_persistence.py tests/integration/test_api_keys_api.py::test_stream_401_retry_success_finalizes_once -q
# 6 passed

ruff format --check app/modules/proxy/_service/api_key_usage.py app/modules/proxy/_service/streaming/retry.py tests/unit/test_proxy_utils.py
ruff check app/modules/proxy/_service/api_key_usage.py app/modules/proxy/_service/streaming/retry.py tests/unit/test_proxy_utils.py
ty check app/modules/proxy/_service/api_key_usage.py app/modules/proxy/_service/streaming/retry.py app/modules/proxy/_service/websocket/mixin.py tests/unit/test_proxy_utils.py
python scripts/check_proxy_architecture.py
openspec validate sequence-websocket-health-after-settlement --strict
openspec validate --specs --strict
# all passed; 48/48 main specs valid

Full local CI was not run; the focused Sensitive checks above cover the changed seam, and required GitHub CI remains the integration gate.

Checklist

  • Title is in Conventional Commits format (<type>(<scope>)?: <subject>).
  • Linked the related issue / discussion above.
  • Added or updated tests covering the change.
  • Ran the relevant local test, lint, type, architecture, and spec subset.
  • If touching specs: openspec validate --specs passes and the change verification is clean.
  • Simplicity gates reviewed: no setting, setup step, README section, .env.example, dashboard navigation, or default changed.
  • CHANGELOG is not edited by hand.

@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: ef0d53702b

ℹ️ 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/api_key_usage.py Outdated
@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: c4daa0cf8d

ℹ️ 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/api_key_usage.py
@mastertyko

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: b4a88d590f

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

Traced the three touched paths against main; the invariant violation is real and the fix holds up.

  • Bug confirmed: on main, api_key_usage.py's wait branch (await asyncio.shield(task) + except Exception: pass) discards _settle_once's False, so a failed settlement transaction lets websocket/mixin.py record account health while the fallback release is still a detached background task — exactly the ordering the inline comment at mixin.py:4772 promises not to do. Same hole in retry.py's deferred post-refresh penalties.
  • Ownership is right: fallback release runs inside the same tracked settlement task, so drain_persistence_tasks covers both phases; release_on_failure=False prevents the tracker double-releasing an ordinary False result, while pre-start cancellation (task.cancelled() in the caller loop) still hands release to the tracked callback. Only one path starts fallback in every interleaving I could construct.
  • Secondary fix worth keeping: reconnect_requested/retire_after_drain now set before settlement/health persistence, so a raising _handle_stream_error can no longer leave the failed upstream connection un-retired (on main those flags were only set after a successful health write).
  • Known conservative trade-offs, fine as designed: cancellation during primary/fallback reports unconfirmed even when the fallback committed (one dropped health observation), and an unconfirmed retry settlement drops the cleared pending penalties permanently. Both err toward skipping health writes rather than reordering.
  • The not settlement.usage_settlement_transferred guard in retry.py is now load-bearing — previously dead because the helper always returned True — and correctly prevents a second settlement of a transferred reservation.

Overlap note: #1546 (closed) contained this core fix; this PR is the surviving isolated version. #1545 touches the same api_key_usage.py region — content-compatible, but whichever merges second needs a rebase. Adjacent to #1594 (heartbeat leak on aborted paths that never reach this finalizer) without conflicting. #1550's websocket/mixin.py hunks (~1017, ~4636) are disjoint from the finalizer region here — the two compose.

Fix-class, MERGEABLE, CI fully green, OpenSpec delta validates. Merge-ready.

@Soju06
Soju06 merged commit d77011b into Soju06:main Aug 4, 2026
58 of 71 checks passed
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