From 05772674fd0c277b39969377ad15366abf1ff7cd Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 22 Oct 2026 22:53:46 +0900 Subject: [PATCH 1/2] test: settle Windows ACL work and owner locks before fixture removal Three Windows fixture teardowns exhausted the removal retry budget (#7515). Their lock diagnostics show what blocked removal: in two of them the native-main owner or claim lock file under the home had an empty DACL. Real async icacls had rewritten it in suites that assert nothing about ACLs. Rename still worked, but recursive rm got EPERM. - codex-auth-context: the suite meant to keep real icacls out, but it stubbed only the sync runner, and the claim lock is hardened through the async one. Stub both, and tear down with drainAndRemoveFixtureRoots in place of the blocking remover, which never waited for ACL children. - server-management-auth: stub the async runner for the suite; the stub stays installed until removal ends. - main-account-hard-lock-fallback-routing: two cases reached removal with the config-dir ACL harden still in flight. Use the shared drain teardown. - fixture-teardown: before removing a root, refuse it and name any native-main owner still holding its SQLite lock below it, via the new nativeMainOwnerHomesOpenAtOrBelow probe. The teardown now fails at once, without spending the 15 s retry budget on EBUSY. No retry count, sleep, or timeout changes. Refs #7515 --- src/codex/native-main-owner.ts | 23 ++++++++- .../fixture-teardown-helper.test.ts | 36 ++++++++++++++ .../codex-auth-context.test.ts | 47 +++++++++++-------- ...account-hard-lock-fallback-routing.test.ts | 16 +++++-- tests/helpers/fixture-teardown.ts | 14 +++++- tests/server/server-management-auth.test.ts | 6 +++ 6 files changed, 116 insertions(+), 26 deletions(-) diff --git a/src/codex/native-main-owner.ts b/src/codex/native-main-owner.ts index df2d7b7698b..47f4dd248ec 100644 --- a/src/codex/native-main-owner.ts +++ b/src/codex/native-main-owner.ts @@ -1,4 +1,4 @@ -import { join } from "node:path"; +import { isAbsolute, join, relative, resolve, sep } from "node:path"; import { Database } from "bun:sqlite"; @@ -299,6 +299,27 @@ export function nativeMainOwnerSnapshot(context: NativeProfileContext): NativeMa return entry ? { ...entry.snapshot } : null; } +/** + * Homes at or below `root` whose owner entry still has its lock database or side + * descriptor open, including entries detached while their release is still closing. + * + * Removal code asks this before deleting a home. Windows refuses to delete a file + * SQLite holds open, so a removal that starts while the owner is still open fails with + * EBUSY only after its whole retry budget, without naming the owner. + */ +export function nativeMainOwnerHomesOpenAtOrBelow(root: string): string[] { + const fold = (path: string) => (process.platform === "win32" ? resolve(path).toLowerCase() : resolve(path)); + const base = fold(root); + const open: string[] = []; + for (const entry of [...entries.values(), ...detachedEntries]) { + if (!entry.database && !entry.file) continue; + const suffix = relative(base, fold(entry.context.codexHome)); + if (suffix === ".." || suffix.startsWith(`..${sep}`) || isAbsolute(suffix)) continue; + open.push(entry.context.codexHome); + } + return open; +} + export function assertNativeMainOwner(context: NativeProfileContext): void { const entry = entries.get(context.homeId); if (entry?.snapshot.status === "held" && entry.database && !entry.closing) return; diff --git a/tests/ci-workflows/fixture-teardown-helper.test.ts b/tests/ci-workflows/fixture-teardown-helper.test.ts index bdd936dca17..3bc8059aeed 100644 --- a/tests/ci-workflows/fixture-teardown-helper.test.ts +++ b/tests/ci-workflows/fixture-teardown-helper.test.ts @@ -1,4 +1,8 @@ import { describe, expect, test } from "bun:test"; +import { mkdirSync, mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { retainNativeMainOwner } from "../../src/codex/native-main-owner"; import { flushConfigDirHardening } from "../../src/config/paths"; import { defaultFixtureTeardownDeps, @@ -6,6 +10,7 @@ import { type FixtureTeardownDeps, type FixtureTeardownPlan, } from "../helpers/fixture-teardown"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; function harness(overrides: Partial = {}) { const calls: string[] = []; @@ -14,6 +19,7 @@ function harness(overrides: Partial = {}) { settleConfigFlights: async root => { calls.push(`flight:${root}`); }, closeHistory: () => { calls.push("history"); }, drainAcl: async root => { calls.push(`acl:${root}`); }, + openLocks: () => [], remove: root => { calls.push(`remove:${root}`); }, ...overrides, }; @@ -127,4 +133,34 @@ describe("fixture removal ownership", () => { h.plan.restoreEnvironment = () => { throw last; }; await expect(drainAndRemoveFixtureRoots(h.plan, h.deps)).rejects.toBe(first); }); + + test("an owner still open under a root keeps only that root and names it", async () => { + const h = harness({ openLocks: root => (root === "A" ? ["A/codex"] : []) }); + await expect(drainAndRemoveFixtureRoots(h.plan, h.deps)).rejects.toThrow("native-main owner still open for A/codex"); + expect(h.calls).not.toContain("remove:A"); + expect(h.calls).toContain("remove:B"); + expect(h.calls.filter(call => call === "restore")).toHaveLength(1); + }); + + test("the default lock probe sees a held owner below the root until its release closes", async () => { + const root = mkdtempSync(join(tmpdir(), "ocx-teardown-lock-")); + const codexHome = join(root, "codex"); + const sibling = `${root}-sibling`; + mkdirSync(codexHome); + const owner = retainNativeMainOwner( + { codexHome, homeId: `teardown-lock-${process.pid}` } as never, + { hardenPath: async () => {} }, + ); + try { + await new Promise(held => owner.subscribe(snapshot => { if (snapshot.status === "held") held(); })); + expect(defaultFixtureTeardownDeps.openLocks(root)).toEqual([codexHome]); + expect(defaultFixtureTeardownDeps.openLocks(codexHome)).toEqual([codexHome]); + expect(defaultFixtureTeardownDeps.openLocks(sibling)).toEqual([]); + await owner.release(); + expect(defaultFixtureTeardownDeps.openLocks(root)).toEqual([]); + } finally { + await owner.release(); + removeTreeWithRetry(root); + } + }); }); diff --git a/tests/codex-integration/codex-auth-context.test.ts b/tests/codex-integration/codex-auth-context.test.ts index cdd00a433db..b26014ff727 100644 --- a/tests/codex-integration/codex-auth-context.test.ts +++ b/tests/codex-integration/codex-auth-context.test.ts @@ -69,7 +69,7 @@ import { resetCodexRoutingForManualSelection, } from "../../src/codex/routing"; import type { OcxConfig, OcxProviderConfig } from "../../src/types"; -import { setIcaclsRunnerForTests } from "../../src/lib/windows-secret-acl"; +import { setAsyncIcaclsRunnerForTests, setIcaclsRunnerForTests } from "../../src/lib/windows-secret-acl"; import { blockNativeMainStartupForUnownedServiceHome, completeNativeMainRecovery, @@ -86,17 +86,19 @@ import { recordContextSessionOwner, clearContextSessionOwnersForTests } from ".. import { handleContextHistory } from "../../src/server/context-history"; import { resetContextRelayActivationForTests } from "../../src/codex/context-compat"; import { hasForwardableCodexBearer } from "../../src/server/auth-cors"; -import { removeTreeWithRetry } from "../helpers/remove-tree"; +import { drainAndRemoveFixtureRoots } from "../helpers/fixture-teardown"; let testDir: string; let previousOpencodexHome: string | undefined; let previousCodexHome: string | undefined; beforeEach(() => { - // This suite validates refresh admission and auth-context outcomes. Real icacls - // processes are covered elsewhere and can retain temp-dir handles long enough - // to obscure those assertions under Windows isolated-test load. + // This suite validates refresh admission and auth-context outcomes; real icacls is + // covered by tests/windows. Both runners are stubbed: the native-main claim lock is + // hardened through the ASYNC runner, and with only the sync one stubbed, real icacls + // still rewrote that lock's DACL under the home this teardown removes (#7515). setIcaclsRunnerForTests(() => ({ success: true, exitCode: 0, timedOut: false, stdout: "" })); + setAsyncIcaclsRunnerForTests(async () => ({ success: true, exitCode: 0, timedOut: false, stdout: "" })); testDir = mkdtempSync(join(tmpdir(), "ocx-auth-ctx-")); previousOpencodexHome = process.env.OPENCODEX_HOME; process.env.OPENCODEX_HOME = testDir; @@ -113,21 +115,28 @@ beforeEach(() => { clearAccountNeedsReauth("pool-b"); }); -afterEach(() => { +afterEach(async () => { resetContextRelayActivationForTests(); - setIcaclsRunnerForTests(null); - removeTreeWithRetry(testDir); - clearThreadAccountMap(); - clearCodexUpstreamHealth(); - clearAccountQuota(); - setMainAccountPlan(null); - __resetGuardianState(); - clearAccountNeedsReauth("pool-a"); - clearAccountNeedsReauth("pool-b"); - if (previousOpencodexHome === undefined) delete process.env.OPENCODEX_HOME; - else process.env.OPENCODEX_HOME = previousOpencodexHome; - if (previousCodexHome === undefined) delete process.env.CODEX_HOME; - else process.env.CODEX_HOME = previousCodexHome; + // Settle producers, config flights and ACL work, and refuse a home whose native-main + // owner is still open, before removal; the stubs stay installed until removal ends. + await drainAndRemoveFixtureRoots({ + roots: [{ path: testDir }], + restoreEnvironment: () => { + setIcaclsRunnerForTests(null); + setAsyncIcaclsRunnerForTests(null); + clearThreadAccountMap(); + clearCodexUpstreamHealth(); + clearAccountQuota(); + setMainAccountPlan(null); + __resetGuardianState(); + clearAccountNeedsReauth("pool-a"); + clearAccountNeedsReauth("pool-b"); + if (previousOpencodexHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousOpencodexHome; + if (previousCodexHome === undefined) delete process.env.CODEX_HOME; + else process.env.CODEX_HOME = previousCodexHome; + }, + }); }); function config(): OcxConfig { diff --git a/tests/codex-integration/main-account-hard-lock-fallback-routing.test.ts b/tests/codex-integration/main-account-hard-lock-fallback-routing.test.ts index 5db587eed26..07a1191cb6c 100644 --- a/tests/codex-integration/main-account-hard-lock-fallback-routing.test.ts +++ b/tests/codex-integration/main-account-hard-lock-fallback-routing.test.ts @@ -18,7 +18,7 @@ import { captureConfigGeneration } from "../../src/lib/state-store-sweeper"; import type { RequestLogContext } from "../../src/server/request-log"; import type { OcxConfig } from "../../src/types"; import { acquireOwnedSpendHome } from "../helpers/owned-spend-home"; -import { removeTreeWithRetry } from "../helpers/remove-tree"; +import { drainAndRemoveFixtureRoots } from "../helpers/fixture-teardown"; const now = Date.UTC(2026, 8, 5); const FALLBACK = { model: "deepseek/deepseek-chat", reasoningEffort: "low" }; @@ -39,7 +39,7 @@ beforeEach(() => { observeMainQuotaIdentity("fallback-account-a"); }); -afterEach(() => { +afterEach(async () => { releaseSpendHome?.(); releaseSpendHome = undefined; globalThis.fetch = originalFetch; @@ -47,9 +47,15 @@ afterEach(() => { clearMainAccountInfoCache(); deleteAllScopedHealth("pool-a"); clearComboRecallForTests(); - if (previousHome === undefined) delete process.env.OPENCODEX_HOME; - else process.env.OPENCODEX_HOME = previousHome; - removeTreeWithRetry(home); + // Saving a pool account starts the config-dir ACL harden without awaiting it; the + // shared teardown settles that flight and its icacls child before removing the home. + await drainAndRemoveFixtureRoots({ + roots: [{ path: home }], + restoreEnvironment: () => { + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + }, + }); }); function observe(quota: Omit): void { diff --git a/tests/helpers/fixture-teardown.ts b/tests/helpers/fixture-teardown.ts index e64fde2fb87..86648bff577 100644 --- a/tests/helpers/fixture-teardown.ts +++ b/tests/helpers/fixture-teardown.ts @@ -1,4 +1,5 @@ import { flushNativeMainStartupReleases } from "../../src/codex/native-profile-startup"; +import { nativeMainOwnerHomesOpenAtOrBelow } from "../../src/codex/native-main-owner"; import { flushConfigDirHardening } from "../../src/config/paths"; import { closeRequestHistoryIndex } from "../../src/routing/history/indexer"; import { flushWindowsSecretAclReapsBeforeRemoval } from "../../src/lib/windows-secret-acl"; @@ -11,6 +12,7 @@ export interface FixtureTeardownDeps { settleConfigFlights(root: string): Promise; closeHistory(): void; drainAcl(root: string): Promise; + openLocks(root: string): string[]; remove(path: string): void; } export const defaultFixtureTeardownDeps: Readonly = { @@ -18,9 +20,18 @@ export const defaultFixtureTeardownDeps: Readonly = { settleConfigFlights: flushConfigDirHardening, closeHistory: closeRequestHistoryIndex, drainAcl: flushWindowsSecretAclReapsBeforeRemoval, + openLocks: nativeMainOwnerHomesOpenAtOrBelow, remove: removeTreeWithRetry, }; +/** A lock this process still holds keeps its root: the owner is named now, not after a retry budget. */ +function assertNoOpenLocks(root: string, openLocks: (root: string) => string[]): void { + const open = openLocks(root); + if (open.length === 0) return; + throw new Error(`[test] not removing ${root}: native-main owner still open for ${open.join(", ")}; ` + + "stop the server or release the owner before teardown"); +} + /** Remove only roots whose owners settled; restoration cannot mask the first failure. */ export async function drainAndRemoveFixtureRoots( plan: FixtureTeardownPlan, @@ -39,7 +50,8 @@ export async function drainAndRemoveFixtureRoots( const historyClosed = await attempt(() => deps.closeHistory()); for (const [index, root] of plan.roots.entries()) { const drained = await attempt(() => deps.drainAcl(root.path)); - ready[index] = ready[index] && drained; + const unlocked = drained && await attempt(() => assertNoOpenLocks(root.path, deps.openLocks)); + ready[index] = ready[index] && unlocked; } if (producersSettled && historyClosed) { for (const [index, root] of plan.roots.entries()) { diff --git a/tests/server/server-management-auth.test.ts b/tests/server/server-management-auth.test.ts index cf0c80f58a4..6469416ed34 100644 --- a/tests/server/server-management-auth.test.ts +++ b/tests/server/server-management-auth.test.ts @@ -28,6 +28,7 @@ import { hardenSecretPath, hardenedSecretPathCountForTests, resetHardenedStateForTests, + setAsyncIcaclsRunnerForTests, setIcaclsRunnerForTests, setPlatformForTests, timedOutSecretPathCountForTests, @@ -117,6 +118,10 @@ beforeEach(() => { resetContextRelayActivationForTests(); process.env.OPENCODEX_API_AUTH_TOKEN = "data-secret"; process.env.OPENCODEX_ADMIN_AUTH_TOKEN = "admin-secret"; + // These cases assert credential separation, not ACLs (tests/windows owns those). Real + // async icacls on the native-main owner lock under testHome left it with an empty DACL + // that the teardown could not delete (#7515). Sync-runner cases still inject their own. + setAsyncIcaclsRunnerForTests(async () => ({ success: true, exitCode: 0, timedOut: false, stdout: "" })); }); afterEach(async () => { @@ -130,6 +135,7 @@ afterEach(async () => { else process.env.CODEX_HOME = previousCodexHome; setSystemRestartIoForTests(); setIcaclsRunnerForTests(null); + setAsyncIcaclsRunnerForTests(null); setPlatformForTests(null); resetHardenedStateForTests(); if (previousHome === undefined) delete process.env.OPENCODEX_HOME; From e54251224f210efd92a996705dbf311214dc4689 Mon Sep 17 00:00:00 2001 From: JUN Date: Fri, 23 Oct 2026 00:21:12 +0900 Subject: [PATCH 2/2] test: treat an acquiring, retrying or closing native-main owner as open The teardown probe saw only published handles. While prepareOwnerDatabase awaits its hardener, the stable descriptor and SQLite database live in locals, so the probe reported no owner and teardown could remove a home the process still held. Treat an in-flight acquisition, a pending retry timer and any detached (closing) entry as open. Regression: a real owner with its hardener paused keeps its root through acquisition and release, and the root is removed only once release ends. Document the owner check in structure/ops/test-sandbox-cleanup.md. Refs #7515 --- src/codex/native-main-owner.ts | 19 ++++++---- structure/ops/test-sandbox-cleanup.md | 15 +++++--- .../fixture-teardown-helper.test.ts | 37 +++++++++++++++++++ 3 files changed, 59 insertions(+), 12 deletions(-) diff --git a/src/codex/native-main-owner.ts b/src/codex/native-main-owner.ts index 47f4dd248ec..a15b6b7fdc4 100644 --- a/src/codex/native-main-owner.ts +++ b/src/codex/native-main-owner.ts @@ -300,19 +300,24 @@ export function nativeMainOwnerSnapshot(context: NativeProfileContext): NativeMa } /** - * Homes at or below `root` whose owner entry still has its lock database or side - * descriptor open, including entries detached while their release is still closing. + * Homes at or below `root` whose owner entry may still hold or reopen handles there. * - * Removal code asks this before deleting a home. Windows refuses to delete a file - * SQLite holds open, so a removal that starts while the owner is still open fails with - * EBUSY only after its whole retry budget, without naming the owner. + * Conservative on purpose: besides a published database or side descriptor, an + * acquisition in flight (`drive`) holds its descriptor and database in locals until + * hardening returns, a pending retry timer will reopen them, and a detached entry is + * still closing. Removal code asks this before deleting a home. Windows refuses to + * delete a file SQLite holds open, so a removal that starts early fails with EBUSY + * only after its whole retry budget, without naming the owner. */ export function nativeMainOwnerHomesOpenAtOrBelow(root: string): string[] { const fold = (path: string) => (process.platform === "win32" ? resolve(path).toLowerCase() : resolve(path)); const base = fold(root); const open: string[] = []; - for (const entry of [...entries.values(), ...detachedEntries]) { - if (!entry.database && !entry.file) continue; + const candidates = [ + ...[...entries.values()].filter(entry => entry.database || entry.file || entry.drive || entry.timer), + ...detachedEntries, + ]; + for (const entry of candidates) { const suffix = relative(base, fold(entry.context.codexHome)); if (suffix === ".." || suffix.startsWith(`..${sep}`) || isAbsolute(suffix)) continue; open.push(entry.context.codexHome); diff --git a/structure/ops/test-sandbox-cleanup.md b/structure/ops/test-sandbox-cleanup.md index 882147d6d1c..a8349d35a0b 100644 --- a/structure/ops/test-sandbox-cleanup.md +++ b/structure/ops/test-sandbox-cleanup.md @@ -73,11 +73,16 @@ fixture-specific cleanup or claim OS ACL coverage for synthetic tests. case roots (`tests/codex-integration/codex-quota-auto-refresh.test.ts`), and Anthropic instance root removal (`tests/helpers/anthropic-instance-fixture.ts`). It settles producers, then each root's config flights, closes the request-history SQLite index, -drains each root's ACL work, and only then removes drained roots. A producer or history failure -blocks every removal; a root-specific drain failure preserves that root while other drained roots -can be removed. Removal failures do not stop later roots. Environment restoration runs in finally, -and the first failure is rethrown. Undrained roots remain for run-level sandbox cleanup. -`tests/ci-workflows/fixture-teardown-helper.test.ts` verifies ordering and failure gating with injected owners. +drains each root's ACL work, then asks `nativeMainOwnerHomesOpenAtOrBelow` whether a native-main +owner below the root is still open, acquiring (descriptor and SQLite handle held while its hardener +runs), waiting on a retry, or closing; only roots that pass both are removed. An open owner fails +that root at once with an error naming the owner home, without spending the removal retry budget. +A producer or history failure blocks every removal; a root-specific drain or owner failure +preserves that root while other ready roots can be removed. Removal failures do not stop later +roots. Environment restoration runs in finally, and the first failure is rethrown. Undrained roots +remain for run-level sandbox cleanup. +`tests/ci-workflows/fixture-teardown-helper.test.ts` verifies ordering and failure gating with injected +owners, and with a real owner whose hardener is paused mid-acquisition. `tests/helpers/spawn-test-child.ts` owns behavior and warm-up children on the parent's live event loop. It captures output continuously, kills at the child deadline, and bounds exit/pipe diff --git a/tests/ci-workflows/fixture-teardown-helper.test.ts b/tests/ci-workflows/fixture-teardown-helper.test.ts index 3bc8059aeed..11bc42443ba 100644 --- a/tests/ci-workflows/fixture-teardown-helper.test.ts +++ b/tests/ci-workflows/fixture-teardown-helper.test.ts @@ -163,4 +163,41 @@ describe("fixture removal ownership", () => { removeTreeWithRetry(root); } }); + + test("an owner paused mid-acquisition keeps its root until preparation and release finish", async () => { + const root = mkdtempSync(join(tmpdir(), "ocx-teardown-acquire-")); + const codexHome = join(root, "codex"); + mkdirSync(codexHome); + let entered!: () => void, resume!: () => void; + const hardening = new Promise(done => { entered = done; }); + const resumed = new Promise(done => { resume = done; }); + const owner = retainNativeMainOwner( + { codexHome, homeId: `teardown-acquire-${process.pid}` } as never, + { hardenPath: async () => { entered(); await resumed; } }, + ); + const removed: string[] = []; + const plan = (): FixtureTeardownPlan => ({ roots: [{ path: root, remove: path => { removed.push(path); removeTreeWithRetry(path); } }], restoreEnvironment: () => {} }); + try { + // The descriptor and SQLite handle exist only in locals while the hardener runs. + await hardening; + expect(owner.snapshot().status).toBe("acquiring"); + await expect(drainAndRemoveFixtureRoots(plan())).rejects.toThrow(`native-main owner still open for ${codexHome}`); + expect(removed).toEqual([]); + + const released = owner.release(); + // Still closing: the release waits for the paused acquisition. + await expect(drainAndRemoveFixtureRoots(plan())).rejects.toThrow("native-main owner still open"); + expect(removed).toEqual([]); + + resume(); + await released; + expect(defaultFixtureTeardownDeps.openLocks(root)).toEqual([]); + await drainAndRemoveFixtureRoots(plan()); + expect(removed).toEqual([root]); + } finally { + resume(); + await owner.release(); + if (removed.length === 0) removeTreeWithRetry(root); + } + }); });