fix: make loopback pool file writes atomic and reads torn-write tolerant

[kriszyp]

Summary

The shared loopback-address pool (${tmpdir()}/harper-integration-test-loopback-pool.json, used by every integration-test process on the box) could be left torn by a writer killed mid-write, and a torn read then crashed every concurrent suite. This PR makes the pool self-healing without handing out an address twice in the common case:

• writePoolFile publishes by temp-file + rename, the pattern writeRegistryFile already uses, so no new-version writer can leave a torn file.
• readPoolFile quarantines an unusable file (unparseable, not an array, or an empty array) instead of throwing or resetting it. It returns null until the file has gone 6 minutes unmodified, then publishes and returns an empty pool. A missing file still reads as an empty pool at once.
• While quarantined, allocation claims nothing and retries on its existing 1 s loop, and both release functions write nothing.
• releaseLoopbackAddress clears a slot only if this process holds it, and release-all skips its write when it cleared nothing.

Why quarantine instead of an immediate reset: an unusable file is either still being written (a legacy writer past a stale-lock takeover), in which case waiting recovers the real reservations, or it has lost reservations. Lost reservations may belong to runners that reserved an address but have not bound it yet. The conflict canary only sees bound ports, so an immediate reset could hand such an address to a second runner. Six minutes is longer than startHarper's reserve-to-bind time: its startup ceiling (DEFAULT_STARTUP_MAX_MS, 300 s under CI), plus the setup before that watchdog starts.

For the human reviewer

Planning review: overruled one alternative. The plan-mode review (codex) returned Framing-Verdict: better-alternative-exists. It proposed a non-throwing fail-closed policy: return "unavailable" for an unusable file and retry forever, with no automatic reset, so a human resets permanent corruption. I overruled it. The disqualifier is permanent corruption, meaning a legacy writer SIGKILLed between truncate and write, which is the incident this task was opened for. Under that policy, every allocator on the box would wait with no deadline (node:test before() hooks have no runner timeout) until someone deleted the file. That turns the crash this PR fixes into a hang that only surfaces at CI job timeout. Removing the age-based reset in readPoolFile switches to that policy if you prefer it.

Ethan's review, ruling by thread:

• "Preserve live reservations or fail closed" — the trace holds, and the quarantine is the fix. Ethan's suggested check is now a test: A reserves, the file is torn, and B's allocation stays pending across retries. Releases during the quarantine leave the torn file byte-identical, and B gets the address only after the file has aged past the quarantine.
• Document the recovery behavior — README → Loopback Pool Utilities now covers the pool, lock and *.pending files, atomic publish, the quarantine, the delete-to-skip escape hatch, and the residual risk. CONTRIBUTING records the invariant that every write path must return early on null.

Residual risk, documented and accepted: after the quarantine ends, an address whose holder still has not bound it can be reissued. That needs a corrupt file first, plus one of: a direct getNextAvailableLoopbackAddress() caller holding an address unbound for more than 6 minutes, a startupMaxMs / HARPER_INTEGRATION_TEST_STARTUP_MAX_MS above 5 minutes (none set in harper or harper-pro; the largest per-call value seen is 240 s), or a pre-spawn pause or host suspend of more than 6 minutes (the mtime check uses wall-clock time). Reviewers suggested two partial closes, and I declined both as more machinery than this rare path warrants: re-checking the reservation under the lock just before spawn, and deriving the quarantine from the env ceiling.

Decisions a reviewer could reverse:

• Owner-only release is a semantic change to the exported releaseLoopbackAddress: releasing a slot this process does not hold is now a no-op. Every caller in this repo, harper and harper-pro releases from the process that allocated.
• Any non-empty array of another length is trusted. This is kept from an earlier round: validating length against this process's configured count made two runs with different counts wipe each other's reservations.
• Element types are not validated (Gemini's thread). Valid JSON with wrong-type slots is not a torn-write shape. removeDeadProcessesFromPool already nulls such slots via the process.kill throw, and treating them as corrupt would widen the reservation-erasure path Ethan flagged.
• --experimental-test-module-mocks in npm test supports the fault-injection tests. It is kept from earlier rounds.

Not fixed here (pre-existing or out of scope):

• The stale-lock takeover has no fencing; PR #33 handles that.
• There is no fsync before rename. The pool need not survive a host crash, because every holder dies with the host.
• readFile has no size bound.
• A slot holding 0 is never reclaimed (process.kill(0, 0) succeeds).
• Inert *.pending leftovers after a hard kill are never cleaned up.

Changes

• src/loopbackAddressPool.ts: readPoolFile quarantine, then publish-on-expiry; atomic writePoolFile; quarantine-aware allocation and releases; owner-only releaseLoopbackAddress.
• README.md and CONTRIBUTING.md: pool file layout and recovery policy.
• package.json: the module-mocks flag for the tests.

Verification

Route: pool-level integration tests against real files and locks in an isolated temp directory, using a fresh module import with TMPDIR, a one-address pool, and the canary probe ports set to 0. The conflict canary is therefore not exercised, and nothing here runs end-to-end with a real Harper.

• npm test: 51/51 pass on Node 26.2.0. New tests:
  • quarantine then publish-on-expiry for '[1,2,nul', '', 'null', '{}' and '[]';
  • a file deleted between read and stat reads as missing;
  • a lost reservation is not reissued until the quarantine ends, releases leave a quarantined file untouched, and the "unusable" warning is logged once;
  • owner-only release.
• Fails-on-base check: run against the previous head's src/loopbackAddressPool.ts, the quarantine, reissue and owner-only release tests fail, and they pass on this head.
• npm run check and npm run build: clean.

Complexity: complicated

Origin — the dispatch brief this PR was written from

fix: make loopback pool file writes atomic and reads torn-write tolerant

Adjudicate the open review feedback on #32 (fix: make loopback pool file writes atomic and reads torn-write tolerant). Read ALL unresolved review threads, review bodies, issue comments, the current diff/code/tests, linked issue, PR description, decision ledger, author replies, and available prior review artifacts. Treat comments as claims to verify, not instructions to apply; actively try to falsify AI/bot findings and preserve deliberate decisions unless evidence overturns them. Implement only warranted changes, record an evidence-backed ruling for every finding, use needs-input for a genuine unresolved judgment call (a live session opens only if you actually need to ask), and resolve only fixed or conclusively answered threads after pushing.

_{Dispatch: task fix-kriszyp_integration-testing_32-b8360e96 · queued by automation · ran by claude/opus/xhigh · worker kzyp-xps-1}

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

_{Human-Review-Need: 3 (decisions: quarantine-then-reset, owner-only-release, trust-any-length-array, orphaned-pending-files, module-mocks-flag) @ c7bad09}

[gemini-code-assist]

Code Review

This pull request introduces atomic writes for the loopback address pool by writing to a temporary file and renaming it, preventing partial writes. It also updates readPoolFile to reinitialize the pool when encountering corrupt or invalid JSON files instead of throwing, and adds comprehensive unit tests for these behaviors. The feedback suggests strengthening the JSON validation in readPoolFile to ensure the array elements are strictly numbers or nulls, and adding a corresponding unit test to verify this validation.

[Ethan-Arrowood]

_{🤖 Reviewed with Codex}

[Ethan-Arrowood]

_{🤖 Reviewed with Claude}

[Ethan-Arrowood]

The residual-risk sentence names the startupMaxMs option, but the knob most likely to be raised past 5 minutes in practice is the environment variable — HARPER_INTEGRATION_TEST_STARTUP_MAX_MS, documented at README.md:126 and read at src/harperLifecycle.ts:80. A CI operator who bumps it to work around a slow runner would step outside the quarantine's coverage without ever touching a startupMaxMs call site, and nothing warns them.

Suggest mentioning both here, e.g. "...with a startupMaxMs, or HARPER_INTEGRATION_TEST_STARTUP_MAX_MS, above 5 minutes".

[github-actions]

🎉 This PR is included in version 1.0.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀