fix(ingestion): prune stale vectors when a previously-ingested file is skipped - #235
Open
detail-app[bot] wants to merge 1 commit into
Open
detail-app[bot] wants to merge 1 commit into
detail-app[bot] wants to merge 1 commit into
Conversation
…s 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Detail bug report: View on Detail
Closes #222
Bug
SourceCodeFileIngestionProcessor.process()returnedskippedFile()carrying the file URL before any prior-state cleanup whenever a previously-ingested file subsequently exceeded the 1 MiB limit (or became binary/blank). The runner then added the skipped URL toactiveFileUrlsunconditionally, which shielded the stale Qdrant points and local marker frompurgeDeletedFileOrphansfor the rest of the run. The stale chunks — contradicting the file's on-disk bytes and reachable by search with no exclusion filter — stayed in the collection until the file shrank back below the limit or was deleted.Root cause: the oversized skip branch at the top of
process()short-circuited beforereadFileIngestionRecordor any prune path was reached, and the runner treated every returned URL (including skipped ones) as active. Both behaviors were present verbatim in the initial source-code ingestion pipeline (499894a) and never modified since.Fix
Treat a skip of a previously-ingested file as a removal rather than a no-op, reusing the existing strict prune path the runner's own orphan purger already takes.
process(), callpruneSkipIfPreviouslyIngested(...)at both skip-before-prune sites (thevalidateFileAttributesoversized branch, gated oninstanceof Skippedso theFailedmissing-filename branch is excluded; and thereadAndFingerprintFileempty branch covering binaryMalformedInputExceptionand blankfileText.isBlank()).IngestedFilePruneService.pruneCollectionFileStrict(canonicalCollectionName, fileUrl, null)— the same null-record prune the orphan purger uses, which deletes by URL payload and reconstructs chunk hashes from parsed chunks on disk.repositoryContext.storedFileUrls().contains(fileUrl)so a never-ingested oversized file never triggers a QdrantdeleteByUrlround-trip.IOExceptionat WARN so a best-effort prune failure never escalates aSkippedto aFailedoutcome (which would trip the runner'sfailed() > 0terminal throw over cleanup that was best-effort to begin with).CRLF_INJECTION_LOGSSpotBugs exclusion forSourceCodeFileIngestionProcessor, matching the existingGitHubRepoProcessor/LocalDocsFileIngestionProcessorpattern — the newlog.warnpasses the path throughrenderPathForLog, which escapes CR/LF before formatting, but FindSecBugs cannot follow the custom sanitizer.Testing
SourceCodeFileIngestionProcessorTest, 6 new): oversized/binary/blank transitions prune prior state; never-ingested and unrelated-URL stored sets do not trigger a prune; a pruneIOExceptionkeeps the outcomeSkipped. Confirmed the four positive-prune tests fail without the production fix and pass with it.GitHubRepoProcessorIsolationTest, 1 new): a skipped oversized URL stays inactiveFileUrls(no double-prune) while a genuinely deleted file is still purged by the orphan purger.service.ingestion,cli,application.ingestion, anddomain.ingestionpackages, 0 failures.spotlessCheck,pmdMain,pmdTest,spotbugsMain,spotbugsTestall pass;make lint(ast-grep + frontend + JVM static analysis) andmake buildpass.test_github_repository_metadata_contract.sh,test_github_sync_failure_contract.sh) pass, as do all 16make test-shellscripts.SPRING_PROFILE/state-dir env. Run N (ingesting the sub-1 MiB sentinel file) reproducibly failed atphase=qdrant-replacementbecause the application'sreplaceUrlDocuments/doUpsertpost-upsert identity verification rejects the mock's vectors, so the sentinel-bearing file was never committed to the collection and the oversized→prune transition could not be demonstrated end-to-end. Reproducing it would require a realqwen/qwen3-embedding-4bmodel server this sandbox cannot provide. The fix's behavioral guarantee is fully covered by the unit and runner tests above, which exercise the prune at the observable code boundary without the embedding/Qdrant runtime.Automatic Fixes PRs can be configured here.