diff --git a/native/windows/README.md b/native/windows/README.md index 9d96ca7eb0..28f0f9344c 100644 --- a/native/windows/README.md +++ b/native/windows/README.md @@ -111,9 +111,14 @@ automatically. Alternatively, build it using the Linux/MinGW lane above. - Cleanup walks locked directory objects, removes files by handle, and unlinks reparse entries without traversing their targets. Failed removal retains the root and ancestor leases for retry; a partially removed runtime accepts only - cleanup, and its registry entry remains until removal succeeds. The owned root - handle includes attribute-write access so cleanup can clear a read-only marker, - matching the child cleanup handles. Normal close, cancellation, + cleanup, and its registry entry remains until removal succeeds. The runtime + owner retries only native close failures with Win32 sharing/lock codes 32/33, + for at most ten attempts with a 100 ms delay between attempts. Other native + errors are returned immediately; exhausting the retry budget preserves the + runtime owner for a later explicit close. These cleanup retries do not use an + analysis cancellation signal and schedule up to 900 ms of backoff, in + addition to native cleanup time. The owned root handle includes attribute-write access so cleanup can clear a + read-only marker, matching the child cleanup handles. Normal close, cancellation, startup failure, and timeout release provider resources. Abrupt owner death kills the owned job. The Android provider additionally uses a detached helper that owns its private runtime directory and cleans it when the owner pipe @@ -133,7 +138,9 @@ text do not establish a safe retry. Analysis errors propagate the producer's retry disposition through JavaScript, source-map and WABT workflows; transient failures report `category: unavailable` and retain the original reason. Incomplete cleanup blocks an operation retry until its owned resources settle. -REA adds no automatic retry loop or new analysis cache for this behavior. +The close-stage retry is separate from allocation and analysis retries: it +repeats only the existing owned cleanup operation, and does not retry provider +work or create another runtime. Ordinary drive-absolute paths accept backslashes, forward slashes, or mixed separators. File identity retains the original requested spelling. Separator diff --git a/scripts/verify-packaged-wasm-artifact.mjs b/scripts/verify-packaged-wasm-artifact.mjs index 98cd9ccb76..7d9e390274 100644 --- a/scripts/verify-packaged-wasm-artifact.mjs +++ b/scripts/verify-packaged-wasm-artifact.mjs @@ -2,6 +2,7 @@ import { join } from "node:path"; import { mkdir } from "node:fs/promises"; import { PrivateRuntimeRoot } from "../dist/process/PrivateRuntimeRoot.js"; +import { closeVerifierResources } from "./lib/verifier-cleanup.mjs"; import { exec, execNpm } from "./lib/verify-package-core.mjs"; import { WABT_WORKFLOW_TIMEOUT_MS } from "./lib/wabt-verifier-timeouts.mjs"; import { verifyPackagePack } from "./verify-package-pack.mjs"; @@ -9,6 +10,7 @@ import { verifyPackagePack } from "./verify-package-pack.mjs"; const root = await PrivateRuntimeRoot.create({ prefix: "rea-wabt-package-verifier-", }); +let primaryError; try { const { tarball } = await verifyPackagePack({ root: process.cwd(), @@ -56,6 +58,9 @@ try { }, ); process.stdout.write(verified.stdout); +} catch (cause) { + primaryError = cause; + throw cause; } finally { - await root.close(); + await closeVerifierResources([() => root.close()], primaryError); } diff --git a/scripts/verify-packet-capture.mjs b/scripts/verify-packet-capture.mjs index 9993117c2b..4f7452983b 100644 --- a/scripts/verify-packet-capture.mjs +++ b/scripts/verify-packet-capture.mjs @@ -12,6 +12,7 @@ import { StdioClientTransport } from "@modelcontextprotocol/client/stdio"; import { Ajv2020 } from "ajv/dist/2020.js"; import { PrivateRuntimeRoot } from "../dist/process/PrivateRuntimeRoot.js"; import { parseEvidence } from "../dist/domain/evidence.js"; +import { closeVerifierResources } from "./lib/verifier-cleanup.mjs"; import { createVerifierRun, completeVerifierRun } from "./lib/verifier-run.mjs"; if (process.platform !== "linux") @@ -106,6 +107,7 @@ const expectedAddressSpace = Math.min( .map((value) => (value === "unlimited" ? Infinity : Number(value))), ); +let primaryError; try { await writeFile(path, source); await writeFile(bluetooth, bluetoothSource); @@ -463,10 +465,14 @@ try { "network with spaces.pcap", "paged.pcap", ]); +} catch (cause) { + primaryError = cause; + throw cause; } finally { - await client.close(); - await transport.close(); - await root.close(); + await closeVerifierResources( + [() => client.close(), () => transport.close(), () => root.close()], + primaryError, + ); } console.log( JSON.stringify( diff --git a/scripts/verify-signal-recording.mjs b/scripts/verify-signal-recording.mjs index 2d3910e62a..848b4ec134 100644 --- a/scripts/verify-signal-recording.mjs +++ b/scripts/verify-signal-recording.mjs @@ -12,6 +12,7 @@ import { StdioClientTransport } from "@modelcontextprotocol/client/stdio"; import { Ajv2020 } from "ajv/dist/2020.js"; import { PrivateRuntimeRoot } from "../dist/process/PrivateRuntimeRoot.js"; import { parseEvidence } from "../dist/domain/evidence.js"; +import { closeVerifierResources } from "./lib/verifier-cleanup.mjs"; import { createVerifierRun, completeVerifierRun } from "./lib/verifier-run.mjs"; if (process.platform !== "linux") @@ -83,6 +84,7 @@ const expectedAddressSpace = Math.min( .map((value) => (value === "unlimited" ? Infinity : Number(value))), ); +let primaryError; try { await writeFile(path, source); await client.connect(transport); @@ -383,10 +385,14 @@ try { "paged.vcd", "recording with spaces.vcd", ]); +} catch (cause) { + primaryError = cause; + throw cause; } finally { - await client.close(); - await transport.close(); - await root.close(); + await closeVerifierResources( + [() => client.close(), () => transport.close(), () => root.close()], + primaryError, + ); } console.log( JSON.stringify( diff --git a/scripts/verify-windows-native.mjs b/scripts/verify-windows-native.mjs index 5b0729b01f..f27cadd7a6 100644 --- a/scripts/verify-windows-native.mjs +++ b/scripts/verify-windows-native.mjs @@ -23,6 +23,7 @@ import { pathToFileURL } from "node:url"; import { verifyWindowsProtocolInput } from "./lib/windows-protocol-input.mjs"; import { verifyWindowsJobMemoryLimit } from "./lib/windows-job-memory-limit.mjs"; import { verifyVolumeRootAppendLease } from "./lib/windows-volume-root-append-lease.mjs"; +import { closeVerifierResources } from "./lib/verifier-cleanup.mjs"; if (process.platform !== "win32") throw new Error( @@ -66,6 +67,49 @@ const exists = (path) => () => true, () => false, ); +const startDeleteSharingDeniedReader = async (path) => { + const child = spawn( + "powershell.exe", + [ + "-NoProfile", + "-NonInteractive", + "-Command", + "$ErrorActionPreference = 'Stop'; " + + "$stream = [IO.File]::Open($env:REA_CLEANUP_HELD_PATH, [IO.FileMode]::Open, [IO.FileAccess]::Read, [IO.FileShare]::ReadWrite); " + + "try { [Console]::WriteLine('ready'); [Console]::In.ReadLine() | Out-Null } " + + "finally { $stream.Dispose() }", + ], + { + env: { ...process.env, REA_CLEANUP_HELD_PATH: path }, + stdio: ["pipe", "pipe", "pipe"], + }, + ); + const closed = once(child, "close"); + void closed.catch(() => undefined); + try { + const [ready] = await once(child.stdout, "data", { + signal: AbortSignal.timeout(15_000), + }); + assert.equal(String(ready).trim(), "ready"); + } catch (cause) { + try { + child.kill(); + await closed; + } catch (cleanupCause) { + throw new AggregateError( + [cause, cleanupCause], + "Reader startup and child cleanup failed", + { cause }, + ); + } + throw cause; + } + return { child, closed }; +}; +const stopDeleteSharingDeniedReader = async ({ child, closed }) => { + child.kill(); + await closed; +}; const live = (pid) => { try { process.kill(pid, 0); @@ -516,32 +560,14 @@ process.stdout.write(JSON.stringify({executable:basename(process.execPath),abiVe "cleanup-retry-", ); const heldPath = join(retryRuntime.observation.path, "snapshot.bin"); - await retryRuntime.snapshot(source, "snapshot.bin"); - const holder = spawn( - "powershell.exe", - [ - "-NoProfile", - "-NonInteractive", - "-Command", - "$ErrorActionPreference = 'Stop'; " + - "$stream = [IO.File]::Open($env:REA_CLEANUP_HELD_PATH, [IO.FileMode]::Open, [IO.FileAccess]::Read, [IO.FileShare]::ReadWrite); " + - "try { [Console]::WriteLine('ready'); [Console]::In.ReadLine() | Out-Null } " + - "finally { $stream.Dispose() }", - ], - { - env: { ...process.env, REA_CLEANUP_HELD_PATH: heldPath }, - stdio: ["pipe", "pipe", "pipe"], - }, - ); - const holderClosed = once(holder, "close"); - void holderClosed.catch(() => undefined); + let holder; + let primaryError; try { - const [ready] = await once(holder.stdout, "data", { - signal: AbortSignal.timeout(15_000), - }); - assert.equal(String(ready).trim(), "ready"); + await retryRuntime.snapshot(source, "snapshot.bin"); + holder = await startDeleteSharingDeniedReader(heldPath); // The reader denies delete sharing. Cleanup must retain ownership when - // its handle-relative open receives ERROR_SHARING_VIOLATION (32). + // its handle-relative open receives ERROR_SHARING_VIOLATION (32) through + // the bounded automatic retries; a later explicit close must succeed. await assert.rejects(retryRuntime.close(), { win32Code: 32 }); assert.equal( windowsPrivateRuntime(retryRuntime.observation.path), @@ -552,8 +578,8 @@ process.stdout.write(JSON.stringify({executable:basename(process.execPath),abiVe () => retryRuntime.writeFile("after-close.txt", "forbidden"), /cleanup is pending/u, ); - holder.stdin.end("\n"); - const [exitCode] = await holderClosed; + holder.child.stdin.end("\n"); + const [exitCode] = await holder.closed; assert.equal(exitCode, 0); await retryRuntime.close(); await retryRuntime.close(); @@ -563,10 +589,62 @@ process.stdout.write(JSON.stringify({executable:basename(process.execPath),abiVe /No native private runtime/u, ); report.controls.cleanupFailureRetainsOwnershipForRetry = true; + } catch (cause) { + primaryError = cause; + throw cause; } finally { - holder.kill(); - await holderClosed.catch(() => undefined); - await retryRuntime.close(); + await closeVerifierResources( + [ + ...(holder === undefined + ? [] + : [() => stopDeleteSharingDeniedReader(holder)]), + () => retryRuntime.close(), + ], + primaryError, + ); + } + const transientRuntime = WindowsPrivateRuntime.create( + workspace, + "cleanup-transient-", + ); + const transientHeldPath = join( + transientRuntime.observation.path, + "snapshot.bin", + ); + let transientHolder; + let transientPrimaryError; + try { + await transientRuntime.snapshot(source, "snapshot.bin"); + transientHolder = await startDeleteSharingDeniedReader(transientHeldPath); + // The first native close is synchronous and must fail while this reader + // is held. Releasing it while the owner waits allows its own retry to + // complete without the caller issuing another close. + const closing = transientRuntime.close(); + transientHolder.child.stdin.end("\n"); + const [, [exitCode]] = await Promise.all([ + closing, + transientHolder.closed, + ]); + assert.equal(exitCode, 0); + assert.equal(await exists(transientRuntime.observation.path), false); + assert.throws( + () => windowsPrivateRuntime(transientRuntime.observation.path), + /No native private runtime/u, + ); + report.controls.cleanupTransientConflictRetries = true; + } catch (cause) { + transientPrimaryError = cause; + throw cause; + } finally { + await closeVerifierResources( + [ + ...(transientHolder === undefined + ? [] + : [() => stopDeleteSharingDeniedReader(transientHolder)]), + () => transientRuntime.close(), + ], + transientPrimaryError, + ); } report.controls = { ...report.controls, diff --git a/src/windows/WindowsPrivateRuntime.test.ts b/src/windows/WindowsPrivateRuntime.test.ts new file mode 100644 index 0000000000..08cfda6246 --- /dev/null +++ b/src/windows/WindowsPrivateRuntime.test.ts @@ -0,0 +1,151 @@ +import { describe, expect, it, vi } from "vitest"; + +const nativeCall = vi.hoisted(() => vi.fn()); + +vi.mock("./WindowsNativeLoader.js", async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + requireWindowsNativeAuthority: () => ({ call: nativeCall }), + }; +}); + +import { + WindowsPrivateRuntime, + windowsPrivateRuntime, +} from "./WindowsPrivateRuntime.js"; + +const observation = { + requestedPath: "C:\\rea-private", + finalPath: "C:\\rea-private", + filesystem: "NTFS", + volumeSerial: "0123456789abcdef", + fileId: "0123456789abcdef0123456789abcdef", + size: 0, + directory: true, + path: "C:\\rea-private", + handle: {}, + privateDacl: true, +}; + +const nativeFailure = (win32Code: number): Error => + Object.assign(new Error(`native close failed (${win32Code})`), { + code: "ERR_REA_WINDOWS_NATIVE", + win32Code, + }); + +describe("Windows private runtime cleanup retry", () => { + it("retries a transient sharing conflict through the same owned close", async () => { + const conflict = nativeFailure(32); + let closes = 0; + nativeCall.mockImplementation((operation: string) => { + if (operation === "runtime_create") return observation; + if (operation === "runtime_close" && closes++ === 0) throw conflict; + return null; + }); + const runtime = WindowsPrivateRuntime.create("C:\\", "rea-"); + + await runtime.close(); + + expect(closes).toBe(2); + expect(() => windowsPrivateRuntime(observation.path)).toThrow( + /No native private runtime/u, + ); + }); + + it.each([ + ["a native access denial", nativeFailure(5)], + [ + "a POSIX-style access denial", + Object.assign(new Error("access denied"), { + code: "EACCES", + win32Code: 32, + }), + ], + [ + "a native error with string code 32", + Object.assign(new Error("sharing conflict"), { + code: "ERR_REA_WINDOWS_NATIVE", + win32Code: "32", + }), + ], + [ + "a native error without a Win32 code", + Object.assign(new Error("native failure"), { + code: "ERR_REA_WINDOWS_NATIVE", + }), + ], + [ + "a missing-path error carrying a Win32 sharing code", + Object.assign(new Error("path disappeared"), { + code: "ENOENT", + win32Code: 32, + }), + ], + [ + "a nested cleanup detail carrying a Win32 sharing code", + Object.assign(new Error("native failure"), { + code: "ERR_REA_WINDOWS_NATIVE", + cleanup: { win32Code: 32 }, + }), + ], + ] as const)( + "does not retry %s and retains the owner for a later close", + async (_label, denied) => { + let closes = 0; + nativeCall.mockImplementation((operation: string) => { + if (operation === "runtime_create") return observation; + if (operation === "runtime_close" && closes++ === 0) throw denied; + return null; + }); + const runtime = WindowsPrivateRuntime.create("C:\\", "rea-"); + + await expect(runtime.close()).rejects.toBe(denied); + expect(closes).toBe(1); + expect(windowsPrivateRuntime(observation.path)).toBe(runtime); + await runtime.close(); + expect(closes).toBe(2); + }, + ); + + it("shares one in-flight close across concurrent callers", async () => { + let closes = 0; + nativeCall.mockImplementation((operation: string) => { + if (operation === "runtime_create") return observation; + if (operation === "runtime_close" && closes++ === 0) + throw nativeFailure(33); + return null; + }); + const runtime = WindowsPrivateRuntime.create("C:\\", "rea-"); + const first = runtime.close(); + const second = runtime.close(); + + expect(second).toBe(first); + await Promise.all([first, second]); + expect(closes).toBe(2); + }); + + it("retains the exact final native error and owner when conflicts persist", async () => { + const conflict = nativeFailure(32); + let closes = 0; + nativeCall.mockImplementation((operation: string) => { + if (operation === "runtime_create") return observation; + if (operation === "runtime_close") { + closes++; + throw conflict; + } + return null; + }); + const runtime = WindowsPrivateRuntime.create("C:\\", "rea-"); + + await expect(runtime.close()).rejects.toBe(conflict); + + expect(closes).toBe(10); + expect(windowsPrivateRuntime(observation.path)).toBe(runtime); + nativeCall.mockImplementation((operation: string) => + operation === "runtime_create" ? observation : null, + ); + await runtime.close(); + }); +}); diff --git a/src/windows/WindowsPrivateRuntime.ts b/src/windows/WindowsPrivateRuntime.ts index 2c361a6d0c..e1cd227ee7 100644 --- a/src/windows/WindowsPrivateRuntime.ts +++ b/src/windows/WindowsPrivateRuntime.ts @@ -1,4 +1,5 @@ import { win32 } from "node:path"; +import { setTimeout as delay } from "node:timers/promises"; import { z } from "zod"; import { PrivateRuntimeRootUnavailableError } from "../process/PrivateRuntimeRootUnavailableError.js"; @@ -31,6 +32,12 @@ const transientRuntimeAdmissionSchema = z.object({ win32Code: z.union([z.literal(32), z.literal(33)]), runtimeAllocation: z.literal("not-started"), }); +const transientRuntimeCleanupSchema = z.object({ + code: z.literal("ERR_REA_WINDOWS_NATIVE"), + win32Code: z.union([z.literal(32), z.literal(33)]), +}); +const RUNTIME_CLEANUP_ATTEMPTS = 10; +const RUNTIME_CLEANUP_RETRY_DELAY_MS = 100; /** Retain private DACL and path handles until native recursive cleanup settles. */ export class WindowsPrivateRuntime { @@ -160,10 +167,29 @@ export class WindowsPrivateRuntime { if (this.#pending !== undefined) this.authority.call("runtime_snapshot_cancel", [this.observation.handle]); await this.#pending?.catch(() => undefined); - this.authority.call("runtime_close", [this.observation.handle]); + await this.#closeRuntime(); this.#closed = true; runtimes.delete(windowsRuntimeIdentityKey(this.observation)); } + + async #closeRuntime(): Promise { + for (let attempt = 1; ; attempt++) { + try { + this.authority.call("runtime_close", [this.observation.handle]); + return; + } catch (cause: unknown) { + if ( + attempt >= RUNTIME_CLEANUP_ATTEMPTS || + !transientRuntimeCleanupSchema.safeParse(cause).success + ) + throw cause; + // Native cleanup retains the same owned root and permits only another + // close after a failed walk. Retry that owner operation briefly when + // Windows reports a sharing/lock conflict; other errors remain final. + await delay(RUNTIME_CLEANUP_RETRY_DELAY_MS); + } + } + } } /** File id returned by the OS. Path spelling is not part of the lease key. */