fix(workspace-server): emit null removed_at when timestamp fetch fails

#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) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang 2026-04-30 22:24:59 -07:00
parent b97a346fbf
commit 364c70fc71
2 changed files with 83 additions and 8 deletions

View File

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

View File

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