Skip to content

Address connection races - #6867

Open
SteffenDE wants to merge 11 commits into
mainfrom
sd-connection-races
Open

Address connection races#6867
SteffenDE wants to merge 11 commits into
mainfrom
sd-connection-races

Conversation

@SteffenDE

@SteffenDE SteffenDE commented Oct 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Closes #6856.

I did multiple passes with Codex and Claude, since I was sure there are other cases. Turns out there indeed were quite a bunch of race conditions when handling connection lifecycle events (#6856 addresses one of them):

  • Channels stayed joined when the close event of a torn-down connection arrived late (teardown stops waiting after ~1.5s). They were not rejoined on the next connection, and the server answered their pushes with "unmatched topic"
  • A late close event replaced the new connection's onclose handler with a no-op, fired the onClose callbacks, errored the channels, and could schedule a reconnect that tore down the healthy new connection
  • A teardown started by a heartbeat timeout or a visibility change reconnected the socket even when disconnect was called while it was waiting for the connection to close
  • Calling connect twice while a disconnect was still tearing down the old connection replaced the first new connection without closing it, leaking it until the server timed it out
  • When two disconnects overlapped with a connect between them, the older teardown finishing made a following connect do nothing, leaving the socket disconnected
  • After a normal close by the server (1000), showing the page again still reconnected, while calling connect did nothing because the closed connection was still set
  • A channel error callback that disconnected or reconnected while a close was handled could lead to a reconnect that tore down the new connection; if this happened during a heartbeat timeout, this could reconnect and explicitly disconnected socket
  • A join buffered while a connection attempt failed was sent in addition to the rejoin on the next connection, leading to two joins with the server killing the first immediately (duplicate join)
  • A channel joined, or rejoined, from another channel's error callback was errored again by the same or a later error dispatch for that connection, e.g. when a failing connection emitted an error and then a close event
  • Heartbeats kept running after disconnect when the close event arrived late, so the next heartbeat timed out and reconnected the socket
  • A normal close by the server did not cancel the fallback health check, which then reconnected over LongPoll
  • Callbacks that disconnected during a fallback (onOpen, onError, or channel error callbacks) did not stop it from connecting over LongPoll
  • When the connection dropped while the page was hidden, showing the page created a new connection, but the reconnect that was already scheduled tore it down again. The same happened with Chrome firing both resume and visibilitychange events
  • In case an asynchronous encoder was used: an encode finishing after the connection was closed or replaced threw (no connection), or sent an outdated join on the replacement. A delayed decode delivered messages after the connection was replaced or closed. A late heartbeat reply even restarted the heartbeats, which reconnected the socket after the server had closed it with 1000.

Longpoll specific:

  • Longpoll fallback error callbacks accumulated with every reconnect, and the callback of an earlier attempt could trigger a second fallback
  • Aborting requests on close invoked their callbacks: synchronously for XHR, causing a spurious error and a nested close-and-retry; asynchronously for fetch, reviving a closed transport
  • Callbacks of requests cancelled by a retry were still handled
  • The first poll was still sent after an immediate disconnect
  • Messages of a poll response that were queued for delivery were still delivered after the transport closed
  • Sends after a retry were added to a batch whose timer had been cancelled, so they were never sent

Other notable changes:

  • Normal close: a close with code 1000 by the server is now treated like disconnect(), which seems to be the original intention from 470337d, so the socket doesn't reconnect on visibility change (I missed that behavior in Stop reconnecting when page is hidden #6534). Calling connect() afterwards opens a new connection (which it did not previously, that was a bug)
  • socket.push(data, channel): Push now passes its channel as a second argument. It's optional, so existing callers still work.
  • socket.conn: now a read-only getter for the current connection's transport to keep compatibility if anyone relied on socket.conn (undocumented)
  • replaceTransport now switches the transport before channel error callbacks run, so a callback that connects uses the new transport. On main the switch happened after the callbacks

Assisted-by: Codex GPT-6.1-Sol, Claude Code Opus 5.5

Most of the added lines are regression tests.

SteffenDE added 11 commits October 5, 2026 13:45
@SteffenDE
Scope socket state to the connection it belongs to
12f5732 
The socket kept the state of its current transport in its own fields,
so asynchronous events, timers and user callbacks belonging to one
connection could act on the next one, which caused a series of races.

Wrap each transport instance in a Connection, which owns the state of
that connection: its events are only handled while it is the socket's
current connection, and it records whether we closed it, whether the
socket gave up on it, whether its channels were errored, and which
channel joins it carried. Code continuing after user callbacks or
asynchronous waits checks whether the socket still uses its connection,
as callbacks can disconnect or connect at any time.

This fixes that:

- channels stayed joined when the close event arrived late or after
  connect() replaced the connection, resulting in "unmatched topic"
- events of a replaced connection acted on its replacement
- heartbeats kept running after disconnecting, or were restarted by a
  late reply, and then reconnected the socket
- teardowns, heartbeat timeouts and close or error handling reconnected
  after callbacks had disconnected or connected
- disconnecting while connecting fell back to LongPoll, fallback
  callbacks accumulated and acted on later attempts, and a normal close
  by the server did not cancel the fallback health check
- connecting twice while disconnecting leaked a connection
- a stale buffered join was sent in addition to the rejoin
- a heartbeat timeout errored channels while still connected
- resume and visibilitychange tore down the connection they created
- callbacks removed while dispatching a message were skipped
- a normal close by the server was reconnected on visibility change,
  while connect() did nothing
- channels joined by error callbacks were errored again
- asynchronous encoders and decoders acted on replaced or closed
  connections
@SteffenDE
Cancel pending LongPoll work when it stops
46437f5 
LongPoll aborted its requests when closing, but aborting can invoke the
request callbacks, synchronously for XHR and asynchronously for fetch,
and those still acted on the closed transport: a spurious error and a
retry, or making it active again after a disconnect. Messages queued
for delivery were still delivered after the close, a retry overrode a
close callback that closed it for good, and an open callback closing it
did not stop the next poll.

A request now belongs to the transport until it completes or the
transport stops, and callbacks of a request that does not belong to it
anymore are ignored. Stopping also cancels the queued messages and the
pending outgoing batch, a retry becomes active again before the close
callbacks run, and polling stops once the transport is closed.
@SteffenDE
Pass the channel along with its pushes
2462811 
Socket.push only received the message, so finding out whether a push
was outdated meant searching all channels for the one with its topic
and join ref, up to three times per push: when it was buffered and
flushed, when it was sent, and when its encoding finished.

Push now passes its channel to socket.push, which turns these checks
into comparing the push's join ref with the channel's current one and
checking whether the channel left.
@SteffenDE
fmt
3be13c6
@SteffenDE
Make socket tests catch the regressions they are meant for
9efa0a1 
Breaking the code each test guards showed that some tests no longer
failed for their regression:

- "does not throw when encoding finishes after disconnect completed"
  could not fail anymore, as its join push is dropped as outdated before
  it is sent. It now checks that a push is not sent once its connection
  was replaced while it was encoded.
- "does not connect twice" only checked after the scheduled reconnect,
  when an extra connection was still being torn down. It now also
  checks right after connect().
- "ignores errors and messages of a connection that was replaced"
  closed the old connection, which already detached its error handler.
  The old connection now still drains its buffer.
- "does not error channels when the connection never opened" now
  checks that the channel is not errored at all.

Also cover that replacing the transport while a teardown waits prevents
its reconnect, like disconnecting does.
@SteffenDE
Cover dropping the pending LongPoll batch on retry
9f36bb9 
When LongPoll stops, it now also drops a batch that was still collecting
sends, as its timer is cancelled. Otherwise, sends after a retry were
appended to that batch and never sent.

Also remove the test for a timeout after close, as the tests for abort
callbacks after close and for timeouts after a retry cover it.
@SteffenDE
Remove detaching the handlers of a torn down connection
13b7a61 
A connection only handles the events of its transport while it is the
socket's current connection, so detaching the handlers of a replaced or
torn down connection made no difference.
@SteffenDE
Cover cancelling a scheduled reconnect on a normal close
c814057 
A normal close by the server cancels a scheduled reconnect, which would
otherwise tear down a connection created by calling connect() again
before the reconnect was due.
@SteffenDE
Error channels after socket error callbacks replaced the connection
e04df0e 
onConnError skipped erroring the channels when its callbacks replaced
the connection. However, when a callback replaced the transport of a
connection that already failed (and was therefore no longer open),
replaceTransport did not error the channels either, so they stayed
joined and did not rejoin on the next connection.

Always error the channels for the connection afterwards, which only
errors the joins it carried once it was replaced.
@SteffenDE
Remove redundant checks of the fallback attempt
4c57d97 
fallback() checked whether its attempt was still current, but the
fallback timer is cleared whenever the attempt ends, and the error and
open callbacks only call it while the attempt is current. The ping of
the health check checked it as well, but its reply can only arrive on
the connection that sent it, and it shows that the primary transport
works even after a disconnect.
@SteffenDE
more fixes
67b8e28

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@SteffenDE