fix(rivetkit-core): cancel driver alarm before sqlite teardown on destroy

[abcxff]

No description provided.

[abcxff]

Stack for rivet-dev/rivet

• #5539 feat(container-runner): report actors as crashed on unexpect …
• #5536 feat(container-runner): log unexpected platform SIGTERM as a …
• #5530 fix(rivetkit-core): cancel driver alarm before sqlite teardo … 👈
• #5531 feat(container-runner): keep instance warm instead of self-e …
• #5533 chore(container-runner): default stop grace to 10s
• #5528 fix(rivetkit): log and surface actor startup failures
• #5529 fix(rivetkit): treat empty actor input bytes as absent
• main

Get stack: forklift get 5530
Push local edits: forklift submit
Merge when ready: forklift merge 5530

_{change qprwzozl}

[claude]

Review

Small, well-scoped fix that moves ShutdownKind::Destroy's cancel_driver_alarm_logged() call earlier in finish_shutdown_cleanup_with_ctx (from after cleanup_sqlite to right before wait_for_pending_alarm_writes).

Correctness

Traced the call chain to confirm the core fix is sound:

• cancel_driver_alarm_logged() leads to set_alarm_tracked(None), which spawns a task that awaits the envoy ack and then calls persist_last_pushed_alarm, pushing its completion receiver onto schedule_pending_alarm_writes.
• persist_last_pushed_alarm writes to the actor's own SQLite DB (internal_storage::persist_last_pushed_alarm).
• cleanup_for_shutdown(reusable=false) for Destroy calls self.close(), closing that same DB.

In the old ordering, cancel_driver_alarm_logged() ran after cleanup_sqlite, so the persist task it spawned raced against (or ran after) the DB close, plausibly producing the sqlite.transaction_closed error referenced in the comment. Moving the call before wait_for_pending_alarm_writes() ensures that write is drained before cleanup_sqlite runs. The Sleep path is untouched (alarm stays armed across sleep, only the local timer is cancelled after cleanup), so behavior there is preserved. The match also correctly enumerates both ShutdownKind variants per the no-fallthrough convention, and the explanatory comment is a good addition given this ordering constraint isn't obvious from the code alone.

Possible new race between the two alarm writes (worth double-checking)

finish_shutdown_cleanup_with_ctx calls ctx.sync_alarm_logged().await immediately before the new cancel_driver_alarm_logged() call. sync_alarm_logged resolves as soon as set_alarm_tracked queues its background persist task; it does not wait for that task to finish. So when schedule_dirty_since_push is true (for example the actor called schedule.after(...) shortly before being destroyed), this sequence now spawns two concurrent background writes to last_pushed_alarm: one persisting the real timestamp (from sync_alarm_logged) and one persisting None (from cancel_driver_alarm_logged), both drained together by the single wait_for_pending_alarm_writes() call below. Nothing enforces that the None write lands last, so it is possible for the real-timestamp write to complete after the None write and leave a stale non-None last_pushed_alarm persisted.

In the old code this could not happen, because wait_for_pending_alarm_writes() fully drained the sync write before cancel_driver_alarm_logged() was even invoked (it ran later, after cleanup_sqlite), so the two writes were strictly ordered.

Impact is likely low in practice since the actor is being destroyed and its storage should eventually be torn down or reused under a new generation regardless, but it would be good to confirm this stale-value window is actually harmless (for example via the single-writer/generation invariants) rather than relying on it being unobservable.

Test coverage gap

The existing tests (destroy_shutdown_still_clears_driver_alarm_after_cleanup, sleep_shutdown_preserves_driver_alarm_after_cleanup) only assert test_driver_alarm_cancel_count(), i.e. that the cancel happened exactly once, and neither configures a schedule_envoy_handle. So cancel_driver_alarm_logged() hits its early None return (schedule.rs:933) and none of these tests actually exercise the SQLite persist path at all; they would pass identically under the old, buggy ordering. Nothing in the diff exercises the actual race this fixes (persist write vs. DB close), or the new potential race described above. It would strengthen this PR to add a test that configures a schedule envoy handle, schedules an alarm, destroys the actor, and asserts the shutdown completes cleanly with the final persisted alarm state being None, something that would have failed (or been flaky) before this change and is deterministic after.

Other

• No security concerns; this only reorders steps in a single-actor shutdown path, and the actor-local SQLite trust boundary is unaffected.
• Performance impact is negligible: set_alarm_with_ack's ack fires as soon as the message is queued to the envoy send loop rather than waiting on a network round trip, so awaiting it during destroy shutdown should not add meaningful latency.
• Scope is minimal and consistent with the stated fix, with no unrelated changes.