Repository navigation
fix(kilo-indexing): split oversized embedding batches on context overflow - #15456
PeterHodges15 wants to merge 1 commit into
Conversation
…flow When an embedding batch exceeds the model's maximum input context (e.g. small-context models such as Granite that reject batches over 512 tokens), the provider returns a deterministic 400 that the scanner retried three times before skipping the file, silently dropping files from the index. - Split the batch reactively in embedBatches when the endpoint rejects it with a context-overflow error (isContextOverflowError), preserving order and summing token usage. - Stop retrying deterministic 4xx embedding responses (except 429) in the scanner so unsplittable single-item overflows fail fast instead of burning retries. - Preserve the HTTP status on formatted embedding errors so callers can distinguish retryable from deterministic failures.
github-actions
Bot
added
community
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| }, | ||
| } | ||
| } | ||
| throw error |
There was a problem hiding this comment.
SUGGESTION: A single oversized item still fails the entire batch
When one block is itself over the model's limit (batch.length === 1), this rethrow aborts the whole batch, so every file represented in that batch is still dropped from the index — just faster than before. That case is reachable for small-context models because the batching guard is still MAX_ITEM_TOKENS = 8191 (the openai-compatible default), far above e.g. Granite's ~512 tokens, so any single block between those sizes triggers it. Consider truncating an over-limit item to the model limit, or skipping just that block with a warning, instead of failing its neighbours.
| * 5xx (server) errors are still considered retryable. | ||
| */ | ||
| export function isNonRetryableEmbeddingError(error: unknown): boolean { | ||
| const status = extractStatusCode(error) |
There was a problem hiding this comment.
SUGGESTION: extractStatusCode misses AWS SDK (Bedrock) errors
extractStatusCode only reads error.status, error.response.status, and HTTP <n>: in the message. AWS SDK v3 errors (Bedrock) expose the HTTP status via $metadata.httpStatusCode, so here status is undefined: deterministic Bedrock 4xx responses stay retryable at the scanner, and isContextOverflowError falls back to message-only matching. Adding error.$metadata?.httpStatusCode to extractStatusCode would make the new fast-fail behaviour match the PR's provider-agnostic intent.
| const status = extractStatusCode(error) | ||
| if (status === undefined) return false | ||
| if (status === 429) return false | ||
| return status >= 400 && status < 500 |
There was a problem hiding this comment.
SUGGESTION: Transient 4xx statuses are now treated as non-retryable
408 Request Timeout and 425 Too Early are retryable 4xx responses. Classifying every 4xx except 429 as deterministic means a genuine timeout now fails the batch on the first attempt. Consider excluding 408 and 425 alongside 429 here.
| // burning the remaining retries — this is what produced the "400 after 3 | ||
| // retries" symptom in #15450. Transient errors (429, 5xx, network) still retry. | ||
| if (isNonRetryableEmbeddingError(error)) { | ||
| break |
There was a problem hiding this comment.
SUGGESTION: Failure reporting still claims every retry was used
After this early break, the code below still logs Failed to process batch after ${this.maxBatchRetries} attempts and calls emitError(mode, "scanner:processBatch", lastError, this.maxBatchRetries), so a failure that happened on the first attempt is reported and telemetried as if three retries were exhausted. Passing the actual attempts value would keep the log line, the error telemetry retryCount, and the onError message accurate.
| ["Rate limit exceeded", 429], | ||
| ["Internal server error", 500], | ||
| ["Input had invalid characters", 400], | ||
| ])("does not treat a non-overflow %d as a context overflow: %s", (message, status) => { |
There was a problem hiding this comment.
SUGGESTION: Format specifiers do not match the tuple order
Each row is [message, status], so %d is substituted with a string and renders NaN in the test name.
| ])("does not treat a non-overflow %d as a context overflow: %s", (message, status) => { | |
| ])("does not treat a non-overflow %s as a context overflow: %d", (message, status) => { |
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (8 files)
Fix these issues in Kilo Cloud Reviewed by deepseek-flash-latest · Input: 63.5K · Output: 18.2K · Cached: 615.3K Review guidance: REVIEW.md from base branch |
Summary
Fixes #15450 — the indexer silently skips files when an embedding batch exceeds the embedding model's maximum input context.
Problem
The indexer caps individual items at OpenAI-sized token limits (
MAX_ITEM_TOKENS = 8191) and packs up toBATCH_SEGMENT_THRESHOLD = 60blocks per batch. Small-context embedding models (e.g. Granite, ~512-token input) reject such batches with a deterministic400:The scanner treats this as transient and retries it three times, then skips the file — so files are silently dropped from the index (
Indexing partially failed: Only 27 of 96 files were indexed).Fix
embedBatchesnow detects a context-overflow400(isContextOverflowError) from the endpoint and binary-splits the batch, re-requesting the halves. This is provider-agnostic and lives in the shared path used by all five embedders (openai-compatible, openai, voyage, openrouter, bedrock). Item order is preserved and token usage is summed.formatEmbeddingErrornow carries the HTTPstatuson the returned error so callers can distinguish retryable from deterministic failures.Testing
embedder-helpers.test.ts: split behaviour (order, usage summation, split-to-single, rethrow on single-item overflow, no split on non-overflow 4xx/5xx).validation-helpers.test.ts:isContextOverflowError,isNonRetryableEmbeddingError, and status preservation.scanner.test.ts: a deterministic 4xx embed failure is not retried.openai-compatible.test.ts: end-to-end split through the OpenAI client.All
kilo-indexingunit tests pass;tsgo --noEmitclean.