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>
210 lines
8.0 KiB
JavaScript
210 lines
8.0 KiB
JavaScript
/**
|
|
* Two overlapping external imports insert every file twice (#1162).
|
|
*
|
|
* The route checked for an existing external_relpath and then inserted, with
|
|
* an fs.stat and a `sharp().metadata()` read sitting in between. A reporter
|
|
* double-clicked a slow import of a 6012-file tree and got 8004 rows.
|
|
*
|
|
* Both halves of the fix are driven here through the real route:
|
|
*
|
|
* - the in-flight guard, which turns the second click into a 409 instead of
|
|
* a second full walk of the tree;
|
|
* - convergence when the guard cannot help (another replica, another
|
|
* process), which is the unique index from migration 186 firing and the
|
|
* loop counting a skip rather than dying or duplicating.
|
|
*
|
|
* The second is exercised by inserting a competing row from inside the mocked
|
|
* `sharp().metadata()` call — literally inside the window the bug lived in.
|
|
*/
|
|
|
|
const fs = require('fs');
|
|
const path = require('path');
|
|
const os = require('os');
|
|
const express = require('express');
|
|
const request = require('supertest');
|
|
|
|
describe('concurrent external imports (#1162)', () => {
|
|
let tmpDir; let db; let app; let mediaRoot;
|
|
// When set, the mocked sharp metadata read inserts this row first — the
|
|
// other run winning the race between our SELECT and our INSERT.
|
|
let stealDuringMetadata = null;
|
|
let thumbnailDelayMs = 0;
|
|
|
|
beforeAll(async () => {
|
|
tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-extdup-'));
|
|
mediaRoot = path.join(tmpDir, 'media');
|
|
await fs.promises.mkdir(path.join(mediaRoot, 'nas', 'individual'), { recursive: true });
|
|
for (const name of ['a.jpg', 'b.jpg', 'c.jpg']) {
|
|
await fs.promises.writeFile(path.join(mediaRoot, 'nas', 'individual', name), 'not-a-real-jpeg');
|
|
}
|
|
|
|
process.env.NODE_ENV = 'test';
|
|
process.env.TEST_DATABASE_PATH = path.join(tmpDir, 'data', 'db.sqlite');
|
|
await fs.promises.mkdir(path.dirname(process.env.TEST_DATABASE_PATH), { recursive: true });
|
|
process.env.STORAGE_PATH = path.join(tmpDir, 'storage');
|
|
process.env.EXTERNAL_MEDIA_ROOT = mediaRoot;
|
|
process.env.JWT_SECRET = process.env.JWT_SECRET || 'extdup-secret';
|
|
|
|
jest.resetModules();
|
|
|
|
jest.doMock('../../src/middleware/auth', () => ({
|
|
adminAuth: (req, _res, next) => { req.admin = { id: 1, username: 'tester', roleName: 'admin' }; next(); },
|
|
}));
|
|
jest.doMock('../../src/middleware/permissions', () => ({
|
|
requirePermission: () => (_req, _res, next) => next(),
|
|
}));
|
|
jest.doMock('../../src/middleware/ownership', () => ({
|
|
requireEventOwnership: (_req, _res, next) => next(),
|
|
}));
|
|
|
|
// The window. In production this is a real decode of a NAS-hosted file —
|
|
// hundreds of milliseconds during which the row we just proved absent can
|
|
// appear. Standing in for the other run here makes that deterministic.
|
|
jest.doMock('sharp', () => () => ({
|
|
metadata: async () => {
|
|
if (stealDuringMetadata) {
|
|
const { db: liveDb } = require('../../src/database/db');
|
|
await liveDb('photos').insert(stealDuringMetadata);
|
|
stealDuringMetadata = null;
|
|
}
|
|
return { width: 100, height: 200 };
|
|
},
|
|
}));
|
|
|
|
jest.doMock('../../src/services/imageProcessor', () => ({
|
|
generateThumbnail: jest.fn(async () => {
|
|
if (thumbnailDelayMs) await new Promise((r) => setTimeout(r, thumbnailDelayMs));
|
|
return 'thumbnails/mock.jpg';
|
|
}),
|
|
ensureThumbnail: jest.fn(),
|
|
}));
|
|
|
|
jest.doMock('../../src/utils/logger', () => ({
|
|
debug: jest.fn(), info: jest.fn(), warn: jest.fn(), error: jest.fn(),
|
|
}));
|
|
|
|
({ db } = await require('./helpers/crmDb').bootCrmDb());
|
|
|
|
app = express();
|
|
app.use(express.json());
|
|
app.use('/api/admin/external-media', require('../../src/routes/adminExternalMedia'));
|
|
}, 180000);
|
|
|
|
afterAll(async () => {
|
|
if (db) await db.destroy?.();
|
|
await fs.promises.rm(tmpDir, { recursive: true, force: true }).catch(() => {});
|
|
});
|
|
|
|
async function seedEvent() {
|
|
await db('photos').del();
|
|
await db('events').del();
|
|
stealDuringMetadata = null;
|
|
thumbnailDelayMs = 0;
|
|
const [e] = await db('events').insert({
|
|
slug: `extdup-${Math.random().toString(36).slice(2, 8)}`,
|
|
event_type: 'wedding',
|
|
event_name: 'extdup',
|
|
event_date: '2026-01-01',
|
|
host_email: 'h@example.com',
|
|
admin_email: 'a@example.com',
|
|
password_hash: 'x',
|
|
share_link: `extdup-${Math.random()}`,
|
|
expires_at: new Date().toISOString(),
|
|
source_mode: 'reference',
|
|
}).returning('id');
|
|
return typeof e === 'object' ? e.id : e;
|
|
}
|
|
|
|
const runImport = (eventId) => request(app)
|
|
.post(`/api/admin/external-media/events/${eventId}/import-external`)
|
|
.send({ external_path: 'nas', recursive: true });
|
|
|
|
async function relpathCounts(eventId) {
|
|
const rows = await db('photos').where({ event_id: eventId }).select('external_relpath');
|
|
const counts = new Map();
|
|
for (const r of rows) counts.set(r.external_relpath, (counts.get(r.external_relpath) || 0) + 1);
|
|
return counts;
|
|
}
|
|
|
|
it('rejects a second import while the first is still running', async () => {
|
|
const eventId = await seedEvent();
|
|
// Enough to keep the first request inside its loop while the second
|
|
// arrives — the "slow import looks hung, so I clicked again" case.
|
|
thumbnailDelayMs = 20;
|
|
|
|
const [first, second] = await Promise.all([runImport(eventId), runImport(eventId)]);
|
|
|
|
const statuses = [first.status, second.status].sort();
|
|
expect(statuses).toEqual([200, 409]);
|
|
const rejected = first.status === 409 ? first : second;
|
|
expect(rejected.body.error).toMatch(/already running/i);
|
|
});
|
|
|
|
it('leaves exactly one row per file after both runs', async () => {
|
|
const eventId = await seedEvent();
|
|
thumbnailDelayMs = 20;
|
|
|
|
await Promise.all([runImport(eventId), runImport(eventId)]);
|
|
|
|
const counts = await relpathCounts(eventId);
|
|
expect(counts.size).toBe(3);
|
|
expect([...counts.values()]).toEqual([1, 1, 1]);
|
|
});
|
|
|
|
it('releases the event once the import finishes, so a re-import still works', async () => {
|
|
const eventId = await seedEvent();
|
|
|
|
expect((await runImport(eventId)).status).toBe(200);
|
|
// Not 409 — the guard is per run, not a permanent lock on the event.
|
|
const second = await runImport(eventId);
|
|
expect(second.status).toBe(200);
|
|
expect(second.body.imported).toBe(0);
|
|
expect(second.body.skipped).toBe(3);
|
|
});
|
|
|
|
it('converges when another writer wins the race mid-file', async () => {
|
|
// The guard is in-process, so it cannot see a second replica. This is what
|
|
// the unique index is for: the insert bounces, and the file is counted as
|
|
// skipped rather than duplicated or lost to a 500.
|
|
const eventId = await seedEvent();
|
|
stealDuringMetadata = {
|
|
event_id: eventId,
|
|
filename: 'a.jpg',
|
|
path: 'x/a.jpg',
|
|
type: 'individual',
|
|
source_origin: 'external',
|
|
// Root-relative, as the route now writes it (#1163) — the competing
|
|
// writer has to target the same value for the race to be real.
|
|
external_relpath: path.join('nas', 'individual', 'a.jpg'),
|
|
};
|
|
|
|
const res = await runImport(eventId);
|
|
|
|
expect(res.status).toBe(200);
|
|
const counts = await relpathCounts(eventId);
|
|
expect(counts.get(path.join('nas', 'individual', 'a.jpg'))).toBe(1);
|
|
// Two imported by us, one lost to the other writer and reported honestly.
|
|
expect(res.body.imported).toBe(2);
|
|
expect(res.body.skipped).toBe(1);
|
|
});
|
|
|
|
it('does not let one contended file abort the rest of the import', async () => {
|
|
const eventId = await seedEvent();
|
|
stealDuringMetadata = {
|
|
event_id: eventId,
|
|
filename: 'a.jpg',
|
|
path: 'x/a.jpg',
|
|
type: 'individual',
|
|
source_origin: 'external',
|
|
// Root-relative, as the route now writes it (#1163) — the competing
|
|
// writer has to target the same value for the race to be real.
|
|
external_relpath: path.join('nas', 'individual', 'a.jpg'),
|
|
};
|
|
|
|
await runImport(eventId);
|
|
|
|
// All three files present — the contended one via the other writer's row.
|
|
expect((await relpathCounts(eventId)).size).toBe(3);
|
|
});
|
|
});
|