fix: sam local invoke leaks Docker container and temp directory on OOM - #9184
fix: sam local invoke leaks Docker container and temp directory on OOM#9184Adityaj0 wants to merge 5 commits into
Conversation
_on_invoke_done() called _check_exit_state(container) before _container_manager.stop(container) and _clean_decompressed_paths(). When a function is OOM-killed, _check_exit_state() raises ContainerFailureError, which propagated out of _on_invoke_done() before either cleanup step ran, leaking the stopped-but-not-removed container and the per-invocation decompressed-code temp directory on every OOM'd invocation. Wrap the check in try/finally so cleanup always runs regardless of whether _check_exit_state() raises. Fixes aws#9182
github-actions
Bot
added
area/local/start-api
![aws-sam-tooling-bot[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
| self._check_exit_state(container) | ||
| finally: | ||
| if container: | ||
| self._container_manager.stop(container) |
There was a problem hiding this comment.
[RESOURCE_MANAGEMENT] The two cleanup steps run sequentially in the same finally block, so a failure in the first still skips the second — the same class of leak this PR fixes.
ContainerManager.stop() calls Container.stop() and Container.delete(), and both re-raise docker.errors.APIError unless the message matches the "removal of container ... is already in progress" special case (see samcli/local/docker/container.py). If that raises, self._clean_decompressed_paths() never runs, leaving the unzipped-code temp directory behind. It also replaces the in-flight ContainerFailureError, so an OOM'd invocation would surface an opaque Docker API error instead of "Container invocation failed due to maximum memory usage".
Nesting the cleanup makes both steps independent and preserves the original exception:
try:
if container:
self._check_exit_state(container)
finally:
try:
if container:
self._container_manager.stop(container)
finally:
self._clean_decompressed_paths()
Reviewer noted that container_manager.stop() and clean_decompressed_paths() were both in the same finally block, so a docker.errors.APIError from Container.stop()/delete() (raised for any Docker API error other than the "removal already in progress" special case) would still skip _clean_decompressed_paths(), reproducing the same leak class this PR fixes for the OOM path. Nest the two cleanup steps in their own try/finally so a failure in one cannot suppress the other, and the original exception (e.g. ContainerFailureError) keeps propagating. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Verified this independently and it's correct. Traced Pushed Added two regression tests in
Verified: |
| finally: | ||
| try: | ||
| if container: | ||
| self._container_manager.stop(container) |
There was a problem hiding this comment.
[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()callsContainer.stop()andContainer.delete(), both of which re-raisedocker.errors.APIErrorunless the message matches the "removal of container ... is already in progress" special case (samcli/local/docker/container.py).Container.delete()additionally doesshutil.rmtree(self._host_tmp_dir)._clean_decompressed_paths()callsshutil.rmtree()with noignore_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.
…rror The previous nested try/finally made both cleanup steps independent of each other, but a failure in either one still propagated and replaced whatever exception was already in flight from _check_exit_state() -- e.g. an OOM'd invoke's ContainerFailureError would be discarded in favor of a raw docker.errors.APIError from a failed stop(), turning a friendly "Container invocation failed due to maximum memory usage" message into an opaque unhandled-exception trace. Cleanup failures are best-effort and shouldn't determine the invoke result: log and swallow them instead, so the original error (if any) is what actually propagates, while both cleanup steps still always run.
|
Good catch — you're right that nesting the try/finally only fixed the "both steps always run" half of this; it didn't stop a cleanup failure from replacing whatever exception was already in flight. Pushed a fix (67676eb) that makes both cleanup steps best-effort:
Updated the existing |
| except Exception: | ||
| LOG.warning("Failed to stop/remove container during cleanup", exc_info=True) | ||
| try: | ||
| self._clean_decompressed_paths() |
There was a problem hiding this comment.
[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)| # Docker/OS error, and each step must run independently of the other's success. | ||
| if container: | ||
| try: | ||
| self._container_manager.stop(container) |
There was a problem hiding this comment.
[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()…ging them Two deeper issues in the cleanup path the previous commit only papered over by logging and swallowing failures: 1. ContainerManager.stop() called container.stop() then container.delete() sequentially. If stop() raised (e.g. docker.errors.APIError), delete() -- the call that actually removes the container -- never ran, so catching that exception at the call site just logged a warning while the container stayed orphaned. Wrap stop() in try/finally so delete() always runs regardless of stop()'s outcome. 2. LambdaRuntime._clean_decompressed_paths() aborted its loop on the first shutil.rmtree() failure, and only cleared self._temp_uncompressed_paths_to_be_cleaned after the loop finished. Since that list is append-only, a single failing entry got stuck in it forever: every later invoke would re-hit the same failing path first and abort again, permanently blocking cleanup of every temp dir added afterwards for the life of the LambdaRuntime instance (start-api/start-lambda reuse one instance for the server's lifetime). Snapshot-and-clear the list under the lock up front, then clean each path independently so one failure can't block the rest or get stuck.
|
Both confirmed real, and you're right that catching-and-logging at the call site wasn't enough — the leaks were still happening one level down, just silently now instead of loudly. Fixed both at the source (41f7ea8):
Full |
| 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 |
There was a problem hiding this comment.
[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:
- Producer thread loads the attribute → old list
L1. - Cleanup thread takes the lock, sets
paths_to_clean = L1, rebinds the attribute to[], releases the lock. - Producer extends
L1with its freshly unzipped dir and stores the attribute back toL1. - 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 toL1, 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)| for decompressed_dir in paths_to_clean: | ||
| try: | ||
| shutil.rmtree(decompressed_dir) | ||
| except OSError: |
There was a problem hiding this comment.
[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_pathsNote 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.
…ses; retry failed removals Two further gaps in the cleanup path: 1. _get_code_dir() appended to self._temp_uncompressed_paths_to_be_cleaned with `+=` outside self._lock, while _clean_decompressed_paths() snapshots and clears that same list under the lock. start-api/start-lambda run threaded, so a request thread's append could race with a concurrent cleanup's snapshot: the new entry could be silently dropped, or worse, removed by rmtree while the request that just unzipped it is still about to use it. Hold the lock on the producer side too. 2. A path whose rmtree() failed was being dropped from the cleanup list permanently, leaking silently for the rest of the process. Since failures at this call site (a directory just unmounted from a just-stopped container) are commonly transient, requeue failed paths instead of discarding them, so they get retried on the next cleanup pass.
|
Both real, and both go deeper than the previous fixes reached: Race between producer and consumer — confirmed: Failed removals dropped permanently — also confirmed as a regression from the previous fix: the snapshot-and-clear approach fixed "one bad entry blocks/stalls everything" but introduced "one bad entry silently vanishes forever," trading one leak class for another, quieter one. Requeued failed paths back into the list (under lock) instead of discarding them, so a transient failure gets retried on the next cleanup pass rather than leaking for the life of the process. Pushed c298d1b. Added Full |
| # `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: |
There was a problem hiding this comment.
[CONCURRENCY] Taking self._lock around the append does fix the torn-list/lost-entry race, but it does not prevent the second scenario the new comment claims it does — "having it removed by rmtree while still in use". That race is caused by the list being process-global rather than per-invocation, and it survives this change:
# thread A (request 1): create() -> getcode_dir() appends dirA, container A starts with dirA bind-mounted
# thread B (request 2): invoke finishes -> oninvoke_done() -> cleandecompressed_paths()
# snapshot = [dirA, dirB] -> rmtree(dirA) while container A is still running_clean_decompressed_paths() always removes every queued path, and _on_invoke_done() calls it on each individual invoke, so any concurrent in-flight invocation's decompressed directory is deleted underneath it. samcli/local/services/base_local_service.py:62 runs Flask multi-threaded whenever not debugging, and start-api/start-lambda use plain LambdaRuntime (not WarmLambdaRuntime) unless --warm-containers is passed, so two concurrent requests against .jar/.zip CodeUris hit this. The result is a container reading from a deleted host directory — a harder-to-diagnose failure than the leak being fixed.
Fixing it properly means scoping the paths to the invocation (e.g. have create()/_get_code_dir() return the paths it unzipped and pass only those to _on_invoke_done() for cleanup). If that is out of scope for this PR, please drop the "removed by rmtree while still in use" claim from the comment so it doesn't read as a guarantee the code does not provide.
| # 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 |
There was a problem hiding this comment.
[RESOURCE_MANAGEMENT] Requeuing failed paths only helps a process that will call _clean_decompressed_paths() again. For sam local invoke — the headline case in the PR description — there is no later pass, so a requeued path is still a permanent leak with only a warning:
- InvokeContext.exit (samcli/commands/local/cli_common/invoke_context.py:348) calls _clean_running_containers_and_related_resources() only when _containers_mode == ContainersMode.WARM.
- For cold mode it calls LambdaRuntime.clean_runtime_containers(), which only handles the durable-lambda and emulator containers and never touches _temp_uncompressed_paths_to_be_cleaned.
- Even in warm mode, anything requeued by the final clean_running_containers_and_related_resources() call is dropped when the process exits.
Since the added comment states these rmtree failures are "commonly transient (e.g. a directory that was just bind-mounted into a just-stopped container isn't released yet)", the single-shot invoke path — where the container was stopped microseconds earlier — is exactly where a transient failure is most likely and where the retry never happens.
A last best-effort attempt at teardown closes the gap, e.g. calling self._clean_decompressed_paths() from LambdaRuntime.clean_runtime_containers(), which InvokeContext.exit already invokes for every container mode:
def clean_runtime_containers(self):
...
# existing container cleanup
...
# Final best-effort pass so paths requeued by earlier failures get one more attempt
# before the process exits.
self._clean_decompressed_paths()1 participant
Which issue(s) does this change fix?
Fixes #9182
Why is this change necessary?
LambdaRuntime._on_invoke_done()calls_check_exit_state(container)beforeself._container_manager.stop(container)andself._clean_decompressed_paths():_on_invoke_doneis invoked unconditionally frominvoke()'sfinallyblock, but has no try/except of its own. When a function is OOM-killed,_check_exit_state()raisesContainerFailureError, and that exception propagates immediately out of_on_invoke_done, skipping both_container_manager.stop(container)and_clean_decompressed_paths().Every OOM'd invocation of
sam local invoke(orstart-api/start-lambda) therefore leaves a stopped-but-not-removed Docker container and an unzipped-code temp directory on disk. This accumulates over repeated local-invoke testing, which is exactly the workflow most likely to trigger it (iterating onMemorySizetuning).How does this change work?
Wrap the exit-state check in a
try/finallyso the container is always stopped and the temp directory is always cleaned up, regardless of whether_check_exit_stateraises:The
ContainerFailureError(or any other exception from_check_exit_state) still propagates after cleanup runs — this only fixes cleanup, not the error-reporting behavior for OOM.What tests ran and what were the results?
Added a regression test,
test_on_invoke_done_stops_container_and_cleans_paths_even_when_check_exit_state_raises, intests/unit/local/lambdafn/test_runtime.py. It fails against the pre-fix code (stop()is never called when_check_exit_stateraises) and passes after the fix.Full unit suite (
tests/unit) passes: 9387 passed, 25 skipped.mypyon the changed file: clean.Checklist
make prpasses locally (unit tests + mypy on changed files; full localmake prtarget run viapytest tests/unit -qand scopedmypy)By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.