From e3cfd1ca5f1288dde47bc0cf02ee00652da62cec Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 23 Oct 2026 17:19:28 +0000 Subject: [PATCH 1/5] tests: add the memory and foreign-holder SessionLocks layers to the contract SessionLocksMemory keeps lock state in a table per build and waits on the HostClock, so a TestClock test moves time instead of waiting. withForeignHolders plays other backends: a live holder refuses at once with today's text, and a dead one is reclaimed. The contract now runs on node, memory, node + foreign and memory + foreign. --- .../foreignHolders.testHarness.ts | 174 +++++++++++++ .../memorySessionLocks.testHarness.ts | 246 ++++++++++++++++++ .../sessionLocks.contract.test.ts | 217 ++++++++++++++- 3 files changed, 631 insertions(+), 6 deletions(-) create mode 100644 src/node/services/sessionLocks/foreignHolders.testHarness.ts create mode 100644 src/node/services/sessionLocks/memorySessionLocks.testHarness.ts diff --git a/src/node/services/sessionLocks/foreignHolders.testHarness.ts b/src/node/services/sessionLocks/foreignHolders.testHarness.ts new file mode 100644 index 00000000000..25583d7ab10 --- /dev/null +++ b/src/node/services/sessionLocks/foreignHolders.testHarness.ts @@ -0,0 +1,174 @@ +/** + * A SessionLocks layer that plays other backends: a test puts a live or dead + * foreign holder on a lock path, and the wrapped layer refuses or reclaims as + * the Node locks would, with today's text. A live holder refuses at once (the + * Node locks would wait the whole timeout first), so tests need no real wait. + */ +import assert from "node:assert"; +import { Effect, Layer } from "effect"; +import { SessionLocksTag } from "@/node/services/di/tags"; +import { + workspaceMutationInProgressMessage, + workspaceMutationLockPath, +} from "@/node/services/workspaceUseLeases"; +import { fileLockTimeoutMessage, type FileLockBlocker } from "@/node/utils/concurrency/fileLock"; +import { + CrossProcessLockTimeoutError, + WorkspaceMutationInProgressError, +} from "@/node/utils/concurrency/lockErrors"; +import type { ProcessIdentity } from "@/node/utils/concurrency/processLiveness"; +import { + crossProcessLockTimeoutMessage, + describeHolder, + type CrossProcessLockHolder, +} from "@/node/utils/main/crossProcessLock"; +import type { SessionLocks } from "./sessionLocks"; + +/** The Node locks' reason for a live holder in this PID domain (processLiveness.judgeHolder). */ +const LIVE_WHY = process.platform === "linux" ? "that process is running" : "that pid is running"; + +/** Why the Node locks refuse a live holder with this pid: the blocker of their timeout text. */ +export function liveBlocker(pid: number, identity?: ProcessIdentity): FileLockBlocker { + return { holder: { pid, identity }, why: LIVE_WHY }; +} + +/** The record a live foreign holder would have written: describeHolder reads pid and identity. */ +export function foreignRecord(pid: number, identity?: ProcessIdentity): CrossProcessLockHolder { + return { pid, token: `foreign-${pid}`, acquiredAt: 0, identity }; +} + +/** The handle that the test keeps. It holds the foreign holders, not the locks. */ +export interface ForeignHolders { + /** + * Another backend holds lockPath. live: refuse at once. + * dead: the next acquire reclaims it, and inspect says "dead" until then. + */ + hold(lockPath: string, state: "live" | "dead", holder?: { pid: number }): void; + /** The foreign holder of lockPath released it cleanly. */ + release(lockPath: string): void; + /** Paths whose dead holder an acquire reclaimed, in order. */ + readonly reclaimed: readonly string[]; +} + +interface Holder { + state: "live" | "dead"; + pid: number; +} + +/** The test's side of makeForeignHolders: wrapWithForeignHolders reads the same map. */ +const holdersOf = new WeakMap>(); + +/** A pid no test process has: the fake holder must not be this process. */ +const DEFAULT_FOREIGN_PID = 4242; + +export function makeForeignHolders(): ForeignHolders { + const table = new Map(); + const reclaimed: string[] = []; + const holders: ForeignHolders = { + hold: (lockPath, state, holder) => { + const pid = holder?.pid ?? DEFAULT_FOREIGN_PID; + assert(pid !== process.pid, "a foreign holder must not be this process"); + table.set(lockPath, { state, pid }); + }, + release: (lockPath) => { + assert(table.delete(lockPath), `no foreign holder on ${lockPath}`); + }, + reclaimed, + }; + holdersOf.set(holders, table); + return holders; +} + +/** + * The locks that check the foreign holders before each call. A dead holder is + * reclaimed by the acquire that meets it: it leaves the table, the path goes to + * `reclaimed`, and the call goes on to the inner locks. + */ +export function wrapWithForeignHolders(inner: SessionLocks, holders: ForeignHolders): SessionLocks { + const table = holdersOf.get(holders); + assert(table !== undefined, "holders must come from makeForeignHolders"); + const reclaimed = holders.reclaimed as string[]; + /** The live holder of lockPath, after it reclaims a dead one. */ + const liveHolder = (lockPath: string): Holder | undefined => { + const holder = table.get(lockPath); + if (holder?.state === "dead") { + table.delete(lockPath); + reclaimed.push(lockPath); + return undefined; + } + return holder; + }; + // One object per config, as workspaceUseLeasesFor gives. + const leasesByConfig = new WeakMap>(); + return { + workspaceFiles: inner.workspaceFiles, + acquireProcessFileLock: async (options) => { + const holder = liveHolder(options.lockPath); + if (holder !== undefined) { + throw new Error( + fileLockTimeoutMessage( + options.label, + options.lockPath, + options.timeoutMs, + liveBlocker(holder.pid) + ) + ); + } + return await inner.acquireProcessFileLock(options); + }, + acquireCrossProcessLock: async (options) => { + const holder = liveHolder(options.lockPath); + if (holder !== undefined) { + throw new CrossProcessLockTimeoutError( + crossProcessLockTimeoutMessage(options.timeoutMessage, options.lockPath, { + holder: foreignRecord(holder.pid), + why: LIVE_WHY, + }) + ); + } + return await inner.acquireCrossProcessLock(options); + }, + // A probe never reclaims: it reports what an acquire would do. + inspectCrossProcessLock: async (lockPath) => { + const holder = table.get(lockPath); + if (holder === undefined) return await inner.inspectCrossProcessLock(lockPath); + if (holder.state === "dead") return { state: "dead" }; + return { state: "held", holder: describeHolder(foreignRecord(holder.pid)), why: LIVE_WHY }; + }, + useLeasesFor: (config) => { + const cached = leasesByConfig.get(config); + if (cached !== undefined) return cached; + const leases = inner.useLeasesFor(config); + const wrapped: ReturnType = { + // A live foreign mutator holds the gate. The inner hold reads the gate on + // disk, where a fake holder leaves no file, so the check comes first. + hold: async (workspaceId, kind) => { + const gate = table.get(workspaceMutationLockPath(config.rootDir, workspaceId)); + if (gate?.state === "live") { + throw new WorkspaceMutationInProgressError( + workspaceMutationInProgressMessage(workspaceId, { + holder: describeHolder(foreignRecord(gate.pid)), + why: LIVE_WHY, + }) + ); + } + return await leases.hold(workspaceId, kind); + }, + isHeld: (workspaceId, kind) => leases.isHeld(workspaceId, kind), + }; + leasesByConfig.set(config, wrapped); + return wrapped; + }, + }; +} + +/** A layer that builds inner, then checks the foreign holders before each call. */ +export function withForeignHolders( + inner: Layer.Layer, + holders: ForeignHolders +): Layer.Layer { + return Layer.effect( + SessionLocksTag, + Effect.map(SessionLocksTag, (locks) => wrapWithForeignHolders(locks, holders)) + ).pipe(Layer.provide(inner)); +} diff --git a/src/node/services/sessionLocks/memorySessionLocks.testHarness.ts b/src/node/services/sessionLocks/memorySessionLocks.testHarness.ts new file mode 100644 index 00000000000..e05228ba517 --- /dev/null +++ b/src/node/services/sessionLocks/memorySessionLocks.testHarness.ts @@ -0,0 +1,246 @@ +/** + * SessionLocks in memory: the lock files and records live in a table, and a + * waiter waits on the HostClock, so under a TestClock a test moves time + * instead of waiting. Refusals use today's text (fileLockTimeoutMessage, + * crossProcessLockTimeoutMessage). Two parts stay on the Node locks: + * - Init-record locks: InitStateManager lists their folder (initStateManager.ts + * isRecordLockHeld), so a holder in memory would leave no file, and a live + * init would look abandoned. + * - The use leases: the mutation gate, isHeld and taskService read other + * backends' lease files. Phase 5 moves them. + */ +import assert from "node:assert"; +import * as path from "node:path"; +import { Effect, Layer } from "effect"; +import { SessionLocksTag } from "@/node/services/di/tags"; +import { HostClockTag } from "@/node/services/di/hostClockLive"; +import type { HostClock } from "@/node/utils/hostClock"; +import type { ProcessFileLock } from "@/node/utils/concurrency/fileLock"; +import { fileLockTimeoutMessage } from "@/node/utils/concurrency/fileLock"; +import { CrossProcessLockTimeoutError } from "@/node/utils/concurrency/lockErrors"; +import { getSelfIdentity, type ProcessIdentity } from "@/node/utils/concurrency/processLiveness"; +import { + crossProcessLockTimeoutMessage, + describeHolder, + type CrossProcessLockState, +} from "@/node/utils/main/crossProcessLock"; +import { nodeSessionLocks } from "./nodeSessionLocks"; +import type { SessionLocks } from "./sessionLocks"; +import { foreignRecord, liveBlocker } from "./foreignHolders.testHarness"; + +/** Who holds a lock in the table. */ +interface Entry { + pid: number; + identity?: ProcessIdentity; + /** A foreign holder whose process is gone: the next acquire reclaims it. */ + dead: boolean; +} + +/** Why an acquire did not get the lock: the blocker of the timeout text. */ +interface Blocker { + pid: number; + identity?: ProcessIdentity; + why: string; +} + +interface Waiter { + take(entry: Entry): void; +} + +/** The lock state of one build of the memory layer. */ +export class MemoryLockTable { + private readonly held = new Map(); + /** FIFO per lock path: a release hands the lock to the first waiter. */ + private readonly waiters = new Map(); + + /** Another backend holds lockPath (pid 4242 unless given). */ + hold(lockPath: string, state: "live" | "dead", holder?: { pid: number }): void { + assert(!this.held.has(lockPath), `${lockPath} is already held`); + const pid = holder?.pid ?? 4242; + assert(pid !== process.pid, "a foreign holder must not be this process"); + this.held.set(lockPath, { pid, dead: state === "dead" }); + } + + /** The foreign holder of lockPath released it cleanly. */ + release(lockPath: string): void { + const entry = this.held.get(lockPath); + assert(entry !== undefined && entry.pid !== process.pid, `no foreign holder on ${lockPath}`); + this.free(lockPath, entry); + } + + owner(lockPath: string): Entry | undefined { + return this.held.get(lockPath); + } + + /** Takes a free or dead lock now. Else null, and the caller waits or refuses. */ + tryTake(lockPath: string): Entry | null { + const entry = this.held.get(lockPath); + if (entry !== undefined && !entry.dead) return null; + const own: Entry = { pid: process.pid, identity: getSelfIdentity(), dead: false }; + this.held.set(lockPath, own); + return own; + } + + /** Queues a waiter. The returned function takes it out again (timeout, abort). */ + wait(lockPath: string, waiter: Waiter): () => void { + const queue = this.waiters.get(lockPath) ?? []; + queue.push(waiter); + this.waiters.set(lockPath, queue); + return () => { + const index = queue.indexOf(waiter); + if (index >= 0) queue.splice(index, 1); + if (queue.length === 0 && this.waiters.get(lockPath) === queue) this.waiters.delete(lockPath); + }; + } + + /** Frees lockPath if entry still holds it, and hands it to the first waiter. */ + free(lockPath: string, entry: Entry): void { + if (this.held.get(lockPath) !== entry) return; + this.held.delete(lockPath); + const queue = this.waiters.get(lockPath); + const next = queue?.shift(); + if (queue?.length === 0) this.waiters.delete(lockPath); + if (next === undefined) return; + const own = this.tryTake(lockPath); + assert(own !== null, "a freed lock is free"); + next.take(own); + } +} + +/** The Node locks' reason for a holder (processLiveness.judgeHolder). */ +function blockerOf(entry: Entry): Blocker { + if (entry.pid === process.pid) { + return { pid: entry.pid, identity: entry.identity, why: "it is held by this process" }; + } + const { holder, why } = liveBlocker(entry.pid, entry.identity); + assert(holder !== undefined); + return { pid: holder.pid, identity: holder.identity, why }; +} + +/** + * Takes lockPath, or waits until a release hands it over, or refuses with + * refusal(blocker) once timeoutMs passes on the clock (at once for 0). + */ +function acquireInMemory( + table: MemoryLockTable, + lockPath: string, + timeoutMs: number, + clock: HostClock, + refusal: (blocker: Blocker) => Error, + signal?: AbortSignal +): Promise { + const aborted = () => new Error("Lock acquisition was aborted"); + if (signal?.aborted) return Promise.reject(aborted()); + const taken = table.tryTake(lockPath); + if (taken !== null) return Promise.resolve(taken); + const holder = table.owner(lockPath); + assert(holder !== undefined, "a lock that cannot be taken has a holder"); + if (timeoutMs <= 0) return Promise.reject(refusal(blockerOf(holder))); + return new Promise((resolve, reject) => { + const settle = () => { + leave(); + cancelTimer(); + signal?.removeEventListener("abort", onAbort); + }; + const onAbort = () => { + settle(); + reject(aborted()); + }; + const leave = table.wait(lockPath, { + take: (entry) => { + settle(); + resolve(entry); + }, + }); + const cancelTimer = clock.setTimer(() => { + settle(); + const current = table.owner(lockPath); + assert(current !== undefined, "a waiter waits only on a held lock"); + reject(refusal(blockerOf(current))); + }, timeoutMs); + signal?.addEventListener("abort", onAbort, { once: true }); + }); +} + +function inspectInMemory(table: MemoryLockTable, lockPath: string): CrossProcessLockState { + const entry = table.owner(lockPath); + if (entry === undefined) return { state: "absent" }; + if (entry.dead) return { state: "dead" }; + const { pid, identity, why } = blockerOf(entry); + return { state: "held", holder: describeHolder(foreignRecord(pid, identity)), why }; +} + +/** An init-record lock: InitStateManager finds these by listing their folder. */ +export function foundByReaddir(lockPath: string): boolean { + return lockPath.split(path.sep).join("/").includes("/locks/init-record/"); +} + +/** The memory locks over one table. SessionLocksMemory gives each build its own. */ +export function memorySessionLocks(clock: HostClock, table: MemoryLockTable): SessionLocks { + return { + // Pure memory already: every session-file service shares this one FIFO. + workspaceFiles: nodeSessionLocks.workspaceFiles, + acquireProcessFileLock: async (options): Promise => { + const { lockPath, label, timeoutMs } = options; + // The same argument checks as fileLock.ts acquireProcessFileLock. + assert(lockPath.length > 0, "acquireProcessFileLock requires a lock path"); + assert(timeoutMs > 0, "acquireProcessFileLock timeoutMs must be positive"); + const entry = await acquireInMemory(table, lockPath, timeoutMs, clock, (blocker) => { + const { why, ...holder } = blocker; + return new Error(fileLockTimeoutMessage(label, lockPath, timeoutMs, { holder, why })); + }); + return { + assertStillOwned: () => { + if (table.owner(lockPath) === entry) return Promise.resolve(); + // fileLock.ts assertStillOwned + return Promise.reject( + new Error( + `${label} ${lockPath} is no longer owned by this holder (displaced or reclaimed); ` + + `aborting before mutation` + ) + ); + }, + [Symbol.asyncDispose]: () => { + table.free(lockPath, entry); + return Promise.resolve(); + }, + }; + }, + acquireCrossProcessLock: async (options) => { + if (foundByReaddir(options.lockPath)) { + return await nodeSessionLocks.acquireCrossProcessLock(options); + } + const { lockPath, acquireTimeoutMs, timeoutMessage, signal } = options; + const refusal = (blocker: Blocker) => + new CrossProcessLockTimeoutError( + crossProcessLockTimeoutMessage(timeoutMessage, lockPath, { + holder: foreignRecord(blocker.pid, blocker.identity), + why: blocker.why, + }) + ); + const entry = await acquireInMemory( + table, + lockPath, + acquireTimeoutMs, + clock, + refusal, + signal + ); + return () => { + table.free(lockPath, entry); + return Promise.resolve(); + }; + }, + inspectCrossProcessLock: async (lockPath) => + foundByReaddir(lockPath) + ? await nodeSessionLocks.inspectCrossProcessLock(lockPath) + : inspectInMemory(table, lockPath), + useLeasesFor: (config) => nodeSessionLocks.useLeasesFor(config), + }; +} + +/** Each build gets fresh lock tables: no lock state outlives a test's layer. */ +export const SessionLocksMemory: Layer.Layer = Layer.effect( + SessionLocksTag, + Effect.map(HostClockTag, (clock) => memorySessionLocks(clock, new MemoryLockTable())) +); diff --git a/src/node/services/sessionLocks/sessionLocks.contract.test.ts b/src/node/services/sessionLocks/sessionLocks.contract.test.ts index f9143a4680c..2789da584e7 100644 --- a/src/node/services/sessionLocks/sessionLocks.contract.test.ts +++ b/src/node/services/sessionLocks/sessionLocks.contract.test.ts @@ -2,8 +2,29 @@ import { afterEach, 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 { Layer } from "effect"; +import { SessionLocksTag } from "@/node/services/di/tags"; +import { HostClockTag } from "@/node/services/di/hostClockLive"; +import { makeTestEffectRunner } from "@/node/services/di/testEffectRunner"; +import { + WorkspaceMutationInProgressError, + workspaceMutationInProgressMessage, + workspaceMutationLockPath, +} from "@/node/services/workspaceUseLeases"; import { fileLockTimeoutMessage } from "@/node/utils/concurrency/fileLock"; import type { ProcessIdentity } from "@/node/utils/concurrency/processLiveness"; +import { describeHolder } from "@/node/utils/main/crossProcessLock"; +import { + foreignRecord, + liveBlocker, + makeForeignHolders, + withForeignHolders, +} from "./foreignHolders.testHarness"; +import { + MemoryLockTable, + memorySessionLocks, + SessionLocksMemory, +} from "./memorySessionLocks.testHarness"; import { SessionLocksNode } from "./nodeSessionLocks"; import { buildSessionLocks, @@ -11,6 +32,7 @@ import { type ForeignHolder, type LocksUnderTest, } from "./sessionLocks.contract.testHarness"; +import type { SessionLocks } from "./sessionLocks"; const CHILD = path.join(import.meta.dir, "sessionLocks.testChild.ts"); const children: Array> = []; @@ -75,7 +97,54 @@ async function nodeLocks(): Promise { }; } +/** The memory layer over a table the test can reach, on a fresh TestClock. */ +async function memoryLocks(): Promise { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "xum-session-locks-")); + const clock = makeTestEffectRunner(); + const table = new MemoryLockTable(); + return { + layer: Layer.succeed(SessionLocksTag)(memorySessionLocks(clock.hostClock, table)), + dir, + foreignHolder: (lockPath, state) => { + table.hold(lockPath, state, { pid: 4242 }); + return Promise.resolve({ + pid: 4242, + identity: null, + release: () => Promise.resolve(table.release(lockPath)), + }); + }, + advance: (ms) => clock.adjust(ms), + cleanup: async () => { + await clock.dispose(); + await fs.rm(dir, { recursive: true, force: true }); + }, + }; +} + +/** Wraps a layer under test: its foreign holders come from makeForeignHolders. */ +function withForeign(make: () => Promise): () => Promise { + return async () => { + const t = await make(); + const holders = makeForeignHolders(); + return { + ...t, + layer: withForeignHolders(t.layer, holders), + foreignHolder: (lockPath, state) => { + holders.hold(lockPath, state, { pid: 4343 }); + return Promise.resolve({ + pid: 4343, + identity: null, + release: () => Promise.resolve(holders.release(lockPath)), + }); + }, + }; + }; +} + describeSessionLocksContract("node", nodeLocks); +describeSessionLocksContract("memory", memoryLocks); +describeSessionLocksContract("node + foreign", withForeign(nodeLocks)); +describeSessionLocksContract("memory + foreign", withForeign(memoryLocks)); test("node: the timeout text for a live child holder is fileLockTimeoutMessage", async () => { const t = await nodeLocks(); @@ -89,16 +158,152 @@ test("node: the timeout text for a live child holder is fileLockTimeoutMessage", () => null, (caught: unknown) => caught ); - // The fake layers build their refusal with the same function, so both texts match. - const why = process.platform === "linux" ? "that process is running" : "that pid is running"; + // The fake layers refuse with liveBlocker, so the fake text equals the Node text. expect((error as Error).message).toBe( - fileLockTimeoutMessage("contract lock", lockPath, 200, { - holder: { pid: holder.pid, identity: holder.identity ?? undefined }, - why, - }) + fileLockTimeoutMessage( + "contract lock", + lockPath, + 200, + liveBlocker(holder.pid, holder.identity ?? undefined) + ) ); await holder.release(); } finally { await t.cleanup(); } }); + +/** Runs fn on the memory layer of a fresh memoryLocks(), then cleans up. */ +async function onMemory( + fn: (locks: SessionLocks, t: LocksUnderTest) => Promise +): Promise { + const t = await memoryLocks(); + try { + await fn(await buildSessionLocks(t.layer), t); + } finally { + await t.cleanup(); + } +} + +test("memory: a 60 s timeout passes on TestClock.adjust, with no real wait", () => + onMemory(async (locks, t) => { + const lockPath = path.join(t.dir, "long.lock"); + await t.foreignHolder(lockPath, "live", "file"); + const refused = locks + .acquireProcessFileLock({ lockPath, timeoutMs: 60_000, label: "contract lock" }) + .then( + () => null, + (error: unknown) => error + ); + await t.advance(59_999); + let settled = false; + void refused.then(() => (settled = true)); + await Promise.resolve(); + expect(settled).toBe(false); + await t.advance(1); + expect(((await refused) as Error).message).toBe( + fileLockTimeoutMessage("contract lock", lockPath, 60_000, liveBlocker(4242)) + ); + })); + +test("memory: a release hands the file lock to the waiters in FIFO order", () => + onMemory(async (locks, t) => { + const options = { lockPath: path.join(t.dir, "fifo.lock"), timeoutMs: 1_000, label: "l" }; + const first = await locks.acquireProcessFileLock(options); + const order: string[] = []; + const waiter = (name: string) => + locks.acquireProcessFileLock(options).then(async (lock) => { + order.push(name); + await lock[Symbol.asyncDispose](); + }); + const both = Promise.all([waiter("b"), waiter("c")]); + await first[Symbol.asyncDispose](); + await both; + expect(order).toEqual(["b", "c"]); + await expect(first.assertStillOwned()).rejects.toThrow("is no longer owned by this holder"); + })); + +test("memory: an init-record lock makes a real file, and inspect sees it", () => + onMemory(async (locks, t) => { + const lockPath = path.join(t.dir, "locks", "init-record", "ws", "backend.lock"); + const release = await locks.acquireCrossProcessLock({ + lockPath, + acquireTimeoutMs: 0, + staleMs: 60_000, + timeoutMessage: "init record is held.", + }); + expect((await fs.stat(lockPath)).isFile()).toBe(true); + expect(await locks.inspectCrossProcessLock(lockPath)).toMatchObject({ state: "held" }); + await release(); + expect(await locks.inspectCrossProcessLock(lockPath)).toEqual({ state: "absent" }); + })); + +test("memory: each build of SessionLocksMemory has its own lock table", async () => { + const clock = makeTestEffectRunner(); + try { + const layer = SessionLocksMemory.pipe( + Layer.provide(Layer.succeed(HostClockTag)(clock.hostClock)) + ); + const options = { lockPath: "/memory/one.lock", timeoutMs: 200, label: "l" }; + const a = await (await buildSessionLocks(layer)).acquireProcessFileLock(options); + const b = await (await buildSessionLocks(layer)).acquireProcessFileLock(options); + await a[Symbol.asyncDispose](); + await b[Symbol.asyncDispose](); + } finally { + await clock.dispose(); + } +}); + +test("foreign: dead holders are reclaimed in order, and a probe reclaims none", async () => { + const t = await nodeLocks(); + try { + const holders = makeForeignHolders(); + const locks = await buildSessionLocks(withForeignHolders(t.layer, holders)); + const file = path.join(t.dir, "a.lock"); + const record = path.join(t.dir, "b.lock"); + holders.hold(record, "dead"); + holders.hold(file, "dead"); + expect(await locks.inspectCrossProcessLock(record)).toEqual({ state: "dead" }); + const lock = await locks.acquireProcessFileLock({ lockPath: file, timeoutMs: 200, label: "l" }); + const release = await locks.acquireCrossProcessLock({ + lockPath: record, + acquireTimeoutMs: 0, + staleMs: 60_000, + timeoutMessage: "held.", + }); + expect(holders.reclaimed).toEqual([file, record]); + await lock[Symbol.asyncDispose](); + await release(); + } finally { + await t.cleanup(); + } +}); + +test("foreign: a live foreign mutator makes hold() refuse with today's text", async () => { + const t = await nodeLocks(); + try { + const holders = makeForeignHolders(); + const locks = await buildSessionLocks(withForeignHolders(t.layer, holders)); + const config = { rootDir: t.dir }; + const leases = locks.useLeasesFor(config); + expect(locks.useLeasesFor(config)).toBe(leases); + holders.hold(workspaceMutationLockPath(t.dir, "ws-gate"), "live", { pid: 4444 }); + const error = await leases.hold("ws-gate", "turn").then( + () => null, + (caught: unknown) => caught + ); + expect(error).toBeInstanceOf(WorkspaceMutationInProgressError); + expect((error as Error).message).toBe( + workspaceMutationInProgressMessage("ws-gate", { + holder: describeHolder(foreignRecord(4444)), + why: liveBlocker(4444).why, + }) + ); + expect(await leases.isHeld("ws-gate", "turn")).toBe(false); + holders.release(workspaceMutationLockPath(t.dir, "ws-gate")); + const lease = await leases.hold("ws-gate", "turn"); + await lease.release(); + } finally { + await t.cleanup(); + } +}); From 30fe6b1db7d170c2c5ea6951f5d56bc7a3a4855f Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 23 Oct 2026 18:24:47 +0000 Subject: [PATCH 2/5] tests: match the Node locks in the fake layers on abort, missing parents, gate order and holder text Codex round 1 on #7367: an aborted signal wins over a fake live holder, the memory layer rejects with ENOENT when createParentDirectory is false and the parent is gone, the fake gate is probed after the lease is published, fake holders record an identity so the text equals the Node text for a current record, and fake pids are offsets from process.pid. --- .../foreignHolders.testHarness.ts | 53 ++++++++++++++---- .../memorySessionLocks.testHarness.ts | 23 +++++--- .../sessionLocks.contract.test.ts | 55 +++++++++++++++---- .../sessionLocks.contract.testHarness.ts | 38 +++++++++++++ 4 files changed, 141 insertions(+), 28 deletions(-) diff --git a/src/node/services/sessionLocks/foreignHolders.testHarness.ts b/src/node/services/sessionLocks/foreignHolders.testHarness.ts index 25583d7ab10..e00729bd1e3 100644 --- a/src/node/services/sessionLocks/foreignHolders.testHarness.ts +++ b/src/node/services/sessionLocks/foreignHolders.testHarness.ts @@ -16,7 +16,7 @@ import { CrossProcessLockTimeoutError, WorkspaceMutationInProgressError, } from "@/node/utils/concurrency/lockErrors"; -import type { ProcessIdentity } from "@/node/utils/concurrency/processLiveness"; +import { getSelfIdentity, type ProcessIdentity } from "@/node/utils/concurrency/processLiveness"; import { crossProcessLockTimeoutMessage, describeHolder, @@ -24,19 +24,49 @@ import { } from "@/node/utils/main/crossProcessLock"; import type { SessionLocks } from "./sessionLocks"; -/** The Node locks' reason for a live holder in this PID domain (processLiveness.judgeHolder). */ +/** + * The Node locks' reason for a live holder with a current record in this PID + * domain (processLiveness.judgeHolder). A record without an identity takes the + * legacy branch, which words it otherwise, so every fake holder has one. + */ const LIVE_WHY = process.platform === "linux" ? "that process is running" : "that pid is running"; +/** + * The identity a fake foreign holder records: another backend on this host, so + * the same platform, boot and PID namespace. It borrows this process's start + * time, which only describeHolder prints. + */ +export function foreignIdentity(): ProcessIdentity { + return getSelfIdentity(); +} + +/** A fake holder pid: n > 0 apart from this process, so it is never this process. */ +export function foreignPid(n = 1): number { + assert(Number.isInteger(n) && n > 0, "foreignPid needs a positive integer"); + return process.pid + n; +} + /** Why the Node locks refuse a live holder with this pid: the blocker of their timeout text. */ -export function liveBlocker(pid: number, identity?: ProcessIdentity): FileLockBlocker { +export function liveBlocker( + pid: number, + identity: ProcessIdentity = foreignIdentity() +): FileLockBlocker { return { holder: { pid, identity }, why: LIVE_WHY }; } /** The record a live foreign holder would have written: describeHolder reads pid and identity. */ -export function foreignRecord(pid: number, identity?: ProcessIdentity): CrossProcessLockHolder { +export function foreignRecord( + pid: number, + identity: ProcessIdentity = foreignIdentity() +): CrossProcessLockHolder { return { pid, token: `foreign-${pid}`, acquiredAt: 0, identity }; } +/** The error of a Node acquire whose signal is aborted: it checks the signal before any attempt. */ +export function abortedAcquireError(): Error { + return new Error("Lock acquisition was aborted"); +} + /** The handle that the test keeps. It holds the foreign holders, not the locks. */ export interface ForeignHolders { /** @@ -58,15 +88,12 @@ interface Holder { /** The test's side of makeForeignHolders: wrapWithForeignHolders reads the same map. */ const holdersOf = new WeakMap>(); -/** A pid no test process has: the fake holder must not be this process. */ -const DEFAULT_FOREIGN_PID = 4242; - export function makeForeignHolders(): ForeignHolders { const table = new Map(); const reclaimed: string[] = []; const holders: ForeignHolders = { hold: (lockPath, state, holder) => { - const pid = holder?.pid ?? DEFAULT_FOREIGN_PID; + const pid = holder?.pid ?? foreignPid(); assert(pid !== process.pid, "a foreign holder must not be this process"); table.set(lockPath, { state, pid }); }, @@ -117,6 +144,8 @@ export function wrapWithForeignHolders(inner: SessionLocks, holders: ForeignHold return await inner.acquireProcessFileLock(options); }, acquireCrossProcessLock: async (options) => { + // crossProcessLock.ts checks the signal before each attempt: abort wins over a holder. + if (options.signal?.aborted) throw abortedAcquireError(); const holder = liveHolder(options.lockPath); if (holder !== undefined) { throw new CrossProcessLockTimeoutError( @@ -141,10 +170,14 @@ export function wrapWithForeignHolders(inner: SessionLocks, holders: ForeignHold const leases = inner.useLeasesFor(config); const wrapped: ReturnType = { // A live foreign mutator holds the gate. The inner hold reads the gate on - // disk, where a fake holder leaves no file, so the check comes first. + // disk, where a fake holder leaves no file. Like WorkspaceUseLeases.hold, the + // lease is published first and the gate probed second, so a mutator that + // takes the gate meanwhile is not missed. hold: async (workspaceId, kind) => { + const lease = await leases.hold(workspaceId, kind); const gate = table.get(workspaceMutationLockPath(config.rootDir, workspaceId)); if (gate?.state === "live") { + await lease.release(); throw new WorkspaceMutationInProgressError( workspaceMutationInProgressMessage(workspaceId, { holder: describeHolder(foreignRecord(gate.pid)), @@ -152,7 +185,7 @@ export function wrapWithForeignHolders(inner: SessionLocks, holders: ForeignHold }) ); } - return await leases.hold(workspaceId, kind); + return lease; }, isHeld: (workspaceId, kind) => leases.isHeld(workspaceId, kind), }; diff --git a/src/node/services/sessionLocks/memorySessionLocks.testHarness.ts b/src/node/services/sessionLocks/memorySessionLocks.testHarness.ts index e05228ba517..2ba03b0fb63 100644 --- a/src/node/services/sessionLocks/memorySessionLocks.testHarness.ts +++ b/src/node/services/sessionLocks/memorySessionLocks.testHarness.ts @@ -10,6 +10,7 @@ * backends' lease files. Phase 5 moves them. */ import assert from "node:assert"; +import * as fs from "node:fs/promises"; import * as path from "node:path"; import { Effect, Layer } from "effect"; import { SessionLocksTag } from "@/node/services/di/tags"; @@ -26,7 +27,13 @@ import { } from "@/node/utils/main/crossProcessLock"; import { nodeSessionLocks } from "./nodeSessionLocks"; import type { SessionLocks } from "./sessionLocks"; -import { foreignRecord, liveBlocker } from "./foreignHolders.testHarness"; +import { + abortedAcquireError, + foreignIdentity, + foreignPid, + foreignRecord, + liveBlocker, +} from "./foreignHolders.testHarness"; /** Who holds a lock in the table. */ interface Entry { @@ -53,12 +60,12 @@ export class MemoryLockTable { /** FIFO per lock path: a release hands the lock to the first waiter. */ private readonly waiters = new Map(); - /** Another backend holds lockPath (pid 4242 unless given). */ + /** Another backend holds lockPath (pid foreignPid() unless given). */ hold(lockPath: string, state: "live" | "dead", holder?: { pid: number }): void { assert(!this.held.has(lockPath), `${lockPath} is already held`); - const pid = holder?.pid ?? 4242; + const pid = holder?.pid ?? foreignPid(); assert(pid !== process.pid, "a foreign holder must not be this process"); - this.held.set(lockPath, { pid, dead: state === "dead" }); + this.held.set(lockPath, { pid, identity: foreignIdentity(), dead: state === "dead" }); } /** The foreign holder of lockPath released it cleanly. */ @@ -129,8 +136,7 @@ function acquireInMemory( refusal: (blocker: Blocker) => Error, signal?: AbortSignal ): Promise { - const aborted = () => new Error("Lock acquisition was aborted"); - if (signal?.aborted) return Promise.reject(aborted()); + if (signal?.aborted) return Promise.reject(abortedAcquireError()); const taken = table.tryTake(lockPath); if (taken !== null) return Promise.resolve(taken); const holder = table.owner(lockPath); @@ -144,7 +150,7 @@ function acquireInMemory( }; const onAbort = () => { settle(); - reject(aborted()); + reject(abortedAcquireError()); }; const leave = table.wait(lockPath, { take: (entry) => { @@ -211,6 +217,9 @@ export function memorySessionLocks(clock: HostClock, table: MemoryLockTable): Se return await nodeSessionLocks.acquireCrossProcessLock(options); } const { lockPath, acquireTimeoutMs, timeoutMessage, signal } = options; + // Node rejects with ENOENT when the parent is gone and may not be recreated + // (a removed workflow run directory). Callers rely on that refusal. + if (options.createParentDirectory === false) await fs.stat(path.dirname(lockPath)); const refusal = (blocker: Blocker) => new CrossProcessLockTimeoutError( crossProcessLockTimeoutMessage(timeoutMessage, lockPath, { diff --git a/src/node/services/sessionLocks/sessionLocks.contract.test.ts b/src/node/services/sessionLocks/sessionLocks.contract.test.ts index 2789da584e7..fca6993622f 100644 --- a/src/node/services/sessionLocks/sessionLocks.contract.test.ts +++ b/src/node/services/sessionLocks/sessionLocks.contract.test.ts @@ -15,17 +15,20 @@ import { fileLockTimeoutMessage } from "@/node/utils/concurrency/fileLock"; import type { ProcessIdentity } from "@/node/utils/concurrency/processLiveness"; import { describeHolder } from "@/node/utils/main/crossProcessLock"; import { + foreignIdentity, + foreignPid, foreignRecord, liveBlocker, makeForeignHolders, withForeignHolders, + wrapWithForeignHolders, } from "./foreignHolders.testHarness"; import { MemoryLockTable, memorySessionLocks, SessionLocksMemory, } from "./memorySessionLocks.testHarness"; -import { SessionLocksNode } from "./nodeSessionLocks"; +import { nodeSessionLocks, SessionLocksNode } from "./nodeSessionLocks"; import { buildSessionLocks, describeSessionLocksContract, @@ -106,10 +109,10 @@ async function memoryLocks(): Promise { layer: Layer.succeed(SessionLocksTag)(memorySessionLocks(clock.hostClock, table)), dir, foreignHolder: (lockPath, state) => { - table.hold(lockPath, state, { pid: 4242 }); + table.hold(lockPath, state, { pid: foreignPid(1) }); return Promise.resolve({ - pid: 4242, - identity: null, + pid: foreignPid(1), + identity: foreignIdentity(), release: () => Promise.resolve(table.release(lockPath)), }); }, @@ -130,10 +133,10 @@ function withForeign(make: () => Promise): () => Promise { - holders.hold(lockPath, state, { pid: 4343 }); + holders.hold(lockPath, state, { pid: foreignPid(2) }); return Promise.resolve({ - pid: 4343, - identity: null, + pid: foreignPid(2), + identity: foreignIdentity(), release: () => Promise.resolve(holders.release(lockPath)), }); }, @@ -202,7 +205,7 @@ test("memory: a 60 s timeout passes on TestClock.adjust, with no real wait", () expect(settled).toBe(false); await t.advance(1); expect(((await refused) as Error).message).toBe( - fileLockTimeoutMessage("contract lock", lockPath, 60_000, liveBlocker(4242)) + fileLockTimeoutMessage("contract lock", lockPath, 60_000, liveBlocker(foreignPid(1))) ); })); @@ -287,7 +290,7 @@ test("foreign: a live foreign mutator makes hold() refuse with today's text", as const config = { rootDir: t.dir }; const leases = locks.useLeasesFor(config); expect(locks.useLeasesFor(config)).toBe(leases); - holders.hold(workspaceMutationLockPath(t.dir, "ws-gate"), "live", { pid: 4444 }); + holders.hold(workspaceMutationLockPath(t.dir, "ws-gate"), "live", { pid: foreignPid(3) }); const error = await leases.hold("ws-gate", "turn").then( () => null, (caught: unknown) => caught @@ -295,8 +298,8 @@ test("foreign: a live foreign mutator makes hold() refuse with today's text", as expect(error).toBeInstanceOf(WorkspaceMutationInProgressError); expect((error as Error).message).toBe( workspaceMutationInProgressMessage("ws-gate", { - holder: describeHolder(foreignRecord(4444)), - why: liveBlocker(4444).why, + holder: describeHolder(foreignRecord(foreignPid(3))), + why: liveBlocker(foreignPid(3)).why, }) ); expect(await leases.isHeld("ws-gate", "turn")).toBe(false); @@ -307,3 +310,33 @@ test("foreign: a live foreign mutator makes hold() refuse with today's text", as await t.cleanup(); } }); + +test("foreign: hold() publishes its lease before it probes the gate", async () => { + const t = await nodeLocks(); + try { + const holders = makeForeignHolders(); + const gate = workspaceMutationLockPath(t.dir, "ws-order"); + // A mutator takes the gate while the inner hold publishes the lease. + const inner = { + ...nodeSessionLocks, + useLeasesFor: (config: { readonly rootDir: string }) => { + const leases = nodeSessionLocks.useLeasesFor(config); + return { + hold: async (...args: Parameters) => { + const lease = await leases.hold(...args); + holders.hold(gate, "live", { pid: foreignPid(4) }); + return lease; + }, + isHeld: (...args: Parameters) => leases.isHeld(...args), + }; + }, + }; + const leases = wrapWithForeignHolders(inner, holders).useLeasesFor({ rootDir: t.dir }); + await expect(leases.hold("ws-order", "turn")).rejects.toBeInstanceOf( + WorkspaceMutationInProgressError + ); + expect(await leases.isHeld("ws-order", "turn")).toBe(false); + } finally { + await t.cleanup(); + } +}); diff --git a/src/node/services/sessionLocks/sessionLocks.contract.testHarness.ts b/src/node/services/sessionLocks/sessionLocks.contract.testHarness.ts index c6fa7f73436..2a2d39de503 100644 --- a/src/node/services/sessionLocks/sessionLocks.contract.testHarness.ts +++ b/src/node/services/sessionLocks/sessionLocks.contract.testHarness.ts @@ -152,6 +152,44 @@ export function describeSessionLocksContract( await release(); })); + test("an aborted signal wins over a live holder of a record", () => + withLayer(async (locks, t) => { + const lockPath = path.join(t.dir, "aborted.lock"); + const holder = await t.foreignHolder(lockPath, "live", "record"); + const error = await locks + .acquireCrossProcessLock({ + lockPath, + acquireTimeoutMs: SHORT_MS, + staleMs: 60_000, + timeoutMessage: "contract record is held.", + signal: AbortSignal.abort(), + }) + .then( + () => null, + (caught: unknown) => caught + ); + expect(error).not.toBeInstanceOf(CrossProcessLockTimeoutError); + expect((error as Error).message).toBe("Lock acquisition was aborted"); + await holder.release(); + })); + + test("createParentDirectory false on a missing parent rejects with ENOENT", () => + withLayer(async (locks, t) => { + const error = await locks + .acquireCrossProcessLock({ + lockPath: path.join(t.dir, "removed-run", "run.lock"), + createParentDirectory: false, + acquireTimeoutMs: 0, + staleMs: 60_000, + timeoutMessage: "contract record is held.", + }) + .then( + () => null, + (caught: unknown) => caught + ); + expect((error as NodeJS.ErrnoException).code).toBe("ENOENT"); + })); + test("useLeasesFor gives one object per config: hold, isHeld, release", () => withLayer(async (locks, t) => { const config = { rootDir: t.dir }; From 9675e47d03b74357983ae5f652484408d6f1a56e Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 23 Oct 2026 18:33:42 +0000 Subject: [PATCH 3/5] tests: check the signal first and the parent again after the take in the memory record lock Codex round 2 on #7367: an aborted signal wins over a missing parent, and a parent removed after the first check (or during the wait) refuses with ENOENT and frees the lock, as Node's write would. --- .../memorySessionLocks.testHarness.ts | 23 +++++++++++-- .../sessionLocks.contract.test.ts | 33 +++++++++++++++++++ .../sessionLocks.contract.testHarness.ts | 18 ++++++++++ 3 files changed, 72 insertions(+), 2 deletions(-) diff --git a/src/node/services/sessionLocks/memorySessionLocks.testHarness.ts b/src/node/services/sessionLocks/memorySessionLocks.testHarness.ts index 2ba03b0fb63..77f042a3b0a 100644 --- a/src/node/services/sessionLocks/memorySessionLocks.testHarness.ts +++ b/src/node/services/sessionLocks/memorySessionLocks.testHarness.ts @@ -217,9 +217,18 @@ export function memorySessionLocks(clock: HostClock, table: MemoryLockTable): Se return await nodeSessionLocks.acquireCrossProcessLock(options); } const { lockPath, acquireTimeoutMs, timeoutMessage, signal } = options; + // crossProcessLock.ts checks the signal before each attempt, so abort wins. + if (signal?.aborted) throw abortedAcquireError(); // Node rejects with ENOENT when the parent is gone and may not be recreated - // (a removed workflow run directory). Callers rely on that refusal. - if (options.createParentDirectory === false) await fs.stat(path.dirname(lockPath)); + // (a removed workflow run directory): each attempt's first write fails, before + // any holder is judged. Callers rely on that refusal. + const checkParent = + options.createParentDirectory === false + ? async () => { + await fs.stat(path.dirname(lockPath)); + } + : null; + await checkParent?.(); const refusal = (blocker: Blocker) => new CrossProcessLockTimeoutError( crossProcessLockTimeoutMessage(timeoutMessage, lockPath, { @@ -235,6 +244,16 @@ export function memorySessionLocks(clock: HostClock, table: MemoryLockTable): Se refusal, signal ); + // Check again once the lock is taken: a parent removed after the first check + // (or while this waited) must refuse here too, as Node's write would. + if (checkParent !== null) { + try { + await checkParent(); + } catch (error) { + table.free(lockPath, entry); + throw error; + } + } return () => { table.free(lockPath, entry); return Promise.resolve(); diff --git a/src/node/services/sessionLocks/sessionLocks.contract.test.ts b/src/node/services/sessionLocks/sessionLocks.contract.test.ts index fca6993622f..d7589997c86 100644 --- a/src/node/services/sessionLocks/sessionLocks.contract.test.ts +++ b/src/node/services/sessionLocks/sessionLocks.contract.test.ts @@ -226,6 +226,39 @@ test("memory: a release hands the file lock to the waiters in FIFO order", () => await expect(first.assertStillOwned()).rejects.toThrow("is no longer owned by this holder"); })); +test("memory: a parent removed while the acquire waits refuses with ENOENT", () => + onMemory(async (locks, t) => { + const runDir = path.join(t.dir, "run"); + await fs.mkdir(runDir); + const lockPath = path.join(runDir, "run.lock"); + const holder = await t.foreignHolder(lockPath, "live", "record"); + const acquire = locks + .acquireCrossProcessLock({ + lockPath, + createParentDirectory: false, + acquireTimeoutMs: 1_000, + staleMs: 60_000, + timeoutMessage: "run lock is held.", + }) + .then( + () => null, + (caught: unknown) => caught + ); + await fs.rm(runDir, { recursive: true }); + await holder.release(); + expect(((await acquire) as NodeJS.ErrnoException).code).toBe("ENOENT"); + // The refusal frees the lock: a later acquire takes it at once. + await fs.mkdir(runDir); + const release = await locks.acquireCrossProcessLock({ + lockPath, + createParentDirectory: false, + acquireTimeoutMs: 0, + staleMs: 60_000, + timeoutMessage: "run lock is held.", + }); + await release(); + })); + test("memory: an init-record lock makes a real file, and inspect sees it", () => onMemory(async (locks, t) => { const lockPath = path.join(t.dir, "locks", "init-record", "ws", "backend.lock"); diff --git a/src/node/services/sessionLocks/sessionLocks.contract.testHarness.ts b/src/node/services/sessionLocks/sessionLocks.contract.testHarness.ts index 2a2d39de503..8526e200d8d 100644 --- a/src/node/services/sessionLocks/sessionLocks.contract.testHarness.ts +++ b/src/node/services/sessionLocks/sessionLocks.contract.testHarness.ts @@ -190,6 +190,24 @@ export function describeSessionLocksContract( expect((error as NodeJS.ErrnoException).code).toBe("ENOENT"); })); + test("an aborted signal wins over a missing parent", () => + withLayer(async (locks, t) => { + const error = await locks + .acquireCrossProcessLock({ + lockPath: path.join(t.dir, "removed-run", "run.lock"), + createParentDirectory: false, + acquireTimeoutMs: 0, + staleMs: 60_000, + timeoutMessage: "contract record is held.", + signal: AbortSignal.abort(), + }) + .then( + () => null, + (caught: unknown) => caught + ); + expect((error as Error).message).toBe("Lock acquisition was aborted"); + })); + test("useLeasesFor gives one object per config: hold, isHeld, release", () => withLayer(async (locks, t) => { const config = { rootDir: t.dir }; From f7525029ffa159734a3200406e96814d91c0d30f Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 23 Oct 2026 18:44:37 +0000 Subject: [PATCH 4/5] tests: leave non-creatable record locks on the Node locks and count fake leases in isHeld Codex round 3 on #7367. The memory layer's parent checks did not converge, so a record lock with createParentDirectory false now goes to the Node locks, whose own write gives WorkflowRunStore the ENOENT it needs. Other record locks create their parent as Node does. The foreign layer's isHeld counts a live fake lease in the workspace's lease folder. --- .../foreignHolders.testHarness.ts | 13 ++++- .../memorySessionLocks.testHarness.ts | 34 ++++-------- .../sessionLocks.contract.test.ts | 52 +++++++------------ .../sessionLocks.contract.testHarness.ts | 14 +++++ 4 files changed, 54 insertions(+), 59 deletions(-) diff --git a/src/node/services/sessionLocks/foreignHolders.testHarness.ts b/src/node/services/sessionLocks/foreignHolders.testHarness.ts index e00729bd1e3..540ec9add82 100644 --- a/src/node/services/sessionLocks/foreignHolders.testHarness.ts +++ b/src/node/services/sessionLocks/foreignHolders.testHarness.ts @@ -5,11 +5,13 @@ * Node locks would wait the whole timeout first), so tests need no real wait. */ import assert from "node:assert"; +import * as path from "node:path"; import { Effect, Layer } from "effect"; import { SessionLocksTag } from "@/node/services/di/tags"; import { workspaceMutationInProgressMessage, workspaceMutationLockPath, + workspaceUseLockDir, } from "@/node/services/workspaceUseLeases"; import { fileLockTimeoutMessage, type FileLockBlocker } from "@/node/utils/concurrency/fileLock"; import { @@ -187,7 +189,16 @@ export function wrapWithForeignHolders(inner: SessionLocks, holders: ForeignHold } return lease; }, - isHeld: (workspaceId, kind) => leases.isHeld(workspaceId, kind), + // WorkspaceUseLeases.isHeld lists every backend's "..lock" in the + // workspace's lease folder. A live fake there is another backend's lease. + isHeld: async (workspaceId, kind) => { + const dir = workspaceUseLockDir(config.rootDir, workspaceId); + for (const [lockPath, holder] of table) { + const lease = path.dirname(lockPath) === dir && lockPath.endsWith(`.${kind}.lock`); + if (lease && holder.state === "live") return true; + } + return await leases.isHeld(workspaceId, kind); + }, }; leasesByConfig.set(config, wrapped); return wrapped; diff --git a/src/node/services/sessionLocks/memorySessionLocks.testHarness.ts b/src/node/services/sessionLocks/memorySessionLocks.testHarness.ts index 77f042a3b0a..10db7f2c606 100644 --- a/src/node/services/sessionLocks/memorySessionLocks.testHarness.ts +++ b/src/node/services/sessionLocks/memorySessionLocks.testHarness.ts @@ -2,10 +2,13 @@ * SessionLocks in memory: the lock files and records live in a table, and a * waiter waits on the HostClock, so under a TestClock a test moves time * instead of waiting. Refusals use today's text (fileLockTimeoutMessage, - * crossProcessLockTimeoutMessage). Two parts stay on the Node locks: + * crossProcessLockTimeoutMessage). Three parts stay on the Node locks: * - Init-record locks: InitStateManager lists their folder (initStateManager.ts * isRecordLockHeld), so a holder in memory would leave no file, and a live * init would look abandoned. + * - Record locks with createParentDirectory false: WorkflowRunStore needs their + * acquire to fail with ENOENT once the run directory is gone, with no window + * between a check and the take. Only the Node lock's own write gives that. * - The use leases: the mutation gate, isHeld and taskService read other * backends' lease files. Phase 5 moves them. */ @@ -213,22 +216,12 @@ export function memorySessionLocks(clock: HostClock, table: MemoryLockTable): Se }; }, acquireCrossProcessLock: async (options) => { - if (foundByReaddir(options.lockPath)) { + if (foundByReaddir(options.lockPath) || options.createParentDirectory === false) { return await nodeSessionLocks.acquireCrossProcessLock(options); } const { lockPath, acquireTimeoutMs, timeoutMessage, signal } = options; - // crossProcessLock.ts checks the signal before each attempt, so abort wins. - if (signal?.aborted) throw abortedAcquireError(); - // Node rejects with ENOENT when the parent is gone and may not be recreated - // (a removed workflow run directory): each attempt's first write fails, before - // any holder is judged. Callers rely on that refusal. - const checkParent = - options.createParentDirectory === false - ? async () => { - await fs.stat(path.dirname(lockPath)); - } - : null; - await checkParent?.(); + // As crossProcessLock.ts does first: a caller may write into the folder once it holds the lock. + await fs.mkdir(path.dirname(lockPath), { recursive: true }); const refusal = (blocker: Blocker) => new CrossProcessLockTimeoutError( crossProcessLockTimeoutMessage(timeoutMessage, lockPath, { @@ -244,23 +237,14 @@ export function memorySessionLocks(clock: HostClock, table: MemoryLockTable): Se refusal, signal ); - // Check again once the lock is taken: a parent removed after the first check - // (or while this waited) must refuse here too, as Node's write would. - if (checkParent !== null) { - try { - await checkParent(); - } catch (error) { - table.free(lockPath, entry); - throw error; - } - } return () => { table.free(lockPath, entry); return Promise.resolve(); }; }, + // A path the table does not hold may be a Node lock (see the module doc). inspectCrossProcessLock: async (lockPath) => - foundByReaddir(lockPath) + table.owner(lockPath) === undefined ? await nodeSessionLocks.inspectCrossProcessLock(lockPath) : inspectInMemory(table, lockPath), useLeasesFor: (config) => nodeSessionLocks.useLeasesFor(config), diff --git a/src/node/services/sessionLocks/sessionLocks.contract.test.ts b/src/node/services/sessionLocks/sessionLocks.contract.test.ts index d7589997c86..055b4df3d9d 100644 --- a/src/node/services/sessionLocks/sessionLocks.contract.test.ts +++ b/src/node/services/sessionLocks/sessionLocks.contract.test.ts @@ -10,6 +10,7 @@ import { WorkspaceMutationInProgressError, workspaceMutationInProgressMessage, workspaceMutationLockPath, + workspaceUseLockDir, } from "@/node/services/workspaceUseLeases"; import { fileLockTimeoutMessage } from "@/node/utils/concurrency/fileLock"; import type { ProcessIdentity } from "@/node/utils/concurrency/processLiveness"; @@ -226,39 +227,6 @@ test("memory: a release hands the file lock to the waiters in FIFO order", () => await expect(first.assertStillOwned()).rejects.toThrow("is no longer owned by this holder"); })); -test("memory: a parent removed while the acquire waits refuses with ENOENT", () => - onMemory(async (locks, t) => { - const runDir = path.join(t.dir, "run"); - await fs.mkdir(runDir); - const lockPath = path.join(runDir, "run.lock"); - const holder = await t.foreignHolder(lockPath, "live", "record"); - const acquire = locks - .acquireCrossProcessLock({ - lockPath, - createParentDirectory: false, - acquireTimeoutMs: 1_000, - staleMs: 60_000, - timeoutMessage: "run lock is held.", - }) - .then( - () => null, - (caught: unknown) => caught - ); - await fs.rm(runDir, { recursive: true }); - await holder.release(); - expect(((await acquire) as NodeJS.ErrnoException).code).toBe("ENOENT"); - // The refusal frees the lock: a later acquire takes it at once. - await fs.mkdir(runDir); - const release = await locks.acquireCrossProcessLock({ - lockPath, - createParentDirectory: false, - acquireTimeoutMs: 0, - staleMs: 60_000, - timeoutMessage: "run lock is held.", - }); - await release(); - })); - test("memory: an init-record lock makes a real file, and inspect sees it", () => onMemory(async (locks, t) => { const lockPath = path.join(t.dir, "locks", "init-record", "ws", "backend.lock"); @@ -373,3 +341,21 @@ test("foreign: hold() publishes its lease before it probes the gate", async () = await t.cleanup(); } }); + +test("foreign: a live fake lease of another backend makes isHeld true", async () => { + const t = await nodeLocks(); + try { + const holders = makeForeignHolders(); + const leases = (await buildSessionLocks(withForeignHolders(t.layer, holders))).useLeasesFor({ + rootDir: t.dir, + }); + const dir = workspaceUseLockDir(t.dir, "ws-leases"); + holders.hold(path.join(dir, "other-backend.turn.lock"), "live"); + holders.hold(path.join(dir, "other-backend.exec.lock"), "dead"); + expect(await leases.isHeld("ws-leases", "turn")).toBe(true); + expect(await leases.isHeld("ws-leases", "exec")).toBe(false); + expect(await leases.isHeld("ws-leases", "terminal")).toBe(false); + } finally { + await t.cleanup(); + } +}); diff --git a/src/node/services/sessionLocks/sessionLocks.contract.testHarness.ts b/src/node/services/sessionLocks/sessionLocks.contract.testHarness.ts index 8526e200d8d..63778070a99 100644 --- a/src/node/services/sessionLocks/sessionLocks.contract.testHarness.ts +++ b/src/node/services/sessionLocks/sessionLocks.contract.testHarness.ts @@ -4,6 +4,7 @@ * case builds a fresh layer: no holders, clock or lock state is shared. */ import { describe, expect, test } from "bun:test"; +import * as fs from "node:fs/promises"; import * as path from "node:path"; import { Effect, type Layer } from "effect"; import { SessionLocksTag } from "@/node/services/di/tags"; @@ -190,6 +191,19 @@ export function describeSessionLocksContract( expect((error as NodeJS.ErrnoException).code).toBe("ENOENT"); })); + test("a record lock creates its missing parent by default", () => + withLayer(async (locks, t) => { + const lockPath = path.join(t.dir, "new-dir", "record.lock"); + const release = await locks.acquireCrossProcessLock({ + lockPath, + acquireTimeoutMs: 0, + staleMs: 60_000, + timeoutMessage: "contract record is held.", + }); + expect((await fs.stat(path.dirname(lockPath))).isDirectory()).toBe(true); + await release(); + })); + test("an aborted signal wins over a missing parent", () => withLayer(async (locks, t) => { const error = await locks From 423f813f4229b734bd69265e3384aa4734495343 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 23 Oct 2026 18:53:56 +0000 Subject: [PATCH 5/5] tests: assert the two rejections without awaiting a void matcher @typescript-eslint/await-thenable (CI lint-full): bun types rejects.toThrow and rejects.toBeInstanceOf as void. --- .../sessionLocks/sessionLocks.contract.test.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/node/services/sessionLocks/sessionLocks.contract.test.ts b/src/node/services/sessionLocks/sessionLocks.contract.test.ts index 055b4df3d9d..47f71203a46 100644 --- a/src/node/services/sessionLocks/sessionLocks.contract.test.ts +++ b/src/node/services/sessionLocks/sessionLocks.contract.test.ts @@ -224,7 +224,11 @@ test("memory: a release hands the file lock to the waiters in FIFO order", () => await first[Symbol.asyncDispose](); await both; expect(order).toEqual(["b", "c"]); - await expect(first.assertStillOwned()).rejects.toThrow("is no longer owned by this holder"); + const stale = await first.assertStillOwned().then( + () => null, + (error: unknown) => error + ); + expect((stale as Error).message).toContain("is no longer owned by this holder"); })); test("memory: an init-record lock makes a real file, and inspect sees it", () => @@ -333,9 +337,11 @@ test("foreign: hold() publishes its lease before it probes the gate", async () = }, }; const leases = wrapWithForeignHolders(inner, holders).useLeasesFor({ rootDir: t.dir }); - await expect(leases.hold("ws-order", "turn")).rejects.toBeInstanceOf( - WorkspaceMutationInProgressError + const refused = await leases.hold("ws-order", "turn").then( + () => null, + (error: unknown) => error ); + expect(refused).toBeInstanceOf(WorkspaceMutationInProgressError); expect(await leases.isHeld("ws-order", "turn")).toBe(false); } finally { await t.cleanup();