mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-10-06 19:53:08 +00:00
fix: third-round audit findings on the invite-device check
Two of these are defects in the previous round's fixes. 1. An offline check poisoned the cache. fetchAll returns an empty list for a device that reached no relay rather than throwing, so "nobody has published one" and "we are offline" arrive identically as NONE — and the banner's catch for unreachable relays never fires. Caching that for the full 15 minutes suppressed the warning long after the network came back. This is in direct tension with the previous round, which cached NONE precisely to stop re-fanning the query on every entry; both concerns are real, so NONE now gets a 60-second life while a definite answer keeps the full one. 2. The automatic publish paths never invalidated the cache. republishKeyPackageConfirmed did, but publishMarmotKeyPackage and publishMarmotKeyPackages — the post-Welcome rotation — did not, so the banner kept warning for up to 15 minutes after a rotation had already made this device the owner. 3. The banner's publish caught CancellationException and popped a failure toast for a banner that had already left composition, twelve lines below a LaunchedEffect that correctly rethrows it. 4. The drawer label duplicates marmot_groups_title, which commonsUI has translated into 56 locales. Not fixable in place: labelRes is an Android @StringRes shared by every catalog entry, and a Compose resource cannot be referenced there without changing the type for all of them. Documented as an accepted duplication; Crowdin manages both resource sets, so the Android copy gets translated too. 5. A comment of mine overclaimed. getUserIfExists returns null for an unseen sender, so observeUserInfo is never reached and the kind:0 the comment promised is never requested. Corrected the comment rather than the code: the LoadUser idiom would put a metadata REQ behind every row for senders the reader never asked about, which a preview prefix does not justify. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLiEQmy9z71bEvebmUGkSP
This commit is contained in:
@@ -38,6 +38,18 @@ import com.vitorpamplona.quartz.utils.Log
|
||||
import com.vitorpamplona.quartz.utils.TimeUtils
|
||||
import kotlin.coroutines.cancellation.CancellationException
|
||||
|
||||
/**
|
||||
* How long a [LatestKeyPackageOwner.NONE] answer may be reused.
|
||||
*
|
||||
* Much shorter than a definite answer's life, because NONE is ambiguous:
|
||||
* `fetchAll` returns an empty list for a device that reached no relay at all
|
||||
* rather than throwing, so "nobody has published one" and "we are offline"
|
||||
* arrive identically. Long enough to stop repeated entries from re-fanning the
|
||||
* query, short enough that the banner is not suppressed for a quarter of an
|
||||
* hour after the network comes back.
|
||||
*/
|
||||
private const val NONE_MAX_AGE_SECONDS = 60L
|
||||
|
||||
/**
|
||||
* Which install owns the newest KeyPackage currently on the account's relays.
|
||||
*
|
||||
@@ -356,6 +368,10 @@ class AccountMarmotActions(
|
||||
}
|
||||
account.client.publish(event, relays)
|
||||
}
|
||||
// A rotation we just published makes this device the newest owner.
|
||||
// Leaving the old answer in place would keep the banner warning for
|
||||
// up to its full life about a state that no longer exists.
|
||||
lastOwnerCheck = null
|
||||
}
|
||||
}
|
||||
|
||||
@@ -376,6 +392,9 @@ class AccountMarmotActions(
|
||||
}
|
||||
account.cache.justConsumeMyOwnEvent(event)
|
||||
account.client.publish(event, relays)
|
||||
// Same as the rotation path: we have just changed who owns the newest
|
||||
// KeyPackage, so any cached answer is stale by construction.
|
||||
lastOwnerCheck = null
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -450,8 +469,14 @@ class AccountMarmotActions(
|
||||
// whole write set — and the thing it asks about only changes when
|
||||
// another device publishes, which is rare enough to cache.
|
||||
val cached = lastOwnerCheck
|
||||
if (maxAgeSeconds > 0 && cached != null && TimeUtils.now() - cached.first <= maxAgeSeconds) {
|
||||
return cached.second
|
||||
if (maxAgeSeconds > 0 && cached != null) {
|
||||
val maxAge =
|
||||
if (cached.second == LatestKeyPackageOwner.NONE) {
|
||||
minOf(maxAgeSeconds, NONE_MAX_AGE_SECONDS)
|
||||
} else {
|
||||
maxAgeSeconds
|
||||
}
|
||||
if (TimeUtils.now() - cached.first <= maxAge) return cached.second
|
||||
}
|
||||
|
||||
val latest = KeyPackageFetcher.fetchKeyPackage(account.client, account.signer.pubKey, relays)
|
||||
|
||||
+6
@@ -391,6 +391,12 @@ val NavBarCatalog: Map<NavBarItem, NavBarItemDef> =
|
||||
NavBarItem.MARMOT_GROUPS to
|
||||
NavBarItemDef(
|
||||
id = NavBarItem.MARMOT_GROUPS,
|
||||
// Duplicates the already-translated `marmot_groups_title` in
|
||||
// commonsUI's composeResources, which the screen's own title uses.
|
||||
// Unavoidable here: `labelRes` is an Android @StringRes and every
|
||||
// other catalog entry is one, so a Compose resource cannot be
|
||||
// referenced without changing the type for all ~60 of them. Crowdin
|
||||
// manages both resource sets, so this one gets translated too.
|
||||
labelRes = R.string.marmot_groups_title,
|
||||
// Lock, not Group: the rooms list already labels a Marmot room
|
||||
// with this symbol, so it is the signifier users have learned
|
||||
|
||||
+7
-2
@@ -317,8 +317,13 @@ fun MarmotGroupListItem(
|
||||
//
|
||||
// Read through `observeUserInfo`, not `metadataOrNull()`: the latter is a
|
||||
// plain StateFlow.value read, so a kind:0 arriving after the row composed
|
||||
// would never reach it — and nothing would have asked for that kind:0
|
||||
// either. This both subscribes and recomposes when it lands.
|
||||
// would never reach it.
|
||||
//
|
||||
// Deliberately `getUserIfExists` rather than the `LoadUser` idiom: this only
|
||||
// subscribes for a sender the cache already knows, and a sender it does not
|
||||
// simply goes unprefixed. Creating a User per unknown sender would put a
|
||||
// metadata REQ behind every row of a list that is mostly strangers' names
|
||||
// the reader never asked for — a preview line is not worth that.
|
||||
val senderUser =
|
||||
previewEvent
|
||||
?.pubKey
|
||||
|
||||
+6
@@ -181,6 +181,12 @@ fun MarmotInviteDeviceBanner(
|
||||
owner = LatestKeyPackageOwner.OTHER_DEVICE
|
||||
}
|
||||
}
|
||||
} catch (e: CancellationException) {
|
||||
// Leaving the screen mid-publish is not a failure to
|
||||
// report, and swallowing it here would break the
|
||||
// scope's cancellation as well as pop a toast for a
|
||||
// banner that is already gone.
|
||||
throw e
|
||||
} catch (e: Exception) {
|
||||
val failureMessage =
|
||||
stringRes(context, R.string.marmot_invite_device_failure, e.message ?: "")
|
||||
|
||||
Reference in New Issue
Block a user