CLDSRV-872: store checksum in object metadata by leif-scality · Pull Request #6113 · scality/cloudserver

leif-scality · 2026-03-17T15:00:27Z

No description provided.

…ksums

…after upload

…bject

claude · 2026-03-17T15:02:19Z

lib/api/apiUtils/object/storeObject.js

+            if (checksumedStream.stream.writableFinished) {
+                return doValidate();
+            }
+            checksumedStream.stream.once('finish', doValidate);


In storeObject.js, when data.put completes and the callback fires, you listen for 'finish' on the ChecksumTransform if writableFinished is not yet true. However, if the ChecksumTransform's _flush fails (e.g. crc64nvme digest throws), the stream will emit 'error', not 'finish'. In that case, doValidate is never called, but the upgraded onStreamError handler will fire and clean up — so this is safe. Just noting there is no explicit timeout or fallback if neither 'finish' nor 'error' fires (e.g. stream stalls), but that would be a broader Node.js stream issue, not specific to this PR.

— Claude Code

claude · 2026-03-17T15:02:25Z

lib/api/apiUtils/object/storeObject.js

 * error, dataRetrievalInfo, and completedHash (if any)
 */
-function checkHashMatchMD5(stream, hashedStream, dataRetrievalInfo, log, cb) {
+function checkHashMatchMD5(stream, hashedStream, dataRetrievalInfo, checksumStream, log, cb) {


The checkHashMatchMD5 JSDoc is missing the new checksumStream parameter. Minor, but the existing params are documented and this one is not.

— Claude Code

claude · 2026-03-17T15:02:37Z

lib/api/apiUtils/object/prepareStream.js

+            );
+            checksumedStream.on('error', onStreamError);
+            stream.pipe(checksumedStream);
+            return { error: null, stream: checksumedStream };


The STREAMING-AWS4-HMAC-SHA256-PAYLOAD case creates a ChecksumTransform and pipes V4Transform into it, but if the request includes x-amz-trailer header (i.e. checksumAlgo.isTrailer === true), no one will ever call setExpectedChecksum() on the ChecksumTransform — unlike the TRAILER case which wires up the 'trailer' event. This means trailer-based checksums through the V4 signed chunked path would always fail with TrailerMissing.

Is STREAMING-AWS4-HMAC-SHA256-PAYLOAD-TRAILER intentionally unsupported (it is in unsupportedSignatureChecksums)? If so, consider adding a guard here rejecting isTrailer === true for this case, to fail fast with a clear error rather than silently producing a TrailerMissing error after data is already stored and then cleaned up.

— Claude Code

claude · 2026-03-17T15:02:46Z

lib/api/apiUtils/integrity/validateChecksums.js

+            };
+        }
+
+        const trailer = headers['x-amz-trailer'];


In getChecksumDataFromHeaders, when no checksum headers and no trailer are present, this defaults to crc64nvme. This means every PutObject request (even those not requesting checksum validation) will now compute a CRC64NVME hash of the entire body. This is an intentional design choice (always store a checksum in metadata), but it is a performance change — every put now pays the cost of CRC64NVME computation. Worth documenting this behavioral change.

— Claude Code

claude · 2026-03-17T15:02:59Z

lib/auth/streamingV4/trailingChecksumTransform.js

+        if (!this.streamClosed && this.readingTrailer && this.trailerBuffer.length === 0) {
+            // Nothing came after "0\r\n", don't fail.
+            // If the x-amz-trailer header was present then the trailer is required and ChecksumTransform will fail.
+            return callback();


The _flush method has three conditions that could be simplified. The second branch (readingTrailer && trailerBuffer.length !== 0) means data arrived after the 0\r\n marker but the trailer line never terminated with \r\n. This is correct behavior.

However, there is a subtle issue: when the stream ends with 0\r\n and nothing else (no trailer, no final \r\n), the first branch returns callback() (success). But readingTrailer is true and streamClosed is false. The comment says ChecksumTransform will fail if x-amz-trailer header was present — but if x-amz-trailer was NOT present, this silently succeeds with an incomplete chunked body. This seems intentional based on test testS3PutTrailerNoTrailerInBodyAndHeader, but is worth a comment clarifying this is by design.

— Claude Code

claude · 2026-03-17T15:03:57Z

Summary of issues found:

prepareStream.js:67 - STREAMING-AWS4-HMAC-SHA256-PAYLOAD with isTrailer=true never wires up setExpectedChecksum. Consider a fast-fail guard.
storeObject.js:20 - checkHashMatchMD5 JSDoc missing new checksumStream param.
validateChecksums.js:211 - Every PutObject now computes CRC64NVME. Worth documenting this perf change.
trailingChecksumTransform.js:41 - Silent success on zero-size chunk with no trailer. Intentional but warrants a comment.
storeObject.js:125 - finish/error handling is safe. No action needed.

Overall solid implementation with good error handling and test coverage.

Review by Claude Code

codecov · 2026-03-17T15:13:12Z

❌ 128 Tests Failed:

Tests completed	Failed	Passed	Skipped
240	128	112	0

View the top 3 failed test(s) by shortest run time

PUT data should respond with 403 Forbidden if backbeat user has wrong secret key::backbeat routes backbeat authorization checks PUT data should respond with 403 Forbidden if backbeat user has wrong secret key

Stack Traces | 0s run time

Expected values to be strictly equal:

undefined !== 403

PUT data should respond with 403 Forbidden if the account does not match the backbeat user::backbeat routes backbeat authorization checks PUT data should respond with 403 Forbidden if the account does not match the backbeat user

Stack Traces | 0s run time

Expected values to be strictly equal:

undefined !== 403

PUT metadata should respond with 403 Forbidden if no credentials are provided::backbeat routes backbeat authorization checks PUT metadata should respond with 403 Forbidden if no credentials are provided

Stack Traces | 0s run time

Expected values to be strictly equal:

undefined !== 403

should remove old object data locations if version is overwritten with same contents::backbeat routes backbeat PUT routes should remove old object data locations if version is overwritten with same contents

Stack Traces | 0s run time

ifError got unwanted exception: connect ECONNREFUSED 127.0.0.1:8000

should skip batch delete of a non-existent location::backbeat routes Batch Delete Route should skip batch delete of a non-existent location

Stack Traces | 0s run time

connect ECONNREFUSED 127.0.0.1:8000

should skip batch delete of empty location array::backbeat routes Batch Delete Route should skip batch delete of empty location array

Stack Traces | 0s run time

connect ECONNREFUSED 127.0.0.1:8000

with encryption configuration and legacy API v1::backbeat routes backbeat PUT routes PUT data + metadata should create a new complete object with encryption configuration and legacy API v1

Stack Traces | 0s run time

ifError got unwanted exception: connect ECONNREFUSED 127.0.0.1:8000

with encryption configuration::backbeat routes backbeat PUT routes PUT data + metadata should create a new complete object with encryption configuration

Stack Traces | 0s run time

ifError got unwanted exception: connect ECONNREFUSED 127.0.0.1:8000

with key notes/spring/march/1.txt::backbeat routes backbeat PUT routes PUT data + metadata should create a new complete object with key notes/spring/march/1.txt

Stack Traces | 0s run time

ifError got unwanted exception: connect ECONNREFUSED 127.0.0.1:8000

with key notes/summer/1.txt::backbeat routes backbeat PUT routes PUT data + metadata should create a new complete object with key notes/summer/1.txt

Stack Traces | 0s run time

ifError got unwanted exception: connect ECONNREFUSED 127.0.0.1:8000

with key notes/summer/2.txt::backbeat routes backbeat PUT routes PUT data + metadata should create a new complete object with key notes/summer/2.txt

Stack Traces | 0s run time

ifError got unwanted exception: connect ECONNREFUSED 127.0.0.1:8000

with key notes/zaphod/Beeblebrox.txt::backbeat routes backbeat PUT routes PUT data + metadata should create a new complete object with key notes/zaphod/Beeblebrox.txt

Stack Traces | 0s run time

ifError got unwanted exception: connect ECONNREFUSED 127.0.0.1:8000

with legacy API v1::backbeat routes backbeat PUT routes PUT data + metadata should create a new complete object with legacy API v1

Stack Traces | 0s run time

ifError got unwanted exception: connect ECONNREFUSED 127.0.0.1:8000

"after each" hook for "DELETE .system-d26a9498-cb7c-4a87-a44a-8ae204f5ba6c/system.xml should delete the XML file"::Veeam routes: veeam DELETE routes: "after each" hook for "DELETE .system-d26a9498-cb7c-4a87-a44a-8ae204f5ba6c/system.xml should delete the XML file"