fix(rivetkit): stall stop handler until start completes#4192
Conversation
|
🚅 Deployed to the rivet-pr-4192 environment in rivet-frontend
|
PR Review: fix(rivetkit): stall stop handler until start completesThe core fix is correct and well-motivated: instead of immediately rejecting the start promise when stop is called during startup (which left the actor in a broken state), the new code awaits start completion before proceeding with teardown. The added test coverage is a solid improvement. A few issues worth addressing: High Priority1. No timeout on awaiting start — potential indefinite hang In 2. The onDestroy test has a logical flaw In the test Medium Priority3. lifecycleObserver is registered but never used
4. conn-error-serialization test may not correctly pass params to createConnState The test passes Low Priority5. Test naming inconsistency The test 6. Date.now() keys risk collisions in parallel test runs Using 7. Trivial assertion
8. conn.dispose() after a connection error may itself throw In the error-serialization test, Minor NitThe whitespace-only changes ( |
| this.#runnerStarted.resolve(undefined); | ||
| }, | ||
| onDisconnected: (_code, _reason) => {}, | ||
| onDisconnected: (_code, _reason) => { }, |
There was a problem hiding this comment.
Remove spaces inside the empty function braces to comply with Biome linter formatting rules. Change onDisconnected: (_code, _reason) => { }, to onDisconnected: (_code, _reason) => {},
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| return streamSSE(c, async (stream) => { | ||
| // NOTE: onAbort does not work reliably | ||
| stream.onAbort(() => {}); | ||
| stream.onAbort(() => { }); |
There was a problem hiding this comment.
Remove spaces inside the empty function braces to comply with Biome linter formatting rules. Change stream.onAbort(() => { }); to stream.onAbort(() => {});
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| import { actor } from "rivetkit"; | ||
|
|
||
| /** | ||
| * Actor designed to test start/stop race conditions. | ||
| * Has a slow initialization to make race conditions easier to trigger. | ||
| */ | ||
| export const startStopRaceActor = actor({ | ||
| state: { | ||
| initialized: false, | ||
| startTime: 0, | ||
| destroyCalled: false, | ||
| startCompleted: false, | ||
| }, | ||
| onWake: async (c) => { | ||
| c.state.startTime = Date.now(); | ||
|
|
||
| // Simulate slow initialization to create window for race condition | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
|
|
||
| c.state.initialized = true; | ||
| c.state.startCompleted = true; | ||
| }, | ||
| onDestroy: (c) => { | ||
| c.state.destroyCalled = true; | ||
| // Don't save state here - the actor framework will save it automatically | ||
| }, | ||
| actions: { | ||
| getState: (c) => { | ||
| return { | ||
| initialized: c.state.initialized, | ||
| startTime: c.state.startTime, | ||
| destroyCalled: c.state.destroyCalled, | ||
| startCompleted: c.state.startCompleted, | ||
| }; | ||
| }, | ||
| ping: (c) => { | ||
| return "pong"; | ||
| }, | ||
| destroy: (c) => { | ||
| c.destroy(); | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| /** | ||
| * Observer actor to track lifecycle events from other actors | ||
| */ | ||
| export const lifecycleObserver = actor({ | ||
| state: { | ||
| events: [] as Array<{ | ||
| actorKey: string; | ||
| event: string; | ||
| timestamp: number; | ||
| }>, | ||
| }, | ||
| actions: { | ||
| recordEvent: (c, params: { actorKey: string; event: string }) => { | ||
| c.state.events.push({ | ||
| actorKey: params.actorKey, | ||
| event: params.event, | ||
| timestamp: Date.now(), | ||
| }); | ||
| }, | ||
| getEvents: (c) => { | ||
| return c.state.events; | ||
| }, | ||
| clearEvents: (c) => { | ||
| c.state.events = []; | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Run the Biome formatter on this file to ensure proper formatting and sorted imports. The file is new and likely has formatting issues that don't match the project's style guide.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
1b0d832 to
0f6bab1
Compare
| import { startStopRaceActor, lifecycleObserver } from "./start-stop-race"; | ||
| import { connErrorSerializationActor } from "./conn-error-serialization"; |
There was a problem hiding this comment.
Imports should be sorted alphabetically. Consider reordering these imports to maintain consistent sorting.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| import { runActorConnHibernationTests } from "./tests/actor-conn-hibernation"; | ||
| import { runActorConnStateTests } from "./tests/actor-conn-state"; | ||
| import { runActorDbTests } from "./tests/actor-db"; | ||
| import { runConnErrorSerializationTests } from "./tests/conn-error-serialization"; |
There was a problem hiding this comment.
This import should be sorted alphabetically with the other imports to maintain consistent ordering.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
0f6bab1 to
8f7b569
Compare
8f7b569 to
3e27725
Compare
3e27725 to
02b2c93
Compare
02b2c93 to
2d6e590
Compare

Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: