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(