diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt index 0dfaf99..d5a2f1a 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt @@ -190,16 +190,22 @@ class AccountRepository @Inject constructor( * loses only its cursor, because the account it was reconciled against is * gone. * + * ⚠️ On a re-authentication this also has to see the account's *own* lists, + * not just detached ones — the re-auth path walks the same picker, so a + * lookup that missed them would insert a second copy of every list the + * account already syncs. On a create the id was minted a statement earlier, + * so nothing is attached to it yet and only detached rows can match. + * * @return the ids of the lists this call *created*, which are the only ones * a rollback may delete. */ private fun attach(accountId: Long, selected: Set): List { - val orphans = database.taskLists().orphaned().associateBy { it.href } + val known = database.taskLists().attachable(accountId).associateBy { it.href } val inserted = mutableListOf() selected.forEach { collection -> val href = collection.url.toString() - val orphan = orphans[href] - if (orphan == null) { + val current = known[href] + if (current == null) { inserted += database.taskLists().insert( TaskListEntity( name = collection.displayName ?: collection.url.pathSegments @@ -212,14 +218,17 @@ class AccountRepository @Inject constructor( ), ) } else { + // Changing hands, as opposed to re-authenticating the account + // that already owns it: a cursor from the previous owner says + // nothing about this one, while the current owner's is still + // good and throwing it away costs a full reconciliation. + val changingHands = current.accountId != accountId database.taskLists().update( - orphan.copy( + current.copy( accountId = accountId, isReadOnly = collection.readOnly, - // A cursor from the account that used to own this list - // says nothing about the one that owns it now. - syncToken = null, - ctag = null, + syncToken = current.syncToken.takeUnless { changingHands }, + ctag = current.ctag.takeUnless { changingHands }, ), ) } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncCadenceStore.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncCadenceStore.kt index 2b70c6f..135de79 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncCadenceStore.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncCadenceStore.kt @@ -31,7 +31,14 @@ class SyncCadenceStore @Inject constructor( @SyncStateDataStore private val dataStore: DataStore, ) { - /** Last full reconciliation per list id. */ + /** + * Last *scheduled* full reconciliation per list id. + * + * ⚠️ Not "the last time a full listing was read". A collection whose server + * has no `sync-collection` support reads one on every run, and recording + * each would keep this permanently fresh — so nothing hung off the periodic + * mark would ever come due again. + */ suspend fun lastFullSync(): Map = dataStore.data.first()[KEY].orEmpty().mapNotNull { entry -> val separator = entry.lastIndexOf(SEPARATOR) diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncEngine.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncEngine.kt index 5998cd6..0348a6c 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncEngine.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncEngine.kt @@ -147,22 +147,32 @@ class SyncEngine @Inject constructor( val now = kotlin.time.Clock.System.now() val lastFull = cadence.lastFullSync() + // Never reconciled, or the token has been trusted long enough. + val due = lists.associate { list -> + val since = lastFull[list.id] + list.id to (since == null || now - since >= SyncCadenceStore.FULL_RECONCILIATION_INTERVAL) + } + val reports = lists.map { list -> val url = list.href?.toHttpUrlOrNull() ?: return@map SyncReport(list.id, list.name, failure = "list has no collection URL") - val since = lastFull[list.id] syncer.sync( list = list, remote = remoteFor(url), quarantine = counts, - // Never reconciled, or the token has been trusted long enough. - fullReconciliationDue = since == null || - now - since >= SyncCadenceStore.FULL_RECONCILIATION_INTERVAL, + fullReconciliationDue = due[list.id] == true, ) } + // ⚠️ Only the runs that were *due*. A server without `sync-collection` + // reconciles in full every single time, so recording each one kept the + // clock permanently fresh and `fullReconciliationDue` permanently false + // — which costs nothing on that path, since the cursor is null anyway, + // but silently disables everything else hung off the periodic mark. The + // download-side quarantine probe is the one that matters: for exactly + // those servers it would never have fired. cadence.record( - reports.filter { it.reconciledInFull && it.failure == null } + reports.filter { it.reconciledInFull && it.failure == null && due[it.listId] == true } .associate { it.listId to now }, ) diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskListDao.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskListDao.kt index 4e719b1..b0d8058 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskListDao.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskListDao.kt @@ -84,16 +84,25 @@ interface TaskListDao { fun setSyncToken(listId: Long, token: String?) /** - * Lists an account left behind: detached, but still naming a collection. + * Lists a collection URL may be pointed at instead of inserting a new row: + * the ones [accountId] already holds, and the ones a previous removal left + * detached. * - * ⚠️ `remove()` deliberately leaves the lists as device-only ones, so - * re-adding the same account must *re-attach* these rather than insert a - * second copy of every collection — the user would otherwise get their old - * "Personal" full of tasks beside a freshly synced "Personal" holding the - * same tasks from the server. + * ⚠️ Both halves matter, and each is a duplicate bug on its own. + * `remove()` deliberately leaves lists as device-only ones, so re-adding the + * account must re-attach them or the user gets their old "Personal" full of + * tasks beside a freshly synced "Personal" holding the same tasks from the + * server. And re-*authenticating* walks the same picker, so a lookup that + * only saw detached rows would insert a second copy of every list the + * account already syncs — there is no unique index on `href` to catch it. */ - @Query("SELECT * FROM task_lists WHERE account_id IS NULL AND href IS NOT NULL") - fun orphaned(): List + @Query( + """ + SELECT * FROM task_lists + WHERE href IS NOT NULL AND (account_id IS NULL OR account_id = :accountId) + """ + ) + fun attachable(accountId: Long): List /** The synced collections of one account, in the order sync walks them. */ @Query("SELECT * FROM task_lists WHERE account_id = :accountId AND is_synced = 1 ORDER BY id") diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt index 004da5d..bbccd79 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt @@ -293,7 +293,18 @@ class AddAccountViewModel @Inject constructor( // a gateway that keeps saying "pending" would have it poll forever. repeat(MAX_POLL_ATTEMPTS) { delay(POLL_INTERVAL_MILLIS) - when (val result = gateway.pollLoginFlow(flow)) { + // ⚠️ The *call*, not just what follows it. `pollLoginFlow` is a + // blocking execute() inside withContext, and withContext throws + // on return if the job was cancelled meanwhile — so a back + // gesture landing in that window discarded a 200 the server had + // already answered, and with it the only copy of a password it + // had already minted and deleted its flow row for. The persisted + // flow cannot reclaim that one either: the row is gone, so a + // later poll can only report expiry. Bounded by the gateway's + // own per-call budget, and the delay above stays cancellable, so + // the loop still stops promptly. + val result = withContext(NonCancellable) { gateway.pollLoginFlow(flow) } + when (result) { is NextcloudLoginFlow.PollResult.Approved -> { // ⚠️ Uncancellable. The 200 that carried this password is // the only one there will ever be — the server deletes the @@ -302,10 +313,18 @@ class AddAccountViewModel @Inject constructor( // credential with nothing left holding it. Recording it is // what makes handing it back possible. withContext(NonCancellable) { - // Spent: the server deleted the row before answering, - // so a persisted token is now a thing that would be - // polled again for nothing. - pendingFlow.forget() + // ⚠️ Forgotten *here*, and nowhere else. This is the + // one point where the token stops being worth + // anything: the server deleted the flow row before + // answering, so there is nothing left to collect. + // Every other way out of this step — start over, the + // back arrow, a failure, leaving the screen — + // abandons a flow the user may still go on to + // approve in the browser that is still open, and the + // record is what lets the next launch collect that + // password and hand it back. A poll past the + // deadline reports expiry and clears it. + runCatching { pendingFlow.forget() } username = result.credentials.loginName appPassword = result.credentials.appPassword serverRoot = result.credentials.server @@ -400,7 +419,6 @@ class AddAccountViewModel @Inject constructor( // The one exit that never reaches onStartOver: if approval already // landed, that password is about to be replaced by a typed one. discardMintedPassword() - appScope.launch { runCatching { pendingFlow.forget() } } pollJob?.cancel() _state.update { it.copy(step = AddAccountStep.EnterCredentials(error = null), openInBrowser = null) @@ -502,7 +520,6 @@ class AddAccountViewModel @Inject constructor( */ fun onStartOver() { discardMintedPassword() - appScope.launch { runCatching { pendingFlow.forget() } } pollJob?.cancel() pollJob = null found = null @@ -587,13 +604,7 @@ class AddAccountViewModel @Inject constructor( it.copy(step = AddAccountStep.EnterServer(input = typedInput, error = cause)) } - private fun browserFailed(reason: AddAccountMessage) { - // Whatever ended it, nothing is coming back through this token. - appScope.launch { runCatching { pendingFlow.forget() } } - failedInBrowser(reason) - } - - private fun failedInBrowser(reason: AddAccountMessage) = _state.update { + private fun browserFailed(reason: AddAccountMessage) = _state.update { // Keeps whatever the step was carrying. The host-mismatch note is often // the *explanation* for the failure, so dropping it removes the warning // exactly when it becomes worth reading. diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt index 6c7affd..f69703f 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt @@ -311,18 +311,26 @@ class AddAccountViewModelTest { } @Test - fun `a flow that ends badly is forgotten too`() = runTest(dispatcher) { - gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) - gateway.loginFlow = flow() - gateway.pollResults += NextcloudLoginFlow.PollResult.Expired("window closed") + fun `an abandoned flow is kept, because the user may still approve it`() = + runTest(dispatcher) { + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + gateway.loginFlow = flow() + gateway.pollResults += NextcloudLoginFlow.PollResult.Expired("window closed") - val vm = viewModel() - vm.onServerInputChanged("https://cloud.example.com/") - vm.onServerSubmitted() - advanceUntilIdle() + val vm = viewModel() + vm.onServerInputChanged("https://cloud.example.com/") + vm.onServerSubmitted() + advanceUntilIdle() + vm.onStartOver() + advanceUntilIdle() - assertThat(record.remembered).isNull() - } + // ⚠️ We gave up; the browser tab did not. Approving in it still + // mints a password, and this record is the only thing that can + // collect it and hand it back — a poll past the deadline is what + // retires it, not our giving up. + assertThat(record.remembered).isNotNull() + assertThat(record.log).containsExactly("remember") + } } @Nested