Skip to content

ci(publish): build and publish container runner release artifacts#5393

Merged
abcxff merged 2 commits into
mainfrom
stack/ci-publish-build-and-publish-container-runner-release-artifacts-lysuzroz
Jul 13, 2026
Merged

ci(publish): build and publish container runner release artifacts#5393
abcxff merged 2 commits into
mainfrom
stack/ci-publish-build-and-publish-container-runner-release-artifacts-lysuzroz

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

@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Review

This PR adds the rivet-container-runner crate (a serverless runner that spawns a child game-server process and proxies Rivet's tunneled HTTP/WebSocket traffic to it, Cloud Run 1:1 model) plus a CI publish target for it. Most of the 1886 changed files are the vendored container-runner/examples/unity-demo FishNet/Unity package, which I did not review in detail (third-party vendored code, not hand-written for this PR). This review focuses on the new container-runner/src/* crate, the e2e/CI wiring, and the Docker/publish changes.

Overview

  • main.rs - CLI parsing, signal handling, wires the serverless runtime + callbacks together.
  • child.rs - spawns/reaps the child process, pumps logs, waits for readiness, graceful SIGTERM to SIGKILL stop.
  • callbacks.rs - implements EnvoyCallbacks; enforces the one-actor-per-container invariant, wires start/stop/fetch/websocket to the child.
  • serverless.rs - the /api/rivet/start SSE front door (a slimmed reimplementation of rivetkit-core's serverless HTTP handling, since CoreServerlessRuntime is hard-coded to the in-process actor registry).
  • proxy.rs - HTTP/WS bridging to the child's local port.
  • inspector.rs - minimal custom inspector UI + KV inspector-token bootstrap.
  • input.rs - decodes the opaque ActorConfig.input JSON into a launch spec.

Overall this is a well-documented, carefully-reasoned crate. The module doc comments explain several non-obvious races/ordering constraints (reaper-task ownership of Child, port-reuse guard before spawn, v:1 inspector handshake quirk, SSE-drop-triggers-exit flow) that would otherwise be easy to regress.

Code quality / convention notes

  • Arc<TokioMutex<HashMap<...>>> in callbacks.rs (ContainerRunnerCallbacks::actors) goes against the repo-wide convention in CLAUDE.md: "Never use Mutex<HashMap<...>> ... Use scc::HashMap (preferred) ... for concurrent maps," which is followed elsewhere (depot-client, pegboard-runner, cache, universalpubsub). Given this container only ever holds 0-1 actor entries in practice, contention isn't a real concern, but it's worth either switching to scc::HashMap for consistency or noting the exemption inline given the single-actor design.
  • proxy::http_proxy creates a new reqwest::Client::new() per request (container-runner/src/proxy.rs:35). This runs on the hot path for every proxied HTTP request and throws away connection pooling/keep-alive to the child each time. Every other reqwest::Client::new() call in the repo is in test/CLI one-shot code; this is the only per-request instantiation. Worth hoisting into a OnceLock<reqwest::Client> or building it once in ContainerRunnerCallbacks/ServerlessRuntime and threading it through.

Potential issues

  • ActorInput uses #[serde(deny_unknown_fields)] (input.rs). If a newer client/tooling starts sending additional optional fields in the input JSON, older deployed container-runner binaries will hard-fail the actor start instead of ignoring the unknown field. Given the emphasis elsewhere in this codebase on forward/backward protocol compatibility, it might be worth relaxing this (or being deliberate that this is an intentionally strict, versioned-together boundary since users control both the runner binary and the input payload).
  • Stale map entries for a different, already-exited actor id in ContainerRunnerCallbacks::on_actor_start: when the "different actor already running" guard passes because the existing entry has_exited(), that stale entry is never removed from actors, only the new actor id gets inserted later. Harmless in the 1:1 Cloud Run model (the container exits right after request_exit), but worth a one-line cleanup (guard.retain(|_, c| !c.has_exited())) for robustness if the container-reuse assumption ever loosens.
  • ChildProcess::stop signals by raw pid (nix::sys::signal::kill(Pid::from_raw(self.pid as i32), ...)) after checking has_exited(). There's a narrow TOCTOU window between that check and the kill call where the reaper task could observe exit and the OS could theoretically recycle the pid before the signal is sent. Very low practical risk (reaper's wait() and signal delivery happen essentially back-to-back), but worth a comment acknowledging it given how carefully the rest of the file documents its ordering guarantees.
  • /api/rivet/start has no auth check of its own beyond forwarding whatever x-rivet-endpoint/x-rivet-token headers the caller supplies to start_envoy. This mirrors the upstream rivetkit-core design per the module comment, but since this is a new standalone binary/network surface, it's worth confirming the front-door port is never reachable from outside the trusted Cloud Run to engine path (an attacker who can reach it could point x-rivet-endpoint at an arbitrary host, causing this container to make outbound calls there).

Test coverage

  • container-runner/tests/inline/inspector.rs has solid unit coverage for the inspector path-matching, HTML handshake contract, and base64 encoder.
  • child.rs, callbacks.rs, serverless.rs, proxy.rs, and input.rs (the process-supervision, lifecycle-race, and proxying logic) have no unit tests. The container-runner/examples/e2e-test/host/run-host-tests.sh script exercises a lot of this (crash handling, orphan-process checks, HTTP passthrough) but isn't wired into any CI workflow (searching for e2e-test/run-host in .github/workflows/*.yaml finds nothing), so these scenarios currently only run when a human remembers to invoke the script manually.
  • Per CLAUDE.md, Rust tests should live under tests/, not inline #[cfg(test)] mod tests in src/. This is followed correctly here via the #[path = "../tests/inline/inspector.rs"] mod tests; shim in inspector.rs.

CI / Docker

  • publish.yaml, docker/build/linux-x64-musl.Dockerfile, docker/build/linux-arm64-musl.Dockerfile, and Dockerfile.release changes are small, consistent with the existing rivet-cli build pattern, and correctly gated to musl static builds (release_only: false so it builds on every push, matching the "users curl this binary" use case described in the README).
  • .gitignore/.dockerignore additions correctly exclude e2e local state and Unity editor artifacts while keeping the Unity Linux server build (needed for Dockerfile.unity), and explicitly call out that .last-cloud-* files holding gateway tokens must never be committed, which is a good catch.

Nice addition overall. The invariants around single-actor-per-container are enforced defensively in multiple places (port-reuse guard, duplicate-start guard, cross-actor guard) rather than just assumed, which is exactly the kind of defensive design this component needs given it's the untrusted-boundary-facing piece.

@abcxff abcxff force-pushed the stack/ci-publish-build-and-publish-container-runner-release-artifacts-lysuzroz branch from d123997 to 1a80bdd Compare July 9, 2026 22:00
@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
@abcxff abcxff requested a review from NathanFlurry July 9, 2026 23:06
@abcxff abcxff force-pushed the stack/ci-publish-build-and-publish-container-runner-release-artifacts-lysuzroz branch from 1a80bdd to f1606eb Compare July 13, 2026 18:54
@abcxff abcxff force-pushed the stack/feat-container-runner-add-container-runner-crate-and-e2e-harness-tmuruzkr branch from 081809c to be744d4 Compare July 13, 2026 18:54
@abcxff abcxff changed the base branch from stack/feat-container-runner-add-container-runner-crate-and-e2e-harness-tmuruzkr to main July 13, 2026 18:56
@abcxff abcxff merged commit f1606eb into main Jul 13, 2026
9 of 12 checks passed
@abcxff abcxff deleted the stack/ci-publish-build-and-publish-container-runner-release-artifacts-lysuzroz branch July 13, 2026 19:24
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