diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 24db0bf433..8130c33191 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1301,12 +1301,12 @@ }, "services/code-index/__tests__/manager.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 87 + "count": 84 } }, "services/code-index/__tests__/orchestrator.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 25 + "count": 23 } }, "services/code-index/__tests__/service-factory.spec.ts": { diff --git a/src/services/code-index/__tests__/code-index-scan-executor.spec.ts b/src/services/code-index/__tests__/code-index-scan-executor.spec.ts index 33b71584ff..bcc63f7c00 100644 --- a/src/services/code-index/__tests__/code-index-scan-executor.spec.ts +++ b/src/services/code-index/__tests__/code-index-scan-executor.spec.ts @@ -136,14 +136,23 @@ describe("CodeIndexScanExecutor", () => { }, ) - it.each([0, 2])("preserves incremental batch-error tolerance with %s indexed blocks", async (indexed) => { + it.each([0, 2])("rejects incremental batch errors with %s indexed blocks", async (indexed) => { const { executor, scanner } = setup() + const failures = [new Error("embedding failure"), new Error("upsert failure"), new Error("embedding failure")] scanner.scanDirectory.mockImplementation(async (_path, onError, onIndexed, onParsed) => { onParsed?.(3) onIndexed?.(indexed) - onError?.(new Error("batch failure")) + for (const failure of failures) onError?.(failure) return { stats: { processed: 1, skipped: 0 }, totalBlockCount: 3 } }) - await expect(executor.runIncrementalScan(new AbortController().signal)).resolves.toBe(true) + const result = executor.runIncrementalScan(new AbortController().signal) + await expect(result).rejects.toBeInstanceOf(AggregateError) + await expect(result).rejects.toMatchObject({ + errors: failures, + message: "Incremental scan failed with 3 errors:\nembedding failure\nupsert failure", + }) + await result.catch((error: AggregateError) => { + for (const [index, failure] of failures.entries()) expect(error.errors[index]).toBe(failure) + }) }) }) diff --git a/src/services/code-index/__tests__/manager.spec.ts b/src/services/code-index/__tests__/manager.spec.ts index 952e205fdd..5038cc247e 100644 --- a/src/services/code-index/__tests__/manager.spec.ts +++ b/src/services/code-index/__tests__/manager.spec.ts @@ -3,6 +3,8 @@ import { makeExtensionContext } from "../../../test-utils/vscode" import { CodeIndexManager } from "../manager" import { CodeIndexManagerRegistry } from "../code-index-manager-registry" import { CodeIndexServiceFactory } from "../service-factory" +import { CodeIndexOrchestrator } from "../orchestrator" +import { CodeIndexSearchService } from "../search-service" import type { MockedClass } from "vitest" import * as path from "path" import { providerIdentifiers } from "@roo-code/types/provider-identifiers" @@ -474,63 +476,19 @@ describe("CodeIndexManager - handleSettingsChange regression", () => { ;(manager as any)._configManager = mockConfigManager }) - it("should validate embedder during _recreateServices when validation succeeds", async () => { - // Arrange - mockServiceFactoryInstance.validateEmbedder.mockResolvedValue({ valid: true }) - - // Act - directly call the private method for testing - await (manager as any)._recreateServices() - - // Assert - expect(mockServiceFactoryInstance.createServices).toHaveBeenCalled() - const createdEmbedder = mockServiceFactoryInstance.createServices.mock.results[0].value.embedder - expect(mockServiceFactoryInstance.validateEmbedder).toHaveBeenCalledWith(createdEmbedder) - expect(mockStateManager.setSystemState).not.toHaveBeenCalledWith("Error", expect.any(String)) - }) - - it("should set error state when embedder validation fails", async () => { - // Arrange + it("should create indexing services without a startup embedder validation request", async () => { mockServiceFactoryInstance.validateEmbedder.mockResolvedValue({ valid: false, - error: "embeddings:validation.authenticationFailed", + error: "Embedder unavailable", }) - // Act & Assert - await expect((manager as any)._recreateServices()).rejects.toThrow( - "embeddings:validation.authenticationFailed", - ) + await manager["_recreateServices"]() - // Assert other expectations expect(mockServiceFactoryInstance.createServices).toHaveBeenCalled() - const createdEmbedder = mockServiceFactoryInstance.createServices.mock.results[0].value.embedder - expect(mockServiceFactoryInstance.validateEmbedder).toHaveBeenCalledWith(createdEmbedder) - expect(mockStateManager.setSystemState).toHaveBeenCalledWith( - "Error", - "embeddings:validation.authenticationFailed", - ) - }) - - it("should set generic error state when embedder validation throws", async () => { - // Arrange - // Since the real service factory catches exceptions, we should mock it to resolve with an error - mockServiceFactoryInstance.validateEmbedder.mockResolvedValue({ - valid: false, - error: "embeddings:validation.configurationError", - }) - - // Act & Assert - await expect((manager as any)._recreateServices()).rejects.toThrow( - "embeddings:validation.configurationError", - ) - - // Assert other expectations - expect(mockServiceFactoryInstance.createServices).toHaveBeenCalled() - const createdEmbedder = mockServiceFactoryInstance.createServices.mock.results[0].value.embedder - expect(mockServiceFactoryInstance.validateEmbedder).toHaveBeenCalledWith(createdEmbedder) - expect(mockStateManager.setSystemState).toHaveBeenCalledWith( - "Error", - "embeddings:validation.configurationError", - ) + expect(mockServiceFactoryInstance.validateEmbedder).not.toHaveBeenCalled() + expect(mockStateManager.setSystemState).not.toHaveBeenCalledWith("Error", expect.any(String)) + expect(manager["_orchestrator"]).toBeInstanceOf(CodeIndexOrchestrator) + expect(manager["_searchService"]).toBeInstanceOf(CodeIndexSearchService) }) it("should handle embedder creation failure", async () => { @@ -684,7 +642,7 @@ describe("CodeIndexManager - handleSettingsChange regression", () => { // Assert - manager should be initialized again expect(manager.isInitialized).toBe(true) expect(mockServiceFactoryInstance.createServices).toHaveBeenCalled() - expect(mockServiceFactoryInstance.validateEmbedder).toHaveBeenCalled() + expect(mockServiceFactoryInstance.validateEmbedder).not.toHaveBeenCalled() }) it("should be safe to call when not in error state (idempotent)", async () => { diff --git a/src/services/code-index/__tests__/orchestrator.spec.ts b/src/services/code-index/__tests__/orchestrator.spec.ts index bad3d69be3..1a81c1abfa 100644 --- a/src/services/code-index/__tests__/orchestrator.spec.ts +++ b/src/services/code-index/__tests__/orchestrator.spec.ts @@ -1,5 +1,7 @@ import { describe, it, expect, beforeEach, vi } from "vitest" import { CodeIndexOrchestrator } from "../orchestrator" +import { TelemetryService } from "@roo-code/telemetry" +import { TelemetryEventName } from "@roo-code/types" import { clearAllMocks } from "../../../test-utils/reset" @@ -42,8 +44,8 @@ vi.mock("@roo-code/telemetry", () => ({ })) // Mock i18n translator used in orchestrator messages -vi.mock("../../i18n", () => ({ - t: (key: string, params?: any) => { +vi.mock("../../../i18n", () => ({ + t: (key: string, params?: { errorMessage?: string }) => { if (key === "embeddings:orchestrator.failedDuringInitialScan" && params?.errorMessage) { return `Failed during initial scan: ${params.errorMessage}` } @@ -200,12 +202,150 @@ describe("CodeIndexOrchestrator - error path cleanup gating", () => { expect(lastCall[0]).toBe("Error") }) - it("should call clearCollection() and clear cache when an error occurs after initialize() succeeds (indexing started)", async () => { - // Arrange: initialize succeeds; fail soon after to enter error path with indexingStarted=true - vectorStore.initialize.mockResolvedValue(false) // existing collection - vectorStore.hasIndexedData.mockResolvedValue(false) // force full scan path - vectorStore.markIndexingIncomplete.mockRejectedValue(new Error("mark incomplete failure")) + it.each([false, true])( + "only cleans up a failed full scan when the collection was created by this run: %s", + async (created) => { + vectorStore.initialize.mockResolvedValue(created) + vectorStore.hasIndexedData.mockResolvedValue(false) // force full scan path + vectorStore.markIndexingIncomplete.mockRejectedValue(new Error("mark incomplete failure")) + + const orchestrator = new CodeIndexOrchestrator( + configManager, + stateManager, + workspacePath, + cacheManager, + vectorStore, + scanner, + fileWatcher, + ) + + // Act + await orchestrator.startIndexing() + + expect(vectorStore.clearCollection).toHaveBeenCalledTimes(created ? 1 : 0) + // A new collection clears stale cache at initialization and again on failure. + expect(cacheManager.clearCacheFile).toHaveBeenCalledTimes(created ? 2 : 0) + + // Error state should be set + expect(stateManager.setSystemState).toHaveBeenCalled() + const lastCall = stateManager.setSystemState.mock.calls[stateManager.setSystemState.mock.calls.length - 1] + expect(lastCall[0]).toBe("Error") + }, + ) + + it.each([false, true])( + "only cleans up after partial full-scan progress when the collection was created by this run: %s", + async (created) => { + const failure = new Error("batch failure after partial progress") + vectorStore.initialize.mockResolvedValue(created) + vectorStore.hasIndexedData.mockResolvedValue(false) + vectorStore.markIndexingIncomplete.mockResolvedValue(undefined) + scanner.scanDirectory.mockImplementation( + async ( + _dir: string, + onError: (error: Error) => void, + onIndexed: (count: number) => void, + onParsed: (count: number) => void, + ) => { + onParsed(3) + onIndexed(1) + onError(failure) + return { stats: { processed: 1, skipped: 0 }, totalBlockCount: 3 } + }, + ) + const orchestrator = new CodeIndexOrchestrator( + configManager, + stateManager, + workspacePath, + cacheManager, + vectorStore, + scanner, + fileWatcher, + ) + + await orchestrator.startIndexing() + + expect(scanner.scanDirectory).toHaveBeenCalledOnce() + expect(stateManager.reportBlockIndexingProgress).toHaveBeenLastCalledWith(1, 3) + expect(stateManager.setSystemState).toHaveBeenLastCalledWith( + "Error", + expect.stringContaining(failure.message), + ) + expect(stateManager.setSystemState).not.toHaveBeenCalledWith("Indexed", expect.any(String)) + expect(vectorStore.clearCollection).toHaveBeenCalledTimes(created ? 1 : 0) + // New collections clear stale cache before scanning and again during error cleanup. + expect(cacheManager.clearCacheFile).toHaveBeenCalledTimes(created ? 2 : 0) + expect(vectorStore.markIndexingIncomplete).toHaveBeenCalledOnce() + expect(vectorStore.markIndexingComplete).not.toHaveBeenCalled() + expect(fileWatcher.initialize).not.toHaveBeenCalled() + expect(fileWatcher.dispose).toHaveBeenCalledOnce() + }, + ) + + it.each([false, true])( + "keeps private error details out of telemetry (cleanup failure: %s)", + async (cleanupFails) => { + const messages = [ + "Embedding failed (Workspace: /Users/private-user/Secret Project, File: /Users/private-user/Secret Project/src/private.ts)", + "Embedding failed (Workspace: C:\\Users\\private-user\\Secret Project, File: C:\\Users\\private-user\\Secret Project\\private.ts)", + ] + const failures = messages.map((message) => { + const error = new Error(message) + error.stack = `${message}\n at privateFunction (/Users/private-user/Secret Project/private.ts:12:3)` + return error + }) + vectorStore.initialize.mockResolvedValue(cleanupFails) + vectorStore.hasIndexedData.mockResolvedValue(!cleanupFails) + vectorStore.markIndexingIncomplete.mockResolvedValue(undefined) + scanner.scanDirectory.mockImplementation(async (_dir: string, onError: (error: Error) => void) => { + for (const failure of failures) onError(failure) + return { stats: { processed: 0, skipped: 0 }, totalBlockCount: 0 } + }) + if (cleanupFails) vectorStore.clearCollection.mockRejectedValue(failures[1]) + const orchestrator = new CodeIndexOrchestrator( + configManager, + stateManager, + workspacePath, + cacheManager, + vectorStore, + scanner, + fileWatcher, + ) + + await orchestrator.startIndexing() + + const expectedEvents = [ + [TelemetryEventName.CODE_INDEX_ERROR, { error: "Indexing failed", location: "startIndexing" }], + ] + if (cleanupFails) { + expectedEvents.push([ + TelemetryEventName.CODE_INDEX_ERROR, + { error: "Index cleanup failed", location: "startIndexing.cleanup" }, + ]) + } + // Exact payload assertions also exclude stacks, nested causes and aggregate error objects. + expect(vi.mocked(TelemetryService.instance.captureEvent).mock.calls).toEqual(expectedEvents) + expect(stateManager.setSystemState).toHaveBeenLastCalledWith("Error", expect.stringContaining(messages[0])) + if (!cleanupFails) { + expect(stateManager.setSystemState).toHaveBeenLastCalledWith( + "Error", + expect.stringContaining(messages[1]), + ) + } + }, + ) + it("preserves an existing index after an incremental failure and a failed full-scan retry", async () => { + let complete = true + vectorStore.initialize.mockResolvedValue(false) + vectorStore.hasIndexedData.mockImplementation(async () => complete) + vectorStore.markIndexingIncomplete.mockImplementation(async () => { + complete = false + }) + scanner.scanDirectory.mockImplementation(async (_dir: string, onError: (error: Error) => void) => { + onError(new Error("embedding failed")) + return { stats: { processed: 0, skipped: 0 }, totalBlockCount: 0 } + }) const orchestrator = new CodeIndexOrchestrator( configManager, stateManager, @@ -216,17 +356,23 @@ describe("CodeIndexOrchestrator - error path cleanup gating", () => { fileWatcher, ) - // Act - await orchestrator.startIndexing() - - // Assert: cleanup gated behind indexingStarted should have happened - expect(vectorStore.clearCollection).toHaveBeenCalledTimes(1) - expect(cacheManager.clearCacheFile).toHaveBeenCalledTimes(1) - - // Error state should be set - expect(stateManager.setSystemState).toHaveBeenCalled() - const lastCall = stateManager.setSystemState.mock.calls[stateManager.setSystemState.mock.calls.length - 1] - expect(lastCall[0]).toBe("Error") + for (let attempt = 1; attempt <= 2; attempt++) { + await orchestrator.startIndexing() + expect(complete).toBe(false) + expect(orchestrator.state).toBe("Error") + expect(scanner.scanDirectory).toHaveBeenCalledTimes(attempt) + expect(vectorStore.markIndexingIncomplete).toHaveBeenCalledTimes(attempt) + expect(vectorStore.clearCollection).not.toHaveBeenCalled() + expect(cacheManager.clearCacheFile).not.toHaveBeenCalled() + expect(vectorStore.markIndexingComplete).not.toHaveBeenCalled() + expect(fileWatcher.initialize).not.toHaveBeenCalled() + } + expect(stateManager.setSystemState).toHaveBeenCalledWith("Indexing", "Checking for new or modified files...") + expect(stateManager.setSystemState).toHaveBeenCalledWith( + "Indexing", + "Services ready. Starting workspace scan...", + ) + expect(stateManager.setSystemState).not.toHaveBeenCalledWith("Indexed", expect.any(String)) }) it("collects batch errors from full scan and transitions to Error when all blocks fail", async () => { @@ -259,36 +405,49 @@ describe("CodeIndexOrchestrator - error path cleanup gating", () => { expect(calls[calls.length - 1]).toBe("Error") }) - it("collects batch errors from incremental scan and still completes indexing", async () => { - const batchError = new Error("incremental batch failure") - vectorStore.initialize.mockResolvedValue(false) // existing collection - vectorStore.hasIndexedData.mockResolvedValue(true) // force incremental scan path - vectorStore.markIndexingIncomplete.mockResolvedValue(undefined) - vectorStore.markIndexingComplete.mockResolvedValue(undefined) - - // Incremental scan reports a batch error but returns a result — orchestrator completes normally - scanner.scanDirectory.mockImplementation(async (_dir: string, onBatchError: (e: Error) => void) => { - onBatchError(batchError) - return { stats: { processed: 0, skipped: 0 }, totalBlockCount: 0 } - }) + it.each([0, 2])( + "preserves the existing index on incremental batch failure after %s indexed blocks", + async (indexed) => { + const batchError = new Error("incremental batch failure") + vectorStore.initialize.mockResolvedValue(false) // existing collection + vectorStore.hasIndexedData.mockResolvedValue(true) // force incremental scan path + vectorStore.markIndexingIncomplete.mockResolvedValue(undefined) + vectorStore.markIndexingComplete.mockResolvedValue(undefined) + + scanner.scanDirectory.mockImplementation( + async (_dir: string, onBatchError: (e: Error) => void, onIndexed: (count: number) => void) => { + onIndexed(indexed) + onBatchError(batchError) + return { stats: { processed: 1, skipped: 0 }, totalBlockCount: 3 } + }, + ) - const orchestrator = new CodeIndexOrchestrator( - configManager, - stateManager, - workspacePath, - cacheManager, - vectorStore, - scanner, - fileWatcher, - ) + const orchestrator = new CodeIndexOrchestrator( + configManager, + stateManager, + workspacePath, + cacheManager, + vectorStore, + scanner, + fileWatcher, + ) - await orchestrator.startIndexing() + await orchestrator.startIndexing() - // Incremental scan doesn't gate on batch errors — Indexed state is still reached - const calls = stateManager.setSystemState.mock.calls.map((c: any[]) => c[0]) - expect(calls[calls.length - 1]).toBe("Indexed") - expect(calls).not.toContain("Error") - }) + expect(orchestrator.state).toBe("Error") + expect(stateManager.setSystemState).toHaveBeenLastCalledWith( + "Error", + expect.stringContaining(batchError.message), + ) + expect(stateManager.setSystemState).not.toHaveBeenCalledWith("Indexed", expect.any(String)) + expect(vectorStore.markIndexingIncomplete).toHaveBeenCalledOnce() + expect(vectorStore.markIndexingComplete).not.toHaveBeenCalled() + expect(vectorStore.clearCollection).not.toHaveBeenCalled() + expect(cacheManager.clearCacheFile).not.toHaveBeenCalled() + expect(fileWatcher.initialize).not.toHaveBeenCalled() + expect(fileWatcher.dispose).toHaveBeenCalledOnce() + }, + ) }) describe("CodeIndexOrchestrator - stopIndexing", () => { diff --git a/src/services/code-index/code-index-scan-executor.ts b/src/services/code-index/code-index-scan-executor.ts index 2ff55454d9..54760c50f2 100644 --- a/src/services/code-index/code-index-scan-executor.ts +++ b/src/services/code-index/code-index-scan-executor.ts @@ -20,7 +20,15 @@ export class CodeIndexScanExecutor { const summary = await this.scanWorkspace(signal, "incremental") if (!summary) return false - // Preserve the existing incremental policy: reported batch errors do not prevent completion. + // Do not mark an existing index complete when some updates failed. + if (summary.batchErrors.length > 0) { + const messages = [...new Set(summary.batchErrors.map((error) => error.message))] + throw new AggregateError( + summary.batchErrors, + `Incremental scan failed with ${summary.batchErrors.length} errors:\n${messages.join("\n")}`, + ) + } + if (summary.found > 0) { console.log( `[CodeIndexOrchestrator] Incremental scan completed: ${summary.indexed} blocks indexed from new/changed files`, diff --git a/src/services/code-index/manager.ts b/src/services/code-index/manager.ts index bb186dc105..aa382e2131 100644 --- a/src/services/code-index/manager.ts +++ b/src/services/code-index/manager.ts @@ -416,14 +416,6 @@ export class CodeIndexManager { rooIgnoreController, ) - // Validate embedder configuration before proceeding - const validationResult = await this._serviceFactory.validateEmbedder(embedder) - if (!validationResult.valid) { - const errorMessage = validationResult.error || "Embedder configuration validation failed" - this._stateManager.setSystemState("Error", errorMessage) - throw new Error(errorMessage) - } - // (Re)Initialize orchestrator this._orchestrator = new CodeIndexOrchestrator( this._configManager!, diff --git a/src/services/code-index/orchestrator.ts b/src/services/code-index/orchestrator.ts index c055fff370..8d58b7fb8b 100644 --- a/src/services/code-index/orchestrator.ts +++ b/src/services/code-index/orchestrator.ts @@ -130,15 +130,13 @@ export class CodeIndexOrchestrator { const signal = this._abortController.signal this.stateManager.setSystemState("Indexing", "Initializing services...") - // Track whether we successfully connected to Qdrant and started indexing - // This helps us decide whether to preserve cache on error - let indexingStarted = false + // Only clean up collections created by this run; existing data must survive failed retries. + let clearIndexOnError = false try { const collectionCreated = await this.vectorStore.initialize() - // Successfully connected to Qdrant - indexingStarted = true + clearIndexOnError = collectionCreated if (collectionCreated) { await this.cacheManager.clearCacheFile() @@ -188,37 +186,29 @@ export class CodeIndexOrchestrator { } console.error("[CodeIndexOrchestrator] Error during indexing:", error) + // Scanner/provider errors and stacks may contain local paths or other private data. + // Keep details local; send only a fixed failure category across the telemetry boundary. TelemetryService.instance.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { - error: error instanceof Error ? error.message : String(error), - stack: error instanceof Error ? error.stack : undefined, + error: "Indexing failed", location: "startIndexing", }) - if (indexingStarted) { + if (clearIndexOnError) { try { await this.vectorStore.clearCollection() } catch (cleanupError) { console.error("[CodeIndexOrchestrator] Failed to clean up after error:", cleanupError) TelemetryService.instance.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { - error: cleanupError instanceof Error ? cleanupError.message : String(cleanupError), - stack: cleanupError instanceof Error ? cleanupError.stack : undefined, + error: "Index cleanup failed", location: "startIndexing.cleanup", }) } - } - - // Only clear cache if indexing had started (Qdrant connection succeeded) - // If we never connected to Qdrant, preserve cache for incremental scan when it comes back - if (indexingStarted) { // Indexing started but failed mid-way - clear cache to avoid cache-Qdrant mismatch await this.cacheManager.clearCacheFile() console.log( "[CodeIndexOrchestrator] Indexing failed after starting. Clearing cache to avoid inconsistency.", ) } else { - // Never connected to Qdrant - preserve cache for future incremental scan - console.log( - "[CodeIndexOrchestrator] Failed to connect to Qdrant. Preserving cache for future incremental scan.", - ) + console.log("[CodeIndexOrchestrator] Preserving existing index and cache for a retry.") } this.stateManager.setSystemState(