fix(volumes): rename-safe volume identity — sweep, purge, create and recovery (#1664, #1665, #1667)

[dolho]

Now carries #1664 + #1665 + #1667 (#1665 was PR #1668, folded in here; #1667 folded in the same way). Both fix one root cause with one primitive, and #1665 could not merge before this PR anyway — while a stacked PR gets no CI test net on this repo (backend-unit-test.yml triggers only on branches: [dev, main]), so the split cost the safety net and bought nothing.

Fixes the P0 in #1664: the #1581 orphan-volume sweep can silently, unrecoverably destroy a live renamed agent's /home/developer — including #1169 data_paths runtime data.

Related to #1664, #1665, #1667

Root cause

Rename keeps the agent's Docker data volumes — Docker can rename neither a volume nor its immutable trinity.agent-name label — so after a rename both the volume name and its label permanently describe an agent that no longer exists. The sweep read that self-description as ownership truth: label → old-name → no ownership row → orphan → volume rm -f.

The old is_reclaimable_agent_volume double-guard could not catch it: it only verifies the volume's name and label agree with each other, and both carry the same stale identity. Nothing asked whether the data was alive. Only Docker's in-use 409 stood in the way — and recreate_container_with_updated_config (guardrails / resources / shared-folder change, operator rebuild) removes the old container before creating the new one, so a sweep landing in that window succeeds.

The fix: ownership is a DB question, not a volume-label question

• agent_ownership.volume_base_name (NULL ⇒ agent_name, so no backfill) pins the volume identity. Written in the same statement as the rename (atomic — a crash cannot leave a renamed row unpinned) and frozen across re-renames (COALESCE, not SET — the volumes never moved).
• db.is_volume_base_reserved replaces is_agent_name_reserved as the sweep's orphan predicate. Soft-deleted rows still match, so the Soft-delete all entities + configurable data retention policy #834 recovery window holds for renamed agents too.
• Purge resolves the base BEFORE deleting the row that holds it — a renamed agent's real volumes are now the ones reclaimed (previously remove_agent_volumes(new_name) targeted volumes that never existed and the real ones leaked).
• Boot heal for agents renamed before the column existed: _heal_renamed_volume_bases reconstructs the pin from Docker's own mount table (container agent-{current} mounting agent-{other}-workspace is a completed rename). One-shot, idempotent, runs ahead of the startup sweep, and can never overwrite an existing pin.

Defence in depth (#1638: cleanup must fail safe)

This sweep force-removes durable user data, so two more gates stand in front of it — each independently sufficient:

• Mounted ⇒ never touched. list_attached_volume_names reads mounts across all containers (unfiltered — a volume someone mounts is somebody's live data whatever it calls itself). Fail-closed: if the mount table can't be read the whole cycle is skipped rather than reclaiming blind.
• Unattached-streak. A candidate must be seen unattached for ORPHAN_VOLUME_UNATTACHED_STRIKES = 3 consecutive cycles (~15 min). The recreate gap is seconds, so one unattached sighting is not evidence of an orphan; any attached sighting resets the streak. This is a backstop sweep with no urgency (the purge path is the primary reclaim).

is_reclaimable_agent_volume stays as the last line of defence, with its docstring corrected — its safety claim ("a live agent's data is unreachable by a bug") was exactly what rename falsified.

Verification

Real Docker daemon, real sweep code, synthetic renamed-agent volume in the recreate window (grace bypassed — orthogonal, covered by the #1581 suite):

A) pre-#1664 ownership answer   -> volume survives: False  <- the bug, reproduced (data destroyed)
B) renamed agent owns the base  -> volume survives: True   <- fixed (8 unattached cycles)
C) true orphan, 2 cycles        -> volume survives: True   <- streak not met
   true orphan, cycle 3         -> volume survives: False  <- sweep still works (reclaimed=1)
D) mounted but DB says unowned  -> volume survives: True   <- Docker-as-truth gate

Full lifecycle, zero mocks — real DatabaseManager (real schema + migrations), real Docker daemon, real sweep:

1. agent created                : volume=True  base=repro1664
2. renamed -> repro1664-renamed : pinned base='repro1664'
                                  is_agent_name_reserved('repro1664')=False   <- the trap
                                  is_volume_base_reserved('repro1664')=True   <- the fix
3. 8 sweeps, container gone     : volume survives=True   <- the P0, closed
3b. soft-deleted, 5 sweeps      : volume survives=True   <- recovery window holds for a renamed agent
4. after purge, 3 sweeps        : volume survives=False (reclaimed=1)  <- reclaim still works

Also exercised both new Docker helpers against the live daemon (9/9 agent containers mapped correctly; Mounts confirmed present on the list payload — one API call, no per-container inspect).

Tests: 23 new (tests/unit/test_1664_renamed_agent_volume_safety.py) covering the helpers, all three sweep gates, the purge base-resolution, the heal, and the DB predicate/pin on the real schema (SQLite + PG via db_harness); test_1581_agent_volume_reclaim.py updated to the new contract. Full unit suite green — 4129 passed; the 4 failures in the run (test_admin_email_login, Europe/Kiev tzdata) reproduce identically on origin/dev and are unrelated to this change.

A second commit worth reviewing

6c100a91 adds the three DatabaseManager pass-throughs. The unit tests mock db, so they could not see that the facade had no is_volume_base_reserved / get_volume_base_name / set_volume_base_name — every call would have raised AttributeError, and since the sweep swallows per-cycle exceptions, the reclaim would have silently degraded to a permanent no-op. The test_database_facade_delegation guard caught it; the mock-free run above is what proves the wiring.

Found by /review (self-review of this diff)

Two things the review turned up, both now fixed in-branch:

1. A regression this PR introduced (ee75479b). get_public_volume_name / get_shared_volume_name name off the live agent name, so a renamed agent owns volumes under two bases: workspace under the old, agent-{new}-public under the current. My predicate matched only the pin, so is_volume_base_reserved("new-name") was False for a live agent — and the public volume is unmounted whenever file-sharing is toggled off, which defeats the attached-check. Verified against a real daemon: it deleted a live agent's public volume. Same data-loss shape I was fixing, one layer up. Ownership is now the union of both identities, which is also a strict superset of the pre-bug: #1581 orphan-volume sweep can destroy a live renamed agent's home volume (rename keeps old volume name+label) #1664 is_agent_name_reserved (that predicate is exactly the union's first branch), so it can only protect more, never less. The purge had the mirror gap (leaked agent-{new}-public) and now removes both bases, deduped.

2. A cross-tenant P0 in the surrounding code (4a0a41c9, per maintainer decision to gate it here). Rename frees the name while the volume lives on, and crud.py provisions with get-then-create — an existing volume is silently reused. So an agent created under the freed name booted on the renamed agent's /home/developer. Verified on a real daemon: the new agent read agent A's ANTHROPIC_API_KEY from its .env, and the two owners need not be the same person. The create gate now consults is_volume_base_reserved and 409s before any container/volume work. This closes the rename-driven case; the orphaned-volume case (no row claims it, so the predicate says "free" and get-then-create still adopts it) needs an explicit refuse-or-adopt decision and is filed as bug: agent creation silently reuses a pre-existing Docker volume of the same name (get-then-create, no adopt/refuse decision) #1667.

#1665 — recovery mounted a renamed agent's empty workspace (folded in)

The same root cause pointed at recovery. recreate_missing_container (#1559) f-stringed the agent's current name, which for a renamed agent names no volume — and containers.run creates a missing named volume rather than failing, so recovery rebuilt the agent on a fresh empty /home/developer and started it. Healthy-looking, just empty, with its real data (incl. #1169 data_paths) stranded under the old base.

recovery mounts today : agent-r1665-new-workspace
   -> agent sees      : <volume does not exist -> docker CREATES it empty>
resolved via #1664 pin: agent-r1665-old-workspace
   -> agent sees      : the agents real data

One rule — resolve through the ownership row, never the name (_workspace_volume_name; NULL pin ⇒ the name itself, so un-renamed agents are byte-identical to today; a DB error falls back to the name rather than blocking a rebuild):

Site	Was	Now
recreate_missing_container	agent-{current}-workspace → empty workspace	resolved base → the agent's real data
_read_template_yaml_from_volume	read the same non-existent volume → {} → recovery rebuilt on default agent-type/runtime	recovers the committed researcher/codex
recreate_container_with_updated_config	a dead agent_volume_name = f"agent-{agent_name}-workspace" that read as if it were the mount in use	removed + annotated (mounts are carried forward from the old container — that is what keeps a renamed agent on its volume across a recreate)

deploy.py's version-name volume is correct by construction (fresh name, no row, no rename in its past) — annotated as audited, not changed. Verified mock-free (real DB + real daemon); 3 new tests. Filed #1669 for the fourth site the audit found (public/shared volumes name off the live name) — deliberately not bundled: agents renamed in the field already hold data under agent-{new}-public, so resolving to the pinned base would orphan that instead. It needs an adopt/migrate decision.

#1667 — silent volume adoption at create (folded in)

crud.py provisioned the home volume with get-then-create and no branch: an existing agent-{name}-workspace was silently mounted as the new agent's /home/developer, so the previous holder of that name had its .env, .credentials.enc and workspace resurface inside a different agent — possibly a different owner's. #1664's gate covers a volume some row still claims (rename); this covers the volume nothing claims: a purge whose removal hit an in-use 409, a crash between volume_create and the ownership INSERT, or a restored backup.

The fact that reshaped the fix (and killed the option I favoured when filing #1667): emptiness cannot discriminate, because deploy-local (#950) pre-populates the volume with the template before calling create — a legitimate adopt is non-empty. (Docker also auto-populates a named volume from the image on first mount, so "empty" wouldn't even identify a crashed create.)

So the adopter declares itself: adopt_existing_workspace — a function kwarg, mirroring skip_name_sanitization, deliberately not a field on AgentConfig where an API caller could set it and re-open the disclosure. deploy passes True; everyone else gets a 409 naming the volume and how to clear it. Adoption is now logged, so it is never silent again.

Raised before the docker try-block — that block catches Exception, rolls back, and re-raises a generic 500, so a 409 inside it would lose both its status and its message (the same reason fork_to_own raises its destination 409 before the block). Nothing is built yet there, so nothing needs rolling back. Fail-open on a probe error; ghosts skip it (volume-less by construction).

Verified mock-free against a real daemon:

A) normal create over a stranger's leftover volume:
   409: Data volume 'agent-leftover1667-workspace' already exists and no agent claims it...
B) deploy-local (declares the adopt of its pre-populated volume):
   -> passes the gate (fails later on an unrelated mock artifact)

Unaffected by design, checked rather than assumed: system_agent_service builds its recycled-name container directly instead of via create_agent_internal; the Cornelius seeder is first-run-only behind a durable flag.

Notes for review

• Dual-track migration per invariant Feature/vector log retention #3: SQLite agent_ownership_volume_base_name + Alembic 0024_agent_ownership_volume_base_name, plus schema.py / tables.py. Verified the SQLite migration applies to a pre-existing table and is idempotent on re-run.
• Strike state is in-process only: a restart restarts the count — the safe direction.
• Out of scope, filed as bug: renamed agent's soft-delete recovery mounts a fresh empty workspace volume (rename keeps the old volume) #1665: three other paths still f-string agent-{current_name}-workspace (soft-delete recovery recreate_missing_container, _read_template_yaml_from_volume, deploy pre-populate). For a renamed agent, recovery mounts a fresh empty volume — a real bug, but not data destruction, and it wants the primitive this PR introduces (db.get_volume_base_name). Kept out to keep the P0 fix tight.

🤖 Generated with Claude Code

[github-actions]

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

[dolho]

/review — self-review of this PR

Two passes over the branch diff (the second because the first pass's fixes were themselves unreviewed code). Scope: clean. Plan: all #1664 acceptance criteria done. Four critical findings — notable that all four were in the fix, not in the original bug, and three of the four were green under the mocked unit tests.

#	Found by	Finding	Status
C1	self-review	Ownership predicate ignored a renamed agent's second volume base — get_public_volume_name/get_shared_volume_name name off the live name, so agent-{new}-public exists alongside the old-based workspace. Matching only the pin marked a live agent's public volume an orphan, and that volume is unmounted whenever file-sharing is off, defeating the attached-check. Verified on a real daemon: it deleted a live agent's public volume — the same data-loss shape this PR fixes, one layer up.	fixed ee75479b — predicate is now the union of both identities, a strict superset of the pre-#1664 is_agent_name_reserved (that predicate is exactly the union's first branch), so it can only protect more, never less
C2	review of surrounding code	Cross-tenant credential disclosure (pre-existing on dev): rename frees the name while the volume lives on, and crud.py provisions get-then-create — an existing volume is silently reused. An agent created under the freed name booted on the renamed agent's /home/developer. Verified on a real daemon: the new agent read agent A's ANTHROPIC_API_KEY out of its .env, and the two owners need not be the same person.	gated c4728b3a (maintainer's call to fix here) — create now consults is_volume_base_reserved and 409s before any container/volume work. Remainder (orphaned volume, no row → predicate says "free" → still adopted) filed as #1667
C3	re-review	The both-bases purge from C1 could delete a different live agent's volumes. volume_base_name has no unique constraint, and installs predating C2's gate can hold the collision that gate now prevents (agent new pinned to base old plus a live agent named old, sharing volumes). Purging new removed agent-old-*. Before the both-bases change purge touched only agent-new-*, so this branch widened the blast radius rather than inheriting it.	fixed b6f7d562 — the purged row is gone by then, so a still-True is_volume_base_reserved means another row claims the base: skip + WARNING. Verified on a real DB+daemon: purge completes, removed 0 data volumes, live volume survives
C4	CI regression-diff	The C2 gate 409'd all 9 test_fork_to_own tests: that module mocks the whole database module, and an unstubbed MagicMock attribute is truthy, so the new call read as "base taken" and refused every create. Test-harness gap, not production (fork destinations, deploy.py pre-populate #950 and versioned redeploy all use fresh names with no owning row).	fixed — stub added; audited all modules with the same wholesale-db mock and every caller of create_agent_internal, all green. Ghosts also exempted from the gate (volume-less by construction)

Informational (not blocking)

• N+1: one is_volume_base_reserved per volume per cycle (~3/agent/5 min). Same shape as the code it replaces — not a regression; an IN query would fix it if fleets grow.
• Strike map is per-process: with --workers 2 each worker needs its own 3-cycle streak. Independently safe (both must agree), just slower to reclaim.
• Both docker_utils list calls are deliberately unfiltered — an unrecognised container's mount still counts as in-use. Slightly heavier on busy hosts; the fail-safe is the point.

Clean

SQL injection (all SQLAlchemy expression-level) · auth boundaries (no new endpoints) · credential exposure in logs (names only) · bare excepts (none; broad catches documented per-sweep) · enum completeness (none added) · docs (architecture.md updated for schema + predicate; learnings ledger entry added).

The durable lesson (now in docs/memory/learnings.md)

A resource's self-description is not its ownership, and renames are what prove it. The pre-existing "double-guard" (volume name AND label must agree) could never catch this class: after a rename both halves are equally, consistently stale — a self-consistent lie. Ownership has to live in a row we control.

The corollary this PR learned the hard way, three times: mocking db makes every new db.* call invisible. It hid a facade gap that would have AttributeError'd in production and — because the sweep swallows per-cycle exceptions — silently degraded the reclaim to a permanent no-op; it hid C4; and mock-based sweep tests were happy through C1. Only the real-DB/real-Docker runs and CI's base-vs-head diff caught them. A -k subset is a guess about blast radius: C4 slipped through locally because my filter didn't match the filename.

Test state (honest accounting)

CI green: 19/19 checks, and the regression-diff (base vs head, 3 seeds each) reports no new failures introduced by HEAD — that job is what caught C4 and the earlier facade gap, so it is load-bearing here, not decoration.

The local full unit suite is 4135 passed, 7 failed. None are attributable to this branch, and rather than assert that, I checked each: 5 reproduce on a clean origin/dev worktree (test_admin_email_login ×3, Europe/Kiev tzdata missing locally, test_1474_read_boundary_z::test_schedules_summary_last_run_at_normalized); the other 2 (test_1081_physical_meter, sqlite) are order-dependent — they pass in isolation on both branches, and pairing them directly with this PR's new test file passes, so the new real-DB harness usage is not the polluter. CI's seeded base-vs-head runs agree.

Recommendation

Merge-ready pending human review. What's left is judgment rather than mechanics, and worth a reviewer's opinion: (1) is a 3-cycle (~15 min) unattached streak the right conservatism for a backstop sweep? (2) should the create gate 409, or offer an explicit adopt_existing_volume? (#1667 has the options written up.)

[obasilakis]

Reviewed the full diff against the merge-base (68699cf9); all 48 tests pass locally (test_1664_* + test_1581_*). No blocking findings — the identity model is right, the sweep's three fail-closed conditions are well-chosen, the dual-track migration is complete (SQLite + Alembic 0024 + schema.py + tables.py + facade), and the learnings entry is a genuinely good one.

Two non-blocking notes, one filed separately:

1. Rename is the one producer of volume_base_name left ungated → filed as #1671 (P1), not a merge blocker.

is_volume_base_reserved has exactly two enforcement call-sites: crud.py:298 (create) and the purge/sweep. Rename checks only get_agent_container(sanitized_name) and agent_name == new_name, so rename → a freed base is still permitted, and the collision your purge-path comment anticipates ("installs predating the create-time gate … can hold a collision") can be minted fresh post-fix by an ordinary swap: rename prod-agent → prod-agent-old, then staging-agent → prod-agent. Since get_public_volume_name names off the live name (deliberately, per your is_volume_base_reserved docstring), the new prod-agent then get-then-creates onto the old agent's agent-prod-agent-public — the #1667 class through the ungated path. The purge handling stays correctly fail-safe (skips both bases rather than destroying), so this is a prevention gap, not a safety one — hence the follow-up rather than a change request here. Full repro + suggested fix in #1671.

2. Docstring points at a function that isn't there — db/migrations.py:2895:

those are healed at boot by heal_renamed_volume_bases (db/agents.py)

It's _heal_renamed_volume_bases in services/cleanup_service.py:1758; db/agents.py has no such function. Worth correcting on your next push — the migration docstring is exactly where someone goes to understand why the backfill is a no-op.

Nice work on this one. The is_reclaimable_agent_volume docstring change in particular — explicitly demoting it from "safety" to "the caller aimed correctly, not evidence the data is dead" — is the sort of thing that stops the next person trusting a guard for something it never proved.

[dolho]

Both notes addressed, and I took the #1671 gap into this PR rather than leaving it as a follow-up. Thank you for the review — the rename analysis in particular.

1. #1671 — rename gated (74f4a9a7), closing the issue

You framed it as a prevention gap rather than a safety one, which is right, but I think it's a merge blocker for a reason worth stating: this PR's own purge guard assumes a single claimant. With two rows on a base, is_volume_base_reserved stays true after either is purged, so the guard skips both bases and the orphan sweep never reclaims them — the volumes become unreachable and undeletable by any automated path. Shipping the invariant enforced at one of its two producers leaves my own guard resting on something the code doesn't guarantee. So: gated.

Taken your stronger suggestion — enforced inside db.rename_agent's transaction (chokepoint: closes the check-then-write gap, covers future callers), plus a router gate before the container is stopped/renamed, so a refusal leaves nothing half-done and returns an actionable 409 instead of that path's generic 500: Failed to update database. That's the #1445 router+chokepoint pattern.

One correction to the suggested fix. The one-liner as written refuses a legitimate rename-back:

create `B` → rename `B`→`A`   (row: agent_name='A', volume_base_name='B')
rename `A`→`B`  →  is_volume_base_reserved('B') is True — via A's own pin

That rename is fine: the agent already owns base B, and the result has exactly one claimant. The naive gate refuses it and tells the owner their own volumes belong to someone else. So is_volume_base_reserved grew exclude_agent — "does anyone else claim this base?" — and rename passes itself.

Verified against a real DB (your repro, verbatim):

1. rename prod -> prod-old          : pin='prod'
   name 'prod' free? True           base 'prod' claimed? True
2. rename staging -> prod (the swap): allowed=False        <- was: allowed
3. rows claiming base 'prod'        : ['prod-old']         <- exactly one
4. rename-back B->A->B              : allowed=True         <- still works

Tests in test_1664_renamed_agent_volume_safety.py::TestRenameIsGatedOnVolumeBase (5): your repro, the rename-back, exclude_agent scoping, an ordinary rename, and a router test asserting the 409 fires before container_stop and that rename_agent is never reached.

On your partial-unique-index note — agreed it doesn't map cleanly: the claim is agent_name OR volume_base_name, so a single-column index can't express it. A CHECK-style structural encoding would need the base materialised into one column (i.e. writing volume_base_name for every row instead of NULL-means-name), which trades the zero-backfill property #1664 was built around. Checked-not-structural is the right call here, but it's worth revisiting if a third producer ever appears.

2. Migration docstring corrected

db/migrations.py now points at CleanupService._heal_renamed_volume_bases (services/cleanup_service.py). You're right that it's exactly where someone goes to understand why the backfill is a no-op — a wrong pointer there is worse than none.

Not a real item: the nightly bot's merge-conflict warning

Left as-is deliberately. That comment is from 08:21 UTC, before my last push; dev (68699cf9) is an ancestor of this branch, git merge origin/dev says Already up-to-date, and GitHub reports MERGEABLE. BLOCKED is the review requirement, not a conflict. Flagging rather than silently "fixing" a phantom.

[obasilakis]

Approved. Both review notes verified in 74f4a9a7, not just claimed:

Docstring — db/migrations.py:2892 now points at CleanupService._heal_renamed_volume_bases (services/cleanup_service.py). Correct.

#1671 rename gate — taken into this PR and enforced at both producers: the chokepoint inside db.rename_agent's transaction (closes the check-then-write gap, covers future callers) and a router gate at agent_rename.py:118, which fires before container_stop at 139 — a refusal leaves nothing half-done and returns an actionable 409 instead of the generic 500: Failed to update database.

Your correction to my suggested fix is right and I'm glad you pushed back. My one-liner would have refused a legitimate rename-back (B->A->B): the agent's own pin makes is_volume_base_reserved true, so the gate would refuse the owner their own volumes and tell them the base belongs to someone else. exclude_agent — "does anyone else claim this base?" — is the right predicate, and the chokepoint expresses it directly in SQL (volume_base_name == new_name AND agent_name != old_name).

And your reason for treating it as a blocker is sharper than my reason for filing it as a follow-up. I read it as prevention-vs-safety; you spotted that two claimants on one base leave is_volume_base_reserved true after either row is purged, so the guard skips both bases and the sweep never reclaims them — unreachable, undeletable volumes. This PR's own guard assumed a single claimant, so enforcing the invariant at only one of its two producers would have rested it on something the code didn't guarantee. Right call.

Agreed on checked-not-structural: the claim is agent_name OR volume_base_name, which no single-column partial index expresses, and materialising the base into one column would trade away the zero-backfill property #1664 was built around. Worth revisiting only if a third producer appears.

The nightly bot's conflict warning is stale — confirmed dev (68699cf9) is an ancestor and GitHub reports MERGEABLE. Flagging it rather than silently 'fixing' a phantom was the right instinct.

19/19 checks green on 74f4a9a7. Merging.