Skip to content

refactor(container-runner): rewrite on rivetkit actor runtime#5394

Open
abcxff wants to merge 1 commit into
stack/feat-rivetkit-re-export-serverless_http-and-default-absent-actor-input-kknzrylwfrom
stack/refactor-container-runner-rewrite-on-rivetkit-actor-runtime-mwnmozsx
Open

refactor(container-runner): rewrite on rivetkit actor runtime#5394
abcxff wants to merge 1 commit into
stack/feat-rivetkit-re-export-serverless_http-and-default-absent-actor-input-kknzrylwfrom
stack/refactor-container-runner-rewrite-on-rivetkit-actor-runtime-mwnmozsx

Conversation

@abcxff

@abcxff abcxff commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@abcxff

abcxff commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Review: refactor(container-runner): rewrite on rivetkit actor runtime

Nice migration overall — this replaces ~800 lines of hand-rolled EnvoyCallbacks/axum serverless plumbing and a custom inspector shim with the rivetkit (Rust) Actor trait + serverless_http runtime, which is a solid simplification and keeps container-runner aligned with the rest of the RivetKit ecosystem. I verified the new rivetkit API usage (Actor hook signatures, Ctx::destroy(), WebSocket callback sync/async contracts, ActorConfig/ServeConfig fields, Response::from_parts) against the actual crate source and it all lines up correctly, including the sync-vs-async distinction between configure_message_event_callback (sync, must not block) and configure_close_event_callback (async) in proxy.rs.

Correctness concern: loss of the "one actor per container" guard

The old callbacks.rs::on_actor_start explicitly guarded two failure modes:

  • a duplicate start for the same actor id → idempotent no-op
  • a different actor id landing on the same container → loud bail! referencing the required max_concurrent_actors=1 / Cloud Run concurrency=1 config

The new GameServer::on_start (container-runner/src/actor.rs) has neither check. It unconditionally builds a SpawnSpec and calls ChildProcess::spawn(...) before ever checking children() for a conflict — the only conflict detection left is children().insert_async(...).is_err() after the child process has already been spawned. Two consequences:

  1. A duplicate/retried start for the same actor now spawns a full child process (binding the same port) before detecting the duplicate and killing it, instead of the previous cheap no-op. If the second spawn attempt fails to bind the port (which is likely, since the first child already holds it), ChildProcess::spawn returns Err, and on_start treats that as a poisoning failure and calls request_exit() — tearing down the whole container over what used to be a harmless retry.
  2. There is no longer any defense-in-depth against a different actor landing on the same container instance (e.g. misconfigured pool concurrency). rivetkit-core's serverless runtime has no equivalent single-actor-per-process enforcement (confirmed: no max_concurrent/already running logic in rivetkit-core/src/registry or serverless.rs), so this container-runner-level check was the only guard. Losing it is more than cosmetic here: EXIT/CHILDREN are process-global (main.rs), so one actor's request_exit() (from a stop, crash, or the above spawn-conflict case) now tears down the entire process, which would also kill any second, legitimately co-located actor's child mid-flight.

Worth restoring an explicit check (either "existing entry for this actor id → no-op" or "different actor id already running → bail") before spawning, mirroring the old behavior, given the docs (README.md) still describe this as a strict one-actor-per-container model.

Behavior change: crash reporting to the engine

Previously, an unexpected child exit was reported via handle.stop_actor(actor_id, generation, Some("child exited: ...")), an errored stop that fed the engine's crash policy. The new watchdog (GameServer::run) only calls ctx.destroy(), which — per the PR's own TODO comment — reports a clean stop, because the rivetkit wrapper has no errored-stop API yet. This is called out honestly in-code, but it's a real operational regression: unexpected game-server crashes will now be indistinguishable from graceful actor stops to the engine's crash policy (e.g. auto-restart-on-crash), until that API gap is closed. Worth confirming this is an acceptable interim tradeoff (and maybe tracking the follow-up) before merging, since it changes restart behavior for crashed containers in production.

Minor

  • actor.rs:124 and main.rs:279 use println! for the "child exited unexpectedly" / "stopping N child(ren)" log lines instead of tracing::info!/tracing::warn!. Root CLAUDE.md disallows println!/eprintln! for logging in Rust. (Pre-existing println! calls in the untouched child.rs already violate this, but since this PR is touching/moving these specific lines it'd be a good time to convert them too.)
  • The old inspector shim's manual bearer-token seeding (KV key [0x03]) and Hello-World inspector page are removed with no replacement visible in this diff. Presumably the rivetkit actor runtime now serves the real inspector protocol for registered actors automatically — worth a quick manual check that the dashboard inspector panel still lights up for a container-runner-hosted actor, since previously this was needed specifically because the child game server couldn't answer it itself.
  • http_proxy's body-presence check changed from Option<Vec<u8>>::is_some() to !body.is_empty() (proxy.rs). Functionally fine for a plain reverse proxy, but note it can no longer distinguish "no body sent" from "empty body explicitly sent" — shouldn't matter for this use case, just flagging the semantic narrowing.

Test coverage

input.rs and boot_id.rs inline tests were preserved/moved appropriately, and inspector.rs's tests were correctly deleted along with the module. No new tests cover the on_start duplicate/conflict path called out above — given that's exactly the kind of race this rewrite touches, a regression test (even a unit test around children() insert-conflict handling) would help guard the restored behavior.

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