Repository navigation
Leave the original file alone when a download fails, and widen upload writes - #142
Open
Sadykhzadeh wants to merge 9 commits into
Open
Sadykhzadeh wants to merge 9 commits into
Sadykhzadeh wants to merge 9 commits into
Conversation
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.
|
PR Summary by QodoPreserve local files on failed SFTP downloads and buffer uploads
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
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.
Sadykhzadeh
force-pushed
the
fix/sftp-transfer-integrity
branch
from
October 16, 2026 23:08
44d28c4 to
66203b9
Compare
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.
… writes An overwrite download truncated the user's file before the first byte arrived, so a dropped connection or a server timeout part way through left a cut-off file where their original had been. Overwrites now go to a temporary file beside the target that is renamed over it once it is complete and synced; a new file is removed again if the transfer fails. Uploads read through a 256 KiB buffer. russh-sftp pipelines up to 16 writes, each the size of the slice it is given, and tokio::io::copy's 8 KiB reads capped that at 128 KiB in flight per round trip.
…anly Closing a session dropped the download future, which could interrupt the local open or the final rename halfway and leave a file behind. The close now stops only the copy, and the download cleans up after itself with ordinary awaits, logging a file it could not remove. The staging file is created owner-only and takes the replaced file's permissions before the rename, so a 0600 file is not left world-readable under a 022 umask. A symbolic link is resolved first, so the file it names is replaced and the link survives. Staging names keep only the start of the target's name, so a name at the length limit still fits, and a download to a name that does not exist yet writes it directly.
Sadykhzadeh
force-pushed
the
fix/sftp-transfer-integrity
branch
from
October 17, 2026 06:43
66203b9 to
c96ad56
Compare
A second download that chose to overwrite could rename its finished file over the path before the first one failed; the first then deleted the second's result by name.
Removal no longer depends on the handle having been opened with delete sharing on Windows.
A download to a new name was written in place, so quitting the app or a crash part way left a truncated file under the final name, and on Windows a failed download could still delete a finished file another download had put at the same path. Every download now goes through a staging file. A download that may not overwrite takes its name through a hard link, which refuses an existing name in the same step, so a failure only ever removes its own staging file. Replacing by rename needs write access only to the directory, so a file marked read-only was silently replaced. The target is now opened for writing first and refused if that fails, and only a regular file is replaced. Set-ID bits are not carried over, and a failed chmod is logged instead of discarding a complete download. An overwrite through a dangling symlink failed with "local file already exists"; links are now followed with read_link, so the file they name is created. A close that lands after the last byte, while the file is being synced, still stops the download before it is put in place, and it reports the same SSH error as every other operation. Local writes go through a 256 KiB buffer, the handle is closed only after tokio's in-flight write finishes, and the directory is synced after the rename. The upload buffer comment claimed russh-sftp sends writes as large as the slice it is given; it caps them at 32 KiB by default, so the buffer keeps packets full (about 512 KiB in flight) rather than 4 MiB.
8 of 12 tasks
Member
|
@codex review the PR |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbcf4a4d87
ℹ️ 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".
Comment on lines
+619
to
+620
| } | ||
| path |
There was a problem hiding this comment.
When an overwrite targets a chain of 41 symbolic links, the loop returns the 41st link rather than rejecting the excessive chain. replaceable then follows the remaining link successfully, but publish renames the staging file over that link itself, destroying an intermediate symlink and leaving the actual destination file unchanged while reporting success. The previous direct open rejected this path with ELOOP. Return an error if the traversal limit is exhausted and the resulting path is still a symlink, rather than passing that unresolved link to the replacement logic.
Useful? React with 👍 / 👎.
#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #141. Stacked on #140, which narrows the SFTP lock this code runs under. Merge #140 first; this PR's base then becomes
main.What this changes
Downloads never leave a partial file
write_downloadnow owns the local side of a download..<name>.<uuid>.partbeside the target. It is flushed,sync_all'd, then renamed over the target. On any error the.partfile is removed and the original is untouched. A rename in the same directory is atomic on every platform the app ships on, so a reader sees either the old file or the complete new one.create_newas before, and removed if the transfer fails.Cancellation is covered too. #140 now ends a transfer as soon as its session closes, by dropping the transfer future. A
PartialDownloadguard removes the file written so far whenever it is dropped before the download is in place, so that path leaves nothing behind either. The.partfile is only left behind if the process itself dies mid-download. It is a dot-file named after the target, so it is easy to recognise.Uploads keep more data in flight
The local file is read through a 256 KiB
BufReaderwithcopy_buf, so eachpoll_writehands russh-sftp a full-size write. With its 16-write pipeline that is about 4× the in-flight datatokio::io::copy's 8 KiB reads allowed, which matters on long links.Tests
New unit tests in
core/src/sftp.rs:an_overwrite_replaces_the_file_only_once_complete: the target holds the new content and no.partis left.a_failed_overwrite_keeps_the_original_and_no_partial_file: a source that fails part way, like a dropped connection, leaves the original content and nothing else in the directory.a_failed_new_download_leaves_nothing_behinda_download_without_overwrite_keeps_an_existing_filea_cancelled_download_leaves_the_original_and_nothing_else: a source that goes quiet is cancelled by a timeout, for both an overwrite and a new file, and only the original file remainsTest plan
cargo test -p clavyn-core: all suites pass (--lib sftp: 7)node scripts/verify/check-comments.mjs: okNeed help on this PR? Tag
@codesmith-botwith what you need.Review follow-up
0600and takes the replaced file's permissions before the rename.an_overwrite_keeps_the_original_permissions,an_overwrite_through_a_link_replaces_the_file_it_names,an_overwrite_works_for_a_name_at_the_length_limit,a_download_stopped_by_a_close_leaves_the_original_and_nothing_else.cargo test -p clavyn-corepasses in full (126 unit tests + all integration suites).