-
Notifications
You must be signed in to change notification settings - Fork 33
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
base: main
Are you sure you want to change the base?
Conversation
- 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>
How to use the Graphite Merge QueueAdd either label to this PR to merge it via 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. |
Claude finished @NathanFlurry's task —— View job PR Review CompleteI've thoroughly analyzed the warning system implementation for detecting race conditions when mixing actor handles and connections. Here's my comprehensive review: Todo List
Overall AssessmentThe 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
|
@rivetkit/actor
@rivetkit/core
@rivetkit/db
rivetkit
@rivetkit/redis
@rivetkit/framework-base
@rivetkit/react
@rivetkit/cloudflare-workers
commit: |
packages/core/src/client/client.ts
Outdated
@@ -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; |
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.
silenceWarnings should be called specifically related to this error, not a universal silence warnings. update this everywhere.
packages/core/src/client/client.ts
Outdated
return new ActorHandleRaw( | ||
this, | ||
this.#driver, | ||
params, | ||
this.#encodingKind, | ||
actorQuery, | ||
silenceWarnings, |
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.
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( |
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.
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>>(); |
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.
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.
@claude handle the review |
Claude encountered an error —— View job I'll implement the review feedback to address the concerns raised. Todo List
Now implementing the requested changes... |
Claude finished @NathanFlurry's task —— View job I've successfully implemented all the review feedback requested by @NathanFlurry. Todo List
Summary of Changes Made
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>
Adds warning system to detect and alert users when mixing actor handle and connection usage patterns that may cause race conditions.
Changes
Closes #1061
Generated with Claude Code