refactor(airtrail): save the settings in one transaction

The address and the key it may no longer carry now change together, as
they do for Dawarich, so the transaction ratchet drops the AirTrail entry.
The tests that pinned the old no-transaction shape now pin the new one.

server/scripts/tx-writes-baseline.json

@@ -28,7 +28,6 @@
   "src/nest/integrations/airtrail-link.service.ts#AirtrailLinkService.pushReservationToAirtrail": 2,
   "src/nest/integrations/airtrail-sync.service.ts#AirtrailSyncService.runAirtrailSync": 2,
   "src/nest/integrations/airtrail-sync.service.ts#AirtrailSyncService.syncOwner": 8,
-  "src/nest/integrations/airtrail.service.ts#AirtrailService.saveSettings": 2,
   "src/nest/integrations/dawarich-sync.service.ts#DawarichSyncService.runSync": 2,
   "src/nest/integrations/dawarich-sync.service.ts#DawarichSyncService.syncTripWindow": 3,
   "src/nest/integrations/dawarich-sync.service.ts#DawarichSyncService.syncUserOnce": 3,

server/src/nest/integrations/airtrail.service.ts

@@ -2,6 +2,7 @@ import { UserAirtrailRepository } from '../../db/repositories/UserAirtrail.repos
 import { checkSsrf } from '../../utils/ssrfGuard';
 import { AuditService } from '../audit/audit.service';
 import { maybe_encrypt_api_key, decrypt_api_key } from '../common/crypto/apiKeyCrypto';
+import { UnitOfWork } from '../database/unit-of-work';
 import { AirtrailAuthError, AirtrailRequestError, type AirtrailCreds } from './airtrail.client';
 import { AirtrailClient } from './airtrail.client';
 import { normalizeFlight } from './airtrail.mapper';
@@ -34,10 +35,9 @@ interface UserConnRow {
  * `UserAirtrailRepository`'s (same `users` table) `getAirtrailConnRow`/`getAirtrailWriteEnabled`/
  * `setAirtrailSettingsWithKey`/`setAirtrailSettings`/`clearAirtrailApiKey`
  * methods (R7 — the repository sees only opaque, already-encrypted TEXT;
- * `maybe_encrypt_api_key`/`decrypt_api_key` stay here). `saveSettings` keeps
- * its pre-existing asymmetry with `DawarichService#saveSettings`
- * (Task 3): Dawarich wraps its writes in `uow.transactional`, this one does
- * not — preserved exactly, not "fixed" to match.
+ * `maybe_encrypt_api_key`/`decrypt_api_key` stay here). `saveSettings` writes
+ * in one `uow.transactional`, as `DawarichService#saveSettings` does: the new
+ * address and the key it may no longer carry change together or not at all.
  */
 @Injectable()
 export class AirtrailService {
@@ -45,6 +45,7 @@ export class AirtrailService {
     private readonly airtrailRepo: UserAirtrailRepository,
     private readonly audit: AuditService,
     private readonly client: AirtrailClient,
+    private readonly uow: UnitOfWork,
   ) {}
 
   private async readRow(userId: number): Promise<UserConnRow | undefined> {
@@ -122,34 +123,32 @@ export class AirtrailService {
     // A key belongs to the instance it was issued on, as in testConnection. A
     // new host with the key field left masked or blank would otherwise send the
     // old instance's key to the new one on the next sync, so the key goes with
-    // the host. A path edit on the same origin keeps it. The key is cleared
-    // before the new address is written, so no moment stores the new address
-    // beside the old key.
-    if (newKey === undefined) {
-      const previousUrl = (await this.readRow(userId))?.airtrail_url ?? null;
-      if (movedHost(previousUrl, trimmedUrl)) await this.airtrailRepo.clearAirtrailApiKey(userId);
-    }
+    // the host. A path edit on the same origin keeps it. An address cleared with
+    // no key left drops the key too: it would be a credential for no server.
+    const previousUrl = newKey === undefined ? ((await this.readRow(userId))?.airtrail_url ?? null) : null;
+    const dropKey = newKey === undefined && (!trimmedUrl || movedHost(previousUrl, trimmedUrl));
 
-    if (newKey !== undefined) {
-      await this.airtrailRepo.setAirtrailSettingsWithKey(
-        userId,
-        trimmedUrl || null,
-        newKey,
-        allowInsecureTls ? 1 : 0,
-        writeEnabled ? 1 : 0,
-      );
-    } else {
+    // The address and the key it may no longer carry change together, so no
+    // moment stores the new address beside the old key.
+    await this.uow.transactional(async () => {
+      if (newKey !== undefined) {
+        await this.airtrailRepo.setAirtrailSettingsWithKey(
+          userId,
+          trimmedUrl || null,
+          newKey,
+          allowInsecureTls ? 1 : 0,
+          writeEnabled ? 1 : 0,
+        );
+        return;
+      }
       await this.airtrailRepo.setAirtrailSettings(
         userId,
         trimmedUrl || null,
         allowInsecureTls ? 1 : 0,
         writeEnabled ? 1 : 0,
       );
-      // Clearing the URL with no key left makes the connection meaningless — drop the key too.
-      if (!trimmedUrl) {
-        await this.airtrailRepo.clearAirtrailApiKey(userId);
-      }
-    }
+      if (dropKey) await this.airtrailRepo.clearAirtrailApiKey(userId);
+    });
 
     return { success: true, warning };
   }

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

@@ -17,6 +17,7 @@ import { Users } from '../../../src/db/entities/Users.entity';
 import { UserAirtrailRepository } from '../../../src/db/repositories/UserAirtrail.repository';
 import type { UsersRepository } from '../../../src/db/repositories/Users.repository';
 import { AuditService } from '../../../src/nest/audit/audit.service';
+import { UnitOfWork } from '../../../src/nest/database/unit-of-work';
 import type { AirtrailClient } from '../../../src/nest/integrations/airtrail.client';
 import { AirtrailService } from '../../../src/nest/integrations/airtrail.service';
 import { createSnapshotTestDb } from '../../helpers/db-mock';
@@ -42,9 +43,14 @@ const listFlights = vi.fn();
 beforeAll(async () => {
   t = await createTestOrm(testDb);
   usersRepo = t.repo(Users);
-  svc = new AirtrailService(new UserAirtrailRepository(t.orm.em), new AuditService(t.repo(AuditLog), usersRepo), {
-    listFlights,
-  } as unknown as AirtrailClient);
+  svc = new AirtrailService(
+    new UserAirtrailRepository(t.orm.em),
+    new AuditService(t.repo(AuditLog), usersRepo),
+    {
+      listFlights,
+    } as unknown as AirtrailClient,
+    new UnitOfWork(t.orm.em),
+  );
 });
 beforeEach(() => {
   resetTestDb(testDb);

server/tests/unit/nest/integrations/airtrailTestConnection.test.ts

@@ -3,6 +3,7 @@ import { AuditLog } from '../../../../src/db/entities/AuditLog.entity';
 import { Users } from '../../../../src/db/entities/Users.entity';
 import { UserAirtrailRepository } from '../../../../src/db/repositories/UserAirtrail.repository';
 import { AuditService } from '../../../../src/nest/audit/audit.service';
+import { UnitOfWork } from '../../../../src/nest/database/unit-of-work';
 import type { AirtrailClient } from '../../../../src/nest/integrations/airtrail.client';
 import { AirtrailService } from '../../../../src/nest/integrations/airtrail.service';
 import { createUser } from '../../../helpers/factories';
@@ -24,9 +25,14 @@ let t: TestOrm;
 
 beforeAll(async () => {
   t = await createTestOrm(db);
-  svc = new AirtrailService(new UserAirtrailRepository(t.orm.em), new AuditService(t.repo(AuditLog), t.repo(Users)), {
-    listFlights,
-  } as unknown as AirtrailClient);
+  svc = new AirtrailService(
+    new UserAirtrailRepository(t.orm.em),
+    new AuditService(t.repo(AuditLog), t.repo(Users)),
+    {
+      listFlights,
+    } as unknown as AirtrailClient,
+    new UnitOfWork(t.orm.em),
+  );
 });
 
 afterAll(async () => {

server/tests/unit/nest/integrations/airtrailWriteback.test.ts

@@ -10,6 +10,7 @@ import { AuditLog } from '../../../../src/db/entities/AuditLog.entity';
 import { Users } from '../../../../src/db/entities/Users.entity';
 import { UserAirtrailRepository } from '../../../../src/db/repositories/UserAirtrail.repository';
 import { AuditService } from '../../../../src/nest/audit/audit.service';
+import { UnitOfWork } from '../../../../src/nest/database/unit-of-work';
 import { AirtrailClient } from '../../../../src/nest/integrations/airtrail.client';
 import { AirtrailService } from '../../../../src/nest/integrations/airtrail.service';
 import { createUser } from '../../../helpers/factories';
@@ -39,6 +40,7 @@ beforeAll(async () => {
     new UserAirtrailRepository(t.orm.em),
     new AuditService(t.repo(AuditLog), t.repo(Users)),
     new AirtrailClient(),
+    new UnitOfWork(t.orm.em),
   );
   getConnectionSettings = (...args) => svc.getConnectionSettings(...args);
   isAirtrailWriteEnabled = (...args) => svc.isAirtrailWriteEnabled(...args);
@@ -72,45 +74,32 @@ describe('airtrail writeback opt-in persistence (#1240)', () => {
   });
 });
 
-describe('airtrail saveSettings preserves its pre-existing NO-transaction asymmetry with Dawarich (Plan 3h Task 4, R7/Risks)', () => {
-  it('ATC-TX-001: AirtrailService is constructed with no UnitOfWork at all — saveSettings has nothing to wrap its writes in', () => {
-    // `DawarichService#saveSettings`'s equivalent DOES take a `UnitOfWork` and
-    // wraps its writes in `uow.transactional`; AirtrailService's own
-    // constructor (`UserAirtrailRepository`, `AuditService`, `AirtrailClient`) never
-    // gained one during the Plan 3h Task 4 conversion — a structural
-    // guarantee that a future edit re-introducing a transactional wrap here
-    // would have to ALSO add a new constructor param to do it, not just
-    // touch `saveSettings`'s body.
+describe('airtrail saveSettings writes in one transaction, as Dawarich does', () => {
+  it('ATC-TX-001: AirtrailService is constructed with a UnitOfWork to wrap its writes in', () => {
     const paramTypes = Reflect.getMetadata('design:paramtypes', AirtrailService) as unknown[] | undefined;
-    expect(paramTypes).toBeDefined();
-    expect(paramTypes!.map((p) => (p as { name?: string })?.name)).toEqual([
+    expect(paramTypes?.map((p) => (p as { name?: string })?.name)).toEqual([
       'UserAirtrailRepository',
       'AuditService',
       'AirtrailClient',
+      'UnitOfWork',
     ]);
   });
 
-  it('ATC-TX-002: the URL-cleared branch performs its two writes as genuinely separate repository calls, not one atomic statement', async () => {
+  it('ATC-TX-002: clearing the URL drops the key inside the same transaction as the address write', async () => {
     const { user } = createUser(db);
     await saveSettings(user.id, 'https://at.example.com', 'secret-key', false, false, null);
 
-    const airtrailRepo = (svc as unknown as { airtrailRepo: UserAirtrailRepository }).airtrailRepo;
-    const setSpy = vi.spyOn(airtrailRepo, 'setAirtrailSettings');
-    const clearSpy = vi.spyOn(airtrailRepo, 'clearAirtrailApiKey');
+    const inner = svc as unknown as { airtrailRepo: UserAirtrailRepository; uow: UnitOfWork };
+    const txSpy = vi.spyOn(inner.uow, 'transactional');
+    const setSpy = vi.spyOn(inner.airtrailRepo, 'setAirtrailSettings');
+    const clearSpy = vi.spyOn(inner.airtrailRepo, 'clearAirtrailApiKey');
     try {
-      // Clearing the URL with no key left drops the key too (ATC4 then ATC5) —
-      // TWO independent nativeUpdate calls, not a single transactional unit.
       await saveSettings(user.id, '', undefined, false, false, null);
+      expect(txSpy).toHaveBeenCalledTimes(1);
       expect(setSpy).toHaveBeenCalledTimes(1);
       expect(clearSpy).toHaveBeenCalledTimes(1);
-      // clearAirtrailApiKey only runs AFTER setAirtrailSettings's own write
-      // resolved (sequential awaits in the service body, never Promise.all
-      // or a single wrapped call) — the same "report, don't fix" asymmetry
-      // the plan's Risks section flags.
-      const setOrder = setSpy.mock.invocationCallOrder[0];
-      const clearOrder = clearSpy.mock.invocationCallOrder[0];
-      expect(clearOrder).toBeGreaterThan(setOrder);
     } finally {
+      txSpy.mockRestore();
       setSpy.mockRestore();
       clearSpy.mockRestore();
     }