fix(rivetkit): skip alarm sync during destroy cleanup

[NathanFlurry]

No description provided.

[railway-app]

🚅 Deployed to the actors-pr-5658 environment in rivet-frontend

Service	Status	Web	Updated
frontend-inspector	😴 Sleeping (View Logs)	Web	Sep 4, 2026 at 3:26 am UTC
kitchen-sink	😴 Sleeping (View Logs)	Web	Sep 4, 2026 at 3:22 am UTC
frontend-cloud	😴 Sleeping (View Logs)	Web	Sep 4, 2026 at 3:20 am UTC
mcp-hub	✅ Success (View Logs)	Web	Sep 4, 2026 at 3:19 am UTC
ladle	✅ Success (View Logs)	Web	Sep 4, 2026 at 3:16 am UTC
website	❌ Build Failed (View Logs)	Web	Sep 4, 2026 at 3:14 am UTC

[claude]

Review

Looked at the diff (rivetkit-rust/packages/rivetkit-core/src/actor/task.rs, tests/task.rs, and the internals doc) plus the surrounding schedule.rs / state.rs / internal_storage/mod.rs code paths.

Bug: the destroy-path SQLite write isn't actually eliminated

The fix reorders shutdown cleanup so ctx.sync_alarm_logged().await (a SQLite read) only runs on ShutdownKind::Sleep, and is skipped for ShutdownKind::Destroy. The stated goal (per the new comment and the updated rivetkit-core-internals.md) is to avoid touching a SQLite coordinator that a foreign runtime may have already closed during its destroy callback.

However, ShutdownKind::Destroy still calls ctx.cancel_driver_alarm_logged() (task.rs:1871). That function (schedule.rs:984) calls set_alarm_tracked(envoy_handle, None, generation) whenever an envoy handle is registered (schedule.rs:990-994). set_alarm_tracked spawns a background task (schedule.rs:1043-1060) that awaits the envoy ack and then calls state_ctx.persist_last_pushed_alarm(timestamp_ms), a SQLite write (state.rs:934 -> internal_storage::persist_last_pushed_alarm, internal_storage/mod.rs:1026). That spawned task's completion is registered into schedule_pending_alarm_writes and is explicitly awaited right after by ctx.wait_for_pending_alarm_writes().await, which runs immediately after the reordered block in task.rs.

So if the SQLite coordinator was already closed (the exact scenario this PR targets), this write still fails via transaction_coordinator_closed_error() (sqlite/tx.rs, guarded in begin_regular_operation) and logs "failed to persist last pushed actor alarm" (schedule.rs:1051-1055), the same class of error the PR is trying to eliminate, just from a different code path.

The new test, destroy_cleanup_does_not_sync_alarm_after_runtime_closes_sqlite, doesn't catch this for two independent reasons:

1. It uses new_with_kv, which calls configure_envoy(...) and does register a schedule envoy handle, so cancel_driver_alarm_logged does reach set_alarm_tracked and does spawn the failing persist task.
2. The test only wraps the outer finish_shutdown_cleanup_with_ctx(...) future in .with_subscriber(dispatch). The persist task is spawned via handle.spawn(...) as a separate top-level tokio task, so it runs outside that wrapped future's poll scope and the thread-local dispatch override doesn't apply to it, meaning its tracing::error! call is invisible to the test's log capture regardless. The test also only asserts against the unrelated message "failed to sync scheduled actor alarm", not the message this path actually logs.

So in production, with a real envoy handle attached, destroy cleanup after a foreign runtime closes SQLite will still hit a "failed to persist last pushed actor alarm" error, even though the new test passes.

Worth checking: should cancel_driver_alarm_logged (or set_alarm_tracked) skip the persist-on-ack step when called from the destroy path, mirroring the skip that was just added for sync_alarm_logged? Or is there a reason the write is expected to be safe/no-op here that the read wasn't? If the latter, a short comment explaining why would help, since it isn't obvious from the code.

Nits

• The updated docs-internal/engine/rivetkit-core-internals.md persistence-order bullet ("Sync the driver alarm for sleep, or cancel it without querying SQLite for destroy.") reads a bit ambiguously as one combined line; might be clearer split per shutdown kind, though this is minor.

Other areas checked, no issues found

• No security or performance concerns beyond the correctness issue above.
• The log-capture pattern using a Dispatch + with_subscriber is reasonable in general; the gap here is specifically that it doesn't cover work spawned onto a separate top-level task.