Skip to content

test: prove downstream H2 GOAWAY drain semantics - #79

Draft
seonghobae wants to merge 13 commits into
test/downstream-h2-reset-sibling-survival-v1from
test/downstream-h2-goaway-drain-v1
Draft

seonghobae wants to merge 13 commits into
test/downstream-h2-reset-sibling-survival-v1from
test/downstream-h2-goaway-drain-v1

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Refs #51. Writer-safe child of #78 exact b4c54e22c252aa99a19b2b90881343653c6aa995.

This PR owns the real-wire HTTP/2 graceful-shutdown/GOAWAY acceptance only. Production routing, Admin Config values, TLS/cipher policy, product authentication/business logic, Keyverse/Wardnet/EgressWeave authority, supplier source and deployment authority remain unchanged.

Released-source review corrected two fixture assumptions. Pingora 0.9.0 at tag/source 702f69015e53f7244d6ad2e743de571d859a70a4 broadcasts shutdown notification before Server::run enters grace_period_seconds; the five-second CWL grace is a process-level interval after service shutdown notification, not a mandatory pre-GOAWAY delay. Resolved h2 = 0.4.19 sends initial graceful GOAWAY(NO_ERROR, 2^31-1) and uses shutdown PING/ACK before the later GOAWAY, so the raw H2 client ACKs non-ACK PING with the identical eight-octet payload.

Current exact c0406907aea4c2a13d76d219cfacd9c508a2a467 contains the protocol-correct control-frame handling plus the Rust 1.98.0 formatter-only repair. The executable oracle remains: stream 1 is admitted before SIGTERM; stream 3 is sent immediately after SIGTERM and before the client observes GOAWAY; initial GOAWAY is observed according to the supplier shutdown ordering; admitted streams complete without reset; later GOAWAY last-stream-id does not increase; final drain and process exit remain bounded. Production Rust, graceful-shutdown timing, routing, HTTP policy and ownership boundaries are unchanged.

Exact-current evidence is terminal GREEN: CI 34573964785, Supply Chain 34573964798, and PgErd bounded-origin capacity 34573964782. Fresh review-thread enumeration is empty. This PR is Ready for review. Technical COMMENT reviews are evidence only; no independent APPROVED governance credit is claimed, so protected merge is not yet credited.

This remains narrower than decoded header/body admission, connection/stream flow control and backpressure, partial-body cancellation semantics, origin failure/recovery, forwarding trust, H2→H1 Cookie/body framing, HTTP/3/QUIC, representative TLS handshake/reuse and NUMA performance, immutable release, canary/shadow, rollback, cutover and legacy removal. No bypass, protected merge, release or cutover credit is claimed.

@coderabbitai

coderabbitai Bot commented Sep 11, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 3ccc2618-3cf7-44ed-82d6-a5a1d058da0d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exact-head pre-execution technical review for f08cfd5ea1b7ef0b426b853ced1c65c3ff19ea13 (not an approval). Fresh parent→child compare is ahead-only with merge base exactly #78 9731fc3c... and one effective path: tests/downstream_tls_h2_goaway_drain_wire.rs. The oracle follows RFC 9113 §6.8 rather than manufacturing a post-GOAWAY client stream: stream 3 is sent during the configured SIGTERM grace/race window before GOAWAY is observed; initial GOAWAY must be NO_ERROR/max stream-id, admitted streams must survive without reset, later last-stream-id must not increase, and process exit remains bounded. Hosted exact execution is still required; no GREEN, Ready, merge, or release credit is assigned by this review.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exact-head technical review for d1d6f3c479fe38ba0ee1ecb4d9d543263c8465b4 (not an approval). Fresh parent→child compare is ahead-only with merge base exactly current #78 441725515442c5f592134eb6ece8245be0c9f0d6; the effective delta remains only tests/downstream_tls_h2_goaway_drain_wire.rs. The fixture uses the RFC 9113 graceful-shutdown pattern: stream 3 is admitted during the configured pre-GOAWAY grace/race window, initial GOAWAY must be NO_ERROR with max last-stream-id, admitted streams must not reset, later last-stream-id cannot increase, and process exit stays within the existing hard-kill budget. Hosted exact execution is still required; no GREEN/Ready/merge/release credit is assigned here.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exact-head technical review for a7185f61bc217e568b56bff8f0f9727ba34291ef (not an approval). This is an ordinary two-parent restack over current #78 b4c54e22c252aa99a19b2b90881343653c6aa995; no force/rebase was used. The GOAWAY test is preserved while inheriting #78's cancellation-order and sibling-response-oracle repairs. Hosted exact-current execution is still required before any GREEN/Ready/merge/release credit.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exact-head technical review after CI RCA and fixture repair. The previous a7185f61... CI failure was formatter-only; ea151608... added only the missing EOF newline. Fresh review then found a distinct false-GREEN in the grace-period oracle: waiting on the stream-3 origin channel before reading frames allowed an immediately emitted GOAWAY to remain unread while wall time accumulated. Current 5232c94d... removes that ordering artifact by timestamping stream-3 origin admission, reading H2 frames immediately after the stream-3 flush, timestamping the initial GOAWAY on observation, and requiring both (a) stream-3 admission inside the configured 5 s CWL pre-shutdown grace and before observed GOAWAY and (b) observed GOAWAY at/after that grace boundary. Production Rust/policy is unchanged. This is technical evidence only, not an approval; current exact gates must terminate before promotion.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exact-head technical review after the second GOAWAY-fixture pass. 5232c94d... correctly moved the causal oracle to captured stream-3/GOAWAY timestamps, but the subsequent process-local channel receipt still had an unrelated 100 ms deadline. That bound was not an RFC or CWL runtime invariant and could false-RED solely from scheduler delay after the evidence event had already occurred. Current 8348cacf... uses the configured five-second pre-shutdown grace as the channel receipt budget while retaining the captured timestamp requirements: stream 3 must have been observed before GOAWAY and within grace, and GOAWAY itself must be observed at/after grace. Therefore the longer receipt wait cannot manufacture GREEN. Production Rust and shutdown policy remain unchanged. This is review evidence only, not approval; exact-current gates remain authority.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exact-head technical review after CI log RCA (not an approval). Predecessor 8348cacf... had Supply Chain and bounded-origin capacity GREEN, while CI stopped only at Rust 1.98.0 cargo fmt --all -- --check; load-contract and OCI runtime were GREEN. Job 103158728017 prescribed four layout-only changes in tests/downstream_tls_h2_goaway_drain_wire.rs. Current 8b054eef... applies exactly those four formatter deltas; the GOAWAY timing oracle, 5 s policy-bound evidence receipt, production Rust, shutdown policy, and protocol semantics are unchanged. No unresolved review thread is present. Fresh exact-current gates, not predecessor receipts, remain promotion authority.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Current-head technical review for fa064688187e15ae16d48ee227b09f9a150e5a6e: the previous exact CI failure is causally explained by the raw H2 client not ACKing the graceful-shutdown PING used by resolved h2 0.4.19 after initial GOAWAY(MAX). This exact adds only protocol-correct control-frame handling in the fixture: non-ACK SETTINGS remain acknowledged; non-ACK stream-0 PING is required to carry exactly eight octets and is echoed unchanged in PING ACK in both drain loops. Production runtime policy, shutdown timing, routing and authority boundaries are unchanged. No new source finding in this reviewed delta. This COMMENT is evidence only, not independent approval; keep Draft until exact-current CI/Supply Chain/capacity are terminal.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exact-current technical review: predecessor CI 34572255720 failed only at Rust 1.98.0 cargo fmt --all -- --check, whose job log showed a single requested layout change for the SETTINGS acknowledgement flush. Commit c040690 applies exactly that formatter output; the commit diff contains no production or protocol-semantic change. Current CI/Supply Chain/capacity receipts are newly queued, so this is evidence only, not independent APPROVED or merge/release credit.

@seonghobae
seonghobae marked this pull request as ready for review September 11, 2026 08:19
@seonghobae seonghobae added bug Something isn't working priority: high labels Sep 12, 2026 — with ChatGPT Codex Connector
@seonghobae
seonghobae marked this pull request as draft September 17, 2026 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority: high

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant