fix(admin): narrow the restore-original catch to the race it exists for (#281)

Round 1 caught every throw from restoreImageOriginal() and reported it as a 404, on the theory that losing the concurrent-restore race is the only way that call fails. But the throw carried nothing to distinguish that race from a genuinely different failure during the same UPDATE — a dropped database connection, a transient outage — so a real failure was now silently reinterpreted as "someone already restored this" instead of surfacing as the loud 500 it was before.

backend/src/intake/backgroundRemoval.ts now exports NoOriginalToRestoreError, a named subclass of Error thrown in place of the bare Error restoreImageOriginal previously threw. The message text is unchanged, so backgroundRemoval.integration.test.ts's rejects.toThrow(/no original/) assertion keeps passing without modification.

backend/src/routes/adminItemDrafts.ts catches that class specifically in the restore-original handler and rethrows anything else, so a real failure still reaches the app-level error handler and comes back as a 500 instead of being mislabeled as "already done".

backend/tests/integration/adminItemDrafts.integration.test.ts adds a test that spies on restoreImageOriginal via jest.spyOn on the module namespace (the project compiles to CommonJS, so the route's call site reads the export off that object at call time, which makes the spy effective without jest.mock) to reject once with a plain Error, and asserts the response is 500 rather than 404 — proving the narrowing changes real behavior, not just internal structure. The spy is restored in a finally block.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

backend/src/intake/backgroundRemoval.ts

@@ -23,6 +23,17 @@ interface ImageRow {
   original_image_path: string | null;
 }
 
+/**
+ * Thrown when there is no original to put back.
+ *
+ * A named class rather than a bare Error because the route has to tell this
+ * apart from a database that is not answering. The two need opposite replies:
+ * this one means another admin has already restored the photo and the work is
+ * done, which is a 404; anything else means the application is in trouble and
+ * must stay loud rather than being reported as "already done".
+ */
+export class NoOriginalToRestoreError extends Error {}
+
 /**
  * The path a cut-out of `imagePath` is written to.
  *
@@ -114,7 +125,7 @@ export async function restoreImageOriginal(imageId: number): Promise<void> {
     [imageId]
   );
   if (rowCount === 0) {
-    throw new Error(`image ${imageId} has no original to restore`);
+    throw new NoOriginalToRestoreError(`image ${imageId} has no original to restore`);
   }
 }
 

backend/src/routes/adminItemDrafts.ts

@@ -3,7 +3,11 @@ import { pool } from '../db';
 import { asyncRoute } from '../asyncRoute';
 import { draftQueued } from '../intake/draftingWorker';
 import { nextPriceSource, PriceSource } from '../intake/priceSource';
-import { removeImageBackground, restoreImageOriginal } from '../intake/backgroundRemoval';
+import {
+  NoOriginalToRestoreError,
+  removeImageBackground,
+  restoreImageOriginal,
+} from '../intake/backgroundRemoval';
 import { isRembgConfigured } from '../intake/rembgClient';
 
 const router = Router();
@@ -303,13 +307,18 @@ router.post(
     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.
+      // Narrow on purpose: only NoOriginalToRestoreError means "another
+      // request already did this, the work is done". 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 specific error. Any
+      // other failure (a dropped connection, a transient outage) must not be
+      // reported the same way — it needs to stay loud as a 500, so it is
+      // rethrown here for asyncRoute's app-level handler to catch.
+      if (!(err instanceof NoOriginalToRestoreError)) {
+        throw err;
+      }
       console.error(`[drafts] restore for image ${imageId}:`, err);
       return res.status(404).json({ error: 'this photo has no original to restore' });
     }

backend/tests/integration/adminItemDrafts.integration.test.ts

@@ -7,6 +7,7 @@ import { AddressInfo } from 'net';
 import { promises as fsp } from 'fs';
 import os from 'os';
 import path from 'path';
+import * as backgroundRemoval from '../../src/intake/backgroundRemoval';
 
 beforeEach(async () => {
   await resetDb();
@@ -425,6 +426,30 @@ describe('the review queue’s background-removal control', () => {
     expect(statuses).toEqual([200, 404]);
   });
 
+  // The catch around restoreImageOriginal exists only for the race above, not
+  // for every failure. A different kind of throw — a dropped connection, a
+  // transient outage — must not be reinterpreted as "already restored"; it has
+  // to stay a loud 500 so it reaches the app-level error handler.
+  it('stays a 500, not a 404, when restoreImageOriginal fails for a reason other than the 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 spy = jest
+      .spyOn(backgroundRemoval, 'restoreImageOriginal')
+      .mockRejectedValueOnce(new Error('database is down'));
+    try {
+      const res = await request(app).post(
+        `/api/admin/item-drafts/${itemId}/images/${imageId}/restore-original`
+      );
+      expect(res.status).toBe(500);
+    } finally {
+      spy.mockRestore();
+    }
+  });
+
   // 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.