fix: sam local invoke leaks Docker container and temp directory on OOM

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):
         """

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):
         """

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")

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"""