build/wasm: restore runner reliability and executable acceptance - #2660
Merged
cpunion merged 18 commits intoSep 27, 2026
Merged
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review summary
Solid, well-tested change adding runner-recovery for the WebAssembly/emscripten paths: a host-side timeout for llgo run/llgo test, a structured runnerFailure error that distinguishes missing/unavailable runner, start error, timeout, and non-zero exit, generalized implicit-execution temp-dir cleanup, and CI acceptance scripts with their own self-tests. The time.Duration overflow guard in testRunnerTimeout, the DeadlineExceeded classification via runContext.Err(), and the append-only CI report design (errexit save/restore + EXIT-trap publish) are all nicely handled.
A few findings below. Nothing blocking.
Confirmed non-issue: the runNative ModeRun change (dropping mockable.Exit(s.ExitCode()) in favor of returning through build.Do) does not regress llgo run exit codes — in the previous code that call only ran on the success path (exit 0), while non-zero child exits already returned an error and were mapped to exit 1 by the cmd/run caller. Behavior is unchanged, and success now correctly runs build.Do's deferred cleanup. Security review found no issues (argv-form exec, %s-formatted awk report, mktemp-based temp handling).
Additional note (no reliable inline anchor): runInEmulator's ModeRun and ModeTest switch arms in internal/build/run.go are now identical; they could be collapsed to case ModeRun, ModeTest: for clarity.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
(cherry picked from commit e3184c9)
(cherry picked from commit 946f93e)
(cherry picked from commit 3309729)
(cherry picked from commit 5435e47)
(cherry picked from commit 0ab8713)
(cherry picked from commit 0061e3a)
(cherry picked from commit a72d7f7)
(cherry picked from commit 2e9289b)
(cherry picked from commit 32e7610)
(cherry picked from commit c8c31fb)
cpunion
force-pushed
the
codex/wasm-r3-runner-recovery-20260924
branch
from
September 26, 2026 14:39
431da14 to
1f8733d
Compare
visualfc
reviewed
Sep 27, 2026
visualfc
left a comment
visualfc
left a comment
Collaborator
There was a problem hiding this comment.
Review
Can merge. This restores still-useful R3 runner behavior: classified host failures for llgo run/llgo test, process-group termination on a positive deadline, cleanup of implicit Wasm artifacts, and CI reporting of the profiles that actually ran. It does not change compile semantics.
The earlier fennoai notes are addressed: -timeout now flows through native, WasmRuntime(), and emulator execution via the same runRunnerCommand; Unix uses Setpgid plus group kill, Windows uses taskkill /T /F, and WaitDelay bounds pipe draining. The extra Cancel() after a failed exit is the right shape for Go preferring a nonzero exit over ErrWaitDelay.
Suggestions
1. llgo test drops the classified runner error
runRunnerCommand puts runner failed: phase=test status=timeout ... on the returned error and does not write it to the test stdout/stderr. runTestPrograms only uses err != nil to set failed and then prints FAIL\tpkg. build.go calls mockable.Exit(1) on test failure, so the classified error never reaches cmd/internal/test.
llgo run -timeout=1s shows status=timeout (that is what expect_llgo_runner_timeout asserts). A hung or timed-out host runner under llgo test mostly looks like FAIL plus truncated guest output.
Please print err.Error() for *runnerFailure from reportTestProgramResult. That is the command this diagnostics work is meant to serve.
2. Timed Unix runs steal the foreground TTY
Default llgo test -timeout=10m yields a 10m30s host deadline, so every wasm/emulator test gets Setpgid. When stdin is the developer's terminal, configureRunnerCancellation also sets Foreground=true on each child.
With default BuildParallelism (GOMAXPROCS), several node/wasmtime processes take turns owning the same controlling terminal. CI stdin is usually not a TTY, so the ioctl fails and this stays hidden. llgo run needs the terminal; llgo test usually does not.
Limit foreground takeover to phase=run, or to sequential runs whose child actually reads the terminal. signal.Ignore(SIGTTOU) is process-wide and is more brittle when combined with parallel takeover.
3. Windows cleanup after a failed exit is a post-mortem kill
The timeout path is fine: CommandContext calls Cancel() while the parent is still alive, so taskkill /T /F /PID can reach the tree.
The failed-exit path calls Cancel() after cmd.Run() returns. The parent PID is often already gone, children are reparented, taskkill /T fails, and Process.Kill() cannot reach them. Unix kill(-pgid) still works after the leader exits; Windows has no equivalent. fail-with-child lives only in run_timeout_unix_test.go.
Timeout tree-kill is the main goal, and the Windows timeout tests already passed. If exit 1 must also not leak node workers, the process needs a Job Object (or equivalent) at start, not a kill after the parent is reaped.
Nits
- Native
llgo test(program.runner == "") still uses a bareexec.Commandand ignoresRunnerTimeout. That is reasonable: the guest has the testing watchdog, and the host deadline is for a runner that stops forwarding exit. A one-line comment would help. dev/test_wasm_stdlib.shruns three packages on three targets withLLGO_BUILD_CACHE=offand a 60s guest timeout. Current CI fits the 20-minute step; a slower json run will kill the guest first.- The wasm-runtime timeout helper builds
WasmRuntime()withstrings.Split(..., " "). The emulator path quotes with%q. A Windows path with spaces will split (existingWasmRuntime()convention; the new timeout test follows it). - The PR text still mentions a local Binaryen
llgo-v132.2run; CI is pinned tollgo-v132.3.
visualfc
approved these changes
Sep 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
llgo runandllgo testcan leave implicit Wasm entry modules and sidecars after runner execution, report an unclassified host error, or wait indefinitely when a runner hangs. This PR restores the useful runner and executable-acceptance changes from the retired R3 stack on the current J32/J64/W32 baseline.llgo testprints the classified failure beforeFAIL. Nativellgo run,WasmRuntime()execution, and emulator execution share the same deadline handling; raw Go-compatible Wasm tests also have host deadlines.taskkill /T /Fon Windows, with direct-child termination as a fallback. Bound inherited-output draining withWaitDelay. Unixllgo runpreserves interactive terminal input and restores foreground ownership;llgo testdoes not take the foreground terminal while its test processes run. A zero timeout keeps the existing unlimited execution behavior.bytes,strings, andencoding/jsontest packages on Emscripten Memory32, Emscripten Memory64, and WASI.This consolidates the still-useful changes from fork PRs #223, #226, #227, #229, and #231. The prerequisite reflectlite Kind and Windows
wasm-optfixes are already onmain.Review-fix validation:
CI follow-up: the report failure probe uses the
test-commandsuite so it reaches a recorded case before any runtime-only preflight, and prints the captured report on a mismatch. It passed on Linux AMD64 and ARM64 containers against the current upstreammainmerge. The terminal test waits for its helper to exit normally and passes its coverage directory through, so the exercised Unix process-group path is included in the host Go coverage profile; local focused coverage for that function rose from 57.9% to 100%.go test ./internal/build ./cmd/internal/run ./cmd/internal/test -count=1passed. After the review fixes, focused runner and CLI suites passed again; the pseudo-terminal test passed ten repetitions, and a real helper-process test verifies thatllgo testprints the classified timeout cause.Regression tests launch descendants that write heartbeats and verify termination for native,
WasmRuntime(), and emulator paths. Unix tests cover both successful and failed parent exits while a descendant retains output pipes, plus interactive terminal input and foreground restoration. The failed-exit regression fails without the final cleanup fix and passes with it. Zero-timeout native execution is also covered.Actual native
llgo run -timeout=150msand Emscripten/Nodellgo run -timeout=1sruns returned classified timeout failures. The Emscripten run used LLGo Binaryenllgo-v132.3.dev/wasm_ci_report_test.shpassed; the platform cancellation helpers compiled for Linux and Windows on the earlier head. Windows runtime behavior still needs the updated PR CI. Post-exit descendant cleanup on Windows is not guaranteed if the parent has already exited:taskkill /Tonly covers descendants while the parent still exists. Guaranteeing that case requires a Job Object; this PR does not claim it.Earlier executable validation passed the runtime/test-command suites (24/24 and 10/10 checks), all nine stdlib package runs with cache disabled,
shellcheck,actionlint, anddev/test_wasm_stdlib_test.sh. Those runs used LLGo Binaryenllgo-v132.2, Emscripten 6.0.8, Node 26.8.1, and Wasmtime 48.0.1.