CLDSRV-892: store part checksum in part shadow object by leif-scality · Pull Request #6154 · scality/cloudserver

leif-scality · 2026-04-16T16:33:30Z

UploadPart
- Store part checksum in part shadow objet
- Verify MPU checksum config with received headers
- Handle dual checksum case (default MPU checksum + custom client checkum)
Refactor
- dataStore now takes checksum object as argument instead of parsing the headers
- getChecksumDataFromHeaders returns null instead of a default checksum, it is now up to the caller to select a default
Functional tests will be added by https://scality.atlassian.net/browse/S3C-11120 that will make ListParts return the checksum

bert-e · 2026-04-16T16:33:34Z

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-04-16T16:33:40Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-892 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-892, or the target
branch of this pull request.

codecov · 2026-04-16T16:35:28Z

Codecov Report

❌ Patch coverage is 95.78947% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.97%. Comparing base (41b498a) to head (fc49fd1).
⚠️ Report is 4 commits behind head on development/9.4.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
lib/api/apiUtils/object/storeObject.js	90.47%	2 Missing ⚠️
lib/routes/routeBackbeat.js	80.00%	2 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
lib/api/apiUtils/integrity/validateChecksums.js	`98.98% <100.00%> (+<0.01%)`	⬆️
lib/api/apiUtils/object/createAndStoreObject.js	`86.07% <100.00%> (+0.54%)`	⬆️
lib/api/apiUtils/object/prepareStream.js	`100.00% <100.00%> (ø)`
lib/api/objectPutPart.js	`88.34% <100.00%> (+1.25%)`	⬆️
lib/api/apiUtils/object/storeObject.js	`96.96% <90.47%> (-3.04%)`	⬇️
lib/routes/routeBackbeat.js	`76.75% <80.00%> (+0.01%)`	⬆️

... and 2 files with indirect coverage changes

@@                 Coverage Diff                 @@
##           development/9.4    #6154      +/-   ##
===================================================
- Coverage            84.98%   84.97%   -0.02%     
===================================================
  Files                  207      207              
  Lines                13694    13760      +66     
===================================================
+ Hits                 11638    11692      +54     
- Misses                2056     2068      +12

Flag	Coverage Δ
file-ft-tests	`68.75% <88.42%> (+0.07%)`	⬆️
kmip-ft-tests	`28.11% <26.31%> (-0.05%)`	⬇️
mongo-v0-ft-tests	`69.92% <88.42%> (-0.06%)`	⬇️
mongo-v1-ft-tests	`69.93% <88.42%> (-0.08%)`	⬇️
multiple-backend	`36.39% <36.84%> (-0.05%)`	⬇️
sur-tests	`36.35% <41.05%> (+0.81%)`	⬆️
sur-tests-inflights	`37.37% <41.05%> (+<0.01%)`	⬆️
unit	`71.46% <89.47%> (+0.07%)`	⬆️
utapi-v2-tests	`34.60% <48.42%> (+0.10%)`	⬆️

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-04-16T16:37:02Z

Bug: null dereference in createAndStoreObject.js:332 — checksum.type = 'FULL_OBJECT' crashes when the md-only path passes null as checksum (line 291: next(null, dataGetInfo, _md5, null)). Wrap in if (checksum) guard.
- Guard with if (checksum) { checksum.type = 'FULL_OBJECT'; }
Test quality: require() inside it() callbacks in objectPutPartChecksum.js — The same require('validateChecksums') is repeated at lines 193, 223, and 259 inside test functions.
- Move to the top of the file with the other requires

Review by Claude Code

claude · 2026-04-16T16:48:31Z

Crash bug: createAndStoreObject.js:333 — checksum.type = 'FULL_OBJECT' throws TypeError when checksum is null (md-only path passes null at line 291)
- Guard with if (checksum) before setting .type
Missing error handler wrapping: prepareStream.js:117 — default/UNSIGNED-PAYLOAD branch does not wrap errCb with jsutil.once() when secondary is present, unlike V4 and trailer cases
- Wrap with jsutil.once(errCb) for consistency
require() inside test callbacks: tests/unit/api/objectPutPartChecksum.js:192,222,258 — require() calls should be at the top of the file
- Move to top-level require block
Missing .catch(done) on Promise chains: tests/unit/api/objectPutPartChecksum.js:197,227,261 — unhandled rejections will cause test hangs
- Add .catch(done) to each Promise chain

Review by Claude Code

claude · 2026-04-16T17:14:30Z

LGTM

Review by Claude Code

claude · 2026-04-16T18:42:36Z

LGTM

The primary/secondary checksum architecture is clean. All callers of dataStore and prepareStream are updated with the new checksums parameter. The getChecksumDataFromHeaders returning null instead of the default is handled correctly at every call site with || defaultChecksumData. Stream error guards (jsutil.once) are properly applied when secondary introduces a third error source. Trailer wiring in the STREAMING-UNSIGNED-PAYLOAD-TRAILER path correctly routes to the secondary stream only when present, and falls back to the primary when there is no secondary. Test coverage is thorough across unit and functional layers, covering single-checksum, dual-checksum, algo mismatch rejection, and trailer flows.

Review by Claude Code

claude · 2026-04-16T18:44:26Z

LGTM

The refactor cleanly separates checksum parsing (caller) from stream setup (prepareStream), and the dual-checksum pipeline for MPU parts is well-designed: secondary upstream for validation, primary downstream for storage. Stream error handling correctly uses jsutil.once guards when multiple transforms are piped. The checksum algorithm mismatch rejection logic in objectPutPart correctly allows any algo for default MPUs while rejecting mismatches for explicitly configured ones. Test coverage is thorough across unit and functional tests.

Review by Claude Code

claude · 2026-04-23T10:35:23Z

LGTM

The refactoring cleanly separates checksum parsing from stream construction by having callers build the checksums object before passing it to dataStore/prepareStream. The dual-checksum logic (primary for storage, secondary for client-facing validation) is correctly piped and validated. All callers of the updated getChecksumDataFromHeaders (now returning null instead of a default) properly fall back to defaultChecksumData. Tests are thorough and cover single-checksum, dual-checksum, trailer, and algorithm mismatch paths.

Review by Claude Code

BourgoisMickael · 2026-04-23T23:21:55Z

                return checkHashMatchMD5(stream, hashedStream, dataRetrievalInfo,
-                    checksumedStream.stream, log, cbOnce);
+                    checksumedStream.stream, log, (err, dataInfo, hash, primaryChecksum) => {
+                        if (err) {return cbOnce(err);}


Suggested change

if (err) {return cbOnce(err);}

if (err) { return cbOnce(err); }

lets not use such 1 liners, ideally it should fail our linter
Another reference for this one: https://github.com/scality/Vault/pull/2274#discussion_r3090254295

BourgoisMickael · 2026-04-23T23:26:39Z

+
+before(async () => {
+    for (const algo of allAlgos) {
+


trailing whitespaces should be caught by linter

Suggested change

BourgoisMickael · 2026-04-23T23:35:06Z

+                    try {
+                        await s3.send(new UploadPartCommand({
+                            Bucket: bucket, Key: key, UploadId: uploadId,
+                            PartNumber: partNum, Body: partBody,
+                            [checksumField[mpuAlgo]]: wrongDigest[mpuAlgo],
+                        }));
+                        assert.fail('Expected BadDigest error');
+                    } catch (err) {
+                        assert.strictEqual(err.name, 'BadDigest');
+                    }


for this kind of pattern, to avoid try /catch in tests we can use assert.rejects, assert.throws

Done — converted all three test blocks to assert.rejects.

claude · 2026-04-24T08:51:25Z

LGTM

The refactoring is clean: moving checksum parsing out of prepareStream into callers and introducing the { primary, secondary } checksums object is a solid design that enables dual-checksum support without complicating the stream pipeline. The dual-checksum flow in storeObject correctly validates the secondary (client-facing) checksum first, then the primary (storage) checksum, and swaps them in the callback so the response echoes the client's algo while metadata stores the MPU's algo. Test coverage is thorough — unit tests cover algo matching, dual-checksum, trailer wiring, and metadata storage; functional tests cover the SDK-level contract.

Review by Claude Code

claude · 2026-04-24T10:51:44Z

Solid refactor. The dual-checksum design (primary for storage, secondary for client validation) is clean and well-tested. A few observations:

- checkHashMatchMD5 now always returns a checksum object (never null), which means every object put — even those without a client checksum header — now gets a stored checksum in metadata. This is the intended behavior but is a behavioral change worth noting for downstream consumers of metadata.
- In the STREAMING-UNSIGNED-PAYLOAD-TRAILER branch of prepareStream, the trailer event is exclusively routed to either the secondary or primary stream, but never both. This silently assumes the primary in dual-checksum mode is never a trailer checksum. Safe today, but could surprise a future caller.
- Functional tests use a shared mutable partNum counter incremented inside it() blocks — fragile under --grep or isolated test runs. Consider static per-test part numbers.

Review by Claude Code

claude · 2026-04-24T11:08:13Z

LGTM

The dual-checksum pipeline (primary for storage, secondary for client validation) is well-structured. All callers of dataStore and prepareStream are updated to the new signatures, and the Veeam route's prepareStream import is from arsenal — not affected. The behavioral change in checkHashMatchMD5 (always returning a checksum object instead of conditionally returning null) is safe given the writableFinished guard that guarantees digest is set. Test coverage is thorough — unit tests for algo match, dual-checksum, part metadata storage, and functional tests for each MPU algorithm configuration.

Review by Claude Code

leif-scality · 2026-04-24T12:45:38Z

ping

bert-e · 2026-04-24T12:45:47Z

Waiting for approval

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

the author
2 peers

leif-scality · 2026-04-24T12:46:03Z

/approve

bert-e · 2026-04-24T12:46:14Z

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

✔️ development/9.4

The following branches have NOT changed:

development/7.10
development/7.4
development/7.70
development/8.8
development/9.0
development/9.1
development/9.2
development/9.3

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

Goodbye leif-scality.

The following options are set: approve