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

[abcxff]

No description provided.

[abcxff]

Stack for rivet-dev/rivet

• #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
• #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, focused fix to the destroy shutdown sequence in rivetkit-core's ActorTask::finish_shutdown_cleanup_with_ctx. Moves cancel_driver_alarm_logged() for ShutdownKind::Destroy from after cleanup_sqlite to before wait_for_pending_alarm_writes().

Correctness

• The fix is well targeted. cancel_driver_alarm_logged() calls set_alarm_tracked(..., None, ...) (schedule.rs:927), which spawns a detached task that awaits the engine ack and then calls persist_last_pushed_alarm, pushing the future into schedule_pending_alarm_writes (schedule.rs:964). In the old ordering this ran after ctx.sql().cleanup_for_shutdown(...) had already torn down the actor's SQLite connection during a Destroy shutdown, so the persist write could race the teardown and fail (transaction_closed), only surfacing as a logged background error since the spawn is fire-and-forget. Moving the call before wait_for_pending_alarm_writes().await ensures the persist is queued and awaited before cleanup_sqlite runs, closing the race.
• Leaving ShutdownKind::Sleep's cancel_local_alarm_timeouts() after cleanup_sqlite (unchanged) is correct: it only aborts a local Tokio timer via an atomic epoch bump, no I/O, no SQLite involvement, and per the comment this intentionally keeps the persisted engine alarm armed across sleep for the next instance, matching the reference TS runtime.
• The match reason { ... } enumerates both ShutdownKind variants explicitly with no wildcard arm, consistent with CLAUDE.md's match-statement convention.
• The inline comment explaining why the reordering matters is a good addition given CLAUDE.md's guidance to document non-obvious rationale rather than deltas.

Test coverage gap

• No test file is touched by this PR. tests/task.rs already has destroy_shutdown_still_clears_driver_alarm_after_cleanup and sleep_shutdown_preserves_driver_alarm_after_cleanup, but both only assert test_driver_alarm_cancel_count() — a count, not ordering. Both would pass identically under the old, buggy ordering, since the race manifests as a background tracing::error! in the spawned persist task, not as a failure the test would observe. This PR doesn't add anything that would catch a regression of this exact bug in the future.
  • Consider a regression test that fails under the old ordering: e.g. a fake driver/hook that errors if a SQLite write is attempted after cleanup_for_shutdown/close() has started, or a hook that asserts wait_for_pending_alarm_writes actually drained the write generated by cancel_driver_alarm_logged before cleanup_sqlite's step-completion log fires.
  • destroy_shutdown_still_clears_driver_alarm_after_cleanup is now a slightly misleading name, since the cancel now happens before cleanup_sqlite for Destroy. Not blocking, but a rename would keep it self-documenting.

Minor

• No other functional issues. The diff is minimal and self-contained, and follows the existing step-by-step tracing/debug-log pattern used throughout this shutdown path. Log field usage (actor_id, reason, step) matches the structured-logging convention in CLAUDE.md.

Overall: a solid, minimal race-condition fix with clear rationale. Main suggestion is adding a regression test that actually exercises the ordering being fixed, since the current tests can't tell the two orderings apart.