Skip to content

fix(rivetkit-napi): release runtime_state ref via TSF instead of leaking it - #5634

Open
AlSh007 wants to merge 1 commit into
rivet-dev:mainfrom
AlSh007:fix/napi-runtime-state-ref-leak
Open

fix(rivetkit-napi): release runtime_state ref via TSF instead of leaking it#5634
AlSh007 wants to merge 1 commit into
rivet-dev:mainfrom
AlSh007:fix/napi-runtime-state-ref-leak

Conversation

@AlSh007

@AlSh007 AlSh007 commented Sep 2, 2026

Copy link
Copy Markdown

Summary

  • Fixes (Issue)- unbounded memory growth during actors cycling. #5607: ActorContextShared::reset_runtime_state() (and its Drop impl) run on Tokio worker threads with no N-API Env available, so releasing the runtime_state napi Ref there previously fell back to std::mem::forget. Since this runs on every actor wake cycle, a long-running process with many actors cycling through sleep/wake accumulates leaked V8 reference slots without bound.
  • Replaces the unconditional mem::forget with a lazily-installed ThreadsafeFunction that hops onto the JS thread to call Ref::unref properly. The TSF is installed the first time runtime_state(env) is called with a real Env, and is itself unref'd so it doesn't keep the Node process alive. The mem::forget fallback is kept only for the case where the dropper was never installed (e.g. a test context with no JS runtime) or where unref itself fails (e.g. environment teardown), avoiding a debug-mode zero-count assertion panic in napi-rs.
  • This matches the fix design already sketched in this repo's own docs-internal/engine/napi-bridge.md notes.

Verification

  • cargo check -p rivetkit-napi and cargo clippy -p rivetkit-napi --no-deps are clean.
  • Full existing cargo test -p rivetkit-napi --lib suite passes (20/20), including the test that exercises reset_runtime_state.
  • Built the real native .node addon (napi build --platform) and, with temporary instrumentation (not included in this diff), confirmed against a real Node process that 50 consecutive runtime_state(env) + reset_runtime_state() cycles resulted in all 50 references being released through the new dropper rather than leaked.
  • I did not add a permanent automated integration test for this, since doing so would need either new test-only #[napi] surface or a flake-prone memory-growth assertion. Happy to add one if maintainers have a preferred pattern for this (the napi-bridge.md note mentions wanting a NAPI integration test that "waits for the cleanup TSF to drain, and verifies native reference counts return to zero").

Test plan

  • cargo check -p rivetkit-napi
  • cargo clippy -p rivetkit-napi --no-deps
  • cargo test -p rivetkit-napi --lib
  • Manual verification against a real built .node addon (see above)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

(Issue)- unbounded memory growth during actors cycling.

1 participant