diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c28522f..4ad1d7d 100644 --- a/.github/workflows/ci.yml +++ b/.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 + + # "numberheadprio" 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 }}" diff --git a/app/src/main/kotlin/org/libremail/data/sync/MailPruner.kt b/app/src/main/kotlin/org/libremail/data/sync/MailPruner.kt index b922dcf..07a9d8b 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/MailPruner.kt +++ b/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( diff --git a/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt b/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt index b059125..e701ae9 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/MailSyncer.kt +++ b/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 = 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 diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailSyncerTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailSyncerTest.kt index 03f9beb..4e1a75a 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailSyncerTest.kt +++ b/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 = emptyList(), ): MailSyncer { val accountDao = mockk() coEvery { accountDao.getById("acct") } returns account val messageDao = mockk(relaxed = true) coEvery { messageDao.getSyncedIds(any(), any()) } returns emptyList() coEvery { messageDao.getUnfetchedIds("acct", "INBOX") } returns listOf("acct:INBOX:1") + lastMessageDao = messageDao val imapClient = mockk() - coEvery { imapClient.fetchRecent(any(), any(), any()) } returns emptyList() + coEvery { imapClient.fetchRecent(any(), any(), any()) } returns fetched lastImapClient = imapClient val connectionFactory = mockk() coEvery { connectionFactory.imapParamsFor(any()) } returns mockk() @@ -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(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()