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 4 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
11 changes: 8 additions & 3 deletions samcli/local/docker/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
Expand Down
33 changes: 27 additions & 6 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 Expand Up @@ -482,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

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

[CONCURRENCY] The snapshot taken under the lock does not actually isolate paths_to_clean from concurrent producers, because _get_code_dir() mutates the same list object without holding self._lock:

# samcli/local/lambdafn/runtime.py (_get_code_dir, unchanged by this PR)
self._temp_uncompressed_paths_to_be_cleaned += [decompressed_dir]

+= on a list is a read / in-place-extend / store-back sequence. If it interleaves with the new swap, the following happens:

  1. Producer thread loads the attribute → old list L1.
  2. Cleanup thread takes the lock, sets paths_to_clean = L1, rebinds the attribute to [], releases the lock.
  3. Producer extends L1 with its freshly unzipped dir and stores the attribute back to L1.
  4. Cleanup thread, now iterating outside the lock, rmtrees that dir while the producer's invoke is still about to mount it — and the attribute is now back to L1, holding paths that were already removed.

This is reachable in practice: sam local start-api / start-lambda run Flask with threaded=True when not debugging (samcli/local/services/base_local_service.py:79), and in non-warm mode every request thread goes through _get_code_dir() and then _on_invoke_done() → _clean_decompressed_paths() on the same LambdaRuntime instance. Moving rmtree out of the lock widens the window and the comment above the swap asserts a safety property the code does not have.

Guarding the producer side makes the snapshot meaningful:

# in getcode_dir()
with self._lock:
   self._temp_uncompressed_paths_to_be_cleaned.append(decompressed_dir)

self._temp_uncompressed_paths_to_be_cleaned = []
for decompressed_dir in paths_to_clean:
try:
shutil.rmtree(decompressed_dir)
except OSError:

@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] A path whose rmtree fails is now dropped from _temp_uncompressed_paths_to_be_cleaned permanently (the list is cleared up front), so that directory is never retried by a later invoke or by WarmLambdaRuntime.clean_running_containers_and_related_resources() — it leaks silently for the rest of the process, with only a warning log.

This matters most for the transient failures that dominate this call site: _clean_decompressed_paths() runs immediately after the container is stopped/removed, and removing a directory that was just bind-mounted into a container commonly fails once with PermissionError on Windows and succeeds moments later. For a long-running sam local start-api in non-warm mode, every such one-off failure accumulates a permanently orphaned temp dir — the same accumulating-leak class of bug this PR set out to fix.

Re-queueing only the failed paths keeps the "one bad entry can't block the others" property from this change while preserving a retry:

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:
   with self._lock:
       self._temp_uncompressed_paths_to_be_cleaned += failed_paths

Note on prior review threads: the two cleanup steps now running independently, the in-flight exception no longer being replaced by a cleanup error, and ContainerManager.stop() skipping container.delete() after a failed container.stop() all appear addressed by this revision. The decision to log rather than re-raise cleanup failures in _on_invoke_done() was explicitly stated as intentional, so it is not re-raised here.

LOG.warning("Failed to remove temporary directory %s", decompressed_dir, exc_info=True)

def get_or_create_emulator_container(self):
"""
Expand Down
21 changes: 21 additions & 0 deletions tests/unit/local/docker/test_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
132 changes: 132 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,138 @@ 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_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"""
Expand Down