diff --git a/docs/memory/architecture/reliability.md b/docs/memory/architecture/reliability.md index 1e7abac29..24c800298 100644 --- a/docs/memory/architecture/reliability.md +++ b/docs/memory/architecture/reliability.md @@ -43,7 +43,7 @@ agent:heartbeat:misses:{name} → STRING(int), ~60s TTL. Consecutive-miss counte **Blast-radius guard (#1644).** #1638 made the *defaults* fail-safe; it added no guard on the prune. So every other route to a destructive window stayed open — `PUT /api/settings/ops/config` (which until ent#297 took a raw `Dict[str, str]` with no type/range/clamp), a future default regression, a direct DB write. **Write-path hardening (ent#297):** the windows now have exactly ONE API write path — `PUT /api/settings/ops/config`, which type- and range-validates every value (`config.validate_ops_setting`, all-or-nothing, 422 on the first bad one) and audit-logs the change (`ops_settings_change`, naming which retention windows moved; neither this route nor `/ops/reset` logged anything before, so the one route that could shrink a window was also the one that left no trace). **`/ops/reset` audits too, as of #1966** — ent#297 wrote the sentence above but wired the entry into `/ops/config` only, so the asymmetry it objected to survived one route over. It emits `ops_settings_reset` carrying the `reset`/`skipped` key lists — **keys and counts only**, since every one of those rows is being deleted and the durable fact is which keys reverted to their code default, not what they held on the way out — and it logs **unconditionally**, not gated on having deleted something: an admin resetting already-default settings is still an administrative act, and its absence from the log would be indistinguishable from never having been attempted. Retention windows genuinely cannot be reset here (#1638 `continue`s over them, and the entry reports them under `skipped`); what could silently revert with no trace was everything else — `ssh_access_enabled`, which decides whether ephemeral SSH credentials may be minted at all, included. The generic `PUT /api/settings/{key}` catch-all now 422s `RETENTION_OPS_KEYS` and points at it, closing the second unvalidated path — same shape as the `max_parallel_tasks_ceiling` (#506), `PROACTIVE_RATE_LIMIT_DEFAULTS` (#1609) and `telemetry_sharing_*` (ent#12) redirects. Note what validation does NOT buy, since the asymmetry is counter-intuitive: garbage always failed *safe* (unparseable → `0` → sweep disabled → retain forever), and **a small valid integer is the catastrophic input** — `"1"` is well-typed and in-range and is exactly the PoC. No range check can separate it from an operator who genuinely wants a one-day window, so validation buys a loud failure instead of a silent coercion; the controls that stop the *attack* are the admin gate rejecting agent principals (ent#293/#297) and this guard. The community floor is deliberately NOT applied as a clamp here (#1039/#1638: it is a fresh-install seed plus an enterprise entitlement clamp, never an OSS hard limit). `services/retention_guard.py` gates all **11** window-driven destructive prunes (`RETENTION_OPS_KEYS` carried 8 from #1296's `agent_reminders_retention_days` until ent#433 added `subscription_headroom_retention_days` and `subscription_failure_event_retention_days`, and ent#478 added `metrics_retention_days`; `cleanup_service` has 11 `_guard_allows` sites): it takes a **bounded** count of the candidate set (`LIMIT threshold+1` → O(threshold), not O(candidates)) and, if it exceeds the threshold, **refuses**, logs ERROR on the green→red transition, and raises an `operator_queue` alarm. `cleanup_service._guard_allows` is not its only consumer — `GET /api/settings/retention` re-runs the same `evaluate` **live** so the Settings panel can offer an approve control, and that call is **unwrapped**, which is why the guard must refuse rather than raise (#1833). Key properties: **stateless detection** — no watermark of the last-seen window, because that row would be deletable through the same endpoints that cause the bug (`learnings.md` #1638 Lesson 2); the ack *is* state, but deleting an ack fails safe. **Absolute counts, no percentage** — there is no denominator for `execution_log` (its predicate *includes* `execution_log IS NOT NULL`), a total would cost a full scan and divide-by-zero on an empty table, and a percentage *inverts* on the agent sweep (3 purged agents ≈ 0% of any table but 3 destroyed volume sets); steady state is a trickle and any anomaly is large, so table size is irrelevant. **Fail-closed** — every error path refuses: the count throws (`count_failed`), the count cannot be compared to the threshold (`count_uninterpretable`, #1833), the count is negative i.e. an error sentinel rather than a count (`count_negative`, #1833 — `-1 <= threshold` is True and `-1` is the module's own "unknown" value, so this was a genuine fail-OPEN), the ack lookup throws (`ack_lookup_failed`). There is **no** "threshold unreadable" path, because the threshold is a constant. A guard that fails open manufactures confidence — and one that RAISES instead of refusing keeps the data (control never reaches `db.prune_*`) while losing the alarm, which is why #1833 moved the comparisons inside the try rather than documenting the raise. The comparison result is **type-checked**, not coerced: an object whose `__le__` returns a truthy non-bool would make a bare `bool(...)` True and authorize a prune on a count the guard never understood. `verdict.candidates` is normalised to an int before publication (`-1` = unknown) because it reaches the alarm message, the alarm `context` via `json.dumps`, and `GET /api/settings/retention` — where a bare `NaN` is valid to Python and rejected by a browser's `JSON.parse`. A refusal for a reason an ack cannot clear says so instead of prescribing one, and `GET /api/settings/retention` reports it under `blocked_sweeps` rather than rendering a clean "nothing pending" for a sweep that is blocked forever — **scoped, like the `pending_acknowledgements` list it sits beside, to the three ack-gated sweeps that endpoint re-runs** (`agent_soft_delete_retention_days`, `schedule_soft_delete_retention_days`, and ent#478's `metrics_retention_days` — added there because for a point store a refusal is the EXPECTED path after a narrowing, not an exception, so the alarm's link would otherwise be the only unblock); the other eight windows are never evaluated there, so their refusals reach an operator only through the durable operator-queue alarm. Widening that loop to all 11 is a follow-up, not an oversight — it would change what the endpoint costs and what an "approve" control means for a sweep whose floor is 1000. **The ack endpoint is the gate; the queue item is only an alarm** — `create_item` is a blind `INSERT ... ON CONFLICT DO NOTHING`, so a load-bearing queue item would wedge shut permanently once it reached any terminal state (e.g. Clear All → `cancelled`), and `prune_terminal_items` would delete the approval at 90 days — the sweep deleting its own authorization. **A failed alarm write is re-attempted every cycle for the life of the refusal episode (#1834)** — the memo used to be written *before* the attempt, so one failed write permanently suppressed its own retry and the durable half of the signal was lost until a restart. Never abandoned, which is the deliberate divergence from #1897's give-up: that sink is an external webhook, this one is the platform's own DB and its outages routinely outlast any budget, so a give-up would ship #1834's symptom inside #1834's fix. The per-attempt WARNING escalates **once** to ERROR past `ALARM_ESCALATION_AGE_SECONDS`, worded to claim only what that worker knows (`cleanup_service` runs in every uvicorn worker with no leader lease, so a sibling may have landed the row). Safe precisely because the queue item authorizes nothing, and because "delivered" means "the call did not raise" — never "a row was inserted", or the second worker's conflict no-op would retry forever. `POST /api/settings/retention/acknowledge` (admin **+ `reject_agent_principal`**, since an agent-scoped key resolves to its owner *carrying the owner's role* and would otherwise pass `require_admin` on a default admin-owned install — see trinity-ops-agent#232) records an ack **bound to the window in force** (409 on mismatch) and **single-use** (consumed by `cleanup_service` once the prune actually runs, so the guard re-arms). The threshold (`retention_guard.MAX_ROWS_PER_SWEEP`, 1000) is a **fixed constant, deliberately not a setting** — it was briefly an operator knob with a Settings panel, which was wrong twice over: nobody can reason about the right value (it depends on per-cycle churn the operator can't see, so the panel needed a caption explaining that *bigger is worse* — a control that must explain which way is safe is the wrong control), and a mutable constant read at action time gating a destructive op is structurally identical to `OPS_SETTINGS_DEFAULTS`, i.e. #1638 one level up. Deleting the knob deleted its clamp, its range-validated endpoint, its catch-all blocklist entry, and one whole fail-closed branch (a constant cannot fail to read). It is chosen against **steady state, not table size**: at any sane window only rows crossing the cutoff within one 5-min cycle are candidates — a trickle of tens; four digits means something changed. Lowering is always safe; raising weakens every install at once, so `tests/unit/test_1644_retention_guard.py` pins the *direction* (`MAX_ROWS_PER_SWEEP <= PREVIOUS`), not the value, and asserts it is not settings-backed. Reported read-only via `GET /api/settings/retention` → `guard.max_rows`. Per-sweep floors: rows→`MAX_ROWS_PER_SWEEP`, schedules→100, **agents→0** (every purge destroys volumes, #1581 — always acked), **metric points→100 000** (ent#478: one agent-day at the default cap; at the row default any real ingest rate refuses every cycle, alarms once, and then grows behind single-use acks). Each count **shares its prune's predicate by construction** (`_execution_row_prune_predicate` et al.) so the two can never drift; the alarm hosts on the reserved sentinel `_retention-guard`, uncreatable because `sanitize_agent_name` strips the leading `_`, and excluded from canary L-03's `operator_queue` orphan scan (it is not a ghost agent). Alarm `context` carries **counts and identifiers only, never sample rows** (canary G-04's lesson). -**Retention resolution — floor-by-seed, not floor-by-default (#1638).** Each window resolves **DB row → code default**, except the keys listed in `config.ENV_BACKED_OPS_KEYS` (ent#478's `metrics_retention_days` and `metrics_daily_point_cap`), which resolve **DB row → env → code default** through `settings_service.resolve_ops_setting`. The env tier is opt-in per key precisely so the other windows' precedence — which `GET /api/settings/retention` documents — is unchanged, and env is a LIVE fallback rather than a one-time seed: the #2085 seeder skips a key whose variable is set, so a later change to the variable is still honoured and a saved row still wins over both. "Set" has to mean *an operator set it*: every backend compose file forwards the env-backed keys with an EMPTY default (`${METRICS_DAILY_POINT_CAP:-}`) and `.env.example` leaves them commented (trinity-enterprise#671) — with `:-100000`-style defaults and a copied example line the variable was set on every install, so the seeder never seeded the window, every source read `env`, and the Settings rows built on that source would have locked (`tests/unit/test_ent671_env_backed_ops_forwarding.py`). (Log archival separately reads `LOG_*`.) Because the code default is the fallback for every install that never wrote a row — the default state — it is read at *prune* time and must stay at the **widest** value: `OPS_SETTINGS_DEFAULTS` is a **safety floor, not a policy knob**. Lowering one retroactively hard-DELETEs the existing data of every un-configured install seconds after its next boot, silently (#1638, which cost a real instance ~3 months of history and shipped green because a test asserted the destructive default). The #1039 `COMMUNITY_RETENTION_FLOOR_DAYS=5` community floor therefore reaches **new installs only**, by *seeding* explicit rows into a fresh DB (`config.COMMUNITY_FRESH_INSTALL_SEED` → `database._seed_fresh_install_retention{,_engine}`, run inside `init_database()` after `init_schema` and **before** `_ensure_admin_user` — an empty `users` table is the fresh-install signal). The seed is fail-safe (never raises: `init_database()` runs at import, so raising would crash-loop boot) and idempotent (`INSERT OR IGNORE` / `on_conflict_do_nothing` — both migration locks fail open). **#2085 completes the mechanism for installs that are not fresh.** #1645 seeded rows on a fresh DB only, so every install that has ever *upgraded* rather than been created fresh still had no rows at all and resolved all 12 windows at prune time from the image — leaving a code comment as the only thing between a future edit to `OPS_SETTINGS_DEFAULTS` and the #1638 failure mode. `database._seed_retention_windows{,_engine}` now writes an explicit row for **every** `RETENTION_OPS_KEYS` member that has none, at **the value already in force**, on every boot and regardless of install age — so it is behaviourally inert (nothing prunes differently the day it runs) and needs no migration note. Key set is derived from `RETENTION_OPS_KEYS`, never a second hand-written list, so a window added later is covered the day it ships instead of quietly inheriting the image default forever (ent#433 added two, #2216 a third). **Ordering is load-bearing and pinned by test in both arms**: it MUST run after `_seed_fresh_install_retention` in `init_database()`, because both writers are insert-or-ignore and the first to reach a key wins — reversed, a fresh install would silently get the wide defaults (30/90/7/30) instead of the #1039 floor, i.e. the community floor deleted by the change meant to protect retention. **Stated tradeoff:** a seeded install stops inheriting later changes to the code default in *either* direction, so widening a window for existing installs becomes a deliberate migration rather than something that arrives silently with an image — the intended consequence, making retention explicit per-install config instead of implicit inheritance from whatever image is running. Same fail-safe contract as the #1638 seed (never raises; a skip leaves the install exactly where it is today). Note `backup_retention_days` is seeded too, which makes `OPS_SETTINGS_DEFAULTS`' value the one that lands in the DB for a key whose private reader (`effective_backup_retention_days`) falls back to its **own** module constant — the two are now parity-tested. `agent_soft_delete_retention_days` is **exempt from the floor in every edition**: its expiry destroys agent data volumes (#1581), making it a *recovery* window, not a log window. The floor is **not enforced** — any admin may widen a window via `PUT /api/settings/ops/config` (no clamp); the enterprise `retention` module clamps only *unentitled writes* to a **minimum** of 5 and sells the managed panel (audit/`updated_by`/hot-reload), not the capability. `POST /api/settings/ops/reset` **skips** `RETENTION_OPS_KEYS` (#1638 — "reset to defaults" must not silently change how much operator data is kept). **`RETENTION_OPS_KEYS` carries all 12 keys — 11 row windows + #2216's file-artifact `backup_retention_days`** — #1644 added `agent_reports_retention_days` and `operator_queue_retention_days`, which had been missing, so `/ops/reset` **deleted** those two rows while reporting `"retention windows unchanged"`, `GET /api/settings/retention` hid them, and boot logging never showed them; #1296 then added `agent_reminders_retention_days`, taking the row set to 8 (this doc said 7 in two places until #1833), ent#433 took it to 10 with `subscription_headroom_retention_days` (the new probe-history table) and `subscription_failure_event_retention_days` (which **converted** `subscription_rate_limit_events`' hardcoded 24h sweep into a real window — that table held the only durable record of real agent work hitting a provider rate limit and destroyed it daily with none of the three protections every sibling had), and ent#478 took it to 11 with `metrics_retention_days` (recorded business-metric points, a year by default). Membership means "is a retention window", NOT "gets the community floor" (that set is `COMMUNITY_FRESH_INSTALL_SEED`, still 4 — both ent#433 keys are deliberately excluded, since the 5-day floor would truncate the 7-day default read window while the UI labelled it 7 days). Surfaced read-only at `GET /api/settings/retention`, which reports per-key `source` (`db-row` vs `code-default`) plus the effective (clamped) `guard.max_rows`; `cleanup_service.log_effective_retention_windows()` logs the same at boot *before* the first sweep, and a prune hitting the per-transaction chunk size logs at WARNING (a routine trickle stays INFO). Runbook: [docs/migrations/RETENTION_DEFAULTS_2026-07.md](../../migrations/RETENTION_DEFAULTS_2026-07.md). **#1142 operator_queue retention** — DELETEs terminal `operator_queue` rows (acknowledged/cancelled/expired) past `operator_queue_retention_days` (default 90, `0`=off); `responded` rows use a more generous fixed floor (`OPERATOR_QUEUE_RESPONDED_MIN_RETENTION_DAYS`, 30d — they still carry an operator answer the 5s write-back loop must deliver), `pending` never deleted (`db.prune_operator_queue_terminal_items`, select-ids-then-delete, capped). This is the automatic complement to #1017's manual Clear-All (which only *hid* via `cleared_at`). `0` disables; `PRAGMA wal_checkpoint(TRUNCATE)` when any sweep reclaims rows. Also purges expired `idempotency_keys`. **There is no per-cycle row cap (#1644 — this doc previously claimed one).** `RETENTION_CHUNK_SIZE_PER_CYCLE=5000` bounds each *transaction*, and only sometimes the call. Counted at #1833 (this doc said "six of the seven", wrong in both terms — the denominator was 8, and the split 5/3; ent#433 makes it 10 and 7/3): **seven** accessors loop `while True` and drain the **entire** candidate set in one sweep — `prune_execution_logs`, `prune_execution_rows`, `cleanup_old_health_records`, `prune_agent_reports`, `prune_agent_reminders`, and ent#433's `prune_headroom_history` + `cleanup_old_rate_limit_events`. **Three** are genuinely capped per cycle: `prune_operator_queue_terminal_items` takes `limit=` directly, and the two soft-delete sweeps select their victims through `find_soft_deleted_{agents,schedules}_past_retention(limit=RETENTION_CHUNK_SIZE_PER_CYCLE)` before purging one by one. #1638 deleting 5352 rows in one startup sweep is the proof that chunking is not a cap on the first five — a real 5000 cap could not produce that number. So for those five the bound on destruction is the blast-radius guard below, not chunking; for the other three it is both, and the guard is still the one that stops at the *threshold* rather than at 5000. **#1581 volume reclaim** — at the #834 hard-purge (the instant the agent becomes unrecoverable, NOT at soft-delete) the agent's `agent-{name}-{workspace,public,shared}` Docker volumes are removed via `docker_utils.remove_agent_volumes` (double-guarded: exact name AND `trinity.agent-name`+`trinity.platform` labels must match, fail-closed; NotFound/in-use tolerated → retry). A separate bounded Docker-as-truth orphan sweep (`_sweep_orphan_agent_volumes`, ≤100 agents/cycle, 1h creation-grace) reclaims agent-data volumes whose ownership row is gone. Closes the unbounded volume leak (`volume_remove` previously had zero lifecycle callers). **Orphanhood is a DB question, not a volume-label question (#1664):** rename keeps the agent's volumes, so their name AND immutable `trinity.agent-name` label both stay at the pre-rename name — reading that as ownership marked a LIVE agent's home volume an orphan and force-removed it during any container-recreate gap (silent #1169 `data_paths` loss). Ownership is now resolved through `agent_ownership.volume_base_name` (NULL ⇒ `agent_name`; pinned to the pre-rename name in the same statement as the rename, frozen across re-renames) via `db.is_volume_base_reserved` — soft-deleted rows still match, so the recovery window is respected for a renamed agent too. The predicate is the **union of both identities** (`agent_name` OR `volume_base_name`), because a renamed agent owns volumes under **two** bases: its workspace keeps the old one, while `get_public_volume_name`/`get_shared_volume_name` name off the LIVE name, so `agent-{new}-public` appears if file-sharing is enabled after a rename (and that volume is unmounted whenever sharing is off, so the attached-check can't cover it). The union is strictly safer than the pre-#1664 `is_agent_name_reserved` it replaces — that predicate is exactly its first branch. The purge removes both bases for the same reason (deduped; one call for an un-renamed agent). **One row per base is the invariant, enforced at BOTH producers (#1671):** creation gates on `is_volume_base_reserved` (`crud.py`), and rename gates on it too — at the router (actionable 409, raised before the container is touched) and inside `db.rename_agent`'s transaction (the chokepoint that closes the check-then-write gap, #1445 pattern). Rename passes `exclude_agent=` so an agent renamed `B`→`A` (pin `B`) can still be renamed back to `B` — it already owns that base and the result has a single claimant. Leaving rename ungated let an ordinary swap (`x`→`x-old`, then `y`→`x`) mint two claimants of base `x`, which is the #1667 silent-adopt disclosure via `get_public_volume_name` (it names off the LIVE name) AND strands the volumes forever — with two claimants the purge guard skips both bases and the orphan sweep never reclaims them. Two further fail-safe gates (#1638 principle) stand between the sweep and a `volume rm -f`: a candidate mounted by ANY container is skipped (`docker_utils.list_attached_volume_names`, fail-closed — an unreadable mount table skips the whole cycle rather than reclaiming blind), and it must be observed unattached for `ORPHAN_VOLUME_UNATTACHED_STRIKES` (3 ≈ 15 min) consecutive cycles, since `recreate_container_with_updated_config` removes the old container before creating the new one and leaves a live volume briefly unattached. Agents renamed before the column existed are healed at boot from Docker's own mount table (`_heal_renamed_volume_bases`, one-shot, idempotent, runs before the startup sweep). **Startup hook (#740, rewritten by #2523):** one-shot `LoopService.reconcile_after_restart()`. It used to be `mark_orphan_loops_interrupted()`, flipping every `agent_loops` row left `queued`/`running` to `interrupted` — correct while a loop was an in-process `asyncio.Task` whose state died with the process, and pure data loss now that the loop is advanced by execution terminals and lives entirely on its row. The reconcile leaves alone any loop with a non-terminal execution in flight (that terminal, or this service's own recovery of it, advances the loop), leaves a loop parked on `next_run_at` to the 5s due sweep, advances a loop whose open run already went terminal (the event was lost with the restart; re-arming there would dispatch a duplicate `run_number`), and re-arms only the loops with nothing in flight at all. **Loops auto-resume across a restart now**; nothing is marked `interrupted`. +**Retention resolution — floor-by-seed, not floor-by-default (#1638).** Each window resolves **DB row → code default**, except the keys listed in `config.ENV_BACKED_OPS_KEYS` (ent#478's `metrics_retention_days` and `metrics_daily_point_cap`), which resolve **DB row → env → code default** through `settings_service.resolve_ops_setting`. The env tier is opt-in per key precisely so the other windows' precedence — which `GET /api/settings/retention` documents — is unchanged, and env is a LIVE fallback rather than a one-time seed: the #2085 seeder skips a key whose variable is set, so a later change to the variable is still honoured and a saved row still wins over both. "Set" has to mean *an operator set it*: every backend compose file forwards the env-backed keys with an EMPTY default (`${METRICS_DAILY_POINT_CAP:-}`) and `.env.example` leaves them commented (trinity-enterprise#671) — with `:-100000`-style defaults and a copied example line the variable was set on every install, so the seeder never seeded the window, every source read `env`, and the Settings rows built on that source would have locked (`tests/unit/test_ent671_env_backed_ops_forwarding.py`). (Log archival separately reads `LOG_*`.) Because the code default is the fallback for every install that never wrote a row — the default state — it is read at *prune* time and must stay at the **widest** value: `OPS_SETTINGS_DEFAULTS` is a **safety floor, not a policy knob**. Lowering one retroactively hard-DELETEs the existing data of every un-configured install seconds after its next boot, silently (#1638, which cost a real instance ~3 months of history and shipped green because a test asserted the destructive default). The #1039 `COMMUNITY_RETENTION_FLOOR_DAYS=5` community floor therefore reaches **new installs only**, by *seeding* explicit rows into a fresh DB (`config.COMMUNITY_FRESH_INSTALL_SEED` → `database._seed_fresh_install_retention{,_engine}`, run inside `init_database()` after `init_schema` and **before** `_ensure_admin_user` — an empty `users` table is the fresh-install signal). The seed is fail-safe (never raises: `init_database()` runs at import, so raising would crash-loop boot) and idempotent (`INSERT OR IGNORE` / `on_conflict_do_nothing` — both migration locks fail open). **#2085 completes the mechanism for installs that are not fresh.** #1645 seeded rows on a fresh DB only, so every install that has ever *upgraded* rather than been created fresh still had no rows at all and resolved all 12 windows at prune time from the image — leaving a code comment as the only thing between a future edit to `OPS_SETTINGS_DEFAULTS` and the #1638 failure mode. `database._seed_retention_windows{,_engine}` now writes an explicit row for **every** `RETENTION_OPS_KEYS` member that has none, at **the value already in force**, on every boot and regardless of install age — so it is behaviourally inert (nothing prunes differently the day it runs) and needs no migration note. Key set is derived from `RETENTION_OPS_KEYS`, never a second hand-written list, so a window added later is covered the day it ships instead of quietly inheriting the image default forever (ent#433 added two, #2216 a third). **Ordering is load-bearing and pinned by test in both arms**: it MUST run after `_seed_fresh_install_retention` in `init_database()`, because both writers are insert-or-ignore and the first to reach a key wins — reversed, a fresh install would silently get the wide defaults (30/90/7/30) instead of the #1039 floor, i.e. the community floor deleted by the change meant to protect retention. **Stated tradeoff:** a seeded install stops inheriting later changes to the code default in *either* direction, so widening a window for existing installs becomes a deliberate migration rather than something that arrives silently with an image — the intended consequence, making retention explicit per-install config instead of implicit inheritance from whatever image is running. Same fail-safe contract as the #1638 seed (never raises; a skip leaves the install exactly where it is today). Note `backup_retention_days` is seeded too, which makes `OPS_SETTINGS_DEFAULTS`' value the one that lands in the DB for a key whose private reader (`effective_backup_retention_days`) falls back to its **own** module constant — the two are now parity-tested. `agent_soft_delete_retention_days` is **exempt from the floor in every edition**: its expiry destroys agent data volumes (#1581), making it a *recovery* window, not a log window. The floor is **not enforced** — any admin may widen a window via `PUT /api/settings/ops/config` (no clamp); the enterprise `retention` module clamps only *unentitled writes* to a **minimum** of 5 and sells the managed panel (audit/`updated_by`/hot-reload), not the capability. `POST /api/settings/ops/reset` **skips** `RETENTION_OPS_KEYS` (#1638 — "reset to defaults" must not silently change how much operator data is kept). **`RETENTION_OPS_KEYS` carries all 12 keys — 11 row windows + #2216's file-artifact `backup_retention_days`** — #1644 added `agent_reports_retention_days` and `operator_queue_retention_days`, which had been missing, so `/ops/reset` **deleted** those two rows while reporting `"retention windows unchanged"`, `GET /api/settings/retention` hid them, and boot logging never showed them; #1296 then added `agent_reminders_retention_days`, taking the row set to 8 (this doc said 7 in two places until #1833), ent#433 took it to 10 with `subscription_headroom_retention_days` (the new probe-history table) and `subscription_failure_event_retention_days` (which **converted** `subscription_rate_limit_events`' hardcoded 24h sweep into a real window — that table held the only durable record of real agent work hitting a provider rate limit and destroyed it daily with none of the three protections every sibling had), and ent#478 took it to 11 with `metrics_retention_days` (recorded business-metric points, a year by default). Membership means "is a retention window", NOT "gets the community floor" (that set is `COMMUNITY_FRESH_INSTALL_SEED`, still 4 — both ent#433 keys are deliberately excluded, since the 5-day floor would truncate the 7-day default read window while the UI labelled it 7 days). Surfaced read-only at `GET /api/settings/retention`, which reports per-key `source` (`db-row` vs `code-default`) plus the effective (clamped) `guard.max_rows`; `cleanup_service.log_effective_retention_windows()` logs the same at boot *before* the first sweep, and a prune hitting the per-transaction chunk size logs at WARNING (a routine trickle stays INFO). Runbook: [docs/migrations/RETENTION_DEFAULTS_2026-07.md](../../migrations/RETENTION_DEFAULTS_2026-07.md). **#1142 operator_queue retention** — DELETEs terminal `operator_queue` rows (acknowledged/cancelled/expired) past `operator_queue_retention_days` (default 90, `0`=off); `responded` rows use a more generous fixed floor (`OPERATOR_QUEUE_RESPONDED_MIN_RETENTION_DAYS`, 30d — they still carry an operator answer the 5s write-back loop must deliver), `pending` never deleted (`db.prune_operator_queue_terminal_items`, select-ids-then-delete, capped). This is the automatic complement to #1017's manual Clear-All (which only *hid* via `cleared_at`). `0` disables; `PRAGMA wal_checkpoint(TRUNCATE)` when any sweep reclaims rows. Also purges expired `idempotency_keys`. **There is no per-cycle row cap (#1644 — this doc previously claimed one).** `RETENTION_CHUNK_SIZE_PER_CYCLE=5000` bounds each *transaction*, and only sometimes the call. Counted at #1833 (this doc said "six of the seven", wrong in both terms — the denominator was 8, and the split 5/3; ent#433 makes it 10 and 7/3): **seven** accessors loop `while True` and drain the **entire** candidate set in one sweep — `prune_execution_logs`, `prune_execution_rows`, `cleanup_old_health_records`, `prune_agent_reports`, `prune_agent_reminders`, and ent#433's `prune_headroom_history` + `cleanup_old_rate_limit_events`. **Three** are genuinely capped per cycle: `prune_operator_queue_terminal_items` takes `limit=` directly, and the two soft-delete sweeps select their victims through `find_soft_deleted_{agents,schedules}_past_retention(limit=RETENTION_CHUNK_SIZE_PER_CYCLE)` before purging one by one. #1638 deleting 5352 rows in one startup sweep is the proof that chunking is not a cap on the first five — a real 5000 cap could not produce that number. So for those five the bound on destruction is the blast-radius guard below, not chunking; for the other three it is both, and the guard is still the one that stops at the *threshold* rather than at 5000. **#1581 volume reclaim** — at the #834 hard-purge (the instant the agent becomes unrecoverable, NOT at soft-delete) the agent's `agent-{name}-{workspace,public,shared}` Docker volumes are removed via `docker_utils.remove_agent_volumes` (double-guarded: exact name AND `trinity.agent-name`+`trinity.platform` labels must match, fail-closed; NotFound/in-use tolerated → retry). A separate bounded Docker-as-truth orphan sweep (`_sweep_orphan_agent_volumes`, ≤100 agents/cycle, 1h creation-grace) reclaims agent-data volumes whose ownership row is gone. Closes the unbounded volume leak (`volume_remove` previously had zero lifecycle callers). **Orphanhood is a DB question, not a volume-label question (#1664):** rename keeps the agent's volumes, so their name AND immutable `trinity.agent-name` label both stay at the pre-rename name — reading that as ownership marked a LIVE agent's home volume an orphan and force-removed it during any container-recreate gap (silent #1169 `data_paths` loss). Ownership is now resolved through `agent_ownership.volume_base_name` (NULL ⇒ `agent_name`; pinned to the pre-rename name in the same statement as the rename, frozen across re-renames) via `db.is_volume_base_reserved` — soft-deleted rows still match, so the recovery window is respected for a renamed agent too. The predicate is the **union of both identities** (`agent_name` OR `volume_base_name`), because a renamed agent owns volumes under **two** bases: its workspace keeps the old one, while `get_public_volume_name`/`get_shared_volume_name` name off the LIVE name, so `agent-{new}-public` appears if file-sharing is enabled after a rename (and that volume is unmounted whenever sharing is off, so the attached-check can't cover it). The union is strictly safer than the pre-#1664 `is_agent_name_reserved` it replaces — that predicate is exactly its first branch. The purge removes both bases for the same reason (deduped; one call for an un-renamed agent). **One row per base is the invariant, enforced at BOTH producers (#1671):** creation gates on `is_volume_base_reserved` (`crud.py`), and rename gates on it too — at the router (actionable 409, raised before the container is touched) and inside `db.rename_agent`'s transaction (the chokepoint that closes the check-then-write gap, #1445 pattern). Rename passes `exclude_agent=` so an agent renamed `B`→`A` (pin `B`) can still be renamed back to `B` — it already owns that base and the result has a single claimant. Leaving rename ungated let an ordinary swap (`x`→`x-old`, then `y`→`x`) mint two claimants of base `x`, which is the #1667 silent-adopt disclosure via `get_public_volume_name` (it names off the LIVE name) AND strands the volumes forever — with two claimants the purge guard skips both bases and the orphan sweep never reclaims them. Two further fail-safe gates (#1638 principle) stand between the sweep and a `volume rm -f`: a candidate mounted by ANY container is skipped (`docker_utils.list_attached_volume_names`, fail-closed — an unreadable mount table skips the whole cycle rather than reclaiming blind), and it must be observed unattached for `ORPHAN_VOLUME_UNATTACHED_STRIKES` (3 ≈ 15 min) consecutive cycles, since `recreate_container_with_updated_config` removes the old container before creating the new one and leaves a live volume briefly unattached. **Stack-scoped since #3214:** Docker volumes are daemon-global while orphanhood is read from ONE stack's DB, so a second stack on a shared daemon used to reclaim every other stack's unattached agent volumes (207 of 238 on one developer host). Every agent data volume is now created through `docker_utils.agent_volume_labels`, which stamps `trinity.instance=` (`instance_identity.get_instance_id` — the same durable id the alert label abbreviates); the sweep lists candidates on that label's key AND value (`list_agent_data_volumes(instance_id)`), skips the cycle when the id is unresolvable, and `is_reclaimable_agent_volume` refuses a volume labelled for another instance on the purge path too (two stacks may both have an agent `alpha`). **Legacy volumes fail closed:** labels are immutable, so pre-#3214 volumes carry none and the sweep never reclaims them; unowned+unattached ones are named in one WARNING per change of that set for a human (`_report_unlabelled_orphan_volumes`). Every reclaim/removal logs at WARNING as unrecoverable, naming the volumes (#1638). Agents renamed before the column existed are healed at boot from Docker's own mount table (`_heal_renamed_volume_bases`, one-shot, idempotent, runs before the startup sweep). **Startup hook (#740, rewritten by #2523):** one-shot `LoopService.reconcile_after_restart()`. It used to be `mark_orphan_loops_interrupted()`, flipping every `agent_loops` row left `queued`/`running` to `interrupted` — correct while a loop was an in-process `asyncio.Task` whose state died with the process, and pure data loss now that the loop is advanced by execution terminals and lives entirely on its row. The reconcile leaves alone any loop with a non-terminal execution in flight (that terminal, or this service's own recovery of it, advances the loop), leaves a loop parked on `next_run_at` to the 5s due sweep, advances a loop whose open run already went terminal (the event was lost with the restart; re-arming there would dispatch a duplicate `run_number`), and re-arms only the loops with nothing in flight at all. **Loops auto-resume across a restart now**; nothing is marked `interrupted`. ### Automatic Database Backups (#2216) diff --git a/docs/memory/learnings/2026-09-24-a-second-stack-reads-other-stacks-volumes-as-orphans.md b/docs/memory/learnings/2026-09-24-a-second-stack-reads-other-stacks-volumes-as-orphans.md index 9ab6a3bb9..11c091c4e 100644 --- a/docs/memory/learnings/2026-09-24-a-second-stack-reads-other-stacks-volumes-as-orphans.md +++ b/docs/memory/learnings/2026-09-24-a-second-stack-reads-other-stacks-volumes-as-orphans.md @@ -1,3 +1,4 @@ ## 2026-09-24 — pitfall — A second Trinity stack on the same Docker daemon reads every other stack's agent volumes as orphans **Context**: trinity-enterprise#465's local demo. `cleanup_service._sweep_orphan_agent_volumes` (#1581) reclaims an `agent-*` volume that (1) no ownership row claims, (2) no container mounts, (3) for three consecutive cycles, past a 1h creation grace — up to 100 per cycle, force-removed. All three conditions are evaluated against the stack's OWN database and the daemon's WHOLE volume list. A stack booted with a fresh database (a demo, a `--keep` verify run, a second worktree's dev stack) owns no rows, so every labelled agent volume on the host that nothing currently mounts is an orphan to it; on the developer machine where this surfaced that was 207 of 238. There is no setting that turns the sweep off, and `/verify-local`'s overlay does not neutralise it — a run that stays up past ~15 minutes is inside the window. **Lesson**: Docker volumes are host-global state; a sweep that decides "unowned" from a per-stack database is only safe when that database is the only one on the daemon. Before booting any second stack on a shared daemon, count `docker volume ls --filter label=trinity.agent-name` minus the attached ones — if it is non-zero, disable that sweep for the extra stack (a demo-only `sitecustomize` import hook that no-ops the method works without touching the branch) or scope the sweep to volumes this stack created. The structural fix is a stack-identity label on every agent volume, checked by the sweep before it reclaims anything. +**Follow-up**: filed and fixed as trinity#3214 — agent data volumes now carry `trinity.instance=` and the sweep reclaims only its own; pre-#3214 (unlabelled) volumes are never auto-reclaimed (named in a WARNING instead). The manual workaround above is still needed for volumes created before that fix. diff --git a/src/backend/services/agent_service/crud.py b/src/backend/services/agent_service/crud.py index cf9ae9310..0f07d8459 100644 --- a/src/backend/services/agent_service/crud.py +++ b/src/backend/services/agent_service/crud.py @@ -30,7 +30,8 @@ get_agent_status_from_container, ) from services.docker_utils import ( - volume_get, volume_create, containers_run, container_remove + volume_get, volume_create, containers_run, container_remove, + agent_volume_labels ) from services.agent_runtime_state import clear_agent_breakers, clear_agent_runtime_state from services.template_service import ( @@ -2053,10 +2054,7 @@ async def _workspace_volume_mount(config: AgentConfig, volumes: dict) -> None: except docker.errors.NotFound: await volume_create( name=agent_volume_name, - labels={ - 'trinity.platform': 'agent-workspace', - 'trinity.agent-name': config.name - } + labels=agent_volume_labels(config.name, 'agent-workspace'), ) volumes[agent_volume_name] = {'bind': '/home/developer', 'mode': 'rw'} # Persistent workspace @@ -2089,10 +2087,7 @@ async def _shared_folder_mounts( except docker.errors.NotFound: await volume_create( name=shared_volume_name, - labels={ - 'trinity.platform': 'agent-shared', - 'trinity.agent-name': config.name - } + labels=agent_volume_labels(config.name, 'agent-shared'), ) volume_created = True @@ -2136,10 +2131,7 @@ async def _public_volume_mount(config: AgentConfig, volumes: dict) -> None: except docker.errors.NotFound: await volume_create( name=public_volume_name, - labels={ - 'trinity.platform': 'agent-public', - 'trinity.agent-name': config.name, - }, + labels=agent_volume_labels(config.name, 'agent-public'), ) public_volume_created = True diff --git a/src/backend/services/agent_service/deploy.py b/src/backend/services/agent_service/deploy.py index 7b63e4e4b..6f0dd9a16 100644 --- a/src/backend/services/agent_service/deploy.py +++ b/src/backend/services/agent_service/deploy.py @@ -35,7 +35,7 @@ collect_mcp_credential_warnings, ) from services.docker_service import get_agent_container -from services.docker_utils import container_stop, container_start +from services.docker_utils import container_stop, container_start, agent_volume_labels from utils.helpers import sanitize_agent_name from services.settings_service import get_agent_quota_for_role from redis_breaker_util import SingleFlightLock # #1920 shared single-flight lock @@ -544,10 +544,7 @@ def _ensure_fresh_workspace_volume(client, workspace_vol: str, version_name: str client.volumes.create( name=workspace_vol, - labels={ - "trinity.platform": "agent-workspace", - "trinity.agent-name": version_name, - }, + labels=agent_volume_labels(version_name, "agent-workspace"), ) diff --git a/src/backend/services/agent_service/lifecycle.py b/src/backend/services/agent_service/lifecycle.py index e291f2f4a..0db2c33ab 100644 --- a/src/backend/services/agent_service/lifecycle.py +++ b/src/backend/services/agent_service/lifecycle.py @@ -24,7 +24,8 @@ ) from services.docker_utils import ( container_stop, container_remove, container_start, container_reload, - volume_get, volume_create, containers_run, image_get, container_stop + volume_get, volume_create, containers_run, image_get, container_stop, + agent_volume_labels ) from services.agent_service.helpers import validate_base_image from services.agent_runtime_state import clear_agent_breakers @@ -1379,10 +1380,7 @@ async def _provision_folders_and_run_agent_container( except docker.errors.NotFound: await volume_create( name=shared_volume_name, - labels={ - 'trinity.platform': 'agent-shared', - 'trinity.agent-name': agent_name - } + labels=agent_volume_labels(agent_name, 'agent-shared'), ) volume_created = True @@ -1421,10 +1419,7 @@ async def _provision_folders_and_run_agent_container( except docker.errors.NotFound: await volume_create( name=public_volume_name, - labels={ - 'trinity.platform': 'agent-public', - 'trinity.agent-name': agent_name, - }, + labels=agent_volume_labels(agent_name, 'agent-public'), ) public_volume_created = True diff --git a/src/backend/services/cleanup_service.py b/src/backend/services/cleanup_service.py index ef5a58220..be178bbb9 100644 --- a/src/backend/services/cleanup_service.py +++ b/src/backend/services/cleanup_service.py @@ -843,6 +843,9 @@ def __init__(self, poll_interval: int = CLEANUP_INTERVAL_SECONDS): # unowned. In-process only (a restart just restarts the count — the # safe direction). self._unattached_volume_strikes: Dict[str, int] = {} + # #3214: the legacy unlabelled orphan volumes last named in a WARNING, + # so the report repeats only when that set changes. + self._reported_unlabelled_volumes: frozenset = frozenset() def start(self): """Start the background cleanup loop.""" @@ -1821,15 +1824,44 @@ async def _sweep_orphan_agent_volumes(self, report: CleanupReport) -> None: Removal still goes through the name+label guard (`remove_agent_volumes`) as a last line of defence. Also carries a creation-grace window (creation writes the volume before the ownership row). + + **Stack scope (#3214).** Docker volumes are daemon-global while (1) is + answered from THIS stack's database, so a second stack on the same + daemon used to read every other stack's unattached agent volumes as + its own orphans and force-remove them. Candidates are therefore listed + on the `trinity.instance=` label (key AND value): + a volume another stack created is never a candidate. If this stack's + id cannot be resolved, the sweep does nothing. + + **Legacy volumes — fail-closed.** Volumes created before the instance + label existed carry none, and Docker labels cannot be added later. The + sweep NEVER reclaims an unlabelled volume: on a shared daemon it cannot + say whose it is. Unlabelled ones that are also unowned and unattached + are named in one WARNING (repeated only when that set changes) for a + human to remove. This stack's own agents still lose their volumes at + the retention purge, which is driven by the ownership row, labelled or + not. """ try: from services.docker_utils import ( + AGENT_VOLUME_INSTANCE_LABEL, list_agent_data_volumes, list_attached_volume_names, remove_agent_volumes, ) + from services.instance_identity import get_instance_id - volumes = await list_agent_data_volumes() + instance_id = get_instance_id() + if not instance_id: + logger.warning( + "[#1581] skipping orphan-volume sweep: this instance's id is " + "unavailable (cannot tell its volumes from another stack's)" + ) + return + + await self._report_unlabelled_orphan_volumes(AGENT_VOLUME_INSTANCE_LABEL) + + volumes = await list_agent_data_volumes(instance_id) if not volumes: self._unattached_volume_strikes.clear() return @@ -1900,9 +1932,13 @@ async def _sweep_orphan_agent_volumes(self, report: CleanupReport) -> None: for stale in set(self._unattached_volume_strikes) - seen_volumes: self._unattached_volume_strikes.pop(stale, None) - for volume_base in orphan_agents: + reclaimed_bases = [] + for volume_base in sorted(orphan_agents): try: - reclaimed += await remove_agent_volumes(volume_base) + n = await remove_agent_volumes(volume_base, instance_id) + reclaimed += n + if n: + reclaimed_bases.append(volume_base) except Exception as e: logger.warning( f"[#1581] orphan volume reclaim for {volume_base} failed: {e}" @@ -1910,12 +1946,65 @@ async def _sweep_orphan_agent_volumes(self, report: CleanupReport) -> None: report.orphan_agent_volumes_reclaimed = reclaimed if reclaimed > 0: - logger.info( - f"[#1581] reclaimed {reclaimed} orphaned agent volume(s)" + # Always WARNING (#3214, the #1638 rule the purge path already + # follows): this destroys agent data and is unrecoverable — + # there is no routine, unremarkable one. Each volume is also + # named on its own line by `remove_agent_volumes`. + logger.warning( + f"[#1581] reclaimed {reclaimed} orphaned agent volume(s) of " + f"this instance — unrecoverable: " + + ", ".join(f"agent-{b}-*" for b in reclaimed_bases) ) except Exception as e: logger.error(f"[Cleanup] Error in orphan agent-volume sweep: {e}") + async def _report_unlabelled_orphan_volumes(self, instance_label: str) -> None: + """#3214: name the legacy (unlabelled) agent volumes that look orphaned + — no ownership row, not mounted — for a human. Never reclaims: without + an instance label nothing says which stack's data a volume holds. + + One WARNING per change of that set, not one per 5-minute cycle. Never + raises. + """ + try: + from services.docker_utils import ( + list_all_agent_data_volumes, + list_attached_volume_names, + ) + + volumes = await list_all_agent_data_volumes() + attached = await list_attached_volume_names() + if attached is None: + return + names = [] + for volume in volumes: + labels = (volume.attrs.get("Labels") or {}) if getattr(volume, "attrs", None) else {} + if labels.get(instance_label): + continue + base = labels.get("trinity.agent-name") + name = getattr(volume, "name", None) + if not base or not name or name in attached: + continue + if db.is_volume_base_reserved(base): + continue + names.append(name) + current = frozenset(names) + if current == self._reported_unlabelled_volumes: + return + self._reported_unlabelled_volumes = current + if current: + shown = sorted(current) + logger.warning( + f"[#3214] {len(shown)} unlabelled agent volume(s) with no owner " + f"and no container — NOT reclaimed automatically (created " + f"before stack-identity labels; on a shared Docker daemon they " + f"may belong to another stack). Review and remove by hand: " + + ", ".join(shown[:50]) + + (f" … (+{len(shown) - 50} more)" if len(shown) > 50 else "") + ) + except Exception as e: + logger.debug(f"[#3214] unlabelled-volume report skipped: {e}") + @staticmethod def _volume_age_seconds(created_raw: Optional[str], now) -> Optional[float]: """Age in seconds of a Docker volume from its ``CreatedAt`` string. diff --git a/src/backend/services/docker_utils.py b/src/backend/services/docker_utils.py index 9a26381f8..3569dfc07 100644 --- a/src/backend/services/docker_utils.py +++ b/src/backend/services/docker_utils.py @@ -349,6 +349,30 @@ async def volume_remove(volume, force: bool = False) -> None: # Label key present on every agent data volume — the cheap list filter for the # orphan sweep (catches all three platforms in one call). _AGENT_VOLUME_LABEL_KEY = "trinity.agent-name" +# #3214: the stack that created the volume — this install's `installation_id` +# (`instance_identity.get_instance_id`). Docker volumes are daemon-global, so a +# second stack on the same daemon must be able to tell its own volumes from +# another stack's; the orphan sweep reclaims only volumes carrying OUR value. +# Labels are immutable after creation, so volumes created before this label +# existed carry none — they are never auto-reclaimed by the sweep. +AGENT_VOLUME_INSTANCE_LABEL = "trinity.instance" + + +def _resolve_instance_id() -> Optional[str]: + from services.instance_identity import get_instance_id + return get_instance_id() + + +def agent_volume_labels(volume_base: str, platform: str) -> Dict[str, str]: + """The labels every agent data volume is created with (#3214) — the ONE + builder, so no creation path can forget the instance label. An unresolvable + instance id yields an unlabelled volume (never a wrong label): such a + volume is simply never an orphan-sweep candidate.""" + labels = {"trinity.platform": platform, "trinity.agent-name": volume_base} + instance_id = _resolve_instance_id() + if instance_id: + labels[AGENT_VOLUME_INSTANCE_LABEL] = instance_id + return labels # Mount destination of the agent's durable home volume (#1169) — the bind every # `agent-{base}-workspace` volume is attached at. AGENT_HOME_PATH = "/home/developer" @@ -361,7 +385,7 @@ def _volume_labels(volume) -> Dict[str, str]: return {} -def is_reclaimable_agent_volume(volume, volume_base: str) -> bool: +def is_reclaimable_agent_volume(volume, volume_base: str, instance_id: Optional[str] = None) -> bool: """Fail-closed guard: True ONLY if ``volume`` is one of ``volume_base``'s agent data volumes (name AND label both match; #1581 double-guard). @@ -379,6 +403,14 @@ def is_reclaimable_agent_volume(volume, volume_base: str) -> bool: `list_attached_volume_names` for in-use — and `volume_base` must therefore come from `db.get_volume_base_name(agent)`, never from f-stringing an agent's current name. + + Stack scope (#3214): a volume labelled for ANOTHER instance is never + reclaimable, and a labelled volume is refused when ``instance_id`` (ours) + is unknown — two stacks on one daemon can both have an agent named + ``alpha``, so name + label alone cannot say whose data it is. An + UNLABELLED (pre-#3214) volume keeps the guard as it was: the retention + purge reaches it only from this stack's own ownership row, and the orphan + sweep never lists one. """ if not volume_base: return False @@ -390,10 +422,13 @@ def is_reclaimable_agent_volume(volume, volume_base: str) -> bool: return False if labels.get("trinity.platform") not in _AGENT_VOLUME_PLATFORMS: return False + owner = labels.get(AGENT_VOLUME_INSTANCE_LABEL) + if owner is not None and (not instance_id or owner != instance_id): + return False return True -async def remove_agent_volumes(volume_base: str) -> int: +async def remove_agent_volumes(volume_base: str, instance_id: Optional[str] = None) -> int: """Remove the data volumes (workspace/public/shared) under ``volume_base`` — #1581. @@ -407,9 +442,16 @@ async def remove_agent_volumes(volume_base: str) -> int: a renamed agent's volumes keep the pre-rename base, so callers holding an agent name must resolve it via ``db.get_volume_base_name(agent)`` — while the ownership row still exists. + + ``instance_id`` is this stack's identity (#3214), resolved here when not + given; a volume labelled for another stack is refused by the guard. + Every removal is logged at WARNING as unrecoverable — it destroys the + agent's data, so no instance of it is routine (#1638). """ if docker_client is None: return 0 + if instance_id is None: + instance_id = _resolve_instance_id() removed = 0 for suffix in _AGENT_VOLUME_SUFFIXES: vol_name = f"agent-{volume_base}-{suffix}" @@ -420,16 +462,19 @@ async def remove_agent_volumes(volume_base: str) -> int: except Exception as e: logger.warning(f"[#1581] could not read volume {vol_name}: {e}") continue - if not is_reclaimable_agent_volume(volume, volume_base): + if not is_reclaimable_agent_volume(volume, volume_base, instance_id): logger.error( f"[#1581] refusing to remove volume {vol_name}: guard " - f"(name+label) did not match base {volume_base}" + f"(name+label+instance) did not match base {volume_base}" ) continue try: await volume_remove(volume, force=True) removed += 1 - logger.info(f"[#1581] removed agent volume {vol_name}") + logger.warning( + f"[#1581] removed agent volume {vol_name} — its data is " + f"unrecoverable" + ) except docker.errors.NotFound: continue except docker.errors.APIError as e: @@ -527,21 +572,41 @@ def volume_base_from_workspace_volume(volume_name: str) -> Optional[str]: return base or None -async def list_agent_data_volumes() -> List[Any]: - """All agent data volumes across the fleet (any agent), by label key. +async def list_agent_data_volumes(instance_id: str) -> List[Any]: + """THIS stack's agent data volumes (any agent) — the #1581 orphan sweep's + candidates. - Used by the #1581 orphan sweep. Returns raw volume objects — callers read + Filtered at the daemon on the agent label key AND the instance label's + key+value (#3214): Docker volumes are daemon-global, and a volume another + stack created must never be a candidate here, whatever its ownership looks + like from this stack's database. Returns raw volume objects — callers read ``.name`` / ``.attrs`` (``Labels['trinity.agent-name']``, ``CreatedAt``). """ + if docker_client is None or not instance_id: + return [] + return await _list_volumes( + [_AGENT_VOLUME_LABEL_KEY, f"{AGENT_VOLUME_INSTANCE_LABEL}={instance_id}"] + ) + + +async def list_all_agent_data_volumes() -> List[Any]: + """Every agent data volume on the daemon, by label key — ANY stack's. + + Never a removal candidate list (#3214): the orphan sweep reads it only to + NAME legacy unlabelled volumes for a human. Reclaiming from it is the bug + #3214 fixed. + """ if docker_client is None: return [] + return await _list_volumes([_AGENT_VOLUME_LABEL_KEY]) + + +async def _list_volumes(label_filters: List[str]) -> List[Any]: loop = asyncio.get_event_loop() try: return await loop.run_in_executor( _docker_executor, - lambda: docker_client.volumes.list( - filters={"label": _AGENT_VOLUME_LABEL_KEY} - ), + lambda: docker_client.volumes.list(filters={"label": label_filters}), ) except Exception as e: logger.error(f"[#1581] listing agent data volumes failed: {e}") diff --git a/src/backend/services/instance_identity.py b/src/backend/services/instance_identity.py index 6c7c13bc5..2c2193f9b 100644 --- a/src/backend/services/instance_identity.py +++ b/src/backend/services/instance_identity.py @@ -172,3 +172,29 @@ def get_instance_label() -> Optional[str]: return derived return _label_from_installation_id() + + +def get_instance_id() -> Optional[str]: + """This install's durable, unambiguous identity — the full ``installation_id`` + UUID — or ``None`` when it cannot be read. Never raises. + + The machine counterpart of :func:`get_instance_label` (#3214): the label is + a human name that may be absent or shared by two stacks, which is fine for + attributing an alert and unsafe for deciding whose data a volume holds. The + #1581 orphan sweep stamps this on every agent data volume and reclaims only + volumes carrying it, so a second stack on a shared Docker daemon can never + read another stack's volumes as its own orphans. + + One source, not a second notion: it is the same id the label's last tier + abbreviates, read through the same write-once accessor (ent#545), so two + workers resolve the same value. Like that tier it can MINT the id on an + install that has none yet; a ``None`` here makes every caller fail closed + (no label written, nothing reclaimed). + """ + try: + from services.operator_intake_service import get_or_create_installation_id + installation_id = (get_or_create_installation_id() or "").strip() + except Exception: # noqa: BLE001 — unresolvable is "no identity", never a raise + logger.debug("instance id: installation_id unavailable", exc_info=True) + return None + return installation_id or None diff --git a/tests/unit/test_1581_agent_volume_reclaim.py b/tests/unit/test_1581_agent_volume_reclaim.py index 1751bf050..6ec747cf3 100644 --- a/tests/unit/test_1581_agent_volume_reclaim.py +++ b/tests/unit/test_1581_agent_volume_reclaim.py @@ -263,7 +263,7 @@ async def test_orphan_sweep_reclaims_only_rowless_past_grace(self): # renamed agent owns volumes named after its former self. db.is_volume_base_reserved.side_effect = lambda base: base == "live-agent" - async def fake_remove(name): + async def fake_remove(name, instance_id=None): # #3214: + this instance return 1 with patch.object(cs, "db", db), \ @@ -279,7 +279,7 @@ async def fake_remove(name): for _ in range(cs.ORPHAN_VOLUME_UNATTACHED_STRIKES): await svc._sweep_orphan_agent_volumes(report) - rm.assert_awaited_once_with("orphan-old") + assert [c.args[0] for c in rm.await_args_list] == ["orphan-old"] assert report.orphan_agent_volumes_reclaimed == 1 @pytest.mark.asyncio diff --git a/tests/unit/test_1664_renamed_agent_volume_safety.py b/tests/unit/test_1664_renamed_agent_volume_safety.py index 1cb829e76..5f504e412 100644 --- a/tests/unit/test_1664_renamed_agent_volume_safety.py +++ b/tests/unit/test_1664_renamed_agent_volume_safety.py @@ -263,7 +263,7 @@ async def test_orphan_needs_consecutive_unattached_cycles(self): rm.assert_not_awaited() rm = await self._run_cycles(svc, report, db=db, vols=vols, attached=set()) - rm.assert_awaited_once_with("gone") + assert [c.args[0] for c in rm.await_args_list] == ["gone"] # #3214: + instance id assert report.orphan_agent_volumes_reclaimed == 1 @pytest.mark.asyncio diff --git a/tests/unit/test_2669_minting_accessor_callers.py b/tests/unit/test_2669_minting_accessor_callers.py index 9f9ef7b3b..c4cf7e174 100644 --- a/tests/unit/test_2669_minting_accessor_callers.py +++ b/tests/unit/test_2669_minting_accessor_callers.py @@ -123,6 +123,12 @@ "the canary alert label — a DOCUMENTED deliberate write (#1987): the " "docstring says 'This tier can WRITE' and why a read-only variant would " "return None on exactly the un-configured install the tier exists to label", + ("services/instance_identity.py", "get_instance_id", + "get_or_create_installation_id"): + "the agent-volume ownership stamp (#3214) — a volume created while the " + "id does not exist yet would carry no owner and could never be reclaimed " + "by the #1581 sweep, so the create path must make it exist; the id is the " + "same write-once value the label tier above already mints (ent#545)", ("services/telemetry_sharing_service.py", "set_consent", "get_or_mint_sharing_id"): "opting IN mints the egress id (opting out deletes it in the same " diff --git a/tests/unit/test_3214_volume_instance_scope.py b/tests/unit/test_3214_volume_instance_scope.py new file mode 100644 index 000000000..76a796853 --- /dev/null +++ b/tests/unit/test_3214_volume_instance_scope.py @@ -0,0 +1,276 @@ +"""#3214 — the #1581 orphan agent-volume sweep is scoped to THIS stack. + +Docker volumes are daemon-global; the sweep's ownership check reads one stack's +database. Before this fix a second stack on the same daemon read every other +stack's unattached agent volumes as its own orphans and force-removed them, +logged at INFO as routine housekeeping (207 of 238 on one developer machine, +docs/memory/learnings/2026-09-24-a-second-stack-reads-other-stacks-volumes-as-orphans.md). + +Pinned here: + * every agent data volume is created with a `trinity.instance` label carrying + this install's durable id (`installation_id`, via instance_identity); + * the sweep's candidate listing filters on that label's key AND value, so a + foreign volume is never a candidate; + * the last-line guard refuses a volume labelled for another instance — on the + retention-purge path too (two stacks can both have an agent named `alpha`); + * legacy (unlabelled) volumes are never auto-reclaimed by the sweep + (fail-closed); unowned+unattached ones are named in a WARNING for a human; + * the reclaim is logged at WARNING as unrecoverable, naming what was removed; + * the original #1581 case (this stack's own orphan) is still reclaimed. +""" +from __future__ import annotations + +import importlib.util +import logging +import sys +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, Mock, patch + +import pytest + +_project_root = Path(__file__).resolve().parents[2] +_backend = str(_project_root / "src" / "backend") +if _backend not in sys.path: + sys.path.insert(0, _backend) + +OURS = "11111111-1111-4111-8111-111111111111" +THEIRS = "22222222-2222-4222-8222-222222222222" + + +def _load_docker_utils(): + mock_client = Mock() + with patch.dict("sys.modules", {"services.docker_service": Mock(docker_client=mock_client)}): + spec = importlib.util.spec_from_file_location("docker_utils", f"{_backend}/services/docker_utils.py") + mod = importlib.util.module_from_spec(spec) + mod.docker_client = mock_client + spec.loader.exec_module(mod) + return mod, mock_client + + +def _volume(name, base, platform="agent-workspace", instance=OURS, created="2020-01-01T00:00:00Z"): + v = Mock() + v.name = name + labels = {"trinity.agent-name": base, "trinity.platform": platform} + if instance is not None: + labels["trinity.instance"] = instance + v.attrs = {"Labels": labels, "CreatedAt": created} + v.remove = Mock() + return v + + +# --------------------------------------------------------------------------- # +# One identity source +# --------------------------------------------------------------------------- # +@pytest.mark.unit +class TestInstanceId: + def test_is_the_full_installation_id(self, monkeypatch): + import services.operator_intake_service as ois + from services import instance_identity + monkeypatch.setattr(ois, "get_or_create_installation_id", lambda: OURS) + assert instance_identity.get_instance_id() == OURS + + def test_is_none_when_unresolvable_and_never_raises(self, monkeypatch): + import services.operator_intake_service as ois + from services import instance_identity + + def boom(): + raise RuntimeError("db down") + + monkeypatch.setattr(ois, "get_or_create_installation_id", boom) + assert instance_identity.get_instance_id() is None + monkeypatch.setattr(ois, "get_or_create_installation_id", lambda: "") + assert instance_identity.get_instance_id() is None + + +# --------------------------------------------------------------------------- # +# Creation: every path stamps the label +# --------------------------------------------------------------------------- # +@pytest.mark.unit +class TestLabelsAtCreation: + def test_the_label_builder_stamps_this_instance(self): + du, _ = _load_docker_utils() + with patch.object(du, "_resolve_instance_id", lambda: OURS): + labels = du.agent_volume_labels("alpha", "agent-public") + assert labels == {"trinity.platform": "agent-public", "trinity.agent-name": "alpha", + "trinity.instance": OURS} + + def test_an_unresolvable_id_creates_an_unlabelled_volume_never_a_wrong_one(self): + du, _ = _load_docker_utils() + with patch.object(du, "_resolve_instance_id", lambda: None): + labels = du.agent_volume_labels("alpha", "agent-workspace") + assert "trinity.instance" not in labels + + def test_no_creation_site_builds_volume_labels_by_hand(self): + """Every agent data volume goes through `agent_volume_labels`: a site + that spells the platform label itself is a site that can forget the + instance label. Scans the backend for agent-data platform literals + outside the builder.""" + import re + offenders = [] + for path in Path(_backend, "services").rglob("*.py"): + if path.name == "docker_utils.py": + continue + text = path.read_text() + for m in re.finditer(r"['\"]trinity\.platform['\"]\s*:\s*['\"]agent-(workspace|public|shared)['\"]", text): + offenders.append(f"{path.relative_to(_backend)}:{text[:m.start()].count(chr(10)) + 1}") + assert offenders == [], f"build agent volume labels with agent_volume_labels(): {offenders}" + + +# --------------------------------------------------------------------------- # +# Guard: a foreign volume is never reclaimable — sweep OR purge +# --------------------------------------------------------------------------- # +@pytest.mark.unit +class TestGuard: + def setup_method(self): + self.du, _ = _load_docker_utils() + + def test_own_labelled_volume_is_reclaimable(self): + v = _volume("agent-alpha-workspace", "alpha") + assert self.du.is_reclaimable_agent_volume(v, "alpha", instance_id=OURS) is True + + def test_a_volume_labelled_for_another_instance_is_refused(self): + v = _volume("agent-alpha-workspace", "alpha", instance=THEIRS) + assert self.du.is_reclaimable_agent_volume(v, "alpha", instance_id=OURS) is False + + def test_a_labelled_volume_is_refused_when_our_id_is_unknown(self): + v = _volume("agent-alpha-workspace", "alpha", instance=THEIRS) + assert self.du.is_reclaimable_agent_volume(v, "alpha", instance_id=None) is False + + def test_a_legacy_unlabelled_volume_keeps_todays_guard(self): + """The purge path is driven by this stack's own ownership row.""" + v = _volume("agent-alpha-workspace", "alpha", instance=None) + assert self.du.is_reclaimable_agent_volume(v, "alpha", instance_id=OURS) is True + + @pytest.mark.asyncio + async def test_purge_does_not_remove_another_stacks_same_named_agent(self): + du, client = _load_docker_utils() + foreign = _volume("agent-alpha-workspace", "alpha", instance=THEIRS) + client.volumes.get.side_effect = lambda n: foreign if n == "agent-alpha-workspace" else (_ for _ in ()).throw( + __import__("docker").errors.NotFound("gone")) + with patch.object(du, "_resolve_instance_id", lambda: OURS): + removed = await du.remove_agent_volumes("alpha") + assert removed == 0 + foreign.remove.assert_not_called() + + @pytest.mark.asyncio + async def test_each_removed_volume_is_logged_as_unrecoverable(self, caplog): + du, client = _load_docker_utils() + own = _volume("agent-alpha-workspace", "alpha") + client.volumes.get.side_effect = lambda n: own if n == "agent-alpha-workspace" else (_ for _ in ()).throw( + __import__("docker").errors.NotFound("gone")) + with patch.object(du, "_resolve_instance_id", lambda: OURS), caplog.at_level(logging.INFO): + assert await du.remove_agent_volumes("alpha") == 1 + lines = [r for r in caplog.records if "agent-alpha-workspace" in r.getMessage() and "removed" in r.getMessage()] + assert lines and all(r.levelno == logging.WARNING for r in lines) + assert "unrecoverable" in lines[0].getMessage() + + +# --------------------------------------------------------------------------- # +# Enumeration: key AND value +# --------------------------------------------------------------------------- # +@pytest.mark.unit +class TestEnumeration: + @pytest.mark.asyncio + async def test_the_sweep_listing_filters_on_this_instance(self): + du, client = _load_docker_utils() + client.volumes.list.return_value = [] + await du.list_agent_data_volumes(instance_id=OURS) + filters = client.volumes.list.call_args.kwargs["filters"] + assert f"trinity.instance={OURS}" in filters["label"] + assert "trinity.agent-name" in filters["label"] + + +# --------------------------------------------------------------------------- # +# The sweep +# --------------------------------------------------------------------------- # +def _service(): + from services.cleanup_service import CleanupService + svc = CleanupService(poll_interval=300) + return svc + + +async def _sweep(svc, *, scoped, unscoped=None, attached=frozenset(), owned=frozenset(), instance=OURS, cycles=None): + from services.cleanup_service import CleanupReport + import services.cleanup_service as cs + report = CleanupReport() + db = MagicMock() + db.is_volume_base_reserved.side_effect = lambda base: base in owned + removed = [] + + async def fake_remove(base, instance_id=None): + removed.append((base, instance_id)) + return 1 + + async def fake_list(instance_id=None): + # The real listing filters key+value at the daemon; model that here. + return [v for v in scoped if (v.attrs["Labels"].get("trinity.instance") == instance_id)] + + with patch.object(cs, "db", db), \ + patch("services.instance_identity.get_instance_id", lambda: instance), \ + patch("services.docker_utils.list_agent_data_volumes", AsyncMock(side_effect=fake_list)), \ + patch("services.docker_utils.list_all_agent_data_volumes", + AsyncMock(return_value=list(unscoped if unscoped is not None else scoped))), \ + patch("services.docker_utils.list_attached_volume_names", AsyncMock(return_value=set(attached))), \ + patch("services.docker_utils.remove_agent_volumes", AsyncMock(side_effect=fake_remove)): + for _ in range(cycles or cs.ORPHAN_VOLUME_UNATTACHED_STRIKES): + await svc._sweep_orphan_agent_volumes(report) + return report, removed + + +@pytest.mark.unit +class TestSweep: + @pytest.mark.asyncio + async def test_a_foreign_volume_survives_any_number_of_cycles(self): + """AC2: labelled for another instance, unattached, old, 6 cycles.""" + foreign = _volume("agent-alpha-workspace", "alpha", instance=THEIRS) + report, removed = await _sweep(_service(), scoped=[foreign], cycles=6) + assert removed == [] + assert report.orphan_agent_volumes_reclaimed == 0 + + @pytest.mark.asyncio + async def test_this_stacks_own_orphan_is_still_reclaimed(self): + """AC5: the original #1581 case.""" + own = _volume("agent-gone-workspace", "gone") + report, removed = await _sweep(_service(), scoped=[own]) + assert removed == [("gone", OURS)] + assert report.orphan_agent_volumes_reclaimed == 1 + + @pytest.mark.asyncio + async def test_a_legacy_unlabelled_orphan_is_never_reclaimed(self): + """AC3: fail-closed for volumes created before the label existed.""" + legacy = _volume("agent-old-workspace", "old", instance=None) + report, removed = await _sweep(_service(), scoped=[legacy], unscoped=[legacy], cycles=6) + assert removed == [] + + @pytest.mark.asyncio + async def test_legacy_orphans_are_named_once_in_a_warning(self, caplog): + legacy = _volume("agent-old-workspace", "old", instance=None) + owned_legacy = _volume("agent-kept-workspace", "kept", instance=None) + attached_legacy = _volume("agent-used-workspace", "used", instance=None) + svc = _service() + with caplog.at_level(logging.WARNING): + await _sweep(svc, scoped=[], unscoped=[legacy, owned_legacy, attached_legacy], + owned={"kept"}, attached={"agent-used-workspace"}, cycles=3) + warnings = [r.getMessage() for r in caplog.records + if r.levelno == logging.WARNING and "unlabelled" in r.getMessage()] + assert len(warnings) == 1 # once, not every cycle + assert "agent-old-workspace" in warnings[0] + assert "agent-kept-workspace" not in warnings[0] # owned: not an orphan + assert "agent-used-workspace" not in warnings[0] # mounted: not an orphan + + @pytest.mark.asyncio + async def test_the_sweep_does_nothing_when_this_instance_cannot_be_identified(self): + own = _volume("agent-gone-workspace", "gone") + report, removed = await _sweep(_service(), scoped=[own], instance=None) + assert removed == [] + + @pytest.mark.asyncio + async def test_the_reclaim_is_a_warning_that_says_unrecoverable(self, caplog): + """AC4.""" + own = _volume("agent-gone-workspace", "gone") + with caplog.at_level(logging.INFO): + await _sweep(_service(), scoped=[own]) + lines = [r for r in caplog.records if "reclaimed" in r.getMessage()] + assert lines and all(r.levelno == logging.WARNING for r in lines) + assert "unrecoverable" in lines[-1].getMessage() + assert "gone" in lines[-1].getMessage() diff --git a/tests/unit/test_agent_readiness_probe.py b/tests/unit/test_agent_readiness_probe.py index 5f7cbfa7f..3aed56d1c 100644 --- a/tests/unit/test_agent_readiness_probe.py +++ b/tests/unit/test_agent_readiness_probe.py @@ -56,6 +56,7 @@ def wait_for_agent_ready(monkeypatch): container_reload=None, volume_get=None, volume_create=None, containers_run=None, image_get=None, # added to lifecycle.py import (#1809) + agent_volume_labels=None, # added to lifecycle.py import (#3214) ), "services.agent_service.helpers": types.SimpleNamespace( validate_base_image=None,