fix(memories): save the Immich connection and auto_upload in one transaction

PUT /api/integrations/memories/immich/settings saved the connection through
one service call and then wrote auto_upload through a second one from
the controller, each committed on its own. A failure between them left
a new connection with the old upload choice.

saveImmichSettings takes the auto_upload flag and writes it with the
connection (or the disconnect) inside one transaction, after the SSRF
check that does the DNS lookup. An absent or non-boolean flag still
keeps the stored choice, a refused URL still writes nothing, and the
response is unchanged. The controller's write sequence leaves the
lint:tx baseline, and the MemoriesService facade loses the
immichSetAutoUpload pass-through it no longer needs. No MCP tool writes
these settings.

server/scripts/tx-writes-baseline.json

@@ -40,7 +40,6 @@
   "src/nest/maps/maps.service.ts#MapsService.getPlaceDetails": 2,
   "src/nest/maps/maps.service.ts#MapsService.getPlaceDetailsExpanded": 2,
   "src/nest/maps/maps.service.ts#MapsService.getPlacePhoto": 7,
-  "src/nest/memories/immich.controller.ts#ImmichMemoriesController.putSettings": 2,
   "src/nest/memories/photo-resolver.service.ts#PhotoResolverService.streamCachedThumbnail": 3,
   "src/nest/memories/photo-resolver.service.ts#PhotoResolverService.streamPhoto": 2,
   "src/nest/memories/synology.service.ts#SynologyService._fetchAllSynologyAlbums": 2,

server/src/nest/memories/immich.controller.ts

@@ -37,15 +37,14 @@ export class ImmichMemoriesController {
   ): Promise<void> {
     const { immich_url, immich_api_key, auto_upload, allow_insecure_tls } = body;
     // Absent stays undefined and leaves the stored choice alone, so an older
-    // client cannot clear it by saving.
-    const result = await this.memories.immichSaveSettings(user.id, immich_url, immich_api_key, getClientIp(req), allow_insecure_tls);
+    // client cannot clear it by saving. auto_upload is written with the
+    // connection in one transaction, and only when it is a boolean.
+    const autoUpload = typeof auto_upload === 'boolean' ? auto_upload : undefined;
+    const result = await this.memories.immichSaveSettings(user.id, immich_url, immich_api_key, getClientIp(req), allow_insecure_tls, autoUpload);
     if (!result.success) {
       res.status(400).json({ error: result.error });
       return;
     }
-    if (typeof auto_upload === 'boolean') {
-      await this.memories.immichSetAutoUpload(user.id, auto_upload);
-    }
     if (result.warning) {
       res.json({ success: true, warning: result.warning });
       return;

server/src/nest/memories/immich.service.ts

@@ -11,6 +11,7 @@ import path from 'node:path';
 import { MemoriesAccessService } from './memories-access.service';
 import { describeFetchFailure, fail, handleServiceResult, isWithinLocalDayRange, pipeAsset, shiftCalendarDay, sortAssetsByTakenAtDesc, type Selection } from './memories.helpers';
 import { Users } from '../../db/entities/Users.entity';
+import { UnitOfWork } from '../database/unit-of-work';
 import type { UsersRepository } from '../../db/repositories/Users.repository';
 
 const ALBUM_PAGE_SIZE = 1000;
@@ -81,6 +82,7 @@ export class ImmichService {
     private readonly access: MemoriesAccessService,
     private readonly storage: StorageService,
     @InjectRepository(Users) private readonly users: UsersRepository,
+    private readonly uow: UnitOfWork,
   ) {}
 
   async getImmichCredentials(userId: number): Promise<ImmichCreds | null> {
@@ -138,7 +140,9 @@ export class ImmichService {
    * `allowInsecureTls` left undefined keeps the stored choice while the URL
    * stays the same, so a client that does not know the switch cannot clear it
    * by saving. The switch trusts one server, so a new URL without it starts
-   * off, and disconnecting (no URL) always turns it off again.
+   * off, and disconnecting (no URL) always turns it off again. `autoUpload`
+   * is written with the connection in one transaction, after the URL check;
+   * left undefined it keeps the stored choice.
    */
   async saveImmichSettings(
     userId: number,
@@ -146,32 +150,38 @@ export class ImmichService {
     immichApiKey: string | undefined,
     clientIp: string | null,
     allowInsecureTls?: boolean,
+    autoUpload?: boolean,
   ): Promise<{ success: boolean; warning?: string; error?: string }> {
+    let ssrf: Awaited<ReturnType<typeof checkSsrf>> | null = null;
+    const url = (immichUrl?.endsWith('/') ? immichUrl.slice(0, -1) : immichUrl ?? '').trim();
     if (immichUrl) {
-      if (immichUrl.endsWith('/')) {
-        immichUrl = immichUrl.slice(0, -1);
-      }
-      const ssrf = await checkSsrf(immichUrl.trim());
+      ssrf = await checkSsrf(url);
       if (!ssrf.allowed) {
         return { success: false, error: `Invalid Immich URL: ${ssrf.error}` };
       }
-      const insecure = allowInsecureTls === undefined ? null : Number(allowInsecureTls);
-      // The stored URL decides whether an undefined switch keeps its value (IM4).
-      await this.users.setImmichSettings(userId, immichUrl.trim(), maybe_encrypt_api_key(immichApiKey), insecure);
-      if (ssrf.isPrivate) {
-        await this.audit.writeAudit({
-          userId,
-          action: 'immich.private_ip_configured',
-          ip: clientIp,
-          details: { immich_url: immichUrl.trim(), resolved_ip: ssrf.resolvedIp },
-        });
-        return {
-          success: true,
-          warning: `Immich URL resolves to a private IP address (${ssrf.resolvedIp}). Make sure this is intentional.`,
-        };
+    }
+    // The DNS check above is network I/O, so it runs before the transaction opens.
+    await this.uow.transactional(async () => {
+      if (immichUrl) {
+        const insecure = allowInsecureTls === undefined ? null : Number(allowInsecureTls);
+        // The stored URL decides whether an undefined switch keeps its value (IM4).
+        await this.users.setImmichSettings(userId, url, maybe_encrypt_api_key(immichApiKey), insecure);
+      } else {
+        await this.users.clearImmichSettings(userId, maybe_encrypt_api_key(immichApiKey));
       }
-    } else {
-      await this.users.clearImmichSettings(userId, maybe_encrypt_api_key(immichApiKey));
+      if (typeof autoUpload === 'boolean') await this.setImmichAutoUpload(userId, autoUpload);
+    });
+    if (ssrf?.isPrivate) {
+      await this.audit.writeAudit({
+        userId,
+        action: 'immich.private_ip_configured',
+        ip: clientIp,
+        details: { immich_url: url, resolved_ip: ssrf.resolvedIp },
+      });
+      return {
+        success: true,
+        warning: `Immich URL resolves to a private IP address (${ssrf.resolvedIp}). Make sure this is intentional.`,
+      };
     }
     return { success: true };
   }

server/src/nest/memories/memories.service.ts

@@ -67,12 +67,8 @@ export class MemoriesService {
     return this.immich.getConnectionSettings(userId);
   }
 
-  immichSaveSettings(userId: number, immichUrl: string | undefined, immichApiKey: string | undefined, clientIp: string | null, allowInsecureTls?: boolean) {
-    return this.immich.saveImmichSettings(userId, immichUrl, immichApiKey, clientIp, allowInsecureTls);
-  }
-
-  immichSetAutoUpload(userId: number, enabled: boolean): Promise<void> {
-    return this.immich.setImmichAutoUpload(userId, enabled);
+  immichSaveSettings(userId: number, immichUrl: string | undefined, immichApiKey: string | undefined, clientIp: string | null, allowInsecureTls?: boolean, autoUpload?: boolean) {
+    return this.immich.saveImmichSettings(userId, immichUrl, immichApiKey, clientIp, allowInsecureTls, autoUpload);
   }
 
   immichGetConnectionStatus(userId: number) {

server/tests/e2e/memories.e2e.test.ts

@@ -206,7 +206,8 @@ describe('Memories e2e (real auth guard + temp SQLite)', () => {
       const ok = await request(server).put(`${IMMICH}/settings`).set('Cookie', sessionCookie(1)).send({ immich_url: 'https://x', immich_api_key: 'k', auto_upload: true });
       expect(ok.status).toBe(200);
       expect(ok.body).toEqual({ success: true });
-      expect(immich.setImmichAutoUpload).toHaveBeenCalledWith(1, true);
+      expect(immich.saveImmichSettings).toHaveBeenCalledWith(1, 'https://x', 'k', expect.anything(), undefined, true);
+      expect(immich.setImmichAutoUpload).not.toHaveBeenCalled();
 
       immich.saveImmichSettings.mockResolvedValue({ success: false, error: 'Invalid Immich URL: bad' });
       const bad = await request(server).put(`${IMMICH}/settings`).set('Cookie', sessionCookie(1)).send({ immich_url: 'bad' });

server/tests/helpers/mcp-test-controllers.ts

@@ -403,7 +403,7 @@ export async function createMcpTestRegistry(): Promise<McpRegistry> {
     // Plan 3g Task 4 constructor-ripple: MA1/MA2/MA6's journey half.
     await createTestJourneysRepo(db), await createTestJourneyContributorsRepo(db), await createTestJourneyPhotosRepo(db),
   );
-  const immichService = new ImmichService(new AuditService(auditLogRepo, usersRepo), memoriesAccess, generalStorage, usersRepo);
+  const immichService = new ImmichService(new AuditService(auditLogRepo, usersRepo), memoriesAccess, generalStorage, usersRepo, await createTestUnitOfWork(db));
   const synologyService = new SynologyService(memoriesAccess, notificationsStub(), usersRepo);
   const trekPhotos = new TrekPhotoRegistrationService(mcpOrm.repo(TrekPhotos), mcpOrm.repo(TripPhotos), await createTestJourneyPhotosRepo(db));
   const captureBackfill = new PhotoCaptureBackfillService(new PhotoResolverService(trekPhotos, new ThumbnailService(addonsService, generalStorage, mcpOrm.repo(TrekPhotos)), new TrekPhotoCacheService(mcpOrm.repo(TrekPhotoCacheMeta), generalStorage), new PhotoProviderRegistry([new ImmichPhotoProvider(immichService), new SynologyPhotoProvider(synologyService)]), generalStorage), trekPhotos, generalStorage);

server/tests/unit/nest/immich.service.test.ts

@@ -47,7 +47,7 @@ import type { MemoriesAccessService } from '../../../src/nest/memories/memories-
 import fs from 'node:fs';
 import path from 'node:path';
 import { makeStorageFixture } from '../../helpers/storage-fixture';
-import { createTestUsersRepo } from '../../helpers/test-uow';
+import { createTestUnitOfWork, createTestUsersRepo } from '../../helpers/test-uow';
 import { PROVIDER_SELECT_ALL_MAX_PAGES } from '@trek/shared';
 
 const audit = { writeAudit: vi.fn() };
@@ -76,7 +76,7 @@ function upstream(opts: { ok?: boolean; status?: number; json?: unknown; url?: s
 
 beforeAll(async () => {
   const users = await createTestUsersRepo(testDb);
-  svc = new ImmichService(audit as unknown as AuditService, access as unknown as MemoriesAccessService, journeyFx.storage, users);
+  svc = new ImmichService(audit as unknown as AuditService, access as unknown as MemoriesAccessService, journeyFx.storage, users, await createTestUnitOfWork(testDb));
 });
 
 beforeEach(() => {
@@ -176,6 +176,35 @@ describe('saveImmichSettings', () => {
     expect(await svc.getImmichCredentials(USER)).toBeNull();
   });
 
+  it('IMMICH-052: writes auto_upload with the connection, and keeps it when the flag is absent', async () => {
+    const autoUpload = () => (testDb.prepare('SELECT immich_auto_upload FROM users WHERE id = ?').get(USER) as { immich_auto_upload: number }).immich_auto_upload;
+
+    await svc.saveImmichSettings(USER, 'https://new.test', 'k2', null, undefined, true);
+    expect(autoUpload()).toBe(1);
+    await svc.saveImmichSettings(USER, 'https://new.test', 'k2', null);
+    expect(autoUpload()).toBe(1);
+    await svc.saveImmichSettings(USER, undefined, undefined, null, undefined, false);
+    expect(autoUpload()).toBe(0);
+  });
+
+  it('IMMICH-053: a refused URL writes no auto_upload either', async () => {
+    checkSsrf.mockResolvedValue({ allowed: false, error: 'blocked host' });
+
+    await svc.saveImmichSettings(USER, 'http://169.254.169.254', 'k', null, undefined, true);
+
+    expect(testDb.prepare('SELECT immich_auto_upload FROM users WHERE id = ?').get(USER)).toEqual({ immich_auto_upload: 0 });
+  });
+
+  it('IMMICH-054: a failing auto_upload write rolls the new connection back with it', async () => {
+    const spy = vi.spyOn(svc, 'setImmichAutoUpload').mockRejectedValueOnce(new Error('boom'));
+    try {
+      await expect(svc.saveImmichSettings(USER, 'https://new.test', 'k2', null, undefined, true)).rejects.toThrow('boom');
+      expect((await svc.getImmichCredentials(USER))!.immich_url).toBe('https://immich.test');
+    } finally {
+      spy.mockRestore();
+    }
+  });
+
   // R6 — the encrypt call still runs; this file mocks `maybe_encrypt_api_key`
   // as identity everywhere else (so cases above can assert on the plaintext
   // round-trip), so this one case swaps in the REAL crypto for the duration

server/tests/unit/nest/memories.controller.test.ts

@@ -236,23 +236,21 @@ describe('ImmichMemoriesController (parity with /api/integrations/memories/immic
       expect(res.json).toHaveBeenCalledWith({ error: 'Bad URL' });
     });
 
-    it('applies auto_upload when it is a boolean and returns success', async () => {
+    it('hands auto_upload to the one settings save when it is a boolean and returns success', async () => {
       const immichSaveSettings = vi.fn().mockResolvedValue({ success: true });
-      const immichSetAutoUpload = vi.fn();
-      const svc = makeService({ immichSaveSettings, immichSetAutoUpload });
+      const svc = makeService({ immichSaveSettings });
       const res = makeRes();
       await new ImmichMemoriesController(svc).putSettings(user, { immich_url: 'x', immich_api_key: 'k', auto_upload: true }, req, res);
-      expect(immichSaveSettings).toHaveBeenCalledWith(7, 'x', 'k', '1.2.3.4', undefined);
-      expect(immichSetAutoUpload).toHaveBeenCalledWith(7, true);
+      expect(immichSaveSettings).toHaveBeenCalledTimes(1);
+      expect(immichSaveSettings).toHaveBeenCalledWith(7, 'x', 'k', '1.2.3.4', undefined, true);
       expect(res.json).toHaveBeenCalledWith({ success: true });
     });
 
-    it('skips auto_upload when it is not a boolean', async () => {
+    it('leaves auto_upload undefined when it is not a boolean', async () => {
       const immichSaveSettings = vi.fn().mockResolvedValue({ success: true });
-      const immichSetAutoUpload = vi.fn();
-      const svc = makeService({ immichSaveSettings, immichSetAutoUpload });
+      const svc = makeService({ immichSaveSettings });
       await new ImmichMemoriesController(svc).putSettings(user, { auto_upload: 'yes' as unknown as boolean }, req, makeRes());
-      expect(immichSetAutoUpload).not.toHaveBeenCalled();
+      expect(immichSaveSettings).toHaveBeenCalledWith(7, undefined, undefined, '1.2.3.4', undefined, undefined);
     });
 
     it('hands the self-signed switch to the save both ways (#2475)', async () => {
@@ -270,7 +268,7 @@ describe('ImmichMemoriesController (parity with /api/integrations/memories/immic
       const immichSaveSettings = vi.fn().mockResolvedValue({ success: true });
       const svc = makeService({ immichSaveSettings });
       await new ImmichMemoriesController(svc).putSettings(user, { immich_url: 'x', immich_api_key: 'k' }, req, makeRes());
-      expect(immichSaveSettings).toHaveBeenCalledWith(7, 'x', 'k', '1.2.3.4', undefined);
+      expect(immichSaveSettings).toHaveBeenCalledWith(7, 'x', 'k', '1.2.3.4', undefined, undefined);
     });
 
     it('returns the warning when the save carries one', async () => {

server/tests/unit/nest/memories.service.test.ts

@@ -121,10 +121,10 @@ describe('MemoriesService (delegation wrapper over services/memories/*)', () =>
     expect(immich.getConnectionSettings).toHaveBeenCalledWith(7);
 
     await svc.immichSaveSettings(7, 'u', 'k', '1.2.3.4', true);
-    expect(immich.saveImmichSettings).toHaveBeenCalledWith(7, 'u', 'k', '1.2.3.4', true);
+    expect(immich.saveImmichSettings).toHaveBeenCalledWith(7, 'u', 'k', '1.2.3.4', true, undefined);
 
-    await svc.immichSetAutoUpload(7, true);
-    expect(immich.setImmichAutoUpload).toHaveBeenCalledWith(7, true);
+    await svc.immichSaveSettings(7, 'u', 'k', '1.2.3.4', undefined, false);
+    expect(immich.saveImmichSettings).toHaveBeenLastCalledWith(7, 'u', 'k', '1.2.3.4', undefined, false);
 
     await svc.immichGetConnectionStatus(7);
     expect(immich.getConnectionStatus).toHaveBeenCalledWith(7);