fix: fleet restart adopts rebuilt base images through the canonical lifecycle path (#1860) - #1912
Merged
Merged
Conversation
…ts adopt rebuilt base images (#1860) 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 0ec3a7f); 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 <noreply@anthropic.com>
3 tasks
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…odeQL 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 <noreply@anthropic.com>
…d append collision, keep all four entries
3 tasks
obasilakis
approved these changes
Jul 31, 2026
obasilakis
left a comment
obasilakis
left a comment
Contributor
There was a problem hiding this comment.
Reviewed via /review (structural pass on the merge-base diff).
No critical findings. Auth is strictly tightened and correctly scoped — verified User.agent_name is set only for scope == "agent" (dependencies.py:360), so system-scoped keys keep access and no platform automation breaks. The CodeQL exposure fix holds: the entire reachable call graph from start_agent_internal has exactly two detail= sites and both are static, so nothing dynamic can ride HTTPException.detail through. container_start import removal verified against zero remaining references. Test coverage is genuinely thorough (both exception shapes, leak regression guard, full lock lifecycle + both fail-open paths, all four skips, audit contents).
Three informational notes, none blocking:
- Lease refresh isn't ownership-checked (
ops.py:296-300) — release is compare-and-delete, butexpirefires unconditionally onlock_held. If one agent's restart ever outlives the 900s TTL, the lock expires, a second caller acquires, and this loop then extends their lease while both run — the interleaving the lock exists to prevent, masked by the guard. Low probability (the refresh only has to cover a single agent), but aGET-then-EXPIREor Lua CAS would close it. - Lock acquired outside the
try/finally—list_all_agents_fast()sits between acquire (:262-280) andtry:(:293). Unreachable today (it swallows everything and returns[]), but any statement added there later leaks a 900s fleet-restart lockout. - AC3 in substance, not in wording — every agent now cold-starts, so
skip_injectionis alwaysFalseand each restart payswait_for_agent_ready(60s cap) + credential retries + skill injection. The PR decides the consequences (sequential loop, timeout semantics, audit-as-record); the injection cost itself is implicit rather than labelled.
Happy for 1 and 2 to be a follow-up rather than a respin.
3 tasks
dolho
added a commit
that referenced
this pull request
Aug 3, 2026
…onses (#1917) PR #1912 fixed `restart_fleet` after CodeQL flagged py/stack-trace-exposure, and deliberately left the sibling sites alone under the minimal-changes rule. This closes them with the same pattern — response carries platform-authored text or the exception CLASS NAME; the full message + traceback go to the backend log via exc_info=True. Sites (located by content, not the issue's line numbers — dev has moved): routers/ops.py fleet health probe, stop_fleet per-agent, _stop_agent_container (feeds emergency_stop), ops costs routers/system_agent.py health probe (open CodeQL alert #231), workspace cleanup, template copy, the re-initialize and restart 500s, and the terminal WebSocket error frame routers/agents.py start / stop / logs HTTPException details The health probe had `str(e)[:50]`. Truncation bounds a leak, it does not remove one: docker and httpx messages lead with the host, socket path or URL, so the first 50 characters are exactly the sensitive part. Two sites were not in the issue and are the same defect, found by grepping the named files rather than trusting the list: system_agent's re-initialize/restart 500 details, and its terminal WebSocket error frame — the only one here that reaches a browser rather than an admin API client. Guards: 8 tests, all 8 failing against pre-fix code. Each drives the real router function with a sentinel in the exception message and asserts BOTH that the sentinel is gone and that the class name survives — a response that dropped the error entirely would pass a sentinel-only check while making the failure undiagnosable. Plus a static ban on str(e) in the two routers where every occurrence was this defect (agents.py is deliberately excluded: it has many str(e) uses that never reach a response, so a blanket ban there would be false). The fleet-health test initially patched a `get_agent_context_info` that does not exist — `raising=False` made that silent, the probe never raised, and the test passed against the UNPATCHED router. The pre-fix run is what caught it; it now patches `get_agent_client` and asserts the probe actually ran. Same vacuous-negative class as #1932/#1951. Related to #1917 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
Aug 3, 2026
…onses (#1917) (#1959) PR #1912 fixed `restart_fleet` after CodeQL flagged py/stack-trace-exposure, and deliberately left the sibling sites alone under the minimal-changes rule. This closes them with the same pattern — response carries platform-authored text or the exception CLASS NAME; the full message + traceback go to the backend log via exc_info=True. Sites (located by content, not the issue's line numbers — dev has moved): routers/ops.py fleet health probe, stop_fleet per-agent, _stop_agent_container (feeds emergency_stop), ops costs routers/system_agent.py health probe (open CodeQL alert #231), workspace cleanup, template copy, the re-initialize and restart 500s, and the terminal WebSocket error frame routers/agents.py start / stop / logs HTTPException details The health probe had `str(e)[:50]`. Truncation bounds a leak, it does not remove one: docker and httpx messages lead with the host, socket path or URL, so the first 50 characters are exactly the sensitive part. Two sites were not in the issue and are the same defect, found by grepping the named files rather than trusting the list: system_agent's re-initialize/restart 500 details, and its terminal WebSocket error frame — the only one here that reaches a browser rather than an admin API client. Guards: 8 tests, all 8 failing against pre-fix code. Each drives the real router function with a sentinel in the exception message and asserts BOTH that the sentinel is gone and that the class name survives — a response that dropped the error entirely would pass a sentinel-only check while making the failure undiagnosable. Plus a static ban on str(e) in the two routers where every occurrence was this defect (agents.py is deliberately excluded: it has many str(e) uses that never reach a response, so a blanket ban there would be false). The fleet-health test initially patched a `get_agent_context_info` that does not exist — `raising=False` made that silent, the probe never raised, and the test passed against the UNPATCHED router. The pre-fix run is what caught it; it now patches `get_agent_client` and asserts the probe actually ran. Same vacuous-negative class as #1932/#1951. Related to #1917 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
POST /api/ops/fleet/restartbypassedstart_agent_internalwith raw Docker stop/start — "Restart All" afterbuild-base-image.shupgraded nothing (no config-drift predicates, no bug: a rebuilt agent base image is never picked up — no image-drift predicate, so the v0.8.5 "stop/start-with-recreate" upgrade step is a no-op #1809 image-drift adoption, no bug(lifecycle): stale transport circuit-breaker Redis key survives agent delete/recreate — a fresh healthy agent fast-fails as "unhealthy" #1560 breaker clearing). The most intuitive operator action was the one path guaranteed not to upgrade anything.lifecycle.restart_agent_internal(name, *, stop_timeout=30)— the canonical stop→cold-start helper (the explicit stop is load-bearing: the bug: a rebuilt agent base image is never picked up — no image-drift predicate, so the v0.8.5 "stop/start-with-recreate" upgrade step is a no-op #1809 image predicate is cold-start-gated).restart_fleetroutes through it; the helper is the future home of refactor: per-agent start lock — complete the concurrent-start race hardening from #1809 #1817's per-agent start lock (the remaining inline copies insystem_agent.py/systems.py/subscription_auto_switch.pyconsolidate there).reject_agent_principal(the endpoint now replaces containers — Invariant security: implement safe tar extraction with symlink/hardlink validation #8 / bug: trinity-system never adopts a rebuilt base image — ensure_deployed short-circuits on 'already running' #1816 rule), a single-flight Redis SETNX lock (409 fleet_restart_in_progress; guards the client-timeout→retry overlap, the bug: SUB-003 auto-switch has no per-agent lock — concurrent 429s race the restart #799/refactor: per-agent start lock — complete the concurrent-start race hardening from #1809 #1817 wedge class), an ephemeral-ghost skip (config predicates aren't ephemeral-gated; a recreate destroys a volume-less ghost workspace, ent#69), and a partial-safefleet_restartaudit entry with a per-agent{name: recreate_reason}map — the durable record when a slow fleet outlasts proxy timeouts (restores the audit dropped in0ec3a7fc).Changes
src/backend/services/agent_service/lifecycle.py— newrestart_agent_internalhelpersrc/backend/routers/ops.py—restart_fleetrewrite (delegation, skips, lock, principal gate, result allowlist copy, audit-in-finally,invalidate_context_stats_cache, actionable bug(recovery): soft-delete recovery dead-ends —recovertells you tostart, butstart404s when the container is gone #1559 recovery errors)tests/unit/test_1860_fleet_restart_adoption.py— 16 mocked unit tests (adoption surfacing, failure isolation, containerless recovery hint, skips, 403, 409 lock + fail-open, audit map, cache invalidation)architecture.md(feat: credential rotation via hot-reload, not container recreate #1089 reword +ops:fleet_restartRedis key),agent-lifecycle.md(new bug: fleet restart bypasses start_agent_internal — no agent adopts a rebuilt base image on "Restart All" #1860 subsection),internal-system-agent.md+mobile-admin-pwa.mdendpoint rows, 2 learnings-ledger entries, CSO diff report (PASS, zero findings)Test Plan
pytest tests/unit/test_1860_fleet_restart_adoption.py -v(16/16)test_1069, unrelated)image_drift, container image IDs verified against newtrinity-agent-base:latest;trinity-systemskipped; workspace marker file survived;stats200 +clone_status: okpost-recreaterecreated: false, container IDs unchangedconfig_drift409 fleet_restart_in_progress; agent-scoped MCP key →403human-only; audit row inspected with per-agent recreate mapBlast-radius decision (AC3)
Sequential loop kept deliberately: admin-initiated rare op, zero concurrent-recreate load, no dependency on unshipped #1817. A client timeout (nginx 60s / Cloudflare ~100s at ≳3-6 recreating agents) does NOT mean failure — the loop continues server-side and the audit row is the durable record (endpoint docstring documents this + the
system_prefix/filter_statuschunking workaround). Fleet-wide breaker reset on cold starts is intended stop→start semantics. Note for operators: fleet restart now applies pending config drift — container IDs change on what previously looked like "just a restart".Fixes #1860
🤖 Generated with Claude Code