diff --git a/.github/workflows/windows-process.yml b/.github/workflows/windows-process.yml index 9e0232cdd..f0f6cc3e4 100644 --- a/.github/workflows/windows-process.yml +++ b/.github/workflows/windows-process.yml @@ -16,10 +16,14 @@ on: branches: [main] paths: - "internal/util/process*.go" + - "internal/runfile/*.go" + - "internal/restart/*.go" - ".github/workflows/windows-process.yml" pull_request: paths: - "internal/util/process*.go" + - "internal/runfile/*.go" + - "internal/restart/*.go" - ".github/workflows/windows-process.yml" workflow_dispatch: @@ -40,3 +44,12 @@ jobs: - name: Process liveness tests shell: pwsh run: go test -tags goolm -count=1 -v -run "IsProcessAlive" ./internal/util/ + - name: Runfile/restart Windows suites (#3079) + # Same real-machine policy as above: processExists wait-state semantics + # (#3079/#1290) and the restart script generator only behave as written + # on a real Windows kernel. Full package suites - the windows-tagged + # probes (exit-code-259 child, creation-time token, cd warning) were + # previously never executed anywhere (ci.yml Test is ubuntu-only and + # this workflow's paths only matched internal/util). + shell: pwsh + run: go test -tags goolm -count=1 ./internal/runfile/ ./internal/restart/ diff --git a/internal/restart/restart_windows.go b/internal/restart/restart_windows.go index 9dee739bb..724520544 100644 --- a/internal/restart/restart_windows.go +++ b/internal/restart/restart_windows.go @@ -46,8 +46,12 @@ echo [ggcode restart] process %PARENT_PID% exited REM 2. Brief pause ping -n 1 127.0.0.1 >nul -REM 3. cd to original working directory +REM 3. cd to original working directory. #3079: warn on failure instead of +REM silently launching from the wrong cwd when the work dir was deleted +REM during the restart wait - the user sees why the binary path resolved +REM differently instead of an unexplained launch failure. cd /d "%WORK_DIR%" 2>nul +if errorlevel 1 echo [ggcode restart] WARNING: could not cd to "%WORK_DIR%" - launching from "%CD%" REM 4. Self-delete: the classic 'start /b del' races the parent cmd's open REM file handle and loses 100% of the time (#798). The (goto) 2>nul idiom diff --git a/internal/runfile/process_windows.go b/internal/runfile/process_windows.go index 41709257c..3aaefeae8 100644 --- a/internal/runfile/process_windows.go +++ b/internal/runfile/process_windows.go @@ -3,18 +3,27 @@ package runfile import ( - "os" + "fmt" "syscall" ) // processExists checks if a process with the given PID is running. // On Windows, syscall.Kill is not available; we use OpenProcess instead. +// +// #3079: the legacy GetExitCodeProcess==259 check was ambiguous - 259 is +// STILL_ACTIVE, but a process whose REAL exit code happened to be exactly +// 259 stayed "alive" forever. The wait state of the handle is unambiguous: +// WaitForSingleObject(handle, 0) returns WAIT_TIMEOUT while the process +// lives and WAIT_OBJECT_0 once it terminated, whatever its exit code was. func processExists(pid int) bool { if pid <= 0 { return false } - const PROCESS_QUERY_LIMITED_INFORMATION = 0x1000 - handle, err := syscall.OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, false, uint32(pid)) + const ( + PROCESS_QUERY_LIMITED_INFORMATION = 0x1000 + SYNCHRONIZE = 0x00100000 + ) + handle, err := syscall.OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION|SYNCHRONIZE, false, uint32(pid)) if err != nil { // #1290: mirror the Unix #799 EPERM semantics. ERROR_ACCESS_DENIED // (cross-user, protected/elevated process under UAC integrity-level @@ -27,17 +36,43 @@ func processExists(pid int) bool { return false } defer syscall.CloseHandle(handle) + + // Preferred: wait-state check, immune to the STILL_ACTIVE=259 exit-code + // ambiguity (#3079). + if event, waitErr := syscall.WaitForSingleObject(handle, 0); waitErr == nil { + // WAIT_OBJECT_0 (signaled) = terminated; WAIT_TIMEOUT = still alive. + return event == syscall.WAIT_TIMEOUT + } + // Degraded path (wait right unavailable, e.g. some protected processes + // grant query but not synchronize): fall back to the legacy heuristic + // rather than misreporting a live process as dead. var exitCode uint32 - err = syscall.GetExitCodeProcess(handle, &exitCode) - if err != nil { + if err := syscall.GetExitCodeProcess(handle, &exitCode); err != nil { return false } return exitCode == 259 // STILL_ACTIVE } -// PID is a convenience helper for the current process. -var _ = os.Getpid - -// procStartTime is the unix identity token (#1624 case C); Windows has no -// /proc, so the identity check degrades to signal-0 liveness. -func procStartTime(pid int) string { return "" } +// procStartTime returns the process-creation identity token (#1624 case C). +// Windows has no /proc; the token is the raw GetProcessTimes CreationTime +// FILETIME (opaque but unique per process start), which is all the +// PID-recycle check needs. A non-empty token here (re)activates the recycle +// fallback on Windows, which previously returned "" and degraded to plain +// liveness - a recycled PID kept its ghost port file until the number was +// reused by yet another process (#3079). +func procStartTime(pid int) string { + if pid <= 0 { + return "" + } + const PROCESS_QUERY_LIMITED_INFORMATION = 0x1000 + handle, err := syscall.OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, false, uint32(pid)) + if err != nil { + return "" + } + defer syscall.CloseHandle(handle) + var create, exit, kernel, user syscall.Filetime + if err := syscall.GetProcessTimes(handle, &create, &exit, &kernel, &user); err != nil { + return "" + } + return fmt.Sprintf("%d", uint64(create.HighDateTime)<<32|uint64(create.LowDateTime)) +} diff --git a/internal/runfile/zz_issue3079_test.go b/internal/runfile/zz_issue3079_test.go new file mode 100644 index 000000000..bd0742ded --- /dev/null +++ b/internal/runfile/zz_issue3079_test.go @@ -0,0 +1,75 @@ +//go:build windows + +package runfile + +// Regression probes for #3079 (process_windows.go): +// V2a: exit code 259 is STILL_ACTIVE, but a process whose REAL exit code +// was exactly 259 was judged alive forever (GetExitCodeProcess==259 +// ambiguity); the wait-state check is unambiguous. +// V2b: procStartTime returned "" on Windows, fully disabling the #1624 +// PID-recycle fallback (runfile.go's gate is skipped when either +// side has no token). +// +// Runs only on Windows CI; GOOS=windows compile/vet gates the rest. + +import ( + "os" + "testing" +) + +// TestIssue3079_ExitCode259IsDead is THE V2a regression probe: a process +// that genuinely exited with code 259 must be reported dead. +func TestIssue3079_ExitCode259IsDead(t *testing.T) { + p, err := os.StartProcess(`C:\Windows\System32\cmd.exe`, []string{`/c`, `exit 259`}, &os.ProcAttr{}) + if err != nil { + t.Skipf("cannot spawn cmd.exe: %v", err) + } + ws, err := p.Wait() + if err != nil { + t.Fatalf("wait: %v", err) + } + if ws.ExitCode() != 259 { + t.Skipf("child exit code = %d, want 259 (cannot build the ambiguity case)", ws.ExitCode()) + } + if processExists(p.Pid) { + t.Fatal("process that exited with code 259 reported alive (V2a: STILL_ACTIVE ambiguity)") + } +} + +// TestIssue3079_ProcStartTimeToken covers V2b: non-empty for a live pid, +// stable across calls, empty for a dead pid. +func TestIssue3079_ProcStartTimeToken(t *testing.T) { + self := os.Getpid() + tok1 := procStartTime(self) + if tok1 == "" { + t.Fatal("procStartTime(self) empty - PID-recycle fallback still disabled (V2b)") + } + if tok2 := procStartTime(self); tok1 != tok2 { + t.Fatalf("creation-time token unstable across calls: %q vs %q", tok1, tok2) + } + for _, r := range tok1 { + if r < '0' || r > '9' { + t.Fatalf("token not a plain decimal number: %q", tok1) + } + } + + p, err := os.StartProcess(`C:\Windows\System32\cmd.exe`, []string{`/c`, `exit`}, &os.ProcAttr{}) + if err != nil { + t.Skipf("cannot spawn cmd.exe: %v", err) + } + _, _ = p.Wait() + if got := procStartTime(p.Pid); got != "" { + t.Fatalf("procStartTime(dead pid) = %q, want empty (V2b)", got) + } +} + +// TestIssue3079_SelfAlive pins the basics after the rewrite (companion of +// the #1290 suite). +func TestIssue3079_SelfAlive(t *testing.T) { + if !processExists(os.Getpid()) { + t.Fatal("current process must be reported alive via wait-state check") + } + if processExists(0) { + t.Fatal("pid 0 must be dead") + } +}