fix: address PR review on token throttle PR

Two CodeRabbit findings:

1. Add `revokedAt: null` to the throttled `updateMany` WHERE clause so
   a token revoked between findFirst and updateMany doesn't get a
   stale lastAccessedAt write. Matches the original findFirst guard.

2. Replace the ±50ms time tolerance in the cutoff assertion with
   `vi.useFakeTimers()` + a fixed system time so the assertion is
   exact and won't flake on slow CI runners. Also asserts the new
   `revokedAt: null` predicate is in place.

Author: ericallam

apps/webapp/app/services/organizationAccessToken.server.ts

@@ -113,10 +113,13 @@ export async function authenticateOrganizationAccessToken(
 
   // Conditional updateMany — only writes if the existing lastAccessedAt is
   // null or older than the throttle window. The WHERE runs inside the UPDATE
-  // so concurrent auths don't race into a double-write.
+  // so concurrent auths don't race into a double-write. `revokedAt: null`
+  // matches the findFirst guard above so a token revoked between the read
+  // and write doesn't get a stale lastAccessedAt update.
   await prisma.organizationAccessToken.updateMany({
     where: {
       id: organizationAccessToken.id,
+      revokedAt: null,
       OR: [
         { lastAccessedAt: null },
         { lastAccessedAt: { lt: new Date(Date.now() - OAT_LAST_ACCESSED_THROTTLE_MS) } },

apps/webapp/app/services/personalAccessToken.server.ts

@@ -213,10 +213,13 @@ export async function authenticatePersonalAccessToken(
 
   // Conditional updateMany — only writes if the existing lastAccessedAt is
   // null or older than the throttle window. The WHERE runs inside the UPDATE
-  // so concurrent auths don't race into a double-write.
+  // so concurrent auths don't race into a double-write. `revokedAt: null`
+  // matches the findFirst guard above so a token revoked between the read
+  // and write doesn't get a stale lastAccessedAt update.
   await prisma.personalAccessToken.updateMany({
     where: {
       id: personalAccessToken.id,
+      revokedAt: null,
       OR: [
         { lastAccessedAt: null },
         { lastAccessedAt: { lt: new Date(Date.now() - PAT_LAST_ACCESSED_THROTTLE_MS) } },

apps/webapp/test/services/organizationAccessToken.test.ts

@@ -1,4 +1,4 @@
-import { beforeEach, describe, expect, test, vi } from "vitest";
+import { afterEach, beforeEach, describe, expect, test, vi } from "vitest";
 
 const { findFirstMock, updateManyMock } = vi.hoisted(() => ({
   findFirstMock: vi.fn(),
@@ -29,11 +29,17 @@ import {
 } from "~/services/organizationAccessToken.server";
 
 beforeEach(() => {
+  vi.useFakeTimers();
+  vi.setSystemTime(new Date("2026-01-01T00:00:00.000Z"));
   findFirstMock.mockReset();
   updateManyMock.mockReset();
   updateManyMock.mockResolvedValue({ count: 1 });
 });
 
+afterEach(() => {
+  vi.useRealTimers();
+});
+
 describe("authenticateOrganizationAccessToken — lastAccessedAt throttle", () => {
   test("issues a conditional updateMany that skips writes when lastAccessedAt is recent", async () => {
     findFirstMock.mockResolvedValueOnce({
@@ -42,15 +48,14 @@ describe("authenticateOrganizationAccessToken — lastAccessedAt throttle", () =
       hashedToken: "hashed:tr_oat_validtoken",
     });
 
-    const before = Date.now();
     const result = await authenticateOrganizationAccessToken("tr_oat_validtoken");
-    const after = Date.now();
 
     expect(result).toEqual({ organizationId: "org_1" });
     expect(updateManyMock).toHaveBeenCalledTimes(1);
 
     const call = updateManyMock.mock.calls[0][0];
     expect(call.where.id).toBe("oat_123");
+    expect(call.where.revokedAt).toBeNull();
     expect(call.data.lastAccessedAt).toBeInstanceOf(Date);
 
     // The WHERE clause should require the existing lastAccessedAt to be null
@@ -60,9 +65,9 @@ describe("authenticateOrganizationAccessToken — lastAccessedAt throttle", () =
       { lastAccessedAt: { lt: expect.any(Date) } },
     ]);
 
+    // With fake timers, the cutoff lands exactly throttle-ms before "now".
     const cutoff = call.where.OR[1].lastAccessedAt.lt as Date;
-    expect(cutoff.getTime()).toBeGreaterThanOrEqual(before - OAT_LAST_ACCESSED_THROTTLE_MS - 50);
-    expect(cutoff.getTime()).toBeLessThanOrEqual(after - OAT_LAST_ACCESSED_THROTTLE_MS + 50);
+    expect(cutoff.getTime()).toBe(Date.now() - OAT_LAST_ACCESSED_THROTTLE_MS);
   });
 
   test("skips updateMany when token is not found", async () => {

apps/webapp/test/services/personalAccessToken.test.ts

@@ -1,4 +1,4 @@
-import { beforeEach, describe, expect, test, vi } from "vitest";
+import { afterEach, beforeEach, describe, expect, test, vi } from "vitest";
 
 const { findFirstMock, updateManyMock } = vi.hoisted(() => ({
   findFirstMock: vi.fn(),
@@ -35,11 +35,17 @@ import {
 } from "~/services/personalAccessToken.server";
 
 beforeEach(() => {
+  vi.useFakeTimers();
+  vi.setSystemTime(new Date("2026-01-01T00:00:00.000Z"));
   findFirstMock.mockReset();
   updateManyMock.mockReset();
   updateManyMock.mockResolvedValue({ count: 1 });
 });
 
+afterEach(() => {
+  vi.useRealTimers();
+});
+
 describe("authenticatePersonalAccessToken — lastAccessedAt throttle", () => {
   test("issues a conditional updateMany that skips writes when lastAccessedAt is recent", async () => {
     findFirstMock.mockResolvedValueOnce({
@@ -49,15 +55,14 @@ describe("authenticatePersonalAccessToken — lastAccessedAt throttle", () => {
       encryptedToken: { nonce: "n", ciphertext: "c", tag: "t" },
     });
 
-    const before = Date.now();
     const result = await authenticatePersonalAccessToken("tr_pat_validtoken");
-    const after = Date.now();
 
     expect(result).toEqual({ userId: "user_1" });
     expect(updateManyMock).toHaveBeenCalledTimes(1);
 
     const call = updateManyMock.mock.calls[0][0];
     expect(call.where.id).toBe("pat_123");
+    expect(call.where.revokedAt).toBeNull();
     expect(call.data.lastAccessedAt).toBeInstanceOf(Date);
 
     // The WHERE clause should require the existing lastAccessedAt to be null
@@ -67,11 +72,9 @@ describe("authenticatePersonalAccessToken — lastAccessedAt throttle", () => {
       { lastAccessedAt: { lt: expect.any(Date) } },
     ]);
 
+    // With fake timers, the cutoff lands exactly throttle-ms before "now".
     const cutoff = call.where.OR[1].lastAccessedAt.lt as Date;
-    // Cutoff should be exactly throttle-ms before "now" (within the test
-    // window). Confirms the throttle constant is wired through correctly.
-    expect(cutoff.getTime()).toBeGreaterThanOrEqual(before - PAT_LAST_ACCESSED_THROTTLE_MS - 50);
-    expect(cutoff.getTime()).toBeLessThanOrEqual(after - PAT_LAST_ACCESSED_THROTTLE_MS + 50);
+    expect(cutoff.getTime()).toBe(Date.now() - PAT_LAST_ACCESSED_THROTTLE_MS);
   });
 
   test("skips updateMany when token is not found", async () => {