Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

@vercel
Copy link

vercel bot commented Jan 10, 2026

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

Project Deployment Review Updated (UTC)
rivet-cloud Ready Ready Preview, Comment Jan 10, 2026 0:15am
rivetkit-serverless Error Error Jan 10, 2026 0:15am
2 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-inspector Ignored Ignored Preview Jan 10, 2026 0:15am
rivet-site Ignored Ignored Preview Jan 10, 2026 0:15am

@railway-app
Copy link

railway-app bot commented Jan 10, 2026

🚅 Deployed to the rivet-pr-3810 environment in rivet-frontend

Service Status Web Updated (UTC)
frontend-cloud ✅ Success (View Logs) Web Jan 10, 2026 at 12:16 am
frontend-inspector 🕗 Deploying (View Logs) Web Jan 10, 2026 at 12:16 am
website ❌ Build Failed (View Logs) Web Jan 10, 2026 at 12:14 am

@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-3810 January 10, 2026 00:13 Destroyed
Copy link
Contributor Author


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.

@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 10, 2026

Merge activity

  • Jan 10, 12:14 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Jan 10, 12:15 AM UTC: CI is running for this pull request on a draft pull request (#3811) due to your merge queue CI optimization settings.
  • Jan 10, 12:16 AM UTC: Merged by the Graphite merge queue via draft PR: #3811.

@claude
Copy link

claude bot commented Jan 10, 2026

Code Review - PR #3810: fix(rivetkit): save alarm timeout per actor

Summary

This PR fixes a critical bug where alarm timeouts were shared across all actors in the EngineActorDriver. The fix correctly moves the alarmTimeout storage from a class-level property to per-actor storage in the ActorHandler interface.

✅ Positive Feedback

Critical Bug Fix: This change correctly addresses a race condition where multiple actors would overwrite each other's alarm timeouts. The previous implementation stored #alarmTimeout at the class level, meaning:

  • Setting an alarm for Actor A would overwrite Actor B's alarm
  • Only the most recently set alarm would fire
  • This would cause unpredictable behavior in multi-actor scenarios

Clean Implementation: The fix properly:

  • Adds alarmTimeout to the ActorHandler interface (line 71)
  • Looks up the handler before modifying the timeout (lines 200-207)
  • Gracefully handles missing handlers with appropriate logging (lines 202-206)
  • Cleans up the timeout reference when it fires (line 219)

🔍 Code Quality Observations

Minor: Whitespace Changes (lines 155, 360)
The formatting changes to empty arrow functions (() => { }) appear unintentional. These are stylistic and don't affect functionality, but they add noise to the diff:

// Before: onDisconnected: (_code, _reason) => {},
// After:  onDisconnected: (_code, _reason) => { },

According to CLAUDE.md, formatting should be handled by pre-commit hooks, so these changes could be reverted.

🧪 Test Coverage Concerns

Missing Test Coverage: There are no automated tests covering the multi-actor alarm scenario. I found tests in packages/rivetkit/src/driver-test-suite/tests/actor-sleep.ts that test alarms, but they don't validate that multiple actors can have independent alarms simultaneously.

Suggested Test Case:

// Test: Multiple actors should maintain independent alarm timeouts
test('multiple actors with independent alarms', async () => {
  const actor1 = await createActor('test-actor-1');
  const actor2 = await createActor('test-actor-2');
  
  // Set alarms at different times
  await actor1.setAlarm(Date.now() + 1000);
  await actor2.setAlarm(Date.now() + 2000);
  
  // Verify both alarms fire independently
  // ...
});

🛡️ Security & Performance

No concerns identified:

  • Memory management: Timeouts are properly cleaned up via .abort() (line 211)
  • Resource leaks: Timeout references are cleared after firing (line 219)
  • Error handling: Missing handlers are logged appropriately (lines 202-206)

📊 Architectural Correctness

The fix aligns well with the existing architecture:

  • Follows the pattern of storing per-actor state in ActorHandler
  • Maintains consistency with how actor and actorStartPromise are stored
  • Properly integrates with the LongTimeoutHandle abstraction

✨ Recommendations

  1. Revert whitespace-only changes on lines 155 and 360 to reduce diff noise
  2. Add test coverage for multi-actor alarm scenarios in the driver test suite
  3. Consider adding a comment in ActorHandler interface explaining that alarmTimeout stores the per-actor alarm handle (optional, for future maintainers)

🎯 Verdict

Approved ✅ - This is a solid bug fix that correctly addresses a critical issue with alarm timeout management. The implementation is clean and follows existing patterns. The whitespace changes are minor nitpicks that don't affect functionality.

Great catch on this bug! The previous implementation would have caused serious issues in production with multiple actors.


Review generated by Claude Code

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 10, 2026

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@3810

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 2573a29

@graphite-app graphite-app bot closed this Jan 10, 2026
@graphite-app graphite-app bot deleted the 01-09-fix_rivetkit_save_alarm_timeout_per_actor branch January 10, 2026 00:16
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.

3 participants