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
• 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 the actor scheduling system, allowing users to clear scheduled alarms without removing the underlying scheduled events. The implementation follows the existing patterns and properly integrates with all three actor drivers (engine, file-system, and cloudflare-workers).

---

Code Quality ✅

Strengths:

• Clean, consistent implementation across all driver types
• Proper use of async/await patterns throughout
• Good separation of concerns between clearAlarm and clearAllEvents
• Follows existing code conventions (structured logging, error handling)
• Uses the existing SinglePromiseQueue to ensure alarm operations are serialized

Style Compliance:

• ✅ Structured logging with tracing (schedule-manager.ts:396)
• ✅ Lowercase log messages ("cleared alarm")
• ✅ Proper async patterns without blocking operations

---

Potential Issues ⚠️

1. API Naming Inconsistency

In src/actor/schedule.ts, the public method is named clear():

async clear() {
    await this.#actor.clearAlarm();
}

Issue: The name clear() is ambiguous - it's unclear whether it clears:

• The alarm only (current behavior)
• All scheduled events (like clearAllEvents)
• Both

Recommendation: Consider renaming to clearAlarm() for clarity, or add JSDoc to clarify the behavior.

2. Missing Test Coverage ⚠️

The PR adds new functionality but no tests. The existing actor-schedule.ts test file has no tests for clearAlarm or clearAllEvents.

Recommendation: Add tests covering:

• Clearing an alarm stops the alarm from firing
• Scheduled events remain in the queue after clearAlarm()
• Events don't fire after alarm is cleared
• Setting a new alarm after clearing works correctly
• Both file-system and engine drivers properly clear alarms

Example test:

test("clearAlarm prevents scheduled events from firing", async (c) => {
    const { client } = await setupDriverTest(c, driverTestConfig);
    const scheduled = client.scheduled.getOrCreate();
    
    // Schedule a task
    await scheduled.scheduleTaskAfter(250);
    
    // Clear the alarm before it fires
    await scheduled.clearAlarm();
    
    // Wait past when it would have fired
    await waitFor(driverTestConfig, 500);
    
    // Verify it didn't run
    const scheduledCount = await scheduled.getScheduledCount();
    expect(scheduledCount).toBe(0);
});

3. State Inconsistency in FileSystemDriver

In file-system/global-state.ts:623-654:

• The timeout is cleared
• alarmTimestamp is set to undefined
• The alarm file is deleted from disk

But in schedule-manager.ts, clearAlarm() doesn't modify this.#persist.scheduledEvents. This creates a state where:

• Events are still in the queue
• But no alarm is set to trigger them

Issue: If the actor restarts, initializeAlarms() (schedule-manager.ts:137-143) will recreate the alarm from the persisted events, but there's a window where events won't fire.

Recommendation: Add documentation clarifying this is intentional behavior, or consider whether clearAlarm should also prevent alarm recreation on restart.

4. Error Handling in File Deletion

In file-system/global-state.ts:642-649:

} catch (err: any) {
    if (err?.code !== "ENOENT") {
        logger().error({
            msg: "failed to delete alarm file",
            actorId,
            error: stringifyError(err),
        });
    }
}

Issue: Errors are logged but not propagated. If the file deletion fails (e.g., permission issues), the function silently succeeds, leaving orphaned alarm files.

Recommendation: Consider whether to propagate errors for non-ENOENT failures, or document why silent failure is acceptable.

---

Performance Considerations ✅

• Uses existing SinglePromiseQueue to serialize alarm writes - good!
• File system operations use async properly
• No blocking operations or unnecessary waits
• Minimal overhead - just clearing existing state

---

Security Considerations ✅

No security concerns identified:

• No user input validation needed (internal API)
• File operations properly scoped to actor directories
• No risk of path traversal or injection

---

Documentation Needs 📝

1. Missing JSDoc on public API

   • Schedule.clear() in schedule.ts:18-20 needs documentation
   • Should clarify it only clears the alarm, not the events

2. Behavior Documentation

   • Document what happens to events after clearing alarm
   • Clarify alarm recreation on actor restart
   • Add examples to CLAUDE.md or RivetKit docs

---

Recommendations

High Priority:

1. ✅ Add test coverage - Critical for ensuring correctness across drivers
2. ✅ Clarify API naming - Avoid confusion between clear(), clearAlarm(), and clearAllEvents()

Medium Priority:
3. ⚠️ Add JSDoc to public methods
4. ⚠️ Document state behavior regarding alarm recreation on restart

Low Priority:
5. ℹ️ Consider whether file deletion errors should propagate

---

Overall Assessment

The implementation is solid and follows existing patterns well. The main concerns are:

• Missing test coverage (important for reliability)
• Ambiguous API naming in the public interface
• Unclear behavior around actor restart scenarios

With tests and clearer naming, this would be ready to merge. The core implementation is sound and properly handles concurrency, logging, and error cases.

[graphite-app]

Merge activity

• Feb 4, 8:37 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Feb 4, 8:38 PM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #4002.
• Feb 4, 8:38 PM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #4002.