Prevent stale loopback pool writers from overwriting address claims

[kriszyp]

⊙ Problem

The atomic writer from #32 is now in main, so readers see complete pool snapshots. A different race remains: holder A can read the pool, pause until its age-based lock is reclaimed, then resume after holder B publishes and rename A's stale snapshot over B's claim. The regression reproduces that interleave with separate pool and lock files (stale-holder test).

💡 Solution

Each lock acquisition writes a unique token. A shared-pool publisher checks that token after its pending file closes and immediately before rename; a visible mismatch or missing lock abandons that callback. withLock then acquires a new token and reruns the callback, which rereads the pool, up to three retries. A fourth observed ownership loss rejects with LockOwnershipLostError.

readPoolFile also passes the held token to its aged-corrupt-pool repair write. Lock release compares the on-disk token before unlinking. If writing a newly acquired token fails, the code closes the handle and leaves the empty lock for the 10-second stale reclaimer; after a long stall, unlinking by path could delete a successor's lock instead.

⚖️ Alternatives

• Keep age-based stale recovery and reject a takeover when the token check observes it. This bounds retries and handles visible takeover; it still leaves races between check and rename, token read and unlink, and stale-stat and unlink.
• Require stronger ownership before reclaiming, such as confirming the holder is dead or using a platform lock primitive. That needs a separate decision about paused holders, PID namespaces and reuse, and portability.
• Retrying has a teardown cost: if releaseLoopbackAddress exhausts its retries, teardownHarper can reject before removing the install directory (current cleanup order); lifecycle cleanup is a follow-up.
• The writer removes pending files after ordinary errors, but a process crash can leave an inert .pending file. Crash-orphan cleanup belongs with the writer and its naming contract; this PR does not add a sweeper.

❓ Your call: Is this bounded, best-effort ownership fence acceptable with the documented residual races, or should #33 wait for a stronger ownership protocol? Recommendation: keep this scoped fence and track the stronger protocol separately; it has distinct liveness, PID-identity, and portability tradeoffs.

🔧 Changes

Product and architecture tour

A stale pending update is rejected when ownership loss is observed

What changes after a newer process takes over the pool lock?

The age-based lock still supports crash recovery after 10 seconds. A per-acquisition token lets a writer detect a takeover it can observe. The shared writer closes its pending file before checking ownership, and checks immediately before rename. The aged-corrupt-pool repair path receives the same token, so allocation, release, and repair all use the fence.

A token mismatch or missing lock observed before shared-pool rename abandons that callback; withLock retries from a fresh pool read up to three times.

• The token check is after pending-file close so no later asynchronous preparation separates it from rename.
• This is an observation, not an atomic compare-and-rename; takeover can still race after the check.

A superseded callback starts over from the successor's pool

The stale holder drops its attempt when it sees the replacement token or a missing lock, then reads current state under a new lock.

sequenceDiagram
    participant A as Stale holder
    participant L as Lock file
    participant B as Successor
    participant P as Pool file
    A->>P: read snapshot
    B->>L: reclaim stale lock; write token B
    B->>P: publish claim B
    A->>L: check token A before rename
    L-->>A: token B or missing
    A->>A: abandon callback; retry
    B->>L: release token B
    A->>L: acquire new token C
    A->>P: reread current snapshot
    A->>P: publish fresh update

Loading

Publication order

loopbackAddressPool.ts:324

const pendingFileHandle = await open(pendingPath, 'wx')
try {
  await pendingFileHandle.writeFile(JSON.stringify(pool))
} finally {
  await pendingFileHandle.close()
}
if (poolPath === HARPER_LOOPBACK_POOL_PATH) {
  if (!lockToken) ✗
  await assertLockHeld(lockToken)
}
await rename(pendingPath, poolPath)

• src/loopbackAddressPool.ts writes an acquisition token and leaves a failed token-write path for stale reclamation (release comparison, retry policy, repair token, check before rename, allocation and release call sites).
• test/loopback-lock-fencing.test.ts covers a stale allocation, takeover during pending preparation, and takeover during read-time repair (pending-close takeover, repair takeover, release, failed-write and retry cases).
• README.md documents the warning and rejection after repeated takeovers. AGENTS.md and CONTRIBUTING.md document the Node test-runner flag and fault-injection setup.

✅ Verification

On Node 26.2.0, npm run check, npm run build, and npm test passed: 73 passed, 12 skipped, 0 failed. On the current PR head c9d9e45, all nine macOS, Ubuntu, and Windows jobs on Node 22, 24, and 26 and the commitlint job pass (GitHub Actions run).

Before #42 lands after this PR, rebase its new reservation and release helpers with the lock token threaded through their shared-pool reads and writes; if #42 lands first, rebase this PR and preserve that token plumbing. #35 touches teardown/restart cleanup, where an exhausted release retry can interrupt directory removal.

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

Related PRs: #32 overlaps, #35 overlaps, #42 overlaps, 1 other independent

Complexity: complicated

_{Review-Coverage: authored=codex; ran=gemini,claude,cursor-composer,codex; adjudicated=domain; declined=cursor-grok,cursor-kimi,cursor-muse; rounds=11; full=6 @ c9d9e45}

_{Review-Attention: study ~9m (decisions: do-less-alternative, fencing-strength, retry-policy, ownership-error-api; raised: open major) @ c9d9e45}

[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.