fix(rivetkit-core): fail closed sqlite during shutdown

[NathanFlurry]

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[NathanFlurry]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• fix(rivetkit-core): fail closed sqlite during shutdown #4912 👈 (View in Graphite)
• chore(depot-client): migrate to worker thread #4907 : 2 other dependent PRs (#4909 , #4918 )
• test(driver): select matrix cells with vitest filters #4906
• fix(pegboard): restore hibernating requests on serverful start #4904 : 1 other dependent PR (#4905 )
• fix(rivetkit): drain shutdown work before sleep #4902 : 1 other dependent PR (#4903 )
• feat(sqlite): wire remote sqlite execute handlers #4897
• test(serverless): avoid pinning header validation order #4896
• chore(driver-test-runner): add native/wasm runtime matrix to skill #4884 : 3 other dependent PRs (#4892 , #4894 , #4901 )
• chroe: rename rivetkit-sqlite to depot-client #4883
• fix(rivetkit-wasm): fix mem leaks #4880
• chore(rivetkit): wasm support #4860
• feat(sqlite): add cold read benchmarks and simplify optimizations #4857
• feat(sqlite): benchmark cold reads #4848 : 1 other dependent PR (#4864 )
• docs(actors): document manual lifecycle controls #4847
• test(sqlite): expand db stress coverage #4846 : 1 other dependent PR (#4849 )
• test(raw-websocket): add serverless smoke coverage #4845
• feat(serverless): configure drain grace period #4844
• fix(publish): enable frontend build for engine target and use turbo build instead of turbo build:engine #4843
• feat(rivetkit): expose conn handle in onWebSocket context #4842
• fix(rivetkit): normalize waitUntil/keepAwake promises to null to avoid serde undefined error #4841
• fix(rivetkit): remove requirement for token #4839 : 1 other dependent PR (#4840 )
• fix(pegboard-envoy): allow sqlite requests across lifecycle states #4838
• fix(pegboard): route scoped actor key reads by runner dc #4837
• fix(sqlite-storage): let sqlite open take over stale owners #4834
• fix(engine-tests): parallelize multi-DC startup + enable 6 multi-DC tests #4833
• fix(api): honor cursor in actor_ids list path #4832
• fix(api): silently filter invalid IDs from actor_ids query instead of 400 #4831
• fix(api-peer): make actors_delete idempotent for already-destroyed actors #4830
• fix(pegboard): use Iterator streaming mode in list_names so limit param is honored #4829
• chore(engine-tests): convert envoy tests to Sleep crash policy + triage ignored #4828
• feat(engine-tests): wire envoy actor names through prepopulate_actor_names #4827
• chore(depot): fault injection tests #4868
• feat(sqlite): pitr & forking #4859 : 1 other dependent PR (#4863 )
• chore(sqlite): stateless storage refactor #4856
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[claude]

Code Review: fix(rivetkit-core): fail closed sqlite during shutdown

Overview

This PR fixes a correctness bug where stale cloned c.db handles could reopen a local SQLite worker or keep sending remote SQL after actor cleanup. The fix adds a shared closed: Arc<AtomicBool> flag to SqliteDb, moves SQLite cleanup before final state serialization, parallelizes the remaining shutdown tasks, and adds a malloc_trim call after actor shutdown to reclaim native allocator memory. It also ships new tooling (memory soak harness, VFS metrics, registry diagnostics) and bumps the serverless start payload default to 16 MiB.

---

Correctness Issues

1. sync_alarm_logged() dropped on the Destroy path (potential bug)

In the old finish_shutdown_cleanup_with_ctx, sync_alarm_logged() was called unconditionally before the match reason arm, so it ran for both Sleep and Destroy. In the new run_shutdown_task_cleanup_alarm, it only runs inside the ShutdownKind::Sleep arm. The Destroy path now calls only cancel_driver_alarm_logged() + wait_for_pending_alarm_writes. Whether omitting sync_alarm_logged on Destroy is intentional should be verified — if not, this is a silent regression.

2. Double impl EnvoyHandle block in handle.rs

After extracting decode_serverless_actor_start_payload as a free function the diff adds a second impl EnvoyHandle { ... } block. This compiles fine in Rust but is clearly unintentional — the trailing methods (send_kv_request, etc.) should stay inside the original impl block rather than starting a new one.

3. onSleep/onDestroy are now unconditionally registered

Previously these callbacks were only registered when the user provided them (onSleep ? wrapNativeCallback(...) : undefined). They are now always registered as live NAPI callbacks. Core is expected to always fire them regardless. Confirm that rivetkit-core never optimised the None callback path in a way that produced different observable behaviour (e.g. skip the callback entirely, skipping the new closeDatabase/clearNativeRuntimeState side-effects).

---

Design & Convention Notes

4. trim_native_allocator_after_shutdown on every shutdown

Calling malloc_trim(0) after every actor stop is intentionally aggressive and was validated against the soak results. The unsafe extern "C" block is sound (standard glibc symbol). The concern is CPU overhead under high-concurrency shutdown bursts — each call can scan the full arena freelist. Consider a heuristic (e.g. only trim when the actor had SQLite enabled, or rate-limit trims) if the per-shutdown cost shows up in profiling.

5. JsSqliteVfsMetrics counter fields are f64 in NAPI

This is correct for NAPI (JS numbers are all f64), but the field names (commitCount, pageCacheEntries, etc.) suggest integer semantics. Documenting that these are counts-as-floats would help callers who might do integer arithmetic on them.

6. normalizeNativeMetrics dual-key fallback (camelCase + snake_case)

The helper reads both raw[camelKey] and raw[snakeKey] to support both NAPI (camelCase) and wasm (snake_case) backends. This works but is worth a one-line comment explaining the dual naming — a future reader may wonder why the fallback exists.

7. MALLOC_ARENA_MAX / MALLOC_TRIM_THRESHOLD_ defaults set only on the spawned engine

set_default_allocator_env applies only to the child engine process. The kitchen-sink serverless runner itself only gets these variables from dev:serverless or the user's environment. Consider noting in a comment that the caller process must set these independently if the allocator savings matter for the serverless host too.

---

Test Coverage

8. No test for ensure_open() fail-closed invariant

The shared closed flag is the core correctness fix. There's no new test asserting that a SqliteDb returns Closed errors after close() is called, or that a second open() attempt after close fails. The new action_dispatch_blocks_idle_sleep_until_reply test covers the keep-awake fix well, but the fail-closed invariant deserves its own unit test.

9. wait_actor_registered_then_stopped has no test

This new method drives the serverless SSE stopping event. A unit test that verifies the function returns on actor deregistration (and does not hang when the actor never registers) would give confidence in the notification race.

---

Minor / Style

10. Inconsistent indentation in proc-metrics-report.ts — the cpuChart, ioChart, and cycle latency scatter trace have inner properties indented one extra level relative to the rest of the file. No functional issue, but worth cleaning up before merge.

11. Checklist not filled out — the PR description checklist is blank. For a correctness fix touching the shutdown critical path, ticking at least the self-review and test items would be appropriate.

---

Summary

The core SQLite fail-closed fix and the shutdown ordering change are sound. The main items to verify before merge are: (1) whether sync_alarm_logged being dropped on the Destroy path is intentional, (2) the duplicate impl block in handle.rs, and (3) adding a test for the ensure_open invariant. Everything else is minor or advisory.