fix: a deploy that changes nothing, and a /health field that names the wrong hop

Twelve findings from the review of this PR, plus one raised by cswap's pin
against the fix for the last of them. Each has a test that dies when its fix is
reverted.

THE DEPLOY ONES, which is why this is not a tidy-up:

  otherHolderOn() compared process AGE only. Every incumbent outlives a process
  that just started, so on every deploy the NEW code judged itself surplus, exited
  0, and the OLD holder kept serving with nothing saying so. Now a holder is a
  duplicate only when it is running the same code.

  holderPidOn() answered "holder" on the mere presence of a run-service, which
  made runningOurCode() unreachable: the holder always keeps a descriptor to the
  listening socket, so the loop always returned before the fingerprint branch.

  bindFailed() read no error code, so a bind that can NEVER work — an address not
  on this host, a privileged port — took the "someone else has it" path, found no
  incumbent to ask, and exited 0. A deploy that started nothing reported success.
  Bind errors also carried libuv's errno through two hardcoded literals that named
  EADDRINUSE and called everything else EACCES; util.getSystemErrorName is right
  on both platforms and for every code.

  The holder matched its child's release announcement against a RAW CHUNK while
  the port line beside it was line-buffered. A chunk boundary inside "(handed
  off)" reads a handover as a plain release, so the holder reclaims the port from
  the successor already serving on it and spawns a second — the failure the
  (handed off) marker exists to prevent, re-entered through the marker itself.

  Ownership probes asked lsof about 127.0.0.1 while the bind honoured
  CACHE_FIX_PROXY_BIND. Under any other address the probe matched nothing.

  CACHE_FIX_PROXY_PORT=0 was rewritten to the legacy 9801 by `Number(env) || 9801`
  ("0" is a truthy string), while proxy/config.mjs read the same variable with
  envInt and yielded 0.

THE SHUTDOWN ONES:

  shutdown() had no re-entry guard although it is bound to SIGTERM, SIGINT and
  SIGHUP, and a control-group stop delivers more than one. Each entry can put
  another successor on fd 3. The window only exists while something is draining,
  which a live session always is.

  handle.close() always rejected on that path, because shutdown() closes the
  server one line earlier and the second close reports ERR_SERVER_NOT_RUNNING.
  Only the process.exit() inside .finally() beat the unhandled-rejection report.

THE HOP ONES:

  /health.https_proxy published a configured candidate. resolveHop() falls
  THROUGH the chain, so it named ":8118" while CONNECTs left via the second
  fallback or via nothing at all. It now publishes the hop a resolve actually
  used, and null when the chain was checked and found dead.

  cswap's pin raised that the fix left one field carrying two meanings — a URL is
  either measured or merely configured and a reader cannot tell. Split into
  https_proxy_measured, and direct_last: a sticky ISO instant of the last direct
  fall-through, under the name and for the reason the pin uses. A chain flaps back
  within ~1s, so a point-in-time field cannot report the outage that happened.

  hopAlive() and parseProxy() defaulted an https:// hop with no explicit port to
  80, so a live TLS hop read as dead and the chain fell through past it.

  CONNECT fell open to a direct dial with no way to refuse. Fail-open stays the
  default on both ends of the chain — a hop restarting is back in ~1s and refusing
  strands a session whose HTTPS_PROXY was baked at exec — but CACHE_FIX_REQUIRE_HOP=1
  now exists for a deployment where the hop is a policy boundary rather than a cache.

Ref cnighswonger#304

Co-Authored-By: Claude <noreply@anthropic.com>

Author: codeslake

bin/claude-via-proxy.mjs

@@ -9,6 +9,7 @@ import { X509Certificate, createHash, randomUUID } from "node:crypto";
 import http from "node:http";
 import net from "node:net";
 import { EventEmitter } from "node:events";
+import { getSystemErrorName } from "node:util";
 import { bundleUsable, carriesOurCA, salvageBundle } from "./ca-trust.mjs";
 
 const __dirname = dirname(fileURLToPath(import.meta.url));
@@ -152,8 +153,13 @@ class HolderSocket extends EventEmitter {
       try { h.close(); } catch { /* never bound */ }
       const e = new Error(`bind ${host}:${port} failed`);
       // The code net.Server would have emitted, so callers that branch on
-      // EADDRINUSE keep working.
-      e.code = err === -98 || err === -48 ? "EADDRINUSE" : "EACCES";
+      // EADDRINUSE keep working. From libuv's own table rather than the two
+      // literals it replaced (-98 linux / -48 darwin): those named the in-use
+      // case on both platforms and called EVERYTHING ELSE "EACCES", so a bind
+      // address not on this host (EADDRNOTAVAIL) and a privileged port arrived
+      // indistinguishable — and bindFailed() reads the code to tell "someone
+      // else is serving" from "this bind can never work".
+      e.code = getSystemErrorName(err);
       queueMicrotask(() => this.emit("error", e));
       return this;
     }
@@ -347,14 +353,21 @@ class HolderSocket extends EventEmitter {
   }
 }
 
+// The address the proxy binds, read the same way by the bind and by every
+// ownership probe. They disagreed: the probes asked lsof about 127.0.0.1 while
+// the bind honoured CACHE_FIX_PROXY_BIND, so with any other bind address lsof
+// matched nothing, holderPidOn() answered null, and takeOver() took "cannot
+// identify it: leave it alone" and exited 0 beside a live proxy of ours.
+const bindAddr = () => process.env.CACHE_FIX_PROXY_BIND || "127.0.0.1";
+
 // Returns "holder" when the owner is a holder of ours (nothing to do), a pid
 // when it is something else we may ask to stop, or null when we cannot tell —
 // and NULL MEANS LEAVE IT ALONE. Signalling a pid we did not identify is how a
 // deploy comes to kill an unrelated service that happened to be on the port.
 function holderPidOn(port) {
   let out = "";
   try {
-    out = execFileSync("lsof", ["-nP", "-t", `-iTCP@127.0.0.1:${port}`, "-sTCP:LISTEN"],
+    out = execFileSync("lsof", ["-nP", "-t", `-iTCP@${bindAddr()}:${port}`, "-sTCP:LISTEN"],
                        { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"] });
   } catch { return null; }
   // EVERY owner, not the first line. The holder keeps a bound descriptor AND a
@@ -366,12 +379,17 @@ function holderPidOn(port) {
   // function exists to prevent.
   const pids = out.trim().split("\n").map(Number).filter((n) => Number.isInteger(n) && n > 1);
   if (!pids.length) return null;
-  // A holder among them settles it: it is ours and it is already serving.
+  // A holder among them settles it ONLY IF IT RUNS OUR CODE. Returning "holder"
+  // on the mere presence of a run-service made runningOurCode() dead: the holder
+  // always keeps a descriptor to the listening socket, so it is always in this
+  // list, so this loop always returned before the fingerprint branch below.
+  // Measured: a deploy printed "this one is surplus", exited 0, and left the OLD
+  // code serving — every upgrade a no-op.
   for (const p of pids) {
     try {
       const c = execFileSync("ps", ["-p", String(p), "-o", "command="],
                              { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"] });
-      if (/\brun-service\b/.test(c)) return "holder";
+      if (/\brun-service\b/.test(c)) return runningOurCode(port) ? "holder" : p;
     } catch { /* gone between lsof and ps */ }
   }
   // NOT THE STANDBY, unless it is all there is. lsof returns ascending pid order
@@ -437,7 +455,7 @@ function holderPidOn(port) {
 function otherHolderOn(port) {
   let pids = [];
   try {
-    pids = execFileSync("lsof", ["-nP", "-t", `-iTCP@127.0.0.1:${port}`, "-sTCP:LISTEN"],
+    pids = execFileSync("lsof", ["-nP", "-t", `-iTCP@${bindAddr()}:${port}`, "-sTCP:LISTEN"],
                         { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"] })
       .trim().split("\n").map(Number).filter((n) => Number.isInteger(n) && n > 1 && n !== process.pid);
   } catch { return 0; }
@@ -452,7 +470,14 @@ function otherHolderOn(port) {
     // process has been up; a tie goes to the incumbent, which is the safe way
     // round — the surplus one leaving is free, two holders is not.
     const theirs = Number(line.split(/\s+/)[0]);
-    if (Number.isFinite(theirs) && theirs >= Math.floor(process.uptime())) return p;
+    if (!Number.isFinite(theirs) || theirs < Math.floor(process.uptime())) continue;
+    // AGE IS NOT ENOUGH. Every incumbent outlives a process that just started,
+    // so age alone fired on every deploy: the NEW code called itself surplus and
+    // left, and the old holder kept serving with nothing saying so. Only a
+    // holder running the SAME code is a duplicate; a different one is what the
+    // deploy exists to replace.
+    if (!runningOurCode(port)) continue;
+    return p;
   }
   return 0;
 }
@@ -526,8 +551,15 @@ function runningOurCode(port) {
 function holdPort(rest) {
   // The proxy's own default: holding a different port than the proxy would have
   // served leaves nothing at the documented address.
-  const port = Number(process.env.CACHE_FIX_PROXY_PORT) || 9801;
-  const bind = process.env.CACHE_FIX_PROXY_BIND || "127.0.0.1";
+  // `|| 9801` REWROTE PORT 0 to 9801. "0" is a truthy string so the run-service
+  // guard let it through, and this line then bound the LEGACY port — measured:
+  // `CACHE_FIX_PROXY_PORT=0 run-service` listening on 127.0.0.1:9801, the "took
+  // 9801 while the fleet dialled 9901" failure that guard exists to prevent.
+  // proxy/config.mjs reads the same variable with envInt and yields 0.
+  const rawPort = process.env.CACHE_FIX_PROXY_PORT;
+  const port = rawPort === undefined || rawPort === "" || Number.isNaN(Number(rawPort))
+    ? 9801 : Number(rawPort);
+  const bind = bindAddr();
 
   return new Promise((resolveP) => {
     let child = null, childPort = 0, stopping = false, restart = null, failures = 0, served = false;
@@ -869,18 +901,22 @@ function holdPort(rest) {
       // owns the accept path; we keep the descriptor so we can start the next
       // one, including after a crash. Deploys measured at 149,038 / 148,658 /
       // 146,225 requests, zero lost, zero refused, zero reset.
-      // Buffered until a newline: the port arrives on stdout, and a chunk
-      // boundary inside that line would otherwise lose it silently — every
-      // connection would then wait out the relay's deadline.
-      let line = "";
-      me.stdout.on("data", (chunk) => {
-        process.stdout.write(chunk);
+      // WHOLE LINES, for every announcement and not just the port. Buffering
+      // was added for the port line — a chunk boundary inside it loses the
+      // port silently and every connection then waits out the relay's deadline
+      // — and the release test was left reading the raw chunk. Same defect,
+      // worse outcome: a boundary between "…listening socket" and "(handed
+      // off)" reads a handover as a plain release, so this holder reclaims the
+      // port from the successor already serving on it and spawns a second one.
+      // That is the "one extra proxy per deploy, 3 alive after 4" the (handed
+      // off) test exists to prevent, re-entered through the test itself.
+      const onLine = (line) => {
         // The proxy announces the release before it drains, so the port comes
         // back to us at the START of its shutdown rather than at its exit.
         // Retire it here: it is no longer the proxy this holder supervises, so
         // the successor can boot while it finishes its in-flight work, and its
         // eventual exit must not be read as a death needing a respawn.
-        if (!retired && String(chunk).includes("releasing the listening socket")) {
+        if (!retired && line.includes("releasing the listening socket")) {
           retired = true;
           if (child === me) child = null;
           // "(handed off)" means the proxy already put its own successor on the
@@ -889,7 +925,7 @@ function holdPort(rest) {
           // add a second — measured without this: one extra proxy per deploy,
           // 3 alive after 4 deploys. Nothing to do but stop supervising the
           // one that left.
-          if (String(chunk).includes("(handed off)")) return;
+          if (line.includes("(handed off)")) return;
           reclaim();
           // AND ask for the successor. reclaim() only starts one if its bind
           // lands after this point; when the port is already ours — a proxy
@@ -900,10 +936,9 @@ function holdPort(rest) {
           spawnWhenReady();
         }
         if (childPort) return;
-        line += chunk;
-        const m = /listening on [\d.]+:(\d+)\n/.exec(line);
+        const m = /listening on [\d.]+:(\d+)$/.exec(line);
         if (m) {
-          childPort = Number(m[1]); served = true; failures = 0; line = "";
+          childPort = Number(m[1]); served = true; failures = 0;
           // The proxy has generated its CA by the time it says this, so publish
           // it now. A host wired by rc starts the proxy HERE and launches claude
           // from the shell, so the --remote-control path that used to be the
@@ -912,7 +947,21 @@ function holdPort(rest) {
           // terminates no TLS, so it has no CA to offer.
           if (process.env.CACHE_FIX_FORWARD_PROXY === "on") publishOurCA(ourCAPath());
         }
-        else if (line.length > 4096) line = line.slice(-256);
+      };
+      let buf = "";
+      me.stdout.on("data", (chunk) => {
+        process.stdout.write(chunk);
+        buf += chunk;
+        // Both announcements end in "\n" (server.mjs `say`), so a complete line
+        // is the whole fact and a partial one is never acted on.
+        for (let nl; (nl = buf.indexOf("\n")) !== -1;) {
+          const line = buf.slice(0, nl);
+          buf = buf.slice(nl + 1);
+          onLine(line);
+        }
+        // A child that writes megabytes without a newline must not grow this
+        // without bound; keep only enough tail to finish a split announcement.
+        if (buf.length > 4096) buf = buf.slice(-256);
       });
       me.on("error", (err) => {
         process.stderr.write(`Failed to start proxy server: ${err.message}\n`);
@@ -1022,8 +1071,18 @@ function holdPort(rest) {
     // A later server error must not start a second proxy beside the first.
     // Under run-service the collision is the ANSWER, not a fallback: something
     // is already serving, which is all the caller asked for.
-    const bindFailed = () => {
+    const bindFailed = (e) => {
       holder.off("error", bindFailed);
+      // ONLY EADDRINUSE MEANS "SOMEONE ELSE HAS IT". Every other bind failure —
+      // EADDRNOTAVAIL from a CACHE_FIX_PROXY_BIND that is not an address here,
+      // EACCES on a privileged port as non-root — went down the same path:
+      // takeOver() found no listener to identify, hit "cannot identify it:
+      // leave it alone" and exited 0. A deploy that started nothing reported
+      // success, and deploy.sh has no way to tell that from a real no-op.
+      if (e?.code && e.code !== "EADDRINUSE") {
+        process.stderr.write(`[cache-fix] cannot bind ${bind}:${port} — ${e.code}\n`);
+        return settle(1);
+      }
       if (alreadyRunning) return takeOver();
       resolveP(runProxy(rest));
     };

proxy/forward-proxy.mjs

@@ -241,7 +241,10 @@ export function ensureCA() {
 // Parse an http(s)://host:port proxy URL into { host, port }.
 function parseProxy(url) {
   if (!url) return null;
-  try { const u = new URL(url); return { host: u.hostname, port: Number(u.port) || 80 }; }
+  // Scheme-defaulted, like hopAlive(): `|| 80` sent the CONNECT for an
+  // `https://hop` carrying no explicit port to :80, so the tunnel died against
+  // a hop hopAlive() had just confirmed on :443.
+  try { const u = new URL(url); return { host: u.hostname, port: Number(u.port) || (u.protocol === "https:" ? 443 : 80) }; }
   catch { return null; }
 }
 
@@ -262,6 +265,18 @@ function parseProxy(url) {
 // hanging.
 const hopFor = async () => parseProxy(await resolveHop(true));
 
+// Falling open — dialling the target directly when no chain hop answers — is
+// the DEFAULT and stays that way: a hop restarting is back in ~1s, and refusing
+// meanwhile strands a session whose HTTPS_PROXY was baked at exec, which is the
+// outage this whole chain exists to avoid.
+//
+// It is wrong where the hop is a POLICY boundary rather than a cache. There the
+// direct dial is a silent bypass: the client gets "200 Connection Established"
+// and cannot tell it left unproxied, and until /health started publishing the
+// resolved hop nothing downstream could either. One opt-in, off by default, so
+// the deployment that needs fail-closed can say so instead of discovering it.
+const requireHop = () => process.env.CACHE_FIX_REQUIRE_HOP === "1";
+
 // Blind-tunnel a CONNECT to `target` (host:port) untouched. Routes through the
 // resolved hop when there is one, else dials the target directly. No TLS
 // termination; bytes pass through opaque.
@@ -272,6 +287,11 @@ async function blindTunnel(target, clientSocket, head) {
   // The client may have given up while we probed the chain; dialling for a
   // dead socket leaks the upstream connection.
   if (clientSocket.destroyed) return;
+  if (!via && requireHop()) {
+    process.stderr.write(`[forward-proxy] no chain hop reachable and CACHE_FIX_REQUIRE_HOP=1 — refusing ${target}\n`);
+    clientSocket.end("HTTP/1.1 502 Bad Gateway\r\n\r\n");
+    return;
+  }
   const onUpstream = (upstream) => {
     clientSocket.write("HTTP/1.1 200 Connection Established\r\n\r\n");
     if (head && head.length) upstream.write(head);
@@ -321,6 +341,7 @@ async function connectUpstreamTLS(cb, onErr) {
   // Same chain as the blind tunnel above — see hopFor().
   let via;
   try { via = await hopFor(); } catch (err) { return onErr(err); }
+  if (!via && requireHop()) return onErr(new Error("no chain hop reachable (CACHE_FIX_REQUIRE_HOP=1)"));
   if (via) {
     const r = http.request({ host: via.host, port: via.port, method: "CONNECT",
                              path: `${upHost}:${upPort}`, headers: { host: `${upHost}:${upPort}` } });

proxy/server.mjs

@@ -3,7 +3,7 @@ import { createHash } from "node:crypto";
 import https from "node:https";
 import { pathToFileURL, URL } from "node:url";
 import config from "./config.mjs";
-import { forwardRequest, parseAbsoluteForm, getAgent, fallbackProxyUrls } from "./upstream.mjs";
+import { forwardRequest, parseAbsoluteForm, getAgent, fallbackProxyUrls, lastHop, directLast } from "./upstream.mjs";
 import { streamResponse, createTelemetryRecord } from "./stream.mjs";
 import { loadExtensions, snapshotRegistry, runOnRequest, runOnResponseStart, runOnResponse, getFailedExtensions } from "./pipeline.mjs";
 import { startWatcher } from "./watcher.mjs";
@@ -398,7 +398,33 @@ function handleHealth(_req, res) {
     status: "ok",
     version: config.version,
     forward_proxy: _forwardActive > 0,
-    https_proxy: (_forwardActive > 0 && hopAddress(config.httpsProxy || fallbackProxyUrls()[0])) || null,
+    // THE HOP IN USE ONCE THERE IS ONE. resolveHop() falls THROUGH the chain
+    // and can end at a direct dial, so naming candidate #1 published ":8118"
+    // while CONNECTs left via the second fallback — or via nothing at all. Same
+    // class of lie as the config.httpsProxy-only read above it, one step
+    // further along.
+    //
+    // The three states are distinct and only two of them are a claim about a
+    // dial: a URL is the hop the last one took; "" is "we checked the whole
+    // chain and nothing answered", which must publish null rather than a
+    // candidate; `undefined` is a proxy that has dialled nothing yet — every
+    // successor is in that state for its first request — and there the
+    // configured candidate is the only thing known and asserts nothing false.
+    https_proxy: (_forwardActive > 0 && hopAddress(
+      lastHop() ?? (config.httpsProxy || fallbackProxyUrls()[0]))) || null,
+    // MEASURED OR MERELY CONFIGURED. The line above publishes a URL in two
+    // different situations — the hop a resolve actually used, and the first
+    // candidate on a proxy that has dialled nothing yet — and a reader cannot
+    // tell them apart from the string. cswap's pin raised exactly this against
+    // the fix above: one field carrying two meanings is the same defect as the
+    // one being fixed, and its confirm logic would have to guess.
+    //
+    // So: true = that address was used, false = it is a candidate. `null` in
+    // https_proxy needs no flag; it already means "checked, nothing reachable".
+    https_proxy_measured: _forwardActive > 0 && !!lastHop(),
+    // Sticky: when the chain last fell through to a direct dial (never = null).
+    // See directLast() — a point-in-time field cannot report a flap.
+    direct_last: directLast(),
     // Content fingerprint of the source this process LOADED. Hot-reload is
     // off, so after an edit without a restart this stays at the old value
     // while the working tree moves on — which is precisely the drift an
@@ -897,7 +923,15 @@ export async function startProxy(options = {}) {
         try {
           if (watcher) watcher.close();
         } catch {}
-        server.close((err) => (err ? reject(err) : resolve()));
+        // ERR_SERVER_NOT_RUNNING is not a failure HERE. shutdown() unbinds
+        // first — announcing while we still hold the socket makes the
+        // supervisor race a bind it must lose — and then drains through this,
+        // so the second close always reports "not running" and this promise
+        // ALWAYS rejected on the one path that calls it. Nothing handles that
+        // rejection; only the process.exit() inside .finally() beat the
+        // unhandled-rejection report to it. Both callbacks fire on the same
+        // 'close' event, after the drain, so resolving is the true answer.
+        server.close((err) => (err && err.code !== "ERR_SERVER_NOT_RUNNING" ? reject(err) : resolve()));
       }),
   };
 }
@@ -1220,7 +1254,16 @@ if (invokedAsScript) {
   // "status=1/FAILURE", which (a) makes a crash and a clean stop
   // indistinguishable in the journal and (b) trips Restart=on-failure on a
   // deliberate stop. Force the laggards, report the forcing on stderr, exit 0.
+  // ONCE. SIGTERM, SIGINT and SIGHUP all land here, and a supervised stop
+  // delivers more than one: systemd SIGTERMs the whole control group, so the
+  // proxy gets it directly AND the holder forwards its own SIGHUP. Re-entering
+  // spawns a SECOND successor on fd 3 — two proxies on one socket, which is the
+  // "one extra per deploy" the (handed off) announcement exists to stop — and
+  // announces the release twice, and arms a second 5s force-close.
+  let shuttingDown = false;
   const shutdown = () => {
+    if (shuttingDown) return;
+    shuttingDown = true;
     if (!active) {
       process.exit(0);
       return;