fix(knowledge): stop one env knob from setting the embedding request fan-out - #6852
Conversation
…fan-out KB_CONFIG_CONCURRENCY_LIMIT was read in three places with three meanings: the document-processing queue depth, the number of embedding requests issued concurrently inside a single embed call, and (divided by five) the in-process document concurrency. The first two multiply — every admitted task run reaches the embed path and opens its own fan-out — so the default put roughly a thousand requests in flight against one provider key. A rate limit is per key, so the pipeline held itself at the limit, and no retry policy can absorb a load its own concurrency is generating. Each variable is now read by exactly one consumer, which also removes the drift that hid this: the same variable was read with a different inline fallback in each place, and since createEnv runs with skipValidation the declared defaults never execute, so the fallbacks were the real ones and disagreed. The divisors are gone and the previous effective values are the declared defaults, so only the embedding fan-out changes: 50 to 8. KB_CONFIG_BATCH_SIZE had the same conflation between chunks-per-embedding-request and documents-per-batch, and is split the same way. Rate-limit rejections also discarded what the provider said about when to come back. The response headers were dropped when building EmbeddingAPIError, so the retry loop's support for a server-stated wait was dead code on this path and every attempt fired blind, exhausting the budget inside a window that had not reopened. The headers now travel with the error the way fetchWithRetry already does for connectors, and the wait is read from Retry-After or, failing that, the reset header for whichever limit dimension is actually exhausted. Those carry a Go duration rather than the epoch seconds the shared connector helper expects, so the reading lives with the provider instead of changing retry behaviour for every connector. The retry budget is sized against a rate-limit window rather than a blip, since a 10s ceiling clamped every stated wait below the reopen time.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Embedding retries now read provider rate-limit signals from failed responses (
Reviewed by Cursor Bugbot for commit a4ae48f. Configure here. |
Greptile SummaryThe PR separates document-processing concurrency controls from embedding-request fan-out and adds provider-aware rate-limit retry timing.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/core/config/env.ts | Declares independent defaults for task queue depth, embedding fan-out, and in-process document limits. |
| apps/sim/lib/embeddings/client.ts | Uses the dedicated embedding concurrency setting and propagates provider retry timing into the embedding retry and fallback flow. |
| apps/sim/lib/embeddings/rate-limit.ts | Adds strict parsing and selection of Retry-After and OpenAI rate-limit reset headers. |
| apps/sim/lib/knowledge/documents/service.ts | Replaces divisor-derived document limits with dedicated positive configuration values. |
| apps/sim/lib/embeddings/client.test.ts | Covers retry-header propagation, retry-budget behavior, recovery, and fallback attempt counts. |
| apps/sim/lib/embeddings/rate-limit.test.ts | Covers strict Go-duration parsing and selection of the exhausted rate-limit dimension. |
Reviews (3): Last reviewed commit: "fix(knowledge): measure a stated wait ag..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Honoring the provider's stated wait introduced a case the retry budget could not serve. When a provider states a reset longer than the ceiling, the loop clamps every attempt to that ceiling, so the whole budget is spent inside a window that has not reopened — and with five attempts at thirty seconds that delayed the fallback provider by around two and a half minutes, where the previous blind backoff reached it in about seven seconds. A stated wait past the ceiling now refuses the retry outright. The error still classifies as transient, and the fallback chain classifies separately through shouldFallback, so the next provider is reached immediately instead of after the budget burns down. Retrying was never going to succeed in that window, so nothing is given up.
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 2b357ff. Configure here.
Refusing to retry once the stated wait passed the per-attempt ceiling was too blunt. Each wait is clamped individually but the attempts accumulate, so a window a little longer than one clamped delay still reopens partway through the budget: a 35s wait is reachable on the second attempt. Rejecting those stranded a caller with no fallback provider, which would have recovered by waiting. The comparison is now against the budget the attempts span in total. A window inside it is retried and can recover; only one that outlasts every attempt is unreachable, and that still fails fast so the fallback chain is reached at once rather than after the budget burns down.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a4ae48f. Configure here.
1 participant
Problem
KB_CONFIG_CONCURRENCY_LIMITwas read in three places with three different meanings:background/knowledge-processing.tslib/embeddings/client.tslib/knowledge/documents/service.ts/5→ concurrent documents in the in-process pathThe first two multiply. Every admitted task run reaches the embed path and opens its own fan-out, so the default put on the order of 1,000 embedding requests in flight against a single provider key. A provider's rate limit is per key, so the pipeline held itself at its own limit — and no retry policy can absorb a load its own concurrency is generating.
Two things hid this:
createEnvruns withskipValidation: true, and t3-env returnsruntimeEnvuntouched in that mode, so the.default(...)declarations never execute — the inline fallbacks are the real defaults, and they disagreed with each other and with the declaration.KB_CONFIG_BATCH_SIZEhad the same conflation, between chunks-per-embedding-request (2000) and documents-per-batch (/2→ 10).Separately, rate-limit rejections discarded what the provider said about when to retry.
EmbeddingAPIErrorwas built from status text only and the response headers were dropped, soretryWithExponentialBackoff's support for a server-stated wait was dead code on this path. Every attempt fired blind: 3 retries capped at 10s, ~7s of total backoff, all of it spent inside a window that had not reopened.Change
One consumer per variable, so the fallbacks cannot drift:
KB_CONFIG_CONCURRENCY_LIMIT— task-queue depth onlyKB_CONFIG_EMBEDDING_CONCURRENCY(new) — embedding fan-out, 50 → 8KB_CONFIG_DOCUMENT_CONCURRENCY(new) — in-process document concurrency, default 4KB_CONFIG_BATCH_SIZE— chunks per embedding request onlyKB_CONFIG_DOCUMENT_BATCH_SIZE(new) — documents per batch, default 10The divisors are gone and the previous effective values are the declared defaults, so when these variables are unset the only behaviour change is the embedding fan-out.
If a deployment sets
KB_CONFIG_CONCURRENCY_LIMITorKB_CONFIG_BATCH_SIZEexplicitly, more moves — that is the whole point of the split, but it is worth stating exactly what:KB_CONFIG_CONCURRENCY_LIMIT=50KB_CONFIG_BATCH_SIZE=2000Each of those collateral values was set by a divisor rather than by intent, so the new numbers are the intended ones — but they would be changes, not no-ops.
Checked against every env source, and none of these variables are set anywhere:
KB_CONFIG_*settrigger.config.tssyncs onlyDB_APP_NAME, so the worker env is the dashboard's and had to be checked separately from Secrets Manager. Since every variable is unset, the inline fallbacks are what run today, and for this deployment the sole behavioural change is the embedding fan-out, 50 → 8.Honor the provider's stated wait. The headers now travel with the error the way
fetchWithRetryalready does for connectors, via the existingattachRetryHeaders(non-enumerable, so the bag stays out of log lines). The wait is read fromRetry-After, falling back to the reset header for whichever limit dimension is actually exhausted — a tokens-per-minute rejection and a requests-per-minute rejection reopen at different times, and a reset read while quota remains says nothing about this request.Those reset headers carry a Go duration (
6m0s,12ms,23h47m36.648s), not the epoch seconds the shared connector helper reads. Teaching the shared reader both spellings would change retry behaviour for every connector onfetchWithRetry, so the provider-specific reading lives in a newlib/embeddings/rate-limit.tsinstead. Notemvsms: a loosely-scanning parser reads12msas twelve minutes, so the parser matches sticky and rejects anything it cannot fully consume. There is a test for exactly that.The retry budget is sized against a rate-limit window rather than a blip (5 retries, 30s ceiling) — the previous 10s ceiling clamped every stated wait to below the reopen time, spending the whole budget for nothing. It stays well inside
KB_CONFIG_MAX_DURATION, and batches wait concurrently rather than in sequence.Verification
ms/mtest genuinely catches itNot in this PR
The stuck-document sweep re-queues every
pending/faileddoc in its 7-day window on every sync, with no attempt counter and no cooldown (there is no retry-count column). Under sustained failure that re-attempts the whole backlog each cycle. Fixing it properly needs a schema migration, which does not belong in a config and retry change — filed as follow-up.