Merge pull request #5589 from cloudflare/milan/STOR-4521-repro

Don't schedule a move-later alarm if an interleaving commit setAlarm()

Author: MellowYarker

src/workerd/io/actor-sqlite-test.c++

@@ -1045,7 +1045,9 @@ KJ_TEST("setting later alarm times does scheduling after db commit") {
   auto gateWait2Ms = test.gate.wait();
   KJ_ASSERT(!gateWait2Ms.poll(test.ws));
 
-  // Set alarm to 3ms.  Expect 3ms db commit to start.
+  // Set alarm to 3ms.  Expect 3ms db commit to start. The 2ms scheduleRun will never happen now
+  // that we've overwritten it while it was persisting to SQLite (before we send an update to the
+  // alarm manager).
   test.setAlarm(threeMs);
   auto commit3MsFulfiller = kj::mv(test.pollAndExpectCalls({"commit"})[0]);
   test.pollAndExpectCalls({});
@@ -1054,11 +1056,10 @@ KJ_TEST("setting later alarm times does scheduling after db commit") {
   auto gateWait3Ms = test.gate.wait();
   KJ_ASSERT(!gateWait3Ms.poll(test.ws));
 
-  // Fulfill 2ms db commit.  Expect 2ms alarm to be scheduled and 2ms gate to be unblocked.
+  // Expect 2ms gate to be unblocked once the commit finishes, but don't expect a scheduleRun(2ms).
   KJ_ASSERT(!gateWait2Ms.poll(test.ws));
   commit2MsFulfiller->fulfill();
   KJ_ASSERT(gateWait2Ms.poll(test.ws));
-  auto fulfiller2Ms = kj::mv(test.pollAndExpectCalls({"scheduleRun(2ms)"})[0]);
   test.pollAndExpectCalls({});
 
   // Fulfill 3ms db commit.  Expect 3ms alarm to be scheduled and 3ms gate to be unblocked.
@@ -1067,10 +1068,6 @@ KJ_TEST("setting later alarm times does scheduling after db commit") {
   commit3MsFulfiller->fulfill();
   KJ_ASSERT(gateWait3Ms.poll(test.ws));
 
-  // The 3ms scheduleRun waits for the 2ms scheduleRun to complete first, since we chain
-  // "move later" operations to ensure they execute in order at the alarm manager.
-  fulfiller2Ms->fulfill();
-
   auto fulfiller3Ms = kj::mv(test.pollAndExpectCalls({"scheduleRun(3ms)"})[0]);
   test.pollAndExpectCalls({});
 

src/workerd/io/actor-sqlite.c++

@@ -433,6 +433,12 @@ kj::Promise<void> ActorSqlite::commitImpl(ActorSqlite::PrecommitAlarmState preco
   // pending commit so that the next commitImpl() invocation starts its own set of precommit alarm
   // updates and db commit.
   auto alarmStateForCommit = metadata.getAlarm();
+
+  // Capture the alarm version before going async to detect concurrent alarm changes. If the
+  // alarmVersion changes while we are in-flight, we should skip attempting any move-later alarm
+  // update.
+  auto alarmVersionBeforeAsync = alarmVersion;
+
   auto commitCallbackPromise = commitCallback();
   pendingCommit = kj::none;
 
@@ -449,27 +455,36 @@ kj::Promise<void> ActorSqlite::commitImpl(ActorSqlite::PrecommitAlarmState preco
   // Notify any merged commitImpl() requests that the db persistence completed.
   fulfiller->fulfill();
 
-  // If the db state is now later than the in-flight scheduled alarms, issue a request to update
-  // it to match the db state.
-  if (willFireEarlier(alarmScheduledNoLaterThan, alarmStateForCommit)) {
-    if (debugAlarmSync) {
-      KJ_LOG(WARNING, "NOSENTRY DEBUG_ALARM: Moving alarm later", "sqlite_has",
-          logDate(alarmStateForCommit), "alarmScheduledNoLaterThan",
-          logDate(alarmScheduledNoLaterThan));
+  // If another commit modified the alarm while we were async, skip post-commit alarm sync.
+  //
+  // We do this for a few reasons:
+  //  1. The other commit will handle its own alarm sync
+  //  2. Post-commit syncs are inherently optional (the alarm will self-correct)
+  //  3. This coalesces redundant alarm updates for better performance
+  //  4. This avoids race conditions where a later commit moved the alarm earlier, requiring a
+  //     pre-commit alarm update, and this update may have already been made before we get here.
+  if (alarmVersion == alarmVersionBeforeAsync) {
+    // No intervening alarm changes, it is safe to schedule a move-later alarm update if needed.
+    if (willFireEarlier(alarmScheduledNoLaterThan, alarmStateForCommit)) {
+      if (debugAlarmSync) {
+        KJ_LOG(WARNING, "NOSENTRY DEBUG_ALARM: Moving alarm later", "sqlite_has",
+            logDate(alarmStateForCommit), "alarmScheduledNoLaterThan",
+            logDate(alarmScheduledNoLaterThan));
+      }
+      // We need to extend our alarmLaterChain now that we're adding a new "move-later" alarm task.
+      //
+      // Technically, we don't need serialize our "move-later" alarms since SQLite has the later
+      // time committed locally. We could just set the `alarmLaterChain` and pass a `kj::READY_NOW`
+      // to requestScheduledAlarm, and so if we have a partial failure we would just recover when
+      // the alarm runs early. That said, it doesn't hurt to serialize on the client-side.
+      alarmLaterChain = requestScheduledAlarm(alarmStateForCommit, alarmLaterChain.addBranch())
+                            .catch_([](kj::Exception&& e) {
+        // If an exception occurs when scheduling the alarm later, it's OK -- the alarm will
+        // eventually fire at the earlier time, and the rescheduling will be retried.
+        // We catch here to prevent the chain from breaking on errors.
+        LOG_WARNING_PERIODICALLY("NOSENTRY SQLite reschedule later alarm failed", e);
+      }).fork();
     }
-    // We need to extend our alarmLaterChain now that we're adding a new "move-later" alarm task.
-    //
-    // Technically, we don't need serialize our "move-later" alarms since SQLite has the later
-    // time committed locally. We could just set the `alarmLaterChain` and pass a `kj::READY_NOW`
-    // to requestScheduledAlarm, and so if we have a partial failure we would just recover when
-    // the alarm runs early. That said, it doesn't hurt to serialize on the client-side.
-    alarmLaterChain = requestScheduledAlarm(alarmStateForCommit, alarmLaterChain.addBranch())
-                          .catch_([](kj::Exception&& e) {
-      // If an exception occurs when scheduling the alarm later, it's OK -- the alarm will
-      // eventually fire at the earlier time, and the rescheduling will be retried.
-      // We catch here to prevent the chain from breaking on errors.
-      LOG_WARNING_PERIODICALLY("NOSENTRY SQLite reschedule later alarm failed", e);
-    }).fork();
   }
 }
 
@@ -642,6 +657,9 @@ kj::Maybe<kj::Promise<void>> ActorSqlite::setAlarm(
     kj::Maybe<kj::Date> newAlarmTime, WriteOptions options) {
   requireNotBroken();
 
+  // Increment version counter on any alarm modification
+  ++alarmVersion;
+
   // TODO(someday): When deleting alarm data in an otherwise empty database, clear the database to
   // free up resources?
 

src/workerd/io/actor-sqlite.h

@@ -239,6 +239,11 @@ class ActorSqlite final: public ActorCacheInterface, private kj::TaskSet::ErrorH
   // at the alarm manager. Each update waits for the previous one to complete.
   kj::ForkedPromise<void> alarmLaterChain = kj::Promise<void>(kj::READY_NOW).fork();
 
+  // Version counter that increments on every alarm change. Used to detect if another commit
+  // modified the alarm while we were async, allowing us to skip redundant post-commit alarm
+  // syncs. This provides automatic coalescing of rapid alarm changes.
+  uint64_t alarmVersion = 0;
+
   // Debug flag for tracing alarm synchronization issues for specific namespaces
   bool debugAlarmSync = false;