CLDSRV-898: CompleteMultipartUpload checksums by leif-scality · Pull Request #6166 · scality/cloudserver

leif-scality · 2026-05-07T21:37:29Z

Calculate and stores the final checksum when FULL_OBJECT (COMPOSITE are going to be stored by https://scality.atlassian.net/browse/S3C-10399)
Calculate and compare the final object checksum with the one sent by the headers
Check that all parts have the correct checksum and checksum type

bert-e · 2026-05-07T21:37:33Z

Hello leif-scality,

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 · 2026-05-07T21:37:39Z

Incorrect fix version

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

None

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

9.4.0

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

…hecksum

claude · 2026-05-07T21:41:22Z

+            ];
+
+            describe(label, () => {
+                it('accepts when every part includes the matching checksum', () => {


Test names using it() should start with should (e.g. should accept when every part includes the matching checksum). Multiple tests in this file use verb-first naming like accepts..., returns..., rejects..., does not....

— Claude Code

claude · 2026-05-07T21:41:23Z

+        assert(got.value.endsWith('-3'),
+            `expected -N suffix, got ${got.value}`);
+        // computeCompositeMPUChecksum's deterministic output for these
+        // exact placeholder digests:


Move require('crypto') and require('.../validateChecksums') to the top of the file. crypto is already imported at line 2 and algorithms at line 18 — these inner requires are unnecessary duplicates.

— Claude Code

claude · 2026-05-07T21:41:24Z

+            });
+        });
+    });
+});


Missing newline at end of file.

— Claude Code

claude · 2026-05-07T21:41:37Z

 module.exports = completeMultipartUpload;
+module.exports.validatePerPartChecksums = validatePerPartChecksums;
+module.exports.computeFinalChecksum = computeFinalChecksum;
+module.exports.validateExpectedFinalChecksum = validateExpectedFinalChecksum;


Missing newline at end of file.

— Claude Code

claude · 2026-05-07T21:41:46Z

Missing newline at end of file in lib/api/completeMultipartUpload.js and tests/unit/api/apiUtils/integrity/crcCombine.js
- Add a trailing newline to both files
- require() calls inside test functions in tests/unit/api/completeMultipartUpload.js (lines 542, 567-568)
- Move to top of file; crypto and algorithms are already imported there
- Test names in tests/unit/api/completeMultipartUpload.js don't start with should
- Prefix it() descriptions with should per project convention

Review by Claude Code

claude · 2026-05-07T21:44:40Z

+            ];
+
+            describe(label, () => {
+                it('accepts when every part includes the matching checksum', () => {


Multiple it() test names in this file do not start with "should" (e.g. line 108 "accepts when every part...", line 120 "returns BadDigest when...", line 357 "accepts CompleteMPU when...", line 413 "rejects CompleteMPU with..."). Per project convention, it() names should start with "should".

— Claude Code

claude · 2026-05-07T21:44:41Z

+        // exact placeholder digests:
+        const expected = require('crypto').createHash('sha256')
+            .update(Buffer.concat([d1, d2, d3].map(x => Buffer.from(x, 'base64'))))
+            .digest('base64');


require() calls inside it() blocks. crypto is already imported at line 2 and algorithms at line 18. These should be removed in favor of the top-level imports.

Same pattern repeats at lines 568-569 (crypto2/algorithms re-imported) and line 659 (require('crypto') inline).

— Claude Code

claude · 2026-05-07T21:44:43Z

 module.exports = completeMultipartUpload;
+module.exports.validatePerPartChecksums = validatePerPartChecksums;
+module.exports.computeFinalChecksum = computeFinalChecksum;
+module.exports.validateExpectedFinalChecksum = validateExpectedFinalChecksum;


Missing trailing newline at end of file.

— Claude Code

claude · 2026-05-07T21:44:44Z

+            });
+        });
+    });
+});


Missing trailing newline at end of file.

— Claude Code

claude · 2026-05-07T21:44:52Z

        "@hapi/joi": "^17.1.1",
        "@smithy/node-http-handler": "^3.0.0",
-        "arsenal": "git+https://github.com/scality/Arsenal#8.4.1",
+        "arsenal": "git+https://github.com/scality/Arsenal#improvement/ARSN-580-handle-checksums-in-complete-mpu",


arsenal is pinned to a branch (improvement/ARSN-580-handle-checksums-in-complete-mpu) instead of a tag. Git-based dependencies must pin to a tag for reproducible builds. Replace with the actual release tag once the Arsenal changes are tagged.

— Claude Code

claude · 2026-05-07T21:45:04Z

arsenal dependency pinned to a branch instead of a tag (package.json:36). Must be pinned to a release tag before merge.
- Replace branch ref with a versioned tag once the Arsenal PR is merged and tagged.
- Multiple it() test names in tests/unit/api/completeMultipartUpload.js do not start with "should" (project convention).
- Prefix test descriptions with "should" (e.g. "should accept when every part...").
- require() calls inside it() blocks in tests/unit/api/completeMultipartUpload.js (lines 545, 568-569, 659). crypto and algorithms are already imported at the top of the file.
- Remove inline require() calls and use the existing top-level imports.
- Missing trailing newline at end of lib/api/completeMultipartUpload.js and tests/unit/api/apiUtils/integrity/crcCombine.js.
- Add trailing newline.

Review by Claude Code

codecov · 2026-05-07T21:58:00Z

Codecov Report

❌ Patch coverage is 93.42723% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.39%. Comparing base (0f6d34d) to head (054e5ef).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
lib/api/completeMultipartUpload.js	91.33%	22 Missing ⚠️
lib/api/listParts.js	91.30%	6 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
lib/api/apiUtils/integrity/crcCombine.js	`100.00% <100.00%> (ø)`
lib/api/apiUtils/integrity/validateChecksums.js	`99.04% <100.00%> (+0.05%)`	⬆️
lib/api/apiUtils/object/objectAttributes.js	`100.00% <100.00%> (ø)`
lib/api/listParts.js	`92.37% <91.30%> (ø)`
lib/api/completeMultipartUpload.js	`90.56% <91.33%> (+1.26%)`	⬆️

... and 1 file with indirect coverage changes

@@                 Coverage Diff                 @@
##           development/9.4    #6166      +/-   ##
===================================================
+ Coverage            85.26%   85.39%   +0.12%     
===================================================
  Files                  207      208       +1     
  Lines                13794    13990     +196     
===================================================
+ Hits                 11762    11947     +185     
- Misses                2032     2043      +11

Flag	Coverage Δ
kmip-ft-tests	`28.01% <3.99%> (-0.33%)`	⬇️
multiple-backend	`36.29% <4.46%> (-0.45%)`	⬇️
sur-tests	`36.21% <3.99%> (+0.44%)`	⬆️
sur-tests-inflights	`38.16% <58.45%> (+0.49%)`	⬆️
unit	`72.20% <93.19%> (+0.37%)`	⬆️
utapi-v2-tests	`35.38% <60.32%> (+0.35%)`	⬆️

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.

claude · 2026-05-11T14:06:45Z

        "@hapi/joi": "^17.1.1",
        "@smithy/node-http-handler": "^3.0.0",
-        "arsenal": "git+https://github.com/scality/Arsenal#8.4.1",
+        "arsenal": "git+https://github.com/scality/Arsenal#improvement/ARSN-580-handle-checksums-in-complete-mpu",


Arsenal is pinned to a branch (improvement/ARSN-580-handle-checksums-in-complete-mpu) instead of a tag. Git-based deps must pin to a tag for reproducible builds; branch refs are mutable and can change without warning.

— Claude Code

claude · 2026-05-11T14:06:52Z

 module.exports = completeMultipartUpload;
+module.exports.validatePerPartChecksums = validatePerPartChecksums;
+module.exports.computeFinalChecksum = computeFinalChecksum;
+module.exports.validateExpectedFinalChecksum = validateExpectedFinalChecksum;


Missing newline at end of file. The diff shows No newline at end of file — this will cause lint warnings and untidy diffs on future edits.

— Claude Code

claude · 2026-05-11T14:07:01Z

+    });
+
+    it('should return FULL_OBJECT checksum without -N suffix for CRC64NVME', async () => {
+        const crypto2 = require('crypto');


require('crypto') and require('.../validateChecksums') are called inside it() blocks (lines 543, 568-570, 659) instead of at the top of the file. Both modules are already imported at the top-level (crypto on line 2, algorithms on line 18). Use the existing top-level bindings instead.

— Claude Code

claude · 2026-05-11T14:07:08Z

+            ];
+
+            describe(label, () => {
+                it('accepts when every part includes the matching checksum', () => {


Test names using it() should start with should per project convention. Multiple tests in this file use imperative forms like accepts, returns, rejects, does not, header comparison, validateMethodChecksumNoChunking, etc. For example: it('accepts when every part includes...') → it('should accept when every part includes...').

— Claude Code

claude · 2026-05-11T14:07:30Z

Arsenal dependency pinned to a branch instead of a tag — branch refs are mutable and break reproducible builds. Pin to a tag (e.g. 8.4.2).
- lib/api/completeMultipartUpload.js and tests/unit/api/apiUtils/integrity/crcCombine.js are missing a trailing newline.
- tests/unit/api/completeMultipartUpload.js: require('crypto') and require('.../validateChecksums') called inside it() blocks when they are already imported at the top of the file.
- tests/unit/api/completeMultipartUpload.js: many it() test names do not start with should per project convention.

Review by Claude Code

…ecksum header

claude · 2026-05-11T15:30:24Z

        "@hapi/joi": "^17.1.1",
        "@smithy/node-http-handler": "^3.0.0",
-        "arsenal": "git+https://github.com/scality/Arsenal#8.4.1",
+        "arsenal": "git+https://github.com/scality/Arsenal#improvement/ARSN-580-handle-checksums-in-complete-mpu",


Arsenal is pinned to a branch (improvement/ARSN-580-handle-checksums-in-complete-mpu) instead of a tag. Git-based dependencies must pin to a tag for reproducible builds.

— Claude Code

claude · 2026-05-11T15:30:33Z

+            ];
+
+            describe(label, () => {
+                it('accepts when every part includes the matching checksum', () => {


Many it() test names in this file don't start with should (e.g. "accepts when every part includes...", "returns BadDigest when...", "header comparison is case-insensitive"). Per project convention, test names should start with should.

— Claude Code

claude · 2026-05-11T15:30:41Z

+            `expected -N suffix, got ${got.value}`);
+        // computeCompositeMPUChecksum's deterministic output for these
+        // exact placeholder digests:
+        const expected = require('crypto').createHash('sha256')


crypto and algorithms are already imported at the top of the file (lines 2 and 19). Move these require() calls out of the test body — the same redundant requires appear at lines 568-569 and 659.

— Claude Code

claude · 2026-05-11T15:30:53Z

Arsenal dependency is pinned to a branch instead of a tag — must be pinned to a tag for reproducible builds.
- Pin to the next Arsenal release tag once the ARSN-580 changes are merged and tagged.
- Many it() test names in tests/unit/api/completeMultipartUpload.js don't start with should.
- Prefix test names with should (e.g. should accept when every part includes the matching checksum).
- Redundant require('crypto') and require('.../validateChecksums') calls inside it() blocks (lines 543, 568-569, 659 in tests/unit/api/completeMultipartUpload.js).
- Use the top-level imports already present at lines 2 and 19.

Review by Claude Code

claude · 2026-05-11T15:56:51Z

        "@hapi/joi": "^17.1.1",
        "@smithy/node-http-handler": "^3.0.0",
-        "arsenal": "git+https://github.com/scality/Arsenal#8.4.1",
+        "arsenal": "git+https://github.com/scality/Arsenal#improvement/ARSN-580-handle-checksums-in-complete-mpu",


Arsenal is pinned to a branch (improvement/ARSN-580-handle-checksums-in-complete-mpu) instead of a tag. Git-based internal deps must pin to a tag for reproducible builds.

— Claude Code

claude · 2026-05-11T15:57:00Z

+        ],
+        (err, destinationBucket, xml) => {
+            const corsHeaders = collectCorsHeaders(request.headers.origin, request.method, destinationBucket);
+            monitoring.promMetrics('GET', bucketName, 400, 'listMultipartUploadParts');


monitoring.promMetrics('GET', bucketName, 400, 'listMultipartUploadParts') is called unconditionally in the final waterfall callback. On success, waterfall step 5 already emits a 200 metric, so this line double-counts every successful request as both 200 and 400. Guard with if (err).

— Claude Code

claude · 2026-05-11T15:57:17Z

+            const stored = [makeStoredPart(1, { algorithm, value: d1 }), makeStoredPart(2, { algorithm, value: d2 })];
+
+            describe(label, () => {
+                it('accepts when every part includes the matching checksum', () => {


Test names in it() blocks should start with should per project conventions (e.g. 'should accept when every part includes the matching checksum'). Multiple tests in this file use 'accepts', 'returns', etc.

— Claude Code

claude · 2026-05-11T15:57:26Z

+    });
+
+    it('should return FULL_OBJECT checksum without -N suffix for CRC64NVME', async () => {
+        const crypto2 = require('crypto');


crypto and algorithms are already imported at the top of the file (lines 1-2 and 14-15). Move these require() calls to the file-level imports instead of re-requiring inside it() blocks.

— Claude Code

claude · 2026-05-11T15:57:41Z

arsenal dependency pinned to a branch instead of a tag (package.json:36). Git-based internal deps must pin to a tag for reproducible builds.
- Pin to the corresponding Arsenal release tag once it is published.

- monitoring.promMetrics('GET', bucketName, 400, 'listMultipartUploadParts') in listParts.js:348 is called unconditionally in the final waterfall callback. On success the 200 metric is already emitted in waterfall step 5, so every successful request is double-counted as both 200 and 400. This is a pre-existing issue carried forward by the reformat; wrapping with if (err) would fix it.
- Guard the 400 metric with if (err) { ... }.

- Test names in tests/unit/api/completeMultipartUpload.js do not follow the should prefix convention for it() blocks.
- Prefix test names with should (e.g. 'should accept when...', 'should return BadDigest when...').

- require('crypto') and require('.../validateChecksums') are called inside it() blocks in tests/unit/api/completeMultipartUpload.js (lines 522, 553, 665) despite being already imported at the top of the file.
- Remove the redundant inner require() calls and use the existing top-level imports.

Review by Claude Code

leif-scality added 3 commits May 7, 2026 23:39

CLDSRV-898: rename getObjectAttributesXMLTag to xmlTag

bd07e58

CLDSRV-898: validate per-part checksums and x-amz-checksum-type

842e281

CLDSRV-898: calculate final FULL_OBJECT (crc combine) and COMPOSITE c…

546656c

…hecksum

claude Bot reviewed May 7, 2026

View reviewed changes

leif-scality force-pushed the improvement/CLDSRV-898-complete-mpu-checksums branch from 3a4f7e2 to 223fb7f Compare May 7, 2026 21:41

claude Bot reviewed May 7, 2026

View reviewed changes

leif-scality force-pushed the improvement/CLDSRV-898-complete-mpu-checksums branch from 223fb7f to 8095101 Compare May 11, 2026 14:04

claude Bot reviewed May 11, 2026

View reviewed changes

leif-scality added 4 commits May 11, 2026 17:08

CLDSRV-898: CompleteMPU calculate and validate final checksum with ch…

8f70fe3

…ecksum header

CLDSRV-898: CompleteMPU store FULL_OBJECT checksum in object metadata

a4660e4

CLDSRV-898: CompleteMPU set checksum value and type in response XML body

eaf722a

CLDSRV-898: CompleteMPU checksum functional tests

b62f706

leif-scality force-pushed the improvement/CLDSRV-898-complete-mpu-checksums branch from 8095101 to 9e8bcea Compare May 11, 2026 15:27

claude Bot reviewed May 11, 2026

View reviewed changes

leif-scality added 2 commits May 11, 2026 17:49

CLDSRV-898: fix lint issues

cdc81b8

TMP use arsenal branch

054e5ef

leif-scality force-pushed the improvement/CLDSRV-898-complete-mpu-checksums branch from 9e8bcea to 054e5ef Compare May 11, 2026 15:52

github-advanced-security AI found potential problems May 11, 2026

View reviewed changes

claude Bot reviewed May 11, 2026

View reviewed changes

+                          });
+                      });
+                  });
+              });

                
                    No newline at end of file

Conversation

leif-scality commented May 7, 2026

Uh oh!

bert-e commented May 7, 2026

Hello leif-scality,

Uh oh!

bert-e commented May 7, 2026

Incorrect fix version

Uh oh!

claude Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented May 7, 2026

Uh oh!

claude Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented May 7, 2026

Uh oh!

codecov Bot commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

claude Bot May 11, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 11, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 11, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 11, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented May 11, 2026

Uh oh!

claude Bot May 11, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 11, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 11, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented May 11, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot May 11, 2026

codecov Bot commented May 7, 2026 •

edited

Loading