test: prove H2 pressure bounds upstream read-ahead - #84
seonghobae wants to merge 5 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
left a comment
There was a problem hiding this comment.
Current-head technical review for fec42c8318242edee906fff9a13047e15ce54aab (COMMENT only; not an independent approval).
Reviewed the effective #83→#84 delta after the Rust 1.98 formatter repair. The acceptance isolates shared H2 connection credit by advertising a 128 MiB stream window, consumes exactly the RFC 9113 initial 65,535-byte connection DATA window, and then observes end-to-end upstream pressure through nonblocking H1 origin WouldBlock before the 64 MiB stimulus can be handed off in full. It does not convert the stimulus size or host RSS into a product threshold. The PING barrier remains control-plane evidence while DATA credit is zero; recovery grants only stream-0 connection credit and requires the full response to complete intact without RST_STREAM/GOAWAY. Production Rust and external authority boundaries are untouched.
The traceability update ties this oracle to released Pingora 0.9.0 reserve_capacity/poll_capacity behavior but correctly treats supplier source as supporting evidence, not a substitute for the real-wire oracle. Dedicated baseline lane #61 remains untouched.
No additional causal source/test defect found in this exact review. Hosted exact-head checks remain authoritative; do not transfer predecessor results or mark Ready on this comment alone.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head technical review for 46682aeb9e5bb10914f88c0e87fdee7d06272ff4 (COMMENT only; not an independent approval).
Re-reviewed the #83→#84 effective delta after removing the unsourced 2-second observer cutoff. The WouldBlock notification now uses the already configured 5-second upstream read/write budget, so the fixture no longer creates an independent timing policy merely to make a deterministic test. The semantic oracle remains event-based: after the RFC 9113 shared connection window is exhausted, the origin must encounter socket backpressure before full 64 MiB handoff; the 64 MiB is a stress stimulus rather than a product memory ceiling. Stream credit stays non-limiting, PING must remain live while DATA is stalled, and recovery grants connection credit only.
Exact fec42c8318242edee906fff9a13047e15ce54aab had already passed formatting and compile/test, including the pressure fixture, before this timeout-alignment movement. Those predecessor results are not transferred to 46682a...; fresh hosted checks are authoritative.
No additional causal source/test defect found in this exact review. Keep Draft until current exact checks are terminal GREEN and review threads remain clean.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current technical review for 46682aeb9e5bb10914f88c0e87fdee7d06272ff4: re-read the two-path child range after execution. The raw-H2 fixture keeps stream credit non-limiting, consumes exactly 65,535 bytes of shared connection DATA credit, requires origin-side kernel WouldBlock before the full 64 MiB pressure body can be handed off, verifies PING liveness while DATA remains stalled, and restores only connection credit before byte-exact completion. The 64 MiB value remains stimulus, not a product memory ceiling. No production Rust, authority boundary, threshold weakening, or host-specific RSS claim is introduced. CI 34590209144, Supply Chain 34590209141, and PgErd bounded-origin capacity 34590209255 are all terminal success; review-thread enumeration is empty. No additional writer-safe source/test/documentation finding is present in the current range. This COMMENT is technical evidence only and is not independent APPROVED governance credit.
Refs #51.
Ready-for-review writer-safe child of #83 exact
6d0bf05bae5bff0e3b4360024567716e59350236. This increment targets the bounded-memory causal gap without inventing an RSS ceiling or changing production Rust.The raw certificate-verified H2 client advertises a 128 MiB stream window so stream credit cannot manufacture backpressure, opens one response whose H1 origin body is 64 MiB, and withholds shared connection WINDOW_UPDATE after exactly the RFC 9113 initial 65,535-byte connection DATA window is consumed. The origin writes the body through a nonblocking TCP socket. Acceptance requires kernel-visible
WouldBlockafter the downstream connection window is exhausted and before the origin can hand the complete body to the gateway. This is causal bounded upstream read-ahead evidence rather than a host-specific VmRSS threshold.While DATA remains stalled, a PING barrier must still be acknowledged and no non-empty DATA may cross the exhausted connection window. The client then restores only connection credit, releases the origin writer, and requires the full 64 MiB response to complete without RST_STREAM/GOAWAY and with every DATA byte intact. The origin may complete only after credit is restored.
The 64 MiB stimulus is pressure input, not a product sizing recommendation. The semantic oracle is that finite downstream credit propagates backpressure before full-body handoff; no fixed number of gateway-buffered bytes or process RSS is accepted as universal behavior. Released Pingora 0.9.0 source independently shows H2 body writes reserve and await
SendStreamcapacity, but source structure is evidence only; this PR requires the real end-to-end effect.Effective child scope is
tests/downstream_tls_h2_bounded_upstream_read_wire.rsplus the focuseddocs/doctoring/DOWNSTREAM_TLS_HTTP2_TRACEABILITY.mdprojection.docs/product-technical-gap-baseline.mdremains owned by dedicated lane #61. Product auth/business logic, Keyverse/Wardnet/EgressWeave authority, Admin Config, TLS policy, routing/retry semantics, supplier source and deployment authority are unchanged.Writer review repaired an invalid nine-octet PING fixture literal, applied the exact Rust 1.98 formatter diff, and removed an unsourced 2-second observer timeout in favor of the already configured 5-second upstream I/O budget. Those were fixture/evidence repairs only; the pressure oracle and production behavior were not weakened.
Current exact is
46682aeb9e5bb10914f88c0e87fdee7d06272ff4. Exact-current CI34590209144, Supply Chain34590209141, and PgErd bounded-origin capacity34590209255are all terminal GREEN. Formatting, compile/test including the 64 MiB pressure fixture, Clippy, rustdoc, owned-production coverage, dependency audit, candidate image/SBOM/scan/source binding, controlled load/runtime, and bounded-origin capacity evidence all completed successfully. Fresh review-thread enumeration is empty. Technical COMMENT5178059676re-read the two-path current range after execution and found no additional writer-safe defect; it is evidence only, not independentAPPROVEDgovernance credit.This PR is Ready for independent governance review. No protected merge, release, canary, cutover or legacy-removal credit is claimed.