Skip to content

fix(proxy): detect stalled upstream websockets - #1579

Open
leventov wants to merge 6 commits into
Soju06:mainfrom
leventov:fix/websocket-network-liveness
Open

fix(proxy): detect stalled upstream websockets#1579
leventov wants to merge 6 commits into
Soju06:mainfrom
leventov:fix/websocket-network-liveness

Conversation

@leventov

@leventov leventov commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Detect silent upstream Responses WebSockets after a network/VPN transition and terminate affected conversations promptly instead of leaving them frozen until the container restarts.

This is a focused follow-up to #1234. That PR made observed host-network failures recoverable and account-neutral; this closes the remaining gap where an already-open WebSocket can become half-open without producing a read or write error.

Type of change

  • fix: — bug fix (no behavior change beyond the bug)
  • Breaking change

Linked issue: None — reported directly; no matching issue was found.

OpenSpec

  • This PR includes / updates an OpenSpec change
  • This PR touches a codex-faithful path and preserves upstream-equivalent behavior

Change directory: openspec/changes/archive/2026-08-04-recover-responses-websocket-liveness/

Changes

  • Apply the existing downstream WebSocket idle-timeout value to upstream liveness: aiohttp heartbeat for routed sockets and websockets ping_timeout for direct sockets. No new setting or background watcher is introduced.
  • Normalize library-specific watchdog failures to upstream_websocket_liveness_timeout, including aiohttp's stored heartbeat exception and websockets' acknowledged send-first 1011 close shape.
  • Treat the failure as transport-level and account-neutral, never transparently replay an ambiguously delivered response.create, settle every pending reservation/request exactly once, and retire the dead upstream generation.
  • Preserve sequenced downstream behavior by suppressing synthetic terminal frames and closing the downstream socket with 1011 when a fresh request is required.
  • Add production comments around timeout provenance, no-replay behavior, collection-level settlement ownership, and socket retirement so these invariants do not drift.

Simplicity

  • Works with zero config using the existing 120-second default.
  • No new required setup step.
  • New setting(s) and why each can't be a default: None. The existing proxy_downstream_websocket_idle_timeout_seconds value is reused.
  • README sections / .env.example / dashboard nav remain unchanged and within budget.

Test plan

Passed locally:

uv run ruff check .
uv run ruff format --check .
uv run ty check
uv run python scripts/check_proxy_architecture.py

uv run pytest -q tests/unit/test_proxy_websocket_client.py
# 51 passed
uv run pytest -q tests/unit/test_proxy_utils.py
# 910 passed
uv run pytest -q tests/unit/test_proxy_http_bridge.py
# 390 passed
uv run pytest -q tests/unit/test_realtime_live.py tests/unit/test_codex_upstream_paths.py
# 73 passed

uv run pytest -q tests/integration/test_proxy_websocket_responses.py
# 105 passed (two dependency/thread-cleanup warnings)
uv run pytest -q tests/integration/test_http_responses_bridge.py
# 112 passed

npx --yes @fission-ai/openspec@1.6.0 validate --specs
# 49 passed, 0 failed
npx --yes @fission-ai/openspec@1.6.0 validate --changes
# 80 passed, 0 failed

Three serial local Codex review rounds were completed. Five P1 findings across the rounds were fixed and covered by regressions: stored aiohttp heartbeat exceptions, acknowledged direct close handshakes, bridge double settlement, bridge sibling settlement, and direct send/receive settlement ownership.

Screenshots / output

No dashboard-visible change. On upstream watchdog expiry, unsequenced requests receive a response.failed event with code upstream_websocket_liveness_timeout; sequenced streams close with WebSocket code 1011 after settlement.

Checklist

  • Title is in Conventional Commits format.
  • Related merged PR is linked above; no matching issue exists.
  • Added tests covering direct, routed, bridge, race, settlement, and sequenced paths.
  • Ran the relevant local CI subsets listed above.
  • OpenSpec validation passes and the change was verified, synchronized, and archived.
  • Simplicity gates P1-P5 reviewed.
  • CHANGELOG was not edited by hand.

@Komzpa

Komzpa commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

# Conflicts:
#	app/core/clients/proxy_websocket.py
#	app/modules/proxy/_service/http_bridge/upstream_events.py
#	app/modules/proxy/_service/websocket/mixin.py
#	tests/unit/test_proxy_http_bridge.py
@Soju06

Soju06 commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Reviewed watchdog lifecycle and settlement ownership. Transport side is sound: no custom watcher task, both watchdogs are library-managed, and the direct-WS send/receive settlement claim in websocket/mixin.py is airtight (synchronous claim, event set in finally, no await between claim and try-entry).

One settlement gap in the bridge reader blocks merge:

P1 — session.closed is an overloaded stand-in for liveness-settlement ownership. The break-without-settling guards in app/modules/proxy/_service/http_bridge/upstream_events.py (message path ~L966, exception path ~L1006) assume closed=True means a submitter owns the whole pending deque. But session.closed = True is written by ~30 sites, and some settle only their own request: e.g. the bridge_continuity_persistence_failed paths in request_submit.py (main ~L1114/L1130) set closed + retire_after_drain and leave sibling requests pending on a still-healthy socket. If that socket later heartbeat-expires, the reader hits the guard, breaks without settling the siblings, and exits the loop that enforced stream_idle_timeout — pre-PR the strand was bounded at ~600s by the idle timeout; post-PR it is unbounded until client disconnect. Repro: two requests pending on one bridge session, third submit fails record_recovery_attempt (closed=True, siblings untouched), then kill pongs — the liveness close settles nothing. Suggest a dedicated claim on the session (mirroring _WebSocketUpstreamControl.claim_liveness_settlement), set only by the liveness-owning submitter under lifecycle_lock, instead of keying on closed.

P3 — classification is pinned to library-internal strings (_AIOHTTP_HEARTBEAT_TIMEOUT_PREFIX = "No PONG received after ", sent-1011 + "keepalive ping timeout", proxy_websocket.py ~L82-86/L208-224). The new tests construct these exceptions themselves, so a dependency bump that rewords either library silently degrades liveness failures to upstream_unavailable — re-penalizing the account and dropping force-retire. One integration test driving a real keepalive expiry (short timeout against a fake server that never pongs) would pin the installed library's shape.

P3 — residual false-positive note, no change requested: both libraries stop processing pongs while transport reading is paused under recv backpressure, so a relay stalled >~ping_interval+timeout on downstream writes can kill a healthy upstream. Bounded by the same 120s downstream idle budget and the failure is account-neutral + retryable; the ops.md monitoring note covers it.

Heads-up: this rewrites the same settlement/admission-waiter region as #1594 and the reservation paths #1536 touches — whichever lands second will need a careful rebase. #1558 (finalizer settlement ordering) is merging ahead in this same territory; expect a rebase over its websocket/mixin.py finalizer changes.

@Komzpa

Komzpa commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@leventov

leventov commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in 5036851b after merging current main (including #1558) in 871addf7.

For the P1 settlement gap:

  • _HTTPBridgeSession now has a dedicated liveness_settlement_owner claim. Production comments explicitly document that session.closed controls admission/retirement only and does not imply pending-deque settlement ownership.
  • The submitter publishes the send claim synchronously inside the existing lifecycle_lock, immediately beside the classified liveness failure from send_text and before releasing that lock.
  • Both bridge-reader guards now skip settlement only for that explicit send claim plus upstream_websocket_liveness_timeout; an unrelated closed=True no longer suppresses reader settlement.
  • Added the exact regression shape from the review: two sibling requests remain pending, a third submit fails record_recovery_attempt and closes the session without a liveness claim, then the already-running reader receives the heartbeat expiry. The test verifies both siblings receive one terminal liveness failure, the deque and queue count drain, the account is not penalized, and forced retirement occurs.
  • The pre-existing simultaneous send/receive liveness race test now also verifies that the submitter actually published the send claim and that settlement still occurs exactly once.

For the P3 library-shape coverage:

  • Added an integration test that performs a real WebSocket handshake against a local raw server which reads frames but deliberately never answers pings.
  • The installed websockets client runs with short real ping_interval / ping_timeout values; the production adapter must classify the library-generated close as upstream_websocket_liveness_timeout.
  • The local connection uses proxy=None, following the local adversarial review, so host or CI proxy variables cannot divert the loopback test.

The backpressure false-positive note remains unchanged as requested; the existing account-neutral/retryable behavior and ops monitoring note still bound and document it.

Local verification after the #1558 merge:

  • complete affected HTTP-bridge unit and WebSocket integration files: 554 passed
  • focused ownership and real-watchdog tests under deliberately hostile proxy environment variables: 3 passed
  • Ruff check + format, ty, and proxy architecture check: passed
  • responses-api-compat OpenSpec strict validation: passed

GitHub CI for 5036851b is still in progress, so I am not claiming the cloud gate green yet.

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