fix(upload): answer client-caused chunk states with their own status code
An unknown, finished or expired upload id, and a complete call with chunks missing, were plain Errors with no statusCode, so both routes fell through to the blanket 500 and logged at error level. All four are the client's mistake: they read as a backend fault in monitoring and invite a retry that can never succeed. They now carry 404, 409, 410 and 400 respectively, and both routes pass a tagged status through instead of matching on the two they happened to know about. Only genuinely unexpected errors reach the 500 and the error log. No client is affected: uploadLargeFile, the only caller of this endpoint family, still has no callers of its own. Relates to issue 1403
This commit is contained in:
@@ -222,6 +222,33 @@ describe('chunked upload streams the body under a cap (#1403)', () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// These were plain Errors, so the routes answered 500 for what are plainly
|
||||||
|
// client mistakes — a backend fault in monitoring, and an invitation to
|
||||||
|
// retry something that can never succeed.
|
||||||
|
describe('client-caused states carry their own status code', () => {
|
||||||
|
it('404s an unknown upload id rather than 500', async () => {
|
||||||
|
await expect(chunkedUpload.uploadChunk('does-not-exist', 0, Buffer.alloc(10)))
|
||||||
|
.rejects.toMatchObject({ statusCode: 404 });
|
||||||
|
});
|
||||||
|
|
||||||
|
// 409 (wrong status) and 410 (expired) share uploadStateError with the two
|
||||||
|
// covered here. Reaching them from the public surface needs either a clock
|
||||||
|
// or a setter the service does not expose, and a test that pretends to
|
||||||
|
// exercise them while actually hitting the 404 path is worse than none.
|
||||||
|
|
||||||
|
it('404s completing an unknown upload rather than 500', async () => {
|
||||||
|
await expect(chunkedUpload.completeUpload('does-not-exist'))
|
||||||
|
.rejects.toMatchObject({ statusCode: 404 });
|
||||||
|
});
|
||||||
|
|
||||||
|
it('400s completing an upload that is missing chunks', async () => {
|
||||||
|
const { uploadId } = await init();
|
||||||
|
await chunkedUpload.uploadChunk(uploadId, 0, Buffer.alloc(10));
|
||||||
|
await expect(chunkedUpload.completeUpload(uploadId))
|
||||||
|
.rejects.toMatchObject({ statusCode: 400 });
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe('the happy path still works', () => {
|
describe('the happy path still works', () => {
|
||||||
it('writes a streamed chunk and reports progress', async () => {
|
it('writes a streamed chunk and reports progress', async () => {
|
||||||
const { uploadId } = await init();
|
const { uploadId } = await init();
|
||||||
|
|||||||
@@ -1714,7 +1714,10 @@ router.post('/:eventId/chunked-upload/:uploadId/chunk/:chunkIndex', adminAuth, r
|
|||||||
|
|
||||||
res.json(result);
|
res.json(result);
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
if (error.statusCode === 413 || error.statusCode === 400) {
|
// Client-caused states (unknown/finished/expired upload, bad index, too
|
||||||
|
// large) carry their own status. Only a genuinely unexpected error should
|
||||||
|
// reach the 500 below and the error log with it.
|
||||||
|
if (error.statusCode) {
|
||||||
return res.status(error.statusCode).json({ error: error.message });
|
return res.status(error.statusCode).json({ error: error.message });
|
||||||
}
|
}
|
||||||
logger.error('Error uploading chunk:', error);
|
logger.error('Error uploading chunk:', error);
|
||||||
@@ -1764,8 +1767,10 @@ router.post('/:eventId/chunked-upload/:uploadId/complete', adminAuth, requirePer
|
|||||||
photos: uploadedPhotos
|
photos: uploadedPhotos
|
||||||
});
|
});
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
if (error.statusCode === 413) {
|
// Same rule as the chunk route: a tagged status is a client-caused state
|
||||||
return res.status(413).json({ error: error.message });
|
// (unknown/expired upload, missing chunks), not a server fault.
|
||||||
|
if (error.statusCode) {
|
||||||
|
return res.status(error.statusCode).json({ error: error.message });
|
||||||
}
|
}
|
||||||
logger.error('Error completing chunked upload:', error);
|
logger.error('Error completing chunked upload:', error);
|
||||||
res.status(500).json({ error: error.message || 'Failed to complete upload' });
|
res.status(500).json({ error: error.message || 'Failed to complete upload' });
|
||||||
|
|||||||
@@ -41,6 +41,17 @@ function overAllowanceError() {
|
|||||||
return Object.assign(new Error('CHUNK_OVER_ALLOWANCE'), { overAllowance: true });
|
return Object.assign(new Error('CHUNK_OVER_ALLOWANCE'), { overAllowance: true });
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// A client-supplied upload id that is unknown, finished or expired is the
|
||||||
|
// client's mistake, not the server's. These used to be plain Errors, so the
|
||||||
|
// routes answered 500 — which reads as a backend fault in monitoring and
|
||||||
|
// invites the client to retry something that will never succeed.
|
||||||
|
function uploadStateError(message, statusCode) {
|
||||||
|
const err = new Error(message);
|
||||||
|
err.code = 'UPLOAD_STATE';
|
||||||
|
err.statusCode = statusCode;
|
||||||
|
return err;
|
||||||
|
}
|
||||||
|
|
||||||
function invalidChunkError(message) {
|
function invalidChunkError(message) {
|
||||||
const err = new Error(message);
|
const err = new Error(message);
|
||||||
err.code = 'INVALID_CHUNK';
|
err.code = 'INVALID_CHUNK';
|
||||||
@@ -209,17 +220,17 @@ async function uploadChunk(uploadId, chunkIndex, source, { declaredBytes } = {})
|
|||||||
const uploadMeta = activeUploads.get(uploadId);
|
const uploadMeta = activeUploads.get(uploadId);
|
||||||
|
|
||||||
if (!uploadMeta) {
|
if (!uploadMeta) {
|
||||||
throw new Error('Upload not found or expired');
|
throw uploadStateError('Upload not found or expired', 404);
|
||||||
}
|
}
|
||||||
|
|
||||||
if (uploadMeta.status !== 'in_progress') {
|
if (uploadMeta.status !== 'in_progress') {
|
||||||
throw new Error(`Upload is ${uploadMeta.status}`);
|
throw uploadStateError(`Upload is ${uploadMeta.status}`, 409);
|
||||||
}
|
}
|
||||||
|
|
||||||
// Check expiration
|
// Check expiration
|
||||||
if (Date.now() > uploadMeta.expiresAt) {
|
if (Date.now() > uploadMeta.expiresAt) {
|
||||||
await abortUpload(uploadId);
|
await abortUpload(uploadId);
|
||||||
throw new Error('Upload expired');
|
throw uploadStateError('Upload expired', 410);
|
||||||
}
|
}
|
||||||
|
|
||||||
// Only the announced chunk indices are valid — anything else would merge
|
// Only the announced chunk indices are valid — anything else would merge
|
||||||
@@ -320,12 +331,13 @@ async function completeUpload(uploadId) {
|
|||||||
const uploadMeta = activeUploads.get(uploadId);
|
const uploadMeta = activeUploads.get(uploadId);
|
||||||
|
|
||||||
if (!uploadMeta) {
|
if (!uploadMeta) {
|
||||||
throw new Error('Upload not found or expired');
|
throw uploadStateError('Upload not found or expired', 404);
|
||||||
}
|
}
|
||||||
|
|
||||||
// Verify all chunks received
|
// Verify all chunks received
|
||||||
if (uploadMeta.receivedChunks.size !== uploadMeta.expectedChunks) {
|
if (uploadMeta.receivedChunks.size !== uploadMeta.expectedChunks) {
|
||||||
throw new Error(`Missing chunks: received ${uploadMeta.receivedChunks.size} of ${uploadMeta.expectedChunks}`);
|
throw uploadStateError(
|
||||||
|
`Missing chunks: received ${uploadMeta.receivedChunks.size} of ${uploadMeta.expectedChunks}`, 400);
|
||||||
}
|
}
|
||||||
|
|
||||||
uploadMeta.status = 'merging';
|
uploadMeta.status = 'merging';
|
||||||
|
|||||||
Reference in New Issue
Block a user