Repository navigation
Sessions: explicit lock hierarchy (SessionLocks) and documented lock order; disconnect during a running command returns quickly #923
Copy link
Copy link
Open
Description
Goal
The order in which a session's locks may be taken is enforced by the API and documented in one place, so new code cannot introduce a deadlock between SFTP, transport and the elevated channel.
Context
- A session holds about 20 locks: 6 tokio mutexes, 2 in the transport (
crates/app-logic/src/session.rs:~403/~407), 9 std mutexes, 3 std RwLocks, plus atomics (SessionParts,session.rs:696-991). Nested:crates/app-shell/src/elevated_sftp.rs:51/195(Arc<AsyncMutex<Option<_>>>inside aStdMutex<HashMap>). - The order is only stated in comments:
crates/app-logic/src/orchestration/remote_files.rs(SFTP before transport),crates/app-logic/src/start_directory.rs:157-163(avoids the reverse on purpose),crates/app-shell/src/elevated_sftp.rs:414-420(revocation outside the transport lock because ofdisconnect). crates/app-shell/src/elevated_sftp.rs:401-403holds the transport lock up toPROBE_TIMEOUTduring the elevation probe.- Since Keep the file browser, terminal and disconnect usable while a long command runs #328, AI commands release the transport lock after
start_exec(crates/app-logic/src/orchestration/action_exec.rs:2004-2016); only transports without split start keep it for the whole command. - No std guard is held across
.await(clippyawait_holding_lockfinds 0) – keep it so.
Requirements
- Introduce
SessionLocks(inapp-logic, next to the session parts after their split) that owns the async locks of a session and exposes acquisition methods only in the permitted order, e.g.sftp(),transport(),sftp_then_transport(); there is no method that takes transport and then SFTP. Elevated-channel slots get the same treatment (documented level). - All production sites that take more than one of these locks use
SessionLocks; single-lock sites may keep direct access through it. - Elevation probe: bound the time the transport lock is held to the probe command start (split start like Keep the file browser, terminal and disconnect usable while a long command runs #328) where the transport supports it.
- Document the hierarchy (levels, which lock protects what, rules: no std guard across
.await, revoke outside the transport lock) indocs/architecture.md, and link it from the comments that currently state the order. - Add
clippy::await_holding_lock = "deny"to the workspace lints if the lint table exists.
Acceptance Criteria
-
SessionLocksexists; acompile_faildoctest shows that acquiring transport then SFTP through the API does not compile (or is not expressible). - Test: disconnect while an AI command runs on a transport without split start (mock) returns within 2 s (ordering/virtual time), the command is cancelled.
- Test: disconnect during an elevation probe returns within 2 s.
-
docs/architecture.mdhas the lock hierarchy section; the three comments link to it. - Existing session, SFTP pairing (Spec 0086 A3), elevated revocation (Spec 0085) and cancel (Spec 0027) tests pass unchanged.
- Relevant automated tests pass.
Technical Notes
- Security-critical: session pairing (Spec 0086 A3), elevated channel revocation (Spec 0085), the execution path (
.claude/skills/smart-ssh-coder/references/security-invariants.md). Review priority elevated. - Builds on the split of
SessionPartsinto sub-structures.
Spec impact: update 0086 (lock order as part of A3)
Related Work
- Blocked by app-logic: group SessionParts fields into AiConfig, SafetyState, ChatPersistence and TransportState #871 (
SessionPartssplit). - Keep the file browser, terminal and disconnect usable while a long command runs #328, Spec 0027, Spec 0085, Spec 0086
Metadata
Metadata
Assignees
Type
Fields
Priority
P2 - Normal
Activity
3 remaining items