perf(webapp): throttle PAT + OAT lastAccessedAt writes to once per 5 min#3493
Merged
perf(webapp): throttle PAT + OAT lastAccessedAt writes to once per 5 min#3493
Conversation
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).
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
🧰 Additional context used📓 Path-based instructions (8)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{packages/core,apps/webapp}/**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.ts📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.ts{,x}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
apps/webapp/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/**/*.server.ts📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
🧠 Learnings (11)📓 Common learnings📚 Learning: 2025-08-14T10:53:54.526ZApplied to files:
📚 Learning: 2026-04-30T21:28:35.705ZApplied to files:
📚 Learning: 2026-04-17T11:28:56.451ZApplied to files:
📚 Learning: 2026-04-13T21:44:00.032ZApplied to files:
📚 Learning: 2026-04-20T15:06:19.815ZApplied to files:
📚 Learning: 2026-03-26T10:02:25.354ZApplied to files:
📚 Learning: 2025-08-14T10:35:38.344ZApplied to files:
📚 Learning: 2026-03-22T13:26:12.060ZApplied to files:
📚 Learning: 2026-03-22T19:24:14.403ZApplied to files:
📚 Learning: 2026-03-26T09:02:07.973ZApplied to files:
🔇 Additional comments (2)
WalkthroughThis pull request throttles database writes to Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/webapp/app/services/organizationAccessToken.server.ts`:
- Around line 117-124: The throttled updateMany on organizationAccessToken can
still overwrite lastAccessedAt if a token is revoked between findFirst and
updateMany; modify the where predicate in organizationAccessToken.updateMany to
include revokedAt: null so the update only applies to non-revoked tokens (keep
the existing id and OR conditions for lastAccessedAt) to prevent stale writes to
revoked tokens.
In `@apps/webapp/app/services/personalAccessToken.server.ts`:
- Around line 217-224: The throttled update in
prisma.personalAccessToken.updateMany can still modify a token revoked after the
prior find; add revokedAt: null to the WHERE clause alongside id:
personalAccessToken.id and the existing OR on lastAccessedAt (and keep the
existing PAT_LAST_ACCESSED_THROTTLE_MS logic) so the update only applies when
the token remains unrevoked.
In `@apps/webapp/test/services/organizationAccessToken.test.ts`:
- Around line 45-66: The test's time tolerance assertions are flaky; replace the
before/after Date.now() approach by freezing the system clock with Jest fake
timers so cutoff can be asserted exactly: use jest.useFakeTimers('modern') and
jest.setSystemTime(fixedTime) before calling
authenticateOrganizationAccessToken("tr_oat_validtoken"), compute the expected
cutoff as new Date(fixedTime - OAT_LAST_ACCESSED_THROTTLE_MS) and assert
call.where.OR[1].lastAccessedAt.lt equals that exact Date, then restore timers
(jest.useRealTimers()) and keep the existing assertions that updateManyMock was
called and that call.where.id is "oat_123".
In `@apps/webapp/test/services/personalAccessToken.test.ts`:
- Around line 52-75: The test for authenticatePersonalAccessToken is flaky
because it compares a computed cutoff Date with a live Date window; replace the
±50ms tolerance by freezing time so the cutoff can be asserted exactly: in the
test around authenticatePersonalAccessToken("tr_pat_validtoken") use Jest fake
timers or a deterministic time mock (e.g., jest.useFakeTimers/Date.now mock /
advanceTo) to set Date.now() to a fixed value, call the function, then assert
the cutoff equals new Date(fixedNow - PAT_LAST_ACCESSED_THROTTLE_MS) and keep
the existing checks for updateManyMock and call.where OR unchanged; reference
authenticatePersonalAccessToken, PAT_LAST_ACCESSED_THROTTLE_MS, and
updateManyMock to locate the code to update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5547a904-9971-4181-8693-3562a10b7ece
📒 Files selected for processing (5)
.server-changes/throttle-token-last-accessed-at.mdapps/webapp/app/services/organizationAccessToken.server.tsapps/webapp/app/services/personalAccessToken.server.tsapps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/test/services/personalAccessToken.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / e2e-webapp / 🧪 E2E Tests: Webapp
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
apps/webapp/app/services/organizationAccessToken.server.tsapps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/app/services/personalAccessToken.server.tsapps/webapp/test/services/personalAccessToken.test.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/services/organizationAccessToken.server.tsapps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/app/services/personalAccessToken.server.tsapps/webapp/test/services/personalAccessToken.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Add crumbs as you write code using
//@Crumbscomments or `// `#region` `@crumbsblocks. These are temporary debug instrumentation and must be stripped usingagentcrumbs stripbefore merge.
Files:
apps/webapp/app/services/organizationAccessToken.server.tsapps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/app/services/personalAccessToken.server.tsapps/webapp/test/services/personalAccessToken.test.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/services/organizationAccessToken.server.tsapps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/app/services/personalAccessToken.server.tsapps/webapp/test/services/personalAccessToken.test.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/services/organizationAccessToken.server.tsapps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/app/services/personalAccessToken.server.tsapps/webapp/test/services/personalAccessToken.test.ts
**/*.ts{,x}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from
@trigger.dev/sdkwhen writing Trigger.dev tasks. Never use@trigger.dev/sdk/v3or deprecatedclient.defineJob.
Files:
apps/webapp/app/services/organizationAccessToken.server.tsapps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/app/services/personalAccessToken.server.tsapps/webapp/test/services/personalAccessToken.test.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: Access environment variables through theenvexport ofenv.server.tsinstead of directly accessingprocess.env
Use subpath exports from@trigger.dev/corepackage instead of importing from the root@trigger.dev/corepathUse named constants for sentinel/placeholder values (e.g.
const UNSET_VALUE = '__unset__') instead of raw string literals scattered across comparisons
Files:
apps/webapp/app/services/organizationAccessToken.server.tsapps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/app/services/personalAccessToken.server.tsapps/webapp/test/services/personalAccessToken.test.ts
apps/webapp/**/*.server.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
apps/webapp/**/*.server.ts: Never userequest.signalfor detecting client disconnects. UsegetRequestAbortSignal()fromapp/services/httpAsyncStorage.server.tsinstead, which is wired directly to Expressres.on('close')and fires reliably
Access environment variables viaenvexport fromapp/env.server.ts. Never useprocess.envdirectly
Always usefindFirstinstead offindUniquein Prisma queries.findUniquehas an implicit DataLoader that batches concurrent calls and has active bugs even in Prisma 6.x (uppercase UUIDs returning null, composite key SQL correctness issues, 5-10x worse performance).findFirstis never batched and avoids this entire class of issues
Files:
apps/webapp/app/services/organizationAccessToken.server.tsapps/webapp/app/services/personalAccessToken.server.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
apps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/test/services/personalAccessToken.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptivedescribeanditblocks
Tests should avoid mocks or stubs and use the helpers from@internal/testcontainerswhen Redis or Postgres are needed
Use vitest for running unit tests
Files:
apps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/test/services/personalAccessToken.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Use vitest exclusively for testing. Never mock anything — use testcontainers instead.
Place test files next to source files with naming patternMyService.ts->MyService.test.ts.
For Redis/PostgreSQL tests in vitest, use testcontainers helpers:redisTest,postgresTest, orcontainerTestimported from@internal/testcontainers.
Files:
apps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/test/services/personalAccessToken.test.ts
apps/webapp/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Do not import
env.server.tsdirectly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testableFor testable code, never import
env.server.tsin test files. Pass configuration as options instead (e.g.,realtimeClient.server.tstakes config as constructor arg,realtimeClientGlobal.server.tscreates singleton with env config)
Files:
apps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/test/services/personalAccessToken.test.ts
🧠 Learnings (17)
📓 Common learnings
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/app/services/taskIdentifierRegistry.server.ts:24-67
Timestamp: 2026-04-13T21:44:00.032Z
Learning: In `apps/webapp/app/services/taskIdentifierRegistry.server.ts`, the sequential upsert/updateMany/findMany writes in `syncTaskIdentifiers` are intentionally NOT wrapped in a Prisma transaction. This function runs only during deployment-change events (low-concurrency path), and any partial `isInLatestDeployment` state is acceptable because it self-corrects on the next deployment. Do not flag this as a missing-transaction/atomicity issue in future reviews.
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 3473
File: internal-packages/database/prisma/schema.prisma:59-60
Timestamp: 2026-04-30T21:28:35.705Z
Learning: In `triggerdotdev/trigger.dev` (`apps/webapp/app/services/sessionDuration.server.ts`), session duration lower-bound validation is enforced entirely at the app layer via `isAllowedSessionDuration`, which requires values to be present in `SESSION_DURATION_OPTIONS`. The minimum allowed value is 300 seconds (5 minutes), not 60 seconds. `User.sessionDuration` is a non-nullable `Int`; only `Organization.maxSessionDuration` is nullable (`Int?`). No DB-level CHECK constraints exist anywhere in the project's migrations — the project pattern is app-layer validation only. Do not flag missing DB-level CHECK constraints on these session duration fields in future reviews.
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2391
File: apps/webapp/app/services/organizationAccessToken.server.ts:50-0
Timestamp: 2025-08-14T10:53:54.526Z
Learning: In the Trigger.dev codebase, token service functions (like revokePersonalAccessToken and revokeOrganizationAccessToken) don't include tenant scoping in their database queries. Instead, authorization and tenant scoping happens at a higher level in the authentication flow (typically in route handlers) before these service functions are called. This is a consistent pattern across both Personal Access Tokens (PATs) and Organization Access Tokens (OATs).
📚 Learning: 2026-04-30T21:28:35.705Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 3473
File: internal-packages/database/prisma/schema.prisma:59-60
Timestamp: 2026-04-30T21:28:35.705Z
Learning: In `triggerdotdev/trigger.dev` (`apps/webapp/app/services/sessionDuration.server.ts`), session duration lower-bound validation is enforced entirely at the app layer via `isAllowedSessionDuration`, which requires values to be present in `SESSION_DURATION_OPTIONS`. The minimum allowed value is 300 seconds (5 minutes), not 60 seconds. `User.sessionDuration` is a non-nullable `Int`; only `Organization.maxSessionDuration` is nullable (`Int?`). No DB-level CHECK constraints exist anywhere in the project's migrations — the project pattern is app-layer validation only. Do not flag missing DB-level CHECK constraints on these session duration fields in future reviews.
Applied to files:
.server-changes/throttle-token-last-accessed-at.mdapps/webapp/app/services/organizationAccessToken.server.ts
📚 Learning: 2025-08-14T10:53:54.526Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2391
File: apps/webapp/app/services/organizationAccessToken.server.ts:50-0
Timestamp: 2025-08-14T10:53:54.526Z
Learning: In the Trigger.dev codebase, token service functions (like revokePersonalAccessToken and revokeOrganizationAccessToken) don't include tenant scoping in their database queries. Instead, authorization and tenant scoping happens at a higher level in the authentication flow (typically in route handlers) before these service functions are called. This is a consistent pattern across both Personal Access Tokens (PATs) and Organization Access Tokens (OATs).
Applied to files:
apps/webapp/app/services/organizationAccessToken.server.ts
📚 Learning: 2026-04-13T21:44:00.032Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/app/services/taskIdentifierRegistry.server.ts:24-67
Timestamp: 2026-04-13T21:44:00.032Z
Learning: In `apps/webapp/app/services/taskIdentifierRegistry.server.ts`, the sequential upsert/updateMany/findMany writes in `syncTaskIdentifiers` are intentionally NOT wrapped in a Prisma transaction. This function runs only during deployment-change events (low-concurrency path), and any partial `isInLatestDeployment` state is acceptable because it self-corrects on the next deployment. Do not flag this as a missing-transaction/atomicity issue in future reviews.
Applied to files:
apps/webapp/app/services/organizationAccessToken.server.tsapps/webapp/app/services/personalAccessToken.server.ts
📚 Learning: 2026-04-17T11:28:56.451Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3333
File: apps/webapp/app/services/dataStores/organizationDataStoresRegistry.server.ts:115-178
Timestamp: 2026-04-17T11:28:56.451Z
Learning: In `apps/webapp/app/services/dataStores/organizationDataStoresRegistry.server.ts`, the `addDataStore`, `updateDataStore`, and `deleteDataStore` methods intentionally do NOT call `reload()` or update the in-memory `_lookup` map after writing to the database. The registry runs as a singleton on many servers simultaneously; refreshing in-memory state on only the server that handled the mutation would create inconsistent routing across the fleet. The intended propagation mechanism is the periodic background reload controlled by `ORGANIZATION_DATA_STORES_RELOAD_INTERVAL_MS` (default 60s) in `organizationDataStoresRegistryInstance.server.ts`. Some brief routing disruption when switching an org between data stores is an accepted operational trade-off. Do not flag the mutation methods for missing in-memory refresh in future reviews.
Applied to files:
apps/webapp/app/services/organizationAccessToken.server.ts
📚 Learning: 2025-08-14T10:35:38.344Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2391
File: packages/cli-v3/src/commands/login.ts:99-0
Timestamp: 2025-08-14T10:35:38.344Z
Learning: In the whoami v2 endpoint, organization access tokens (OATs) return the same schema structure as personal access tokens (PATs), so existing CLI flows that expect userId and email fields work correctly with both token types.
Applied to files:
apps/webapp/app/services/organizationAccessToken.server.ts
📚 Learning: 2026-03-22T13:26:12.060Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/code/TextEditor.tsx:81-86
Timestamp: 2026-03-22T13:26:12.060Z
Learning: In the triggerdotdev/trigger.dev codebase, do not flag `navigator.clipboard.writeText(...)` calls for `missing-await`/`unhandled-promise` issues. These clipboard writes are intentionally invoked without `await` and without `catch` handlers across the project; keep that behavior consistent when reviewing TypeScript/TSX files (e.g., usages like in `apps/webapp/app/components/code/TextEditor.tsx`).
Applied to files:
apps/webapp/app/services/organizationAccessToken.server.tsapps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/app/services/personalAccessToken.server.tsapps/webapp/test/services/personalAccessToken.test.ts
📚 Learning: 2026-03-22T19:24:14.403Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts:200-204
Timestamp: 2026-03-22T19:24:14.403Z
Learning: In the triggerdotdev/trigger.dev codebase, webhook URLs are not expected to contain embedded credentials/secrets (e.g., fields like `ProjectAlertWebhookProperties` should only hold credential-free webhook endpoints). During code review, if you see logging or inclusion of raw webhook URLs in error messages, do not automatically treat it as a credential-leak/secrets-in-logs issue by default—first verify the URL does not contain embedded credentials (for example, no username/password in the URL, no obvious secret/token query params or fragments). If the URL is credential-free per this project’s conventions, allow the logging.
Applied to files:
apps/webapp/app/services/organizationAccessToken.server.tsapps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/app/services/personalAccessToken.server.tsapps/webapp/test/services/personalAccessToken.test.ts
📚 Learning: 2026-03-26T09:02:07.973Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 3274
File: apps/webapp/app/services/runsReplicationService.server.ts:922-924
Timestamp: 2026-03-26T09:02:07.973Z
Learning: When parsing Trigger.dev task run annotations in server-side services, keep `TaskRun.annotations` strictly conforming to the `RunAnnotations` schema from `trigger.dev/core/v3`. If the code already uses `RunAnnotations.safeParse` (e.g., in a `#parseAnnotations` helper), treat that as intentional/necessary for atomic, schema-accurate annotation handling. Do not recommend relaxing the annotation payload schema or using a permissive “passthrough” parse path, since the annotations are expected to be written atomically in one operation and should not contain partial/legacy payloads that would require a looser parser.
Applied to files:
apps/webapp/app/services/organizationAccessToken.server.tsapps/webapp/app/services/personalAccessToken.server.ts
📚 Learning: 2026-04-16T13:45:22.317Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/test/engine/taskIdentifierRegistry.test.ts:3-19
Timestamp: 2026-04-16T13:45:22.317Z
Learning: In `apps/webapp/test/engine/taskIdentifierRegistry.test.ts`, the `vi.mock` calls for `~/services/taskIdentifierCache.server` (stubbing `getTaskIdentifiersFromCache` and `populateTaskIdentifierCache`), `~/models/task.server` (stubbing `getAllTaskIdentifiers`), and `~/db.server` (stubbing `prisma` and `$replica`) are intentional. The suite uses real Postgres via testcontainers for all `TaskIdentifier` DB operations, but isolates the Redis cache layer and legacy query fallback as separate concerns not exercised in this test file. Do not flag these mocks as violations of the no-mocks policy in future reviews.
Applied to files:
apps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/test/services/personalAccessToken.test.ts
📚 Learning: 2026-04-07T14:12:18.946Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3331
File: apps/webapp/test/engine/batchPayloads.test.ts:5-24
Timestamp: 2026-04-07T14:12:18.946Z
Learning: In `apps/webapp/test/engine/batchPayloads.test.ts`, using `vi.mock` for `~/v3/objectStore.server` (stubbing `hasObjectStoreClient` and `uploadPacketToObjectStore`), `~/env.server` (overriding offload thresholds), and `~/v3/tracer.server` (stubbing `startActiveSpan`) is intentional and acceptable. Simulating controlled transient upload failures (e.g., fail N times then succeed) to verify `p-retry` behavior cannot be reproduced with real services or testcontainers. This file is an explicit exception to the repo's general no-mocks policy.
Applied to files:
apps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/test/services/personalAccessToken.test.ts
📚 Learning: 2026-04-15T15:39:06.868Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T15:39:06.868Z
Learning: Applies to **/*.test.{ts,tsx} : Use vitest exclusively for testing. Never mock anything — use testcontainers instead.
Applied to files:
apps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/test/services/personalAccessToken.test.ts
📚 Learning: 2026-03-03T13:07:33.177Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3166
File: internal-packages/run-engine/src/batch-queue/tests/index.test.ts:711-713
Timestamp: 2026-03-03T13:07:33.177Z
Learning: In `internal-packages/run-engine/src/batch-queue/tests/index.test.ts`, test assertions for rate limiter stubs can use `toBeGreaterThanOrEqual` rather than exact equality (`toBe`) because the consumer loop may call the rate limiter during empty pops in addition to actual item processing, and this over-calling is acceptable in integration tests.
Applied to files:
apps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/test/services/personalAccessToken.test.ts
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Use vitest for all tests in the Trigger.dev repository
Applied to files:
apps/webapp/test/services/organizationAccessToken.test.ts
📚 Learning: 2026-03-06T14:44:55.489Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3173
File: packages/trigger-sdk/src/v3/chat.test.ts:103-104
Timestamp: 2026-03-06T14:44:55.489Z
Learning: In `packages/trigger-sdk/src/v3/chat.test.ts`, mocking `global.fetch` with `vi.fn()` is acceptable and intentional. `TriggerChatTransport` is a browser-facing SSE/HTTP client, and using testcontainers for these tests is not required. This file is an explicit exception to the repo's general no-mocks policy.
Applied to files:
apps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/test/services/personalAccessToken.test.ts
📚 Learning: 2026-01-15T10:48:02.687Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T10:48:02.687Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Tests should avoid mocks or stubs and use the helpers from `internal/testcontainers` when Redis or Postgres are needed
Applied to files:
apps/webapp/test/services/organizationAccessToken.test.tsapps/webapp/test/services/personalAccessToken.test.ts
📚 Learning: 2026-03-13T13:42:59.104Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3213
File: apps/webapp/app/routes/admin.api.v1.llm-models.$modelId.ts:40-43
Timestamp: 2026-03-13T13:42:59.104Z
Learning: In `apps/webapp/app/routes/admin.api.v1.llm-models.$modelId.ts` and `apps/webapp/app/routes/admin.api.v1.llm-models.ts`, the `startDate` field in `UpdateModelSchema` and `CreateModelSchema` intentionally uses `z.string().optional()` (or `.nullable().optional()`) without strict ISO datetime validation. Invalid date strings are rejected at the Prisma/DB layer. This is acceptable because these are admin-only API routes protected by Personal Access Token (PAT) authentication and are not user-facing.
Applied to files:
apps/webapp/app/services/personalAccessToken.server.ts
🔇 Additional comments (1)
.server-changes/throttle-token-last-accessed-at.md (1)
1-7: Change note is clear and actionable.This documents the operational intent, scope, and expected UX tradeoff well.
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.
nicktrn
approved these changes
May 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Each successful PAT (
PersonalAccessToken) or OAT (OrganizationAccessToken) authentication issues aprisma.X.update({ lastAccessedAt: new Date() })to bump the timestamp. For tokens used at high frequency (CLI clients, integrations) this generates a per-request DB write that is mostly redundant — thelastAccessedAtfield is only surfaced on the settings page so users can decide which tokens to revoke, and "within the last 5 minutes" is plenty of granularity for that.Design
Replace each unconditional
updatewith a conditionalupdateManywhoseWHERErequires the existinglastAccessedAtto beNULLor strictly older than 5 minutes:The conditional runs inside the SQL
UPDATE, so concurrent auths can't race into a double-write.No schema change. No migration. No new infrastructure. Throttle is a hardcoded constant (
5 * 60 * 1000) — easy to revisit.Test plan
pnpm run typecheck --filter webapppnpm vitest run ./test/services/personalAccessToken.test.ts ./test/services/organizationAccessToken.test.ts— 6/6 pass, verifying the throttleWHEREclause is constructed correctly and theupdateis skipped on token-not-found / wrong-prefix paths