From 5950ac4251690c73f16b7202614f7186e4aafa40 Mon Sep 17 00:00:00 2001 From: Aditya Jain Date: Fri, 14 Aug 2026 17:33:44 -0700 Subject: [PATCH 1/5] fix: sam local invoke leaks Docker container and temp directory on OOM _on_invoke_done() called _check_exit_state(container) before _container_manager.stop(container) and _clean_decompressed_paths(). When a function is OOM-killed, _check_exit_state() raises ContainerFailureError, which propagated out of _on_invoke_done() before either cleanup step ran, leaking the stopped-but-not-removed container and the per-invocation decompressed-code temp directory on every OOM'd invocation. Wrap the check in try/finally so cleanup always runs regardless of whether _check_exit_state() raises. Fixes #9182 --- samcli/local/lambdafn/runtime.py | 11 +++++++---- tests/unit/local/lambdafn/test_runtime.py | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/samcli/local/lambdafn/runtime.py b/samcli/local/lambdafn/runtime.py index 5070b33694..e3f2a27d27 100644 --- a/samcli/local/lambdafn/runtime.py +++ b/samcli/local/lambdafn/runtime.py @@ -356,10 +356,13 @@ def _on_invoke_done(self, container): container: Container The current running container """ - if container: - self._check_exit_state(container) - self._container_manager.stop(container) - self._clean_decompressed_paths() + try: + if container: + self._check_exit_state(container) + finally: + if container: + self._container_manager.stop(container) + self._clean_decompressed_paths() def _check_exit_state(self, container: Container): """ diff --git a/tests/unit/local/lambdafn/test_runtime.py b/tests/unit/local/lambdafn/test_runtime.py index 9fb6e2a043..a65e9272e5 100644 --- a/tests/unit/local/lambdafn/test_runtime.py +++ b/tests/unit/local/lambdafn/test_runtime.py @@ -2014,6 +2014,24 @@ def test_on_invoke_done_with_none_container_only_cleans_paths(self): # Verify cleanup was called self.runtime._clean_decompressed_paths.assert_called_once() + def test_on_invoke_done_stops_container_and_cleans_paths_even_when_check_exit_state_raises(self): + """Regression test: when the container was OOM-killed, _check_exit_state raises + ContainerFailureError. The container must still be stopped and the decompressed + code path must still be cleaned up, not skipped by the propagating exception. + """ + from samcli.local.docker.exceptions import ContainerFailureError + + container = Mock() + + self.runtime._check_exit_state = Mock(side_effect=ContainerFailureError("out of memory")) + self.runtime._clean_decompressed_paths = Mock() + + with self.assertRaises(ContainerFailureError): + self.runtime._on_invoke_done(container) + + self.manager_mock.stop.assert_called_once_with(container) + self.runtime._clean_decompressed_paths.assert_called_once() + class TestWarmLambdaRuntime_create_container_branch(TestCase): """Test WarmLambdaRuntime.create method container branch - lines 470->473""" From 3f23ceb0aea6e91738c29476e5586e4ac71ba726 Mon Sep 17 00:00:00 2001 From: Aditya Jain Date: Sat, 15 Aug 2026 16:00:37 -0700 Subject: [PATCH 2/5] fix: nest container-stop and temp-dir cleanup so each runs independently Reviewer noted that container_manager.stop() and clean_decompressed_paths() were both in the same finally block, so a docker.errors.APIError from Container.stop()/delete() (raised for any Docker API error other than the "removal already in progress" special case) would still skip _clean_decompressed_paths(), reproducing the same leak class this PR fixes for the OOM path. Nest the two cleanup steps in their own try/finally so a failure in one cannot suppress the other, and the original exception (e.g. ContainerFailureError) keeps propagating. Co-Authored-By: Claude Sonnet 5 --- samcli/local/lambdafn/runtime.py | 8 +++-- tests/unit/local/lambdafn/test_runtime.py | 36 +++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/samcli/local/lambdafn/runtime.py b/samcli/local/lambdafn/runtime.py index e3f2a27d27..44447df02e 100644 --- a/samcli/local/lambdafn/runtime.py +++ b/samcli/local/lambdafn/runtime.py @@ -360,9 +360,11 @@ def _on_invoke_done(self, container): if container: self._check_exit_state(container) finally: - if container: - self._container_manager.stop(container) - self._clean_decompressed_paths() + try: + if container: + self._container_manager.stop(container) + finally: + self._clean_decompressed_paths() def _check_exit_state(self, container: Container): """ diff --git a/tests/unit/local/lambdafn/test_runtime.py b/tests/unit/local/lambdafn/test_runtime.py index a65e9272e5..ea19ef81ec 100644 --- a/tests/unit/local/lambdafn/test_runtime.py +++ b/tests/unit/local/lambdafn/test_runtime.py @@ -2032,6 +2032,42 @@ def test_on_invoke_done_stops_container_and_cleans_paths_even_when_check_exit_st self.manager_mock.stop.assert_called_once_with(container) self.runtime._clean_decompressed_paths.assert_called_once() + def test_on_invoke_done_cleans_paths_even_when_container_manager_stop_raises(self): + """Regression test: if _container_manager.stop() itself raises (e.g. docker.errors.APIError + from Container.stop()/delete() for a reason other than "removal already in progress"), + _clean_decompressed_paths() must still run and not be skipped by the propagating exception. + """ + container = Mock() + + self.runtime._check_exit_state = Mock() + self.manager_mock.stop = Mock(side_effect=RuntimeError("docker API error")) + self.runtime._clean_decompressed_paths = Mock() + + with self.assertRaises(RuntimeError): + self.runtime._on_invoke_done(container) + + self.manager_mock.stop.assert_called_once_with(container) + self.runtime._clean_decompressed_paths.assert_called_once() + + def test_on_invoke_done_cleans_paths_when_both_check_exit_state_and_stop_raise(self): + """Regression test: when the container is OOM-killed (_check_exit_state raises + ContainerFailureError) AND the subsequent stop() also raises (e.g. a Docker API error), + _clean_decompressed_paths() must still run. + """ + from samcli.local.docker.exceptions import ContainerFailureError + + container = Mock() + + self.runtime._check_exit_state = Mock(side_effect=ContainerFailureError("out of memory")) + self.manager_mock.stop = Mock(side_effect=RuntimeError("docker API error")) + self.runtime._clean_decompressed_paths = Mock() + + with self.assertRaises(RuntimeError): + self.runtime._on_invoke_done(container) + + self.manager_mock.stop.assert_called_once_with(container) + self.runtime._clean_decompressed_paths.assert_called_once() + class TestWarmLambdaRuntime_create_container_branch(TestCase): """Test WarmLambdaRuntime.create method container branch - lines 470->473""" From 67676eb8d3ada6122ed083858e87ae8c92f30a5b Mon Sep 17 00:00:00 2001 From: Aditya Jain Date: Sat, 15 Aug 2026 16:08:02 -0700 Subject: [PATCH 3/5] fix(local): don't let cleanup failures replace the in-flight invoke error The previous nested try/finally made both cleanup steps independent of each other, but a failure in either one still propagated and replaced whatever exception was already in flight from _check_exit_state() -- e.g. an OOM'd invoke's ContainerFailureError would be discarded in favor of a raw docker.errors.APIError from a failed stop(), turning a friendly "Container invocation failed due to maximum memory usage" message into an opaque unhandled-exception trace. Cleanup failures are best-effort and shouldn't determine the invoke result: log and swallow them instead, so the original error (if any) is what actually propagates, while both cleanup steps still always run. --- samcli/local/lambdafn/runtime.py | 13 +++++-- tests/unit/local/lambdafn/test_runtime.py | 47 ++++++++++++++++++++--- 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/samcli/local/lambdafn/runtime.py b/samcli/local/lambdafn/runtime.py index 44447df02e..23305a1b59 100644 --- a/samcli/local/lambdafn/runtime.py +++ b/samcli/local/lambdafn/runtime.py @@ -360,11 +360,18 @@ def _on_invoke_done(self, container): if container: self._check_exit_state(container) finally: - try: - if container: + # Best-effort cleanup: a failure here should not replace an in-flight exception + # from _check_exit_state() above (e.g. ContainerFailureError on OOM) with a raw + # Docker/OS error, and each step must run independently of the other's success. + if container: + try: self._container_manager.stop(container) - finally: + except Exception: + LOG.warning("Failed to stop/remove container during cleanup", exc_info=True) + try: self._clean_decompressed_paths() + except Exception: + LOG.warning("Failed to clean decompressed code directories during cleanup", exc_info=True) def _check_exit_state(self, container: Container): """ diff --git a/tests/unit/local/lambdafn/test_runtime.py b/tests/unit/local/lambdafn/test_runtime.py index ea19ef81ec..e9bfa87ee1 100644 --- a/tests/unit/local/lambdafn/test_runtime.py +++ b/tests/unit/local/lambdafn/test_runtime.py @@ -2035,7 +2035,8 @@ def test_on_invoke_done_stops_container_and_cleans_paths_even_when_check_exit_st def test_on_invoke_done_cleans_paths_even_when_container_manager_stop_raises(self): """Regression test: if _container_manager.stop() itself raises (e.g. docker.errors.APIError from Container.stop()/delete() for a reason other than "removal already in progress"), - _clean_decompressed_paths() must still run and not be skipped by the propagating exception. + _clean_decompressed_paths() must still run and not be skipped. Cleanup failures are + best-effort and must not propagate out of _on_invoke_done when there's no other error. """ container = Mock() @@ -2043,8 +2044,8 @@ def test_on_invoke_done_cleans_paths_even_when_container_manager_stop_raises(sel self.manager_mock.stop = Mock(side_effect=RuntimeError("docker API error")) self.runtime._clean_decompressed_paths = Mock() - with self.assertRaises(RuntimeError): - self.runtime._on_invoke_done(container) + # Should not raise: stop()'s failure is logged, not propagated. + self.runtime._on_invoke_done(container) self.manager_mock.stop.assert_called_once_with(container) self.runtime._clean_decompressed_paths.assert_called_once() @@ -2052,7 +2053,9 @@ def test_on_invoke_done_cleans_paths_even_when_container_manager_stop_raises(sel def test_on_invoke_done_cleans_paths_when_both_check_exit_state_and_stop_raise(self): """Regression test: when the container is OOM-killed (_check_exit_state raises ContainerFailureError) AND the subsequent stop() also raises (e.g. a Docker API error), - _clean_decompressed_paths() must still run. + _clean_decompressed_paths() must still run, and the original ContainerFailureError must + be what propagates -- not stop()'s cleanup-only error, which would otherwise replace the + user-facing OOM message with an opaque Docker API error. """ from samcli.local.docker.exceptions import ContainerFailureError @@ -2062,7 +2065,41 @@ def test_on_invoke_done_cleans_paths_when_both_check_exit_state_and_stop_raise(s self.manager_mock.stop = Mock(side_effect=RuntimeError("docker API error")) self.runtime._clean_decompressed_paths = Mock() - with self.assertRaises(RuntimeError): + with self.assertRaises(ContainerFailureError): + self.runtime._on_invoke_done(container) + + self.manager_mock.stop.assert_called_once_with(container) + self.runtime._clean_decompressed_paths.assert_called_once() + + def test_on_invoke_done_stop_still_runs_when_clean_decompressed_paths_raises(self): + """Regression test: if _clean_decompressed_paths() itself raises (e.g. shutil.rmtree + OSError), that failure is best-effort and must not propagate, and must not prevent + stop() from having already run. + """ + container = Mock() + + self.runtime._check_exit_state = Mock() + self.runtime._clean_decompressed_paths = Mock(side_effect=OSError("could not remove temp dir")) + + # Should not raise: _clean_decompressed_paths()'s failure is logged, not propagated. + self.runtime._on_invoke_done(container) + + self.manager_mock.stop.assert_called_once_with(container) + self.runtime._clean_decompressed_paths.assert_called_once() + + def test_on_invoke_done_original_error_propagates_when_clean_decompressed_paths_also_raises(self): + """Regression test: when _check_exit_state raises ContainerFailureError AND + _clean_decompressed_paths() also raises, the original ContainerFailureError must be + what propagates, not the cleanup-only error. + """ + from samcli.local.docker.exceptions import ContainerFailureError + + container = Mock() + + self.runtime._check_exit_state = Mock(side_effect=ContainerFailureError("out of memory")) + self.runtime._clean_decompressed_paths = Mock(side_effect=OSError("could not remove temp dir")) + + with self.assertRaises(ContainerFailureError): self.runtime._on_invoke_done(container) self.manager_mock.stop.assert_called_once_with(container) From 41f7ea8161dfff2784de1ad7848afb5c7f9d4997 Mon Sep 17 00:00:00 2001 From: Aditya Jain Date: Sat, 15 Aug 2026 16:13:58 -0700 Subject: [PATCH 4/5] fix(local): fix cleanup leaks at their root cause instead of just logging them Two deeper issues in the cleanup path the previous commit only papered over by logging and swallowing failures: 1. ContainerManager.stop() called container.stop() then container.delete() sequentially. If stop() raised (e.g. docker.errors.APIError), delete() -- the call that actually removes the container -- never ran, so catching that exception at the call site just logged a warning while the container stayed orphaned. Wrap stop() in try/finally so delete() always runs regardless of stop()'s outcome. 2. LambdaRuntime._clean_decompressed_paths() aborted its loop on the first shutil.rmtree() failure, and only cleared self._temp_uncompressed_paths_to_be_cleaned after the loop finished. Since that list is append-only, a single failing entry got stuck in it forever: every later invoke would re-hit the same failing path first and abort again, permanently blocking cleanup of every temp dir added afterwards for the life of the LambdaRuntime instance (start-api/start-lambda reuse one instance for the server's lifetime). Snapshot-and-clear the list under the lock up front, then clean each path independently so one failure can't block the rest or get stuck. --- samcli/local/docker/manager.py | 11 ++++-- samcli/local/lambdafn/runtime.py | 13 +++++-- tests/unit/local/docker/test_manager.py | 21 ++++++++++++ tests/unit/local/lambdafn/test_runtime.py | 41 +++++++++++++++++++++++ 4 files changed, 81 insertions(+), 5 deletions(-) diff --git a/samcli/local/docker/manager.py b/samcli/local/docker/manager.py index 6c431b94a6..6797ee9269 100644 --- a/samcli/local/docker/manager.py +++ b/samcli/local/docker/manager.py @@ -114,9 +114,14 @@ def stop(self, container: Container) -> None: :param samcli.local.docker.container.Container container: Container to stop """ - if self.do_shutdown_event: - container.stop() - container.delete() + # container.delete() is what actually removes the container; it must run even if + # container.stop() raises (e.g. a docker.errors.APIError), otherwise a failed stop() + # leaves the container running/orphaned with no further cleanup attempt. + try: + if self.do_shutdown_event: + container.stop() + finally: + container.delete() def pull_image(self, image_name, tag=None, stream=None): """ diff --git a/samcli/local/lambdafn/runtime.py b/samcli/local/lambdafn/runtime.py index 23305a1b59..b297975ce6 100644 --- a/samcli/local/lambdafn/runtime.py +++ b/samcli/local/lambdafn/runtime.py @@ -494,10 +494,19 @@ def _clean_decompressed_paths(self): Clean the temporary decompressed code dirs """ LOG.debug("Cleaning all decompressed code dirs") + # Snapshot and clear the list up front, under the lock, so that a failure removing one + # directory can't abort the loop and leave every entry (including ones already removed + # or added after this call started) stuck in the list forever -- since this list is only + # ever appended to, a stuck entry would otherwise block cleanup of every directory added + # in subsequent invokes for the lifetime of this LambdaRuntime instance. with self._lock: - for decompressed_dir in self._temp_uncompressed_paths_to_be_cleaned: - shutil.rmtree(decompressed_dir) + paths_to_clean = self._temp_uncompressed_paths_to_be_cleaned self._temp_uncompressed_paths_to_be_cleaned = [] + for decompressed_dir in paths_to_clean: + try: + shutil.rmtree(decompressed_dir) + except OSError: + LOG.warning("Failed to remove temporary directory %s", decompressed_dir, exc_info=True) def get_or_create_emulator_container(self): """ diff --git a/tests/unit/local/docker/test_manager.py b/tests/unit/local/docker/test_manager.py index 1784b1020d..9a00d3abd9 100644 --- a/tests/unit/local/docker/test_manager.py +++ b/tests/unit/local/docker/test_manager.py @@ -441,6 +441,27 @@ def test_must_call_delete_on_container(self, mock_create_client): manager.stop(container) container.delete.assert_called_with() + @patch("samcli.local.docker.container_client_factory.ContainerClientFactory.create_client") + def test_must_call_delete_even_when_container_stop_raises(self, mock_create_client): + """Regression test: container.delete() is what actually removes the container, and must + still run even if container.stop() raises (e.g. a docker.errors.APIError) -- otherwise a + failed stop() leaves the container running/orphaned with no further cleanup attempt. + """ + with patch( + "samcli.local.docker.container_client_factory.ContainerClientFactory.get_admin_container_preference", + return_value=None, + ): + manager = ContainerManager(do_shutdown_event=True) + container = Mock() + container.stop = Mock(side_effect=RuntimeError("docker API error")) + container.delete = Mock() + + with self.assertRaises(RuntimeError): + manager.stop(container) + + container.stop.assert_called_once() + container.delete.assert_called_once() + class TestContainerManager_inspect(TestCase): @patch("samcli.local.docker.container_client_factory.ContainerClientFactory.create_client") diff --git a/tests/unit/local/lambdafn/test_runtime.py b/tests/unit/local/lambdafn/test_runtime.py index e9bfa87ee1..b143dbc207 100644 --- a/tests/unit/local/lambdafn/test_runtime.py +++ b/tests/unit/local/lambdafn/test_runtime.py @@ -2106,6 +2106,47 @@ def test_on_invoke_done_original_error_propagates_when_clean_decompressed_paths_ self.runtime._clean_decompressed_paths.assert_called_once() +class TestLambdaRuntime_clean_decompressed_paths(TestCase): + def setUp(self): + self.manager_mock = Mock() + self.lambda_image_mock = Mock() + self.runtime = LambdaRuntime(self.manager_mock, self.lambda_image_mock) + + @patch("samcli.local.lambdafn.runtime.shutil") + def test_all_paths_cleaned_and_list_cleared_on_success(self, shutil_mock): + self.runtime._temp_uncompressed_paths_to_be_cleaned = ["path1", "path2"] + + self.runtime._clean_decompressed_paths() + + self.assertEqual(shutil_mock.rmtree.call_args_list, [call("path1"), call("path2")]) + self.assertEqual(self.runtime._temp_uncompressed_paths_to_be_cleaned, []) + + @patch("samcli.local.lambdafn.runtime.shutil") + def test_failure_removing_one_path_does_not_block_the_others_or_leave_the_list_stuck(self, shutil_mock): + """Regression test: previously, if shutil.rmtree() raised for one directory, the loop + aborted immediately and self._temp_uncompressed_paths_to_be_cleaned was never reset + (the reset only ran after the loop finished). Since this list is append-only, every + entry -- including ones successfully removed before the failure, and any added by later + invokes -- would be stuck in it forever, and every subsequent call would re-hit the same + first failing path and abort again, permanently leaking all newer temp dirs. + """ + self.runtime._temp_uncompressed_paths_to_be_cleaned = ["path1", "bad_path", "path3"] + + def rmtree_side_effect(path): + if path == "bad_path": + raise OSError("boom") + + shutil_mock.rmtree = Mock(side_effect=rmtree_side_effect) + + # Should not raise, and must still attempt every path. + self.runtime._clean_decompressed_paths() + + self.assertEqual(shutil_mock.rmtree.call_args_list, [call("path1"), call("bad_path"), call("path3")]) + # The list must be cleared regardless of the failure, so a later invoke's new temp dirs + # aren't queued up behind a permanently-stuck failing entry. + self.assertEqual(self.runtime._temp_uncompressed_paths_to_be_cleaned, []) + + class TestWarmLambdaRuntime_create_container_branch(TestCase): """Test WarmLambdaRuntime.create method container branch - lines 470->473""" From c298d1b3d65721ee715d31fcd8e8122a2c3fd38d Mon Sep 17 00:00:00 2001 From: Aditya Jain Date: Sat, 15 Aug 2026 16:29:29 -0700 Subject: [PATCH 5/5] fix(local): guard cleanup-list producer with the same lock consumer uses; retry failed removals Two further gaps in the cleanup path: 1. _get_code_dir() appended to self._temp_uncompressed_paths_to_be_cleaned with `+=` outside self._lock, while _clean_decompressed_paths() snapshots and clears that same list under the lock. start-api/start-lambda run threaded, so a request thread's append could race with a concurrent cleanup's snapshot: the new entry could be silently dropped, or worse, removed by rmtree while the request that just unzipped it is still about to use it. Hold the lock on the producer side too. 2. A path whose rmtree() failed was being dropped from the cleanup list permanently, leaking silently for the rest of the process. Since failures at this call site (a directory just unmounted from a just-stopped container) are commonly transient, requeue failed paths instead of discarding them, so they get retried on the next cleanup pass. --- samcli/local/lambdafn/runtime.py | 15 +++++- tests/unit/local/lambdafn/test_runtime.py | 62 +++++++++++++++++++++-- 2 files changed, 72 insertions(+), 5 deletions(-) diff --git a/samcli/local/lambdafn/runtime.py b/samcli/local/lambdafn/runtime.py index b297975ce6..ac03bb2db9 100644 --- a/samcli/local/lambdafn/runtime.py +++ b/samcli/local/lambdafn/runtime.py @@ -462,7 +462,12 @@ def _get_code_dir(self, code_path: str) -> str: if code_path and os.path.isfile(code_path) and code_path.endswith(self.SUPPORTED_ARCHIVE_EXTENSIONS): decompressed_dir: str = _unzip_file(code_path, mount_symlinks=self._mount_symlinks) - self._temp_uncompressed_paths_to_be_cleaned += [decompressed_dir] + # Must hold the same lock _clean_decompressed_paths() uses to swap this list out -- + # `start-api`/`start-lambda` run threaded, so a request thread appending here can + # race with a concurrent cleanup's snapshot-and-clear, either losing this entry + # entirely or (worse) having it removed by rmtree while still in use. + with self._lock: + self._temp_uncompressed_paths_to_be_cleaned.append(decompressed_dir) return decompressed_dir LOG.debug("Code %s is not a zip/jar file", code_path) @@ -502,11 +507,19 @@ def _clean_decompressed_paths(self): with self._lock: paths_to_clean = self._temp_uncompressed_paths_to_be_cleaned self._temp_uncompressed_paths_to_be_cleaned = [] + failed_paths = [] for decompressed_dir in paths_to_clean: try: shutil.rmtree(decompressed_dir) except OSError: LOG.warning("Failed to remove temporary directory %s", decompressed_dir, exc_info=True) + failed_paths.append(decompressed_dir) + if failed_paths: + # Removal failures at this call site are commonly transient (e.g. a directory that + # was just bind-mounted into a just-stopped container isn't released yet), so retry + # them on the next cleanup pass instead of dropping them permanently. + with self._lock: + self._temp_uncompressed_paths_to_be_cleaned += failed_paths def get_or_create_emulator_container(self): """ diff --git a/tests/unit/local/lambdafn/test_runtime.py b/tests/unit/local/lambdafn/test_runtime.py index b143dbc207..df0d8893cb 100644 --- a/tests/unit/local/lambdafn/test_runtime.py +++ b/tests/unit/local/lambdafn/test_runtime.py @@ -2106,6 +2106,35 @@ def test_on_invoke_done_original_error_propagates_when_clean_decompressed_paths_ self.runtime._clean_decompressed_paths.assert_called_once() +class TestLambdaRuntime_get_code_dir_locking(TestCase): + def setUp(self): + self.manager_mock = Mock() + self.lambda_image_mock = Mock() + self.runtime = LambdaRuntime(self.manager_mock, self.lambda_image_mock) + + @patch("samcli.local.lambdafn.runtime._unzip_file") + @patch("samcli.local.lambdafn.runtime.os.path.isfile", return_value=True) + @patch("samcli.local.lambdafn.runtime.os.path.exists", return_value=True) + def test_appending_to_cleanup_list_holds_the_same_lock_cleanup_uses(self, exists_mock, isfile_mock, unzip_mock): + """Regression test: _clean_decompressed_paths() snapshots and clears + self._temp_uncompressed_paths_to_be_cleaned under self._lock so a concurrent cleanup + can't observe a torn/partial list. That's only meaningful if every producer of that list + holds the same lock while mutating it -- start-api/start-lambda run threaded, and a + request thread appending here without the lock could race with a concurrent cleanup's + snapshot: the entry could be silently lost, or (worse) removed by rmtree while the + request that just unzipped it is still about to use it. + """ + unzip_mock.return_value = "/tmp/decompressed" + self.runtime._lock = MagicMock() + + result = self.runtime._get_code_dir("code.zip") + + self.assertEqual(result, "/tmp/decompressed") + self.runtime._lock.__enter__.assert_called_once() + self.runtime._lock.__exit__.assert_called_once() + self.assertEqual(self.runtime._temp_uncompressed_paths_to_be_cleaned, ["/tmp/decompressed"]) + + class TestLambdaRuntime_clean_decompressed_paths(TestCase): def setUp(self): self.manager_mock = Mock() @@ -2122,13 +2151,14 @@ def test_all_paths_cleaned_and_list_cleared_on_success(self, shutil_mock): self.assertEqual(self.runtime._temp_uncompressed_paths_to_be_cleaned, []) @patch("samcli.local.lambdafn.runtime.shutil") - def test_failure_removing_one_path_does_not_block_the_others_or_leave_the_list_stuck(self, shutil_mock): + def test_failure_removing_one_path_does_not_block_the_others(self, shutil_mock): """Regression test: previously, if shutil.rmtree() raised for one directory, the loop aborted immediately and self._temp_uncompressed_paths_to_be_cleaned was never reset (the reset only ran after the loop finished). Since this list is append-only, every entry -- including ones successfully removed before the failure, and any added by later invokes -- would be stuck in it forever, and every subsequent call would re-hit the same - first failing path and abort again, permanently leaking all newer temp dirs. + first failing path and abort again, permanently leaking all newer temp dirs. A single + failure must not prevent the other paths in the same batch from being attempted. """ self.runtime._temp_uncompressed_paths_to_be_cleaned = ["path1", "bad_path", "path3"] @@ -2142,8 +2172,32 @@ def rmtree_side_effect(path): self.runtime._clean_decompressed_paths() self.assertEqual(shutil_mock.rmtree.call_args_list, [call("path1"), call("bad_path"), call("path3")]) - # The list must be cleared regardless of the failure, so a later invoke's new temp dirs - # aren't queued up behind a permanently-stuck failing entry. + + @patch("samcli.local.lambdafn.runtime.shutil") + def test_failed_path_is_requeued_for_retry_not_dropped_permanently(self, shutil_mock): + """Regression test: rmtree() failures at this call site are commonly transient (e.g. a + directory just bind-mounted into a just-stopped container isn't released yet), so a + failed path must be requeued for the next cleanup pass rather than dropped permanently + with only a log warning -- otherwise every one-off failure in a long-running + `start-api`/`start-lambda` process accumulates into a silent, permanent leak. + """ + self.runtime._temp_uncompressed_paths_to_be_cleaned = ["path1", "bad_path", "path3"] + + def rmtree_side_effect(path): + if path == "bad_path": + raise OSError("boom") + + shutil_mock.rmtree = Mock(side_effect=rmtree_side_effect) + + self.runtime._clean_decompressed_paths() + + # Only the failed path remains queued; successfully-removed paths are gone. + self.assertEqual(self.runtime._temp_uncompressed_paths_to_be_cleaned, ["bad_path"]) + + # And it's actually retried (and this time succeeds) on the next cleanup pass. + shutil_mock.rmtree = Mock() + self.runtime._clean_decompressed_paths() + shutil_mock.rmtree.assert_called_once_with("bad_path") self.assertEqual(self.runtime._temp_uncompressed_paths_to_be_cleaned, [])