Repository navigation
Preserve Deno cancellation across streamed HTTP bodies - #5410
kojiwakayama wants to merge 24 commits into
Conversation
Deno2.9.7 and2.9.8 can lose the HTTP1 disconnect signal after bodyEOF. Reuse public Node transports only for those affected Deno listeners, retaining cleartext HTTP2, exact-request WebSocket ownership, native server DOM behavior and request-local context. Keep all accepted sockets and sessions owned through denial, cancellation and shutdown. Constraint: Preserve valid HTTP1, h2c and WebSocket contracts without new dependencies Rejected: Older compiler rollback | known security advisories Rejected: Early response or timeout workaround | changes streaming semantics without fixing cancellation Rejected: Native WebSocket bridge | public shutdown remained hung Confidence: high Scope-risk: moderate Directive: Retain HTTP2, WebSocket and segmented-body cancellation regressions when changing transport selection Tested: Pinned Deno focused69tests187steps; Node51tests; Bun2files; npm/dnt; normal proxy compile; macARM fullchain10of10abort1to3ms Not-tested: Actual Linux execution and staging Acceptance2086/2173
Reuse the native bind-error boundary for the selected Node listener so DNS and address collisions retain actionable classification without leaking private bind hosts through Node error causes. Leave unrelated startup failures intact. Keep the public server module documentation ahead of imports so generated references retain their examples. Confidence: high Scope-risk: narrow Tested: Pinned Deno 2.9.7 lifecycle suite, 20 steps; real selected DNS failure; API-reference check, 49 files; lint and formatting Not-tested: Linux execution and deployed staging cancellation remain integration gates
The successful WebSocket response now completes its drain entry before the router catch. Locate the Host-error catch directly instead of assuming its completion call is first or that its response fits a fixed character window. Preserve checks for drain completion, typed Host rejection and HTTP 400. Confidence: high Scope-risk: narrow Tested: Pinned Deno 2.9.7 standard test:file, 3 suites and 12 steps; lint; clean uncommitted review
Propagate the compat upgrade response union through the existing workflow handler so branded Node upgrade responses remain type-correct under Deno. The handler continues to return ordinary HTTP responses for invalid run IDs. Confidence: high Scope-risk: narrow Tested: Pinned Deno 2.9.7 publisher/test typecheck; standard test:file, 4 suites and 42 steps; lint; clean uncommitted review
The selected listener must commit response headers while an application holds its body stream. Otherwise the existing native ingress cancellation contract cannot observe a disconnect during response delivery. Flush after acquiring the response reader only for the Deno-selected listener; retain Node defaults. Confidence: high Scope-risk: narrow Tested: Existing real cancellation suite, 54 steps; HTTP1/HTTP2/upgrade and listener regressions, 6 suites and 53 steps; clean uncommitted review
Use the public HTTP2 writeHead operation alongside HTTP1 flushHeaders. Exercise both protocols with a held body: headers arrive before any chunk, client disconnect cancels the handler and reader, sibling traffic continues, and shutdown completes. Removing the flush makes both regressions fail. Confidence: high Scope-risk: narrow Tested: Pinned Deno 2.9.7 transport/typecheck, 5 real-wire tests; no-flush control fails both added cases; clean uncommitted review
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
Native CI correctly rejects effect-bearing new unit tests without semantic dispositions. Relocate both unchanged regressions to the existing integration discovery lane instead of adding migration exemptions. Rejected: Add semantic audit migration entries | new effect-bearing units must relocate instead Tested: Two focused tests, frozen typecheck, lint, semantic unit-boundary audit Scope-risk: narrow
Selected Deno listeners must suppress held HEAD response bodies and settle HTTP independently of producer cleanup. Cancel without awaiting or exposing cleanup failure to the response, preserving ordinary Node behavior. Pair the HTTP1 and h2c regressions with native Deno HTTP1 controls for resolved, pending and rejected cleanup. Align three existing test helpers with the actual serve-handler type rather than hiding WebSocket upgrade responses behind Response annotations. Constraint: Native Deno h2c itself does not finish the held-HEAD control; validate selected h2c against HTTP HEAD semantics without claiming native bug-for-bug parity Tested: Fourteen pinned Deno wire tests; three fixture suites with eleven steps; frozen typecheck; lint; forty ordinary Node transport tests Scope-risk: narrow
The new transport integration tests run in native CI but were absent from its coverage collection. Include the four focused wire suites while retaining all existing integration-client coverage paths. Replace duplicate HTTP1/HTTP2 write branches with one structurally checked byte writer; both overloaded public response APIs satisfy that contract without a cast. Constraint: Preserve the existing coverage threshold and integration selection Rejected: Direct call on the overloaded response union | incompatible TypeScript call signatures Tested: Frozen HTTP response typecheck; lint; fourteen Deno wire tests; forty Node HTTP transport steps Scope-risk: narrow
Deno's Node HTTP implementation closes completed sockets when its Server is not listening. A separate TCP demultiplexer feeding an unbound HTTP server therefore advertised keep-alive but closed each response, racing the renderer's next pooled request. Let the actual HTTP1 server own the public listener and defer only its connection dispatch until fixed-prefix protocol detection completes. Reuse the captured public event dispatcher without removing parser listeners; retain h2c session ownership, upgrade events, shutdown and private collection discipline. Rejected: Fake listening flags or private handles | violates actual transport ownership Rejected: Additional hidden listeners or Connection close | unnecessary exposure or persistent-connection regression Tested: Deterministic same-socket POST204 then GET red/green; thirty-three wire tests and four steps; frozen checks; lint; forty Node transport tests; parallel renderer thirty-two steps Scope-risk: moderate
Validate incoming transport sockets with the captured intrinsic hasInstance operation. A direct instanceof check would invoke a mutable Socket.hasInstance hook installed by project code, exposing the raw connection before request credential isolation. Keep the original exact Socket validation and cover the authored observer with a failing-first regression. Tested: Eighteen ownership and real wire tests; frozen typecheck; lint; mandatory uncommitted review Scope-risk: narrow
The native anti-slop gate rejects generic object parameters. Use the existing HTTP handler response and Node WebSocket connection contracts for private metadata keys, and construct a valid response and connection in the primordial-observer regression. Constraint: Preserve exact-object grants and captured collection intrinsics without adding lint exemptions. Confidence: high Scope-risk: narrow Tested: Pinned Deno 2.9.7 frozen typecheck, full lint, anti-slop baseline, and eight real WebSocket/provenance tests with two steps. Not-tested: Changed-head native Linux CI remains required.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e7a44a15a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Native review found a sibling service listener bypassing the affected-version transport and eager implicit WebSocket loading expanding HTTP-only permissions. Reuse the canonical service transport and resolve the default provider only after a registered upgrade, retaining captured disabled/custom policy, single-flight loading and shutdown ownership. Preserve callback error identity, reject malformed authorities as client errors, and copy SharedArrayBuffer WebSocket payloads as declared binary inputs. Constraint: Keep Node/Bun policy, valid WebSocket behavior, HTTP2, credential boundaries and native CI gates intact. Rejected: Reject SharedArrayBuffer to mimic pinned native behavior | That narrows the declared ArrayBufferLike surface. Confidence: high Scope-risk: moderate Tested: Pinned Deno 2.9.7 focused 61 tests and 35 steps, additional relocated least-privilege subprocess, six lazy-provider wire cases, full lint, affected frozen types, semantic/anti-slop/dependency/module audits, normal proxy compilation, clean mandatory uncommitted review. Not-tested: Changed-head native Linux CI and staging remain required.
Four existing hosted-service tests replace native Deno.serve. Explicitly scope those fixtures to the unaffected native transport so the corrected affected-version service route cannot bypass their injected lifecycle. Separately occupy the actual service bind address and verify that affected-version failures are captured, flushed, reset and exit nonzero even when flushing rejects. Constraint: Preserve every existing telemetry assertion and verify the newly selected real listener without a production test injection or hook bypass. Confidence: high Scope-risk: narrow Tested: Pinned Deno 2.9.7 hosted suite 66 tests and five steps, frozen typecheck, lint, unchanged anti-slop baseline, clean mandatory uncommitted review. Not-tested: Full normal push hooks and changed-head native CI remain required.
Move the selected-transport occupied-port regressions without changing telemetry or cleanup assertions. Native injected lifecycle fixtures remain unit tests. Constraint: Real server effects must not enter the semantic unit boundary. Rejected: Expand migration dispositions | conceals a real integration effect. Confidence: high Scope-risk: narrow Tested: Pinned Deno 2.9.7 hosted and integration tests, 66 tests and 5 steps; exact changed-base semantic audit; lint; mandatory uncommitted review.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76a0a17536
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Reuse the shared temporary-directory helper while retaining agent registries and bundler cleanup. This removes copied primitive violations without widening lint baselines. Confidence: high Scope-risk: narrow Tested: Exact native composite lint chain and separate test typecheck; pinned integration tests; mandatory uncommitted review.
Reuse the owned denial writer before invoking application handlers or loading WebSocket providers. Preserve ordinary denial cleanup and test HTTP1, h2c and upgrade authority rejection. Confidence: high Scope-risk: narrow Tested: Red wire regression, 32 passing wire tests and 2 steps; full changed-base lint:ci; mandatory uncommitted review. Not-tested: Changed-head native Linux CI and staging remain pending.
Count bytes owned by the Deno facade during Blob conversion and ordered sends, transferring ownership when the transport accepts them. Release accounting on failure or closure. Native Deno 2.9.7 omits these queued bytes; retain that observation separately while honoring the selected facade DOM contract. Constraint: Preserve the existing native closing and closed zero behavior without redesigning the state machine. Confidence: high Scope-risk: narrow Tested: Four deterministic red-to-green cases; 36 focused tests and 2 steps; full changed-base lint:ci; mandatory uncommitted review.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Both public createHandler wrappers convert ordinary responses to the native class. Preserve the exact registered upgrade signal before that conversion so DenoAdapter can attach the authorized socket instead of denying it. Confidence: high Scope-risk: narrow Tested: Real public development and production WebSocket connected frames fail before fix and pass after; pinned Deno 2.9.7 nine tests/nine steps; typecheck; lint; boundaries; required uncommitted and base Codex reviews Not-tested: No external endpoint or paid model run
|
@codex review |
|
@codex review |
The integration fixture cleans up its sockets, server, handler and adapter. Remove unnecessary sanitizer opt-outs so the new test satisfies the existing CI baseline while exercising both public handler modes with leak checks. Constraint: Do not expand sanitizer exception baselines Confidence: high Scope-risk: narrow Tested: Pinned Deno public-handler WebSocket integration with default sanitizers
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5c6c242ca
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
The current head is not merge-ready despite passing CI. All three native P2 findings reproduce on pinned Deno 2.9.7: implicit WebSocket upgrades add read/environment permissions, a supplied bootstrap with an omitted provider disables upgrades, and inbound TRACE fails before application dispatch. The bootstrap omission has a bounded local fix and a passing real-WebSocket regression; the final feedback batch is not pushed yet. A real adapter control also exposes a fourth compatibility issue: after a client sends the full POST and closes only its write side while continuing to read, native Deno delivers the response, but the selected transport cancels the handler and drops the response. The new local regression checks both premature cancellation and actual response delivery with default sanitizers. A proposed native The upstream observation gap is visible in Deno 2.9.7 peer polling, and remains in 2.9.8/current upstream main. TCP half-close semantics prohibit treating read EOF alone as proof that a client abandoned its response. Diagnosis confidence: 95%. PR returned to draft; native findings stay unresolved. No publication, staging dispatch, permission expansion, or weakened assertions are proposed. The remaining work is a proven transport/runtime solution that passes both cancellation and compatibility controls, followed by the complete test/review cycle. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
# Conflicts: # src/server/bootstrap.ts
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
# Conflicts: # src/proxy/main.test.ts # src/proxy/main.ts
|
Deno 2.9.7 and 2.9.8 can lose HTTP/1 disconnect notification after consuming a streamed request body. Cancellation then stops the upstream API request while the standalone proxy and canonical runtime continue executing. Select the existing Node transports on those affected versions while preserving Deno's filesystem/environment/shell identity.
The actual HTTP1 server owns the listening lifecycle, preserving same-socket keep-alive across bodyless POST and subsequent requests. Its public event dispatcher routes the fixed protocol prefix without private handles, fake listening flags, hidden ports or parser-listener removal. The Deno listener retains cleartext HTTP/1+h2c on one port, WebSocket upgrades, streaming/backpressure, response status/cookies, request-local context and deterministic socket/session cleanup. WebSocket provider intent is explicit: direct callers retain the first-party default, disabled extensions remain disabled, and custom providers remain selected. Exact original-request upgrade grants reject clones and spoofed headers; captured collection operations and intrinsic socket validation prevent authored observers seeing request/transport objects.
The implicit WebSocket provider loads only after an authorized upgrade, preserving HTTP-only startup with network permission alone. The service-server entry point uses the same affected-version cancellation transport.
The Deno WebSocket facade preserves server DOM metadata, binaryType/Blob ordering, actual transport buffering, supported close behavior and default/nonzero idle timeout. Malformed reserved close codes fail synchronously rather than reproducing native Deno's invalid wire frames. Bind failures preserve Deno classification and sanitize private hostnames; caller onListen errors retain their original identity. Malformed HTTP1 Host and h2 authority parsing returns redacted 400 responses before application handlers. Direct SharedArrayBuffer and shared-backed views are copied into binary frames, honoring the declared ArrayBufferLike contract; pinned native Deno 2.9.7 instead rejects those inputs, which the paired control documents. Ordinary Node/Bun transport selection and dependency versions remain unchanged.
Validation:
Normal pinned pre-push gate passes: formatting, lint, src/index.ts typecheck and all parallel/serial/cwd/cwd-exclusion unit phases.
Pinned Deno 2.9.7 focused 69 tests and 187 steps and a separate 20-step bind/lifecycle run; actual Node 51 tests, no skips; focused Bun: 2 applicable files.
Native-vs-selected h2c parity; mixed HTTP1/WebSocket/h2c streaming, concurrent stream reset isolation, cancellation after segmented body EOF, fragmented/malformed/stalled prefaces and shutdown.
Held HEAD responses complete without consuming their body or waiting for resolved/pending/rejected cleanup, paired with native HTTP1 controls.
Held-body HTTP1/h2c response headers arrive before the first chunk; disconnect cancels the handler/reader, a sibling survives, and shutdown completes. Both regressions fail when early header delivery is removed.
Same-socket POST204→GET regression fails on the earlier mux and passes with the actual HTTP1 listener owner. Parallel renderer/perf/full-lifecycle group: 32 steps passed.
Final focused wire group: 34 tests and 4 steps passed; four runtime wire suites now contribute to the existing integration coverage collection, retaining prior selections and thresholds. Actual collected LCOV confirms the Deno WebSocket facade and HTTP writer execute.
Exact-request provenance and collection observer regressions; asynchronous denial error containment and unread-denial teardown.
Normal npm/dnt build, macOS ARM64 compiled proxy, Linux x86_64 cross-compile, typecheck/lint/module boundaries/core dependencies.
Hosted startup telemetry: 66 tests/5 steps passed, including explicit native injection fixtures and actual selected-listener bind failure with successful/rejected flush.
Latest native-review repair: 61 focused tests/35 steps passed, plus the relocated least-privilege subprocess. Real regressions cover malformed authority, callback error identity/cleanup, shared-buffer wire bytes, single-flight lazy provider loading, disabled/custom policy snapshots, loader failure and shutdown during resolution.
Rebuilt latest compiled proxy+Caddy 2.11.4+canonical DenoAdapter, 65,552-byte segmented synthetic body and 4,700 ms disconnect delay: 10/10 backend aborts in 1–4 ms on macOS ARM64.
Execution qualification uses pinned Deno 2.9.7; 2.9.8 routing is covered by unit tests and the byte-identical affected native HTTP source. Actual Linux execution and staging Acceptance 2086/2173 remain required before claiming the deployed issue resolved. This PR does not publish or deploy.
Draft gates: required uncommitted and refreshed-base CLI reviews found no actionable regressions. Native CI and Sonar passed on the preceding head; changed-head native CI/review and deployed staging qualification remain pending. No merge, queue, publication or deployment is authorized by this draft.
Latest review followups also return HTTP400 for malformed upgrade authorities before handler/provider admission and count facade-owned queued WebSocket bytes during Blob conversion. Native Deno2.9.7 queued-Blob bufferedAmount omission remains explicitly qualified; selected transport follows the DOM queue contract while retaining existing closing/closed zero behavior. Full changed-base lint:ci and36focusedtests/2steps pass.
The public createHandler development and production wrappers now preserve the exact registered WebSocket upgrade response. Both paths are exercised through a real DenoAdapter listener and must receive the connected JSON frame; the regressions fail before this repair. Pinned Deno 2.9.7 passes all nine transport tests and nine steps, focused typecheck, lint, formatting and module boundaries. Required uncommitted, refreshed-base and exact committed-head CLI reviews are clean. The full normal pre-push gate passed and commit 2f0fabc is pushed. Changed-head hosted CI and native review remain pending.
Changed-head lint followup: the new integration suite now uses default resource and operation sanitizers. Both real public-handler cases pass with leak detection enabled; the existing sanitizer baseline stays unchanged. Full lint:ci passes (37 suites, 300 steps), both required CLI reviews and the exact committed-head refreshed-base review are clean, and the full normal push hook passes at e5c6c24. Changed-head hosted CI and native review remain pending.