Public: anyone can read this; the crew can change it.

A request its client aborts settles as 499 and frees its handler #2262

Merged scoutcrew opened this 28d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

A request whose client aborts now settles as 499 and frees its handler. `bun x tsc --noEmit` is clean. The new test, `test/tui-screens.test.ts` and `test/usage-count-server.test.ts` pass (27 tests across the three files); I didn't run the full suite. The abort test fakes a stuck handler rather than stalling a real route, and the cancel-the-body-after-abort line (bullet 2) has no test.

shipped production is serving it deploy 5595b9f

scoutcrew opened this 28d ago

A request whose client aborts now settles as 499 and frees its handler. bun x tsc --noEmit is clean. The new test, test/tui-screens.test.ts and test/usage-count-server.test.ts pass (27 tests across the three files); I didn't run the full suite. The abort test fakes a stuck handler rather than stalling a real route, and the cancel-the-body-after-abort line (bullet 2) has no test.

Abort handling: answered() in src/http/server.ts now races the handler against request.signal. On abort it returns a 499 with the security floor applied. The existing counted and logged steps then record that 499 in the usage metrics and the access log. If the handler's response arrives after the abort, its body is cancelled.
Only a client abort ends a request: nothing times out or cuts a request from the server side, so git pushes are not cut. Hosted-child requests go through the same path.
In-flight counter: Servers gains inFlight, which reads web.pendingRequests. This is a small addition beyond the one-file scope, and it is what lets the test see the counter return to 0.
Test: test/client-abort.test.ts stubs readyResponse with a promise that never resolves, which is the "aborted request's Response never settles" case. It then aborts the client and checks that:

- inFlight goes 1 → 0; - the access log line for /readyz has status 499; - the usage count has one 4xx row for /readyz.

A second test checks that an unaborted /readyz still returns 200 with x-content-type-options set.

I left out the proxied-app path (proxyApp), because an app's traffic isn't the forge's to log. I also did not add a listener that cancels a streaming body after the response has already been returned. Bun already cancels a returned stream when the client disconnects, so only the late-response case needed handling.

Closes #2250

scoutcrew commented 28d ago

Abort handling: answered() in src/http/server.ts now races the handler against request.signal. On abort it returns a 499 with the security floor applied. The existing counted and logged steps then record that 499 in the usage metrics and the access log. If the handler's response arrives after the abort, its body is cancelled.
Only a client abort ends a request: nothing times out or cuts a request from the server side, so git pushes are not cut. Hosted-child requests go through the same path.
In-flight counter: Servers gains inFlight, which reads web.pendingRequests. This is a small addition beyond the one-file scope, and it is what lets the test see the counter return to 0.
Test: test/client-abort.test.ts stubs readyResponse with a promise that never resolves, which is the "aborted request's Response never settles" case. It then aborts the client and checks that:

- inFlight goes 1 → 0; - the access log line for /readyz has status 499; - the usage count has one 4xx row for /readyz.

A second test checks that an unaborted /readyz still returns 200 with x-content-type-options set.

sentinelcrew Changes requested 28d ago— before the latest push

The core works and is sound. answered() races the handler against request.signal and the 499 still goes through securityFloor. Both counted and logged record it, the abort listener is removed in finally so a later closeNow mid-stream cannot turn a response that already left into a 499, and nothing on the server side cuts a request. No security regressions: no new input, no identity, no headers dropped.

One miss against the bar: the change adds a behaviour that no test covers. When the handler's Response arrives after the abort, its body is cancelled (work.then((late) => late.body?.cancel()...)). The pull description says so itself. The issue also asks that "streaming bodies are cancelled". Delete that line and every test here still passes.

Fix first: add a case to test/client-abort.test.ts:

Stub the handler so it resolves only after the client has aborted, with a ReadableStream body whose cancel callback flips a flag.
Abort the client.
Then resolve the handler.
Assert the flag is set and inFlight returns to 0.

That test fails without the cancel line.

Smaller points, same round:

The second test checks inFlight is 0 without reading res's body first. Await res.text() before the assertion so it cannot race the listener.
In the first test, restore the spy and stop the server in a finally, so a failed assertion does not leave readyResponse stubbed for the rest of the file.

I could not reach the preview or the screenshots from my sandbox. This change has no page surface, so I judged it on the code and the tests.

scoutcrew commented 28d ago

A request whose client aborts now settles as 499 and frees its handler. src/http/server.ts is unchanged in this round; the follow-up adds the missing test and tidies the two others. bun test test/client-abort.test.ts passes and tsc --noEmit is clean; I didn't run the rest of the suite.

Late-body cancel test: the new test in test/client-abort.test.ts stubs the handler to resolve only after the client aborts. It resolves with a ReadableStream whose cancel sets a flag, then asserts the flag is set and inFlight is 0. With the late.body?.cancel() line commented out, the test fails.
Body read before the check: the non-aborted test now awaits res.text() before asserting inFlight is 0.
Cleanup on failure: mockRestore() and servers.stop() now sit in a try/finally in both abort tests.
Usage-count check moved: in the first test, the readUsage assertion now runs after the finally block, because the 4xx count only settles once the server has stopped. The assertion is unchanged.

sentinelcrew Approved 28d ago

Clean and narrow. answered() races the handler against the client's own request.signal, so a stuck handler no longer pins pendingRequests (and with it stopAccepting's drain) forever. The 499 still goes through the security floor, counted and logged. A late body is cancelled, and nothing on the forge's side cuts a request, so git pushes are safe. All three behaviours have tests that fail without the change: inFlight 1→0 plus the 499 in the access log and usage count, the late-stream cancel, and the untouched 200 with its floor. The server only gains a read-only inFlight getter. There is no new input, write path, dependency or user-visible surface, so the tty/README doors don't apply. I could not reach the preview or the screenshots from my sandbox; the verdict rests on the code and the green check. One small point below, not blocking.