Skip to content

fix(rivetkit-core): don't install tokio::signal::ctrl_c() handler inside serve_with_config#4731

Draft
NathanFlurry wants to merge 1 commit into04-23-refactor_rivetkit-core_use_subtle_constanttimeeq_for_inspector_token_verifyfrom
04-23-fix_rivetkit-core_don_t_install_tokio_signal_ctrl_c_handler_inside_serve_with_config
Draft

fix(rivetkit-core): don't install tokio::signal::ctrl_c() handler inside serve_with_config#4731
NathanFlurry wants to merge 1 commit into04-23-refactor_rivetkit-core_use_subtle_constanttimeeq_for_inspector_token_verifyfrom
04-23-fix_rivetkit-core_don_t_install_tokio_signal_ctrl_c_handler_inside_serve_with_config

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

NathanFlurry commented Apr 24, 2026

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

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

@NathanFlurry NathanFlurry force-pushed the 04-23-fix_rivetkit-core_don_t_install_tokio_signal_ctrl_c_handler_inside_serve_with_config branch from 397fe1f to 8a8be67 Compare April 24, 2026 09:52
@NathanFlurry NathanFlurry force-pushed the 04-23-refactor_rivetkit-core_use_subtle_constanttimeeq_for_inspector_token_verify branch from d26d955 to 1c8fb57 Compare April 24, 2026 09:52
@NathanFlurry NathanFlurry mentioned this pull request Apr 24, 2026
11 tasks
@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Code Review: fix(rivetkit-core): don't install ctrl_c() handler inside serve_with_config

Overview

This PR fixes a real and subtle bug: calling tokio::signal::ctrl_c() inside serve_with_config installs a sigaction(SIGINT, ...) handler at the POSIX level. When rivetkit-core is embedded in Node.js via NAPI, this override prevents the host process from responding to SIGINT, leaving it unable to exit normally. The fix moves ctrl+c handling out of the library and into the callers (examples), and uses drop-based resource cleanup via std::future::pending().

The fix is correct and necessary. A few areas worth considering before merging:


Issues

1. Envoy handle is dropped without sending a shutdown signal

The old code explicitly called handle.shutdown(false) which sends ToEnvoyMessage::Shutdown to the envoy task, giving it a chance to drain connections. The new code just drops _handle, which decrements the Arc<SharedContext> ref count but sends no signal. The envoy task will only notice the shutdown when the envoy_tx sender side disappears (when SharedContext fully drops). Active connections/in-flight operations may be cut off more abruptly than before.

If the NAPI caller is responsible for graceful shutdown, there should be a documented path for it — either a method on a returned handle, or a CancellationToken parameter. Right now there is no way for a caller to trigger graceful shutdown short of dropping the spawned task.

2. Engine process gets SIGKILL'd instead of SIGTERM'd

kill_on_drop(true) uses SIGKILL on Unix, not SIGTERM. The old EngineProcessManager::shutdown() called terminate_engine_process which presumably sends SIGTERM and waits for clean exit before escalating. Any in-flight persistence operations in the engine process are now cut off hard.

For the embedded-in-NAPI case this may be acceptable (the engine process is local and its state is durable), but the change in behavior is worth calling out explicitly.

3. Log tasks are detached on drop

EngineProcessManager has stdout_task and stderr_task JoinHandles. Dropping a JoinHandle in Tokio detaches the task, it does not abort it. These tasks will continue running briefly after drop until the stdout/stderr pipes close (which happens right after kill_on_drop fires). No data loss, but worth noting if anyone later adds a Drop impl or tries to join these tasks.

4. _handle naming is slightly misleading

Underscore-prefixed identifiers conventionally mean "intentionally unused." Here, _handle is very much intentionally alive — it is a drop guard that keeps the envoy connection open. A short comment makes the intent clear:

// Keep the envoy handle alive for the duration of serve; drop drives shutdown.
let _handle = start_envoy(...).await;

Positive Notes

  • The comment on std::future::pending is excellent — it explains the why (POSIX sigaction override), where the behavior applies (NAPI embedding), and what the caller is expected to do instead. Follows the project's comment conventions well.
  • Moving ctrl_c into the examples is the right call. Libraries should not install process-wide signal handlers.
  • Adding kill_on_drop(true) is necessary for correctness — without it, the child engine process would be orphaned when the future is cancelled.
  • The tokio::select! pattern in the examples is idiomatic and clear.

Minor

The _engine_process rename in the same function could also use a short comment (same drop-guard rationale as _handle), since both now have the underscore-but-live pattern in the same function body.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Code Review: fix(rivetkit-core): remove tokio::signal::ctrl_c() from serve_with_config

Overview

This PR removes the tokio::signal::ctrl_c() handler from serve_with_config to fix SIGINT override when rivetkit-core is embedded in Node.js via NAPI. serve_with_config now uses std::future::pending() to run indefinitely until the caller drops the task, and example binaries handle ctrl_c themselves via tokio::select!. Additionally, .kill_on_drop(true) is added to the engine child process.

The fix is directionally correct. There are a few issues worth addressing before merging.


Critical: Envoy client still installs a ctrl_c handler

engine/sdks/rust/envoy-client/src/envoy.rs:274-279 spawns its own ctrl_c handler inside start_envoy_sync_inner:

// Start signal handler
let handle2 = handle.clone();
tokio::spawn(async move {
    let _ = tokio::signal::ctrl_c().await;
    handle2.shutdown(false);
});

This calls sigaction(SIGINT, ...) just like the removed handler, so the Node.js SIGINT override bug is still present at runtime. The fix needs to address this location as well. One option: make the signal handler opt-in via a flag on EnvoyConfig (off by default when embedded, on for standalone binaries), mirroring how serve_with_config callers now own signal handling.


Moderate: Graceful envoy shutdown path is removed

The old code called handle.shutdown(false) which sends ToEnvoyMessage::Shutdown and lets the envoy flush in-flight actor work. EnvoyHandle has no Drop implementation, so renaming to _handle does not trigger a graceful shutdown. The background envoy task keeps running — the spawned signal-handler task holds an Arc<SharedContext> that keeps envoy_tx alive, so the channel never closes from drop alone.

Consider adding a Drop impl to EnvoyHandle that calls shutdown(false), or documenting that callers must call handle.shutdown() explicitly before cancelling the serve_with_config future.


Minor: Engine process teardown regresses from SIGTERM to SIGKILL

Old path: engine_process.shutdown().await calls terminate_engine_process which sends SIGTERM, waits, then SIGKILL.

New path: kill_on_drop(true) sends immediate SIGKILL on drop.

For a local dev engine this is probably acceptable, but it is a silent behavioral change. A Drop impl on EngineProcessManager calling terminate_engine_process in a block_in_place would restore graceful teardown.


What Is Done Well

  • The comment in registry/mod.rs is excellent — naming sigaction(SIGINT, ...) and the NAPI consequence is exactly the kind of non-obvious constraint that belongs in code comments.
  • std::future::pending() is idiomatic for "run until cancelled" and correctly shifts lifecycle ownership to the caller.
  • Moving ctrl_c to example main functions via tokio::select! is the right pattern for binary entry points.
  • .kill_on_drop(true) is a good safety net against orphan processes.

Summary

The fix is incomplete: the ctrl_c handler in envoy-client/src/envoy.rs still fires in a NAPI context and needs to be addressed. The graceful shutdown regression for the envoy should also be resolved, either via a Drop impl or explicit documentation of the new caller contract.

@NathanFlurry NathanFlurry force-pushed the 04-23-refactor_rivetkit-core_use_subtle_constanttimeeq_for_inspector_token_verify branch from 1c8fb57 to 7377471 Compare April 24, 2026 10:19
@NathanFlurry NathanFlurry force-pushed the 04-23-fix_rivetkit-core_don_t_install_tokio_signal_ctrl_c_handler_inside_serve_with_config branch from 8a8be67 to 026a6ce Compare April 24, 2026 10:19
@NathanFlurry NathanFlurry force-pushed the 04-23-refactor_rivetkit-core_use_subtle_constanttimeeq_for_inspector_token_verify branch from 7377471 to 6b8454a Compare April 24, 2026 10:32
@NathanFlurry NathanFlurry force-pushed the 04-23-fix_rivetkit-core_don_t_install_tokio_signal_ctrl_c_handler_inside_serve_with_config branch from 026a6ce to 836ab72 Compare April 24, 2026 10:32
@NathanFlurry NathanFlurry force-pushed the 04-23-fix_rivetkit-core_don_t_install_tokio_signal_ctrl_c_handler_inside_serve_with_config branch from 836ab72 to 82a4042 Compare April 24, 2026 11:48
@NathanFlurry NathanFlurry force-pushed the 04-23-refactor_rivetkit-core_use_subtle_constanttimeeq_for_inspector_token_verify branch from 6b8454a to 4026613 Compare April 24, 2026 11:48
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.

1 participant