fix(external-media): store external paths from the media root (#1163) (#1168)

* 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 <[email protected]>
This commit is contained in:
Paul Nothaft
2026-08-26 08:36:45 +02:00
committed by GitHub
co-authored by Paul Nothaft
parent 36192708ab
commit a7b74bcd87
17 changed files with 1248 additions and 79 deletions
@@ -0,0 +1,364 @@
/**
* Folding the event's base path into every external row (#1163).
*
* Two things can go wrong and both are silent, which is why they are pinned
* here rather than left to review: folding a path that was ALREADY folded
* (every original moves), and "repairing" a healthy install because the media
* root happened to be unmounted when the migration ran (every original moves).
*
* The repair itself is driven against a real temp directory tree, because the
* whole mechanism is "is this file actually there" and a mocked fs would only
* be testing the mock.
*/
const path = require('path');
const fs = require('fs');
const os = require('os');
describe('migration 187 — external_relpath from the media root (#1163)', () => {
let knex; let tmpDir; let mediaRoot; let migration;
/** Writes `bytes` bytes and returns the size, so fixtures can record it the
* way an import would have. */
const touch = async (rel, bytes = 8) => {
const full = path.join(mediaRoot, rel);
await fs.promises.mkdir(path.dirname(full), { recursive: true });
await fs.promises.writeFile(full, Buffer.alloc(bytes));
return bytes;
};
beforeAll(async () => {
tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-mig187-'));
mediaRoot = path.join(tmpDir, 'media');
await fs.promises.mkdir(mediaRoot, { recursive: true });
process.env.EXTERNAL_MEDIA_ROOT = mediaRoot;
// The service caches the root on first call, so it must not have been
// resolved before EXTERNAL_MEDIA_ROOT was set above.
jest.resetModules();
migration = require('../../migrations/core/187_external_relpath_from_root');
knex = require('knex')({
client: 'sqlite3',
connection: { filename: path.join(tmpDir, 'db.sqlite') },
useNullAsDefault: true,
});
});
afterAll(async () => {
if (knex) await knex.destroy();
await fs.promises.rm(tmpDir, { recursive: true, force: true }).catch(() => {});
delete process.env.EXTERNAL_MEDIA_ROOT;
});
beforeEach(async () => {
await knex.schema.dropTableIfExists('photos');
await knex.schema.dropTableIfExists('events');
await knex.schema.dropTableIfExists('app_settings');
await knex.schema.createTable('events', (t) => {
t.increments('id').primary();
t.string('external_path');
});
await knex.schema.createTable('photos', (t) => {
t.increments('id').primary();
t.integer('event_id');
t.string('external_relpath');
t.integer('size_bytes');
t.string('source_origin').defaultTo('managed');
});
await knex.schema.createTable('app_settings', (t) => {
t.increments('id').primary();
t.string('setting_key');
t.text('setting_value');
t.string('setting_type');
t.string('updated_at');
});
await fs.promises.rm(mediaRoot, { recursive: true, force: true });
await fs.promises.mkdir(mediaRoot, { recursive: true });
});
const relpaths = async () =>
(await knex('photos').orderBy('id', 'asc').select('external_relpath'))
.map((r) => r.external_relpath);
it('folds the base path into every row of a healthy event', async () => {
await touch('Trip/Leknes/a.jpg');
await touch('Trip/Leknes/b.jpg');
await knex('events').insert({ id: 1, external_path: 'Trip' });
await knex('photos').insert([
{ event_id: 1, external_relpath: 'Leknes/a.jpg', source_origin: 'external' },
{ event_id: 1, external_relpath: 'Leknes/b.jpg', source_origin: 'external' },
]);
await migration.up(knex);
expect(await relpaths()).toEqual(['Trip/Leknes/a.jpg', 'Trip/Leknes/b.jpg']);
});
it('repairs rows an earlier import had rebased', async () => {
// The reported shape: a parent imported first, a child imported second, so
// events.external_path is the child and the parent's rows resolve into a
// path that does not exist.
const oldSize = await touch('Trip/Leknes/old.jpg', 11); // from the first import
const newSize = await touch('Trip/Sub/new.jpg', 22); // from the second
await knex('events').insert({ id: 1, external_path: 'Trip/Sub' });
await knex('photos').insert([
{ event_id: 1, external_relpath: 'Leknes/old.jpg', size_bytes: oldSize, source_origin: 'external' },
{ event_id: 1, external_relpath: 'new.jpg', size_bytes: newSize, source_origin: 'external' },
]);
await migration.up(knex);
// The old row is placed where the file actually is; the new one keeps
// resolving exactly where it resolved before.
expect(await relpaths()).toEqual(['Trip/Leknes/old.jpg', 'Trip/Sub/new.jpg']);
});
it('refuses an ancestor whose file is a different size', async () => {
// The dangerous case: the row's own file was simply deleted, and an
// UNRELATED file one directory up happens to share its name. Adopting it
// would make downloads serve the wrong original — worse than a dead link.
await touch('Trip/photo.jpg', 999);
await knex('events').insert({ id: 1, external_path: 'Trip/Sub' });
await knex('photos').insert({
event_id: 1, external_relpath: 'photo.jpg', size_bytes: 42, source_origin: 'external',
});
await migration.up(knex);
expect(await relpaths()).toEqual(['Trip/Sub/photo.jpg']);
});
it('refuses an ancestor when the row records no size to check against', async () => {
// Nothing to verify provenance with, so the row stays where it resolves
// today rather than adopting a same-named stranger.
await touch('Trip/photo.jpg', 100);
await knex('events').insert({ id: 1, external_path: 'Trip/Sub' });
await knex('photos').insert({
event_id: 1, external_relpath: 'photo.jpg', size_bytes: null, source_origin: 'external',
});
await migration.up(knex);
expect(await relpaths()).toEqual(['Trip/Sub/photo.jpg']);
});
it('leaves nothing folded when a rewrite fails partway', async () => {
// Without a transaction, a crash between the first event's UPDATE and the
// marker leaves mixed formats behind — and the next run folds the already
// folded rows a second time, putting every original one directory deeper.
await touch('A/one.jpg');
await touch('B/two.jpg');
await knex('events').insert([
{ id: 1, external_path: 'A' },
{ id: 2, external_path: 'B' },
]);
await knex('photos').insert([
{ event_id: 1, external_relpath: 'one.jpg', source_origin: 'external' },
{ event_id: 2, external_relpath: 'two.jpg', source_origin: 'external' },
]);
// app_settings is written last, in the same transaction as the rewrites.
await knex.schema.dropTableIfExists('app_settings_backup');
await knex.raw('CREATE TRIGGER fail_marker BEFORE INSERT ON app_settings '
+ "BEGIN SELECT RAISE(ABORT, 'boom'); END");
await expect(migration.up(knex)).rejects.toThrow(/boom/);
await knex.raw('DROP TRIGGER fail_marker');
// Every row still base-relative, and no marker — so a retry is correct.
expect(await relpaths()).toEqual(['one.jpg', 'two.jpg']);
expect(await knex('app_settings').where('setting_key', 'external_relpath_root_relative').first())
.toBeUndefined();
});
it('removes the losing row when two paths converge, instead of stranding it', async () => {
// Trip/Sub/c.jpg imported once via `Trip` (as `Sub/c.jpg`) and once via
// `Trip/Sub` (as `c.jpg`). Both fold to the same path. Skipping the loser
// would leave it base-relative under a root-only resolver — pointing at
// <root>/c.jpg — with the marker claiming the conversion is complete.
const size = await touch('Trip/Sub/c.jpg', 33);
await knex('events').insert({ id: 1, external_path: 'Trip/Sub' });
await knex('photos').insert([
{ event_id: 1, external_relpath: 'Sub/c.jpg', size_bytes: size, source_origin: 'external' },
{ event_id: 1, external_relpath: 'c.jpg', size_bytes: size, source_origin: 'external' },
]);
await migration.up(knex);
const rows = await knex('photos').select('external_relpath');
expect(rows).toHaveLength(1);
expect(rows[0].external_relpath).toBe('Trip/Sub/c.jpg');
});
it('survives a final path that equals another row\'s current path', async () => {
// `photo.jpg` repairs to `Trip/photo.jpg` while the row already holding
// `Trip/photo.jpg` folds to `Trip/Sub/Trip/photo.jpg`. Every FINAL value is
// distinct, but a one-pass rewrite collides halfway through — and on
// Postgres that 23505 is misread by the migration runner as "already
// applied", leaving everything unconverted.
const a = await touch('Trip/photo.jpg', 11);
const b = await touch('Trip/Sub/Trip/photo.jpg', 22);
await knex('events').insert({ id: 1, external_path: 'Trip/Sub' });
await knex('photos').insert([
{ event_id: 1, external_relpath: 'photo.jpg', size_bytes: a, source_origin: 'external' },
{ event_id: 1, external_relpath: 'Trip/photo.jpg', size_bytes: b, source_origin: 'external' },
]);
await migration.up(knex);
expect(await relpaths()).toEqual(['Trip/photo.jpg', 'Trip/Sub/Trip/photo.jpg']);
});
it('does not re-prefix a row inserted while the probe was running', async () => {
// Phase 1 runs outside the transaction and can take minutes on a cold
// mount. An import finishing in that window writes an already
// root-relative row, which a `where event_id` bulk update would prefix a
// second time with the stale base.
await touch('Trip/a.jpg');
await knex('events').insert({ id: 1, external_path: 'Trip' });
await knex('photos').insert({ event_id: 1, external_relpath: 'a.jpg', source_origin: 'external' });
const { foldExternalRelpaths } = require('../../src/services/externalRelpathFold');
const realStat = fs.promises.stat;
let injected = false;
jest.spyOn(fs.promises, 'access').mockImplementation(async (...args) => {
if (!injected) {
injected = true;
await knex('photos').insert({
event_id: 1, external_relpath: 'Trip/late.jpg', source_origin: 'external',
});
}
return realStat(args[0]).then(() => undefined);
});
await foldExternalRelpaths(knex);
fs.promises.access.mockRestore();
expect((await relpaths()).sort()).toEqual(['Trip/a.jpg', 'Trip/late.jpg']);
});
it('leaves a row it cannot place resolving where it resolves today', async () => {
// Never guess below current behaviour: a file that is genuinely gone must
// not have its path rewritten to some other file that happens to exist.
await touch('Trip/Sub/present.jpg');
await knex('events').insert({ id: 1, external_path: 'Trip/Sub' });
await knex('photos').insert([
{ event_id: 1, external_relpath: 'present.jpg', source_origin: 'external' },
{ event_id: 1, external_relpath: 'vanished.jpg', source_origin: 'external' },
]);
await migration.up(knex);
expect(await relpaths()).toEqual(['Trip/Sub/present.jpg', 'Trip/Sub/vanished.jpg']);
});
it('folds without repairing when the media root is unmounted', async () => {
// An unmounted share leaves the mountpoint as an empty directory, so every
// file looks missing. Repairing off that signal would move every original
// on a perfectly healthy install.
await knex('events').insert({ id: 1, external_path: 'Trip/Sub' });
await knex('photos').insert([
{ event_id: 1, external_relpath: 'Leknes/a.jpg', source_origin: 'external' },
]);
// mediaRoot is empty — see beforeEach.
await migration.up(knex);
expect(await relpaths()).toEqual(['Trip/Sub/Leknes/a.jpg']);
});
it('leaves managed rows alone', async () => {
await touch('Trip/a.jpg');
await knex('events').insert({ id: 1, external_path: 'Trip' });
await knex('photos').insert([
{ event_id: 1, external_relpath: null, source_origin: 'managed' },
{ event_id: 1, external_relpath: 'a.jpg', source_origin: 'external' },
]);
await migration.up(knex);
expect(await relpaths()).toEqual([null, 'Trip/a.jpg']);
});
it('leaves an event with no base path alone — its rows are already root-relative', async () => {
await touch('a.jpg');
await knex('events').insert({ id: 1, external_path: null });
await knex('photos').insert({ event_id: 1, external_relpath: 'a.jpg', source_origin: 'external' });
await migration.up(knex);
expect(await relpaths()).toEqual(['a.jpg']);
});
it('folds each event with its own base', async () => {
await touch('A/one.jpg');
await touch('B/two.jpg');
await knex('events').insert([
{ id: 1, external_path: 'A' },
{ id: 2, external_path: 'B' },
]);
await knex('photos').insert([
{ event_id: 1, external_relpath: 'one.jpg', source_origin: 'external' },
{ event_id: 2, external_relpath: 'two.jpg', source_origin: 'external' },
]);
await migration.up(knex);
expect(await relpaths()).toEqual(['A/one.jpg', 'B/two.jpg']);
});
it('tolerates a base path with stray slashes', async () => {
await touch('Trip/a.jpg');
await knex('events').insert({ id: 1, external_path: '/Trip/' });
await knex('photos').insert({ event_id: 1, external_relpath: 'a.jpg', source_origin: 'external' });
await migration.up(knex);
expect(await relpaths()).toEqual(['Trip/a.jpg']);
});
it('does not fold twice when run again', async () => {
// The failure this guards is total: every original on the install moves one
// directory deeper, and there is no undo.
await touch('Trip/a.jpg');
await knex('events').insert({ id: 1, external_path: 'Trip' });
await knex('photos').insert({ event_id: 1, external_relpath: 'a.jpg', source_origin: 'external' });
await migration.up(knex);
await migration.up(knex);
expect(await relpaths()).toEqual(['Trip/a.jpg']);
});
it('does not fold twice when the base repeats in the relpath', async () => {
// The inference this migration deliberately does NOT use: `Trip/x.jpg`
// under base `Trip` already "starts with the base", but has not been
// folded — it is a subfolder that shares its parent's name.
await touch('Trip/Trip/x.jpg');
await knex('events').insert({ id: 1, external_path: 'Trip' });
await knex('photos').insert({ event_id: 1, external_relpath: 'Trip/x.jpg', source_origin: 'external' });
await migration.up(knex);
expect(await relpaths()).toEqual(['Trip/Trip/x.jpg']);
});
it('rollback does not clear the marker, so a re-run cannot double-fold', async () => {
await touch('Trip/a.jpg');
await knex('events').insert({ id: 1, external_path: 'Trip' });
await knex('photos').insert({ event_id: 1, external_relpath: 'a.jpg', source_origin: 'external' });
await migration.up(knex);
await migration.down(knex);
await migration.up(knex);
expect(await relpaths()).toEqual(['Trip/a.jpg']);
});
it('no-ops before 041 has added the column', async () => {
await knex.schema.dropTableIfExists('photos');
await knex.schema.createTable('photos', (t) => { t.increments('id').primary(); });
await expect(migration.up(knex)).resolves.toBeUndefined();
});
});