fix(kb): legacy pipeline stands down for a promoted knowledge base - #900
Merged
Conversation
Routing exclusivity was only ever enforced on one side. The managed ingestion consumer returns immediately for a legacy document, but the legacy pipeline had no engine gate at all, and its `s3:ObjectCreated` notification is still live alongside the consumer's EventBridge rule. So every document added to a promoted knowledge base was handled twice, and design.md §537's "a document is indexed on exactly one backend" was untrue. The duplicate vectors are the cheap half of the problem. The expensive half is that two writers owned one `status` field and the last one won by luck. Both outcomes were observed in dev: * `DOC-b5d5d8019f44` was marked `complete` by the legacy pipeline at +30 s while the managed knowledge base — which is what actually serves this assistant — could not answer for it until +95 s. Sixty-five seconds of "your document is ready" followed by an answer that does not mention it. That is precisely the failure ingestion_consumer.py's docstring says it polls to prevent, defeated by a second writer nobody had gated. * `DOC-d637491d6cb1`, an image-only 4-year flowchart, failed Docling outright (`Docling produced zero chunks`) and was marked `failed`, while Bedrock's image extraction indexed it successfully and served it. It only ended up reading `complete` because the managed consumer happened to finish second. Reverse the finishing order — purely a function of parse time — and a good, retrievable document reads `failed` permanently, with no retry endpoint to recover it (task 14.4 is still open). So `handler.py` now resolves the engine before writing any status and returns early for `managed`, doing no parse, no embed and no status write. Keyed on the ENGINE, not on the presence of a KB record and not on `migrationState`. During `shadow` and `verify` the legacy path is still authoritative and must keep working (Requirements 16.1, 16.6); only `promote` writes `retrievalEngine`. Two tests pin that, including one for a record in `shadow`. An unreadable record resolves to legacy on purpose, because the two errors are not symmetric: wrong towards legacy costs a duplicate index while the consumer still drives the document to a correct terminal state, whereas wrong towards skipping leaves an upload un-ingested with no error and no way out but re-uploading. Same convention as resolver.load_record. Delegates to `records.resolve_engine` rather than reading the attribute inline, so "absence means legacy" keeps exactly one definition — the same call the consumer makes for the mirror-image decision. That needs `apis/shared/kb_backend/` in the rag-ingestion image, which is cheap: the package's module-level imports are stdlib only, enforced by test_kb_backend_boundary.py. Added to the Dockerfile and to build-one.sh's SOURCE_DIRS so the content hash moves with the gate. No IAM or environment change: the Lambda already carries DYNAMODB_ASSISTANTS_TABLE_NAME and already holds dynamodb:GetItem (verified against the deployed function and role in dev). The bootstrap copy at infrastructure/bootstrap-assets/rag-ingestion/handler.py needs no gate — it is a 33-line no-op placeholder that indexes nothing. Guards: 8 tests in tests/ingestion/test_ingestion_engine_gate.py. Three mutations verified caught, each by correctly-named tests and each parsed before running so a syntax error could not masquerade as a detection: - gate removed -> the 3 "promoted KB is left alone" tests - fail-open inverted -> the unreadable-record test - Dockerfile COPY dropped -> test_lambda_image_imports[rag-ingestion] Backend suite 6,825 passed; the 5 failures are the pre-existing Strands SDK contract tests documented in HANDOFF §2.
The mirror of the ingestion gate in the previous commit. Ingestion now routes by engine; deletion did not route at all. `cleanup_service` removed the legacy S3 Vectors copy and the `DOC#` row and never touched the managed knowledge base, so on a promoted knowledge base a deleted document stayed indexed forever. The managed delete path existed — `kb_backend/tombstones.py` — but its only callers are in the reconciler, which is report-only with `reconcilerArmed` off. Three consequences, none of which raised anything: * Storage was paid for indefinitely, at $5.00/GB-month against S3 Vectors' ~$0.15 — orphans on the expensive engine. * Retrieval quietly degraded. The status filter runs AFTER retrieval, so each orphan consumed a slot in `top_k` and was then dropped: a query could return five chunks and the model see two, with nothing logged. * The only thing preventing deleted content from being served was the fail-closed status filter, which has exactly one fail-open branch (`if not doc_ids: return vectors`). That line became load-bearing in a way it was never designed to be. `cleanup_document_resources` gains a third phase, engine-gated, conjoined into `all_succeeded` so a failure blocks the hard delete. The asymmetry with the ingestion gate is deliberate. There, an unreadable KB record resolves to legacy, because being wrong costs a duplicate index while the consumer still finishes the document. Here it must FAIL: the `DOC#` row is what the status filter joins against, so reporting success on a failed managed delete would remove the row *and* leave the content — the one combination that turns a storage leak into a disclosure. `ManagedKbNotProvisioned` is the exception, treated as terminal success: nothing was ever indexed, so there is nothing to remove and no reason to retry. IAM — the part with no code to give it away ------------------------------------------- Neither the app-api task role nor the kb-sync worker could delete from a managed knowledge base, so the code above would have deployed clean and failed on first use. New `grantManagedKbDocumentDeletion`, deliberately NOT `grantDirectIngestion`: these callers only ever remove documents, so a bug in the delete path cannot add content and a bug in the ingest path cannot remove it. It includes `bedrock:StartIngestionJob` and that again looks wrong. AWS groups the whole `KnowledgeBaseDocs` family with it in one statement, and this feature has already shipped a grant naming only the matching API and failing at runtime. Granting it costs nothing here, since `DeleteKnowledgeBaseDocuments` is itself the destructive verb. kb-sync also needed the image change: its worker calls `cleanup_document_resources` after soft-deleting a document whose upstream source has vanished, so `apis/shared/kb_backend/` is added to Dockerfile.kb-sync and to build-one.sh's kb-sync SOURCE_DIRS alongside the rag-ingestion entry. Pre-existing cleanup tests now declare their engine --------------------------------------------------- Ten blocks across tests/routes/test_cleanup_service.py and tests/property/test_pbt_cleanup_service.py patched `records.get_kb_record -> None`. They describe a legacy knowledge base — the state of every assistant predating this feature — and previously said nothing about the engine because there was no third phase to say it to. Left alone they reached a real DynamoDB read. Guards: 7 tests in tests/documents/test_managed_delete_propagation.py, plus grant-shape tests in managed-kb.test.ts and wiring assertions on the real roles in kb-sync.test.ts and platform-stack.test.ts — the latter because a grant proven on a fake role says nothing about the identity that runs the code, which is how two earlier defects on this feature shipped. Four mutations verified caught, each by correctly-named tests: - managed phase dropped from the conjunction -> test_a_failed_managed_delete_blocks_the_hard_delete - every KB treated as legacy -> 3 tests incl. test_the_document_is_deleted_from_the_managed_kb - unreadable record reports success -> test_an_unreadable_record_fails_rather_than_assuming_legacy - both grant attachments removed -> KbSyncConstruct + PlatformStack wiring tests Backend 6,832 passed; infra 640 passed. The 5 backend failures are the pre-existing Strands SDK contract tests documented in HANDOFF §2.
DerrickF
added a commit
that referenced
this pull request
Sep 1, 2026
…nostics Records findings 32–36 and the tooling that found them. All but one are fixed in PR #900; §5.33 is the only one from 2026-08-31 still open. The four new entries share a root cause worth stating once: **the two engines were never made exclusive.** The ingestion consumer stands down for a legacy document; nothing made the legacy pipeline stand down for a managed one, and nothing propagated a deletion to the managed engine at all. Double-indexing (32), a status field with two owners (34), and an orphaned managed corpus (36) are three costumes on that one cause. Item 34 is the one to read. Two writers owned `status`, so "ready" was decided by whichever finished last. Observed both ways within an hour: a PDF marked `complete` 65 s before the managed KB could answer for it, and an image-only PDF marked `failed` while the managed KB served it correctly — the latter only ended up correct because the consumer happened to finish second. The generalisable lesson is that any field two components can write needs a stated owner, and nobody chose this race; it appeared because a writer was added beside an old one and the question was never asked. Item 35 is not a defect but a measured capability difference: Bedrock's image extraction works. A pure-diagram curriculum flowchart is permanently `failed` on legacy (`docling_processor.py` sets `do_ocr=False`) and retrievable in 94.5 s on managed, with the vision model's own description in the chunks. Also records that the old bundled `aws` CLI silently omits `mediaExtractionConfiguration` from `get-data-source`, so a field can look unset when it is not — do not conclude from CLI output alone. Item 36 documents the deliberate asymmetry between the two gates, which is the part most likely to be "cleaned up" by someone later: on ingest an unreadable KB record resolves to legacy, on delete it must fail. Same question, opposite answers, because the `DOC#` row is what the fail-closed status filter joins against. Corrects one claim in item 32: an earlier revision said the missing gate affected "both copies" of the handler including the bootstrap asset. The bootstrap copy is a 33-line no-op placeholder that indexes nothing and needs no gate. Adds two read-only diagnostics, documented in §2 with the numbers they produced: - `kb-doc-timings.py` — per-document ingestion timing and which engine did the work, derived from the record rather than guessed. Legacy 30 s vs managed 95 s on the same 132 KB PDF; `INDEXED → retrievable` 0.9 s, independently confirming the evaluation's 0.75–1.03 s. - `kb-compare-engines.py` — one query through both engines, exploiting the `retain` window where both indexes still hold the corpus. Marks each chunk against MAX_CONTEXT_CHARS, which is what makes the comparison honest: ~2,000 characters reach the model, so one or two chunks, so precision@1 is nearly everything. On `CS434` legacy's five chunks sit within 0.056 of each other and its top chunk does not contain the string; managed separates by 0.4988 with the literal match first. Notes that #900 ends the both-indexes trick, so a future A/B needs two assistants or documents predating promotion. Also adds an "engine visibility" item to the work queue: nothing logs which engine served a query, so "is the new one working?" is currently only answerable from the KB record — worth closing before a wide rollout given that this feature's risk profile is silent regressions.
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
1 participant
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.
Closes the half of routing exclusivity that was never built.
The bug
The managed ingestion consumer returns immediately for a legacy document. The legacy pipeline had no engine gate at all, and its
s3:ObjectCreatednotification is still live alongside the consumer's EventBridge rule. So every document added to a promoted knowledge base was handled by both, and design.md §537's "a document is indexed on exactly one backend" was simply untrue.Duplicate vectors are the cheap half. The expensive half: two writers owned one
statusfield and the last one won by luck. Both outcomes were observed in dev today.DOC-b5d5d8019f44completeat +30 s; the managed KB could not answer until +95 s. 65 seconds of "ready" followed by an answer that omits the document.DOC-d637491d6cb1(image-only 4-year flowchart)failed. Bedrock's image extraction indexed it fine and served it. It only readscompletebecause the consumer finished second.That second row is the alarming one. Reverse the finishing order — purely a function of parse time — and a good, retrievable document reads
failedforever, with no retry endpoint to recover it (task 14.4 open).It also defeats the exact protection
ingestion_consumer.pydocuments: "the UI says the upload worked, the user asks a question straight away, and the answer does not mention their document." The consumer polls until the document is genuinely retrievable to avoid that. A second, ungated writer undid it.The fix
handler.pyresolves the engine before writing any status and returns early formanaged— no parse, no embed, no status write.Keyed on the engine, not on the presence of a record and not on
migrationState. Duringshadow/verifythe legacy path is still authoritative and must keep working (Reqs 16.1, 16.6); onlypromotewritesretrievalEngine. Two tests pin that, including one for a record sitting inshadow.An unreadable record resolves to legacy, deliberately. The errors are asymmetric: wrong towards legacy costs a duplicate index while the consumer still drives the document to a correct terminal state; wrong towards skipping leaves an upload un-ingested with no error and no way out but re-uploading. Same convention as
resolver.load_record.Delegates to
records.resolve_enginerather than reading the attribute inline, so "absence means legacy" keeps one definition — the same call the consumer makes for the mirror decision. That needsapis/shared/kb_backend/in the rag-ingestion image, which is cheap: module-level imports there are stdlib only (enforced bytest_kb_backend_boundary.py). Added to the Dockerfile and tobuild-one.sh'sSOURCE_DIRSso the content hash moves with the gate.No IAM or env change — the Lambda already carries
DYNAMODB_ASSISTANTS_TABLE_NAMEand already holdsdynamodb:GetItem, both verified against the deployed function and role in dev.The bootstrap copy needs no gate: it is a 33-line no-op placeholder that indexes nothing.
Verification
8 new tests. Three mutations verified caught, each by correctly-named tests, and each
ast.parsed before running so a syntax error could not masquerade as a detection:test_a_lookup_failure_does_not_skip_the_documenttest_lambda_image_imports[rag-ingestion]Backend suite 6,825 passed; the 5 failures are the pre-existing Strands SDK contract tests documented in HANDOFF §2.
Note for whoever compares engines after this
Until now, a document uploaded to a promoted knowledge base landed in both indexes, which made
scripts/local-dev/kb-compare-engines.pya true A/B on identical documents for any assistant inretain. After this change new uploads land in the managed index only, so a like-for-like comparison needs two assistants, or documents added before promotion.