diff --git a/packages/webui/src/runtime/terminal.ts b/packages/webui/src/runtime/terminal.ts index 7470c271..ecd0fae2 100644 --- a/packages/webui/src/runtime/terminal.ts +++ b/packages/webui/src/runtime/terminal.ts @@ -70,6 +70,30 @@ export class WebuiTerminalManager implements WebuiTerminalProcessPort { signal?.removeEventListener("abort", onAbort); } } - disposeBySession() { for (const terminal of this.terminals.values()) terminal.pty.kill(); this.terminals.clear(); } + /** + * Kill every terminal in this session. + * + * Node-pty's Windows kill path forks a helper that lists the processes + * attached to the shell's console, and `child_process.fork` hands that + * child this process's `execArgv`. Whatever this process was started with + * is therefore loaded inside the helper, and the helper has no use for our + * arguments. Under the acceptance host that meant tsx's loader ran inside + * node-pty's helper, which is the shape microsoft/node-pty#974 describes; + * the helper then failed on `AttachConsole` and the kill fell back to + * node-pty's five-second timer. + * + * The kill is synchronous and the helper needs none of our flags, so the + * flags are put aside for the length of the kill and put back afterwards. + */ + disposeBySession() { + const execArgv = process.execArgv; + process.execArgv = []; + try { + for (const terminal of this.terminals.values()) terminal.pty.kill(); + } finally { + process.execArgv = execArgv; + this.terminals.clear(); + } + } private get(id: string) { const terminal = this.terminals.get(id); if (!terminal) throw new Error("终端不存在"); return terminal; } } diff --git a/packages/webui/src/server/service.ts b/packages/webui/src/server/service.ts index 72f7c5af..0b2a9d01 100644 --- a/packages/webui/src/server/service.ts +++ b/packages/webui/src/server/service.ts @@ -342,18 +342,23 @@ export class WebuiService { * * Attempt every owner in order and report all failures after release. */ - close(): Promise { - this.closing ??= this.#closeOwners(); + close(onOwner?: (owner: string) => void): Promise { + this.closing ??= this.#closeOwners(onOwner); return this.closing; } - async #closeOwners(): Promise { + async #closeOwners(onOwner?: (owner: string) => void): Promise { /** One owner that failed, kept with its name for the aggregate. */ const failures: { readonly owner: string; readonly error: unknown }[] = []; const attempt = async ( owner: string, step: () => void | Promise, ): Promise => { + // The owner is announced before it is awaited, so a caller that is + // waiting on the close knows which owner it is waiting for. Every + // await here is unbounded, and a shutdown that never finishes is + // otherwise a silence. + onOwner?.(owner); try { await step(); } catch (error) { diff --git a/packages/webui/test/browser/native-runtime.spec.mjs b/packages/webui/test/browser/native-runtime.spec.mjs index 06e8d2cd..a7b1b401 100644 --- a/packages/webui/test/browser/native-runtime.spec.mjs +++ b/packages/webui/test/browser/native-runtime.spec.mjs @@ -47,13 +47,28 @@ test.afterAll(async () => { // The host's own trace of where it is in the shutdown is the only thing // that says which step stopped answering, so a shutdown that does not // finish has to carry it. + // The budget is below Playwright's own hook timeout on purpose: this + // hook's message carries the host's trace, and a tie would leave + // Playwright's bare "afterAll hook timeout" as the only thing said. const [message] = await within( stopped, - 30_000, + 25_000, () => new Error(`the native host did not finish closing\n${logs}`), ); - expect(message).toMatchObject({ closed: true, terminals: 0 }); - expect(message.exited).toBeGreaterThanOrEqual(6); + // The host reports the step it stopped at when its own shutdown does not + // finish, so a stall says which step it was instead of leaving the hook + // with a bare timeout and no way to tell what is stuck. + expect( + message, + `the native host did not close${message?.stalledAt ? ` (stalled at ${message.stalledAt})` : ""}\n${logs}`, + ).toMatchObject({ closed: true, terminals: 0 }); + // The case creates six terminals before the shutdown, so a smaller count + // means the body failed before it got that far. Saying so keeps the + // teardown's second complaint from reading like a second defect. + expect( + message.exited, + `the host closed ${message.exited} terminals; the case creates six before the shutdown, so fewer means the body failed earlier\n${logs}`, + ).toBeGreaterThanOrEqual(6); // Once the host has said it closed, it has nothing left to do and it must // be gone. Node-pty's Windows console-list helper cannot attach to a // ConPTY shell, so every terminal it kills leaves a five-second fallback diff --git a/packages/webui/test/browser/support/native-browser-host.ts b/packages/webui/test/browser/support/native-browser-host.ts index bb62972e..ec539400 100644 --- a/packages/webui/test/browser/support/native-browser-host.ts +++ b/packages/webui/test/browser/support/native-browser-host.ts @@ -62,6 +62,17 @@ const service = new WebuiService({ port, operations, terminal, const info = await service.start(); process.send?.({ url: `http://127.0.0.1:${info.tcpPort}/?token=${info.credential.token}#session=${sessionId}`, workspaceDir, sessionId }); let closing: Promise | undefined; +/** + * How long the service close is given before the host says it stopped there. + * + * The suite's own hook budget is thirty seconds, and the host used to spend + * all of it saying nothing: the service close never answered, so the hook + * timed out on a step it could not name. The host now answers first, and the + * answer names the step. + */ +const SERVICE_CLOSE_DEADLINE_MS = 20_000; +/** What a service close that did not answer leaves behind. */ +const STALLED = Symbol("service.close did not finish"); async function close() { if (closing) return closing; closing = (async () => { @@ -69,8 +80,39 @@ async function close() { // finish is otherwise a silence, and the suite cannot tell which step // stopped answering. const step = (name: string) => { process.stderr.write(`[native-host] ${name}\n`); }; + // The report leaves with the process, so the exit is taken in the send + // callback: the message has to be flushed first. + const report = (message: Record) => { + process.send?.(message, () => { + process.disconnect?.(); + process.exit(0); + }); + }; step("service.close"); - await service.close(); + // The service announces each owner before it awaits it, so the host can + // say which owner the shutdown is waiting on. A stall that names its + // owner is the difference between "the service close did not finish" and + // "the harness port did not finish". + let owner = "service.close"; + const outcome = await new Promise((resolve) => { + const timer = setTimeout(() => resolve(STALLED), SERVICE_CLOSE_DEADLINE_MS); + service.close((closing) => { + owner = closing; + step(`owner ${closing}`); + }).then( + () => { clearTimeout(timer); resolve("closed"); }, + (error: unknown) => { clearTimeout(timer); resolve(error); }, + ); + }); + if (outcome === STALLED) { + // The service close did not answer. The terminals were already killed — + // that is the first thing the close does — so the host reports where it + // stopped and goes, instead of leaving the suite to time out blind. + step(`service.close did not finish (at ${owner})`); + report({ closed: false, terminals: 0, exited: exits.length, stalledAt: owner }); + return; + } + if (outcome !== "closed") throw outcome; step("terminals exited"); await Promise.all(exits); step("apiHost.close"); @@ -89,10 +131,7 @@ async function close() { // under load that was enough for the suite's shutdown hook to miss its // deadline. The callback runs once the message has been flushed, so the // report is not lost by exiting here. - process.send?.({ closed: true, terminals: 0, exited: exits.length }, () => { - process.disconnect?.(); - process.exit(0); - }); + report({ closed: true, terminals: 0, exited: exits.length }); })(); return closing; }