refactor: integrate band-sdk-core Session into BandLink - #603
refactor: integrate band-sdk-core Session into BandLink#603AlexanderZ-Band wants to merge 12 commits into
Conversation
tests/websocket/test_client.py and test_watchdog.py each defined a byte-for-byte duplicate _fast_session_policy helper. Move it to a shared tests/websocket/conftest.py so the Session-driven reconnect tests INT-1303 adds next have one place to build a fast SessionPolicy from, instead of a third copy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FuwaxzwGje1eq94W7V8zrW
WebSocketClient now owns a band_sdk_core.Session, built from the same SessionPolicy the watchdog already uses, and ends it in __aexit__ for lifecycle symmetry with the existing watchdog stop. No behavior change yet -- nothing calls into self._session outside __init__/__aexit__ until the next commit replaces the reconnect-backoff loop. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FuwaxzwGje1eq94W7V8zrW
Replace WebSocketClient.__aenter__'s fixed 11-attempt cap and hand-rolled _initial_reconnect_delay (a pure exponential formula, no uptime awareness) with band_sdk_core.Session-driven backoff: begin_attempt/on_upgrade_rejected/ on_socket_close/on_connected now decide retry timing and terminal/retryable classification via classify_close/classify_upgrade, replacing the vendored ReconnectPolicy import this loop was the sole user of. Two real behavior changes fall out of routing through classify_upgrade: 429/503 upgrade rejections are now retried (using Retry-After as a delay floor) instead of raised immediately, and the give-up bound is now a rolling 5-minute rapid-disconnect window (10 by default) instead of a fixed attempt count. WebSocketDisconnectReason gains dead_reason/stale_reason fields, populated by a new _disconnect_reason_from_exception for a permanently-failed initial connect (no platform wire payload exists for that case, unlike a supersede). BandLink.connect() now propagates that reason onto last_disconnect_reason even though self._ws is never assigned on this failure path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FuwaxzwGje1eq94W7V8zrW
WebSocketClient.handle_supersede arbitrates a supersede event through Session -- whether it's actually current, and whether a retryable supersede should keep the connection reconnecting -- rather than treating every supersede as unconditionally terminal. BandLink._on_supersede now only records a terminal disconnect and queues WebSocketDisconnectedEvent when Session says the outcome is Dead. The platform hardcodes retryable=False on every real supersede today, so this preserves today's exact behavior for every real-world case; the Reconnecting branch has no real-world trigger yet, only the new synthetic regression test, so the SDK stays correct if the platform ever starts sending retryable=True. SupersedePayload.to_disconnect_reason gains an optional SessionOutcome parameter to populate WebSocketDisconnectReason's dead_reason/stale_reason fields, backward compatible with any no-argument caller. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FuwaxzwGje1eq94W7V8zrW
Same precedent as INT-1245's ClaimRegistry/RetryTracker/ParticipantRoster addition: prove the pinned band_sdk_core wheel's Session/SessionPolicy is actually callable from the isolated installed wheel, not just importable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FuwaxzwGje1eq94W7V8zrW
69aca96 to
f5c4196
Compare
self._session persisted for the WebSocketClient instance's whole lifetime, and __aexit__ calls self._session.end() (Dead). Reusing an already-exited instance across sequential `async with` blocks -- which worked before Session existed, since a fresh PHXChannelsClient was always built per entry -- then made the second __aenter__'s begin_attempt() return None, raising "WebSocket session is no longer connectable" instead of reconnecting. Confirmed as a real regression against main (pre-PR): the same reuse pattern works there. Fix: build a fresh Session at the top of __aenter__, mirroring the fresh PHXChannelsClient built right after it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FuwaxzwGje1eq94W7V8zrW
amit-gazal-band
left a comment
There was a problem hiding this comment.
Requesting changes on two correctness issues that undermine this PR's core "fail fast on rapid disconnects" guarantee, plus two smaller robustness gaps worth closing in the same pass. Everything else from the fuller review (efficiency/maintainability/style/test-coverage) is lower stakes and fine as follow-up.
Two fail-loud fixes and one timing fix in __aenter__'s initial-connect retry loop, from human review on PR #603: - Capture `now` before classify_initial_upgrade_error's live-socket probe (up to open_timeout=5s), not after. Charging the probe's own latency to Session's rapid-disconnect timing understated how fast repeated connect failures actually were, risking a delayed or missed trip to Dead. - Replace the bare `assert outcome.retry_after_s is not None` with an explicit `if ... raise RuntimeError(...)`, matching the sibling `epoch is None` check in the same function. An assert is stripped under `python -O`, which would otherwise turn an impossible-today SessionOutcome into an unhandled `asyncio.sleep(None)` TypeError. - Call record_terminal_disconnect() before raising when Session.begin_attempt() returns None, matching every other Dead path in this class, so last_disconnect_reason is never left stale if this (currently unreachable) branch is ever hit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YDYPGiaDSYNZnyC5mg18ch
PR #603 review: classify_initial_upgrade_error's live-socket probe (open_timeout=5s) reran on every backoff retry of the same 429/503 upgrade rejection, since PHXChannelsClient's supervisor swallows the real HTTP details into a generic PHXConnectionError on every attempt. Auto-retrying that rejection (this PR's own change) turned a one-time probe cost into a recurring one, stacking real network latency onto every computed backoff delay. Split the expensive probe out of classify_initial_upgrade_error into probe_upgrade_error, and cache its result on WebSocketClient across repeat unclassifiable PHXConnectionErrors within one __aenter__ retry loop -- reset alongside the fresh Session on each new entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018j6NRbrMzkT4jTP6dBChsn
_classify_connect_failure cached the live-socket probe result behind a
one-shot boolean, reused for every later unclassifiable PHXConnectionError
in the same __aenter__ retry loop regardless of whether the underlying
condition had actually changed. A transient failure probed clean on
attempt 1 could mask a genuinely different, terminal rejection (e.g. a
409) on a later attempt, or a cached 429 could hide a later terminal
400/409 -- both delaying or misclassifying Session's Dead transition.
PHXConnectionError's wrapped message embeds str() of the real underlying
exception (websockets' InvalidStatus.__str__ is just "HTTP {status}", no
per-request noise), so it is a cheap, reliable signature of "is this the
same failure as last time." Key the cache on that message instead of a
bool: reuse the cached classification only while it is unchanged,
re-probe when it differs.
Also extracts the scriptable PHXChannelsClient/probe test doubles
duplicated across the retry-loop tests in test_client.py into shared
scripted_connect/scripted_probe/no_real_sleep fixtures in
tests/websocket/conftest.py, so each test declares its scenario instead
of repeating the same double.
PR #603 review.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017avBjP1yL5RbYRv3UEFeP4
AlexanderZ-Band
left a comment
There was a problem hiding this comment.
Found one correctness issue in the initial-connect retry cache.
…ries The probed-failure cache key is the wrapped PHXConnectionError's message, which for an InvalidStatus 429 is always just "HTTP 429" -- it can't capture Retry-After. Two real, distinct 429s with different Retry-After values hashed to the same key, so the cache silently reused the first one's delay instead of the server's current value. Any cached classification carrying a retry_after now forces a fresh probe on the next occurrence, regardless of message match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017avBjP1yL5RbYRv3UEFeP4
There was a problem hiding this comment.
🟡 Changes recommended
There is at least one correctness issue in exception re-raising (traceback loss) plus a few naming/type issues in newly introduced shared test helpers that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors the SDK’s WebSocket connection lifecycle by integrating band_sdk_core.Session into WebSocketClient/BandLink, replacing the prior hand-rolled retry/backoff and supersede classification logic while expanding disconnect-reason reporting.
Changes:
- Drive initial-connect retry/backoff and terminal classification via
Session(including upgrade-rejection retry semantics and rolling rapid-disconnect window behavior). - Route
agent_control:supersedethroughSession.on_supersede, and record richer terminal/stale disconnect metadata. - Consolidate websocket test helpers in
tests/websocket/conftest.pyand extend CI wheel-smoke to exerciseSession/SessionPolicy.
File summaries
| File | Description |
|---|---|
| tests/websocket/test_watchdog.py | Switch watchdog tests to use the shared fast_session_policy helper. |
| tests/websocket/test_client.py | Add/adjust tests for Session-driven initial-connect retries, probe caching behavior, and richer disconnect reason propagation. |
| tests/websocket/conftest.py | Introduce shared scripted-connect/probe helpers, a no-real-sleep fixture, and fast_session_policy. |
| tests/platform/test_link.py | Update link tests to reflect Session-driven supersede handling and initial-connect terminal reason propagation. |
| src/band/platform/link.py | Integrate Session-aware supersede handling and propagate terminal initial-connect disconnect reason to BandLink. |
| src/band/client/streaming/errors.py | Replace initial-upgrade classification helper with a probe-based upgrade-error recovery function. |
| src/band/client/streaming/client.py | Replace initial-connect retry loop with Session state machine integration; add disconnect reason synthesis and supersede routing. |
| .github/workflows/ci.yml | Extend wheel-smoke to validate Session/SessionPolicy are callable from the built wheel. |
Review details
Suppressed comments (1)
tests/websocket/conftest.py:31
- After renaming
_ScripttoScript, update this call site soScriptedPHXClientconstructs the renamed helper type.
def __init__(self, *script: Exception | object):
self._script = _Script(script)
self.auto_reconnect: bool | None = None
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Use bare `raise` instead of `raise raise_exc` when re-raising the exception already being handled -- the explicit form added a spurious extra traceback frame (verified live: same exception, one more frame than a bare re-raise). - Rename `_Script`/`_ScriptedProbeConnection` in tests/websocket/conftest.py to drop the leading underscore, per this repo's class-naming convention. - Type probed_urls as list[tuple[str, float]], matching open_timeout: float. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017avBjP1yL5RbYRv3UEFeP4
Summary
Implements INT-1303: replaces band-sdk-python's hand-rolled reconnect/backoff/disconnect-classification logic in
WebSocketClient/BandLinkwith band_sdk_core'sSessionstate machine, per the ticket's attached implementation plan. All 5 migration steps shipped, squashed into 5 commits matching the plan's own step boundaries:_fast_session_policytest helper intotests/websocket/conftest.py(prep, test-only)SessionintoWebSocketClient.__init__/__aexit__(no behavior change)__aenter__'s retry loop withSession-driven backoffagent_control:supersedethroughSession.on_supersedeSession/SessionPolicycallDesign decisions (from the plan) — all 4 shipped as specified
Session.on_upgrade_rejected(usingRetry-Afteras a delay floor), instead of raised immediately as before.Session.on_supersede'sReconnectingbranch is implemented inBandLink._on_supersede, even though the platform hardcodesretryable=Falseon every real supersede today (only a synthetic regression test exercises it) — keeps the SDK correct about not treating it as terminal if that ever changes. It does not yet drive actual reconnect timing off that decision — see below.WebSocketDisconnectReasongaineddead_reason/stale_reasonfields.__aenter__'s terminal-failure path (a new_disconnect_reason_from_exception, since there's no platform wire payload for that case) — wideningBandLink.last_disconnect_reason's trigger scenarios to include a permanently-failed initial connect, not just a supersede.Out of scope (per the ticket, untouched): taking ownership of the vendored
PHXChannelsClient's internalauto_reconnectloop — separate, larger follow-up work, tracked as INT-1355 (filed during review: a retryable supersede'sretry_after_sis classified but not enforced without that ownership change; not reachable today since the platform always sendsretryable=False).Other real, intentional behavior change from Step 3: the initial-connect give-up bound is now a rolling 5-minute rapid-disconnect window (10 by default) instead of a fixed 11-attempt cap.
INT-1303: https://linear.app/thenvoi/issue/INT-1303/integrate-band-sdk-core-session-connection-reliability-into-band-sdk
Review history
Two rounds of external review, both verified and closed out:
/code-review high: confirmed a real exception-chaining regression (raise raise_exchad droppedfrom excfor the classified-upgrade-error case) — fixed, with a regression assertion. A comment-style finding (narrating removed mechanisms) — fixed./my-here-is-a-review(2 rounds) on the retryable-supersederetry_after_sgap: confirmed real but architecturally out of scope (needs the deferredauto_reconnect-ownership work) — filed as INT-1355, PR thread resolved.Test plan
uv run ruff check ./uv run ruff format ./uv run pyrefly check/ full unit suite (uv run pytest tests/ --ignore=tests/integration/ --ignore=tests/e2e/) — cleantest_aenter_restores_reconnect_after_successful_initial_connect,test_uses_retry_after_header_for_429_upgrade_error,test_supersede_event_records_terminal_reason_and_disables_reconnect,test_watchdog_forces_close_and_reconnect_when_ack_withheld,test_cancelled_connect_closes_the_half_opened_client_and_allows_retry,test_disconnect_after_supersede_still_cleans_up_websocket,test_close_without_supersede_leaves_disconnect_reason_empty, all oftests/websocket/test_watchdog.py)Dead, 429 upgrade rejection retried via the probe fallback then succeeding, retryable supersede leaves the connection non-terminal,connect()propagates a terminal initial-connect failure ontolast_disconnect_reason, exception-chaining regression guards🤖 Generated with Claude Code
https://claude.ai/code/session_01FuwaxzwGje1eq94W7V8zrW