diff --git a/src/codex/native-main-owner.ts b/src/codex/native-main-owner.ts index df2d7b7698..a15b6b7fdc 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,32 @@ export function nativeMainOwnerSnapshot(context: NativeProfileContext): NativeMa return entry ? { ...entry.snapshot } : null; } +/** + * Homes at or below `root` whose owner entry may still hold or reopen handles there. + * + * 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[] = []; + 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); + } + 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/structure/ops/test-sandbox-cleanup.md b/structure/ops/test-sandbox-cleanup.md index 0f52b0837c..9c68d01cf4 100644 --- a/structure/ops/test-sandbox-cleanup.md +++ b/structure/ops/test-sandbox-cleanup.md @@ -77,11 +77,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 bdd936dca1..11bc42443b 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,71 @@ 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); + } + }); + + 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); + } + }); }); diff --git a/tests/codex-integration/codex-auth-context.test.ts b/tests/codex-integration/codex-auth-context.test.ts index cdd00a433d..b26014ff72 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 5db587eed2..07a1191cb6 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 e64fde2fb8..86648bff57 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 cf0c80f58a..6469416ed3 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;