fix(backgrounds): report a partial restore instead of throwing (#293)

restoreOriginalsForItem rethrew anything that was not NoOriginalToRestoreError, which handed asyncRoute a bare 500 and discarded how far the restore had already got. That breaks the invariant the feature is built on: photos restored before the failure really are back, and an admin standing in front of the modal needs the count to decide whether pressing the button again is worth anything. RestoreSummary now carries `failed` and the loop stops and reports, exactly the shape and the reasoning removeBackgroundsForItem already had.

The restore-originals route gains the missing 404 for an item that does not exist — remove-backgrounds always had it, and the two handlers are copy-paste rather than a shared helper, so nothing would have caught them diverging. draftingWorker's .catch is now only reachable if the image-listing query itself throws, since removeBackgroundsForItem no longer rejects over a single photo; its comment says so rather than describing behaviour that has moved.

The `failed` branch is covered by a unit test that stubs the database module in its own module registry. It cannot honestly be an integration test: the only failure the function can report is a database fault, and the only way to inject one into a real run is to interfere with the single pool every integration suite in the --runInBand process shares and that afterAll calls pool.end() on. Two tests that did exactly that are removed here — they left the suite reporting a failure against its own afterAll and leaking a handle that stopped it exiting. Nor is the fault reachable through data alone: the swap's WHERE original_image_path IS NOT NULL guarantees the value it writes into the NOT NULL image_path, and item_images carries no unique, check or foreign-key constraint on either column, so no row can be seeded that makes the statement fail.

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

backend/src/intake/backgroundRemoval.ts

@@ -189,14 +189,19 @@ export interface RemovalSummary {
 /**
  * What a whole-item restore did.
  *
- * No `failed`, because restoring cannot fail the way removing can: it is a
- * database swap with no sidecar in it, and a photo that was never cut out is
- * skipped rather than being an error.
+ * Carries `failed` for the same reason `RemovalSummary` does. Restoring is a
+ * database swap with no sidecar in it, so it fails far less often than
+ * removing does — but "far less often" is not "never", and a database error
+ * partway through a four-photo restore is exactly the moment an admin needs
+ * the count rather than a bare 500. A photo that was never cut out is skipped
+ * rather than being an error either way.
  */
 export interface RestoreSummary {
   total: number;
   /** How many were put back. Photos that were never cut out are not counted. */
   restored: number;
+  /** Whether it stopped early because one of them failed. */
+  failed: boolean;
 }
 
 /** Every photo of one item, in order. */
@@ -252,6 +257,26 @@ export async function removeBackgroundsForItem(itemId: number): Promise<RemovalS
  * A photo that was never cut out is skipped rather than refused — the mixed
  * state a partial removal leaves behind has to be restorable too, and half an
  * item is exactly when somebody reaches for this.
+ *
+ * Stops at the first genuine failure and reports the count, the same shape
+ * `removeBackgroundsForItem` uses and for the same reason: a caller standing
+ * in front of the screen needs to know how far it got, and rethrowing here
+ * would discard that in favour of a bare 500. `NoOriginalToRestoreError` is
+ * not a genuine failure — it is skipped, as before — so it never reaches this
+ * stop.
+ *
+ * The `failed` path has no integration test, deliberately. The only failure it
+ * can report is a database fault, and the only way to inject one into a real
+ * run is to interfere with the single pool every integration suite in the
+ * `--runInBand` process shares — the same pool `afterAll` calls `pool.end()`
+ * on. Tests that did exactly that left the suite reporting a failure against
+ * its own `afterAll` and leaking a handle that stopped it exiting. Nor is the
+ * fault reachable through data alone: the swap's `WHERE original_image_path IS
+ * NOT NULL` guarantees the value it writes into the `NOT NULL` `image_path`,
+ * and `item_images` carries no unique, check or foreign-key constraint on
+ * either column, so no row can be seeded that makes the statement fail. The
+ * branch is covered instead by `tests/unit/backgroundRemoval.test.ts`, which
+ * stubs the database module in its own module registry and shares nothing.
  */
 export async function restoreOriginalsForItem(itemId: number): Promise<RestoreSummary> {
   const imageIds = await imageIdsFor(itemId);
@@ -262,10 +287,14 @@ export async function restoreOriginalsForItem(itemId: number): Promise<RestoreSu
       await restoreImageOriginal(imageId);
       restored += 1;
     } catch (err) {
-      // Only "there was nothing to restore" is skipped. Anything else is a real
-      // failure and belongs to the caller.
-      if (!(err instanceof NoOriginalToRestoreError)) throw err;
+      // Only "there was nothing to restore" is skipped. Anything else is a
+      // real failure, logged for the same reason removeBackgroundsForItem
+      // logs rather than throws: the caller gets the count, which is the
+      // thing it can act on, and the reason belongs in the log.
+      if (err instanceof NoOriginalToRestoreError) continue;
+      console.error(`[background-removal] restoring item ${itemId}, image ${imageId}:`, err);
+      return { total: imageIds.length, restored, failed: true };
     }
   }
-  return { total: imageIds.length, restored };
+  return { total: imageIds.length, restored, failed: false };
 }

backend/src/intake/draftingWorker.ts

@@ -165,8 +165,16 @@ export async function draftQueued(limit = DEFAULT_BATCH): Promise<SweepResult> {
       //
       // This is the sender's tick from the submission page, honoured here so
       // they never waited for it — and a failure must not mark a draft that was
-      // written correctly as failed. The photo keeps its original in that case,
-      // and the admin's per-photo control is still there to do it by hand.
+      // written correctly as failed.
+      //
+      // removeBackgroundsForItem no longer rejects over a single photo failing
+      // (#293 gave it a summary instead, for the admin screen that acts on the
+      // count) — so this .catch now fires only if the image-listing query
+      // itself throws, which is rare enough to warrant a log and nothing more.
+      // A per-photo failure comes back as `failed: true` in the summary, which
+      // this sweep discards; the photo keeps its original in that case, and the
+      // admin's per-photo control in the review queue is still there to do it
+      // by hand.
       //
       // Awaited, unlike the notification below, so a sweep that has returned
       // has finished its work. Nothing is waiting on this: the worker is off

backend/src/routes/admin.ts

@@ -352,7 +352,7 @@ router.post('/items/:id/mark-available', asyncRoute(async (req: Request, res: Re
 }));
 
 /**
- * One item's images, or null when the item does not exist.
+ * Whether an item with this id exists.
  *
  * Checked before acting so an absent item is a 404 rather than a cheerful
  * summary of nothing. `removeBackgroundsForItem` would happily report
@@ -396,6 +396,11 @@ router.post('/items/:id/remove-backgrounds', asyncRoute(async (req: Request, res
  * The reason removing is safe to try. Photos that were never cut out are
  * skipped rather than refused, so a half-done item — what a partial failure
  * leaves behind — is restorable too.
+ *
+ * Answers 200 once the id is valid, same as remove-backgrounds and for the
+ * same reason: `restoreOriginalsForItem` stops at the first genuine failure
+ * rather than throwing, so there is always a summary to return, never a bare
+ * 500 that discards how far it got.
  */
 router.post('/items/:id/restore-originals', asyncRoute(async (req: Request, res: Response) => {
   const itemId = readId(req.params.id);

backend/tests/integration/adminItemBackgrounds.integration.test.ts

@@ -142,7 +142,7 @@ describe('restoring every original on an item', () => {
     const res = await request(app).post(`/api/admin/items/${itemId}/restore-originals`);
 
     expect(res.status).toBe(200);
-    expect(res.body).toEqual({ total: 2, restored: 2 });
+    expect(res.body).toEqual({ total: 2, restored: 2, failed: false });
   });
 
   it('answers 404 for an id that cannot be read', async () => {
@@ -150,6 +150,15 @@ describe('restoring every original on an item', () => {
 
     expect((await request(app).post('/api/admin/items/abc/restore-originals')).status).toBe(404);
   });
+
+  // remove-backgrounds has always had this case; restore-originals did not,
+  // and the two handlers are copy-paste rather than a shared helper — nothing
+  // would have caught them diverging.
+  it('answers 404 for an item that does not exist', async () => {
+    await startStub(answerWithPng);
+
+    expect((await request(app).post('/api/admin/items/999999/restore-originals')).status).toBe(404);
+  });
 });
 
 // Extra scope beyond the endpoints themselves: Task 3 derives its button label

backend/tests/integration/backgroundRemoval.integration.test.ts

@@ -381,7 +381,7 @@ describe('putting every original back', () => {
 
     const summary = await restoreOriginalsForItem(itemId);
 
-    expect(summary).toEqual({ total: 2, restored: 2 });
+    expect(summary).toEqual({ total: 2, restored: 2, failed: false });
 
     const { rows: after } = await pool.query<{ image_path: string }>(
       `SELECT image_path FROM item_images WHERE item_id = $1 ORDER BY sort_order`,
@@ -412,6 +412,6 @@ describe('putting every original back', () => {
     );
     await removeImageBackground(images[0]!.id);
 
-    expect(await restoreOriginalsForItem(itemId)).toEqual({ total: 2, restored: 1 });
+    expect(await restoreOriginalsForItem(itemId)).toEqual({ total: 2, restored: 1, failed: false });
   });
 });

backend/tests/unit/backgroundRemoval.test.ts

@@ -1,4 +1,41 @@
-import { cutoutPathFor } from '../../src/intake/backgroundRemoval';
+import { pool } from '../../src/db';
+import { cutoutPathFor, restoreOriginalsForItem } from '../../src/intake/backgroundRemoval';
+
+// The database module is replaced outright rather than spied on. This has to
+// be a unit test: the only failure `restoreOriginalsForItem` can report is a
+// database fault, and the only way to inject one is to make a query fail on
+// command. Doing that in the integration suite means interfering with the
+// single pool every suite in the same `--runInBand` process shares, and that
+// `afterAll` calls `pool.end()` on — which made the whole suite unrunnable.
+// Here no `Pool` is ever constructed: jest gives this file its own module
+// registry, so there is nothing shared to break.
+jest.mock('../../src/db', () => ({ pool: { query: jest.fn(), connect: jest.fn() } }));
+
+const mockPool = pool as unknown as { query: jest.Mock; connect: jest.Mock };
+
+/** The distinctive text of the swap that puts one photo back. */
+const SWAP_SQL = 'SET image_path = original_image_path';
+
+/**
+ * A pool over `imageIds` whose nth restore swap fails.
+ *
+ * Matching on the swap's SQL rather than counting queries: `restoreImageOriginal`
+ * also issues BEGIN, the `item_drafts` update and COMMIT on the same client, so
+ * a bare call counter would break whichever query happened to land nth.
+ */
+function poolWhoseSwapFailsOn(imageIds: number[], failOnSwap: number): void {
+  mockPool.query.mockResolvedValue({ rows: imageIds.map((id) => ({ id })) });
+  let swaps = 0;
+  mockPool.connect.mockImplementation(async () => ({
+    query: jest.fn(async (sql: string) => {
+      if (!String(sql).includes(SWAP_SQL)) return { rows: [] };
+      swaps += 1;
+      if (swaps === failOnSwap) throw new Error('database is down');
+      return { rows: [{ item_id: 1 }] };
+    }),
+    release: jest.fn()
+  }));
+}
 
 describe('where a cut-out is written', () => {
   // A new file rather than a rewrite of the original, which is what makes the
@@ -20,3 +57,42 @@ describe('where a cut-out is written', () => {
     expect(cutoutPathFor('/uploads/x.png')).toBe('/uploads/x-cutout.png');
   });
 });
+
+describe('when a restore fails partway through an item', () => {
+  // Logged rather than thrown, so the log line is expected here; silenced to
+  // keep it out of the suite's output rather than because it does not matter.
+  let logged: jest.SpyInstance;
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+    logged = jest.spyOn(console, 'error').mockImplementation(() => {});
+  });
+
+  afterEach(() => {
+    logged.mockRestore();
+  });
+
+  // The case the summary's `failed` field exists for: a genuine database
+  // failure partway through must not turn into a bare 500 that discards how
+  // far the restore got. It has to stop, report the count, and let the caller
+  // decide whether to retry — the same contract removeBackgroundsForItem has.
+  it('stops at the first genuine failure and reports how far it got', async () => {
+    poolWhoseSwapFailsOn([11, 22, 33], 2);
+
+    await expect(restoreOriginalsForItem(4)).resolves.toEqual({
+      total: 3,
+      restored: 1,
+      failed: true
+    });
+  });
+
+  // The third photo is never attempted, which is what makes pressing Restore
+  // again worth something: it resumes rather than starting over.
+  it('does not go on to the photos after the one that failed', async () => {
+    poolWhoseSwapFailsOn([11, 22, 33], 2);
+
+    await restoreOriginalsForItem(4);
+
+    expect(mockPool.connect).toHaveBeenCalledTimes(2);
+  });
+});