perf(sftp): honour --parallel up to 16 connections, and say when a provider ceiling binds - #761
perf(sftp): honour --parallel up to 16 connections, and say when a provider ceiling binds#761axpnet wants to merge 1 commit into
Conversation
…ovider ceiling binds The SFTP pool ceiling was a literal 4, so --parallel 16 delivered 4 connections without a word. The comment above it asked for a live benchmark on the target server before raising it; the DAG engine review battery on the Hetzner lab (wired gigabit, 5000 x 4 KiB files, 2026-09-08) is that benchmark, on two binaries. From --parallel 4 to 16 rclone gained 47% on upload (687 s to 367 s) and 73% on download (422 s to 116 s); AeroFTP moved under 1% (1391 s to 1383 s, 1294 s to 1293 s) because the cap silently bound the flag. After the SFTP connection reuse of #735 made every leg four times faster the slope was unchanged: rclone still gained 47% and 75% from the same step, AeroFTP 4% and 3% (331 s to 316 s, 273 s to 264 s). The ceiling is now 16, the same range as --sftp-concurrency; the effective count stays the lower of the ceiling and --parallel, so the default of 4 connections is unchanged. On every provider, put -r and get -r now print a note when the provider ceiling is lower than the requested --parallel (text mode; quiet, JSON and machine stay silent): a flag that accepts 32 and delivers 4 without a word is worse than a lower documented limit. Tests: the SFTP ceiling equals the intra-file stream range; and the door itself, a pool-backed counting provider with the ceiling SftpProvider advertises, driven through the real provider executor and the batch DAG with a rendezvous that only opens when 8 downloads hold 8 sessions at once: --parallel 8 delivers 8, --parallel 32 delivers 16, and with the literal 4 put back the test fails with 4 against 8. Signed-off-by: axpnet <45786925+axpnet@users.noreply.github.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe SFTP transfer pool ceiling increases from 4 to 16 sessions. The CLI reports lower provider ceilings in text mode. Tests verify requested parallelism and ceiling enforcement. ChangesSFTP parallel transfers
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The SFTP concurrency increase and provider-capacity notice preserve the documented limits and output-mode behavior. No actionable merge risk remains. Sequence Diagram(s)sequenceDiagram
participant CLI
participant TransferSettings
participant SFTPProvider
participant ProviderDownloadExecutor
participant StorageSessions
CLI->>TransferSettings: resolve --parallel and provider ceiling
TransferSettings->>SFTPProvider: get maximum sessions
SFTPProvider-->>TransferSettings: return ceiling 16
TransferSettings-->>CLI: return effective concurrency
CLI->>ProviderDownloadExecutor: execute batch
ProviderDownloadExecutor->>StorageSessions: open effective number of sessions
StorageSessions-->>ProviderDownloadExecutor: complete transfers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 35 functions across 2 files. (3 skipped: 2 unsupported, 1 too large.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ast-grep (0.45.2)src-tauri/src/bin/aeroftp_cli.rsast-grep timed out on this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary
Finding 1 of the DAG engine review's before/after battery (test station, Hetzner lab over wired gigabit, 5000 x 4 KiB files). The SFTP transfer pool ceiling was a literal 4, so
--parallel 16delivered 4 connections without a word. The comment above the constant asked for a live benchmark on the target server before raising it; this battery is that benchmark, on two binaries.Measured, from
--parallel 4to--parallel 16on the same cell:#735 made every leg about four times faster (connection reuse), and the slope did not move: rclone keeps gaining from the step, AeroFTP does not, because the cap silently bound the flag. After this change on SFTP the first-pass
syncis already ahead of rclone (313.60 s against 649.91 s) whileget -rat p16 is still behind (264.41 against 109.89): one door still closed, this PR opens it.Change.
SFTP_POOL_MAX_SESSIONS = 16, the same range as--sftp-concurrency; the effective count stays the lower of the ceiling and--parallel, so the default of 4 connections is unchanged. The list pool follows the transfer pool (it reads the same constant), so--checkerson SFTP is bounded by 16 too.put -randget -rprint a note when the provider ceiling is lower than the requested--parallel(text mode only; quiet, JSON and machine output stay silent). A flag that accepts 32 and delivers 4 without a word is worse than a lower documented limit.--parallelrow names each provider's ceiling.Tests
providers::sftp::tests::sftp_pool_ceiling_matches_the_intra_file_stream_range: the constant equals the intra-file stream range.transfer_dag_batch::tests::sftp_pool_delivers_the_requested_parallelism_up_to_the_provider_ceiling: the door, not the guard. A pool-backed counting provider with the ceilingSftpProvideradvertises, driven through the realProviderDownloadExecutorand the batch DAG, resolved the way the CLI and the GUI resolve it, with a rendezvous that only opens when 8 downloads hold 8 sessions at once:--parallel 8delivers 8,--parallel 32delivers 16. With the literal 4 put back the test fails withleft: 4, right: 8after the rendezvous times out (seen, then restored).The step measured from outside (rclone gains, AeroFTP does not) and this test from inside are independent measurements of the same door.
Verified:
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings(rc 0), the two tests above plus the existing SFTP scan-pool test, on the branch rebased onto53ed27ce8.Numbers to expect
The test station re-measures tree-small-p16-labsftp on the squash: expected, AeroFTP gains from p4 to p16 like rclone does; the p4 rows do not move (default unchanged).
Summary by CodeRabbit
New Features
--parallel.Documentation