fix: roll back KB sync change gates when the S3 stage fails - #1373
Merged
Merged
Conversation
The Drive and web re-crawl sync paths write sourceEtag/contentHash alongside previousChunkCount and stagedContentHash BEFORE staging to S3 (those two must precede the ObjectCreated event). When the put raised, the run failed but the gates stayed advanced, so every later run short-circuited as "unchanged" and the change was never ingested, on both legacy and managed knowledge bases. A failed stage now conditionally restores sourceEtag, contentHash, stagedContentHash and lastSyncedAt to their prior values, only if they still hold what this run wrote, so a newer run's write and the managed consumer's over-cap REMOVE are left alone. The crawler reports a new "stage_failed" outcome instead of letting the exception escape its worker task, marks a never-staged new page failed, and still walks the fetched page's links so its children aren't counted as misses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Problem
_sync_drive_file's changed branch, and_sync_web_crawl'son_result("changed"), write the newsourceEtagandcontentHashtogether withpreviousChunkCountandstagedContentHashbefore the S3 put. If the put raises, the run is recorded as failed, but theDOC#row already carries the new gates. The next run then short-circuits at gate 1 (etag equal) or gate 2 (hash equal) and returnsunchanged. The change is never staged and never ingested, on both legacy and managed knowledge bases.The web path had two more problems:
created) had itscontentHashwritten before staging too, so a failed put left it stuck in the same way._put_markdown's exception escaped the crawler's worker task unobserved.Fix
The write-before-stage ordering stays.
previousChunkCount(legacy tail-delete) andstagedContentHash(the managed consumer's re-ingest marker) must be on the row before ObjectCreated fires. What changes is that a failed stage now rolls the gates back.records.rollback_document_sync_fields. It restoressourceEtag,contentHash,stagedContentHashandlastSyncedAtto their pre-run values. Any value that was absent before gets REMOVEd. The write is conditioned on every attribute still holding what this run wrote, so a newer run's write is left alone. So is the consumer's_record_reingest_over_capREMOVE: an attribute that no longer exists fails the equality check, so a late rollback can't bring the gates back.stagedContentHashis restored as well. Left in place, it would name a version the object doesn't hold, and a redelivered event could then record that hash as ingested against the old bytes._stage_to_s3, then the exception is re-raised. The run still recordsfailed._put_markdownand emits a newstage_failedoutcome. The worker rolls back whatever it wrote for that page onchangedorcreated. The crawler also counts the page as failed, marks a never-staged new pagefailed, and still walks the fetched page's links. Without that last step, an S3 failure on the root would leave every child page unseen, and refresh counts unseen pages as misses toward deletion. The run result no longer reportschangedfor pages that failed to stage.I chose rollback over "advance the gates only after a successful stage" deliberately. A post-stage write can race the consumer's over-cap REMOVE (ObjectCreated → consumer → REMOVE, then the late SET puts the gates back) and silently cancel the forced re-stage. The rollback only ever runs when no object was written, so no consumer event exists to race with.
Behavior change on initial crawls: a put failure used to leave the page's doc
pendingindefinitely because of the unobserved task exception. It now marks the docfailedwith "The page could not be stored."Residual gap: a hard Lambda kill between the DynamoDB write and the put still leaves the gates advanced. That window is a single
put_objectcall.Tests
TestStageFailure(Drive):_stage_to_s3followed by a second run with the same Drive version and bytes returnschangedand stagesTestWebCrawlSync::test_failed_stage_rolls_back_page_gates: rollback plus the next run'sRefreshStatestill holding the old gates.TestWebCrawlStageFailureEndToEnd: the worker plus the real crawler (httpxMockTransport, put stubbed to fail on run 1). On run 2, both a changed page and a page new to the crawl are staged.test_crawler.py::test_refresh_stage_failure_emits_stage_failed.tests/lambdas(includingtest_kb_sync_managed_reingest.py),tests/apis/app_api/web_sources,tests/architecture, and the sync-policy, kb-migration and supply-chain suites all pass (741 tests).🤖 Generated with Claude Code