fix: shrink oversized embedding input instead of dropping the document - #90
Merged
Merged
Conversation
`createEmbeddingOrEmpty` logs a failed embedding as "transient/non-fatal" and
moves on. For a rate limit that is right. For an over-length input it is not:
the input is deterministic, so the same document fails on every rebuild, and
the index keeps a permanent hole that looks like a passing run.
Observed on a 567-table MySQL connection during a full reindex:
Skipping embedding for RELATIONSHIP document b7efe9d2-... due to
transient/non-fatal error: 400: Invalid 'input': maximum context length
is 8192 tokens.
The cause is the character budget, not a missing one. `truncate` already cut
input to `app.embedding.max-chars` (default 30,000) on the stated assumption of
"roughly 4 chars per token", which would be 7,500 tokens. That ratio holds for
prose. It does not hold for what this service embeds: schema and relationship
documents are dense identifiers, underscores, punctuation and repeated
scaffolding, which tokenize closer to 2-3 chars per token. At that density
30,000 chars is 10,000-15,000 tokens and the provider rejects the call.
No fixed ratio is safe across content, so this stops betting on one.
`LlmErrorCategory.CONTEXT_LENGTH` already documents itself as "never retry; the
caller may trim" — until now nothing trimmed. `embedWithShrink` halves the
budget on each CONTEXT_LENGTH rejection (30,000 -> 15,000 -> ... -> 1,875,
floor 1,000) and lets the provider decide when the call fits. That needs no
tokenizer dependency and stays correct for any content and any model window.
Deliberately narrow:
- Only CONTEXT_LENGTH shrinks. Retries and fail-open are untouched for every
other category, since a smaller input answers nothing about a rate limit or
a rejected credential.
- The inner attempt runs with fail-open off so the rejection reaches the shrink
loop; fail-open would convert it into an empty vector indistinguishable from
a real one. The operator's fail-open setting is still honoured once shrinking
is exhausted.
- Shrinking a batch only affects members longer than the budget, so one
oversized text costs the short ones nothing.
- Bounded at 4 halvings, so a pathological document cannot loop.
Tests cover all three paths: shrink-then-succeed, exhaust-then-fail-open, and
no-shrink for a category shrinking cannot fix.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e ceiling Six issues from review, one of which partly defeated the original fix. 1. The budget was seeded from `maxChars` rather than from the text being sent, so `truncate` (which only cuts when length > budget) made the first halvings byte-identical resends. A document below maxChars/2^4 — 1,875 chars at the default — could never shrink at all: five identical rejected calls and then the same permanent index hole this change exists to close, at 5x the cost. The budget now starts at min(maxChars, longest input). 2. The batch comment claimed one oversized member "cannot cost the short ones any content". Untrue: the budget is per-request, so a halving forced by one text also trims every other member above the new budget. The comment now says so, and seeds from the longest member. Also notes that a provider may reject on the request's aggregate token count, where trimming members is the right lever but the floor may arrive before the batch fits. 3. Intermediate attempts ran through `attempt(..., mayFailOpen=false)`, whose `handleFailure` logs before it rethrows — so every successful shrink emitted "Embedding failed" with a stack trace for a call that then succeeded, and a terminal failure logged twice with contradictory failOpen values. The retry loop is now `attemptOrThrow`, with logging left to the single terminal `handleFailure`. 4. The 4-arg `attempt` overload became unreachable once both callers moved to `embedWithShrink`; removed. 5. `givesUpAfterTheShrinkFloor...` asserted the attempt cap, not the floor — at a 30,000 ceiling the cap is always reached first. Renamed to say what it tests, and a real floor test added at a 1,500 ceiling. 6. The shrink log printed the budget as though it were the payload size, which would mislead exactly the person debugging (1). New tests: shrinksRelativeToTheInputNotTheConfiguredCeiling (regression for 1, fails on the previous commit) and stopsAtTheCharacterFloorRatherThanEmbedding ATokenOfContent (coverage for 5). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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
2 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.
Problem
createEmbeddingOrEmptylogs a failed embedding as "transient/non-fatal" and moves on. For a rate limit that's right. For an over-length input it isn't: the input is deterministic, so the same document fails on every rebuild, and the index keeps a permanent hole that looks like a passing run.Observed on a 567-table MySQL connection during a full reindex:
Cause: the character budget, not a missing one
truncatealready cut input toapp.embedding.max-chars(default 30,000) on the stated assumption of "roughly 4 chars per token" — which would be 7,500 tokens, safely under 8,192.That ratio holds for prose. It does not hold for what this service actually embeds: schema and relationship documents are dense identifiers, underscores, punctuation and repeated scaffolding, which tokenize closer to 2–3 chars per token. At that density 30,000 chars is 10,000–15,000 tokens, and the provider rejects the call.
Fix
No fixed ratio is safe across content, so this stops betting on one.
LlmErrorCategory.CONTEXT_LENGTHalready documents itself as "never retry; the caller may trim" — until now nothing trimmed.embedWithShrinkhalves the budget on each CONTEXT_LENGTH rejection (30,000 → 15,000 → … → 1,875, floor 1,000) and lets the provider decide when the call fits. No tokenizer dependency, and correct for any content and any model window.Deliberately narrow
Test plan
shrinksTheInputWhenTheProviderRejectsItAsTooLong— succeeds after one halving; asserts the two calls were 30,000 then 15,000 chars.givesUpAfterTheShrinkFloorAndStillHonoursFailOpen— 5 attempts then empty, proving the bound.doesNotShrinkForFailuresThatShrinkingCannotFix— AUTH gets exactly one call.Follow-up (not in this PR)
The
"transient/non-fatal"wording inTrainingService.createEmbeddingOrEmptyis what made this invisible for so long — it asserts transience for six call sites without knowing the category. Worth revisiting separately.🤖 Generated with Claude Code