Merge remote-tracking branch 'origin/ci-preflight-api35-api37' into ci-preflight-api35-api37

.github/workflows/ci.yml

@@ -20,8 +20,130 @@ env:
   ANDROID_BUILD_TOOLS: "build-tools;37.0.0"
 
 jobs:
+  # ── Priority-based runner orchestration ──────────────────────────────────────
+  # Runs FIRST (the heavy jobs below all `needs: traffic-control`). It reads THIS
+  # PR's P0–P9 label — P0 = highest priority, P9 = lowest, default P5 when the PR
+  # carries no P-label — and PREEMPTS: it cancels the in-progress / queued CI runs
+  # of strictly-LOWER-priority OTHER open PRs, freeing their runners for this
+  # higher-priority PR. A preempted PR simply re-runs on its next push / autoupdate
+  # rebase.
+  #
+  # Hard safety rules, all enforced in the script below:
+  #   • never cancels a run on main / a push event (filters --event pull_request);
+  #   • never cancels THIS PR's own run (skips self by PR number + run id);
+  #   • never cancels an equal-or-higher-priority PR (only prio > self);
+  #   • only strictly-lower-priority OTHER open PRs' active runs are cancelled.
+  #
+  # It is deliberately NOT a merge-gate check: it is absent from `ci-passed`'s
+  # needs, every API call is guarded, the script always exits 0, and the step is
+  # `continue-on-error` — so a hiccup (API error, missing permission, fork PR)
+  # can never fail or block CI. The heavy jobs only *order* after it via `needs`;
+  # if it were ever skipped/failed they'd be skipped, which `ci-passed` now treats
+  # as a gate failure (fail-safe: blocks merge, never spuriously passes).
+  traffic-control:
+    name: Traffic control (runner priority)
+    runs-on: ubuntu-latest
+    timeout-minutes: 5
+    permissions:
+      actions: write # cancel workflow runs on lower-priority PRs
+      pull-requests: read # read PR P0–P9 labels
+    env:
+      GH_TOKEN: ${{ github.token }}
+      GH_REPO: ${{ github.repository }}
+      SELF_PR: ${{ github.event.pull_request.number }}
+    steps:
+      # No checkout: this job only calls the gh CLI (auto-configured from GH_TOKEN /
+      # GH_REPO), so it needs neither the repo contents nor the default contents:read.
+      - name: Preempt lower-priority PR runs
+        continue-on-error: true # belt-and-suspenders: never let this fail the run
+        run: |
+          # GitHub invokes run steps with `bash -eo pipefail`. Disable errexit so a
+          # single failed API call can't abort the step; we guard every call and
+          # always exit 0. Attacker-influenced values (branch names, labels) are only
+          # ever read via env / gh JSON into shell vars — never interpolated as code.
+          set +e
+
+          if [ "${GITHUB_EVENT_NAME:-}" != "pull_request" ] || [ -z "${SELF_PR:-}" ]; then
+            echo "Not a pull_request event (or no PR number) — nothing to preempt."
+            exit 0
+          fi
+
+          # Effective priority (0–9) of a labels JSON array read on stdin: the
+          # highest-priority (lowest-numbered) P0–P9 label present, else 5.
+          prio_of() {
+            jq -r '[ .[] | .name | select(test("^P[0-9]$")) | ltrimstr("P") | tonumber ]
+                   | if length == 0 then 5 else min end' 2>/dev/null
+          }
+
+          # One snapshot of every open PR (number, head branch, labels).
+          if ! gh pr list --state open --limit 300 \
+                 --json number,headRefName,labels > open_prs.json 2>err.txt; then
+            echo "::warning::Could not list open PRs — skipping preemption. $(cat err.txt 2>/dev/null)"
+            exit 0
+          fi
+
+          self_labels=$(jq -c --argjson pr "$SELF_PR" \
+            '([ .[] | select(.number == $pr) | .labels ] | .[0]) // []' open_prs.json 2>/dev/null)
+          self_prio=$(printf '%s' "${self_labels:-[]}" | prio_of)
+          case "$self_prio" in ''|*[!0-9]*) self_prio=5 ;; esac
+          echo "This PR #$SELF_PR has effective priority P$self_prio (P0 = highest, P9 = lowest)."
+
+          if [ "$self_prio" -ge 9 ]; then
+            echo "P$self_prio is the lowest tier — no strictly-lower-priority PRs to preempt."
+            exit 0
+          fi
+
+          # "number<TAB>head<TAB>prio" for every OTHER open PR.
+          jq -r --argjson self "$SELF_PR" '
+            .[] | select(.number != $self)
+            | [ .number, .headRefName,
+                ([ .labels[] | .name | select(test("^P[0-9]$")) | ltrimstr("P") | tonumber ]
+                  | if length == 0 then 5 else min end) ]
+            | @tsv' open_prs.json 2>/dev/null > others.tsv
+
+          cancelled_total=0
+          while IFS=$'\t' read -r num head prio; do
+            [ -n "${num:-}" ] || continue
+            case "$prio" in ''|*[!0-9]*) prio=5 ;; esac
+
+            if [ "$prio" -le "$self_prio" ]; then
+              echo "· PR #$num (P$prio): equal-or-higher priority — left untouched."
+              continue
+            fi
+
+            echo "· PR #$num (P$prio, head '$head'): strictly lower priority — checking for active CI runs."
+            # Active (non-completed) CI runs on that PR's head branch, PR events only.
+            run_ids=$(gh run list --workflow ci.yml --branch "$head" --event pull_request \
+                        --limit 100 --json databaseId,status,headBranch,event 2>/dev/null \
+                      | jq -r '.[]
+                          | select(.event == "pull_request")
+                          | select(.headBranch != "main")
+                          | select(.status != "completed")
+                          | .databaseId' 2>/dev/null)
+
+            if [ -z "$run_ids" ]; then
+              echo "    no active CI runs."
+              continue
+            fi
+
+            while IFS= read -r run_id; do
+              [ -n "$run_id" ] || continue
+              [ "$run_id" = "${GITHUB_RUN_ID:-}" ] && continue # never cancel our own run
+              if gh run cancel "$run_id" 2>err.txt; then
+                echo "    cancelled run $run_id (freed its runner)."
+                cancelled_total=$((cancelled_total + 1))
+              else
+                echo "::warning::could not cancel run $run_id — likely already finished. $(cat err.txt 2>/dev/null)"
+              fi
+            done <<< "$run_ids"
+          done < others.tsv
+
+          echo "Preemption pass complete — cancelled $cancelled_total lower-priority run(s)."
+          exit 0
+
   debug-build:
     name: Debug build
+    needs: traffic-control # order after runner-priority preemption
     # x86_64: Linux-arm64 runners can't set up this SDK — android-actions/setup-android's sdkmanager
     # fails (exit 1) on the android-37.0 preview platform, and the emulator package has no arm64-Linux
     # build. Build/unit-test results are host-arch-independent anyway (R8/AGP/JVM); real arm64
@@ -58,6 +180,7 @@ jobs:
 
   unit-tests:
     name: Unit tests
+    needs: traffic-control # order after runner-priority preemption
     runs-on: ubuntu-latest
     steps:
       - name: Check out source
@@ -106,6 +229,7 @@ jobs:
 
   static-analysis:
     name: Static analysis
+    needs: traffic-control # order after runner-priority preemption
     runs-on: ubuntu-latest
     steps:
       - name: Check out source
@@ -143,6 +267,7 @@ jobs:
 
   e2e:
     name: E2E
+    needs: traffic-control # order after runner-priority preemption
     runs-on: ubuntu-latest
     strategy:
       fail-fast: false
@@ -253,6 +378,7 @@ jobs:
   # 37 into the main `e2e` matrix and delete this job.
   e2e-preview:
     name: E2E (API 37 preview)
+    needs: traffic-control # order after runner-priority preemption
     runs-on: ubuntu-latest
     timeout-minutes: 35
     env:
@@ -363,11 +489,16 @@ jobs:
   ci-passed:
     name: CI passed
     if: always()
+    # `traffic-control` is intentionally NOT listed here — it is a best-effort
+    # optimizer, not a merge requirement. But because the heavy jobs `needs:` it,
+    # a (should-never-happen) traffic-control failure would mark them 'skipped';
+    # treating 'skipped' as a gate failure below keeps that fail-safe (blocks the
+    # merge rather than letting it through untested).
     needs: [static-analysis, debug-build, unit-tests, e2e, e2e-preview]
     runs-on: ubuntu-latest
     steps:
       - name: Verify every required job succeeded
-        if: ${{ contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') }}
+        if: ${{ contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') || contains(needs.*.result, 'skipped') }}
         run: |
           echo "Required CI jobs did not all succeed:"
           echo "  static-analysis: ${{ needs.static-analysis.result }}"

app/src/main/kotlin/org/libremail/data/sync/MailPruner.kt

@@ -25,6 +25,11 @@ import javax.inject.Singleton
  * Precedence with the #12 backfill is guaranteed two ways: backfill stops paging at the same
  * retention floor this pruner deletes below (their working sets are disjoint), and both jobs share
  * [MailMaintenanceGate] so they never run at once.
+ *
+ * Foreground sync ([MailSyncer]) is aligned the same way in BOTH retention modes so it never
+ * re-inserts what this pruner deletes: it caps its fetch window to the retention count AND drops
+ * anything older than the age cutoff before persisting (#193). The two limits are independent, so
+ * both bounds apply together.
  */
 @Singleton
 class MailPruner @Inject constructor(

app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt

@@ -87,11 +87,19 @@ class MailSyncer @Inject constructor(
     private suspend fun syncFolderHeaders(account: Account, folder: String, notify: Boolean): Result<Int> =
         runCatching {
             val params = connectionFactory.imapParamsFor(account)
+            val policy = accountSettingsRepository.effectiveRetention(settingsRepository, account.id)
             // Never fetch more of the recent window than device-only retention (#13) would keep. Without
             // this, a count limit BELOW the window would make foreground sync re-download the same rows
             // the pruner just trimmed, on every sync — an endless re-download/re-prune fight.
-            val fetched = imapClient.fetchRecent(params, folder, recentWindowFor(account)) // cancellable network I/O
+            val window = policy.countLimit?.let { minOf(FETCH_LIMIT, it) } ?: FETCH_LIMIT
+            val fetched = imapClient.fetchRecent(params, folder, window) // cancellable network I/O
+            // Age-based retention (#193): drop anything older than the age cutoff before persisting. On a
+            // low-traffic mailbox the newest-N can extend PAST the cutoff, so without this a sync re-inserts
+            // rows the age pruner just deleted and the next prune deletes them again — a churn loop. Count/
+            // unlimited modes have a null cutoff and keep the full window, so their behavior is unchanged.
+            val cutoff = policy.ageCutoffMillis(System.currentTimeMillis())
             val entities = fetched.map { it.toEntity(account.id, folder) }
+                .let { mapped -> if (cutoff == null) mapped else mapped.filter { it.timestampMillis >= cutoff } }
 
             // Persist and notify atomically with respect to cancellation: an IDLE renewal that cancels
             // mid-sync must not drop a notification (the rows would then look "already seen" next time).
@@ -104,9 +112,11 @@ class MailSyncer @Inject constructor(
                     entities.filter { it.id !in existingIds && !it.isRead }
                 }
 
-                if (entities.isEmpty()) {
+                if (fetched.isEmpty()) {
                     // An empty recent window means the server folder itself is empty, so nothing (not
-                    // even backfilled history) should remain cached for it.
+                    // even backfilled history) should remain cached for it. Keyed on the raw fetch, not the
+                    // age-filtered set: a folder holding only mail older than the age cutoff is NOT empty on
+                    // the server, so its stale local rows are left to the pruner rather than wiped here.
                     messageDao.deleteSyncedByAccountFolder(account.id, folder)
                 } else {
                     val ids = entities.map { it.id }
@@ -147,16 +157,6 @@ class MailSyncer @Inject constructor(
             fetched.size
         }
 
-    /**
-     * The number of recent headers to fetch: the standard [FETCH_LIMIT], but capped by the account's
-     * effective device-only retention count so foreground sync never re-downloads rows the pruner
-     * would immediately trim. Age-only or unlimited retention leaves the full window in place.
-     */
-    private suspend fun recentWindowFor(account: Account): Int {
-        val policy = accountSettingsRepository.effectiveRetention(settingsRepository, account.id)
-        return policy.countLimit?.let { minOf(FETCH_LIMIT, it) } ?: FETCH_LIMIT
-    }
-
     /**
      * Aggressively pre-caches each not-yet-fetched message's full content (body + attachments) per the
      * user's fetch policy, pausing at low battery regardless of policy — see

app/src/test/kotlin/org/libremail/data/sync/MailSyncerTest.kt

@@ -46,6 +46,9 @@ class MailSyncerTest {
     /** The IMAP client of the most recently built [syncer], for verifying the fetch window size. */
     private lateinit var lastImapClient: ImapClient
 
+    /** The MessageDao of the most recently built [syncer], for verifying what got persisted. */
+    private lateinit var lastMessageDao: MessageDao
+
     /** A syncer whose header sync is a no-op (no server messages) so tests focus on the prefetch step. */
     private fun syncer(
         policy: FetchPolicy,
@@ -54,14 +57,16 @@ class MailSyncerTest {
         accountSettings: AccountSettings = AccountSettings("acct"),
         globalSettings: AppSettings = AppSettings(),
         battery: BatteryStatus = BatteryStatus(percent = 100, isCharging = false),
+        fetched: List<FetchedMessage> = emptyList(),
     ): MailSyncer {
         val accountDao = mockk<AccountDao>()
         coEvery { accountDao.getById("acct") } returns account
         val messageDao = mockk<MessageDao>(relaxed = true)
         coEvery { messageDao.getSyncedIds(any(), any()) } returns emptyList()
         coEvery { messageDao.getUnfetchedIds("acct", "INBOX") } returns listOf("acct:INBOX:1")
+        lastMessageDao = messageDao
         val imapClient = mockk<ImapClient>()
-        coEvery { imapClient.fetchRecent(any(), any(), any()) } returns emptyList()
+        coEvery { imapClient.fetchRecent(any(), any(), any()) } returns fetched
         lastImapClient = imapClient
         val connectionFactory = mockk<MailConnectionFactory>()
         coEvery { connectionFactory.imapParamsFor(any()) } returns mockk<ImapConnectionParams>()
@@ -137,6 +142,30 @@ class MailSyncerTest {
         coVerify { lastImapClient.fetchRecent(any(), "INBOX", 50) }
     }
 
+    @Test
+    fun `age retention does not re-insert fetched messages older than the cutoff`() = runTest {
+        val repo = mockk<MailRepository>(relaxed = true)
+        val now = System.currentTimeMillis()
+        val day = 24L * 60 * 60 * 1000
+        val recentTs = now - 10 * day // well within a 6-month window
+        val oldTs = now - 400 * day // well past a 6-month window — the pruner would delete it
+        val fetched = listOf(
+            FetchedMessage("2", "New", "new@example.org", "recent", recentTs, isRead = true, isFlagged = false),
+            FetchedMessage("1", "Old", "old@example.org", "stale", oldTs, isRead = true, isFlagged = false),
+        )
+
+        syncer(
+            FetchPolicy.ON_DEMAND,
+            repo,
+            accountSettings = AccountSettings("acct", retentionMonths = 6),
+            fetched = fetched,
+        ).syncFolder("acct", "INBOX")
+
+        // Only the in-window message is persisted; the past-cutoff one is never re-inserted, so the age
+        // pruner won't just delete it again next cycle (#193 — no re-download/re-prune churn loop).
+        coVerify { lastMessageDao.insertNew(match { batch -> batch.map { it.timestampMillis } == listOf(recentTs) }) }
+    }
+
     @Test
     fun `WIFI_ONLY prefetches on an unmetered network`() = runTest {
         val repo = mockk<MailRepository>()