fix(external-media): record capture dates on import, and backfill existing libraries (#1172) (#1179)

* fix(external-media): record capture dates on import, and backfill existing libraries (#1172)

External imports never read EXIF, so photos.captured_at stayed NULL for every
row they created. The gallery sorts "Date Taken" with
COALESCE(captured_at, uploaded_at), which on a bulk import is the import
timestamp — so the sort silently degraded into "order by import batch" with no
error and nothing in the UI to say the sort key was missing. The reporter's
12-day trip came back with its first two days at positions 4204-5296 of 5555,
because those folders happened to be imported second.

- the import reads the capture date next to the sharp().metadata() call that
  already opens the file, so this costs one more read of the same source rather
  than a second pass over the mount. Best-effort like the dimensions: a source
  without EXIF imports with captured_at NULL, as before.
- POST /api/admin/photos/repair-capture-dates backfills existing libraries,
  modelled on the dimension repair beside it — background pass, in-flight
  guard, status endpoint, and resolvePhotoFilePath, which is what reaches an
  external row at all. Not a migration: the originals sit on a mount that may
  be down at upgrade time, reading 8000+ of them would block the boot, and a
  run that found nothing has to be repeatable.
- "no EXIF date" is counted separately from "could not read the file". An
  operator needs to tell "these files carry no date" from "the mount is
  broken" before deciding to re-run.
- the update is guarded whereNull, so an import finishing mid-run is not
  overwritten by a slower pass.
- every sort branch now carries photos.id as a tiebreaker, not just
  capture_date. A bulk import writes hundreds of rows inside one second, so
  uploaded_at and the COALESCE fallback both collapse and the grid reshuffles
  between loads. id is insertion order, which makes the fallback meaningful.

Not addressed: extractCaptureDate reads no OffsetTimeOriginal, and exifr
resolves a naive EXIF timestamp against the HOST timezone — so captured_at is
not a true instant, and the same file imported on two machines yields two
values. That predates this and applies to managed uploads equally; the tests
here deliberately assert ordering rather than an absolute instant so they do
not encode the bug. Worth its own issue.

* fix(capture-dates): read managed originals through storage, skip archived, claim the run flag (#1172)

Four holes in the backfill endpoint, all found in review:

- Managed photos were resolved with resolvePhotoFilePath, which builds a
  STORAGE_PATH filesystem path. On an S3 install nothing is there, so every
  managed row failed. Now split the way the thumbnail regenerator does:
  external rows read from the mount directly, managed rows go through
  resolvePhotoStorageKey + withLocalCopy.
- Archived events keep their photos rows but their originals are deleted on
  archive, so those rows failed every run and kept the button lit forever.
  Excluded from both the job and the status counts.
- isRunning was claimed after the candidate query, so two concurrent POSTs
  could both pass the guard and start a pass. Claimed before the await, with
  every early exit releasing it.
- The noExif comment promised a distinction extractCaptureDate does not make
  (it returns null for unreadable files too). Reworded to what it is.

* chore: drop a stray node_modules symlink committed by mistake

The .gitignore pattern is `node_modules/`, which matches a directory and
not a symlink of the same name, so a local convenience link slipped past it.
It pointed at an absolute path on one machine and would dangle everywhere
else, breaking `cd backend && npm install`.

* fix(capture-dates): gate the backfill as system maintenance, stop overstating the counters (#1172)

The endpoint walks every event in the install and rewrites their metadata,
but required only photos.edit — which the built-in team_photographer preset
holds (175_granular_permissions_and_presets.js:106). That role exists for a
contributing shooter, who should not be able to start a whole-library S3/NAS
scan or touch another owner's photos. Now system.manage, with the status
endpoint on system.view so the panel simply stays hidden for everyone else.

The "without EXIF date" wording also promised a distinction the code does not
draw: extractCaptureDate returns null for an unparseable file as well as for
one that genuinely carries no date, so both land in that bucket. Reworded to
"no date found" / "unreachable" in en, de and fr, which is what the two
numbers actually separate.

* docs: point the permission note at the follow-up PR (#1172)

The dimension repair's matching gate landed in #1182, so the comment no
longer needs to describe it as unaddressed.

* fix(i18n): align the Slovenian capture-date wording with the other locales (#1172)

sl was missed when the counters were reworded from 'without EXIF date' /
'unreadable' to what they actually measure.

* fix(capture-dates): gate the status card on the permission the button needs (#1172)

system.view and system.manage are independent grants, and StatusTab has no
permission gate of its own — a successful status payload is what renders the
card and its enabled button (StatusTab.tsx:637). Gating the status endpoint on
system.view therefore handed a system.view-only role a live Backfill button
whose every click 403s, with no error surfaced by the mutation.

The comment above it already claimed this endpoint matched the POST. Now it
does.

* fix(gallery): make the Date Taken sort correct on SQLite (#1172)

photos.captured_at does not hold one type on SQLite. Three writers put three
different things in it:

  integer  managed uploads — photoProcessor.js:488 hands knex a Date, which the
           sqlite3 binding stores as epoch milliseconds
  text     external imports and the backfill, which write ISO-8601
  null     no capture date, so the sort falls through to uploaded_at, itself
           text in knex's 'YYYY-MM-DD HH:MM:SS' default shape

A plain COALESCE over that is not an ordering. SQLite sorts INTEGER before TEXT
unconditionally, so every managed photo carrying EXIF came back ahead of every
photo that did not, whatever the dates said — a 2027 capture landing before a
2020 one. Among the text values 'T' (0x54) also outranks the space (0x20), so a
same-day ISO 01:15 sorted behind a fallback 23:00.

Both failures predate this branch — the first needs only two managed photos —
but making that sort correct is what #1172 is about, so it is fixed here rather
than left for the issue it belongs to.

Normalised in the ORDER BY rather than by rewriting the column: the data fix
would have to touch every existing row and every writer, which is a far heavier
change than the sort it corrects. The cost is that this sort no longer uses
idx_photos_captured_at on SQLite — an acceptable trade on the fallback engine,
where the alternative is an index-assisted wrong answer. Postgres is untouched:
captured_at is a real timestamp there and COALESCE already compares correctly.

The regression tests drive the real gallery route on real SQLite. They write
the epoch-millisecond integer directly, because the Date that produces it in
production cannot be reproduced inside jest — there the binding's type dispatch
misses sandbox Dates and stores "[object Object]" (CLAUDE.md). All four
behavioural tests fail on the unfixed ORDER BY; verified by reverting it.

* fix(gallery): normalise epoch-integer uploaded_at too, and stop polling a 403 (#1172)

Two follow-ups from review.

uploaded_at is not always text on SQLite either. A legacy archive restore
leaves epoch milliseconds in it — there is a test pinning exactly that
(__tests__/integration/sqliteEpochTimestamps.test.js) — and the fallback branch
read it with substr(), so '1830297600000' was compared against
'2020-01-01 00:00:00' as text and a 2028 upload sorted first. Both columns now
get the integer/real branch.

The status card also polled every ten seconds regardless of permission. With
the endpoint correctly requiring system.manage, anyone who can open the Status
tab but cannot run the job would have had a 403 and a logged denial every ten
seconds for a panel they were never shown. The query is now gated on the same
permission the endpoint requires, so it never starts.

* style: quote convention in the capture-sort test (#1172)

* fix(capture-dates): skip watcher-imported videos, and make the status counts consistent (#1172)

Three follow-ups from review.

fileWatcher.processNewPhoto sets type='video' and a video/* mime but never
media_type (fileWatcher.js:128-130), so those rows keep the 'image' default
from migration 048. Filtering on media_type alone queued every such video on
every run — extractCaptureDate returns null for a video, captured_at stays
null, and the backlog never cleared. Candidate query and status scope now check
all three markers.

The status counts were two separate queries, so an import committing a dated
photo between them could be counted by the second and not the first: the card
then showed withCaptureDate > total and a negative backlog, with the button
enabled to "fix" it. One aggregate now.

And the card's render checked only the cached payload. TanStack keeps that
after `enabled` flips false, so a lower-privileged admin logging in behind a
system.manage user inside the cache lifetime would still have seen the card and
a button whose POST 403s. The permission is part of the render condition now.

---------

Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
2026-08-26 08:43:13 +02:00
committed by GitHub
co-authored by Paul Nothaft
parent 9426ade2d3
commit 410b8f8f6f
11 changed files with 998 additions and 7 deletions
+28 -2
View File
@@ -8,7 +8,7 @@ const { list, resolveExternalPath, getExternalMediaRoot } = require('../services
const { db, logActivity } = require('../database/db');
const sharp = require('sharp');
const logger = require('../utils/logger');
const { generateThumbnail } = require('../services/imageProcessor');
const { generateThumbnail, extractCaptureDate } = require('../services/imageProcessor');
const { isUniqueViolation } = require('../utils/dbErrors');
const router = express.Router();
@@ -208,6 +208,27 @@ router.post('/events/:id/import-external', adminAuth, requirePermission('photos.
logger.warn(`Could not extract dimensions for ${f.rel}: ${dimErr.message}`);
}
// Capture date from EXIF (#1172). Managed uploads get this from
// photoProcessor, which external media never goes through — so
// captured_at stayed NULL for every externally imported photo, and the
// gallery's "Date Taken" sort silently degraded into import order via
// its COALESCE fallback. On a library imported in two batches that put
// the first days of a trip after the last ones.
//
// Read here because the file is already open a few lines above for the
// dimensions, so this costs one more read of the same source rather
// than a second pass over the mount.
//
// Best-effort, exactly like the dimensions: a source without EXIF, or
// one Sharp/exifr cannot parse, imports with captured_at NULL and
// falls back to uploaded_at as before.
let capturedAt = null;
try {
capturedAt = await extractCaptureDate(f.full);
} catch (dateErr) {
logger.warn(`Could not extract capture date for ${f.rel}: ${dateErr.message}`);
}
let inserted;
try {
inserted = await db('photos')
@@ -222,7 +243,12 @@ router.post('/events/:id/import-external', adminAuth, requirePermission('photos.
width,
height,
source_origin: 'external',
external_relpath: relFromRoot
external_relpath: relFromRoot,
// .toISOString() rather than the Date: inside jest, Dates handed
// to the sqlite3 binding land as the literal string
// "[object Object]" (see CLAUDE.md). Strings round-trip on both
// engines.
captured_at: capturedAt ? capturedAt.toISOString() : null
})
.returning('id');
} catch (insertErr) {
+239
View File
@@ -14,6 +14,14 @@ let repairProgress = {
lastResult: null
};
// Same shape, separate state: the two jobs walk the same photos but read
// different things out of them, and one running must not block or report for
// the other.
let captureDateProgress = {
isRunning: false,
lastResult: null
};
// Repair photo dimensions (background job)
router.post('/repair-dimensions', adminAuth, requirePermission('photos.edit'), async (req, res) => {
try {
@@ -152,4 +160,235 @@ router.get('/repair-dimensions/status', adminAuth, requirePermission('photos.vie
}
});
/**
* Backfill captured_at from EXIF (#1172).
*
* External imports never read EXIF before this release, so every
* externally-imported photo carries captured_at NULL — and the gallery's
* "Date Taken" sort falls back to uploaded_at, which on a bulk import is the
* import timestamp. A 5555-photo trip came back ordered by which folder was
* imported first.
*
* Deliberately an endpoint rather than a migration: the originals live on a
* mount that may be unavailable at upgrade time, reading 8000+ of them blocks
* the boot, and a run that found nothing needs to be repeatable once the mount
* is back. Same reasoning, and the same shape, as the dimension repair above —
* including resolvePhotoFilePath, which is what makes it work for external
* rows at all (the thumbnail regenerator resolves under
* storage/events/active/<path>, which never exists for them).
*/
// system.manage, not photos.edit: this walks every event in the install and
// rewrites their metadata. photos.edit is held by the team_photographer preset
// (175_granular_permissions_and_presets.js:106), which exists precisely for a
// contributing shooter who should not be able to start a whole-library S3/NAS
// scan or touch another owner's photos. The dimension repair above had the same
// exposure and predates this; it is brought in line separately in #1182.
router.post('/repair-capture-dates', adminAuth, requirePermission('system.manage'), async (req, res) => {
try {
if (captureDateProgress.isRunning) {
return res.status(409).json({ error: 'Capture date backfill is already running' });
}
// Claimed here, not after the candidate query: that query is an await, and
// two POSTs arriving inside it would both read isRunning === false and both
// start a pass over the same rows. Every early exit below has to release it
// again, hence the try/catch around the query.
captureDateProgress.isRunning = true;
captureDateProgress.lastResult = null;
let photos;
try {
photos = await db('photos')
.join('events', 'photos.event_id', 'events.id')
.whereNull('photos.captured_at')
// Three markers, because no single one is reliable. fileWatcher's
// auto-import sets type='video' and a video/* mime but never
// media_type (fileWatcher.js:128-130), so those rows keep the 'image'
// default from migration 048 and a media_type-only filter queues them
// forever: extractCaptureDate returns null for a video, captured_at
// stays null, and every run picks it up again.
.where(function () {
this.where('photos.media_type', '!=', 'video').orWhereNull('photos.media_type');
})
.where(function () {
this.where('photos.type', '!=', 'video').orWhereNull('photos.type');
})
.where(function () {
this.whereNull('photos.mime_type').orWhere('photos.mime_type', 'not like', 'video/%');
})
// Archiving deletes the originals from storage but keeps the photos
// rows (archiveService.js:166,199). Those files are inside the zip and
// nothing here can read them, so including them would fail every row
// on every run and leave the button permanently lit.
.where(function () {
this.where('events.is_archived', false).orWhereNull('events.is_archived');
})
.select(
'photos.id', 'photos.path', 'photos.filename',
'photos.source_origin', 'photos.external_relpath', 'photos.event_id',
'events.source_mode', 'events.external_path', 'events.slug'
);
} catch (err) {
captureDateProgress.isRunning = false;
throw err;
}
if (photos.length === 0) {
captureDateProgress.isRunning = false;
return res.json({ message: 'No photos need a capture date', count: 0 });
}
res.json({
message: `Started backfilling capture dates for ${photos.length} photos`,
count: photos.length
});
setImmediate(async () => {
const { extractCaptureDate, withLocalCopy } = require('../services/imageProcessor');
const { resolvePhotoStorageKey } = require('../services/photoResolver');
let successCount = 0;
let missingCount = 0;
let errorCount = 0;
for (const photo of photos) {
try {
const event = { source_mode: photo.source_mode, external_path: photo.external_path, slug: photo.slug };
const isExternal = photo.source_origin === 'external' || photo.source_origin === 'reference';
// Two source shapes, same split the thumbnail regenerator uses
// (imageProcessor.js:391-420). External rows live on a local mount
// and are read directly; managed rows live behind the storage
// backend, which on an S3 install is not a filesystem at all — going
// through resolvePhotoFilePath there would build a STORAGE_PATH that
// holds nothing and fail every managed photo.
let captured;
if (isExternal) {
let fullPath;
try {
fullPath = resolvePhotoFilePath(event, photo);
} catch (err) {
logger.warn(`Photo ${photo.id} has no resolvable path, skipping capture date: ${err.message}`);
errorCount++;
continue;
}
try {
await fs.access(fullPath);
} catch (err) {
logger.warn(`File not found for photo ${photo.id}: ${fullPath}`);
errorCount++;
continue;
}
captured = await extractCaptureDate(fullPath);
} else {
let sourceKey;
try {
sourceKey = resolvePhotoStorageKey(event, photo);
} catch (err) {
logger.warn(`Photo ${photo.id} has no resolvable storage key, skipping capture date: ${err.message}`);
errorCount++;
continue;
}
// In local-fs mode withLocalCopy hands back the resolved path
// without checking it exists, so the access probe stays. In S3
// mode a missing object throws out of getToFile and lands in the
// outer catch — both end up counted as failures, which is what a
// missing original is.
captured = await withLocalCopy(sourceKey, async (localPath) => {
await fs.access(localPath);
return extractCaptureDate(localPath);
});
}
if (!captured) {
// No date recovered. Usually genuine — plenty of sources carry no
// EXIF — but extractCaptureDate also returns null when the file is
// unreadable as an image, so this bucket is "nothing to write",
// not "definitely has no EXIF". The failure counter above is the
// one that means the storage is broken.
missingCount++;
continue;
}
// whereNull, not a blanket set: the job can run for a long time on a
// large library, and an import or a replacement finishing meanwhile
// has already written a date this pass would otherwise overwrite
// with the same-or-worse value.
const updated = await db('photos')
.where({ id: photo.id })
.whereNull('captured_at')
.update({ captured_at: captured.toISOString() });
if (updated) successCount++;
if (successCount % 50 === 0 && successCount > 0) {
logger.info(`Capture date backfill progress: ${successCount} updated...`);
}
} catch (error) {
logger.error(`Error backfilling capture date for photo ${photo.id}:`, error);
errorCount++;
}
}
captureDateProgress.isRunning = false;
captureDateProgress.lastResult = { success: successCount, noExif: missingCount, failed: errorCount };
logger.info(`Capture date backfill complete: ${successCount} updated, ${missingCount} without EXIF, ${errorCount} errors`);
});
} catch (error) {
logger.error('Error starting capture date backfill:', error);
res.status(500).json({ error: 'Failed to start capture date backfill' });
}
});
// The same permission as the POST, not the read-only system.view. system.view
// and system.manage are independent grants, and StatusTab has no permission
// gate of its own — a successful status payload is what renders the card and
// its enabled button (StatusTab.tsx:637). Gating this on system.view therefore
// handed a system.view-only role a live button whose every click 403s with no
// error surfaced. Requiring system.manage makes the card appear only for
// someone who can actually run it, which is what this comment always claimed.
router.get('/repair-capture-dates/status', adminAuth, requirePermission('system.manage'), async (req, res) => {
try {
// Same scope as the job itself — counting archived photos here would show
// a permanent backlog the button can never clear.
const scoped = () => db('photos')
.join('events', 'photos.event_id', 'events.id')
.where(function () {
this.where('photos.media_type', '!=', 'video').orWhereNull('photos.media_type');
})
.where(function () {
this.where('photos.type', '!=', 'video').orWhereNull('photos.type');
})
.where(function () {
this.whereNull('photos.mime_type').orWhere('photos.mime_type', 'not like', 'video/%');
})
.where(function () {
this.where('events.is_archived', false).orWhereNull('events.is_archived');
});
// One query, two aggregates. As two separate counts an import committing a
// dated photo between them could be counted by the second and not the
// first, so withCaptureDate came out larger than total and the card showed
// a negative backlog — with the button enabled to "fix" it.
const counts = await scoped()
.count('photos.id as total')
.count({ dated: db.raw('CASE WHEN photos.captured_at IS NOT NULL THEN 1 END') })
.first();
const total = Number(counts.total);
const withCaptureDate = Number(counts.dated);
res.json({
total,
withCaptureDate,
withoutCaptureDate: total - withCaptureDate,
isRunning: captureDateProgress.isRunning,
lastResult: captureDateProgress.lastResult
});
} catch (error) {
logger.error('Error fetching capture date backfill status:', error);
res.status(500).json({ error: 'Failed to fetch capture date backfill status' });
}
});
module.exports = router;
+56 -5
View File
@@ -674,15 +674,66 @@ router.get('/:slug/photos', verifyGalleryAccess, resolveGuest, async (req, res)
photosQuery = photosQuery.where('photos.category_id', req.event.show_category_id);
}
// Apply sort option
// Apply sort option.
//
// Every branch carries photos.id as a tiebreaker (#1172). Without one the
// order within a tie is whatever the engine happens to return, and ties are
// the normal case rather than the exception: a bulk import writes hundreds
// of rows inside the same second, so uploaded_at collapses — and with
// captured_at NULL the COALESCE below collapses onto it too. The visible
// symptom is a grid that reshuffles between page loads. id is insertion
// order, so it also makes the fallback ordering meaningful rather than
// arbitrary.
if (sort === 'capture_date') {
// Sort by capture date, falling back to uploaded_at if capture date is null
photosQuery = photosQuery.orderByRaw('COALESCE(photos.captured_at, photos.uploaded_at) ' + sortOrder);
// Sort by capture date, falling back to uploaded_at if capture date is null.
//
// On SQLite that fallback cannot be a plain COALESCE, because the two
// columns do not hold one type. photos.captured_at ends up carrying three
// different storage classes:
//
// integer managed uploads — photoProcessor.js:488 writes a Date, which
// the sqlite3 binding stores as epoch milliseconds
// text external imports and the backfill, which write ISO-8601
// ('2026-06-03T01:15:00.000Z') per the CLAUDE.md rule that
// Dates must not be handed to the binding in tests
// null no capture date, so the sort falls through to uploaded_at —
// usually text in knex's 'YYYY-MM-DD HH:MM:SS' default shape,
// but epoch milliseconds on rows written by a legacy archive
// restore (see __tests__/integration/sqliteEpochTimestamps.js),
// so that column needs the same two branches
//
// SQLite orders INTEGER before TEXT unconditionally, so every managed
// photo carrying EXIF sorted ahead of every photo that did not, whatever
// the actual dates — a 2027 capture landing before a 2020 one. Among the
// text values the 'T' separator (0x54) also outranks the space (0x20), so
// a same-day ISO 01:15 sorted after a fallback 23:00.
//
// Normalising in the ORDER BY rather than rewriting the column: the data
// fix would have to touch every existing row and every writer, which is a
// much heavier change than the sort it is meant to correct. The cost here
// is that this sort stops using idx_photos_captured_at on SQLite — an
// acceptable trade on the fallback engine, where the alternative is an
// index-assisted wrong answer.
//
// Postgres is untouched: captured_at is a real timestamp there, so
// COALESCE already compares correctly.
if (db.client.config.client === 'pg') {
photosQuery = photosQuery
.orderByRaw('COALESCE(photos.captured_at, photos.uploaded_at) ' + sortOrder);
} else {
photosQuery = photosQuery.orderByRaw(`CASE
WHEN typeof(photos.captured_at) IN ('integer', 'real') THEN datetime(photos.captured_at / 1000, 'unixepoch')
WHEN photos.captured_at IS NOT NULL THEN replace(replace(substr(photos.captured_at, 1, 19), 'T', ' '), 'Z', '')
WHEN typeof(photos.uploaded_at) IN ('integer', 'real') THEN datetime(photos.uploaded_at / 1000, 'unixepoch')
ELSE substr(photos.uploaded_at, 1, 19)
END ${sortOrder}`);
}
photosQuery = photosQuery.orderBy('photos.id', sortOrder);
} else if (sort === 'filename') {
photosQuery = photosQuery.orderBy('photos.filename', sortOrder);
photosQuery = photosQuery.orderBy('photos.filename', sortOrder).orderBy('photos.id', sortOrder);
} else {
// Default: sort by upload date
photosQuery = photosQuery.orderBy('photos.uploaded_at', sortOrder);
photosQuery = photosQuery.orderBy('photos.uploaded_at', sortOrder).orderBy('photos.id', sortOrder);
}
// Reveal mode (#838): while the gallery is hidden, plain guests get