Merge branch 'improvement/CLDSRV-952' into q/9.4

Author: bert-e

lib/routes/routeBackbeat.js

@@ -587,6 +587,9 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) {
     // Note: '' is falsy so the presence check must be !== undefined, not truthy.
     const hasMicroVersionId = encodedMicroVersionId !== undefined;
     let incomingMicroVersionId = null;
+    const metadataOptions = {
+        overheadField: constants.overheadField,
+    };
     if (hasMicroVersionId && objMd) {
         // '' means source has no microVersionId, treated as older revision
         incomingMicroVersionId = encodedMicroVersionId === '' ? null : decode(encodedMicroVersionId);
@@ -640,6 +643,13 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) {
                 conflictErr.mvId,
             );
         }
+
+        // Atomic counterpart of the JS pre-checks above, performed at database level.
+        if (incomingMicroVersionId !== null) {
+            metadataOptions.conditions = {
+                $or: [{ microVersionId: { $exists: false } }, { microVersionId: { $gt: incomingMicroVersionId } }],
+            };
+        }
     }
 
     return _getRequestPayload(request, (err, payload) => {
@@ -791,25 +801,21 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) {
             omVal.replicationInfo.isNFS = !omVal.replicationInfo.isReplica;
         }
 
-        const options = {
-            overheadField: constants.overheadField,
-        };
-
         // NOTE: When 'versioning' is set to true and no 'versionId' is specified,
         // it results in the creation of a "new" version, which also updates the master.
         // NOTE: Since option fields are converted to strings when they're sent to Metadata via the query string,
         // Metadata interprets the value "false" as if it were true.
         // Therefore, to avoid this confusion, we don't pass the versioning parameter at all if its value is false.
         if (versioning) {
-            options.versioning = true;
+            metadataOptions.versioning = true;
         }
 
         // NOTE: When options fields are sent to Metadata through the query string,
         // they are converted to strings. As a result, Metadata interprets the value undefined
         // in the versionId field as an empty string ('').
         // To prevent this, the versionId field is only included in options when it is defined.
         if (versionId !== undefined) {
-            options.versionId = versionId;
+            metadataOptions.versionId = versionId;
             omVal.versionId = versionId;
 
             if (isNull) {
@@ -829,13 +835,13 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) {
             // The update is only done when putting a new version as updating versions that already exist
             // shouldn't affect the master.
             if (!objMd) {
-                options.repairMaster = true;
+                metadataOptions.repairMaster = true;
             }
         }
 
         // If the new null keys logic (S3C-7352) is not supported (compatibility mode), 'isNull' remains undefined.
         if (!nullVersionCompatMode) {
-            options.isNull = isNull;
+            metadataOptions.isNull = isNull;
         }
 
         const isReplicationWrite = !!headers['x-scal-replication-content'];
@@ -937,7 +943,7 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) {
                         );
 
                         if (versioningPreprocessingResult) {
-                            options.deleteNullKey = versioningPreprocessingResult.deleteNullKey;
+                            metadataOptions.deleteNullKey = versioningPreprocessingResult.deleteNullKey;
 
                             // The master references a null version only via extraMD,
                             // which is set solely in nullVersionCompatMode. In null-key
@@ -953,9 +959,9 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) {
                     log.trace('putting object version', {
                         objectKey: request.objectKey,
                         omVal,
-                        options,
+                        metadataOptions,
                     });
-                    return metadata.putObjectMD(bucketName, objectKey, omVal, options, log, (err, md) => {
+                    return metadata.putObjectMD(bucketName, objectKey, omVal, metadataOptions, log, (err, md) => {
                         if (err) {
                             // Handle duplicate key error during repair operation
                             // This can happen due to race conditions when multiple operations
@@ -964,7 +970,7 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) {
                             // treat this as success.
                             const errorMessage = err.message || err.toString() || '';
                             const isRepairDuplicateKeyError =
-                                options.repairMaster &&
+                                metadataOptions.repairMaster &&
                                 (errorMessage.includes('E11000') ||
                                     errorMessage.includes('duplicate key') ||
                                     errorMessage.includes('repair'));
@@ -1045,7 +1051,25 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) {
                     });
                 },
             ],
-            callback,
+            (err, results) => {
+                if (err?.is?.PreconditionFailed && metadataOptions.conditions) {
+                    log.debug('putMetadata: write rejected, incoming microVersionId is not newer than stored', {
+                        method: 'putMetadata',
+                        bucketName,
+                        objectKey,
+                    });
+                    request.resume();
+                    return _respondWithHeaderCrrConflict(
+                        response,
+                        log,
+                        callback,
+                        StaleMicroVersionIdException.name,
+                        'incoming revision is not newer than stored',
+                        objMd?.microVersionId,
+                    );
+                }
+                return callback(err, results);
+            },
         );
     });
 }

package.json

@@ -35,7 +35,7 @@
         "@opentelemetry/instrumentation-ioredis": "~0.64.0",
         "@opentelemetry/instrumentation-mongodb": "~0.69.0",
         "@smithy/node-http-handler": "^3.0.0",
-        "arsenal": "git+https://github.com/scality/arsenal#8.5.12",
+        "arsenal": "git+https://github.com/scality/arsenal#8.5.14",
         "async": "2.6.4",
         "aws-crt": "^1.24.0",
         "bucketclient": "scality/bucketclient#8.2.7",

tests/functional/backbeat/putMetadata.js

@@ -289,6 +289,51 @@ describe('putMetadata : microVersionId conditional updates (no replication conte
             },
         );
     });
+
+    it('should allow first write to pre-cascade object then reject the same microVersionId', async () => {
+        const key = 'putmetadata-cond-pre-cascade';
+
+        // Step 1: write a pre-cascade object : no microVersionId in body, no header
+        await backbeatClient.send(
+            new PutMetadataCommand({
+                Bucket: TEST_BUCKET,
+                Key: key,
+                Body: buildMetadataBody({}),
+            }),
+        );
+        const { Body: beforeBody } = await backbeatClient.send(
+            new GetMetadataCommand({ Bucket: TEST_BUCKET, Key: key }),
+        );
+        assert.strictEqual(
+            new ObjectMD(JSON.parse(beforeBody)).getMicroVersionId(),
+            undefined,
+            'pre-cascade object should have no microVersionId',
+        );
+
+        // Step 2: first putMetadata with a microVersionId : $exists: false arm passes
+        const mvId = makeMicroVersionId();
+        await putMetadata(key, mvId);
+        const { Body: afterFirst } = await backbeatClient.send(
+            new GetMetadataCommand({ Bucket: TEST_BUCKET, Key: key }),
+        );
+        assert.strictEqual(
+            new ObjectMD(JSON.parse(afterFirst)).getMicroVersionId(),
+            mvId.raw,
+            'first write should store the microVersionId',
+        );
+
+        // Step 3: second write with the same microVersionId : already stored, should be rejected
+        await assert.rejects(
+            () => putMetadata(key, mvId),
+            err => {
+                assert.ok(
+                    err instanceof MicroVersionIdAlreadyStoredException,
+                    `expected MicroVersionIdAlreadyStoredException, got ${err.constructor.name}`,
+                );
+                return true;
+            },
+        );
+    });
 });
 
 // These tests send x-scal-replication-content to simulate backbeat replication writes.

tests/unit/routes/routeBackbeat.js

@@ -395,6 +395,115 @@ describe('routeBackbeat', () => {
             assert.deepStrictEqual(mockResponse.body, {});
         });
 
+        it('should set an atomic microVersionId condition when updating an existing version', async () => {
+            // reverse-chronological ordering: sorted ascending, the newer revision (incoming) sorts first
+            const [incomingRaw, storedRaw] = [
+                versioning.VersionID.generateVersionId('test', 'RG001'),
+                versioning.VersionID.generateVersionId('test', 'RG001'),
+            ].sort();
+
+            mockRequest = preparePutMetadataRequest({ microVersionId: incomingRaw });
+            mockRequest.headers['x-scal-micro-version-id'] = versioning.VersionID.encode(incomingRaw);
+            mockRequest.url = '/_/backbeat/metadata/bucket0/key0' + '?versionId=aIXVkw5Tw2Pd00000000001I4j3QKsvf';
+
+            metadataUtils.standardMetadataValidateBucketAndObj.callsFake((_params, _denies, _log, callback) => {
+                callback(null, bucketInfo, { microVersionId: storedRaw });
+            });
+
+            const putObjectMDStub = sandbox
+                .stub(metadata, 'putObjectMD')
+                .callsFake((_bucketName, _objectKey, _omVal, options, _logParam, cb) => {
+                    assert.deepStrictEqual(options.conditions, {
+                        $or: [{ microVersionId: { $exists: false } }, { microVersionId: { $gt: incomingRaw } }],
+                    });
+                    cb(null, {});
+                });
+
+            routeBackbeat('127.0.0.1', mockRequest, mockResponse, log);
+            void (await endPromise);
+
+            sinon.assert.called(putObjectMDStub);
+            assert.strictEqual(mockResponse.statusCode, 200);
+        });
+
+        it('should return 409 without writing when the incoming microVersionId is already stored', async () => {
+            const incomingRaw = versioning.VersionID.generateVersionId('test', 'RG001');
+
+            mockRequest = preparePutMetadataRequest({ microVersionId: incomingRaw });
+            mockRequest.headers['x-scal-micro-version-id'] = versioning.VersionID.encode(incomingRaw);
+            mockRequest.url = '/_/backbeat/metadata/bucket0/key0' + '?versionId=aIXVkw5Tw2Pd00000000001I4j3QKsvf';
+
+            metadataUtils.standardMetadataValidateBucketAndObj.callsFake((_params, _denies, _log, callback) => {
+                callback(null, bucketInfo, { microVersionId: incomingRaw });
+            });
+
+            const putObjectMDStub = sandbox.stub(metadata, 'putObjectMD');
+
+            routeBackbeat('127.0.0.1', mockRequest, mockResponse, log);
+            void (await endPromise);
+
+            sinon.assert.notCalled(putObjectMDStub);
+            assert.strictEqual(mockResponse.statusCode, 409);
+            assert.strictEqual(mockResponse.body.code, 'MicroVersionIdAlreadyStoredException');
+        });
+
+        it(
+            'should set an atomic microVersionId condition even when the stored revision has none ' +
+                '(pre-cascade object)',
+            async () => {
+                const incomingRaw = versioning.VersionID.generateVersionId('test', 'RG001');
+
+                mockRequest = preparePutMetadataRequest({ microVersionId: incomingRaw });
+                mockRequest.headers['x-scal-micro-version-id'] = versioning.VersionID.encode(incomingRaw);
+                mockRequest.url = '/_/backbeat/metadata/bucket0/key0' + '?versionId=aIXVkw5Tw2Pd00000000001I4j3QKsvf';
+
+                metadataUtils.standardMetadataValidateBucketAndObj.callsFake((_params, _denies, _log, callback) => {
+                    callback(null, bucketInfo, {});
+                });
+
+                const putObjectMDStub = sandbox
+                    .stub(metadata, 'putObjectMD')
+                    .callsFake((_bucketName, _objectKey, _omVal, options, _logParam, cb) => {
+                        assert.deepStrictEqual(options.conditions, {
+                            $or: [{ microVersionId: { $exists: false } }, { microVersionId: { $gt: incomingRaw } }],
+                        });
+                        cb(null, {});
+                    });
+
+                routeBackbeat('127.0.0.1', mockRequest, mockResponse, log);
+                void (await endPromise);
+
+                sinon.assert.called(putObjectMDStub);
+                assert.strictEqual(mockResponse.statusCode, 200);
+            },
+        );
+
+        it('should return 409 when the metadata write rejects a stale microVersionId', async () => {
+            const [incomingRaw, storedRaw] = [
+                versioning.VersionID.generateVersionId('test', 'RG001'),
+                versioning.VersionID.generateVersionId('test', 'RG001'),
+            ].sort();
+
+            mockRequest = preparePutMetadataRequest({ microVersionId: incomingRaw });
+            mockRequest.headers['x-scal-micro-version-id'] = versioning.VersionID.encode(incomingRaw);
+            mockRequest.url = '/_/backbeat/metadata/bucket0/key0' + '?versionId=aIXVkw5Tw2Pd00000000001I4j3QKsvf';
+
+            metadataUtils.standardMetadataValidateBucketAndObj.callsFake((_params, _denies, _log, callback) => {
+                callback(null, bucketInfo, { microVersionId: storedRaw });
+            });
+
+            sandbox
+                .stub(metadata, 'putObjectMD')
+                .callsFake((_bucketName, _objectKey, _omVal, _options, _logParam, cb) => {
+                    cb(errors.PreconditionFailed);
+                });
+
+            routeBackbeat('127.0.0.1', mockRequest, mockResponse, log);
+            void (await endPromise);
+
+            assert.strictEqual(mockResponse.statusCode, 409);
+        });
+
         it('should handle error when putting metadata', async () => {
             const putObjectMDStub = sandbox.stub(metadata, 'putObjectMD');
             putObjectMDStub.onCall(0).callsFake((bucketName, objectKey, omVal, options, logParam, cb) => {