fix(serve): fix two P0 resource-reuse bugs (instance-type reuse + Bedrock permission fail-open) - #6109
Open
eliseharvey wants to merge 1 commit into
Open
Conversation
…rock permission fail-open) Two independent reuse defects surfaced during Nova SDK dogfooding, both under reuse_resources=True. Fixed together as they share the reuse code paths. --- 1. instance_type mismatch silently reused --- deploy() trusted the endpoint candidate cached by build() in _reused_endpoint_name without re-validating it. That cache is resolved at build time, before instance_type (a deploy() argument) is known, so a deploy on a different instance type reused the wrong endpoint. Separately, _reused_endpoint_matches_config skipped the instance_type check entirely for Inference Component (IC) endpoints: the IC early-return (no ModelName on the variant) returned True before reaching the check. deploy() now re-validates the cached endpoint against the requested instance_type and falls back to a fresh discovery on a miss or mismatch. _reused_endpoint_matches_config checks instance_type -- which lives on the production variant and is available for every endpoint, including IC ones -- before the IC early-return, so an instance-type mismatch is never silently reused. --- 2. Bedrock reuse_resources=True fails open on missing permission --- Reuse discovery (find_existing_bedrock_model, find_active_bedrock_deployment_for_model, find_existing_sagemaker_endpoint) swallowed every exception and returned None. When the execution role lacked a read permission (e.g. bedrock:ListTagsForResource), a denied discovery call was indistinguishable from "nothing to reuse", so reuse fell through to creating the resource -- which then failed with a confusing "ValidationException: Model with name '...' already exists" because the prior run's resource still held the deterministic name, never surfacing the real cause. A new _reraise_if_access_denied helper re-raises AccessDeniedException as a PermissionError naming the missing IAM action; all other errors still fail open (warn and return None) so transient failures like throttling do not block a deploy. Tests: 86 unit tests pass across test_model_builder.py and test_model_reuse.py, including regression guards for both fixes (IC instance-type mismatch, build-time cache re-validation, access-denied-raises, and fail-open-on-other-error).
eliseharvey
marked this pull request as ready for review
July 28, 2026 01:04
amazeAmazing
approved these changes
Jul 28, 2026
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.
This PR fixes two independent P0 resource-reuse defects found during Nova SDK
dogfooding. Both occur only under
reuse_resources=Trueand share the reusecode paths, so they are fixed together.
1. instance_type mismatch silently reused
deploy()trusted the endpoint candidate cached bybuild()in_reused_endpoint_namewithout re-validating it. That cache is resolved atbuild time, before
instance_type(adeploy()argument) is known, so a deployon a different instance type reused the wrong endpoint. Separately,
_reused_endpoint_matches_configskipped theinstance_typecheck entirely forInference Component (IC) endpoints -- the IC early-return (no
ModelNameon theproduction variant) returned
Truebefore reaching the check.Fix:
deploy()now re-validates the cached endpoint against the requestedinstance_typeand falls back to a fresh discovery on a miss or mismatch._reused_endpoint_matches_configchecksinstance_type-- which lives on theproduction variant and is available for every endpoint, including IC ones --
before the IC early-return, so an instance-type mismatch is never silently
reused.
2. Bedrock reuse_resources=True fails open on missing permission
Reuse discovery (
find_existing_bedrock_model,find_active_bedrock_deployment_for_model,find_existing_sagemaker_endpoint)swallowed every exception and returned
None. When the execution role lacked aread permission (e.g.
bedrock:ListTagsForResource), a denied discovery callwas indistinguishable from "nothing to reuse", so reuse fell through to creating
the resource -- which then failed with a confusing
ValidationException: Model with name '...' already existsbecause the priorrun's resource still held the deterministic name, never surfacing the real
cause.
Fix: a new
_reraise_if_access_deniedhelper re-raisesAccessDeniedExceptionas aPermissionErrornaming the missing IAM action.All other errors still fail open (warn and return
None), so transient failureslike throttling do not block a deploy.
Testing
86 unit tests pass across
test_model_builder.pyandtest_model_reuse.py,including new regression guards:
deploy()re-validation of the build-time cache (mismatch ignores cache, match reuses it)PermissionErrorfor all three discovery functionsNoneIssue #, if available:
Description of changes: See above.
By submitting this pull request, I confirm that you can use, modify, copy, and
redistribute this contribution, under the terms of your choice.