diff --git a/backend/src/intake/backgroundRemoval.ts b/backend/src/intake/backgroundRemoval.ts index 23eb313..eb2247b 100644 --- a/backend/src/intake/backgroundRemoval.ts +++ b/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 { [imageId] ); if (rowCount === 0) { - throw new Error(`image ${imageId} has no original to restore`); + throw new NoOriginalToRestoreError(`image ${imageId} has no original to restore`); } } diff --git a/backend/src/routes/adminItemDrafts.ts b/backend/src/routes/adminItemDrafts.ts index 1b99a7f..8a662d9 100644 --- a/backend/src/routes/adminItemDrafts.ts +++ b/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' }); } diff --git a/backend/tests/integration/adminItemDrafts.integration.test.ts b/backend/tests/integration/adminItemDrafts.integration.test.ts index c808311..29e6f8b 100644 --- a/backend/tests/integration/adminItemDrafts.integration.test.ts +++ b/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.