chore: initial state machine plumbing - #2046
Conversation
|
size-limit report 📦
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| this.sendLifecycleInput( | ||
| this.lifecycleState === 'reconnecting' | ||
| ? { type: 'reconnectComplete' } | ||
| : { type: 'connectComplete' }, | ||
| ); |
There was a problem hiding this comment.
🟡 A signal connection that finishes after the client gave up still starts a heartbeat timer that never stops
The result of moving the connection lifecycle forward is discarded (sendLifecycleInput(...) at src/api/SignalClient.ts:1114-1118) before the heartbeat timer and message reader are started, so a connection that completes after the client already gave up leaves a repeating timer running forever.
Impact: A cancelled or failed connect can leave a permanently repeating background timer and a stray message reader behind for the lifetime of the page.
Ignored lifecycle transition still starts pings and the read loop
handleSignalConnected picks reconnectComplete vs connectComplete from the current lifecycle state, but never checks whether the machine accepted the input. Both inputs are only handled in connecting/reconnecting (src/api/SignalClientStateMachine.ts:80-118), so if the lifecycle has already left those states the transition is a no-op while lines 1119-1122 still run.
Reachable path: Room.disconnect() aborts a pending connect (src/room/Room.ts:1157), the abort handler inside connect() rejects the promise (src/api/SignalClient.ts:434) while the body is still awaiting the first message read, and join()'s catch drives the machine to closed via connectFailed (src/api/SignalClient.ts:348). If the first message then arrives, handleSignalConnected runs with lifecycleState === 'closed': connectComplete is dropped, yet startPingInterval() arms a CriticalTimers.setInterval that is only ever cleared by teardownTransport()/close() — which have already run — and startReadingLoop begins dispatching signal messages (participant updates, leave, etc.) into the callbacks of an abandoned session.
Guarding on the boolean that sendLifecycleInput already returns keeps the transport work tied to an accepted transition.
| this.sendLifecycleInput( | |
| this.lifecycleState === 'reconnecting' | |
| ? { type: 'reconnectComplete' } | |
| : { type: 'connectComplete' }, | |
| ); | |
| const accepted = this.sendLifecycleInput( | |
| this.lifecycleState === 'reconnecting' | |
| ? { type: 'reconnectComplete' } | |
| : { type: 'connectComplete' }, | |
| ); | |
| if (!accepted) { | |
| // the lifecycle has moved on (closed or replaced by a newer attempt): don't attach pings | |
| // or a reading loop to a connection nobody owns anymore | |
| this.log.debug('signal connected after the attempt was abandoned, ignoring'); | |
| clearTimeout(timeoutHandle); | |
| return; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
This generally looks like a great start to me!
One thing I would find really helpful would be some sort of way to visualize the state machine. If there isn't a tool already which can read the machina state machine and generate a mermaid / graphviz / etc type visualization from it, I suspect this would be pretty easy to add. I'm thinking some sort of small script which could import in the SignalClient (and in the future, other state machines all combined together), convert to mermaid or graphviz, then generate a big svg. This is also something a LLM could use to generate diagrams for describing potential changes.
The dream / probably a stretch goal would be some way for the example app to be able to show the state machine live so as you test out scenarios you can actually watch the "active" state propagate through the state machine interactively. At a past job I did this for a large distributed state machine which was not too dissimilar from livekit's domain and it proved invaluable both for engineering understanding and explaining failure modes to less technical folks.
| * Public projection of the lifecycle machine's states. `new`, `offline` and `closed` are all | ||
| * reported as `DISCONNECTED`: they differ in what may happen next | ||
| */ | ||
| const lifecycleToConnectionState: Record<SignalLifecycleState, SignalConnectionState> = { |
There was a problem hiding this comment.
nitpick: should this constant be in SCREAMING_SNAKE_CASE? There's a few other constants as well like signalStates which I think this also applies to.
| const lifecycleToConnectionState: Record<SignalLifecycleState, SignalConnectionState> = { | |
| const LIFECYCLE_TO_CONNECTION_STATE: Record<SignalLifecycleState, SignalConnectionState> = { |
|
|
||
| const signalStates = { | ||
| new: { | ||
| connect: startConnect, | ||
| reconnect: startReconnect, | ||
| close: requestClose, | ||
| }, | ||
| connecting: { | ||
| connect: startConnect, | ||
| reconnect: startReconnect, | ||
| connectComplete: 'connected', |
There was a problem hiding this comment.
nitpick: you may consider breaking out some of these repeated states instead into some more "component level" objects which can be spread in rather than breaking out hte functions individually. As is I find myself going back and forth a lot between function and state transition map when reasoning about this.
| const signalStates = { | |
| new: { | |
| connect: startConnect, | |
| reconnect: startReconnect, | |
| close: requestClose, | |
| }, | |
| connecting: { | |
| connect: startConnect, | |
| reconnect: startReconnect, | |
| connectComplete: 'connected', | |
| const common = { | |
| connect: on<'connect'>(({ ctx }) => { | |
| ctx.attemptId += 1; | |
| ctx.lastError = undefined; | |
| return 'connecting'; | |
| }), | |
| reconnect: on<'reconnect'>(({ ctx }) => { | |
| ctx.attemptId += 1; | |
| ctx.lastError = undefined; | |
| return 'reconnecting'; | |
| }), | |
| close: on<'close'>(({ ctx }, event) => { | |
| ctx.closeReason = event.reason; | |
| return 'disconnecting'; | |
| }), | |
| }; | |
| const signalStates = { | |
| new: common, | |
| connecting: { | |
| ...common, // etc | |
| connectComplete: 'connected', |
2 participants
No description provided.