chore(rivetkit): split ActorInstance logic in to multiple classes #3421

NathanFlurry · 2025-11-09T05:06:31Z

No description provided.

vercel · 2025-11-09T05:06:33Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Nov 13, 2025 1:32am

3 Skipped Deployments

Project	Deployment	Preview	Updated (UTC)
rivet-cloud	Ignored		Nov 13, 2025 1:32am
rivet-inspector	Ignored	Preview	Nov 13, 2025 1:32am
rivet-site	Ignored	Preview	Nov 13, 2025 1:32am

NathanFlurry · 2025-11-09T05:06:47Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

This stack of pull requests is managed by Graphite. Learn more about stacking.

claude · 2025-11-09T05:08:41Z

PR Review: Split ActorInstance logic into multiple classes

Summary

This is a well-executed refactoring that improves code organization by splitting the monolithic ActorInstance class (~2200 lines) into focused, single-responsibility manager classes. The changes enhance maintainability while preserving existing functionality.

Positive Aspects ✅

1. Excellent Code Organization

Clear separation of concerns with dedicated managers:
- ConnectionManager - connection lifecycle and hibernation
- StateManager - state persistence and change tracking
- EventManager - event broadcasting and handling
- ScheduleManager - scheduled task management
Each manager has well-defined responsibilities and encapsulation

2. API Improvements

Changed from underscored private-style methods (_onAlarm, _broadcast, _sendMessage) to proper public methods (onAlarm, broadcast, sendMessage)
Better use of TypeScript symbols for internal state (CONN_PERSIST_SYMBOL, ACTOR_INSTANCE_PERSIST_SYMBOL)
More intuitive driver interface with factory functions (createWebSocketSocket, createHttpSocket)

3. Improved Driver Architecture

Simplified driver interface by moving driver-specific logic into factory functions
Eliminated discriminated union pattern (ConnDriverState) in favor of closure-based approach
Better encapsulation with drivers holding their own state

4. File Organization

Logical directory structure (actor/conn/, actor/contexts/, actor/instance/)
Clear module boundaries with appropriate re-exports

Issues & Recommendations ⚠️

1. Missing Error Handling in WebSocket Driver 🔴

Location: rivetkit-typescript/packages/rivetkit/src/actor/conn/drivers/websocket.ts:92-94

terminate: () => {
    (websocket as any).terminate();
},

Issue: The terminate() method uses type assertion to call a method that may not exist on WSContext.

Recommendation: Add proper type checking or error handling:

terminate: () => {
    if ('terminate' in websocket && typeof websocket.terminate === 'function') {
        (websocket as any).terminate();
    } else {
        actor.rLog.warn('WebSocket does not support terminate(), using close instead');
        websocket.close(1011, 'Connection terminated');
    }
},

2. Potential Memory Leak in Close Promises 🟡

Location: rivetkit-typescript/packages/rivetkit/src/actor/router-endpoints.ts:142

const closePromiseResolvers = promiseWithResolvers<void>();

Issue: If closePromiseResolvers.resolve() is never called (e.g., due to errors in WebSocket lifecycle), the promise will hang indefinitely. The disconnect method awaits this promise.

Recommendation: Add timeout handling:

disconnect: async (actor, conn, reason) => {
    websocket.close(1000, reason);
    
    // Wait for close with timeout
    const timeoutMs = 5000;
    await Promise.race([
        closePromise,
        new Promise((_, reject) => 
            setTimeout(() => reject(new Error('Close timeout')), timeoutMs)
        )
    ]).catch(err => {
        actor.rLog.warn({ msg: 'websocket close timeout', error: err });
    });
},

3. Inconsistent Type Annotations 🟡

Location: Multiple files in actor/instance/

Several manager constructors use any for config parameters:

// state-manager.ts:48
#config: any; // ActorConfig type

// connection-manager.ts:99
eventManager: any, // EventManager type

Recommendation: Use proper generic types or define specific config interfaces to maintain type safety throughout the refactoring.

4. Deleted File May Have Active Usage 🟡

Location: rivetkit-typescript/packages/rivetkit/src/actor/unstable-react.ts

This file was completely removed (110 lines). Ensure there are no external consumers of this unstable API before merging.

Recommendation: Verify with a grep for any imports:

grep -r "unstable-react" rivetkit-typescript/

5. Symbol Export Strategy 🟢

Location: actor/conn/mod.ts:25-26

export const CONN_PERSIST_SYMBOL = Symbol("persist");
export const CONN_DRIVER_SYMBOL = Symbol("driver");

Minor concern: Exporting symbols makes them accessible but doesn't truly enforce privacy since symbols are unique but discoverable.

Recommendation: Document in comments that these are internal-only and should not be used by consumers.

Security Considerations 🔒

1. Type Assertions on User Input

The WebSocket message handling uses type assertions that could be exploited if validation is insufficient upstream. Ensure all message deserialization is properly validated.

2. Closure-based State

The new driver pattern uses closures to hold state (encoding, websocket reference, etc.). This is secure as long as the factory functions are only called by trusted code.

Performance Considerations ⚡

Positive:

Manager delegation should have minimal overhead
Closure-based drivers avoid repeated state lookups

Neutral:

Additional indirection through managers (acceptable tradeoff for maintainability)

Testing Coverage 📋

Concern: Only one test file was modified (tests/actor-types.test.ts). Such a large refactoring should include:

Unit tests for each new manager class
Integration tests verifying manager interactions
Tests for edge cases in connection lifecycle
Tests for the new driver factory pattern

Recommendation: Add comprehensive test coverage before merging.

Migration Impact 🔄

Breaking Changes:

Method renames (_onAlarm → onAlarm, etc.) may affect internal consumers
Symbol changes may affect code using __persist and __socket
File reorganization requires import updates

Recommendation: Document migration path for any internal consumers.

Final Verdict

Overall Assessment: ✅ Approve with minor changes

This is a solid refactoring that significantly improves code organization and maintainability. The architectural changes are sound, and the code quality is high. The identified issues are minor and can be addressed before or shortly after merge.

Priority Fixes:

Fix WebSocket terminate() error handling
Add timeout to WebSocket close promise
Replace any types with proper generics
Verify no external consumers of deleted unstable-react.ts
Add test coverage for new manager classes

Nice to Have:

Document symbol export strategy
Add migration guide for breaking changes

Great work on improving the codebase architecture! 🎉

pkg-pr-new · 2025-11-09T05:08:49Z

More templates

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/actor@3421

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3421

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/core@3421

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3421

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3421

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3421

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3421

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3421

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3421

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3421

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3421

commit: 3449509

claude · 2025-11-13T01:33:42Z

Code Review: ActorInstance Refactoring

Summary

This is an excellent refactoring that improves code organization and maintainability by splitting the large ~2200 line ActorInstance class into focused, single-responsibility manager classes. The changes follow good software engineering principles and align well with the codebase conventions.

✅ Strengths

1. Architecture & Design

Separation of Concerns: Breaking down the monolithic ActorInstance into dedicated managers (ConnectionManager, StateManager, EventManager, ScheduleManager) significantly improves code organization
Single Responsibility: Each manager has a clear, focused purpose
Maintainability: Much easier to understand, test, and modify individual components
File Size: Reducing the main file from 2217 lines to ~1027 lines is a huge improvement for readability

2. Connection Driver Refactoring

The new driver pattern (moving from enum-based dispatch to factory functions) is cleaner and more flexible
createWebSocketSocket() and createHttpSocket() factory functions are more intuitive than the previous ConnDriverKind enum approach
The interface-based design makes it easier to add new driver types in the future

3. Symbol-Based Privacy

Good use of symbols (ACTOR_INSTANCE_PERSIST_SYMBOL, CONN_PERSIST_SYMBOL, CONN_DRIVER_SYMBOL) for internal properties that need to be accessible across modules but shouldn't be part of the public API
This is a better approach than prefixing with __ (double underscore)

4. Code Quality

Consistent error handling and logging throughout
Good documentation with clear comments and JSDoc
Proper TypeScript types maintained across the refactoring

⚠️ Areas for Improvement

1. Type Safety Issues

StateManager.ts:140-154 - Unreachable validation logic:

if (target === null || typeof target !== "object") {
    // This validates, but then returns early
    let invalidPath = "";
    if (!isCborSerializable(target, ...)) {
        throw new errors.InvalidStateType({ path: invalidPath });
    }
    return target;  // ❌ This line is unreachable
}

Issue: If target is not an object, the function returns target without creating a proxy, but that line appears after the validation. This logic should either throw an error or be restructured.

Recommendation:

if (target === null || typeof target !== "object") {
    throw new errors.InvalidStateType({ 
        path: "persist",
        message: "Persist object must be a non-null object" 
    });
}

2. Type Looseness

Multiple locations use any types where more specific types would be better:

EventManager.ts:62, 68 - Uses (this.#actor as any).connectionManager
ConnectionManager.ts:148, 288 - Uses (this.#actor as any).config
ScheduleManager.ts:20 - #config: any
StateManager.ts:48 - #config: any

Recommendation: Define proper types for these cross-manager dependencies:

interface ManagerDependencies {
    connectionManager: ConnectionManager<S, CP, CS, V, I, DB>;
    stateManager: StateManager<S, CP, CS, I>;
    eventManager: EventManager<S, CP, CS, V, I, DB>;
    scheduleManager: ScheduleManager<S, CP, CS, V, I, DB>;
}

3. Missing Error Handling

ScheduleManager.ts:299-309 - Error handling doesn't prevent processing:

} catch (error) {
    this.#actor.log.error({
        msg: "error executing scheduled event",
        error: stringifyError(error),
        // ...
    });
    // ✅ Good: Comment says "Continue processing other events"
}

While this is intentional (good!), consider adding metrics/counters for failed events for monitoring purposes.

4. HTTP Driver TODOs

drivers/http.ts:11-12, 16 - Several TODOs remain:

getConnectionReadyState(_actor, _conn) {
    // TODO: This might not be the correct logic
    return DriverReadyState.OPEN;
},
disconnect: async () => {
    // TODO: Configure with abort signals to abort the request
},

Recommendation: These should be addressed or tracked in issues, especially the abort signal support which could cause resource leaks.

5. Inconsistent Access Patterns

connection-manager.ts:206 - Direct access to internal property:

conn.persistRaw  // Accessing raw persist object

Yet in other places, it properly uses the symbol:

conn[CONN_PERSIST_SYMBOL]  // ✅ Correct symbol access

Recommendation: Ensure consistent use of symbols vs. public getters. Consider if persistRaw should be a public getter or if symbol access should be used everywhere.

6. Potential Race Condition

StateManager.ts:193-202 - Promise handling:

if (!this.#onPersistSavedPromise) {
    this.#onPersistSavedPromise = promiseWithResolvers();
}
// Save throttled
this.savePersistThrottled();
// Wait for save
await this.#onPersistSavedPromise.promise;

Issue: If savePersistThrottled() is called multiple times before the promise resolves, subsequent calls will wait on the same promise that may resolve before their changes are persisted.

Recommendation: Consider a queue-based approach or ensure each save request gets its own promise that resolves when its changes are persisted.

🔒 Security Considerations

✅ No major security issues identified

Proper input validation on state serialization
CBOR validation prevents injection attacks
WebSocket close codes are handled correctly (1000 for normal closure)

🚀 Performance Considerations

Positive:

Lazy initialization: Managers are created only when needed
Throttled saves: StateManager properly throttles KV writes
Efficient indexing: EventManager uses Map<string, Set<Conn>> for O(1) lookups
Single promise queue: Prevents write conflicts in state/alarm operations

Potential Concerns:

ConnectionManager.ts:199-212 - Iterates changed connections on every save. Consider batching if connection churn is high.
EventManager.ts:192-206 - Creates a new CachedSerializer for every broadcast. This is likely fine, but monitor if broadcasts become a bottleneck.

📝 Test Coverage

Concern: The PR doesn't show test updates. With such a major refactoring:

✅ Verify existing tests still pass
⚠️ Consider adding unit tests for each new manager class
⚠️ Test edge cases (e.g., reconnection logic, alarm firing during shutdown)

🎯 Recommendations Priority

High Priority:

Fix type safety issues - Add proper types instead of any where possible
Address HTTP driver TODOs - Implement abort signal support to prevent resource leaks
Document the new architecture - Update any architecture docs to reflect the new manager pattern

Medium Priority:

Add manager-level unit tests - Each manager should have focused tests
Review promise handling - Ensure no race conditions in StateManager.saveState
Consistent symbol usage - Standardize internal property access patterns

Low Priority:

Add monitoring metrics - Track failed scheduled events, connection errors
Performance profiling - Baseline the new structure vs. old for regression testing

📊 Overall Assessment

Score: 8.5/10 - Excellent refactoring with strong architectural improvements. The remaining issues are mostly polish and potential edge cases rather than fundamental problems.

Final Verdict: ✅ APPROVE with minor changes

This refactoring significantly improves code maintainability and follows solid engineering principles. The separation of concerns makes the codebase much easier to reason about and extend. Address the type safety issues and TODOs, and this will be production-ready.

Great work on tackling a complex refactoring! 🎉

Generated with Claude Code

vercel · 2025-11-13T02:47:21Z

Deployment failed with the following error:

Too many requests - try again in 5 minutes (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

claude · 2025-11-13T02:54:09Z

Code Review - PR #3421: Split ActorInstance Logic into Multiple Classes

Summary

This is an excellent refactoring PR that significantly improves the maintainability and organization of the RivetKit actor system. The 2,200+ line ActorInstance class has been thoughtfully decomposed into focused manager classes.

Strengths

1. Excellent Separation of Concerns ✅

ConnectionManager: Handles connection lifecycle, hibernation, and tracking
StateManager: Manages persistence, proxying, and state synchronization
EventManager: Handles subscriptions and event broadcasting
ScheduleManager: Manages scheduled events and alarms

2. Clean Symbol-Based Encapsulation ✅
Using symbols for internal state provides proper encapsulation while maintaining flexibility.

3. Improved Driver Architecture ✅
Factory functions (createWebSocketSocket, createHttpSocket) provide clear construction paths.

4. Better Type Safety ✅
Maintains strong typing throughout while improving clarity.

Issues & Recommendations

1. Missing Error Handling in Connection Cleanup ⚠️
Location: connection-manager.ts:253-262
The #cleanupDriverState method doesn't handle errors from driver.disconnect(). Add .catch() to handle potential errors.

2. Type Safety Issues with any in Manager Access ⚠️
Locations: event-manager.ts:61-70, connection-manager.ts:288
Several places access managers through (this.#actor as any). Add proper getters to ActorInstance for type-safe access.

3. Incomplete WebSocket Terminate Implementation ⚠️
Location: conn/drivers/websocket.ts:92-94
The terminate method uses (websocket as any).terminate(). Add proper type guard or existence check.

4. Missing JSDoc on Public Methods ℹ️
Public methods lack documentation (e.g., ConnectionManager.getConnForId, clearChangedConnections).

5. HTTP Driver TODOs Need Attention ℹ️
Location: conn/drivers/http.ts:11-16
TODO comments about connection state logic and abort signals. Either implement or document rationale.

Performance Considerations

✅ State Save Throttling Preserved
✅ Connection Lookup Efficiency (Map usage)
⚠️ Monitor subscription index memory usage with many connections

Security

✅ Symbol-based encapsulation prevents accidental external access
✅ No exposed secrets

Test Coverage

Recommend ensuring integration tests cover:

Connection hibernation and reconnection flows
State persistence during connection churn
Event subscription lifecycle
Schedule event execution timing

Summary

High-quality refactoring that improves maintainability without changing external behavior.

Recommended Actions Before Merge:

Address error handling in #cleanupDriverState
Remove as any casts by adding proper getters
Improve WebSocket terminate() safety
Resolve or document HTTP driver TODOs
Add JSDoc to public manager methods

Overall: Approved with minor fixes recommended

Great work on this refactoring!

graphite-app · 2025-11-13T20:47:11Z

Merge activity

Nov 13, 8:47 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Nov 13, 8:48 PM UTC: CI is running for this pull request on a draft pull request (#3463) due to your merge queue CI optimization settings.
Nov 13, 8:50 PM UTC: Merged by the Graphite merge queue via draft PR: #3463.

)

This was referenced Nov 9, 2025

chore(rivetkit): remove deprecated conn tokens & sse support #3419

Closed

chore(rivetkit): move conns to separate persisted kv keys #3420

Closed

vercel bot deployed to Preview – rivetkit-serverless November 9, 2025 05:08 View deployment

NathanFlurry changed the base branch from 11-08-chore_rivetkit_move_conns_to_separate_persisted_kv_keys to graphite-base/3421 November 9, 2025 05:44

NathanFlurry changed the base branch from graphite-base/3421 to 11-08-chore_rivetkit_move_conns_to_separate_persisted_kv_keys November 9, 2025 23:02

chore(rivetkit): split ActorInstance logic in to multiple classes

3449509

NathanFlurry force-pushed the 11-08-chore_rivetkit_move_conns_to_separate_persisted_kv_keys branch from d818375 to c57be74 Compare November 13, 2025 02:47

NathanFlurry force-pushed the 11-08-chore_rivetkit_split_actorinstance_logic_in_to_multiple_classes branch from faa6045 to 3449509 Compare November 13, 2025 02:47

This was referenced Nov 13, 2025

chore(rivetkit-typescript): remove dependency on node modules #3460

Merged

chore(rivetkit): switch dynamic node imports to use require #3461

Merged

chore(rivetkit): fix type checks #3462

Merged

This was referenced Nov 13, 2025

feat(gas): implement smart pulling for horizontally scaled workers #3417

Merged

fix(guard): handle websocket tasks during shutdown #3448

Merged

fix: improve sigterm handling for the entire runtime #3443

Merged

NathanFlurry mentioned this pull request Nov 13, 2025

feat(gateway): websocket hibernation #3400

Merged

This was referenced Nov 13, 2025

fix(pb): dont immediately reallocate actors on runner stopping #3413

Merged

fix(guard): dont send open msg for hibernating ws, hibernating ws keepalive #3449

Merged

NathanFlurry mentioned this pull request Nov 13, 2025

chore(gas): rename wake sub to bump sub #3408

Merged

MasterPtato approved these changes Nov 13, 2025

View reviewed changes

graphite-app bot pushed a commit that referenced this pull request Nov 13, 2025

chore(rivetkit): split ActorInstance logic in to multiple classes (#3421

b84ced3

)

graphite-app bot closed this Nov 13, 2025

graphite-app bot deleted the 11-08-chore_rivetkit_split_actorinstance_logic_in_to_multiple_classes branch November 13, 2025 20:50

This was referenced Nov 13, 2025

chore(rivetkit): implement new hibernating ws protocol #3464

Open

feat(cloudflare-workers): add createInlineClient #3466

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore(rivetkit): split ActorInstance logic in to multiple classes #3421

chore(rivetkit): split ActorInstance logic in to multiple classes #3421

Uh oh!

NathanFlurry commented Nov 9, 2025

Uh oh!

vercel bot commented Nov 9, 2025 •

edited

Loading

Uh oh!

NathanFlurry commented Nov 9, 2025 •

edited by jog1t

Loading

Uh oh!

claude bot commented Nov 9, 2025

Uh oh!

pkg-pr-new bot commented Nov 9, 2025 •

edited

Loading

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

vercel bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

graphite-app bot commented Nov 13, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chore(rivetkit): split ActorInstance logic in to multiple classes #3421

chore(rivetkit): split ActorInstance logic in to multiple classes #3421

Uh oh!

Conversation

NathanFlurry commented Nov 9, 2025

Uh oh!

vercel bot commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NathanFlurry commented Nov 9, 2025 • edited by jog1t Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

claude bot commented Nov 9, 2025

PR Review: Split ActorInstance logic into multiple classes

Summary

Positive Aspects ✅

1. Excellent Code Organization

2. API Improvements

3. Improved Driver Architecture

4. File Organization

Issues & Recommendations ⚠️

1. Missing Error Handling in WebSocket Driver 🔴

2. Potential Memory Leak in Close Promises 🟡

3. Inconsistent Type Annotations 🟡

4. Deleted File May Have Active Usage 🟡

5. Symbol Export Strategy 🟢

Security Considerations 🔒

1. Type Assertions on User Input

2. Closure-based State

Performance Considerations ⚡

Positive:

Neutral:

Testing Coverage 📋

Migration Impact 🔄

Final Verdict

Uh oh!

pkg-pr-new bot commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Nov 13, 2025

Code Review: ActorInstance Refactoring

Summary

✅ Strengths

1. Architecture & Design

2. Connection Driver Refactoring

3. Symbol-Based Privacy

4. Code Quality

⚠️ Areas for Improvement

1. Type Safety Issues

2. Type Looseness

3. Missing Error Handling

4. HTTP Driver TODOs

5. Inconsistent Access Patterns

6. Potential Race Condition

🔒 Security Considerations

🚀 Performance Considerations

Positive:

Potential Concerns:

📝 Test Coverage

🎯 Recommendations Priority

High Priority:

Medium Priority:

Low Priority:

📊 Overall Assessment

Final Verdict: ✅ APPROVE with minor changes

Uh oh!

vercel bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

Code Review - PR #3421: Split ActorInstance Logic into Multiple Classes

Summary

Strengths

Issues & Recommendations

Performance Considerations

Security

Test Coverage

Summary

Uh oh!

graphite-app bot commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

vercel bot commented Nov 9, 2025 •

edited

Loading

NathanFlurry commented Nov 9, 2025 •

edited by jog1t

Loading

pkg-pr-new bot commented Nov 9, 2025 •

edited

Loading

graphite-app bot commented Nov 13, 2025 •

edited

Loading