Skip to content

fix(android): harden download polling against DB crashes and zombie rows #456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Ayush7614 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(android): harden download polling against DB crashes and zombie rows #456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions __tests__/integration/models/downloadZombieRecovery.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
const mockBackgroundDownloadService = {
isAvailable: jest.fn(),
getActiveDownloads: jest.fn(),
};

jest.mock('../../../src/services/backgroundDownloadService', () => ({
backgroundDownloadService: mockBackgroundDownloadService,
}));

const { hydrateDownloadStore } = require('../../../src/services/downloadHydration');
const { useDownloadStore } = require('../../../src/stores/downloadStore');
const { isRetryable } = require('../../../src/utils/downloadErrors');

describe('download zombie recovery integration', () => {
beforeEach(() => {
jest.clearAllMocks();
useDownloadStore.setState({
downloads: {},
downloadIdIndex: {},
repairingVisionIds: {},
});
mockBackgroundDownloadService.isAvailable.mockReturnValue(true);
});

it('hydrates interrupted downloads as failed with a retryable reason code', async () => {
mockBackgroundDownloadService.getActiveDownloads.mockResolvedValue([
{
downloadId: 'dl-zombie-1',
modelId: 'qwen3-4b',
modelKey: 'qwen3-4b:Q4_K_M.gguf',
fileName: 'Q4_K_M.gguf',
modelType: 'text',
status: 'failed',
bytesDownloaded: 0,
totalBytes: 2_500_000_000,
reason: 'The download was interrupted.',
reasonCode: 'download_interrupted',
createdAt: 1_700_000_000_000,
},
]);

await hydrateDownloadStore();

const entry = useDownloadStore.getState().downloads['qwen3-4b:Q4_K_M.gguf'];
expect(entry).toBeDefined();
expect(entry.status).toBe('failed');
expect(entry.errorCode).toBe('download_interrupted');
expect(entry.errorMessage).toBe('The download was interrupted.');
expect(isRetryable(entry.errorCode)).toBe(true);
});

it('does not drop failed rows so the Download Manager can show retry', async () => {
mockBackgroundDownloadService.getActiveDownloads.mockResolvedValue([
{
downloadId: 'dl-zombie-2',
modelId: 'whisper-base',
modelKey: 'whisper-base:ggml-base.en.bin',
fileName: 'ggml-base.en.bin',
modelType: 'stt',
status: 'failed',
bytesDownloaded: 32,
totalBytes: 148_000_000,
reason: 'The download was interrupted.',
reasonCode: 'download_interrupted',
createdAt: 1_700_000_000_100,
},
]);

await hydrateDownloadStore();

expect(Object.keys(useDownloadStore.getState().downloads)).toHaveLength(1);
expect(useDownloadStore.getState().downloadIdIndex['dl-zombie-2']).toBe(
'whisper-base:ggml-base.en.bin',
);
});
});
36 changes: 32 additions & 4 deletions android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,12 +312,40 @@ class DownloadManagerModule(reactContext: ReactApplicationContext) :
@ReactMethod
fun startProgressPolling() {
scope.launch {
val active = withContext(Dispatchers.IO) {
downloadDao.getAllDownloads().first().filter {
it.status == DownloadStatus.QUEUED || it.status == DownloadStatus.RUNNING
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
}
Comment on lines +315 to 348

@qodo-code-review qodo-code-review Bot Jul 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

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

@qodo-code-review qodo-code-review Bot Jul 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

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

active.forEach { if (!workObservers.containsKey(it.id)) registerObserver(it.id, it.fileName, it.modelId) }
}
}

Expand Down
14 changes: 14 additions & 0 deletions android/app/src/main/java/ai/offgridmobile/download/DownloadPolling.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package ai.offgridmobile.download

import androidx.work.WorkInfo

/**
* Pure helpers for [DownloadManagerModule.startProgressPolling].
* Extracted so zombie-detection rules are unit-testable without Room/WorkManager.
*/
internal object DownloadPolling {
fun isZombieWorkStates(states: List<WorkInfo.State>): Boolean =
states.isEmpty() || states.all {
it == WorkInfo.State.CANCELLED || it == WorkInfo.State.FAILED
}
}
71 changes: 71 additions & 0 deletions android/app/src/test/java/ai/offgridmobile/download/DownloadManagerModuleTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package ai.offgridmobile.download

import android.app.Application
import androidx.work.WorkInfo
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNotNull
Expand Down Expand Up @@ -303,5 +304,75 @@ class DownloadManagerModuleTest {
as android.app.NotificationManager
assertNotNull(mgr.getNotificationChannel("model_downloads"))
}

// ── DownloadPolling.isZombieWorkStates ────────────────────────────────────

@Test
fun isZombieWorkStatesTreatsEmptyListAsZombie() {
assertTrue(DownloadPolling.isZombieWorkStates(emptyList()))
}

@Test
fun isZombieWorkStatesTreatsAllCancelledAsZombie() {
assertTrue(
DownloadPolling.isZombieWorkStates(
listOf(WorkInfo.State.CANCELLED, WorkInfo.State.CANCELLED),
),
)
}

@Test
fun isZombieWorkStatesTreatsAllFailedAsZombie() {
assertTrue(
DownloadPolling.isZombieWorkStates(
listOf(WorkInfo.State.FAILED),
),
)
}

@Test
fun isZombieWorkStatesTreatsMixedCancelledAndFailedAsZombie() {
assertTrue(
DownloadPolling.isZombieWorkStates(
listOf(WorkInfo.State.CANCELLED, WorkInfo.State.FAILED),
),
)
}

@Test
fun isZombieWorkStatesRejectsRunningWork() {
assertFalse(
DownloadPolling.isZombieWorkStates(
listOf(WorkInfo.State.RUNNING),
),
)
}

@Test
fun isZombieWorkStatesRejectsEnqueuedWork() {
assertFalse(
DownloadPolling.isZombieWorkStates(
listOf(WorkInfo.State.ENQUEUED),
),
)
}

@Test
fun isZombieWorkStatesRejectsCancelledPlusRunning() {
assertFalse(
DownloadPolling.isZombieWorkStates(
listOf(WorkInfo.State.CANCELLED, WorkInfo.State.RUNNING),
),
)
}

@Test
fun downloadInterruptedReasonIsRetryable() {
assertTrue(DownloadReason.isRetryable(DownloadReason.DOWNLOAD_INTERRUPTED))
assertEquals(
"The download was interrupted.",
DownloadReason.messageFor(DownloadReason.DOWNLOAD_INTERRUPTED),
)
}
}

6 changes: 4 additions & 2 deletions docs/PENDING_FIXES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ Issues identified but not yet applied to code. Address before or alongside the n

---

## 1. SQLiteException not caught in `startProgressPolling`
## ~~1. SQLiteException not caught in `startProgressPolling`~~ ✅ Fixed in `fix/android-download-zombie-polling`


**File:** `android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt`

Expand Down Expand Up @@ -36,7 +37,8 @@ fun startProgressPolling() {

---

## 2. Zombie download entries after app update
## ~~2. Zombie download entries after app update~~ ✅ Fixed in `fix/android-download-zombie-polling`


**File:** `android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt`

Expand Down