Lambda: fix transient connection errors on first container invoke with retry logic #12522
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 1h 33m 42s ⏱️ - 19m 24s Results for commit a7fa7f1. ± Comparison against base commit 1724451. This pull request removes 1183 and adds 20 tests. Note that renamed tests count towards both.This pull request removes 102 skipped tests and adds 11 skipped tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
dfangl
reviewed
Apr 18, 2025
| for attempt_count in range(max_retry_on_connection_error + 1): # 1 initial + n retries | ||
| try: | ||
| response = requests.post(url=invocation_url, json=payload, proxies=proxies) | ||
| response.raise_for_status() |
There was a problem hiding this comment.
The response is checked on line 200 as well - this will prevent the other error handling, and leak request errors to the callers (instead of the expected InvokeSendError). We should not raise for the status here, I think the connection error should still be raised, right?
There was a problem hiding this comment.
Yes indeed I could confirm; I removed this raise in the invoke routine
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
This change improves the robustness of the container invocation logic by addressing a timing issue observed during Step Functions integration tests #12512, where transient ConnectionErrors occurred despite the container signaling readiness via
startup_future. Although the startup flag may be marked as FINISHED, this does not guarantee that the container is immediately ready to accept incoming connections. To handle this transitional state, the _perform_invoke method has been updated to automatically retry on requests.exceptions.ConnectionError up to_INVOCATION_CONNECTION_ERROR_MAX_RETRYtimes, using exponential backoff defined by_INVOCATION_CONNECTION_ERROR_BACKOFF_FACTOR_SECONDS.Alternative solutions were considered:
startup_futurewithin thestatus_readlogic was explored, but we currently lack a reliable mechanism to verify full container readiness without issuing an actual invocationstartup_futureresolution until the first successful invoke was also evaluated, but since transient network errors may occur beyond the initial request, a retry mechanism on every invoke call was deemed both simpler and more robust.Although this retry logic is primarily expected to benefit the first invocation following container startup, it applies to all invokes to ensure resilience under intermittent network conditions.
Changes
total_max_attemptsback to 1, as this issue appears now resolved