a7b74bcd87
* fix(external-media): store external paths from the media root (#1163) Importing a second folder into an event silently invalidated every photo already in it. photos.external_relpath was stored relative to events.external_path, and every import overwrites that column — so the older rows were rebased onto the new folder and their originals resolved to paths that do not exist. Nothing errored, and the grid still looked intact: thumbnails are written to local storage during the import while the base path is still correct. Only what needs the original broke — preview generation, the lightbox, downloads — which presents as a gallery that looks slow rather than one that is broken. The reporter had 7547 of 8004 rows pointing into the void and spent a while chasing it as a CPU problem. - external_relpath is now relative to EXTERNAL_MEDIA_ROOT, so a row is self-describing and nothing an admin does to the event afterwards can move an already-imported photo. - migration 187 folds each event's base path into its rows. Where the current resolution is missing on disk it walks up the base path for an ancestor under which the file IS there — the already-rebased case — and where it finds nothing it leaves the row resolving exactly where it resolves today. Skipped entirely when the media root is unmounted, since every file looks missing then. - the fold also runs after a .picpeak restore: knex_migrations is excluded from the archive, so a pre-#1163 backup would otherwise land base-relative rows on a migrated instance. - drops the duplicate-leaf-segment guess in photoResolver. It papered over this same double-prefixing and actively corrupts a root-relative path whose first segment legitimately repeats (base 'Trip', row 'Trip/x.jpg'). * fix(external-media): verify provenance and fold atomically (#1163) External review found four real defects in the fold. Repair could adopt the wrong file. Existence alone was accepted as proof that an ancestor candidate was the row's original — so a row whose file an admin simply deleted would adopt any same-named file one directory up (base `Trip/Sub`, relpath `photo.jpg`, an unrelated `Trip/photo.jpg`), and downloads would then serve a different photo. Worse than a dead link. An ancestor must now also match photos.size_bytes, which the import recorded from the very file the row describes; rows carrying no size are never repaired from an ancestor. The CURRENT base is still accepted on existence alone, because nothing is being inferred there — that is where the row already resolves. The fold was not atomic. Every UPDATE committed independently and the marker came last, so a process killed mid-fold left converted and unconverted rows with no marker — and the next run folded the converted ones a second time, putting every original one directory deeper with no undo. Probing is now a read-only first phase (so a slow cold NAS does not hold a write transaction open), and every rewrite plus the marker commit together. Failed rewrites certified a partial conversion. The per-row catch counted any error as a collision, carried on, and wrote the marker anyway — leaving that row in the old format for a resolver that now reads it differently. It also could not tell a genuine duplicate from a SQLite lock or I/O fault. Target collisions are now resolved in the planning phase, where they can be identified honestly, and a write that fails rolls the whole fold back. Restore ordering. The fold ran after the face requeue, with the worker live — so a worker could claim an external row while it was still base-relative, resolve it against the wrong path, and burn it to 'failed', a state only an explicit Re-scan clears. The fold now runs first, for the same reason the requeue already sat after restoreFiles. * fix(external-media): close the fold's remaining stranding paths (#1163) Second review round, three findings. A collision loser was left stranded. When an event imported one file through both `Trip` and `Trip/Sub`, two rows folded to the same path and the loser was skipped — keeping a base-relative value that the root-only resolver then reads as `<root>/<relpath>`, permanently wrong, with the marker claiming conversion was complete. It is a duplicate by construction, so it now goes through migration 186's deleteDuplicatePhotos, which reparents its feedback and marks and reconciles the face clusters instead of orphaning them. This branch is rebased onto #1162 for that helper. The other restore path had the same face-ordering bug. restoreService queued face scans in step 6, before step 7c runs pending migrations — so a pre-187 full or database restore handed the live worker rows whose paths were still event-relative, and it burned them to 'failed', a state the later fold does not clear. The requeue now happens after the migrations, where the files already are. A failed conversion was reported as a clean restore. The fold is transactional, so a failure leaves every external path in the old format under a resolver that reads from the media root — every original unreachable. It was logged as a warning and the restore returned success. It now returns externalPathsConverted/externalPathError, and suppresses the face requeue, which would otherwise mark those photos failed on top. * fix(external-media): make the fold safe against its own intermediate states (#1163) Third review round, four findings. A one-pass rewrite could collide with itself. Every FINAL path is distinct, but a final value can equal another row's CURRENT one — `photo.jpg` repairing to `Trip/photo.jpg` while the row already holding `Trip/photo.jpg` folds deeper — so the update violated migration 186's unique index halfway through. On Postgres that surfaces as 23505, which run-migrations-safe.js mistakes for "schema already exists" and records 187 as applied after the rollback, leaving every path unconverted with nothing to retry. Rows now park on a per-row staging value first, and migration 187 re-throws without the driver's code so the runner cannot misread it. The bulk update targeted rows the plan never saw. Phase 1 probes outside the transaction and can run for minutes; an import finishing in that window inserts an already root-relative row, and `where event_id` prefixed it again with the stale base. It now updates by the ids phase 1 captured. The restore UI never showed a conversion failure. The API carried externalPathsConverted, but PicpeakBackupCard neither declared nor read it and showed a green success either way — so an admin whose external originals were all unreachable was told the restore worked. restoreService requeued faces even when the migrations failed. The step 7c catch is deliberately non-fatal, so a pre-187 backup whose fold never ran still handed the live worker event-relative paths to burn to 'failed'. * fix(external-media): the fold's staging value must be storable on Postgres (#1163) External review of the stable twin caught this, and it was on both branches. The two-pass rewrite parks each row on a temporary value, and that value was written with a leading NUL. SQLite stores NUL in TEXT without complaint; Postgres rejects it outright — "invalid byte sequence for encoding UTF8: 0x00" — so migration 187 rolled back on exactly the installs that need the two-pass repair, and only on the engine most of them run. Restores hit the same wall and reported the conversion as failed. The prefix is ordinary text now. It still cannot collide with a real relative path and is still obviously wrong if a crash leaves one behind. Adds a gated Postgres test alongside the existing picpeakRestorePg one, because a SQLite-only suite structurally cannot catch this class: restoring the NUL makes exactly the two-pass repair case fail with that error, and nothing else. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
299 lines
13 KiB
JavaScript
299 lines
13 KiB
JavaScript
/**
|
|
* scripts/regenerate-thumbnails.js against external photos (#1148).
|
|
*
|
|
* The same defect #1129 fixed in the admin route, still standing in the CLI
|
|
* fallback: the script resolved every source as
|
|
* `storage/events/active/<photo.path>` and fs.access'd it. External and
|
|
* reference rows do not live there — their originals sit under
|
|
* `events.external_path` — so every one failed the check and was counted as an
|
|
* error. On an install where all photos are external the script did nothing at
|
|
* all, while reporting one error per photo.
|
|
*
|
|
* Driven against a REAL file on a REAL external mount with the real
|
|
* imageProcessor, not a mock: the whole point is that the source resolves off
|
|
* the mount, and a mocked ensureThumbnail would assert nothing about that.
|
|
*/
|
|
|
|
const fs = require('fs');
|
|
const path = require('path');
|
|
const os = require('os');
|
|
const sharp = require('sharp');
|
|
const { execFile } = require('child_process');
|
|
|
|
describe('regenerate-thumbnails script (#1148)', () => {
|
|
let tmpDir; let db; let cleanup; let regenerateThumbnails;
|
|
let eventId; let externalPhotoId; let videoPhotoId; let watcherVideoId; let repairPhotoId;
|
|
let vanishingPhotoId;
|
|
let externalRoot;
|
|
|
|
beforeAll(async () => {
|
|
tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-regen-script-'));
|
|
process.env.NODE_ENV = 'test';
|
|
process.env.TEST_DATABASE_PATH = path.join(tmpDir, 'data', 'test.db');
|
|
process.env.STORAGE_PATH = path.join(tmpDir, 'storage');
|
|
// External sources are sandboxed under EXTERNAL_MEDIA_ROOT. Rows carry a
|
|
// path relative to that root (#1163), so the 'wedding/' prefix on each
|
|
// external_relpath below is the event folder, not decoration.
|
|
process.env.EXTERNAL_MEDIA_ROOT = path.join(tmpDir, 'media');
|
|
externalRoot = path.join(process.env.EXTERNAL_MEDIA_ROOT, 'wedding');
|
|
|
|
await fs.promises.mkdir(path.dirname(process.env.TEST_DATABASE_PATH), { recursive: true });
|
|
await fs.promises.mkdir(process.env.STORAGE_PATH, { recursive: true });
|
|
await fs.promises.mkdir(externalRoot, { recursive: true });
|
|
|
|
jest.resetModules();
|
|
({ db, cleanup } = await require('./helpers/crmDb').bootCrmDb());
|
|
|
|
// A real image on the external mount — never under events/active.
|
|
await sharp({
|
|
create: { width: 1200, height: 800, channels: 3, background: { r: 10, g: 90, b: 160 } },
|
|
}).jpeg().toFile(path.join(externalRoot, 'shot.jpg'));
|
|
|
|
const [ev] = await db('events').insert({
|
|
slug: 'regen-script-event',
|
|
event_type: 'wedding',
|
|
event_name: 'Regen Script',
|
|
event_date: '2026-08-01',
|
|
host_email: 'h@example.com',
|
|
admin_email: 'a@example.com',
|
|
password_hash: 'x',
|
|
share_link: '/gallery/regen-script-event/share',
|
|
expires_at: new Date(Date.now() + 7 * 24 * 3600 * 1000).toISOString(),
|
|
source_mode: 'reference',
|
|
external_path: 'wedding',
|
|
created_at: new Date().toISOString(),
|
|
}).returning('id');
|
|
eventId = typeof ev === 'object' ? ev.id : ev;
|
|
|
|
const [p] = await db('photos').insert({
|
|
event_id: eventId,
|
|
filename: 'shot.jpg',
|
|
// `path` is what the old script joined onto events/active. Left
|
|
// populated on purpose: the fix must ignore it for an external row.
|
|
path: 'regen-script-event/shot.jpg',
|
|
type: 'individual',
|
|
source_origin: 'external',
|
|
external_relpath: 'wedding/shot.jpg',
|
|
uploaded_at: new Date().toISOString(),
|
|
}).returning('id');
|
|
externalPhotoId = typeof p === 'object' ? p.id : p;
|
|
|
|
const [v] = await db('photos').insert({
|
|
event_id: eventId,
|
|
filename: 'clip.mp4',
|
|
path: 'regen-script-event/clip.mp4',
|
|
type: 'individual',
|
|
media_type: 'video',
|
|
mime_type: 'video/mp4',
|
|
source_origin: 'external',
|
|
external_relpath: 'wedding/clip.mp4',
|
|
uploaded_at: new Date().toISOString(),
|
|
}).returning('id');
|
|
videoPhotoId = typeof v === 'object' ? v.id : v;
|
|
|
|
// How fileWatcher.processNewPhoto actually writes a video: `type` and
|
|
// `mime_type` set, media_type left to its 'image' default. A media_type-only
|
|
// filter lets this through and hands the container to Sharp.
|
|
//
|
|
// The file has to EXIST, otherwise the row fails resolution and looks
|
|
// skipped for the wrong reason — the bug is Sharp being handed a video, not
|
|
// a missing source. Real MP4 header bytes, no image in sight.
|
|
await fs.promises.writeFile(
|
|
path.join(externalRoot, 'watched.mp4'),
|
|
Buffer.from('00000018667479706d70343200000000', 'hex')
|
|
);
|
|
const [wv] = await db('photos').insert({
|
|
event_id: eventId,
|
|
filename: 'watched.mp4',
|
|
path: 'regen-script-event/watched.mp4',
|
|
type: 'video',
|
|
mime_type: 'video/mp4',
|
|
source_origin: 'external',
|
|
external_relpath: 'wedding/watched.mp4',
|
|
uploaded_at: new Date().toISOString(),
|
|
}).returning('id');
|
|
watcherVideoId = typeof wv === 'object' ? wv.id : wv;
|
|
expect((await db('photos').where('id', watcherVideoId).first()).media_type).not.toBe('video');
|
|
|
|
// A photo whose thumbnail_path points at something that is no longer there.
|
|
await sharp({
|
|
create: { width: 900, height: 600, channels: 3, background: { r: 200, g: 40, b: 40 } },
|
|
}).jpeg().toFile(path.join(externalRoot, 'repair.jpg'));
|
|
const [rp] = await db('photos').insert({
|
|
event_id: eventId,
|
|
filename: 'repair.jpg',
|
|
path: 'regen-script-event/repair.jpg',
|
|
type: 'individual',
|
|
thumbnail_path: 'thumbnails/thumb_ext_missing_repair.jpg',
|
|
source_origin: 'external',
|
|
external_relpath: 'wedding/repair.jpg',
|
|
uploaded_at: new Date().toISOString(),
|
|
}).returning('id');
|
|
repairPhotoId = typeof rp === 'object' ? rp.id : rp;
|
|
|
|
// A photo whose source will be removed after its canonical thumbnail is
|
|
// cached — the "mount went away" case, where the canonical rendition is
|
|
// served from cache but a tier still needs to read the original.
|
|
await sharp({
|
|
create: { width: 1000, height: 700, channels: 3, background: { r: 30, g: 140, b: 60 } },
|
|
}).jpeg().toFile(path.join(externalRoot, 'vanishing.jpg'));
|
|
const [vp] = await db('photos').insert({
|
|
event_id: eventId,
|
|
filename: 'vanishing.jpg',
|
|
path: 'regen-script-event/vanishing.jpg',
|
|
type: 'individual',
|
|
source_origin: 'external',
|
|
external_relpath: 'wedding/vanishing.jpg',
|
|
uploaded_at: new Date().toISOString(),
|
|
}).returning('id');
|
|
vanishingPhotoId = typeof vp === 'object' ? vp.id : vp;
|
|
|
|
({ regenerateThumbnails } = require('../../scripts/regenerate-thumbnails'));
|
|
}, 180000);
|
|
|
|
afterAll(async () => {
|
|
if (cleanup) await cleanup();
|
|
await fs.promises.rm(tmpDir, { recursive: true, force: true }).catch(() => {});
|
|
});
|
|
|
|
it('builds a thumbnail for an external photo instead of erroring on events/active', async () => {
|
|
// The location the old script computed and fs.access'd. Nothing is there,
|
|
// which is the whole defect — it is not where an external original lives.
|
|
// (The old script cannot be driven from a test directly: it had no export
|
|
// and ran on require, calling process.exit. Making it importable is part
|
|
// of this fix.)
|
|
const legacyPath = path.join(process.env.STORAGE_PATH, 'events/active', 'regen-script-event/shot.jpg');
|
|
expect(fs.existsSync(legacyPath)).toBe(false);
|
|
|
|
const result = await regenerateThumbnails(eventId, { tiers: false });
|
|
|
|
// The old script reported an error for this photo and wrote nothing.
|
|
expect(result.errorCount).toBe(0);
|
|
// The external photo, the repair row and the vanishing one; no video.
|
|
expect(result.successCount).toBe(3);
|
|
|
|
const row = await db('photos').where('id', externalPhotoId).first();
|
|
expect(row.thumbnail_path).toBeTruthy();
|
|
const onDisk = path.join(process.env.STORAGE_PATH, row.thumbnail_path);
|
|
expect(fs.existsSync(onDisk)).toBe(true);
|
|
|
|
// Named per-photo so two events referencing one NAS basename cannot
|
|
// clobber each other — the property ensureThumbnail owns and the reason
|
|
// the script must not build this name itself.
|
|
expect(path.basename(row.thumbnail_path)).toContain(`ext${externalPhotoId}_`);
|
|
});
|
|
|
|
it('leaves videos alone', async () => {
|
|
// A video thumbnail is a poster frame from videoProcessor; handing the
|
|
// container to Sharp produced one error per video row.
|
|
const row = await db('photos').where('id', videoPhotoId).first();
|
|
expect(row.thumbnail_path).toBeFalsy();
|
|
});
|
|
|
|
it('leaves a watcher-imported video alone, which carries no media_type', async () => {
|
|
// fileWatcher writes type + mime_type and lets media_type default to
|
|
// 'image', so filtering on media_type alone still fed these to Sharp. The
|
|
// signal is errorCount: the images are already done by now, so the only
|
|
// thing that can fail this run is a video reaching Sharp.
|
|
const result = await regenerateThumbnails(eventId, { tiers: false });
|
|
|
|
expect(result.errorCount).toBe(0);
|
|
const row = await db('photos').where('id', watcherVideoId).first();
|
|
expect(row.thumbnail_path).toBeFalsy();
|
|
});
|
|
|
|
it('is idempotent — a second run skips instead of rebuilding', async () => {
|
|
const before = await db('photos').where('id', externalPhotoId).first();
|
|
const result = await regenerateThumbnails(eventId, { tiers: false });
|
|
|
|
expect(result.errorCount).toBe(0);
|
|
expect(result.successCount).toBe(0);
|
|
expect(result.skipCount).toBe(3);
|
|
|
|
const after = await db('photos').where('id', externalPhotoId).first();
|
|
expect(after.thumbnail_path).toBe(before.thumbnail_path);
|
|
});
|
|
|
|
it('counts a repaired thumbnail as generated, not skipped', async () => {
|
|
// Both images are valid at this point. Destroy ONE thumbnail object while
|
|
// leaving thumbnail_path pointing at it — the corrupt/missing case.
|
|
const row = await db('photos').where('id', repairPhotoId).first();
|
|
const onDisk = path.join(process.env.STORAGE_PATH, row.thumbnail_path);
|
|
await fs.promises.rm(onDisk);
|
|
|
|
const result = await regenerateThumbnails(eventId, { tiers: false });
|
|
|
|
// On local and external storage the rebuilt key is identical, so inferring
|
|
// "skipped" from an unchanged path reports this repair as already valid —
|
|
// the one number an operator running this is actually reading.
|
|
expect(result.successCount).toBe(1);
|
|
expect(result.skipCount).toBe(2);
|
|
expect(result.errorCount).toBe(0);
|
|
expect(fs.existsSync(onDisk)).toBe(true);
|
|
});
|
|
|
|
it('backfills the responsive tiers, which is what a backfill is for', async () => {
|
|
// The tiers (#1095/#1109) are cached separately from thumbnail_path, so a
|
|
// gallery can hold every canonical rendition and still serve phones the
|
|
// full-size image. The old script only ever produced `thumb_<filename>` at
|
|
// a hard-coded 300px and could not backfill them at all.
|
|
const { THUMBNAIL_WIDTHS } = require('../../src/services/imageProcessor');
|
|
const imageRows = 3; // external, repaired and vanishing; videos excluded
|
|
const result = await regenerateThumbnails(eventId, { tiers: true });
|
|
|
|
expect(result.errorCount).toBe(0);
|
|
expect(result.tierCount).toBe(THUMBNAIL_WIDTHS.length * imageRows);
|
|
expect(result.tierFailures).toBe(0);
|
|
});
|
|
|
|
it('reports tiers it could not build instead of claiming success', async () => {
|
|
// ensureThumbnailAtWidth handles the expected failures itself and returns
|
|
// NULL rather than throwing — an unreachable mount, a storage write that
|
|
// did not land. A try/catch alone never sees those, so the run counted
|
|
// zero errors and printed a clean summary after backfilling nothing.
|
|
//
|
|
// Reproduced the honest way: cache the canonical rendition, then take the
|
|
// source away. The canonical is served from cache; the tiers still need
|
|
// the original.
|
|
const row = await db('photos').where('id', vanishingPhotoId).first();
|
|
expect(row.thumbnail_path).toBeTruthy();
|
|
|
|
const { deleteThumbnailTiers } = require('../../src/services/imageProcessor');
|
|
await deleteThumbnailTiers(row).catch(() => {});
|
|
await fs.promises.rm(path.join(externalRoot, 'vanishing.jpg'));
|
|
|
|
const result = await regenerateThumbnails(eventId, { tiers: true });
|
|
|
|
expect(result.tierFailures).toBeGreaterThan(0);
|
|
// Still not an error against the photo: the canonical rendition is intact
|
|
// and the gallery falls back to it.
|
|
expect(result.errorCount).toBe(0);
|
|
});
|
|
|
|
/** Run the CLI the way cron does, and hand back its exit status. */
|
|
const runCli = (args = []) => new Promise((resolve) => {
|
|
execFile(
|
|
process.execPath,
|
|
[path.join(__dirname, '..', '..', 'scripts', 'regenerate-thumbnails.js'), ...args],
|
|
{ env: { ...process.env }, cwd: path.join(__dirname, '..', '..') },
|
|
(error, stdout, stderr) => resolve({ code: error?.code ?? 0, stdout, stderr })
|
|
);
|
|
});
|
|
|
|
it('exits nonzero when work was left unfinished', async () => {
|
|
// Exit status is the only thing a cron job reads. `vanishing.jpg` still
|
|
// has no source, so its tiers cannot be built.
|
|
const failed = await runCli([String(eventId)]);
|
|
expect(failed.code).toBe(1);
|
|
expect(failed.stderr).toContain('completed with failures');
|
|
}, 120000);
|
|
|
|
it('exits zero when there is nothing left to do', async () => {
|
|
// Same event with tiers switched off: every canonical rendition is already
|
|
// valid, so a clean run must not cry wolf at automation.
|
|
const ok = await runCli([String(eventId), '--no-tiers']);
|
|
expect(ok.code).toBe(0);
|
|
expect(ok.stdout).toContain('Script completed successfully');
|
|
}, 120000);
|
|
});
|