diff --git a/backend/src/intake/backgroundRemoval.ts b/backend/src/intake/backgroundRemoval.ts index eb2247b..13eb41d 100644 --- a/backend/src/intake/backgroundRemoval.ts +++ b/backend/src/intake/backgroundRemoval.ts @@ -110,22 +110,57 @@ export async function removeImageBackground(imageId: number): Promise { } /** - * Puts the original back. + * Puts the original back, and turns off the submitter's auto-removal intent + * for the item this photo belongs to. * * The cut-out file is left on disk deliberately. Removing a background is * exactly the operation that produces an occasional bad result on an unusual * object, so somebody restoring one is quite likely to try again — and this * module deletes nothing in any case. + * + * `item_drafts.remove_background` is written once at intake and otherwise + * never updated — without this, a restored photo looks identical to one that + * was simply never cut out, and Regenerate reads the same stale `true` and + * cuts it out again, quietly undoing the admin's decision. An admin restoring + * *any* photo on an item has overridden the submitter's request for that + * item: the flag is per-item while the swap is per-photo, so there is no + * per-photo place to record "leave this one alone" separately. Turning the + * whole item's auto-removal off is the conservative direction — the + * alternative is a worker that re-cuts a photo a person deliberately undid, + * which is the bug this fixes. + * + * Both writes happen in one transaction so a restore that succeeded while the + * flag update failed cannot reintroduce the bug it exists to close. */ export async function restoreImageOriginal(imageId: number): Promise { - const { rowCount } = await pool.query( - `UPDATE item_images - SET image_path = original_image_path, original_image_path = NULL - WHERE id = $1 AND original_image_path IS NOT NULL`, - [imageId] - ); - if (rowCount === 0) { - throw new NoOriginalToRestoreError(`image ${imageId} has no original to restore`); + const client = await pool.connect(); + try { + await client.query('BEGIN'); + const { rows } = await client.query<{ item_id: number }>( + `UPDATE item_images + SET image_path = original_image_path, original_image_path = NULL + WHERE id = $1 AND original_image_path IS NOT NULL + RETURNING item_id`, + [imageId] + ); + const restored = rows[0]; + if (!restored) { + await client.query('ROLLBACK'); + throw new NoOriginalToRestoreError(`image ${imageId} has no original to restore`); + } + + await client.query(`UPDATE item_drafts SET remove_background = false WHERE item_id = $1`, [ + restored.item_id + ]); + + await client.query('COMMIT'); + } catch (err) { + if (!(err instanceof NoOriginalToRestoreError)) { + await client.query('ROLLBACK'); + } + throw err; + } finally { + client.release(); } } diff --git a/backend/src/routes/adminItemDrafts.ts b/backend/src/routes/adminItemDrafts.ts index 8a662d9..7a48238 100644 --- a/backend/src/routes/adminItemDrafts.ts +++ b/backend/src/routes/adminItemDrafts.ts @@ -294,6 +294,10 @@ router.post( * occasional poor result on an unusual object, and this makes that survivable * rather than something to prevent. Nothing is deleted: the cut-out file stays * on disk, because somebody restoring one is quite likely to try again. + * + * restoreImageOriginal also turns off remove_background for this item, so a + * later Regenerate does not silently re-cut a photo the admin just put back — + * see the reasoning on that function. */ router.post( '/:itemId/images/:imageId/restore-original', diff --git a/backend/tests/integration/draftingBackgroundRemoval.integration.test.ts b/backend/tests/integration/draftingBackgroundRemoval.integration.test.ts index c4ae9d6..3180f6e 100644 --- a/backend/tests/integration/draftingBackgroundRemoval.integration.test.ts +++ b/backend/tests/integration/draftingBackgroundRemoval.integration.test.ts @@ -8,6 +8,7 @@ import { resetDb, closeDb } from './setup/testDb'; import { draftQueued } from '../../src/intake/draftingWorker'; import { resetAnthropicClient } from '../../src/intake/anthropicClient'; import { draftListing } from '../../src/intake/draftListing'; +import { restoreImageOriginal } from '../../src/intake/backgroundRemoval'; /** * The worker's background-removal step (#281). @@ -150,4 +151,39 @@ describe('background removal after a draft', () => { expect(rows[0]?.state).toBe('ready'); expect(await originalPathOf(itemId)).toBeNull(); }); + + // The bug this closes: remove_background is written once at intake and + // otherwise never updated, so a restored photo used to look identical to + // one that was simply never cut out — and Regenerate would read the same + // stale `true` and cut it out again, quietly undoing what the admin just + // did. restoreImageOriginal now clears the flag as part of the restore + // itself, so the worker respects the admin's decision on the next pass. + it('does not re-cut a photo the admin restored, even after Regenerate', async () => { + const itemId = await seedSubmissionWithPhoto({ removeBackground: true }); + await draftQueued(1); + expect(await originalPathOf(itemId)).toBe('/uploads/worker.jpg'); + + const { rows: imageRows } = await pool.query<{ id: number }>( + `SELECT id FROM item_images WHERE item_id = $1`, + [itemId] + ); + await restoreImageOriginal(imageRows[0]!.id); + + const { rows: flagRows } = await pool.query<{ remove_background: boolean }>( + `SELECT remove_background FROM item_drafts WHERE item_id = $1`, + [itemId] + ); + expect(flagRows[0]?.remove_background).toBe(false); + + // Mirrors what the admin's Regenerate button does: back to queued, with + // attempts cleared, so the worker picks the item up again. + await pool.query( + `UPDATE item_drafts SET state = 'queued', attempts = 0, ai_error = NULL WHERE item_id = $1`, + [itemId] + ); + + await draftQueued(1); + + expect(await originalPathOf(itemId)).toBeNull(); + }); });