From e37d486edc513911002af189bc19fbb8eda7dd17 Mon Sep 17 00:00:00 2001 From: Alex Robinson Date: Fri, 6 Feb 2026 18:20:16 +0000 Subject: [PATCH] Add compat flag that causes deleteAll to delete alarms as well as data 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. --- src/workerd/api/actor-state.c++ | 5 +- src/workerd/api/tests/BUILD.bazel | 6 + .../tests/delete-all-deletes-alarm-test.js | 70 +++++ .../delete-all-deletes-alarm-test.wd-test | 38 +++ src/workerd/io/actor-cache-test.c++ | 293 ++++++++++++++++++ src/workerd/io/actor-cache.c++ | 72 ++++- src/workerd/io/actor-cache.h | 16 +- src/workerd/io/actor-sqlite-test.c++ | 132 ++++++++ src/workerd/io/actor-sqlite.c++ | 39 ++- src/workerd/io/actor-sqlite.h | 3 +- src/workerd/io/compatibility-date.capnp | 10 + 11 files changed, 657 insertions(+), 27 deletions(-) create mode 100644 src/workerd/api/tests/delete-all-deletes-alarm-test.js create mode 100644 src/workerd/api/tests/delete-all-deletes-alarm-test.wd-test diff --git a/src/workerd/api/actor-state.c++ b/src/workerd/api/actor-state.c++ index a5c5d059dbf..9e26354485c 100644 --- a/src/workerd/api/actor-state.c++ +++ b/src/workerd/api/actor-state.c++ @@ -568,7 +568,10 @@ jsg::Promise DurableObjectStorage::deleteAll( auto traceContext = context.makeUserTraceSpan("durable_object_storage_deleteAll"_kjc); auto options = configureOptions(kj::mv(maybeOptions).orDefault(PutOptions{})); - auto deleteAll = cache->deleteAll(options, context.getCurrentTraceSpan()); + DeleteAllOptions deleteAllOptions{ + .deleteAlarm = FeatureFlags::get(js).getDeleteAllDeletesAlarm(), + }; + auto deleteAll = cache->deleteAll(options, context.getCurrentTraceSpan(), deleteAllOptions); context.addTask(updateStorageDeletes(context, currentActorMetrics(), kj::mv(deleteAll.count))); diff --git a/src/workerd/api/tests/BUILD.bazel b/src/workerd/api/tests/BUILD.bazel index 4422b8e2dd8..a9bc9ecd282 100644 --- a/src/workerd/api/tests/BUILD.bazel +++ b/src/workerd/api/tests/BUILD.bazel @@ -13,6 +13,12 @@ wd_test( data = ["actor-alarms-delete-test.js"], ) +wd_test( + src = "delete-all-deletes-alarm-test.wd-test", + args = ["--experimental"], + data = ["delete-all-deletes-alarm-test.js"], +) + wd_test( src = "actor-alarms-test.wd-test", args = ["--experimental"], diff --git a/src/workerd/api/tests/delete-all-deletes-alarm-test.js b/src/workerd/api/tests/delete-all-deletes-alarm-test.js new file mode 100644 index 00000000000..f300c07035a --- /dev/null +++ b/src/workerd/api/tests/delete-all-deletes-alarm-test.js @@ -0,0 +1,70 @@ +// Tests that the delete_all_deletes_alarm compat flag correctly controls whether +// deleteAll() also deletes alarms. This file is used by two test services: one +// with the flag enabled and one with it disabled. The EXPECT_ALARM_DELETED binding +// tells this code which behavior to expect. + +import * as assert from 'node:assert'; + +export class DurableObjectExample { + constructor(state) { + this.state = state; + } + + async fetch(request) { + const url = new URL(request.url); + const expectDeleted = url.searchParams.get('expectDeleted') === 'true'; + + // Set an alarm for 10 minutes from now. + const alarmTime = Date.now() + 10 * 60 * 1000; + await this.state.storage.setAlarm(alarmTime); + assert.equal(await this.state.storage.getAlarm(), alarmTime); + + // Also put some KV data so deleteAll has something to delete. + await this.state.storage.put('key', 'value'); + + // Call deleteAll(). + await this.state.storage.deleteAll(); + + // KV data should always be deleted. + assert.equal(await this.state.storage.get('key'), undefined); + + const alarmAfter = await this.state.storage.getAlarm(); + + if (expectDeleted) { + // With the compat flag enabled, the alarm should be deleted. + assert.equal( + alarmAfter, + null, + `Expected alarm to be null after deleteAll(), got ${alarmAfter}` + ); + } else { + // Without the compat flag, the alarm should be preserved. + assert.equal( + alarmAfter, + alarmTime, + `Expected alarm to be preserved after deleteAll(), got ${alarmAfter}` + ); + // Clean up the alarm so it doesn't fire during the test. + await this.state.storage.deleteAlarm(); + } + + return new Response('OK'); + } + + async alarm() { + // Should not be invoked during the test. + throw new Error('alarm handler unexpectedly invoked'); + } +} + +export const test = { + async test(ctrl, env, ctx) { + let id = env.ns.idFromName('A'); + let obj = env.ns.get(id); + let res = await obj.fetch( + `http://foo/test?expectDeleted=${env.EXPECT_ALARM_DELETED}` + ); + let text = await res.text(); + assert.equal(text, 'OK'); + }, +}; diff --git a/src/workerd/api/tests/delete-all-deletes-alarm-test.wd-test b/src/workerd/api/tests/delete-all-deletes-alarm-test.wd-test new file mode 100644 index 00000000000..c47191b6cb8 --- /dev/null +++ b/src/workerd/api/tests/delete-all-deletes-alarm-test.wd-test @@ -0,0 +1,38 @@ +using Workerd = import "/workerd/workerd.capnp"; + +const unitTests :Workerd.Config = ( + services = [ + ( name = "delete-all-deletes-alarm-enabled", + worker = ( + modules = [ + ( name = "worker", esModule = embed "delete-all-deletes-alarm-test.js" ) + ], + compatibilityFlags = ["experimental", "nodejs_compat", "delete_all_deletes_alarm"], + durableObjectNamespaces = [ + (className = "DurableObjectExample", uniqueKey = "210bd0cbd803ef7883a1ee9d86cce06e"), + ], + durableObjectStorage = (inMemory = void), + bindings = [ + (name = "ns", durableObjectNamespace = "DurableObjectExample"), + (name = "EXPECT_ALARM_DELETED", text = "true"), + ], + ) + ), + ( name = "delete-all-preserves-alarm-disabled", + worker = ( + modules = [ + ( name = "worker", esModule = embed "delete-all-deletes-alarm-test.js" ) + ], + compatibilityFlags = ["experimental", "nodejs_compat", "delete_all_preserves_alarm"], + durableObjectNamespaces = [ + (className = "DurableObjectExample", uniqueKey = "310bd0cbd803ef7883a1ee9d86cce06f"), + ], + durableObjectStorage = (inMemory = void), + bindings = [ + (name = "ns", durableObjectNamespace = "DurableObjectExample"), + (name = "EXPECT_ALARM_DELETED", text = "false"), + ], + ) + ), + ], +); diff --git a/src/workerd/io/actor-cache-test.c++ b/src/workerd/io/actor-cache-test.c++ index 893d458f1e3..42bbca41f2b 100644 --- a/src/workerd/io/actor-cache-test.c++ +++ b/src/workerd/io/actor-cache-test.c++ @@ -5123,6 +5123,299 @@ KJ_TEST("ActorCache alarm delete when flush fails") { } } +KJ_TEST("ActorCache deleteAll() with deleteAlarm deletes alarm after deleteAll succeeds") { + // Tests that deleteAll() with deleteAlarm=true deletes the alarm in the post-deleteAll + // flush, ensuring the alarm is only deleted after the deleteAll RPC succeeds. + ActorCacheTest test; + auto& ws = test.ws; + auto& mockStorage = test.mockStorage; + + auto oneMs = 1 * kj::MILLISECONDS + kj::UNIX_EPOCH; + + // First, set an alarm so we have something to delete. + test.setAlarm(oneMs); + + mockStorage->expectCall("setAlarm", ws) + .withParams(CAPNP(scheduledTimeMs = 1)) + .thenReturn(CAPNP()); + + { + auto time = expectCached(test.getAlarm()); + KJ_ASSERT(time == oneMs); + } + + // Call deleteAll() with deleteAlarm=true. + auto deleteAll = test.cache.deleteAll({}, nullptr, {.deleteAlarm = true}); + + // The deleteAll RPC goes through first. + mockStorage->expectCall("deleteAll", ws).thenReturn(CAPNP(numDeleted = 0)); + + KJ_ASSERT(deleteAll.count.wait(ws) == 0); + + // After deleteAll succeeds, the alarm deletion is flushed in the post-deleteAll flush. + mockStorage->expectCall("deleteAlarm", ws) + .withParams(CAPNP(timeToDeleteMs = 0)) + .thenReturn(CAPNP(deleted = true)); + test.gate.wait(nullptr).wait(test.ws); + + { + auto time = expectCached(test.getAlarm()); + KJ_ASSERT(time == kj::none); + } +} + +KJ_TEST("ActorCache deleteAll() without deleteAlarm preserves alarm") { + // Tests that deleteAll() without deleteAlarm (the default) does not delete the alarm. + ActorCacheTest test; + auto& ws = test.ws; + auto& mockStorage = test.mockStorage; + + auto oneMs = 1 * kj::MILLISECONDS + kj::UNIX_EPOCH; + + // First, set an alarm. + test.setAlarm(oneMs); + + mockStorage->expectCall("setAlarm", ws) + .withParams(CAPNP(scheduledTimeMs = 1)) + .thenReturn(CAPNP()); + + { + auto time = expectCached(test.getAlarm()); + KJ_ASSERT(time == oneMs); + } + + // Call deleteAll() without deleteAlarm (default). + auto deleteAll = test.cache.deleteAll({}, nullptr); + + // The deleteAll RPC goes through. + mockStorage->expectCall("deleteAll", ws).thenReturn(CAPNP(numDeleted = 0)); + + KJ_ASSERT(deleteAll.count.wait(ws) == 0); + + // Wait for the output gate to complete. + test.gate.wait(nullptr).wait(test.ws); + + // The alarm should still be set. + { + auto time = expectCached(test.getAlarm()); + KJ_ASSERT(time == oneMs); + } +} + +KJ_TEST("ActorCache deleteAll() with deleteAlarm does not overwrite subsequent setAlarm") { + // Tests that if setAlarm() is called after deleteAll({.deleteAlarm = true}) but before the + // deleteAll RPC completes, the new alarm is preserved rather than being clobbered by the + // post-deleteAll alarm deletion. + ActorCacheTest test; + auto& ws = test.ws; + auto& mockStorage = test.mockStorage; + + auto oneMs = 1 * kj::MILLISECONDS + kj::UNIX_EPOCH; + auto twoMs = 2 * kj::MILLISECONDS + kj::UNIX_EPOCH; + + // First, set an alarm so we have something to delete. + test.setAlarm(oneMs); + + mockStorage->expectCall("setAlarm", ws) + .withParams(CAPNP(scheduledTimeMs = 1)) + .thenReturn(CAPNP()); + + { + auto time = expectCached(test.getAlarm()); + KJ_ASSERT(time == oneMs); + } + + // Call deleteAll() with deleteAlarm=true. + auto deleteAll = test.cache.deleteAll({}, nullptr, {.deleteAlarm = true}); + + // Hold the deleteAll RPC in flight. + auto mockDeleteAll = mockStorage->expectCall("deleteAll", ws); + + // While the deleteAll RPC is in flight, set a new alarm. This simulates the user's JS code + // calling setAlarm() after deleteAll() but before the deleteAll RPC has completed. + test.setAlarm(twoMs); + + // Now complete the deleteAll RPC. + kj::mv(mockDeleteAll).thenReturn(CAPNP(numDeleted = 0)); + + KJ_ASSERT(deleteAll.count.wait(ws) == 0); + + // The post-deleteAll flush should send setAlarm for the new time, not deleteAlarm. + mockStorage->expectCall("setAlarm", ws) + .withParams(CAPNP(scheduledTimeMs = 2)) + .thenReturn(CAPNP()); + test.gate.wait(nullptr).wait(test.ws); + + // Verify the alarm is set to the new time. + { + auto time = expectCached(test.getAlarm()); + KJ_ASSERT(time == twoMs); + } +} + +KJ_TEST("ActorCache deleteAll() with deleteAlarm during alarm handler cancels deferred delete") { + // Tests that calling deleteAll() with deleteAlarm=true while an alarm handler is running + // overwrites the DeferredAlarmDelete state, so when the handler finishes and the + // DeferredAlarmDeleter fires, it does nothing (since currentAlarmTime is no longer + // DeferredAlarmDelete). The alarm deletion instead happens via the normal post-deleteAll + // flush path. + ActorCacheTest test; + auto& ws = test.ws; + auto& mockStorage = test.mockStorage; + + auto oneMs = 1 * kj::MILLISECONDS + kj::UNIX_EPOCH; + auto testCurrentTime = kj::UNIX_EPOCH; + + // Set an alarm so we have something to delete. + test.setAlarm(oneMs); + + mockStorage->expectCall("setAlarm", ws) + .withParams(CAPNP(scheduledTimeMs = 1)) + .thenReturn(CAPNP()); + + { + auto time = expectCached(test.getAlarm()); + KJ_ASSERT(time == oneMs); + } + + { + // Arm the alarm handler, putting us in DeferredAlarmDelete state. + auto armResult = test.cache.armAlarmHandler(oneMs, nullptr, testCurrentTime); + KJ_ASSERT(armResult.is()); + + // During the handler, getAlarm() should return none (deferred delete is active). + auto time = expectCached(test.getAlarm()); + KJ_ASSERT(time == kj::none); + + // Call deleteAll() with deleteAlarm=true while the handler is running. This overwrites + // currentAlarmTime from DeferredAlarmDelete to KnownAlarmTime{CLEAN, none}. + auto deleteAll = test.cache.deleteAll({}, nullptr, {.deleteAlarm = true}); + + // getAlarm() should still return none. + { + auto time = expectCached(test.getAlarm()); + KJ_ASSERT(time == kj::none); + } + + // Drop the RunAlarmHandler. Since currentAlarmTime is no longer DeferredAlarmDelete, + // the DeferredAlarmDeleter does nothing. + } + + // The deleteAll RPC goes through. + mockStorage->expectCall("deleteAll", ws).thenReturn(CAPNP(numDeleted = 0)); + + // After deleteAll succeeds, the alarm deletion is flushed in the post-deleteAll flush. + mockStorage->expectCall("deleteAlarm", ws) + .withParams(CAPNP(timeToDeleteMs = 0)) + .thenReturn(CAPNP(deleted = true)); + test.gate.wait(nullptr).wait(test.ws); + + { + auto time = expectCached(test.getAlarm()); + KJ_ASSERT(time == kj::none); + } +} + +KJ_TEST( + "ActorCache deleteAll() without deleteAlarm during alarm handler preserves deferred delete") { + // Tests that calling deleteAll() without deleteAlarm while an alarm handler is running + // leaves the DeferredAlarmDelete state intact. When the handler finishes, the deferred + // deletion fires normally and the alarm is deleted via the pre-deleteAll flush. + ActorCacheTest test; + auto& ws = test.ws; + auto& mockStorage = test.mockStorage; + + auto oneMs = 1 * kj::MILLISECONDS + kj::UNIX_EPOCH; + auto testCurrentTime = kj::UNIX_EPOCH; + + // Set an alarm so we have something to delete. + test.setAlarm(oneMs); + + mockStorage->expectCall("setAlarm", ws) + .withParams(CAPNP(scheduledTimeMs = 1)) + .thenReturn(CAPNP()); + + { + auto time = expectCached(test.getAlarm()); + KJ_ASSERT(time == oneMs); + } + + { + // Arm the alarm handler, putting us in DeferredAlarmDelete state. + auto armResult = test.cache.armAlarmHandler(oneMs, nullptr, testCurrentTime); + KJ_ASSERT(armResult.is()); + + // During the handler, getAlarm() should return none (deferred delete is active). + auto time = expectCached(test.getAlarm()); + KJ_ASSERT(time == kj::none); + + // Call deleteAll() without deleteAlarm while the handler is running. + // This does NOT touch currentAlarmTime, so DeferredAlarmDelete is preserved. + auto deleteAll = test.cache.deleteAll({}, nullptr); + + // getAlarm() still returns none because DeferredAlarmDelete is still active. + { + auto time = expectCached(test.getAlarm()); + KJ_ASSERT(time == kj::none); + } + + // Drop the RunAlarmHandler. DeferredAlarmDeleter fires, sets status to READY, + // and calls ensureFlushScheduled(). + } + + // The deferred alarm delete is flushed in the pre-deleteAll flush, using the original + // alarm time as the timeToDeleteMs. + mockStorage->expectCall("deleteAlarm", ws) + .withParams(CAPNP(timeToDeleteMs = 1)) + .thenReturn(CAPNP(deleted = true)); + + // Then the deleteAll RPC goes through. + mockStorage->expectCall("deleteAll", ws).thenReturn(CAPNP(numDeleted = 0)); + + test.gate.wait(nullptr).wait(test.ws); + + { + auto time = expectCached(test.getAlarm()); + KJ_ASSERT(time == kj::none); + } +} + +KJ_TEST("ActorCache deleteAll() failure with deleteAlarm does not delete alarm") { + // Tests that if the deleteAll RPC fails, the alarm deletion RPC is never sent. + ActorCacheTest test({.monitorOutputGate = false}); + auto& ws = test.ws; + auto& mockStorage = test.mockStorage; + + auto brokenPromise = test.gate.onBroken(); + + auto oneMs = 1 * kj::MILLISECONDS + kj::UNIX_EPOCH; + + // First, set an alarm so we have something to delete. + test.setAlarm(oneMs); + + mockStorage->expectCall("setAlarm", ws) + .withParams(CAPNP(scheduledTimeMs = 1)) + .thenReturn(CAPNP()); + + { + auto time = expectCached(test.getAlarm()); + KJ_ASSERT(time == oneMs); + } + + // Call deleteAll() with deleteAlarm=true. + auto deleteAll = test.cache.deleteAll({}, nullptr, {.deleteAlarm = true}); + + // The deleteAll RPC fails. + mockStorage->expectCall("deleteAll", ws) + .thenThrow(KJ_EXCEPTION(FAILED, "jsg.Error: deleteAll failed")); + + // The output gate should be broken due to the failure. + KJ_EXPECT_THROW_MESSAGE("deleteAll failed", brokenPromise.wait(ws)); + + // No deleteAlarm RPC should have been sent since the deleteAll failed. + mockStorage->expectNoActivity(ws); +} + KJ_TEST("ActorCache can wait for flush") { // This test confirms that `onNoPendingFlush()` will return a promise that resolves when any // scheduled or in-flight flush completes. diff --git a/src/workerd/io/actor-cache.c++ b/src/workerd/io/actor-cache.c++ index 190b60b3c5a..f63ec04f8e9 100644 --- a/src/workerd/io/actor-cache.c++ +++ b/src/workerd/io/actor-cache.c++ @@ -1996,7 +1996,8 @@ kj::Own ActorCache::startTransaction() { return kj::heap(*this); } -ActorCache::DeleteAllResults ActorCache::deleteAll(WriteOptions options, SpanParent traceSpan) { +ActorCache::DeleteAllResults ActorCache::deleteAll( + WriteOptions options, SpanParent traceSpan, DeleteAllOptions deleteAllOptions) { // Since deleteAll() cannot be performed as part of another transaction, in order to maintain // our ordering guarantees, we will have to complete all writes that occurred prior to the // deleteAll(), then submit the deleteAll(), then do any writes afterwards. Conveniently, though, @@ -2037,18 +2038,34 @@ ActorCache::DeleteAllResults ActorCache::deleteAll(WriteOptions options, SpanPar return entry; }); - if (requestedDeleteAll == kj::none) { - auto paf = kj::newPromiseAndFulfiller(); - result = kj::mv(paf.promise); - requestedDeleteAll = DeleteAllState{ - .deletedDirty = kj::mv(deletedDirty), .countFulfiller = kj::mv(paf.fulfiller)}; - ensureFlushScheduled(options, kj::mv(traceSpan)); - } else { + KJ_IF_SOME(existing, requestedDeleteAll) { // A previous deleteAll() was scheduled and hasn't been committed yet. This means that we // can actually coalesce the two, and there's no need to commit any writes that happened // between them. So we can throw away `deletedDirty`. // We also don't want to double-bill for a coalesced deleteAll, so we don't update - // result in this branch. + // result in this branch. We do ensure the alarm is deleted if requested, though. + existing.deleteAlarm = existing.deleteAlarm || deleteAllOptions.deleteAlarm; + } else { + // If no previous deleteAll() was scheduled, then schedule this one. + auto paf = kj::newPromiseAndFulfiller(); + result = kj::mv(paf.promise); + requestedDeleteAll = DeleteAllState{ + .deletedDirty = kj::mv(deletedDirty), + .countFulfiller = kj::mv(paf.fulfiller), + .deleteAlarm = deleteAllOptions.deleteAlarm, + }; + ensureFlushScheduled(options, kj::mv(traceSpan)); + } + + if (deleteAllOptions.deleteAlarm) { + // Update the in-memory alarm state immediately so that getAlarm() returns null right away. + // The actual alarm deletion from storage is deferred to flushImplDeleteAll(), which sets + // currentAlarmTime to DIRTY after the deleteAll RPC succeeds, causing the post-deleteAll + // flush to send the deleteAlarm RPC. + currentAlarmTime = KnownAlarmTime{ + .status = KnownAlarmTime::Status::CLEAN, + .time = kj::none, + }; } // This is called for consistency, but deleteAll() strictly reduces cache usage, so it's not @@ -2174,6 +2191,18 @@ void ActorCache::ensureFlushScheduled(const WriteOptions& options, SpanParent tr if (lru.options.neverFlush) { // Skip all flushes. Used for preview sessions where data is strictly kept in memory. + // Handle deleteAll state that would normally be processed during flushImplDeleteAll(). + KJ_IF_SOME(deleteAllState, requestedDeleteAll) { + deleteAllState.countFulfiller->fulfill(0); + if (deleteAllState.deleteAlarm) { + currentAlarmTime = KnownAlarmTime{ + .status = KnownAlarmTime::Status::DIRTY, + .time = kj::none, + }; + } + requestedDeleteAll = kj::none; + } + // Also, we need to handle scheduling or canceling any alarm changes locally. KJ_SWITCH_ONEOF(currentAlarmTime) { KJ_CASE_ONEOF(knownAlarmTime, ActorCache::KnownAlarmTime) { @@ -3014,7 +3043,9 @@ kj::Promise ActorCache::flushImplDeleteAll(uint retryCount) { .then( [this](capnp::Response results) -> kj::Promise { - KJ_ASSERT_NONNULL(requestedDeleteAll).countFulfiller->fulfill(results.getNumDeleted()); + auto& deleteAllState = KJ_ASSERT_NONNULL(requestedDeleteAll); + deleteAllState.countFulfiller->fulfill(results.getNumDeleted()); + bool shouldDeleteAlarm = deleteAllState.deleteAlarm; // Success! We can now null out `requestedDeleteAll`. Note that we don't have to worry about // `requestedDeleteAll` having changed since we flushed it earlier, because it can't change @@ -3024,6 +3055,27 @@ kj::Promise ActorCache::flushImplDeleteAll(uint retryCount) { // the two deleteAll()s had been coalesced into a single one. requestedDeleteAll = kj::none; + if (shouldDeleteAlarm) { + // The deleteAll() was requested with deleteAlarm=true. Now that the deleteAll RPC has + // succeeded, we dirty the alarm to kj::none so that it will be flushed in the post-deleteAll + // flushImpl() call below. This ordering ensures the alarm is only deleted after KV data has + // been successfully deleted. + // + // However, if currentAlarmTime is already DIRTY, that means a subsequent setAlarm() call + // was made after the deleteAll() was initiated, and that newer alarm value should take + // precedence over the deletion. + bool alreadyDirty = false; + KJ_IF_SOME(known, currentAlarmTime.tryGet()) { + alreadyDirty = known.status == KnownAlarmTime::Status::DIRTY; + } + if (!alreadyDirty) { + currentAlarmTime = ActorCache::KnownAlarmTime{ + .status = ActorCache::KnownAlarmTime::Status::DIRTY, + .time = kj::none, + }; + } + } + { auto lock = lru.cleanList.lockExclusive(); evictOrOomIfNeeded(lock); diff --git a/src/workerd/io/actor-cache.h b/src/workerd/io/actor-cache.h index 5aa4b4a4994..b58a0ff98f2 100644 --- a/src/workerd/io/actor-cache.h +++ b/src/workerd/io/actor-cache.h @@ -48,6 +48,13 @@ struct ActorCacheWriteOptions { bool noCache = false; }; +struct DeleteAllOptions { + // When true, deleteAll() will also delete any scheduled alarm. The alarm deletion is + // guaranteed to take effect only after the deleteAll() itself succeeds, so that we never + // end up in a state where the alarm is deleted but KV data remains. + bool deleteAlarm = false; +}; + // Common interface between ActorCache and ActorCache::Transaction. class ActorCacheOps { public: @@ -214,7 +221,8 @@ class ActorCacheInterface: public ActorCacheOps { // The returned count only includes keys that were actually deleted from storage, not keys in // cache -- we only use the returned deleteAll count for billing, and not counting deletes of // entries that are only in cache is no problem for billing, those deletes don't cost us anything. - virtual DeleteAllResults deleteAll(WriteOptions options, SpanParent traceSpan) = 0; + virtual DeleteAllResults deleteAll( + WriteOptions options, SpanParent traceSpan, DeleteAllOptions deleteAllOptions = {}) = 0; // Call each time the isolate lock is taken to evict stale entries. If this returns a promise, // then the caller must hold off on JavaScript execution until the promise resolves -- this @@ -361,7 +369,8 @@ class ActorCache final: public ActorCacheInterface { // See ActorCacheOps. kj::Own startTransaction() override; - DeleteAllResults deleteAll(WriteOptions options, SpanParent traceSpan) override; + DeleteAllResults deleteAll( + WriteOptions options, SpanParent traceSpan, DeleteAllOptions deleteAllOptions = {}) override; kj::Maybe> evictStale(kj::Date now) override; void shutdown(kj::Maybe maybeException) override; @@ -762,6 +771,9 @@ class ActorCache final: public ActorCacheInterface { // deleteAll(). kj::Vector> deletedDirty; kj::Own> countFulfiller; + + // If true, the alarm should also be deleted after the deleteAll() RPC succeeds. + bool deleteAlarm = false; }; kj::Maybe requestedDeleteAll; diff --git a/src/workerd/io/actor-sqlite-test.c++ b/src/workerd/io/actor-sqlite-test.c++ index 282d5f62d50..5ba5c353cf8 100644 --- a/src/workerd/io/actor-sqlite-test.c++ +++ b/src/workerd/io/actor-sqlite-test.c++ @@ -1341,6 +1341,138 @@ KJ_TEST("calling deleteAll() during an implicit transaction preserves alarm stat KJ_ASSERT(expectSync(test.getAlarm()) == oneMs); } +KJ_TEST("deleteAll with deleteAlarm option deletes alarm") { + // Tests that deleteAll() with deleteAlarm=true deletes the alarm along with KV data, + // instead of preserving the alarm as it does by default. + ActorSqliteTest test; + + // Initialize alarm state to 1ms. + test.setAlarm(oneMs); + test.pollAndExpectCalls({"scheduleRun(1ms)"})[0]->fulfill(); + test.pollAndExpectCalls({"commit"})[0]->fulfill(); + test.pollAndExpectCalls({}); + KJ_ASSERT(expectSync(test.getAlarm()) == oneMs); + + // Call deleteAll() with deleteAlarm=true. + ActorCache::DeleteAllResults results = test.actor.deleteAll({}, nullptr, {.deleteAlarm = true}); + + // The alarm should now be deleted. + KJ_ASSERT(expectSync(test.getAlarm()) == kj::none); + + // Commit should include scheduling the alarm cancellation. + test.pollAndExpectCalls({"commit"})[0]->fulfill(); + test.pollAndExpectCalls({"scheduleRun(none)"})[0]->fulfill(); + test.pollAndExpectCalls({}); + + KJ_ASSERT(results.count.wait(test.ws) == 0); + KJ_ASSERT(expectSync(test.getAlarm()) == kj::none); +} + +KJ_TEST("deleteAll without deleteAlarm option preserves alarm") { + // Tests that deleteAll() without deleteAlarm (the default) preserves the alarm, + // which is the existing behavior. + ActorSqliteTest test; + + // Initialize alarm state to 1ms. + test.setAlarm(oneMs); + test.pollAndExpectCalls({"scheduleRun(1ms)"})[0]->fulfill(); + test.pollAndExpectCalls({"commit"})[0]->fulfill(); + test.pollAndExpectCalls({}); + KJ_ASSERT(expectSync(test.getAlarm()) == oneMs); + + // Call deleteAll() without deleteAlarm (default behavior). + ActorCache::DeleteAllResults results = test.actor.deleteAll({}, nullptr); + + // The alarm should be preserved. + KJ_ASSERT(expectSync(test.getAlarm()) == oneMs); + + test.pollAndExpectCalls({"commit"})[0]->fulfill(); + test.pollAndExpectCalls({}); + + KJ_ASSERT(results.count.wait(test.ws) == 0); + KJ_ASSERT(expectSync(test.getAlarm()) == oneMs); +} + +KJ_TEST("deleteAll with deleteAlarm during alarm handler cancels deferred delete") { + // Tests that calling deleteAll() with deleteAlarm=true while an alarm handler is running + // correctly deletes the alarm and cancels the deferred alarm deletion (haveDeferredDelete). + // When the handler's DeferredAlarmDeleter is dropped, it should NOT write a null alarm row + // since deleteAll already handled the deletion. + ActorSqliteTest test; + + // Initialize alarm state to 1ms. + test.setAlarm(oneMs); + test.pollAndExpectCalls({"scheduleRun(1ms)"})[0]->fulfill(); + test.pollAndExpectCalls({"commit"})[0]->fulfill(); + test.pollAndExpectCalls({}); + KJ_ASSERT(expectSync(test.getAlarm()) == oneMs); + + { + auto armResult = test.actor.armAlarmHandler(oneMs, nullptr, testCurrentTime); + KJ_ASSERT(armResult.is()); + + // During the handler, getAlarm() should return none (deferred delete is active). + KJ_ASSERT(expectSync(test.getAlarm()) == kj::none); + + // Call deleteAll() with deleteAlarm=true while the handler is running. + auto results = test.actor.deleteAll({}, nullptr, {.deleteAlarm = true}); + + // getAlarm() should still return none. + KJ_ASSERT(expectSync(test.getAlarm()) == kj::none); + + // Drop the DeferredAlarmDeleter (simulating handler success). Since deleteAll already + // cleared haveDeferredDelete, this should NOT write to the metadata table. + } + + // The deleteAll commit should go through commitImpl(), which detects the alarm moved to none + // and schedules the cancellation. + test.pollAndExpectCalls({"commit"})[0]->fulfill(); + test.pollAndExpectCalls({"scheduleRun(none)"})[0]->fulfill(); + test.pollAndExpectCalls({}); + + KJ_ASSERT(expectSync(test.getAlarm()) == kj::none); +} + +KJ_TEST("deleteAll without deleteAlarm during alarm handler still has deferred delete") { + // Tests that calling deleteAll() without deleteAlarm while an alarm handler is running + // restores the alarm in metadata but leaves haveDeferredDelete active. When the handler + // finishes, the deferred deletion deletes the restored alarm. + ActorSqliteTest test; + + // Initialize alarm state to 1ms. + test.setAlarm(oneMs); + test.pollAndExpectCalls({"scheduleRun(1ms)"})[0]->fulfill(); + test.pollAndExpectCalls({"commit"})[0]->fulfill(); + test.pollAndExpectCalls({}); + KJ_ASSERT(expectSync(test.getAlarm()) == oneMs); + + { + auto armResult = test.actor.armAlarmHandler(oneMs, nullptr, testCurrentTime); + KJ_ASSERT(armResult.is()); + + // During the handler, getAlarm() should return none (deferred delete is active). + KJ_ASSERT(expectSync(test.getAlarm()) == kj::none); + + // Call deleteAll() without deleteAlarm while the handler is running. + // This restores the alarm in metadata, but haveDeferredDelete is still true. + test.actor.deleteAll({}, nullptr); + + // getAlarm() still returns none because haveDeferredDelete is still active. + KJ_ASSERT(expectSync(test.getAlarm()) == kj::none); + + // Drop the DeferredAlarmDeleter (simulating handler success). This triggers + // maybeDeleteDeferredAlarm() which deletes the restored alarm. + } + + // The deleteAll commit goes first, then the deferred alarm deletion triggers its own commit + // with alarm scheduling. + test.pollAndExpectCalls({"commit"})[0]->fulfill(); + test.pollAndExpectCalls({"scheduleRun(none)"})[0]->fulfill(); + test.pollAndExpectCalls({}); + + KJ_ASSERT(expectSync(test.getAlarm()) == kj::none); +} + KJ_TEST("rolling back transaction leaves alarm in expected state") { ActorSqliteTest test; diff --git a/src/workerd/io/actor-sqlite.c++ b/src/workerd/io/actor-sqlite.c++ index 0eb72b6a95b..e65c6a692c2 100644 --- a/src/workerd/io/actor-sqlite.c++ +++ b/src/workerd/io/actor-sqlite.c++ @@ -756,7 +756,7 @@ kj::Own ActorSqlite::startTransaction() { } ActorCacheInterface::DeleteAllResults ActorSqlite::deleteAll( - WriteOptions options, SpanParent traceSpan) { + WriteOptions options, SpanParent traceSpan, DeleteAllOptions deleteAllOptions) { requireNotBroken(); disableAllowUnconfirmed(options, "deleteAll is not supported"); @@ -764,8 +764,7 @@ ActorCacheInterface::DeleteAllResults ActorSqlite::deleteAll( currentCommitSpan = kj::mv(traceSpan); // kv.deleteAll() clears the database, so we need to save and possibly restore alarm state in - // the metadata table, to try to match the behavior of ActorCache, which preserves the set alarm - // when running deleteAll(). + // the metadata table to maintain behavior from before the deleteAllDeletesAlarm compat flag. auto localAlarmState = metadata.getAlarm(); // deleteAll() cannot be part of a transaction because it deletes the database altogether. So, @@ -810,7 +809,12 @@ ActorCacheInterface::DeleteAllResults ActorSqlite::deleteAll( // We don't want to commit the deletion without that transaction. return kj::READY_NOW; } else { - return commitCallback(); + // Use commitImpl() rather than commitCallback() so that alarm scheduling is handled. + // This is important when deleteAll() deletes an alarm: commitImpl() detects that + // metadata.getAlarm() moved to kj::none and notifies the scheduler via + // requestScheduledAlarm(kj::none, ...). + auto precommitAlarmState = startPrecommitAlarmScheduling(); + return commitImpl(kj::mv(precommitAlarmState), currentCommitSpan.addRef()); } }), currentCommitSpan.addRef())); @@ -819,19 +823,28 @@ ActorCacheInterface::DeleteAllResults ActorSqlite::deleteAll( uint count = kv.deleteAll(); - // TODO(correctness): Since workerd doesn't have a separate durability step, in the unlikely - // event of a failure here, between deleteAll() and setAlarm(), we could theoretically lose the - // current alarm state when running under workerd. Not sure if there's a practical way to avoid - // this. - // Reset alarm state, if necessary. If no alarm is set, OK to just leave metadata table // uninitialized. if (localAlarmState != kj::none) { - if (metadata.setAlarm(localAlarmState, options.allowUnconfirmed)) { + if (deleteAllOptions.deleteAlarm) { + // The caller wants the alarm deleted along with KV data. Since kv.deleteAll() already + // wiped the database (including the alarm metadata), metadata.getAlarm() will naturally + // return kj::none without creating any tables or rows. We just need to increment + // alarmVersion so the deleteAllCommitScheduled callback triggers alarm scheduling in + // commitImpl(), which will detect the alarm moved to kj::none and notify the scheduler. ++alarmVersion; - if (debugAlarmSync) { - KJ_LOG(WARNING, "NOSENTRY DEBUG_ALARM: deleteAll restored alarm", logDate(localAlarmState), - alarmVersion); + haveDeferredDelete = false; + } else { + // TODO(correctness): Since workerd doesn't have a separate durability step, in the unlikely + // event of a failure here, between deleteAll() and setAlarm(), we could theoretically lose the + // current alarm state when running under workerd. Not sure if there's a practical way to avoid + // this. + if (metadata.setAlarm(localAlarmState, options.allowUnconfirmed)) { + ++alarmVersion; + if (debugAlarmSync) { + KJ_LOG(WARNING, "NOSENTRY DEBUG_ALARM: deleteAll restored alarm", + logDate(localAlarmState), alarmVersion); + } } } } diff --git a/src/workerd/io/actor-sqlite.h b/src/workerd/io/actor-sqlite.h index c878b5c6d86..f2803e3d6b7 100644 --- a/src/workerd/io/actor-sqlite.h +++ b/src/workerd/io/actor-sqlite.h @@ -91,7 +91,8 @@ class ActorSqlite final: public ActorCacheInterface, private kj::TaskSet::ErrorH // See ActorCacheOps. kj::Own startTransaction() override; - DeleteAllResults deleteAll(WriteOptions options, SpanParent traceSpan) override; + DeleteAllResults deleteAll( + WriteOptions options, SpanParent traceSpan, DeleteAllOptions deleteAllOptions = {}) override; kj::Maybe> evictStale(kj::Date now) override; void shutdown(kj::Maybe maybeException) override; kj::OneOf armAlarmHandler(kj::Date scheduledTime, diff --git a/src/workerd/io/compatibility-date.capnp b/src/workerd/io/compatibility-date.capnp index dfa4f22014f..8ee81829069 100644 --- a/src/workerd/io/compatibility-date.capnp +++ b/src/workerd/io/compatibility-date.capnp @@ -1378,4 +1378,14 @@ struct CompatibilityFlags @0x8f8c1b68151b6cef { # When enabled, the UTF-16le TextDecoder will replace lone surrogates with U+FFFD # (the Unicode replacement character) as required by the spec. Previously, lone # surrogates were passed through unchanged, producing non-well-formed strings. + + deleteAllDeletesAlarm @160 :Bool + $compatEnableFlag("delete_all_deletes_alarm") + $compatDisableFlag("delete_all_preserves_alarm") + $compatEnableDate("2026-02-24"); + # When enabled, calling storage.deleteAll() on a Durable Object also deletes + # any scheduled alarm, in addition to deleting all stored key-value data. + # + # Previously, deleteAll() preserved the alarm state. This was surprising + # behavior since the intent of deleteAll() is to clear all state. }