diff --git a/src/app/api/settings/proxies/auto-test/route.ts b/src/app/api/settings/proxies/auto-test/route.ts index 9023f4f909..2a7cc82e7a 100644 --- a/src/app/api/settings/proxies/auto-test/route.ts +++ b/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); diff --git a/src/lib/proxyHealth/decision.ts b/src/lib/proxyHealth/decision.ts index daefaa6a5b..ae58cde705 100644 --- a/src/lib/proxyHealth/decision.ts +++ b/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 = 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 }; } diff --git a/src/lib/proxyHealth/scheduler.ts b/src/lib/proxyHealth/scheduler.ts index 2008acf709..6a0a4c4da0 100644 --- a/src/lib/proxyHealth/scheduler.ts +++ b/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 { 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 { 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 { } 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` ); } diff --git a/tests/unit/proxy-health-blocked-outcome.test.ts b/tests/unit/proxy-health-blocked-outcome.test.ts new file mode 100644 index 0000000000..3acb447cf2 --- /dev/null +++ b/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]); +});