From 80ccfbd703d2e12191439e437eca7687b2e85b89 Mon Sep 17 00:00:00 2001 From: twoimo Date: Fri, 31 Jul 2026 05:39:13 +0900 Subject: [PATCH] fix(config): bind cross-process file locks to their owning host Owner liveness is probed against the local process table, but a state directory on a shared volume (NFS home directories are supported) is contended by several hosts. A remote owner's pid is almost never a live local pid, so a foreign host's freshly-taken lock read as `dead` and was reclaimed immediately with no stale grace period, letting two hosts run the same critical section concurrently. Stamp the owning host into the lock record. A foreign-host owner is now reclaimable only after the `staleMs` elapsed-time heuristic - the same grace a local owner of indeterminate liveness receives - and host identity is part of owner equality for the guarded lock-dir removal, so one host cannot delete another's lock on a pid/timestamp coincidence. Records without a host id keep the previous local-pid semantics. --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/config/file-lock.ts | 62 ++++++- .../test/file-lock-cross-host.test.ts | 161 ++++++++++++++++++ 3 files changed, 220 insertions(+), 4 deletions(-) create mode 100644 packages/coding-agent/test/file-lock-cross-host.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index b4506214d6..fa10ad2b9d 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -8,6 +8,7 @@ - Fixture quality gates that complete intermediate Ultragoal stories now write file-backed adversarial artifact proof; skill-state hooks and computer red-team fixtures match the unconditional adversarial path check so #3543 CI stays fail-closed without weakening hydration exactness (#3543). - Runtime settings reconciliation now validates every `web_search.fallback` entry against the declared provider enum instead of accepting unsupported or non-string array items (#3601). - Ultragoal critic-gate, dogfood, review, durable-completion, and runtime test suites now pin `CI_DEV_CHANGED_PATHS` hermetically in their setup/teardown. Their temp checkpoints live inside the enclosing git work tree, so the CI planner's changed paths (which include computer control surface paths on branches that touch them) previously leaked into the computed change set and falsely triggered the mandatory computer red-team suite (`COMPUTER_REDTEAM_CASE_MISSING: … must include kill-switch-bypass`). The production kill-switch-bypass gate is unchanged; only the test fixtures now isolate their own contract from the host branch's diff (#3533). +- Cross-process file locks now record the host that owns them, so a state directory on a shared volume (NFS home directories are explicitly supported) no longer lets one host reap another host's freshly-taken lock. Owner liveness is probed against the *local* process table, and a remote owner's pid is almost never a live local pid, so a foreign owner previously read as `dead` and was reclaimed immediately with no stale grace period — two hosts could then hold the same lock and run the same critical section concurrently. A foreign-host owner is now reclaimable only after the `staleMs` elapsed-time heuristic, and host identity is part of owner equality for the guarded lock-dir removal so one host cannot delete another's lock on a pid/timestamp coincidence. Records written before this change carry no host id and keep the previous local-pid semantics. ### Fixed diff --git a/packages/coding-agent/src/config/file-lock.ts b/packages/coding-agent/src/config/file-lock.ts index 9ab6a0ddd2..22e0354089 100644 --- a/packages/coding-agent/src/config/file-lock.ts +++ b/packages/coding-agent/src/config/file-lock.ts @@ -1,5 +1,6 @@ import type { Stats } from "node:fs"; import * as fs from "node:fs/promises"; +import * as os from "node:os"; import * as path from "node:path"; import { hasFsCode, isEnoent } from "@gajae-code/utils/fs-error"; @@ -40,6 +41,29 @@ function currentProcessStartTime(): string { return ownProcessStartTime; } +let ownHostId: string | undefined; + +/** + * Stable identity of the host that owns a lock record. + * + * Liveness below is probed against the *local* process table, which is only + * meaningful for a lock written by this host. A state directory on a shared + * volume (NFS home directories are explicitly supported) is contended by + * several hosts, and a remote owner's pid is almost never a live local pid — so + * without this field a foreign host's freshly-taken lock reads as `dead` and is + * reclaimed instantly, with no stale grace period at all. + */ +function currentHostId(): string { + if (ownHostId === undefined) { + try { + ownHostId = os.hostname() || "unknown"; + } catch { + ownHostId = "unknown"; + } + } + return ownHostId; +} + function cachedProcessStartTime(owner: FileLockOwnerToken, cache?: Map): string | null { if (!cache) return processStartTime(owner.pid); const key = `${owner.pid}:${owner.start_time ?? ""}`; @@ -50,7 +74,15 @@ function cachedProcessStartTime(owner: FileLockOwnerToken, cache?: Map): boolean { + // A remote owner's pid indexes a different process table; never claim to know + // it is alive, and never let the caller conclude it is dead either (see + // `staleLockSnapshot`, which routes foreign owners to elapsed time only). + if (ownerIsForeignHost(owner)) return false; if (ownerLiveness(owner.pid) !== "alive") return false; if (!owner.start_time) return true; const currentStartTime = cachedProcessStartTime(owner, startTimeCache); @@ -58,7 +90,12 @@ function ownerIsAlive(owner: FileLockOwnerToken, startTimeCache?: Map { - const info: LockInfo = { pid: process.pid, start_time: currentProcessStartTime(), timestamp: Date.now() }; + const info: LockInfo = { + pid: process.pid, + start_time: currentProcessStartTime(), + host_id: currentHostId(), + timestamp: Date.now(), + }; return Bun.write(`${lockPath}/info`, JSON.stringify(info)).then(() => info); } @@ -72,17 +109,18 @@ async function readLockInfo(lockPath: string): Promise { } if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) return null; - const { pid, start_time, timestamp } = parsed as Partial; + const { pid, start_time, host_id, timestamp } = parsed as Partial; if ( typeof pid !== "number" || !Number.isInteger(pid) || pid <= 0 || typeof timestamp !== "number" || !Number.isFinite(timestamp) || - (start_time !== undefined && (typeof start_time !== "string" || !start_time)) + (start_time !== undefined && (typeof start_time !== "string" || !start_time)) || + (host_id !== undefined && (typeof host_id !== "string" || !host_id)) ) return null; - return { pid, start_time, timestamp }; + return { pid, start_time, host_id, timestamp }; } /** @internal */ @@ -94,6 +132,11 @@ export async function readFileLockInfoForGc(lockDir: string): Promise staleMs ? { stale: true, owner: info } : { stale: false }; + } if (ownerLiveness(info.pid) === "dead" || Date.now() - info.timestamp > staleMs) { return { stale: true, owner: info }; } diff --git a/packages/coding-agent/test/file-lock-cross-host.test.ts b/packages/coding-agent/test/file-lock-cross-host.test.ts new file mode 100644 index 0000000000..627f7c3136 --- /dev/null +++ b/packages/coding-agent/test/file-lock-cross-host.test.ts @@ -0,0 +1,161 @@ +import { describe, expect, test } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { readFileLockInfoForGc, removeFileLockDirForGc, withFileLock } from "../src/config/file-lock"; + +/** + * A lock owner's liveness is probed against the *local* process table. That is + * only meaningful for a lock this host wrote. When the state directory lives on + * a shared volume (NFS home directories are supported), a remote owner's pid is + * almost never a live local pid, so without host identity a foreign host's + * freshly-taken lock is judged dead and reclaimed instantly — with none of the + * stale grace period a local owner of unknown liveness receives. + */ +describe("file lock cross-host owner identity", () => { + const FOREIGN_HOST = "gjc-foreign-host-fixture"; + + async function stageForeignLock( + dir: string, + file: string, + overrides: Record = {}, + ): Promise { + const lockDir = `${path.join(dir, file)}.lock`; + await fs.mkdir(lockDir, { recursive: true }); + await Bun.write( + path.join(lockDir, "info"), + JSON.stringify({ + // A pid that is not alive on this host, which is the normal case for a + // process owned by a different machine. + pid: 999_999, + start_time: "Mon Jan 1 00:00:00 2029", + host_id: FOREIGN_HOST, + timestamp: Date.now(), + ...overrides, + }), + ); + return lockDir; + } + + test("refuses to steal another host's freshly-taken lock", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "gjc-lock-xhost-")); + try { + await stageForeignLock(dir, "state.json"); + let ran = false; + await expect( + withFileLock( + path.join(dir, "state.json"), + async () => { + ran = true; + }, + { retries: 2, retryDelayMs: 5, staleMs: 60_000 }, + ), + ).rejects.toThrow(/Failed to acquire lock/); + expect(ran).toBe(false); + // The foreign owner's record must survive untouched. + const info = await readFileLockInfoForGc(`${path.join(dir, "state.json")}.lock`); + expect(info?.host_id).toBe(FOREIGN_HOST); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); + + test("still reclaims another host's lock once it exceeds the stale window", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "gjc-lock-xhost-")); + try { + await stageForeignLock(dir, "state.json", { timestamp: Date.now() - 120_000 }); + let ran = false; + await withFileLock( + path.join(dir, "state.json"), + async () => { + ran = true; + }, + { retries: 3, retryDelayMs: 5, staleMs: 10_000 }, + ); + expect(ran).toBe(true); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); + + test("a dead local owner is still reclaimed immediately", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "gjc-lock-xhost-")); + try { + // No host_id at all: a pre-host-id record keeps the previous local-pid + // semantics, so a dead pid is reclaimed without waiting out staleMs. + const lockDir = `${path.join(dir, "state.json")}.lock`; + await fs.mkdir(lockDir, { recursive: true }); + await Bun.write(path.join(lockDir, "info"), JSON.stringify({ pid: 999_999, timestamp: Date.now() })); + let ran = false; + await withFileLock( + path.join(dir, "state.json"), + async () => { + ran = true; + }, + { retries: 3, retryDelayMs: 5, staleMs: 60_000 }, + ); + expect(ran).toBe(true); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); + + test("a live local owner is never reclaimed by elapsed time", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "gjc-lock-xhost-")); + try { + const lockDir = `${path.join(dir, "state.json")}.lock`; + await fs.mkdir(lockDir, { recursive: true }); + await Bun.write( + path.join(lockDir, "info"), + JSON.stringify({ pid: process.pid, timestamp: Date.now() - 600_000 }), + ); + await expect( + withFileLock(path.join(dir, "state.json"), async () => undefined, { + retries: 2, + retryDelayMs: 5, + staleMs: 1, + }), + ).rejects.toThrow(/Failed to acquire lock/); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); + + test("guarded removal refuses a token whose host differs", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "gjc-lock-xhost-")); + try { + const lockDir = await stageForeignLock(dir, "state.json"); + const onDisk = await readFileLockInfoForGc(lockDir); + expect(onDisk).not.toBeNull(); + // Same pid, start_time and timestamp, different host: two machines sharing + // a volume can coincide on all of those, so host identity must gate the + // delete rather than being ignored. + expect( + await removeFileLockDirForGc(lockDir, { + pid: onDisk!.pid, + start_time: onDisk!.start_time, + host_id: "some-other-host", + timestamp: onDisk!.timestamp, + }), + ).toBe("owner_changed"); + expect(await readFileLockInfoForGc(lockDir)).not.toBeNull(); + // The exact owner token still removes it. + expect(await removeFileLockDirForGc(lockDir, onDisk!)).toBe("removed"); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); + + test("a lock this process takes records its own host identity", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "gjc-lock-xhost-")); + try { + let observed: string | undefined; + await withFileLock(path.join(dir, "state.json"), async () => { + observed = (await readFileLockInfoForGc(`${path.join(dir, "state.json")}.lock`))?.host_id; + }); + expect(observed).toBe(os.hostname() || "unknown"); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); +});