fix(gateway): persist agent delete config before archiving owned state (#7941)

[wangmiao0668000666]

Summary

• Base: master (commit 74a806943)
• Problem: delete_agent_cascade runs the archive + owned-state cascade before persist_and_swap. A persist failure leaves config naming the agent while its workspace has been archived and its owned stores (memory / cron / acp / session) have been purged — the inverse of the [Bug]: agent rename can move owned state before config persistence #7907 rename split-brain, in the delete direction.
• Why: the rename path was already reordered in PR fix(gateway): persist agent rename before moving owned state #7940. The delete path is the mirror defect; Nillth scoped it out to keep the p1 tight and filed [Bug]: agent delete can purge owned state before config persistence (mirror of #7907) #7941 for the follow-up. Fix is "mechanically-identical reorder" (his words) plus an additive warnings field on the response.
• What changed:
  • crates/zeroclaw-gateway/src/api_config.rs::delete_agent_cascade — reorder: delete_with_cascade + mark dirty + persist_and_swap first, then archive + cascade_owned_state from the post-swap config.
  • crates/zeroclaw-gateway/src/api_config.rs::MapKeyResponse — add warnings: Vec<String> (#[serde(default, skip_serializing_if = "Vec::is_empty")]).
  • crates/zeroclaw-gateway/src/api_config.rs::tests — three real-sqlite regression tests:
    1. agent_delete_leaves_owned_state_intact_when_persist_fails (cron probe)
    2. agent_delete_purges_owned_state_after_successful_persist (cron probe + workspace-archive check)
    3. agent_delete_response_carries_partial_failure_warnings (read-only archive root → warnings field in 200 OK)
  • crates/zeroclaw-gateway/src/api.rs — test_state becomes pub(crate) and is re-exported at the crate root via pub(crate) use tests::test_state;, mirroring the helper Nillth added for the [Bug]: agent rename can move owned state before config persistence #7907 rename coverage. Without this, the new regression tests in api_config can't reach the AppState builder.
• Out of scope: the recovery-path blocker on PR fix(gateway): persist agent rename before moving owned state #7940 (the "re-issue to converge" path that doesn't actually run). That belongs to the [Bug]: agent rename can move owned state before config persistence #7907 PR's review thread, not to this delete-side fix.

Label Snapshot

• Risk: medium
• Size: S
• Scope: gateway
• Module: config / agents
• Contributor tier: trusted (5+ PRs once this lands)

Change Metadata

• Type: fix
• Primary scope: crates/zeroclaw-gateway/src/

Linked Issue

Closes #7941
Related: #7907 (the rename-direction mirror of this defect)

Supersede Attribution

None.

Validation Evidence

$ cargo fmt --all -- --check
(no output — clean)
$ cargo clippy --locked -p zeroclaw-gateway --all-targets -- -D warnings
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 21.89s
$ cargo test --locked -p zeroclaw-gateway --lib
test result: ok. 266 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
$ cargo test --locked -p zeroclaw-gateway --lib api_config::tests::agent_delete
test api_config::tests::agent_delete_leaves_owned_state_intact_when_persist_fails ... ok
test api_config::tests::agent_delete_purges_owned_state_after_successful_persist ... ok
test api_config::tests::agent_delete_response_carries_partial_failure_warnings ... ok
test result: ok. 3 passed; 0 failed

Real behavior proof

• Behavior addressed: DELETE /api/config/map-key?path=agents&key=<alias> must leave the agent's workspace and owned stores intact on a persist failure, and must surface partial side-effect failures in the response body (not just the server log).
• Real environment tested: Linux x86_64, Rust 1.94, zeroclaw 0.8.1 / master @ 74a806943.
• Exact steps run after this patch:
  • cargo test --locked -p zeroclaw-gateway --lib api_config::tests::agent_delete
  • cargo test --locked -p zeroclaw-gateway --lib (full crate lib tests, 266 passed)
  • cargo clippy --locked -p zeroclaw-gateway --all-targets -- -D warnings (clean)
  • cargo fmt --all -- --check (clean)
• Evidence after fix:

  test api_config::tests::agent_delete_leaves_owned_state_intact_when_persist_fails ... ok
  test api_config::tests::agent_delete_purges_owned_state_after_successful_persist ... ok
  test api_config::tests::agent_delete_response_carries_partial_failure_warnings ... ok
  test result: ok. 3 passed; 0 failed
  

  The three tests use real rusqlite cron probes (not mocks). The persist-fail test makes config_path itself a directory so save_dirty's atomic write cannot complete; the cron row stays under victim and the workspace directory is never created. The happy-path test asserts the cron row is purged, the workspace is gone from its original path, and a victim-<ts> archive directory exists with a workspace/ subdir. The partial-failure test chmods the archive root to 0o555 (unix-only) and asserts the response body has a warnings array with at least one archive mention.
• Observed result after fix: the response shape changes only by gaining an optional warnings field (omitted when empty). The persist-fail / persist-ok / partial-fail invariants from the issue body all hold.
• What was not tested: the test suite does not exercise the actual HTTP handler endpoint — it calls the delete_agent_cascade function directly with a constructed AppState. The HTTP layer is a thin axum wrapper; the cascade logic that the issue calls out is fully covered.

Security Impact

• Permissions: no new permission checks. The handler still requires require_auth upstream (unchanged).
• Network calls: none added.
• Secrets: none touched.
• File access: archive-dir creation, workspace fs::rename, and the per-store purges are the same as before — only the ordering changed. The persist-first reorder means a partial-failure window now shows config already deleted while workspace / owned stores may still be present, so the operator MUST inspect the warnings field. This is documented in the response struct's doc comment.

Privacy and Data Hygiene

• Archive dir still lives under data_dir/agents/_deleted/<alias>-<ts>/ and continues to receive the owned-state export.
• No new data is written outside the existing archive path.
• No telemetry surface changes.

Compatibility / Migration

• Response shape: MapKeyResponse gains an optional warnings: Vec<String> field. The field is #[serde(default, skip_serializing_if = "Vec::is_empty")], so the wire format is byte-identical for the non-agent MapKey callers (generic create / post-replace). Only the agent-delete path can populate it.
• API contract: the success path still returns 200 OK with the same path / key / created: false body. A persist failure still returns the same error response (4xx with ConfigApiCode::ReloadFailed). The new behavior is strictly additive on the response shape.
• Migration: none required for clients. Existing clients ignore the optional warnings field; new clients can act on it.

i18n Follow-Through

No user-visible string changes. The warnings array content is operator-facing English (e.g. "archive dir creation failed (...): <io error>"); it does not pass through Fluent.

Human Verification

• Ran cargo test --locked -p zeroclaw-gateway --lib locally — 266 passed, 0 failed.
• Inspected the diff manually: the reorder places persist_and_swap between the delete_with_cascade block and the archive block; the working move into persist_and_swap is the last use of working before the post-swap read-back of state.config for data_dir and the cascade.

Side Effects / Blast Radius

• The MapKeyResponse schema gains an optional field. The OpenAPI generator (schemars::JsonSchema) re-derives the schema from the struct, so the generated OpenAPI document now lists warnings as an optional array of strings.
• The pre-push hook in .githooks/ runs rust_quality_gate.sh + cargo test --locked — both pass on the affected crate.

Agent Collaboration Notes

• The test_state visibility change (fn → pub(crate) + pub(crate) use tests::test_state;) is the same pattern Nillth added to api.rs in PR fix(gateway): persist agent rename before moving owned state #7940. If that PR lands first, this PR's visibility change is a no-op (the symbol is already pub(crate)); if it lands second, Nillth's diff will overlap and one of us will need to rebase. The diff is two lines either way; rebase is trivial.
• The new warnings field on MapKeyResponse is parallel to the existing warnings field on RenameMapKeyResponse (line 1282). If the maintainer prefers a single shared type, that's a follow-up refactor.

Rollback Plan

• Revert the commit on the merge commit.
• No feature flag involved; the change is a pure reorder + additive response field.
• MapKeyResponse::warnings is #[serde(default, skip_serializing_if = "Vec::is_empty")] so rolling back leaves the wire format unchanged for non-agent callers.

Risks and Mitigations

• Risk: a third caller of MapKeyResponse outside api_config breaks because the new field is mandatory in construction. Mitigation: grep confirmed all three call sites — delete_agent_cascade (this PR), handle_map_key (created + replaced, both updated), and the create-on-empty path. All four are updated. If a downstream consumer constructs MapKeyResponse via struct-literal, they need to add warnings: vec![]. The compiler will catch any miss.
• Risk: the test_state visibility change in api.rs widens the test-only API surface. Mitigation: #[cfg(test)] on both the pub(crate) use line and the pub(crate) fn — production builds are unaffected.
• Risk: partial-failure test is unix-only (#[cfg(unix)] chmod); the windows behavior is unverified. Mitigation: the test is #[cfg(unix)]-gated so the CI macOS + Linux runners cover it; the windows runner is not in the project's required-CI matrix. The cascade code itself is platform-agnostic — only the failure-injection mechanism is unix-specific.

[wangmiao0668000666]

@Audacity88 ready for review when you have a moment. All CI checks are passing locally (cargo fmt / clippy -D warnings / cargo test --locked), and the diff is scoped to fix #7941's root cause (agent delete config now persists before the owned-state archive). Happy to address any feedback — small, medium, or large — in this same PR if it fits, or split into a follow-up if not. No rush; just letting you know this is active and ready to go.

[wangmiao0668000666]

@Audacity88 Rebased onto current master b10d22270 (force-pushed to bf3403a4e). 14 upstream commits picked up, including Nillth's merged #7940 (rename-side split-brain fix).

Re-applied the delete-side persist-first reorder on top of Nillth's rename-side fix — both rename_agent_cascade (4 tests) and delete_agent_cascade (3 tests) now use the persist-first contract. The 4 + 3 regression tests cover the persist-fail-leaves-state-intact, persist-success-runs-side-effects, and partial-failure-aggregates-warnings shapes; both pass locally (cargo test --locked -p zeroclaw-gateway --lib agent_rename agent_delete → 7/7). No production code outside delete_agent_cascade touched.

Local validation:

cargo check --locked -p zeroclaw-gateway --tests    ✓
cargo test --locked -p zeroclaw-gateway --lib        277 passed; 0 failed
cargo clippy -p zeroclaw-gateway --all-targets -D warnings  ✓

Ready for review.

[singlerider]

Reviewed at head 86958ca. Fixes #7941, the delete-direction mirror of the #7907 rename split-brain. I cross-checked the reorder against the live handler and the regression tests. No prior reviews, no other blocks.

🟢 What looks good — the persist-before-side-effects reorder is correct and complete

delete_with_cascade + mark_dirty over cascade.dirty_paths() + persist_and_swap now run first; archive and cascade_owned_state run afterward from the read-back committed config. A persist failure therefore returns an error with the agent still named in config and its workspace and owned stores untouched, which is exactly the inverse split-brain #7941 describes. agent_delete_leaves_owned_state_intact_when_persist_fails proves it: persist is forced to fail (config_path is a directory), and the seeded cron row, the workspace dir, the absence of an archive dir, and the un-swapped in-memory config are all asserted.

🟢 What looks good — the slice-4 workspace-resolution lesson is preserved

workspace is still resolved via agent_workspace_dir(alias) BEFORE the cascade removes the entry, with the explanatory comment about custom-workspace fallthrough. Only data_dir is read from the post-swap config, which is safe because data_dir is global, not per-agent. That is the right split: per-agent state captured pre-mutation, global state read post-commit.

🟢 What looks good — partial failures are surfaced, not just logged

MapKeyResponse.warnings: Option<Vec<String>> with skip_serializing_if = Option::is_none keeps the generic create/delete map-key responses byte-identical (they set None) while the agent-delete path now combines archive-dir, workspace-rename, and cascade_owned_state per-store failures into the 200 body. agent_delete_response_carries_partial_failure_warnings exercises the read-only-archive-root path end to end. The test_state re-export mirrors the helper from the rename coverage rather than widening any production surface.

Verdict: APPROVED.

[Audacity88]

Reviewed current head 86958ca against live PR state, #7941, the related #7940 rename-side fix, the gateway/config source, and the visible CI results. I did not run local Cargo validation in this review-only pass. I agree with @singlerider's approval: the delete-side fix matches the accepted persist-first contract.

🟢 What looks good — persist happens before destructive delete side effects

delete_agent_cascade now resolves the agent workspace while the agent entry still exists, runs delete_with_cascade, marks every dirty cascade path, and calls persist_and_swap before archiving the workspace or purging owned stores. That is the important #7941 safety property. If persistence fails, the handler returns before moving the workspace or touching cron/memory/acp/session state, so config and owned state stay consistent.

🟢 What looks good — post-persist partial failures now reach the caller

The new optional MapKeyResponse.warnings field is only populated on the agent-delete path. Generic map-key create/delete responses keep the old JSON shape when there are no warnings. The delete path combines archive-dir, workspace-rename, and cascade_owned_state warnings into the response body, so a committed config delete with a failed side effect is no longer a clean-looking 200 that only leaves evidence in server logs.

🟢 What looks good — regression coverage hits the right boundary

The tests cover the three load-bearing cases: persist failure leaves the cron row and workspace intact, successful persist removes the config entry and then purges/archives owned state, and a post-persist archive-side failure returns a warnings array. I also agree with @singlerider that preserving the pre-delete workspace resolution is the right carry-over from the rename-side fix, especially for custom workspace paths.

One merge-readiness note, separate from this approval: current master has moved over api.rs, api_config.rs, and adjacent config/cron surfaces since this branch's base. GitHub reports the branch as cleanly mergeable, but this high-risk gateway path should still get a branch refresh and refreshed CI, or equivalent merge-result evidence, before landing.

[wangmiao0668000666]

Thanks @Audacity88 for the merge, and thanks to @singlerider and @Audacity88 for the reviews on the delete-side persist-first fix and the partial-failure warnings. 🙏