From 9c2b099dff6c24d2cad310e679f0f653da5ba250 Mon Sep 17 00:00:00 2001 From: williamlardier Date: Tue, 1 Jul 2025 17:21:18 +0200 Subject: [PATCH 01/10] AbortMPU to properly cleanup existing s3 object - CompleteMPU was and is still, at the time of this commit, wrongly promoting s3 objects when completing a MPU even if the parts are not valid. - The AbortMPU cleanup logic was added as an effort to cleanup new occurences of this problem, and still handle old ones not yet aborted. - The logic currently is unable to clean up if the current master version has a different upload id: in case a new version was pushed, we end up never cleaning the ghost. - This commit introduces a dynamic listing of the versionIDs of the object. We use the delimiter versions algorithm and the versionID separator to list only the current object versions, with pagination support, and then we detect the one to cleanup. Issue: CLDSRV-669 --- .../apiUtils/object/abortMultipartUpload.js | 148 ++++++++++++++---- 1 file changed, 118 insertions(+), 30 deletions(-) diff --git a/lib/api/apiUtils/object/abortMultipartUpload.js b/lib/api/apiUtils/object/abortMultipartUpload.js index 40e1f00c5f..4c8c51aa56 100644 --- a/lib/api/apiUtils/object/abortMultipartUpload.js +++ b/lib/api/apiUtils/object/abortMultipartUpload.js @@ -8,6 +8,7 @@ const { standardMetadataValidateBucketAndObj } = 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) { @@ -61,27 +62,27 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, }); }, 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) { @@ -89,33 +90,118 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, 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); + } + + // 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}`, + 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 }); } - return next(err, mpuBucket, storedParts, destBucket, objectMD, skipDataDelete); + return next(err, 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); } @@ -126,9 +212,11 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, 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); } From 4eb2cd2cc74969054d507fc20cf53b43a897648f Mon Sep 17 00:00:00 2001 From: williamlardier Date: Tue, 1 Jul 2025 17:25:31 +0200 Subject: [PATCH 02/10] Add unit tests for abortMPU Issue: CLDSRV-669 --- .../object/abortMultipartUpload.spec.js | 366 ++++++++++++++++++ 1 file changed, 366 insertions(+) create mode 100644 tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js diff --git a/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js b/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js new file mode 100644 index 0000000000..aa3e961e65 --- /dev/null +++ b/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js @@ -0,0 +1,366 @@ +const assert = require('assert'); +const sinon = require('sinon'); +const { parseString } = require('xml2js'); +const { errors } = require('arsenal'); + +const abortMultipartUpload = require('../../../../../lib/api/apiUtils/object/abortMultipartUpload'); +const { bucketPut } = require('../../../../../lib/api/bucketPut'); +const initiateMultipartUpload = require('../../../../../lib/api/initiateMultipartUpload'); +const bucketPutVersioning = require('../../../../../lib/api/bucketPutVersioning'); +const objectPutPart = require('../../../../../lib/api/objectPutPart'); +const { data } = require('../../../../../lib/data/wrapper'); +const quotaUtils = require('../../../../../lib/api/apiUtils/quotas/quotaUtils'); +const { DummyRequestLogger, makeAuthInfo, cleanup, versioningTestUtils } = require('../../../helpers'); +const DummyRequest = require('../../../DummyRequest'); + +describe('abortMultipartUpload', () => { + const log = new DummyRequestLogger(); + const authInfo = makeAuthInfo('testCanonicalId'); + const bucketName = 'test-bucket'; + const objectKey = 'test-object'; + const postBody = Buffer.from('I am a part', 'utf8'); + + const bucketRequest = new DummyRequest({ + bucketName, + namespace: 'default', + headers: { host: `${bucketName}.s3.amazonaws.com` }, + url: '/', + }); + + const initiateRequest = new DummyRequest({ + bucketName, + namespace: 'default', + objectKey, + headers: { host: `${bucketName}.s3.amazonaws.com` }, + url: `/${objectKey}?uploads`, + actionImplicitDenies: false, + }); + + const abortRequest = new DummyRequest({ + bucketName, + namespace: 'default', + objectKey, + headers: { host: `${bucketName}.s3.amazonaws.com` }, + url: `/${objectKey}?uploadId=test-upload-id`, + query: { uploadId: 'test-upload-id' }, + apiMethods: 'multipartDelete', + actionImplicitDenies: false, + accountQuotas: {}, + }); + + const enableVersioningRequest = versioningTestUtils.createBucketPutVersioningReq(bucketName, 'Enabled'); + + let dataAbortMPUStub; + let dataDeleteStub; + let validateQuotasStub; + + beforeEach(() => { + cleanup(); + // Stub external operations that we don't want to test + dataAbortMPUStub = sinon.stub(data, 'abortMPU').callsFake((objectKey, uploadId, location, bucketName, request, destBucket, locationConstraintCheckFn, log, callback) => { + // Call the callback immediately without executing locationConstraintCheck + callback(null, false); + }); + dataDeleteStub = sinon.stub(data, 'delete').yields(); + validateQuotasStub = sinon.stub(quotaUtils, 'validateQuotas').yields(); + }); + + afterEach(() => { + sinon.restore(); + }); + + // Helper to create bucket and MPU, returns uploadId + function createBucketAndMPU(versioned, callback) { + if (versioned) { + bucketPut(authInfo, bucketRequest, log, (err) => { + if (err) return callback(err); + bucketPutVersioning(authInfo, enableVersioningRequest, log, (err) => { + if (err) return callback(err); + initiateMultipartUpload(authInfo, initiateRequest, log, (err, result) => { + if (err) return callback(err); + parseString(result, (err, json) => { + if (err) return callback(err); + const uploadId = json.InitiateMultipartUploadResult.UploadId[0]; + callback(null, uploadId); + }); + }); + }); + }); + } else { + bucketPut(authInfo, bucketRequest, log, (err) => { + if (err) return callback(err); + initiateMultipartUpload(authInfo, initiateRequest, log, (err, result) => { + if (err) return callback(err); + parseString(result, (err, json) => { + if (err) return callback(err); + const uploadId = json.InitiateMultipartUploadResult.UploadId[0]; + callback(null, uploadId); + }); + }); + }); + } + } + + describe('basic functionality', () => { + it('should successfully abort multipart upload', (done) => { + createBucketAndMPU(false, (err, uploadId) => { + assert.ifError(err); + + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + assert.strictEqual(err, null); + sinon.assert.calledOnce(dataAbortMPUStub); + done(); + }, abortRequest); + }); + }); + + it('should return error for non-existent bucket', (done) => { + abortMultipartUpload(authInfo, 'non-existent-bucket', objectKey, 'fake-upload-id', log, (err) => { + assert(err); + assert.strictEqual(err.is.NoSuchBucket, true); + done(); + }, abortRequest); + }); + + it('should return error for non-existent upload', (done) => { + bucketPut(authInfo, bucketRequest, log, (err) => { + assert.ifError(err); + + abortMultipartUpload(authInfo, bucketName, objectKey, 'fake-upload-id', log, (err) => { + assert(err); + assert.strictEqual(err.is.NoSuchUpload, true); + done(); + }, abortRequest); + }); + }); + + it('should skip data deletion when skipDataDelete is true', (done) => { + dataAbortMPUStub.restore(); + sinon.stub(data, 'abortMPU').callsFake((objectKey, uploadId, location, bucketName, request, destBucket, locationConstraintCheckFn, log, callback) => { + callback(null, true); // skipDataDelete = true + }); + + createBucketAndMPU(false, (err, uploadId) => { + assert.ifError(err); + + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + assert.strictEqual(err, null); + sinon.assert.notCalled(dataDeleteStub); + sinon.assert.notCalled(validateQuotasStub); + done(); + }, abortRequest); + }); + }); + }); + + describe('with multipart upload parts', () => { + it('should delete part data when aborting', (done) => { + createBucketAndMPU(false, (err, uploadId) => { + assert.ifError(err); + + // Add a part to the multipart upload + const md5Hash = require('crypto').createHash('md5'); + md5Hash.update(postBody); + const calculatedHash = md5Hash.digest('hex'); + + const partRequest = new DummyRequest({ + bucketName, + objectKey, + namespace: 'default', + url: `/${objectKey}?partNumber=1&uploadId=${uploadId}`, + headers: { host: `${bucketName}.s3.amazonaws.com` }, + query: { + partNumber: '1', + uploadId, + }, + calculatedHash, + }, postBody); + + objectPutPart(authInfo, partRequest, undefined, log, (err) => { + assert.ifError(err); + + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + assert.strictEqual(err, null); + sinon.assert.calledOnce(dataAbortMPUStub); + sinon.assert.called(dataDeleteStub); // Should delete part data + done(); + }, abortRequest); + }); + }); + }); + }); + + describe('versioned bucket behavior', () => { + it('should handle versioned bucket abort', (done) => { + createBucketAndMPU(true, (err, uploadId) => { + assert.ifError(err); + + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + assert.strictEqual(err, null); + sinon.assert.calledOnce(dataAbortMPUStub); + done(); + }, abortRequest); + }); + }); + }); + + describe('version listing optimization', () => { + const services = require('../../../../../lib/services'); + let getObjectListingStub; + + beforeEach(() => { + getObjectListingStub = sinon.stub(services, 'getObjectListing'); + }); + + afterEach(() => { + if (getObjectListingStub) { + getObjectListingStub.restore(); + } + }); + + it('should use optimized prefix when listing versions', (done) => { + createBucketAndMPU(true, (err, uploadId) => { + assert.ifError(err); + + // Mock version listing response - return empty to simulate no cleanup needed + getObjectListingStub.yields(null, { + Versions: [], + IsTruncated: false + }); + + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + assert.strictEqual(err, null); + + // Check if version listing was called with the correct prefix + const versionListingCalls = getObjectListingStub.getCalls().filter(call => + call.args[1] && call.args[1].listingType === 'DelimiterVersions' + ); + + if (versionListingCalls.length > 0) { + // If version listing was called, verify the prefix optimization + const expectedPrefix = `${objectKey}\u0000`; // objectKey + VersionId separator + assert.strictEqual(versionListingCalls[0].args[1].prefix, expectedPrefix); + assert.strictEqual(versionListingCalls[0].args[1].maxKeys, 1000); + } + + done(); + }, abortRequest); + }); + }); + + it('should handle version listing pagination', (done) => { + createBucketAndMPU(true, (err, uploadId) => { + assert.ifError(err); + + // First page - no match, truncated + getObjectListingStub.onFirstCall().yields(null, { + Versions: [ + { + key: objectKey, + value: { uploadId: 'different-upload-id', versionId: 'version-456' } + } + ], + IsTruncated: true, + NextKeyMarker: objectKey, + NextVersionIdMarker: 'version-456' + }); + + // Second page - no match, not truncated + getObjectListingStub.onSecondCall().yields(null, { + Versions: [], + IsTruncated: false + }); + + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + assert.strictEqual(err, null); + + // Check if pagination markers were used correctly + const versionListingCalls = getObjectListingStub.getCalls().filter(call => + call.args[1] && call.args[1].listingType === 'DelimiterVersions' + ); + + if (versionListingCalls.length >= 2) { + // First call should not have markers + assert.strictEqual(versionListingCalls[0].args[1].keyMarker, undefined); + assert.strictEqual(versionListingCalls[0].args[1].versionIdMarker, undefined); + + // Second call should have markers from first response + assert.strictEqual(versionListingCalls[1].args[1].keyMarker, objectKey); + assert.strictEqual(versionListingCalls[1].args[1].versionIdMarker, 'version-456'); + } + + done(); + }, abortRequest); + }); + }); + + it('should handle version listing errors gracefully', (done) => { + createBucketAndMPU(true, (err, uploadId) => { + assert.ifError(err); + + // Simulate error during version listing + getObjectListingStub.yields(errors.InternalError); + + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + assert.strictEqual(err, null); // Should continue despite version listing error + done(); + }, abortRequest); + }); + }); + }); + + describe('error handling', () => { + it('should handle external data abort error', (done) => { + dataAbortMPUStub.restore(); + sinon.stub(data, 'abortMPU').callsFake((objectKey, uploadId, location, bucketName, request, destBucket, locationConstraintCheckFn, log, callback) => { + callback(errors.InternalError); + }); + + createBucketAndMPU(false, (err, uploadId) => { + assert.ifError(err); + + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + assert(err); + assert.strictEqual(err.is.InternalError, true); + done(); + }, abortRequest); + }); + }); + + it('should continue despite data deletion errors', (done) => { + dataDeleteStub.restore(); + sinon.stub(data, 'delete').yields(errors.InternalError); // Fail data deletion + + createBucketAndMPU(false, (err, uploadId) => { + assert.ifError(err); + + // Add a part so there's data to delete + const md5Hash = require('crypto').createHash('md5'); + md5Hash.update(postBody); + const calculatedHash = md5Hash.digest('hex'); + + const partRequest = new DummyRequest({ + bucketName, + objectKey, + namespace: 'default', + url: `/${objectKey}?partNumber=1&uploadId=${uploadId}`, + headers: { host: `${bucketName}.s3.amazonaws.com` }, + query: { + partNumber: '1', + uploadId, + }, + calculatedHash, + }, postBody); + + objectPutPart(authInfo, partRequest, undefined, log, (err) => { + assert.ifError(err); + + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + assert.strictEqual(err, null); // Should succeed despite data deletion failure + done(); + }, abortRequest); + }); + }); + }); + }); +}); From 0c3795692e8accd8b1003a97275498709ade3795 Mon Sep 17 00:00:00 2001 From: williamlardier Date: Tue, 1 Jul 2025 17:21:18 +0200 Subject: [PATCH 03/10] AbortMPU to properly cleanup existing s3 object - CompleteMPU was and is still, at the time of this commit, wrongly promoting s3 objects when completing a MPU even if the parts are not valid. - The AbortMPU cleanup logic was added as an effort to cleanup new occurences of this problem, and still handle old ones not yet aborted. - The logic currently is unable to clean up if the current master version has a different upload id: in case a new version was pushed, we end up never cleaning the ghost. - This commit introduces a dynamic listing of the versionIDs of the object. We use the delimiter versions algorithm and the versionID separator to list only the current object versions, with pagination support, and then we detect the one to cleanup. Issue: CLDSRV-669 --- lib/api/apiUtils/object/abortMultipartUpload.js | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/api/apiUtils/object/abortMultipartUpload.js b/lib/api/apiUtils/object/abortMultipartUpload.js index 4c8c51aa56..3aa586b124 100644 --- a/lib/api/apiUtils/object/abortMultipartUpload.js +++ b/lib/api/apiUtils/object/abortMultipartUpload.js @@ -195,9 +195,21 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, 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, objMDWithMatchingUploadID, 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, objMDWithMatchingUploadID, From 7705177b2d3c10384b7abbe78c061aa0acabba31 Mon Sep 17 00:00:00 2001 From: williamlardier Date: Tue, 1 Jul 2025 18:19:32 +0200 Subject: [PATCH 04/10] Add unit and functional tests Issue: CLDSRV-669 --- .../aws-node-sdk/test/object/abortMPU.js | 272 +++++++++-- .../object/abortMultipartUpload.spec.js | 431 +++++++++++++++--- 2 files changed, 611 insertions(+), 92 deletions(-) diff --git a/tests/functional/aws-node-sdk/test/object/abortMPU.js b/tests/functional/aws-node-sdk/test/object/abortMPU.js index 787d9d1304..6d9a74c956 100644 --- a/tests/functional/aws-node-sdk/test/object/abortMPU.js +++ b/tests/functional/aws-node-sdk/test/object/abortMPU.js @@ -25,18 +25,20 @@ describe('Abort MPU', () => { bucketUtil = new BucketUtility('default', sigCfg); s3 = bucketUtil.s3; return s3.createBucket({ Bucket: bucket }).promise() - .then(() => s3.createMultipartUpload({ - Bucket: bucket, Key: key }).promise()) - .then(res => { - uploadId = res.UploadId; - return s3.uploadPart({ Bucket: bucket, Key: key, - PartNumber: 1, UploadId: uploadId, Body: bodyFirstPart, - }).promise(); - }) - .catch(err => { - process.stdout.write(`Error in beforeEach: ${err}\n`); - throw err; - }); + .then(() => s3.createMultipartUpload({ + Bucket: bucket, Key: key + }).promise()) + .then(res => { + uploadId = res.UploadId; + return s3.uploadPart({ + Bucket: bucket, Key: key, + PartNumber: 1, UploadId: uploadId, Body: bodyFirstPart, + }).promise(); + }) + .catch(err => { + process.stdout.write(`Error in beforeEach: ${err}\n`); + throw err; + }); }); afterEach(() => @@ -45,23 +47,24 @@ describe('Abort MPU', () => { Key: key, UploadId: uploadId, }).promise() - .then(() => bucketUtil.empty(bucket)) - .then(() => bucketUtil.deleteOne(bucket)) + .then(() => bucketUtil.empty(bucket)) + .then(() => bucketUtil.deleteOne(bucket)) ); // aws-sdk now (v2.363.0) returns 'UriParameterError' error // this test was not replaced in any other suite it.skip('should return InvalidRequest error if aborting without key', - done => { - s3.abortMultipartUpload({ - Bucket: bucket, - Key: '', - UploadId: uploadId }, - err => { - checkError(err, 'InvalidRequest', 'A key must be specified'); - done(); + done => { + s3.abortMultipartUpload({ + Bucket: bucket, + Key: '', + UploadId: uploadId + }, + err => { + checkError(err, 'InvalidRequest', 'A key must be specified'); + done(); + }); }); - }); }); }); @@ -273,16 +276,221 @@ describe('Abort MPU - No Such Upload', () => { afterEach(() => bucketUtil.deleteOne(bucket)); it('should return NoSuchUpload error when aborting non-existent mpu', - done => { - s3.abortMultipartUpload({ - Bucket: bucket, - Key: key, - UploadId: uuidv4().replace(/-/g, '') }, - err => { - assert.notEqual(err, null, 'Expected failure but got success'); - assert.strictEqual(err.code, 'NoSuchUpload'); - done(); + done => { + s3.abortMultipartUpload({ + Bucket: bucket, + Key: key, + UploadId: uuidv4().replace(/-/g, '') + }, + err => { + assert.notEqual(err, null, 'Expected failure but got success'); + assert.strictEqual(err.code, 'NoSuchUpload'); + done(); + }); }); + }); +}); + +describe('Abort MPU - Versioned Bucket Cleanup', function testSuite() { + this.timeout(120000); + + withV4(sigCfg => { + let bucketUtil; + let s3; + const bucketName = `abort-mpu-versioned-${Date.now()}`; + const objectKey = 'test-object-with-versions'; + + beforeEach(done => { + bucketUtil = new BucketUtility('default', sigCfg); + s3 = bucketUtil.s3; + + async.series([ + next => s3.createBucket({ Bucket: bucketName }, next), + next => s3.putBucketVersioning({ + Bucket: bucketName, + VersioningConfiguration: { Status: 'Enabled' }, + }, next), + ], done); + }); + + afterEach(async () => { + // Clean up all multipart uploads + const listMPUResponse = await s3.listMultipartUploads({ Bucket: bucketName }).promise(); + await Promise.all(listMPUResponse.Uploads.map(upload => + s3.abortMultipartUpload({ + Bucket: bucketName, + Key: upload.Key, + UploadId: upload.UploadId, + }).promise().catch(err => { + if (err.code !== 'NoSuchUpload') throw err; + }) + )); + + // Clean up all object versions + const listVersionsResponse = await s3.listObjectVersions({ Bucket: bucketName }).promise(); + await Promise.all([ + ...listVersionsResponse.Versions.map(version => + s3.deleteObject({ + Bucket: bucketName, + Key: version.Key, + VersionId: version.VersionId, + }).promise() + ), + ...listVersionsResponse.DeleteMarkers.map(marker => + s3.deleteObject({ + Bucket: bucketName, + Key: marker.Key, + VersionId: marker.VersionId, + }).promise() + ), + ]); + + await bucketUtil.deleteOne(bucketName); + }); + + it('should handle aborting MPU with many versions of same object', done => { + const numberOfVersions = 5; + let currentVersion = 0; + let finalUploadId; + + // Create multiple versions of the same object + async.whilst( + () => currentVersion < numberOfVersions, + callback => { + currentVersion++; + const data = Buffer.from(`Version ${currentVersion} data`); + + async.waterfall([ + next => { + s3.createMultipartUpload({ + Bucket: bucketName, + Key: objectKey, + }, (err, result) => { + assert.ifError(err); + if (currentVersion === numberOfVersions) { + finalUploadId = result.UploadId; // Save the last one for aborting + } + next(null, result.UploadId); + }); + }, + (uploadId, next) => { + s3.uploadPart({ + Bucket: bucketName, + Key: objectKey, + PartNumber: 1, + UploadId: uploadId, + Body: data, + }, (err, result) => { + assert.ifError(err); + next(null, uploadId, result.ETag); + }); + }, + (uploadId, etag, next) => { + if (currentVersion === numberOfVersions) { + // Don't complete the last one - we'll abort it + return next(); + } + + return s3.completeMultipartUpload({ + Bucket: bucketName, + Key: objectKey, + UploadId: uploadId, + MultipartUpload: { + Parts: [{ ETag: etag, PartNumber: 1 }], + }, + }, next); + }, + ], callback); + }, + err => { + assert.ifError(err); + + // Now abort the final MPU + s3.abortMultipartUpload({ + Bucket: bucketName, + Key: objectKey, + UploadId: finalUploadId, + }, err => { + assert.ifError(err); + + // Verify we still have the correct number of completed versions + s3.listObjectVersions({ Bucket: bucketName }, (err, data) => { + assert.ifError(err); + + const objectVersions = data.Versions.filter(v => v.Key === objectKey); + assert.strictEqual(objectVersions.length, numberOfVersions - 1, + `Expected ${numberOfVersions - 1} versions after abort, got ${objectVersions.length}`); + + done(); + }); + }); + } + ); + }); + + it('should handle abort MPU when object has no versions', done => { + let uploadId; + const data = Buffer.from('test data for single MPU abort'); + + async.waterfall([ + // Create and upload part for MPU + next => { + s3.createMultipartUpload({ + Bucket: bucketName, + Key: objectKey, + }, (err, result) => { + assert.ifError(err); + uploadId = result.UploadId; + next(); + }); + }, + next => { + s3.uploadPart({ + Bucket: bucketName, + Key: objectKey, + PartNumber: 1, + UploadId: uploadId, + Body: data, + }, err => { + assert.ifError(err); + next(); + }); + }, + + // Abort the MPU + next => { + s3.abortMultipartUpload({ + Bucket: bucketName, + Key: objectKey, + UploadId: uploadId, + }, err => { + assert.ifError(err); + next(); + }); + }, + + // Verify no object exists + next => { + s3.getObject({ Bucket: bucketName, Key: objectKey }, err => { + assert.notEqual(err, null, 'Expected NoSuchKey error'); + assert.strictEqual(err.code, 'NoSuchKey'); + next(); + }); + }, + + // Verify no versions exist + next => { + s3.listObjectVersions({ Bucket: bucketName }, (err, data) => { + assert.ifError(err); + + const objectVersions = data.Versions.filter(v => v.Key === objectKey); + assert.strictEqual(objectVersions.length, 0, + `Expected 0 versions after abort, got ${objectVersions.length}`); + + next(); + }); + }, + ], done); }); }); }); diff --git a/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js b/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js index aa3e961e65..b8151ee8db 100644 --- a/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js +++ b/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js @@ -2,6 +2,7 @@ const assert = require('assert'); const sinon = require('sinon'); const { parseString } = require('xml2js'); const { errors } = require('arsenal'); +const crypto = require('crypto'); const abortMultipartUpload = require('../../../../../lib/api/apiUtils/object/abortMultipartUpload'); const { bucketPut } = require('../../../../../lib/api/bucketPut'); @@ -10,6 +11,8 @@ const bucketPutVersioning = require('../../../../../lib/api/bucketPutVersioning' const objectPutPart = require('../../../../../lib/api/objectPutPart'); const { data } = require('../../../../../lib/data/wrapper'); const quotaUtils = require('../../../../../lib/api/apiUtils/quotas/quotaUtils'); +const services = require('../../../../../lib/services'); +const metadata = require('../../../../../lib/metadata/wrapper'); const { DummyRequestLogger, makeAuthInfo, cleanup, versioningTestUtils } = require('../../../helpers'); const DummyRequest = require('../../../DummyRequest'); @@ -57,7 +60,8 @@ describe('abortMultipartUpload', () => { beforeEach(() => { cleanup(); // Stub external operations that we don't want to test - dataAbortMPUStub = sinon.stub(data, 'abortMPU').callsFake((objectKey, uploadId, location, bucketName, request, destBucket, locationConstraintCheckFn, log, callback) => { + dataAbortMPUStub = sinon.stub(data, 'abortMPU').callsFake((objectKey, uploadId, location, + bucketName, request, destBucket, locationConstraintCheckFn, log, callback) => { // Call the callback immediately without executing locationConstraintCheck callback(null, false); }); @@ -72,29 +76,43 @@ describe('abortMultipartUpload', () => { // Helper to create bucket and MPU, returns uploadId function createBucketAndMPU(versioned, callback) { if (versioned) { - bucketPut(authInfo, bucketRequest, log, (err) => { - if (err) return callback(err); - bucketPutVersioning(authInfo, enableVersioningRequest, log, (err) => { - if (err) return callback(err); - initiateMultipartUpload(authInfo, initiateRequest, log, (err, result) => { - if (err) return callback(err); - parseString(result, (err, json) => { - if (err) return callback(err); + bucketPut(authInfo, bucketRequest, log, err => { + if (err) { + return callback(err); + } + return bucketPutVersioning(authInfo, enableVersioningRequest, log, err => { + if (err) { + return callback(err); + } + return initiateMultipartUpload(authInfo, initiateRequest, log, (err, result) => { + if (err) { + return callback(err); + } + return parseString(result, (err, json) => { + if (err) { + return callback(err); + } const uploadId = json.InitiateMultipartUploadResult.UploadId[0]; - callback(null, uploadId); + return callback(null, uploadId); }); }); }); }); } else { - bucketPut(authInfo, bucketRequest, log, (err) => { - if (err) return callback(err); - initiateMultipartUpload(authInfo, initiateRequest, log, (err, result) => { - if (err) return callback(err); - parseString(result, (err, json) => { - if (err) return callback(err); + bucketPut(authInfo, bucketRequest, log, err => { + if (err) { + return callback(err); + } + return initiateMultipartUpload(authInfo, initiateRequest, log, (err, result) => { + if (err) { + return callback(err); + } + return parseString(result, (err, json) => { + if (err) { + return callback(err); + } const uploadId = json.InitiateMultipartUploadResult.UploadId[0]; - callback(null, uploadId); + return callback(null, uploadId); }); }); }); @@ -102,11 +120,11 @@ describe('abortMultipartUpload', () => { } describe('basic functionality', () => { - it('should successfully abort multipart upload', (done) => { + it('should successfully abort multipart upload', done => { createBucketAndMPU(false, (err, uploadId) => { assert.ifError(err); - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { assert.strictEqual(err, null); sinon.assert.calledOnce(dataAbortMPUStub); done(); @@ -114,19 +132,19 @@ describe('abortMultipartUpload', () => { }); }); - it('should return error for non-existent bucket', (done) => { - abortMultipartUpload(authInfo, 'non-existent-bucket', objectKey, 'fake-upload-id', log, (err) => { + it('should return error for non-existent bucket', done => { + abortMultipartUpload(authInfo, 'non-existent-bucket', objectKey, 'fake-upload-id', log, err => { assert(err); assert.strictEqual(err.is.NoSuchBucket, true); done(); }, abortRequest); }); - it('should return error for non-existent upload', (done) => { - bucketPut(authInfo, bucketRequest, log, (err) => { + it('should return error for non-existent upload', done => { + bucketPut(authInfo, bucketRequest, log, err => { assert.ifError(err); - abortMultipartUpload(authInfo, bucketName, objectKey, 'fake-upload-id', log, (err) => { + abortMultipartUpload(authInfo, bucketName, objectKey, 'fake-upload-id', log, err => { assert(err); assert.strictEqual(err.is.NoSuchUpload, true); done(); @@ -134,16 +152,17 @@ describe('abortMultipartUpload', () => { }); }); - it('should skip data deletion when skipDataDelete is true', (done) => { + it('should skip data deletion when skipDataDelete is true', done => { dataAbortMPUStub.restore(); - sinon.stub(data, 'abortMPU').callsFake((objectKey, uploadId, location, bucketName, request, destBucket, locationConstraintCheckFn, log, callback) => { + sinon.stub(data, 'abortMPU').callsFake((objectKey, uploadId, location, + bucketName, request, destBucket, locationConstraintCheckFn, log, callback) => { callback(null, true); // skipDataDelete = true }); createBucketAndMPU(false, (err, uploadId) => { assert.ifError(err); - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { assert.strictEqual(err, null); sinon.assert.notCalled(dataDeleteStub); sinon.assert.notCalled(validateQuotasStub); @@ -154,12 +173,12 @@ describe('abortMultipartUpload', () => { }); describe('with multipart upload parts', () => { - it('should delete part data when aborting', (done) => { + it('should delete part data when aborting', done => { createBucketAndMPU(false, (err, uploadId) => { assert.ifError(err); // Add a part to the multipart upload - const md5Hash = require('crypto').createHash('md5'); + const md5Hash = crypto.createHash('md5'); md5Hash.update(postBody); const calculatedHash = md5Hash.digest('hex'); @@ -176,10 +195,10 @@ describe('abortMultipartUpload', () => { calculatedHash, }, postBody); - objectPutPart(authInfo, partRequest, undefined, log, (err) => { + objectPutPart(authInfo, partRequest, undefined, log, err => { assert.ifError(err); - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { assert.strictEqual(err, null); sinon.assert.calledOnce(dataAbortMPUStub); sinon.assert.called(dataDeleteStub); // Should delete part data @@ -191,11 +210,11 @@ describe('abortMultipartUpload', () => { }); describe('versioned bucket behavior', () => { - it('should handle versioned bucket abort', (done) => { + it('should handle versioned bucket abort', done => { createBucketAndMPU(true, (err, uploadId) => { assert.ifError(err); - - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { assert.strictEqual(err, null); sinon.assert.calledOnce(dataAbortMPUStub); done(); @@ -205,7 +224,6 @@ describe('abortMultipartUpload', () => { }); describe('version listing optimization', () => { - const services = require('../../../../../lib/services'); let getObjectListingStub; beforeEach(() => { @@ -218,40 +236,40 @@ describe('abortMultipartUpload', () => { } }); - it('should use optimized prefix when listing versions', (done) => { + it('should use optimized prefix when listing versions', done => { createBucketAndMPU(true, (err, uploadId) => { assert.ifError(err); - + // Mock version listing response - return empty to simulate no cleanup needed getObjectListingStub.yields(null, { Versions: [], IsTruncated: false }); - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { assert.strictEqual(err, null); - + // Check if version listing was called with the correct prefix - const versionListingCalls = getObjectListingStub.getCalls().filter(call => + const versionListingCalls = getObjectListingStub.getCalls().filter(call => call.args[1] && call.args[1].listingType === 'DelimiterVersions' ); - + if (versionListingCalls.length > 0) { // If version listing was called, verify the prefix optimization const expectedPrefix = `${objectKey}\u0000`; // objectKey + VersionId separator assert.strictEqual(versionListingCalls[0].args[1].prefix, expectedPrefix); assert.strictEqual(versionListingCalls[0].args[1].maxKeys, 1000); } - + done(); }, abortRequest); }); }); - it('should handle version listing pagination', (done) => { + it('should handle version listing pagination', done => { createBucketAndMPU(true, (err, uploadId) => { assert.ifError(err); - + // First page - no match, truncated getObjectListingStub.onFirstCall().yields(null, { Versions: [ @@ -271,55 +289,348 @@ describe('abortMultipartUpload', () => { IsTruncated: false }); - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { assert.strictEqual(err, null); - + // Check if pagination markers were used correctly - const versionListingCalls = getObjectListingStub.getCalls().filter(call => + const versionListingCalls = getObjectListingStub.getCalls().filter(call => call.args[1] && call.args[1].listingType === 'DelimiterVersions' ); - + if (versionListingCalls.length >= 2) { // First call should not have markers assert.strictEqual(versionListingCalls[0].args[1].keyMarker, undefined); assert.strictEqual(versionListingCalls[0].args[1].versionIdMarker, undefined); - + // Second call should have markers from first response assert.strictEqual(versionListingCalls[1].args[1].keyMarker, objectKey); assert.strictEqual(versionListingCalls[1].args[1].versionIdMarker, 'version-456'); } - + done(); }, abortRequest); }); }); - it('should handle version listing errors gracefully', (done) => { + it('should handle version listing errors gracefully', done => { createBucketAndMPU(true, (err, uploadId) => { assert.ifError(err); - + // Simulate error during version listing getObjectListingStub.yields(errors.InternalError); - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { assert.strictEqual(err, null); // Should continue despite version listing error done(); }, abortRequest); }); }); + + it('should only list versions for exact object key, not similar prefixes', done => { + const testObjectKey = 'myfile.txt'; + + // Mock that we have an overview object to trigger cleanup logic + const metadataValidateMultipartStub = sinon.stub(services, 'metadataValidateMultipart') + .yields(null, { + getName: () => 'mpuShadowBuckettest-bucket', + getMdBucketModelVersion: () => 2 + }, { controllingLocationConstraint: 'us-east-1' }); + + const getMPUpartsStub = sinon.stub(services, 'getMPUparts').yields(null, { + Contents: [{ + key: 'part-key', + value: { Size: 1024, partLocations: [{ key: 'loc-1' }] } + }] + }); + + const batchDeleteObjectMetadataStub = sinon.stub(services, 'batchDeleteObjectMetadata').yields(); + + // Simulate version listing response with objects that have similar prefixes + getObjectListingStub.yields(null, { + Versions: [ + // This should be found (exact match) + { + key: testObjectKey, // 'myfile.txt' + value: { uploadId: 'target-upload-id', versionId: 'version-123' } + }, + // These should NOT be found (have similar prefixes but different keys) + { + key: 'myfile.txt.backup', // has prefix 'myfile.txt' + value: { uploadId: 'target-upload-id', versionId: 'version-456' } + }, + { + key: 'myfile.txt2', // has prefix 'myfile.txt' + value: { uploadId: 'target-upload-id', versionId: 'version-789' } + } + ], + IsTruncated: false + }); + + bucketPut(authInfo, bucketRequest, log, err => { + assert.ifError(err); + bucketPutVersioning(authInfo, enableVersioningRequest, log, err => { + assert.ifError(err); + + abortMultipartUpload(authInfo, bucketName, testObjectKey, 'target-upload-id', log, err => { + assert.strictEqual(err, undefined); + + // Verify the prefix optimization was used correctly + const versionListingCalls = getObjectListingStub.getCalls().filter(call => + call.args[1] && call.args[1].listingType === 'DelimiterVersions' + ); + + assert(versionListingCalls.length > 0, 'Should have called version listing'); + + // Verify the exact prefix was used + const expectedPrefix = `${testObjectKey}\u0000`; // 'myfile.txt\u0000' + assert.strictEqual(versionListingCalls[0].args[1].prefix, expectedPrefix); + + // The prefix ensures we only get versions that start with 'myfile.txt\u0000' + // This would match: 'myfile.txt\u0000version1', 'myfile.txt\u0000version2', etc. + // But NOT: 'myfile.txt.backup\u0000...', 'myfile.txt2\u0000...', etc. + + // Cleanup + metadataValidateMultipartStub.restore(); + getMPUpartsStub.restore(); + batchDeleteObjectMetadataStub.restore(); + done(); + }, abortRequest); + }); + }); + }); + + it('should delete the correct object version, not the master version', done => { + const targetUploadId = 'upload-to-cleanup'; + const masterVersionId = 'master-version-123'; + const orphanedVersionId = 'orphaned-version-456'; + + // Mock that we have an overview object to trigger cleanup logic + const metadataValidateMultipartStub = sinon.stub(services, 'metadataValidateMultipart') + .yields(null, { + getName: () => 'mpuShadowBuckettest-bucket', + getMdBucketModelVersion: () => 2 + }, { controllingLocationConstraint: 'us-east-1' }); + + const getMPUpartsStub = sinon.stub(services, 'getMPUparts').yields(null, { + Contents: [{ + key: 'part-key', + value: { Size: 1024, partLocations: [{ key: 'loc-1' }] } + }] + }); + + const batchDeleteObjectMetadataStub = sinon.stub(services, 'batchDeleteObjectMetadata').yields(); + const deleteObjectMDStub = sinon.stub(metadata, 'deleteObjectMD').yields(); + + // Simulate version listing response with multiple versions + getObjectListingStub.yields(null, { + Versions: [ + // Master version (current/latest) - should NOT be deleted + { + key: objectKey, + value: { + uploadId: 'different-upload-id', // Different uploadId - this is the valid master + versionId: masterVersionId, + isLatest: true + } + }, + // Orphaned version with matching uploadId - should be deleted + { + key: objectKey, + value: { + uploadId: targetUploadId, // Matching uploadId - this is orphaned metadata + versionId: orphanedVersionId, + isLatest: false + } + } + ], + IsTruncated: false + }); + + bucketPut(authInfo, bucketRequest, log, err => { + assert.ifError(err); + bucketPutVersioning(authInfo, enableVersioningRequest, log, err => { + assert.ifError(err); + + abortMultipartUpload(authInfo, bucketName, objectKey, targetUploadId, log, err => { + assert.strictEqual(err, undefined); + + // Should have called deleteObjectMD for object cleanup + sinon.assert.called(deleteObjectMDStub); + + // Verify it deleted the correct version (the one with matching uploadId) + const deleteObjectCalls = deleteObjectMDStub.getCalls().filter(call => + call.args[0] === bucketName && call.args[1] === objectKey + ); + + assert.strictEqual(deleteObjectCalls.length, 1, 'Should delete exactly one object version'); + + // Should delete the orphaned version, not the master + const deletedVersionId = deleteObjectCalls[0].args[2].versionId; + assert.strictEqual(deletedVersionId, orphanedVersionId, + 'Should delete the orphaned version with matching uploadId'); + assert.notStrictEqual(deletedVersionId, masterVersionId, + 'Should NOT delete the master version'); + + // Cleanup + metadataValidateMultipartStub.restore(); + getMPUpartsStub.restore(); + batchDeleteObjectMetadataStub.restore(); + deleteObjectMDStub.restore(); + done(); + }, abortRequest); + }); + }); + }); + + it('should handle multiple versions with same uploadId and delete all matching ones', done => { + const targetUploadId = 'upload-to-cleanup'; + + // Mock that we have an overview object to trigger cleanup logic + const metadataValidateMultipartStub = sinon.stub(services, 'metadataValidateMultipart') + .yields(null, { + getName: () => 'mpuShadowBuckettest-bucket', + getMdBucketModelVersion: () => 2 + }, { controllingLocationConstraint: 'us-east-1' }); + + const getMPUpartsStub = sinon.stub(services, 'getMPUparts').yields(null, { + Contents: [{ + key: 'part-key', + value: { Size: 1024, partLocations: [{ key: 'loc-1' }] } + }] + }); + + const batchDeleteObjectMetadataStub = sinon.stub(services, 'batchDeleteObjectMetadata').yields(); + const deleteObjectMDStub = sinon.stub(metadata, 'deleteObjectMD').yields(); + + // Simulate finding the first matching version, which should stop the search + getObjectListingStub.yields(null, { + Versions: [ + // First version with matching uploadId - should be found and deleted + { + key: objectKey, + value: { + uploadId: targetUploadId, + versionId: 'first-match-version' + } + }, + // There could be more versions after this, but the optimization + // should stop at the first match for efficiency + ], + IsTruncated: false + }); + + bucketPut(authInfo, bucketRequest, log, err => { + assert.ifError(err); + bucketPutVersioning(authInfo, enableVersioningRequest, log, err => { + assert.ifError(err); + + abortMultipartUpload(authInfo, bucketName, objectKey, targetUploadId, log, err => { + assert.strictEqual(err, undefined); + + // Should have found and processed the first matching version + sinon.assert.called(deleteObjectMDStub); + + // Verify it found the first matching version + const deleteObjectCalls = deleteObjectMDStub.getCalls().filter(call => + call.args[0] === bucketName && call.args[1] === objectKey + ); + + assert.strictEqual(deleteObjectCalls.length, 1, + 'Should process the first matching version found'); + + const deletedVersionId = deleteObjectCalls[0].args[2].versionId; + assert.strictEqual(deletedVersionId, 'first-match-version', + 'Should delete the first matching version found'); + + // Cleanup + metadataValidateMultipartStub.restore(); + getMPUpartsStub.restore(); + batchDeleteObjectMetadataStub.restore(); + deleteObjectMDStub.restore(); + done(); + }, abortRequest); + }); + }); + }); + + it('should log error when object metadata deletion fails with non-NoSuchKey error', done => { + const targetUploadId = 'upload-to-cleanup'; + const logErrorSpy = sinon.spy(log, 'error'); + + // Mock that we have an overview object to trigger cleanup logic + const metadataValidateMultipartStub = sinon.stub(services, 'metadataValidateMultipart') + .yields(null, { + getName: () => 'mpuShadowBuckettest-bucket', + getMdBucketModelVersion: () => 2 + }, { controllingLocationConstraint: 'us-east-1' }); + + const getMPUpartsStub = sinon.stub(services, 'getMPUparts').yields(null, { + Contents: [{ + key: 'part-key', + value: { Size: 1024, partLocations: [{ key: 'loc-1' }] } + }] + }); + + const batchDeleteObjectMetadataStub = sinon.stub(services, 'batchDeleteObjectMetadata').yields(); + + // Mock deleteObjectMD to fail with a non-NoSuchKey error + const deleteObjectMDStub = sinon.stub(metadata, 'deleteObjectMD').yields(errors.InternalError); + + // Simulate finding a matching version + getObjectListingStub.yields(null, { + Versions: [ + { + key: objectKey, + value: { + uploadId: targetUploadId, + versionId: 'version-to-delete' + } + } + ], + IsTruncated: false + }); + + bucketPut(authInfo, bucketRequest, log, err => { + assert.ifError(err); + bucketPutVersioning(authInfo, enableVersioningRequest, log, err => { + assert.ifError(err); + + abortMultipartUpload(authInfo, bucketName, objectKey, targetUploadId, log, err => { + assert.strictEqual(err, undefined); // Should continue despite deletion error + + // Verify the error was logged + const errorLogCalls = logErrorSpy.getCalls().filter(call => + call.args[0] === 'error deleting object metadata' + ); + assert.strictEqual(errorLogCalls.length, 1, + 'Should log error when object metadata deletion fails'); + assert.strictEqual(errorLogCalls[0].args[1].error.is.InternalError, true); + + // Cleanup + logErrorSpy.restore(); + metadataValidateMultipartStub.restore(); + getMPUpartsStub.restore(); + batchDeleteObjectMetadataStub.restore(); + deleteObjectMDStub.restore(); + done(); + }, abortRequest); + }); + }); + }); }); describe('error handling', () => { - it('should handle external data abort error', (done) => { + it('should handle external data abort error', done => { dataAbortMPUStub.restore(); - sinon.stub(data, 'abortMPU').callsFake((objectKey, uploadId, location, bucketName, request, destBucket, locationConstraintCheckFn, log, callback) => { + sinon.stub(data, 'abortMPU').callsFake((objectKey, uploadId, location, bucketName, + request, destBucket, locationConstraintCheckFn, log, callback) => { callback(errors.InternalError); }); createBucketAndMPU(false, (err, uploadId) => { assert.ifError(err); - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { assert(err); assert.strictEqual(err.is.InternalError, true); done(); @@ -327,7 +638,7 @@ describe('abortMultipartUpload', () => { }); }); - it('should continue despite data deletion errors', (done) => { + it('should continue despite data deletion errors', done => { dataDeleteStub.restore(); sinon.stub(data, 'delete').yields(errors.InternalError); // Fail data deletion @@ -335,7 +646,7 @@ describe('abortMultipartUpload', () => { assert.ifError(err); // Add a part so there's data to delete - const md5Hash = require('crypto').createHash('md5'); + const md5Hash = crypto.createHash('md5'); md5Hash.update(postBody); const calculatedHash = md5Hash.digest('hex'); @@ -352,10 +663,10 @@ describe('abortMultipartUpload', () => { calculatedHash, }, postBody); - objectPutPart(authInfo, partRequest, undefined, log, (err) => { + objectPutPart(authInfo, partRequest, undefined, log, err => { assert.ifError(err); - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, (err) => { + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { assert.strictEqual(err, null); // Should succeed despite data deletion failure done(); }, abortRequest); From 29859c10bdb1cf1023fb819df21b6793e98d3017 Mon Sep 17 00:00:00 2001 From: williamlardier Date: Tue, 22 Jul 2025 14:08:57 +0200 Subject: [PATCH 05/10] Rework log level, and simplify cleanup checks - Also, some test implementation was improved to reduce duplication. Issue: CLDSRV-669 --- .../apiUtils/object/abortMultipartUpload.js | 15 ++--- .../aws-node-sdk/test/object/abortMPU.js | 36 +++++------ .../object/abortMultipartUpload.spec.js | 63 +++++++------------ 3 files changed, 45 insertions(+), 69 deletions(-) diff --git a/lib/api/apiUtils/object/abortMultipartUpload.js b/lib/api/apiUtils/object/abortMultipartUpload.js index 3aa586b124..1575b7566f 100644 --- a/lib/api/apiUtils/object/abortMultipartUpload.js +++ b/lib/api/apiUtils/object/abortMultipartUpload.js @@ -78,10 +78,10 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, } // 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); + return next(null, mpuBucket, destBucket, objectMD, skipDataDelete); }); }, - function getPartLocations(mpuBucket, mpuOverviewObj, destBucket, objectMD, skipDataDelete, + function getPartLocations(mpuBucket, destBucket, objectMD, skipDataDelete, next) { services.getMPUparts(mpuBucket.getName(), uploadId, log, (err, result) => { @@ -90,7 +90,7 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, return next(err, destBucket); } const storedParts = result.Contents; - return next(null, mpuBucket, mpuOverviewObj, storedParts, destBucket, objectMD, + return next(null, mpuBucket, storedParts, destBucket, objectMD, skipDataDelete); }); }, @@ -103,13 +103,8 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, // 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, + function ensureCleanupIsRequired(mpuBucket, 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); - } - // 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) { @@ -235,7 +230,7 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, return async.eachLimit(locations, 5, (loc, cb) => { data.delete(loc, log, err => { if (err) { - log.fatal('delete ObjectPart failed', { err }); + log.warn('delete ObjectPart failed', { err }); } cb(); }); diff --git a/tests/functional/aws-node-sdk/test/object/abortMPU.js b/tests/functional/aws-node-sdk/test/object/abortMPU.js index 6d9a74c956..12f62bb796 100644 --- a/tests/functional/aws-node-sdk/test/object/abortMPU.js +++ b/tests/functional/aws-node-sdk/test/object/abortMPU.js @@ -26,7 +26,8 @@ describe('Abort MPU', () => { s3 = bucketUtil.s3; return s3.createBucket({ Bucket: bucket }).promise() .then(() => s3.createMultipartUpload({ - Bucket: bucket, Key: key + Bucket: bucket, + Key: key, }).promise()) .then(res => { uploadId = res.UploadId; @@ -322,28 +323,25 @@ describe('Abort MPU - Versioned Bucket Cleanup', function testSuite() { Key: upload.Key, UploadId: upload.UploadId, }).promise().catch(err => { - if (err.code !== 'NoSuchUpload') throw err; - }) + if (err.code !== 'NoSuchUpload') { + throw err; + } + }), )); // Clean up all object versions const listVersionsResponse = await s3.listObjectVersions({ Bucket: bucketName }).promise(); - await Promise.all([ - ...listVersionsResponse.Versions.map(version => - s3.deleteObject({ - Bucket: bucketName, - Key: version.Key, - VersionId: version.VersionId, - }).promise() - ), - ...listVersionsResponse.DeleteMarkers.map(marker => - s3.deleteObject({ - Bucket: bucketName, - Key: marker.Key, - VersionId: marker.VersionId, - }).promise() - ), - ]); + const allObjects = [ + ...listVersionsResponse.Versions, + ...listVersionsResponse.DeleteMarkers, + ]; + await Promise.all(allObjects.map(obj => + s3.deleteObject({ + Bucket: bucketName, + Key: obj.Key, + VersionId: obj.VersionId, + }).promise() + )); await bucketUtil.deleteOne(bucketName); }); diff --git a/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js b/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js index b8151ee8db..fa2e260f42 100644 --- a/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js +++ b/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js @@ -2,6 +2,7 @@ const assert = require('assert'); const sinon = require('sinon'); const { parseString } = require('xml2js'); const { errors } = require('arsenal'); +const async = require('async'); const crypto = require('crypto'); const abortMultipartUpload = require('../../../../../lib/api/apiUtils/object/abortMultipartUpload'); @@ -75,48 +76,24 @@ describe('abortMultipartUpload', () => { // Helper to create bucket and MPU, returns uploadId function createBucketAndMPU(versioned, callback) { + const tasks = [ + next => bucketPut(authInfo, bucketRequest, log, err => next(err)), + ]; + if (versioned) { - bucketPut(authInfo, bucketRequest, log, err => { - if (err) { - return callback(err); - } - return bucketPutVersioning(authInfo, enableVersioningRequest, log, err => { - if (err) { - return callback(err); - } - return initiateMultipartUpload(authInfo, initiateRequest, log, (err, result) => { - if (err) { - return callback(err); - } - return parseString(result, (err, json) => { - if (err) { - return callback(err); - } - const uploadId = json.InitiateMultipartUploadResult.UploadId[0]; - return callback(null, uploadId); - }); - }); - }); - }); - } else { - bucketPut(authInfo, bucketRequest, log, err => { - if (err) { - return callback(err); - } - return initiateMultipartUpload(authInfo, initiateRequest, log, (err, result) => { - if (err) { - return callback(err); - } - return parseString(result, (err, json) => { - if (err) { - return callback(err); - } - const uploadId = json.InitiateMultipartUploadResult.UploadId[0]; - return callback(null, uploadId); - }); - }); - }); + tasks.push(next => bucketPutVersioning(authInfo, enableVersioningRequest, log, err => next(err))); } + + tasks.push( + next => initiateMultipartUpload(authInfo, initiateRequest, log, (err, result) => next(err, result)), + (result, next) => parseString(result, (err, json) => next(err, json)), + (json, next) => { + const uploadId = json.InitiateMultipartUploadResult.UploadId[0]; + next(null, uploadId); + } + ); + + async.waterfall(tasks, callback); } describe('basic functionality', () => { @@ -202,6 +179,12 @@ describe('abortMultipartUpload', () => { assert.strictEqual(err, null); sinon.assert.calledOnce(dataAbortMPUStub); sinon.assert.called(dataDeleteStub); // Should delete part data + // ensure the right part was deleted - data.delete is called with location object + const firstCall = dataDeleteStub.getCalls()[0]; + const locationArg = firstCall.args[0]; + assert(typeof locationArg === 'object', 'First argument should be location object'); + assert(locationArg.dataStoreName, 'Location should have dataStoreName'); + assert(locationArg.key, 'Location should have key'); done(); }, abortRequest); }); From c2e8978cb98445c3b4fd524684706b83505f7b18 Mon Sep 17 00:00:00 2001 From: williamlardier Date: Tue, 22 Jul 2025 14:13:41 +0200 Subject: [PATCH 06/10] Test that warn log is catched by test - data delete error path Issue: CLDSRV-669 --- .../unit/api/apiUtils/object/abortMultipartUpload.spec.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js b/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js index fa2e260f42..cab26400fa 100644 --- a/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js +++ b/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js @@ -624,6 +624,7 @@ describe('abortMultipartUpload', () => { it('should continue despite data deletion errors', done => { dataDeleteStub.restore(); sinon.stub(data, 'delete').yields(errors.InternalError); // Fail data deletion + const logWarnSpy = sinon.spy(log, 'warn'); createBucketAndMPU(false, (err, uploadId) => { assert.ifError(err); @@ -651,6 +652,13 @@ describe('abortMultipartUpload', () => { abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { assert.strictEqual(err, null); // Should succeed despite data deletion failure + sinon.assert.called(logWarnSpy); + const warnCall = logWarnSpy.getCalls().find(call => + call.args[0] === 'delete ObjectPart failed' + ); + assert(warnCall, 'Expected warning log about failed data deletion'); + assert(warnCall.args[1].err, 'Expected error object in log warning'); + logWarnSpy.restore(); done(); }, abortRequest); }); From e81db96767e731839ab1b5a633e24d8627b5a7ad Mon Sep 17 00:00:00 2001 From: williamlardier Date: Mon, 28 Jul 2025 12:05:59 +0200 Subject: [PATCH 07/10] Move uploadId listing and adapt/add tests Issue: CLDSRV-669 --- .../apiUtils/object/abortMultipartUpload.js | 80 +-- lib/services.js | 66 ++ .../object/abortMultipartUpload.spec.js | 602 +++--------------- tests/unit/lib/services.spec.js | 64 ++ 4 files changed, 221 insertions(+), 591 deletions(-) create mode 100644 tests/unit/lib/services.spec.js diff --git a/lib/api/apiUtils/object/abortMultipartUpload.js b/lib/api/apiUtils/object/abortMultipartUpload.js index 1575b7566f..fcb13b6f85 100644 --- a/lib/api/apiUtils/object/abortMultipartUpload.js +++ b/lib/api/apiUtils/object/abortMultipartUpload.js @@ -3,12 +3,10 @@ const async = require('async'); const constants = require('../../../../constants'); const { data } = require('../../../data/wrapper'); const locationConstraintCheck = require('../object/locationConstraintCheck'); -const { standardMetadataValidateBucketAndObj } = - require('../../../metadata/metadataUtils'); +const metadataUtils = require('../../../metadata/metadataUtils'); 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) { @@ -31,7 +29,8 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, async.waterfall([ function checkDestBucketVal(next) { - standardMetadataValidateBucketAndObj(metadataValParams, authzIdentityResult, log, + // FIX: Call the function from the imported module object. + metadataUtils.standardMetadataValidateBucketAndObj(metadataValParams, authzIdentityResult, log, (err, destinationBucket, objectMD) => { if (err) { log.error('error validating request', { error: err }); @@ -105,9 +104,13 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, // supported by Metadata. function ensureCleanupIsRequired(mpuBucket, storedParts, destBucket, objectMD, skipDataDelete, next) { + if (!objectMD) { + return next(null, mpuBucket, storedParts, destBucket, null, skipDataDelete); + } + // 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) { + if (objectMD.uploadId === uploadId) { return next(null, mpuBucket, storedParts, destBucket, objectMD, skipDataDelete); } @@ -117,64 +120,21 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, 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}`, - 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(); + // Otherwise, list all versions to find one with a matching uploadId. + return services.findObjectVersionByUploadId(bucketName, objectKey, uploadId, log, (err, foundVersion) => { + if (err) { + log.error('error finding object version by uploadId, proceeding without cleanup', { + error: err, + method: 'abortMultipartUpload.ensureCleanupIsRequired', }); - }, - err => next(null, mpuBucket, storedParts, destBucket, err ? null : foundVersion, skipDataDelete) - ); + // On error, continue the abort without an objectMD to clean up. + return next(null, mpuBucket, storedParts, destBucket, null, skipDataDelete); + } + return next(null, mpuBucket, storedParts, destBucket, 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); } @@ -220,7 +180,7 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, } // Add object data locations if they exist and we can trust uploadId matches - if (objMDWithMatchingUploadID && objMDWithMatchingUploadID.location) { + if (objMDWithMatchingUploadID?.location) { const existingLocations = new Set(locations.map(loc => loc.key)); const remainingObjectLocations = objMDWithMatchingUploadID. location.filter(loc => !existingLocations.has(loc.key)); diff --git a/lib/services.js b/lib/services.js index a312137e5e..ac742f1c17 100644 --- a/lib/services.js +++ b/lib/services.js @@ -6,6 +6,7 @@ const { errors, s3middleware } = require('arsenal'); const ObjectMD = require('arsenal').models.ObjectMD; const BucketInfo = require('arsenal').models.BucketInfo; const ObjectMDArchive = require('arsenal').models.ObjectMDArchive; +const { versioning } = require('arsenal'); const acl = require('./metadata/acl'); const constants = require('../constants'); const { config } = require('./Config'); @@ -418,6 +419,71 @@ const services = { }); }, + /** + * Finds a specific object version by its uploadId by listing and filtering all versions. + * This is used during MPU abort to clean up potentially orphaned object metadata. + * @param {string} bucketName - The name of the bucket. + * @param {string} objectKey - The key of the object. + * @param {string} uploadId - The uploadId to search for. + * @param {object} log - The logger instance. + * @param {function} cb - The callback, called with (err, foundVersion). + * @returns {undefined} + */ + findObjectVersionByUploadId(bucketName, objectKey, uploadId, log, cb) { + 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}`, + maxKeys: 1000, + }; + + if (keyMarker) { + listParams.keyMarker = keyMarker; + } + if (versionIdMarker) { + listParams.versionIdMarker = versionIdMarker; + } + + return this.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 => cb(err, err ? null : foundVersion) + ); + }, + /** * Gets list of objects ready to be lifecycled * @param {object} bucketName - bucket in which objectMetadata is stored diff --git a/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js b/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js index cab26400fa..8d80431bd2 100644 --- a/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js +++ b/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js @@ -14,6 +14,7 @@ const { data } = require('../../../../../lib/data/wrapper'); const quotaUtils = require('../../../../../lib/api/apiUtils/quotas/quotaUtils'); const services = require('../../../../../lib/services'); const metadata = require('../../../../../lib/metadata/wrapper'); +const metadataUtils = require('../../../../../lib/metadata/metadataUtils'); const { DummyRequestLogger, makeAuthInfo, cleanup, versioningTestUtils } = require('../../../helpers'); const DummyRequest = require('../../../DummyRequest'); @@ -54,58 +55,53 @@ describe('abortMultipartUpload', () => { const enableVersioningRequest = versioningTestUtils.createBucketPutVersioningReq(bucketName, 'Enabled'); - let dataAbortMPUStub; - let dataDeleteStub; - let validateQuotasStub; - beforeEach(() => { cleanup(); - // Stub external operations that we don't want to test - dataAbortMPUStub = sinon.stub(data, 'abortMPU').callsFake((objectKey, uploadId, location, - bucketName, request, destBucket, locationConstraintCheckFn, log, callback) => { - // Call the callback immediately without executing locationConstraintCheck - callback(null, false); - }); - dataDeleteStub = sinon.stub(data, 'delete').yields(); - validateQuotasStub = sinon.stub(quotaUtils, 'validateQuotas').yields(); + sinon.stub(data, 'abortMPU').callsFake( + (objectKey, uploadId, location, bucketName, request, + destBucket, locationConstraintCheckFn, log, callback) => callback(null, false) + ); + sinon.stub(data, 'delete').yields(null); + sinon.stub(quotaUtils, 'validateQuotas').yields(null); + + sinon.stub(services, 'metadataValidateMultipart') + .yields(null, { + getName: () => 'mpu-shadow-bucket', + getMdBucketModelVersion: () => 2, + isVersioningEnabled: () => true, + }, { controllingLocationConstraint: 'us-east-1' }); + + sinon.stub(services, 'getMPUparts').yields(null, { Contents: [] }); + sinon.stub(services, 'batchDeleteObjectMetadata').yields(null); }); afterEach(() => { sinon.restore(); }); - // Helper to create bucket and MPU, returns uploadId function createBucketAndMPU(versioned, callback) { - const tasks = [ + async.waterfall([ next => bucketPut(authInfo, bucketRequest, log, err => next(err)), - ]; - - if (versioned) { - tasks.push(next => bucketPutVersioning(authInfo, enableVersioningRequest, log, err => next(err))); - } - - tasks.push( + next => { + if (versioned) { + return bucketPutVersioning(authInfo, enableVersioningRequest, log, err => next(err)); + } + return next(); + }, next => initiateMultipartUpload(authInfo, initiateRequest, log, (err, result) => next(err, result)), - (result, next) => parseString(result, (err, json) => next(err, json)), - (json, next) => { - const uploadId = json.InitiateMultipartUploadResult.UploadId[0]; - next(null, uploadId); - } - ); - - async.waterfall(tasks, callback); + (result, next) => parseString(result, (err, json) => + next(err, json.InitiateMultipartUploadResult.UploadId[0])), + ], callback); } describe('basic functionality', () => { it('should successfully abort multipart upload', done => { createBucketAndMPU(false, (err, uploadId) => { assert.ifError(err); - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { assert.strictEqual(err, null); - sinon.assert.calledOnce(dataAbortMPUStub); done(); - }, abortRequest); + }, { ...abortRequest, query: { uploadId } }); }); }); @@ -118,9 +114,9 @@ describe('abortMultipartUpload', () => { }); it('should return error for non-existent upload', done => { + services.metadataValidateMultipart.yields(errors.NoSuchUpload); bucketPut(authInfo, bucketRequest, log, err => { assert.ifError(err); - abortMultipartUpload(authInfo, bucketName, objectKey, 'fake-upload-id', log, err => { assert(err); assert.strictEqual(err.is.NoSuchUpload, true); @@ -128,541 +124,85 @@ describe('abortMultipartUpload', () => { }, abortRequest); }); }); - - it('should skip data deletion when skipDataDelete is true', done => { - dataAbortMPUStub.restore(); - sinon.stub(data, 'abortMPU').callsFake((objectKey, uploadId, location, - bucketName, request, destBucket, locationConstraintCheckFn, log, callback) => { - callback(null, true); // skipDataDelete = true - }); - - createBucketAndMPU(false, (err, uploadId) => { - assert.ifError(err); - - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { - assert.strictEqual(err, null); - sinon.assert.notCalled(dataDeleteStub); - sinon.assert.notCalled(validateQuotasStub); - done(); - }, abortRequest); - }); - }); }); describe('with multipart upload parts', () => { it('should delete part data when aborting', done => { createBucketAndMPU(false, (err, uploadId) => { assert.ifError(err); - - // Add a part to the multipart upload - const md5Hash = crypto.createHash('md5'); - md5Hash.update(postBody); - const calculatedHash = md5Hash.digest('hex'); - const partRequest = new DummyRequest({ - bucketName, - objectKey, - namespace: 'default', + bucketName, objectKey, namespace: 'default', url: `/${objectKey}?partNumber=1&uploadId=${uploadId}`, headers: { host: `${bucketName}.s3.amazonaws.com` }, - query: { - partNumber: '1', - uploadId, - }, - calculatedHash, + query: { partNumber: '1', uploadId }, + calculatedHash: crypto.createHash('md5').update(postBody).digest('hex'), }, postBody); objectPutPart(authInfo, partRequest, undefined, log, err => { assert.ifError(err); + services.getMPUparts.yields(null, { + Contents: [{ + key: `1${uploadId}`, + value: { + Size: 11, + partLocations: [{ key: 'a-key' }], + }, + }], + }); abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { assert.strictEqual(err, null); - sinon.assert.calledOnce(dataAbortMPUStub); - sinon.assert.called(dataDeleteStub); // Should delete part data - // ensure the right part was deleted - data.delete is called with location object - const firstCall = dataDeleteStub.getCalls()[0]; - const locationArg = firstCall.args[0]; - assert(typeof locationArg === 'object', 'First argument should be location object'); - assert(locationArg.dataStoreName, 'Location should have dataStoreName'); - assert(locationArg.key, 'Location should have key'); + sinon.assert.called(data.delete); done(); - }, abortRequest); + }, { ...abortRequest, query: { uploadId } }); }); }); }); }); - describe('versioned bucket behavior', () => { - it('should handle versioned bucket abort', done => { - createBucketAndMPU(true, (err, uploadId) => { - assert.ifError(err); - - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { - assert.strictEqual(err, null); - sinon.assert.calledOnce(dataAbortMPUStub); - done(); - }, abortRequest); - }); - }); - }); - - describe('version listing optimization', () => { - let getObjectListingStub; - - beforeEach(() => { - getObjectListingStub = sinon.stub(services, 'getObjectListing'); - }); - - afterEach(() => { - if (getObjectListingStub) { - getObjectListingStub.restore(); - } - }); - - it('should use optimized prefix when listing versions', done => { - createBucketAndMPU(true, (err, uploadId) => { - assert.ifError(err); - - // Mock version listing response - return empty to simulate no cleanup needed - getObjectListingStub.yields(null, { - Versions: [], - IsTruncated: false - }); - - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { - assert.strictEqual(err, null); - - // Check if version listing was called with the correct prefix - const versionListingCalls = getObjectListingStub.getCalls().filter(call => - call.args[1] && call.args[1].listingType === 'DelimiterVersions' - ); - - if (versionListingCalls.length > 0) { - // If version listing was called, verify the prefix optimization - const expectedPrefix = `${objectKey}\u0000`; // objectKey + VersionId separator - assert.strictEqual(versionListingCalls[0].args[1].prefix, expectedPrefix); - assert.strictEqual(versionListingCalls[0].args[1].maxKeys, 1000); - } - - done(); - }, abortRequest); - }); - }); - - it('should handle version listing pagination', done => { - createBucketAndMPU(true, (err, uploadId) => { - assert.ifError(err); - - // First page - no match, truncated - getObjectListingStub.onFirstCall().yields(null, { - Versions: [ - { - key: objectKey, - value: { uploadId: 'different-upload-id', versionId: 'version-456' } - } - ], - IsTruncated: true, - NextKeyMarker: objectKey, - NextVersionIdMarker: 'version-456' - }); - - // Second page - no match, not truncated - getObjectListingStub.onSecondCall().yields(null, { - Versions: [], - IsTruncated: false - }); - - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { - assert.strictEqual(err, null); - - // Check if pagination markers were used correctly - const versionListingCalls = getObjectListingStub.getCalls().filter(call => - call.args[1] && call.args[1].listingType === 'DelimiterVersions' - ); - - if (versionListingCalls.length >= 2) { - // First call should not have markers - assert.strictEqual(versionListingCalls[0].args[1].keyMarker, undefined); - assert.strictEqual(versionListingCalls[0].args[1].versionIdMarker, undefined); - - // Second call should have markers from first response - assert.strictEqual(versionListingCalls[1].args[1].keyMarker, objectKey); - assert.strictEqual(versionListingCalls[1].args[1].versionIdMarker, 'version-456'); - } - - done(); - }, abortRequest); - }); - }); - - it('should handle version listing errors gracefully', done => { - createBucketAndMPU(true, (err, uploadId) => { - assert.ifError(err); - - // Simulate error during version listing - getObjectListingStub.yields(errors.InternalError); - - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { - assert.strictEqual(err, null); // Should continue despite version listing error - done(); - }, abortRequest); - }); - }); - - it('should only list versions for exact object key, not similar prefixes', done => { - const testObjectKey = 'myfile.txt'; - - // Mock that we have an overview object to trigger cleanup logic - const metadataValidateMultipartStub = sinon.stub(services, 'metadataValidateMultipart') - .yields(null, { - getName: () => 'mpuShadowBuckettest-bucket', - getMdBucketModelVersion: () => 2 - }, { controllingLocationConstraint: 'us-east-1' }); - - const getMPUpartsStub = sinon.stub(services, 'getMPUparts').yields(null, { - Contents: [{ - key: 'part-key', - value: { Size: 1024, partLocations: [{ key: 'loc-1' }] } - }] - }); - - const batchDeleteObjectMetadataStub = sinon.stub(services, 'batchDeleteObjectMetadata').yields(); - - // Simulate version listing response with objects that have similar prefixes - getObjectListingStub.yields(null, { - Versions: [ - // This should be found (exact match) - { - key: testObjectKey, // 'myfile.txt' - value: { uploadId: 'target-upload-id', versionId: 'version-123' } - }, - // These should NOT be found (have similar prefixes but different keys) - { - key: 'myfile.txt.backup', // has prefix 'myfile.txt' - value: { uploadId: 'target-upload-id', versionId: 'version-456' } - }, - { - key: 'myfile.txt2', // has prefix 'myfile.txt' - value: { uploadId: 'target-upload-id', versionId: 'version-789' } - } - ], - IsTruncated: false - }); + describe('orphaned version cleanup', () => { + let findObjectVersionStub; + beforeEach(done => { + findObjectVersionStub = sinon.stub(services, 'findObjectVersionByUploadId'); bucketPut(authInfo, bucketRequest, log, err => { assert.ifError(err); - bucketPutVersioning(authInfo, enableVersioningRequest, log, err => { - assert.ifError(err); - - abortMultipartUpload(authInfo, bucketName, testObjectKey, 'target-upload-id', log, err => { - assert.strictEqual(err, undefined); - - // Verify the prefix optimization was used correctly - const versionListingCalls = getObjectListingStub.getCalls().filter(call => - call.args[1] && call.args[1].listingType === 'DelimiterVersions' - ); - - assert(versionListingCalls.length > 0, 'Should have called version listing'); - - // Verify the exact prefix was used - const expectedPrefix = `${testObjectKey}\u0000`; // 'myfile.txt\u0000' - assert.strictEqual(versionListingCalls[0].args[1].prefix, expectedPrefix); - - // The prefix ensures we only get versions that start with 'myfile.txt\u0000' - // This would match: 'myfile.txt\u0000version1', 'myfile.txt\u0000version2', etc. - // But NOT: 'myfile.txt.backup\u0000...', 'myfile.txt2\u0000...', etc. - - // Cleanup - metadataValidateMultipartStub.restore(); - getMPUpartsStub.restore(); - batchDeleteObjectMetadataStub.restore(); - done(); - }, abortRequest); - }); - }); - }); - - it('should delete the correct object version, not the master version', done => { - const targetUploadId = 'upload-to-cleanup'; - const masterVersionId = 'master-version-123'; - const orphanedVersionId = 'orphaned-version-456'; - - // Mock that we have an overview object to trigger cleanup logic - const metadataValidateMultipartStub = sinon.stub(services, 'metadataValidateMultipart') - .yields(null, { - getName: () => 'mpuShadowBuckettest-bucket', - getMdBucketModelVersion: () => 2 - }, { controllingLocationConstraint: 'us-east-1' }); - - const getMPUpartsStub = sinon.stub(services, 'getMPUparts').yields(null, { - Contents: [{ - key: 'part-key', - value: { Size: 1024, partLocations: [{ key: 'loc-1' }] } - }] - }); - - const batchDeleteObjectMetadataStub = sinon.stub(services, 'batchDeleteObjectMetadata').yields(); - const deleteObjectMDStub = sinon.stub(metadata, 'deleteObjectMD').yields(); - - // Simulate version listing response with multiple versions - getObjectListingStub.yields(null, { - Versions: [ - // Master version (current/latest) - should NOT be deleted - { - key: objectKey, - value: { - uploadId: 'different-upload-id', // Different uploadId - this is the valid master - versionId: masterVersionId, - isLatest: true - } - }, - // Orphaned version with matching uploadId - should be deleted - { - key: objectKey, - value: { - uploadId: targetUploadId, // Matching uploadId - this is orphaned metadata - versionId: orphanedVersionId, - isLatest: false - } - } - ], - IsTruncated: false - }); - - bucketPut(authInfo, bucketRequest, log, err => { - assert.ifError(err); - bucketPutVersioning(authInfo, enableVersioningRequest, log, err => { - assert.ifError(err); - - abortMultipartUpload(authInfo, bucketName, objectKey, targetUploadId, log, err => { - assert.strictEqual(err, undefined); - - // Should have called deleteObjectMD for object cleanup - sinon.assert.called(deleteObjectMDStub); - - // Verify it deleted the correct version (the one with matching uploadId) - const deleteObjectCalls = deleteObjectMDStub.getCalls().filter(call => - call.args[0] === bucketName && call.args[1] === objectKey - ); - - assert.strictEqual(deleteObjectCalls.length, 1, 'Should delete exactly one object version'); - - // Should delete the orphaned version, not the master - const deletedVersionId = deleteObjectCalls[0].args[2].versionId; - assert.strictEqual(deletedVersionId, orphanedVersionId, - 'Should delete the orphaned version with matching uploadId'); - assert.notStrictEqual(deletedVersionId, masterVersionId, - 'Should NOT delete the master version'); - - // Cleanup - metadataValidateMultipartStub.restore(); - getMPUpartsStub.restore(); - batchDeleteObjectMetadataStub.restore(); - deleteObjectMDStub.restore(); - done(); - }, abortRequest); - }); - }); - }); - - it('should handle multiple versions with same uploadId and delete all matching ones', done => { - const targetUploadId = 'upload-to-cleanup'; - - // Mock that we have an overview object to trigger cleanup logic - const metadataValidateMultipartStub = sinon.stub(services, 'metadataValidateMultipart') - .yields(null, { - getName: () => 'mpuShadowBuckettest-bucket', - getMdBucketModelVersion: () => 2 - }, { controllingLocationConstraint: 'us-east-1' }); - - const getMPUpartsStub = sinon.stub(services, 'getMPUparts').yields(null, { - Contents: [{ - key: 'part-key', - value: { Size: 1024, partLocations: [{ key: 'loc-1' }] } - }] - }); - - const batchDeleteObjectMetadataStub = sinon.stub(services, 'batchDeleteObjectMetadata').yields(); - const deleteObjectMDStub = sinon.stub(metadata, 'deleteObjectMD').yields(); - - // Simulate finding the first matching version, which should stop the search - getObjectListingStub.yields(null, { - Versions: [ - // First version with matching uploadId - should be found and deleted - { - key: objectKey, - value: { - uploadId: targetUploadId, - versionId: 'first-match-version' - } - }, - // There could be more versions after this, but the optimization - // should stop at the first match for efficiency - ], - IsTruncated: false - }); - - bucketPut(authInfo, bucketRequest, log, err => { - assert.ifError(err); - bucketPutVersioning(authInfo, enableVersioningRequest, log, err => { - assert.ifError(err); - - abortMultipartUpload(authInfo, bucketName, objectKey, targetUploadId, log, err => { - assert.strictEqual(err, undefined); - - // Should have found and processed the first matching version - sinon.assert.called(deleteObjectMDStub); - - // Verify it found the first matching version - const deleteObjectCalls = deleteObjectMDStub.getCalls().filter(call => - call.args[0] === bucketName && call.args[1] === objectKey - ); - - assert.strictEqual(deleteObjectCalls.length, 1, - 'Should process the first matching version found'); - - const deletedVersionId = deleteObjectCalls[0].args[2].versionId; - assert.strictEqual(deletedVersionId, 'first-match-version', - 'Should delete the first matching version found'); - - // Cleanup - metadataValidateMultipartStub.restore(); - getMPUpartsStub.restore(); - batchDeleteObjectMetadataStub.restore(); - deleteObjectMDStub.restore(); - done(); - }, abortRequest); - }); + done(); }); }); - it('should log error when object metadata deletion fails with non-NoSuchKey error', done => { - const targetUploadId = 'upload-to-cleanup'; - const logErrorSpy = sinon.spy(log, 'error'); - - // Mock that we have an overview object to trigger cleanup logic - const metadataValidateMultipartStub = sinon.stub(services, 'metadataValidateMultipart') - .yields(null, { - getName: () => 'mpuShadowBuckettest-bucket', - getMdBucketModelVersion: () => 2 - }, { controllingLocationConstraint: 'us-east-1' }); - - const getMPUpartsStub = sinon.stub(services, 'getMPUparts').yields(null, { - Contents: [{ - key: 'part-key', - value: { Size: 1024, partLocations: [{ key: 'loc-1' }] } - }] - }); - - const batchDeleteObjectMetadataStub = sinon.stub(services, 'batchDeleteObjectMetadata').yields(); - - // Mock deleteObjectMD to fail with a non-NoSuchKey error - const deleteObjectMDStub = sinon.stub(metadata, 'deleteObjectMD').yields(errors.InternalError); - - // Simulate finding a matching version - getObjectListingStub.yields(null, { - Versions: [ - { - key: objectKey, - value: { - uploadId: targetUploadId, - versionId: 'version-to-delete' - } - } - ], - IsTruncated: false - }); - - bucketPut(authInfo, bucketRequest, log, err => { + it('should NOT search for orphans if master object does not exist', done => { + bucketPutVersioning(authInfo, enableVersioningRequest, log, err => { assert.ifError(err); - bucketPutVersioning(authInfo, enableVersioningRequest, log, err => { + abortMultipartUpload(authInfo, bucketName, objectKey, 'any-id', log, err => { assert.ifError(err); - - abortMultipartUpload(authInfo, bucketName, objectKey, targetUploadId, log, err => { - assert.strictEqual(err, undefined); // Should continue despite deletion error - - // Verify the error was logged - const errorLogCalls = logErrorSpy.getCalls().filter(call => - call.args[0] === 'error deleting object metadata' - ); - assert.strictEqual(errorLogCalls.length, 1, - 'Should log error when object metadata deletion fails'); - assert.strictEqual(errorLogCalls[0].args[1].error.is.InternalError, true); - - // Cleanup - logErrorSpy.restore(); - metadataValidateMultipartStub.restore(); - getMPUpartsStub.restore(); - batchDeleteObjectMetadataStub.restore(); - deleteObjectMDStub.restore(); - done(); - }, abortRequest); - }); - }); - }); - }); - - describe('error handling', () => { - it('should handle external data abort error', done => { - dataAbortMPUStub.restore(); - sinon.stub(data, 'abortMPU').callsFake((objectKey, uploadId, location, bucketName, - request, destBucket, locationConstraintCheckFn, log, callback) => { - callback(errors.InternalError); - }); - - createBucketAndMPU(false, (err, uploadId) => { - assert.ifError(err); - - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { - assert(err); - assert.strictEqual(err.is.InternalError, true); + sinon.assert.notCalled(findObjectVersionStub); done(); }, abortRequest); }); }); - it('should continue despite data deletion errors', done => { - dataDeleteStub.restore(); - sinon.stub(data, 'delete').yields(errors.InternalError); // Fail data deletion - const logWarnSpy = sinon.spy(log, 'warn'); - - createBucketAndMPU(false, (err, uploadId) => { - assert.ifError(err); - - // Add a part so there's data to delete - const md5Hash = crypto.createHash('md5'); - md5Hash.update(postBody); - const calculatedHash = md5Hash.digest('hex'); - - const partRequest = new DummyRequest({ - bucketName, - objectKey, - namespace: 'default', - url: `/${objectKey}?partNumber=1&uploadId=${uploadId}`, - headers: { host: `${bucketName}.s3.amazonaws.com` }, - query: { - partNumber: '1', - uploadId, - }, - calculatedHash, - }, postBody); + it('should delete the correct orphaned object version', done => { + const deleteObjectMDStub = sinon.stub(metadata, 'deleteObjectMD').yields(null); + findObjectVersionStub.yields(null, { uploadId: 'abort-id', versionId: 'orphan-vid' }); - objectPutPart(authInfo, partRequest, undefined, log, err => { - assert.ifError(err); + const standardMetadataValidateStub = sinon.stub(metadataUtils, 'standardMetadataValidateBucketAndObj'); + const mockBucket = { + isVersioningEnabled: () => true, + getOwner: () => 'testCanonicalId', + getName: () => bucketName, + }; + const mockMasterMD = { + 'uploadId': 'master-id', + }; + standardMetadataValidateStub.yields(null, mockBucket, mockMasterMD); - abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { - assert.strictEqual(err, null); // Should succeed despite data deletion failure - sinon.assert.called(logWarnSpy); - const warnCall = logWarnSpy.getCalls().find(call => - call.args[0] === 'delete ObjectPart failed' - ); - assert(warnCall, 'Expected warning log about failed data deletion'); - assert(warnCall.args[1].err, 'Expected error object in log warning'); - logWarnSpy.restore(); - done(); - }, abortRequest); - }); - }); + abortMultipartUpload(authInfo, bucketName, objectKey, 'abort-id', log, err => { + assert.ifError(err); + sinon.assert.calledOnce(deleteObjectMDStub); + assert.strictEqual(deleteObjectMDStub.getCall(0).args[2].versionId, 'orphan-vid'); + done(); + }, { ...abortRequest, query: { uploadId: 'abort-id' } }); }); }); }); diff --git a/tests/unit/lib/services.spec.js b/tests/unit/lib/services.spec.js new file mode 100644 index 0000000000..cf8b5eb276 --- /dev/null +++ b/tests/unit/lib/services.spec.js @@ -0,0 +1,64 @@ +const assert = require('assert'); +const sinon = require('sinon'); +const { versioning } = require('arsenal'); + +const services = require('../../../lib/services'); +const { DummyRequestLogger } = require('../helpers'); + +const { VersionId } = versioning.VersioningConstants; + +describe('services', () => { + const log = new DummyRequestLogger(); + const bucketName = 'test-bucket'; + const objectKey = 'test-object'; + + afterEach(() => { + sinon.restore(); + }); + + describe('findObjectVersionByUploadId', () => { + let getObjectListingStub; + + beforeEach(() => { + getObjectListingStub = sinon.stub(services, 'getObjectListing'); + }); + + it('should call getObjectListing with an optimized prefix', done => { + getObjectListingStub.yields(null, { Versions: [], IsTruncated: false }); + + services.findObjectVersionByUploadId(bucketName, objectKey, 'any-upload-id', log, err => { + assert.ifError(err); + sinon.assert.calledOnce(getObjectListingStub); + + const listParams = getObjectListingStub.getCall(0).args[1]; + const expectedPrefix = `${objectKey}${VersionId.Separator}`; + assert.strictEqual(listParams.prefix, expectedPrefix); + assert.strictEqual(listParams.listingType, 'DelimiterVersions'); + done(); + }); + }); + + it('should correctly handle pagination', done => { + getObjectListingStub.onFirstCall().yields(null, { + Versions: [], + IsTruncated: true, + NextKeyMarker: 'key-marker', + NextVersionIdMarker: 'version-marker', + }); + getObjectListingStub.onSecondCall().yields(null, { + Versions: [], + IsTruncated: false, + }); + + services.findObjectVersionByUploadId(bucketName, objectKey, 'any-upload-id', log, err => { + assert.ifError(err); + sinon.assert.calledTwice(getObjectListingStub); + + const secondCallParams = getObjectListingStub.getCall(1).args[1]; + assert.strictEqual(secondCallParams.keyMarker, 'key-marker'); + assert.strictEqual(secondCallParams.versionIdMarker, 'version-marker'); + done(); + }); + }); + }); +}); From c859be775ba335409b674d06186004d5ca097d95 Mon Sep 17 00:00:00 2001 From: williamlardier Date: Mon, 28 Jul 2025 13:58:34 +0200 Subject: [PATCH 08/10] Add corner case test coverage, and revert unneeded format changes Issue: CLDSRV-699 --- .../apiUtils/object/abortMultipartUpload.js | 31 ++++---- .../object/abortMultipartUpload.spec.js | 78 ++++++++++++++++++- tests/unit/lib/services.spec.js | 30 +++++++ 3 files changed, 120 insertions(+), 19 deletions(-) diff --git a/lib/api/apiUtils/object/abortMultipartUpload.js b/lib/api/apiUtils/object/abortMultipartUpload.js index fcb13b6f85..8dd70cb8bb 100644 --- a/lib/api/apiUtils/object/abortMultipartUpload.js +++ b/lib/api/apiUtils/object/abortMultipartUpload.js @@ -29,7 +29,6 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, async.waterfall([ function checkDestBucketVal(next) { - // FIX: Call the function from the imported module object. metadataUtils.standardMetadataValidateBucketAndObj(metadataValParams, authzIdentityResult, log, (err, destinationBucket, objectMD) => { if (err) { @@ -67,21 +66,21 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, // 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, destBucket, objectMD, skipDataDelete); + }); }, function getPartLocations(mpuBucket, destBucket, objectMD, skipDataDelete, - next) { + next) { services.getMPUparts(mpuBucket.getName(), uploadId, log, (err, result) => { if (err) { @@ -90,7 +89,7 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, } const storedParts = result.Contents; return next(null, mpuBucket, storedParts, destBucket, objectMD, - skipDataDelete); + skipDataDelete); }); }, // During Abort, we dynamically detect if the previous CompleteMPU call @@ -123,7 +122,7 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, // Otherwise, list all versions to find one with a matching uploadId. return services.findObjectVersionByUploadId(bucketName, objectKey, uploadId, log, (err, foundVersion) => { if (err) { - log.error('error finding object version by uploadId, proceeding without cleanup', { + log.warn('error finding object version by uploadId, proceeding without cleanup', { error: err, method: 'abortMultipartUpload.ensureCleanupIsRequired', }); diff --git a/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js b/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js index 8d80431bd2..89c25a9bf7 100644 --- a/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js +++ b/tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js @@ -124,6 +124,21 @@ describe('abortMultipartUpload', () => { }, abortRequest); }); }); + + it('should return error if data backend fails to abort', done => { + const testError = new Error('Data backend abort failed'); + // This stub is now more explicit to avoid side-effects. + data.abortMPU.callsFake((objKey, upId, loc, bucket, req, destB, locCheckFn, log, cb) => { + cb(testError); + }); + createBucketAndMPU(false, (err, uploadId) => { + assert.ifError(err); + abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => { + assert.deepStrictEqual(err, testError); + done(); + }, { ...abortRequest, query: { uploadId } }); + }); + }); }); describe('with multipart upload parts', () => { @@ -182,6 +197,45 @@ describe('abortMultipartUpload', () => { }); }); + it('should NOT search for orphans on non-versioned bucket with mismatched master uploadId', done => { + const standardMetadataValidateStub = sinon.stub(metadataUtils, 'standardMetadataValidateBucketAndObj'); + const mockBucket = { + isVersioningEnabled: () => false, + getOwner: () => 'testCanonicalId', + getName: () => bucketName, + }; + const mockMasterMD = { 'uploadId': 'master-id' }; + standardMetadataValidateStub.yields(null, mockBucket, mockMasterMD); + + abortMultipartUpload(authInfo, bucketName, objectKey, 'abort-id', log, err => { + assert.ifError(err); + sinon.assert.notCalled(findObjectVersionStub); + done(); + }, { ...abortRequest, query: { uploadId: 'abort-id' } }); + }); + + it('should proceed without cleanup if finding object version fails', done => { + const testError = new Error('Find version failed'); + findObjectVersionStub.yields(testError); + const deleteObjectMDStub = sinon.stub(metadata, 'deleteObjectMD').yields(null); + + const standardMetadataValidateStub = sinon.stub(metadataUtils, 'standardMetadataValidateBucketAndObj'); + const mockBucket = { + isVersioningEnabled: () => true, + getOwner: () => 'testCanonicalId', + getName: () => bucketName, + }; + const mockMasterMD = { 'uploadId': 'master-id' }; + standardMetadataValidateStub.yields(null, mockBucket, mockMasterMD); + + abortMultipartUpload(authInfo, bucketName, objectKey, 'abort-id', log, err => { + assert.ifError(err); + sinon.assert.calledOnce(findObjectVersionStub); + sinon.assert.notCalled(deleteObjectMDStub); + done(); + }, { ...abortRequest, query: { uploadId: 'abort-id' } }); + }); + it('should delete the correct orphaned object version', done => { const deleteObjectMDStub = sinon.stub(metadata, 'deleteObjectMD').yields(null); findObjectVersionStub.yields(null, { uploadId: 'abort-id', versionId: 'orphan-vid' }); @@ -192,9 +246,7 @@ describe('abortMultipartUpload', () => { getOwner: () => 'testCanonicalId', getName: () => bucketName, }; - const mockMasterMD = { - 'uploadId': 'master-id', - }; + const mockMasterMD = { 'uploadId': 'master-id' }; standardMetadataValidateStub.yields(null, mockBucket, mockMasterMD); abortMultipartUpload(authInfo, bucketName, objectKey, 'abort-id', log, err => { @@ -204,5 +256,25 @@ describe('abortMultipartUpload', () => { done(); }, { ...abortRequest, query: { uploadId: 'abort-id' } }); }); + + it('should proceed if orphaned object version is already deleted (NoSuchKey)', done => { + const deleteObjectMDStub = sinon.stub(metadata, 'deleteObjectMD').yields(errors.NoSuchKey); + findObjectVersionStub.yields(null, { uploadId: 'abort-id', versionId: 'orphan-vid' }); + + const standardMetadataValidateStub = sinon.stub(metadataUtils, 'standardMetadataValidateBucketAndObj'); + const mockBucket = { + isVersioningEnabled: () => true, + getOwner: () => 'testCanonicalId', + getName: () => bucketName, + }; + const mockMasterMD = { 'uploadId': 'master-id' }; + standardMetadataValidateStub.yields(null, mockBucket, mockMasterMD); + + abortMultipartUpload(authInfo, bucketName, objectKey, 'abort-id', log, err => { + assert.ifError(err); + sinon.assert.calledOnce(deleteObjectMDStub); + done(); + }, { ...abortRequest, query: { uploadId: 'abort-id' } }); + }); }); }); diff --git a/tests/unit/lib/services.spec.js b/tests/unit/lib/services.spec.js index cf8b5eb276..eca63839c7 100644 --- a/tests/unit/lib/services.spec.js +++ b/tests/unit/lib/services.spec.js @@ -60,5 +60,35 @@ describe('services', () => { done(); }); }); + + it('should handle error from getObjectListing', done => { + const testError = new Error('listing failed'); + getObjectListingStub.yields(testError); + + services.findObjectVersionByUploadId(bucketName, objectKey, 'any-upload-id', log, err => { + assert.deepStrictEqual(err, testError); + sinon.assert.calledOnce(getObjectListingStub); + done(); + }); + }); + + it('should find a version with the matching uploadId', done => { + const uploadIdToFind = 'the-correct-upload-id'; + const correctVersionValue = { uploadId: uploadIdToFind, data: 'this is it' }; + const versions = [ + { key: objectKey, value: { uploadId: 'some-other-id' } }, + // Version with a different key but same uploadId to test the key check + { key: 'another-object-key', value: { uploadId: uploadIdToFind } }, + { key: objectKey, value: correctVersionValue }, + ]; + getObjectListingStub.yields(null, { Versions: versions, IsTruncated: false }); + + services.findObjectVersionByUploadId(bucketName, objectKey, uploadIdToFind, log, (err, foundVersion) => { + assert.ifError(err); + sinon.assert.calledOnce(getObjectListingStub); + assert.deepStrictEqual(foundVersion, correctVersionValue); + done(); + }); + }); }); }); From fdca4e6336d80ac9271f28b88c8aae2f25bf5297 Mon Sep 17 00:00:00 2001 From: williamlardier Date: Thu, 31 Jul 2025 09:32:06 +0200 Subject: [PATCH 09/10] Add test coverage and make code simpler to understand Issue: CLDSRV-669 --- .../apiUtils/object/abortMultipartUpload.js | 28 +++--- .../aws-node-sdk/test/object/abortMPU.js | 2 +- tests/unit/lib/services.spec.js | 98 +++++++++++++++---- 3 files changed, 95 insertions(+), 33 deletions(-) diff --git a/lib/api/apiUtils/object/abortMultipartUpload.js b/lib/api/apiUtils/object/abortMultipartUpload.js index 8dd70cb8bb..4f9e3c8cdc 100644 --- a/lib/api/apiUtils/object/abortMultipartUpload.js +++ b/lib/api/apiUtils/object/abortMultipartUpload.js @@ -101,7 +101,7 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, // 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, storedParts, destBucket, + function findObjectToCleanup(mpuBucket, storedParts, destBucket, objectMD, skipDataDelete, next) { if (!objectMD) { return next(null, mpuBucket, storedParts, destBucket, null, skipDataDelete); @@ -124,7 +124,7 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, if (err) { log.warn('error finding object version by uploadId, proceeding without cleanup', { error: err, - method: 'abortMultipartUpload.ensureCleanupIsRequired', + method: 'abortMultipartUpload.findObjectToCleanup', }); // On error, continue the abort without an objectMD to clean up. return next(null, mpuBucket, storedParts, destBucket, null, skipDataDelete); @@ -132,21 +132,21 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, return next(null, mpuBucket, storedParts, destBucket, foundVersion, skipDataDelete); }); }, - function deleteObjectMetadata(mpuBucket, storedParts, destBucket, objMDWithMatchingUploadID, + function deleteObjectMetadata(mpuBucket, storedParts, destBucket, objectMD, skipDataDelete, next) { - if (!objMDWithMatchingUploadID) { - return next(null, mpuBucket, storedParts, destBucket, objMDWithMatchingUploadID, skipDataDelete); + if (!objectMD) { + return next(null, mpuBucket, storedParts, destBucket, objectMD, skipDataDelete); } - log.debug('Object has existing metadata with matching uploadId, deleting them', { + log.debug('Object has existing metadata, deleting them', { method: 'abortMultipartUpload', bucketName, objectKey, uploadId, - versionId: objMDWithMatchingUploadID.versionId, + versionId: objectMD.versionId, }); return metadata.deleteObjectMD(bucketName, objectKey, { - versionId: objMDWithMatchingUploadID.versionId, + versionId: objectMD.versionId, }, log, err => { if (err) { // Handle concurrent deletion of this object metadata @@ -155,7 +155,7 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, method: 'abortMultipartUpload', bucketName, objectKey, - versionId: objMDWithMatchingUploadID.versionId, + versionId: objectMD.versionId, }); } else { log.error('error deleting object metadata', { error: err }); @@ -163,10 +163,10 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, } // 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); + return next(null, mpuBucket, storedParts, destBucket, objectMD, skipDataDelete); }); }, - function deleteData(mpuBucket, storedParts, destBucket, objMDWithMatchingUploadID, + function deleteData(mpuBucket, storedParts, destBucket, objectMD, skipDataDelete, next) { if (skipDataDelete) { return next(null, mpuBucket, storedParts, destBucket); @@ -178,10 +178,10 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, return next(null, mpuBucket, storedParts, destBucket); } - // Add object data locations if they exist and we can trust uploadId matches - if (objMDWithMatchingUploadID?.location) { + // Add object data locations if they exist + if (objectMD?.location) { const existingLocations = new Set(locations.map(loc => loc.key)); - const remainingObjectLocations = objMDWithMatchingUploadID. + const remainingObjectLocations = objectMD. location.filter(loc => !existingLocations.has(loc.key)); locations.push(...remainingObjectLocations); } diff --git a/tests/functional/aws-node-sdk/test/object/abortMPU.js b/tests/functional/aws-node-sdk/test/object/abortMPU.js index 12f62bb796..8374ce5274 100644 --- a/tests/functional/aws-node-sdk/test/object/abortMPU.js +++ b/tests/functional/aws-node-sdk/test/object/abortMPU.js @@ -293,7 +293,7 @@ describe('Abort MPU - No Such Upload', () => { }); describe('Abort MPU - Versioned Bucket Cleanup', function testSuite() { - this.timeout(120000); + this.timeout(30000); withV4(sigCfg => { let bucketUtil; diff --git a/tests/unit/lib/services.spec.js b/tests/unit/lib/services.spec.js index eca63839c7..24ace13332 100644 --- a/tests/unit/lib/services.spec.js +++ b/tests/unit/lib/services.spec.js @@ -38,54 +38,116 @@ describe('services', () => { }); }); - it('should correctly handle pagination', done => { + it('should handle an error from getObjectListing', done => { + const testError = new Error('listing failed'); + getObjectListingStub.yields(testError); + + services.findObjectVersionByUploadId(bucketName, objectKey, 'any-upload-id', log, err => { + assert.deepStrictEqual(err, testError); + sinon.assert.calledOnce(getObjectListingStub); + done(); + }); + }); + + it('should return null if no matching version is found', done => { + getObjectListingStub.yields(null, { Versions: [], IsTruncated: false }); + + services.findObjectVersionByUploadId(bucketName, objectKey, 'any-upload-id', log, (err, foundVersion) => { + assert.ifError(err); + sinon.assert.calledOnce(getObjectListingStub); + assert.strictEqual(foundVersion, null); + done(); + }); + }); + + it('should find a version on the only page of results', done => { + const uploadIdToFind = 'the-correct-upload-id'; + const correctVersionValue = { uploadId: uploadIdToFind, data: 'this is it' }; + const versions = [ + { key: objectKey, value: { uploadId: 'some-other-id' } }, + // Version with a different key but same uploadId to test the key check + { key: 'another-object-key', value: { uploadId: uploadIdToFind } }, + { key: objectKey, value: correctVersionValue }, + ]; + getObjectListingStub.yields(null, { Versions: versions, IsTruncated: false }); + + services.findObjectVersionByUploadId(bucketName, objectKey, uploadIdToFind, log, (err, foundVersion) => { + assert.ifError(err); + sinon.assert.calledOnce(getObjectListingStub); + assert.deepStrictEqual(foundVersion, correctVersionValue); + done(); + }); + }); + + it('should read all pages if a matching version is not found', done => { getObjectListingStub.onFirstCall().yields(null, { - Versions: [], + Versions: [{ key: objectKey, value: { uploadId: 'id-page-1' } }], IsTruncated: true, NextKeyMarker: 'key-marker', NextVersionIdMarker: 'version-marker', }); getObjectListingStub.onSecondCall().yields(null, { - Versions: [], + Versions: [{ key: objectKey, value: { uploadId: 'id-page-2' } }], IsTruncated: false, }); - services.findObjectVersionByUploadId(bucketName, objectKey, 'any-upload-id', log, err => { + services.findObjectVersionByUploadId(bucketName, objectKey, 'non-existent-upload-id', log, (err, foundVersion) => { assert.ifError(err); sinon.assert.calledTwice(getObjectListingStub); const secondCallParams = getObjectListingStub.getCall(1).args[1]; assert.strictEqual(secondCallParams.keyMarker, 'key-marker'); assert.strictEqual(secondCallParams.versionIdMarker, 'version-marker'); + assert.strictEqual(foundVersion, null); done(); }); }); - it('should handle error from getObjectListing', done => { - const testError = new Error('listing failed'); - getObjectListingStub.yields(testError); + it('should find a version on the first page of many and stop listing', done => { + const uploadIdToFind = 'the-correct-upload-id'; + const correctVersionValue = { uploadId: uploadIdToFind, data: 'this is it' }; + const versions = [{ key: objectKey, value: correctVersionValue }]; + + getObjectListingStub.onFirstCall().yields(null, { + Versions: versions, + IsTruncated: true, + NextKeyMarker: 'key-marker', + NextVersionIdMarker: 'version-marker', + }); + getObjectListingStub.onSecondCall().yields(new Error('should not have been called')); - services.findObjectVersionByUploadId(bucketName, objectKey, 'any-upload-id', log, err => { - assert.deepStrictEqual(err, testError); + services.findObjectVersionByUploadId(bucketName, objectKey, uploadIdToFind, log, (err, foundVersion) => { + assert.ifError(err); sinon.assert.calledOnce(getObjectListingStub); + assert.deepStrictEqual(foundVersion, correctVersionValue); done(); }); }); - it('should find a version with the matching uploadId', done => { + it('should find a version on a subsequent page', done => { const uploadIdToFind = 'the-correct-upload-id'; const correctVersionValue = { uploadId: uploadIdToFind, data: 'this is it' }; - const versions = [ - { key: objectKey, value: { uploadId: 'some-other-id' } }, - // Version with a different key but same uploadId to test the key check - { key: 'another-object-key', value: { uploadId: uploadIdToFind } }, - { key: objectKey, value: correctVersionValue }, - ]; - getObjectListingStub.yields(null, { Versions: versions, IsTruncated: false }); + const secondPageVersions = [{ key: objectKey, value: correctVersionValue }]; + + getObjectListingStub.onFirstCall().yields(null, { + Versions: [{ key: objectKey, value: { uploadId: 'some-other-id' } }], + IsTruncated: true, + NextKeyMarker: 'key-marker', + NextVersionIdMarker: 'version-marker', + }); + getObjectListingStub.onSecondCall().yields(null, { + Versions: secondPageVersions, + IsTruncated: false, + }); services.findObjectVersionByUploadId(bucketName, objectKey, uploadIdToFind, log, (err, foundVersion) => { assert.ifError(err); - sinon.assert.calledOnce(getObjectListingStub); + sinon.assert.calledTwice(getObjectListingStub); + + const secondCallParams = getObjectListingStub.getCall(1).args[1]; + assert.strictEqual(secondCallParams.keyMarker, 'key-marker'); + assert.strictEqual(secondCallParams.versionIdMarker, 'version-marker'); + assert.deepStrictEqual(foundVersion, correctVersionValue); done(); }); From 34ee809bde634843d4361d6c22b5eb560290a6e3 Mon Sep 17 00:00:00 2001 From: williamlardier Date: Thu, 31 Jul 2025 09:34:35 +0200 Subject: [PATCH 10/10] Linter fix Issue: CLDSRV-669 --- tests/unit/lib/services.spec.js | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/tests/unit/lib/services.spec.js b/tests/unit/lib/services.spec.js index 24ace13332..3c7b28fde7 100644 --- a/tests/unit/lib/services.spec.js +++ b/tests/unit/lib/services.spec.js @@ -91,23 +91,24 @@ describe('services', () => { IsTruncated: false, }); - services.findObjectVersionByUploadId(bucketName, objectKey, 'non-existent-upload-id', log, (err, foundVersion) => { - assert.ifError(err); - sinon.assert.calledTwice(getObjectListingStub); - - const secondCallParams = getObjectListingStub.getCall(1).args[1]; - assert.strictEqual(secondCallParams.keyMarker, 'key-marker'); - assert.strictEqual(secondCallParams.versionIdMarker, 'version-marker'); - assert.strictEqual(foundVersion, null); - done(); - }); + services.findObjectVersionByUploadId(bucketName, objectKey, 'non-existent-upload-id', + log, (err, foundVersion) => { + assert.ifError(err); + sinon.assert.calledTwice(getObjectListingStub); + + const secondCallParams = getObjectListingStub.getCall(1).args[1]; + assert.strictEqual(secondCallParams.keyMarker, 'key-marker'); + assert.strictEqual(secondCallParams.versionIdMarker, 'version-marker'); + assert.strictEqual(foundVersion, null); + done(); + }); }); it('should find a version on the first page of many and stop listing', done => { const uploadIdToFind = 'the-correct-upload-id'; const correctVersionValue = { uploadId: uploadIdToFind, data: 'this is it' }; const versions = [{ key: objectKey, value: correctVersionValue }]; - + getObjectListingStub.onFirstCall().yields(null, { Versions: versions, IsTruncated: true,