fix(ingestion): prune stale vectors when a changed file is guard-rejected - #237
Open
detail-app[bot] wants to merge 1 commit into
Open
Conversation
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 #224
Bug
When a previously-ingested local docs file's on-disk content changed to a body
HtmlContentGuardrejects (e.g. a re-fetched mirror file overwritten by a Cloudflare "Just a moment…" challenge or a404 Not Foundbody), the content-guard rejection branch inLocalDocsFileIngestionProcessor.prepare()returned afailedFileafter only copying the file to quarantine. It discarded the already-computedrequiresFullReindexandmarkerContext, so it never deleted the URL's prior Qdrant points, never pruned the now-orphaned local parsed-chunk state, and never advanced the file marker.Consequences:
snippet(storeddoc_content) disagreed with itsurl.processExcludedPagepath already honored the correct contract for "previously-indexed URL no longer indexable" (delete vectors, prune local state, re-mark with empty chunk hashes), but the guard branch skipped all three steps.Fix
Route the guard-rejection branch through the same cleanup sequence
processExcludedPageuses, while preserving thefailedFileoutcome so batch-stop semantics are unchanged:prepare()capturesrequiresFullReindexandmarkerContextand passes them into the deferred quarantine transition (previously onlyfileandrejectionReasonwere closed over).quarantineRejectedFileruns the forensic quarantine copy as best-effort (its try/catch is narrowed to the copy only), and whenrequiresFullReindexit deletes the URL's vectors (deleteByUrl), prunes obsolete local state (pruneObsoleteLocalStateAfterReplacement(..., List.of())), and writes a freshFileIngestionRecordwith the rejected file's new fingerprint andchunkHashes = List.of(). The phase stayscontent-guard(orquarantine-writewhen the copy fails), soprocessBatchstill stops on rejection.Stringlocals to stay under the PMDCouplingBetweenObjectsthreshold (the class sits at 35; anIngestionLocalFailurelocal would have pushed it over).Testing
LocalDocsFileIngestionProcessorTest(Mockito, no external services):shouldNotLeaveStaleVectorsWhenPreviouslyIngestedFileIsGuardRejectedassertsdeleteByUrl,pruneObsoleteLocalStateAfterReplacement, andmarkFileIngested(..., List.of())are all invoked when a previously-ingested file is guard-rejected.shouldNotLeaveStaleVectorsWhenQuarantineWriteFailsForPreviouslyIngestedFilestubs the quarantine write to throwIOExceptionand asserts the same cleanup still runs, pinning the best-effort ordering.shouldBeIdempotentOnNextRunAfterGuardRejectedPreviouslyIngestedFiledrives two consecutive runs: the first rejects and cleans up; the second, over the same rejected bytes, returns a terminalskippedFilewith no re-quarantine, no re-cleanup, and no chunking — pinning that the marker advance makes the next run idempotent.deleteByUrlnever invoked) and pass with the fix.compileJava/compileTestJava; Spotless (Palantir) check; PMDpmdMain/pmdTest(noCouplingBetweenObjectsviolation); SpotBugsspotbugsMain/spotbugsTest;make lint(ast-grep rules, chat-model SSOT check, frontend oxlint/eslint/svelte-check); and the CI lane (./gradlew build -x test+spotbugsMain pmdMain).LocalDocsFileIngestionProcessorTestclass (31 cases), the fullcom.williamcallahan.javachat.service.ingestion.*package (101 cases), and the full Java unit suite (1165 cases) pass with zero failures — the existing first-time-rejection tests (shouldStopBeforeLaterFileWhenContentIsRejected,shouldStopBeforeLaterFileWhenQuarantineWriteFails), the siblingprocessExcludedPagecontract, and the chunk-replacement / marker / prune ordering are all unchanged.make compose-up), confirmed/healthzpassed, and ran the repository's synthetic hybrid-contract check (make test-qdrant-integration), which exercises the Qdrant REST surface (schema, indexes, upsert, exact count, hybrid filter, discovery, citation scroll) that the fix'sdeleteByUrloperation targets. Passed.replaceUrlDocumentsrecovers the URL. Both require a reachable embedding gateway. The configured gateway (api.llm-gateway.iocloudhost.net) is reachable but rejects the only available placeholder credential withHTTP 401 Invalid API Key, and no realOPENAI_API_KEYis provisioned in this environment, so the live embedding round-trip could not run. The unit tests above pin the cleanup behavior these procedures would exercise end-to-end.Automatic Fixes PRs can be configured here.