Skip to content

AbortMPU cleanup - revisited #5854

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

AbortMPU cleanup - revisited #5854

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
9d3829d
AbortMPU to properly cleanup existing s3 object
williamlardier Jul 1, 2025
6d352b5
Add unit tests for abortMPU
williamlardier Jul 1, 2025
f70c06a
Add unit and functional tests
williamlardier Jul 1, 2025
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
162 changes: 131 additions & 31 deletions lib/api/apiUtils/object/abortMultipartUpload.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
const { validateQuotas } = require('../quotas/quotaUtils');
const services = require('../../../services');
const metadata = require('../../../metadata/wrapper');
const { versioning } = require('arsenal');

function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log,
callback, request) {
Expand Down Expand Up @@ -61,61 +62,158 @@
});
},
function abortExternalMpu(mpuBucket, mpuOverviewObj, destBucket, objectMD,
next) {
next) {
const location = mpuOverviewObj.controllingLocationConstraint;
const originalIdentityAuthzResults = request.actionImplicitDenies;
// eslint-disable-next-line no-param-reassign
delete request.actionImplicitDenies;
return data.abortMPU(objectKey, uploadId, location, bucketName,
request, destBucket, locationConstraintCheck, log,
(err, skipDataDelete) => {
// eslint-disable-next-line no-param-reassign
request.actionImplicitDenies = originalIdentityAuthzResults;
if (err) {
log.error('error aborting MPU', { error: err });
return next(err, destBucket);
}
// for Azure and GCP we do not need to delete data
// for all other backends, skipDataDelete will be set to false
return next(null, mpuBucket, destBucket, objectMD, skipDataDelete);
});
request, destBucket, locationConstraintCheck, log,
(err, skipDataDelete) => {
// eslint-disable-next-line no-param-reassign
request.actionImplicitDenies = originalIdentityAuthzResults;
if (err) {
log.error('error aborting MPU', { error: err });
return next(err, destBucket);
}
// for Azure and GCP we do not need to delete data
// for all other backends, skipDataDelete will be set to false
return next(null, mpuBucket, mpuOverviewObj, destBucket, objectMD, skipDataDelete);
});
},
function getPartLocations(mpuBucket, destBucket, objectMD, skipDataDelete,
next) {
function getPartLocations(mpuBucket, mpuOverviewObj, destBucket, objectMD, skipDataDelete,
next) {
services.getMPUparts(mpuBucket.getName(), uploadId, log,
(err, result) => {
if (err) {
log.error('error getting parts', { error: err });
return next(err, destBucket);
}
const storedParts = result.Contents;
return next(null, mpuBucket, storedParts, destBucket, objectMD,
skipDataDelete);
return next(null, mpuBucket, mpuOverviewObj, storedParts, destBucket, objectMD,
skipDataDelete);
});
},
function deleteObjectMetadata(mpuBucket, storedParts, destBucket, objectMD, skipDataDelete, next) {
if (!objectMD || metadataValMPUparams.uploadId !== objectMD.uploadId) {
// During Abort, we dynamically detect if the previous CompleteMPU call
// created potential object metadata wrongly, e.g. by creating
// an object version when some of the parts are missing.
// By passing a null objectMD, we tell the subsequent steps
// to skip the cleanup.
// Another approach is possible, but not supported by all backends:
// to honor the uploadId filter in standardMetadataValidateBucketAndObj
// ensuring the objMD returned has the right uploadId. But this is not
// supported by Metadata.
function ensureCleanupIsRequired(mpuBucket, mpuOverviewObj, storedParts, destBucket,
objectMD, skipDataDelete, next) {
// If no overview object, skip - this wasn't a failed completeMPU
if (!mpuOverviewObj) {
return next(null, mpuBucket, storedParts, destBucket, null, skipDataDelete);

Check warning on line 110 in lib/api/apiUtils/object/abortMultipartUpload.js

View check run for this annotation

Codecov / codecov/patch

lib/api/apiUtils/object/abortMultipartUpload.js#L110 

Added line #L110 was not covered by tests
}

// If objectMD exists and has matching uploadId, use it directly
// This handles all non-versioned cases, and some versioned cases.
if (objectMD && objectMD.uploadId === uploadId) {
return next(null, mpuBucket, storedParts, destBucket, objectMD, skipDataDelete);
}
// In case there has been an error during cleanup after a complete MPU
// (e.g. failure to delete MPU MD in shadow bucket),
// we need to ensure that the MPU metadata is deleted.
log.debug('Object has existing metadata, deleting them', {

// If bucket is not versioned, no need to check versions:
// as the uploadId is not the same, we skip the cleanup.
if (!destBucket.isVersioningEnabled()) {
return next(null, mpuBucket, storedParts, destBucket, null, skipDataDelete);
}

// Otherwise, we list all versions of the object. We try to stop as early
// as possible, by using pagination.
let keyMarker = null;
let versionIdMarker = null;
let foundVersion = null;
let shouldContinue = true;

return async.whilst(
() => shouldContinue && !foundVersion,
callback => {
const listParams = {
listingType: 'DelimiterVersions',
// To only list the specific key, we need to add the versionId separator
prefix: `${objectKey}${versioning.VersioningConstants.VersionId.Separator}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will need further tests to ensure this works file - we do not officially have a way to list all versions of just one object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test done, it's working well. Example:

{
    "CommonPrefixes": [],
    "Versions": [
        {
            "key": "bug-test-bda30555",
            "value": {
                "Size": 1048576,
                "ETag": "3eaf2c9512f76a55da265ad51ac21393-1",
                "VersionId": "98248542529027999999RG00001",
                "IsDeleteMarker": false,
                "LastModified": "2025-07-02T11:57:50.972Z",
                "Owner": {
                    "DisplayName": "acc",
                    "ID": "5874c85ff66d0ede3bd6e8fe9610f0640d1700a4607f8b7e414503fbd2cb03de"
                },
                "StorageClass": "STANDARD"
            }
        },
        {
            "key": "bug-test-bda30555",
            "value": {
                "Size": 1048576,
                "ETag": "3eaf2c9512f76a55da265ad51ac21393-1",
                "VersionId": "98248542540139999999RG00001",
                "IsDeleteMarker": false,
                "LastModified": "2025-07-02T11:57:39.857Z",
                "Owner": {
                    "DisplayName": "acc",
                    "ID": "5874c85ff66d0ede3bd6e8fe9610f0640d1700a4607f8b7e414503fbd2cb03de"
                },
                "StorageClass": "STANDARD"
            }
        }
    ],
    "IsTruncated": false
}

(while the same key with suffix exists)

maxKeys: 1000,
};

if (keyMarker) {
listParams.keyMarker = keyMarker;
}
if (versionIdMarker) {
listParams.versionIdMarker = versionIdMarker;
}

return services.getObjectListing(bucketName, listParams, log, (err, listResponse) => {
if (err) {
log.error('error listing object versions', { error: err });
return callback(err);
}

// Check each version in current batch for matching uploadId
const matchedVersion = (listResponse.Versions || []).find(version =>
version.key === objectKey &&
version.value &&
version.value.uploadId === uploadId
);

if (matchedVersion) {
foundVersion = matchedVersion.value;
}

// Set up for next iteration if needed
if (listResponse.IsTruncated && !foundVersion) {
keyMarker = listResponse.NextKeyMarker;
versionIdMarker = listResponse.NextVersionIdMarker;
} else {
shouldContinue = false;
}

return callback();
});
},
err => next(null, mpuBucket, storedParts, destBucket, err ? null : foundVersion, skipDataDelete)
);
},
function deleteObjectMetadata(mpuBucket, storedParts, destBucket, objMDWithMatchingUploadID,
skipDataDelete, next) {
// If no objMDWithMatchingUploadID, nothing to delete
if (!objMDWithMatchingUploadID) {
return next(null, mpuBucket, storedParts, destBucket, objMDWithMatchingUploadID, skipDataDelete);
}

log.debug('Object has existing metadata with matching uploadId, deleting them', {
method: 'abortMultipartUpload',
bucketName,
objectKey,
uploadId,
versionId: objectMD.versionId,
versionId: objMDWithMatchingUploadID.versionId,
});
return metadata.deleteObjectMD(bucketName, objectKey, { versionId: objectMD.versionId }, log, err => {
return metadata.deleteObjectMD(bucketName, objectKey, {
versionId: objMDWithMatchingUploadID.versionId,
}, log, err => {
if (err) {
log.error('error deleting object metadata', { error: err });
// Handle concurrent deletion of this object metadata
if (err.is?.NoSuchKey) {
log.debug('object metadata already deleted or does not exist', {
method: 'abortMultipartUpload',
bucketName,
objectKey,
versionId: objMDWithMatchingUploadID.versionId,
});
} else {
log.error('error deleting object metadata', { error: err });
}
}
return next(err, mpuBucket, storedParts, destBucket, objectMD, skipDataDelete);
// Continue with the operation regardless of deletion success/failure
// The important part is that we tried to clean up
return next(null, mpuBucket, storedParts, destBucket, objMDWithMatchingUploadID, skipDataDelete);
});
},
function deleteData(mpuBucket, storedParts, destBucket, objectMD,
skipDataDelete, next) {
function deleteData(mpuBucket, storedParts, destBucket, objMDWithMatchingUploadID,
skipDataDelete, next) {
if (skipDataDelete) {
return next(null, mpuBucket, storedParts, destBucket);
}
Expand All @@ -126,9 +224,11 @@
return next(null, mpuBucket, storedParts, destBucket);
}

if (objectMD && objectMD.location && objectMD.uploadId === metadataValMPUparams.uploadId) {
// Add object data locations if they exist and we can trust uploadId matches
if (objMDWithMatchingUploadID && objMDWithMatchingUploadID.location) {
const existingLocations = new Set(locations.map(loc => loc.key));
const remainingObjectLocations = objectMD.location.filter(loc => !existingLocations.has(loc.key));
const remainingObjectLocations = objMDWithMatchingUploadID.
location.filter(loc => !existingLocations.has(loc.key));
locations.push(...remainingObjectLocations);
}

Expand Down
Loading
Loading