Skip to content

feat(engine): rust runner#4582

Open
NathanFlurry wants to merge 1 commit into04-06-chore_remove_non-engine_driversfrom
04-06-feat_engine_rust_runner
Open

feat(engine): rust runner#4582
NathanFlurry wants to merge 1 commit into04-06-chore_remove_non-engine_driversfrom
04-06-feat_engine_rust_runner

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

@claude
Copy link
Copy Markdown

claude bot commented Apr 7, 2026

PR Review: feat(engine): rust runner

Note: This is a draft PR — review is preliminary.

This is a substantial architectural refactor: replacing the TypeScript test runner with a Rust binary (rivet-test-envoy), promoting runner-protocol from SDK to engine package, and adding serverless actor support. The changes are largely well-structured. A few items worth addressing:


Bugs / Issues

Race condition in start_serverless (engine/sdks/rust/test-envoy/src/server.rs)

Concurrent POST /api/rivet/start calls can each start a new Envoy, with the last writer winning in the shared state.envoy lock. Earlier envoy instances would be orphaned. Consider a compare-and-swap guard or returning 409 if an envoy is already running.

SSE stream never terminates (server.rs:start_serverless)

The ping loop runs forever with no mechanism to close the SSE connection when the serverless actor finishes. The engine may hold the request open indefinitely.

Only SIGINT handled, not SIGTERM (server.rs:install_signal_handlers)

Docker sends SIGTERM on docker stop; only ctrl_c (SIGINT) is handled. The envoy will be SIGKILLed without clean shutdown after the grace period. Add a SIGTERM handler via tokio::signal::unix::signal(SignalKind::terminate()).


Security

Unauthenticated /shutdown endpoint (server.rs)

GET /shutdown has no authentication. Any process reachable on the internal network can kill the envoy. Since this is test infrastructure it may be acceptable, but worth adding at least a shared secret or binding to localhost only.


Style / Conventions

Direct reqwest::Client::new() in auto_configure_serverless (server.rs)

CLAUDE.md says to never build a new reqwest client from scratch. Use rivet_pools::reqwest::client().await?. rivet-test-envoy is a standalone binary without rivet_pools, so this may be intentional. A brief comment clarifying the exception would be useful.

Arc<Mutex<Option<Arc<Envoy>>>> in AppState

For a single optional value this is fine, but tokio::sync::RwLock would be a slight improvement since most accesses are reads, not writes.

Default token hardcoded as "dev"

Fine for local dev. Panicking with a clear error if RIVET_TOKEN is missing in non-dev environments would be safer than silently using a wrong token.


Breaking Changes

Config schema: datacenters array to object

All config.jsonc files are consistently updated. This is a breaking change for self-hosters. A migration note in the PR description would help operators upgrading.


Incomplete / To Verify

rivet-engine-test-envoy-native in Cargo.lock but source not visible in diff

napi/napi-build/napi-derive appear in the lockfile for a new rivet-engine-test-envoy-native package under engine/sdks/typescript/test-envoy-native/. The Rust source is not visible in the diff. Is this intentional or an oversight?


Positive Notes

  • Structural moves (runner-protocol to engine/packages/, TypeScript runner to rivetkit-typescript/packages/) reflect the right ownership boundaries.
  • The backward-compat shim in test_runner.rs lets engine tests migrate without a flag day.
  • auto_configure_serverless is a good addition for serverless registration.
  • Config changes applied consistently across all docker environments.
  • CLAUDE.md updated with new paths.

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