Skip to content

Improve handling of LDM shutdowns during invocations #13002

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 4 commits into from
Aug 13, 2025
Merged

Improve handling of LDM shutdowns during invocations #13002

merged 4 commits into from
Aug 13, 2025
+16 −1

Conversation

MEPalma
Copy link
Contributor

@MEPalma MEPalma commented Aug 13, 2025 •
edited
Loading

Motivation

The current Lambda version manager invocation logic for LDM does not handle cases correctly when an LDM configuration is removed during invocation. These changes improve the process to ensure such scenarios are handled more gracefully.

Changes

  • Handle canceled errors during LDM invokes

@MEPalma MEPalma added this to the 4.8 milestone Aug 13, 2025
@MEPalma MEPalma requested a review from joe4dev August 13, 2025 10:49
@MEPalma MEPalma self-assigned this Aug 13, 2025
@MEPalma MEPalma added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Aug 13, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 13, 2025 •
edited
Loading

Test Results - Preflight, Unit

22 106 tests  ±0   20 371 ✅ ±0   6m 16s ⏱️ -14s
     1 suites ±0    1 735 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 594a9a1. ± Comparison against base commit a1ae8f0.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 13, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 20m 19s ⏱️ - 24m 47s
3 379 tests  - 1 249  3 042 ✅  - 1 146  337 💤  - 103  0 ❌ ±0 
3 381 runs   - 1 249  3 042 ✅  - 1 146  339 💤  - 103  0 ❌ ±0 

Results for commit 594a9a1. ± Comparison against base commit a1ae8f0.

This pull request removes 1249 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 Aug 13, 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.

Thanks for improving the general user experience when Lambda invocations are interrupted (e.g., when manually destroying the container during an invocation).
lgtm once CI is 🟢

localstack-core/localstack/services/lambda_/invocation/version_manager.py
@@ -212,6 +213,16 @@ def invoke(self, *, invocation: Invocation) -> InvocationResult:
self.store_logs(
invocation_result=invocation_result, execution_env=ldm_execution_environment
)
except CancelledError:
# Timeouts for invocation futures are managed by LDM, a cancelled error here is
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a debug log with exception details (stack trace only in debug mode following #12989), in case we need to troubleshoot cancellations?

dfangl
dfangl approved these changes Aug 13, 2025
View reviewed changes
Copy link
Member

@dfangl dfangl left a comment

Choose a reason for hiding this comment

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

LGTM, makes sense!

localstack-core/localstack/services/lambda_/invocation/version_manager.py
@@ -212,6 +213,16 @@ def invoke(self, *, invocation: Invocation) -> InvocationResult:
self.store_logs(
invocation_result=invocation_result, execution_env=ldm_execution_environment
)
except CancelledError:
# Timeouts for invocation futures are managed by LDM, a cancelled error here is
Copy link
Member

Choose a reason for hiding this comment

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

We set a timeout, right? it's just a very long one, but it still might have actually run out. Maybe we can include this in the error message?

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 13, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 15s ⏱️ +9s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 594a9a1. ± Comparison against base commit a1ae8f0.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

Test Results (amd64) - Integration, Bootstrap

    5 files  ±    0      5 suites  ±0   1h 50m 6s ⏱️ - 31m 6s
3 403 tests  - 1 584  3 070 ✅  - 1 325  333 💤  - 259  0 ❌ ±0 
3 409 runs   - 1 584  3 070 ✅  - 1 325  339 💤  - 259  0 ❌ ±0 

Results for commit 594a9a1. ± Comparison against base commit a1ae8f0.

This pull request removes 1584 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]
…

@MEPalma MEPalma merged commit 969e613 into main Aug 13, 2025
40 checks passed
@MEPalma MEPalma deleted the MEP-LDM-fix_stop_during_invoke branch August 13, 2025 14:50
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

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

@MEPalma MEPalma

Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Milestone
4.8
Development

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