Skip to content

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

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
benzekrimaha wants to merge 5 commits into
base: development/9.3
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3598f59
Handle BucketAlreadyExists races in cloudserver callers.
benzekrimaha Jul 13, 2026
41d9765
Add unit tests for BucketAlreadyExists race handling.
benzekrimaha Jul 13, 2026
da31c7c
Bump arsenal to ARSN-613 at 07aa045b for paired testing.
benzekrimaha Jul 13, 2026
c478b7c
Prettify files touched by PR.
benzekrimaha Jul 13, 2026
84a0d65
Avoid updateBucket on BucketAlreadyExists create races.
benzekrimaha Jul 13, 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
303 changes: 159 additions & 144 deletions lib/api/apiUtils/bucket/bucketCreation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
const zenkoSeparator = constants.zenkoSeparator;
const userBucketOwner = 'admin';


function addToUsersBucket(canonicalID, bucketName, bucketMD, log, cb) {
// BACKWARD: Simplify once do not have to deal with old
// usersbucket name and old splitter
Expand All @@ -28,8 +27,7 @@
if (err && !err.is.NoSuchBucket && !err.is.BucketAlreadyExists) {
return cb(err);
}
const splitter = usersBucketAttrs ?
constants.splitter : constants.oldSplitter;
const splitter = usersBucketAttrs ? constants.splitter : constants.oldSplitter;
let key = createKeyForUserBucket(canonicalID, splitter, bucketName);
const omVal = {
creationDate: new Date().toJSON(),
Expand All @@ -38,46 +36,44 @@
// If the new format usersbucket does not exist, try to put the
// key in the old usersBucket using the old splitter.
// Otherwise put the key in the new format usersBucket
const usersBucketBeingCalled = usersBucketAttrs ?
usersBucket : oldUsersBucket;
return metadata.putObjectMD(usersBucketBeingCalled, key,
omVal, {}, log, err => {
if (err?.is?.NoSuchBucket) {
// There must be no usersBucket so createBucket
// one using the new format
log.trace('users bucket does not exist, ' +
'creating users bucket');
key = `${canonicalID}${constants.splitter}` +
`${bucketName}`;
const creationDate = new Date().toJSON();
const freshBucket = new BucketInfo(usersBucket,
userBucketOwner, userBucketOwner, creationDate,
BucketInfo.currentModelVersion());
return metadata.createBucket(usersBucket,
freshBucket, log, err => {
// Note: In the event that two
// users' requests try to create the
// usersBucket at the same time,
// this will prevent one of the users
// from getting a BucketAlreadyExists
// error with respect
// to the usersBucket.
// TODO: move to `.is` once BKTCLT-9 is done and bumped in Cloudserver
if (err && !err.BucketAlreadyExists) {
log.error('error from metadata', {
error: err,
});
return cb(err);
}
log.trace('Users bucket created');
// Finally put the key in the new format
// usersBucket
return metadata.putObjectMD(usersBucket,
key, omVal, {}, log, cb);
const usersBucketBeingCalled = usersBucketAttrs ? usersBucket : oldUsersBucket;
return metadata.putObjectMD(usersBucketBeingCalled, key, omVal, {}, log, err => {
if (err?.is?.NoSuchBucket) {
// There must be no usersBucket so createBucket
// one using the new format
log.trace('users bucket does not exist, ' + 'creating users bucket');
key = `${canonicalID}${constants.splitter}` + `${bucketName}`;
const creationDate = new Date().toJSON();
const freshBucket = new BucketInfo(
usersBucket,
userBucketOwner,
userBucketOwner,
creationDate,
BucketInfo.currentModelVersion(),
);
return metadata.createBucket(usersBucket, freshBucket, log, err => {
// Note: In the event that two
// users' requests try to create the
// usersBucket at the same time,
// this will prevent one of the users
// from getting a BucketAlreadyExists
// error with respect
// to the usersBucket.
// TODO: move to `.is` once BKTCLT-9 is done and bumped in Cloudserver
if (err && !err.BucketAlreadyExists) {
log.error('error from metadata', {
error: err,
});
}
return cb(err);
});
return cb(err);
}
log.trace('Users bucket created');
// Finally put the key in the new format
// usersBucket
return metadata.putObjectMD(usersBucket, key, omVal, {}, log, cb);
});
}
return cb(err);
});
});
}

Expand All @@ -92,17 +88,18 @@
function freshStartCreateBucket(bucket, canonicalID, log, callback) {
const bucketName = bucket.getName();
metadata.createBucket(bucketName, bucket, log, err => {
if (err) {
if (err && !err?.is?.BucketAlreadyExists) {
log.debug('error from metadata', { error: err });
return callback(err);
}
if (err?.is?.BucketAlreadyExists) {
log.trace('bucket already exists in metadata');
// Ensure users-bucket listing only; do not updateBucket with our
// local BucketInfo when a concurrent request won createBucket.
return addToUsersBucket(canonicalID, bucketName, bucket, log, callback);

@SylvainSenechal SylvainSenechal Jul 15, 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.

I don't have much knowledge of this, but isn't it problematic to run addToUsersBucket when the bucket already exists ? or is it idempotent or something ? But even then, isn't addToUsersBucket already called when the bucket was created ?

}
log.trace('created bucket in metadata');
return addToUsersBucket(canonicalID, bucketName, bucket, log, err => {
if (err) {
return callback(err);
}
return removeTransientOrDeletedLabel(bucket, log, callback);
});
return cleanUpBucket(bucket, canonicalID, log, callback);

@SylvainSenechal SylvainSenechal Jul 15, 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.

I don't really know this functions but yeah it's hard to understand what's happening here, why are we dropping removeTransientOrDeletedLabel and using cleanUpBucket ?

});
}

Expand Down Expand Up @@ -139,16 +136,15 @@
* @callback called with (err, sseInfo: object)
*/
function bucketLevelServerSideEncryption(bucket, headers, log, cb) {
kms.bucketLevelEncryption(
bucket, headers, log, (err, sseInfo) => {
if (err) {
log.debug('error getting bucket encryption info', {
error: err,
});
return cb(err);
}
return cb(null, sseInfo);
});
kms.bucketLevelEncryption(bucket, headers, log, (err, sseInfo) => {
if (err) {
log.debug('error getting bucket encryption info', {
error: err,
});
return cb(err);
}
return cb(null, sseInfo);
});
}

/**
Expand All @@ -163,135 +159,154 @@
* @param {function} cb - callback to bucketPut
* @return {undefined}
*/
function createBucket(authInfo, bucketName, headers,
locationConstraint, log, cb) {
function createBucket(authInfo, bucketName, headers, locationConstraint, log, cb) {
// Prefer using undefined instead of null for unused properties so they are not present in metadata payload
log.trace('Creating bucket');
assert.strictEqual(typeof bucketName, 'string');
const canonicalID = authInfo.getCanonicalID();
const ownerDisplayName =
authInfo.getAccountDisplayName();
const ownerDisplayName = authInfo.getAccountDisplayName();
const creationDate = new Date().toJSON();
const isNFSEnabled = headers['x-scal-nfs-enabled'] === 'true' || undefined;
const headerObjectLock = headers['x-amz-bucket-object-lock-enabled'];
const objectLockEnabled
= headerObjectLock && headerObjectLock.toLowerCase() === 'true';
const bucket = new BucketInfo(bucketName, canonicalID, ownerDisplayName,
creationDate, BucketInfo.currentModelVersion(), undefined, undefined, undefined, undefined,
undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, isNFSEnabled,
undefined, undefined, objectLockEnabled);
const objectLockEnabled = headerObjectLock && headerObjectLock.toLowerCase() === 'true';
const bucket = new BucketInfo(
bucketName,
canonicalID,
ownerDisplayName,
creationDate,
BucketInfo.currentModelVersion(),
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
isNFSEnabled,
undefined,
undefined,
objectLockEnabled,
);
let locationConstraintVal = null;

if (locationConstraint) {
const [locationConstraintStr, ingestion] =
locationConstraint.split(zenkoSeparator);
const [locationConstraintStr, ingestion] = locationConstraint.split(zenkoSeparator);
if (locationConstraintStr) {
locationConstraintVal = locationConstraintStr;
bucket.setLocationConstraint(locationConstraintStr);
}
if (ingestion === 'ingest') {
bucket.enableIngestion();
//automatically enable versioning for ingestion buckets
bucket.setVersioningConfiguration({ Status: 'Enabled' });
}
}
if (objectLockEnabled) {
// default versioning configuration AWS sets
// when a bucket is created with object lock
const versioningConfiguration = {
Status: 'Enabled',
MfaDelete: 'Disabled',
};
bucket.setVersioningConfiguration(versioningConfiguration);
}
const parseAclParams = {
headers,
resourceType: 'bucket',
acl: bucket.acl,
log,
};
async.parallel({
prepareNewBucketMD: function prepareNewBucketMD(callback) {
acl.parseAclFromHeaders(parseAclParams, (err, parsedACL) => {
if (err) {
log.debug('error parsing acl from headers', {
error: err,
});
return callback(err);
}
bucket.setFullAcl(parsedACL);
return callback(null, bucket);
});
},
getAnyExistingBucketInfo: function getAnyExistingBucketInfo(callback) {
metadata.getBucket(bucketName, log, (err, data) => {
// TODO: move to `.is` once BKTCLT-9 is done and bumped in Cloudserver
if (err && err.NoSuchBucket) {
return callback(null, 'NoBucketYet');
}
if (err) {
return callback(err);
}
return callback(null, data);
});
async.parallel(
{
prepareNewBucketMD: function prepareNewBucketMD(callback) {
acl.parseAclFromHeaders(parseAclParams, (err, parsedACL) => {
if (err) {
log.debug('error parsing acl from headers', {
error: err,
});
return callback(err);
}
bucket.setFullAcl(parsedACL);
return callback(null, bucket);
});
},

Check notice

Code scanning / CodeQL

Callback-style function (async migration) Note

This function uses a callback parameter ('callback'). Refactor to async/await.
getAnyExistingBucketInfo: function getAnyExistingBucketInfo(callback) {
metadata.getBucket(bucketName, log, (err, data) => {
// TODO: move to `.is` once BKTCLT-9 is done and bumped in Cloudserver
if (err && err.NoSuchBucket) {
return callback(null, 'NoBucketYet');
}
if (err) {
return callback(err);
}
return callback(null, data);
});
},
},
},
// Function to run upon finishing both parallel requests
(err, results) => {
if (err) {
return cb(err);
}
const existingBucketMD = results.getAnyExistingBucketInfo;
if (existingBucketMD instanceof BucketInfo &&
existingBucketMD.getOwner() !== canonicalID &&
!isServiceAccount(canonicalID)) {
// return existingBucketMD to collect cors headers
return cb(errors.BucketAlreadyExists, existingBucketMD);
}
const newBucketMD = results.prepareNewBucketMD;
if (existingBucketMD === 'NoBucketYet') {
const bucketSseConfig = parseBucketEncryptionHeaders(headers);
// Function to run upon finishing both parallel requests
(err, results) => {
if (err) {
return cb(err);
}
const existingBucketMD = results.getAnyExistingBucketInfo;
if (
existingBucketMD instanceof BucketInfo &&
existingBucketMD.getOwner() !== canonicalID &&
!isServiceAccount(canonicalID)
) {
// return existingBucketMD to collect cors headers
return cb(errors.BucketAlreadyExists, existingBucketMD);
}
const newBucketMD = results.prepareNewBucketMD;
if (existingBucketMD === 'NoBucketYet') {
const bucketSseConfig = parseBucketEncryptionHeaders(headers);

// Apply global SSE configuration when global encryption is enabled
// and no SSE settings were specified during bucket creation.
// Bucket-specific SSE headers override the default encryption.
const sseConfig = config.globalEncryptionEnabled && !bucketSseConfig.algorithm
? {
algorithm: 'AES256',
mandatory: true,
} : bucketSseConfig;
// Apply global SSE configuration when global encryption is enabled
// and no SSE settings were specified during bucket creation.
// Bucket-specific SSE headers override the default encryption.
const sseConfig =
config.globalEncryptionEnabled && !bucketSseConfig.algorithm
? {
algorithm: 'AES256',
mandatory: true,
}
: bucketSseConfig;

return bucketLevelServerSideEncryption(
bucket, sseConfig, log,
(err, sseInfo) => {
return bucketLevelServerSideEncryption(bucket, sseConfig, log, (err, sseInfo) => {
if (err) {
return cb(err);
}
newBucketMD.setServerSideEncryption(sseInfo);
log.trace(
'new bucket without flags; adding transient label');
log.trace('new bucket without flags; adding transient label');
newBucketMD.addTransientFlag();
return freshStartCreateBucket(newBucketMD, canonicalID,
log, cb);
return freshStartCreateBucket(newBucketMD, canonicalID, log, cb);
});
}
if (existingBucketMD.hasTransientFlag() ||
existingBucketMD.hasDeletedFlag()) {
log.trace('bucket has transient flag or deleted flag. cleaning up');
return cleanUpBucket(newBucketMD, canonicalID, log, cb);
}
// If bucket already exists in non-transient and non-deleted
// state and owned by requester, then return BucketAlreadyOwnedByYou
// error unless old AWS behavior (us-east-1)
// Existing locationConstraint must have legacyAwsBehavior === true
// New locationConstraint should have legacyAwsBehavior === true
if (isLegacyAWSBehavior(locationConstraintVal) &&
isLegacyAWSBehavior(existingBucketMD.getLocationConstraint())) {
log.trace('returning 200 instead of 409 to mirror us-east-1');
return cb(null, existingBucketMD);
}
return cb(errors.BucketAlreadyOwnedByYou, existingBucketMD);
});
}
if (existingBucketMD.hasTransientFlag() || existingBucketMD.hasDeletedFlag()) {
log.trace('bucket has transient flag or deleted flag. cleaning up');
return cleanUpBucket(newBucketMD, canonicalID, log, cb);
}
// If bucket already exists in non-transient and non-deleted
// state and owned by requester, then return BucketAlreadyOwnedByYou
// error unless old AWS behavior (us-east-1)
// Existing locationConstraint must have legacyAwsBehavior === true
// New locationConstraint should have legacyAwsBehavior === true
if (
isLegacyAWSBehavior(locationConstraintVal) &&
isLegacyAWSBehavior(existingBucketMD.getLocationConstraint())
) {
log.trace('returning 200 instead of 409 to mirror us-east-1');
return cb(null, existingBucketMD);
}
return cb(errors.BucketAlreadyOwnedByYou, existingBucketMD);
},
);
}

Check notice

Code scanning / CodeQL

Callback-style function (async migration) Note

This function uses a callback parameter ('cb'). Refactor to async/await.

module.exports = {
Expand Down
Loading
Loading