fix(proxy): shorten fast-fail negative health cache - #5255
diegosouzapw merged 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a short negative cache for transiently unreachable proxy health checks, preventing failed probes from poisoning the proxy cache for the full TTL duration, and adds a corresponding unit test. The review feedback correctly identifies a potential issue where an invalid or empty environment variable for the unhealthy cache TTL could result in NaN propagation, disabling the negative cache, and provides a robust fallback suggestion.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const UNHEALTHY_CACHE_TTL_MS = parseInt( | ||
| process.env.PROXY_HEALTH_UNHEALTHY_CACHE_TTL_MS ?? "2000", | ||
| 10 | ||
| ); |
There was a problem hiding this comment.
If process.env.PROXY_HEALTH_UNHEALTHY_CACHE_TTL_MS is configured with an invalid non-numeric value or an empty string, parseInt will return NaN. This will cause Math.min(cacheTtlMs, UNHEALTHY_CACHE_TTL_MS) to evaluate to NaN, which in turn disables the negative cache entirely and triggers a new TCP probe on every request.
To prevent this, we should validate the parsed integer and fall back to the default value of 2000 if it is NaN or negative.
| const UNHEALTHY_CACHE_TTL_MS = parseInt( | |
| process.env.PROXY_HEALTH_UNHEALTHY_CACHE_TTL_MS ?? "2000", | |
| 10 | |
| ); | |
| const UNHEALTHY_CACHE_TTL_MS = (() => { | |
| const parsed = parseInt(process.env.PROXY_HEALTH_UNHEALTHY_CACHE_TTL_MS ?? "2000", 10); | |
| return isNaN(parsed) || parsed < 0 ? 2000 : parsed; | |
| })(); |
a7f192b to
cba1a81
Compare
|
Updated the branch after Fast Quality Gates caught the new env var contract. Added Additional local verification after the update:
|
|
Merge-ready from my side. The rerun after the env/docs sync is green: Fast Quality Gates, Unit Tests fast-path 1/2 + 2/2, Vitest, DAST smoke, semgrep, and semgrep-cloud-platform all pass. This addresses #5109 by shortening transient proxy fast-fail negative caching while keeping failed-proxy protection. |
|
#5255 is ready for maintainer merge after the fresh CI retrigger. Current status on head
The previous env-doc failure was stale relative to the branch tip; local |
Integrated into release/v3.8.40
Summary
Why
Issue #5109 reports high-concurrency load incorrectly marking working residential SOCKS5 proxies unreachable. The existing code coalesces concurrent probes, but a single transient failed probe was cached for the same 30s window as a success. This lets transient timeout/load blips recover quickly without removing fast-fail protection for truly dead proxies.
Verification
node --max-old-space-size=8192 --import tsx --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-force-exit tests/unit/t14-proxy-fast-fail.test.ts tests/unit/proxy-fetch.test.tsnpm run check:file-sizenpm run check:any-budget:t11Refs #5109