fix(rivetkit-napi): release runtime_state ref via TSF instead of leaking it - #5634
Open
AlSh007 wants to merge 1 commit into
Open
fix(rivetkit-napi): release runtime_state ref via TSF instead of leaking it#5634AlSh007 wants to merge 1 commit into
AlSh007 wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ActorContextShared::reset_runtime_state()(and itsDropimpl) run on Tokio worker threads with no N-APIEnvavailable, so releasing theruntime_statenapiRefthere previously fell back tostd::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.mem::forgetwith a lazily-installedThreadsafeFunctionthat hops onto the JS thread to callRef::unrefproperly. The TSF is installed the first timeruntime_state(env)is called with a realEnv, and is itselfunref'd so it doesn't keep the Node process alive. Themem::forgetfallback is kept only for the case where the dropper was never installed (e.g. a test context with no JS runtime) or whereunrefitself fails (e.g. environment teardown), avoiding a debug-mode zero-count assertion panic in napi-rs.docs-internal/engine/napi-bridge.mdnotes.Verification
cargo check -p rivetkit-napiandcargo clippy -p rivetkit-napi --no-depsare clean.cargo test -p rivetkit-napi --libsuite passes (20/20), including the test that exercisesreset_runtime_state..nodeaddon (napi build --platform) and, with temporary instrumentation (not included in this diff), confirmed against a real Node process that 50 consecutiveruntime_state(env)+reset_runtime_state()cycles resulted in all 50 references being released through the new dropper rather than leaked.#[napi]surface or a flake-prone memory-growth assertion. Happy to add one if maintainers have a preferred pattern for this (thenapi-bridge.mdnote 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-napicargo clippy -p rivetkit-napi --no-depscargo test -p rivetkit-napi --lib.nodeaddon (see above)