fix(lftp): stop overriding Connections settings under FTPS#568
Conversation
configure_lftp() hard-coded a fixed parallelism profile (parallel-files=4, dir-pget=1, root-pget=4, connection-limit=8) whenever protocol=ftps, silently ignoring the user's Connections settings — the Settings UI showed values that were never applied. Empirical throughput testing also showed the forced profile was slower than a single stream on an aggregate-throttled seedbox (more streams = lower throughput). Remove the override so persisted Connections values apply for both protocols; what the UI shows is now what lftp runs. Flip the pair_context test to assert FTPS passes persisted values through instead of clobbering them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR simplifies FTPS protocol handling in ChangesFTPS Protocol Handling Simplification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
configure_lftp()hard-coded a fixed parallelism profile (parallel-files=4,dir-pget=1,root-pget=4,connection-limit=8) wheneverprotocol=ftps, silently ignoring the user's Connections settings — the Settings UI showed values that were never applied under FTPS.This removes the override so the persisted Connections values apply for both protocols. What the UI shows is now what lftp runs.
Why
Empirical throughput testing against a live seedbox (FTPS, ~3.2 GB file, 138 ms RTT) showed the forced profile was not just invisible to the user but actively counterproductive on an aggregate-throttled account:
More streams = lower throughput — the signature of a fixed aggregate cap on the seedbox side, where extra connections only add per-stream overhead. SFTP and FTPS hit the same ~45 MB/s ceiling, so the forced FTPS parallelism profile could never help and the user couldn't tune it away because the UI values weren't being applied.
Changes
controller/pair_context.py— delete theif cfg.protocol == "ftps":branch; parallelism is taken from persisted Connections settings unconditionally. Docstring updated to explain the removal (and warn against re-adding it).tests/unittests/test_controller/test_pair_context.py—test_ftps_profile_overrides_persisted_sftp_tuning→test_ftps_passes_persisted_values_through. Persisted values (3 / 20 / 20 / 0) are deliberately all distinct from the old profile (4 / 4 / 1 / 8), so asserting they reach lftp proves the override is gone.Test plan
test_pair_context.py— 20/20 passruff check— cleanruff format --check— cleanprotocol == "ftps"sites — all are connection/port/scheme setup (controller.py,lftp.py,validate_configport check); none touch parallelism, so nothing else depended on the forced profile.🤖 Generated with Claude Code
Summary by CodeRabbit