fix(ssh): map mkdir-on-existing-dir to AlreadyExists so dir-merge copy works - #172
Merged
Conversation
…y works
Copying a directory onto a remote SFTP path that already exists failed with
"Copy failed: …" (the reporter saw "not found"). OpenSSH's sftp-server reports
`mkdir` on an existing path as a generic SSH_FX_FAILURE — there is no dedicated
"already exists" status — and different servers phrase it differently ("Failure"
vs "not found"). The SSH backend surfaced this as an opaque `Backend` error, but
the transfer engine's `copy_tree` only tolerates `VfsError::AlreadyExists`; any
other error aborts the whole copy. So merging into an existing remote directory
(the common case: the destination folder is already there) always failed.
Fix `SftpVfs::create_dir` to disambiguate server-agnostically: on failure, stat
the path — if it's already a directory, return `AlreadyExists` (matching every
other backend, which lets the merge copy proceed); otherwise surface the original
error unchanged (a real failure, or a collision with a file, still propagates).
- Make the `MockSftp` faithful: `mkdir` on an existing path now fails like the
real server (generic SSH_FX_FAILURE), so this is covered hermetically.
- Unit test (mock): create_dir on an existing dir → AlreadyExists; on an existing
file → the real Backend error, not masked.
- Integration test (`tests/copy_repro.rs`, `CAIRN_IT_SFTP=1`): drives the real
OpenSSH sftp-server as the transfer destination and runs the actual
`cairn_transfer::run_transfer` local→SFTP — reproduces the abort before the fix
and passes after (files: 2, dirs: 1). Adds dev-deps cairn-transfer +
cairn-backend-local to exercise the engine and backend together end-to-end.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rts safely Review follow-up (bug-bot): the recovery probe used `stat`, which follows symlinks — so a symlink-to-directory at the destination would be treated as the directory it targets, and the merge copy would descend and write the source's children *through* the link into its target, possibly outside the destination tree. Use `lstat` instead: a symlink classifies as `Symlink` (not `Dir`), so we fall through and surface the original mkdir error and the copy aborts, mirroring the deliberate `lstat` caution already used in `remove`. Only a genuine directory maps to `AlreadyExists`. Adds a regression test for the symlink-dest case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
What
Copying a directory onto a remote SFTP directory that already exists failed with
Copy failed: …(the reporter saw
Copy failed: not found). This fixes it so a directory-merge copy proceeds.Root cause
OpenSSH's
sftp-serverreportsmkdiron an existing path as a genericSSH_FX_FAILURE— theSFTP protocol has no dedicated "already exists" status — and different server builds phrase it
differently (
"Failure"on the OpenSSH box I reproduced against,"not found"on the reporter'sDietPi host). The SSH backend surfaced this as an opaque
VfsError::Backend, but the transferengine's
copy_treeonly toleratesVfsError::AlreadyExists:Since the destination folder almost always already exists (you're copying into it), every such
copy aborted.
Fix
SftpVfs::create_dirnow disambiguates server-agnostically: on a failedmkdir, it stats thepath — if it's already a directory, it returns
AlreadyExists(matching every other backend, so themerge proceeds); otherwise it surfaces the original error unchanged. The extra stat happens only on
the failure path, not the happy path. A collision with an existing file still surfaces the real
error (not masked as
AlreadyExists).Reproduction & testing
tests/copy_repro.rs,CAIRN_IT_SFTP=1): drives the real OpenSSHsftp-serveras the transfer destination and runs the actualcairn_transfer::run_transferlocal→SFTP. Before the fix:
Err(Vfs(Backend { msg: "Failure: Failure" })). After:Ok(TransferOutcome { files: 2, dirs: 1, bytes: 10 }).MockSftpmade faithful (rejectsmkdiron an existing path like the realserver);
create_diron an existing dir →AlreadyExists, on an existing file → the realBackenderror.fmt+clippy --all-targets --all-features -D warningsclean.Adds dev-deps
cairn-transfer+cairn-backend-localto the SSH crate so the engine and backend canbe exercised together against a real server (test-only; no impact on the shipped/lean build).
Follow-up (noted, not in this PR)
map_errinreal.rsclassifies SFTP errors by matching on the stringified message. Matchingrussh-sftp's typed
StatusCodeinstead would be more robust; tracked separately.Review gates
bug-botandcode-reviewrun on the diff.🤖 Generated with Claude Code