diff --git a/ui/src/__tests__/exclusion-preview-store.test.ts b/ui/src/__tests__/exclusion-preview-store.test.ts index 05a61767..2e4925b9 100644 --- a/ui/src/__tests__/exclusion-preview-store.test.ts +++ b/ui/src/__tests__/exclusion-preview-store.test.ts @@ -44,6 +44,7 @@ import { createExclusionPreview, isUnconstrainedIncludePattern, unconstrainedIncludePatterns, + PRE_ID_PARK_CAP, type ExclusionPreviewController, } from "../stores/exclusionPreview"; import type { ExclusionPreviewBatch, ExclusionPreviewNode } from "../ipc/types"; @@ -104,6 +105,65 @@ beforeEach(() => { unlistenError.mockReset(); }); +describe("pre-generation-id park", () => { + // A controller whose `start` never resolves a generation id leaves `currentId` + // null FOREVER, while its `listen()` registrations - which are global by event + // name - keep receiving every later preview's batches. Nothing drains the park + // in that state, so the park is the one place in this controller where events + // can pile up unboundedly. This pins the bound. + it("caps the park so a controller that never resolves an id cannot grow without bound", async () => { + const preview = createExclusionPreview(); + await preview.subscribe(); + // The start command REJECTS, so no generation id is ever adopted. + invokeMock.mockRejectedValue(new Error("start failed")); + await preview.start({ + sourceId: "src-1", + respectGitignore: true, + includePatterns: [], + excludePatterns: [], + }); + expect(preview.currentPreviewId()).toBeNull(); + + // Four caps' worth of traffic from OTHER previews still arrives, because the + // listeners are registered by event name rather than per generation. + const fired = PRE_ID_PARK_CAP * 4; + for (let i = 0; i < fired; i += 1) { + batchHandler!(batch(`gen-${i}`, [node(`f${i}.txt`, false, true, 1)])); + } + + expect(preview.preIdParkedCount()).toBe(PRE_ID_PARK_CAP); + expect(preview.preIdParkedCount()).toBeLessThan(fired); + }); + + it("still parks and replays everything that arrives before a real id lands", async () => { + const preview = createExclusionPreview(); + await preview.subscribe(); + let resolveStart: (id: string) => void = () => {}; + invokeMock.mockReturnValue( + new Promise((resolve) => { + resolveStart = resolve; + }) + ); + const starting = preview.start({ + sourceId: "src-1", + respectGitignore: true, + includePatterns: [], + excludePatterns: [], + }); + // The walk streams before `preview_exclusions_start` resolves. + batchHandler!(batch("gen-1", [node("dir", true, true), node("dir/a.txt", false, true, 7)])); + expect(preview.preIdParkedCount()).toBe(1); + + resolveStart("gen-1"); + await starting; + preview.flush(); + + expect(preview.preIdParkedCount()).toBe(0); + expect(preview.nodeAt("dir/a.txt")?.included).toBe(true); + expect(preview.includedCount.value).toBe(1); + }); +}); + describe("anchoredPatternForPath", () => { // THE SAME TABLE the Rust `anchored_pattern_vectors_are_stable` test asserts // against `driven_core::exclude::anchored_pattern_for_path` (which is in turn diff --git a/ui/src/__tests__/exclusion-preview-tree.test.ts b/ui/src/__tests__/exclusion-preview-tree.test.ts index 2081bfd3..af6cb7e1 100644 --- a/ui/src/__tests__/exclusion-preview-tree.test.ts +++ b/ui/src/__tests__/exclusion-preview-tree.test.ts @@ -18,6 +18,12 @@ vi.mock("@tauri-apps/api/core", () => ({ let batchHandler: ((payload: unknown) => void) | null = null; let doneHandler: ((payload: unknown) => void) | null = null; let errorHandler: ((payload: unknown) => void) | null = null; +/** Every unlisten handed out by `listen()`, so a test can assert that NONE is + * left registered after the component goes away. */ +const unlistenSpies: Array> = []; +/** When set, `listen()` awaits this before resolving - which is what lets a test + * unmount the component while `subscribe()` is still in flight. */ +let listenGate: Promise | null = null; vi.mock("@tauri-apps/api/event", () => ({ listen: vi.fn(async (event: string, cb: (e: { payload: unknown }) => void) => { if (event === "exclusion_preview:batch") { @@ -29,7 +35,10 @@ vi.mock("@tauri-apps/api/event", () => ({ if (event === "exclusion_preview:error") { errorHandler = (payload: unknown) => cb({ payload }); } - return vi.fn(); + if (listenGate) await listenGate; + const unlisten = vi.fn(); + unlistenSpies.push(unlisten); + return unlisten; }), })); @@ -88,6 +97,59 @@ beforeEach(() => { batchHandler = null; doneHandler = null; errorHandler = null; + unlistenSpies.length = 0; + listenGate = null; +}); + +describe("ExclusionPreviewTree teardown", () => { + it("tears down every listener when unmounted while subscribe is still in flight", async () => { + // The editor mounts under a `v-if` (SourceTable's inline editor, + // AddSourceWizard's exclusions step), so opening and immediately closing it + // is ordinary use. `subscribe()` is three async `listen()` round-trips; if + // the component unmounts inside that window, the resolved unlisteners must + // still be invoked. They cannot be recovered later: `listen` registers + // GLOBALLY BY EVENT NAME, so a stranded set keeps receiving every later + // preview's batches and parks them in a controller nobody can reach. + let openGate: () => void = () => {}; + listenGate = new Promise((resolve) => { + openGate = resolve; + }); + + const wrapper = mount(ExclusionPreviewTree, { + global: globalMountOptions, + props: { + sourceId: "src-1", + respectGitignore: true, + includePatterns: [], + excludePatterns: [], + }, + }); + // Nothing has resolved yet - this is the race window. + expect(unlistenSpies).toHaveLength(0); + + wrapper.unmount(); + openGate(); + await flushPromises(); + + expect(unlistenSpies).toHaveLength(3); + for (const unlisten of unlistenSpies) { + expect(unlisten).toHaveBeenCalledTimes(1); + } + // ...and no walk is started for a tree that is no longer on screen. + expect(invokeMock).not.toHaveBeenCalledWith("preview_exclusions_start", expect.anything()); + }); + + it("tears down every listener on an ordinary unmount", async () => { + const wrapper = await mountWithNodes([node("a.txt", false, true, 10)]); + expect(unlistenSpies).toHaveLength(3); + + wrapper.unmount(); + await flushPromises(); + + for (const unlisten of unlistenSpies) { + expect(unlisten).toHaveBeenCalledTimes(1); + } + }); }); describe("ExclusionPreviewTree", () => { diff --git a/ui/src/components/ExclusionPreviewTree.vue b/ui/src/components/ExclusionPreviewTree.vue index 4802b1d8..174abb04 100644 --- a/ui/src/components/ExclusionPreviewTree.vue +++ b/ui/src/components/ExclusionPreviewTree.vue @@ -58,13 +58,36 @@ const expanded = ref(new Set()); const shownLimit = ref(new Map()); let teardown: (() => void) | null = null; +/** Subscription INTENT, re-checked after `subscribe()` resolves. + * + * `subscribe()` is async (three `listen()` round-trips), so a component that + * unmounts while it is in flight would run `onUnmounted` with `teardown` still + * null and tear down nothing - stranding all three listeners for the life of + * the process. Those listeners are registered globally BY EVENT NAME, so the + * orphan keeps receiving every later preview's `exclusion_preview:batch`, and + * its controller (whose generation id never resolved) parks each one forever. + * The editor opens and closes on `v-if`, so losing that race is a normal + * interaction, not a pathological one. + * + * Same shape as `activity.ts`'s `desiredSubscribed`: flip the intent first, + * then have the resolving side honour it. */ +let subscribeWanted = false; onMounted(async () => { - teardown = await preview.subscribe(); + subscribeWanted = true; + const stop = await preview.subscribe(); + if (!subscribeWanted) { + // Unmounted while subscribing: tear the listeners down now, and do NOT + // start a walk nobody is rendering. + stop(); + return; + } + teardown = stop; await restart(); }); onUnmounted(() => { + subscribeWanted = false; teardown?.(); teardown = null; }); diff --git a/ui/src/stores/exclusionPreview.ts b/ui/src/stores/exclusionPreview.ts index 5c3ef239..e0c2a76d 100644 --- a/ui/src/stores/exclusionPreview.ts +++ b/ui/src/stores/exclusionPreview.ts @@ -171,6 +171,31 @@ const scheduleFrame: (cb: () => void) => void = setTimeout(cb, 16); }; +/** Park at most this many pre-generation-id events of each kind (see the + * `preIdBatches` declaration for why the park needs a cap at all). + * + * The backend caps one generation at `NODE_STREAM_CAP` (50,000) nodes in + * `BATCH_MAX_NODES` (400) sized batches, so 125 FULL batches - and only the + * ones arriving before `previewExclusionsStart` resolves are ever parked. That + * window is a single IPC round trip: since #177 the command hands back its + * generation id immediately and does the slow matcher build in the spawned + * task, so at the `BATCH_MAX_INTERVAL` (100ms) partial-flush cadence only a + * batch or two can land inside it. 256 is 2x the full-batch ceiling and orders + * above the realistic park, while still pinning an unresolvable controller's + * retention to a constant. + * + * Over the cap the NEWEST event is dropped rather than the oldest: the backend + * streams breadth-first, so the oldest batches carry the ancestors every later + * row hangs off, and truncating the tail is exactly the already-handled + * `truncated` case. */ +export const PRE_ID_PARK_CAP = 256; + +/** Park `event` in `park` unless it is already at [`PRE_ID_PARK_CAP`]. */ +function parkPreId(park: T[], event: T): void { + if (park.length >= PRE_ID_PARK_CAP) return; + park.push(event); +} + export type ExclusionPreviewController = ReturnType; /** @@ -233,8 +258,16 @@ export function createExclusionPreview() { let startSeq = 0; /** The walk begins before `preview_exclusions_start` resolves, so its first * batches can legitimately arrive BEFORE we know the generation id. They are - * parked here and replayed once the id lands (bounded by the backend's own - * node cap, so this cannot grow without limit). */ + * parked here and replayed once the id lands. + * + * The park is capped at [`PRE_ID_PARK_CAP`]. The backend's own node cap + * bounds ONE generation's stream, which is all a controller that goes on to + * resolve an id can ever park - but a controller whose id never resolves + * (a rejected `previewExclusionsStart`, or a controller orphaned by an + * unmount that raced `subscribe`) leaves `currentId` null forever while its + * globally-registered listeners keep receiving EVERY later preview's + * batches. Nothing drains the park in that state, so without a cap it grows + * for the life of the process. The cap makes that retention constant. */ let preIdBatches: ExclusionPreviewBatch[] = []; let preIdDone: ExclusionPreviewDone[] = []; let preIdErrors: ExclusionPreviewError[] = []; @@ -398,7 +431,7 @@ export function createExclusionPreview() { /** Take a batch from the event stream (or from the pre-id park). */ function ingestBatch(batch: ExclusionPreviewBatch): void { if (currentId === null) { - preIdBatches.push(batch); + parkPreId(preIdBatches, batch); return; } // A superseded walk's in-flight events must never touch the live tree. @@ -409,7 +442,7 @@ export function createExclusionPreview() { function ingestDone(done: ExclusionPreviewDone): void { if (currentId === null) { - preIdDone.push(done); + parkPreId(preIdDone, done); return; } if (done.previewId !== currentId) return; @@ -427,7 +460,7 @@ export function createExclusionPreview() { * user fixes the rule the next generation swaps a real tree back in. */ function ingestError(error: ExclusionPreviewError): void { if (currentId === null) { - preIdErrors.push(error); + parkPreId(preIdErrors, error); return; } if (error.previewId !== currentId) return; @@ -536,5 +569,9 @@ export function createExclusionPreview() { /** Look up a node of the tree ON SCREEN by path - which during a recompute * is still the previous generation's. Test/diagnostic seam. */ nodeAt: (path: string) => displayedIndex.get(path), + /** Events parked awaiting a generation id, capped at `PRE_ID_PARK_CAP`. + * Test/diagnostic seam: this is the retention of a controller whose id + * never resolves, so it is a memory bound, not a queue length. */ + preIdParkedCount: (): number => preIdBatches.length + preIdDone.length + preIdErrors.length, }; }