From 24f9401545d1bdd3fbf72feb24f4287e494e6cdf Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 31 Oct 2026 02:06:37 -0700 Subject: [PATCH 1/2] fix(grants): a close names only the grants its own write revoked (TASK-333) A mint racing a close could make the close's grant.revoked line wrong in two ways (Otto's note on #2699): - markMintedGrantRejected wrote revokedAt with no revokedAt:null guard, so it could overwrite the revocation a close had just written. - revokeTaskGrants revoked with updateMany and named the grants by reading back its own revokedAt and reason. A refused mint revoked in the same millisecond with reason task.done read back as the close's. The rejection is now guarded on revokedAt:null. The close revokes each candidate with its own guarded findOneAndUpdate and names exactly the rows those writes matched. Two race arms in roomGrantRecord.test.js freeze Date only. Reverting either half of the fix reddens its own arm and nothing else. The close-failure route test now injects its write failure at findOneAndUpdate, since the close no longer calls updateMany. Co-Authored-By: Claude Opus 5.5 Co-authored-by: vera Agent-Model: claude/claude-opus-5-5/default --- .../routes/tasksApi.taskGrantClose.test.js | 2 +- .../unit/services/roomGrantRecord.test.js | 55 +++++++++++++++++++ backend/services/roomGrantService.ts | 40 ++++++++------ 3 files changed, 78 insertions(+), 19 deletions(-) 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..7dfe1cc4b5 100644 --- a/backend/__tests__/unit/services/roomGrantRecord.test.js +++ b/backend/__tests__/unit/services/roomGrantRecord.test.js @@ -168,3 +168,58 @@ 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]]); +}); diff --git a/backend/services/roomGrantService.ts b/backend/services/roomGrantService.ts index 7de0c3fce1..f99b500e63 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,21 @@ 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; + const revoked = await Promise.all(candidates.map(({ grantId }) => RoomGrant.findOneAndUpdate( + { ...filter, grantId }, + { $set: { revokedAt, revocationReason: 'task.done' } }, + { projection: { grantId: 1 }, lean: true }, + ))); + const grantIds = Array.from(new Set(revoked.flatMap((grant) => (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 grantIds.length; }; /** From 6afc00a30d2f6fe27ef4449d32edb7ed316d0874 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 31 Oct 2026 02:23:23 -0700 Subject: [PATCH 2/2] fix(grants): name the grants a close revoked even when a sibling write fails (TASK-333) Promise.all rejected on the first failed per-grant write, so the grant.revoked line never ran, even for writes that had already committed. Those grants stay revoked and no later close can name them, because its filter needs revokedAt:null. The close now uses allSettled, writes the line for every match, then rethrows the failure for onTaskClosed to log. Raised by Rhea's gate and CodeRabbit. New arm: two bound grants, the first write rejects. The close names exactly the one that was revoked and logs once. Going back to Promise.all fails the arm, and so does dropping the rethrow. Co-Authored-By: Claude Opus 5.5 Co-authored-by: vera Agent-Model: claude/claude-opus-5-5/default --- .../unit/services/roomGrantRecord.test.js | 16 ++++++++++++++++ backend/services/roomGrantService.ts | 11 +++++++++-- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/backend/__tests__/unit/services/roomGrantRecord.test.js b/backend/__tests__/unit/services/roomGrantRecord.test.js index 7dfe1cc4b5..c0ad71d726 100644 --- a/backend/__tests__/unit/services/roomGrantRecord.test.js +++ b/backend/__tests__/unit/services/roomGrantRecord.test.js @@ -223,3 +223,19 @@ test('a refused mint the close already revoked keeps the close\'s revocation and 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 f99b500e63..864733dd16 100644 --- a/backend/services/roomGrantService.ts +++ b/backend/services/roomGrantService.ts @@ -362,18 +362,25 @@ export const revokeTaskGrants = async ({ taskId, podId, closedEpoch }: Pick RoomGrant.findOneAndUpdate( + // 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(revoked.flatMap((grant) => (grant ? [String(grant.grantId)] : [])))); + 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; };