diff --git a/backend/__tests__/unit/routes/tasksApi.taskGrantClose.test.js b/backend/__tests__/unit/routes/tasksApi.taskGrantClose.test.js index aeaa5f44ed..5967845f1b 100644 --- a/backend/__tests__/unit/routes/tasksApi.taskGrantClose.test.js +++ b/backend/__tests__/unit/routes/tasksApi.taskGrantClose.test.js @@ -89,7 +89,7 @@ test.each(['complete', 'patch', 'github', 'onboarding'])('%s close revokes the b }); test('a failed revocation write leaves the close committed and the grant refused by its epoch', async () => { - const spy = jest.spyOn(RoomGrant, 'updateMany').mockRejectedValueOnce(new Error('revocation write failed')); + const spy = jest.spyOn(RoomGrant, 'findOneAndUpdate').mockRejectedValueOnce(new Error('revocation write failed')); expect((await request(app).post(`/tasks/${POD}/TASK-1/complete`).send({})).status).toBe(200); expect(spy).toHaveBeenCalledTimes(1); spy.mockRestore(); diff --git a/backend/__tests__/unit/services/roomGrantRecord.test.js b/backend/__tests__/unit/services/roomGrantRecord.test.js index 12e134b174..c0ad71d726 100644 --- a/backend/__tests__/unit/services/roomGrantRecord.test.js +++ b/backend/__tests__/unit/services/roomGrantRecord.test.js @@ -168,3 +168,74 @@ test('a pod outside RECORD_V1_PODS gets no lines, and its grants still revoke', expect(mockChains.size).toBe(0); expect((await mongoose.model('RoomGrant').findById(grant._id)).revocationReason).toBe('task.done'); }); + +// TASK-333: a mint racing a close. Only Date is faked, so both writers can be +// given one timestamp; the driver's timers and hrtime stay real. +const DATE_ONLY = { + doNotFake: ['hrtime', 'nextTick', 'performance', 'queueMicrotask', 'requestAnimationFrame', 'cancelAnimationFrame', + 'requestIdleCallback', 'cancelIdleCallback', 'setImmediate', 'clearImmediate', 'setInterval', 'clearInterval', + 'setTimeout', 'clearTimeout'], +}; +const RoomGrant = () => mongoose.model('RoomGrant'); +const raceCreate = (between) => { + const realCreate = RoomGrant().create.bind(RoomGrant()); + return jest.spyOn(RoomGrant(), 'create').mockImplementationOnce(async (doc) => { + const created = await realCreate(doc); + await between(created); + return created; + }); +}; + +test('a grant a refused mint revoked in the same millisecond is not named by the close', async () => { + let live; + jest.useFakeTimers({ ...DATE_ONLY, now: Date.now() }); + try { + live = await service.mintGrant(input()); + const spy = raceCreate(() => Task.updateOne({ podId: POD, taskId: 'TASK-1' }, { $inc: { grantEpoch: 1 } })); + await expect(service.mintGrant(input())).rejects.toMatchObject({ code: 'task_closed' }); + spy.mockRestore(); + await close(); + } finally { + jest.useRealTimers(); + } + const revoked = (await chainLines()).filter((l) => l.kind === 'grant.revoked'); + expect(revoked.map((l) => l.data.grantIds)).toEqual([[live.grantId]]); +}); + +test('a refused mint the close already revoked keeps the close\'s revocation and stays named', async () => { + let closedAt; + jest.useFakeTimers({ ...DATE_ONLY, now: Date.now() }); + try { + const spy = raceCreate(async (created) => { + await close(); + closedAt = (await RoomGrant().findById(created._id)).revokedAt; + jest.setSystemTime(Date.now() + 1000); + }); + await expect(service.mintGrant(input())).rejects.toMatchObject({ code: 'task_closed' }); + spy.mockRestore(); + } finally { + jest.useRealTimers(); + } + const [grant] = await RoomGrant().find({}).lean(); + expect(closedAt).toBeInstanceOf(Date); + expect(grant.revokedAt).toEqual(closedAt); + expect(grant.revocationReason).toBe('task.done'); + const revoked = (await chainLines()).filter((l) => l.kind === 'grant.revoked'); + expect(revoked.map((l) => l.data.grantIds)).toEqual([[grant.grantId]]); +}); + +test('a close whose write fails for one grant still names the grants it did revoke', async () => { + const grants = [await service.mintGrant(input()), await service.mintGrant(input())]; + const spy = jest.spyOn(RoomGrant(), 'findOneAndUpdate').mockRejectedValueOnce(new Error('revocation write failed')); + const logged = jest.spyOn(console, 'error').mockImplementation(() => {}); + await close(); + expect(spy).toHaveBeenCalledTimes(2); + spy.mockRestore(); + expect(logged).toHaveBeenCalledTimes(1); + logged.mockRestore(); + const rows = await RoomGrant().find({ grantId: { $in: grants.map((g) => g.grantId) } }).lean(); + const revokedIds = rows.filter((row) => row.revokedAt).map((row) => row.grantId); + expect(revokedIds).toHaveLength(1); + const revoked = (await chainLines()).filter((l) => l.kind === 'grant.revoked'); + expect(revoked.map((l) => l.data.grantIds)).toEqual([revokedIds]); +}); diff --git a/backend/services/roomGrantService.ts b/backend/services/roomGrantService.ts index 7de0c3fce1..864733dd16 100644 --- a/backend/services/roomGrantService.ts +++ b/backend/services/roomGrantService.ts @@ -240,8 +240,10 @@ const assertGrantLineageActive = async ( return lineage; }; +// Guarded on `revokedAt: null`: a close that already revoked this grant keeps +// its own timestamp and reason, so the close's record line still names it. const markMintedGrantRejected = async (grantId: string, error: RoomGrantError): Promise => { - await RoomGrant.updateOne({ grantId }, { $set: { revokedAt: new Date(), + await RoomGrant.updateOne({ grantId, revokedAt: null }, { $set: { revokedAt: new Date(), ...(error.code === 'task_closed' ? { revocationReason: 'task.done' } : {}), } }); throw error; @@ -346,9 +348,11 @@ const assertTaskContext = async (grant: IRoomGrant | Record, op /** * Hook successful close transitions. An old close never revokes new grants issued after reopen. - * updateMany reports a count, not ids, so the record line reads back the grants - * carrying this write's own `revokedAt`. A grant an earlier close or another - * writer revoked keeps its own timestamp and is not named again. + * Each grant is revoked by its own guarded write, and the record line names + * exactly the grants those writes matched. Reading back by `revokedAt` instead + * would also name a grant a refused mint revoked in the same millisecond with + * the same reason. A grant an earlier close or another writer revoked keeps its + * own timestamp and is not named again. */ export const revokeTaskGrants = async ({ taskId, podId, closedEpoch }: Pick & { closedEpoch: number }): Promise => { @@ -356,21 +360,28 @@ export const revokeTaskGrants = async ({ taskId, podId, closedEpoch }: Pick 0) { - const revoked = await RoomGrant.find({ - 'binding.taskId': taskId, 'binding.podId': podId, revokedAt, revocationReason: 'task.done', - }).select('grantId').lean(); - const grantIds = Array.from(new Set(revoked.map((grant) => String(grant.grantId)))); - // ADR-033 §4: the close's own task.done line is queued first, by the caller. - if (grantIds.length) { - emitRecord({ podId, taskId }, `grant.revoked/task.done/${closedEpoch}`, [ - { kind: 'grant.revoked', actor: BOARD_ACTOR, data: { grantIds, reason: 'task.done' } }, - ]); - } - } - return result.modifiedCount; + // allSettled, not all: a write that committed stays revoked even when a + // sibling write fails, and no later close can name it (its filter needs + // `revokedAt: null`), so the line is written for every match before the + // failure is rethrown to the caller's log. + const settled = await Promise.allSettled(candidates.map(({ grantId }) => RoomGrant.findOneAndUpdate( + { ...filter, grantId }, + { $set: { revokedAt, revocationReason: 'task.done' } }, + { projection: { grantId: 1 }, lean: true }, + ))); + const grantIds = Array.from(new Set(settled.flatMap((result) => ( + result.status === 'fulfilled' && result.value ? [String(result.value.grantId)] : [])))); + // ADR-033 §4: the close's own task.done line is queued first, by the caller. + if (grantIds.length) { + emitRecord({ podId, taskId }, `grant.revoked/task.done/${closedEpoch}`, [ + { kind: 'grant.revoked', actor: BOARD_ACTOR, data: { grantIds, reason: 'task.done' } }, + ]); + } + const failure = settled.find((result): result is PromiseRejectedResult => result.status === 'rejected'); + if (failure) throw failure.reason; + return grantIds.length; }; /**