Prevent stale loopback pool writers from overwriting address claims

[kriszyp]

⊙ Problem

The loopback address pool is shared by test processes. If holder A reads the pool, its lock is reclaimed as stale, holder B publishes a claim, and A resumes, A can overwrite B's newer snapshot. The deterministic regression test exercises that interleave with isolated pool and lock files (test).

❓ Your call: Keep this stale-writer fix in #33 alongside #32? The current split assigns torn-write tolerance and .pending recovery to #32; when both changes land, its publisher must check the token immediately before rename and pass the token through read-time repair.

💡 Solution

Every lock acquisition writes a per-acquisition token. Pool publication checks that token immediately before writing, and release compares it before attempting to unlink. If a check observes that another process took over the lock, the whole callback is retried from a fresh pool read with a [loopback-pool] warning, up to three retries; a fourth observed loss rejects with LockOwnershipLostError. The comparisons expose common takeovers but leave narrow races between check and write, read and unlink, and competing stale reclaimers' stat and unlink operations.

⚖️ Alternatives

• Keep age-based stale recovery and detect a takeover with the token, as implemented. Reclaiming only when the token's PID is dead plus a long mtime ceiling would close the remaining windows, but a paused holder could block every runner and PID namespaces or reuse need handling.
• Retry forever, fail on the first observed loss, or warn and retry three times. Three retries bound repeated lost ownership while allowing a transient stall to recover. After four losses, teardownHarper can reject before removing the install directory.
• Keep or remove the pending-file sweeper. It was removed because this branch has no pending-file writer and fix: make loopback pool file writes atomic and reads torn-write tolerant #32 is changing that naming contract; orphan cleanup belongs with the writer and its tests.
• Share one lock implementation with src/harperInstanceRegistry.ts. That extraction remains follow-up work; the two locks currently differ in fencing, poll interval, acquisition timeout, and publication method.

🔧 Changes

Product and architecture tour

A stale holder retries from current pool state

The lock still uses age-based crash recovery after 10 seconds. A token lets a holder detect when it can see that its lock was replaced; withLock then reruns the callback, which reads the pool again. The checks are observations, so an ownership change can still race after a check.

A token mismatch observed by assertLockHeld rejects that attempt; withLock retries from a fresh pool read up to three times.

A superseded holder starts a fresh attempt

The ownership check catches the interleave covered by the regression test; it does not make the age-based reclaim operation atomic.

sequenceDiagram
    participant A as Stale holder
    participant L as Lock file
    participant B as New holder
    participant P as Pool file
    A->>L: acquire token A; read pool
    B->>L: reclaim stale lock; write token B
    B->>P: publish claim B
    A->>L: check token A
    L-->>A: token B
    A->>A: warn and retry
    A->>P: read current pool; publish a new attempt

Loading

• src/loopbackAddressPool.ts mints a PID, module counter, and random token on acquisition; after a token-write error it closes the handle but leaves the path for stale reclaim (failure path). releaseLock compares the token before unlinking (release); assertLockHeld rejects a visible mismatch or missing file, and writePoolFile calls it before the pool write (check, publish).
• withLock warns and retries a lost-ownership callback three times; allocation and both release paths pass the acquisition token through, and the allocator's JSDoc names the final LockOwnershipLostError rejection (allocation, release paths, error contract).
• README.md documents the warning and rejection behavior after four consecutive takeovers.
• package.json enables Node's experimental module mocks for tests; CONTRIBUTING.md and AGENTS.md document the test runner flag and its use.

✅ Verification

The regression tests use fresh module instances and isolated pool files. They cover a writer superseded after reading, a replacement lock preserved during release, a successor lock preserved after token-write failure, and the three-retry cap; the socket probe is mocked with node:net so the tests do not probe host ports on other platforms (interleave and socket fixture, release and failed-write cases, retry cap). npm run check, npm run build, and npm test passed on Node 26.2.0 after the fixture and documentation updates: 56 passed, 12 platform-specific skips, 0 failed.

_{🤖 Generated by OpenAI Codex; posted via @kriszyp.}

Related PRs: #32 overlaps (atomic publication and torn-write tolerance are adjacent; carry the ownership check immediately before its rename); #34 independent (installed CLI resolution shares README.md); #35 independent (restart cleanup shares CONTRIBUTING.md and README.md); #36 independent (log-tail behavior shares README.md); #40 independent (foreign-listener rejection is already in main; this PR fences pool writers)

Complexity: medium

_{Review-Coverage: authored=codex; ran=gemini,claude,cursor-composer,codex; adjudicated=domain; declined=cursor-grok,cursor-kimi,cursor-muse; rounds=9; full=5 @ 738ee0e}

_{Review-Attention: read ~4m (decisions: do-less-alternative, lock-primitive, retry-policy) @ 738ee0e}

[Ethan-Arrowood]

seems like the review picked up some major issues with this implementation.

_{🤖 Reviewed with Claude}

[Ethan-Arrowood]

The fencing fix closes the "superseded writer clobbers the successor's claim" hole, but the pool file is still published by a truncating in-place write — which is the hole harperInstanceRegistry.writeRegistryFile was explicitly changed to avoid. Its comment at src/harperInstanceRegistry.ts:293-302 describes this exact failure.

Concrete scenario: a process is SIGKILLed between writeFile's truncate and its final write (the kill this whole stale-lock machinery exists to survive). The pool file is left as torn JSON, e.g. [null,null,nu. readPoolFile (src/loopbackAddressPool.ts:235-246) only special-cases ENOENT, so JSON.parse throws a SyntaxError that propagates out of withLock — it isn't a LockOwnershipLostError, so the retry loop rethrows it. Every subsequent getNextAvailableLoopbackAddress, releaseLoopbackAddress, and releaseAllLoopbackAddressesForCurrentProcess on that machine then fails permanently, with no self-healing and no "delete this file to reset" hint (contrast src/harperInstanceRegistry.ts:280). A narrower version of the same thing happens without a kill: a reclaimer that stole the lock as stale can readPoolFile while the previous holder's writeFile is mid-flight and observe a partial file.

Suggested fix — mirror the registry:

async function writePoolFile(pool: LoopbackPool, lockToken: string): Promise<void> {
	await assertLockHeld(lockToken);
	const pendingPath = `${HARPER_LOOPBACK_POOL_PATH}.${process.pid}.${++pendingWriteCounter}.${randomBytes(8).toString('hex')}.pending`;
	try {
		const pendingFileHandle = await open(pendingPath, 'wx');
		try {
			await pendingFileHandle.writeFile(JSON.stringify(pool));
		} finally {
			await pendingFileHandle.close();
		}
		await rename(pendingPath, HARPER_LOOPBACK_POOL_PATH);
	} catch (error) {
		await unlink(pendingPath).catch(() => {});
		throw error;
	}
}

That also gives sweepStalePendingPoolFiles something real to clean up (a writer killed between open and rename leaks the pending file, which is the only way one can appear).

To confirm the current behavior without the fix: write [null,null,nu to ${tmpdir()}/harper-integration-test-loopback-pool.json and call getNextAvailableLoopbackAddress() — expected result today is an unhandled SyntaxError on every call until the file is deleted by hand. Worth adding as a regression test alongside the fix.

[kriszyp]

Good catch, and the scenario is right. It is, though, the subject of fix: make loopback pool file writes atomic and reads torn-write tolerant, which is open and being reworked right now (its read side is moving to quarantine-then-reinitialize). Landing a second copy of the same writer here would put two diverging writePoolFile versions in flight, so this PR stays limited to lock ownership.

The merge obligation is in the PR description. Whichever PR lands second must:

• keep assertLockHeld immediately before the rename, after the pending file closes
• thread the lock token into fix: make loopback pool file writes atomic and reads torn-write tolerant #32's internal reinitialize-and-publish write

I'm leaving this thread open so you can confirm that split.

— Claude Opus 5.5

[Ethan-Arrowood]

You have some conflicts that need to be resolved.