Skip to content

feat(rivetkit): move HTTP listener to Rust + RIVETKIT_RUNTIME_MODE env var#5132

Open
abcxff wants to merge 2 commits into
mainfrom
06-03-feat_rivetkit_move_http_listener_to_rust_auto-listen_on_prod_start_
Open

feat(rivetkit): move HTTP listener to Rust + RIVETKIT_RUNTIME_MODE env var#5132
abcxff wants to merge 2 commits into
mainfrom
06-03-feat_rivetkit_move_http_listener_to_rust_auto-listen_on_prod_start_

Conversation

@abcxff

@abcxff abcxff commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

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

abcxff commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@railway-app

railway-app Bot commented Jun 3, 2026

Copy link
Copy Markdown

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

Service Status Web Updated (UTC)
kitchen-sink ❌ Build Failed (View Logs) Web Jun 14, 2026 at 3:20 am
website 😴 Sleeping (View Logs) Web Jun 9, 2026 at 4:13 pm
mcp-hub ✅ Success (View Logs) Web Jun 9, 2026 at 4:03 pm
frontend-cloud ❌ Build Failed (View Logs) Web Jun 9, 2026 at 4:02 pm
frontend-inspector ❌ Build Failed (View Logs) Web Jun 9, 2026 at 4:02 pm
ladle ✅ Success (View Logs) Web Jun 3, 2026 at 6:58 pm

@abcxff abcxff force-pushed the 06-03-feat_rivetkit_move_http_listener_to_rust_auto-listen_on_prod_start_ branch from 8945f57 to df0149e Compare June 3, 2026 18:43
@abcxff abcxff force-pushed the 06-03-feat_rivetkit_move_http_listener_to_rust_auto-listen_on_prod_start_ branch from df0149e to f4e0a85 Compare June 3, 2026 18:55
@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

PR Review: feat(rivetkit): move HTTP listener to Rust, auto-listen on prod start()

This is a solid architectural improvement. Moving the HTTP listener to native Rust eliminates the multi-runtime (Node/Bun/Deno) abstraction in serve.ts and unifies serving under axum. The public_dir static-file fallback and PORT env-var integration are well-motivated. A few issues to address before landing:

Medium: 413 error body reports size=0 when Content-Length is absent

In serverless_http.rs forward_request, the .unwrap_or(0) fallback means that when the client omits Content-Length (common with chunked-encoded uploads), the response reports size=0, limit=N which is misleading since the body was clearly larger than N. Since to_bytes fails at exactly body_limit bytes, .unwrap_or(body_limit) is the more accurate fallback.

Low: Silent drops in into_axum_response should log

Both the invalid-header drop and the invalid-status-code fallback to 500 are silent. Both should emit tracing::warn! to make serverless-handler bugs observable at the axum layer.

Low: start() auto-promote has a silent-failure path on non-Node runtimes

In registry/index.ts, if listen() throws (e.g. the WASM runtime explicit error) and process is not available, the error is logged but execution continues silently. The user intended start() to boot an envoy, not fail silently. Consider propagating the error or documenting that production start() is unsupported outside Node.

Low: Test polling loop should use vi.waitFor

waitForResponse in listener.test.ts is a hand-rolled while (Date.now() < deadline) poll. The project convention, enforced by pnpm run check:wait-for-comments, is to use vi.waitFor with an explanatory comment on the immediately preceding line. A custom loop bypasses that check.

Observation: no test coverage for RIVETKIT_AUTO_LISTEN or publicDir

The two headline features have no direct test coverage. A unit test for shouldAutoListen() based on the env var and NODE_ENV would be small and prevent regressions.

Nits

  • shutdown_fut in serverless_http.rs::serve has an unnecessary extra clone() and block; let shutdown_fut = async move { shutdown.cancelled().await }; is sufficient since shutdown moves into the closure.
  • forward_request calls .to_ascii_lowercase() on each header name, but axum normalizes HeaderName to lowercase already -- redundant.

@abcxff abcxff force-pushed the 06-03-feat_rivetkit_move_http_listener_to_rust_auto-listen_on_prod_start_ branch from f4e0a85 to d07e88c Compare June 3, 2026 19:03
@abcxff abcxff force-pushed the 06-03-feat_rivetkit_move_http_listener_to_rust_auto-listen_on_prod_start_ branch from d07e88c to 106d943 Compare June 3, 2026 19:24
@abcxff abcxff force-pushed the 06-03-feat_rivetkit_move_http_listener_to_rust_auto-listen_on_prod_start_ branch from 106d943 to 992bb34 Compare June 3, 2026 19:45
@abcxff abcxff force-pushed the 06-03-feat_rivetkit_move_http_listener_to_rust_auto-listen_on_prod_start_ branch from 992bb34 to cc44f96 Compare June 3, 2026 22:11
@abcxff abcxff requested a review from NathanFlurry June 3, 2026 22:12
@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Review: feat(rivetkit): move HTTP listener to Rust, auto-listen on prod start()

Good overall direction — consolidating the HTTP listener into Rust eliminates the Hono/Node-server cross-platform patchwork and the fragile crossPlatformServe dance. A few items to address before merging, ordered by severity.


CRITICAL: Duplicate shutdown() method — TypeScript error

registry/index.ts now defines public async shutdown() twice: the original at line 576 (with idempotency via #shutdownInFlight, signal handler teardown, and the grace-period-aware #drain) and a new simpler one added by this PR at line 696. TypeScript will reject this as a duplicate identifier. In JS runtime the second definition would silently win, bypassing the grace period and the actor-stop-threshold lookup entirely.

The new method is strictly weaker — #drain already handles the listen() case because listen() sets both #runtimeServeConfiguredPromise and #runtimeServerlessPromise to the same configuredRegistryPromise. The fix is to simply remove the new simple shutdown() added at line 696 and keep the existing one.


HIGH: #printWelcome fires before the port is actually bound

listen() calls #printWelcome (which logs Listening on http://...) before await runtime.serveListener(...) completes. If the bind fails (e.g. port already in use), users see the "Listening" line immediately before the error. Rust does log after a successful bind (tracing::info!(..., "rivetkit server listening")), but the JS-level welcome precedes that. Consider moving the welcome print until after serveListener resolves, or omitting the port from the pre-bind welcome.

HIGH: Documented RIVETKIT_AUTO_LISTEN env var is not implemented

The JSDoc on start() says: The RIVETKIT_AUTO_LISTEN env var overrides the heuristic: 1 forces auto-listen on, 0 forces it off. But the implementation only reads RIVETKIT_RUNTIME_MODE via getRivetkitRuntimeMode(). RIVETKIT_AUTO_LISTEN is never checked. Either implement the override or remove the documentation.


MEDIUM: Double shutdownRegistry call when listen() is used

listen() sets both #runtimeServeConfiguredPromise and #runtimeServerlessPromise to the same promise. #drain then fans out to both, calling runtime.shutdownRegistry twice on the same registry. The inner try/catch swallows the second call's failure, so this works today only if shutdownRegistry is idempotent. Worth adding a dedup guard (e.g., check if both fields point to the same reference before pushing into registries) to make the invariant explicit rather than accidental.

MEDIUM: Removed request body size from error message

IncomingMessageTooLong previously reported the received byte count alongside the limit. The error message was tightened to just "Exceeded limit of {limit} bytes." because axum::body::to_bytes does not expose how many bytes arrived before truncating. This is a reasonable tradeoff but worth documenting — clients debugging 413s no longer see how far over-limit they were.


LOW: body_bytes.to_vec() copies the buffer unnecessarily

In serverless_http.rs: axum::body::to_bytes returns Bytes, and Bytes::to_vec allocates a new Vec. If ServerlessRequest.body were changed to accept Bytes this copy could be avoided. Minor perf concern, no correctness issue.

LOW: is_length_limit_error walks the error source chain — valid pattern but depends on axum's internal chaining of LengthLimitError. Axum does not guarantee this chain is stable across versions. Not a blocker.

LOW: waitForResponse in tests uses a polling loop — Per conventions, vi.waitFor is preferred over hand-rolled Date.now() + setTimeout polling for test coordination, and requires a justification comment. The existing comment explains why polling is necessary (the listener takes a moment to bind), which is the right intent — just worth migrating the form.


Tests

Test coverage of the new units is solid (getRivetkitRuntimeMode, parsePortEnv, and the live listener e2e cases). Two gaps worth tracking: (1) no test verifies that RIVETKIT_RUNTIME_MODE=serverless causes start() to invoke listen(); (2) the ServeDir + fallback wiring in serverless_http.rs has no test for publicDir static-file serving.


Overall the Rust Axum listener is cleanly structured — the CancelOnDropStream guard, graceful-shutdown wiring, and header comma-folding per RFC 9110 §5.3 are all correct. The duplicate shutdown() is the blocking issue.

@abcxff abcxff force-pushed the 06-03-feat_rivetkit_move_http_listener_to_rust_auto-listen_on_prod_start_ branch from cc44f96 to 8331535 Compare June 3, 2026 22:34
@abcxff abcxff force-pushed the 06-03-feat_rivetkit_move_http_listener_to_rust_auto-listen_on_prod_start_ branch from 8331535 to 8086664 Compare June 3, 2026 22:43
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5132 June 3, 2026 22:43 Destroyed
@abcxff abcxff marked this pull request as ready for review June 3, 2026 22:47
@abcxff abcxff force-pushed the 06-03-feat_rivetkit_move_http_listener_to_rust_auto-listen_on_prod_start_ branch from 8086664 to 21b19a1 Compare June 3, 2026 23:08
@abcxff abcxff force-pushed the 06-03-feat_rivetkit_move_http_listener_to_rust_auto-listen_on_prod_start_ branch from 21b19a1 to 6df2c7f Compare June 9, 2026 15:59
@claude

claude Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Code Review: feat(rivetkit): move HTTP listener to Rust + RIVETKIT_RUNTIME_MODE env var

Overview

This PR replaces the Hono/@hono/node-server TypeScript HTTP listener with an Axum-based Rust listener in rivetkit-core. It also introduces a RIVETKIT_RUNTIME_MODE env var that lets registry.start() auto-switch between envoy mode (Mode A) and serverless listener mode (Mode B). The direction is solid: moving the listener to Rust removes Node-runtime coupling, eliminates cross-platform adapter complexity, and simplifies deployment. The CancelOnDrop/CancelOnDropStream mechanism for client-abort signaling is well-designed — request_token.clone() is correctly placed in ServerlessRequest.cancel_token, so dropping the response body stream cancels the handler.


Bugs / Correctness

Welcome message prints before the listener is bound

In registry/index.ts::listen(), #printWelcome logs "Listening at http://host:port" before runtime.serveListener has bound the socket. If the bind fails (e.g. port already in use), the user sees a success banner followed by a crash. The Rust layer already logs at tracing::info! after TcpListener::bind succeeds; consider either moving #printWelcome after await runtime.serveListener(...) resolves (not possible since it blocks until shutdown), or signaling port readiness back through the NAPI layer.

start() serverless path defaults publicDir to /public unconditionally

If the container has no /public directory, ServeDir::new("/public") is still configured. tower-http's ServeDir with a fallback won't crash, but it adds a stat attempt on every request before falling through to forward_service. If this is an intentional deployment convention, document it in the env-vars docs entry. If not, passing publicDir: undefined when getRivetkitPublicDir() returns undefined skips the ServeDir layer entirely.


Design / Architecture

Removing size from IncomingMessageTooLong reduces error diagnostics

The error changed from "Received {size} bytes, limit is {limit} bytes" to "Exceeded limit of {limit} bytes." The received size is often the most actionable field when debugging an oversized request. This is a regression in observability.

parsePortEnv error message hardcodes RIVET_PORT

The function takes an opaque raw: string | undefined but the env-var name is baked into the error string. If the helper is reused for another port variable, the error message will be misleading. Consider accepting the variable name as a second parameter: parsePortEnv(raw, varName = "RIVET_PORT").

Silent header drops in into_axum_response

Invalid response headers are silently discarded in the into_axum_response conversion loop. Per project logging conventions, a tracing::warn! on the Err branch would surface header encoding bugs that are otherwise invisible.

Double-promise caching comment in listen() is too terse

The comment "Cache on both promise fields so the shutdown drain sees Mode A and B" explains the intent but not the contract: which shutdown path reads each field, and what breaks if they diverge. A link to the shutdown drain logic or one more sentence of context would prevent a future reader from treating the dual-assignment as a copy-paste error.


Testing

waitForResponse polling loop lacks call-site justification comment

CLAUDE.md requires a one-line // comment immediately before each polling call explaining why direct awaiting is not possible. The JSDoc on waitForResponse partially covers this, but the per-call-site comment is what pnpm run check:wait-for-comments enforces.

No Rust unit tests for serverless_http.rs

The new module has no coverage under tests/. At minimum, is_length_limit_error and the header-joining logic are unit-testable without a full HTTP round-trip and would guard against Axum version bumps silently changing the error-source chain.

Duplicate registry setup across E2E tests

The two registry.listen() tests share identical setup({...}) calls. A makeTestRegistry(port) helper would reduce duplication.


Minor / Nits

  • CancelOnDropStream doesn't delegate size_hint() from the inner stream — callers that inspect the hint see (0, None).
  • is_length_limit_error traverses the error source chain dynamically; a comment noting the dependency on Axum's internal error-wrapping contract would help future maintainers.
  • The serve_listener NAPI method correctly uses u32 for port with explicit try_into() bounds checking — well-handled.
  • Docs table for RIVET_PORT correctly states "Defaults to 3000", matching the code fallback chain.

Summary

The architectural move to Rust+Axum is well-structured, the new env-var API is clean, and the client-abort signaling via CancelOnDrop is correct. The main issues to address before merge: the premature welcome banner, the unconditional /public default overhead in start(), and the silent header drop without logging. The IncomingMessageTooLong diagnostic regression and the test polling-comment requirement are also worth fixing.

@abcxff abcxff changed the title feat(rivetkit): move HTTP listener to Rust, auto-listen on prod start() feat(rivetkit): move HTTP listener to Rust, RIVETKIT_RUNTIME_MODE env var, listener hardening Jun 9, 2026
@abcxff abcxff changed the title feat(rivetkit): move HTTP listener to Rust, RIVETKIT_RUNTIME_MODE env var, listener hardening feat(rivetkit): move HTTP listener to Rust + RIVETKIT_RUNTIME_MODE env var Jun 9, 2026
@abcxff abcxff force-pushed the 06-03-feat_rivetkit_move_http_listener_to_rust_auto-listen_on_prod_start_ branch from 89abcb3 to 2e5c4ef Compare June 12, 2026 11:08
@abcxff abcxff force-pushed the 06-03-feat_rivetkit_move_http_listener_to_rust_auto-listen_on_prod_start_ branch from 2e5c4ef to 66ba4c8 Compare June 12, 2026 11:13

abcxff commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

Merge activity

  • Jun 13, 12:31 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jun 13, 12:32 AM UTC: Graphite couldn't merge this PR because it had merge conflicts.

@abcxff abcxff force-pushed the 06-03-feat_rivetkit_move_http_listener_to_rust_auto-listen_on_prod_start_ branch from 66ba4c8 to d390612 Compare June 14, 2026 03:13
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