From 364c70fc71b4eb065856063da9540a604be2f781 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 30 Apr 2026 22:24:59 -0700 Subject: [PATCH] fix(workspace-server): emit null removed_at when timestamp fetch fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #2429 review finding. The 410-Gone path issues a follow-up `SELECT updated_at` after detecting status='removed'. If that query fails (workspace row deleted between the two queries, transient DB error, etc.), `removedAt` stays as Go's zero time and the JSON body emits `"removed_at": "0001-01-01T00:00:00Z"` — a misleading timestamp the client has to know to ignore. Now we branch on `removedAt.IsZero()` and emit `null` for the failed path. The actionable signal (the 410 + hint) is unchanged; only the timestamp shape gets cleaner. Pinned by `TestWorkspaceGet_RemovedReturns410WithNullRemovedAtOnTimestampFetchFailure`, which simulates the row vanishing via `sqlmock`'s `WillReturnError(sql.ErrNoRows)`. The original `_RemovedReturns410` test now also asserts that the happy-path timestamp is a non-null value (was just checking the key existed). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../internal/handlers/workspace.go | 24 +++++-- .../internal/handlers/workspace_test.go | 67 ++++++++++++++++++- 2 files changed, 83 insertions(+), 8 deletions(-) diff --git a/workspace-server/internal/handlers/workspace.go b/workspace-server/internal/handlers/workspace.go index 23d67f44..78181e61 100644 --- a/workspace-server/internal/handlers/workspace.go +++ b/workspace-server/internal/handlers/workspace.go @@ -660,16 +660,28 @@ func (h *WorkspaceHandler) Get(c *gin.Context) { // to content negotiation. if status, _ := ws["status"].(string); status == string(models.StatusRemoved) { if c.Query("include_removed") != "true" { + // Best-effort fetch of the removal timestamp. If the row was + // deleted (or some transient DB error fired) between the + // scanWorkspaceRow above and this follow-up SELECT, + // removedAt stays as Go's zero time. Emit `null` in that + // case rather than the misleading `0001-01-01T00:00:00Z` + // the client would otherwise see — the actionable signal + // is the 410 + hint, not the timestamp. var removedAt time.Time _ = db.DB.QueryRowContext(c.Request.Context(), `SELECT updated_at FROM workspaces WHERE id = $1`, id, ).Scan(&removedAt) - c.JSON(http.StatusGone, gin.H{ - "error": "workspace removed", - "id": id, - "removed_at": removedAt, - "hint": "Regenerate workspace + token from the canvas → Tokens tab", - }) + body := gin.H{ + "error": "workspace removed", + "id": id, + "hint": "Regenerate workspace + token from the canvas → Tokens tab", + } + if removedAt.IsZero() { + body["removed_at"] = nil + } else { + body["removed_at"] = removedAt + } + c.JSON(http.StatusGone, body) return } } diff --git a/workspace-server/internal/handlers/workspace_test.go b/workspace-server/internal/handlers/workspace_test.go index f1093191..4e17ca6a 100644 --- a/workspace-server/internal/handlers/workspace_test.go +++ b/workspace-server/internal/handlers/workspace_test.go @@ -151,8 +151,8 @@ func TestWorkspaceGet_RemovedReturns410(t *testing.T) { if resp["id"] != id { t.Errorf("expected id %q, got %v", id, resp["id"]) } - if _, ok := resp["removed_at"]; !ok { - t.Errorf("expected removed_at in 410 body, got: %v", resp) + if v, ok := resp["removed_at"]; !ok || v == nil { + t.Errorf("expected removed_at to be a real timestamp on the happy path, got: %v", v) } if _, ok := resp["hint"]; !ok { t.Errorf("expected hint in 410 body, got: %v", resp) @@ -163,6 +163,69 @@ func TestWorkspaceGet_RemovedReturns410(t *testing.T) { } } +// If the follow-up `SELECT updated_at` query fails (workspace row +// disappeared in the gap, transient DB error, etc.), removedAt stays +// as Go's zero time. We emit JSON `null` for that case rather than +// the misleading `"0001-01-01T00:00:00Z"` the client would otherwise +// see — the actionable signal is the 410 + hint, not the timestamp. +func TestWorkspaceGet_RemovedReturns410WithNullRemovedAtOnTimestampFetchFailure(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + id := "cccccccc-0012-0000-0000-000000000000" + + columns := []string{ + "id", "name", "role", "tier", "status", "agent_card", "url", + "parent_id", "active_tasks", "max_concurrent_tasks", "last_error_rate", "last_sample_error", + "uptime_seconds", "current_task", "runtime", "workspace_dir", "x", "y", "collapsed", + "budget_limit", "monthly_spend", + } + mock.ExpectQuery("SELECT w.id, w.name"). + WithArgs(id). + WillReturnRows(sqlmock.NewRows(columns). + AddRow(id, "Vanished", "worker", 1, string(models.StatusRemoved), []byte(`null`), + "", nil, 0, 1, 0.0, "", 0, "", "langgraph", + "", 0.0, 0.0, false, + nil, 0)) + // Simulate the row vanishing between the two queries. + mock.ExpectQuery(`SELECT updated_at FROM workspaces`). + WithArgs(id). + WillReturnError(sql.ErrNoRows) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: id}} + c.Request = httptest.NewRequest("GET", "/workspaces/"+id, nil) + + handler.Get(c) + + if w.Code != http.StatusGone { + t.Fatalf("expected 410 Gone, got %d: %s", w.Code, w.Body.String()) + } + + var resp map[string]interface{} + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("failed to parse 410 body: %v", err) + } + if resp["removed_at"] != nil { + t.Errorf( + "expected removed_at == null when timestamp fetch fails; got %v (type %T). "+ + "Misleading 0001-01-01 timestamps in the JSON would confuse clients.", + resp["removed_at"], resp["removed_at"], + ) + } + // Other fields must still be present. + if resp["error"] != "workspace removed" || resp["id"] != id || resp["hint"] == nil { + t.Errorf("expected error/id/hint to survive the timestamp fetch failure; got %v", resp) + } + + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + // Audit-trail consumers (admin views, "show me deleted workspaces" // tooling) opt into the legacy 200 + body shape via // ?include_removed=true. Without this opt-in path the audit trail