diff --git a/src/lib/config-ownership.ts b/src/lib/config-ownership.ts index b9d5a0c1a0f..08b62e6760c 100644 --- a/src/lib/config-ownership.ts +++ b/src/lib/config-ownership.ts @@ -149,6 +149,13 @@ let lastReconciledGeneration = 0; */ const lockedManifestIdentity = new Map(); const warnedFullManifest = new Set(); +/** + * The metadata state in which a creation wait already ran out for a home, keyed as + * {@link ownershipCache}. An unowned home whose manifest and owner marker keep the same identity + * has no publication left to wait for (a stopped creation, or a corrupt marker), so a registration + * must not pay the bounded wait again; either file changing re-arms a single wait. + */ +const expiredCreationWait = new Map(); /** * Open while this process runs `ocx uninstall` itself. Its own registrations then skip the * tombstone check: the same process runs removeOwnedConfigState afterwards, whose locked snapshot @@ -190,6 +197,27 @@ function manifestIdentity(configDir: string): string | undefined { } } +/** + * Which file each ownership metadata name refers to while a manifest exists, in the same + * dev:ino:size:mtime terms {@link manifestIdentity} uses, or null when no manifest exists: without + * one the publication wait ends in a single pass, so there is no spent wait to remember. The + * manifest is replaced atomically and the owner marker written once, so either file changing + * produces a new identity and re-arms a single wait. + */ +function creationWaitIdentity(configDir: string): string | null { + const describe = (name: string): string | null => { + try { + const entry = lstatSync(join(configDir, name), { bigint: true }); + return `${entry.dev}:${entry.ino}:${entry.size}:${entry.mtimeNs}`; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + return code === "ENOENT" ? null : `unreadable:${code ?? "unknown"}`; + } + }; + const manifest = describe(CONFIG_UNINSTALL_MANIFEST); + return manifest === null ? null : `${manifest}|${describe(CONFIG_OWNER_FILE)}`; +} + export function listLiveConfigOwnershipRoots(currentConfigDir: string): ReadonlySet { const currentRoot = ownershipCacheKey(currentConfigDir); const roots = new Set([currentRoot]); @@ -211,6 +239,7 @@ export function reconcileConfigOwnershipRoots(context: GenerationContext): numbe existsSync(join(root, ...rel.split("/")))); if (hasLiveOwnedPath) continue; ownershipCache.delete(root); + expiredCreationWait.delete(root); removed += 1; } lastReconciledGeneration = context.generation; @@ -525,24 +554,24 @@ export function initializeConfigOwnership(configDir: string): boolean { const cacheKey = ownershipCacheKey(configDir); if (!existsSync(configDir)) { ownershipCache.delete(cacheKey); + expiredCreationWait.delete(cacheKey); mkdirSync(configDir, { recursive: true, mode: 0o700 }); } let ownership = ownershipCache.get(cacheKey); - if (ownership === undefined) { - ownership = loadOrCreateOwnership(configDir); - ownershipCache.set(cacheKey, ownership); - } else if (ownership === null) { - // A home cached as unowned may have finished being created by another process since: re-read - // the metadata (no waiting) so a late owner marker is picked up. A root that has become empty - // since is claimed like an absent one; a nonempty unowned root still never is. - ownership = loadOwnership(configDir); - if (!ownership) { - try { - ownership = createOwnership(configDir); - } catch (error) { - if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error; - ownership = loadOwnership(configDir); - } + if (ownership === undefined || ownership === null) { + // A previously unowned home can enter creation later; use the same bounded publication wait, + // but spend it at most once per unchanged metadata pair. A manifest left without a valid + // owner marker — a stopped creation, or a corrupt marker — is not a creation still in + // progress, and registrations run on the request path: charging each of them the full wait + // stalls every request to that home. Either file changing re-arms a single wait. + const observed = creationWaitIdentity(configDir); + if (observed !== null && expiredCreationWait.get(cacheKey) === observed) { + ownership = null; + } else { + ownership = loadOrCreateOwnership(configDir); + const current = creationWaitIdentity(configDir); + if (ownership === null && current !== null) expiredCreationWait.set(cacheKey, current); + else expiredCreationWait.delete(cacheKey); } ownershipCache.set(cacheKey, ownership); } @@ -573,13 +602,13 @@ function loadOwnershipForUninstall(configDir: string): { owner: ConfigOwner; man return loadOwnership(configDir) ?? tombstonedManifestWithoutOwner(configDir); } -/** How long a first registration waits for another process that is creating the same home. */ +/** How long an uncached or unowned registration waits for another process creating the same home. */ const CONCURRENT_CREATION_WAIT_MS = 100; /** * Load the ownership metadata, or create it in an empty root. Two processes can start on the same * absent home: the loser of the exclusive manifest write, or one that sees the manifest before the - * owner marker (creation writes them in that order), waits briefly and loads the winner's metadata + * complete owner marker (creation writes them in that order), waits briefly and loads the winner's metadata * instead of caching the home as unowned for the rest of its life. */ function loadOrCreateOwnership(configDir: string): { owner: ConfigOwner; manifest: ConfigUninstallManifest } | null { @@ -593,7 +622,9 @@ function loadOrCreateOwnership(configDir: string): { owner: ConfigOwner; manifes } catch (error) { if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error; } - const creating = existsSync(join(configDir, CONFIG_UNINSTALL_MANIFEST)) && !existsSync(join(configDir, CONFIG_OWNER_FILE)) + // Exclusive create publishes the owner file name before its JSON bytes are complete. + // Only a valid matching pair proves ownership; file presence alone must not end this wait. + const creating = existsSync(join(configDir, CONFIG_UNINSTALL_MANIFEST)) && !tombstonedManifestWithoutOwner(configDir); if (!creating || Date.now() >= deadline) return loadOwnership(configDir); Bun.sleepSync(5); diff --git a/structure/config-ownership.md b/structure/config-ownership.md index 6f3a7ad7cf4..2086cf9b6e4 100644 --- a/structure/config-ownership.md +++ b/structure/config-ownership.md @@ -62,9 +62,11 @@ before the tombstone can still create its file afterwards; see the last item bel including one from a process that cached the ownership earlier, is refused as an uninstall in progress, not treated as an unowned home. A home cached as unowned is re-read on later registrations, so an owner marker another process writes late is picked up, and a root that has - become empty is claimed like an absent one. A first registration that finds a manifest without an - owner marker waits up to 100 ms for the process creating the home, then records nothing; uninstall - reports a file it writes meanwhile as a residual path. + become empty is claimed like an absent one. An uncached or previously unowned registration with a + manifest but no valid matching ownership pair waits up to 100 ms for publication to finish, including when the owner + file exists but its JSON is still being written. A complete pair loads immediately; a tombstoned + manifest without an owner is not treated as creation. Expiry records nothing, and damaged or + mismatched metadata never grants ownership; uninstall reports an unrecorded file as a residual. - An interrupted uninstall (a crash, or a path it could not remove) leaves the tombstone. Registrations keep refusing, and the next `ocx uninstall` finishes from the same manifest. The rerun's own cleanup steps (Desktop picker, disconnect, integrations) run inside `beginConfigOwnershipTeardown`, where this diff --git a/tests/config/config-ownership-concurrent-record.test.ts b/tests/config/config-ownership-concurrent-record.test.ts index 48037798bea..f6a83459d7d 100644 --- a/tests/config/config-ownership-concurrent-record.test.ts +++ b/tests/config/config-ownership-concurrent-record.test.ts @@ -42,6 +42,31 @@ const childScript = (dir: string, rel: string, gate?: { ready: string; go: strin ].join("\n"); const spawnChild = (dir: string, rel: string, gate?: { ready: string; go: string }) => Bun.spawn([process.execPath, "-e", childScript(dir, rel, gate)], { stderr: "inherit" }); + +/** Complete the owner's JSON at the first creation poll, without relying on process scheduling. */ +function observeOwnerPublication(dir: string, completedOwner: string | null, cacheUnowned = false): string { + const script = [ + "import { writeFileSync } from \"node:fs\";", + "import { initializeConfigOwnership, registerOwnedConfigPath } from " + JSON.stringify(modulePath) + ";", + cacheUnowned ? "if (initializeConfigOwnership(" + JSON.stringify(dir) + ")) throw new Error(\"fixture unexpectedly owned before publication\");" : "", + // The spent publication wait is remembered per unchanged metadata pair; only a changed owner + // marker re-arms it, so the cached-unowned publication case needs a new incomplete marker to + // wait on. The unfinished variants keep theirs: unchanged files must not re-enter the wait. + cacheUnowned && completedOwner !== null ? "writeFileSync(" + JSON.stringify(join(dir, ".opencodex-owner.json")) + ", \"still-incomplete\");" : "", + "const completedOwner = " + JSON.stringify(completedOwner) + ";", + "const realNow = Date.now; let reads = 0;", + "Date.now = () => {", + " reads++;", + " if (completedOwner !== null && reads === 2) writeFileSync(" + JSON.stringify(join(dir, ".opencodex-owner.json")) + ", completedOwner);", + // An abandoned publication reaches the existing deadline without a wall-clock sleep. + " return completedOwner === null ? reads * 20_000 : realNow();", + "};", + "console.log(JSON.stringify(registerOwnedConfigPath(" + JSON.stringify(dir) + ", " + JSON.stringify(join(dir, "after-publication")) + ")));", + ].join("\n"); + const child = Bun.spawnSync([process.execPath, "-e", script]); + expect(child.exitCode, child.stderr.toString()).toBe(0); + return JSON.parse(child.stdout.toString()) as string; +} const manifest = (dir: string) => JSON.parse(readFileSync(join(dir, CONFIG_UNINSTALL_MANIFEST), "utf8")) as { paths: string[]; uninstallStartedAt?: number }; @@ -798,6 +823,34 @@ describe("ownership edge cases from review (#8093)", () => { expect(manifest(dir).paths).toContain("later"); })); + for (const cached of [false, true]) for (const state of ["empty", "mismatched"] as const) { + const context = cached ? "cached-unowned" : "uncached"; + test(`${context} registration waits for ${state} owner publication`, withHome(dir => { + expect(recordOwnedConfigPath(dir, join(dir, "seed"))).toBe(true); + const ownerPath = join(dir, ".opencodex-owner.json"); + const owner = readFileSync(ownerPath, "utf8"); + // Exclusive create exposes the file name before writeFileSync finishes its bytes. + const incomplete = state === "empty" ? "" : JSON.stringify({ ...JSON.parse(owner), ownerId: "00000000-0000-4000-8000-000000000000" }); + writeFileSync(ownerPath, incomplete); + expect(observeOwnerPublication(dir, owner, cached)).toBe("recorded"); + expect(readFileSync(ownerPath, "utf8")).toBe(owner); + expect(manifest(dir).paths).toContain("after-publication"); + })); + + test(`${context} unfinished ${state} owner publication stays unowned and unchanged`, withHome(dir => { + expect(recordOwnedConfigPath(dir, join(dir, "seed"))).toBe(true); + const ownerPath = join(dir, ".opencodex-owner.json"); + const owner = JSON.parse(readFileSync(ownerPath, "utf8")); + const invalid = state === "empty" ? "" : JSON.stringify({ ...owner, ownerId: "00000000-0000-4000-8000-000000000000" }); + writeFileSync(ownerPath, invalid); + const before = readFileSync(join(dir, CONFIG_UNINSTALL_MANIFEST), "utf8"); + expect(observeOwnerPublication(dir, null, cached)).toBe("unowned"); + expect(readFileSync(ownerPath, "utf8")).toBe(invalid); + expect(readFileSync(join(dir, CONFIG_UNINSTALL_MANIFEST), "utf8")).toBe(before); + expect(existsSync(join(dir, "after-publication"))).toBe(false); + })); + } + test("a first registration waits for a home another process is still creating", withHome(dir => { expect(recordOwnedConfigPath(dir, join(dir, "seed"))).toBe(true); removeTreeWithRetry(join(dir, ".opencodex-owner.json")); @@ -811,4 +864,32 @@ describe("ownership edge cases from review (#8093)", () => { expect(result.recorded).toBe(false); expect(result.waited).toBeGreaterThanOrEqual(90); })); + + test("an unchanged ownerless manifest spends the publication wait once, not per registration", withHome(dir => { + expect(recordOwnedConfigPath(dir, join(dir, "seed"))).toBe(true); + const ownerPath = join(dir, ".opencodex-owner.json"); + const owner = JSON.parse(readFileSync(ownerPath, "utf8")); + // A creation stopped after the manifest, or a corrupt marker: the pair never validates. + writeFileSync(ownerPath, JSON.stringify({ ...owner, ownerId: "00000000-0000-4000-8000-000000000000" })); + // The locked reload drops the stale cache entry; the next registration pays the one wait. + expect(recordOwnedConfigPath(dir, join(dir, "observer"))).toBe(false); + let sleeps = 0; + const realSleep = Bun.sleepSync; + Bun.sleepSync = (ms: number) => { sleeps += 1; realSleep(ms); }; + let waitedMs = 0; + try { + const first = performance.now(); + expect(recordOwnedConfigPath(dir, join(dir, "first"))).toBe(false); + waitedMs = performance.now() - first; + const firstSleeps = sleeps; + expect(firstSleeps).toBeGreaterThan(0); + const repeats = performance.now(); + for (let i = 0; i < 20; i += 1) expect(recordOwnedConfigPath(dir, join(dir, `repeat-${i}`))).toBe(false); + // Twenty repeats stay under the single wait the first registration already paid: with the + // wait wrongly spent per call this would take twenty times as long. + expect(performance.now() - repeats).toBeLessThan(waitedMs); + expect(sleeps - firstSleeps).toBe(0); + } finally { Bun.sleepSync = realSleep; } + expect(waitedMs).toBeGreaterThanOrEqual(90); + })); });