Skip to content

One public path to close a session completely, used by disconnect, backup import and embedding app crates #816

Closed

Description

@smart-ssh-planner

Goal

There is exactly one function that closes an open session completely — elevated file-browser channel revoked, local path grants removed, pending confirmation rejected, running command cancelled and drained, transport disconnected, status event sent. Disconnect, the backup import and any embedding app crate or extension (Tauri plugin) close sessions only through it, so a closed session never keeps an elevated channel or grants alive.

Context

  • Canonical close sequence today: crates/app-shell/src/commands/chat.rs:627-700 (disconnect): elevated.remove_session(…) (:639), auto_reconnect.stop(), grants.remove_session (:647), reject_pending_confirmation_on_close (:656), transport.mark_closed(), cancel the running command and wait_until_idle (2 s), disconnect(), drop the terminal sender, emit_connection_status_changed(Disconnected), remove the edit-session temp directory (:700); then disconnect-specific work (end the chat session record, note suggestion task).
  • Copy in the backup import: crates/app-shell/src/commands/backup.rs:217-262 (ShellSessions::close_affected, Spec 0109 I8). It uses elevated.remove_session but misses grants.remove_session, the cancellation of a running command and wait_until_idle.
  • ElevatedSftpRegistry::remove_session (crates/app-shell/src/elevated_sftp.rs:273-311) is pub(crate) and documented as "the one function" that removes a session and its elevated channel (Spec 0084 A2.1: nothing but it and tests calls SessionManager::remove). Revoking means mark_revoked() + stop_elevated(…, Revoked).
  • An embedding app crate or extension that needs to close the open sessions (e.g. before replacing data, like the backup import) cannot reach remove_session and has to reimplement the sequence with state.sessions.remove(…) (SessionManager::remove, crates/app-logic/src/session.rs:1842), which skips the elevated-channel revocation.
  • Spec 0084 §7 notes that the "one place removes sessions" promise is secured only by search, not by the compiler; there is no guard test.
  • The trait app_logic::backup::OpenSessions (affected, close_affected) is what the import calls.

Requirements

  • Add one public close path in app-shell, e.g. pub async fn close_session(app: &AppHandle<R>, session_id: SessionId) -> bool (true if a session was closed) and a public type implementing app_logic::backup::OpenSessions that closes every non-local open session through it (the current ShellSessions, made public, e.g. app_shell::sessions::AppOpenSessions). Export both from crates/app-shell/src/lib.rs.
  • The close function runs the full sequence from disconnect in the same order: ElevatedSftpRegistry::remove_session (stays pub(crate), called only from here) → stop auto-reconnect → LocalPathGrants::remove_session → reject_pending_confirmation_on_close → mark_closed → cancel the running command → wait_until_idle (2 s timeout) → disconnect() (2 s timeout) → drop the terminal sender → ConnectionStatus::Disconnected event → remove the edit-session temp directory.
  • disconnect calls the new function and keeps its own extra work afterwards (ending the chat session record, the note-suggestion task); its user-visible behaviour does not change, including the "Session nicht gefunden" error and the MCP-session cleanup when no Session exists.
  • ShellSessions::close_affected (backup import) uses the same function; the local pseudo-server is skipped as today.
  • Guard test in app-shell (source scan with self-test): outside crates/app-shell/src/elevated_sftp.rs, SessionManager's own definition and test files, no Rust source in crates/ calls SessionManager::remove (pattern for .sessions.remove( on the session manager; the MCP session map in crates/app-logic/src/mcp_sessions.rs is a different map and is allowlisted by path). The self-test proves the scan finds a planted call.
  • The public API is documented (//!////) as the only supported way for embedding crates to close sessions; docs/architecture.md (section on the elevated channel) names it.
  • Update Spec 0084 (A2.1 / §7: the one place is public as the close path, guarded by a test) and Spec 0109 I8 (import closes sessions through the same path).

Acceptance Criteria

  • Test: an elevated session closed via the new public function ⇒ its slot is revoked and stop_elevated(…, Revoked) was called; grants of the session are gone; a pending confirmation is rejected.
  • Same test via disconnect and via the backup import's close_affected (all three paths).
  • Test: backup import while an AI command runs ⇒ the command is cancelled and the close waits for idle (bounded by the timeout) before disconnecting.
  • disconnect behaviour unchanged: existing disconnect tests stay green; unknown session id still yields the same error.
  • Guard test: no SessionManager::remove call outside the allowlisted places; the self-test fails on a planted call.
  • The public function and type are reachable from a crate outside app-shell (a doc test or an integration test under crates/app-shell/tests/ that only uses the public API compiles).
  • Regression test for the backup-import gap fails on the current code (counter-proof noted in the PR).
  • Specs 0084 and 0109 and docs/architecture.md are updated.
  • Relevant automated tests pass (cargo test -p app-shell -p app-logic).

Technical Notes

  • Security-critical (elevated channel, pending confirmations, execution path; review priority ELEVATED). Read .claude/skills/smart-ssh-coder/references/security-invariants.md. Keep the lock order documented on ElevatedSftpRegistry (session removed before the slot entry) and the Spec 0084 §9 clarification (revocation is the immediate flag; disconnect must not wait on a hanging transfer).
  • Do not make ElevatedSftpRegistry::remove_session or the registry itself public; only the complete close path is public.
  • Keep app-logic Tauri-free: the close path lives in app-shell (it needs AppHandle state).
  • Do not change what the AI or MCP can reach: the elevated channel stays unreachable for them (Spec 0067 A).

Spec impact: update 0084, update 0109

Open questions

  • Generic over the Tauri runtime? Options: (a) close_session<R: tauri::Runtime>(app: &AppHandle<R>, …) so a Tauri plugin generic over R can call it, (b) concrete AppHandle (default runtime) like the existing commands. Recommendation: (a) if the managed state lookups work generically (they do via Manager::state); otherwise (b) and document that callers use the default runtime.
  • Module name. Recommendation: a new crates/app-shell/src/sessions.rs (pub mod sessions) holding close_session and the OpenSessions implementation, with disconnect and backup.rs calling into it.

Related Work

Activity

  1. added this to the 0.8.0 milestone on Oct 26, 2026
  2. smart-ssh-coder commented on Oct 26, 2026

    @smart-ssh-coder
    Contributor

    Implementation ready for review. PR #929. Added app_shell::sessions (close_session, AppOpenSessions) as the single full close path; disconnect and backup import use it. Added tests for revocation, grants and confirmation, running-command drain, the source-scan guard (with self-test) and public API reachability. Specs 0084/0109, architecture doc and changelog updated. CI green on PR #929.

  3. smart-ssh-reviewer commented on Oct 26, 2026

    @smart-ssh-reviewer

    Review passed – moving on to merge. Approved: single public close path, import now closes fully, counter-proofs verified, specs updated.

  4. smart-ssh-coordinator commented on Oct 26, 2026

    @smart-ssh-coordinator
    Contributor

    Merge not possible, back to coding: PR #929 has conflicts with develop – please resolve them.

  5. smart-ssh-coder commented on Oct 26, 2026

    @smart-ssh-coder
    Contributor

    Implementation ready for review. PR #929. Merged develop into the branch; the only conflict was in Spec 0109 (kept the I8 text about the full close path and added the new I10 from develop). cargo check passes and CI is green on PR #929.

  6. smart-ssh-reviewer commented on Oct 26, 2026

    @smart-ssh-reviewer

    Review passed – moving on to merge. Re-approved after the develop merge: the PR diff is unchanged in substance, CI is green, and cargo test -p app-shell passes (405 tests).

  7. smart-ssh-coordinator commented on Oct 26, 2026

    @smart-ssh-coordinator
    Contributor

    Merged and branch deleted: PR #929 (squash) in develop

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

Metadata

Metadata

Assignees

No one assigned

    Type

    Fields

    Priority

    P1 - High

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions