Skip to content

feat(web): add safe persisted session deletion - #396

Open
testikun wants to merge 3 commits into
openpi-dev:mainfrom
testikun:codex/issue-347-session-delete
Open

feat(web): add safe persisted session deletion#396
testikun wants to merge 3 commits into
openpi-dev:mainfrom
testikun:codex/issue-347-session-delete

Conversation

@testikun

@testikun testikun commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Problem

Related to #347. Web Workbench can archive Sessions but cannot safely remove a persisted non-active Session. Deletion must not be confused with archive metadata removal, and the active Session must never be deleted.

Value

Adds a bounded, auditable persistence-management primitive for Session retention while keeping Pi JSONL files authoritative and preventing accidental active-session loss.

Approach

  • Add an authenticated DELETE /api/sessions?path=... endpoint.
  • Resolve the target from the canonical Pi Session projection and require a .jsonl file inside the configured Web Session directory.
  • Reject the active Session with an explicit 409 SESSION_CONFLICT response.
  • Remove the persisted file first, then clean Web-owned archived and ungrouped derived indexes.
  • Publish a session_deleted event for connected clients.
  • Keep native confirmation/UI, archive browsing, historical editing, and fork semantics out of this backend-only slice; those remain separate lifecycle work.

Validation

  • Focused Node tests: 34 passed, 0 failed.
  • Full Node/Vitest suite: 1339 passed, 1 skipped, 0 failed; Vitest 30 passed.
  • biome format / biome lint --error-on-warnings: passed.
  • tsc --noEmit: passed.
  • Config-contract, discipline-ledger, and Web syntax checks: passed.
  • Ablation: removing the typed deletion-conflict error caused the active-session API regression to return 500 instead of the required 409; the error boundary was restored.
  • bun is not installed in this environment, so the equivalent repository scripts were run with the bundled Node 24 executable and local Biome/Vitest binaries.

Impact

  • User-visible behavior: backend deletion API and deletion event; no UI changes in this PR.
  • Model-visible context/tools: none.
  • Runtime/lifecycle: active Session deletion is rejected; non-active persisted files are removed.
  • Persisted data: targeted JSONL file is deleted and Web-owned index entries are cleaned.
  • Compatibility/risk: endpoint is authenticated and exact-target bounded; native confirmation remains a required follow-up before exposing this mutation in UI.

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact-head review: persisted Session deletion is valuable, but two blockers remain. Focused adapter/host tests passed 42/42. An additional probe using the real PiWebRuntime activation/retention methods, real Pi SessionManager files, and PiWebAdapter reproduced deletion of a still-streaming background Session and loss of its original history. The fake agent lifecycle seam follows existing runtime tests; no provider call or installed UI acceptance is claimed. The destructive endpoint also omits the native reviewed confirmation required by #347. No source changes or merge performed.

Comment thread web/adapter/pi-adapter.ts
const canonical = resolve(session.path);
const activePath = this.runtime.sessionManager.getSessionFile();
if (
session.id === this.runtime.sessionManager.getSessionId() ||

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Protect every live Session owner before deleting its file

This checks only the foreground runtime.sessionManager. PiWebRuntime deliberately retains a previous runtime while it is streaming after activateCandidate switches to another Session. Reproduced A streaming → switch to B → delete A: deletion succeeds while A remains in retainedRuntimes with isStreaming=true. A subsequent real SessionManager.appendMessage recreates the file with only that new message, without the Session header or original history. Please move deletion admission behind the runtime lifecycle boundary so retained/in-flight owners and transitions cannot race with deletion; cover this supported background-generation path.

Comment thread web/host/web-host.ts
if (!path)
return this.json(response, 400, { error: "session path is required" });
try {
const deletedPath = await this.adapter.deleteSession(path);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Enforce the required reviewed confirmation before exposing deletion

Issue #347 explicitly requires destructive deletion to show the exact persisted target and use native reviewed confirmation. This authenticated endpoint already calls rm through the adapter with only a path, so a direct API request bypasses that requirement entirely. Deferring confirmation to a future UI does not protect the callable backend mutation. Please enforce confirmation bound to the exact canonical deletion target before committing the operation, with a regression proving a bare authenticated DELETE cannot delete it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants