fix(android): harden download polling against DB crashes and zombie rows#456
Open
Ayush7614 wants to merge 1 commit into
Open
fix(android): harden download polling against DB crashes and zombie rows#456Ayush7614 wants to merge 1 commit into
Ayush7614 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds zombie-download detection to WorkManager polling, marks interrupted downloads failed with ChangesZombie Download Recovery
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoAndroid: prevent polling crash + fail zombie downloads for retry
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
Context used✅ Tickets:
🎫 [Bug] Downloads in 0.0.97 not working✅ Compliance rules (platform):
25 rules 1. Cancellation swallowed
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+315
to
348
| try { | ||
| val active = withContext(Dispatchers.IO) { | ||
| downloadDao.getAllDownloads().first().filter { | ||
| it.status == DownloadStatus.QUEUED || it.status == DownloadStatus.RUNNING | ||
| } | ||
| } | ||
| for (download in active) { | ||
| if (workObservers.containsKey(download.id)) continue | ||
| val workInfos = withContext(Dispatchers.IO) { | ||
| workManager.getWorkInfosForUniqueWork(WorkerDownload.workName(download.id)).get() | ||
| } | ||
| if (DownloadPolling.isZombieWorkStates(workInfos.map { it.state })) { | ||
| withContext(Dispatchers.IO) { | ||
| downloadDao.updateStatus( | ||
| download.id, | ||
| DownloadStatus.FAILED, | ||
| DownloadReason.DOWNLOAD_INTERRUPTED, | ||
| ) | ||
| } | ||
| DownloadEventBridge.error( | ||
| download.id, | ||
| download.fileName, | ||
| download.modelId, | ||
| DownloadReason.messageFor(DownloadReason.DOWNLOAD_INTERRUPTED) | ||
| ?: "Download was interrupted.", | ||
| DownloadReason.DOWNLOAD_INTERRUPTED, | ||
| ) | ||
| } else { | ||
| registerObserver(download.id, download.fileName, download.modelId) | ||
| } | ||
| } | ||
| } catch (_: Exception) { | ||
| // DB unavailable — polling skipped, no crash | ||
| } |
There was a problem hiding this comment.
1. Cancellation swallowed 🐞 Bug ☼ Reliability
startProgressPolling() catches Exception, which includes CancellationException, so coroutine cancellation won’t propagate normally when the module scope is cancelled (e.g., teardown). This can lead to inconsistent lifecycle behavior (e.g., partially running polling work) instead of clean cancellation.
Agent Prompt
### Issue description
`startProgressPolling()` wraps all polling logic in `try { ... } catch (_: Exception) {}`. In Kotlin coroutines, `CancellationException` is an `Exception`, so this catch will intercept cancellations and prevent normal cancellation propagation semantics.
### Issue Context
The module cancels its `scope` during teardown (`onCatalystInstanceDestroy()`), so `startProgressPolling()` should not swallow cancellation.
### Fix Focus Areas
- android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt[312-349]
### Suggested fix
- Change the catch to capture the exception and rethrow cancellation:
- `catch (e: Exception) { if (e is CancellationException) throw e; ... }`
- (Optional but helpful) Narrow the catch to known DB exceptions if that’s the only intended suppression (e.g., SQLiteException).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+315
to
348
| try { | ||
| val active = withContext(Dispatchers.IO) { | ||
| downloadDao.getAllDownloads().first().filter { | ||
| it.status == DownloadStatus.QUEUED || it.status == DownloadStatus.RUNNING | ||
| } | ||
| } | ||
| for (download in active) { | ||
| if (workObservers.containsKey(download.id)) continue | ||
| val workInfos = withContext(Dispatchers.IO) { | ||
| workManager.getWorkInfosForUniqueWork(WorkerDownload.workName(download.id)).get() | ||
| } | ||
| if (DownloadPolling.isZombieWorkStates(workInfos.map { it.state })) { | ||
| withContext(Dispatchers.IO) { | ||
| downloadDao.updateStatus( | ||
| download.id, | ||
| DownloadStatus.FAILED, | ||
| DownloadReason.DOWNLOAD_INTERRUPTED, | ||
| ) | ||
| } | ||
| DownloadEventBridge.error( | ||
| download.id, | ||
| download.fileName, | ||
| download.modelId, | ||
| DownloadReason.messageFor(DownloadReason.DOWNLOAD_INTERRUPTED) | ||
| ?: "Download was interrupted.", | ||
| DownloadReason.DOWNLOAD_INTERRUPTED, | ||
| ) | ||
| } else { | ||
| registerObserver(download.id, download.fileName, download.modelId) | ||
| } | ||
| } | ||
| } catch (_: Exception) { | ||
| // DB unavailable — polling skipped, no crash | ||
| } |
There was a problem hiding this comment.
2. Polling aborts on error 🐞 Bug ☼ Reliability
A single exception while processing one active row (e.g., during WorkManager work info lookup) exits the entire polling loop, so later active downloads won’t be zombie-checked or have observers registered in that cycle. The empty catch also makes these failures silent, reducing debuggability when polling stops working.
Agent Prompt
### Issue description
`startProgressPolling()` wraps the entire “active downloads” loop in one try/catch. If any one download iteration throws (DB read, WorkManager query, event emission), the coroutine jumps to the catch and skips processing the remaining active downloads.
### Issue Context
This PR’s goal is to *harden* polling and zombie recovery; the current structure can still leave some downloads stuck/unobserved if one problematic row/work query fails.
### Fix Focus Areas
- android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt[312-349]
### Suggested fix
- Keep a guarded try/catch around the initial DB read (if desired), but move exception handling inside the per-download loop so one failure doesn’t prevent processing others.
- Add a warning log when skipping due to exception (at least include downloadId/modelId) while still preventing crashes.
- Ensure cancellation still propagates (rethrow `CancellationException`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
android/app/src/test/java/ai/offgridmobile/download/DownloadManagerModuleTest.kt (1)
310-368: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering a
SUCCEEDEDstate case.Good coverage of terminal-failure/empty combinations vs. active states. Optionally add a case with
WorkInfo.State.SUCCEEDEDalone to lock in that a completed-but-untracked work item isn't misclassified as zombie by the helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/test/java/ai/offgridmobile/download/DownloadManagerModuleTest.kt` around lines 310 - 368, Add a test case in DownloadManagerModuleTest for DownloadPolling.isZombieWorkStates with WorkInfo.State.SUCCEEDED alone to cover the completed-work path. Keep it alongside the existing isZombieWorkStates tests and assert that SUCCEEDED is not treated as zombie, matching the current active-state rejection cases.android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt (1)
315-348: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMove exception handling inside the loop so one bad download doesn't skip the rest.
The
try/catchwraps the entire loop. IfgetWorkInfosForUniqueWork(...).get(),updateStatus, orregisterObserverthrows for one download, the loop aborts and every subsequent active download is silently skipped — those rows never get an observer registered and won't update in the UI until the next poll. The blanketcatch (_: Exception)also swallows the failure with no log, making such partial failures invisible.Consider handling per-download and logging unexpected failures:
♻️ Suggested per-item isolation
try { val active = withContext(Dispatchers.IO) { downloadDao.getAllDownloads().first().filter { it.status == DownloadStatus.QUEUED || it.status == DownloadStatus.RUNNING } } for (download in active) { if (workObservers.containsKey(download.id)) continue - val workInfos = withContext(Dispatchers.IO) { - workManager.getWorkInfosForUniqueWork(WorkerDownload.workName(download.id)).get() - } - if (DownloadPolling.isZombieWorkStates(workInfos.map { it.state })) { - ... - } else { - registerObserver(download.id, download.fileName, download.modelId) - } + try { + val workInfos = withContext(Dispatchers.IO) { + workManager.getWorkInfosForUniqueWork(WorkerDownload.workName(download.id)).get() + } + if (DownloadPolling.isZombieWorkStates(workInfos.map { it.state })) { + // ... mark FAILED + emit error ... + } else { + registerObserver(download.id, download.fileName, download.modelId) + } + } catch (e: Exception) { + Log.w(NAME, "Polling failed for ${download.id}", e) + } } - } catch (_: Exception) { - // DB unavailable — polling skipped, no crash + } catch (e: Exception) { + Log.w(NAME, "Progress polling skipped: DB unavailable", e) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt` around lines 315 - 348, The polling block in DownloadManagerModule’s active-download loop is too broadly wrapped, so one exception from getWorkInfosForUniqueWork, updateStatus, or registerObserver can stop processing the remaining downloads. Move the exception handling inside the for loop so each download is isolated, and keep the outer DB-unavailable guard only around the initial fetch. Also add logging in the per-download catch so failures are visible instead of being silently swallowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt`:
- Around line 315-348: The polling block in DownloadManagerModule’s
active-download loop is too broadly wrapped, so one exception from
getWorkInfosForUniqueWork, updateStatus, or registerObserver can stop processing
the remaining downloads. Move the exception handling inside the for loop so each
download is isolated, and keep the outer DB-unavailable guard only around the
initial fetch. Also add logging in the per-download catch so failures are
visible instead of being silently swallowed.
In
`@android/app/src/test/java/ai/offgridmobile/download/DownloadManagerModuleTest.kt`:
- Around line 310-368: Add a test case in DownloadManagerModuleTest for
DownloadPolling.isZombieWorkStates with WorkInfo.State.SUCCEEDED alone to cover
the completed-work path. Keep it alongside the existing isZombieWorkStates tests
and assert that SUCCEEDED is not treated as zombie, matching the current
active-state rejection cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 73797697-31ce-421f-bd89-bb5e179423a0
📒 Files selected for processing (5)
__tests__/integration/models/downloadZombieRecovery.test.tsandroid/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.ktandroid/app/src/main/java/ai/offgridmobile/download/DownloadPolling.ktandroid/app/src/test/java/ai/offgridmobile/download/DownloadManagerModuleTest.ktdocs/PENDING_FIXES.md
Wrap startProgressPolling in try/catch so a Room/SQLite failure cannot crash the app, and reconcile QUEUED/RUNNING DB rows whose WorkManager tasks were cancelled or dropped after an app update. Mark zombies FAILED with download_interrupted so the Download Manager shows a retry button.
Ayush7614
force-pushed
the
fix/android-download-zombie-polling
branch
from
7fc5fa3 to
b6a2d1a
Compare
|
❌ The last analysis has failed. |
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
startProgressPollingin try/catch so a Room/SQLite migration failure cannot crash the app on launch (documented indocs/PENDING_FIXES.mdFeat/local dream #1).FAILEDwithdownload_interruptedand emit an error event so ghost downloads show a retry button instead of staying stuck forever (Feat/testing e2e cleanup #2).DownloadPolling.isZombieWorkStatesas a pure helper with Android unit tests.Type of Change
Related Issues
Relates to #420 (Android download manager reliability)
Test plan
npx jest __tests__/integration/models/downloadZombieRecovery.test.tspasses locallycd android && ./gradlew :app:testDebugUnitTest --tests "ai.offgridmobile.download.DownloadManagerModuleTest"(requires Android SDK)Summary by CodeRabbit
Summary of Changes
Bug Fixes
Tests
Documentation