From f7db3485f6022cc570c507d78c07178b69bbf341 Mon Sep 17 00:00:00 2001 From: Ameya Naik Date: Fri, 6 Mar 2026 20:40:05 -0800 Subject: [PATCH 1/2] fix: fall back to individual deletes when backend returns NotImplemented for DeleteObjects Some S3-compatible backends (e.g. GCS in S3 interoperability mode) do not support the DeleteObjects batch API and return a NotImplemented error. Catch this and fall back to individual DeleteObjectCommand calls via Promise.allSettled, so a single failure does not abort remaining deletes mid-flight. --- src/storage/backend/s3/adapter.ts | 10 ++++++ src/test/s3-adapter.test.ts | 52 +++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/src/storage/backend/s3/adapter.ts b/src/storage/backend/s3/adapter.ts index 5c8fc4736..ab0d051b1 100644 --- a/src/storage/backend/s3/adapter.ts +++ b/src/storage/backend/s3/adapter.ts @@ -343,6 +343,16 @@ export class S3Backend implements StorageBackendAdapter { }) await this.client.send(command) } catch (e) { + // Some S3-compatible backends (e.g. GCS) do not support DeleteObjects; fall back to individual deletes + const code = (e as { Code?: string; name?: string })?.Code ?? (e as { name?: string })?.name + if (code === 'NotImplemented') { + await Promise.allSettled( + prefixes.map((key) => + this.client.send(new DeleteObjectCommand({ Bucket: bucket, Key: key })) + ) + ) + return + } throw StorageBackendError.fromError(e) } } diff --git a/src/test/s3-adapter.test.ts b/src/test/s3-adapter.test.ts index 0d5288e5f..27bb565db 100644 --- a/src/test/s3-adapter.test.ts +++ b/src/test/s3-adapter.test.ts @@ -74,4 +74,56 @@ describe('S3Backend', () => { expect(result.metadata.mimetype).toBe('image/png') }) }) + + describe('deleteObjects', () => { + test('should use batch DeleteObjectsCommand when backend supports it', async () => { + mockSend.mockResolvedValue({ + Deleted: [{ Key: 'file1.txt' }, { Key: 'file2.txt' }], + $metadata: { httpStatusCode: 200 }, + }) + + const backend = new S3Backend({ region: 'us-east-1', endpoint: 'http://localhost:9000' }) + await backend.deleteObjects('test-bucket', ['file1.txt', 'file2.txt']) + + expect(mockSend).toHaveBeenCalledTimes(1) + expect(mockSend.mock.calls[0][0].constructor.name).toBe('DeleteObjectsCommand') + }) + + test('should fall back to individual DeleteObjectCommands when backend returns NotImplemented', async () => { + const err = Object.assign(new Error('NotImplemented'), { Code: 'NotImplemented' }) + mockSend + .mockRejectedValueOnce(err) + .mockResolvedValue({ $metadata: { httpStatusCode: 204 } }) + + const backend = new S3Backend({ region: 'us-east-1', endpoint: 'http://localhost:9000' }) + await backend.deleteObjects('test-bucket', ['file1.txt', 'file2.txt']) + + expect(mockSend).toHaveBeenCalledTimes(3) + expect(mockSend.mock.calls[0][0].constructor.name).toBe('DeleteObjectsCommand') + expect(mockSend.mock.calls[1][0].constructor.name).toBe('DeleteObjectCommand') + expect(mockSend.mock.calls[2][0].constructor.name).toBe('DeleteObjectCommand') + }) + + test('should not throw if some individual fallback deletes fail', async () => { + const notImplemented = Object.assign(new Error('NotImplemented'), { Code: 'NotImplemented' }) + mockSend + .mockRejectedValueOnce(notImplemented) + .mockResolvedValueOnce({ $metadata: { httpStatusCode: 204 } }) + .mockRejectedValueOnce(new Error('AccessDenied')) + + const backend = new S3Backend({ region: 'us-east-1', endpoint: 'http://localhost:9000' }) + await expect( + backend.deleteObjects('test-bucket', ['file1.txt', 'file2.txt']) + ).resolves.toBeUndefined() + }) + + test('should rethrow errors that are not NotImplemented', async () => { + const err = Object.assign(new Error('AccessDenied'), { Code: 'AccessDenied' }) + mockSend.mockRejectedValue(err) + + const backend = new S3Backend({ region: 'us-east-1', endpoint: 'http://localhost:9000' }) + await expect(backend.deleteObjects('test-bucket', ['file1.txt'])).rejects.toThrow() + expect(mockSend).toHaveBeenCalledTimes(1) + }) + }) }) From cf308f16846ebf17f8dd8fc04068c4fa950679a1 Mon Sep 17 00:00:00 2001 From: Ameya Naik Date: Fri, 6 Mar 2026 20:49:19 -0800 Subject: [PATCH 2/2] fix: inspect allSettled results in NotImplemented fallback Follow the same pattern as the file.ts backend: iterate allSettled results and throw on any individual delete failure except NoSuchKey (object already gone), preventing orphaned S3 objects when a delete fails mid-batch. Also updates tests to cover NoSuchKey-ignore and real-error-throw behaviour. --- src/storage/backend/s3/adapter.ts | 12 +++++++++++- src/test/s3-adapter.test.ts | 25 ++++++++++++++++++++----- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/src/storage/backend/s3/adapter.ts b/src/storage/backend/s3/adapter.ts index ab0d051b1..ca1fd8112 100644 --- a/src/storage/backend/s3/adapter.ts +++ b/src/storage/backend/s3/adapter.ts @@ -346,11 +346,21 @@ export class S3Backend implements StorageBackendAdapter { // Some S3-compatible backends (e.g. GCS) do not support DeleteObjects; fall back to individual deletes const code = (e as { Code?: string; name?: string })?.Code ?? (e as { name?: string })?.name if (code === 'NotImplemented') { - await Promise.allSettled( + const results = await Promise.allSettled( prefixes.map((key) => this.client.send(new DeleteObjectCommand({ Bucket: bucket, Key: key })) ) ) + for (const result of results) { + if (result.status === 'rejected') { + const errCode = + (result.reason as { Code?: string })?.Code ?? + (result.reason as { name?: string })?.name + if (errCode !== 'NoSuchKey') { + throw StorageBackendError.fromError(result.reason) + } + } + } return } throw StorageBackendError.fromError(e) diff --git a/src/test/s3-adapter.test.ts b/src/test/s3-adapter.test.ts index 27bb565db..036400004 100644 --- a/src/test/s3-adapter.test.ts +++ b/src/test/s3-adapter.test.ts @@ -20,9 +20,9 @@ describe('S3Backend', () => { beforeEach(() => { jest.clearAllMocks() mockSend = jest.fn() - ;(S3Client as jest.Mock).mockImplementation(() => ({ - send: mockSend, - })) + ; (S3Client as jest.Mock).mockImplementation(() => ({ + send: mockSend, + })) }) describe('getObject', () => { @@ -104,12 +104,13 @@ describe('S3Backend', () => { expect(mockSend.mock.calls[2][0].constructor.name).toBe('DeleteObjectCommand') }) - test('should not throw if some individual fallback deletes fail', async () => { + test('should ignore NoSuchKey errors in the individual fallback', async () => { const notImplemented = Object.assign(new Error('NotImplemented'), { Code: 'NotImplemented' }) + const noSuchKey = Object.assign(new Error('NoSuchKey'), { Code: 'NoSuchKey' }) mockSend .mockRejectedValueOnce(notImplemented) .mockResolvedValueOnce({ $metadata: { httpStatusCode: 204 } }) - .mockRejectedValueOnce(new Error('AccessDenied')) + .mockRejectedValueOnce(noSuchKey) const backend = new S3Backend({ region: 'us-east-1', endpoint: 'http://localhost:9000' }) await expect( @@ -117,6 +118,20 @@ describe('S3Backend', () => { ).resolves.toBeUndefined() }) + test('should throw when an individual fallback delete fails with a real error', async () => { + const notImplemented = Object.assign(new Error('NotImplemented'), { Code: 'NotImplemented' }) + const accessDenied = Object.assign(new Error('AccessDenied'), { Code: 'AccessDenied' }) + mockSend + .mockRejectedValueOnce(notImplemented) + .mockResolvedValueOnce({ $metadata: { httpStatusCode: 204 } }) + .mockRejectedValueOnce(accessDenied) + + const backend = new S3Backend({ region: 'us-east-1', endpoint: 'http://localhost:9000' }) + await expect( + backend.deleteObjects('test-bucket', ['file1.txt', 'file2.txt']) + ).rejects.toThrow() + }) + test('should rethrow errors that are not NotImplemented', async () => { const err = Object.assign(new Error('AccessDenied'), { Code: 'AccessDenied' }) mockSend.mockRejectedValue(err)