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
 	}
 }

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)
 	}
 }

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, "[email protected]")
+	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)
+	}
+}