Skip to content

Bun.serve: drain a tryEnd response tail before closing on peer FIN - #35088

Merged
Jarred-Sumner merged 2 commits into
mainfrom
farm/fe9f9f24/serve-half-close-drain
Jul 23, 2026
Merged

Bun.serve: drain a tryEnd response tail before closing on peer FIN#35088
Jarred-Sumner merged 2 commits into
mainfrom
farm/fe9f9f24/serve-half-close-drain

Conversation

@robobun

@robobun robobun commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Problem

const server = Bun.serve({
  port: 0,
  fetch: () => new Response(Buffer.alloc(8 * 1024 * 1024, 'a')),
});
// raw net client: socket.end('GET / HTTP/1.1\r\nHost: x\r\n\r\n')
// expected: body bytes 8388608
// actual:   body bytes 2621440 (truncated at the first kernel send), then connection drops

A raw-socket client that sends its request with socket.end(...) (half-closes its write side) receives only what the kernel accepted on the first send(); the rest of the response body is dropped. Same result for a static routes: { '/': new Response(bigBuffer) } and for Bun.serve({ tls }).

Cause

HttpContext::onOpen<false> does not set s->flags.allow_half_open, so loop.c force-closes the socket right after dispatching onEnd<false>, which unconditionally closes. A tryEnd that did not complete holds its tail as HttpResponseData::offset < total with nothing in AsyncSocketData::buffer (it writes with optional=true); that tail is discarded on close.

Fix

  • onOpen: set allow_half_open for every HTTP server socket (the IsNodeHttp guard is lifted; usockets: wait for the TLS spill at the HTTP close gates; drain a large https response after peer FIN #35109 already dropped the !SSL guard). onEnd closes in its fall-through so this is not a behavior change on its own.
  • onEnd<false>: defer close only when the response is already fully determined: a tryEnd tail (HTTP_END_CALLED set by the content-length internalEnd path while HTTP_RESPONSE_PENDING is still set), or a completed response that has not fully drained. The connection shuts down from the existing shouldCloseConnection() gates once those bytes have drained. A streaming body the application is still producing (HTTP_END_CALLED clear, HTTP_RESPONSE_PENDING set) closes here as before so onAborted / request.signal fires on client disconnect; the onWritable slot being armed is not treated as pending output because do_render_stream keeps it armed for a streaming response's lifetime regardless of backpressure. The drain checks use hasFullyDrained() (from usockets: wait for the TLS spill at the HTTP close gates; drain a large https response after peer FIN #35109), which accounts for the TLS ciphertext spill, so the defer is accurate for both transports.
  • onWritable: close a deferred connection on a zero-progress writable event after FIN, for both deferred shapes: a stuck buffered flush (flushed == 0, the existing node:http check lifted out of its IsNodeHttp guard) and a tryEnd retry whose offset did not advance.

Scope

The sendfile path is intentionally left for follow-up: new Response(Bun.file(big)) bytes are driven by FileResponseStream directly on the fd rather than handed to uWS, and uws_res_prepare_for_sendfile does not set HTTP_END_CALLED (only uws_res_end_sendfile does, at completion), so the defer would need a new state bit. Behavior unchanged from main.

Relation to #35034 / #35109

#35034 (merged) is the node:http sibling and is scoped if constexpr (IsNodeHttp); this PR is the Bun.serve (!IsNodeHttp) side. #35109 (merged) added hasFullyDrained() and made the shouldCloseConnection() close gates wait for the TLS ciphertext spill; this PR's onEnd<false> defer and onWritable zero-progress check use that helper so both transports are covered.

Verification

New describe in test/js/bun/http/serve.test.ts (requests send no Connection: close, so the post-drain shutdown is driven by the HTTP_NODE_RECEIVED_FIN clause of shouldCloseConnection()):

  • fetch-handler, static-route, and https fetch-handler tryEnd-tail cases receive 2621440 bytes on main and the full 8 MiB with the fix.
  • a destroy()-mid-drain case pins the zero-progress close (bounded poll on server.pendingRequests; idleTimeout 60 so a spin misses the deadline rather than being masked by an idle-timeout close). On Windows loopback the whole body fits the kernel send buffer so there is no tryEnd tail; pendingRequests is the portable observable.
  • request.signal still fires on client FIN for a streaming (SSE-shaped) response.

serve.test.ts, node-http-backpressure.test.ts (including #35034's and #35109's cases), bun-serve-static.test.ts, bun-serve-routes.test.ts, bun-serve-file.test.ts, bun-serve-ssl.test.ts pass. 20/20 runs of the new tests on Windows aarch64 (including the HTTPS case).


no test proof · iteration 6 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/serve.test.ts

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

The HTTP server now handles peer FIN during response draining, closes stalled sockets, preserves streaming abort behavior, and adds raw TCP regression tests for complete responses and no-spin shutdown.

HTTP half-close lifecycle

Layer / File(s) Summary
FIN state and response-end handling
packages/bun-uws/src/HttpContext.h
Clarifies non-TLS half-open semantics and records Bun.serve FIN states for pending or buffered responses.
Writable drain closure behavior
packages/bun-uws/src/HttpContext.h
Closes sockets when flushing makes no progress after FIN or when a pending response has no remaining output.
Half-close and disconnect regression coverage
test/js/bun/http/serve.test.ts
Tests complete 8 MiB responses, connection closure, no-spin shutdown, and streaming request aborts after client FIN.

Possibly related PRs

  • oven-sh/bun#32488: Changes related HttpContext.h FIN and half-close compatibility handling.
  • oven-sh/bun#34161: Covers peer half-close behavior that can cause teardown hangs or writable spinning.
  • oven-sh/bun#35034: Adjusts onWritable and onEnd FIN handling to prevent loops and truncated responses.

Suggested reviewers: jarred-sumner, cirospaciari

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly reflects the main change: draining a tryEnd tail before closing after peer FIN.
Description check ✅ Passed The description covers the problem, cause, fix, scope, and verification, satisfying the template's intent.

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

@robobun

robobun commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 7:17 PM PT - Jul 22nd, 2026

@autofix-ci[bot], your commit 1c5df3c1fcc09ded41999735884317f533d00581 passed in Build #78189! 🎉


🧪   To try this PR locally:

bunx bun-pr 35088

That installs a local version of the PR into your bun-35088 executable, so you can run:

bun-35088 --bun

Comment thread packages/bun-uws/src/HttpContext.h Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing the SSL gate — the unconditional allow_half_open plus the HTTPS tls.connect test cover that variant now. I didn't find further issues in 9dce9c1, but this changes connection-close semantics on every Bun.serve socket (and now HTTPS too), so it's worth a human look.

Checked: the onEnd<false> defer is gated on HTTP_END_CALLED so streaming bodies still fire onAborted (covered by the SSE test); the zero-progress close in onWritable is guarded by HTTP_NODE_RECEIVED_FIN so it can't fire on ordinary backpressure; shouldCloseConnection() already includes HTTP_NODE_RECEIVED_FIN so the deferred connection shuts down once the tail drains.

Extended reasoning...

Overview

Changes packages/bun-uws/src/HttpContext.h to set allow_half_open = 1 on every HTTP server socket (previously node:http-only, non-SSL-only), adds a Bun.serve-specific defer branch in onEnd<false> for a tryEnd tail or a buffered-but-complete response, and adds zero-progress-after-FIN detection in onWritable (both the buffered-flush path and the tryEnd-retry path). HttpResponseData.h only updates the HTTP_NODE_RECEIVED_FIN comment. Five new tests in serve.test.ts cover fetch handler, static route, HTTPS, mid-drain destroy, and the streaming-body abort guard.

Security risks

None identified. The concern with half-open is resource retention — a peer that FINs then stops reading. The PR handles this: the zero-progress check in onWritable closes on EPIPE, resetTimeout() is still called on each writable event so idle timeout remains the backstop, and the defer only applies when the response is fully determined (bounded output). No auth/crypto/parsing surface touched.

Level of scrutiny

High. This is the socket-lifecycle core of Bun.serveonOpen/onEnd/onWritable run for every connection, and the allow_half_open change now also applies to HTTPS and node:http-over-TLS sockets (the if constexpr (!SSL) gate that previously scoped it was dropped in the follow-up commit). Getting close-on-FIN semantics wrong can leak sockets or truncate responses across the whole server. This is well outside the "simple/mechanical" bar for auto-approval.

Other factors

My prior review flagged the stale !SSL premise; 9dce9c1 addressed it by dropping the gate, correcting the comment to reference the openssl.c paths that honor the flag, and adding an HTTPS test. The state-flag gating (HTTP_END_CALLED && HTTP_RESPONSE_PENDING for tryEnd tail vs. !PENDING && bufferedAmount > 0 for done-but-buffered) reads correctly against internalEnd's flag writes, and the SSE test guards the streaming-body abort path. The PR description lists the relevant regression suites as passing. Deferring because the change is behaviorally broad, not because anything looks wrong.

Comment thread packages/bun-usockets/src/crypto/openssl.c Outdated
Comment thread packages/bun-uws/src/HttpContext.h
Comment thread packages/bun-usockets/src/crypto/openssl.c Outdated
Comment thread test/js/bun/http/serve.test.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/bun/http/serve.test.ts`:
- Around line 3516-3521: Condense the comments at test/js/bun/http/serve.test.ts
lines 3516-3521 and 3575-3582 to three lines or fewer each, preserving the
half-close response-draining invariant and the portability rationale; no code
behavior changes are needed.
- Around line 3542-3552: Update the socket wait helpers in
test/js/bun/http/serve.test.ts at lines 3542-3552, 3593-3601, and 3624-3632 to
reject their connect, close, drain, or data promises when an unexpected socket
error occurs. Preserve the intentional destroy() handling at lines 3593-3601,
ensuring errors are rejected only before that expected teardown.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 965bf566-b05e-4fb9-88f5-4026349b4b95

📥 Commits

Reviewing files that changed from the base of the PR and between 74c91fd and 3b99acc.

📒 Files selected for processing (3)
  • packages/bun-uws/src/HttpContext.h
  • packages/bun-uws/src/HttpResponseData.h
  • test/js/bun/http/serve.test.ts

Comment thread test/js/bun/http/serve.test.ts
Comment thread test/js/bun/http/serve.test.ts
@robobun
robobun force-pushed the farm/fe9f9f24/serve-half-close-drain branch from 3b99acc to 0011b71 Compare July 22, 2026 09:28

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/bun-uws/src/HttpContext.h`:
- Around line 219-220: Update the SSL branches in the HttpContext connection
lifecycle, including the allow_half_open setup near the non-TLS condition and
the FIN-deferral logic around ssl_spill, so HTTPS client FINs are deferred until
the response drains. Ensure ssl_spill is drained and accounted for consistently
with the non-TLS path, then add regression coverage proving HTTPS responses
survive a client half-close.

In `@test/js/bun/http/serve.test.ts`:
- Around line 3597-3601: Update the test around the socket data callback to
create a promise that resolves when the callback runs and destroys the socket,
then await that promise after dispatched.promise and before polling
pendingRequests. Ensure the test cannot pass unless the client reaches the
mid-drain destroy path, while preserving the existing deadline and
pendingRequests assertion.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0810f033-a8f6-4d6d-869d-2849af1ec9e5

📥 Commits

Reviewing files that changed from the base of the PR and between 3b99acc and 0011b71.

📒 Files selected for processing (2)
  • packages/bun-uws/src/HttpContext.h
  • test/js/bun/http/serve.test.ts

Comment thread packages/bun-uws/src/HttpContext.h Outdated
Comment thread test/js/bun/http/serve.test.ts
Comment thread packages/bun-uws/src/HttpContext.h Outdated
@robobun

robobun commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator Author

CI build 78189 (1c5df3c, rebased on #35109): the new serve.test.ts half-close tests passed on every lane including the https case. 194/196 jobs passed (2 still scheduled); every test failure is retry-flaky and unrelated (install registry hoisting, webview-chrome, bun-run-dir, http-server-connections-checking-leak, fs-promises readFile, repl-close). No [new] failures. Diff is green; ready for review.

Jarred-Sumner pushed a commit that referenced this pull request Jul 22, 2026
…ge https response after peer FIN (#35109)

### Problem

```js
import https from 'node:https';
const server = https.createServer({ key, cert }, (req, res) => {
  res.writeHead(200, { 'content-length': String(8 * 1024 * 1024) });
  res.write(Buffer.alloc(8 * 1024 * 1024, 'a'));
  res.end();
});
// raw tls client: socket.end('GET / HTTP/1.1\\r\\nHost: x\\r\\n\\r\\n')
// Node: body bytes 8388608
// Bun:  body bytes 2637824 (truncated at the first kernel-accepted batch)
```

A node:https server responding with a body that backpressures truncates
at the first kernel-accepted write when the client half-closes right
after its request. Same for `res.end(bigBuffer)`. Node.js delivers the
full body.

### Cause

Two server-side layers and one Windows-only client-side layer:

* **TLS spill not counted at the close gates**:
`us_internal_ssl_write()` seals plaintext into 16 KB TLS records and
flushes them to the kernel in ~128 KB batches. A partial kernel write
parks the remainder of the batch in the loop's `ssl_spill` slot and
returns the full plaintext count as written, so
`AsyncSocket::getBufferedAmount()` (which reads only
`AsyncSocketData::buffer`) reports 0 while up to one batch of ciphertext
is still in userspace. The `shouldCloseConnection()` close gates in
`HttpResponse::internalEnd` / `HttpResponse::cork` /
`HttpContext::onData` / `HttpContext::onWritable` key on
`getBufferedAmount() == 0` and so fire early;
`us_internal_ssl_close(code=0)` does one best-effort drain and frees the
rest.
* **`allow_half_open` gated on `!SSL` for node:http**:
`HttpContext::onOpen` only set `allow_half_open` for non-TLS
`IsNodeHttp` sockets, so a client FIN on a node:https connection made
`us_internal_ssl_on_end` force-close the socket right after dispatching
`onEnd`, discarding the buffered response. `onEnd<IsNodeHttp>`'s
existing defer was never reached. #35034 fixed the plain-TCP case; the
TLS side was left gated out because the spill made the close gates
unsafe.
* **Windows eof-drain** (surfaced by the new test's client side):
`poll_cb` (libuv.c) maps AFD `UV_DISCONNECT` to the eof hint for a
socket whose write side we already shut down. AFD reports DISCONNECT
while the tail of the peer's stream is still queued in the kernel, but
the Windows branch of `loop.c`'s read loop only did one extra `recv()`
(the RST probe) before falling through to the `is_shut_down` raw-close,
discarding the rest. Reproduces on released bun against a Node.js
server: a half-closed `net.Socket`/`tls` client intermittently loses the
end of a large response on Windows only.

### Fix

* **`us_socket_ssl_spill_pending()`** (openssl.c / socket.c /
libusockets.h): ciphertext bytes already sealed for this socket and
reported as written by `us_socket_write()`, still waiting on a writable
event. Returns 0 for plain-TCP sockets.
* **`AsyncSocket::hasFullyDrained()`**: `buffer.length() == 0 &&
spill_pending == 0`. The HTTP close-after-drain gates
(`HttpResponse::internalEnd` / `cork`, `HttpContext::onData` tail /
`onWritable` / `onEnd<IsNodeHttp>`) switch from `getBufferedAmount() ==
0` to this. `getBufferedAmount()` itself is unchanged so WebSocket's
`maxBackpressure` policy and the JS-exposed `bufferedAmount` stay a
plaintext count. The spill is bounded (≤ one 128 KB batch) and
`us_internal_ssl_on_writable` drains it before dispatching the
user-level writable, so the existing drain loops terminate: once
`AsyncSocketData::buffer` empties, the next writable event drains the
final spill and the close gate fires with nothing pending.
* **`HttpContext::onOpen<IsNodeHttp>`**: drop the `!SSL` guard on
`allow_half_open`. `us_internal_ssl_on_end` already honours the flag;
`onEnd<IsNodeHttp>`'s `hasQueuedOutgoing` now accounts for the spill,
and `onWritable`'s zero-progress-after-FIN close is not gated on `!SSL`.
* **`us_internal_ssl_on_writable`**: release a zero-progress spill once
the peer's readable side has ended, so a FIN-then-RST client does not
wedge the writable dispatch before the close gate is reached (the drain
would otherwise re-arm writable on a send() that keeps failing). #34510
is the general fix for stuck TLS sends; this is the narrow case the new
`allow_half_open` path opens.
* **`loop.c` Windows read loop**: drain on the eof hint like the POSIX
branch already does (`recv()` returning 0 or `WSAEWOULDBLOCK` ends the
loop, bounded by the kernel receive buffer).

### Relation to #35088

\#35088 is the `Bun.serve` (`!IsNodeHttp`) sibling and is currently
gated on `!SSL` because the spill was invisible to its `onEnd` defer's
`doneButBuffered` check and to `internalEnd`'s close gate (its scope
note says so). With `hasFullyDrained()` at those gates that PR's `onEnd`
defer is accurate for TLS too, so it can drop its `!SSL` gates
(switching its `getBufferedAmount() > 0` to `!hasFullyDrained()`).

### Verification

New `describe('https')` in
`test/js/node/http/node-http-backpressure.test.ts` mirrors the existing
plain-HTTP half-close tests over TLS for `res.write()+res.end()`,
`res.end(payload)` (the `optional=false` `internalEnd` buffer path), and
`httpAllowHalfOpen` with `res.end()` after drain; each receives ~2.6 MB
on main and the full 8 MiB with the fix, matching Node.js. Looped 5× per
case so the on_writable drain cycle is exercised past the first
kernel-accepted write; the loop also covers the Windows client-side
eof-drain. A fourth test half-closes then destroys the client after
first data and asserts the server-side socket `'close'` fires (would
wedge on a stuck spill).

Verified on linux-x64 (14/14) and windows-x64 (14/14, 5 consecutive runs
of the new tests). `node-http-backpressure.test.ts`,
`node-http-pinned-write.test.ts`,
`node-http-server-socket-end-drain.test.ts`, `bun-serve-ssl.test.ts`,
`node-tls-connect.test.ts`, `node-https-checkServerIdentity.test.ts`,
`serve.test.ts`, `socket.test.ts` pass (pre-existing container-only /
debug-timeout failures unchanged from main).

<!-- robobun:evidence:begin -->

---

**[review]** gate passed · iteration 1 · 9 files touched

<details><summary>fails on main (without fix)</summary>

```console
ASAN without fix: 3 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/http/node-http-backpressure.test.ts
bun test v1.4.0 (41c2fbb)

test/js/node/http/node-http-backpressure.test.ts:
(pass) backpressure > should handle backpressure [588.62ms]
(pass) backpressure > Connection: close does not truncate a response that is still flushing > when the client requested the close [400.31ms]
(pass) backpressure > Connection: close does not truncate a response that is still flushing > when the server sets Connection: close on a keep-alive request [190.66ms]
(pass) backpressure > Connection: close does not truncate a response that is still flushing > when the whole body is passed to res.end() [216.37ms]
(pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() then res.end() [179.05ms]
(pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() without res.end() [136.15ms]
(pass) backpressure > a client FIN right after the request does not truncate a response that is still
... (truncated)

release without fix: all passed
bun test v1.4.0-canary.1 (3d1c56b)

test/js/node/http/node-http-backpressure.test.ts:
(pass) backpressure > should handle backpressure [32.49ms]
(pass) backpressure > Connection: close does not truncate a response that is still flushing > when the client requested the close [22.78ms]
(pass) backpressure > Connection: close does not truncate a response that is still flushing > when the server sets Connection: close on a keep-alive request [18.46ms]
(pass) backpressure > Connection: close does not truncate a response that is still flushing > when the whole body is passed to res.end() [11.04ms]
(pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() then res.end() [9.60ms]
(pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() without res.end() [7.83ms]
(pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() then res.end() after drain, httpAllowHalfOpen [6.33ms]
(pass) backpressure > a client FIN right after the request does not truncate a response that is still 
... (truncated)
```

</details>

<details><summary>passes on PR (with fix)</summary>

```console
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/http/node-http-backpressure.test.ts
bun test v1.4.0 (41c2fbb)

test/js/node/http/node-http-backpressure.test.ts:
(pass) backpressure > should handle backpressure [517.49ms]
(pass) backpressure > Connection: close does not truncate a response that is still flushing > when the client requested the close [423.69ms]
(pass) backpressure > Connection: close does not truncate a response that is still flushing > when the server sets Connection: close on a keep-alive request [187.06ms]
(pass) backpressure > Connection: close does not truncate a response that is still flushing > when the whole body is passed to res.end() [172.00ms]
(pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() then res.end() [215.92ms]
(pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() without res.end() [158.36ms]
(pass) backpressure > a client FIN right after the request does not truncate a response that is still
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 738ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[0/25] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

�[1m�[92m   Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
�[1m�[92m   Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno)
�[1m�[92m   Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr)
�[1m�[92m   Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys)
�[1m�[92m   Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety)
�[1m�[92m   Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys)
�[1m�[92m   Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys)
�[1m�[92m   Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd)
�[1m�[92m   Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp)
�[1m�[92m   Compiling�[0m bun_output v0.0.0 (/workspace/bun/src/output)
�[1m�[92m   Compiling�[0m bun_clap v0.0.0 (/workspace/bun/src/clap)
�[1m�[92m   Compiling�[0m bun_valkey v
... (truncated)
```

</details>

<details><summary>diff hotspot</summary>

```
packages/bun-usockets/src/crypto/openssl.c       | 27 +++++++
 packages/bun-usockets/src/internal/internal.h    |  1 +
 packages/bun-usockets/src/libusockets.h          |  5 ++
 packages/bun-usockets/src/loop.c                 | 13 ++++
 packages/bun-usockets/src/socket.c               |  7 ++
 packages/bun-uws/src/AsyncSocket.h               | 15 ++++
 packages/bun-uws/src/HttpContext.h               | 21 +++---
 packages/bun-uws/src/HttpResponse.h              |  6 +-
 test/js/node/http/node-http-backpressure.test.ts | 95 ++++++++++++++++++++++++
 9 files changed, 176 insertions(+), 14 deletions(-)
```

</details>

**gate history** · 3 passed · 0 rejected · iteration 1

<details><summary>evidence per changed file</summary>

```
file                                              reads  edits  tests
packages/bun-usockets/src/crypto/openssl.c            9      4      0
packages/bun-usockets/src/internal/internal.h         1      1      0
packages/bun-usockets/src/libusockets.h               1      1      0
packages/bun-usockets/src/loop.c                      4      1      0
packages/bun-usockets/src/socket.c                    6      1      0
packages/bun-uws/src/AsyncSocket.h                    5      3      0
packages/bun-uws/src/HttpContext.h                    4      7      0
packages/bun-uws/src/HttpResponse.h                   5      3      0
test/js/node/http/node-http-backpressure.test.ts      4      8      0
```

</details>

<!-- robobun:evidence:end -->
@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

@robobun fix conflicts. maybe use the new helper method in the conflicted code

A client that half-closes its write side right after the request (the raw
socket.end(request) pattern) received only what the kernel accepted on the
first send for a large Response(Buffer); the rest of the body was dropped.

onOpen<false> never set allow_half_open, so loop.c force-closed right after
dispatching onEnd<false>, which unconditionally closed. A tryEnd that did not
complete holds its tail as offset < total with nothing in AsyncSocketData::
buffer; that tail was discarded.

Set allow_half_open for every HTTP server socket (the IsNodeHttp guard is
lifted). In onEnd<false>, defer close only when the response is already fully
determined: a tryEnd tail (HTTP_END_CALLED with HTTP_RESPONSE_PENDING still
set), or a completed response that has not fully drained (hasFullyDrained()
accounts for the TLS ciphertext spill). A streaming body the application is
still producing (HTTP_END_CALLED clear) closes here as before so onAborted /
request.signal fires on client disconnect. onWritable closes a deferred
connection on a zero-progress writable event after FIN so it cannot spin the
writable dispatch: the buffered-flush case is the existing node:http check
lifted out of its IsNodeHttp guard, and a tryEnd-retry offset-did-not-advance
check covers the Bun.serve external-tail shape.
@robobun
robobun force-pushed the farm/fe9f9f24/serve-half-close-drain branch from 0011b71 to d88a814 Compare July 22, 2026 21:30
@robobun

robobun commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased on #35109 in d88a814. The onEnd<false> defer and onWritable zero-progress check now use hasFullyDrained(), and the !SSL gate is gone, so TLS is covered too; the HTTPS test is back in and goes 20/20 on Windows aarch64 (where it was flaking before). allow_half_open is now set for every HTTP server socket.

Comment thread packages/bun-uws/src/HttpContext.h

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues found in the post-rebase diff, but this reworks the Bun.serve connection-close state machine (onOpen/onEnd/onWritable half-open handling) in a way that's subtle enough to warrant a human look — especially given it flipped TLS in → out → in across the review.

What was reviewed:

  • onEnd defer conditions: checked that HTTP_END_CALLED && HTTP_RESPONSE_PENDING correctly identifies only the tryEnd content-length tail, and that streaming bodies (HTTP_END_CALLED clear) still fall through to close() so onAborted/request.signal fires — the new SSE test pins this.
  • onWritable zero-progress guard: the lifted flushed == 0 && RECEIVED_FIN check and the new offset == offsetBefore tryEnd-tail check both close on EPIPE rather than spinning; hasFullyDrained() covers the TLS spill per #35109.
  • Tests: half-close cases await socket close (fires regardless of ECONNRESET), the mid-drain destroy uses a bounded pendingRequests poll with idleTimeout 60 so a spin misses the deadline, and the https variant is back with 20/20 Windows aarch64 runs reported.
Extended reasoning...

Overview

Two files: packages/bun-uws/src/HttpContext.h (onOpen, onWritable, onEnd) and test/js/bun/http/serve.test.ts (a 5-case describe block). The fix lifts allow_half_open = 1 out of the IsNodeHttp guard so every HTTP server socket survives peer FIN at the loop level, then teaches onEnd<false> to defer close when a fully-determined response (tryEnd tail or done-but-buffered) still has bytes to drain, and teaches onWritable to close on a zero-progress writable after FIN so a deferred connection cannot spin. The rebase onto #35109 replaced the earlier !SSL gates with hasFullyDrained(), which counts the TLS ciphertext spill, so both transports are covered.

Security risks

None identified. The change only defers close for responses whose full byte count is already determined (HTTP_END_CALLED), and the existing idle-timeout plus the new zero-progress close prevent a half-closed peer from holding a connection open indefinitely. Streaming bodies still abort on FIN as before, so there is no new resource-hold vector.

Level of scrutiny

High. This is the connection-lifecycle state machine for every Bun.serve socket — a wrong condition here either truncates responses (the bug being fixed) or leaks connections / breaks abort signaling. The PR already went through one TLS scope reversal (in → out → back in after #35109), and Jarred has engaged directly. The logic is well-commented and the tests cover the tryEnd tail (http/https/static), the EPIPE spin guard, and the streaming-abort negative case, but the interaction between HTTP_END_CALLED, HTTP_RESPONSE_PENDING, shouldCloseConnection() gates, and the sendfile scope-out is the kind of thing a maintainer should sign off on.

Other factors

All prior review threads (mine on the stale PR body, coderabbit on TLS scope, error swallowing, comment length, mid-drain assertion) are resolved. The PR body now matches the diff. CI on the pre-rebase revision was green modulo known main-break flakes; the post-rebase build has an autofix.ci commit on top. No CODEOWNER file gates this path. The sendfile sibling is explicitly scoped out with a stated reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants