Repository navigation
fix(ModelSync): shared loopback readiness gate + IPv4 force - #2221
diegosouzapw merged 4 commits into
Conversation
…-process fallback (#14) * fix(ModelSync): retry self-fetch with network-error backoff before in-process fallback At container boot, 17 ModelSync events fire one-shot HTTP self-fetches against the in-process server before its HTTP listener is fully ready. The single-attempt contract produced a noisy 17-warning burst on every boot and contributed to the 19/h ProxyFetch undici-dispatcher-failed metric. Extracts the self-fetch into selfFetchWithRetry(url, opts) with: - maxRetries=5 with linear backoff (200ms * (attempt+1) by default) - Retry only on network-level failures (ECONNREFUSED, fetch failed) which indicate the loopback listener is not yet accepting connections - HTTP responses (2xx, 4xx, 5xx) returned as-is: server is up, propagate result - In-process route still used as last-resort fallback after all retries exhausted - Pure-function shape with injectable fetch + in-process callback for testing Tests cover: retry-succeeds-on-attempt-3, exhaust-then-fallback, HTTP-errors-returned-immediately. Updated model-sync-route.test.ts fallback test to expect 5 self-fetch retries before in-process fallback. Sanity check confirmed: maxRetries=1 prevents retry loop, causing attempt-count assertions in retry tests to fail as expected. * fix(ModelSync): add shared loopback readiness gate to eliminate retry amplification The prior commit reduced per-connection self-fetch failures via retry, but the 5-attempt-per-connection retry against a not-yet-ready loopback server amplified the ProxyFetch undici-failure metric from 17/boot to 85/boot (17 connections x 5 attempts each). This commit adds ensureLoopbackServerReady() -- a module-level shared promise that all selfFetchWithRetry callers await before firing. The actual HTTP readiness probe runs exactly once per process; the 17 boot-time connections all share the same promise, so the loopback HTTP server only sees one readiness probe sequence instead of 17 x 5 = 85 attempts. After readiness, each connection makes one HTTP self-fetch -- no retry amplification possible because the server is confirmed up. Readiness endpoint: /api/providers/__readiness_probe__/models (returns 404 once Next.js is dispatching; any HTTP status confirms readiness). Reduced default maxRetries from 5 to 3: the gate handles the boot race; retries are now only for transient failures after the server is confirmed up. Tests (8 total, all pass): - Existing retry tests updated with skipReadinessGate: true for isolation - ensureLoopbackServerReady: 17 callers share ONE probe sequence (<=15 attempts) - ensureLoopbackServerReady: rejects after maxWaitMs timeout - ensureLoopbackServerReady: resolves on 4xx (any HTTP status = server up) - selfFetchWithRetry with gate: 17 callers -> 1 probe sequence + 17 fetches - Sanity check: disabling gate -> >17 fetch attempts confirmed (amplification) Resolves Section F follow-up concerns from review. * fix(ModelSync): drop cache:no-store from loopback self-fetch — bypasses Next.js fetch override failure ProxyFetch falls back to Next.js-patched originalFetch when undici fails for loopback (127.0.0.1). Next.js-patched fetch fails with TypeError: fetch failed for cache:no-store requests made outside of a Next.js request-context (which is the case for the ProxyFetch native fallback path). Without cache:no-store the request is passed through to Node.js built-in fetch successfully. The readiness probe works (no cache directive) but the model fetches failed (had cache:no-store), explaining the 17 self-fetch warnings despite the gate. Remove cache:no-store from selfFetchWithRetry. The in-process fallback already handles any response caching concerns; the no-store directive was purely defensive and is not needed for loopback internal traffic. * fix(ModelSync): force IPv4 (127.0.0.1) for loopback self-fetch — avoid localhost->IPv6 resolution failure In Docker containers, "localhost" resolves to both ::1 (IPv6) and 127.0.0.1 (IPv4). Node.js may prefer ::1 for the connection attempt. The Next.js HTTP server binds to 0.0.0.0 (IPv4 only), so ::1 connections are refused with TypeError: fetch failed — even though the server is fully up and ::1 would ordinarily be fine on a host with IPv6 loopback. The readiness probe already hardcodes http://127.0.0.1:20128, so it succeeds. The model fetches used safeOrigin derived from the incoming request URL, which the scheduler sends as http://localhost:20128 — that resolves to ::1 first and fails. This caused all 17 connections to exhaust their 3 retries (51 ProxyFetch warnings) and fall back to in-process despite the readiness gate resolving. Fix: always construct safeOrigin as http://127.0.0.1:PORT, never http://localhost:PORT. --------- Co-authored-by: OmniRoute Ops <ops@nomenak.dev>
Two small polish items from Section F code review: 1. SelfFetchWithRetryOptions.maxRetries JSDoc said Default: 5 but the function body uses ?? 3 (changed when the readiness gate was added). Sync the JSDoc to match. 2. modelSyncScheduler constructs INTERNAL_BASE_URL as http://localhost:PORT. In Docker, localhost can resolve to ::1 (IPv6) before 127.0.0.1 (IPv4), and Next.js binds IPv4 only — would ECONNREFUSED. Defense-in-depth fix mirroring the route-level safeOrigin guard. Co-authored-by: OmniRoute Ops <ops@nomenak.dev>
There was a problem hiding this comment.
Code Review
This pull request introduces a shared loopback readiness gate to optimize boot-time self-fetches, preventing retry amplification by serializing readiness probes. It also updates selfFetchWithRetry to use this gate and enforces consistent use of IPv4 (127.0.0.1) for loopback connections. Feedback identified a hardcoded port in the readiness probe that should be dynamic, a discrepancy between the retry documentation and actual implementation, and a test case that fails to account for the updated default retry count.
| // readiness — we only care that the dispatcher succeeds (no | ||
| // ECONNREFUSED). Using a synthetic connection id so no real DB lookup | ||
| // is needed; the 404 is sufficient proof the server is dispatching. | ||
| const res = await f("http://127.0.0.1:20128/api/providers/__readiness_probe__/models", { |
There was a problem hiding this comment.
The port 20128 is hardcoded here, but the application port is configurable (e.g., via the PORT environment variable). If the server runs on a different port, this readiness probe will target the wrong address and likely time out. It should use the dynamic port, similar to the logic used in fetchProviderModelsForSync.
| const res = await f("http://127.0.0.1:20128/api/providers/__readiness_probe__/models", { | |
| const port = process.env.PORT || "20128"; | |
| const res = await f(`http://127.0.0.1:${port}/api/providers/__readiness_probe__/models`, { |
There was a problem hiding this comment.
Done in b60a2c1 — probe URL now uses process.env.OMNIROUTE_PORT || process.env.PORT || "20128", matching the pattern in src/lib/runtime/ports.ts and fetchProviderModelsForSync below. Thanks for catching that — we hardcoded 20128 in the initial draft and forgot to thread it through.
| * - HTTP 5xx: treated as transient — retry. | ||
| * - HTTP 4xx: treated as definitive — skip remaining retries and fall back. |
There was a problem hiding this comment.
There is a discrepancy between this docstring and the actual implementation. The docstring states that HTTP 5xx responses should be retried and 4xx should "fall back", but the code in selfFetchWithRetry (lines 286-290) and the unit tests indicate that any HTTP response returns immediately. Please align the documentation with the intended behavior.
There was a problem hiding this comment.
Done in b60a2c1 — synced the docstring to match the implementation. You were right that the prior text ("5xx retry, 4xx fall back") didn't reflect the code, which returns ANY HTTP response (2xx/4xx/5xx) as-is and only retries on network-level failures. The retry-contract block now says exactly that.
| // selfFetchWithRetry default maxRetries=5: all 5 attempts throw, then in-process | ||
| // fallback fires (which triggers the upstream bltcy.ai fetch). So fetchCalls | ||
| // contains 5 self-fetch URLs followed by 1 upstream URL. | ||
| const selfFetchUrl = `http://localhost/api/providers/${connection.id}/models?refresh=true`; | ||
| assert.equal( | ||
| fetchCalls.slice(0, 5).every((u) => u === selfFetchUrl), | ||
| true, | ||
| "first 5 calls should be self-fetch retries" | ||
| ); | ||
| assert.equal(fetchCalls[5], "https://api.bltcy.ai/v1/models", "6th call should be upstream"); | ||
| assert.equal(fetchCalls.length, 6, "should have exactly 5 retries + 1 upstream call"); |
There was a problem hiding this comment.
The default maxRetries was changed from 5 to 3 in the implementation, but this test still expects 5 retries (total 6 calls). This will cause the test to fail. The assertions and comments should be updated to reflect the new default of 3 attempts (total 4 calls).
// selfFetchWithRetry default maxRetries=3: all 3 attempts throw, then in-process
// fallback fires (which triggers the upstream bltcy.ai fetch). So fetchCalls
// contains 3 self-fetch URLs followed by 1 upstream URL.
const selfFetchUrl = `http://localhost/api/providers/${connection.id}/models?refresh=true`;
assert.equal(
fetchCalls.slice(0, 3).every((u) => u === selfFetchUrl),
true,
"first 3 calls should be self-fetch retries"
);
assert.equal(fetchCalls[3], "https://api.bltcy.ai/v1/models", "4th call should be upstream");
assert.equal(fetchCalls.length, 4, "should have exactly 3 retries + 1 upstream call");There was a problem hiding this comment.
Done in b60a2c1. The test required three coordinated fixes: (1) retry count 3 not 5, (2) URL expectation updated to http://127.0.0.1:${PORT}/... since the route now forces IPv4 origin (not http://localhost), and (3) the mock fetch now handles the readiness-probe URL and we call __resetLoopbackReadinessForTests() so the gate state doesn't leak between tests. Verified locally — the test passes in ~720ms now.
- Readiness probe URL now uses OMNIROUTE_PORT / PORT env (was hardcoded 20128). Matches the pattern already in src/lib/runtime/ports.ts and fetchProviderModelsForSync below. - Retry-contract docstring synced with implementation: any HTTP response (2xx/4xx/5xx) is returned as-is — only network errors retry. The prior \"5xx retry / 4xx fall back\" text was aspirational, not what the code does. - \"falls back to in-process discovery\" test: reset readiness gate + handle probe URL in the mock fetch + update expected retry count (3, not 5) + expected URL (http://127.0.0.1:PORT, not http://localhost) to match the IPv4 origin the route now constructs.
253f5e5
into
cheaper-inference:release/v3.8.0
|
Thanks @NomenAK! Your contribution has been integrated into The PR branch was synced with the latest Reviewed and merged via the |
After merging PRs #2221 (ModelSync shared loopback readiness gate + IPv4 force) and #2219 (Antigravity loadCodeAssist bootstrap + fetchAvailableModels fallback) into release/v3.8.0, two test suites needed updates to match the new routing: - tests/unit/model-sync-route.test.ts: * resetStorage() now calls __resetLoopbackReadinessForTests() so the module-level __loopbackReadyPromise cache does not leak between tests. * Every fetch mock now answers the /__readiness_probe__/ URL with 404 so the gate opens immediately (any HTTP response satisfies the probe). * Self-fetch target URL assertions updated from http://localhost/... to http://127.0.0.1:20128/... per PR #2221's IPv4-force. - tests/unit/provider-models-route.test.ts: * The Antigravity discovery-retry test now treats loadCodeAssist calls as non-fatal failures so the discovery path is still exercised. * The expected discovery URL sequence is updated to the new fetchAvailableModels-first order introduced by PR #2219.
Deep audit of all 320 commits since v3.7.9 found: - 18 merged PRs not documented in CHANGELOG (4 features, 10 bug fixes, 1 security, 2 chores, 1 debug improvement) - 3 contributors entirely missing from credits table (@NomenAK with 12 PRs, @kang-heewon, @one-vs) - 4 existing contributors with inaccurate PR counts (@oyi77 8→12, @ddarkr 2→3, @andrewmunsell 2→3, @nickwizard 2→3) New entries added: - feat: #2135 (1proxy settings), #2227 (antigravity project ID), #2238 (Z.AI Search), #2240 (CLI Suite) - fix: #2217, #2218, #2219, #2221, #2222, #2223, #2224, #2231, #2233, #2236, #2242, #2243 - security: #2209 (stack trace exposure) - chore: #2228, #2234 Total contributors updated from 50+ to 55+.
…inference#2221) Integrated into release/v3.8.0
After merging PRs cheaper-inference#2221 (ModelSync shared loopback readiness gate + IPv4 force) and cheaper-inference#2219 (Antigravity loadCodeAssist bootstrap + fetchAvailableModels fallback) into release/v3.8.0, two test suites needed updates to match the new routing: - tests/unit/model-sync-route.test.ts: * resetStorage() now calls __resetLoopbackReadinessForTests() so the module-level __loopbackReadyPromise cache does not leak between tests. * Every fetch mock now answers the /__readiness_probe__/ URL with 404 so the gate opens immediately (any HTTP response satisfies the probe). * Self-fetch target URL assertions updated from http://localhost/... to http://127.0.0.1:20128/... per PR cheaper-inference#2221's IPv4-force. - tests/unit/provider-models-route.test.ts: * The Antigravity discovery-retry test now treats loadCodeAssist calls as non-fatal failures so the discovery path is still exercised. * The expected discovery URL sequence is updated to the new fetchAvailableModels-first order introduced by PR cheaper-inference#2219.
Deep audit of all 320 commits since v3.7.9 found: - 18 merged PRs not documented in CHANGELOG (4 features, 10 bug fixes, 1 security, 2 chores, 1 debug improvement) - 3 contributors entirely missing from credits table (@NomenAK with 12 PRs, @kang-heewon, @one-vs) - 4 existing contributors with inaccurate PR counts (@oyi77 8→12, @ddarkr 2→3, @andrewmunsell 2→3, @nickwizard 2→3) New entries added: - feat: cheaper-inference#2135 (1proxy settings), cheaper-inference#2227 (antigravity project ID), cheaper-inference#2238 (Z.AI Search), cheaper-inference#2240 (CLI Suite) - fix: cheaper-inference#2217, cheaper-inference#2218, cheaper-inference#2219, cheaper-inference#2221, cheaper-inference#2222, cheaper-inference#2223, cheaper-inference#2224, cheaper-inference#2231, cheaper-inference#2233, cheaper-inference#2236, cheaper-inference#2242, cheaper-inference#2243 - security: cheaper-inference#2209 (stack trace exposure) - chore: cheaper-inference#2228, cheaper-inference#2234 Total contributors updated from 50+ to 55+.
…inference#2221) Integrated into release/v3.8.0
After merging PRs cheaper-inference#2221 (ModelSync shared loopback readiness gate + IPv4 force) and cheaper-inference#2219 (Antigravity loadCodeAssist bootstrap + fetchAvailableModels fallback) into release/v3.8.0, two test suites needed updates to match the new routing: - tests/unit/model-sync-route.test.ts: * resetStorage() now calls __resetLoopbackReadinessForTests() so the module-level __loopbackReadyPromise cache does not leak between tests. * Every fetch mock now answers the /__readiness_probe__/ URL with 404 so the gate opens immediately (any HTTP response satisfies the probe). * Self-fetch target URL assertions updated from http://localhost/... to http://127.0.0.1:20128/... per PR cheaper-inference#2221's IPv4-force. - tests/unit/provider-models-route.test.ts: * The Antigravity discovery-retry test now treats loadCodeAssist calls as non-fatal failures so the discovery path is still exercised. * The expected discovery URL sequence is updated to the new fetchAvailableModels-first order introduced by PR cheaper-inference#2219.
Deep audit of all 320 commits since v3.7.9 found: - 18 merged PRs not documented in CHANGELOG (4 features, 10 bug fixes, 1 security, 2 chores, 1 debug improvement) - 3 contributors entirely missing from credits table (@NomenAK with 12 PRs, @kang-heewon, @one-vs) - 4 existing contributors with inaccurate PR counts (@oyi77 8→12, @ddarkr 2→3, @andrewmunsell 2→3, @nickwizard 2→3) New entries added: - feat: cheaper-inference#2135 (1proxy settings), cheaper-inference#2227 (antigravity project ID), cheaper-inference#2238 (Z.AI Search), cheaper-inference#2240 (CLI Suite) - fix: cheaper-inference#2217, cheaper-inference#2218, cheaper-inference#2219, cheaper-inference#2221, cheaper-inference#2222, cheaper-inference#2223, cheaper-inference#2224, cheaper-inference#2231, cheaper-inference#2233, cheaper-inference#2236, cheaper-inference#2242, cheaper-inference#2243 - security: cheaper-inference#2209 (stack trace exposure) - chore: cheaper-inference#2228, cheaper-inference#2234 Total contributors updated from 50+ to 55+.
…inference#2221) Integrated into release/v3.8.0
After merging PRs cheaper-inference#2221 (ModelSync shared loopback readiness gate + IPv4 force) and cheaper-inference#2219 (Antigravity loadCodeAssist bootstrap + fetchAvailableModels fallback) into release/v3.8.0, two test suites needed updates to match the new routing: - tests/unit/model-sync-route.test.ts: * resetStorage() now calls __resetLoopbackReadinessForTests() so the module-level __loopbackReadyPromise cache does not leak between tests. * Every fetch mock now answers the /__readiness_probe__/ URL with 404 so the gate opens immediately (any HTTP response satisfies the probe). * Self-fetch target URL assertions updated from http://localhost/... to http://127.0.0.1:20128/... per PR cheaper-inference#2221's IPv4-force. - tests/unit/provider-models-route.test.ts: * The Antigravity discovery-retry test now treats loadCodeAssist calls as non-fatal failures so the discovery path is still exercised. * The expected discovery URL sequence is updated to the new fetchAvailableModels-first order introduced by PR cheaper-inference#2219.
Deep audit of all 320 commits since v3.7.9 found: - 18 merged PRs not documented in CHANGELOG (4 features, 10 bug fixes, 1 security, 2 chores, 1 debug improvement) - 3 contributors entirely missing from credits table (@NomenAK with 12 PRs, @kang-heewon, @one-vs) - 4 existing contributors with inaccurate PR counts (@oyi77 8→12, @ddarkr 2→3, @andrewmunsell 2→3, @nickwizard 2→3) New entries added: - feat: cheaper-inference#2135 (1proxy settings), cheaper-inference#2227 (antigravity project ID), cheaper-inference#2238 (Z.AI Search), cheaper-inference#2240 (CLI Suite) - fix: cheaper-inference#2217, cheaper-inference#2218, cheaper-inference#2219, cheaper-inference#2221, cheaper-inference#2222, cheaper-inference#2223, cheaper-inference#2224, cheaper-inference#2231, cheaper-inference#2233, cheaper-inference#2236, cheaper-inference#2242, cheaper-inference#2243 - security: cheaper-inference#2209 (stack trace exposure) - chore: cheaper-inference#2228, cheaper-inference#2234 Total contributors updated from 50+ to 55+.
What
Add a shared loopback-server readiness gate to
selfFetchModelsto eliminate 17× boot-time fetch failure bursts, plus force IPv4 for the scheduler's outbound URL.Why
At container boot, ModelSync fires concurrent HTTP self-fetches against
http://localhost:20128for every provider connection (17 in our deployment) before the in-process HTTP server is fully accepting connections. The single-attempt contract produced a noisy 17-warning burst on every boot.Two compounding root causes:
localhostresolves to::1(IPv6) in our Docker network, but Next.js binds IPv4 only — even after retry, all self-fetchesECONNREFUSED.How
Commit 1 (readiness gate):
ensureLoopbackServerReady(opts)— module-level shared promise. The first caller polls until any HTTP response (any status ≥ 200, including 404 = server alive) succeeds; all subsequent callersawaitthe same promise.selfFetchModels(connectionId, opts)nowawaitsensureLoopbackServerReady()before firing the model fetch. Exactly 1 probe runs per process regardless of caller count.__resetLoopbackReadinessForTests()andskipReadinessGate?: booleanoption for unit isolation.Commit 2 (IPv4 + polish):
modelSyncScheduler.ts:25:http://localhost:${dashboardPort}→http://127.0.0.1:${dashboardPort}— defense in depth mirroring the route-levelsafeOriginguard.SelfFetchWithRetryOptions.maxRetries: "Default: 5" → "Default: 3" (synced with the actual default after readiness gate was added).8 new tests in
tests/unit/api/sync-models-readiness.test.ts: concurrent-callers share one probe, fallback after maxRetries, HTTP 4xx not retried (definitive), HTTP 5xx not retried, gate timeout, etc.Notes
The readiness probe targets
/api/providers/__readiness_probe__/modelsbecause any HTTP status confirms the server is dispatching (even 404). Happy to revise the probe endpoint if you have a/healthor similar.