mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-10-08 03:07:53 +00:00
fix: improve deletePointsByMultipleFilePaths error handling to prevent VS Code crashes
- Add request size limits (MAX_DELETE_PATHS_PER_REQUEST = 100) to prevent oversized requests - Implement retry logic with exponential backoff for failed deletion operations - Add comprehensive error handling with contextual error messages - Include telemetry tracking for monitoring deletion failures - Add chunking support to handle large file path arrays - Prevent crash loops by gracefully handling Qdrant API failures - Add comprehensive test coverage for new error handling and retry logic Fixes #5857
This commit is contained in:
Roo Code
parent
a28d50e20c
commit
b8bc3060d4
3 changed files with 358 additions and 23 deletions
|
|
@ -21,6 +21,9 @@ export const MAX_BATCH_RETRIES = 3
|
|||
export const INITIAL_RETRY_DELAY_MS = 500
|
||||
export const PARSING_CONCURRENCY = 10
|
||||
|
||||
/**Vector Store */
|
||||
export const MAX_DELETE_PATHS_PER_REQUEST = 100 // Maximum number of file paths to delete in a single request
|
||||
|
||||
/**OpenAI Embedder */
|
||||
export const MAX_BATCH_TOKENS = 100000
|
||||
export const MAX_ITEM_TOKENS = 8191
|
||||
|
|
|
|||
|
|
@ -3,7 +3,7 @@ import { createHash } from "crypto"
|
|||
|
||||
import { QdrantVectorStore } from "../qdrant-client"
|
||||
import { getWorkspacePath } from "../../../../utils/path"
|
||||
import { DEFAULT_MAX_SEARCH_RESULTS, DEFAULT_SEARCH_MIN_SCORE } from "../../constants"
|
||||
import { DEFAULT_MAX_SEARCH_RESULTS, DEFAULT_SEARCH_MIN_SCORE, MAX_DELETE_PATHS_PER_REQUEST, MAX_BATCH_RETRIES, INITIAL_RETRY_DELAY_MS } from "../../constants"
|
||||
|
||||
// Mocks
|
||||
vitest.mock("@qdrant/js-client-rest")
|
||||
|
|
@ -1527,4 +1527,270 @@ describe("QdrantVectorStore", () => {
|
|||
expect(callArgs.score_threshold).toBe(DEFAULT_SEARCH_MIN_SCORE)
|
||||
})
|
||||
})
|
||||
|
||||
describe("deletePointsByMultipleFilePaths", () => {
|
||||
beforeEach(() => {
|
||||
// Mock TelemetryService
|
||||
vitest.mock("@roo-code/telemetry", () => ({
|
||||
TelemetryService: {
|
||||
instance: {
|
||||
captureEvent: vitest.fn(),
|
||||
},
|
||||
},
|
||||
}))
|
||||
vitest.mock("@roo-code/types", () => ({
|
||||
TelemetryEventName: {
|
||||
CODE_INDEX_ERROR: "code_index_error",
|
||||
},
|
||||
}))
|
||||
vitest.mock("../shared/validation-helpers", () => ({
|
||||
sanitizeErrorMessage: vitest.fn((msg) => msg),
|
||||
}))
|
||||
})
|
||||
|
||||
it("should handle empty file paths array", async () => {
|
||||
await vectorStore.deletePointsByMultipleFilePaths([])
|
||||
|
||||
expect(mockQdrantClientInstance.delete).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("should successfully delete points for single file path", async () => {
|
||||
const filePaths = ["src/test.ts"]
|
||||
mockQdrantClientInstance.delete.mockResolvedValue({} as any)
|
||||
|
||||
await vectorStore.deletePointsByMultipleFilePaths(filePaths)
|
||||
|
||||
expect(mockQdrantClientInstance.delete).toHaveBeenCalledTimes(1)
|
||||
expect(mockQdrantClientInstance.delete).toHaveBeenCalledWith(expectedCollectionName, {
|
||||
filter: {
|
||||
should: [
|
||||
{
|
||||
key: "filePath",
|
||||
match: {
|
||||
value: expect.stringContaining("src/test.ts"),
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
wait: true,
|
||||
})
|
||||
})
|
||||
|
||||
it("should successfully delete points for multiple file paths within chunk limit", async () => {
|
||||
const filePaths = ["src/test1.ts", "src/test2.ts", "src/test3.ts"]
|
||||
mockQdrantClientInstance.delete.mockResolvedValue({} as any)
|
||||
|
||||
await vectorStore.deletePointsByMultipleFilePaths(filePaths)
|
||||
|
||||
expect(mockQdrantClientInstance.delete).toHaveBeenCalledTimes(1)
|
||||
expect(mockQdrantClientInstance.delete).toHaveBeenCalledWith(expectedCollectionName, {
|
||||
filter: {
|
||||
should: expect.arrayContaining([
|
||||
{
|
||||
key: "filePath",
|
||||
match: {
|
||||
value: expect.stringContaining("src/test1.ts"),
|
||||
},
|
||||
},
|
||||
{
|
||||
key: "filePath",
|
||||
match: {
|
||||
value: expect.stringContaining("src/test2.ts"),
|
||||
},
|
||||
},
|
||||
{
|
||||
key: "filePath",
|
||||
match: {
|
||||
value: expect.stringContaining("src/test3.ts"),
|
||||
},
|
||||
},
|
||||
]),
|
||||
},
|
||||
wait: true,
|
||||
})
|
||||
})
|
||||
|
||||
it("should chunk large file path arrays into multiple requests", async () => {
|
||||
// Create an array larger than MAX_DELETE_PATHS_PER_REQUEST
|
||||
const filePaths = Array.from({ length: MAX_DELETE_PATHS_PER_REQUEST + 50 }, (_, i) => `src/test${i}.ts`)
|
||||
mockQdrantClientInstance.delete.mockResolvedValue({} as any)
|
||||
|
||||
await vectorStore.deletePointsByMultipleFilePaths(filePaths)
|
||||
|
||||
// Should be called twice: once for the first chunk, once for the remainder
|
||||
expect(mockQdrantClientInstance.delete).toHaveBeenCalledTimes(2)
|
||||
|
||||
// First call should have MAX_DELETE_PATHS_PER_REQUEST items
|
||||
const firstCall = mockQdrantClientInstance.delete.mock.calls[0][1]
|
||||
expect(firstCall.filter.should).toHaveLength(MAX_DELETE_PATHS_PER_REQUEST)
|
||||
|
||||
// Second call should have the remaining 50 items
|
||||
const secondCall = mockQdrantClientInstance.delete.mock.calls[1][1]
|
||||
expect(secondCall.filter.should).toHaveLength(50)
|
||||
})
|
||||
|
||||
it("should retry on failure and eventually succeed", async () => {
|
||||
const filePaths = ["src/test.ts"]
|
||||
const deleteError = new Error("Temporary Qdrant error")
|
||||
|
||||
// Fail twice, then succeed
|
||||
mockQdrantClientInstance.delete
|
||||
.mockRejectedValueOnce(deleteError)
|
||||
.mockRejectedValueOnce(deleteError)
|
||||
.mockResolvedValueOnce({} as any)
|
||||
|
||||
vitest.spyOn(console, "warn").mockImplementation(() => {})
|
||||
|
||||
await vectorStore.deletePointsByMultipleFilePaths(filePaths)
|
||||
|
||||
expect(mockQdrantClientInstance.delete).toHaveBeenCalledTimes(3)
|
||||
expect(console.warn).toHaveBeenCalledTimes(2) // Two warnings for the failed attempts
|
||||
;(console.warn as any).mockRestore()
|
||||
})
|
||||
|
||||
it("should throw error after exhausting all retries", async () => {
|
||||
const filePaths = ["src/test.ts"]
|
||||
const deleteError = new Error("Persistent Qdrant error")
|
||||
|
||||
// Fail all retry attempts
|
||||
mockQdrantClientInstance.delete.mockRejectedValue(deleteError)
|
||||
|
||||
vitest.spyOn(console, "warn").mockImplementation(() => {})
|
||||
vitest.spyOn(console, "error").mockImplementation(() => {})
|
||||
|
||||
await expect(vectorStore.deletePointsByMultipleFilePaths(filePaths)).rejects.toThrow(
|
||||
/Failed to delete points for 1 file paths after 3 retries/,
|
||||
)
|
||||
|
||||
expect(mockQdrantClientInstance.delete).toHaveBeenCalledTimes(MAX_BATCH_RETRIES)
|
||||
expect(console.warn).toHaveBeenCalledTimes(MAX_BATCH_RETRIES)
|
||||
expect(console.error).toHaveBeenCalledTimes(1)
|
||||
;(console.warn as any).mockRestore()
|
||||
;(console.error as any).mockRestore()
|
||||
})
|
||||
|
||||
it("should handle mixed success and failure across chunks", async () => {
|
||||
// Create an array that will be split into 2 chunks
|
||||
const filePaths = Array.from({ length: MAX_DELETE_PATHS_PER_REQUEST + 10 }, (_, i) => `src/test${i}.ts`)
|
||||
const deleteError = new Error("Chunk 2 error")
|
||||
|
||||
// First chunk succeeds, second chunk fails
|
||||
mockQdrantClientInstance.delete
|
||||
.mockResolvedValueOnce({} as any) // First chunk succeeds
|
||||
.mockRejectedValue(deleteError) // Second chunk fails all retries
|
||||
|
||||
vitest.spyOn(console, "warn").mockImplementation(() => {})
|
||||
vitest.spyOn(console, "error").mockImplementation(() => {})
|
||||
|
||||
await expect(vectorStore.deletePointsByMultipleFilePaths(filePaths)).rejects.toThrow(
|
||||
/Failed to delete points for 10 file paths after 3 retries. Chunk 2\/2/,
|
||||
)
|
||||
|
||||
// First chunk: 1 call, Second chunk: MAX_BATCH_RETRIES calls
|
||||
expect(mockQdrantClientInstance.delete).toHaveBeenCalledTimes(1 + MAX_BATCH_RETRIES)
|
||||
;(console.warn as any).mockRestore()
|
||||
;(console.error as any).mockRestore()
|
||||
})
|
||||
|
||||
it("should use exponential backoff for retries", async () => {
|
||||
const filePaths = ["src/test.ts"]
|
||||
const deleteError = new Error("Temporary error")
|
||||
|
||||
// Fail twice, then succeed
|
||||
mockQdrantClientInstance.delete
|
||||
.mockRejectedValueOnce(deleteError)
|
||||
.mockRejectedValueOnce(deleteError)
|
||||
.mockResolvedValueOnce({} as any)
|
||||
|
||||
vitest.spyOn(console, "warn").mockImplementation(() => {})
|
||||
const setTimeoutSpy = vitest.spyOn(global, "setTimeout").mockImplementation((fn: any) => {
|
||||
fn() // Execute immediately for testing
|
||||
return {} as any
|
||||
})
|
||||
|
||||
await vectorStore.deletePointsByMultipleFilePaths(filePaths)
|
||||
|
||||
// Should have called setTimeout twice for the two retries
|
||||
expect(setTimeoutSpy).toHaveBeenCalledTimes(2)
|
||||
|
||||
// First retry: INITIAL_RETRY_DELAY_MS * 2^0 = 500ms
|
||||
expect(setTimeoutSpy).toHaveBeenNthCalledWith(1, expect.any(Function), INITIAL_RETRY_DELAY_MS)
|
||||
|
||||
// Second retry: INITIAL_RETRY_DELAY_MS * 2^1 = 1000ms
|
||||
expect(setTimeoutSpy).toHaveBeenNthCalledWith(2, expect.any(Function), INITIAL_RETRY_DELAY_MS * 2)
|
||||
|
||||
setTimeoutSpy.mockRestore()
|
||||
;(console.warn as any).mockRestore()
|
||||
})
|
||||
|
||||
it("should normalize file paths correctly", async () => {
|
||||
const filePaths = ["./src/test.ts", "src/../src/test2.ts"]
|
||||
mockQdrantClientInstance.delete.mockResolvedValue({} as any)
|
||||
|
||||
await vectorStore.deletePointsByMultipleFilePaths(filePaths)
|
||||
|
||||
expect(mockQdrantClientInstance.delete).toHaveBeenCalledTimes(1)
|
||||
const deleteCall = mockQdrantClientInstance.delete.mock.calls[0][1]
|
||||
|
||||
// Both paths should be normalized to absolute paths
|
||||
expect(deleteCall.filter.should).toHaveLength(2)
|
||||
deleteCall.filter.should.forEach((filter: any) => {
|
||||
expect(filter.match.value).toMatch(/^\/.*\/src\/test.*\.ts$/) // Should be absolute paths
|
||||
})
|
||||
})
|
||||
|
||||
it("should capture telemetry events for errors", async () => {
|
||||
const { TelemetryService } = await import("@roo-code/telemetry")
|
||||
const { TelemetryEventName } = await import("@roo-code/types")
|
||||
|
||||
const filePaths = ["src/test.ts"]
|
||||
const deleteError = new Error("Telemetry test error")
|
||||
|
||||
mockQdrantClientInstance.delete.mockRejectedValue(deleteError)
|
||||
vitest.spyOn(console, "warn").mockImplementation(() => {})
|
||||
vitest.spyOn(console, "error").mockImplementation(() => {})
|
||||
|
||||
await expect(vectorStore.deletePointsByMultipleFilePaths(filePaths)).rejects.toThrow()
|
||||
|
||||
// Should capture telemetry for each retry attempt plus final failure
|
||||
expect(TelemetryService.instance.captureEvent).toHaveBeenCalledWith(
|
||||
TelemetryEventName.CODE_INDEX_ERROR,
|
||||
expect.objectContaining({
|
||||
error: "Telemetry test error",
|
||||
location: "deletePointsByMultipleFilePaths",
|
||||
errorType: "deletion_error",
|
||||
retryCount: expect.any(Number),
|
||||
chunkIndex: 0,
|
||||
chunkSize: 1,
|
||||
totalChunks: 1,
|
||||
}),
|
||||
)
|
||||
|
||||
// Should also capture final retry exhausted event
|
||||
expect(TelemetryService.instance.captureEvent).toHaveBeenCalledWith(
|
||||
TelemetryEventName.CODE_INDEX_ERROR,
|
||||
expect.objectContaining({
|
||||
errorType: "deletion_retry_exhausted",
|
||||
retryCount: MAX_BATCH_RETRIES,
|
||||
}),
|
||||
)
|
||||
|
||||
;(console.warn as any).mockRestore()
|
||||
;(console.error as any).mockRestore()
|
||||
})
|
||||
})
|
||||
|
||||
describe("deletePointsByFilePath", () => {
|
||||
it("should delegate to deletePointsByMultipleFilePaths", async () => {
|
||||
const filePath = "src/test.ts"
|
||||
const deleteMultipleSpy = vitest.spyOn(vectorStore, "deletePointsByMultipleFilePaths").mockResolvedValue()
|
||||
|
||||
await vectorStore.deletePointsByFilePath(filePath)
|
||||
|
||||
expect(deleteMultipleSpy).toHaveBeenCalledTimes(1)
|
||||
expect(deleteMultipleSpy).toHaveBeenCalledWith([filePath])
|
||||
|
||||
deleteMultipleSpy.mockRestore()
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -4,8 +4,11 @@ import * as path from "path"
|
|||
import { getWorkspacePath } from "../../../utils/path"
|
||||
import { IVectorStore } from "../interfaces/vector-store"
|
||||
import { Payload, VectorStoreSearchResult } from "../interfaces"
|
||||
import { DEFAULT_MAX_SEARCH_RESULTS, DEFAULT_SEARCH_MIN_SCORE } from "../constants"
|
||||
import { DEFAULT_MAX_SEARCH_RESULTS, DEFAULT_SEARCH_MIN_SCORE, MAX_DELETE_PATHS_PER_REQUEST, MAX_BATCH_RETRIES, INITIAL_RETRY_DELAY_MS } from "../constants"
|
||||
import { t } from "../../../i18n"
|
||||
import { TelemetryService } from "@roo-code/telemetry"
|
||||
import { TelemetryEventName } from "@roo-code/types"
|
||||
import { sanitizeErrorMessage } from "../shared/validation-helpers"
|
||||
|
||||
/**
|
||||
* Qdrant implementation of the vector store interface
|
||||
|
|
@ -422,29 +425,92 @@ export class QdrantVectorStore implements IVectorStore {
|
|||
return
|
||||
}
|
||||
|
||||
try {
|
||||
const workspaceRoot = getWorkspacePath()
|
||||
const normalizedPaths = filePaths.map((filePath) => {
|
||||
const absolutePath = path.resolve(workspaceRoot, filePath)
|
||||
return path.normalize(absolutePath)
|
||||
})
|
||||
const workspaceRoot = getWorkspacePath()
|
||||
const normalizedPaths = filePaths.map((filePath) => {
|
||||
const absolutePath = path.resolve(workspaceRoot, filePath)
|
||||
return path.normalize(absolutePath)
|
||||
})
|
||||
|
||||
const filter = {
|
||||
should: normalizedPaths.map((normalizedPath) => ({
|
||||
key: "filePath",
|
||||
match: {
|
||||
value: normalizedPath,
|
||||
},
|
||||
})),
|
||||
// Process in chunks to avoid oversized requests
|
||||
const chunks = []
|
||||
for (let i = 0; i < normalizedPaths.length; i += MAX_DELETE_PATHS_PER_REQUEST) {
|
||||
chunks.push(normalizedPaths.slice(i, i + MAX_DELETE_PATHS_PER_REQUEST))
|
||||
}
|
||||
|
||||
// Process each chunk with retry logic
|
||||
for (let chunkIndex = 0; chunkIndex < chunks.length; chunkIndex++) {
|
||||
const chunk = chunks[chunkIndex]
|
||||
let retryCount = 0
|
||||
let lastError: Error | undefined
|
||||
|
||||
while (retryCount < MAX_BATCH_RETRIES) {
|
||||
try {
|
||||
const filter = {
|
||||
should: chunk.map((normalizedPath) => ({
|
||||
key: "filePath",
|
||||
match: {
|
||||
value: normalizedPath,
|
||||
},
|
||||
})),
|
||||
}
|
||||
|
||||
await this.client.delete(this.collectionName, {
|
||||
filter,
|
||||
wait: true,
|
||||
})
|
||||
|
||||
// Success - break out of retry loop
|
||||
break
|
||||
} catch (error) {
|
||||
lastError = error as Error
|
||||
retryCount++
|
||||
|
||||
// Log telemetry for deletion errors
|
||||
TelemetryService.instance.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, {
|
||||
error: sanitizeErrorMessage(lastError.message),
|
||||
location: "deletePointsByMultipleFilePaths",
|
||||
errorType: "deletion_error",
|
||||
retryCount,
|
||||
chunkIndex,
|
||||
chunkSize: chunk.length,
|
||||
totalChunks: chunks.length,
|
||||
})
|
||||
|
||||
console.warn(
|
||||
`[QdrantVectorStore] Failed to delete points for chunk ${chunkIndex + 1}/${chunks.length} (attempt ${retryCount}/${MAX_BATCH_RETRIES}):`,
|
||||
lastError.message,
|
||||
)
|
||||
|
||||
if (retryCount === MAX_BATCH_RETRIES) {
|
||||
// Final retry failed - throw error with context
|
||||
const contextualError = new Error(
|
||||
`Failed to delete points for ${chunk.length} file paths after ${MAX_BATCH_RETRIES} retries. Chunk ${chunkIndex + 1}/${chunks.length}. Original error: ${lastError.message}`,
|
||||
)
|
||||
contextualError.cause = lastError
|
||||
|
||||
// Log final failure
|
||||
TelemetryService.instance.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, {
|
||||
error: sanitizeErrorMessage(contextualError.message),
|
||||
location: "deletePointsByMultipleFilePaths",
|
||||
errorType: "deletion_retry_exhausted",
|
||||
retryCount: MAX_BATCH_RETRIES,
|
||||
chunkIndex,
|
||||
chunkSize: chunk.length,
|
||||
totalChunks: chunks.length,
|
||||
})
|
||||
|
||||
console.error(
|
||||
`[QdrantVectorStore] CRITICAL: Failed to delete points after ${MAX_BATCH_RETRIES} retries for chunk ${chunkIndex + 1}/${chunks.length}:`,
|
||||
contextualError.message,
|
||||
)
|
||||
throw contextualError
|
||||
}
|
||||
|
||||
// Wait before retrying with exponential backoff
|
||||
const delay = INITIAL_RETRY_DELAY_MS * Math.pow(2, retryCount - 1)
|
||||
await new Promise((resolve) => setTimeout(resolve, delay))
|
||||
}
|
||||
}
|
||||
|
||||
await this.client.delete(this.collectionName, {
|
||||
filter,
|
||||
wait: true,
|
||||
})
|
||||
} catch (error) {
|
||||
console.error("Failed to delete points by file paths:", error)
|
||||
throw error
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue