From f3fbdc39cffa499552c390f6ee73dc592e371cc3 Mon Sep 17 00:00:00 2001 From: kptdobe Date: Mon, 4 May 2026 11:24:44 +0200 Subject: [PATCH] fix: handle NoSuchKey error name in copyFile catch block When R2 throws a NoSuchKey error without $metadata.httpStatusCode, the 404 check was silently failing, causing the error to propagate as a rejected promise in moveObject and returning a 500 partial_failure response. Now also checks e.name === 'NoSuchKey' and returns a normalised 404 metadata object so moveObject treats missing source keys as expected skips. Co-Authored-By: Claude Sonnet 4.6 --- src/storage/object/copy.js | 4 +-- test/storage/object/copy.test.js | 44 ++++++++++++++++++++++++++++++++ test/storage/object/move.test.js | 34 ++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 2 deletions(-) diff --git a/src/storage/object/copy.js b/src/storage/object/copy.js index 98e73e6f..fb4fc83f 100644 --- a/src/storage/object/copy.js +++ b/src/storage/object/copy.js @@ -115,8 +115,8 @@ export const copyFile = async (config, env, daCtx, sourceKey, details, isRename) const client = new S3Client(config); // This is a move so copy to the new location return /* await */ client.send(new CopyObjectCommand(input)); - } else if (e.$metadata?.httpStatusCode === 404) { - return { $metadata: e.$metadata }; + } else if (e.$metadata?.httpStatusCode === 404 || e.name === 'NoSuchKey') { + return { $metadata: { httpStatusCode: 404 } }; } throw e; } finally { diff --git a/test/storage/object/copy.test.js b/test/storage/object/copy.test.js index f85fa0c9..5a73a0a7 100644 --- a/test/storage/object/copy.test.js +++ b/test/storage/object/copy.test.js @@ -696,6 +696,50 @@ describe('Object copy', () => { assert.strictEqual(puwv[0].u.body, preBuffered, 'non-ReadableStream body must be passed through unchanged'); }); + it('returns 404 when CopyObjectCommand throws NoSuchKey with no $metadata.httpStatusCode', async () => { + const error = new Error('The specified key does not exist.'); + error.name = 'NoSuchKey'; + + const mockS3Client = class { + // eslint-disable-next-line class-methods-use-this + send() { + throw error; + } + + middlewareStack = { add: () => {} }; + }; + + const mockGetObject = async (env, { bucket, org, key }, head) => { + if (head && bucket === 'test-bucket' && org === 'testorg' && key === 'src/missing.html') { + return { contentType: 'text/html', status: 200, contentLength: 0 }; + } + return null; + }; + + // eslint-disable-next-line no-shadow + const { copyFile } = await esmock('../../../src/storage/object/copy.js', { + '@aws-sdk/client-s3': { + S3Client: mockS3Client, + }, + '../../../src/storage/object/get.js': { + default: mockGetObject, + }, + }); + + const env = { dacollab: { fetch: () => ({ body: { cancel: () => {} } }) } }; + const daCtx = { + bucket: 'test-bucket', + org: 'testorg', + origin: 'https://test.com', + users: [{ email: 'test@example.com' }], + }; + daCtx.aclCtx = await getAclCtx(env, daCtx.org, daCtx.users, '/'); + const details = { source: 'src', destination: 'dst' }; + + const resp = await copyFile({}, env, daCtx, 'src/missing.html', details, true); + assert.deepStrictEqual(resp, { $metadata: { httpStatusCode: 404 } }); + }); + it('Copy content when origin does not exists', async () => { const error = { $metadata: { httpStatusCode: 404, hi: 'ha' }, diff --git a/test/storage/object/move.test.js b/test/storage/object/move.test.js index 20793876..24fefa1e 100644 --- a/test/storage/object/move.test.js +++ b/test/storage/object/move.test.js @@ -252,6 +252,40 @@ describe('Move', () => { ); }); + it('Returns 204 (not 500) when some source keys do not exist (NoSuchKey)', async () => { + mockSendFn = () => ({ Contents: [{ Key: 'myorg/somewhere/b.html' }] }); + + const copyFileCalled = []; + const copyFile = (c, e, x, k) => { + copyFileCalled.push(k); + return { $metadata: { httpStatusCode: 404 } }; + }; + + const moveObject = await esmock('../../../src/storage/object/move.js', { + '@aws-sdk/client-s3': { S3Client: MockS3Client }, + '../../../src/storage/object/copy.js': { copyFile }, + '../../../src/storage/object/delete.js': { deleteObject: () => ({ status: 204 }) }, + }); + + const pathLookup = new Map(); + pathLookup.set('blah@foo.org', [ + { path: '/somewhere/+**', actions: ['read', 'write'] }, + { path: '/somedest/+**', actions: ['read', 'write'] }, + ]); + const ctx = { + org: 'myorg', + aclCtx: { pathLookup }, + users: [{ email: 'blah@foo.org' }], + isFile: false, + key: 'q.html', + }; + const resp = await moveObject({}, ctx, { source: 'somewhere', destination: 'somedest' }); + + assert.strictEqual(resp.status, 204); + assert(copyFileCalled.includes('somewhere'), 'copyFile must be called for the source key'); + assert(copyFileCalled.includes('somewhere/b.html'), 'copyFile must be called for listed keys'); + }); + it('Does not re-process page 1 keys on page 2 iteration', async () => { let callCount = 0; mockSendFn = () => {