Skip to content

Commit e31dc22

Browse files
axpnetaeroftp[bot]claude
committed
feat(transfer): SFTP connection pool for parallel file transfers (PD-SFTP-1)
SFTP folder/batch transfers in the shared engine were serialised by the single locked provider session. Give SFTP the same pooled, file-level parallelism FTP already has, instead of a separate ad-hoc mechanism. SftpProvider now retains a secure SftpConnectionSpec (password and key passphrase kept in SecretString, captured at connect() before the outer config is zeroized: the exact posture the FTP manager already ships). clone_for_transfer() returns an independent, not-yet-connected worker carrying only that spec; the first transfer dials its own SSH connection (separate socket and auth), so N files run over N independent connections with no SSH handle or channel shared. Host-key safety on the re-dial: pooled dials never trust unknown hosts (the key is already in known_hosts from the first connect, a changed key is rejected) and the freshly accepted fingerprint is verified against the one pinned at the first connect, aborting the worker on mismatch. Wired through the existing shared clone-pool executor dispatch (the same path S3 and Azure use) under a distinct SftpConnectionPool executor kind and an Sftp session-lease label, so capacity and metrics never mislabel the transport. The capability only flips to pool-backed once the secure spec exists; without it SFTP stays a single locked lease (no overclaim, non-regression). An earlier attempt multiplexed channels over one SSH session; it was reworked because the shared engine must use one consistent model, not a third parallelism path. Live-validated against the repo SFTP Docker fixture (reproducible, no secrets): 8 x 16 MiB, byte-identical SHA-256 at concurrency 1/3/5 with a real ~2.9x speedup, proving an actual pool rather than false parallelism. New tests/integration_sftp_pool.rs (ignored by default). Co-Authored-By: aeroftp[bot] <aeroftp[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent fcac0e5 commit e31dc22

5 files changed

Lines changed: 458 additions & 15 deletions

File tree

src-tauri/src/provider_transfer_executor.rs

Lines changed: 53 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,13 @@ pub enum ProviderExecutorSessionModel {
3232
provider_type: ProviderType,
3333
max_leases: usize,
3434
},
35+
/// SFTP: N independent SSH connections re-dialled by clone workers
36+
/// (PD-SFTP-1). Same clone-worker dispatch as `HttpClonePool`; the
37+
/// session pool is labelled `Sftp`, never `HttpClone`.
38+
SftpConnectionPool {
39+
provider_type: ProviderType,
40+
max_leases: usize,
41+
},
3542
}
3643

3744
impl ProviderExecutorSessionModel {
@@ -45,11 +52,17 @@ impl ProviderExecutorSessionModel {
4552
Self::HttpClonePool { max_leases, .. } => {
4653
TransferSessionPoolHandle::http_clone(label, *max_leases)
4754
}
55+
Self::SftpConnectionPool { max_leases, .. } => {
56+
TransferSessionPoolHandle::sftp_connection(label, *max_leases)
57+
}
4858
}
4959
}
5060

5161
fn is_clone_pool(&self) -> bool {
52-
matches!(self, Self::HttpClonePool { .. })
62+
matches!(
63+
self,
64+
Self::HttpClonePool { .. } | Self::SftpConnectionPool { .. }
65+
)
5366
}
5467
}
5568

@@ -101,23 +114,37 @@ pub async fn resolve_provider_executor_session_model(
101114

102115
let provider_type = provider.provider_type();
103116
let caps = provider.transfer_capabilities();
104-
let executor_can_clone =
105-
provider.transfer_executor_kind() == ProviderTransferExecutorKind::HttpClonePool;
117+
let executor_kind = provider.transfer_executor_kind();
106118
let scheduler_can_parallelize = caps.file_parallel == Capability::Supported
107119
&& caps.session_pool == Capability::Supported
108120
&& provider.clone_for_transfer().is_ok();
109121

110-
if executor_can_clone && scheduler_can_parallelize {
111-
let advertised = caps
112-
.max_file_slots
113-
.unwrap_or_else(|| provider.transfer_executor_max_sessions())
114-
.max(1) as usize;
115-
ProviderExecutorSessionModel::HttpClonePool {
116-
provider_type,
117-
max_leases: advertised.min(max_concurrent.max(1)).max(1),
122+
if !scheduler_can_parallelize {
123+
return ProviderExecutorSessionModel::locked(Some(provider_type));
124+
}
125+
126+
let max_leases = caps
127+
.max_file_slots
128+
.unwrap_or_else(|| provider.transfer_executor_max_sessions())
129+
.max(1) as usize;
130+
let max_leases = max_leases.min(max_concurrent.max(1)).max(1);
131+
132+
match executor_kind {
133+
ProviderTransferExecutorKind::HttpClonePool => {
134+
ProviderExecutorSessionModel::HttpClonePool {
135+
provider_type,
136+
max_leases,
137+
}
138+
}
139+
ProviderTransferExecutorKind::SftpConnectionPool => {
140+
ProviderExecutorSessionModel::SftpConnectionPool {
141+
provider_type,
142+
max_leases,
143+
}
144+
}
145+
ProviderTransferExecutorKind::LockedSingle => {
146+
ProviderExecutorSessionModel::locked(Some(provider_type))
118147
}
119-
} else {
120-
ProviderExecutorSessionModel::locked(Some(provider_type))
121148
}
122149
}
123150

@@ -822,6 +849,19 @@ mod tests {
822849
assert_eq!(pool.capacity().max_leases, 4);
823850
}
824851

852+
#[test]
853+
fn sftp_connection_model_uses_sftp_lease_kind_not_http() {
854+
let model = ProviderExecutorSessionModel::SftpConnectionPool {
855+
provider_type: ProviderType::Sftp,
856+
max_leases: 4,
857+
};
858+
let pool = model.session_pool("provider-test");
859+
860+
assert!(model.is_clone_pool());
861+
assert_eq!(pool.capacity().kind, SessionLeaseKind::Sftp);
862+
assert_eq!(pool.capacity().max_leases, 4);
863+
}
864+
825865
#[test]
826866
fn locked_list_model_uses_single_legacy_lease() {
827867
let model = ProviderListSessionModel::locked(Some(ProviderType::WebDav));

src-tauri/src/providers/mod.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,11 @@ impl Default for TransferOptimizationHints {
302302
pub enum ProviderTransferExecutorKind {
303303
LockedSingle,
304304
HttpClonePool,
305+
/// SFTP file-level parallelism via N **independent** SSH connections
306+
/// re-dialled from a retained secure connection spec (PD-SFTP-1, same
307+
/// model as the FTP pool). Distinct from `HttpClonePool` so the
308+
/// session pool / metrics label the transport as SFTP, never HTTP.
309+
SftpConnectionPool,
305310
}
306311

307312
/// Execution model the Core DAG scanner may use for remote list/checker work.
@@ -795,7 +800,11 @@ pub trait StorageProvider: Send + Sync {
795800
self.supports_server_copy(),
796801
);
797802

798-
if self.transfer_executor_kind() == ProviderTransferExecutorKind::HttpClonePool {
803+
if matches!(
804+
self.transfer_executor_kind(),
805+
ProviderTransferExecutorKind::HttpClonePool
806+
| ProviderTransferExecutorKind::SftpConnectionPool
807+
) {
799808
caps.file_parallel = crate::transfer_dag::Capability::Supported;
800809
caps.session_pool = crate::transfer_dag::Capability::Supported;
801810
caps.max_file_slots = Some(self.transfer_executor_max_sessions().max(1));

src-tauri/src/providers/sftp.rs

Lines changed: 166 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,10 @@
99
// Copyright (c) 2024-2026 axpnet: AI-assisted (see AI-TRANSPARENCY.md)
1010

1111
use super::types::is_session_closed_error_message;
12-
use super::{ProviderError, ProviderType, RemoteEntry, SftpConfig, StorageProvider};
12+
use super::{
13+
ProviderError, ProviderTransferExecutorKind, ProviderType, RemoteEntry, SftpConfig,
14+
StorageProvider,
15+
};
1316
use crate::ssh_exec::ssh_exec_collect;
1417
use async_trait::async_trait;
1518
use russh::client::AuthResult;
@@ -194,6 +197,51 @@ impl Handler for SshHandler {
194197
}
195198
}
196199

200+
/// Secure connection spec retained after `connect()` so the shared
201+
/// transfer engine can re-dial N **independent** SSH+SFTP connections for
202+
/// file-level parallelism (PD-SFTP-1).
203+
///
204+
/// This mirrors `FtpConnectionSpec` / `FtpManager::connection_spec()`:
205+
/// `provider_connect` zeroizes the outer config password after the first
206+
/// connect, so the provider must retain its own `SecretString` copy.
207+
/// Holding credentials for the provider's lifetime is the exact security
208+
/// posture FTP already ships. Secrets are only exposed (`ExposeSecret`)
209+
/// at dial time, never on IPC or in logs.
210+
#[derive(Clone)]
211+
pub struct SftpConnectionSpec {
212+
pub host: String,
213+
pub port: u16,
214+
pub username: String,
215+
pub password: Option<secrecy::SecretString>,
216+
pub private_key_path: Option<String>,
217+
pub key_passphrase: Option<secrecy::SecretString>,
218+
pub initial_path: Option<String>,
219+
pub timeout_secs: u64,
220+
/// SHA-256 hex of the host key accepted by the first connect. Pool
221+
/// re-dials verify the new connection's key against this (defense in
222+
/// depth on top of `known_hosts`), same posture as the U-02 rsync pin.
223+
pub pinned_host_key_sha256: Option<String>,
224+
}
225+
226+
impl SftpConnectionSpec {
227+
/// Rebuild an `SftpConfig` for an independent worker. `trust_unknown_hosts`
228+
/// is forced to `false`: a pooled re-dial must never TOFU, the host key
229+
/// is already in `known_hosts` from the first connect.
230+
fn to_config(&self) -> SftpConfig {
231+
SftpConfig {
232+
host: self.host.clone(),
233+
port: self.port,
234+
username: self.username.clone(),
235+
password: self.password.clone(),
236+
private_key_path: self.private_key_path.clone(),
237+
key_passphrase: self.key_passphrase.clone(),
238+
initial_path: self.initial_path.clone(),
239+
timeout_secs: self.timeout_secs,
240+
trust_unknown_hosts: false,
241+
}
242+
}
243+
}
244+
197245
/// SFTP Provider
198246
///
199247
/// Provides secure file transfer over SSH using the SFTP protocol.
@@ -221,6 +269,12 @@ pub struct SftpProvider {
221269
/// SSH connection (U-02) so the fresh TCP socket it opens for
222270
/// `aerorsync_serve` does not skip host-key verification.
223271
host_key_sha256_hex: Arc<std::sync::OnceLock<String>>,
272+
/// Secure connection spec for re-dialling independent pool sessions.
273+
/// `Some` once captured: either at the end of a successful `connect()`
274+
/// (before `provider_connect` zeroizes the outer config) or when this
275+
/// provider was produced by `clone_for_transfer()` as a not-yet-
276+
/// connected pool worker.
277+
connection_spec: Option<SftpConnectionSpec>,
224278
}
225279

226280
impl SftpProvider {
@@ -241,6 +295,7 @@ impl SftpProvider {
241295
// in practice. Override per-call with --chunk-size / --buffer-size.
242296
buffer_size: 256 * 1024,
243297
host_key_sha256_hex: Arc::new(std::sync::OnceLock::new()),
298+
connection_spec: None,
244299
}
245300
}
246301

@@ -255,6 +310,52 @@ impl SftpProvider {
255310
self.host_key_sha256_hex.get().cloned()
256311
}
257312

313+
/// Secure connection spec retained after a successful `connect()`.
314+
/// Mirrors `FtpManager::connection_spec()`. `None` until connected (or
315+
/// until set by `clone_for_transfer()` on a pool worker).
316+
pub fn connection_spec(&self) -> Option<SftpConnectionSpec> {
317+
self.connection_spec.clone()
318+
}
319+
320+
/// Ensure this provider has its own independent, authenticated SSH+SFTP
321+
/// connection (PD-SFTP-1). A `clone_for_transfer()` worker starts
322+
/// unconnected and carries only the secure spec; the first transfer
323+
/// dials a **separate** SSH connection (separate TCP socket, separate
324+
/// auth) so N files run truly in parallel, exactly like the FTP pool.
325+
///
326+
/// Host-key safety on the re-dial: `connect()` still goes through
327+
/// `SshHandler` -> `known_hosts` with `trust_unknown_hosts = false`
328+
/// (never TOFU on a pooled dial; the key is already known from the
329+
/// first connect, `KeyChanged` is rejected). Defense in depth: the
330+
/// freshly accepted fingerprint is compared against the pin captured
331+
/// at the first connect and a mismatch aborts the worker.
332+
async fn ensure_connected(&mut self) -> Result<(), ProviderError> {
333+
if self.sftp.is_some() {
334+
return Ok(());
335+
}
336+
let spec = self
337+
.connection_spec
338+
.clone()
339+
.ok_or(ProviderError::NotConnected)?;
340+
self.config = spec.to_config();
341+
// Fresh per-connection slot so the comparison reflects this dial.
342+
self.host_key_sha256_hex = Arc::new(std::sync::OnceLock::new());
343+
self.connect().await?;
344+
if let Some(pinned) = spec.pinned_host_key_sha256.as_deref() {
345+
match self.accepted_host_key_sha256_hex().as_deref() {
346+
Some(seen) if seen == pinned => {}
347+
other => {
348+
let _ = self.disconnect().await;
349+
return Err(ProviderError::ConnectionFailed(format!(
350+
"SFTP pool re-dial host key mismatch (expected {}, got {:?}): aborting worker",
351+
pinned, other
352+
)));
353+
}
354+
}
355+
}
356+
Ok(())
357+
}
358+
258359
/// Return a cloneable handle to the underlying SSH session, if connected.
259360
///
260361
/// Exposed to let sibling modules (rsync-over-SSH, port forwarding, ...)
@@ -807,6 +908,27 @@ impl StorageProvider for SftpProvider {
807908
self.ssh_handle = Some(Arc::new(TokioMutex::new(handle)));
808909
self.sftp = Some(sftp);
809910

911+
// PD-SFTP-1: capture a secure connection spec now, while
912+
// `self.config` still holds the secrets (`provider_connect`
913+
// zeroizes the outer config only after this returns). The pinned
914+
// host-key fingerprint was populated by `SshHandler` during the
915+
// handshake; preserve an earlier pin if this dial reused one.
916+
let prior_pin = self
917+
.connection_spec
918+
.as_ref()
919+
.and_then(|s| s.pinned_host_key_sha256.clone());
920+
self.connection_spec = Some(SftpConnectionSpec {
921+
host: self.config.host.clone(),
922+
port: self.config.port,
923+
username: self.config.username.clone(),
924+
password: self.config.password.clone(),
925+
private_key_path: self.config.private_key_path.clone(),
926+
key_passphrase: self.config.key_passphrase.clone(),
927+
initial_path: self.config.initial_path.clone(),
928+
timeout_secs: self.config.timeout_secs,
929+
pinned_host_key_sha256: self.host_key_sha256_hex.get().cloned().or(prior_pin),
930+
});
931+
810932
tracing::info!(
811933
"SFTP: Connected successfully to {} (home: {})",
812934
self.config.host,
@@ -984,6 +1106,7 @@ impl StorageProvider for SftpProvider {
9841106
local_path: &str,
9851107
on_progress: Option<Box<dyn Fn(u64, u64) + Send>>,
9861108
) -> Result<(), ProviderError> {
1109+
self.ensure_connected().await?;
9871110
let sftp = self.get_sftp()?;
9881111
let full_path = self.normalize_path(remote_path);
9891112

@@ -1100,6 +1223,7 @@ impl StorageProvider for SftpProvider {
11001223
) -> Result<(), ProviderError> {
11011224
use tokio::io::AsyncWriteExt;
11021225

1226+
self.ensure_connected().await?;
11031227
let sftp = self.get_sftp()?;
11041228
let full_path = self.normalize_path(remote_path);
11051229

@@ -1581,6 +1705,47 @@ impl StorageProvider for SftpProvider {
15811705
}
15821706
}
15831707

1708+
/// PD-SFTP-1: advertise real file-level parallelism only once a secure
1709+
/// connection spec exists to re-dial independent SSH connections from.
1710+
/// Without it (never connected, or credentials unavailable) SFTP stays
1711+
/// a single locked lease: honest non-regression, no overclaim.
1712+
fn transfer_executor_kind(&self) -> ProviderTransferExecutorKind {
1713+
if self.connection_spec.is_some() {
1714+
ProviderTransferExecutorKind::SftpConnectionPool
1715+
} else {
1716+
ProviderTransferExecutorKind::LockedSingle
1717+
}
1718+
}
1719+
1720+
/// Conservative initial cap, mirroring the FTP pool clamp (1..8).
1721+
/// Each lease is a full independent SSH connection; raise only after a
1722+
/// live benchmark on the target server says it pays.
1723+
fn transfer_executor_max_sessions(&self) -> u16 {
1724+
4
1725+
}
1726+
1727+
/// Produce an independent transfer worker. It is **not connected**:
1728+
/// it carries only the secure `SftpConnectionSpec` and dials its own
1729+
/// separate SSH connection lazily on the first transfer
1730+
/// (`ensure_connected`). No SSH handle or channel is shared, so N
1731+
/// workers are N independent connections, exactly like the FTP pool.
1732+
fn clone_for_transfer(&self) -> Result<Box<dyn StorageProvider>, ProviderError> {
1733+
let spec = self.connection_spec.clone().ok_or_else(|| {
1734+
ProviderError::NotSupported(
1735+
"SFTP clone_for_transfer requires a captured connection spec".to_string(),
1736+
)
1737+
})?;
1738+
let mut worker = SftpProvider::new(spec.to_config());
1739+
worker.current_dir = self.current_dir.clone();
1740+
worker.home_dir = self.home_dir.clone();
1741+
worker.download_limit_bps = self.download_limit_bps;
1742+
worker.upload_limit_bps = self.upload_limit_bps;
1743+
worker.compression_enabled = self.compression_enabled;
1744+
worker.buffer_size = self.buffer_size;
1745+
worker.connection_spec = Some(spec);
1746+
Ok(Box::new(worker))
1747+
}
1748+
15841749
fn set_chunk_sizes(&mut self, upload: Option<u64>, download: Option<u64>) {
15851750
// Cap at 16 MB (larger buffers waste memory without improving throughput)
15861751
let cap = 16 * 1024 * 1024;

src-tauri/src/transfer_dag/session_pool.rs

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,21 @@ impl TransferSessionPoolHandle {
170170
Self::new(FtpSessionPoolAdapter::new(label, pool))
171171
}
172172

173+
/// Concurrency gate for SFTP file-level parallelism (PD-SFTP-1).
174+
///
175+
/// The real resource is N **independent** SSH connections re-dialled by
176+
/// `clone_for_transfer()` workers from a retained secure connection
177+
/// spec (same model the FTP pool uses). This handle only bounds how
178+
/// many run at once. Labelled `Sftp`, never `HttpClone`, so capacity
179+
/// and metrics tell the truth about the transport.
180+
pub fn sftp_connection(label: impl Into<String>, capacity: usize) -> Self {
181+
Self::new(SemaphoreSessionPool::with_capacity(
182+
SessionLeaseKind::Sftp,
183+
label,
184+
capacity,
185+
))
186+
}
187+
173188
pub fn executor_managed_ftp(label: impl Into<String>, capacity: usize) -> Self {
174189
Self::new(ExecutorManagedSessionPool::new(
175190
SessionLeaseKind::Ftp,
@@ -489,6 +504,17 @@ mod tests {
489504
assert_eq!(lease.info().label, "s3-list");
490505
}
491506

507+
#[tokio::test]
508+
async fn sftp_connection_pool_reports_sftp_kind_not_http() {
509+
let pool = TransferSessionPoolHandle::sftp_connection("axpbuntu-remote", 4);
510+
let lease = pool.acquire().await.unwrap();
511+
512+
assert_eq!(pool.capacity().kind, SessionLeaseKind::Sftp);
513+
assert_eq!(pool.capacity().max_leases, 4);
514+
assert_eq!(lease.kind(), SessionLeaseKind::Sftp);
515+
assert_eq!(lease.info().label, "axpbuntu-remote");
516+
}
517+
492518
#[tokio::test]
493519
async fn executor_managed_ftp_lease_preserves_capacity_without_double_acquire() {
494520
let pool = TransferSessionPoolHandle::executor_managed_ftp("ftp-batch", 3);

0 commit comments

Comments
 (0)