From 78ecac7d3bc37fe80ba1c746f0eecb9e6d72f372 Mon Sep 17 00:00:00 2001 From: Cristian Tcaci <59696583+Chris0Jeky@users.noreply.github.com> Date: Mon, 21 Sep 2026 13:00:43 +0100 Subject: [PATCH 1/2] test(integrations): reproduce read ownership races --- .../store/integrationStoreOwnership.spec.ts | 173 ++++++++++++++++++ 1 file changed, 173 insertions(+) create mode 100644 frontend/taskdeck-web/src/tests/store/integrationStoreOwnership.spec.ts diff --git a/frontend/taskdeck-web/src/tests/store/integrationStoreOwnership.spec.ts b/frontend/taskdeck-web/src/tests/store/integrationStoreOwnership.spec.ts new file mode 100644 index 000000000..6e7922187 --- /dev/null +++ b/frontend/taskdeck-web/src/tests/store/integrationStoreOwnership.spec.ts @@ -0,0 +1,173 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { createPinia, setActivePinia } from 'pinia' +import { integrationsApi } from '../../api/integrationsApi' +import { useIntegrationStore } from '../../store/integrationStore' +import type { IntegrationConnector, IntegrationConnectorDetail } from '../../types/integration' + +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/integrationsApi', () => ({ + integrationsApi: { + listConnectors: vi.fn(), + getConnector: vi.fn(), + registerConnector: vi.fn(), + updateConnector: vi.fn(), + deleteConnector: vi.fn(), + enableConnector: vi.fn(), + disableConnector: vi.fn(), + }, +})) + +vi.mock('../../store/toastStore', () => ({ + useToastStore: () => toastMocks, +})) + +vi.mock('../../composables/useErrorMapper', () => ({ + getErrorDisplay: (_error: unknown, fallback: string) => ({ message: fallback }), +})) + +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 connector(id: string, name: string): IntegrationConnector { + return { + id, + name, + connectorType: 'BrowserClipper', + direction: 'Inbound', + status: 'Active', + configuration: null, + createdAt: '2026-09-21T00:00:00Z', + updatedAt: '2026-09-21T00:00:00Z', + } +} + +function detail(id: string, name: string): IntegrationConnectorDetail { + return { ...connector(id, name), recentEvents: [] } +} + +describe('integrationStore request ownership', () => { + beforeEach(() => { + setActivePinia(createPinia()) + vi.clearAllMocks() + }) + + it('does not let an old A detail replace the newer A visit after A to B to A', async () => { + const oldA = deferred() + const boardB = deferred() + const newA = deferred() + vi.mocked(integrationsApi.getConnector) + .mockReturnValueOnce(oldA.promise) + .mockReturnValueOnce(boardB.promise) + .mockReturnValueOnce(newA.promise) + const store = useIntegrationStore() + + const oldRequest = store.fetchConnectorDetail('connector-a') + const bRequest = store.fetchConnectorDetail('connector-b') + boardB.resolve(detail('connector-b', 'B')) + await bRequest + + const newRequest = store.fetchConnectorDetail('connector-a') + newA.resolve(detail('connector-a', 'A new')) + await newRequest + expect(store.selectedConnector?.name).toBe('A new') + + oldA.resolve(detail('connector-a', 'A old')) + await oldRequest + + expect(store.selectedConnector?.name).toBe('A new') + expect(store.error).toBeNull() + }) + + it('invalidates an old same-id detail across reset without letting its failure clear the new result', async () => { + const oldA = deferred() + const newA = deferred() + vi.mocked(integrationsApi.getConnector) + .mockReturnValueOnce(oldA.promise) + .mockReturnValueOnce(newA.promise) + const store = useIntegrationStore() + + const oldRequest = store.fetchConnectorDetail('connector-a') + store.$reset() + const newRequest = store.fetchConnectorDetail('connector-a') + newA.resolve(detail('connector-a', 'A new')) + await newRequest + + oldA.reject(new Error('old request failed')) + await oldRequest + + expect(store.selectedConnector?.name).toBe('A new') + expect(store.error).toBeNull() + expect(toastMocks.error).not.toHaveBeenCalled() + }) + + it('keeps a reset list empty when the pre-reset request settles', async () => { + const pending = deferred() + vi.mocked(integrationsApi.listConnectors).mockReturnValue(pending.promise) + const store = useIntegrationStore() + + const request = store.fetchConnectors() + store.$reset() + pending.resolve([connector('old', 'Old session')]) + await request + + expect(store.connectors).toEqual([]) + expect(store.loading).toBe(false) + expect(store.error).toBeNull() + }) + + it('keeps the newest list when two reads settle in reverse order', async () => { + const older = deferred() + const newer = deferred() + vi.mocked(integrationsApi.listConnectors) + .mockReturnValueOnce(older.promise) + .mockReturnValueOnce(newer.promise) + const store = useIntegrationStore() + + const oldRequest = store.fetchConnectors() + const newRequest = store.fetchConnectors() + newer.resolve([connector('new', 'New')]) + await newRequest + older.resolve([connector('old', 'Old')]) + await oldRequest + + expect(store.connectors.map(item => item.id)).toEqual(['new']) + }) + + it('keeps loading true until the current list and detail owners both settle', async () => { + const list = deferred() + const selected = deferred() + vi.mocked(integrationsApi.listConnectors).mockReturnValue(list.promise) + vi.mocked(integrationsApi.getConnector).mockReturnValue(selected.promise) + const store = useIntegrationStore() + + const listRequest = store.fetchConnectors() + const detailRequest = store.fetchConnectorDetail('connector-a') + expect(store.loading).toBe(true) + + list.resolve([connector('connector-a', 'A')]) + await listRequest + expect(store.loading).toBe(true) + + selected.resolve(detail('connector-a', 'A')) + await detailRequest + expect(store.loading).toBe(false) + }) +}) From 4e232e22778489681b9db5746e5c221ada7a088a Mon Sep 17 00:00:00 2001 From: Cristian Tcaci <59696583+Chris0Jeky@users.noreply.github.com> Date: Mon, 21 Sep 2026 13:05:24 +0100 Subject: [PATCH 2/2] fix(integrations): bind reads to unique request owners --- .../2026-09-21-integration-read-ownership.md | 37 +++++++++ .../src/store/integrationStore.ts | 75 ++++++++++++++----- .../store/integrationStoreOwnership.spec.ts | 21 ++++++ 3 files changed, 115 insertions(+), 18 deletions(-) create mode 100644 docs/analysis/2026-09-21-integration-read-ownership.md diff --git a/docs/analysis/2026-09-21-integration-read-ownership.md b/docs/analysis/2026-09-21-integration-read-ownership.md new file mode 100644 index 000000000..fd2be2199 --- /dev/null +++ b/docs/analysis/2026-09-21-integration-read-ownership.md @@ -0,0 +1,37 @@ +# Integration-store read ownership + +Status: draft PR #3329, 2026-09-21. Base: `307c3b8b50bec1cb0bfaea3e570a942bcb1d4451`. + +## Reproduced defects + +The selected connector was guarded only by connector ID. An older A request could +therefore become authoritative again after A → B → A, or after reset followed by +a new request for A. List reads had no identity at all, so reverse settlement or +a post-reset response could replace current state. List and detail reads also +shared one last-settler-wins loading Boolean. + +A supplemental runner transpiled and executed the actual store module with only +its framework/API boundaries stubbed. All five original ownership schedules +failed against `main`; the canonical Vitest suite adds the same schedules plus +separate reset success and stale-failure cases. + +## Contract + +- List and detail are independent read lanes, each with a unique request owner. +- Starting a newer request retires the preceding owner for that lane. +- Reset advances an epoch before clearing visible state. +- Success, failure, toast and final loading settlement require current ownership. +- Loading remains true while either current lane still owns visible work. +- Superseded transport may finish, but cannot mutate store state or messaging. + +The integration API, DTOs and mutation behavior are unchanged. This slice does +not claim to invalidate connector mutations that were already sent before reset. + +## Verification and remaining gates + +The actual-module supplemental suite changed from 0/5 ownership cases passing on +`main` to 5/5 after the correction. TypeScript syntax transpilation passes. The +committed Pinia/Vitest regressions require the repository's pinned Node 24 +frontend qualification and exact-head hosted CI. Before review-ready, run lint, +typecheck, build, full coverage tests and independent diff review. No merge, +release or deployment qualification is claimed here. diff --git a/frontend/taskdeck-web/src/store/integrationStore.ts b/frontend/taskdeck-web/src/store/integrationStore.ts index 6bb30b207..0b653ca8c 100644 --- a/frontend/taskdeck-web/src/store/integrationStore.ts +++ b/frontend/taskdeck-web/src/store/integrationStore.ts @@ -19,8 +19,50 @@ export const useIntegrationStore = defineStore('integration', () => { const loading = ref(false) const error = ref(null) - /** Tracks the connector ID for the in-flight detail fetch so late responses are discarded. */ - let _pendingDetailId: string | null = null + type ReadLane = 'list' | 'detail' + interface ReadOwner { + epoch: number + token: symbol + } + + let readEpoch = 0 + const readOwners = new Map() + const activeReadTokens = new Set() + + function syncLoading() { + loading.value = activeReadTokens.size > 0 + } + + function beginRead(lane: ReadLane): ReadOwner { + const previous = readOwners.get(lane) + if (previous?.epoch === readEpoch) activeReadTokens.delete(previous.token) + + const owner = { epoch: readEpoch, token: Symbol(lane) } + readOwners.set(lane, owner) + activeReadTokens.add(owner.token) + error.value = null + syncLoading() + return owner + } + + function ownsRead(lane: ReadLane, owner: ReadOwner): boolean { + const current = readOwners.get(lane) + return owner.epoch === readEpoch && current?.token === owner.token + } + + function finishRead(lane: ReadLane, owner: ReadOwner) { + if (!ownsRead(lane, owner)) return + readOwners.delete(lane) + activeReadTokens.delete(owner.token) + syncLoading() + } + + function invalidateReads() { + readEpoch += 1 + readOwners.clear() + activeReadTokens.clear() + loading.value = false + } function guardDemoMutation(): never | void { if (isDemoMode) { @@ -35,17 +77,20 @@ export const useIntegrationStore = defineStore('integration', () => { error.value = 'Integrations are not available in demo mode.' return } + + const owner = beginRead('list') try { - loading.value = true - error.value = null - connectors.value = await integrationsApi.listConnectors() + const result = await integrationsApi.listConnectors() + if (!ownsRead('list', owner)) return + connectors.value = result } catch (e: unknown) { + if (!ownsRead('list', owner)) return connectors.value = [] const msg = getErrorDisplay(e, 'Failed to fetch integrations').message error.value = msg toast.error(msg) } finally { - loading.value = false + finishRead('list', owner) } } @@ -54,25 +99,20 @@ export const useIntegrationStore = defineStore('integration', () => { error.value = 'Integrations are not available in demo mode.' return } - _pendingDetailId = id + + const owner = beginRead('detail') try { - loading.value = true - error.value = null const result = await integrationsApi.getConnector(id) - // Discard stale response if the user selected a different connector while we were loading - if (_pendingDetailId !== id) return + if (!ownsRead('detail', owner)) return selectedConnector.value = result } catch (e: unknown) { - // Only update state if this is still the active request - if (_pendingDetailId !== id) return + if (!ownsRead('detail', owner)) return const msg = getErrorDisplay(e, 'Failed to fetch connector details').message error.value = msg selectedConnector.value = null toast.error(msg) } finally { - if (_pendingDetailId === id) { - loading.value = false - } + finishRead('detail', owner) } } @@ -166,11 +206,10 @@ export const useIntegrationStore = defineStore('integration', () => { } function $reset() { + invalidateReads() connectors.value = [] selectedConnector.value = null - loading.value = false error.value = null - _pendingDetailId = null } return { diff --git a/frontend/taskdeck-web/src/tests/store/integrationStoreOwnership.spec.ts b/frontend/taskdeck-web/src/tests/store/integrationStoreOwnership.spec.ts index 6e7922187..23de529e6 100644 --- a/frontend/taskdeck-web/src/tests/store/integrationStoreOwnership.spec.ts +++ b/frontend/taskdeck-web/src/tests/store/integrationStoreOwnership.spec.ts @@ -96,6 +96,27 @@ describe('integrationStore request ownership', () => { expect(store.error).toBeNull() }) + it('invalidates an old same-id detail success across reset', async () => { + const oldA = deferred() + const newA = deferred() + vi.mocked(integrationsApi.getConnector) + .mockReturnValueOnce(oldA.promise) + .mockReturnValueOnce(newA.promise) + const store = useIntegrationStore() + + const oldRequest = store.fetchConnectorDetail('connector-a') + store.$reset() + const newRequest = store.fetchConnectorDetail('connector-a') + newA.resolve(detail('connector-a', 'A new')) + await newRequest + + oldA.resolve(detail('connector-a', 'A old')) + await oldRequest + + expect(store.selectedConnector?.name).toBe('A new') + expect(store.error).toBeNull() + }) + it('invalidates an old same-id detail across reset without letting its failure clear the new result', async () => { const oldA = deferred() const newA = deferred()