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

Open
maeldonn wants to merge 3 commits into
base: development/9.4
Choose a base branch
from
+167 −7
Open

Enforce microVersionId conflicts at the metadata write level #6235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3c706fc
Bump arsenal dependency
maeldonn Jul 15, 2026
ff93206
Enforce microVersionId conflict checks at the metadata write level
maeldonn Jul 15, 2026
9b5d259
Reformat updated files with prettier
maeldonn Jul 23, 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
25 changes: 24 additions & 1 deletion lib/routes/routeBackbeat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,11 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) {
overheadField: constants.overheadField,
};

// atomic counterpart of the pre-checks above ($gt cannot match a missing microVersionId)
if (incomingMicroVersionId !== null && objMd?.microVersionId && metadata.supportsConditionalPutObjectMD?.()) {
options.conditions = { microVersionId: { $gt: incomingMicroVersionId } };

@benzekrimaha benzekrimaha Jul 24, 2026 •
edited
Loading

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.

could the condition also cover the missing-field case, e.g. $or: [{ microVersionId: { $exists: false } }, { microVersionId: { $gt: … } }]? otherwise this window stays unprotected , if translateConditions cannot express it, worth mentionning in the comment.

}

// 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,
Expand Down Expand Up @@ -1067,7 +1072,25 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) {
});
},
],
callback,
(err, results) => {
if (err?.is?.PreconditionFailed) {
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.6",
"arsenal": "git+https://github.com/scality/arsenal#fa9abec316dc0a48b9511eec5b41f7b9a626d5f4",
Comment thread
maeldonn marked this conversation as resolved.

@benzekrimaha benzekrimaha Jul 24, 2026 •
edited
Loading

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.

reminder to point to the released arsenal tag containing ARSN-619 before merging (/after_pull_request on scality/Arsenal#2678 if needed).

"async": "2.6.4",
"aws-crt": "^1.24.0",
"bucketclient": "scality/bucketclient#8.2.7",
Expand Down
137 changes: 137 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 @@ -255,12 +255,19 @@ describe('routeBackbeat', () => {
mockRequest = preparePutMetadataRequest();
dataDeleteSpy = sandbox.stub(dataWrapper.data, 'delete').callsFake((loc, log, cb) => cb(null));

// fake the capability (mem backend has none) to exercise the condition path
metadata.supportsConditionalPutObjectMD = () => true;

// Setup default version enabled bucket and empty object metadata
metadataUtils.standardMetadataValidateBucketAndObj.callsFake((params, denies, log, callback) => {
callback(null, bucketInfo, {});
});
});

afterEach(() => {
delete metadata.supportsConditionalPutObjectMD;
});

it('should allow CRR destination requests (putMetadata) when versioning is enabled', async () => {
sandbox.stub(metadata, 'putObjectMD').callsFake((bucketName, objectKey, omVal, options, logParam, cb) => {
cb(null, {});
Expand Down Expand Up @@ -358,6 +365,136 @@ 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, { 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);

@claude claude Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Only asserting statusCode is 409 doesn't distinguish this from the StaleMicroVersionIdException 409 response. Assert the body code so a regression that returns the wrong 409 type fails the test.

Suggested change
assert.strictEqual(mockResponse.statusCode, 409);
assert.strictEqual(mockResponse.statusCode, 409);
assert.strictEqual(mockResponse.body.code, 'MicroVersionIdAlreadyStoredException');

});

it('should not set a microVersionId condition when the stored revision has none', 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.strictEqual(options.conditions, undefined);
cb(null, {});
});

routeBackbeat('127.0.0.1', mockRequest, mockResponse, log);
void (await endPromise);

sinon.assert.called(putObjectMDStub);
assert.strictEqual(mockResponse.statusCode, 200);
});

it('should not set a microVersionId condition when the backend does not support it', async () => {
const [incomingRaw, storedRaw] = [
versioning.VersionID.generateVersionId('test', 'RG001'),
versioning.VersionID.generateVersionId('test', 'RG001'),
].sort();

delete metadata.supportsConditionalPutObjectMD;

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.strictEqual(options.conditions, undefined);
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);

@claude claude Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same here — assert the body code so this test doesn't pass if a different 409 path fires.

Suggested change
assert.strictEqual(mockResponse.statusCode, 409);
assert.strictEqual(mockResponse.statusCode, 409);
assert.strictEqual(mockResponse.body.code, 'StaleMicroVersionIdException');

});

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
10 changes: 5 additions & 5 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
"@aws-sdk/types" "^3.222.0"
tslib "^1.11.1"

"@aws-crypto/crc32@5.2.0", "@aws-crypto/crc32@^5.2.0":
"@aws-crypto/crc32@5.2.0":
version "5.2.0"
resolved "https://registry.yarnpkg.com/@aws-crypto/crc32/-/crc32-5.2.0.tgz#cfcc22570949c98c6689cfcbd2d693d36cdae2e1"
integrity sha512-nLbCWqQNgUiwwtFsen1AdzAtvuLRsQS8rYgMuxCrdKf9kOssamGLuPwyTY9wyYblNr9+1XM8v6zoDTPPSIeANg==
Expand All @@ -28,7 +28,7 @@
"@aws-sdk/types" "^3.222.0"
tslib "^2.6.2"

"@aws-crypto/crc32c@5.2.0", "@aws-crypto/crc32c@^5.2.0":
"@aws-crypto/crc32c@5.2.0":
version "5.2.0"
resolved "https://registry.yarnpkg.com/@aws-crypto/crc32c/-/crc32c-5.2.0.tgz#4e34aab7f419307821509a98b9b08e84e0c1917e"
integrity sha512-+iWb8qaHLYKrNvGRbiYRHSdKRWhto5XlZUEBwDjYNf+ly5SVYG6zEoYIdxvf5R3zyeP16w4PLBn3rH1xc74Rag==
Expand Down Expand Up @@ -6596,9 +6596,9 @@ arraybuffer.prototype.slice@^1.0.4:
optionalDependencies:
ioctl "^2.0.2"

"arsenal@git+https://github.com/scality/arsenal#8.5.6":
version "8.5.6"
resolved "git+https://github.com/scality/arsenal#0db557930c7d13204167188a7503e6e00d154df5"
"arsenal@git+https://github.com/scality/arsenal#fa9abec316dc0a48b9511eec5b41f7b9a626d5f4":
version "8.5.11"
resolved "git+https://github.com/scality/arsenal#fa9abec316dc0a48b9511eec5b41f7b9a626d5f4"
dependencies:
"@aws-sdk/client-kms" "^3.975.0"
"@aws-sdk/client-s3" "^3.975.0"
Expand Down
Loading