fix(rivetkit): reject conflicting local envoy connections

[NathanFlurry]

No description provided.

[NathanFlurry]

Stack for rivet-dev/rivet

• #5512 fix(actors): surface unregistered actor startup errors
• #5514 fix(rivetkit): protect local serverless handler ownership
• #5513 fix(rivetkit): reject conflicting local envoy connections 👈
• #5510 feat(rivetkit): isolate managed engine storage by port
• #5511 feat(engine-process): support caller-defined engine instance …
• main

Get stack: forklift get 5513
Push local edits: forklift submit
Merge when ready: forklift merge 5513

_{change sskuossq}

[claude]

Review

Re-reviewed the current diff (commit 4c625c0, still unchanged since the last review). This adds an opt-in check (reject_existing_envoy) that queries the Engine's /envoys list before starting a serve-mode Envoy, and bails out with an actionable error if another Envoy is already connected for the same namespace/pool. Independently re-traced the same code paths this pass; findings are unchanged.

Correctness - likely bug: missing "local engine" gate

ensure_development_envoy_available (rivetkit-rust/packages/rivetkit-core/src/registry/runner_config.rs:50) runs unconditionally whenever config.reject_existing_envoy is true. Unlike its sibling ensure_local_normal_runner_config (same file, line 33), it never checks is_local_engine_endpoint(&config.endpoint).

On the TypeScript side, both call sites set the flag purely from NODE_ENV:

rejectExistingEnvoy: isDev(),  // native.ts:5032, runtime.ts:773

isDev() is getNodeEnv() !== "production" - true whenever NODE_ENV isn't exactly "production" (unset, "development", "test", self-hosted containers that never set it, CI, staging builds, etc.), regardless of whether config.endpoint points at localhost or a shared/remote/cloud Engine. Confirmed buildServeConfig in native.ts sets rejectExistingEnvoy with no other endpoint-based gating around it.

Per docs-internal/engine/envoy-load-balancing.md, having many Envoys concurrently connected to the same (namespace, pool) is the normal, expected way pools scale horizontally (hash-ring load balancing across envoys is a first-class feature, not an edge case), and poolName defaults to a fixed "default" string shared across replicas (registry/config/index.ts), not something auto-uniqued per instance. So the common "run N replicas of the same service" deployment shares one (namespace, pool).

Net effect: any process where NODE_ENV != "production" and which points at a non-local Engine (e.g. a container/staging/preview deployment that never sets NODE_ENV=production, or multiple replicas in such an environment) will now refuse to start as soon as a second replica boots and finds the first replica's Envoy already registered for that pool - even though multiple Envoys per pool is by design. This looks like it will misfire well outside the single-machine hot-reload scenario the feature is meant for.

Suggest gating ensure_development_envoy_available the same way as ensure_local_normal_runner_config: early-return when !is_local_engine_endpoint(&config.endpoint), so the check only ever fires for loopback/localhost Engine endpoints. That also matches the error message's own framing ("stop the other project", RIVET_RUN_ENGINE_PORT), which is clearly written for the local-dev-only case. Worth adding a test that a non-local endpoint skips the check even when reject_existing_envoy is true (there's currently no coverage for this, since the guard itself is missing).

Minor / non-blocking

• TOCTOU race: ensure_development_envoy_available checks-then-starts; two processes racing to start against the same local Engine within the ~500ms retry window could both pass the check and both start Envoys. Presumably acceptable for a best-effort dev UX guard, but worth a one-line comment noting it's advisory, not a hard exclusion guarantee.
• No retry on transport/decode errors: inside ensure_development_envoy_available_with_retry, envoy_is_connected(...).await? propagates any network error, non-2xx status, or JSON decode failure immediately via ?, bypassing the retry loop entirely (only the "envoy present" case retries). This is asymmetric with ensure_local_normal_runner_config_with_retry, whose AttemptError::Retryable/Fatal split explicitly retries transient network/5xx failures. Likely low-risk in practice since ensure_local_normal_runner_config already confirms the Engine is responsive immediately beforehand, but worth confirming that's intentional given the function's _with_retry naming.
• Dead wiring on wasm: reject_existing_envoy is threaded through WasmServeConfig / rivetkit-wasm/src/lib.rs and set from isDev() in runtime.ts, but the call to ensure_development_envoy_available in mod.rs is behind #[cfg(feature = "native-runtime")], which wasm builds don't enable per this repo's wasm feature set. The field is harmless but currently a no-op for wasm - worth a comment noting that, or skip wiring it there until the wasm path can act on it.
• Ordering: ensure_local_normal_runner_config (which upserts runner config on the Engine) runs before ensure_development_envoy_available (which may then bail). Not harmful since the upsert is idempotent, but if the intent is to fail fast before touching the Engine's runner config, the order could be flipped.

Test coverage

The new Rust tests (runner_config.rs) cover: disabled-check skip, retry-until-clear, and exhausted-retries-with-actionable-message. Good coverage for the retry logic itself. As noted above, missing coverage for the local-vs-remote endpoint gating, because the gate doesn't exist yet. No TS-side tests were added for the rejectExistingEnvoy: isDev() wiring in native.ts/runtime.ts.