From cabf95195b43471e031df604e479112b9c7eaf27 Mon Sep 17 00:00:00 2001 From: Epinephrine Date: Mon, 9 Nov 2026 13:10:34 +0900 Subject: [PATCH] fix(config): revalidate ownership before honoring a spent publication wait A spent publication wait was remembered under the identity sampled after loadOrCreateOwnership() returned. Another process completing the owner marker between the wait's last failed read and that sampling stored the completed pair's identity, so every later registration matched it and returned unowned without reloading - the bounded timeout became a persistent registration failure. - On an identity match, run one plain loadOwnership() before refusing: the record suppresses the wait, never the reload. - Only record a spent wait when the identity stayed unchanged across it, so it always names the state the last failed read actually saw; a pair that changed during the wait spends its own wait next registration. Reported by chatgpt-codex-connector review on #8270. --- src/lib/config-ownership.ts | 11 ++- ...config-ownership-concurrent-record.test.ts | 93 ++++++++++++++++++- 2 files changed, 101 insertions(+), 3 deletions(-) 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); + })); });