Skip to content

feat(container-runner): add container runner crate and e2e harness#5391

Open
abcxff wants to merge 1 commit into
mainfrom
stack/feat-container-runner-add-container-runner-crate-and-e2e-harness-tmuruzkr
Open

feat(container-runner): add container runner crate and e2e harness#5391
abcxff wants to merge 1 commit into
mainfrom
stack/feat-container-runner-add-container-runner-crate-and-e2e-harness-tmuruzkr

Conversation

@abcxff

@abcxff abcxff commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@abcxff

abcxff commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@abcxff abcxff force-pushed the stack/feat-container-runner-add-container-runner-crate-and-e2e-harness-tmuruzkr branch from a8b9943 to 081809c Compare July 9, 2026 22:00
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Review

This PR adds a new container-runner crate (serverless runner that spawns a child game-server process and proxies Rivet tunnel traffic to it) plus a large e2e harness. Scope note: of the ~1880 changed files, ~1840 are the vendored examples/unity-demo/ Unity project (including the third-party FishNet/Bayou networking library and its own LICENSE.txt). I focused this review on the actual crate source (container-runner/src/*.rs), Dockerfiles, and e2e scripts, since the Unity assets aren't really reviewable as code.

Correctness / bugs

  • src/proxy.rs::http_proxy discards HttpRequest.body_stream entirely (body_stream: _) and only forwards body. Per engine-runner's handle_req_start (engine/sdks/rust/envoy-client/src/actor.rs:531), when a request is created with stream: true, body is None and the data instead arrives via body_stream. Any streamed request body proxied through this runner is silently forwarded as empty instead of erroring or actually streaming. Response bodies have the same limitation in reverse: the proxy always fully buffers the child's response into body and never populates HttpResponse.body_stream, so large or long-lived (e.g. SSE) responses from the child are buffered in memory rather than streamed back.
  • src/child.rs::spawn: .env("PORT", child_port.to_string()).envs(&env) — since env (attacker/client-controlled via ActorInput, see below) is applied after the PORT var, a caller can override PORT in input.env, causing the child to bind somewhere other than child_port while the proxy still targets child_port, breaking the proxy silently (looks like a plain start failure rather than an obvious misconfiguration).
  • src/child.rs::stop: has_exited() is checked and then signal::kill(Pid::from_raw(self.pid...)) is issued as a separate step. If the child has already exited and its PID gets recycled by the OS between the check and the kill, the signal could hit an unrelated process. Low probability given one-actor-per-container, but worth a comment at least if intentionally accepted.
  • src/callbacks.rs::on_actor_start: the "one actor per container" guard is check-then-act but not atomic. The actors lock is held only for the initial lookup/bail check, then dropped before ActorInput::parse and ChildProcess::spawn(...) run (spawn includes the readiness wait, up to readiness_timeout_secs, default 30s) — the map isn't updated with the new entry until actors.lock().await.insert(...) at the very end. If on_actor_start is invoked concurrently for two different actor ids (e.g. an engine retry racing a real second start, or Cloud Run concurrency misconfigured above 1), both calls can pass the "no other actor running" check before either inserts, so the explicit bail!("container-runner hosts one actor per container...") diagnostic doesn't actually fire. In practice this is partially caught downstream: if both children target the same child_port, ChildProcess::spawn's pre-bind check ("child port already in use") still rejects the second one, just with a less clear message. But if the two starts specify different input.port values, both children spawn successfully and run concurrently, silently violating the documented one-actor-per-container invariant with no error at all.

Security

  • src/input.rs (ActorInput) + src/callbacks.rs::on_actor_start: ActorConfig.input is client-supplied JSON (see examples/e2e-test/create-actor.mjs, which POSTs input with no auth against local dev). ActorInput.command fully overrides the CLI-configured program/args, and ActorInput.env is passed straight to Command::envs(). Since Command::new doesn't invoke a shell, this isn't classic shell injection, but it does mean whoever can create an actor against this runner (e.g. a namespace's public-token holder, if this follows the usual actor-creation trust model) can execute an arbitrary binary and arbitrary environment (LD_PRELOAD, PATH, etc.) inside the container. That may be intentional for this "bring your own game server image" design, but CLAUDE.md's trust-boundary rule ("Validate and authorize all client-originated data at the engine edge") suggests this should be an explicit, documented decision rather than implicit — worth confirming with the team whether input.command/input.env need to be gated behind a more privileged caller than plain actor creation, and documenting the resulting trust model in README.md.
  • Dockerfiles (Dockerfile, Dockerfile.pingpong, Dockerfile.unity) run the child process as root (no USER directive) and pull floating base tags (rust:1-bookworm, node:20-slim). These are e2e/example images rather than the documented production pattern (curl a released binary into the user's own image), so lower priority, but worth a non-root user at least in the example Dockerfiles.

Style / CLAUDE.md conventions

  • src/child.rs and src/callbacks.rs use println!/eprintln! with string-interpolated messages for the runner's own lifecycle logging (spawn, SIGTERM/SIGKILL, unexpected exit, shutdown — e.g. child.rs:100,111,145,188,193,196,202,216,219, callbacks.rs:56,84,153). CLAUDE.md requires tracing::info!/warn!/error! with structured fields instead of println!/eprintln! and instead of formatting parameters into the message string. The one legitimate exception is the raw child stdout/stderr passthrough in spawn_log_pump (that's forwarding data, not logging), but the runner's own status lines should go through tracing.
  • src/child.rs::spawn_log_pump has no bound on line length — BufReader::lines() will grow an unbounded buffer if the child (or an attacker via input.env/input.command) writes an arbitrarily long line with no newline. Probably fine in practice but worth a sanity cap given this proxies untrusted-ish child output.
  • src/proxy.rs::http_proxy builds a fresh reqwest::Client::new() per request instead of constructing one client once and reusing it (loses connection pooling/keep-alive to the child on every proxied request).

Architecture

  • src/serverless.rs's own doc comment calls it "a slimmed re-implementation of rivetkit-core's serverless.rs + serverless_http.rs". Given CLAUDE.md directs all new actor-hosting work to pegboard-envoy and flags the runner-parity requirement between engine/sdks/typescript/runner and engine/sdks/rust/engine-runner, it'd be good to confirm with maintainers that a third, independent reimplementation of the serverless front door here is intentional rather than something that should build on existing runner infrastructure.

Test coverage

  • Only src/inspector.rs has unit tests (tests/inline/inspector.rs, pure functions: path matching, HTML content, base64 encoding). The lifecycle-critical paths — ChildProcess::spawn/stop/wait_until_ready in child.rs and the duplicate-start/different-actor guard in callbacks.rs::on_actor_start (including the race described above) — have no test coverage at all. These are exactly the areas most likely to have subtle races; worth at least an integration test that exercises concurrent on_actor_start calls.

Nits

  • container-runner/examples/unity-demo/ vendors FishNet/Bayou/SimpleWebTransport as raw source directly in the repo (not a package reference or submodule), which is ~1500+ files and a large one-time repo bloat. Given CLAUDE.md says "Do not use Git LFS ... jj can snapshot raw working-tree bytes" this is presumably intentional, but it's worth double-checking license compliance tracking for the vendored third-party code (it does carry its own LICENSE.txt, which is good) and whether this belongs in a separate examples repo instead of the main monorepo.
  • .gitignore/.dockerignore additions correctly exclude .last-cloud-* files that hold gateway tokens — good catch already made in the PR itself.
  • tests/inline/inspector.rs::metadata_reports_version_at_or_above_2_3_0 is named "at or above" but asserts an exact string match ("version":"2.3.0"), so it doesn't actually test the "at or above" claim in its name — either rename it or make the assertion a real version comparison.

Overall the core runner logic (signal handling, reaper task ownership to avoid deadlocking on Child::wait(), idempotent start handling, watchdog reporting unexpected exits) is solid and thoughtfully commented. The main things worth resolving before merge are the streaming-body gap in proxy.rs, the on_actor_start TOCTOU race, and getting an explicit answer on the input.command/input.env trust model.

@abcxff abcxff requested a review from NathanFlurry July 9, 2026 23:06
@railway-app

railway-app Bot commented Jul 9, 2026

Copy link
Copy Markdown

🚅 Deployed to the rivet-pr-5391 environment in rivet-frontend

Service Status Web Updated (UTC)
kitchen-sink 😴 Sleeping (View Logs) Web Jul 9, 2026 at 10:09 pm
frontend-cloud 😴 Sleeping (View Logs) Web Jul 9, 2026 at 10:04 pm
frontend-inspector 😴 Sleeping (View Logs) Web Jul 9, 2026 at 10:04 pm
website 😴 Sleeping (View Logs) Web Jul 9, 2026 at 9:59 pm
mcp-hub ✅ Success (View Logs) Web Jul 9, 2026 at 9:54 pm
ladle ✅ Success (View Logs) Web Jul 9, 2026 at 9:54 pm

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