From f629091b18a014608cf10b9f9e613e6bb13feb83 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 8 Jul 2026 07:07:35 -0500 Subject: [PATCH 1/2] ci(mergify): Phase 2 - enable batching (batch_size 5) (#410) --- .mergify.yml | 173 ++++++++++++++++++++------------------------------- 1 file changed, 66 insertions(+), 107 deletions(-) diff --git a/.mergify.yml b/.mergify.yml index 6e95b77..a434490 100644 --- a/.mergify.yml +++ b/.mergify.yml @@ -1,85 +1,65 @@ # SPDX-License-Identifier: GPL-3.0-or-later # # ============================================================================ -# Mergify configuration — PHASE 1: serial merge queue (issue #409). +# Mergify configuration — PHASE 2: batched merge queue (issue #410). # # Spec: docs/ci/mergify-integration-spec.md + docs/ci/mergify.yml.proposed # (issues #407 / #408). # Schema: https://docs.mergify.com/configuration/file-format/ -# Verified against the LIVE Mergify docs on 2026-07-07 (file-format, queue -# rules, priority, merge-queue lifecycle/setup/batches, and the -# merge-protections auto-merge pages) — the config format evolves, so this is +# Verified against the LIVE Mergify docs (file-format, queue rules, priority, +# merge-queue batches) on 2026-07-08 — the config format evolves, so this is # not from memory. -# 2026-07-07 CHANGE: auto-queueing migrated OFF the `pull_request_rules` -# queue-action path (which no longer auto-queues — a green matching PR just -# reported "Merge queue is ready — use `@Mergifyio queue`" and sat there) ONTO -# `merge_protections_settings.auto_merge_conditions` (see that block below). -# The old `autoqueue`/queue-action auto path is DEPRECATED and "will stop -# working on 2026-07-16" (docs.mergify.com/merge-queue/rules). This changes only -# the TRIGGER; the queue's merge semantics (below) are untouched. +# +# HISTORY +# Phase 1 (#409; landed #422, proven by #425/#426) ran a SERIAL queue — batch_size 1 +# + max_parallel_checks 1 + queue_conditions == merge_conditions — which kept GitHub's +# "Require branches up to date before merging" checkbox LITERALLY on. It was proven +# end-to-end: mergify[bot] auto-merged #425/#426, and serialised #426 -> #427 by +# updating #427 onto the new `main` (incl. #426) and re-running CI before merging. +# Phase 2 (this file, #410) turns on BATCHING now that the queue is proven. # ============================================================================ # # WHAT THIS DOES -# A SERIAL merge queue that ends the manual serial-bump grind and supersedes the -# hand-rolled "poor-man's merge queue" (autoupdate.yml + ci-trigger.yml + -# traffic-control.yml — all already `disabled_manually`). Mergify updates each -# queued PR onto the latest `main`, re-runs CI, and merges it with a MERGE COMMIT -# when the single required gate — the "CI passed" check — is green. One PR at a -# time, in P0–P9 priority order. +# A BATCHED merge queue. Mergify takes up to `batch_size` queued PRs, builds ONE +# speculative branch = (latest `main` + all the batched PRs), runs CI on that combined +# branch ONCE, and — if green — merges the whole batch (each as a MERGE COMMIT) in +# P0-P9 priority order. That is ~`batch_size`x the throughput of Phase-1 serial (one CI +# cycle merges many PRs, not one) while STILL testing every PR against the latest `main` +# (they all ride the same speculative batch branch). # -# HARD INVARIANTS (do NOT relax without the trilemma decision recorded in the spec): -# * require-up-to-date STAYS ON. This is Phase 1 = batch_size 1 + merge_method: -# merge — the ONLY trilemma combination that keeps GitHub's "Require branches to -# be up to date before merging" LITERALLY enabled AND preserves the merge-commit -# policy. Mergify honours it by updating each PR onto the latest `main` and -# re-running CI before merging ("Updates PRs against the latest main before -# merging" — docs.mergify.com/merge-queue/setup). NO batching: batching would -# require turning that checkbox OFF (docs.mergify.com/merge-queue/batches) and is -# the blocked Phase 2 / issue #410 — explicitly OUT OF SCOPE here. +# THE require-up-to-date SWAP (the one hard change from Phase 1 — do not misread it): +# * Batching is INCOMPATIBLE with GitHub's "Require branches to be up to date before +# merging" (docs.mergify.com/merge-queue/batches): a batch branch is by construction +# "ahead of" its member PRs, so that per-PR linear check cannot pass. It is therefore +# turned OFF in the `main` ruleset (18347032 -> `required_status_checks +# .strict_required_status_checks_policy` = false). The required "CI passed" CHECK +# itself STAYS required — only the "must be up to date" part is dropped. +# * The INVARIANT that option protected — never merge code untested against the latest +# `main` — is NOT lost; it MOVES to Mergify. The speculative batch branch IS +# latest-`main`-plus-the-batch, so a green batch check IS the against-latest-main +# test. This is the sanctioned swap (invariant preserved, enforcement relocated), +# authorised ONLY because Phase 1 proved the queue actually performs that update+re-CI. +# Do NOT drop require-up-to-date for any reason that does NOT relocate the invariant. # * The single required status check stays "CI passed" — the exact `name:` of the -# `ci-passed` job in .github/workflows/ci.yml. NOT "ci-passed". A wrong name means -# PRs queue but never merge. -# * IN-PLACE CHECKS, not speculative draft-PR checks. GitHub's strict -# `required_status_checks` ruleset (require-branches-up-to-date) rejects -# speculative checks outright — Mergify surfaced this as a "Configuration not -# compatible with `required_status_checks` ruleset rule" check on #422. The fix -# (per Mergify: docs.mergify.com/merge-queue/rules) is to make Mergify validate -# each PR IN PLACE, on the real PR branch, which requires ALL THREE of: -# (a) `merge_queue.max_parallel_checks: 1` (below), -# (b) every `queue_rules[].batch_size: 1` (below), and -# (c) `queue_rules.default.queue_conditions` IDENTICAL (same conditions, same -# order) to `queue_rules.default.merge_conditions` — i.e. no "two-step CI" -# where the conditions to ENTER the queue differ from the conditions to -# MERGE. Mergify runs three condition sets, sequentially: -# `merge_protections_settings.auto_merge_conditions` (TRIGGERS auto-queueing) -# → `queue_conditions` (validates a PR's queue ENTRY) → `merge_conditions` -# (validates the MERGE). Omitting `queue_conditions` — as this config first -# did — reads as a two-step-CI mismatch and re-trips the incompatibility -# check, so we keep all three lists identical. Do not let them drift apart. +# `ci-passed` job in .github/workflows/ci.yml. NOT "ci-passed". # --------------------------------------------------------------------------- -# queue_rules — how a queued PR is validated and merged. +# queue_rules — how a batch of queued PRs is validated and merged. # --------------------------------------------------------------------------- queue_rules: - name: default - # Final merge gate. Merge ONLY when the single required context is green (the exact - # same check branch protection requires), the PR targets `main`, is not a draft, has - # no merge conflicts, and is not flagged `broken`. NOTE: branch protection requires 0 - # approvals here (the active repository ruleset sets required_approving_review_count - # = 0), so there is deliberately NO `#approved-reviews-by` condition — adding one - # would wedge the solo-maintainer flow, where nobody can approve their own PR. + # Merge a batch ONLY when the combined batch branch is green on the single required + # context ("CI passed"), every member PR targets `main`, is not a draft, has no merge + # conflict, and is not flagged `broken`. NOTE: the ruleset requires 0 approvals + # (required_approving_review_count = 0), so there is deliberately NO `#approved-reviews-by` + # condition — it would wedge the solo-maintainer flow (nobody can approve their own PR). # - # IN-PLACE CHECKS: `queue_conditions` (what a PR must satisfy to ENTER/stay in the - # queue) MUST be IDENTICAL (same conditions, same order) to `merge_conditions` (what - # it must satisfy to MERGE) below. When those two lists match — plus batch_size 1 and - # max_parallel_checks 1 — Mergify validates each PR IN PLACE on the real PR branch - # instead of running speculative draft-PR checks, which is what GitHub's strict - # `required_status_checks` ruleset (require-branches-up-to-date) demands. Omitting - # `queue_conditions` (as this config originally did) is treated as a "two-step CI" - # mismatch and Mergify flags the ruleset as incompatible. Keep the three lists here — - # `queue_conditions`, `merge_conditions`, and - # `merge_protections_settings.auto_merge_conditions` — all identical; if any diverge, - # Mergify's ruleset-compatibility check fails again. + # queue_conditions (queue ENTRY) are kept IDENTICAL — same conditions, same order — to + # merge_conditions (MERGE). Under Phase 1 this identity was REQUIRED for in-place-checks + # compatibility with the strict ruleset; with require-up-to-date now off, batching uses + # speculative batch checks and the identity is no longer mandatory — but it is kept so + # auto_merge_conditions / queue_conditions / merge_conditions remain one single source of + # truth (no reason for entry and merge gates to differ). Keep all three lists identical. queue_conditions: - base = main - -draft @@ -92,31 +72,33 @@ queue_rules: - -conflict - label != broken - check-success = CI passed - # SERIAL: exactly one PR per merge. No batching (Phase 2 / #410). One merge commit - # per PR, which is what lets require-up-to-date stay literally ON. - batch_size: 1 - # Merge commit — never squash / rebase / fast-forward (repo policy: merges use - # merge commits, never squash). + # BATCHING (Phase 2 / #410): validate up to 5 PRs together on ONE speculative branch, so + # a single ~15-min CI cycle can merge up to 5 PRs instead of 1. `batch_max_wait_time` + # bounds how long Mergify waits to fill a batch before starting CI on a partial one, so a + # lone PR is not left waiting for companions. Requires the require-up-to-date checkbox OFF + # (see the SWAP note in the header). + batch_size: 5 + batch_max_wait_time: 5 min + # Merge commit — never squash / rebase / fast-forward (repo policy: merges use merge + # commits, never squash). merge_method: merge # --------------------------------------------------------------------------- # merge_queue — queue-wide options. # --------------------------------------------------------------------------- merge_queue: - # Validate ONE PR at a time — true serial, no speculative parallel checks. This is the - # strictest, unambiguously require-up-to-date-compatible setting: Mergify updates the - # REAL PR branch onto the latest `main`, runs CI on that branch, and merges on the real - # green "CI passed" — with no speculative temp-branch/real-branch check mismatch to - # reason about. It also caps the expensive, wedge-prone ~15-min E2E matrix at a single - # concurrent run. Raising this (speculative parallelism) is a throughput optimisation to - # weigh alongside the Phase 2 / #410 batching decision — not part of serial Phase 1. + # One batch validated at a time. `batch_size` (above) — not parallelism — is the Phase-2 + # throughput lever: a single batch of up to 5 PRs merges per CI cycle, keeping the + # expensive/wedge-prone ~15-min E2E matrix to ONE concurrent run. Raising this would run + # multiple batches' CI concurrently (more runner load / cost) — a later tuning knob, not + # needed to get the batching win. max_parallel_checks: 1 # --------------------------------------------------------------------------- -# priority_rules — map the repo's P0–P9 labels onto queue priority. +# priority_rules — map the repo's P0-P9 labels onto queue priority. # Higher number merges first (Mergify keywords: low=1000 / medium=2000 / high=3000; -# numeric range 1–10000). P0 is emergency-only and outranks everything. PRs with no P-label -# fall to Mergify's default `medium` (2000). +# numeric range 1-10000). P0 is emergency-only and outranks everything. PRs with no P-label +# fall to Mergify's default `medium` (2000). Priority also orders merges within a batch. # --------------------------------------------------------------------------- priority_rules: - name: p0-emergency @@ -170,37 +152,14 @@ priority_rules: # --------------------------------------------------------------------------- # merge_protections_settings — WHICH PRs are AUTOMATICALLY added to the queue. +# (Unchanged from Phase 1 — this is the auto-queue TRIGGER, orthogonal to batching.) # -# This REPLACES the old `pull_request_rules` `queue` action. That action no longer -# auto-queues in current Mergify: a green, matching PR just reported "Merge queue is -# ready — use `@Mergifyio queue`" and sat there forever (never merged). Automatic -# queueing now lives in `auto_merge_conditions` under `merge_protections_settings`. The -# old `queue_rules[].autoqueue` field (and the queue-action auto path) is DEPRECATED and -# "will stop working on 2026-07-16. Use `auto_merge_conditions` in -# `merge_protections_settings` instead" (docs.mergify.com/merge-queue/rules). +# Automatic queueing lives in `auto_merge_conditions` (the old `pull_request_rules` queue +# action no longer auto-queues — deprecated 2026-07-16). Same audience as before: green on +# "CI passed", targeting `main`, not a draft, no conflicts, not `broken`. A matched PR is +# auto-QUEUED (not merged directly); the batched queue then routes + merges it. # -# `auto_merge_conditions` accepts `true` (auto-queue every mergeable PR) or, as here, -# "a list of conditions to restrict the audience" (docs.mergify.com/configuration/ -# file-format). We give the SAME set the old queue action used, so EXACTLY the same PRs -# auto-queue: green on "CI passed", targeting `main`, not a draft, no conflicts, not -# `broken`. -# -# WHY THIS PRESERVES require-up-to-date: this changes only the TRIGGER (manual → -# automatic). It does NOT touch how the queue validates or merges — batch_size 1, -# merge_method merge, and max_parallel_checks 1 above are unchanged — and those are the -# settings that interact with require-up-to-date (only BATCHING, batch_size > 1, forces -# that checkbox OFF; see the invariants header + docs.mergify.com/merge-queue/batches). -# When a merge queue is configured, a matched PR is auto-QUEUED, not merged directly: -# "Every PR is auto-queued. The merge queue then handles routing and merging" -# (docs.mergify.com/merge-protections/auto-merge) — so it still goes through the serial -# queue, gets updated onto the latest `main`, re-runs CI, and merges on the real green -# "CI passed". Mergify also auto-reads GitHub branch protection (the required "CI passed" -# check + require-up-to-date) and injects it as a merge condition, so the GitHub gate is -# enforced on top of queue_rules.merge_conditions. -# -# This list MUST stay IDENTICAL (same conditions, same order) to -# `queue_rules.default.queue_conditions` and `.merge_conditions` above — see the note -# there and the IN-PLACE CHECKS hard invariant at the top of this file. +# Kept IDENTICAL (same conditions, same order) to queue_conditions / merge_conditions above. # --------------------------------------------------------------------------- merge_protections_settings: auto_merge_conditions: -- 2.47.3 From d0ed168fcb7b967ec74cd22c206aa23213c393c3 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 8 Jul 2026 08:09:12 -0500 Subject: [PATCH 2/2] refactor(data): tighten data-core atomicity, chunking, and dead code (#313) Addresses the below-cut data-core review nits from #313: - SignatureRepository.delete: wrap delete + default-promotion in one SignatureDao @Transaction (deletePromotingDefault) so a crash between them can't leave an account with signatures but no default; log the promotion (PII-free). - SignatureRepository.create: move the count-then-default check-then-act into a SignatureDao @Transaction (insertMakingFirstDefault) so two concurrent first-creates can't both become default. - AccountSettingsRepository.update: route the read-modify-write through an AccountSettingsDao @Transaction (readModifyWrite) so concurrent per-field setters can't clobber each other. - MailRepositoryImpl expunge/move/move-by-role: chunk the unbounded getRoutingByIds/deleteByIds IN(:ids) queries (500/chunk) like MailPruner, removing the latent SQLITE_MAX_VARIABLE_NUMBER crash. - MessageDao.observeSummaries: remove the dead whole-table projection (superseded by Paging #124/#214); migrate test/debug-probe callers to getById or the paged query (which now guards the #51 CursorWindow regression). - AccountDataMigrator: fix stale KDoc (schema is v2 with sortOrder, not v1). - DatabaseEncryption.migrate: also sweep the stale -journal sidecar (journal_mode = DELETE), matching AccountDataMigrator's sweep. Unit tests updated for the repository delegations; instrumented DAO tests cover the new @Transaction behaviour; MailRepositoryImplTest covers the chunk split; DatabaseEncryptionTest covers the -journal sweep. Closes #313 --- .../data/local/AccountSettingsDaoTest.kt | 25 ++++++ .../data/local/DatabaseEncryptionTest.kt | 31 ++++++-- .../DatabaseProvisionerInstrumentedTest.kt | 9 +-- .../data/local/LibreMailDatabaseTest.kt | 32 +++++--- .../libremail/data/local/MessageDaoTest.kt | 9 ++- .../libremail/data/local/SignatureDaoTest.kt | 43 ++++++++++ .../di/DatabaseModuleInstrumentedTest.kt | 7 +- .../data/local/coldopen/ColdOpenCacheProbe.kt | 5 +- .../data/local/AccountDataMigrator.kt | 9 ++- .../data/local/DatabaseEncryption.kt | 6 +- .../data/local/dao/AccountSettingsDao.kt | 12 +++ .../libremail/data/local/dao/MessageDao.kt | 12 --- .../libremail/data/local/dao/SignatureDao.kt | 28 +++++++ .../data/repository/MailRepositoryImpl.kt | 32 ++++++-- .../settings/AccountSettingsRepository.kt | 11 ++- .../data/settings/SignatureRepository.kt | 21 +++-- .../data/repository/MailRepositoryImplTest.kt | 24 ++++++ .../settings/AccountSettingsRepositoryTest.kt | 62 +++++++++------ .../data/settings/SignatureRepositoryTest.kt | 78 ++++++++++++------- config/detekt/detekt.yml | 3 + docs/perf/issue-86-profiling.md | 3 +- 21 files changed, 348 insertions(+), 114 deletions(-) diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountSettingsDaoTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountSettingsDaoTest.kt index 9cf3e6b..2db5c9b 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountSettingsDaoTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountSettingsDaoTest.kt @@ -88,4 +88,29 @@ class AccountSettingsDaoTest { assertEquals(500, stored?.retentionCount) assertEquals(6, stored?.retentionMonths) } + + @Test + fun readModifyWriteAppliesTheTransformToTheStoredRow() = runBlocking { + insertAccount() + dao.upsert(AccountSettingsEntity("acct", signature = "old", notificationsEnabled = false)) + + // The read + transform + write run in one transaction (issue #313); the transform gets the stored + // row and changes one field, so the un-touched fields are carried forward. + dao.readModifyWrite("acct") { stored -> stored!!.copy(signature = "new") } + + val result = dao.get("acct") + assertEquals("new", result?.signature) + assertEquals(false, result?.notificationsEnabled) + } + + @Test + fun readModifyWriteTransformsANullRowForAnUnconfiguredAccount() = runBlocking { + insertAccount() + // No settings row yet: the transform receives null and builds the first row. + dao.readModifyWrite("acct") { stored -> + stored?.copy(signature = "x") ?: AccountSettingsEntity("acct", signature = "seeded") + } + + assertEquals("seeded", dao.get("acct")?.signature) + } } diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/DatabaseEncryptionTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/DatabaseEncryptionTest.kt index 17876f2..a102b8a 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/DatabaseEncryptionTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/DatabaseEncryptionTest.kt @@ -5,7 +5,6 @@ import android.content.Context import androidx.room.Room import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 -import kotlinx.coroutines.flow.first import kotlinx.coroutines.runBlocking import net.zetetic.database.sqlcipher.SQLiteDatabase import net.zetetic.database.sqlcipher.SupportOpenHelperFactory @@ -56,7 +55,7 @@ class DatabaseEncryptionTest { DatabaseEncryption.ensureEncrypted(dbFile, passphrase) assertTrue("file must not read as plaintext once encrypted", DatabaseEncryption.isEncrypted(dbFile)) openEncrypted().apply { - assertEquals(listOf("acct:1"), messageDao().observeSummaries().first().map { it.id }) + assertEquals("acct:1", messageDao().getById("acct:1")?.id) close() } @@ -64,7 +63,7 @@ class DatabaseEncryptionTest { DatabaseEncryption.ensurePlaintext(dbFile, passphrase) assertFalse("file must be plaintext again after decrypt", DatabaseEncryption.isEncrypted(dbFile)) openPlaintext().apply { - assertEquals(listOf("acct:1"), messageDao().observeSummaries().first().map { it.id }) + assertEquals("acct:1", messageDao().getById("acct:1")?.id) close() } } @@ -105,7 +104,7 @@ class DatabaseEncryptionTest { DatabaseEncryption.ensureEncrypted(dbFile, passphrase) assertTrue("the file stays encrypted", DatabaseEncryption.isEncrypted(dbFile)) openEncrypted().apply { - assertEquals(listOf("acct:1"), messageDao().observeSummaries().first().map { it.id }) + assertEquals("acct:1", messageDao().getById("acct:1")?.id) close() } } @@ -122,7 +121,29 @@ class DatabaseEncryptionTest { assertFalse(DatabaseEncryption.isEncrypted(dbFile)) openPlaintext().apply { - assertEquals(listOf("acct:1"), messageDao().observeSummaries().first().map { it.id }) + assertEquals("acct:1", messageDao().getById("acct:1")?.id) + close() + } + } + + @Test + fun conversionSweepsAStaleRollbackJournalSidecar() = runBlocking { + openPlaintext().apply { + messageDao().insertNew(listOf(message("acct:1"))) + close() + } + // A stray `-journal` left next to the file by an interrupted rollback-journal-mode session. The + // conversion runs in journal_mode = DELETE, so `-journal` is the sidecar that can actually linger + // (the pre-existing sweep only removed `-wal`/`-shm`) — issue #313. + val staleJournal = File(dbFile.parentFile, "$dbName-journal") + staleJournal.outputStream().use { it.write(0) } + assertTrue("precondition: a stale journal exists", staleJournal.exists()) + + DatabaseEncryption.ensureEncrypted(dbFile, passphrase) + + assertFalse("the conversion must sweep the stale -journal sidecar", staleJournal.exists()) + openEncrypted().apply { + assertEquals("the data still round-trips", "acct:1", messageDao().getById("acct:1")?.id) close() } } diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/DatabaseProvisionerInstrumentedTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/DatabaseProvisionerInstrumentedTest.kt index ee49502..799ec3d 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/DatabaseProvisionerInstrumentedTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/DatabaseProvisionerInstrumentedTest.kt @@ -16,7 +16,6 @@ import io.mockk.unmockkAll import io.mockk.unmockkObject import io.mockk.verify import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.runBlocking import net.zetetic.database.sqlcipher.SupportOpenHelperFactory @@ -137,7 +136,7 @@ class DatabaseProvisionerInstrumentedTest { // The keyed open the provisioner reported must actually succeed on real SQLCipher (no crash). openEncrypted().apply { - assertEquals(listOf("acct:1"), messageDao().observeSummaries().first().map { it.id }) + assertEquals("acct:1", messageDao().getById("acct:1")?.id) close() } } @@ -164,7 +163,7 @@ class DatabaseProvisionerInstrumentedTest { // whatever ABI / page size this device or emulator image uses. openEncrypted().apply { messageDao().insertNew(listOf(message("acct:1"))) - assertEquals(listOf("acct:1"), messageDao().observeSummaries().first().map { it.id }) + assertEquals("acct:1", messageDao().getById("acct:1")?.id) close() } assertTrue("the fresh cache was created in SQLCipher (encrypted) form", DatabaseEncryption.isEncrypted(dbFile)) @@ -182,7 +181,7 @@ class DatabaseProvisionerInstrumentedTest { assertEquals(CacheOpenMode.Plaintext, mode) assertFalse("the cache must be decrypted so the unkeyed open works", DatabaseEncryption.isEncrypted(dbFile)) openPlaintext().apply { - assertEquals(listOf("acct:1"), messageDao().observeSummaries().first().map { it.id }) + assertEquals("acct:1", messageDao().getById("acct:1")?.id) close() } } @@ -198,7 +197,7 @@ class DatabaseProvisionerInstrumentedTest { assertEquals(CacheOpenMode.Plaintext, mode) assertFalse("a plaintext-with-encryption-off start converts nothing", DatabaseEncryption.isEncrypted(dbFile)) openPlaintext().apply { - assertEquals(listOf("acct:1"), messageDao().observeSummaries().first().map { it.id }) + assertEquals("acct:1", messageDao().getById("acct:1")?.id) close() } } diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt index 31bbde3..99d05a2 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt @@ -2,6 +2,7 @@ package org.libremail.data.local import android.content.Context +import androidx.paging.PagingSource import androidx.room.Room import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 @@ -9,6 +10,7 @@ import kotlinx.coroutines.flow.first import kotlinx.coroutines.runBlocking import org.junit.After import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test @@ -16,6 +18,7 @@ import org.junit.runner.RunWith import org.libremail.data.local.entity.AttachmentEntity import org.libremail.data.local.entity.FolderEntity import org.libremail.data.local.entity.MessageEntity +import org.libremail.data.local.entity.MessageSummary /** * Schema-behavior tests on a fresh in-memory database at the current version. The migration DDL @@ -52,6 +55,12 @@ class LibreMailDatabaseTest { isStarred = false, ) + /** Refreshes a [PagingSource] and returns the first loaded page's ids in order. */ + private suspend fun PagingSource.refreshIds(loadSize: Int = 20): List { + val result = load(PagingSource.LoadParams.Refresh(key = null, loadSize = loadSize, placeholdersEnabled = false)) + return (result as PagingSource.LoadResult.Page).data.map { it.id } + } + @Test fun observeUnreadCountsAggregatesUnreadSyncedRowsPerAccountAndFolder() = runBlocking { val messageDao = db.messageDao() @@ -124,17 +133,17 @@ class LibreMailDatabaseTest { assertEquals(listOf("acct:1"), messageDao.getSyncedIds("acct", "INBOX")) messageDao.deleteSearchRows() - val remaining = messageDao.observeSummaries().first().map { it.id } - assertEquals(listOf("acct:1"), remaining) + assertEquals("the synced inbox row survives", "acct:1", messageDao.getById("acct:1")?.id) + assertNull("the transient search-only row is cleared", messageDao.getById("acct:2")) } @Test - fun observeSummariesReadsRowsWhoseBodiesExceedTheCursorWindow() = runBlocking { + fun pagedSummariesReadRowsWhoseBodiesExceedTheCursorWindow() = runBlocking { val messageDao = db.messageDao() - // Each body is larger than SQLite's shared (~2 MB) CursorWindow. The old list query did - // SELECT * and dragged these bodies through the window, overflowing it with - // "Couldn't read row … from CursorWindow" (issue #51). observeSummaries omits body, so the - // rows stay tiny and read fine. + // Each body is larger than SQLite's shared (~2 MB) CursorWindow. A `SELECT *` list query + // dragged these bodies through the window, overflowing it with "Couldn't read row … from + // CursorWindow" (issue #51). The paged mailbox projection omits body, so the rows stay tiny + // and read fine — asserted against the real production query (issue #124/#214). val hugeBody = "x".repeat(3 * 1024 * 1024) messageDao.insertNew( listOf( @@ -143,7 +152,7 @@ class LibreMailDatabaseTest { ), ) - val ids = messageDao.observeSummaries().first().map { it.id }.toSet() + val ids = messageDao.pagingUnifiedFolderSummaries("INBOX").refreshIds().toSet() assertEquals(setOf("acct:1", "acct:2"), ids) } @@ -194,9 +203,8 @@ class LibreMailDatabaseTest { // Reconciling the inbox must not touch other folders' rows (windowed reconcile; whole-inbox // window since these rows have uid 0). messageDao.deleteSyncedInWindowNotIn("acct", "INBOX", minWindowUid = 0, keepIds = listOf("acct:INBOX:1")) - assertEquals( - setOf("acct:INBOX:1", "acct:Archive:1"), - messageDao.observeSummaries().first().map { it.id }.toSet(), - ) + assertEquals("the kept inbox row survives", "acct:INBOX:1", messageDao.getById("acct:INBOX:1")?.id) + assertNull("the reconciled-away inbox row is deleted", messageDao.getById("acct:INBOX:2")) + assertEquals("the other folder is untouched", "acct:Archive:1", messageDao.getById("acct:Archive:1")?.id) } } diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt index 35d0169..b25d42b 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoTest.kt @@ -6,7 +6,6 @@ import androidx.paging.PagingSource import androidx.room.Room import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 -import kotlinx.coroutines.flow.first import kotlinx.coroutines.runBlocking import org.junit.After import org.junit.Assert.assertEquals @@ -422,7 +421,9 @@ class MessageDaoTest { dao.deleteByIds(listOf("a", "c")) - assertEquals(listOf("b"), dao.observeSummaries().first().map { it.id }) + assertNull("a is deleted", dao.getById("a")) + assertNull("c is deleted", dao.getById("c")) + assertEquals("b survives", "b", dao.getById("b")?.id) } @Test @@ -437,7 +438,9 @@ class MessageDaoTest { dao.deleteByAccount("acct") - assertEquals(listOf("acct2:1"), dao.observeSummaries().first().map { it.id }) + assertNull("the account's INBOX row is deleted", dao.getById("acct:1")) + assertNull("the account's other-folder row is deleted", dao.getById("acct:Archive:1")) + assertEquals("the other account survives", "acct2:1", dao.getById("acct2:1")?.id) } @Test diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/SignatureDaoTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/SignatureDaoTest.kt index d0a513f..a925f35 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/SignatureDaoTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/SignatureDaoTest.kt @@ -150,4 +150,47 @@ class SignatureDaoTest { // clearDefault in setDefault only touches the target account; acct2's default is untouched. assertEquals("b1", dao.getDefault("acct2")?.id) } + + @Test + fun insertMakingFirstDefaultMakesOnlyTheAccountsFirstSignatureDefault() = runBlocking { + insertAccount() + // The passed isDefault is a placeholder; the transaction decides it from the current count (#313). + dao.insertMakingFirstDefault(signature("s-1", "First", isDefault = false)) + dao.insertMakingFirstDefault(signature("s-2", "Second", isDefault = true)) + + assertEquals(true, dao.getById("s-1")?.isDefault) + assertEquals(false, dao.getById("s-2")?.isDefault) + assertEquals("s-1", dao.getDefault("acct")?.id) + } + + @Test + fun deletePromotingDefaultPromotesTheFirstRemainingWhenTheDefaultIsRemoved() = runBlocking { + insertAccount() + dao.upsert(signature("s-default", "Zeta", isDefault = true)) + dao.upsert(signature("s-other", "alpha")) // name-first among the remaining rows + + val promoted = dao.deletePromotingDefault("s-default") + + assertEquals("s-other", promoted) + assertNull("the deleted default is gone", dao.getById("s-default")) + assertEquals("the first remaining becomes default", "s-other", dao.getDefault("acct")?.id) + } + + @Test + fun deletePromotingDefaultPromotesNothingForANonDefaultOrTheLastRow() = runBlocking { + insertAccount() + dao.upsert(signature("s-default", "Default", isDefault = true)) + dao.upsert(signature("s-plain", "Plain")) + + // Deleting a non-default leaves the account's default untouched — nothing to promote. + assertNull(dao.deletePromotingDefault("s-plain")) + assertEquals("s-default", dao.getDefault("acct")?.id) + + // Deleting the last (default) signature has no remaining row to promote. + assertNull(dao.deletePromotingDefault("s-default")) + assertNull(dao.getDefault("acct")) + + // A missing id is a no-op. + assertNull(dao.deletePromotingDefault("absent")) + } } diff --git a/app/src/androidTest/kotlin/org/libremail/di/DatabaseModuleInstrumentedTest.kt b/app/src/androidTest/kotlin/org/libremail/di/DatabaseModuleInstrumentedTest.kt index 2b763a3..95f5301 100644 --- a/app/src/androidTest/kotlin/org/libremail/di/DatabaseModuleInstrumentedTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/di/DatabaseModuleInstrumentedTest.kt @@ -15,7 +15,6 @@ import io.mockk.mockkObject import io.mockk.unmockkAll import io.mockk.verify import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.runBlocking import org.junit.After @@ -138,7 +137,7 @@ class DatabaseModuleInstrumentedTest { // ever opened this genuinely-encrypted file with the plaintext framework helper instead of // SQLCipher's, this would throw (a plaintext driver can't parse SQLCipher ciphertext) rather // than return the seeded row. - assertEquals(listOf("acct:1"), database.messageDao().observeSummaries().first().map { it.id }) + assertEquals("acct:1", database.messageDao().getById("acct:1")?.id) } finally { database.close() } @@ -157,7 +156,7 @@ class DatabaseModuleInstrumentedTest { val database = DatabaseModule.provideDatabase(context, provisioner()) try { database.messageDao().insertNew(listOf(message("acct:1"))) - assertEquals(listOf("acct:1"), database.messageDao().observeSummaries().first().map { it.id }) + assertEquals("acct:1", database.messageDao().getById("acct:1")?.id) } finally { database.close() } @@ -182,7 +181,7 @@ class DatabaseModuleInstrumentedTest { val database = DatabaseModule.provideDatabase(context, provisioner()) try { assertThrows(Throwable::class.java) { - runBlocking { database.messageDao().observeSummaries().first() } + runBlocking { database.messageDao().getById("acct:1") } } } finally { runCatching { database.close() } diff --git a/app/src/debug/kotlin/org/libremail/data/local/coldopen/ColdOpenCacheProbe.kt b/app/src/debug/kotlin/org/libremail/data/local/coldopen/ColdOpenCacheProbe.kt index bd6c4ac..0d22ac8 100644 --- a/app/src/debug/kotlin/org/libremail/data/local/coldopen/ColdOpenCacheProbe.kt +++ b/app/src/debug/kotlin/org/libremail/data/local/coldopen/ColdOpenCacheProbe.kt @@ -10,7 +10,6 @@ import android.os.Bundle import androidx.room.Room import androidx.sqlite.db.SupportSQLiteDatabase import androidx.sqlite.db.SupportSQLiteOpenHelper -import kotlinx.coroutines.flow.first import kotlinx.coroutines.runBlocking import net.zetetic.database.sqlcipher.SupportOpenHelperFactory import org.libremail.data.local.DatabaseEncryption @@ -112,8 +111,8 @@ class ColdOpenCacheProbe : ContentProvider() { ) .build() try { - val ids = runBlocking { database.messageDao().observeSummaries().first().map { it.id } } - if (ids == listOf(EXPECTED_ROW_ID)) OPEN_OK else "$OPEN_ROWS$ids" + val id = runBlocking { database.messageDao().getById(EXPECTED_ROW_ID)?.id } + if (id == EXPECTED_ROW_ID) OPEN_OK else "$OPEN_ROWS$id" } finally { database.close() } diff --git a/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt b/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt index 74769b1..af2423e 100644 --- a/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt +++ b/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt @@ -98,10 +98,11 @@ class AccountDataMigrator @Inject constructor( private val TABLES = listOf("accounts", "credentials", "account_settings", "signatures") /** - * DDL for the account tables in [AccountDatabase] v1, copied verbatim from the exported Room - * schema (`schemas/org.libremail.data.local.AccountDatabase/1.json`). It MUST stay byte-for-byte - * identical to what Room generates for those entities, or Room silently accepts a subtly wrong - * schema (its identity check only compares the hash it writes, not the pre-existing tables). + * DDL for the account tables in [AccountDatabase] v2, copied verbatim from the exported Room + * schema (`schemas/org.libremail.data.local.AccountDatabase/2.json` — v2 added `accounts.sortOrder`, + * issue #164). It MUST stay byte-for-byte identical to what Room generates for those entities, or + * Room silently accepts a subtly wrong schema (its identity check only compares the hash it writes, + * not the pre-existing tables). * `AccountDataMigratorTest.migratorDdlMatchesExportedAccountDatabaseSchema` guards it against the * exported schema; `internal` only so that test can read it. */ diff --git a/app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt b/app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt index 3ede05f..f2826b4 100644 --- a/app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt +++ b/app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt @@ -80,10 +80,14 @@ object DatabaseEncryption { target.close() } - // Swap the converted file into place; drop any stale WAL/SHM sidecars from either file first. + // Swap the converted file into place; drop any stale sidecars from either file first. Both files + // are in rollback-journal mode (journal_mode = DELETE), so the sidecar that can actually linger + // after an interrupted attempt is the `-journal`; the `-wal`/`-shm` deletes are belt-and-suspenders + // for a file left in WAL mode by an older build (mirrors AccountDataMigrator's sweep). listOf(dbFile.name, tmp.name).forEach { base -> File(dir, "$base-wal").delete() File(dir, "$base-shm").delete() + File(dir, "$base-journal").delete() } if (!tmp.renameTo(dbFile)) { tmp.copyTo(dbFile, overwrite = true) diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/AccountSettingsDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/AccountSettingsDao.kt index 4cd8260..952b831 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/AccountSettingsDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/AccountSettingsDao.kt @@ -5,6 +5,7 @@ import androidx.room.Dao import androidx.room.Insert import androidx.room.OnConflictStrategy import androidx.room.Query +import androidx.room.Transaction import kotlinx.coroutines.flow.Flow import org.libremail.data.local.entity.AccountSettingsEntity @@ -18,4 +19,15 @@ interface AccountSettingsDao { @Insert(onConflict = OnConflictStrategy.REPLACE) suspend fun upsert(settings: AccountSettingsEntity) + + /** + * Reads [accountId]'s row (or null when absent), applies [transform], and writes the result — all in + * one transaction so a per-field setter's read-modify-write can't interleave with a concurrent + * setter and clobber the other field (issue #313). [transform] receives the stored entity, or null + * when no row exists yet. + */ + @Transaction + suspend fun readModifyWrite(accountId: String, transform: (AccountSettingsEntity?) -> AccountSettingsEntity) { + upsert(transform(get(accountId))) + } } diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt index 97546cb..e04eaa9 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt @@ -15,18 +15,6 @@ import org.libremail.data.local.entity.MessageSummary @Dao interface MessageDao { - /** - * Mailbox-list projection ordered newest-first. Deliberately omits the large `body`/`isHtml` - * columns: the list observes every cached message at once, and pulling full bodies through - * SQLite's shared ~2 MB CursorWindow overflows it once enough large bodies are cached - * (issue #51). Bodies are loaded lazily per-message via [getById] when a message is opened. - */ - @Query( - "SELECT id, accountId, sender, senderEmail, subject, snippet, timestampMillis, " + - "isRead, isStarred, folder, inInbox, bodyFetched FROM messages ORDER BY timestampMillis DESC", - ) - fun observeSummaries(): Flow> - /** * Paged unified-inbox projection: folder-synced rows of [folder] across every account, * newest-first, as a Paging 3 [PagingSource] (issue #124). Room loads only the requested window diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/SignatureDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/SignatureDao.kt index ddf12c9..3568acb 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/SignatureDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/SignatureDao.kt @@ -44,4 +44,32 @@ interface SignatureDao { clearDefault(accountId) markDefault(id) } + + /** + * Inserts [signature], making it the account's default when it is the account's first — the count + * and the insert run in one transaction so two concurrent first-creates can't both read "count 0" + * and both become default (issue #313). [signature]'s own `isDefault` is ignored: this method + * decides it from the current count. + */ + @Transaction + suspend fun insertMakingFirstDefault(signature: SignatureEntity) { + upsert(signature.copy(isDefault = countForAccount(signature.accountId) == 0)) + } + + /** + * Deletes [id] and, when it was the account's default, promotes the account's first remaining + * signature — both in one transaction so a crash between the delete and the promote can't leave an + * account with signatures but no default (issue #313). No-op when [id] is absent. Returns the id of + * the signature promoted to default, or null when nothing was promoted (id absent, the deleted row + * wasn't the default, or no signatures remain). + */ + @Transaction + suspend fun deletePromotingDefault(id: String): String? { + val existing = getById(id) ?: return null + delete(id) + if (!existing.isDefault) return null + val promoted = firstForAccount(existing.accountId) ?: return null + markDefault(promoted.id) + return promoted.id + } } diff --git a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt index ca8a48f..5d47286 100644 --- a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt +++ b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt @@ -337,16 +337,16 @@ class MailRepositoryImpl @Inject constructor( moveByRole(ids, FolderRole.TRASH, fallbackExpunge = true) override suspend fun expunge(ids: List): Result = runCatching { - val routings = messageDao.getRoutingByIds(ids) - messageDao.deleteByIds(ids) // optimistic + val routings = messageDao.getRoutingByIdsChunked(ids) + messageDao.deleteByIdsChunked(ids) // optimistic forEachAccountFolder(routings) { params, folder, group -> imapClient.deleteMessages(params, folder, group.map { uidOf(it.id) }) } } override suspend fun moveToFolder(ids: List, destFolderFullName: String): Result = runCatching { - val routings = messageDao.getRoutingByIds(ids) - messageDao.deleteByIds(ids) // optimistic + val routings = messageDao.getRoutingByIdsChunked(ids) + messageDao.deleteByIdsChunked(ids) // optimistic forEachAccountFolder(routings) { params, folder, group -> if (folder != destFolderFullName) { imapClient.moveMessages(params, folder, group.map { uidOf(it.id) }, destFolderFullName) @@ -393,8 +393,8 @@ class MailRepositoryImpl @Inject constructor( */ private suspend fun moveByRole(ids: List, role: FolderRole, fallbackExpunge: Boolean): Result = runCatching { - val routings = messageDao.getRoutingByIds(ids) - messageDao.deleteByIds(ids) // optimistic + val routings = messageDao.getRoutingByIdsChunked(ids) + messageDao.deleteByIdsChunked(ids) // optimistic val destByAccount = routings.map { it.accountId }.distinct() .associateWith { resolveRoleFolder(it, role) } forEachAccountFolder(routings) { params, folder, group -> @@ -556,6 +556,12 @@ private const val NANOS_PER_MS = 1_000_000L /** Rows per page for the unified inbox (issue #124) — a page is a few screenfuls of message rows. */ private const val MAILBOX_PAGE_SIZE = 40 +/** + * Ids per `IN (:ids)` query in the batch move/delete/expunge paths, kept under SQLite's 999 + * host-parameter limit on older Android (matches [org.libremail.data.sync.MailPruner]'s DELETE chunk). + */ +private const val SQL_IN_CHUNK = 500 + /** Attempts for the background best-effort SEEN-flag push before giving up silently (issue #148). */ private const val SEEN_FLAG_PUSH_MAX_ATTEMPTS = 3 @@ -565,6 +571,20 @@ private const val SEEN_FLAG_RETRY_BACKOFF_MS = 2_000L /** Message id is ":"; the uid is the trailing segment. */ private fun uidOf(id: String): String = id.substringAfterLast(':') +/** + * [MessageDao.getRoutingByIds] over an arbitrarily large [ids] list, chunked so the expanded `IN (:ids)` + * never exceeds SQLite's host-parameter limit (999 on older Android). The batch move/delete/expunge + * callers are bounded by the multi-select cap today, but chunking removes the latent + * `SQLITE_MAX_VARIABLE_NUMBER` crash the same way [org.libremail.data.sync.MailPruner] does (issue #313). + */ +private suspend fun MessageDao.getRoutingByIdsChunked(ids: List): List = + ids.chunked(SQL_IN_CHUNK).flatMap { getRoutingByIds(it) } + +/** [MessageDao.deleteByIds] chunked under SQLite's host-parameter limit (see [getRoutingByIdsChunked]). */ +private suspend fun MessageDao.deleteByIdsChunked(ids: List) { + ids.chunked(SQL_IN_CHUNK).forEach { deleteByIds(it) } +} + /** * Builds the SQL `LIKE` pattern the paged-search DAO queries take (issue #214), preserving the old * `matchesSearch` literal-substring semantics: escape the LIKE metacharacters (`\ % _`) — the `\` diff --git a/app/src/main/kotlin/org/libremail/data/settings/AccountSettingsRepository.kt b/app/src/main/kotlin/org/libremail/data/settings/AccountSettingsRepository.kt index 53279fe..14239e3 100644 --- a/app/src/main/kotlin/org/libremail/data/settings/AccountSettingsRepository.kt +++ b/app/src/main/kotlin/org/libremail/data/settings/AccountSettingsRepository.kt @@ -49,7 +49,14 @@ class AccountSettingsRepository @Inject constructor(private val dao: AccountSett it.copy(retentionMonths = months?.coerceAtLeast(0)) } - private suspend inline fun update(accountId: String, transform: (AccountSettings) -> AccountSettings) { - dao.upsert(transform(get(accountId)).toEntity()) + /** + * Read-modify-writes an account's settings row through the DAO's single-transaction helper so a + * concurrent per-field setter can't clobber the read-modify-write (issue #313). A missing row is + * transformed from the account's defaults, preserving the "not configured yet = defaults" contract. + */ + private suspend fun update(accountId: String, transform: (AccountSettings) -> AccountSettings) { + dao.readModifyWrite(accountId) { stored -> + transform(stored?.toDomain() ?: AccountSettings(accountId)).toEntity() + } } } diff --git a/app/src/main/kotlin/org/libremail/data/settings/SignatureRepository.kt b/app/src/main/kotlin/org/libremail/data/settings/SignatureRepository.kt index 5bc3955..22a396e 100644 --- a/app/src/main/kotlin/org/libremail/data/settings/SignatureRepository.kt +++ b/app/src/main/kotlin/org/libremail/data/settings/SignatureRepository.kt @@ -6,6 +6,7 @@ import kotlinx.coroutines.flow.map import org.libremail.data.local.dao.SignatureDao import org.libremail.data.local.entity.SignatureEntity import org.libremail.domain.model.Signature +import org.libremail.reporting.AppLog import java.util.UUID import javax.inject.Inject import javax.inject.Singleton @@ -28,8 +29,9 @@ class SignatureRepository @Inject constructor(private val dao: SignatureDao) { /** Creates a signature; makes it the default when it is the account's first. Returns its id. */ suspend fun create(accountId: String, name: String, html: String): String { val id = UUID.randomUUID().toString() - val isFirst = dao.countForAccount(accountId) == 0 - dao.upsert(SignatureEntity(id, accountId, name, html, isDefault = isFirst)) + // The count-then-default decision runs atomically in the DAO so two concurrent first-creates + // can't both become default (issue #313); the isDefault passed here is a placeholder. + dao.insertMakingFirstDefault(SignatureEntity(id, accountId, name, html, isDefault = false)) return id } @@ -39,15 +41,20 @@ class SignatureRepository @Inject constructor(private val dao: SignatureDao) { } suspend fun delete(id: String) { - val existing = dao.getById(id) ?: return - dao.delete(id) - // If we removed the default, promote the account's first remaining signature. - if (existing.isDefault) { - dao.firstForAccount(existing.accountId)?.let { dao.markDefault(it.id) } + // Delete + promote-a-new-default run in one DAO transaction so a crash between them can't strand + // the account with signatures but no default (issue #313). + val promotedId = dao.deletePromotingDefault(id) + if (promotedId != null) { + // PII-free: no signature content, name, or account address — just the state transition. + AppLog.i(TAG, "promoted a replacement default signature after deleting the previous default") } } suspend fun setDefault(accountId: String, id: String) = dao.setDefault(accountId, id) private fun SignatureEntity.toDomain() = Signature(id, accountId, name, contentHtml, isDefault) + + private companion object { + const val TAG = "LibreMailSignatures" + } } diff --git a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt index 097774a..bca3438 100644 --- a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt +++ b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt @@ -445,6 +445,30 @@ class MailRepositoryImplTest { coVerify { imapClient.deleteMessages(any(), "Trash", listOf("3")) } } + @Test + fun `expunge chunks the id queries under SQLite's host-parameter limit`() = runTest { + // 501 ids force a split: an unchunked IN (:ids) would bind 501 host parameters, latent-crashing + // near SQLite's 999 limit on older Android (issue #313). Chunked at 500 -> a 500 + 1 split. + val ids = (1..501).map { "acct:INBOX:$it" } + coEvery { messageDao.getRoutingByIds(any()) } coAnswers { + firstArg>().map { messageRouting(it, "INBOX") } + } + coEvery { messageDao.deleteByIds(any()) } just Runs + coEvery { accountDao.getById("acct") } returns accountEntity() + coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() + + val result = repository.expunge(ids) + + assertTrue(result.isSuccess) + // Both the routing read and the optimistic delete run one query per <=500-id chunk. + coVerify(exactly = 1) { messageDao.getRoutingByIds(match { it.size == 500 }) } + coVerify(exactly = 1) { messageDao.getRoutingByIds(match { it.size == 1 }) } + coVerify(exactly = 1) { messageDao.deleteByIds(match { it.size == 500 }) } + coVerify(exactly = 1) { messageDao.deleteByIds(match { it.size == 1 }) } + // Chunking is only a DB concern: all 501 UIDs still reach the server EXPUNGE in one grouped call. + coVerify { imapClient.deleteMessages(any(), "INBOX", match { it.size == 501 }) } + } + @Test fun `moveToFolder moves messages to the chosen destination`() = runTest { val id = "acct:INBOX:11" diff --git a/app/src/test/kotlin/org/libremail/data/settings/AccountSettingsRepositoryTest.kt b/app/src/test/kotlin/org/libremail/data/settings/AccountSettingsRepositoryTest.kt index d43d281..b076249 100644 --- a/app/src/test/kotlin/org/libremail/data/settings/AccountSettingsRepositoryTest.kt +++ b/app/src/test/kotlin/org/libremail/data/settings/AccountSettingsRepositoryTest.kt @@ -2,6 +2,7 @@ package org.libremail.data.settings import app.cash.turbine.test +import io.mockk.CapturingSlot import io.mockk.Runs import io.mockk.coEvery import io.mockk.coVerify @@ -24,6 +25,20 @@ class AccountSettingsRepositoryTest { private val dao = mockk() private val repository = AccountSettingsRepository(dao) + /** + * Stubs the DAO's single-transaction read-modify-write (issue #313) to apply the repository's + * transform against [current] (the stored row, or null) and capture the entity it would persist — so + * these setter tests still assert the transformed row directly, while AccountSettingsDaoTest covers + * the real transaction. + */ + private fun captureUpdate(current: AccountSettingsEntity?): CapturingSlot { + val saved = slot() + coEvery { dao.readModifyWrite(any(), any()) } coAnswers { + saved.captured = secondArg<(AccountSettingsEntity?) -> AccountSettingsEntity>().invoke(current) + } + return saved + } + @Test fun `get returns defaults when no row exists`() = runTest { coEvery { dao.get("acct") } returns null @@ -38,10 +53,9 @@ class AccountSettingsRepositoryTest { @Test fun `setSignature reads, modifies, and writes the row`() = runTest { - coEvery { dao.get("acct") } returns - AccountSettingsEntity("acct", signature = "old", signatureEnabled = true, notificationsEnabled = false) - val saved = slot() - coEvery { dao.upsert(capture(saved)) } just Runs + val saved = captureUpdate( + AccountSettingsEntity("acct", signature = "old", signatureEnabled = true, notificationsEnabled = false), + ) repository.setSignature("acct", "new") @@ -101,10 +115,9 @@ class AccountSettingsRepositoryTest { @Test fun `setSignatureEnabled reads, modifies, and writes the row`() = runTest { - coEvery { dao.get("acct") } returns - AccountSettingsEntity("acct", signature = "keep", signatureEnabled = true, notificationsEnabled = true) - val saved = slot() - coEvery { dao.upsert(capture(saved)) } just Runs + val saved = captureUpdate( + AccountSettingsEntity("acct", signature = "keep", signatureEnabled = true, notificationsEnabled = true), + ) repository.setSignatureEnabled("acct", false) @@ -115,9 +128,7 @@ class AccountSettingsRepositoryTest { @Test fun `setNotificationsEnabled reads, modifies, and writes the row`() = runTest { - coEvery { dao.get("acct") } returns AccountSettingsEntity("acct", notificationsEnabled = true) - val saved = slot() - coEvery { dao.upsert(capture(saved)) } just Runs + val saved = captureUpdate(AccountSettingsEntity("acct", notificationsEnabled = true)) repository.setNotificationsEnabled("acct", false) @@ -126,9 +137,7 @@ class AccountSettingsRepositoryTest { @Test fun `setRetentionCount clamps a negative override to zero`() = runTest { - coEvery { dao.get("acct") } returns AccountSettingsEntity("acct") - val saved = slot() - coEvery { dao.upsert(capture(saved)) } just Runs + val saved = captureUpdate(AccountSettingsEntity("acct")) repository.setRetentionCount("acct", -5) @@ -137,9 +146,7 @@ class AccountSettingsRepositoryTest { @Test fun `setRetentionCount preserves null as inherit-the-global-default`() = runTest { - coEvery { dao.get("acct") } returns AccountSettingsEntity("acct", retentionCount = 10) - val saved = slot() - coEvery { dao.upsert(capture(saved)) } just Runs + val saved = captureUpdate(AccountSettingsEntity("acct", retentionCount = 10)) repository.setRetentionCount("acct", null) @@ -148,9 +155,7 @@ class AccountSettingsRepositoryTest { @Test fun `setRetentionMonths clamps a negative override to zero`() = runTest { - coEvery { dao.get("acct") } returns AccountSettingsEntity("acct") - val saved = slot() - coEvery { dao.upsert(capture(saved)) } just Runs + val saved = captureUpdate(AccountSettingsEntity("acct")) repository.setRetentionMonths("acct", -3) @@ -159,12 +164,23 @@ class AccountSettingsRepositoryTest { @Test fun `setRetentionMonths keeps a positive override as given`() = runTest { - coEvery { dao.get("acct") } returns AccountSettingsEntity("acct") - val saved = slot() - coEvery { dao.upsert(capture(saved)) } just Runs + val saved = captureUpdate(AccountSettingsEntity("acct")) repository.setRetentionMonths("acct", 6) assertEquals(6, saved.captured.retentionMonths) } + + @Test + fun `a setter transforms the account defaults when no row exists yet`() = runTest { + // The DAO hands the transform a null stored row for a never-configured account; the repository + // must transform from the account's defaults so the "not configured = defaults" contract holds. + val saved = captureUpdate(current = null) + + repository.setSignature("acct", "first") + + assertEquals("acct", saved.captured.accountId) + assertEquals("first", saved.captured.signature) + assertTrue(saved.captured.signatureEnabled, "defaults carry through the transform") + } } diff --git a/app/src/test/kotlin/org/libremail/data/settings/SignatureRepositoryTest.kt b/app/src/test/kotlin/org/libremail/data/settings/SignatureRepositoryTest.kt index 3430e18..58b0ea0 100644 --- a/app/src/test/kotlin/org/libremail/data/settings/SignatureRepositoryTest.kt +++ b/app/src/test/kotlin/org/libremail/data/settings/SignatureRepositoryTest.kt @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.data.settings +import android.util.Log import app.cash.turbine.test import io.mockk.Runs import io.mockk.coEvery @@ -8,12 +9,18 @@ import io.mockk.coVerify import io.mockk.every import io.mockk.just import io.mockk.mockk +import io.mockk.mockkStatic import io.mockk.slot +import io.mockk.unmockkAll import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Before import org.junit.Test import org.libremail.data.local.dao.SignatureDao import org.libremail.data.local.entity.SignatureEntity +import org.libremail.reporting.AppLog +import org.libremail.reporting.RingLogBuffer import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertNull @@ -24,51 +31,70 @@ class SignatureRepositoryTest { private val dao = mockk(relaxed = true) private val repository = SignatureRepository(dao) + // delete() breadcrumbs a promotion via AppLog; android.util.Log is an unmocked stub in plain JVM + // tests, so mock it class-wide (mirrors MailRepositoryImplTest). + @Before + fun setUp() { + mockkStatic(Log::class) + every { Log.i(any(), any()) } returns 0 + } + + @After + fun tearDown() = unmockkAll() + private fun entity(id: String, isDefault: Boolean) = SignatureEntity(id, accountId = "acct", name = "N", contentHtml = "

x

", isDefault = isDefault) @Test - fun `the first signature for an account becomes its default`() = runTest { - coEvery { dao.countForAccount("acct") } returns 0 + fun `create routes through the atomic first-default insert with the given fields`() = runTest { + // The first-becomes-default decision now lives in the DAO transaction (issue #313); the repository + // just forwards the new row (isDefault a placeholder) and returns its generated id. val saved = slot() - coEvery { dao.upsert(capture(saved)) } just Runs + coEvery { dao.insertMakingFirstDefault(capture(saved)) } just Runs - repository.create("acct", "Work", "

hi

") + val id = repository.create("acct", "Work", "

hi

") - assertTrue(saved.captured.isDefault) + assertEquals("acct", saved.captured.accountId) + assertEquals("Work", saved.captured.name) + assertEquals("

hi

", saved.captured.contentHtml) + assertEquals(id, saved.captured.id) + assertFalse(saved.captured.isDefault, "the DAO decides the default flag, not the repository") } @Test - fun `later signatures are not made default`() = runTest { - coEvery { dao.countForAccount("acct") } returns 2 - val saved = slot() - coEvery { dao.upsert(capture(saved)) } just Runs - - repository.create("acct", "Personal", "

hey

") - - assertFalse(saved.captured.isDefault) - } - - @Test - fun `deleting the default promotes the first remaining signature`() = runTest { - coEvery { dao.getById("s1") } returns entity("s1", isDefault = true) - coEvery { dao.firstForAccount("acct") } returns entity("s2", isDefault = false) + fun `delete routes through the atomic delete-and-promote`() = runTest { + coEvery { dao.deletePromotingDefault("s1") } returns null repository.delete("s1") - coVerify { dao.delete("s1") } - coVerify { dao.markDefault("s2") } + coVerify { dao.deletePromotingDefault("s1") } } @Test - fun `deleting a non-default signature promotes nothing`() = runTest { - coEvery { dao.getById("s2") } returns entity("s2", isDefault = false) + fun `deleting a default that promotes a replacement logs a breadcrumb`() = runTest { + val buffer = RingLogBuffer() + AppLog.install(buffer) + coEvery { dao.deletePromotingDefault("s1") } returns "s2" + + repository.delete("s1") + + assertTrue( + buffer.snapshot().any { it.message.contains("promoted a replacement default signature") }, + "the promotion is recorded for a debug report", + ) + } + + @Test + fun `deleting without a promotion logs nothing`() = runTest { + val buffer = RingLogBuffer() + AppLog.install(buffer) + coEvery { dao.deletePromotingDefault("s2") } returns null repository.delete("s2") - coVerify { dao.delete("s2") } - coVerify(exactly = 0) { dao.firstForAccount(any()) } - coVerify(exactly = 0) { dao.markDefault(any()) } + assertFalse( + buffer.snapshot().any { it.message.contains("promoted a replacement default") }, + ) } @Test diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index f190f3c..4329cf0 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -73,6 +73,9 @@ style: # Reader-path perf logging (issue #358): the repository's openMessage and the reader ViewModel # log via AppLog, so their unit tests mockkStatic(Log) too. - '**/data/repository/MailRepositoryImplTest.kt' + # SignatureRepository.delete breadcrumbs a default-promotion via AppLog (issue #313), so its unit + # test mockkStatic(Log) — it does not bypass the facade. + - '**/data/settings/SignatureRepositoryTest.kt' - '**/data/repository/MailRepositoryImplCoverageTest.kt' # Account-add breadcrumb (issue #403): addImapAccount/addOutlookAccount log via AppLog, so this # suite mockkStatic(Log) so the calls don't crash on the throwing JVM stub. diff --git a/docs/perf/issue-86-profiling.md b/docs/perf/issue-86-profiling.md index e9edc15..1b38c85 100644 --- a/docs/perf/issue-86-profiling.md +++ b/docs/perf/issue-86-profiling.md @@ -110,7 +110,8 @@ UNIFIED folder-only : SCAN TABLE messages USING INDEX index_messages_timestampMi search (`matchesSearch`) over the small folder-scoped set — never the whole cache. `StateFlow`'s built-in equality de-dup means an unrelated write now costs one cheap scoped re-query and no recomposition. -- `observeSummaries()` is retained as the #51 CursorWindow regression-guard target in the DB tests. +- The #51 CursorWindow regression guard now targets the paged `pagingUnifiedFolderSummaries()` + projection in the DB tests; the superseded whole-table `observeSummaries()` was removed (issue #313). ## Deferred follow-ups -- 2.47.3