From f4772500bca8450d74d22151fcf72ed5d31c8c49 Mon Sep 17 00:00:00 2001 From: Dizzle <112548150+maxmad64bis@users.noreply.github.com> Date: Thu, 20 Aug 2026 15:32:38 +0200 Subject: [PATCH] fix(api): share one probe-target resolution between both proxy health checks (#10657) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merged — locally validated (15/15 focused tests, typecheck:core clean, file-size/changelog gates green) after resolving base-drift against #10654 (both landed today, same file — combined import block, no logical conflict). Thanks! --- .env.example | 7 + docs/reference/ENVIRONMENT.md | 4 +- .../api/settings/proxies/auto-test/route.ts | 23 ++- src/lib/proxyHealth/probeTarget.ts | 84 +++++++++ src/lib/proxyHealth/scheduler.ts | 21 ++- tests/unit/proxy-probe-target.test.ts | 161 ++++++++++++++++++ 6 files changed, 289 insertions(+), 11 deletions(-) create mode 100644 src/lib/proxyHealth/probeTarget.ts create mode 100644 tests/unit/proxy-probe-target.test.ts diff --git a/.env.example b/.env.example index 940ef094a3..9c4597f72d 100644 --- a/.env.example +++ b/.env.example @@ -1988,6 +1988,13 @@ APP_LOG_TO_FILE=true # Reachability probe target for the scheduler and the auto-test endpoint. # Point it at an internal/self-hosted URL to avoid the public default. # PROXY_HEALTH_TEST_URL=https://httpbin.org/ip +# Probes started at once per batch, for the scheduler and the auto-test endpoint. +# Floored at 1 and capped at 50. Default: 10. +# PROXY_HEALTH_TEST_CONCURRENCY=10 +# Delay in ms between two probe departures inside a batch. Without it the whole batch +# leaves at once and a shared egress IP can trip a rate-limited target. 0 disables the +# spacing; capped at 5000. Default: 100. +# PROXY_HEALTH_TEST_STAGGER_MS=100 # Set "true" to let the scheduler auto-remove proxies after repeated failures. # PROXY_AUTO_REMOVE=false # Consecutive failures before an auto-remove fires. Default: 3. diff --git a/docs/reference/ENVIRONMENT.md b/docs/reference/ENVIRONMENT.md index e1308c8782..9e79b5ffd9 100644 --- a/docs/reference/ENVIRONMENT.md +++ b/docs/reference/ENVIRONMENT.md @@ -1032,7 +1032,9 @@ Anthropic-compatible provider instead. | `PROXY_HEALTH_UNHEALTHY_CACHE_TTL_MS` | `2000` | `src/lib/proxyHealth.ts` | Cache TTL for failed proxy health probes. Keep this shorter than `PROXY_HEALTH_CACHE_TTL_MS` so transient proxy timeouts under high concurrency retry quickly without disabling fast-fail for truly dead proxies. | | `PROXY_HEALTH_ENABLED` | `true` | `src/lib/proxyHealth/scheduler.ts` | Set `false` to disable the background proxy health scheduler that periodically probes registered proxies. | | `PROXY_HEALTH_INTERVAL_MS` | `600000` | `src/lib/proxyHealth/scheduler.ts` | Background health-scheduler sweep interval in ms (minimum `60000`). | -| `PROXY_HEALTH_TEST_URL` | `https://httpbin.org/ip` | `src/lib/proxyHealth/scheduler.ts` | Reachability probe target used by the scheduler and the `/api/settings/proxies/auto-test` endpoint. Point it at an internal/self-hosted URL to avoid the public default. | +| `PROXY_HEALTH_TEST_URL` | `https://httpbin.org/ip` | `src/lib/proxyHealth/probeTarget.ts` | Reachability probe target used by the scheduler and the `/api/settings/proxies/auto-test` endpoint. Point it at an internal/self-hosted URL to avoid the public default. | +| `PROXY_HEALTH_TEST_CONCURRENCY` | `10` | `src/lib/proxyHealth/probeTarget.ts` | Probes started at once per batch, shared by the scheduler and the `/api/settings/proxies/auto-test` endpoint. Floored at 1 and capped at 50. | +| `PROXY_HEALTH_TEST_STAGGER_MS` | `100` | `src/lib/proxyHealth/probeTarget.ts` | Delay in ms between two probe departures inside a batch. Without it the whole batch leaves at the same moment and a shared egress IP can trip a rate-limited target. Set to `0` to disable the spacing; capped at 5000. | | `PROXY_HEALTH_AUTO_DEACTIVATE` | `false` | `src/lib/proxyHealth/statusPolicy.ts` | When `false` (default), automated reachability probes (the scheduler + the `/api/settings/proxies/auto-test` "Test All" button) are **read-only** and never write a proxy's status — only the operator sets active/inactive, so a flaky probe can't strand an assigned proxy (#6246). Set `true` to restore the legacy test-and-set behaviour. | | `PROXY_AUTO_REMOVE` | `false` | `src/lib/proxyHealth/scheduler.ts` | Set `true` to let the scheduler auto-remove proxies after repeated consecutive failures. | | `PROXY_AUTO_REMOVE_AFTER` | `3` | `src/lib/proxyHealth/scheduler.ts` | Consecutive failures before the scheduler auto-removes a proxy (when `PROXY_AUTO_REMOVE=true`). | diff --git a/src/app/api/settings/proxies/auto-test/route.ts b/src/app/api/settings/proxies/auto-test/route.ts index 2a7cc82e7a..b38c0de441 100644 --- a/src/app/api/settings/proxies/auto-test/route.ts +++ b/src/app/api/settings/proxies/auto-test/route.ts @@ -6,14 +6,20 @@ import { createProxyDispatcher, proxyConfigToUrl } from "@omniroute/open-sse/uti import { fetch as undiciFetch } from "undici"; import { classifyProbeStatus } from "@/lib/proxyHealth/decision"; import { resolveHealthCheckStatusWrite } from "@/lib/proxyHealth/statusPolicy"; +import { + resolveProbeConcurrency, + resolveProbeStaggerMs, + resolveProbeTarget, + waitForProbeSlot, +} from "@/lib/proxyHealth/probeTarget"; import { isValidationFailure, validateBody } from "@/shared/validation/helpers"; import { createErrorResponse } from "@/lib/api/errorResponse"; const TEST_TIMEOUT_MS = 5000; -// Reachability probe target. Configurable so operators can point it at an -// internal/self-hosted endpoint instead of the public default. -const TEST_URL = process.env.PROXY_HEALTH_TEST_URL || "https://httpbin.org/ip"; -const CONCURRENCY = 10; +// Shared with the background sweep — see src/lib/proxyHealth/probeTarget.ts. +const TEST_URL = resolveProbeTarget(); +const CONCURRENCY = resolveProbeConcurrency(); +const STAGGER_MS = resolveProbeStaggerMs(); const autoTestSchema = z.object({ ids: z.array(z.string()).optional(), @@ -149,7 +155,14 @@ export async function POST(request: Request) { const results: TestResult[] = []; for (let i = 0; i < proxiesToTest.length; i += CONCURRENCY) { const batch = proxiesToTest.slice(i, i + CONCURRENCY); - const batchResults = await Promise.allSettled(batch.map((proxy) => testSingleProxy(proxy))); + const batchResults = await Promise.allSettled( + batch.map(async (proxy, indexInBatch) => { + // Same intra-batch spacing as the background sweep: "Test All" fires the whole + // batch at once too, so it is just as capable of tripping a rate-limited target. + await waitForProbeSlot(indexInBatch, STAGGER_MS); + return testSingleProxy(proxy); + }) + ); for (const result of batchResults) { if (result.status === "fulfilled") results.push(result.value); } diff --git a/src/lib/proxyHealth/probeTarget.ts b/src/lib/proxyHealth/probeTarget.ts new file mode 100644 index 0000000000..ee423f7c5c --- /dev/null +++ b/src/lib/proxyHealth/probeTarget.ts @@ -0,0 +1,84 @@ +/** + * Shared resolution of the reachability-probe parameters (#8411). + * + * The scheduler sweep and the bulk "Test All" endpoint each carried their own copy of the + * probe target and batch size. Both now resolve through here, so an operator tunes one + * surface instead of two that can silently drift apart. + * + * The resolvers are pure and take the environment as a parameter, so tests never have to + * mutate `process.env`. + */ + +import { sleep } from "@omniroute/open-sse/utils/sleep"; + +export const DEFAULT_PROBE_TARGET = "https://httpbin.org/ip"; +export const DEFAULT_PROBE_CONCURRENCY = 10; +export const DEFAULT_PROBE_STAGGER_MS = 100; + +/** + * Upper bounds. Making the batch size configurable without a ceiling would let a single + * env var recreate the very probe storm this module exists to damp. + */ +export const MAX_PROBE_CONCURRENCY = 50; +export const MAX_PROBE_STAGGER_MS = 5000; + +type ProbeEnv = Record; + +function resolveBoundedInt(raw: string | undefined, fallback: number, min: number, max: number) { + const parsed = parseInt(raw ?? "", 10); + if (!Number.isFinite(parsed)) return fallback; + return Math.min(Math.max(parsed, min), max); +} + +/** + * Deliberately keeps the historical `||` semantics: a target that is empty falls back to the + * default, but one made only of whitespace is passed through untouched. Trimming it here would + * silently change how an existing deployment behaves. + */ +export function resolveProbeTarget(env: ProbeEnv = process.env): string { + return env.PROXY_HEALTH_TEST_URL || DEFAULT_PROBE_TARGET; +} + +/** Floored at 1: a zero batch size would make the `i += concurrency` loop never advance. */ +export function resolveProbeConcurrency(env: ProbeEnv = process.env): number { + return resolveBoundedInt( + env.PROXY_HEALTH_TEST_CONCURRENCY, + DEFAULT_PROBE_CONCURRENCY, + 1, + MAX_PROBE_CONCURRENCY + ); +} + +export function resolveProbeStaggerMs(env: ProbeEnv = process.env): number { + return resolveBoundedInt( + env.PROXY_HEALTH_TEST_STAGGER_MS, + DEFAULT_PROBE_STAGGER_MS, + 0, + MAX_PROBE_STAGGER_MS + ); +} + +/** + * Delay before the Nth probe of a batch starts. + * + * A batch fires `Promise.allSettled(batch.map(...))`, so without this every probe leaves at the + * same tick and a shared egress IP hits the target with `concurrency` simultaneous requests. + * Spacing the departures is what removes that spike; the delay is a plain multiple of the index + * rather than a random jitter so a sweep stays reproducible and exactly testable. + * + * The first probe of a batch always returns 0 — no batch is ever slowed down at its head. + */ +export function staggerDelayMs(indexInBatch: number, stepMs: number): number { + if (indexInBatch <= 0 || stepMs <= 0) return 0; + return indexInBatch * stepMs; +} + +/** + * Hold a probe back until its slot in the batch. Call this from the batch `map`, before the + * probe itself: both call sites arm their timeout inside their own test function, so waiting + * out here is what keeps every probe's timeout budget whole. + */ +export async function waitForProbeSlot(indexInBatch: number, stepMs: number): Promise { + const delay = staggerDelayMs(indexInBatch, stepMs); + if (delay > 0) await sleep(delay); +} diff --git a/src/lib/proxyHealth/scheduler.ts b/src/lib/proxyHealth/scheduler.ts index 6a0a4c4da0..e70583ce85 100644 --- a/src/lib/proxyHealth/scheduler.ts +++ b/src/lib/proxyHealth/scheduler.ts @@ -34,16 +34,24 @@ import { decideProxyHealthAction, type ProxyProbeOutcome, } from "./decision.ts"; +import { + resolveProbeConcurrency, + resolveProbeStaggerMs, + resolveProbeTarget, + waitForProbeSlot, +} from "./probeTarget.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 // flipped healthy proxies to inactive. Raise it and treat our own timeout as // inconclusive (see testOneProxy) rather than a proxy failure. const TEST_TIMEOUT_MS = 15000; -// Reachability probe target for proxy health checks. Configurable so operators -// can point it at an internal/self-hosted endpoint instead of the public default. -const TEST_URL = process.env.PROXY_HEALTH_TEST_URL || "https://httpbin.org/ip"; -const CONCURRENCY = 10; +// Probe target, batch size and intra-batch spacing come from probeTarget.ts, which the +// auto-test endpoint reads too — one surface to tune instead of two that can drift apart. +// Resolved at module load, as these constants always were. +const TEST_URL = resolveProbeTarget(); +const CONCURRENCY = resolveProbeConcurrency(); +const STAGGER_MS = resolveProbeStaggerMs(); const INITIAL_DELAY_MS = 60_000; const DEFAULT_INTERVAL_MS = 600_000; const DEFAULT_REMOVE_AFTER = 3; @@ -160,7 +168,10 @@ async function sweep(): Promise { for (let i = 0; i < proxies.length; i += CONCURRENCY) { const batch = proxies.slice(i, i + CONCURRENCY); const results = await Promise.allSettled( - batch.map(async (proxy) => { + batch.map(async (proxy, indexInBatch) => { + // Spread the departures: without this the whole batch leaves at the same tick and a + // shared egress IP hits the target with CONCURRENCY simultaneous requests. + await waitForProbeSlot(indexInBatch, STAGGER_MS); const outcome = await testOneProxy(proxy); return { id: proxy.id, outcome }; }) diff --git a/tests/unit/proxy-probe-target.test.ts b/tests/unit/proxy-probe-target.test.ts new file mode 100644 index 0000000000..30f66a0b6d --- /dev/null +++ b/tests/unit/proxy-probe-target.test.ts @@ -0,0 +1,161 @@ +/** + * Guards for the shared probe-target resolution (#8411). + * + * The scheduler sweep and the "Test All" endpoint used to carry their own copy of the probe + * target and batch size. They now share src/lib/proxyHealth/probeTarget.ts, so these tests pin + * two things: that the defaults still match what both call sites hardcoded before, and that no + * environment value can produce a batch size that would hang the sweep loop. + */ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { + DEFAULT_PROBE_CONCURRENCY, + DEFAULT_PROBE_STAGGER_MS, + DEFAULT_PROBE_TARGET, + MAX_PROBE_CONCURRENCY, + MAX_PROBE_STAGGER_MS, + resolveProbeConcurrency, + resolveProbeStaggerMs, + resolveProbeTarget, + staggerDelayMs, + waitForProbeSlot, +} from "../../src/lib/proxyHealth/probeTarget.ts"; + +// ─── the loop-safety invariant ───────────────────────── + +test("no environment value can yield a batch size below 1", () => { + // `for (i = 0; i < n; i += concurrency)` never advances at 0 and walks backwards below it, + // so this is the one property that turns a bad config into a hung sweep rather than a slow one. + const hostile = [ + "0", + "-1", + "-9999", + "", + " ", + "abc", + "NaN", + "Infinity", + "-Infinity", + "1e-9", + "0.4", + "null", + undefined, + ]; + for (const raw of hostile) { + const resolved = resolveProbeConcurrency({ PROXY_HEALTH_TEST_CONCURRENCY: raw }); + assert.ok( + Number.isInteger(resolved) && resolved >= 1, + `concurrency ${JSON.stringify(raw)} resolved to ${resolved}` + ); + } +}); + +// ─── defaults: what both call sites hardcoded before ─── + +test("the defaults reproduce the previous hardcoded values", () => { + assert.equal(resolveProbeTarget({}), DEFAULT_PROBE_TARGET); + assert.equal(DEFAULT_PROBE_TARGET, "https://httpbin.org/ip"); + assert.equal(resolveProbeConcurrency({}), DEFAULT_PROBE_CONCURRENCY); + assert.equal(DEFAULT_PROBE_CONCURRENCY, 10); + assert.equal(resolveProbeStaggerMs({}), DEFAULT_PROBE_STAGGER_MS); +}); + +// ─── target ──────────────────────────────────────────── + +test("an operator-supplied target wins", () => { + assert.equal( + resolveProbeTarget({ PROXY_HEALTH_TEST_URL: "https://probe.internal/ping" }), + "https://probe.internal/ping" + ); +}); + +test("an empty target falls back, a whitespace one is left untouched", () => { + assert.equal(resolveProbeTarget({ PROXY_HEALTH_TEST_URL: "" }), DEFAULT_PROBE_TARGET); + // Whitespace is truthy, so it reached the probe before this refactor. Trimming it here would + // silently repair a broken deployment — a behavior change this change is not entitled to make. + assert.equal(resolveProbeTarget({ PROXY_HEALTH_TEST_URL: " " }), " "); +}); + +// ─── concurrency ─────────────────────────────────────── + +test("concurrency honours a sane value and is capped", () => { + assert.equal(resolveProbeConcurrency({ PROXY_HEALTH_TEST_CONCURRENCY: "3" }), 3); + assert.equal( + resolveProbeConcurrency({ PROXY_HEALTH_TEST_CONCURRENCY: "10000" }), + MAX_PROBE_CONCURRENCY + ); +}); + +test("an unparseable concurrency falls back instead of yielding NaN", () => { + assert.equal( + resolveProbeConcurrency({ PROXY_HEALTH_TEST_CONCURRENCY: "abc" }), + DEFAULT_PROBE_CONCURRENCY + ); +}); + +// ─── stagger ─────────────────────────────────────────── + +test("the stagger step is clamped to its bounds", () => { + assert.equal(resolveProbeStaggerMs({ PROXY_HEALTH_TEST_STAGGER_MS: "250" }), 250); + assert.equal(resolveProbeStaggerMs({ PROXY_HEALTH_TEST_STAGGER_MS: "0" }), 0); + assert.equal(resolveProbeStaggerMs({ PROXY_HEALTH_TEST_STAGGER_MS: "-50" }), 0); + assert.equal( + resolveProbeStaggerMs({ PROXY_HEALTH_TEST_STAGGER_MS: "999999" }), + MAX_PROBE_STAGGER_MS + ); +}); + +test("an unparseable stagger falls back instead of yielding NaN", () => { + const resolved = resolveProbeStaggerMs({ PROXY_HEALTH_TEST_STAGGER_MS: "later" }); + assert.equal(resolved, DEFAULT_PROBE_STAGGER_MS); + assert.ok(Number.isFinite(resolved)); +}); + +// ─── delay computation ───────────────────────────────── + +test("the head of a batch is never delayed", () => { + assert.equal(staggerDelayMs(0, 100), 0); +}); + +test("delays grow strictly with the position in the batch", () => { + const step = 100; + for (let i = 1; i < 10; i++) { + assert.equal(staggerDelayMs(i, step), i * step); + assert.ok(staggerDelayMs(i, step) > staggerDelayMs(i - 1, step)); + } +}); + +test("a zero or negative step inserts no wait at all", () => { + for (const step of [0, -1]) { + for (let i = 0; i < 5; i++) { + assert.equal(staggerDelayMs(i, step), 0); + } + } +}); + +test("a negative index cannot produce a negative wait", () => { + assert.equal(staggerDelayMs(-1, 100), 0); +}); + +// ─── the wait itself ─────────────────────────────────── + +test("the head of a batch is not held back at all", async () => { + const before = Date.now(); + await waitForProbeSlot(0, 100); + // No timer is armed for slot 0, so this resolves on the microtask queue. The assertion is + // deliberately loose: it proves no ~100ms timer fired, not a precise scheduler timing. + assert.ok(Date.now() - before < 50, "slot 0 must not wait"); +}); + +test("a later slot is actually held back", async () => { + const before = Date.now(); + await waitForProbeSlot(2, 30); + assert.ok(Date.now() - before >= 55, "slot 2 must wait about two steps"); +}); + +test("a disabled step holds nobody back", async () => { + const before = Date.now(); + await Promise.all([waitForProbeSlot(5, 0), waitForProbeSlot(9, 0)]); + assert.ok(Date.now() - before < 50, "a zero step must insert no wait"); +});