From 575114056e701a11b1156cd0e0575fa9b6b6c824 Mon Sep 17 00:00:00 2001 From: Cristian Tcaci <59696583+Chris0Jeky@users.noreply.github.com> Date: Mon, 21 Sep 2026 12:48:46 +0100 Subject: [PATCH 1/2] fix(devtools): isolate replay execution lifetimes Give timers and asynchronous handlers generation-scoped ownership so stop, seek and dispose revoke stale continuations. Prevent duplicate play scheduling, preserve pause/resume semantics, guard reentrant state delivery, and add focused lifecycle regression coverage. --- .../2026-09-21-trace-replay-lifetime.md | 48 ++++ .../tests/utils/traceReplayLifecycle.spec.ts | 247 ++++++++++++++++++ .../taskdeck-web/src/utils/traceReplay.ts | 122 +++++---- 3 files changed, 366 insertions(+), 51 deletions(-) create mode 100644 docs/analysis/2026-09-21-trace-replay-lifetime.md create mode 100644 frontend/taskdeck-web/src/tests/utils/traceReplayLifecycle.spec.ts diff --git a/docs/analysis/2026-09-21-trace-replay-lifetime.md b/docs/analysis/2026-09-21-trace-replay-lifetime.md new file mode 100644 index 0000000000..331e443c52 --- /dev/null +++ b/docs/analysis/2026-09-21-trace-replay-lifetime.md @@ -0,0 +1,48 @@ +# Trace replay execution lifetime + +Status: unpublished draft candidate, 2026-09-21. Base: `307c3b8b50bec1cb0bfaea3e570a942bcb1d4451`. + +## Reproduced defects + +Repeated `play()` calls could schedule duplicate delivery. A handler awaiting a +promise retained permission to advance state, emit an error or schedule work +after `stop`, `seekTo` or `dispose`. Pause/resume during that promise could invoke +the same action again. Reentrant state listeners could receive an obsolete state +after a preceding listener had stopped playback. Non-finite speed and fractional +or non-finite seek indices were accepted. + +## Contract and structure + +One timer and one active execution own the current generation. Stop, seek and +dispose revoke that ownership; every awaited handler checks it before committing +state or proceeding to another handler. Timers capture an immutable index and +generation. Timer cleanup and scheduling have one implementation each. + +Pause allows the already-running action to settle once, but schedules no +successor. Resume does not invoke that pending action again. Disposal is final; +subsequent controls and handler registration do nothing. A completed/error trace +can be sought and resumed at the selected index. State-emission revisions stop +obsolete notifications after synchronous observer reentry. + +This engine cannot undo or cancel external effects already started by a handler. +It suppresses obsolete continuations; it does not claim transactional cancellation. +The engine is internal tooling, currently used by `DevToolsView.vue` for analysis. +No routes, application data, dependency manifests, schemas or CI controls change. + +## Verification and remaining gates + +Supplemental direct-production execution on Node 22 passes 22 cases; the same +final suite exposes 17 failures on the original code. A self-review added the +reentrant-observer regression, observed it fail, then corrected it. +A standalone TypeScript 5.8.3 production-module check passes. + +A new `traceReplayLifecycle.spec.ts` contains canonical Vitest regressions. It is +not added to the existing spec quarantine. The established `traceReplay.spec.ts` +is retained unchanged. Neither Vitest suite has been run here: the required Node +24 runtime/dependencies were unavailable and npm registry DNS failed. + +Before ready-for-review: run both replay suites and the full frontend lint, +typecheck, build and Vitest commands on the pinned toolchain; require exact-head +hosted CI and independent review. Particular review targets are pending-handler +pause/resume semantics, reentrant callbacks and final disposal. No merge, release +qualification or independent-review approval is claimed. diff --git a/frontend/taskdeck-web/src/tests/utils/traceReplayLifecycle.spec.ts b/frontend/taskdeck-web/src/tests/utils/traceReplayLifecycle.spec.ts new file mode 100644 index 0000000000..86cc5312ac --- /dev/null +++ b/frontend/taskdeck-web/src/tests/utils/traceReplayLifecycle.spec.ts @@ -0,0 +1,247 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { createReplayEngine, type TraceReplayEngine } from '../../utils/traceReplay' +import type { Trace } from '../../types/trace' + +function deferred() { + let resolve!: () => void + let reject!: (reason: Error) => void + const promise = new Promise((yes, no) => { resolve = yes; reject = no }) + return { promise, resolve, reject } +} + +const engines: TraceReplayEngine[] = [] +function createEngine() { + const trace: Trace = { + id: 'lifecycle', name: 'Lifecycle', startedAt: '2026-09-21T00:00:00Z', + endedAt: '2026-09-21T00:00:01Z', durationMs: 200, + actions: [0, 1, 2].map(index => ({ + id: `a${index}`, type: 'click', timestamp: '2026-09-21T00:00:00Z', + offsetMs: index * 100, label: `Action ${index}`, payload: {}, + })), + } + const engine = createReplayEngine(trace) + engines.push(engine) + return engine +} + +describe('replay execution ownership', () => { + beforeEach(() => { vi.useFakeTimers() }) + afterEach(() => { + for (const engine of engines.splice(0)) engine.dispose() + vi.useRealTimers() + }) + + it('does not duplicate delivery when Play is pressed twice', async () => { + const engine = createEngine() + const executed: string[] = [] + engine.onAction(action => { executed.push(action.id) }) + engine.play() + engine.play() + await vi.advanceTimersByTimeAsync(0) + expect(executed).toEqual(['a0']) + await vi.advanceTimersByTimeAsync(100) + expect(executed).toEqual(['a0', 'a1']) + }) + + it('does not let a stopped action advance or emit state after settlement', async () => { + const engine = createEngine() + const pending = deferred() + const stateChanged = vi.fn() + engine.onAction(() => pending.promise) + engine.onStateChange(stateChanged) + engine.play() + await vi.advanceTimersByTimeAsync(0) + engine.stop() + const snapshot = engine.getState() + stateChanged.mockClear() + pending.resolve() + await vi.advanceTimersByTimeAsync(500) + expect(engine.getState()).toEqual(snapshot) + expect(stateChanged).not.toHaveBeenCalled() + }) + + it.each(['resolve', 'reject'] as const)( + 'ignores an obsolete action that will %s after a replacement run starts', async (settlement) => { + const engine = createEngine() + const pending = deferred() + const executed: string[] = [] + engine.onAction(action => { + executed.push(action.id) + if (executed.length === 1) return pending.promise + }) + engine.play() + await vi.advanceTimersByTimeAsync(0) + engine.stop() + engine.play() + await vi.advanceTimersByTimeAsync(0) + expect(engine.getState().currentIndex).toBe(1) + if (settlement === 'resolve') pending.resolve() + else pending.reject(new Error('obsolete run')) + await vi.advanceTimersByTimeAsync(0) + expect(engine.getState().status).toBe('playing') + await vi.advanceTimersByTimeAsync(100) + expect(executed).toEqual(['a0', 'a0', 'a1']) + }, + ) + + it('preserves a seek target when a previous action settles', async () => { + const engine = createEngine() + const pending = deferred() + engine.onAction(() => pending.promise) + engine.play() + await vi.advanceTimersByTimeAsync(0) + engine.seekTo(2) + pending.resolve() + await vi.advanceTimersByTimeAsync(0) + expect(engine.getState()).toMatchObject({ status: 'paused', currentIndex: 2 }) + }) + + it('freezes a disposed engine even when an action is pending', async () => { + const engine = createEngine() + const pending = deferred() + engine.onAction(() => pending.promise) + engine.play() + await vi.advanceTimersByTimeAsync(0) + engine.dispose() + const snapshot = engine.getState() + pending.resolve() + await vi.advanceTimersByTimeAsync(500) + expect(engine.getState()).toEqual(snapshot) + expect(vi.getTimerCount()).toBe(0) + }) + + it('does not restart or register handlers after disposal', async () => { + const engine = createEngine() + engine.dispose() + const snapshot = engine.getState() + const action = vi.fn() + const state = vi.fn() + engine.onAction(action) + engine.onStateChange(state) + engine.play() + engine.seekTo(1) + engine.setSpeed(2) + engine.stop() + engine.pause() + await vi.advanceTimersByTimeAsync(500) + expect(engine.getState()).toEqual(snapshot) + expect(action).not.toHaveBeenCalled() + expect(state).not.toHaveBeenCalled() + expect(vi.getTimerCount()).toBe(0) + }) + + it('resumes a pending action without invoking its handler again', async () => { + const engine = createEngine() + const pending = deferred() + const executed: string[] = [] + engine.onAction(action => { + executed.push(action.id) + if (action.id === 'a0') return pending.promise + }) + engine.play() + await vi.advanceTimersByTimeAsync(0) + engine.pause() + engine.play() + await vi.advanceTimersByTimeAsync(0) + expect(executed).toEqual(['a0']) + pending.resolve() + await vi.advanceTimersByTimeAsync(100) + expect(executed).toEqual(['a0', 'a1']) + }) + + it('lets a paused action finish once without starting the next action', async () => { + const engine = createEngine() + const pending = deferred() + const executed: string[] = [] + engine.onAction(action => { + executed.push(action.id) + if (action.id === 'a0') return pending.promise + }) + engine.play() + await vi.advanceTimersByTimeAsync(0) + engine.pause() + pending.resolve() + await vi.advanceTimersByTimeAsync(500) + expect(engine.getState()).toMatchObject({ status: 'paused', currentIndex: 1 }) + expect(executed).toEqual(['a0']) + engine.play() + await vi.advanceTimersByTimeAsync(0) + expect(executed).toEqual(['a0', 'a1']) + }) + + it('stops delivery to the remaining handlers when a handler stops playback', async () => { + const engine = createEngine() + const nextHandler = vi.fn() + engine.onAction(() => engine.stop()) + engine.onAction(nextHandler) + engine.play() + await vi.advanceTimersByTimeAsync(0) + expect(nextHandler).not.toHaveBeenCalled() + expect(engine.getState()).toMatchObject({ status: 'idle', currentIndex: 0 }) + }) + + it('honours a state observer that pauses before the first timer is installed', async () => { + const engine = createEngine() + const action = vi.fn() + engine.onAction(action) + engine.onStateChange(state => { if (state.status === 'playing') engine.pause() }) + engine.play() + await vi.advanceTimersByTimeAsync(500) + expect(action).not.toHaveBeenCalled() + expect(vi.getTimerCount()).toBe(0) + }) + + it('schedules only one successor after an observer pauses and resumes', async () => { + const engine = createEngine() + const executed: string[] = [] + let toggled = false + engine.onAction(action => { executed.push(action.id) }) + engine.onStateChange(state => { + if (!toggled && state.currentIndex === 1) { + toggled = true + engine.pause() + engine.play() + } + }) + engine.play() + await vi.advanceTimersByTimeAsync(100) + expect(executed).toEqual(['a0', 'a1']) + }) + + it('honours a seek after completion instead of restarting at zero', async () => { + const engine = createEngine() + const executed: string[] = [] + engine.onAction(action => { executed.push(action.id) }) + engine.play() + await vi.advanceTimersByTimeAsync(200) + expect(engine.getState().status).toBe('completed') + engine.seekTo(1) + engine.play() + await vi.advanceTimersByTimeAsync(0) + expect(executed).toEqual(['a0', 'a1', 'a2', 'a1']) + }) + + it.each([NaN, Infinity, -Infinity, 0.5])('ignores an invalid seek index %s', (index) => { + const engine = createEngine() + const snapshot = engine.getState() + engine.seekTo(index) + expect(engine.getState()).toEqual(snapshot) + }) + + it.each([NaN, Infinity, -Infinity])('ignores a non-finite speed %s', (speed) => { + const engine = createEngine() + engine.setSpeed(speed) + expect(engine.getState().playbackSpeed).toBe(1) + }) + + it('does not deliver obsolete state to later observers after a reentrant stop', async () => { + const engine = createEngine() + const statuses: string[] = [] + engine.onStateChange(state => { if (state.status === 'playing') engine.stop() }) + engine.onStateChange(state => { statuses.push(state.status) }) + engine.play() + await vi.advanceTimersByTimeAsync(0) + expect(statuses).toEqual(['idle']) + }) + +}) diff --git a/frontend/taskdeck-web/src/utils/traceReplay.ts b/frontend/taskdeck-web/src/utils/traceReplay.ts index 8a447d018d..4b084c1a77 100644 --- a/frontend/taskdeck-web/src/utils/traceReplay.ts +++ b/frontend/taskdeck-web/src/utils/traceReplay.ts @@ -15,7 +15,7 @@ export interface TraceReplayEngine { getState: () => ReplayState /** Start or resume playback. */ play: () => void - /** Pause playback. */ + /** Pause after any already-running action settles, without starting a successor. */ pause: () => void /** Stop playback and reset to the beginning. */ stop: () => void @@ -27,7 +27,7 @@ export interface TraceReplayEngine { onAction: (handler: ReplayActionHandler) => void /** Register a handler called when replay state changes. */ onStateChange: (handler: (state: ReplayState) => void) => void - /** Clean up timers. */ + /** Permanently dispose timers/handlers and invalidate pending settlements. */ dispose: () => void } @@ -36,6 +36,10 @@ export function createReplayEngine(trace: Trace): TraceReplayEngine { let currentIndex = 0 let playbackSpeed = 1 let timerId: ReturnType | null = null + let generation = 0 + let stateRevision = 0 + let activeExecution: object | null = null + let disposed = false const actionHandlers: ReplayActionHandler[] = [] const stateHandlers: Array<(state: ReplayState) => void> = [] @@ -52,45 +56,70 @@ export function createReplayEngine(trace: Trace): TraceReplayEngine { } } - function emitStateChange(): void { + function emitStateChange(error?: string): void { + const revision = ++stateRevision const state = buildState() + if (error !== undefined) state.error = error for (const handler of stateHandlers) { + // Observers can synchronously stop, seek, pause, or dispose playback. + if (disposed || stateRevision !== revision) return handler(state) } } - async function executeAction(index: number): Promise { - if (index >= trace.actions.length) { - status = 'completed' - emitStateChange() - return - } + function ownsPlayback(ownerGeneration: number): boolean { + return !disposed && generation === ownerGeneration + } + + function clearTimer(): void { + if (timerId !== null) clearTimeout(timerId) + timerId = null + } + function invalidatePlayback(): void { + clearTimer() + generation += 1 + activeExecution = null + } + + function scheduleAction(index: number, delay: number, ownerGeneration: number): void { + if (!ownsPlayback(ownerGeneration) || status !== 'playing' || timerId !== null || activeExecution) return + timerId = setTimeout(() => { + if (!ownsPlayback(ownerGeneration)) return + timerId = null + void executeAction(index, ownerGeneration) + }, Math.max(0, delay)) + } + + async function executeAction(index: number, ownerGeneration: number): Promise { + if (!ownsPlayback(ownerGeneration) || status !== 'playing') return + const owner = {} + activeExecution = owner const action = trace.actions[index] for (const handler of actionHandlers) { try { await handler(action, index) } catch (err) { + if (!ownsPlayback(ownerGeneration) || activeExecution !== owner) return + activeExecution = null status = 'error' - const state = buildState() - state.error = err instanceof Error ? err.message : String(err) - for (const h of stateHandlers) { - h(state) - } + emitStateChange(err instanceof Error ? err.message : String(err)) return } + // Stop/seek/dispose cannot cancel the handler's external work, but they do + // revoke its authority to deliver more handlers or commit replay state. + if (!ownsPlayback(ownerGeneration) || activeExecution !== owner) return } currentIndex = index + 1 emitStateChange() - + if (!ownsPlayback(ownerGeneration) || activeExecution !== owner) return + activeExecution = null if (status !== 'playing') return - // Schedule next action based on timing delta if (currentIndex < trace.actions.length) { - const nextAction = trace.actions[currentIndex] - const delay = (nextAction.offsetMs - action.offsetMs) / playbackSpeed - timerId = setTimeout(() => executeAction(currentIndex), Math.max(0, delay)) + const delay = (trace.actions[currentIndex].offsetMs - action.offsetMs) / playbackSpeed + scheduleAction(currentIndex, delay, ownerGeneration) } else { status = 'completed' emitStateChange() @@ -98,79 +127,70 @@ export function createReplayEngine(trace: Trace): TraceReplayEngine { } function play(): void { + if (disposed || status === 'playing') return if (trace.actions.length === 0) { status = 'completed' emitStateChange() return } + if (status === 'completed') currentIndex = 0 - if (status === 'completed') { - currentIndex = 0 - } - + const ownerGeneration = generation status = 'playing' emitStateChange() + if (!ownsPlayback(ownerGeneration) || status !== 'playing' || activeExecution) return if (currentIndex < trace.actions.length) { - const action = trace.actions[currentIndex] - const delay = currentIndex === 0 - ? action.offsetMs / playbackSpeed - : 0 - timerId = setTimeout(() => executeAction(currentIndex), Math.max(0, delay)) + const delay = currentIndex === 0 ? trace.actions[currentIndex].offsetMs / playbackSpeed : 0 + scheduleAction(currentIndex, delay, ownerGeneration) + } else { + status = 'completed' + emitStateChange() } } function pause(): void { - if (status !== 'playing') return - if (timerId !== null) { - clearTimeout(timerId) - timerId = null - } + if (disposed || status !== 'playing') return + clearTimer() status = 'paused' emitStateChange() } function stop(): void { - if (timerId !== null) { - clearTimeout(timerId) - timerId = null - } + if (disposed) return + invalidatePlayback() status = 'idle' currentIndex = 0 emitStateChange() } function seekTo(index: number): void { - if (index < 0 || index >= trace.actions.length) return - const wasPlaying = status === 'playing' - if (timerId !== null) { - clearTimeout(timerId) - timerId = null - } + if (disposed || !Number.isInteger(index) || index < 0 || index >= trace.actions.length) return + invalidatePlayback() currentIndex = index - status = wasPlaying ? 'paused' : status + // A completed/error cursor must become resumable at the selected index. + if (status !== 'idle') status = 'paused' emitStateChange() } function setSpeed(speed: number): void { - if (speed <= 0) return + if (disposed || !Number.isFinite(speed) || speed <= 0) return playbackSpeed = speed emitStateChange() } function onAction(handler: ReplayActionHandler): void { - actionHandlers.push(handler) + if (!disposed) actionHandlers.push(handler) } function onStateChange(handler: (state: ReplayState) => void): void { - stateHandlers.push(handler) + if (!disposed) stateHandlers.push(handler) } function dispose(): void { - if (timerId !== null) { - clearTimeout(timerId) - timerId = null - } + if (disposed) return + disposed = true + invalidatePlayback() actionHandlers.length = 0 stateHandlers.length = 0 } From b51fd55fbcc477b15503f428f3cbab64a93b4414 Mon Sep 17 00:00:00 2001 From: Cristian Tcaci <59696583+Chris0Jeky@users.noreply.github.com> Date: Mon, 21 Sep 2026 12:49:27 +0100 Subject: [PATCH 2/2] docs(devtools): link replay evidence to draft PR --- docs/analysis/2026-09-21-trace-replay-lifetime.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/analysis/2026-09-21-trace-replay-lifetime.md b/docs/analysis/2026-09-21-trace-replay-lifetime.md index 331e443c52..baf67dc45a 100644 --- a/docs/analysis/2026-09-21-trace-replay-lifetime.md +++ b/docs/analysis/2026-09-21-trace-replay-lifetime.md @@ -1,6 +1,6 @@ # Trace replay execution lifetime -Status: unpublished draft candidate, 2026-09-21. Base: `307c3b8b50bec1cb0bfaea3e570a942bcb1d4451`. +Status: draft PR #3326, 2026-09-21. Base: `307c3b8b50bec1cb0bfaea3e570a942bcb1d4451`. ## Reproduced defects