Merge pull request 'Feature/281 background removal plan' (#284) from feature/281-background-removal-plan into main
Reviewed-on: #284
This commit was merged in pull request #284.
This commit is contained in:
@@ -0,0 +1,35 @@
|
||||
exports.up = (pgm) => {
|
||||
pgm.sql(`
|
||||
-- The submitter's intent, per submission, because that is how it is
|
||||
-- expressed: one checkbox above the send button, ticked by default.
|
||||
--
|
||||
-- The worker acts on it rather than the intake route. Removing inline
|
||||
-- would make the sender wait, would put a CPU-heavy model run in a path
|
||||
-- anyone holding a link can trigger — the surface #227 exists to bound —
|
||||
-- and would force a choice, when the sidecar is unreachable, between
|
||||
-- failing their submission and silently ignoring what they asked for.
|
||||
--
|
||||
-- NOT NULL DEFAULT true so a row written before this migration, or by any
|
||||
-- path that does not mention the column, behaves like the new default.
|
||||
ALTER TABLE item_drafts
|
||||
ADD COLUMN IF NOT EXISTS remove_background BOOLEAN NOT NULL DEFAULT true;
|
||||
|
||||
-- Where the photo came from, per image, because that is how it is undone.
|
||||
-- Null until a photo has been cut out, so it is also the answer to "can
|
||||
-- this be restored?" — one fact in one place rather than a flag that can
|
||||
-- disagree with a path.
|
||||
--
|
||||
-- Nullable and with no default: an existing image has no original other
|
||||
-- than itself, and claiming otherwise would offer a Restore that swapped a
|
||||
-- photo for a copy of itself.
|
||||
ALTER TABLE item_images
|
||||
ADD COLUMN IF NOT EXISTS original_image_path TEXT;
|
||||
`);
|
||||
};
|
||||
|
||||
exports.down = (pgm) => {
|
||||
pgm.sql(`
|
||||
ALTER TABLE item_drafts DROP COLUMN IF EXISTS remove_background;
|
||||
ALTER TABLE item_images DROP COLUMN IF EXISTS original_image_path;
|
||||
`);
|
||||
};
|
||||
@@ -29,6 +29,7 @@ export const itemImages = pgTable("item_images", {
|
||||
itemId: integer("item_id").notNull(),
|
||||
imagePath: text("image_path").notNull(),
|
||||
sortOrder: integer("sort_order").default(0).notNull(),
|
||||
originalImagePath: text("original_image_path"),
|
||||
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
|
||||
}, (table) => [
|
||||
foreignKey({
|
||||
@@ -233,6 +234,7 @@ export const itemDrafts = pgTable("item_drafts", {
|
||||
itemId: integer("item_id").notNull(),
|
||||
uploadLinkId: integer("upload_link_id"),
|
||||
submitterNote: text("submitter_note"),
|
||||
removeBackground: boolean("remove_background").default(true).notNull(),
|
||||
state: text().default('queued').notNull(),
|
||||
attempts: integer().default(0).notNull(),
|
||||
model: text(),
|
||||
|
||||
@@ -0,0 +1,189 @@
|
||||
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();
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Every photo of one item, in order.
|
||||
*
|
||||
* Sequential rather than parallel: the sidecar is assumed to handle one
|
||||
* request at a time, and the worker it runs inside is not in a hurry. A
|
||||
* failure on one photo stops the rest, and the caller logs it — the item keeps
|
||||
* whatever was already done, and nothing is left half-written.
|
||||
*/
|
||||
export async function removeBackgroundsForItem(itemId: number): Promise<void> {
|
||||
if (!isRembgConfigured()) return;
|
||||
|
||||
const { rows } = await pool.query<{ id: number }>(
|
||||
`SELECT id FROM item_images WHERE item_id = $1 ORDER BY sort_order`,
|
||||
[itemId]
|
||||
);
|
||||
for (const row of rows) {
|
||||
await removeImageBackground(row.id);
|
||||
}
|
||||
}
|
||||
@@ -7,6 +7,7 @@ import { getAnthropicClient } from './anthropicClient';
|
||||
import { draftListing } from './draftListing';
|
||||
import { applyDraft } from './applyDraft';
|
||||
import { notifyDraftReady } from './notifyDraft';
|
||||
import { removeBackgroundsForItem } from './backgroundRemoval';
|
||||
|
||||
/**
|
||||
* Turns queued submissions into drafts.
|
||||
@@ -19,6 +20,13 @@ import { notifyDraftReady } from './notifyDraft';
|
||||
* The photos are often the only copy of an item no longer in the sender's
|
||||
* hands, so every failure below leaves the row and its images intact and merely
|
||||
* undrafted. Nothing in this file deletes anything.
|
||||
*
|
||||
* Background removal (#281) follows drafting rather than running on its own
|
||||
* pass. That couples the two: an environment with no ANTHROPIC_API_KEY drafts
|
||||
* nothing, so it cuts out nothing either. That is the intended trade — a
|
||||
* separate pass would re-attempt an unreachable sidecar on every sweep for a
|
||||
* row that is going to sit at 'queued' indefinitely — and the admin's per-photo
|
||||
* control in the review queue is the way to do it by hand meanwhile.
|
||||
*/
|
||||
|
||||
/** Three tries, then it waits for a person rather than burning money on a loop. */
|
||||
@@ -32,6 +40,7 @@ type Photo = { mediaType: string; base64: string };
|
||||
interface QueuedRow {
|
||||
item_id: number;
|
||||
submitter_note: string | null;
|
||||
remove_background: boolean;
|
||||
}
|
||||
|
||||
export interface SweepResult {
|
||||
@@ -119,7 +128,7 @@ async function draftOne(
|
||||
|
||||
export async function draftQueued(limit = DEFAULT_BATCH): Promise<SweepResult> {
|
||||
const { rows } = await pool.query<QueuedRow>(
|
||||
`SELECT item_id, submitter_note FROM item_drafts
|
||||
`SELECT item_id, submitter_note, remove_background FROM item_drafts
|
||||
WHERE state = 'queued' AND attempts < $2
|
||||
ORDER BY created_at
|
||||
LIMIT $1`,
|
||||
@@ -146,6 +155,22 @@ export async function draftQueued(limit = DEFAULT_BATCH): Promise<SweepResult> {
|
||||
await draftOne(client, row.item_id, row.submitter_note, photos);
|
||||
drafted++;
|
||||
|
||||
// Deliberately after the draft is committed, and catching for itself.
|
||||
//
|
||||
// This is the sender's tick from the submission page, honoured here so
|
||||
// they never waited for it — and a failure must not mark a draft that was
|
||||
// written correctly as failed. The photo keeps its original in that case,
|
||||
// and the admin's per-photo control is still there to do it by hand.
|
||||
//
|
||||
// Awaited, unlike the notification below, so a sweep that has returned
|
||||
// has finished its work. Nothing is waiting on this: the worker is off
|
||||
// the request path, which is the whole reason drafting lives here.
|
||||
if (row.remove_background) {
|
||||
await removeBackgroundsForItem(row.item_id).catch((err) =>
|
||||
console.error(`[drafting] background removal for item ${row.item_id}:`, err)
|
||||
);
|
||||
}
|
||||
|
||||
// Fire and forget, and deliberately after the draft is committed. A mail
|
||||
// failure must never mark a draft that was written correctly as failed —
|
||||
// the queue is what the admin actually works from, and the email is a
|
||||
|
||||
@@ -0,0 +1,117 @@
|
||||
import { trimTrailingSlashes } from '../utils';
|
||||
import { SIGNATURE_BYTES, signatureMatches } from '../uploadTypes';
|
||||
|
||||
/**
|
||||
* The one place that talks to the background-removal sidecar.
|
||||
*
|
||||
* A sidecar rather than in-process inference: the application runs in a
|
||||
* container, and putting Python and ONNX into the image would add roughly
|
||||
* 300 MB to one already over a gigabyte. See
|
||||
* docs/ops/image-background-removal-stack.md for the measurements.
|
||||
*/
|
||||
|
||||
/**
|
||||
* NEVER remove this, and never make it configurable.
|
||||
*
|
||||
* The sidecar's default model is `bria-rmbg`, and BRIA's RMBG models are
|
||||
* licensed for NON-COMMERCIAL use. This is a shop. The default is reached by
|
||||
* simply not naming a model, so it is a licensing problem that happens
|
||||
* silently and produces a perfectly good image — there is nothing in the
|
||||
* output that could reveal it.
|
||||
*
|
||||
* `u2net` is Apache-2.0, and also ten times faster (1.1–2.3 s against
|
||||
* 14–20 s) at a sixth the size, so nothing is being traded away for it.
|
||||
*/
|
||||
const MODEL = 'u2net';
|
||||
|
||||
/**
|
||||
* Generous on purpose. The sidecar takes about 40 seconds to answer after a
|
||||
* container start and its first call per model downloads 168 MB, so a tight
|
||||
* timeout would turn an ordinary cold start into a failure. Nobody is waiting
|
||||
* on this in the worker's path, and an admin who clicked a button would rather
|
||||
* wait than be told it did not work.
|
||||
*/
|
||||
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;
|
||||
if (raw === undefined || raw.trim() === '') return null;
|
||||
return trimTrailingSlashes(raw.trim());
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether the feature exists in this environment.
|
||||
*
|
||||
* Unconfigured is not a failure. It means the submitter sees no checkbox, the
|
||||
* admin sees no control and the worker skips the step — an unconfigured
|
||||
* environment must be a working one, which is the same rule
|
||||
* `getAnthropicClient` follows by returning null rather than throwing.
|
||||
*/
|
||||
export function isRembgConfigured(): boolean {
|
||||
return baseUrl() !== null;
|
||||
}
|
||||
|
||||
/**
|
||||
* The cut-out, as PNG bytes.
|
||||
*
|
||||
* Rejects on every failure — unconfigured, unreachable, a non-2xx answer, or a
|
||||
* body that is not actually a PNG. Every caller catches, and none of them lets
|
||||
* the rejection reach a submission or a draft.
|
||||
*/
|
||||
export async function removeBackground(bytes: Buffer, mediaType: string): Promise<Buffer> {
|
||||
const base = baseUrl();
|
||||
if (base === null) {
|
||||
throw new Error('REMBG_URL is not set');
|
||||
}
|
||||
|
||||
const body = new FormData();
|
||||
// A copy through Uint8Array because Buffer is not a BlobPart. The filename is
|
||||
// a constant: the sidecar does not use it, and passing the stored name would
|
||||
// put a value from the uploads volume into an outbound request for nothing.
|
||||
body.append('file', new Blob([new Uint8Array(bytes)], { type: mediaType }), 'photo');
|
||||
body.append('model', MODEL);
|
||||
|
||||
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 SidecarRequestError(`rembg answered ${res.status}`);
|
||||
}
|
||||
|
||||
const out = Buffer.from(await res.arrayBuffer());
|
||||
|
||||
// The bytes, not the Content-Type header. A proxy error page served as
|
||||
// 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 SidecarRequestError('rembg response is not a PNG');
|
||||
}
|
||||
|
||||
return out;
|
||||
}
|
||||
@@ -3,6 +3,12 @@ import { pool } from '../db';
|
||||
import { asyncRoute } from '../asyncRoute';
|
||||
import { draftQueued } from '../intake/draftingWorker';
|
||||
import { nextPriceSource, PriceSource } from '../intake/priceSource';
|
||||
import {
|
||||
NoOriginalToRestoreError,
|
||||
removeImageBackground,
|
||||
restoreImageOriginal,
|
||||
} from '../intake/backgroundRemoval';
|
||||
import { isRembgConfigured, SidecarRequestError } from '../intake/rembgClient';
|
||||
|
||||
const router = Router();
|
||||
|
||||
@@ -27,7 +33,10 @@ const DRAFT_SELECT = `
|
||||
i.price_cents, i.status,
|
||||
l.label AS upload_link_label,
|
||||
COALESCE((
|
||||
SELECT json_agg(json_build_object('id', img.id, 'image_path', img.image_path)
|
||||
SELECT json_agg(json_build_object(
|
||||
'id', img.id,
|
||||
'image_path', img.image_path,
|
||||
'original_image_path', img.original_image_path)
|
||||
ORDER BY img.sort_order)
|
||||
FROM item_images img WHERE img.item_id = d.item_id
|
||||
), '[]'::json) AS images
|
||||
@@ -52,7 +61,9 @@ router.get(
|
||||
? await pool.query(`${DRAFT_SELECT} WHERE d.state = $1 ORDER BY d.created_at DESC`, [state])
|
||||
: await pool.query(`${DRAFT_SELECT} WHERE d.state <> 'discarded' ORDER BY d.created_at DESC`);
|
||||
|
||||
res.json({ drafts: rows });
|
||||
// Whether the control has anything behind it, alongside the rows. A second
|
||||
// endpoint for one boolean would be a round trip the queue already makes.
|
||||
res.json({ drafts: rows, backgroundRemoval: isRembgConfigured() });
|
||||
})
|
||||
);
|
||||
|
||||
@@ -220,4 +231,123 @@ router.post(
|
||||
})
|
||||
);
|
||||
|
||||
/**
|
||||
* One photo's current paths, if it belongs to this item.
|
||||
*
|
||||
* Scoped by item as well as by image so an image id from a different
|
||||
* submission cannot be acted on through this item's URL — the id is a serial,
|
||||
* so guessing one is not hard.
|
||||
*/
|
||||
async function imageOfItem(
|
||||
itemId: string,
|
||||
imageId: string
|
||||
): Promise<{ image_path: string; original_image_path: string | null } | null> {
|
||||
const { rows } = await pool.query<{ image_path: string; original_image_path: string | null }>(
|
||||
`SELECT image_path, original_image_path
|
||||
FROM item_images WHERE id = $1 AND item_id = $2`,
|
||||
[imageId, itemId]
|
||||
);
|
||||
return rows[0] ?? null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Remove the background from one photo.
|
||||
*
|
||||
* The other half of the submitter's checkbox: for the photos nobody ticked it
|
||||
* for, and for the ones where the worker could not reach the sidecar. Both go
|
||||
* through the same module, so a cut-out obtained either way is identical and
|
||||
* either can be undone by Restore.
|
||||
*
|
||||
* Synchronous, unlike the worker's path. A warm request measures 1.1–2.3 s and
|
||||
* this is an admin who just clicked a button and is watching for the result.
|
||||
* The reason drafting was moved off the request path — that a stranger can
|
||||
* trigger it and must never wait — does not apply behind the admin gate.
|
||||
*/
|
||||
router.post(
|
||||
'/:itemId/images/:imageId/remove-background',
|
||||
asyncRoute(async (req: Request, res: Response) => {
|
||||
const { itemId = '', imageId = '' } = req.params;
|
||||
if ((await imageOfItem(itemId, imageId)) === null) {
|
||||
return res.status(404).json({ error: 'no such photo on this item' });
|
||||
}
|
||||
|
||||
try {
|
||||
await removeImageBackground(Number(imageId));
|
||||
} catch (err) {
|
||||
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));
|
||||
})
|
||||
);
|
||||
|
||||
/**
|
||||
* Put the original photo back.
|
||||
*
|
||||
* The reason a cut-out is safe to try at all. Background removal produces the
|
||||
* 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',
|
||||
asyncRoute(async (req: Request, res: Response) => {
|
||||
const { itemId = '', imageId = '' } = req.params;
|
||||
const existing = await imageOfItem(itemId, imageId);
|
||||
if (existing === null || existing.original_image_path === null) {
|
||||
return res.status(404).json({ error: 'this photo has no original to restore' });
|
||||
}
|
||||
|
||||
try {
|
||||
await restoreImageOriginal(Number(imageId));
|
||||
} catch (err) {
|
||||
// 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' });
|
||||
}
|
||||
|
||||
res.json(await imageOfItem(itemId, imageId));
|
||||
})
|
||||
);
|
||||
|
||||
export default router;
|
||||
|
||||
@@ -8,6 +8,7 @@ import { draftQueued } from '../intake/draftingWorker';
|
||||
import { checkCapacity, countForLinkSince, windowStart } from '../intake/capacity';
|
||||
import { alertCeilingReached, alertLinkThreshold } from '../intake/abuseAlert';
|
||||
import { getSettings } from '../adminSettings';
|
||||
import { isRembgConfigured } from '../intake/rembgClient';
|
||||
|
||||
const router = Router();
|
||||
|
||||
@@ -128,8 +129,9 @@ router.get('/:token', intakeViewLimiter, asyncRoute(async (req: Request, res: Re
|
||||
if (!link) {
|
||||
return res.status(404).json({ error: 'not found' });
|
||||
}
|
||||
// The label only. Nothing about the catalogue, the admin, or other links.
|
||||
res.json({ label: link.label });
|
||||
// The label, and whether the background-removal control has anything behind
|
||||
// it. Still nothing about the catalogue, the admin, or other links.
|
||||
res.json({ label: link.label, backgroundRemoval: isRembgConfigured() });
|
||||
}));
|
||||
|
||||
router.post(
|
||||
@@ -159,6 +161,15 @@ router.post(
|
||||
|
||||
const note = typeof req.body?.note === 'string' ? req.body.note.trim() : '';
|
||||
|
||||
// Absent means yes: the checkbox on the page is ticked by default, so a
|
||||
// client that does not send the field — an older build, or a script — gets
|
||||
// what every other submission gets rather than silently opting out.
|
||||
//
|
||||
// Only the exact string opts out. Multipart fields arrive as strings, and
|
||||
// reading a stray value as "no" would quietly deny somebody something they
|
||||
// asked for.
|
||||
const removeBackground = req.body?.removeBackground !== 'false';
|
||||
|
||||
const client = await pool.connect();
|
||||
try {
|
||||
await client.query('BEGIN');
|
||||
@@ -178,9 +189,9 @@ router.post(
|
||||
await insertItemImages(client, itemId, files, 0);
|
||||
|
||||
await client.query(
|
||||
`INSERT INTO item_drafts (item_id, upload_link_id, submitter_note)
|
||||
VALUES ($1, $2, $3)`,
|
||||
[itemId, link.id, note === '' ? null : note]
|
||||
`INSERT INTO item_drafts (item_id, upload_link_id, submitter_note, remove_background)
|
||||
VALUES ($1, $2, $3, $4)`,
|
||||
[itemId, link.id, note === '' ? null : note, removeBackground]
|
||||
);
|
||||
|
||||
// Counted inside the transaction and guarded on the same conditions as
|
||||
|
||||
@@ -2,6 +2,12 @@ import request from 'supertest';
|
||||
import app from '../../src/app';
|
||||
import { pool } from '../../src/db';
|
||||
import { resetDb, closeDb } from './setup/testDb';
|
||||
import http from 'http';
|
||||
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();
|
||||
@@ -304,3 +310,214 @@ describe('the other three actions', () => {
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('the review queue’s background-removal control', () => {
|
||||
const PNG_BYTES = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]);
|
||||
const JPEG_BYTES = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0]);
|
||||
|
||||
let uploads = '';
|
||||
let stub: http.Server | null = null;
|
||||
let previousUploadsDir: string | undefined;
|
||||
|
||||
/** A stub sidecar on an ephemeral port, and a temporary uploads directory. */
|
||||
async function startStub(status: number, body: Buffer | string): Promise<void> {
|
||||
uploads = await fsp.mkdtemp(path.join(os.tmpdir(), 'adminbg-'));
|
||||
previousUploadsDir = process.env.UPLOADS_DIR;
|
||||
process.env.UPLOADS_DIR = uploads;
|
||||
|
||||
stub = http.createServer((req, res) => {
|
||||
req.on('data', () => undefined);
|
||||
req.on('end', () => {
|
||||
res.writeHead(status, { 'Content-Type': 'image/png' });
|
||||
res.end(body);
|
||||
});
|
||||
});
|
||||
await new Promise<void>((resolve) => stub!.listen(0, '127.0.0.1', resolve));
|
||||
process.env.REMBG_URL = `http://127.0.0.1:${(stub!.address() as AddressInfo).port}`;
|
||||
}
|
||||
|
||||
afterEach(async () => {
|
||||
delete process.env.REMBG_URL;
|
||||
if (stub) {
|
||||
await new Promise<void>((resolve) => stub!.close(() => resolve()));
|
||||
stub = null;
|
||||
}
|
||||
if (uploads !== '') {
|
||||
await fsp.rm(uploads, { recursive: true, force: true });
|
||||
uploads = '';
|
||||
}
|
||||
if (previousUploadsDir === undefined) delete process.env.UPLOADS_DIR;
|
||||
else process.env.UPLOADS_DIR = previousUploadsDir;
|
||||
previousUploadsDir = undefined;
|
||||
});
|
||||
|
||||
/** A ready draft with one photo, on disk, named to match the assertions. */
|
||||
async function seedDraftWithImage(): Promise<{ itemId: number; imageId: number }> {
|
||||
const { rows } = await pool.query<{ id: number }>(
|
||||
`INSERT INTO items (name, status) VALUES ('Submission placeholder', 'pending') RETURNING id`
|
||||
);
|
||||
const itemId = rows[0]!.id;
|
||||
await pool.query(`INSERT INTO item_drafts (item_id, state) VALUES ($1, 'ready')`, [itemId]);
|
||||
const image = await pool.query<{ id: number }>(
|
||||
`INSERT INTO item_images (item_id, image_path, sort_order)
|
||||
VALUES ($1, '/uploads/original.jpg', 0) RETURNING id`,
|
||||
[itemId]
|
||||
);
|
||||
if (uploads !== '') await fsp.writeFile(path.join(uploads, 'original.jpg'), JPEG_BYTES);
|
||||
return { itemId, imageId: image.rows[0]!.id };
|
||||
}
|
||||
|
||||
it('says whether there is a sidecar behind the control at all', async () => {
|
||||
process.env.REMBG_URL = 'http://rembg-syn:7000';
|
||||
const on = await request(app).get('/api/admin/item-drafts');
|
||||
expect(on.body.backgroundRemoval).toBe(true);
|
||||
|
||||
delete process.env.REMBG_URL;
|
||||
const off = await request(app).get('/api/admin/item-drafts');
|
||||
expect(off.body.backgroundRemoval).toBe(false);
|
||||
});
|
||||
|
||||
// The UI decides between "Remove background" and "Restore original" from
|
||||
// this field alone, so it has to be in the payload the queue is built from.
|
||||
it('includes original_image_path on every image', async () => {
|
||||
await seedDraftWithImage();
|
||||
|
||||
const res = await request(app).get('/api/admin/item-drafts');
|
||||
|
||||
expect(res.body.drafts[0].images[0]).toHaveProperty('original_image_path', null);
|
||||
});
|
||||
|
||||
it('cuts out one photo and answers with its new paths', async () => {
|
||||
await startStub(200, PNG_BYTES);
|
||||
const { itemId, imageId } = await seedDraftWithImage();
|
||||
|
||||
const res = await request(app).post(
|
||||
`/api/admin/item-drafts/${itemId}/images/${imageId}/remove-background`
|
||||
);
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.image_path).toBe('/uploads/original-cutout.png');
|
||||
expect(res.body.original_image_path).toBe('/uploads/original.jpg');
|
||||
});
|
||||
|
||||
it('puts the original back', 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 res = await request(app).post(
|
||||
`/api/admin/item-drafts/${itemId}/images/${imageId}/restore-original`
|
||||
);
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.image_path).toBe('/uploads/original.jpg');
|
||||
expect(res.body.original_image_path).toBeNull();
|
||||
});
|
||||
|
||||
// The precheck and restoreImageOriginal's own guard can disagree under a
|
||||
// race. Whichever call loses, the answer must say "already done" rather than
|
||||
// "something is wrong with this application".
|
||||
it('does not answer 500 when two restores 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 results = await Promise.all([
|
||||
request(app).post(`/api/admin/item-drafts/${itemId}/images/${imageId}/restore-original`),
|
||||
request(app).post(`/api/admin/item-drafts/${itemId}/images/${imageId}/restore-original`)
|
||||
]);
|
||||
|
||||
const statuses = results.map((r) => r.status).sort((a, b) => a - b);
|
||||
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.
|
||||
it('answers 502 when the sidecar will not, and leaves the photo alone', async () => {
|
||||
await startStub(500, 'boom');
|
||||
const { itemId, imageId } = await seedDraftWithImage();
|
||||
|
||||
const res = await request(app).post(
|
||||
`/api/admin/item-drafts/${itemId}/images/${imageId}/remove-background`
|
||||
);
|
||||
|
||||
expect(res.status).toBe(502);
|
||||
const { rows } = await pool.query<{ image_path: string }>(
|
||||
`SELECT image_path FROM item_images WHERE id = $1`,
|
||||
[imageId]
|
||||
);
|
||||
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.
|
||||
it('refuses an image that does not belong to the item', async () => {
|
||||
await startStub(200, PNG_BYTES);
|
||||
const first = await seedDraftWithImage();
|
||||
const second = await seedDraftWithImage();
|
||||
|
||||
const res = await request(app).post(
|
||||
`/api/admin/item-drafts/${first.itemId}/images/${second.imageId}/remove-background`
|
||||
);
|
||||
|
||||
expect(res.status).toBe(404);
|
||||
});
|
||||
|
||||
it('refuses to restore a photo that was never cut out', async () => {
|
||||
const { itemId, imageId } = await seedDraftWithImage();
|
||||
|
||||
const res = await request(app).post(
|
||||
`/api/admin/item-drafts/${itemId}/images/${imageId}/restore-original`
|
||||
);
|
||||
|
||||
expect(res.status).toBe(404);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -0,0 +1,252 @@
|
||||
import http from 'http';
|
||||
import { AddressInfo } from 'net';
|
||||
import { promises as fsp } from 'fs';
|
||||
import os from 'os';
|
||||
import path from 'path';
|
||||
import { pool } from '../../src/db';
|
||||
import { resetDb, closeDb } from './setup/testDb';
|
||||
import { removeImageBackground, restoreImageOriginal } from '../../src/intake/backgroundRemoval';
|
||||
|
||||
beforeEach(async () => {
|
||||
await resetDb();
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await pool.end();
|
||||
await closeDb();
|
||||
});
|
||||
|
||||
/** An item with a draft row and one image, which is what a submission leaves. */
|
||||
async function seedSubmission(
|
||||
imagePath = '/uploads/photo.jpg'
|
||||
): Promise<{ itemId: number; imageId: number }> {
|
||||
const item = await pool.query<{ id: number }>(
|
||||
`INSERT INTO items (name, status) VALUES ('Submission placeholder', 'pending') RETURNING id`
|
||||
);
|
||||
const itemId = item.rows[0]!.id;
|
||||
await pool.query(`INSERT INTO item_drafts (item_id) VALUES ($1)`, [itemId]);
|
||||
const image = await pool.query<{ id: number }>(
|
||||
`INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, 0) RETURNING id`,
|
||||
[itemId, imagePath]
|
||||
);
|
||||
return { itemId, imageId: image.rows[0]!.id };
|
||||
}
|
||||
|
||||
describe('the background-removal columns', () => {
|
||||
// Default true because the submitter's checkbox is ticked by default, and
|
||||
// because a row written by any path that does not mention the column should
|
||||
// behave like the new default rather than needing a backfill.
|
||||
it('defaults remove_background to true', async () => {
|
||||
const { itemId } = await seedSubmission();
|
||||
|
||||
const { rows } = await pool.query<{ remove_background: boolean }>(
|
||||
`SELECT remove_background FROM item_drafts WHERE item_id = $1`,
|
||||
[itemId]
|
||||
);
|
||||
expect(rows[0]?.remove_background).toBe(true);
|
||||
});
|
||||
|
||||
// Null is also the answer to "can this be restored?", which is why there is
|
||||
// no separate flag: one fact, one place.
|
||||
it('leaves original_image_path null until a photo has been cut out', async () => {
|
||||
const { imageId } = await seedSubmission();
|
||||
|
||||
const { rows } = await pool.query<{ original_image_path: string | null }>(
|
||||
`SELECT original_image_path FROM item_images WHERE id = $1`,
|
||||
[imageId]
|
||||
);
|
||||
expect(rows[0]?.original_image_path).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
const PNG_BYTES = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]);
|
||||
const JPEG_BYTES = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0]);
|
||||
|
||||
interface ImagePaths {
|
||||
image_path: string;
|
||||
original_image_path: string | null;
|
||||
}
|
||||
|
||||
let uploads = '';
|
||||
let stub: http.Server | null = null;
|
||||
let previousUploadsDir: string | undefined;
|
||||
|
||||
/**
|
||||
* A real uploads directory and a stub sidecar.
|
||||
*
|
||||
* A temporary directory rather than the configured one, because these tests
|
||||
* write files and a suite that leaves rubbish in a developer's uploads volume
|
||||
* is a suite people stop running. `afterEach` removes it and puts back
|
||||
* whatever UPLOADS_DIR held before, so this suite does not leak either the
|
||||
* directory or the environment variable into whatever runs after it.
|
||||
*
|
||||
* No test here contacts the real sidecar. It takes forty seconds to start, and
|
||||
* a suite that depends on that is broken by construction.
|
||||
*/
|
||||
async function startStub(
|
||||
handler: (req: http.IncomingMessage, res: http.ServerResponse) => void
|
||||
): Promise<void> {
|
||||
uploads = await fsp.mkdtemp(path.join(os.tmpdir(), 'bgremoval-'));
|
||||
previousUploadsDir = process.env.UPLOADS_DIR;
|
||||
process.env.UPLOADS_DIR = uploads;
|
||||
|
||||
stub = http.createServer((req, res) => {
|
||||
// Drain the body before answering, or the client sees a reset rather than
|
||||
// the status this test is about.
|
||||
req.on('data', () => undefined);
|
||||
req.on('end', () => handler(req, res));
|
||||
});
|
||||
await new Promise<void>((resolve) => stub!.listen(0, '127.0.0.1', resolve));
|
||||
process.env.REMBG_URL = `http://127.0.0.1:${(stub!.address() as AddressInfo).port}`;
|
||||
}
|
||||
|
||||
function answerWithPng(_req: http.IncomingMessage, res: http.ServerResponse): void {
|
||||
res.writeHead(200, { 'Content-Type': 'image/png' });
|
||||
res.end(PNG_BYTES);
|
||||
}
|
||||
|
||||
afterEach(async () => {
|
||||
delete process.env.REMBG_URL;
|
||||
if (stub) {
|
||||
await new Promise<void>((resolve) => stub!.close(() => resolve()));
|
||||
stub = null;
|
||||
}
|
||||
if (uploads !== '') {
|
||||
await fsp.rm(uploads, { recursive: true, force: true });
|
||||
uploads = '';
|
||||
}
|
||||
if (previousUploadsDir === undefined) delete process.env.UPLOADS_DIR;
|
||||
else process.env.UPLOADS_DIR = previousUploadsDir;
|
||||
previousUploadsDir = undefined;
|
||||
});
|
||||
|
||||
async function seedWithFile(): Promise<{ itemId: number; imageId: number; name: string }> {
|
||||
const name = 'original.jpg';
|
||||
const seeded = await seedSubmission(`/uploads/${name}`);
|
||||
await fsp.writeFile(path.join(uploads, name), JPEG_BYTES);
|
||||
return { ...seeded, name };
|
||||
}
|
||||
|
||||
async function pathsOf(imageId: number): Promise<ImagePaths> {
|
||||
const { rows } = await pool.query<ImagePaths>(
|
||||
`SELECT image_path, original_image_path FROM item_images WHERE id = $1`,
|
||||
[imageId]
|
||||
);
|
||||
return rows[0]!;
|
||||
}
|
||||
|
||||
describe('removing one photo’s background', () => {
|
||||
it('points the row at the cut-out and records where the original went', async () => {
|
||||
await startStub(answerWithPng);
|
||||
const { imageId } = await seedWithFile();
|
||||
|
||||
await removeImageBackground(imageId);
|
||||
|
||||
const paths = await pathsOf(imageId);
|
||||
expect(paths.image_path).toBe('/uploads/original-cutout.png');
|
||||
expect(paths.original_image_path).toBe('/uploads/original.jpg');
|
||||
});
|
||||
|
||||
// The original is the only copy of an item that may no longer be in the
|
||||
// sender's hands. Nothing in this module is allowed to remove it.
|
||||
it('leaves the original file on disk', async () => {
|
||||
await startStub(answerWithPng);
|
||||
const { imageId, name } = await seedWithFile();
|
||||
|
||||
await removeImageBackground(imageId);
|
||||
|
||||
await expect(fsp.access(path.join(uploads, name))).resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
it('writes the cut-out where the row now says it is', async () => {
|
||||
await startStub(answerWithPng);
|
||||
const { imageId } = await seedWithFile();
|
||||
|
||||
await removeImageBackground(imageId);
|
||||
|
||||
await expect(fsp.readFile(path.join(uploads, 'original-cutout.png'))).resolves.toEqual(
|
||||
PNG_BYTES
|
||||
);
|
||||
});
|
||||
|
||||
// The guard that stops a second pass recording the cut-out as the original
|
||||
// and losing the real one for good.
|
||||
it('is a no-op on a photo that has already been cut out', async () => {
|
||||
await startStub(answerWithPng);
|
||||
const { imageId } = await seedWithFile();
|
||||
|
||||
await removeImageBackground(imageId);
|
||||
await removeImageBackground(imageId);
|
||||
|
||||
expect((await pathsOf(imageId)).original_image_path).toBe('/uploads/original.jpg');
|
||||
});
|
||||
});
|
||||
|
||||
describe('when the sidecar will not answer', () => {
|
||||
it('leaves the row untouched on a 500', async () => {
|
||||
await startStub((_req, res) => {
|
||||
res.writeHead(500);
|
||||
res.end('boom');
|
||||
});
|
||||
const { imageId } = await seedWithFile();
|
||||
|
||||
await expect(removeImageBackground(imageId)).rejects.toThrow(/500/);
|
||||
|
||||
const paths = await pathsOf(imageId);
|
||||
expect(paths.image_path).toBe('/uploads/original.jpg');
|
||||
expect(paths.original_image_path).toBeNull();
|
||||
});
|
||||
|
||||
it('leaves the row untouched when it returns something that is not an image', async () => {
|
||||
await startStub((_req, res) => {
|
||||
res.writeHead(200, { 'Content-Type': 'image/png' });
|
||||
res.end('<html>gateway error</html>');
|
||||
});
|
||||
const { imageId } = await seedWithFile();
|
||||
|
||||
await expect(removeImageBackground(imageId)).rejects.toThrow(/not a PNG/);
|
||||
|
||||
const paths = await pathsOf(imageId);
|
||||
expect(paths.image_path).toBe('/uploads/original.jpg');
|
||||
expect(paths.original_image_path).toBeNull();
|
||||
});
|
||||
|
||||
it('leaves the row untouched when it is unreachable', async () => {
|
||||
await startStub(answerWithPng);
|
||||
// Closed before the call, so the connection is refused rather than hung.
|
||||
// The URL stays set, which is the case worth modelling: configured, and
|
||||
// not there.
|
||||
await new Promise<void>((resolve) => stub!.close(() => resolve()));
|
||||
stub = null;
|
||||
const { imageId } = await seedWithFile();
|
||||
|
||||
await expect(removeImageBackground(imageId)).rejects.toThrow();
|
||||
|
||||
const paths = await pathsOf(imageId);
|
||||
expect(paths.image_path).toBe('/uploads/original.jpg');
|
||||
expect(paths.original_image_path).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('putting the original back', () => {
|
||||
it('swaps the paths back and clears the record', async () => {
|
||||
await startStub(answerWithPng);
|
||||
const { imageId } = await seedWithFile();
|
||||
await removeImageBackground(imageId);
|
||||
|
||||
await restoreImageOriginal(imageId);
|
||||
|
||||
const paths = await pathsOf(imageId);
|
||||
expect(paths.image_path).toBe('/uploads/original.jpg');
|
||||
expect(paths.original_image_path).toBeNull();
|
||||
});
|
||||
|
||||
it('refuses a photo that was never cut out, rather than blanking its path', async () => {
|
||||
await startStub(answerWithPng);
|
||||
const { imageId } = await seedWithFile();
|
||||
|
||||
await expect(restoreImageOriginal(imageId)).rejects.toThrow(/no original/);
|
||||
|
||||
expect((await pathsOf(imageId)).image_path).toBe('/uploads/original.jpg');
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,198 @@
|
||||
import http from 'http';
|
||||
import { AddressInfo } from 'net';
|
||||
import { promises as fsp } from 'fs';
|
||||
import os from 'os';
|
||||
import path from 'path';
|
||||
import { pool } from '../../src/db';
|
||||
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).
|
||||
*
|
||||
* The model is mocked rather than reached. What is under test is what the
|
||||
* worker does *after* a draft is written — which of the two paths it takes,
|
||||
* and what survives when the sidecar does not answer — and none of that
|
||||
* depends on what the model said.
|
||||
*/
|
||||
jest.mock('../../src/intake/draftListing');
|
||||
|
||||
const draftListingMock = draftListing as jest.MockedFunction<typeof draftListing>;
|
||||
|
||||
const JPEG_BYTES = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0]);
|
||||
const PNG_BYTES = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]);
|
||||
|
||||
let uploads = '';
|
||||
let stub: http.Server | null = null;
|
||||
let previousUploadsDir: string | undefined;
|
||||
|
||||
beforeEach(async () => {
|
||||
await resetDb();
|
||||
|
||||
draftListingMock.mockResolvedValue({
|
||||
draft: {
|
||||
name: 'Blue stoneware vase',
|
||||
description: 'Hand-thrown, chipped base.',
|
||||
category: null,
|
||||
tags: [],
|
||||
suggestedPriceCents: 4500
|
||||
},
|
||||
model: 'claude-sonnet-5',
|
||||
inputTokens: 1000,
|
||||
outputTokens: 200,
|
||||
costMicros: 4000
|
||||
});
|
||||
|
||||
// Non-empty is all that is needed: getAnthropicClient only has to return
|
||||
// something other than null, and the mock above is what answers.
|
||||
process.env.ANTHROPIC_API_KEY = 'sk-ant-not-used';
|
||||
resetAnthropicClient();
|
||||
|
||||
uploads = await fsp.mkdtemp(path.join(os.tmpdir(), 'workerbg-'));
|
||||
previousUploadsDir = process.env.UPLOADS_DIR;
|
||||
process.env.UPLOADS_DIR = uploads;
|
||||
|
||||
stub = http.createServer((req, res) => {
|
||||
req.on('data', () => undefined);
|
||||
req.on('end', () => {
|
||||
res.writeHead(200, { 'Content-Type': 'image/png' });
|
||||
res.end(PNG_BYTES);
|
||||
});
|
||||
});
|
||||
await new Promise<void>((resolve) => stub!.listen(0, '127.0.0.1', resolve));
|
||||
process.env.REMBG_URL = `http://127.0.0.1:${(stub!.address() as AddressInfo).port}`;
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
delete process.env.REMBG_URL;
|
||||
delete process.env.ANTHROPIC_API_KEY;
|
||||
resetAnthropicClient();
|
||||
if (stub) {
|
||||
await new Promise<void>((resolve) => stub!.close(() => resolve()));
|
||||
stub = null;
|
||||
}
|
||||
if (uploads !== '') {
|
||||
await fsp.rm(uploads, { recursive: true, force: true });
|
||||
uploads = '';
|
||||
}
|
||||
if (previousUploadsDir === undefined) delete process.env.UPLOADS_DIR;
|
||||
else process.env.UPLOADS_DIR = previousUploadsDir;
|
||||
previousUploadsDir = undefined;
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await pool.end();
|
||||
await closeDb();
|
||||
});
|
||||
|
||||
/** A queued submission with one real file on disk and the intent set. */
|
||||
async function seedSubmissionWithPhoto(options: { removeBackground: boolean }): Promise<number> {
|
||||
const { rows } = await pool.query<{ id: number }>(
|
||||
`INSERT INTO items (name, status) VALUES ('Submission placeholder', 'pending') RETURNING id`
|
||||
);
|
||||
const itemId = rows[0]!.id;
|
||||
await pool.query(
|
||||
`INSERT INTO item_drafts (item_id, submitter_note, remove_background)
|
||||
VALUES ($1, 'a note', $2)`,
|
||||
[itemId, options.removeBackground]
|
||||
);
|
||||
await pool.query(
|
||||
`INSERT INTO item_images (item_id, image_path, sort_order)
|
||||
VALUES ($1, '/uploads/worker.jpg', 0)`,
|
||||
[itemId]
|
||||
);
|
||||
await fsp.writeFile(path.join(uploads, 'worker.jpg'), JPEG_BYTES);
|
||||
return itemId;
|
||||
}
|
||||
|
||||
async function originalPathOf(itemId: number): Promise<string | null> {
|
||||
const { rows } = await pool.query<{ original_image_path: string | null }>(
|
||||
`SELECT original_image_path FROM item_images WHERE item_id = $1`,
|
||||
[itemId]
|
||||
);
|
||||
return rows[0]?.original_image_path ?? null;
|
||||
}
|
||||
|
||||
describe('background removal after a draft', () => {
|
||||
// The mock has to actually be in play, or the two cases below would both
|
||||
// pass for the wrong reason — a draft that never happened cuts nothing out.
|
||||
it('drafts successfully, which is what the removal step follows', async () => {
|
||||
await seedSubmissionWithPhoto({ removeBackground: false });
|
||||
|
||||
expect(await draftQueued(1)).toEqual({ drafted: 1, failed: 0, skipped: 0 });
|
||||
});
|
||||
|
||||
// Recorded at submission and acted on here, so the sender never waits and a
|
||||
// sidecar that is down cannot fail their upload.
|
||||
it('cuts out the photos when the submitter asked for it', async () => {
|
||||
const itemId = await seedSubmissionWithPhoto({ removeBackground: true });
|
||||
|
||||
await draftQueued(1);
|
||||
|
||||
expect(await originalPathOf(itemId)).toBe('/uploads/worker.jpg');
|
||||
});
|
||||
|
||||
it('leaves the photos alone when they did not', async () => {
|
||||
const itemId = await seedSubmissionWithPhoto({ removeBackground: false });
|
||||
|
||||
await draftQueued(1);
|
||||
|
||||
expect(await originalPathOf(itemId)).toBeNull();
|
||||
});
|
||||
|
||||
// The governing rule: removal is a convenience on top of a draft that was
|
||||
// written correctly. A sidecar failure must never turn a good draft into a
|
||||
// failed one, because the queue is what the admin actually works from.
|
||||
it('leaves the draft ready when the sidecar fails', async () => {
|
||||
const itemId = await seedSubmissionWithPhoto({ removeBackground: true });
|
||||
// Configured, and nothing listening on it.
|
||||
process.env.REMBG_URL = 'http://127.0.0.1:1';
|
||||
|
||||
await draftQueued(1);
|
||||
|
||||
const { rows } = await pool.query<{ state: string }>(
|
||||
`SELECT state FROM item_drafts WHERE item_id = $1`,
|
||||
[itemId]
|
||||
);
|
||||
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();
|
||||
});
|
||||
});
|
||||
@@ -39,6 +39,13 @@ async function issueLink(label = 'Sarah', maxSubmissions?: number | null): Promi
|
||||
return res.body.token as string;
|
||||
}
|
||||
|
||||
/** A submission with optional extra multipart fields beside the photo. */
|
||||
function postPhoto(token: string, fields: Record<string, string> = {}) {
|
||||
const req = request(app).post(`/api/intake/${token}`);
|
||||
for (const [name, value] of Object.entries(fields)) void req.field(name, value);
|
||||
return req.attach('images', PNG, 'a.png');
|
||||
}
|
||||
|
||||
describe('checking a link before showing the form', () => {
|
||||
it('names the link so the page can greet the sender', async () => {
|
||||
const token = await issueLink('Sarah');
|
||||
@@ -207,3 +214,62 @@ describe('a submitted item does not reach the storefront', () => {
|
||||
expect(res.body).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('the background-removal intent', () => {
|
||||
// Ticked by default on the page, so absent means yes. An older client or a
|
||||
// curl call then behaves like the current default rather than silently
|
||||
// opting out of something every other submission gets.
|
||||
it('defaults to true when the field is not sent', async () => {
|
||||
const token = await issueLink();
|
||||
await postPhoto(token);
|
||||
|
||||
const { rows } = await pool.query<{ remove_background: boolean }>(
|
||||
`SELECT remove_background FROM item_drafts ORDER BY id DESC LIMIT 1`
|
||||
);
|
||||
expect(rows[0]?.remove_background).toBe(true);
|
||||
});
|
||||
|
||||
it('records a submitter who unticked it', async () => {
|
||||
const token = await issueLink();
|
||||
await postPhoto(token, { removeBackground: 'false' });
|
||||
|
||||
const { rows } = await pool.query<{ remove_background: boolean }>(
|
||||
`SELECT remove_background FROM item_drafts ORDER BY id DESC LIMIT 1`
|
||||
);
|
||||
expect(rows[0]?.remove_background).toBe(false);
|
||||
});
|
||||
|
||||
// Only the exact string opts out. A stray value is not a considered "no",
|
||||
// and reading it as one would quietly deny somebody something they asked for.
|
||||
it('treats anything other than "false" as consent', async () => {
|
||||
const token = await issueLink();
|
||||
await postPhoto(token, { removeBackground: 'no' });
|
||||
|
||||
const { rows } = await pool.query<{ remove_background: boolean }>(
|
||||
`SELECT remove_background FROM item_drafts ORDER BY id DESC LIMIT 1`
|
||||
);
|
||||
expect(rows[0]?.remove_background).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('what the submission page is told', () => {
|
||||
it('says the feature is off when there is no sidecar', async () => {
|
||||
delete process.env.REMBG_URL;
|
||||
const token = await issueLink();
|
||||
|
||||
const res = await request(app).get(`/api/intake/${token}`);
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.backgroundRemoval).toBe(false);
|
||||
});
|
||||
|
||||
it('says it is on when there is one', async () => {
|
||||
process.env.REMBG_URL = 'http://rembg-syn:7000';
|
||||
const token = await issueLink();
|
||||
|
||||
const res = await request(app).get(`/api/intake/${token}`);
|
||||
|
||||
expect(res.body.backgroundRemoval).toBe(true);
|
||||
delete process.env.REMBG_URL;
|
||||
});
|
||||
});
|
||||
|
||||
@@ -0,0 +1,22 @@
|
||||
import { cutoutPathFor } from '../../src/intake/backgroundRemoval';
|
||||
|
||||
describe('where a cut-out is written', () => {
|
||||
// A new file rather than a rewrite of the original, which is what makes the
|
||||
// original restorable at all — and what makes the JPEG-to-PNG change free,
|
||||
// since no existing path is renamed.
|
||||
it('sits beside the original with a -cutout suffix and a .png extension', () => {
|
||||
expect(cutoutPathFor('/uploads/abc-123.jpg')).toBe('/uploads/abc-123-cutout.png');
|
||||
});
|
||||
|
||||
// image_path is a contract, not a string: #103 made the stored value the
|
||||
// path uploadUrl joins an origin onto.
|
||||
it('keeps the /uploads/ prefix', () => {
|
||||
expect(cutoutPathFor('/uploads/x.webp')).toBe('/uploads/x-cutout.png');
|
||||
});
|
||||
|
||||
// The extension is replaced rather than appended, so a second pass cannot
|
||||
// produce `.png.png`.
|
||||
it('replaces the extension rather than appending to it', () => {
|
||||
expect(cutoutPathFor('/uploads/x.png')).toBe('/uploads/x-cutout.png');
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,146 @@
|
||||
import http from 'http';
|
||||
import { AddressInfo } from 'net';
|
||||
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]);
|
||||
const JPEG = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0]);
|
||||
|
||||
interface Capture {
|
||||
url: string;
|
||||
body: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* A stub sidecar on an ephemeral port.
|
||||
*
|
||||
* Port 0 rather than a fixed number: several ports in the 55000s are
|
||||
* Hyper-V-reserved on the development machine and bind with EACCES, and a
|
||||
* fixed port would also stop this suite running beside itself.
|
||||
*
|
||||
* A real HTTP server rather than a mocked `fetch`, because what is being
|
||||
* checked is the shape of the request that reaches the wire — above all that
|
||||
* `model=u2net` is in it.
|
||||
*/
|
||||
async function withStub(
|
||||
handler: (req: http.IncomingMessage, res: http.ServerResponse) => void,
|
||||
run: (capture: Capture) => Promise<void>
|
||||
): Promise<void> {
|
||||
const capture: Capture = { url: '', body: '' };
|
||||
const server = http.createServer((req, res) => {
|
||||
capture.url = req.url ?? '';
|
||||
const chunks: Buffer[] = [];
|
||||
req.on('data', (c: Buffer) => chunks.push(c));
|
||||
req.on('end', () => {
|
||||
capture.body = Buffer.concat(chunks).toString('latin1');
|
||||
handler(req, res);
|
||||
});
|
||||
});
|
||||
|
||||
await new Promise<void>((resolve) => server.listen(0, '127.0.0.1', resolve));
|
||||
const { port } = server.address() as AddressInfo;
|
||||
process.env.REMBG_URL = `http://127.0.0.1:${port}`;
|
||||
|
||||
try {
|
||||
await run(capture);
|
||||
} finally {
|
||||
delete process.env.REMBG_URL;
|
||||
await new Promise<void>((resolve) => server.close(() => resolve()));
|
||||
}
|
||||
}
|
||||
|
||||
function respondWithPng(_req: http.IncomingMessage, res: http.ServerResponse): void {
|
||||
res.writeHead(200, { 'Content-Type': 'image/png' });
|
||||
res.end(PNG);
|
||||
}
|
||||
|
||||
describe('whether the sidecar is configured', () => {
|
||||
it('is false when REMBG_URL is unset', () => {
|
||||
delete process.env.REMBG_URL;
|
||||
expect(isRembgConfigured()).toBe(false);
|
||||
});
|
||||
|
||||
// A variable set to spaces is a configuration mistake, not a value — the
|
||||
// same reading envValidation applies everywhere else.
|
||||
it('is false when REMBG_URL is blank', () => {
|
||||
process.env.REMBG_URL = ' ';
|
||||
expect(isRembgConfigured()).toBe(false);
|
||||
delete process.env.REMBG_URL;
|
||||
});
|
||||
|
||||
it('is true when REMBG_URL is set', () => {
|
||||
process.env.REMBG_URL = 'http://rembg-syn:7000';
|
||||
expect(isRembgConfigured()).toBe(true);
|
||||
delete process.env.REMBG_URL;
|
||||
});
|
||||
});
|
||||
|
||||
describe('asking the sidecar to remove a background', () => {
|
||||
/**
|
||||
* The single most important assertion in this file.
|
||||
*
|
||||
* The image's default model is `bria-rmbg`, licensed non-commercial, and it
|
||||
* is selected by simply not naming a model. Nothing about the returned image
|
||||
* would reveal it had been used, so this is the only place it can be caught.
|
||||
*/
|
||||
it('names u2net explicitly, because the default is licensed non-commercial', async () => {
|
||||
await withStub(respondWithPng, async (capture) => {
|
||||
await removeBackground(JPEG, 'image/jpeg');
|
||||
expect(capture.body).toContain('name="model"');
|
||||
expect(capture.body).toContain('u2net');
|
||||
});
|
||||
});
|
||||
|
||||
it('posts the file to /api/remove and returns the PNG it gets back', async () => {
|
||||
await withStub(respondWithPng, async (capture) => {
|
||||
const out = await removeBackground(JPEG, 'image/jpeg');
|
||||
expect(capture.url).toBe('/api/remove');
|
||||
expect(capture.body).toContain('name="file"');
|
||||
expect(out.subarray(0, 8)).toEqual(PNG.subarray(0, 8));
|
||||
});
|
||||
});
|
||||
|
||||
it('rejects rather than returning bytes when the sidecar errors', async () => {
|
||||
await withStub(
|
||||
(_req, res) => {
|
||||
res.writeHead(500);
|
||||
res.end('boom');
|
||||
},
|
||||
async () => {
|
||||
await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toThrow(/500/);
|
||||
await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toBeInstanceOf(
|
||||
SidecarRequestError
|
||||
);
|
||||
}
|
||||
);
|
||||
});
|
||||
|
||||
// The failure that would otherwise write an HTML error page over a
|
||||
// photograph. Checked with the same magic-byte helper the upload path uses,
|
||||
// rather than by trusting the Content-Type the sidecar sent.
|
||||
it('rejects a response that is not actually a PNG', async () => {
|
||||
await withStub(
|
||||
(_req, res) => {
|
||||
res.writeHead(200, { 'Content-Type': 'image/png' });
|
||||
res.end('<html>not an image</html>');
|
||||
},
|
||||
async () => {
|
||||
await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toThrow(/not a PNG/);
|
||||
await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toBeInstanceOf(
|
||||
SidecarRequestError
|
||||
);
|
||||
}
|
||||
);
|
||||
});
|
||||
|
||||
// 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
|
||||
);
|
||||
});
|
||||
});
|
||||
+12
-6
@@ -92,18 +92,20 @@
|
||||
# failing, so an empty value is a working configuration.
|
||||
# ANTHROPIC_API_KEY Optional. Drafts a listing from a submitted photo
|
||||
# (#223). Unset means submissions still arrive and wait
|
||||
# ANTHROPIC_WORKSPACE_ID Required alongside the key above when that key is
|
||||
# identity-linked. Without it every draft fails with a
|
||||
# 400 naming the missing header (#271).
|
||||
# INTAKE_ACTION_SECRET Optional. Signs the regenerate and discard links in the
|
||||
# intake notification email (#224). Absent, the email
|
||||
# still sends and carries no shortcuts.
|
||||
# undrafted, which is a working configuration for the
|
||||
# same reason USPS is. The one credential here that
|
||||
# spends money per call, and on a path anybody holding
|
||||
# an upload link can trigger — put a spend limit on the
|
||||
# key in the Anthropic console, because nothing in this
|
||||
# repository can enforce one.
|
||||
# ANTHROPIC_WORKSPACE_ID Required alongside the key above when that key is
|
||||
# identity-linked. Without it every draft fails with a
|
||||
# 400 naming the missing header (#271).
|
||||
# INTAKE_ACTION_SECRET Optional. Signs the regenerate and discard links in the
|
||||
# intake notification email (#224). Absent, the email
|
||||
# still sends and carries no shortcuts.
|
||||
# REMBG_URL Optional. The background-removal sidecar, e.g.
|
||||
# http://rembg-syn:7000. Unset turns the feature off.
|
||||
#
|
||||
# The names above are what this file reads. A stack variable under any other
|
||||
# name is substituted nowhere and never reaches the container, so reconciling
|
||||
@@ -238,6 +240,10 @@ services:
|
||||
# keys need no workspace.
|
||||
- ANTHROPIC_WORKSPACE_ID=${ANTHROPIC_WORKSPACE_ID:-}
|
||||
|
||||
# Optional. The background-removal sidecar (#281).
|
||||
# See docs/ops/image-background-removal-stack.md.
|
||||
- REMBG_URL=${REMBG_URL:-}
|
||||
|
||||
# Signs the regenerate and discard links in the intake notification email
|
||||
# (#224). Optional: absent, the notification still sends and links to the
|
||||
# review queue without shortcuts. Rotating it revokes every outstanding
|
||||
|
||||
@@ -63,6 +63,10 @@
|
||||
# in the notification email (#224). Absent, the email still
|
||||
# sends and simply carries no shortcuts. Its own value, not
|
||||
# production's: a link signed with it acts without a login.
|
||||
# QA_REMBG_URL — optional. The background-removal sidecar, e.g.
|
||||
# http://rembg-syn:7000. Unset turns the feature off rather
|
||||
# than breaking anything. The sidecar must be on the same
|
||||
# network as this stack.
|
||||
|
||||
services:
|
||||
redefined-designs-qa:
|
||||
@@ -164,6 +168,12 @@ services:
|
||||
# would turn the ordinary case into a different error.
|
||||
- ANTHROPIC_WORKSPACE_ID=${QA_ANTHROPIC_WORKSPACE_ID:-}
|
||||
|
||||
# Optional. The background-removal sidecar (#281). Unset means the
|
||||
# feature does not exist: no checkbox on the submission page, no control
|
||||
# in the review queue, and the worker skips the step. Empty default so an
|
||||
# unset stack variable cannot fail a deploy.
|
||||
- REMBG_URL=${QA_REMBG_URL:-}
|
||||
|
||||
# Signs the regenerate and discard links in the intake notification email
|
||||
# (#224). Optional: absent, the notification still sends and simply links
|
||||
# to the review queue without shortcuts. Anyone holding a link can act on
|
||||
|
||||
@@ -2,7 +2,7 @@
|
||||
|
||||
Setup notes for the Python image-processing stack behind the review queue's background-removal option.
|
||||
|
||||
**Status: evaluated, not adopted.** The engine choice is still open — see the "Alternatives ruled out" section. Everything below was run and measured on 2026-09-02 against `danielgatis/rembg:latest`, on a Windows dev box under Docker Desktop. **The NAS is a different machine and will be slower**; treat these as an upper bound on capability, not a promise.
|
||||
**Status: adopted (#281).** The engine choice is settled — see "The one thing that must not be got wrong" below. Everything here was run and measured on 2026-09-02 against `danielgatis/rembg:latest`, on a Windows dev box under Docker Desktop. **The NAS is a different machine and will be slower**; treat these as an upper bound on capability, not a promise.
|
||||
|
||||
## Why a sidecar and not the host's Python
|
||||
|
||||
@@ -88,8 +88,17 @@ curl -s -F "file=@any.jpg" -F "model=u2net" -o /dev/null http://localhost:7000/a
|
||||
|
||||
**A hosted API** (remove.bg, Photoroom) — clear terms, no disk cost, best quality. Roughly $0.20 an image, another credential, another external dependency in the pipeline, and would want the budget ceiling that #227 gave submissions.
|
||||
|
||||
## If this is adopted
|
||||
## How the application uses it
|
||||
|
||||
The application would `POST` to the sidecar rather than doing any inference itself, which keeps every Python and ONNX dependency out of the Node image. Given 1–2 s warm, a synchronous request from the review queue is reasonable — unlike drafting, which was deliberately moved off the request path because a stranger can trigger it and must never wait.
|
||||
`backend/src/intake/rembgClient.ts` is the only thing that talks to the sidecar. It posts to `/api/remove` and **always sends `model=u2net`**; a unit test asserts that parameter is present, because nothing about the returned image would reveal its absence.
|
||||
|
||||
The four design decisions already taken are independent of the engine: the cut-out replaces the item's image while the original is kept and restorable, the result is a transparent PNG, and the control sits on each photo in the review queue.
|
||||
`REMBG_URL` points at it — `http://rembg-syn:7000` on the NAS, where the container publishes `32700:7000`. The variable is optional in both compose files: unset means the submission page shows no checkbox, the review queue shows no control, and the drafting worker skips the step. An environment without a sidecar is a working environment.
|
||||
|
||||
Two entry points, one module (`backend/src/intake/backgroundRemoval.ts`):
|
||||
|
||||
- **The drafting worker**, honouring the checkbox on `/submit/:token`, which is ticked by default. The submitter's tick is recorded and acted on later, so nobody waits on a model run and an unreachable sidecar cannot fail an upload.
|
||||
- **The review queue**, per photo, synchronously — 1.1–2.3 s warm is a wait an admin who just clicked a button can absorb.
|
||||
|
||||
Because removal follows drafting in the worker, an environment with no `ANTHROPIC_API_KEY` drafts nothing and so cuts out nothing. The per-photo control in the review queue is the way to do it by hand there.
|
||||
|
||||
The original file is never destroyed. `item_images.original_image_path` records where it went, and **Restore original** swaps it back. Nothing in the feature deletes a file or a row.
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
@@ -10,7 +10,15 @@ import Empty from 'antd/es/empty';
|
||||
import Alert from 'antd/es/alert';
|
||||
import Modal from 'antd/es/modal';
|
||||
import message from 'antd/es/message';
|
||||
import { Draft, PriceSource, actOnDraft, fetchDrafts, publishDraft } from './draftsApi';
|
||||
import {
|
||||
Draft,
|
||||
DraftImage,
|
||||
PriceSource,
|
||||
actOnDraft,
|
||||
fetchDrafts,
|
||||
publishDraft,
|
||||
setImageBackground
|
||||
} from './draftsApi';
|
||||
|
||||
const { TextArea } = Input;
|
||||
|
||||
@@ -25,6 +33,64 @@ function priceLabel(source: PriceSource): string {
|
||||
return 'default price — nobody chose this';
|
||||
}
|
||||
|
||||
/**
|
||||
* One photo, with the control that cuts it out or puts it back.
|
||||
*
|
||||
* Per photo rather than per item because that is how a poor result is undone:
|
||||
* background removal produces the occasional bad cut on an unusual object, and
|
||||
* the answer is to restore that one photograph, not to unpick the submission.
|
||||
*
|
||||
* The label is the state. `original_image_path` is the only thing consulted,
|
||||
* so there is no second flag that could disagree with what the button does.
|
||||
*
|
||||
* `enabled` is computed by the caller as `backgroundRemoval || cutOut`, not as
|
||||
* `backgroundRemoval` alone. The two branches this button offers need opposite
|
||||
* things: removing calls the sidecar and has nothing to do without one, but
|
||||
* restoring is a pure database swap that needs no sidecar at all. Gating both
|
||||
* on `backgroundRemoval` hides Restore the moment REMBG_URL is unset — which
|
||||
* happens for real when the sidecar is decommissioned after photos were
|
||||
* already cut out — leaving a cut-out photo with no control and no way back to
|
||||
* the original short of hand-editing the database. That breaks the invariant
|
||||
* this whole feature rests on: the original is always restorable.
|
||||
*/
|
||||
function DraftPhoto({
|
||||
image,
|
||||
itemId,
|
||||
enabled,
|
||||
onChanged
|
||||
}: Readonly<{
|
||||
image: DraftImage;
|
||||
itemId: number;
|
||||
enabled: boolean;
|
||||
onChanged: () => void;
|
||||
}>) {
|
||||
const [busy, setBusy] = useState(false);
|
||||
const cutOut = image.original_image_path !== null;
|
||||
|
||||
const act = async () => {
|
||||
setBusy(true);
|
||||
try {
|
||||
await setImageBackground(itemId, image.id, cutOut ? 'restore-original' : 'remove-background');
|
||||
onChanged();
|
||||
} catch (err) {
|
||||
message.error(err instanceof Error ? err.message : 'that did not work');
|
||||
} finally {
|
||||
setBusy(false);
|
||||
}
|
||||
};
|
||||
|
||||
return (
|
||||
<Space direction="vertical" size={4} align="center">
|
||||
<img src={image.image_path} alt="" style={{ width: 120, height: 120, objectFit: 'cover' }} />
|
||||
{enabled && (
|
||||
<Button size="small" loading={busy} onClick={() => void act()}>
|
||||
{cutOut ? 'Restore original' : 'Remove background'}
|
||||
</Button>
|
||||
)}
|
||||
</Space>
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* One submission, with everything needed to judge it.
|
||||
*
|
||||
@@ -33,7 +99,11 @@ function priceLabel(source: PriceSource): string {
|
||||
* so — and 80.00 is a plausible price rather than an obvious sentinel, which is
|
||||
* exactly why it has to be called out rather than left to be noticed.
|
||||
*/
|
||||
function DraftCard({ draft, onChanged }: Readonly<{ draft: Draft; onChanged: () => void }>) {
|
||||
function DraftCard({
|
||||
draft,
|
||||
backgroundRemoval,
|
||||
onChanged
|
||||
}: Readonly<{ draft: Draft; backgroundRemoval: boolean; onChanged: () => void }>) {
|
||||
const [name, setName] = useState(draft.ai_name ?? draft.item_name);
|
||||
const [description, setDescription] = useState(
|
||||
draft.ai_description ?? draft.item_description ?? ''
|
||||
@@ -82,13 +152,14 @@ function DraftCard({ draft, onChanged }: Readonly<{ draft: Draft; onChanged: ()
|
||||
<Space direction="vertical" style={{ width: '100%' }} size="middle">
|
||||
{draft.ai_error && <Alert type="warning" message={`Drafting failed: ${draft.ai_error}`} />}
|
||||
|
||||
<Space wrap>
|
||||
<Space wrap align="start">
|
||||
{draft.images.map((image) => (
|
||||
<img
|
||||
<DraftPhoto
|
||||
key={image.id}
|
||||
src={image.image_path}
|
||||
alt=""
|
||||
style={{ width: 120, height: 120, objectFit: 'cover' }}
|
||||
image={image}
|
||||
itemId={draft.item_id}
|
||||
enabled={backgroundRemoval || image.original_image_path !== null}
|
||||
onChanged={onChanged}
|
||||
/>
|
||||
))}
|
||||
</Space>
|
||||
@@ -148,12 +219,15 @@ function DraftCard({ draft, onChanged }: Readonly<{ draft: Draft; onChanged: ()
|
||||
|
||||
export default function DraftQueue() {
|
||||
const [drafts, setDrafts] = useState<Draft[]>([]);
|
||||
const [backgroundRemoval, setBackgroundRemoval] = useState(false);
|
||||
const [state, setState] = useState<string | undefined>(undefined);
|
||||
const [error, setError] = useState<string | null>(null);
|
||||
|
||||
const load = useCallback(async () => {
|
||||
try {
|
||||
setDrafts(await fetchDrafts(state));
|
||||
const payload = await fetchDrafts(state);
|
||||
setDrafts(payload.drafts);
|
||||
setBackgroundRemoval(payload.backgroundRemoval);
|
||||
setError(null);
|
||||
} catch {
|
||||
setError('Could not load the review queue.');
|
||||
@@ -188,7 +262,12 @@ export default function DraftQueue() {
|
||||
{error && <Alert type="error" message={error} />}
|
||||
{!error && drafts.length === 0 && <Empty description="Nothing waiting for review" />}
|
||||
{drafts.map((draft) => (
|
||||
<DraftCard key={draft.item_id} draft={draft} onChanged={() => void load()} />
|
||||
<DraftCard
|
||||
key={draft.item_id}
|
||||
draft={draft}
|
||||
backgroundRemoval={backgroundRemoval}
|
||||
onChanged={() => void load()}
|
||||
/>
|
||||
))}
|
||||
</div>
|
||||
);
|
||||
|
||||
@@ -3,6 +3,12 @@ export type PriceSource = 'default' | 'ai' | 'admin';
|
||||
export interface DraftImage {
|
||||
id: number;
|
||||
image_path: string;
|
||||
/**
|
||||
* Where this photo came from, once it has been cut out. Null means it never
|
||||
* was — which is also the answer to whether Restore has anything to do, so
|
||||
* there is no second flag that could disagree with it.
|
||||
*/
|
||||
original_image_path: string | null;
|
||||
}
|
||||
|
||||
export interface Draft {
|
||||
@@ -31,11 +37,24 @@ async function send(path: string, init?: RequestInit): Promise<Response> {
|
||||
});
|
||||
}
|
||||
|
||||
export async function fetchDrafts(state?: string): Promise<Draft[]> {
|
||||
export interface DraftQueueResponse {
|
||||
drafts: Draft[];
|
||||
/**
|
||||
* Whether a background-removal sidecar is configured. False hides the
|
||||
* remove-background control rather than showing one that would answer 502
|
||||
* — an environment without a sidecar is a working environment. It does
|
||||
* *not* hide Restore original: that endpoint is a pure database swap and
|
||||
* needs no sidecar, so DraftQueue shows it whenever a photo has an
|
||||
* original to restore, regardless of this flag.
|
||||
*/
|
||||
backgroundRemoval: boolean;
|
||||
}
|
||||
|
||||
export async function fetchDrafts(state?: string): Promise<DraftQueueResponse> {
|
||||
const query = state ? `?state=${encodeURIComponent(state)}` : '';
|
||||
const res = await send(query);
|
||||
if (!res.ok) throw new Error('could not load the review queue');
|
||||
return (await res.json()).drafts;
|
||||
return res.json();
|
||||
}
|
||||
|
||||
export interface PublishInput {
|
||||
@@ -71,3 +90,29 @@ export async function actOnDraft(
|
||||
const res = await send(`/${itemId}/${action}`, { method: 'POST' });
|
||||
if (!res.ok) throw new Error(`could not ${action} this draft`);
|
||||
}
|
||||
|
||||
/**
|
||||
* Cut one photo out, or put its original back.
|
||||
*
|
||||
* The server's message is preferred over a generic one for the same reason
|
||||
* publishDraft prefers it: a 502 here says the removal service did not answer
|
||||
* and the photo is unchanged, which is the difference between "try again" and
|
||||
* "something is wrong with this item".
|
||||
*/
|
||||
export async function setImageBackground(
|
||||
itemId: number,
|
||||
imageId: number,
|
||||
action: 'remove-background' | 'restore-original'
|
||||
): Promise<void> {
|
||||
const res = await send(`/${itemId}/images/${imageId}/${action}`, { method: 'POST' });
|
||||
if (res.ok) return;
|
||||
|
||||
let message = 'could not change this photo';
|
||||
try {
|
||||
message = (await res.json()).error ?? message;
|
||||
} catch {
|
||||
// A non-JSON body is a proxy or gateway error rather than the app
|
||||
// refusing. The generic message is the honest thing to show.
|
||||
}
|
||||
throw new Error(message);
|
||||
}
|
||||
|
||||
@@ -8,6 +8,7 @@ import Input from 'antd/es/input';
|
||||
import Alert from 'antd/es/alert';
|
||||
import Spin from 'antd/es/spin';
|
||||
import Space from 'antd/es/space';
|
||||
import Checkbox from 'antd/es/checkbox';
|
||||
import { UploadOutlined } from '@ant-design/icons';
|
||||
import type { UploadFile } from 'antd/es/upload/interface';
|
||||
import { fetchIntakeLink, submitItem } from './intakeApi';
|
||||
@@ -33,6 +34,10 @@ export default function Submit() {
|
||||
const [checking, setChecking] = useState(true);
|
||||
const [files, setFiles] = useState<UploadFile[]>([]);
|
||||
const [note, setNote] = useState('');
|
||||
// Ticked by default. Most items look better cut out, and a submitter who
|
||||
// wants their kitchen table in the photograph can say so — the reverse
|
||||
// default would mean almost nobody got it.
|
||||
const [removeBackground, setRemoveBackground] = useState(true);
|
||||
const [sending, setSending] = useState(false);
|
||||
const [sent, setSent] = useState(false);
|
||||
const [error, setError] = useState<string | null>(null);
|
||||
@@ -62,7 +67,8 @@ export default function Submit() {
|
||||
// rather than map-then-filter because a type predicate cannot narrow to
|
||||
// File here — antd's RcFile extends it, so the predicate would widen.
|
||||
files.flatMap((f) => (f.originFileObj ? [f.originFileObj] : [])),
|
||||
note
|
||||
note,
|
||||
removeBackground
|
||||
);
|
||||
|
||||
setSending(false);
|
||||
@@ -129,6 +135,7 @@ export default function Submit() {
|
||||
onClick={() => {
|
||||
setFiles([]);
|
||||
setNote('');
|
||||
setRemoveBackground(true);
|
||||
setSent(false);
|
||||
}}
|
||||
>
|
||||
@@ -171,6 +178,17 @@ export default function Submit() {
|
||||
placeholder="What is it, what is it made of, how big, what condition, where did it come from? Anything you know helps — a photo cannot show any of it."
|
||||
/>
|
||||
|
||||
{state.kind === 'usable' && state.link.backgroundRemoval && (
|
||||
<Checkbox
|
||||
checked={removeBackground}
|
||||
onChange={(e) => setRemoveBackground(e.target.checked)}
|
||||
>
|
||||
{/* Described by what it does, not by how. Nobody sending in a
|
||||
vase knows what a cut-out or an alpha channel is. */}
|
||||
Remove the background from my photos
|
||||
</Checkbox>
|
||||
)}
|
||||
|
||||
{error && <Alert type="error" message={error} showIcon />}
|
||||
|
||||
<Button type="primary" onClick={send} loading={sending} disabled={files.length === 0}>
|
||||
|
||||
@@ -1,5 +1,11 @@
|
||||
export interface IntakeLink {
|
||||
label: string;
|
||||
/**
|
||||
* Whether there is a background-removal sidecar behind the checkbox. False
|
||||
* hides it entirely rather than showing a control that would do nothing —
|
||||
* an unconfigured environment is a working one, not a broken one.
|
||||
*/
|
||||
backgroundRemoval: boolean;
|
||||
}
|
||||
|
||||
export type LinkState =
|
||||
@@ -36,13 +42,18 @@ export type SubmitResult = { ok: true } | { ok: false; error: string };
|
||||
export async function submitItem(
|
||||
token: string,
|
||||
files: File[],
|
||||
note: string
|
||||
note: string,
|
||||
removeBackground: boolean
|
||||
): Promise<SubmitResult> {
|
||||
const body = new FormData();
|
||||
// The field name the server's multer instance listens on. Sending several
|
||||
// under one name is what makes req.files an array.
|
||||
for (const file of files) body.append('images', file);
|
||||
body.append('note', note);
|
||||
// A string, because that is all a multipart field can be. The server treats
|
||||
// only the exact 'false' as an opt-out, so this is the one value that has to
|
||||
// be got right.
|
||||
body.append('removeBackground', removeBackground ? 'true' : 'false');
|
||||
|
||||
const res = await fetch(`/api/intake/${encodeURIComponent(token)}`, {
|
||||
method: 'POST',
|
||||
|
||||
@@ -98,4 +98,23 @@ test.describe('The review queue', () => {
|
||||
await dialog.getByRole('button', { name: 'Cancel', exact: true }).click();
|
||||
await expect(card.getByText(/nobody chose this/)).toBeVisible();
|
||||
});
|
||||
|
||||
// The control that makes a poor cut survivable. Its label is its state:
|
||||
// "Remove background" until an original has been recorded, "Restore
|
||||
// original" afterwards, read from one field rather than two that could
|
||||
// disagree.
|
||||
//
|
||||
// Only the label is asserted, not a click. Pressing it would need a sidecar,
|
||||
// and a test that depends on a service taking forty seconds to start is
|
||||
// broken by construction — the swap itself is covered in the integration
|
||||
// suite against a stub.
|
||||
test('offers to remove the background on each photo', async ({ page, admin }) => {
|
||||
const note = `Cutout ${RUN}`;
|
||||
await submitAnItem(page, note);
|
||||
|
||||
await admin.open('Review queue');
|
||||
|
||||
const card = page.locator('.ant-card').filter({ hasText: note });
|
||||
await expect(card.getByRole('button', { name: 'Remove background' })).toBeVisible();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -73,4 +73,32 @@ test.describe('Sending in an item through a link', () => {
|
||||
// is not on the site.
|
||||
await expect(page.getByText(/Nothing is listed for sale until/)).toBeVisible();
|
||||
});
|
||||
|
||||
// Ticked by default, because that is the decision: most items look better
|
||||
// cut out, and the reverse default would mean almost nobody got it.
|
||||
test('offers to remove the background, already ticked', async ({ page }) => {
|
||||
await page.goto(`/submit/${token}`);
|
||||
|
||||
const control = page.getByRole('checkbox', { name: /remove the background/i });
|
||||
await expect(control).toBeVisible();
|
||||
await expect(control).toBeChecked();
|
||||
});
|
||||
|
||||
test('lets a sender turn it off and still send', async ({ page }) => {
|
||||
await page.goto(`/submit/${token}`);
|
||||
|
||||
const png = Buffer.from(
|
||||
'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==',
|
||||
'base64'
|
||||
);
|
||||
await page.setInputFiles('input[type="file"]', {
|
||||
name: `${RUN}-nobg.png`,
|
||||
mimeType: 'image/png',
|
||||
buffer: png
|
||||
});
|
||||
await page.getByRole('checkbox', { name: /remove the background/i }).uncheck();
|
||||
await page.getByRole('button', { name: 'Send' }).click();
|
||||
|
||||
await expect(page.getByRole('heading', { name: 'Thank you — it arrived' })).toBeVisible();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -270,6 +270,11 @@ function Start-Backend {
|
||||
# No PayPal credentials locally. DEMO_MODE lets the whole cart and checkout
|
||||
# path run without them and with no way to reach live PayPal.
|
||||
$env:DEMO_MODE = 'true'
|
||||
# Needs to be set for the submission page's checkbox to appear at all, but
|
||||
# not to resolve: no e2e submission reaches the sidecar, because the
|
||||
# worker only cuts out after a draft and drafting is not configured
|
||||
# locally.
|
||||
$env:REMBG_URL = 'http://127.0.0.1:7000'
|
||||
New-Item -ItemType Directory -Force -Path $env:UPLOADS_DIR *>$null
|
||||
|
||||
Write-Step 'Running migrations'
|
||||
|
||||
Reference in New Issue
Block a user