Skip to content

Commit 1ffdc37

Browse files
committed
fix(knowledge): stop one env knob from setting the embedding request 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.
1 parent cb6c842 commit 1ffdc37

6 files changed

Lines changed: 314 additions & 16 deletions

File tree

apps/sim/lib/core/config/env.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -390,8 +390,11 @@ export const env = createEnv({
390390
KB_CONFIG_RETRY_FACTOR: z.number().optional().default(2), // Retry backoff factor
391391
KB_CONFIG_MIN_TIMEOUT: z.number().optional().default(1000), // Min timeout in ms
392392
KB_CONFIG_MAX_TIMEOUT: z.number().optional().default(10000), // Max timeout in ms
393-
KB_CONFIG_CONCURRENCY_LIMIT: z.number().optional().default(50), // Concurrent embedding API calls
393+
KB_CONFIG_CONCURRENCY_LIMIT: z.number().optional().default(20), // Concurrent document-processing task runs (Trigger.dev queue depth)
394+
KB_CONFIG_EMBEDDING_CONCURRENCY: z.number().optional().default(8), // Concurrent embedding API requests within one embed call
395+
KB_CONFIG_DOCUMENT_CONCURRENCY: z.number().optional().default(4), // Concurrent documents in the in-process (non-Trigger) path
394396
KB_CONFIG_BATCH_SIZE: z.number().optional().default(2000), // Chunks to process per embedding batch
397+
KB_CONFIG_DOCUMENT_BATCH_SIZE: z.number().optional().default(10), // Documents per batch in the in-process (non-Trigger) path
395398
KB_CONFIG_DELAY_BETWEEN_BATCHES: z.number().optional().default(0), // Delay between batches in ms (0 for max speed)
396399
KB_CONFIG_DELAY_BETWEEN_DOCUMENTS: z.number().optional().default(50), // Delay between documents in ms
397400
KB_CONFIG_CHUNK_CONCURRENCY: z.number().optional().default(10), // Concurrent PDF chunk OCR processing
@@ -727,6 +730,13 @@ export { getEnv }
727730
* `z.number()` arrive as raw strings when sourced from `process.env` or Helm.
728731
* Use this helper anywhere a numeric env override is consumed to normalize the
729732
* type at the boundary instead of relying on JS implicit coercion.
733+
*
734+
* Skipping validation also means the schema never runs, so a `.default(...)` in
735+
* the declaration above never executes: **the fallback passed here is the real
736+
* default**, and the declared one is documentation. Keep the two in agreement —
737+
* a variable read in more than one place with a different fallback each time has
738+
* no single default at all, which is how one knob came to set both the
739+
* document-processing queue depth and the embedding request fan-out.
730740
*/
731741
export function envNumber(
732742
value: number | string | undefined | null,

apps/sim/lib/embeddings/client.test.ts

Lines changed: 38 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import { resetEnvMock, setEnv } from '@sim/testing'
55
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
66
import {
7+
EMBEDDING_MAX_RETRIES,
78
EmbeddingAPIError,
89
embed,
910
embedKnowledgeForDeployment,
@@ -28,11 +29,13 @@ vi.mock('@/lib/api-key/byok', () => ({
2829

2930
const originalFetch = global.fetch
3031

31-
function jsonResponse(body: unknown, status = 200): Response {
32+
function jsonResponse(body: unknown, status = 200, responseHeaders?: HeadersInit): Response {
3233
return {
3334
ok: status >= 200 && status < 300,
3435
status,
3536
statusText: String(status),
37+
// A real Response always carries these; the failure path reads them for rate-limit signals.
38+
headers: new Headers(responseHeaders),
3639
json: async () => body,
3740
text: async () => JSON.stringify(body),
3841
} as Response
@@ -677,11 +680,12 @@ describe('knowledge embedding transport fallback', () => {
677680
await vi.runAllTimersAsync()
678681
const result = await pending
679682

680-
expect(fetchMock).toHaveBeenCalledTimes(5)
681-
expect(fetchMock.mock.calls.slice(0, 4).every(([url]) => url.includes('api.openai.com'))).toBe(
682-
true
683-
)
684-
expect(fetchMock.mock.calls[4][0]).toBe('https://openrouter.ai/api/v1/embeddings')
683+
const attempts = EMBEDDING_MAX_RETRIES + 1
684+
expect(fetchMock).toHaveBeenCalledTimes(attempts + 1)
685+
expect(
686+
fetchMock.mock.calls.slice(0, attempts).every(([url]) => url.includes('api.openai.com'))
687+
).toBe(true)
688+
expect(fetchMock.mock.calls[attempts][0]).toBe('https://openrouter.ai/api/v1/embeddings')
685689
expect(projectInputs).toHaveBeenCalledOnce()
686690
expect(result.embeddings).toEqual([[7, 8]])
687691
})
@@ -713,13 +717,40 @@ describe('knowledge embedding transport fallback', () => {
713717
.filter(([url]) => url === 'https://openrouter.ai/api/v1/embeddings')
714718
.flatMap(([, init]) => JSON.parse((init as RequestInit).body as string).input as string[])
715719
expect(openRouterInputs).toEqual([secondInput])
716-
expect(fetchMock).toHaveBeenCalledTimes(6)
720+
// The succeeding batch, every attempt on the failing one, then its fallback.
721+
expect(fetchMock).toHaveBeenCalledTimes(1 + (EMBEDDING_MAX_RETRIES + 1) + 1)
717722
expect(result.embeddings).toEqual([[1], [2]])
718723
expect(result.totalTokens).toBe(6)
719724
expect(result.billableTokens).toBe(3)
720725
expect(result.isBYOK).toBe(false)
721726
})
722727

728+
/**
729+
* The retry loop replaces its own backoff with a provider-stated wait, but only
730+
* if the wait reaches it. Nothing downstream of the transport could see the
731+
* response headers, so a rate-limited embedding request retried blind.
732+
*/
733+
it('carries the provider-stated retry wait onto the thrown error', async () => {
734+
vi.useFakeTimers()
735+
setEnv({ OPENAI_API_KEY: 'openai-test' })
736+
fetchMock.mockResolvedValue({
737+
ok: false,
738+
status: 429,
739+
statusText: '429',
740+
headers: new Headers({ 'retry-after': '42' }),
741+
json: async () => ({ error: 'rate limited' }),
742+
text: async () => 'rate limited',
743+
} as Response)
744+
745+
const pending = embed(['hello'], { ...options, apiKey: 'openai-test' }).catch((e) => e)
746+
await vi.runAllTimersAsync()
747+
const error = await pending
748+
749+
expect(error).toBeInstanceOf(EmbeddingAPIError)
750+
expect(error.status).toBe(429)
751+
expect(error.retryAfterMs).toBe(42_000)
752+
})
753+
723754
it('classifies only transient embedding failures for failover', () => {
724755
expect(isTransientEmbeddingError(new EmbeddingAPIError('unavailable', 503))).toBe(true)
725756
expect(isTransientEmbeddingError(new EmbeddingAPIError('rate limited', 429))).toBe(true)

apps/sim/lib/embeddings/client.ts

Lines changed: 60 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,19 +20,34 @@ import {
2020
import { resolveProviderKey } from '@/lib/embeddings/keys'
2121
import { DEFAULT_OPENROUTER_EMBEDDING_MODEL } from '@/lib/embeddings/openrouter-models'
2222
import { getAdapterFactory } from '@/lib/embeddings/providers'
23+
import { resolveEmbeddingRetryDelayMs } from '@/lib/embeddings/rate-limit'
2324
import type {
2425
EmbeddingProviderAdapter,
2526
EmbeddingTaskType,
2627
EmbedOptions,
2728
EmbedResult,
2829
OpenRouterEmbedOptions,
2930
} from '@/lib/embeddings/types'
30-
import { isRetryableError, retryWithExponentialBackoff } from '@/lib/knowledge/documents/utils'
31+
import {
32+
attachRetryHeaders,
33+
isRetryableError,
34+
retryWithExponentialBackoff,
35+
} from '@/lib/knowledge/documents/utils'
3136
import { batchByTokenLimit, estimateTokenCount, truncateToTokenLimit } from '@/lib/tokenization'
3237

3338
const logger = createLogger('EmbeddingClient')
3439

35-
const MAX_CONCURRENT_BATCHES = envNumber(env.KB_CONFIG_CONCURRENCY_LIMIT, 50)
40+
/**
41+
* Embedding requests issued concurrently within a single embed call.
42+
*
43+
* A provider's rate limit is per API key, so this multiplies with however many
44+
* documents are being processed at once: the document-processing queue admits
45+
* {@link env.KB_CONFIG_CONCURRENCY_LIMIT} task runs, each reaching here. It was
46+
* previously read from that same variable, so one knob set both factors and the
47+
* product reached four figures of in-flight requests against one key — enough to
48+
* hold a provider at its limit indefinitely, which no retry policy can absorb.
49+
*/
50+
const MAX_CONCURRENT_BATCHES = envNumber(env.KB_CONFIG_EMBEDDING_CONCURRENCY, 8)
3651
const EMBEDDING_REQUEST_TIMEOUT_MS = 60_000
3752

3853
/**
@@ -47,9 +62,21 @@ const EMBEDDING_REQUEST_TIMEOUT_MS = 60_000
4762
*/
4863
const BATCH_TOKEN_TARGET = 8192
4964

65+
/** Retries after the initial attempt, per embedding request. */
66+
export const EMBEDDING_MAX_RETRIES = 5
67+
68+
/** Ceiling on a single wait between embedding attempts, including a provider-stated one. */
69+
export const EMBEDDING_MAX_RETRY_DELAY_MS = 30_000
70+
5071
export class EmbeddingAPIError extends Error {
5172
public status: number
5273

74+
/**
75+
* Wait the provider asked for, read from the rejected response. Consumed by
76+
* {@link retryWithExponentialBackoff}, which prefers it over its own backoff.
77+
*/
78+
public retryAfterMs?: number
79+
5380
constructor(message: string, status: number) {
5481
super(message)
5582
this.name = 'EmbeddingAPIError'
@@ -188,10 +215,27 @@ async function callEmbeddingAPI(
188215

189216
if (!response.ok) {
190217
const errorText = await response.text()
191-
throw new EmbeddingAPIError(
218+
const error = new EmbeddingAPIError(
192219
`Embedding API failed: ${response.status} ${response.statusText} - ${errorText}`,
193220
response.status
194221
)
222+
223+
/**
224+
* Carry the provider's own answer to "when may I retry" onto the error,
225+
* the way `fetchWithRetry` does for connectors. Without it the retry
226+
* loop had nothing but blind exponential backoff and would exhaust every
227+
* attempt inside a rate-limit window that had not yet reopened.
228+
*
229+
* The headers travel non-enumerably so the retry condition can re-read
230+
* them without the bag reaching a log line.
231+
*/
232+
attachRetryHeaders(error, response.headers)
233+
const waitMs = resolveEmbeddingRetryDelayMs(response.headers)
234+
if (waitMs !== null) {
235+
error.retryAfterMs = waitMs
236+
}
237+
238+
throw error
195239
}
196240

197241
const json = await response.json()
@@ -208,9 +252,20 @@ async function callEmbeddingAPI(
208252
return { embeddings, totalTokens }
209253
},
210254
{
211-
maxRetries: 3,
255+
/**
256+
* Sized against a rate-limit window rather than a transient blip. The
257+
* provider states its reset in tens of seconds, and the loop clamps that
258+
* stated wait to `maxDelayMs` — at the previous 10s ceiling every attempt
259+
* fired before the window reopened, so the budget was spent without one
260+
* retry landing in the reopened window.
261+
*
262+
* Bounded so a fully saturated provider cannot outlive the task: five
263+
* attempts at the ceiling is well inside `KB_CONFIG_MAX_DURATION`, and
264+
* batches wait concurrently rather than one after another.
265+
*/
266+
maxRetries: EMBEDDING_MAX_RETRIES,
212267
initialDelayMs: 1000,
213-
maxDelayMs: 10000,
268+
maxDelayMs: EMBEDDING_MAX_RETRY_DELAY_MS,
214269
retryCondition: isTransientEmbeddingError,
215270
}
216271
)
Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it } from 'vitest'
5+
import { parseGoDurationMs, resolveEmbeddingRetryDelayMs } from '@/lib/embeddings/rate-limit'
6+
7+
function headers(values: Record<string, string>): { get(name: string): string | null } {
8+
return { get: (name: string) => values[name] ?? null }
9+
}
10+
11+
describe('parseGoDurationMs', () => {
12+
it('reads the shapes OpenAI documents for its reset headers', () => {
13+
expect(parseGoDurationMs('1s')).toBe(1000)
14+
expect(parseGoDurationMs('6m0s')).toBe(360_000)
15+
expect(parseGoDurationMs('23h47m36.648s')).toBeCloseTo(85_656_648, 0)
16+
})
17+
18+
/**
19+
* `m` and `ms` share a prefix, so a parser that matched the shorter unit first
20+
* would read a twelve-millisecond wait as a twelve-minute one — a 60,000x
21+
* overstatement that would stall ingestion on a healthy provider.
22+
*/
23+
it('does not confuse milliseconds with minutes', () => {
24+
expect(parseGoDurationMs('12ms')).toBe(12)
25+
expect(parseGoDurationMs('12m')).toBe(720_000)
26+
})
27+
28+
it('refuses a value the format does not fully describe', () => {
29+
expect(parseGoDurationMs('')).toBeNull()
30+
expect(parseGoDurationMs(' ')).toBeNull()
31+
expect(parseGoDurationMs('soon')).toBeNull()
32+
expect(parseGoDurationMs('60')).toBeNull()
33+
expect(parseGoDurationMs('1s later')).toBeNull()
34+
expect(parseGoDurationMs('1y')).toBeNull()
35+
})
36+
})
37+
38+
describe('resolveEmbeddingRetryDelayMs', () => {
39+
it('prefers Retry-After, which names the wait directly', () => {
40+
expect(
41+
resolveEmbeddingRetryDelayMs(
42+
headers({ 'retry-after': '20', 'x-ratelimit-reset-tokens': '6m0s' })
43+
)
44+
).toBe(20_000)
45+
})
46+
47+
/**
48+
* The cap in `parseRetryAfter` defaults to 30s. The retry loop owns the clamp
49+
* against its own ceiling, so a longer stated wait must arrive intact rather
50+
* than being silently truncated on the way in.
51+
*/
52+
it('passes a long Retry-After through uncapped', () => {
53+
expect(resolveEmbeddingRetryDelayMs(headers({ 'retry-after': '120' }))).toBe(120_000)
54+
})
55+
56+
it('falls back to the reset of the dimension that is actually exhausted', () => {
57+
expect(
58+
resolveEmbeddingRetryDelayMs(
59+
headers({
60+
'x-ratelimit-remaining-tokens': '0',
61+
'x-ratelimit-reset-tokens': '45s',
62+
'x-ratelimit-remaining-requests': '4999',
63+
'x-ratelimit-reset-requests': '6m0s',
64+
})
65+
)
66+
).toBe(45_000)
67+
})
68+
69+
it('waits out the longer window when both dimensions are exhausted', () => {
70+
expect(
71+
resolveEmbeddingRetryDelayMs(
72+
headers({
73+
'x-ratelimit-remaining-tokens': '0',
74+
'x-ratelimit-reset-tokens': '45s',
75+
'x-ratelimit-remaining-requests': '0',
76+
'x-ratelimit-reset-requests': '6m0s',
77+
})
78+
)
79+
).toBe(360_000)
80+
})
81+
82+
/**
83+
* Providers stamp these headers on every response. A reset read while quota
84+
* remains says when the window rolls over, not when this request may be
85+
* retried — using it would pin an unrelated failure to a flat wait.
86+
*/
87+
it('says nothing when no dimension is exhausted', () => {
88+
expect(
89+
resolveEmbeddingRetryDelayMs(
90+
headers({
91+
'x-ratelimit-remaining-tokens': '150000',
92+
'x-ratelimit-reset-tokens': '6m0s',
93+
})
94+
)
95+
).toBeNull()
96+
})
97+
98+
it('says nothing when the response carries no rate-limit headers', () => {
99+
expect(resolveEmbeddingRetryDelayMs(headers({}))).toBeNull()
100+
})
101+
})

0 commit comments

Comments
 (0)