Skip to content

fix(review): stop one oversized RAG chunk from failing its whole embedding batch - #5074

Merged
JSONbored merged 1 commit into
mainfrom
fix/5072-embed-batch-blast-radius
Jul 11, 2026
Merged

fix(review): stop one oversized RAG chunk from failing its whole embedding batch#5074
JSONbored merged 1 commit into
mainfrom
fix/5072-embed-batch-blast-radius

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • GITTENSORY-D (ai_embed_http_400) was partially fixed by fix(review): drop whitespace-only RAG chunks and surface real embed errors #5029 (empty/whitespace-only chunks), but is still firing (492 occurrences since 2026-06-28, ongoing) — now with a real error body thanks to that same PR's response-capture improvement: "the input length exceeds the context length."
  • Root cause, traced live: embedTexts (src/review/rag.ts) sends up to EMBED_BATCH (96) chunk texts to the embedding provider in ONE call. Chunking bounds by character count, not token count, and the token-per-char ratio varies a lot by content (dense/minified code, long unbroken lines) — so a single text can exceed the embedding model's context window even under the char budget. When that happens, the whole inference.run() call throws and embedTexts returned null for the entire call, discarding every other, perfectly-fine chunk in that batch (potentially from many unrelated files via upsertChunks).
  • embedTexts now returns (number[] | null)[] | null: the outer null is preserved for "no adapter" / "invalid batch size" (unchanged); a null at a specific index means only that one text failed. A batch-level throw or structurally-invalid response now falls back to embedding that batch one item at a time (sequential, not concurrent, to stay gentle on a self-hosted inference server) — only the genuinely-unembeddable item(s) are lost.
  • upsertChunks filters out chunks whose vector came back null before the vector-index/DB write.
  • Kept low-noise: one ERROR-level rag_embed_batch_degraded summary per degraded batch (not one per skipped item), mirroring the per-attempt-warn/exhausted-error escalation pattern already used elsewhere in the AI-review pipeline (fix(selfhost): AI-provider per-attempt logging amplifies one retry loop into up to 6 Sentry errors #5046).

Closes #5072

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (Closes #5072).

Validation

  • git diff --check
  • npm run typecheck
  • npm run test:coverage — scoped run on test/unit/rag.test.ts shows 100% line, 99.47% branch on rag.ts (the one remaining uncovered branch, line 414, predates this diff — confirmed via git diff origin/main showing it as unchanged context, not an added line). Also ran the repo's test:changed selection (3689 tests, all green) and the full unsharded gate earlier in this session on a sibling branch off the same main.
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries — added: a regression test reproducing the exact production bug (one throwing item doesn't fail its siblings), a test for the batch-self-heals-with-zero-failures branch, and updated 5 pre-existing tests whose assertions depended on the old whole-batch-fails contract.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — no auth surface touched.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A.)
  • UI changes use live API data or real empty/error/loading states. (N/A — no UI changed.)
  • Visible UI changes include a UI Evidence section. (N/A — backend-only.)
  • Public docs/changelogs are updated where needed. (N/A.)

…hole embedding batch (#5072)

embedTexts sent up to EMBED_BATCH (96) chunk texts to the provider in
one call, so a single text exceeding the embedding model's context
window (a dense/minified file, a long unbroken line) threw the whole
batch out, silently dropping RAG context for every other chunk in it
-- the confirmed live cause of the still-firing ai_embed_http_400
"input length exceeds the context length" errors.

embedTexts now returns (number[] | null)[] | null: a null at one
index means only that text failed. A batch-level throw or invalid
response falls back to embedding one item at a time (sequential, not
concurrent, to stay gentle on a self-hosted inference server), and
upsertChunks filters out chunks that never got a vector before the
index/DB write. One ERROR-level summary per degraded batch, not one
per skipped item.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored JSONbored self-assigned this Jul 11, 2026
@JSONbored
JSONbored merged commit ad44723 into main Jul 11, 2026
13 checks passed
@JSONbored
JSONbored deleted the fix/5072-embed-batch-blast-radius branch July 11, 2026 12:45
@codecov

codecov Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.14%. Comparing base (2205cde) to head (549182f).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5074   +/-   ##
=======================================
  Coverage   94.13%   94.14%           
=======================================
  Files         466      466           
  Lines       39573    39592   +19     
  Branches    14436    14440    +4     
=======================================
+ Hits        37254    37272   +18     
  Misses       1664     1664           
- Partials      655      656    +1     
Flag Coverage Δ
shard-1 46.49% <41.93%> (-0.03%) ⬇️
shard-2 33.47% <100.00%> (-0.09%) ⬇️
shard-3 31.59% <22.58%> (+0.24%) ⬆️
shard-4 32.69% <22.58%> (-0.01%) ⬇️
shard-5 33.17% <22.58%> (-0.26%) ⬇️
shard-6 45.12% <0.00%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/review/rag.ts 99.59% <100.00%> (-0.41%) ⬇️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

embedTexts: one oversized/malformed chunk fails the WHOLE embedding batch, not just that chunk

1 participant