From 62fa33c5dd709d46b3c562967a305db81a5a131a Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Wed, 20 May 2026 04:18:29 -0700 Subject: [PATCH 1/2] feat: expose request.signal AbortSignal, aborted on client disconnect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #513. Construct an AbortController per Request and expose `request.signal` so Resource methods can forward cancellation to external work — most concretely `scope.models.generateStream(input, { signal })` in #510. Without a forwardable signal, a generator body whose client has already gone away keeps tokenizing upstream, wasting tokens and GPU time. - Node HTTP/SSE path: hook `nodeResponse.on('close', ...)` from the Request constructor, guarded by `!writableFinished` so clean completion does not abort. - WebSocket-upgrade path: the Request is created without a response in `http.ts` (the WS connection upstream); abort from the `ws.on('close')` hook in `REST.ts` via a new `_abort()` method, and also wire `nodeRequest.socket.once('close')` as a fallback when no response is present. - Bun runtime: surface the Web Request's native `signal`, which already aborts on client disconnect under `Bun.serve()`. - `isAborted` now reflects the live signal state (previously TODO). - Constructor's `nodeResponse` argument is now optional to match the WS-upgrade call site in `http.ts:929`, which has always passed one argument; the optional + duck-typed `on` guard preserves all existing test mocks that pass `{}` as the response. Unit tests cover: initial unaborted state, close-before-finish aborts, close-after-finish does not abort, socket-close abort on WS-upgrade path, and explicit `_abort()`. Co-Authored-By: Claude Opus 4.7 (1M context) --- server/REST.ts | 1 + server/serverHelpers/Request.ts | 34 +++++++++-- .../server/serverHelpers/Request.test.js | 59 ++++++++++++++++++- 3 files changed, 89 insertions(+), 5 deletions(-) diff --git a/server/REST.ts b/server/REST.ts index 3791cde78e..e079f421f4 100644 --- a/server/REST.ts +++ b/server/REST.ts @@ -340,6 +340,7 @@ export function handleApplication(scope: import('../components/Scope.ts').Scope) recordActionBinary(!hasError, 'connection', 'ws', 'disconnect'); incomingMessages.emit('close'); if (iterator) iterator.return(); + request._abort?.(); }); try { await chainCompletion; diff --git a/server/serverHelpers/Request.ts b/server/serverHelpers/Request.ts index 6726d23f0b..748668a5ce 100644 --- a/server/serverHelpers/Request.ts +++ b/server/serverHelpers/Request.ts @@ -27,6 +27,7 @@ interface IncomingMessage extends NodeIncomingMessage { export class Request { #body: RequestBody | undefined; #peerCertificate: any; + #abortController = new AbortController(); public _nodeRequest: IncomingMessage; public _nodeResponse?: NodeServerResponse; public method: string; @@ -60,7 +61,7 @@ export class Request { public lastModified?: number; public lastRefreshed?: number; - constructor(nodeRequest: IncomingMessage, nodeResponse: NodeServerResponse) { + constructor(nodeRequest: IncomingMessage, nodeResponse?: NodeServerResponse) { this.method = nodeRequest.method; const url = nodeRequest.url; this._nodeRequest = nodeRequest; @@ -68,6 +69,26 @@ export class Request { this.url = url; this.headers = new RequestHeaders(nodeRequest.headers); this.__harperRequestUpgraded = false; + // Abort the request's signal on premature client disconnect. nodeResponse 'close' + // also fires on clean completion; the writableFinished guard restricts to disconnect. + if (typeof nodeResponse?.on === 'function') { + nodeResponse.on('close', () => { + if (!nodeResponse.writableFinished) this.#abortController.abort(); + }); + } else if (typeof nodeRequest.socket?.once === 'function') { + // WebSocket-upgrade path: no response. The TCP socket is the only signal. + nodeRequest.socket.once('close', () => this.#abortController.abort()); + } + } + get signal(): AbortSignal { + return this.#abortController.signal; + } + /** + * Abort this request's signal. Used by transports (e.g. WebSocket) that need to + * signal client-side cancellation independently of the Node response lifecycle. + */ + _abort(): void { + this.#abortController.abort(); } get absoluteURL() { return this.protocol + '://' + this.host + this.url; @@ -119,8 +140,7 @@ export class Request { return this._nodeRequest.httpVersion; } get isAborted() { - // TODO: implement this - return false; + return this.#abortController.signal.aborted; } // Expose node request for cases that need direct access (e.g., replication) get nodeRequest() { @@ -391,7 +411,13 @@ export class BunRequest { return '1.1'; } get isAborted() { - return false; + return this._webRequest.signal?.aborted ?? false; + } + get signal(): AbortSignal { + return this._webRequest.signal; + } + _abort(): void { + // On Bun, abort is driven by the underlying Web Request's signal; no-op for parity with Node path. } get nodeRequest() { return null; diff --git a/unitTests/server/serverHelpers/Request.test.js b/unitTests/server/serverHelpers/Request.test.js index a640b2ab2f..b35ca94c46 100644 --- a/unitTests/server/serverHelpers/Request.test.js +++ b/unitTests/server/serverHelpers/Request.test.js @@ -260,11 +260,68 @@ describe('Request class', function () { }); it('should return isAborted status', function () { - // Currently always returns false (TODO in implementation) assert.strictEqual(request.isAborted, false); }); }); + describe('signal (AbortSignal)', function () { + const { EventEmitter } = require('node:events'); + + function makeNodeRequest() { + return { + method: 'GET', + url: '/test', + headers: {}, + socket: Object.assign(new EventEmitter(), { + encrypted: true, + remoteAddress: '127.0.0.1', + getPeerCertificate: sinon.stub().returns({}), + }), + }; + } + + function makeNodeResponse({ writableFinished = false } = {}) { + const res = new EventEmitter(); + res.writableFinished = writableFinished; + return res; + } + + it('exposes a live AbortSignal that is not initially aborted', function () { + const request = new Request(makeNodeRequest(), makeNodeResponse()); + assert.ok(request.signal instanceof AbortSignal); + assert.strictEqual(request.signal.aborted, false); + assert.strictEqual(request.isAborted, false); + }); + + it('aborts the signal on nodeResponse close before write is finished', function () { + const nodeResponse = makeNodeResponse({ writableFinished: false }); + const request = new Request(makeNodeRequest(), nodeResponse); + nodeResponse.emit('close'); + assert.strictEqual(request.signal.aborted, true); + assert.strictEqual(request.isAborted, true); + }); + + it('does NOT abort the signal on nodeResponse close after write finishes', function () { + const nodeResponse = makeNodeResponse({ writableFinished: true }); + const request = new Request(makeNodeRequest(), nodeResponse); + nodeResponse.emit('close'); + assert.strictEqual(request.signal.aborted, false); + }); + + it('aborts on socket close when no nodeResponse is provided (WebSocket-upgrade path)', function () { + const nodeRequest = makeNodeRequest(); + const request = new Request(nodeRequest); + nodeRequest.socket.emit('close'); + assert.strictEqual(request.signal.aborted, true); + }); + + it('_abort() aborts the signal explicitly', function () { + const request = new Request(makeNodeRequest(), makeNodeResponse()); + request._abort(); + assert.strictEqual(request.signal.aborted, true); + }); + }); + describe('body getter', function () { it('should create RequestBody instance lazily', function () { const mockNodeRequest = { From 166e0db177fd5b3b7657e8b4c6a632b2c2dd4816 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Wed, 20 May 2026 04:29:45 -0700 Subject: [PATCH 2/2] docs: clarify dual-abort and Bun WS signal limitations Post cross-model review feedback: add comments explaining (1) why the socket fallback in Request constructor coexists with REST.ts's ws.on('close') hook (idempotent abort, defense for any future single-arg construction), and (2) that Bun's request.signal abort semantics on WebSocket upgrade are implementation-defined and would need an explicit Bun-side AbortController if guaranteed. Co-Authored-By: Claude Opus 4.7 (1M context) --- server/serverHelpers/Request.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/server/serverHelpers/Request.ts b/server/serverHelpers/Request.ts index 748668a5ce..99c5212aa0 100644 --- a/server/serverHelpers/Request.ts +++ b/server/serverHelpers/Request.ts @@ -76,7 +76,12 @@ export class Request { if (!nodeResponse.writableFinished) this.#abortController.abort(); }); } else if (typeof nodeRequest.socket?.once === 'function') { - // WebSocket-upgrade path: no response. The TCP socket is the only signal. + // No response on this Request — typically the WebSocket-upgrade path + // (http.ts creates the Request before the ws library takes over). The TCP + // socket close is the fallback abort trigger; REST.ts's ws.on('close') hook + // also calls _abort() and is the primary signal in the WS case. The two + // are redundant by design (idempotent abort) so any future single-arg + // caller still gets disconnect semantics without relying on the WS layer. nodeRequest.socket.once('close', () => this.#abortController.abort()); } } @@ -414,6 +419,10 @@ export class BunRequest { return this._webRequest.signal?.aborted ?? false; } get signal(): AbortSignal { + // Bun.serve() aborts this signal on HTTP client disconnect. Behavior on + // WebSocket-upgrade is implementation-defined; if the WS path needs + // guaranteed abort-on-close under Bun, wire it through Bun's ws close + // handler with a Bun-side AbortController, similar to REST.ts on Node. return this._webRequest.signal; } _abort(): void {