Skip to content

fix: clear pendingHeartbeatRef unconditionally in onConnOpen - #54

Open
g0rdonL wants to merge 1 commit into
supabase:mainfrom
g0rdonL:fix/worker-mode-heartbeat-ref-clear
Open

fix: clear pendingHeartbeatRef unconditionally in onConnOpen#54
g0rdonL wants to merge 1 commit into
supabase:mainfrom
g0rdonL:fix/worker-mode-heartbeat-ref-clear

Conversation

@g0rdonL

Copy link
Copy Markdown

Bug

onConnOpen() only clears a stranded pendingHeartbeatRef when autoSendHeartbeat is true (it happens inside the resetHeartbeat() call, which is itself gated on that flag). Clients configured with autoSendHeartbeat: false — notably @supabase/realtime-js's RealtimeClient in worker: true mode, a normal production configuration where heartbeats are driven by a Worker instead of the socket's own timer — never get that clear.

Consequence: a pendingHeartbeatRef armed on one connection and left pending by a drop-with-beat-in-flight (network blip, laptop sleep, server-side close) survives onto the next, healthy connection. That connection's first externally-driven heartbeat then sees the stale non-null ref, takes the timeout branch in heartbeatTimeout(), and force-closes the still-healthy socket with "heartbeat timeout" having sent zero frames.

Filed upstream as supabase/supabase-js#2625 (realtime-js consumes this library and surfaces the bug there — repro included, verified there against the published @supabase/realtime-js/@supabase/phoenix packages before tracing it to this file).

Fix

Hoist the pendingHeartbeatRef/heartbeatSentAt clear out of the autoSendHeartbeat gate in onConnOpen() so it always runs on connection open — matching what resetHeartbeat() already does unconditionally for the autoSendHeartbeat: true path. Only the timer-(re)arming call stays gated, which is unaffected since worker-mode clients drive heartbeats externally.

Verification

  • Added a regression test (onConnOpen › clears a stale pendingHeartbeatRef even when autoSendHeartbeat is false) that fails against unfixed main (confirmed) and passes with this change.
  • Full suite: npm test → 217/220 passing (3 pre-existing skips), up from 216/219 before the new test.
  • npm run lint clean.
  • Also re-ran the standalone repro from the linked issue (fake WebSocket + fake Worker driving worker: true mode end-to-end through @supabase/realtime-js) against the published packages to confirm the observed behavior before tracing the root cause here.

onConnOpen() only cleared a stranded pendingHeartbeatRef when
autoSendHeartbeat was true (inside the resetHeartbeat() call gated by
that flag). Clients configured with autoSendHeartbeat: false — e.g.
@supabase/realtime-js's RealtimeClient in worker: true mode, where
heartbeats are driven by a Worker instead of the socket's own timer —
never got that clear.
Consequence: a pendingHeartbeatRef armed on one connection and left
pending by a drop-with-beat-in-flight (network blip, sleep, server
close) survives onto the next, healthy connection. That connection's
first externally-driven heartbeat then sees the stale non-null ref,
takes the timeout branch in heartbeatTimeout(), and force-closes the
still-healthy socket with "heartbeat timeout" having sent zero frames.
Fix: hoist the pendingHeartbeatRef/heartbeatSentAt clear out of the
autoSendHeartbeat gate in onConnOpen() so it always runs on connection
open, matching what resetHeartbeat() already does unconditionally for
the autoSendHeartbeat: true path. Only the timer-(re)arming call stays
gated, which is unaffected since worker-mode clients drive heartbeats
externally.
Added a regression test covering onConnOpen() with
autoSendHeartbeat: false and a pre-armed stale ref. Full suite:
217/220 passing (3 pre-existing skips), eslint clean.
Sign up for freeto 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.

1 participant

@g0rdonL