diff --git a/src/lib/config-ownership.ts b/src/lib/config-ownership.ts index 08b62e6760c..f41dbda8e1c 100644 --- a/src/lib/config-ownership.ts +++ b/src/lib/config-ownership.ts @@ -566,11 +566,18 @@ export function initializeConfigOwnership(configDir: string): boolean { // 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; + // A spent wait suppresses only the wait itself, never the reload: the identity it keeps + // may already name a completed pair. One plain load stays cheap for an unchanged invalid + // pair and lets a valid one register instead of sticking unowned. + ownership = loadOwnership(configDir); + if (ownership !== null) expiredCreationWait.delete(cacheKey); } else { ownership = loadOrCreateOwnership(configDir); const current = creationWaitIdentity(configDir); - if (ownership === null && current !== null) expiredCreationWait.set(cacheKey, current); + // Record only the identity the wait actually ran out on: unchanged across it, so its last + // failed read describes the same state. A pair that changed during the wait, including one + // that completed just after that read, spends its own wait on the next registration. + if (ownership === null && current !== null && current === observed) expiredCreationWait.set(cacheKey, current); else expiredCreationWait.delete(cacheKey); } ownershipCache.set(cacheKey, ownership); diff --git a/tests/config/config-ownership-concurrent-record.test.ts b/tests/config/config-ownership-concurrent-record.test.ts index 6d55dbeb6b5..e5658c26268 100644 --- a/tests/config/config-ownership-concurrent-record.test.ts +++ b/tests/config/config-ownership-concurrent-record.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; import { Database } from "bun:sqlite"; -import { chmodSync, existsSync, linkSync, mkdirSync, mkdtempSync, readFileSync, symlinkSync, writeFileSync } from "node:fs"; +import { chmodSync, existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, symlinkSync, utimesSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { @@ -70,6 +70,80 @@ function observeOwnerPublication(dir: string, completedOwner: string | null, cac const manifest = (dir: string) => JSON.parse(readFileSync(join(dir, CONFIG_UNINSTALL_MANIFEST), "utf8")) as { paths: string[]; uninstallStartedAt?: number }; +/** + * A fresh process that spends its publication wait on an invalid pair, then completes the pair + * while keeping the exact stat identity the spent wait recorded — what the race between a + * creation's last failed read and the spent-wait sampling produces (#8270 review). Returns the + * following registration's outcome; exit 4 means the identity could not be preserved. + */ +function registerAfterPairCompletesAtSpentIdentity(dir: string): string { + // Ownership metadata comes from a separate process: a process that creates it caches the + // valid pair and would answer later initialization without touching the files. + const seed = Bun.spawnSync([process.execPath, "-e", + "import { initializeConfigOwnership } from " + JSON.stringify(modulePath) + ";\n" + + "initializeConfigOwnership(" + JSON.stringify(dir) + ");"]); + expect(seed.exitCode, seed.stderr.toString()).toBe(0); + const script = [ + "import { lstatSync, readFileSync, utimesSync, writeFileSync } from \"node:fs\";", + "import { join } from \"node:path\";", + "import { initializeConfigOwnership, registerOwnedConfigPath } from " + JSON.stringify(modulePath) + ";", + "const dir = " + JSON.stringify(dir) + ";", + "const ownerPath = join(dir, \".opencodex-owner.json\");", + "const owner = readFileSync(ownerPath, \"utf8\");", + // A same-length owner id swap: complete in stat terms, an invalid pair semantically. + "const invalid = owner.replace(/\"ownerId\": \"[^\"]+\"/, '\"ownerId\": \"00000000-0000-4000-8000-000000000000\"');", + "writeFileSync(ownerPath, invalid);", + // A round-millisecond mtime the completing rewrite can restore exactly. + "const stamp = new Date(1_700_000_000_000);", + "utimesSync(ownerPath, stamp, stamp);", + "const spent = lstatSync(ownerPath, { bigint: true });", + "if (initializeConfigOwnership(dir)) throw new Error(\"invalid pair unexpectedly owned\");", + "writeFileSync(ownerPath, owner);", + "utimesSync(ownerPath, stamp, stamp);", + "const now = lstatSync(ownerPath, { bigint: true });", + "if (spent.dev !== now.dev || spent.ino !== now.ino || spent.size !== now.size || spent.mtimeNs !== now.mtimeNs) {", + " console.log(\"identity-not-preserved\"); process.exit(4);", + "}", + "console.log(JSON.stringify(registerOwnedConfigPath(dir, join(dir, \"after-completion\"))));", + ].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; +} + +/** + * A fresh process whose metadata pair changes mid-wait. Returns each registration's outcome and + * how long its ownership initialization took: a pair that changed while a wait ran has not spent + * its own wait yet, so the next registration must pay it once (#8270 review). + */ +function registerAfterPairChangedMidWait(dir: string): { first: boolean; second: boolean; third: boolean; firstMs: number; secondMs: number; thirdMs: number } { + const seed = Bun.spawnSync([process.execPath, "-e", + "import { initializeConfigOwnership } from " + JSON.stringify(modulePath) + ";\n" + + "initializeConfigOwnership(" + JSON.stringify(dir) + ");"]); + expect(seed.exitCode, seed.stderr.toString()).toBe(0); + const script = [ + "import { readFileSync, unlinkSync, writeFileSync } from \"node:fs\";", + "import { join } from \"node:path\";", + "import { initializeConfigOwnership } from " + JSON.stringify(modulePath) + ";", + "const dir = " + JSON.stringify(dir) + ";", + "const ownerPath = join(dir, \".opencodex-owner.json\");", + "const owner = readFileSync(ownerPath, \"utf8\");", + "const invalid = owner.replace(/\"ownerId\": \"[^\"]+\"/, '\"ownerId\": \"00000000-0000-4000-8000-000000000000\"');", + "unlinkSync(ownerPath);", + // Completing the owner bytes on the third clock read lands the change inside the wait. + "const realNow = Date.now; let reads = 0;", + "Date.now = () => { reads++; if (reads === 3) writeFileSync(ownerPath, invalid); return realNow(); };", + "let at = performance.now(); const first = initializeConfigOwnership(dir); const firstMs = performance.now() - at;", + "Date.now = realNow;", + "at = performance.now(); const second = initializeConfigOwnership(dir); const secondMs = performance.now() - at;", + "at = performance.now(); const third = initializeConfigOwnership(dir); const thirdMs = performance.now() - at;", + "console.log(JSON.stringify({ first, second, third, firstMs, secondMs, thirdMs }));", + ].join("\n"); + const child = Bun.spawnSync([process.execPath, "-e", script]); + expect(child.exitCode, child.stderr.toString()).toBe(0); + return JSON.parse(child.stdout.toString()); +} + function holdLock(dir: string): () => void { const database = new Database(join(dir, CONFIG_OWNERSHIP_LOCK), { create: false, readwrite: true }); database.exec("PRAGMA busy_timeout = 0; BEGIN IMMEDIATE"); @@ -895,4 +969,21 @@ describe("ownership edge cases from review (#8093)", () => { } finally { Bun.sleepSync = realSleep; } expect(waitedMs).toBeGreaterThanOrEqual(90); })); + + // #8270 review: a pair completing between the wait's last failed read and the spent-wait + // sampling must not stay unowned on the identity match; the record suppresses only the wait. + test("a spent wait recorded on a completed pair still reloads and records", withHome(dir => { + expect(registerAfterPairCompletesAtSpentIdentity(dir)).toBe("recorded"); + expect(manifest(dir).paths).toContain("after-completion"); + })); + + test("a pair changed during the publication wait spends its own wait", withHome(dir => { + const result = registerAfterPairChangedMidWait(dir); + expect([result.first, result.second, result.third]).toEqual([false, false, false]); + expect(result.firstMs).toBeGreaterThanOrEqual(90); + // The identity sampled before the wait no longer matches: the new pair waits once, then the + // honestly spent record suppresses further waits. + expect(result.secondMs).toBeGreaterThanOrEqual(90); + expect(result.thirdMs).toBeLessThan(result.secondMs); + })); });