Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 3 additions & 15 deletions src/storage/version/put.js
Original file line number Diff line number Diff line change
Expand Up @@ -349,9 +349,7 @@ export async function putObjectWithVersion(
}

export async function postObjectVersionWithLabel(label, env, daCtx) {
const {
body, contentLength, contentType, status: currentStatus,
} = await getObject(env, daCtx);
const { body, contentLength, contentType } = 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;
Expand All @@ -368,18 +366,8 @@ export async function postObjectVersionWithLabel(label, env, daCtx) {

if (resp.status !== 200) return { status: resp.status };
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,
inferredType,
ext: daCtx.ext,
hadLabel: label != null,
currentStatus,
});
return { status: 500, error: 'Version was not created' };
// Binary content can't be versioned — not a server error, so 422 not 500.
return { status: 422, error: 'Version was not created: content type is not versionable' };
}
return { status: 201 };
}
Expand Down
34 changes: 10 additions & 24 deletions test/storage/version/put.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3414,11 +3414,15 @@ describe('Version Put', () => {
assert.ok(auditCalls[0].entry.versionId);
});

it('logs diagnostics and returns 500 when labelled version requested on non-versionable ext', async () => {
it('returns 422 when labelled version requested on non-versionable ext', async () => {
// Preserves the binary-never-version semantics: when the file extension does not map
// to a versionable mime (and the stored contentType is also not versionable), the
// labelled-version request still 500s and the diagnostic log fires with inferredType + ext
// captured so we can spot future legacy patterns in Cloudflare Logs.
// labelled-version request is rejected.
//
// Regression: this used to return 500, misclassifying an expected/permanent rejection
// (bulk media migrations calling POST /versionsource on jpg/png/pdf) as a server error.
// That inflated 5xx error-rate alerts with non-retryable, by-design rejections. 422
// (client asked for something that cannot be done) is correct; 500 is not.
const mockGetObject = async () => ({
body: 'binary content',
contentType: 'application/octet-stream',
Expand Down Expand Up @@ -3446,28 +3450,10 @@ describe('Version Put', () => {
bucket: 'b', org: 'o', site: 'mysite', key: 'mysite/data.bin', ext: 'bin', 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;
}
const resp = await postObjectVersionWithLabel('My Label', {}, daCtx);

assert.strictEqual(resp.status, 500);
assert.strictEqual(resp.error, 'Version was not created');
assert(errors.length > 0, 'diagnostic log must fire for the unhealed octet-stream path');
const payload = errors[0].find((a) => a && typeof a === 'object');
assert(payload, 'log must include a structured diagnostic payload');
assert.strictEqual(payload.contentType, 'application/octet-stream');
assert.strictEqual(payload.inferredType, 'application/octet-stream', 'inference must fall through for unknown ext');
assert.strictEqual(payload.ext, 'bin');
assert.strictEqual(payload.hadLabel, true);
assert.strictEqual(payload.currentStatus, 200);
assert.strictEqual(resp.status, 422);
assert.strictEqual(resp.error, 'Version was not created: content type is not versionable');
});
it('plain PUT (no label) still skips auto-version for non-html/json contentType', async () => {
// Companion regression: the labelled-path mime inference must NOT bleed into plain
Expand Down
Loading