Skip to content

Commit e3b2865

Browse files
authored
fix(tui): stop cancel-then-resend from racing the daemon's session lock (#2912)
Closes #2901 Stacked on #2899 (base branch: `tmi/issue-2899-tui-stats`) — not `main`. ## Why this matters Cancelling a turn (Esc/Ctrl+C) used to free the composer immediately, letting a follow-up query go out on the same session before the daemon had actually finished the previous one — the daemon only releases its per-session lock when the sidecar's worker thread notices the cancel and unwinds (cooperative, checked once per agent-loop step), not when the client drops its connection. A fast enough retype could beat that release and land on a bare, confusing 409. Live testing on real hardware then found the fix itself had a worse problem: a **second** cancel-then-resend in the same session quit the app outright (3/3, clean exit, no panic). Root cause: a cancelled turn on the daemon transport never actually settles via `doneMsg` — it settles via its own terminal event (the server's cooperative cancel just produces an ordinary "stopped" answer), and that code path never rearms the listener `doneMsg` depends on. The turn's cancel bookkeeping was left stuck "pending" for the rest of the session, so the *next* Esc/Ctrl+C failed its own guard and fell through to quit. Fixed — settlement now happens on every real terminal signal for the live turn, not only the one that could never arrive. **Scope note:** live testing also measured input staying blocked ~18–25s per cancel while the server's cooperative cancellation finishes whatever step is already in flight. That is server-side latency, not something this client-side fix can shorten, so it is split out to #2917 with a recommended direction (queue the resend instead of blocking on it) rather than folded in here. When a conflict does still happen (a genuinely concurrent request), the client reports it with an actionable message naming the busy session, instead of the generic "check daemon status" copy meant for an unreachable daemon. The server-side 409 policy itself is intentional and untouched (`hub/agents/email/python/gaia_agent_email/query_routes.py`) — it is documented, tested behavior that prevents two turns from stepping on one session's agent state. This PR only closes the client-side windows that let a resend either race that lock or crash the app. ## Live evidence - **409s:** 0 of 8 cancel-then-resend attempts (down from 5/5 before this PR). - **Second-cycle crash:** 3/3 reproducible before this fix, closed by a failing-test-first regression (`TestSecondCancelThenResendDoesNotQuitTheApp`, `TestSecondCtrlCThenResendDoesNotQuitTheApp`) that fails against the pre-fix code and passes after. - **Input-blocking (~18–25s per cancel):** confirmed real, not fixable client-side alone — tracked in #2917. ## Test plan - [x] `cd tui && go test ./...` — all packages pass - [x] `go test -race ./internal/ui/chat/... ./internal/client/...` — clean - [x] New regression tests reproduce the live two-cycle crash end to end (cancel turn 1 → settle via its own terminal event → resend turn 2 → cancel again) and fail against the pre-fix code, pass after - [ ] Live-mailbox validation (cancel-then-resend, including the two-cycle case) — scheduled separately
1 parent b38f5e6 commit e3b2865

10 files changed

Lines changed: 1007 additions & 53 deletions

File tree

tui/internal/client/client.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,26 @@ type TranscriptResetter interface {
3636
ResetTranscript()
3737
}
3838

39+
// AgentCanceler is implemented by transports where the server, not this
40+
// client dropping its connection, decides when a cancelled run has actually
41+
// settled (#2901) — e.g. a daemon-relay session guarded by a server-side
42+
// lock that a worker thread releases on its own cooperative schedule.
43+
//
44+
// Cancel asks the server to stop the active run out of band. It deliberately
45+
// does NOT tear down the caller's own read of the run's event channel: that
46+
// read has to keep going until the channel closes on its own, because THAT
47+
// closure — not this call returning — is the one signal proven to follow the
48+
// server's cleanup. A transport with no such server-side lock (e.g. a local
49+
// subprocess) does not implement this; for it, tearing down the local
50+
// connection/process IS the whole cancellation, and the caller's own
51+
// context.CancelFunc already does that.
52+
type AgentCanceler interface {
53+
// Cancel asks the server to stop the currently active run. It returns an
54+
// actionable error if the request could not be delivered; a run that has
55+
// already ended is not an error (there is nothing left to cancel).
56+
Cancel(ctx context.Context) error
57+
}
58+
3959
// AgentConfirmer is implemented by transports that can resolve a
4060
// needs_confirmation pause under the resume model (spec §5: the event carries
4161
// a non-empty confirm_url and the run stays paused server-side awaiting it).

tui/internal/client/sse.go

Lines changed: 82 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,8 @@ func (s *SSEClient) Send(ctx context.Context, query string) (<-chan interface{},
236236
status := resp.StatusCode
237237
resp.Body.Close()
238238
cancel()
239-
if status == http.StatusNotFound {
239+
switch status {
240+
case http.StatusNotFound:
240241
// A 404 on the query path is specifically "this route does not exist
241242
// there" — most often a sidecar predating the canonical /query
242243
// endpoint. Saying so beats making the user decode a bare 404.
@@ -246,10 +247,22 @@ func (s *SSEClient) Send(ctx context.Context, query string) (<-chan interface{},
246247
"`gaia daemon agents` and reinstall/update the agent — or no agent with "+
247248
"that id is registered with the daemon",
248249
s.agentID, detail)
250+
case http.StatusConflict:
251+
// The session's run_lock is still held by a previous turn (#2901) —
252+
// most often the tail of a just-cancelled one the daemon has not
253+
// finished unwinding yet. Name the busy resource, like Respond and
254+
// Confirm already do for their own 409s, instead of the generic
255+
// relay-refused copy below (which points at a daemon that is down,
256+
// not one that is merely still finishing).
257+
return nil, fmt.Errorf(
258+
"the '%s' agent is still finishing the previous turn on this session (%s). "+
259+
"Wait a moment and try again",
260+
s.agentID, detail)
261+
default:
262+
return nil, fmt.Errorf(
263+
"the daemon relay refused the '%s' query (%s). Check `gaia daemon status`",
264+
s.agentID, detail)
249265
}
250-
return nil, fmt.Errorf(
251-
"the daemon relay refused the '%s' query (%s). Check `gaia daemon status`",
252-
s.agentID, detail)
253266
}
254267

255268
handle := &runHandle{runID: runID, cancel: cancel}
@@ -588,6 +601,21 @@ type confirmRequest struct {
588601
Approved bool `json:"approved"`
589602
}
590603

604+
// postCancel POSTs /v1/<agent>/query/{runID}/cancel and returns the response,
605+
// or an error if the request itself could not be delivered. Shared by
606+
// cancelRun (best-effort, fire-and-forget) and Cancel (the caller-facing,
607+
// error-reporting seam — #2901).
608+
func (s *SSEClient) postCancel(ctx context.Context, inst *daemon.Instance, runID string) (*http.Response, error) {
609+
resp, _, err := s.daemon.Do(ctx, inst, daemon.Request{
610+
Method: http.MethodPost,
611+
Path: fmt.Sprintf("/v1/%s/query/%s/cancel",
612+
url.PathEscape(s.agentID), url.PathEscape(runID)),
613+
HTTPClient: s.cancelHTTP,
614+
Op: fmt.Sprintf("cancel the '%s' run", s.agentID),
615+
})
616+
return resp, err
617+
}
618+
591619
// cancelRun asks the relay to drop a run we are abandoning.
592620
//
593621
// Best-effort: the sidecar may already be gone. It owns its own background
@@ -605,13 +633,7 @@ func (s *SSEClient) cancelRun(handle *runHandle) {
605633
ctx, cancel := context.WithTimeout(context.Background(), cancelTimeout)
606634
defer cancel()
607635

608-
resp, _, err := s.daemon.Do(ctx, inst, daemon.Request{
609-
Method: http.MethodPost,
610-
Path: fmt.Sprintf("/v1/%s/query/%s/cancel",
611-
url.PathEscape(s.agentID), url.PathEscape(handle.runID)),
612-
HTTPClient: s.cancelHTTP,
613-
Op: fmt.Sprintf("cancel the '%s' run", s.agentID),
614-
})
636+
resp, err := s.postCancel(ctx, inst, handle.runID)
615637
if err != nil {
616638
s.opts.Logf("sse: best-effort cancel for '%s' run_id=%s failed: %v",
617639
s.agentID, handle.runID, err)
@@ -632,6 +654,54 @@ func (s *SSEClient) cancelRun(handle *runHandle) {
632654
}
633655
}
634656

657+
// Cancel implements client.AgentCanceler (#2901). It asks the server to stop
658+
// the active run WITHOUT touching this client's own read of the run's SSE
659+
// stream — unlike the caller's context.CancelFunc, which tears that read
660+
// down immediately and is exactly what let a resend beat the daemon's
661+
// session run_lock (cooperative cancellation, released by the sidecar's
662+
// worker thread's own `finally`, checked once per agent-loop step; see
663+
// hub/agents/email/python/gaia_agent_email/query_routes.py).
664+
//
665+
// Leaving the read running is what makes the eventual terminal signal a
666+
// near-certain settlement signal in practice — not a proven one. The
667+
// sidecar's `finally` runs signal_done() and only THEN run_lock.release(),
668+
// same thread, no I/O between the two statements, so by the time anything
669+
// downstream notices signal_done the lock is released in all but a
670+
// vanishingly rare preemption between those two lines (#2912 review). A
671+
// client that aborts its own read instead observes a "done" that is
672+
// guaranteed to race ahead of the release, which is the bug this method
673+
// exists to avoid reintroducing. Closing that last window for good is a
674+
// one-line server-side reorder (release, then signal_done) — tracked
675+
// separately, not required here since this is a Go-only change.
676+
func (s *SSEClient) Cancel(ctx context.Context) error {
677+
s.mu.Lock()
678+
inst := s.inst
679+
active := s.active
680+
s.mu.Unlock()
681+
if inst == nil || active == nil {
682+
// Nothing live to cancel — most often the run already reached its own
683+
// terminal event between the keypress and this call. Not an error: the
684+
// caller's read will observe that completion on its own.
685+
return nil
686+
}
687+
688+
resp, err := s.postCancel(ctx, inst, active.runID)
689+
if err != nil {
690+
return fmt.Errorf("could not deliver the cancel request for the '%s' agent: %w", s.agentID, err)
691+
}
692+
defer resp.Body.Close()
693+
694+
switch resp.StatusCode {
695+
case http.StatusOK, http.StatusNotFound:
696+
// 404 means the run had already ended by the time this landed — the
697+
// caller's read will see that completion on its own; nothing failed.
698+
return nil
699+
default:
700+
return fmt.Errorf("cancelling the '%s' run failed (%s)",
701+
s.agentID, daemon.ErrorDetail(resp))
702+
}
703+
}
704+
635705
func (s *SSEClient) clearActive(handle *runHandle) {
636706
s.mu.Lock()
637707
if s.active == handle {
@@ -782,5 +852,6 @@ var (
782852
_ AgentClient = (*SSEClient)(nil)
783853
_ AgentResponder = (*SSEClient)(nil)
784854
_ AgentConfirmer = (*SSEClient)(nil)
855+
_ AgentCanceler = (*SSEClient)(nil)
785856
_ TranscriptResetter = (*SSEClient)(nil)
786857
)

0 commit comments

Comments
 (0)