From 78bf243b83588b0ef6f1db22cf09e2d5fbd95b9d Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Thu, 8 Oct 2026 10:14:05 +0700 Subject: [PATCH] fix(runtime): wait on close for widget dev work already started, with a bound `WidgetDevSessions.close()` now returns a promise that waits for every session's queued work, including a superseded snapshot still being removed with retries, for at most two seconds. The node's shutdown starts it with the other stops and awaits it before the database closes. The spec awaits `close()` and removes its folder with the async cleanup helper. The held-snapshot test no longer races a 300 ms timer against the retry budget: one attempt meets the hold, the holder exits, and then the runtime's own removal runs, whose retry options are asserted. Refs #626, #631 Fixes #639 --- .../src/application/widget-dev-sessions.ts | 38 ++++++- .../src/bootstrap/runtime-bootstrap.ts | 8 +- apps/runtime/src/main.ts | 3 + apps/runtime/test/slash-commands.spec.ts | 4 +- apps/runtime/test/widget-dev-sessions.spec.ts | 104 ++++++++++++++++-- 5 files changed, 139 insertions(+), 18 deletions(-) diff --git a/apps/runtime/src/application/widget-dev-sessions.ts b/apps/runtime/src/application/widget-dev-sessions.ts index b48402b5..720b613d 100644 --- a/apps/runtime/src/application/widget-dev-sessions.ts +++ b/apps/runtime/src/application/widget-dev-sessions.ts @@ -128,12 +128,29 @@ export interface WidgetDevSessions { forget(root: string): WidgetDevResult<{ root: string; forgotten: boolean; stillCoveredBy?: string }>; /** Start watching again every session that was live when the node stopped. */ resume(): Promise; - close(): void; + /** + * Stop watching every folder, then wait for the work each session already started (a build being followed, a + * superseded snapshot being removed) to finish, for at most `closeWaitMs`, so the caller can let go of the database and + * the data folder without pulling them from under a removal. Past the bound it resolves anyway: a shutdown never hangs. + */ + close(): Promise; } /** How often a session waiting on the person's answer looks for it, so an approval in the inbox is followed promptly. */ const ANSWER_POLL_MS = 2_000; +/** + * How a superseded snapshot is removed: the promise form of `rm`, retrying a file still held open (on Windows) after 100, + * 200, 300, 400 and 500 ms, about 1.5 s in all. + */ +export const SNAPSHOT_REMOVAL = { recursive: true, force: true, maxRetries: 5, retryDelay: 100 } as const; + +/** + * The longest `close` waits for work already started: one snapshot's removal with all its retries, with room to spare, + * and well inside the node's shutdown grace. + */ +export const WIDGET_DEV_CLOSE_WAIT_MS = 2_000; + interface LiveSession { sessionId: string; engine: DevEngine; @@ -202,6 +219,8 @@ export function createWidgetDevSessions( answerPollMs?: number; /** How long a folder may keep failing to be looked at before its session stops (`DEV_ENGINE_ROOT_UNREADABLE_MS`). */ rootUnreadableMs?: number; + /** The longest `close` waits for work already started (`WIDGET_DEV_CLOSE_WAIT_MS`). */ + closeWaitMs?: number; } = {}, ): WidgetDevSessions { const live = new Map(); @@ -774,7 +793,7 @@ export function createWidgetDevSessions( try { // The promise form on purpose: on Windows `rmSync` reports a held file as `EBUSY` or `EPERM` at once and never // runs its retries. These wait up to about 1.5 s on this session's chain, never on the event loop. - await rm(path, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + await rm(path, SNAPSHOT_REMOVAL); removed.push(digest); } catch (cause) { process.stderr.write(`widget dev: could not remove the superseded snapshot ${digest} yet: ${messageOf(cause)}\n`); @@ -1111,9 +1130,22 @@ export function createWidgetDevSessions( } }, - close() { + async close() { for (const session of live.values()) end(session); live.clear(); + // Each chain already settles rather than rejects; work queued behind a closed session finds it gone and returns. + const started = Promise.all([...chains.values()]); + let bound: ReturnType | undefined; + const timedOut = new Promise((done) => { + bound = setTimeout(() => done(true), options.closeWaitMs ?? WIDGET_DEV_CLOSE_WAIT_MS); + }); + try { + if (await Promise.race([started.then(() => false), timedOut])) { + process.stderr.write("widget dev: work a session started was still running at close; closing anyway\n"); + } + } finally { + clearTimeout(bound); + } }, }; } diff --git a/apps/runtime/src/bootstrap/runtime-bootstrap.ts b/apps/runtime/src/bootstrap/runtime-bootstrap.ts index 4de415cd..ed1c952b 100644 --- a/apps/runtime/src/bootstrap/runtime-bootstrap.ts +++ b/apps/runtime/src/bootstrap/runtime-bootstrap.ts @@ -116,6 +116,11 @@ export interface RuntimeBootstrapDeps { export interface RuntimeHandles { /** Stops the periodic update-check timer (ยง update-checks.ts). A no-op on a fixture node, which never starts one. */ stopUpdateChecks: () => void; + /** + * Stops watching widget dev folders and waits, bounded, for work their sessions already started, such as removing a + * superseded snapshot (`WidgetDevSessions.close`). Awaited before the database closes. + */ + closeWidgetDev: () => Promise; } /** @@ -632,9 +637,8 @@ export function wireRuntime(deps: RuntimeBootstrapDeps): RuntimeHandles { return { stopUpdateChecks: () => { updateChecks?.stop(); - // Beside the update check: both are timers and watchers this function started, ended when the node closes. - widgetDev.close(); }, + closeWidgetDev: () => widgetDev.close(), }; } diff --git a/apps/runtime/src/main.ts b/apps/runtime/src/main.ts index d3d1628c..46872e62 100644 --- a/apps/runtime/src/main.ts +++ b/apps/runtime/src/main.ts @@ -550,6 +550,8 @@ async function main(): Promise { hardStop.unref(); leaseSweeper.stop(); runtimeHandles.stopUpdateChecks(); + // Started now and awaited before the database closes: a snapshot a session is removing finishes, within its bound. + const widgetDevClosed = runtimeHandles.closeWidgetDev(); services.expirySweep?.stop(); services.artifactSweep?.stop(); effectNotices.stop(); @@ -591,6 +593,7 @@ async function main(): Promise { if (channelsStopped !== undefined && !(await channelsStopped)) { process.stderr.write("channel turns were still running at shutdown; closing anyway\n"); } + await widgetDevClosed; await modelTurn?.dispose(); // Every browser token still held is withdrawn where its provider allows, rather than left to lapse. await services.browserTokens?.close(); diff --git a/apps/runtime/test/slash-commands.spec.ts b/apps/runtime/test/slash-commands.spec.ts index c7342078..d408ee65 100644 --- a/apps/runtime/test/slash-commands.spec.ts +++ b/apps/runtime/test/slash-commands.spec.ts @@ -195,7 +195,7 @@ describe("slash commands in a conversation", () => { // The card only offers: nothing starts until the person presses it. expect(services.widgetDev.list()).toEqual([]); } finally { - services.widgetDev.close(); + await services.widgetDev.close(); } }); @@ -210,7 +210,7 @@ describe("slash commands in a conversation", () => { // The word is the command's, not a folder: nothing is offered to develop. expect(card?.rows.some((row) => row.rowId === "proposed")).toBe(false); } finally { - services.widgetDev.close(); + await services.widgetDev.close(); } }); }); diff --git a/apps/runtime/test/widget-dev-sessions.spec.ts b/apps/runtime/test/widget-dev-sessions.spec.ts index 1932b237..02ae8b03 100644 --- a/apps/runtime/test/widget-dev-sessions.spec.ts +++ b/apps/runtime/test/widget-dev-sessions.spec.ts @@ -31,7 +31,7 @@ import { writeRegisteredPreference, } from "@clarkcant/core"; -import { createWidgetDevSessions } from "../src/application/widget-dev-sessions.ts"; +import { SNAPSHOT_REMOVAL, createWidgetDevSessions } from "../src/application/widget-dev-sessions.ts"; import { WIDGET_DEV_STORE_MAX, readDevSessions, writeDevSessions } from "../src/application/widget-dev-store.ts"; import { createDevelopWidgetTool } from "../src/develop-widget-tool.ts"; import { hostText } from "../src/host-text.ts"; @@ -43,8 +43,11 @@ import { holdDirectory } from "./hold-directory.ts"; /** A folder whose `stat` fails with `EPERM`, as an antivirus or indexer holding it on Windows makes it fail. */ const statFailure = vi.hoisted(() => ({ path: undefined as string | undefined })); -/** Seen as the runtime starts to remove a path, in either form, before the removal itself runs. */ -const removal = vi.hoisted(() => ({ starting: undefined as ((path: string) => void) | undefined })); +/** + * Seen as the runtime starts to remove a path, in either form, before the removal itself runs. The promise form waits + * for what it returns, so a test can hold a removal (`options` are the ones the runtime asked for). + */ +const removal = vi.hoisted(() => ({ starting: undefined as ((path: string, options?: fs.RmOptions) => void | Promise) | undefined })); vi.mock("node:fs", async (importOriginal) => { const actual = await importOriginal(); @@ -55,7 +58,7 @@ vi.mock("node:fs", async (importOriginal) => { return actual.statSync(path, options); }) as typeof actual.statSync; const rmSync = ((path: fs.PathLike, options?: fs.RmOptions) => { - removal.starting?.(resolve(String(path))); + void removal.starting?.(resolve(String(path)), options); actual.rmSync(path, options); }) as typeof actual.rmSync; return { ...actual, statSync, rmSync, default: { ...actual, statSync, rmSync } }; @@ -64,12 +67,15 @@ vi.mock("node:fs", async (importOriginal) => { vi.mock("node:fs/promises", async (importOriginal) => { const actual = await importOriginal(); const rm = (async (path: fs.PathLike, options?: fs.RmOptions) => { - removal.starting?.(resolve(String(path))); + await removal.starting?.(resolve(String(path)), options); await actual.rm(path, options); }) as typeof actual.rm; return { ...actual, rm, default: { ...actual, rm } }; }); +/** The removal itself, past the mock, for a test that tries a path once on its own. */ +const { rm: actualRm } = await vi.importActual("node:fs/promises"); + /** The home folder the node sees, when a test needs it to be one of the test's own folders. */ const homeOverride = vi.hoisted(() => ({ path: undefined as string | undefined })); @@ -235,12 +241,14 @@ beforeEach(() => { deps = { services, now: () => new Date().toISOString() as never }; }); -afterEach(() => { +afterEach(async () => { statFailure.path = undefined; homeOverride.path = undefined; - services.widgetDev?.close(); + removal.starting = undefined; + // A snapshot a session is still removing is finished (or given up on) before the database and the folder go. + await services.widgetDev?.close(); services.runtime.close(); - rmSync(dir, { recursive: true, force: true }); + await removeTestDirectory(dir); }); describe("a widget dev session", () => { @@ -968,13 +976,22 @@ describe("a widget dev session", () => { if (first === undefined) throw new Error("the first build left no snapshot"); const firstPath = resolve(local, first); // On Windows a directory that is someone's working directory cannot be removed until they let go; elsewhere the - // hold changes nothing. It ends 300 ms after the removal starts, so only a removal that really retries finds it free. + // hold changes nothing. Not timed against the retries: one attempt without them meets the hold, the holder is let go + // and has exited, and only then does the removal the runtime asked for run, so its outcome never depends on load. const hold = holdDirectory(firstPath); await hold.ready; let released: Promise | undefined; - removal.starting = (path) => { + let asked: fs.RmOptions | undefined; + let held: string | undefined; + removal.starting = async (path, options) => { if (path !== firstPath || released !== undefined) return; - released = new Promise((done) => setTimeout(done, 300)).then(() => hold.release()); + asked = options; + held = await actualRm(path, { ...options, maxRetries: 0 }).then( + () => undefined, + (cause: unknown) => (cause as NodeJS.ErrnoException).code, + ); + released = hold.release(); + await released; }; try { // The second build supersedes the first, which stays as the generation a rollback returns to; the third prunes it. @@ -983,6 +1000,10 @@ describe("a widget dev session", () => { expect(session(await call("POST", `/widget-dev/sessions/${started.sessionId}/rebuild`)).running?.generation).toBeGreaterThan(1); } expect(released).toBeDefined(); + // The removal retries a held file (the promise form; `rmSync` would not), for the budget the runtime names. + expect(asked).toMatchObject(SNAPSHOT_REMOVAL); + expect(SNAPSHOT_REMOVAL.maxRetries).toBeGreaterThan(0); + if (process.platform === "win32") expect(held).toMatch(/^(EBUSY|EPERM)$/); expect(existsSync(firstPath)).toBe(false); // Removed, so no longer on the session's list of snapshots to try again. const listed = readDevSessions(join(dir, "node"))[0]?.snapshots ?? []; @@ -994,6 +1015,67 @@ describe("a widget dev session", () => { } }); + /** Start pruning the first build's snapshot and hold its removal until `finish` is called. */ + async function holdPrune(): Promise<{ firstPath: string; rebuilding: Promise; finish: () => void; removed: () => boolean }> { + const started = session(await call("POST", "/widget-dev/sessions", { root })); + const local = join(dir, "node", "package-cache", "local"); + const [first] = readdirSync(local); + if (first === undefined) throw new Error("the first build left no snapshot"); + const firstPath = resolve(local, first); + let removing = (): void => undefined; + const removingStarted = new Promise((done) => { + removing = done; + }); + let finish = (): void => undefined; + const gate = new Promise((done) => { + finish = done; + }); + let removed = false; + removal.starting = async (path) => { + if (path !== firstPath) return; + removing(); + await gate; + // Marked once the removal itself may run; `existsSync` says when it has. + removed = true; + }; + writePackage("

second

\n"); + await call("POST", `/widget-dev/sessions/${started.sessionId}/rebuild`); + // The third build supersedes the second, so the first is pruned, and its removal waits on the gate. + writePackage("

third

\n"); + const rebuilding = call("POST", `/widget-dev/sessions/${started.sessionId}/rebuild`); + await removingStarted; + return { firstPath, rebuilding, finish, removed: () => removed }; + } + + it("waits on close for a superseded snapshot still being removed", async () => { + const { firstPath, rebuilding, finish } = await holdPrune(); + let closed = false; + const closing = Promise.resolve(services.widgetDev?.close()).then(() => { + closed = true; + }); + await new Promise((done) => setTimeout(done, 100)); + expect(closed).toBe(false); + finish(); + await closing; + expect(existsSync(firstPath)).toBe(false); + expect((await rebuilding).status).toBe(200); + }); + + it("stops waiting on close after its bound, so a removal that does not end cannot hold a shutdown", async () => { + await services.widgetDev?.close(); + services.widgetDev = createWidgetDevSessions(() => services, { watch: false, closeWaitMs: 200 }); + const { firstPath, rebuilding, finish, removed } = await holdPrune(); + const before = Date.now(); + await services.widgetDev.close(); + expect(Date.now() - before).toBeGreaterThanOrEqual(150); + expect(removed()).toBe(false); + expect(existsSync(firstPath)).toBe(true); + // Let it end, so the folder is free before the test's own cleanup. + finish(); + expect((await rebuilding).status).toBe(200); + expect(existsSync(firstPath)).toBe(false); + }); + it("forgets the oldest stopped sessions rather than failing when the store is full", async () => { const at = new Date(Date.UTC(2026, 0, 1)).toISOString(); writeDevSessions(