From 93cd6153c16a39b6608176d83db8fdd7ea3250ce Mon Sep 17 00:00:00 2001 From: Charon Date: Sat, 11 Apr 2026 17:33:49 +0200 Subject: [PATCH 1/2] fix: hybrid retrieval graceful degradation with Promise.allSettled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace Promise.all with Promise.allSettled in hybridRetrieval() so that if either vector or BM25 search throws, the other search's results are still used instead of failing the entire query. Behavior: - One search fails → other search results returned, failed side logged - Both fail → throws with failureStage=hybrid.parallelSearch - Diagnostics correctly reflect 0 for failed search counts Fixes Bug 3 from community code audit. --- src/retriever.ts | 39 ++++++++++++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 7 deletions(-) diff --git a/src/retriever.ts b/src/retriever.ts index 769c248b..67f4c787 100644 --- a/src/retriever.ts +++ b/src/retriever.ts @@ -916,24 +916,49 @@ export class MemoryRetriever { trace?.startStage("parallel_search", []); failureStage = "hybrid.parallelSearch"; - const [vectorResults, bm25Results] = await Promise.all([ + const settledResults = await Promise.allSettled([ this.runVectorSearch( queryVector, candidatePoolSize, scopeFilter, category, - ).catch((error) => { - throw attachFailureStage(error, "hybrid.vectorSearch"); - }), + ), this.runBM25Search( bm25Query, candidatePoolSize, scopeFilter, category, - ).catch((error) => { - throw attachFailureStage(error, "hybrid.bm25Search"); - }), + ), ]); + + const vectorResult_ = settledResults[0]; + const bm25Result_ = settledResults[1]; + + let vectorResults: RetrievalResult[]; + let bm25Results: RetrievalResult[]; + + if (vectorResult_.status === "rejected") { + const error = attachFailureStage(vectorResult_.reason, "hybrid.vectorSearch"); + console.warn(`[Retriever] vector search failed: ${error.message}`); + vectorResults = []; + } else { + vectorResults = vectorResult_.value; + } + + if (bm25Result_.status === "rejected") { + const error = attachFailureStage(bm25Result_.reason, "hybrid.bm25Search"); + console.warn(`[Retriever] bm25 search failed: ${error.message}`); + bm25Results = []; + } else { + bm25Results = bm25Result_.value; + } + + if (vectorResults.length === 0 && bm25Results.length === 0) { + throw attachFailureStage( + new Error("both vector and BM25 search failed"), + "hybrid.parallelSearch", + ); + } if (diagnostics) { diagnostics.vectorResultCount = vectorResults.length; diagnostics.bm25ResultCount = bm25Results.length; From e43a31880a37e564efaf92e8ae95c754703fd7c4 Mon Sep 17 00:00:00 2001 From: Charon Date: Fri, 17 Apr 2026 08:50:26 +0200 Subject: [PATCH 2/2] Fix both-backend-fail guard: check rejection status, not array length MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves reviewer rwmjhb's F1 and MR1 feedback: **F1: Fix both-fail guard** - Changed from checking `results.length === 0` to checking `settledResults.status === "rejected"` for both backends - Empty result sets are now correctly distinguished from failures - Error message enriched with rejection reasons for debugging **MR1: Update existing tests + add new test coverage** - Updated 2 existing tests in query-expander.test.mjs to reflect graceful-degradation behavior (no longer throws on partial failures) - Added new test file: retriever-graceful-degradation.test.mjs with 7 comprehensive tests covering: - Both backends fail → throws error - One fails, one succeeds → uses successful backend results - Both succeed with empty results → returns empty array - Empty results vs. both-fail distinction **SE-approved implementation** (3 conditions verified): 1. Fallback behavior confirmed: fuseResults() handles partial failures 2. Type safety: Promise.allSettled pattern maintained 3. Error enrichment: rejection reasons included in error message All 20 tests passing (7 new + 13 existing). --- package-lock.json | 1 - src/retriever.ts | 11 +- test/query-expander.test.mjs | 69 ++-- test/retriever-graceful-degradation.test.mjs | 322 +++++++++++++++++++ 4 files changed, 375 insertions(+), 28 deletions(-) create mode 100644 test/retriever-graceful-degradation.test.mjs diff --git a/package-lock.json b/package-lock.json index 96bfabe4..de165655 100644 --- a/package-lock.json +++ b/package-lock.json @@ -233,7 +233,6 @@ "resolved": "https://registry.npmjs.org/apache-arrow/-/apache-arrow-18.1.0.tgz", "integrity": "sha512-v/ShMp57iBnBp4lDgV8Jx3d3Q5/Hac25FWmQ98eMahUiHPXcvwIMKJD0hBIgclm/FCG+LwPkAKtkRO1O/W0YGg==", "license": "Apache-2.0", - "peer": true, "dependencies": { "@swc/helpers": "^0.5.11", "@types/command-line-args": "^5.2.3", diff --git a/src/retriever.ts b/src/retriever.ts index 67f4c787..a1a53a2e 100644 --- a/src/retriever.ts +++ b/src/retriever.ts @@ -953,9 +953,16 @@ export class MemoryRetriever { bm25Results = bm25Result_.value; } - if (vectorResults.length === 0 && bm25Results.length === 0) { + // Check if BOTH backends failed (rejected), not just empty results + // Empty result sets are valid; only throw when both promises reject + const bothFailed = + vectorResult_.status === "rejected" && bm25Result_.status === "rejected"; + + if (bothFailed) { + const vectorError = vectorResult_.reason?.message || "unknown"; + const bm25Error = bm25Result_.reason?.message || "unknown"; throw attachFailureStage( - new Error("both vector and BM25 search failed"), + new Error(`both vector and BM25 search failed: ${vectorError}, ${bm25Error}`), "hybrid.parallelSearch", ); } diff --git a/test/query-expander.test.mjs b/test/query-expander.test.mjs index 6035281d..1ad83a2e 100644 --- a/test/query-expander.test.mjs +++ b/test/query-expander.test.mjs @@ -255,7 +255,7 @@ describe("retriever BM25 query expansion gating", () => { }); it("distinguishes vector-search failures inside the hybrid parallel stage", async () => { - const { retriever } = createRetrieverHarness( + const { retriever, bm25Queries } = createRetrieverHarness( {}, { async vectorSearch() { @@ -264,51 +264,70 @@ describe("retriever BM25 query expansion gating", () => { }, ); - await assert.rejects( - retriever.retrieve({ - query: "普通查询", - limit: 1, - source: "manual", - }), - /simulated vector search failure/, - ); + // With graceful degradation, BM25 results should be returned even though vector failed + const results = await retriever.retrieve({ + query: "普通查询", + limit: 1, + source: "manual", + }); + + // Results from BM25 should be returned + assert.ok(results.length > 0); + assert.ok(bm25Queries.length > 0); + // Overall retrieval did not fail (graceful degradation) assert.equal( retriever.getLastDiagnostics()?.failureStage, - "hybrid.vectorSearch", + undefined, ); + // Vector result count should be 0 (failed) assert.equal( - retriever.getLastDiagnostics()?.errorMessage, - "simulated vector search failure", + retriever.getLastDiagnostics()?.vectorResultCount, + 0, + ); + // BM25 result count should be > 0 (succeeded) + assert.ok( + (retriever.getLastDiagnostics()?.bm25ResultCount ?? 0) > 0, ); }); it("distinguishes bm25-search failures inside the hybrid parallel stage", async () => { - const { retriever } = createRetrieverHarness( + const { retriever, bm25Queries } = createRetrieverHarness( {}, { - async bm25Search() { + async bm25Search(query) { + bm25Queries.push(query); throw new Error("simulated bm25 search failure"); }, }, ); - await assert.rejects( - retriever.retrieve({ - query: "普通查询", - limit: 1, - source: "manual", - }), - /simulated bm25 search failure/, - ); + // With graceful degradation, vector results should be returned even though BM25 failed + const results = await retriever.retrieve({ + query: "普通查询", + limit: 1, + source: "manual", + }); + // Results from vector should be returned (empty by default in harness) + assert.equal(results.length, 0); + // BM25 was attempted (even though it failed) + assert.equal(bm25Queries.length, 1); + + // Overall retrieval did not fail (graceful degradation) assert.equal( retriever.getLastDiagnostics()?.failureStage, - "hybrid.bm25Search", + undefined, + ); + // Vector result count should be 0 (empty by default in harness) + assert.equal( + retriever.getLastDiagnostics()?.vectorResultCount, + 0, ); + // BM25 result count should be 0 (failed) assert.equal( - retriever.getLastDiagnostics()?.errorMessage, - "simulated bm25 search failure", + retriever.getLastDiagnostics()?.bm25ResultCount, + 0, ); }); }); diff --git a/test/retriever-graceful-degradation.test.mjs b/test/retriever-graceful-degradation.test.mjs new file mode 100644 index 00000000..9994224b --- /dev/null +++ b/test/retriever-graceful-degradation.test.mjs @@ -0,0 +1,322 @@ +import assert from "node:assert"; +import { describe, it } from "node:test"; +import jitiFactory from "jiti"; + +const jiti = jitiFactory(import.meta.url, { + interopDefault: true, +}); + +const { createRetriever } = jiti("../src/retriever.ts"); + +function buildResult(id = "memory-1", text = "test result") { + return { + entry: { + id, + text, + vector: [0.1, 0.2, 0.3], + category: "other", + scope: "global", + importance: 0.7, + timestamp: 1700000000000, + metadata: "{}", + }, + score: 0.9, + }; +} + +function createRetrieverHarness( + config = {}, + storeOverrides = {}, + embedderOverrides = {}, +) { + const bm25Queries = []; + const embeddedQueries = []; + + const retriever = createRetriever( + { + hasFtsSupport: true, + async vectorSearch() { + return []; + }, + async bm25Search(query) { + bm25Queries.push(query); + return [buildResult()]; + }, + async hasId() { + return true; + }, + async get() { + return null; + }, + async upsert() { + return []; + }, + async delete() { + return; + }, + ...storeOverrides, + }, + { + async embedQuery(query) { + embeddedQueries.push(query); + return [0.1, 0.2, 0.3]; + }, + ...embedderOverrides, + }, + config, + ); + + return { retriever, bm25Queries, embeddedQueries }; +} + +describe("Retriever Graceful Degradation (Promise.allSettled)", () => { + it("throws when both vector and BM25 search reject", async () => { + const { retriever } = createRetrieverHarness( + {}, + { + async vectorSearch() { + throw new Error("vector failed"); + }, + async bm25Search() { + throw new Error("bm25 failed"); + }, + }, + ); + + await assert.rejects( + retriever.retrieve({ query: "test", limit: 1, source: "manual" }), + /both vector and BM25 search failed.*vector failed.*bm25 failed/, + ); + + assert.equal( + retriever.getLastDiagnostics()?.failureStage, + "hybrid.parallelSearch", + ); + assert.equal( + retriever.getLastDiagnostics()?.vectorResultCount, + 0, + ); + assert.equal( + retriever.getLastDiagnostics()?.bm25ResultCount, + 0, + ); + }); + + it("uses vector-only results when BM25 fails", async () => { + const { retriever, bm25Queries } = createRetrieverHarness( + {}, + { + async vectorSearch() { + return [buildResult()]; + }, + async bm25Search(query) { + bm25Queries.push(query); + throw new Error("bm25 failed"); + }, + }, + ); + + const results = await retriever.retrieve({ + query: "test", + limit: 1, + source: "manual", + }); + + assert.equal(results.length, 1); + assert.equal(bm25Queries.length, 1); // BM25 was attempted and failed + assert.equal( + retriever.getLastDiagnostics()?.vectorResultCount, + 1, + ); + assert.equal( + retriever.getLastDiagnostics()?.bm25ResultCount, + 0, + ); + assert.equal( + retriever.getLastDiagnostics()?.failureStage, + null, // No overall failure + ); + }); + + it("uses bm25-only results when vector fails", async () => { + const { retriever, bm25Queries } = createRetrieverHarness( + {}, + { + async vectorSearch() { + throw new Error("vector failed"); + }, + async bm25Search(query) { + bm25Queries.push(query); + return [buildResult()]; + }, + }, + ); + + const results = await retriever.retrieve({ + query: "test", + limit: 1, + source: "manual", + }); + + assert.equal(results.length, 1); + assert.equal(bm25Queries.length, 1); + assert.equal( + retriever.getLastDiagnostics()?.vectorResultCount, + 0, + ); + assert.equal( + retriever.getLastDiagnostics()?.bm25ResultCount, + 1, + ); + assert.equal( + retriever.getLastDiagnostics()?.failureStage, + null, // No overall failure + ); + }); + + it("returns empty results when both backends succeed with no matches", async () => { + const { retriever, bm25Queries } = createRetrieverHarness( + {}, + { + async vectorSearch() { + return []; // ✅ Success, but empty + }, + async bm25Search(query) { + bm25Queries.push(query); + return []; // ✅ Success, but empty + }, + }, + ); + + const results = await retriever.retrieve({ + query: "test", + limit: 1, + source: "manual", + }); + + // Empty result set is valid — should not throw + assert.equal(results.length, 0); + assert.equal( + retriever.getLastDiagnostics()?.vectorResultCount, + 0, + ); + assert.equal( + retriever.getLastDiagnostics()?.bm25ResultCount, + 0, + ); + assert.equal( + retriever.getLastDiagnostics()?.failureStage, + null, // No failure + ); + }); + + it("uses vector-only results when BM25 fails and vector returns empty", async () => { + const { retriever, bm25Queries } = createRetrieverHarness( + {}, + { + async vectorSearch() { + return []; // ✅ Success, but empty + }, + async bm25Search(query) { + bm25Queries.push(query); + throw new Error("bm25 failed"); + }, + }, + ); + + const results = await retriever.retrieve({ + query: "test", + limit: 1, + source: "manual", + }); + + // Empty result from successful vector search is valid + assert.equal(results.length, 0); + assert.equal(bm25Queries.length, 1); + assert.equal( + retriever.getLastDiagnostics()?.vectorResultCount, + 0, + ); + assert.equal( + retriever.getLastDiagnostics()?.bm25ResultCount, + 0, + ); + assert.equal( + retriever.getLastDiagnostics()?.failureStage, + null, // No failure + ); + }); + + it("uses bm25-only results when vector fails and bm25 returns empty", async () => { + const { retriever, bm25Queries } = createRetrieverHarness( + {}, + { + async vectorSearch() { + throw new Error("vector failed"); + }, + async bm25Search(query) { + bm25Queries.push(query); + return []; // ✅ Success, but empty + }, + }, + ); + + const results = await retriever.retrieve({ + query: "test", + limit: 1, + source: "manual", + }); + + // Empty result from successful BM25 search is valid + assert.equal(results.length, 0); + assert.equal(bm25Queries.length, 1); + assert.equal( + retriever.getLastDiagnostics()?.vectorResultCount, + 0, + ); + assert.equal( + retriever.getLastDiagnostics()?.bm25ResultCount, + 0, + ); + assert.equal( + retriever.getLastDiagnostics()?.failureStage, + null, // No failure + ); + }); + + it("normalizes empty results as distinct from both-fail errors", async () => { + const { retriever } = createRetrieverHarness( + {}, + { + async vectorSearch() { + return []; // ✅ Success, empty results + }, + async bm25Search(query) { + return []; // ✅ Success, empty results + }, + }, + ); + + const results = await retriever.retrieve({ + query: "nonexistent", + limit: 10, + source: "manual", + }); + + // This was the bug: empty results were treated as both-fail + // Now empty results should succeed (valid search outcome) + assert.equal(results.length, 0); + assert.equal( + retriever.getLastDiagnostics()?.failureStage, + null, + ); + assert.equal( + retriever.getLastDiagnostics()?.vectorResultCount, + 0, + ); + assert.equal( + retriever.getLastDiagnostics()?.bm25ResultCount, + 0, + ); + }); +});