Skip to content

Commit 5b5ee37

Browse files
codeslakeclaude
andcommitted
Merge remote-tracking branch 'origin/fix/reap-fingerprint-records' into HEAD
Two judgement conflicts, both orthogonal-halves-of-one-block (same pattern already recorded for cnighswonger#368/cnighswonger#369): test/proc-helpers.mjs: cnighswonger#345 guards onPort(0) against selecting every proxy child (CACHE_FIX_PROXY_PORT=0), cnighswonger#369 (already in this build) added probeHealth/waitForHolder right after the same line. No shared subject; kept both — theirs' guarded onPort(), ours' probeHealth/waitForHolder unchanged. test/proxy-held-port.test.mjs, block in 'refuses nothing when the proxy under it dies': cnighswonger#345 proposes swapping the local 'ok'-sentinel probe (cnighswonger#355's, ours) for the shared health(port) helper it adds elsewhere in the file, which resolves the number 200 rather than the string "ok". Three lines below this hunk, 'const cut = seen.filter((c) => c !== "ok")' already depends on the 'ok' sentinel — taking theirs would silently make cut === seen (the exact defect this file's classify()/probe rewrite exists to prevent). Kept ours whole. Also dropped two merge-additive duplicates the mechanical classifier cannot see, same class as the recorded 'cnighswonger#356 duplicates' ledger entry: a byte-identical second copy of 'classify survives a probe that answers with a status code' (cnighswonger#345's own commit landing both directly and via cnighswonger#356's earlier cherry-pick of it), and a second, differently-worded typeof guard cnighswonger#345 stacked under the first. Verified: node --test test/proxy-fingerprint-reap.test.mjs (7/7, including 'onPort(0) selects nothing, and still selects on a real port'), and --test-name-pattern=classify in test/proxy-held-port.test.mjs (4/4, no duplicate case). Co-Authored-By: Claude <noreply@anthropic.com>
2 parents f8d11e8 + 0617d45 commit 5b5ee37

6 files changed

Lines changed: 516 additions & 23 deletions

bin/claude-via-proxy.mjs

Lines changed: 97 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -754,8 +754,18 @@ function codeFingerprint(root) {
754754
} catch { return ""; }
755755
}
756756

757+
// BOTH HALVES, because the reaper matches the basenames readdirSync() yields and
758+
// cannot derive them from a joined path. A literal on either side drifts the
759+
// reaper into matching nothing, which collects nothing and reports nothing.
760+
const RECORD_PREFIX = "cache-fix-proxy-";
761+
const RECORD_SUFFIX = ".sha256";
762+
763+
// Shared with the scratch-CA reaper below: the two gates are equal by
764+
// construction, not by a comment claiming they are.
765+
const REAP_AGE_MS = 7 * 86_400_000;
766+
757767
function fingerprintPath(port) {
758-
return join(tmpdir(), `cache-fix-proxy-${port}.sha256`);
768+
return join(tmpdir(), `${RECORD_PREFIX}${port}${RECORD_SUFFIX}`);
759769
}
760770

761771
// Temp + rename: a reader that opens this mid-write would compare against a
@@ -770,10 +780,88 @@ function publishFingerprint(port) {
770780
} catch { /* best effort: an unwritable tmpdir must not stop a proxy starting */ }
771781
}
772782

783+
// Nothing else removes these, and the port is ephemeral wherever the OS picks
784+
// one, so a record accumulates per proxy start without bound.
785+
//
786+
// A PORT THAT STILL ANSWERS OUTRANKS THE CLOCK. Nothing republishes a record —
787+
// its mtime is the last child spawn — so age alone reaps the record of a holder
788+
// that has merely been up a week. runningOurCode() then answers null and
789+
// takeOver() exits 0 announcing a deploy that has not taken effect.
790+
//
791+
// Seven days on top, matching the scratch-CA reaper, bounds what a crashed
792+
// holder leaves behind on a port nobody rebinds.
793+
//
794+
// publishFingerprint's `<record>.<pid>` temp is spared while it is fresh -- a
795+
// pending rename is not litter -- and collected past the same gate, where the
796+
// only thing that leaves one behind is a publish that died.
797+
async function reapFingerprintRecords() {
798+
let seen = 0;
799+
try {
800+
for (const f of readdirSync(tmpdir())) {
801+
// Yield periodically. Nothing awaited below reaches the poll phase, so an
802+
// uninterrupted pass holds the event loop between the bind and the first
803+
// accept — which is the delay deferring this was meant to avoid.
804+
if (++seen % 100 === 0) await new Promise(setImmediate);
805+
if (!f.startsWith(RECORD_PREFIX)) continue;
806+
const isRecord = f.endsWith(RECORD_SUFFIX);
807+
// A rename pends for microseconds, so a `<record>.<pid>` this far over the
808+
// gate is a crashed publish. Nothing else collects it: the suffix test
809+
// alone would skip the name forever.
810+
const isTemp = !isRecord && f.includes(`${RECORD_SUFFIX}.`);
811+
if (!isRecord && !isTemp) continue;
812+
const p = join(tmpdir(), f);
813+
try {
814+
if (Date.now() - statSync(p).mtimeMs <= REAP_AGE_MS) continue;
815+
// Only a record answers to a port. A temp is nobody's to read.
816+
if (isRecord && !(await portFree(f.slice(RECORD_PREFIX.length, -RECORD_SUFFIX.length)))) continue;
817+
// RE-READ: publishFingerprint renames a new record over this path, and the
818+
// probe's await is wide enough to land inside. Losing a FRESH record makes
819+
// runningOurCode() answer null, which holderVerdict() reads as an incumbent
820+
// of ours and takeOver() reports as a deploy that has not landed.
821+
if (Date.now() - statSync(p).mtimeMs <= REAP_AGE_MS) continue;
822+
rmSync(p);
823+
} catch { /* raced, gone, or refused; a survivor is disk, not correctness */ }
824+
}
825+
} catch { /* unreadable tmpdir: publishing already degraded, say nothing more */ }
826+
}
827+
828+
// ASKED BY BINDING, NOT BY lsof: holderPidOn needs a pid and has to shell out,
829+
// this needs one bit, and a host without lsof would otherwise make the reap a
830+
// silent no-op. A name whose port is not a number is not ours to judge.
831+
//
832+
// THREE THINGS THIS DOES NOT ANSWER, all narrow, none free to close here:
833+
//
834+
// 1. "Is anything LISTENING" is not "is anyone using this port". A holder in the
835+
// bound-but-not-listening state this file creates on purpose reads as free,
836+
// so an over-age record can be lost in the window before its relay takes over.
837+
// 2. A record is keyed by PORT ALONE, and this asks about bindAddr(). A holder on
838+
// another address reads as free to a reaper on loopback. Probing the wildcard
839+
// would close it and open a worse one — an externally reachable socket per
840+
// over-age record — and carrying the address would change the record format.
841+
// 3. The probe is itself a listener while it asks, so a launcher in otherHolderOn()
842+
// can read it as an incumbent, print "this one is surplus" and settle(0),
843+
// leaving the port empty. Needs a record for the exact port a second launcher
844+
// is binding, i.e. one idle seven days and then reused.
845+
//
846+
// All three end in the same place: runningOurCode() answers null and takeOver()
847+
// exits 0 announcing a deploy that has not taken effect. The local variant of 3
848+
// cannot happen — listen() is the last synchronous statement of holdPort's
849+
// executor, so this launcher already owns its own port when the reap runs.
850+
function portFree(port) {
851+
const n = Number(port);
852+
if (!Number.isInteger(n) || n < 1 || n > 65535) return Promise.resolve(false);
853+
return new Promise((res) => {
854+
const s = net.createServer();
855+
s.once("error", () => res(false));
856+
s.listen(n, bindAddr(), () => s.close(() => res(true)));
857+
});
858+
}
859+
773860
// TRUE, FALSE, or NULL for "cannot tell" — a third state because the callers
774861
// must be able to TELL unknown apart, not because they answer it differently.
775862
// Both end at exit 0; only one of them says why (see otherHolderOn). Unknown is
776-
// ordinary: the record lives in /tmp, which systemd-tmpfiles sweeps.
863+
// ordinary: the record is age-reaped (see reapFingerprintRecords) and a host
864+
// with a /tmp sweeper clears it too.
777865
function runningOurCode(port) {
778866
let theirs = "";
779867
try { theirs = readFileSync(fingerprintPath(port), "utf8").trim(); } catch { return null; }
@@ -817,6 +905,12 @@ function holdPort(rest) {
817905
for (const s of [process.stdout, process.stderr]) {
818906
s.on("error", () => { /* the reader left; putting the proxy back is the job */ });
819907
}
908+
// Here, not in publishFingerprint: that returns early when the fingerprint is
909+
// unreadable, which would stop the reap exactly when publishing is broken, and
910+
// it runs on every respawn. Deferred and unref'd because the scan walks the
911+
// whole tmpdir, nothing waits on its result, and it must neither delay the
912+
// bind nor hold the process open.
913+
setTimeout(reapFingerprintRecords, 0).unref();
820914
// The proxy's own default: holding a different port than the proxy would have
821915
// served leaves nothing at the documented address.
822916
// `|| 9801` REWROTE PORT 0 to 9801. "0" is a truthy string so the run-service
@@ -2312,11 +2406,10 @@ if (remoteControl) {
23122406
// unique; it guarantees nothing about what else shares a prefix, so the two
23132407
// sites read one constant rather than two matching string literals.
23142408
try {
2315-
const scratchAgeMs = 7 * 86_400_000;
23162409
for (const f of readdirSync(tmpdir())) {
23172410
if (!f.startsWith(SCRATCH_PREFIX)) continue;
23182411
const p = join(tmpdir(), f);
2319-
try { if (Date.now() - statSync(p).mtimeMs > scratchAgeMs) rmSync(p, { recursive: true }); }
2412+
try { if (Date.now() - statSync(p).mtimeMs > REAP_AGE_MS) rmSync(p, { recursive: true }); }
23202413
// Someone else's, already gone, or REFUSED (dir mode 0500, measured).
23212414
// A survivor is litter under tmpdir() that nothing reads — unlike a
23222415
// refused delete that leaves a live state armed, which would need code.

test/proc-helpers.mjs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,12 @@ export const HOP_ENV = ["HTTPS_PROXY", "https_proxy", "HTTP_PROXY", "http_proxy"
124124
// once, the oldest 788 s, accumulating across files and runs until a later
125125
// file's readiness assertion times out on the CPU and ports they hold. See
126126
// ours() for the mechanism and the two markers it reads.
127-
export const onPort = (port) => [...new Set([...listeners(port), ...ours(port)])];
127+
// Port 0 is the "assign me one" sentinel, never a port anything holds -- and it
128+
// is what a caller's `let port = 0` still carries if it throws before the
129+
// assignment. ours() would then match every proxy child running with
130+
// CACHE_FIX_PROXY_PORT=0, the operator's live one included.
131+
export const onPort = (port) =>
132+
Number(port) > 0 ? [...new Set([...listeners(port), ...ours(port)])] : [];
128133

129134
// The HTTP health probe every holder-wait fixture used to hand-roll, once. THE
130135
// STATUS is what answers, not merely a reply: a standby relay carrying this

0 commit comments

Comments
 (0)