Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Adityaj0 wants to merge 5 commits into
base: develop
Choose a base branch
from
+256 −10
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5950ac4
fix: sam local invoke leaks Docker container and temp directory on OOM
Adityaj0 Aug 15, 2026
3f23ceb
fix: nest container-stop and temp-dir cleanup so each runs independently
Adityaj0 Aug 15, 2026
67676eb
fix(local): don't let cleanup failures replace the in-flight invoke e…
Adityaj0 Aug 15, 2026
41f7ea8
fix(local): fix cleanup leaks at their root cause instead of just log…
Adityaj0 Aug 15, 2026
c298d1b
fix(local): guard cleanup-list producer with the same lock consumer u…
Adityaj0 Aug 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions samcli/local/lambdafn/runtime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,10 +356,15 @@ 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:
try:
if container:
self._container_manager.stop(container)

@aws-sam-tooling-bot aws-sam-tooling-bot Bot Aug 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[ERROR_HANDLING] Cleanup now runs in finally blocks, so an exception from cleanup replaces the in-flight exception instead of propagating it. Concretely, on an OOM'd invoke where _check_exit_state() has raised ContainerFailureError, if self._container_manager.stop(container) or self._clean_decompressed_paths() then raises, the OOM error is discarded (kept only as __context__) and the cleanup error surfaces instead.

This is reachable with the existing code:

  • ContainerManager.stop() calls Container.stop() and Container.delete(), both of which re-raise docker.errors.APIError unless the message matches the "removal of container ... is already in progress" special case (samcli/local/docker/container.py). Container.delete() additionally does shutil.rmtree(self._host_tmp_dir).
  • _clean_decompressed_paths() calls shutil.rmtree() with no ignore_errors.

The user-visible impact is a regression in error reporting: ContainerFailureError is a UserException, so it produces the friendly "Container invocation failed due to maximum memory usage" message and exit code 1. A raw docker.errors.APIError / OSError is not, so the actual OOM cause is hidden behind an unhandled-exception trace. The PR's own test_on_invoke_done_cleans_paths_when_both_check_exit_state_and_stop_raise encodes this behavior by asserting RuntimeError propagates rather than ContainerFailureError.

Since these are best-effort cleanup steps whose failures should not determine the invoke result, log them instead of letting them escape:

def oninvoke_done(self, container):
   try:
       if container:
           self._check_exit_state(container)
   finally:
       if container:
           try:
               self._container_manager.stop(container)
           except Exception:  # best-effort cleanup
               LOG.warning("Failed to stop/remove container during cleanup", exc_info=True)
       try:
           self._clean_decompressed_paths()
       except Exception:  # best-effort cleanup
           LOG.warning("Failed to clean decompressed code directories", exc_info=True)

This still guarantees both cleanup steps run (the point of the fix) while preserving the original error. If you prefer cleanup failures to remain fatal when there is no in-flight exception, the alternative is to re-raise the original exception explicitly when one exists; either way, the OOM error should not be swallowed. The third test's assertion would need updating to match whichever behavior you choose.

finally:
self._clean_decompressed_paths()

@aws-sam-tooling-bot aws-sam-tooling-bot Bot Aug 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[RESOURCE_MANAGEMENT] Swallowing the exception from _clean_decompressed_paths() turns a one-off temp-dir removal failure into a permanent, silent leak, because that method is not restartable.

# samcli/local/lambdafn/runtime.py:480
def cleandecompressed_paths(self):
   LOG.debug("Cleaning all decompressed code dirs")
   with self._lock:
       for decompressed_dir in self._temp_uncompressed_paths_to_be_cleaned:
           shutil.rmtree(decompressed_dir)
       self._temp_uncompressed_paths_to_be_cleaned = []

If shutil.rmtree raises on any entry (Windows file locks on a directory that was bind-mounted into the container, or an OSError from a partially-removed tree), the loop aborts and self._temp_uncompressed_paths_to_be_cleaned = [] never runs. The failing path stays in the list forever, and every later invoke on the same LambdaRuntime instance — start-api/start-lambda reuse one instance for the life of the server — re-enters the loop, hits that same stale entry first (now typically FileNotFoundError if it was in fact deleted), and aborts again. Newer decompressed dirs appended after it are then never cleaned. Before this PR the error at least surfaced to the user; now it is a warning line that leaves a growing set of temp dirs behind.

Making the loop itself per-path resilient fixes the root cause and makes the outer except here redundant:

def cleandecompressed_paths(self):
   LOG.debug("Cleaning all decompressed code dirs")
   with self._lock:
       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 _check_exit_state(self, container: Container):
"""
Expand Down
54 changes: 54 additions & 0 deletions tests/unit/local/lambdafn/test_runtime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2014,6 +2014,60 @@ 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 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"""
Expand Down