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

[abcxff]

Fix alarm-cancel persist racing SQLite teardown on destroy

• On actor destroy, the engine alarm cancel ran after wait_for_pending_alarm_writes, so the persist it spawns was never awaited before SQLite teardown closed the transaction.
• This raced cleanup_sqlite: teardown usually won, logging failed to persist last pushed actor alarm (transaction_closed) and dropping the durable alarm-clear, leaving stale on-disk alarm state. Timing-dependent and intermittent; destroy-only (sleep never took this path).
• Move the destroy-time alarm cancel before wait_for_pending_alarm_writes so the drain awaits the persist, which now completes before teardown.
• Sleep still skips the cancel and only aborts its local timer after cleanup, keeping the engine alarm armed for the next instance.

[slop]

[abcxff]

Stack for rivet-dev/actors

• #5557 docs(container-runner): sync deploy docs with runner source
• #5556 fix(container-runner): read cgroup v1 memory usage under gvi …
• #5548 feat(container-runner): log version and git sha at actor sta …
• #5549 feat(container-runner): setup instance memory and cpu usage …
• #5546 fix(rivetkit): keep actor event loop alive through shutdown …
• #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
• #5558 fix(sdks): drop rivet-util metrics dep from protocol crates …
• main

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

_{change qprwzozl}

[claude]

Review

Re-verified after another rebase (now commit 2f59725), diff is unchanged from the prior review.

Small, well-scoped fix. The reordering is correct and fixes a real race.

What the change does

Moves ctx.cancel_driver_alarm_logged() (Destroy-only) to run right after sync_alarm_logged() and before wait_for_pending_alarm_writes() / cleanup_sqlite(), instead of after SQLite teardown. cancel_driver_alarm_logged calls set_alarm_tracked(None), which spawns a task that persists last_pushed_alarm to SQLite once the envoy ack arrives (persist_last_pushed_alarm in actor/state.rs:593, backed by internal_storage::persist_last_pushed_alarm). With the old ordering, that write ran after cleanup_for_shutdown had already closed the local SQLite backend (sqlite/mod.rs::close), so it could fail against a closed connection (surfacing as sqlite.transaction_closed) and was never awaited by shutdown anyway. Moving the call earlier means wait_for_pending_alarm_writes() now drains and awaits that specific pending write before SQLite is torn down, closing the race.

I traced through the ack path (envoy-client/src/actor.rs::ToActor::SetAlarm) to check whether issuing sync_alarm_logged()'s "set" and the immediately-following "cancel" back-to-back (with no intervening wait) could let their two persist writes land out of order. It shouldn't: both SetAlarm messages go through the same per-actor FIFO channel/task, so their acks (and the resulting persist submissions) resolve in send order, consistent with the single-writer SQLite invariant elsewhere in this codebase. So no new ordering hazard there.

This also brings Destroy in line with the existing pattern already used for sync_alarm_logged()'s pending write (already awaited via wait_for_pending_alarm_writes() before cleanup), so it's consistent with precedent rather than a new architecture. The match reason { Destroy => ..., Sleep => {} } arms are exhaustive per repo convention (no _ fallthrough), and the untouched Sleep path (cancel_local_alarm_timeouts() after cleanup, keeping the persisted engine alarm armed for the next instance) still looks correct.

Minor notes (non-blocking)

• Test coverage: destroy_shutdown_still_clears_driver_alarm_after_cleanup in tests/task.rs only asserts test_driver_alarm_cancel_count(), which is unchanged by this reordering. Nothing in the diff would fail if the old (buggy) ordering were reintroduced. Consider a regression test that specifically exercises "the pending alarm-cancel write completes before SQLite cleanup starts" (e.g. via a subscriber capture like the ones already at tests/task.rs:3053/4059, asserting no "failed to persist last pushed actor alarm" error is logged), so the fix can't silently regress. The destroy test's name (..._after_cleanup) is also now slightly stale since the cancel happens before cleanup, worth a rename if a new test is added.
• New blocking dependency on shutdown path: awaiting the pending alarm write means Destroy shutdown now blocks on the envoy round-trip ack for set_alarm(None) before SQLite cleanup proceeds. This mirrors existing behavior for the sync_alarm_logged write (already awaited the same way before this change), so it isn't a new risk class, but worth confirming there's an overall shutdown deadline/timeout upstream so a slow or unresponsive envoy can't stall actor destroy indefinitely.
• Comment style: the added comment includes a parenthetical ((rather than after cleanup_sqlite)) that the repo's CLAUDE.md style guidance discourages ("avoid fragmented structures with parentheticals and dashes"). Minor, but could be rephrased as its own sentence. It also explains both the Destroy-path reordering here and the Sleep-path behavior further down (cancel_local_alarm_timeouts after cleanup), which duplicates the existing comment already at that Sleep call site; consider trimming the last sentence here and letting the Sleep-side comment stand on its own.

No security or naming/style concerns beyond the above. The fix is narrowly targeted and doesn't touch unrelated code paths.