From c63c9e4fe878253a7309d8eda2a37b951de6e1e4 Mon Sep 17 00:00:00 2001 From: webmixgamer Date: Fri, 31 Jul 2026 11:37:28 +0100 Subject: [PATCH 1/2] fix: route fleet restart through the canonical lifecycle path so agents adopt rebuilt base images (#1860) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /api/ops/fleet/restart stopped/started agents with raw Docker calls, bypassing start_agent_internal — no config-drift predicate ran and a rebuilt trinity-agent-base was never adopted on "Restart All" (#1809's cold-start gate never fired). - lifecycle.restart_agent_internal(): the canonical stop→cold-start helper (explicit stop is load-bearing for the #1809 image predicate; future home of #1817's per-agent start lock) - restart_fleet routes through it; per-agent recreated/recreate_reason surfaced via explicit allowlist copy, summary.recreated count - skips ephemeral ghosts (config predicates aren't ephemeral-gated — a recreate would destroy a volume-less ghost workspace, ent#69) - reject_agent_principal beside assert_admin (the endpoint now replaces containers — Invariant #8 escalation rule, #1816 precedent) - single-flight Redis SETNX lock ops:fleet_restart (409 on contention, own-lease refresh, compare-and-delete release, fail-open) — guards the client-timeout→retry overlap (#799/#1817 wedge class) - partial-safe fleet_restart audit entry with a per-agent recreate map (restores the entry dropped in 0ec3a7fc); sync cleanup ordered before the awaited audit so a shutdown CancelledError can't leave the lock held - actionable containerless-recovery errors (#1559), context-stats cache invalidation, 16 mocked unit tests, flow/architecture docs, learnings, CSO diff report (PASS) Fixes #1860 Co-Authored-By: Claude Fable 5 --- docs/memory/architecture.md | 3 +- docs/memory/feature-flows/agent-lifecycle.md | 19 + .../feature-flows/internal-system-agent.md | 2 +- docs/memory/feature-flows/mobile-admin-pwa.md | 1 + docs/memory/learnings.md | 8 + ...so-diff-2026-07-31-1860-fleet-restart.json | 65 +++ .../cso-diff-2026-07-31-1860-fleet-restart.md | 74 ++++ src/backend/routers/ops.py | 274 +++++++++--- .../services/agent_service/lifecycle.py | 22 + tests/registry.json | 14 +- .../unit/test_1860_fleet_restart_adoption.py | 395 ++++++++++++++++++ 11 files changed, 810 insertions(+), 67 deletions(-) create mode 100644 docs/security-reports/cso-diff-2026-07-31-1860-fleet-restart.json create mode 100644 docs/security-reports/cso-diff-2026-07-31-1860-fleet-restart.md create mode 100644 tests/unit/test_1860_fleet_restart_adoption.py diff --git a/docs/memory/architecture.md b/docs/memory/architecture.md index 5e9d211aa1..993cf91ae8 100644 --- a/docs/memory/architecture.md +++ b/docs/memory/architecture.md @@ -317,7 +317,7 @@ Vector 0.43.1 (`timberio/vector:0.43.1-alpine`). Captures all container stdout/s The agent server also runs two loops: the 15-min git `auto_sync` heartbeat (see [Git Sync Health](#git-sync-health-389390)) and the 5s liveness heartbeat (see [Heartbeat Liveness](#heartbeat-liveness-reliability-004-307)). -**Durable subscription-token override (#1089):** `startup.sh` exports `CLAUDE_CODE_OAUTH_TOKEN` from `/var/lib/trinity/oauth-token` (when present, non-empty) **before** launching the agent server, so a token rotated via hot-reload survives a plain stop+start (a fleet restart via `routers/ops.py` does a raw `container_stop`+`container_start` that bypasses `start_agent_internal` and would otherwise revert to the baked `Config.Env` token). The path is deliberately on the writable layer, **not** under the persisted `/home/developer` volume: it survives `stop`→`start` (same container) but is wiped on recreate (fresh layer), so a DB-driven recreate cleanly re-bakes `Config.Env` from the DB and the stale override is gone — self-reconciling, no marker logic. Dir created+chowned to UID 1000 in the base-image Dockerfile. +**Durable subscription-token override (#1089):** `startup.sh` exports `CLAUDE_CODE_OAUTH_TOKEN` from `/var/lib/trinity/oauth-token` (when present, non-empty) **before** launching the agent server, so a token rotated via hot-reload survives any plain stop+start of the same container (historically that included `routers/ops.py`'s fleet restart, which bypassed `start_agent_internal` entirely; since #1860 a fleet restart routes through `lifecycle.restart_agent_internal` — a no-drift agent keeps its container and the override survives, a drifted agent is recreated and cleanly re-bakes `Config.Env` from the DB). The path is deliberately on the writable layer, **not** under the persisted `/home/developer` volume: it survives `stop`→`start` (same container) but is wiped on recreate (fresh layer), so a DB-driven recreate cleanly re-bakes `Config.Env` from the DB and the stale override is gone — self-reconciling, no marker logic. Dir created+chowned to UID 1000 in the base-image Dockerfile. **Template-supplied pre-check** (SCHED-COND-001, #454): if the template ships an executable `~/.trinity/pre-check`, the backend's internal endpoint `POST /api/internal/agents/{name}/pre-check` runs it via `docker exec` before a cron-triggered chat. Language-agnostic — interpreter selected by shebang. The hook's stdout becomes the chat message; empty stdout + exit 0 records a skipped execution (Claude never invoked). Uses the same `execute_command_in_container` primitive as `git_service.py`, `ssh_service.py`, and the agent terminal — no agent-server HTTP endpoint. @@ -1926,6 +1926,7 @@ CREATE INDEX idx_agent_reports_created ON agent_reports(created_at); -- retenti - **Compatibility fix lock**: `compat_fix:{name}` (SET NX, 30s TTL) serialises the per-agent gitignore auto-fix read-modify-write (#668). - **Operator-queue sync keys** (#1632): `opqueue:leader` (SET NX, TTL `max(3×poll-interval, 30s)` floor — the single-syncing-worker lease, mirror `monitoring:leader`) and the create rate-limit windows `ratelimit:operator_queue_create:{agent}` + `ratelimit:operator_queue_create:_fleet` (ZSET, via `rate_limiter.check`, fail-open). Not `agent:*`-named, so the #1560 name-keyed registry doesn't apply; both fail open. - **Ephemeral-agent keys** (trinity-enterprise#69): `ephemeral:quota:{owner_id}` (owner-keyed atomic ghost-quota counter — deliberately not `agent:*`, so the #1560 name-keyed registry doesn't apply) and `ephemeral:discard:{name}` (SETNX+TTL discard lock). Both fail-open. +- **Fleet-restart lock** (#1860): `ops:fleet_restart` — SETNX + 900s TTL single-flight guard for `POST /api/ops/fleet/restart` (409 on contention); the live loop refreshes its own lease per agent, release is compare-and-delete. Not `agent:*`-named (fleet-scoped, #1560 registry doesn't apply); fail-open when Redis is down. --- diff --git a/docs/memory/feature-flows/agent-lifecycle.md b/docs/memory/feature-flows/agent-lifecycle.md index bfb4092571..cc948ae49a 100644 --- a/docs/memory/feature-flows/agent-lifecycle.md +++ b/docs/memory/feature-flows/agent-lifecycle.md @@ -531,6 +531,22 @@ extracted-but-never-read variable, which is precisely why `unless-stopped` silen recreate. Read null-safely as `(old_host_config.get("RestartPolicy") or {}).get("Name")`: the key can exist with a `null` value, and `.get` on `None` would abort the recreate. +**Canonical cold restart — `restart_agent_internal` (#1860)** (`src/backend/services/agent_service/lifecycle.py:469-488`, directly after `start_agent_internal` :257): + +```python +async def restart_agent_internal(agent_name: str, *, stop_timeout: int = 30) -> dict +``` + +Explicit `container_stop(container, timeout=stop_timeout)` (:487), then the full `start_agent_internal` path (:488); returns its result dict unchanged. The explicit stop is **load-bearing**: the #1809 image-drift predicate runs only on a COLD start (`not was_already_running`), and starting a running agent is a deliberate idempotent no-op — so a restart that should adopt a rebuilt base image must stop first. A missing container falls through to `start_agent_internal`, which rebuilds it from persisted config (#1559, live ownership row) or 404s. This helper is the one home for the stop→start shape and the future home of #1817's per-agent start lock; `routers/system_agent.py`, `routers/systems.py`, and `services/subscription_auto_switch.py` still carry inline copies (consolidation deferred to #1817). + +**First consumer — `restart_fleet`** (`POST /api/ops/fleet/restart`, `src/backend/routers/ops.py:234-472`): each agent routes `container is None` guard → `restart_agent_internal(agent_name)` (:363-365) instead of the old raw `container_stop`+`container_start`, so a fleet restart applies pending config drift and adopts a rebuilt base image exactly like a manual stop → start. Semantics: + +- **Skips**: system agent (:302-312) AND ephemeral ghosts (:314-330) — the config-drift predicates are not ephemeral-gated, so a cold start could recreate a drifted ghost and destroy its volume-less workspace (ent#69 "ghosts never recreate"); GC owns ghost lifecycle. Pre-existing `filter_status` / `system_prefix` / not-running skips unchanged. +- **Principals**: `reject_agent_principal` beside `assert_admin` (:256-260) — the endpoint now *replaces* containers, and escalating a handler's destructiveness re-prices every principal that could already reach it (Invariant #8, the #1816 rule). +- **Single-flight**: Redis SETNX lock `ops:fleet_restart` (:42-43, :262-277) — a second caller gets 409 `fleet_restart_in_progress`; 900s TTL bounds a crashed holder, the live loop refreshes its own lease each iteration (:296-300), release is compare-and-delete of the holder's own token (:425-432); fail-open when Redis is down. Guards the client-timeout-retry race (a retried POST interleaving stop/start with the still-running first loop — the #799/#1817 wedge class). +- **Result surfacing**: per-agent entries explicitly copy `recreated` / `recreate_reason` / `credentials_injection` / `skills_injection` (:369-377) — the #1809 allowlist learning: this router builds its own result dicts, service fields do NOT flow through on their own — and `summary.recreated` counts replacements (:469). A failed recreate that leaves NO container gets an actionable error naming `POST /api/agents/{name}/start` (#1559) as the recovery path (:404-408). +- **Audit as durable record**: a partial-safe `fleet_restart` entry (`AuditEventType.SYSTEM`) is written in `finally` (:437-460) with totals + a per-agent `recreated: {name: reason}` map (:453) — the sequential loop can outlast proxy timeouts (nginx 60s / Cloudflare ~100s), so a client 504/524 does NOT mean the restart failed; the outcome lands here. `invalidate_context_stats_cache()` (:424) and the lock release run before the awaited audit call, so a shutdown `CancelledError` at the await can't leave the lock held for a full TTL. + **Authentication Model** (Updated 2026-03-03 for SUB-002): Claude Code checks credentials in **priority order** (highest first): @@ -1028,6 +1044,8 @@ await log_audit_event( - [ ] Delete any remaining test agents - [ ] `docker ps -a | grep test-` - verify no orphans +**Unit tests (#1860)**: `tests/unit/test_1860_fleet_restart_adoption.py` — 16 mocked tests for the fleet-restart path (adoption surfaced per agent, failure isolation, system/ephemeral skips, agent-principal 403, single-flight 409, audit `recreated` map, fail-open lock paths). + --- **Last Updated**: 2026-03-26 @@ -1040,6 +1058,7 @@ await log_audit_event( | Date | Changes | |------|---------| +| 2026-07-31 | **#1860 — fleet restart adopts a rebuilt base image**: new canonical cold-restart helper `restart_agent_internal(agent_name, *, stop_timeout=30)` (`lifecycle.py:469-488` — explicit stop is load-bearing so the #1809 cold-start-only drift predicate can fire); `POST /api/ops/fleet/restart` routes each agent through it instead of raw stop+start, skips ephemeral ghosts (ent#69), adds `reject_agent_principal`, a single-flight `ops:fleet_restart` SETNX lock (409, 900s TTL, compare-and-delete release), explicit `recreated`/`recreate_reason`/injection field copy + `summary.recreated`, and a partial-safe `fleet_restart` audit entry written in `finally` with a per-agent `recreated: {name: reason}` map. Inline stop→start copies in `system_agent.py`/`systems.py`/`subscription_auto_switch.py` consolidate under #1817. See "Canonical cold restart" above. | | 2026-07-28 | **#1816 — base-image adoption for `trinity-system`**: `check_base_image_matches` split into a 3-state `check_base_image_state` core + an unchanged boolean wrapper (`state != "drift"`); `check_full_capabilities_match` made system-aware and `recreate_container_with_updated_config` gained a keyword-only `full_capabilities` override behind one shared `helpers.is_system_agent_name()`; `_provision_folders_and_run_agent_container` gained a keyword-only `restart_policy` and the recreate now carries `unless-stopped` forward (it was silently dropped); `start_agent_internal` gained the structural AC2 gate (`is_system AND was_already_running` ⇒ `recreate_deferred="system_agent_running"`, surfaced through `routers/agents.py`). See [internal-system-agent.md](internal-system-agent.md) → Base-image adoption. | | 2026-07-18 | **#1484 — `create_agent_internal` decomposition (no behavior change)**: the CC-187 / 945-SLOC monolith was cut into a thin orchestrator + fenced private phase-helpers (each CC<20 / <100 SLOC), reading a single orchestrator-populated `_RollbackHandles` dataclass; the `if docker_client: try/except/else` and CC-trivial gates stay inline. Signature/callers unchanged. Characterization-tests-first (`tests/unit/test_1484_create_agent_characterization.py`, 40 tests) pin the byte-identical contract. See `architecture.md` (Services → `agent_service/crud.py`, Network Topology `_create_agent_container`). The numbered "Business Logic" line references above are now stale (helper-relocated). | | 2026-03-26 | **Line number refresh + Trinity injection removal**: Updated all line numbers across agents.py (647 lines), lifecycle.py (403 lines), crud.py (552 lines), helpers.py (467 lines), terminal.py (320 lines). Removed all `inject_trinity_meta_prompt()` references and AgentClient injection code (Issue #136). Updated startup injection order to: Credentials, Skills, Read-Only Hooks. Added documentation for bulk endpoints (context-stats :157, execution-stats :163, autonomy-status :232, slots :240), queue endpoints (queue :537, clear :546, release :555), and activity endpoints (activities :568, timeline :594). Updated delete flow for EVT-001 event subscriptions (:381) and AVATAR-002 emotion image cleanup (:410-425). | diff --git a/docs/memory/feature-flows/internal-system-agent.md b/docs/memory/feature-flows/internal-system-agent.md index b69f8b53c4..e6e4eb20ea 100644 --- a/docs/memory/feature-flows/internal-system-agent.md +++ b/docs/memory/feature-flows/internal-system-agent.md @@ -281,7 +281,7 @@ The dedicated `SystemAgent.vue` has been removed. The system agent now uses the |----------|--------|-------------| | `/api/ops/fleet/status` | GET | All agents with status, context, activity | | `/api/ops/fleet/health` | GET | Health summary with critical/warning issues | -| `/api/ops/fleet/restart` | POST | Restart all/filtered agents | +| `/api/ops/fleet/restart` | POST | Restart all/filtered running agents through the canonical stop→`start_agent_internal` path — applies pending config drift and adopts a rebuilt base image (#1860). Skips system + ephemeral agents; agent-scoped keys 403 (`reject_agent_principal` — system-scoped keys unaffected); 409 while another fleet restart is in flight; outcome durably recorded in the audit log (`fleet_restart`) even if the client times out | | `/api/ops/fleet/stop` | POST | Stop all/filtered agents | ### Schedule Control diff --git a/docs/memory/feature-flows/mobile-admin-pwa.md b/docs/memory/feature-flows/mobile-admin-pwa.md index 6cf7994ac7..3a0a481d83 100644 --- a/docs/memory/feature-flows/mobile-admin-pwa.md +++ b/docs/memory/feature-flows/mobile-admin-pwa.md @@ -39,6 +39,7 @@ Browser (/m) - **Manual PWA** (no vite-plugin-pwa): Service worker + manifest injected dynamically in `onMounted`, cleaned up in `onUnmounted`. Follows Sparky PWA reference pattern - **Dark-only**: Forces `dark` class on ``, OLED-friendly `#111827` background - **401 handling**: `main.js` axios interceptor skips redirect to `/login` when on `/m` +- **Restart All is a real upgrade path (#1860)**: `POST /api/ops/fleet/restart` routes each running agent through the canonical stop→`start_agent_internal` path, so it applies pending config drift and adopts a rebuilt base image (container ids change). A slow fleet can outlast the proxy timeout — the loop keeps running server-side and the outcome lands in the audit log (`fleet_restart`); a second tap while one is in flight gets 409 `fleet_restart_in_progress` ## Files diff --git a/docs/memory/learnings.md b/docs/memory/learnings.md index 0a04df8c99..b14616df6e 100644 --- a/docs/memory/learnings.md +++ b/docs/memory/learnings.md @@ -188,3 +188,11 @@ plan or review. `/autoplan` reads this before planning; write for that reader. ## 2026-07-30 — pitfall — Count the parallel surfaces yourself, and put the guard where CI actually looks **Context**: trinity#1880. Five canary invariants shipped with no Slack-alert metadata, so a green→red page rendered `🚨 canary G-04 G-04 (critical): G-04 fired 1 violation(s).` — the critical credential-leak check with no name, no evidence, and the id stuttered. The issue diagnosed it correctly as "two hand-maintained lists with nothing tying them together" and made the guard test the headline AC. Three things in that framing did not survive contact with the code. (1) There are **four** id-keyed surfaces in `services/canary_alerts.py`, not two — the `_INVARIANT_NAMES`/`_INVARIANT_RUNBOOKS` dicts plus the `if invariant_id == …` chains in `_render_message` and `_render_forensic`, all four independently covering the identical 10 of 15 ids. The two the issue missed are the ones carrying per-row *evidence*, so satisfying the written ACs would have given G-04 a name, a runbook, a count, and no rows. (2) The guard's natural home — `tests/test_canary_invariants.py`, 3,726 lines, the only file exercising this code — **runs in no CI workflow**: every gating job is `cd tests && pytest unit/`, and 111 root-level test files are collected by nothing. A guard there would never have gone red. (3) The `_INVARIANT_NAMES` comment said names are "paired with the catalog at `docs/testing/orchestration-invariant-catalog.md`", and the catalog is wrong or over-claiming on 4 of the 5: catalog `E-06` is the unimplemented #129 check while registry `E-06` is "no overdue `next_run_at`" (#1472, semantically catalog `SCH-03`); `G-04`'s catalog title promises "/ logs" coverage that does not exist. Sourcing a name from it puts a confidently wrong label on a real alert, which is worse than the bare-id fallback. Git archaeology showed the mechanism cleanly: Phase 2/3 (#882) populated all four surfaces, Phase 4 (#1497) and #1472 populated none — the manual pairing worked once, then failed twice running. **Lesson**: (1) When an issue says "N hand-maintained lists", **count them with an AST sweep before believing N** — grep the file for every structure keyed by the same identifier, including `if x == "..."` chains, which no dict-shaped search finds. The surfaces the issue omits are disproportionately the ones with no obvious shape, and here they were also the load-bearing ones. (2) **A guard must run where CI runs.** Before writing one, read the workflow's actual pytest invocation — a test directory that no job names is decoration, and placing a parity guard beside the code it guards is the intuitive wrong answer. (`tests/unit/test_canary_env_prod_parity.py` had already written this rule into its docstring one PR earlier; it was not in this ledger, so it did not travel.) (3) **Validate an anti-stub heuristic against the existing corpus before shipping it.** "Runbook must contain a backticked identifier" looked like a clean proxy for "points somewhere concrete" and fired on two *correct* entries (E-02 names a bug class; L-03 defers to its own forensic block, which lists tables dynamically). A heuristic that flags good content is the one a future contributor deletes — taking the honest length floor with it. Assert the regression's signature (a stub is *short*), not a proxy for quality. (4) A **fallback can be a security property**. `_render_forensic`'s `return None` and `_render_message`'s count-only string read like leftovers, but they are why an un-rendered invariant cannot leak; a parity gate creates standing pressure toward the one-line generic `observed_state.items()` dumper that satisfies it forever, which is invisible in review (it *removes* a special case) and relocates the trust boundary away from the checks that scrub at source. Pin such a fallback with a negative test before adding the gate that pressures it. + +## 2026-07-31 — pitfall — The lifecycle's ephemeral guards cover image-drift + credential injection ONLY; any new cold-start caller must skip ghosts itself +**Context**: #1860 fleet restart. The plan assumed "start_agent_internal already guards ephemeral" and both independent reviewers refuted it: the 8 config-drift predicates (lifecycle.py `needs_recreation`) have no ephemeral gate, so a cold start of a config-drifted ghost recreates it — destroying its volume-less workspace mid-budget (ent#69 "ghosts never recreate"). +**Lesson**: a bulk/new caller that cold-starts agents through `start_agent_internal` (or `restart_agent_internal`) must itself exclude `is_ephemeral` rows, the way heartbeat watch and fleet health already do. Do not infer "the lifecycle guards ghosts" from the two guards that exist. + +## 2026-07-31 — pitfall — In a `finally` that both awaits and cleans up, do sync cleanup FIRST: `except Exception` does not catch CancelledError, so an awaited call mid-finally skips everything after it +**Context**: #1860 restart_fleet's finally block ordered audit-await → cache-invalidate → lock-release; a backend deploy mid-loop cancels the handler, the audit await re-raises CancelledError (a BaseException), and the lock stayed held for a full TTL after every deploy. +**Lesson**: order shutdown-sensitive `finally` blocks sync-cleanup-first, awaited-best-effort-last, and remember `except Exception` around an `await` is not cancellation-safe (#1804's shutdown handlers are the same class). diff --git a/docs/security-reports/cso-diff-2026-07-31-1860-fleet-restart.json b/docs/security-reports/cso-diff-2026-07-31-1860-fleet-restart.json new file mode 100644 index 0000000000..8fc858182d --- /dev/null +++ b/docs/security-reports/cso-diff-2026-07-31-1860-fleet-restart.json @@ -0,0 +1,65 @@ +{ + "version": "1.1", + "date": "2026-07-31", + "mode": "daily", + "scope": "diff", + "branch": "feature/1860-fleet-restart-adoption", + "base": "dev@5b289996", + "issue": "abilityai/trinity#1860", + "phases_run": [ + "P0", + "P1", + "P2-diff", + "P5-lock", + "P6", + "P7", + "P9-A01/A03/A09", + "P12", + "P13", + "P14" + ], + "attack_surface": { + "new_endpoints": 0, + "hardened_endpoints": [ + "POST /api/ops/fleet/restart" + ], + "new_auth_gates": [ + "reject_agent_principal (ops.py:260)" + ], + "new_redis_keys": [ + "ops:fleet_restart (SETNX single-flight, backend ACL user)" + ], + "new_inputs": 0 + }, + "findings": [], + "filter_stats": { + "raw_candidates": 4, + "discarded_by_rule": { + "fail-open-lock (platform convention)": 1, + "error-text-to-admin (existing practice)": 1, + "dos-excluded (sequential duration)": 1, + "ttl-extension-benign (expire w/o token)": 1 + } + }, + "totals": { + "critical": 0, + "high": 0, + "medium": 0, + "low": 0 + }, + "verification": { + "live": [ + "agent-key 403", + "concurrent 409", + "audit row content", + "image adoption 3/3", + "no-drift idempotence", + "config-drift single-agent" + ], + "unit": "16/16 tests/unit/test_1860_fleet_restart_adoption.py" + }, + "trend": { + "note": "first diff-scope report for this branch; verdict PASS, net hardening" + }, + "verdict": "PASS" +} \ No newline at end of file diff --git a/docs/security-reports/cso-diff-2026-07-31-1860-fleet-restart.md b/docs/security-reports/cso-diff-2026-07-31-1860-fleet-restart.md new file mode 100644 index 0000000000..cf3492dc36 --- /dev/null +++ b/docs/security-reports/cso-diff-2026-07-31-1860-fleet-restart.md @@ -0,0 +1,74 @@ +# CSO Diff Audit — #1860 fleet-restart adoption (2026-07-31) + +**Mode**: `--diff` (daily gate, 8/10) · **Branch**: `feature/1860-fleet-restart-adoption` vs `dev@5b289996` +**Scope**: `src/backend/routers/ops.py`, `src/backend/services/agent_service/lifecycle.py`, +`tests/unit/test_1860_fleet_restart_adoption.py`, 3 doc files, `tests/registry.json` + +## Architecture delta (Phase 0) + +`POST /api/ops/fleet/restart` stops routing each agent through raw `container_stop`+`container_start` +and instead delegates to the new `lifecycle.restart_agent_internal` (explicit stop → full +`start_agent_internal` path). No new endpoints, no new input paths (same two query params), no new +network flows. One new Redis key (`ops:fleet_restart`, backend-side, SETNX single-flight lock). +Trust-boundary movement is **inward**: the endpoint's effective privilege escalated from +restart-in-place to container replacement, and the diff re-prices principals accordingly. + +## Attack-surface census (Phase 1, diff) + +| Category | Delta | +|---|---| +| Endpoints | 0 new; 1 hardened (`/api/ops/fleet/restart`) | +| Auth gates | +`reject_agent_principal` (ops.py:260, beside `assert_admin` ops.py:256) | +| Concurrency | +single-flight SETNX lock, 409 `fleet_restart_in_progress` (ops.py:270-273) | +| Redis keys | +`ops:fleet_restart` via `get_breaker_redis` (backend ACL user, data-ops only) | +| New inputs / uploads / WS / integrations | none | + +## Findings + +**None at the 8/10 gate.** (Zero findings also below the gate worth an appendix.) + +## What was checked (clean categories, with evidence) + +- **Secrets archaeology (P2, diff)**: pattern scan of the working-tree diff + new test file — + clean. No enterprise paid-feature disclosure in the 3 doc edits (mechanism-level #1860 text only). +- **A01 access control**: gate is *strengthened* — `assert_admin(current_user)` (ops.py:256) + + `reject_agent_principal(current_user)` (ops.py:260), the Invariant #8 escalation rule for + endpoints that replace containers (#1816 precedent; trinity-ops-agent#232 class). Live-verified: + agent-scoped key → 403 `"This operation is human-only"`. System-scoped principals unaffected + (`User.agent_name` set only for scope="agent") — the documented system-agent use keeps working. + No enumeration surface (no agent-name path param; fleet op, admin-only). +- **Agent-key self-boundaries (P6)**: n/a in reverse — the endpoint now rejects ALL agent keys. +- **A03 injection**: no SQL, no subprocess, no template rendering in the diff. Ephemeral check uses + the existing parameterized accessor (`db.get_agent_ephemeral_info`). +- **Concurrency/lock correctness (P5)**: SETNX+TTL with own-lease refresh; release is + compare-and-delete (ops.py:427) so a second runner's lock can never be deleted by the first; + worst-case anomaly of the unconditional per-iteration `expire` is extending an *active* lock's + TTL — protection-extending, not protection-breaking. Fail-open on Redis down matches the + platform's uniform breaker/limiter convention (fail-closed would make a Redis outage block all + fleet ops). Live-verified: concurrent second call → 409. +- **Credential exposure (P2/P7)**: per-agent results carry injection *status strings* only + (`"skipped"`/`"success"`...), never the service result dicts; audit `details` carries counts, + agent names, and platform-enum recreate reasons (`recreated_map` ops.py:286/380, + `failed_agents` ops.py:287) — no free-form agent-authored text, no secrets. Exception text in + per-agent `error` fields is served only to the admin caller (same practice as the single-agent + stop endpoint's 500 detail). +- **A09 logging/monitoring**: improved — restores the `fleet_restart` audit entry dropped in + `0ec3a7fc`, now partial-completion-safe with per-agent recreate traceability. +- **LLM/AI surface (P7)**: none touched — no prompts, no MCP tool changes, no advertised + descriptions, no agent-reachable surface (lock key lives on the platform-side Redis the agent + network cannot route to, #589). +- **CI/CD, Docker, dependencies**: not in diff; no workflow/compose/requirements changes. + +## Residual risks (accepted, documented — not findings) + +1. **Lock fail-open when Redis is down** — overlapping fleet restarts become possible again in + that degraded state; consistent with every other Trinity breaker/limiter and documented in code. +2. **Sequential loop duration** — an operator-experience concern (504 with server-side + completion), mitigated by the audit-as-durable-record + docstring + chunking params; DoS-class + concerns are per skill policy out of scope. + +## Verdict + +**PASS** — the diff is a net security improvement: one endpoint's principal surface narrowed +(agent keys excluded), a concurrency wedge closed (single-flight lock), and an audit gap restored. +No findings to remediate. diff --git a/src/backend/routers/ops.py b/src/backend/routers/ops.py index 0a7600e7e3..ef44573ebb 100644 --- a/src/backend/routers/ops.py +++ b/src/backend/routers/ops.py @@ -12,6 +12,7 @@ import os import logging import concurrent.futures +import uuid from datetime import datetime, timedelta from typing import Optional, List from fastapi import APIRouter, Depends, HTTPException, Request, Query @@ -19,17 +20,28 @@ from models import User from database import db -from dependencies import get_current_user, assert_admin +from dependencies import get_current_user, assert_admin, reject_agent_principal from services.docker_service import get_agent_container, docker_client, list_all_agents_fast -from services.docker_utils import container_stop, container_start +from services.docker_utils import container_stop from services.agent_client import get_agent_client +from services.agent_service.lifecycle import restart_agent_internal +from services.agent_service.stats import invalidate_context_stats_cache from db.agents import SYSTEM_AGENT_NAME +from redis_breaker_util import get_breaker_redis from utils.helpers import utc_now_iso from services.platform_audit_service import platform_audit_service, AuditEventType router = APIRouter(prefix="/api/ops", tags=["operations"]) logger = logging.getLogger(__name__) +# #1860: one fleet restart at a time. A client timeout (nginx 60s / Cloudflare +# ~100s) invites a retry while the first loop is still running server-side, and +# two loops interleaving stop/start on the same agent is the #799/#1817 +# stop-racing-start wedge class. TTL bounds a crashed holder; the live loop +# refreshes its own lease each iteration so a long fleet never loses the lock. +_FLEET_RESTART_LOCK_KEY = "ops:fleet_restart" +_FLEET_RESTART_LOCK_TTL_SECONDS = 900 + # ============================================================================ # Fleet Status & Health @@ -229,9 +241,40 @@ async def restart_fleet( """ Restart all agents in the fleet. - Admin-only. Excludes the system agent. + Admin-only (human principals only). Excludes the system agent and + ephemeral ghosts. Each agent is stopped, then started through the + canonical lifecycle path (``restart_agent_internal``), so a fleet restart + applies pending config drift and adopts a rebuilt base image + (#1809/#1860) exactly like a manual stop → start. + + The loop is sequential; a large fleet can outlast proxy timeouts. A + client timeout (504/524) does NOT mean the restart failed — the loop + keeps running server-side and the outcome lands in the audit log + (``fleet_restart`` entry). Use ``system_prefix`` / ``filter_status`` to + restart in chunks. 409 while another fleet restart is in flight. """ assert_admin(current_user) + # #1860: this endpoint replaces containers, not just restarts them — + # operator-scale destructiveness re-prices principals (Invariant #8, the + # #1816 escalation rule). No-op for human/system principals. + reject_agent_principal(current_user) + + # Single-flight lock (SETNX + TTL, fail-open when Redis is down). + lock_client = get_breaker_redis() + lock_token = uuid.uuid4().hex + lock_held = False + if lock_client is not None: + try: + lock_held = bool(lock_client.set( + _FLEET_RESTART_LOCK_KEY, lock_token, + nx=True, ex=_FLEET_RESTART_LOCK_TTL_SECONDS, + )) + if not lock_held: + raise HTTPException(status_code=409, detail="fleet_restart_in_progress") + except HTTPException: + raise + except Exception: + lock_client = None # fail-open: proceed unlocked agents = list_all_agents_fast() @@ -239,78 +282,182 @@ async def restart_fleet( successes = 0 failures = 0 skipped = 0 + recreated_count = 0 + recreated_map = {} + failed_agents = [] - for agent in agents: - agent_name = agent.name - status = agent.status - - # Skip system agent - owner = db.get_agent_owner(agent_name) - is_system = owner.get("is_system", False) if owner else False - if is_system: - results.append({ - "agent": agent_name, - "result": "skipped", - "reason": "system agent" - }) - skipped += 1 - continue + try: + for agent in agents: + agent_name = agent.name + status = agent.status - # Apply filters - if filter_status and status != filter_status: - results.append({ - "agent": agent_name, - "result": "skipped", - "reason": f"status is {status}, not {filter_status}" - }) - skipped += 1 - continue + # Refresh our own lease so a long fleet never loses the lock + # mid-run while a crashed holder still expires within the TTL. + if lock_held and lock_client is not None: + try: + lock_client.expire(_FLEET_RESTART_LOCK_KEY, _FLEET_RESTART_LOCK_TTL_SECONDS) + except Exception: + pass + + # Skip system agent + owner = db.get_agent_owner(agent_name) + is_system = owner.get("is_system", False) if owner else False + if is_system: + results.append({ + "agent": agent_name, + "result": "skipped", + "reason": "system agent" + }) + skipped += 1 + continue + + # Skip ephemeral ghosts (#1860): the config-drift predicates are + # not ephemeral-gated, so a cold start could recreate a drifted + # ghost — destroying its volume-less workspace mid-budget + # (trinity-enterprise#69 "ghosts never recreate"). Restarting a + # disposable ghost has no adoption upside; GC owns its lifecycle. + try: + eph = db.get_agent_ephemeral_info(agent_name) + except Exception: + eph = None + if isinstance(eph, dict) and eph.get("is_ephemeral"): + results.append({ + "agent": agent_name, + "result": "skipped", + "reason": "ephemeral" + }) + skipped += 1 + continue - if system_prefix and not agent_name.startswith(system_prefix + "-"): - results.append({ - "agent": agent_name, - "result": "skipped", - "reason": f"doesn't match prefix {system_prefix}" - }) - skipped += 1 - continue + # Apply filters + if filter_status and status != filter_status: + results.append({ + "agent": agent_name, + "result": "skipped", + "reason": f"status is {status}, not {filter_status}" + }) + skipped += 1 + continue - # Skip stopped agents - if status != "running": - results.append({ - "agent": agent_name, - "result": "skipped", - "reason": "not running" - }) - skipped += 1 - continue + if system_prefix and not agent_name.startswith(system_prefix + "-"): + results.append({ + "agent": agent_name, + "result": "skipped", + "reason": f"doesn't match prefix {system_prefix}" + }) + skipped += 1 + continue - # Restart the agent - try: - container = get_agent_container(agent_name) - if container: - await container_stop(container, timeout=30) - await container_start(container) + # Skip stopped agents + if status != "running": results.append({ "agent": agent_name, - "result": "success", - "previous_status": status + "result": "skipped", + "reason": "not running" }) - successes += 1 - else: + skipped += 1 + continue + + # Restart the agent through the canonical lifecycle path. + try: + container = get_agent_container(agent_name) + if container: + start_result = await restart_agent_internal(agent_name) + # Explicit field copy — this router builds its own result + # dicts, so service fields do NOT flow through on their + # own (#1809's allowlist learning, routers/agents.py). + entry = { + "agent": agent_name, + "result": "success", + "previous_status": status, + "recreated": bool(start_result.get("recreated")), + "recreate_reason": start_result.get("recreate_reason"), + "credentials_injection": start_result.get("credentials_injection"), + "skills_injection": start_result.get("skills_injection"), + } + if entry["recreated"]: + recreated_count += 1 + recreated_map[agent_name] = entry["recreate_reason"] or "unknown" + results.append(entry) + successes += 1 + logger.info( + f"Fleet restart: {agent_name} restarted " + f"(recreated={entry['recreated']}, reason={entry['recreate_reason']})" + ) + else: + results.append({ + "agent": agent_name, + "result": "failed", + "error": "container not found" + }) + failed_agents.append(agent_name) + failures += 1 + except Exception as e: + if isinstance(e, HTTPException): + error_text = f"{e.status_code}: {e.detail}" + else: + error_text = str(e) or e.__class__.__name__ + # A recreate that failed after removing the old container + # leaves the agent with NO container — it vanishes from fleet + # listings (Docker-as-truth). Name the recovery path (#1559). + try: + if get_agent_container(agent_name) is None: + error_text += ( + f" (no container present — recover via " + f"POST /api/agents/{agent_name}/start)" + ) + except Exception: + pass results.append({ "agent": agent_name, "result": "failed", - "error": "container not found" + "error": error_text }) + failed_agents.append(agent_name) failures += 1 - except Exception as e: - results.append({ - "agent": agent_name, - "result": "failed", - "error": str(e) - }) - failures += 1 + logger.warning(f"Fleet restart: {agent_name} failed: {error_text}") + finally: + # Sync cleanup FIRST, awaited audit LAST: a CancelledError raised at + # the audit await (backend shutdown mid-loop) is a BaseException the + # try below doesn't catch — anything after it in this block would be + # skipped, leaving the lock held for a full TTL after a deploy. + invalidate_context_stats_cache() + if lock_held and lock_client is not None: + try: + # Release only our own lock (compare-and-delete). + current = lock_client.get(_FLEET_RESTART_LOCK_KEY) + if current == lock_token.encode() or current == lock_token: + lock_client.delete(_FLEET_RESTART_LOCK_KEY) + except Exception: + pass + # The audit row is the durable record — a client that timed out finds + # the outcome (incl. partial completion) here. Never fails the op. + # Restores the fleet_restart entry dropped in 0ec3a7fc; SYSTEM event + # type matches this router's emergency_stop precedent. + try: + await platform_audit_service.log( + event_type=AuditEventType.SYSTEM, + event_action="fleet_restart", + source="api", + actor_user=current_user, + actor_ip=request.client.host if request.client else None, + endpoint=str(request.url.path), + request_id=getattr(request.state, "request_id", None), + details={ + "total": len(agents), + "successes": successes, + "failures": failures, + "skipped": skipped, + # {agent: reason} for recreated agents only — preserves + # #1809's changed-container-id traceability at fleet scale. + "recreated": recreated_map or None, + "failed_agents": failed_agents or None, + "filter_status": filter_status, + "system_prefix": system_prefix, + }, + ) + except Exception: + logger.warning("Fleet restart: audit write failed", exc_info=True) return { "timestamp": utc_now_iso(), @@ -318,7 +465,8 @@ async def restart_fleet( "total": len(agents), "successes": successes, "failures": failures, - "skipped": skipped + "skipped": skipped, + "recreated": recreated_count }, "results": results } diff --git a/src/backend/services/agent_service/lifecycle.py b/src/backend/services/agent_service/lifecycle.py index f29f1a5773..099fe676b7 100644 --- a/src/backend/services/agent_service/lifecycle.py +++ b/src/backend/services/agent_service/lifecycle.py @@ -466,6 +466,28 @@ async def start_agent_internal(agent_name: str) -> dict: } +async def restart_agent_internal(agent_name: str, *, stop_timeout: int = 30) -> dict: + """The canonical cold restart: explicit stop, then the full start path (#1860). + + The explicit stop is load-bearing — the #1809 image-drift predicate runs + only on a COLD start (``not was_already_running``), so a restart that + should adopt a rebuilt base image must stop first; ``start_agent_internal`` + on a running agent is a deliberate idempotent no-op. This helper is the one + home for the stop→start shape (routers/system_agent.py, routers/systems.py + and subscription_auto_switch still carry inline copies — consolidating them + is #1817's business, together with the per-agent start lock, which belongs + in here once it exists). + + A missing container falls through to ``start_agent_internal``, which either + rebuilds it from persisted config (#1559, live ownership row) or 404s. + Returns ``start_agent_internal``'s result dict unchanged. + """ + container = get_agent_container(agent_name) + if container: + await container_stop(container, timeout=stop_timeout) + return await start_agent_internal(agent_name) + + async def recreate_container_with_updated_config( agent_name: str, old_container, diff --git a/tests/registry.json b/tests/registry.json index dc2c0bffc4..ed0055359c 100644 --- a/tests/registry.json +++ b/tests/registry.json @@ -866,7 +866,7 @@ "infrastructure", "brain-orb" ], - "description": "Cornelius first-run seeder (ent#107): ensure_seeded first-run gating via durable cornelius_seeded flag (deleted agent not resurrected); fresh-install scoping (existing non-system agents -> skip + converge flag, orb flag untouched); owner-must-exist deferral (pre-setup skip without burning the flag); Docker-unavailable skip; Brain Orb flag defaulted ON existence-guarded (admin OFF preserved); 409-on-exists convergence vs generic-failure retry (flag not burned, never raises); --workers 2 Redis SETNX provision lock (held -> skip, winner provisions+releases with TTL, Redis-down fail-open); _provision builds a github:Abilityai/cornelius create with request=None and source_mode default True (#1656 — the trinity-enterprise#123 tokenless public-clone path is source-mode only); real-DB smoke of db.count_non_system_agents() facade delegation. create_agent_internal/db/redis/docker seams patched; no Docker/backend. ent#124 addition: precomputed freshness verdict \u2014 ensure_seeded(fresh=True) skips the internal count, fresh=False converges the flag without provisioning, fresh=None preserves the legacy count path." + "description": "Cornelius first-run seeder (ent#107): ensure_seeded first-run gating via durable cornelius_seeded flag (deleted agent not resurrected); fresh-install scoping (existing non-system agents -> skip + converge flag, orb flag untouched); owner-must-exist deferral (pre-setup skip without burning the flag); Docker-unavailable skip; Brain Orb flag defaulted ON existence-guarded (admin OFF preserved); 409-on-exists convergence vs generic-failure retry (flag not burned, never raises); --workers 2 Redis SETNX provision lock (held -> skip, winner provisions+releases with TTL, Redis-down fail-open); _provision builds a github:Abilityai/cornelius create with request=None and source_mode default True (#1656 \u2014 the trinity-enterprise#123 tokenless public-clone path is source-mode only); real-DB smoke of db.count_non_system_agents() facade delegation. create_agent_internal/db/redis/docker seams patched; no Docker/backend. ent#124 addition: precomputed freshness verdict \u2014 ensure_seeded(fresh=True) skips the internal count, fresh=False converges the flag without provisioning, fresh=None preserves the legacy count path." }, { "file": "unit/test_1557_autonomy_breaker_decoupled.py", @@ -1246,6 +1246,16 @@ "guard" ], "description": "Static guard: every durable container Trinity creates has a bounded log (#1871). Exists because of the named bug class in docs/memory/learnings.md (2026-07-10) - 'the create path is never one call site'. Scans BOTH docker-py creation entry points - containers_run(...) AND .containers.create(...) - because covering only the first was the guard's own first blind spot (deploy.py already used the second). Rule: pass log_config (durable -> capped), or remove=True (ephemeral -> Docker deletes the log with the container), or be allowlisted with a stated reason (a create/teardown pair like deploy.py's finally: remove(force=True), which a static scan cannot distinguish from a durable create). Uses the AST, not a regex, so a MENTION of containers_run( in a comment or docstring is never a call site (lifecycle.py's own comments do exactly that, and a regex draft flagged them). A 'containers' substring pre-filter keeps the guard interpreter-agnostic - an unrelated file using syntax newer than the running Python (except* under a 3.9 venv) is skipped rather than reported - while staying fail-closed for any unparseable file that could contain a call; the token is deliberately 'containers' and not 'create', which matches half the codebase and re-lights those false positives. Meta-tested on the guard's own machinery: planted violations for both call forms, a capped create, an ephemeral helper, unrelated .create() on volumes/networks/services, a mixed file where only one of two calls is capped, prose/newer-syntax skips, pre-filter-covers-every-call-form, and allowlisted-paths-still-exist - so neither the scan nor its exemptions can rot into a no-op." + }, + { + "file": "unit/test_1860_fleet_restart_adoption.py", + "feature": "#1860", + "added": "2026-07-31", + "categories": [ + "backend", + "unit", + "ops" + ] } ] -} +} \ No newline at end of file diff --git a/tests/unit/test_1860_fleet_restart_adoption.py b/tests/unit/test_1860_fleet_restart_adoption.py new file mode 100644 index 0000000000..5ffb6425b3 --- /dev/null +++ b/tests/unit/test_1860_fleet_restart_adoption.py @@ -0,0 +1,395 @@ +"""#1860 — fleet restart routes through the canonical lifecycle path. + +`POST /api/ops/fleet/restart` used to stop/start each agent with raw Docker +calls, bypassing `start_agent_internal()` — so none of the config-drift +predicates ran and a rebuilt base image was never adopted (#1809's cold-start +gate never fired). The fix delegates the loop body to +`lifecycle.restart_agent_internal` (explicit stop → full start path) and adds: +per-agent `recreated`/`recreate_reason` in the results (explicit allowlist +copy), an ephemeral-ghost skip, a single-flight SETNX lock, a partial-safe +audit entry, and the `reject_agent_principal` human-only gate. + +The live suite (`tests/test_ops.py`) exercises only response *structure* with +every agent skipped via a nonexistent prefix — the loop body's behavior is +covered HERE, mocked, per the `test_1816_system_agent_adoption.py` pattern. +""" + +from __future__ import annotations + +import asyncio +from unittest.mock import AsyncMock, MagicMock + +import pytest +from fastapi import HTTPException + + +def _run(coro): + loop = asyncio.new_event_loop() + try: + return loop.run_until_complete(coro) + finally: + loop.close() + + +def _human_caller(): + """A JWT/user-scoped principal. `User.agent_name` is set only for + scope="agent" keys, and `reject_agent_principal` keys off exactly that — so + a bare MagicMock (truthy `.agent_name`) would read as an agent key and 403.""" + caller = MagicMock() + caller.agent_name = None + return caller + + +def _agent_caller(): + caller = MagicMock() + caller.agent_name = "parent-agent" + return caller + + +class _Agent: + def __init__(self, name, status="running"): + self.name = name + self.status = status + + +class _Container: + def __init__(self, status="running"): + self.status = status + self.short_id = "cafe1234" + + +_START_OK = { + "recreated": False, + "recreate_reason": None, + "credentials_injection": "skipped", + "skills_injection": "skipped", +} + +_START_RECREATED = { + "recreated": True, + "recreate_reason": "image_drift", + "credentials_injection": "success", + "skills_injection": "success", +} + + +@pytest.fixture +def ops(monkeypatch): + """The real ``routers.ops`` with its auth + Docker + Redis surface patched. + + ``reject_agent_principal`` is deliberately NOT stubbed — the human-only + gate is exercised for real. Callers pass ``_human_caller()``. + """ + import routers.ops as mod + + monkeypatch.setattr(mod, "assert_admin", lambda user: None) + monkeypatch.setattr(mod, "db", MagicMock()) + mod.db.get_agent_owner.return_value = {"is_system": False} + mod.db.get_agent_ephemeral_info.return_value = None + monkeypatch.setattr(mod, "list_all_agents_fast", lambda: [_Agent("a1")]) + monkeypatch.setattr(mod, "get_agent_container", lambda name: _Container()) + monkeypatch.setattr( + mod, "restart_agent_internal", AsyncMock(return_value=dict(_START_OK)) + ) + monkeypatch.setattr(mod, "platform_audit_service", MagicMock(log=AsyncMock())) + monkeypatch.setattr(mod, "get_breaker_redis", lambda: None) # lock fail-open + monkeypatch.setattr(mod, "invalidate_context_stats_cache", MagicMock()) + return mod + + +def _call(mod, caller=None, **query): + return _run( + mod.restart_fleet( + MagicMock(), + current_user=caller if caller is not None else _human_caller(), + filter_status=query.get("filter_status"), + system_prefix=query.get("system_prefix"), + ) + ) + + +# --------------------------------------------------------------------------- +# Adoption + result surfacing +# --------------------------------------------------------------------------- + + +def test_drift_recreate_is_surfaced_per_agent_and_in_summary(ops, monkeypatch): + """The whole point of #1860: the fleet loop reaches the image-drift + predicate, and the router explicitly copies the recreate fields into its + own result dicts (they do NOT flow through on their own — #1809 learning).""" + monkeypatch.setattr( + ops, "list_all_agents_fast", lambda: [_Agent("a1"), _Agent("a2")] + ) + monkeypatch.setattr( + ops, + "restart_agent_internal", + AsyncMock(return_value=dict(_START_RECREATED)), + ) + + out = _call(ops) + + assert out["summary"] == { + "total": 2, "successes": 2, "failures": 0, "skipped": 0, "recreated": 2, + } + for entry, name in zip(out["results"], ("a1", "a2")): + assert entry["agent"] == name + assert entry["result"] == "success" + assert entry["previous_status"] == "running" + assert entry["recreated"] is True + assert entry["recreate_reason"] == "image_drift" + assert entry["credentials_injection"] == "success" + assert entry["skills_injection"] == "success" + assert [c.args for c in ops.restart_agent_internal.await_args_list] == [ + ("a1",), ("a2",), + ] + + +def test_no_drift_plain_restart_reports_recreated_false(ops): + out = _call(ops) + + assert out["summary"]["recreated"] == 0 + (entry,) = out["results"] + assert entry["result"] == "success" + assert entry["recreated"] is False + assert entry["recreate_reason"] is None + + +# --------------------------------------------------------------------------- +# Failure isolation +# --------------------------------------------------------------------------- + + +def test_one_agent_failing_never_aborts_the_loop_and_errors_are_legible( + ops, monkeypatch +): + """An `except HTTPException: raise` copied from the single-agent endpoint + would abort the fleet loop — pin that both exception shapes are recorded + (with non-empty, useful text) and later agents still restart.""" + monkeypatch.setattr( + ops, + "list_all_agents_fast", + lambda: [_Agent("a1"), _Agent("a2"), _Agent("a3")], + ) + monkeypatch.setattr( + ops, + "restart_agent_internal", + AsyncMock( + side_effect=[ + HTTPException(status_code=404, detail="Agent not found"), + RuntimeError("docker exploded"), + dict(_START_OK), + ] + ), + ) + + out = _call(ops) + + assert out["summary"]["failures"] == 2 + assert out["summary"]["successes"] == 1 + r1, r2, r3 = out["results"] + assert r1["result"] == "failed" and r1["error"] == "404: Agent not found" + assert r2["result"] == "failed" and r2["error"] == "docker exploded" + assert r3["result"] == "success", "the loop must continue past failures" + + +def test_containerless_failure_names_the_1559_recovery_path(ops, monkeypatch): + """A recreate that dies after removing the old container leaves NO + container — the agent vanishes from Docker-as-truth fleet listings. The + error must name the recovery (single-agent start → #1559).""" + handles = [_Container(), None] # pre-try lookup, then the hint re-check + monkeypatch.setattr(ops, "get_agent_container", lambda name: handles.pop(0)) + monkeypatch.setattr( + ops, + "restart_agent_internal", + AsyncMock(side_effect=RuntimeError("containers_run failed")), + ) + + out = _call(ops) + + (entry,) = out["results"] + assert entry["result"] == "failed" + assert "containers_run failed" in entry["error"] + assert "POST /api/agents/a1/start" in entry["error"] + + +def test_container_gone_before_restart_is_a_failed_row_not_1559(ops, monkeypatch): + """The pre-existing `container is None` guard is kept: an agent deleted + mid-loop records "container not found" rather than being routed into + #1559 missing-container recovery.""" + monkeypatch.setattr(ops, "get_agent_container", lambda name: None) + + out = _call(ops) + + (entry,) = out["results"] + assert entry == {"agent": "a1", "result": "failed", "error": "container not found"} + ops.restart_agent_internal.assert_not_awaited() + + +# --------------------------------------------------------------------------- +# Skips +# --------------------------------------------------------------------------- + + +def test_system_agent_is_skipped(ops): + ops.db.get_agent_owner.return_value = {"is_system": True} + + out = _call(ops) + + (entry,) = out["results"] + assert entry["result"] == "skipped" and entry["reason"] == "system agent" + ops.restart_agent_internal.assert_not_awaited() + + +def test_ephemeral_ghost_is_skipped(ops): + """Config-drift predicates are NOT ephemeral-gated in the lifecycle — a + cold start could recreate a drifted ghost and destroy its volume-less + workspace mid-budget (ent#69). The fleet loop must not cold-start ghosts.""" + ops.db.get_agent_ephemeral_info.return_value = {"is_ephemeral": True} + + out = _call(ops) + + (entry,) = out["results"] + assert entry["result"] == "skipped" and entry["reason"] == "ephemeral" + ops.restart_agent_internal.assert_not_awaited() + + +def test_ephemeral_lookup_failure_fails_open_to_restart(ops): + """A broken ephemeral accessor must not stop a durable agent's restart.""" + ops.db.get_agent_ephemeral_info.side_effect = RuntimeError("db down") + + out = _call(ops) + + assert out["summary"]["successes"] == 1 + ops.restart_agent_internal.assert_awaited_once() + + +def test_stopped_agent_is_skipped(ops, monkeypatch): + monkeypatch.setattr( + ops, "list_all_agents_fast", lambda: [_Agent("a1", status="exited")] + ) + + out = _call(ops) + + (entry,) = out["results"] + assert entry["result"] == "skipped" and entry["reason"] == "not running" + ops.restart_agent_internal.assert_not_awaited() + + +# --------------------------------------------------------------------------- +# Principal gate +# --------------------------------------------------------------------------- + + +def test_agent_scoped_principal_is_rejected_403(ops): + """`assert_admin` is a role gate, not a human gate — an agent-scoped key + carries its owner's role. Container replacement at fleet scale is + operator-only (Invariant #8; real `reject_agent_principal`).""" + with pytest.raises(HTTPException) as exc: + _call(ops, caller=_agent_caller()) + + assert exc.value.status_code == 403 + ops.restart_agent_internal.assert_not_awaited() + + +# --------------------------------------------------------------------------- +# Single-flight lock +# --------------------------------------------------------------------------- + + +def _lock_client(set_result=True): + client = MagicMock() + store = {} + + def _set(key, val, nx=None, ex=None): + if set_result: + store[key] = val + return set_result + + client.set.side_effect = _set + client.get.side_effect = lambda key: store.get(key) + return client + + +def test_concurrent_fleet_restart_is_refused_409(ops, monkeypatch): + client = _lock_client(set_result=False) + monkeypatch.setattr(ops, "get_breaker_redis", lambda: client) + + with pytest.raises(HTTPException) as exc: + _call(ops) + + assert exc.value.status_code == 409 + assert exc.value.detail == "fleet_restart_in_progress" + ops.restart_agent_internal.assert_not_awaited() + # A refused request did no work — it must not write a misleading audit row. + ops.platform_audit_service.log.assert_not_awaited() + + +def test_lock_is_acquired_refreshed_and_released_compare_and_delete( + ops, monkeypatch +): + client = _lock_client(set_result=True) + monkeypatch.setattr(ops, "get_breaker_redis", lambda: client) + + _call(ops) + + client.set.assert_called_once() + assert client.set.call_args.kwargs.get("nx") is True + client.expire.assert_called() # own-lease refresh inside the loop + client.delete.assert_called_once_with(ops._FLEET_RESTART_LOCK_KEY) + + +def test_redis_down_fails_open_and_still_restarts(ops, monkeypatch): + client = MagicMock() + client.set.side_effect = RuntimeError("redis down") + monkeypatch.setattr(ops, "get_breaker_redis", lambda: client) + + out = _call(ops) + + assert out["summary"]["successes"] == 1 + client.delete.assert_not_called() + + +# --------------------------------------------------------------------------- +# Audit + cache invalidation +# --------------------------------------------------------------------------- + + +def test_audit_carries_per_agent_recreate_map_and_failed_names(ops, monkeypatch): + monkeypatch.setattr( + ops, "list_all_agents_fast", lambda: [_Agent("a1"), _Agent("a2")] + ) + monkeypatch.setattr( + ops, + "restart_agent_internal", + AsyncMock( + side_effect=[dict(_START_RECREATED), RuntimeError("kaput")] + ), + ) + + _call(ops, system_prefix=None) + + ops.platform_audit_service.log.assert_awaited_once() + kwargs = ops.platform_audit_service.log.await_args.kwargs + assert kwargs["event_action"] == "fleet_restart" + details = kwargs["details"] + assert details["recreated"] == {"a1": "image_drift"} + assert details["failed_agents"] == ["a2"] + assert details["successes"] == 1 and details["failures"] == 1 + + +def test_audit_failure_never_fails_the_endpoint(ops): + ops.platform_audit_service.log.side_effect = RuntimeError("audit down") + + out = _call(ops) + + assert out["summary"]["successes"] == 1 + + +def test_context_stats_cache_invalidated_once(ops, monkeypatch): + monkeypatch.setattr( + ops, "list_all_agents_fast", lambda: [_Agent("a1"), _Agent("a2")] + ) + + _call(ops) + + ops.invalidate_context_stats_cache.assert_called_once() From 0fc440205cdc4630cafafcfa31f3b5d9e0f34cc8 Mon Sep 17 00:00:00 2001 From: webmixgamer Date: Fri, 31 Jul 2026 12:11:16 +0100 Subject: [PATCH 2/2] fix: never flow a raw exception message into fleet-restart results (CodeQL py/stack-trace-exposure) Per-agent failure rows now carry HTTPException .detail (platform-authored) or the exception class name only; the full message + traceback go to the backend log (exc_info). The #1559 containerless recovery hint is preserved. Tests strengthened into leak regression guards (raw message asserted absent); flow-doc line refs re-verified. Refs #1860 Co-Authored-By: Claude Fable 5 --- docs/memory/feature-flows/agent-lifecycle.md | 8 ++++---- src/backend/routers/ops.py | 16 ++++++++++++++-- tests/unit/test_1860_fleet_restart_adoption.py | 12 ++++++++++-- 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/docs/memory/feature-flows/agent-lifecycle.md b/docs/memory/feature-flows/agent-lifecycle.md index cc948ae49a..9cac38bb23 100644 --- a/docs/memory/feature-flows/agent-lifecycle.md +++ b/docs/memory/feature-flows/agent-lifecycle.md @@ -539,13 +539,13 @@ async def restart_agent_internal(agent_name: str, *, stop_timeout: int = 30) -> Explicit `container_stop(container, timeout=stop_timeout)` (:487), then the full `start_agent_internal` path (:488); returns its result dict unchanged. The explicit stop is **load-bearing**: the #1809 image-drift predicate runs only on a COLD start (`not was_already_running`), and starting a running agent is a deliberate idempotent no-op — so a restart that should adopt a rebuilt base image must stop first. A missing container falls through to `start_agent_internal`, which rebuilds it from persisted config (#1559, live ownership row) or 404s. This helper is the one home for the stop→start shape and the future home of #1817's per-agent start lock; `routers/system_agent.py`, `routers/systems.py`, and `services/subscription_auto_switch.py` still carry inline copies (consolidation deferred to #1817). -**First consumer — `restart_fleet`** (`POST /api/ops/fleet/restart`, `src/backend/routers/ops.py:234-472`): each agent routes `container is None` guard → `restart_agent_internal(agent_name)` (:363-365) instead of the old raw `container_stop`+`container_start`, so a fleet restart applies pending config drift and adopts a rebuilt base image exactly like a manual stop → start. Semantics: +**First consumer — `restart_fleet`** (`POST /api/ops/fleet/restart`, `src/backend/routers/ops.py:234-484`): each agent routes `container is None` guard → `restart_agent_internal(agent_name)` (:363-365) instead of the old raw `container_stop`+`container_start`, so a fleet restart applies pending config drift and adopts a rebuilt base image exactly like a manual stop → start. Semantics: - **Skips**: system agent (:302-312) AND ephemeral ghosts (:314-330) — the config-drift predicates are not ephemeral-gated, so a cold start could recreate a drifted ghost and destroy its volume-less workspace (ent#69 "ghosts never recreate"); GC owns ghost lifecycle. Pre-existing `filter_status` / `system_prefix` / not-running skips unchanged. - **Principals**: `reject_agent_principal` beside `assert_admin` (:256-260) — the endpoint now *replaces* containers, and escalating a handler's destructiveness re-prices every principal that could already reach it (Invariant #8, the #1816 rule). -- **Single-flight**: Redis SETNX lock `ops:fleet_restart` (:42-43, :262-277) — a second caller gets 409 `fleet_restart_in_progress`; 900s TTL bounds a crashed holder, the live loop refreshes its own lease each iteration (:296-300), release is compare-and-delete of the holder's own token (:425-432); fail-open when Redis is down. Guards the client-timeout-retry race (a retried POST interleaving stop/start with the still-running first loop — the #799/#1817 wedge class). -- **Result surfacing**: per-agent entries explicitly copy `recreated` / `recreate_reason` / `credentials_injection` / `skills_injection` (:369-377) — the #1809 allowlist learning: this router builds its own result dicts, service fields do NOT flow through on their own — and `summary.recreated` counts replacements (:469). A failed recreate that leaves NO container gets an actionable error naming `POST /api/agents/{name}/start` (#1559) as the recovery path (:404-408). -- **Audit as durable record**: a partial-safe `fleet_restart` entry (`AuditEventType.SYSTEM`) is written in `finally` (:437-460) with totals + a per-agent `recreated: {name: reason}` map (:453) — the sequential loop can outlast proxy timeouts (nginx 60s / Cloudflare ~100s), so a client 504/524 does NOT mean the restart failed; the outcome lands here. `invalidate_context_stats_cache()` (:424) and the lock release run before the awaited audit call, so a shutdown `CancelledError` at the await can't leave the lock held for a full TTL. +- **Single-flight**: Redis SETNX lock `ops:fleet_restart` (:42-43, :262-277) — a second caller gets 409 `fleet_restart_in_progress`; 900s TTL bounds a crashed holder, the live loop refreshes its own lease each iteration (:296-300), release is compare-and-delete of the holder's own token (:437-444); fail-open when Redis is down. Guards the client-timeout-retry race (a retried POST interleaving stop/start with the still-running first loop — the #799/#1817 wedge class). +- **Result surfacing**: per-agent entries explicitly copy `recreated` / `recreate_reason` / `credentials_injection` / `skills_injection` (:369-377) — the #1809 allowlist learning: this router builds its own result dicts, service fields do NOT flow through on their own — and `summary.recreated` counts replacements (:481). Failure rows never carry a raw exception message — HTTPException `.detail` (platform-authored) or the exception class name only, full text + traceback in the backend log (`py/stack-trace-exposure`, :396-418); a failed recreate that leaves NO container gets an actionable error naming `POST /api/agents/{name}/start` (#1559) as the recovery path (:414-418). +- **Audit as durable record**: a partial-safe `fleet_restart` entry (`AuditEventType.SYSTEM`) is written in `finally` (:450-473) with totals + a per-agent `recreated: {name: reason}` map (:465) — the sequential loop can outlast proxy timeouts (nginx 60s / Cloudflare ~100s), so a client 504/524 does NOT mean the restart failed; the outcome lands here. `invalidate_context_stats_cache()` (:436) and the lock release run before the awaited audit call, so a shutdown `CancelledError` at the await can't leave the lock held for a full TTL. **Authentication Model** (Updated 2026-03-03 for SUB-002): diff --git a/src/backend/routers/ops.py b/src/backend/routers/ops.py index ef44573ebb..70f0d5eee8 100644 --- a/src/backend/routers/ops.py +++ b/src/backend/routers/ops.py @@ -394,9 +394,19 @@ async def restart_fleet( failures += 1 except Exception as e: if isinstance(e, HTTPException): + # Platform-authored client text (FastAPI returns .detail + # to callers by design) — not an exception-internals leak. error_text = f"{e.status_code}: {e.detail}" else: - error_text = str(e) or e.__class__.__name__ + # py/stack-trace-exposure (CodeQL, PR #1912): never flow + # a raw exception message into the response — docker/OS + # error strings can embed internals (the #1885 / git-PAT + # stderr class). Class name only; the full message + + # traceback go to the backend log line below. + error_text = ( + f"restart failed ({e.__class__.__name__} — " + f"details in backend logs)" + ) # A recreate that failed after removing the old container # leaves the agent with NO container — it vanishes from fleet # listings (Docker-as-truth). Name the recovery path (#1559). @@ -415,7 +425,9 @@ async def restart_fleet( }) failed_agents.append(agent_name) failures += 1 - logger.warning(f"Fleet restart: {agent_name} failed: {error_text}") + logger.warning( + f"Fleet restart: {agent_name} failed: {e}", exc_info=True + ) finally: # Sync cleanup FIRST, awaited audit LAST: a CancelledError raised at # the audit await (backend shutdown mid-loop) is a BaseException the diff --git a/tests/unit/test_1860_fleet_restart_adoption.py b/tests/unit/test_1860_fleet_restart_adoption.py index 5ffb6425b3..3d4b94979b 100644 --- a/tests/unit/test_1860_fleet_restart_adoption.py +++ b/tests/unit/test_1860_fleet_restart_adoption.py @@ -188,7 +188,12 @@ def test_one_agent_failing_never_aborts_the_loop_and_errors_are_legible( assert out["summary"]["successes"] == 1 r1, r2, r3 = out["results"] assert r1["result"] == "failed" and r1["error"] == "404: Agent not found" - assert r2["result"] == "failed" and r2["error"] == "docker exploded" + # py/stack-trace-exposure (CodeQL, PR #1912): an unexpected exception's + # raw message must NEVER reach the response — class name only, full + # detail goes to the server log. + assert r2["result"] == "failed" + assert "RuntimeError" in r2["error"] + assert "docker exploded" not in r2["error"] assert r3["result"] == "success", "the loop must continue past failures" @@ -208,7 +213,10 @@ def test_containerless_failure_names_the_1559_recovery_path(ops, monkeypatch): (entry,) = out["results"] assert entry["result"] == "failed" - assert "containers_run failed" in entry["error"] + # Class name + recovery hint surface; the raw exception message does not + # (py/stack-trace-exposure — see the failure-isolation test above). + assert "RuntimeError" in entry["error"] + assert "containers_run failed" not in entry["error"] assert "POST /api/agents/a1/start" in entry["error"]