From bb398e4575a0acfc905174682cf2e30e953cf857 Mon Sep 17 00:00:00 2001 From: kptdobe Date: Fri, 22 May 2026 09:39:59 +0200 Subject: [PATCH] fix(version): log diagnostics on POST /versionsource silent 500 When postObjectVersionWithLabel reaches the `return 500, 'Version was not created'` branch, no console.error is emitted, so the failure arrives in Cloudflare Logs with empty Logs[] / Exceptions[]. Forensics on a recent cluster of these failures on legacy HTML imports traced them to source objects missing ContentType metadata: shouldCreateVersion(undefined) is false, putObjectWithVersion returns { status: 200, versionCreated: false }, and the silent 500 falls out. Adds a single, scoped diagnostic log capturing contentType, hadLabel, and the source-object status so the next log review can confirm the contentType-missing theory without further code archaeology. Test asserts the silent-500 path now emits a structured log row with the expected fields when the source contentType is undefined. Co-Authored-By: Paperclip --- src/storage/version/put.js | 17 +++++++-- test/storage/version/put.test.js | 62 ++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/src/storage/version/put.js b/src/storage/version/put.js index 0150383c..2b432844 100644 --- a/src/storage/version/put.js +++ b/src/storage/version/put.js @@ -314,7 +314,9 @@ export async function putObjectWithVersion( } export async function postObjectVersionWithLabel(label, env, daCtx) { - const { body, contentLength, contentType } = await getObject(env, daCtx); + const { + body, contentLength, contentType, status: currentStatus, + } = await getObject(env, daCtx); // Buffer the ReadableStream so the body survives retries inside putObjectWithVersion. // A ReadableStream can only be consumed once; ArrayBuffer can be reused freely. const bodyBuffer = body instanceof ReadableStream ? await new Response(body).arrayBuffer() : body; @@ -325,7 +327,18 @@ export async function postObjectVersionWithLabel(label, env, daCtx) { }, true); if (resp.status !== 200) return { status: resp.status }; - if (!resp.versionCreated) return { status: 500, error: 'Version was not created' }; + if (!resp.versionCreated) { + // Diagnostic: the silent-500 path lands here when the source object's contentType + // is not html/json (e.g. legacy imports with missing ContentType metadata) so + // shouldCreateVersion gates out and no version is written. + // eslint-disable-next-line no-console + console.error('Failed to version (no version created)', { + contentType, + hadLabel: label != null, + currentStatus, + }); + return { status: 500, error: 'Version was not created' }; + } return { status: 201 }; } diff --git a/test/storage/version/put.test.js b/test/storage/version/put.test.js index 9583826c..df42459b 100644 --- a/test/storage/version/put.test.js +++ b/test/storage/version/put.test.js @@ -3162,6 +3162,68 @@ describe('Version Put', () => { assert.strictEqual(resp.status, 201, 'must return 201 when version already exists (concurrent 412)'); }); + it('logs diagnostics when versionCreated is false (silent 500 path)', async () => { + // Regression: legacy HTML imports can land in storage with no S3 + // ContentType metadata. shouldCreateVersion(undefined) returns false -> + // shouldCreateVersionObject is false -> putObjectWithVersion returns + // { status: 200, versionCreated: false } -> postObjectVersionWithLabel + // returns a silent 500 with empty Cloudflare Logs (no console.error). + // The fix adds a single diagnostic log capturing contentType / hadLabel / + // currentStatus so the next daily review can confirm root cause. + const mockGetObject = async () => ({ + body: 'doc content', + contentType: undefined, + contentLength: 200, + status: 200, + metadata: { id: 'doc-id', version: 'v1' }, + etag: '"etag1"', + }); + + const mainClient = { + async send() { return { $metadata: { httpStatusCode: 200 } }; }, + }; + const versionClient = { + async send() { return { $metadata: { httpStatusCode: 200 } }; }, + }; + + const { postObjectVersionWithLabel } = await esmock('../../../src/storage/version/put.js', { + '../../../src/storage/object/get.js': { default: mockGetObject }, + '../../../src/storage/utils/version.js': { + ifNoneMatch: () => versionClient, + ifMatch: () => mainClient, + }, + '../../../src/storage/version/audit.js': { + writeAuditEntry: async () => ({ status: 200 }), + }, + '../../../src/storage/utils/config.js': { default: () => ({}) }, + }); + + const daCtx = { + bucket: 'b', org: 'o', key: 'doc.html', ext: 'html', users: [], + }; + + const errors = []; + const origError = console.error; + console.error = (...args) => { + errors.push(args); + }; + let resp; + try { + resp = await postObjectVersionWithLabel('My Label', {}, daCtx); + } finally { + console.error = origError; + } + + assert.strictEqual(resp.status, 500); + assert.strictEqual(resp.error, 'Version was not created'); + assert(errors.length > 0, 'silent-500 path must emit a console.error so Cloudflare Logs is non-empty'); + const payload = errors[0].find((a) => a && typeof a === 'object'); + assert(payload, 'log must include a structured diagnostic payload'); + assert.strictEqual(payload.contentType, undefined, 'payload must record contentType (undefined when source metadata is missing)'); + assert.strictEqual(payload.hadLabel, true, 'payload must record whether a label was supplied'); + assert.strictEqual(payload.currentStatus, 200, 'payload must record the source-object status'); + }); + it('returns 201 when version client body is a ReadableStream that would be disturbed by SDK retry', async () => { // Regression test: putVersion passes current.body (a ReadableStream from getObject) to // PutObjectCommand. The AWS SDK retries on network failures using the same stream — but a