fix(api): keep catalog builds responsive and hash cache keys (#9147, #10313) - #10538
Merged
Merged
Conversation
… never live in the key string (#10313)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Owner
Author
|
This PR remains deferred pending current-head and ownership revalidation. The prior review recorded an ownership hold, but this round did not confirm that worktree as active. |
diegosouzapw
added
deferred-v3.8.50
…0313-catalog-cache # Conflicts: # src/app/api/v1/models/catalog.ts # src/app/api/v1/models/catalogCache.ts
…boundary Post-sync-merge fixup for #9147/#10313 against release/v3.8.50: - Resolve the catalog.ts/catalogCache.ts merge conflicts against several catalog PRs merged since this branch was cut: keep isModelHiddenBulk() (this PR's perf fix) alongside isExcludedByProviderConnections() (a concurrently landed feature), and adopt the already-merged canonical fingerprintCatalogAuthKey() helper for the cache-key hashing instead of the now-duplicate inline sha256 computation. - getHiddenModelsByProvider() was hoisted above buildUnifiedModelsResponseCore's try/catch, so a read failure there rejected the builder promise instead of being caught and turned into a sanitized 500 like every other failure in this function. Combined with the pre-existing promise.finally() dangling chain in catalogCache.ts's in-flight coalescing, that produced a genuine unhandled rejection. Move the bulk-load call back inside the try block. - Align tests/unit/models-catalog-route.test.ts and tests/unit/10313-catalog-cache-key-hashing.test.ts with the current implementation (bulk query text/method, truncated fingerprint format).
Combining this PR's own bulk hidden-model optimization with the already-merged isExcludedByProviderConnections() check (from a different PR) reintroduced an O(connections) scan per model inside the catalog builder's hot loop, regressing the exact single-stretch event-loop budget tests/unit/9147-catalog-eventloop-yield.test.ts enforces (was passing on this PR's own commit before the merge). Memoizing getConnectionsForProvider() by its (unordered) key-set substantially reduces the redundant per-model connection scans (measured ~497ms -> ~210-300ms worst single stretch across repeated runs), but does NOT fully close the gap to the 150ms budget — still red. Committing this as a real, safe improvement; flagging for further investigation (likely getConnectionsForProvider's first-call cost per provider, or hasEligibleConnectionForModel) before this PR merges. NOT deciding to relax the test threshold myself.
giauphan
pushed a commit
to giauphan/OmniRoute
that referenced
this pull request
Aug 20, 2026
…uzapw#9147, diegosouzapw#10313) (diegosouzapw#10538) * fix(catalog): hash API key in buildCatalogCacheKey so raw credentials never live in the key string (diegosouzapw#10313) * fix(api): yield event loop and bulk-load override tables in catalog build (diegosouzapw#9147) * fix(api): keep bulk hidden-model load inside catalog builder's error boundary Post-sync-merge fixup for diegosouzapw#9147/diegosouzapw#10313 against release/v3.8.50: - Resolve the catalog.ts/catalogCache.ts merge conflicts against several catalog PRs merged since this branch was cut: keep isModelHiddenBulk() (this PR's perf fix) alongside isExcludedByProviderConnections() (a concurrently landed feature), and adopt the already-merged canonical fingerprintCatalogAuthKey() helper for the cache-key hashing instead of the now-duplicate inline sha256 computation. - getHiddenModelsByProvider() was hoisted above buildUnifiedModelsResponseCore's try/catch, so a read failure there rejected the builder promise instead of being caught and turned into a sanitized 500 like every other failure in this function. Combined with the pre-existing promise.finally() dangling chain in catalogCache.ts's in-flight coalescing, that produced a genuine unhandled rejection. Move the bulk-load call back inside the try block. - Align tests/unit/models-catalog-route.test.ts and tests/unit/10313-catalog-cache-key-hashing.test.ts with the current implementation (bulk query text/method, truncated fingerprint format). * perf(api): memoize getConnectionsForProvider in catalog builder Combining this PR's own bulk hidden-model optimization with the already-merged isExcludedByProviderConnections() check (from a different PR) reintroduced an O(connections) scan per model inside the catalog builder's hot loop, regressing the exact single-stretch event-loop budget tests/unit/9147-catalog-eventloop-yield.test.ts enforces (was passing on this PR's own commit before the merge). Memoizing getConnectionsForProvider() by its (unordered) key-set substantially reduces the redundant per-model connection scans (measured ~497ms -> ~210-300ms worst single stretch across repeated runs), but does NOT fully close the gap to the 150ms budget — still red. Committing this as a real, safe improvement; flagging for further investigation (likely getConnectionsForProvider's first-call cost per provider, or hasEligibleConnectionForModel) before this PR merges. NOT deciding to relax the test threshold myself. --------- Co-authored-by: adevwithpurpose <adevwithpurpose@users.noreply.github.com>
muhamadgalihsaputra
pushed a commit
to niyatna/NiyatnaRoute
that referenced
this pull request
Sep 27, 2026
…uzapw#9147, diegosouzapw#10313) (diegosouzapw#10538) * fix(catalog): hash API key in buildCatalogCacheKey so raw credentials never live in the key string (diegosouzapw#10313) * fix(api): yield event loop and bulk-load override tables in catalog build (diegosouzapw#9147) * fix(api): keep bulk hidden-model load inside catalog builder's error boundary Post-sync-merge fixup for diegosouzapw#9147/diegosouzapw#10313 against release/v3.8.50: - Resolve the catalog.ts/catalogCache.ts merge conflicts against several catalog PRs merged since this branch was cut: keep isModelHiddenBulk() (this PR's perf fix) alongside isExcludedByProviderConnections() (a concurrently landed feature), and adopt the already-merged canonical fingerprintCatalogAuthKey() helper for the cache-key hashing instead of the now-duplicate inline sha256 computation. - getHiddenModelsByProvider() was hoisted above buildUnifiedModelsResponseCore's try/catch, so a read failure there rejected the builder promise instead of being caught and turned into a sanitized 500 like every other failure in this function. Combined with the pre-existing promise.finally() dangling chain in catalogCache.ts's in-flight coalescing, that produced a genuine unhandled rejection. Move the bulk-load call back inside the try block. - Align tests/unit/models-catalog-route.test.ts and tests/unit/10313-catalog-cache-key-hashing.test.ts with the current implementation (bulk query text/method, truncated fingerprint format). * perf(api): memoize getConnectionsForProvider in catalog builder Combining this PR's own bulk hidden-model optimization with the already-merged isExcludedByProviderConnections() check (from a different PR) reintroduced an O(connections) scan per model inside the catalog builder's hot loop, regressing the exact single-stretch event-loop budget tests/unit/9147-catalog-eventloop-yield.test.ts enforces (was passing on this PR's own commit before the merge). Memoizing getConnectionsForProvider() by its (unordered) key-set substantially reduces the redundant per-model connection scans (measured ~497ms -> ~210-300ms worst single stretch across repeated runs), but does NOT fully close the gap to the 150ms budget — still red. Committing this as a real, safe improvement; flagging for further investigation (likely getConnectionsForProvider's first-call cost per provider, or hasEligibleConnectionForModel) before this PR merges. NOT deciding to relax the test threshold myself. --------- Co-authored-by: adevwithpurpose <adevwithpurpose@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cluster fix: catalog build at scale (#9147) + cache-key secret leak (#10313)
Two independent catalog defects on the same
/v1/modelspath, fixed togetherbecause they touch overlapping code in the build/enrichment tail.
#9147 — catalog build pins the single Node.js event loop at scale
Reprodução (Hard Rule #18, TDD): novo teste
tests/unit/9147-catalog-eventloop-yield.test.tsseed 60 conexões / 720 synced models e mede o maior gap entre ticks de
setTimeout(0)num build frio do
/v1/models. Antes da correção o build prendia o event loop(multi-segundos de bloqueio contínuo) em prejuízo do dashboard LiveWS heartbeat;
depois, todo o build cede ao event loop a cada fração.
Causa-raiz: o builder
getUnifiedModelsResponse → buildUnifiedModelsResponseCore(
src/app/api/v1/models/catalog.ts) e o tail de enrichment(
src/app/api/v1/models/catalogResponse.ts) rodavam como um único turno síncronono App Router single-thread: sem
await/yield em nenhum dos loops do walk deconexões + registries de modelos + combos + aliases + synced/custom, e com N+1
queries SQLite por entry (
getModelIsHidden,getModelCapabilityOverride,getResolvedModelContextOverride).Correção:
setImmediate) em todos os loops quentes do builder(combos, static, codex, synced, openrouter, custom, fallback) e no enrichment
map + post-filter passes — o event loop respira mesmo em catálogos de milhares
de entries.
(
getHiddenModelsByProviderno catalog.ts; snapshotcreateModelCapabilityResolutionSnapshotno enrichment, [v3.8.50] fix(models): keep model catalogs responsive #9199) eliminando as~N queries SQLite síncronas por entry.
applyCatalogPostFiltersagora async, com yields entre os passes variantes.#10313 — raw API key no Map key do catalog cache
Reprodução (TDD):
tests/unit/10313-catalog-cache-key-hashing.test.tscapturatodas as chaves de
Map.prototype.setdurante um build e assere que (a) nenhumachave contém a bearer secret bruta, (b) a chave embute o digest sha256 da key,
(c) keys iguais particionam/colidem corretamente.
Causa-raiz:
buildCatalogCacheKey()usava oapiKeybruto literal no key doMapde cache — o segredo vazava para um heap dump / sessão--inspect/log de debug. Correção: hash sha256 da key (idioma estabelecido
hashKeydoapiKeys.ts; não é password hash, key é token de alta entropia).Validação
npm run test:vitest: 362/362 offline.npm run typecheck:core: limpo.npm run lint: limpo nos arquivos alterados.npm run check:cycles: sem ciclos novos.catalog-order,hide-auto-no-think,synced-static,opencode-zen,functional-gateway,pricing-*): verdes.npm run test:unit: executado sob carga concorrente do devbox; terminou com 31.091passes, 24 falhas e 22 skips. O run incluiu falhas preexistentes/ambientais em múltiplas
áreas e o teste de timing fix(providers): Overload occurs when there are too many connections/models. #9147 foi sensível à saturação; o PR não deve ser considerado
merge-ready até a confirmação isolada em ambiente sem essa concorrência.