Skip to content

Fix visibility check in _pre_delete_checks to use is_visible attribute #13223

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
baermat merged 2 commits into from
Oct 9, 2025
Merged

Fix visibility check in _pre_delete_checks to use is_visible attribute #13223

baermat merged 2 commits into from
Oct 9, 2025
+103 −2

Conversation

@xhoantran
Copy link
Contributor

@xhoantran xhoantran commented Oct 5, 2025 •
edited
Loading

Motivation

Resolves #12954

This pull request updates the logic for validating SQS message receipt handles before deletion. The main change is to use the is_visible property of the message instead of calculating expiration based on the last received timestamp and visibility timeout.

Changes

  • Updated the _pre_delete_checks method in localstack-core/localstack/services/sqs/models.py to check the is_visible property of a message when validating receipt handles, rather than computing expiration from the last received time and visibility timeout. This simplifies and potentially improves the accuracy of message deletion checks.

Testing

  • Added test_fifo_delete_after_visibility_timeout_extended
  • Validated SNAPSHOT_UPDATE=1 TEST_TARGET=AWS_CLOUD TEST_PATH="tests/aws/services/sqs" make test
  • Validated TEST_PATH="tests/aws/services/sqs" make test

localstack-bot
localstack-bot reviewed Oct 5, 2025
View reviewed changes
Copy link
Contributor

@localstack-bot localstack-bot left a comment

Choose a reason for hiding this comment

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

Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.

@localstack-bot
Copy link
Contributor

localstack-bot commented Oct 5, 2025 •
edited
Loading

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@xhoantran
Copy link
Contributor Author

xhoantran commented Oct 5, 2025

I have read the CLA Document and I hereby sign the CLA

localstack-bot added a commit that referenced this pull request Oct 5, 2025
@localstack-bot
@xhoantran has signed the CLA in #13223
4030f06
@xhoantran
Copy link
Contributor Author

xhoantran commented Oct 6, 2025

@baermat can you help me review this PR please?

@thrau thrau added semver: patch Non-breaking changes which can be included in patch releases docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes labels Oct 6, 2025
@thrau
Copy link
Member

thrau commented Oct 6, 2025

this looks like a really good change, thanks @xhoantran! i've triggered the CI run and will let @baermat shepherd the contribution from here 👍

@xhoantran
Copy link
Contributor Author

xhoantran commented Oct 8, 2025

Hi @baermat, all the builds are passed. Can you help me review please?

baermat
baermat approved these changes Oct 9, 2025
View reviewed changes
Copy link
Member

@baermat baermat left a comment

Choose a reason for hiding this comment

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

Hello @xhoantran , thank you for contribution! These changes are looking good, they will be a valuable addition to LocalStack!

@baermat baermat merged commit 9a98fd2 into localstack:main Oct 9, 2025
50 of 54 checks passed
@xhoantran xhoantran deleted the feature/fifo_sqs_visibility branch October 9, 2025 17:24
baermat pushed a commit that referenced this pull request Oct 13, 2025
@xhoantran @baermat
Fix visibility check in _pre_delete_checks to use is_visible attribute (
03006aa 
#13223)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@baermat baermat baermat approved these changes

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

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

@localstack-bot localstack-bot Awaiting requested review from localstack-bot

Assignees

No one assigned

Labels

docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes 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: the _pre_delete_checks doesn't respect the ChangeMessageVisibilityTimeout

4 participants

@xhoantran @localstack-bot @thrau @baermat