diff --git a/server/scripts/tx-writes-baseline.json b/server/scripts/tx-writes-baseline.json index 05dcbee14..8b6101c56 100644 --- a/server/scripts/tx-writes-baseline.json +++ b/server/scripts/tx-writes-baseline.json @@ -8,7 +8,6 @@ "src/nest/auth/passkey.service.ts#PasskeyService.passkeyLoginVerify": 2, "src/nest/auth/passkey.service.ts#PasskeyService.passkeyRegisterOptions": 2, "src/nest/auth/passkey.service.ts#PasskeyService.passkeyRegisterVerify": 2, - "src/nest/auth/user-cleanup.service.ts#UserCleanupService.erasePluginUserData": 2, "src/nest/booking-import/booking-import.service.ts#BookingImportService.confirm": 4, "src/nest/doc-sync/doc-sync-config.service.ts#DocSyncConfigService.upsertConnection": 3, "src/nest/doc-sync/doc-sync.controller.ts#DocSyncController.createLink": 4, @@ -97,7 +96,6 @@ "src/nest/plugins/plugins.controller.ts#PluginsController.update": 2, "src/nest/plugins/plugins.controller.ts#PluginsController.updateConfig": 3, "src/nest/reservations/reservations.controller.ts#ReservationsController.update": 2, - "src/nest/trip-members/trip-members.service.ts#TripMembersService.deleteGuest": 2, "src/nest/trips/trips.service.ts#TripsService.update": 2, "src/nest/vacay/vacay.service.ts#VacayService.updatePlan": 2 } diff --git a/server/src/nest/auth/user-cleanup.service.ts b/server/src/nest/auth/user-cleanup.service.ts index 48aa522ae..651b981d6 100644 --- a/server/src/nest/auth/user-cleanup.service.ts +++ b/server/src/nest/auth/user-cleanup.service.ts @@ -116,22 +116,29 @@ export class UserCleanupService { * here, now via the same repository. */ async erasePluginUserData(userId: number): Promise { - await this.maintenance.deletePluginUserData(userId); // UC1, Plan 4 Task 4 - try { - const rows = await this.pluginsRepo.listIdsAndPermissions(); // UC2 — Plan 4 Task 8a - const installed = new Set(rows.map((r) => r.id)); - await enqueueHookUserDataErasures(this.pluginUserErasureQueueRepo, rows, userId); // UC3 — Plan 4 Task 8a - // Also enqueue for plugins UNINSTALLED with their data retained (deleteData=false): - // their data dir still holds the user's rows and a same-id reinstall would re-adopt - // them. No permissions record survives uninstall, so we can't check hook:user-data — - // enqueue for every orphan data dir; the row sits inert (no FK) and drains only if - // that id is reinstalled + active (erasure delivery is a duty, not grant-gated). + // The delete and the erasures it queues are one write: the host rows never go + // without the queue rows that purge the plugins' own copies. Only DB writes and + // a synchronous readdir run in here, no network I/O. A caller that deletes the + // user (deleteUserCompletely, TripMembersService.deleteGuest) calls this inside + // its own transaction, so this becomes a savepoint of it. + await this.uow.transactional(async () => { + await this.maintenance.deletePluginUserData(userId); // UC1, Plan 4 Task 4 try { - for (const entry of fs.readdirSync(pluginsDataRoot(), { withFileTypes: true })) { - if (entry.isDirectory() && !installed.has(entry.name)) await this.pluginUserErasureQueueRepo.insertIgnore(entry.name, userId); - } - } catch { /* no plugin data root yet */ } - } catch { /* plugins / queue table absent (slim schema) */ } + const rows = await this.pluginsRepo.listIdsAndPermissions(); // UC2, Plan 4 Task 8a + const installed = new Set(rows.map((r) => r.id)); + await enqueueHookUserDataErasures(this.pluginUserErasureQueueRepo, rows, userId); // UC3, Plan 4 Task 8a + // Also enqueue for plugins UNINSTALLED with their data retained (deleteData=false): + // their data dir still holds the user's rows and a same-id reinstall would re-adopt + // them. No permissions record survives uninstall, so we can't check hook:user-data; + // enqueue for every orphan data dir; the row sits inert (no FK) and drains only if + // that id is reinstalled + active (erasure delivery is a duty, not grant-gated). + try { + for (const entry of fs.readdirSync(pluginsDataRoot(), { withFileTypes: true })) { + if (entry.isDirectory() && !installed.has(entry.name)) await this.pluginUserErasureQueueRepo.insertIgnore(entry.name, userId); + } + } catch { /* no plugin data root yet */ } + } catch { /* plugins / queue table absent (slim schema) */ } + }); } /** diff --git a/server/src/nest/trip-members/trip-members.service.ts b/server/src/nest/trip-members/trip-members.service.ts index 055d4597f..8aeadc5d6 100644 --- a/server/src/nest/trip-members/trip-members.service.ts +++ b/server/src/nest/trip-members/trip-members.service.ts @@ -279,15 +279,15 @@ export class TripMembersService { async deleteGuest(tripId: string | number, guestUserId: number): Promise { if (!(await this.guestOfTrip(tripId, guestUserId))) return false; - // A guest is still a user id a plugin may hold data for, so erase that too — the - // host-side per-user tables + a durable own-db erasure per granted plugin — exactly - // like a full account deletion (otherwise a deleted guest's plugin data lingers). - await this.userCleanup.erasePluginUserData(guestUserId); - // Quirk fix on top of the 1:1 move: the budget re-split and the user delete - // run in one transaction, so a failure mid-flow can't leave the expense - // divisors re-derived for a guest that still exists (or vice versa). The - // plugin-side erasure/notification keep their order around it. + // The plugin erasure, the budget re-split and the user delete run in one + // transaction, so a failure mid-flow can't leave the expense divisors re-derived, + // or the guest's plugin rows erased, for a guest that still exists (or vice + // versa). The plugin notification goes out after the commit. await this.uow.transactional(async () => { + // A guest is still a user id a plugin may hold data for, so erase that too (the + // host-side per-user tables plus a durable own-db erasure per granted plugin), exactly + // like a full account deletion; otherwise a deleted guest's plugin data lingers. + await this.userCleanup.erasePluginUserData(guestUserId); // Re-split the expenses they were part of before the cascade takes their member // rows away — the divisor is denormalized and cannot follow a foreign key (#1553). await this.budget.removeUserFromBudgetItems(guestUserId); diff --git a/server/tests/unit/nest/trip-members.service.test.ts b/server/tests/unit/nest/trip-members.service.test.ts index 5ae44fa41..38f3faf6d 100644 --- a/server/tests/unit/nest/trip-members.service.test.ts +++ b/server/tests/unit/nest/trip-members.service.test.ts @@ -430,6 +430,34 @@ describe('Task 6 review items — rollback and concurrency', () => { expect(after).toBe(before); }); + it('MEMBERS-SVC-019: deleteGuest rolls the plugin erasure back with the user delete, so a guest that survives keeps its plugin rows and no erasure is queued', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const { member: guest } = await roster.createGuest(trip.id, 'Rollback Ray', owner.id); + testDb.prepare('INSERT INTO plugins (id, name, version, permissions) VALUES (?, ?, ?, ?)') + .run('members-svc-019', 'members-svc-019', '1.0.0', JSON.stringify(['hook:user-data'])); + testDb.prepare('INSERT INTO plugin_user_config (plugin_id, user_id, config) VALUES (?, ?, ?)') + .run('members-svc-019', guest.id, '{"token":"keep-me"}'); + + const spy = vi.spyOn(usersRepo, 'deleteGuest').mockRejectedValueOnce(new Error('boom')); + try { + await expect(roster.deleteGuest(trip.id, guest.id)).rejects.toThrow('boom'); + + // The erasure ran first, then the users delete rejected. Erasing outside the + // transaction would have left the host rows deleted and an erasure queued for + // a guest that still exists. + expect(testDb.prepare('SELECT id FROM users WHERE id = ?').get(guest.id)).toBeDefined(); + expect(testDb.prepare('SELECT user_id FROM plugin_user_config WHERE user_id = ?').get(guest.id)).toBeDefined(); + expect(testDb.prepare('SELECT plugin_id FROM plugin_user_erasure_queue WHERE user_id = ?').all(guest.id)).toEqual([]); + } finally { + spy.mockRestore(); + // resetTestDb leaves the plugin tables alone, so this test cleans up its own rows. + for (const t of ['plugin_user_erasure_queue', 'plugin_user_config', 'plugins']) { + testDb.prepare(`DELETE FROM ${t} WHERE ${t === 'plugins' ? 'id' : 'plugin_id'} = ?`).run('members-svc-019'); + } + } + }); + it('MEMBERS-SVC-018 (concurrency, §18.4, copies TRIP-JOIN-005): two concurrent addMember calls for the same (trip, user) race the TM5→TM6 check-then-act window', async () => { const { user: owner } = createUser(testDb); const { user: invitee } = createUser(testDb);