From 5129112266e1740b9e093717c6c2fbbb527c22de Mon Sep 17 00:00:00 2001
From: Thom Lamb <thomlamb@gmail.com>
Date: Thu, 3 Sep 2026 12:56:15 -0500
Subject: [PATCH] fix(admin): answer 404, not 500, when two restores race
 (#281)

restore-original's precheck (existing.original_image_path === null) and restoreImageOriginal's own guard (WHERE ... AND original_image_path IS NOT NULL) could disagree under a race: two concurrent restores, or a rapid double-click, could both pass the precheck before either commits, and the loser's UPDATE would then match zero rows and throw. The handler had no try/catch around that call, so the throw propagated through asyncRoute to the app-level error handler and the caller got a bare 500, breaking the route's documented 200 | 404 contract even though the row itself was left correct.

Wraps the restoreImageOriginal call in a try/catch, matching the shape remove-background already uses in this file, but answering 404 rather than 502: losing this race means another admin already finished the restore, not that a downstream service failed. Adds a comment on the catch explaining why it exists, and a test that fires two restores concurrently and asserts neither comes back 500.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 backend/src/routes/adminItemDrafts.ts         | 15 ++++++++++++++-
 .../adminItemDrafts.integration.test.ts       | 19 +++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

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.