Skip to content

revert(http): return main to Envoy protocol v6 - #5515

Merged
NathanFlurry merged 1 commit into
mainfrom
stack/revert-http-return-main-to-envoy-protocol-v6-qznqnzvy
Jul 30, 2026
Merged

revert(http): return main to Envoy protocol v6#5515
NathanFlurry merged 1 commit into
mainfrom
stack/revert-http-return-main-to-envoy-protocol-v6-qznqnzvy

Conversation

@NathanFlurry

@NathanFlurry NathanFlurry commented Jul 30, 2026

Copy link
Copy Markdown
Member
  • Remove the end-to-end HTTP/SSE streaming changes and their dependent Flue streaming integration from main.
  • Return the Envoy wire protocol to v6 while the work is reviewed as a three-PR stack.

@NathanFlurry

Copy link
Copy Markdown
Member Author

Stack for rivet-dev/rivet

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

change qznqnzvy

@railway-app

railway-app Bot commented Jul 30, 2026

Copy link
Copy Markdown

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

Service Status Web Updated (UTC)
frontend-cloud 🕒 Building (View Logs) Web Jul 30, 2026 at 8:59 pm
website 🕒 Building (View Logs) Web Jul 30, 2026 at 8:59 pm
frontend-inspector 🕒 Building (View Logs) Web Jul 30, 2026 at 8:59 pm
ladle 🕒 Building (View Logs) Web Jul 30, 2026 at 8:59 pm
mcp-hub 🕒 Building (View Logs) Web Jul 30, 2026 at 8:59 pm
kitchen-sink 🕒 Building (View Logs) Web Jul 30, 2026 at 8:59 pm

@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5515 July 30, 2026 20:59 Destroyed
@NathanFlurry
NathanFlurry merged commit 990124a into main Jul 30, 2026
4 of 17 checks passed
@NathanFlurry
NathanFlurry deleted the stack/revert-http-return-main-to-envoy-protocol-v6-qznqnzvy branch July 30, 2026 20:59
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review

This reverts the HTTP/SSE streaming feature and drops the Envoy protocol back to v6. Most of the ~100-file diff is a clean, mechanical revert (version negotiation in envoy-protocol/src/versioned/mod.rs, the tunnel/gateway plumbing, and the integrations/flue-runtime downgrade all check out consistently). However, I found compile-breaking dangling references and one behavioral regression that should block merge until fixed.

🔴 Compile-breaking: rivetkit-core re-exports two types that no longer exist

rivetkit-rust/packages/rivetkit-core/src/lib.rs:146-148:

pub use rivet_envoy_client::config::{
	HTTP_BODY_STREAM_CHANNEL_CAPACITY, HttpRequestBodyStream, ResponseChunk,
};

Neither HttpRequestBodyStream nor HTTP_BODY_STREAM_CHANNEL_CAPACITY are defined anywhere in engine/sdks/rust/envoy-client/src/ (only ResponseChunk survived the consolidation of callbacks.rs/http.rs/websocket.rs back into config.rs). This is an unresolved import — cargo check -p rivetkit-core should fail outright, which would break every downstream crate.

This cascades into two more dangling references that only make sense if the above is fixed first:

  • rivetkit-typescript/packages/rivetkit-wasm/src/lib.rs:19,816 imports and calls rivetkit_core::ActorHttpResponse, which isn't defined anywhere in the repo anymore (grep -rl ActorHttpResponse --include=*.rs . only matches this one file).
  • rivetkit-typescript/packages/rivetkit-napi/tests/actor_factory.rs:10,15,134,136 (the moved_tests module) still references rivetkit_core::HttpRequestBodyStream and a local NAPI HttpRequestBodyStream/parse_bridge_rivet_error that were only defined in the deleted src/http.rs. This looks like a leftover chunk from the streaming branch that didn't get removed along with tests/http.rs.

Please double check CI actually ran a full cargo check/cargo test across these crates before merging — none of these three would be caught by a partial check.

🟠 Likely regression: in-flight request counter is released twice for actor (CustomServe) HTTP requests

engine/packages/guard-core/src/proxy_service.rs:

  • The outer handle_request wrapper (lines ~746-761) unconditionally spawns a release_in_flight(client_ip, in_flight_request_id) task once res is available, for both the WebSocket and HTTP paths. This matches the pre-streaming v6 behavior and is fine on its own.
  • But the revert also adds two new self.state.release_in_flight(...) calls inside handle_http_request's ResolveRouteOutput::CustomServe branch (lines 1007 and 1015, both pure additions with no corresponding removal in the diff) — one before the successful return res, one before the retry-exhausted error return.

Since InFlightCounter::release() (engine/packages/guard-core/src/utils.rs:42) is a bare saturating_sub(1) with no idempotency guard, every actor request routed through CustomServe (i.e. virtually all Rivet Actor traffic via PegboardGateway2) now decrements the per-IP in-flight counter twice for one acquire_in_flight increment. That silently under-counts concurrent in-flight requests per client IP, letting more concurrent requests through than max_in_flight allows — a rate-limiter bypass. Note the plain reverse-proxy path (ResolveRouteOutput::Target) did not get this addition, so the asymmetry itself is a signal this is unintentional rather than a deliberate design change.

Suggest removing the two new release_in_flight calls in the CustomServe branch and relying solely on the outer wrapper's unconditional release, consistent with the Target branch.

Minor / style

  • integrations/flue-runtime/src/target.ts:14'@rivet-dev/flue' in the external array picked up an extra tab of indentation vs. its siblings (hard-tab convention violation, likely rebase artifact).
  • integrations/flue-runtime/test/rivet-target.test.ts:88-91 — a catch block and its closing braces are de-indented one tab relative to the enclosing try/catch. Cosmetic, but worth a formatter pass.
  • rivetkit-rust/packages/rivetkit-core/src/registry/mod.rs:43Response was added to a use list; I didn't find another reference to it in the file, so it may trigger an unused-import warning under -D warnings.
  • engine/artifacts/config-schema.json lost its trailing newline (cosmetic).

What looks good

  • envoy-protocol version negotiation (versioned/mod.rs, lib.rs) cleanly drops all V7/HttpStream* variants and match arms with no leftover references; the TS envoy-protocol VERSION constant was reverted 7→6 in lockstep with the Rust side, consistent with the PROTOCOL_VERSION pairing convention in CLAUDE.md.
  • pegboard-gateway2's lib.rs/metrics_task.rs restructuring (inlining the deleted request_metrics.rs into a Metric enum) preserves the same metric names/bounded labels (namespace_id, actor name, kind) — no metrics-convention violations.
  • Deleted test files (response_body.rs, *_payload_accounting.rs, http_stream_*, versioned_http_abort.rs, actor-fetch-retry.test.ts, streaming-http.test.ts, sse-contract-harness.ts) all correspond to functionality that's actually gone — no coverage gap for surviving v6 code paths.
  • integrations/flue-runtime downgrade (-rivet.2-rivet.1) is consistent across package.json, pnpm-lock.yaml, and the transitive @earendil-works/pi-ai pin; remaining tests in rivet-target.test.ts and rivet-store-contracts.test.ts still assert real behavior after the streaming-specific cases were removed.

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