From a908fdf68b75f8aed18c3aca9cb172f76b5a0a2c Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 17:09:33 -0700 Subject: [PATCH] Coordinate admin hard delete with pending self-deletion Make the final admin hard delete conditional on the deletion marker being absent, with409ACCOUNT_CHANGED on a missed predicate. Preserve the global admin lock, actor recheck/recount, hosted connection refusal and beginWrite unknown-outcome retention. A self-deletion claim after the admin target read now wins atomically; admin-first makes self-deletion refuse before Stripe. Old handlers reproduce admin200/Usergone/File1/receipt0 during billing failure, plus target-read-then-marker deletion. Both new overlap controls fail when only the atomic marker predicate is removed; admin-first passes. Billing failure now leaves a recoverable user/uploads/active credential, and a fresh reauthenticated retry completes cleanup. Default focused74/74; realMongo7/PG16 final54/54 after switching the service fixture to one listener. Initial real50/54 had unexpected404/session outcomes; final post-mutant default38 assertions passed but the suite exited1 from a MongoMemoryServer stdout JSON parser error. Do not call that suite green. Source tsc, targeted TS lint and diffcheck pass. Broader legacy admin cleanup is unchanged: admin-first still deletes only the human, not uploads/keys. Written by Sprint Impl, a Commonly agent https://commonly.me/v2/pods/6addd4d47f8fc798a6f66bbb#message-87661 Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- .../__tests__/service/accountDeletion.test.js | 84 ++++++++++++++++++- .../service/adminPauseLeavesHostedRow.test.js | 15 ++-- .../service/adminRoleConcurrency.test.js | 2 +- .../__tests__/unit/routes/admin.users.test.js | 10 ++- backend/routes/admin/users.ts | 13 ++- docs/api/account-deletion.md | 25 ++++-- 6 files changed, 132 insertions(+), 17 deletions(-) diff --git a/backend/__tests__/service/accountDeletion.test.js b/backend/__tests__/service/accountDeletion.test.js index a177873d71..a45cecb2eb 100644 --- a/backend/__tests__/service/accountDeletion.test.js +++ b/backend/__tests__/service/accountDeletion.test.js @@ -63,9 +63,12 @@ describe('Self-serve account deletion', () => { recovery.get('/profile', probeLimit, auth, (_req, res) => res.sendStatus(204)); app.use('/api/auth', recovery); app.get('/runtime-probe', probeLimit, agentRuntimeAuth, (_req, res) => res.sendStatus(204)); + // Reuse one listener across the counted loops and nested requests instead + // of allocating a fresh ephemeral port for every supertest request. + app = app.listen(0); }); afterEach(async () => { await clearMongoDb(); await clearPgDb(); await mockPg.query('DELETE FROM users'); }); - afterAll(async () => { await closeMongoDb(); await closePgDb(); }); + afterAll(async () => { await new Promise((resolve) => app.close(resolve)); await closeMongoDb(); await closePgDb(); }); it('deletes own bytes and legacy uploads, revokes credentials, preserves agents and other users', async () => { const deviceToken = 'cm_delete-device'; @@ -443,6 +446,85 @@ describe('Self-serve account deletion', () => { expect(await Lock.countDocuments()).toBe(0); }); + it('keeps a billing-failed self-deletion recoverable when admin hard delete races its claim', async () => { + const admin = await makeUser({ role: 'admin' }); + const user = await makeUser({ billing: { subscriptionId: 'sub-admin-race', subscriptionStatus: 'active' } }); + const token = issueSessionToken(user); + const file = await addFile(user, 'admin-race-legacy.png', true); + const credential = await AgentCredential.create({ tokenHash: 'admin-race-key', kind: 'runtime', ownerUserId: user._id }); + let adminResult; + mockStripe.subscriptions.cancel.mockImplementationOnce(async () => { + adminResult = await request(app).delete(`/api/admin/users/${user.id}`) + .set('Authorization', `Bearer ${issueSessionToken(admin)}`); + throw new Error('Stripe unavailable before receipt'); + }); + await deletion(app, token).expect(503); + expect({ status: adminResult.status, userRetained: !!(await User.findById(user.id)), + files: await File.countDocuments({ uploadedBy: user.id }), receipts: await AccountDeletion.countDocuments() }) + .toEqual({ status: 409, userRetained: true, files: 1, receipts: 0 }); + expect(adminResult.body.code).toBe('ACCOUNT_CHANGED'); + const retained = await User.findById(user.id); + expect(retained).not.toBeNull(); + expect(retained.accountDeletionRequestedAt).toBeUndefined(); + expect(await AccountDeletion.countDocuments()).toBe(0); + expect(await File.findById(file.id)).not.toBeNull(); + expect((await AgentCredential.findById(credential.id)).status).toBe('active'); + expect(await require('../../models/AdminUserMutationLock').countDocuments()).toBe(0); + await deletion(app, token).expect(204); + expect(await User.findById(user.id)).toBeNull(); + expect(await File.findById(file.id)).toBeNull(); + expect((await AgentCredential.findById(credential.id)).status).toBe('revoked'); + }); + + it('refuses hard delete when the deletion marker appears after the admin target read', async () => { + const admin = await makeUser({ role: 'admin' }); const user = await makeUser(); + const Integration = require('../../models/Integration'); + const find = Integration.find.bind(Integration); + let nestedStatus; let claimed = false; + jest.spyOn(Integration, 'find').mockImplementation((filter) => { + const query = find(filter); + if (!claimed && String(filter.createdBy) === user.id) { + claimed = true; + return { select: (fields) => ({ lean: async () => { + jest.spyOn(mockPg, 'query').mockRejectedValueOnce(new Error('PG unavailable')); + nestedStatus = (await deletion(app, issueSessionToken(user))).status; + return query.select(fields).lean(); + } }) }; + } + return query; + }); + const result = await request(app).delete(`/api/admin/users/${user.id}`) + .set('Authorization', `Bearer ${issueSessionToken(admin)}`); + expect(nestedStatus).toBe(503); + expect(result.status).toBe(409); + expect(result.body.code).toBe('ACCOUNT_CHANGED'); + expect((await User.findById(user.id)).accountDeletionRequestedAt).toBeTruthy(); + expect(await AccountDeletion.exists({ _id: user.id })).toBeTruthy(); + expect(await require('../../models/AdminUserMutationLock').countDocuments()).toBe(0); + }); + + it('refuses a self-deletion claim when admin hard delete wins first', async () => { + const admin = await makeUser({ role: 'admin' }); + const user = await makeUser({ billing: { subscriptionId: 'sub-admin-first', subscriptionStatus: 'active' } }); + const update = User.updateOne.bind(User); + let adminResult; let deleted = false; + jest.spyOn(User, 'updateOne').mockImplementation((filter, mutation, ...rest) => { + if (!deleted && mutation.$set?.accountDeletionRequestedAt) return (async () => { + deleted = true; + adminResult = await request(app).delete(`/api/admin/users/${user.id}`) + .set('Authorization', `Bearer ${issueSessionToken(admin)}`); + return update(filter, mutation, ...rest); + })(); + return update(filter, mutation, ...rest); + }); + mockStripe.subscriptions.cancel.mockClear(); + await deletion(app, issueSessionToken(user)).expect(409); + expect(adminResult.status).toBe(200); + expect(await User.findById(user.id)).toBeNull(); + expect(mockStripe.subscriptions.cancel).not.toHaveBeenCalled(); + expect(await AccountDeletion.countDocuments()).toBe(0); + }); + it('refuses an admin role change after deletion has marked the account', async () => { const admin = await makeUser({ role: 'admin' }); const user = await makeUser({ accountDeletionRequestedAt: new Date() }); diff --git a/backend/__tests__/service/adminPauseLeavesHostedRow.test.js b/backend/__tests__/service/adminPauseLeavesHostedRow.test.js index 8c37b195bd..5aa1fa1066 100644 --- a/backend/__tests__/service/adminPauseLeavesHostedRow.test.js +++ b/backend/__tests__/service/adminPauseLeavesHostedRow.test.js @@ -37,7 +37,7 @@ jest.mock('../../middleware/auth', () => (req, _res, next) => { }); jest.mock('../../middleware/adminAuth', () => (_req, _res, next) => next()); -jest.mock('../../models/User', () => ({ findById: jest.fn(), countDocuments: jest.fn() })); +jest.mock('../../models/User', () => ({ findById: jest.fn(), findOneAndDelete: jest.fn(), countDocuments: jest.fn() })); jest.mock('../../services/emailService', () => ({ sendEmail: jest.fn() })); const Integration = require('../../models/Integration'); @@ -201,7 +201,6 @@ describe('the admin user delete refuses while the user owns a hosted row (wren 7 _id: new mongoose.Types.ObjectId(), isBot: false, role: 'user', - deleteOne: jest.fn().mockResolvedValue({}), }); const hostedFor = (userId, overrides = {}) => ({ type: 'hosted-mcp', @@ -215,6 +214,7 @@ describe('the admin user delete refuses while the user owns a hosted row (wren 7 it("refuses with a code and each row's id and entry, and does not delete the person", async () => { const target = person(); User.findById.mockResolvedValue(target); + User.findOneAndDelete.mockResolvedValue(target); const live = await Integration.create(hostedFor(target._id)); // The case the predicate exists for (vera 75850): the provider step failed, // so the row sits at step 2 of the removal — `disconnected`, its material @@ -239,18 +239,19 @@ describe('the admin user delete refuses while the user owns a hosted row (wren 7 // removal route takes: an admin who gets only ids has to look each one up. expect(res.body.connections.map((row) => row.entryId).sort()).toEqual(['linear', 'notion']); // The control: the refusal is what stopped it, not a failed lookup. - expect(target.deleteOne).not.toHaveBeenCalled(); + expect(User.findOneAndDelete).not.toHaveBeenCalled(); expect(await Integration.countDocuments({ createdBy: target._id })).toBe(2); }); it('accepts the delete for a person who owns no hosted row', async () => { const target = person(); User.findById.mockResolvedValue(target); + User.findOneAndDelete.mockResolvedValue(target); const res = await request(app).delete(`/admin/users/${target._id}`); expect(res.status).toBe(200); - expect(target.deleteOne).toHaveBeenCalledTimes(1); + expect(User.findOneAndDelete).toHaveBeenCalledTimes(1); }); it('refuses for a row that is mid-exchange, which carries no ref yet', async () => { @@ -265,6 +266,7 @@ describe('the admin user delete refuses while the user owns a hosted row (wren 7 // it is there, so the connect lands into an ownerless row. const target = person(); User.findById.mockResolvedValue(target); + User.findOneAndDelete.mockResolvedValue(target); const midExchange = await Integration.create(hostedFor(target._id, { status: 'pending', config: { entryId: 'linear', intake: 'oauth' }, @@ -275,7 +277,7 @@ describe('the admin user delete refuses while the user owns a hosted row (wren 7 expect(res.status).toBe(409); expect(res.body.code).toBe('hosted_mcp_connection_owned'); expect(res.body.connections).toEqual([{ id: String(midExchange._id), entryId: 'linear' }]); - expect(target.deleteOne).not.toHaveBeenCalled(); + expect(User.findOneAndDelete).not.toHaveBeenCalled(); }); it('does not refuse for a row that is not a hosted one', async () => { @@ -285,6 +287,7 @@ describe('the admin user delete refuses while the user owns a hosted row (wren 7 // Integration turns this arm red, which is what keeps it honest. const target = person(); User.findById.mockResolvedValue(target); + User.findOneAndDelete.mockResolvedValue(target); await Integration.create({ type: 'github-app', scope: 'user', @@ -296,6 +299,6 @@ describe('the admin user delete refuses while the user owns a hosted row (wren 7 const res = await request(app).delete(`/admin/users/${target._id}`); expect(res.status).toBe(200); - expect(target.deleteOne).toHaveBeenCalledTimes(1); + expect(User.findOneAndDelete).toHaveBeenCalledTimes(1); }); }); diff --git a/backend/__tests__/service/adminRoleConcurrency.test.js b/backend/__tests__/service/adminRoleConcurrency.test.js index dc721d9f35..0317ffcc0a 100644 --- a/backend/__tests__/service/adminRoleConcurrency.test.js +++ b/backend/__tests__/service/adminRoleConcurrency.test.js @@ -101,7 +101,7 @@ describe('Administrator role concurrency', () => { const Lock = require('../../models/AdminUserMutationLock'); if (kind === 'ban') await User.updateOne({ _id: bob._id }, { $set: { role: 'user' } }); const write = (kind === 'role' ? jest.spyOn(User, 'findOneAndUpdate') - : jest.spyOn(User.prototype, kind === 'delete' ? 'deleteOne' : 'save')) + : kind === 'delete' ? jest.spyOn(User, 'findOneAndDelete') : jest.spyOn(User.prototype, 'save')) .mockRejectedValueOnce(new Error('write outcome unknown')); try { const pending = kind === 'delete' ? request(app).delete(`/api/admin/users/${bob._id}`) diff --git a/backend/__tests__/unit/routes/admin.users.test.js b/backend/__tests__/unit/routes/admin.users.test.js index 6c9986f45a..2a0a84f899 100644 --- a/backend/__tests__/unit/routes/admin.users.test.js +++ b/backend/__tests__/unit/routes/admin.users.test.js @@ -16,6 +16,7 @@ jest.mock('../../../models/User', () => ({ find: jest.fn(), findById: jest.fn(), findOneAndUpdate: jest.fn(), + findOneAndDelete: jest.fn(), countDocuments: jest.fn(), })); @@ -121,13 +122,15 @@ describe('admin users routes', () => { _id: '000000000000000000000002', role: 'user', isBot: false, - deleteOne: jest.fn().mockResolvedValue(true), }; User.findById.mockResolvedValue(target); + User.findOneAndDelete.mockResolvedValue(target); await handler(req, res); - expect(target.deleteOne).toHaveBeenCalledTimes(1); + expect(User.findOneAndDelete).toHaveBeenCalledWith({ + _id: target._id, accountDeletionRequestedAt: { $exists: false }, + }); expect(res.json).toHaveBeenCalledWith({ message: 'User deleted successfully' }); }); @@ -148,6 +151,7 @@ describe('admin users routes', () => { save: jest.fn().mockResolvedValue(true), }; User.findById.mockResolvedValue(target); + User.findOneAndDelete.mockResolvedValue(target); User.findOneAndUpdate.mockResolvedValue({ ...target, role: 'admin' }); await handler(req, res); @@ -368,6 +372,7 @@ describe('admin users routes', () => { const save = jest.fn().mockResolvedValue(undefined); const target = { _id: '000000000000000000000002', username: 'mallory', role: 'user', isBot: false, save }; User.findById.mockResolvedValue(target); + User.findOneAndDelete.mockResolvedValue(target); const req = { params: { userId: '000000000000000000000002' }, body: { banned: true, reason: 'spam' }, user: { id: 'admin1' } }; const res = createRes(); @@ -387,6 +392,7 @@ describe('admin users routes', () => { const save = jest.fn().mockResolvedValue(undefined); const target = { _id: '000000000000000000000002', username: 'mallory', role: 'user', isBot: false, banned: true, banReason: 'spam', save }; User.findById.mockResolvedValue(target); + User.findOneAndDelete.mockResolvedValue(target); const req = { params: { userId: '000000000000000000000002' }, body: { banned: false }, user: { id: 'admin1' } }; const res = createRes(); diff --git a/backend/routes/admin/users.ts b/backend/routes/admin/users.ts index 3e2ed22294..81485a1d43 100644 --- a/backend/routes/admin/users.ts +++ b/backend/routes/admin/users.ts @@ -386,8 +386,19 @@ router.delete('/:userId', adminWriteLimiter, auth, adminAuth, async (req: any, r }); } + // Self-deletion claims do not take the admin lock. Coordinate on the + // target document itself so a claim appearing after the reads above + // cannot lose its recoverable owner during billing preflight or cleanup. beginWrite(); - await target.deleteOne(); + const deleted = await User.findOneAndDelete({ + _id: target._id, accountDeletionRequestedAt: { $exists: false }, + }); + if (!deleted) { + return mutationResponse(409, { + error: 'Account changed or self-deletion is pending. Refresh before trying again.', + code: 'ACCOUNT_CHANGED', + }); + } return mutationResponse(200, { message: 'User deleted successfully' }); }); return res.status(result.status).json(result.body); diff --git a/docs/api/account-deletion.md b/docs/api/account-deletion.md index aaf8f10341..ea9388884c 100644 --- a/docs/api/account-deletion.md +++ b/docs/api/account-deletion.md @@ -172,9 +172,22 @@ See [the lock runbook](../runbooks/admin-user-mutation-lock.md) for uncertain-wr recovery and the mixed-version rollout constraint. Synthetic purge DM cleanup remains a separate TASK-005 follow-up. -The pre-existing administrative hard-delete endpoint remains separate from this -lifecycle. Its `target.deleteOne()` does not condition on the self-deletion marker, -so the global admin lock alone does not coordinate it with an in-flight -self-deletion claim. TASK-006 tracks a controlled reproduction and atomic -coordination/recoverable cleanup; this PR does not fix that accepted nonblocking -finding. It is distinct from TASK-005's synthetic-purge DM cleanup. +Administrative hard delete remains a separate lifecycle: it does not become the +self-serve cleanup flow. TASK-006 makes its final `findOneAndDelete` conditional +on `accountDeletionRequestedAt` being absent. The admin lock still serializes +administrator writers, rechecks the actor and counts administrators; the target +predicate additionally coordinates with self-deletion, which does not take that +lock. If a claim wins before the hard delete, the admin returns 409 `ACCOUNT_CHANGED` +and retains the owner through billing preflight or retryable cleanup. This also +covers a marker written after the admin read its target. A stale marker still +refuses until the self-deletion recovery path resolves it. + +If admin hard delete wins first, the self-deletion claim matches no user and +returns 409 before Stripe or receipt creation. The pre-existing administrative +hard-delete behavior still removes only the human row and does not clean uploads +or credentials; this change does not claim to fix that broader lifecycle. A +billing-preflight failure after a refused admin delete leaves the human and their +uploads/credentials intact, releases only its own claim and allows a fresh +reauthenticated retry. Hosted-connection refusal and unknown-write lock retention +remain. The real-handler service cases exercise these orders and retry recovery. +This is distinct from TASK-005's synthetic-purge DM cleanup.