Files
redefined-designs/backend/src/intake/backgroundRemoval.ts
T
bermudalambandClaude Opus 5 445c9c4a22 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>
2026-09-04 12:02:23 -05:00

301 lines
12 KiB
TypeScript

import { promises as fs } from 'fs';
import path from 'path';
import { pool } from '../db';
import { typeForExtension } from '../uploadTypes';
import { isRembgConfigured, removeBackground } from './rembgClient';
/**
* Swapping a photo for a cut-out of itself, and swapping it back.
*
* One module rather than two, because the worker's path and the admin's path
* must produce identical results: a cut-out obtained either way has to be
* undoable the same way. A near-copy that drifted would mean a photo the
* Restore button could not restore.
*
* Nothing here deletes anything. The original file always stays on disk,
* because the submitter's photos are often the only copy of an item no longer
* in their hands — the same rule Discard follows in the review queue. A
* cut-out is not as durable: `cutoutPathFor` is deterministic, so a photo that
* is restored and then cut out again writes over the previous cut-out at the
* same path. That is harmless — no original is ever touched — but it means
* "every cut-out ever made" is not actually true, so this comment used to
* overstate it.
*/
interface ImageRow {
image_path: string;
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.
*
* Pure, so the naming rule can be checked without a database or a sidecar.
* Always `.png` because the result is transparent, and the storefront's dark
* theme would show a flat white background as a bright box behind every
* product.
*/
export function cutoutPathFor(imagePath: string): string {
const base = path.basename(imagePath, path.extname(imagePath));
return `/uploads/${base}-cutout.png`;
}
function uploadsDir(): string {
return process.env.UPLOADS_DIR ?? '';
}
/**
* Replaces one image with a cut-out, keeping the original.
*
* Idempotent by way of the `original_image_path IS NOT NULL` check rather than
* a separate flag. That guard is load-bearing twice over: it makes a repeat
* call a no-op, and it stops a second pass from recording the *cut-out* as the
* original and losing the real one for good.
*
* Throws on every failure. Nothing is written to the row unless the file is
* already on disk, so a caller that catches and moves on leaves the photo
* exactly as it was.
*/
export async function removeImageBackground(imageId: number): Promise<void> {
const { rows } = await pool.query<ImageRow>(
`SELECT image_path, original_image_path FROM item_images WHERE id = $1`,
[imageId]
);
const row = rows[0];
if (!row) {
throw new Error(`no image ${imageId}`);
}
if (row.original_image_path !== null) {
// Already cut out. Doing it again would overwrite the record of where the
// real original went.
return;
}
// basename only: image_path is stored as '/uploads/<name>' and the directory
// it lives in is a server constant. Same rule readPhotos follows in the
// drafting worker.
const sourceName = path.basename(row.image_path);
const mediaType = typeForExtension(path.extname(sourceName));
if (mediaType === null) {
throw new Error(`cannot read ${sourceName}: unrecognised extension`);
}
const cutout = await removeBackground(
await fs.readFile(path.join(uploadsDir(), sourceName)),
mediaType
);
const cutoutPath = cutoutPathFor(row.image_path);
await fs.writeFile(path.join(uploadsDir(), path.basename(cutoutPath)), cutout);
// The row is pointed at the new file only after the file exists. The other
// order would leave a window in which the storefront rendered a broken image.
//
// `original_image_path = image_path` reads the pre-update value, which is how
// Postgres evaluates an UPDATE's right-hand side — so this records where the
// photo came from in the same statement that moves it.
await pool.query(
`UPDATE item_images
SET image_path = $2, original_image_path = image_path
WHERE id = $1 AND original_image_path IS NULL`,
[imageId, cutoutPath]
);
}
/**
* 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<void> {
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();
}
}
/**
* What a whole-item removal actually did.
*
* `void` was enough for the drafting worker, which catches and logs and would
* not fail a draft over a background — but not for an admin standing in front
* of the screen, who needs to know whether the thing they pressed happened.
* Three of four is the normal shape of a bad day here, not an exception, and
* the count is what decides whether pressing it again is worth anything.
*/
export interface RemovalSummary {
/** How many images the item has. */
total: number;
/** How many now carry a cut-out, including any that already did. */
removed: number;
/** Whether it stopped early because one of them failed. */
failed: boolean;
}
/**
* What a whole-item restore did.
*
* 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. */
async function imageIdsFor(itemId: number): Promise<number[]> {
const { rows } = await pool.query<{ id: number }>(
`SELECT id FROM item_images WHERE item_id = $1 ORDER BY sort_order`,
[itemId]
);
return rows.map((row) => row.id);
}
/**
* Cuts out every photo of one item.
*
* Sequential rather than parallel: the sidecar is assumed to handle one request
* at a time, and neither caller is in a hurry.
*
* Stops at the first failure rather than pushing on. Six attempts against a
* sidecar that is not answering helps nobody, and stopping costs nothing
* because `removeImageBackground` skips a photo that already has an original
* recorded — so pressing the button again resumes where this stopped instead of
* starting over. The summary is what makes that retry an informed choice rather
* than a guess.
*
* An unconfigured environment is not a failure, here as everywhere else in this
* feature: nothing was attempted, so nothing went wrong.
*/
export async function removeBackgroundsForItem(itemId: number): Promise<RemovalSummary> {
const imageIds = await imageIdsFor(itemId);
if (!isRembgConfigured()) {
return { total: imageIds.length, removed: 0, failed: false };
}
let removed = 0;
for (const imageId of imageIds) {
try {
await removeImageBackground(imageId);
removed += 1;
} catch (err) {
// Logged rather than thrown. The caller gets the count, which is the
// thing it can act on; the reason belongs in the log, because the admin's
// next move is the same whatever it was.
console.error(`[background-removal] item ${itemId}, image ${imageId}:`, err);
return { total: imageIds.length, removed, failed: true };
}
}
return { total: imageIds.length, removed, failed: false };
}
/**
* Puts every cut-out photo of one item back.
*
* 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);
let restored = 0;
for (const imageId of imageIds) {
try {
await restoreImageOriginal(imageId);
restored += 1;
} catch (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, failed: false };
}