Skip to content

fix ListObjectVersions pagination when version id marker is deleted #12259

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 2 commits into from
Feb 13, 2025
Merged

fix ListObjectVersions pagination when version id marker is deleted #12259

merged 2 commits into from
Feb 13, 2025

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Feb 13, 2025 •
edited
Loading

Motivation

As reported with #12255, we've had an issue with ListObjectVersions and deletion.
The failing scenario is reproduced in the added test, but it can be summarized like that:

The list operation, when paginated and arriving at the max amount it can return, will return a VersionIdMarker, representing the last marker it returned (newer operations of S3 return the next one, which is a bit smarter).

If you happen to delete this version, AWS will return the next value in line after this now deleted marker. Not sure exactly how they do it, if they encode the data in the version id, or keep track of deleted versions. I've tried to reverse engineer the VersionId base64 encoding, but didn't really get anything and didn't try very hard.

We didn't have anything to compare against as those values were randomly generated, and we didn't have a way to compare them against each other. I've introduced some ever increasing number to be the prefix of the b64 encoded value, so that we can lexicographically sort and compare them.

Changes

  • introduce ever-increasing number byte prefix in the version id of S3 objects to allow comparison
  • add the proper comparison in the pagination of ListObjectVersions
  • add new test for the scenario of the version id marker being deleted, and pagination in general

Side notes

  • I'm packing the sequence number as 6 bytes, so a max of 281 474 976 710 656. Minus the start time in millisecond right now, it would give approx. 279735559077354 increases (meaning new versions) before we overflow. I think we're good 😄 (irrational fear of overflow).

  • pagination is a mess in S3, not one operation implements it the same way, there's always a subtle difference 😅

fixes #12255

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels Feb 13, 2025
@bentsku bentsku added this to the 4.2 milestone Feb 13, 2025
@bentsku bentsku self-assigned this Feb 13, 2025
@bentsku bentsku mentioned this pull request Feb 13, 2025
Closed
1 task
@bentsku bentsku requested review from k-a-il and cloutierMat and removed request for cloutierMat February 13, 2025 03:49
@github-actions GitHub Actions
Copy link

S3 Image Test Results (AMD64 / ARM64)

  2 files    2 suites   8m 6s ⏱️
477 tests 425 ✅  52 💤 0 ❌
954 runs  850 ✅ 104 💤 0 ❌

Results for commit b5b330f.

@github-actions GitHub Actions
Copy link

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   59m 54s ⏱️ - 53m 23s
1 718 tests  - 2 379  1 556 ✅  - 2 225  162 💤  - 154  0 ❌ ±0 
1 720 runs   - 2 379  1 556 ✅  - 2 225  164 💤  - 154  0 ❌ ±0 

Results for commit b5b330f. ± Comparison against base commit 25d4d82.

This pull request removes 2380 and adds 1 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.s3.test_s3_list_operations.TestS3ListObjectVersions ‑ test_list_objects_versions_with_prefix_only_and_pagination

k-a-il
k-a-il approved these changes Feb 13, 2025
View reviewed changes
Copy link
Contributor

@k-a-il k-a-il left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@bentsku bentsku merged commit 904f55a into master Feb 13, 2025
38 checks passed
@bentsku bentsku deleted the fix-s3-versions-pagination branch February 13, 2025 18:15
@bentsku bentsku mentioned this pull request Feb 13, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@k-a-il k-a-il k-a-il approved these changes

Assignees

@bentsku bentsku

Labels
aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

bug: Deleting versions while listing them is broken since 3.0.0
2 participants
@bentsku @k-a-il