Skip to content

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

Merged
diegosouzapw merged 1 commit into
cheaper-inference:release/v3.8.50from
maxmad64bis:fix/proxy-probe-target-block-status
Aug 20, 2026
Merged

diegosouzapw merged 1 commit into
cheaper-inference:release/v3.8.50from
maxmad64bis:fix/proxy-probe-target-block-status

Conversation

@maxmad64bis

Copy link
Copy Markdown
Contributor

Summary

  • The proxy reachability probe classified every response under 500 as ok
    (src/lib/proxyHealth/scheduler.ts). A 401/403/429 from the probe target — the shape a
    destination uses to refuse a banned or rate-limited egress IP — was therefore 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 exactly that.
  • Adds a blocked outcome for 401/403/429, produced by a shared pure classifyProbeStatus().
    The sweep and the "Test All" endpoint (/api/settings/proxies/auto-test) were deriving the
    same status < 500 rule independently; they now share one definition.
  • The sweep log gains a blocked by target count next to alive / inconclusive.

Behaviour is deliberately unchanged.

  • blocked is neutral in decideProxyHealthAction, exactly like inconclusive: no failure
    count, no status write. One target refusing an IP does not make the proxy dead, and the
    removal policy stays operator-owned ([BUG] Massive issue with proxies since 3.8.44 - IP leak #6246 policy C/D).
  • On /api/settings/proxies/auto-test, alive keeps its exact prior meaning (any status under
    500). The refusal is surfaced as a separate optional blockedByTarget field, so nothing
    changes for the opt-in status write behind PROXY_HEALTH_AUTO_DEACTIVATE.

No migration, no feature flag, no UI string, no i18n churn.

Related Issues

Validation

  • Change type: routing / proxy health (+ API field)
  • Focused tests and category gates from the golden path
  • npm run lint
  • Reconciled with the current active release base; focused checks rerun afterward
  • Production-code changes include a new or updated automated test in this PR
$ node --import tsx/esm --test tests/unit/proxy-health-blocked-outcome.test.ts
# tests 8   # pass 8   # fail 0

$ node --import tsx/esm --test tests/unit/proxy-health-6246.test.ts                  # pass 7  # fail 0
$ node --import tsx/esm --test tests/unit/proxy-health-decide-action-6246.test.ts    # pass 6  # fail 0
$ node --import tsx/esm --test tests/unit/proxy-health-auto-disable-decision.test.ts # pass 7  # fail 0
$ node --import tsx/esm --test tests/unit/proxy-batch-routes-5918.test.ts            # pass 5  # fail 0

$ npm run lint                       # rc=0 (cold cache)
$ npm run check:cycles               # OK — no cycles across 419 files
$ npm run typecheck:core 2>&1 | grep "error TS"
open-sse/services/compression/engines/omniglyphAdapter.ts(126,54): error TS2339: ...   # 1, pre-existing on base

The single typecheck error is already tracked on the base and lives in a file this PR does not
touch.

⚠️ base-red inherited: #9985

Tests Added Or Updated

  • tests/unit/proxy-health-blocked-outcome.test.ts (new, 8 tests):
    • classifyProbeStatus maps 401/403/429 → blocked, 200/204/301/400/404 → ok,
      500/502/503 → inconclusive;
    • exhaustive over 100–599: exactly [401, 403, 429] are flagged blocked, and
      alive (ok || blocked) equals the previous status < 500 for every status — the
      regression guard for the "Test All" endpoint keeping its behaviour;
    • decideProxyHealthAction on blocked touches neither count nor status, including at the
      removal threshold with both autoRemove and autoDisable enabled — the destructive path is
      proven unreachable from blocked;
    • blocked does not reset a failure streak the way ok does.

Coverage Notes

  • Touched src/: classifyProbeStatus() (pure, every branch covered), the blocked branch of
    decideProxyHealthAction (covered, including the destructive-path case), and the two call
    sites that now consume the shared classifier.
  • No coverage moved down; scheduler.ts loses an inline ternary to a tested function.

Reviewer Notes

  • ProxyProbeOutcome widens from three values to four. Both consumers live in this PR
    (scheduler.ts, decision.ts), and the union is closed, so TypeScript enforces that the new
    case is handled — that exhaustiveness is the point.
  • 407 is not part of this change. undici never surfaces a 407 as a response — it throws,
    both on an HTTPS CONNECT tunnel and on a plain HTTP request — so a proxy demanding
    authentication already lands in the catch and is classified fail. Verified against a local
    proxy answering 407 to everything; no change was warranted.
  • Known limitation, deliberately not guarded: if the probe target itself degrades and answers 403
    to everyone, every proxy is reported blocked for that sweep. Because blocked is neutral, the
    worst case is one misleading log line — no proxy is disabled or removed. A guard would need
    pool-wide aggregation in the sweep for a strictly cosmetic gain.
  • blockedByTarget is optional and only present when true, so existing consumers of the
    "Test All" response are unaffected.
  • Folding blocked into alive was considered and rejected: under
    PROXY_HEALTH_AUTO_DEACTIVATE=true it would start deactivating proxies that are currently left
    active, which is a policy change rather than an observability fix.

The reachability probe classified every 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 exactly that.

Add a "blocked" outcome for 401/403/429, produced by a shared, pure
classifyProbeStatus() so the sweep and the "Test All" endpoint stop deriving
the same rule twice.

Behaviour is deliberately unchanged:
- "blocked" is neutral in decideProxyHealthAction, like "inconclusive": no
  failure count, no status write. One target refusing an IP does not make the
  proxy dead, and the removal policy stays operator-owned (cheaper-inference#6246 policy C/D).
- On /api/settings/proxies/auto-test, `alive` keeps its exact prior meaning
  (any status under 500). The refusal is surfaced as a separate optional
  `blockedByTarget` field, so nothing changes for the opt-in status write
  behind PROXY_HEALTH_AUTO_DEACTIVATE.

The sweep log gains a "blocked by target" count. No API contract change beyond
one optional field, no UI string, no migration.
@diegosouzapw

Copy link
Copy Markdown
Collaborator

Verified locally against release/v3.8.50: classifyProbeStatus and the blocked branch of decideProxyHealthAction are covered by 8 new tests (all passing), and the adjacent proxy-health suites (25 tests) still pass with no regressions. The widened ProxyProbeOutcome union has exactly two consumers (both touched here) and both handle the new case exhaustively. The two ESLint findings on the touched files are pre-existing allowlisted no-restricted-imports violations on an untouched import line, not introduced by this diff. This looks merge-ready as-is — thanks for the thorough test coverage and the exhaustive-range regression guard proving alive semantics are unchanged for the "Test All" endpoint.

@diegosouzapw
diegosouzapw merged commit b43ad73 into cheaper-inference:release/v3.8.50 Aug 20, 2026
5 checks passed
diegosouzapw pushed a commit that referenced this pull request Aug 20, 2026
… checks (#10657)

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!
diegosouzapw pushed a commit that referenced this pull request Aug 20, 2026
…ic target (#10664)

Merged — locally validated (16/16 focused tests, typecheck:core clean, file-size/changelog gates green) after resolving base-drift against #10654/#10657 (both landed earlier today, same file chain — combined additively, no logical conflict). Nice chain of proxy-probe-target fixes, thanks!
diegosouzapw added a commit to maxmad64bis/OmniRouteFork that referenced this pull request Sep 15, 2026
…ag (default off)

Registers the switch for the proxy health policy change of cheaper-inference#13608 as a
regular feature flag (health category, defaultValue "false"), so a
target-refused probe keeps the deliberate cheaper-inference#10654 neutral policy unless an
operator opts in. The environment variable of the same name works through
the normal flag resolution (DB override > env > default).

Adds the catalog row in FEATURE_FLAGS.md, the description i18n key (vi and
pt-BR translated, other locales deferred with the __MISSING__ marker) and
bumps the flag-count assertions from 55 to 56.
diegosouzapw added a commit to maxmad64bis/OmniRouteFork that referenced this pull request Sep 15, 2026
… only behind PROXY_HEALTH_BLOCKED_RESETS_STREAK

cheaper-inference#13608 made every target-refused probe (401/403/429) reset the
consecutive-failure streak, reversing the deliberate cheaper-inference#10654 policy for all
installs. Keep the idea as an opt-in:

- decideProxyHealthAction() takes an optional blockedResetsStreak input
  (default false = blocked stays neutral like inconclusive, exactly as on the
  base); the sweep reads the PROXY_HEALTH_BLOCKED_RESETS_STREAK feature flag
  once per sweep and passes it in. With the flag on a refusal resets the
  streak but still never counts, sets a status or removes a proxy.
- Fix the comments: decision.ts policy E no longer claims that any relayed
  HTTP response resets the streak (a relayed 5xx stays inconclusive and keeps
  it), and the stale testOneProxy JSDoc in scheduler.ts describes both modes.
- Restore the existing policy-E assertions of
  proxy-health-blocked-outcome.test.ts to the base contract and add flag-on
  cases next to them.
- Replace the source-grep check of the sweep summary with a behavioral test
  through forceProxyHealthSweep(): a local relay answering 403 drives
  fail -> blocked -> fail with PROXY_AUTO_DISABLE and a threshold of 2 (flag
  off: the proxy ends dead; flag on: it keeps its status), and the captured
  summary line pins the refusal tally.
diegosouzapw pushed a commit that referenced this pull request Sep 15, 2026
…ure streak (#13608)

Behind the new `PROXY_HEALTH_BLOCKED_RESETS_STREAK` flag (default off), a probe the target refuses (401/403/429) resets the proxy's consecutive-failure streak, so a proxy that clearly relays is not marked dead by spaced-out real failures.

Maintainer rework before merge (kept the idea, no default behavior change):
- The original reversed the deliberate #10654 policy for everyone; with the flag off a refusal stays neutral, and the existing assertions are restored. The stale JSDoc and the wrong "any relayed response resets" comment are fixed (5xx stays inconclusive).
- The source-grep test became a real sweep test: a local relay answering 403 drives fail → blocked → fail with auto-disable, in both flag modes.

Validated first on the combined board of all 38 PRs of this batch (10 merged as-is, 28 after the maintainer rework) on top of release/v3.8.51 c0f92ec: typecheck:core, check:open-sse-typecheck and check:dashboard-typecheck clean; ESLint clean on every changed file; file-size (rebaselined for the combined growth), complexity, cognitive-complexity, changelog-integrity, docs-counts, docs-sync, migration-numbering and i18n new-key gates green; 735 focused node:test cases with the only batch-caused failure (a flag-count assertion) fixed. Then re-validated alone on the fresh release tip right before this merge: ESLint on the changed files, typecheck:core, check:open-sse-typecheck, the file-size/complexity/changelog gates and this PR's own tests.

Thanks @maxmad64bis!
@maxmad64bis
maxmad64bis deleted the fix/proxy-probe-target-block-status branch September 24, 2026 21:15
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…aper-inference#10654)

Merged — locally validated in a combined batch worktree with the related proxy-health-probe PRs (typecheck:core clean, gates green). Thanks!
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
… checks (cheaper-inference#10657)

Merged — locally validated (15/15 focused tests, typecheck:core clean, file-size/changelog gates green) after resolving base-drift against cheaper-inference#10654 (both landed today, same file — combined import block, no logical conflict). Thanks!
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…ic target (cheaper-inference#10664)

Merged — locally validated (16/16 focused tests, typecheck:core clean, file-size/changelog gates green) after resolving base-drift against cheaper-inference#10654/cheaper-inference#10657 (both landed earlier today, same file chain — combined additively, no logical conflict). Nice chain of proxy-probe-target fixes, thanks!
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…ure streak (cheaper-inference#13608)

Behind the new `PROXY_HEALTH_BLOCKED_RESETS_STREAK` flag (default off), a probe the target refuses (401/403/429) resets the proxy's consecutive-failure streak, so a proxy that clearly relays is not marked dead by spaced-out real failures.

Maintainer rework before merge (kept the idea, no default behavior change):
- The original reversed the deliberate cheaper-inference#10654 policy for everyone; with the flag off a refusal stays neutral, and the existing assertions are restored. The stale JSDoc and the wrong "any relayed response resets" comment are fixed (5xx stays inconclusive).
- The source-grep test became a real sweep test: a local relay answering 403 drives fail → blocked → fail with auto-disable, in both flag modes.

Validated first on the combined board of all 38 PRs of this batch (10 merged as-is, 28 after the maintainer rework) on top of release/v3.8.51 d61b804: typecheck:core, check:open-sse-typecheck and check:dashboard-typecheck clean; ESLint clean on every changed file; file-size (rebaselined for the combined growth), complexity, cognitive-complexity, changelog-integrity, docs-counts, docs-sync, migration-numbering and i18n new-key gates green; 735 focused node:test cases with the only batch-caused failure (a flag-count assertion) fixed. Then re-validated alone on the fresh release tip right before this merge: ESLint on the changed files, typecheck:core, check:open-sse-typecheck, the file-size/complexity/changelog gates and this PR's own tests.

Thanks @maxmad64bis!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants