Deduplicate transfer managers into transfer_common and cap finished history#121
Merged
Conversation
Improve UI responsiveness by enqueuing directory download jobs immediately with 0 totals instead of blocking to calculate them first. A background task now walks the remote directory and updates the job totals once complete. Also add warning logging in walk_remote_dir_inner for read_dir errors instead of silently returning. Fixes macnev2013#111
Extract common transfer utilities (ETA calculation, progress tracking, concurrency management) from S3, SCP, and SFTP transfer managers into a new `transfer_common` module to reduce code duplication and improve maintainability. Key improvements: - Add EMA smoothing for speed calculation for more stable readings - Reduce speed window from 2s to 500ms for faster UI responsiveness - Implement ProgressFields trait to standardize progress field access - Centralize ETA calculation and concurrency management logic - Improve S3 multipart upload and range-based download - Fix SFTP progress aggregation to use unbounded channel instead of direct calls
iamnoksio
marked this pull request as draft
July 21, 2026 01:03
Refactor S3, SCP, and SFTP transfer managers to eliminate code duplication by moving shared constants (EMIT_THROTTLE, MAX_FINISHED_HISTORY, SPEED_WINDOW) and the record_finished function to transfer_common. Introduce FinishedStatus trait to abstract terminal status checking, and simplify record_progress signature by using centralized constants instead of parameters.
Contributor
Author
|
Fixed #111 |
iamnoksio
marked this pull request as ready for review
July 23, 2026 06:29
- set_job_status drops the DashMap write-guard before record_finished, which re-enters the map to evict old entries; a same-shard hit would deadlock once history passed the cap - set_job_status treats terminal statuses as final, so a queued-cancel followed by the worker's dequeue (or a cancel racing the InProgress mark) can't emit or record a job twice - updateTransfer skips finished_order ids already present, so repeated terminal events can't burn extra history slots - drop the redundant last_emit stamp after record_progress (it already stamps on emit) and fix the MAX_FINISHED_HOSTORY typo
Rust: record_finished evicts only still-terminal jobs past the cap, tolerates order ids missing from the map, and a watchdog test fails (rather than hangs) if eviction ever re-enters the map while holding a guard. Frontend: duplicate terminal events keep one history slot, trimming never evicts active transfers, and remove/clear release their order slots.
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.
Greptile Summary
This PR extracts duplicated transfer logic (progress tracking, speed/ETA computation, history capping) from the SFTP, SCP, and S3 managers into a new
transfer_commonmodule, and caps the finished-transfer history at 200 entries on both the Rust and TypeScript sides. It also upgrades S3 uploads to multipart and downloads to range-based fetching, makes SFTP directory-download stat walks asynchronous for faster UI appearance, and fixes the previously flagged DashMap re-entrancy deadlock and doublerecord_finishedcall via anis_terminal()guard inset_job_status.transfer_common: new sharedrecord_progress(with EMA speed smoothing,SPEED_WINDOWshortened to 500 ms),record_finishedwith cap + eviction,apply_concurrency,eta_secs, and accompanying unit tests including a deadlock watchdog.set_job_statusnow drops the DashMap shard guard before callingrecord_finished, and early-returns if the job is already terminal — resolving both the shard-lock reëntry deadlock and the duplicate history entry from queued-cancel racing the worker.finished_ordertracks terminal transfer IDs in insertion order;trimFinishedevicts the oldest terminal entry when the 200-entry cap is exceeded; a deduplication guard prevents the same ID from occupying two slots.Confidence Score: 4/5
Safe to merge; all previously flagged deadlock and double-push paths are correctly guarded. One untreated gap: retry() does not remove the transfer ID from finished_order, leaking phantom history slots on repeated retries.
The three transfer managers each have a retry() function that resets a failed/cancelled job to Queued without removing its ID from finished_order. Each subsequent failure re-appends the ID, so N retries produce N−1 stale phantom entries that consume history slots without ever being cleaned up. With enough retries per job the effective history cap silently shrinks. All other logic—deadlock avoidance, terminal-state idempotency, TypeScript deduplication, and the Rust eviction tests—looks correct.
Files Needing Attention: retry() in src-tauri/src/sftp/transfer_manager.rs, src-tauri/src/s3/transfer_manager.rs, and src-tauri/src/scp/transfer_manager.rs each need a finished_order cleanup step.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A([Job enqueued\nQueued]) --> B{cancel_token\ncancelled?} B -- yes, Queued --> C[set status = Cancelled\ndrop shard guard\nemit event] C --> RF1[record_finished\nfinished_order mutex\n→ push + evict] B -- no --> D[set status = InProgress\nworker runs] D --> E{Result} E -- Ok --> F[bytes_transferred = total\nset Completed] E -- TransferCancelled --> G[set Cancelled] E -- Err --> H[set Failed] F & G & H --> SJS[set_job_status\nacquire shard guard] SJS --> IT{is_terminal\nalready?} IT -- yes --> NOOP([return — no duplicate\nrecord_finished]) IT -- no --> UPD[update status/error\ndrop shard guard\nemit event] UPD --> RF2[record_finished\nfinished_order mutex\n→ push + evict] RF2 --> DONE([Done]) RF1 --> RETRY{retry\ncalled?} RETRY -- yes --> RST[reset to Queued\nNOT removed from\nfinished_order ⚠️] RST --> D RETRY -- no --> DONEReviews (3): Last reviewed commit: "test(transfers): pin finished-history in..." | Re-trigger Greptile