From 48423c95e6df2f4f3b0b2b428c1110ad9054a682 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Fri, 11 Sep 2026 09:30:09 +0200 Subject: [PATCH] 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 --- .../services/chunkedUploadStreaming.test.js | 27 +++++++++++++++++++ backend/src/routes/adminPhotos.js | 11 +++++--- backend/src/services/chunkedUploadService.js | 22 +++++++++++---- 3 files changed, 52 insertions(+), 8 deletions(-) diff --git a/backend/__tests__/services/chunkedUploadStreaming.test.js b/backend/__tests__/services/chunkedUploadStreaming.test.js index 38fccb51..ad3fb40d 100644 --- a/backend/__tests__/services/chunkedUploadStreaming.test.js +++ b/backend/__tests__/services/chunkedUploadStreaming.test.js @@ -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', () => { it('writes a streamed chunk and reports progress', async () => { const { uploadId } = await init(); diff --git a/backend/src/routes/adminPhotos.js b/backend/src/routes/adminPhotos.js index 09f46c81..a9557b5d 100644 --- a/backend/src/routes/adminPhotos.js +++ b/backend/src/routes/adminPhotos.js @@ -1714,7 +1714,10 @@ router.post('/:eventId/chunked-upload/:uploadId/chunk/:chunkIndex', adminAuth, r res.json(result); } 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 }); } logger.error('Error uploading chunk:', error); @@ -1764,8 +1767,10 @@ router.post('/:eventId/chunked-upload/:uploadId/complete', adminAuth, requirePer photos: uploadedPhotos }); } catch (error) { - if (error.statusCode === 413) { - return res.status(413).json({ error: error.message }); + // Same rule as the chunk route: a tagged status is a client-caused state + // (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); res.status(500).json({ error: error.message || 'Failed to complete upload' }); diff --git a/backend/src/services/chunkedUploadService.js b/backend/src/services/chunkedUploadService.js index 6b11b4f3..3ca14df1 100644 --- a/backend/src/services/chunkedUploadService.js +++ b/backend/src/services/chunkedUploadService.js @@ -41,6 +41,17 @@ function overAllowanceError() { 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) { const err = new Error(message); err.code = 'INVALID_CHUNK'; @@ -209,17 +220,17 @@ async function uploadChunk(uploadId, chunkIndex, source, { declaredBytes } = {}) const uploadMeta = activeUploads.get(uploadId); if (!uploadMeta) { - throw new Error('Upload not found or expired'); + throw uploadStateError('Upload not found or expired', 404); } if (uploadMeta.status !== 'in_progress') { - throw new Error(`Upload is ${uploadMeta.status}`); + throw uploadStateError(`Upload is ${uploadMeta.status}`, 409); } // Check expiration if (Date.now() > uploadMeta.expiresAt) { await abortUpload(uploadId); - throw new Error('Upload expired'); + throw uploadStateError('Upload expired', 410); } // Only the announced chunk indices are valid — anything else would merge @@ -320,12 +331,13 @@ async function completeUpload(uploadId) { const uploadMeta = activeUploads.get(uploadId); if (!uploadMeta) { - throw new Error('Upload not found or expired'); + throw uploadStateError('Upload not found or expired', 404); } // Verify all chunks received 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';