You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Code Review: chore: move ActorDefinition and ActorInstance to interfaces
This is a well-structured architectural refactor. Introducing a minimal ActorInstance interface that both StaticActorInstance and DynamicActorInstance implement is sound and enables the engine to handle dynamic actors alongside static ones without leaking internal implementation details. The approach is largely correct and coherent. A few concerns below.
Architecture and Design
DynamicActorDefinition.instantiate() throws at runtime. The method is required by the ActorDefinition interface contract but deliberately throws:
instantiate(): ActorInstance<...>{thrownewError("dynamic actor definitions are instantiated by the runtime driver",);}
This violates the Liskov Substitution Principle. The guards in actor-driver.ts and global-state.ts correctly check isDynamicActorDefinition before calling instantiate(), but nothing at the type level prevents a future code path from calling instantiate() on an AnyActorDefinition and hitting this unexpected throw. Consider either narrowing the return type signature or splitting DynamicActorDefinition away from the ActorDefinition interface entirely.
Three copies of loadStaticActor. The same guard helper appears independently in router-endpoints.ts, router-websocket-endpoints.ts, and router.ts. All three call actorDriver.loadActor, assert the result is a StaticActorInstance, and throw if not. Additionally, error types are inconsistent: router-endpoints.ts throws errors.InternalError (the proper domain error) while the other two throw a plain new Error(...). This should be unified into a shared utility with consistent error handling.
ExtractActorState, ExtractActorConnParams, ExtractActorConnState now hardcode StaticActorInstance. These utility types only return non-never for StaticActorInstance. If the intent is that extraction only makes sense for static actors, the type constraint should be A extends AnyStaticActorInstance rather than the misleading A extends AnyActorInstance.
Potential Bugs
Race condition in #runnerDynamicWebSocket message flush. The flush runs:
flushPendingMessages is not awaited, and the outer websocket.addEventListener("message", ...) handler also calls runtime.forwardIncomingWebSocketMessage without coordination. If messages arrive while flushPendingMessages is draining the queue (after actorWebSocketReady = true but before clearing all pending entries), a concurrent incoming message will also try to forward immediately. This can cause messages to be delivered out of order.
Silent discard of messages to a stopping actor. In the engine WebSocket handler, messages to a stopping dynamic actor are silently dropped with return. For hibernatable WebSocket paths, the ACK is never sent for the dropped message. If the gateway waits on an ACK before delivering the next message, this will stall the connection without a clean close. It would be safer to close the WebSocket explicitly when discarding due to a stopping actor.
dynamicGetHibernatingWebSocketsEnvelope silently drops connections when symbol lookup fails. The isolate code accesses conn[CONN_STATE_MANAGER_SYMBOL]. If the isolate's require("rivetkit") resolves a different symbol than the one used internally (e.g., due to module caching or version mismatch), the lookup returns undefined with no warning, silently omitting connections from the hibernating snapshot.
Code Quality
(session.websocket as any).triggerMessage(...) casts. The as any casts suggest the VirtualWebSocket type accessible from host-runtime.ts may not yet have the updated triggerMessage signature (with the new rivetMessageIndex parameter). If the shared package type is now properly typed, these casts should be removed.
DynamicActorConfigInput may be missing noSleep. The old dynamicActor implementation set noSleep: true on the host-side placeholder actor. The new DynamicActorConfigInput only exposes GlobalActorOptionsInput, and noSleep lives in InstanceActorOptionsBaseSchema, so it cannot be passed through. Worth confirming this was intentionally dropped rather than accidentally omitted.
Inconsistent indentation in several new files. There are a few spots with misaligned tab depths in mod.ts (around db.createClient), host-runtime.ts (in #sendWebSocketMessage and openWebSocket array arguments), and driver-engine-dynamic.test.ts (the describe callback body). These look like carry-over formatting artifacts.
Security
No new attack surface is introduced. The hibernation metadata serialization (base64 of ArrayBuffer across the isolate boundary) is handled correctly, including the byteOffset fix when slicing from a Node.js Buffer.
Test Coverage
The new driver-engine-dynamic.test.ts covers the main happy paths well (URL-loaded source, action invocation, KV, WebSocket ping/pong, sleep/wake, alarms). A few gaps worth noting:
No test for the hibernatable WebSocket path (isHibernatable: true / isRestoringHibernatable: true flow).
No test for the error path when the dynamic runtime fails to start (verifying cleanup from #dynamicRuntimes).
No test verifying that messages buffered while actorWebSocketReady = false are flushed in order.
The readWebSocketJson helper has a hardcoded 5-second timeout with no per-test override, which may be fragile on slow CI for the sleep/alarm tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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: