From 94478e8a15459126fe2e3d24cccc6ae243185cf4 Mon Sep 17 00:00:00 2001 From: luvs01 Date: Thu, 6 Aug 2026 23:36:59 +0900 Subject: [PATCH] fix native-main ACL timeout recovery --- src/codex/native-main-lock-file.ts | 6 +- src/codex/native-main-owner.ts | 20 +++- src/lib/windows-secret-acl.ts | 88 ++++++++++++--- tests/native-main-owner-lifetime.test.ts | 78 +++++++++++++ tests/windows-secret-acl.test.ts | 135 ++++++++++++++++++++++- 5 files changed, 303 insertions(+), 24 deletions(-) diff --git a/src/codex/native-main-lock-file.ts b/src/codex/native-main-lock-file.ts index af22cda206..a61490cfab 100644 --- a/src/codex/native-main-lock-file.ts +++ b/src/codex/native-main-lock-file.ts @@ -140,12 +140,16 @@ export function assertStableLockFile(path: string, handle: StableLockFile): void export async function hardenStableLockFile( path: string, platform: NodeJS.Platform = process.platform, + options: { retryTimedOutOnce?: boolean } = {}, ): Promise { if (platform === "win32") { // Best-effort here: POSIX modes are not authoritative on NTFS, and the // required ACL hardening below is what actually decides. try { chmodSync(path, 0o600); } catch { /* ACL below is authoritative. */ } - await hardenSecretPathAsync(path, { required: true }); + await hardenSecretPathAsync(path, { + required: true, + retryTimedOutOnce: options.retryTimedOutOnce, + }); return; } // On POSIX the mode IS the mechanism, so a failure may not be swallowed. diff --git a/src/codex/native-main-owner.ts b/src/codex/native-main-owner.ts index 70aed9cb6a..df2d7b7698 100644 --- a/src/codex/native-main-owner.ts +++ b/src/codex/native-main-owner.ts @@ -53,6 +53,7 @@ interface OwnerEntry { timer?: ReturnType; drive?: Promise; activeOperations: Set>; + aclTimeoutRetryUsed: boolean; closing: boolean; } @@ -149,8 +150,13 @@ async function prepareOwnerDatabase(entry: OwnerEntry): Promise { // here still exercises the host's branch, and the production default — the // thing that actually hardens the owner's lock file — stays unproved. An // audit replaced this fallback with a no-op and 91 tests stayed green. - await (entry.options.hardenPath - ?? ((target: string) => hardenStableLockFile(target, entry.options.platform)))(entry.lockPath); + if (entry.options.hardenPath) { + await entry.options.hardenPath(entry.lockPath); + } else { + await hardenStableLockFile(entry.lockPath, entry.options.platform, { + retryTimedOutOnce: entry.aclTimeoutRetryUsed, + }); + } assertStableLockFile(entry.lockPath, file); entry.file = file; file = undefined; @@ -196,6 +202,13 @@ async function drive(entry: OwnerEntry, generation: number): Promise { scheduleRetry(entry, generation); return; } + if (errorCode(error) === "ETIMEDOUT" && !entry.aclTimeoutRetryUsed) { + entry.aclTimeoutRetryUsed = true; + // A timeout is neither ownership contention nor a permanent ACL denial. + // Stay fail-closed in `acquiring` and spend exactly one fresh-budget retry. + scheduleRetry(entry, generation); + return; + } publish(entry, { status: "unavailable", homeId: entry.context.homeId, reason: "lock-unavailable" }); } } @@ -222,6 +235,7 @@ function entryFor(context: NativeProfileContext, options: NativeMainOwnerOptions snapshot: { status: "acquiring", homeId: context.homeId }, listeners: new Set(), activeOperations: new Set(), + aclTimeoutRetryUsed: false, prepared: false, closing: false, }; @@ -239,7 +253,7 @@ export function retainNativeMainOwner( throw new NativeProfileError("NATIVE_MAIN_OWNER_BUSY", "Native-main ownership is closing.", 503, true); } entry.refs += 1; - if (!entry.drive && !entry.database && !entry.timer) { + if (!entry.drive && !entry.database && !entry.timer && entry.snapshot.status !== "unavailable") { const generation = entry.generation; entry.drive = drive(entry, generation).finally(() => { if (entry.generation === generation) entry.drive = undefined; diff --git a/src/lib/windows-secret-acl.ts b/src/lib/windows-secret-acl.ts index f8cc2ec90b..f1b72bce85 100644 --- a/src/lib/windows-secret-acl.ts +++ b/src/lib/windows-secret-acl.ts @@ -34,8 +34,12 @@ import { env, platform } from "node:process"; const hardenedDirectories = new Map(); const hardenedPaths = new Map(); -/** Paths whose harden TIMED OUT this process: do not re-stall every loadConfig on them. */ -const timedOutPaths = new Set(); +/** + * Paths whose harden TIMED OUT this process: do not re-stall every loadConfig on them. + * `false` means one explicitly authorized recovery attempt remains; `true` means + * that attempt was consumed. Ordinary callers never consume it. + */ +const timedOutPaths = new Map(); /** * The memo value: `object:freshness` for a file a harden was actually attributed @@ -212,6 +216,12 @@ export interface HardenOptions { * Must NOT be a parent directory — directory ACLs are not authoritative for new files. */ timeoutMemoKey?: string; + /** + * Consume the one recovery attempt for a previously timed-out memo key. + * Only a caller that owns its own single-flight and bounded retry policy should + * set this. It never clears or bypasses an already-consumed timeout memo. + */ + retryTimedOutOnce?: boolean; } /** @@ -524,6 +534,50 @@ function isTimeoutError(error: unknown): boolean { && String((error as NodeJS.ErrnoException).code) === "ETIMEDOUT"; } +/** Preserve only the bounded machine-readable cause on a sanitized public error. */ +function sanitizedAclError(diagnostics: string, cause: unknown): NodeJS.ErrnoException { + const error = new Error(diagnostics) as NodeJS.ErrnoException; + const code = cause && typeof cause === "object" && "code" in cause + ? String((cause as { code?: unknown }).code) + : ""; + if (code === "ETIMEDOUT" || code === "EICACLS" || code === "EACCES" || code === "EPERM") { + error.code = code; + } + return error; +} + +function previousTimeoutError(retryConsumed: boolean): NodeJS.ErrnoException { + if (retryConsumed) { + const error = new Error( + "ACL hardening skipped — the previous timeout recovery was already consumed", + ) as NodeJS.ErrnoException; + error.code = "EACLRETRYEXHAUSTED"; + return error; + } + return sanitizedAclError( + "ACL hardening skipped — previous attempt timed out", + Object.assign(new Error("timeout"), { code: "ETIMEDOUT" }), + ); +} + +/** Consume, but never reset, the single explicit recovery attempt for this key. */ +function timeoutMemoErrorIfBlocked( + memoKey: string, + opts: HardenOptions, +): NodeJS.ErrnoException | null { + const retryConsumed = timedOutPaths.get(memoKey); + if (retryConsumed === undefined) return null; + if (opts.retryTimedOutOnce && retryConsumed === false) { + timedOutPaths.set(memoKey, true); + return null; + } + return previousTimeoutError(retryConsumed); +} + +function recordTimeout(memoKey: string): void { + if (!timedOutPaths.has(memoKey)) timedOutPaths.set(memoKey, false); +} + /** * Diagnostic-only post-timeout probe (never promotes to ok:true — a clean /findsid * does not prove inheritance was disabled or the user grant ran; only a fully @@ -588,10 +642,10 @@ function hardenEntry( if (effectivePlatform() !== "win32") return { ok: true }; if (memoSatisfied(cache, targetPath)) return { ok: true }; const memoKey = timeoutMemoKey(targetPath, opts); - if (timedOutPaths.has(memoKey)) { - const diagnostics = "ACL hardening skipped — previous attempt timed out"; - if (opts.required) throw new Error(diagnostics); - return { ok: false, diagnostics }; + const timeoutMemoError = timeoutMemoErrorIfBlocked(memoKey, opts); + if (timeoutMemoError) { + if (opts.required) throw timeoutMemoError; + return { ok: false, diagnostics: timeoutMemoError.message }; } const deadline = nowFn() + resolveHardenDeadlineMs(); @@ -606,6 +660,7 @@ function hardenEntry( if (opts.required) throw new Error(SUBSTITUTED_DIAGNOSTIC); return { ok: false, diagnostics: SUBSTITUTED_DIAGNOSTIC }; } + timedOutPaths.delete(memoKey); return { ok: true }; } catch (err) { // A substitution is not a transient icacls stall; do not spend the retry on it. @@ -617,14 +672,14 @@ function hardenEntry( const diagnostics = sanitizeDiagnostics(lastErr); if (isTimeoutError(lastErr)) { - timedOutPaths.add(memoKey); + recordTimeout(memoKey); const state = describeAclStateAfterTimeout(targetPath, deadline); const annotated = `${diagnostics}; ${state}`; - if (opts.required) throw new Error(annotated); + if (opts.required) throw sanitizedAclError(annotated, lastErr); console.warn(`[opencodex] ${annotated} — continuing without NTFS ACL harden`); return { ok: false, diagnostics: annotated }; } - if (opts.required) throw new Error(diagnostics); + if (opts.required) throw sanitizedAclError(diagnostics, lastErr); return { ok: false, diagnostics }; } @@ -639,10 +694,10 @@ async function hardenEntryAsync( if (effectivePlatform() !== "win32") return { ok: true }; if (memoSatisfied(cache, targetPath)) return { ok: true }; const memoKey = timeoutMemoKey(targetPath, opts); - if (timedOutPaths.has(memoKey)) { - const diagnostics = "ACL hardening skipped — previous attempt timed out"; - if (opts.required) throw new Error(diagnostics); - return { ok: false, diagnostics }; + const timeoutMemoError = timeoutMemoErrorIfBlocked(memoKey, opts); + if (timeoutMemoError) { + if (opts.required) throw timeoutMemoError; + return { ok: false, diagnostics: timeoutMemoError.message }; } const deadline = nowFn() + resolveHardenDeadlineMs(); @@ -656,6 +711,7 @@ async function hardenEntryAsync( if (opts.required) throw new Error(SUBSTITUTED_DIAGNOSTIC); return { ok: false, diagnostics: SUBSTITUTED_DIAGNOSTIC }; } + timedOutPaths.delete(memoKey); return { ok: true }; } catch (err) { if (err instanceof Error && err.message === SUBSTITUTED_DIAGNOSTIC) throw err; @@ -666,14 +722,14 @@ async function hardenEntryAsync( const diagnostics = sanitizeDiagnostics(lastErr); if (isTimeoutError(lastErr)) { - timedOutPaths.add(memoKey); + recordTimeout(memoKey); const state = await describeAclStateAfterTimeoutAsync(targetPath, deadline); const annotated = `${diagnostics}; ${state}`; - if (opts.required) throw new Error(annotated); + if (opts.required) throw sanitizedAclError(annotated, lastErr); console.warn(`[opencodex] ${annotated} — continuing without NTFS ACL harden`); return { ok: false, diagnostics: annotated }; } - if (opts.required) throw new Error(diagnostics); + if (opts.required) throw sanitizedAclError(diagnostics, lastErr); return { ok: false, diagnostics }; } diff --git a/tests/native-main-owner-lifetime.test.ts b/tests/native-main-owner-lifetime.test.ts index 1c7482d575..58610181da 100644 --- a/tests/native-main-owner-lifetime.test.ts +++ b/tests/native-main-owner-lifetime.test.ts @@ -252,6 +252,84 @@ function isContended(event: Event): boolean { } describe("native-main process owner lease", () => { + test("one coded ACL timeout retries once before ownership becomes held", async () => { + const f = fixture("acl-timeout-recovery"); + let attempts = 0; + const trace: string[] = []; + const owner = retainNativeMainOwner(f.manager.context, { + retryMs: 10, + hardenPath: async () => { + attempts += 1; + if (attempts === 1) throw Object.assign(new Error("transient"), { code: "ETIMEDOUT" }); + }, + }); + const unsubscribe = owner.subscribe(snapshot => { trace.push(snapshot.status); }); + try { + await waitUntil(() => owner.snapshot().status === "held" ? true : null); + expect(attempts).toBe(2); + expect(trace).toEqual(["acquiring", "held"]); + } finally { + unsubscribe(); + await owner.release(); + } + }); + + test("a second coded ACL timeout becomes terminal and a release cancels a pending retry", async () => { + const f = fixture("acl-timeout-terminal"); + let attempts = 0; + const trace: string[] = []; + const owner = retainNativeMainOwner(f.manager.context, { + retryMs: 10, + hardenPath: async () => { + attempts += 1; + throw Object.assign(new Error("transient"), { code: "ETIMEDOUT" }); + }, + }); + const unsubscribe = owner.subscribe(snapshot => { trace.push(snapshot.status); }); + try { + await waitUntil(() => owner.snapshot().status === "unavailable" ? true : null); + await Bun.sleep(50); + expect(attempts).toBe(2); + expect(trace).toEqual(["acquiring", "unavailable"]); + } finally { + unsubscribe(); + await owner.release(); + } + + const f2 = fixture("acl-timeout-release"); + let releasedAttempts = 0; + const releasing = retainNativeMainOwner(f2.manager.context, { + retryMs: 100, + hardenPath: async () => { + releasedAttempts += 1; + throw Object.assign(new Error("transient"), { code: "ETIMEDOUT" }); + }, + }); + await waitUntil(() => releasedAttempts === 1 ? true : null); + await releasing.release(); + await Bun.sleep(150); + expect(releasedAttempts).toBe(1); + }); + + test("an ETIMEDOUT-looking message without the code remains a permanent failure", async () => { + const f = fixture("acl-timeout-message-only"); + let attempts = 0; + const owner = retainNativeMainOwner(f.manager.context, { + retryMs: 10, + hardenPath: async () => { + attempts += 1; + throw new Error("ETIMEDOUT in untrusted prose"); + }, + }); + try { + await waitUntil(() => owner.snapshot().status === "unavailable" ? true : null); + await Bun.sleep(50); + expect(attempts).toBe(1); + } finally { + await owner.release(); + } + }); + test("same-process references retain one owner and do not deadlock the transaction lock", async () => { const f = fixture(); const first = retainNativeMainOwner(f.manager.context, { retryMs: 10, hardenPath: async () => {} }); diff --git a/tests/windows-secret-acl.test.ts b/tests/windows-secret-acl.test.ts index 26e07df0f9..acf3cb343f 100644 --- a/tests/windows-secret-acl.test.ts +++ b/tests/windows-secret-acl.test.ts @@ -576,7 +576,15 @@ describe("async hardenSecretPath (issue #612)", () => { test("async permission failure still throws on required paths", async () => { setAsyncIcaclsRunnerForTests(async () => denied); - await expect(hardenSecretPathAsync(secretFile(), { required: true })).rejects.toThrow(/EICACLS/); + let caught: unknown; + try { + await hardenSecretPathAsync(secretFile(), { required: true }); + } catch (error) { + caught = error; + } + expect(caught).toBeInstanceOf(Error); + expect((caught as NodeJS.ErrnoException).code).toBe("EICACLS"); + expect((caught as Error).message).toMatch(/EICACLS/); }); test("timeoutMemoKey shares the timeout cache across distinct temp paths", async () => { @@ -598,6 +606,66 @@ describe("async hardenSecretPath (issue #612)", () => { expect(calls).toBe(0); // destination-keyed memo; not a parent-directory shortcut }); + test("a required timeout preserves ETIMEDOUT and one explicit recovery gets a fresh budget", async () => { + const target = secretFile("one-time-recovery.json"); + let now = 0; + let grantCalls = 0; + setNowForTests(() => now); + setAsyncIcaclsRunnerForTests(async args => { + if (args.includes("/grant:r")) grantCalls += 1; + if (grantCalls === 1) { + now = 5_000; // exhaust the first call's entire budget + return timeout; + } + return ok; + }); + + let first: unknown; + try { + await hardenSecretPathAsync(target, { required: true }); + } catch (error) { + first = error; + } + expect((first as NodeJS.ErrnoException).code).toBe("ETIMEDOUT"); + expect((first as Error).message).not.toContain(target); + expect(timedOutSecretPathCountForTests()).toBe(1); + + await expect(hardenSecretPathAsync(target, { + required: true, + retryTimedOutOnce: true, + })).resolves.toEqual({ ok: true }); + expect(grantCalls).toBe(2); + expect(timedOutSecretPathCountForTests()).toBe(0); + }); + + test("the explicit timeout recovery cannot be consumed more than once", async () => { + const target = secretFile("consumed-recovery.json"); + let now = 0; + let grantCalls = 0; + setNowForTests(() => now); + setAsyncIcaclsRunnerForTests(async args => { + if (args.includes("/grant:r")) grantCalls += 1; + now += 5_000; + return timeout; + }); + + await expect(hardenSecretPathAsync(target, { required: true })).rejects.toMatchObject({ + code: "ETIMEDOUT", + }); + await expect(hardenSecretPathAsync(target, { + required: true, + retryTimedOutOnce: true, + })).rejects.toMatchObject({ code: "ETIMEDOUT" }); + const callsAfterRecovery = grantCalls; + await expect(hardenSecretPathAsync(target, { + required: true, + retryTimedOutOnce: true, + })).rejects.toMatchObject({ code: "EACLRETRYEXHAUSTED" }); + expect(grantCalls).toBe(callsAfterRecovery); + expect(grantCalls).toBe(2); + expect(timedOutSecretPathCountForTests()).toBe(1); + }); + test("optional timeout memo does not poison a later required harden of the same path", () => { setIcaclsRunnerForTests(() => timeout); const first = hardenSecretPath(secretFile(), { required: false }); @@ -1093,7 +1161,12 @@ describe("hardenStableLockFile — the production call edge, not just the primit const lockPath = join(testDir, "coordinator-posix.sqlite"); writeFileSync(lockPath, "x", "utf8"); chmodSync(lockPath, 0o644); - expect(statSync(lockPath).mode & 0o777).toBe(0o644); + // Windows does not expose POSIX mode bits faithfully even when this test + // forces the caller's platform branch to Linux. Keep the real mode proof on + // POSIX hosts; on Windows this case still proves no ACL command is invoked. + if (process.platform !== "win32") { + expect(statSync(lockPath).mode & 0o777).toBe(0o644); + } let calls = 0; setAsyncIcaclsRunnerForTests(async () => { @@ -1102,7 +1175,9 @@ describe("hardenStableLockFile — the production call edge, not just the primit }); try { await hardenStableLockFile(lockPath, "linux"); - expect(statSync(lockPath).mode & 0o777).toBe(0o600); + if (process.platform !== "win32") { + expect(statSync(lockPath).mode & 0o777).toBe(0o600); + } expect(calls).toBe(0); } finally { setAsyncIcaclsRunnerForTests(null); @@ -1141,6 +1216,7 @@ describe("the production default hardener is reached, with the resolved platform run: (codexHome: string) => Promise, onGrant: () => void = () => {}, gate?: Promise, + resultFor?: (args: string[]) => IcaclsResult | Promise, ): Promise => { const seen: string[][] = []; setPlatformForTests("win32"); @@ -1152,7 +1228,9 @@ describe("the production default hardener is reached, with the resolved platform // A deferred runner lets a test observe the window WHILE hardening is in // flight, which is the only way to assert nothing was published early. if (gate) await gate; - return { success: true, exitCode: 0, timedOut: false, stdout: "" }; + return resultFor + ? await resultFor(args) + : { success: true, exitCode: 0, timedOut: false, stdout: "" }; }); const codexHome = mkdtempSync(join(tmpdir(), "ocx-default-harden-")); try { @@ -1237,6 +1315,49 @@ describe("the production default hardener is reached, with the resolved platform expect(seen.some(args => args.includes("/grant:r"))).toBe(true); expect(seen.every(args => args[0] === expected)).toBe(true); }); + + test("the production owner default consumes one timeout memo and then acquires", async () => { + resetHardenedStateForTests(); + let now = 0; + let grantAttempts = 0; + const trace: string[] = []; + let expected = ""; + setNowForTests(() => now); + try { + const seen = await forcedWindows(async codexHome => { + expected = join(codexHome, NATIVE_MAIN_OWNER_DB); + const owner = retainNativeMainOwner( + { codexHome } as never, + { platform: "win32", retryMs: 10 }, + ); + const unsubscribe = owner.subscribe(snapshot => { trace.push(snapshot.status); }); + try { + const deadline = Date.now() + 5_000; + while (owner.snapshot().status === "acquiring" && Date.now() < deadline) { + await Bun.sleep(10); + } + expect(owner.snapshot()).toMatchObject({ status: "held" }); + } finally { + unsubscribe(); + await owner.release(); + } + }, () => {}, undefined, args => { + if (args.includes("/grant:r")) { + grantAttempts += 1; + if (grantAttempts === 1) { + now = 5_000; + return { success: false, exitCode: null, timedOut: true, stdout: "" }; + } + } + return { success: true, exitCode: 0, timedOut: false, stdout: "" }; + }); + expect(trace).toEqual(["acquiring", "held"]); + expect(grantAttempts).toBe(2); + expect(seen.every(args => args[0] === expected)).toBe(true); + } finally { + setNowForTests(null); + } + }); }); describe("a required hardening failure stops the operation it protects", () => { @@ -1324,6 +1445,7 @@ describe("a required hardening failure stops the operation it protects", () => { await forcedWindowsFailure(async codexHome => { expected = join(codexHome, NATIVE_MAIN_OWNER_DB); const owner = retainNativeMainOwner({ codexHome } as never, { platform: "win32", retryMs: 10 }); + let second: ReturnType | undefined; const trace: string[] = []; const unsubscribe = owner.subscribe(snapshot => { trace.push(snapshot.status); }); try { @@ -1353,8 +1475,13 @@ describe("a required hardening failure stops the operation it protects", () => { expect(hardenAttempts).toBe(1); // Exactly one attempt, against exactly the owner's own database. expect(targets).toEqual([expected]); + second = retainNativeMainOwner({ codexHome } as never, { platform: "win32", retryMs: 10 }); + await Bun.sleep(60); + expect(second.snapshot()).toMatchObject({ status: "unavailable" }); + expect(hardenAttempts).toBe(1); } finally { unsubscribe(); + if (second) await second.release(); await owner.release(); } }, args => { hardenAttempts += 1; targets.push(args[0]!); });