Add compat flag that causes deleteAll to delete alarms as well as data

[a-robinson]

This has been requested on multiple occasions now and is a more reasonable default behavior than leaving alarms behind.

I was also able to structure things such that this leaves the object entirely empty after the deleteAll, which is a nice side effect.

---

This was largely done by Claude, although with a fair bit of back and forth and a handful of manual fixes. Here's the series of prompts used (excluding ones about minor things like running / fixing / addition extra tests), since that likely helps convey how things got here:

Currently when an actor / Durable Object calls deleteAll(), it deletes all of the stored data in the actor but does not delete the alarm if one is set/stored. We would like to change that, such that deleteAll deletes any scheduled alarm in addition to deleting all data. We want to do this for both storage engines available to Durable Objects. Can you please create an implementation plan for changing that behavior, gated behind a compatibility date?

The code you added allows the cache->setAlarm() call to overlap with the cache->deleteAll() call, but I'm worried that this is unsafe because it may be possible for the setAlarm() to complete and the deleteAll() to fail. Can you check through the code in ActorCache to confirm whether that's a possible problem?

Thanks! The new alarm deletion logic looks better. There's one thing I'm still curious about. For sqlite-backed actors, the alarm time is also stored in ActorStorage (which is backed by cockroachdb), which is responsible for ensuring the actor is woken up at the appropriate time. Two questions:
  1. Does the new code delete the alarm from ActorStorage?
  2. If not, what will happen when the alarm gets invoked at the time that's still stored in ActorStorage? Will the actor's constructor be run or will we notice that the alarm has been deleted before that?

Does calling setAlarm(kj::none, ...) cause the database to be non-empty after the deleteAll? It would really be preferable if the database state could be left entirely empty rather than writing a null alarm time row into it if that's what's now happening.

Can you please add test coverage for the case where an alarm handler is running (and thus there is a deferred delete) when deleteAll is called with the new compat flag set?

In the most recent workerd commit, a couple of tests were added relating to whether alarms are deleted when deleteAll is called in the durable object. Can you please add a test case to actor-cache-test that if the deleteAll request to storage fails that the alarm is not deleted?

I'm suspicious of the code in ActorCache::flushImplDeleteAll that sets the alarm time to null after the deleteAll has completed. What if the user's JS code called setAlarm() to some non-null time after deleteAll was called? It would be incorrect to null out the alarm in that case. Can you please a add a test that exercises this code path, where deleteAll is called with deleteAlarm=true but then setAlarm() is called before the deleteAlarm finishes?

Thanks! I notice that we're setting the currentAlarmTime to CLEAN in ActorCache::deleteAll, and that if we didn't then this fix wouldn't have been needed. Why do we need to set currentAlarmTime to CLEAN in ActorCache::deleteAll? Is that important for some other reason?
> yes this was needed to avoid deleting the alarm before calling flushImplDeleteAll

The latest commit appears to change the handling of requestedDeleteAll in ensureFlushScheduled when lru.options.neverFlush is true. Is that a meaningful change in behavior? Why do we need to add any requestedDeleteAll handling there beyond doing something with the deleteAlarm field?
> this was filling in missing functionality

// In a fresh instance for code review
Can you please review the current diff in this repository and the most recent two commits in the workerd submodule? The change is intended to provide a new compatibility flag that deletes any existing alarm when deleteAll is called on a Durable Object. It should ensure that the alarm is never deleted unless all the data has also been successfully deleted, and in the case of sqlite durable objects it should leave the database in an empty, uninitialized state.

@jclee @MellowYarker as the people who have touched the actor-sqlite.c++ alarm code the most, since I'm less familiar with that code. I'm quite familiar with the actor-cache code and feel good about how that looks after the multiple rounds of back and forth with claude. Just one of you reviewing should be sufficient though, I don't think it needs double review.

[codspeed-hq]

CodSpeed Performance Report

Merging this PR will not alter performance

_{Comparing arobinson/delete-all-deletes-alarm (e37d486) with main (3b4a776)}

Summary

✅ 70 untouched benchmarks
⏩ 129 skipped benchmarks¹

Footnotes

1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩