fix(api): a target refusing the egress IP is not a healthy proxy (#10654)

Merged — locally validated in a combined batch worktree with the related proxy-health-probe PRs (typecheck:core clean, gates green). Thanks!

src/app/api/settings/proxies/auto-test/route.ts

@@ -4,6 +4,7 @@ import { createErrorResponseFromUnknown } from "@/lib/api/errorResponse";
 import { requireManagementAuth } from "@/lib/api/requireManagementAuth";
 import { createProxyDispatcher, proxyConfigToUrl } from "@omniroute/open-sse/utils/proxyDispatcher";
 import { fetch as undiciFetch } from "undici";
+import { classifyProbeStatus } from "@/lib/proxyHealth/decision";
 import { resolveHealthCheckStatusWrite } from "@/lib/proxyHealth/statusPolicy";
 import { isValidationFailure, validateBody } from "@/shared/validation/helpers";
 import { createErrorResponse } from "@/lib/api/errorResponse";
@@ -24,6 +25,13 @@ interface TestResult {
   host: string;
   port: number;
   alive: boolean;
+  /**
+   * The proxy relayed, but the target refused this egress IP (401/403/429).
+   * Reported alongside `alive` rather than inside it: a refused IP is still a
+   * reachable proxy, so folding it into `alive` would change what the opt-in
+   * status write (`PROXY_HEALTH_AUTO_DEACTIVATE`) deactivates.
+   */
+  blockedByTarget?: boolean;
   latencyMs: number | null;
   error?: string;
 }
@@ -66,13 +74,24 @@ async function testSingleProxy(proxy: {
       headers: { "User-Agent": "OmniRoute/1.0" },
     });
     const latencyMs = Date.now() - start;
-    const alive = resp.status < 500;
+    const outcome = classifyProbeStatus(resp.status);
+    // Same shared classifier the sweep uses. `alive` keeps its exact prior meaning
+    // (any status under 500): "blocked" covers 401/403/429, which were — and stay —
+    // alive here, so no proxy changes state because of this field.
+    const alive = outcome === "ok" || outcome === "blocked";
     // #6246: "Test All" is a test, not test-and-set. By default an automated probe
     // never mutates a proxy's status (only the operator does). Opt back into the
     // legacy write with PROXY_HEALTH_AUTO_DEACTIVATE=true.
     const statusWrite = resolveHealthCheckStatusWrite(alive);
     if (statusWrite) await updateProxy(proxy.id, { status: statusWrite }).catch(() => {});
-    return { proxyId: proxy.id, host: proxy.host, port: proxy.port, alive, latencyMs };
+    return {
+      proxyId: proxy.id,
+      host: proxy.host,
+      port: proxy.port,
+      alive,
+      ...(outcome === "blocked" ? { blockedByTarget: true } : {}),
+      latencyMs,
+    };
   } catch (err) {
     const latencyMs = Date.now() - start;
     const statusWrite = resolveHealthCheckStatusWrite(false);

src/lib/proxyHealth/decision.ts

@@ -2,7 +2,7 @@
  * Pure, network-free decision for the proxy health scheduler (#6246).
  *
  * Separated from the sweep so the status/removal policy can be unit-tested
- * exhaustively without any I/O. The sweep classifies each probe into a tri-state
+ * exhaustively without any I/O. The sweep classifies each probe into a
  * {@link ProxyProbeOutcome} and applies the returned {@link ProxyHealthDecision}.
  *
  * Policy (agreed for #6246, extended for the auto-disable mode below):
@@ -27,12 +27,33 @@
  *       is free once autoDisable participates in `managesStatus` below. If
  *       both flags are set, auto-remove (destructive) wins: a proxy that is
  *       about to be deleted has no use for a soft-disable in between.
+ *   E — a `blocked` probe (the TARGET refused this egress IP: 401/403/429) is
+ *       neutral like `inconclusive`. The proxy relayed correctly, so it is not
+ *       failing; but it is not serving that destination either, which `ok` hid.
+ *       Kept out of the failure count on purpose: one target refusing an IP
+ *       does not make the proxy dead, and the operator owns the removal policy.
  */
 
-export type ProxyProbeOutcome = "ok" | "fail" | "inconclusive";
+export type ProxyProbeOutcome = "ok" | "fail" | "inconclusive" | "blocked";
+
+/** Statuses that mean the TARGET refused this egress IP rather than served it. */
+const TARGET_BLOCK_STATUSES: ReadonlySet<number> = new Set([401, 403, 429]);
+
+/**
+ * PURE: classify a probe response status into a {@link ProxyProbeOutcome}.
+ *
+ * `ok` requires the target to have actually served the request. A 401/403/429
+ * means the proxy relayed but the destination refused the egress IP — the case
+ * a generic "status < 500" test reported as a healthy proxy.
+ */
+export function classifyProbeStatus(status: number): ProxyProbeOutcome {
+  if (TARGET_BLOCK_STATUSES.has(status)) return "blocked";
+  // A 5xx means the proxy DID relay — the target is at fault, not the proxy.
+  return status < 500 ? "ok" : "inconclusive";
+}
 
 export interface ProxyHealthDecisionInput {
-  /** Tri-state result of the reachability probe for this proxy. */
+  /** Classified result of the reachability probe for this proxy. */
   outcome: ProxyProbeOutcome;
   /** Consecutive failure count recorded BEFORE this probe. */
   priorFailures: number;
@@ -65,8 +86,8 @@ export function decideProxyHealthAction(input: ProxyHealthDecisionInput): ProxyH
   // Either opt-in flag hands status control from the operator to the sweep.
   const managesStatus = autoRemove || autoDisable;
 
-  // B: inconclusive probes are neutral — do not touch count or status.
-  if (outcome === "inconclusive") {
+  // B/E: inconclusive and blocked probes are neutral — no count, no status.
+  if (outcome === "inconclusive" || outcome === "blocked") {
     return { failures: priorFailures, clearFailures: false, setStatus: null, remove: false };
   }
 

src/lib/proxyHealth/scheduler.ts

@@ -29,7 +29,11 @@ import {
   proxyConfigToUrl,
 } from "@omniroute/open-sse/utils/proxyDispatcher";
 import { fetch as undiciFetch } from "undici";
-import { decideProxyHealthAction, type ProxyProbeOutcome } from "./decision.ts";
+import {
+  classifyProbeStatus,
+  decideProxyHealthAction,
+  type ProxyProbeOutcome,
+} from "./decision.ts";
 
 // #6246: a HEAD to the public probe target through a legit (often loaded) proxy
 // can exceed a few seconds; the old 5s ceiling produced false negatives that
@@ -90,9 +94,12 @@ function isBackgroundServicesDisabled(): boolean {
 }
 
 /**
- * Reachability probe for one proxy, classified into a tri-state so the pure
+ * Reachability probe for one proxy, classified so the pure
  * decision layer can apply the #6246 policy:
- *   - "ok"           — the proxy relayed and the target answered (<500).
+ *   - "ok"           — the proxy relayed and the target served the request.
+ *   - "blocked"      — the proxy relayed, but the TARGET refused this egress IP
+ *                      (401/403/429). Neutral like "inconclusive": the proxy is
+ *                      not at fault, yet it is not serving that destination.
  *   - "inconclusive" — NOT the proxy's fault: our own timeout/abort, or the probe
  *                      TARGET returned a 5xx (the proxy connected fine). Never
  *                      penalizes the proxy.
@@ -124,9 +131,7 @@ async function testOneProxy(proxy: {
       dispatcher,
       headers: { "User-Agent": "OmniRoute/1.0" },
     });
-    // A 5xx from the probe target means the proxy DID relay — the target is at
-    // fault, not the proxy. Do not penalize the proxy for that.
-    return resp.status < 500 ? "ok" : "inconclusive";
+    return classifyProbeStatus(resp.status);
   } catch {
     // Our own deadline elapsed → inconclusive (slow, not necessarily dead).
     // Any other error is a genuine proxy-level connection failure.
@@ -148,6 +153,7 @@ async function sweep(): Promise<void> {
   let tested = 0;
   let alive = 0;
   let inconclusive = 0;
+  let blocked = 0;
   let removed = 0;
   let disabled = 0;
 
@@ -166,6 +172,7 @@ async function sweep(): Promise<void> {
       tested++;
       if (outcome === "ok") alive++;
       else if (outcome === "inconclusive") inconclusive++;
+      else if (outcome === "blocked") blocked++;
 
       const decision = decideProxyHealthAction({
         outcome,
@@ -202,8 +209,8 @@ async function sweep(): Promise<void> {
   }
 
   console.log(
-    `${LOG_PREFIX} Sweep complete: ${tested} tested, ${alive} alive, ${inconclusive} inconclusive, ` +
-      `${removed} auto-removed, ${disabled} auto-disabled`
+    `${LOG_PREFIX} Sweep complete: ${tested} tested, ${alive} alive, ${blocked} blocked by target, ` +
+      `${inconclusive} inconclusive, ${removed} auto-removed, ${disabled} auto-disabled`
   );
 }
 

tests/unit/proxy-health-blocked-outcome.test.ts

@@ -0,0 +1,105 @@
+/**
+ * A target that refuses the egress IP is not a healthy proxy (policy E).
+ *
+ * The probe classified any response under 500 as "ok", so a 401/403/429 from the
+ * probe target — the shape a destination uses to refuse a banned or rate-limited
+ * IP — was reported as a healthy proxy. The proxy did relay, so it is not
+ * failing; but it is not serving that destination either, and "ok" hid that.
+ *
+ * `blocked` is deliberately NEUTRAL in the decision layer: one target refusing an
+ * IP does not make the proxy dead, and the removal policy stays operator-owned.
+ */
+import test from "node:test";
+import assert from "node:assert/strict";
+
+const { classifyProbeStatus, decideProxyHealthAction } =
+  await import("../../src/lib/proxyHealth/decision.ts");
+
+// ─── classifyProbeStatus ───────────────────────────────
+
+test("a target refusing the egress IP is blocked, not ok", () => {
+  for (const status of [401, 403, 429]) {
+    assert.equal(classifyProbeStatus(status), "blocked", `status ${status}`);
+  }
+});
+
+test("a target that served the request is ok", () => {
+  for (const status of [200, 204, 301, 400, 404]) {
+    assert.equal(classifyProbeStatus(status), "ok", `status ${status}`);
+  }
+});
+
+test("a 5xx from the target stays inconclusive — the proxy relayed fine", () => {
+  for (const status of [500, 502, 503]) {
+    assert.equal(classifyProbeStatus(status), "inconclusive", `status ${status}`);
+  }
+});
+
+// ─── decideProxyHealthAction: policy E ─────────────────
+
+test("blocked never counts as a failure and never touches status", () => {
+  const d = decideProxyHealthAction({
+    outcome: "blocked",
+    priorFailures: 2,
+    autoRemove: false,
+    autoDisable: false,
+    removeAfter: 3,
+  });
+  assert.deepEqual(d, { failures: 2, clearFailures: false, setStatus: null, remove: false });
+});
+
+test("blocked cannot remove or disable a proxy, even at the threshold with both flags on", () => {
+  // The destructive path is the one that must never be reachable from `blocked`:
+  // prior failures already sit at the threshold and both opt-ins are enabled, so
+  // an outcome counted as a failure WOULD delete the proxy here.
+  const d = decideProxyHealthAction({
+    outcome: "blocked",
+    priorFailures: 3,
+    autoRemove: true,
+    autoDisable: true,
+    removeAfter: 3,
+  });
+  assert.equal(d.remove, false);
+  assert.equal(d.setStatus, null);
+  assert.equal(d.failures, 3, "the streak must be neither advanced nor reset");
+});
+
+test("blocked does not reset a failure streak the way ok does", () => {
+  const blockedDecision = decideProxyHealthAction({
+    outcome: "blocked",
+    priorFailures: 2,
+    autoRemove: true,
+    autoDisable: false,
+    removeAfter: 3,
+  });
+  const okDecision = decideProxyHealthAction({
+    outcome: "ok",
+    priorFailures: 2,
+    autoRemove: true,
+    autoDisable: false,
+    removeAfter: 3,
+  });
+  assert.equal(blockedDecision.clearFailures, false);
+  assert.equal(okDecision.clearFailures, true);
+});
+
+// ─── behaviour preservation for "Test All" ─────────────
+
+test("alive keeps its exact prior meaning for every status", () => {
+  // `/api/settings/proxies/auto-test` computed `alive = status < 500`. It now derives
+  // it from the shared classifier; this asserts the two agree on the whole range, so
+  // no proxy changes state under PROXY_HEALTH_AUTO_DEACTIVATE because of this PR.
+  for (let status = 100; status < 600; status++) {
+    const outcome = classifyProbeStatus(status);
+    const alive = outcome === "ok" || outcome === "blocked";
+    assert.equal(alive, status < 500, `status ${status}`);
+  }
+});
+
+test("only the target-refusal statuses are flagged blocked across the whole range", () => {
+  const flagged = [];
+  for (let status = 100; status < 600; status++) {
+    if (classifyProbeStatus(status) === "blocked") flagged.push(status);
+  }
+  assert.deepEqual(flagged, [401, 403, 429]);
+});