fix(gateway): persist agent rename before moving owned state

[Nillth]

Summary

Fixes #7907. rename_agent_cascade (added by #7841) moved external owned state — the agent workspace dir and the memory/cron/acp/session stores — before the config rename was durably persisted. If persist_and_swap failed after those side-effects, the persisted + in-memory config still named from while the workspace and owned stores had already moved to to: the inverse split-brain of the one #7841 fixed.

This reorders the cascade to persist the config first:

• Compute new_ws off the rewritten config, then persist_and_swap first. On persist failure, return the error as before — but now no side-effects have run, so config, workspace, and owned state are all consistently on from (a clean abort; persist_and_swap reverts the on-disk file and never swaps state.config).
• Only after a durable persist, run move_renamed_workspace + cascade_rename_agent, reading the committed config via a short-lived clone (state.config.read().clone() — never a lock guard held across the .await; the cascade needs only data_dir, unchanged by the rename).
• A post-persist side-effect failure keeps renamed: true and surfaces via the existing warnings (the config rename did commit; persist-first makes each side-effect idempotently re-runnable from the corrected config — the workspace move early-returns once the source is gone, owned-store ops re-point by WHERE agent_alias = from — so re-issuing the rename converges). No compensation/rollback saga.
• The completion log escalates INFO → WARN when warnings is non-empty, so a degraded "config moved, a follower didn't — re-issue to converge" outcome is visible operationally instead of buried at INFO.

Scope: the rename path only (the reported #7907). The mirror sibling delete_agent_cascade has the identical persist-after-side-effect defect and additionally swallows failures (its MapKeyResponse carries no warnings) — filed separately as a follow-up so this p1 stays tight.

No API or schema change: RenameMapKeyResponse already carries warnings; the response shape is unchanged.

Validation Evidence

cargo fmt --all -- --check                                    # clean (exit 0)
cargo clippy -p zeroclaw-gateway --all-targets -- -D warnings # no warnings (exit 0)
cargo test  -p zeroclaw-gateway                               # 265 passed, 0 failed (+2 new)
cargo build --workspace --all-targets                         # clean (exit 0)

Two regression tests (api_config::tests):

• agent_rename_leaves_owned_state_put_when_persist_fails — forces persist to fail (config_path is a directory) and asserts the cron job stays under from, no cron under to, and state.config still names from. Verified to FAIL under the pre-fix ordering (cron moved to to before the failing persist) and pass with the fix.
• agent_rename_moves_owned_state_after_successful_persist — happy path: a successful persist re-points cron to to and moves the workspace (guards against an over-correction that skips the side-effects).

The probe is cron (real sqlite at <data_dir>/cron/jobs.db, public cron::add_job/list_jobs_by_agent) — MockMemory's rename_agent is unsupported and would only emit a warning, not a move. A small #[cfg(test)] pub(crate) re-export of test_state lets the api_config tests reuse the existing AppState builder; no production code is exposed.

Security & Privacy Impact

• No new permissions, capabilities, or filesystem scope; the side-effects are the same ones, only reordered.
• No new external network calls; no secrets handling change.
• The post-persist read is a short-lived config clone (no lock guard across .await).
• No PII in the diff.

Compatibility

Backward compatible. No config/env/CLI surface change and no response-schema change (RenameMapKeyResponse.warnings already existed). The observable change is the side-effect ordering (a correctness fix) and one completion-log level (INFO→WARN on partial outcomes). No upgrade steps.

Rollback

git revert — a self-contained reorder of one handler plus two tests and a test-only pub(crate) re-export. No state or schema migration.

[Nillth]

The mirror sibling delete_agent_cascade — same persist-after-side-effect ordering, and it additionally swallows partial failures (no warnings on MapKeyResponse) — is filed as #7941. Scoped out of this PR to keep the p1 tight; the fix there is a mechanically-identical reorder plus an additive MapKeyResponse.warnings field.

[Audacity88]

@Nillth I reviewed the live PR state, linked issue #7907, follow-up issue #7941, the gateway/config diff, and the current CI results. I did not run local Cargo validation. The core persist-first reorder is the right direction for #7907, and the new cron-backed regression test is a good probe for the owned-state ordering bug.

🟢 What looks good — Persisting config before side effects fixes the reported split-brain

The main reorder in rename_agent_cascade addresses the #7907 failure mode: if persist_and_swap fails now, the workspace move and owned-state cascade have not run yet, so config, workspace, and owned state stay on from. That is the right direction, and the persist-failure regression test covers the important pre-fix failure shape.

🔴 Blocking — The partial-failure retry path cannot actually be reissued

The new post-persist warning path says a side-effect failure is “idempotently re-runnable” and logs “re-issue the rename to converge”. That does not match the actual API contract. After persist_and_swap succeeds, state.config no longer contains from; it contains to. A second from -> to request reaches rename_with_cascade, which calls rename_map_key and returns RenameError::NotFound when the source alias is absent, before move_renamed_workspace or cascade_rename_agent can retry anything.

That means the warning case still leaves the operator without the recovery path the PR relies on. The individual side effects may be idempotent once they are reached, but the API layer rejects the retry before it can reach them: config is committed to to, a workspace or owned store may still be under from, and the documented retry instruction fails at the config rename step. Please either make this warning path genuinely recoverable, for example by detecting the already-renamed config state and rerunning the side effects for the same from -> to request, or replace the “re-issue to converge” contract with a concrete safe remediation/compensation path. This should also get a regression test that forces a post-persist side-effect failure and proves the chosen recovery path works.

One separate CI note: the current Check (32-bit) job is red, so it still needs a clean rerun before merge. I am not treating that CI failure as the code blocker here.

[WareWolf-MoonWall]

Reviewed at b9bb218. CI Required Gate: green (all 18 checks passing).

@Audacity88's CHANGES_REQUESTED is active and the blocking item is correct — I'm commenting rather than approving over it. My read of the diff agrees with the analysis below.

🟢 What looks good — persist-first ordering is the right fix for #7907

The core reorder — persist_and_swap before move_renamed_workspace + cascade_rename_agent — addresses the reported failure mode exactly. Under the previous ordering, a persist failure after the workspace move left state.config still naming from while the workspace and owned stores had already moved to to. Persist-first means an early failure returns an error before any side-effect runs; config, workspace, and owned state all remain consistently on from. That is a clean abort with no split-brain.

🟢 What looks good — regression tests use real sqlite probes

agent_rename_leaves_owned_state_put_when_persist_fails forces persist to fail (config path is a directory) and asserts cron stays under from. agent_rename_moves_owned_state_after_successful_persist proves the reorder didn't accidentally suppress the side-effects on the success path. Using real sqlite cron probes rather than mocks means the test covers the actual storage contract.

🟢 What looks good — delete_agent_cascade parallel defect scoped out

Keeping the p1 rename fix tight and deferring the delete_agent_cascade issue (filed as #7941) is the right decision. The rename path is the reported and tested bug; the delete path gets its own targeted PR.

---

Concurring with @Audacity88's blocking item:

🔴 Blocking — "re-issue the rename to converge" recovery path cannot actually be executed

The WARN log message and the code comment both say:

"re-issue the rename to converge"

This recovery instruction is incorrect. After persist_and_swap succeeds, state.config contains to, not from. A second request from → to reaches rename_agent_cascade → rename_map_key → RenameError::NotFound because the source alias is absent from the committed config. The API rejects the retry before move_renamed_workspace or cascade_rename_agent can run.

In the partial-failure case — config durably committed to to, workspace or an owned store failed to move — the operator is left without a working recovery command and without a compensation path. The current state is: config says to, but workspace/owned stores may still be at from. The documented instruction to "re-issue the rename" silently fails at the API layer.

To fix this the recovery path needs to either:

1. Detect the already-committed config state in rename_map_key (e.g. recognise from absent + to present as a "resume post-persist" case) and re-run only the side-effects, or
2. Replace the "re-issue to converge" contract with an explicit safe remediation path (e.g. a dedicated idempotent POST /api/config/repair-agent-state endpoint, or a documented manual procedure) and a regression test that covers it.

The test that @Audacity88 requested — forcing a post-persist side-effect failure and proving the chosen recovery path converges — is necessary before this merges.

[singlerider]

Reviewed at b9bb218 on a fresh checkout of fix/agent-rename-persist-order. No new commit has landed since @Audacity88's and @WareWolf-MoonWall's reviews, so I read the cascade path directly to confirm their blocking item still holds. It does.

🔴 Blocking — the "re-issue to converge" recovery path cannot run (concurring)

I traced this independently and reach the same conclusion. The success-path comment in rename_agent_cascade (api_config.rs ~1499) states:

persist-first makes them all idempotently re-runnable from the corrected config ... so re-issuing the same rename converges.

The comment's own parenthetical is the contradiction: "every owned-store op re-points by WHERE agent_alias = from". That re-point only happens if rename_agent_cascade runs again with from still present in config. But persist_and_swap has already committed config to to, so a re-issued from -> to request enters rename_with_cascade, the source alias is absent, and it returns RenameError::NotFound before move_renamed_workspace or the owned-store cascade can re-run. In the partial-failure window (config durably to, workspace or an owned store still at from) the operator has no working recovery command, which is exactly the contract this code documents and relies on.

The two regression tests pass locally:

agent_rename_leaves_owned_state_put_when_persist_fails       ok
agent_rename_moves_owned_state_after_successful_persist      ok

but neither forces a post-persist side-effect failure, so the broken recovery path has no coverage. The fix needs either a resume-aware path (detect from-absent + to-present and re-run only the side-effects) or an explicit compensation route, plus a test that drives a post-persist failure and proves convergence — as both prior reviews asked.

🟢 The persist-first reorder is the right fix for the clean-abort case

Persisting config before the workspace move and owned-state cascade does fix the #7907 split-brain: an early persist failure now returns before any side-effect runs, leaving config, workspace, and owned state all on from. Scoping the parallel delete_agent_cascade defect out to #7941 is the right call. The reorder and its abort-path test are good; the gap is strictly the partial-failure recovery contract above.

@Audacity88's CHANGES_REQUESTED is the gating review and remains accurate; @WareWolf-MoonWall already concurred. Posting this as a third concurring read rather than another formal block. Separately, the Check (32-bit) job needs a green rerun before merge.

[Nillth]

Thanks all - and Audacity88, good catch on the resume trigger. You're right that detecting the resume purely from config state was too loose. Fixed: the resume is now request-correlated to genuine partial-failure residue, not just the committed-to config shape.

The defect, concretely (two layers):

1. The persist-first reorder ([Bug]: agent rename can move owned state before config persistence #7907) fixed the clean-abort split-brain but left the post-persist recovery contract broken: once persist_and_swap commits config to to, a re-issued from -> to reaches rename_with_cascade and is rejected before any side-effect can re-run - NotFound (source absent) or an InvalidName collision (target present). So in the partial-failure window the WARN's "re-issue the rename to converge" silently failed at the API layer. I made that path resume-aware.

2. Audacity88's concern: my first resume guard was agent(from).is_none() && agent(to).is_some() - pure config state. An unrelated X -> to (where to already exists and X is just absent from config) matches that exact shape too, and would be silently swallowed as a "resume" - returning 2xx and running no-op side-effects instead of surfacing the operator's error. Benign today (the owned-store repoints are WHERE agent_alias = X, matching nothing) but a real correctness gap.

The fix (crates/zeroclaw-gateway/src/api_config.rs): only treat committed-to as a resume when there is actual lagging side-effect residue still referencing from - the exact fingerprint a real partial failure leaves:

let committed_to = working.agent(from).is_none() && working.agent(to).is_some();
let dirty_count = if committed_to && rename_residue_exists(state, &working, from).await {
    0 // genuine resume: skip the already-done rewrite+persist, re-run lagging side-effects only
} else {
    // normal path: rewrite refs, persist FIRST (#7907), then side-effects
};

When config shows to + from absent but no residue under from, it is NOT a resume (unrelated request, or an already-fully-converged duplicate) → it falls through to the normal branch, where rename_with_cascade(from -> to) with from absent returns NotFound (or a to collision) → rename_error_response. The operator's error surfaces instead of a silent success.

rename_residue_exists probes every store the side-effects touch (a false negative here would break a genuine resume in the store it missed, so each probe is the read-only twin of the corresponding mutation in move_renamed_workspace + cascade_rename_agent):

• workspace - the default per-alias dir for from still exists (agent_workspace_dir(from).exists(); custom alias-independent paths aren't moved, so they aren't residue);
• cron - cron::list_jobs_by_agent(from) non-empty;
• acp - AcpSessionStore::list_sessions_by_agent(from) non-empty. Note I probe all sessions (live OR killed), not count_live_sessions_by_agent: rename_sessions_by_agent re-points both, so a live-only probe would miss killed-only residue → false-negative resume break;
• memory - new read-only Memory::count_agent(from). It mirrors exactly what rename_agent moves: the SQL backends re-point the agents row (alias → UUID), not the memories rows, so the probe counts the agents row (an agent with an agents row but zero memories still gets re-pointed - an export_agent/memory-row count would have been a false negative). Qdrant (alias lives in the point payload) counts matching points; lucid delegates to its local SQLite mirror; markdown/none default to 0 (no DB rows, rename_agent is a no-op);
• sessions - new read-only SessionBackend::count_agent_attribution(from), mirroring the WHERE agent_alias = from predicate rename_agent_attribution re-points (SQLite override; file/no-metadata backends default to 0).

The two new read-only methods are minimal, non-mutating, default-implemented (so no backend is forced to implement them), and carry no #[allow(dead_code)] - both are wired into the residue probe. Lock discipline preserved: rename_residue_exists takes a short-lived state.config.read().clone() and never holds the guard across an .await.

Tests (api_config.rs):

• Kept agent_rename_resume_converges_when_config_already_to - a genuine resume (cron + workspace residue under from) still converges 2xx.
• Added agent_rename_unrelated_collision_is_not_treated_as_resume - committed-to, source absent, and no residue under the source → asserts the response is an error (NotFound), not a silent 2xx, and config is left untouched. This pins Audacity88's exact scenario.

Validation (cargo +1.93.0): fmt --all clean; clippy -p zeroclaw-gateway --all-targets clean (also clippy-clean on zeroclaw-memory + zeroclaw-infra for the new APIs); test -p zeroclaw-gateway agent_rename = 4 passed (all rename tests incl. the new one); full zeroclaw-memory + zeroclaw-infra suites green.

Scope note: kept surgical to the rename path; did not touch the analogous delete_agent_cascade concern (tracked separately as #7941).

[Audacity88]

Reviewed current head 90271620af6f227c05ac90d518a90c5ba9998d42 against the live PR state, linked issue #7907, scoped follow-up #7941, my prior CHANGES_REQUESTED, the concurring WareWolf-MoonWall and singlerider comments, the author follow-up comments, current checks, and the current gateway/memory/session diff. I did not run local Cargo validation.

✅ Resolved — the re-issued rename now reaches the side-effect recovery path

My prior blocker was that the post-persist warning told operators to re-issue the same rename, but a second from -> to request would fail at the config rename layer before move_renamed_workspace or cascade_rename_agent could run. The current head fixes that by detecting the already-committed config shape and, when real lagging residue still exists under from, skipping the already-done config rewrite/persist and rerunning only the idempotent side effects.

That is the recovery contract the earlier review asked for. agent_rename_resume_converges_when_config_already_to now seeds the partial-failure shape directly: config already names to, cron and the workspace still lag under from, and re-issuing the same rename converges both side effects instead of returning NotFound.

✅ Resolved — the resume path is no longer just config-shape based

The first resume fix would have treated any from-absent / to-present config as a resume, which could swallow an unrelated operator error. The new rename_residue_exists guard fixes that by requiring actual lagging side-effect residue under from before taking the resume branch. With committed to but no residue, the handler falls through to the normal rename_with_cascade path and returns the expected error.

The added agent_rename_unrelated_collision_is_not_treated_as_resume test pins that exact case. It is the important companion to the convergence test because it proves the recovery affordance does not turn every absent-source collision into a silent 2xx.

🟢 What looks good — the residue probes mirror the stores they repair

The read-only probes are scoped to the same stores the side-effect cascade mutates: default workspace path, cron rows, ACP sessions, memory backend state, and session attribution. The SQL memory probes count the agents alias row, matching what rename_agent updates; Qdrant counts payload points because that backend stores the alias in point payloads; Lucid delegates to its local SQLite mirror; and the session backend count mirrors the same agent_alias = from predicate used by rename_agent_attribution.

That keeps the resume decision tied to the state the reissued command can actually repair, while leaving the mirror delete_agent_cascade defect in #7941 as a separate, correctly scoped follow-up.

Approving from my side. This clears my previous block on #7940; the current checks are green, and the remaining delete-path sibling stays tracked separately in #7941.

[WareWolf-MoonWall]

Re-review — #7940 fix(gateway): persist agent rename before moving owned state

Reviewer: WareWolf-MoonWall
Prior head: b9bb2187 (Round 18 — 💬 comment, concurring with Audacity88's CR)
Current head: 90271620
Verdict: --approve

---

Overview

Substantial new work since b9bb2187: Nillth added rename_residue_exists, a resume-aware branch in rename_agent_cascade, a new count_agent default method on the Memory trait, and three regression tests covering the partial-failure convergence and the unrelated-collision guard. Audacity88's CHANGES_REQUESTED is cleared — they issued an APPROVED review at 90271620 with explicit ✅ Resolved markers on both prior blocking items. CI is fully green (18/18). No active CHANGES_REQUESTED from any reviewer.

---

✅ Resolved — "re-issue the rename to converge" recovery path now genuinely works

The prior blocker: after persist_and_swap commits config to to, a re-issued from -> to request was rejected at the config layer (RenameError::NotFound or InvalidName) before any side-effect could re-run. The fix detects the committed-to state and, when rename_residue_exists confirms actual lagging residue under from, skips the already-done rewrite+persist and re-runs only the idempotent side-effects (move_renamed_workspace + cascade_rename_agent). The convergence test agent_rename_resume_converges_when_config_already_to seeds the partial-failure shape directly (config already to, cron and workspace still at from) and proves re-issuing the same rename converges both rather than returning NotFound.

---

✅ Resolved — Resume path is request-correlated to actual residue, not config shape alone

The first resume guard (pure config-shape: from absent + to present) was too loose — an unrelated X -> to request where to already exists would have matched it and silently 2xx-ed. rename_residue_exists now gates the resume on actual lagging state: the default workspace dir still exists under from, OR cron rows, ACP sessions, memory count_agent, or session attribution are non-empty under from. With committed to but no residue, the handler falls through to the normal rename_with_cascade path and returns the expected NotFound or collision error.

agent_rename_unrelated_collision_is_not_treated_as_resume pins exactly this case — committed to, no residue, re-issued rename returns error rather than silent 2xx. This is the necessary companion to the convergence test.

---

🟡 Rollback section missing "Observable failure symptoms" sub-bullet for risk: high

The current rollback reads: "git revert — a self-contained reorder of one handler plus two tests and a test-only pub(crate) re-export. No state or schema migration." For risk: high the template requires three explicit sub-bullets, including observable failure symptoms. Suggested addition before squash-merge:

• Fast rollback command/path: git revert <sha>
• Feature flags or config toggles: None
• Observable failure symptoms: after a rename, workspace or owned-store state (cron jobs, memory, sessions) still homing to the old alias — visible as missing cron entries under the new agent or workspace directory residue at the old agent path; the WARN log "agent rename persisted but a post-persist side-effect did not follow" identifies the partial-failure window

Two lines to add; non-blocking given Audacity88's thorough review, but should be in the body before squash-merge.

---

🔵 Related: confirm disposition of #8018 after this update

WareWolf holds a Round 20 CHANGES_REQUESTED on #8018 (wangmiao0668000666's follow-up for the same #7907 issue) citing missing attribution for cherry-picked Nillth code. Now that #7940 has been substantially updated with the resume-aware path, it would be worth confirming whether #8018 should land as a further independent fix, be superseded by this PR, or be updated with the attribution fix and merged as a follow-on. Not a merge blocker here, but the two tracks should be reconciled before one of them merges without the other being accounted for.

---

🟢 What looks good — rename_residue_exists mirrors exactly what the cascade mutates

The probe covers the same five stores move_renamed_workspace and cascade_rename_agent touch: default workspace path, cron rows, ACP sessions (live and killed), memory count_agent (the agents alias row, not a memory-row count — correctly chosen per the doc), and session attribution. The doc comment is explicit: "MUST mirror every store ... or a false negative here would break a real resume in the store it missed." The count_agent default of Ok(0) is safe behavior — a backend that can't probe returns no residue, so the resume doesn't fire and the operator gets the normal error rather than a silent partial-success.

🟢 What looks good — count_agent trait method is clean, no DRY violation

count_agent on Memory is a new method with a default impl returning Ok(0) for backends that have no DB rows to probe (markdown, none). It reads from each backend's own canonical store without duplicating state. Each SQL backend's implementation probes its own rows — the canonical source of truth remains the DB, not a cache.

🟢 What looks good — CI is fully green (18/18)

All checks pass including Lint, Test, Check (32-bit), and the CI Required Gate.

[WareWolf-MoonWall]

@singlerider — now that this PR is approved and the resume path is addressed, please confirm disposition of #8018 (wangmiao0668000666's follow-up which cherry-picked this fix). If #7940 lands first, #8018 becomes redundant and should be closed. If #8018 is intended to supersede #7940, that should be declared per the supersede-attribution template.