From 6e72cf1e19595b4a071b4024014b4ea7d8dd5481 Mon Sep 17 00:00:00 2001 From: Eric Allam Date: Fri, 1 May 2026 14:27:28 +0100 Subject: [PATCH 1/2] perf(webapp): throttle PAT + OAT lastAccessedAt writes to once per 5 min MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each successful PAT (`PersonalAccessToken`) or OAT (`OrganizationAccessToken`) authentication was issuing an unconditional `prisma.X.update({ lastAccessedAt: new Date() })` on the auth path. Prod observation (2026-05-01): - PersonalAccessToken: ~2.4 writes/sec, 19,617 lifetime autovacuums, vacuumed every ~5 minutes. - OrganizationAccessToken: ~0.9 writes/sec, similar shape. Same denormalization-on-the-hot-path pattern as TRI-8891 — a small narrow table getting hammered with per-event writes that drive frequent autovacuum churn. The `lastAccessedAt` field is only ever read on the /account/tokens settings page to show "last used X ago" so users can decide which tokens to revoke; UI granularity of "within the last 5 minutes" is more than sufficient. Replace each unconditional `update` with a conditional `updateMany` whose WHERE requires the existing `lastAccessedAt` to be NULL or strictly older than 5 minutes. The conditional runs inside the SQL UPDATE, so concurrent auths don't race into double-writes. Estimated impact: ~95% reduction in writes on these two tables. Each table's autovacuum cadence drifts from every ~5 min to every ~hour or longer. Mock-based unit tests verify the throttle WHERE clause is constructed correctly. Behavioral verification will happen post-deploy via DB stats (n_tup_upd rate + autovacuum_count drift). --- .../throttle-token-last-accessed-at.md | 6 ++ .../organizationAccessToken.server.ts | 15 ++- .../services/personalAccessToken.server.ts | 15 ++- .../services/organizationAccessToken.test.ts | 84 +++++++++++++++++ .../test/services/personalAccessToken.test.ts | 93 +++++++++++++++++++ 5 files changed, 211 insertions(+), 2 deletions(-) create mode 100644 .server-changes/throttle-token-last-accessed-at.md create mode 100644 apps/webapp/test/services/organizationAccessToken.test.ts create mode 100644 apps/webapp/test/services/personalAccessToken.test.ts diff --git a/.server-changes/throttle-token-last-accessed-at.md b/.server-changes/throttle-token-last-accessed-at.md new file mode 100644 index 00000000000..53379622cc3 --- /dev/null +++ b/.server-changes/throttle-token-last-accessed-at.md @@ -0,0 +1,6 @@ +--- +area: webapp +type: improvement +--- + +Throttle `PersonalAccessToken.lastAccessedAt` and `OrganizationAccessToken.lastAccessedAt` writes to at most once per 5 minutes per token. Eliminates ~95% of writes on two narrow hot tables that were autovacuuming every ~5 minutes — same denormalization-on-the-hot-path shape as the schedule engine fix in TRI-8891. The settings UI continues to display "last used" with at most 5-minute lag. diff --git a/apps/webapp/app/services/organizationAccessToken.server.ts b/apps/webapp/app/services/organizationAccessToken.server.ts index ba11374c24d..65626166d4b 100644 --- a/apps/webapp/app/services/organizationAccessToken.server.ts +++ b/apps/webapp/app/services/organizationAccessToken.server.ts @@ -8,6 +8,12 @@ const tokenValueLength = 40; //lowercase only, removed 0 and l to avoid confusion const tokenGenerator = customAlphabet("123456789abcdefghijkmnopqrstuvwxyz", tokenValueLength); +// Skip the lastAccessedAt write if the existing value is already within this +// window. Eliminates per-auth UPDATE churn on a small narrow hot table; the +// settings UI reads this field at human granularity so a few-minute +// staleness is fine. +export const OAT_LAST_ACCESSED_THROTTLE_MS = 5 * 60 * 1000; + type CreateOrganizationAccessTokenOptions = { name: string; organizationId: string; @@ -105,9 +111,16 @@ export async function authenticateOrganizationAccessToken( return; } - await prisma.organizationAccessToken.update({ + // 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. + await prisma.organizationAccessToken.updateMany({ where: { id: organizationAccessToken.id, + OR: [ + { lastAccessedAt: null }, + { lastAccessedAt: { lt: new Date(Date.now() - OAT_LAST_ACCESSED_THROTTLE_MS) } }, + ], }, data: { lastAccessedAt: new Date(), diff --git a/apps/webapp/app/services/personalAccessToken.server.ts b/apps/webapp/app/services/personalAccessToken.server.ts index cceb576c9d8..aad9b1a9003 100644 --- a/apps/webapp/app/services/personalAccessToken.server.ts +++ b/apps/webapp/app/services/personalAccessToken.server.ts @@ -10,6 +10,12 @@ const tokenValueLength = 40; //lowercase only, removed 0 and l to avoid confusion const tokenGenerator = customAlphabet("123456789abcdefghijkmnopqrstuvwxyz", tokenValueLength); +// Skip the lastAccessedAt write if the existing value is already within this +// window. Eliminates per-auth UPDATE churn on a small narrow hot table; the +// /account/tokens UI reads this field at human granularity so a few-minute +// staleness is fine. +export const PAT_LAST_ACCESSED_THROTTLE_MS = 5 * 60 * 1000; + type CreatePersonalAccessTokenOptions = { name: string; userId: string; @@ -205,9 +211,16 @@ export async function authenticatePersonalAccessToken( return; } - await prisma.personalAccessToken.update({ + // 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. + await prisma.personalAccessToken.updateMany({ where: { id: personalAccessToken.id, + OR: [ + { lastAccessedAt: null }, + { lastAccessedAt: { lt: new Date(Date.now() - PAT_LAST_ACCESSED_THROTTLE_MS) } }, + ], }, data: { lastAccessedAt: new Date(), diff --git a/apps/webapp/test/services/organizationAccessToken.test.ts b/apps/webapp/test/services/organizationAccessToken.test.ts new file mode 100644 index 00000000000..d5e43ad92f1 --- /dev/null +++ b/apps/webapp/test/services/organizationAccessToken.test.ts @@ -0,0 +1,84 @@ +import { beforeEach, describe, expect, test, vi } from "vitest"; + +const { findFirstMock, updateManyMock } = vi.hoisted(() => ({ + findFirstMock: vi.fn(), + updateManyMock: vi.fn(), +})); + +vi.mock("~/db.server", () => ({ + prisma: { + organizationAccessToken: { + findFirst: findFirstMock, + updateMany: updateManyMock, + }, + }, + $replica: {}, +})); + +vi.mock("~/utils/tokens.server", () => ({ + hashToken: (t: string) => `hashed:${t}`, +})); + +vi.mock("./logger.server", () => ({ + logger: { warn: vi.fn(), error: vi.fn() }, +})); + +import { + authenticateOrganizationAccessToken, + OAT_LAST_ACCESSED_THROTTLE_MS, +} from "~/services/organizationAccessToken.server"; + +beforeEach(() => { + findFirstMock.mockReset(); + updateManyMock.mockReset(); + updateManyMock.mockResolvedValue({ count: 1 }); +}); + +describe("authenticateOrganizationAccessToken — lastAccessedAt throttle", () => { + test("issues a conditional updateMany that skips writes when lastAccessedAt is recent", async () => { + findFirstMock.mockResolvedValueOnce({ + id: "oat_123", + organizationId: "org_1", + 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.data.lastAccessedAt).toBeInstanceOf(Date); + + // The WHERE clause should require the existing lastAccessedAt to be null + // or strictly older than the throttle window — that's the entire point. + expect(call.where.OR).toEqual([ + { lastAccessedAt: null }, + { lastAccessedAt: { lt: expect.any(Date) } }, + ]); + + 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); + }); + + test("skips updateMany when token is not found", async () => { + findFirstMock.mockResolvedValueOnce(null); + + const result = await authenticateOrganizationAccessToken("tr_oat_validtoken"); + + expect(result).toBeUndefined(); + expect(updateManyMock).not.toHaveBeenCalled(); + }); + + test("skips updateMany when token doesn't start with prefix", async () => { + const result = await authenticateOrganizationAccessToken("not_an_oat"); + + expect(result).toBeUndefined(); + expect(findFirstMock).not.toHaveBeenCalled(); + expect(updateManyMock).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/webapp/test/services/personalAccessToken.test.ts b/apps/webapp/test/services/personalAccessToken.test.ts new file mode 100644 index 00000000000..88cee84e574 --- /dev/null +++ b/apps/webapp/test/services/personalAccessToken.test.ts @@ -0,0 +1,93 @@ +import { beforeEach, describe, expect, test, vi } from "vitest"; + +const { findFirstMock, updateManyMock } = vi.hoisted(() => ({ + findFirstMock: vi.fn(), + updateManyMock: vi.fn(), +})); + +vi.mock("~/db.server", () => ({ + prisma: { + personalAccessToken: { + findFirst: findFirstMock, + updateMany: updateManyMock, + }, + }, + $replica: {}, +})); + +vi.mock("~/env.server", () => ({ + env: { ENCRYPTION_KEY: "0".repeat(64) }, +})); + +vi.mock("~/utils/tokens.server", () => ({ + hashToken: (t: string) => `hashed:${t}`, + encryptToken: () => ({ nonce: "n", ciphertext: "c", tag: "t" }), + decryptToken: () => "tr_pat_validtoken", +})); + +vi.mock("./logger.server", () => ({ + logger: { warn: vi.fn(), error: vi.fn() }, +})); + +import { + authenticatePersonalAccessToken, + PAT_LAST_ACCESSED_THROTTLE_MS, +} from "~/services/personalAccessToken.server"; + +beforeEach(() => { + findFirstMock.mockReset(); + updateManyMock.mockReset(); + updateManyMock.mockResolvedValue({ count: 1 }); +}); + +describe("authenticatePersonalAccessToken — lastAccessedAt throttle", () => { + test("issues a conditional updateMany that skips writes when lastAccessedAt is recent", async () => { + findFirstMock.mockResolvedValueOnce({ + id: "pat_123", + userId: "user_1", + hashedToken: "hashed:tr_pat_validtoken", + 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.data.lastAccessedAt).toBeInstanceOf(Date); + + // The WHERE clause should require the existing lastAccessedAt to be null + // or strictly older than the throttle window — that's the entire point. + expect(call.where.OR).toEqual([ + { lastAccessedAt: null }, + { lastAccessedAt: { lt: expect.any(Date) } }, + ]); + + 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); + }); + + test("skips updateMany when token is not found", async () => { + findFirstMock.mockResolvedValueOnce(null); + + const result = await authenticatePersonalAccessToken("tr_pat_validtoken"); + + expect(result).toBeUndefined(); + expect(updateManyMock).not.toHaveBeenCalled(); + }); + + test("skips updateMany when token doesn't start with prefix", async () => { + const result = await authenticatePersonalAccessToken("not_a_pat"); + + expect(result).toBeUndefined(); + expect(findFirstMock).not.toHaveBeenCalled(); + expect(updateManyMock).not.toHaveBeenCalled(); + }); +}); From b3adbf194ad1462dc421d03194906e92d75770e0 Mon Sep 17 00:00:00 2001 From: Eric Allam Date: Fri, 1 May 2026 14:58:51 +0100 Subject: [PATCH 2/2] fix: address PR review on token throttle PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../services/organizationAccessToken.server.ts | 5 ++++- .../app/services/personalAccessToken.server.ts | 5 ++++- .../services/organizationAccessToken.test.ts | 15 ++++++++++----- .../test/services/personalAccessToken.test.ts | 17 ++++++++++------- 4 files changed, 28 insertions(+), 14 deletions(-) diff --git a/apps/webapp/app/services/organizationAccessToken.server.ts b/apps/webapp/app/services/organizationAccessToken.server.ts index 65626166d4b..77519ef8d1f 100644 --- a/apps/webapp/app/services/organizationAccessToken.server.ts +++ b/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) } }, diff --git a/apps/webapp/app/services/personalAccessToken.server.ts b/apps/webapp/app/services/personalAccessToken.server.ts index aad9b1a9003..e781cdfeb7a 100644 --- a/apps/webapp/app/services/personalAccessToken.server.ts +++ b/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) } }, diff --git a/apps/webapp/test/services/organizationAccessToken.test.ts b/apps/webapp/test/services/organizationAccessToken.test.ts index d5e43ad92f1..cf3900f4c47 100644 --- a/apps/webapp/test/services/organizationAccessToken.test.ts +++ b/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 () => { diff --git a/apps/webapp/test/services/personalAccessToken.test.ts b/apps/webapp/test/services/personalAccessToken.test.ts index 88cee84e574..0b52a7bf706 100644 --- a/apps/webapp/test/services/personalAccessToken.test.ts +++ b/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 () => {