Skip to content

fix(rag-api): handle empty/oversize embeddings, propagate Qdrant errors - #3

Merged
AkeRyuu merged 3 commits into
mainfrom
fix/embedding-empty-vectors
May 11, 2026
Merged

AkeRyuu merged 3 commits into
mainfrom
fix/embedding-empty-vectors

Conversation

@AkeRyuu

@AkeRyuu AkeRyuu commented May 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Three-layer defense against the indexer crash where Ollama returns [] for empty or oversize prompts and the entire Qdrant upsert batch fails with 400. Bug reported by cdl project on 2026-05-11 (report — kept locally in /home/ake/cdl/.claude/rag-backup-pre-2560d/REKA_BUG_REPORT.md).

Root causeEmbeddingService happily returned [] from Ollama, indexer pushed empty-vector points to vectorStore.upsert, Qdrant rejected the entire batch with 400, and clients saw only an opaque UNKNOWN_ERROR.

Changes

Embedding layer (root cause)

  • New EmbeddingError (502, non-retryable)
  • sanitizeInput: trims, rejects empty, truncates over EMBEDDING_MAX_INPUT_CHARS
  • validateOutput: rejects empty / undersized vectors, slices to VECTOR_SIZE
  • Applied to all 6 provider methods. Ollama batch falls back to per-text computeEmbedding for individual bad slots so one poison input no longer kills the other 31 valid embeddings.

Indexer (defense in depth)

  • filterValidDensePoints / filterValidSparsePoints exported helpers
  • Applied at all 9 upsert / upsertSparse call sites in indexProject, indexFiles, and reindexAfterModelChange. Skipped points counted in stats.errors.

Vector store (observability)

  • wrapQdrantError extracts qdrant.error.message from the client envelope
  • upsert / upsertSparse / upsertWithBM25 now throw ExternalServiceError with the actual Qdrant response. Clients see qdrant: Wrong vector size: expected 1024, got 0 instead of UNKNOWN_ERROR.

Boot + health

  • verifyEmbeddingDim() probes the provider at startup; fails fast on dim mismatch (gated by EMBEDDING_STARTUP_DIM_CHECK, default true); warns when provider is unreachable
  • /health exposes embeddingDim (measured) alongside vectorSize (configured)

New env

  • EMBEDDING_MAX_INPUT_CHARS (default 24000)
  • EMBEDDING_STARTUP_DIM_CHECK (default true)

Version

rag-api 2.3.0 → 2.3.1

Test plan

  • Unit: 5 new embedding cases (empty input, empty/short vector responses, oversize truncation, batch poisoning)
  • Unit: 3 new indexer cases (filterValidDensePoints / filterValidSparsePoints)
  • Unit: 1 new vector-store case (Qdrant 400 wrapping)
  • npm run build clean
  • npx vitest run — 13 embedding + 8 indexer + 34 vector-store tests green
  • Smoke: mcp__cdl-rag__index_codebase --force on cdl monorepo (~150-10K files) completes without 400
  • Smoke: /health returns embeddingDim: 2560 against running Ollama + qwen3-embedding:4b

Not in this PR (deliberate)

  • Context-aware token-level chunking (chars truncation is sufficient for this bug)
  • Auto-recreate collections on dim mismatch (fail-fast is safer; auto-recreate could mask unauthorized model changes)
  • Changing embedding return type to nullable (91 call sites; throw is less invasive)

🤖 Generated with Claude Code

AkeRyuu and others added 2 commits May 11, 2026 21:20
Three-layer defense against the indexer crash where Ollama returns []
for empty or oversize prompts and the entire upsert batch fails with
Qdrant 400 (reported by cdl on 2026-05-11).

Embedding layer (root cause):
- New EmbeddingError (502, non-retryable) in utils/errors
- sanitizeInput: trims, rejects empty, truncates over EMBEDDING_MAX_INPUT_CHARS
- validateOutput: rejects empty / undersized vectors, slices to VECTOR_SIZE
- Applied to all 6 provider methods (BGE single/batch/full/batchFull,
  Ollama single/batch, OpenAI). Ollama batch falls back to per-text
  computeEmbedding for individual bad slots so one poison input
  no longer kills 31 valid embeddings.

Indexer (defense in depth):
- filterValidDensePoints / filterValidSparsePoints exported helpers
- Applied at all 9 vectorStore.upsert / upsertSparse call sites in
  indexProject, indexFiles, and reindexAfterModelChange. Skipped
  points are counted in stats.errors and logged.

Vector store (observability):
- wrapQdrantError extracts qdrant.error.message from the client envelope
- upsert / upsertSparse / upsertWithBM25 now throw ExternalServiceError
  with the actual Qdrant response instead of the opaque UNKNOWN_ERROR

Boot + health:
- verifyEmbeddingDim() probes the provider at startup, fails fast on
  dim mismatch (gated by EMBEDDING_STARTUP_DIM_CHECK, default true)
  and warns when provider is unreachable
- /health now exposes embeddingDim (measured) alongside vectorSize (configured)

New env:
- EMBEDDING_MAX_INPUT_CHARS (default 24000)
- EMBEDDING_STARTUP_DIM_CHECK (default true)

Tests:
- 5 new embedding cases (empty input, empty/short vector responses,
  oversize truncation, batch poisoning)
- 3 new indexer cases (filter helpers)
- 1 new vector-store case (Qdrant 400 wrapping)

Version: 2.3.0 → 2.3.1

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rror tests

Addresses code review on PR #3.

Blocker: embedBatchOllama fallback to computeEmbedding could throw and
escape the per-slot loop, killing the remaining valid embeddings — exactly
the failure mode the per-slot recovery was supposed to prevent. Now the
fallback failure is caught, logged, and the slot is parked as []. The
indexer's filterValidDensePoints drops it before upsert and bumps
stats.errors, so the rest of the batch survives.

Major: missing test coverage for the central new logic.
- Added 2 EmbeddingError cases in errors.test.ts (statusCode/code/retryable + isRetryableError)
- Added 2 embedBatchOllama cases:
  - per-slot recovery: empty slot triggers per-text fallback, batch survives
  - fallback also fails: empty placeholder parked, other slots preserved

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@AkeRyuu

AkeRyuu commented May 11, 2026

Copy link
Copy Markdown
Contributor Author

Code review pass (commit 4b746c3):

Blocker fixedembedBatchOllama per-slot fallback now wraps computeEmbedding in try/catch. If the fallback also fails (e.g. provider regression hits the same input), the slot is parked as [] and continues, preserving the remaining valid embeddings in the batch. Indexer's filterValidDensePoints drops the placeholder at upsert and increments stats.errors.

Major fixes — added test coverage for the central new logic:

  • errors.test.ts: 2 cases for EmbeddingError (statusCode 502, code, retryable=false, message format, integration with isRetryableError)
  • embedding.test.ts: 2 embedBatchOllama cases — happy-path per-slot recovery (one empty slot → per-text fallback succeeds → batch intact) and pessimistic case (fallback also fails → empty placeholder parked → other slots preserved)

Reviewer's other comments (502 vs 422 status, broad wrapQdrantError semantics, actualEmbeddingDim race) — kept as-is, low impact, can be revisited in a follow-up.

All 83 tests in the affected files green. Build clean.

…ssertion)

These tests have been failing on main for some time but only surfaced now
because we want a clean suite for the PR. Not caused by the embedding fix —
both fail at HEAD~2 the same way.

auth.test.ts (2 cases):
- "denies access when no API keys configured" and "allows anonymous access"
  both call resetKeys() expecting an empty keyStore, but loadKeys() reads
  process.cwd()/data/keys.json which exists in any workspace where rag-api
  has run (Docker volume mount, local dev). The two scenarios then take
  the wrong code branch (AUTH_REQUIRED instead of AUTH_NOT_CONFIGURED, etc.)
- Fix: vi.mock fs with existsSync stubbed to false in beforeEach so
  loadKeys() sees an absent keys file regardless of workspace state.

llm.test.ts (1 case):
- "routes utility tasks to Ollama with think:false" asserted body.think
  to be undefined, but llm.ts:213 unconditionally sets body.think =
  enableThink. Comment in llm.ts explains why: qwen3.5 chat endpoint
  returns empty responses if the field is omitted entirely.
- Fix: assert body.think === false (the actual current behavior).

After this commit: 907 tests pass, 0 failures.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@AkeRyuu

AkeRyuu commented May 11, 2026

Copy link
Copy Markdown
Contributor Author

Commit `15c7d51` — fixed 3 pre-existing test failures unrelated to the embedding work, so the suite goes green for this PR:

  • `auth.test.ts` (2): `resetKeys()` reloads from `process.cwd()/data/keys.json` — when that file exists (Docker volume mount, local dev), the "no keys configured" scenarios take the wrong branch. Now mocks `fs.existsSync` so the keys file appears absent.
  • `llm.test.ts` (1): assertion expected `body.think === undefined`, but `llm.ts:213` unconditionally sets the field (qwen3.5 returns empty responses otherwise — comment in source). Updated to assert `false`.

907 tests pass, 0 failures, build clean.

@AkeRyuu
AkeRyuu merged commit 59f8bb7 into main May 11, 2026
3 of 5 checks passed
@AkeRyuu
AkeRyuu deleted the fix/embedding-empty-vectors branch May 11, 2026 22:03
AkeRyuu added a commit that referenced this pull request Jun 11, 2026
fix(mcp): @getreka/mcp in setup_project + expose tribunal_debate
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.

1 participant