feat(middleware): define HTTP response pre-return interface - #3073
feat(middleware): define HTTP response pre-return interface#3073pimlock wants to merge 13 commits into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
pimlock
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
The interface and caller migration are internally consistent, and the pre-0.1.0 symbol changes are not blockers. One enforcement gap must be fixed before this can advance: OpenShell currently accepts the new response binding even though no supported HTTP relay invokes it.
Action required: keep HTTP_RESPONSE/PRE_RETURN manifest registration rejected until the response runner is wired into every supported HTTP relay, or land that wiring with this change.
Blocking findings:
GATOR-11ef5b93-01: accepted response middleware bindings are silently unenforced
Carried findings:
- None
Gator metadata
- Validation: Project-valid as part of accepted issue #2691 and the documented response-middleware stack.
- Docs: Interface documentation is deferred to #3075; no runtime response behavior is enabled by this PR.
- Checks: Current-head required checks are green.
- E2E: Not requested for this interface-only review; the operator did not authorize applying
test:e2eor rerunning workflows. - Head SHA:
11ef5b932f859cf8f9a14a35270e18eb536a0c49 - Base SHA:
4b3b641b0c5c93ddc9e6d51ac11b45a271aa70b4 - Merge base SHA:
4b3b641b0c5c93ddc9e6d51ac11b45a271aa70b4 - Patch ID:
02da4f41c061dcfb3ea51e92faa8123f75da53be - Gator payload:
8 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
pimlock
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @pimlock. I checked your September 2 update that rejects HTTP_RESPONSE/PRE_RETURN until relay dispatch exists and adds a regression test. The manifest now fails closed for that binding, so GATOR-11ef5b93-01 is resolved; the independent delta review found no new blocking issues. Branch Checks are running, so Gator will monitor the pipeline next.
Blocking findings:
- No blocking findings remain
Carried findings:
GATOR-11ef5b93-01: resolved at this head by rejecting the undispatched response binding
Gator metadata
- Validation: Project-valid as part of accepted issue #2691 and the response-middleware stack.
- Docs: The protobuf contract is documented in-tree; runtime integration and the user guide remain scoped to #3074 and #3075.
- Checks:
OpenShell / Branch Checksis pending;OpenShell / Helm Lint, DCO, dependency review, and published E2E gate statuses are green. - E2E: No
test:e2elabel was applied because this PR leaves response dispatch disabled, and the operator did not authorize E2E dispatch,/ok to test, or workflow reruns. - Head SHA:
81ac0972414a9bd4cdc484642eaf30f7bcea877d - Base SHA:
4b3b641b0c5c93ddc9e6d51ac11b45a271aa70b4 - Merge base SHA:
4b3b641b0c5c93ddc9e6d51ac11b45a271aa70b4 - Patch ID:
a9784c43c09dea502f2c981aece30cb981ee88a8 - Gator payload:
8 - Review mode:
follow_up - Previous reviewed SHA:
11ef5b932f859cf8f9a14a35270e18eb536a0c49 - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:watch-pipeline
drew
left a comment
There was a problem hiding this comment.
Review feedback
I do not think the V1 interface is ready to approve yet. I found four contract-level issues:
-
The PR omits response trailers required by accepted issue #2691. The stream exposes only preflight, body, and session-end events, so middleware cannot inspect or safely mutate trailers. This leaves trailer-carried integrity metadata or sensitive data outside response enforcement. Please restore trailer events and declared trailer names, or formally re-scope the accepted issue.
-
block_deliveryexpands middleware authority beyond the accepted design. Issue #2691 explicitly defers successful response denial, but this PR adds it to preflight and body processing regardless ofon_error. It also describes only a “platform error,” without specifying its status, body, HEAD behavior, or stable error code. Please remove it from V1 or update and re-accept the design with complete client-visible semantics. -
A shared body-mode list conflicts with stage-local payload limits.
max_payload_bytesis the effective limit for the current stage, while every stage is documented as receiving the samepermitted_body_modes. For a 2 MiB response and stages limited to 1 MiB and 4 MiB,WHOLE_BODY_BYTESmust either be incorrectly offered to the first stage or incorrectly withheld from the second. Please compute response-shape eligibility once, then intersect it with each stage effective limit. -
The disconnect enum change is parse-compatible but not semantically wire-compatible. Previously, value
2meant genericPEER_DISCONNECT; it now means specificallyDOWNSTREAM_DISCONNECT. A new middleware connected to an older supervisor will classify an old upstream disconnect as downstream. Please preserve value2as direction-neutral and allocate new values for directional reasons, or version the contract.
The overall stream and transport shape is otherwise coherent. I also verified that the earlier Gator finding is resolved: HTTP_RESPONSE/PRE_RETURN registration is now rejected until relay dispatch lands.
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Replace the separate body_end event with HttpResponseBodyUnit.end_of_stream. Every body-inspecting stage receives exactly one flagged unit, which may be empty; a zero-byte body is one empty flagged unit and OpenShell never reads ahead to set the flag. Defer response trailers from V1 and reserve their field numbers. HTTP/1.0 clients and Content-Length bodies cannot carry trailers and that behavior was undefined. Add HttpResponsePreflight.permitted_body_modes, computed once from the original upstream head so every stage sees the same list, and make an unlisted selection a failure rather than a downgrade. Add the block_delivery preflight action as a successful decision enforced regardless of on_error. Expose Content-Length, Content-Encoding, and Content-Range read-only in preflight. Cap STREAM_BYTES input units at half of max_payload_bytes and permit deferring bytes across replacements only for fail_closed bindings, surfaced as deferral_permitted. Split PEER_DISCONNECT into DOWNSTREAM_DISCONNECT and UPSTREAM_DISCONNECT and attribute WebSocket relay failures by direction instead of a generic peer error. Compile the content-guard example in lint and branch checks so proto renames cannot break it silently. BREAKING CHANGE: WebSocketSessionEndReason and WebSocketSessionEnd are replaced by the shared MiddlewareSessionEndReason and MiddlewareSessionEnd. NORMAL_CLOSE is now NORMAL, UPSTREAM_REJECTED is now UPSTREAM_FAILURE, and PEER_DISCONNECT is split into DOWNSTREAM_DISCONNECT and UPSTREAM_DISCONNECT. Enum numbers are unchanged so binary wire compatibility is preserved; generated symbols and JSON names change. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
…tions Body results may now stop delivery or opt out of inspecting the rest of the response after a prefix. One HttpResponseBlockDelivery message is shared by preflight and body results and documents the difference between blocking before and after head commitment. Drop the field reservations, since nothing in this contract has shipped, and renumber session_end to close the gap. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
HttpBodyUnit, HttpBodyPassThrough, HttpBodyTransform, HttpBodySkipRemaining, and HttpBodyMode carry no response-specific semantics, so name them for reuse by the streaming request hook. Envelopes, results, preflight, and block_delivery stay response-specific because commitment semantics differ. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Reverts the shared HttpBody* naming. A direction-specific payload such as a response-only semantic mode would otherwise add unreachable variants to the other direction or force a source-breaking fork after 0.1.0. The streaming request hook defines its own HttpRequestBody* messages and copies the shape; SDKs present a direction-neutral body handler over both. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
81ac097 to
590b193
Compare
drew
left a comment
There was a problem hiding this comment.
Additional review feedback
A follow-up contract pass found three additional items beyond my earlier review:
-
The PR description and protobuf disagree about reserved trailer fields. The description says response trailers are deferred from V1 “with field numbers reserved,” but
proto/supervisor_middleware.protocontains noreserveddeclaration. The current event/result oneofs immediately assign session-end field3and expose no reservation for the trailer messages described in #2691. Please either add the intended reservations or correct the description and document how trailers will be added compatibly. -
Several other V1 contract changes have not been reconciled with the accepted plan. In addition to trailers and blocking, this interface replaces the planned
body_endevent withend_of_stream, addsskip_remainingand cross-unit deferral, reduces stream inputs from the full effective limit to half the limit, adds a whole-body accumulation timeout, and moves diagnostics into the decision envelopes. Some may be good changes, but they alter the accepted public contract. Please update #2691 and obtain maintainer confirmation for the revised V1 surface before freezing its field numbers and semantics. -
The new upstream-disconnect path lacks behavioral coverage. The mapping unit test verifies that
WebSocketTerminationCause::UpstreamDisconnectmaps to the enum, but no relay test verifies that an upstream EOF actually deliversUPSTREAM_DISCONNECTto an opened middleware session. Since the directional split changes externally observed lifecycle semantics, please add a relay-level test that closes the upstream and asserts the terminal reason.
Summary
Define the public
HTTP_RESPONSE/PRE_RETURNcontract without enabling response processing in the proxy. This lets reviewers evaluate the protobuf lifecycle, naming, extensibility, and transport shape independently from the runtime implementation.The proposed stream begins with a preflight action, uses lockstep pass-through or transform actions for body units, marks the final unit with
end_of_stream, and ends with a shared middleware session-end reason plus typed protocol-error detail.API interaction examples
See HTTP response pre-return middleware API interaction examples for whole-body, streaming, skip, and block-delivery exchanges, failure scenarios, and an illustrative Rust/tonic middleware implementation.
Related Issue
Part of #2691
Changes
HttpResponsePreReturn.Evaluateservice with preflight, body unit, body result, and terminal event messages.skip,inspect, orblock_delivery; body results returnpass_throughortransform. OpenShell tells each stage which body modes it may select and whether it may defer bytes across units.end_of_stream. Response trailers are deferred from V1 with field numbers reserved.MiddlewareSessionEndand its reason enum across streaming middleware APIs, with disconnects split by direction and typed protocol-error detail.HTTP_RESPONSE/PRE_RETURNmanifest binding and add in-process and remote gRPC stream interfaces.Breaking changes
WebSocketSessionEndReasonandWebSocketSessionEndare replaced by the sharedMiddlewareSessionEndReasonandMiddlewareSessionEnd.NORMAL_CLOSEis nowNORMAL,UPSTREAM_REJECTEDis nowUPSTREAM_FAILURE, andPEER_DISCONNECTis split intoDOWNSTREAM_DISCONNECTandUPSTREAM_DISCONNECT. Enum numbers are unchanged, so binary wire compatibility is preserved; generated symbols and JSON names change. This lands in the pre-0.1.0 window.Testing
mise run pre-commitpassesmise run testpassesChecklist
Stack