Skip to content

feat: add on_heartbeat_ack callback and close_connection method - #54

Merged
AlexanderZ-Band merged 6 commits into
mainfrom
int-1323-add-shared-heartbeat-interval-dead-threshold-policy-to-band
Aug 30, 2026
Merged

AlexanderZ-Band merged 6 commits into
mainfrom
int-1323-add-shared-heartbeat-interval-dead-threshold-policy-to-band

Conversation

@AlexanderZ-Band

Copy link
Copy Markdown
Contributor

Summary

In short: adds two abilities band-sdk-python needs to build a heartbeat "watchdog" — a way to get notified when the server confirms a heartbeat (on_heartbeat_ack), and a way to force a reconnect without permanently shutting the client down (close_connection). While building it, found and fixed two real bugs: an oversized close reason could crash the client, and a forced reconnect wasn't guaranteed to actually happen (it depended on a code the remote server echoes back, not something we control) — both fixed. Also cleaned up a few duplicated magic numbers/strings into shared, named constants.

Part of INT-1323band-sdk-core (band-ai/band-sdk-core#60, merged) added a shared heartbeat_interval/dead_threshold policy that band-sdk-python will use to drive a watchdog against this client. Today there's no public way to observe heartbeat-ack liveness or force a reconnect from outside: phx responses are routed straight to a private callback (protocol_handler.py:158-161), and the only public teardown method, shutdown(), permanently ends the client.

  • on_heartbeat_ack: Callable[[], None] | None — new constructor callback, mirrors on_reconnect/on_disconnect. Fires from the existing _handle_heartbeat_response right after _pending_heartbeat_ref clears; exceptions are caught and logged, same as the other two callbacks. Rejects an async callback passed by mistake (closes the coroutine and logs it as an error) instead of silently discarding it, since — unlike on_reconnect/on_disconnect — this one runs inline on the message-routing hot path and must stay synchronous and fast.
  • close_connection(reason: str) — new public method. Verified shutdown() doesn't fit a watchdog's "close and let it reconnect" need: it cancels the supervisor task and transitions to CLOSED permanently. Also verified (correcting an earlier assumption) that band-sdk-python's auto_reconnect=False only guards its initial connect retry loop — once connected, the same PHXChannelsClient instance's own supervisor loop owns every subsequent reconnect, so there's no outer loop to rebuild it. close_connection closes just the current socket with a close code (4000, RFC 6455 §7.4.2 private-use range) outside _classify_disconnect's special-cased codes, so the existing reconnect classification decides what happens next — no new disconnect-handling branch.
    • The reason is truncated to fit RFC 6455 §5.5.1's 125-byte close-frame cap (123 bytes after the 2-byte code) before being sent — a realistic watchdog message could otherwise raise an uncaught ProtocolError.
    • A forced close now always reconnects, regardless of what close code the remote happens to echo back. _classify_disconnect reads the code the remote echoes, not the one we sent — a server that doesn't echo 4000 (e.g. answering 1000) could silently defeat close_connection under the default reconnect_on_normal_close=False. A _forced_close_pending flag now bypasses classification entirely for a close we initiated ourselves.

Also, while reviewing: reconnect_controller.py's close-code branches (1000, 1001, 1008, 1012, 1013) now use websockets.frames.CloseCode instead of raw ints; extracted _invoke_callback_safely for the on_reconnect/on_disconnect try/except-log duplication; added a shared PHOENIX_TOPIC constant (phx_messages.py) instead of the "phoenix" literal duplicated in protocol_handler.py/supervisor.py.

Test plan

  • uv run pytest — 114 passed
  • uv run ruff check / uv run ruff format --check — clean
  • uv run pyrefly check / uv run pyright — 0 errors

Note

Supersedes #53, which was opened as a cross-repo PR from a personal fork (AlexanderZ-Band) because I didn't have write access to this repo at the time. Now that I do, this reopens the same branch/commits as a same-repo PR instead.

Exposes heartbeat-ack liveness so a caller can drive its own
dead-threshold watchdog: today `topic == "phoenix"` responses are
routed to a private callback and never reach public dispatch.
on_heartbeat_ack fires on every ack; close_connection lets a watchdog
force a close that flows through the existing reconnect classification,
instead of a full shutdown().
DisconnectCallback and ReconnectCallback are both re-exported
alongside PHXChannelsClient; HeartbeatAckCallback was missing from
both the import and __all__, so band-sdk-python can't type its
on_heartbeat_ack callback against it.
close_connection() sent an unbounded reason straight into the WS close
frame with no length check; RFC 6455 caps control frames at 125 bytes
total (2 for the code, 123 left for the reason), so a realistic
watchdog message could raise an uncaught ProtocolError. Truncate to
fit.

Also, _classify_disconnect keyed off close_code, but that reflects the
code the remote echoes back, not the 4000 we sent — a server that
doesn't echo it (e.g. answering 1000) could silently defeat
close_connection under the default reconnect_on_normal_close=False.
Track that a close was force-initiated and always reconnect for it,
bypassing classification instead of trusting the echo.

Also swap reconnect_controller.py's raw close-code literals for
websockets.frames.CloseCode (already RFC 6455-named there); _FORCED_CLOSE_CODE
stays a private constant since 4000 has no RFC name (private-use range).
on_heartbeat_ack is typed sync but nothing stopped a caller from
passing an async function by analogy with on_reconnect/on_disconnect;
the coroutine was silently constructed and discarded, so its body
never ran. Detect that case, close the coroutine, and log it as an
error instead of a silent no-op. Also document that this callback
runs inline on the message-routing hot path (unlike the two async
callbacks) and must be synchronous and fast.

Extract _invoke_callback_safely for the on_reconnect/on_disconnect
try/except-log duplication.

Add PHOENIX_TOPIC in phx_messages.py (matching PHXEvent's existing
"phx_*" constants) instead of the raw "phoenix" literal duplicated in
protocol_handler.py and supervisor.py.
A mutated-list flag reads the same and needs no nonlocal binding.
@linear-code

linear-code Bot commented Aug 30, 2026

Copy link
Copy Markdown

INT-1323

@AlexanderZ-Band
AlexanderZ-Band requested a review from a team August 30, 2026 05:50
AlexanderZ-Band added a commit to band-ai/band-sdk-python that referenced this pull request Aug 30, 2026
Removes the [tool.uv.sources] override early rather than waiting for
PR band-ai/phoenix-channels-python-client#54 to merge and release.
The >=0.3.0 floor now resolves against PyPI as a real consumer would
see it, which currently fails (only <=0.2.3 is published) -- expected,
and self-resolving the moment #54 releases 0.3.0, with no further edit
needed here.

uv.lock is intentionally left unregenerated (uv lock cannot resolve
without the override); it will be regenerated once #54 releases.

@amit-gazal-band amit-gazal-band left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One non-blocking finding worth a follow-up:

close_connection() can leave _forced_close_pending stuck True, causing the wrong reconnect decision on a later disconnect.

  • close_connection() sets _forced_close_pending = True then calls await connection.close(...) with no exception handling — unlike the equivalent close in _cleanup_connection, which is wrapped in try/except Exception.
  • The flag is only reset once the current disconnect cycle runs to completion (lines ~290–291).
  • If connection.close() raises before that cycle completes (e.g. cancellation during shutdown, a non-ConnectionClosed error), the exception propagates and the flag stays True.
  • On the next disconnect — even a legitimate server-initiated close (policy violation, or a normal close with reconnect_on_normal_close=False) — the supervisor takes the forced_close branch instead of running _classify_disconnect, reconnecting unconditionally and silently overriding the configured reconnect policy.

Suggest wrapping the connection.close() call in close_connection() in try/except like _cleanup_connection, and/or resetting _forced_close_pending on the exception path.

@AlexanderZ-Band
AlexanderZ-Band merged commit 21226c1 into main Aug 30, 2026
17 checks passed
@AlexanderZ-Band
AlexanderZ-Band deleted the int-1323-add-shared-heartbeat-interval-dead-threshold-policy-to-band branch August 30, 2026 06:23
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