AbortMPU cleanup - revisited #5854

williamlardier · 2025-07-01T16:21:20Z

Relates to https://scality.atlassian.net/wiki/spaces/OS/pages/3258220958/Retriable+CompleteMultipartUpload

See ticket & commits for context

Following this PR, we will fix the behavior of the CompleteMPU API to avoid promoting wrongly s3 objects if not all parts are valid. This might not be feasible because we want to be unified with S3C, that is not 100% standard with MPU management, as we can abort after a complete officially.

bert-e · 2025-07-01T16:21:25Z

Hello williamlardier,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

bert-e · 2025-07-01T16:21:31Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-669 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

8.8.54
9.0.19
9.1.0

Please check the Fix Version/s of CLDSRV-669, or the target
branch of this pull request.

codecov · 2025-07-01T16:26:00Z

Codecov Report

❌ Patch coverage is 95.91837% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.44%. Comparing base (4993509) to head (34ee809).
⚠️ Report is 10 commits behind head on development/8.8.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
lib/api/apiUtils/object/abortMultipartUpload.js	91.30%	2 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
lib/services.js	`78.66% <100.00%> (+1.58%)`	⬆️
lib/api/apiUtils/object/abortMultipartUpload.js	`91.39% <91.30%> (+6.58%)`	⬆️

... and 2 files with indirect coverage changes

@@                 Coverage Diff                 @@
##           development/8.8    #5854      +/-   ##
===================================================
+ Coverage            65.27%   65.44%   +0.16%     
===================================================
  Files                  188      188              
  Lines                12024    12064      +40     
===================================================
+ Hits                  7849     7895      +46     
+ Misses                4175     4169       -6

Flag	Coverage Δ
unit	`65.44% <95.91%> (+0.16%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

williamlardier · 2025-07-01T16:27:59Z

lib/api/apiUtils/object/abortMultipartUpload.js

+                    const listParams = {
+                        listingType: 'DelimiterVersions',
+                        // To only list the specific key, we need to add the versionId separator
+                        prefix: `${objectKey}${versioning.VersioningConstants.VersionId.Separator}`,


Will need further tests to ensure this works file - we do not officially have a way to list all versions of just one object

Test done, it's working well. Example:

{ "CommonPrefixes": [], "Versions": [ { "key": "bug-test-bda30555", "value": { "Size": 1048576, "ETag": "3eaf2c9512f76a55da265ad51ac21393-1", "VersionId": "98248542529027999999RG00001", "IsDeleteMarker": false, "LastModified": "2025-07-02T11:57:50.972Z", "Owner": { "DisplayName": "acc", "ID": "5874c85ff66d0ede3bd6e8fe9610f0640d1700a4607f8b7e414503fbd2cb03de" }, "StorageClass": "STANDARD" } }, { "key": "bug-test-bda30555", "value": { "Size": 1048576, "ETag": "3eaf2c9512f76a55da265ad51ac21393-1", "VersionId": "98248542540139999999RG00001", "IsDeleteMarker": false, "LastModified": "2025-07-02T11:57:39.857Z", "Owner": { "DisplayName": "acc", "ID": "5874c85ff66d0ede3bd6e8fe9610f0640d1700a4607f8b7e414503fbd2cb03de" }, "StorageClass": "STANDARD" } } ], "IsTruncated": false }

(while the same key with suffix exists)

bert-e · 2025-07-03T18:15:16Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-669 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

8.8.55
9.0.20
9.1.0

Please check the Fix Version/s of CLDSRV-669, or the target
branch of this pull request.

bert-e · 2025-07-17T12:05:40Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-669 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

8.8.55
9.0.21
9.1.0

Please check the Fix Version/s of CLDSRV-669, or the target
branch of this pull request.

lib/api/apiUtils/object/abortMultipartUpload.js

tests/functional/aws-node-sdk/test/object/abortMPU.js

tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js

bert-e · 2025-07-22T12:47:18Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-669 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

8.8.56
9.0.21
9.1.0

Please check the Fix Version/s of CLDSRV-669, or the target
branch of this pull request.

bert-e · 2025-07-22T12:59:26Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-669 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

8.8.56
9.0.22
9.1.0

Please check the Fix Version/s of CLDSRV-669, or the target
branch of this pull request.

.github/workflows/tests.yaml

francoisferrand · 2025-07-24T14:53:02Z

lib/api/apiUtils/object/abortMultipartUpload.js

+            let foundVersion = null;
+            let shouldContinue = true;
+
+            return async.whilst(


this function is already quite big, would make code more readable to introduce a helper function like findVersionWithUploadId()

I moved it to services.js, added tests for it, and refactored as a result the existing ones.

lib/api/apiUtils/object/abortMultipartUpload.js

bert-e · 2025-07-24T15:07:56Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-669 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

8.8.57
9.0.23
9.1.0

Please check the Fix Version/s of CLDSRV-669, or the target
branch of this pull request.

- CompleteMPU was and is still, at the time of this commit, wrongly promoting s3 objects when completing a MPU even if the parts are not valid. - The AbortMPU cleanup logic was added as an effort to cleanup new occurences of this problem, and still handle old ones not yet aborted. - The logic currently is unable to clean up if the current master version has a different upload id: in case a new version was pushed, we end up never cleaning the ghost. - This commit introduces a dynamic listing of the versionIDs of the object. We use the delimiter versions algorithm and the versionID separator to list only the current object versions, with pagination support, and then we detect the one to cleanup. Issue: CLDSRV-669

Issue: CLDSRV-669

- CompleteMPU was and is still, at the time of this commit, wrongly promoting s3 objects when completing a MPU even if the parts are not valid. - The AbortMPU cleanup logic was added as an effort to cleanup new occurences of this problem, and still handle old ones not yet aborted. - The logic currently is unable to clean up if the current master version has a different upload id: in case a new version was pushed, we end up never cleaning the ghost. - This commit introduces a dynamic listing of the versionIDs of the object. We use the delimiter versions algorithm and the versionID separator to list only the current object versions, with pagination support, and then we detect the one to cleanup. Issue: CLDSRV-669

Issue: CLDSRV-669

- Also, some test implementation was improved to reduce duplication. Issue: CLDSRV-669

Issue: CLDSRV-669

bert-e · 2025-07-28T10:06:35Z

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

Issue: CLDSRV-669

Issue: CLDSRV-699

lib/api/apiUtils/object/abortMultipartUpload.js

lib/services.js

tests/functional/aws-node-sdk/test/object/abortMPU.js

tests/unit/lib/services.spec.js

Issue: CLDSRV-669

williamlardier · 2025-07-31T07:35:05Z

/create_integration_branches

bert-e · 2025-07-31T07:35:24Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge bugfix/CLDSRV-669 into
development/8.8
w/9.0/bugfix/CLDSRV-669 will be merged into development/9.0
w/9.1/bugfix/CLDSRV-669 will be merged into development/9.1

The following branches will NOT be impacted:

development/7.10
development/7.4
development/7.70

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

The following options are set: create_integration_branches

bert-e · 2025-07-31T07:35:25Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

The following options are set: create_integration_branches

williamlardier · 2025-07-31T07:37:13Z

/approve

bert-e · 2025-07-31T07:53:49Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/8.8
✔️ development/9.0
✔️ development/9.1

The following branches have NOT changed:

development/7.10
development/7.4
development/7.70

Please check the status of the associated issue CLDSRV-669.

Goodbye williamlardier.

The following options are set: approve, create_integration_branches

williamlardier force-pushed the bugfix/CLDSRV-669 branch from 5cf7573 to a814106 Compare July 1, 2025 16:21

williamlardier commented Jul 1, 2025

View reviewed changes

williamlardier force-pushed the bugfix/CLDSRV-669 branch 2 times, most recently from a71a04f to f70c06a Compare July 2, 2025 12:18

williamlardier marked this pull request as ready for review July 3, 2025 18:23

williamlardier requested review from benzekrimaha and Kerkesni July 17, 2025 12:05

williamlardier requested a review from francoisferrand July 17, 2025 12:05

DarkIsDude reviewed Jul 22, 2025

View reviewed changes

williamlardier force-pushed the bugfix/CLDSRV-669 branch 4 times, most recently from 9880217 to a243976 Compare July 22, 2025 12:25

DarkIsDude approved these changes Jul 23, 2025

View reviewed changes

.github/workflows/tests.yaml Outdated Show resolved Hide resolved

francoisferrand reviewed Jul 24, 2025

View reviewed changes

williamlardier added 6 commits July 28, 2025 09:23

Add unit tests for abortMPU

4eb2cd2

Issue: CLDSRV-669

Add unit and functional tests

7705177

Issue: CLDSRV-669

Rework log level, and simplify cleanup checks

29859c1

- Also, some test implementation was improved to reduce duplication. Issue: CLDSRV-669

Test that warn log is catched by test - data delete error path

c2e8978

Issue: CLDSRV-669

williamlardier force-pushed the bugfix/CLDSRV-669 branch from 9373b6c to ba31070 Compare July 28, 2025 10:06

Move uploadId listing and adapt/add tests

e81db96

Issue: CLDSRV-669

williamlardier force-pushed the bugfix/CLDSRV-669 branch from ba31070 to e81db96 Compare July 28, 2025 10:07

williamlardier requested a review from francoisferrand July 28, 2025 10:07

Add corner case test coverage, and revert unneeded format changes

c859be7

Issue: CLDSRV-699

francoisferrand reviewed Jul 29, 2025

View reviewed changes

lib/api/apiUtils/object/abortMultipartUpload.js Outdated Show resolved Hide resolved

francoisferrand reviewed Jul 29, 2025

View reviewed changes

francoisferrand approved these changes Jul 29, 2025

View reviewed changes

williamlardier added 2 commits July 31, 2025 09:32

Add test coverage and make code simpler to understand

fdca4e6

Issue: CLDSRV-669

Linter fix

34ee809

Issue: CLDSRV-669

bert-e merged commit 34ee809 into development/8.8 Jul 31, 2025
26 checks passed

bert-e deleted the bugfix/CLDSRV-669 branch July 31, 2025 07:53

AbortMPU cleanup - revisited #5854

AbortMPU cleanup - revisited #5854

Conversation

williamlardier commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bert-e commented Jul 1, 2025

Hello williamlardier,

Uh oh!

bert-e commented Jul 1, 2025

Incorrect fix version

Uh oh!

codecov bot commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

williamlardier Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

williamlardier Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

bert-e commented Jul 3, 2025

Incorrect fix version

Uh oh!

bert-e commented Jul 17, 2025

Incorrect fix version

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bert-e commented Jul 22, 2025

Incorrect fix version

Uh oh!

bert-e commented Jul 22, 2025

Incorrect fix version

Uh oh!

Uh oh!

Uh oh!

francoisferrand Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

williamlardier Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bert-e commented Jul 24, 2025

Incorrect fix version

Uh oh!

bert-e commented Jul 28, 2025

Request integration branches

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

williamlardier commented Jul 31, 2025

Uh oh!

bert-e commented Jul 31, 2025

Integration data created

Uh oh!

bert-e commented Jul 31, 2025

Waiting for approval

Uh oh!

williamlardier commented Jul 31, 2025

Uh oh!

bert-e commented Jul 31, 2025

Uh oh!

Uh oh!

Uh oh!

williamlardier commented Jul 1, 2025 •

edited

Loading

codecov bot commented Jul 1, 2025 •

edited

Loading