From e6322a7330de64e9106260f288baea327e266e25 Mon Sep 17 00:00:00 2001 From: Rick Getz Date: Tue, 18 Nov 2025 12:25:34 -0500 Subject: [PATCH 01/14] feat: allow concurrent artifact uploads --- samcli/commands/deploy/command.py | 12 +++ samcli/commands/deploy/core/options.py | 1 + samcli/commands/deploy/deploy_context.py | 3 + samcli/commands/deploy/guided_context.py | 13 +++ samcli/commands/deploy/utils.py | 3 + samcli/commands/package/package_context.py | 3 + samcli/lib/package/artifact_exporter.py | 71 +++++++++++++- samcli/lib/package/ecr_uploader.py | 8 +- schema/samcli.json | 7 +- tests/unit/commands/deploy/test_command.py | 36 +++++-- .../lib/package/test_artifact_exporter.py | 93 +++++++++++++++++++ 11 files changed, 237 insertions(+), 13 deletions(-) diff --git a/samcli/commands/deploy/command.py b/samcli/commands/deploy/command.py index dd750ce5e58..ff0d7e2943d 100644 --- a/samcli/commands/deploy/command.py +++ b/samcli/commands/deploy/command.py @@ -146,6 +146,12 @@ @image_repository_option @image_repositories_option @force_upload_option +@click.option( + "--parallel-upload", + is_flag=True, + default=False, + help="Enable parallel upload of artifacts to S3/ECR during packaging before deployment.", +) @s3_prefix_option @kms_key_id_option @role_arn_option @@ -177,6 +183,7 @@ def cli( image_repository, image_repositories, force_upload, + parallel_upload, no_progressbar, s3_prefix, kms_key_id, @@ -212,6 +219,7 @@ def cli( image_repository, image_repositories, force_upload, + parallel_upload, no_progressbar, s3_prefix, kms_key_id, @@ -246,6 +254,7 @@ def do_cli( image_repository, image_repositories, force_upload, + parallel_upload, no_progressbar, s3_prefix, kms_key_id, @@ -300,6 +309,7 @@ def do_cli( config_env=config_env, config_file=config_file, disable_rollback=disable_rollback, + parallel_upload=parallel_upload, ) guided_context.run() else: @@ -331,6 +341,7 @@ def do_cli( kms_key_id=kms_key_id, use_json=use_json, force_upload=force_upload, + parallel_upload=guided_context.guided_parallel_upload if guided else parallel_upload, no_progressbar=no_progressbar, metadata=metadata, on_deploy=True, @@ -357,6 +368,7 @@ def do_cli( image_repository=guided_context.guided_image_repository if guided else image_repository, image_repositories=guided_context.guided_image_repositories if guided else image_repositories, force_upload=force_upload, + parallel_upload=guided_context.guided_parallel_upload if guided else parallel_upload, no_progressbar=no_progressbar, s3_prefix=guided_context.guided_s3_prefix if guided else s3_prefix, kms_key_id=kms_key_id, diff --git a/samcli/commands/deploy/core/options.py b/samcli/commands/deploy/core/options.py index 44503368af0..1af6dffdcb1 100644 --- a/samcli/commands/deploy/core/options.py +++ b/samcli/commands/deploy/core/options.py @@ -37,6 +37,7 @@ "disable_rollback", "on_failure", "force_upload", + "parallel_upload", "max_wait_duration", ] diff --git a/samcli/commands/deploy/deploy_context.py b/samcli/commands/deploy/deploy_context.py index 33ac1711568..70369100379 100644 --- a/samcli/commands/deploy/deploy_context.py +++ b/samcli/commands/deploy/deploy_context.py @@ -75,6 +75,7 @@ def __init__( poll_delay, on_failure, max_wait_duration, + parallel_upload=False, ): self.template_file = template_file self.stack_name = stack_name @@ -82,6 +83,7 @@ def __init__( self.image_repository = image_repository self.image_repositories = image_repositories self.force_upload = force_upload + self.parallel_upload = parallel_upload self.no_progressbar = no_progressbar self.s3_prefix = s3_prefix self.kms_key_id = kms_key_id @@ -164,6 +166,7 @@ def run(self): self.signing_profiles, self.use_changeset, self.disable_rollback, + self.parallel_upload, ) return self.deploy( self.stack_name, diff --git a/samcli/commands/deploy/guided_context.py b/samcli/commands/deploy/guided_context.py index 8657debd57e..a2030647af6 100644 --- a/samcli/commands/deploy/guided_context.py +++ b/samcli/commands/deploy/guided_context.py @@ -62,6 +62,7 @@ def __init__( config_env=None, config_file=None, disable_rollback=None, + parallel_upload=False, ): self.template_file = template_file self.stack_name = stack_name @@ -95,6 +96,8 @@ def __init__( self.color = Colored() self.function_provider = None self.disable_rollback = disable_rollback + self.parallel_upload = parallel_upload + self.guided_parallel_upload = None @property def guided_capabilities(self): @@ -162,6 +165,14 @@ def guided_prompts(self, parameter_override_keys): click.secho("\t#Preserves the state of previously provisioned resources when an operation fails") disable_rollback = confirm(f"\t{self.start_bold}Disable rollback{self.end_bold}", default=self.disable_rollback) + if self.parallel_upload: + parallel_upload = True + else: + click.secho("\t#Speed up artifact uploads by running them in parallel") + parallel_upload = confirm( + f"\t{self.start_bold}Enable parallel uploads{self.end_bold}", default=False + ) + self.prompt_authorization(stacks) self.prompt_code_signing_settings(stacks) @@ -204,6 +215,7 @@ def guided_prompts(self, parameter_override_keys): self.guided_s3_prefix = stack_name self.guided_region = region self.guided_profile = self.profile + self.guided_parallel_upload = parallel_upload self._capabilities = input_capabilities if input_capabilities else default_capabilities self._parameter_overrides = ( input_parameter_overrides if input_parameter_overrides else self.parameter_overrides_from_cmdline @@ -587,6 +599,7 @@ def run(self): capabilities=self._capabilities, signing_profiles=self.signing_profiles, disable_rollback=self.disable_rollback, + parallel_upload=self.guided_parallel_upload, ) @staticmethod diff --git a/samcli/commands/deploy/utils.py b/samcli/commands/deploy/utils.py index 421a16c75df..56270e5ae98 100644 --- a/samcli/commands/deploy/utils.py +++ b/samcli/commands/deploy/utils.py @@ -20,6 +20,7 @@ def print_deploy_args( signing_profiles, use_changeset, disable_rollback, + parallel_upload, ): """ Print a table of the values that are used during a sam deploy. @@ -48,6 +49,7 @@ def print_deploy_args( :param signing_profiles: Signing profile details which will be used to sign functions/layers :param use_changeset: Flag to use or skip the usage of changesets :param disable_rollback: Preserve the state of previously provisioned resources when an operation fails. + :param parallel_upload: Whether artifact uploads run in parallel prior to deployment. """ _parameters = parameter_overrides.copy() @@ -70,6 +72,7 @@ def print_deploy_args( if use_changeset: click.echo(f"\tConfirm changeset : {confirm_changeset}") click.echo(f"\tDisable rollback : {disable_rollback}") + click.echo(f"\tParallel uploads : {parallel_upload}") if image_repository: msg = "Deployment image repository : " # NOTE(sriram-mv): tab length is 8 spaces. diff --git a/samcli/commands/package/package_context.py b/samcli/commands/package/package_context.py index 4a649817458..1d813fef39a 100644 --- a/samcli/commands/package/package_context.py +++ b/samcli/commands/package/package_context.py @@ -71,6 +71,7 @@ def __init__( parameter_overrides=None, on_deploy=False, signing_profiles=None, + parallel_upload=False, ): self.template_file = template_file self.s3_bucket = s3_bucket @@ -81,6 +82,7 @@ def __init__( self.output_template_file = output_template_file self.use_json = use_json self.force_upload = force_upload + self.parallel_upload = parallel_upload self.no_progressbar = no_progressbar self.metadata = metadata self.region = region @@ -161,6 +163,7 @@ def _export(self, template_path, use_json): self.code_signer, normalize_template=True, normalize_parameters=True, + parallel_upload=self.parallel_upload, ) exported_template = template.export() diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index bc38ee4a5e2..dd8a6618381 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -15,7 +15,9 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. import os -from typing import Dict, List, Optional +import threading +from concurrent.futures import FIRST_EXCEPTION, ThreadPoolExecutor, wait +from typing import Callable, Dict, List, Optional from botocore.utils import set_value_from_jmespath @@ -52,6 +54,28 @@ # NOTE: sriram-mv, A cyclic dependency on `Template` needs to be broken. +DEFAULT_PARALLEL_UPLOAD_WORKERS = max(4, min(32, (os.cpu_count() or 1) * 2)) + + +class _ThreadSafeUploadCache: + """Simple thread-safe mapping used to deduplicate uploads across threads.""" + + def __init__(self, initial: Optional[Dict[str, str]] = None): + self._cache = initial or {} + self._lock = threading.Lock() + + def __contains__(self, key: str) -> bool: # pragma: no cover - small helper + with self._lock: + return key in self._cache + + def __getitem__(self, key: str) -> str: # pragma: no cover - small helper + with self._lock: + return self._cache[key] + + def __setitem__(self, key: str, value: str) -> None: # pragma: no cover - small helper + with self._lock: + self._cache[key] = value + class CloudFormationStackResource(ResourceZip): """ @@ -89,6 +113,7 @@ def do_export(self, resource_id, resource_dict, parent_dir): normalize_template=True, normalize_parameters=True, parent_stack_id=resource_id, + parallel_upload=getattr(self, "parallel_upload", False), ).export() exported_template_str = yaml_dump(exported_template_dict) @@ -179,6 +204,7 @@ def __init__( normalize_template: bool = False, normalize_parameters: bool = False, parent_stack_id: str = "", + parallel_upload: bool = False, ): """ Reads the template and makes it ready for export @@ -202,6 +228,7 @@ def __init__( self.metadata_to_export = metadata_to_export self.uploaders = uploaders self.parent_stack_id = parent_stack_id + self.parallel_upload = parallel_upload def _export_global_artifacts(self, template_dict: Dict) -> Dict: """ @@ -279,7 +306,10 @@ def export(self) -> Dict: cache: Optional[Dict] = None if is_experimental_enabled(ExperimentalFlag.PackagePerformance): cache = {} + if cache is not None and self.parallel_upload: + cache = _ThreadSafeUploadCache(cache) + export_jobs: List[Callable[[], None]] = [] for resource_logical_id, resource in self.template_dict["Resources"].items(): resource_type = resource.get("Type", None) resource_dict = resource.get("Properties", {}) @@ -291,12 +321,45 @@ def export(self) -> Dict: continue if resource_dict.get("PackageType", ZIP) != exporter_class.ARTIFACT_TYPE: continue - # Export code resources - exporter = exporter_class(self.uploaders, self.code_signer, cache) - exporter.export(full_path, resource_dict, self.template_dir) + + export_jobs.append( + self._build_export_job(exporter_class, full_path, resource_dict, cache) + ) + + if self.parallel_upload and export_jobs: + self._execute_jobs_in_parallel(export_jobs) + else: + for job in export_jobs: + job() return self.template_dict + def _build_export_job( + self, + exporter_class, + resource_full_path: str, + resource_dict: Dict, + cache: Optional[Dict], + ) -> Callable[[], None]: + def _job() -> None: + exporter = exporter_class(self.uploaders, self.code_signer, cache) + setattr(exporter, "parallel_upload", self.parallel_upload) + exporter.export(resource_full_path, resource_dict, self.template_dir) + + return _job + + def _execute_jobs_in_parallel(self, jobs: List[Callable[[], None]]) -> None: + max_workers = min(len(jobs), DEFAULT_PARALLEL_UPLOAD_WORKERS) + if max_workers <= 1: + jobs[0]() + return + + with ThreadPoolExecutor(max_workers=max_workers) as executor: + futures = [executor.submit(job) for job in jobs] + wait(futures, return_when=FIRST_EXCEPTION) + for future in futures: + future.result() + def delete(self, retain_resources: List): """ Deletes all the artifacts referenced by the given Cloudformation template diff --git a/samcli/lib/package/ecr_uploader.py b/samcli/lib/package/ecr_uploader.py index ae0337cfdb0..af93528999f 100644 --- a/samcli/lib/package/ecr_uploader.py +++ b/samcli/lib/package/ecr_uploader.py @@ -4,6 +4,7 @@ import base64 import logging +import threading from io import StringIO from pathlib import Path from typing import Dict @@ -50,6 +51,7 @@ def __init__( self.stream = StreamWriter(stream=stream, auto_flush=True) self.log_streamer = LogStreamer(stream=self.stream) self.login_session_active = False + self._login_lock = threading.Lock() @property def docker_client(self): @@ -88,8 +90,10 @@ def upload(self, image, resource_name): :return: remote ECR image path that has been uploaded. """ if not self.login_session_active: - self.login() - self.login_session_active = True + with self._login_lock: + if not self.login_session_active: + self.login() + self.login_session_active = True # Sometimes the `resource_name` is used as the `image` parameter to `tag_translation`. # This is because these two cases (directly from an archive or by ID) are effectively diff --git a/schema/samcli.json b/schema/samcli.json index 9c14395d644..17510947c1a 100644 --- a/schema/samcli.json +++ b/schema/samcli.json @@ -1227,6 +1227,11 @@ "type": "boolean", "description": "Indicates whether to override existing files in the S3 bucket. Specify this flag to upload artifacts even if they match existing artifacts in the S3 bucket." }, + "parallel_upload": { + "title": "parallel_upload", + "type": "boolean", + "description": "Enable parallel upload of artifacts to S3/ECR before deployment." + }, "s3_prefix": { "title": "s3_prefix", "type": "string", @@ -2346,4 +2351,4 @@ } } } -} \ No newline at end of file +} diff --git a/tests/unit/commands/deploy/test_command.py b/tests/unit/commands/deploy/test_command.py index 51b3b89bc8d..ab6c64b7f59 100644 --- a/tests/unit/commands/deploy/test_command.py +++ b/tests/unit/commands/deploy/test_command.py @@ -35,6 +35,7 @@ def setUp(self): self.fail_on_empty_changset = True self.role_arn = "role_arn" self.force_upload = False + self.parallel_upload = False self.no_progressbar = False self.metadata = {"abc": "def"} self.region = None @@ -81,6 +82,7 @@ def test_all_args(self, mock_deploy_context, mock_deploy_click, mock_package_con image_repository=self.image_repository, image_repositories=None, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix=self.s3_prefix, kms_key_id=self.kms_key_id, @@ -114,6 +116,7 @@ def test_all_args(self, mock_deploy_context, mock_deploy_click, mock_package_con image_repository=self.image_repository, image_repositories=None, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix=self.s3_prefix, kms_key_id=self.kms_key_id, @@ -171,7 +174,7 @@ def test_all_args_guided_no_to_authorization_confirmation_prompt( context_mock = Mock() mockauth_per_resource.return_value = [("HelloWorldResource1", False), ("HelloWorldResource2", False)] mock_deploy_context.return_value.__enter__.return_value = context_mock - mock_confirm.side_effect = [True, True, False, True, False] + mock_confirm.side_effect = [True, True, False, False, True, False] mock_prompt.side_effect = [ "sam-app", "us-east-1", @@ -199,6 +202,7 @@ def test_all_args_guided_no_to_authorization_confirmation_prompt( image_repository=None, image_repositories=None, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix=self.s3_prefix, kms_key_id=self.kms_key_id, @@ -268,7 +272,7 @@ def test_all_args_guided_use_defaults( mock_sam_function_provider.return_value.get_all.return_value = [function_mock] mockauth_per_resource.return_value = [("HelloWorldResource", False)] mock_deploy_context.return_value.__enter__.return_value = context_mock - mock_confirm.side_effect = [True, False, True, True, True, True, True] + mock_confirm.side_effect = [True, False, True, False, True, True, True, True] mock_prompt.side_effect = [ "sam-app", "us-east-1", @@ -301,6 +305,7 @@ def test_all_args_guided_use_defaults( image_repository=None, image_repositories=None, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix=self.s3_prefix, kms_key_id=self.kms_key_id, @@ -334,6 +339,7 @@ def test_all_args_guided_use_defaults( image_repository=None, image_repositories={"HelloWorldFunction": "123456789012.dkr.ecr.us-east-1.amazonaws.com/managed-ecr"}, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix="sam-app", kms_key_id=self.kms_key_id, @@ -374,6 +380,7 @@ def test_all_args_guided_use_defaults( s3_prefix="sam-app", signing_profiles=self.signing_profiles, disable_rollback=True, + parallel_upload=self.parallel_upload, ) mock_managed_stack.assert_called_with(profile=self.profile, region="us-east-1") self.assertEqual(context_mock.run.call_count, 1) @@ -419,7 +426,7 @@ def test_all_args_guided( mock_sam_function_provider.return_value.get_all.return_value = [function_mock] mockauth_per_resource.return_value = [("HelloWorldResource", False)] mock_deploy_context.return_value.__enter__.return_value = context_mock - mock_confirm.side_effect = [True, False, True, True, True, True, True] + mock_confirm.side_effect = [True, False, True, False, True, True, True, True] mock_prompt.side_effect = [ "sam-app", "us-east-1", @@ -447,6 +454,7 @@ def test_all_args_guided( image_repository=None, image_repositories=None, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix=self.s3_prefix, kms_key_id=self.kms_key_id, @@ -480,6 +488,7 @@ def test_all_args_guided( image_repository=None, image_repositories={"HelloWorldFunction": "123456789012.dkr.ecr.us-east-1.amazonaws.com/test1"}, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix="sam-app", kms_key_id=self.kms_key_id, @@ -520,6 +529,7 @@ def test_all_args_guided( s3_prefix="sam-app", signing_profiles=self.signing_profiles, disable_rollback=True, + parallel_upload=self.parallel_upload, ) mock_managed_stack.assert_called_with(profile=self.profile, region="us-east-1") self.assertEqual(context_mock.run.call_count, 1) @@ -584,7 +594,7 @@ def test_all_args_guided_no_save_echo_param_to_config( "testconfig.toml", "test-env", ] - mock_confirm.side_effect = [True, False, True, True, True, True, True] + mock_confirm.side_effect = [True, False, True, False, True, True, True, True] mock_managed_stack.return_value = "managed-s3-bucket" mock_signer_config_per_function.return_value = ({}, {}) @@ -596,6 +606,7 @@ def test_all_args_guided_no_save_echo_param_to_config( image_repository=None, image_repositories=None, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix=self.s3_prefix, kms_key_id=self.kms_key_id, @@ -629,6 +640,7 @@ def test_all_args_guided_no_save_echo_param_to_config( image_repository=None, image_repositories={"HelloWorldFunction": "123456789012.dkr.ecr.us-east-1.amazonaws.com/test1"}, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix="sam-app", kms_key_id=self.kms_key_id, @@ -745,7 +757,7 @@ def test_all_args_guided_no_params_save_config( "testconfig.toml", "test-env", ] - mock_confirm.side_effect = [True, False, True, True, True, True, True] + mock_confirm.side_effect = [True, False, True, False, True, True, True, True] mock_get_cmd_names.return_value = ["deploy"] mock_managed_stack.return_value = "managed-s3-bucket" mock_signer_config_per_function.return_value = ({}, {}) @@ -757,6 +769,7 @@ def test_all_args_guided_no_params_save_config( image_repository=None, image_repositories=None, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix=self.s3_prefix, kms_key_id=self.kms_key_id, @@ -790,6 +803,7 @@ def test_all_args_guided_no_params_save_config( image_repository=None, image_repositories={"HelloWorldFunction": "123456789012.dkr.ecr.us-east-1.amazonaws.com/test1"}, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix="sam-app", kms_key_id=self.kms_key_id, @@ -885,7 +899,7 @@ def test_all_args_guided_no_params_no_save_config( "us-east-1", ("CAPABILITY_IAM",), ] - mock_confirm.side_effect = [True, True, False, True, False, True, True] + mock_confirm.side_effect = [True, True, False, False, True, False, True, True] mock_managed_stack.return_value = "managed-s3-bucket" mock_signer_config_per_function.return_value = ({}, {}) @@ -898,6 +912,7 @@ def test_all_args_guided_no_params_no_save_config( image_repository=None, image_repositories=None, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix=self.s3_prefix, kms_key_id=self.kms_key_id, @@ -931,6 +946,7 @@ def test_all_args_guided_no_params_no_save_config( image_repository=None, image_repositories={"HelloWorldFunction": "123456789012.dkr.ecr.us-east-1.amazonaws.com/test1"}, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix="sam-app", kms_key_id=self.kms_key_id, @@ -976,6 +992,7 @@ def test_all_args_resolve_s3( image_repository=None, image_repositories=None, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix=self.s3_prefix, kms_key_id=self.kms_key_id, @@ -1007,6 +1024,7 @@ def test_all_args_resolve_s3( stack_name=self.stack_name, s3_bucket="managed-s3-bucket", force_upload=self.force_upload, + parallel_upload=self.parallel_upload, image_repository=None, image_repositories=None, no_progressbar=self.no_progressbar, @@ -1042,6 +1060,7 @@ def test_resolve_s3_and_s3_bucket_both_set(self): image_repository=None, image_repositories=None, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix=self.s3_prefix, kms_key_id=self.kms_key_id, @@ -1094,6 +1113,7 @@ def test_all_args_resolve_image_repos( image_repository=None, image_repositories=None, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix=self.s3_prefix, kms_key_id=self.kms_key_id, @@ -1125,6 +1145,7 @@ def test_all_args_resolve_image_repos( stack_name=self.stack_name, s3_bucket=self.s3_bucket, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, image_repository=None, image_repositories={"HelloWorldFunction1": self.image_repository}, no_progressbar=self.no_progressbar, @@ -1169,6 +1190,7 @@ def test_passing_parameter_overrides_to_context( image_repository=self.image_repository, image_repositories=None, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix=self.s3_prefix, kms_key_id=self.kms_key_id, @@ -1202,6 +1224,7 @@ def test_passing_parameter_overrides_to_context( image_repository=self.image_repository, image_repositories=None, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, s3_prefix=self.s3_prefix, kms_key_id=self.kms_key_id, @@ -1233,6 +1256,7 @@ def test_passing_parameter_overrides_to_context( kms_key_id=self.kms_key_id, use_json=self.use_json, force_upload=self.force_upload, + parallel_upload=self.parallel_upload, no_progressbar=self.no_progressbar, metadata=self.metadata, on_deploy=True, diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index c4d9e81fe90..301aa0609b3 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -22,6 +22,7 @@ CloudFormationStackResource, CloudFormationStackSetResource, ServerlessApplicationResource, + _ThreadSafeUploadCache, ) from samcli.lib.package.packageable_resources import ( GraphQLApiCodeResource, @@ -1201,6 +1202,7 @@ def test_export_cloudformation_stack(self, TemplateMock): normalize_parameters=True, normalize_template=True, parent_stack_id="id", + parallel_upload=False, ) template_instance_mock.export.assert_called_once_with() self.s3_uploader_mock.upload.assert_called_once_with(mock.ANY, mock.ANY) @@ -1377,6 +1379,7 @@ def test_export_serverless_application(self, TemplateMock): normalize_parameters=True, normalize_template=True, parent_stack_id="id", + parallel_upload=False, ) template_instance_mock.export.assert_called_once_with() self.s3_uploader_mock.upload.assert_called_once_with(mock.ANY, mock.ANY) @@ -1543,6 +1546,96 @@ def test_template_export(self, yaml_parse_mock): resource_type2_class.assert_called_once_with(self.uploaders_mock, self.code_signer_mock, None) resource_type2_instance.export.assert_called_once_with("Resource2", mock.ANY, template_dir) + @patch.object(Template, "_execute_jobs_in_parallel") + @patch("samcli.lib.package.artifact_exporter.yaml_parse") + def test_template_export_parallel_invokes_executor(self, yaml_parse_mock, executor_mock): + parent_dir = os.path.sep + template_dir = os.path.join(parent_dir, "foo", "bar") + template_path = os.path.join(template_dir, "path") + template_str = self.example_yaml_template() + + resource_type1_class = Mock() + resource_type1_class.RESOURCE_TYPE = "resource_type1" + resource_type1_class.ARTIFACT_TYPE = ZIP + resource_type1_class.EXPORT_DESTINATION = Destination.S3 + resource_type1_class.return_value = Mock() + + resource_type2_class = Mock() + resource_type2_class.RESOURCE_TYPE = "resource_type2" + resource_type2_class.ARTIFACT_TYPE = ZIP + resource_type2_class.EXPORT_DESTINATION = Destination.S3 + resource_type2_class.return_value = Mock() + + resources_to_export = [resource_type1_class, resource_type2_class] + properties = {"foo": "bar"} + template_dict = { + "Resources": { + "Resource1": {"Type": "resource_type1", "Properties": properties}, + "Resource2": {"Type": "resource_type2", "Properties": properties}, + } + } + + yaml_parse_mock.return_value = template_dict + with patch("samcli.lib.package.artifact_exporter.open", mock.mock_open(read_data=template_str)): + template_exporter = Template( + template_path, + parent_dir, + self.uploaders_mock, + self.code_signer_mock, + resources_to_export, + parallel_upload=True, + ) + template_exporter.export() + + executor_mock.assert_called_once() + jobs = executor_mock.call_args[0][0] + self.assertEqual(len(jobs), 2) + + @patch("samcli.lib.package.artifact_exporter.is_experimental_enabled") + @patch("samcli.lib.package.artifact_exporter.yaml_parse") + def test_template_export_parallel_wraps_cache(self, yaml_parse_mock, is_experimental_enabled_mock): + is_experimental_enabled_mock.side_effect = lambda *args: { + (ExperimentalFlag.PackagePerformance,): True, + }.get(args, False) + + parent_dir = os.path.sep + template_dir = os.path.join(parent_dir, "foo", "bar") + template_path = os.path.join(template_dir, "path") + template_str = self.example_yaml_template() + + resource_type_class = Mock() + resource_type_class.RESOURCE_TYPE = "resource_type1" + resource_type_class.ARTIFACT_TYPE = ZIP + resource_type_class.EXPORT_DESTINATION = Destination.S3 + resource_type_instance = Mock() + resource_type_class.return_value = resource_type_instance + + captured_cache = {} + + def capture_cache(uploaders, code_signer, cache): + nonlocal captured_cache + captured_cache = cache + return resource_type_instance + + resource_type_class.side_effect = capture_cache + + properties = {"foo": "bar"} + template_dict = {"Resources": {"Resource1": {"Type": "resource_type1", "Properties": properties}}} + + yaml_parse_mock.return_value = template_dict + with patch("samcli.lib.package.artifact_exporter.open", mock.mock_open(read_data=template_str)): + template_exporter = Template( + template_path, + parent_dir, + self.uploaders_mock, + self.code_signer_mock, + [resource_type_class], + parallel_upload=True, + ) + template_exporter.export() + + self.assertIsInstance(captured_cache, _ThreadSafeUploadCache) + @patch("samcli.lib.package.artifact_exporter.is_experimental_enabled") @patch("samcli.lib.package.artifact_exporter.yaml_parse") def test_template_export_with_experimental_flag(self, yaml_parse_mock, is_experimental_enabled_mock): From dae5be7980a4b1d5e75229432ecd909cec3d3525 Mon Sep 17 00:00:00 2001 From: Rick Getz Date: Tue, 18 Nov 2025 17:48:25 -0500 Subject: [PATCH 02/14] docs: update docs to inlcude parallel uploads flag --- docs/sam-config-docs.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/docs/sam-config-docs.md b/docs/sam-config-docs.md index 944a78c2884..48bdac676c6 100644 --- a/docs/sam-config-docs.md +++ b/docs/sam-config-docs.md @@ -23,6 +23,16 @@ output_template_file="packaged.yaml" [default.deploy.parameters] stack_name="using_config_file" + +### Specifying a Boolean deployment option + +``` +[default.deploy.parameters] +parallel_upload=true +``` + +Setting `parallel_upload` to `true` is equivalent to passing `--parallel-upload` on +`sam deploy`, enabling concurrent S3/ECR uploads during the packaging phase. capabilities="CAPABILITY_IAM" region="us-east-1" profile="srirammv" @@ -94,4 +104,3 @@ stack_name="using_config_file" [default.build.parameters] debug=true ``` - From 2f424b65efe68f4146c1b146509ab577fdca3d5b Mon Sep 17 00:00:00 2001 From: Rick Getz Date: Tue, 18 Nov 2025 18:00:21 -0500 Subject: [PATCH 03/14] chore: run black formatter --- samcli/commands/deploy/guided_context.py | 4 +--- samcli/lib/package/artifact_exporter.py | 4 +--- schema/samcli.json | 6 +++--- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/samcli/commands/deploy/guided_context.py b/samcli/commands/deploy/guided_context.py index a2030647af6..8c9edea9e4f 100644 --- a/samcli/commands/deploy/guided_context.py +++ b/samcli/commands/deploy/guided_context.py @@ -169,9 +169,7 @@ def guided_prompts(self, parameter_override_keys): parallel_upload = True else: click.secho("\t#Speed up artifact uploads by running them in parallel") - parallel_upload = confirm( - f"\t{self.start_bold}Enable parallel uploads{self.end_bold}", default=False - ) + parallel_upload = confirm(f"\t{self.start_bold}Enable parallel uploads{self.end_bold}", default=False) self.prompt_authorization(stacks) self.prompt_code_signing_settings(stacks) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index dd8a6618381..bf9a6f776b7 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -322,9 +322,7 @@ def export(self) -> Dict: if resource_dict.get("PackageType", ZIP) != exporter_class.ARTIFACT_TYPE: continue - export_jobs.append( - self._build_export_job(exporter_class, full_path, resource_dict, cache) - ) + export_jobs.append(self._build_export_job(exporter_class, full_path, resource_dict, cache)) if self.parallel_upload and export_jobs: self._execute_jobs_in_parallel(export_jobs) diff --git a/schema/samcli.json b/schema/samcli.json index 17510947c1a..115562fc5d0 100644 --- a/schema/samcli.json +++ b/schema/samcli.json @@ -1147,7 +1147,7 @@ "properties": { "parameters": { "title": "Parameters for the deploy command", - "description": "Available parameters for the deploy command:\n* guided:\nSpecify this flag to allow SAM CLI to guide you through the deployment using guided prompts.\n* template_file:\nAWS SAM template which references built artifacts for resources in the template. (if applicable)\n* no_execute_changeset:\nIndicates whether to execute the change set. Specify this flag to view stack changes before executing the change set.\n* fail_on_empty_changeset:\nSpecify whether AWS SAM CLI should return a non-zero exit code if there are no changes to be made to the stack. Defaults to a non-zero exit code.\n* confirm_changeset:\nPrompt to confirm if the computed changeset is to be deployed by SAM CLI.\n* disable_rollback:\nPreserves the state of previously provisioned resources when an operation fails.\n* on_failure:\nProvide an action to determine what will happen when a stack fails to create. Three actions are available:\n\n- ROLLBACK: This will rollback a stack to a previous known good state.\n\n- DELETE: The stack will rollback to a previous state if one exists, otherwise the stack will be deleted.\n\n- DO_NOTHING: The stack will not rollback or delete, this is the same as disabling rollback.\n\nDefault behaviour is ROLLBACK.\n\n\n\nThis option is mutually exclusive with --disable-rollback/--no-disable-rollback. You can provide\n--on-failure or --disable-rollback/--no-disable-rollback but not both at the same time.\n* max_wait_duration:\nMaximum duration in minutes to wait for the deployment to complete.\n* stack_name:\nName of the AWS CloudFormation stack.\n* s3_bucket:\nAWS S3 bucket where artifacts referenced in the template are uploaded.\n* image_repository:\nAWS ECR repository URI where artifacts referenced in the template are uploaded.\n* image_repositories:\nMapping of Function Logical ID to AWS ECR Repository URI.\n\nExample: Function_Logical_ID=ECR_Repo_Uri\nThis option can be specified multiple times.\n* force_upload:\nIndicates whether to override existing files in the S3 bucket. Specify this flag to upload artifacts even if they match existing artifacts in the S3 bucket.\n* s3_prefix:\nPrefix name that is added to the artifact's name when it is uploaded to the AWS S3 bucket.\n* kms_key_id:\nThe ID of an AWS KMS key that is used to encrypt artifacts that are at rest in the AWS S3 bucket.\n* role_arn:\nARN of an IAM role that AWS Cloudformation assumes when executing a deployment change set.\n* use_json:\nIndicates whether to use JSON as the format for the output AWS CloudFormation template. YAML is used by default.\n* resolve_s3:\nAutomatically resolve AWS S3 bucket for non-guided deployments. Enabling this option will also create a managed default AWS S3 bucket for you. If one does not provide a --s3-bucket value, the managed bucket will be used. Do not use --guided with this option.\n* resolve_image_repos:\nAutomatically create and delete ECR repositories for image-based functions in non-guided deployments. A companion stack containing ECR repos for each function will be deployed along with the template stack. Automatically created image repositories will be deleted if the corresponding functions are removed.\n* metadata:\nMap of metadata to attach to ALL the artifacts that are referenced in the template.\n* notification_arns:\nARNs of SNS topics that AWS Cloudformation associates with the stack.\n* tags:\nList of tags to associate with the stack.\n* parameter_overrides:\nString that contains AWS CloudFormation parameter overrides encoded as key=value pairs.\n* signing_profiles:\nA string that contains Code Sign configuration parameters as FunctionOrLayerNameToSign=SigningProfileName:SigningProfileOwner Since signing profile owner is optional, it could also be written as FunctionOrLayerNameToSign=SigningProfileName\n* no_progressbar:\nDoes not showcase a progress bar when uploading artifacts to S3 and pushing docker images to ECR\n* capabilities:\nList of capabilities that one must specify before AWS Cloudformation can create certain stacks.\n\nAccepted Values: CAPABILITY_IAM, CAPABILITY_NAMED_IAM, CAPABILITY_RESOURCE_POLICY, CAPABILITY_AUTO_EXPAND.\n\nLearn more at: https://docs.aws.amazon.com/serverlessrepo/latest/devguide/acknowledging-application-capabilities.html\n* profile:\nSelect a specific profile from your credential file to get AWS credentials.\n* region:\nSet the AWS Region of the service. (e.g. us-east-1)\n* beta_features:\nEnable/Disable beta features.\n* debug:\nTurn on debug logging to print debug message generated by AWS SAM CLI and display timestamps.\n* save_params:\nSave the parameters provided via the command line to the configuration file.", + "description": "Available parameters for the deploy command:\n* guided:\nSpecify this flag to allow SAM CLI to guide you through the deployment using guided prompts.\n* template_file:\nAWS SAM template which references built artifacts for resources in the template. (if applicable)\n* no_execute_changeset:\nIndicates whether to execute the change set. Specify this flag to view stack changes before executing the change set.\n* fail_on_empty_changeset:\nSpecify whether AWS SAM CLI should return a non-zero exit code if there are no changes to be made to the stack. Defaults to a non-zero exit code.\n* confirm_changeset:\nPrompt to confirm if the computed changeset is to be deployed by SAM CLI.\n* disable_rollback:\nPreserves the state of previously provisioned resources when an operation fails.\n* on_failure:\nProvide an action to determine what will happen when a stack fails to create. Three actions are available:\n\n- ROLLBACK: This will rollback a stack to a previous known good state.\n\n- DELETE: The stack will rollback to a previous state if one exists, otherwise the stack will be deleted.\n\n- DO_NOTHING: The stack will not rollback or delete, this is the same as disabling rollback.\n\nDefault behaviour is ROLLBACK.\n\n\n\nThis option is mutually exclusive with --disable-rollback/--no-disable-rollback. You can provide\n--on-failure or --disable-rollback/--no-disable-rollback but not both at the same time.\n* max_wait_duration:\nMaximum duration in minutes to wait for the deployment to complete.\n* stack_name:\nName of the AWS CloudFormation stack.\n* s3_bucket:\nAWS S3 bucket where artifacts referenced in the template are uploaded.\n* image_repository:\nAWS ECR repository URI where artifacts referenced in the template are uploaded.\n* image_repositories:\nMapping of Function Logical ID to AWS ECR Repository URI.\n\nExample: Function_Logical_ID=ECR_Repo_Uri\nThis option can be specified multiple times.\n* force_upload:\nIndicates whether to override existing files in the S3 bucket. Specify this flag to upload artifacts even if they match existing artifacts in the S3 bucket.\n* parallel_upload:\nEnable parallel upload of artifacts to S3/ECR during packaging before deployment.\n* s3_prefix:\nPrefix name that is added to the artifact's name when it is uploaded to the AWS S3 bucket.\n* kms_key_id:\nThe ID of an AWS KMS key that is used to encrypt artifacts that are at rest in the AWS S3 bucket.\n* role_arn:\nARN of an IAM role that AWS Cloudformation assumes when executing a deployment change set.\n* use_json:\nIndicates whether to use JSON as the format for the output AWS CloudFormation template. YAML is used by default.\n* resolve_s3:\nAutomatically resolve AWS S3 bucket for non-guided deployments. Enabling this option will also create a managed default AWS S3 bucket for you. If one does not provide a --s3-bucket value, the managed bucket will be used. Do not use --guided with this option.\n* resolve_image_repos:\nAutomatically create and delete ECR repositories for image-based functions in non-guided deployments. A companion stack containing ECR repos for each function will be deployed along with the template stack. Automatically created image repositories will be deleted if the corresponding functions are removed.\n* metadata:\nMap of metadata to attach to ALL the artifacts that are referenced in the template.\n* notification_arns:\nARNs of SNS topics that AWS Cloudformation associates with the stack.\n* tags:\nList of tags to associate with the stack.\n* parameter_overrides:\nString that contains AWS CloudFormation parameter overrides encoded as key=value pairs.\n* signing_profiles:\nA string that contains Code Sign configuration parameters as FunctionOrLayerNameToSign=SigningProfileName:SigningProfileOwner Since signing profile owner is optional, it could also be written as FunctionOrLayerNameToSign=SigningProfileName\n* no_progressbar:\nDoes not showcase a progress bar when uploading artifacts to S3 and pushing docker images to ECR\n* capabilities:\nList of capabilities that one must specify before AWS Cloudformation can create certain stacks.\n\nAccepted Values: CAPABILITY_IAM, CAPABILITY_NAMED_IAM, CAPABILITY_RESOURCE_POLICY, CAPABILITY_AUTO_EXPAND.\n\nLearn more at: https://docs.aws.amazon.com/serverlessrepo/latest/devguide/acknowledging-application-capabilities.html\n* profile:\nSelect a specific profile from your credential file to get AWS credentials.\n* region:\nSet the AWS Region of the service. (e.g. us-east-1)\n* beta_features:\nEnable/Disable beta features.\n* debug:\nTurn on debug logging to print debug message generated by AWS SAM CLI and display timestamps.\n* save_params:\nSave the parameters provided via the command line to the configuration file.", "type": "object", "properties": { "guided": { @@ -1230,7 +1230,7 @@ "parallel_upload": { "title": "parallel_upload", "type": "boolean", - "description": "Enable parallel upload of artifacts to S3/ECR before deployment." + "description": "Enable parallel upload of artifacts to S3/ECR during packaging before deployment." }, "s3_prefix": { "title": "s3_prefix", @@ -2351,4 +2351,4 @@ } } } -} +} \ No newline at end of file From 5153c9e59acd703738f4363c494b736575642a9c Mon Sep 17 00:00:00 2001 From: Rick Getz Date: Tue, 18 Nov 2025 18:14:37 -0500 Subject: [PATCH 04/14] test: ensure parallel-upload is recognized in tests --- samcli/lib/package/artifact_exporter.py | 28 +++++++---- .../commands/deploy/test_guided_context.py | 46 +++++++++++++------ .../unit/commands/samconfig/test_samconfig.py | 2 + 3 files changed, 52 insertions(+), 24 deletions(-) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index bf9a6f776b7..32cf22d1dfd 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -16,6 +16,7 @@ # language governing permissions and limitations under the License. import os import threading +from collections.abc import MutableMapping from concurrent.futures import FIRST_EXCEPTION, ThreadPoolExecutor, wait from typing import Callable, Dict, List, Optional @@ -57,17 +58,14 @@ DEFAULT_PARALLEL_UPLOAD_WORKERS = max(4, min(32, (os.cpu_count() or 1) * 2)) -class _ThreadSafeUploadCache: +class _ThreadSafeUploadCache(MutableMapping[str, str]): """Simple thread-safe mapping used to deduplicate uploads across threads.""" - def __init__(self, initial: Optional[Dict[str, str]] = None): - self._cache = initial or {} + def __init__(self, initial: Optional[MutableMapping[str, str]] = None): + # Copy into a regular dict so we can safely snapshot under a lock + self._cache: Dict[str, str] = dict(initial or {}) self._lock = threading.Lock() - def __contains__(self, key: str) -> bool: # pragma: no cover - small helper - with self._lock: - return key in self._cache - def __getitem__(self, key: str) -> str: # pragma: no cover - small helper with self._lock: return self._cache[key] @@ -76,6 +74,18 @@ def __setitem__(self, key: str, value: str) -> None: # pragma: no cover - small with self._lock: self._cache[key] = value + def __delitem__(self, key: str) -> None: # pragma: no cover - small helper + with self._lock: + del self._cache[key] + + def __iter__(self): # pragma: no cover - small helper + with self._lock: + return iter(dict(self._cache)) + + def __len__(self) -> int: # pragma: no cover - small helper + with self._lock: + return len(self._cache) + class CloudFormationStackResource(ResourceZip): """ @@ -303,7 +313,7 @@ def export(self) -> Dict: self._apply_global_values() self.template_dict = self._export_global_artifacts(self.template_dict) - cache: Optional[Dict] = None + cache: Optional[MutableMapping[str, str]] = None if is_experimental_enabled(ExperimentalFlag.PackagePerformance): cache = {} if cache is not None and self.parallel_upload: @@ -337,7 +347,7 @@ def _build_export_job( exporter_class, resource_full_path: str, resource_dict: Dict, - cache: Optional[Dict], + cache: Optional[MutableMapping[str, str]], ) -> Callable[[], None]: def _job() -> None: exporter = exporter_class(self.uploaders, self.code_signer, cache) diff --git a/tests/unit/commands/deploy/test_guided_context.py b/tests/unit/commands/deploy/test_guided_context.py index b52c8fbac51..cce1e1f51e9 100644 --- a/tests/unit/commands/deploy/test_guided_context.py +++ b/tests/unit/commands/deploy/test_guided_context.py @@ -73,7 +73,7 @@ def test_guided_prompts_check_defaults_non_public_resources_zips( patched_auth_per_resource.return_value = [ ("HelloWorldFunction", True), ] - patched_confirm.side_effect = [True, False, False, "", True, True, True] + patched_confirm.side_effect = [True, False, False, False, "", True, True, True] patched_manage_stack.return_value = "managed_s3_stack" patched_signer_config_per_function.return_value = ({}, {}) self.gc.guided_prompts(parameter_override_keys=None) @@ -82,6 +82,7 @@ def test_guided_prompts_check_defaults_non_public_resources_zips( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), + call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call(f"\t{self.gc.start_bold}Save arguments to configuration file{self.gc.end_bold}", default=True), call( f"\t {self.gc.start_bold}Delete the unreferenced repositories listed above when deploying?{self.gc.end_bold}", @@ -125,7 +126,7 @@ def test_guided_prompts_check_defaults_public_resources_zips( patched_get_buildable_stacks.return_value = (Mock(), []) # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] - patched_confirm.side_effect = [True, False, False, True, False, True, True] + patched_confirm.side_effect = [True, False, False, False, True, False, True, True] patched_manage_stack.return_value = "managed_s3_stack" self.gc.guided_prompts(parameter_override_keys=None) # Now to check for all the defaults on confirmations. @@ -133,6 +134,7 @@ def test_guided_prompts_check_defaults_public_resources_zips( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), + call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, @@ -192,7 +194,7 @@ def test_guided_prompts_check_defaults_public_resources_images( # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] get_resource_full_path_by_id_mock.return_value = None - patched_confirm.side_effect = [True, False, False, True, False, True, True] + patched_confirm.side_effect = [True, False, False, False, True, False, True, True] patched_manage_stack.return_value = "managed_s3_stack" self.gc.guided_prompts(parameter_override_keys=None) # Now to check for all the defaults on confirmations. @@ -200,6 +202,7 @@ def test_guided_prompts_check_defaults_public_resources_images( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), + call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, @@ -230,6 +233,7 @@ def test_guided_prompts_check_defaults_public_resources_images( call("\t#Shows you resources changes to be deployed and require a 'Y' to initiate deploy"), call("\t#SAM needs permission to be able to create roles to connect to the resources in your template"), call("\t#Preserves the state of previously provisioned resources when an operation fails"), + call("\t#Speed up artifact uploads by running them in parallel"), call("\n\tManaged S3 bucket: managed_s3_stack", bold=True), ] self.assertEqual(expected_click_secho_calls, patched_click_secho.call_args_list) @@ -270,7 +274,7 @@ def test_guided_prompts_check_defaults_public_resources_images_ecr_url( # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] get_resource_full_path_by_id_mock.return_value = "RandomFunction" - patched_confirm.side_effect = [True, False, False, True, False, True, True] + patched_confirm.side_effect = [True, False, False, False, True, False, True, True] patched_manage_stack.return_value = "managed_s3_stack" patched_signer_config_per_function.return_value = ({}, {}) self.gc.guided_prompts(parameter_override_keys=None) @@ -279,6 +283,7 @@ def test_guided_prompts_check_defaults_public_resources_images_ecr_url( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), + call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, @@ -307,6 +312,7 @@ def test_guided_prompts_check_defaults_public_resources_images_ecr_url( call("\t#Shows you resources changes to be deployed and require a 'Y' to initiate deploy"), call("\t#SAM needs permission to be able to create roles to connect to the resources in your template"), call("\t#Preserves the state of previously provisioned resources when an operation fails"), + call("\t#Speed up artifact uploads by running them in parallel"), call("\n\tManaged S3 bucket: managed_s3_stack", bold=True), ] self.assertEqual(expected_click_secho_calls, patched_click_secho.call_args_list) @@ -347,7 +353,7 @@ def test_guided_prompts_images_illegal_image_uri( # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] get_resource_full_path_by_id_mock.return_value = "RandomFunction" - patched_confirm.side_effect = [True, False, False, True, False, False, True] + patched_confirm.side_effect = [True, False, False, False, True, False, False, True] patched_manage_stack.return_value = "managed_s3_stack" patched_signer_config_per_function.return_value = ({}, {}) with self.assertRaises(GuidedDeployFailedError): @@ -393,7 +399,7 @@ def test_guided_prompts_images_missing_repo( ] # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] - patched_confirm.side_effect = [True, False, False, True, False, True, True] + patched_confirm.side_effect = [True, False, False, False, True, False, True, True] patched_manage_stack.return_value = "managed_s3_stack" patched_signer_config_per_function.return_value = ({}, {}) @@ -403,6 +409,7 @@ def test_guided_prompts_images_missing_repo( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), + call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, @@ -431,6 +438,7 @@ def test_guided_prompts_images_missing_repo( call("\t#Shows you resources changes to be deployed and require a 'Y' to initiate deploy"), call("\t#SAM needs permission to be able to create roles to connect to the resources in your template"), call("\t#Preserves the state of previously provisioned resources when an operation fails"), + call("\t#Speed up artifact uploads by running them in parallel"), call("\n\tManaged S3 bucket: managed_s3_stack", bold=True), ] self.assertEqual(expected_click_secho_calls, patched_click_secho.call_args_list) @@ -472,7 +480,7 @@ def test_guided_prompts_images_no_repo( # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] patched_get_resource_full_path_by_id.return_value = "RandomFunction" - patched_confirm.side_effect = [True, False, False, True, False, False, True] + patched_confirm.side_effect = [True, False, False, False, True, False, False, True] patched_manage_stack.return_value = "managed_s3_stack" patched_signer_config_per_function.return_value = ({}, {}) @@ -482,6 +490,7 @@ def test_guided_prompts_images_no_repo( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), + call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, @@ -514,6 +523,7 @@ def test_guided_prompts_images_no_repo( call("\t#Shows you resources changes to be deployed and require a 'Y' to initiate deploy"), call("\t#SAM needs permission to be able to create roles to connect to the resources in your template"), call("\t#Preserves the state of previously provisioned resources when an operation fails"), + call("\t#Speed up artifact uploads by running them in parallel"), call("\n\tManaged S3 bucket: managed_s3_stack", bold=True), ] self.assertEqual(expected_click_secho_calls, patched_click_secho.call_args_list) @@ -554,7 +564,7 @@ def test_guided_prompts_images_deny_deletion( ] # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] - patched_confirm.side_effect = [True, False, False, True, False, True, False] + patched_confirm.side_effect = [True, False, False, False, True, False, True, False] patched_manage_stack.return_value = "managed_s3_stack" patched_signer_config_per_function.return_value = ({}, {}) with self.assertRaises(GuidedDeployFailedError): @@ -597,7 +607,7 @@ def test_guided_prompts_images_blank_image_repository( ] # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] - patched_confirm.side_effect = [True, False, False, True, False, False, True] + patched_confirm.side_effect = [True, False, False, False, True, False, False, True] patched_manage_stack.return_value = "managed_s3_stack" patched_signer_config_per_function.return_value = ({}, {}) with self.assertRaises(GuidedDeployFailedError): @@ -642,13 +652,14 @@ def test_guided_prompts_with_given_capabilities( patched_get_buildable_stacks.return_value = (Mock(), []) self.gc.capabilities = given_capabilities # Series of inputs to confirmations so that full range of questions are asked. - patched_confirm.side_effect = [True, False, False, "", True, True, True] + patched_confirm.side_effect = [True, False, False, False, "", True, True, True] self.gc.guided_prompts(parameter_override_keys=None) # Now to check for all the defaults on confirmations. expected_confirmation_calls = [ call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), + call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call(f"\t{self.gc.start_bold}Save arguments to configuration file{self.gc.end_bold}", default=True), call( f"\t {self.gc.start_bold}Delete the unreferenced repositories listed above when deploying?{self.gc.end_bold}", @@ -690,7 +701,7 @@ def test_guided_prompts_check_configuration_file_prompt_calls( patched_signer_config_per_function.return_value = ({}, {}) # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] - patched_confirm.side_effect = [True, False, False, True, True, True, True] + patched_confirm.side_effect = [True, False, False, False, True, True, True, True] patched_manage_stack.return_value = "managed_s3_stack" patched_get_resource_full_path_by_id.return_value = "RandomFunction" self.gc.guided_prompts(parameter_override_keys=None) @@ -699,6 +710,7 @@ def test_guided_prompts_check_configuration_file_prompt_calls( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), + call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, @@ -752,7 +764,7 @@ def test_guided_prompts_check_parameter_from_template( # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] patched_get_resource_full_path_by_id.return_value = "RandomFunction" - patched_confirm.side_effect = [True, False, False, True, False, True, True] + patched_confirm.side_effect = [True, False, False, False, True, False, True, True] patched_manage_stack.return_value = "managed_s3_stack" patched_signer_config_per_function.return_value = ({}, {}) parameter_override_from_template = {"MyTestKey": {"Default": "MyTemplateDefaultVal"}} @@ -763,6 +775,7 @@ def test_guided_prompts_check_parameter_from_template( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), + call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, @@ -811,7 +824,7 @@ def test_guided_prompts_check_parameter_from_cmd_or_config( # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] patched_get_resource_full_path_by_id.return_value = "RandomFunction" - patched_confirm.side_effect = [True, False, False, True, False, True, True] + patched_confirm.side_effect = [True, False, False, False, True, False, True, True] patched_signer_config_per_function.return_value = ({}, {}) patched_manage_stack.return_value = "managed_s3_stack" parameter_override_from_template = {"MyTestKey": {"Default": "MyTemplateDefaultVal"}} @@ -822,6 +835,7 @@ def test_guided_prompts_check_parameter_from_cmd_or_config( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), + call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, @@ -884,7 +898,7 @@ def test_guided_prompts_with_code_signing( patched_signer_config_per_function.return_value = given_code_signing_configs patched_get_buildable_stacks.return_value = (Mock(), []) # Series of inputs to confirmations so that full range of questions are asked. - patched_confirm.side_effect = [True, False, False, given_sign_packages_flag, "", True, True, True] + patched_confirm.side_effect = [True, False, False, False, given_sign_packages_flag, "", True, True, True] patched_get_resource_full_path_by_id.return_value = "RandomFunction" self.gc.guided_prompts(parameter_override_keys=None) # Now to check for all the defaults on confirmations. @@ -892,6 +906,7 @@ def test_guided_prompts_with_code_signing( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), + call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}Do you want to sign your code?{self.gc.end_bold}", default=True, @@ -955,7 +970,7 @@ def test_guided_prompts_check_default_config_region( # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] patched_get_resource_full_path_by_id.return_value = "RandomFunction" - patched_confirm.side_effect = [True, False, False, True, True, True, True] + patched_confirm.side_effect = [True, False, False, False, True, True, True, True] patched_signer_config_per_function.return_value = ({}, {}) patched_manage_stack.return_value = "managed_s3_stack" patched_get_default_aws_region.return_value = "default_config_region" @@ -967,6 +982,7 @@ def test_guided_prompts_check_default_config_region( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), + call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, diff --git a/tests/unit/commands/samconfig/test_samconfig.py b/tests/unit/commands/samconfig/test_samconfig.py index 1a7b929c5d1..53380c3f7a0 100644 --- a/tests/unit/commands/samconfig/test_samconfig.py +++ b/tests/unit/commands/samconfig/test_samconfig.py @@ -934,6 +934,7 @@ def test_deploy(self, do_cli_mock, template_artifacts_mock1, template_artifacts_ None, True, False, + False, "myprefix", "mykms", {"Key": "Value"}, @@ -1049,6 +1050,7 @@ def test_deploy_different_parameter_override_format( None, True, False, + False, "myprefix", "mykms", {"Key1": "Value1", "Key2": "Multiple spaces in the value"}, From f4a5f0b2c2f250968813700acc1f8638a2f1df66 Mon Sep 17 00:00:00 2001 From: Rick Getz Date: Tue, 18 Nov 2025 21:41:31 -0500 Subject: [PATCH 05/14] chore: increase docker timeout --- samcli/local/docker/container_client.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/samcli/local/docker/container_client.py b/samcli/local/docker/container_client.py index 5a1bebc468f..815274ca558 100644 --- a/samcli/local/docker/container_client.py +++ b/samcli/local/docker/container_client.py @@ -96,6 +96,9 @@ def __init__(self, client_version, base_url=None): # Specify minimum version client_params["version"] = client_version + # Increase client timeout to tolerate longer pushes/pulls + client_params["timeout"] = int(os.environ.get("SAM_CLI_DOCKER_TIMEOUT", "600")) + # Initialize DockerClient with processed parameters LOG.debug(f"Creating container client with parameters: {client_params}") super().__init__(**client_params) From af6cb4a478fdcb2bb3d928b16d5084623aa1a13e Mon Sep 17 00:00:00 2001 From: Rick Getz Date: Wed, 7 Jan 2026 23:43:31 -0500 Subject: [PATCH 06/14] fix: docker client init and add tests for parallel upload paths --- samcli/local/docker/container_client.py | 2 +- tests/unit/commands/deploy/test_utils.py | 66 +++++++++++++ .../unit/hook_packages/terraform/test_main.py | 12 +++ .../lib/package/test_artifact_exporter.py | 98 +++++++++++++++++++ .../lib/package/test_stream_cursor_utils.py | 4 + tests/unit/lib/utils/test_profile.py | 11 +++ 6 files changed, 192 insertions(+), 1 deletion(-) create mode 100644 tests/unit/commands/deploy/test_utils.py create mode 100644 tests/unit/hook_packages/terraform/test_main.py create mode 100644 tests/unit/lib/utils/test_profile.py diff --git a/samcli/local/docker/container_client.py b/samcli/local/docker/container_client.py index bcb6d78202e..7b83f33f5c7 100644 --- a/samcli/local/docker/container_client.py +++ b/samcli/local/docker/container_client.py @@ -98,7 +98,7 @@ def __init__(self, base_url=None): self.client_params["version"] = os.environ.get(GlobalConfig.DOCKER_API_ENV_VAR, DOCKER_MIN_API_VERSION) # Increase client timeout to tolerate longer pushes/pulls - client_params["timeout"] = int(os.environ.get("SAM_CLI_DOCKER_TIMEOUT", "600")) + self.client_params["timeout"] = int(os.environ.get("SAM_CLI_DOCKER_TIMEOUT", "600")) # Initialize DockerClient with processed parameters LOG.debug(f"Creating container client with parameters: {self.client_params}") diff --git a/tests/unit/commands/deploy/test_utils.py b/tests/unit/commands/deploy/test_utils.py new file mode 100644 index 00000000000..51d141e0f85 --- /dev/null +++ b/tests/unit/commands/deploy/test_utils.py @@ -0,0 +1,66 @@ +from unittest import TestCase +from unittest.mock import patch + +from samcli.commands.deploy.utils import hide_noecho_parameter_overrides, print_deploy_args, sanitize_parameter_overrides + + +class TestDeployUtils(TestCase): + @patch("samcli.commands.deploy.utils.click.secho") + @patch("samcli.commands.deploy.utils.click.echo") + def test_print_deploy_args_prints_parallel_upload_and_optional_sections(self, echo_mock, secho_mock): + print_deploy_args( + stack_name="stack", + s3_bucket="bucket", + image_repository={"MyFunc": "123.dkr.ecr.us-east-1.amazonaws.com/repo"}, + region="us-east-1", + capabilities=["CAPABILITY_IAM"], + parameter_overrides={"Param": "Value"}, + confirm_changeset=False, + signing_profiles={"MyFunc": {"profile_name": "pname", "profile_owner": "powner"}}, + use_changeset=True, + disable_rollback=False, + parallel_upload=True, + ) + + echo_texts = [call.args[0] for call in echo_mock.call_args_list] + self.assertTrue(any("Parallel uploads" in text for text in echo_texts)) + self.assertTrue(any("Confirm changeset" in text for text in echo_texts)) + self.assertTrue(any("Deployment image repository" in text for text in echo_texts)) + + # Basic smoke check that we printed the header/footer sections. + self.assertGreaterEqual(secho_mock.call_count, 2) + + @patch("samcli.commands.deploy.utils.click.secho") + @patch("samcli.commands.deploy.utils.click.echo") + def test_print_deploy_args_without_optional_sections(self, echo_mock, secho_mock): + print_deploy_args( + stack_name="stack", + s3_bucket="bucket", + image_repository=None, + region="us-east-1", + capabilities=["CAPABILITY_IAM"], + parameter_overrides={"Param": "Value"}, + confirm_changeset=False, + signing_profiles=None, + use_changeset=False, + disable_rollback=False, + parallel_upload=False, + ) + + echo_texts = [call.args[0] for call in echo_mock.call_args_list] + self.assertFalse(any("Confirm changeset" in text for text in echo_texts)) + self.assertFalse(any("Deployment image repository" in text for text in echo_texts)) + + def test_sanitize_parameter_overrides(self): + self.assertEqual( + {"A": "1", "B": "2"}, + sanitize_parameter_overrides({"A": {"Value": "1"}, "B": "2"}), + ) + + def test_hide_noecho_parameter_overrides(self): + template_parameters = {"Parameters": {"Secret": {"NoEcho": True}, "Visible": {"NoEcho": False}}} + overrides = {"Secret": "shh", "Visible": "ok"} + self.assertEqual( + {"Secret": "*" * 5, "Visible": "ok"}, + hide_noecho_parameter_overrides(template_parameters, overrides), + ) diff --git a/tests/unit/hook_packages/terraform/test_main.py b/tests/unit/hook_packages/terraform/test_main.py new file mode 100644 index 00000000000..52020863426 --- /dev/null +++ b/tests/unit/hook_packages/terraform/test_main.py @@ -0,0 +1,12 @@ +from unittest import TestCase +from unittest.mock import patch + +from samcli.hook_packages.terraform import main + + +class TestTerraformHookEntrypoints(TestCase): + @patch("samcli.hook_packages.terraform.main.prepare_hook") + def test_prepare_delegates_to_prepare_hook(self, prepare_hook_mock): + prepare_hook_mock.return_value = {"ok": True} + self.assertEqual({"ok": True}, main.prepare({"hello": "world"})) + prepare_hook_mock.assert_called_once_with({"hello": "world"}) diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 301aa0609b3..fecc75651f3 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -1636,6 +1636,104 @@ def capture_cache(uploaders, code_signer, cache): self.assertIsInstance(captured_cache, _ThreadSafeUploadCache) + @patch("samcli.lib.package.artifact_exporter.yaml_parse") + def test_template_export_skips_mismatched_package_type(self, yaml_parse_mock): + parent_dir = os.path.sep + template_dir = os.path.join(parent_dir, "foo", "bar") + template_path = os.path.join(template_dir, "path") + template_str = self.example_yaml_template() + + resource_type_class = Mock() + resource_type_class.RESOURCE_TYPE = "resource_type1" + resource_type_class.ARTIFACT_TYPE = ZIP + resource_type_class.EXPORT_DESTINATION = Destination.S3 + + # Same resource type, but template says Image package type -> should be skipped + properties = {"PackageType": IMAGE, "foo": "bar"} + template_dict = {"Resources": {"Resource1": {"Type": "resource_type1", "Properties": properties}}} + yaml_parse_mock.return_value = template_dict + + with patch("samcli.lib.package.artifact_exporter.open", mock.mock_open(read_data=template_str)): + template_exporter = Template( + template_path, parent_dir, self.uploaders_mock, self.code_signer_mock, [resource_type_class] + ) + template_exporter.export() + + resource_type_class.assert_not_called() + + @patch("samcli.lib.package.artifact_exporter.yaml_parse") + def test_template_delete_skips_mismatched_package_type(self, yaml_parse_mock): + parent_dir = os.path.sep + template_dir = os.path.join(parent_dir, "foo", "bar") + template_path = os.path.join(template_dir, "path") + template_str = self.example_yaml_template() + + resource_type_class = Mock() + resource_type_class.RESOURCE_TYPE = "resource_type1" + resource_type_class.ARTIFACT_TYPE = ZIP + resource_type_class.EXPORT_DESTINATION = Destination.S3 + resource_type_instance = Mock() + resource_type_class.return_value = resource_type_instance + + properties = {"PackageType": IMAGE, "foo": "bar"} + template_dict = {"Resources": {"Resource1": {"Type": "resource_type1", "Properties": properties}}} + yaml_parse_mock.return_value = template_dict + + with patch("samcli.lib.package.artifact_exporter.open", mock.mock_open(read_data=template_str)): + template_exporter = Template( + template_path, parent_dir, self.uploaders_mock, self.code_signer_mock, [resource_type_class] + ) + template_exporter.delete(retain_resources=[]) + + resource_type_class.assert_not_called() + + @patch("samcli.lib.package.artifact_exporter.wait") + @patch("samcli.lib.package.artifact_exporter.ThreadPoolExecutor") + def test_execute_jobs_in_parallel_submits_and_waits(self, executor_mock, wait_mock): + class _FakeFuture: + def __init__(self, job): + self._job = job + + def result(self): + return self._job() + + class _FakeExecutor: + def __init__(self, max_workers): + self.max_workers = max_workers + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def submit(self, job): + return _FakeFuture(job) + + executor_mock.side_effect = lambda max_workers: _FakeExecutor(max_workers=max_workers) + + job1 = Mock() + job2 = Mock() + template_exporter = Template.__new__(Template) + template_exporter._execute_jobs_in_parallel([job1, job2]) + + executor_mock.assert_called_once_with(max_workers=2) + wait_mock.assert_called_once() + job1.assert_called_once() + job2.assert_called_once() + + @patch("samcli.lib.package.artifact_exporter.wait") + @patch("samcli.lib.package.artifact_exporter.ThreadPoolExecutor") + def test_execute_jobs_in_parallel_falls_back_to_single_worker(self, executor_mock, wait_mock): + job1 = Mock() + + template_exporter = Template.__new__(Template) + template_exporter._execute_jobs_in_parallel([job1]) + + job1.assert_called_once() + executor_mock.assert_not_called() + wait_mock.assert_not_called() + @patch("samcli.lib.package.artifact_exporter.is_experimental_enabled") @patch("samcli.lib.package.artifact_exporter.yaml_parse") def test_template_export_with_experimental_flag(self, yaml_parse_mock, is_experimental_enabled_mock): diff --git a/tests/unit/lib/package/test_stream_cursor_utils.py b/tests/unit/lib/package/test_stream_cursor_utils.py index dc4c66266de..b38c8bee4d6 100644 --- a/tests/unit/lib/package/test_stream_cursor_utils.py +++ b/tests/unit/lib/package/test_stream_cursor_utils.py @@ -1,6 +1,7 @@ from unittest import TestCase from samcli.lib.package.stream_cursor_utils import ( + CursorFormatter, CursorUpFormatter, CursorDownFormatter, CursorLeftFormatter, @@ -14,3 +15,6 @@ def test_cursor_utils(self): self.assertEqual(CursorDownFormatter().cursor_format(count=1), "\x1b[1B") self.assertEqual(CursorLeftFormatter().cursor_format(), "\x1b[0G") self.assertEqual(ClearLineFormatter().cursor_format(), "\x1b[0K") + + def test_base_formatter_is_noop(self): + self.assertIsNone(CursorFormatter().cursor_format(count=0)) diff --git a/tests/unit/lib/utils/test_profile.py b/tests/unit/lib/utils/test_profile.py new file mode 100644 index 00000000000..0c9810fce53 --- /dev/null +++ b/tests/unit/lib/utils/test_profile.py @@ -0,0 +1,11 @@ +from unittest import TestCase +from unittest.mock import patch + +from samcli.lib.utils.profile import list_available_profiles + + +class TestProfileUtils(TestCase): + @patch("samcli.lib.utils.profile.Session") + def test_list_available_profiles(self, session_mock): + session_mock.return_value.available_profiles = ["p1", "p2"] + self.assertEqual(["p1", "p2"], list_available_profiles()) From 432d67c6fc68507360a4ae66d0f10ee6052162ca Mon Sep 17 00:00:00 2001 From: andy klier Date: Wed, 30 Sep 2026 15:58:09 -0600 Subject: [PATCH 07/14] fix: address review feedback on parallel uploads - Bound in-flight uploads across nested stacks with a shared semaphore; nested-stack exports coordinate children and do not take a slot. - Serialize check-then-upload per local path so concurrent jobs sharing a CodeUri upload it once (per-key lock on the thread-safe cache). - Disable S3/ECR progress bars when uploading in parallel; their in-place redraws interleave across threads. - Fail fast: cancel queued jobs on the first failure and log every other failure before re-raising the first. - Move the samconfig docs example outside the enclosing code fence. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/sam-config-docs.md | 9 +- samcli/commands/package/package_context.py | 6 +- samcli/lib/package/artifact_exporter.py | 38 ++++++-- samcli/lib/package/utils.py | 50 ++++++----- .../commands/package/test_package_context.py | 27 ++++++ .../lib/package/test_artifact_exporter.py | 90 +++++++++++++------ tests/unit/lib/package/test_utils.py | 42 +++++++++ 7 files changed, 202 insertions(+), 60 deletions(-) diff --git a/docs/sam-config-docs.md b/docs/sam-config-docs.md index 48bdac676c6..28b55adae9a 100644 --- a/docs/sam-config-docs.md +++ b/docs/sam-config-docs.md @@ -23,6 +23,10 @@ output_template_file="packaged.yaml" [default.deploy.parameters] stack_name="using_config_file" +capabilities="CAPABILITY_IAM" +region="us-east-1" +profile="srirammv" +``` ### Specifying a Boolean deployment option @@ -33,10 +37,6 @@ parallel_upload=true Setting `parallel_upload` to `true` is equivalent to passing `--parallel-upload` on `sam deploy`, enabling concurrent S3/ECR uploads during the packaging phase. -capabilities="CAPABILITY_IAM" -region="us-east-1" -profile="srirammv" -``` Version ------- @@ -104,3 +104,4 @@ stack_name="using_config_file" [default.build.parameters] debug=true ``` + diff --git a/samcli/commands/package/package_context.py b/samcli/commands/package/package_context.py index abad4d59dd2..5275adf793a 100644 --- a/samcli/commands/package/package_context.py +++ b/samcli/commands/package/package_context.py @@ -154,13 +154,15 @@ def run(self): # Pass None instead of validating Docker client upfront - ECRUploader will validate only when needed docker_client = None + # Progress bars redraw the terminal in place and interleave badly across threads. + no_progressbar = self.no_progressbar or self.parallel_upload s3_uploader = S3Uploader( - s3_client, self.s3_bucket, self.s3_prefix, self.kms_key_id, self.force_upload, self.no_progressbar + s3_client, self.s3_bucket, self.s3_prefix, self.kms_key_id, self.force_upload, no_progressbar ) # attach the given metadata to the artifacts to be uploaded s3_uploader.artifact_metadata = self.metadata ecr_uploader = ECRUploader( - docker_client, ecr_client, self.image_repository, self.image_repositories, self.no_progressbar + docker_client, ecr_client, self.image_repository, self.image_repositories, no_progressbar ) self.uploaders = Uploaders(s3_uploader, ecr_uploader) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index d4b3ddbafb9..e3090304d58 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -70,6 +70,11 @@ DEFAULT_PARALLEL_UPLOAD_WORKERS = max(4, min(32, (os.cpu_count() or 1) * 2)) +# Bounds in-flight artifact uploads across the whole export, including nested stacks. Nested-stack +# exports only coordinate their children and do not take a slot, so a parent waiting on its +# children can never starve them. +_UPLOAD_SLOTS = threading.BoundedSemaphore(DEFAULT_PARALLEL_UPLOAD_WORKERS) + class _ThreadSafeUploadCache(MutableMapping[str, str]): """Simple thread-safe mapping used to deduplicate uploads across threads.""" @@ -78,6 +83,12 @@ def __init__(self, initial: Optional[MutableMapping[str, str]] = None): # Copy into a regular dict so we can safely snapshot under a lock self._cache: Dict[str, str] = dict(initial or {}) self._lock = threading.Lock() + self._key_locks: Dict[str, threading.Lock] = {} + + def key_lock(self, key: str) -> threading.Lock: + """Lock serializing check-then-upload for a single key; other keys proceed concurrently.""" + with self._lock: + return self._key_locks.setdefault(key, threading.Lock()) def __getitem__(self, key: str) -> str: # pragma: no cover - small helper with self._lock: @@ -681,7 +692,11 @@ def _job() -> None: exporter.parent_parameter_values = self.parameter_values exporter.language_extensions_enabled = self.language_extensions_enabled exporter.parallel_upload = self.parallel_upload - exporter.export(resource_full_path, resource_dict, self.template_dir) + if not self.parallel_upload or isinstance(exporter, CloudFormationStackResource): + exporter.export(resource_full_path, resource_dict, self.template_dir) + return + with _UPLOAD_SLOTS: + exporter.export(resource_full_path, resource_dict, self.template_dir) return _job @@ -691,11 +706,24 @@ def _execute_jobs_in_parallel(self, jobs: List[Callable[[], None]]) -> None: jobs[0]() return - with ThreadPoolExecutor(max_workers=max_workers) as executor: + executor = ThreadPoolExecutor(max_workers=max_workers) + try: futures = [executor.submit(job) for job in jobs] - wait(futures, return_when=FIRST_EXCEPTION) - for future in futures: - future.result() + done, _ = wait(futures, return_when=FIRST_EXCEPTION) + failed = [future for future in done if future.exception() is not None] + if failed: + # Fail fast: drop queued jobs, let running ones finish, then report every failure. + executor.shutdown(wait=True, cancel_futures=True) + first_error = cast(BaseException, failed[0].exception()) + for future in futures: + if future.cancelled() or future is failed[0]: + continue + error = future.exception() + if error is not None: + LOG.error("Parallel artifact upload also failed: %s", error, exc_info=error) + raise first_error + finally: + executor.shutdown(wait=True) def delete(self, retain_resources: List): """ diff --git a/samcli/lib/package/utils.py b/samcli/lib/package/utils.py index fb5f22e55bc..eb1b9da734c 100644 --- a/samcli/lib/package/utils.py +++ b/samcli/lib/package/utils.py @@ -186,29 +186,33 @@ def upload_local_artifacts( local_path = make_abs_path(parent_dir, local_path) - if previously_uploaded and local_path in previously_uploaded: - result = previously_uploaded[local_path] - LOG.debug("Skipping upload of %s since is already uploaded to %s", local_path, result) - return cast(str, result) - - # Or, pointing to a folder. Zip the folder and upload (zip_method is changed based on resource type) - if is_local_folder(local_path): - result = zip_and_upload( - local_path, - uploader, - extension, - zip_method=make_zip_with_lambda_permissions if resource_type in LAMBDA_LOCAL_RESOURCES else make_zip, - ) - if previously_uploaded is not None: - previously_uploaded[local_path] = result - return result - - # Path could be pointing to a file. Upload the file - if is_local_file(local_path): - result = uploader.upload_with_dedup(local_path) - if previously_uploaded is not None: - previously_uploaded[local_path] = result - return result + # A thread-safe cache (parallel uploads) exposes a per-key lock so the check-then-upload below + # is atomic per path: concurrent jobs sharing a local path upload it once. + key_lock = getattr(previously_uploaded, "key_lock", None) + with key_lock(local_path) if key_lock else contextlib.nullcontext(): + if previously_uploaded and local_path in previously_uploaded: + result = previously_uploaded[local_path] + LOG.debug("Skipping upload of %s since is already uploaded to %s", local_path, result) + return cast(str, result) + + # Or, pointing to a folder. Zip the folder and upload (zip_method is changed based on resource type) + if is_local_folder(local_path): + result = zip_and_upload( + local_path, + uploader, + extension, + zip_method=make_zip_with_lambda_permissions if resource_type in LAMBDA_LOCAL_RESOURCES else make_zip, + ) + if previously_uploaded is not None: + previously_uploaded[local_path] = result + return result + + # Path could be pointing to a file. Upload the file + if is_local_file(local_path): + result = uploader.upload_with_dedup(local_path) + if previously_uploaded is not None: + previously_uploaded[local_path] = result + return result raise InvalidLocalPathError(resource_id=resource_id, property_name=property_path, local_path=local_path) diff --git a/tests/unit/commands/package/test_package_context.py b/tests/unit/commands/package/test_package_context.py index 29b0a1e6ff1..90228250792 100644 --- a/tests/unit/commands/package/test_package_context.py +++ b/tests/unit/commands/package/test_package_context.py @@ -99,6 +99,33 @@ def test_template_path_valid_with_output_template(self, patched_boto, mock_get_v ) package_command_context.run() + @patch("samcli.commands.package.package_context.ECRUploader") + @patch("samcli.commands.package.package_context.S3Uploader") + @patch.object(ResourceMetadataNormalizer, "normalize", MagicMock()) + @patch.object(Template, "export", MagicMock(return_value={})) + @patch("boto3.client") + def test_parallel_upload_disables_progress_bars(self, patched_boto, s3_uploader_mock, ecr_uploader_mock): + with tempfile.NamedTemporaryFile(mode="w", delete=False) as temp_template_file: + PackageContext( + template_file=temp_template_file.name, + s3_bucket="s3-bucket", + s3_prefix="s3-prefix", + image_repository="image-repo", + image_repositories=None, + kms_key_id="kms-key-id", + output_template_file=None, + use_json=True, + force_upload=True, + no_progressbar=False, + metadata={}, + region="us-east-2", + profile=None, + parallel_upload=True, + ).run() + + self.assertTrue(s3_uploader_mock.call_args.args[5]) + self.assertTrue(ecr_uploader_mock.call_args.args[4]) + @patch("samcli.lib.package.ecr_uploader.get_validated_container_client") @patch.object(ResourceMetadataNormalizer, "normalize", MagicMock()) @patch.object(Template, "export", MagicMock(return_value={})) diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 0147852abad..4cea957c9d3 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -8,8 +8,11 @@ import shutil import string import tempfile +import threading +import time import unittest import zipfile +from concurrent.futures import ThreadPoolExecutor from contextlib import contextmanager, closing from pathlib import Path from typing import Optional, Dict @@ -1732,41 +1735,76 @@ def test_template_delete_skips_mismatched_package_type(self, yaml_parse_mock): resource_type_class.assert_not_called() - @patch("samcli.lib.package.artifact_exporter.wait") - @patch("samcli.lib.package.artifact_exporter.ThreadPoolExecutor") - def test_execute_jobs_in_parallel_submits_and_waits(self, executor_mock, wait_mock): - class _FakeFuture: - def __init__(self, job): - self._job = job - - def result(self): - return self._job() - - class _FakeExecutor: - def __init__(self, max_workers): - self.max_workers = max_workers - - def __enter__(self): - return self - - def __exit__(self, exc_type, exc, tb): - return False - - def submit(self, job): - return _FakeFuture(job) - - executor_mock.side_effect = lambda max_workers: _FakeExecutor(max_workers=max_workers) - + @patch("samcli.lib.package.artifact_exporter.ThreadPoolExecutor", wraps=ThreadPoolExecutor) + def test_execute_jobs_in_parallel_submits_and_waits(self, executor_mock): job1 = Mock() job2 = Mock() template_exporter = Template.__new__(Template) template_exporter._execute_jobs_in_parallel([job1, job2]) executor_mock.assert_called_once_with(max_workers=2) - wait_mock.assert_called_once() job1.assert_called_once() job2.assert_called_once() + @patch("samcli.lib.package.artifact_exporter.LOG") + @patch("samcli.lib.package.artifact_exporter.DEFAULT_PARALLEL_UPLOAD_WORKERS", 2) + def test_execute_jobs_in_parallel_fails_fast_and_reports_all_failures(self, log_mock): + slow_started = threading.Event() + queued_runs = [] + + def fail_first(): + slow_started.wait(5) + raise ValueError("first failure") + + def fail_slow(): + slow_started.set() + time.sleep(0.2) + raise RuntimeError("second failure") + + def queued(): + queued_runs.append(1) + time.sleep(0.05) + + template_exporter = Template.__new__(Template) + with self.assertRaises(ValueError): + template_exporter._execute_jobs_in_parallel([fail_first, fail_slow] + [queued] * 20) + + # Queued jobs are cancelled on the first failure; only jobs a free worker had already + # picked up can still run. + self.assertLess(len(queued_runs), 20) + logged = [call.args[1] for call in log_mock.error.call_args_list] + self.assertTrue(any(isinstance(error, RuntimeError) for error in logged)) + + @patch("samcli.lib.package.artifact_exporter._UPLOAD_SLOTS") + def test_parallel_export_job_bounds_leaf_uploads_but_not_nested_stacks(self, slots_mock): + class _NestedStack(CloudFormationStackResource): + def __init__(self, *args, **kwargs): + pass + + def export(self, *args, **kwargs): + pass + + leaf_exporter = Mock() + template_exporter = Template.__new__(Template) + template_exporter.uploaders = Mock() + template_exporter.code_signer = Mock() + template_exporter.parameter_values = None + template_exporter.language_extensions_enabled = False + template_exporter.template_dir = "dir" + template_exporter.parallel_upload = True + + template_exporter._build_export_job(_NestedStack, "Nested", {}, None)() + slots_mock.__enter__.assert_not_called() + + template_exporter._build_export_job(Mock(return_value=leaf_exporter), "Leaf", {}, None)() + slots_mock.__enter__.assert_called_once() + leaf_exporter.export.assert_called_once_with("Leaf", {}, "dir") + + def test_thread_safe_upload_cache_key_lock_is_per_key(self): + cache = _ThreadSafeUploadCache() + self.assertIs(cache.key_lock("a"), cache.key_lock("a")) + self.assertIsNot(cache.key_lock("a"), cache.key_lock("b")) + @patch("samcli.lib.package.artifact_exporter.wait") @patch("samcli.lib.package.artifact_exporter.ThreadPoolExecutor") def test_execute_jobs_in_parallel_falls_back_to_single_worker(self, executor_mock, wait_mock): diff --git a/tests/unit/lib/package/test_utils.py b/tests/unit/lib/package/test_utils.py index 073802f5314..e69dafac580 100644 --- a/tests/unit/lib/package/test_utils.py +++ b/tests/unit/lib/package/test_utils.py @@ -1,5 +1,8 @@ import tempfile +import threading +import time from unittest import TestCase +from unittest.mock import Mock, patch from parameterized import parameterized @@ -61,3 +64,42 @@ def test_zip_folder_uses_different_path_for_same_file_in_different_run(self): previous_md5_hash = md5_hash else: self.assertEqual(previous_md5_hash, md5_hash) + + def test_upload_local_artifacts_uploads_shared_path_once_across_threads(self): + from samcli.lib.package.artifact_exporter import _ThreadSafeUploadCache + + cache = _ThreadSafeUploadCache({}) + calls = [] + + def slow_zip_and_upload(local_path, *args, **kwargs): + calls.append(local_path) + time.sleep(0.1) + return "s3://bucket/key" + + with ( + tempfile.TemporaryDirectory() as folder, + patch.object(utils, "zip_and_upload", side_effect=slow_zip_and_upload), + ): + results = [] + + def upload(): + results.append( + utils.upload_local_artifacts( + resource_type="AWS::Serverless::Function", + resource_id="Function", + resource_dict={"CodeUri": folder}, + property_path="CodeUri", + parent_dir=folder, + uploader=Mock(), + previously_uploaded=cache, + ) + ) + + threads = [threading.Thread(target=upload) for _ in range(4)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + self.assertEqual(1, len(calls)) + self.assertEqual(["s3://bucket/key"] * 4, results) From 95d5dbc55971c8a0a3c7eee6cf3e20b054258777 Mon Sep 17 00:00:00 2001 From: andy klier Date: Wed, 30 Sep 2026 16:13:48 -0600 Subject: [PATCH 08/14] fix: share one upload executor across nested stacks; drop guided prompt - Create a single ThreadPoolExecutor at the root Template and pass it to nested-stack child Templates, so one pool bounds both threads and in-flight uploads for the whole export (replaces the upload semaphore). Nested-stack exports run in the calling thread rather than a pool worker, since they wait on their children; their children's uploads go to the shared pool. - Surface the first failure in submission order so the reported error is deterministic when several uploads fail; cancel pending uploads if a nested-stack export fails. - Remove the new --guided prompt: it shifted the positional answers used by scripted guided deploys and the guided integration tests. --guided keeps the --parallel-upload flag value and saves it to samconfig. Co-Authored-By: Claude Opus 5.5 (1M context) --- samcli/commands/deploy/guided_context.py | 9 +- samcli/lib/package/artifact_exporter.py | 116 +++++++++++------- tests/unit/commands/deploy/test_command.py | 12 +- .../commands/deploy/test_guided_context.py | 81 +++++++----- .../lib/package/test_artifact_exporter.py | 109 +++++++++------- 5 files changed, 196 insertions(+), 131 deletions(-) diff --git a/samcli/commands/deploy/guided_context.py b/samcli/commands/deploy/guided_context.py index 48bc9b4a1b9..3826d181e8b 100644 --- a/samcli/commands/deploy/guided_context.py +++ b/samcli/commands/deploy/guided_context.py @@ -168,12 +168,6 @@ def guided_prompts(self, parameter_override_keys): click.secho("\t#Preserves the state of previously provisioned resources when an operation fails") disable_rollback = confirm(f"\t{self.start_bold}Disable rollback{self.end_bold}", default=self.disable_rollback) - if self.parallel_upload: - parallel_upload = True - else: - click.secho("\t#Speed up artifact uploads by running them in parallel") - parallel_upload = confirm(f"\t{self.start_bold}Enable parallel uploads{self.end_bold}", default=False) - self.prompt_authorization(stacks) self.prompt_code_signing_settings(stacks) @@ -216,7 +210,8 @@ def guided_prompts(self, parameter_override_keys): self.guided_s3_prefix = stack_name self.guided_region = region self.guided_profile = self.profile - self.guided_parallel_upload = parallel_upload + # Not prompted, so scripted guided deploys keep their answer order; the flag value is saved. + self.guided_parallel_upload = self.parallel_upload self._capabilities = input_capabilities if input_capabilities else default_capabilities self._parameter_overrides = ( input_parameter_overrides if input_parameter_overrides else self.parameter_overrides_from_cmdline diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index e3090304d58..5879894aa7d 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -19,8 +19,8 @@ import os import threading from collections.abc import MutableMapping -from concurrent.futures import FIRST_EXCEPTION, ThreadPoolExecutor, wait -from typing import Any, Callable, Dict, List, Optional, Sequence, cast +from concurrent.futures import FIRST_EXCEPTION, Future, ThreadPoolExecutor, wait +from typing import Any, Callable, Dict, List, Optional, Sequence, Tuple, cast from botocore.utils import set_value_from_jmespath @@ -70,11 +70,6 @@ DEFAULT_PARALLEL_UPLOAD_WORKERS = max(4, min(32, (os.cpu_count() or 1) * 2)) -# Bounds in-flight artifact uploads across the whole export, including nested stacks. Nested-stack -# exports only coordinate their children and do not take a slot, so a parent waiting on its -# children can never starve them. -_UPLOAD_SLOTS = threading.BoundedSemaphore(DEFAULT_PARALLEL_UPLOAD_WORKERS) - class _ThreadSafeUploadCache(MutableMapping[str, str]): """Simple thread-safe mapping used to deduplicate uploads across threads.""" @@ -323,6 +318,7 @@ def _do_export_without_language_extensions(self, resource_id: str, template_path parent_stack_id=resource_id, language_extensions_enabled=False, parallel_upload=getattr(self, "parallel_upload", False), + upload_executor=getattr(self, "upload_executor", None), ).export() def _do_export_with_language_extensions( @@ -418,6 +414,7 @@ def _do_export_with_language_extensions( parameter_values=parameter_values, language_extensions_enabled=self.language_extensions_enabled, parallel_upload=getattr(self, "parallel_upload", False), + upload_executor=getattr(self, "upload_executor", None), ) exported_template = template.export() @@ -452,6 +449,7 @@ def _do_export_with_language_extensions( parameter_values=parameter_values, language_extensions_enabled=self.language_extensions_enabled, parallel_upload=getattr(self, "parallel_upload", False), + upload_executor=getattr(self, "upload_executor", None), ).export() return exported_template_dict @@ -535,6 +533,7 @@ def __init__( template_dict: Optional[Dict] = None, language_extensions_enabled: bool = False, parallel_upload: bool = False, + upload_executor: Optional[ThreadPoolExecutor] = None, ): """ Reads the template and makes it ready for export @@ -580,6 +579,9 @@ def __init__( self.parameter_values = parameter_values self.language_extensions_enabled = language_extensions_enabled self.parallel_upload = parallel_upload + # Shared by the root template and every nested-stack child so that one pool bounds both + # threads and in-flight uploads for the whole export. Created by the root when None. + self.upload_executor = upload_executor def _export_global_artifacts(self, template_dict: Dict) -> Dict: """See module-level _export_global_artifacts_pass for the canonical @@ -656,7 +658,22 @@ def export(self) -> Dict: if cache is not None and self.parallel_upload: cache = _ThreadSafeUploadCache(cache) - export_jobs: List[Callable[[], None]] = [] + if not self.parallel_upload: + for _, job in self._collect_export_jobs(cache, None): + job() + elif self.upload_executor is not None: + self._run_export_jobs(self._collect_export_jobs(cache, self.upload_executor), self.upload_executor) + else: + with ThreadPoolExecutor(max_workers=DEFAULT_PARALLEL_UPLOAD_WORKERS) as executor: + self._run_export_jobs(self._collect_export_jobs(cache, executor), executor) + + return self.template_dict + + def _collect_export_jobs( + self, cache: Optional[MutableMapping[str, str]], executor: Optional[ThreadPoolExecutor] + ) -> List[Tuple[bool, Callable[[], None]]]: + """Return (is_nested_stack, job) pairs for every resource that has artifacts to export.""" + jobs: List[Tuple[bool, Callable[[], None]]] = [] for resource_logical_id, resource in iter_regular_resources(self.template_dict): resource_type = resource.get("Type", None) resource_dict = resource.get("Properties", {}) @@ -669,15 +686,12 @@ def export(self) -> Dict: if resource_dict.get("PackageType", ZIP) != exporter_class.ARTIFACT_TYPE: continue - export_jobs.append(self._build_export_job(exporter_class, full_path, resource_dict, cache)) - - if self.parallel_upload and export_jobs: - self._execute_jobs_in_parallel(export_jobs) - else: - for job in export_jobs: - job() - - return self.template_dict + is_nested_stack = isinstance(exporter_class, type) and issubclass( + exporter_class, CloudFormationStackResource + ) + job = self._build_export_job(exporter_class, full_path, resource_dict, cache, executor) + jobs.append((is_nested_stack, job)) + return jobs def _build_export_job( self, @@ -685,6 +699,7 @@ def _build_export_job( resource_full_path: str, resource_dict: Dict, cache: Optional[MutableMapping[str, str]], + executor: Optional[ThreadPoolExecutor] = None, ) -> Callable[[], None]: def _job() -> None: # Export code resources @@ -692,38 +707,49 @@ def _job() -> None: exporter.parent_parameter_values = self.parameter_values exporter.language_extensions_enabled = self.language_extensions_enabled exporter.parallel_upload = self.parallel_upload - if not self.parallel_upload or isinstance(exporter, CloudFormationStackResource): - exporter.export(resource_full_path, resource_dict, self.template_dir) - return - with _UPLOAD_SLOTS: - exporter.export(resource_full_path, resource_dict, self.template_dir) + exporter.upload_executor = executor + exporter.export(resource_full_path, resource_dict, self.template_dir) return _job - def _execute_jobs_in_parallel(self, jobs: List[Callable[[], None]]) -> None: - max_workers = min(len(jobs), DEFAULT_PARALLEL_UPLOAD_WORKERS) - if max_workers <= 1: - jobs[0]() - return - - executor = ThreadPoolExecutor(max_workers=max_workers) + @staticmethod + def _run_export_jobs(jobs: List[Tuple[bool, Callable[[], None]]], executor: ThreadPoolExecutor) -> None: + """ + Submit artifact uploads to the shared executor and run nested-stack exports in the calling + thread. A nested stack waits for its children, so running it inside a pool worker could + deadlock the shared pool; its children's uploads are submitted to the same executor. + """ + futures = [executor.submit(job) for is_nested_stack, job in jobs if not is_nested_stack] try: - futures = [executor.submit(job) for job in jobs] - done, _ = wait(futures, return_when=FIRST_EXCEPTION) - failed = [future for future in done if future.exception() is not None] - if failed: - # Fail fast: drop queued jobs, let running ones finish, then report every failure. - executor.shutdown(wait=True, cancel_futures=True) - first_error = cast(BaseException, failed[0].exception()) - for future in futures: - if future.cancelled() or future is failed[0]: - continue - error = future.exception() - if error is not None: - LOG.error("Parallel artifact upload also failed: %s", error, exc_info=error) - raise first_error - finally: - executor.shutdown(wait=True) + for is_nested_stack, job in jobs: + if is_nested_stack: + job() + except BaseException: + for future in futures: + future.cancel() + wait(futures) + raise + Template._wait_fail_fast(futures) + + @staticmethod + def _wait_fail_fast(futures: List[Future]) -> None: + """Wait for futures; on the first failure cancel the rest, log other failures, re-raise the first.""" + done, _ = wait(futures, return_when=FIRST_EXCEPTION) + # Submission order keeps the surfaced error stable when several jobs have already failed. + failed = [future for future in futures if future in done and future.exception() is not None] + if not failed: + return + for future in futures: + future.cancel() + wait(futures) + first_error = cast(BaseException, failed[0].exception()) + for future in futures: + if future is failed[0] or future.cancelled(): + continue + error = future.exception() + if error is not None: + LOG.error("Parallel artifact upload also failed: %s", error, exc_info=error) + raise first_error def delete(self, retain_resources: List): """ diff --git a/tests/unit/commands/deploy/test_command.py b/tests/unit/commands/deploy/test_command.py index cfbffd29b8c..ae9bc4f2581 100644 --- a/tests/unit/commands/deploy/test_command.py +++ b/tests/unit/commands/deploy/test_command.py @@ -298,7 +298,7 @@ def test_all_args_guided_no_to_authorization_confirmation_prompt( context_mock = Mock() mockauth_per_resource.return_value = [("HelloWorldResource1", False), ("HelloWorldResource2", False)] mock_deploy_context.return_value.__enter__.return_value = context_mock - mock_confirm.side_effect = [True, True, False, False, True, False] + mock_confirm.side_effect = [True, True, False, True, False] mock_prompt.side_effect = [ "sam-app", "us-east-1", @@ -399,7 +399,7 @@ def test_all_args_guided_use_defaults( mock_sam_function_provider.return_value.get_all.return_value = [function_mock] mockauth_per_resource.return_value = [("HelloWorldResource", False)] mock_deploy_context.return_value.__enter__.return_value = context_mock - mock_confirm.side_effect = [True, False, True, False, True, True, True, True] + mock_confirm.side_effect = [True, False, True, True, True, True, True] mock_prompt.side_effect = [ "sam-app", "us-east-1", @@ -558,7 +558,7 @@ def test_all_args_guided( mock_sam_function_provider.return_value.get_all.return_value = [function_mock] mockauth_per_resource.return_value = [("HelloWorldResource", False)] mock_deploy_context.return_value.__enter__.return_value = context_mock - mock_confirm.side_effect = [True, False, True, False, True, True, True, True] + mock_confirm.side_effect = [True, False, True, True, True, True, True] mock_prompt.side_effect = [ "sam-app", "us-east-1", @@ -731,7 +731,7 @@ def test_all_args_guided_no_save_echo_param_to_config( "testconfig.toml", "test-env", ] - mock_confirm.side_effect = [True, False, True, False, True, True, True, True] + mock_confirm.side_effect = [True, False, True, True, True, True, True] mock_managed_stack.return_value = "managed-s3-bucket" mock_signer_config_per_function.return_value = ({}, {}) @@ -900,7 +900,7 @@ def test_all_args_guided_no_params_save_config( "testconfig.toml", "test-env", ] - mock_confirm.side_effect = [True, False, True, False, True, True, True, True] + mock_confirm.side_effect = [True, False, True, True, True, True, True] mock_get_cmd_names.return_value = ["deploy"] mock_managed_stack.return_value = "managed-s3-bucket" mock_signer_config_per_function.return_value = ({}, {}) @@ -1048,7 +1048,7 @@ def test_all_args_guided_no_params_no_save_config( "us-east-1", ("CAPABILITY_IAM",), ] - mock_confirm.side_effect = [True, True, False, False, True, False, True, True] + mock_confirm.side_effect = [True, True, False, True, False, True, True] mock_managed_stack.return_value = "managed-s3-bucket" mock_signer_config_per_function.return_value = ({}, {}) diff --git a/tests/unit/commands/deploy/test_guided_context.py b/tests/unit/commands/deploy/test_guided_context.py index 64397c2eb2f..976faff8320 100644 --- a/tests/unit/commands/deploy/test_guided_context.py +++ b/tests/unit/commands/deploy/test_guided_context.py @@ -73,7 +73,7 @@ def test_guided_prompts_check_defaults_non_public_resources_zips( patched_auth_per_resource.return_value = [ ("HelloWorldFunction", True), ] - patched_confirm.side_effect = [True, False, False, False, "", True, True, True] + patched_confirm.side_effect = [True, False, False, "", True, True, True] patched_manage_stack.return_value = "managed_s3_stack" patched_signer_config_per_function.return_value = ({}, {}) self.gc.guided_prompts(parameter_override_keys=None) @@ -82,7 +82,6 @@ def test_guided_prompts_check_defaults_non_public_resources_zips( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), - call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call(f"\t{self.gc.start_bold}Save arguments to configuration file{self.gc.end_bold}", default=True), call( f"\t {self.gc.start_bold}Delete the unreferenced repositories listed above when deploying?{self.gc.end_bold}", @@ -105,6 +104,41 @@ def test_guided_prompts_check_defaults_non_public_resources_zips( language_extensions_enabled=False, ) + @parameterized.expand([(True,), (False,)]) + @patch("samcli.commands.deploy.guided_context.get_resource_full_path_by_id") + @patch("samcli.commands.deploy.guided_context.prompt") + @patch("samcli.commands.deploy.guided_context.confirm") + @patch("samcli.commands.deploy.guided_context.manage_stack") + @patch("samcli.commands.deploy.guided_context.auth_per_resource") + @patch("samcli.commands.deploy.guided_context.SamLocalStackProvider.get_stacks") + @patch("samcli.commands.deploy.guided_context.SamFunctionProvider") + @patch("samcli.commands.deploy.guided_context.signer_config_per_function") + def test_guided_prompts_keep_parallel_upload_flag_without_prompting( + self, + parallel_upload, + patched_signer_config_per_function, + patched_sam_function_provider, + patched_get_buildable_stacks, + patchedauth_per_resource, + patched_manage_stack, + patched_confirm, + patched_prompt, + get_resource_full_path_by_id_mock, + ): + patched_signer_config_per_function.return_value = (None, None) + patched_sam_function_provider.return_value.functions = {} + patched_get_buildable_stacks.return_value = (Mock(), []) + patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] + patched_confirm.side_effect = [True, False, False, True, False, True, True] + patched_manage_stack.return_value = "managed_s3_stack" + self.gc.parallel_upload = parallel_upload + + self.gc.guided_prompts(parameter_override_keys=None) + + self.assertEqual(parallel_upload, self.gc.guided_parallel_upload) + prompted = [confirm_call.args[0] for confirm_call in patched_confirm.call_args_list] + self.assertFalse(any("parallel" in text.lower() for text in prompted)) + @patch("samcli.commands.deploy.guided_context.get_resource_full_path_by_id") @patch("samcli.commands.deploy.guided_context.prompt") @patch("samcli.commands.deploy.guided_context.confirm") @@ -129,7 +163,7 @@ def test_guided_prompts_check_defaults_public_resources_zips( patched_get_buildable_stacks.return_value = (Mock(), []) # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] - patched_confirm.side_effect = [True, False, False, False, True, False, True, True] + patched_confirm.side_effect = [True, False, False, True, False, True, True] patched_manage_stack.return_value = "managed_s3_stack" self.gc.guided_prompts(parameter_override_keys=None) # Now to check for all the defaults on confirmations. @@ -137,7 +171,6 @@ def test_guided_prompts_check_defaults_public_resources_zips( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), - call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, @@ -197,7 +230,7 @@ def test_guided_prompts_check_defaults_public_resources_images( # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] get_resource_full_path_by_id_mock.return_value = None - patched_confirm.side_effect = [True, False, False, False, True, False, True, True] + patched_confirm.side_effect = [True, False, False, True, False, True, True] patched_manage_stack.return_value = "managed_s3_stack" self.gc.guided_prompts(parameter_override_keys=None) # Now to check for all the defaults on confirmations. @@ -205,7 +238,6 @@ def test_guided_prompts_check_defaults_public_resources_images( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), - call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, @@ -236,7 +268,6 @@ def test_guided_prompts_check_defaults_public_resources_images( call("\t#Shows you resources changes to be deployed and require a 'Y' to initiate deploy"), call("\t#SAM needs permission to be able to create roles to connect to the resources in your template"), call("\t#Preserves the state of previously provisioned resources when an operation fails"), - call("\t#Speed up artifact uploads by running them in parallel"), call("\n\tManaged S3 bucket: managed_s3_stack", bold=True), ] self.assertEqual(expected_click_secho_calls, patched_click_secho.call_args_list) @@ -277,7 +308,7 @@ def test_guided_prompts_check_defaults_public_resources_images_ecr_url( # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] get_resource_full_path_by_id_mock.return_value = "RandomFunction" - patched_confirm.side_effect = [True, False, False, False, True, False, True, True] + patched_confirm.side_effect = [True, False, False, True, False, True, True] patched_manage_stack.return_value = "managed_s3_stack" patched_signer_config_per_function.return_value = ({}, {}) self.gc.guided_prompts(parameter_override_keys=None) @@ -286,7 +317,6 @@ def test_guided_prompts_check_defaults_public_resources_images_ecr_url( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), - call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, @@ -315,7 +345,6 @@ def test_guided_prompts_check_defaults_public_resources_images_ecr_url( call("\t#Shows you resources changes to be deployed and require a 'Y' to initiate deploy"), call("\t#SAM needs permission to be able to create roles to connect to the resources in your template"), call("\t#Preserves the state of previously provisioned resources when an operation fails"), - call("\t#Speed up artifact uploads by running them in parallel"), call("\n\tManaged S3 bucket: managed_s3_stack", bold=True), ] self.assertEqual(expected_click_secho_calls, patched_click_secho.call_args_list) @@ -356,7 +385,7 @@ def test_guided_prompts_images_illegal_image_uri( # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] get_resource_full_path_by_id_mock.return_value = "RandomFunction" - patched_confirm.side_effect = [True, False, False, False, True, False, False, True] + patched_confirm.side_effect = [True, False, False, True, False, False, True] patched_manage_stack.return_value = "managed_s3_stack" patched_signer_config_per_function.return_value = ({}, {}) with self.assertRaises(GuidedDeployFailedError): @@ -402,7 +431,7 @@ def test_guided_prompts_images_missing_repo( ] # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] - patched_confirm.side_effect = [True, False, False, False, True, False, True, True] + patched_confirm.side_effect = [True, False, False, True, False, True, True] patched_manage_stack.return_value = "managed_s3_stack" patched_signer_config_per_function.return_value = ({}, {}) @@ -412,7 +441,6 @@ def test_guided_prompts_images_missing_repo( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), - call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, @@ -441,7 +469,6 @@ def test_guided_prompts_images_missing_repo( call("\t#Shows you resources changes to be deployed and require a 'Y' to initiate deploy"), call("\t#SAM needs permission to be able to create roles to connect to the resources in your template"), call("\t#Preserves the state of previously provisioned resources when an operation fails"), - call("\t#Speed up artifact uploads by running them in parallel"), call("\n\tManaged S3 bucket: managed_s3_stack", bold=True), ] self.assertEqual(expected_click_secho_calls, patched_click_secho.call_args_list) @@ -483,7 +510,7 @@ def test_guided_prompts_images_no_repo( # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] patched_get_resource_full_path_by_id.return_value = "RandomFunction" - patched_confirm.side_effect = [True, False, False, False, True, False, False, True] + patched_confirm.side_effect = [True, False, False, True, False, False, True] patched_manage_stack.return_value = "managed_s3_stack" patched_signer_config_per_function.return_value = ({}, {}) @@ -493,7 +520,6 @@ def test_guided_prompts_images_no_repo( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), - call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, @@ -526,7 +552,6 @@ def test_guided_prompts_images_no_repo( call("\t#Shows you resources changes to be deployed and require a 'Y' to initiate deploy"), call("\t#SAM needs permission to be able to create roles to connect to the resources in your template"), call("\t#Preserves the state of previously provisioned resources when an operation fails"), - call("\t#Speed up artifact uploads by running them in parallel"), call("\n\tManaged S3 bucket: managed_s3_stack", bold=True), ] self.assertEqual(expected_click_secho_calls, patched_click_secho.call_args_list) @@ -567,7 +592,7 @@ def test_guided_prompts_images_deny_deletion( ] # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] - patched_confirm.side_effect = [True, False, False, False, True, False, True, False] + patched_confirm.side_effect = [True, False, False, True, False, True, False] patched_manage_stack.return_value = "managed_s3_stack" patched_signer_config_per_function.return_value = ({}, {}) with self.assertRaises(GuidedDeployFailedError): @@ -610,7 +635,7 @@ def test_guided_prompts_images_blank_image_repository( ] # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] - patched_confirm.side_effect = [True, False, False, False, True, False, False, True] + patched_confirm.side_effect = [True, False, False, True, False, False, True] patched_manage_stack.return_value = "managed_s3_stack" patched_signer_config_per_function.return_value = ({}, {}) with self.assertRaises(GuidedDeployFailedError): @@ -655,14 +680,13 @@ def test_guided_prompts_with_given_capabilities( patched_get_buildable_stacks.return_value = (Mock(), []) self.gc.capabilities = given_capabilities # Series of inputs to confirmations so that full range of questions are asked. - patched_confirm.side_effect = [True, False, False, False, "", True, True, True] + patched_confirm.side_effect = [True, False, False, "", True, True, True] self.gc.guided_prompts(parameter_override_keys=None) # Now to check for all the defaults on confirmations. expected_confirmation_calls = [ call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), - call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call(f"\t{self.gc.start_bold}Save arguments to configuration file{self.gc.end_bold}", default=True), call( f"\t {self.gc.start_bold}Delete the unreferenced repositories listed above when deploying?{self.gc.end_bold}", @@ -704,7 +728,7 @@ def test_guided_prompts_check_configuration_file_prompt_calls( patched_signer_config_per_function.return_value = ({}, {}) # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] - patched_confirm.side_effect = [True, False, False, False, True, True, True, True] + patched_confirm.side_effect = [True, False, False, True, True, True, True] patched_manage_stack.return_value = "managed_s3_stack" patched_get_resource_full_path_by_id.return_value = "RandomFunction" self.gc.guided_prompts(parameter_override_keys=None) @@ -713,7 +737,6 @@ def test_guided_prompts_check_configuration_file_prompt_calls( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), - call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, @@ -767,7 +790,7 @@ def test_guided_prompts_check_parameter_from_template( # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] patched_get_resource_full_path_by_id.return_value = "RandomFunction" - patched_confirm.side_effect = [True, False, False, False, True, False, True, True] + patched_confirm.side_effect = [True, False, False, True, False, True, True] patched_manage_stack.return_value = "managed_s3_stack" patched_signer_config_per_function.return_value = ({}, {}) parameter_override_from_template = {"MyTestKey": {"Default": "MyTemplateDefaultVal"}} @@ -778,7 +801,6 @@ def test_guided_prompts_check_parameter_from_template( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), - call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, @@ -827,7 +849,7 @@ def test_guided_prompts_check_parameter_from_cmd_or_config( # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] patched_get_resource_full_path_by_id.return_value = "RandomFunction" - patched_confirm.side_effect = [True, False, False, False, True, False, True, True] + patched_confirm.side_effect = [True, False, False, True, False, True, True] patched_signer_config_per_function.return_value = ({}, {}) patched_manage_stack.return_value = "managed_s3_stack" parameter_override_from_template = {"MyTestKey": {"Default": "MyTemplateDefaultVal"}} @@ -838,7 +860,6 @@ def test_guided_prompts_check_parameter_from_cmd_or_config( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), - call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, @@ -901,7 +922,7 @@ def test_guided_prompts_with_code_signing( patched_signer_config_per_function.return_value = given_code_signing_configs patched_get_buildable_stacks.return_value = (Mock(), []) # Series of inputs to confirmations so that full range of questions are asked. - patched_confirm.side_effect = [True, False, False, False, given_sign_packages_flag, "", True, True, True] + patched_confirm.side_effect = [True, False, False, given_sign_packages_flag, "", True, True, True] patched_get_resource_full_path_by_id.return_value = "RandomFunction" self.gc.guided_prompts(parameter_override_keys=None) # Now to check for all the defaults on confirmations. @@ -909,7 +930,6 @@ def test_guided_prompts_with_code_signing( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), - call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}Do you want to sign your code?{self.gc.end_bold}", default=True, @@ -973,7 +993,7 @@ def test_guided_prompts_check_default_config_region( # Series of inputs to confirmations so that full range of questions are asked. patchedauth_per_resource.return_value = [("HelloWorldFunction", False)] patched_get_resource_full_path_by_id.return_value = "RandomFunction" - patched_confirm.side_effect = [True, False, False, False, True, True, True, True] + patched_confirm.side_effect = [True, False, False, True, True, True, True] patched_signer_config_per_function.return_value = ({}, {}) patched_manage_stack.return_value = "managed_s3_stack" patched_get_default_aws_region.return_value = "default_config_region" @@ -985,7 +1005,6 @@ def test_guided_prompts_check_default_config_region( call(f"\t{self.gc.start_bold}Confirm changes before deploy{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Allow SAM CLI IAM role creation{self.gc.end_bold}", default=True), call(f"\t{self.gc.start_bold}Disable rollback{self.gc.end_bold}", default=False), - call(f"\t{self.gc.start_bold}Enable parallel uploads{self.gc.end_bold}", default=False), call( f"\t{self.gc.start_bold}HelloWorldFunction has no authentication. Is this okay?{self.gc.end_bold}", default=False, diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 4cea957c9d3..3ff7e3710e1 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -12,7 +12,7 @@ import time import unittest import zipfile -from concurrent.futures import ThreadPoolExecutor +from concurrent.futures import Future, ThreadPoolExecutor from contextlib import contextmanager, closing from pathlib import Path from typing import Optional, Dict @@ -1231,6 +1231,7 @@ def test_export_cloudformation_stack(self, TemplateMock): parameter_values=mock.ANY, language_extensions_enabled=True, parallel_upload=False, + upload_executor=None, ) template_instance_mock.export.assert_called_once_with() self.s3_uploader_mock.upload.assert_called_once_with(mock.ANY, mock.ANY) @@ -1415,6 +1416,7 @@ def test_export_serverless_application(self, TemplateMock): parameter_values=mock.ANY, language_extensions_enabled=True, parallel_upload=False, + upload_executor=None, ) template_instance_mock.export.assert_called_once_with() self.s3_uploader_mock.upload.assert_called_once_with(mock.ANY, mock.ANY) @@ -1594,9 +1596,9 @@ def test_template_export(self, yaml_parse_mock): resource_type2_class.assert_called_once_with(self.uploaders_mock, self.code_signer_mock, None) resource_type2_instance.export.assert_called_once_with("Resource2", mock.ANY, template_dir) - @patch.object(Template, "_execute_jobs_in_parallel") + @patch.object(Template, "_run_export_jobs") @patch("samcli.lib.package.artifact_exporter.yaml_parse") - def test_template_export_parallel_invokes_executor(self, yaml_parse_mock, executor_mock): + def test_template_export_parallel_invokes_executor(self, yaml_parse_mock, run_jobs_mock): parent_dir = os.path.sep template_dir = os.path.join(parent_dir, "foo", "bar") template_path = os.path.join(template_dir, "path") @@ -1635,9 +1637,10 @@ def test_template_export_parallel_invokes_executor(self, yaml_parse_mock, execut ) template_exporter.export() - executor_mock.assert_called_once() - jobs = executor_mock.call_args[0][0] - self.assertEqual(len(jobs), 2) + run_jobs_mock.assert_called_once() + jobs, executor = run_jobs_mock.call_args[0] + self.assertEqual([False, False], [is_nested_stack for is_nested_stack, _ in jobs]) + self.assertIsInstance(executor, ThreadPoolExecutor) @patch("samcli.lib.package.artifact_exporter.is_experimental_enabled") @patch("samcli.lib.package.artifact_exporter.yaml_parse") @@ -1735,20 +1738,23 @@ def test_template_delete_skips_mismatched_package_type(self, yaml_parse_mock): resource_type_class.assert_not_called() - @patch("samcli.lib.package.artifact_exporter.ThreadPoolExecutor", wraps=ThreadPoolExecutor) - def test_execute_jobs_in_parallel_submits_and_waits(self, executor_mock): - job1 = Mock() - job2 = Mock() - template_exporter = Template.__new__(Template) - template_exporter._execute_jobs_in_parallel([job1, job2]) + def test_run_export_jobs_submits_uploads_and_runs_nested_stacks_inline(self): + caller = threading.current_thread() + ran_on = {} + + def record(name): + return lambda: ran_on.setdefault(name, threading.current_thread()) + + jobs = [(False, record("upload1")), (True, record("nested")), (False, record("upload2"))] + with ThreadPoolExecutor(max_workers=2) as executor: + Template._run_export_jobs(jobs, executor) - executor_mock.assert_called_once_with(max_workers=2) - job1.assert_called_once() - job2.assert_called_once() + self.assertIs(caller, ran_on["nested"]) + self.assertIsNot(caller, ran_on["upload1"]) + self.assertIsNot(caller, ran_on["upload2"]) @patch("samcli.lib.package.artifact_exporter.LOG") - @patch("samcli.lib.package.artifact_exporter.DEFAULT_PARALLEL_UPLOAD_WORKERS", 2) - def test_execute_jobs_in_parallel_fails_fast_and_reports_all_failures(self, log_mock): + def test_run_export_jobs_fails_fast_and_reports_all_failures(self, log_mock): slow_started = threading.Event() queued_runs = [] @@ -1765,26 +1771,59 @@ def queued(): queued_runs.append(1) time.sleep(0.05) - template_exporter = Template.__new__(Template) - with self.assertRaises(ValueError): - template_exporter._execute_jobs_in_parallel([fail_first, fail_slow] + [queued] * 20) + jobs = [(False, fail_first), (False, fail_slow)] + [(False, queued)] * 20 + with ThreadPoolExecutor(max_workers=2) as executor: + with self.assertRaises(ValueError): + Template._run_export_jobs(jobs, executor) # Queued jobs are cancelled on the first failure; only jobs a free worker had already # picked up can still run. self.assertLess(len(queued_runs), 20) - logged = [call.args[1] for call in log_mock.error.call_args_list] + logged = [log_call.args[1] for log_call in log_mock.error.call_args_list] self.assertTrue(any(isinstance(error, RuntimeError) for error in logged)) - @patch("samcli.lib.package.artifact_exporter._UPLOAD_SLOTS") - def test_parallel_export_job_bounds_leaf_uploads_but_not_nested_stacks(self, slots_mock): + @patch("samcli.lib.package.artifact_exporter.LOG") + def test_wait_fail_fast_surfaces_first_failure_in_submission_order(self, log_mock): + first, second = Future(), Future() + second.set_exception(RuntimeError("submitted second")) + first.set_exception(ValueError("submitted first")) + + with self.assertRaises(ValueError): + Template._wait_fail_fast([first, second]) + log_mock.error.assert_called_once() + + def test_run_export_jobs_cancels_uploads_when_nested_stack_fails(self): + release = threading.Event() + blocker_started = threading.Event() + + def blocker(): + blocker_started.set() + release.wait(5) + + queued = Mock() + + def failing_nested_stack(): + blocker_started.wait(5) + raise ValueError("nested failure") + + jobs = [(False, blocker), (False, queued), (True, failing_nested_stack)] + with ThreadPoolExecutor(max_workers=1) as executor: + with self.assertRaises(ValueError): + threading.Timer(0.2, release.set).start() + Template._run_export_jobs(jobs, executor) + + queued.assert_not_called() + + def test_export_job_passes_shared_executor_to_nested_stacks(self): + captured = {} + class _NestedStack(CloudFormationStackResource): def __init__(self, *args, **kwargs): pass def export(self, *args, **kwargs): - pass + captured["executor"] = self.upload_executor - leaf_exporter = Mock() template_exporter = Template.__new__(Template) template_exporter.uploaders = Mock() template_exporter.code_signer = Mock() @@ -1792,31 +1831,17 @@ def export(self, *args, **kwargs): template_exporter.language_extensions_enabled = False template_exporter.template_dir = "dir" template_exporter.parallel_upload = True + shared_executor = Mock() - template_exporter._build_export_job(_NestedStack, "Nested", {}, None)() - slots_mock.__enter__.assert_not_called() + template_exporter._build_export_job(_NestedStack, "Nested", {}, None, shared_executor)() - template_exporter._build_export_job(Mock(return_value=leaf_exporter), "Leaf", {}, None)() - slots_mock.__enter__.assert_called_once() - leaf_exporter.export.assert_called_once_with("Leaf", {}, "dir") + self.assertIs(shared_executor, captured["executor"]) def test_thread_safe_upload_cache_key_lock_is_per_key(self): cache = _ThreadSafeUploadCache() self.assertIs(cache.key_lock("a"), cache.key_lock("a")) self.assertIsNot(cache.key_lock("a"), cache.key_lock("b")) - @patch("samcli.lib.package.artifact_exporter.wait") - @patch("samcli.lib.package.artifact_exporter.ThreadPoolExecutor") - def test_execute_jobs_in_parallel_falls_back_to_single_worker(self, executor_mock, wait_mock): - job1 = Mock() - - template_exporter = Template.__new__(Template) - template_exporter._execute_jobs_in_parallel([job1]) - - job1.assert_called_once() - executor_mock.assert_not_called() - wait_mock.assert_not_called() - @patch("samcli.lib.package.artifact_exporter.is_experimental_enabled") @patch("samcli.lib.package.artifact_exporter.yaml_parse") def test_template_export_with_experimental_flag(self, yaml_parse_mock, is_experimental_enabled_mock): From 69662de8b29e51a0eb8ffbe2ded95718114778de Mon Sep 17 00:00:00 2001 From: andy klier Date: Wed, 30 Sep 2026 16:54:55 -0600 Subject: [PATCH 09/14] fix: serialize shared code paths in parallel mode; log all upload failures - In parallel mode always pass a thread-safe cache so jobs sharing a local path take the same per-path lock. Without the PackagePerformance flag the cache is lock-only (results are not stored), so behaviour matches a serial export: every job zips, and the uploader's remote existence check skips re-uploading identical content. This avoids enabling the path-keyed experimental cache implicitly. - Log leaf upload failures on the nested-stack failure path as well, so no upload error is dropped. Co-Authored-By: Claude Opus 5.5 (1M context) --- samcli/lib/package/artifact_exporter.py | 32 +++++++--- .../lib/package/test_artifact_exporter.py | 59 +++++++++++++++++++ tests/unit/lib/package/test_utils.py | 46 +++++++++++++++ 3 files changed, 129 insertions(+), 8 deletions(-) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index 5879894aa7d..5f1e645b675 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -72,11 +72,18 @@ class _ThreadSafeUploadCache(MutableMapping[str, str]): - """Simple thread-safe mapping used to deduplicate uploads across threads.""" + """ + Thread-safe mapping used to deduplicate uploads across threads. + + With ``store=False`` it only provides per-key locks: results are not remembered, so jobs that + share a local path run one after another and rely on the uploader's own remote dedup, exactly + as a serial export does, without enabling the experimental upload cache. + """ - def __init__(self, initial: Optional[MutableMapping[str, str]] = None): + def __init__(self, initial: Optional[MutableMapping[str, str]] = None, store: bool = True): # Copy into a regular dict so we can safely snapshot under a lock self._cache: Dict[str, str] = dict(initial or {}) + self._store = store self._lock = threading.Lock() self._key_locks: Dict[str, threading.Lock] = {} @@ -89,7 +96,9 @@ def __getitem__(self, key: str) -> str: # pragma: no cover - small helper with self._lock: return self._cache[key] - def __setitem__(self, key: str, value: str) -> None: # pragma: no cover - small helper + def __setitem__(self, key: str, value: str) -> None: + if not self._store: + return with self._lock: self._cache[key] = value @@ -655,8 +664,10 @@ def export(self) -> Dict: cache: Optional[MutableMapping[str, str]] = None if is_experimental_enabled(ExperimentalFlag.PackagePerformance): cache = {} - if cache is not None and self.parallel_upload: - cache = _ThreadSafeUploadCache(cache) + if self.parallel_upload: + # Always provide per-path locks in parallel mode so jobs sharing a code path don't all + # upload it at once; only remember results when the experimental cache is enabled. + cache = _ThreadSafeUploadCache(cache, store=cache is not None) if not self.parallel_upload: for _, job in self._collect_export_jobs(cache, None): @@ -728,6 +739,7 @@ def _run_export_jobs(jobs: List[Tuple[bool, Callable[[], None]]], executor: Thre for future in futures: future.cancel() wait(futures) + Template._log_other_failures(futures) raise Template._wait_fail_fast(futures) @@ -742,14 +754,18 @@ def _wait_fail_fast(futures: List[Future]) -> None: for future in futures: future.cancel() wait(futures) - first_error = cast(BaseException, failed[0].exception()) + Template._log_other_failures(futures, surfaced=failed[0]) + raise cast(BaseException, failed[0].exception()) + + @staticmethod + def _log_other_failures(futures: List[Future], surfaced: Optional[Future] = None) -> None: + """Log failures that are not the one being re-raised, so no upload error is silently lost.""" for future in futures: - if future is failed[0] or future.cancelled(): + if future is surfaced or future.cancelled(): continue error = future.exception() if error is not None: LOG.error("Parallel artifact upload also failed: %s", error, exc_info=error) - raise first_error def delete(self, retain_resources: List): """ diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 3ff7e3710e1..dcc7cf71d1c 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -1837,6 +1837,65 @@ def export(self, *args, **kwargs): self.assertIs(shared_executor, captured["executor"]) + def test_lock_only_upload_cache_does_not_remember_results(self): + cache = _ThreadSafeUploadCache(store=False) + cache["path"] = "s3://bucket/key" + self.assertNotIn("path", cache) + self.assertEqual(0, len(cache)) + self.assertIs(cache.key_lock("path"), cache.key_lock("path")) + + @patch("samcli.lib.package.artifact_exporter.is_experimental_enabled", return_value=False) + @patch("samcli.lib.package.artifact_exporter.yaml_parse") + def test_template_export_parallel_without_experimental_cache_uses_lock_only_cache( + self, yaml_parse_mock, is_experimental_enabled_mock + ): + captured = {} + + def capture_cache(uploaders, code_signer, cache): + captured["cache"] = cache + return Mock() + + resource_type_class = Mock(side_effect=capture_cache) + resource_type_class.RESOURCE_TYPE = "resource_type1" + resource_type_class.ARTIFACT_TYPE = ZIP + resource_type_class.EXPORT_DESTINATION = Destination.S3 + yaml_parse_mock.return_value = {"Resources": {"Resource1": {"Type": "resource_type1", "Properties": {}}}} + + with patch("samcli.lib.package.artifact_exporter.open", mock.mock_open(read_data="")): + Template( + os.path.join(os.path.sep, "foo", "path"), + os.path.sep, + self.uploaders_mock, + self.code_signer_mock, + [resource_type_class], + parallel_upload=True, + ).export() + + self.assertIsInstance(captured["cache"], _ThreadSafeUploadCache) + captured["cache"]["path"] = "s3://bucket/key" + self.assertNotIn("path", captured["cache"]) + + @patch("samcli.lib.package.artifact_exporter.LOG") + def test_run_export_jobs_logs_upload_failures_when_nested_stack_fails(self, log_mock): + upload_failed = threading.Event() + + def failing_upload(): + try: + raise RuntimeError("upload failure") + finally: + upload_failed.set() + + def failing_nested_stack(): + upload_failed.wait(5) + raise ValueError("nested failure") + + with ThreadPoolExecutor(max_workers=1) as executor: + with self.assertRaises(ValueError): + Template._run_export_jobs([(False, failing_upload), (True, failing_nested_stack)], executor) + + logged = [log_call.args[1] for log_call in log_mock.error.call_args_list] + self.assertTrue(any(isinstance(error, RuntimeError) for error in logged)) + def test_thread_safe_upload_cache_key_lock_is_per_key(self): cache = _ThreadSafeUploadCache() self.assertIs(cache.key_lock("a"), cache.key_lock("a")) diff --git a/tests/unit/lib/package/test_utils.py b/tests/unit/lib/package/test_utils.py index e69dafac580..6eca0d3f142 100644 --- a/tests/unit/lib/package/test_utils.py +++ b/tests/unit/lib/package/test_utils.py @@ -103,3 +103,49 @@ def upload(): self.assertEqual(1, len(calls)) self.assertEqual(["s3://bucket/key"] * 4, results) + + def test_upload_local_artifacts_serializes_shared_path_with_lock_only_cache(self): + from samcli.lib.package.artifact_exporter import _ThreadSafeUploadCache + + cache = _ThreadSafeUploadCache(store=False) + active = [] + overlaps = [] + state_lock = threading.Lock() + + def slow_zip_and_upload(local_path, *args, **kwargs): + with state_lock: + active.append(local_path) + overlaps.append(len(active)) + time.sleep(0.05) + with state_lock: + active.remove(local_path) + return "s3://bucket/key" + + with ( + tempfile.TemporaryDirectory() as folder, + patch.object(utils, "zip_and_upload", side_effect=slow_zip_and_upload), + ): + threads = [ + threading.Thread( + target=utils.upload_local_artifacts, + kwargs=dict( + resource_type="AWS::Serverless::Function", + resource_id="Function", + resource_dict={"CodeUri": folder}, + property_path="CodeUri", + parent_dir=folder, + uploader=Mock(), + previously_uploaded=cache, + ), + ) + for _ in range(4) + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + # Without the experimental cache every job still zips (as a serial export does, with the + # uploader skipping objects that already exist), but never concurrently for the same path. + self.assertEqual(4, len(overlaps)) + self.assertEqual(1, max(overlaps)) From 84e9cae3bc0e95085cd7f3ff40846f97e08aadaf Mon Sep 17 00:00:00 2001 From: andy klier Date: Wed, 30 Sep 2026 17:12:35 -0600 Subject: [PATCH 10/14] fix: run sibling nested stacks concurrently; configurable upload workers - Run nested-stack coordination on a small per-level pool (one thread per nested stack) instead of inline, so sibling nested stacks submit their uploads to the shared upload pool at the same time. Coordination threads only wait, so they stay out of the upload pool. One fail-fast wait now covers uploads and nested stacks, so a failure is no longer hidden behind the remaining nested-stack exports. - Default to 8 concurrent uploads (each in-flight zip upload holds a temp zip on disk) and allow overriding it with SAM_CLI_PARALLEL_UPLOAD_WORKERS; document it in the flag help, samconfig docs and schema. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/sam-config-docs.md | 2 + samcli/commands/deploy/command.py | 3 +- samcli/lib/package/artifact_exporter.py | 55 +++++++++++++------ schema/samcli.json | 4 +- .../lib/package/test_artifact_exporter.py | 36 ++++++++---- 5 files changed, 70 insertions(+), 30 deletions(-) diff --git a/docs/sam-config-docs.md b/docs/sam-config-docs.md index 28b55adae9a..1cabad27c4b 100644 --- a/docs/sam-config-docs.md +++ b/docs/sam-config-docs.md @@ -37,6 +37,8 @@ parallel_upload=true Setting `parallel_upload` to `true` is equivalent to passing `--parallel-upload` on `sam deploy`, enabling concurrent S3/ECR uploads during the packaging phase. +Up to 8 uploads run at once; set the `SAM_CLI_PARALLEL_UPLOAD_WORKERS` environment variable +to change the number of concurrent uploads. Version ------- diff --git a/samcli/commands/deploy/command.py b/samcli/commands/deploy/command.py index c183efe37ad..7be33ab55f2 100644 --- a/samcli/commands/deploy/command.py +++ b/samcli/commands/deploy/command.py @@ -163,7 +163,8 @@ "--parallel-upload", is_flag=True, default=False, - help="Enable parallel upload of artifacts to S3/ECR during packaging before deployment.", + help="Enable parallel upload of artifacts to S3/ECR during packaging before deployment. " + "Runs up to 8 uploads at once; set the SAM_CLI_PARALLEL_UPLOAD_WORKERS environment variable to change this.", ) @s3_prefix_option @kms_key_id_option diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index 5f1e645b675..2450091d417 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -68,7 +68,30 @@ # NOTE: sriram-mv, A cyclic dependency on `Template` needs to be broken. -DEFAULT_PARALLEL_UPLOAD_WORKERS = max(4, min(32, (os.cpu_count() or 1) * 2)) +# Uploads are I/O bound, and every in-flight zip upload holds a temporary zip on disk (and member +# files in memory), so the pool is kept small by default and is configurable via the environment. +DEFAULT_PARALLEL_UPLOAD_WORKERS = 8 +PARALLEL_UPLOAD_WORKERS_ENV_VAR = "SAM_CLI_PARALLEL_UPLOAD_WORKERS" + + +def get_parallel_upload_workers() -> int: + """Number of concurrent artifact uploads for --parallel-upload.""" + value = os.environ.get(PARALLEL_UPLOAD_WORKERS_ENV_VAR) + if not value: + return DEFAULT_PARALLEL_UPLOAD_WORKERS + try: + workers = int(value) + except ValueError: + workers = 0 + if workers < 1: + LOG.warning( + "Ignoring invalid %s=%r; using %d parallel upload workers", + PARALLEL_UPLOAD_WORKERS_ENV_VAR, + value, + DEFAULT_PARALLEL_UPLOAD_WORKERS, + ) + return DEFAULT_PARALLEL_UPLOAD_WORKERS + return workers class _ThreadSafeUploadCache(MutableMapping[str, str]): @@ -675,7 +698,7 @@ def export(self) -> Dict: elif self.upload_executor is not None: self._run_export_jobs(self._collect_export_jobs(cache, self.upload_executor), self.upload_executor) else: - with ThreadPoolExecutor(max_workers=DEFAULT_PARALLEL_UPLOAD_WORKERS) as executor: + with ThreadPoolExecutor(max_workers=get_parallel_upload_workers()) as executor: self._run_export_jobs(self._collect_export_jobs(cache, executor), executor) return self.template_dict @@ -726,22 +749,20 @@ def _job() -> None: @staticmethod def _run_export_jobs(jobs: List[Tuple[bool, Callable[[], None]]], executor: ThreadPoolExecutor) -> None: """ - Submit artifact uploads to the shared executor and run nested-stack exports in the calling - thread. A nested stack waits for its children, so running it inside a pool worker could - deadlock the shared pool; its children's uploads are submitted to the same executor. + Artifact uploads go to the shared upload executor. Nested-stack exports only coordinate: + they wait on their children's uploads, so they run on a separate pool with one thread per + nested stack at this level, never in the upload pool, where waiting could deadlock it. + Sibling nested stacks therefore proceed concurrently while the shared pool bounds uploads, + and a single fail-fast wait covers both. """ - futures = [executor.submit(job) for is_nested_stack, job in jobs if not is_nested_stack] - try: - for is_nested_stack, job in jobs: - if is_nested_stack: - job() - except BaseException: - for future in futures: - future.cancel() - wait(futures) - Template._log_other_failures(futures) - raise - Template._wait_fail_fast(futures) + upload_futures = [executor.submit(job) for is_nested_stack, job in jobs if not is_nested_stack] + nested_jobs = [job for is_nested_stack, job in jobs if is_nested_stack] + if not nested_jobs: + Template._wait_fail_fast(upload_futures) + return + with ThreadPoolExecutor(max_workers=len(nested_jobs)) as coordinator: + nested_futures = [coordinator.submit(job) for job in nested_jobs] + Template._wait_fail_fast(upload_futures + nested_futures) @staticmethod def _wait_fail_fast(futures: List[Future]) -> None: diff --git a/schema/samcli.json b/schema/samcli.json index f5e31d6ccd1..e45140708ad 100644 --- a/schema/samcli.json +++ b/schema/samcli.json @@ -1325,7 +1325,7 @@ "properties": { "parameters": { "title": "Parameters for the deploy command", - "description": "Available parameters for the deploy command:\n* guided:\nSpecify this flag to allow SAM CLI to guide you through the deployment using guided prompts.\n* template_file:\nAWS SAM template which references built artifacts for resources in the template. (if applicable)\n* no_execute_changeset:\nIndicates whether to execute the change set. Specify this flag to view stack changes before executing the change set.\n* fail_on_empty_changeset:\nSpecify whether AWS SAM CLI should return a non-zero exit code if there are no changes to be made to the stack. Defaults to a non-zero exit code.\n* confirm_changeset:\nPrompt to confirm if the computed changeset is to be deployed by SAM CLI.\n* disable_rollback:\nPreserves the state of previously provisioned resources when an operation fails.\n* on_failure:\nProvide an action to determine what will happen when a stack fails to create. Three actions are available:\n\n- ROLLBACK: This will rollback a stack to a previous known good state.\n\n- DELETE: The stack will rollback to a previous state if one exists, otherwise the stack will be deleted.\n\n- DO_NOTHING: The stack will not rollback or delete, this is the same as disabling rollback.\n\nDefault behaviour is ROLLBACK.\n\n\n\nThis option is mutually exclusive with --disable-rollback/--no-disable-rollback. You can provide\n--on-failure or --disable-rollback/--no-disable-rollback but not both at the same time.\n* max_wait_duration:\nMaximum duration in minutes to wait for the deployment to complete.\n* express:\nUse CloudFormation Express mode to speed up deployments by completing once resource configuration is applied, without waiting for full stabilization.\n* stack_name:\nName of the AWS CloudFormation stack.\n* s3_bucket:\nAWS S3 bucket where artifacts referenced in the template are uploaded.\n* image_repository:\nAWS ECR repository URI where artifacts referenced in the template are uploaded.\n* image_repositories:\nMapping of Function Logical ID to AWS ECR Repository URI.\n\nExample: Function_Logical_ID=ECR_Repo_Uri\nThis option can be specified multiple times.\n* force_upload:\nIndicates whether to override existing files in the S3 bucket. Specify this flag to upload artifacts even if they match existing artifacts in the S3 bucket.\n* parallel_upload:\nEnable parallel upload of artifacts to S3/ECR during packaging before deployment.\n* s3_prefix:\nPrefix name that is added to the artifact's name when it is uploaded to the AWS S3 bucket.\n* kms_key_id:\nThe ID of an AWS KMS key that is used to encrypt artifacts that are at rest in the AWS S3 bucket.\n* role_arn:\nARN of an IAM role that AWS Cloudformation assumes when executing a deployment change set.\n* use_json:\nIndicates whether to use JSON as the format for the output AWS CloudFormation template. YAML is used by default.\n* resolve_s3:\nAutomatically resolve AWS S3 bucket for non-guided deployments. Enabling this option will also create a managed default AWS S3 bucket for you. If one does not provide a --s3-bucket value, the managed bucket will be used. Do not use --guided with this option.\n* resolve_image_repos:\nAutomatically create and delete ECR repositories for image-based functions in non-guided deployments. A companion stack containing ECR repos for each function will be deployed along with the template stack. Automatically created image repositories will be deleted if the corresponding functions are removed.\n* metadata:\nMap of metadata to attach to ALL the artifacts that are referenced in the template.\n* notification_arns:\nARNs of SNS topics that AWS Cloudformation associates with the stack.\n* tags:\nList of tags to associate with the stack.\n* parameter_overrides:\nString that contains AWS CloudFormation parameter overrides encoded as key=value pairs.\n* signing_profiles:\nA string that contains Code Sign configuration parameters as FunctionOrLayerNameToSign=SigningProfileName:SigningProfileOwner Since signing profile owner is optional, it could also be written as FunctionOrLayerNameToSign=SigningProfileName\n* no_progressbar:\nDoes not showcase a progress bar when uploading artifacts to S3 and pushing docker images to ECR\n* capabilities:\nList of capabilities that one must specify before AWS Cloudformation can create certain stacks.\n\nAccepted Values: CAPABILITY_IAM, CAPABILITY_NAMED_IAM, CAPABILITY_RESOURCE_POLICY, CAPABILITY_AUTO_EXPAND.\n\nLearn more at: https://docs.aws.amazon.com/serverlessrepo/latest/devguide/acknowledging-application-capabilities.html\n* language_extensions:\nExpand AWS::LanguageExtensions transforms (Fn::ForEach, Fn::Length, Fn::ToJsonString, Fn::FindInMap with DefaultValue) locally before running SAM transforms. Off by default. Equivalent env var: SAM_CLI_ENABLE_LANGUAGE_EXTENSIONS=1.\n* output:\nOutput the results from the command in a given output format. Supported formats: text (default), json.\n* profile:\nSelect a specific profile from your credential file to get AWS credentials.\n* region:\nSet the AWS Region of the service. (e.g. us-east-1)\n* beta_features:\nEnable/Disable beta features.\n* debug:\nTurn on debug logging to print debug message generated by AWS SAM CLI and display timestamps.\n* save_params:\nSave the parameters provided via the command line to the configuration file.", + "description": "Available parameters for the deploy command:\n* guided:\nSpecify this flag to allow SAM CLI to guide you through the deployment using guided prompts.\n* template_file:\nAWS SAM template which references built artifacts for resources in the template. (if applicable)\n* no_execute_changeset:\nIndicates whether to execute the change set. Specify this flag to view stack changes before executing the change set.\n* fail_on_empty_changeset:\nSpecify whether AWS SAM CLI should return a non-zero exit code if there are no changes to be made to the stack. Defaults to a non-zero exit code.\n* confirm_changeset:\nPrompt to confirm if the computed changeset is to be deployed by SAM CLI.\n* disable_rollback:\nPreserves the state of previously provisioned resources when an operation fails.\n* on_failure:\nProvide an action to determine what will happen when a stack fails to create. Three actions are available:\n\n- ROLLBACK: This will rollback a stack to a previous known good state.\n\n- DELETE: The stack will rollback to a previous state if one exists, otherwise the stack will be deleted.\n\n- DO_NOTHING: The stack will not rollback or delete, this is the same as disabling rollback.\n\nDefault behaviour is ROLLBACK.\n\n\n\nThis option is mutually exclusive with --disable-rollback/--no-disable-rollback. You can provide\n--on-failure or --disable-rollback/--no-disable-rollback but not both at the same time.\n* max_wait_duration:\nMaximum duration in minutes to wait for the deployment to complete.\n* express:\nUse CloudFormation Express mode to speed up deployments by completing once resource configuration is applied, without waiting for full stabilization.\n* stack_name:\nName of the AWS CloudFormation stack.\n* s3_bucket:\nAWS S3 bucket where artifacts referenced in the template are uploaded.\n* image_repository:\nAWS ECR repository URI where artifacts referenced in the template are uploaded.\n* image_repositories:\nMapping of Function Logical ID to AWS ECR Repository URI.\n\nExample: Function_Logical_ID=ECR_Repo_Uri\nThis option can be specified multiple times.\n* force_upload:\nIndicates whether to override existing files in the S3 bucket. Specify this flag to upload artifacts even if they match existing artifacts in the S3 bucket.\n* parallel_upload:\nEnable parallel upload of artifacts to S3/ECR during packaging before deployment. Runs up to 8 uploads at once; set the SAM_CLI_PARALLEL_UPLOAD_WORKERS environment variable to change this.\n* s3_prefix:\nPrefix name that is added to the artifact's name when it is uploaded to the AWS S3 bucket.\n* kms_key_id:\nThe ID of an AWS KMS key that is used to encrypt artifacts that are at rest in the AWS S3 bucket.\n* role_arn:\nARN of an IAM role that AWS Cloudformation assumes when executing a deployment change set.\n* use_json:\nIndicates whether to use JSON as the format for the output AWS CloudFormation template. YAML is used by default.\n* resolve_s3:\nAutomatically resolve AWS S3 bucket for non-guided deployments. Enabling this option will also create a managed default AWS S3 bucket for you. If one does not provide a --s3-bucket value, the managed bucket will be used. Do not use --guided with this option.\n* resolve_image_repos:\nAutomatically create and delete ECR repositories for image-based functions in non-guided deployments. A companion stack containing ECR repos for each function will be deployed along with the template stack. Automatically created image repositories will be deleted if the corresponding functions are removed.\n* metadata:\nMap of metadata to attach to ALL the artifacts that are referenced in the template.\n* notification_arns:\nARNs of SNS topics that AWS Cloudformation associates with the stack.\n* tags:\nList of tags to associate with the stack.\n* parameter_overrides:\nString that contains AWS CloudFormation parameter overrides encoded as key=value pairs.\n* signing_profiles:\nA string that contains Code Sign configuration parameters as FunctionOrLayerNameToSign=SigningProfileName:SigningProfileOwner Since signing profile owner is optional, it could also be written as FunctionOrLayerNameToSign=SigningProfileName\n* no_progressbar:\nDoes not showcase a progress bar when uploading artifacts to S3 and pushing docker images to ECR\n* capabilities:\nList of capabilities that one must specify before AWS Cloudformation can create certain stacks.\n\nAccepted Values: CAPABILITY_IAM, CAPABILITY_NAMED_IAM, CAPABILITY_RESOURCE_POLICY, CAPABILITY_AUTO_EXPAND.\n\nLearn more at: https://docs.aws.amazon.com/serverlessrepo/latest/devguide/acknowledging-application-capabilities.html\n* language_extensions:\nExpand AWS::LanguageExtensions transforms (Fn::ForEach, Fn::Length, Fn::ToJsonString, Fn::FindInMap with DefaultValue) locally before running SAM transforms. Off by default. Equivalent env var: SAM_CLI_ENABLE_LANGUAGE_EXTENSIONS=1.\n* output:\nOutput the results from the command in a given output format. Supported formats: text (default), json.\n* profile:\nSelect a specific profile from your credential file to get AWS credentials.\n* region:\nSet the AWS Region of the service. (e.g. us-east-1)\n* beta_features:\nEnable/Disable beta features.\n* debug:\nTurn on debug logging to print debug message generated by AWS SAM CLI and display timestamps.\n* save_params:\nSave the parameters provided via the command line to the configuration file.", "type": "object", "properties": { "guided": { @@ -1413,7 +1413,7 @@ "parallel_upload": { "title": "parallel_upload", "type": "boolean", - "description": "Enable parallel upload of artifacts to S3/ECR during packaging before deployment." + "description": "Enable parallel upload of artifacts to S3/ECR during packaging before deployment. Runs up to 8 uploads at once; set the SAM_CLI_PARALLEL_UPLOAD_WORKERS environment variable to change this." }, "s3_prefix": { "title": "s3_prefix", diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index dcc7cf71d1c..b9978dad2d4 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -19,6 +19,7 @@ from unittest import mock from unittest.mock import call, patch, Mock, MagicMock +from parameterized import parameterized from samcli.commands._utils.experimental import ExperimentalFlag from samcli.commands.package import exceptions from samcli.commands.package.exceptions import ExportFailedError @@ -38,6 +39,7 @@ _export_global_artifacts_pass, _resolve_nested_stack_parameters, _ThreadSafeUploadCache, + get_parallel_upload_workers, ) from samcli.lib.package.language_extensions_packaging import merge_language_extensions_s3_uris from samcli.lib.intrinsic_resolver.intrinsics_symbol_table import IntrinsicsSymbolTable @@ -1738,20 +1740,26 @@ def test_template_delete_skips_mismatched_package_type(self, yaml_parse_mock): resource_type_class.assert_not_called() - def test_run_export_jobs_submits_uploads_and_runs_nested_stacks_inline(self): - caller = threading.current_thread() + def test_run_export_jobs_runs_nested_stacks_outside_the_upload_pool(self): ran_on = {} def record(name): - return lambda: ran_on.setdefault(name, threading.current_thread()) + return lambda: ran_on.setdefault(name, threading.current_thread().name) jobs = [(False, record("upload1")), (True, record("nested")), (False, record("upload2"))] - with ThreadPoolExecutor(max_workers=2) as executor: + with ThreadPoolExecutor(max_workers=2, thread_name_prefix="upload") as executor: Template._run_export_jobs(jobs, executor) - self.assertIs(caller, ran_on["nested"]) - self.assertIsNot(caller, ran_on["upload1"]) - self.assertIsNot(caller, ran_on["upload2"]) + self.assertTrue(ran_on["upload1"].startswith("upload")) + self.assertTrue(ran_on["upload2"].startswith("upload")) + self.assertFalse(ran_on["nested"].startswith("upload")) + + def test_run_export_jobs_runs_sibling_nested_stacks_concurrently(self): + # Each nested stack waits for the other; this only completes if they run at the same time. + barrier = threading.Barrier(2, timeout=5) + jobs = [(True, barrier.wait), (True, barrier.wait)] + with ThreadPoolExecutor(max_workers=1) as executor: + Template._run_export_jobs(jobs, executor) @patch("samcli.lib.package.artifact_exporter.LOG") def test_run_export_jobs_fails_fast_and_reports_all_failures(self, log_mock): @@ -1876,7 +1884,7 @@ def capture_cache(uploaders, code_signer, cache): self.assertNotIn("path", captured["cache"]) @patch("samcli.lib.package.artifact_exporter.LOG") - def test_run_export_jobs_logs_upload_failures_when_nested_stack_fails(self, log_mock): + def test_run_export_jobs_reports_nested_stack_and_upload_failures(self, log_mock): upload_failed = threading.Event() def failing_upload(): @@ -1890,11 +1898,19 @@ def failing_nested_stack(): raise ValueError("nested failure") with ThreadPoolExecutor(max_workers=1) as executor: - with self.assertRaises(ValueError): + with self.assertRaises(RuntimeError): Template._run_export_jobs([(False, failing_upload), (True, failing_nested_stack)], executor) logged = [log_call.args[1] for log_call in log_mock.error.call_args_list] - self.assertTrue(any(isinstance(error, RuntimeError) for error in logged)) + self.assertTrue(any(isinstance(error, ValueError) for error in logged)) + + @parameterized.expand([(None, 8), ("3", 3), ("0", 8), ("-2", 8), ("abc", 8)]) + def test_parallel_upload_workers_from_environment(self, value, expected): + env = {} if value is None else {"SAM_CLI_PARALLEL_UPLOAD_WORKERS": value} + with patch.dict(os.environ, env, clear=False): + if value is None: + os.environ.pop("SAM_CLI_PARALLEL_UPLOAD_WORKERS", None) + self.assertEqual(expected, get_parallel_upload_workers()) def test_thread_safe_upload_cache_key_lock_is_per_key(self): cache = _ThreadSafeUploadCache() From 1418fe5af888891204d88c64f80752e40b185432 Mon Sep 17 00:00:00 2001 From: andy klier Date: Wed, 30 Sep 2026 17:44:51 -0600 Subject: [PATCH 11/14] fix: memoize shared code paths per export; bound nested template uploads - In parallel mode without the experimental cache, memoize uploads for the duration of the export, keyed by local path plus packaging (Lambda zip vs plain zip, and extension), so jobs sharing a CodeUri zip and upload once and waiters return immediately instead of re-zipping while holding pool workers. Differently packaged uses of one directory stay separate. - Upload rendered nested-stack templates through the shared upload pool so the configured worker count bounds every S3 upload. Co-Authored-By: Claude Opus 5.5 (1M context) --- samcli/lib/package/artifact_exporter.py | 37 ++++++++---- samcli/lib/package/utils.py | 21 ++++--- .../lib/package/test_artifact_exporter.py | 46 ++++++++++++--- tests/unit/lib/package/test_utils.py | 59 +++++++++---------- 4 files changed, 104 insertions(+), 59 deletions(-) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index 2450091d417..776231baf3b 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -98,18 +98,26 @@ class _ThreadSafeUploadCache(MutableMapping[str, str]): """ Thread-safe mapping used to deduplicate uploads across threads. - With ``store=False`` it only provides per-key locks: results are not remembered, so jobs that - share a local path run one after another and rely on the uploader's own remote dedup, exactly - as a serial export does, without enabling the experimental upload cache. + With ``key_by_packaging=True`` it is an export-scoped memo keyed by local path *and* how the + path is packaged (zip method and extension), so resources that share a directory but package it + differently, e.g. a Lambda function and an Elastic Beanstalk application version, never receive + each other's artifact. Without it, keys are plain local paths, matching the experimental + PackagePerformance cache. """ - def __init__(self, initial: Optional[MutableMapping[str, str]] = None, store: bool = True): + def __init__(self, initial: Optional[MutableMapping[str, str]] = None, key_by_packaging: bool = False): # Copy into a regular dict so we can safely snapshot under a lock self._cache: Dict[str, str] = dict(initial or {}) - self._store = store + self._key_by_packaging = key_by_packaging self._lock = threading.Lock() self._key_locks: Dict[str, threading.Lock] = {} + def cache_key(self, local_path: str, packaging: str, extension: Optional[str]) -> str: + """Key for an upload of ``local_path`` packaged as ``packaging`` with ``extension``.""" + if not self._key_by_packaging: + return local_path + return f"{packaging}|{extension or ''}|{local_path}" + def key_lock(self, key: str) -> threading.Lock: """Lock serializing check-then-upload for a single key; other keys proceed concurrently.""" with self._lock: @@ -119,9 +127,7 @@ def __getitem__(self, key: str) -> str: # pragma: no cover - small helper with self._lock: return self._cache[key] - def __setitem__(self, key: str, value: str) -> None: - if not self._store: - return + def __setitem__(self, key: str, value: str) -> None: # pragma: no cover - small helper with self._lock: self._cache[key] = value @@ -325,7 +331,13 @@ def do_export(self, resource_id, resource_dict, parent_dir): temporary_file.write(exported_template_str) temporary_file.flush() remote_path = get_uploaded_s3_object_name(file_path=temporary_file.name, extension="template") - url = self.uploader.upload(temporary_file.name, remote_path) + upload_executor = getattr(self, "upload_executor", None) + if upload_executor is not None: + # Keep the rendered child template upload under the shared upload bound; this runs on + # a nested-stack coordination thread, which may wait on the upload pool. + url = upload_executor.submit(self.uploader.upload, temporary_file.name, remote_path).result() + else: + url = self.uploader.upload(temporary_file.name, remote_path) # TemplateUrl property requires S3 URL to be in path-style format parts = parse_s3_url(url, version_property="Version") @@ -688,9 +700,10 @@ def export(self) -> Dict: if is_experimental_enabled(ExperimentalFlag.PackagePerformance): cache = {} if self.parallel_upload: - # Always provide per-path locks in parallel mode so jobs sharing a code path don't all - # upload it at once; only remember results when the experimental cache is enabled. - cache = _ThreadSafeUploadCache(cache, store=cache is not None) + # Parallel jobs sharing a code path must not all zip and upload it at once. Keep the + # experimental cache's path keys when it is on; otherwise use an export-scoped memo that + # also keys on packaging, so it cannot conflate differently packaged artifacts. + cache = _ThreadSafeUploadCache(cache, key_by_packaging=cache is None) if not self.parallel_upload: for _, job in self._collect_export_jobs(cache, None): diff --git a/samcli/lib/package/utils.py b/samcli/lib/package/utils.py index eb1b9da734c..73e5881c2a2 100644 --- a/samcli/lib/package/utils.py +++ b/samcli/lib/package/utils.py @@ -186,12 +186,17 @@ def upload_local_artifacts( local_path = make_abs_path(parent_dir, local_path) - # A thread-safe cache (parallel uploads) exposes a per-key lock so the check-then-upload below - # is atomic per path: concurrent jobs sharing a local path upload it once. + is_lambda_package = resource_type in LAMBDA_LOCAL_RESOURCES + zip_method = make_zip_with_lambda_permissions if is_lambda_package else make_zip + # A thread-safe cache (parallel uploads) keys results by how the path is packaged and exposes a + # per-key lock, so the check-then-upload below is atomic: concurrent jobs sharing a local path + # zip and upload it once and the rest reuse the result. + key_fn = getattr(previously_uploaded, "cache_key", None) + cache_key = key_fn(local_path, "lambda-zip" if is_lambda_package else "zip", extension) if key_fn else local_path key_lock = getattr(previously_uploaded, "key_lock", None) - with key_lock(local_path) if key_lock else contextlib.nullcontext(): - if previously_uploaded and local_path in previously_uploaded: - result = previously_uploaded[local_path] + with key_lock(cache_key) if key_lock else contextlib.nullcontext(): + if previously_uploaded and cache_key in previously_uploaded: + result = previously_uploaded[cache_key] LOG.debug("Skipping upload of %s since is already uploaded to %s", local_path, result) return cast(str, result) @@ -201,17 +206,17 @@ def upload_local_artifacts( local_path, uploader, extension, - zip_method=make_zip_with_lambda_permissions if resource_type in LAMBDA_LOCAL_RESOURCES else make_zip, + zip_method=zip_method, ) if previously_uploaded is not None: - previously_uploaded[local_path] = result + previously_uploaded[cache_key] = result return result # Path could be pointing to a file. Upload the file if is_local_file(local_path): result = uploader.upload_with_dedup(local_path) if previously_uploaded is not None: - previously_uploaded[local_path] = result + previously_uploaded[cache_key] = result return result raise InvalidLocalPathError(resource_id=resource_id, property_name=property_path, local_path=local_path) diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index b9978dad2d4..b71977e8046 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -1845,16 +1845,17 @@ def export(self, *args, **kwargs): self.assertIs(shared_executor, captured["executor"]) - def test_lock_only_upload_cache_does_not_remember_results(self): - cache = _ThreadSafeUploadCache(store=False) - cache["path"] = "s3://bucket/key" - self.assertNotIn("path", cache) - self.assertEqual(0, len(cache)) - self.assertIs(cache.key_lock("path"), cache.key_lock("path")) + def test_packaging_keyed_upload_cache_separates_packaging(self): + cache = _ThreadSafeUploadCache(key_by_packaging=True) + function_key = cache.cache_key("/code", "lambda-zip", None) + layer_key = cache.cache_key("/code", "zip", None) + self.assertNotEqual(function_key, layer_key) + self.assertNotEqual(function_key, cache.cache_key("/code", "lambda-zip", "jar")) + self.assertEqual("/code", _ThreadSafeUploadCache().cache_key("/code", "zip", None)) @patch("samcli.lib.package.artifact_exporter.is_experimental_enabled", return_value=False) @patch("samcli.lib.package.artifact_exporter.yaml_parse") - def test_template_export_parallel_without_experimental_cache_uses_lock_only_cache( + def test_template_export_parallel_without_experimental_cache_uses_packaging_keyed_cache( self, yaml_parse_mock, is_experimental_enabled_mock ): captured = {} @@ -1880,8 +1881,7 @@ def capture_cache(uploaders, code_signer, cache): ).export() self.assertIsInstance(captured["cache"], _ThreadSafeUploadCache) - captured["cache"]["path"] = "s3://bucket/key" - self.assertNotIn("path", captured["cache"]) + self.assertNotEqual("/code", captured["cache"].cache_key("/code", "zip", None)) @patch("samcli.lib.package.artifact_exporter.LOG") def test_run_export_jobs_reports_nested_stack_and_upload_failures(self, log_mock): @@ -1912,6 +1912,34 @@ def test_parallel_upload_workers_from_environment(self, value, expected): os.environ.pop("SAM_CLI_PARALLEL_UPLOAD_WORKERS", None) self.assertEqual(expected, get_parallel_upload_workers()) + @patch("samcli.lib.package.artifact_exporter.Template") + @patch("samcli.lib.package.artifact_exporter.yaml_dump", return_value="template") + def test_nested_stack_template_upload_goes_through_shared_upload_pool(self, yaml_dump_mock, template_mock): + template_mock.return_value.export.return_value = {} + uploader = Mock() + uploader.upload.return_value = "s3://bucket/child.template" + uploader.to_path_style_s3_url.return_value = "https://s3.amazonaws.com/bucket/child.template" + uploaders = Mock() + uploaders.get.return_value = uploader + stack_resource = CloudFormationStackResource(uploaders, Mock()) + stack_resource.language_extensions_enabled = False + stack_resource.parallel_upload = True + + with ( + tempfile.TemporaryDirectory() as parent_dir, + ThreadPoolExecutor(max_workers=1, thread_name_prefix="upload") as executor, + ): + open(os.path.join(parent_dir, "child.yaml"), "w").close() + stack_resource.upload_executor = executor + upload_threads = [] + uploader.upload.side_effect = lambda *args: ( + upload_threads.append(threading.current_thread().name) or "s3://bucket/child.template" + ) + stack_resource.do_export("Child", {"TemplateURL": "child.yaml"}, parent_dir) + + self.assertEqual(1, len(upload_threads)) + self.assertTrue(upload_threads[0].startswith("upload")) + def test_thread_safe_upload_cache_key_lock_is_per_key(self): cache = _ThreadSafeUploadCache() self.assertIs(cache.key_lock("a"), cache.key_lock("a")) diff --git a/tests/unit/lib/package/test_utils.py b/tests/unit/lib/package/test_utils.py index 6eca0d3f142..6eff420db48 100644 --- a/tests/unit/lib/package/test_utils.py +++ b/tests/unit/lib/package/test_utils.py @@ -104,48 +104,47 @@ def upload(): self.assertEqual(1, len(calls)) self.assertEqual(["s3://bucket/key"] * 4, results) - def test_upload_local_artifacts_serializes_shared_path_with_lock_only_cache(self): + def test_upload_local_artifacts_memoizes_by_packaging_across_threads(self): from samcli.lib.package.artifact_exporter import _ThreadSafeUploadCache - cache = _ThreadSafeUploadCache(store=False) - active = [] - overlaps = [] - state_lock = threading.Lock() + cache = _ThreadSafeUploadCache(key_by_packaging=True) + calls = [] - def slow_zip_and_upload(local_path, *args, **kwargs): - with state_lock: - active.append(local_path) - overlaps.append(len(active)) + def slow_zip_and_upload(local_path, uploader, extension, zip_method): + calls.append(zip_method) time.sleep(0.05) - with state_lock: - active.remove(local_path) - return "s3://bucket/key" + return f"s3://bucket/{len(calls)}" with ( tempfile.TemporaryDirectory() as folder, patch.object(utils, "zip_and_upload", side_effect=slow_zip_and_upload), ): - threads = [ - threading.Thread( - target=utils.upload_local_artifacts, - kwargs=dict( - resource_type="AWS::Serverless::Function", - resource_id="Function", - resource_dict={"CodeUri": folder}, - property_path="CodeUri", - parent_dir=folder, - uploader=Mock(), - previously_uploaded=cache, - ), + + def upload(resource_type): + return utils.upload_local_artifacts( + resource_type=resource_type, + resource_id="Resource", + resource_dict={"CodeUri": folder}, + property_path="CodeUri", + parent_dir=folder, + uploader=Mock(), + previously_uploaded=cache, ) - for _ in range(4) + + results = [] + threads = [ + threading.Thread(target=lambda: results.append(upload("AWS::Serverless::Function"))) for _ in range(4) ] for thread in threads: thread.start() for thread in threads: thread.join() - - # Without the experimental cache every job still zips (as a serial export does, with the - # uploader skipping objects that already exist), but never concurrently for the same path. - self.assertEqual(4, len(overlaps)) - self.assertEqual(1, max(overlaps)) + bundle_url = upload("AWS::ElasticBeanstalk::ApplicationVersion") + + # Four functions sharing a directory zip and upload it once; a non-Lambda resource using the + # same directory is packaged differently, so it gets its own upload, not the functions' zip. + self.assertEqual(1, len(set(results))) + self.assertEqual(2, len(calls)) + self.assertIs(utils.make_zip_with_lambda_permissions, calls[0]) + self.assertIs(utils.make_zip, calls[1]) + self.assertNotIn(bundle_url, results) From f32ed536b613371a7a801dc96f752126ece235f5 Mon Sep 17 00:00:00 2001 From: andy klier Date: Wed, 30 Sep 2026 18:01:42 -0600 Subject: [PATCH 12/14] fix: propagate fail-fast through nested stacks with a shared abort signal Pass a threading.Event down with the shared upload executor. The first failure anywhere in the export sets it; export jobs, nested-stack job submission and nested template uploads that have not started yet raise an internal _UploadAborted instead of doing work, so a failure no longer lets the rest of the nested tree upload to completion. Skipped work is not logged as an additional failure, and the surfaced error is always the first real failure in submission order. Co-Authored-By: Claude Opus 5.5 (1M context) --- samcli/lib/package/artifact_exporter.py | 70 ++++++++++++++----- .../lib/package/test_artifact_exporter.py | 59 +++++++++++++++- 2 files changed, 111 insertions(+), 18 deletions(-) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index 776231baf3b..c6c9457fc04 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -94,6 +94,10 @@ def get_parallel_upload_workers() -> int: return workers +class _UploadAborted(Exception): + """Raised by parallel export work that was skipped because another upload already failed.""" + + class _ThreadSafeUploadCache(MutableMapping[str, str]): """ Thread-safe mapping used to deduplicate uploads across threads. @@ -332,6 +336,9 @@ def do_export(self, resource_id, resource_dict, parent_dir): temporary_file.flush() remote_path = get_uploaded_s3_object_name(file_path=temporary_file.name, extension="template") upload_executor = getattr(self, "upload_executor", None) + upload_abort = getattr(self, "upload_abort", None) + if upload_abort is not None and upload_abort.is_set(): + raise _UploadAborted() if upload_executor is not None: # Keep the rendered child template upload under the shared upload bound; this runs on # a nested-stack coordination thread, which may wait on the upload pool. @@ -363,6 +370,7 @@ def _do_export_without_language_extensions(self, resource_id: str, template_path language_extensions_enabled=False, parallel_upload=getattr(self, "parallel_upload", False), upload_executor=getattr(self, "upload_executor", None), + upload_abort=getattr(self, "upload_abort", None), ).export() def _do_export_with_language_extensions( @@ -459,6 +467,7 @@ def _do_export_with_language_extensions( language_extensions_enabled=self.language_extensions_enabled, parallel_upload=getattr(self, "parallel_upload", False), upload_executor=getattr(self, "upload_executor", None), + upload_abort=getattr(self, "upload_abort", None), ) exported_template = template.export() @@ -494,6 +503,7 @@ def _do_export_with_language_extensions( language_extensions_enabled=self.language_extensions_enabled, parallel_upload=getattr(self, "parallel_upload", False), upload_executor=getattr(self, "upload_executor", None), + upload_abort=getattr(self, "upload_abort", None), ).export() return exported_template_dict @@ -578,6 +588,7 @@ def __init__( language_extensions_enabled: bool = False, parallel_upload: bool = False, upload_executor: Optional[ThreadPoolExecutor] = None, + upload_abort: Optional[threading.Event] = None, ): """ Reads the template and makes it ready for export @@ -626,6 +637,8 @@ def __init__( # Shared by the root template and every nested-stack child so that one pool bounds both # threads and in-flight uploads for the whole export. Created by the root when None. self.upload_executor = upload_executor + # Set on the first failure anywhere in the export so descendants stop starting new uploads. + self.upload_abort = upload_abort def _export_global_artifacts(self, template_dict: Dict) -> Dict: """See module-level _export_global_artifacts_pass for the canonical @@ -706,18 +719,23 @@ def export(self) -> Dict: cache = _ThreadSafeUploadCache(cache, key_by_packaging=cache is None) if not self.parallel_upload: - for _, job in self._collect_export_jobs(cache, None): + for _, job in self._collect_export_jobs(cache, None, None): job() elif self.upload_executor is not None: - self._run_export_jobs(self._collect_export_jobs(cache, self.upload_executor), self.upload_executor) + jobs = self._collect_export_jobs(cache, self.upload_executor, self.upload_abort) + self._run_export_jobs(jobs, self.upload_executor, self.upload_abort) else: + abort = threading.Event() with ThreadPoolExecutor(max_workers=get_parallel_upload_workers()) as executor: - self._run_export_jobs(self._collect_export_jobs(cache, executor), executor) + self._run_export_jobs(self._collect_export_jobs(cache, executor, abort), executor, abort) return self.template_dict def _collect_export_jobs( - self, cache: Optional[MutableMapping[str, str]], executor: Optional[ThreadPoolExecutor] + self, + cache: Optional[MutableMapping[str, str]], + executor: Optional[ThreadPoolExecutor], + abort: Optional[threading.Event], ) -> List[Tuple[bool, Callable[[], None]]]: """Return (is_nested_stack, job) pairs for every resource that has artifacts to export.""" jobs: List[Tuple[bool, Callable[[], None]]] = [] @@ -736,7 +754,7 @@ def _collect_export_jobs( is_nested_stack = isinstance(exporter_class, type) and issubclass( exporter_class, CloudFormationStackResource ) - job = self._build_export_job(exporter_class, full_path, resource_dict, cache, executor) + job = self._build_export_job(exporter_class, full_path, resource_dict, cache, executor, abort) jobs.append((is_nested_stack, job)) return jobs @@ -747,49 +765,67 @@ def _build_export_job( resource_dict: Dict, cache: Optional[MutableMapping[str, str]], executor: Optional[ThreadPoolExecutor] = None, + abort: Optional[threading.Event] = None, ) -> Callable[[], None]: def _job() -> None: + if abort is not None and abort.is_set(): + raise _UploadAborted() # Export code resources exporter = exporter_class(self.uploaders, self.code_signer, cache) exporter.parent_parameter_values = self.parameter_values exporter.language_extensions_enabled = self.language_extensions_enabled exporter.parallel_upload = self.parallel_upload exporter.upload_executor = executor + exporter.upload_abort = abort exporter.export(resource_full_path, resource_dict, self.template_dir) return _job @staticmethod - def _run_export_jobs(jobs: List[Tuple[bool, Callable[[], None]]], executor: ThreadPoolExecutor) -> None: + def _run_export_jobs( + jobs: List[Tuple[bool, Callable[[], None]]], + executor: ThreadPoolExecutor, + abort: Optional[threading.Event] = None, + ) -> None: """ Artifact uploads go to the shared upload executor. Nested-stack exports only coordinate: they wait on their children's uploads, so they run on a separate pool with one thread per nested stack at this level, never in the upload pool, where waiting could deadlock it. Sibling nested stacks therefore proceed concurrently while the shared pool bounds uploads, - and a single fail-fast wait covers both. + and a single fail-fast wait covers both. ``abort`` is shared by the whole export: once any + job fails, work that has not started yet at any level is skipped. """ + if abort is not None and abort.is_set(): + raise _UploadAborted() upload_futures = [executor.submit(job) for is_nested_stack, job in jobs if not is_nested_stack] nested_jobs = [job for is_nested_stack, job in jobs if is_nested_stack] if not nested_jobs: - Template._wait_fail_fast(upload_futures) + Template._wait_fail_fast(upload_futures, abort) return with ThreadPoolExecutor(max_workers=len(nested_jobs)) as coordinator: nested_futures = [coordinator.submit(job) for job in nested_jobs] - Template._wait_fail_fast(upload_futures + nested_futures) + Template._wait_fail_fast(upload_futures + nested_futures, abort) @staticmethod - def _wait_fail_fast(futures: List[Future]) -> None: - """Wait for futures; on the first failure cancel the rest, log other failures, re-raise the first.""" + def _wait_fail_fast(futures: List[Future], abort: Optional[threading.Event] = None) -> None: + """ + Wait for futures. On the first failure, signal the rest of the export to stop, cancel what + has not started here, wait for running work, log the other failures and re-raise the first + real one (in submission order, so the surfaced error is stable). + """ done, _ = wait(futures, return_when=FIRST_EXCEPTION) - # Submission order keeps the surfaced error stable when several jobs have already failed. - failed = [future for future in futures if future in done and future.exception() is not None] - if not failed: + if not any(future.exception() is not None for future in done): return + if abort is not None: + abort.set() for future in futures: future.cancel() wait(futures) - Template._log_other_failures(futures, surfaced=failed[0]) - raise cast(BaseException, failed[0].exception()) + failed = [future for future in futures if not future.cancelled() and future.exception() is not None] + real = [future for future in failed if not isinstance(future.exception(), _UploadAborted)] + surfaced = (real or failed)[0] + Template._log_other_failures(futures, surfaced=surfaced) + raise cast(BaseException, surfaced.exception()) @staticmethod def _log_other_failures(futures: List[Future], surfaced: Optional[Future] = None) -> None: @@ -798,7 +834,7 @@ def _log_other_failures(futures: List[Future], surfaced: Optional[Future] = None if future is surfaced or future.cancelled(): continue error = future.exception() - if error is not None: + if error is not None and not isinstance(error, _UploadAborted): LOG.error("Parallel artifact upload also failed: %s", error, exc_info=error) def delete(self, retain_resources: List): diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index b71977e8046..88221db0c10 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -39,6 +39,7 @@ _export_global_artifacts_pass, _resolve_nested_stack_parameters, _ThreadSafeUploadCache, + _UploadAborted, get_parallel_upload_workers, ) from samcli.lib.package.language_extensions_packaging import merge_language_extensions_s3_uris @@ -1234,6 +1235,7 @@ def test_export_cloudformation_stack(self, TemplateMock): language_extensions_enabled=True, parallel_upload=False, upload_executor=None, + upload_abort=None, ) template_instance_mock.export.assert_called_once_with() self.s3_uploader_mock.upload.assert_called_once_with(mock.ANY, mock.ANY) @@ -1419,6 +1421,7 @@ def test_export_serverless_application(self, TemplateMock): language_extensions_enabled=True, parallel_upload=False, upload_executor=None, + upload_abort=None, ) template_instance_mock.export.assert_called_once_with() self.s3_uploader_mock.upload.assert_called_once_with(mock.ANY, mock.ANY) @@ -1640,9 +1643,10 @@ def test_template_export_parallel_invokes_executor(self, yaml_parse_mock, run_jo template_exporter.export() run_jobs_mock.assert_called_once() - jobs, executor = run_jobs_mock.call_args[0] + jobs, executor, abort = run_jobs_mock.call_args[0] self.assertEqual([False, False], [is_nested_stack for is_nested_stack, _ in jobs]) self.assertIsInstance(executor, ThreadPoolExecutor) + self.assertIsInstance(abort, threading.Event) @patch("samcli.lib.package.artifact_exporter.is_experimental_enabled") @patch("samcli.lib.package.artifact_exporter.yaml_parse") @@ -1940,6 +1944,59 @@ def test_nested_stack_template_upload_goes_through_shared_upload_pool(self, yaml self.assertEqual(1, len(upload_threads)) self.assertTrue(upload_threads[0].startswith("upload")) + @patch("samcli.lib.package.artifact_exporter.LOG") + def test_failure_stops_nested_subtrees_from_starting_new_uploads(self, log_mock): + abort = threading.Event() + upload_failed = threading.Event() + child_upload = Mock() + + def failing_upload(): + try: + raise RuntimeError("bucket is gone") + finally: + upload_failed.set() + + def nested_stack(): + # A nested stack that only reaches its own uploads after the root upload has failed. + upload_failed.wait(5) + abort.wait(5) + Template._run_export_jobs([(False, child_upload)], executor, abort) + + with ThreadPoolExecutor(max_workers=2) as executor: + with self.assertRaises(RuntimeError): + Template._run_export_jobs([(False, failing_upload), (True, nested_stack)], executor, abort) + + child_upload.assert_not_called() + self.assertTrue(abort.is_set()) + logged = [log_call.args[1] for log_call in log_mock.error.call_args_list] + self.assertFalse(any(isinstance(error, _UploadAborted) for error in logged)) + + def test_wait_fail_fast_surfaces_real_failure_over_aborted_work(self): + aborted, real = Future(), Future() + aborted.set_exception(_UploadAborted()) + real.set_exception(ValueError("real failure")) + + with self.assertRaises(ValueError): + Template._wait_fail_fast([aborted, real], threading.Event()) + + def test_export_job_is_skipped_once_export_is_aborted(self): + exporter = Mock() + template_exporter = Template.__new__(Template) + template_exporter.uploaders = Mock() + template_exporter.code_signer = Mock() + template_exporter.parameter_values = None + template_exporter.language_extensions_enabled = False + template_exporter.template_dir = "dir" + template_exporter.parallel_upload = True + abort = threading.Event() + abort.set() + + job = template_exporter._build_export_job(Mock(return_value=exporter), "Leaf", {}, None, Mock(), abort) + + with self.assertRaises(_UploadAborted): + job() + exporter.export.assert_not_called() + def test_thread_safe_upload_cache_key_lock_is_per_key(self): cache = _ThreadSafeUploadCache() self.assertIs(cache.key_lock("a"), cache.key_lock("a")) From fb6e226fbdcd66bf81f7bdf4e5f0a19374789251 Mon Sep 17 00:00:00 2001 From: andy klier Date: Thu, 1 Oct 2026 07:45:12 -0600 Subject: [PATCH 13/14] fix: log additional parallel upload failures without tracebacks Each additional failure is logged as a single line; its traceback is only emitted at debug level. A missing bucket previously printed a full traceback for every concurrent upload that failed alongside the surfaced error. Co-Authored-By: Claude Opus 5.5 (1M context) --- samcli/lib/package/artifact_exporter.py | 5 ++++- tests/unit/lib/package/test_artifact_exporter.py | 10 ++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index c6c9457fc04..75bb4e21b47 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -835,7 +835,10 @@ def _log_other_failures(futures: List[Future], surfaced: Optional[Future] = None continue error = future.exception() if error is not None and not isinstance(error, _UploadAborted): - LOG.error("Parallel artifact upload also failed: %s", error, exc_info=error) + # One line per extra failure; the surfaced error is reported normally and the full + # traceback of the others is only shown with --debug. + LOG.error("Parallel artifact upload also failed: %s", error) + LOG.debug("Traceback for the parallel upload failure above", exc_info=error) def delete(self, retain_resources: List): """ diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 88221db0c10..6eada0af303 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -1997,6 +1997,16 @@ def test_export_job_is_skipped_once_export_is_aborted(self): job() exporter.export.assert_not_called() + @patch("samcli.lib.package.artifact_exporter.LOG") + def test_other_failures_log_one_line_with_traceback_only_at_debug(self, log_mock): + other = Future() + other.set_exception(RuntimeError("other failure")) + + Template._log_other_failures([other]) + + self.assertNotIn("exc_info", log_mock.error.call_args.kwargs) + self.assertIs(other.exception(), log_mock.debug.call_args.kwargs["exc_info"]) + def test_thread_safe_upload_cache_key_lock_is_per_key(self): cache = _ThreadSafeUploadCache() self.assertIs(cache.key_lock("a"), cache.key_lock("a")) From 2860bb432955c79d0f166a1a21102c2c9d291f7b Mon Sep 17 00:00:00 2001 From: andy klier Date: Thu, 1 Oct 2026 09:23:19 -0600 Subject: [PATCH 14/14] fix: recognize aborts wrapped by nested-stack exporters ResourceZip.export wraps do_export failures in ExportFailedError(ex=...), so a nested stack skipped after an abort reached its parent wrapped (once per nesting level) and was treated as a real failure: it could be surfaced instead of the actual error and was logged as an extra failure. Detect aborts through the wrapping chain when choosing the surfaced error and when logging the others. Co-Authored-By: Claude Opus 5.5 (1M context) --- samcli/lib/package/artifact_exporter.py | 19 ++++++++-- .../lib/package/test_artifact_exporter.py | 36 +++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index 75bb4e21b47..26335dfa6bb 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -98,6 +98,21 @@ class _UploadAborted(Exception): """Raised by parallel export work that was skipped because another upload already failed.""" +def _is_upload_abort(error: Optional[BaseException]) -> bool: + """ + True if ``error`` is, or wraps, an ``_UploadAborted``. Exporters wrap ``do_export`` failures in + ``ExportFailedError(ex=...)``, so a nested stack that was skipped reaches its parent wrapped, + once per nesting level. + """ + seen = set() + while error is not None and id(error) not in seen: + if isinstance(error, _UploadAborted): + return True + seen.add(id(error)) + error = getattr(error, "ex", None) or error.__cause__ + return False + + class _ThreadSafeUploadCache(MutableMapping[str, str]): """ Thread-safe mapping used to deduplicate uploads across threads. @@ -822,7 +837,7 @@ def _wait_fail_fast(futures: List[Future], abort: Optional[threading.Event] = No future.cancel() wait(futures) failed = [future for future in futures if not future.cancelled() and future.exception() is not None] - real = [future for future in failed if not isinstance(future.exception(), _UploadAborted)] + real = [future for future in failed if not _is_upload_abort(future.exception())] surfaced = (real or failed)[0] Template._log_other_failures(futures, surfaced=surfaced) raise cast(BaseException, surfaced.exception()) @@ -834,7 +849,7 @@ def _log_other_failures(futures: List[Future], surfaced: Optional[Future] = None if future is surfaced or future.cancelled(): continue error = future.exception() - if error is not None and not isinstance(error, _UploadAborted): + if error is not None and not _is_upload_abort(error): # One line per extra failure; the surfaced error is reported normally and the full # traceback of the others is only shown with --debug. LOG.error("Parallel artifact upload also failed: %s", error) diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 6eada0af303..e0265355985 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -40,6 +40,7 @@ _resolve_nested_stack_parameters, _ThreadSafeUploadCache, _UploadAborted, + _is_upload_abort, get_parallel_upload_workers, ) from samcli.lib.package.language_extensions_packaging import merge_language_extensions_s3_uris @@ -2007,6 +2008,41 @@ def test_other_failures_log_one_line_with_traceback_only_at_debug(self, log_mock self.assertNotIn("exc_info", log_mock.error.call_args.kwargs) self.assertIs(other.exception(), log_mock.debug.call_args.kwargs["exc_info"]) + @patch("samcli.lib.package.artifact_exporter.LOG") + def test_wait_fail_fast_ignores_aborts_wrapped_by_nested_stack_exporters(self, log_mock): + # Sibling nested stack A was skipped after B failed. ResourceZip.export wraps both in + # ExportFailedError, once per nesting level; A comes first in submission order. + aborted_a, failed_b = Future(), Future() + aborted_a.set_exception( + exceptions.ExportFailedError( + resource_id="Outer", + property_name="TemplateURL", + property_value="outer.yaml", + ex=exceptions.ExportFailedError( + resource_id="A", property_name="TemplateURL", property_value="a.yaml", ex=_UploadAborted() + ), + ) + ) + real_error = exceptions.ExportFailedError( + resource_id="B", property_name="TemplateURL", property_value="b.yaml", ex=RuntimeError("bucket is gone") + ) + failed_b.set_exception(real_error) + + with self.assertRaises(exceptions.ExportFailedError) as raised: + Template._wait_fail_fast([aborted_a, failed_b], threading.Event()) + + self.assertIs(real_error, raised.exception) + log_mock.error.assert_not_called() + + def test_is_upload_abort_follows_wrapping(self): + self.assertTrue(_is_upload_abort(_UploadAborted())) + wrapped = exceptions.ExportFailedError( + resource_id="A", property_name="TemplateURL", property_value="a.yaml", ex=_UploadAborted() + ) + self.assertTrue(_is_upload_abort(wrapped)) + self.assertFalse(_is_upload_abort(RuntimeError("real"))) + self.assertFalse(_is_upload_abort(None)) + def test_thread_safe_upload_cache_key_lock_is_per_key(self): cache = _ThreadSafeUploadCache() self.assertIs(cache.key_lock("a"), cache.key_lock("a"))