Store Durable Object alarms per-namespace on disk#6104
Store Durable Object alarms per-namespace on disk#6104threepointone wants to merge 3 commits intomainfrom
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
the workers-sdk test failure seems to be just about fixing the file count on the workers-sdk side |
|
/bigbonk roast this PR. Hi Sunil. testing bonk |
|
/bigbonk roast this PR |
|
OK, I've seen enough. Time to roast. The Roast"(thanks opus)" -- The PR description is literally better written than most theses I've reviewed, and you're crediting it to a chatbot. Sunil, this is the equivalent of showing up to a potluck with a Michelin-star catered dish and going "oh yeah my microwave did this." The actual code though? Let's talk about it. The You check The
The test (line 2293+) sets an alarm for File count assertions -- you bumped
The deletion of The facets test fix (line 5031+) where you added Bottom line: The design is sound -- per-namespace alarm storage is clearly the right call, the PR description is immaculate (thanks, Opus), and the test coverage is genuinely good. But the implementation has the telltale signs of "an AI wrote the structure and a human glued it together" -- slightly redundant control flow, a few too many "trust me, this is fine" comments, and magic numbers scattered like confetti. 7/10 -- would merge after a cleanup pass. The architecture carries it. |
src/workerd/server/server.c++
Outdated
| this->actorStorage.emplace(dir.openSubdir( | ||
| kj::Path({d.uniqueKey}), kj::WriteMode::CREATE | kj::WriteMode::MODIFY)); | ||
|
|
||
| // Create per-namespace alarm scheduler backed by on-disk storage in the | ||
| // namespace directory, alongside the per-actor .sqlite files. | ||
| auto& as = KJ_ASSERT_NONNULL(this->actorStorage); |
There was a problem hiding this comment.
| this->actorStorage.emplace(dir.openSubdir( | |
| kj::Path({d.uniqueKey}), kj::WriteMode::CREATE | kj::WriteMode::MODIFY)); | |
| // Create per-namespace alarm scheduler backed by on-disk storage in the | |
| // namespace directory, alongside the per-actor .sqlite files. | |
| auto& as = KJ_ASSERT_NONNULL(this->actorStorage); | |
| auto& as = this->actorStorage.emplace(dir.openSubdir( | |
| kj::Path({d.uniqueKey}), kj::WriteMode::CREATE | kj::WriteMode::MODIFY)); | |
| // Create per-namespace alarm scheduler backed by on-disk storage in the | |
| // namespace directory, alongside the per-actor .sqlite files. |
src/workerd/server/server.c++
Outdated
|
|
||
| KJ_IF_SOME(d, config.tryGet<Durable>()) { | ||
| auto idFactory = kj::heap<ActorIdFactoryImpl>(d.uniqueKey); | ||
| sched->registerNamespace(d.uniqueKey, |
There was a problem hiding this comment.
So, AlarmScheduler is currently designed to handle multiple namespaces registered at once, and to that end the table it creates has a column called actor_unique_key which identifies the DO namespace.
But now that we store a separate database per namespace, we don't really want that anymore. It's just wasting bytes and potentially causing headaches if someone wants to change keys or something.
Seems like we should update the AlarmScheduler implementation to remove the redundant functionality.
This is something that's best to do now, not later after a bunch of these tables have already been written to disk...
threepointone
force-pushed
the
alarm-storage
branch
from
95b7ec1 to
a99f831
Compare
|
got the clanker to rebase and force push a rewrite, 🏓 |
|
LGTM but how do we make Bonk re-review? |
|
/bigbonk review this pr again |
|
ResolveMessage: Cannot find module '@opencode-ai/plugin' from '/home/runner/work/workerd/workerd/.opencode/tools/bazel-deps.ts' |
|
@threepointone Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review this pr again |
|
error: Your local changes to the following files would be overwritten by checkout: |
|
Maybe you need to rebase on main to pick up bonk fixes? |
|
/bigbonk review this pr again |
|
The generated output of |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
kentonv's two review comments are addressed: the actor_unique_key column has been removed from the _cf_ALARM schema, and the registerNamespace() / multi-namespace machinery has been stripped from AlarmScheduler in favor of a single GetActorFn passed at construction time. kentonv marked LGTM.
Findings below are minor — the design is sound and the implementation is correct. The per-namespace ownership model is clean, lifetime safety looks good, and the persistence test is a valuable addition.
This review was generated by an AI assistant (ask-bonk) and may contain errors. Please verify any suggestions before applying them.
There was a problem hiding this comment.
Review Summary
kentonv's two review comments are addressed: the actor_unique_key column has been removed from the _cf_ALARM schema, and the registerNamespace() / multi-namespace machinery has been stripped from AlarmScheduler in favor of a single GetActorFn passed at construction time. kentonv marked LGTM.
Findings below are minor -- the design is sound and the implementation is correct. The per-namespace ownership model is clean, lifetime safety looks good, and the persistence test is a valuable addition.
This review was generated by an AI assistant (ask-bonk) and may contain errors. Please verify any suggestions before applying them.
| } | ||
|
|
||
| this->alarmScheduler = alarmScheduler; | ||
| KJ_IF_SOME(d, config.tryGet<Durable>()) { |
There was a problem hiding this comment.
Nit: link() calls config.tryGet<Durable>() twice -- once at line 2229 inside the storage block, and again here for the alarm scheduler block. kentonv's original suggestion was to fold these together. The second check could be nested inside the first to avoid the redundant dispatch:
void link(kj::Maybe<const kj::Directory&> serviceActorStorage) {
KJ_IF_SOME(d, config.tryGet<Durable>()) {
KJ_IF_SOME(dir, serviceActorStorage) {
this->actorStorage.emplace(dir.openSubdir(
kj::Path({d.uniqueKey}), kj::WriteMode::CREATE | kj::WriteMode::MODIFY));
}
auto idFactory = kj::heap<ActorIdFactoryImpl>(d.uniqueKey);
// ... rest of alarm scheduler setup
}
}That said, kentonv already marked LGTM on the current structure, so this is just a nit.
|
|
||
| // Having torn everything down, the WAL files should be gone. | ||
| KJ_EXPECT(dir->openSubdir(kj::Path({"mykey"}))->listNames().size() == 2); | ||
| KJ_EXPECT(dir->openSubdir(kj::Path({"mykey"}))->listNames().size() == 3); |
There was a problem hiding this comment.
After teardown the count is 3 (two actor .sqlite files + metadata.sqlite), but metadata.sqlite isn't explicitly asserted here unlike the two actor files on lines 2271-2274. Consider adding:
KJ_EXPECT(dir->exists(kj::Path({"mykey", "metadata.sqlite"})));for symmetry with the assertions above and to make the test self-documenting about what the third file is.
| auto dir = kj::newInMemoryDirectory(kj::nullClock()); | ||
|
|
||
| // A far-future alarm time (won't fire during the test). | ||
| kj::StringPtr alarmTime = "4102444800000"; |
There was a problem hiding this comment.
Nit: 4102444800000 is January 1, 2100 UTC in milliseconds. A comment or named constant would make this clearer at a glance, e.g.:
// January 1, 2100 00:00:00 UTC in milliseconds since epoch
kj::StringPtr alarmTime = "4102444800000";The comment on line 2359 helps, but embedding the date in a constant name or a more explicit inline comment would help future readers.
|
Review posted on PR #6104. Here's a summary of what I found: Status: kentonv's two review comments are both addressed in the second commit. He asked to (1) restructure Findings posted (all minor nits):
No blocking issues found. The design is sound -- per-namespace alarm storage with correct lifetime ordering, safe |
|
I rebased on main. But it appears that #1918 was merged recently and it conflicts with this PR -- not explicitly like a merge conflict, but the resulting code doesn't compile. At a glance I'm not really sure what the right answer is. That PR makes But in the abstract, it doesn't seem like "abort all durable objects" should imply "delete all alarms"! Aborting is not deleting! Perhaps in practice this Do we really just want to delete the alarms, or would it make more sense to delete all the data? I feel like "delete alarms but not content" just means your DOs are left in an inconsistent state. Maybe vitest is already deleting content in these cases, after it aborts the DOs? Can someone with context on vitest suggest what is really the right thing to do here? @petebacondarwin, @dario-piotrowicz, @penalosa? |
I think for Vitest we just want to cancel a "session", which involves aborting DOs but also deleting alarms. How about we split the API into two (abort DOs and delete alarms) then Vitest can call both and abortAllDurableObjects would no longer delete the alarms? |
Previously, all alarms were stored in a single global in-memory SQLite database, meaning they were lost on process restart. This made it impossible to test alarm resiliency scenarios. Move alarm scheduler ownership from Server into ActorNamespace, so each DO namespace gets its own AlarmScheduler backed by metadata.sqlite in the namespace's storage directory. On-disk namespaces get persistent alarms; in-memory namespaces get in-memory alarms. No new configuration is needed -- alarm storage follows the existing durableObjectStorage setting. Ref: #605 (comment) Co-authored-by: Cursor <cursoragent@cursor.com>
Address review feedback from kentonv: since each DO namespace now has its own separate SQLite database for alarm storage, the actor_unique_key column in the _cf_ALARM table is redundant. Previously, AlarmScheduler was designed to handle multiple namespaces in a single database, using actor_unique_key to distinguish between them. Now that we store a separate database per namespace, this column just wastes bytes and could cause headaches if someone wants to change keys later. As kentonv noted, this is best fixed now before any of these tables are written to disk in the wild. Changes: - Simplified ActorKey struct to contain only actorId (removed uniqueKey) - Simplified _cf_ALARM schema from (actor_unique_key, actor_id, scheduled_time) with a compound primary key to (actor_id, scheduled_time) with actor_id as the sole primary key - Removed registerNamespace() -- the GetActorFn is now passed directly to the AlarmScheduler constructor, since each scheduler serves exactly one namespace - Replaced the namespaces HashMap with a single GetActorFn member - Restructured ActorNamespace::link() in server.c++ to create the GetActorFn once and pass it to whichever storage path (on-disk or in-memory) is used, eliminating duplication - Removed unused setAlarmInMemory declaration and unused byte type alias
|
I think the workers-sdk test failures are genuine. I imagine we'll need to filter out the |
An upcoming workerd change (cloudflare/workerd#6104) stores per-namespace alarm metadata in a metadata.sqlite file alongside per-actor .sqlite files. Filter it out so it isn't counted as a Durable Object instance.
|
The Windows failure looks genuine but unfortunately the stack trace isn't symbolized which makes it pretty hard for me to debug. :/ |
These are now green after the workers-sdk fix |
|
I've sent both Opus 4.6 and GPT 5.4 off to carefully read the code and try to blind-debug the Windows issue, since I don't have a Windows machine available. If that doesn't work, I'm going to need someone with a Windows machine to try to do some debugging. |
|
They both got it. ChatGPT 5.4: 99k tokens (7 minutes) I think Opus is running a little slow today, maybe overloaded, but GPT is the clear winner either way. GPT: Opus: |
6 participants
(thanks opus)
Alarms in workerd are currently stored in a single global in-memory SQLite database. This means all scheduled alarms are lost on process restart, which prevents testing alarm resiliency scenarios and doesn't match production behavior.
This PR moves alarm scheduler ownership from
ServerintoActorNamespace, so each DO namespace gets its ownAlarmSchedulerbacked bymetadata.sqlitein the namespace's storage directory. On-disk namespaces get persistent alarms; in-memory namespaces get in-memory alarms.Follows up on the design discussion in #605 (comment).
Design decisions
Per-namespace, not global. Kenton's original feedback on #605 was that a single global alarm database is problematic: it decouples alarm storage from namespace data, it makes splitting/combining configs lossy, and it creates the confusing possibility of on-disk DOs with in-memory alarms (or vice versa). Each namespace now owns its scheduler and stores alarms alongside its actor
.sqlitefiles.No new configuration. Alarm storage mode is inferred from the existing
durableObjectStoragesetting. If the namespace useslocalDisk, alarms go on disk. If it'sinMemoryornone, alarms stay in memory. This was an explicit goal from the #605 discussion -- no new config knobs needed.metadata.sqliteas the filename. Named generically (rather thanalarms.sqlite) so it can hold other per-namespace metadata in the future, as Kenton suggested.AlarmSchedulerclass unchanged. The class already accepted a VFS + path in its constructor, so no API changes were needed. The only change is where schedulers are created and who owns them.Changes
server.h: Removed globalAlarmSchedulermember,startAlarmScheduler()declaration, and thealarm-scheduler.hinclude (moved to.c++).server.c++:ActorNamespacenow owns akj::Maybe<kj::Own<AlarmScheduler>>. Created inlink()using the namespace's own VFS (on-disk) or an in-memory VFS (fallback). The namespace self-registers with its scheduler at link time. Removed the global scheduler,LinkedIoChannels::alarmScheduler, and all related wiring.server-test.c++: Added a new "Durable Object alarm persistence (on disk)" test that sets an alarm, tears down the server, restarts with the same storage directory, and verifies the alarm survived. Updated two existing tests whose file-count assertions now includemetadata.sqlite.Notes for reviewers
alarm-scheduler.handalarm-scheduler.c++are completely untouched.kj::systemPreciseCalendarClock()call in theActorNamespaceconstructor is a pre-existing pattern -- the oldstartAlarmScheduler()called it the same way. Threading a calendar clock through theServerconstructor would be cleaner but significantly more churn for no immediate benefit.ActorNamespacematters:ownAlarmScheduleris declared beforeactorsso it outlives all actors that reference it (same constraint as the existingactorStoragefield).thiscapture in theregisterNamespacelambda is safe because the scheduler is owned by the namespace and destroyed first. This matches the original pattern which captured&actorNs.