fix(aws): fail adoption gracefully when Cloud Control resource cannot be read back - #1447
Merged
Conversation
… be read back When CreateResource fails with AlreadyExists but GetResource returns nothing for the conflicting identifier, the adopt path crashed with "TypeError: Cannot convert undefined or null to object" — a non-null assertion let undefined flow into the patch computation. This happens when the name is taken in a globally shared namespace but not visible in the caller's account (e.g. an S3 bucket owned by another AWS account), which Cloud Control also reports as AlreadyExists. Retry the read-back briefly to absorb eventual consistency, then throw an actionable error (with the original conflict as cause) when the resource genuinely cannot be adopted. Applies to both the AlreadyExistsError and ConcurrentOperationError adoption branches. Also rename the bucket used by the "replace resource when immutable property changes" test: its previous deterministic name is squatted in the global S3 namespace, making the test fail unconditionally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
commit: |
Every S3 bucket name previously used by these suites is taken in the global namespace by an unknown account, so creation fails with AlreadyExists while the bucket is invisible to the test account. Rename the base testIds so all derived bucket names are fresh. Co-Authored-By: Claude Fable 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
In the Cloud Control adopt path, when
CreateResourcefails withAlreadyExistsbutGetResourceon the conflicting identifier returns nothing, a non-null assertion letundefinedflow into the patch computation:The same unchecked pattern existed in the
ConcurrentOperationErroradoption branch.Root cause investigation
This is trivially reachable with S3: bucket names live in a globally shared namespace, so
CreateResourcereportsAlreadyExistswhen any AWS account owns the name — butGetResource(scoped to the caller's account) finds nothing to adopt:The existing test
replace resource when immutable property changeswas failing deterministically with thisTypeErrorbecause its deterministic bucket name (<prefix>-test-bucket-resource-immutable-test) is squatted outside the test account.Fix
Both adoption branches now go through a
getResourceForAdoptionhelper that:GetResource), andcause— when the resource cannot be read back, since adoption is impossible:Usage
No API change — the failure mode just becomes diagnosable:
Testing
cause) instead of theTypeError.main, 6 of the 10 tests intest/aws/control/fail in my environment: every S3 bucket name these suites have historically used (samgoodwin-test-bucket-resource*,samgoodwin-test-bucket-proxy*) is taken in the global namespace by an unknown account, so creation fails withAlreadyExistswhile the bucket is invisible to the test account — which is exactly the crash this PR fixes. This PR renames the basetestIdin both test files so all derived bucket names are fresh.test/aws/control/suite passes against live AWS:🤖 Generated with Claude Code