review(persistence): unverified triage findings from 2026-07-09 whole-repo review (10 medium, 15 low) #500

Open
opened 2026-07-10 19:15:31 +00:00 by JMR-dev · 0 comments
JMR-dev commented 2026-07-10 19:15:31 +00:00 (Migrated from github.com)

Source: whole-repo multi-agent review, 2026-07-09 (run wf_b41de68c-e85). The run was cut short by usage limits before its verification pass, so every finding below is an unverified finder candidate — validate each against the current code before implementing. Findings are listed medium first, then low. Critical/high candidates from the same run were verified separately and have their own issues.

Medium (10)

app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt:70 — migrateIfNeeded() — the done-flag gating, the encrypted-source passphrase branch, and the missing-cache skip — has zero test coverage: AccountDataMigratorTest exercises only the static copyAccountTables core, and every other test (DatabaseProvisionerTest, DatabaseProvisionerInstrumentedTest, DatabaseModuleInstrumentedTest, AccountDatabaseModuleInstrumentedTest) stubs the whole class with mockk.

  • severity: medium · category: test-gap · angle: tests

A regression in the orchestration — e.g. markDone() moved before the copy (or reached after a swallowed copy failure), the isDone() early-return inverted, or the DatabaseEncryption.isEncrypted(cacheFile) branch resolving the wrong passphrase source — passes the entire suite green. On an upgrading pre-#111 install the copy is then skipped or fails silently, migrateIfNeeded marks done, Room opens the cache and MIGRATION_15_16 drops accounts/credentials/settings/signatures: every upgrading user is permanently signed out with their credentials destroyed, and no test in the repo can catch it because none ever runs the real migrateIfNeeded against a real DataStore done-flag.

app/src/main/kotlin/org/libremail/data/local/AccountMigrations.kt:25 — The AccountDatabase migration chain lacks the chain-completeness and DI-registration parity guards that MigrationTest gives the cache database (issue #312): AccountMigrationTest only replays 1->2, and nothing asserts AccountDatabaseModule's addMigrations(ACCOUNT_MIGRATION_1_2) registers every declared ACCOUNT_ migration up to the newest exported AccountDatabase schema JSON.

  • severity: medium · category: test-gap · angle: tests

A future ACCOUNT_MIGRATION_2_3 is authored in AccountMigrations.kt, its 3.json schema committed, and a replay test added — but the AccountDatabaseModule.addMigrations call (line 56 of di/AccountDatabaseModule.kt) is not updated. All tests stay green (the replay test passes the migration explicitly), yet AccountDatabase registers no destructive fallback, so every upgrading user crash-loops at first open of the database that holds their accounts and credentials — the exact bug class #312's guards were added to prevent for the cache DB, unreplicated here.

app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt:106 — deleteAccount removes the account row (AccountDatabase) FIRST and cleans the cache database afterwards with no retry marker, so process death mid-sequence permanently orphans the deleted account's messages, folders, drafts and backfill progress.

  • severity: medium · category: crash-safety · angle: invariants · flagged by 2 finder(s)

User deletes an account; the process is killed right after accountDao.deleteById commits (separate database file — no cross-DB transaction possible). On restart the account is gone from every list so the deletion can't be re-run, but its message rows remain: pagingUnifiedFolderSummaries doesn't join accounts, so the removed account's mail keeps appearing in the unified inbox forever (openMessage finds account == null and renders an empty body for unfetched rows); MailSyncer/MailPruner iterate accountDao.getAll() and therefore never touch or clean the orphans. The mail the user asked to remove also stays on disk indefinitely. Deleting cache rows first and the account row last would make the operation crash-retryable.

app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt:112 — deleteAccount is a non-atomic multi-step teardown with no coordination with in-flight sync/backfill: MailBackfiller iterates a snapshot from accountDao.getAll() and keeps calling messageDao.insertNew long after the account row and its messages are deleted, re-creating orphan rows that nothing ever prunes (MailPruner and MailSyncer only iterate existing accounts).

  • severity: medium · category: concurrency · angle: crossfile · flagged by 2 finder(s)

User deletes an account while its full-history backfill (the #12 default) is running: after messageDao.deleteByAccount, the backfiller inserts further pages of that account's messages (an authenticated reused connection keeps working even after credentialStore.delete). The unified inbox permanently shows ghost messages whose taps fail with 'Account not found', and their attachment-cache dirs (written after the getIdsForAccount enumeration) leak. A process kill between accountDao.deleteById and the cache-side deletes produces the same orphans without any race.

app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt:121 — deleteAccount runs one attachmentCacheDir(...).deleteRecursively() per message row of the deleted account on the main thread, and attachmentCacheDir compiles a fresh Regex per call — N filesystem stats/walks + N regex compilations where N is the account's entire message history.

  • severity: medium · category: concurrency · angle: concurrency · flagged by 2 finder(s)

User removes an account whose full history was backfilled (issue #12 fetches entire history by default — tens of thousands of rows): AccountSettingsViewModel.removeAccount launches deleteAccount in viewModelScope (Main.immediate), so the messageIds.forEach loop performs ~50k+ File stat/delete operations plus 50k Regex compiles on the UI thread, freezing the Settings screen for multiple seconds (ANR risk). Cheaper: run the cleanup under withContext(Dispatchers.IO) and replace the per-id probe with a single listFiles() of the attachments/ root, deleting only dirs whose name starts with the account's sanitized id prefix (message ids are 'accountId:folder:uid').

app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:308 — ensureAttachmentFile's cache check (exists && length > 0) plus a non-atomic direct write to the final path lets a concurrent or interrupted writer make a truncated attachment file look like a complete cache hit — prefetch and user-tap download deliberately share this file with no locking or temp-file+rename.

  • severity: medium · category: concurrency · angle: concurrency

Backfill's prefetchMessage is mid-write on a 5 MB attachment when the user taps that attachment (or an inline image is loaded): the second caller sees exists() && length()>0 on the partially written file and returns it, so a corrupted PDF/image is opened or shared. Process death mid-write leaves the truncated file passing the same check forever, permanently serving a corrupt attachment from cache.

app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:367 — setStarred's comment 'next sync reconciles on failure' is false: sync deliberately never touches read/star flags (updateHeaderContent excludes them) and there is no retry, so a failed FLAGGED push diverges local and server star state permanently.

  • severity: medium · category: correctness · angle: logic · flagged by 3 finder(s)

User stars a message while the network is flaky; messageDao.setStarred commits locally, imapClient.setFlag throws. Unlike the SEEN path (which at least retries 3 times in pushSeenFlagInBackground and documents the gap), there is no retry and no reconcile: MailSyncer.updateHeaderContents intentionally preserves local optimistic flags, so the local star is never corrected and the server flag is never set. The device shows the message starred forever while every other client shows it unstarred (and vice versa for un-starring).

app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:384 — deleteMessage, trash/archive/spam (moveByRole), expunge and moveToFolder delete message rows (attachment rows cascade) but never delete the per-message on-disk attachment cache, orphaning the files — only MailPruner and deleteAccount clean that directory.

  • severity: medium · category: resource-leak · angle: logic

User deletes or trashes messages with large downloaded attachments: the rows vanish, but cacheDir/attachments// keeps the bytes with no row left to enumerate them (the exact issue-#299 leak, fixed for account deletion only); with retention off (the #12 fetch-everything default means prune never visits them) the orphans accumulate until Android evicts the whole cache dir under storage pressure, and deleted-message content also lingers on disk after the user 'deleted' it.

app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:461 — The optimistic local delete in moveByRole (also expunge/moveToFolder) is only reconciled for messages inside the recent sync UID window; for older backfilled messages a failed server op makes the message vanish from the app permanently while it still exists on the server.

  • severity: medium · category: correctness · angle: invariants · flagged by 2 finder(s)

In airplane mode the user archives (or trashes) a 3-month-old message whose UID is below MailSyncer's recent fetch window. deleteByIdsChunked removes the local row, then the IMAP move throws. MailSyncer.deleteSyncedInWindowNotIn/insertNew only re-add rows within the recent window, and MailBackfiller never re-pages a range it already passed (backfill_progress is at/below that UID or complete), so the row is never re-fetched. The message remains in INBOX on the server (visible to every other client) but is invisible in LibreMail until the user resets backfill or clears the cache — despite the code comment claiming 'reconciled on the next sync'.

app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:587 — searchServer re-implements the batched header refresh by looping the single-row MessageDao.updateHeaderContent per search hit instead of calling the existing MessageDao.updateHeaderContents(entities) batch helper that was added for exactly this (issue #310).

  • severity: medium · category: efficiency · angle: crossfile · flagged by 4 finder(s)

A server search returning the SEARCH_LIMIT of 50 hits per account issues 50 separate UPDATE statements, each in its own implicit transaction — 50 journal commits/fsyncs per account per search, amplified on the opt-in SQLCipher-encrypted cache — exactly the N-commits-per-sync cost updateHeaderContents' own KDoc documents eliminating. MailSyncer (line 137) and MailBackfiller (line 285) already use the batched overload; if the header-refresh contract changes (e.g. a new fold column added to the batch path), the searchServer copy silently diverges and search-inserted rows get stale/unfolded columns. Fix: replace the entities.forEach { messageDao.updateHeaderContent(...) } loop with messageDao.updateHeaderContents(entities).

Low (15)

app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt:88 — The SQLite sidecar suffix list (-wal/-shm/-journal) is hand-rolled in three places — DatabaseEncryption.migrate (lines 88-90), DatabaseEncryption.probeKeyedOpen (line 152), and AccountDataMigrator.copyAccountTables (line 196) — instead of reusing DatabaseFiles.SIDECAR_SUFFIXES / DatabaseFiles.fileNames(), whose KDoc declares it 'the single source of truth for which on-disk files make up a database file'.

  • severity: low · category: reuse · angle: reuse

If a new sidecar kind is ever added to DatabaseFiles.SIDECAR_SUFFIXES (the exact evolution its doc anticipates — BackupPolicy already derives its never-back-up set from fileNames so it 'can never silently fall out'), the three hand-rolled sweeps do not pick it up: the encryption converter's pre-swap cleanup and the account migrator's post-copy cleanup would leave the new stale sidecar next to the swapped/re-opened file for Room to mis-read, while backup exclusion (driven by the shared list) stays correct — a divergence invisible until a corrupted-open bug. Fix: expose SIDECAR_SUFFIXES (or a sidecarNames(name) helper) from DatabaseFiles and use it at all three sites.

app/src/main/kotlin/org/libremail/data/local/DatabaseProvisioner.kt:23 — CacheOpenMode.Encrypted is a data class holding the raw SQLCipher passphrase, so its auto-generated toString() prints the passphrase — a latent credential-leak hazard given the repo's strict no-secrets-in-logs rule.

  • severity: low · category: security · angle: security

The memoized prepared value lives for the process lifetime; any future debug log, exception message, or state dump that stringifies the CacheOpenMode (e.g. AppLog.d(TAG, "open mode: $mode") added during diagnosis, or a when branch logging the unexpected value) would emit "Encrypted(passphrase=<64-hex-key>)" into Logcat and the RingLogBuffer that feeds user-shared DebugReports, exposing the cache encryption key. Overriding toString() (or holding a ByteArray/non-data class) removes the trap.

app/src/main/kotlin/org/libremail/data/local/DatabaseProvisioner.kt:102 — The key-invalidation wipe path (isClearPending -> DatabaseFiles.clear + resetSealedPassphrase + clearClearPending) destroys the user's entire local mail cache with no AppLog entry at all, violating CLAUDE.md's Definition of done requirement that 'lifecycle transitions, error/fallback paths, significant state changes' be logged 'so behaviour is diagnosable from a user's debug report.'

  • severity: low · category: conventions · angle: conventions

User re-enrolls biometrics or removes the screen lock; on next launch runStartupSequence silently deletes libremail.db and reseals the passphrase (lines 101-105, and DatabaseFiles.clear itself is also log-free). The user files a debug report saying 'all my cached mail vanished and the app re-downloaded everything' — the RingLogBuffer/DebugReport contains no record that a wipe happened or why, so the report cannot distinguish the intended clear-and-resync from data-loss corruption.

app/src/main/kotlin/org/libremail/data/local/dao/DraftDao.kt:24 — DraftDao.getAll() (and OutboxDao.getAll()) are SELECT * queries used by AttachmentUriGrants.releaseUnreferenced solely to read the attachments JSON column, dragging every draft's and queued message's full body + bodyHtml through SQLite's shared ~2 MB CursorWindow on every draft delete, outbox cancel, and account delete.

  • severity: low · category: efficiency · angle: efficiency

A user with several large rich-HTML drafts (each hundreds of KB of bodyHtml) deletes one draft: releaseUnreferenced materializes every draft and outbox row in full just to parse their attachment URI lists, wasting CursorWindow paging and allocation proportional to total draft body size — the same over-fetch pattern issue #186/#51 removed from the message paths. Cheaper: add projection queries (e.g. @Query("SELECT attachments FROM drafts") suspend fun getAllAttachmentJson(): List, and the outbox equivalent) and use them in AttachmentUriGrants and AccountRepositoryImpl.deleteAccount.

app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt:27 — pagingUnifiedFolderSummaries' KDoc still claims 'the first page loads flat regardless of total cache size on the existing indices, so no (folder, …) index / schema migration is added', but MIGRATION_19_20 and MessageEntity's Index("folder", "inInbox", "timestampMillis") added exactly that index because the query was in fact a full SCAN (issue #187).

  • severity: low · category: conventions · angle: reuse

The stale sentence asserts the opposite of the current schema and of MessageEntity's own index comment/MIGRATION_19_20's rationale. A maintainer optimizing or re-planning this query from the DAO doc concludes no folder-led index exists (and that none is needed), and either re-derives/duplicates the EXPLAIN QUERY PLAN investigation issue #187 already did or wrongly trusts the 'loads flat on existing indices' claim when changing the WHERE/ORDER BY. Fix: replace the sentence with a pointer to index_messages_folder_inInbox_timestampMillis (issue #187 / MIGRATION_19_20).

app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt:74 — The escaped-LIKE search seam is never exercised end-to-end on real SQLite: MailRepositoryImplTest pins likePattern's output ("%50\%\_off%") only against a MockK'd DAO string capture, while MessageDaoTest's instrumented search tests only ever pass metacharacter-free patterns like "%report%", so the queries' ESCAPE '' clause and SQLite's actual treatment of the escaped pattern are unverified.

  • severity: low · category: test-gap · angle: tests

If the ESCAPE '' clause were dropped or its escape character changed in the four *SearchSummaries queries (or likePattern's escaping drifted out of sync with it), every existing test still passes — the unit test compares a captured string, the DAO tests never send an escape sequence. A user searching for a term containing %, _ or \ (e.g. "50%off") then silently gets wrong results: '%' matches any run of characters and '' any single character, so the search returns unrelated messages (or none), with no failing test to flag the regression.

app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:96 — pagedUnifiedFolderMessages inlines a PagingConfig (pageSize/initialLoadSize/enablePlaceholders/maxSize) byte-for-byte identical to the one encapsulated in the private mailboxPager helper defined directly below it, which the other three pagers already use.

  • severity: low · category: reuse · angle: reuse · flagged by 2 finder(s)

Two copies of the mailbox window sizing now exist. When someone retunes the paging window (as happened when maxSize was introduced — the maxSize comment explains the pageSize + 2*prefetchDistance constraint only on the inline copy), editing mailboxPager alone leaves the unified inbox on the old sizing (or vice versa), so the unified and per-account lists silently diverge in memory footprint and load behaviour. Fix: pagedUnifiedFolderMessages = mailboxPager { messageDao.pagingUnifiedFolderSummaries(folder) }, moving the sizing-rationale comments onto the helper.

app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:232 — pushSeenFlagInBackground gives up after SEEN_FLAG_PUSH_MAX_ATTEMPTS with no AppLog on the permanent-failure path (its own doc comment says 'gives up silently'), violating CLAUDE.md's Definition of done: error/fallback paths must be logged 'so behaviour is diagnosable from a user's debug report.'

  • severity: low · category: conventions · angle: conventions

Device loses connectivity right after a cached message is opened; all 3 setFlag attempts throw inside runCatching at line 231 and the coroutine exits at line 232 without any log. The server copy stays unread forever (folder sync never re-drives the push), the user reports 'messages I read on my phone show unread in webmail,' and the debug report contains no evidence the SEEN push was attempted or abandoned.

app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:361 — Gmail-specific bandwidth policy is hard-wired into the generic prefetch path: MailRepositoryImpl injects GmailBandwidthTracker and inlines a GmailSyncLimits.appliesTo(account) branch, rather than recording bytes against a per-provider limits policy resolved from the account.

  • severity: low · category: altitude · angle: altitude

When the sibling Yahoo (#362) daily-budget ticket lands (iCloud #363 and Outlook #364 already each added their own MailProvider.forImapHost checks elsewhere), this generic repository must gain a second injected tracker and an 'appliesTo' if-chain, and the same provider dispatch is re-duplicated in MailSyncer/MailBackfiller's prefetchIfEnabled deferral — every new provider budget means re-touching the provider-agnostic repository and sync engine instead of adding one provider-policy object; a missed call site silently exempts that provider's downloads from its budget.

app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:472 — moveByRole lacks moveToFolder's source != destination guard, so archiving/spam-reporting/trashing a message while viewing that same role folder issues an IMAP move of a folder onto itself.

  • severity: low · category: correctness · angle: logic

User browses the Spam folder per-account and taps 'report spam' (or archives from Archive): the local rows are optimistically deleted, then imapClient.moveMessages(params, '[Gmail]/Spam', uids, '[Gmail]/Spam') runs — a COPY-to-self plus delete/expunge that, depending on the server, duplicates the messages or churns their UIDs, after which the next sync re-downloads them with new ids (bodies refetched, read state from server). moveToFolder (line 411) has exactly this guard, showing the omission is accidental.

app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:568 — cancelOutboxMessage performs File(...).deleteRecursively() on the caller's dispatcher — the main thread via OutboxViewModel (viewModelScope.launch) — unlike the repository's other file-touching methods which wrap in withContext(Dispatchers.IO).

  • severity: low · category: concurrency · angle: concurrency

Cancelling a queued message that staged several large attachments deletes those files synchronously on the main thread; on slow flash storage this janks (dropped frames / brief freeze) the outbox screen every time a send is cancelled.

app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:580 — searchServer swallows every per-account failure in a bare runCatching with no AppLog, so a server search that fails (offline, auth expiry, throttling) silently produces zero results and is undiagnosable from a debug report.

  • severity: low · category: conventions · angle: logic · flagged by 4 finder(s)

User searches while their Gmail account is being throttled or the token has expired: imapClient.search throws for every account, runCatching discards each error, the UI just shows only local matches with no indication server search failed, and the RingLogBuffer/DebugReport contains nothing — contradicting the project's definition-of-done requirement that error/fallback paths log via AppLog (compare openMessage/pushSeenFlag which do).

app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:627 — The SQLite 999-host-parameter protection (issue #313) is implemented per call site — private SQL_IN_CHUNK=500 extension wrappers here plus a copy-pasted DELETE_CHUNK=500 in MailPruner — while the underlying MessageDao IN-list methods (deleteByIds, markSynced, existingIds, getRoutingByIds) remain unguarded and rely on every caller remembering to stay under the limit.

  • severity: low · category: altitude · angle: altitude

The next caller of messageDao.deleteByIds/markSynced with an unbounded list (e.g. a 'select all' action, raising the multi-select cap, or a backfill batch size increase past 999 — today's safety is only FETCH_LIMIT=50/BACKFILL_BATCH_SIZE=50 staying small) crashes with SQLiteException 'too many SQL variables', and the fix gets re-applied as a third copy of the chunking constant instead of once as DAO default methods (the layer where updateHeaderContents already wraps its transaction).

app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:635 — uidOf's KDoc documents the message-id format as ":" but the actual format built by FetchedMessage.toEntity (Mappers.kt line 137) has been "::" since the v8 folder-aware migration; only the 'trailing segment' half of the sentence is still true.

  • severity: low · category: conventions · angle: reuse · flagged by 2 finder(s)

The doc is the one place the id format is stated in this file, and it is wrong. A maintainer composing or parsing ids from it — e.g. reconstructing an id as "$accountId:$uid" for a lookup, or extracting the folder with substringAfter(':') — produces ids that match no row (silent lookup misses / duplicate rows) since real ids embed the folder between the two documented segments. Fix: correct the comment to "::" (matching MIGRATION_7_8's doc) or point it at FetchedMessage.toEntity as the format's single source of truth.

app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt:371 — The SEEN-flag retry test runs on the real clock — MailRepositoryImpl.backgroundScope hardcodes Dispatchers.IO so the 2s real delay() cannot be virtualized, making the test cost >=2s wall time per run with only a 3s margin (coVerify timeout 5s) — and the retry give-up cap (SEEN_FLAG_PUSH_MAX_ATTEMPTS = 3) is not tested at all.

  • severity: low · category: test-gap · angle: tests

On a loaded CI shard the second attempt (first retry fires after a real 2s backoff on Dispatchers.IO) can land past the 5s verify window, flaking the suite; and a regression that removes the attempt cap (making pushSeenFlagInBackground retry forever, hammering the IMAP server every few seconds for each opened cached-unread message and draining battery/connection slots) passes every existing test, since only 'at least 2 attempts' is ever asserted and no test observes that attempts stop at 3.

Source: whole-repo multi-agent review, 2026-07-09 (run `wf_b41de68c-e85`). The run was cut short by usage limits before its verification pass, so every finding below is an **unverified finder candidate** — validate each against the current code before implementing. Findings are listed medium first, then low. Critical/high candidates from the same run were verified separately and have their own issues. ## Medium (10) ### `app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt:70` — migrateIfNeeded() — the done-flag gating, the encrypted-source passphrase branch, and the missing-cache skip — has zero test coverage: AccountDataMigratorTest exercises only the static copyAccountTables core, and every other test (DatabaseProvisionerTest, DatabaseProvisionerInstrumentedTest, DatabaseModuleInstrumentedTest, AccountDatabaseModuleInstrumentedTest) stubs the whole class with mockk. - severity: **medium** · category: `test-gap` · angle: `tests` > A regression in the orchestration — e.g. markDone() moved before the copy (or reached after a swallowed copy failure), the isDone() early-return inverted, or the DatabaseEncryption.isEncrypted(cacheFile) branch resolving the wrong passphrase source — passes the entire suite green. On an upgrading pre-#111 install the copy is then skipped or fails silently, migrateIfNeeded marks done, Room opens the cache and MIGRATION_15_16 drops accounts/credentials/settings/signatures: every upgrading user is permanently signed out with their credentials destroyed, and no test in the repo can catch it because none ever runs the real migrateIfNeeded against a real DataStore done-flag. ### `app/src/main/kotlin/org/libremail/data/local/AccountMigrations.kt:25` — The AccountDatabase migration chain lacks the chain-completeness and DI-registration parity guards that MigrationTest gives the cache database (issue #312): AccountMigrationTest only replays 1->2, and nothing asserts AccountDatabaseModule's addMigrations(ACCOUNT_MIGRATION_1_2) registers every declared ACCOUNT_ migration up to the newest exported AccountDatabase schema JSON. - severity: **medium** · category: `test-gap` · angle: `tests` > A future ACCOUNT_MIGRATION_2_3 is authored in AccountMigrations.kt, its 3.json schema committed, and a replay test added — but the AccountDatabaseModule.addMigrations call (line 56 of di/AccountDatabaseModule.kt) is not updated. All tests stay green (the replay test passes the migration explicitly), yet AccountDatabase registers no destructive fallback, so every upgrading user crash-loops at first open of the database that holds their accounts and credentials — the exact bug class #312's guards were added to prevent for the cache DB, unreplicated here. ### `app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt:106` — deleteAccount removes the account row (AccountDatabase) FIRST and cleans the cache database afterwards with no retry marker, so process death mid-sequence permanently orphans the deleted account's messages, folders, drafts and backfill progress. - severity: **medium** · category: `crash-safety` · angle: `invariants` · flagged by 2 finder(s) > User deletes an account; the process is killed right after accountDao.deleteById commits (separate database file — no cross-DB transaction possible). On restart the account is gone from every list so the deletion can't be re-run, but its message rows remain: pagingUnifiedFolderSummaries doesn't join accounts, so the removed account's mail keeps appearing in the unified inbox forever (openMessage finds account == null and renders an empty body for unfetched rows); MailSyncer/MailPruner iterate accountDao.getAll() and therefore never touch or clean the orphans. The mail the user asked to remove also stays on disk indefinitely. Deleting cache rows first and the account row last would make the operation crash-retryable. ### `app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt:112` — deleteAccount is a non-atomic multi-step teardown with no coordination with in-flight sync/backfill: MailBackfiller iterates a snapshot from accountDao.getAll() and keeps calling messageDao.insertNew long after the account row and its messages are deleted, re-creating orphan rows that nothing ever prunes (MailPruner and MailSyncer only iterate existing accounts). - severity: **medium** · category: `concurrency` · angle: `crossfile` · flagged by 2 finder(s) > User deletes an account while its full-history backfill (the #12 default) is running: after messageDao.deleteByAccount, the backfiller inserts further pages of that account's messages (an authenticated reused connection keeps working even after credentialStore.delete). The unified inbox permanently shows ghost messages whose taps fail with 'Account not found', and their attachment-cache dirs (written after the getIdsForAccount enumeration) leak. A process kill between accountDao.deleteById and the cache-side deletes produces the same orphans without any race. ### `app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt:121` — deleteAccount runs one attachmentCacheDir(...).deleteRecursively() per message row of the deleted account on the main thread, and attachmentCacheDir compiles a fresh Regex per call — N filesystem stats/walks + N regex compilations where N is the account's entire message history. - severity: **medium** · category: `concurrency` · angle: `concurrency` · flagged by 2 finder(s) > User removes an account whose full history was backfilled (issue #12 fetches entire history by default — tens of thousands of rows): AccountSettingsViewModel.removeAccount launches deleteAccount in viewModelScope (Main.immediate), so the messageIds.forEach loop performs ~50k+ File stat/delete operations plus 50k Regex compiles on the UI thread, freezing the Settings screen for multiple seconds (ANR risk). Cheaper: run the cleanup under withContext(Dispatchers.IO) and replace the per-id probe with a single listFiles() of the attachments/ root, deleting only dirs whose name starts with the account's sanitized id prefix (message ids are 'accountId:folder:uid'). ### `app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:308` — ensureAttachmentFile's cache check (exists && length > 0) plus a non-atomic direct write to the final path lets a concurrent or interrupted writer make a truncated attachment file look like a complete cache hit — prefetch and user-tap download deliberately share this file with no locking or temp-file+rename. - severity: **medium** · category: `concurrency` · angle: `concurrency` > Backfill's prefetchMessage is mid-write on a 5 MB attachment when the user taps that attachment (or an inline image is loaded): the second caller sees exists() && length()>0 on the partially written file and returns it, so a corrupted PDF/image is opened or shared. Process death mid-write leaves the truncated file passing the same check forever, permanently serving a corrupt attachment from cache. ### `app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:367` — setStarred's comment 'next sync reconciles on failure' is false: sync deliberately never touches read/star flags (updateHeaderContent excludes them) and there is no retry, so a failed FLAGGED push diverges local and server star state permanently. - severity: **medium** · category: `correctness` · angle: `logic` · flagged by 3 finder(s) > User stars a message while the network is flaky; messageDao.setStarred commits locally, imapClient.setFlag throws. Unlike the SEEN path (which at least retries 3 times in pushSeenFlagInBackground and documents the gap), there is no retry and no reconcile: MailSyncer.updateHeaderContents intentionally preserves local optimistic flags, so the local star is never corrected and the server flag is never set. The device shows the message starred forever while every other client shows it unstarred (and vice versa for un-starring). ### `app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:384` — deleteMessage, trash/archive/spam (moveByRole), expunge and moveToFolder delete message rows (attachment rows cascade) but never delete the per-message on-disk attachment cache, orphaning the files — only MailPruner and deleteAccount clean that directory. - severity: **medium** · category: `resource-leak` · angle: `logic` > User deletes or trashes messages with large downloaded attachments: the rows vanish, but cacheDir/attachments/<safeId>/ keeps the bytes with no row left to enumerate them (the exact issue-#299 leak, fixed for account deletion only); with retention off (the #12 fetch-everything default means prune never visits them) the orphans accumulate until Android evicts the whole cache dir under storage pressure, and deleted-message content also lingers on disk after the user 'deleted' it. ### `app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:461` — The optimistic local delete in moveByRole (also expunge/moveToFolder) is only reconciled for messages inside the recent sync UID window; for older backfilled messages a failed server op makes the message vanish from the app permanently while it still exists on the server. - severity: **medium** · category: `correctness` · angle: `invariants` · flagged by 2 finder(s) > In airplane mode the user archives (or trashes) a 3-month-old message whose UID is below MailSyncer's recent fetch window. deleteByIdsChunked removes the local row, then the IMAP move throws. MailSyncer.deleteSyncedInWindowNotIn/insertNew only re-add rows within the recent window, and MailBackfiller never re-pages a range it already passed (backfill_progress is at/below that UID or complete), so the row is never re-fetched. The message remains in INBOX on the server (visible to every other client) but is invisible in LibreMail until the user resets backfill or clears the cache — despite the code comment claiming 'reconciled on the next sync'. ### `app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:587` — searchServer re-implements the batched header refresh by looping the single-row MessageDao.updateHeaderContent per search hit instead of calling the existing MessageDao.updateHeaderContents(entities) batch helper that was added for exactly this (issue #310). - severity: **medium** · category: `efficiency` · angle: `crossfile` · flagged by 4 finder(s) > A server search returning the SEARCH_LIMIT of 50 hits per account issues 50 separate UPDATE statements, each in its own implicit transaction — 50 journal commits/fsyncs per account per search, amplified on the opt-in SQLCipher-encrypted cache — exactly the N-commits-per-sync cost updateHeaderContents' own KDoc documents eliminating. MailSyncer (line 137) and MailBackfiller (line 285) already use the batched overload; if the header-refresh contract changes (e.g. a new fold column added to the batch path), the searchServer copy silently diverges and search-inserted rows get stale/unfolded columns. Fix: replace the entities.forEach { messageDao.updateHeaderContent(...) } loop with messageDao.updateHeaderContents(entities). ## Low (15) ### `app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt:88` — The SQLite sidecar suffix list (-wal/-shm/-journal) is hand-rolled in three places — DatabaseEncryption.migrate (lines 88-90), DatabaseEncryption.probeKeyedOpen (line 152), and AccountDataMigrator.copyAccountTables (line 196) — instead of reusing DatabaseFiles.SIDECAR_SUFFIXES / DatabaseFiles.fileNames(), whose KDoc declares it 'the single source of truth for which on-disk files make up a database file'. - severity: **low** · category: `reuse` · angle: `reuse` > If a new sidecar kind is ever added to DatabaseFiles.SIDECAR_SUFFIXES (the exact evolution its doc anticipates — BackupPolicy already derives its never-back-up set from fileNames so it 'can never silently fall out'), the three hand-rolled sweeps do not pick it up: the encryption converter's pre-swap cleanup and the account migrator's post-copy cleanup would leave the new stale sidecar next to the swapped/re-opened file for Room to mis-read, while backup exclusion (driven by the shared list) stays correct — a divergence invisible until a corrupted-open bug. Fix: expose SIDECAR_SUFFIXES (or a sidecarNames(name) helper) from DatabaseFiles and use it at all three sites. ### `app/src/main/kotlin/org/libremail/data/local/DatabaseProvisioner.kt:23` — CacheOpenMode.Encrypted is a data class holding the raw SQLCipher passphrase, so its auto-generated toString() prints the passphrase — a latent credential-leak hazard given the repo's strict no-secrets-in-logs rule. - severity: **low** · category: `security` · angle: `security` > The memoized `prepared` value lives for the process lifetime; any future debug log, exception message, or state dump that stringifies the CacheOpenMode (e.g. AppLog.d(TAG, "open mode: $mode") added during diagnosis, or a `when` branch logging the unexpected value) would emit "Encrypted(passphrase=<64-hex-key>)" into Logcat and the RingLogBuffer that feeds user-shared DebugReports, exposing the cache encryption key. Overriding toString() (or holding a ByteArray/non-data class) removes the trap. ### `app/src/main/kotlin/org/libremail/data/local/DatabaseProvisioner.kt:102` — The key-invalidation wipe path (isClearPending -> DatabaseFiles.clear + resetSealedPassphrase + clearClearPending) destroys the user's entire local mail cache with no AppLog entry at all, violating CLAUDE.md's Definition of done requirement that 'lifecycle transitions, error/fallback paths, significant state changes' be logged 'so behaviour is diagnosable from a user's debug report.' - severity: **low** · category: `conventions` · angle: `conventions` > User re-enrolls biometrics or removes the screen lock; on next launch runStartupSequence silently deletes libremail.db and reseals the passphrase (lines 101-105, and DatabaseFiles.clear itself is also log-free). The user files a debug report saying 'all my cached mail vanished and the app re-downloaded everything' — the RingLogBuffer/DebugReport contains no record that a wipe happened or why, so the report cannot distinguish the intended clear-and-resync from data-loss corruption. ### `app/src/main/kotlin/org/libremail/data/local/dao/DraftDao.kt:24` — DraftDao.getAll() (and OutboxDao.getAll()) are SELECT * queries used by AttachmentUriGrants.releaseUnreferenced solely to read the attachments JSON column, dragging every draft's and queued message's full body + bodyHtml through SQLite's shared ~2 MB CursorWindow on every draft delete, outbox cancel, and account delete. - severity: **low** · category: `efficiency` · angle: `efficiency` > A user with several large rich-HTML drafts (each hundreds of KB of bodyHtml) deletes one draft: releaseUnreferenced materializes every draft and outbox row in full just to parse their attachment URI lists, wasting CursorWindow paging and allocation proportional to total draft body size — the same over-fetch pattern issue #186/#51 removed from the message paths. Cheaper: add projection queries (e.g. @Query("SELECT attachments FROM drafts") suspend fun getAllAttachmentJson(): List<String>, and the outbox equivalent) and use them in AttachmentUriGrants and AccountRepositoryImpl.deleteAccount. ### `app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt:27` — pagingUnifiedFolderSummaries' KDoc still claims 'the first page loads flat regardless of total cache size on the existing indices, so no (folder, …) index / schema migration is added', but MIGRATION_19_20 and MessageEntity's Index("folder", "inInbox", "timestampMillis") added exactly that index because the query was in fact a full SCAN (issue #187). - severity: **low** · category: `conventions` · angle: `reuse` > The stale sentence asserts the opposite of the current schema and of MessageEntity's own index comment/MIGRATION_19_20's rationale. A maintainer optimizing or re-planning this query from the DAO doc concludes no folder-led index exists (and that none is needed), and either re-derives/duplicates the EXPLAIN QUERY PLAN investigation issue #187 already did or wrongly trusts the 'loads flat on existing indices' claim when changing the WHERE/ORDER BY. Fix: replace the sentence with a pointer to index_messages_folder_inInbox_timestampMillis (issue #187 / MIGRATION_19_20). ### `app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt:74` — The escaped-LIKE search seam is never exercised end-to-end on real SQLite: MailRepositoryImplTest pins likePattern's output ("%50\\%\\_off%") only against a MockK'd DAO string capture, while MessageDaoTest's instrumented search tests only ever pass metacharacter-free patterns like "%report%", so the queries' ESCAPE '\' clause and SQLite's actual treatment of the escaped pattern are unverified. - severity: **low** · category: `test-gap` · angle: `tests` > If the ESCAPE '\' clause were dropped or its escape character changed in the four *SearchSummaries queries (or likePattern's escaping drifted out of sync with it), every existing test still passes — the unit test compares a captured string, the DAO tests never send an escape sequence. A user searching for a term containing %, _ or \ (e.g. "50%_off") then silently gets wrong results: '%' matches any run of characters and '_' any single character, so the search returns unrelated messages (or none), with no failing test to flag the regression. ### `app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:96` — pagedUnifiedFolderMessages inlines a PagingConfig (pageSize/initialLoadSize/enablePlaceholders/maxSize) byte-for-byte identical to the one encapsulated in the private mailboxPager helper defined directly below it, which the other three pagers already use. - severity: **low** · category: `reuse` · angle: `reuse` · flagged by 2 finder(s) > Two copies of the mailbox window sizing now exist. When someone retunes the paging window (as happened when maxSize was introduced — the maxSize comment explains the pageSize + 2*prefetchDistance constraint only on the inline copy), editing mailboxPager alone leaves the unified inbox on the old sizing (or vice versa), so the unified and per-account lists silently diverge in memory footprint and load behaviour. Fix: pagedUnifiedFolderMessages = mailboxPager { messageDao.pagingUnifiedFolderSummaries(folder) }, moving the sizing-rationale comments onto the helper. ### `app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:232` — pushSeenFlagInBackground gives up after SEEN_FLAG_PUSH_MAX_ATTEMPTS with no AppLog on the permanent-failure path (its own doc comment says 'gives up silently'), violating CLAUDE.md's Definition of done: error/fallback paths must be logged 'so behaviour is diagnosable from a user's debug report.' - severity: **low** · category: `conventions` · angle: `conventions` > Device loses connectivity right after a cached message is opened; all 3 setFlag attempts throw inside runCatching at line 231 and the coroutine exits at line 232 without any log. The server copy stays unread forever (folder sync never re-drives the push), the user reports 'messages I read on my phone show unread in webmail,' and the debug report contains no evidence the SEEN push was attempted or abandoned. ### `app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:361` — Gmail-specific bandwidth policy is hard-wired into the generic prefetch path: MailRepositoryImpl injects GmailBandwidthTracker and inlines a GmailSyncLimits.appliesTo(account) branch, rather than recording bytes against a per-provider limits policy resolved from the account. - severity: **low** · category: `altitude` · angle: `altitude` > When the sibling Yahoo (#362) daily-budget ticket lands (iCloud #363 and Outlook #364 already each added their own MailProvider.forImapHost checks elsewhere), this generic repository must gain a second injected tracker and an 'appliesTo' if-chain, and the same provider dispatch is re-duplicated in MailSyncer/MailBackfiller's prefetchIfEnabled deferral — every new provider budget means re-touching the provider-agnostic repository and sync engine instead of adding one provider-policy object; a missed call site silently exempts that provider's downloads from its budget. ### `app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:472` — moveByRole lacks moveToFolder's source != destination guard, so archiving/spam-reporting/trashing a message while viewing that same role folder issues an IMAP move of a folder onto itself. - severity: **low** · category: `correctness` · angle: `logic` > User browses the Spam folder per-account and taps 'report spam' (or archives from Archive): the local rows are optimistically deleted, then imapClient.moveMessages(params, '[Gmail]/Spam', uids, '[Gmail]/Spam') runs — a COPY-to-self plus delete/expunge that, depending on the server, duplicates the messages or churns their UIDs, after which the next sync re-downloads them with new ids (bodies refetched, read state from server). moveToFolder (line 411) has exactly this guard, showing the omission is accidental. ### `app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:568` — cancelOutboxMessage performs File(...).deleteRecursively() on the caller's dispatcher — the main thread via OutboxViewModel (viewModelScope.launch) — unlike the repository's other file-touching methods which wrap in withContext(Dispatchers.IO). - severity: **low** · category: `concurrency` · angle: `concurrency` > Cancelling a queued message that staged several large attachments deletes those files synchronously on the main thread; on slow flash storage this janks (dropped frames / brief freeze) the outbox screen every time a send is cancelled. ### `app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:580` — searchServer swallows every per-account failure in a bare runCatching with no AppLog, so a server search that fails (offline, auth expiry, throttling) silently produces zero results and is undiagnosable from a debug report. - severity: **low** · category: `conventions` · angle: `logic` · flagged by 4 finder(s) > User searches while their Gmail account is being throttled or the token has expired: imapClient.search throws for every account, runCatching discards each error, the UI just shows only local matches with no indication server search failed, and the RingLogBuffer/DebugReport contains nothing — contradicting the project's definition-of-done requirement that error/fallback paths log via AppLog (compare openMessage/pushSeenFlag which do). ### `app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:627` — The SQLite 999-host-parameter protection (issue #313) is implemented per call site — private SQL_IN_CHUNK=500 extension wrappers here plus a copy-pasted DELETE_CHUNK=500 in MailPruner — while the underlying MessageDao IN-list methods (deleteByIds, markSynced, existingIds, getRoutingByIds) remain unguarded and rely on every caller remembering to stay under the limit. - severity: **low** · category: `altitude` · angle: `altitude` > The next caller of messageDao.deleteByIds/markSynced with an unbounded list (e.g. a 'select all' action, raising the multi-select cap, or a backfill batch size increase past 999 — today's safety is only FETCH_LIMIT=50/BACKFILL_BATCH_SIZE=50 staying small) crashes with SQLiteException 'too many SQL variables', and the fix gets re-applied as a third copy of the chunking constant instead of once as DAO default methods (the layer where updateHeaderContents already wraps its transaction). ### `app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt:635` — uidOf's KDoc documents the message-id format as "<accountId>:<uid>" but the actual format built by FetchedMessage.toEntity (Mappers.kt line 137) has been "<accountId>:<folder>:<uid>" since the v8 folder-aware migration; only the 'trailing segment' half of the sentence is still true. - severity: **low** · category: `conventions` · angle: `reuse` · flagged by 2 finder(s) > The doc is the one place the id format is stated in this file, and it is wrong. A maintainer composing or parsing ids from it — e.g. reconstructing an id as "$accountId:$uid" for a lookup, or extracting the folder with substringAfter(':') — produces ids that match no row (silent lookup misses / duplicate rows) since real ids embed the folder between the two documented segments. Fix: correct the comment to "<accountId>:<folder>:<uid>" (matching MIGRATION_7_8's doc) or point it at FetchedMessage.toEntity as the format's single source of truth. ### `app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt:371` — The SEEN-flag retry test runs on the real clock — MailRepositoryImpl.backgroundScope hardcodes Dispatchers.IO so the 2s real delay() cannot be virtualized, making the test cost >=2s wall time per run with only a 3s margin (coVerify timeout 5s) — and the retry give-up cap (SEEN_FLAG_PUSH_MAX_ATTEMPTS = 3) is not tested at all. - severity: **low** · category: `test-gap` · angle: `tests` > On a loaded CI shard the second attempt (first retry fires after a real 2s backoff on Dispatchers.IO) can land past the 5s verify window, flaking the suite; and a regression that removes the attempt cap (making pushSeenFlagInBackground retry forever, hammering the IMAP server every few seconds for each opened cached-unread message and draining battery/connection slots) passes every existing test, since only 'at least 2 attempts' is ever asserted and no test observes that attempts stop at 3.
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: JMR-dev/LibreMail#500