-
Notifications
You must be signed in to change notification settings - Fork 150
feat: actor messages & queues #3989
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
|
🚅 Deployed to the rivet-pr-3989 environment in rivet-frontend
|
How to use the Graphite Merge QueueAdd 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. |
Code Review - Actor Messages & Queues FeatureThis PR adds message queue functionality to RivetKit actors. Overall this is a solid implementation with good architectural decisions. Strengths
Critical IssuesRace Condition in Waiter Resolution (queue-manager.ts:300-325) The loop in maybeResolveWaiters could allow multiple waiters to claim the same messages. Each waiter calls drainMessages which loads all queue messages fresh from storage. Without tracking claimed messages within the resolution cycle, concurrent waiters matching the same queue names could receive duplicate messages. Recommendation: Track which messages have been claimed during the current resolution cycle. Memory Scaling (queue-manager.ts:246-279) Every queue operation loads ALL messages into memory via kvListPrefix. For actors with hundreds or thousands of queued messages, this creates unnecessary memory pressure and latency. Recommendation: Implement pagination or use the existing maxQueueSize as a hard limit on loading. Important IssuesSilent Decode Failures (queue-manager.ts:266-271) Failed message decoding only logs an error. These ghost messages occupy space in the queue but can never be delivered or removed. Over time this could exhaust the queue capacity. Recommendation: Track decode failures and implement dead-letter handling or automatic cleanup. Unpersisted Size Corrections (queue-manager.ts:274-277) When metadata size mismatches actual message count, the code corrects it in memory but doesn't persist the fix. This means every subsequent load will detect and re-correct the same discrepancy. Recommendation: Persist corrected metadata when detecting mismatches. Timeout Cleanup (queue-manager.ts:183-203) The timeout handler removes the waiter and resolves the promise but doesn't clean up the abort signal listeners. If the actor aborts after timeout, the onStop listener will attempt to reject an already-resolved promise. Recommendation: Remove event listeners in all exit paths (timeout, abort, success). Minor Issues
Security & PerformanceSecurity: Proper validation of CBOR serialization, size limits enforced, abort signals handled correctly ✅ Performance:
Test CoverageGood coverage of basic scenarios but missing:
RecommendationApprove with revisions - The race condition in maybeResolveWaiters should be fixed before merge as it could cause message loss or duplication in production. The memory scaling issue should also be addressed for actors with large queues. The implementation follows good TypeScript patterns and the overall architecture is sound. Focus on the concurrency edge cases and resource scaling concerns. |
20d84dd to
ed996b2
Compare
ed996b2 to
11d2f22
Compare
11d2f22 to
266ebbd
Compare
266ebbd to
2dd6f17
Compare
2dd6f17 to
92b8706
Compare
92b8706 to
893fa88
Compare
6474860 to
1e5f0e0
Compare
PR Review: Actor Messages & QueuesSummaryThis PR adds a comprehensive queue messaging system to RivetKit actors, allowing asynchronous message passing between actors and clients. The implementation includes queue management, persistence, configurable limits, and comprehensive test coverage. Code Quality & Best Practices✅ Strengths
|

No description provided.