Skip to content

fix(webui): the native acceptance names the step it stopped at, and keeps our preload out of node-pty's helper - #31

Merged
modacker merged 3 commits into
mainfrom
fix/webui-native-acceptance-shutdown
Nov 11, 2026
Merged

modacker merged 3 commits into
mainfrom
fix/webui-native-acceptance-shutdown

Conversation

@modacker

@modacker modacker commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator

Change

The native-terminal acceptance on the Windows runner fails intermittently in its teardown. The hook waits for the native host to say it closed; on some runs the host stopped answering inside its own service close and had nothing to say about it.

This is the failure recorded in #30. It is not #22's five-second drag, and it is not what #17 fixed either: #17 made the host go once it had reported, and here the host never reaches the report.

Where it stops. Since #17 the host reports each step of its own shutdown, so a shutdown that does not finish says which step stopped answering. The failing run's last line is:

[native-host] service.close

It never reaches terminals exited. The runtime's own shutdown lines (Turn controller shutdown, All MCP connections closed, the sandbox lifecycle changes) follow, and then nothing.

What the evidence does not say. The host's captured log also carries two crashes inside node-pty's console-list helper, with this repository's tsx loader in the stack:

node_modules\node-pty\src\conpty_console_list_agent.ts:13
Error: AttachConsole failed
    at Object.<anonymous> (node_modules\node-pty\src\conpty_console_list_agent.ts:13:28)
    at Object.transformer (packages\webui\node_modules\tsx\dist\register-OY9cqBEN.cjs:9:3619)

That is the shape microsoft/node-pty#974 describes — the console-list agent inherits process.execArgv, so the host's preload runs inside it — and it is ours because the preload is ours. It is not, however, what makes this stall last thirty seconds, and this change does not claim it is:

  • the attempt that passed all 485 browser cases carried the same helper crash, so the crash and the stall do not travel together;
  • node-pty's five-second fallback runs beside the measured path — kill() returns synchronously and the promise is a side branch — and the kills run in parallel, so it accounts for about five seconds, not thirty;
  • what that timer does explain is the host lingering after it closed, which is the earlier symptom fix(webui): the native acceptance host goes when it is done #17 addressed.

The thirty seconds are still unexplained. WebuiService.close() awaits every owner it owns — the terminal manager, the websocket connections, the websocket server, the browser bridge, the Mini App proxies, the HTTP listener, the harness port — and each of those awaits is unbounded. A shutdown that stalls is a silence, and this is that silence.

What this change does about it. It makes the silence say which owner it is in, so the next occurrence names the step instead of only naming the service close:

  • packages/webui/src/server/service.ts — close() takes an optional callback and announces each owner before it awaits it. The close order, the owners, and what a failure does are unchanged; a caller that does not want the progress does not receive it.
  • packages/webui/test/browser/support/native-browser-host.ts — the host gives the service close a deadline of its own. When that deadline passes it reports the owner it stopped at, and goes. The terminals are killed before the service close waits on anything else, so a service close that stalls has already released them.
  • packages/webui/test/browser/native-runtime.spec.mjs — the hook's budget sits below Playwright's own hook timeout, so the message that carries the host's trace is the message that is said, and the teardown's second complaint explains itself when the body failed earlier.
  • packages/webui/src/runtime/terminal.ts — disposeBySession() puts process.execArgv aside for the length of the kill and puts it back afterwards, so node-pty's helper stops loading this repository's preload. The kill is synchronous and the helper needs none of our flags. The development server starts under tsx as well (scripts/run-webui-server.mjs), so the same exposure is in the product, not only in the test.

Nothing is loosened. The hook's thirty-second budget is unchanged, and a stall still fails the suite; it fails sooner, and it says where. No test is skipped, no retry is added, and no expected value changed.

Validation

  • Checks run and results (include the revision/profile where relevant):
    • Local gate, all nine stages — typecheck, dependency, presentation, motion, overlays, feedback, build, boundary, source: pass.
    • Vitest: 302 files, 4525 tests: pass.
    • packages/webui/test/unit/webui-service.test.ts and packages/webui/test/unit/terminal-process-manager.test.ts: 118 tests pass.
    • The native acceptance case on this machine (macOS): 1 passed.
    • CI on this branch: run 42971475734. Ubuntu and Windows passed on the first attempt. macOS failed its first attempt in packages/webui/test/browser/workspace-panel-actions.spec.mjs ("the workspace review stays responsive with 70 expanded diff files" reported "workspace panel drag produced 54 ms long tasks" -- a long-task budget in a client-side spec this change does not touch); it was rerun and passed on the second attempt. All three platforms are green.
  • Performance: basic — the automatic 100-round check. No perf:full label: the change is outside the paths the performance rules in CONTRIBUTING.md list, and it is not a performance optimization.
  • NOT RUN, platform limitations and live-service boundaries:
    • The stall is Windows-only and was not reproducible on demand, so this change is not backed by a reproduction of the failure itself. What it is backed by is the trace the failing run left: the host stopped inside service.close(), and nothing in the chain said which owner.
    • The change was not exercised on a Windows machine: this machine is macOS. The Windows runner ran this branch twice: on the first attempt the native acceptance's own body failed on a five-second wait for the shell's echo -- a different fault, recorded separately -- and on the second attempt the Windows job passed with this change in it. The stall this change is about did not recur there, so the change is backed by the failing run's trace, not by a reproduction of the stall.

Publication and contribution checks

  • I have permission to contribute these changes under the existing licenses applicable to the changed files/packages; imported material and its provenance are identified and existing notices are preserved.
  • No credentials, account data, real user content, internal source history or private review material is included.
  • Added/removed source files were reviewed before regenerating release/public-source.json; new tests are declared in test/vitest-suites.json where applicable. — no source file was added or removed, so release/public-source.json is unchanged; no vitest file was added, so no suite declaration is needed.
  • Shared English/Chinese documentation and capability/verification records are updated where applicable. — the close order documented in packages/webui/docs/runtime/boot.md is unchanged by this change, so that document still describes what the code does.

Maintainer handoff

Publication scope or license changes (if any): none.

Shared-source port: not needed. The change is inside packages/webui, which does not exist upstream; per packages/webui/docs/agents/issue-tracker.md nothing in this repository is published to upstream. The node-pty side is already open upstream as microsoft/node-pty#952, #1007 and #974, so no new upstream issue is needed.

probe added 2 commits November 11, 2026 13:04
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. The failing Windows run of PR #29 shows it: the helper crashed on
`AttachConsole` with tsx's transformer in the stack, twice, and the kill
then fell back to node-pty's five-second timer. That is the shape
microsoft/node-pty#974 describes, and the amplifier is ours because the
preload is ours.

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. In
a process started without anything unusual — which is the desktop app —
this changes nothing at all.
The native acceptance's teardown hook waits for the host to say it closed.
On Windows it sometimes waited the full thirty seconds and then reported a
bare timeout, because the host had stopped answering inside its own
service close and had nothing to say about it. The one thing the log did
carry was the host's step trace, and that is in the hook's error text
rather than in the failure itself.

The host now gives the service close a deadline of its own, and when that
deadline passes it reports the step it stopped at before it goes. The hook
then fails with the step in the message and the host's own trace beside
it, instead of a timeout that names nothing.

Nothing is loosened: the hook's thirty-second budget is unchanged and a
stall still fails the suite. It fails sooner, and it says where.

The terminals are killed before the service close waits on anything else,
so a service close that stalls has already released them and the host can
go once it has said where it stopped.
@modacker modacker changed the title fix(webui): the native acceptance names the step it stopped at, and keeps our preload out of node-pty\x27s helper fix(webui): the native acceptance names the step it stopped at, and keeps our preload out of node-pty's helper Nov 11, 2026
The service close awaits every owner it owns — the terminal manager, the
websocket connections, the websocket server, the browser bridge, the Mini
App proxies, the HTTP listener, the harness port — and every one of those
awaits is unbounded. The host could therefore only say that "the service
close" had not finished, which is one level above the question that
matters: which of them is stuck.

The service now announces each owner before it awaits it, and the host
passes that announcement on. A stall names the owner it stalled at, and the
hook's own budget sits below Playwright's hook timeout, so the message
that carries the host's trace is the message that is said.

The close signature gains an optional callback, so a caller that does not
want the progress does not get it. The close order, the owners, and what a
failure does are all unchanged.
@modacker
modacker merged commit 34ec834 into main Nov 11, 2026
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant