From 48c25ad0380ad92f32221c93423090c2cfb23def Mon Sep 17 00:00:00 2001 From: "detail-app[bot]" <180357370+detail-app[bot]@users.noreply.github.com> Date: Sun, 6 Sep 2026 13:59:20 +0000 Subject: [PATCH] fix(ingestion): prune stale vectors when a previously-ingested file is skipped A previously-ingested file that subsequently exceeded the 1 MiB limit (or became binary/blank) returned skippedFile() before any prior-state cleanup. The runner still added the skipped URL to activeFileUrls, which shielded the stale Qdrant points and local marker from the orphan purger for the rest of the run, leaving chunks that contradicted the file's on-disk bytes reachable by search. The bug was introduced by the initial source-code ingestion pipeline in 499894a, which treated oversized as a pure no-op skip. - prune stale prior state on any Skipped outcome whose URL is present in the pre-run storedFileUrls snapshot, reusing the existing strict prune path (IngestedFilePruneService.pruneCollectionFileStrict(..., null)) - gate the prune so never-ingested oversized files never trigger a Qdrant deleteByUrl round-trip - swallow prune IOExceptions at WARN so a best-effort cleanup never escalates a skip to a failed run - add a class-level CRLF_INJECTION_LOGS SpotBugs exclusion matching the sanitizer pattern already used by GitHubRepoProcessor and LocalDocsFileIngestionProcessor - pin oversized, binary, and blank skip-time prune behavior, the never-ingested and unrelated-URL gates, and the prune-failure-stays-skip contract at the unit boundary; pin the runner interaction where the oversized URL stays active while a genuinely deleted file is still purged --- config/spotbugs/spotbugs-exclude.xml | 8 + .../SourceCodeFileIngestionProcessor.java | 33 ++++ .../cli/GitHubRepoProcessorIsolationTest.java | 27 +++ .../SourceCodeFileIngestionProcessorTest.java | 176 ++++++++++++++++++ 4 files changed, 244 insertions(+) diff --git a/config/spotbugs/spotbugs-exclude.xml b/config/spotbugs/spotbugs-exclude.xml index fd34e59a..75767cc9 100644 --- a/config/spotbugs/spotbugs-exclude.xml +++ b/config/spotbugs/spotbugs-exclude.xml @@ -162,6 +162,14 @@ + + + + + diff --git a/src/main/java/com/williamcallahan/javachat/service/ingestion/SourceCodeFileIngestionProcessor.java b/src/main/java/com/williamcallahan/javachat/service/ingestion/SourceCodeFileIngestionProcessor.java index 7437288d..4f3f72d6 100644 --- a/src/main/java/com/williamcallahan/javachat/service/ingestion/SourceCodeFileIngestionProcessor.java +++ b/src/main/java/com/williamcallahan/javachat/service/ingestion/SourceCodeFileIngestionProcessor.java @@ -123,12 +123,16 @@ public SourceFileProcessingResult process(RepositoryIngestionContext repositoryC Optional validationFailure = validateFileAttributes(sourceFilePath); if (validationFailure.isPresent()) { + if (validationFailure.get() instanceof LocalDocsFileOutcome.Skipped) { + pruneSkipIfPreviouslyIngested(repositoryContext, canonicalCollectionName, fileUrl, sourceFilePath); + } return new SourceFileProcessingResult(validationFailure.get(), fileUrl); } ValidatedFileContext fileContext = buildFileContext(sourceFilePath, repositoryRoot, repositoryMetadata); Optional readableContent = readAndFingerprintFile(sourceFilePath, fileContext); if (readableContent.isEmpty()) { + pruneSkipIfPreviouslyIngested(repositoryContext, canonicalCollectionName, fileUrl, sourceFilePath); return new SourceFileProcessingResult(LocalDocsFileOutcome.skippedFile(), fileUrl); } ReadableFileContent fileContent = readableContent.get(); @@ -254,6 +258,35 @@ private Optional readAndFingerprintFile( } } + /** + * Best-effort prune of stale prior state when a previously-ingested file is now skipped. + * + *

A skip (e.g. oversized, binary, or blank) returns without reindexing. When the file was + * previously ingested, its prior Qdrant chunks and local marker are left behind and would + * otherwise be shielded from orphan purging because the skip still reports the URL as active. + * This strict prune removes the stale vectors and marker so search cannot surface chunks that + * contradict the file's current on-disk bytes. A prune failure is logged and swallowed so the + * skip remains a {@link LocalDocsFileOutcome.Skipped} outcome rather than escalating to failed + * run health over best-effort cleanup.

+ */ + private void pruneSkipIfPreviouslyIngested( + RepositoryIngestionContext repositoryContext, + String canonicalCollectionName, + String fileUrl, + Path sourceFilePath) { + if (!repositoryContext.storedFileUrls().contains(fileUrl)) { + return; + } + try { + ingestedFilePruneService.pruneCollectionFileStrict(canonicalCollectionName, fileUrl, null); + } catch (IOException pruneException) { + log.warn( + "Skip-time prune of formerly-ingested file failed (stale vectors may persist): {}", + renderPathForLog(sourceFilePath.toString()), + pruneException); + } + } + private static String sanitizeControlCharacters(String rawText, String relativePath) { StringBuilder sanitizedTextBuilder = new StringBuilder(rawText.length()); int removedControlCharacterCount = 0; diff --git a/src/test/java/com/williamcallahan/javachat/cli/GitHubRepoProcessorIsolationTest.java b/src/test/java/com/williamcallahan/javachat/cli/GitHubRepoProcessorIsolationTest.java index 148dee67..246587ea 100644 --- a/src/test/java/com/williamcallahan/javachat/cli/GitHubRepoProcessorIsolationTest.java +++ b/src/test/java/com/williamcallahan/javachat/cli/GitHubRepoProcessorIsolationTest.java @@ -8,6 +8,7 @@ import static org.mockito.ArgumentMatchers.anyMap; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -156,6 +157,32 @@ void fileFailurePreventsOrphanPruningAndMetadataRefresh(@TempDir Path temporaryD verifyNoInteractions(processorFixture.ingestedFilePruneService()); } + @Test + void skippedOversizedUrlStaysActiveWhileGenuinelyDeletedFileIsPurged(@TempDir Path repositoryRoot) + throws IOException { + for (int fileIndex = 0; fileIndex < ELIGIBLE_FILE_COUNT; fileIndex++) { + Files.writeString(repositoryRoot.resolve("Source" + fileIndex + ".java"), "class Source {}"); + } + String oversizedUrl = "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/openai/java-chat/blob/main/package-lock.json"; + String deletedUrl = "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/openai/java-chat/blob/main/Deleted.java"; + ProcessorFixture processorFixture = processorFixture(); + when(processorFixture.hybridVectorService().scrollAllUrlsInCollection(COLLECTION_NAME)) + .thenReturn(Set.of(oversizedUrl, deletedUrl)); + when(processorFixture + .fileProcessor() + .process( + any(SourceCodeFileIngestionProcessor.RepositoryIngestionContext.class), + any(Path.class))) + .thenReturn(new SourceFileProcessingResult(LocalDocsFileOutcome.skippedFile(), oversizedUrl)); + + processorFixture.processor().processRepository(repositoryMetadata(repositoryRoot)); + + verify(processorFixture.ingestedFilePruneService()) + .pruneCollectionFileStrict(COLLECTION_NAME, deletedUrl, null); + verify(processorFixture.ingestedFilePruneService(), never()) + .pruneCollectionFileStrict(COLLECTION_NAME, oversizedUrl, null); + } + private static GitHubRepoMetadata repositoryMetadata(Path repositoryRoot) { return new GitHubRepoMetadata( repositoryRoot.toString(), diff --git a/src/test/java/com/williamcallahan/javachat/service/ingestion/SourceCodeFileIngestionProcessorTest.java b/src/test/java/com/williamcallahan/javachat/service/ingestion/SourceCodeFileIngestionProcessorTest.java index abef2930..31d04b7e 100644 --- a/src/test/java/com/williamcallahan/javachat/service/ingestion/SourceCodeFileIngestionProcessorTest.java +++ b/src/test/java/com/williamcallahan/javachat/service/ingestion/SourceCodeFileIngestionProcessorTest.java @@ -672,6 +672,117 @@ void processAlwaysIncludesFileUrlInResult(@TempDir Path temporaryDirectory) thro assertTrue(sourceFileProcessing.fileUrl().startsWith("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/owner/repo/blob/main/src/Empty.java")); } + @Test + void oversizedPreviouslyIngestedFilePrunesStalePriorState(@TempDir Path temporaryDirectory) throws IOException { + IngestionProcessorHarness harness = ingestionProcessorHarness(temporaryDirectory, "BigFile.java"); + Files.writeString( + harness.sourceFilePath(), + "a".repeat((int) SourceCodeFileIngestionProcessor.MAX_FILE_SIZE_BYTES + 1), + StandardCharsets.UTF_8); + + SourceFileProcessingResult sourceFileProcessing = harness.ingestionProcessor() + .process(harness.repositoryContext(Set.of(harness.sourceUrl())), harness.sourceFilePath()); + + assertFalse(sourceFileProcessing.outcome().processed()); + assertTrue(sourceFileProcessing.outcome().failure().isEmpty()); + assertEquals(harness.sourceUrl(), sourceFileProcessing.fileUrl()); + verify(harness.ingestedFilePruneService()) + .pruneCollectionFileStrict(TARGET_COLLECTION_NAME, harness.sourceUrl(), null); + verify(harness.fileIngestionMarkerStore(), never()).readFileIngestionRecord(anyString()); + verifyNoInteractions(harness.chunkProcessingService()); + } + + @Test + void oversizedNeverIngestedFileSkipsWithoutPruning(@TempDir Path temporaryDirectory) throws IOException { + IngestionProcessorHarness harness = ingestionProcessorHarness(temporaryDirectory, "BigFile.java"); + Files.writeString( + harness.sourceFilePath(), + "a".repeat((int) SourceCodeFileIngestionProcessor.MAX_FILE_SIZE_BYTES + 1), + StandardCharsets.UTF_8); + + SourceFileProcessingResult sourceFileProcessing = + harness.ingestionProcessor().process(harness.repositoryContext(Set.of()), harness.sourceFilePath()); + + assertFalse(sourceFileProcessing.outcome().processed()); + assertTrue(sourceFileProcessing.outcome().failure().isEmpty()); + assertEquals(harness.sourceUrl(), sourceFileProcessing.fileUrl()); + verifyNoInteractions(harness.ingestedFilePruneService()); + verifyNoInteractions(harness.fileIngestionMarkerStore()); + verifyNoInteractions(harness.chunkProcessingService()); + } + + @Test + void oversizedFileWithUnrelatedStoredUrlSkipsWithoutPruning(@TempDir Path temporaryDirectory) throws IOException { + IngestionProcessorHarness harness = ingestionProcessorHarness(temporaryDirectory, "BigFile.java"); + Files.writeString( + harness.sourceFilePath(), + "a".repeat((int) SourceCodeFileIngestionProcessor.MAX_FILE_SIZE_BYTES + 1), + StandardCharsets.UTF_8); + String unrelatedStoredUrl = "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/openai/java-chat/blob/main/src/Unrelated.java"; + + SourceFileProcessingResult sourceFileProcessing = harness.ingestionProcessor() + .process(harness.repositoryContext(Set.of(unrelatedStoredUrl)), harness.sourceFilePath()); + + assertFalse(sourceFileProcessing.outcome().processed()); + assertEquals(harness.sourceUrl(), sourceFileProcessing.fileUrl()); + verifyNoInteractions(harness.ingestedFilePruneService()); + } + + @Test + void oversizedPruneFailureKeepsSkipOutcome(@TempDir Path temporaryDirectory) throws IOException { + IngestionProcessorHarness harness = ingestionProcessorHarness(temporaryDirectory, "BigFile.java"); + Files.writeString( + harness.sourceFilePath(), + "a".repeat((int) SourceCodeFileIngestionProcessor.MAX_FILE_SIZE_BYTES + 1), + StandardCharsets.UTF_8); + Mockito.doThrow(new IOException("prune failed")) + .when(harness.ingestedFilePruneService()) + .pruneCollectionFileStrict(TARGET_COLLECTION_NAME, harness.sourceUrl(), null); + + SourceFileProcessingResult sourceFileProcessing = harness.ingestionProcessor() + .process(harness.repositoryContext(Set.of(harness.sourceUrl())), harness.sourceFilePath()); + + assertFalse(sourceFileProcessing.outcome().processed()); + assertTrue( + sourceFileProcessing.outcome().failure().isEmpty(), + "Prune failure must not escalate a skip to a failure"); + assertEquals(harness.sourceUrl(), sourceFileProcessing.fileUrl()); + verify(harness.ingestedFilePruneService()) + .pruneCollectionFileStrict(TARGET_COLLECTION_NAME, harness.sourceUrl(), null); + } + + @Test + void binaryPreviouslyIngestedFilePrunesStalePriorState(@TempDir Path temporaryDirectory) throws IOException { + IngestionProcessorHarness harness = ingestionProcessorHarness(temporaryDirectory, "Binary.java"); + Files.write(harness.sourceFilePath(), new byte[] {(byte) 0xFF, (byte) 0xFE, 0x00, (byte) 0xAD}); + + SourceFileProcessingResult sourceFileProcessing = harness.ingestionProcessor() + .process(harness.repositoryContext(Set.of(harness.sourceUrl())), harness.sourceFilePath()); + + assertFalse(sourceFileProcessing.outcome().processed()); + assertTrue(sourceFileProcessing.outcome().failure().isEmpty()); + verify(harness.ingestedFilePruneService()) + .pruneCollectionFileStrict(TARGET_COLLECTION_NAME, harness.sourceUrl(), null); + verifyNoInteractions(harness.chunkProcessingService()); + verify(harness.fileIngestionMarkerStore(), never()).readFileIngestionRecord(anyString()); + } + + @Test + void blankPreviouslyIngestedFilePrunesStalePriorState(@TempDir Path temporaryDirectory) throws IOException { + IngestionProcessorHarness harness = ingestionProcessorHarness(temporaryDirectory, "Empty.java"); + Files.writeString(harness.sourceFilePath(), "", StandardCharsets.UTF_8); + + SourceFileProcessingResult sourceFileProcessing = harness.ingestionProcessor() + .process(harness.repositoryContext(Set.of(harness.sourceUrl())), harness.sourceFilePath()); + + assertFalse(sourceFileProcessing.outcome().processed()); + assertTrue(sourceFileProcessing.outcome().failure().isEmpty()); + verify(harness.ingestedFilePruneService()) + .pruneCollectionFileStrict(TARGET_COLLECTION_NAME, harness.sourceUrl(), null); + verifyNoInteractions(harness.chunkProcessingService()); + verify(harness.fileIngestionMarkerStore(), never()).readFileIngestionRecord(anyString()); + } + private static SourceCodeFileIngestionProcessor.RepositoryIngestionContext repositoryContext( Path repositoryRoot, GitHubRepoMetadata repositoryMetadata) { return new SourceCodeFileIngestionProcessor.RepositoryIngestionContext( @@ -749,4 +860,69 @@ private SourceCodeFileIngestionProcessor.RepositoryIngestionContext repositoryCo repositoryRoot, repositoryMetadata, storedFileUrls); } } + + private static IngestionProcessorHarness ingestionProcessorHarness(Path temporaryDirectory, String sourceFileName) + throws IOException { + ChunkProcessingService chunkProcessingService = Mockito.mock(ChunkProcessingService.class); + HybridVectorService hybridVectorService = Mockito.mock(HybridVectorService.class); + LocalStoreService localStoreService = Mockito.mock(LocalStoreService.class); + FileIngestionMarkerStore fileIngestionMarkerStore = Mockito.mock(FileIngestionMarkerStore.class); + ContentHasher contentHasher = Mockito.mock(ContentHasher.class); + IngestedFilePruneService ingestedFilePruneService = Mockito.mock(IngestedFilePruneService.class); + SourceCodeFileIngestionProcessor ingestionProcessor = new SourceCodeFileIngestionProcessor( + new IngestionStorageServices( + hybridVectorService, + chunkProcessingService, + contentHasher, + localStoreService, + fileIngestionMarkerStore, + Mockito.mock(QdrantCollectionRouter.class)), + Mockito.mock(ProgressTracker.class), + ingestedFilePruneService); + Path repositoryRoot = temporaryDirectory.resolve("repository"); + Path sourceFilePath = repositoryRoot.resolve("src").resolve(sourceFileName); + Files.createDirectories(Objects.requireNonNull(sourceFilePath.getParent(), "sourceFilePath parent")); + GitHubRepoMetadata repositoryMetadata = new GitHubRepoMetadata( + repositoryRoot.toString(), + GitHubRepositoryIdentity.of("openai", "java-chat"), + TARGET_COLLECTION_NAME, + "main", + "abcdef123456", + "MIT", + "Example repository"); + String encodedSourceFileName = + URLEncoder.encode(sourceFileName, StandardCharsets.UTF_8).replace("+", "%20"); + String sourceUrl = "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/openai/java-chat/blob/main/src/" + encodedSourceFileName; + return new IngestionProcessorHarness( + ingestionProcessor, + chunkProcessingService, + hybridVectorService, + localStoreService, + fileIngestionMarkerStore, + contentHasher, + ingestedFilePruneService, + repositoryRoot, + sourceFilePath, + repositoryMetadata, + sourceUrl); + } + + private record IngestionProcessorHarness( + SourceCodeFileIngestionProcessor ingestionProcessor, + ChunkProcessingService chunkProcessingService, + HybridVectorService hybridVectorService, + LocalStoreService localStoreService, + FileIngestionMarkerStore fileIngestionMarkerStore, + ContentHasher contentHasher, + IngestedFilePruneService ingestedFilePruneService, + Path repositoryRoot, + Path sourceFilePath, + GitHubRepoMetadata repositoryMetadata, + String sourceUrl) { + private SourceCodeFileIngestionProcessor.RepositoryIngestionContext repositoryContext( + Set storedFileUrls) { + return new SourceCodeFileIngestionProcessor.RepositoryIngestionContext( + repositoryRoot, repositoryMetadata, storedFileUrls); + } + } }