fix(holder): a handover must not freeze the config it was started with - #356
Open
codeslake wants to merge 43 commits into
Open
fix(holder): a handover must not freeze the config it was started with#356codeslake wants to merge 43 commits into
codeslake wants to merge 43 commits into
Conversation
SIGUSR2 hands the listening socket to a successor spawned with
`{ ...process.env }`. That is faithful, and therefore stale: a CACHE_FIX_*
switch added after a holder started could not reach it without releasing the
address, and releasing it cuts whatever is streaming across the gap. The
switches were reachable only by an outage.
The successor now re-reads CACHE_FIX_* settings from a file the holder does not
own -- ${CLAUDE_CONFIG_DIR:-~/.claude}/cache-fix-handover.env, relocatable with
CACHE_FIX_HANDOVER_ENV -- and lets them win over what it inherited. Under
claudeHome() rather than a global path, for the reason proxy/claude-home.mjs
gives: one proxy per config dir, so a single box-wide file would make two of
them share one override.
EVERY FAILURE IS OFF. Absent, unreadable, malformed, unrecognised: the
inherited value stands. A handover that refuses over a bad config file is worse
than one carrying a stale switch.
Four keys are pinned over anything the file says, and each is a distinct
failure:
PROXY_PORT, HOLDER_HANDOVER, EXIT_WITH_PARENT -- already pinned; the file
must not reach them.
PROXY_BIND -- new. The successor ADOPTS fd 3, so a bind written in the file
cannot move the socket, but listen() still stores it as _host and every
downstream label is built from that: HELD_HOST for the gap relay and the
standby, PROXY_BIND for the proxy child, /health's config.bind. gap-relay
builds its self-exclusion list from HELD_HOST, and this file already
records what a wrong value there costs (22 -> 8,195 -> 29,814 descriptors).
Ignored would be fine; a right socket under wrong names is not.
STANDBY -- new, and reachable only because of this change. The successor
carries it into openGap(), whose env sheds HOLDER_TREE and HELD_BY but not
this one; gap-relay then takes the standby branch and refuses for want of
STANDBY_PARENT. An armed gap relaying nothing, in the window it exists to
cover.
Parsed by hand rather than util.parseEnv: measured undefined on node 18, which
package.json declares as the minimum, and the call would throw outside the
guard that makes every failure off.
DELIBERATELY NOT INCLUDED: proxy/server.mjs's own fd-3 successor on SIGTERM has
the same freeze and the same shape. It belongs in a later change and only
alongside a holder-side refresh -- the holder respawns its child from its own
env on every self-heal, so a value picked up by the child alone would revert at
the next respawn, and a setting that flaps between restarts is harder to reason
about than one that is plainly stale.
Also not fixed, and not fixable here: stdio. `inherit` passes file descriptors,
not environment, so a lineage started with its stderr on /dev/null keeps it
however this file is written.
Measured: full suite 1954 tests / 1953 pass / 0 fail / 1 skipped. Mutation
table, each run fresh against the restored tree, each killing exactly one test:
spread moved after the pinned keys 23/1; bind pin removed 23/1; STANDBY clear
removed 16/1; default path frozen at import 23/1; claudeHome() replaced by a
global path 23/1; restore clean. The undefined-drops-the-key mechanism is
measured directly, not assumed: a parent holding CACHE_FIX_STANDBY=1 spawns a
child that reads it as undefined.
Co-Authored-By: Claude <noreply@anthropic.com>
The drain budget had two arms and three cases. `handedOff` means the proxy spawned its own fd-3 successor; everything else took the 5s that was measured for a supervised stop, where the supervisor waits serially and a longer grace is pure downtime. A HOLDER handover reached this code as neither: the holder signalled its proxy child, that set `releasing`, `askForSuccessor` went false, and `handedOff` never became true. So the axe fell on a path where nothing supervises anything — the successor holder had already adopted fd 3 and placed its own proxy, and this process had released the listening socket a few lines up. That is the lingering predecessor the long budget exists for. The defect is conflating "I did not spawn the successor" with "there is no successor". Those come apart on exactly one path. WHY THIS NEEDED A NEW SIGNAL. The obvious repair is to widen the condition to `releasingPort`, which is true on the handover path. It is wrong, and the measurement that says so is thirty lines below the line it changes: the holder's `forward()` rewrites EVERY stop to SIGHUP — a supervisor stop, an interrupt, a plain kill, and a takeover asking it to release the port all arrive as one word — and `releasingPort` is set by all of them. Widening to it gives a plain service stop the 30-minute budget, which was measured at 120s against a 90s TimeoutStopSec: SIGKILLed at the cap, restart downtime 5.0s -> 53.9s. A receiver cannot recover a distinction the sender flattened, so the sender gets a second word. The handover path now signals SIGUSR2; every stop path still speaks SIGHUP; the proxy's SIGUSR2 handler is the only thing that sets the flag the budget reads. `releasingPort` is out of the budget entirely, so no stop path can reach the long arm by construction. SIGUSR2 IS FATAL TO A PROXY THAT PREDATES THE HANDLER — node's default action terminates, cutting everything with no drain, which is worse than the 5s this removes. Two invariants keep it unreachable, and each has its own test because neither implies the other. The target is always ours: the only assignment that creates a `child` spawns SERVER_PATH from beside the launcher, so it ships with the handler (SERVER_PATH is the single point a harness rewrites to redirect the proxy, which is why that resolution is asserted too). And the signal has exactly one delivery site: the two sends in the file are the handover's own child, and a holder-to-holder request already gated on the target being a run-service holder. Adopting a foreign proxy, adding a third sender, or dropping that gate each turn the suite red. Measured over one evening of six handovers, from the proxy's own log: every `proxy listening` line is followed 2-4 lines later by `forcing close, cut N in-flight request(s) after 5s` — 17/18/18/16/15/14, all mid-response, 0 before headers. 6 boots, 6 cuts, 1:1, no exceptions. That six is a floor, not a count: the log had already been truncated at its 4 MB cap. Corroborated from the client end by a scan of 14713 transcripts (control 14470) finding user-visible "Streaming response ended before any complete data was received" events in independent sessions, two matching a proxy boot to within 10ms, and zero such events on two other hosts scanned with live controls. The fix is the branch half only. The handover arm still inherits a 1,800,000 ms CLOCK, and a clock has been retuned three times in a sibling component (30s -> cut 16, 600s -> cut 12, 1800s -> cuts a 31-minute reply). Ageing each owed connection from its own last byte is the real predicate and is not in this commit; the paragraph below the budget that calls such a predicate impossible is arguing against a throughput threshold, not against last-byte age, and has to be rewritten with it. Not covered: a holder stop with requests in flight. The existing holder cases signal with nothing in flight, so they pass under either budget. Mutation-tested, each on a distinct assertion: removing the handover term, weakening the disjunction, collapsing the condition, widening the supervised arm to the long budget, letting SIGHUP set the handover flag, sending the handover as SIGHUP again, letting the holder adopt a child it did not spawn, adding a third SIGUSR2 sender, and dropping the run-service gate. Co-Authored-By: Claude <noreply@anthropic.com>
codeslake
force-pushed
the
fix/handover-rereads-config
branch
from
August 25, 2026 02:08
bbf99a4 to
b1b1409
Compare
The file has no syntax that removes a key, so deleting a line does not turn a
switch off — the outgoing holder's value stands, and a switch set through this
file is then sticky across every later handover. That is the right default (a
handover must never lose config it was not asked to lose) but it is the opposite
of what deleting a line looks like it does.
Documented at the parser and pinned by a test, rather than given an unset
syntax: the off value each switch already reads is the way off, and it works
today. Mutation: starting the result from `{}` instead of the inherited env
turns the test red.
Co-Authored-By: Claude <noreply@anthropic.com>
The holder forwards our stdout to the log AND parses it for this line, so on a holder-driven handover — the one where that holder has already settled and is leaving — the line reaches neither. Measured on one host: 8 startup stdout lines present, 0 of these, while every stderr line of the same shutdown block is there. Anyone grepping `(handed off)` to separate a handover from a stop reads "no handover ever happened" on the machine that has had nothing else. No behaviour change, and none is needed: on that path the line has no reader by construction, and the arm is recoverable from stderr on both outcomes — a cut says `after <budget>s`, a completed drain says `of <budget>s budget`, and 5 against 1800 is the arm. The comment is so the next reader does not build on the stdout copy. Co-Authored-By: Claude <noreply@anthropic.com>
The handover arm had a 30-minute ceiling and it cut anyway. Measured on one
host, same code, consecutive handovers:
after 5s cut 14, 15, 16, 17, 17, 18, 18 every one mid-response
after 1800s cut 13 every one mid-response
Thirty minutes bought four completions. A neighbouring component retuned the
same number three times — 30s, 600s, 1800s — and was wrong after each one,
because a budget is a bet on how long a reply takes and it sits in the middle of
a real distribution of reply durations. Every value of it cuts someone.
So the handover arm no longer has a ceiling. It ends when no owed connection has
written a byte for the stall window, and `CACHE_FIX_DRAIN_MS` survives only as a
backstop against a bug in that test — if it is what fires, the log says so in
those words, because a drain that reaches the backstop means the predicate never
answered and that is a defect here, not a slow client.
WHY BYTES AND NOT A RATE. This file used to say no honest predicate existed, and
that sentence is why the answer stayed a number for as long as it did. What it
actually refutes is a THRESHOLD ON THROUGHPUT: a peer measured content at
490 B/s against a heartbeat at 35 B/s on one stream, and no cutoff separates
those. This test never divides anything — a reply and a heartbeat both answer
"moving", which is correct for both, and what it excludes is a connection
delivering nothing at all. That is a different shape, not a smaller number of
the same one. The peer's 292 drains: content-free waits reach 186s while
byte-free waits top out at 2s, two populations with nothing in between, which is
why their 90s has never been retuned while every budget has. The paragraph is
corrected in place rather than deleted — the wrong version is the kind that
stops the next reader from looking.
90s is a judgement call bounded by two observations, not a percentile; their
byte-free sample is n=6. Said so at the constant, because a number presented as
derived when it was not is worse than an honest guess.
Read by polling `socket.bytesWritten` during the drain rather than stamping
every write, so the hot path pays nothing. It is also the only thing that
separates a streaming reply from one blocked upstream: `headersSent` goes true
at writeHead with bytesWritten still 0, which is why the `mid-response` count in
the forced-close line is an upper bound and this is not.
The supervised arm is unchanged at 5s. It is a real ceiling — the supervisor
waits serially there, and 120s against a 90s TimeoutStopSec took restart
downtime from 5.0s to 53.9s.
Two functional cases, and they only mean anything as a pair: a drain with
nothing moving must end without reaching the ceiling, and a drain with bytes
moving must not end while they move. Either alone is satisfied by a shorter
clock. Mutation-tested: dropping the movement reset, scoring elapsed instead of
stalled, and widening the stall window to infinity each turn one of them red.
The first cut of the movement fixture delivered no bytes at all and was caught
by its own premise assertion rather than passing while proving nothing.
Co-Authored-By: Claude <noreply@anthropic.com>
…test Two gaps left over from the drain work, plus a stale number in the comment that would have invited the next reader to make things worse. NAME THE ROUTES. The cut line carried a count and nothing else, so nobody reading it could say what was lost. This port carries CLI turns alongside bridge traffic, quota polls, statusline and title generation, and only the first kind is a reply a person is watching. One host measured roughly 98 cuts against 6 user-visible events, and neither of the two sessions looking at it could name the other ninety-two. The line now ends `routes: /v1/messages=11 /api/claude_cli=2`. Grouped to two path segments with everything after `?` discarded. The proxy sees whole request URLs and this line goes to a log that outlives the process, so the grouping is what keeps an identifier in a path out of it; it also bounds the cardinality, which a per-URL tally on a passthrough route would not. The tally rides the cut line only — on the no-cut line it would be a list of things that were not cut. A caller that passes none gets no `routes:` field at all rather than an empty one, so every existing line is byte-identical. THE OPERATOR STOP HAD NO TEST WITH ANYTHING IN FLIGHT. Every holder case here signalled with nothing owed, so `close()` resolved at once and they passed under any budget: a ceiling was never reached and a stall test was never asked. That left the one path an operator actually takes — a supervisor stop, an interrupt, a plain kill — unmeasured with a reply mid-delivery. It matters because the holder rewrites every stop to SIGHUP, and if that flag ever reaches the handover arm again an operator stop inherits the uncapped drain and is SIGKILLed at TimeoutStopSec. The new case streams a reply through a real holder, stops it, and requires both that the reply was still moving at the moment of the stop and that everything left the port well inside the supervised bound. Restoring the earlier `|| releasingPort` shape turns it red; before this, nothing did. THE STALL COMMENT CITED A CEILING THAT HAS SINCE MOVED. It said byte-free waits top out at 2s over 6 samples. A real reply on a busy host has since gone 23s without a byte and finished clean. The default is unaffected and stays at 90s — the two modes still do not overlap — but the margin is 3.9x rather than 45x, and the comment justified itself by distance from the observed maximum, which is an invitation to tighten. It now says not to, and why: bimodality is what lets you pick a threshold without knowing n, and it is not what tells you your margin. Only n does that, and every sample under 2s came from a quiet host. Mutation-tested: restoring `|| releasingPort` on the handover arm, and dropping the query-string cut from the route grouping, each turn a distinct case red. Both mutations assert their own anchor count before running, because three times tonight a mutation that failed to apply printed a pass. Co-Authored-By: Claude <noreply@anthropic.com>
The stall predicate held one shared `lastMoved` and reset it whenever ANY
owed connection moved a byte. On a port with traffic that clock never
expires: one live stream answers "moving" on behalf of every connection,
so a stalled one is never asked about and never ages. Its first firing in
production cut 6 in-flight requests on the 1800s backstop with the stall
test never having fired once.
Per-connection stamping alone does not fix it. The cut was all-or-nothing,
so a predicate that ends the whole drain on the first quiet connection
takes the live ones with it, which is the same zero-interruption violation
with the sign flipped. Both moved: each owed connection carries its own
stamp, one quiet for the window is ended alone, and the drain ends when
nothing is owed or on the backstop.
Connections are aged from arrival (`res._bornAt`), not from first sight in
the drain loop. A connection already stalled when the drain began has no
byte to date from, and stamping it at first sight hands it a fresh window
it has not earned.
The label is read before the connection is ended: `res.end()` on a
response with no header WRITES one, so asking afterwards reports
"mid-response" about the request that was blocked upstream, the one case
the label exists to separate.
Evidence, all on one linux host:
RED the new case on the aggregate code, failing on "the stalled
connection was still open"; both premises passed, so the
fixture is sound
GREEN 15/15 in test/shutdown-exit-code.test.mjs
MUTATION stamping kept, cut made aggregate again -> killed by the
"drain has not returned" assertion
The new case needs two connections AND three assertions. Both existing
drain cases use one connection, and with one connection "did anything
move" and "did this one move" are the same question, so neither fixture
could fail either implementation. The third assertion is what sees the
all-or-nothing close: a case that checks only "the stalled one died" is
blind to it. A two-connection fixture that computes whether the stalled
one ended and never asserts on it passes on the unfixed code, measured.
Not independently mutation-killed: the "moving one is still delivering"
assertion. The mutation above died on the assertion after it.
Co-Authored-By: Claude <noreply@anthropic.com>
`res._bornAt` was added with the per-connection drain and no case in the
suite could tell it from stamping at first sight: every other fixture
opens its connection moments before the handover, where arrival and first
sight are the same instant. A guard no test kills is one the next reader
deletes.
This case idles a connection for 6s against a 4s window before signalling,
so an age-from-arrival test is already satisfied when the drain begins.
fixed code ended 2s after the handover (stamped on tick 1
carrying its real age, ended on tick 2)
stamp at first sight ended 5002ms after the handover, having bought
itself a fresh full window
Whole file 16/16 with the guard in place.
Co-Authored-By: Claude <noreply@anthropic.com>
The change that landed the per-connection drain added 38 lines of comment
against 31 of code. Cut to 31 against 32:
- one paragraph, not two, for reading `headersSent` before ending. Both
said `res.end()` writes a header.
- the production cut count moves out of the comment. It is in the
commit that introduced the fix, which is where a measurement from one
run belongs; the invariant above it stands without it.
- the hoist rationale for `routeOf` goes from three lines to one. The
function is five lines and pure.
- `elapsed` reuses the clock the loop already read into `now` rather
than taking a second one that can disagree with the reads the endings
were just decided against.
- the two-connection case loses the restatement of what each assertion
catches. The assertion messages say it at the point of failure, which
is where it is read.
Investigated and NOT fixed, because it could not be reproduced: after
ending a connection the loop forgets it, so a response still in the live
set on a later tick would be re-stamped from its arrival time, found
older than the window, and ended again. `res.end()` only queues the FIN,
so a client that has stopped reading looked like a way to hold one there.
A fixture that floods 64 KB chunks at a socket with no reader, then
signals, ends the connection exactly once — with the marking guard and
without it. No implementation reachable from here violates the invariant,
so the guard and its assertions were removed rather than shipped
unkilled.
16/16.
Co-Authored-By: Claude <noreply@anthropic.com>
The arrival stamp was applied to every connection, and the comment beside
it already named the scope the code was missing: a connection with no byte
to date from. One that HAS been delivering was back-dated to its arrival
too, so on the first tick that saw no byte cross, `now - at` was the
REQUEST'S AGE rather than its silence — and any reply older than the stall
window was cut about two seconds into every handover.
That is a regression, and worse than the bug it sat next to. Under the
aggregate clock the same connection survived, because a moving one kept
the shared clock fresh. It also selects for exactly what the drain
protects: a reply is exposed only once it is older than the window, so the
longer a turn has run the more certainly it qualifies.
streaming reply, 3s chunk gap, window 4s, request age 16s
before cut 2 ticks into the drain, "no byte written for 11s"
(11s was its age; it had written 1s earlier)
after still delivering, drain still open
`_bornBytes` and not `n === 0`: `bytesWritten` belongs to the socket, so
the second request on a keep-alive connection starts nonzero and would be
read as already-written, handing it a fresh window.
Also, an ended connection is now MARKED rather than deleted from the
record. `res.end()` only queues the FIN, so a response whose client has
stopped reading never leaves the live set, and a deleted entry is
re-stamped as new and ended again every window. Measured with a 50 MB
burst into a paused client: 3 ends for one connection, 1 after. An earlier
attempt used 64 KB chunks against a live upstream and could not reproduce
it — the writes kept `bytesWritten` advancing, so the stall never fired —
and the guard was removed on that evidence. The fixture was too weak, not
the defect absent.
Two log lines corrected:
- reaching the backstop is NOT proof of a defect, and the line said it
was. A connection still moving when the budget expires, a CONNECT
tunnel or upgrade (never in the live set, because only the request
handler fills it — the common shape in forward mode), and the Node 18
keep-alive case all reach it with the predicate working. Measured: it
printed "neither moved nor stalled, which is a bug here" about a
connection that had moved 80 times in the preceding 8 seconds. It now
reports what is owed.
- a drain that cut replies no longer calls itself "clean". The phrase is
matched unanchored by its reader, so a suffix naming the cut does not
save it.
18/18.
Co-Authored-By: Claude <noreply@anthropic.com>
Second complexity pass, on the commit before it: 28 comment lines against
17 of code, now 19 against 15.
- the arrival stamp's rationale goes from ten lines to five. The two
invariants are which connections may be dated from arrival and why the
byte count rather than a zero test; the rest restated them.
- "this line used to say it was" is narrative about the change, not a
property of the code. The three ways a working predicate reaches the
backstop are the part a reader needs.
- the mark-never-delete note keeps the mechanism and drops the retelling.
- the clean-drain line stops duplicating its whole string across two
branches of a ternary. One template, one place for the wording to
change, which is the defect the duplication was one edit away from.
18/18.
Co-Authored-By: Claude <noreply@anthropic.com>
`headersSent` goes true at writeHead with `bytesWritten` still 0, so a
response blocked upstream after its headers were buffered was ended rather
than reset — and `res.end()` on one completes a well-formed empty 200. A
client cannot tell that from a real empty success and will not retry. It
is the same non-retryable answer the bulk close was fixed for, and the
comment forty lines above the defect already named the exact
discriminator and called `headersSent` an upper bound.
Not a regression: the same empty 200 comes out of the bulk close on the
base. But this range makes it far more reachable. The base reaches it only
when the WHOLE port is byte-silent for the window, which the production
incident shows never happens; here it is reached per connection, one
window after the request arrived, on every handover.
upstream flushes headers and sends no body
headersSent client receives HTTP/1.1 200 OK ...; logged mid-response
bytes client receives nothing; logged before headers
The byte count is in scope at that line, so the label is now exact rather
than an upper bound.
The fixture needed `flushHeaders()` to exercise it at all. Without it
`writeHead` buffers, upstream headers never reach the proxy, its own
`headersSent` stays false, and BOTH arms take the destroy path — the case
was green against the defect until that line was added. Its absence is
recorded beside it, because the next person to write a headers-only
upstream will hit the same silent pass.
Also: the backstop's `owed` no longer counts connections the stall test
already ended. `_live` holds them until their FIN flushes, so one
connection was reported as both "1 ended on the stall test" and "1
response(s) still owed" in the same line, which reads as two. That line
exists to be trusted about a number.
Two stale texts corrected: the double-end fixture's comment described
re-stamping "from arrival", which stopped being true once the arrival
stamp was scoped — it re-stamps as new and re-ends once per window, not
every tick. And a failure message still blamed the forced close for a
phrase that now comes from the per-connection line, which would point a
future reader at the wrong code.
19/19.
Co-Authored-By: Claude <noreply@anthropic.com>
Third complexity pass, on the commit before it: 10 comment lines against
4 of code, now 5 against 4.
- the byte-split rationale keeps why `headersSent` is the wrong test and
what `res.end()` does there. That the bulk close has only `headersSent`,
and that the label stops being an upper bound, are consequences a
reader can see from the line itself; both are in the previous commit's
message.
- the `owed` note says what it must not count and why in one sentence
instead of restating the log line it protects.
19/19.
Co-Authored-By: Claude <noreply@anthropic.com>
The forced close re-counted what the stall test had already ended. An `end()`ed response stays in the live set ONLY because its FIN cannot flush, and while it is held `server.close()` cannot resolve — so the backstop always fires and every such connection was reported twice, once by the stall test and again as `cut N in-flight`: cut 1 in-flight request(s) ... — 1 ended on the stall test, 0 still owed one connection, two numbers, on a line that reads as a total. The co-occurrence is structural, not occasional. `seen` moves above `forceClose` so the bulk count can consult it; it is empty on the supervised arm, where the stall loop never runs. The previous commit's `owed` fix shipped with NO TEST — reverting it to the exact pre-fix line passed the whole suite. Every other invariant on this branch is pinned by a case that fails without it, so that one was one refactor away from coming back silently. The new case pins both halves of the line, and both reverts now fail it. That line is not decoration: a sibling monitor greps `drained clean in|cut \d+ in-flight`, and this file's own deploy evidence (`cut 4 -> 14 -> 17 -> 14 -> 16`) is quoted from it. A count that inflates corrupts the instrument the project reasons with. Held back deliberately: a guard for a response that has already ended itself, where `res.end()` is a no-op and the drain would log a cut it did not make. The state could not be reached from a test in four fixture shapes, and there is a mechanism for why — `upstreamRes.pipe(clientRes)` honours backpressure, so a client that stops reading leaves `end()` uncalled at any body size, and the one path that ends the response anyway puts it in the state `closeIdleConnections()` severs at drain start. Not shipping an unpinned guard; the finding is recorded against that separate defect. 20/20. Co-Authored-By: Claude <noreply@anthropic.com>
`res.end()` on a finished response returns silently, so the stall test cut nothing and reported a cut anyway — inventing a number in a line an external monitor parses, and flipping "drained clean" off for a drain that lost none. Reaching it needs the NON-STREAMING branch. The streaming path pipes, and `pipe()` honours backpressure, so a client that stops reading leaves `clientRes.end()` uncalled at any body size. The buffered branch collects the whole upstream reply and answers with one `end(rawResponse)` that ignores backpressure, leaving the response `writableEnded` with megabytes queued. Measured on that path: writableEnded=true writableFinished=false headersSent=true The reply must also complete AFTER the signal. `closeIdleConnections()` runs once at drain start and Node counts a finished-but-unflushed response as idle, so one that completed earlier is severed there and never reaches the stall loop — which is a separate defect, filed separately. Four earlier fixtures failed to reach the branch, all of them on the streaming path, and the near-miss is worth naming: the mechanism found for why they could not (`pipe()` backpressure) was correct for the path tested and wrong as a general claim. The case records which branch it needs and why, so the next reader does not rediscover it. Both premises are implementation-independent: the drain must have reached its backstop still holding the connection, and the client must have received part of a body it could not finish. The body is 4x what this box's socket buffers absorbed, so a machine with larger buffers fails the premise loudly rather than passing green. 21/21. Co-Authored-By: Claude <noreply@anthropic.com>
Five comment lines on a one-line guard, now three. What a reader needs is that `res.end()` on a finished response is a no-op, and WHICH branch can put a response in that state — the second cost a full review round to find, so it stays. That the false count flips the clean-drain line off is visible from `stallEnded++` three lines down. Comment-only: with `//` lines and blank lines stripped, the file hashes identically before and after. Co-Authored-By: Claude <noreply@anthropic.com>
Two wording corrections found by review; no behaviour change. The FIN-cannot-flush case claimed to pin marking-versus-deleting. It cannot. `rec.done` and the `writableEnded` check are mutually redundant on the ended arm, because `res.end()` makes `writableEnded` true and the later guard catches the re-visit; on the destroyed arm `close` drops the response before the next tick. Measured: seen.delete() restored, guard kept case survives guard removed, marking kept case survives both removed 3 ends over six windows So it pins "at least one of the two survives". The redundancy is deliberate and cheap, and the comment now says which claim the case supports rather than the stronger one it does not. The stall case's name described the pre-branch model, where the stall ended the DRAIN. It ends a connection now; the drain ended because that was the only owed one and `close()` then resolved. All four of its assertions were already accurate. Co-Authored-By: Claude <noreply@anthropic.com>
The branch introduced `CACHE_FIX_DRAIN_STALL_MS` and documented neither it nor the change it belongs to. An undocumented knob is half a knob: the default is derived from measurements on one fleet's links, and a reader who cannot find the override inherits that latency as their ceiling. The README row names the paired `CACHE_FIX_DRAIN_MS` inside it rather than adding a second row. Either knob alone is half the picture — one bounds a connection, the other bounds the drain — and the pairing is what makes them findable together. `CACHE_FIX_DRAIN_MS` predates this branch and its own absence from the table is not this change's to fix. The row also carries the two observations the default sits between and the warning not to tighten toward an observed maximum, because a number presented as derived when it was chosen is worse than an honest one. The CHANGELOG entry describes the drain rewrite, which the existing entry did not mention at all — it covered the config-freeze fix this branch started as. Documentation only; with comments and blanks stripped, `proxy/server.mjs` and the test file hash identically before and after. 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>
(cherry picked from commit 392369b)
…g it Node counts a response whose end() has been CALLED as idle, so server.close() destroys one that is complete but still flushing -- at drain start, before any predicate can judge it. The drain then reports the handover clean, so the phrase that means "cut nothing" is printed for a reply cut to a fraction of its length. Measured through the proxy: a client received 4,217,623 bytes of a declared 16,777,216 while stderr said `drained clean in 0.0s of 5s budget`. Discriminated on plain Node, three arms, no proxy involved: no close() 100.0% delivered server.close() alone 6.2% server.close() + closeIdle 6.2% so the agent is close(), not closeIdleConnections(). Both paths that reach Node close() are guarded; each one removed alone lets the reply be severed again, so neither is redundant. The predicate lives beside _live so there is one spelling of the question. No timer of its own -- the drain budget and its forced-close backstop already bound the wait. Co-Authored-By: Claude <noreply@anthropic.com>
… quiet `N still owed` cannot tell a stalled reply from a live one, so every forced close has needed re-derivation rather than reading. Measured once under real traffic, on the first drain after the per-connection rewrite shipped: shutdown: forcing close, cut 4 in-flight request(s) after 1800660ms on the BACKSTOP budget -- 0 ended on the stall test, 4 response(s) still owed (4 mid-response, 0 before headers) routes: /v1/code=4 `0 ended on the stall test` across 1800s against a 90s window means bytes kept moving every tick, so the predicate was right to hold them and the ceiling cut four live replies -- but that is an inference from a zero, not a reading. The predicate already dates each connection from its last byte, so this reports what is there rather than counting anything new. A range, not one entry per connection: the minimum answers "was any of them live", the maximum "was any of them stalled", and two numbers cannot blow up a line that once cut 17. Both arms, not just the backstop -- a supervised stop cuts at a hard 5s with no predicate at all and its line was equally unreadable. Co-Authored-By: Claude <noreply@anthropic.com>
test/proxy-handover-env.test.mjs makes a temp dir per case plus one per fixture file and removed none of them. Measured on one run of that file alone: before 269, after 277 -- eight per run, 285 accumulated over roughly 35 runs, the oldest from the previous day. Removed on process exit rather than in a per-case finally: a throwing assertion skips a finally, so a RED run leaked more than a GREEN one, and the cleanup has to outlive the case that failed. NOT a root `after()` hook, which was the first attempt. node:test runs files concurrently and a root hook registered from this file reddened three timing cases in others -- holder handover, hop fallback, self-heal attach. Attributed with a control rather than assumed: the same worktree under the same load with only this file reverted was 1974/1973/0, so it was the change and not the load. Co-Authored-By: Claude <noreply@anthropic.com>
…ve replies Measured twice on one machine, identical both times: forcing close, cut 4 in-flight request(s) after 1800660ms on the BACKSTOP budget -- 0 ended on the stall test, 4 response(s) still owed (4 mid-response) forcing close, cut 4 in-flight request(s) after 1800015ms ... same shape `0 ended on the stall test` means the per-connection predicate ended nothing: it judged all four alive and was right about all four. The wall clock then overruled every one of them. A last resort that overrules the only informed opinion in the system is not a last resort, it is a second policy on a timer. So the budget becomes a re-evaluation point. At expiry, end what the predicate would end anyway and keep waiting while any connection is still delivering, saying so periodically -- the tick is 1s and a live stream can hold the drain open, so that line is rate-limited rather than per-tick. Waiting is affordable and cutting is not: the code already states that a lingering predecessor holds no listener and costs RAM, while what sits on the other side of this branch is a reply someone is reading. The honest gap is that a client that never finishes now lingers; a ceiling for that belongs in units of RAM or count, not seconds, and a wrong-unit ceiling is what was costing replies. Both halves mutation-checked: removing the guard cuts the live reply again, removing the notice leaves a drain that stays past its budget silently. Co-Authored-By: Claude <noreply@anthropic.com>
A holder deliberately leaves a standby relay behind, and killing the holder is what ARMS it -- that is the standby s whole purpose. It keeps the address alive so a session whose HTTPS_PROXY was fixed at exec is not stranded. Surviving its holder is correct behaviour, not a leak in the relay. It is a leak here. Production wants an armed standby holding a real port; a test wants its ephemeral port released, and nothing else ends one. Measured: two runs of this file, one orphaned standby each, both confirmed by the standby s own declaration of its parent rather than by name or age. Selected on ppid != CACHE_FIX_STANDBY_PARENT, and only for parents THIS file spawned. That scoping is what keeps the sweep off the production relay and off other sessions on a shared box -- a name match would have killed the live one. /proc, so linux only. CI runs linux and exercises the guard; on a mac the orphan survives until the OS reclaims it, which is the smaller wrong. Whole suite after: 0 orphans created, 1975 tests, 0 fail. Mutation: unregister the sweep and one orphan returns per run. Co-Authored-By: Claude <noreply@anthropic.com>
An appending writer grows without bound unless something bounds it; an overwriting one is bounded by one file s content. Only the first class can fill a disk, so only that class is inventoried. This does NOT check for a cap. Whether a writer is bounded is semantic -- a retention loop, an env cap, a caller that truncates -- and no pattern match can decide it. Grepping for MAX_BYTES and friends would answer confidently about the wrong thing, which is how a cap reading 4 MiB in its own constant rotated at 64 KiB in a neighbouring component: the constant was right and importing it verified nothing. So it enforces the one thing a test can: the set of appending writers is exactly this list. A new one cannot arrive silently, and adding a file to the list is a deliberate act that puts "what bounds this?" in front of a reviewer at the moment it can be answered. Measured when the list was taken: 16 appenders, 9 with no cap of any kind. That is a fact about the tree, not an approval of those nine. Three mutations, all fatal: a new appending file, a stale entry that no longer appends, and a scan pattern that matches nothing -- the last because an empty scan passes a subset check silently and would guard nothing while reading green. Co-Authored-By: Claude <noreply@anthropic.com>
`drained clean in Xs of Ys budget` means everything owed FINISHED inside the budget. It does not mean nothing was owed -- and nothing said how much was, so a stop with three replies in flight that all completed and a stop with an idle port printed the identical line. How often this path has anything at risk was not derivable from these logs at all. That number is what the supervised arm s cost is argued from, and the sample behind it is four terminations. Both terminal lines now carry it, counted at drain start rather than at the line -- by then the set has drained, which is the question the line already answers. SCOPE, stated so nobody builds on more than it supports: this says how often a stop has anything owed. It does NOT say how often waiting would have saved a live reply. headersSent means headers were sent, not alive, so an owed count includes connections that already died. Placed at the END of the forced-close line. A first attempt put it beside the budget and broke a case pinning `after <N>s (<n> mid-response,` as one adjacency; that case looks over-specified and a reviewer would trim it, and its brittleness is the contract check. The reason is now a comment on forcedCloseLine, phrased as the rule rather than as a claim about any particular reader. Mutation: remove the report and the case dies. Both emitted lines read back by deliberately breaking the assertion rather than trusting a green. Co-Authored-By: Claude <noreply@anthropic.com>
…of cutting
The 5s ceiling was never chosen by a deadline. It was chosen by a signal
conversion: the holder rewrites every stop to SIGHUP, SIGHUP sets `releasing`,
`releasing` forces askForSuccessor false, and the budget read "no successor was
spawned" as "no time". Three different questions collapsed into one constant.
Measured cost of that on one host: 15 in-flight replies cut across four stops
(4, 3, 1, 7), every one at `after 5s` with no stall predicate installed at all.
Two independently-written filters over the same log disagreed on the total, and
that disagreement is the only reason this arm was looked at.
A ceiling is payable only when somebody's wait is serial. It was, and the fix is
in a PAIR, because neither half works alone:
- the holder settles on the RELEASE ANNOUNCEMENT when stopping, not on the
child's exit. From that line the proxy owns no listening socket and nothing
but the replies it still owes.
- the drain budget keys on "nothing waits on us" (`unwaited`) instead of on
"we spawned a successor".
Landing the proxy half alone puts back the downtime the ceiling was bought with
(120s against a 90s TimeoutStopSec took a restart from 5.0s to 53.9s). Landing
the holder half alone orphans a drainer that still cuts at 5s.
STANDALONE KEEPS THE CEILING, and that is not an oversight. With no holder
above it the proxy IS the process someone waits on, so the bet is real there. A
control case asserts it still cuts at 5s; without that control this change reads
as deleting the ceiling everywhere.
WHAT NOT CUTTING COSTS. The process stays until its replies finish, so a stop
can leave a resident. Bounded by concurrent activity, not by deploy count:
measured across three machines, two carried a resident for over two hours each
while the third held zero connections and produced no drainer at all. A stop-arm
resident is worse than a handover-arm one because nothing is coming to make use
of it. No count bound here: the only lever a naive one has is cutting the
oldest, which is cutting the reply this change exists to protect.
Exit 0, not 75, is now load-bearing rather than incidental. One machine carries a
dormant launchd agent for a different install of this proxy whose KeepAlive
fires only on a non-zero exit; a drainer that starts exiting 75 there returns as
a second listener from a stale tree. Asserted, held and standalone.
The premise this rests on, stated precisely because the loose version was wrong:
the holder is unsupervised AS DEPLOYED (parented to init on all three machines),
not "unsupervised everywhere". Under a supervisor that reaps its control group
the drainer dies with it and this trade does not hold.
Both halves are mutation-proven, in different files because they fail in
different places. Drop `heldByLiveHolder` and the proxy case reproduces the
production line verbatim ("cut 1 in-flight request(s) after 5s") while the
standalone control stays green. Drop the holder's early settle and the stop
BLOCKS: 2,254 ms against a 20,001 ms timeout end to end, and the lifted dispatch
case catches the same thing in 5 ms with "got reclaim+spawn". Every proxy-side
case stays green under that second mutation — the reply is delivered perfectly
and nothing can stop the service — which is why one file could not cover it.
The replacement for that case measures GROWTH past the ceiling, not a total.
`chunks > before` cannot see a cut at all — a 5s ceiling delivers five seconds of
bytes first, so the total rises either way, and a 4s sample lands inside the
window it is trying to detect. Measured: the first draft stayed GREEN with the
held arm reverted. Two late samples with growth required between them catch both
halves from one case — the proxy half as "stopped at 94 chunks and never moved
again", the holder half as "still on the port 25106ms after SIGTERM".
Co-Authored-By: Claude <noreply@anthropic.com>
codeslake
force-pushed
the
fix/handover-rereads-config
branch
from
August 25, 2026 20:37
53177de to
9f0f0bf
Compare
`a stop returns when the proxy releases` waited for THREE chunks before signalling. Three was copied from a sibling case that measures growth and genuinely needs several; this one only needs the reply ESTABLISHED and OWED, and an SSE stream that has delivered a byte stays owed until something cuts it. So the three bought nothing and smuggled in a rate assumption. It cost a deterministic failure. `node --test` fans out at CPU count, so a 48-core host runs an order of magnitude more test files at once than a 4-core runner: 2 chunks arrived where 75 were due, and the premise guard refused rather than passing vacuously. Measured 2/2 failing before, 2/2 green after, same host, same whole-suite load. CI was green on all three node versions throughout — that host is a HARSHER environment than CI, not a preview of it, which is why the case belongs there rather than opted out of it. The threshold is the fix; dropping the upstream interval is margin. The 15s deadline is deliberately unchanged: widening a budget papers over the assumption instead of removing it, and fails again on the next box. Co-Authored-By: Claude <noreply@anthropic.com>
A half-written file parses cleanly, which is what makes it worse than a malformed one. writeFileSync truncates and then writes, so a handover that lands mid-write reads a prefix, and a cut line like CACHE_FIX_PROXY_UPSTREAM=http://ho is a well-formed assignment carrying a broken value. "Every failure is off" did not cover it: this failure looks like a success. Nothing later corrects it either, because the successor carries the value until something hands the port on again. Measured before the fix: a file ending mid-value applied 'http://ho' over an inherited upstream. The unterminated tail is now dropped. A file with no final newline therefore loses its last line, which is the fail-safe direction the rest of this module already takes: the inherited value stands. README says so. Co-Authored-By: Claude <noreply@anthropic.com>
Two defects, both introduced by this branch and both measured against the
merge base.
1. THE DRAIN KEPT THE LISTENING SOCKET.
Deferring server.close() behind _unflushed() took the unbind with it. The
release is announced BEFORE the drain and a holder settles on that line, so
from there the address has to be free for whoever takes it next. It was not:
measured on a stop under a live holder with one finished-but-unflushed reply,
new connections were still ACCEPTED for the whole probe, against ECONNREFUSED
within a second at the merge base. The bound is the backstop budget, 30 minutes
by default, during which the dying proxy goes on accepting requests it will cut
and a successor binding the port would get EADDRINUSE. The comment above the
call still says "server.close() unbinds at once ... so a supervisor that waits
for our EXIT sees the port unowned for the whole drain", which had stopped
being true.
The agent is not close(), it is the IDLE SWEEP that http.Server.close() runs
before it unbinds (httpServerPreClose). The three-arm discrimination behind the
deferral could not separate them, because every arm that closed also swept. The
missing arm, on plain node:
no close at all 100.0% delivered STILL LISTENING
http close() 24.9% ECONNREFUSED
http close() + explicit sweep 24.9% ECONNREFUSED
net close() 100.0% ECONNREFUSED
net close() + explicit sweep 24.9% ECONNREFUSED
So the unbind is free and only the sweep has to wait. Both close paths now go
through server._unbind(), which is net's close; the sweep keeps the _unflushed
guard it had, in the one place that severs. Neither guard was removed.
2. THE FORCED CLOSE REPORTED A LIVE REPLY AS QUIET.
The stall loop runs on the handover arm only, so on a supervised stop `seen` is
empty and the quiet figure was dated from ARRIVAL: measured, `quiet 17.5s` for
a reply delivering a chunk every 200ms whose last byte had left 152ms earlier.
That is the field an operator reads to decide whether a cut mattered, and it
said the cut took something already dead. It now falls back to the same rule
the stall loop's first sight uses: arrival only while nothing has left the
socket since. Resolution on that arm is the budget, so a reply that stalls
mid-drain reads 0, erring toward calling the cut costly rather than free.
Tests: two cases, both red first. The port case asserts BOTH halves, since
unbinding by severing satisfies the first alone. The seam test for a genuine
close failure now stubs _unbind, or it would assert against a function nothing
calls.
Full suite: 1985 tests, 1984 pass, 0 fail, 1 skipped.
Co-Authored-By: Claude <noreply@anthropic.com>
Two entries the branch had no line for. The drain one because the finished-but-unflushed delivery shipped without a changelog bullet at all, and its mechanism is the part a future reader will otherwise re-derive: the idle sweep severs, http close() runs it before unbinding, net close() does not. The handover-env one extends the existing "every failure is off" sentence, which did not cover the failure that looks like a success. Co-Authored-By: Claude <noreply@anthropic.com>
Two shrinks from a complexity review of this branch, no behaviour change. `owedAtStart` spread a Set into an array to read `.length`. `Set.size` is the same number without the allocation. The neighbouring "do not simplify the spread away" note guards an ITERATION that ends responses while the set mutates; this one only counts, so it does not apply here. `quietTally` called Math.min and Math.max three times across a nested ternary. Hoisted to one call each and flattened. Suite after: 62 of 62 in the two files that own these lines, 0 fail. Co-Authored-By: Claude <noreply@anthropic.com>
The clean-drain case matched `owed [0-9]+ at the start`, which `owed 0` satisfies -- and `owed 0` is the exact reading the field exists to distinguish "nothing was in flight" from. The case holds a reply open across the stop, so the count is known to be at least one; the assertion now says so. Mutation, against the strengthened case: replace the count with a literal 0 and it dies with Got: shutdown: drained clean in 4.4s of 5s budget, owed 0 at the start expected: /owed [1-9][0-9]* at the start/ The same mutation passes the old pattern, so the field's VALUE had no test. Co-Authored-By: Claude <noreply@anthropic.com>
Three findings from an independent review of this branch, plus the fixture
extraction a complexity review asked for.
THE OWED COUNT. The stall loop marked a self-ended reply `done` so it would not
report a cut for a `res.end()` that is a no-op. But `done` is read twice with
different meanings: the forced close reads it as "already counted" and the
backstop reads it as "no longer owed". The second is wrong. A buffered reply
that called end() with megabytes queued IS owed -- the client is not reading and
closeAllConnections() destroys it -- so the drain printed `0 response(s) still
owed` and `cut no responses` about a reply it then truncated. That is the
severed-and-called-clean shape this drain exists to remove, one branch over.
The marker is dropped; re-entering the test each tick costs one comparison.
THE LOG CONTRACT. The block above forcedCloseLine records that a field between
the budget and `(<n> mid-response` broke an external parser, and that new fields
therefore go at the end. `why` was then appended to the budget, taking exactly
that position -- and only on the backstop arm, which no pinned case rendered.
Moved to the tail. The same arm passed the ELAPSED time, which is not a multiple
of 1000, so it alone rendered `after 1800660ms` where every other renders
`after 1800s`; it is rounded to the second at the call site.
A CREDENTIAL IN THE ROUTE TALLY. The tally cut the query string and not the
authority. A foreign absolute-form target reaches handlePassthrough
un-normalised, so `http://user:pass@host/v1/x` rendered as
`/http:/user:pass@host` -- userinfo, in a log, from the rule written to keep
identifiers out of one. It now takes the pathname when the target parses as
absolute-form. Lifted to a module-level `drainRoute` so the case can call it.
RED for each, before the fix:
cut 4 in-flight request(s) after 1800s, on the BACKSTOP budget - 0 ended
(4 mid-response, 0 before headers)
expected: /cut (\d+) in-flight request\(s\) after (\d+)s \((\d+) mid-response/
drainRoute("http://user:hunter2@example.test/v1/messages?x=1")
actual: '/http:/user:hunter2@example.test'
expected: '/v1/messages'
The owed-count case is the existing "does not report a cut for a reply that had
already ended itself", which reached the backstop and asserted only that the
stall test claimed nothing. It now also requires a nonzero owed count, a
reported cut, and a budget in seconds.
Also: two fixture helpers, `hungUpstream()` and `sseUpstream()`, replacing four
and three hand-rolled copies. One of them found a real defect in this commit --
the lifted teardown ignored the callback the old call site passed, and the case
hung rather than failing.
Co-Authored-By: Claude <noreply@anthropic.com>
`CACHE_FIX_HANDOVER_ENV` was honoured from inside the file it names, so one write to the handover file pointed every later handover at a path outside the config dir. Because absence means inherit and there is no syntax that unsets a key, tightening or reverting the original file could not take it back. This grants nothing past the boundary the README already states -- write access to that file is control of the proxy -- but it makes the boundary un-revertable, which is a different property and one nothing recorded. Pinned by the holder alongside the port, the bind and the orphan guard. RED: the assertion added to the SIGUSR2 spawn guard in suite-collection fails against the unpinned successor env. Co-Authored-By: Claude <noreply@anthropic.com>
The stall entry said "Unchanged: the supervised-stop path keeps its 5 s
ceiling". Measured against the merge base, a stop under a live holder went from
5000 to 1800000 -- and this tree uses "supervised" for exactly that case
("a live holder supervises us"). The 5 s ceiling now belongs to the STANDALONE
arm alone, and the sentence says so.
Nothing documented the holder change at all: a stop now returns on the proxy's
release announcement instead of its exit, so it completes in under a second and
leaves the old proxy resident, holding no listener and no lock, until its
replies finish or the backstop expires. That is the most operationally visible
change in the branch and it was recorded only in a test comment.
CACHE_FIX_DRAIN_STALL_MS landed in the OAuth-refresher table, under a heading
that gates on CACHE_FIX_OAUTH_REFRESH; moved to the proxy settings table.
CACHE_FIX_DRAIN_MS had no row at all while the new one referenced it.
Co-Authored-By: Claude <noreply@anthropic.com>
The unreadable-path case chmod 000 and expected the read to fail. Mode bits do not stop root, and this package runs in root containers -- there the chmod succeeds, the read succeeds, and the case fails on an assertion unrelated to what it tests. A directory throws for every uid and exercises the same branch: any failure leaves the inherited value standing. Co-Authored-By: Claude <noreply@anthropic.com>
… all The block above `drainRoute` states the rule it exists to enforce: a foreign absolute-form target reaches the passthrough un-normalised and the raw request-target would put `user:pass@host` in a log line that outlives the process. `parseAbsoluteForm` carries http and https, and every OTHER authority-first shape fell straight through to the raw target: //user:pass@host/v1/messages -> /user:pass@host/v1 ftp://user:pass@host/v1/messages -> /ftp:/user:pass@host host:443 -> /host:443 The case that guards this pinned only the `http://` scheme, so all three passed it. Origin-form is a SINGLE leading slash. That is the whole discriminator: it separates `/v1/messages` from `//host/v1/messages`, and everything the parser did not recognise now renders `?` rather than an authority. RED: an authority-first target rendered into the log: /user:hunter2@example.test/v1 GREEN: 32/32 in the file Co-Authored-By: Claude <noreply@anthropic.com>
…tick
Both drain knobs parsed with `Number(x) || fallback`, which is wrong in two
opposite directions:
CACHE_FIX_DRAIN_STALL_MS=0 -> 90000 an explicit "cut now", discarded
CACHE_FIX_DRAIN_STALL_MS=-1 -> -1 passed straight through
The second is the one that matters. `now - rec.at < stallMs` is false on the
first tick for a negative budget, so every owed connection is scored stalled
and ended at once. That is the guillotine this drain exists to replace, and it
is one typo in an env file away. The comment directly above the knob says
anything under ~60s is inside the observed range of a healthy stream.
`drainBudgetMs` takes only a finite non-negative number and falls back
otherwise, so garbage still gets the default and 0 now means 0.
The lifted-source case that evals the budget line gets the REAL helper passed
in rather than a copy, for the reason its own comment already gives about
`unwaited`: a re-implementation lets the two drift into agreeing about a
budget the server does not use.
RED: SyntaxError: does not provide an export named 'drainBudgetMs'
then: a negative budget cuts every owed connection on the first tick
GREEN: 33/33 in the file
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
Attempted Codex R1 today and it crashed mid-review (exit code 1 after 488s, empty stdout, 711KB stderr — no verdict, no findings posted, no labels). The pattern matches what we've seen when a single review exceeds Codex's per-run context/token budget: +2444/-85 with a contract-level semantic change is the hard case for one-shot review. Following up on the "diff size vs stated scope" concern I raised in the R1 focus items (the scope calls that were sent to Codex, now moot since it didn't complete): would you consider splitting this into two PRs?
Two smaller PRs would let Codex complete a review on each AND give the human reviewer (Chris) a way to trace the semantic change without wading through refactor noise. If you'd rather keep it as one PR, that's your call — we can revisit fallback options on our end (staged partial reviews focused on the holder contract only), but the split gets you clean approvals faster. No urgency from our side — Proxy Builder's current focus is elsewhere, and the smaller PRs in the queue (#347/#346/#355/#345/#352/#353) all have their R1 verdicts up for you already. — AI Team Lead |
…annot hide an authority
Both defects are in the commit before this one, found by reviewing it rather
than the code it replaced.
`Number()` READS WHITESPACE AS 0. Rejecting a negative left `" "`, `"\t"` and
`"\n"` -- each trivially written into an env file or a unit -- coercing to an
explicit budget of zero, which for the stall knob is the guillotine the drain
exists to remove. Measured end to end: `CACHE_FIX_DRAIN_STALL_MS=" "` severs a
healthy 3-second-gap stream that survives at any real value. `null` and `-0`
went the same way. Both knobs now take a plain non-negative decimal and fall
back otherwise; exotic spellings that used to be guessed at (`1e3`, `0x10`,
`Infinity`) fall back rather than being interpreted.
A SHAPE FILTER CANNOT ENFORCE A CONTENT RULE. "A single leading slash" admits
`/http://user:pass@host/v1`, `/\user:pass@host/x`, a percent-encoded `//`, a
`;`-prefixed authority, and `#` survives the `?` split entirely -- five ways to
put an authority or a fragment into a line that outlives the process, in the
function whose own header says that is what it prevents. It now judges the
label it is about to write: neither `@` nor `#` belongs in a route.
README already documented both knobs; what it did not say is which values they
accept, which is exactly what changed. Said now, with a CHANGELOG entry.
RED: whitespace " " was read as an explicit budget
an authority-first target rendered into the log: /http:/user:hunter2@example.test
GREEN: 33/33
mutation: dropping the decimal test kills the whitespace case; dropping the
[@#] filter kills the authority case; restored 33/33
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Author
|
Thanks for the crash detail — the exit code and the stderr size are more useful than a timeout would have been. I measured the diff before answering, because the split as proposed rests on there being ~2000 lines of refactor, and there is not: So there are no renames, moves, or structural cleanup to peel off. The other ~2000 lines are tests for the contract change, at roughly 4:1 — which A split is available, just along a different seam. The body describes one of the 39 commits (the handover-env re-read); the rest is a per-connection drain rewrite — the net-layer unbind, The cheaper thing, if it helps your side: the production diff is 483 lines across 3 files. If a one-shot review can take that with the tests available as reference rather than as input, that covers every semantic change in the PR. Two things I owe regardless, and am doing:
Happy to take the split if you'd rather have it — say the word and I'll cut it along the handover-env / drain line. 🤖 Generated with Claude Code |
codeslake
added a commit
to codeslake/claude-code-cache-fix
that referenced
this pull request
Aug 26, 2026
Resolved test/proxy-held-port.test.mjs, both hunks to the incumbent side. The first is a third wording of one type guard in classify(); all three are behaviourally identical, and the one kept names the mechanism (freePort releases a port before its caller binds it, so a neighbour's 200 arrives as a Number) rather than the single 502 that surfaced it. The second is not a wording difference: cnighswonger#356 predates the two cases pr-345 added here (the port-1 refusal probe and readyBody), so taking its side would delete them. Kept. Co-Authored-By: Claude <noreply@anthropic.com>
…n it starts
`_forwardActive--` and `removeSelfHeal()` ran synchronously inside
close()'s promise executor, so a stop retired this instance's routing
vote before a single owed byte had drained. The two readers of that
counter -- the absolute-form rewrite and the passthrough -- then send
everything that is not /health, POST /v1/messages or the bootstrap to
handleNotFound, and the process-wide uncaughtException swallower is gone
for the same window. Both are wrong for a process whose entire purpose in
that window is to finish what it owes.
Moved into the `_unbind` callback, which fires on 'close' after the drain,
and retired on both the resolve and reject paths: the server is going away
either way. The double-close guard is unchanged.
WHAT IS NOT CLAIMED. The end-to-end 404 was reported as reachable on a
surviving connection; I could not reproduce it in two arrangements, and
say so rather than repeat the severity:
- an idle keep-alive socket opened before the stop is swept at drain
start, so the next request is ECONNREFUSED and never reaches the
handler
- a request pipelined behind an in-flight stream is queued behind it and
is not serviced while that stream is open
So this removes a state that is wrong by construction -- routing disabled
while still serving -- rather than a defect observed from outside. A
behavioural case was written and then removed: its own control reported
"could not measure", and a case that can only say that is not a test.
Whole suite 1987 pass, 0 fail.
Co-Authored-By: Claude <noreply@anthropic.com>
codeslake
added a commit
to codeslake/claude-code-cache-fix
that referenced
this pull request
Sep 3, 2026
…ger#356 merge left behind conflict-shape.sh's additive resolution on the cnighswonger#356 merge kept both sides of two hunks where cnighswonger#356 (cut from upstream, before cnighswonger#345 or cnighswonger#355 existed) independently re-added content cnighswonger#345 had already added on this branch: a second `if (typeof body !== "string") return null;` guard (with its own, now-superseded comment) stacked dead beneath the one already in classify(), and a second, byte-identical copy of the "classify survives a probe that answers with a status code" test right after it. Both were textually different insertions at the same conflict hunk (so "additive" concatenated them) but semantically the same content twice. Neither duplicate changed behaviour -- the second guard clause is unreachable, and node:test does not refuse a duplicate case name -- so the suite passed either way; kept once, as the recorded resolution for this file already documents keeping ours' guard. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QFPGPPSYmx8NNGqbEsSpNc Co-Authored-By: Claude <noreply@anthropic.com>
codeslake
added a commit
to codeslake/claude-code-cache-fix
that referenced
this pull request
Sep 5, 2026
… test case (rebuild-resolutions.md: cnighswonger#356 duplicates) Co-Authored-By: Claude <noreply@anthropic.com>
codeslake
added a commit
to codeslake/claude-code-cache-fix
that referenced
this pull request
Sep 6, 2026
Conflict in test/proxy-held-port.test.mjs (classify()'s typeof guard): matches rebuild-resolutions.md's alternative 2, block 1, standalone (block 2 of that entry does not arise in this build order since cnighswonger#345 has not merged yet). Kept ours' guard (if (typeof body !== "string") return null;, carried into this build via cnighswonger#356's cherry-pick of cnighswonger#345's 35ac847) and dropped theirs' (cnighswonger#355, 5d2159f) removal of it. Measured: re-applying theirs' hunk alone and running 'classify survives a probe that answers with a status code' reproduces the exact TypeError the ledger records (body.startsWith is not a function). The resolved file passes that case and the other two classify cases (3/3); the parent commit passes them too, so nothing here is new. Co-Authored-By: Claude <noreply@anthropic.com>
codeslake
added a commit
to codeslake/claude-code-cache-fix
that referenced
this pull request
Sep 6, 2026
…to 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>
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
1 participant
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.
Two subjects
bin/handover-env.mjs(new),bin/claude-via-proxy.mjsproxy/server.mjs13 files, +2444 / −85, 39 commits. Four fifths of the diff is tests, at roughly 4:1 — there is no refactor in here to peel off. The seam a split would run along is the table above, not code-vs-cleanup.
1. A handover must not freeze the config it started with
SIGUSR2hands the listening socket to a successor spawned from the holder's own environment. That environment is a snapshot of whatever the first launcher was started with, so a handover carried stale gate values forward indefinitely — the proxy kept serving with settings the operator had already changed.bin/handover-env.mjsreads them back from a file at handover time. The holder still pins five keys over the file, because they describe this process and not the operator's intent: a successor that inherited them would claim a port it does not hold.A key absent from the file is inherited, not unset.
CACHE_FIX_STANDBY: undefinedreally drops the key — measured against the control ({"present":true,"value":"1"}inherited vs{"present":false}pinned, with the parent holding it set).The file is honoured only once its trailing newline is written, so a half-written line is never read as a setting.
2. A drain ends when the bytes stop, not when a clock does
A stop used to cut every in-flight reply at a fixed 5 s ceiling. Measured on one host: 15 replies severed across four stops (4, 3, 1, 7), every one mid-response, with no stall predicate installed at all.
The drain is now per connection. A connection that has written a byte within
CACHE_FIX_DRAIN_STALL_MS(default 90000) keeps the drain alive; one that has gone silent that long is ended alone.CACHE_FIX_DRAIN_MS(default 1800000) is a backstop and a re-evaluation point, not a deadline — a connection still moving bytes when it expires survives it and is reported every 60 s.The 5 s ceiling is kept for exactly one arm: a standalone stop, where the process draining IS the process a service manager is waiting on. The predicate is one line —
handedOff || handoverRelease || heldByLiveHolder— and each term is measured in the code's own comment.Other changes in this half: the listening socket is unbound at the net layer immediately, so the port is free while the drain runs; the forced-close line reports silence, not the age of the request; a reply that ended itself is counted as owed but not as a cut; and
SIGUSR2replacesSIGHUPfor the handover so a plain stop and a handover stop being told apart no longer depends on a log line.3. Three defects this branch fixed in itself
Number(x) || fallbacktook the fallback for an explicit0and passed a negative through, andnow - at < -1is false on the first tick — the guillotine this drain replaced, one typo away.Number(" ")is0. Measured end to end:CACHE_FIX_DRAIN_STALL_MS=" "severs a healthy 3-second-gap stream that survives at any real value. Both knobs now take a plain non-negative decimal.drainRouteguarded on the request-target's SHAPE, and a shape test cannot enforce a content rule:/http://user:pass@host/v1, a backslash, a percent-encoded//, a;-prefixed authority and a#fragment are each one leading slash. It judges the label it is about to write now — no@, no#.Verification
Whole suite 1954 → 2006 tests, 2005 pass, 0 fail, 1 skipped, and the rebuilt integration branch carrying this plus four sibling PRs is green on the same count.
drainBudgetMswhitespace " " was read as an explicit budget[@#]filter indrainRoutean authority-first target rendered into the log: /http:/user:...The lifted-source case that evaluates the budget line is handed the real helper rather than a copy, for the reason its own comment already gives about
unwaited: a re-implementation lets the two drift into agreeing about a budget the server does not use.Known, and not fixed here
A resident drainer holds the launcher's inherited stderr for the whole drain. The holder settles on the release line and exits in milliseconds; the drainer keeps fd 2, so anything capturing
run-service's output — a CI step,$(… 2>&1),ssh host '…'— waits out the drain. Measured, with a control:A terminal is unaffected; only a pipe blocks. Every available fix trades the drain's live reporting — which four of this PR's own cases read — against the hang, so it wants its own change rather than a patch tacked on here.