-
Notifications
You must be signed in to change notification settings - Fork 398
fix(proxy): quarantine silent HTTP bridge sessions (takeover of #1405) #1630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
5a4f527
b473212
4e63d62
9d8b458
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,189 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import logging | ||
| import time | ||
| from dataclasses import dataclass | ||
| from typing import Any | ||
|
|
||
| from app.modules.proxy._service.http_bridge.helpers import _log_http_bridge_event | ||
| from app.modules.proxy._service.support import ( | ||
| _REQUEST_TRANSPORT_HTTP, | ||
| _HTTPBridgeSession, | ||
| _HTTPBridgeSessionKey, | ||
| _WebSocketRequestState, | ||
| ) | ||
| from app.modules.proxy.affinity import _extract_model_class | ||
|
|
||
| logger = logging.getLogger("app.modules.proxy.service") | ||
|
|
||
| # Quarantine is a bounded, in-memory, session-scoped (never account-scoped) | ||
| # marker for HTTP bridge session keys that have proven silent/wedged: a later | ||
| # request must not re-attach to them and must take the existing fresh | ||
| # session/no-anchor path instead (#1534). It complements — and never replaces | ||
| # — the in-flight recovery machinery: the eventless watchdog and bounded | ||
| # replay (#1394) recover the request that is currently stuck, the fenced | ||
| # durable-anchor clear (#1563) stops a *fully eventless* full-resend anchor | ||
| # from being re-injected, and the durable retry circuit backs off in-place | ||
| # retries. Quarantine covers what those leave open: the reattached stream | ||
| # that delivers response events but never gets ``response.created`` (the | ||
| # ``response_event_count == 0`` gates in the stale/eventless detection never | ||
| # trip on it), and the repeated-wedge case where consecutive eventless | ||
| # timeouts keep rebuilding the same reattach. | ||
| _HTTP_BRIDGE_QUARANTINE_TTL_SECONDS = 600.0 | ||
| _HTTP_BRIDGE_QUARANTINE_EVENTLESS_TIMEOUT_THRESHOLD = 2 | ||
| _HTTP_BRIDGE_QUARANTINE_MAX_ENTRIES = 1024 | ||
|
|
||
| _HTTP_BRIDGE_QUARANTINE_WEDGED_REATTACH_REASON = "reattach_missing_response_created" | ||
| _HTTP_BRIDGE_QUARANTINE_REPEATED_EVENTLESS_REASON = "repeated_eventless_timeout" | ||
|
|
||
|
|
||
| @dataclass(slots=True) | ||
| class _HTTPBridgeQuarantineEntry: | ||
| quarantined_until: float = 0.0 | ||
| consecutive_eventless_timeouts: int = 0 | ||
| last_touched_monotonic: float = 0.0 | ||
| reason: str | None = None | ||
|
|
||
|
|
||
| def _http_bridge_quarantine_registry( | ||
| service: Any, | ||
| ) -> dict[_HTTPBridgeSessionKey, _HTTPBridgeQuarantineEntry]: | ||
| registry = getattr(service, "_http_bridge_quarantined_keys", None) | ||
| if registry is None: | ||
| registry = {} | ||
| service._http_bridge_quarantined_keys = registry | ||
| return registry | ||
|
|
||
|
|
||
| def _prune_http_bridge_quarantine_registry( | ||
| registry: dict[_HTTPBridgeSessionKey, _HTTPBridgeQuarantineEntry], | ||
| now: float, | ||
| ) -> None: | ||
| expiry = now - _HTTP_BRIDGE_QUARANTINE_TTL_SECONDS | ||
| for key, entry in list(registry.items()): | ||
| if entry.last_touched_monotonic <= expiry and entry.quarantined_until <= now: | ||
| registry.pop(key, None) | ||
| overflow = len(registry) - _HTTP_BRIDGE_QUARANTINE_MAX_ENTRIES | ||
| if overflow > 0: | ||
| for stale_key in sorted(registry, key=lambda candidate: registry[candidate].last_touched_monotonic)[:overflow]: | ||
| registry.pop(stale_key, None) | ||
|
|
||
|
|
||
| def _http_bridge_request_state_wedged_reattach(request_state: _WebSocketRequestState) -> bool: | ||
| """Identify the #1534 wedge shape on a request that is being failed/retired. | ||
|
|
||
| A reattached stream (proxy-injected ``previous_response_id``) whose | ||
| ``response.create`` was sent and that observed upstream response events, | ||
| but whose ``response.created`` was never assigned. This is only evaluated | ||
| when the request is already being failed or its session retired — never | ||
| against a live owned turn — so legitimate long event gaps (for example | ||
| deferred-reasoning streams) can never trip it, and any request whose | ||
| ``response.created`` was observed (``response_id`` or created latency set) | ||
| is excluded by construction. | ||
| """ | ||
| return ( | ||
| getattr(request_state, "transport", None) == _REQUEST_TRANSPORT_HTTP | ||
| and not getattr(request_state, "skip_request_log", False) | ||
| and getattr(request_state, "proxy_injected_previous_response_id", False) | ||
| and getattr(request_state, "response_create_sent_at", None) is not None | ||
| and getattr(request_state, "response_id", None) is None | ||
| and getattr(request_state, "latency_response_created_ms", None) is None | ||
| and getattr(request_state, "response_event_count", 0) > 0 | ||
| ) | ||
|
|
||
|
|
||
| def _http_bridge_session_key_quarantined(service: Any, key: _HTTPBridgeSessionKey) -> bool: | ||
| registry = _http_bridge_quarantine_registry(service) | ||
| now = time.monotonic() | ||
| _prune_http_bridge_quarantine_registry(registry, now) | ||
| entry = registry.get(key) | ||
| return entry is not None and entry.quarantined_until > now | ||
|
|
||
|
|
||
| def _quarantine_http_bridge_session(service: Any, session: _HTTPBridgeSession, *, reason: str) -> None: | ||
| """Quarantine a bridge session that has proven silent/wedged. | ||
|
|
||
| Session-scoped only: no account-health writes happen here, and the entry | ||
| is bounded by TTL, a registry size cap, and the healthy-completion clear. | ||
| """ | ||
| now = time.monotonic() | ||
| registry = _http_bridge_quarantine_registry(service) | ||
| entry = registry.setdefault(session.key, _HTTPBridgeQuarantineEntry()) | ||
| already_quarantined = entry.quarantined_until > now | ||
| entry.quarantined_until = max(entry.quarantined_until, now + _HTTP_BRIDGE_QUARANTINE_TTL_SECONDS) | ||
| entry.last_touched_monotonic = now | ||
| entry.reason = reason | ||
| _prune_http_bridge_quarantine_registry(registry, now) | ||
| session.quarantined = True | ||
| if already_quarantined: | ||
| return | ||
| _log_http_bridge_event( | ||
| "session_quarantined", | ||
| session.key, | ||
| account_id=session.account.id, | ||
| model=session.request_model, | ||
| detail=f"reason={reason}, ttl_seconds={_HTTP_BRIDGE_QUARANTINE_TTL_SECONDS:.0f}", | ||
| cache_key_family=session.key.affinity_kind, | ||
| model_class=_extract_model_class(session.request_model) if session.request_model else None, | ||
| ) | ||
|
|
||
|
|
||
| def _record_http_bridge_quarantine_wedged_pending( | ||
| service: Any, | ||
| session: _HTTPBridgeSession, | ||
| request_states: Any, | ||
| ) -> bool: | ||
| """Quarantine the session when a failed/retired pending request proves the wedge shape.""" | ||
| if not any(_http_bridge_request_state_wedged_reattach(request_state) for request_state in request_states): | ||
| return False | ||
| _quarantine_http_bridge_session( | ||
| service, | ||
| session, | ||
| reason=_HTTP_BRIDGE_QUARANTINE_WEDGED_REATTACH_REASON, | ||
| ) | ||
| return True | ||
|
|
||
|
|
||
| def _record_http_bridge_quarantine_eventless_timeout(service: Any, session: _HTTPBridgeSession) -> None: | ||
| """Count a ``missing_response_created_timeout`` retire; quarantine on repeats. | ||
|
|
||
| The first eventless timeout is left to the merged recovery machinery | ||
| (bounded pre-created retry, fenced durable-anchor clear). A second | ||
| consecutive one for the same session key proves that path is also | ||
| rebuilding a wedged attach, so later requests must stop re-attaching. | ||
| """ | ||
| now = time.monotonic() | ||
| registry = _http_bridge_quarantine_registry(service) | ||
| # Prune before touching the entry: a strike whose TTL already lapsed must | ||
| # not be resurrected into a "consecutive" second strike hours later. | ||
| _prune_http_bridge_quarantine_registry(registry, now) | ||
| entry = registry.setdefault(session.key, _HTTPBridgeQuarantineEntry()) | ||
| entry.consecutive_eventless_timeouts += 1 | ||
| entry.last_touched_monotonic = now | ||
| if entry.consecutive_eventless_timeouts < _HTTP_BRIDGE_QUARANTINE_EVENTLESS_TIMEOUT_THRESHOLD: | ||
| return | ||
| _quarantine_http_bridge_session( | ||
| service, | ||
| session, | ||
| reason=_HTTP_BRIDGE_QUARANTINE_REPEATED_EVENTLESS_REASON, | ||
| ) | ||
|
|
||
|
|
||
| def _clear_http_bridge_quarantine(service: Any, session: _HTTPBridgeSession) -> None: | ||
| """A completed response on this key disproves the wedge; drop all state.""" | ||
| registry = _http_bridge_quarantine_registry(service) | ||
| session.quarantined = False | ||
| entry = registry.pop(session.key, None) | ||
| if entry is None: | ||
| return | ||
| if entry.quarantined_until <= time.monotonic(): | ||
| return | ||
| _log_http_bridge_event( | ||
| "session_quarantine_cleared", | ||
| session.key, | ||
| account_id=session.account.id, | ||
| model=session.request_model, | ||
| detail=f"reason={entry.reason}", | ||
| cache_key_family=session.key.affinity_kind, | ||
| model_class=_extract_model_class(session.request_model) if session.request_model else None, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,6 +101,9 @@ | |
| _reserve_http_bridge_unanchored_handoff, | ||
| _trim_http_bridge_previous_response_input_items, | ||
| ) | ||
| from app.modules.proxy._service.http_bridge.quarantine import ( | ||
| _http_bridge_session_key_quarantined, | ||
| ) | ||
| from app.modules.proxy._service.http_bridge.service_stubs import ( | ||
| _build_rewritten_stream_response_failed_event, | ||
| _codex_keepalive_frame, | ||
|
|
@@ -1411,6 +1414,27 @@ def classify_durable_full_resend( | |
| and durable_lookup.latest_response_id is not None | ||
| and (not payload_looks_like_full_resend or durable_anchor_trimmable) | ||
| ) | ||
| if ( | ||
| fresh_reattach_can_use_durable_anchor | ||
| and payload_looks_like_full_resend | ||
| and _http_bridge_session_key_quarantined(self, bridge_session_key) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When stale-gate cleanup quarantines a session while a sibling request keeps it live, AGENTS.md reference: AGENTS.md:L129-L132 Useful? React with 👍 / 👎. |
||
| ): | ||
| # The previous attach on this key proved silent/wedged | ||
| # (#1534). The client's own payload already carries the full | ||
| # conversation, so send it unanchored on the fresh path | ||
| # instead of rebuilding the same reattach. Delta-only | ||
| # payloads keep the anchor: it is their only way to convey | ||
| # prior context (same boundary as the fenced anchor clear). | ||
| fresh_reattach_can_use_durable_anchor = False | ||
| _log_http_bridge_event( | ||
| "fresh_reattach_anchor_skipped_quarantined", | ||
| bridge_session_key, | ||
| account_id=durable_lookup.account_id, | ||
| model=payload.model, | ||
| detail=f"response_id={durable_lookup.latest_response_id}", | ||
| cache_key_family=bridge_session_key.affinity_kind, | ||
| model_class=_extract_model_class(payload.model) if payload.model else None, | ||
| ) | ||
| if ( | ||
| fresh_reattach_can_use_durable_anchor | ||
| and payload_looks_like_full_resend | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| schema: spec-driven | ||
| created: 2026-08-06 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the 600-second TTL elapses—or the size cap evicts an entry—this removes only the registry record and leaves the corresponding live session's
quarantinedflag set. A session that survived stale-gate cleanup without a later completion therefore remains permanently ineligible for reuse even though the documented quarantine window expired; this is observable for Codex sessions whose configured idle TTL can exceed 600 seconds. Expiry/eviction must also clear or stop relying on the per-session flag.AGENTS.md reference: AGENTS.md:L24-L27
Useful? React with 👍 / 👎.