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';