diff --git a/internal/api/agent.go b/internal/api/agent.go index 622d8c1..9b45989 100644 --- a/internal/api/agent.go +++ b/internal/api/agent.go @@ -26,6 +26,11 @@ import ( // by everything at once, which reads as a hang — and the whole design rests on // watching the canvas change as it happens. +// keepAliveInterval is how often a quiet stream emits a comment frame. Well +// under the 30–60s idle timeout typical of reverse proxies, which is the thing +// it exists to stay ahead of. +const keepAliveInterval = 20 * time.Second + // chatRequest is the body of POST /agent/chat. type chatRequest struct { GardenID int64 `json:"gardenId" binding:"required"` @@ -68,9 +73,11 @@ func (h *handlers) agentChat(c *gin.Context) { send := stream.send // A model thinking hard between tool calls sends nothing for a while, and an - // idle proxy will cut a quiet connection. A comment frame every 20s keeps it - // open; SSE ignores comments, so this costs the client nothing. - stopBeat := stream.keepAlive(20 * time.Second) + // idle proxy will cut a quiet connection. Deferred so a panic in the run + // can't leak the ticker goroutine; stopping it twice is harmless. + stopBeat := stream.keepAlive(keepAliveInterval) + defer stopBeat() + turn, err := h.agent.Run(c.Request.Context(), actor.ID, req.GardenID, req.Message, replayHistory(history), func(s mdagent.Step) { @@ -163,8 +170,11 @@ func (s *eventStream) keepAlive(every time.Duration) func() { } } }() + // Idempotent: the handler stops it explicitly when the run returns and again + // via defer, so a panic can't leak the goroutine. + var once sync.Once return func() { - close(done) + once.Do(func() { close(done) }) <-stopped } } diff --git a/internal/service/revisions.go b/internal/service/revisions.go index 273f469..de03ad7 100644 --- a/internal/service/revisions.go +++ b/internal/service/revisions.go @@ -151,16 +151,12 @@ func partialSummary(summary string) string { // set only by RevertChangeSet. A scope with no revisions writes nothing — an // operation that changed nothing doesn't belong in history. // -// The write is DETACHED FROM CANCELLATION, always. By the time it runs, the data -// changes it describes have already committed — so cancelling it cannot undo -// anything, it can only lose the record of what happened and leave real changes -// with no way to undo them. -// -// This was originally only done on the failure path, on the reasoning that a -// cancelled context is why fn failed. That missed the commoner case: an agent -// turn whose client disconnects can still COMPLETE, and then the success path -// commits with a dead context and loses the history anyway. Found in production -// with 18 plantings and no change set behind them. +// The write is DETACHED FROM CANCELLATION, always, and that belongs here rather +// than at each call site so no caller can be the one that forgets. By the time a +// commit runs, the data changes it describes have already been written — so +// cancelling it cannot undo anything. It can only lose the record of what +// happened and leave real changes with no way to undo them, which is the one +// thing the whole change-set design exists to prevent. func (s *Service) commitScope(ctx context.Context, sc *changeScope, revertsID *int64) (*domain.ChangeSet, error) { revs := sc.taken() if len(revs) == 0 { @@ -207,9 +203,13 @@ func (s *Service) record(ctx context.Context, gardenID, actorID int64, summary s sc.append(revs) return } - if _, err := s.store.WriteChangeSet(ctx, &domain.ChangeSet{ - GardenID: gardenID, ActorID: actorID, Source: domain.SourceUI, Summary: summary, - }, revs); err != nil { + // Auto-scope: one operation, its own change set. Written through the same + // detached path as everything else — a REST client that hangs up right after + // its PATCH landed must not leave that change without history, and this is + // the path virtually every mutation takes. + auto := &changeScope{gardenID: gardenID, actorID: actorID, source: domain.SourceUI, summary: summary} + auto.append(revs) + if _, err := s.commitScope(ctx, auto, nil); err != nil { slog.Error("service: record change set", "error", err, "garden", gardenID, "summary", summary) } } diff --git a/internal/service/revisions_test.go b/internal/service/revisions_test.go index 15234f9..f83c008 100644 --- a/internal/service/revisions_test.go +++ b/internal/service/revisions_test.go @@ -859,3 +859,37 @@ func TestSucceededTurnRecordsEvenIfTheCallerWentAway(t *testing.T) { t.Errorf("%d plantings survived the undo", len(active)) } } + +// TestAutoScopedMutationRecordsEvenIfTheCallerWentAway — the same rule for the +// path virtually every mutation takes. +// +// A plain REST PATCH auto-scopes into its own change set. If the client hangs up +// between the row landing and the change set being written, that change is +// orphaned exactly as an agent turn's was — and this path is used far more. +func TestAutoScopedMutationRecordsEvenIfTheCallerWentAway(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + bed := seedBed(t, s, owner, g.ID) + before := len(history(t, s, owner, g.ID)) + + // The row write and the history write share this context; cancelling after + // the mutation returns is the client hanging up mid-request. + ctx, cancel := context.WithCancel(context.Background()) + if _, err := s.UpdateObject(ctx, owner, bed.ID, ObjectPatch{XCM: f64Ptr(300)}, bed.Version); err != nil { + t.Fatalf("UpdateObject: %v", err) + } + cancel() + + after := history(t, s, owner, g.ID) + if len(after) != before+1 { + t.Fatalf("recorded %d change sets, want 1 — the move is otherwise un-undoable", len(after)-before) + } + if _, conflicts, err := s.RevertChangeSet(context.Background(), owner, after[0].ID, domain.SourceUI); err != nil || len(conflicts) != 0 { + t.Fatalf("the recorded move should be undoable: err=%v conflicts=%+v", err, conflicts) + } + back, _ := s.store.GetObject(context.Background(), bed.ID) + if back.XCM != bed.XCM { + t.Errorf("undo left x at %v, want %v", back.XCM, bed.XCM) + } +}