From ea6f79565e8fbcb537fc2ceb2d1bf7eaeb9868d9 Mon Sep 17 00:00:00 2001 From: Rudra Singh Date: Wed, 9 Sep 2026 15:12:28 +0530 Subject: [PATCH 1/5] fix(ui): surface stopping state and stop clobbering the agent card MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking Stop on the dashboard left the card reading running with a live Stop button until the stop finished, then repainted it wrong — metadata tags blanked, a stale port still showing — so only a manual refresh gave a correct card back. Three defects stacked: - StateStopping was never assigned anywhere. The card already renders it (status dot, spinner, disabled buttons via isBusy), so the UI was waiting on a signal the backend never sent. - The only broadcast happened AFTER the blocking `forge serve stop`, and handleStopAgent is synchronous, so nothing reached the browser until the child exited. Announce before cmd.Run() instead: handleSSE runs on its own goroutine and delivers while Stop is still blocked. - Stop broadcast a stub &AgentInfo{ID, Directory, Status}. A partial struct is not a partial JSON object — version/framework/model/tools/channels/ skills have no omitempty and marshal as zero values, which the dashboard's object-spread merge wrote over the real values, while `port,omitempty` was omitted and left the stale running port in place. Stop now takes the full *AgentInfo so every event carries a complete record, and clears Port. Also route all six broadcasts through broadcastStatus, which sends a snapshot rather than the pointer: SSEBroker queues events and handleSSE marshals them on drain, so sharing the live pointer let a later mutation rewrite an already-queued event — the stopping event would serialize as stopped. Start had the same latent aliasing. A failed stop now rolls status/port back and attaches stderr, so the card can't wedge on stopping with both buttons disabled. handleStop also flips optimistically on click, since Broadcast drops events on a full buffer and the click shouldn't depend on the stream. Tests: new TestProcessManagerStopBroadcastsStoppingFirst asserts the first event is stopping, carries full metadata, and is not the caller's pointer; TestProcessManagerStopExecError gains rollback assertions. --- forge-ui/handlers.go | 2 +- forge-ui/process.go | 57 ++++++++++++++++++++++++++++++-------- forge-ui/process_test.go | 60 +++++++++++++++++++++++++++++++++++++++- forge-ui/static/app.js | 11 ++++++++ 4 files changed, 117 insertions(+), 13 deletions(-) diff --git a/forge-ui/handlers.go b/forge-ui/handlers.go index 64ed7f15..3cb4f3de 100644 --- a/forge-ui/handlers.go +++ b/forge-ui/handlers.go @@ -126,7 +126,7 @@ func (s *UIServer) handleStopAgent(w http.ResponseWriter, r *http.Request) { return } - if err := s.pm.Stop(id, agent.Directory); err != nil { + if err := s.pm.Stop(id, agent); err != nil { writeError(w, http.StatusConflict, err.Error()) return } diff --git a/forge-ui/process.go b/forge-ui/process.go index d828d9e8..0229e56c 100644 --- a/forge-ui/process.go +++ b/forge-ui/process.go @@ -89,6 +89,15 @@ func NewProcessManager(exePath string, broker *SSEBroker, basePort int) *Process } } +// broadcastStatus emits a snapshot of info rather than the pointer itself. +// SSEBroker queues events and handleSSE marshals them when it drains the +// channel, so sharing the pointer would let a later mutation rewrite an +// already-queued event — a "stopping" event would serialize as "stopped". +func (pm *ProcessManager) broadcastStatus(info *AgentInfo) { + snapshot := *info + pm.broker.Broadcast(SSEEvent{Type: "agent_status", Data: &snapshot}) +} + // Start launches an agent via `forge serve start`. func (pm *ProcessManager) Start(agentID string, info *AgentInfo, passphrase string) error { pm.mu.Lock() @@ -124,7 +133,7 @@ func (pm *ProcessManager) Start(agentID string, info *AgentInfo, passphrase stri info.Status = StateErrored info.Error = errMsg - pm.broker.Broadcast(SSEEvent{Type: "agent_status", Data: info}) + pm.broadcastStatus(info) return fmt.Errorf("forge serve start failed: %s", errMsg) } @@ -144,7 +153,7 @@ func (pm *ProcessManager) Start(agentID string, info *AgentInfo, passphrase stri info.Status = StateErrored info.Error = errMsg - pm.broker.Broadcast(SSEEvent{Type: "agent_status", Data: info}) + pm.broadcastStatus(info) return fmt.Errorf("agent failed to start: %s", errMsg) } @@ -152,7 +161,7 @@ func (pm *ProcessManager) Start(agentID string, info *AgentInfo, passphrase stri info.Status = StateRunning info.Port = port info.Error = "" - pm.broker.Broadcast(SSEEvent{Type: "agent_status", Data: info}) + pm.broadcastStatus(info) return nil } @@ -238,15 +247,41 @@ func (pm *ProcessManager) readServeLogs(agentDir string) string { } // Stop stops an agent via `forge serve stop`. -func (pm *ProcessManager) Stop(agentID string, agentDir string) error { +// +// Takes the full *AgentInfo so every broadcast carries a complete record: +// fields without omitempty marshal as zero values, and the dashboard merges +// events by object spread, so a stub event blanks the card's metadata. +func (pm *ProcessManager) Stop(agentID string, info *AgentInfo) error { pm.mu.Lock() defer pm.mu.Unlock() + // Announce before the blocking cmd.Run() — the HTTP handler won't + // respond until it returns, so this is the only thing that can flip + // the card to "stopping". SSE is served on its own goroutine. + prevStatus, prevPort := info.Status, info.Port + info.Status = StateStopping + pm.broadcastStatus(info) + cmd := exec.Command(pm.exePath, "serve", "stop") - cmd.Dir = agentDir + cmd.Dir = info.Directory + + var stderr bytes.Buffer + cmd.Stderr = &stderr if err := cmd.Run(); err != nil { - return fmt.Errorf("forge serve stop failed: %w", err) + errMsg := strings.TrimSpace(stderr.String()) + if errMsg == "" { + errMsg = err.Error() + } + + // Roll back, or the card stays wedged on "stopping" with its + // buttons disabled. + info.Status = prevStatus + info.Port = prevPort + info.Error = errMsg + pm.broadcastStatus(info) + + return fmt.Errorf("forge serve stop failed: %s", errMsg) } if port, ok := pm.allocated[agentID]; ok { @@ -254,11 +289,11 @@ func (pm *ProcessManager) Stop(agentID string, agentDir string) error { delete(pm.allocated, agentID) } - pm.broker.Broadcast(SSEEvent{Type: "agent_status", Data: &AgentInfo{ - ID: agentID, - Directory: agentDir, - Status: StateStopped, - }}) + // Clear the port — otherwise a stopped card still shows a port tag. + info.Status = StateStopped + info.Port = 0 + info.Error = "" + pm.broadcastStatus(info) return nil } diff --git a/forge-ui/process_test.go b/forge-ui/process_test.go index bc2d23c5..103fa244 100644 --- a/forge-ui/process_test.go +++ b/forge-ui/process_test.go @@ -57,10 +57,68 @@ func TestProcessManagerStopExecError(t *testing.T) { broker := NewSSEBroker() pm := NewProcessManager("/nonexistent/binary", broker, 9100) - err := pm.Stop("nonexistent", t.TempDir()) + info := &AgentInfo{ + ID: "nonexistent", + Directory: t.TempDir(), + Status: StateRunning, + Port: 9100, + } + + err := pm.Stop("nonexistent", info) if err == nil { t.Error("expected error stopping with non-existent binary") } + + // A failed stop must roll back, not leave the card on "stopping". + if info.Status != StateRunning { + t.Errorf("status = %q, want %q after failed stop", info.Status, StateRunning) + } + if info.Port != 9100 { + t.Errorf("port = %d, want 9100 preserved after failed stop", info.Port) + } + if info.Error == "" { + t.Error("expected Error to be populated after failed stop") + } +} + +// Stop must emit a "stopping" event BEFORE it blocks on `forge serve stop`, +// and every event must carry a full record — a stub would blank the card's +// metadata in the dashboard. +func TestProcessManagerStopBroadcastsStoppingFirst(t *testing.T) { + broker := NewSSEBroker() + ch := broker.Subscribe() + defer broker.Unsubscribe(ch) + + pm := NewProcessManager("/nonexistent/binary", broker, 9100) + + info := &AgentInfo{ + ID: "test-agent", + Version: "1.2.3", + Framework: "langchain", + Channels: []string{"slack"}, + Directory: t.TempDir(), + Status: StateRunning, + Port: 9100, + } + + _ = pm.Stop("test-agent", info) + + first := <-ch + got, ok := first.Data.(*AgentInfo) + if !ok { + t.Fatalf("event data type = %T, want *AgentInfo", first.Data) + } + if got.Status != StateStopping { + t.Errorf("first event status = %q, want %q", got.Status, StateStopping) + } + if got.Version != "1.2.3" || got.Framework != "langchain" || len(got.Channels) != 1 { + t.Errorf("first event dropped metadata: %+v", got) + } + // The event must be a snapshot, not the live pointer — otherwise + // Stop's later mutations rewrite an already-queued event. + if got == info { + t.Error("event aliases the caller's AgentInfo; broadcast a copy") + } } func TestProcessManagerStopAll(t *testing.T) { diff --git a/forge-ui/static/app.js b/forge-ui/static/app.js index b589331d..5c9f52b4 100644 --- a/forge-ui/static/app.js +++ b/forge-ui/static/app.js @@ -3286,10 +3286,21 @@ function App() { }, [passphrasePrompt]); const handleStop = useCallback(async (id) => { + // Flip to "stopping" on click. The server broadcasts this too, but + // SSEBroker.Broadcast drops events for a full buffer, so the click + // must not depend on the stream to feel responsive. + setAgents(prev => prev.map(a => + a.id === id ? { ...a, status: 'stopping', error: '' } : a + )); try { await stopAgent(id); } catch (err) { console.error('Failed to stop agent:', err); + // The server rolls its own state back; mirror that locally so the + // card doesn't stay stuck on "stopping" with disabled buttons. + setAgents(prev => prev.map(a => + a.id === id ? { ...a, status: 'running', error: err.message } : a + )); } }, []); From b732c1f3db66ad8148651a1320f39ed50cc48bf8 Mon Sep 17 00:00:00 2001 From: Rudra Singh Date: Wed, 9 Sep 2026 15:15:12 +0530 Subject: [PATCH 2/5] fix(process): don't treat handle openability as liveness on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `forge serve stop` reported failure on Windows even though the daemon was already dead, dumping `sending SIGKILL: TerminateProcess: Access is denied` plus the whole cobra help text into the dashboard. IsAlive used OpenProcess success as the liveness test, but Windows keeps a terminated process's kernel object alive as long as any handle to it stays open (so callers can still read its exit code), and OpenProcess succeeds against that object. serveStopRun holds exactly such a handle — from os.FindProcess, needed to do the killing — for the whole function, so the stop path pinned its own dead child and then asked whether it was running: 1. sendTermSignal -> proc.Kill(), daemon actually dies 2. IsAlive keeps reporting alive: the held handle keeps the object resolvable 3. full 10s poll loop burns 4. sendKillSignal on the dead process -> ERROR_ACCESS_DENIED 5. non-zero exit; the stop succeeded but reported failure Open the handle with SYNCHRONIZE as well and wait on it with a zero timeout instead. A process handle becomes signaled exactly when the process exits, which is unambiguous: WAIT_TIMEOUT means still running, signaled means gone. GetExitCodeProcess was the other option but its STILL_ACTIVE sentinel (259) collides with a genuine exit code of 259. Also in forge-cli: - serve stop/status/logs were missing SilenceUsage, so any error dumped flag help after it (serve and start already had it). - A kill that fails because the target is already gone is now a successful stop, closing the race between the liveness check and the kill. Correct on Unix too, where killing a dead pid returns ESRCH. TestIsAlive_ChildAfterExit could not catch this: cmd.Wait() closes Go's handle, releasing the object before the check runs. The new TestIsAlive_FalseWhileHandleHeld holds a handle across the kill, and fails against the old implementation. Windows-only (//go:build windows); process_unix.go is untouched and Unix keeps its Signal(0) probe. --- forge-cli/cmd/serve.go | 31 +++++++--- forge-core/util/process/process_windows.go | 43 ++++++++++---- .../util/process/process_windows_test.go | 57 +++++++++++++++++++ 3 files changed, 112 insertions(+), 19 deletions(-) create mode 100644 forge-core/util/process/process_windows_test.go diff --git a/forge-cli/cmd/serve.go b/forge-cli/cmd/serve.go index 84c99009..9481150a 100644 --- a/forge-cli/cmd/serve.go +++ b/forge-cli/cmd/serve.go @@ -91,21 +91,24 @@ var serveStartCmd = &cobra.Command{ } var serveStopCmd = &cobra.Command{ - Use: "stop", - Short: "Stop the running agent daemon", - RunE: serveStopRun, + Use: "stop", + Short: "Stop the running agent daemon", + SilenceUsage: true, // a failed stop shouldn't bury the error in flag help + RunE: serveStopRun, } var serveStatusCmd = &cobra.Command{ - Use: "status", - Short: "Show agent daemon status", - RunE: serveStatusRun, + Use: "status", + Short: "Show agent daemon status", + SilenceUsage: true, + RunE: serveStatusRun, } var serveLogsCmd = &cobra.Command{ - Use: "logs", - Short: "Tail daemon log output", - RunE: serveLogsRun, + Use: "logs", + Short: "Tail daemon log output", + SilenceUsage: true, + RunE: serveLogsRun, } func registerServeFlags(cmd *cobra.Command) { @@ -370,6 +373,16 @@ func serveStopRun(cmd *cobra.Command, args []string) error { fmt.Fprintln(os.Stderr, "Daemon did not stop in time, sending SIGKILL...") if err := sendKillSignal(proc); err != nil { os.Remove(statePath) //nolint:errcheck + + // The daemon can exit between the last liveness check and this + // kill, and killing an already-dead process is an error on both + // platforms (Windows reports "Access is denied"). The goal was + // for it to be gone, and it is — that's a successful stop. + if !process.IsAlive(state.PID) { + fmt.Fprintln(os.Stderr, "Daemon stopped.") + return nil + } + return fmt.Errorf("sending SIGKILL: %w", err) } diff --git a/forge-core/util/process/process_windows.go b/forge-core/util/process/process_windows.go index 91c575fd..5d7c0ef2 100644 --- a/forge-core/util/process/process_windows.go +++ b/forge-core/util/process/process_windows.go @@ -13,6 +13,14 @@ import "syscall" // rights than this check needs. const processQueryLimitedInfo = 0x1000 +// processSynchronize (SYNCHRONIZE) is required to wait on a process +// handle, which is how liveness is actually determined below. +const processSynchronize = 0x00100000 + +// waitTimeout (WAIT_TIMEOUT) is WaitForSingleObject's answer when the +// handle is NOT signaled — i.e. the process has not exited. +const waitTimeout = 0x00000102 + // IsAlive reports whether a process with the given PID is currently running. // // On Windows, the Unix idiom os.Process.Signal(syscall.Signal(0)) does not @@ -21,19 +29,34 @@ const processQueryLimitedInfo = 0x1000 // "operating system does not support signal". This always-error response // makes Signal(0) useless as a liveness probe on Windows. // -// Instead, open the process handle with PROCESS_QUERY_LIMITED_INFORMATION -// rights. OpenProcess fails only when the PID doesn't exist or the caller -// lacks rights even for the limited-info subset; both cases reasonably map -// to "not alive" for our use case (the forge daemon is the caller's child, -// so it always has rights to its own PID). +// Handle openability is NOT liveness either: Windows keeps a terminated +// process's kernel object alive as long as any handle to it remains open +// (so callers can still read its exit code), and OpenProcess succeeds +// against that object. A caller that holds a handle while polling — as +// `forge serve stop` does via os.FindProcess — would therefore see its +// own dead child reported as running forever, wait out the full timeout, +// and then fail with "Access is denied" trying to kill it twice. // -// The handle is closed immediately — we only care that OpenProcess -// succeeded, not what the handle exposes. +// So open the handle and wait on it with a zero timeout instead. A +// process handle becomes signaled exactly when the process exits, which +// is unambiguous: signaled means exited, WAIT_TIMEOUT means still +// running. (GetExitCodeProcess is the other option, but its STILL_ACTIVE +// sentinel is 259 and collides with a genuine exit code of 259.) func IsAlive(pid int) bool { - h, err := syscall.OpenProcess(processQueryLimitedInfo, false, uint32(pid)) + h, err := syscall.OpenProcess(processQueryLimitedInfo|processSynchronize, false, uint32(pid)) + if err != nil { + // PID doesn't exist, or we lack even limited-info rights — + // both map to "not alive" for our use case (the forge daemon + // is the caller's child, so rights are never the issue). + return false + } + defer syscall.CloseHandle(h) //nolint:errcheck + + event, err := syscall.WaitForSingleObject(h, 0) if err != nil { + // WAIT_FAILED. Report not-alive rather than pinning a caller + // in a poll loop it can never exit. return false } - _ = syscall.CloseHandle(h) - return true + return event == waitTimeout } diff --git a/forge-core/util/process/process_windows_test.go b/forge-core/util/process/process_windows_test.go new file mode 100644 index 00000000..03e084af --- /dev/null +++ b/forge-core/util/process/process_windows_test.go @@ -0,0 +1,57 @@ +//go:build windows + +package process + +import ( + "os/exec" + "syscall" + "testing" +) + +// Windows keeps a terminated process's kernel object alive while any +// handle to it stays open, so OpenProcess still succeeds against a dead +// PID. `forge serve stop` holds exactly such a handle (from +// os.FindProcess) while it polls IsAlive, which made an +// openability-based check report its own dead child as running until the +// timeout expired — then fail with "Access is denied" on the second kill. +// +// TestIsAlive_ChildAfterExit does not cover this: cmd.Wait() closes Go's +// handle, releasing the object before the check runs. The handle must be +// held across the kill to reproduce it. +func TestIsAlive_FalseWhileHandleHeld(t *testing.T) { + // ping runs long enough to be observed alive and needs no stdin + // (timeout/pause both fail when stdin isn't a console). + cmd := exec.Command("ping", "-n", "30", "127.0.0.1") + if err := cmd.Start(); err != nil { + t.Fatalf("starting subprocess: %v", err) + } + pid := cmd.Process.Pid + defer func() { + _ = cmd.Process.Kill() + _, _ = cmd.Process.Wait() + }() + + // Mirror os.FindProcess: an independent handle held across the kill. + h, err := syscall.OpenProcess(processQueryLimitedInfo|processSynchronize, false, uint32(pid)) + if err != nil { + t.Fatalf("OpenProcess: %v", err) + } + defer syscall.CloseHandle(h) //nolint:errcheck + + if !IsAlive(pid) { + t.Fatalf("IsAlive(pid=%d) = false while running, want true", pid) + } + + if err := cmd.Process.Kill(); err != nil { + t.Fatalf("Kill: %v", err) + } + if _, err := cmd.Process.Wait(); err != nil { + t.Fatalf("Wait: %v", err) + } + + // h is still open, so the PID remains openable. IsAlive must not be + // fooled by that. + if IsAlive(pid) { + t.Errorf("IsAlive(pid=%d) = true for a terminated process while a handle is held, want false", pid) + } +} From b9f974f88d12900e3f4994183e3d08c5c2ade98e Mon Sep 17 00:00:00 2001 From: Rudra Singh Date: Wed, 9 Sep 2026 15:24:19 +0530 Subject: [PATCH 3/5] fix(chat): ensure proper cleanup of chat stream on component unmount --- forge-ui/static/app.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/forge-ui/static/app.js b/forge-ui/static/app.js index 5c9f52b4..0e15d5cf 100644 --- a/forge-ui/static/app.js +++ b/forge-ui/static/app.js @@ -461,6 +461,13 @@ function useChatStream(agentId) { const [sessionId, setSessionId] = useState(null); const abortRef = useRef(null); + // Abort any in-flight stream on unmount. Switching agents remounts this + // hook (ChatPage is keyed by agent id), so without this the previous + // agent's request keeps streaming into a dead component. + useEffect(() => () => { + if (abortRef.current) abortRef.current.abort(); + }, []); + const loadSession = useCallback(async (sid) => { try { const data = await fetchSession(agentId, sid); @@ -3321,7 +3328,12 @@ function App() { const renderPage = () => { switch (route.page) { case 'chat': - return html`<${ChatPage} agentId=${route.params.id} agents=${agents} />`; + // key forces a fresh instance per agent. Without it Preact reuses + // the mounted ChatPage on an agentId change, and useState survives + // — so messages/sessionId/streaming stay on the previous agent + // while the header and session list (props / agentId-keyed effect) + // correctly re-render. + return html`<${ChatPage} key=${route.params.id} agentId=${route.params.id} agents=${agents} />`; case 'create': return html`<${CreatePage} />`; case 'config': From 4d950a1fba63edde1b3aa2185410cf11f1963a35 Mon Sep 17 00:00:00 2001 From: Rudra Singh Date: Wed, 9 Sep 2026 15:31:58 +0530 Subject: [PATCH 4/5] fix(sse): ensure all event types are handled in EventSource listeners --- forge-ui/static/app.js | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/forge-ui/static/app.js b/forge-ui/static/app.js index 0e15d5cf..217ca5a7 100644 --- a/forge-ui/static/app.js +++ b/forge-ui/static/app.js @@ -256,12 +256,19 @@ function useSSE(onEvent) { useEffect(() => { const es = new EventSource('/api/events'); - es.addEventListener('agent_status', (e) => { + // EventSource dispatches by `event:` name, so every type the server + // broadcasts needs an explicit listener — an unlisted one is silently + // dropped. agent_created (handlers_create.go) was being dropped, which + // is why a newly created agent only appeared after a refresh or the + // 60s poll. The type is forwarded so the caller can tell them apart. + const forward = (type) => (e) => { try { - const data = JSON.parse(e.data); - callbackRef.current(data); + callbackRef.current(type, JSON.parse(e.data)); } catch { /* ignore parse errors */ } - }); + }; + + es.addEventListener('agent_status', forward('agent_status')); + es.addEventListener('agent_created', forward('agent_created')); es.onerror = () => { // EventSource auto-reconnects @@ -3240,7 +3247,15 @@ function App() { }, [loadAgents]); // SSE real-time updates - useSSE((agentData) => { + useSSE((type, agentData) => { + // agent_created carries only {id, directory}, not a full AgentInfo, so + // refetch to pick up the record the cards render from (model, tools, + // channels, status). The merge below deliberately ignores unknown ids, + // so a create can't be handled there. + if (type === 'agent_created') { + loadAgents(); + return; + } setAgents(prev => { const idx = prev.findIndex(a => a.id === agentData.id); if (idx === -1) return prev; From 78c9faab7876ede1abfb61c5010c84dfbaa847c1 Mon Sep 17 00:00:00 2001 From: Rudra Singh Date: Wed, 9 Sep 2026 19:13:10 +0530 Subject: [PATCH 5/5] review(ui): document broadcastStatus aliasing + restore true prior status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback: - broadcastStatus: spell out that *info is a SHALLOW copy. It decouples the scalar fields Start/Stop mutate (Status, Port, Error), but Tools, Channels and DeniedChannels still alias the caller's backing arrays and StartedAt stays a shared pointer. That is only safe because those are populated once by Scanner.scanDir and read-only afterwards; mutating a slice element in place would reintroduce the aliasing bug, so deep-copy if that changes. - handleStop rolled back to a hardcoded 'running', which mislabels an agent that was in some other state (e.g. already errored) for the window before the authoritative agent_status event lands. Capture the real prior status and restore that, mirroring ProcessManager.Stop's rollback-to-previous. Read from the rendered `agents` rather than inside the setAgents updater: the updater is not guaranteed to have run by the time the request settles, and assigning to a closure variable from it would be an impure reducer. handleStop therefore takes `agents` as a dependency, matching handleStart. Kept the optimistic rollback rather than leaning solely on SSE, since SSEBroker.Broadcast drops events when a subscriber's buffer is full — the case the optimistic update exists to cover. The code-agent prompt change (runner.go + code-agent/SKILL.md) has been split out of this branch onto fix/code-agent-prompt, as requested. --- forge-ui/process.go | 8 ++++++++ forge-ui/static/app.js | 16 ++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/forge-ui/process.go b/forge-ui/process.go index 0229e56c..933d599b 100644 --- a/forge-ui/process.go +++ b/forge-ui/process.go @@ -93,6 +93,14 @@ func NewProcessManager(exePath string, broker *SSEBroker, basePort int) *Process // SSEBroker queues events and handleSSE marshals them when it drains the // channel, so sharing the pointer would let a later mutation rewrite an // already-queued event — a "stopping" event would serialize as "stopped". +// +// The snapshot is a SHALLOW copy: it decouples the scalar fields this package +// mutates (Status, Port, Error), but Tools, Channels and DeniedChannels still +// alias the caller's backing arrays, and StartedAt stays a shared pointer. +// That is safe only because Start/Stop never write through them — they are +// populated once by Scanner.scanDir and read-only thereafter. Mutating a slice +// element in place (rather than replacing the slice) would reintroduce the +// aliasing bug for that field, so deep-copy here if that ever changes. func (pm *ProcessManager) broadcastStatus(info *AgentInfo) { snapshot := *info pm.broker.Broadcast(SSEEvent{Type: "agent_status", Data: &snapshot}) diff --git a/forge-ui/static/app.js b/forge-ui/static/app.js index 217ca5a7..f0f5bd78 100644 --- a/forge-ui/static/app.js +++ b/forge-ui/static/app.js @@ -3311,6 +3311,14 @@ function App() { // Flip to "stopping" on click. The server broadcasts this too, but // SSEBroker.Broadcast drops events for a full buffer, so the click // must not depend on the stream to feel responsive. + // + // Capture the prior status so a failure restores what was actually + // there. Hardcoding 'running' would mislabel an agent that was, say, + // already errored — the server's authoritative event reconciles it, + // but not before the card renders the wrong state. Read it from the + // rendered state rather than inside the updater, which is not + // guaranteed to have run by the time the request settles. + const prevStatus = agents.find(a => a.id === id)?.status; setAgents(prev => prev.map(a => a.id === id ? { ...a, status: 'stopping', error: '' } : a )); @@ -3318,13 +3326,13 @@ function App() { await stopAgent(id); } catch (err) { console.error('Failed to stop agent:', err); - // The server rolls its own state back; mirror that locally so the - // card doesn't stay stuck on "stopping" with disabled buttons. + // Mirror ProcessManager.Stop's rollback-to-previous so the card + // doesn't stay stuck on "stopping" with disabled buttons. setAgents(prev => prev.map(a => - a.id === id ? { ...a, status: 'running', error: err.message } : a + a.id === id ? { ...a, status: prevStatus || 'running', error: err.message } : a )); } - }, []); + }, [agents]); const handleRescan = useCallback(async () => { setLoading(true);