Skip to content

Fix lambda timeout race condition #12465

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

Jump to bottom
Merged
merged 3 commits into from
Apr 2, 2025
Merged

Fix lambda timeout race condition #12465

merged 3 commits into from
Apr 2, 2025

Conversation

dfangl
Copy link
Member

@dfangl dfangl commented Apr 1, 2025

Motivation

We recently discovered that a race condition is possible, killing in-progress invocations of lambda functions if the environment is scheduled for removal due to inactivity at the same time.

This will lead to different errors (depending on the phase of the execution), and has to be fixed.

Also, the error messages we return to the caller in case of internal errors during invocations do not convey the actual error - making it harder to trace the error, especially for internal calls.
Here we fix this, to clearly communicate the error to the caller.

Old error message:

Internal error while executing lambda

New error message (containing request id and function name + qualifier):

[fd2d870e-827c-44a2-b365-2eb7efc38e86] Internal error while executing lambda test-function-28f7fd59:None. Caused by AssignmentException: Could not start new environment: NoSuchImage:public.ecr.aws/lambda/python:3.12

The error logging in various places has also been improved to log the exception type as well as the message.

Changes

  • Fix race condition during environment timeout
  • Improve error messages

@dfangl dfangl added this to the 4.4 milestone Apr 1, 2025
@dfangl dfangl added the semver: patch Non-breaking changes which can be included in patch releases label Apr 1, 2025
@dfangl dfangl self-assigned this Apr 1, 2025
@dfangl dfangl marked this pull request as ready for review April 1, 2025 17:19
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 1, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 27m 58s ⏱️ - 23m 41s
3 131 tests  - 1 166  2 905 ✅  - 1 064  226 💤  - 102  0 ❌ ±0 
3 133 runs   - 1 166  2 905 ✅  - 1 064  228 💤  - 102  0 ❌ ±0 

Results for commit 46754dc. ± Comparison against base commit 9383d50.

This pull request removes 1166 tests. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

♻️ This comment has been updated with latest results.

joe4dev
joe4dev approved these changes Apr 2, 2025
View reviewed changes
Copy link
Member

@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Glad we caught this race condition and good logging improvements 📈

localstack-core/localstack/services/lambda_/invocation/execution_environment.py
"Keepalive timer passed, but current runtime status is %s. Aborting keepalive stop.",
self.status,
)
return
Copy link
Member

Choose a reason for hiding this comment

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

Great catch ✨

localstack-core/localstack/services/lambda_/invocation/execution_environment.py
@@ -274,6 +275,14 @@ def keepalive_passed(self) -> None:
self.id,
self.function_version.qualified_arn,
)
with self.status_lock:
Copy link
Member

Choose a reason for hiding this comment

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

docs: Would it make sense to add some explanation about the motivation why this is needed to guard against the race condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, added!

@dfangl dfangl merged commit e1f3422 into master Apr 2, 2025
31 checks passed
@dfangl dfangl deleted the lambda/timeout-race-condition branch April 2, 2025 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe4dev joe4dev joe4dev approved these changes

@dominikschubert dominikschubert Awaiting requested review from dominikschubert dominikschubert is a code owner

@gregfurman gregfurman Awaiting requested review from gregfurman gregfurman is a code owner

Assignees

@dfangl dfangl

Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
2 participants
@dfangl @joe4dev