fix(launcher): reap the fingerprint records the proxy leaves behind - #345
Open
codeslake wants to merge 11 commits into
Open
fix(launcher): reap the fingerprint records the proxy leaves behind#345codeslake wants to merge 11 commits into
codeslake wants to merge 11 commits into
Conversation
Every proxy start writes cache-fix-proxy-<port>.sha256 into the temp dir and
nothing removed it. The port is ephemeral wherever the OS picks one, so the
records accumulate one per start without bound. runningOurCode() called that
ordinary because "systemd-tmpfiles sweeps /tmp" — a deployment assumption, not
a guarantee, and false in a container. Measured on one with zsh as PID 1 and no
sweeper: 6,743 records, 461 a day.
The cost is not the 284 KB. The launcher's own scratch-CA reaper walks
readdirSync(tmpdir()) on every start, and that scan measured 117 ms at 74,493
entries — this litter is what fills it, so the leak taxes the startup path meant
to clear it.
Seven days, matching the scratch reaper: publishFingerprint has one call site,
the spawn path, and nothing republishes, so an unrestarted holder's mtime is its
launch time. A shorter gate deletes a live holder's own record, runningOurCode()
answers null, and takeOver() exits 0 — the "swept /tmp turned a deploy into a
no-op that read as a success" incident, caused by us this time.
Driven from holdPort rather than publishFingerprint, which returns early when
the fingerprint is unreadable (reaping exactly when publishing is persistently
broken is the mistake the CA reaper documents) and runs on every respawn.
Deferred with setTimeout(...).unref() because the scan is not free and nothing
waits on it: 135-193 ms at this box's 68,533 entries. Inline it delays the bind
— interleaved against the merge base at one load, proxy-held-port.test.mjs
failed 2 of 10 runs with the scan inline and 0 of 5 with the same diff's scan
body disabled, against 0 of 14 for the base.
RECORD_PREFIX is shared by the writer and the reaper. Deriving it at runtime
read lastIndexOf("/"), which is -1 on Windows: the whole absolute path became
the prefix, no basename from readdirSync ever matched, and the reaper was a
silent no-op there.
Ref cnighswonger#304 — the record and the systemd-tmpfiles comment both arrived in 84ba7c2.
Co-Authored-By: Claude <noreply@anthropic.com>
Comments explain the code, not how it was found. The reap block carried the census that motivated it, the timings that sized it, the A/B table that placed the call site, and two quoted incidents; the test carried the same again. All of it is in 92d2c08's message and in the PR, where it is read once rather than on every visit to this file. What stays is what a reader needs in order not to break it: why seven days rather than one, why the call sits in holdPort rather than publishFingerprint, why it is deferred, and why the prefix is one shared constant. Production comment lines 36 -> 25; no behaviour change, tests unchanged and green. Co-Authored-By: Claude <noreply@anthropic.com>
Age alone made the seven-day gate a deadline rather than a margin. Nothing republishes a record — publishFingerprint has one call site, the spawn path — so a holder that neither respawns nor is redeployed for a week is fully live with an over-age record, and the next launcher to start deleted it. runningOurCode() then answers null, holderVerdict returns "holder", and takeOver() exits 0 announcing a deploy that has not taken effect: the incident the launcher already documents, made reachable by the fix meant to prevent litter. Measured end to end before this commit: a scratch TMPDIR holding an 8-day-old record for a live holder's port, one unrelated run-service start, record gone. So ask lsof, which the launcher already relies on for holderPidOn and otherHolderOn and which works on macOS. One call, made only once a record is actually eligible, so a swept host still pays nothing but a readdir. A probe that cannot answer keeps everything rather than reading "could not ask" as "nothing is listening" — that reading would hand the reaper every record on the box. The age gate stays, now bounding what a crashed holder leaves on a port nobody rebinds rather than standing alone. Two comment claims corrected. The scratch-CA reaper does not share a walk with this one: it sits after `await dispatch()` and runs in wrapper mode only, never in a run-service holder. And "age is the only discriminator available" was false — a listening-port set was one lsof away, which is what this commit uses. Tests: a live-listener case, a probe-cannot-answer case, and a case that spawns a real run-service and waits for a planted stale record to vanish. That last one exists because every other case runs lifted source and none of them can see whether the launcher ever calls the reaper — commenting the call out left them all green. Co-Authored-By: Claude <noreply@anthropic.com>
The case added to prove the reaper is reachable spawns a run-service and SIGKILLs it. run-service leaves a DETACHED standby gap-relay that stands down only for a claimant's SIGHUP, so killing the launcher reparented it to init still holding an ephemeral port. Measured: one per run, eight alive at once here, the oldest over three hours, each one a listener in the range the suite's own fixtures bind. proc-helpers exports onPort for exactly this and its comment already records the same incident from proxy-held-port. Not importing it was the whole defect. Holder first, then whatever is left on the port, which is the order that file's sweep documents. Measured after: orphan delta 0 across three consecutive runs, and no ccf-fpreap-* scratch left behind. suite-collection gains a guard for it, named rather than swept. A form-based sweep — every .test.mjs carrying both "run-service" and SIGKILL — was tried and measured: it also flags proxy-probe-bounded and stdio-epipe-survival, whose orphan delta over a full run is 0, because they spawn a holder that never reaches the standby. The predicate describes the shape of the code rather than the debt it incurs, and a guard that reds two innocent files is one someone deletes. Co-Authored-By: Claude <noreply@anthropic.com>
CI went red on Node 18 and the cause was not the Node version — it was lsof. Reproduced locally by running the file with lsof off PATH: 3 of 6 cases fail, the same failure CI reported. listeningPorts() shelled out, a missing lsof threw, and the fail-closed branch then kept every record, so the reaper became a silent no-op. The leak fix would have done nothing on any host without lsof, which is exactly where nobody would look for it. A bind answers the same question with no external tool. holderPidOn needs a PID and has to shell out; this needs one bit, and the kernel gives it directly. It is also exact where lsof was not: lsof sees only this uid, so a holder under another account read as "nothing is listening". The reap was already deferred with setTimeout and nothing awaits it, so making it async costs nothing. Every path still catches, so there is no unhandled rejection to leak out of the timer. A name whose port is not a number is kept rather than judged — listen(NaN) throws ERR_SOCKET_BAD_PORT rather than answering, and cache-fix-proxy-healthcheck.* is a name this reaper has no business deciding about. Measured: 6/6 with lsof present, 6/6 with lsof off PATH. Mutations killed — removing the port guard, and inverting the non-numeric case to reap. Co-Authored-By: Claude <noreply@anthropic.com>
The guard added alongside the orphan fix named proxy-fingerprint-reap directly, which is the same blindness the guard above it already had: it goes stale the moment a third file learns the debt. It now scans every .test.mjs that spawns a launcher and SIGKILLs it, and accepts any spelling of the sweep — onPort(), listeners(), or a raw lsof -iTCP. stdio-epipe-survival sweeps with the last of those, and a predicate that only knew the first would have called it an offender. A file that kills a launcher and genuinely owes no sweep says so with NO-STANDBY: and why. proxy-probe-bounded is the one: the hang under test is a probe, so the launcher blocks before it ever binds and there is no detached standby to reparent. Measured with its deadline forced to 200 ms, 3 s and 6 s — orphan delta 0 at all three. Measured over all 24 test files: 5 sweep, 1 exempt, 0 offenders. Mutations killed — removing the onPort sweep reds it, and removing the marker reds it naming proxy-probe-bounded. Co-Authored-By: Claude <noreply@anthropic.com>
Two claims in the reap comments were stronger than the code. It tests LISTEN, not ownership: a holder in this file's deliberate bound-but-not-listening state reads as free, which is reachable in the ~80 ms before the gap relay boots. And the loop does not yield — measured at 3,000 over-age records it held the event loop for 176 ms, because listen and close resolve on nextTick and the await never reaches the poll phase. Neither changes what the code should do. Both would have been inherited as fact. Co-Authored-By: Claude <noreply@anthropic.com>
Three claims were stronger than what had been measured, all in permanent records. 4411af4's message said "measured over all 24 test files: 5 sweep, 1 exempt, 0 offenders". That number came from a review summary rather than from running the predicate. Run here over the guard's own testDir: 114 .test.mjs, 7 candidates, 6 sweep, 1 exempt, 0 offenders. The guard was right; the census reporting it was not, and it omitted proxy-fingerprint-reap itself. The "~80 ms before the gap relay boots" was borrowed from a nearby comment that measured the proxy CHILD's boot, a much larger spawn. Nothing measured the relay, so the comment now says the window is unmeasured rather than naming a number. And portFree is itself a listener while it asks. A launcher starting concurrently runs otherHolderOn(), which selects on a LISTEN socket plus a run-service command line plus greater uptime; a peer mid-probe can satisfy all three and be read as an incumbent. Bounded by the bind lifetime, under 59 µs per record, and now stated. Co-Authored-By: Claude <noreply@anthropic.com>
The predicate accepted `-iTCP` as a spelling of "sweeps the port", and stdio-epipe-survival passed on it. That file's `lsof -iTCP` is its STIMULUS — it kills the proxy child mid-body to provoke a restart log, three lines above the assertion — while its actual cleanup is `t.after(() => reap(holder))`, a process-GROUP kill. The file was protected the whole time, by a mechanism the guard could not see, and passed on a coincidental substring. A group kill reaps the standby as a child of the group without ever naming a port, so it discharges the same debt. The predicate now names that instead of a third spelling of lsof, and the comment says which three mechanisms count and why they look nothing alike. Measured: all 24 candidates reclassify identically (6 sweep, 1 exempt, 0 offenders), and removing the group kill from stdio-epipe-survival now reds the guard naming that file. Co-Authored-By: Claude <noreply@anthropic.com>
… code
CI went red on Node 22 with `body.startsWith is not a function`, inside "refuses
nothing when the proxy under it dies". Six of this file's seven probes resolve
`ERR:${e.code}` — a string with the prefix classify() tests for. The seventh, at
the forced-kill case, resolves a bare `r.statusCode` on success and a bare
`e.code` on error. Its caller filters out 200 and hands everything else to
classify(), so a 502 arrives as a Number and the case dies where the answer is
simply "that was a reply, not an outage".
The path only opens when a non-200 is actually observed, which is why it survived
every local run and three CI matrices before this one. A unit case pins it now:
a status code classifies as null, an ERR: string still classifies, and a bare
ECONNRESET with no prefix stays null.
Measured: removing the type guard reds that case with the exact CI message.
Nothing else on this branch touches this file — the crash predates it and was
merely surfaced here. It rides along rather than waiting behind its own PR
because leaving this branch red would mean explaining the red in a comment and
making the two land in a fixed order, for a five-line guard.
Co-Authored-By: Claude <noreply@anthropic.com>
codeslake
marked this pull request as ready for review
August 20, 2026 09:20
A hardcoded port sits inside the kernel's ephemeral range, so a sibling test's launcher can be handed it and hold it for a whole run. portFree then answers false, the reaper correctly keeps the record, and only this file is wrong — a red that reads as flake. Measured: occupying 40404 turns "a record whose port still has a listener is kept however old it is" red on demand and leaving it free turns it green; occupying 40808 exhausts the e2e case's 25s deadline. Across ten interleaved suite runs these were the only two failures, and they repeated rather than wandered, which is what separates them from the load-shaped reds this suite also has. The three cases whose records must actually be reaped now take a port the kernel has just released. The four fixtures that never reach portFree — kept by the age gate or the suffix filter — move below the ephemeral floor so they cannot collide with the derived one. The e2e case reuses the same helper instead of its own inline copy. Mutation table unchanged: forcing portFree true still kills two cases, removing the age gate still kills one. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft. The review loop is still running on this branch; I will mark it ready when it exits clean.
What leaks
publishFingerprint(port)writescache-fix-proxy-<port>.sha256into the temp dir before each spawn, and nothing removed it. The port is ephemeral wherever the OS picks one, so the records accumulate one per proxy start, without bound.Measured on a long-lived container: 6,743 records, none older than 14.6 days only because the box had not run this code that long.
That rate is not a production leak rate, and it would be wrong to read it as one. Counted by hour on a dev box, creation is bursty and tracks test runs: 1,443 of 2,145 records in a single hour, the hour a full suite ran repeatedly. What the number establishes is that nothing removes them and the directory only grows — not how fast a user's machine fills. A production host spawns a proxy when a session starts, not a thousand times an hour.
runningOurCode()calls an unreadable record ordinary because "the record lives in /tmp, which systemd-tmpfiles sweeps". That host haszshas PID 1, no systemd, and no sweeper at all. The comment describes a deployment assumption, not a guarantee, and it is false wherever the proxy runs in a container. This PR replaces that sentence too.Why it is worth code
Not the 284 KB. The launcher's own scratch-CA reaper walks
readdirSync(tmpdir())on every start; that scan measured 117 ms at 74,493 entries, and this litter is what fills the directory. The leak taxes the startup path that exists to clean up after it.The shape, and why each part is the way it is
Seven days, matching
SCRATCH_PREFIX's reaper.publishFingerprinthas exactly one call site — the spawn path — and nothing republishes. So an unrestarted holder's record mtime is its launch time, and a shorter gate deletes a live holder's own record:runningOurCode()→null→takeOver()exits 0, which is the "swept /tmp turned a deploy into a no-op that read as a success" failure the launcher already documents. Age is not the only discriminator, and relying on it alone is what the second commit had to undo — see below.Driven from
holdPort, not frompublishFingerprint. That function opens withif (!fp) return;, so reaping from there stops exactly when publishing is persistently broken — the mistake the sibling CA reaper spells out. It also runs on every respawn, on a ladder this file measures at 51 spawns in 1.2 s.Deferred with
setTimeout(..., 0).unref(). The scan is not free (135–193 ms at this box's 68,533 entries) and nothing waits on its result. Run inline it delays the bind. Interleaved against the merge base at one load:test/proxy-held-port.test.mjs, a different case failing each time — the signature of a slower startup rather than a broken branch.unrefso the timer can never hold the process open.A port that still answers outranks the clock. Age alone made the seven-day gate a deadline, not a margin: nothing republishes a record, so a holder that neither respawns nor is redeployed for a week is fully live with an over-age record, and the next launcher deleted it. Measured end to end before the fix — a scratch
TMPDIRholding an 8-day-old record for a live holder's port, one unrelatedrun-servicestart, record gone.So a record is removed only when it is both over-age and its port does not answer.
portFree()binds the port withnet.createServer()onbindAddr(); a non-integer port resolves false and is kept.Asked by binding, not by
lsof. The first version of that check shelled out, and upstream CI went red on Node 18 — not because of the Node version, but because that runner had nolsof. Reproduced locally by removinglsoffromPATH: 3 of 6 cases fail, the same failure CI reported. The fail-closed branch then kept every record, so the reaper became a silent no-op on any host withoutlsof— exactly where nobody would look for it. A bind asks the kernel directly and is also more exact:lsofsees only this uid.Two things the bind check does NOT do, both measured and both now stated in the code: it tests LISTEN rather than ownership, so a holder in this file's deliberate bound-but-not-listening state reads as free; and the loop does not yield — 176 ms of held event loop at 3,000 over-age records.
RECORD_PREFIXshared by the writer and the reaper. The first version derived the reaper's prefix fromfingerprintPath("")at runtime vialastIndexOf("/"). That is-1where the separator differs, so the whole absolute path became the prefix and no basename fromreaddirSyncever matched — not destructive, but a silent no-op on a supported target.Verified end to end
Real launcher, isolated
TMPDIR, ephemeral port: an 8-day-old record removed, a fresh one kept, a neighbouringcache-fix-ca-scratch-keepme.sha256kept, and its own record published. The deferred timer does fire.Full suite interleaved against the merge base, 3 rounds each: 1949 pass / 0 fail on the branch, 1946 / 0 on the base. CI green on Node 18, 20 and 22.
Tests
test/proxy-fingerprint-reap.test.mjslifts the shipped function out of the launcher and runs it withtmpdir()rebound to a scratch dir, the wayproxy-held-port.test.mjsliftsholderPidOn. It asserts on every slice, so a rename fails the test instead of quietly testing nothing.Mutation-checked, each break made fresh against the final code:
RECORD_PREFIX = ""RECORD_PREFIX = "cache-fix-"(one segment short)InfinityholdPortpublishFingerprintfingerprintPathback to a literallsof-failure fallback changed to an empty Set (before the bind rewrite)The negative control is named
cache-fix-ca-scratch-keepme.sha256on purpose: with any other suffix theendsWith(".sha256")check alone would save it and an empty prefix would pass.Also carried here: a crash this branch surfaced but did not cause
test/proxy-held-port.test.mjshas seven inline health probes. Six resolve`ERR:${e.code}`— a string carrying the prefixclassify()tests for. The seventh, at the forced-kill case, resolves a barer.statusCodeon success; its caller filters out 200 and hands the rest toclassify(), which opens with.startsWith. A 502 arrives as aNumberand the case dies.CI Node 22 hit it on this branch, which touches neither that file nor
classify(). The path only opens when a non-200 is actually observed during the forced kill — every local run and three earlier CI matrices had none.Fix is
if (typeof body !== "string") return null;plus a unit case pinning four answers (a status code is a reply, not an outage). Measured: removing the guard reds that case with the exact CI string.It rides along rather than waiting behind its own PR because leaving this branch red would mean explaining the red in a comment and fixing the order the two had to merge in, for a five-line guard.
Anti-bloat numbers
Per
AGENTS.md, stated rather than asserted:test/proxy-fingerprint-reap.test.mjs)The smallest change that fixes the stated defect is a ~10-line sweep loop; the port discriminator, the bind fallback and the shared prefix are the rest, and this is 26 lines of code. The comments carry why seven days, why
holdPortand notpublishFingerprint, and why deferred — the three things a reader would otherwise undo. The second commit deletes the investigation that had accumulated around them; the evidence lives here and in the commit messages instead.Ref #304 — the record and the
systemd-tmpfilescomment both arrived in 84ba7c2.— Proxy Builder
🤖 Generated with Claude Code