fix(core): recover orphaned downloads on startup - #62
Merged
Conversation
On app restart, downloads stuck in Downloading/Waiting/Checking/Extracting are transitioned to Error so the user can retry. Queued/Retry downloads are re-scheduled automatically via on_slot_freed().
📝 WalkthroughWalkthroughOn startup, the app reconciles persisted downloads with missing in-memory tasks: downloads in Changes
Sequence DiagramsequenceDiagram
participant App as App Startup
participant Repo as Download Repository
participant Recovery as Startup Recovery Service
participant QM as Queue Manager
participant Engine as Download Engine
App->>Recovery: recover_orphaned_downloads(repo)
Recovery->>Repo: query downloads in Orphan States\n(Downloading, Waiting, Checking, Extracting)
Repo-->>Recovery: list of orphaned downloads
loop per download
Recovery->>Recovery: transition to Error ("Interrupted: app restarted")
Recovery->>Repo: save(updated_download)
Repo-->>Recovery: persisted
end
Recovery-->>App: return count
App->>QM: queue_manager.clone().start_listening()
QM-->>App: listening started
App->>App: tokio::spawn async task
activate App
App->>Repo: query downloads in Queued/Retry
Repo-->>App: queued/retry list
App->>QM: on_slot_freed().await (trigger re-scheduling)
QM->>Engine: schedule downloads onto engine slots
Engine-->>QM: ack / start tasks
deactivate App
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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)
Comment |
Greptile SummaryThis PR adds a Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Setup as lib.rs setup()
participant Recovery as startup_recovery
participant DB as SQLite (DownloadRepo)
participant QM as QueueManager
participant Engine as DownloadEngine
Setup->>Recovery: recover_orphaned_downloads(repo)
Recovery->>DB: find_by_state(Downloading/Waiting/Checking/Extracting)
DB-->>Recovery: orphaned downloads
Recovery->>Recovery: download.fail("Interrupted: app restarted")
Recovery->>DB: "save(download) — state = Error"
Recovery-->>Setup: Ok(n recovered)
Setup->>QM: QueueManager::new(repo, engine, bus, 4)
Setup->>QM: start_listening()
Note over QM: event loop running (active_count=0)
Setup->>Setup: spawn_tauri_event_bridge / spawn_notification_bridge / etc.
Setup->>QM: tokio::spawn(on_slot_freed())
Note over QM: runs after setup() returns
QM->>DB: find_by_state(Queued) + find_by_state(Retry)
DB-->>QM: surviving downloads
QM->>QM: download.start() — Queued/Retry → Downloading
QM->>DB: save(download)
QM->>Engine: engine.start(download)
QM->>QM: "active_count += 1"
Reviews (1): Last reviewed commit: "fix(core): recover orphaned downloads on..." | Re-trigger Greptile |
Comment on lines
+211
to
+231
| #[test] | ||
| fn test_recover_mixed_states_only_transitions_orphans() { | ||
| let repo = InMemoryRepo::new(vec![ | ||
| make_downloading(1), | ||
| make_completed(2), | ||
| make_waiting(3), | ||
| make_paused(4), | ||
| make_checking(5), | ||
| make_download(6), // Queued | ||
| ]); | ||
|
|
||
| let count = recover_orphaned_downloads(&repo).expect("recovery"); | ||
|
|
||
| assert_eq!(count, 3); // 1 (Downloading), 3 (Waiting), 5 (Checking) | ||
| assert_eq!(repo.get(1).unwrap().state(), DownloadState::Error); | ||
| assert_eq!(repo.get(2).unwrap().state(), DownloadState::Completed); | ||
| assert_eq!(repo.get(3).unwrap().state(), DownloadState::Error); | ||
| assert_eq!(repo.get(4).unwrap().state(), DownloadState::Paused); | ||
| assert_eq!(repo.get(5).unwrap().state(), DownloadState::Error); | ||
| assert_eq!(repo.get(6).unwrap().state(), DownloadState::Queued); | ||
| } |
There was a problem hiding this comment.
Extracting and Retry fixtures
test_recover_mixed_states_only_transitions_orphans covers only 3 of the 4 ORPHAN_STATES in combination, so if Extracting were accidentally removed from ORPHAN_STATES the count assertion (assert_eq!(count, 3)) would still pass. Likewise, test_recover_ignores_completed_paused_error_queued never puts a download in Retry state, so it cannot detect an accidental inclusion of Retry in ORPHAN_STATES.
Adding both cases to the mixed-state fixture makes it a true regression net for the complete set:
Suggested change
| #[test] | |
| fn test_recover_mixed_states_only_transitions_orphans() { | |
| let repo = InMemoryRepo::new(vec![ | |
| make_downloading(1), | |
| make_completed(2), | |
| make_waiting(3), | |
| make_paused(4), | |
| make_checking(5), | |
| make_download(6), // Queued | |
| ]); | |
| let count = recover_orphaned_downloads(&repo).expect("recovery"); | |
| assert_eq!(count, 3); // 1 (Downloading), 3 (Waiting), 5 (Checking) | |
| assert_eq!(repo.get(1).unwrap().state(), DownloadState::Error); | |
| assert_eq!(repo.get(2).unwrap().state(), DownloadState::Completed); | |
| assert_eq!(repo.get(3).unwrap().state(), DownloadState::Error); | |
| assert_eq!(repo.get(4).unwrap().state(), DownloadState::Paused); | |
| assert_eq!(repo.get(5).unwrap().state(), DownloadState::Error); | |
| assert_eq!(repo.get(6).unwrap().state(), DownloadState::Queued); | |
| } | |
| fn test_recover_mixed_states_only_transitions_orphans() { | |
| let repo = InMemoryRepo::new(vec![ | |
| make_downloading(1), | |
| make_completed(2), | |
| make_waiting(3), | |
| make_paused(4), | |
| make_checking(5), | |
| make_download(6), // Queued | |
| make_extracting(7), // Extracting — 4th orphan state | |
| { | |
| let mut d = make_downloading(8); | |
| d.fail("err".into()).expect("Downloading → Error"); | |
| d.retry().expect("Error → Retry"); // Retry — must be preserved | |
| d | |
| }, | |
| ]); | |
| let count = recover_orphaned_downloads(&repo).expect("recovery"); | |
| assert_eq!(count, 4); // 1 (Downloading), 3 (Waiting), 5 (Checking), 7 (Extracting) | |
| assert_eq!(repo.get(1).unwrap().state(), DownloadState::Error); | |
| assert_eq!(repo.get(2).unwrap().state(), DownloadState::Completed); | |
| assert_eq!(repo.get(3).unwrap().state(), DownloadState::Error); | |
| assert_eq!(repo.get(4).unwrap().state(), DownloadState::Paused); | |
| assert_eq!(repo.get(5).unwrap().state(), DownloadState::Error); | |
| assert_eq!(repo.get(6).unwrap().state(), DownloadState::Queued); | |
| assert_eq!(repo.get(7).unwrap().state(), DownloadState::Error); | |
| assert_eq!(repo.get(8).unwrap().state(), DownloadState::Retry); // must NOT be orphaned | |
| } | |
| <a href="https://app.greptile.com/ide/claude-code?prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0APath%3A%20src-tauri%2Fsrc%2Fapplication%2Fservices%2Fstartup_recovery.rs%0ALine%3A%20211-231%0A%0AComment%3A%0A**Mixed-state%20test%20missing%20%60Extracting%60%20and%20%60Retry%60%20fixtures**%0A%0A%60test_recover_mixed_states_only_transitions_orphans%60%20covers%20only%203%20of%20the%204%20%60ORPHAN_STATES%60%20in%20combination%2C%20so%20if%20%60Extracting%60%20were%20accidentally%20removed%20from%20%60ORPHAN_STATES%60%20the%20count%20assertion%20%28%60assert_eq!%28count%2C%203%29%60%29%20would%20still%20pass.%20Likewise%2C%20%60test_recover_ignores_completed_paused_error_queued%60%20never%20puts%20a%20download%20in%20%60Retry%60%20state%2C%20so%20it%20cannot%20detect%20an%20accidental%20inclusion%20of%20%60Retry%60%20in%20%60ORPHAN_STATES%60.%0A%0AAdding%20both%20cases%20to%20the%20mixed-state%20fixture%20makes%20it%20a%20true%20regression%20net%20for%20the%20complete%20set%3A%0A%0A%60%60%60suggestion%0A%20%20%20%20fn%20test_recover_mixed_states_only_transitions_orphans%28%29%20%7B%0A%20%20%20%20%20%20%20%20let%20repo%20%3D%20InMemoryRepo%3A%3Anew%28vec!%5B%0A%20%20%20%20%20%20%20%20%20%20%20%20make_downloading%281%29%2C%0A%20%20%20%20%20%20%20%20%20%20%20%20make_completed%282%29%2C%0A%20%20%20%20%20%20%20%20%20%20%20%20make_waiting%283%29%2C%0A%20%20%20%20%20%20%20%20%20%20%20%20make_paused%284%29%2C%0A%20%20%20%20%20%20%20%20%20%20%20%20make_checking%285%29%2C%0A%20%20%20%20%20%20%20%20%20%20%20%20make_download%286%29%2C%20%20%20%20%2F%2F%20Queued%0A%20%20%20%20%20%20%20%20%20%20%20%20make_extracting%287%29%2C%20%20%2F%2F%20Extracting%20%E2%80%94%204th%20orphan%20state%0A%20%20%20%20%20%20%20%20%20%20%20%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20let%20mut%20d%20%3D%20make_downloading%288%29%3B%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20d.fail%28%22err%22.into%28%29%29.expect%28%22Downloading%20%E2%86%92%20Error%22%29%3B%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20d.retry%28%29.expect%28%22Error%20%E2%86%92%20Retry%22%29%3B%20%20%2F%2F%20Retry%20%E2%80%94%20must%20be%20preserved%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20d%0A%20%20%20%20%20%20%20%20%20%20%20%20%7D%2C%0A%20%20%20%20%20%20%20%20%5D%29%3B%0A%0A%20%20%20%20%20%20%20%20let%20count%20%3D%20recover_orphaned_downloads%28%26repo%29.expect%28%22recovery%22%29%3B%0A%0A%20%20%20%20%20%20%20%20assert_eq!%28count%2C%204%29%3B%20%2F%2F%201%20%28Downloading%29%2C%203%20%28Waiting%29%2C%205%20%28Checking%29%2C%207%20%28Extracting%29%0A%20%20%20%20%20%20%20%20assert_eq!%28repo.get%281%29.unwrap%28%29.state%28%29%2C%20DownloadState%3A%3AError%29%3B%0A%20%20%20%20%20%20%20%20assert_eq!%28repo.get%282%29.unwrap%28%29.state%28%29%2C%20DownloadState%3A%3ACompleted%29%3B%0A%20%20%20%20%20%20%20%20assert_eq!%28repo.get%283%29.unwrap%28%29.state%28%29%2C%20DownloadState%3A%3AError%29%3B%0A%20%20%20%20%20%20%20%20assert_eq!%28repo.get%284%29.unwrap%28%29.state%28%29%2C%20DownloadState%3A%3APaused%29%3B%0A%20%20%20%20%20%20%20%20assert_eq!%28repo.get%285%29.unwrap%28%29.state%28%29%2C%20DownloadState%3A%3AError%29%3B%0A%20%20%20%20%20%20%20%20assert_eq!%28repo.get%286%29.unwrap%28%29.state%28%29%2C%20DownloadState%3A%3AQueued%29%3B%0A%20%20%20%20%20%20%20%20assert_eq!%28repo.get%287%29.unwrap%28%29.state%28%29%2C%20DownloadState%3A%3AError%29%3B%0A%20%20%20%20%20%20%20%20assert_eq!%28repo.get%288%29.unwrap%28%29.state%28%29%2C%20DownloadState%3A%3ARetry%29%3B%20%2F%2F%20must%20NOT%20be%20orphaned%0A%20%20%20%20%7D%0A%0AHow%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20please%20make%20it%20concise.&repo=mpiton%2Fvortex"><picture><source media="(prefers-color-scheme: dark)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/FixInClaudeDark.svg?v=2"><source media="(prefers-color-scheme: light)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/FixInClaude.svg?v=2"><img alt="Fix in Claude Code" src="https://greptile-static-assets.s3.amazonaws.com/badges/FixInClaude.svg?v=2" height="20"></picture></a> |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src-tauri/src/application/services/startup_recovery.rs (1)
13-20: Avoid duplicating orphan-state definitions across modules.Lines 15-20 mirror the transition gate in
Download::fail(). If one side changes later, startup recovery can silently drift. Consider moving this state list to a shared domain helper/constant and reusing it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/services/startup_recovery.rs` around lines 13 - 20, The ORPHAN_STATES constant duplicates the same state list used as the transition gate in Download::fail(), which risks drifting; extract the array into a single shared constant or helper (e.g., a pub const or function in a common domain module like download::shared or domain::download_states) and replace the ORPHAN_STATES definition with a reference to that shared symbol, updating startup_recovery.rs to use the shared constant and ensuring Download::fail() also references the same shared symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src-tauri/src/application/services/startup_recovery.rs`:
- Around line 13-20: The ORPHAN_STATES constant duplicates the same state list
used as the transition gate in Download::fail(), which risks drifting; extract
the array into a single shared constant or helper (e.g., a pub const or function
in a common domain module like download::shared or domain::download_states) and
replace the ORPHAN_STATES definition with a reference to that shared symbol,
updating startup_recovery.rs to use the shared constant and ensuring
Download::fail() also references the same shared symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17160d36-2839-46f6-8db4-7ba00bf25458
📒 Files selected for processing (1)
src-tauri/src/application/services/startup_recovery.rs
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
1 participant
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.
Summary
• Add
startup_recoveryservice that transitions orphaned downloads (Downloading/Waiting/Checking/Extracting) to Error on app boot• Re-schedule Queued/Retry downloads via
on_slot_freed()after QueueManager starts• 7 unit tests covering all orphan states and non-orphan preservation
Fixes #57
Type
fix
Summary by cubic
On startup, the app marks downloads left in Downloading/Waiting/Checking/Extracting as Error so users can retry, and automatically re-schedules any Queued or Retry downloads once
QueueManagerstarts. Fixes #57.Written for commit c813119. Summary will update on new commits.
Summary by CodeRabbit