Files
picpeak/backend/__tests__/integration/externalImportConcurrency.test.js
T
Paul NothaftandPaul Nothaft 06da1b9f7e fix(external-media): one row per external file per event (#1162) (#1167)
* fix(external-media): one row per external file per event (#1162)

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 186 removes the duplicates that already exist and adds a partial
  unique index on (event_id, external_relpath). The survivor is the lowest id
  that has a thumbnail, so a half-finished import does not cost a grid tile,
  and hero references are repointed first because the FK is SET NULL.
- the route treats a unique violation as a skip and carries on, so a second
  writer this process cannot see (another replica) converges instead of
  duplicating or 500ing.
- a second import while one is already running now gets a 409 rather than
  walking the whole tree to have every insert bounce.

The duplicates' thumbnail files are left behind as unreferenced bytes — a
migration is the wrong place to reach into storage, which may be S3.

* fix(external-media): keep dependent rows and legacy restores intact (#1162)

External review found two real defects in the dedupe half of this fix.

Dangling rows on SQLite. Every FK into photos declares ON DELETE CASCADE, but
PicPeak never sets `PRAGMA foreign_keys = ON` — the codebase says so where it
deletes an event (adminEvents/helpers.js:245) — so on every SQLite install the
cascade is inert and deleting a duplicate photo left its face embeddings,
guest feedback and admin marks behind, pointing at an id that no longer
exists. Biometric data outliving its photo is exactly the invariant the event
delete goes out of its way to hold.

Dependents are now handled explicitly, and moved rather than discarded where
they can be: the duplicates were separate tiles in the grid, so a guest's
comment or an admin's rating could legitimately be on either, and dropping it
inside a fix for silent data loss would be its own bug. Where the target
already holds an equivalent row — the same guest's like, the same admin's
mark, the same transfer's entry — the loser is dropped, because those tables
mean one row per (photo, actor). photo_faces is the deliberate exception: both
rows were scanned, so moving would duplicate every embedding and split the
person clusters built from them.

Legacy restores. Suspending FK enforcement does not suspend a UNIQUE index on
either engine, so a .picpeak backup taken before migration 186 — carrying
exactly the duplicates it removes — would hit the new index mid-batchInsert
and roll the whole restore back, after every table had already been emptied.
The restore now drops the index for the load and rebuilds it after running
the same dedupe.

Also: a failed CREATE INDEX is no longer swallowed. Recording the migration as
applied without it leaves the install permanently racy, with nothing to
trigger a retry.

The shared work moves to services/externalPhotoDedupe.js, which the migration
and the restore both call.

* fix(external-media): reconcile derived state around the dedupe (#1162)

Second review round, four more real findings.

The index throw did not actually stop anything. run-migrations-safe.js treats
23505 as "schema already exists" and marks the migration applied
(run-migrations-safe.js:138) — and a CREATE UNIQUE INDEX that finds duplicate
rows raises exactly 23505 on Postgres. A replica inserting one between the
dedupe and the index lock is a real rolling-deploy shape, and the outcome was
the thing the throw was added to prevent. The index is now verified against
the catalog afterwards, and failure raises a code-less error the runner cannot
mistake for idempotence.

Two people sharing a device were treated as one. photo_feedback carries both
guest_identifier (per device) and guest_id (per person, migration 078), and
feedbackService scopes by guest_id when present. Keying equivalence on the
identifier alone deleted one of two different people's ratings. It now uses
the same COALESCE rule the service does.

Deleting faces raw left ghost people. event_people counts and centroids are
derived from the photo_faces rows being removed, and #1132's separation
snapshots hold a copy of each side's centroid — which is why faceProcessor
exposes purgePhotoFaces and says it is "called from every photo-deletion
path". The dedupe now goes through it.

Reparenting feedback left the survivor's totals stale. photos carries
denormalized feedback_count / like_count / average_rating / favorite_count and
the later reaction and colour counts, so a survivor that now owns feedback kept
rendering zero. updatePhotoFeedbackStats takes a trx so the dedupe can
recompute on its own connection.

Also: the equivalence-key delimiter was a literal NUL byte, which made git
classify the whole file as binary and hide its diff. Escaped.

* fix(external-media): stop the dedupe discarding half-states (#1162)

Third review round. Five findings, four applied.

- is_hidden joins the feedback equivalence key. feedbackService lets a
  moderator-hidden row coexist with the guest's visible replacement and counts
  only the visible one, so ignoring it deleted the visible row as redundant.
- admin marks merge instead of dropping. rating and color_label are written
  independently, so the same admin can have rated one tile and coloured the
  other; the loser now hands over any field the winner has no value for.
- a survivor that loses the only completed scan is requeued. Otherwise the
  purge takes the sole embeddings and nothing re-queues it — the photo just
  silently stops having a face.
- view_count and download_count are carried over. Those are real interactions
  recorded per row, and dropping them quietly lowered the engagement the admin
  grid shows.

Not applied: repointing a category hero can in principle land on a survivor in
another category. It needs the two duplicate rows to have been re-categorised
apart after the racing import, and the result is a cosmetic hero mismatch that
the admin category routes already guard on write. Not worth the extra branch
in a data migration.

* fix(external-media): invalidate the download zip when duplicates are removed (#1162)

External review of the stable twin. Applies to both branches.

The pre-built "download everything" archive still contained the duplicate rows
the dedupe had just deleted, so guests kept receiving them until something
else happened to invalidate it. 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 is not something a
migration should start. getZipInfo already treats a cleared record as a cache
miss and rebuilds on the next request, so this is the durable half of what
invalidate does. The stale object is left in storage for the same reason the
duplicates' thumbnails are — a migration is the wrong place to reach into a
backend that may be S3.

---------

Co-authored-by: Paul Nothaft <[email protected]>
2026-08-26 08:28:48 +02:00

206 lines
7.7 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: '[email protected]',
admin_email: '[email protected]',
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',
external_relpath: path.join('individual', 'a.jpg'),
};
const res = await runImport(eventId);
expect(res.status).toBe(200);
const counts = await relpathCounts(eventId);
expect(counts.get(path.join('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',
external_relpath: path.join('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);
});
});