fix(retention): refuse a prune that would delete most of a table (#1644)#1674
Open
obasilakis wants to merge 5 commits into
Open
fix(retention): refuse a prune that would delete most of a table (#1644)#1674obasilakis wants to merge 5 commits into
obasilakis wants to merge 5 commits into
Conversation
#1638 fixed the specific way a *default change* destroyed data; it added no guard on the prune itself. Every other route to a destructive window stayed open — `PUT /api/settings/ops/config` has zero validation, and a future default regression or a direct DB write has no writer to validate at all. `services/retention_guard.py` gates all 7 window-driven destructive prunes: count the candidate set (bounded, `LIMIT threshold+1` -> O(threshold)), and if it exceeds 1000, REFUSE, log ERROR on the green->red transition, and raise an operator-queue alarm. An admin unblocks it via POST /api/settings/retention/ acknowledge — bound to the window in force (409 on mismatch) and single-use, so one approval can never authorize a larger delete later at the same window. Design notes (each one is load-bearing): - Stateless detection, not a last-seen-window watermark: that row would be deletable through the same endpoints that cause the bug, silently disarming the guard (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 costs a full scan and divides by zero on an empty table, and a percentage inverts on the agent sweep — 3 purged agents are ~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 everywhere: any error refuses. A guard that fails open is worse than no guard because it manufactures confidence. - 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 (Clear All -> cancelled), and prune_terminal_items would delete the approval at 90 days — the sweep deleting its own authorization. - The threshold is a fixed constant, NOT a setting. It was briefly an operator knob: nobody can reason about the right value, and the panel needed a caption explaining that bigger is worse. Worse, a mutable constant read at action time gating a destructive op is #1638 one level up. Deleting the knob deleted its clamp, endpoint, blocklist entry, and a whole fail-closed branch. - Each count SHARES its prune's predicate by construction, so the number the guard reports is the number that would go. This matters most for operator_queue, whose predicate derives a second cutoff internally. - The alarm hosts on the reserved sentinel `_retention-guard` (uncreatable — sanitize_agent_name strips the leading `_`), excluded from canary L-03's orphan scan: it is not a ghost agent. Its context carries counts and identifiers only, never sample rows (canary G-04's lesson). - Agents floor at 0: every purge destroys Docker volumes (#1581). Also fixed, found while investigating: - RETENTION_OPS_KEYS held 5 of 7 windows, so `/ops/reset` DELETED agent_reports_retention_days and operator_queue_retention_days while reporting "retention windows unchanged", and both were invisible to GET /api/settings/retention and to boot-time logging. - The documented "5000 rows/cycle cap" does not exist: 6 of 7 prune accessors drain the entire candidate set in one call (`chunk_size` bounds each transaction). #1638 deleting 5352 rows in one startup sweep is the proof. Corrected the comment, the docstring, and architecture.md. - The Retention panel's inputs specified `border-gray-300` without the `border` class, so no border width applied and browsers fell back to default number-input chrome. Fixed to the app-wide convention (96 other inputs). - A 422 rendered FastAPI's raw Pydantic error array into the page; added utils/apiError.js and pre-flight validation. Known gap, filed separately (trinity-ops-agent#232): `require_admin` does not call `reject_agent_principal`, so an admin-owned agent's own MCP key can set a retention window via the unblocklisted `PUT /api/settings/{key}`. The ack endpoint applies `reject_agent_principal` explicitly rather than relying on `require_admin`, so it is not reachable that way. Fixes #1644
…-blast-radius-guard # Conflicts: # docs/memory/architecture.md # src/backend/db/agents.py
guard The blast-radius guard floors the soft-deleted-agent purge at 0 (every purge destroys Docker volumes, #1581), so `_sweep_soft_deleted_agents` now reaches `purge_agent_ownership` only with a candidate count and an acknowledgement. These tests mock `db` wholesale, so the count came back as a MagicMock — uncomparable to an int, which the guard correctly treats as an error and fails closed, skipping the purge the tests assert on. Also patches `retention_guard.db`: the guard has its own `from database import db` binding, so patching `cleanup_service.db` alone leaves it reading the real database mid-test.
Caught by tests/lint_sys_modules.py. A bare pop leaks the eviction into every later test in the session; monkeypatch restores it at teardown.
…-blast-radius-guard # Conflicts: # tests/registry.json
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
1 participant
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
#1638 made the retention defaults safe. It added no guard on the prune itself. This adds the backstop that doesn't care how a bad window arrived.
Before each of the 7 window-driven destructive prunes, count the candidate set (bounded) and if it exceeds 1000, refuse — log ERROR, raise an ops alarm, delete nothing. An admin unblocks it with one call.
The threat isn't hypothetical.
PUT /api/settings/ops/configisDict[str, str]→if key in OPS_SETTINGS_DEFAULTS→ bare upsert. No type, range, clamp, or audit. Garbage fails safe ("abc"→ 0 → sweep disabled); a small valid integer is the catastrophic input, and no range check catches5typed for50.Two corrections to the issue's own premises
RETENTION_CHUNK_SIZE_PER_CYCLE = 5000rows per cycle". It doesn't — 6 of 7 accessors arewhile True:drain loops;chunk_sizebounds each transaction. One call deletes everything past the cutoff. bug: retention floor (#1065) silently deletes pre-existing execution history on upgrade — currently dev-only, ships to everyone at next release #1638 deleting 5352 rows in one startup sweep is the proof — a real 5000 cap couldn't produce that number. Comment, docstring, and architecture.md corrected._clampismax(value, 5)— a monetization floor, not a safety check. An entitled admin typing5is accepted and is the bug: retention floor (#1065) silently deletes pre-existing execution history on upgrade — currently dev-only, ships to everyone at next release #1638 scenario. It's also a parallel endpoint, so every edition routes around it.Design decisions worth reviewing
learnings.md#1638 Lesson 2. The ack is state, but deleting an ack fails safe.execution_log(its predicate includesexecution_log IS NOT NULL); a total costs a full scan and divides 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; any anomaly is large. Table size is irrelevant.create_itemis a blindINSERT … ON CONFLICT DO NOTHING— a load-bearing item would wedge shut permanently once it hit any terminal state (Clear All →cancelled), andprune_terminal_itemswould delete the approval at 90 days: the sweep deleting its own authorization. Decorative ⇒ both dissolve.operator_queue, whose predicate derives a second cutoff internally (max(responded, terminal)).Fail-closed everywhere. Count throws, ack lookup throws → refuse. A guard that fails open is worse than no guard: it manufactures confidence.
Also fixed (found while investigating)
RETENTION_OPS_KEYSheld 5 of 7 windows — so/ops/resetdeletedagent_reports_retention_daysandoperator_queue_retention_dayswhile returning"retention windows unchanged", and both were invisible toGET /api/settings/retentionand boot logging.border-gray-300withoutborder— no width applied, so browsers rendered default number-input chrome. Now matches the app-wide convention (96 other inputs use it).utils/apiError.js+ pre-flight validation. ~198 other callsites share this latent bug — left alone.trinity-ops-agent#232 (private — public
SECURITY.mdforbids disclosing here).require_adminnever callsreject_agent_principal, and an agent key resolves to its owner carrying the owner's role. On a default install (Cornelius/trinity-system are admin-owned) an agent's own MCP key canPUT /api/settings/execution_row_retention_days {"value":"1"}through the unblocklisted catch-all. This PR does not close it — but the ack endpoint appliesreject_agent_principalexplicitly rather than trustingrequire_admin, so the gate isn't reachable that way (verified: 403).Test Plan
pytest tests/unit/test_1644_retention_guard.py— 26 pass (dual-backend SQLite + PG)<= PREVIOUS) + asserted not settings-backedevaluate()deletes nothingtest_1474_read_boundary_z) is pre-existing — verified identical on stasheddev.999999999; catch-all 422s the guard key; ack 409s on window mismatch; agent principal withrole="admin"gets 403.npm run check:tokenspasses.Expected behaviour worth knowing
A legitimate first-enable of retention on a mature install will trip the guard once. That's intended — the guard can't distinguish a large legitimate backlog from a mistyped window, so it asks once. That's what keeps it a small predicate instead of a "was this narrowed?" detector.
Fixes #1644
🤖 Generated with Claude Code