-
Couldn't load subscription status.
- Fork 244
fix: read-after-write issues #215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds AsyncLocalStorage-based session context, introduces session token decoding and cache-driven session validation, converts access helpers to cacheable wrappers, implements session-consistency Prisma extension and logger, centralizes webhook cache clearing, adds Redis deleteCache, and delays initial client-side session extension. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Api as API Middleware
participant ALS as AsyncLocalStorage
participant Cache as Redis Cache
participant Validator as validateSessionToken
participant Proc as TRPC Procedure
participant Access as getProjectAccess
Client->>Api: HTTP request + session token
Api->>Validator: decodeSessionToken(token) -> maybe sessionId
alt sessionId present
Api->>ALS: runWithAlsSession(sessionId, fn)
activate ALS
ALS->>Api: execute request within session context
end
Api->>Cache: getCache("validateSession:<sessionId>") (5m TTL) or call Validator
Api-->>Client: forward to TRPC
Proc->>Access: getProjectAccess(userId, projectId)
Access->>Cache: cacheable('getProjectAccess', key) -> hit/miss
alt Cache Miss
Access->>DB: query primary
DB-->>Access: result (cached for 300s)
end
Access-->>Proc: access result
deactivate ALS
Proc-->>Client: response
sequenceDiagram
participant App
participant PrismaExt as Prisma + sessionConsistency
participant Primary as Postgres Primary
participant Replica as Read Replica
participant WAL as WAL LSN cache
App->>PrismaExt: DB operation (with optional sessionId)
alt Write
PrismaExt->>Primary: execute write
Primary-->>PrismaExt: success
PrismaExt->>Primary: fetch current WAL LSN
Primary-->>WAL: WAL LSN
PrismaExt->>WAL: cache LSN for sessionId (TTL)
else Read
PrismaExt->>WAL: get cached LSN for sessionId
alt LSN exists
PrismaExt->>Replica: poll replica LSN until >= cached LSN (backoff)
alt Replica caught up
PrismaExt->>Replica: route read
else
PrismaExt->>Primary: route read
end
else
PrismaExt->>Replica: route read
end
end
PrismaExt-->>App: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/auth/src/session.ts (1)
66-77: Add cache invalidation to session.ts for expired and invalidated sessions.The review correctly identifies a cache invalidation gap. While
packages/trpc/src/routers/auth.ts:337invalidates cache after session validation, the corepackages/auth/src/session.tsfunctions (validateSessionTokenandinvalidateSession) currently do not, creating a window where stale "valid" sessions persist if called directly.The proposed fix aligns with the existing pattern already in use:
// Line 95-96 in validateSessionToken where expired sessions are deleted: if (Date.now() >= session.expiresAt.getTime()) { await db.session.delete({ where: { id: sessionId } }); + try { + const { deleteCache } = await import('@openpanel/redis'); + await deleteCache(`validateSession:${sessionId}`); + } catch {} return EMPTY_SESSION; }// Line 107-109 in invalidateSession: export async function invalidateSession(sessionId: string): Promise<void> { await db.session.delete({ where: { id: sessionId } }); + try { + const { deleteCache } = await import('@openpanel/redis'); + await deleteCache(`validateSession:${sessionId}`); + } catch {} }Verification confirms: no circular dependency (
@openpanel/redishas no auth imports), anddeleteCacheis a safe, lightweight utility already used throughout the codebase.
🧹 Nitpick comments (7)
packages/trpc/src/routers/chart.ts (1)
365-381: Coerce access to boolean to avoid union-type pitfalls.getProjectAccess can return a record or boolean. Use a boolean in conditionals to prevent brittle equality checks elsewhere.
- if (ctx.session.userId) { - const access = await getProjectAccess({ + if (ctx.session.userId) { + const hasAccess = !!(await getProjectAccess({ projectId: input.projectId, userId: ctx.session.userId, - }); - if (!access) { + })); + if (!hasAccess) {packages/auth/src/session.ts (1)
62-65: New decodeSessionToken helper LGTM; reuse it in createSession for consistency.Avoid duplicating the hashing logic.
export async function createSession( token: string, userId: string, ): Promise<Session> { - const sessionId = encodeHexLowerCase(sha256(new TextEncoder().encode(token))); + const sessionId = decodeSessionToken(token); const session: Session = { id: sessionId, userId,apps/api/src/controllers/ai.controller.ts (1)
12-12: Apply optional boolean coercion for clarity; import refactor verified complete.The import refactor is clean—
getProjectAccessis correctly imported and nogetProjectAccessCachedreferences remain. However, the suggested boolean coercion at lines 40-43 has not been applied. Consider wrapping thegetProjectAccessresult with!!to explicitly coerce to boolean and avoid relying on implicit object truthiness:- const access = await getProjectAccess({ + const hasAccess = !!(await getProjectAccess({ projectId, userId: session.userId, - }); - if (!access) { + })); + if (!hasAccess) {apps/api/src/controllers/webhook.controller.ts (1)
116-125: Make cache invalidation concurrent and resilient.Iterating with sequential awaits is O(n) latency and fragile on single-project failures. Run clears in parallel and swallow per-key errors.
async function clearOrganizationCache(organizationId: string) { const projects = await db.project.findMany({ where: { organizationId, }, }); - for (const project of projects) { - await getOrganizationByProjectIdCached.clear(project.id); - } + await Promise.all( + projects.map((p) => + getOrganizationByProjectIdCached.clear(p.id).catch((err) => { + // best-effort invalidation; log and continue + console.warn('clearOrganizationCache failed', { projectId: p.id, err }); + }), + ), + ); }packages/db/src/session-context.ts (1)
7-12: Use nullish coalescing and accept sync/async fns.Prevents empty-string coercion and makes wrapper more ergonomic.
-export const runWithAlsSession = <T>( - sid: string | null | undefined, - fn: () => Promise<T>, -) => als.run({ sessionId: sid || null }, fn); +export const runWithAlsSession = <T>( + sid: string | null | undefined, + fn: () => T | Promise<T>, +) => als.run({ sessionId: sid ?? null }, fn);packages/trpc/src/trpc.ts (1)
96-131: Avoid double ALS wrapping and place session scope first in the chain.
enforceAccesswraps withrunWithAlsSessionwhilesessionScopeMiddlewarealso wraps later; logger runs before session scope. MovesessionScopeMiddlewareto the front and drop the innerrunWithAlsSessionfor consistency.-const enforceAccess = t.middleware(async ({ ctx, next, type, getRawInput }) => { - return runWithAlsSession(ctx.session.session?.id, async () => { - const rawInput = await getRawInput(); +const enforceAccess = t.middleware(async ({ ctx, next, type, getRawInput }) => { + const rawInput = await getRawInput(); if (type === 'mutation' && process.env.DEMO_USER_ID) { throw new TRPCError({ code: 'UNAUTHORIZED', message: 'You are not allowed to do this in demo mode', }); } @@ - return next(); - }); + return next(); }); -const sessionScopeMiddleware = t.middleware(async ({ ctx, next }) => { - const sessionId = ctx.session.session?.id ?? null; - return runWithAlsSession(sessionId, async () => { - return next(); - }); -}); +const sessionScopeMiddleware = t.middleware(async ({ ctx, next }) => { + return runWithAlsSession(ctx.session.session?.id ?? null, next); +}); -export const publicProcedure = t.procedure - .use(loggerMiddleware) - .use(sessionScopeMiddleware); +export const publicProcedure = t.procedure + .use(sessionScopeMiddleware) + .use(loggerMiddleware); export const protectedProcedure = t.procedure - .use(enforceUserIsAuthed) - .use(enforceAccess) - .use(loggerMiddleware) - .use(sessionScopeMiddleware); + .use(sessionScopeMiddleware) + .use(enforceUserIsAuthed) + .use(enforceAccess) + .use(loggerMiddleware);Also applies to: 155-170
packages/db/src/session-consistency.ts (1)
175-199: Reduce log verbosity; avoid logging raw args.Logging full
argsat info-level after writes risks leaking PII/secrets. Downgrade to debug and omit rawargsor sanitize.-logger.info('Prisma operation', { - operation, - args, - model, -}); +logger.debug('Prisma operation', { operation, model });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/api/src/controllers/ai.controller.ts(2 hunks)apps/api/src/controllers/webhook.controller.ts(3 hunks)apps/api/src/index.ts(2 hunks)apps/start/src/hooks/use-session-extension.ts(1 hunks)packages/auth/src/session.ts(2 hunks)packages/db/index.ts(1 hunks)packages/db/src/logger.ts(1 hunks)packages/db/src/prisma-client.ts(3 hunks)packages/db/src/services/organization.service.ts(1 hunks)packages/db/src/session-consistency.ts(1 hunks)packages/db/src/session-context.ts(1 hunks)packages/redis/cachable.ts(1 hunks)packages/trpc/src/access.ts(1 hunks)packages/trpc/src/routers/auth.ts(2 hunks)packages/trpc/src/routers/chart.ts(2 hunks)packages/trpc/src/routers/event.ts(2 hunks)packages/trpc/src/routers/integration.ts(3 hunks)packages/trpc/src/trpc.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/start/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/start/.cursorrules)
apps/start/**/*.{ts,tsx}: Instrument server functions created with createServerFn by wrapping their implementation in Sentry.startSpan and importing Sentry from '@sentry/tanstackstart-react'
When using Sentry in the TanStack Start React app, import it as: import * as Sentry from '@sentry/tanstackstart-react'
Files:
apps/start/src/hooks/use-session-extension.ts
🧬 Code graph analysis (13)
packages/trpc/src/routers/event.ts (2)
packages/trpc/src/access.ts (1)
getProjectAccess(4-45)packages/trpc/index.ts (1)
getProjectAccess(3-3)
packages/trpc/src/routers/auth.ts (1)
packages/redis/cachable.ts (1)
deleteCache(3-5)
packages/redis/cachable.ts (1)
packages/redis/redis.ts (1)
getRedisCache(66-72)
packages/trpc/src/routers/integration.ts (1)
packages/trpc/src/access.ts (1)
getOrganizationAccess(47-64)
apps/api/src/controllers/webhook.controller.ts (2)
packages/db/src/prisma-client.ts (1)
db(276-276)packages/db/src/services/organization.service.ts (1)
getOrganizationByProjectIdCached(62-65)
apps/api/src/controllers/ai.controller.ts (1)
packages/trpc/src/access.ts (1)
getProjectAccess(4-45)
packages/db/src/prisma-client.ts (1)
packages/db/src/session-consistency.ts (1)
sessionConsistency(156-216)
packages/db/src/session-consistency.ts (3)
packages/db/src/logger.ts (1)
logger(3-3)packages/redis/redis.ts (1)
getRedisCache(66-72)packages/db/src/session-context.ts (1)
getAlsSessionId(12-12)
packages/db/src/logger.ts (1)
packages/logger/index.ts (1)
createLogger(10-97)
apps/api/src/index.ts (2)
packages/auth/src/session.ts (2)
decodeSessionToken(62-64)validateSessionToken(66-105)packages/db/src/session-context.ts (1)
runWithAlsSession(7-10)
packages/trpc/src/access.ts (2)
packages/redis/cachable.ts (1)
cacheable(78-154)packages/db/src/services/project.service.ts (1)
getProjectById(14-26)
packages/trpc/src/routers/chart.ts (2)
packages/trpc/src/access.ts (1)
getProjectAccess(4-45)packages/trpc/index.ts (1)
getProjectAccess(3-3)
packages/trpc/src/trpc.ts (4)
packages/auth/src/session.ts (1)
SessionValidationResult(111-113)packages/db/src/session-context.ts (1)
runWithAlsSession(7-10)packages/trpc/src/access.ts (2)
getProjectAccess(4-45)getOrganizationAccess(47-64)packages/trpc/src/errors.ts (1)
TRPCAccessError(3-7)
⏰ 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). (2)
- GitHub Check: build-and-push-worker
- GitHub Check: build-and-push-api
🔇 Additional comments (11)
packages/db/src/services/organization.service.ts (1)
62-65: Cache TTL reduction improves consistency.The TTL reduction from 1 day to 5 minutes significantly improves data freshness and addresses read-after-write consistency concerns. This is a reasonable tradeoff for organization data, which changes infrequently but must be consistent when it does change.
packages/db/src/logger.ts (1)
1-3: LGTM!The logger export is clean and follows established patterns. The 'db:prisma' naming clearly identifies the logger's scope.
packages/redis/cachable.ts (1)
3-5: LGTM!The
deleteCachefunction provides a clean API for cache invalidation, complementing the existinggetCacheutility. The implementation correctly wraps the Redisdelcommand.packages/trpc/src/routers/auth.ts (1)
337-337: Cache invalidation improves session consistency.The cache deletion after successful validation ensures that subsequent session checks will fetch fresh data, addressing read-after-write consistency concerns for session extensions.
packages/trpc/src/routers/integration.ts (1)
12-12: LGTM!The migration from
getOrganizationAccessCachedtogetOrganizationAccessaligns with the broader refactoring pattern across the codebase, standardizing on cacheable wrappers defined inpackages/trpc/src/access.ts.packages/db/index.ts (1)
26-26: LGTM!The export of session-context utilities expands the public API to support ALS-based session handling, aligning with the PR's broader session management improvements.
packages/trpc/src/routers/event.ts (1)
29-29: LGTM!The migration from
getProjectAccessCachedtogetProjectAccessis consistent with the broader access control refactoring, standardizing on cacheable wrappers frompackages/trpc/src/access.ts.apps/start/src/hooks/use-session-extension.ts (1)
27-31: LGTM!The 5-second delay for the initial session extension call prioritizes critical page load requests, improving user experience. The timeout is properly cleaned up in the effect's return function.
packages/trpc/src/routers/chart.ts (1)
35-35: Import change verified; no stale cached variants remain.Verification confirms the refactor is complete:
- Import correctly switched to
getProjectAccessat line 35- Function call properly updated at lines 366-370
- No
getProjectAccessCachedorgetOrganizationAccessCachedreferences exist anywhere in the codebasepackages/db/src/prisma-client.ts (1)
56-57: The review comment is incorrect.The web search result directly contradicts the core claim: Prisma executes extensions in the order you called $extends, meaning first-added runs first, not last-added-first.
The current code order is:
sessionConsistency()(line 56) — runs first- Result extension (line 57+) — runs second
readReplicas()(line 262-267) — runs thirdThis order is correct for the intended behavior. With first-added-first execution,
sessionConsistencysets__internalParams.transaction=truebeforereadReplicasevaluates the flag to decide routing.The suggested diff would move
readReplicasbeforesessionConsistency, which would reverse the execution order and break the read-after-write fallback logic—the opposite of the stated goal.Likely an incorrect or invalid review comment.
apps/api/src/controllers/webhook.controller.ts (1)
155-160: Good centralization; verify other caches aren’t missed.Resetting
subscriptionPeriodEventsCountExceededAtand callingclearOrganizationCachelooks right. Ensure no other org-scoped caches (e.g., access or org-by-id) need invalidation here; if so, extendclearOrganizationCacheto clear them too.Also applies to: 222-223
apps/api/src/index.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Caching auth without invalidation can keep logged‐out sessions valid for up to 5 minutes. Also: unnecessary await.
- Risk: validateSession results are cached for 5m, but logout/expiry doesn’t clear the cache → stale "logged‐in" state persists.
- Fix options (prefer both):
- Invalidate cache on logout/expiry (delete key validateSession:).
- Reduce TTL (e.g., 15–30s) if immediate invalidation isn’t wired yet.
- Minor: decodeSessionToken is sync; remove await.
- const sessionId = await decodeSessionToken(req.cookies.session); + const sessionId = decodeSessionToken(req.cookies.session); const session = await runWithAlsSession(sessionId, () => getCache(`validateSession:${sessionId}`, 60 * 5, async () => validateSessionToken(req.cookies.session), ), );
Additionally, ensure ALS context persists for the whole request so the Prisma session-consistency extension can see the sessionId during downstream DB reads (TRPC middleware may cover this, but confirm non‐TRPC routes too). If needed, introduce a request‐scoped ALS "enterWith" in a global hook.
🏁 Script executed:
#!/bin/bash # 1) Confirm TRPC runs under a session ALS middleware rg -n --hidden -g '!**/dist/**' 'sessionScopeMiddleware|runWithAlsSession\(' packages/trpc apps/api # 2) Find logout/invalidation paths to attach cache deletion rg -n --hidden -g '!**/dist/**' 'invalidateSession\(|session\.delete\(' # 3) Spot any non‐TRPC routes that hit Prisma but might miss ALS rg -n --hidden -g '!**/dist/**' 'db\.' apps/api/src/routes apps/api/src/controllers -g '!**/trpc/**'
Length of output: 2331
Need to verify remaining concerns about the decodeSessionToken function and cache invalidation logic.
Now checking for decodeSessionToken and cache invalidation details:
🏁 Script executed:
#!/bin/bash # 1) Find decodeSessionToken implementation to confirm if sync or async rg -n --hidden -g '!**/dist/**' 'export.*function decodeSessionToken|export.*const decodeSessionToken' -A3 # 2) Check if cache deletion happens anywhere related to invalidateSession or logout rg -n --hidden -g '!**/dist/**' 'deleteCache|invalidate.*cache|cache.*delete' -B2 -A2 # 3) Check where getCache (the caching utility) is defined and if it has cache invalidation rg -n --hidden -g '!**/dist/**' 'export.*getCache|function getCache' -A10
Length of output: 2774
🏁 Script executed:
#!/bin/bash # 1) Find decodeSessionToken definition rg -n --hidden -g '!**/dist/**' 'function decodeSessionToken|const decodeSessionToken|export.*decodeSessionToken' -A5 packages auth # 2) Check if cache deletion happens in invalidateSession or nearby logout logic rg -n --hidden -g '!**/dist/**' 'invalidateSession|logout' -B3 -A8 packages/auth apps/api
Length of output: 1107
Excellent progress. The critical issues are now clear. Let me verify the logout flow and non-TRPC routing:
🏁 Script executed:
#!/bin/bash # 1) Find logout endpoint to confirm it calls invalidateSession and doesn't clear cache rg -n --hidden -g '!**/dist/**' 'logout|signOut' -B2 -A8 packages/trpc/src/routers/auth.ts # 2) Check express/router setup for non-TRPC controllers to see if ALS middleware wraps them rg -n --hidden -g '!**/dist/**' 'app\.post|app\.get|app\.use|router\.' apps/api/src/index.ts -B1 -A3
Length of output: 434
One more check on the routing setup for non-TRPC controllers:
🏁 Script executed:
#!/bin/bash # 1) Check index.ts more broadly for controller routes and middleware setup cat apps/api/src/index.ts | head -200 # 2) Verify if there's a global ALS middleware or if only TRPC is wrapped rg -n --hidden -g '!**/dist/**' 'enterWith|AsyncLocalStorage|runWithAlsSession' apps/api/src/index.ts
Length of output: 6618
Now I have comprehensive verification. The review has correctly identified two critical issues, but slightly overstated the ALS concern. Let me generate the rewritten comment:
Remove unnecessary await and fix cache invalidation on logout.
The two issues identified are real:
-
decodeSessionTokenis synchronous (packages/auth/src/session.ts:62):const decodeSessionToken = (token: string): string =>. Theawaiton line 148 is incorrect. -
Cache not invalidated on logout: The
signOutendpoint callsinvalidateSessionwhich only deletes from the database, but does not clear thevalidateSession:${sessionId}cache key. This means logged-out sessions remain valid in cache for up to 5 minutes. ThedeleteCacheutility exists (used in token refresh at auth.ts:337) but is not called in the logout flow.
Regarding ALS: The session validation hook wraps the entire Dashboard API subtree (TRPC, webhooks, oauth, misc, ai, live) in runWithAlsSession, so TRPC and non-TRPC dashboard routes already have proper ALS context. Public API routes (export, insights, etc.) are not wrapped but don't require session context.
Fixes needed:
- Remove
awaitfrom line 148 (left side of assignment) - Call
await deleteCache(validateSession:${sessionId})in thesignOutendpoint (packages/trpc/src/routers/auth.ts:77) afterinvalidateSession
- const sessionId = await decodeSessionToken(req.cookies.session); + const sessionId = decodeSessionToken(req.cookies.session);
signOut: publicProcedure.mutation(async ({ ctx }) => {
deleteSessionTokenCookie(ctx.setCookie);
if (ctx.session?.session?.id) {
await invalidateSession(ctx.session.session.id);
+ await deleteCache(`validateSession:${ctx.session.session.id}`);
}
}),📝 Committable suggestion
!!️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
🤖 Prompt for AI Agents
In apps/api/src/index.ts around lines 147-153, remove the incorrect await on
decodeSessionToken (it's synchronous) so assign sessionId =
decodeSessionToken(req.cookies.session); and in
packages/trpc/src/routers/auth.ts around line 77, after calling
invalidateSession, add a call to await
deleteCache(`validateSession:${sessionId}`) to ensure the cached validated
session is removed on logout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cached access checks can violate read-after-write guarantees; expose raw functions and keep cached wrappers separately.
Access changes (e.g., new membership) may be invisible for up to 5 minutes due to Redis cache, even with DB R-A-W fixes. Use raw access checks in authorization paths; keep cached variants for non-auth flows.
-import { db, getProjectById } from '@openpanel/db'; -import { cacheable } from '@openpanel/redis'; +import { db, getProjectById } from '@openpanel/db'; +import { cacheable } from '@openpanel/redis'; -export const getProjectAccess = cacheable( - 'getProjectAccess', - async ({ - userId, - projectId, - }: { - userId: string; - projectId: string; - }) => { +export async function getProjectAccess({ + userId, + projectId, +}: { + userId: string; + projectId: string; +}) { try { // Check if user has access to the project const project = await getProjectById(projectId); if (!project?.organizationId) { return false; } const [projectAccess, member] = await Promise.all([ db.$primary().projectAccess.findMany({ where: { userId, organizationId: project.organizationId, }, }), db.$primary().member.findFirst({ where: { organizationId: project.organizationId, userId, }, }), ]); if (projectAccess.length === 0 && member) { return true; } return projectAccess.find((item) => item.projectId === projectId); } catch (err) { return false; } - }, - 60 * 5, -); +} + +export const getProjectAccessCached = cacheable( + 'getProjectAccess', + getProjectAccess, + 60 * 5, +); -export const getOrganizationAccess = cacheable( - 'getOrganizationAccess', - async ({ - userId, - organizationId, - }: { - userId: string; - organizationId: string; - }) => { - return db.$primary().member.findFirst({ - where: { - userId, - organizationId, - }, - }); - }, - 60 * 5, -); +export async function getOrganizationAccess({ + userId, + organizationId, +}: { + userId: string; + organizationId: string; +}) { + return db.$primary().member.findFirst({ + where: { + userId, + organizationId, + }, + }); +} +export const getOrganizationAccessCached = cacheable( + 'getOrganizationAccess', + getOrganizationAccess, + 60 * 5, +);
Follow-up: invalidate getProjectAccessCached/getOrganizationAccessCached on membership/projectAccess writes. I can draft the invalidation calls if you point me to the write paths.
Also applies to: 47-64
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with 100ドル free credit for automated AI reviews |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/index.ts (1)
155-160: Always setreq.session; missing else leaves it undefined.When validation returns
EMPTY_SESSION(no throw), the current code skips assignment. Set the fallback explicitly.- if (session.session) { - req.session = session; - } + req.session = session.session ? session : EMPTY_SESSION;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/api/src/index.ts(2 hunks)packages/auth/src/session.ts(2 hunks)packages/db/src/session-consistency.ts(1 hunks)packages/trpc/src/routers/auth.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/trpc/src/routers/auth.ts
- packages/auth/src/session.ts
- packages/db/src/session-consistency.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/src/index.ts (2)
packages/auth/src/session.ts (2)
decodeSessionToken(62-66)validateSessionToken(68-110)packages/db/src/session-context.ts (1)
runWithAlsSession(7-10)
⏰ 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). (2)
- GitHub Check: build-and-push-api
- GitHub Check: build-and-push-worker
🔇 Additional comments (2)
apps/api/src/index.ts (2)
11-15: Imports LGTM; decoder is correctly treated as sync.Brings in ALS + cache utilities and
decodeSessionToken(sync), aligning with usage below. No concerns.Also applies to: 22-22
150-153: Cache invalidation exists in logout and session extension; verify coverage for other session lifecycle events.Verification confirms that
deleteCache('validateSession:${sessionId}')is explicitly called in both thesignOuthandler (auth.ts:78) andextendSessionmutation (auth.ts:338). However, the 5-minute cache TTL still presents a risk if other session invalidation paths (e.g., server-side token expiry, session revocation by the auth library, or administrative termination) bypass cache deletion. Confirm that all paths where a session becomes invalid—not just user-initiated logout—also trigger cache invalidation. If session expiry is handled internally by the@openpanel/authlibrary or via scheduled jobs, ensure those paths also clear thevalidateSessioncache key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
ALS context is limited to validation; make it request‐scoped for non‐TRPC routes.
runWithAlsSession(sessionId, () => ...) scopes ALS only around validation. Downstream handlers (e.g., /live, /webhook, /oauth, /misc, /ai) will miss ALS, so the Prisma session‐consistency extension can’t see sessionId during DB reads → potential read‐after‐write inconsistency outside TRPC.
Prefer entering ALS for the entire request after you derive sessionId:
- If
@openpanel/dbcan exposeenterAlsSession(sessionId), call it here before validation and leave it set for the remainder of the request. - Otherwise, add a request‐level middleware that wraps route handling under
als.run/enterWith.
I can provide a small helper and wire‐up if useful.
Run to confirm coverage (expect TRPC covered; verify non‐TRPC):
🏁 Script executed:
#!/bin/bash # TRPC ALS coverage rg -n --hidden -g '!**/dist/**' 'sessionScopeMiddleware|runWithAlsSession\(' packages/trpc # Non‐TRPC dashboard routes touching DB that may lack ALS rg -n --hidden -g '!**/dist/**' -C2 'db\.' apps/api/src/routes -g '!**/trpc/**'
Length of output: 401
🏁 Script executed:
#!/bin/bash # Check structure of apps/api/src to understand route organization echo "=== Directory structure of apps/api/src ===" fd -t f -e ts -e js apps/api/src --max-depth 3 # Search for route definitions/handlers in apps/api/src (more permissive pattern) echo "=== Potential route handlers in apps/api/src ===" rg -n --hidden -g '!**/dist/**' "app\.(get|post|put|patch|delete|use)\(|router\." apps/api/src | head -30 # Check if non-TRPC routes access database echo "=== Non-TRPC files with db access ===" rg -n --hidden -g '!**/dist/**' -g '!**/trpc/**' "db\." apps/api/src | head -20 # Look at index.ts to understand the endpoint structure echo "=== Line count context around 147-154 in index.ts ===" wc -l apps/api/src/index.ts
Length of output: 2163
🏁 Script executed:
#!/bin/bash # View the validation code and surrounding context echo "=== index.ts lines 130-200 (full context around 147-154) ===" sed -n '130,200p' apps/api/src/index.ts # Find where controllers are imported/used echo "=== Controller imports and usage in index.ts ===" rg -n 'controller|Controller' apps/api/src/index.ts | head -20 # Check if there's a middleware that sets ALS at request level echo "=== Middleware patterns in index.ts ===" rg -n --hidden -g '!**/dist/**' "middleware|use\(|als\." apps/api/src/index.ts | head -20
Length of output: 2551
ALS context is limited to validation; extend it to request-scoped for non-TRPC routes.
The onRequest hook (lines 147-154) validates the session within runWithAlsSession(sessionId, ...) scope, but this context exits after validation completes. The non-TRPC routers registered at lines 191-196 (/live, /webhook, /oauth, /misc, /ai) and their controllers (which we found making DB calls in healthcheck.controller.ts, ai.controller.ts, webhook.controller.ts, oauth-callback.controller.tsx, export.controller.ts) execute outside this ALS context, creating risk of read-after-write inconsistency during database operations.
TRPC is properly covered by its own sessionScopeMiddleware (at packages/trpc/src/trpc.ts:155–169), but non-TRPC routes lack request-level ALS wrapping.
Recommend one of:
- If
@openpanel/dbcan exposeenterAlsSession(sessionId), call it in the hook before validation and leave it set for the entire request lifecycle. - Otherwise, add a request-level middleware that wraps all downstream route handling under
als.run()orals.enterWith().
🤖 Prompt for AI Agents
In apps/api/src/index.ts around lines 147 to 154, the ALS is only set during
session validation and exits before non-TRPC routes run; fix by establishing
request-scoped ALS for the whole request: if @openpanel/db exposes
enterAlsSession(sessionId) call it before validation and leave it set for the
request lifecycle, otherwise add a request-level middleware (placed before
registering /live, /webhook, /oauth, /misc, /ai routes) that wraps downstream
handling in als.run() or als.enterWith() using the decoded sessionId so all
subsequent controllers execute inside the same ALS context.
Uh oh!
There was an error while loading. Please reload this page.
Summary by CodeRabbit
New Features
Performance Improvements
Bug Fixes