fix(archives): take the restored category from the manifest (#1240) (stable) (#1243)

* fix(archives): take the restored category from the manifest (#1240) (stable)

Stable twin of #1240. The reporter hit this on 3.46.7 — stable — with 596
photos restored and 0 categories, so this is the branch the bug was actually
found on.

Stable carries the manifest on both sides already: archiveService selects
`photo_categories.name as category_name` and serialises it, and the restore
route builds manifestByFilename. It just never read the category out of it,
deriving one from the ZIP's first path segment instead. Archives store photos
as they sit on disk, so an event whose photos live in the gallery root
produces a flat zip, no category resolves, and every photo comes back with
category_id null — silently, behind a 200.

Carries the whole of #1240, not a subset: the manifest-first resolution and
the shared resolveCategoryId from Marian's commit, plus the follow-up that
makes the manifest authoritative when it says "no category" — an entry with a
null category_name is a photo that was genuinely uncategorized, and falling
through to the directory contradicted the record being restored from. That
matters because the directory is not a category: entry names are the storage
key minus events/active/{slug}, so a real archive yields `individual/` and
`collages/`, and reading the first segment invents categories with those
names.

Re-verified on stable rather than assumed: all four tests pass here, and three
of them fail against stable's current route, with the legacy no-manifest
fallback passing either way. The two changed files are byte-identical to main.

Co-authored-by: Marian <[email protected]>

* fix(archives): keep the stable twin to stable's schema, and close two category holes

External review on #1243 caught that this twin was ported wrong and that the
category resolver has two holes the main PR shares.

PORTED WRONG. I took main's whole adminArchives.js rather than applying the
category change to stable's, which dragged in main-only face cleanup:
photo_faces and event_people have migrations on main and none on stable, so
every permanent archive deletion would have thrown a missing-table error —
after the ZIP was already unlinked, leaving the event archived with its
archive gone and a 500 back. Rebuilt from stable's file with only the category
change; the diff against stable is now the fix and nothing else.

GLOBAL CATEGORIES WERE CLONED. Seeded categories (Ceremony, Reception) have
event_id NULL, so an event-only lookup missed them and created a second row —
and is_global defaults to TRUE, so that duplicate then appeared in every other
event's category list. The lookup now uses the same visibility rule the photo
routes use (own rows OR global), and anything it does create is explicitly
is_global false.

ORIGINAL-FILENAME ARCHIVES MATCHED NOTHING. With
general_use_original_filenames_for_downloads on at archive time, archiveService
names each ZIP entry after the original filename while the manifest stays keyed
by photos.filename — so the lookup missed every entry and those archives lost
categories exactly as before the fix. The manifest is now indexed by
original_filename as well, without letting it shadow a real filename key.

7 tests, three of them new; each new one fails against the un-fixed route and
the legacy no-manifest fallback passes throughout.

* fix(archives): sanitized original names and deterministic category scope

Round 2 of external review on #1243.

The original_filename index used the raw column, but archiveService runs the
name through sanitizeForZipEntry() before writing the entry — so an original
containing a slash or control byte was emitted under a different name than the
manifest records, and the lookup missed it. Both spellings are indexed now,
using the same helper the writer uses.

Not total, and the comment says so: uniquifyZipNames() appends `_1` when two
photos in one event share an original name, and that suffix cannot be
reconstructed from the manifest. Those fall through to the directory exactly
as they did before this fix — no worse, just not better. Closing it needs the
emitted name recorded at archive time, which is a writer change and a new
archive format.

The category lookup used one OR-query with .first(). An event-scoped category
and a global one may share a name — the category API permits it — so the
engine picked whichever, and a photo could be silently reassigned to the
global row, losing event-local settings like allow_downloads. Two queries now,
event-scoped first: the event's own row is the more specific answer.

9 tests, two new; both fail against the un-fixed route.

* fix(archives): don't adopt another event's legacy row, don't guess an alias

Round 3 of external review on #1243.

The global fallback matched on is_global alone. The very bug fixed here left
rows behind on upgraded instances — event-owned AND is_global true, because
the column defaults true — so restoring event B could adopt event A's
leftover, tying B's photos to a category that disappears when A is deleted.
The fallback now requires event_id IS NULL: genuinely global, not merely
flagged.

The original-filename alias map collapsed rows that share a basename.
archiveService treats `individual/IMG.jpg` and `collages/IMG.jpg` as distinct
paths and suffixes neither, so both manifest rows claimed one alias and
whichever won handed the other photo someone else's category. An alias claimed
by more than one row is now dropped and logged, so those photos fall back to
the directory: an unresolved category is recoverable, a confidently wrong one
is not.

11 tests, two new; both fail against the un-fixed route.

* fix(archives): make the manifest lookup order-independent and collision-safe

Two bugs found by an external review round, both in the manifest index.

The canonical map silently kept the last row for a duplicated
photos.filename. That column is not unique within an event — s3AutoImporter
takes path.basename(entry.key) and dedupes by path, so two imported files in
different subfolders both land as IMG_1234.jpg with different paths. At
restore both ZIP entries reduce to the same basename, so one photo got the
other's category. Contested names are dropped now, like ambiguous aliases
already were.

The alias pass could also evict a canonical key: when one row's
original_filename equalled another row's filename, the collision was marked
ambiguous and the sweep deleted the canonical entry. The comment two lines
above says a real filename key is authoritative and must never be
overwritten — the code did the opposite, and which way it went depended on
manifest iteration order, since the archive query has no ORDER BY.

Split into two passes so canonical names are claimed first and aliases only
fill names no canonical row wanted.

* fix(archives): treat a canonical/alias name clash as ambiguous, resolve categories lazily

Round-2 findings, one of which corrects my own round-1 fix.

Round 1 made a canonical filename outrank any alias. That is the wrong
tiebreak: when photo A's filename equals photo B's original_filename, which
file the ZIP actually emitted under that name depends on whether
original-filename archiving was on at archive time — with it ON the entry is
B's, with it OFF it is A's — and the manifest does not record the mode.
Preferring either silently mislabels the other half of the time, so the name
is dropped and both fall through to the directory. What the two-pass split
still buys is determinism: the archive query has no ORDER BY, so this used to
be a coin flip between dropping the name and overwriting it.

Categories are resolved inside the !existingPhoto branch. resolveCategoryId
find-or-CREATES, and archiveEvent retains photo rows, so restoring an archive
whose rows still exist created a category from the stale manifest name that
nothing then used — renaming a category while its event was archived left the
old name behind as an empty duplicate.

Not fixed: two event-scoped categories may share a display name with distinct
slugs, and the .first() lookup then picks either row, so manifest entries from
both collapse onto one id and can inherit the wrong allow_downloads. Detecting
it is easy; resolving it correctly needs a stable category identifier in the
manifest, which is a writer change and an archive-format bump.

* fix(archives): make a duplicate category name deterministic, and log it

Round-2 finding. Two event-scoped categories may share a display name when
their slugs differ, and the .first() lookup then picked one arbitrarily —
manifest entries for both collapsed onto a single id and half the photos
inherited the wrong per-category settings, allow_downloads above all.

Fixing it properly needs a stable category identifier in the manifest: a
writer change, an archive-format bump, and no help at all for archives
already written. Not worth building before knowing it happens. So the
collision is surfaced instead — a warning naming the category and the row
count — and the tiebreak is made deterministic (lowest id) so at least a
re-run lands the same way twice.

If this never fires in real logs, the format change was not worth making. If
it does, this is the evidence for it.

---------

Co-authored-by: Paul Nothaft <[email protected]>
Co-authored-by: Marian <[email protected]>

backend/__tests__/integration/adminArchives.restoreCategories.test.js

@@ -0,0 +1,477 @@
+/**
+ * Restoring an archive must put the photos back into their categories.
+ *
+ * The archive writer already persists `category_name` per photo in
+ * `photos_manifest.json` — that is why the manifest exists, and the comment
+ * above it says so: "(and category linkage) can't be derived from the
+ * extracted files alone". The restore route then read only
+ * `original_filename` from it and kept deriving the category from the ZIP's
+ * first path segment.
+ *
+ * Archives store photos exactly as they sit on disk, so an event whose photos
+ * live in the gallery root produces a FLAT zip. `path.dirname()` is '.' for
+ * every entry, no category is resolved, and every restored photo lands with
+ * `category_id = null` — silently, with a 200 response.
+ *
+ * These pin the manifest as the source of truth, with the directory as the
+ * fallback that keeps foldered and legacy archives working.
+ */
+
+const fs = require('fs');
+const path = require('path');
+const os = require('os');
+const express = require('express');
+const request = require('supertest');
+
+describe('archive restore restores categories (flat archives included)', () => {
+  let tmpDir; let db; let cleanup; let app; let storagePath;
+
+  beforeAll(async () => {
+    tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-restore-cat-'));
+    storagePath = path.join(tmpDir, 'storage');
+    process.env.NODE_ENV = 'test';
+    process.env.TEST_DATABASE_PATH = path.join(tmpDir, 'data', 'test.db');
+    process.env.STORAGE_PATH = storagePath;
+    await fs.promises.mkdir(path.dirname(process.env.TEST_DATABASE_PATH), { recursive: true });
+    await fs.promises.mkdir(path.join(storagePath, 'archives'), { recursive: true });
+
+    jest.resetModules();
+    jest.doMock('../../src/middleware/auth', () => ({
+      adminAuth: (req, _res, next) => { req.admin = { id: 1, username: 'tester' }; next(); },
+    }));
+    jest.doMock('../../src/middleware/permissions', () => ({
+      requirePermission: () => (_req, _res, next) => next(),
+    }));
+    jest.doMock('../../src/middleware/ownership', () => ({
+      requireEventOwnership: (_req, _res, next) => next(),
+    }));
+
+    ({ db, cleanup } = await require('./helpers/crmDb').bootCrmDb());
+    // bootCrmDb points STORAGE_PATH at its own tmp dir; follow it rather than
+    // fighting it, so the archives the tests write are where the route looks.
+    storagePath = process.env.STORAGE_PATH;
+    await fs.promises.mkdir(path.join(storagePath, 'archives'), { recursive: true });
+
+    app = express();
+    app.use(express.json());
+    app.use('/admin/archives', require('../../src/routes/adminArchives'));
+  }, 180000);
+
+  afterAll(async () => {
+    if (cleanup) await cleanup();
+    await fs.promises.rm(tmpDir, { recursive: true, force: true }).catch(() => {});
+  });
+
+  beforeEach(async () => {
+    await db('photos').del();
+    await db('photo_categories').del();
+    await db('events').del();
+  });
+
+  /** A one-pixel JPEG is enough; the route only stats the extracted file. */
+  const PIXEL = Buffer.from(
+    '/9j/4AAQSkZJRgABAQEAYABgAAD/2wBDAAgGBgcGBQgHBwcJCQgKDBQNDAsLDBkSEw8UHRofHh0a'
+    + 'HBwgJC4nICIsIxwcKDcpLDAxNDQ0Hyc5PTgyPC4zNDL/wAALCAABAAEBAREA/8QAFAABAAAAAAAA'
+    + 'AAAAAAAAAAAACf/EABQQAQAAAAAAAAAAAAAAAAAAAAD/2gAIAQEAAD8AKp//2Q==',
+    'base64',
+  );
+
+  async function writeArchive(name, entries) {
+    // Required lazily: the suite calls jest.resetModules() in beforeAll, and
+    // archiver's readable-stream copy does not survive being split across the
+    // two module registries.
+    const archiver = require('archiver');
+    const archivePath = path.join(storagePath, 'archives', name);
+    await new Promise((resolve, reject) => {
+      const output = fs.createWriteStream(archivePath);
+      const zip = archiver('zip', { zlib: { level: 0 } });
+      output.on('close', resolve);
+      zip.on('error', reject);
+      zip.pipe(output);
+      for (const [entryName, buffer] of Object.entries(entries)) {
+        zip.append(buffer, { name: entryName });
+      }
+      zip.finalize();
+    });
+    return path.join('archives', name);
+  }
+
+  async function seedArchivedEvent(archiveRelPath, slug) {
+    const [row] = await db('events').insert({
+      slug,
+      event_type: 'wedding',
+      event_name: slug,
+      event_date: '2026-06-27',
+      host_email: '[email protected]',
+      admin_email: '[email protected]',
+      password_hash: 'x',
+      share_link: `${slug}-share`,
+      expires_at: new Date().toISOString(),
+      is_archived: 1,  // sqlite stores booleans as 0/1, see utils/dbCompat
+      archive_path: archiveRelPath,
+    }).returning('id');
+    return typeof row === 'object' ? row.id : row;
+  }
+
+  const categoryOf = async (filename) => {
+    const photo = await db('photos').where('filename', filename).first();
+    if (!photo || !photo.category_id) return null;
+    const category = await db('photo_categories').where('id', photo.category_id).first();
+    return category ? category.name : null;
+  };
+
+  it('takes the category from the manifest when the archive is flat', async () => {
+    // Exactly the shape a gallery-root event archives to: no directories.
+    const manifest = JSON.stringify([
+      { filename: 'a.jpg', original_filename: 'DSC_0001.jpg', category_name: 'Polterabend' },
+      { filename: 'b.jpg', original_filename: 'DSC_0002.jpg', category_name: 'Ceremony' },
+    ]);
+    const archiveRelPath = await writeArchive('flat.zip', {
+      'a.jpg': PIXEL,
+      'b.jpg': PIXEL,
+      'photos_manifest.json': Buffer.from(manifest, 'utf8'),
+    });
+    const eventId = await seedArchivedEvent(archiveRelPath, 'flat-event');
+
+    const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({});
+    expect(res.status).toBe(200);
+
+    // The whole bug: both of these used to be null.
+    expect(await categoryOf('a.jpg')).toBe('Polterabend');
+    expect(await categoryOf('b.jpg')).toBe('Ceremony');
+  });
+
+  it('reuses an existing category row instead of creating a duplicate', async () => {
+    const archiveRelPath = await writeArchive('reuse.zip', {
+      'c.jpg': PIXEL,
+      'photos_manifest.json': Buffer.from(JSON.stringify([
+        { filename: 'c.jpg', original_filename: 'DSC_0003.jpg', category_name: 'Party' },
+      ]), 'utf8'),
+    });
+    const eventId = await seedArchivedEvent(archiveRelPath, 'reuse-event');
+    await db('photo_categories').insert({
+      event_id: eventId, name: 'Party', slug: 'party', created_at: new Date(),
+    });
+
+    const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({});
+    expect(res.status).toBe(200);
+
+    expect(await categoryOf('c.jpg')).toBe('Party');
+    const rows = await db('photo_categories').where({ event_id: eventId, name: 'Party' });
+    expect(rows).toHaveLength(1);
+  });
+
+  it('still falls back to the directory for legacy archives with no manifest', async () => {
+    // No manifest at all — the shape every archive had before the manifest
+    // landed. The directory is the only signal left, and it must keep working.
+    //
+    // `individual/` is what a REAL archive contains: entry names are the
+    // storage key minus `events/active/{slug}`, and that layout is
+    // `individual/` / `collages/`. Categories have never been directories, so
+    // the fallback invents a category with that name — not useful, but better
+    // than losing every category, and this pins what actually happens rather
+    // than a category-shaped folder no archive produces.
+    const archiveRelPath = await writeArchive('foldered.zip', {
+      'individual/d.jpg': PIXEL,
+    });
+    const eventId = await seedArchivedEvent(archiveRelPath, 'foldered-event');
+
+    const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({});
+    expect(res.status).toBe(200);
+
+    expect(await categoryOf('d.jpg')).toBe('individual');
+  });
+
+  it('reuses a GLOBAL category instead of cloning it into the event', async () => {
+    // Seeded categories (Ceremony, Reception, ...) have event_id NULL. An
+    // event-only lookup misses them, so the restore used to create a second
+    // "Ceremony" — and because is_global defaults to TRUE, that duplicate then
+    // appeared in every other event's category list.
+    const [g] = await db('photo_categories').insert({
+      event_id: null, name: 'Ceremony', slug: 'ceremony', is_global: true, created_at: new Date(),
+    }).returning('id');
+    const globalId = typeof g === 'object' ? g.id : g;
+
+    const archiveRelPath = await writeArchive('global.zip', {
+      'individual/gl.jpg': PIXEL,
+      'photos_manifest.json': Buffer.from(JSON.stringify([
+        { filename: 'gl.jpg', original_filename: 'DSC_1.jpg', category_name: 'Ceremony' },
+      ]), 'utf8'),
+    });
+    const eventId = await seedArchivedEvent(archiveRelPath, 'global-event');
+
+    const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({});
+    expect(res.status).toBe(200);
+
+    const photo = await db('photos').where('filename', 'gl.jpg').first();
+    expect(photo.category_id).toBe(globalId);
+    // No clone, global or otherwise.
+    const all = await db('photo_categories').where('name', 'Ceremony');
+    expect(all).toHaveLength(1);
+  });
+
+  it('does not create a GLOBAL category when it has to invent one', async () => {
+    // is_global defaults to true on this column, so an unqualified insert would
+    // leak a restore's category name into every gallery on the instance.
+    const archiveRelPath = await writeArchive('newcat.zip', {
+      'individual/nc.jpg': PIXEL,
+      'photos_manifest.json': Buffer.from(JSON.stringify([
+        { filename: 'nc.jpg', original_filename: 'DSC_2.jpg', category_name: 'Polterabend' },
+      ]), 'utf8'),
+    });
+    const eventId = await seedArchivedEvent(archiveRelPath, 'newcat-event');
+
+    const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({});
+    expect(res.status).toBe(200);
+
+    const created = await db('photo_categories').where('name', 'Polterabend').first();
+    expect(created.event_id).toBe(eventId);
+    expect(created.is_global === false || created.is_global === 0).toBe(true);
+  });
+
+  it('matches the manifest when the ZIP was written with original filenames', async () => {
+    // With general_use_original_filenames_for_downloads on at archive time,
+    // archiveService names entries after the ORIGINAL filename while the
+    // manifest stays keyed by photos.filename. Looking up the extracted
+    // basename missed every entry, so categories were lost on exactly those
+    // archives.
+    const archiveRelPath = await writeArchive('original-names.zip', {
+      'individual/DSC_4242.jpg': PIXEL,
+      'photos_manifest.json': Buffer.from(JSON.stringify([
+        { filename: 'stored_9f8e7d.jpg', original_filename: 'DSC_4242.jpg', category_name: 'Drohne' },
+      ]), 'utf8'),
+    });
+    const eventId = await seedArchivedEvent(archiveRelPath, 'original-names-event');
+
+    const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({});
+    expect(res.status).toBe(200);
+
+    expect(await categoryOf('DSC_4242.jpg')).toBe('Drohne');
+  });
+
+  it('prefers the event-scoped category when a global shares its name', async () => {
+    // The category API permits both. A single OR-lookup with .first() returned
+    // whichever the engine chose, so a photo could be reassigned to the global
+    // row and lose event-local settings such as allow_downloads.
+    const archiveRelPath = await writeArchive('collide.zip', {
+      'individual/co.jpg': PIXEL,
+      'photos_manifest.json': Buffer.from(JSON.stringify([
+        { filename: 'co.jpg', original_filename: 'DSC_3.jpg', category_name: 'Reception' },
+      ]), 'utf8'),
+    });
+    const eventId = await seedArchivedEvent(archiveRelPath, 'collide-event');
+
+    await db('photo_categories').insert({
+      event_id: null, name: 'Reception', slug: 'reception-global', is_global: true, created_at: new Date(),
+    });
+    const [own] = await db('photo_categories').insert({
+      event_id: eventId, name: 'Reception', slug: 'reception-own', is_global: false, created_at: new Date(),
+    }).returning('id');
+    const ownId = typeof own === 'object' ? own.id : own;
+
+    const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({});
+    expect(res.status).toBe(200);
+
+    const photo = await db('photos').where('filename', 'co.jpg').first();
+    expect(photo.category_id).toBe(ownId);
+  });
+
+  it('matches a sanitized original filename, as the ZIP would have written it', async () => {
+    // archiveService runs original names through sanitizeForZipEntry() before
+    // writing the entry, so the emitted name differs from the manifest column.
+    const archiveRelPath = await writeArchive('sanitized.zip', {
+      'individual/od_dr_DSC_5.jpg': PIXEL,
+      'photos_manifest.json': Buffer.from(JSON.stringify([
+        { filename: 'stored_abc.jpg', original_filename: 'od/dr/DSC_5.jpg', category_name: 'Strand' },
+      ]), 'utf8'),
+    });
+    const eventId = await seedArchivedEvent(archiveRelPath, 'sanitized-event');
+
+    const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({});
+    expect(res.status).toBe(200);
+
+    expect(await categoryOf('od_dr_DSC_5.jpg')).toBe('Strand');
+  });
+
+  it('ignores a legacy event-owned row when falling back to globals', async () => {
+    // The bug fixed here left rows behind on upgraded instances: event-owned
+    // AND is_global true, because the column defaults true. Matching on the
+    // flag alone would let one event's leftover be adopted by another event's
+    // restore, tying photos to a category that vanishes with someone else's
+    // gallery.
+    const otherEventId = await seedArchivedEvent('archives/none.zip', 'legacy-owner-event');
+    await db('photo_categories').insert({
+      event_id: otherEventId, name: 'Sunset', slug: 'sunset-legacy',
+      is_global: true, created_at: new Date(),
+    });
+
+    const archiveRelPath = await writeArchive('legacy-global.zip', {
+      'individual/lg.jpg': PIXEL,
+      'photos_manifest.json': Buffer.from(JSON.stringify([
+        { filename: 'lg.jpg', original_filename: 'DSC_6.jpg', category_name: 'Sunset' },
+      ]), 'utf8'),
+    });
+    const eventId = await seedArchivedEvent(archiveRelPath, 'legacy-global-event');
+
+    const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({});
+    expect(res.status).toBe(200);
+
+    const photo = await db('photos').where('filename', 'lg.jpg').first();
+    const cat = await db('photo_categories').where('id', photo.category_id).first();
+    // Its own row, not the other event's leftover.
+    expect(cat.event_id).toBe(eventId);
+  });
+
+  it('drops an ambiguous original-name alias rather than guessing', async () => {
+    // Two photos in different ZIP folders can share an original basename;
+    // archiveService treats the paths as distinct and suffixes neither. Both
+    // would collapse onto one alias, and whichever won would hand the other
+    // photo someone else's category.
+    const archiveRelPath = await writeArchive('ambiguous.zip', {
+      'individual/SHARED.jpg': PIXEL,
+      'photos_manifest.json': Buffer.from(JSON.stringify([
+        { filename: 'a_stored.jpg', original_filename: 'SHARED.jpg', category_name: 'Alpha' },
+        { filename: 'b_stored.jpg', original_filename: 'SHARED.jpg', category_name: 'Beta' },
+      ]), 'utf8'),
+    });
+    const eventId = await seedArchivedEvent(archiveRelPath, 'ambiguous-event');
+
+    const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({});
+    expect(res.status).toBe(200);
+
+    // Falls back to the directory rather than picking Alpha or Beta at random.
+    expect(await categoryOf('SHARED.jpg')).toBe('individual');
+    for (const name of ['Alpha', 'Beta']) {
+      expect(await db('photo_categories').where({ event_id: eventId, name }).first()).toBeFalsy();
+    }
+  });
+
+  it('honours a manifest that says UNCATEGORIZED, instead of inventing one from the directory', async () => {
+    // The case the manifest-first change was for. A real archive puts every
+    // photo under `individual/`, so a photo the manifest records as having no
+    // category used to come back filed under a category called "individual" —
+    // the manifest being authoritative for "category X" but not for "none".
+    const manifest = JSON.stringify([
+      { filename: 'u.jpg', original_filename: 'DSC_7000.jpg', category_name: null },
+    ]);
+    const archiveRelPath = await writeArchive('uncategorized.zip', {
+      'individual/u.jpg': PIXEL,
+      'photos_manifest.json': Buffer.from(manifest, 'utf8'),
+    });
+    const eventId = await seedArchivedEvent(archiveRelPath, 'uncategorized-event');
+
+    const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({});
+    expect(res.status).toBe(200);
+
+    expect(await categoryOf('u.jpg')).toBeNull();
+    // And no junk category row was created as a side effect.
+    const rows = await db('photo_categories').where({ event_id: eventId });
+    expect(rows).toHaveLength(0);
+  });
+
+  it('drops a canonical filename that two photos claim, rather than guessing', async () => {
+    // photos.filename is not unique within an event: s3AutoImporter takes
+    // path.basename(entry.key) and dedupes by path, so two imported files in
+    // different subfolders both land as IMG_1234.jpg. Both ZIP entries reduce
+    // to the same basename at restore, so keeping the last row seen would give
+    // one photo the other's category.
+    const archiveRelPath = await writeArchive('dup-canonical.zip', {
+      'individual/IMG_1234.jpg': PIXEL,
+      'photos_manifest.json': Buffer.from(JSON.stringify([
+        { filename: 'IMG_1234.jpg', original_filename: 'a.jpg', category_name: 'Alpha' },
+        { filename: 'IMG_1234.jpg', original_filename: 'b.jpg', category_name: 'Beta' },
+      ]), 'utf8'),
+    });
+    const eventId = await seedArchivedEvent(archiveRelPath, 'dup-canonical-event');
+
+    const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({});
+    expect(res.status).toBe(200);
+
+    expect(await categoryOf('IMG_1234.jpg')).toBe('individual');
+    for (const name of ['Alpha', 'Beta']) {
+      expect(await db('photo_categories').where({ event_id: eventId, name }).first()).toBeFalsy();
+    }
+  });
+
+  it("drops a name that one row owns canonically and another claims as an alias", async () => {
+    // Undecidable: with original-filename archiving ON the ZIP entry under
+    // this name is the ALIAS owner's file, with it OFF it is the canonical
+    // owner's, and the manifest does not record which mode was used. The
+    // point of the two-pass split is that this now resolves the same way
+    // every run — the archive query has no ORDER BY, so it used to be a coin
+    // flip between dropping the name and overwriting it.
+    const archiveRelPath = await writeArchive('alias-vs-canonical.zip', {
+      'individual/CANON.jpg': PIXEL,
+      'photos_manifest.json': Buffer.from(JSON.stringify([
+        { filename: 'CANON.jpg', original_filename: 'unrelated.jpg', category_name: 'Canonical' },
+        { filename: 'other_stored.jpg', original_filename: 'CANON.jpg', category_name: 'Aliased' },
+      ]), 'utf8'),
+    });
+    const eventId = await seedArchivedEvent(archiveRelPath, 'alias-vs-canonical-event');
+
+    const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({});
+    expect(res.status).toBe(200);
+
+    // Falls back to the directory rather than guessing either row.
+    expect(await categoryOf('CANON.jpg')).toBe('individual');
+    for (const name of ['Canonical', 'Aliased']) {
+      expect(await db('photo_categories').where({ event_id: eventId, name }).first()).toBeFalsy();
+    }
+  });
+
+  it('picks the lowest id and warns when two categories share a name', async () => {
+    // Allowed: two event-scoped categories with the same display name and
+    // different slugs. .first() used to pick either, so a re-run could move
+    // photos between them and inherit the wrong allow_downloads.
+    const archiveRelPath = await writeArchive('dupe-category.zip', {
+      'individual/DUPE.jpg': PIXEL,
+      'photos_manifest.json': Buffer.from(JSON.stringify([
+        { filename: 'DUPE.jpg', original_filename: 'DUPE.jpg', category_name: 'Ceremony' },
+      ]), 'utf8'),
+    });
+    const eventId = await seedArchivedEvent(archiveRelPath, 'dupe-category-event');
+
+    const [first] = await db('photo_categories').insert({
+      name: 'Ceremony', slug: 'ceremony-a', is_global: 0, event_id: eventId,
+    }).returning('id');
+    await db('photo_categories').insert({
+      name: 'Ceremony', slug: 'ceremony-b', is_global: 0, event_id: eventId,
+    });
+    const firstId = typeof first === 'object' ? first.id : first;
+
+    const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({});
+    expect(res.status).toBe(200);
+
+    // Stable, not arbitrary: the same run twice lands on the same row.
+    const photo = await db('photos').where({ event_id: eventId, filename: 'DUPE.jpg' }).first();
+    expect(photo.category_id).toBe(firstId);
+    // And no third "Ceremony" was invented.
+    expect((await db('photo_categories').where({ event_id: eventId, name: 'Ceremony' })).length)
+      .toBe(2);
+  });
+
+  it('does not invent a category for a photo row that already exists', async () => {
+    // archiveEvent retains photo rows, so a restore can skip every insert.
+    // Resolving categories before that check created one from the stale
+    // manifest name that nothing then used — renaming a category while its
+    // event was archived left the old name behind as an empty duplicate.
+    const archiveRelPath = await writeArchive('existing-rows.zip', {
+      'individual/KEPT.jpg': PIXEL,
+      'photos_manifest.json': Buffer.from(JSON.stringify([
+        { filename: 'KEPT.jpg', original_filename: 'KEPT.jpg', category_name: 'OldName' },
+      ]), 'utf8'),
+    });
+    const eventId = await seedArchivedEvent(archiveRelPath, 'existing-rows-event');
+    await db('photos').insert({
+      event_id: eventId, filename: 'KEPT.jpg', path: 'whatever/KEPT.jpg', type: 'jpg',
+      uploaded_at: new Date().toISOString(),
+    });
+
+    const res = await request(app).post(`/admin/archives/${eventId}/restore`).send({});
+    expect(res.status).toBe(200);
+
+    expect(await db('photo_categories').where({ event_id: eventId, name: 'OldName' }).first())
+      .toBeFalsy();
+  });
+
+});

backend/src/routes/adminArchives.js

@@ -10,6 +10,7 @@ const archiver = require('archiver');
 const StreamZip = require('node-stream-zip');
 const { requireEventOwnership } = require('../middleware/ownership');
 const { assertZipEntriesWithin } = require('../utils/safePath');
+const { sanitizeForZipEntry } = require('../utils/filenameSanitizer');
 const logger = require('../utils/logger');
 const { getPagination } = require('../utils/routeHelpers');
 const router = express.Router();
@@ -204,15 +205,111 @@ router.post('/:id/restore', adminAuth, requirePermission('archives.restore'), re
       // manifest the archive process writes. Older archives have no manifest;
       // we fall back to filename for those.
       const manifestByFilename = new Map();
+      // Aliases that more than one manifest row claims — see the loop below.
+      const ambiguousAliases = new Set();
       try {
         const manifestRaw = await fs.readFile(
           path.join(eventDir, 'photos_manifest.json'), 'utf8',
         );
         const parsed = JSON.parse(manifestRaw);
         if (Array.isArray(parsed)) {
-          for (const m of parsed) {
+          // Two passes, and the order is the point. Canonical photos.filename
-            if (m && m.filename) manifestByFilename.set(m.filename, m);
+          // keys are claimed first and never yielded afterwards; aliases only
+          // fill names no canonical row wanted. Interleaving them made the
+          // result depend on manifest iteration order — the query has no
+          // ORDER BY — and could delete a canonical key because some OTHER
+          // row's original_filename happened to collide with it.
+          const rows = parsed.filter((m) => m && m.filename);
+
+          // photos.filename is not unique within an event: s3AutoImporter
+          // takes path.basename(entry.key) and dedupes by path, so two
+          // imported files in different subfolders both land as `IMG_1234.jpg`
+          // with different `path` values. At restore both ZIP entries reduce
+          // to the same basename, so whichever row won the key would hand the
+          // other photo someone else's category. Contested names are dropped
+          // rather than guessed.
+          const contestedFilenames = new Set();
+          for (const m of rows) {
+            const held = manifestByFilename.get(m.filename);
+            if (held && held !== m) {
+              contestedFilenames.add(m.filename);
+              continue;
+            }
+            manifestByFilename.set(m.filename, m);
           }
+          for (const name of contestedFilenames) manifestByFilename.delete(name);
+          if (contestedFilenames.size) {
+            logger.warn(
+              `Photos manifest: ${contestedFilenames.size} filename(s) claimed by more than one photo; `
+              + 'those fall back to the directory for their category.'
+            );
+          }
+
+          // Every canonical name, contested ones included — an alias must not
+          // claim a name that a canonical row wanted and lost, either.
+          const canonicalNames = new Set(rows.map((m) => m.filename));
+
+          for (const m of rows) {
+            // Also index by original_filename. When
+            // general_use_original_filenames_for_downloads was on at archive
+            // time, archiveService names each ZIP entry after the ORIGINAL
+            // filename, while the manifest stays keyed by the internal
+            // photos.filename — so a lookup by the extracted basename misses
+            // every entry and the restore silently loses categories on exactly
+            // those archives. Never overwrite a real filename key: that one is
+            // authoritative if both happen to collide.
+            // Index the name as the ZIP would have EMITTED it, not the raw
+            // column: archiveService runs original names through
+            // sanitizeForZipEntry() before writing the entry, so an original
+            // with a slash or a control byte lands under a different name than
+            // the manifest records. Index both, so either spelling resolves.
+            //
+            // Still not total: uniquifyZipNames() appends `_1` when two photos
+            // in one event share an original name, and that suffix cannot be
+            // reconstructed from the manifest. Those few fall through to the
+            // directory, exactly as they did before this fix — no worse, just
+            // not better. Closing that needs the emitted name recorded at
+            // archive time, which is a writer change and a new archive format.
+            for (const alias of [m.original_filename, sanitizeForZipEntry(m.original_filename)]) {
+              if (!alias) continue;
+              // An alias colliding with someone else's canonical name is
+              // genuinely undecidable, so it is dropped rather than resolved
+              // either way. Which photo the ZIP emitted under that name
+              // depends on whether original-filename archiving was on at
+              // archive time, and the manifest does not record that: with it
+              // ON the entry is the ALIAS owner's file, with it OFF it is the
+              // canonical owner's. Preferring either one silently mislabels
+              // the other half of the time.
+              //
+              // What the two-pass split buys is that this is now decided the
+              // same way every run — the archive query has no ORDER BY, so
+              // interleaving the passes previously made it a coin flip
+              // between dropping the name and overwriting it.
+              if (canonicalNames.has(alias)) {
+                if (manifestByFilename.get(alias) !== m) ambiguousAliases.add(alias);
+                continue;
+              }
+              if (manifestByFilename.has(alias)) {
+                // Two rows want the same alias — e.g. `individual/IMG.jpg` and
+                // `collages/IMG.jpg`, which archiveService treats as distinct
+                // paths and does not suffix, but which collapse to one basename
+                // here. Whichever won would give the other photo someone else's
+                // category. Drop the alias so both fall through to the
+                // directory instead: an unresolved category is recoverable, a
+                // confidently wrong one is not.
+                if (manifestByFilename.get(alias) !== m) ambiguousAliases.add(alias);
+                continue;
+              }
+              manifestByFilename.set(alias, m);
+            }
+          }
+        }
+        for (const alias of ambiguousAliases) manifestByFilename.delete(alias);
+        if (ambiguousAliases.size) {
+          logger.warn(
+            `Photos manifest: ${ambiguousAliases.size} original-filename alias(es) claimed by more than one `
+            + 'photo; those fall back to the directory for their category.'
+          );
         }
         logger.info(`Loaded photos manifest: ${manifestByFilename.size} entries`);
       } catch (e) {
@@ -226,9 +323,84 @@ router.post('/:id/restore', adminAuth, requirePermission('archives.restore'), re
       // Get list of extracted files to update database
       const extractedPhotos = [];
 
-      // First, collect all category information from the ZIP structure
+      // Category name -> id, resolved once per name for the whole restore.
       const categoriesMap = new Map();
 
+      // Find-or-create the category by name, among the ones this event can see.
+      const resolveCategoryId = async (categoryName) => {
+        if (!categoryName) return null;
+        if (categoriesMap.has(categoryName)) return categoriesMap.get(categoryName);
+
+        // Globals count as existing. A photo filed under the seeded "Ceremony"
+        // has event_id NULL on its category row, so an event-only lookup misses
+        // it and creates a second "Ceremony" — and since is_global defaults to
+        // TRUE, that duplicate then shows up in every other event's category
+        // list. Same visibility rule the photo routes use: own rows or global.
+        // Two queries, not one with an OR: an event-scoped category and a
+        // global one may share a name, and a single .first() would return
+        // whichever the engine felt like — silently reassigning a photo to the
+        // global row and losing event-local settings like allow_downloads.
+        // The event's own row is the more specific answer, so it wins.
+        //
+        // The global arm requires event_id IS NULL, not just is_global. The
+        // bug fixed here left legacy rows behind on upgraded instances —
+        // event-owned AND is_global true, because the column defaults true —
+        // and matching on the flag alone would let one event's leftover row be
+        // adopted by another event's restore, tying photos to a category that
+        // vanishes with someone else's gallery.
+        // Two event-scoped categories CAN share a display name when their
+        // slugs differ, and .first() would then pick one arbitrarily — both
+        // manifest names collapse onto a single id and half the photos
+        // inherit the wrong per-category settings (allow_downloads above all).
+        // Resolving that properly needs a stable category identifier in the
+        // manifest, which is a writer change and an archive-format bump, and
+        // could not help any archive already written. So: surface it instead
+        // of fixing it blind. If this never fires in real logs, the format
+        // change is not worth making; if it does, this is the evidence for it.
+        const ownRows = await db('photo_categories')
+          .where({ event_id: archive.id, name: categoryName })
+          .select('id');
+        if (ownRows.length > 1) {
+          logger.warn(
+            `Photos manifest: category name "${categoryName}" matches ${ownRows.length} rows in event `
+            + `${archive.id}; picking the lowest id. Photos from the other row(s) will inherit its settings.`
+          );
+        }
+
+        const existingCategory =
+          // Lowest id, not engine order — an arbitrary-but-stable choice beats
+          // a nondeterministic one, so a re-run lands the same way.
+          (ownRows.length
+            ? await db('photo_categories')
+              .where('id', Math.min(...ownRows.map((r) => r.id)))
+              .first()
+            : null)
+          || await db('photo_categories')
+            .where('name', categoryName)
+            .whereNull('event_id')
+            .where('is_global', formatBoolean(true))
+            .first();
+
+        if (existingCategory) {
+          categoriesMap.set(categoryName, existingCategory.id);
+        } else {
+          const insertResult = await db('photo_categories').insert({
+            event_id: archive.id,
+            name: categoryName,
+            slug: slugify(categoryName),
+            // Explicit: the column defaults to true, and a restore inventing a
+            // GLOBAL category would leak this event's naming into every other
+            // gallery. Anything created here belongs to this event alone.
+            is_global: formatBoolean(false),
+            created_at: new Date()
+          }).returning('id');
+
+          categoriesMap.set(categoryName, insertResult[0]?.id || insertResult[0]);
+        }
+
+        return categoriesMap.get(categoryName);
+      };
+
       for (const entry of entries) {
         if (!entry.isDirectory && entry.name.match(/\.(jpg|jpeg|png|gif|webp)$/i)) {
           const filename = path.basename(entry.name);
@@ -239,38 +411,25 @@ router.post('/:id/restore', adminAuth, requirePermission('archives.restore'), re
             // Check if file was extracted successfully
             const stats = await fs.stat(actualFilePath);
             
-            // Determine category from directory structure
+            const manifestEntry = manifestByFilename.get(filename);
-            let categoryId = null;
-            if (dirPath && dirPath !== '.') {
-              // Get the first level directory as category
-              const categoryName = dirPath.split(path.sep)[0];
-              
-              if (!categoriesMap.has(categoryName)) {
-                // Check if this category exists in the database
-                const existingCategory = await db('photo_categories')
-                  .where('event_id', archive.id)
-                  .where('name', categoryName)
-                  .first();
-                
-                if (existingCategory) {
-                  categoriesMap.set(categoryName, existingCategory.id);
-                } else {
-                  // Create the category if it doesn't exist
-                  const insertResult = await db('photo_categories').insert({
-                    event_id: archive.id,
-                    name: categoryName,
-                    slug: slugify(categoryName),
-                    created_at: new Date()
-                  }).returning('id');
-                  
-                  const newCategoryId = insertResult[0]?.id || insertResult[0];
-                  categoriesMap.set(categoryName, newCategoryId);
-                }
-              }
-              
-              categoryId = categoriesMap.get(categoryName);
-            }
 
+            // The manifest is the only faithful source for the category, and
+            // it is authoritative INCLUDING when it says "none". A manifest
+            // entry with a null category_name means the photo was genuinely
+            // uncategorized, so falling through to the directory would
+            // contradict the very record being restored from.
+            //
+            // That matters because the directory is not a category. Archive
+            // entry names are the storage key minus `events/active/{slug}`,
+            // and that layout is `individual/{filename}` / `collages/…` —
+            // categories have never been directories there. Reading the first
+            // path segment on a real archive therefore invents categories
+            // literally named "individual" and "collages".
+            //
+            // So the fallback is confined to photos with NO manifest entry at
+            // all: archives written before the manifest existed, where the
+            // directory is the only signal left and inventing those two names
+            // is still better than losing every category.
             // Check if photo already exists in database
             const existingPhoto = await db('photos')
               .where('event_id', archive.id)
@@ -278,9 +437,21 @@ router.post('/:id/restore', adminAuth, requirePermission('archives.restore'), re
               .first();
 
             if (!existingPhoto) {
+              // Resolved HERE, not above: resolveCategoryId find-or-CREATES,
+              // and archiveEvent retains photo rows. Resolving before this
+              // check meant restoring an archive whose rows still exist
+              // created a category from the stale manifest name that nothing
+              // then used — so renaming a category while its event was
+              // archived left the old name behind as an empty duplicate.
+              let categoryId = null;
+              if (manifestEntry) {
+                categoryId = await resolveCategoryId(manifestEntry.category_name);
+              } else if (dirPath && dirPath !== '.') {
+                categoryId = await resolveCategoryId(dirPath.split(path.sep)[0]);
+              }
+
               // Store relative path from storage root
               const relativePath = path.relative(storagePath, actualFilePath);
-              const manifestEntry = manifestByFilename.get(filename);
               extractedPhotos.push({
                 event_id: archive.id,
                 filename: filename,