From 3cb3b415b0b7a77d24c9a2ee332e23c4433deb11 Mon Sep 17 00:00:00 2001 From: Fable Integrator Date: Fri, 10 Jul 2026 20:52:53 -0400 Subject: [PATCH 1/3] =?UTF-8?q?fix:=20error-overview=20sweep=20=E2=80=94?= =?UTF-8?q?=20TDZ=20import=20cycle,=20expired-session=20UX,=20Sentry=20dou?= =?UTF-8?q?ble-reports,=20miner=20deadlock=20retry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes for every live error across Vercel runtime, admin_events, and Sentry (7d window): - CoachHelm cold-start TDZ ("Cannot access 'a' before initialization" in generateAlerts): trigger-insights-bridge lazily import()ed insights.ts, the dynamic back-edge of a value-level import cycle (insights.ts statically imports the bridge's register fn). Two concurrent first-touches on a cold instance — a background after() round trigger racing a dashboard action — could observe insights.ts mid-evaluation. The bridge no longer imports insights.ts; both real callers (post-round-trigger, roster-sweep cron) now carry a synchronous side-effect import so registration completes at module init with no async gap. - Golf dashboard "Not authenticated" digests: a session can pass the top-of-page check yet expire before the data fetch re-validates it. That narrow case now redirects to /golf/login (both coach and player branches) instead of crashing to the route error boundary; real outages still surface. - Sentry duplicate captures of expected baseball control-flow throws (observed as BaseballUnauthorizedError from a logged-out tab's 60s NotificationBell poll): withBaseballAction already classifies these as handled/expected and re-raises; the re-raise escapes the server-action boundary where onRequestError/console capture re-reports it as an unhandled Error. The six expected error classes are now in sharedIgnoreErrors — they remain fully visible in admin_events/Bridge. - pattern-miner supersede deadlock (40P01, roster-sweep cron racing a round-submit mine over the same team): the supersede UPDATE is idempotent, so retry up to 2x with backoff instead of surfacing a transient as an error. Verified: tsc clean, eslint --max-warnings 0 clean, 494 test files / 4,917 tests green, production build green. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01J2E46URHkhCnEQSYDtZLJX --- .../api/cron/coachhelm-roster-sweep/route.ts | 5 ++++ src/app/golf/(dashboard)/dashboard/page.tsx | 18 ++++++++++-- src/instrumentation.ts | 15 ++++++++++ src/lib/coachhelm/v2/mining/pattern-miner.ts | 28 +++++++++++++----- src/lib/coachhelm/v2/post-round-trigger.ts | 6 ++++ .../coachhelm/v2/trigger-insights-bridge.ts | 29 +++++++++++-------- .../coachhelm/v2/post-round-trigger.test.ts | 6 ++++ 7 files changed, 85 insertions(+), 22 deletions(-) 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/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..a588f8854 100644 --- a/src/lib/coachhelm/v2/mining/pattern-miner.ts +++ b/src/lib/coachhelm/v2/mining/pattern-miner.ts @@ -992,14 +992,26 @@ 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) await new Promise((resolve) => setTimeout(resolve, 200 * attempt)); + 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 From fdb9c926983b9e15c07a51c515fb11c972c0e6bd Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 11 Jul 2026 15:19:03 +0000 Subject: [PATCH 2/3] =?UTF-8?q?fix:=20review=20follow-ups=20=E2=80=94=20au?= =?UTF-8?q?th=20outage=20vs=20expiry=20split,=20jittered=20deadlock=20back?= =?UTF-8?q?off?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from PR #803 review: - dashboard-data.ts: getUser() failures that supabase-js marks retryable (network / GoTrue 5xx) now surface to the route error boundary instead of being misread as 'Not authenticated' and redirected to login. A missing/expired session (AuthSessionMissingError, user null) still maps to the login redirect — applying the reviewer's literal 'throw any authError' suggestion would have broken the expiry redirect, since expiry returns an error object alongside the null user. - pattern-miner.ts: add random jitter (0-150ms) to the supersede deadlock backoff so victims retried in lock-step with the concurrent miner don't re-collide on the same window. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019WJ1Gzjj9MT3UsxhMoMzSV --- src/app/golf/actions/dashboard-data.ts | 17 +++++++++++++++-- src/lib/coachhelm/v2/mining/pattern-miner.ts | 7 ++++++- 2 files changed, 21 insertions(+), 3 deletions(-) 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/lib/coachhelm/v2/mining/pattern-miner.ts b/src/lib/coachhelm/v2/mining/pattern-miner.ts index a588f8854..66f2ba0b6 100644 --- a/src/lib/coachhelm/v2/mining/pattern-miner.ts +++ b/src/lib/coachhelm/v2/mining/pattern-miner.ts @@ -1000,7 +1000,12 @@ export class PatternMiner { // 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) await new Promise((resolve) => setTimeout(resolve, 200 * 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) From 0e0e7bb8b5dd7a79f946cf1782ba99563d68c3a2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 11 Jul 2026 15:22:58 +0000 Subject: [PATCH 3/3] docs: record expired-session login redirect in auth feature doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dashboard's expired-session redirect (PR #803) is user-visible behavior — note it in memory/features/auth-onboarding-join.md per the feature-doc rule, including the outage-vs-expiry split. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019WJ1Gzjj9MT3UsxhMoMzSV --- memory/features/auth-onboarding-join.md | 1 + 1 file changed, 1 insertion(+) 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.