Skip to content

fix(http): harden streaming lifecycle and observability - #5509

Open
NathanFlurry wants to merge 1 commit into
stack/feat-http-stream-http-bodies-end-to-end-npntkwqxfrom
stack/fix-http-harden-streaming-lifecycle-and-observability-rkrtrmpn
Open

fix(http): harden streaming lifecycle and observability#5509
NathanFlurry wants to merge 1 commit into
stack/feat-http-stream-http-bodies-end-to-end-npntkwqxfrom
stack/fix-http-harden-streaming-lifecycle-and-observability-rkrtrmpn

Conversation

@NathanFlurry

@NathanFlurry NathanFlurry commented Jul 30, 2026

Copy link
Copy Markdown
Member
  • Harden streaming cancellation, response lifecycle, and shutdown behavior.
  • Add Envoy protocol v8 abort compatibility and streaming observability.

@NathanFlurry

NathanFlurry commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

Stack for rivet-dev/rivet

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

change rkrtrmpn

@railway-app

railway-app Bot commented Jul 30, 2026

Copy link
Copy Markdown

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

Service Status Web Updated (UTC)
frontend-cloud 😴 Sleeping (View Logs) Web Jul 30, 2026 at 8:09 pm
kitchen-sink 😴 Sleeping (View Logs) Web Jul 30, 2026 at 8:09 pm
website 😴 Sleeping (View Logs) Web Jul 30, 2026 at 8:08 pm
frontend-inspector 😴 Sleeping (View Logs) Web Jul 30, 2026 at 8:06 pm
ladle ✅ Success (View Logs) Web Jul 30, 2026 at 7:59 pm
mcp-hub ✅ Success (View Logs) Web Jul 30, 2026 at 7:59 pm

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review: fix(http) harden streaming lifecycle and observability

Overview

This PR (1) tracks exact ingress/egress bytes via AtomicU64 plus a shared metrics_task instead of size_hint() estimation, (2) bumps the envoy wire protocol to v8, (3) simplifies HTTP request-body cancellation on the NAPI/TS side, and (4) adds golden-byte cross-language tests for the abort protocol. Direction is solid overall. Re-reviewed against the current head commit (74db67c8f) and dug further into the cancellation-token wiring with a dedicated trace; details below refine an earlier pass.

Potential issues

  1. v8.bare is byte-for-byte identical to v7.bare, is the version bump needed?
    Diffing engine/sdks/rust/envoy-protocol/schemas/v7.bare against v8.bare shows no difference. Yet the PR adds v7_to_v8.rs/v8_to_v7.rs (1200+ lines each) with full field-by-field converters for every message type, bumps PROTOCOL_VERSION/VERSION to 8 in both SDKs, and re-points ToEnvoy/ToRivet/etc. Latest to v8. The PR description says "Add Envoy protocol v8 abort compatibility," but HttpStreamAbortReason/detail already exist in v7 (per the v6_request_abort_deserializes_with_unknown_reason test, aborts go back to v6), so nothing in v8's schema differs from v7. The new golden-byte tests (http-abort-golden.test.ts, request_abort_matches_cross_language_golden_bytes) exercise v8 types but would pass identically against v7 types too, since the bytes are the same. Worth confirming: is this bump intentional infrastructure for a feature-negotiation signal (e.g. gating on PROTOCOL_VERSION >= 8 somewhere downstream to know a peer handles aborts correctly), or should this PR just add the golden tests against v7 and drop the v8 plumbing? As-is it's a large amount of generated boilerplate for zero wire-format change.

  2. Test coverage regression: overload/backpressure abort test deleted with no replacement
    engine/sdks/rust/envoy-client/tests/support/actor_http_stream.rs deletes streamed_request_backpressure_does_not_block_actor_control_loop (verifying an overloaded streamed-request body gets aborted with HttpStreamAbortReasonKind::Overloaded and doesn't block the actor control loop), along with its now-unused imports (wait_for_stopped_event, HTTP_BODY_STREAM_CHANNEL_CAPACITY). No other test in envoy-client/tests exercises the Overloaded abort path or reject_pending_request (the helper this PR itself extracts in actor/http.rs). The underlying logic in http.rs is untouched, just refactored, so this looks like a straight coverage loss rather than a deliberate behavior removal. Please restore an equivalent test or explain why it no longer applies.

  3. call_http_request no longer combines the dispatch-timeout cancel token with the request's own cancel token
    In napi_actor_events.rs, ActorEvent::HttpRequest dispatch used to wrap the call in with_dispatch_cancel_token(...), whose DispatchCancelGuard::Drop cancels a token that's combined with request.cancellation_token(). ActorEvent::Action and ActorEvent::QueueSend still do this; only the HTTP path was changed to pass Some(request.cancellation_token()) directly.
    I traced where request.cancellation_token() actually gets cancelled in production (the pegboard-envoy/envoy-client tunnel path, not just the serverless path): rivetkit-rust/packages/rivetkit-core/src/registry/http.rs's RequestCancellationGuard cancels it on drop, and that guard/stream lives through handle_fetch to dispatcher.handle_fetch, wired from envoy_callbacks.rs. So client disconnect and actor shutdown are still covered: envoy-client's abort_all_tasks() on actor stop, and RequestCancellationGuard's drop on early reply/disconnect, both independently cancel this token without needing the dispatch-abort combination. That part of the removal looks safe.
    However, call_http_request(...) is still wrapped in with_structured_timeout(..., timeout, call_http_request(...)), which is tokio::time::timeout(duration, future). When the timeout elapses, tokio::time::timeout drops the inner future rather than letting it run to completion. Previously that drop cancelled the dispatch guard's token (combined into the JS AbortSignal), so a config.on_request_timeout timeout would actually tell the in-flight JS handler to stop. Now that guard is gone from the HTTP path, so dropping the future on a dispatch-level timeout no longer signals request.cancellation_token() at all (nothing else cancels that specific token on this code path). A stuck onRequest handler (e.g. a hung upstream fetch in a proxy actor) will keep running after on_request_timeout has already reported failure to the caller, instead of being told to abort. This is a real, narrower-than-before regression: safe for disconnect/shutdown, not safe for the dispatch timeout case. Worth either restoring the combination or explicitly documenting that on_request_timeout intentionally no longer aborts in-flight JS work.

  4. cancelNativeHttpRequestBody unconditionally cancels the upload stream in onRequest's finally, even when the response streams directly from the request body
    rivetkit-typescript/packages/rivetkit/src/registry/native.ts now calls await cancelNativeHttpRequestBody(request.bodyStream) in the finally block after config.onRequest(...) resolves, per the comment "Handler completion ends upload ownership even when the Web Request body is locked or partly consumed." This is a real fix for leaking an unread native upload stream (good, and covered by a new unit test).
    But the driver-test fixture rivetkit-typescript/packages/rivetkit/fixtures/driver-test-suite/raw-http.ts:43 has /api/echo doing exactly return new Response(request.body, { headers: request.headers }), a request-to-response passthrough. convertNativeHttpResponse's streaming branch (native-http.ts) starts pumpResponseBody(reader, responseBodyStream) but does not await it before returning; the actual first reader.read() on the request body races against the finally's cancelNativeHttpRequestBody call. On the Rust side, HttpRequestBodyStream::read() (rivetkit-napi/src/http.rs:140) checks self.cancel.is_cancelled() first, before even checking buffered initial_body, so if cancel() wins the race, any not-yet-delivered (or even already-buffered-but-unread) bytes are dropped and the passthrough response silently truncates/empties.
    This only matters for genuinely streamed uploads: small requests go through the buffered body field and bodyStream is undefined (build_request_object in http.rs), so cancelNativeHttpRequestBody(undefined) is a no-op, which is exactly why the existing /api/echo driver tests (small JSON/binary payloads) don't catch this. I couldn't fully pin down the exact microtask ordering without running it, so this may or may not reproduce in practice, but there's no test covering "large/chunked upload streamed straight through to the response" via the native runtime, and the explicit "even when locked or partly consumed" framing suggests this tradeoff was made deliberately rather than accidentally. Worth a regression test with a sizeable chunked body through an echo-style handler to confirm the ordering is safe, or gating the cancel on whether the response body reader still holds a lock on the request stream.

Confirmed working well

  • Byte accounting via AtomicU64::fetch_add(..., Ordering::AcqRel) replacing size_hint() estimates is a real correctness improvement for HTTP metrics (previously egress/ingress were rough upper/lower-bound guesses that are frequently None/0 for streaming bodies). The counters are per-request and monotonically increasing, so record_transfer's unsigned diff (new - last) can't underflow.
  • The metrics task is correctly aborted (metrics_abort_tx.send(())) and finished on both the success and error paths in handle_http_request, avoiding a dangling background task on early failure; and since the watch::Receiver::changed() branch fires even if the sender is dropped without sending (e.g. due to upstream cancellation), the task can't leak.
  • CompletionGuard's Drop guarantees the with_completion callback fires exactly once even on early drop/error, so the new metrics-task-abort wiring should be leak-free regardless of how the response body ends.
  • New cross-language golden-byte tests (Rust and TS) for the abort message pin down the wire encoding, valuable regression protection independent of the v8-version question above.

Minor

  • ResponseBody::with_completion moved from pub(crate) to pub (with #[doc(hidden)]) to allow cross-crate use from pegboard-gateway2. Reasonable, just flagging since it widens the public API surface of guard-core.
  • record_ws_transfer renamed to record_transfer in metrics_task.rs, correctly reflecting that this task is now shared between HTTP and WebSocket paths.

No security concerns spotted; changes stay within the already-trusted pegboard-gateway2 and envoy internal boundary or are client-local NAPI/TS plumbing.

@NathanFlurry
NathanFlurry force-pushed the stack/fix-http-harden-streaming-lifecycle-and-observability-rkrtrmpn branch from 284c183 to 5ccfe9c Compare July 30, 2026 21:02
@NathanFlurry
NathanFlurry changed the base branch from main to stack/feat-http-stream-http-bodies-end-to-end-npntkwqx July 30, 2026 21:02
@NathanFlurry
NathanFlurry force-pushed the stack/fix-http-harden-streaming-lifecycle-and-observability-rkrtrmpn branch from 5ccfe9c to 74db67c Compare July 30, 2026 23:09
@NathanFlurry
NathanFlurry force-pushed the stack/feat-http-stream-http-bodies-end-to-end-npntkwqx branch from 1708458 to 3ed475a Compare July 30, 2026 23:09
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