diff --git a/memory/features/auth-onboarding-join.md b/memory/features/auth-onboarding-join.md index 7b7cf69c9..d1f6f92ef 100644 --- a/memory/features/auth-onboarding-join.md +++ b/memory/features/auth-onboarding-join.md @@ -72,6 +72,7 @@ Join code - Server actions must call `supabase.auth.getUser()` before database access. - Join codes are team-scoped and should be treated case-insensitively where the route expects that behavior. - Incomplete player onboarding should redirect to player onboarding with join context preserved. +- A session that expires mid-request on `/golf/dashboard` (passes the top-of-page check, fails the data-fetch re-validation with `Not authenticated`) redirects to `/golf/login?returnTo=/golf/dashboard` instead of hitting the error boundary. Retryable auth failures (network / GoTrue 5xx) still surface to the error boundary — only a genuinely missing/expired session redirects. - College/coach/player role rules must be respected before granting dashboard access. - Service-role logic must stay server-only and admin-bounded. diff --git a/src/app/api/cron/coachhelm-roster-sweep/route.ts b/src/app/api/cron/coachhelm-roster-sweep/route.ts index 96e441b04..20b46368b 100644 --- a/src/app/api/cron/coachhelm-roster-sweep/route.ts +++ b/src/app/api/cron/coachhelm-roster-sweep/route.ts @@ -14,6 +14,11 @@ */ import { NextResponse, type NextRequest } from 'next/server'; import { createAdminClient } from '@/lib/supabase/admin'; +// Side-effect-only: guarantees insights.ts's module-scope registration +// (__registerTriggerPlayerInsightsAfterRound) has run at module-init time — +// the bridge no longer lazily imports insights.ts itself (that dynamic +// back-edge was the cold-start TDZ cycle; see trigger-insights-bridge.ts). +import '@/app/golf/actions/insights'; import { triggerPlayerInsightsAfterRound } from '@/lib/coachhelm/v2/trigger-insights-bridge'; import { logServerError } from '@/lib/server-error-logger'; import { requireCronAuth } from '@/lib/cron/auth'; diff --git a/src/app/golf/(dashboard)/dashboard/page.tsx b/src/app/golf/(dashboard)/dashboard/page.tsx index 5ed01ba32..7e7750a5f 100644 --- a/src/app/golf/(dashboard)/dashboard/page.tsx +++ b/src/app/golf/(dashboard)/dashboard/page.tsx @@ -25,6 +25,20 @@ export const dynamic = 'force-dynamic'; const VALID_RANGES = new Set(['7d', '30d', '90d', 'season', 'all']); +/** + * A session can pass the top-of-page check yet expire before the data fetch + * re-validates it (auth token refresh race — observed live as repeated + * 'Not authenticated' digests on /golf/dashboard). Send that narrow case to + * the login screen; anything else still surfaces to the route error boundary + * (P002/P426 — real outages must never be swallowed). + */ +function redirectToLoginOnExpiredSession(error: unknown): never { + if (error instanceof Error && error.message === 'Not authenticated') { + redirect('/golf/login?returnTo=/golf/dashboard'); + } + throw error; +} + /** * Renders the coach dashboard inside the `.fairway-ds` scope on `bg-canvas`. * Fairway is the only tree (Wave W1) — the legacy CoachDashboard fork has @@ -126,7 +140,7 @@ export default async function GolfDashboardPage({ const [payload, joinRequestsResult] = await Promise.all([ getCachedCoachDashboardData(coach.id, userId, teamId, dateRange), getTeamJoinRequests(), - ]); + ]).catch(redirectToLoginOnExpiredSession); const joinRequests: JoinRequestData[] = joinRequestsResult.success && joinRequestsResult.data ? joinRequestsResult.data : []; @@ -201,7 +215,7 @@ export default async function GolfDashboardPage({ const [payload, hubData] = await Promise.all([ getCachedPlayerDashboardData(player.id, userId, teamId), teamId ? getPlayerHubSummaryData(teamId, player.id) : Promise.resolve(null), - ]); + ]).catch(redirectToLoginOnExpiredSession); const nameParts = `${player.first_name} ${player.last_name}`.split(' '); const data: PlayerDashboardData = { diff --git a/src/app/golf/actions/dashboard-data.ts b/src/app/golf/actions/dashboard-data.ts index 435efede1..fdf6da09b 100644 --- a/src/app/golf/actions/dashboard-data.ts +++ b/src/app/golf/actions/dashboard-data.ts @@ -247,7 +247,15 @@ async function getCoachDashboardDataImpl( .maybeSingle(), ]); - const { data: { user } } = authResult; + const { data: { user }, error: authError } = authResult; + // A genuine auth outage (network failure / GoTrue 5xx — supabase-js marks + // these retryable) must surface to the route error boundary, not be + // misread as a signed-out user. AuthSessionMissingError and other + // no-session results fall through to 'Not authenticated', which the + // dashboard page maps to the login redirect. + if (authError && (authError.name === 'AuthRetryableFetchError' || (authError.status ?? 0) >= 500)) { + throw authError; + } if (!user) throw new Error('Not authenticated'); if (user.id !== userId) throw new Error('Unauthorized'); @@ -755,7 +763,12 @@ async function getPlayerDashboardDataImpl( : Promise.resolve({ data: null }), ]); - const { data: { user } } = authResult; + const { data: { user }, error: authError } = authResult; + // Same outage-vs-expiry split as the coach payload above: retryable auth + // failures surface; a missing session becomes the login redirect. + if (authError && (authError.name === 'AuthRetryableFetchError' || (authError.status ?? 0) >= 500)) { + throw authError; + } if (!user) throw new Error('Not authenticated'); const playerTeamTimezone = (playerTimezoneResult.data as { timezone?: string } | null)?.timezone || 'America/New_York'; diff --git a/src/instrumentation.ts b/src/instrumentation.ts index ac1cf297c..35601665f 100644 --- a/src/instrumentation.ts +++ b/src/instrumentation.ts @@ -18,6 +18,21 @@ const sharedIgnoreErrors = [ // suppressed; every other AuthApiError now reaches Sentry. /AuthApiError: Invalid Refresh Token/, /Refresh Token Not Found/, + // Baseball expected control-flow throws. withBaseballAction already + // classifies these as handled/expected (admin_events + Sentry warning with + // skipSentry) and then RE-RAISES so callers can branch — but the re-raise + // escapes the server-action boundary, where Next's own console.error + + // onRequestError capture it a second time as an unhandled Sentry Error + // (observed live: a logged-out tab's 60 s NotificationBell poll produced + // "BaseballUnauthorizedError: You must be signed in"). These stay fully + // visible in admin_events / Helm Bridge; only the duplicate Sentry Error + // is suppressed. + 'BaseballUnauthorizedError', + 'BaseballNoActiveTeamError', + 'BaseballCapabilityError', + 'BaseballDisabledSourceError', + 'BaseballDemoReadOnlyError', + 'PlayerAccessError', ]; const scrubPii: Sentry.NodeOptions['beforeSend'] = (event) => { diff --git a/src/lib/coachhelm/v2/mining/pattern-miner.ts b/src/lib/coachhelm/v2/mining/pattern-miner.ts index 0b1a4ba4d..66f2ba0b6 100644 --- a/src/lib/coachhelm/v2/mining/pattern-miner.ts +++ b/src/lib/coachhelm/v2/mining/pattern-miner.ts @@ -992,14 +992,31 @@ export class PatternMiner { const playerIds = [...new Set(patterns.map((p) => p.playerId).filter(Boolean))]; if (ids.length > 0 && playerIds.length > 0) { const idList = `(${ids.map((id) => `"${id}"`).join(',')})`; - const { error: supersedeError } = await fromUntyped(supabase, 'golf_patterns_v2') - .update({ is_active: false, updated_at: new Date().toISOString() }) - .in('player_id', playerIds) - .eq('is_active', true) - .not('id', 'in', idList) - // Keep coach-curated patterns visible; only retire auto-detected ones - // (NULL or non-preserved lifecycle_state). - .or('lifecycle_state.is.null,lifecycle_state.not.in.(confirmed,addressed,resolved,dismissed)'); + // Multi-row UPDATE racing a concurrent mine's upserts/supersede over the + // same players can deadlock (observed live: 40P01 during the roster-sweep + // cron, round-submit trigger and cron batch mining the same team + // concurrently). Postgres aborts one victim wholesale and the supersede + // is idempotent (deterministic ids, converges on re-run), so a short + // retry absorbs it instead of surfacing a transient as an error. + let supersedeError: { code?: string } | null = null; + for (let attempt = 0; attempt < 3; attempt++) { + if (attempt > 0) { + // Jitter decorrelates retries from the concurrent miner we + // deadlocked with — fixed delays would re-collide in lock-step. + const backoff = 200 * attempt + Math.floor(Math.random() * 150); + await new Promise((resolve) => setTimeout(resolve, backoff)); + } + const { error } = await fromUntyped(supabase, 'golf_patterns_v2') + .update({ is_active: false, updated_at: new Date().toISOString() }) + .in('player_id', playerIds) + .eq('is_active', true) + .not('id', 'in', idList) + // Keep coach-curated patterns visible; only retire auto-detected ones + // (NULL or non-preserved lifecycle_state). + .or('lifecycle_state.is.null,lifecycle_state.not.in.(confirmed,addressed,resolved,dismissed)'); + supersedeError = error; + if (!error || error.code !== '40P01') break; + } if (supersedeError) { await logServerError('pattern-miner.savePatterns supersede stale', { action: 'pattern-miner.savePatterns', diff --git a/src/lib/coachhelm/v2/post-round-trigger.ts b/src/lib/coachhelm/v2/post-round-trigger.ts index e6501c4a6..f2da87cad 100644 --- a/src/lib/coachhelm/v2/post-round-trigger.ts +++ b/src/lib/coachhelm/v2/post-round-trigger.ts @@ -17,6 +17,12 @@ * NOT a replacement for triggerPlayerInsightsAfterRound — wraps it. */ import type { SupabaseClient } from '@supabase/supabase-js'; +// Side-effect-only: guarantees insights.ts's module-scope registration +// (__registerTriggerPlayerInsightsAfterRound) has run before this module's +// own top-level finishes — at synchronous module-init time, so there is no +// async gap for a concurrent request on the same warm instance to race +// against (the cold-start TDZ crash the bridge header documents). +import '@/app/golf/actions/insights'; import { triggerPlayerInsightsAfterRound } from '@/lib/coachhelm/v2/trigger-insights-bridge'; import { logServerError, logServerEvent } from '@/lib/server-error-logger'; import { diff --git a/src/lib/coachhelm/v2/trigger-insights-bridge.ts b/src/lib/coachhelm/v2/trigger-insights-bridge.ts index 418588fd3..7b8ab0d05 100644 --- a/src/lib/coachhelm/v2/trigger-insights-bridge.ts +++ b/src/lib/coachhelm/v2/trigger-insights-bridge.ts @@ -33,12 +33,21 @@ import 'server-only'; * * Wiring: insights.ts calls `__registerTriggerPlayerInsightsAfterRound(...)` * at its own module scope (a side effect, not an export) to hand this - * bridge a reference to the withAdminObserved-wrapped impl. - * `triggerPlayerInsightsAfterRound` below lazily triggers that registration - * (via a dynamic import of insights.ts) the first time it's called in a - * given process, then delegates to the registered function on every call — - * same object, same behavior, just never exported from a 'use server' - * module. The one legitimate CLIENT-reachable path (a coach manually + * bridge a reference to the withAdminObserved-wrapped impl. Each legitimate + * caller (post-round-trigger.ts, the roster-sweep cron route) carries a + * side-effect `import '@/app/golf/actions/insights'` so that registration + * has ALREADY run, synchronously, by the time the caller's own module + * finishes initializing. This used to be a lazy `await import()` inside + * `triggerPlayerInsightsAfterRound` below — but that made this file the + * dynamic back-edge of a value-level import cycle with insights.ts + * (insights.ts statically imports the register function above), and on a + * cold Fluid Compute instance two concurrent first-touches of the pair + * (a background after() trigger racing a dashboard server action) could + * observe insights.ts mid-evaluation — the intermittent cold-start + * "Cannot access 'a' before initialization" TDZ crash in generateAlerts. + * `triggerPlayerInsightsAfterRound` now only delegates to the registered + * function — same object, same behavior, just never exported from a + * 'use server' module. The one legitimate CLIENT-reachable path (a coach manually * refreshing a player's analysis) is unaffected — it goes through the * already-authed sibling action `refreshPlayerAnalysisAsCoach`, which is * still exported from insights.ts and calls the private impl directly @@ -90,15 +99,11 @@ export async function triggerPlayerInsightsAfterRound( partial?: boolean; code?: 'engine_no_recent_rounds' | 'engine_session_expired'; }> { - if (!impl) { - // Load insights.ts so its registration side effect runs. Cheap after the - // first call — subsequent imports hit the module cache. - await import('@/app/golf/actions/insights'); - } if (!impl) { throw new Error( '[trigger-insights-bridge] triggerPlayerInsightsAfterRound not registered — ' + - 'insights.ts failed to load or its registration call was removed', + "the caller is missing its side-effect import of '@/app/golf/actions/insights' " + + '(see the wiring note in this file header), or the registration call was removed', ); } return impl(playerId); diff --git a/src/test/coachhelm/v2/post-round-trigger.test.ts b/src/test/coachhelm/v2/post-round-trigger.test.ts index c3fb0859c..4289760a7 100644 --- a/src/test/coachhelm/v2/post-round-trigger.test.ts +++ b/src/test/coachhelm/v2/post-round-trigger.test.ts @@ -9,6 +9,12 @@ vi.mock('@/lib/coachhelm/v2/trigger-insights-bridge', () => ({ triggerPlayerInsightsAfterRound: (...args: unknown[]) => mockTrigger(...args), })); +// post-round-trigger.ts carries a side-effect `import '@/app/golf/actions/insights'` +// (registers the bridge impl at module-init time — the fix for the cold-start +// TDZ cycle). The bridge is fully mocked above, so the registration side effect +// is irrelevant here — stub the whole 4,400-line 'use server' module out. +vi.mock('@/app/golf/actions/insights', () => ({})); + // Spy on the actual severity/skipSentry passed to the two logging entry // points. postRoundTrigger is a SECOND independent consumer of the // triggerPlayerInsightsAfterRound result (alongside the withAdminObserved