diff --git a/CHANGELOG.md b/CHANGELOG.md index b6ba7c3c..1c8dd586 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **A restarted session no longer reads as busy forever.** A session whose + agent died mid-turn, or that was closed mid-turn, came back under the same + card still listed as busy to other sessions: a task sent to it waited for a + turn that would never end. It now starts with no state, like a new one. - **Codex, Cursor CLI, Antigravity and Crush stop asking where you already answered.** A sandboxed session of any of them opens straight at its prompt. Cursor CLI and Antigravity no longer ask again in a worktree of a diff --git a/internal/relay/observe.go b/internal/relay/observe.go index bad02269..0fb99090 100644 --- a/internal/relay/observe.go +++ b/internal/relay/observe.go @@ -147,6 +147,25 @@ func (s *Service) recordState(sessionID, state string) { } +// SessionExited drops what the session's last process reported, once lich +// has reaped it: a process that dies mid-turn never sends the SessionEnd +// (idle) that would, so its busy outlived it, and the same id respawned read +// as busy to every delivery and in every roster until its own process ended a +// turn. The terminal calls it for a process that ended on its own +// (terminal.SetSessionExited); a close goes through SessionClosed. +func (s *Service) SessionExited(sessionID string) { + s.mu.Lock() + s.forgetStateLocked(sessionID) + s.mu.Unlock() +} + +// forgetStateLocked removes a session from the turn map and the roster, as an +// idle report does. Called with s.mu held. +func (s *Service) forgetStateLocked(sessionID string) { + delete(s.state, sessionID) + delete(s.reported, sessionID) +} + // endedErrands takes every ticket the report ends off the table and closes its // stall channel, for Observe to announce outside the lock. Called with s.mu // held. diff --git a/internal/relay/relay_test.go b/internal/relay/relay_test.go index b2bd8d2c..364b102d 100644 --- a/internal/relay/relay_test.go +++ b/internal/relay/relay_test.go @@ -3060,3 +3060,33 @@ func TestSelfRefusesACallerWithoutASession(t *testing.T) { } } } + +// A session's process can die mid-turn, and a dead process never reports the +// SessionEnd (idle) that would clear its busy. Once lich has reaped it, or the +// card was closed, the same id respawned is a new process that has reported +// nothing: it must not read busy to deliveries or in the roster. +func TestAReapedOrClosedSessionForgetsItsState(t *testing.T) { + svc := newRelay(workspace(), newFakeTerminal("s1", "s2", "s3"), nil) + svc.Observe("s2", "busy") + svc.Observe("s3", "busy") + + svc.SessionExited("s2") + svc.SessionClosed("s3", "") + + peers, err := svc.Peers(PeersOptions{From: "s1"}) + if err != nil { + t.Fatalf("Peers: %v", err) + } + for _, p := range peers { + if p.State != "" { + t.Errorf("%s state = %q after its process went, want none", p.Name, p.State) + } + } + svc.mu.Lock() + defer svc.mu.Unlock() + for _, id := range []string{"s2", "s3"} { + if turn, ok := svc.state[id]; ok { + t.Errorf("turn state of %s = %q after its process went, want none", id, turn) + } + } +} diff --git a/internal/relay/workeranswer.go b/internal/relay/workeranswer.go index 6662d0c6..00987207 100644 --- a/internal/relay/workeranswer.go +++ b/internal/relay/workeranswer.go @@ -226,12 +226,14 @@ func (s *Service) errandOpenAtLocked(sessionID string) bool { // (WorkerAnswered). The terminal calls this before the process dies // (terminal.SetSessionClosed), so the SessionEnd its CLI reports on the way out // finds nothing to call unanswered. Errands of any other kind end as they -// always did. +// always did. The session's reported state goes too: the same id reopened is +// a new process that has reported nothing yet. func (s *Service) SessionClosed(sessionID, closerID string) { var stopped []*ticket var told []string s.mu.Lock() delete(s.reportedWorkers, sessionID) + s.forgetStateLocked(sessionID) for id, t := range s.tickets { if t.targetID != sessionID || !t.subagent { continue diff --git a/internal/terminal/exit_watch_test.go b/internal/terminal/exit_watch_test.go new file mode 100644 index 00000000..8b34b905 --- /dev/null +++ b/internal/terminal/exit_watch_test.go @@ -0,0 +1,50 @@ +// Drives a real PTY, so Unix-only like the rest of the service's spawn tests +// (terminal_test.go). +//go:build !windows + +package terminal + +import ( + "slices" + "sync" + "testing" +) + +// A process that ends on its own is reported to the exit watcher once it is +// reaped, which is how the relay learns to drop the state that process left +// (relay.SessionExited). A close is not: it reaches the relay through the +// close watcher instead. +func TestAReapedSessionIsReportedToTheExitWatcher(t *testing.T) { + hub, _ := newProbeHub(t) + svc := New(stubBins{bin: exitBin(t, 0)}, nil, hub) + var mu sync.Mutex + var exited []string + svc.SetSessionExited(func(id string) { + mu.Lock() + defer mu.Unlock() + exited = append(exited, id) + }) + t.Cleanup(func() { _ = svc.Close("s1") }) + + if err := svc.Start("s1", "p1", t.TempDir(), "claude", "", "", false, false, 80, 24); err != nil { + t.Fatalf("start: %v", err) + } + waitFor(t, func() bool { + mu.Lock() + defer mu.Unlock() + return slices.Contains(exited, "s1") + }, "the exit watcher to hear s1") + + svc2 := New(stubBins{bin: echoBin(t)}, nil, hub) + closed := false + svc2.SetSessionExited(func(string) { closed = true }) + if err := svc2.Start("s2", "p1", t.TempDir(), "claude", "", "", false, false, 80, 24); err != nil { + t.Fatalf("start: %v", err) + } + if err := svc2.Close("s2"); err != nil { + t.Fatalf("close: %v", err) + } + if closed { + t.Error("a closed session was reported to the exit watcher") + } +} diff --git a/internal/terminal/io.go b/internal/terminal/io.go index 72ffc2c0..3b0ff3a8 100644 --- a/internal/terminal/io.go +++ b/internal/terminal/io.go @@ -48,6 +48,7 @@ func (s *Service) stream(id string, sess *session) { s.mu.Lock() reaped := false + exited := s.onExited if current, ok := s.sessions[id]; ok && current.pty == p { delete(s.sessions, id) close(current.done) @@ -79,6 +80,11 @@ func (s *Service) stream(id string, sess *session) { // Emit blocks on a stalled /events client, and holding s.mu across it would // freeze every session's I/O. if reaped { + // Outside mu: the relay takes its own lock, and it calls into this + // service under it. + if exited != nil { + exited(id) + } s.hub.Emit(exitEventPrefix+id, exitPayload(code)) } } diff --git a/internal/terminal/terminal.go b/internal/terminal/terminal.go index 3b94a70f..1cdff81f 100644 --- a/internal/terminal/terminal.go +++ b/internal/terminal/terminal.go @@ -311,6 +311,10 @@ type Service struct { // errand there, ahead of the SessionEnd the dying CLI reports. Guarded by mu // like onState. onClosed func(id, closerID string) + // onExited, when set, is told about every session whose process ended on + // its own and was reaped: the relay drops the state that process reported. + // Guarded by mu like onState. + onExited func(id string) // onLimit, when set, is told about every turn a usage limit ended that names // its reset (limit.go). Guarded by mu like onState. onLimit func(id string, resetsAt int64) @@ -738,6 +742,15 @@ func (s *Service) SetSessionClosed(fn func(id, closerID string)) { s.onClosed = fn } +// SetSessionExited wires fn to every session whose process ended on its own, +// called once lich has reaped it. Only one watcher, the relay +// (relay.SessionExited). +func (s *Service) SetSessionExited(fn func(id string)) { + s.mu.Lock() + defer s.mu.Unlock() + s.onExited = fn +} + // stateWatcher reads the watcher under the lock, so a report arriving while it // is being wired sees one function or none, never a torn read. func (s *Service) stateWatcher() func(id, state string) { diff --git a/main.go b/main.go index 0ca06789..c4e700c1 100644 --- a/main.go +++ b/main.go @@ -442,6 +442,7 @@ func registerServices(db *store.Service, term *terminal.Service, hub *events.Hub term.SetWorkerAnswer(rl.WorkerAnswered) term.SetWorkerUnanswered(rl.WorkerUnanswered) term.SetSessionClosed(rl.SessionClosed) + term.SetSessionExited(rl.SessionExited) term.SetErrandStatus(rl.Status) // Both questions the relay asks about a provider — whether its sessions // report at all, and whether they can answer with a tool — are about what @@ -516,7 +517,9 @@ func registerServices(db *store.Service, term *terminal.Service, hub *events.Hub // relay.WorkerUnanswered arrives the same way and ends one unanswered. // - relay.SessionClosed is what the terminal tells the relay as it closes a // session: called here it ends a running worker's errand, in silence when -// the closer named is the worker's caller. +// the closer named is the worker's caller. relay.SessionExited is what it +// tells the relay as it reaps a process that ended on its own: called here +// it erases a live session's reported state. // - terminal.CloseBy is a close on behalf of a session, which `lich close` // reaches through spawn.Close: called here with the worker's caller as the // closer, it ends that caller's errand without telling it. The window @@ -547,7 +550,7 @@ func registerServices(db *store.Service, term *terminal.Service, hub *events.Hub // store.SetBranchOf, store.SetTranscriptOf, store.SetConversationsOf, // store.SetCheckoutsOf, terminal.SetDropDir, // terminal.SetRateLimitReports, terminal.SetUsageLimit, terminal.SetWorkerAnswer, -// terminal.SetWorkerUnanswered, terminal.SetStartReports, terminal.SetPromptLanguage and terminal.SetSessionClosed are startup wiring. Called with [null] they silently +// terminal.SetWorkerUnanswered, terminal.SetStartReports, terminal.SetPromptLanguage, terminal.SetSessionClosed and terminal.SetSessionExited are startup wiring. Called with [null] they silently // nil what they wired (encoding/json leaves a func or pointer alone on // null), and the write races the readers already serving — nilling // SetProjects also disarms the guard that keeps two projects off the same @@ -600,6 +603,7 @@ func denyInternal(d *rpc.Handler) { "relay.WorkerAnswered", "relay.WorkerUnanswered", "relay.SessionClosed", + "relay.SessionExited", "terminal.CloseBy", "relay.RunSchedules", "relay.ParkResume", @@ -622,6 +626,7 @@ func denyInternal(d *rpc.Handler) { "terminal.SetWorkerAnswer", "terminal.SetWorkerUnanswered", "terminal.SetSessionClosed", + "terminal.SetSessionExited", "terminal.SetStartReports", "terminal.SetPromptLanguage", "terminal.EnqueueModCommand",