diff --git a/backend/src/intake/rembgClient.ts b/backend/src/intake/rembgClient.ts index e93b79d..c3eb197 100644 --- a/backend/src/intake/rembgClient.ts +++ b/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`, { - method: 'POST', - body, - signal: AbortSignal.timeout(TIMEOUT_MS) - }); + 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; diff --git a/backend/src/routes/adminItemDrafts.ts b/backend/src/routes/adminItemDrafts.ts index 7a48238..5fd4c6d 100644 --- a/backend/src/routes/adminItemDrafts.ts +++ b/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,13 +274,32 @@ 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); - return res - .status(502) - .json({ error: 'the background-removal service did not answer — the photo is unchanged' }); + + // 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)); diff --git a/backend/tests/integration/adminItemDrafts.integration.test.ts b/backend/tests/integration/adminItemDrafts.integration.test.ts index 29e6f8b..4e4897a 100644 --- a/backend/tests/integration/adminItemDrafts.integration.test.ts +++ b/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. diff --git a/backend/tests/unit/rembgClient.test.ts b/backend/tests/unit/rembgClient.test.ts index 1779d86..7330931 100644 --- a/backend/tests/unit/rembgClient.test.ts +++ b/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 + ); }); });