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
Open

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

Show file tree
Hide file tree
Changes from 3 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
20 changes: 16 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,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)

@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.

@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] self._container_manager.stop(container) can itself skip container removal, so catching and logging its failure here does not guarantee the container is gone — the leak this PR is fixing can still occur:

# samcli/local/docker/manager.py:111
def stop(self, container: Container) -> None:
   if self.do_shutdown_event:
       container.stop()
   container.delete()

Container.stop() (container.py:363) re-raises docker.errors.APIError for anything other than the "removal of container ... is already in progress" message, and that propagates out of ContainerManager.stop() before container.delete() — the call that actually removes the container — ever runs. The new except Exception then logs a warning and moves on, so the stopped-but-not-removed container survives exactly as it did before this fix. This path is reached whenever --shutdown is used (do_shutdown_event is set from that flag in invoke_context.py:888), which is the same iterative local-testing workflow the PR targets.

Guaranteeing removal requires delete() to run regardless of stop()'s outcome:

def stop(self, container: Container) -> None:
   try:
       if self.do_shutdown_event:
           container.stop()
   finally:
       container.delete()

except Exception:
LOG.warning("Failed to stop/remove container during cleanup", exc_info=True)
try:
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)

except Exception:
LOG.warning("Failed to clean decompressed code directories during cleanup", exc_info=True)

def _check_exit_state(self, container: Container):
"""
Expand Down
91 changes: 91 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,97 @@ 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 TestWarmLambdaRuntime_create_container_branch(TestCase):
"""Test WarmLambdaRuntime.create method container branch - lines 470->473"""
Expand Down