Author SHA1 Message Date
steveandClaude Opus 4.8 95b9d611c6 Test the deadline is refreshed per-frame, not just extended (#87)
Build image / build-and-push (push) Successful in 10s
Gadfly re-review: the regression test proved the stream outlives the server
WriteTimeout, but its whole run was under a second — far below the 30s
sseWriteTimeout — so it could NOT tell a per-frame refresh from a deadline set
once at open. A revert to set-once would reintroduce the unbounded-block risk
the per-frame refresh exists to prevent, and sail through the test.

Make sseWriteTimeout a var (production never reassigns it) so a test can shrink
it, and split into two focused cases sharing a streamFrames helper:

- TestEventStreamOutlivesServerWriteTimeout — server WriteTimeout 300ms, frames
  straddling it, sseWriteTimeout left at 30s. Catches #78 (no deadline
  management → server cuts the stream). Huge margin, so CI slowness only makes
  it pass more surely — addresses the "timing margin tight" finding too.
- TestEventStreamRefreshesDeadlinePerFrame — sseWriteTimeout shrunk to 400ms,
  8 frames at a 100ms tick (800ms total, 4× margin per gap). A per-frame refresh
  delivers all 8; a set-once deadline expires mid-stream and cuts it.

Verified each fails against its own regression: removing the per-write refresh
gives 3/8 on the per-frame test (EOF at ~400ms); removing both deadline calls
gives 0/3 on the outlives test. Both pass on the real code, 3× under -count.

The "clear the deadline entirely" finding was already addressed by the previous
commit (per-frame refresh); this covers it against reintroduction.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
2026-07-21 20:49:57 -04:00
steveandClaude Opus 4.8 04cdf815df Bound each SSE write instead of removing the deadline entirely
Build image / build-and-push (push) Successful in 8s
Gadfly, security lens: clearing the write deadline outright traded the
truncation bug for an unbounded one. With no deadline, a client that stops
reading fills the socket buffer and blocks Write forever — pinning the agent
run goroutine and this stream's mutex, which also takes down the keep-alive
since it needs the same lock. The old 30s WriteTimeout at least bounded that.

Refresh the deadline per frame instead: the stream as a whole is unbounded,
but no single write is. That is the only shape that satisfies both ends, since
an absolute deadline cuts long turns and no deadline cannot be recovered from.

The probe stays in openEventStream so a writer that cannot take deadlines is
reported once rather than once per frame; the per-write call deliberately
ignores its error for the same reason.

Also widened the test's timing margins, per the second finding. tick is now
GREATER than writeTimeout, so every frame lands after the deadline has already
expired and the margin only ever grows — a loaded CI runner pushes this toward
passing rather than toward flaking. Sized the other way it would flake exactly
when CI is busiest. Passes 3/3 with -count=3.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
2026-07-21 18:23:31 -04:00
steveandClaude Opus 4.8 7088f382bc Clear the SSE write deadline so a turn can outlive WriteTimeout (#78)
Build image / build-and-push (push) Successful in 5s
Gadfly review (reusable) / review (pull_request) Canceled after 9m11s
Adversarial Review (Gadfly) / review (pull_request) Canceled after 9m11s
http.Server.WriteTimeout is an ABSOLUTE deadline from when the request header
was read, not an idle timeout. pansy sets it to 30s (cmd/pansy/main.go:65)
while an agent turn is budgeted 4 minutes, so every turn over 30 seconds was
cut mid-stream. The 4-minute runTimeout was unreachable; the real ceiling was
30 seconds.

That also meant the keep-alive added in #73 could never do its job. It ticks
at 20s, so it got exactly one tick before the connection it was pacing was
destroyed underneath it — a mechanism built to survive long silences that was
structurally incapable of surviving them.

The failure is invisible from the handler: writes made after the deadline
return err == nil and their bytes are discarded. There is no error to check
and none to log, which is why this survived a review that specifically looked
at the discarded write error. Only the client sees it, as an unexpected EOF,
which web/src/lib/agent.ts reports as "The connection dropped partway
through." — indistinguishable from a real network fault.

Because it is invisible server-side, the test drives a real http.Server with a
short WriteTimeout and asserts from the CLIENT side. Against the unfixed code
it gets 1 of 4 frames and an unexpected EOF; a test that inspected the return
of io.WriteString would have passed against the bug.

gin 1.10.1's responseWriter implements Unwrap, so ResponseController reaches
the underlying conn. A failure to clear the deadline IS worth logging: it
means the stream will be cut and we cannot prevent it.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
2026-07-21 18:14:12 -04:00
9 changed files with 169 additions and 379 deletions
-4
View File
@@ -64,8 +64,6 @@ POST /change-sets/:id/revert ← undo an operation; 201, or 409 + the conflicts
POST /gardens/:id/copy ← deep-copy a garden you own (objects + active plops; not shares/link)
POST /gardens/:id/objects PATCH,DELETE /objects/:id
POST /objects/:id/plantings PATCH,DELETE /plantings/:id
POST /objects/:id/fill ← hex-pack a region with one plant; region by compass name or rect
POST /objects/:id/clear ← soft-remove every active plop, as ONE change set
GET,POST /plants PATCH,DELETE /plants/:id (own plants only)
GET,POST /seed-lots GET,PATCH,DELETE /seed-lots/:id (own lots only; private)
GET,POST /gardens/:id/journal PATCH,DELETE /journal/:id (editor writes; author edits own)
@@ -74,8 +72,6 @@ POST /agent/chat ← SSE: step events, then the finished turn (edit
GET,DELETE /gardens/:id/agent/history (the actor's own thread)
GET /capabilities ← what this instance can do, so the UI offers only what works
GET,POST /gardens/:id/shares PATCH,DELETE /gardens/:id/shares/:userId (invite by email)
GET,POST,DELETE /gardens/:id/share-link ← the public read-only token for this garden
GET /public/gardens/:token ← UNAUTHENTICATED read-only /full; the token is the capability
```
**Sync:** plain REST + optimistic UI + last-write-wins with a version guard. Every PATCH/DELETE carries the row's `version`; the server increments on write and returns **409 + the current row** on mismatch; the client rolls back and refetches. No websockets/CRDT — the right cost for household-scale co-editing. Drags PATCH once on drop, not per frame.
+44 -1
View File
@@ -108,6 +108,21 @@ func (h *handlers) agentChat(c *gin.Context) {
send(chatEvent{Done: turn})
}
// sseWriteTimeout bounds ONE write to the stream, not the stream itself.
//
// It is refreshed per frame, which is the only shape that satisfies both ends:
// the server's absolute WriteTimeout would cut a long turn (#78), while removing
// the deadline entirely would let a client that stops reading block a write
// forever once the socket buffer fills — pinning the run goroutine and this
// stream's mutex with it, and taking the keep-alive down too since it needs the
// same lock. Generous, because it is a backstop against a stuck peer and not a
// pacing mechanism.
//
// A var, not a const, ONLY so the test can shrink it to prove the deadline is
// refreshed per frame rather than set once — a set-once 30s deadline would pass
// a test whose whole run is under a second. Production never reassigns it.
var sseWriteTimeout = 30 * time.Second
// eventStream serializes writes to one SSE response.
//
// The mutex is load-bearing, not decoration: step events are sent from the
@@ -116,6 +131,7 @@ func (h *handlers) agentChat(c *gin.Context) {
// frames long before it crashes anything.
type eventStream struct {
c *gin.Context
rc *http.ResponseController
mu sync.Mutex
}
@@ -124,12 +140,34 @@ type eventStream struct {
// Headers go out before the first write and the stream is flushed immediately,
// so a proxy holding the response until it looks complete can't reintroduce
// exactly the silence streaming exists to remove.
//
// Taking the write deadline off the server's absolute WriteTimeout and onto a
// per-write one is what makes a turn longer than 30s possible at all (#78).
// WriteTimeout is an ABSOLUTE deadline from when the request header was read,
// not an idle timeout, so a streaming response is cut mid-turn however recently
// it wrote. Without this the 4-minute runTimeout is unreachable and the
// keep-alive below tops out at one tick — pacing a connection that is destroyed
// underneath it.
//
// That failure is INVISIBLE from in here: writes past the deadline return
// err == nil and their bytes are dropped, so there is nothing to detect on the
// write path. Only the client sees it, as a truncated stream it reports as a
// dropped connection. Hence a deadline set up front and refreshed per frame,
// rather than anything checked after the fact.
func openEventStream(c *gin.Context) *eventStream {
c.Header("Content-Type", "text/event-stream")
c.Header("Cache-Control", "no-cache")
c.Header("X-Accel-Buffering", "no")
s := &eventStream{c: c, rc: http.NewResponseController(c.Writer)}
// Probe once here rather than reporting per frame: a writer that can't take
// deadlines will fail identically on every write, and the operator needs to
// hear it once. If this fails the stream still works — it is just back to
// being cut at WriteTimeout, which is worth saying out loud.
if err := s.rc.SetWriteDeadline(time.Now().Add(sseWriteTimeout)); err != nil {
slog.Error("api: SSE write deadlines unavailable; long turns will be truncated at the server WriteTimeout", "error", err)
}
c.Writer.Flush()
return &eventStream{c: c}
return s
}
func (s *eventStream) send(ev chatEvent) {
@@ -144,6 +182,11 @@ func (s *eventStream) send(ev chatEvent) {
func (s *eventStream) write(frame string) {
s.mu.Lock()
defer s.mu.Unlock()
// Refresh for THIS write, so the stream as a whole is unbounded but no single
// write is. Error deliberately unchecked: openEventStream already reported
// whether deadlines work at all, and this call can only fail the same way, so
// checking here would log once per frame to say the same thing.
_ = s.rc.SetWriteDeadline(time.Now().Add(sseWriteTimeout))
_, _ = io.WriteString(s.c.Writer, frame)
s.c.Writer.Flush()
}
-5
View File
@@ -119,11 +119,6 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine {
objects.PATCH("/:id", h.updateObject)
objects.DELETE("/:id", h.deleteObject)
objects.POST("/:id/plantings", h.createPlanting) // place a plop in this object
// Bulk ops. These wrap the same service methods the agent tools call, so an
// instance with no model configured still gets the most valuable operation in
// the app — and so "clear bed" is ONE change set rather than one per plop.
objects.POST("/:id/fill", h.fillObject)
objects.POST("/:id/clear", h.clearObject)
// Plantings ("plops") are addressed by their own id; the service resolves the
// owning object/garden for the permission check.
-116
View File
@@ -1,116 +0,0 @@
package api
import (
"net/http"
"github.com/gin-gonic/gin"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/service"
)
// Bulk operations on a plantable object (#82): fill a region with one plant, and
// clear everything out of it.
//
// These were reachable only through the agent toolbox until now, which meant the
// most valuable bulk operation in a garden planner — and the one carrying the
// most carefully reasoned geometry in the codebase — did not exist at all on an
// instance with no model configured. They are thin adapters over the same
// service methods `internal/agent/tools.go` calls, so the permission checks and
// the one-change-set-per-operation guarantee come along unchanged.
// fillRect is an explicit rectangle in the object's local frame, the alternative
// to a compass name. A named type (not an inline anonymous struct) to match the
// rest of internal/api and so it can carry its own validity check.
type fillRect struct {
MinX float64 `json:"minXCm"`
MinY float64 `json:"minYCm"`
MaxX float64 `json:"maxXCm"`
MaxY float64 `json:"maxYCm"`
}
// degenerate reports whether the rect encloses no area. Such a rect (including
// the all-zeros an empty `"rect": {}` decodes to) would otherwise slip through
// and plant a single plop at the object's centre — a surprising result for what
// is really malformed input.
func (r fillRect) degenerate() bool {
return r.MaxX <= r.MinX || r.MaxY <= r.MinY
}
// objectFillRequest is the body for POST /objects/:id/fill.
//
// A region is given EITHER by compass name ("ne", "south half", "all") or as an
// explicit rect in the object's local frame. The named form is what a person
// means and what the agent uses; the rect is for a future drag-a-box affordance.
// Exactly one must be supplied — accepting both and silently preferring one
// would make a client bug look like a geometry bug.
type objectFillRequest struct {
PlantID int64 `json:"plantId" binding:"required"`
Region string `json:"region"`
Rect *fillRect `json:"rect"`
// SpacingOverrideCM plants tighter or looser than the plant's mature spacing
// without editing the catalog entry.
SpacingOverrideCM *float64 `json:"spacingOverrideCm"`
}
func (h *handlers) fillObject(c *gin.Context) {
id, ok := parseIDParam(c, "id")
if !ok {
return
}
var req objectFillRequest
if err := c.ShouldBindJSON(&req); err != nil {
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "a plantId and a region are required")
return
}
named, hasRect := req.Region != "", req.Rect != nil
if named == hasRect {
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT",
`supply exactly one of "region" (e.g. "all", "ne", "south half") or "rect"`)
return
}
actor := mustActor(c).ID
var (
created []domain.Planting
err error
)
if rect := req.Rect; rect != nil {
// Reject a zero-area rect here rather than let it plant one stray plop.
// (Binding the pointer to `rect` also keeps the deref visibly guarded,
// instead of reading req.Rect.MinX under an invariant from a line above.)
if rect.degenerate() {
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "rect must enclose a positive area")
return
}
region := service.Region{MinX: rect.MinX, MinY: rect.MinY, MaxX: rect.MaxX, MaxY: rect.MaxY}
created, err = h.svc.FillRegion(c.Request.Context(), actor, id, region, req.PlantID, req.SpacingOverrideCM)
} else {
created, err = h.svc.FillNamedRegion(c.Request.Context(), actor, id, req.Region, req.PlantID, req.SpacingOverrideCM)
}
if err != nil {
writeServiceError(c, err)
return
}
// 200, not 201: a fill can legitimately create nothing (the region is already
// planted), and there is no single resource to point a Location at.
c.JSON(http.StatusOK, gin.H{"plantings": created, "created": len(created)})
}
// clearObject soft-removes every active plop in an object.
//
// Distinct from deleting the object, and — unlike the client-side loop this
// replaces — it lands as ONE change set, so undoing a cleared bed is one click
// rather than one per plop.
func (h *handlers) clearObject(c *gin.Context) {
id, ok := parseIDParam(c, "id")
if !ok {
return
}
n, err := h.svc.ClearObject(c.Request.Context(), mustActor(c).ID, id)
if err != nil {
writeServiceError(c, err)
return
}
c.JSON(http.StatusOK, gin.H{"cleared": n})
}
-226
View File
@@ -1,226 +0,0 @@
package api
import (
"net/http"
"testing"
"github.com/gin-gonic/gin"
)
func fillPath(id int64) string { return objectPath(id) + "/fill" }
func clearPath(id int64) string { return objectPath(id) + "/clear" }
// makeFillPlant creates a custom plant and returns its id. (A near-identical
// createPlantAPI landed alongside the seed-lot tests; consolidating the two into
// one shared helper is a fine follow-up, kept separate here only to avoid a
// merge collision on the shared symbol.)
func makeFillPlant(t *testing.T, r *gin.Engine, cookie *http.Cookie, name string, spacing float64) int64 {
t.Helper()
w := doJSON(t, r, http.MethodPost, "/api/v1/plants", map[string]any{
"name": name, "category": "vegetable", "spacingCm": spacing, "color": "#4a7c3f", "icon": "🌱",
}, cookie)
if w.Code != http.StatusCreated {
t.Fatalf("create plant %q: status %d, body %s", name, w.Code, w.Body.String())
}
return int64(decodeMap(t, w.Body.Bytes())["id"].(float64))
}
// seedFillableBed makes a garden with one plantable bed and a custom plant,
// returning (gardenID, objectID, plantID).
func seedFillableBed(t *testing.T, r *gin.Engine, cookie *http.Cookie, w, h, spacing float64) (int64, int64, int64) {
t.Helper()
gid := createGardenAPI(t, r, cookie, "G")
rec := doJSON(t, r, http.MethodPost, objectsPath(gid), map[string]any{
"kind": "bed", "widthCm": w, "heightCm": h, "plantable": true,
}, cookie)
if rec.Code != http.StatusCreated {
t.Fatalf("create bed: status %d, body %s", rec.Code, rec.Body.String())
}
objID := int64(decodeMap(t, rec.Body.Bytes())["id"].(float64))
plantID := makeFillPlant(t, r, cookie, "Fillable", spacing)
return gid, objID, plantID
}
// countChangeSets reads the history page and reports how many change sets exist.
func countChangeSets(t *testing.T, r *gin.Engine, cookie *http.Cookie, gardenID int64) int {
t.Helper()
w := doJSON(t, r, http.MethodGet, historyPath(gardenID), nil, cookie)
if w.Code != http.StatusOK {
t.Fatalf("history: status %d, body %s", w.Code, w.Body.String())
}
sets, _ := decodeMap(t, w.Body.Bytes())["changeSets"].([]any)
return len(sets)
}
// TestFillAndClearAPI covers the two routes end to end through the router.
//
// These exist because both operations were previously reachable ONLY through the
// agent toolbox, so on an instance with no model configured the most valuable
// bulk operation in the app did not exist at all.
func TestFillAndClearAPI(t *testing.T) {
r := authEngine(t, localCfg())
cookie := registerAndCookie(t, r, "[email protected]")
_, objID, plantID := seedFillableBed(t, r, cookie, 200, 200, 20)
// Fill by compass name.
w := doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{
"plantId": plantID, "region": "all",
}, cookie)
if w.Code != http.StatusOK {
t.Fatalf("fill: status %d, body %s", w.Code, w.Body.String())
}
body := decodeMap(t, w.Body.Bytes())
created := int(body["created"].(float64))
if created == 0 {
t.Fatalf("fill created nothing: %s", w.Body.String())
}
if plops, _ := body["plantings"].([]any); len(plops) != created {
t.Errorf("created=%d but returned %d plantings", created, len(plops))
}
// Clear it: one call, and it reports what it removed.
w = doJSON(t, r, http.MethodPost, clearPath(objID), nil, cookie)
if w.Code != http.StatusOK {
t.Fatalf("clear: status %d, body %s", w.Code, w.Body.String())
}
if n := int(decodeMap(t, w.Body.Bytes())["cleared"].(float64)); n != created {
t.Errorf("cleared %d, want %d (everything the fill made)", n, created)
}
// Clearing an already-empty bed is a no-op, not an error.
w = doJSON(t, r, http.MethodPost, clearPath(objID), nil, cookie)
if w.Code != http.StatusOK {
t.Fatalf("second clear: status %d", w.Code)
}
if n := int(decodeMap(t, w.Body.Bytes())["cleared"].(float64)); n != 0 {
t.Errorf("second clear removed %d, want 0", n)
}
}
// TestFillRegionSelectionAPI: exactly one of region/rect, and a rect fills only
// its own corner of the bed.
func TestFillRegionSelectionAPI(t *testing.T) {
r := authEngine(t, localCfg())
cookie := registerAndCookie(t, r, "[email protected]")
_, objID, plantID := seedFillableBed(t, r, cookie, 400, 400, 20)
// Neither → 400. Both → 400. Accepting both and silently preferring one
// would make a client bug look like a geometry bug.
if w := doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{"plantId": plantID}, cookie); w.Code != http.StatusBadRequest {
t.Errorf("no region = %d, want 400", w.Code)
}
both := map[string]any{
"plantId": plantID, "region": "all",
"rect": map[string]any{"minXCm": -50, "minYCm": -50, "maxXCm": 50, "maxYCm": 50},
}
if w := doJSON(t, r, http.MethodPost, fillPath(objID), both, cookie); w.Code != http.StatusBadRequest {
t.Errorf("both region and rect = %d, want 400", w.Code)
}
// An unknown compass name is rejected rather than silently filling nothing.
if w := doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{
"plantId": plantID, "region": "middle-ish",
}, cookie); w.Code != http.StatusBadRequest {
t.Errorf("bad region name = %d, want 400", w.Code)
}
// A zero-area rect is malformed input, not "plant one at the centre". An empty
// `"rect": {}` decodes to all-zeros and must be caught the same way.
for _, rect := range []map[string]any{
{}, // {} → 0,0,0,0
{"minXCm": 10, "minYCm": 10, "maxXCm": 10, "maxYCm": 50}, // zero width
{"minXCm": 10, "minYCm": 50, "maxXCm": 50, "maxYCm": 50}, // zero height
{"minXCm": 50, "minYCm": 50, "maxXCm": 10, "maxYCm": 10}, // inverted
} {
if w := doJSON(t, r, http.MethodPost, fillPath(objID),
map[string]any{"plantId": plantID, "rect": rect}, cookie); w.Code != http.StatusBadRequest {
t.Errorf("degenerate rect %v = %d, want 400", rect, w.Code)
}
}
// A rect confined to the NE corner produces plops only there. Local frame:
// +x east, -y north.
w := doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{
"plantId": plantID,
"rect": map[string]any{"minXCm": 0, "minYCm": -200, "maxXCm": 200, "maxYCm": 0},
}, cookie)
if w.Code != http.StatusOK {
t.Fatalf("rect fill: status %d, body %s", w.Code, w.Body.String())
}
plops, _ := decodeMap(t, w.Body.Bytes())["plantings"].([]any)
if len(plops) == 0 {
t.Fatal("rect fill created nothing")
}
for _, raw := range plops {
p := raw.(map[string]any)
if x, y := p["xCm"].(float64), p["yCm"].(float64); x < 0 || y > 0 {
t.Errorf("plop at (%v,%v) outside the NE rect", x, y)
}
}
}
// TestClearObjectIsOneChangeSetAPI is the regression test for the behaviour this
// endpoint exists to restore.
//
// The UI used to clear a bed with a loop of PATCHes, and since every service
// mutation auto-scopes its own change set, clearing a 40-plop bed wrote 40 of
// them — 40 presses of Undo to put the bed back. CLAUDE.md states the rule
// directly: multi-row operations record together so they undo as one unit.
func TestClearObjectIsOneChangeSetAPI(t *testing.T) {
r := authEngine(t, localCfg())
cookie := registerAndCookie(t, r, "[email protected]")
gid, objID, plantID := seedFillableBed(t, r, cookie, 300, 300, 20)
w := doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{
"plantId": plantID, "region": "all",
}, cookie)
if w.Code != http.StatusOK {
t.Fatalf("fill: status %d, body %s", w.Code, w.Body.String())
}
created := int(decodeMap(t, w.Body.Bytes())["created"].(float64))
if created < 4 {
t.Fatalf("need several plops to make this meaningful, got %d", created)
}
before := countChangeSets(t, r, cookie, gid)
if w := doJSON(t, r, http.MethodPost, clearPath(objID), nil, cookie); w.Code != http.StatusOK {
t.Fatalf("clear: status %d", w.Code)
}
if after := countChangeSets(t, r, cookie, gid); after != before+1 {
t.Errorf("clearing %d plops added %d change sets, want exactly 1", created, after-before)
}
}
// TestFillClearPermissionsAPI: a viewer may look but not fill or clear, and a
// stranger gets 404 because existence is masked.
func TestFillClearPermissionsAPI(t *testing.T) {
r := authEngine(t, localCfg())
owner := registerAndCookie(t, r, "[email protected]")
viewer := registerAndCookie(t, r, "[email protected]")
stranger := registerAndCookie(t, r, "[email protected]")
gid, objID, plantID := seedFillableBed(t, r, owner, 200, 200, 20)
if w := doJSON(t, r, http.MethodPost, sharesPath(gid),
map[string]any{"email": "[email protected]", "role": "viewer"}, owner); w.Code != http.StatusCreated {
t.Fatalf("share as viewer: status %d, body %s", w.Code, w.Body.String())
}
fillBody := map[string]any{"plantId": plantID, "region": "all"}
if w := doJSON(t, r, http.MethodPost, fillPath(objID), fillBody, viewer); w.Code != http.StatusForbidden {
t.Errorf("viewer fill = %d, want 403 (they can see it but may not do that)", w.Code)
}
if w := doJSON(t, r, http.MethodPost, clearPath(objID), nil, viewer); w.Code != http.StatusForbidden {
t.Errorf("viewer clear = %d, want 403", w.Code)
}
if w := doJSON(t, r, http.MethodPost, fillPath(objID), fillBody, stranger); w.Code != http.StatusNotFound {
t.Errorf("stranger fill = %d, want 404 (existence masked)", w.Code)
}
if w := doJSON(t, r, http.MethodPost, clearPath(objID), nil, stranger); w.Code != http.StatusNotFound {
t.Errorf("stranger clear = %d, want 404", w.Code)
}
if w := doJSON(t, r, http.MethodPost, fillPath(objID), fillBody, nil); w.Code != http.StatusUnauthorized {
t.Errorf("anonymous fill = %d, want 401", w.Code)
}
if w := doJSON(t, r, http.MethodPost, clearPath(objID), nil, nil); w.Code != http.StatusUnauthorized {
t.Errorf("anonymous clear = %d, want 401", w.Code)
}
}
+101
View File
@@ -0,0 +1,101 @@
package api
import (
"bufio"
"net/http/httptest"
"strings"
"testing"
"time"
"github.com/gin-gonic/gin"
)
// streamFrames spins up a real http.Server with the given WriteTimeout and an
// SSE handler that emits `frames` data frames, one every `tick`, then returns.
// It reports how many frames the client actually received and any read error —
// the only vantage point from which the deadline failures in #78/#87 are
// visible, since the writes themselves return nil when the bytes are dropped.
func streamFrames(t *testing.T, serverWriteTimeout, tick time.Duration, frames int) (int, error) {
t.Helper()
gin.SetMode(gin.TestMode)
r := gin.New()
r.GET("/stream", func(c *gin.Context) {
s := openEventStream(c)
for i := 0; i < frames; i++ {
time.Sleep(tick)
s.send(chatEvent{Error: "frame"})
}
})
srv := httptest.NewUnstartedServer(r)
srv.Config.WriteTimeout = serverWriteTimeout
srv.Start()
defer srv.Close()
resp, err := srv.Client().Get(srv.URL + "/stream")
if err != nil {
t.Fatalf("get: %v", err)
}
defer resp.Body.Close()
got := 0
sc := bufio.NewScanner(resp.Body)
for sc.Scan() {
if strings.HasPrefix(sc.Text(), "data: ") {
got++
}
}
return got, sc.Err()
}
// TestEventStreamOutlivesServerWriteTimeout is the regression test for #78.
//
// http.Server.WriteTimeout is an ABSOLUTE deadline measured from when the
// request header was read — not an idle timeout — so a streaming response is cut
// once it passes, however recently the handler wrote. pansy sets it to 30s while
// an agent turn may run for minutes. openEventStream must override it.
//
// This has to be asserted from the CLIENT side because the failure cannot be
// observed from the handler: writes made after the deadline return err == nil
// and their bytes are silently discarded. A test that checked the return of
// io.WriteString would pass against the bug.
func TestEventStreamOutlivesServerWriteTimeout(t *testing.T) {
// sseWriteTimeout stays at its 30s default here, so the per-frame refresh
// keeps the stream alive with a huge margin — CI slowness only ever makes
// this pass more surely. The server's 300ms WriteTimeout is the thing being
// overridden; frames straddle it (300ms/600ms/900ms).
got, err := streamFrames(t, 300*time.Millisecond, 300*time.Millisecond, 3)
if err != nil {
t.Errorf("client read error after %d/3 frames: %v", got, err)
}
if got != 3 {
t.Errorf("client received %d frames, want 3 — the stream was cut at the server WriteTimeout", got)
}
}
// TestEventStreamRefreshesDeadlinePerFrame guards the #87 fix specifically: the
// write deadline is refreshed on EVERY frame, not set once.
//
// A set-once deadline is a plausible "simplification" and it reintroduces the
// unbounded-block risk the per-frame refresh exists to prevent — yet it would
// sail through the test above, whose whole run is far under sseWriteTimeout. So
// shrink sseWriteTimeout below the stream's total duration and send frames whose
// gap stays comfortably under it: per-frame refresh delivers them all, while a
// deadline set once at open would expire mid-stream and cut it short.
func TestEventStreamRefreshesDeadlinePerFrame(t *testing.T) {
orig := sseWriteTimeout
sseWriteTimeout = 400 * time.Millisecond
t.Cleanup(func() { sseWriteTimeout = orig })
// The server WriteTimeout is generous (5s), so it isn't the limiter — the
// per-frame sseWriteTimeout is. 8 frames at a 100ms tick span 800ms, well past
// the 400ms deadline, but each 100ms gap is a 4× margin under it.
got, err := streamFrames(t, 5*time.Second, 100*time.Millisecond, 8)
if err != nil {
t.Errorf("client read error after %d/8 frames: %v", got, err)
}
if got != 8 {
t.Errorf("client received %d frames, want 8 — a set-once deadline would cut the stream at ~%v; the refresh must be per-frame",
got, sseWriteTimeout)
}
}
+6 -8
View File
@@ -5,25 +5,23 @@ import { useClearObject } from '@/lib/objects'
/** Confirm clearing every active plop from a focused bed (soft-remove — the rows
* are kept with removed_at, so history survives). */
export function ClearBedModal({
objectId,
objectName,
plopCount,
plops,
gardenId,
onClose,
}: {
objectId: number
objectName: string
plopCount: number
plops: { id: number; version: number }[]
gardenId: number
onClose: () => void
}) {
const clear = useClearObject(gardenId)
const n = plops.length
return (
<Modal title="Clear bed" onClose={onClose} busy={clear.isPending}>
<div className="flex flex-col gap-4">
<p className="text-sm text-muted">
Remove all <span className="font-medium text-fg">{plopCount}</span>{' '}
{plopCount === 1 ? 'plant' : 'plants'} from{' '}
Remove all <span className="font-medium text-fg">{n}</span> {n === 1 ? 'plant' : 'plants'} from{' '}
<span className="font-medium text-fg">{objectName}</span>? They're marked removed but kept in history.
</p>
<div className="flex justify-end gap-2">
@@ -33,8 +31,8 @@ export function ClearBedModal({
<Button
type="button"
variant="danger"
disabled={clear.isPending || plopCount === 0}
onClick={() => clear.mutate(objectId, { onSuccess: onClose })}
disabled={clear.isPending || n === 0}
onClick={() => clear.mutate(plops, { onSuccess: onClose })}
>
{clear.isPending ? 'Clearing' : 'Clear bed'}
</Button>
+17 -17
View File
@@ -328,28 +328,28 @@ export function useUpdatePlanting(gardenId: number) {
})
}
const clearResultSchema = z.object({ cleared: z.number() })
/** Clear a bed: soft-remove every active plop in an object (#82).
*
* ONE request, and so ONE change set. This used to be a loop of PATCHes, which
* meant clearing a 40-plop bed wrote 40 change sets and took 40 presses of Undo
* to put back — while the agent's clear_object, for the identical user-facing
* action, undid in a single click. The rule it violated is stated in CLAUDE.md:
* multi-row operations record all their changes together so they undo as one
* unit. Doing it server-side also removes the partial-failure case the old loop
* had to reconcile. */
/** Clear a bed: soft-remove every active plop in an object (a loop of PATCHes;
* a bulk ClearObject endpoint arrives with the agent seam, #19). Invalidates
* once at the end. Pass the object's active plops (id + current version). */
export function useClearObject(gardenId: number) {
const qc = useQueryClient()
return useMutation({
mutationFn: async (objectId: number): Promise<number> => {
// No body — clear takes none; passing undefined sends none rather than an
// empty {}. The response is just a count; validate it rather than cast.
const res = clearResultSchema.parse(await api.post(`/objects/${objectId}/clear`))
return res.cleared
mutationFn: async (plops: { id: number; version: number }[]) => {
const today = new Date().toISOString().slice(0, 10)
// allSettled, not all: a partial failure still soft-removed some rows
// server-side, so we must reconcile the cache rather than roll everything
// back. Report how many failed.
const results = await Promise.allSettled(
plops.map((p) => api.patch(`/plantings/${p.id}`, { removedAt: today, version: p.version })),
)
const failed = results.filter((r) => r.status === 'rejected').length
if (failed > 0) {
throw new Error(`${failed} of ${plops.length} plants couldn't be cleared — refresh and try again.`)
}
},
// Reconcile on success OR partial failure, so the cache matches the server.
onSettled: () => qc.invalidateQueries({ queryKey: fullKey(gardenId) }),
onError: (err) => toast.error(objectErrorMessage(err, 'Could not clear the bed.')),
onError: (err) => toast.error(err instanceof Error ? err.message : 'Could not clear the bed.'),
})
}
+1 -2
View File
@@ -526,9 +526,8 @@ export function GardenEditorPage() {
{clearing && focusedObject && (
<ClearBedModal
objectId={focusedObject.id}
objectName={objectDisplayName(focusedObject)}
plopCount={focusedPlops.length}
plops={focusedPlops.map((p) => ({ id: p.id, version: p.version }))}
gardenId={gid}
onClose={() => setClearing(false)}
/>