fix(agents): recovery must mount a renamed agent's real workspace volume (#1665)

[dolho]

Stacked on #1666 (base is feature/1664-..., not dev) — it needs the db.get_volume_base_name primitive that PR adds. Review/merge #1666 first; GitHub retargets this to dev automatically when it lands. The diff shown here is only this fix.

Related to #1665

The bug

Rename keeps the agent's Docker volumes under the pre-rename base — Docker can rename neither a volume nor its immutable trinity.agent-name label — so for a renamed agent the current name names no volume. recreate_missing_container (#1559 soft-delete recovery) f-stringed that current name.

The failure is silent, not loud: containers.run creates a missing named volume rather than erroring, so recovery rebuilt the agent on a brand-new empty /home/developer and started it. The agent comes back looking healthy — just empty — while its real data (including #1169 data_paths) sits unreferenced under the old base.

Reproduced against a real DB + daemon before the fix:

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

The fix — one rule, three sites

Resolve the volume through the ownership row, never from the agent's name (_workspace_volume_name; NULL pin ⇒ the name itself ⇒ every un-renamed agent is byte-identical to today). Fail-safe: a DB error falls back to the agent name — the pre-#1665 behavior — rather than blocking a rebuild.

Site	Was	Now
recreate_missing_container	mounted agent-{current}-workspace → empty workspace	mounts the 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" assignment 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)

The deploy.py version-name volume is correct by construction (a fresh name, no ownership row yet, no rename in its past) — annotated as audited, not changed.

Verification (real DB + real Docker, no mocks)

1. recovery would mount        : agent-v1665-old-workspace
   agent's /home/developer has : the agents real data
2. template.yaml recovered     : {'type': 'researcher', 'runtime': 'codex'}
3. un-renamed agent unaffected : agent-v1665-old-workspace (no row -> falls back to the name)

Tests: 3 new (renamed-agent recovery mounts the real base; the template read resolves too; DB-error fallback), plus 261 passing across the affected suites.

Notes for review

• The mock trap, again. The existing bug(recovery): soft-delete recovery dead-ends — recover tells you to start, but start 404s when the container is gone #1559 recreate test mocks the whole database module, and an unstubbed MagicMock attribute is truthy — it would be f-stringed straight into the volume name. It failed exactly as test_fork_to_own did in fix(volumes): rename-safe volume identity — sweep, purge, create and recovery (#1664, #1665, #1667) #1666; stubbed explicitly. Worth knowing for any future db.* call added to a create/recreate path.
• Out of scope, filed as a follow-up: the audit found a fourth site of the same class that this issue did not name — get_public_volume_name / get_shared_volume_name name off the live agent name, and recreate_container_with_updated_config deliberately drops the old public/shared mounts and re-adds them by current name. So a rename + any recreate silently empties a renamed agent's publish dir and shared-out folder (data orphaned, not deleted — bug: #1581 orphan-volume sweep can destroy a live renamed agent's home volume (rename keeps old volume name+label) #1664's union predicate keeps the sweep off the old base). Unlike the workspace, that one is not a mechanical swap: an agent renamed in the field may already have data under agent-{new}-public, so resolving to the pinned base would orphan that instead. It needs an adopt/migrate decision, not a one-line change.

🤖 Generated with Claude Code

[dolho]

⚠️ Note on this PR's CI: the test net does not run on a stacked base

The check list here shows 5 passing / 0 failing, and that is not evidence of anything. backend-unit-test.yml triggers on pull_request: branches: [dev, main] — this PR targets feature/1664-..., so pytest (3 seeds × base/head) and the regression-diff never ran. Those are exactly the jobs that caught two of the four bugs in #1666, so their silence is the thing worth noticing, not the green ticks.

Compensating, since "no failures reported" and "nothing ran" look identical from the outside:

• Forced the suite onto this branch via workflow_dispatch (run) — real CI evidence for this head.
• Full local unit suite running against the same head; result posted here when it lands.
• The full base-vs-head net engages automatically once fix(volumes): rename-safe volume identity — sweep, purge, create and recovery (#1664, #1665, #1667) #1666 merges and GitHub retargets this PR to dev. Do not merge this before fix(volumes): rename-safe volume identity — sweep, purge, create and recovery (#1664, #1665, #1667) #1666 — apart from the missing primitive, that retarget is what buys back the regression-diff.

Flagging it rather than letting a green-looking PR imply coverage it does not have.

[dolho]

Test results for this head (the net that the stacked base skipped)

Forced CI run — green, and it is the real net: run 29569816574 — all 6 pytest jobs (base + head × 3 seeds), the regression diff, and the sys.modules-pollution lint. conclusion: success, no new failures introduced by HEAD. That regression-diff job is the one that caught two of the four bugs in #1666, so this is the evidence that was missing from the PR check list.

Local full unit suite: 4138 passed, 7 failed — byte-identical failure set to the #1664 branch, and none from this change: 5 reproduce on a clean origin/dev worktree (test_admin_email_login ×3, Europe/Kiev tzdata absent locally, test_1474_read_boundary_z), and 2 (test_1081_physical_meter, sqlite) are order-dependent — they pass in isolation on every branch and are not polluted by the new tests. CI's seeded base-vs-head runs agree.

Merge order stands: #1666 → this. On retarget to dev, the full net runs automatically on every push.

[dolho]

Superseded by #1666 — folded in, per maintainer's call.

GitHub auto-marked this merged the moment feature/1664-... (this PR's base) came to contain commit 01d15fbb — standard stacked-PR behaviour, not a merge to dev. Nothing shipped anywhere: the fix now rides in #1666, which is the PR to review and merge.

Why it was folded rather than kept stacked: this could never merge before #1666 (it needs that PR's db.get_volume_base_name primitive), so the split imposed a merge order without buying independence — and it actively cost coverage. backend-unit-test.yml triggers on pull_request: branches: [dev, main], so a PR based on a feature branch runs no pytest and no regression-diff; every future push here would have looked green while nothing ran. For a silent-data-loss bug class that is the worst available failure mode. Same commit, fast-forwarded, no rewrite.

#1665 stays open until #1666 reaches main. #1669 (public/shared volumes — needs an adopt/migrate decision) is unaffected.