Enforce microVersionId conflicts at the metadata write level by maeldonn · Pull Request #6235 · scality/cloudserver

maeldonn · 2026-07-23T14:04:00Z

The JS pre-checks in putMetadata are not atomic: two concurrent CRR writes can
both pass them and let a stale revision overwrite a newer one. This attaches a
conditional put ($gt on microVersionId) so the write itself rejects stale
revisions with PreconditionFailed, mapped to the same conflict response as the
pre-checks. Enabled only on backends supporting conditional puts (mongodb).

Issue: CLDSRV-952

bert-e · 2026-07-23T14:04:04Z

Hello maeldonn,

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-07-23T14:04:40Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

codecov · 2026-07-23T14:09:25Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.34%. Comparing base (7683a6a) to head (886ca0b).
✅ All tests successful. No failed tests found.

Additional details and impacted files

Files with missing lines	Coverage Δ
lib/routes/routeBackbeat.js	`78.69% <100.00%> (+0.30%)`	⬆️

... and 1 file with indirect coverage changes

@@                 Coverage Diff                 @@
##           development/9.4    #6235      +/-   ##
===================================================
- Coverage            86.38%   86.34%   -0.04%     
===================================================
  Files                  212      212              
  Lines                14567    14574       +7     
===================================================
+ Hits                 12583    12584       +1     
- Misses                1984     1990       +6

Flag	Coverage Δ
checksums-disabled-tests	`35.32% <0.00%> (-0.02%)`	⬇️
file-ft-tests	`69.98% <60.00%> (-0.02%)`	⬇️
file-ft-tests-null-compat	`70.46% <53.33%> (-0.05%)`	⬇️
kmip-ft-tests	`28.07% <0.00%> (-0.02%)`	⬇️
mongo-v0-ft-tests	`71.07% <53.33%> (-0.03%)`	⬇️
mongo-v1-ft-tests	`71.07% <53.33%> (-0.02%)`	⬇️
multiple-backend	`36.08% <53.33%> (-0.01%)`	⬇️
s3c-ft-tests-v0	`65.06% <73.33%> (+<0.01%)`	⬆️
s3c-ft-tests-v0-null-compat	`65.11% <66.66%> (+<0.01%)`	⬆️
s3c-ft-tests-v1	`65.04% <73.33%> (+<0.01%)`	⬆️
sur-tests	`36.44% <0.00%> (-0.02%)`	⬇️
sur-tests-inflights	`39.27% <0.00%> (-0.02%)`	⬇️
unit	`73.94% <93.33%> (+0.11%)`	⬆️
utapi-v2-tests	`35.29% <0.00%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

benzekrimaha

Logic LGTM.

benzekrimaha · 2026-07-24T15:28:39Z

        "@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",


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

benzekrimaha · 2026-07-24T15:28:39Z


+        // 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 } };


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.

Yeah, known limitation, I will rediscuss with Maël next week, but yeah we would need to update arsenal with some more logic to deal with $exist, $or which may be more ocmplex

bert-e · 2026-07-29T10:28:15Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

SylvainSenechal · 2026-07-29T11:22:34Z

        "@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#6675ce3895cb779b0d07756a373762c82ff5e335",


delthas

nit: Can you squash your 2 last commits into the main Mael commit? So that we get proper atomic commits in history.

francoisferrand · 2026-08-10T14:33:05Z

+
+        // Atomic counterpart of the JS pre-checks above, performed at database level.
+        // Only effective when the stored revision already carries a
+        // microVersionId, need $exists and $or implementation on Arsenal for the rest


should be implemented first, so we fix once in cloudserver?

Yes, added in arsenal

Issue: CLDSRV-952

The JS pre-checks in putMetadata are not atomic: two concurrent writes can both pass them and let a stale revision overwrite a newer one. Pass a conditional put to the metadata backend so the write itself rejects stale microVersionIds with PreconditionFailed, mapped to the same CRR conflict response as the pre-checks. Issue: CLDSRV-952

Issue: CLDSRV-952

SylvainSenechal · 2026-08-11T18:26:43Z

will rerun prettier later

SylvainSenechal · 2026-08-12T20:42:05Z

@francoisferrand its approved but you can retake a look if you want, most of the logic is happening in arsenal anyways

maeldonn requested review from a team, SylvainSenechal and delthas July 23, 2026 14:04

maeldonn requested review from benzekrimaha and removed request for delthas July 23, 2026 14:04

scality deleted a comment from bert-e Jul 23, 2026

claude Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread package.json Outdated

claude Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread lib/routes/routeBackbeat.js Outdated

maeldonn changed the title ~~Enforce microVersionId conflict checks at the metadata write level~~ Enforce microVersionId conflicts at the metadata write level Jul 23, 2026

maeldonn force-pushed the improvement/CLDSRV-952 branch from b85feb8 to ff93206 Compare July 23, 2026 14:30

maeldonn marked this pull request as ready for review July 23, 2026 14:30

claude Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread tests/unit/routes/routeBackbeat.js

claude Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread tests/unit/routes/routeBackbeat.js

benzekrimaha requested changes Jul 24, 2026

View reviewed changes

benzekrimaha self-requested a review July 24, 2026 15:36

benzekrimaha approved these changes Jul 24, 2026

View reviewed changes

SylvainSenechal force-pushed the improvement/CLDSRV-952 branch from 9b5d259 to c43d014 Compare July 29, 2026 10:28

scality deleted a comment from bert-e Jul 29, 2026

SylvainSenechal force-pushed the improvement/CLDSRV-952 branch 2 times, most recently from af5972a to 3ca7d7e Compare July 29, 2026 11:15

SylvainSenechal requested a review from delthas July 29, 2026 11:22

SylvainSenechal reviewed Jul 29, 2026

View reviewed changes

claude Bot reviewed Jul 29, 2026

View reviewed changes

Comment thread lib/routes/routeBackbeat.js Outdated

SylvainSenechal force-pushed the improvement/CLDSRV-952 branch from 22062b7 to bfcf42b Compare July 29, 2026 11:28

delthas approved these changes Jul 30, 2026

View reviewed changes

francoisferrand reviewed Aug 10, 2026

View reviewed changes

Comment thread lib/routes/routeBackbeat.js Outdated

francoisferrand reviewed Aug 10, 2026

View reviewed changes

SylvainSenechal force-pushed the improvement/CLDSRV-952 branch 2 times, most recently from 18e0975 to b2ae192 Compare August 11, 2026 17:24

scality deleted a comment from bert-e Aug 11, 2026

maeldonn and others added 4 commits August 11, 2026 19:54

Bump arsenal dependency

5518287

Issue: CLDSRV-952

rename option into metadataOptions

3c39380

Issue: CLDSRV-952

run prettier

886ca0b

Issue: CLDSRV-952

SylvainSenechal force-pushed the improvement/CLDSRV-952 branch from b2ae192 to 886ca0b Compare August 11, 2026 17:55

SylvainSenechal requested a review from francoisferrand August 12, 2026 20:41

Uh oh!

Conversation

maeldonn commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bert-e commented Jul 23, 2026

Hello maeldonn,

Uh oh!

bert-e commented Jul 23, 2026

Waiting for approval

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

benzekrimaha left a comment • 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!

benzekrimaha Jul 24, 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!

benzekrimaha Jul 24, 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!

SylvainSenechal Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

bert-e commented Jul 29, 2026

Waiting for approval

Uh oh!

SylvainSenechal Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

delthas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

francoisferrand Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

SylvainSenechal Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

SylvainSenechal commented Aug 11, 2026

Uh oh!

SylvainSenechal commented Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

maeldonn commented Jul 23, 2026 •

edited

Loading

codecov Bot commented Jul 23, 2026 •

edited

Loading

benzekrimaha left a comment •

edited

Loading

benzekrimaha Jul 24, 2026 •

edited

Loading

benzekrimaha Jul 24, 2026 •

edited

Loading