Skip to content

fix: roll back KB sync change gates when the S3 stage fails - #1373

Merged
philmerrell merged 1 commit into
developfrom
fix/kb-sync-gates-advance-after-stage
Sep 27, 2026
Merged

philmerrell merged 1 commit into
developfrom
fix/kb-sync-gates-advance-after-stage

Conversation

@philmerrell

Copy link
Copy Markdown
Contributor

Problem

_sync_drive_file's changed branch, and _sync_web_crawl's on_result("changed"), write the new sourceEtag and contentHash together with previousChunkCount and stagedContentHash before the S3 put. If the put raises, the run is recorded as failed, but the DOC# row already carries the new gates. The next run then short-circuits at gate 1 (etag equal) or gate 2 (hash equal) and returns unchanged. The change is never staged and never ingested, on both legacy and managed knowledge bases.

The web path had two more problems:

  • A page new to the crawl (created) had its contentHash written 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) and stagedContentHash (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.

  • New records.rollback_document_sync_fields. It restores sourceEtag, contentHash, stagedContentHash and lastSyncedAt to 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_cap REMOVE: an attribute that no longer exists fails the equality check, so a late rollback can't bring the gates back. stagedContentHash is 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.
  • Drive: the rollback runs around _stage_to_s3, then the exception is re-raised. The run still records failed.
  • Web: the crawler catches a failed _put_markdown and emits a new stage_failed outcome. The worker rolls back whatever it wrote for that page on changed or created. The crawler also counts the page as failed, marks a never-staged new page failed, 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 reports changed for 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 pending indefinitely because of the unobserved task exception. It now marks the doc failed with "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_object call.

Tests

  • TestStageFailure (Drive):
    • a failed _stage_to_s3 followed by a second run with the same Drive version and bytes returns changed and stages
    • earlier gate values are restored rather than removed
    • the rollback refuses when a newer write has landed
    • the rollback does not bring back gates the consumer's over-cap path REMOVEd
  • TestWebCrawlSync::test_failed_stage_rolls_back_page_gates: rollback plus the next run's RefreshState still holding the old gates.
  • TestWebCrawlStageFailureEndToEnd: the worker plus the real crawler (httpx MockTransport, 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.
  • The 5 behavioral tests fail against the unfixed source and pass with the fix.
  • tests/lambdas (including test_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

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>
@philmerrell
philmerrell merged commit 38ef4c8 into develop Sep 27, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant