From 41fc500abfaef6569886d598af90fef5f326d2f9 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 3 Nov 2026 19:54:22 +0000 Subject: [PATCH 01/11] test(storage): reproduce the lock's dead-breaker wait, swept waiter and sweep cost Four tests fail on main: - a dead breaker's break file blocks every writer for 60 s; - a waiter whose temp file another writer removed fails with a non-retryable STORAGE_WRITE_ERROR; - a sweep keeps a dead owner's fresh temp file; - an acquisition that finds the lock free still reads the artifacts folder (it removes an old leftover). --- .../unit/storage/artifactManifestLock.test.ts | 77 ++++++++++++++++++- 1 file changed, 76 insertions(+), 1 deletion(-) diff --git a/test/unit/storage/artifactManifestLock.test.ts b/test/unit/storage/artifactManifestLock.test.ts index e0b3a8c6..b4e5c28a 100644 --- a/test/unit/storage/artifactManifestLock.test.ts +++ b/test/unit/storage/artifactManifestLock.test.ts @@ -27,6 +27,7 @@ const fsHooks = vi.hoisted(() => ({ beforeLink: undefined as ((to: string) => Promise) | undefined, beforeRm: undefined as ((path: string) => Promise) | undefined, beforeWriteFile: undefined as ((path: string) => Promise) | undefined, + beforeUtimes: undefined as ((path: string) => Promise) | undefined, })); vi.mock('node:fs/promises', async () => { @@ -51,6 +52,10 @@ vi.mock('node:fs/promises', async () => { ); return actual.writeFile(...args); }, + utimes: async (...args: Parameters) => { + await fsHooks.beforeUtimes?.(String(args[0])); + return actual.utimes(...args); + }, }; }); @@ -119,6 +124,7 @@ describe('artifact manifest lock', () => { fsHooks.beforeLink = undefined; fsHooks.beforeRm = undefined; fsHooks.beforeWriteFile = undefined; + fsHooks.beforeUtimes = undefined; vi.useRealTimers(); await rm(home, { recursive: true, force: true }); }); @@ -224,7 +230,7 @@ describe('artifact manifest lock', () => { }, ); - it('removes old lock temp and break files when it takes the lock', async () => { + it('removes lock temp and break files whose owner is gone once it had to wait for the lock', async () => { const artifacts = join(sessionDirectory, 'artifacts'); const twoMinutesAgo = new Date(Date.now() - 120_000); for (const name of [ @@ -234,13 +240,82 @@ describe('artifact manifest lock', () => { await writeFile(join(artifacts, name), 'left over'); await utimes(join(artifacts, name), twoMinutesAgo, twoMinutesAgo); } + await writeFile( + join(artifacts, 'manifest.json.lock.tmp-dead'), + await deadOwnerRecord(), + ); await writeFile(join(artifacts, 'manifest.json.lock.tmp-new'), 'in use'); + await writeFile(lockPath, await deadOwnerRecord()); + freezeLockClock(); await withArtifactManifestLock(sessionDirectory, () => Promise.resolve()); expect(await readdir(artifacts)).toEqual(['manifest.json.lock.tmp-new']); }); + it('does not read the artifacts folder for leftovers when it finds the lock free', async () => { + const artifacts = join(sessionDirectory, 'artifacts'); + const twoMinutesAgo = new Date(Date.now() - 120_000); + await writeFile(join(artifacts, 'manifest.json.lock.tmp-old'), 'left over'); + await utimes( + join(artifacts, 'manifest.json.lock.tmp-old'), + twoMinutesAgo, + twoMinutesAgo, + ); + + await withArtifactManifestLock(sessionDirectory, () => Promise.resolve()); + + expect(await readdir(artifacts)).toEqual(['manifest.json.lock.tmp-old']); + }); + + it('removes the break file of a breaker that died and breaks the lock at once', async () => { + await writeFile(lockPath, await deadOwnerRecord()); + await writeFile(`${lockPath}.break`, await deadOwnerRecord()); + freezeLockClock(); + + await expect( + withArtifactManifestLock( + sessionDirectory, + () => Promise.resolve('ran'), + FAST, + ), + ).resolves.toBe('ran'); + expect(await readdir(join(sessionDirectory, 'artifacts'))).toEqual([]); + }); + + it('writes its temp file again when another writer removed it during the wait', async () => { + await writeFile(lockPath, ownerRecord(process.pid, true)); + freezeLockClock(); + let refreshes = 0; + // On the second attempt, a writer that took the lock while this one was + // stopped has removed this one's temp file and let go of the lock. + fsHooks.beforeUtimes = async (path) => { + if (!path.startsWith(`${lockPath}.tmp-`)) { + return; + } + refreshes += 1; + if (refreshes === 2) { + await rm(path); + await rm(lockPath); + } + }; + + await expect( + withArtifactManifestLock( + sessionDirectory, + async () => { + const owner = JSON.parse(await readFile(lockPath, 'utf8')) as { + pid: number; + }; + return owner.pid; + }, + FAST, + ), + ).resolves.toBe(process.pid); + expect(refreshes).toBe(2); + expect(await readdir(join(sessionDirectory, 'artifacts'))).toEqual([]); + }); + it('breaks a lock whose owner is gone', async () => { await writeFile(lockPath, await deadOwnerRecord()); freezeLockClock(); From 8c36ac896c7d83785273bdb6d7daa1213507b446 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 3 Nov 2026 19:55:46 +0000 Subject: [PATCH 02/11] fix(storage): judge a break file's owner, rewrite a swept temp file, skip the sweep on a free lock - A breaker that died holding manifest.json.lock.break blocked every writer on that session for 60 s. The break file holds the breaker's record, so writers now judge it like a lock: a dead breaker's break file goes at once, an unknown one after 60 s. - A waiter stopped for over 60 s found its temp file swept by another writer and failed with a non-retryable STORAGE_WRITE_ERROR when it resumed. It now writes the temp file again before the next attempt, and the sweep keeps temp and break files whose owner is alive. - The sweep read the whole artifacts folder on every acquisition (about 110 ms per lock cycle with 20000 files). Only a writer that found the lock taken runs it now. It also removes the files of dead owners at once instead of after 60 s. --- src/storage/README.md | 2 +- src/storage/artifactManifest.ts | 53 ++++++++++++++++++++++++++------- 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/src/storage/README.md b/src/storage/README.md index ef119a6b..94a48bf4 100644 --- a/src/storage/README.md +++ b/src/storage/README.md @@ -38,7 +38,7 @@ - `parseEventLogContent` and `validateEventRecords` require each record to match `EventRecordSchema`, the first `seq` to be 0, and each next `seq` to be the previous one plus 1. Blank lines are skipped. [eventLogCodec.ts:parseEventLogContent] [eventLogCodec.ts:validateEventRecords] - Read and write paths in `manifests.ts` must be non-empty and absolute. Invalid JSON and schema failures throw `MANIFEST_VALIDATION_ERROR`. Other read errors throw `STORAGE_READ_ERROR`. Write failures throw `STORAGE_WRITE_ERROR`, except that `writeManifest` and `writeDestroyMarker` throw `SESSION_NOT_FOUND` when the session directory is gone. `writeDestroyMarker` writes an empty file through a temp file and a rename, like `writeManifest`, so it replaces a symlink at that path instead of writing through it, and it creates no folder. [manifests.ts:readValidatedJsonFile] [manifests.ts:writeTextFileAtomic] [manifests.ts:writeManifest] [manifests.ts:writeDestroyMarker] - `ArtifactEntrySchema` and `ArtifactManifestSchema` are `.strict()`. `kind` is `screenshot`, `snapshot`, `recording` or `video`. `capturedAtSeq` is an integer of at least -1 (-1 for a capture before the first event). The manifest `version` is the literal `1`. The manifest and entry `sessionId` must equal the session directory name. [artifactManifest.ts:ArtifactEntrySchema] [artifactManifest.ts:ArtifactManifestSchema] -- `appendArtifactWithRollback` serializes appends per session directory with an in-process `KeyedSerializer` (from `src/util/keyedSerializer.ts`). `publishArtifactWithRollback` runs its publish step in the same queue, so a failed append cannot delete a file that another capture reused in between. Inside that queue, `withArtifactManifestLock` holds `/artifacts/manifest.json.lock` around the publish step and the manifest read and write, so the host and CLI processes do not drop each other's entries. The lock file is created from a temp file with `createWithoutReplacing` (a `link()`, or an exclusive copy on a filesystem without hard links), with the temp file's timestamps set just before each attempt, and holds `{ version: 1, token, pid, process }`, where `process` is the `captureHost` record or `null`. A writer waits up to 10 s and then throws a retryable `STORAGE_WRITE_ERROR` with `lockPath` and `holderPid`. Other file errors while taking the lock throw `STORAGE_WRITE_ERROR` too (`SESSION_NOT_FOUND` when the folder is gone), and `appendArtifactWithRollback` then removes `rollbackArtifactPath` unless the manifest already lists a file of that name. That check runs without the lock, so a concurrent writer that lists the same file name between the check and the removal is left with a missing file. A lock path that is not a regular file (a FIFO or a symlink) reads as an `unknown` owner, so a writer never blocks on it. It breaks the lock when `judgeHost` says the owner is dead, or when the owner reads `unknown` (a live pid on macOS, another pid namespace, an owner without a process record on Linux, or lock contents that do not parse) and the lock's modification time is more than 60 s away from now, in either direction. To break, a writer first takes `manifest.json.lock.break` the same exclusive way, re-reads the lock, removes it only when its bytes still equal the ones it judged, and then removes its break file. A break file older than 60 s is removed by age alone. Release removes the lock only when it holds this writer's token. It tries 3 times. When all fail, the process remembers the lock, and its next acquisition on that session breaks it at once. Until then, other processes wait for it as for a live owner. A writer that takes the lock also removes `manifest.json.lock.tmp-*` and break files that are older than 60 s. Limits: a writer with an `unknown` owner that stops for over 60 s inside the section loses the lock while it still runs, so two writers can overlap. A breaker stopped for over 60 s while it holds the break file loses it the same way. [artifactManifest.ts:withArtifactManifestLock] [artifactManifest.ts:appendArtifactWithRollback] [artifactManifest.ts:publishArtifactWithRollback] +- `appendArtifactWithRollback` serializes appends per session directory with an in-process `KeyedSerializer` (from `src/util/keyedSerializer.ts`). `publishArtifactWithRollback` runs its publish step in the same queue, so a failed append cannot delete a file that another capture reused in between. Inside that queue, `withArtifactManifestLock` holds `/artifacts/manifest.json.lock` around the publish step and the manifest read and write, so the host and CLI processes do not drop each other's entries. The lock file is created from a temp file with `createWithoutReplacing` (a `link()`, or an exclusive copy on a filesystem without hard links), with the temp file's timestamps set just before each attempt, and holds `{ version: 1, token, pid, process }`, where `process` is the `captureHost` record or `null`. A writer waits up to 10 s and then throws a retryable `STORAGE_WRITE_ERROR` with `lockPath` and `holderPid`. Other file errors while taking the lock throw `STORAGE_WRITE_ERROR` too (`SESSION_NOT_FOUND` when the folder is gone), and `appendArtifactWithRollback` then removes `rollbackArtifactPath` unless the manifest already lists a file of that name. That check runs without the lock, so a concurrent writer that lists the same file name between the check and the removal is left with a missing file. A lock path that is not a regular file (a FIFO or a symlink) reads as an `unknown` owner, so a writer never blocks on it. It breaks the lock when `judgeHost` says the owner is dead, or when the owner reads `unknown` (a live pid on macOS, another pid namespace, an owner without a process record on Linux, or lock contents that do not parse) and the lock's modification time is more than 60 s away from now, in either direction. To break, a writer first takes `manifest.json.lock.break` the same exclusive way, re-reads the lock, removes it only when its bytes still equal the ones it judged, and then removes its break file. The break file holds the breaker's record, and another writer removes it by the same rule as a lock: when `judgeHost` says the breaker is dead, or when the breaker reads `unknown` and the file is more than 60 s old. Release removes the lock only when it holds this writer's token. It tries 3 times. When all fail, the process remembers the lock, and its next acquisition on that session breaks it at once. Until then, other processes wait for it as for a live owner. A writer that found the lock taken removes, once it holds the lock, the `manifest.json.lock.tmp-*` and break files whose owner is gone by the same rule. A writer that finds the lock free does not read the artifacts folder, so the temp file of a waiter that died stays until a later writer finds the lock taken. A waiting writer whose temp file another writer removed writes it again before its next attempt. Limits: a writer with an `unknown` owner that stops for over 60 s inside the section loses the lock while it still runs, so two writers can overlap. A breaker stopped for over 60 s while it holds the break file loses it the same way. A writer with an `unknown` owner that stops for over 60 s between setting its temp file's timestamps and the `link` fails with `STORAGE_WRITE_ERROR` when another writer removed the temp file in that time. [artifactManifest.ts:withArtifactManifestLock] [artifactManifest.ts:appendArtifactWithRollback] [artifactManifest.ts:publishArtifactWithRollback] - `computeArtifactHealth` maps only `MANIFEST_VALIDATION_ERROR` to health `manifest-invalid` and rethrows other errors. It validates its result with `ArtifactHealthSummarySchema`. [artifactHealth.ts:computeArtifactHealth] - The registry read returns `[]` on a read error, invalid JSON or a schema failure. The read checks each `homes` entry on its own and drops an invalid entry, so the valid entries survive. `resolveHomeRegistryPath` ignores an empty or relative `XDG_STATE_HOME`. [homeRegistry.ts:createHomeRegistry] [homeRegistry.ts:resolveHomeRegistryPath] - `upsert` reads and then writes the registry with no lock. A code comment states that concurrent creates can race and drop an entry, which registers again on the next create. [homeRegistry.ts:createHomeRegistry] diff --git a/src/storage/artifactManifest.ts b/src/storage/artifactManifest.ts index 8a2f4790..af3a4efb 100644 --- a/src/storage/artifactManifest.ts +++ b/src/storage/artifactManifest.ts @@ -268,14 +268,13 @@ function isOlderThan(state: LockFileState, limitMs: number): boolean { return Math.abs(Date.now() - state.modifiedAtMs) > limitMs; } -function isStaleLock( - lockPath: string, +// The lock, break and temp files hold their writer's record. Their owner is +// gone when judgeHost says it is dead, or when it reads unknown and the file +// is older than unknownOwnerStaleMs. +function isOwnerGone( state: LockFileState, timing: ManifestLockTiming, ): boolean { - if (unreleasedLocks.get(lockPath) === state.contents) { - return true; - } const owner = parseLockOwner(state.contents); // Without a process record the pid may belong to another pid namespace. const verdict = @@ -287,6 +286,17 @@ function isStaleLock( : verdict === 'dead'; } +function isStaleLock( + lockPath: string, + state: LockFileState, + timing: ManifestLockTiming, +): boolean { + return ( + unreleasedLocks.get(lockPath) === state.contents || + isOwnerGone(state, timing) + ); +} + async function removeIfUnchanged( path: string, contents: string | null, @@ -318,7 +328,7 @@ async function breakStaleManifestLock( const breakPath = `${lockPath}.break`; if (!(await createWithoutReplacing(temporaryPath, breakPath))) { const breaker = await readLockFileState(breakPath); - if (breaker !== null && isOlderThan(breaker, timing.unknownOwnerStaleMs)) { + if (breaker !== null && isOwnerGone(breaker, timing)) { await removeIfUnchanged(breakPath, breaker.contents); } return 'held'; @@ -351,12 +361,29 @@ async function removeOldLockFiles( continue; } const state = await readLockFileState(path).catch(() => null); - if (state !== null && isOlderThan(state, timing.unknownOwnerStaleMs)) { + if (state !== null && isOwnerGone(state, timing)) { await removeIfUnchanged(path, state.contents).catch(() => undefined); } } } +// A writer stopped for over unknownOwnerStaleMs while it waits can find that +// another writer removed its temp file, so it writes the file again. +async function refreshTemporaryFile( + path: string, + contents: string, +): Promise { + const now = new Date(); + try { + await utimes(path, now, now); + } catch (error) { + if (!hasErrorCode(error, 'ENOENT')) { + throw error; + } + await writeFile(path, contents, { flag: 'wx', mode: 0o600 }); + } +} + async function acquireManifestLock( lockPath: string, contents: string, @@ -367,13 +394,19 @@ async function acquireManifestLock( await writeFile(temporaryPath, contents, { flag: 'wx', mode: 0o600 }); const deadline = Date.now() + timing.waitMs; let pollMs = 2; + let foundTaken = false; for (;;) { - const now = new Date(); - await utimes(temporaryPath, now, now); + await refreshTemporaryFile(temporaryPath, contents); if (await createWithoutReplacing(temporaryPath, lockPath)) { - await removeOldLockFiles(lockPath, temporaryPath, timing); + // Writers that die while they wait for or hold the lock leave lock + // files behind. Only a writer that found the lock taken scans the + // artifacts folder for them, so a free lock costs no scan. + if (foundTaken) { + await removeOldLockFiles(lockPath, temporaryPath, timing); + } return; } + foundTaken = true; const outcome = await breakStaleManifestLock( lockPath, temporaryPath, From 4dd56168484ce961908923fe419c8c113e59003b Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 3 Nov 2026 20:15:00 +0000 Subject: [PATCH 03/11] fix(storage): let a live breaker's break file expire after 60 s Codex review on #299: with the owner judgement alone, a breaker that is stopped (SIGSTOP) after it took the break file reads as alive on Linux, so its break file never went and every writer timed out while the stale lock stayed. The break file is a lease again: it also expires after 60 s while its breaker lives. A dead breaker's file still goes at once. --- src/storage/README.md | 2 +- src/storage/artifactManifest.ts | 22 ++++++++++++++-- .../unit/storage/artifactManifestLock.test.ts | 25 +++++++++++++++++++ 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/src/storage/README.md b/src/storage/README.md index 94a48bf4..e55f3748 100644 --- a/src/storage/README.md +++ b/src/storage/README.md @@ -38,7 +38,7 @@ - `parseEventLogContent` and `validateEventRecords` require each record to match `EventRecordSchema`, the first `seq` to be 0, and each next `seq` to be the previous one plus 1. Blank lines are skipped. [eventLogCodec.ts:parseEventLogContent] [eventLogCodec.ts:validateEventRecords] - Read and write paths in `manifests.ts` must be non-empty and absolute. Invalid JSON and schema failures throw `MANIFEST_VALIDATION_ERROR`. Other read errors throw `STORAGE_READ_ERROR`. Write failures throw `STORAGE_WRITE_ERROR`, except that `writeManifest` and `writeDestroyMarker` throw `SESSION_NOT_FOUND` when the session directory is gone. `writeDestroyMarker` writes an empty file through a temp file and a rename, like `writeManifest`, so it replaces a symlink at that path instead of writing through it, and it creates no folder. [manifests.ts:readValidatedJsonFile] [manifests.ts:writeTextFileAtomic] [manifests.ts:writeManifest] [manifests.ts:writeDestroyMarker] - `ArtifactEntrySchema` and `ArtifactManifestSchema` are `.strict()`. `kind` is `screenshot`, `snapshot`, `recording` or `video`. `capturedAtSeq` is an integer of at least -1 (-1 for a capture before the first event). The manifest `version` is the literal `1`. The manifest and entry `sessionId` must equal the session directory name. [artifactManifest.ts:ArtifactEntrySchema] [artifactManifest.ts:ArtifactManifestSchema] -- `appendArtifactWithRollback` serializes appends per session directory with an in-process `KeyedSerializer` (from `src/util/keyedSerializer.ts`). `publishArtifactWithRollback` runs its publish step in the same queue, so a failed append cannot delete a file that another capture reused in between. Inside that queue, `withArtifactManifestLock` holds `/artifacts/manifest.json.lock` around the publish step and the manifest read and write, so the host and CLI processes do not drop each other's entries. The lock file is created from a temp file with `createWithoutReplacing` (a `link()`, or an exclusive copy on a filesystem without hard links), with the temp file's timestamps set just before each attempt, and holds `{ version: 1, token, pid, process }`, where `process` is the `captureHost` record or `null`. A writer waits up to 10 s and then throws a retryable `STORAGE_WRITE_ERROR` with `lockPath` and `holderPid`. Other file errors while taking the lock throw `STORAGE_WRITE_ERROR` too (`SESSION_NOT_FOUND` when the folder is gone), and `appendArtifactWithRollback` then removes `rollbackArtifactPath` unless the manifest already lists a file of that name. That check runs without the lock, so a concurrent writer that lists the same file name between the check and the removal is left with a missing file. A lock path that is not a regular file (a FIFO or a symlink) reads as an `unknown` owner, so a writer never blocks on it. It breaks the lock when `judgeHost` says the owner is dead, or when the owner reads `unknown` (a live pid on macOS, another pid namespace, an owner without a process record on Linux, or lock contents that do not parse) and the lock's modification time is more than 60 s away from now, in either direction. To break, a writer first takes `manifest.json.lock.break` the same exclusive way, re-reads the lock, removes it only when its bytes still equal the ones it judged, and then removes its break file. The break file holds the breaker's record, and another writer removes it by the same rule as a lock: when `judgeHost` says the breaker is dead, or when the breaker reads `unknown` and the file is more than 60 s old. Release removes the lock only when it holds this writer's token. It tries 3 times. When all fail, the process remembers the lock, and its next acquisition on that session breaks it at once. Until then, other processes wait for it as for a live owner. A writer that found the lock taken removes, once it holds the lock, the `manifest.json.lock.tmp-*` and break files whose owner is gone by the same rule. A writer that finds the lock free does not read the artifacts folder, so the temp file of a waiter that died stays until a later writer finds the lock taken. A waiting writer whose temp file another writer removed writes it again before its next attempt. Limits: a writer with an `unknown` owner that stops for over 60 s inside the section loses the lock while it still runs, so two writers can overlap. A breaker stopped for over 60 s while it holds the break file loses it the same way. A writer with an `unknown` owner that stops for over 60 s between setting its temp file's timestamps and the `link` fails with `STORAGE_WRITE_ERROR` when another writer removed the temp file in that time. [artifactManifest.ts:withArtifactManifestLock] [artifactManifest.ts:appendArtifactWithRollback] [artifactManifest.ts:publishArtifactWithRollback] +- `appendArtifactWithRollback` serializes appends per session directory with an in-process `KeyedSerializer` (from `src/util/keyedSerializer.ts`). `publishArtifactWithRollback` runs its publish step in the same queue, so a failed append cannot delete a file that another capture reused in between. Inside that queue, `withArtifactManifestLock` holds `/artifacts/manifest.json.lock` around the publish step and the manifest read and write, so the host and CLI processes do not drop each other's entries. The lock file is created from a temp file with `createWithoutReplacing` (a `link()`, or an exclusive copy on a filesystem without hard links), with the temp file's timestamps set just before each attempt, and holds `{ version: 1, token, pid, process }`, where `process` is the `captureHost` record or `null`. A writer waits up to 10 s and then throws a retryable `STORAGE_WRITE_ERROR` with `lockPath` and `holderPid`. Other file errors while taking the lock throw `STORAGE_WRITE_ERROR` too (`SESSION_NOT_FOUND` when the folder is gone), and `appendArtifactWithRollback` then removes `rollbackArtifactPath` unless the manifest already lists a file of that name. That check runs without the lock, so a concurrent writer that lists the same file name between the check and the removal is left with a missing file. A lock path that is not a regular file (a FIFO or a symlink) reads as an `unknown` owner, so a writer never blocks on it. It breaks the lock when `judgeHost` says the owner is dead, or when the owner reads `unknown` (a live pid on macOS, another pid namespace, an owner without a process record on Linux, or lock contents that do not parse) and the lock's modification time is more than 60 s away from now, in either direction. To break, a writer first takes `manifest.json.lock.break` the same exclusive way, re-reads the lock, removes it only when its bytes still equal the ones it judged, and then removes its break file. The break file holds the breaker's record. Another writer removes it when `judgeHost` says the breaker is dead, or when the file is more than 60 s old, also while the breaker lives. Release removes the lock only when it holds this writer's token. It tries 3 times. When all fail, the process remembers the lock, and its next acquisition on that session breaks it at once. Until then, other processes wait for it as for a live owner. A writer that found the lock taken, once it holds the lock, removes each break file that it could remove as above, and each `manifest.json.lock.tmp-*` file whose owner is dead, or reads `unknown` and the file is more than 60 s old. A writer that finds the lock free does not read the artifacts folder, so the temp file of a waiter that died stays until a later writer finds the lock taken. A waiting writer whose temp file another writer removed writes it again before its next attempt. Limits: a writer with an `unknown` owner that stops for over 60 s inside the section loses the lock while it still runs, so two writers can overlap. A breaker stopped for over 60 s while it holds the break file loses it the same way. A writer with an `unknown` owner that stops for over 60 s between setting its temp file's timestamps and the `link` fails with `STORAGE_WRITE_ERROR` when another writer removed the temp file in that time. [artifactManifest.ts:withArtifactManifestLock] [artifactManifest.ts:appendArtifactWithRollback] [artifactManifest.ts:publishArtifactWithRollback] - `computeArtifactHealth` maps only `MANIFEST_VALIDATION_ERROR` to health `manifest-invalid` and rethrows other errors. It validates its result with `ArtifactHealthSummarySchema`. [artifactHealth.ts:computeArtifactHealth] - The registry read returns `[]` on a read error, invalid JSON or a schema failure. The read checks each `homes` entry on its own and drops an invalid entry, so the valid entries survive. `resolveHomeRegistryPath` ignores an empty or relative `XDG_STATE_HOME`. [homeRegistry.ts:createHomeRegistry] [homeRegistry.ts:resolveHomeRegistryPath] - `upsert` reads and then writes the registry with no lock. A code comment states that concurrent creates can race and drop an entry, which registers again on the next create. [homeRegistry.ts:createHomeRegistry] diff --git a/src/storage/artifactManifest.ts b/src/storage/artifactManifest.ts index af3a4efb..ccdc2cf5 100644 --- a/src/storage/artifactManifest.ts +++ b/src/storage/artifactManifest.ts @@ -286,6 +286,17 @@ function isOwnerGone( : verdict === 'dead'; } +// A break file is a lease: it also expires after unknownOwnerStaleMs while +// its breaker lives, so a stopped breaker cannot block every writer. +function isStaleBreakFile( + state: LockFileState, + timing: ManifestLockTiming, +): boolean { + return ( + isOwnerGone(state, timing) || isOlderThan(state, timing.unknownOwnerStaleMs) + ); +} + function isStaleLock( lockPath: string, state: LockFileState, @@ -328,7 +339,7 @@ async function breakStaleManifestLock( const breakPath = `${lockPath}.break`; if (!(await createWithoutReplacing(temporaryPath, breakPath))) { const breaker = await readLockFileState(breakPath); - if (breaker !== null && isOwnerGone(breaker, timing)) { + if (breaker !== null && isStaleBreakFile(breaker, timing)) { await removeIfUnchanged(breakPath, breaker.contents); } return 'held'; @@ -361,7 +372,14 @@ async function removeOldLockFiles( continue; } const state = await readLockFileState(path).catch(() => null); - if (state !== null && isOwnerGone(state, timing)) { + if (state === null) { + continue; + } + const stale = + name === `${lockName}.break` + ? isStaleBreakFile(state, timing) + : isOwnerGone(state, timing); + if (stale) { await removeIfUnchanged(path, state.contents).catch(() => undefined); } } diff --git a/test/unit/storage/artifactManifestLock.test.ts b/test/unit/storage/artifactManifestLock.test.ts index b4e5c28a..20c664ec 100644 --- a/test/unit/storage/artifactManifestLock.test.ts +++ b/test/unit/storage/artifactManifestLock.test.ts @@ -283,6 +283,31 @@ describe('artifact manifest lock', () => { expect(await readdir(join(sessionDirectory, 'artifacts'))).toEqual([]); }); + it('keeps the break file of a live breaker until it is older than unknownOwnerStaleMs', async () => { + const breakPath = `${lockPath}.break`; + await writeFile(lockPath, await deadOwnerRecord()); + await writeFile(breakPath, ownerRecord(process.pid, true)); + + await expect( + withArtifactManifestLock(sessionDirectory, () => Promise.resolve(), FAST), + ).rejects.toMatchObject({ code: ERROR_CODES.STORAGE_WRITE_ERROR }); + expect(await readFile(breakPath, 'utf8')).toBe( + ownerRecord(process.pid, true), + ); + + const twoMinutesAgo = new Date(Date.now() - 120_000); + await utimes(breakPath, twoMinutesAgo, twoMinutesAgo); + freezeLockClock(); + await expect( + withArtifactManifestLock( + sessionDirectory, + () => Promise.resolve('ran'), + FAST, + ), + ).resolves.toBe('ran'); + expect(await readdir(join(sessionDirectory, 'artifacts'))).toEqual([]); + }); + it('writes its temp file again when another writer removed it during the wait', async () => { await writeFile(lockPath, ownerRecord(process.pid, true)); freezeLockClock(); From 0e8323caba28d2888dfcee901ee8f417c34f3bfd Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 3 Nov 2026 22:11:09 +0000 Subject: [PATCH 04/11] test(storage): reproduce two writers inside after a dead breaker, and pin the sweep of its fresh break file - The verifier's trace T1 on #299, as a deterministic test: a writer that stops while it removes a dead breaker's break file later removes another writer's new break file by path, and two writers end up inside. It fails on 4dd56168 (2 inside). - A writer that waited for a live lock removes a dead breaker's fresh break file in its sweep. It passes, and it fails when the sweep judges break files by age only. --- .../unit/storage/artifactManifestLock.test.ts | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/test/unit/storage/artifactManifestLock.test.ts b/test/unit/storage/artifactManifestLock.test.ts index 20c664ec..5a3e9ec2 100644 --- a/test/unit/storage/artifactManifestLock.test.ts +++ b/test/unit/storage/artifactManifestLock.test.ts @@ -1,3 +1,4 @@ +import { AsyncLocalStorage } from 'node:async_hooks'; import { spawn, spawnSync } from 'node:child_process'; import { once } from 'node:events'; import { @@ -308,6 +309,94 @@ describe('artifact manifest lock', () => { expect(await readdir(join(sessionDirectory, 'artifacts'))).toEqual([]); }); + it("lets one breaker act at a time when one stops while it removes a dead breaker's break file", async () => { + // The trace: W2 judges a dead breaker's break file and stops before its + // rm. W1 then takes a new break file, reads the dead owner's lock and + // stops before its rm. W2 resumes, removes W1's break file by path, + // breaks the lock and enters. W1 resumes and removes W2's fresh lock. + await writeFile(lockPath, await deadOwnerRecord()); + await writeFile(`${lockPath}.break`, await deadOwnerRecord()); + const writer = new AsyncLocalStorage(); + const deferred = (): { promise: Promise; resolve: () => void } => { + let resolve = (): void => undefined; + const promise = new Promise((done) => { + resolve = done; + }); + return { promise, resolve }; + }; + const w2AtBreakRm = deferred(); + const resumeW2 = deferred(); + const w1AtLockRm = deferred(); + const resumeW1 = deferred(); + const firstInside = deferred(); + const secondInside = deferred(); + const leave = deferred(); + let w2Paused = false; + let w2Resumed = false; + let w1Paused = false; + fsHooks.beforeRm = async (path) => { + const who = writer.getStore(); + if (who === 'W2' && path === `${lockPath}.break` && !w2Paused) { + w2Paused = true; + w2AtBreakRm.resolve(); + await resumeW2.promise; + } + if (who === 'W1' && path === lockPath && !w2Resumed && !w1Paused) { + w1Paused = true; + w1AtLockRm.resolve(); + await resumeW1.promise; + } + }; + let inside = 0; + let maxInside = 0; + const update = async (): Promise => { + inside += 1; + maxInside = Math.max(maxInside, inside); + (inside === 1 ? firstInside : secondInside).resolve(); + await leave.promise; + inside -= 1; + }; + const contend = (who: string): Promise => + writer.run(who, () => + withArtifactManifestLock(sessionDirectory, update, { + waitMs: 10_000, + unknownOwnerStaleMs: 60_000, + }), + ); + + const w2 = contend('W2'); + await w2AtBreakRm.promise; + const w1 = contend('W1'); + await Promise.race([w1AtLockRm.promise, sleep(300)]); + w2Resumed = true; + resumeW2.resolve(); + await firstInside.promise; + resumeW1.resolve(); + await Promise.race([secondInside.promise, sleep(300)]); + leave.resolve(); + await Promise.all([w1, w2]); + + expect(maxInside).toBe(1); + expect(await readdir(join(sessionDirectory, 'artifacts'))).toEqual([]); + }); + + it("removes a dead breaker's fresh break file once it had to wait for a live lock", async () => { + await writeFile(lockPath, ownerRecord(process.pid, true)); + await writeFile(`${lockPath}.break`, await deadOwnerRecord()); + freezeLockClock(); + let refreshes = 0; + fsHooks.beforeUtimes = async (path) => { + if (path.startsWith(`${lockPath}.tmp-`) && (refreshes += 1) === 2) { + await rm(lockPath); + } + }; + + await withArtifactManifestLock(sessionDirectory, () => Promise.resolve()); + + expect(refreshes).toBe(2); + expect(await readdir(join(sessionDirectory, 'artifacts'))).toEqual([]); + }); + it('writes its temp file again when another writer removed it during the wait', async () => { await writeFile(lockPath, ownerRecord(process.pid, true)); freezeLockClock(); From ce57bdad279edad17d48724f765e955add38e428 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 3 Nov 2026 22:24:38 +0000 Subject: [PATCH 05/11] fix(storage): let breakers act one at a time through announcements The verifier of #299 (N1) found that a writer that stops while it removes a dead breaker's break file can later remove another breaker's new break file by path, so two breakers break and two writers end up inside. The fixed break file cannot keep breakers apart, because a stale one is read and removed in two steps. A breaker now first links its temp file to a name that only it uses, manifest.json.lock.breaker-, and lists the artifacts folder. When another breaker's announcement is there and not stale, it removes its own and waits with jitter. Of two breakers, the one that lists later sees the other, so at most one breaker is in the section, and only it removes the lock. Stale announcements go by the break-file lease rule; their names are unique, so removing one by path cannot hit a live breaker's. The acting breaker still takes the fixed break file to keep out versions without announcements. The README lists the limits. --- src/storage/README.md | 2 +- src/storage/artifactManifest.ts | 92 +++++++++++++++++++++++++-------- 2 files changed, 71 insertions(+), 23 deletions(-) diff --git a/src/storage/README.md b/src/storage/README.md index e55f3748..f6c42e41 100644 --- a/src/storage/README.md +++ b/src/storage/README.md @@ -38,7 +38,7 @@ - `parseEventLogContent` and `validateEventRecords` require each record to match `EventRecordSchema`, the first `seq` to be 0, and each next `seq` to be the previous one plus 1. Blank lines are skipped. [eventLogCodec.ts:parseEventLogContent] [eventLogCodec.ts:validateEventRecords] - Read and write paths in `manifests.ts` must be non-empty and absolute. Invalid JSON and schema failures throw `MANIFEST_VALIDATION_ERROR`. Other read errors throw `STORAGE_READ_ERROR`. Write failures throw `STORAGE_WRITE_ERROR`, except that `writeManifest` and `writeDestroyMarker` throw `SESSION_NOT_FOUND` when the session directory is gone. `writeDestroyMarker` writes an empty file through a temp file and a rename, like `writeManifest`, so it replaces a symlink at that path instead of writing through it, and it creates no folder. [manifests.ts:readValidatedJsonFile] [manifests.ts:writeTextFileAtomic] [manifests.ts:writeManifest] [manifests.ts:writeDestroyMarker] - `ArtifactEntrySchema` and `ArtifactManifestSchema` are `.strict()`. `kind` is `screenshot`, `snapshot`, `recording` or `video`. `capturedAtSeq` is an integer of at least -1 (-1 for a capture before the first event). The manifest `version` is the literal `1`. The manifest and entry `sessionId` must equal the session directory name. [artifactManifest.ts:ArtifactEntrySchema] [artifactManifest.ts:ArtifactManifestSchema] -- `appendArtifactWithRollback` serializes appends per session directory with an in-process `KeyedSerializer` (from `src/util/keyedSerializer.ts`). `publishArtifactWithRollback` runs its publish step in the same queue, so a failed append cannot delete a file that another capture reused in between. Inside that queue, `withArtifactManifestLock` holds `/artifacts/manifest.json.lock` around the publish step and the manifest read and write, so the host and CLI processes do not drop each other's entries. The lock file is created from a temp file with `createWithoutReplacing` (a `link()`, or an exclusive copy on a filesystem without hard links), with the temp file's timestamps set just before each attempt, and holds `{ version: 1, token, pid, process }`, where `process` is the `captureHost` record or `null`. A writer waits up to 10 s and then throws a retryable `STORAGE_WRITE_ERROR` with `lockPath` and `holderPid`. Other file errors while taking the lock throw `STORAGE_WRITE_ERROR` too (`SESSION_NOT_FOUND` when the folder is gone), and `appendArtifactWithRollback` then removes `rollbackArtifactPath` unless the manifest already lists a file of that name. That check runs without the lock, so a concurrent writer that lists the same file name between the check and the removal is left with a missing file. A lock path that is not a regular file (a FIFO or a symlink) reads as an `unknown` owner, so a writer never blocks on it. It breaks the lock when `judgeHost` says the owner is dead, or when the owner reads `unknown` (a live pid on macOS, another pid namespace, an owner without a process record on Linux, or lock contents that do not parse) and the lock's modification time is more than 60 s away from now, in either direction. To break, a writer first takes `manifest.json.lock.break` the same exclusive way, re-reads the lock, removes it only when its bytes still equal the ones it judged, and then removes its break file. The break file holds the breaker's record. Another writer removes it when `judgeHost` says the breaker is dead, or when the file is more than 60 s old, also while the breaker lives. Release removes the lock only when it holds this writer's token. It tries 3 times. When all fail, the process remembers the lock, and its next acquisition on that session breaks it at once. Until then, other processes wait for it as for a live owner. A writer that found the lock taken, once it holds the lock, removes each break file that it could remove as above, and each `manifest.json.lock.tmp-*` file whose owner is dead, or reads `unknown` and the file is more than 60 s old. A writer that finds the lock free does not read the artifacts folder, so the temp file of a waiter that died stays until a later writer finds the lock taken. A waiting writer whose temp file another writer removed writes it again before its next attempt. Limits: a writer with an `unknown` owner that stops for over 60 s inside the section loses the lock while it still runs, so two writers can overlap. A breaker stopped for over 60 s while it holds the break file loses it the same way. A writer with an `unknown` owner that stops for over 60 s between setting its temp file's timestamps and the `link` fails with `STORAGE_WRITE_ERROR` when another writer removed the temp file in that time. [artifactManifest.ts:withArtifactManifestLock] [artifactManifest.ts:appendArtifactWithRollback] [artifactManifest.ts:publishArtifactWithRollback] +- `appendArtifactWithRollback` serializes appends per session directory with an in-process `KeyedSerializer` (from `src/util/keyedSerializer.ts`). `publishArtifactWithRollback` runs its publish step in the same queue, so a failed append cannot delete a file that another capture reused in between. Inside that queue, `withArtifactManifestLock` holds `/artifacts/manifest.json.lock` around the publish step and the manifest read and write, so the host and CLI processes do not drop each other's entries. The lock file is created from a temp file with `createWithoutReplacing` (a `link()`, or an exclusive copy on a filesystem without hard links), with the temp file's timestamps set just before each attempt, and holds `{ version: 1, token, pid, process }`, where `process` is the `captureHost` record or `null`. A writer waits up to 10 s and then throws a retryable `STORAGE_WRITE_ERROR` with `lockPath` and `holderPid`. Other file errors while taking the lock throw `STORAGE_WRITE_ERROR` too (`SESSION_NOT_FOUND` when the folder is gone), and `appendArtifactWithRollback` then removes `rollbackArtifactPath` unless the manifest already lists a file of that name. That check runs without the lock, so a concurrent writer that lists the same file name between the check and the removal is left with a missing file. A lock path that is not a regular file (a FIFO or a symlink) reads as an `unknown` owner, so a writer never blocks on it. It breaks the lock when `judgeHost` says the owner is dead, or when the owner reads `unknown` (a live pid on macOS, another pid namespace, an owner without a process record on Linux, or lock contents that do not parse) and the lock's modification time is more than 60 s away from now, in either direction. To break, a writer first announces itself with `manifest.json.lock.breaker-`, a link of its temp file, and reads the artifacts folder. When another breaker's announcement is there and not stale (by the break-file rule below), it removes its own and waits. It removes stale announcements, whose names no other writer uses. So breakers act one at a time: of two that announce, the one that reads later sees the other. The acting breaker takes `manifest.json.lock.break` the same exclusive way as the lock, which also keeps out older agent-tty versions, re-reads the lock, removes it only when its bytes still equal the ones it judged, and then removes its break file and its announcement. A waiting writer sleeps a random 0.5 to 1.5 times its poll interval, so two breakers that saw each other do not meet again. The break file and the announcement hold the breaker's record. Another writer removes them when `judgeHost` says the breaker is dead, or when the file is more than 60 s old, also while the breaker lives. Release removes the lock only when it holds this writer's token. It tries 3 times. When all fail, the process remembers the lock, and its next acquisition on that session breaks it at once. Until then, other processes wait for it as for a live owner. A writer that found the lock taken, once it holds the lock, removes each break file and announcement that it could remove as above, and each `manifest.json.lock.tmp-*` file whose owner is dead, or reads `unknown` and the file is more than 60 s old. A writer that finds the lock free does not read the artifacts folder, so the temp file of a waiter that died stays until a later writer finds the lock taken. A waiting writer whose temp file another writer removed writes it again before its next attempt. Limits: a writer with an `unknown` owner that stops for over 60 s inside the section loses the lock while it still runs, so two writers can overlap. A breaker stopped for over 60 s while it holds its announcement or the break file loses them the same way. Writers of agent-tty versions without announcements take only the break file, and removing a stale break file reads it and removes it by path in two steps. So an older writer and a newer one that both remove a dead breaker's break file at the same time can still overlap, as two older writers can. A writer with an `unknown` owner that stops for over 60 s between setting its temp file's timestamps and the `link` fails with `STORAGE_WRITE_ERROR` when another writer removed the temp file in that time. [artifactManifest.ts:withArtifactManifestLock] [artifactManifest.ts:appendArtifactWithRollback] [artifactManifest.ts:publishArtifactWithRollback] - `computeArtifactHealth` maps only `MANIFEST_VALIDATION_ERROR` to health `manifest-invalid` and rethrows other errors. It validates its result with `ArtifactHealthSummarySchema`. [artifactHealth.ts:computeArtifactHealth] - The registry read returns `[]` on a read error, invalid JSON or a schema failure. The read checks each `homes` entry on its own and drops an invalid entry, so the valid entries survive. `resolveHomeRegistryPath` ignores an empty or relative `XDG_STATE_HOME`. [homeRegistry.ts:createHomeRegistry] [homeRegistry.ts:resolveHomeRegistryPath] - `upsert` reads and then writes the registry with no lock. A code comment states that concurrent creates can race and drop an entry, which registers again on the next create. [homeRegistry.ts:createHomeRegistry] diff --git a/src/storage/artifactManifest.ts b/src/storage/artifactManifest.ts index ccdc2cf5..7f17bf54 100644 --- a/src/storage/artifactManifest.ts +++ b/src/storage/artifactManifest.ts @@ -318,13 +318,46 @@ async function removeIfUnchanged( } } +// Returns true when another breaker that is not stale announced itself. +// It removes the announcements of stale breakers on the way. Each has a +// name that no other writer uses, so removing it by path cannot remove a +// live breaker's announcement. +async function otherBreakerAnnounced( + lockPath: string, + announcementPath: string, + timing: ManifestLockTiming, +): Promise { + const directory = dirname(lockPath); + const prefix = `${basename(lockPath)}.breaker-`; + for (const name of await readdir(directory)) { + const path = join(directory, name); + if (!name.startsWith(prefix) || path === announcementPath) { + continue; + } + const state = await readLockFileState(path); + if (state === null) { + continue; + } + if (!isStaleBreakFile(state, timing)) { + return true; + } + await removeIfUnchanged(path, state.contents); + } + return false; +} + /** - * Breakers take `.break` first, so only one of them can remove the - * stale lock, and only while it still holds the bytes that were judged. + * A breaker first announces itself with `.breaker-` and acts only + * when no other live breaker announced, so breakers act one at a time: of + * two that announce, the one that scans later sees the other. The acting + * breaker takes `.break` the same exclusive way, which also keeps out + * older agent-tty versions, and removes the stale lock only while it still + * holds the bytes that were judged. */ async function breakStaleManifestLock( lockPath: string, temporaryPath: string, + announcementPath: string, contents: string, timing: ManifestLockTiming, ): Promise<'retry' | 'held'> { @@ -336,23 +369,33 @@ async function breakStaleManifestLock( return 'held'; } - const breakPath = `${lockPath}.break`; - if (!(await createWithoutReplacing(temporaryPath, breakPath))) { - const breaker = await readLockFileState(breakPath); - if (breaker !== null && isStaleBreakFile(breaker, timing)) { - await removeIfUnchanged(breakPath, breaker.contents); - } - return 'held'; - } + // The name is this writer's own, so it can already exist only from this + // writer's earlier attempt. + await createWithoutReplacing(temporaryPath, announcementPath); try { - await removeIfUnchanged(lockPath, judged.contents); - if (unreleasedLocks.get(lockPath) === judged.contents) { - unreleasedLocks.delete(lockPath); + if (await otherBreakerAnnounced(lockPath, announcementPath, timing)) { + return 'held'; } + const breakPath = `${lockPath}.break`; + if (!(await createWithoutReplacing(temporaryPath, breakPath))) { + const breaker = await readLockFileState(breakPath); + if (breaker !== null && isStaleBreakFile(breaker, timing)) { + await removeIfUnchanged(breakPath, breaker.contents); + } + return 'held'; + } + try { + await removeIfUnchanged(lockPath, judged.contents); + if (unreleasedLocks.get(lockPath) === judged.contents) { + unreleasedLocks.delete(lockPath); + } + } finally { + await removeIfUnchanged(breakPath, contents); + } + return 'retry'; } finally { - await removeIfUnchanged(breakPath, contents); + await rm(announcementPath, { force: true }); } - return 'retry'; } async function removeOldLockFiles( @@ -365,9 +408,11 @@ async function removeOldLockFiles( const names = await readdir(directory).catch((): string[] => []); for (const name of names) { const path = join(directory, name); + const breakFile = + name === `${lockName}.break` || name.startsWith(`${lockName}.breaker-`); if ( path === temporaryPath || - (name !== `${lockName}.break` && !name.startsWith(`${lockName}.tmp-`)) + (!breakFile && !name.startsWith(`${lockName}.tmp-`)) ) { continue; } @@ -375,10 +420,9 @@ async function removeOldLockFiles( if (state === null) { continue; } - const stale = - name === `${lockName}.break` - ? isStaleBreakFile(state, timing) - : isOwnerGone(state, timing); + const stale = breakFile + ? isStaleBreakFile(state, timing) + : isOwnerGone(state, timing); if (stale) { await removeIfUnchanged(path, state.contents).catch(() => undefined); } @@ -407,7 +451,9 @@ async function acquireManifestLock( contents: string, timing: ManifestLockTiming, ): Promise { - const temporaryPath = `${lockPath}.tmp-${randomUUID()}`; + const id = randomUUID(); + const temporaryPath = `${lockPath}.tmp-${id}`; + const announcementPath = `${lockPath}.breaker-${id}`; try { await writeFile(temporaryPath, contents, { flag: 'wx', mode: 0o600 }); const deadline = Date.now() + timing.waitMs; @@ -428,6 +474,7 @@ async function acquireManifestLock( const outcome = await breakStaleManifestLock( lockPath, temporaryPath, + announcementPath, contents, timing, ); @@ -440,7 +487,8 @@ async function acquireManifestLock( }); } if (outcome === 'held') { - await sleep(pollMs); + // Jitter, so two breakers that saw each other do not meet again. + await sleep(pollMs * (0.5 + Math.random())); pollMs = Math.min(pollMs * 2, MAX_LOCK_POLL_MS); } } From 6a994b558dde4ac877ec205da2db63af7be1c2a5 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 3 Nov 2026 23:54:34 +0000 Subject: [PATCH 06/11] test(storage): reproduce breakers that keep meeting after a crash in a large artifacts folder The delta verifier of #299 found that 30 writers on a dead owner's lock all time out when artifacts/ holds 20000 files: each breaker lists the whole folder while its announcement exists, so they keep seeing each other. The test fails on ce57bdad (30 of 30 rejected). --- .../unit/storage/artifactManifestLock.test.ts | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/unit/storage/artifactManifestLock.test.ts b/test/unit/storage/artifactManifestLock.test.ts index 5a3e9ec2..559a8643 100644 --- a/test/unit/storage/artifactManifestLock.test.ts +++ b/test/unit/storage/artifactManifestLock.test.ts @@ -380,6 +380,40 @@ describe('artifact manifest lock', () => { expect(await readdir(join(sessionDirectory, 'artifacts'))).toEqual([]); }); + it("lets 30 writers through a dead owner's lock when the artifacts folder holds 20000 files", async () => { + const artifacts = join(sessionDirectory, 'artifacts'); + for (let start = 0; start < 20_000; start += 200) { + await Promise.all( + Array.from({ length: 200 }, (_, index) => + writeFile(join(artifacts, `f${String(start + index)}.png`), ''), + ), + ); + } + await writeFile(lockPath, await deadOwnerRecord()); + let inside = 0; + let maxInside = 0; + const update = async (): Promise => { + inside += 1; + maxInside = Math.max(maxInside, inside); + await sleep(1); + inside -= 1; + }; + + const results = await Promise.allSettled( + Array.from({ length: 30 }, () => + withArtifactManifestLock(sessionDirectory, update, { + waitMs: 10_000, + unknownOwnerStaleMs: 60_000, + }), + ), + ); + + expect(results.filter((result) => result.status === 'rejected')).toEqual( + [], + ); + expect(maxInside).toBe(1); + }, 60_000); + it("removes a dead breaker's fresh break file once it had to wait for a live lock", async () => { await writeFile(lockPath, ownerRecord(process.pid, true)); await writeFile(`${lockPath}.break`, await deadOwnerRecord()); From aabe0593cf77570e94ee30f331571e085a477a2d Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 4 Nov 2026 00:03:15 +0000 Subject: [PATCH 07/11] test(storage): pin that a breaker lists only the announcements, not the artifacts folder The in-process burst test of 6a994b55 measured the cost of 30 waiters in one event loop (judgeHost reads /proc synchronously on every poll), which no real session has: the host serializes its appends. It is replaced by a deterministic test of the mechanism behind the livelock: while a breaker's announcement exists, no listing of artifacts/ may run. It fails on ce57bdad. The multi-process 30-writer, 20000-file burst is a stress check (the verifier's fuzz.mjs), not a unit test. --- .../unit/storage/artifactManifestLock.test.ts | 65 +++++++++++-------- 1 file changed, 37 insertions(+), 28 deletions(-) diff --git a/test/unit/storage/artifactManifestLock.test.ts b/test/unit/storage/artifactManifestLock.test.ts index 559a8643..5d3d3fe8 100644 --- a/test/unit/storage/artifactManifestLock.test.ts +++ b/test/unit/storage/artifactManifestLock.test.ts @@ -1,6 +1,7 @@ import { AsyncLocalStorage } from 'node:async_hooks'; import { spawn, spawnSync } from 'node:child_process'; import { once } from 'node:events'; +import { readdirSync } from 'node:fs'; import { mkdir, mkdtemp, @@ -29,6 +30,7 @@ const fsHooks = vi.hoisted(() => ({ beforeRm: undefined as ((path: string) => Promise) | undefined, beforeWriteFile: undefined as ((path: string) => Promise) | undefined, beforeUtimes: undefined as ((path: string) => Promise) | undefined, + beforeReaddir: undefined as ((path: string) => Promise) | undefined, })); vi.mock('node:fs/promises', async () => { @@ -53,6 +55,10 @@ vi.mock('node:fs/promises', async () => { ); return actual.writeFile(...args); }, + readdir: async (...args: Parameters) => { + await fsHooks.beforeReaddir?.(String(args[0])); + return actual.readdir(...args); + }, utimes: async (...args: Parameters) => { await fsHooks.beforeUtimes?.(String(args[0])); return actual.utimes(...args); @@ -126,6 +132,7 @@ describe('artifact manifest lock', () => { fsHooks.beforeRm = undefined; fsHooks.beforeWriteFile = undefined; fsHooks.beforeUtimes = undefined; + fsHooks.beforeReaddir = undefined; vi.useRealTimers(); await rm(home, { recursive: true, force: true }); }); @@ -380,39 +387,41 @@ describe('artifact manifest lock', () => { expect(await readdir(join(sessionDirectory, 'artifacts'))).toEqual([]); }); - it("lets 30 writers through a dead owner's lock when the artifacts folder holds 20000 files", async () => { + it('lists only the announcements while it announces itself, not the artifacts folder', async () => { + // A listing of a large artifacts folder while the announcement exists + // made breakers that started together keep seeing each other. const artifacts = join(sessionDirectory, 'artifacts'); - for (let start = 0; start < 20_000; start += 200) { - await Promise.all( - Array.from({ length: 200 }, (_, index) => - writeFile(join(artifacts, `f${String(start + index)}.png`), ''), - ), - ); - } await writeFile(lockPath, await deadOwnerRecord()); - let inside = 0; - let maxInside = 0; - const update = async (): Promise => { - inside += 1; - maxInside = Math.max(maxInside, inside); - await sleep(1); - inside -= 1; + const announced = (): boolean => { + const inFolder = (path: string): string[] => { + try { + return readdirSync(path); + } catch { + return []; + } + }; + return ( + inFolder(`${lockPath}.breakers`).length > 0 || + inFolder(artifacts).some((name) => + name.startsWith('manifest.json.lock.breaker'), + ) + ); + }; + const listedWhileAnnounced: string[] = []; + fsHooks.beforeReaddir = (path) => { + if (announced()) { + listedWhileAnnounced.push(path); + } + return Promise.resolve(); }; - const results = await Promise.allSettled( - Array.from({ length: 30 }, () => - withArtifactManifestLock(sessionDirectory, update, { - waitMs: 10_000, - unknownOwnerStaleMs: 60_000, - }), - ), - ); + await withArtifactManifestLock(sessionDirectory, () => Promise.resolve()); + fsHooks.beforeReaddir = undefined; - expect(results.filter((result) => result.status === 'rejected')).toEqual( - [], - ); - expect(maxInside).toBe(1); - }, 60_000); + expect(listedWhileAnnounced).not.toHaveLength(0); + expect(listedWhileAnnounced).not.toContain(artifacts); + expect(await readdir(artifacts)).toEqual([]); + }); it("removes a dead breaker's fresh break file once it had to wait for a live lock", async () => { await writeFile(lockPath, ownerRecord(process.pid, true)); From 97170677085b0cadb84f93d3ebfd7e328743a1a2 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 4 Nov 2026 00:51:29 +0000 Subject: [PATCH 08/11] fix(storage): keep breaker announcements in their own folder and back off on each meeting The delta verifier of #299 found a livelock after a crash in a large session: each breaker listed the whole artifacts folder while its announcement existed (tens of ms at 20000 files) and retried after 25 to 75 ms, so 30 breakers kept seeing each other and all timed out. Announcements now live in artifacts/manifest.json.lock.breakers/, which holds nothing else, so the listing does not grow with the artifacts. The folder is made with mkdir (no recursive) and removed with rmdir, which removes only an empty folder, so no announcement goes with it. A link into a folder that another breaker just removed fails, and the breaker tries again without an announcement. A breaker that met another waits a random 0.5 to 1.5 times a backoff of 5 ms that doubles per meeting, up to 200 ms. --- src/storage/README.md | 2 +- src/storage/artifactManifest.ts | 99 +++++++++++++++++++++++++-------- 2 files changed, 76 insertions(+), 25 deletions(-) diff --git a/src/storage/README.md b/src/storage/README.md index f6c42e41..7f078b0d 100644 --- a/src/storage/README.md +++ b/src/storage/README.md @@ -38,7 +38,7 @@ - `parseEventLogContent` and `validateEventRecords` require each record to match `EventRecordSchema`, the first `seq` to be 0, and each next `seq` to be the previous one plus 1. Blank lines are skipped. [eventLogCodec.ts:parseEventLogContent] [eventLogCodec.ts:validateEventRecords] - Read and write paths in `manifests.ts` must be non-empty and absolute. Invalid JSON and schema failures throw `MANIFEST_VALIDATION_ERROR`. Other read errors throw `STORAGE_READ_ERROR`. Write failures throw `STORAGE_WRITE_ERROR`, except that `writeManifest` and `writeDestroyMarker` throw `SESSION_NOT_FOUND` when the session directory is gone. `writeDestroyMarker` writes an empty file through a temp file and a rename, like `writeManifest`, so it replaces a symlink at that path instead of writing through it, and it creates no folder. [manifests.ts:readValidatedJsonFile] [manifests.ts:writeTextFileAtomic] [manifests.ts:writeManifest] [manifests.ts:writeDestroyMarker] - `ArtifactEntrySchema` and `ArtifactManifestSchema` are `.strict()`. `kind` is `screenshot`, `snapshot`, `recording` or `video`. `capturedAtSeq` is an integer of at least -1 (-1 for a capture before the first event). The manifest `version` is the literal `1`. The manifest and entry `sessionId` must equal the session directory name. [artifactManifest.ts:ArtifactEntrySchema] [artifactManifest.ts:ArtifactManifestSchema] -- `appendArtifactWithRollback` serializes appends per session directory with an in-process `KeyedSerializer` (from `src/util/keyedSerializer.ts`). `publishArtifactWithRollback` runs its publish step in the same queue, so a failed append cannot delete a file that another capture reused in between. Inside that queue, `withArtifactManifestLock` holds `/artifacts/manifest.json.lock` around the publish step and the manifest read and write, so the host and CLI processes do not drop each other's entries. The lock file is created from a temp file with `createWithoutReplacing` (a `link()`, or an exclusive copy on a filesystem without hard links), with the temp file's timestamps set just before each attempt, and holds `{ version: 1, token, pid, process }`, where `process` is the `captureHost` record or `null`. A writer waits up to 10 s and then throws a retryable `STORAGE_WRITE_ERROR` with `lockPath` and `holderPid`. Other file errors while taking the lock throw `STORAGE_WRITE_ERROR` too (`SESSION_NOT_FOUND` when the folder is gone), and `appendArtifactWithRollback` then removes `rollbackArtifactPath` unless the manifest already lists a file of that name. That check runs without the lock, so a concurrent writer that lists the same file name between the check and the removal is left with a missing file. A lock path that is not a regular file (a FIFO or a symlink) reads as an `unknown` owner, so a writer never blocks on it. It breaks the lock when `judgeHost` says the owner is dead, or when the owner reads `unknown` (a live pid on macOS, another pid namespace, an owner without a process record on Linux, or lock contents that do not parse) and the lock's modification time is more than 60 s away from now, in either direction. To break, a writer first announces itself with `manifest.json.lock.breaker-`, a link of its temp file, and reads the artifacts folder. When another breaker's announcement is there and not stale (by the break-file rule below), it removes its own and waits. It removes stale announcements, whose names no other writer uses. So breakers act one at a time: of two that announce, the one that reads later sees the other. The acting breaker takes `manifest.json.lock.break` the same exclusive way as the lock, which also keeps out older agent-tty versions, re-reads the lock, removes it only when its bytes still equal the ones it judged, and then removes its break file and its announcement. A waiting writer sleeps a random 0.5 to 1.5 times its poll interval, so two breakers that saw each other do not meet again. The break file and the announcement hold the breaker's record. Another writer removes them when `judgeHost` says the breaker is dead, or when the file is more than 60 s old, also while the breaker lives. Release removes the lock only when it holds this writer's token. It tries 3 times. When all fail, the process remembers the lock, and its next acquisition on that session breaks it at once. Until then, other processes wait for it as for a live owner. A writer that found the lock taken, once it holds the lock, removes each break file and announcement that it could remove as above, and each `manifest.json.lock.tmp-*` file whose owner is dead, or reads `unknown` and the file is more than 60 s old. A writer that finds the lock free does not read the artifacts folder, so the temp file of a waiter that died stays until a later writer finds the lock taken. A waiting writer whose temp file another writer removed writes it again before its next attempt. Limits: a writer with an `unknown` owner that stops for over 60 s inside the section loses the lock while it still runs, so two writers can overlap. A breaker stopped for over 60 s while it holds its announcement or the break file loses them the same way. Writers of agent-tty versions without announcements take only the break file, and removing a stale break file reads it and removes it by path in two steps. So an older writer and a newer one that both remove a dead breaker's break file at the same time can still overlap, as two older writers can. A writer with an `unknown` owner that stops for over 60 s between setting its temp file's timestamps and the `link` fails with `STORAGE_WRITE_ERROR` when another writer removed the temp file in that time. [artifactManifest.ts:withArtifactManifestLock] [artifactManifest.ts:appendArtifactWithRollback] [artifactManifest.ts:publishArtifactWithRollback] +- `appendArtifactWithRollback` serializes appends per session directory with an in-process `KeyedSerializer` (from `src/util/keyedSerializer.ts`). `publishArtifactWithRollback` runs its publish step in the same queue, so a failed append cannot delete a file that another capture reused in between. Inside that queue, `withArtifactManifestLock` holds `/artifacts/manifest.json.lock` around the publish step and the manifest read and write, so the host and CLI processes do not drop each other's entries. The lock file is created from a temp file with `createWithoutReplacing` (a `link()`, or an exclusive copy on a filesystem without hard links), with the temp file's timestamps set just before each attempt, and holds `{ version: 1, token, pid, process }`, where `process` is the `captureHost` record or `null`. A writer waits up to 10 s and then throws a retryable `STORAGE_WRITE_ERROR` with `lockPath` and `holderPid`. Other file errors while taking the lock throw `STORAGE_WRITE_ERROR` too (`SESSION_NOT_FOUND` when the folder is gone), and `appendArtifactWithRollback` then removes `rollbackArtifactPath` unless the manifest already lists a file of that name. That check runs without the lock, so a concurrent writer that lists the same file name between the check and the removal is left with a missing file. A lock path that is not a regular file (a FIFO or a symlink) reads as an `unknown` owner, so a writer never blocks on it. It breaks the lock when `judgeHost` says the owner is dead, or when the owner reads `unknown` (a live pid on macOS, another pid namespace, an owner without a process record on Linux, or lock contents that do not parse) and the lock's modification time is more than 60 s away from now, in either direction. To break, a writer first announces itself with `manifest.json.lock.breakers/`, a link of its temp file in a folder that holds only announcements, and lists that folder. So the listing costs the same however many artifacts the session holds. It creates the folder without `recursive`, and the last breaker removes it with `rmdir`, which removes only an empty folder. When another breaker's announcement is there and not stale (by the break-file rule below), it removes its own and waits. It removes stale announcements, whose names no other writer uses. So breakers act one at a time: of two that announce, the one that reads later sees the other. The acting breaker takes `manifest.json.lock.break` the same exclusive way as the lock, which also keeps out older agent-tty versions, re-reads the lock, removes it only when its bytes still equal the ones it judged, and then removes its break file and its announcement. A breaker that saw another one waits a random 0.5 to 1.5 times a backoff of 5 ms that doubles with each such meeting, up to 200 ms. Breakers that keep meeting so spread their attempts until one of them is alone. This makes a meeting less likely each time. It does not rule one out. The break file and the announcement hold the breaker's record. Another writer removes them when `judgeHost` says the breaker is dead, or when the file is more than 60 s old, also while the breaker lives. Release removes the lock only when it holds this writer's token. It tries 3 times. When all fail, the process remembers the lock, and its next acquisition on that session breaks it at once. Until then, other processes wait for it as for a live owner. A writer that found the lock taken, once it holds the lock, removes each break file and announcement that it could remove as above, and each `manifest.json.lock.tmp-*` file whose owner is dead, or reads `unknown` and the file is more than 60 s old. A writer that finds the lock free does not read the artifacts folder, so the temp file of a waiter that died stays until a later writer finds the lock taken. A waiting writer whose temp file another writer removed writes it again before its next attempt. Limits: a writer with an `unknown` owner that stops for over 60 s inside the section loses the lock while it still runs, so two writers can overlap. A breaker stopped for over 60 s while it holds its announcement or the break file loses them the same way. Writers of agent-tty versions without announcements take only the break file, and removing a stale break file reads it and removes it by path in two steps. So an older writer and a newer one that both remove a dead breaker's break file at the same time can still overlap, as two older writers can. A writer with an `unknown` owner that stops for over 60 s between setting its temp file's timestamps and the `link` fails with `STORAGE_WRITE_ERROR` when another writer removed the temp file in that time. [artifactManifest.ts:withArtifactManifestLock] [artifactManifest.ts:appendArtifactWithRollback] [artifactManifest.ts:publishArtifactWithRollback] - `computeArtifactHealth` maps only `MANIFEST_VALIDATION_ERROR` to health `manifest-invalid` and rethrows other errors. It validates its result with `ArtifactHealthSummarySchema`. [artifactHealth.ts:computeArtifactHealth] - The registry read returns `[]` on a read error, invalid JSON or a schema failure. The read checks each `homes` entry on its own and drops an invalid entry, so the valid entries survive. `resolveHomeRegistryPath` ignores an empty or relative `XDG_STATE_HOME`. [homeRegistry.ts:createHomeRegistry] [homeRegistry.ts:resolveHomeRegistryPath] - `upsert` reads and then writes the registry with no lock. A code comment states that concurrent creates can race and drop an entry, which registers again on the next create. [homeRegistry.ts:createHomeRegistry] diff --git a/src/storage/artifactManifest.ts b/src/storage/artifactManifest.ts index 7f17bf54..4ca30ec2 100644 --- a/src/storage/artifactManifest.ts +++ b/src/storage/artifactManifest.ts @@ -1,5 +1,13 @@ import { randomUUID } from 'node:crypto'; -import { lstat, readdir, rm, utimes, writeFile } from 'node:fs/promises'; +import { + lstat, + mkdir, + readdir, + rm, + rmdir, + utimes, + writeFile, +} from 'node:fs/promises'; import { basename, dirname, isAbsolute, join, resolve } from 'node:path'; import process from 'node:process'; import { setTimeout as sleep } from 'node:timers/promises'; @@ -33,6 +41,10 @@ import { KeyedSerializer } from '../util/keyedSerializer.js'; const ARTIFACT_MANIFEST_FILENAME = 'manifest.json'; const ARTIFACT_MANIFEST_LOCK_FILENAME = 'manifest.json.lock'; const MAX_LOCK_POLL_MS = 50; +// A breaker that met another waits this long times 2^(meetings - 1), up +// to MAX_MEETING_BACKOFF_MS, with jitter. +const MEETING_BACKOFF_MS = 5; +const MAX_MEETING_BACKOFF_MS = 200; const MAX_LOCK_RELEASE_ATTEMPTS = 3; const NonEmptyStringSchema = z.string().min(1); const IsoDatetimeSchema = z.iso.datetime(); @@ -318,6 +330,12 @@ async function removeIfUnchanged( } } +// Breakers announce themselves in their own folder, so listing them costs +// nothing however many artifacts the session holds. +function announcementFolder(lockPath: string): string { + return `${lockPath}.breakers`; +} + // Returns true when another breaker that is not stale announced itself. // It removes the announcements of stale breakers on the way. Each has a // name that no other writer uses, so removing it by path cannot remove a @@ -327,11 +345,10 @@ async function otherBreakerAnnounced( announcementPath: string, timing: ManifestLockTiming, ): Promise { - const directory = dirname(lockPath); - const prefix = `${basename(lockPath)}.breaker-`; - for (const name of await readdir(directory)) { - const path = join(directory, name); - if (!name.startsWith(prefix) || path === announcementPath) { + const folder = announcementFolder(lockPath); + for (const name of await readdir(folder)) { + const path = join(folder, name); + if (path === announcementPath) { continue; } const state = await readLockFileState(path); @@ -347,7 +364,7 @@ async function otherBreakerAnnounced( } /** - * A breaker first announces itself with `.breaker-` and acts only + * A breaker first announces itself with `.breakers/` and acts only * when no other live breaker announced, so breakers act one at a time: of * two that announce, the one that scans later sees the other. The acting * breaker takes `.break` the same exclusive way, which also keeps out @@ -360,7 +377,7 @@ async function breakStaleManifestLock( announcementPath: string, contents: string, timing: ManifestLockTiming, -): Promise<'retry' | 'held'> { +): Promise<'retry' | 'held' | 'met'> { const judged = await readLockFileState(lockPath); if (judged === null) { return 'retry'; @@ -369,13 +386,29 @@ async function breakStaleManifestLock( return 'held'; } - // The name is this writer's own, so it can already exist only from this - // writer's earlier attempt. - await createWithoutReplacing(temporaryPath, announcementPath); + // mkdir without recursive, so a removed session folder stays removed. + const folder = announcementFolder(lockPath); + await mkdir(folder).catch((error: unknown) => { + if (!hasErrorCode(error, 'EEXIST')) { + throw error; + } + }); try { - if (await otherBreakerAnnounced(lockPath, announcementPath, timing)) { + // The name is this writer's own, so it can already exist only from this + // writer's earlier attempt. + await createWithoutReplacing(temporaryPath, announcementPath); + } catch (error) { + // Another breaker removed the empty folder after the mkdir. The next + // attempt makes it again. + if (hasErrorCode(error, 'ENOENT')) { return 'held'; } + throw error; + } + try { + if (await otherBreakerAnnounced(lockPath, announcementPath, timing)) { + return 'met'; + } const breakPath = `${lockPath}.break`; if (!(await createWithoutReplacing(temporaryPath, breakPath))) { const breaker = await readLockFileState(breakPath); @@ -395,6 +428,8 @@ async function breakStaleManifestLock( return 'retry'; } finally { await rm(announcementPath, { force: true }); + // rmdir removes only an empty folder, so no announcement goes with it. + await rmdir(folder).catch(() => undefined); } } @@ -405,17 +440,23 @@ async function removeOldLockFiles( ): Promise { const directory = dirname(lockPath); const lockName = basename(lockPath); - const names = await readdir(directory).catch((): string[] => []); - for (const name of names) { - const path = join(directory, name); - const breakFile = - name === `${lockName}.break` || name.startsWith(`${lockName}.breaker-`); - if ( - path === temporaryPath || - (!breakFile && !name.startsWith(`${lockName}.tmp-`)) - ) { + const folder = announcementFolder(lockPath); + const paths = [ + ...(await readdir(directory).catch((): string[] => [])) + .filter( + (name) => + name === `${lockName}.break` || name.startsWith(`${lockName}.tmp-`), + ) + .map((name) => join(directory, name)), + ...(await readdir(folder).catch((): string[] => [])).map((name) => + join(folder, name), + ), + ]; + for (const path of paths) { + if (path === temporaryPath) { continue; } + const breakFile = !basename(path).startsWith(`${lockName}.tmp-`); const state = await readLockFileState(path).catch(() => null); if (state === null) { continue; @@ -427,6 +468,7 @@ async function removeOldLockFiles( await removeIfUnchanged(path, state.contents).catch(() => undefined); } } + await rmdir(folder).catch(() => undefined); } // A writer stopped for over unknownOwnerStaleMs while it waits can find that @@ -453,11 +495,12 @@ async function acquireManifestLock( ): Promise { const id = randomUUID(); const temporaryPath = `${lockPath}.tmp-${id}`; - const announcementPath = `${lockPath}.breaker-${id}`; + const announcementPath = join(announcementFolder(lockPath), id); try { await writeFile(temporaryPath, contents, { flag: 'wx', mode: 0o600 }); const deadline = Date.now() + timing.waitMs; let pollMs = 2; + let meetings = 0; let foundTaken = false; for (;;) { await refreshTemporaryFile(temporaryPath, contents); @@ -486,8 +529,16 @@ async function acquireManifestLock( details: { lockPath, holderPid: parseLockOwner(holder)?.pid ?? null }, }); } - if (outcome === 'held') { - // Jitter, so two breakers that saw each other do not meet again. + if (outcome === 'met') { + // Breakers that saw each other all wait. Each meeting doubles the + // wait, and the jitter spreads their next attempts apart. + meetings += 1; + const backoffMs = Math.min( + MEETING_BACKOFF_MS * 2 ** (meetings - 1), + MAX_MEETING_BACKOFF_MS, + ); + await sleep(backoffMs * (0.5 + Math.random())); + } else if (outcome === 'held') { await sleep(pollMs * (0.5 + Math.random())); pollMs = Math.min(pollMs * 2, MAX_LOCK_POLL_MS); } From 0f6abb1abc36f840772247ccf385308ff5567136 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 4 Nov 2026 00:51:30 +0000 Subject: [PATCH 09/11] test(storage): reproduce a waiter that resumes past its deadline while the lock is free A writer stopped while it judges the lock checks its deadline after the attempt and times out on resume, although the owner let go while it was stopped. The verifier's stopped-waiter script hit this once in five runs. The test fails on the current code. --- .../unit/storage/artifactManifestLock.test.ts | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/unit/storage/artifactManifestLock.test.ts b/test/unit/storage/artifactManifestLock.test.ts index 5d3d3fe8..ebc1cfd0 100644 --- a/test/unit/storage/artifactManifestLock.test.ts +++ b/test/unit/storage/artifactManifestLock.test.ts @@ -31,6 +31,7 @@ const fsHooks = vi.hoisted(() => ({ beforeWriteFile: undefined as ((path: string) => Promise) | undefined, beforeUtimes: undefined as ((path: string) => Promise) | undefined, beforeReaddir: undefined as ((path: string) => Promise) | undefined, + beforeLstat: undefined as ((path: string) => Promise) | undefined, })); vi.mock('node:fs/promises', async () => { @@ -55,6 +56,10 @@ vi.mock('node:fs/promises', async () => { ); return actual.writeFile(...args); }, + lstat: async (...args: Parameters) => { + await fsHooks.beforeLstat?.(String(args[0])); + return actual.lstat(...args); + }, readdir: async (...args: Parameters) => { await fsHooks.beforeReaddir?.(String(args[0])); return actual.readdir(...args); @@ -133,6 +138,7 @@ describe('artifact manifest lock', () => { fsHooks.beforeWriteFile = undefined; fsHooks.beforeUtimes = undefined; fsHooks.beforeReaddir = undefined; + fsHooks.beforeLstat = undefined; vi.useRealTimers(); await rm(home, { recursive: true, force: true }); }); @@ -440,6 +446,28 @@ describe('artifact manifest lock', () => { expect(await readdir(join(sessionDirectory, 'artifacts'))).toEqual([]); }); + it('takes a lock that was let go while it was stopped past its deadline', async () => { + // A writer stopped (SIGSTOP) while it judges the lock resumes after its + // deadline. The owner let go in the meantime, so the lock is free. + await writeFile(lockPath, ownerRecord(process.pid, true)); + freezeLockClock(); + fsHooks.beforeLstat = async (path) => { + if (path === lockPath) { + fsHooks.beforeLstat = undefined; + vi.setSystemTime(Date.now() + 60_000); + await rm(lockPath); + } + }; + + await expect( + withArtifactManifestLock( + sessionDirectory, + () => Promise.resolve('ran'), + FAST, + ), + ).resolves.toBe('ran'); + }); + it('writes its temp file again when another writer removed it during the wait', async () => { await writeFile(lockPath, ownerRecord(process.pid, true)); freezeLockClock(); From 38f9fc3ddc3f7ec455f36e5d67c75a3849963df8 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 4 Nov 2026 00:52:04 +0000 Subject: [PATCH 10/11] fix(storage): check the lock deadline right after a failed link, not after the break attempt A writer stopped inside a break attempt resumed past its deadline and threw at once, although the owner had let go in the meantime. The check now comes before the break attempt, so the writer tries the link once more after it resumes. A writer that times out no longer breaks a stale lock on its way out; the next writer does. --- src/storage/artifactManifest.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/storage/artifactManifest.ts b/src/storage/artifactManifest.ts index 4ca30ec2..845b6fd8 100644 --- a/src/storage/artifactManifest.ts +++ b/src/storage/artifactManifest.ts @@ -514,13 +514,8 @@ async function acquireManifestLock( return; } foundTaken = true; - const outcome = await breakStaleManifestLock( - lockPath, - temporaryPath, - announcementPath, - contents, - timing, - ); + // The check comes right after a failed link: a writer stopped past + // its deadline inside the break attempt still tries the link once. if (Date.now() >= deadline) { const holder = await readLockFile(lockPath).catch(() => null); throw makeCliError(ERROR_CODES.STORAGE_WRITE_ERROR, { @@ -529,6 +524,13 @@ async function acquireManifestLock( details: { lockPath, holderPid: parseLockOwner(holder)?.pid ?? null }, }); } + const outcome = await breakStaleManifestLock( + lockPath, + temporaryPath, + announcementPath, + contents, + timing, + ); if (outcome === 'met') { // Breakers that saw each other all wait. Each meeting doubles the // wait, and the jitter spreads their next attempts apart. From d1c9f2dd2c360d97b32fbe07468578c4f3847fd8 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 4 Nov 2026 01:38:27 +0000 Subject: [PATCH 11/11] test(storage): pin the four announcement guards that no test covered The delta verifier of #299 found four guards that survive mutation: - Q2: a link into an announcement folder that another breaker removed after the mkdir fails with ENOENT, and the writer tries again. - Q3: the meeting backoff grows. With a live announcement that stays, a writer lists the folder fewer than 40 times in 1.5 s. - Q7: the sweep removes a dead breaker's announcement and the empty folder after the writer waited for a live lock. - Q8: a breaker removes a dead breaker's announcement and breaks the lock at once. Each test fails when its guard is removed (the verifier's mutate2.py). --- .../unit/storage/artifactManifestLock.test.ts | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/test/unit/storage/artifactManifestLock.test.ts b/test/unit/storage/artifactManifestLock.test.ts index ebc1cfd0..dbd7daf0 100644 --- a/test/unit/storage/artifactManifestLock.test.ts +++ b/test/unit/storage/artifactManifestLock.test.ts @@ -9,6 +9,7 @@ import { readFile, realpath, rm, + rmdir, stat, utimes, writeFile, @@ -468,6 +469,91 @@ describe('artifact manifest lock', () => { ).resolves.toBe('ran'); }); + it('announces itself again when another breaker removed the empty folder before its link', async () => { + const folder = `${lockPath}.breakers`; + await writeFile(lockPath, await deadOwnerRecord()); + freezeLockClock(); + let removed = false; + fsHooks.beforeLink = async (to) => { + if (to.startsWith(`${folder}/`) && !removed) { + removed = true; + await rmdir(folder); + } + }; + + await expect( + withArtifactManifestLock( + sessionDirectory, + () => Promise.resolve('ran'), + FAST, + ), + ).resolves.toBe('ran'); + expect(removed).toBe(true); + expect(await readdir(join(sessionDirectory, 'artifacts'))).toEqual([]); + }); + + it('waits longer each time it meets another breaker', async () => { + // A live breaker's announcement that stays: every attempt meets it. + const folder = `${lockPath}.breakers`; + await writeFile(lockPath, await deadOwnerRecord()); + await mkdir(folder); + await writeFile(join(folder, 'live'), ownerRecord(process.pid, true)); + let listings = 0; + fsHooks.beforeReaddir = (path) => { + if (path === folder) { + listings += 1; + } + return Promise.resolve(); + }; + + await expect( + withArtifactManifestLock(sessionDirectory, () => Promise.resolve(), { + waitMs: 1_500, + unknownOwnerStaleMs: 60_000, + }), + ).rejects.toMatchObject({ code: ERROR_CODES.STORAGE_WRITE_ERROR }); + // A backoff from 5 ms that doubles up to 200 ms allows about 12 + // attempts in 1.5 s. A backoff that stays at 5 ms allows over 100. + expect(listings).toBeGreaterThan(3); + expect(listings).toBeLessThan(40); + }); + + it("removes a dead breaker's announcement and breaks the lock at once", async () => { + const folder = `${lockPath}.breakers`; + await writeFile(lockPath, await deadOwnerRecord()); + await mkdir(folder); + await writeFile(join(folder, 'dead'), await deadOwnerRecord()); + + await expect( + withArtifactManifestLock(sessionDirectory, () => Promise.resolve('ran'), { + waitMs: 2_000, + unknownOwnerStaleMs: 60_000, + }), + ).resolves.toBe('ran'); + expect(await readdir(join(sessionDirectory, 'artifacts'))).toEqual([]); + }); + + it("sweeps a dead breaker's announcement once it had to wait for a live lock", async () => { + // The lock is live, so this writer never breaks and never lists the + // announcements. Only the sweep can remove the dead one. + const folder = `${lockPath}.breakers`; + await writeFile(lockPath, ownerRecord(process.pid, true)); + await mkdir(folder); + await writeFile(join(folder, 'dead'), await deadOwnerRecord()); + freezeLockClock(); + let refreshes = 0; + fsHooks.beforeUtimes = async (path) => { + if (path.startsWith(`${lockPath}.tmp-`) && (refreshes += 1) === 2) { + await rm(lockPath); + } + }; + + await withArtifactManifestLock(sessionDirectory, () => Promise.resolve()); + + expect(refreshes).toBe(2); + expect(await readdir(join(sessionDirectory, 'artifacts'))).toEqual([]); + }); + it('writes its temp file again when another writer removed it during the wait', async () => { await writeFile(lockPath, ownerRecord(process.pid, true)); freezeLockClock();