From ce37a28b6bc7082afa33b2518d527ee348985ce6 Mon Sep 17 00:00:00 2001 From: Ubuntu Date: Fri, 31 Jul 2026 12:13:18 +0000 Subject: [PATCH] fix(hub): orphaned agents on a dead host no longer block its deletion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting an offline host 409'd with "host still has active agents" even though its host-runner was gone and the agents had died with it. Two separate defects met in that one refusal. First, handleDeleteHost spelled the terminal-status set differently from every other live-agent query in the package: it listed terminated and failed but not crashed. Since 'crashed' is exactly what the host-runner's reconcile loop reports for an agent whose pane has vanished, the hub's own verdict that an agent was dead was not enough to let its host go. Hoist the predicate into one definition (agent_status.go) and use it at all five sites, so the two spellings cannot drift apart again. Second, even with the set corrected, an agent that never reached a terminal status was a dead end. The hub only reaches an agent through its host-runner; when the runner is gone nothing will ever report that agent terminal, so the refusal could not be satisfied by waiting. The fleet view showed agents "running" on a machine that no longer existed and the row could only be cleared by stopping each agent by hand. Now the host's own status decides: online with live agents still 409s (the runner is there and can stop them properly), offline reaps them to 'crashed' and proceeds. Reaping deliberately does NOT run on a timer. An offline host is not proof of a dead agent — M4 agents live in tmux panes that outlive a host-runner restart, so an agent can be healthy while its runner is upgrading. A clock would kill those irreversibly, since tickReconcile skips terminal agents and a wrong 'crashed' never heals back. The director's DELETE is the judgement that the machine is gone; that is where the reap belongs. The reap runs the same aftermath as any crash — session paused, bearer revoked, digest sealed, task derived — via applyAgentCrashEffects, extracted from handlePatchAgent so a dead agent's token cannot outlive it through the new path. Covers the handler, which had no tests: terminal agents never block, online still refuses, offline reaps, the aftermath runs, the reap is scoped to the host, and the SQL agrees with the Go predicate. Each verified by mutation — reintroducing the missing 'crashed' reproduces the original 409. Co-Authored-By: Claude Opus 5 (1M context) --- docs/reference/api-overview.md | 2 +- docs/reference/openapi.yaml | 10 + hub/internal/server/agent_status.go | 46 ++++ hub/internal/server/handlers_agents.go | 100 ++++++-- .../server/handlers_general_steward.go | 2 +- .../server/handlers_host_delete_test.go | 225 ++++++++++++++++++ hub/internal/server/handlers_hosts.go | 74 ++++-- .../server/handlers_project_steward.go | 2 +- lib/l10n/app_en.arb | 2 +- lib/l10n/app_zh.arb | 2 +- 10 files changed, 429 insertions(+), 36 deletions(-) create mode 100644 hub/internal/server/agent_status.go create mode 100644 hub/internal/server/handlers_host_delete_test.go diff --git a/docs/reference/api-overview.md b/docs/reference/api-overview.md index 0782377c8..85631d56c 100644 --- a/docs/reference/api-overview.md +++ b/docs/reference/api-overview.md @@ -99,7 +99,7 @@ Detail: [`hub-agents.md`](hub-agents.md), [ADR-003](../decisions/003-a2a-relay-r |---|---|---| | POST | `/v1/teams/{team}/hosts` | Register a host (host-runner) | | GET | `/v1/teams/{team}/hosts` | List hosts | -| GET / DELETE | `/v1/teams/{team}/hosts/{host}` | Get / delete host | +| GET / DELETE | `/v1/teams/{team}/hosts/{host}` | Get / delete host (delete reaps orphaned agents when the host is offline; 409 when it is online with live agents) | | POST | `/v1/teams/{team}/hosts/{host}/heartbeat` | Heartbeat with build metadata + capabilities | | GET | `/v1/teams/{team}/hosts/{host}/commands` | Pull queued out-of-band commands | | PATCH | `/v1/teams/{team}/hosts/{host}/ssh_hint` | Update non-secret SSH hint | diff --git a/docs/reference/openapi.yaml b/docs/reference/openapi.yaml index 95d46bb9b..e59ac9ec6 100644 --- a/docs/reference/openapi.yaml +++ b/docs/reference/openapi.yaml @@ -781,9 +781,19 @@ paths: tags: [Hosts] operationId: deleteHost summary: Delete a host (admin) + description: >- + Deletes the host row. Agents still assigned to it are handled by + whether the host is reachable: if the host is `online` with live + agents the delete is refused with 409 (its host-runner can stop them + properly first); if the host is `offline` those agents are orphaned by + definition, so they are reaped to `crashed` — sessions paused, bearer + tokens revoked — and the delete proceeds. Agents already in a terminal + status (`terminated`, `crashed`, `failed`) never block a delete. responses: "204": { description: Deleted } "404": { $ref: "#/components/responses/NotFound" } + "409": + description: Host is online and still has live agents /v1/teams/{team}/hosts/{host}/heartbeat: parameters: diff --git a/hub/internal/server/agent_status.go b/hub/internal/server/agent_status.go new file mode 100644 index 000000000..de4fdbb39 --- /dev/null +++ b/hub/internal/server/agent_status.go @@ -0,0 +1,46 @@ +package server + +// Agent terminal statuses — ONE definition, because this predicate decides +// whether an agent still counts as alive and had drifted into two spellings. +// +// The lifecycle (docs/reference/glossary.md, ADR-009) is +// +// pending → running → terminated | crashed | failed → archived +// +// so all three of terminated/crashed/failed are ends of the line: +// +// - terminated — the operator stopped it (or a swap replaced it); +// - crashed — the process died under it (the host-runner's reconcile +// loop reports this when a pane is gone or a driver is missing); +// - failed — it stopped itself with an error. +// +// They differ only in *why* the process ended, never in *whether* it did, so +// any query asking "is this agent still alive" must treat them identically. +// Before this constant existed, handleDeleteHost listed only two of the +// three, which made a crashed agent — a corpse by every other query's +// reckoning — permanently block deletion of the host it died on. +// +// archived_at is a separate axis: archiving is only reachable from a +// terminal status (handleArchiveAgent), so a live-agent check never needs it. +const ( + // sqlAgentNotTerminal is the WHERE-clause fragment for "this agent is + // still alive". Concatenate it into a query; it names no bind + // parameters and no table qualifier, so it composes anywhere `agents` + // is the only table in scope. + sqlAgentNotTerminal = `status NOT IN ('terminated','crashed','failed')` +) + +// agentTerminalStatuses is the same set as Go values, for callers deciding in +// code rather than in SQL. Keep in lockstep with sqlAgentNotTerminal — +// TestAgentTerminalStatusesAgreeWithSQL asserts they do. +var agentTerminalStatuses = []string{"terminated", "crashed", "failed"} + +// isAgentTerminal reports whether an agent status means the process is over. +func isAgentTerminal(status string) bool { + for _, s := range agentTerminalStatuses { + if status == s { + return true + } + } + return false +} diff --git a/hub/internal/server/handlers_agents.go b/hub/internal/server/handlers_agents.go index 7d306da3f..8c72ec03c 100644 --- a/hub/internal/server/handlers_agents.go +++ b/hub/internal/server/handlers_agents.go @@ -151,7 +151,7 @@ func (s *Server) handleListAgents(w http.ResponseWriter, r *http.Request) { case live: q += " AND status IN ('running','idle','paused')" case !includeTerminated: - q += " AND status NOT IN ('terminated','failed','crashed')" + q += " AND " + sqlAgentNotTerminal } if pid := r.URL.Query().Get("project_id"); pid != "" { q += " AND project_id = ?" @@ -397,20 +397,7 @@ func (s *Server) handlePatchAgent(w http.ResponseWriter, r *http.Request) { s.applyAgentTerminationEffects(r.Context(), team, id, "agent stopped via PATCH", false) } else if in.Status != nil && (*in.Status == "crashed" || *in.Status == "failed") { - _, _ = s.writeDB.ExecContext(r.Context(), ` - UPDATE sessions - SET status = 'paused', last_active_at = ? - WHERE team_id = ? - AND current_agent_id = ? - AND status = 'active'`, - NowUTC(), team, id) - _, _ = auth.RevokeAgentTokens(r.Context(), s.writeDB, id, NowUTC()) - // Fold + stamp the run digest now (#118 §4). The operator stop path - // finalizes via stopSessionInternal; a crash/failure flows through - // here instead, so without this the first Insight open after the crash - // pays the full O(n) backfill. finalizeDigestOutcome brings the digest - // current off the read path. - s.finalizeDigestOutcome(r.Context(), team, id) + s.applyAgentCrashEffects(r.Context(), team, id) } // ADR-029 D-3: auto-derive the linked task's status from the // agent's terminal transition. Most-recent-spawn drives; older @@ -466,6 +453,87 @@ func (s *Server) applyAgentTerminationEffects(ctx context.Context, team, id, rea s.finalizeDigestOutcome(ctx, team, id) } +// applyAgentCrashEffects runs the side-effects of an agent reaching a +// terminal status it did NOT choose — 'crashed' or 'failed'. Unlike the +// operator's stop (applyAgentTerminationEffects) there is no host command to +// enqueue and no agent.terminate audit row: nobody asked for this, the +// process simply ended. What remains is cleaning up after it — pause the +// session that now points at nothing, revoke the bearer the dead process +// held, and seal the run digest so the next Insight open doesn't pay an O(n) +// backfill (#118 §4). +// +// Extracted from handlePatchAgent so the orphan reaper produces the identical +// aftermath; a second copy would drift, and a crash whose tokens outlive it +// is a security bug, not a cosmetic one. The caller owns flipping +// agents.status and calling deriveTaskStatusFromAgent (ADR-029 D-3). +func (s *Server) applyAgentCrashEffects(ctx context.Context, team, id string) { + _, _ = s.writeDB.ExecContext(ctx, ` + UPDATE sessions + SET status = 'paused', last_active_at = ? + WHERE team_id = ? + AND current_agent_id = ? + AND status = 'active'`, + NowUTC(), team, id) + _, _ = auth.RevokeAgentTokens(ctx, s.writeDB, id, NowUTC()) + s.finalizeDigestOutcome(ctx, team, id) +} + +// liveAgent is the minimum an orphan sweep needs: who to reap, and what to +// call them in the audit record. +type liveAgent struct { + ID string + Handle string +} + +// liveAgentsOnHost lists the agents assigned to a host whose process has not +// ended. Used by the host-delete guard to decide between refusing and +// reaping. +func (s *Server) liveAgentsOnHost(ctx context.Context, team, host string) ([]liveAgent, error) { + rows, err := s.db.QueryContext(ctx, ` + SELECT id, handle FROM agents + WHERE team_id = ? AND host_id = ? + AND `+sqlAgentNotTerminal, team, host) + if err != nil { + return nil, err + } + defer rows.Close() + var out []liveAgent + for rows.Next() { + var a liveAgent + if err := rows.Scan(&a.ID, &a.Handle); err != nil { + return nil, err + } + out = append(out, a) + } + return out, rows.Err() +} + +// reapOrphanedAgent flips one agent to 'crashed' and runs the crash +// aftermath. Called when the host an agent lived on is being deleted while +// offline: the process is unreachable by construction, so leaving the row +// claiming 'running' would assert something the hub cannot possibly know. +// +// 'crashed' rather than 'terminated' on purpose — nobody stopped this agent, +// its floor fell away. The distinction is load-bearing downstream: ADR-029 +// D-3 derives task.status='blocked' from a crash (work interrupted, worth +// retrying) but 'cancelled' from a terminate with no result (work abandoned). +func (s *Server) reapOrphanedAgent(ctx context.Context, team, id string) error { + res, err := s.writeDB.ExecContext(ctx, ` + UPDATE agents SET status = 'crashed', terminated_at = ? + WHERE team_id = ? AND id = ? + AND `+sqlAgentNotTerminal, NowUTC(), team, id) + if err != nil { + return err + } + // Lost a race with a real termination — the aftermath already ran. + if n, _ := res.RowsAffected(); n == 0 { + return nil + } + s.applyAgentCrashEffects(ctx, team, id) + _ = s.deriveTaskStatusFromAgent(ctx, team, id, "crashed") + return nil +} + // handleStopAgent is POST /v1/teams/{team}/agents/{agent}/stop — the // RESUMABLE kill. Kills the agent and flips its session to `paused`, so // `agents.resume` can respawn it. The principal's "Stop session" does @@ -1484,7 +1552,7 @@ func (s *Server) DoSpawn(ctx context.Context, team string, in spawnIn) (spawnOut UPDATE agents SET status = 'terminated', terminated_at = ? WHERE team_id = ? AND id = ? - AND status NOT IN ('terminated','failed','crashed')`, + AND `+sqlAgentNotTerminal, now, team, priorAgentID); err != nil { return spawnOut{}, http.StatusInternalServerError, err } diff --git a/hub/internal/server/handlers_general_steward.go b/hub/internal/server/handlers_general_steward.go index abf1874cf..7d99f8d54 100644 --- a/hub/internal/server/handlers_general_steward.go +++ b/hub/internal/server/handlers_general_steward.go @@ -185,7 +185,7 @@ func (s *Server) findRunningGeneralSteward(ctx context.Context, team string) (st err := s.db.QueryRowContext(ctx, ` SELECT id FROM agents WHERE team_id = ? AND kind = ? - AND status NOT IN ('terminated','crashed','failed') + AND `+sqlAgentNotTerminal+` AND terminated_at IS NULL ORDER BY created_at DESC LIMIT 1`, diff --git a/hub/internal/server/handlers_host_delete_test.go b/hub/internal/server/handlers_host_delete_test.go new file mode 100644 index 000000000..d2e64315d --- /dev/null +++ b/hub/internal/server/handlers_host_delete_test.go @@ -0,0 +1,225 @@ +package server + +import ( + "context" + "net/http" + "testing" +) + +// The host-delete guard, and the orphan reaping that lets a dead machine +// actually leave the fleet. Before this suite the handler had no tests at +// all, which is how it came to disagree with every other live-agent query in +// the package about whether 'crashed' means alive. + +func seedHostStatus(t *testing.T, s *Server, team, id, name, status string) { + t.Helper() + if _, err := s.db.ExecContext(context.Background(), + `INSERT INTO hosts (id, team_id, name, status, created_at) + VALUES (?, ?, ?, ?, ?)`, id, team, name, status, NowUTC()); err != nil { + t.Fatalf("seed host %s: %v", id, err) + } +} + +func seedAgentOnHost(t *testing.T, s *Server, team, id, handle, hostID, status string) { + t.Helper() + if _, err := s.db.ExecContext(context.Background(), + `INSERT INTO agents (id, team_id, handle, kind, status, host_id, created_at) + VALUES (?, ?, ?, 'claude-code', ?, ?, ?)`, + id, team, handle, status, hostID, NowUTC()); err != nil { + t.Fatalf("seed agent %s: %v", id, err) + } +} + +func agentStatus(t *testing.T, s *Server, id string) string { + t.Helper() + var st string + if err := s.db.QueryRow(`SELECT status FROM agents WHERE id = ?`, id).Scan(&st); err != nil { + t.Fatalf("read agent %s: %v", id, err) + } + return st +} + +func hostExists(t *testing.T, s *Server, id string) bool { + t.Helper() + var n int + if err := s.db.QueryRow(`SELECT COUNT(*) FROM hosts WHERE id = ?`, id).Scan(&n); err != nil { + t.Fatalf("count host %s: %v", id, err) + } + return n > 0 +} + +// The bug the director hit: an agent the reconcile loop had already declared +// dead still blocked its host's deletion, because this one query spelled the +// terminal set differently from the rest of the package. +func TestDeleteHost_TerminalAgentsNeverBlock(t *testing.T) { + for _, status := range agentTerminalStatuses { + t.Run(status, func(t *testing.T) { + s, token := newA2ATestServer(t) + host := NewID() + seedHostStatus(t, s, defaultTeamID, host, "box-"+status, "online") + seedAgentOnHost(t, s, defaultTeamID, NewID(), "w-"+status, host, status) + + code, body := doReq(t, s, token, http.MethodDelete, + "/v1/teams/"+defaultTeamID+"/hosts/"+host, nil) + if code != http.StatusNoContent { + t.Fatalf("delete host with a %s agent: got %d %s, want 204", + status, code, body) + } + if hostExists(t, s, host) { + t.Fatal("host row survived a 204") + } + }) + } +} + +// The guard still earns its keep while somebody is home to honour it. +func TestDeleteHost_OnlineWithLiveAgentsRefuses(t *testing.T) { + s, token := newA2ATestServer(t) + host := NewID() + agent := NewID() + seedHostStatus(t, s, defaultTeamID, host, "live-box", "online") + seedAgentOnHost(t, s, defaultTeamID, agent, "worker", host, "running") + + code, _ := doReq(t, s, token, http.MethodDelete, + "/v1/teams/"+defaultTeamID+"/hosts/"+host, nil) + if code != http.StatusConflict { + t.Fatalf("delete online host with a running agent: got %d, want 409", code) + } + if !hostExists(t, s, host) { + t.Fatal("refused delete still removed the host") + } + if got := agentStatus(t, s, agent); got != "running" { + t.Fatalf("a refused delete must not touch agents: got %q", got) + } +} + +// The deadlock this fixes: the host-runner is gone, so nothing will ever +// report those agents terminal, and nothing can reach them to stop. The +// delete reaps them instead of refusing forever. +func TestDeleteHost_OfflineReapsOrphanedAgents(t *testing.T) { + s, token := newA2ATestServer(t) + host := NewID() + running, pending := NewID(), NewID() + seedHostStatus(t, s, defaultTeamID, host, "dead-box", "offline") + seedAgentOnHost(t, s, defaultTeamID, running, "worker-1", host, "running") + seedAgentOnHost(t, s, defaultTeamID, pending, "worker-2", host, "pending") + + code, body := doReq(t, s, token, http.MethodDelete, + "/v1/teams/"+defaultTeamID+"/hosts/"+host, nil) + if code != http.StatusNoContent { + t.Fatalf("delete offline host: got %d %s, want 204", code, body) + } + if hostExists(t, s, host) { + t.Fatal("host row survived a 204") + } + // 'crashed', not 'terminated' — nobody stopped these agents, and ADR-029 + // D-3 reads the difference (blocked vs cancelled) onto their tasks. + for _, id := range []string{running, pending} { + if got := agentStatus(t, s, id); got != "crashed" { + t.Fatalf("orphaned agent %s: got %q, want crashed", id, got) + } + } + var terminated int + if err := s.db.QueryRow( + `SELECT COUNT(*) FROM agents WHERE terminated_at IS NOT NULL`).Scan(&terminated); err != nil { + t.Fatalf("count terminated_at: %v", err) + } + if terminated != 2 { + t.Fatalf("reaped agents must be stamped terminated_at: got %d of 2", terminated) + } +} + +// Reaping is not just a status write — the aftermath has to run, or a dead +// agent's bearer outlives it and its session keeps claiming to be live. +func TestDeleteHost_ReapRunsCrashAftermath(t *testing.T) { + s, token := newA2ATestServer(t) + host := NewID() + agent := NewID() + session := NewID() + seedHostStatus(t, s, defaultTeamID, host, "dead-box", "offline") + seedAgentOnHost(t, s, defaultTeamID, agent, "worker", host, "running") + if _, err := s.db.Exec(` + INSERT INTO sessions + (id, team_id, scope_kind, current_agent_id, status, opened_at, last_active_at) + VALUES (?, ?, 'agent', ?, 'active', ?, ?)`, + session, defaultTeamID, agent, NowUTC(), NowUTC()); err != nil { + t.Fatalf("seed session: %v", err) + } + + if code, body := doReq(t, s, token, http.MethodDelete, + "/v1/teams/"+defaultTeamID+"/hosts/"+host, nil); code != http.StatusNoContent { + t.Fatalf("delete offline host: got %d %s, want 204", code, body) + } + + var st string + if err := s.db.QueryRow( + `SELECT status FROM sessions WHERE id = ?`, session).Scan(&st); err != nil { + t.Fatalf("read session: %v", err) + } + if st != "paused" { + t.Fatalf("session pointing at a reaped agent: got %q, want paused", st) + } +} + +// The delete must not reach across teams, and a host that was never there is +// a 404 — not a silent 204 that reports success for nothing. +func TestDeleteHost_MissingIs404(t *testing.T) { + s, token := newA2ATestServer(t) + code, _ := doReq(t, s, token, http.MethodDelete, + "/v1/teams/"+defaultTeamID+"/hosts/"+NewID(), nil) + if code != http.StatusNotFound { + t.Fatalf("delete absent host: got %d, want 404", code) + } +} + +// An agent parked on ANOTHER host must never be dragged into this delete. +func TestDeleteHost_ReapIsScopedToTheHost(t *testing.T) { + s, token := newA2ATestServer(t) + dead, alive := NewID(), NewID() + bystander := NewID() + seedHostStatus(t, s, defaultTeamID, dead, "dead-box", "offline") + seedHostStatus(t, s, defaultTeamID, alive, "live-box", "online") + seedAgentOnHost(t, s, defaultTeamID, NewID(), "doomed", dead, "running") + seedAgentOnHost(t, s, defaultTeamID, bystander, "bystander", alive, "running") + + if code, _ := doReq(t, s, token, http.MethodDelete, + "/v1/teams/"+defaultTeamID+"/hosts/"+dead, nil); code != http.StatusNoContent { + t.Fatalf("delete offline host: got %d, want 204", code) + } + if got := agentStatus(t, s, bystander); got != "running" { + t.Fatalf("agent on a different host got reaped: %q", got) + } + if !hostExists(t, s, alive) { + t.Fatal("the wrong host was deleted") + } +} + +// The two spellings of the terminal set must not drift apart again — this is +// the invariant the original bug violated, stated directly. +func TestAgentTerminalStatusesAgreeWithSQL(t *testing.T) { + s, _ := newA2ATestServer(t) + host := NewID() + seedHostStatus(t, s, defaultTeamID, host, "box", "online") + + all := append([]string{"pending", "running", "paused"}, agentTerminalStatuses...) + for _, st := range all { + seedAgentOnHost(t, s, defaultTeamID, NewID(), "a-"+st, host, st) + } + live, err := s.liveAgentsOnHost(context.Background(), defaultTeamID, host) + if err != nil { + t.Fatalf("liveAgentsOnHost: %v", err) + } + // Whatever isAgentTerminal says in Go, the SQL must have filtered. + if len(live) != 3 { + t.Fatalf("live agents: got %d, want 3 (pending/running/paused)", len(live)) + } + for _, ag := range live { + var st string + if err := s.db.QueryRow(`SELECT status FROM agents WHERE id = ?`, ag.ID).Scan(&st); err != nil { + t.Fatalf("read agent: %v", err) + } + if isAgentTerminal(st) { + t.Fatalf("SQL returned %q as live but isAgentTerminal calls it terminal", st) + } + } +} diff --git a/hub/internal/server/handlers_hosts.go b/hub/internal/server/handlers_hosts.go index eb55a28a8..21e1df9f2 100644 --- a/hub/internal/server/handlers_hosts.go +++ b/hub/internal/server/handlers_hosts.go @@ -6,6 +6,7 @@ import ( "errors" "io" "net/http" + "strconv" "strings" "github.com/go-chi/chi/v5" @@ -261,30 +262,70 @@ func (s *Server) handleHostHeartbeat(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) } -// handleDeleteHost removes a host row. Refuses if any agents on this host -// are still alive (status not in terminated/failed) — otherwise those rows -// would silently lose their host_id via the ON DELETE SET NULL edge and -// confuse the org chart. host_commands cascade-delete. +// handleDeleteHost removes a host row, and answers the question "what +// happens to the agents that were on it?" differently depending on whether +// anyone is still home: +// +// - host ONLINE with live agents → 409. The host-runner is right there and +// can stop them properly; deleting underneath it would strand real +// processes and null out their host_id via the ON DELETE SET NULL edge, +// leaving running agents that claim to live nowhere. +// +// - host OFFLINE with live agents → the agents are ORPHANED, so reap them +// to 'crashed' and proceed. Nothing can reach them: the hub only ever +// talks to an agent through its host-runner, and the queued host_commands +// that a stop would enqueue cascade-delete with this row anyway. Refusing +// here used to be a dead end — the fleet view showed agents "running" on +// a machine that no longer exists, and the only way to clear them was to +// stop each one by hand first. The director's DELETE *is* the judgement +// that the machine is gone; a status of 'running' on a host we are about +// to erase is a lie either way, and 'crashed' is the honest reading. +// +// Deliberately NOT a background timer: an offline host is not proof of a dead +// agent. M4 agents live in tmux panes that outlive a host-runner restart +// (tmux is a separate server — see hostrunner/reconcile.go), so an agent can +// be perfectly healthy while its runner is being upgraded. A reaper on a +// clock would kill those, and irreversibly: tickReconcile skips agents in a +// terminal status, so a wrong 'crashed' verdict never heals back to running +// and takes the driver down with it. Reaping only on an explicit delete keeps +// the call where the evidence is — with the human who knows the box is gone. func (s *Server) handleDeleteHost(w http.ResponseWriter, r *http.Request) { team := chi.URLParam(r, "team") host := chi.URLParam(r, "host") - var alive int - err := s.db.QueryRowContext(r.Context(), ` - SELECT COUNT(*) FROM agents - WHERE team_id = ? AND host_id = ? - AND status NOT IN ('terminated','failed')`, team, host).Scan(&alive) + + var name, status string + err := s.db.QueryRowContext(r.Context(), + `SELECT name, status FROM hosts WHERE team_id = ? AND id = ?`, + team, host).Scan(&name, &status) + if errors.Is(err, sql.ErrNoRows) { + writeErr(w, http.StatusNotFound, "host not found") + return + } if err != nil { s.writeDBErr(w, err) return } - if alive > 0 { + + live, err := s.liveAgentsOnHost(r.Context(), team, host) + if err != nil { + s.writeDBErr(w, err) + return + } + if len(live) > 0 && status == "online" { writeErr(w, http.StatusConflict, "host still has active agents — terminate them first") return } - var name string - _ = s.db.QueryRowContext(r.Context(), - `SELECT name FROM hosts WHERE team_id = ? AND id = ?`, team, host).Scan(&name) + reaped := make([]string, 0, len(live)) + for _, ag := range live { + if err := s.reapOrphanedAgent(r.Context(), team, ag.ID); err != nil { + s.log.Warn("reap orphaned agent failed", + "agent", ag.ID, "host", host, "err", err) + continue + } + reaped = append(reaped, ag.Handle) + } + res, err := s.writeDB.ExecContext(r.Context(), `DELETE FROM hosts WHERE team_id = ? AND id = ?`, team, host) if err != nil { @@ -300,8 +341,11 @@ func (s *Server) handleDeleteHost(w http.ResponseWriter, r *http.Request) { if name != "" { summary = "delete host " + name } - s.recordAudit(r.Context(), team, "host.delete", "host", host, - summary, map[string]any{"name": name}) + if len(reaped) > 0 { + summary += " (orphaned " + strconv.Itoa(len(reaped)) + " agent(s))" + } + s.recordAudit(r.Context(), team, "host.delete", "host", host, summary, + map[string]any{"name": name, "status": status, "orphaned_agents": reaped}) w.WriteHeader(http.StatusNoContent) } diff --git a/hub/internal/server/handlers_project_steward.go b/hub/internal/server/handlers_project_steward.go index c10505778..60c78dbe8 100644 --- a/hub/internal/server/handlers_project_steward.go +++ b/hub/internal/server/handlers_project_steward.go @@ -206,7 +206,7 @@ func (s *Server) findRunningProjectSteward(ctx context.Context, team, project st WHERE team_id = ? AND project_id = ? AND kind LIKE 'steward.%' - AND status NOT IN ('terminated','crashed','failed') + AND `+sqlAgentNotTerminal+` AND archived_at IS NULL ORDER BY created_at DESC LIMIT 1`, diff --git a/lib/l10n/app_en.arb b/lib/l10n/app_en.arb index f360bc336..59a12799f 100644 --- a/lib/l10n/app_en.arb +++ b/lib/l10n/app_en.arb @@ -2200,7 +2200,7 @@ "unavailable": "—", "thisEntity": "this {entity}", "@thisEntity": { "placeholders": { "entity": { "type": "String" } } }, - "hostDeleteBody": "Removes the {host} row from the hub. The host-runner, if still running, will register a fresh row on its next boot. The hub refuses the delete if any {agents} are still alive on this {host}.", + "hostDeleteBody": "Removes the {host} row from the hub. The host-runner, if still running, will register a fresh row on its next boot. If this {host} is online and still has live {agents}, the hub refuses the delete — stop them first. If it is offline, its remaining {agents} are unreachable and are marked crashed.", "@hostDeleteBody": { "placeholders": { "host": { "type": "String" }, "agents": { "type": "String" } } }, "hostEntityId": "{host} ID", "@hostEntityId": { "placeholders": { "host": { "type": "String" } } }, diff --git a/lib/l10n/app_zh.arb b/lib/l10n/app_zh.arb index 925ca1bb8..a32617e04 100644 --- a/lib/l10n/app_zh.arb +++ b/lib/l10n/app_zh.arb @@ -2005,7 +2005,7 @@ "unavailable": "—", "thisEntity": "此{entity}", "@thisEntity": { "placeholders": { "entity": { "type": "String" } } }, - "hostDeleteBody": "从 hub 中移除此{host}行。如果 host-runner 仍在运行，它会在下次启动时注册新的行。如果此{host}上仍有任何存活的{agents}，hub 会拒绝删除。", + "hostDeleteBody": "从 hub 中移除此{host}行。如果 host-runner 仍在运行，它会在下次启动时注册新的行。如果此{host}在线且仍有存活的{agents}，hub 会拒绝删除——请先停止它们。如果此{host}离线，其余的{agents}已无法访问，将被标记为 crashed。", "@hostDeleteBody": { "placeholders": { "host": { "type": "String" }, "agents": { "type": "String" } } }, "hostEntityId": "{host} ID", "@hostEntityId": { "placeholders": { "host": { "type": "String" } } },