From cec8eff70ce55d93bdb0b582ec39de400711892b Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Thu, 27 Aug 2026 08:44:19 +0200 Subject: [PATCH] fix(images): fence the capture-date backfill on the file it read (#1201) (#1204) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The capture-date backfill committed its result keyed on the row id alone. It snapshots every candidate up front, then walks them one at a time reading originals off S3 or a NAS mount — a pass that can run for many minutes. replacePhoto, reachable from the replace_by_name upload path, swaps a NEW file under an existing row and rewrites path/filename. A replacement landing inside that window carries no date of its own, so captured_at was still NULL, the whereNull guard passed, and the previous file's EXIF date was written onto the new photo. Silent: nothing errored, the run reported it as a success, and the gallery just sorted that photo to the wrong place. Fenced on path and filename as well as the id — the same fence #1199 put on the orientation backfill for the same reason — so a replaced row matches zero rows and is skipped. The candidate query already selects both columns, so no query change. Knex renders a null value in the object form as `is null` on both the pg and sqlite3 clients, so a row with a NULL path still matches itself. Those skipped candidates are now counted rather than dropped. replacePhoto is not the only writer of path/filename — eventRenameService rewrites both on an event rename, which is not a content change — and another writer filling captured_at first lands in the same place. Without a counter they fell out of the run's arithmetic entirely: success + noExif + failed no longer added up to the count the operator was shown when they started the job, on the card as well as in the log. The card shows the count only when it is non-zero, the same shape the orientation job uses for staleTiers. The wording states what is known — changed by something else, not updated — rather than promising a retry: for the already-dated case there is nothing to retry, and the Missing Capture Date figure above is what says whether work is left. Locale coverage matches the staleTiers key (en, de, fr, sl), with the defaultValue carrying the rest. Regression test: a replacement landing mid-run leaves captured_at NULL and is not counted as updated. Verified to fail against the unfenced code. --- .../integration/captureDateBackfill.test.js | 25 +++++++++++++++ backend/src/routes/adminPhotoDimensions.js | 32 ++++++++++++++++--- .../src/features/settings/tabs/StatusTab.tsx | 18 +++++++++++ frontend/src/i18n/locales/de.json | 1 + frontend/src/i18n/locales/en.json | 1 + frontend/src/i18n/locales/fr.json | 1 + frontend/src/i18n/locales/sl.json | 1 + 7 files changed, 74 insertions(+), 5 deletions(-) diff --git a/backend/__tests__/integration/captureDateBackfill.test.js b/backend/__tests__/integration/captureDateBackfill.test.js index 38074878..3e9301f5 100644 --- a/backend/__tests__/integration/captureDateBackfill.test.js +++ b/backend/__tests__/integration/captureDateBackfill.test.js @@ -198,5 +198,30 @@ describe('capture date backfill (#1172)', () => { expect(new Date((await db('photos').where({ id: photoId }).first()).captured_at).toISOString()).toBe(claimed); expect(done.body.lastResult.success).toBe(0); + // Read but not written, so it is accounted for rather than dropped. + expect(done.body.lastResult.skipped).toBe(1); + }); + + it('does not date a row whose file was replaced while it was reading (#1201)', async () => { + // replacePhoto swaps a NEW file under an existing row and rewrites + // path/filename (reachable from replace_by_name). The replacement carries + // no date of its own, so captured_at is still NULL and the whereNull guard + // alone would let the previous file's EXIF date land on it. The write is + // fenced on the identity that was read, so the row is skipped instead — + // and not counted as updated either. + const { photoId } = await seed({ relpath: 'orig.jpg', exifIso: '2026-06-03T09:00:00Z' }); + + const res = await request(app).post('/api/admin/photos/repair-capture-dates'); + expect(res.body.count).toBe(1); + // Simulate the replacement landing before the loop writes. + await db('photos').where({ id: photoId }) + .update({ path: 'capfill/replaced.jpg', filename: 'replaced.jpg' }); + const done = await settle(); + + expect((await db('photos').where({ id: photoId }).first()).captured_at).toBeNull(); + expect(done.body.lastResult.success).toBe(0); + // Not an error and not "no EXIF" — the date was found, another writer just + // got there first. It stays in the backlog for the next run. + expect(done.body.lastResult).toMatchObject({ noExif: 0, failed: 0, skipped: 1 }); }); }); diff --git a/backend/src/routes/adminPhotoDimensions.js b/backend/src/routes/adminPhotoDimensions.js index 9dcfcafc..a150685c 100644 --- a/backend/src/routes/adminPhotoDimensions.js +++ b/backend/src/routes/adminPhotoDimensions.js @@ -356,6 +356,7 @@ router.post('/repair-capture-dates', adminAuth, requirePermission('system.manage let successCount = 0; let missingCount = 0; let errorCount = 0; + let skippedCount = 0; let lostClaim = false; // Same reasoning as the dimension repair: detached from the request, so @@ -430,11 +431,29 @@ router.post('/repair-capture-dates', adminAuth, requirePermission('system.manage // 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. + // + // Fenced on path and filename as well as the id, for the same reason + // the orientation backfill below is (#1199): replacePhoto — reachable + // from the replace_by_name upload path (adminPhotos.js) — swaps a NEW + // file under an existing row and rewrites path/filename. That + // replacement carries no date of its own, so captured_at is still + // NULL and whereNull alone would let the previous file's EXIF date + // land on it. Matching the identity that was actually read means the + // update affects no rows and the row is simply skipped. const updated = await db('photos') - .where({ id: photo.id }) + .where({ id: photo.id, path: photo.path, filename: photo.filename }) .whereNull('captured_at') .update({ captured_at: captured.toISOString() }); - if (updated) successCount++; + // Counted, not dropped: without this a candidate that was read but + // not written falls out of the run's arithmetic entirely, and + // success + noExif + failed silently stops adding up to the count + // the operator was shown when they started it. Two ways to land + // here, both "another writer got there first" — the row was dated + // meanwhile (whereNull), or its file changed under us (the fence). + // Neither is an error and neither needs a retry: captured_at is + // still NULL for the fenced case, so the status endpoint keeps + // reporting it as backlog and the next run picks it up. + if (updated) successCount++; else skippedCount++; if (successCount % 50 === 0 && successCount > 0) { logger.info(`Capture date backfill progress: ${successCount} updated...`); @@ -449,12 +468,15 @@ router.post('/repair-capture-dates', adminAuth, requirePermission('system.manage logger.warn(`Capture date backfill stopped: claim taken over after ${successCount} updated, ${errorCount} errors`); return; } - await maintenanceJobs.release(JOB_CAPTURE_DATE_BACKFILL, token, { success: successCount, noExif: missingCount, failed: errorCount }); - logger.info(`Capture date backfill complete: ${successCount} updated, ${missingCount} without EXIF, ${errorCount} errors`); + await maintenanceJobs.release(JOB_CAPTURE_DATE_BACKFILL, token, { success: successCount, noExif: missingCount, failed: errorCount, skipped: skippedCount }); + logger.info( + `Capture date backfill complete: ${successCount} updated, ${missingCount} without EXIF, ` + + `${errorCount} errors, ${skippedCount} skipped (dated or replaced mid-run)` + ); } catch (err) { logger.error('Capture date backfill aborted:', err); await maintenanceJobs - .release(JOB_CAPTURE_DATE_BACKFILL, token, { success: successCount, noExif: missingCount, failed: errorCount, error: err.message }) + .release(JOB_CAPTURE_DATE_BACKFILL, token, { success: successCount, noExif: missingCount, failed: errorCount, skipped: skippedCount, error: err.message }) .catch(() => {}); } finally { lease.stop(); diff --git a/frontend/src/features/settings/tabs/StatusTab.tsx b/frontend/src/features/settings/tabs/StatusTab.tsx index 1f018bd8..dbfae259 100644 --- a/frontend/src/features/settings/tabs/StatusTab.tsx +++ b/frontend/src/features/settings/tabs/StatusTab.tsx @@ -734,6 +734,24 @@ export const StatusTab: React.FC = ({ failed: captureDateStatus.lastResult.failed, defaultValue: 'Last run: {{success}} updated, {{noExif}} with no date found, {{failed}} unreachable', })} + {/* Only when it happened, like the orientation job's staleTiers + below. Without it the three numbers above silently stop + adding up to the count the run started with: a photo that + was replaced, renamed or dated by someone else mid-run is + read but not written. + Deliberately says "not updated" and not "will be retried": + one of the two ways to land here is another writer having + filled captured_at, and that photo is finished, not backlog. + The Missing Capture Date figure above is what says whether + anything is actually left to do. */} + {Number(captureDateStatus.lastResult.skipped) > 0 && ( + + {t('settings.captureDates.skipped', { + count: captureDateStatus.lastResult.skipped, + defaultValue: '{{count}} photo(s) were changed by something else while the run was reading them and were not updated.', + })} + + )}

)} diff --git a/frontend/src/i18n/locales/de.json b/frontend/src/i18n/locales/de.json index 6e33e3d9..138f9cd1 100644 --- a/frontend/src/i18n/locales/de.json +++ b/frontend/src/i18n/locales/de.json @@ -2362,6 +2362,7 @@ "running": "Wird nachgetragen...", "noneToFill": "Alle Fotos haben bereits ein Aufnahmedatum", "resultSuccess": "Letzter Lauf: {{success}} aktualisiert, {{noExif}} ohne gefundenes Datum, {{failed}} nicht erreichbar", + "skipped": "{{count}} Foto(s) wurden während des Laufs anderweitig geändert und daher nicht aktualisiert.", "description": "Trägt „Aufnahmedatum\" aus den EXIF-Daten nach, für Fotos die vor dieser Auswertung importiert wurden. Externe Importe haben nie eines gespeichert, dadurch sortieren diese Galerien nach Importreihenfolge statt nach Aufnahmezeit." }, "orientationBackfill": { diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index 847ff941..d9e0487f 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -1903,6 +1903,7 @@ "running": "Backfilling...", "noneToFill": "All photos already have a capture date", "resultSuccess": "Last run: {{success}} updated, {{noExif}} with no date found, {{failed}} unreachable", + "skipped": "{{count}} photo(s) were changed by something else while the run was reading them and were not updated.", "description": "Backfill \"Date Taken\" from EXIF for photos imported before capture dates were read. External/reference imports never recorded one, so their galleries sort by import order instead of when the photos were taken." }, "orientationBackfill": { diff --git a/frontend/src/i18n/locales/fr.json b/frontend/src/i18n/locales/fr.json index 3ecb386f..62b6b5e4 100644 --- a/frontend/src/i18n/locales/fr.json +++ b/frontend/src/i18n/locales/fr.json @@ -1211,6 +1211,7 @@ "running": "Traitement...", "noneToFill": "Toutes les photos ont déjà une date de prise de vue", "resultSuccess": "Dernier passage : {{success}} mises à jour, {{noExif}} sans date trouvée, {{failed}} inaccessibles", + "skipped": "{{count}} photo(s) ont été modifiées par autre chose pendant le passage et n'ont donc pas été mises à jour.", "description": "Complète la « date de prise de vue » depuis les EXIF pour les photos importées avant sa lecture. Les imports externes n'en enregistraient aucune, si bien que ces galeries se trient par ordre d'import plutôt que par date de prise de vue." }, "orientationBackfill": { diff --git a/frontend/src/i18n/locales/sl.json b/frontend/src/i18n/locales/sl.json index 603d4c90..3c4018d1 100644 --- a/frontend/src/i18n/locales/sl.json +++ b/frontend/src/i18n/locales/sl.json @@ -1211,6 +1211,7 @@ "running": "Dopolnjevanje...", "noneToFill": "Vse fotografije že imajo datum zajema", "resultSuccess": "Zadnji zagon: {{success}} posodobljenih, {{noExif}} brez najdenega datuma, {{failed}} nedosegljivih", + "skipped": "{{count}} fotografij je bilo med zagonom spremenjenih drugje in zato niso bile posodobljene.", "description": "Dopolni »datum zajema« iz EXIF za fotografije, uvožene pred njegovim branjem. Zunanji uvozi ga niso zabeležili, zato se te galerije razvrščajo po vrstnem redu uvoza namesto po času zajema." }, "orientationBackfill": {