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 5070b33694..ac03bb2db9 100644 --- a/samcli/local/lambdafn/runtime.py +++ b/samcli/local/lambdafn/runtime.py @@ -356,10 +356,22 @@ 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: + # 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) + 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): """ @@ -450,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) @@ -482,10 +499,27 @@ 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 = [] + 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/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 9fb6e2a043..df0d8893cb 100644 --- a/tests/unit/local/lambdafn/test_runtime.py +++ b/tests/unit/local/lambdafn/test_runtime.py @@ -2014,6 +2014,192 @@ 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() + + 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. Cleanup failures are + best-effort and must not propagate out of _on_invoke_done when there's no other error. + """ + 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() + + # 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() + + 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, 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 + + 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(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) + 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() + 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(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. 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"] + + 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")]) + + @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, []) + class TestWarmLambdaRuntime_create_container_branch(TestCase): """Test WarmLambdaRuntime.create method container branch - lines 470->473"""