I think this cleanup promise is unnecessary. It seems to exist purely to set alarmLaterChain back to null when the chain completes. You can instead leave alarmLaterChain as a promise that has already resolved -- this just means we'll harmlessly wait on an already-resolved promise at the start of the next alarm update. In fact, alarmLaterChain need not be a Maybe, it can instead be initialized to kj::Promise<void>(kj::READY_NOW).

I think this would simplify the code quite a bit.

I started off by doing kj::ForkedPromise<void> alarmLaterChain = kj::Promise<void>(kj::READY_NOW).fork();, but then ran into an issue because we wait for any pending move later alarms before we call requestScheduledAlarm() in startPrecommitAlarmScheduling(), when. That breaks sync scheduling in Workerd.

It looks like:

state.schedulingPromise =
      alarmLaterChain.addBranch().then([this, alarmTime]() { return requestScheduledAlarm(alarmTime); });

so the request to the alarm manager happens on the next event loop turn. This caused the synchronous alarm scheduling tests in actor-sqlite-test to fail.

So after I saw that I figured it would be easier to force it to be sync by clearing the chain. But now I'm wondering if that means:

1. Move earlier
2. Move later
3. Move earlier

would break Workerd because (3) is forced to be async waiting on (2)...

I think what we need here is to drop the cleanup approach, and instead add a method to ActorSqliteHooks which returns a bool indicating whether we're sync (workerd) or async (production).

If we're sync, we just won't ever care about the alarmLaterChain, because everything is sync anyways. If we're async, we care about waiting for the chain + extending the chain.

Does that make sense?

Here is my attempt at fixing that: https://github.com/cloudflare/workerd/compare/5169e0066e0944c2c1194e6b6c368c0022db760c..436a5a6a24847e6f0fed36837180ae34cac922d3

I tried switching to using coroutines instead of relying on the isSynchronous() method, with the thinking being if coroutines are eagerly evaluated in KJ (we set auto initial_suspend() { return stdcoro::suspend_never(); }) then an already resolved promise shouldn't cause us to suspend, whereas .then() would still force us to fire later. However, it doesn't seem to work as I'd have expected (seems like we're still suspending) in tests.

Claude thinks ForkedPromise::addBranch() ensures that the task continues on a future turn of the event loop, which would mean we can't execute synchronously even if the promise is already resolved.

void PromiseNode::OnReadyEvent::init(Event* newEvent) {
  if (event == _kJ_ALREADY_READY) {
    // A new continuation was added to a promise that was already ready.  In this case, we schedule
    // breadth-first, to make it difficult for applications to accidentally starve the event loop
    // by repeatedly waiting on immediate promises.
    if (newEvent) newEvent->armBreadthFirst();
  } else {
    event = newEvent;
  }
}

I'm not very familiar with the inner workings of async KJ, but am curious if that lines up with what you would expect @kentonv.

Optimization: If we know that further alarm updates have been added to the chain after this one, we might as well skip this update.