Skip to content

feat: add warning system for mixing actor handles and connections #1062

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

NathanFlurry
Copy link
Member

Adds warning system to detect and alert users when mixing actor handle and connection usage patterns that may cause race conditions.

Changes

  • Add global connection tracking to detect when handles have active connections
  • Log warning when calling stateless RPCs on handles with active connections
  • Add silenceWarnings option to all handle creation methods
  • Properly clean up connection tracking when connections are disposed

Closes #1061

Generated with Claude Code

- Add global connection tracking to detect when handles have active connections
- Log warning when calling stateless RPCs on handles with active connections  
- Add silenceWarnings option to all handle creation methods (get, getOrCreate, create, getForId)
- Properly clean up connection tracking when connections are disposed
- Prevent race conditions by alerting users to mixed usage patterns

Fixes #1061

Co-authored-by: Nathan Flurry <NathanFlurry@users.noreply.github.com>
Copy link

graphite-app bot commented Jul 3, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge queue - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

Copy link

claude bot commented Jul 3, 2025

Claude finished @NathanFlurry's task —— View job


PR Review Complete

I've thoroughly analyzed the warning system implementation for detecting race conditions when mixing actor handles and connections. Here's my comprehensive review:

Todo List

  • Read and analyze changed files
  • Review code quality and best practices
  • Check for potential bugs and race conditions
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage
  • Provide comprehensive feedback

Overall Assessment

The implementation successfully addresses the issue by adding global connection tracking and warning when stateless RPCs are called on handles with active connections. The approach is sound, but there are several concerns that should be addressed.

✅ Strengths

  1. Clean API Design: The silenceWarnings option provides good flexibility for users who understand the risks
  2. Structured Logging: Warning messages include helpful context (action name, active connections count, query)
  3. Proper Cleanup: Connections are correctly removed from tracking on dispose
  4. Good Documentation: JSDoc comments explain the purpose and usage

⚠️ Code Quality & Best Practices

Handle Identifier Generation (actor-handle.ts:24-29)

function generateHandleIdentifier(actorQuery: ActorQuery, params: unknown): string {
    return JSON.stringify({ query: actorQuery, params });
}

Issue: Using JSON.stringify for object hashing is unreliable due to property ordering and can be expensive.

Recommendation: Consider using a deterministic hashing library or implement a custom serialization that ensures consistent ordering.

Global State Management

The global ACTIVE_CONNECTIONS map is well-designed, but consider adding JSDoc documentation explaining its lifecycle and thread safety implications.

🐛 Potential Bugs & Issues

1. Memory Leak Risk (actor-handle.ts:19)

High Priority: If removeConnectionFromTracking isn't called properly, the global map could accumulate stale entries indefinitely.

Mitigation: Consider adding:

  • WeakMap usage where possible
  • Periodic cleanup mechanism
  • Debug logging for map size growth

2. Race Condition in Tracking (actor-handle.ts:149-153)

if (!ACTIVE_CONNECTIONS.has(this.#handleIdentifier)) {
    ACTIVE_CONNECTIONS.set(this.#handleIdentifier, new Set());
}
ACTIVE_CONNECTIONS.get(this.#handleIdentifier)!.add(conn);

Issue: Between the has check and set operation, another thread could modify the map.

Fix: Use a single atomic operation:

let connections = ACTIVE_CONNECTIONS.get(this.#handleIdentifier);
if (!connections) {
    connections = new Set();
    ACTIVE_CONNECTIONS.set(this.#handleIdentifier, connections);
}
connections.add(conn);

3. Missing Error Handling (actor-handle.ts:35-48)

removeConnectionFromTracking doesn't handle cases where the connection was never tracked or already removed.

Recommendation: Add defensive checks and logging for debugging.

🚀 Performance Considerations

1. Action Call Overhead (actor-handle.ts:104-117)

Every stateless action call now performs:

  • Map lookup
  • Set size check
  • Conditional logging

Impact: Low but measurable overhead on high-frequency action calls.

Optimization: Consider lazy evaluation or caching strategies for repeated checks.

2. Identifier Generation Cost

JSON.stringify is called on every connection establishment and removal.

Suggestion: Cache identifiers in ActorHandleRaw instances.

🔒 Security Concerns

Information Disclosure (actor-handle.ts:108-116)

Warning logs include query details and connection counts, potentially exposing internal application state.

Recommendation:

  • Sanitize logged query information
  • Consider log level controls
  • Avoid exposing sensitive actor parameters

🧪 Test Coverage

Critical Gap: No tests are visible for the new warning functionality.

Required Tests:

  • Warning triggered when calling handle action with active connection
  • Warning suppressed with silenceWarnings: true
  • Proper cleanup when connections are disposed
  • Edge cases like concurrent connection establishment/disposal
  • Memory leak prevention

📝 Minor Issues

Type Safety (actor-handle.ts:73)

client: any,

Should use proper typing instead of any.

Consistent Naming

Consider renaming ACTIVE_CONNECTIONS to HANDLE_CONNECTION_TRACKING for clarity.

🎯 Recommendations

  1. High Priority: Add comprehensive tests for the warning system
  2. High Priority: Fix the race condition in connection tracking
  3. Medium Priority: Replace JSON.stringify with deterministic hashing
  4. Medium Priority: Add error handling in cleanup functions
  5. Low Priority: Optimize performance for high-frequency scenarios

The implementation effectively solves the stated problem, but addressing these concerns will improve reliability and maintainability.


Copy link

pkg-pr-new bot commented Jul 3, 2025

Open in StackBlitz

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-gg/rivetkit/@rivetkit/actor@1062

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-gg/rivetkit/@rivetkit/core@1062

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-gg/rivetkit/@rivetkit/db@1062

rivetkit

pnpm add https://pkg.pr.new/rivet-gg/rivetkit@1062

@rivetkit/redis

pnpm add https://pkg.pr.new/rivet-gg/rivetkit/@rivetkit/redis@1062

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-gg/rivetkit/@rivetkit/framework-base@1062

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-gg/rivetkit/@rivetkit/react@1062

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-gg/rivetkit/@rivetkit/cloudflare-workers@1062

commit: 483ed9b

@@ -133,6 +141,8 @@ export interface CreateOptions extends QueryOptions {
region?: string;
/** Input data to pass to the actor. */
input?: unknown;
/** Silence warnings when using handles with active connections. */
silenceWarnings?: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

silenceWarnings should be called specifically related to this error, not a universal silence warnings. update this everywhere.

return new ActorHandleRaw(
this,
this.#driver,
params,
this.#encodingKind,
actorQuery,
silenceWarnings,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update actorhandleraw to have an opts object instead of passing a million args

/**
* Generates a unique identifier for an actor handle based on its query and parameters.
*/
function generateHandleIdentifier(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't correct, it should accept actor name and the key. then serialize as an array [actorname, actorkey]

* Global registry of actor handles that have established connections.
* Maps from a unique handle identifier to whether connections exist.
*/
const ACTIVE_CONNECTIONS = new Map<string, Set<ActorConnRaw>>();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of having a global variable, store a set of connections for each individual handle. specifically, this should only warn if an actor handle is used to call an action if there is already a connection open for that specific handle.

@NathanFlurry
Copy link
Member Author

@claude handle the review

Copy link

claude bot commented Jul 3, 2025

Claude encountered an error —— View job


I'll implement the review feedback to address the concerns raised.

Todo List

  • Read current implementation files
  • Change silenceWarnings to be specific to this error type
  • Update ActorHandleRaw to use an opts object instead of many args
  • Fix handle identifier generation to use actor name and key as array
  • Replace global variable with per-handle connection sets
  • Run type check to ensure changes work correctly
  • Commit changes to PR branch

Now implementing the requested changes...


Copy link

claude bot commented Jul 3, 2025

Claude finished @NathanFlurry's task —— View job


I've successfully implemented all the review feedback requested by @NathanFlurry.

Todo List

  • Read current implementation files
  • Change silenceWarnings to be specific to this error type
  • Update ActorHandleRaw to use an opts object instead of many args
  • Fix handle identifier generation to use actor name and key as array
  • Replace global variable with per-handle connection sets
  • Run type check to ensure changes work correctly
  • Commit changes to PR branch

Summary of Changes Made

  1. Changed silenceWarnings to silenceConnectionWarnings - Updated all interfaces and method calls to use the more specific name that clearly indicates this setting only affects connection-related warnings.

  2. Updated ActorHandleRaw constructor to use options object - Replaced the constructor that took many individual parameters with a clean options object pattern using ActorHandleRawOptions interface.

  3. Fixed handle identifier generation - Updated generateHandleIdentifier() to use [actorName, actorKey] array format instead of JSON stringifying the entire query and params object. This provides a cleaner, more predictable identifier.

  4. Replaced global connection tracking with per-handle sets - Removed the global ACTIVE_CONNECTIONS map and instead gave each ActorHandleRaw instance its own #activeConnections Set. This ensures warnings only trigger when there are active connections for that specific handle instance.

  5. Improved connection cleanup - Added a callback mechanism where the handle passes a cleanup function to each connection, ensuring proper removal from tracking when connections are disposed.

The changes maintain the same functionality while addressing all the architectural concerns raised in the review. The warning system now operates at the handle level rather than globally, uses more descriptive option naming, and has a cleaner API design.


- Change silenceWarnings to silenceConnectionWarnings for specificity
- Update ActorHandleRaw to use options object instead of many arguments  
- Fix handle identifier generation to use actor name and key as array
- Replace global connection tracking with per-handle connection sets
- Improve connection cleanup through callback mechanism

Co-authored-by: Nathan Flurry <NathanFlurry@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: log warning if mixing using handler & connection
1 participant