fix: make the location-chat identity migration finish on its own

Two gaps left by 04ef4128, both of which would have surfaced as a
half-finished release at the flag flip rather than as a failure now.

The mirror ignored the retirement switch. LEGACY_WRITES_RETIRED was private
to LocalPreferences, so GeohashChatIdentityState could not read it and wrote
to its legacy file unconditionally. Flipping the flag would have retired the
four documented mirrors and left this one running. It is `internal` now and
both of that class's legacy writes are gated on it, so the flip is one
switch that stops everything.

Nothing would ever have deleted the identity's legacy file. It is
`secret_keeper_<pubkey hex>`, not `secret_keeper_<npub>` — the writer passed
signer.pubKey where every other caller passes an npub — and the cleanup
enumerates npub-keyed files. It would have sat on disk holding a seed after
every other legacy file was gone. LegacyAccountFiles.delete now clears and
unlinks both, and verify() refuses while the hex file holds an identity the
current store does not.

That check reads geohashSource(), the hex file. An earlier version of it
read the npub file and was reverted for being unable to fire; this is the
corrected form, and it is paired with actually deleting the file it guards.

The copy also had to stop being lazy, and that is the part that would have
bitten users rather than the release. It ran only from
GeohashChatIdentityState, so only for someone who opened a location chat.
Combined with the new gate that is worse than the orphan it replaced: a user
with an identity who never opens another location chat would never be
copied, and their legacy files could then never be deleted at all. It now
runs on the first account load after the upgrade, beside the cleanup call
and before it, so every existing install converges whether or not location
chat is ever touched again.

It sits next to legacyCleanup rather than inside the loader because
innerLoadCurrentAccountFromEncryptedStorage is at the JVM's 64KB method
limit — AccountStoreData's KDoc already records that every suspend call in
there costs a coroutine state, and adding one broke the build with "Method
too large". Outside the lock, once per load, is also where it belongs: this
is migration housekeeping, not part of building AccountSettings.

Tests: four on the gate (an uncopied identity blocks deletion, a copied one
does not, an account that never opened a location chat is not held hostage,
and both files go together) and one on idempotence, which the copy now needs
because it runs on every load.

The plan doc is updated to match: the group and its two quirks, deletion
covering both files, the fourth condition, the flip stopping this mirror
too, and a device-pass step for the seed — the one migrated value whose loss
is silent rather than visible.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AXvKXakvup4inNFfAhhr4L
This commit is contained in:
Claude
2026-09-25 14:10:26 +00:00
parent 48bb8d37e3
commit e619fdcb41
7 changed files with 278 additions and 13 deletions
@@ -42,10 +42,20 @@ to none of those fails that test at the commit that adds it.
| NIP-46 material, wallets, payment source | the account's encrypted DataStore | **kept** |
| current account, saved accounts | the encrypted roster store | **kept** |
| UI settings (`shared_settings`) | `UiSharedPreferences`' own DataStore | none left |
| location-chat identity — seed, nickname | the account's encrypted DataStore, as its own `GeohashIdentitySecrets` group | **kept** |
The three stores that still mirror are the ones whose loss is not an
annoyance: an account that cannot be listed, signed with, or paid from. They
keep the rollback window open until the device pass below has happened.
The stores that still mirror are the ones whose loss is not an annoyance: an
account that cannot be listed, signed with, or paid from. They keep the
rollback window open until the device pass below has happened.
The location-chat identity is the odd one, in two ways worth knowing before
step 4. It is its **own** group rather than fields on `AccountSecrets`: every
account save mirrors a whole `AccountSecrets` built from `AccountSettings`,
which does not hold these, and that group save removes keys whose value is
null — folded in, the seed would be deleted by the next unrelated save and
every geohash identity the user has would silently change. And its legacy home
is a **different file**, `secret_keeper_<pubkey hex>`, because its writer
passed `signer.pubKey` where every other caller passes an npub.
The UI settings copy is **guarded** where the others are not. That store has
been the real home of these settings for a while, so most installs already
@@ -69,7 +79,11 @@ cleanup treat any *other* unclaimed key as a reason to keep the file.
## Deleting a legacy file
`LegacyPreferenceCleanup` runs after every successful account load and deletes
that account's file only when it can prove nothing would be lost:
that account's files — `secret_keeper_<npub>` **and** the location-chat
identity's `secret_keeper_<pubkey hex>` — only when it can prove nothing would
be lost. Both, because nothing else would ever remove the second one: the
cleanup enumerates npub-keyed files, so left out of this it would sit on disk
holding a seed forever.
1. **Every key in the file is accounted for** — claimed by a table, one of the
secrets, or on the accepted list. Driven from the file's own keys, not from
@@ -83,9 +97,14 @@ that account's file only when it can prove nothing would be lost:
3. **The secrets and the private key read back identical** from the current
stores. Those *are* still dual-written, so the stronger question is
available and is asked.
4. A store that cannot be read is a reason, never a pass.
4. **The location-chat identity has been copied**, when its file holds one.
Read from the hex-keyed file, not the npub one — these two keys were never
in that one, so a check pointed at it would never fire. An account that
never opened a location chat holds neither key, which is a real answer and
must not hold the file hostage.
5. A store that cannot be read is a reason, never a pass.
It refuses today, and says so, because of the fifth condition:
It refuses today, and says so, because of the last condition:
`LEGACY_WRITES_RETIRED` is false. While the app still mirrors into the file,
deleting it achieves nothing — the next save recreates it — and would look
like it had worked.
@@ -97,7 +116,10 @@ like it had worked.
3. Do the device pass below.
4. Flip `LEGACY_WRITES_RETIRED` and drop the legacy writes for the identity,
key, secret and roster stores. This ends the rollback window, so it is a
release of its own.
release of its own. The flag is `internal`, not private, so the
location-chat mirror in `GeohashChatIdentityState` reads the same switch —
flipping it stops that write too, and the cleanup then removes both of the
account's legacy files. One flip, nothing left behind.
5. Keep the reader, and `androidx.security.crypto`, indefinitely.
## Verification this needs and has not had
@@ -110,6 +132,9 @@ the one irreversible step in the whole series.
On a real device, before step 4 ships: upgrade an install holding accounts and
confirm they all list; open one and sign; force-stop and relaunch; add and
remove an account; pair a NIP-46 signer; pay from a wallet; check the
key-backup nudge stays dismissed; confirm UI settings survive the upgrade.
key-backup nudge stays dismissed; confirm UI settings survive the upgrade;
open a location chat under a bunker or external signer and confirm the
throwaway identity and nickname are the same ones as before the upgrade — the
seed is the one migrated value whose loss is silent rather than visible.
Then let the cleanup run with the flag flipped, and confirm the files are gone
and everything above still holds on the next cold start.
@@ -127,6 +127,14 @@ class AccountSecretsStore(
return legacy
}
/**
* What the current store holds for the location-chat identity, with no
* fallback to the legacy file — the same distinction [stored] draws, and
* for the same reader: [LegacyPreferenceCleanup] has to tell "migrated"
* from "falling back and looking migrated".
*/
suspend fun storedGeohashIdentity(npub: String): GeohashIdentitySecrets? = stores.loadGeohashIdentity(npub)
/** Mirrors a save into the new store. The legacy write stays where it is. */
suspend fun mirrorGeohashIdentity(
npub: String,
@@ -25,6 +25,7 @@ import com.vitorpamplona.amethyst.commons.model.preferences.AccountIdentityStore
import com.vitorpamplona.amethyst.commons.model.preferences.AccountSecrets
import com.vitorpamplona.amethyst.commons.model.preferences.DialogDismissalStore
import com.vitorpamplona.amethyst.commons.model.preferences.FeedVisibilityStore
import com.vitorpamplona.amethyst.commons.model.preferences.GeohashIdentitySecrets
import com.vitorpamplona.amethyst.commons.model.preferences.LatestEventCacheStore
import com.vitorpamplona.amethyst.commons.model.preferences.LegacyAccountSecretNames
import com.vitorpamplona.amethyst.commons.model.preferences.LegacyKeyTable
@@ -33,6 +34,7 @@ import com.vitorpamplona.amethyst.commons.model.preferences.NotificationPrefsSto
import com.vitorpamplona.amethyst.commons.model.preferences.RelayAuthStore
import com.vitorpamplona.amethyst.commons.model.preferences.TopNavFollowListStore
import com.vitorpamplona.amethyst.commons.model.preferences.UploadSettingsStore
import com.vitorpamplona.amethyst.commons.model.preferences.readLegacyGeohashIdentity
import com.vitorpamplona.quartz.utils.Log
/**
@@ -100,6 +102,17 @@ sealed interface LegacyCleanupResult {
interface LegacyAccountFiles {
fun source(npub: String): LegacyPreferenceSource
/**
* The account's OTHER legacy file: the location-chat identity, which lives
* in `secret_keeper_<pubkey hex>` rather than `secret_keeper_<npub>`
* because that is the key its writer passed.
*
* Separate from [source] because [delete] removes both, and a check that
* read the npub file for these keys would never find them — they are not
* in it. That mistake was made once already.
*/
fun geohashSource(npub: String): LegacyPreferenceSource
fun exists(npub: String): Boolean
/** Returns false when there was nothing to delete. */
@@ -113,6 +126,14 @@ interface MigratedSecrets {
/** Null only when the account genuinely has no private key. Throws when the store is unreadable. */
suspend fun privateKey(npub: String): String?
/**
* The location-chat identity, or null when it has not been copied across.
*
* Its own question because it migrates out of its own file: [AccountSecrets]
* being present says nothing about whether this was carried over.
*/
suspend fun geohashIdentity(npub: String): GeohashIdentitySecrets?
}
/**
@@ -255,9 +276,42 @@ class LegacyPreferenceCleanup(
}
}
reasons += geohashMismatches(npub)
return reasons
}
/**
* Whether deleting this account's `secret_keeper_<pubkey hex>` file would
* lose its location-chat identity.
*
* Read from [LegacyAccountFiles.geohashSource], not from the npub file the
* rest of [verify] walks: these two keys were never in that one. An account
* that never opened a location chat holds neither, and needs no copy.
*/
private suspend fun geohashMismatches(npub: String): List<String> {
val legacy =
try {
readLegacyGeohashIdentity(files.geohashSource(npub))
} catch (e: Exception) {
Log.w(TAG, "Could not read the location-chat identity file for $npub", e)
return listOf("the location-chat identity file could not be read")
}
if (legacy == GeohashIdentitySecrets()) return emptyList()
return try {
if (secrets.geohashIdentity(npub) == null) {
listOf("the location-chat identity has not been copied across")
} else {
emptyList()
}
} catch (e: Exception) {
Log.w(TAG, "Could not read the location-chat identity store for $npub", e)
listOf("the location-chat identity store could not be read")
}
}
/**
* Deletes the account's legacy file if — and only if — [verify] comes back
* empty and the app has stopped writing to it.
@@ -57,6 +57,7 @@ import com.vitorpamplona.amethyst.commons.model.preferences.UploadSettings
import com.vitorpamplona.amethyst.commons.model.preferences.UploadSettingsStore
import com.vitorpamplona.amethyst.commons.model.preferences.orIfUnusable
import com.vitorpamplona.amethyst.commons.model.preferences.readLegacyAccountSecrets
import com.vitorpamplona.amethyst.commons.model.preferences.readLegacyGeohashIdentity
import com.vitorpamplona.amethyst.commons.model.topNavFeeds.TopFilter
import com.vitorpamplona.amethyst.commons.relayauth.RelayAuthPolicy
import com.vitorpamplona.amethyst.model.AccountSettings
@@ -78,6 +79,7 @@ import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair
import com.vitorpamplona.quartz.nip01Core.metadata.MetadataEvent
import com.vitorpamplona.quartz.nip02FollowList.ContactListEvent
import com.vitorpamplona.quartz.nip17Dm.settings.ChatMessageRelayListEvent
import com.vitorpamplona.quartz.nip19Bech32.bech32.bechToBytes
import com.vitorpamplona.quartz.nip19Bech32.toNpub
import com.vitorpamplona.quartz.nip28PublicChat.list.ChannelListEvent
import com.vitorpamplona.quartz.nip37Drafts.privateOutbox.PrivateOutboxRelayListEvent
@@ -325,8 +327,14 @@ object LocalPreferences {
* Flipping this is a release of its own, and it ends the rollback window.
* It waits on the device pass in
* `amethyst/plans/2026-09-23-encrypted-storage-retirement.md`.
*
* `internal` rather than private because the mirror is not all in this
* file: [com.vitorpamplona.amethyst.model.GeohashChatIdentityState] writes
* the location-chat identity into its own legacy file and reads this to
* know when to stop. Private, it would have kept writing after the flip
* and the switch would only half work.
*/
private const val LEGACY_WRITES_RETIRED = false
internal const val LEGACY_WRITES_RETIRED = false
private val legacyCleanup: LegacyPreferenceCleanup by lazy {
LegacyPreferenceCleanup(
@@ -338,12 +346,25 @@ object LocalPreferences {
override fun exists(npub: String) = legacyAccountFile(npub).exists()
override fun geohashSource(npub: String) = LegacySharedPreferences(encryptedPreferences(geohashLegacyKey(npub)))
override suspend fun delete(npub: String): Boolean {
// Clear before unlinking, as deleteAccount does: the live
// SharedPreferences still holds the values in memory and
// would write them straight back out.
encryptedPreferences(npub).edit(commit = true) { clear() }
return legacyAccountFile(npub).delete()
val removedAccountFile = legacyAccountFile(npub).delete()
// The location-chat identity is in a SECOND file, keyed by the
// pubkey hex rather than the npub, because that is the key its
// writer passed. Nothing else would ever remove it, so it is
// deleted here with the account's own file rather than left as
// an orphan holding a seed forever.
val hex = geohashLegacyKey(npub)
encryptedPreferences(hex).edit(commit = true) { clear() }
legacyAccountFile(hex).delete()
return removedAccountFile
}
},
currentStore = { npub -> accountStores.getDataStore(npub).data.first() },
@@ -352,6 +373,8 @@ object LocalPreferences {
override suspend fun secrets(npub: String) = accountSecretsStore.stored(npub)
override suspend fun privateKey(npub: String) = accountKeyStore.stored(npub)
override suspend fun geohashIdentity(npub: String) = accountSecretsStore.storedGeohashIdentity(npub)
},
legacyWritesRetired = LEGACY_WRITES_RETIRED,
)
@@ -368,6 +391,37 @@ object LocalPreferences {
return File(prefsDirPath, "$name.xml")
}
/**
* The key the location-chat identity's legacy file is named by.
*
* [GeohashChatIdentityState] passed `signer.pubKey` — hex — where every
* other caller of [encryptedPreferences] passes an npub, so that material
* sits in `secret_keeper_<hex>`, a different file from the account's own
* `secret_keeper_<npub>`. Converting here keeps that quirk in one place.
*/
private fun geohashLegacyKey(npub: String): String = npub.bechToBytes("npub").toHexKey()
/**
* Copies the location-chat identity out of `secret_keeper_<pubkey hex>` on
* the first load after the upgrade.
*
* Eager, not lazy. [GeohashChatIdentityState] also copies on first use, but
* only a user who opens a location chat ever reaches it — and the cleanup
* refuses to delete an account's legacy files while that file still holds
* an identity the current store does not. Left to the lazy path alone, a
* user who never opens another location chat would keep both files
* forever, which is the opposite of what the migration is for.
*
* Idempotent: the store's marker makes every run after the first a no-op,
* so re-running it on each load cannot overwrite a later edit.
*/
private suspend fun copyGeohashIdentity(npub: String) {
accountSecretsStore.readGeohashIdentity(
npub = npub,
legacy = readLegacyGeohashIdentity(LegacySharedPreferences(encryptedPreferences(geohashLegacyKey(npub)))),
)
}
/**
* Everything the account's DataStore holds, read in one hop.
*
@@ -1031,7 +1085,15 @@ object LocalPreferences {
// while `mutex` serialises every account load — under the lock, each
// account on a multi-account cold start would wait for the previous
// one's full cleanup pass. Nothing here feeds the load.
if (loadedHere) legacyCleanup.deleteIfVerified(npub)
if (loadedHere) {
// Before the cleanup, which refuses to delete this account's files
// while the location-chat identity has not been copied. Here rather
// than inside the loader for the reason [AccountStoreData] gives:
// that method is at the JVM's 64KB limit and one more suspend call
// inside it does not fit.
copyGeohashIdentity(npub)
legacyCleanup.deleteIfVerified(npub)
}
accountSettings
}
@@ -23,6 +23,7 @@ package com.vitorpamplona.amethyst.model
import androidx.core.content.edit
import com.vitorpamplona.amethyst.Amethyst
import com.vitorpamplona.amethyst.LegacySharedPreferences
import com.vitorpamplona.amethyst.LocalPreferences
import com.vitorpamplona.amethyst.accountSecretsStore
import com.vitorpamplona.amethyst.commons.model.preferences.GeohashIdentitySecrets
import com.vitorpamplona.amethyst.commons.model.preferences.readLegacyGeohashIdentity
@@ -121,7 +122,11 @@ class GeohashChatIdentityState(
persist(current().copy(nickname = trimmed))
// Mirrored, not moved: the legacy file stays readable until the
// legacy writes are retired app-wide, so a rollback keeps the handle.
Amethyst.instance.encryptedStorage(signer.pubKey).edit { putString(PREF_NICKNAME, trimmed) }
// Gated on the same switch as every other mirror — otherwise flipping
// it would retire the documented four and leave this one writing.
if (!LocalPreferences.LEGACY_WRITES_RETIRED) {
Amethyst.instance.encryptedStorage(signer.pubKey).edit { putString(PREF_NICKNAME, trimmed) }
}
}
}
@@ -151,7 +156,9 @@ class GeohashChatIdentityState(
val fresh = RandomInstance.bytes(GeohashKeyDerivation.SEED_SIZE)
persist(current().copy(deviceSeed = fresh.toHexKey()))
Amethyst.instance.encryptedStorage(signer.pubKey).edit { putString(PREF_KEY, fresh.toHexKey()) }
if (!LocalPreferences.LEGACY_WRITES_RETIRED) {
Amethyst.instance.encryptedStorage(signer.pubKey).edit { putString(PREF_KEY, fresh.toHexKey()) }
}
return fresh
}
@@ -26,6 +26,7 @@ import androidx.datastore.preferences.core.emptyPreferences
import androidx.datastore.preferences.core.mutablePreferencesOf
import androidx.datastore.preferences.core.stringPreferencesKey
import com.vitorpamplona.amethyst.commons.model.preferences.AccountSecrets
import com.vitorpamplona.amethyst.commons.model.preferences.GeohashIdentitySecrets
import com.vitorpamplona.amethyst.commons.model.preferences.LegacyBooleanKey
import com.vitorpamplona.amethyst.commons.model.preferences.LegacyKeyTable
import com.vitorpamplona.amethyst.commons.model.preferences.LegacyPreferenceSource
@@ -52,18 +53,26 @@ private class MapSource(
private class FakeFiles(
private val values: Map<String, Any>,
private val geohashValues: Map<String, Any> = emptyMap(),
) : LegacyAccountFiles {
var deleted = false
private set
/** The `secret_keeper_<pubkey hex>` file goes with the account's own. */
var deletedGeohash = false
private set
var present = true
override fun source(npub: String) = MapSource(values)
override fun geohashSource(npub: String) = MapSource(geohashValues)
override fun exists(npub: String) = present
override suspend fun delete(npub: String): Boolean {
deleted = true
deletedGeohash = true
present = false
return true
}
@@ -73,6 +82,7 @@ private class FakeSecrets(
private val stored: AccountSecrets? = AccountSecrets(),
private val key: String? = null,
private val throws: Boolean = false,
private val geohash: GeohashIdentitySecrets? = GeohashIdentitySecrets(),
) : MigratedSecrets {
override suspend fun secrets(npub: String): AccountSecrets? {
if (throws) throw IllegalStateException("keystore unavailable")
@@ -83,6 +93,11 @@ private class FakeSecrets(
if (throws) throw IllegalStateException("keystore unavailable")
return key
}
override suspend fun geohashIdentity(npub: String): GeohashIdentitySecrets? {
if (throws) throw IllegalStateException("keystore unavailable")
return geohash
}
}
/**
@@ -307,4 +322,77 @@ class LegacyPreferenceCleanupTest {
subject.verify(NPUB),
)
}
// ── the location-chat identity's own legacy file ──────────────────
/**
* The seed is in `secret_keeper_<pubkey hex>`, and [delete] removes that
* file too. So the gate has to refuse while it holds something the current
* store does not — otherwise every geohash identity the account has would
* change on the next launch.
*/
@Test
fun anUncopiedLocationChatIdentityBlocksDeletion() =
runTest {
val (files, subject) =
cleanup(
values = emptyMap(),
files = FakeFiles(emptyMap(), mapOf("geohash_chat_device_seed" to "a".repeat(64))),
secrets = FakeSecrets(geohash = null),
)
val result = subject.deleteIfVerified(NPUB)
assertTrue(result is LegacyCleanupResult.Kept)
assertTrue(
"was ${(result as LegacyCleanupResult.Kept).reasons}",
result.reasons.any { it.contains("location-chat identity") },
)
assertTrue(!files.deleted)
}
/** Copied across: nothing to lose, so it must not block. */
@Test
fun aCopiedLocationChatIdentityDoesNotBlockDeletion() =
runTest {
val (files, subject) =
cleanup(
values = emptyMap(),
files = FakeFiles(emptyMap(), mapOf("geohash_chat_device_seed" to "a".repeat(64))),
secrets = FakeSecrets(geohash = GeohashIdentitySecrets(deviceSeed = "a".repeat(64))),
)
assertEquals(LegacyCleanupResult.Deleted, subject.deleteIfVerified(NPUB))
assertTrue(files.deleted)
}
/**
* An account that never opened a location chat holds neither key. That is a
* real answer, not "not migrated", and must not hold the file hostage.
*/
@Test
fun anAccountWithNoLocationChatIdentityIsNotBlocked() =
runTest {
val (files, subject) =
cleanup(
values = emptyMap(),
files = FakeFiles(emptyMap(), emptyMap()),
secrets = FakeSecrets(geohash = null),
)
assertEquals(LegacyCleanupResult.Deleted, subject.deleteIfVerified(NPUB))
assertTrue(files.deleted)
}
/**
* Both files go, or the hex one is an orphan nothing will ever remove —
* the whole reason it is wired into this gate.
*/
@Test
fun deletingTheAccountFileAlsoRemovesTheLocationChatFile() =
runTest {
val (files, subject) = cleanup(values = emptyMap())
assertEquals(LegacyCleanupResult.Deleted, subject.deleteIfVerified(NPUB))
assertTrue("the hex-keyed file must be deleted with the account's own", files.deletedGeohash)
}
}
@@ -296,4 +296,25 @@ class AccountSecretsStoreTest {
assertEquals(identity, subject.loadGeohashIdentity(npub))
}
/**
* The copy has to be idempotent, because the loader now runs it on every
* account load rather than only when a location chat is opened. A second
* run must not overwrite what the user has changed since the first.
*/
@Test
fun recopyingDoesNotClobberALaterEdit() =
runTest {
val subject = stores()
subject.saveGeohashIdentity(npub, GeohashIdentitySecrets(deviceSeed = "a".repeat(64), nickname = "old"))
// What a fresh load would find in the legacy file: the pre-migration value.
val stored = subject.loadGeohashIdentity(npub)
assertEquals("old", stored?.nickname)
subject.saveGeohashIdentity(npub, GeohashIdentitySecrets(deviceSeed = "a".repeat(64), nickname = "new"))
assertEquals("new", subject.loadGeohashIdentity(npub)?.nickname)
assertEquals("a".repeat(64), subject.loadGeohashIdentity(npub)?.deviceSeed)
}
}