Description
ZephAcpAgentState::do_delete_session (crates/zeph-acp/src/agent/mod.rs:1725-1736, feature unstable-session-delete) only removes the session from the in-memory sessions map. It never touches self.store, so when a persistence store is configured (the standard production configuration), the session's row in the acp_sessions table — and its associated conversation history / config snapshot — is never deleted.
The store already has a purpose-built, owner-scoped method for exactly this: SqliteStore::delete_acp_session_for_owner (crates/zeph-memory/src/store/acp_sessions.rs:534-547), added under the #5868 owner-scoping epic. do_delete_session simply never calls it. Compare with do_close_session (line 1668), which does read/write self.store (to persist a config snapshot on graceful close) — do_delete_session has no store interaction at all.
The handler's own doc comment claims this is "Permanent deletion — no usage summary is sent," which is not what happens when persistence is enabled.
Impact
- A user who explicitly deletes a session (e.g. because it contains sensitive content) has their conversation history and session metadata retained indefinitely in the database — contrary to the documented and reasonably-expected semantics of "delete."
do_list_sessions (line 1849) merges persisted rows via list_acp_sessions_for_owner — so a "deleted" session reappears in session/list on any later call.
- The session is fully recoverable via
session/load or session/resume: claim_acp_session_for_owner still finds the row (owned by the same connection's owner_key) and successfully claims/loads it, resurrecting a session the client believed was gone.
Reproduction
- Configure
ZephAcpAgentState with a SqliteStore (.with_store(...)).
- Create a session (
session/new), send a prompt so history is persisted.
- Call
session/delete for that session id.
- Call
session/list — the session reappears (persisted row still present).
- Call
session/load for the same id — it loads successfully with full prior history.
Note: the existing integration test delete_session_removes_session_from_list (crates/zeph-acp/tests/integration.rs:863) does not catch this — it uses test_config(), which leaves store: None (AcpServerConfig::default()), so the store code path is never exercised.
Expected Behavior
do_delete_session should call store.delete_acp_session_for_owner(&args.session_id.to_string(), &self.owner_key) (mirroring the pattern already used by do_load_session/do_fork_session/do_resume_session for claim_acp_session_for_owner) before/after removing the in-memory entry, so persisted data is actually removed and the session cannot be resurrected.
Environment
- Version: HEAD
1fe1d0e2
- Features:
unstable-session-delete (also requires a configured SqliteStore, i.e. [session] enabled = true)
Suggested Fix
In do_delete_session, after removing the in-memory entry, add:
if let Some(ref store) = self.store {
if let Err(e) = store
.delete_acp_session_for_owner(&args.session_id.to_string(), &self.owner_key)
.await
{
tracing::warn!(error = %e, session_id = %args.session_id, "failed to delete persisted ACP session");
}
}
Also extend the integration test to wire a real store and assert the row is gone from the database (not just absent from session/list), since the current test's use of test_config() (no store) is exactly what let this regress silently.
Description
ZephAcpAgentState::do_delete_session(crates/zeph-acp/src/agent/mod.rs:1725-1736, featureunstable-session-delete) only removes the session from the in-memorysessionsmap. It never touchesself.store, so when a persistence store is configured (the standard production configuration), the session's row in theacp_sessionstable — and its associated conversation history / config snapshot — is never deleted.The store already has a purpose-built, owner-scoped method for exactly this:
SqliteStore::delete_acp_session_for_owner(crates/zeph-memory/src/store/acp_sessions.rs:534-547), added under the #5868 owner-scoping epic.do_delete_sessionsimply never calls it. Compare withdo_close_session(line 1668), which does read/writeself.store(to persist a config snapshot on graceful close) —do_delete_sessionhas no store interaction at all.The handler's own doc comment claims this is "Permanent deletion — no usage summary is sent," which is not what happens when persistence is enabled.
Impact
do_list_sessions(line 1849) merges persisted rows vialist_acp_sessions_for_owner— so a "deleted" session reappears insession/liston any later call.session/loadorsession/resume:claim_acp_session_for_ownerstill finds the row (owned by the same connection'sowner_key) and successfully claims/loads it, resurrecting a session the client believed was gone.Reproduction
ZephAcpAgentStatewith aSqliteStore(.with_store(...)).session/new), send a prompt so history is persisted.session/deletefor that session id.session/list— the session reappears (persisted row still present).session/loadfor the same id — it loads successfully with full prior history.Note: the existing integration test
delete_session_removes_session_from_list(crates/zeph-acp/tests/integration.rs:863) does not catch this — it usestest_config(), which leavesstore: None(AcpServerConfig::default()), so the store code path is never exercised.Expected Behavior
do_delete_sessionshould callstore.delete_acp_session_for_owner(&args.session_id.to_string(), &self.owner_key)(mirroring the pattern already used bydo_load_session/do_fork_session/do_resume_sessionforclaim_acp_session_for_owner) before/after removing the in-memory entry, so persisted data is actually removed and the session cannot be resurrected.Environment
1fe1d0e2unstable-session-delete(also requires a configuredSqliteStore, i.e.[session] enabled = true)Suggested Fix
In
do_delete_session, after removing the in-memory entry, add:Also extend the integration test to wire a real store and assert the row is gone from the database (not just absent from
session/list), since the current test's use oftest_config()(no store) is exactly what let this regress silently.