Prevent stale loopback pool writers from overwriting address claims

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.

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.
 

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.

src/loopbackAddressPool.ts

@@ -29,6 +29,11 @@ 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 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;
 
 // 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,24 +144,37 @@ async function releaseRefusedAddress(address: string): Promise<void> {
 	}
 }
 
+class LockOwnershipLostError extends Error {
+	constructor(cause?: unknown) {
+		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';
+	}
+}
+
 /**
  * Acquires a file-based lock by creating the lock file. This enables safe concurrent
  * access to the loopback pool across multiple test processes.
  *
  * 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 A promise that resolves when the lock is acquired
  */
-async function acquireLock(): Promise<void> {
+async function acquireLock(): Promise<string> {
+	const token = `${process.pid}-${++lockTokenCounter}-${randomBytes(8).toString('hex')}`;
 	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;
+			// The write may have outlasted the stale timeout, so the path may no longer be ours to unlink.
+			try {
+				await lockFileHandle.writeFile(token);
+			} finally {
+				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
 			if ((error as NodeJS.ErrnoException).code === 'EEXIST') {
@@ -180,31 +198,40 @@ async function acquireLock(): Promise<void> {
 	}
 }
 
-/**
- * Releases the file-based lock by deleting the lock file.
- */
-async function releaseLock(): Promise<void> {
+/** The read and unlink are not atomic; a takeover between them can still remove a successor's lock. */
+async function releaseLock(token: string): Promise<void> {
 	try {
-		await unlink(HARPER_LOOPBACK_POOL_LOCK_PATH);
-	} catch {
-		// Ignore errors if lock file is already gone
-	}
+		if ((await readFile(HARPER_LOOPBACK_POOL_LOCK_PATH, 'utf-8')) === token) {
+			await unlink(HARPER_LOOPBACK_POOL_LOCK_PATH);
+		}
+	} 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.
- *
- * @param callback The async function to execute while holding the lock
- * @returns The result of the callback function
- */
-async function withLock<T>(callback: () => Promise<T>): Promise<T> {
-	await acquireLock();
+async function assertLockHeld(token: string): Promise<void> {
+	let currentToken: string;
 	try {
-		return await callback();
-	} finally {
-		await releaseLock();
+		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();
+}
+
+/** Reruns the callback after lock ownership is lost, up to LOCK_OWNERSHIP_RETRIES times; callers must reread shared state. */
+export async function withLock<T>(callback: (lockToken: string) => Promise<T>): Promise<T> {
+	for (let attempt = 1; ; attempt++) {
+		const token = await acquireLock();
+		try {
+			return await callback(token);
+		} catch (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);
+		}
 	}
 }
 
@@ -227,9 +254,13 @@ 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(poolPath: string = HARPER_LOOPBACK_POOL_PATH): Promise<LoopbackPool | null> {
+export async function readPoolFile(
+	poolPath: string = HARPER_LOOPBACK_POOL_PATH,
+	lockToken?: string
+): Promise<LoopbackPool | null> {
 	let problem: string;
 	try {
 		const parsed: unknown = JSON.parse(await readFile(poolPath, 'utf-8'));
@@ -262,9 +293,9 @@ export async function readPoolFile(poolPath: string = HARPER_LOOPBACK_POOL_PATH)
 		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);
+	await writePoolFile(pool, poolPath, lockToken);
+	console.warn(`[loopback-pool] ${poolPath} has been unusable for ${Math.round(unmodifiedMs / 1000)}s (${problem}); reinitialized.`);
 	return pool;
 }
 
@@ -276,11 +307,18 @@ 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)
+ * @param lockToken The held lock token required to publish the shared pool
  */
-export async function writePoolFile(pool: LoopbackPool, poolPath: string = HARPER_LOOPBACK_POOL_PATH): Promise<void> {
+export async function writePoolFile(
+	pool: LoopbackPool,
+	poolPath: string = HARPER_LOOPBACK_POOL_PATH,
+	lockToken?: string
+): Promise<void> {
 	const pendingPath = `${poolPath}.${process.pid}.${++pendingWriteCounter}.${randomBytes(8).toString('hex')}.pending`;
 	try {
 		const pendingFileHandle = await open(pendingPath, 'wx');
@@ -289,6 +327,10 @@ 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');
+			await assertLockHeld(lockToken);
+		}
 		await rename(pendingPath, poolPath);
 	} catch (error) {
 		await unlink(pendingPath).catch(() => {});
@@ -465,6 +507,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<string> {
 	// Each index maps to a different loopback address (index 0 -> 127.0.0.2, index 1 -> 127.0.0.3, etc.)
@@ -490,9 +533,9 @@ export async function getNextAvailableLoopbackAddress(): Promise<string> {
 	// the lingering node has had time to exit.
 	const triedIndices = new Set<number>();
 	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 +555,7 @@ export async function getNextAvailableLoopbackAddress(): Promise<string> {
 				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 +667,16 @@ export async function releaseLoopbackAddress(address: string): Promise<void> {
 	// 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 +685,9 @@ export async function releaseLoopbackAddress(address: string): Promise<void> {
  * Useful for cleanup during graceful shutdown.
  */
 export async function releaseAllLoopbackAddressesForCurrentProcess(): Promise<void> {
-	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 +700,6 @@ export async function releaseAllLoopbackAddressesForCurrentProcess(): Promise<vo
 		}
 
 		// Write the updated pool back to the file
-		if (released) await writePoolFile(loopbackPool);
+		if (released) await writePoolFile(loopbackPool, HARPER_LOOPBACK_POOL_PATH, lockToken);
 	});
 }