Keep the session maps unlocked across network I/O by Sadykhzadeh · Pull Request #140 · CodeAny-inc/Clavyn

Sadykhzadeh · 2026-10-16T22:34:58Z

Closes #139.

What this changes

Map locks are never held across network I/O

Both managers now keep Arc'd entries. A call takes the lock, clones the entry and releases it before awaiting anything on the network:

SSH writes. Writes to one session are still serialized, through a per-session writes lock, so keystrokes reach the channel in order. A peer that stops reading now holds up only its own input. The russh Handle no longer sits behind a mutex, since data and disconnect take &self.
SFTP. Every operation, transfers included, runs on its cloned Arc<SftpConnection>. A multi-GB download no longer blocks listing, renaming or closing on another host.
close / close_all. These remove entries first and then say goodbye (below). close_all sends its goodbyes in parallel.

This restores what contain_open_sessions relies on: after a vault reset, no SSH or SFTP session can still be addressed, whatever was running on it.

A close always ends the connection

russh has no call that stops a session. Handle::disconnect only queues a message, and russh stops reading that queue while a channel waits on the peer's window, so a goodbye to a peer that stopped reading never goes out. Dropping every handle doesn't end the session either while a channel half is still alive. Without a way to force it, a close on a stalled peer left the connection authenticated for good, kept the id claimed and never announced the close, even after the peer recovered.

connection::connect opens the TCP stream itself, wraps it in a Transport (new core/src/transport.rs) and returns Connection { handle, transport }.
Both managers close through one goodbye: queue the disconnect, wait up to 5 s for russh to end the session, then cut the stream. A cut fails every pending read and write, so russh ends the session and every channel on it.
After a close, the connection is gone within 5 s, the reader announces the close and the id is free for a reconnect, however the peer behaves.

A close ends what is already running

Each SSH session and SFTP connection carries a CloseSignal, raised by close, close_all and the reader's own cleanup. SSH writes and every SFTP call (listings, transfers and single requests) race against it. A call that already held the entry returns an error right away rather than when the cut lands.

SSH session lifecycle

The reader keeps only the channel's read half and never waits on outbound messages, so it always drains what arrives. Window changes go out from a task of their own.
resize replaces the pending size (a watch) and never waits. On a stalled peer, a bounded resize queue would fill up and every later resize would hang.
When the remote shell exits, the reader announces the close, then removes its own entry and says goodbye from a detached task. It removes the entry only if it is still the one holding this reader's claim, so it can't take a newer session that reused the id.
close drops the entry before awaiting the goodbye, so the entry's share of the id goes at once.
session_write and session_resize look the id up once and fall through to local terminals on SessionNotFound.

SFTP listings

SftpSession::read_dir collects a whole directory before returning anything, so it can't cap a listing or close the directory handle when one is abandoned. Listings now run on a second SFTP channel per connection, driven request by request through RawSftpSession:

Each READDIR is bounded by russh-sftp's 10 s per-request timeout. A server that goes quiet ends the listing, while a large directory on a slow link that keeps answering is listed in full.
A listing past 200,000 entries or 32 MiB of names is refused, so a server that never reports end-of-file can't grow it for good.
The directory handle is closed however the listing ends.

The listing channel opens alongside the main one, so it adds no round trips to the connect. A server that won't open a second session channel falls back to read_dir with an overall 30 s deadline.

Writers to one remote path take turns

SFTP calls on one session no longer serialize on the map lock, so two writers to the same path could run at once.

upload_from_local and write_file take a per-connection lock on their destination path. A second upload to the same path waits for the first to finish, then replaces a complete file. Without the lock, two overwrite uploads truncated and wrote the file together, leaving a mix of both, with zero-filled gaps, while both reported success. A path's lock leaves the map once nothing holds or waits on it.
Without overwrite the remote file is opened with CREATE | EXCLUDE | WRITE, so an upload is refused with "remote file already exists" even when another client creates the file between the existence check and the create.

The file browser shows the latest listing

Listings of one session also run side by side now, and the browser's navigation stays enabled while one loads, so a slow listing could answer after a newer one and move the browser back to a directory the user had left. listDir in desktop/src/stores/sftp.ts publishes entries, path, error and loading state only for the latest listing of the current session. A superseded listing, or one that answers after a disconnect, is ignored. The refresh after an upload goes through listDir too.

Not in this PR

A download cancelled by a close leaves a partial file in place of the original: Leave the original file alone when a download fails, and widen upload writes #142, stacked on this PR.
A cancelled upload leaves a partial remote file: A failed SFTP download truncates the file it was overwriting, and uploads use 8 KiB writes #141.

Tests

core/tests/session_isolation.rs runs both managers against an in-process russh server on loopback. Its SFTP side answers every READDIR with more entries and never with end-of-file, and keeps its files in memory. Its shell exits right away, or stops reading its input.

a_listing_that_never_ends_holds_up_no_other_sftp_session: while the endless listing runs, another session answers, close_all returns within 5 s with both sessions out of reach, and the listing ends with an error.
closing_one_sftp_session_does_not_wait_on_its_listing
a_shell_that_exits_leaves_no_session_behind: the output and close arrive, then the entry and the claim are gone and writes are refused.
a_write_stuck_behind_a_peer_that_stopped_reading_ends_with_the_close: the server stops reading, writes run until one blocks, then close must end that write within 2 s.
closing_a_session_whose_peer_stopped_reading_frees_its_id: after the close, the close is announced and the id can be claimed again.
a_peer_that_recovers_after_the_goodbye_timed_out_is_still_disconnected: the server stops reading for 8 s. After the close the id comes free, and the server sees the connection end once it reads again.
a_resize_never_waits_on_a_peer_that_stopped_reading: 100 resizes on a stalled session each return within 1 s.
a_listing_past_the_entry_cap_is_refused_and_its_handle_closed
of_two_uploads_to_one_path_without_overwrite_only_one_creates_it: slow existence checks let both uploads pass theirs before either creates the file, and exactly one succeeds.
an_overwrite_upload_waits_for_one_already_writing_the_same_path: a 1 MiB overwrite upload is still writing when a 100-byte overwrite to the same path starts. The file has to end up exactly as the 100 bytes.

The first four fail on main. The next five fail on f72acb2, the commit before the teardown changes. The last one fails without the per-path lock. All pass here.

core/src/transport.rs has unit tests showing that a cut ends a pending read and a write blocked on a full buffer, and that ended resolves once the stream is dropped.

desktop/src/stores/sftp.concurrency.test.ts covers the listing guard: an older listing answering last, loading staying on until the latest listing answers, a superseded listing's failure, and an answer after a disconnect. All four fail against the previous store.

Test plan

View with [code]smith
^{Need help on this PR? Tag @codesmith-bot with what you need.}

SessionManager and SftpManager each kept one map lock held across every network await: a keystroke send, a disconnect, a directory listing, and in SFTP's case an entire file transfer. One peer that stopped reading, or a large download, therefore stalled every other session, and close_all could not drain the map. The vault reset gives close_all five seconds and then drops it, so an SFTP session busy in a transfer or an endless listing stayed reachable after the reset that was meant to cut it off. Entries are now Arc'd and looked up under a short lock; the I/O runs after the guard is released. Writes to one SSH session still go out in order through a per-session lock. close and close_all remove entries first and bound the goodbye. SFTP listings get a 30 second overall deadline, since russh-sftp reads until the server says end-of-file with no total limit. The SSH reader also removes its own entry and disconnects when the remote shell exits, instead of leaving the connection and its keepalives up until the pane is closed, and it no longer spins on a closed resize channel while a close is in flight.

greptile-apps · 2026-10-16T22:35:04Z

TREX could not finish testing

~~Plan what to test~~
~~Set up the environment and test the changed flows~~
~~Post test results~~

qodo-code-review · 2026-10-16T22:35:35Z

PR Summary by Qodo

Keep SSH and SFTP session maps unlocked during network I/O

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Release session-map locks before network I/O so stalled peers cannot block other sessions.
• Remove sessions before bounded, parallel close operations; limit SFTP listings to 30 seconds.
• Clean up exited SSH shells and test session isolation against a loopback server.

Diagram

graph TD
  C["Tauri commands"] --> SSH["SSH manager"] --> M[("Session maps")] --> E["Cloned Arc entry"] --> W["Ordered SSH writes"] --> P["Remote peer"]
  C --> SFTP["SFTP manager"] --> M
  E --> P
  M --> D["Bounded close"] --> P

High-Level Assessment

Cloning Arc entries under brief map locks directly addresses cross-session blocking without changing the managers’ public operation model. A dedicated actor per connection could also isolate I/O, but would add message-routing and lifecycle complexity without a clear benefit here.

Files changed (4) +475 / -102

Enhancement (1) +3 / -3

commands.rsCheck SSH session membership without building a list +3/-3

Check SSH session membership without building a list
• Uses SessionManager::contains when routing writes, resizes, and closes, avoiding an allocation and full session-list scan on each command.
desktop/src-tauri/src/commands.rs

Bug fix (2) +181 / -99

session.rsIsolate SSH I/O and clean up exited sessions +104/-38

Isolate SSH I/O and clean up exited sessions

• Stores SSH sessions in Arc entries so writes and resizes do not hold the session-map lock. Adds per-session write ordering, bounded parallel disconnects, a direct contains check, and reader cleanup that respects reused session IDs.

core/src/session.rs

sftp.rsKeep SFTP transfers outside the map lock +77/-61

Keep SFTP transfers outside the map lock

• Runs SFTP operations on cloned connection entries, allowing other sessions and closes to proceed during listings or transfers. Adds a 30-second listing deadline and bounded, parallel closes that shut down in-flight channel requests.

core/src/sftp.rs

Tests (1) +291 / -0

session_isolation.rsTest isolation against an endless SFTP listing +291/-0

Test isolation against an endless SFTP listing

• Adds a loopback SSH/SFTP server whose directory listings never end and whose shell exits immediately. Integration tests verify cross-session progress, prompt close and drain, and cleanup after shell exit.

core/tests/session_isolation.rs

qodo-code-review · 2026-10-16T22:38:29Z

Code Review by Qodo

Taking an entry out of the map stopped new calls from reaching a session, but a call already holding the entry kept it alive: a keystroke send stuck behind a peer that stopped reading, or an SFTP listing or transfer still running, held the connection and the session id until it finished on its own. Each SSH session and SFTP connection now carries a closed signal that close, close_all and the reader's own cleanup flip. Writes, listings and transfers race against it and return an error once it fires. The goodbye sent after a close holds only the connection handle, so it no longer keeps the session id claimed while a stalled peer fails to answer.

Listings and transfers already stopped when their connection closed, but mkdir, remove, rename, canonicalize and stat did not: a request issued just before a close or a vault reset could still complete and report success after teardown began. Every SftpManager operation now runs under the connection's closed signal.

A close timed the goodbye out after 5 s and dropped it. russh stops reading its outbound queue while a channel waits on the peer's window, and the reader's channel kept its session task alive, so a close on a stalled peer left the connection authenticated for good: the id stayed claimed, the close was never announced, and a vault reset left the shell open even after the peer recovered. - connect() wraps the TCP stream in a Transport that can be cut. The shared goodbye queues the disconnect, waits up to 5 s for russh to end the session, then cuts the stream, which ends the session task and every channel on it. SSH and SFTP both close through it. - The SSH reader keeps only the read half and never waits on outbound messages. Window changes go out from their own task, and resize() replaces the pending size instead of queueing on a bounded channel that a stalled peer filled. close() drops the entry before the goodbye. - SFTP listings run on a second, raw SFTP channel: page by page under the per-request timeout, capped at 200k entries / 32 MiB of names, and the directory handle is closed however the listing ends. That replaces the 30 s total cap that refused large directories on slow links. A server that will not open the channel falls back to read_dir. - An upload without overwrite creates the file exclusively, so of two uploads to one path only one creates it now that SFTP calls no longer serialize on the map lock. - One request helper for the single-request SFTP calls; one CloseSignal type for both managers; session_write/session_resize look the id up once. New tests cover a close on a peer that never reads again and on one that recovers after the goodbye timed out, resizes on a stalled peer, the listing cap and handle close, and two racing exclusive uploads. All five fail without this change.

# Conflicts: # core/src/connection.rs

computerbox124 · 2026-11-12T14:16:06Z

@codex review the PR

chatgpt-codex-connector · 2026-11-12T14:16:24Z

Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-13T10:29:57.128127Z	`f729368`	Manual request

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f68bdb419f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

With the map lock gone, SFTP calls on one session run side by side, and two things it used to serialize no longer were. - Two overwrite uploads to the same remote path truncated and wrote the file at once, leaving a mix of both (and zero-filled gaps) while both reported success. Uploads and write_file now take a per-connection lock on their destination path, so the second waits for the first and then replaces a complete file. Locks are dropped from the map once nothing holds or waits on them. - A slow listing could answer after a newer one and move the browser back to a directory the user had left. The store now publishes only the latest listing of the current session; a superseded listing's result, error and loading state are ignored, and the refresh after an upload goes through the same path.

computerbox124 · 2026-11-13T09:47:03Z

@codex review the PR

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ad94fb4ce9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-11-13T09:49:39Z

+            let mut local = open_download_target(local_path, overwrite).await?;
+
+            tokio::io::copy(&mut remote, &mut local).await?;
+            local.flush().await?;


Serialize downloads that write to the same local destination

When two downloads target the same local file, removing the map-wide lock allows both to truncate and write it concurrently. In the inspected desktop flow, SftpBrowser.vue leaves the Download context-menu action enabled during transfers, and sftp_transfer.rs::sftp_download_to_local always passes overwrite = true after the save dialog. A user can therefore start a second download to the same destination while the first is running; both can report success while leaving mixed contents or trailing bytes from the larger transfer. Unlike uploads, downloads have no destination lock. Serialize downloads per local destination across the manager, or write to separate temporary files and atomically publish completed downloads.

Useful? React with 👍 / 👎.

Downloads no longer serialize on the map lock, and the desktop always downloads with overwrite once the save dialog has confirmed it. Two downloads to the same local file, from one session or two, truncated and wrote it together, leaving a mix of both while both reported success. A download now takes a lock on its local destination, held in the manager since the local disk is shared by every session, so the second waits for the first and then replaces a complete file. The key resolves the file's directory, so a path through a symlink or `..` is still the same file. The destination lock is the one uploads use, made generic over its key.

computerbox124 · 2026-11-13T10:26:27Z

@codex review the PR

chatgpt-codex-connector · 2026-11-13T10:29:55Z

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: f72936824a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

computerbox124

@Sadykhzadeh thank you!

#140 landed on main as one squashed commit, while this branch still carried its earlier commits, so session.rs, commands.rs, sftp.rs and session_isolation.rs conflicted. Each takes main's version, which is #140's final state, and this branch's own changes are carried over on top of it. download_to_local keeps main's per-destination lock, so of two downloads to one local file the one started last is left in place, and claims it and opens the remote file under until_closed. The copy then runs through write_download against the connection's CloseSignal, so a close stops the download without cutting its cleanup short. a_download_waits_for_one_already_writing_the_same_local_file watched the target grow, which a staged download never shows until it is complete. It now watches the staging file, checks the first download is still running when the second starts, and that no staging file is left.

qodo-code-review Bot reviewed Oct 16, 2026

View reviewed changes

Comment thread core/src/sftp.rs Resolved

Comment thread core/src/session.rs Resolved

Comment thread core/src/session.rs Resolved

Comment thread core/src/sftp.rs Resolved

Comment thread core/src/session.rs Resolved

qodo-code-review Bot reviewed Oct 16, 2026

View reviewed changes

Comment thread core/src/sftp.rs Resolved

Comment thread core/src/sftp.rs Resolved

This was referenced Oct 16, 2026

Stop reading SSH output the webview has not caught up with #159

Open

Repository hygiene: a test that cannot fail, dead components and imports, stale docs #160

Open

Sadykhzadeh mentioned this pull request Oct 17, 2026

Sign in through keyboard-interactive, explain rejections, and dial each address with its own timeout #177

Open

3 tasks done

computerbox124 added 2 commits November 12, 2026 01:15

Merge remote-tracking branch 'origin/main' into fix/session-lock-scope

f68bdb4

# Conflicts: # core/src/connection.rs

chatgpt-codex-connector Bot reviewed Nov 12, 2026

View reviewed changes

Comment thread core/src/sftp.rs Resolved

Comment thread core/src/sftp.rs Resolved

chatgpt-codex-connector Bot reviewed Nov 13, 2026

View reviewed changes

computerbox124 approved these changes Nov 13, 2026

View reviewed changes

computerbox124 merged commit fa5d973 into main Nov 13, 2026
5 of 7 checks passed

Conversation

Sadykhzadeh commented Oct 16, 2026 • edited by computerbox124 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this changes

Map locks are never held across network I/O

A close always ends the connection

A close ends what is already running

SSH session lifecycle

SFTP listings

Writers to one remote path take turns

The file browser shows the latest listing

Not in this PR

Tests

Test plan

Uh oh!

greptile-apps Bot commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TREX could not finish testing

Uh oh!

qodo-code-review Bot commented Oct 16, 2026

PR Summary by Qodo

Uh oh!

qodo-code-review Bot commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

computerbox124 commented Nov 12, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

computerbox124 commented Nov 13, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Nov 13, 2026

Choose a reason for hiding this comment

Uh oh!

computerbox124 commented Nov 13, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 13, 2026

Uh oh!

computerbox124 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sadykhzadeh commented Oct 16, 2026 •

edited by computerbox124

Loading

greptile-apps Bot commented Oct 16, 2026 •

edited

Loading

qodo-code-review Bot commented Oct 16, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Nov 12, 2026 •

edited

Loading