-
Notifications
You must be signed in to change notification settings - Fork 154
fix(rivetkit): stall stop handler until start completes #4192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| import { actor } from "rivetkit"; | ||
| import { ActorError } from "@/actor/errors"; | ||
|
|
||
| // Custom error that will be thrown in createConnState | ||
| class CustomConnectionError extends ActorError { | ||
| constructor(message: string) { | ||
| super("connection", "custom_error", message, { public: true }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Actor that throws a custom error in createConnState to test error serialization | ||
| */ | ||
| export const connErrorSerializationActor = actor({ | ||
| state: { | ||
| value: 0, | ||
| }, | ||
| createConnState: (_c, params: { shouldThrow?: boolean }) => { | ||
| if (params.shouldThrow) { | ||
| throw new CustomConnectionError("Test error from createConnState"); | ||
| } | ||
| return { initialized: true }; | ||
| }, | ||
| actions: { | ||
| getValue: (c) => c.state.value, | ||
| }, | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| 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 = []; | ||
| }, | ||
| }, | ||
| }); | ||
|
Comment on lines
+1
to
+71
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ import { runActorConnTests } from "./tests/actor-conn"; | |
| 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"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This import should be sorted alphabetically with the other imports to maintain consistent ordering. Spotted by Graphite Agent (based on CI logs) |
||
| import { runActorDestroyTests } from "./tests/actor-destroy"; | ||
| import { runActorDriverTests } from "./tests/actor-driver"; | ||
| import { runActorErrorHandlingTests } from "./tests/actor-error-handling"; | ||
|
|
@@ -111,6 +112,8 @@ export function runDriverTests( | |
|
|
||
| runActorConnHibernationTests(driverTestConfig); | ||
|
|
||
| runConnErrorSerializationTests(driverTestConfig); | ||
|
|
||
| runActorDbTests(driverTestConfig); | ||
|
|
||
| runActorDestroyTests(driverTestConfig); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| import { describe, expect, test } from "vitest"; | ||
| import type { DriverTestConfig } from "../mod"; | ||
| import { setupDriverTest } from "../utils"; | ||
|
|
||
| export function runActorLifecycleTests(driverTestConfig: DriverTestConfig) { | ||
| describe("Actor Lifecycle Tests", () => { | ||
| test("actor stop during start waits for start to complete", async (c) => { | ||
| const { client } = await setupDriverTest(c, driverTestConfig); | ||
|
|
||
| const actorKey = `test-stop-during-start-${Date.now()}`; | ||
|
|
||
| // Create actor - this starts the actor | ||
| const actor = client.startStopRaceActor.getOrCreate([actorKey]); | ||
|
|
||
| // Immediately try to call an action and then destroy | ||
| // This creates a race where the actor might not be fully started yet | ||
| const pingPromise = actor.ping(); | ||
|
|
||
| // Get actor ID | ||
| const actorId = await actor.resolve(); | ||
|
|
||
| // Destroy immediately while start might still be in progress | ||
| await actor.destroy(); | ||
|
|
||
| // The ping should still complete successfully because destroy waits for start | ||
| const result = await pingPromise; | ||
| expect(result).toBe("pong"); | ||
|
|
||
| // Verify actor was actually destroyed | ||
| let destroyed = false; | ||
| try { | ||
| await client.startStopRaceActor.getForId(actorId).ping(); | ||
| } catch (err: any) { | ||
| destroyed = true; | ||
| expect(err.group).toBe("actor"); | ||
| expect(err.code).toBe("not_found"); | ||
| } | ||
| expect(destroyed).toBe(true); | ||
| }); | ||
|
|
||
| test("actor stop before actor instantiation completes cleans up handler", async (c) => { | ||
| const { client } = await setupDriverTest(c, driverTestConfig); | ||
|
|
||
| const actorKey = `test-stop-before-instantiation-${Date.now()}`; | ||
|
|
||
| // Create multiple actors rapidly to increase chance of race | ||
| const actors = Array.from({ length: 5 }, (_, i) => | ||
| client.startStopRaceActor.getOrCreate([ | ||
| `${actorKey}-${i}`, | ||
| ]), | ||
| ); | ||
|
|
||
| // Resolve all actor IDs (this triggers start) | ||
| const ids = await Promise.all(actors.map((a) => a.resolve())); | ||
|
|
||
| // Immediately destroy all actors | ||
| await Promise.all(actors.map((a) => a.destroy())); | ||
|
|
||
| // Verify all actors were cleaned up | ||
| for (const id of ids) { | ||
| let destroyed = false; | ||
| try { | ||
| await client.startStopRaceActor.getForId(id).ping(); | ||
| } catch (err: any) { | ||
| destroyed = true; | ||
| expect(err.group).toBe("actor"); | ||
| expect(err.code).toBe("not_found"); | ||
| } | ||
| expect(destroyed, `actor ${id} should be destroyed`).toBe( | ||
| true, | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| test("onBeforeActorStart completes before stop proceeds", async (c) => { | ||
| const { client } = await setupDriverTest(c, driverTestConfig); | ||
|
|
||
| const actorKey = `test-before-actor-start-${Date.now()}`; | ||
|
|
||
| // Create actor | ||
| const actor = client.startStopRaceActor.getOrCreate([actorKey]); | ||
|
|
||
| // Call action to ensure actor is starting | ||
| const statePromise = actor.getState(); | ||
|
|
||
| // Destroy immediately | ||
| await actor.destroy(); | ||
|
|
||
| // State should be initialized because onBeforeActorStart must complete | ||
| const state = await statePromise; | ||
| expect(state.initialized).toBe(true); | ||
| expect(state.startCompleted).toBe(true); | ||
| }); | ||
|
|
||
| test("multiple rapid create/destroy cycles handle race correctly", async (c) => { | ||
| const { client } = await setupDriverTest(c, driverTestConfig); | ||
|
|
||
| // Perform multiple rapid create/destroy cycles | ||
| for (let i = 0; i < 10; i++) { | ||
| const actorKey = `test-rapid-cycle-${Date.now()}-${i}`; | ||
| const actor = client.startStopRaceActor.getOrCreate([ | ||
| actorKey, | ||
| ]); | ||
|
|
||
| // Trigger start | ||
| const resolvePromise = actor.resolve(); | ||
|
|
||
| // Immediately destroy | ||
| const destroyPromise = actor.destroy(); | ||
|
|
||
| // Both should complete without errors | ||
| await Promise.all([resolvePromise, destroyPromise]); | ||
| } | ||
|
|
||
| // If we get here without errors, the race condition is handled correctly | ||
| expect(true).toBe(true); | ||
| }); | ||
|
|
||
| test("actor stop called with no actor instance cleans up handler", async (c) => { | ||
| const { client } = await setupDriverTest(c, driverTestConfig); | ||
|
|
||
| const actorKey = `test-cleanup-no-instance-${Date.now()}`; | ||
|
|
||
| // Create and immediately destroy | ||
| const actor = client.startStopRaceActor.getOrCreate([actorKey]); | ||
| const id = await actor.resolve(); | ||
| await actor.destroy(); | ||
|
|
||
| // Try to recreate with same key - should work without issues | ||
| const newActor = client.startStopRaceActor.getOrCreate([ | ||
| actorKey, | ||
| ]); | ||
| const result = await newActor.ping(); | ||
| expect(result).toBe("pong"); | ||
|
|
||
| // Clean up | ||
| await newActor.destroy(); | ||
| }); | ||
|
|
||
| test("onDestroy is called even when actor is destroyed during start", async (c) => { | ||
| const { client } = await setupDriverTest(c, driverTestConfig); | ||
|
|
||
| const actorKey = `test-ondestroy-during-start-${Date.now()}`; | ||
|
|
||
| // Create actor | ||
| const actor = client.startStopRaceActor.getOrCreate([actorKey]); | ||
|
|
||
| // Start and immediately destroy | ||
| const statePromise = actor.getState(); | ||
| await actor.destroy(); | ||
|
|
||
| // Verify onDestroy was called (requires actor to be started) | ||
| const state = await statePromise; | ||
| expect(state.destroyCalled).toBe(true); | ||
| }); | ||
| }); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| import { describe, expect, test } from "vitest"; | ||
| import type { DriverTestConfig } from "../mod"; | ||
| import { setupDriverTest } from "../utils"; | ||
|
|
||
| export function runConnErrorSerializationTests(driverTestConfig: DriverTestConfig) { | ||
| describe("Connection Error Serialization Tests", () => { | ||
| test("error thrown in createConnState preserves group and code through WebSocket serialization", async (c) => { | ||
| const { client } = await setupDriverTest(c, driverTestConfig); | ||
|
|
||
| const actorKey = `test-error-serialization-${Date.now()}`; | ||
|
|
||
| // Create actor handle with params that will trigger error in createConnState | ||
| const actor = client.connErrorSerializationActor.getOrCreate( | ||
| [actorKey], | ||
| { params: { shouldThrow: true } }, | ||
| ); | ||
|
|
||
| // Try to connect, which will trigger error in createConnState | ||
| const conn = actor.connect(); | ||
|
|
||
| // Wait for connection to fail | ||
| let caughtError: any; | ||
| try { | ||
| // Try to call an action, which should fail because connection couldn't be established | ||
| await conn.getValue(); | ||
| } catch (err) { | ||
| caughtError = err; | ||
| } | ||
|
|
||
| // Verify the error was caught | ||
| expect(caughtError).toBeDefined(); | ||
|
|
||
| // Verify the error has the correct group and code from the original error | ||
| // Original error: new CustomConnectionError("...") with group="connection", code="custom_error" | ||
| expect(caughtError.group).toBe("connection"); | ||
| expect(caughtError.code).toBe("custom_error"); | ||
|
|
||
| // Clean up | ||
| await conn.dispose(); | ||
| }); | ||
|
|
||
| test("successful createConnState does not throw error", async (c) => { | ||
| const { client } = await setupDriverTest(c, driverTestConfig); | ||
|
|
||
| const actorKey = `test-no-error-${Date.now()}`; | ||
|
|
||
| // Create actor handle with params that will NOT trigger error | ||
| const actor = client.connErrorSerializationActor.getOrCreate( | ||
| [actorKey], | ||
| { params: { shouldThrow: false } }, | ||
| ); | ||
|
|
||
| // Connect without triggering error | ||
| const conn = actor.connect(); | ||
|
|
||
| // This should succeed | ||
| const value = await conn.getValue(); | ||
| expect(value).toBe(0); | ||
|
|
||
| // Clean up | ||
| await conn.dispose(); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.