Skip to content

fix(network): preserve chunked request boundaries - #3530

Open
pimlock wants to merge 7 commits into
mainfrom
fix-chunked-relay-pipelining/pm
Open

pimlock wants to merge 7 commits into
mainfrom
fix-chunked-relay-pipelining/pm

Conversation

@pimlock

@pimlock pimlock commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

Preserve HTTP/1.1 request boundaries with connection-scoped read-ahead buffering so a following pipelined request remains available for its own L7 policy decision. Forward each complete validated chunk promptly without imposing a cumulative framing limit.

Related Issue

No issue required: this is an obvious localized correctness fix in the chunked request relay.

Failure Scenario and Fix

A client can send an allowed chunked request and the next request in one socket write:

POST /allowed HTTP/1.1
Host: example.com
Transfer-Encoding: chunked

0

DELETE /blocked HTTP/1.1
Host: example.com
Content-Length: 0

Previously, the chunked relay could read both requests into its local 8 KiB buffer and write the entire buffer upstream before recognizing the 0\r\n\r\n terminator. The subsequent DELETE therefore reached upstream as overflow from the allowed request before its own policy evaluation.

The inspected connection now owns the read-ahead buffer for its full keep-alive lifetime:

socket:  [allowed chunked body][pipelined DELETE]
buffer:  may read both
parser:  consumes only through the chunked terminator
relay:   forwards only the allowed body
next:    parses and evaluates DELETE from retained read-ahead bytes

The upstream receives only the allowed POST. The remaining DELETE stays in the connection buffer, enters the normal request loop, and receives a separate policy decision.

There is no longer a 32 KiB aggregate framing limit. Valid requests with many tiny chunks continue to relay. Underlying reads use the persistent 8 KiB connection buffer, and each complete validated chunk is forwarded promptly. Individual chunk-size and trailer lines remain subject to the existing 16 KiB line ceiling so an unterminated line cannot grow memory without bound.

Changes

  • Add connection-scoped read-ahead buffers around inspected client and upstream streams
  • Consume only the current chunked body while retaining pipelined bytes for the next request
  • Remove the 32 KiB cumulative chunk-framing cap
  • Forward every complete validated chunk without waiting for 8 KiB or end-of-body
  • Add counting-I/O coverage that accepts 10,000 one-byte chunks with fewer than 100 underlying reads and at most one write per chunk
  • Add a streaming regression proving a complete chunk is forwarded while the body remains open
  • Keep unit coverage for basic chunk termination and prefetched chunk bodies with trailers
  • Keep the Docker-backed proxy regression proving the subsequent denied request does not reach upstream

Testing

  • mise run pre-commit passes
  • mise exec -- cargo test -p openshell-supervisor-network --lib (1,341 passed, 2 ignored)
  • Focused open-stream and 10,000 one-byte chunk regressions pass
  • mise run --no-deps --skip-deps e2e:mcp (3 scenarios passed, including elicitation defaults)
  • OPENSHELL_E2E_DOCKER_TEST=proxy_egress_pipeline mise run e2e:docker (11 passed)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (not applicable; no public contract change)

@pimlock
pimlock requested a review from drew September 21, 2026 20:35
drew
drew previously approved these changes Sep 21, 2026
drew

This comment was marked as outdated.

@drew drew added the gator:in-review Gator is reviewing or awaiting PR review feedback label Sep 21, 2026
@pimlock pimlock added the test:e2e Requires end-to-end coverage label Sep 21, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied for 107ce34. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway, sandbox, and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

drew

This comment was marked as outdated.

@drew drew added gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:in-review Gator is reviewing or awaiting PR review feedback labels Sep 21, 2026
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
@pimlock
pimlock force-pushed the fix-chunked-relay-pipelining/pm branch from 107ce34 to 738965b Compare September 21, 2026 21:43
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
@pimlock
pimlock enabled auto-merge September 21, 2026 22:02
@pimlock
pimlock disabled auto-merge September 21, 2026 22:14
drew

This comment was marked as outdated.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
drew
drew previously approved these changes Sep 21, 2026
@pimlock
pimlock enabled auto-merge September 21, 2026 22:39
drew

This comment was marked as outdated.

@pimlock
pimlock added this pull request to the merge queue Sep 21, 2026
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
@pimlock
pimlock removed this pull request from the merge queue due to a manual request Sep 21, 2026
@pimlock

pimlock commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator Author

The MCP E2E failure was caused by the 8 KiB write-coalescing added in the previous revision. A small chunk on the long-lived MCP stream remained buffered until end-of-body, so the elicitation callback could not run and tools/call timed out after 60 seconds.

Commit 619c86a keeps connection-scoped read-ahead but forwards each complete validated chunk immediately. It also adds an open-stream regression that verifies the first chunk reaches the destination before the terminal chunk is sent.

Verified locally:

  • mise run --no-deps --skip-deps e2e:mcp: all 3 scenarios pass
  • network unit suite: 1,341 passed, 2 ignored
  • mise run pre-commit: pass

@pimlock
pimlock enabled auto-merge September 21, 2026 23:14
drew

This comment was marked as outdated.

@drew drew added gator:approval-needed Gator completed review; maintainer approval needed and removed gator:watch-pipeline Gator is monitoring PR CI/CD status labels Sep 21, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

Was this a regression? IIRC this was a request smuggling issue we fixed a while ago. Def worth getting in as a security hardening if this is ready to approve and merge.

@pimlock

pimlock commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator Author

Was this a regression? IIRC this was a request smuggling issue we fixed a while ago. Def worth getting in as a security hardening if this is ready to approve and merge.

@johntmyers
This wasn't a regression from what I can tell. I did remember filing an issue for this type of bug (#2251) and the agent mentioned that this PR #2373 fixed that one, but this one was one case that wasn't covered by that, here are some details and an example:

https://gist.github.com/pimlock/0c2f649b4c89cd5cb8b52a9f72ce9f31#what-remained-unresolved-after-pr-2373

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
@drew

drew commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

gator-agent

Maintainer Approval Needed

Gator’s critical-only review of the current merge-only update found no new blocker, and all required checks are green. @johntmyers asked whether this was a regression; thanks @pimlock for clarifying that the earlier #2373 work did not cover this pipelined-request boundary case. I checked #2373’s stated scope, the current range-diff, and the three PR-owned file blobs; the five reviewed PR commits and their file contents are unchanged by the latest merge from main.

Review: The earlier framing-amplification finding remains resolved, with no open Gator threads and no new Critical defect in the current author patch.

Human maintainer approval is now required.

Gator metadata
  • Validation: Localized correctness and security hardening for the supported REST network-proxy path, with regression coverage.
  • Docs: Not needed because the change does not alter the published UX or protocol contract.
  • Checks: Current-head Branch Checks, Helm Lint, Trivy Changes, and required E2E are green.
  • E2E: test:e2e is applied and OpenShell / E2E is green.
  • Head SHA: 76a5f92af8e149775b4db54fe60617d9c2e12446
  • Base SHA: c8a4ff5f19ded0ad9f70886f902c3881055941e3
  • Merge base SHA: c8a4ff5f19ded0ad9f70886f902c3881055941e3
  • Patch ID: 5a03d85bed196baf3ba7f075b0c214769330e8de
  • Gator payload: 10
  • Review mode: critical_only
  • Previous reviewed SHA: 619c86ab26ca3a3fa733e02cfd18758d160402c6
  • Review budget exhausted: yes
  • Maintainer decision required: no
  • Next state: gator:approval-needed

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:approval-needed Gator completed review; maintainer approval needed test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants