diff --git a/backend/src/routes/adminItemDrafts.ts b/backend/src/routes/adminItemDrafts.ts index cf4123a..1b99a7f 100644 --- a/backend/src/routes/adminItemDrafts.ts +++ b/backend/src/routes/adminItemDrafts.ts @@ -300,7 +300,20 @@ router.post( return res.status(404).json({ error: 'this photo has no original to restore' }); } - await restoreImageOriginal(Number(imageId)); + try { + await restoreImageOriginal(Number(imageId)); + } catch (err) { + // This precheck and restoreImageOriginal's own `original_image_path IS + // NOT NULL` guard can disagree under a race: two concurrent restores (or + // a double-click) can both pass the precheck before either commits, and + // the loser's UPDATE then matches zero rows and throws. That is not a + // sign anything is wrong — it means another request already did the + // restore, so the honest answer is the same 404 the precheck itself + // gives, not the generic 500 an uncaught throw would produce here. + console.error(`[drafts] restore for image ${imageId}:`, err); + return res.status(404).json({ error: 'this photo has no original to restore' }); + } + res.json(await imageOfItem(itemId, imageId)); }) ); diff --git a/backend/tests/integration/adminItemDrafts.integration.test.ts b/backend/tests/integration/adminItemDrafts.integration.test.ts index 89891f5..c808311 100644 --- a/backend/tests/integration/adminItemDrafts.integration.test.ts +++ b/backend/tests/integration/adminItemDrafts.integration.test.ts @@ -406,6 +406,25 @@ describe('the review queue’s background-removal control', () => { expect(res.body.original_image_path).toBeNull(); }); + // The precheck and restoreImageOriginal's own guard can disagree under a + // race. Whichever call loses, the answer must say "already done" rather than + // "something is wrong with this application". + it('does not answer 500 when two restores race', async () => { + await startStub(200, PNG_BYTES); + const { itemId, imageId } = await seedDraftWithImage(); + await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/remove-background` + ); + + const results = await Promise.all([ + request(app).post(`/api/admin/item-drafts/${itemId}/images/${imageId}/restore-original`), + request(app).post(`/api/admin/item-drafts/${itemId}/images/${imageId}/restore-original`) + ]); + + const statuses = results.map((r) => r.status).sort((a, b) => a - b); + expect(statuses).toEqual([200, 404]); + }); + // 502 rather than 500: the request was fine and the app is fine, and saying // which of the two failed is what stops somebody searching the application // logs for a fault that is not there.