fix(admin): stop reporting every removeImageBackground failure as a sidecar failure (#281)

The remove-background route's catch block turned every throw from removeImageBackground into a 502 "the background-removal service did not answer". But removeImageBackground also throws for an unrecognised file extension (a legacy .jpeg), for a file missing from the uploads volume, and when REMBG_URL is not set at all — none of which involve contacting the sidecar. The admin was told to retry a service that was never reached, while the real reason existed only in the server log.

Added SidecarRequestError in rembgClient.ts, following the NoOriginalToRestoreError pattern already in backgroundRemoval.ts. It is thrown only for failures that happen after actually attempting to reach the sidecar: the fetch call itself throwing (now wrapped in a try/catch, covering unreachable and timed-out), a non-2xx response, or a response that is not a PNG. It is deliberately not thrown for "REMBG_URL is not set", since that path never attempts contact at all.

The remove-background handler now checks err instanceof SidecarRequestError before answering 502; everything else answers 500 with a message that says what actually went wrong.

Added a unit test pairing (rembgClient.test.ts) asserting the sidecar-contacted failures are SidecarRequestError and the unconfigured case is not, and an integration test (adminItemDrafts.integration.test.ts) proving a missing upload file answers something other than 502 with a message that does not claim the service did not answer.

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

backend/src/intake/rembgClient.ts

@@ -33,6 +33,18 @@ const MODEL = 'u2net';
  */
 const TIMEOUT_MS = 120_000;
 
+/**
+ * Thrown only when the sidecar was actually contacted and did not answer
+ * usably — unreachable, timed out, answered with a non-2xx status, or
+ * answered with something that is not a PNG.
+ *
+ * Deliberately not thrown for "REMBG_URL is not set": that failure happens
+ * before any attempt to contact anything, so lumping it in here would tell a
+ * caller "the service did not answer" about a service nothing ever tried to
+ * reach. A caller distinguishes the two to avoid exactly that (#281 review).
+ */
+export class SidecarRequestError extends Error {}
+
 /** The configured base URL, or null when there is none. */
 function baseUrl(): string | null {
   const raw = process.env.REMBG_URL;
@@ -72,14 +84,24 @@ export async function removeBackground(bytes: Buffer, mediaType: string): Promis
   body.append('file', new Blob([new Uint8Array(bytes)], { type: mediaType }), 'photo');
   body.append('model', MODEL);
 
-  const res = await fetch(`${base}/api/remove`, {
+  let res: Response;
+  try {
+    res = await fetch(`${base}/api/remove`, {
       method: 'POST',
       body,
       signal: AbortSignal.timeout(TIMEOUT_MS)
     });
+  } catch (err) {
+    // Unreachable, refused, or timed out — fetch throws for all three rather
+    // than returning a response, so this is the only place that can catch
+    // them and mark them as a sidecar failure rather than a generic error.
+    throw new SidecarRequestError(
+      `rembg did not answer: ${err instanceof Error ? err.message : String(err)}`
+    );
+  }
 
   if (!res.ok) {
-    throw new Error(`rembg answered ${res.status}`);
+    throw new SidecarRequestError(`rembg answered ${res.status}`);
   }
 
   const out = Buffer.from(await res.arrayBuffer());
@@ -88,7 +110,7 @@ export async function removeBackground(bytes: Buffer, mediaType: string): Promis
   // image/png would otherwise be written over a photograph — the same reason
   // uploads are checked by signature rather than by what the caller declared.
   if (!signatureMatches('image/png', out.subarray(0, SIGNATURE_BYTES))) {
-    throw new Error('rembg response is not a PNG');
+    throw new SidecarRequestError('rembg response is not a PNG');
   }
 
   return out;

backend/src/routes/adminItemDrafts.ts

@@ -8,7 +8,7 @@ import {
   removeImageBackground,
   restoreImageOriginal,
 } from '../intake/backgroundRemoval';
-import { isRembgConfigured } from '../intake/rembgClient';
+import { isRembgConfigured, SidecarRequestError } from '../intake/rembgClient';
 
 const router = Router();
 
@@ -274,15 +274,34 @@ router.post(
     try {
       await removeImageBackground(Number(imageId));
     } catch (err) {
-      // 502, not 500. The request was fine and so is this app — the service it
-      // depends on did not answer. The message says the photo is unchanged,
-      // because that is the thing the admin actually needs to know.
       console.error(`[drafts] background removal for image ${imageId}:`, err);
+
+      // 502 only for a SidecarRequestError: the request was fine and so is
+      // this app — the service it depends on was actually contacted and did
+      // not answer usably. The message says the photo is unchanged, because
+      // that is the thing the admin actually needs to know.
+      if (err instanceof SidecarRequestError) {
         return res
           .status(502)
           .json({ error: 'the background-removal service did not answer — the photo is unchanged' });
       }
 
+      // Everything else here never reached the sidecar at all — an
+      // unrecognised file extension (a legacy .jpeg), a file missing from the
+      // uploads volume, or REMBG_URL not being set. Reporting those as "the
+      // service did not answer" would send the admin to retry a service that
+      // was never contacted, and hide the real reason in the server log. The
+      // photo is still unchanged in every one of these cases too:
+      // removeImageBackground only writes the row once the cut-out already
+      // exists on disk.
+      return res.status(500).json({
+        error:
+          err instanceof Error
+            ? `this photo could not be processed: ${err.message}`
+            : 'this photo could not be processed'
+      });
+    }
+
     res.json(await imageOfItem(itemId, imageId));
   })
 );

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

@@ -469,6 +469,24 @@ describe('the review queue’s background-removal control', () => {
     expect(rows[0]?.image_path).toBe('/uploads/original.jpg');
   });
 
+  // MINOR 3 (#281 review): a failure that happens before the sidecar is ever
+  // contacted — here, the file the row points to is missing from the uploads
+  // volume — must not be reported as "the service did not answer". That sends
+  // the admin to retry a service that was never reached, and hides the real
+  // reason in the server log.
+  it('does not answer 502 when the photo cannot be read, even though a sidecar is configured', async () => {
+    await startStub(200, PNG_BYTES);
+    const { itemId, imageId } = await seedDraftWithImage();
+    await fsp.unlink(path.join(uploads, 'original.jpg'));
+
+    const res = await request(app).post(
+      `/api/admin/item-drafts/${itemId}/images/${imageId}/remove-background`
+    );
+
+    expect(res.status).not.toBe(502);
+    expect(res.body.error).not.toMatch(/did not answer/);
+  });
+
   // Scoped by item as well as by image. The id is a serial, so guessing one is
   // not hard, and a photo from another submission must not be reachable
   // through this item's URL.

backend/tests/unit/rembgClient.test.ts

@@ -1,6 +1,6 @@
 import http from 'http';
 import { AddressInfo } from 'net';
-import { isRembgConfigured, removeBackground } from '../../src/intake/rembgClient';
+import { isRembgConfigured, removeBackground, SidecarRequestError } from '../../src/intake/rembgClient';
 
 /** A real PNG header, so the client's own signature check sees what it expects. */
 const PNG = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]);
@@ -108,6 +108,9 @@ describe('asking the sidecar to remove a background', () => {
       },
       async () => {
         await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toThrow(/500/);
+        await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toBeInstanceOf(
+          SidecarRequestError
+        );
       }
     );
   });
@@ -123,12 +126,21 @@ describe('asking the sidecar to remove a background', () => {
       },
       async () => {
         await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toThrow(/not a PNG/);
+        await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toBeInstanceOf(
+          SidecarRequestError
+        );
       }
     );
   });
 
-  it('rejects when it is not configured at all', async () => {
+  // Deliberately not a SidecarRequestError (MINOR 3, #281 review): this
+  // failure happens before any attempt to contact the sidecar, and a caller
+  // has to be able to tell "never tried" apart from "tried and failed".
+  it('rejects when it is not configured at all, without it being a sidecar failure', async () => {
     delete process.env.REMBG_URL;
     await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toThrow(/REMBG_URL/);
+    await expect(removeBackground(JPEG, 'image/jpeg')).rejects.not.toBeInstanceOf(
+      SidecarRequestError
+    );
   });
 });