Properly return already exists error by benzekrimaha · Pull Request #2672 · scality/Arsenal

benzekrimaha · 2026-07-09T15:08:56Z

Context

Concurrent MongoDB createBucket calls could silently behave as successful
idempotent writes. This prevented Cloudserver from distinguishing the winning
request from requests that lost the race and could allow a loser to overwrite
the winner's metadata.

This change restores an explicit BucketAlreadyExists contract matching the
other metadata backends. It must be deployed with
Cloudserver #6222, which
handles this result in normal and MPU shadow-bucket creation paths.

Changes

Use $setOnInsert for the metastore upsert so duplicate requests never
overwrite existing bucket metadata.
Return BucketAlreadyExists when the atomic upsert matches an existing
metastore entry.
Treat BucketAlreadyExists as success while initializing usersBucket.
Treat NamespaceExists as success after a new metastore insert because an
existing backing collection does not imply an existing bucket.
Continue sharding after NamespaceExists and treat AlreadyInitialized as
success.
Keep callbacks outside Promise rejection handling so a callback exception
cannot trigger a second callback.
Return the Promise chain so callback exceptions propagate to the caller.

Issue: ARSN-613

bert-e · 2026-07-09T15:09:01Z

Hello benzekrimaha,

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-09T15:09:06Z

Incorrect fix version

The Fix Version/s in issue ARSN-613 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

8.5.9

Please check the Fix Version/s of ARSN-613, or the target
branch of this pull request.

codecov · 2026-07-09T16:09:54Z

Codecov Report

❌ Patch coverage is 91.66667% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.42%. Comparing base (059ecf4) to head (5060143).
⚠️ Report is 11 commits behind head on development/8.4.

Files with missing lines	Patch %	Lines
...orage/metadata/mongoclient/MongoClientInterface.ts	91.66%	3 Missing ⚠️

Additional details and impacted files

@@                 Coverage Diff                 @@
##           development/8.4    #2672      +/-   ##
===================================================
+ Coverage            74.40%   74.42%   +0.02%     
===================================================
  Files                  229      229              
  Lines                18533    18536       +3     
  Branches              3814     3816       +2     
===================================================
+ Hits                 13790    13796       +6     
+ Misses                4738     4735       -3     
  Partials                 5        5

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bert-e · 2026-07-10T05:57:36Z

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

Issue: ARSN-613

delthas

LGTM code-wise, but didnt go far into the whole issue. Let's wait for Francois approve before merging.

francoisferrand · 2026-07-19T20:31:54Z

+                                log.debug('createBucket: collection already sharded', { bucketName });
+                            })
+                            .then(() => cb(null))
+                            .catch(err => {


not correct: this would incorrectly catch an exception thrown by cb

(not clear if this was prevent before, with the refactoring...)

The callback is now invoked from the terminal two-argument .then(success, failure), so an exception thrown by cb is not caught and converted into a second callback. createBucket also returns the Promise chain. I also added a regression test asserting the callback is called once when it throws.

francoisferrand · 2026-07-19T20:39:27Z

+                    const finishAfterCollectionReady = () => {
+                        if (!this.shardCollections) {
+                            return cb(null);
+                        }
+                        const cmd = {
+                            shardCollection: `${this.database}.${bucketName}`,
+                            key: { _id: 1 },
+                        };
+                        return this.adminDb!.command(cmd, {})
+                            .catch(err => {
+                                // Concurrent createBucket calls may race on
+                                // shardCollection after a successful
+                                // createCollection.
+                                if (err.codeName !== 'AlreadyInitialized') {
+                                    throw err;
+                                }
+                                log.debug('createBucket: collection already sharded', { bucketName });
+                            })
+                            .then(() => cb(null))
+                            .catch(err => {
+                                log.error('createBucket: enabling sharding', { error: err });
+                                return cb(errors.InternalError);
+                            });
+                    };
+
                    return this.db!.createCollection(bucketName)
+                        .then(() => finishAfterCollectionReady())
                        .catch(err => {
-                            // MongoDB returns NamespaceExists (code 48) when
-                            // the collection already exists, e.g. on
-                            // concurrent create/drop/create sequences on MPU
-                            // shadow buckets. The collection being there is
-                            // the desired outcome, so treat it as success:
-                            // this mirrors deleteBucket, which ignores
-                            // NamespaceNotFound when dropping the collection.
-                            if (err.codeName !== 'NamespaceExists') {
-                                throw err;
-                            }
-                            log.debug('createBucket: collection already exists', { bucketName });
-                        })
-                        .then(() => {
-                            if (this.shardCollections) {
-                                const cmd = {
-                                    shardCollection: `${this.database}.${bucketName}`,
-                                    key: { _id: 1 },
-                                };
-                                return this.adminDb!.command(cmd, {})
-                                    .catch(err => {
-                                        // Concurrent createBucket calls may
-                                        // race on shardCollection: sharding
-                                        // an already-sharded collection fails
-                                        // with AlreadyInitialized. The
-                                        // collection is sharded (with the
-                                        // same {_id: 1} key) either way.
-                                        if (err.codeName !== 'AlreadyInitialized') {
-                                            throw err;
-                                        }
-                                        log.debug('createBucket: collection already sharded', { bucketName });
-                                    })
-                                    .then(() => cb(null))
-                                    .catch(err => {
-                                        log.error('createBucket: enabling sharding', { error: err });
-                                        return cb(errors.InternalError);
-                                    });
+                            if (err.codeName === 'NamespaceExists') {
+                                // Metastore insert succeeded: an orphaned backing
+                                // collection is not a bucket-level duplicate.
+                                log.debug('createBucket: collection already exists', { bucketName });
+                                return finishAfterCollectionReady();


not clear what this changes: seems to do the same thing, less readable because of the callback preventing from reading sequentially...

Removed finishAfterCollectionReady and restored the sequential createCollection → optional shardCollection flow. NamespaceExists and AlreadyInitialized are handled directly at their respective operation boundaries.

Keep callbacks outside Promise rejection handling and return the operation chain so callback exceptions propagate without being converted into a second callback. Issue: ARSN-613

Use the next unreleased 8.4 patch version required by Bert-E. Issue: ARSN-613

benzekrimaha changed the title ~~Improvement/arsn 613 return already exists error~~ Return already exists error Jul 9, 2026

benzekrimaha force-pushed the improvement/ARSN-613-return-alreadyExistsError branch from 405953e to dff71c5 Compare July 9, 2026 15:17

benzekrimaha changed the base branch from development/8.5 to development/8.4 July 9, 2026 15:17

benzekrimaha requested review from a team, delthas, francoisferrand and maeldonn July 9, 2026 15:20

benzekrimaha mentioned this pull request Jul 9, 2026

Handle BucketAlreadyExists races and bump arsenal for MPU afterEach fix scality/cloudserver#6222

Open

maeldonn approved these changes Jul 9, 2026

View reviewed changes

Comment thread tests/unit/storage/metadata/mongoclient/MongoClientInterface.spec.js Outdated

benzekrimaha changed the title ~~Return already exists error~~ Properly return already exists error Jul 9, 2026

benzekrimaha force-pushed the improvement/ARSN-613-return-alreadyExistsError branch 3 times, most recently from 94f6b1f to 0bd1b04 Compare July 9, 2026 16:37

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

francoisferrand requested changes Jul 10, 2026

View reviewed changes

Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts Outdated

benzekrimaha force-pushed the improvement/ARSN-613-return-alreadyExistsError branch from 0bd1b04 to db2b9c2 Compare July 10, 2026 13:25

benzekrimaha requested a review from francoisferrand July 10, 2026 13:27

claude Bot reviewed Jul 10, 2026

View reviewed changes

Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts

francoisferrand requested a review from maeldonn July 10, 2026 13:54

claude Bot reviewed Jul 10, 2026

View reviewed changes

Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts Outdated

benzekrimaha added 6 commits July 13, 2026 14:29

Return BucketAlreadyExists from Mongo createBucket on duplicate.

ea33f56

Issue: ARSN-613

Update createBucket stub tests for BucketAlreadyExists contract.

14c8bf0

Issue: ARSN-613

Handle BucketAlreadyExists in usersBucketHack.

b134a51

Issue: ARSN-613

Update mongodb-memory-server createBucket regression tests.

4159e1b

Issue: ARSN-613

Use setOnInsert in createBucket metastore upsert.

a8268f4

Issue: ARSN-613

Treat NamespaceExists as success after a new metastore insert.

55584be

Issue: ARSN-613

Bump package.json to 8.4.20.

1e8bfe1

Issue: ARSN-613

benzekrimaha force-pushed the improvement/ARSN-613-return-alreadyExistsError branch from 07aa045 to 1e8bfe1 Compare July 13, 2026 13:04

delthas approved these changes Jul 15, 2026

View reviewed changes

maeldonn approved these changes Jul 15, 2026

View reviewed changes

francoisferrand reviewed Jul 19, 2026

View reviewed changes

benzekrimaha added 2 commits July 20, 2026 10:47

Prevent createBucket callbacks from being invoked twice

fd15922

Keep callbacks outside Promise rejection handling and return the operation chain so callback exceptions propagate without being converted into a second callback. Issue: ARSN-613

Bump package version to 8.4.21

5060143

Use the next unreleased 8.4 patch version required by Bert-E. Issue: ARSN-613

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Properly return already exists error#2672

Properly return already exists error#2672
benzekrimaha wants to merge 9 commits into
development/8.4from
improvement/ARSN-613-return-alreadyExistsError

benzekrimaha commented Jul 9, 2026 •

edited

Loading

Uh oh!

bert-e commented Jul 9, 2026

Uh oh!

bert-e commented Jul 9, 2026

Uh oh!

Uh oh!

codecov Bot commented Jul 9, 2026 •

edited

Loading

Uh oh!

bert-e commented Jul 10, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

delthas left a comment

Uh oh!

Uh oh!

francoisferrand Jul 19, 2026

Uh oh!

benzekrimaha Jul 20, 2026

Uh oh!

francoisferrand Jul 19, 2026

Uh oh!

benzekrimaha Jul 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

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

Uh oh!

Context

Changes

Uh oh!

bert-e commented Jul 9, 2026

Hello benzekrimaha,

Uh oh!

bert-e commented Jul 9, 2026

Incorrect fix version

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

bert-e commented Jul 10, 2026

Request integration branches

Uh oh!

Uh oh!

Uh oh!

Uh oh!

delthas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

francoisferrand Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

benzekrimaha Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

francoisferrand Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

benzekrimaha Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benzekrimaha commented Jul 9, 2026 •

edited

Loading

codecov Bot commented Jul 9, 2026 •

edited

Loading