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

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

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

BourgoisMickael · 2026-06-16T14:47:56Z

+            if (err) {
+                return done(err);
+            }
+            partStream.once('error', done);


the error is not wrapped here

BourgoisMickael · 2026-06-16T14:49:26Z

+            // into the master passthrough and use its 'end' as the completion
+            // signal — same pattern arsenal's data.copyObject uses.
+            const perPart = new PassThrough();
+            const wrapErr = err => Object.assign(err, {


You could pass the part as param to that function and define it once outside the async.eachSeries

BourgoisMickael · 2026-06-16T15:07:36Z

+        });
+        sourceStream.once('error', finish);
+        checksumStream.once('error', finish);
+        checksumStream.on('data', () => {});


It's not optimal to use a Transform only for the side effect of calculating checksum while discarding the output.
The Transform still works on pushing bytes into it's buffer.
Maybe we could rework the Checksum by implementing a new ChecksumWritable stream that does the same checksum computation but discards immediately the bytes without pushing.

I don't think it is worth it to create a ChecksumWritable just to avoid a NOP. The checksum transform already handles everything and it is already tested

BourgoisMickael · 2026-06-16T15:21:53Z

+        sourceStream.once('error', finish);
+        checksumStream.once('error', finish);


Maybe this error should be wrapped as well to easily troubleshoot if an error is coming from the source or checksum stream

BourgoisMickael · 2026-06-16T15:50:18Z

+        });
+        objectCopy(authInfo, req, sourceBucketName, objectKey, undefined, log, err => {
+            assert(err);
+            assert.strictEqual(err.is.InvalidRequest, true);


not ideal to use boolean comparison with .is

Suggested change

assert.strictEqual(err.is.InvalidRequest, true);

assert.strictEqual(err.message, 'InvalidRequest');

BourgoisMickael · 2026-06-16T15:52:01Z

+            objectCopy(authInfo, req, sourceBucketName, objectKey, undefined, log,
+                assertRecomputed('sha256', 'ChecksumSHA256', expectedDigest, err => {
+                    cipherBundleSpy.restore();
+                    dataPutSpy.restore();


Ideally those restore should be in a hook beforeEach or afterEach to cover the case where an error is thrown before without executing this callback, that would leave the spy in place

BourgoisMickael · 2026-06-16T15:53:54Z

+                            dataGetStub.restore();
+                            assertRecomputed('sha256', 'ChecksumSHA256', expectedDigest, done)(err, xml);
+                        });
+                }, err => {
+                    dataGetStub.restore();


Same here, the stub should be tracked outside this test to be restored in a hook to avoid missing it if any code path throws without reaching this callbacks

BourgoisMickael · 2026-06-16T15:59:13Z

+                        dataPutSpy.restore();
+                        batchDeleteSpy.restore();


same here, the first assert.ifError from metadata.getObjectMD would not restore the spys

BourgoisMickael · 2026-06-16T16:04:58Z

+                assert(xml.includes('<ChecksumCRC32>AAAAAA==</ChecksumCRC32>'));
+                assert(xml.includes('<ChecksumType>FULL_OBJECT</ChecksumType>'));


Prefer assert.match with a regex to see the full detail of the xml + expected pattern in error message, or include a message to the assert to provide more details about actual vs expected value

BourgoisMickael · 2026-06-16T16:13:45Z

+                    socket: {},
+                });
+                objectCopy(authInfo, req, sourceBucketName, objectKey, undefined, log, err => {
+                    batchDeleteSpy.restore();


Overall many sinon spy/stub in many tests seems they could skip restore if some error branches is reached (here the ifError of getObjectMD and putObjectMD).

Maybe we should include in the claude review skill that in test files any mock/spy/stub should be tracked and cleaned in an afterEach hook to make sure every branch is covered. Otherwise one test failure might impact another test.

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

Comment thread lib/api/objectCopy.js Outdated

BourgoisMickael reviewed May 29, 2026

View reviewed changes

Comment thread lib/api/objectCopy.js Outdated

BourgoisMickael reviewed May 29, 2026

View reviewed changes

Comment thread lib/api/objectCopy.js Outdated

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

claude Bot reviewed Jun 3, 2026

View reviewed changes

Comment thread lib/api/objectCopy.js

leif-scality force-pushed the improvement/CLDSRV-908-copy-object-handle-checksums branch from 6385c3e to 8ede498 Compare June 3, 2026 11:50

claude Bot reviewed Jun 3, 2026

View reviewed changes

Comment thread lib/api/objectCopy.js Outdated

claude Bot reviewed Jun 3, 2026

View reviewed changes

Comment thread tests/unit/api/objectCopy.js Outdated

leif-scality requested a review from BourgoisMickael June 3, 2026 20:08

BourgoisMickael reviewed Jun 5, 2026

View reviewed changes

Comment thread tests/unit/api/apiUtils/integrity/validateChecksums.js Outdated

BourgoisMickael reviewed Jun 5, 2026

View reviewed changes

Comment thread tests/unit/api/apiUtils/integrity/validateChecksums.js Outdated

leif-scality force-pushed the improvement/CLDSRV-908-copy-object-handle-checksums branch 2 times, most recently from b866c91 to ae43793 Compare June 11, 2026 14:07

leif-scality force-pushed the improvement/CLDSRV-908-copy-object-handle-checksums branch from ae43793 to 154e192 Compare June 11, 2026 15:26

leif-scality added 8 commits June 11, 2026 17:34

CLDSRV-908: add getCopyObjectChecksumAlgorithm helper

d86996d

CLDSRV-908: detect orphans by data-location key in deleteExistingData

735a329

CLDSRV-908: handle checksums on CopyObject

de35b8a

CLDSRV-908: handle 0-byte source recompute on CopyObject

950d3ad

CLDSRV-908: support Azure-source recompute on CopyObject

bffd651

CLDSRV-908: add functional tests for CopyObject checksum behavior

0ac6bc1

CLDSRV-908: improve Azure-source read error handling on CopyObject

e6ae5ae

leif-scality force-pushed the improvement/CLDSRV-908-copy-object-handle-checksums branch from 154e192 to e8311fb Compare June 11, 2026 15:35

leif-scality requested a review from BourgoisMickael June 12, 2026 11:14

BourgoisMickael reviewed Jun 16, 2026

View reviewed changes

		sourceStream.once('error', finish);
		checksumStream.once('error', finish);

	assert.strictEqual(err.is.InvalidRequest, true);
	assert.strictEqual(err.message, 'InvalidRequest');

		assert(xml.includes('<ChecksumCRC32>AAAAAA==</ChecksumCRC32>'));
		assert(xml.includes('<ChecksumType>FULL_OBJECT</ChecksumType>'));

Conversation

leif-scality commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bert-e commented May 27, 2026

Hello leif-scality,

Uh oh!

bert-e commented May 27, 2026

Incorrect fix version

Uh oh!

Uh oh!

Uh oh!

claude Bot commented May 27, 2026

Uh oh!

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

Uh oh!

❌ 2 Tests Failed:

Uh oh!

claude Bot commented May 28, 2026

Uh oh!

Uh oh!

Uh oh!

claude Bot commented May 28, 2026

Uh oh!

claude Bot commented May 28, 2026

Uh oh!

claude Bot commented May 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Jun 3, 2026

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Jun 3, 2026

Uh oh!

claude Bot commented Jun 3, 2026

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Jun 11, 2026

Uh oh!

claude Bot commented Jun 11, 2026

Uh oh!

claude Bot commented Jun 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BourgoisMickael Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

leif-scality commented May 27, 2026 •

edited

Loading

codecov Bot commented May 27, 2026 •

edited

Loading

BourgoisMickael Jun 16, 2026 •

edited

Loading