CLDSRV-898: CompleteMultipartUpload checksums

[leif-scality]

• 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
• First commits runs prettier over modified files

[bert-e]

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]

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.

[claude]

• 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]

• 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]

Codecov Report

❌ Patch coverage is 93.18182% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.34%. Comparing base (0f6d34d) to head (a9db936).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
lib/api/completeMultipartUpload.js	90.49%	25 Missing ⚠️
lib/api/listParts.js	91.42%	6 Missing ⚠️
lib/api/apiUtils/integrity/validateChecksums.js	97.05%	2 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
lib/api/apiUtils/integrity/crcCombine.js	100.00% <100.00%> (ø)
lib/api/apiUtils/object/objectAttributes.js	100.00% <100.00%> (ø)
lib/api/apiUtils/integrity/validateChecksums.js	98.79% <97.05%> (-0.20%)	⬇️
lib/api/listParts.js	92.43% <91.42%> (+0.06%)	⬆️
lib/api/completeMultipartUpload.js	89.90% <90.49%> (+0.60%)	⬆️

... and 4 files with indirect coverage changes

@@                 Coverage Diff                 @@
##           development/9.4    #6166      +/-   ##
===================================================
+ Coverage            85.26%   85.34%   +0.07%     
===================================================
  Files                  207      208       +1     
  Lines                13794    14039     +245     
===================================================
+ Hits                 11762    11982     +220     
- Misses                2032     2057      +25     

Flag	Coverage Δ
file-ft-tests	69.47% <78.51%> (+0.11%)	⬆️
kmip-ft-tests	27.92% <3.71%> (-0.42%)	⬇️
mongo-v0-ft-tests	70.63% <79.54%> (-0.03%)	⬇️
mongo-v1-ft-tests	70.62% <79.54%> (+0.06%)	⬆️
multiple-backend	36.17% <4.13%> (-0.57%)	⬇️
sur-tests	36.09% <3.71%> (+0.33%)	⬆️
sur-tests-inflights	38.13% <54.33%> (+0.46%)	⬆️
unit	72.22% <91.73%> (+0.39%)	⬆️
utapi-v2-tests	35.36% <55.99%> (+0.32%)	⬆️

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]

• 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

[claude]

• 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]

• algoLabel in validatePerPartChecksums uses .toLowerCase() which produces crc64nvme instead of AWS's CRC64NVME in the BadDigest error message. Consider .toUpperCase() for AWS parity.
  • lib/api/completeMultipartUpload.js:96
• processParts mixes .then() promise chain with the next callback, guarded by a manual nextCalled flag. This pattern is fragile — consider using async.asyncify or a small async wrapper.
  • lib/api/completeMultipartUpload.js:591-650
• finalChecksum is a closure-scoped mutable variable mutated inside a .then() callback. Not a bug today (waterfall is sequential) but a fragility point for future async/await migration.
  • lib/api/completeMultipartUpload.js:269,623
• combineCrcs accesses parts[0].value without an empty-array guard. An empty parts array would throw a TypeError.
  • lib/api/apiUtils/integrity/crcCombine.js:178
    
    Review by Claude Code

[claude]

LGTM — solid implementation of FULL_OBJECT and COMPOSITE checksum computation for CompleteMultipartUpload, with thorough test coverage across all algorithm/type combinations. The CRC combine implementation correctly ports the zlib matrix trick, and the per-part validation logic matches AWS behavior (including the subtle default-MPU rejection). Two minor observations posted inline:

- computeCompositeMPUChecksum is async but all COMPOSITE-eligible digest functions are synchronous — the async is harmless but adds a microtask tick per call
- The chainCache / ensureChainLen growth is bounded in practice (~42 entries × 3 polynomials) but worth a note for future maintainers

Review by Claude Code

[claude]

LGTM — solid implementation of MPU final-object checksums (FULL_OBJECT via CRC linearization, COMPOSITE via digest concatenation) with thorough test coverage across all algorithm/type combinations. One minor log-level concern flagged inline.

Review by Claude Code

[claude]

• callNext guard in processParts (completeMultipartUpload.js:598) silently swallows errors when the Promise rejects after the callback has already fired. Consider logging the discarded error so silent failures don't hide bugs.
  - Add log.warn('processParts: ignoring late rejection', { error: err }) in the else branch.
  
  - controllingLocationConstraint is not listed in keysNotNeeded (completeMultipartUpload.js:766), so it may leak into the stored ObjectMD.
  - Add 'controllingLocationConstraint' to the keysNotNeeded array alongside the other transient fields.
  
  Review by Claude Code

[claude]

LGTM

Clean, well-structured implementation of CompleteMultipartUpload final-object checksums (FULL_OBJECT via CRC linearization + COMPOSITE). Good separation of concerns between per-part validation, final-checksum compute, and header-assertion validation. The callNext double-callback guard in processParts is a smart safety net for the promise/callback boundary. Test coverage is thorough — unit tests for the CRC combine primitives, per-part validation matrix, computeFinalChecksum edge cases, and end-to-end functional tests across all (algorithm, type) pairs.

One notable bug fix in listParts.js: the original code unconditionally emitted a 400 Prometheus metric on every call (success or failure); the fix correctly gates it on err.

Review by Claude Code