Skip to content

Enforce microVersionId conflicts at the metadata write level #6235

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bert-e merged 4 commits into from
Aug 14, 2026
+656 −1,224
Merged

Enforce microVersionId conflicts at the metadata write level #6235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bd0e97d
Bump arsenal dependency
maeldonn Jul 15, 2026
6b472d3
Enforce microVersionId conflict checks at the metadata write level
maeldonn Jul 15, 2026
cfd7087
rename option into metadataOptions
SylvainSenechal Jul 29, 2026
7a82ba5
run prettier
SylvainSenechal Jul 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 37 additions & 13 deletions lib/routes/routeBackbeat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -791,25 +801,21 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) {
omVal.replicationInfo.isNFS = !omVal.replicationInfo.isReplica;
}

@francoisferrand francoisferrand Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could we not have kept options here?

less changes, more locality... and either way we need to "share" a variable from the main function to the continuation callback, so moving does not really help...

		const options = {
            overheadField: constants.overheadField,
        };

		// Atomic counterpart of the JS pre-checks above, performed at database level.
        if (incomingMicroVersionId !== null) {
            options.conditions = {
                $or: [{ microVersionId: { $exists: false } }, { microVersionId: { $gt: incomingMicroVersionId } }],
            };
        }

@SylvainSenechal SylvainSenechal Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its a bit weird to have re add an isolated
if (incomingMicroVersionId !== null)
later in the code when we already have it at the top of the function 🤔

@francoisferrand francoisferrand Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to me feels to me much weirder (and harder to read) to spread the construction of the options...
incomingMicroVersionId is kind of like a parameter, so no problem using it throughout the function

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) {
Expand All @@ -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'];
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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'));
Expand Down Expand Up @@ -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);
},
);
});
}
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
45 changes: 45 additions & 0 deletions tests/functional/backbeat/putMetadata.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
109 changes: 109 additions & 0 deletions tests/unit/routes/routeBackbeat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment thread
SylvainSenechal marked this conversation as resolved.
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);
Comment thread
SylvainSenechal marked this conversation as resolved.
});

it('should handle error when putting metadata', async () => {
const putObjectMDStub = sandbox.stub(metadata, 'putObjectMD');
putObjectMDStub.onCall(0).callsFake((bucketName, objectKey, omVal, options, logParam, cb) => {
Expand Down
Loading
Loading