From f95607fc1f79a73f52fd246af72c45c2f6119e41 Mon Sep 17 00:00:00 2001 From: kptdobe Date: Thu, 23 Jul 2026 08:17:35 +0200 Subject: [PATCH 1/2] fix: log R2 errors on source GET 500s getObject's catch block only logged when the S3/R2 error carried no httpStatusCode. Real backend failures (R2 returning a 500 InternalError) do carry one, so the most common case fell through silently, returning a bare 500 with nothing in Cloudflare Logs to diagnose it. Log whenever status >= 500, matching the convention already used in storage/version/put.js. Co-Authored-By: Claude Sonnet 5 --- src/storage/object/get.js | 6 ++---- test/storage/object/get.test.js | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/storage/object/get.js b/src/storage/object/get.js index 140f7945..127f5e86 100644 --- a/src/storage/object/get.js +++ b/src/storage/object/get.js @@ -74,12 +74,10 @@ export default async function getObject( etag: resp.ETag, }; } catch (e) { - if (!e.$metadata?.httpStatusCode) { - // eslint-disable-next-line no-console - console.error('Error getting object without httpStatusCode', e); - } // Handle conditional request failures (304 Not Modified, 412 Precondition Failed) const status = e.$metadata?.httpStatusCode || 500; + // eslint-disable-next-line no-console + if (status >= 500) console.error('Error getting object', e); if (status === 304 || status === 412) { // Include ETag in 304/412 responses per RFC 7232 return { diff --git a/test/storage/object/get.test.js b/test/storage/object/get.test.js index ec18a0b9..eb9a3d4c 100644 --- a/test/storage/object/get.test.js +++ b/test/storage/object/get.test.js @@ -128,4 +128,37 @@ describe('Get Object', () => { assert.strictEqual(resp.body, ''); assert.strictEqual(resp.contentLength, 0); }); + + it('logs the error when S3/R2 returns a 500 (get_failed path produces empty Cloudflare Logs)', async () => { + // Regression: a backend 500 that carries $metadata.httpStatusCode was never logged, + // so Cloudflare Logs was empty and the root cause was invisible in production. + const error = new Error('We encountered an internal error. Please try again.'); + error.$metadata = { httpStatusCode: 500 }; + s3Mock.on(GetObjectCommand, { + Bucket: BUCKET, + Key: S3_KEY, + }).rejects(error); + + const errors = []; + const origError = console.error; + console.error = (...args) => { + errors.push(args); + }; + let resp; + try { + // eslint-disable-next-line no-shadow + const getObject = (await import('../../../src/storage/object/get.js')).default; + resp = await getObject({}, { bucket: BUCKET, org: ORG, key: KEY }, false); + } finally { + console.error = origError; + } + + assert.strictEqual(resp.status, 500); + assert.strictEqual(resp.error, error.message); + assert(errors.length > 0, 'get_failed must log the error so it appears in Cloudflare Logs'); + assert( + errors[0].some((a) => a instanceof Error || typeof a === 'string'), + 'logged value must include the error or a message', + ); + }); }); From 1bce0b691c474e310993ffbf88b173f5bd501c12 Mon Sep 17 00:00:00 2001 From: kptdobe Date: Thu, 23 Jul 2026 08:43:34 +0200 Subject: [PATCH 2/2] fix: restore distinct log message for no-httpStatusCode errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keep the original 'Error getting object without httpStatusCode' message for network/timeout failures, alongside the new 'Error getting object' message for backend error responses — both now gated on status >= 500. Co-Authored-By: Claude Sonnet 5 --- src/storage/object/get.js | 4 +++- test/storage/object/get.test.js | 33 +++++++++++++++++++++++++++++---- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/storage/object/get.js b/src/storage/object/get.js index 127f5e86..f7294368 100644 --- a/src/storage/object/get.js +++ b/src/storage/object/get.js @@ -77,7 +77,9 @@ export default async function getObject( // Handle conditional request failures (304 Not Modified, 412 Precondition Failed) const status = e.$metadata?.httpStatusCode || 500; // eslint-disable-next-line no-console - if (status >= 500) console.error('Error getting object', e); + if (status >= 500) { + console.error(e.$metadata?.httpStatusCode ? 'Error getting object' : 'Error getting object without httpStatusCode', e); + } if (status === 304 || status === 412) { // Include ETag in 304/412 responses per RFC 7232 return { diff --git a/test/storage/object/get.test.js b/test/storage/object/get.test.js index eb9a3d4c..92e4e0e6 100644 --- a/test/storage/object/get.test.js +++ b/test/storage/object/get.test.js @@ -156,9 +156,34 @@ describe('Get Object', () => { assert.strictEqual(resp.status, 500); assert.strictEqual(resp.error, error.message); assert(errors.length > 0, 'get_failed must log the error so it appears in Cloudflare Logs'); - assert( - errors[0].some((a) => a instanceof Error || typeof a === 'string'), - 'logged value must include the error or a message', - ); + assert.strictEqual(errors[0][0], 'Error getting object'); + assert.strictEqual(errors[0][1], error); + }); + + it('logs a distinct message when the error has no httpStatusCode (network/timeout failure)', async () => { + const error = new Error('Network connection lost'); + s3Mock.on(GetObjectCommand, { + Bucket: BUCKET, + Key: S3_KEY, + }).rejects(error); + + const errors = []; + const origError = console.error; + console.error = (...args) => { + errors.push(args); + }; + let resp; + try { + // eslint-disable-next-line no-shadow + const getObject = (await import('../../../src/storage/object/get.js')).default; + resp = await getObject({}, { bucket: BUCKET, org: ORG, key: KEY }, false); + } finally { + console.error = origError; + } + + assert.strictEqual(resp.status, 500); + assert(errors.length > 0, 'must log the error so it appears in Cloudflare Logs'); + assert.strictEqual(errors[0][0], 'Error getting object without httpStatusCode'); + assert.strictEqual(errors[0][1], error); }); });