diff --git a/server/src/db/repositories/TrekPhotos.repository.ts b/server/src/db/repositories/TrekPhotos.repository.ts index 233f2d47d..1081135dd 100644 --- a/server/src/db/repositories/TrekPhotos.repository.ts +++ b/server/src/db/repositories/TrekPhotos.repository.ts @@ -142,6 +142,46 @@ export class TrekPhotosRepository extends TrekRepository { return changed > 0; } + /** + * Puts in the capture time and place a provider has just confirmed for this + * very photo, over whatever the row held (#3019: Synology rows that carry the + * next photo's). Unlike patchCaptureMetadata it replaces: the coordinates + * become the confirmed pair or none at all, while a missing time leaves the + * stored one, since every library photo has one and none is no correction. + * Answers whether the row changed. + */ + async replaceCaptureMetadata( + id: number, + taken_at: string | null, + lat: number | null, + lng: number | null, + ): Promise { + const row = await this.findOne({ id }); + if (!row) return false; + const next = { taken_at: taken_at ?? row.taken_at ?? null, lat, lng }; + if (next.taken_at === (row.taken_at ?? null) && next.lat === (row.lat ?? null) && next.lng === (row.lng ?? null)) { + return false; + } + await this.nativeUpdate({ id }, next); + return true; + } + + /** + * One provider's photos after `afterId`, oldest first, `limit` at a time, with + * the user whose library each lives in: the pages a provider-wide repair walks. + */ + async listByProviderAfter( + provider: string, + afterId: number, + limit: number, + ): Promise> { + const rows = await this.find( + { provider, id: { $gt: afterId }, owner: { $ne: null }, asset_id: { $ne: null } }, + { orderBy: { id: 'asc' }, limit }, + ); + return rows.flatMap((row) => (row.owner ? [{ id: row.id, owner_id: row.owner.id }] : [])); + } + /** PH11 — `DELETE FROM trek_photos WHERE id = ? AND provider != 'local'`; local photos are never auto-reclaimed here. */ async deleteNonLocal(id: number): Promise { return await this.nativeDelete({ id, provider: { $ne: 'local' } }); diff --git a/server/src/nest/memories/memories.module.ts b/server/src/nest/memories/memories.module.ts index 0fca188f2..e8e9813dc 100644 --- a/server/src/nest/memories/memories.module.ts +++ b/server/src/nest/memories/memories.module.ts @@ -1,3 +1,4 @@ +import { AppSettings } from '../../db/entities/AppSettings.entity'; import { JourneyContributors } from '../../db/entities/JourneyContributors.entity'; import { JourneyPhotos } from '../../db/entities/JourneyPhotos.entity'; import { Journeys } from '../../db/entities/Journeys.entity'; @@ -28,6 +29,7 @@ import { PhotoProviderRegistry } from './photo-provider.registry'; import { PhotoResolverService } from './photo-resolver.service'; import { ImmichPhotoProvider } from './providers/immich.provider'; import { SynologyPhotoProvider } from './providers/synology.provider'; +import { SynologyCaptureRepairJob } from './synology-capture-repair.job'; import { SynologyMemoriesController } from './synology.controller'; import { SynologyService } from './synology.service'; import { ThumbnailService } from './thumbnail.service'; @@ -84,6 +86,7 @@ import { Module } from '@nestjs/common'; StorageModule, UserConnectionRepositoriesModule, MikroOrmModule.forFeature([ + AppSettings, TripPhotos, TrekPhotos, TripAlbumLinks, @@ -109,6 +112,7 @@ import { Module } from '@nestjs/common'; TrekPhotoCacheService, TrekPhotoCacheJob, JourneyThumbsJob, + SynologyCaptureRepairJob, ImmichPhotoProvider, SynologyPhotoProvider, PhotoProviderRegistry, diff --git a/server/src/nest/memories/photo-capture-backfill.service.ts b/server/src/nest/memories/photo-capture-backfill.service.ts index 180ba6ac0..c7dc958c9 100644 --- a/server/src/nest/memories/photo-capture-backfill.service.ts +++ b/server/src/nest/memories/photo-capture-backfill.service.ts @@ -215,6 +215,30 @@ export class PhotoCaptureBackfillService { } } + /** + * Asks the provider again about a photo whose stored capture time and place + * may belong to another one, and puts in what it confirms (#3019). Inside the + * owner's lookup slots like an import, so a repair over a whole library never + * has more than BACKFILL_CONCURRENCY calls open against one NAS. `failed` is + * any photo the provider did not confirm: unreachable, gone, or no item whose + * thumbnail matches. Its row is left as it was. + */ + async reconfirm(id: number, ownerId: number): Promise<'changed' | 'same' | 'failed'> { + try { + const info = await this.withLookupSlot(ownerId, () => this.resolver.getPhotoInfo(ownerId, id)); + if (!info.success) return 'failed'; + const changed = await this.photos.replaceCaptureMetadata(id, { + takenAt: info.data.takenAt ?? null, + lat: info.data.lat ?? null, + lng: info.data.lng ?? null, + }); + return changed ? 'changed' : 'same'; + } catch (err) { + console.error(`[Photos] capture reconfirm failed for ${id}:`, err instanceof Error ? err.message : err); + return 'failed'; + } + } + /** * EXIF of an uploaded file. * diff --git a/server/src/nest/memories/synology-capture-repair.job.ts b/server/src/nest/memories/synology-capture-repair.job.ts new file mode 100644 index 000000000..8d8048467 --- /dev/null +++ b/server/src/nest/memories/synology-capture-repair.job.ts @@ -0,0 +1,92 @@ +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import { TrekPhotos } from '../../db/entities/TrekPhotos.entity'; +import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; +import type { TrekPhotosRepository } from '../../db/repositories/TrekPhotos.repository'; +import { logError, logInfo } from '../audit/audit-log.logger'; +import { CronRegistrarService } from '../scheduling/cron-registrar.service'; +import { PhotoCaptureBackfillService } from './photo-capture-backfill.service'; +import { InjectRepository } from '@mikro-orm/nestjs'; +import { Injectable, type OnApplicationBootstrap } from '@nestjs/common'; + +/** The app_settings row that says every Synology photo has been asked again. */ +export const SYNOLOGY_CAPTURE_REPAIR_DONE_KEY = 'synology_capture_repair_3019'; +/** How many starts have tried so far, while some photo could not be confirmed. */ +export const SYNOLOGY_CAPTURE_REPAIR_TRIES_KEY = 'synology_capture_repair_3019_tries'; + +const PROVIDER = 'synologyphotos'; +const PAGE = 200; +/** + * Starts that try again while photos stay unconfirmed. A NAS that was off for + * one restart gets its photos put right on the next; one taken away for good + * is not asked on every start forever. + */ +const MAX_TRIES = 3; + +/** + * One pass over every Synology photo on the first start after #3019. + * + * Until then the capture time and GPS of a Synology photo were read from the + * item one after it, which on many libraries is the next photo: journeys came + * out in the wrong order with pins in the wrong places, and nobody can edit a + * photo's capture time to put it right. The lookup now names the photo itself + * (SynologyService.getSynologyAssetInfo), and this asks again for every row, + * through each owner's own library, and puts in what Synology confirms. + * A photo it cannot confirm keeps what it has; on a library that was right all + * along the answers are the same and nothing changes. + * + * Not a cron: once the stored rows are right, the fixed lookup keeps them so. + * In the background through `CronRegistrarService.runOnBoot`, like the other + * boot repairs: the server does not wait for it. + */ +@Injectable() +export class SynologyCaptureRepairJob implements OnApplicationBootstrap { + constructor( + private readonly backfill: PhotoCaptureBackfillService, + private readonly registrar: CronRegistrarService, + @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, + @InjectRepository(TrekPhotos) private readonly trekPhotos: TrekPhotosRepository, + ) {} + + onApplicationBootstrap(): void { + if (!this.registrar.isEnabled()) return; + void this.registrar.runOnBoot('synology-capture-repair-boot', () => this.runOnce()); + } + + /** + * The pages after `after`, one after the other: each page's lookups run at + * once, held to a few per library by the backfill's slots. + */ + private async walk(after: number): Promise<{ changed: number; failed: number }> { + const page = await this.trekPhotos.listByProviderAfter(PROVIDER, after, PAGE); + if (page.length === 0) return { changed: 0, failed: 0 }; + const outcomes = await Promise.all(page.map((photo) => this.backfill.reconfirm(photo.id, photo.owner_id))); + const rest = await this.walk(page.at(-1)?.id ?? after); + return { + changed: rest.changed + outcomes.filter((o) => o === 'changed').length, + failed: rest.failed + outcomes.filter((o) => o === 'failed').length, + }; + } + + async runOnce(): Promise { + try { + if ((await this.appSettings.countKeysPresent([SYNOLOGY_CAPTURE_REPAIR_DONE_KEY])) > 0) return; + const tries = Number(await this.appSettings.getValue(SYNOLOGY_CAPTURE_REPAIR_TRIES_KEY)) || 0; + + const { changed, failed } = await this.walk(0); + + if (failed > 0 && tries + 1 < MAX_TRIES) { + await this.appSettings.upsertOrReplace(SYNOLOGY_CAPTURE_REPAIR_TRIES_KEY, String(tries + 1)); + } else { + await this.appSettings.upsertOrReplace(SYNOLOGY_CAPTURE_REPAIR_DONE_KEY, new Date().toISOString()); + await this.appSettings.deleteValue(SYNOLOGY_CAPTURE_REPAIR_TRIES_KEY); + } + if (changed + failed > 0) { + logInfo( + `Synology capture repair: ${changed} photo(s) given their own capture time and place, ${failed} not confirmed by their library`, + ); + } + } catch (err: unknown) { + logError(`Synology capture repair: ${err instanceof Error ? err.message : String(err)}`); + } + } +} diff --git a/server/src/nest/photos/trek-photo-registration.service.ts b/server/src/nest/photos/trek-photo-registration.service.ts index 6d24a563b..7a56f8cd9 100644 --- a/server/src/nest/photos/trek-photo-registration.service.ts +++ b/server/src/nest/photos/trek-photo-registration.service.ts @@ -143,6 +143,21 @@ export class TrekPhotoRegistrationService { return await this.trekPhotos.patchCaptureMetadata(photoId, takenAt, hasPair ? lat : null, hasPair ? lng : null); } + /** + * The capture time and place a provider has confirmed for this very photo, + * put in over what the row holds (#3019). Coordinates stay a pair or none: + * a confirmed photo without GPS loses the pair a wrong lookup gave it. + * Answers whether the row changed. + */ + async replaceCaptureMetadata( + photoId: number, + meta: { takenAt?: string | null; lat?: number | null; lng?: number | null }, + ): Promise { + const { takenAt = null, lat = null, lng = null } = meta; + const hasPair = Number.isFinite(lat) && Number.isFinite(lng); + return await this.trekPhotos.replaceCaptureMetadata(photoId, takenAt, hasPair ? lat : null, hasPair ? lng : null); + } + /** * PH10/PH11 — drop the row once no trip and no journey references it. * A local photo is kept unless the caller deletes it on purpose (`local`, diff --git a/server/tests/unit/nest/photo-capture-backfill.service.test.ts b/server/tests/unit/nest/photo-capture-backfill.service.test.ts index f6b443edc..91d7460fe 100644 --- a/server/tests/unit/nest/photo-capture-backfill.service.test.ts +++ b/server/tests/unit/nest/photo-capture-backfill.service.test.ts @@ -522,3 +522,49 @@ describe('PhotoCaptureBackfillService — local files', () => { await expect(stuck).resolves.toBe(true); }); }); + +/** #3019: a Synology photo stored with the next photo's capture data, asked again. */ +describe('PhotoCaptureBackfillService.reconfirm', () => { + function buildReconfirm(answer: () => Promise, changed = true) { + const replaceCaptureMetadata = vi.fn(async () => changed); + const getPhotoInfo = vi.fn(answer); + const photos = { replaceCaptureMetadata } as unknown as TrekPhotoRegistrationService; + const svc = new PhotoCaptureBackfillService( + { getPhotoInfo } as unknown as PhotoResolverService, + photos, + storageStub, + ); + return { svc, replaceCaptureMetadata, getPhotoInfo }; + } + + it('CAPTURE-100: asks through the owner and puts in what the library confirms', async () => { + const { svc, replaceCaptureMetadata, getPhotoInfo } = buildReconfirm(async () => ({ + success: true, + data: { takenAt: '2024-06-13T08:00:00Z', lat: 48.85, lng: 2.29 }, + })); + + expect(await svc.reconfirm(7, 3)).toBe('changed'); + expect(getPhotoInfo).toHaveBeenCalledWith(3, 7); + expect(replaceCaptureMetadata).toHaveBeenCalledWith(7, { takenAt: '2024-06-13T08:00:00Z', lat: 48.85, lng: 2.29 }); + }); + + it('CAPTURE-101: a row that already held it is the same, with no GPS sent as none', async () => { + const { svc, replaceCaptureMetadata } = buildReconfirm(async () => ({ success: true, data: {} }), false); + + expect(await svc.reconfirm(7, 3)).toBe('same'); + expect(replaceCaptureMetadata).toHaveBeenCalledWith(7, { takenAt: null, lat: null, lng: null }); + }); + + it('CAPTURE-102: leaves the row alone where the library does not confirm the photo, or the lookup throws', async () => { + const unconfirmed = buildReconfirm(async () => ({ success: false, error: 'Photo not found', status: 404 })); + expect(await unconfirmed.svc.reconfirm(7, 3)).toBe('failed'); + expect(unconfirmed.replaceCaptureMetadata).not.toHaveBeenCalled(); + + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const throwing = buildReconfirm(async () => { + throw new Error('NAS gone'); + }); + expect(await throwing.svc.reconfirm(7, 3)).toBe('failed'); + errSpy.mockRestore(); + }); +}); diff --git a/server/tests/unit/nest/synology-capture-repair.job.test.ts b/server/tests/unit/nest/synology-capture-repair.job.test.ts new file mode 100644 index 000000000..57ee77a0c --- /dev/null +++ b/server/tests/unit/nest/synology-capture-repair.job.test.ts @@ -0,0 +1,108 @@ +import { db } from '../../../src/db/database'; +import { AppSettings } from '../../../src/db/entities/AppSettings.entity'; +import type { TrekPhotosRepository } from '../../../src/db/repositories/TrekPhotos.repository'; +import type { PhotoCaptureBackfillService } from '../../../src/nest/memories/photo-capture-backfill.service'; +import { + SYNOLOGY_CAPTURE_REPAIR_DONE_KEY, + SYNOLOGY_CAPTURE_REPAIR_TRIES_KEY, + SynologyCaptureRepairJob, +} from '../../../src/nest/memories/synology-capture-repair.job'; +import type { CronRegistrarService } from '../../../src/nest/scheduling/cron-registrar.service'; +import { findRow } from '../../helpers/factories/rows'; +import { resetTestDb } from '../../helpers/test-db'; +import { createTestAppSettingsRepo, sharedTestOrm } from '../../helpers/test-uow'; + +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +/** #3019: one pass over every Synology photo, asking its library again. */ +describe('SynologyCaptureRepairJob', () => { + const PHOTOS = [ + { id: 1, owner_id: 10 }, + { id: 2, owner_id: 10 }, + { id: 3, owner_id: 11 }, + ]; + // runOnBoot is where the real registrar opens the request context; here it just runs the pass. + const registrar = (enabled: boolean) => + ({ + isEnabled: () => enabled, + runOnBoot: async (_name: string, fn: () => Promise) => fn(), + }) as unknown as CronRegistrarService; + // Pages of two, the way listByProviderAfter answers past an id. + const trekPhotos = { + listByProviderAfter: vi.fn(async (_provider: string, after: number) => + PHOTOS.filter((p) => p.id > after).slice(0, 2), + ), + } as unknown as TrekPhotosRepository; + const reconfirm = vi.fn(); + const backfill = { reconfirm } as unknown as PhotoCaptureBackfillService; + const setting = async (key: string) => (await findRow(await sharedTestOrm(db), AppSettings, { key }))?.value; + const job = async (enabled = true) => + new SynologyCaptureRepairJob(backfill, registrar(enabled), await createTestAppSettingsRepo(db), trekPhotos); + + beforeEach(() => { + resetTestDb(db); + reconfirm.mockReset(); + vi.mocked(trekPhotos.listByProviderAfter).mockClear(); + }); + + it('SYNOREPAIR-001: asks again for every Synology photo, page by page, through its owner, then never again', async () => { + reconfirm.mockResolvedValue('changed'); + const repair = await job(); + + await repair.runOnce(); + + expect(reconfirm.mock.calls).toEqual([ + [1, 10], + [2, 10], + [3, 11], + ]); + expect(vi.mocked(trekPhotos.listByProviderAfter).mock.calls.map((c) => c[0])).toEqual([ + 'synologyphotos', + 'synologyphotos', + 'synologyphotos', + ]); + expect(await setting(SYNOLOGY_CAPTURE_REPAIR_DONE_KEY)).toBeTruthy(); + + reconfirm.mockClear(); + await repair.runOnce(); + expect(reconfirm).not.toHaveBeenCalled(); + }); + + it('SYNOREPAIR-002: tries again on the next start while a library could not confirm, at most three starts', async () => { + reconfirm.mockImplementation(async (id: number) => (id === 3 ? 'failed' : 'same')); + const repair = await job(); + + await repair.runOnce(); + expect(await setting(SYNOLOGY_CAPTURE_REPAIR_DONE_KEY)).toBeUndefined(); + expect(await setting(SYNOLOGY_CAPTURE_REPAIR_TRIES_KEY)).toBe('1'); + + await repair.runOnce(); + expect(await setting(SYNOLOGY_CAPTURE_REPAIR_TRIES_KEY)).toBe('2'); + + await repair.runOnce(); + expect(await setting(SYNOLOGY_CAPTURE_REPAIR_DONE_KEY)).toBeTruthy(); + expect(await setting(SYNOLOGY_CAPTURE_REPAIR_TRIES_KEY)).toBeUndefined(); + }); + + it('SYNOREPAIR-003: an error stops the pass without marking it done, so the next start runs it', async () => { + reconfirm.mockRejectedValue(new Error('boom')); + const repair = await job(); + + await repair.runOnce(); + + expect(await setting(SYNOLOGY_CAPTURE_REPAIR_DONE_KEY)).toBeUndefined(); + }); + + it('SYNOREPAIR-004: starts from the boot hook, but not where the scheduler is off', async () => { + const off = await job(false); + const on = await job(true); + const offRun = vi.spyOn(off, 'runOnce').mockResolvedValue(); + const onRun = vi.spyOn(on, 'runOnce').mockResolvedValue(); + + off.onApplicationBootstrap(); + on.onApplicationBootstrap(); + + expect(offRun).not.toHaveBeenCalled(); + expect(onRun).toHaveBeenCalledTimes(1); + }); +}); diff --git a/server/tests/unit/nest/trek-photo-registration.service.test.ts b/server/tests/unit/nest/trek-photo-registration.service.test.ts index 0bc1545de..027661210 100644 --- a/server/tests/unit/nest/trek-photo-registration.service.test.ts +++ b/server/tests/unit/nest/trek-photo-registration.service.test.ts @@ -113,6 +113,58 @@ describe('TrekPhotoRegistrationService.recordCaptureMetadata', () => { }); }); +describe('TrekPhotoRegistrationService.replaceCaptureMetadata (#3019)', () => { + // A unique asset per row: resetTestDb leaves trek_photos alone. + async function synologyPhoto(): Promise { + const { user } = createUser(testDb); + return await repo.getOrCreate('synologyphotos', `${++seq}_1793757217`, user.id); + } + + it("TREKPHOTO-100: puts the confirmed time and place over a neighbour's", async () => { + const id = await synologyPhoto(); + await repo.recordCaptureMetadata(id, { takenAt: '2025-09-27T10:00:00Z', lat: 1, lng: 2 }); + + expect(await repo.replaceCaptureMetadata(id, { takenAt: '2024-06-13T08:00:00Z', lat: 48.85, lng: 2.29 })).toBe( + true, + ); + expect(await read(id)).toEqual({ taken_at: '2024-06-13T08:00:00Z', lat: 48.85, lng: 2.29 }); + }); + + it('TREKPHOTO-101: a confirmed photo without GPS loses the pair a wrong lookup gave it, and keeps its time', async () => { + const id = await synologyPhoto(); + await repo.recordCaptureMetadata(id, { takenAt: '2025-09-27T10:00:00Z', lat: 1, lng: 2 }); + + expect(await repo.replaceCaptureMetadata(id, { takenAt: null, lat: 48.85, lng: null })).toBe(true); + expect(await read(id)).toEqual({ taken_at: '2025-09-27T10:00:00Z', lat: null, lng: null }); + }); + + it('TREKPHOTO-102: answers false when the row already holds what was confirmed, or is gone', async () => { + const id = await synologyPhoto(); + await repo.recordCaptureMetadata(id, { takenAt: '2024-06-13T08:00:00Z', lat: 48.85, lng: 2.29 }); + + expect(await repo.replaceCaptureMetadata(id, { takenAt: '2024-06-13T08:00:00Z', lat: 48.85, lng: 2.29 })).toBe( + false, + ); + expect(await repo.replaceCaptureMetadata(987654321, { takenAt: '2024-06-13T08:00:00Z' })).toBe(false); + }); +}); + +describe('TrekPhotosRepository.listByProviderAfter (#3019)', () => { + it("TREKPHOTO-103: walks one provider's photos in id order, a page at a time, each with its library's owner", async () => { + const { user } = createUser(testDb); + const a = await repo.getOrCreate('synologyphotos', `${++seq}_a`, user.id); + await repo.getOrCreate('immich', `${++seq}_b`, user.id); + const c = await repo.getOrCreate('synologyphotos', `${++seq}_c`, user.id); + const photos = t.repo(TrekPhotos); + + const first = await photos.listByProviderAfter('synologyphotos', a - 1, 1); + expect(first).toEqual([{ id: a, owner_id: user.id }]); + const rest = await photos.listByProviderAfter('synologyphotos', a, 10); + expect(rest.map((r) => r.id)).toContain(c); + expect(rest.every((r) => r.id > a)).toBe(true); + }); +}); + describe('TrekPhotoRegistrationService.getOrCreate (PH1-3)', () => { it('TREKPHOTO-010: registers a new remote asset', async () => { const { user } = createUser(testDb);