sync: three corrections from the review of this branch
Re-authentication duplicated every list. attach() looked up rows with account_id IS NULL, which is right for a re-add but wrong for a re-auth: the account's own lists never matched, so signing in again after a 401 inserted a second copy of each, with no unique index on href to catch it. The lookup now covers both, and a list already owned by this account keeps its cursor instead of being sent through a full reconciliation for nothing. The poll's uncancellable window started one suspension point too late. pollLoginFlow is a blocking execute() inside withContext, and withContext throws on return if the job was cancelled meanwhile — so a back gesture in that window discarded a 200 the server had already answered, along with the only copy of a password it had already minted and deleted its flow row for. ead84c3's reclaim cannot save that one either: the row is gone, so a later poll can only report expiry. And the flow record is now cleared only when the token is actually spent. Clearing it on start over, the back arrow or a failure threw away the one thing that could collect a password the user goes on to approve in the browser tab we abandoned but they did not. While proving the second of those: a server without sync-collection reconciles in full on every run, so recording each one kept the cadence permanently fresh and fullReconciliationDue permanently false. Harmless for the path that reads it — the cursor is null there anyway — but it meant ddcffa6's quarantine probe would never have fired for exactly those servers. The mark records scheduled reconciliations now.
This commit is contained in:
@@ -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<TaskCollection>): List<Long> {
|
||||
val orphans = database.taskLists().orphaned().associateBy { it.href }
|
||||
val known = database.taskLists().attachable(accountId).associateBy { it.href }
|
||||
val inserted = mutableListOf<Long>()
|
||||
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 },
|
||||
),
|
||||
)
|
||||
}
|
||||
|
||||
@@ -31,7 +31,14 @@ class SyncCadenceStore @Inject constructor(
|
||||
@SyncStateDataStore private val dataStore: DataStore<Preferences>,
|
||||
) {
|
||||
|
||||
/** 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<Long, Instant> =
|
||||
dataStore.data.first()[KEY].orEmpty().mapNotNull { entry ->
|
||||
val separator = entry.lastIndexOf(SEPARATOR)
|
||||
|
||||
@@ -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 },
|
||||
)
|
||||
|
||||
|
||||
@@ -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<TaskListEntity>
|
||||
@Query(
|
||||
"""
|
||||
SELECT * FROM task_lists
|
||||
WHERE href IS NOT NULL AND (account_id IS NULL OR account_id = :accountId)
|
||||
"""
|
||||
)
|
||||
fun attachable(accountId: Long): List<TaskListEntity>
|
||||
|
||||
/** 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")
|
||||
|
||||
@@ -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.
|
||||
|
||||
+18
-10
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user