mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-08-09 08:04:45 +00:00
refactor(blossom): harden import flow after audit
Follow-up fixes from an audit of the import feature: - Cancel an in-flight scan when the source selection changes (toggle/add/remove/enable-all). Without this a scan started against the old selection could land afterwards and offer blobs sourced from a server the user just de-selected — which importSelected() would then mirror from. - Sign the BUD-02 list token once per scan and reuse it across every source and target. The token carries no `server` scope tag, so it's valid everywhere; per-server signing was a round-trip storm with remote NIP-46 signers. - BlossomMirrorQueue.start() now returns whether it actually started a sweep. importSelected() keys the Started/Busy result off that instead of a separate isRunning check, closing a TOCTOU where the import would report "started" but the queue silently dropped the work. - The import screen's empty-state now collects the kind-10063 server list reactively, so the "add servers first" ↔ picker switch recomposes when the list arrives from a relay after the screen opens. - init() re-points at the current account each call (matching the sibling BlobManager VM) while still seeding the source list only once. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5G515Grhc4t7ACoza9eyN
This commit is contained in:
+9
-4
@@ -89,14 +89,18 @@ class BlossomMirrorQueue(
|
||||
|
||||
val isRunning get() = _state.value?.running == true
|
||||
|
||||
/** Enqueue a sweep. No-op if one is already running or there's nothing to do. */
|
||||
/**
|
||||
* Enqueue a sweep. Returns true when this call actually started one; false (no-op) when a
|
||||
* sweep is already running or there's nothing to do — the caller can tell the difference so
|
||||
* it doesn't report an import/sync as started when the work was silently dropped.
|
||||
*/
|
||||
fun start(
|
||||
account: Account,
|
||||
tasks: List<Task>,
|
||||
) {
|
||||
if (isRunning) return
|
||||
): Boolean {
|
||||
if (isRunning) return false
|
||||
val work = tasks.flatMap { t -> t.targets.map { t to it } }
|
||||
if (work.isEmpty()) return
|
||||
if (work.isEmpty()) return false
|
||||
|
||||
// Publish the active state and start the foreground service synchronously (we're on the
|
||||
// foreground thread here, which is what dataSync FGS starts require) before the sweep runs.
|
||||
@@ -122,6 +126,7 @@ class BlossomMirrorQueue(
|
||||
}.awaitAll()
|
||||
_state.update { it?.copy(running = false) }
|
||||
}
|
||||
return true
|
||||
}
|
||||
|
||||
private suspend fun mirrorOne(
|
||||
|
||||
+16
-3
@@ -20,6 +20,7 @@
|
||||
*/
|
||||
package com.vitorpamplona.amethyst.ui.actions.mediaServers
|
||||
|
||||
import android.widget.Toast
|
||||
import androidx.compose.foundation.background
|
||||
import androidx.compose.foundation.clickable
|
||||
import androidx.compose.foundation.layout.Arrangement
|
||||
@@ -52,6 +53,7 @@ import androidx.compose.ui.Alignment
|
||||
import androidx.compose.ui.Modifier
|
||||
import androidx.compose.ui.draw.clip
|
||||
import androidx.compose.ui.graphics.Color
|
||||
import androidx.compose.ui.platform.LocalContext
|
||||
import androidx.compose.ui.res.pluralStringResource
|
||||
import androidx.compose.ui.text.font.FontWeight
|
||||
import androidx.compose.ui.text.style.TextOverflow
|
||||
@@ -97,6 +99,11 @@ fun BlossomImportScreen(
|
||||
val scanning by vm.isScanning.collectAsStateWithLifecycle()
|
||||
val scanned by vm.scanned.collectAsStateWithLifecycle()
|
||||
val error by vm.error.collectAsStateWithLifecycle()
|
||||
// Collect the user's own server list so the empty-state ↔ picker switch recomposes if the
|
||||
// kind-10063 list arrives from a relay just after the screen opens (common on cold start).
|
||||
val targetServers by accountViewModel.account.blossomServers.flow
|
||||
.collectAsStateWithLifecycle()
|
||||
val context = LocalContext.current
|
||||
|
||||
Scaffold(
|
||||
topBar = {
|
||||
@@ -106,7 +113,7 @@ fun BlossomImportScreen(
|
||||
)
|
||||
},
|
||||
) { padding ->
|
||||
if (vm.hasNoTargets) {
|
||||
if (targetServers.isEmpty()) {
|
||||
NoTargetsState(
|
||||
modifier =
|
||||
Modifier
|
||||
@@ -224,8 +231,14 @@ fun BlossomImportScreen(
|
||||
ImportResultCard(
|
||||
count = candidates.size,
|
||||
onImport = {
|
||||
vm.importSelected()
|
||||
nav.popBack()
|
||||
when (vm.importSelected()) {
|
||||
is ImportStart.Started -> nav.popBack()
|
||||
ImportStart.Busy ->
|
||||
Toast
|
||||
.makeText(context, stringRes(context, R.string.blossom_import_busy), Toast.LENGTH_LONG)
|
||||
.show()
|
||||
ImportStart.Empty -> {}
|
||||
}
|
||||
},
|
||||
)
|
||||
}
|
||||
|
||||
+58
-18
@@ -82,6 +82,17 @@ data class ImportSource(
|
||||
val scan: SourceScanState = SourceScanState.Idle,
|
||||
)
|
||||
|
||||
/** Outcome of tapping "import": either the sweep started, the queue was busy, or nothing to do. */
|
||||
sealed interface ImportStart {
|
||||
data class Started(
|
||||
val count: Int,
|
||||
) : ImportStart
|
||||
|
||||
data object Busy : ImportStart
|
||||
|
||||
data object Empty : ImportStart
|
||||
}
|
||||
|
||||
/**
|
||||
* A blob found on a source server that at least one of the user's own servers is
|
||||
* missing — i.e. something worth importing. [sourceUrl] is the absolute URL the
|
||||
@@ -109,7 +120,7 @@ data class ImportCandidate(
|
||||
@Stable
|
||||
class BlossomImportViewModel : ViewModel() {
|
||||
private lateinit var account: Account
|
||||
private var initialized = false
|
||||
private var seeded = false
|
||||
|
||||
private val _sources = MutableStateFlow<List<ImportSource>>(emptyList())
|
||||
val sources = _sources.asStateFlow()
|
||||
@@ -128,9 +139,11 @@ class BlossomImportViewModel : ViewModel() {
|
||||
val error = _error.asStateFlow()
|
||||
|
||||
fun init(accountViewModel: AccountViewModel) {
|
||||
if (initialized) return
|
||||
initialized = true
|
||||
// Re-point at the current account every call (matches the sibling BlobManager VM), but
|
||||
// seed the source list only once so we don't clobber the user's toggles on recomposition.
|
||||
this.account = accountViewModel.account
|
||||
if (seeded) return
|
||||
seeded = true
|
||||
seedSources()
|
||||
}
|
||||
|
||||
@@ -139,9 +152,6 @@ class BlossomImportViewModel : ViewModel() {
|
||||
account.blossomServers.flow.value
|
||||
.distinct()
|
||||
|
||||
/** True when the user hasn't configured any of their own servers to import into. */
|
||||
val hasNoTargets get() = targets().isEmpty()
|
||||
|
||||
private fun seedSources() {
|
||||
val ownHosts = targets().mapTo(HashSet()) { BlossomServerUrl.domain(it) }
|
||||
_sources.value =
|
||||
@@ -162,10 +172,12 @@ class BlossomImportViewModel : ViewModel() {
|
||||
|
||||
fun toggle(baseUrl: String) {
|
||||
_sources.update { list -> list.map { if (it.baseUrl == baseUrl) it.copy(enabled = !it.enabled) else it } }
|
||||
invalidateResults()
|
||||
}
|
||||
|
||||
fun setAll(enabled: Boolean) {
|
||||
_sources.update { list -> list.map { it.copy(enabled = enabled) } }
|
||||
invalidateResults()
|
||||
}
|
||||
|
||||
/** Add a hand-typed server. Ignores blanks and duplicates (matched by host). */
|
||||
@@ -190,10 +202,27 @@ class BlossomImportViewModel : ViewModel() {
|
||||
list + ImportSource(normalized, host, host, enabled = true, custom = true)
|
||||
}
|
||||
}
|
||||
invalidateResults()
|
||||
}
|
||||
|
||||
fun remove(baseUrl: String) {
|
||||
_sources.update { list -> list.filterNot { it.baseUrl == baseUrl } }
|
||||
invalidateResults()
|
||||
}
|
||||
|
||||
/**
|
||||
* Drop the previous scan's results whenever the source selection changes — otherwise the
|
||||
* "Import N files" button could mirror blobs sourced from a server the user just disabled
|
||||
* or removed. Forces a fresh scan against the current selection.
|
||||
*/
|
||||
private fun invalidateResults() {
|
||||
// Cancel an in-flight scan too, so its results (for the old selection) can't land after the change.
|
||||
scanJob?.cancel()
|
||||
_isScanning.value = false
|
||||
_candidates.value = emptyList()
|
||||
_scanned.value = false
|
||||
_error.value = null
|
||||
_sources.update { list -> list.map { if (it.scan == SourceScanState.Idle) it else it.copy(scan = SourceScanState.Idle) } }
|
||||
}
|
||||
|
||||
private fun clientFor(server: String) = BlossomClient(Amethyst.instance.roleBasedHttpClientBuilder.okHttpClientForUploads(server))
|
||||
@@ -235,6 +264,10 @@ class BlossomImportViewModel : ViewModel() {
|
||||
targets: List<String>,
|
||||
) {
|
||||
val pubkey = account.signer.pubKey
|
||||
// A BUD-02 `t=list` token with no `server` tag is generic, so one signature covers
|
||||
// every source AND target list call. Signing once (instead of per server) avoids a
|
||||
// round-trip storm with remote NIP-46 signers.
|
||||
val listAuth = account.createBlossomListAuth("List blobs").toAuthorizationHeader()
|
||||
|
||||
// Phase 1 — /list each enabled source. Collect the user's blobs and remember the
|
||||
// first source that can serve each hash (its descriptor URL is the mirror source).
|
||||
@@ -245,8 +278,7 @@ class BlossomImportViewModel : ViewModel() {
|
||||
async {
|
||||
val listed =
|
||||
try {
|
||||
val auth = account.createBlossomListAuth("List blobs").toAuthorizationHeader()
|
||||
clientFor(source).list(source, pubkey, auth)
|
||||
clientFor(source).list(source, pubkey, listAuth)
|
||||
} catch (e: CancellationException) {
|
||||
throw e
|
||||
} catch (e: Exception) {
|
||||
@@ -273,7 +305,7 @@ class BlossomImportViewModel : ViewModel() {
|
||||
|
||||
// Phase 2 — which of the user's own servers already hold each hash? /list where the
|
||||
// server supports it, HEAD-probe (bounded) the rest, so we only offer the true gaps.
|
||||
val holders = targetHolders(allHashes, targets, pubkey)
|
||||
val holders = targetHolders(allHashes, targets, listAuth)
|
||||
|
||||
_candidates.value =
|
||||
allHashes
|
||||
@@ -289,8 +321,9 @@ class BlossomImportViewModel : ViewModel() {
|
||||
private suspend fun targetHolders(
|
||||
hashes: List<HexKey>,
|
||||
targets: List<String>,
|
||||
pubkey: HexKey,
|
||||
listAuth: String,
|
||||
): Map<String, Set<HexKey>> {
|
||||
val pubkey = account.signer.pubKey
|
||||
val listed: List<Pair<String, Set<HexKey>?>> =
|
||||
coroutineScope {
|
||||
targets
|
||||
@@ -298,8 +331,7 @@ class BlossomImportViewModel : ViewModel() {
|
||||
async {
|
||||
target to
|
||||
try {
|
||||
val auth = account.createBlossomListAuth("List blobs").toAuthorizationHeader()
|
||||
clientFor(target).list(target, pubkey, auth).mapNotNull { it.sha256 }.toSet()
|
||||
clientFor(target).list(target, pubkey, listAuth).mapNotNull { it.sha256 }.toSet()
|
||||
} catch (e: CancellationException) {
|
||||
throw e
|
||||
} catch (e: Exception) {
|
||||
@@ -337,15 +369,23 @@ class BlossomImportViewModel : ViewModel() {
|
||||
/**
|
||||
* Hand every discovered gap to the app-level mirror queue, which asks each of the
|
||||
* user's servers to fetch the blob from its source. Reuses the same queue (and
|
||||
* floating progress banner) as the on-screen "sync all". Returns the file count so
|
||||
* the caller can surface it.
|
||||
* floating progress banner) as the on-screen "sync all".
|
||||
*
|
||||
* There is a single global queue, so if a sweep (or another import) is already in
|
||||
* flight the queue would silently drop this one — [ImportStart.Busy] lets the caller
|
||||
* keep the screen up and tell the user instead of navigating away to nothing.
|
||||
*/
|
||||
fun importSelected(): Int {
|
||||
fun importSelected(): ImportStart {
|
||||
val candidates = _candidates.value
|
||||
val tasks = candidates.map { BlossomMirrorQueue.Task(it.hash, it.sourceUrl, it.size, it.missingTargets) }
|
||||
if (tasks.isEmpty()) return 0
|
||||
Amethyst.instance.blossomMirrorQueue.start(account, tasks)
|
||||
return candidates.size
|
||||
if (tasks.isEmpty()) return ImportStart.Empty
|
||||
// start() itself atomically no-ops if a sweep is already running, so key off its return
|
||||
// rather than a separate isRunning check that could race with a sweep starting.
|
||||
return if (Amethyst.instance.blossomMirrorQueue.start(account, tasks)) {
|
||||
ImportStart.Started(candidates.size)
|
||||
} else {
|
||||
ImportStart.Busy
|
||||
}
|
||||
}
|
||||
|
||||
private fun setScanState(
|
||||
|
||||
@@ -1706,6 +1706,7 @@
|
||||
<string name="blossom_import_none_found">No new files were found on the selected servers.</string>
|
||||
<string name="blossom_import_no_targets">Add your own Blossom servers first, so there\'s somewhere to copy the imported files into.</string>
|
||||
<string name="blossom_import_manage_servers">Manage my servers</string>
|
||||
<string name="blossom_import_busy">A file sync is already running. Try importing again once it finishes.</string>
|
||||
<plurals name="blossom_import_files_found">
|
||||
<item quantity="one">%1$d file</item>
|
||||
<item quantity="other">%1$d files</item>
|
||||
|
||||
Reference in New Issue
Block a user