Paul NothaftandPaul Nothaft
e9fcf4960e
* fix(external-media): one row per external file per event (#1162) Stable twin of #1167. Two overlapping import-external runs against the same event inserted every file twice. The route checked for an existing external_relpath and then inserted, with an fs.stat and a sharp().metadata() read sitting in between — a window wide enough for both runs to see "not there". A reporter's event held 8004 rows for 6012 distinct paths. Nothing at the storage layer stopped it: migration 041 created only a NON-unique (event_id, source_origin) index. - migration 176 removes the existing duplicates and adds a partial unique index on (event_id, external_relpath), verified against the catalog afterwards — a failed CREATE INDEX raises 23505 on Postgres, which run-migrations-safe treats as "schema already exists" and would record as applied on an install that never got the index. - dependent rows are removed explicitly rather than by cascade: PicPeak never sets `PRAGMA foreign_keys = ON`, so on SQLite the declared CASCADE is inert and a bare delete strands feedback and access-log rows. Guest feedback moves to the survivor instead of being discarded, keyed on guest identity the way feedbackService defines it, and the survivor's denormalized counters are recomputed. - the route treats a unique violation as a skip, so a writer this process cannot see converges instead of duplicating, and a second import while one is running gets a 409. - a .picpeak taken before migration 176 carries exactly these duplicates, and suspending FK enforcement does not suspend a unique index — so the restore drops the index for the load and rebuilds it after running the same dedupe. Divergences from the main twin, both because the feature is absent here: faces (no faceProcessor, so no purgePhotoFaces reconciliation — the rows are still deleted so nothing dangles), admin marks, transfer membership, and photos.view_count/download_count. The service guards each on hasTable / hasColumn, so those branches simply do not fire. Verified on this branch: 36 new tests pass; full suite leaves the same 5 pre-existing failures as origin/stable, unchanged. * fix(external-media): invalidate the download zip when duplicates are removed (#1162) External review. Same fix as the main twin. The pre-built "download everything" archive still contained the duplicate rows the dedupe had just deleted, so guests kept receiving them. Every ordinary photo-deletion path calls downloadZipService.invalidate for exactly this reason. The columns are cleared rather than the service being called: that service carries debounce timers and a regeneration queue, which a migration should not start. getZipInfo already treats a cleared record as a cache miss and rebuilds on the next request. The stale object is left in storage, as elsewhere. --------- Co-authored-by: Paul Nothaft <[email protected]>
93 lines
3.5 KiB
JavaScript
93 lines
3.5 KiB
JavaScript
/**
|
|
* A pre-#1162 backup must still restore (#1162 review).
|
|
*
|
|
* `replaceAllTables` suspends FOREIGN KEY enforcement for the load — Postgres
|
|
* via `session_replication_role = replica`, SQLite via `defer_foreign_keys` —
|
|
* but neither of those suspends a UNIQUE index. An archive taken before
|
|
* migration 186 carries exactly the duplicate photo rows that migration
|
|
* removes, so the batchInsert would hit the new index and roll the entire
|
|
* restore back, after every table had already been emptied.
|
|
*
|
|
* These pin the drop → load → dedupe → recreate sequence the restore now
|
|
* performs, and the failure it exists to prevent.
|
|
*/
|
|
|
|
const path = require('path');
|
|
const fs = require('fs');
|
|
const os = require('os');
|
|
|
|
const {
|
|
dedupeExternalPhotos,
|
|
createExternalRelpathIndex,
|
|
dropExternalRelpathIndex,
|
|
} = require('../../src/services/externalPhotoDedupe');
|
|
|
|
describe('restoring an archive that predates the unique index (#1162)', () => {
|
|
let knex; let tmpDir;
|
|
|
|
// What a pre-186 archive's photos.ndjson holds for a racing import: the same
|
|
// file twice, sub-millisecond apart.
|
|
const ARCHIVE_ROWS = [
|
|
{ id: 1, event_id: 1, external_relpath: 'Trip/a.jpg', source_origin: 'external' },
|
|
{ id: 2, event_id: 1, external_relpath: 'Trip/a.jpg', source_origin: 'external' },
|
|
{ id: 3, event_id: 1, external_relpath: 'Trip/b.jpg', source_origin: 'external' },
|
|
];
|
|
|
|
beforeAll(async () => {
|
|
tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-restore-dedupe-'));
|
|
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(() => {});
|
|
});
|
|
|
|
beforeEach(async () => {
|
|
await knex.schema.dropTableIfExists('photos');
|
|
await knex.schema.createTable('photos', (t) => {
|
|
t.integer('id').primary();
|
|
t.integer('event_id');
|
|
t.string('external_relpath');
|
|
t.string('thumbnail_path');
|
|
t.string('source_origin').defaultTo('managed');
|
|
});
|
|
await createExternalRelpathIndex(knex);
|
|
});
|
|
|
|
it('would abort the whole restore without the drop', async () => {
|
|
// The regression, stated directly: this is what the target instance does
|
|
// today when handed a legacy archive.
|
|
await expect(knex.batchInsert('photos', ARCHIVE_ROWS, 100)).rejects.toThrow(/unique/i);
|
|
});
|
|
|
|
it('loads, dedupes and comes back constrained', async () => {
|
|
await dropExternalRelpathIndex(knex);
|
|
await knex.batchInsert('photos', ARCHIVE_ROWS, 100);
|
|
|
|
const removed = await dedupeExternalPhotos(knex);
|
|
await createExternalRelpathIndex(knex);
|
|
|
|
expect(removed).toBe(1);
|
|
expect((await knex('photos').orderBy('id')).map((r) => r.external_relpath))
|
|
.toEqual(['Trip/a.jpg', 'Trip/b.jpg']);
|
|
// The target must not be left unprotected by the restore that dropped it.
|
|
await expect(
|
|
knex('photos').insert({ id: 9, event_id: 1, external_relpath: 'Trip/b.jpg', source_origin: 'external' })
|
|
).rejects.toThrow(/unique/i);
|
|
});
|
|
|
|
it('is a no-op for an archive that has no duplicates', async () => {
|
|
await dropExternalRelpathIndex(knex);
|
|
await knex.batchInsert('photos', ARCHIVE_ROWS.slice(1), 100);
|
|
|
|
expect(await dedupeExternalPhotos(knex)).toBe(0);
|
|
await expect(createExternalRelpathIndex(knex)).resolves.toBeUndefined();
|
|
expect(await knex('photos').count('* as c').first()).toEqual({ c: 2 });
|
|
});
|
|
});
|