fix(windows): shared process.IsAlive for forge-cli and forge-ui (closes #59)#60
Merged
Conversation
#59) forge-ui's pidAlive used os.Process.Signal(syscall.Signal(0)) — the standard Unix idiom for "is this PID alive?". On Windows, Go's stdlib only translates os.Interrupt and os.Kill to platform calls; any other signal returns "operating system does not support signal". So pidAlive returned false on Windows regardless of whether the PID was alive, breaking `forge ui` startup detection: - waitForPort (forge-ui/process.go) fast-fails at the pidAlive check instead of looping until the TCP port responds. The UI shows "agent failed to start" and surfaces the daemon's normal startup banner (which by then is in serve.log) as the supposed error. - Discovery (forge-ui/discovery.go:133, now 134) thinks the daemon is dead and deletes .forge/serve.json — actively orphaning a still-running daemon from the dashboard on every page load. forge-cli already knew about this — serve_windows.go had a working OpenProcess-based isProcessAlive. The duplication is what caused the forge-ui side to miss it. Option A from the issue: extract a single platform-split helper in forge-core/util/process and delete the duplicates. - forge-core/util/process/process_{unix,windows}.go — IsAlive(pid). Unix: FindProcess + Signal(0). Windows: OpenProcess with PROCESS_QUERY_LIMITED_INFORMATION + CloseHandle. - forge-core/util/process/process_test.go — cross-platform tests: IsAlive(self) is true; IsAlive(spawned-then-waited subprocess pid) is false. The subprocess approach is deterministic on both Unix and Windows; picking arbitrary high PIDs is not. - forge-cli/cmd/serve_{unix,windows}.go — delete isProcessAlive, keep only the daemon-attr and signal helpers. - forge-cli/cmd/serve.go — switch to process.IsAlive (2 call sites). - forge-ui/process.go — delete pidAlive; switch waitForPort to process.IsAlive. Drop the now-unused syscall import. - forge-ui/discovery.go — switch to process.IsAlive. There is now exactly one implementation of "is this PID alive" in the tree, build-tag-split, consumed by both forge-cli and forge-ui. Verified: gofmt clean; golangci-lint 0 issues across all four modules; GOOS=windows go build succeeds for forge-core, forge-cli, forge-ui; native tests pass.
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
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.
Summary
Fixes #59 —
forge uion Windows reportsagent failed to startfor healthy daemons becauseforge-ui/process.go'spidAliveusedos.Process.Signal(syscall.Signal(0)), which is Unix-only. On Windows, Go's stdlib only translatesos.Interruptandos.Kill; every other signal — includingSignal(0)— returns"operating system does not support signal". SopidAlivereturnedfalseregardless of whether the PID was alive, breaking both startup detection and discovery on Windows.Symptom chain
forge serve startwaitForPortreads PID from.forge/serve.jsonpidAlive(pid)checks the child is still aliveSignal(0)succeeds →trueSignal(0)errors →falsewaitForPortoutcometruefalsereadServeLogsserve.logcontains the daemon's normal startup bannerforge-ui/discovery.go:133had the same broken check — on Windows it deleted.forge/serve.jsonon every page-load discovery scan, orphaning still-running daemons from the dashboard.The forge-cli daemon code already had the correct platform-aware variant (
forge-cli/cmd/serve_windows.go'sisProcessAliveusesOpenProcess+CloseHandle). The duplication is exactly what caused the forge-ui side to miss the Windows-specific implementation.Approach — Option A from #59
Extract the platform-aware check into a single helper consumed by both
forge-cliandforge-ui. Build-tag split lives in exactly one place.New package:
forge-core/util/process/Call sites updated:
forge-cli/cmd/serve_{unix,windows}.go— deletedisProcessAlive(two duplicates removed).forge-cli/cmd/serve.go— 2 call sites switched toprocess.IsAlive.forge-ui/process.go— deletedpidAlive;waitForPortnow callsprocess.IsAlive. Dropped the now-unusedsyscallimport.forge-ui/discovery.go— usesprocess.IsAlive(this was the actively destructive serve.json-deletion site).There is now exactly one implementation of "is this PID alive" in the tree.
Tests
forge-core/util/process/process_test.go:TestIsAlive_Self—IsAlive(os.Getpid())must betrue.TestIsAlive_ChildAfterExit— spawn a short-lived subprocess (trueon Unix,cmd /c exit 0on Windows), wait for it to exit, assertIsAlive(pid)returnsfalse. Subprocess-based assertion is deterministic across platforms — arbitrary high PIDs are not.Verification
CI doesn't currently run Windows; the
GOOS=windows go buildstep at minimum proves the build-tag split is correct and the package compiles for Windows. The runtime behavior is covered by the OpenProcess + CloseHandle pattern thatforge-cli/cmd/serve_windows.goalready shipped — the helper just consolidates it.Acceptance criteria from #59
truefor live PIDs andfalsefor exited PIDs (proven by the build-tag-split implementation and theforge-clidaemon code that has shipped using this pattern).forge uino longer fast-fails startup on Windows when the daemon is healthy.forge uino longer deletes.forge/serve.jsonfor still-running daemons on Windows.Signal(0)is preserved on Unix.Manual verification
forge ui, start an agent. Confirm the dashboard shows the running state, not "agent failed to start"..forge/serve.jsonis still there and the dashboard still sees the agent.Files changed
Total: +122 / -32. Net new code is in the helper package and its tests; net removed code is the duplicates.