feat(rivetkit): add clearAlarm

[NathanFlurry]

No description provided.

[NathanFlurry]

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

• feat(rivetkit): add clearAlarm #4004 👈 (View in Graphite)
• feat(rivetkit): c.keepAwake #4003
• feat(rivetkit): add run #4002
• feat: actor messages & queues #3989 : 1 other dependent PR (#4005 )
• main

---

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]

PR Review: feat(rivetkit): add clearAlarm

Summary

This PR adds a clearAlarm() method to RivetKit's scheduling system, allowing actors to cancel scheduled alarms without removing the underlying scheduled events. The implementation is clean and follows existing patterns well.

Code Quality ✅

Strengths

1. Consistent API Design: The implementation follows the established pattern used by setAlarm(), using the same queue-based approach and driver abstraction
2. Clean Separation of Concerns: Clear distinction between clearAlarm() (clears alarm only) and clearAllEvents() (clears events + alarm)
3. Good Documentation: JSDoc comments clearly explain the difference between clearing alarms vs clearing all events
4. Proper Error Handling: File system driver gracefully handles ENOENT errors when alarm file doesn't exist

Implementation Details

• All three layers properly implemented:
  • ActorInstance.clearAlarm() - public API at rivetkit-typescript/packages/rivetkit/src/actor/instance/mod.ts:690
  • ScheduleManager.clearAlarm() - internal queue management at rivetkit-typescript/packages/rivetkit/src/actor/instance/schedule-manager.ts:362
  • ActorDriver.clearAlarm() - driver abstraction at rivetkit-typescript/packages/rivetkit/src/actor/driver.ts:47
• Both drivers (engine & file-system) correctly implemented
• Uses existing #alarmWriteQueue to prevent race conditions
• Engine driver properly clears timeout and calls runner.clearAlarm() at rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts:235
• File system driver atomically removes alarm file with proper error handling at rivetkit-typescript/packages/rivetkit/src/drivers/file-system/global-state.ts:623

Issues & Concerns

🔴 Critical: Missing Test Coverage

The PR adds significant new functionality but includes no tests. This is concerning because:

1. No driver test suite coverage: The file rivetkit-typescript/packages/rivetkit/src/driver-test-suite/tests/actor-schedule.ts has no tests for clearAlarm()
2. Edge cases not validated:
   • What happens if you call clearAlarm() when no alarm is set?
   • Does clearing an alarm prevent scheduled events from firing?
   • What happens if you clear an alarm then schedule a new event - does it set a new alarm?
   • Concurrent operations: setAlarm() then immediate clearAlarm()

Recommendation: Add test cases to actor-schedule.ts covering:

test("clearAlarm cancels scheduled alarm", async (c) => {
  const scheduled = client.scheduled.getOrCreate();
  await scheduled.scheduleTaskAfter(250);

  // Clear the alarm before it fires
  await scheduled.clearAlarm();
  await waitFor(driverTestConfig, 500);

  // Verify task did NOT run
  const scheduledCount = await scheduled.getScheduledCount();
  expect(scheduledCount).toBe(0);
});

test("clearAlarm does not remove scheduled events", async (c) => {
  // Verify events remain in queue after clearing alarm
});

⚠️ Semantic Concern: clearAllEvents() Signature Change

// Before (rivetkit-typescript/packages/rivetkit/src/actor/instance/schedule-manager.ts:350)
clearAllEvents(): void {

// After  
async clearAllEvents(): Promise<void> {

This is a breaking change - any code calling clearAllEvents() without awaiting it will now silently fail to clear the alarm. The function changed from synchronous to asynchronous.

Impact:

• No compilation errors (TypeScript allows ignoring Promises)
• Runtime behavior changes - alarm won't be cleared if not awaited
• Could lead to subtle bugs in existing code

Recommendations:

1. Search codebase for all clearAllEvents() calls and ensure they're awaited (good news: I found no existing calls in this repo)
2. Consider adding a lint rule to catch unawaited promises
3. Document this breaking change in commit message or changelog

🟡 Minor: Inconsistent Logging Levels

// schedule-manager.ts:354
this.#actor.log.warn({ msg: "cleared all scheduled events" });

// schedule-manager.ts:364  
this.#actor.log.debug({ msg: "cleared alarm" });

clearAllEvents() uses warn (seems reasonable - destructive operation), but clearAlarm() uses debug. Should clearAlarm() also use warn or info since it's a user-initiated action that affects system behavior? Or is debug intentional because it's less destructive?

Recommendation: Consider using info level for clearAlarm() for consistency with user-initiated actions, or add a comment explaining why debug is appropriate.

🟡 Documentation: Schedule.clear() Method

The public API Schedule.clear() has no JSDoc:

// actor/schedule.ts:18
async clear() {
  await this.#actor.clearAlarm();
}

Recommendation: Add documentation explaining:

• What this method does
• How it differs from clearing individual scheduled events
• When you might want to use it

Example:

/**
 * Clears the scheduled alarm without removing queued events.
 * Events will not fire until a new event is scheduled or alarm is set.
 * Use this to temporarily pause event execution.
 */
async clear() {
  await this.#actor.clearAlarm();
}

Performance Considerations ✅

No performance concerns:

• Uses existing queue pattern to prevent race conditions
• File I/O is async and properly awaited
• Minimal overhead - just clearing timeouts and deleting a file

Security Considerations ✅

No security concerns:

• No new attack surface
• File operations use existing safe patterns with atomic writes
• Error messages don't leak sensitive information

Architecture Fit ✅

The implementation fits well with existing architecture:

• Follows established driver pattern
• Maintains separation between alarm (when to wake) and events (what to execute)
• Queue-based approach prevents race conditions

Recommendations Summary

1. HIGH PRIORITY: Add comprehensive test coverage in driver test suite
2. MEDIUM: Document the breaking change to clearAllEvents() signature
3. LOW: Consider logging level for clearAlarm() (debug vs info)
4. LOW: Add JSDoc to Schedule.clear() method

Verdict

The code quality is good and follows established patterns well. However, the lack of test coverage is a significant concern that should be addressed before merging. The signature change to clearAllEvents() also needs attention to prevent subtle bugs in the future.

Recommendation: Request changes - add tests and document the breaking change to clearAllEvents().

---

Review updated: 2026-01-30