CLDSRV-908: CopyObject handle checksums by leif-scality · Pull Request #6176 · scality/cloudserver

leif-scality · 2026-05-27T19:51:48Z

Forward a src object checksum to the dest object
Recompute the checksum when required (x-amz-checksum-algorithm header set, src object MPU with COMPOSITE checksum, ...). If needed this will trigger a GET operation to download and calculate the object checksum
Compute a CRC64NMVE for the dest object if the src object has no checksum (counts as a recompute so the same conditions as above apply)

  ┌─────┬─────────────────────┬────────────────────────────────────┬──────────────────────────────────────────────────────────────────┐
  │  #  │   Source checksum   │ x-amz-checksum-algorithm requested │                            Recompute?                            │
  ├─────┼─────────────────────┼────────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ 1   │ none                │ none                               │ Yes (Compute default CRC64NVME)                                  │
  ├─────┼─────────────────────┼────────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ 2   │ FULL_OBJECT, algo X │ none                               │ No — propagate as-is                                             │
  ├─────┼─────────────────────┼────────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ 3   │ FULL_OBJECT, algo X │ X (same algo)                      │ No — propagate as-is                                             │
  ├─────┼─────────────────────┼────────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ 4   │ FULL_OBJECT, algo X │ Y (different algo)                 │ Yes                                                              │
  ├─────┼─────────────────────┼────────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ 5   │ COMPOSITE, algo X   │ none                               │ Yes (can't propagate a MPU format digest to a single-object dest)│
  ├─────┼─────────────────────┼────────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ 6   │ COMPOSITE, algo X   │ X (same algo)                      │ Yes                                                              │
  ├─────┼─────────────────────┼────────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ 7   │ COMPOSITE, algo X   │ Y (different algo)                 │ Yes                                                              │
  ├─────┼─────────────────────┼────────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ 8   │ none                │ any algo                           │ Yes (source has no digest, must compute)                         │
  ├─────┼─────────────────────┼────────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ 9   │ any                 │ source is 0-byte                   │ Yes (special-cased — empty-bytes digest, no streaming)           │
  └─────┴─────────────────────┴────────────────────────────────────┴──────────────────────────────────────────────────────────────────┘

Backend / scenario	Copy method	Old Copy method (pre-PR)
file / mem / sproxyd (scality)	`GET` + `PUT` + `DELETE`	`GET` + `PUT` + `DELETE` (same)
aws_s3 / azure / gcp — propagate (FULL_OBJECT src, algorithm absent or matching)	Native server-side copy (`CopyObject` / `beginCopyFromURL` / `copyObject`) — no GET	Native server-side copy (same)
aws_s3 / azure / gcp — recompute	`GET` + `PUT` + `DELETE` — streamed through CloudServer	Native server-side copy — no GET
external, cross-type or external ↔ local (e.g. aws → azure, aws → file)	`GET` + `PUT` + `DELETE`	`GET` + `PUT` + `DELETE` (same)
external, any + SSE	`GET` + `PUT` + `DELETE`	`GET` + `PUT` + `DELETE` (same)
copy-to-self — propagate (same location, no SSE, unversioned)	metadata-only — reuse location (no data)	metadata-only — reuse location (same)
copy-to-self — recompute (any backend)	`GET` only — reuse location (no PUT, no DELETE)	metadata-only — reuse location, no GET
0-byte source (same location / local)	metadata-only — no data	metadata-only — no data (same)
0-byte source → different external backend	`PUT` (`data.put(null)`)	`PUT` (`data.put(null)`) (same)

bert-e · 2026-05-27T19:51:52Z

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-27T19:51:58Z

Incorrect fix version

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

claude · 2026-05-27T19:56:22Z

LGTM — clean implementation of checksum propagation and recompute on CopyObject. Stream handling (jsutil.once guards, Azure per-part passthrough, error propagation) and the _shouldRecomputeChecksum decision logic are solid. One minor test style issue flagged inline.

- require() inside describe/withV4 callback in functional test — move to top of file

Review by Claude Code

codecov · 2026-05-27T20:00:41Z

❌ 2 Tests Failed:

Tests completed	Failed	Passed	Skipped
9367	2	9365	0

View the full list of 2 ❄️ flaky test(s)

"after each" hook for "should fail if trying to overwrite a delete marker"::MPU with x-scal-s3-version-id header With default signature "after each" hook for "should fail if trying to overwrite a delete marker"

Flake rate in main: 100.00% (Passed 0 times, Failed 11 times)

Stack Traces | 0.014s run time

We encountered an internal error. Please try again.

"after each" hook for "should fail if trying to overwrite a delete marker"::MPU with x-scal-s3-version-id header With v4 signature "after each" hook for "should fail if trying to overwrite a delete marker"

Flake rate in main: 100.00% (Passed 0 times, Failed 2 times)

Stack Traces | 0.014s run time

We encountered an internal error. Please try again.

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

claude · 2026-05-28T09:46:02Z

LGTM

Review by Claude Code

claude · 2026-05-28T13:34:35Z

Stream leak on source read error: when sourceStream errors in the recompute path, checksumStream is not destroyed, so data.put (reading from it) may hang indefinitely
- Destroy checksumStream in the sourceStream error handler
- Data orphan on copy-to-self with recompute: the recompute path writes new data via data.put, but deleteExistingData skips cleanup when sourceIsDestination is true, orphaning the old data locations
- Track whether new data was written in the recompute path and allow cleanup in deleteExistingData accordingly

Review by Claude Code

claude · 2026-05-28T15:22:06Z

LGTM

Review by Claude Code

claude · 2026-05-28T16:35:18Z

LGTM

Review by Claude Code

BourgoisMickael · 2026-05-29T13:53:01Z

+                if (err) {
+                    done(err);


When this error happens, it could be any error and the perPart is still piped.

So it could theoretically receive data from azure and continue to stream during a short time window until the final callback calls passthrough.destroy and unpipes the source perPart.

Maybe the perPart should be destroyed here to stop immediately the streaming

This would make every thing more complex for little gain, lets just let error propagate naturally

BourgoisMickael · 2026-05-29T13:54:33Z

+            // into the master passthrough and use its 'end' as the completion
+            // signal — same pattern arsenal's data.copyObject uses.
+            const perPart = new PassThrough();
+            perPart.once('error', done);


Should we consider encapsulating the error to include some part details in the error for better troubleshooting if the error is ever logged somewhere else in the streaming path.

added a commit improving theerrors

BourgoisMickael · 2026-05-29T17:17:21Z

            // and masterKeyId stored properly in metadata
            if (sourceIsDestination && storeMetadataParams.locationMatch
-                && !isVersionedObj && !needsEncryption) {
+                && !isVersionedObj && !needsEncryption && !shouldRecomputeChecksum) {


If you don't skip, you'll trigger a data GET + PUT + DELETE (for previous location).

This only for checksum recomputation. Should you rather define another path where if it's only recompute checksum, you allow only the GET to stream into the ChecksumTransform stream and then you discard the end of the data stream, and avoid having to do a data PUT + DELETE

Fixed I now Get and recompute but dont PUT and DELETE. I also removed recompute for external backend cases, so if the data does not go through cloudserver we dont GET, and the dest object receives no checksum if a recompute was required

claude · 2026-06-03T11:02:48Z

Well-structured PR with thorough test coverage across all checksum scenarios (propagation, recompute, 0-byte, COMPOSITE, multi-part, Azure, copy-to-self, external backends).

- Stream leak on error in _recomputeChecksumAndStore: when one of sourceStream/checksumStream errors, the other is not destroyed, potentially leaking file descriptors from data.get. Add explicit destroy() calls on both streams in the error branch of the finish/done callbacks (applies to both the copy-to-self and stream-through-and-put paths).

Review by Claude Code

claude · 2026-06-03T11:57:05Z

Potential data loss on copy-to-self with external backend (skipExternalBackendRecompute path): deleteExistingData at line 858 of objectCopy.js will delete the existing data locations even though no new data was written, because _shouldRecomputeChecksum() returns true but the recompute was skipped. The _dataReused flag is only set in _recomputeChecksumAndStore, not in the goGetData early-return path. Fix: set storeMetadataParams._dataReused = true before the early return at line 687.
- The unit test at line 1152 (should early-return on copy-to-self) should also spy on data.batchDelete to catch this.

Review by Claude Code

claude · 2026-06-03T14:45:24Z

LGTM — solid implementation of all 9 checksum cases from the PR matrix. The propagation/recompute/default logic is clean, the _orphanedDataLocations helper correctly fixes orphan cleanup for copy-to-self rewrites, and tests cover all key paths (multi-part source, Azure backend, SSE, copy-to-self metadata-only, versioned, 0-byte, external backend skip).

One finding: streams are not destroyed on error in _recomputeChecksumAndStore — see inline comment.

Review by Claude Code

claude · 2026-06-03T15:33:00Z

LGTM

Thorough implementation. The truth table in the PR description maps cleanly to the _shouldRecomputeChecksum logic. Stream error handling is solid throughout (jsutil.once guards, per-stream error listeners, proper destroy on failure). The _orphanedDataLocations fix correctly replaces the blanket !sourceIsDestination guard with identity-based filtering, which closes the orphan leak on cross-backend copy-to-self. Test coverage is comprehensive across all nine cases, including Azure, SSE, multi-part, and 0-byte edge cases.

Review by Claude Code

claude · 2026-06-03T15:54:15Z

LGTM

Thorough checksum handling for CopyObject with clean stream management, proper error handling via jsutil.once, and well-structured orphan detection via _orphanedDataLocations. The nine cases from the PR description are all covered by the implementation and test suite. No correctness issues found.

Review by Claude Code

claude · 2026-06-03T17:05:28Z

LGTM

Review by Claude Code

claude · 2026-06-03T19:52:36Z

LGTM — well-structured implementation with thorough test coverage across all the checksum scenarios (propagation, recompute, 0-byte, multi-part, Azure, copy-to-self, external backends, orphan cleanup, legacy string locations).

One note posted inline:

- Pre-existing needsEncryption bug (.algo vs .algorithm at line 692) makes the SSE guard in the new _recomputeChecksumAndStore copy-to-self path ineffective — not a regression from this PR, but worth tracking as a follow-up fix

Review by Claude Code

BourgoisMickael · 2026-06-05T17:07:53Z

+    it('should lowercase mixed-case input', () => {
+        const result = getCopyObjectChecksumAlgorithm({
+            'x-amz-checksum-algorithm': 'Sha256',
+        });
+        assert.strictEqual(result.error, null);
+        assert.strictEqual(result.algorithm, 'sha256');
+    });
+
+    it('should accept algorithms already in lowercase', () => {
+        const result = getCopyObjectChecksumAlgorithm({
+            'x-amz-checksum-algorithm': 'crc32',
+        });
+        assert.strictEqual(result.error, null);
+        assert.strictEqual(result.algorithm, 'crc32');
+    });


you could include these 2 inside the validAlgorithms array above to run in the loop, the test body is the same

BourgoisMickael · 2026-06-05T17:14:10Z

+    it('should produce an arsenal InvalidRequest error via the standard mapping', () => {
+        const result = getCopyObjectChecksumAlgorithm({
+            'x-amz-checksum-algorithm': 'GARBAGE',
+        });
+        const err = arsenalErrorFromChecksumError(result.error);
+        assert.strictEqual(err.is.InvalidRequest, true);
+        assert(err.description.includes('Checksum algorithm provided is unsupported'),
+            'expected AWS-shaped error description');
+    });


This doesn't seem to be related to getCopyObjectChecksumAlgorithm but rather arsenalErrorFromChecksumError maybe you should have another describe separate for that function

claude · 2026-06-11T14:11:58Z

LGTM

Thorough implementation of checksum handling on CopyObject. The propagation/recompute decision matrix covers all documented cases. Error handling with jsutil.once() guards against double callbacks throughout the stream paths. The orphan-detection filter (_orphanedDataLocations) correctly handles legacy string locations and cross-backend identity comparisons. Test coverage is comprehensive — unit tests for propagation, recompute (per-algorithm, COMPOSITE sources, multi-part, SSE, Azure path, 0-byte, copy-to-self), orphan cleanup, and legacy locations; plus functional end-to-end tests.

Review by Claude Code

claude · 2026-06-11T15:30:59Z

LGTM

Review by Claude Code

The recompute path writes the destination through plain data.put, which (unlike data.copyObject, used by the propagate path) does not return a dataStoreETag, so the destination's location entry was stored without one. Nothing breaks visibly — partNumber readers fall back to treating the copy as a single part — but it left the recompute path as the only data-writing path producing legacy ETag-less locations and dropped the written-bytes MD5 that integrity/audit tooling relies on. Read the MD5 from data.put's hashedStream callback argument (completedHash, which the arsenal wrapper guarantees is set before the callback fires) and stamp the location as 1:<md5>, matching createAndStoreObject. Falls back to dataRetrievalInfo.dataStoreETag for external backends. Adds a unit test asserting the recomputed destination location carries the 1:-prefixed MD5.

claude · 2026-06-11T15:40:42Z

LGTM

Thorough implementation with proper stream error handling (jsutil.once guards against double callbacks), correct orphan detection via (dataStoreName, key) identity, and comprehensive test coverage across all checksum scenarios (propagation, recompute, 0-byte, Azure, multi-part, SSE, copy-to-self, legacy string locations).

Review by Claude Code

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

View reviewed changes

Comment thread lib/api/objectCopy.js Dismissed

claude Bot reviewed May 27, 2026

View reviewed changes

Comment thread tests/functional/aws-node-sdk/test/object/objectCopy.js Outdated

leif-scality force-pushed the improvement/CLDSRV-908-copy-object-handle-checksums branch from 5ca489a to 57d9894 Compare May 28, 2026 09:41

leif-scality force-pushed the improvement/CLDSRV-908-copy-object-handle-checksums branch from 57d9894 to 5bd2262 Compare May 28, 2026 13:29

claude Bot reviewed May 28, 2026

View reviewed changes

Comment thread lib/api/objectCopy.js Outdated

claude Bot reviewed May 28, 2026

View reviewed changes

Comment thread lib/api/objectCopy.js

leif-scality force-pushed the improvement/CLDSRV-908-copy-object-handle-checksums branch from 5bd2262 to 8b2b196 Compare May 28, 2026 15:13

leif-scality force-pushed the improvement/CLDSRV-908-copy-object-handle-checksums branch from 8b2b196 to 720f686 Compare May 28, 2026 16:28

BourgoisMickael reviewed May 28, 2026

View reviewed changes

Comment thread lib/api/apiUtils/integrity/validateChecksums.js Outdated

tcarmet approved these changes May 28, 2026

View reviewed changes

BourgoisMickael reviewed May 29, 2026

View reviewed changes

Comment thread lib/api/objectCopy.js Outdated

BourgoisMickael reviewed May 29, 2026

View reviewed changes

leif-scality force-pushed the improvement/CLDSRV-908-copy-object-handle-checksums branch from 720f686 to 6385c3e Compare June 3, 2026 10:56

github-advanced-security AI found potential problems Jun 3, 2026

View reviewed changes

Comment thread lib/api/objectCopy.js Dismissed