diff --git a/apps/web/e2e/sprint-25-disable-pvm-divergence.spec.ts b/apps/web/e2e/sprint-25-disable-pvm-divergence.spec.ts new file mode 100644 index 0000000..6c275a3 --- /dev/null +++ b/apps/web/e2e/sprint-25-disable-pvm-divergence.spec.ts @@ -0,0 +1,129 @@ +import { expect, test } from "@playwright/test"; + +test.describe("Sprint 25 — No divergence after disabling second PVM", () => { + /** Load a program and wait for the debugger page to be visible. */ + async function loadProgram(page: import("@playwright/test").Page) { + await page.goto("/#/load"); + const card = page.getByTestId("example-card-step-test"); + await expect(card).toBeVisible(); + await card.click(); + await expect(page.getByTestId("debugger-page")).toBeVisible({ + timeout: 15000, + }); + } + + /** Open the settings tab in the bottom drawer. */ + async function openSettings(page: import("@playwright/test").Page) { + await page.getByTestId("drawer-tab-settings").click(); + await expect(page.getByTestId("settings-tab")).toBeVisible(); + } + + /** + * Attempt to enable the Ananas PVM via settings. + * Returns true if both PVM tabs appear, false otherwise. + */ + async function tryEnableAnanas( + page: import("@playwright/test").Page, + ): Promise { + await openSettings(page); + const ananasSwitch = page.getByTestId("pvm-switch-ananas"); + await expect(ananasSwitch).toBeVisible(); + await ananasSwitch.click(); + try { + await expect(page.getByTestId("pvm-tab-ananas")).toHaveRole("tab", { + timeout: 15000, + }); + return true; + } catch { + return false; + } + } + + test("no divergence after enabling then disabling second PVM", async ({ + page, + }) => { + await loadProgram(page); + + // Enable ananas + const enabled = await tryEnableAnanas(page); + expect(enabled).toBe(true); + + // Wait for both PVMs to be fully loaded (blue dots = paused state) + await expect(page.getByTestId("pvm-dot-typeberry")).toHaveClass( + /bg-blue-500/, + { timeout: 15000 }, + ); + await expect(page.getByTestId("pvm-dot-ananas")).toHaveClass( + /bg-blue-500/, + { timeout: 15000 }, + ); + + // Both freshly loaded PVMs should not diverge + await expect(page.getByTestId("divergence-summary")).not.toBeVisible(); + + // Disable ananas (settings already open from tryEnableAnanas) + const ananasSwitch = page.getByTestId("pvm-switch-ananas"); + await ananasSwitch.click(); + + // Wait for ananas to be removed (reverts to grayed-out span, no role="tab") + await expect(page.getByTestId("pvm-tab-ananas")).not.toHaveAttribute( + "role", + "tab", + { timeout: 15000 }, + ); + + // Should NOT show divergence with only one PVM + await expect(page.getByTestId("divergence-summary")).not.toBeVisible(); + + // Typeberry should still be functional (blue dot = paused) + await expect(page.getByTestId("pvm-dot-typeberry")).toHaveClass( + /bg-blue-500/, + { timeout: 15000 }, + ); + }); + + test("no divergence after stepping, enabling, then disabling second PVM", async ({ + page, + }) => { + await loadProgram(page); + + // Step a few times with just typeberry + const nextBtn = page.getByTestId("next-button"); + await expect(nextBtn).toBeVisible({ timeout: 10000 }); + await nextBtn.click(); + await expect(nextBtn).toBeEnabled({ timeout: 5000 }); + await nextBtn.click(); + await expect(nextBtn).toBeEnabled({ timeout: 5000 }); + + // Enable ananas (this resets the session — both PVMs start fresh) + const enabled = await tryEnableAnanas(page); + expect(enabled).toBe(true); + + // Wait for both PVMs to load + await expect(page.getByTestId("pvm-dot-typeberry")).toHaveClass( + /bg-blue-500/, + { timeout: 15000 }, + ); + await expect(page.getByTestId("pvm-dot-ananas")).toHaveClass( + /bg-blue-500/, + { timeout: 15000 }, + ); + + // No divergence — both loaded fresh with identical state + await expect(page.getByTestId("divergence-summary")).not.toBeVisible(); + + // Disable ananas + const ananasSwitch = page.getByTestId("pvm-switch-ananas"); + await ananasSwitch.click(); + + // Wait for transition + await expect(page.getByTestId("pvm-tab-ananas")).not.toHaveAttribute( + "role", + "tab", + { timeout: 15000 }, + ); + + // No divergence after disabling + await expect(page.getByTestId("divergence-summary")).not.toBeVisible(); + }); +}); diff --git a/apps/web/src/hooks/useOrchestratorState.ts b/apps/web/src/hooks/useOrchestratorState.ts index 4be2c82..8820221 100644 --- a/apps/web/src/hooks/useOrchestratorState.ts +++ b/apps/web/src/hooks/useOrchestratorState.ts @@ -59,12 +59,19 @@ export function useOrchestratorState(): OrchestratorReactiveState { const pendingPerPvmErrors = useRef | null>(null); const pendingStepDone = useRef(false); const rafId = useRef(null); + // Tracks the most recently flushed snapshots so post-flush event handlers + // don't fall back to the stale `initial` closure captured at subscription time. + const lastFlushedSnapshots = useRef | null>(null); const scheduleFlush = useCallback(() => { if (rafId.current !== null) return; rafId.current = requestAnimationFrame(() => { rafId.current = null; if (pendingSnapshots.current) { + lastFlushedSnapshots.current = pendingSnapshots.current; setSnapshots(pendingSnapshots.current); pendingSnapshots.current = null; } @@ -91,6 +98,7 @@ export function useOrchestratorState(): OrchestratorReactiveState { useEffect(() => { // Discard any buffered data from the previous orchestrator pendingSnapshots.current = null; + lastFlushedSnapshots.current = null; pendingHostCallInfo.current = null; pendingPerPvmErrors.current = null; pendingSelectedPvmId.current = null; @@ -121,15 +129,22 @@ export function useOrchestratorState(): OrchestratorReactiveState { prev && pvmIds.includes(prev) ? prev : (pvmIds[0] ?? null), ); - // Use `initial` (from this orchestrator) as the fallback base for event - // accumulation — NOT the stale `snapshots` React state from the closure. - // This prevents disabled-PVM data from leaking into the new orchestrator. + // Event handlers accumulate into `pendingSnapshots`. After each rAF flush + // clears it, the fallback base is `lastFlushedSnapshots` (most recent flush), + // then `initial` (pre-loadProgram state from this orchestrator). This chain + // prevents two bugs: + // 1. Stale React state leaking disabled-PVM data into a new orchestrator + // 2. Post-flush events reverting already-loaded PVM state to pre-load defaults + // (the rAF race: PVM A loads → flush → PVM B loads → fallback to `initial` + // would lose A's loaded state without `lastFlushedSnapshots`) const onStateChanged = ( pvmId: string, snapshot: MachineStateSnapshot, lifecycle: PvmLifecycle, ) => { - const base = pendingSnapshots.current ?? new Map(initial); + const base = + pendingSnapshots.current ?? + new Map(lastFlushedSnapshots.current ?? initial); base.set(pvmId, { snapshot, lifecycle }); pendingSnapshots.current = base; pendingSelectedPvmId.current = pvmId; @@ -169,7 +184,9 @@ export function useOrchestratorState(): OrchestratorReactiveState { }; const onTerminated = (pvmId: string, reason: PvmStatus) => { - const base = pendingSnapshots.current ?? new Map(initial); + const base = + pendingSnapshots.current ?? + new Map(lastFlushedSnapshots.current ?? initial); const entry = base.get(pvmId); if (entry) { base.set(pvmId, { @@ -187,7 +204,9 @@ export function useOrchestratorState(): OrchestratorReactiveState { const isTimeout = /timeout/i.test(error.message); const lifecycle: PvmLifecycle = isTimeout ? "timed_out" : "failed"; - const base = pendingSnapshots.current ?? new Map(initial); + const base = + pendingSnapshots.current ?? + new Map(lastFlushedSnapshots.current ?? initial); const entry = base.get(pvmId); if (entry) { base.set(pvmId, { snapshot: entry.snapshot, lifecycle }); diff --git a/apps/web/src/pages/DebuggerPage.tsx b/apps/web/src/pages/DebuggerPage.tsx index c02da88..eab2725 100644 --- a/apps/web/src/pages/DebuggerPage.tsx +++ b/apps/web/src/pages/DebuggerPage.tsx @@ -44,8 +44,17 @@ function DebuggerPageInner() { snapshotVersion, perPvmErrors, } = useOrchestratorState(); - const { summary: divergenceSummary, details: divergenceDetails } = + const { summary: rawDivergenceSummary, details: rawDivergenceDetails } = useDivergenceCheck(snapshots, selectedPvmId, snapshotVersion); + // Suppress divergence during PVM reload — workers respond at different + // times so there's a brief window where one PVM has loaded state and the + // other still has defaults, producing a false divergence flash. + const divergenceSummary = isReloadingRef.current + ? null + : rawDivergenceSummary; + const divergenceDetails = isReloadingRef.current + ? null + : rawDivergenceDetails; const instructions = useDisassembly(envelope); const { activeTab, openToTab, setActiveTab } = useDrawer(); diff --git a/spec/ui/sprint-25-divergence-detection.md b/spec/ui/sprint-25-divergence-detection.md index bce99e0..e9e663b 100644 --- a/spec/ui/sprint-25-divergence-detection.md +++ b/spec/ui/sprint-25-divergence-detection.md @@ -14,6 +14,8 @@ When multiple PVMs are active, detect and surface divergences between them. Show - No divergence output when fewer than 2 PVMs are active - Failed/timed-out PVMs show inline red error text - Divergence clears after reset +- No false divergence when enabling or disabling the second PVM +- No divergence flash during PVM reload (workers responding at different times) ## Prior Sprint Dependencies @@ -23,8 +25,11 @@ When multiple PVMs are active, detect and surface divergences between them. Show ``` apps/web/src/hooks/useDivergenceCheck.ts -apps/web/src/components/debugger/PvmTabs.tsx (extend with divergence display) +apps/web/src/hooks/useOrchestratorState.ts (lastFlushedSnapshots ref) +apps/web/src/pages/DebuggerPage.tsx (isReloadingRef divergence gate) +apps/web/src/components/debugger/PvmTabs.tsx (extend with divergence display) apps/web/e2e/sprint-25-divergence.spec.ts +apps/web/e2e/sprint-25-disable-pvm-divergence.spec.ts ``` ## Divergence Contract @@ -73,6 +78,8 @@ Implementation pitfall: failed/timed-out sessions do not emit `pvmStateChanged`; - divergence clears after reset - error text appears inline for failed PVMs - no divergence shown with single PVM +- no divergence after enabling then disabling second PVM +- no divergence after stepping, enabling, then disabling second PVM ``` ## Acceptance Criteria @@ -81,6 +88,8 @@ Implementation pitfall: failed/timed-out sessions do not emit `pvmStateChanged`; - Summary is concise; tooltip has full details. - Failed/timed-out PVMs show inline error text. - Divergence clears after reset. +- No false divergence when enabling or disabling PVMs. +- No divergence flash during PVM reload. - `cd apps/web && npx vite build` succeeds. - E2E tests pass. @@ -94,9 +103,18 @@ Implementation pitfall: failed/timed-out sessions do not emit `pvmStateChanged`; - Multi-PVM E2E tests depend on PVM switching via settings, which has a pre-existing timing issue from sprint-24 (orchestrator reload clears program state briefly). These tests are conditionally skipped with `test.skip` when PVM switching doesn't stabilize. - Unit tests (22 total) thoroughly cover: all divergence field types, concise summary formatting, edge cases (empty snapshots, missing PVM ID), error display for failed/timed-out PVMs, and tooltip content. +### False divergence on PVM enable/disable + +Two bugs caused false divergence when toggling the second PVM: + +1. **Stale-closure race in rAF flush** (`useOrchestratorState`): The `onStateChanged` handler accumulated into `pendingSnapshots`, using `new Map(initial)` as fallback after each rAF flush cleared `pendingSnapshots.current`. The `initial` variable was captured before `loadProgram` completed, so when two PVM workers responded across different animation frames, the second event reverted the first PVM's loaded state to pre-load defaults (e.g., gas=0 instead of gas=10000). Fix: a `lastFlushedSnapshots` ref tracks the most recently flushed snapshot map, providing an up-to-date fallback base. The chain is: `pendingSnapshots.current ?? new Map(lastFlushedSnapshots.current ?? initial)`. + +2. **Divergence flash during reload** (`DebuggerPage`): Even with the above fix, PVM workers respond at different speeds during `loadProgram`. For one render frame, one PVM has loaded state while the other still has defaults. Fix: divergence results are gated behind `isReloadingRef`, which is already `true` for the duration of `loadProgram` in `onPvmChange`. Since snapshot updates (which trigger renders) arrive while the ref is still `true`, the flash is suppressed. When `loadProgram` finishes and the ref becomes `false`, both PVMs have matching state so divergence is null anyway. + ## Verification ```bash cd apps/web && npx vite build npx playwright test e2e/sprint-25-divergence.spec.ts +npx playwright test e2e/sprint-25-disable-pvm-divergence.spec.ts ```