fix(kb): record the knowledge base id before anything else can fail - #898
Merged
Conversation
The retry after the ACTIVE fix did not reach the data source. It could not: ConflictException: KnowledgeBase with name dev-boisestateai-v2-kb-ast-1a90784a7f18 already exists. `awsKbId` was only ever written by `attach_aws_ids`, which runs after BOTH AWS creates succeed. So a failure between them left a record with no identifier, and every later attempt re-entered the create path and was refused, permanently, because the name was taken. The record was unrecoverable by any number of retries. The `clientToken` does not cover this, and this commit corrects that claim wherever it appears — including in a message I added two commits ago. AWS idempotency tokens expire within minutes; a retry an hour later is a genuinely new request that collides on the unique name. Two changes: * `records.attach_knowledge_base_id` persists `awsKbId` the moment the create returns, guarded on `attribute_not_exists(awsKbId)` so a straggler cannot overwrite a newer attempt's identifier. Everything after that point is resumable. * Provisioning adopts by name on a name-collision `ConflictException`. This is what recovers records already stuck in that state, including the one in dev — without it the only recourse is deleting the knowledge base by hand. Adoption is safe because the name is derived from `app_kb_id`, so a collision can only be this knowledge base's own earlier attempt. The status conflict and the name conflict share the `ConflictException` code, so they are told apart by message. WHY TWO TESTS CERTIFIED THE BUG `test_the_record_survives_as_a_discoverable_retry_anchor` asserted `"awsKbId" not in anchor` — the missing write, encoded as a requirement — and `test_the_retry_does_not_create_a_second_knowledge_base` asserted the retry re-issues the create and relied on the fake deduplicating it. `FakeBedrockAgent` modelled `clientToken` dedup as permanent and did not model name uniqueness at all, so both passed against a design that could not recover. The fake now enforces name uniqueness, treats tokens as expired by default, and answers `list_knowledge_bases`. Both tests are rewritten to assert what actually makes the window survivable, and a third covers the stuck-record state directly. Mutations verified caught: dropping the immediate persist, and dropping adopt-by-name. Tests: 2,465 passed across shared, lambdas, property, architecture and supply_chain.
…on in dev A migration now runs shadow -> verify -> promote -> retain and serves from the managed backend. Three further defects, all found by driving the state machine locally against dev instead of through a deploy cycle. THE EMBEDDING PIN AND MANAGED RERANKING ARE MUTUALLY EXCLUSIVE (Req 8.5 amended) Req 8.5 pinned titan-embed-text-v2:0 via `embeddingModelType: CUSTOM`; Req 11.2 requires `rerankingModelType: MANAGED`. AWS rejects the combination, and §13 had measured the two separately, never together. Measured all four: CUSTOM + MANAGED -> ValidationException CUSTOM + NONE -> ok, scores 1.00/0.982/0.952 (flat) default + MANAGED -> ok, scores 0.413/0.199 (separated) default + NONE -> ok The pin loses, for a better reason than "it buys little": it protected a failure mode that cannot occur here. On S3 Vectors *we* embed the question, so the query model must match the index. Managed retrieval sends text and managed ingestion sends text — we never produce a vector, so Bedrock embeds both sides and consistency is its invariant. §13 measured the pin as worth nothing (9/9 identical) against reranking being "what makes a small context cap defensible". `embeddingModelId`/`embeddingDimensions` are no longer recorded either: with no pin, nothing here knows what Bedrock chose, and a field naming Titan on a knowledge base embedded with something else is worse than an absent one. VERIFY FAILED A GOOD MIGRATION FOR BEING ASKED TOO EARLY The canary returned nothing because the freshly-ingested document was not yet queryable, and that was terminal. Measured ~45 s from ingest to retrievable on a fresh knowledge base, against the docstring's 0.75-1.03 s (a warm figure). Now defers via `records.defer_verify`, bounded at MAX_VERIFY_ATTEMPTS, so latency reads as latency and only a corpus that never answers fails. ADOPTION TOOK A KNOWLEDGE BASE THAT WAS BEING DELETED Found while recreating one locally: the delete had not finished, adopt-by-name took the DELETING knowledge base, and the ACTIVE wait then refused it. Adoption now skips terminal statuses — the name is about to free up, so a fresh create is right. ALSO A test that only passed while MANAGED_KB_SERVICE_ROLE_ARN was absent now deletes it explicitly; the variable is needed in backend/src/.env for the local driver. `scripts/local-dev/run-kb-migration.py` drives the state machine in-process against dev. Three of the last five defects would have been minutes each with it. HANDOFF.md is rewritten: it previously said "Nothing deployed", which has been untrue since 1.16.0 shipped the feature to production behind flags. Tests: 6,817 backend passed (5 pre-existing Strands failures), 626 infra.
…estion A document uploaded to a promoted knowledge base in dev went to `failed` with: AccessDeniedException ... IngestKnowledgeBaseDocuments ... not authorized to perform: bedrock:StartIngestionJob on resource: knowledge-base/M8WQZVQJ8X AWS authorizes `IngestKnowledgeBaseDocuments` under the adjacent action name `bedrock:StartIngestionJob`; both appear in one statement in AWS's direct-ingestion prerequisites. The grant carried only the name matching the API call, so it reviewed as complete, deployed clean, and failed on the first real upload — the same shape as the missing `bedrock:TagResource`. The worker had the identical gap. It receives the same grant and calls the same API, so the first migration driven by the deployed dispatcher would have failed identically. It stayed invisible because every migration so far was driven by scripts/local-dev/run-kb-migration.py under an SSO identity broader than either Lambda role. The action is easy to mistake for a mistake: Requirement 9.2 forbids *calling* StartIngestionJob (0.1 RPS account-wide, one document per ten seconds) and nothing does. Holding it is authorization, not invocation. A docblock and a separately-named test carry that reason so the obvious cleanup fails a test that explains itself. `bedrock:ListKnowledgeBaseDocuments` is in AWS's example policy and left out on purpose: no code path calls it. Guards: three tests, including one asserting both the worker and the ingestion-consumer roles carry the action. Mutation-tested — removing the action fails exactly four tests, each named for the reason.
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.
The record was unrecoverable
The retry after the
ACTIVEfix never reached the data source. It could not:awsKbIdwas only ever written byattach_aws_ids, which runs after both AWS creates succeed. A failure between them left a record with no identifier, so every later attempt re-entered the create path and was refused — permanently, because the name was taken. No number of retries could ever have worked.Correcting a claim I made
The
clientTokendoes not cover this, and this PR fixes that claim wherever it appears — including in a failure message I added two commits ago. AWS idempotency tokens expire within minutes; a retry an hour later is a genuinely new request that collides on the unique name.Two changes
records.attach_knowledge_base_idpersistsawsKbIdthe moment the create returns, guarded onattribute_not_exists(awsKbId)so a straggler from an abandoned attempt cannot overwrite a newer identifier. Everything after that point is resumable.Adopt-by-name on a name-collision
ConflictException. This is what recovers records already in that state, including the one in dev — without it the only recourse is deleting the knowledge base by hand. Adoption is safe because the name derives fromapp_kb_id, so a collision can only be this knowledge base's own earlier attempt. The status conflict and the name conflict share theConflictExceptioncode, so they are distinguished by message.Why two tests certified the bug
This is the part worth reading.
test_the_record_survives_as_a_discoverable_retry_anchorasserted"awsKbId" not in anchor— the missing write, encoded as a requirement.test_the_retry_does_not_create_a_second_knowledge_baseasserted the retry re-issues the create, and relied on the fake to deduplicate it.FakeBedrockAgentmodelledclientTokendedup as permanent and did not model name uniqueness at all. Both tests passed against a design that could not recover, because the test double encoded an incorrect model of the external service.The fake now enforces name uniqueness, treats tokens as expired by default (the realistic case for any retry that is not within minutes), and answers
list_knowledge_bases. Both tests are rewritten to assert what actually makes the window survivable, and a third covers the stuck-record state directly.Verification
2,465 tests pass across
shared,lambdas,property,architectureandsupply_chain.ruffclean.Mutations verified caught: dropping the immediate persist, and dropping adopt-by-name.
After merge
Backend-only —
backend.ymlrebuilds the image, no platform deploy. Then retrying adopts the existing2IRZCDKPNN, records its id, waits forACTIVE(already there), and proceeds to the data source.Added: the IAM action that authorizes direct ingestion
Found by adding a document to the promoted knowledge base in dev. It went to
failed:AWS authorizes
IngestKnowledgeBaseDocumentsunder the adjacent action namebedrock:StartIngestionJob— both appear in one statement in AWS's direct-ingestion prerequisites.grantManagedKbDirectIngestioncarried only the name matching the API call, so it reviewed as complete, deployed clean, and failed the first real upload. Same shape as the missingbedrock:TagResource.The worker had the identical gap — same grant, same API — so the first migration driven by the deployed dispatcher would have failed the same way. It stayed hidden because every migration so far ran through
scripts/local-dev/run-kb-migration.pyunder an SSO identity broader than either Lambda role. Precisely the blind spot HANDOFF.md §2 names.The action invites deletion: Requirement 9.2 forbids calling
StartIngestionJob(0.1 RPS account-wide) and nothing does. Holding it is authorization, not invocation. A docblock and a separately-named test carry that reason.bedrock:ListKnowledgeBaseDocumentsis in AWS's example policy and omitted deliberately — no code path calls it.Guards: three tests, one asserting both the worker and ingestion-consumer roles carry the action. Mutation-tested — removing it fails exactly four tests, each named for the reason. Full infra suite 634 passing.
This touches
infrastructure/lib/constructs/**, so merging todeveloptriggers the PlatformStack deploy that applies it.