From e7882b34f4ac4a26f903c5f49264acc2c19bb562 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Tue, 13 Oct 2026 15:12:17 +0800 Subject: [PATCH] fix(artifacts): honor cancellation at the output publication boundary --- src/artifacts/SafeOutputTree.ts | 33 ++++++++++------ .../extraction/ArtifactExtraction.ts | 2 +- src/browser/assets/PublishWebScripts.ts | 2 +- src/firmware/FirmwarePublication.ts | 2 +- src/javascript/recovery/WakaruProvider.ts | 2 +- .../safeOutputTreeCloseFailures.test.ts | 39 +++++++++++++++++++ ...actProvider.extractionCompleteness.test.ts | 39 ++++++++++++++++++- 7 files changed, 103 insertions(+), 16 deletions(-) diff --git a/src/artifacts/SafeOutputTree.ts b/src/artifacts/SafeOutputTree.ts index 4bb4cfe1a..8fa40876d 100644 --- a/src/artifacts/SafeOutputTree.ts +++ b/src/artifacts/SafeOutputTree.ts @@ -280,9 +280,11 @@ export class SafeOutputTree { } /** Sync the owned output tree and prevent further writes through this instance. */ - async commit(): Promise { + async commit(signal?: AbortSignal): Promise { this.#assertWritable(); + abortIfNeeded(signal); await this.#assertOwnedDirectories(); + abortIfNeeded(signal); // Windows has no directory fsync; file contents are already synced in write(). if (this.platform === "win32") { this.#published = true; @@ -315,6 +317,7 @@ export class SafeOutputTree { ); await output.handle.sync(); await parent.handle.sync(); + abortIfNeeded(signal); this.#published = true; } catch (cause: unknown) { sync = { kind: "failed", cause }; @@ -336,23 +339,26 @@ export class SafeOutputTree { closeFailure, this.#outputRoot, ); - if (sync.kind === "failed") - throw ArtifactReaderFailure.withCleanup( - new ArtifactReaderFailure( - "path", - "Could not durably sync extraction output", - { cause: sync.cause }, - ), - cleanup, - ); + if (sync.kind === "failed") { + const primary = isCancellationFailure(sync.cause) + ? sync.cause + : new ArtifactReaderFailure( + "path", + "Could not durably sync extraction output", + { cause: sync.cause }, + ); + throw ArtifactReaderFailure.withCleanup(primary, cleanup); + } throw ArtifactReaderFailure.withCleanup(closeFailure, cleanup); } - if (sync.kind === "failed") + if (sync.kind === "failed") { + if (isCancellationFailure(sync.cause)) throw sync.cause; throw new ArtifactReaderFailure( "path", "Could not durably sync extraction output", { cause: sync.cause }, ); + } } /** Remove only this operation's unsealed tree and verify absence. */ @@ -590,6 +596,11 @@ const abortIfNeeded = (signal?: AbortSignal): void => { ); }; +const isCancellationFailure = ( + cause: unknown, +): cause is ArtifactReaderFailure => + cause instanceof ArtifactReaderFailure && cause.reason === "cancelled"; + const isAlreadyExists = (cause: unknown): boolean => cause instanceof Error && "code" in cause && cause.code === "EEXIST"; diff --git a/src/artifacts/extraction/ArtifactExtraction.ts b/src/artifacts/extraction/ArtifactExtraction.ts index ee92954eb..8cad3543e 100644 --- a/src/artifacts/extraction/ArtifactExtraction.ts +++ b/src/artifacts/extraction/ArtifactExtraction.ts @@ -249,7 +249,7 @@ const materializeSelection = async ({ compareUnicodeCodePoints(left.relative_path, right.relative_path), ); completed = createExtractionResult(input, inventory, selected, extracted); - await output.commit(); + await output.commit(signal); return completed; } catch (cause: unknown) { const cleanupFailures = await releaseExtractionOwners( diff --git a/src/browser/assets/PublishWebScripts.ts b/src/browser/assets/PublishWebScripts.ts index 2db5d7dcf..ba886f00e 100644 --- a/src/browser/assets/PublishWebScripts.ts +++ b/src/browser/assets/PublishWebScripts.ts @@ -113,7 +113,7 @@ export const publishWebScripts = async ( options.signal, ); options.signal?.throwIfAborted(); - await tree.commit(); + await tree.commit(options.signal); return result; } catch (cause: unknown) { if (cause instanceof SafeOutputTreeCreationFailure) tree = cause.tree; diff --git a/src/firmware/FirmwarePublication.ts b/src/firmware/FirmwarePublication.ts index bc53084b1..b2bc4ddd5 100644 --- a/src/firmware/FirmwarePublication.ts +++ b/src/firmware/FirmwarePublication.ts @@ -96,7 +96,7 @@ export const publishFirmwareExtraction = async (context: { published_bytes: files.reduce((sum, file) => sum + file.size, 0), }); context.signal.throwIfAborted(); - await tree.commit(); + await tree.commit(context.signal); return result; } catch (cause: unknown) { if (cause instanceof SafeOutputTreeCreationFailure) tree = cause.tree; diff --git a/src/javascript/recovery/WakaruProvider.ts b/src/javascript/recovery/WakaruProvider.ts index b1260bcd1..88a51a74b 100644 --- a/src/javascript/recovery/WakaruProvider.ts +++ b/src/javascript/recovery/WakaruProvider.ts @@ -139,7 +139,7 @@ export class WakaruProvider implements JavaScriptRecoveryPort { await rm(root, { recursive: true, force: true }); root = undefined; checkRecoveryDeadline(deadline, options?.signal); - await tree.commit(); + await tree.commit(options?.signal); } catch (cause: unknown) { failed = { cause }; } diff --git a/tests/boundary/filesystem/safeOutputTreeCloseFailures.test.ts b/tests/boundary/filesystem/safeOutputTreeCloseFailures.test.ts index ee285dbd5..d835b1e46 100644 --- a/tests/boundary/filesystem/safeOutputTreeCloseFailures.test.ts +++ b/tests/boundary/filesystem/safeOutputTreeCloseFailures.test.ts @@ -10,6 +10,8 @@ const injectedFilesystemFailures = vi.hoisted(() => ({ closeNext: false, blockCloseRetries: false, chmodNext: false, + syncAbortAfter: undefined as AbortController | undefined, + syncCount: 0, rootIdentityPath: undefined as string | undefined, })); @@ -44,6 +46,15 @@ vi.mock("node:fs/promises", async (importOriginal) => { await close(); }; } + if (injectedFilesystemFailures.syncAbortAfter !== undefined) { + const sync = handle.sync.bind(handle); + handle.sync = async () => { + await sync(); + injectedFilesystemFailures.syncCount += 1; + if (injectedFilesystemFailures.syncCount === 2) + injectedFilesystemFailures.syncAbortAfter?.abort(); + }; + } return handle; }, }; @@ -82,6 +93,34 @@ describe("safe artifact output close failures", () => { }, ); + it.skipIf(process.platform === "win32")( + "keeps the tree unpublished when cancellation arrives during commit durability", + async () => { + const parent = await createTestTempDirectory("rea-safe-output-cancel-"); + const output = join(parent, "unpublished"); + const tree = await SafeOutputTree.create(output); + const bytes = Buffer.from("pending output"); + await tree.write("file.txt", Readable.from(bytes), { + sha256: createHash("sha256").update(bytes).digest("hex"), + bytes: bytes.byteLength, + }); + const controller = new AbortController(); + injectedFilesystemFailures.syncCount = 0; + injectedFilesystemFailures.syncAbortAfter = controller; + try { + await expect(tree.commit(controller.signal)).rejects.toMatchObject({ + reason: "cancelled", + }); + expect(tree.published).toBe(false); + } finally { + injectedFilesystemFailures.syncAbortAfter = undefined; + injectedFilesystemFailures.syncCount = 0; + } + expect(await tree.rollback()).toMatchObject({ status: "complete" }); + await expect(access(output)).rejects.toMatchObject({ code: "ENOENT" }); + }, + ); + it.skipIf(process.platform === "win32")( "preserves setup failure when closing the staging handle also fails", async () => { diff --git a/tests/boundary/providers/native/artifactProvider.extractionCompleteness.test.ts b/tests/boundary/providers/native/artifactProvider.extractionCompleteness.test.ts index d42762601..0bcf12bb2 100644 --- a/tests/boundary/providers/native/artifactProvider.extractionCompleteness.test.ts +++ b/tests/boundary/providers/native/artifactProvider.extractionCompleteness.test.ts @@ -3,7 +3,7 @@ import { access, mkdir, readFile, rm, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { createPackage, createPackageWithOptions } from "@electron/asar"; -import { expect, it } from "vitest"; +import { expect, it, vi } from "vitest"; import { ArtifactProvider } from "../../../../src/artifacts/ArtifactProvider.js"; import { @@ -13,6 +13,7 @@ import { import { artifactExtractionResultSchema } from "../../../../src/domain/artifactGraph.js"; import { projectAnalysisError } from "../../../../src/domain/analysisErrorProjection.js"; import { createTestTempDirectory } from "../../../fixtures/temporaryDirectory.js"; +import { DirectoryArtifactReader } from "../../../../src/artifacts/DirectoryArtifactReader.js"; it.each(["file", "directory"] as const)( "rolls back when an inventoried %s disappears before extraction", @@ -61,6 +62,42 @@ it("still refuses a regular file added after inventory and rolls back", async () await expect(access(output)).rejects.toThrow(); }); +it("rolls back when cancellation arrives while closing the materialization reader", async () => { + const root = await createTestTempDirectory("rea-extract-cancel-close-"); + const source = join(root, "source"); + await mkdir(source); + await writeFile(join(source, "payload.txt"), "complete payload\n"); + const snapshot = await scanArtifactInventory(source); + const output = join(root, "output"); + const controller = new AbortController(); + const close = DirectoryArtifactReader.prototype.close; + const closeSpy = vi + .spyOn(DirectoryArtifactReader.prototype, "close") + .mockImplementation(async function (this: DirectoryArtifactReader) { + controller.abort(); + await close.call(this); + }); + try { + await expect( + materializeArtifactInventory( + { + inputPath: source, + inputFormat: "asar", + outputRoot: output, + environment: process.env, + integrityPolicy: "fail", + }, + source, + snapshot, + controller.signal, + ), + ).rejects.toMatchObject({ reason: "cancelled" }); + await expect(access(output)).rejects.toThrow(); + } finally { + closeSpy.mockRestore(); + } +}); + it("preserves every equal-content occurrence and copies a nested ASAR as its containing file", async () => { const root = await createTestTempDirectory("rea-extract-nested-"); const source = join(root, "source");