diff --git a/.github/workflows/push-test.yml b/.github/workflows/push-test.yml index e8876d3c9..ac645b8a0 100644 --- a/.github/workflows/push-test.yml +++ b/.github/workflows/push-test.yml @@ -51,6 +51,42 @@ jobs: exit 1 fi + # The Windows process helpers (cmd/mxcli/docker/procgroup_windows.go) cannot be + # exercised by the ubuntu job: Signal(0) succeeds on Linux and there is no + # process tree to reap. `mxcli run --local` hung forever on Windows at + # "Starting mxbuild --serve..." because alive() asked Signal(0) (unsupported on + # Windows, so a live mxbuild read as dead) and Stop() killed only the wrapper, + # leaving mxbuild's Deno web-ext worker holding the stdout pipe so cmd.Wait() + # blocked. This job runs the Windows-only regression tests on a real Windows + # runner so neither half can come back. + windows-process-regression: + runs-on: windows-latest + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-go@v7 + with: + go-version: '1.26.6' + - name: Test the Windows process helpers + shell: bash + # Scoped with -run: the rest of the docker package's tests assert POSIX + # file modes and shell out to `sh`, so the full package does not pass on + # Windows (tracked in #897). The test binary is still COMPILED for this + # platform, so a Windows build break in the package is caught too. + # + # -run can pass vacuously if the tests are renamed or deleted, so assert + # that the expected number actually ran. + run: | + out=$(go test -v -count=1 -run 'TestProcessAlive|TestKillProcessGroup|TestServeServer_AliveTracksProcess|TestLocalRuntime_AliveTracksProcess' ./cmd/mxcli/docker/) + echo "$out" + n=$(printf '%s\n' "$out" | grep -c '^--- PASS: Test' || true) + echo "windows process tests executed: $n" + if [ "$n" -lt 5 ]; then + echo "FAIL: expected at least 5 Windows process tests to run, -run matched $n." + echo " procgroup_windows_test.go must keep its processAlive / tree-kill /" + echo " alive() tests, or the Windows local-boot regression is unguarded." + exit 1 + fi + build-and-test: runs-on: ubuntu-latest steps: diff --git a/CHANGELOG.md b/CHANGELOG.md index a63e97ace..cb52184cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`mxcli run --local` hung forever on Windows at `Starting mxbuild --serve...`** — two POSIX assumptions in `cmd/mxcli/docker` combined. `ServeServer.alive()` (and `LocalRuntime.alive()`) asked `os.Process.Signal(syscall.Signal(0))`, which Go implements as `EWINDOWS` ("not supported by windows") for every signal but `Kill` — so a live mxbuild read as dead and `waitReady()` aborted the boot the instant it launched it. And `Stop()` waited on `cmd.Wait()` after a `killProcessGroup` that on Windows only called `p.Kill()`: mxbuild.exe is a wrapper that launches a Deno web-ext worker (`modeler/tools/deno/win-x64/deno.exe`) which inherits the stdout/stderr pipe `exec.Cmd` hands out, so the orphan kept the pipe open and `Wait()` never saw EOF — no error, no exit, just a hang. Windows now has a real liveness check (`OpenProcess(SYNCHRONIZE)` + `WaitForSingleObject`) and a tree kill (`taskkill /F /T`) behind `signalProcessGroup`/`killProcessGroup`; the POSIX implementations are unchanged. The Windows-only tests in `procgroup_windows_test.go` pin both halves — the tree-kill test fails on the pre-fix code, verified as a control — and a `windows-latest` CI job runs them (asserting they actually executed) so neither can come back. + - **`raise error;` on a microflow's main flow passed check and exec, then failed the build** (mendixlabs/mxcli#1030) — with `[error] [CE0710] "The main flow cannot join an error flow or end in an error event."`, one per microflow. Mendix's error event *re-raises the error being handled*, so it is legal only where an error is in scope: inside an `on error { … }` handler. Studio Pro will not draw the connection from the normal flow to an error event; mxcli could, and did. It is now **MDL084**, at error severity, so `exec`'s pre-flight refuses the script with nothing written (`--no-check` still applies it, for reproducing the build failure). The report's own diagnosis — a trailing End event appended because `RaiseErrorStmt` never set the builder's "ends with return" flag — is not the cause: `isTerminalStmt` has treated it as a terminator all along, and the graph mxcli builds for `raise error;` is exactly the one the report asks for (start event, error event, one sequence flow, no trailing End event, no outgoing flow), pinned by a test. No wiring makes a main-flow error event legal, which is why the fix is a refusal rather than a builder change. diff --git a/cmd/mxcli/docker/localboot.go b/cmd/mxcli/docker/localboot.go index ad2b015bf..06f10d482 100644 --- a/cmd/mxcli/docker/localboot.go +++ b/cmd/mxcli/docker/localboot.go @@ -711,16 +711,18 @@ func (rt *LocalRuntime) Log() string { return rt.log.String() } // alive reports whether the runtime process is still running. // -// Signal(0) is only a correct liveness test BECAUSE watchExit reaps: it succeeds -// on an unreaped zombie — measured, proc state `Z`, err nil — and returns -// "process already finished" once Wait has run. Before the reaper existed this -// function reported a runtime that had terminated itself hours earlier as alive -// (mxcli-formula1 FINDINGS §60). Removing watchExit silently breaks this line. +// Signal(0) is only a correct liveness test on POSIX, and even there only +// BECAUSE watchExit reaps: it succeeds on an unreaped zombie — measured, proc +// state `Z`, err nil — and returns "process already finished" once Wait has run. +// Before the reaper existed this function reported a runtime that had terminated +// itself hours earlier as alive (mxcli-formula1 FINDINGS §60). Removing watchExit +// silently breaks that on POSIX. On Windows Signal(0) is not supported at all, so +// processAlive uses WaitForSingleObject instead (see procgroup_windows.go). func (rt *LocalRuntime) alive() bool { if rt.cmd == nil || rt.cmd.Process == nil { return false } - return rt.cmd.Process.Signal(syscall.Signal(0)) == nil + return processAlive(rt.cmd.Process) } // Stop shuts the runtime down gracefully via the admin API, then terminates the diff --git a/cmd/mxcli/docker/mxserve.go b/cmd/mxcli/docker/mxserve.go index c2ff88946..6fe30a65e 100644 --- a/cmd/mxcli/docker/mxserve.go +++ b/cmd/mxcli/docker/mxserve.go @@ -385,12 +385,16 @@ func (s *ServeServer) Build(req BuildRequest) (*BuildResult, error) { return &res, nil } -// alive reports whether the serve process is still running (Linux: signal 0). +// alive reports whether the serve process is still running. +// +// Delegates to processAlive: Signal(0) is a correct liveness test on POSIX but +// returns EWINDOWS on Windows, where it made waitReady() treat a just-started +// mxbuild as dead. func (s *ServeServer) alive() bool { if s.cmd == nil || s.cmd.Process == nil { return false } - return s.cmd.Process.Signal(syscall.Signal(0)) == nil + return processAlive(s.cmd.Process) } // Log returns the captured mxbuild --serve output (for diagnostics). diff --git a/cmd/mxcli/docker/procgroup_unix.go b/cmd/mxcli/docker/procgroup_unix.go index f15bc72c1..039e4eb6a 100644 --- a/cmd/mxcli/docker/procgroup_unix.go +++ b/cmd/mxcli/docker/procgroup_unix.go @@ -46,3 +46,13 @@ func signalProcessGroup(p *os.Process, sig syscall.Signal) error { func killProcessGroup(p *os.Process) error { return signalProcessGroup(p, syscall.SIGKILL) } + +// processAlive reports whether p is still running. On POSIX, signal 0 succeeds +// for a live process; it also succeeds for an unreaped zombie, which is why the +// callers that care (LocalRuntime) reap via watchExit. +func processAlive(p *os.Process) bool { + if p == nil { + return false + } + return p.Signal(syscall.Signal(0)) == nil +} diff --git a/cmd/mxcli/docker/procgroup_windows.go b/cmd/mxcli/docker/procgroup_windows.go index 73014576f..6e1a3fd91 100644 --- a/cmd/mxcli/docker/procgroup_windows.go +++ b/cmd/mxcli/docker/procgroup_windows.go @@ -7,29 +7,88 @@ package docker import ( "os" "os/exec" + "strconv" "syscall" ) -// procgroup_windows.go provides no-op / single-process fallbacks for the -// process-group helpers. Windows has no POSIX process groups; the warm loop is a -// Linux-devcontainer feature, so this only needs to keep the package compiling -// and preserve the prior single-PID signalling behaviour. +// procgroup_windows.go provides the Windows implementations of the process +// helpers. Windows has no POSIX process groups, so the "group" operations are +// implemented by terminating the whole process tree with taskkill instead. -// setProcessGroup is a no-op on Windows. +// setProcessGroup is a no-op on Windows: there is no PGID to set. Tree teardown +// is handled by signalProcessGroup / killProcessGroup below. func setProcessGroup(cmd *exec.Cmd) {} -// signalProcessGroup signals the process itself (no group semantics on Windows). +// signalProcessGroup terminates the process tree led by p. +// +// Windows has no signal semantics — os.Process.Signal only supports Kill and +// returns EWINDOWS for everything else — so a "graceful signal" cannot be +// delivered and the process would otherwise linger until the caller's grace +// period expired. Terminating the tree here keeps shutdown prompt. func signalProcessGroup(p *os.Process, sig syscall.Signal) error { if p == nil { return nil } + if err := killProcessTree(p.Pid); err == nil { + return nil + } return p.Signal(sig) } -// killProcessGroup force-terminates the process. +// killProcessGroup force-terminates the process tree led by p. +// +// The tree, not just p: mxbuild.exe is a wrapper that launches a Deno web-ext +// worker (modeler/tools/deno/win-x64/deno.exe) which inherits the stdout/stderr +// pipe this package hands to exec.Cmd. Killing only the wrapper leaves that +// worker alive holding the pipe, so cmd.Wait() never sees EOF and blocks +// forever — which is why `mxcli run --local` used to hang at "Starting mxbuild +// --serve..." with no error. taskkill /T reaps the grandchild too. +// +// The direct p.Kill() is only a fallback for when taskkill is unavailable: once +// the tree kill has terminated p, calling Kill on it again returns "invalid +// argument", which must not be reported as a teardown failure. func killProcessGroup(p *os.Process) error { if p == nil { return nil } + if err := killProcessTree(p.Pid); err == nil { + return nil + } return p.Kill() } + +// killProcessTree force-terminates pid and all of its descendants, returning the +// taskkill error (or nil when pid <= 0, which is nothing to do). +func killProcessTree(pid int) error { + if pid <= 0 { + return nil + } + cmd := exec.Command("taskkill", "/F", "/T", "/PID", strconv.Itoa(pid)) + cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} + return cmd.Run() +} + +// processAlive reports whether p is still running. +// +// os.Process.Signal(syscall.Signal(0)) is NOT a liveness test on Windows: Go +// returns EWINDOWS ("not supported by windows") for every signal except Kill, so +// it reports a live process as dead. waitReady() relied on it, concluded mxbuild +// had exited the instant it was launched, and the local loop never got past +// "Starting mxbuild --serve...". WaitForSingleObject on a SYNCHRONIZE handle is +// the real check: it returns WAIT_TIMEOUT while the process runs and +// WAIT_OBJECT_0 once it has terminated (even before it is reaped). +func processAlive(p *os.Process) bool { + if p == nil { + return false + } + h, err := syscall.OpenProcess(syscall.SYNCHRONIZE, false, uint32(p.Pid)) + if err != nil { + return false + } + defer syscall.CloseHandle(h) + event, err := syscall.WaitForSingleObject(h, 0) + if err != nil { + return false + } + return event == syscall.WAIT_TIMEOUT +} diff --git a/cmd/mxcli/docker/procgroup_windows_test.go b/cmd/mxcli/docker/procgroup_windows_test.go new file mode 100644 index 000000000..9e58c5a66 --- /dev/null +++ b/cmd/mxcli/docker/procgroup_windows_test.go @@ -0,0 +1,191 @@ +// SPDX-License-Identifier: Apache-2.0 + +//go:build windows + +package docker + +import ( + "os" + "os/exec" + "strings" + "syscall" + "testing" + "time" +) + +// These tests exist because `mxcli run --local` hung forever on Windows at +// "Starting mxbuild --serve...". Two POSIX assumptions were behind it, both in +// the process helpers this file exercises: +// +// 1. alive() used os.Process.Signal(syscall.Signal(0)). Go returns EWINDOWS +// ("not supported by windows") for every signal except Kill — even for a +// process that is very much alive — so waitReady() concluded mxbuild had +// exited the instant it was launched. +// +// 2. Stop() waited on cmd.Wait(), and mxbuild.exe is a wrapper that launches a +// Deno web-ext worker which inherits the stdout/stderr pipe exec.Cmd hands +// out. Killing only the wrapper (the old killProcessGroup did p.Kill()) +// left the worker holding the pipe, so Wait() never saw EOF and blocked +// forever. +// +// The unix implementation is covered by procgroup_unix_test.go; this file is the +// Windows half, and the CI job that runs it is what keeps it from regressing. + +// TestWindowsProcessHelper is not a test of its own: it is the re-exec target the +// tests below use to get a real, controllable process tree on Windows (where +// there is no `sh -c`). It exits before the testing framework prints anything. +func TestWindowsProcessHelper(t *testing.T) { + switch os.Getenv("MXCLI_PROC_HELPER") { + case "": + return // normal `go test` run: this is not a test + case "sleep": + time.Sleep(60 * time.Second) + os.Exit(0) + case "spawn": + // A grandchild that inherits our stdout/stderr — the inherited pipe is + // exactly what kept cmd.Wait() blocked in the field. + gc := exec.Command("cmd", "/c", "ping", "-n", "60", "127.0.0.1") + gc.Stdout = os.Stdout + gc.Stderr = os.Stderr + if err := gc.Start(); err != nil { + os.Exit(3) + } + os.Stdout.WriteString("grandchild-started\n") + time.Sleep(60 * time.Second) + os.Exit(0) + } +} + +// helperCmd re-execs this test binary in the requested helper mode. +func helperCmd(t *testing.T, mode string) *exec.Cmd { + t.Helper() + cmd := exec.Command(os.Args[0], "-test.run=TestWindowsProcessHelper") + cmd.Env = append(os.Environ(), "MXCLI_PROC_HELPER="+mode) + return cmd +} + +// processAlive must report a running process as alive. The CONTROL in the middle +// is the point: it asserts that the API processAlive replaces really is unusable +// on Windows, so nobody "simplifies" the workaround back into the bug. +func TestProcessAlive_ReportsRunningProcess(t *testing.T) { + cmd := helperCmd(t, "sleep") + if err := cmd.Start(); err != nil { + t.Fatalf("start helper: %v", err) + } + t.Cleanup(func() { _ = cmd.Process.Kill() }) + + if !processAlive(cmd.Process) { + t.Fatal("processAlive() = false for a running process") + } + if err := cmd.Process.Signal(syscall.Signal(0)); err == nil { + t.Log("note: os.Process.Signal(0) succeeded on Windows; " + + "processAlive's WaitForSingleObject may no longer be required") + } + + _ = cmd.Process.Kill() + _ = cmd.Wait() + if processAlive(cmd.Process) { + t.Fatal("processAlive() = true for an exited process") + } +} + +func TestProcessAlive_FalseWhenNilOrExited(t *testing.T) { + if processAlive(nil) { + t.Fatal("processAlive(nil) = true") + } + cmd := exec.Command("cmd", "/c", "exit", "0") + if err := cmd.Start(); err != nil { + t.Fatalf("start: %v", err) + } + _ = cmd.Wait() + if processAlive(cmd.Process) { + t.Fatal("processAlive() = true for an exited process") + } +} + +// ServeServer.alive() is the exact call waitReady() makes. Before the fix it +// returned false for a live mxbuild, which is what aborted the local boot. +func TestServeServer_AliveTracksProcess(t *testing.T) { + if (&ServeServer{}).alive() { + t.Fatal("alive() = true with no process") + } + + cmd := helperCmd(t, "sleep") + if err := cmd.Start(); err != nil { + t.Fatalf("start helper: %v", err) + } + s := &ServeServer{cmd: cmd, log: &syncBuffer{}} + + if !s.alive() { + t.Fatal("ServeServer.alive() = false for a running process " + + "(the Windows Signal(0) bug that aborted the local boot)") + } + + _ = cmd.Process.Kill() + _ = cmd.Wait() + if s.alive() { + t.Fatal("ServeServer.alive() = true after the process exited") + } +} + +// LocalRuntime.alive() shares the same helper and the same fix. +func TestLocalRuntime_AliveTracksProcess(t *testing.T) { + cmd := helperCmd(t, "sleep") + if err := cmd.Start(); err != nil { + t.Fatalf("start helper: %v", err) + } + rt := &LocalRuntime{cmd: cmd, log: &syncBuffer{}} + + if !rt.alive() { + t.Fatal("LocalRuntime.alive() = false for a running process") + } + + _ = cmd.Process.Kill() + _ = cmd.Wait() + if rt.alive() { + t.Fatal("LocalRuntime.alive() = true after the process exited") + } +} + +// TestKillProcessGroup_ReapsGrandchildAndUnblocksWait is the regression test for +// the hang. The helper spawns a grandchild that inherits the stdout pipe; the old +// single-PID kill left it alive, so cmd.Wait() blocked forever (there was no +// error, no exit — just a hung `mxcli run --local`). A tree kill closes the pipe +// and Wait() returns. +// +// It would have failed on the pre-fix code: p.Kill() terminates only the helper, +// the grandchild keeps the write end open, and Wait() never returns. +func TestKillProcessGroup_ReapsGrandchildAndUnblocksWait(t *testing.T) { + var log syncBuffer + cmd := helperCmd(t, "spawn") + cmd.Stdout = &log + cmd.Stderr = &log + setProcessGroup(cmd) + if err := cmd.Start(); err != nil { + t.Fatalf("start helper: %v", err) + } + t.Cleanup(func() { _ = killProcessGroup(cmd.Process) }) + + // Wait for the grandchild to be running and holding the inherited pipe. + deadline := time.Now().Add(15 * time.Second) + for !strings.Contains(log.String(), "grandchild-started") { + if time.Now().After(deadline) { + t.Fatalf("helper never reported its grandchild; output so far: %q", log.String()) + } + time.Sleep(50 * time.Millisecond) + } + + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + + if err := killProcessGroup(cmd.Process); err != nil { + t.Fatalf("killProcessGroup: %v", err) + } + + select { + case <-done: + case <-time.After(20 * time.Second): + t.Fatal("cmd.Wait() did not return after killProcessGroup: a surviving " + + "grandchild still holds the stdout pipe (the old single-PID kill hung here)") + } +}