Skip to content

Deleting a FIFO message with an expired receipt handle should raise an error #12442

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
thrau merged 7 commits into from
Apr 1, 2025
Merged

Deleting a FIFO message with an expired receipt handle should raise an error #12442

thrau merged 7 commits into from
Apr 1, 2025

Conversation

@baermat
Copy link
Member

@baermat baermat commented Mar 26, 2025 •
edited
Loading

Motivation

As per title. FIFO messages should not be deleted if the provided receipt handle has expired, tests against AWS failed consistently. Reversely, the same test succeeded consistently on standard queues, making fifo specific code necessary.
fixes #12278

Changes

  • adds tests to confirm the issue
  • adds new methods to decouple some of the extraction steps for the receipt handle to access time-related information
  • adds _pre_delete_checks hook when removing messages, because this behaviour is fifo specific, and a complete remodel of the delete method to separate standard and fifo behaviour to make this cleaner was out of scope for this PR.

@baermat baermat added the semver: patch Non-breaking changes which can be included in patch releases label Mar 26, 2025
@github-actions
Copy link

github-actions bot commented Mar 26, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 13m 6s ⏱️ - 39m 58s
3 056 tests  - 1 258  2 927 ✅  - 1 059  129 💤  - 199  0 ❌ ±0 
3 058 runs   - 1 258  2 927 ✅  - 1 059  131 💤  - 199  0 ❌ ±0 

Results for commit bf73c2c. ± Comparison against base commit 1005e96.

This pull request removes 1262 and adds 4 tests. Note that renamed tests count towards both. 
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]
…

tests.aws.services.sqs.test_sqs.TestSqsProvider ‑ test_delete_after_visibility_timeout[sqs]
tests.aws.services.sqs.test_sqs.TestSqsProvider ‑ test_delete_after_visibility_timeout[sqs_query]
tests.aws.services.sqs.test_sqs.TestSqsProvider ‑ test_fifo_delete_after_visibility_timeout[sqs]
tests.aws.services.sqs.test_sqs.TestSqsProvider ‑ test_fifo_delete_after_visibility_timeout[sqs_query]

♻️ This comment has been updated with latest results.

@baermat baermat changed the title Deleting a FIFO message with a expired receipt handle should raise an error Deleting a FIFO message with an expired receipt handle should raise an error Mar 26, 2025
@baermat baermat marked this pull request as ready for review March 26, 2025 17:22
gregfurman
gregfurman requested changes Mar 27, 2025
View reviewed changes
Copy link
Contributor

@gregfurman gregfurman left a comment

Choose a reason for hiding this comment

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

Have left some comments mostly related to snapshot testing. Feel free to ignore my nits 🙂

@baermat baermat requested a review from gregfurman March 28, 2025 14:43
thrau
thrau approved these changes Mar 31, 2025
View reviewed changes
Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for fixing this :-)

just two minor nits, nothing blocking

@baermat baermat force-pushed the sqs-fifo-visibility-timeout-11774 branch from 0ce4b09 to bf73c2c Compare April 1, 2025 08:24
@thrau thrau merged commit ac0ff24 into master Apr 1, 2025
31 checks passed
@thrau thrau deleted the sqs-fifo-visibility-timeout-11774 branch April 1, 2025 09:38
@woodywei woodywei mentioned this pull request Aug 5, 2025
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thrau thrau thrau approved these changes

@gregfurman gregfurman Awaiting requested review from gregfurman

Assignees

No one assigned

Labels

semver: patch Non-breaking changes which can be included in patch releases

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug: Deleting a message from the queue after visibility timeout expired doesn't throw ReceiptHandleIsInvalid

3 participants

@baermat @thrau @gregfurman