From dd23e3c2f20bf9a10435b3603121563bee2e674e Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 16 Sep 2026 09:06:56 -0600 Subject: [PATCH 01/11] fix: fence loopback pool lock ownership Co-Authored-By: GPT-5 Codex --- src/loopbackAddressPool.ts | 127 +++++++++++++++----- test/loopback-lock-fencing.test.ts | 185 +++++++++++++++++++++++++++++ 2 files changed, 284 insertions(+), 28 deletions(-) create mode 100644 test/loopback-lock-fencing.test.ts diff --git a/src/loopbackAddressPool.ts b/src/loopbackAddressPool.ts index b7c107e..ebb06eb 100644 --- a/src/loopbackAddressPool.ts +++ b/src/loopbackAddressPool.ts @@ -1,8 +1,8 @@ import { setTimeout as sleep } from 'node:timers/promises'; import { randomBytes } from 'node:crypto'; -import { join } from 'node:path'; +import { basename, dirname, join } from 'node:path'; import { tmpdir } from 'node:os'; -import { open, readFile, rename, stat, unlink, writeFile } from 'node:fs/promises'; +import { lstat, open, readFile, readdir, rename, stat, unlink, writeFile } from 'node:fs/promises'; import { createServer } from 'node:net'; import { findAcceptingPort } from './portUtils.ts'; @@ -29,6 +29,9 @@ const HARPER_LOOPBACK_POOL_LOCK_PATH = join(tmpdir(), 'harper-integration-test-l // Constants for timeouts and retries const LOCK_STALE_TIMEOUT_MS = 10000; const RETRY_DELAY_MS = 1000; +const PENDING_FILE_STALE_TIMEOUT_MS = LOCK_STALE_TIMEOUT_MS; + +let lockTokenCounter = 0; // How long an unusable pool file must go unmodified before it is reinitialized. The reservations it // lost may not be bound yet, which the conflict canary can't see, so this must outlast startHarper's @@ -139,6 +142,13 @@ async function releaseRefusedAddress(address: string): Promise { } } +class LockOwnershipLostError extends Error { + constructor(cause?: unknown) { + super('Loopback address pool lock ownership was lost before publishing', { cause }); + this.name = 'LockOwnershipLostError'; + } +} + /** * Acquires a file-based lock by creating the lock file. This enables safe concurrent * access to the loopback pool across multiple test processes. @@ -147,16 +157,21 @@ async function releaseRefusedAddress(address: string): Promise { * a simple but effective cross-process mutex. Handles stale locks by removing lock files * older than LOCK_STALE_TIMEOUT_MS (10 seconds). * - * @returns A promise that resolves when the lock is acquired + * @returns The token that identifies this lock acquisition and whether it reclaimed a stale lock */ -async function acquireLock(): Promise { +async function acquireLock(): Promise<{ token: string; reclaimedStaleLock: boolean }> { + const token = `${process.pid}-${++lockTokenCounter}-${randomBytes(8).toString('hex')}`; + let reclaimedStaleLock = false; while (true) { try { // The 'wx' flag causes the open to fail if the file already exists const lockFileHandle = await open(HARPER_LOOPBACK_POOL_LOCK_PATH, 'wx'); - // We have the lock - close the handle as we don't intend to write to it - await lockFileHandle.close(); - return; + try { + await lockFileHandle.writeFile(token); + } finally { + await lockFileHandle.close(); + } + return { token, reclaimedStaleLock }; } catch (error) { // If the lock file already exists, it's either stale or we wait for it to be released if ((error as NodeJS.ErrnoException).code === 'EEXIST') { @@ -165,6 +180,7 @@ async function acquireLock(): Promise { // If the lock file is older than the timeout, consider it stale and remove it if (Date.now() - lockFileStat.mtimeMs > LOCK_STALE_TIMEOUT_MS) { await unlink(HARPER_LOOPBACK_POOL_LOCK_PATH); + reclaimedStaleLock = true; } } catch { // Lock file may have been removed by another process, continue @@ -181,16 +197,53 @@ async function acquireLock(): Promise { } /** - * Releases the file-based lock by deleting the lock file. + * Releases the file-based lock if it still belongs to this acquisition. */ -async function releaseLock(): Promise { +async function releaseLock(token: string): Promise { try { - await unlink(HARPER_LOOPBACK_POOL_LOCK_PATH); + if ((await readFile(HARPER_LOOPBACK_POOL_LOCK_PATH, 'utf-8')) === token) { + await unlink(HARPER_LOOPBACK_POOL_LOCK_PATH); + } } catch { // Ignore errors if lock file is already gone } } +async function assertLockHeld(token: string): Promise { + let currentToken: string; + try { + currentToken = await readFile(HARPER_LOOPBACK_POOL_LOCK_PATH, 'utf-8'); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') throw new LockOwnershipLostError(error); + throw error; + } + if (currentToken !== token) throw new LockOwnershipLostError(); +} + +export async function sweepStalePendingPoolFiles( + poolPath: string = HARPER_LOOPBACK_POOL_PATH, + now: number = Date.now() +): Promise { + let fileNames: string[]; + try { + fileNames = await readdir(dirname(poolPath)); + } catch { + return; + } + const pendingPrefix = `${basename(poolPath)}.`; + for (const fileName of fileNames) { + if (!fileName.startsWith(pendingPrefix)) continue; + if (!/^\d+\.\d+\.[0-9a-f]+\.pending$/.test(fileName.slice(pendingPrefix.length))) continue; + const pendingPath = join(dirname(poolPath), fileName); + try { + const pendingStat = await lstat(pendingPath); + if (now - pendingStat.mtimeMs > PENDING_FILE_STALE_TIMEOUT_MS) await unlink(pendingPath); + } catch { + // Cleanup is best-effort; a concurrent removal or foreign file must not fail allocation. + } + } +} + /** * Executes a callback function while holding the lock. Automatically acquires * and releases the lock, ensuring the lock is always released even if the callback @@ -199,12 +252,17 @@ async function releaseLock(): Promise { * @param callback The async function to execute while holding the lock * @returns The result of the callback function */ -async function withLock(callback: () => Promise): Promise { - await acquireLock(); - try { - return await callback(); - } finally { - await releaseLock(); +export async function withLock(callback: (lockToken: string) => Promise): Promise { + while (true) { + const { token, reclaimedStaleLock } = await acquireLock(); + try { + if (reclaimedStaleLock) await sweepStalePendingPoolFiles(); + return await callback(token); + } catch (error) { + if (!(error instanceof LockOwnershipLostError)) throw error; + } finally { + await releaseLock(token); + } } } @@ -229,7 +287,10 @@ function emptyPool(): LoopbackPool { * @param poolPath The pool file path (overridable for tests; defaults to the shared pool file) * @returns The loopback pool array, or `null` while the pool file is quarantined */ -export async function readPoolFile(poolPath: string = HARPER_LOOPBACK_POOL_PATH): Promise { +export async function readPoolFile( + poolPath: string = HARPER_LOOPBACK_POOL_PATH, + lockToken?: string +): Promise { let problem: string; try { const parsed: unknown = JSON.parse(await readFile(poolPath, 'utf-8')); @@ -264,7 +325,7 @@ export async function readPoolFile(poolPath: string = HARPER_LOOPBACK_POOL_PATH) quarantinedPoolPaths.delete(poolPath); console.warn(`[loopback-pool] ${poolPath} has been unusable for ${Math.round(unmodifiedMs / 1000)}s (${problem}); reinitializing.`); const pool = emptyPool(); - await writePoolFile(pool, poolPath); + await writePoolFile(pool, poolPath, lockToken); return pool; } @@ -280,7 +341,11 @@ let pendingWriteCounter = 0; * @param pool The loopback pool array to persist * @param poolPath The pool file path (overridable for tests; defaults to the shared pool file) */ -export async function writePoolFile(pool: LoopbackPool, poolPath: string = HARPER_LOOPBACK_POOL_PATH): Promise { +export async function writePoolFile( + pool: LoopbackPool, + poolPath: string = HARPER_LOOPBACK_POOL_PATH, + lockToken?: string +): Promise { const pendingPath = `${poolPath}.${process.pid}.${++pendingWriteCounter}.${randomBytes(8).toString('hex')}.pending`; try { const pendingFileHandle = await open(pendingPath, 'wx'); @@ -289,6 +354,12 @@ export async function writePoolFile(pool: LoopbackPool, poolPath: string = HARPE } finally { await pendingFileHandle.close(); } + if (poolPath === HARPER_LOOPBACK_POOL_PATH) { + if (!lockToken) throw new Error('A lock token is required to publish the shared loopback pool'); + // Check only after the pending file closes and immediately before rename. A takeover after + // this point can still publish a stale snapshot; the token narrows but does not close that race. + await assertLockHeld(lockToken); + } await rename(pendingPath, poolPath); } catch (error) { await unlink(pendingPath).catch(() => {}); @@ -490,9 +561,9 @@ export async function getNextAvailableLoopbackAddress(): Promise { // the lingering node has had time to exit. const triedIndices = new Set(); while (true) { - const assignedIndex = await withLock(async () => { + const assignedIndex = await withLock(async (lockToken) => { // Read the pool file - const loopbackPool = await readPoolFile(); + const loopbackPool = await readPoolFile(HARPER_LOOPBACK_POOL_PATH, lockToken); if (!loopbackPool) return null; // Find the first available index we haven't already found in-use this attempt @@ -512,7 +583,7 @@ export async function getNextAvailableLoopbackAddress(): Promise { loopbackPool[index] = process.pid; } // Write the updated pool back to the file - await writePoolFile(loopbackPool); + await writePoolFile(loopbackPool, HARPER_LOOPBACK_POOL_PATH, lockToken); return index; }); @@ -624,16 +695,16 @@ export async function releaseLoopbackAddress(address: string): Promise { // Validate and parse the address const index = parseLoopbackAddress(address); - await withLock(async () => { + await withLock(async (lockToken) => { // Read the pool file - const loopbackPool = await readPoolFile(); + const loopbackPool = await readPoolFile(HARPER_LOOPBACK_POOL_PATH, lockToken); if (!loopbackPool || loopbackPool[index] !== process.pid) return; // Release the address by setting it to null loopbackPool[index] = null; // Write the updated pool back to the file - await writePoolFile(loopbackPool); + await writePoolFile(loopbackPool, HARPER_LOOPBACK_POOL_PATH, lockToken); }); } @@ -642,9 +713,9 @@ export async function releaseLoopbackAddress(address: string): Promise { * Useful for cleanup during graceful shutdown. */ export async function releaseAllLoopbackAddressesForCurrentProcess(): Promise { - await withLock(async () => { + await withLock(async (lockToken) => { // Read the pool file - const loopbackPool = await readPoolFile(); + const loopbackPool = await readPoolFile(HARPER_LOOPBACK_POOL_PATH, lockToken); if (!loopbackPool) return; // Find and release all addresses assigned to this process @@ -657,6 +728,6 @@ export async function releaseAllLoopbackAddressesForCurrentProcess(): Promise; resolve: () => void } { + let resolve!: () => void; + const promise = new Promise((res) => { + resolve = res; + }); + return { promise, resolve }; +} + +function restoreEnv(name: string, value: string | undefined): void { + if (value === undefined) delete process.env[name]; + else process.env[name] = value; +} + +function createAvailableServer() { + const emitter = new EventEmitter(); + const server = { + once(event: string, listener: (...args: unknown[]) => void) { + emitter.once(event, listener); + return server; + }, + off(event: string, listener: (...args: unknown[]) => void) { + emitter.off(event, listener); + return server; + }, + listen(...args: unknown[]) { + const listener = args.at(-1) as () => void; + queueMicrotask(listener); + return server; + }, + close(listener: () => void) { + queueMicrotask(listener); + return server; + }, + }; + return server; +} + +test('a writer superseded after reading cannot erase the successor claim', async () => { + const isolatedTmpDir = await mkdtemp(join(tmpdir(), 'loopback-lock-fencing-')); + const poolPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.json'); + const lockPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.lock'); + const orphanedPendingPath = `${poolPath}.123.1.abcdef0123456789.pending`; + const previousEnv = new Map( + [ + 'TMPDIR', + 'TMP', + 'TEMP', + 'HARPER_INTEGRATION_TEST_LOOPBACK_POOL_START', + 'HARPER_INTEGRATION_TEST_LOOPBACK_POOL_COUNT', + 'HARPER_INTEGRATION_TEST_CONFLICT_PROBE_PORT', + 'HARPER_INTEGRATION_TEST_HTTP_CONFLICT_PROBE_PORT', + ].map((name) => [name, process.env[name]]) + ); + process.env.TMPDIR = isolatedTmpDir; + process.env.TMP = isolatedTmpDir; + process.env.TEMP = isolatedTmpDir; + process.env.HARPER_INTEGRATION_TEST_LOOPBACK_POOL_START = '2'; + process.env.HARPER_INTEGRATION_TEST_LOOPBACK_POOL_COUNT = '3'; + process.env.HARPER_INTEGRATION_TEST_CONFLICT_PROBE_PORT = '0'; + process.env.HARPER_INTEGRATION_TEST_HTTP_CONFLICT_PROBE_PORT = '0'; + + const staleSnapshotRead = deferred(); + const resumeStaleWriter = deferred(); + let pauseFirstPoolRead = true; + const fsMock = mock.module('node:fs/promises', { + namedExports: { + ...fsPromises, + readFile: async (path: string, encoding: BufferEncoding) => { + const contents = await readFile(path, encoding); + if (path === poolPath && pauseFirstPoolRead) { + pauseFirstPoolRead = false; + staleSnapshotRead.resolve(); + await resumeStaleWriter.promise; + } + return contents; + }, + }, + }); + const netMock = mock.module('node:net', { + namedExports: { + ...net, + createServer: createAvailableServer, + }, + }); + + let staleWriter: Promise | undefined; + try { + await writeFile(poolPath, JSON.stringify([null, null, null])); + const { getNextAvailableLoopbackAddress } = await import(freshModuleUrl()); + staleWriter = getNextAvailableLoopbackAddress(); + await staleSnapshotRead.promise; + + await writeFile(poolPath, JSON.stringify([process.ppid, null, null])); + const staleTime = new Date(Date.now() - 20000); + await utimes(lockPath, staleTime, staleTime); + await writeFile(orphanedPendingPath, 'orphaned'); + await utimes(orphanedPendingPath, staleTime, staleTime); + + const successorAddress = await getNextAvailableLoopbackAddress(); + strictEqual(successorAddress, '127.0.0.3'); + await rejects(readFile(orphanedPendingPath), (error: NodeJS.ErrnoException) => error.code === 'ENOENT'); + resumeStaleWriter.resolve(); + + const staleWriterAddress = await staleWriter; + notStrictEqual(staleWriterAddress, successorAddress); + deepStrictEqual(JSON.parse(await readFile(poolPath, 'utf-8')), [process.ppid, process.pid, process.pid]); + } finally { + resumeStaleWriter.resolve(); + await staleWriter?.catch(() => {}); + fsMock.restore(); + netMock.restore(); + for (const [name, value] of previousEnv) restoreEnv(name, value); + await rm(isolatedTmpDir, { recursive: true, force: true }); + } +}); + +test('a superseded holder does not unlink the replacement lock', async () => { + const isolatedTmpDir = await mkdtemp(join(tmpdir(), 'loopback-lock-release-')); + const lockPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.lock'); + const previousTmpEnv = new Map(['TMPDIR', 'TMP', 'TEMP'].map((name) => [name, process.env[name]])); + for (const name of previousTmpEnv.keys()) process.env[name] = isolatedTmpDir; + const sectionEntered = deferred(); + const resumeSection = deferred(); + try { + const { withLock } = await import(freshModuleUrl()); + const staleHolder = withLock(async () => { + sectionEntered.resolve(); + await resumeSection.promise; + }); + await sectionEntered.promise; + await writeFile(lockPath, 'replacement-holder'); + resumeSection.resolve(); + await staleHolder; + + strictEqual(await readFile(lockPath, 'utf-8'), 'replacement-holder'); + } finally { + resumeSection.resolve(); + for (const [name, value] of previousTmpEnv) restoreEnv(name, value); + await rm(isolatedTmpDir, { recursive: true, force: true }); + } +}); + +test('pending-file cleanup removes only old pool pending files', async () => { + const isolatedTmpDir = await mkdtemp(join(tmpdir(), 'loopback-pending-sweep-')); + const poolPath = join(isolatedTmpDir, 'pool.json'); + const oldPending = `${poolPath}.123.1.abcdef0123456789.pending`; + const freshPending = `${poolPath}.123.2.abcdef0123456789.pending`; + const unrelatedPending = `${poolPath}.not-a-writer.pending`; + const now = Date.now(); + try { + await Promise.all([ + writeFile(oldPending, 'old'), + writeFile(freshPending, 'fresh'), + writeFile(unrelatedPending, 'unrelated'), + ]); + const staleTime = new Date(now - 10001); + await utimes(oldPending, staleTime, staleTime); + + const { sweepStalePendingPoolFiles } = await import(freshModuleUrl()); + await sweepStalePendingPoolFiles(poolPath, now); + + deepStrictEqual( + (await readdir(isolatedTmpDir)).sort(), + [freshPending, unrelatedPending].map((path) => basename(path)).sort() + ); + } finally { + await unlink(oldPending).catch(() => {}); + await rm(isolatedTmpDir, { recursive: true, force: true }); + } +}); From c3b50e994e2bf8c8b017a16b212bd242c2f1f5b9 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 16 Sep 2026 09:13:18 -0600 Subject: [PATCH 02/11] test: cover replacement lock ownership Co-Authored-By: GPT-5 Codex --- CONTRIBUTING.md | 4 ++-- src/loopbackAddressPool.ts | 14 +++++--------- test/loopback-lock-fencing.test.ts | 5 +++-- 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5d22b90..a97ef93 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -22,10 +22,10 @@ There are two `tsconfig` files: ```sh npm run check # Type-check only (no output) npm run build # Compile src/ → dist/ -npm test # Run the tests (node:test over test/**/*.test.ts) +npm test # Run the tests (node:test with module mocks enabled over test/**/*.test.ts) ``` -Tests live in `test/` (separate from `src/`) and run on the built-in Node.js test runner via `npm test`. They execute the `.ts` files directly using Node's native type stripping, which requires **Node 22.18+** (reflected in `devEngines`). Because they live outside `src/`, the published build (which emits only `src/` → `dist/`) never includes them. `tsconfig.json` type-checks both `src/` and `test/`; `tsconfig.build.json` narrows the build to `src/`. Beyond these tests, validation also includes type-checking and manual testing via dependent projects. +Tests live in `test/` (separate from `src/`) and run on the built-in Node.js test runner via `npm test`, with `--experimental-test-module-mocks` enabled for isolated fault injection. Node prints an expected `ExperimentalWarning` while that API remains experimental. Tests execute the `.ts` files directly using Node's native type stripping, which requires **Node 22.18+** (reflected in `devEngines`). Because they live outside `src/`, the published build (which emits only `src/` → `dist/`) never includes them. `tsconfig.json` type-checks both `src/` and `test/`; `tsconfig.build.json` narrows the build to `src/`. Beyond these tests, validation also includes type-checking and manual testing via dependent projects. Internal-only helpers may be exported from their modules for testing (e.g. `runHarperCommand` in `harperLifecycle.ts`) but are deliberately **not** re-exported from `src/index.ts`, keeping them out of the public API. diff --git a/src/loopbackAddressPool.ts b/src/loopbackAddressPool.ts index ebb06eb..c4ef8ea 100644 --- a/src/loopbackAddressPool.ts +++ b/src/loopbackAddressPool.ts @@ -204,9 +204,7 @@ async function releaseLock(token: string): Promise { if ((await readFile(HARPER_LOOPBACK_POOL_LOCK_PATH, 'utf-8')) === token) { await unlink(HARPER_LOOPBACK_POOL_LOCK_PATH); } - } catch { - // Ignore errors if lock file is already gone - } + } catch {} } async function assertLockHeld(token: string): Promise { @@ -238,16 +236,14 @@ export async function sweepStalePendingPoolFiles( try { const pendingStat = await lstat(pendingPath); if (now - pendingStat.mtimeMs > PENDING_FILE_STALE_TIMEOUT_MS) await unlink(pendingPath); - } catch { - // Cleanup is best-effort; a concurrent removal or foreign file must not fail allocation. - } + } catch {} } } /** - * Executes a callback function while holding the lock. Automatically acquires - * and releases the lock, ensuring the lock is always released even if the callback - * throws an error. + * Executes a callback while holding the lock and releases only the lock this call acquired. + * A section superseded before publication is rerun from fresh pool state so teardown callers + * do not inherit a transient lost-lock failure. * * @param callback The async function to execute while holding the lock * @returns The result of the callback function diff --git a/test/loopback-lock-fencing.test.ts b/test/loopback-lock-fencing.test.ts index d364a0d..ede5945 100644 --- a/test/loopback-lock-fencing.test.ts +++ b/test/loopback-lock-fencing.test.ts @@ -2,7 +2,7 @@ import test, { mock } from 'node:test'; import { deepStrictEqual, notStrictEqual, rejects, strictEqual } from 'node:assert'; import { EventEmitter } from 'node:events'; import * as fsPromises from 'node:fs/promises'; -import { mkdtemp, readdir, readFile, rm, unlink, utimes, writeFile } from 'node:fs/promises'; +import { mkdtemp, readdir, readFile, rm, utimes, writeFile } from 'node:fs/promises'; import * as net from 'node:net'; import { tmpdir } from 'node:os'; import { basename, join } from 'node:path'; @@ -114,6 +114,8 @@ test('a writer superseded after reading cannot erase the successor claim', async const successorAddress = await getNextAvailableLoopbackAddress(); strictEqual(successorAddress, '127.0.0.3'); await rejects(readFile(orphanedPendingPath), (error: NodeJS.ErrnoException) => error.code === 'ENOENT'); + await writeFile(lockPath, 'replacement-holder'); + await utimes(lockPath, staleTime, staleTime); resumeStaleWriter.resolve(); const staleWriterAddress = await staleWriter; @@ -179,7 +181,6 @@ test('pending-file cleanup removes only old pool pending files', async () => { [freshPending, unrelatedPending].map((path) => basename(path)).sort() ); } finally { - await unlink(oldPending).catch(() => {}); await rm(isolatedTmpDir, { recursive: true, force: true }); } }); From f1690843b55fd7e21311893e6b5ed32adaebc56c Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 16 Sep 2026 09:22:29 -0600 Subject: [PATCH 03/11] fix: clean up failed lock token writes Co-Authored-By: GPT-5 Codex --- src/loopbackAddressPool.ts | 7 +++++-- test/loopback-lock-fencing.test.ts | 31 ++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/loopbackAddressPool.ts b/src/loopbackAddressPool.ts index c4ef8ea..85dd9a7 100644 --- a/src/loopbackAddressPool.ts +++ b/src/loopbackAddressPool.ts @@ -168,9 +168,12 @@ async function acquireLock(): Promise<{ token: string; reclaimedStaleLock: boole const lockFileHandle = await open(HARPER_LOOPBACK_POOL_LOCK_PATH, 'wx'); try { await lockFileHandle.writeFile(token); - } finally { - await lockFileHandle.close(); + } catch (error) { + await lockFileHandle.close().catch(() => {}); + await unlink(HARPER_LOOPBACK_POOL_LOCK_PATH).catch(() => {}); + throw error; } + await lockFileHandle.close(); return { token, reclaimedStaleLock }; } catch (error) { // If the lock file already exists, it's either stale or we wait for it to be released diff --git a/test/loopback-lock-fencing.test.ts b/test/loopback-lock-fencing.test.ts index ede5945..b79ed6f 100644 --- a/test/loopback-lock-fencing.test.ts +++ b/test/loopback-lock-fencing.test.ts @@ -157,6 +157,37 @@ test('a superseded holder does not unlink the replacement lock', async () => { } }); +test('a failed token write does not leave an empty lock file', async () => { + const isolatedTmpDir = await mkdtemp(join(tmpdir(), 'loopback-lock-write-failure-')); + const lockPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.lock'); + const previousTmpEnv = new Map(['TMPDIR', 'TMP', 'TEMP'].map((name) => [name, process.env[name]])); + for (const name of previousTmpEnv.keys()) process.env[name] = isolatedTmpDir; + const fsMock = mock.module('node:fs/promises', { + namedExports: { + ...fsPromises, + open: async (path: string, flags: string) => { + const fileHandle = await fsPromises.open(path, flags); + if (path !== lockPath) return fileHandle; + return { + close: () => fileHandle.close(), + writeFile: async () => { + throw new Error('simulated token write failure'); + }, + }; + }, + }, + }); + try { + const { withLock } = await import(freshModuleUrl()); + await rejects(withLock(async () => {}), /simulated token write failure/); + await rejects(readFile(lockPath), (error: NodeJS.ErrnoException) => error.code === 'ENOENT'); + } finally { + fsMock.restore(); + for (const [name, value] of previousTmpEnv) restoreEnv(name, value); + await rm(isolatedTmpDir, { recursive: true, force: true }); + } +}); + test('pending-file cleanup removes only old pool pending files', async () => { const isolatedTmpDir = await mkdtemp(join(tmpdir(), 'loopback-pending-sweep-')); const poolPath = join(isolatedTmpDir, 'pool.json'); From cc2628e8c8874d5ba88c00ad6fa1b16359d6166e Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 1 Oct 2026 11:12:13 -0600 Subject: [PATCH 04/11] fix: drop the pending-file sweep until a writer produces pending files Nothing on this branch writes `....pending` files: the pool is still written in place, and the pending-then-rename writer lives in the separate atomic-publication PR. The sweep was matching a filename no production code produces, verified only against hand-made fixtures, and its naming contract with that PR was untested. A reaper belongs with the writer that leaks the files. Co-Authored-By: Claude Opus 5.5 Dispatch-Task: fix-kriszyp_integration-testing_33-33b5cb8c --- src/loopbackAddressPool.ts | 38 +++++------------------------- test/loopback-lock-fencing.test.ts | 36 ++-------------------------- 2 files changed, 8 insertions(+), 66 deletions(-) diff --git a/src/loopbackAddressPool.ts b/src/loopbackAddressPool.ts index 85dd9a7..031a06a 100644 --- a/src/loopbackAddressPool.ts +++ b/src/loopbackAddressPool.ts @@ -1,8 +1,8 @@ import { setTimeout as sleep } from 'node:timers/promises'; import { randomBytes } from 'node:crypto'; -import { basename, dirname, join } from 'node:path'; +import { join } from 'node:path'; import { tmpdir } from 'node:os'; -import { lstat, open, readFile, readdir, rename, stat, unlink, writeFile } from 'node:fs/promises'; +import { open, readFile, rename, stat, unlink, writeFile } from 'node:fs/promises'; import { createServer } from 'node:net'; import { findAcceptingPort } from './portUtils.ts'; @@ -29,7 +29,6 @@ const HARPER_LOOPBACK_POOL_LOCK_PATH = join(tmpdir(), 'harper-integration-test-l // Constants for timeouts and retries const LOCK_STALE_TIMEOUT_MS = 10000; const RETRY_DELAY_MS = 1000; -const PENDING_FILE_STALE_TIMEOUT_MS = LOCK_STALE_TIMEOUT_MS; let lockTokenCounter = 0; @@ -157,11 +156,10 @@ class LockOwnershipLostError extends Error { * a simple but effective cross-process mutex. Handles stale locks by removing lock files * older than LOCK_STALE_TIMEOUT_MS (10 seconds). * - * @returns The token that identifies this lock acquisition and whether it reclaimed a stale lock + * @returns The token that identifies this lock acquisition */ -async function acquireLock(): Promise<{ token: string; reclaimedStaleLock: boolean }> { +async function acquireLock(): Promise { const token = `${process.pid}-${++lockTokenCounter}-${randomBytes(8).toString('hex')}`; - let reclaimedStaleLock = false; while (true) { try { // The 'wx' flag causes the open to fail if the file already exists @@ -174,7 +172,7 @@ async function acquireLock(): Promise<{ token: string; reclaimedStaleLock: boole throw error; } await lockFileHandle.close(); - return { token, reclaimedStaleLock }; + return token; } catch (error) { // If the lock file already exists, it's either stale or we wait for it to be released if ((error as NodeJS.ErrnoException).code === 'EEXIST') { @@ -183,7 +181,6 @@ async function acquireLock(): Promise<{ token: string; reclaimedStaleLock: boole // If the lock file is older than the timeout, consider it stale and remove it if (Date.now() - lockFileStat.mtimeMs > LOCK_STALE_TIMEOUT_MS) { await unlink(HARPER_LOOPBACK_POOL_LOCK_PATH); - reclaimedStaleLock = true; } } catch { // Lock file may have been removed by another process, continue @@ -221,28 +218,6 @@ async function assertLockHeld(token: string): Promise { if (currentToken !== token) throw new LockOwnershipLostError(); } -export async function sweepStalePendingPoolFiles( - poolPath: string = HARPER_LOOPBACK_POOL_PATH, - now: number = Date.now() -): Promise { - let fileNames: string[]; - try { - fileNames = await readdir(dirname(poolPath)); - } catch { - return; - } - const pendingPrefix = `${basename(poolPath)}.`; - for (const fileName of fileNames) { - if (!fileName.startsWith(pendingPrefix)) continue; - if (!/^\d+\.\d+\.[0-9a-f]+\.pending$/.test(fileName.slice(pendingPrefix.length))) continue; - const pendingPath = join(dirname(poolPath), fileName); - try { - const pendingStat = await lstat(pendingPath); - if (now - pendingStat.mtimeMs > PENDING_FILE_STALE_TIMEOUT_MS) await unlink(pendingPath); - } catch {} - } -} - /** * Executes a callback while holding the lock and releases only the lock this call acquired. * A section superseded before publication is rerun from fresh pool state so teardown callers @@ -253,9 +228,8 @@ export async function sweepStalePendingPoolFiles( */ export async function withLock(callback: (lockToken: string) => Promise): Promise { while (true) { - const { token, reclaimedStaleLock } = await acquireLock(); + const token = await acquireLock(); try { - if (reclaimedStaleLock) await sweepStalePendingPoolFiles(); return await callback(token); } catch (error) { if (!(error instanceof LockOwnershipLostError)) throw error; diff --git a/test/loopback-lock-fencing.test.ts b/test/loopback-lock-fencing.test.ts index b79ed6f..8ee5c5b 100644 --- a/test/loopback-lock-fencing.test.ts +++ b/test/loopback-lock-fencing.test.ts @@ -2,10 +2,10 @@ import test, { mock } from 'node:test'; import { deepStrictEqual, notStrictEqual, rejects, strictEqual } from 'node:assert'; import { EventEmitter } from 'node:events'; import * as fsPromises from 'node:fs/promises'; -import { mkdtemp, readdir, readFile, rm, utimes, writeFile } from 'node:fs/promises'; +import { mkdtemp, readFile, rm, utimes, writeFile } from 'node:fs/promises'; import * as net from 'node:net'; import { tmpdir } from 'node:os'; -import { basename, join } from 'node:path'; +import { join } from 'node:path'; let freshImportCounter = 0; @@ -54,7 +54,6 @@ test('a writer superseded after reading cannot erase the successor claim', async const isolatedTmpDir = await mkdtemp(join(tmpdir(), 'loopback-lock-fencing-')); const poolPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.json'); const lockPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.lock'); - const orphanedPendingPath = `${poolPath}.123.1.abcdef0123456789.pending`; const previousEnv = new Map( [ 'TMPDIR', @@ -108,12 +107,9 @@ test('a writer superseded after reading cannot erase the successor claim', async await writeFile(poolPath, JSON.stringify([process.ppid, null, null])); const staleTime = new Date(Date.now() - 20000); await utimes(lockPath, staleTime, staleTime); - await writeFile(orphanedPendingPath, 'orphaned'); - await utimes(orphanedPendingPath, staleTime, staleTime); const successorAddress = await getNextAvailableLoopbackAddress(); strictEqual(successorAddress, '127.0.0.3'); - await rejects(readFile(orphanedPendingPath), (error: NodeJS.ErrnoException) => error.code === 'ENOENT'); await writeFile(lockPath, 'replacement-holder'); await utimes(lockPath, staleTime, staleTime); resumeStaleWriter.resolve(); @@ -187,31 +183,3 @@ test('a failed token write does not leave an empty lock file', async () => { await rm(isolatedTmpDir, { recursive: true, force: true }); } }); - -test('pending-file cleanup removes only old pool pending files', async () => { - const isolatedTmpDir = await mkdtemp(join(tmpdir(), 'loopback-pending-sweep-')); - const poolPath = join(isolatedTmpDir, 'pool.json'); - const oldPending = `${poolPath}.123.1.abcdef0123456789.pending`; - const freshPending = `${poolPath}.123.2.abcdef0123456789.pending`; - const unrelatedPending = `${poolPath}.not-a-writer.pending`; - const now = Date.now(); - try { - await Promise.all([ - writeFile(oldPending, 'old'), - writeFile(freshPending, 'fresh'), - writeFile(unrelatedPending, 'unrelated'), - ]); - const staleTime = new Date(now - 10001); - await utimes(oldPending, staleTime, staleTime); - - const { sweepStalePendingPoolFiles } = await import(freshModuleUrl()); - await sweepStalePendingPoolFiles(poolPath, now); - - deepStrictEqual( - (await readdir(isolatedTmpDir)).sort(), - [freshPending, unrelatedPending].map((path) => basename(path)).sort() - ); - } finally { - await rm(isolatedTmpDir, { recursive: true, force: true }); - } -}); From fbf381b7e913fb583526fc83f30e965059cd8db9 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 1 Oct 2026 11:12:51 -0600 Subject: [PATCH 05/11] fix: stop retrying a pool update that keeps losing the lock A section whose lock is taken over as stale before it publishes now warns and reruns at most three times, then throws LockOwnershipLostError. A one-off stall past the 10s stale threshold still recovers transparently; repeated takeovers (fresh locks judged stale through clock or mtime skew) now fail loudly instead of wedging teardown with no output. Also documents that the ownership check narrows, but does not close, the check-then-write window. Co-Authored-By: Claude Opus 5.5 Dispatch-Task: fix-kriszyp_integration-testing_33-33b5cb8c --- src/loopbackAddressPool.ts | 22 +++++++++++++---- test/loopback-lock-fencing.test.ts | 38 +++++++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/src/loopbackAddressPool.ts b/src/loopbackAddressPool.ts index 031a06a..64362b2 100644 --- a/src/loopbackAddressPool.ts +++ b/src/loopbackAddressPool.ts @@ -29,6 +29,9 @@ const HARPER_LOOPBACK_POOL_LOCK_PATH = join(tmpdir(), 'harper-integration-test-l // Constants for timeouts and retries const LOCK_STALE_TIMEOUT_MS = 10000; const RETRY_DELAY_MS = 1000; +// A one-off stall past LOCK_STALE_TIMEOUT_MS loses the lock once; losing it repeatedly means another +// process keeps judging fresh locks stale (clock or mtime skew), which retrying cannot fix. +const LOCK_OWNERSHIP_RETRIES = 3; let lockTokenCounter = 0; @@ -143,7 +146,10 @@ async function releaseRefusedAddress(address: string): Promise { class LockOwnershipLostError extends Error { constructor(cause?: unknown) { - super('Loopback address pool lock ownership was lost before publishing', { cause }); + super( + `Another process took over the loopback pool lock as stale (older than ${LOCK_STALE_TIMEOUT_MS}ms) before this process published its update`, + { cause } + ); this.name = 'LockOwnershipLostError'; } } @@ -220,19 +226,23 @@ async function assertLockHeld(token: string): Promise { /** * Executes a callback while holding the lock and releases only the lock this call acquired. - * A section superseded before publication is rerun from fresh pool state so teardown callers - * do not inherit a transient lost-lock failure. + * A section whose lock was taken over as stale before it published is rerun from fresh pool + * state, up to LOCK_OWNERSHIP_RETRIES times, so teardown callers do not inherit a one-off + * lost-lock failure. * * @param callback The async function to execute while holding the lock * @returns The result of the callback function */ export async function withLock(callback: (lockToken: string) => Promise): Promise { - while (true) { + for (let attempt = 1; ; attempt++) { const token = await acquireLock(); try { return await callback(token); } catch (error) { - if (!(error instanceof LockOwnershipLostError)) throw error; + if (!(error instanceof LockOwnershipLostError) || attempt > LOCK_OWNERSHIP_RETRIES) throw error; + console.warn( + `[loopback-pool] ${error.message}; retrying from the current pool state (retry ${attempt} of ${LOCK_OWNERSHIP_RETRIES}).` + ); } finally { await releaseLock(token); } @@ -310,6 +320,8 @@ let pendingWriteCounter = 0; * Publishes by write-to-temp-then-rename, so readers only ever see the old complete file or * the new complete file. The pending name must be unpredictable, not just unique: `wx` refuses * a file already at that name, so a guessable one could be pre-planted to make every write fail. + * The ownership check must stay after the pending file closes and immediately before rename. It narrows, + * but does not close, the window in which a takeover lets a stale holder publish over its successor. * * @param pool The loopback pool array to persist * @param poolPath The pool file path (overridable for tests; defaults to the shared pool file) diff --git a/test/loopback-lock-fencing.test.ts b/test/loopback-lock-fencing.test.ts index 8ee5c5b..2fd5cad 100644 --- a/test/loopback-lock-fencing.test.ts +++ b/test/loopback-lock-fencing.test.ts @@ -2,7 +2,7 @@ import test, { mock } from 'node:test'; import { deepStrictEqual, notStrictEqual, rejects, strictEqual } from 'node:assert'; import { EventEmitter } from 'node:events'; import * as fsPromises from 'node:fs/promises'; -import { mkdtemp, readFile, rm, utimes, writeFile } from 'node:fs/promises'; +import { mkdtemp, readFile, rm, unlink, utimes, writeFile } from 'node:fs/promises'; import * as net from 'node:net'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -96,6 +96,7 @@ test('a writer superseded after reading cannot erase the successor claim', async createServer: createAvailableServer, }, }); + const warn = mock.method(console, 'warn', () => {}); let staleWriter: Promise | undefined; try { @@ -117,11 +118,13 @@ test('a writer superseded after reading cannot erase the successor claim', async const staleWriterAddress = await staleWriter; notStrictEqual(staleWriterAddress, successorAddress); deepStrictEqual(JSON.parse(await readFile(poolPath, 'utf-8')), [process.ppid, process.pid, process.pid]); + strictEqual(warn.mock.callCount(), 1); } finally { resumeStaleWriter.resolve(); await staleWriter?.catch(() => {}); fsMock.restore(); netMock.restore(); + warn.mock.restore(); for (const [name, value] of previousEnv) restoreEnv(name, value); await rm(isolatedTmpDir, { recursive: true, force: true }); } @@ -183,3 +186,36 @@ test('a failed token write does not leave an empty lock file', async () => { await rm(isolatedTmpDir, { recursive: true, force: true }); } }); + +test('a holder that keeps losing the lock gives up instead of retrying forever', async () => { + const isolatedTmpDir = await mkdtemp(join(tmpdir(), 'loopback-lock-persistent-loss-')); + const poolPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.json'); + const lockPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.lock'); + const previousTmpEnv = new Map(['TMPDIR', 'TMP', 'TEMP'].map((name) => [name, process.env[name]])); + for (const name of previousTmpEnv.keys()) process.env[name] = isolatedTmpDir; + const fsMock = mock.module('node:fs/promises', { + namedExports: { + ...fsPromises, + readFile: async (path: string, encoding: BufferEncoding) => { + const contents = await readFile(path, encoding); + // Every section has its lock reclaimed as stale between reading and publishing. + if (path === poolPath) await unlink(lockPath); + return contents; + }, + }, + }); + const warn = mock.method(console, 'warn', () => {}); + try { + const pool = JSON.stringify([process.pid, null, null]); + await writeFile(poolPath, pool); + const { releaseAllLoopbackAddressesForCurrentProcess } = await import(freshModuleUrl()); + await rejects(releaseAllLoopbackAddressesForCurrentProcess(), { name: 'LockOwnershipLostError' }); + strictEqual(warn.mock.callCount(), 3); + strictEqual(await readFile(poolPath, 'utf-8'), pool); + } finally { + fsMock.restore(); + warn.mock.restore(); + for (const [name, value] of previousTmpEnv) restoreEnv(name, value); + await rm(isolatedTmpDir, { recursive: true, force: true }); + } +}); From ab2c1078cbcf02d72c6a2f4917fd735532ca70b1 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 1 Oct 2026 11:12:51 -0600 Subject: [PATCH 06/11] test: drop probe-port overrides the net mock already neutralizes The mocked createServer never emits `error`, so the conflict canary reports every port free regardless of these values. Co-Authored-By: Claude Opus 5.5 Dispatch-Task: fix-kriszyp_integration-testing_33-33b5cb8c --- test/loopback-lock-fencing.test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/loopback-lock-fencing.test.ts b/test/loopback-lock-fencing.test.ts index 2fd5cad..e95e987 100644 --- a/test/loopback-lock-fencing.test.ts +++ b/test/loopback-lock-fencing.test.ts @@ -61,8 +61,6 @@ test('a writer superseded after reading cannot erase the successor claim', async 'TEMP', 'HARPER_INTEGRATION_TEST_LOOPBACK_POOL_START', 'HARPER_INTEGRATION_TEST_LOOPBACK_POOL_COUNT', - 'HARPER_INTEGRATION_TEST_CONFLICT_PROBE_PORT', - 'HARPER_INTEGRATION_TEST_HTTP_CONFLICT_PROBE_PORT', ].map((name) => [name, process.env[name]]) ); process.env.TMPDIR = isolatedTmpDir; @@ -70,8 +68,6 @@ test('a writer superseded after reading cannot erase the successor claim', async process.env.TEMP = isolatedTmpDir; process.env.HARPER_INTEGRATION_TEST_LOOPBACK_POOL_START = '2'; process.env.HARPER_INTEGRATION_TEST_LOOPBACK_POOL_COUNT = '3'; - process.env.HARPER_INTEGRATION_TEST_CONFLICT_PROBE_PORT = '0'; - process.env.HARPER_INTEGRATION_TEST_HTTP_CONFLICT_PROBE_PORT = '0'; const staleSnapshotRead = deferred(); const resumeStaleWriter = deferred(); From 98a61451698d7110b36745cbf60912be8655cb3f Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 1 Oct 2026 11:32:41 -0600 Subject: [PATCH 07/11] fix: leave a failed lock-token write to stale reclaim A holder that stalls past the stale timeout between creating the lock and writing its token can find the path already holds a successor's lock, so unlinking by path on the write failure could delete it. The empty lock is now left for stale reclaim, as the instance-registry lock already does. Also documents release's check-then-unlink window and notes the new LockOwnershipLostError rejection in the README. Co-Authored-By: Claude Opus 5.5 Dispatch-Task: fix-kriszyp_integration-testing_33-33b5cb8c --- README.md | 2 ++ src/loopbackAddressPool.ts | 15 +++++++-------- test/loopback-lock-fencing.test.ts | 12 ++++++------ 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 5bcd190..ba3f7ca 100644 --- a/README.md +++ b/README.md @@ -306,6 +306,8 @@ A pool file that is unparseable, not an array, or an empty array can still be pr The quarantine bounds, rather than eliminates, the risk of handing out an address twice: a caller that holds an address for longer than that without binding it (for example, with a `startupMaxMs` above 5 minutes) is not covered. +If other processes take over the pool lock as stale (held over 10 seconds) before this process publishes its update, the allocate and release functions retry from the current pool state with a `[loopback-pool]` warning; after four takeovers in a row they reject with an error named `LockOwnershipLostError`. + ## Node.js Test Runner > Tests executed by this runner must use the `node:test` API. If you're using a different test framework, use the [lifecycle APIs](#api) directly and refer to the [runner source](./src/run.ts) as a reference implementation. diff --git a/src/loopbackAddressPool.ts b/src/loopbackAddressPool.ts index 64362b2..2d53598 100644 --- a/src/loopbackAddressPool.ts +++ b/src/loopbackAddressPool.ts @@ -29,8 +29,8 @@ const HARPER_LOOPBACK_POOL_LOCK_PATH = join(tmpdir(), 'harper-integration-test-l // Constants for timeouts and retries const LOCK_STALE_TIMEOUT_MS = 10000; const RETRY_DELAY_MS = 1000; -// A one-off stall past LOCK_STALE_TIMEOUT_MS loses the lock once; losing it repeatedly means another -// process keeps judging fresh locks stale (clock or mtime skew), which retrying cannot fix. +// A one-off stall past LOCK_STALE_TIMEOUT_MS loses the lock once; losing it repeatedly means sections keep +// outlasting it (a stalling event loop, or clock or mtime skew), which retrying cannot fix. const LOCK_OWNERSHIP_RETRIES = 3; let lockTokenCounter = 0; @@ -170,14 +170,12 @@ async function acquireLock(): Promise { try { // The 'wx' flag causes the open to fail if the file already exists const lockFileHandle = await open(HARPER_LOOPBACK_POOL_LOCK_PATH, 'wx'); + // On a failed write the path may already be a successor's lock, so it is left to stale reclaim. try { await lockFileHandle.writeFile(token); - } catch (error) { - await lockFileHandle.close().catch(() => {}); - await unlink(HARPER_LOOPBACK_POOL_LOCK_PATH).catch(() => {}); - throw error; + } finally { + await lockFileHandle.close(); } - await lockFileHandle.close(); return token; } catch (error) { // If the lock file already exists, it's either stale or we wait for it to be released @@ -203,7 +201,8 @@ async function acquireLock(): Promise { } /** - * Releases the file-based lock if it still belongs to this acquisition. + * Like writePoolFile's ownership check, the token comparison narrows, but does not close, the window + * in which a holder whose lock was taken over as stale deletes its successor's lock. */ async function releaseLock(token: string): Promise { try { diff --git a/test/loopback-lock-fencing.test.ts b/test/loopback-lock-fencing.test.ts index e95e987..0dd5ae5 100644 --- a/test/loopback-lock-fencing.test.ts +++ b/test/loopback-lock-fencing.test.ts @@ -152,7 +152,7 @@ test('a superseded holder does not unlink the replacement lock', async () => { } }); -test('a failed token write does not leave an empty lock file', async () => { +test('a failed token write does not delete a successor lock', async () => { const isolatedTmpDir = await mkdtemp(join(tmpdir(), 'loopback-lock-write-failure-')); const lockPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.lock'); const previousTmpEnv = new Map(['TMPDIR', 'TMP', 'TEMP'].map((name) => [name, process.env[name]])); @@ -161,11 +161,12 @@ test('a failed token write does not leave an empty lock file', async () => { namedExports: { ...fsPromises, open: async (path: string, flags: string) => { - const fileHandle = await fsPromises.open(path, flags); - if (path !== lockPath) return fileHandle; + if (path !== lockPath) return fsPromises.open(path, flags); + await writeFile(path, '', { flag: flags }); return { - close: () => fileHandle.close(), + close: async () => {}, writeFile: async () => { + await writeFile(lockPath, 'replacement-holder'); throw new Error('simulated token write failure'); }, }; @@ -175,7 +176,7 @@ test('a failed token write does not leave an empty lock file', async () => { try { const { withLock } = await import(freshModuleUrl()); await rejects(withLock(async () => {}), /simulated token write failure/); - await rejects(readFile(lockPath), (error: NodeJS.ErrnoException) => error.code === 'ENOENT'); + strictEqual(await readFile(lockPath, 'utf-8'), 'replacement-holder'); } finally { fsMock.restore(); for (const [name, value] of previousTmpEnv) restoreEnv(name, value); @@ -194,7 +195,6 @@ test('a holder that keeps losing the lock gives up instead of retrying forever', ...fsPromises, readFile: async (path: string, encoding: BufferEncoding) => { const contents = await readFile(path, encoding); - // Every section has its lock reclaimed as stale between reading and publishing. if (path === poolPath) await unlink(lockPath); return contents; }, From a40739e0455542b1effc0b07dcd3d0f5e9428d9f Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 6 Oct 2026 12:50:44 -0600 Subject: [PATCH 08/11] test(loopback): make lock fencing fixture hermetic Also align the documented test command and allocator lock-loss error. Co-Authored-By: GPT-5 Codex Dispatch-Task: pr-maint-8c5461fc9b5574d777500f74f01ad393 --- AGENTS.md | 2 +- src/loopbackAddressPool.ts | 1 + test/loopback-lock-fencing.test.ts | 9 +++++++++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index d6cc0a2..194ddac 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -21,5 +21,5 @@ Review the `README.md` and `CONTRIBUTING.md` for all relevant repository informa ## Testing Tips - Use `npm link` in this directory and `npm link @harperfast/integration-testing` in other project directories to test out changes locally - Use `npm run check` to type-check the project without generating a build output -- Use `npm test` to run the tests (`node --test` over `test/**/*.test.ts`). Tests live in `test/`, separate from `src/`, so the build (which emits only `src/`) never includes them. +- Use `npm test` to run the tests (`node --experimental-test-module-mocks --test "test/**/*.test.ts"`). Tests live in `test/`, separate from `src/`, so the build (which emits only `src/`) never includes them. - To keep them runnable without a real Harper, internal-only helpers are exported from their modules (e.g. `runHarperCommand` in `harperLifecycle.ts`) but intentionally NOT re-exported from `index.ts`, so they stay out of the public API. diff --git a/src/loopbackAddressPool.ts b/src/loopbackAddressPool.ts index 2d53598..2cd90d3 100644 --- a/src/loopbackAddressPool.ts +++ b/src/loopbackAddressPool.ts @@ -520,6 +520,7 @@ export async function validateLoopbackAddressPool(): Promise<{ * checked for another process's listener (the slot is released in that case) * @throws {ForeignListenerError} If another process accepts connections on the address's canary * ports and `HARPER_INTEGRATION_TEST_ALLOW_FOREIGN_LISTENERS` is not set (the slot is released) + * @throws {LockOwnershipLostError} If ownership is lost on the initial attempt and all three retries */ export async function getNextAvailableLoopbackAddress(): Promise { // Each index maps to a different loopback address (index 0 -> 127.0.0.2, index 1 -> 127.0.0.3, etc.) diff --git a/test/loopback-lock-fencing.test.ts b/test/loopback-lock-fencing.test.ts index 0dd5ae5..0c67388 100644 --- a/test/loopback-lock-fencing.test.ts +++ b/test/loopback-lock-fencing.test.ts @@ -50,6 +50,14 @@ function createAvailableServer() { return server; } +function createRefusedSocket(): net.Socket { + const socket = new EventEmitter() as unknown as net.Socket; + socket.setTimeout = () => socket; + socket.destroy = () => socket; + queueMicrotask(() => socket.emit('error', Object.assign(new Error('Connection refused'), { code: 'ECONNREFUSED' }))); + return socket; +} + test('a writer superseded after reading cannot erase the successor claim', async () => { const isolatedTmpDir = await mkdtemp(join(tmpdir(), 'loopback-lock-fencing-')); const poolPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.json'); @@ -90,6 +98,7 @@ test('a writer superseded after reading cannot erase the successor claim', async namedExports: { ...net, createServer: createAvailableServer, + connect: createRefusedSocket, }, }); const warn = mock.method(console, 'warn', () => {}); From fccdd3c81daccbcfc2dc79943596cf1a841ff826 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 6 Oct 2026 13:04:48 -0600 Subject: [PATCH 09/11] docs(loopback): qualify the lock release guarantee Co-Authored-By: GPT-5 Codex Dispatch-Task: pr-maint-8c5461fc9b5574d777500f74f01ad393 --- src/loopbackAddressPool.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/loopbackAddressPool.ts b/src/loopbackAddressPool.ts index 2cd90d3..0c5df45 100644 --- a/src/loopbackAddressPool.ts +++ b/src/loopbackAddressPool.ts @@ -224,7 +224,7 @@ async function assertLockHeld(token: string): Promise { } /** - * Executes a callback while holding the lock and releases only the lock this call acquired. + * Executes a callback while holding the lock and checks the current token before attempting release. * A section whose lock was taken over as stale before it published is rerun from fresh pool * state, up to LOCK_OWNERSHIP_RETRIES times, so teardown callers do not inherit a one-off * lost-lock failure. From 4d6c5f5c2dc2a0010ccc67bfc7754c02852e9357 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 12 Oct 2026 18:24:03 -0600 Subject: [PATCH 10/11] fix(loopback): test takeover during atomic pool publication Co-Authored-By: GPT-5 Codex Dispatch-Task: pr-maint-8c5461fc9b5574d777500f74f01ad393 --- src/loopbackAddressPool.ts | 11 +-- test/loopback-lock-fencing.test.ts | 125 ++++++++++++++++++++++++++++- 2 files changed, 128 insertions(+), 8 deletions(-) diff --git a/src/loopbackAddressPool.ts b/src/loopbackAddressPool.ts index 0c5df45..47ceba6 100644 --- a/src/loopbackAddressPool.ts +++ b/src/loopbackAddressPool.ts @@ -200,10 +200,7 @@ async function acquireLock(): Promise { } } -/** - * Like writePoolFile's ownership check, the token comparison narrows, but does not close, the window - * in which a holder whose lock was taken over as stale deletes its successor's lock. - */ +/** The read and unlink are not atomic; a takeover between them can still remove a successor's lock. */ async function releaseLock(token: string): Promise { try { if ((await readFile(HARPER_LOOPBACK_POOL_LOCK_PATH, 'utf-8')) === token) { @@ -267,6 +264,7 @@ function emptyPool(): LoopbackPool { * each other's reservations. * * @param poolPath The pool file path (overridable for tests; defaults to the shared pool file) + * @param lockToken The held lock token used if an aged unusable shared pool needs reinitializing * @returns The loopback pool array, or `null` while the pool file is quarantined */ export async function readPoolFile( @@ -305,9 +303,9 @@ export async function readPoolFile( return null; } quarantinedPoolPaths.delete(poolPath); - console.warn(`[loopback-pool] ${poolPath} has been unusable for ${Math.round(unmodifiedMs / 1000)}s (${problem}); reinitializing.`); const pool = emptyPool(); await writePoolFile(pool, poolPath, lockToken); + console.warn(`[loopback-pool] ${poolPath} has been unusable for ${Math.round(unmodifiedMs / 1000)}s (${problem}); reinitialized.`); return pool; } @@ -324,6 +322,7 @@ let pendingWriteCounter = 0; * * @param pool The loopback pool array to persist * @param poolPath The pool file path (overridable for tests; defaults to the shared pool file) + * @param lockToken The held lock token required to publish the shared pool */ export async function writePoolFile( pool: LoopbackPool, @@ -340,8 +339,6 @@ export async function writePoolFile( } if (poolPath === HARPER_LOOPBACK_POOL_PATH) { if (!lockToken) throw new Error('A lock token is required to publish the shared loopback pool'); - // Check only after the pending file closes and immediately before rename. A takeover after - // this point can still publish a stale snapshot; the token narrows but does not close that race. await assertLockHeld(lockToken); } await rename(pendingPath, poolPath); diff --git a/test/loopback-lock-fencing.test.ts b/test/loopback-lock-fencing.test.ts index 0c67388..afa40aa 100644 --- a/test/loopback-lock-fencing.test.ts +++ b/test/loopback-lock-fencing.test.ts @@ -2,7 +2,7 @@ import test, { mock } from 'node:test'; import { deepStrictEqual, notStrictEqual, rejects, strictEqual } from 'node:assert'; import { EventEmitter } from 'node:events'; import * as fsPromises from 'node:fs/promises'; -import { mkdtemp, readFile, rm, unlink, utimes, writeFile } from 'node:fs/promises'; +import { mkdtemp, readFile, readdir, rm, unlink, utimes, writeFile } from 'node:fs/promises'; import * as net from 'node:net'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -58,6 +58,40 @@ function createRefusedSocket(): net.Socket { return socket; } +function mockTakeoverAfterPendingClose(poolPath: string, lockPath: string, successorPool: (number | null)[]): { + fsMock: ReturnType; + getRenameCount: () => number; +} { + let tookOver = false; + let renameCount = 0; + const fsMock = mock.module('node:fs/promises', { + namedExports: { + ...fsPromises, + open: async (path: string, flags: string) => { + const handle = await fsPromises.open(path, flags); + if (!path.startsWith(`${poolPath}.`) || !path.endsWith('.pending')) return handle; + return { + writeFile: (data: string) => handle.writeFile(data), + close: async () => { + await handle.close(); + if (tookOver) return; + tookOver = true; + await writeFile(poolPath, JSON.stringify(successorPool)); + await writeFile(lockPath, 'successor-holder'); + const staleTime = new Date(Date.now() - 20000); + await utimes(lockPath, staleTime, staleTime); + }, + }; + }, + rename: async (from: string, to: string) => { + renameCount++; + await fsPromises.rename(from, to); + }, + }, + }); + return { fsMock, getRenameCount: () => renameCount }; +} + test('a writer superseded after reading cannot erase the successor claim', async () => { const isolatedTmpDir = await mkdtemp(join(tmpdir(), 'loopback-lock-fencing-')); const poolPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.json'); @@ -135,6 +169,95 @@ test('a writer superseded after reading cannot erase the successor claim', async } }); +test('a takeover during pending preparation prevents the stale rename and retries from the successor pool', async () => { + const isolatedTmpDir = await mkdtemp(join(tmpdir(), 'loopback-lock-pending-takeover-')); + const poolPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.json'); + const lockPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.lock'); + const previousEnv = new Map( + [ + 'TMPDIR', + 'TMP', + 'TEMP', + 'HARPER_INTEGRATION_TEST_LOOPBACK_POOL_START', + 'HARPER_INTEGRATION_TEST_LOOPBACK_POOL_COUNT', + ].map((name) => [name, process.env[name]]) + ); + process.env.TMPDIR = isolatedTmpDir; + process.env.TMP = isolatedTmpDir; + process.env.TEMP = isolatedTmpDir; + process.env.HARPER_INTEGRATION_TEST_LOOPBACK_POOL_START = '2'; + process.env.HARPER_INTEGRATION_TEST_LOOPBACK_POOL_COUNT = '3'; + const successorPool = [process.ppid, null, null]; + const takeover = mockTakeoverAfterPendingClose(poolPath, lockPath, successorPool); + const netMock = mock.module('node:net', { + namedExports: { + ...net, + createServer: createAvailableServer, + connect: createRefusedSocket, + }, + }); + const warn = mock.method(console, 'warn', () => {}); + try { + await writeFile(poolPath, JSON.stringify([null, null, null])); + const { getNextAvailableLoopbackAddress } = await import(freshModuleUrl()); + + strictEqual(await getNextAvailableLoopbackAddress(), '127.0.0.3'); + deepStrictEqual(JSON.parse(await readFile(poolPath, 'utf-8')), [process.ppid, process.pid, null]); + strictEqual(takeover.getRenameCount(), 1, 'only the retry should rename a pending file'); + strictEqual(warn.mock.callCount(), 1); + deepStrictEqual(await readdir(isolatedTmpDir), ['harper-integration-test-loopback-pool.json']); + } finally { + takeover.fsMock.restore(); + netMock.restore(); + warn.mock.restore(); + for (const [name, value] of previousEnv) restoreEnv(name, value); + await rm(isolatedTmpDir, { recursive: true, force: true }); + } +}); + +test('read-time repair passes its lock token through pending preparation', async () => { + const isolatedTmpDir = await mkdtemp(join(tmpdir(), 'loopback-lock-repair-takeover-')); + const poolPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.json'); + const lockPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.lock'); + const previousTmpEnv = new Map( + ['TMPDIR', 'TMP', 'TEMP', 'HARPER_INTEGRATION_TEST_LOOPBACK_POOL_START', 'HARPER_INTEGRATION_TEST_LOOPBACK_POOL_COUNT'] + .map((name) => [name, process.env[name]]) + ); + process.env.TMPDIR = isolatedTmpDir; + process.env.TMP = isolatedTmpDir; + process.env.TEMP = isolatedTmpDir; + process.env.HARPER_INTEGRATION_TEST_LOOPBACK_POOL_START = '2'; + process.env.HARPER_INTEGRATION_TEST_LOOPBACK_POOL_COUNT = '3'; + const successorPool = [process.ppid, null, null]; + const takeover = mockTakeoverAfterPendingClose(poolPath, lockPath, successorPool); + const netMock = mock.module('node:net', { + namedExports: { + ...net, + createServer: createAvailableServer, + connect: createRefusedSocket, + }, + }); + const warn = mock.method(console, 'warn', () => {}); + try { + await writeFile(poolPath, '['); + const staleTime = new Date(Date.now() - 10 * 60 * 1000); + await utimes(poolPath, staleTime, staleTime); + const { getNextAvailableLoopbackAddress } = await import(freshModuleUrl()); + + strictEqual(await getNextAvailableLoopbackAddress(), '127.0.0.3'); + deepStrictEqual(JSON.parse(await readFile(poolPath, 'utf-8')), [process.ppid, process.pid, null]); + strictEqual(takeover.getRenameCount(), 1, 'only allocation after repair retry should rename a pending file'); + strictEqual(warn.mock.callCount(), 1, 'only the lost-ownership retry should warn before succeeding'); + deepStrictEqual(await readdir(isolatedTmpDir), ['harper-integration-test-loopback-pool.json']); + } finally { + takeover.fsMock.restore(); + netMock.restore(); + warn.mock.restore(); + for (const [name, value] of previousTmpEnv) restoreEnv(name, value); + await rm(isolatedTmpDir, { recursive: true, force: true }); + } +}); + test('a superseded holder does not unlink the replacement lock', async () => { const isolatedTmpDir = await mkdtemp(join(tmpdir(), 'loopback-lock-release-')); const lockPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.lock'); From c9d9e45557f85e667d6c532b1ee505696d4d0b0b Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 12 Oct 2026 18:48:13 -0600 Subject: [PATCH 11/11] test(loopback): model lock takeover by replacement Co-Authored-By: GPT-5 Codex Dispatch-Task: pr-maint-8c5461fc9b5574d777500f74f01ad393 --- src/loopbackAddressPool.ts | 14 ++------- test/loopback-lock-fencing.test.ts | 46 ++++++++++++++++++++++++++---- 2 files changed, 42 insertions(+), 18 deletions(-) diff --git a/src/loopbackAddressPool.ts b/src/loopbackAddressPool.ts index 47ceba6..b4c2a64 100644 --- a/src/loopbackAddressPool.ts +++ b/src/loopbackAddressPool.ts @@ -161,8 +161,6 @@ class LockOwnershipLostError extends Error { * Uses the 'wx' file flag which atomically fails if the file already exists, providing * a simple but effective cross-process mutex. Handles stale locks by removing lock files * older than LOCK_STALE_TIMEOUT_MS (10 seconds). - * - * @returns The token that identifies this lock acquisition */ async function acquireLock(): Promise { const token = `${process.pid}-${++lockTokenCounter}-${randomBytes(8).toString('hex')}`; @@ -170,7 +168,7 @@ async function acquireLock(): Promise { try { // The 'wx' flag causes the open to fail if the file already exists const lockFileHandle = await open(HARPER_LOOPBACK_POOL_LOCK_PATH, 'wx'); - // On a failed write the path may already be a successor's lock, so it is left to stale reclaim. + // The write may have outlasted the stale timeout, so the path may no longer be ours to unlink. try { await lockFileHandle.writeFile(token); } finally { @@ -220,15 +218,7 @@ async function assertLockHeld(token: string): Promise { if (currentToken !== token) throw new LockOwnershipLostError(); } -/** - * Executes a callback while holding the lock and checks the current token before attempting release. - * A section whose lock was taken over as stale before it published is rerun from fresh pool - * state, up to LOCK_OWNERSHIP_RETRIES times, so teardown callers do not inherit a one-off - * lost-lock failure. - * - * @param callback The async function to execute while holding the lock - * @returns The result of the callback function - */ +/** Reruns the callback after lock ownership is lost, up to LOCK_OWNERSHIP_RETRIES times; callers must reread shared state. */ export async function withLock(callback: (lockToken: string) => Promise): Promise { for (let attempt = 1; ; attempt++) { const token = await acquireLock(); diff --git a/test/loopback-lock-fencing.test.ts b/test/loopback-lock-fencing.test.ts index afa40aa..305278e 100644 --- a/test/loopback-lock-fencing.test.ts +++ b/test/loopback-lock-fencing.test.ts @@ -77,7 +77,8 @@ function mockTakeoverAfterPendingClose(poolPath: string, lockPath: string, succe if (tookOver) return; tookOver = true; await writeFile(poolPath, JSON.stringify(successorPool)); - await writeFile(lockPath, 'successor-holder'); + await unlink(lockPath); + await writeFile(lockPath, 'successor-holder', { flag: 'wx' }); const staleTime = new Date(Date.now() - 20000); await utimes(lockPath, staleTime, staleTime); }, @@ -150,7 +151,7 @@ test('a writer superseded after reading cannot erase the successor claim', async const successorAddress = await getNextAvailableLoopbackAddress(); strictEqual(successorAddress, '127.0.0.3'); - await writeFile(lockPath, 'replacement-holder'); + await writeFile(lockPath, 'replacement-holder', { flag: 'wx' }); await utimes(lockPath, staleTime, staleTime); resumeStaleWriter.resolve(); @@ -272,7 +273,8 @@ test('a superseded holder does not unlink the replacement lock', async () => { await resumeSection.promise; }); await sectionEntered.promise; - await writeFile(lockPath, 'replacement-holder'); + await unlink(lockPath); + await writeFile(lockPath, 'replacement-holder', { flag: 'wx' }); resumeSection.resolve(); await staleHolder; @@ -294,11 +296,12 @@ test('a failed token write does not delete a successor lock', async () => { ...fsPromises, open: async (path: string, flags: string) => { if (path !== lockPath) return fsPromises.open(path, flags); - await writeFile(path, '', { flag: flags }); + const handle = await fsPromises.open(path, flags); return { - close: async () => {}, + close: () => handle.close(), writeFile: async () => { - await writeFile(lockPath, 'replacement-holder'); + await unlink(lockPath); + await writeFile(lockPath, 'replacement-holder', { flag: 'wx' }); throw new Error('simulated token write failure'); }, }; @@ -316,6 +319,37 @@ test('a failed token write does not delete a successor lock', async () => { } }); +test('a failed token write leaves its empty lock for stale reclamation', async () => { + const isolatedTmpDir = await mkdtemp(join(tmpdir(), 'loopback-lock-empty-write-failure-')); + const lockPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.lock'); + const previousTmpEnv = new Map(['TMPDIR', 'TMP', 'TEMP'].map((name) => [name, process.env[name]])); + for (const name of previousTmpEnv.keys()) process.env[name] = isolatedTmpDir; + const fsMock = mock.module('node:fs/promises', { + namedExports: { + ...fsPromises, + open: async (path: string, flags: string) => { + if (path !== lockPath) return fsPromises.open(path, flags); + const handle = await fsPromises.open(path, flags); + return { + close: () => handle.close(), + writeFile: async () => { + throw new Error('simulated ENOSPC'); + }, + }; + }, + }, + }); + try { + const { withLock } = await import(freshModuleUrl()); + await rejects(withLock(async () => {}), /simulated ENOSPC/); + strictEqual(await readFile(lockPath, 'utf-8'), ''); + } finally { + fsMock.restore(); + for (const [name, value] of previousTmpEnv) restoreEnv(name, value); + await rm(isolatedTmpDir, { recursive: true, force: true }); + } +}); + test('a holder that keeps losing the lock gives up instead of retrying forever', async () => { const isolatedTmpDir = await mkdtemp(join(tmpdir(), 'loopback-lock-persistent-loss-')); const poolPath = join(isolatedTmpDir, 'harper-integration-test-loopback-pool.json');