diff --git a/docs/analysis/2026-09-21-metrics-read-ownership.md b/docs/analysis/2026-09-21-metrics-read-ownership.md new file mode 100644 index 0000000000..8218c93ccd --- /dev/null +++ b/docs/analysis/2026-09-21-metrics-read-ownership.md @@ -0,0 +1,47 @@ +# Metrics and forecast request ownership + +Status: corrective draft for #3346 / PR #3347. Base: `307c3b8b50bec1cb0bfaea3e570a942bcb1d4451`. + +## Reproduced defects + +Board metrics and forecast own separate visible surfaces, but the original store committed every response, failure, toast and `finally`. Board/date changes could restore older results or clear current loading, `$reset()` did not invalidate in-flight work, and the store had no session boundary. + +Review then exposed two token-refresh defects: + +1. full reset on same-user refresh cleared already loaded dashboard data; +2. preservation alone stranded an empty first load because the old request was retired and the unchanged route did not refetch. + +## Contract + +- Metrics and forecast retain independent latest-request owners. +- A newer request retires only the previous owner in the same lane. +- User identity, authentication or demo-session replacement advances the epoch and clears both data surfaces. +- Token-only rotation preserves settled data and errors, retires old-token UI settlement, and restarts only an active lane whose visible surface is still null. +- Retried metrics/forecast reads retain the exact query captured by the active request. +- Stale requests still resolve or reject to their original callers, but cannot write results, errors, toasts, loading or final state. +- A current failure preserves the previous result and the public error/toast/rejection behavior. +- Metrics and forecast loading remain independent. +- No mutation is replayed and no endpoint, query type or public store API changes. + +This is client-state integrity, not transport cancellation or a server metrics-authorization change. + +## Test-first evidence + +The initial real Pinia suite covered seven deferred schedules. A bounded actual-module runner changed from **1/7 passing on `main`** to **7/7 passing** after the first correction. + +Review-regression head `f9f6bc9479ec7d211077b545be95a64cf63e65ae` isolated loaded-dashboard preservation. The corrected head `8f31b2b72e6941b5e77ab730aea34da8da75e9af` passed Smart CI, Extended and the complete Required CI matrix. + +Issue #3352 then added test-only head `b7425560e7f3c90833dde8bc74d82543d39ff389`, covering a token rotation while both metrics and forecast are still null. A dependency-free runner transpiled and executed the actual production module: + +- before the retry correction: each API was called once and both loading flags became false; +- after the correction: each API was called twice, old-token settlement was suppressed, and fresh-token results populated both lanes. + +Hosted exact-head qualification remains authoritative; the supplemental runner does not replace it. + +## Remaining gates + +Current production correction: `8566adbabd9abe5ddca9a5b09b79a928616db10f` before the current review fix; settled token-rotation errors are now preserved and retry failures are covered. + +Exact final-head lint, typecheck, production build, complete Vitest on Ubuntu and Windows, Required CI, Extended, Self-Test and fresh-context review remain required. Review should focus on query capture, no retry loops, and no mutation replay. + +No merge, release or deployment qualification is claimed. diff --git a/frontend/taskdeck-web/src/store/metricsStore.ts b/frontend/taskdeck-web/src/store/metricsStore.ts index 3006fa48bc..35ef19f5bb 100644 --- a/frontend/taskdeck-web/src/store/metricsStore.ts +++ b/frontend/taskdeck-web/src/store/metricsStore.ts @@ -1,13 +1,15 @@ import { defineStore } from 'pinia' -import { ref } from 'vue' +import { ref, watch } from 'vue' import { metricsApi } from '../api/metricsApi' import { useToastStore } from './toastStore' +import { useSessionStore } from './sessionStore' import { isDemoMode } from '../utils/demoMode' import { getErrorDisplay } from '../composables/useErrorMapper' import type { BoardMetricsResponse, BoardForecastResponse, MetricsQuery, ForecastQuery } from '../types/metrics' export const useMetricsStore = defineStore('metrics', () => { const toast = useToastStore() + const session = useSessionStore() const metrics = ref(null) const loading = ref(false) @@ -17,61 +19,162 @@ export const useMetricsStore = defineStore('metrics', () => { const forecastLoading = ref(false) const forecastError = ref(null) + type ReadRetry = () => Promise + + interface RequestOwner { + epoch: number + token: symbol + } + + let credentialEpoch = 0 + let metricsOwner: RequestOwner | null = null + let forecastOwner: RequestOwner | null = null + let metricsRetry: ReadRetry | null = null + let forecastRetry: ReadRetry | null = null + + function beginMetricsRequest(retry: ReadRetry): RequestOwner { + const owner = { epoch: credentialEpoch, token: Symbol('board-metrics') } + metricsOwner = owner + metricsRetry = retry + loading.value = true + error.value = null + return owner + } + + function ownsMetricsRequest(owner: RequestOwner): boolean { + return owner.epoch === credentialEpoch && metricsOwner?.token === owner.token + } + + function finishMetricsRequest(owner: RequestOwner): void { + if (!ownsMetricsRequest(owner)) return + metricsOwner = null + metricsRetry = null + loading.value = false + } + + function beginForecastRequest(retry: ReadRetry): RequestOwner { + const owner = { epoch: credentialEpoch, token: Symbol('board-forecast') } + forecastOwner = owner + forecastRetry = retry + forecastLoading.value = true + forecastError.value = null + return owner + } + + function ownsForecastRequest(owner: RequestOwner): boolean { + return owner.epoch === credentialEpoch && forecastOwner?.token === owner.token + } + + function finishForecastRequest(owner: RequestOwner): void { + if (!ownsForecastRequest(owner)) return + forecastOwner = null + forecastRetry = null + forecastLoading.value = false + } + + function invalidateRequests(options: { preserveErrors?: boolean } = {}): void { + credentialEpoch += 1 + metricsOwner = null + forecastOwner = null + metricsRetry = null + forecastRetry = null + loading.value = false + forecastLoading.value = false + if (!options.preserveErrors) { + error.value = null + forecastError.value = null + } + } + + function retryEmptyActiveRequests(): void { + const pendingMetricsRetry = metricsOwner && metrics.value === null ? metricsRetry : null + const pendingForecastRetry = forecastOwner && forecast.value === null ? forecastRetry : null + + invalidateRequests({ preserveErrors: true }) + if (pendingMetricsRetry) { + void pendingMetricsRetry().catch(() => { + // The retried store action owns current error/toast state. + }) + } + if (pendingForecastRetry) { + void pendingForecastRetry().catch(() => { + // The retried store action owns current error/toast state. + }) + } + } + + function $reset(): void { + invalidateRequests() + metrics.value = null + forecast.value = null + } + + watch( + () => [session.userId, session.isAuthenticated, session.isDemo], + $reset, + { flush: 'sync' }, + ) + + watch( + () => session.token, + retryEmptyActiveRequests, + { flush: 'sync' }, + ) + async function fetchBoardMetrics(query: MetricsQuery) { if (isDemoMode) { - loading.value = true - error.value = null - metrics.value = null + metricsOwner = null + metricsRetry = null loading.value = false error.value = 'Metrics are not available in demo mode.' + metrics.value = null return } + + const owner = beginMetricsRequest(() => fetchBoardMetrics(query)) try { - loading.value = true - error.value = null - metrics.value = await metricsApi.getBoardMetrics(query) + const result = await metricsApi.getBoardMetrics(query) + if (!ownsMetricsRequest(owner)) return + metrics.value = result } catch (e: unknown) { - const msg = getErrorDisplay(e, 'Failed to fetch board metrics').message - error.value = msg - toast.error(msg) + if (ownsMetricsRequest(owner)) { + const msg = getErrorDisplay(e, 'Failed to fetch board metrics').message + error.value = msg + toast.error(msg) + } throw e } finally { - loading.value = false + finishMetricsRequest(owner) } } async function fetchBoardForecast(query: ForecastQuery) { if (isDemoMode) { - forecastLoading.value = true - forecastError.value = null - forecast.value = null + forecastOwner = null + forecastRetry = null forecastLoading.value = false forecastError.value = 'Forecast is not available in demo mode.' + forecast.value = null return } + + const owner = beginForecastRequest(() => fetchBoardForecast(query)) try { - forecastLoading.value = true - forecastError.value = null - forecast.value = await metricsApi.getBoardForecast(query) + const result = await metricsApi.getBoardForecast(query) + if (!ownsForecastRequest(owner)) return + forecast.value = result } catch (e: unknown) { - const msg = getErrorDisplay(e, 'Failed to fetch board forecast').message - forecastError.value = msg - toast.error(msg) + if (ownsForecastRequest(owner)) { + const msg = getErrorDisplay(e, 'Failed to fetch board forecast').message + forecastError.value = msg + toast.error(msg) + } throw e } finally { - forecastLoading.value = false + finishForecastRequest(owner) } } - function $reset() { - metrics.value = null - loading.value = false - error.value = null - forecast.value = null - forecastLoading.value = false - forecastError.value = null - } - return { metrics, loading, diff --git a/frontend/taskdeck-web/src/tests/store/metricsStoreOwnership.spec.ts b/frontend/taskdeck-web/src/tests/store/metricsStoreOwnership.spec.ts new file mode 100644 index 0000000000..3a5c088142 --- /dev/null +++ b/frontend/taskdeck-web/src/tests/store/metricsStoreOwnership.spec.ts @@ -0,0 +1,340 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { createPinia, setActivePinia } from 'pinia' +import { metricsApi } from '../../api/metricsApi' +import { useMetricsStore } from '../../store/metricsStore' +import { useSessionStore } from '../../store/sessionStore' +import type { BoardForecastResponse, BoardMetricsResponse } from '../../types/metrics' + +const toastMocks = vi.hoisted(() => ({ + error: vi.fn(), + success: vi.fn(), + info: vi.fn(), + warning: vi.fn(), +})) + +vi.mock('../../utils/demoMode', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, isDemoMode: false } +}) + +vi.mock('../../api/metricsApi', () => ({ + metricsApi: { + getBoardMetrics: vi.fn(), + getBoardForecast: vi.fn(), + exportBoardMetricsCsv: vi.fn(), + }, +})) + +vi.mock('../../api/authApi', () => ({ + authApi: { + login: vi.fn(), + register: vi.fn(), + changePassword: vi.fn(), + refreshToken: vi.fn(), + exchangeOAuthCode: vi.fn(), + exchangeOidcCode: vi.fn(), + }, +})) + +vi.mock('../../store/toastStore', () => ({ + useToastStore: () => toastMocks, +})) + +function deferred() { + let resolve!: (value: T) => void + let reject!: (reason: unknown) => void + const promise = new Promise((yes, no) => { + resolve = yes + reject = no + }) + return { promise, resolve, reject } +} + +function metrics(boardId: string): BoardMetricsResponse { + return { + boardId, + from: '2026-08-01T00:00:00Z', + to: '2026-09-01T00:00:00Z', + throughput: [], + averageCycleTimeDays: 0, + cycleTimeEntries: [], + wipSnapshots: [], + totalWip: 0, + blockedCount: 0, + blockedCards: [], + } +} + +function forecast(boardId: string): BoardForecastResponse { + return { + boardId, + remainingCards: 0, + completedCards: 0, + averageThroughputPerDay: 0, + throughputStdDev: 0, + averageCycleTimeDays: 0, + estimatedCompletionDate: null, + confidenceBand: null, + dataPointCount: 0, + historyDaysUsed: 30, + assumptions: [], + caveats: [], + } +} + +describe('metricsStore async ownership', () => { + let session: ReturnType + let store: ReturnType + + beforeEach(() => { + setActivePinia(createPinia()) + session = useSessionStore() + session.userId = 'user-a' + session.token = 'token-a' + store = useMetricsStore() + vi.clearAllMocks() + }) + + it('keeps the newest metrics request when responses settle in reverse order', async () => { + const older = deferred() + const newer = deferred() + vi.mocked(metricsApi.getBoardMetrics) + .mockReturnValueOnce(older.promise) + .mockReturnValueOnce(newer.promise) + + const oldRequest = store.fetchBoardMetrics({ boardId: 'board-old' }) + const newRequest = store.fetchBoardMetrics({ boardId: 'board-new' }) + newer.resolve(metrics('board-new')) + await newRequest + older.resolve(metrics('board-old')) + await oldRequest + + expect(store.metrics?.boardId).toBe('board-new') + }) + + it('keeps the newest forecast request when responses settle in reverse order', async () => { + const older = deferred() + const newer = deferred() + vi.mocked(metricsApi.getBoardForecast) + .mockReturnValueOnce(older.promise) + .mockReturnValueOnce(newer.promise) + + const oldRequest = store.fetchBoardForecast({ boardId: 'board-old' }) + const newRequest = store.fetchBoardForecast({ boardId: 'board-new' }) + newer.resolve(forecast('board-new')) + await newRequest + older.resolve(forecast('board-old')) + await oldRequest + + expect(store.forecast?.boardId).toBe('board-new') + }) + + it('suppresses stale metrics failure UI after a newer success while preserving rejection', async () => { + const older = deferred() + const newer = deferred() + vi.mocked(metricsApi.getBoardMetrics) + .mockReturnValueOnce(older.promise) + .mockReturnValueOnce(newer.promise) + + const oldRequest = store.fetchBoardMetrics({ boardId: 'board-old' }) + const newRequest = store.fetchBoardMetrics({ boardId: 'board-new' }) + newer.resolve(metrics('board-new')) + await newRequest + older.reject(new Error('stale metrics failure')) + await expect(oldRequest).rejects.toThrow('stale metrics failure') + + expect(store.metrics?.boardId).toBe('board-new') + expect(store.error).toBeNull() + expect(toastMocks.error).not.toHaveBeenCalled() + }) + + it('does not let an older metrics finally clear the current metrics loading owner', async () => { + const older = deferred() + const newer = deferred() + vi.mocked(metricsApi.getBoardMetrics) + .mockReturnValueOnce(older.promise) + .mockReturnValueOnce(newer.promise) + + const oldRequest = store.fetchBoardMetrics({ boardId: 'board-old' }) + const newRequest = store.fetchBoardMetrics({ boardId: 'board-new' }) + older.resolve(metrics('board-old')) + await oldRequest + expect(store.loading).toBe(true) + + newer.resolve(metrics('board-new')) + await newRequest + expect(store.loading).toBe(false) + }) + + it('keeps metrics loading cleared when the newer request settles first', async () => { + const older = deferred() + const newer = deferred() + vi.mocked(metricsApi.getBoardMetrics) + .mockReturnValueOnce(older.promise) + .mockReturnValueOnce(newer.promise) + + const oldRequest = store.fetchBoardMetrics({ boardId: 'board-old' }) + const newRequest = store.fetchBoardMetrics({ boardId: 'board-new' }) + newer.resolve(metrics('board-new')) + await newRequest + expect(store.loading).toBe(false) + + older.resolve(metrics('board-old')) + await oldRequest + expect(store.metrics?.boardId).toBe('board-new') + expect(store.loading).toBe(false) + }) + + it('keeps metrics and forecast lanes independently concurrent', async () => { + const pendingMetrics = deferred() + const pendingForecast = deferred() + vi.mocked(metricsApi.getBoardMetrics).mockReturnValue(pendingMetrics.promise) + vi.mocked(metricsApi.getBoardForecast).mockReturnValue(pendingForecast.promise) + + const metricsRequest = store.fetchBoardMetrics({ boardId: 'board-a' }) + const forecastRequest = store.fetchBoardForecast({ boardId: 'board-a' }) + + pendingMetrics.resolve(metrics('board-a')) + await metricsRequest + expect(store.loading).toBe(false) + expect(store.forecastLoading).toBe(true) + + pendingForecast.resolve(forecast('board-a')) + await forecastRequest + expect(store.forecastLoading).toBe(false) + }) + + it('$reset invalidates pending success and failure settlements', async () => { + const pendingMetrics = deferred() + const pendingForecast = deferred() + vi.mocked(metricsApi.getBoardMetrics).mockReturnValue(pendingMetrics.promise) + vi.mocked(metricsApi.getBoardForecast).mockReturnValue(pendingForecast.promise) + + const metricsRequest = store.fetchBoardMetrics({ boardId: 'board-old' }) + const forecastRequest = store.fetchBoardForecast({ boardId: 'board-old' }) + store.$reset() + + pendingMetrics.resolve(metrics('board-old')) + pendingForecast.reject(new Error('stale forecast failure')) + await metricsRequest + await expect(forecastRequest).rejects.toThrow('stale forecast failure') + + expect(store.metrics).toBeNull() + expect(store.forecast).toBeNull() + expect(store.loading).toBe(false) + expect(store.forecastLoading).toBe(false) + expect(store.error).toBeNull() + expect(store.forecastError).toBeNull() + expect(toastMocks.error).not.toHaveBeenCalled() + }) + + it('preserves loaded dashboard data while invalidating old-token work on refresh', async () => { + store.metrics = metrics('existing') + store.forecast = forecast('existing') + const pendingMetrics = deferred() + const pendingForecast = deferred() + vi.mocked(metricsApi.getBoardMetrics).mockReturnValue(pendingMetrics.promise) + vi.mocked(metricsApi.getBoardForecast).mockReturnValue(pendingForecast.promise) + + const metricsRequest = store.fetchBoardMetrics({ boardId: 'board-old' }) + const forecastRequest = store.fetchBoardForecast({ boardId: 'board-old' }) + session.token = 'token-b' + + expect(store.metrics?.boardId).toBe('existing') + expect(store.forecast?.boardId).toBe('existing') + expect(store.loading).toBe(false) + expect(store.forecastLoading).toBe(false) + + pendingMetrics.resolve(metrics('old-token')) + pendingForecast.reject(new Error('old-token forecast failure')) + await metricsRequest + await expect(forecastRequest).rejects.toThrow('old-token forecast failure') + + expect(store.metrics?.boardId).toBe('existing') + expect(store.forecast?.boardId).toBe('existing') + expect(store.error).toBeNull() + expect(store.forecastError).toBeNull() + expect(toastMocks.error).not.toHaveBeenCalled() + }) + + it('preserves settled errors when same-user token rotation retires old work', async () => { + vi.mocked(metricsApi.getBoardMetrics).mockRejectedValueOnce(new Error('metrics failure')) + vi.mocked(metricsApi.getBoardForecast).mockRejectedValueOnce(new Error('forecast failure')) + + await expect(store.fetchBoardMetrics({ boardId: 'board-a' })).rejects.toThrow('metrics failure') + await expect(store.fetchBoardForecast({ boardId: 'board-a' })).rejects.toThrow('forecast failure') + expect(store.error).toBe('metrics failure') + expect(store.forecastError).toBe('forecast failure') + + session.token = 'token-b' + + expect(store.error).toBe('metrics failure') + expect(store.forecastError).toBe('forecast failure') + expect(toastMocks.error).toHaveBeenCalledTimes(2) + }) + + it('surfaces a failure from a token-rotation retry', async () => { + const oldMetrics = deferred() + const freshMetrics = deferred() + vi.mocked(metricsApi.getBoardMetrics) + .mockReturnValueOnce(oldMetrics.promise) + .mockReturnValueOnce(freshMetrics.promise) + + const oldRequest = store.fetchBoardMetrics({ boardId: 'board-a' }) + session.token = 'token-b' + oldMetrics.reject(new Error('old-token failure')) + await expect(oldRequest).rejects.toThrow('old-token failure') + + freshMetrics.reject(new Error('fresh-token failure')) + await vi.waitFor(() => { + expect(store.error).toBe('fresh-token failure') + }) + expect(toastMocks.error).toHaveBeenCalledWith('fresh-token failure') + }) + + it('retries empty initial metrics and forecast reads after same-user token rotation', async () => { + const oldMetrics = deferred() + const freshMetrics = deferred() + const oldForecast = deferred() + const freshForecast = deferred() + vi.mocked(metricsApi.getBoardMetrics) + .mockReturnValueOnce(oldMetrics.promise) + .mockReturnValueOnce(freshMetrics.promise) + vi.mocked(metricsApi.getBoardForecast) + .mockReturnValueOnce(oldForecast.promise) + .mockReturnValueOnce(freshForecast.promise) + + const metricsRequest = store.fetchBoardMetrics({ boardId: 'board-a' }) + const forecastRequest = store.fetchBoardForecast({ boardId: 'board-a' }) + session.token = 'token-b' + + expect(metricsApi.getBoardMetrics).toHaveBeenCalledTimes(2) + expect(metricsApi.getBoardForecast).toHaveBeenCalledTimes(2) + expect(store.metrics).toBeNull() + expect(store.forecast).toBeNull() + expect(store.loading).toBe(true) + expect(store.forecastLoading).toBe(true) + + oldMetrics.resolve(metrics('old-token')) + oldForecast.reject(new Error('old-token forecast failure')) + await metricsRequest + await expect(forecastRequest).rejects.toThrow('old-token forecast failure') + + expect(store.metrics).toBeNull() + expect(store.forecast).toBeNull() + expect(store.loading).toBe(true) + expect(store.forecastLoading).toBe(true) + expect(store.error).toBeNull() + expect(store.forecastError).toBeNull() + expect(toastMocks.error).not.toHaveBeenCalled() + + freshMetrics.resolve(metrics('fresh-token')) + freshForecast.resolve(forecast('fresh-token')) + await vi.waitFor(() => { + expect(store.metrics?.boardId).toBe('fresh-token') + expect(store.forecast?.boardId).toBe('fresh-token') + expect(store.loading).toBe(false) + expect(store.forecastLoading).toBe(false) + }) + }) +})