Skip to content

fix(proxy): separate websocket scope cleanup budget - #1723

Open
kevinsslin wants to merge 3 commits into
Soju06:mainfrom
kevinsslin:fix/websocket-scope-cleanup-budget
Open

fix(proxy): separate websocket scope cleanup budget#1723
kevinsslin wants to merge 3 commits into
Soju06:mainfrom
kevinsslin:fix/websocket-scope-cleanup-budget

Conversation

@kevinsslin

Copy link
Copy Markdown

Summary

Direct Responses WebSocket scope teardown reused the generic one-second task-cancellation timeout as its entire normal-operation cleanup budget. Under ordinary load, existing request/lease finalization can exceed that bound and emit Websocket scope cleanup exceeded its remaining drain budget.

This PR gives normal-operation scope teardown a fixed five-second observation budget while preserving the one-second generic child-task cancellation bound and the existing shared shutdown deadline.

Type of change

  • fix: — bug fix
  • feat:
  • refactor:
  • Breaking change

Linked issue: Refs #1711

OpenSpec

  • This PR includes / updates an OpenSpec change
  • Change directory: openspec/changes/websocket-scope-cleanup-budget/

Changes

  • Add _WEBSOCKET_SCOPE_CLEANUP_TIMEOUT_SECONDS = 5.0 for normal direct WebSocket scope finalization.
  • Keep _TASK_CANCEL_TIMEOUT_SECONDS = 1.0 for ordinary child-task cancellation.
  • Continue using the remaining shared shutdown drain deadline when drain is active; the five-second normal-operation budget never extends shutdown.
  • Preserve tracked cleanup ownership after the observation budget expires.
  • Add a real route-level cancellation regression that fails against the baseline one-second behavior.
  • Add the OpenSpec proposal, design, delta spec, and tasks.

Root cause

shutdown_state.remaining_drain_timeout_seconds() returns None during normal operation. The finalizer then fell back to _TASK_CANCEL_TIMEOUT_SECONDS, which was designed for individual cancellation waits rather than a sequence of request finalization, reservation/lease cleanup, and connection-lease release.

Scope

This addresses the cleanup-budget warning and reduces cleanup-induced reconnect pressure. It does not change the upstream response.created watchdog, retry/replay policy, or the terminal outcome of a request that receives no upstream acknowledgement.

Test plan

  • uv run pytest tests/unit/test_websocket_terminal_cancellation.py -q — 27 passed
  • uv run pytest tests/integration/test_proxy_websocket_responses.py -q — 110 passed
  • uv run ruff check app/modules/proxy/_service/websocket/mixin.py tests/unit/test_websocket_terminal_cancellation.py
  • uv run ruff format --check app/modules/proxy/_service/websocket/mixin.py tests/unit/test_websocket_terminal_cancellation.py
  • uv run python scripts/check_proxy_architecture.py
  • uv run ty check
  • npx --yes @fission-ai/openspec@1.9.0 validate websocket-scope-cleanup-budget --strict
  • npx --yes @fission-ai/openspec@1.9.0 validate --specs — 50 passed

The new regression was also run against a temporary baseline mutation and failed as expected at the old ~1 second bound, including the warning.

Production evidence

Our codex-lb 1.23.0 deployment observed approximately 15 instances of this warning in the post-upgrade observation window, with 559 successful requests and one ~60-second missing_response_created_timeout. The timeout/recovery behavior is tracked separately; this PR targets the cleanup budget warning observed alongside it.

Keep the one-second generic task cancellation bound while allowing normal direct Responses WebSocket scope finalization to use a bounded five-second observation window. Preserve the shared shutdown deadline and add a regression plus OpenSpec contract.

Refs Soju06#1711
@kevinsslin
kevinsslin marked this pull request as ready for review August 13, 2026 17:24

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

ℹ️ 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/unit/test_websocket_terminal_cancellation.py Outdated
@Komzpa Komzpa added the 🤖 codex: needs work [@codex review] raised an issue label Aug 13, 2026
kevinsslin added a commit to kevinsslin/codex-lb that referenced this pull request Aug 14, 2026
Integrates the reviewed transport reliability fixes from upstream PRs Soju06#1723, Soju06#1726, and Soju06#1736 into the production fork main after combined local validation.
@Komzpa Komzpa removed the 🤖 codex: needs work [@codex review] raised an issue label Aug 14, 2026
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.

2 participants