From 67f74c5fb9a34e1f2fadf0d122467d7b287d2984 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Wed, 11 Nov 2026 17:28:23 +0800 Subject: [PATCH] fix(setup): retain Hopper operation failure attribution (#9958) * fix(setup): retain Hopper operation failure attribution * fix(setup): retain falsy Hopper abort reasons from body reads After response headers, Node's fetch body reader rejects a null, false, 0, or empty-string abort with a fresh AbortError instead of that reason. Replace that substituted error with the caller's reason. A body AbortError while the caller has not aborted still propagates. * fix(setup): preserve independent response body aborts * fix(setup): identify aborts through owned transport --------- Co-authored-by: Cursor Agent --- src/application/HopperDownload.test.ts | 180 +++++++++++++++++++ src/application/HopperDownload.ts | 43 ++++- src/application/LinuxHopper.test.ts | 66 +++++++ src/application/LinuxHopper.ts | 15 +- src/application/LinuxHopperPublisher.test.ts | 50 ++++++ src/application/LinuxHopperPublisher.ts | 23 ++- src/application/MacHopper.test.ts | 54 ++++++ src/application/MacHopper.ts | 5 +- 8 files changed, 414 insertions(+), 22 deletions(-) create mode 100644 src/application/HopperDownload.test.ts create mode 100644 src/application/LinuxHopperPublisher.test.ts diff --git a/src/application/HopperDownload.test.ts b/src/application/HopperDownload.test.ts new file mode 100644 index 0000000000..87a6f1e79b --- /dev/null +++ b/src/application/HopperDownload.test.ts @@ -0,0 +1,180 @@ +import { createServer } from "node:http"; +import { once } from "node:events"; + +import { afterEach, expect, it, vi } from "vitest"; + +import { downloadHopperPackage } from "./HopperDownload.js"; + +afterEach(() => { + vi.unstubAllGlobals(); + vi.useRealTimers(); +}); + +it.each(["physical", "caller", "node-wrapper", "default-abort"] as const)( + "attributes fetch rejection to the operation that failed (%s)", + async (kind) => { + const controller = new AbortController(); + const reason = { caller: "stop download" }; + const physical = new TypeError("network read failed"); + const wrapped = Object.assign( + new Error("operation aborted", { cause: reason }), + { + name: "AbortError", + code: "ABORT_ERR", + }, + ); + let thrown: unknown; + vi.stubGlobal("fetch", async () => { + if (kind === "default-abort") controller.abort(); + else controller.abort(reason); + thrown = + kind === "physical" + ? physical + : kind === "node-wrapper" + ? wrapped + : controller.signal.reason; + throw thrown; + }); + const download = downloadHopperPackage("https://fixture.invalid/archive", { + signal: controller.signal, + }); + if (kind === "physical") + expect(await download).toMatchObject({ + ok: false, + error: { kind: "network", detail: physical.message }, + }); + else await expect(download).rejects.toBe(thrown); + }, +); + +it.each([false, true])( + "attributes an expired download deadline to its own rejection (%s)", + async (interrupted) => { + vi.useFakeTimers(); + vi.stubGlobal("fetch", async (_url: string, options: RequestInit) => { + await new Promise((resolve) => setTimeout(resolve, 2)); + throw interrupted + ? options.signal?.reason + : new TypeError("physical network failure"); + }); + const download = downloadHopperPackage("https://fixture.invalid/archive", { + timeoutMs: 1, + }); + await vi.advanceTimersByTimeAsync(2); + expect(await download).toMatchObject({ + ok: false, + error: interrupted + ? { kind: "timeout", limit: "deadline", timeoutMs: 1 } + : { kind: "network", detail: "physical network failure" }, + }); + }, +); + +it.each([null, false, 0, ""] as const)( + "retains a falsy caller abort reason when the response body is cancelled (%j)", + async (reason) => { + const controller = new AbortController(); + vi.stubGlobal( + "fetch", + async (_url: string, options: RequestInit) => + new Response( + new ReadableStream({ + pull(stream) { + controller.abort(reason); + stream.error(options.signal?.reason); + }, + }), + { status: 200 }, + ), + ); + await expect( + downloadHopperPackage("https://fixture.invalid/archive", { + signal: controller.signal, + }), + ).rejects.toBe(reason); + }, +); + +it("does not treat an AbortError body failure as cancellation when the caller did not abort", async () => { + const failure = new DOMException("The operation was aborted", "AbortError"); + vi.stubGlobal( + "fetch", + async () => + new Response( + new ReadableStream({ + pull(stream) { + stream.error(failure); + }, + }), + { status: 200 }, + ), + ); + await expect( + downloadHopperPackage("https://fixture.invalid/archive"), + ).rejects.toMatchObject({ name: "AbortError", code: DOMException.ABORT_ERR }); +}); + +it.each([null, false, 0, "", { caller: "stop download" }] as const)( + "preserves an independent body failure when the caller also aborts (%j)", + async (reason) => { + const controller = new AbortController(); + const failure = new DOMException( + "independent response failure", + "AbortError", + ); + vi.stubGlobal( + "fetch", + async () => + new Response( + new ReadableStream({ + pull(stream) { + controller.abort(reason); + stream.error(failure); + }, + }), + ), + ); + await expect( + downloadHopperPackage("https://fixture.invalid/archive", { + signal: controller.signal, + }), + ).rejects.toBe(failure); + }, +); + +it.each([null, false, { caller: "cancel" }] as const)( + "retains the exact caller reason through a native response body read (%j)", + async (reason) => { + const server = createServer((_request, response) => { + response.writeHead(200); + response.write("first chunk"); + }); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + const address = server.address(); + if (address === null || typeof address === "string") + throw new Error("Expected a TCP listener"); + const controller = new AbortController(); + const nativeFetch = globalThis.fetch; + vi.stubGlobal("fetch", async (_url: string, options: RequestInit) => { + const response = await nativeFetch( + `http://127.0.0.1:${address.port}/`, + options, + ); + setImmediate(() => controller.abort(reason)); + return response; + }); + try { + await expect( + downloadHopperPackage("https://fixture.invalid/archive", { + signal: controller.signal, + }), + ).rejects.toBe(reason); + } finally { + server.closeAllConnections(); + await new Promise((resolve, reject) => + server.close((error) => (error ? reject(error) : resolve())), + ); + } + }, +); diff --git a/src/application/HopperDownload.ts b/src/application/HopperDownload.ts index 2df8c52f0b..f92d143a83 100644 --- a/src/application/HopperDownload.ts +++ b/src/application/HopperDownload.ts @@ -60,6 +60,19 @@ export const HOPPER_DOWNLOAD_TIMEOUT_MS = 15 * 60_000; /** Small release feed and key requests must not use the package bounds. */ export const HOPPER_METADATA_TIMEOUT_MS = 10_000; +/** Attribute a raw rejection to this caller's exact cancellation. */ +export const isHopperRequestCancellation = ( + cause: unknown, + signal: AbortSignal | undefined, +): boolean => + signal?.aborted === true && + (cause === signal.reason || + (cause instanceof Error && + cause.name === "AbortError" && + "code" in cause && + cause.code === "ABORT_ERR" && + cause.cause === signal.reason)); + /** * Bound observed response bytes before retaining them, including chunked bodies. * Expected transport outcomes are returned; caller cancellation still rejects @@ -115,19 +128,34 @@ export const downloadHopperPackage = async ( expire("stalled", HOPPER_DOWNLOAD_STALL_MS); }, HOPPER_DOWNLOAD_STALL_MS); }; + const selectedSignal = + options.signal === undefined + ? deadline.signal + : AbortSignal.any([options.signal, deadline.signal]); + // Node may replace falsy abort reasons during body reads. Send a unique, + // truthy reason through fetch so only its own rejection can be attributed. + const transport = new AbortController(); + const transportReason = {}; + const cancelTransport = () => transport.abort(transportReason); + if (selectedSignal.aborted) cancelTransport(); + else + selectedSignal.addEventListener("abort", cancelTransport, { once: true }); try { progressed(0); return await readHopperResponse(url, reported, { - signal: - options.signal === undefined - ? deadline.signal - : AbortSignal.any([options.signal, deadline.signal]), + signal: transport.signal, maxBytes, progressed, }); - } catch (cause: unknown) { - if (options.signal?.aborted === true) throw cause; - if (expired !== undefined) + } catch (observed: unknown) { + const cause = isHopperRequestCancellation(observed, transport.signal) + ? selectedSignal.reason + : observed; + if (isHopperRequestCancellation(cause, options.signal)) throw cause; + if ( + expired !== undefined && + isHopperRequestCancellation(cause, deadline.signal) + ) return err({ url: reported, kind: "timeout", ...expired }); // Fetch reports network errors (DNS, TLS, reset, truncated body) as TypeError. if (cause instanceof TypeError) @@ -138,6 +166,7 @@ export const downloadHopperPackage = async ( }); throw cause; } finally { + selectedSignal.removeEventListener("abort", cancelTransport); clearTimeout(overall); clearTimeout(stall); } diff --git a/src/application/LinuxHopper.test.ts b/src/application/LinuxHopper.test.ts index 803a2ad372..9c00fbad3d 100644 --- a/src/application/LinuxHopper.test.ts +++ b/src/application/LinuxHopper.test.ts @@ -646,3 +646,69 @@ it("reports missing ldd as unresolved launcher inspection while retaining actual await host.close?.(); } }); + +it.each(["physical", "abort-name", "caller", "node-wrapper"] as const)( + "attributes a rejected archive write through cleanup (%s)", + async (kind) => { + const host = new RecordingLinuxHost(); + const controller = new AbortController(); + const reason = { caller: "stop during archive write" }; + const physical = Object.assign(new Error("archive write denied"), { + code: "EACCES", + name: kind === "abort-name" ? "AbortError" : "Error", + }); + const wrapped = Object.assign( + new Error("operation aborted", { cause: reason }), + { + name: "AbortError", + code: "ABORT_ERR", + }, + ); + host.writeArchive = async () => { + controller.abort(reason); + throw kind === "caller" + ? reason + : kind === "node-wrapper" + ? wrapped + : physical; + }; + host.cleanupFails = true; + const result = await installLinuxHopper(host, { + signal: controller.signal, + }); + const cancelled = kind === "caller" || kind === "node-wrapper"; + expect(result).toMatchObject({ + status: "failed", + reason: cancelled ? "cancelled" : "archive_write", + cleanupFailure: { + path: "/tmp/rea test", + reason: "temporary path is busy", + }, + }); + if (result.status !== "failed") throw new Error("Expected failed write"); + if (cancelled) expect(result.cancellationReason).toBe(reason); + else + expect(result.operationFailure).toMatchObject({ + operation: "archive_write", + systemCode: "EACCES", + detail: physical.message, + }); + expect(host.installed).toEqual([]); + expect(host.cleaned).toEqual(["/tmp/rea test"]); + }, +); + +it("preserves the package producer's cancellation when a later caller reason differs", async () => { + const host = new RecordingLinuxHost(); + const controller = new AbortController(); + const producerReason = { producer: "package operation cancelled" }; + host.installPackage = async () => { + controller.abort({ caller: "later stop" }); + return { status: "cancelled", cause: producerReason }; + }; + const result = await installLinuxHopper(host, { signal: controller.signal }); + expect(result).toMatchObject({ status: "failed", reason: "cancelled" }); + if (result.status !== "failed") throw new Error("Expected cancellation"); + expect(result.cancellationReason).toBe(producerReason); + expect(host.cleaned).toEqual(["/tmp/rea test"]); +}); diff --git a/src/application/LinuxHopper.ts b/src/application/LinuxHopper.ts index 9e88b2dd5f..34d2007c89 100644 --- a/src/application/LinuxHopper.ts +++ b/src/application/LinuxHopper.ts @@ -10,6 +10,7 @@ import { execFileOutput } from "../process/ExecFileOutput.js"; import { ArtifactReaderFailure } from "../artifacts/ArtifactReader.js"; import { downloadHopperPackage, + isHopperRequestCancellation, type HopperDownload, type HopperRequestFailure, } from "./HopperDownload.js"; @@ -486,19 +487,14 @@ export const installLinuxHopper = async ( return { status: "installed", launcherPath }; })(); } catch (cause: unknown) { + const cancelled = isHopperRequestCancellation(cause, options.signal); result = { status: "failed", - reason: - isAborted(options.signal) || isAbortCause(cause) ? "cancelled" : stage, + reason: cancelled ? "cancelled" : stage, operationFailure: operationFailure(stage, cause, operationPath), + ...(cancelled ? { cancellationReason: options.signal?.reason } : {}), }; } - if ( - result.status === "failed" && - result.reason === "cancelled" && - isAborted(options.signal) - ) - result = { ...result, cancellationReason: options.signal?.reason }; if (temporary === undefined) return result; try { await host.cleanup(temporary); @@ -739,6 +735,7 @@ const publisherFailure = ( status: "failed", reason, verificationFailure: error, + ...(reason === "cancelled" ? { cancellationReason: error.cause } : {}), ...(error.download === undefined ? {} : { download: error.download }), }); @@ -868,5 +865,3 @@ const versionAtLeast = ( ): boolean => version !== undefined && Number.parseInt(version.split(".")[0] ?? "0", 10) >= minimum; -const isAbortCause = (cause: unknown): boolean => - cause instanceof Error && cause.name === "AbortError"; diff --git a/src/application/LinuxHopperPublisher.test.ts b/src/application/LinuxHopperPublisher.test.ts new file mode 100644 index 0000000000..d82e0028c8 --- /dev/null +++ b/src/application/LinuxHopperPublisher.test.ts @@ -0,0 +1,50 @@ +import { writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { expect, it } from "vitest"; + +import { createTestTempDirectory } from "../../tests/fixtures/temporaryDirectory.js"; +import { ExecFileOutputCancelledError } from "../process/ExecFileOutput.js"; +import { LinuxHopperPublisher } from "./LinuxHopperPublisher.js"; + +it.each(["physical", "caller", "node-wrapper", "producer"] as const)( + "attributes package-family observation failure after hashing (%s)", + async (kind) => { + const root = await createTestTempDirectory("rea-publisher-cancellation-"); + const path = join(root, "inert-launcher"); + await writeFile(path, "inert launcher bytes"); + const controller = new AbortController(); + const reason = { caller: "stop inspection" }; + const physical = new Error("distribution inspection failed"); + const wrapped = Object.assign( + new Error("operation aborted", { cause: reason }), + { + name: "AbortError", + code: "ABORT_ERR", + }, + ); + const publisher = new LinuxHopperPublisher({ HOME: root }, async () => { + if (kind !== "producer") controller.abort(reason); + throw kind === "physical" + ? physical + : kind === "caller" + ? reason + : kind === "node-wrapper" + ? wrapped + : new ExecFileOutputCancelledError(reason); + }); + try { + const result = await publisher.inspectLauncher(path, controller.signal); + expect(result).toMatchObject({ + ok: false, + error: { + reason: + kind === "physical" ? "verification_unavailable" : "cancelled", + }, + }); + if (result.ok) throw new Error("Expected failed observation"); + expect(result.error.cause).toBe(kind === "physical" ? physical : reason); + } finally { + await publisher.close(); + } + }, +); diff --git a/src/application/LinuxHopperPublisher.ts b/src/application/LinuxHopperPublisher.ts index 766f0d8d66..aae258f471 100644 --- a/src/application/LinuxHopperPublisher.ts +++ b/src/application/LinuxHopperPublisher.ts @@ -27,13 +27,17 @@ import { } from "../filesystem/OwnedRegularFile.js"; import { readBoundedFileBytes } from "../process/BoundedFileBytes.js"; import { verifyDetachedPgpSignature } from "../process/DetachedPgpSignature.js"; -import { execFileOutput } from "../process/ExecFileOutput.js"; +import { + execFileOutput, + ExecFileOutputCancelledError, +} from "../process/ExecFileOutput.js"; import { PrivateRuntimeRoot } from "../process/PrivateRuntimeRoot.js"; import { waitForAbortableDelay } from "../process/ProviderDeadline.js"; import { acquireHopperReleaseCacheLease } from "../process/HopperLease.js"; import type { OwnedLease } from "../process/OwnedLease.js"; import { downloadHopperPackage, + isHopperRequestCancellation, HOPPER_DOWNLOAD_BYTES, HOPPER_METADATA_TIMEOUT_MS, } from "./HopperDownload.js"; @@ -232,7 +236,12 @@ export class LinuxHopperPublisher { ? "cancelled" : "publisher_verification_failed", verified.error.message, - { cause: verified.error }, + { + cause: + verified.error.reason === "cancelled" + ? verified.error.cause + : verified.error, + }, ), ); const mainExeHash = await this.#withTemporary( @@ -1079,6 +1088,12 @@ const releaseFailure = ( signal: AbortSignal | undefined, fallback: HopperReleaseError["reason"], ): HopperReleaseError => { + if (isHopperRequestCancellation(cause, signal)) + return new HopperReleaseError( + "cancelled", + "Hopper publisher verification was cancelled.", + { cause: signal?.reason }, + ); if (cause instanceof HopperReleaseError) return cause; if (cause instanceof ArtifactReaderFailure && cause.cleanup !== undefined) return new HopperReleaseError( @@ -1093,11 +1108,11 @@ const releaseFailure = ( "Hopper publisher verification was cancelled.", { cause: cancellation.reason }, ); - if (!(cause instanceof StableFileHashFailure) && signal?.aborted === true) + if (cause instanceof ExecFileOutputCancelledError) return new HopperReleaseError( "cancelled", "Hopper publisher verification was cancelled.", - { cause }, + { cause: cause.cause }, ); return new HopperReleaseError( fallback, diff --git a/src/application/MacHopper.test.ts b/src/application/MacHopper.test.ts index 771bb4e792..3aef489dda 100644 --- a/src/application/MacHopper.test.ts +++ b/src/application/MacHopper.test.ts @@ -613,3 +613,57 @@ it("retains the completed installation but skips opening when cancelled during i expect(host.unmounts).toBe(1); expect(host.cleanups).toBe(1); }); + +it.each(["physical", "abort-name", "caller", "node-wrapper"] as const)( + "attributes a rejected package write and retains cleanup (%s)", + async (kind) => { + const host = new FakeMacHopperHost(); + const controller = new AbortController(); + const reason = { caller: "stop during package write" }; + const physical = Object.assign(new Error("package write denied"), { + name: kind === "abort-name" ? "AbortError" : "Error", + }); + const wrapped = Object.assign( + new Error("operation aborted", { cause: reason }), + { + name: "AbortError", + code: "ABORT_ERR", + }, + ); + host.writePackage = async () => { + controller.abort(reason); + throw kind === "caller" + ? reason + : kind === "node-wrapper" + ? wrapped + : physical; + }; + let retained: (() => Promise) | undefined; + let release = false; + host.cleanup = async () => { + if (!release) throw new Error("temporary root busy"); + }; + const result = await installMacHopper( + { signal: controller.signal }, + { + ...host, + retainCleanup: (cleanup, resources) => { + expect(resources).toEqual(["/tmp/rea-hopper-test"]); + retained = cleanup; + }, + }, + ); + expect(result).toMatchObject({ + status: "failed", + reason: + kind === "caller" || kind === "node-wrapper" ? "cancelled" : "mount", + cleanupFailure: expect.stringContaining("temporary root busy"), + }); + if (kind === "physical" || kind === "abort-name") + expect(result).toMatchObject({ diagnostic: physical.message }); + expect(host.installs).toBe(0); + expect(retained).toBeTypeOf("function"); + release = true; + await retained?.(); + }, +); diff --git a/src/application/MacHopper.ts b/src/application/MacHopper.ts index b9de3cac0b..7a09970289 100644 --- a/src/application/MacHopper.ts +++ b/src/application/MacHopper.ts @@ -22,6 +22,7 @@ import { macDeveloperIdRequirement } from "../process/MacCodeSignature.js"; import { macHopperUserApplicationPath } from "./MacHopperPaths.js"; import { downloadHopperPackage, + isHopperRequestCancellation, HOPPER_METADATA_TIMEOUT_MS, type HopperDownload, type HopperRequestFailure, @@ -310,7 +311,9 @@ export const installMacHopper = async ( } catch (cause: unknown) { result = { status: "failed", - reason: options.signal?.aborted === true ? "cancelled" : installStage, + reason: isHopperRequestCancellation(cause, options.signal) + ? "cancelled" + : installStage, diagnostic: failureMessage(cause), }; }