cloudflare / cloudflare/workerd
ActorSqlite: a parked move-later alarm is applied after a commit moved the stored alarm earlier, leaving the scheduled alarm later than the stored one
- Dominant language
- C++
- Stars
- 8.7k
- Forks
- 739
- Avg merge
- 2d 20h
- Merged PRs (30d)
- 174
Description
## Summary
With SQLite-backed Durable Objects, `ActorSqlite` can leave the AlarmManager's scheduled alarm **later** than the alarm stored in SQLite, with no later commit to correct it. The trigger is a move-later that was parked in `pendingLaterAlarmTime` while an earlier move-later was in flight. When the in-flight request resolves, the parked time is applied even though a commit in between moved the stored alarm earlier. After that commit, `commitImpl` re-checks only the later direction. The alarm then doesn't fire at the stored time. It fires only at the stale parked time, or when a later commit happens to re-sync.
## Mechanism (from `src/workerd/io/actor-sqlite.c++` on `main`)
- **Move-earlier happens before the commit.** `startPrecommitAlarmScheduling()` and the `while (willFireEarlier(metadata.getAlarm(), alarmScheduledNoLaterThan))` loop in `commitImpl()` run it.
- **Move-later happens after `co_await commitCallbackPromise`**, via `scheduleLaterAlarm()`:
- If `alarmLaterIsInFlight`, the new time is stored in `pendingLaterAlarmTime` and the call returns.
- The continuation of the in-flight request later calls `scheduleLaterAlarm(nextTime)` with that parked value, without comparing it to the current `metadata.getAlarm()`.
- **`requestScheduledAlarm()` raises `alarmScheduledNoLaterThan` immediately when moving later.**
- **The post-commit check** in `commitImpl()` is `if (willFireEarlier(alarmScheduledNoLaterThan, alarmStateForCommit)) scheduleLaterAlarm(...)`. That covers the later direction only.
## Sequence that strands the alarm
All three writes are in one object, within roughly one AlarmManager round trip. Times are relative to the alarm firing at t=0.
1. **The alarm handler runs** and sets the alarm to t+30s, a move-later. That request is **in flight**, and `alarmScheduledNoLaterThan` is now t+30s.
2. **The next event's commit sets the alarm to t+24h**, also later. The first request is still in flight, so t+24h is **parked** in `pendingLaterAlarmTime`.
3. **The next event's commit sets the alarm to t+60s.** That is earlier than the parked t+24h, but later than `alarmScheduledNoLaterThan` (t+30s), so no precommit move-earlier runs.
- During this commit's `commitCallback` await, step 1's request resolves. Its continuation applies the parked t+24h, and `alarmScheduledNoLaterThan` becomes t+24h.
- The post-commit check asks whether t+24h fires before t+60s. It doesn't, so it skips.
Final state: SQLite holds t+60s, and the AlarmManager is scheduled for t+24h. Rewriting the same stored value doesn't bump `alarmVersion`, so nothing re-syncs until some later commit's precommit sees `stored < alarmScheduledNoLaterThan`. An idle object never has that commit.
## Observed
In production, one object's alarm, stored for about 60 s ahead, didn't fire for about 14 minutes. Worker telemetry showed no alarm invocation in between. It fired immediately after the next commit that wrote an earlier alarm value.
The window is a few milliseconds: the in-flight move-later must resolve inside the third commit's `commitCallback`. Fifteen controlled live reproductions all fired on time. The evidence is:
- the code path above;
- a two-clock model of these rules replaying the exact write sequence;
- the timing of the three commits in telemetry;
- the heal signature: same-value writes never re-synced, and the first changed earlier write did.
## Suggested fix
One of:
- When the in-flight move-later continuation drains `pendingLaterAlarmTime`, schedule `min(pendingLaterAlarmTime, metadata.getAlarm())` instead of the parked value. Equivalently, drop the parked value if the stored alarm is now earlier.
- Or, after `co_await commitCallbackPromise`, re-run the move-earlier check (`willFireEarlier(alarmStateForCommit, alarmScheduledNoLaterThan)`) as well as the move-later one.
## Workaround we applied
Only the alarm handler ever moves the alarm later. It writes the alarm once per run, and every other event only moves it earlier, so no later value is ever parked behind the handler's own request. Any event that finds its stored alarm past due re-arms it to now+1ms, which forces a precommit re-sync.
Contributor guide
Research direction
Read src/workerd/io/actor-sqlite.c++, focusing on startPrecommitAlarmScheduling(), commitImpl(), scheduleLaterAlarm(), and requestScheduledAlarm(). Trace the parked pendingLaterAlarmTime sequence and verify that a commit moving the stored alarm earlier cannot leave AlarmManager scheduled later; done means the stored and scheduled alarm times remain synchronized without a later commit.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, sqlite
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100