Skip to content

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

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

pimlock wants to merge 5 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 drew left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gator-agent

PR Review Status

This focused network-proxy correctness fix is valid for OpenShell, but the boundary-preserving implementation introduces one reachable resource-amplification regression that must be addressed before pipeline handoff.

Action required: @pimlock, preserve the request boundary without byte-at-a-time framing I/O, or add a conservative tested bound on aggregate framing/chunk amplification.

Blocking findings:

  • GATOR-5cc8e2f8-01: chunked framing is relayed with one read and write per byte.

Carried findings:

  • None

Non-blocking suggestions:

  • None
Gator metadata
  • Validation: Localized correctness and security fix for the supported REST network-proxy path, with a concrete reproducer and regression coverage.
  • Docs: Not needed because the fix does not change the published UX or protocol contract.
  • Checks: Current-head Branch Checks are still running; completed DCO, vouch, metadata, Helm, and Trivy gates are green.
  • E2E: test:e2e is required for network-proxy behavior but will be dispatched after blocking feedback is resolved.
  • Head SHA: 5cc8e2f808040ac8ecb7ebd0636316055595d459
  • Base SHA: cb6e88acb7fda83cd4355330b24dab1196105729
  • Merge base SHA: 484f0768fc6a0d93e0a2be295c1679aed24e18a9
  • Patch ID: c36e941826640402277bae4353c7d9932a9ecd9d
  • Gator payload: 10
  • Review mode: initial
  • Previous reviewed SHA: none
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:in-review

Comment thread crates/openshell-supervisor-network/src/l7/rest.rs
@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 drew left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gator-agent

Re-check After Author Update

Thanks @pimlock. I checked your 2026-09-21 update against the earlier chunk-framing amplification finding. The new 32 KiB aggregate framing limit covers chunk-size lines, per-chunk terminators, and trailers, and the counting-I/O regression exercises the previously unbounded tiny-chunk case. The prior finding is resolved, and the follow-up review found no new blockers in the author-only delta.

Blocking findings:

  • No blocking findings remain.

Carried findings:

  • GATOR-5cc8e2f8-01: resolved by the current framing cap and regression coverage.
Gator metadata
  • Validation: Localized correctness and security fix for the supported REST network-proxy path, with a concrete reproducer and regression coverage.
  • Docs: Not needed because the fix does not change the published UX or protocol contract.
  • Checks: Required current-head checks are running; no failures are currently reported.
  • E2E: test:e2e is applied and the current-head E2E workflow is running.
  • Head SHA: 107ce348e98ade65d52fdab10562dc07b0a22ebb
  • Base SHA: cb6e88acb7fda83cd4355330b24dab1196105729
  • Merge base SHA: 484f0768fc6a0d93e0a2be295c1679aed24e18a9
  • Patch ID: 4a84d6fcb3d6c801f01ca6d6e65a717e9554d41d
  • Gator payload: 10
  • Review mode: follow_up
  • Previous reviewed SHA: 5cc8e2f808040ac8ecb7ebd0636316055595d459
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:watch-pipeline

@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 drew left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gator-agent

Re-check After Author Update

Thanks @pimlock. I reviewed the latest author-only change that boxes the sandbox runtime future. It does not alter the chunk-boundary implementation, reopen the resolved framing-amplification finding, or introduce a new blocker.

Blocking findings:

  • No blocking findings remain.

Carried findings:

  • GATOR-5cc8e2f8-01: remains resolved by the framing cap and regression coverage.
Gator metadata
  • Validation: Localized correctness and security fix for the supported REST network-proxy path, with a concrete reproducer and regression coverage.
  • Docs: Not needed because neither the network fix nor the latest compile fix changes the published UX or protocol contract.
  • Checks: Required current-head Branch Checks and E2E jobs are still running; no failures are currently reported.
  • E2E: test:e2e is applied and the current-head E2E workflow is running.
  • Head SHA: 249d6c741006299905b21275ce11ffa38dde449a
  • Base SHA: cb6e88acb7fda83cd4355330b24dab1196105729
  • Merge base SHA: cb6e88acb7fda83cd4355330b24dab1196105729
  • Patch ID: d2105651050e1cdfc3c5613e075dc0367e884a98
  • Gator payload: 10
  • Review mode: follow_up
  • Previous reviewed SHA: 107ce348e98ade65d52fdab10562dc07b0a22ebb
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:watch-pipeline

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 drew left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gator-agent

Re-check After Author Update

Thanks @pimlock. I checked your latest update that replaces the aggregate framing cap with connection-scoped read-ahead and coalesced upstream writes. In the required critical-only review, the author delta does not introduce a new Critical defect, and the earlier framing-amplification finding remains resolved.

Blocking findings:

  • No blocking findings remain.

Carried findings:

  • GATOR-5cc8e2f8-01: remains resolved by connection buffering, write coalescing, and the counting-I/O regression.
Gator metadata
  • Validation: Localized correctness and security fix for the supported REST network-proxy path, with a concrete reproducer and regression coverage.
  • Docs: Not needed because the fix does not change the published UX or protocol contract.
  • Checks: Required current-head Branch Checks and E2E jobs are still running; no failures are currently reported.
  • E2E: test:e2e is applied and the current-head E2E workflow is running.
  • Head SHA: 02090a52e60a91f0de8960de632169573d3ce4f3
  • Base SHA: cb6e88acb7fda83cd4355330b24dab1196105729
  • Merge base SHA: cb6e88acb7fda83cd4355330b24dab1196105729
  • Patch ID: 0af969138f73a35573bb2748fbff3abe8dd1cb22
  • Gator payload: 10
  • Review mode: critical_only
  • Previous reviewed SHA: 249d6c741006299905b21275ce11ffa38dde449a
  • Review budget exhausted: yes
  • Maintainer decision required: no
  • Next state: gator:watch-pipeline

@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 drew left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gator-agent

Re-check After Author Update

Thanks @pimlock. I checked your September 21 update explaining that the previous write coalescing delayed a live MCP stream. The critical-only review covered the author delta that now forwards each complete validated chunk immediately and adds an open-stream regression; it found no newly introduced Critical defect.

Blocking findings:

  • No blocking findings remain.

Carried findings:

  • GATOR-5cc8e2f8-01: remains resolved because reads stay connection-buffered and writes occur per validated chunk rather than per framing byte.
Gator metadata
  • Validation: Localized correctness and security fix for the supported REST network-proxy path, with a concrete reproducer and regression coverage.
  • Docs: Not needed because the fix does not change the published UX or protocol contract.
  • Checks: Current-head Branch Checks and E2E are running with no reported failures; Helm Lint and Trivy Changes are green.
  • E2E: test:e2e is applied and the current-head E2E workflow is running.
  • Head SHA: 619c86ab26ca3a3fa733e02cfd18758d160402c6
  • Base SHA: cb6e88acb7fda83cd4355330b24dab1196105729
  • Merge base SHA: cb6e88acb7fda83cd4355330b24dab1196105729
  • Patch ID: e28f57b03e51be8c0f50acddd0c4632e7c645e59
  • Gator payload: 10
  • Review mode: critical_only
  • Previous reviewed SHA: 02090a52e60a91f0de8960de632169573d3ce4f3
  • Review budget exhausted: yes
  • Maintainer decision required: no
  • Next state: gator:watch-pipeline

@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
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.

2 participants