Repository navigation
fix: stop deletion after incomplete cleanup - #1690
ben-kaufman wants to merge 3 commits into
Conversation
 [Medium impact] This PR appears safe to merge within its stated cleanup scope. SummaryThis PR stops profile deletion when private payment-link withdrawal or contact removal is incomplete.
No actionable issues were found. Tests were inspected but not run. ben-kaufman explicitly deferred session ownership, manual-retry binding, restart reconciliation, and cross-device conditional removal. This assessment does not claim those limitations are resolved. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Confirm profile deletion] --> B[Withdraw private payment endpoints]
B -->|Failure| F[Show deletion failure options]
B -->|Success| C[Read and remove contacts]
C -->|Read error or uncertain result| F
C -->|Partial removal| D[Keep unacknowledged contact data]
D --> F
C -->|Complete| E[Delete profile and sign out]
E -->|Failure| F
E -->|Success| G[Clear local private payment state]
G -->|Failure| H[Show error without success or deletion retry]
G -->|Success| I[Report success and leave profile]
Reviews (1) · Last reviewed commit: "fix: stop profile deletion after incompl..." · Reviewed by Greptile |
|
Triage: 3.1.0. It fixes shipped profile deletion, and 3.1.0 is due 28 Oct. |
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Profile deletion now stops when private endpoint withdrawal, the contact read or the contact removal is incomplete, keeps the contacts and overrides that were not acknowledged as removed, and no longer reports success when local finalization fails. Reviewed 479c530, full tier, stacked on #1688 (fix/profile-delete-session-recovery), so the delta against that base is what I read.
What I checked, and 6 candidates I ruled out
Read in full: PubkyRepo.deleteProfile / deleteAllContacts / signOut / clearAuthenticatedState, EditProfileViewModel.attemptDeleteProfile / disconnectProfile, PrivatePaykitRepo.removePublishedEndpointsForCleanup / removePublishedEndpointsLocked / closeAndClear, PaykitSdkService.contactRecords / removeContacts, and remove_contacts_and_block_peers / contact_records in paykit-rs at v0.1.1-rc1 (the pinned paykit version).
Call sites traced: deleteAllContacts (only deleteProfile), removePublishedEndpointsForCleanup(..., deferAppSync = true) (only the delete flow), closeAndClear (delete and disconnect).
CI: build-local green; build, lint and the e2e matrix still running when this was posted.
Ruled out
- A missing-storage read now blocks deletion: dropping the
isMissingPubkyData()fallback is safe becausecontact_recordsis a local SDK storage transaction; it fails only on an uninitialized identity or a storage error, neither of which is evidence of an empty contact list. - Overrides of local-only contacts survive a full success: the old code cleared them to
emptyMap(), the new code subtracts onlyremovedKeys; on successsignOut()reachesclearAuthenticatedState, whosepubkyStore.reset()drops them anyway. - Key format mismatch between records, overrides and
_contacts: all three are compared afterensurePubkyPrefix(), and override keys are written prefixed (PubkyRepo.kt:2054); the new partial-removal test exercises the subtraction. - Contacts the SDK retains by design (active recurring request, peer lease, published marker) now make deletion fail on every retry: this is the stated intent of the PR and its journey (partial removal preserves contacts and prevents deletion), not an accident.
- User left on Edit Profile after
closeAndClearfails post-deletion: deliberate per the newfailed local finalization does not report deletion successtest; the failure dialog is not shown because the remote deletion did happen. - Endpoint withdrawal failure leaves the deletion flag set:
endProfileDeletion()stays in thefinallyunderNonCancellable, andremovePublishedEndpointsForCleanupmarkscontactSharingCleanupPendingon failure.
Not built here: on the Bitkit track an auto review does not run a PR branch's Gradle build, so this is reasoned from code and CI.
Merge confidence: 4/5, no findings, but the change is reasoned from code rather than built here, and build, lint and e2e had not finished.
Reviewer model: claude-opus-5-5
Reasoning effort: Unknown (not exposed to the reviewing session)
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: one LOW inline, reproduced with a failing unit test.
Checked and clean:
- When cleanup is incomplete, the user gets the existing Retry / Disconnect / Cancel dialog.
- Retry is idempotent after a late
signOutfailure, becausefetch_paykit_profilereturnsNonefor an absent profile, so dropping the 404 tolerance is safe. - No path locks the user out of deletion or sign-in for good.
- Private endpoints are now withdrawn before the profile delete, which closes a gap on master.
- With #1688 removing the automatic replay, each stop is final until a manual Retry; the two don't conflict.
profile-cleanup-failure.xml's fault point is right: contact removal goes through the shared-state storage adapter (PubkyShared,put_locked), so a homeserver proxy can fail it.- Twin synonymdev/bitkit-ios#1124 clears the cleanup flag on a successful withdrawal, so it doesn't have the LOW.
Device gate (partial): 2daec90 — not run yet. profile-cleanup-failure.xml needs a homeserver fault proxy. The partial-removal stop can be driven without one, using a contact with an active recurring payment request on two devices: deletion stops, the dialog shows, and after Cancel the contact can or can't pay privately. Device results will follow in a separate review.
| privatePaykitRepo.removePublishedEndpointsForCleanup(TAG, deferAppSync = true) | ||
| val result = pubkyRepo.deleteProfile() | ||
| val result = runSuspendCatching { | ||
| privatePaykitRepo.removePublishedEndpointsForCleanup(TAG, deferAppSync = true).getOrThrow() |
There was a problem hiding this comment.
When deletion stops after this deferred withdrawal succeeded, private endpoints stay withdrawn until the next foreground or network event. With deferAppSync = true, a successful withdrawal leaves cleanupPending = true (PrivatePaykitRepo.kt:508-513). scheduleContactPreparation returns early while it's set. endProfileDeletion() only flips the in-progress flag, and AppViewModel's follow-up synchronizePaykitContacts → scheduleSavedContactPreparation also does nothing. Only retryPendingEndpointRemoval clears the flag, and it runs on foreground (ContentView.kt:319 → AppViewModel.kt:871), network restore, channel events or a USDT availability change.
So after Cancel on the failure dialog the user is signed in, but their contacts can't pay them privately until the app is backgrounded and reopened. Master had the same deferred flag only on the delete-fails branch. This PR adds three stop cases that now reach it: a partial contact removal, a contact-read error, and a 404 from the profile delete. iOS clears the flag on any successful withdrawal (PubkyProfileManager.swift:1498).
Fix: keep the flag (the crash-recovery test interrupted profile deletion retries registry sync from persisted cleanup state depends on it) and reconcile on the stop path instead, in this function's finally:
withContext(NonCancellable) {
privatePaykitRepo.endProfileDeletion()
if (_uiState.value.showDeleteFailureDialog) {
privatePaykitRepo.retryPendingEndpointRemoval(pubkyRepo.contacts.value.map { it.publicKey })
privatePaykitRepo.refreshKnownSavedContactEndpoints(TAG)
}
}In production, route this through contactPaymentSettingsRepo.reconcilePendingEndpoints or an equivalent entry point, as the foreground path does.
LOW.
Reproduction
EditProfileViewModelTest > stopped deletion reconciles withdrawn private endpoints fails at 2daec90 and passes with the fix (20/20):
Wanted but not invoked:
privatePaykitRepo.retryPendingEndpointRemoval(...)
PrivatePaykitRepoTest > stopped profile deletion keeps private endpoints withdrawn until pending cleanup is retried passes at head by design. It shows the stuck state against the real repo: no ensureLinkWithPeer after preparation is rescheduled, with the flag still set. A retry plus a refresh republishes.
./gradlew testDevDebugUnitTest --tests 'to.bitkit.ui.screens.profile.EditProfileViewModelTest' --tests 'to.bitkit.repositories.PrivatePaykitRepoTest'
diff --git a/app/src/test/java/to/bitkit/repositories/PrivatePaykitRepoTest.kt b/app/src/test/java/to/bitkit/repositories/PrivatePaykitRepoTest.kt
index 5e0aa48fe..cb5ef2438 100644
--- a/app/src/test/java/to/bitkit/repositories/PrivatePaykitRepoTest.kt
+++ b/app/src/test/java/to/bitkit/repositories/PrivatePaykitRepoTest.kt
@@ -370,6 +370,38 @@ class PrivatePaykitRepoTest : BaseUnitTest(StandardTestDispatcher()) {
sut.closeAndClear()
}
+ @Test
+ fun `stopped profile deletion keeps private endpoints withdrawn until pending cleanup is retried`() = test {
+ settingsData.update {
+ SettingsData(
+ sharesPrivatePaykitEndpoints = true,
+ publicPaykitLightningEnabled = false,
+ publicPaykitUsdtEnabled = false
+ )
+ }
+ cacheData.update { it.copy(contacts = mapOf(CONTACT_KEY to cachedPublishedContact())) }
+ sut = createSut()
+
+ sut.beginProfileDeletion()
+ sut.removePublishedEndpointsForCleanup("profile deletion", deferAppSync = true).getOrThrow()
+ sut.endProfileDeletion()
+ sut.scheduleSavedContactPreparation(listOf(CONTACT_KEY)).getOrThrow()
+ runCurrent()
+ sut.awaitContactPreparation()
+
+ assertTrue(cacheData.value.cleanupPending)
+ verify(paykitSdkService, never()).ensureLinkWithPeer(CONTACT_KEY)
+
+ sut.retryPendingEndpointRemoval(listOf(CONTACT_KEY)).getOrThrow()
+ sut.refreshKnownSavedContactEndpoints("profile deletion stopped").getOrThrow()
+ runCurrent()
+ sut.awaitContactPreparation()
+
+ assertFalse(cacheData.value.cleanupPending)
+ verify(paykitSdkService).ensureLinkWithPeer(CONTACT_KEY)
+ sut.closeAndClear()
+ }
+
@Test
fun `handleOnchainActivity skips SDK access when publication is locally unnecessary`() = test {
settingsData.value = SettingsData(
diff --git a/app/src/test/java/to/bitkit/ui/screens/profile/EditProfileViewModelTest.kt b/app/src/test/java/to/bitkit/ui/screens/profile/EditProfileViewModelTest.kt
index 30f48762a..eda68d8ea 100644
--- a/app/src/test/java/to/bitkit/ui/screens/profile/EditProfileViewModelTest.kt
+++ b/app/src/test/java/to/bitkit/ui/screens/profile/EditProfileViewModelTest.kt
@@ -12,6 +12,7 @@ import kotlinx.coroutines.test.advanceUntilIdle
import org.junit.Test
import org.mockito.Mockito.clearInvocations
import org.mockito.kotlin.any
+import org.mockito.kotlin.anyOrNull
import org.mockito.kotlin.doSuspendableAnswer
import org.mockito.kotlin.eq
import org.mockito.kotlin.inOrder
@@ -40,6 +41,7 @@ class EditProfileViewModelTest : BaseUnitTest() {
private val context: Context = mock()
private val pubkyRepo: PubkyRepo = mock()
private val privatePaykitRepo: PrivatePaykitRepo = mock()
+ private val contacts = MutableStateFlow<List<PubkyProfile>>(emptyList())
@Test
fun `disconnect finishes private cleanup after the screen is closed`() = test {
@@ -318,6 +320,28 @@ class EditProfileViewModelTest : BaseUnitTest() {
}
}
+ @Test
+ fun `stopped deletion reconciles withdrawn private endpoints`() = test {
+ whenever(pubkyRepo.deleteProfile()).thenReturn(Result.failure(TestAppError("contact retained")))
+ contacts.value = listOf(createProfile())
+ whenever { privatePaykitRepo.retryPendingEndpointRemoval(any()) }.thenReturn(Result.success(Unit))
+ whenever { privatePaykitRepo.refreshKnownSavedContactEndpoints(any(), any(), any(), anyOrNull()) }
+ .thenReturn(Result.success(Unit))
+ val sut = createSut()
+ advanceUntilIdle()
+
+ sut.deleteProfile()
+ advanceUntilIdle()
+
+ assertTrue(sut.uiState.value.showDeleteFailureDialog)
+ inOrder(privatePaykitRepo).apply {
+ verify(privatePaykitRepo).removePublishedEndpointsForCleanup(any(), deferAppSync = eq(true))
+ verify(privatePaykitRepo).endProfileDeletion()
+ verify(privatePaykitRepo).retryPendingEndpointRemoval(listOf(TEST_PUBLIC_KEY))
+ verify(privatePaykitRepo).refreshKnownSavedContactEndpoints(any(), any(), any(), anyOrNull())
+ }
+ }
+
@Test
fun `deleteProfile should show retry dialog when delete still fails`() = test {
whenever(pubkyRepo.deleteProfile()).thenReturn(
@@ -426,6 +450,10 @@ class EditProfileViewModelTest : BaseUnitTest() {
whenever { privatePaykitRepo.removePublishedEndpointsForCleanup(any(), deferAppSync = eq(true)) }
.thenReturn(Result.success(Unit))
whenever { privatePaykitRepo.closeAndClear() }.thenReturn(Result.success(Unit))
+ whenever(pubkyRepo.contacts).thenReturn(contacts)
+ whenever { privatePaykitRepo.retryPendingEndpointRemoval(any()) }.thenReturn(Result.success(Unit))
+ whenever { privatePaykitRepo.refreshKnownSavedContactEndpoints(any(), any(), any(), anyOrNull()) }
+ .thenReturn(Result.success(Unit))
return EditProfileViewModel(
context = context,There was a problem hiding this comment.
The retained cleanup flag and the missing immediate follow-up are real at this head. I am addressing this with the original-owner/restart repair tracked in #1687 rather than automatically republishing after an uncertain destructive operation. That candidate persists a removal obligation before cleanup, shows pending recovery, and offers read-only Review followed by a new confirmation or an explicitly local Disconnect. It deliberately keeps authenticated publication paused until the obligation is resolved. Its UI integration and controlled retry tests are under final validation; it is not published yet. I will link the follow-up and its evidence here. Leaving this thread open.
There was a problem hiding this comment.
The integrated recovery follow-up is now published in #1693 (stacked on this PR). It persists pending removal before destructive work, keeps authenticated publication paused after an uncertain outcome, and exposes read-only Review with a fresh original-owner confirmation or explicit local-only Disconnect. It deliberately does not republish automatically after a potentially completed deletion. Final 4,725 unit/component tests, full compile/format/lint and three installed Compose recovery cases passed; two consecutive independent final reviews are clean and both commits are signed/verified. Complete authenticated fault journeys and native contact CAS remain documented gaps in #1687. Leaving this thread open for review of that recovery behavior rather than claiming the parent alone restores publication safely.
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: I found no additional defects. The existing publication-pause finding remains acknowledged at this head; the author's follow-up tracks its recovery separately.
QA for 2daec90: Android 16 / API 36, Core 0.8.0-rc11 and Paykit 0.1.1-rc1. Device cases used this head plus the test fixture below, with disposable identities and an unfunded regtest wallet. The real Edit Profile screen, ViewModel, repositories and native SDK handled deletion; a homeserver proxy supplied the faults. The installed APK hash matched the built fixture APK.
Test 1: Manual 1 / J1 — private cleanup fails ✅ Failure stops before contact or profile deletion.
The proxy returned 503 for the native shared-state read during private cleanup. After Cancel, the refreshed inspector confirmed the session, both contacts and overrides, and both sharing settings remained. Call counters and the proxy trace confirmed no contact or profile deletion.
1a.2x.mp4
Private cleanup failure and retained state, at 2× speed.
Test 2: Manual 1 / J1 — unconfirmed contact removal ✅ Local evidence survives without entering profile deletion.
The proxy accepted the native write-ahead record, then rejected shared-state publication with 503. The failure dialog appeared. The post-failure inspector and call trace confirmed both contacts and overrides remained, with no profile deletion or success feedback.
| Failure flow | Refreshed post-failure inspector |
|---|---|
1b.2x.mp4 |
 |
Unconfirmed cleanup at 2× speed; the still shows the refreshed counters after Cancel.
Test 3: Manual 1 / J1 — partial native removal ✅ Only the acknowledged contact and override disappear.
The SDK retained a contact with a published public marker and acknowledged the other removal. The inspector showed one remaining contact and override; the persisted override key matched the retained marker. Failure appeared without entering profile deletion. The marker was provisioned through the native SDK.
1c.2x.mp4
Partial removal and retained evidence, at 2× speed.
Test 4: Manual 2 — profile deletion returns 404 ✅ Failure preserves the session and sharing settings.
The proxy trace confirmed profile GET 200 followed by DELETE 404. The app showed failure and retained its session and both sharing settings, with no success feedback. Earlier acknowledged contact removals remained applied, as the refreshed inspector shows.
2.2x.mp4
Rejected profile deletion and retained session, at 2× speed.
Test 5: J1 — Cancel and cold restart ✅ Normal startup restores the same identity without replaying profile deletion.
After unconfirmed contact cleanup and Cancel, a cold launch through the normal app entry restored the same authenticated identity on Home and Profile. The profile QR identifier and restoration log confirmed the owner. Proxy sampling covered over 85 seconds, with successful profile reads and no profile DELETE.
restart.2x.mp4
Normal cold startup and restored profile, at 2× speed.
Check 1: unmodified-head build and regression checks ✅ The app build and affected suites pass.
assembleDevDebug and testDevDebugUnitTest with PubkyRepoTest and EditProfileViewModelTest passed on the unmodified head. git diff --check passed. These focused checks include unavailable contact reads, uncertain/partial removals, 404-like profile errors, private cleanup failure and failed local finalization.
Full review of 99b446a..2daec90, all six changed files and reached cleanup paths. Contact-read errors and local-finalization storage failures had source/unit coverage rather than separate device cases. Active-recurring and peer-lease retention variants were not separately driven. Manual Retry/Disconnect ownership and native cross-device conditional removal remain outside this PR's scope.
Twin: bitkit-ios#1124 fix: stop deletion after incomplete cleanup matches the scoped cleanup/error propagation behavior at a062618; its authenticated device report supplies the iOS evidence.
Test fixture used by Tests 1–5
Not pushed. Provisions disposable native identities and contacts, displays call/state evidence, and renders the actual Edit Profile screen. Public-contact publication is enabled to stage the retained-marker variant. SDK calls return native results. Test 5 launches the normal app entry.
diff --git a/app/src/debug/AndroidManifest.xml b/app/src/debug/AndroidManifest.xml
index b88fe73a7..304e984d2 100644
--- a/app/src/debug/AndroidManifest.xml
+++ b/app/src/debug/AndroidManifest.xml
@@ -6,6 +6,8 @@
android:usesCleartextTraffic="true"
tools:ignore="MissingApplicationIcon">
+ <activity android:name="to.bitkit.dev.CleanupProbeActivity" android:exported="true" /> <!-- test hook -->
+
<provider
android:name="to.bitkit.dev.DevToolsProvider"
android:authorities="${applicationId}.devtools"
diff --git a/app/src/main/java/to/bitkit/services/PaykitSdkService.kt b/app/src/main/java/to/bitkit/services/PaykitSdkService.kt
index 87c452f64..011d57041 100644
--- a/app/src/main/java/to/bitkit/services/PaykitSdkService.kt
+++ b/app/src/main/java/to/bitkit/services/PaykitSdkService.kt
@@ -666,6 +666,11 @@ class PaykitSdkService @Inject constructor(
suspend fun fetchPubkyFollows(publicKey: String): List<String> =
publicRead { it.fetchPubkyFollows(publicKey, maxEntries = 10_000u) }
+ internal suspend fun reviewPublishContact(key: String) { // test hook: provision real native retained marker
+ isSetup.await() // test hook
+ operationLock.withLock { withStateRevisionTracking { handle -> completeSdkCall { handle.publishPublicContact(key) } } } // test hook
+ } // test hook
+
suspend fun contactRecords(): List<ContactRecord> {
isSetup.await()
return operationLock.withLock {
@@ -1952,7 +1957,7 @@ internal object BitkitPaykitSdkConfig {
}
internal fun paykitSdkConfig() = defaultConfig("bitkit").copy(
- publicContactSharing = BitkitPaykitSdkConfig.publicContactSharing,
+ publicContactSharing = com.synonym.paykit.PublicContactSharingPolicy.ENABLED, // test hook: native public-marker fixture
)
internal fun paykitPubkyClientConfig(
diff --git a/app/src/main/java/to/bitkit/services/PubkyService.kt b/app/src/main/java/to/bitkit/services/PubkyService.kt
index e9105a40c..560146420 100644
--- a/app/src/main/java/to/bitkit/services/PubkyService.kt
+++ b/app/src/main/java/to/bitkit/services/PubkyService.kt
@@ -208,6 +208,7 @@ class PubkyService @Inject constructor(
}
suspend fun deletePaykitProfile() = ServiceQueue.CORE.background {
+ to.bitkit.dev.CleanupProbe.deletes++; to.bitkit.dev.CleanupProbe.note("profile delete entered") // test hook
paykitSdkService.deletePaykitProfile()
}
@@ -216,6 +217,7 @@ class PubkyService @Inject constructor(
}
suspend fun contactRecords(): List<ContactRecord> = ServiceQueue.CORE.background {
+ to.bitkit.dev.CleanupProbe.reads++; to.bitkit.dev.CleanupProbe.note("contact read entered") // test hook
paykitSdkService.contactRecords()
}
@@ -252,6 +254,7 @@ class PubkyService @Inject constructor(
}
suspend fun removeContacts(publicKeys: List<String>): List<ContactRecord> = ServiceQueue.CORE.background {
+ to.bitkit.dev.CleanupProbe.removals++; to.bitkit.dev.CleanupProbe.note("contact remove entered count=${publicKeys.size}") // test hook
paykitSdkService.removeContacts(publicKeys)
}
diff --git a/app/src/debug/java/to/bitkit/dev/CleanupProbeActivity.kt b/app/src/debug/java/to/bitkit/dev/CleanupProbeActivity.kt
new file mode 100644
index 000000000..f0c70a077
--- /dev/null
+++ b/app/src/debug/java/to/bitkit/dev/CleanupProbeActivity.kt
@@ -0,0 +1,113 @@
+package to.bitkit.dev
+
+// test hook: disposable native fixture and inspector; deletion decisions use the unchanged screen/viewmodel/repositories.
+import android.os.Bundle
+import android.util.Log
+import androidx.activity.ComponentActivity
+import androidx.activity.compose.setContent
+import androidx.compose.foundation.layout.*
+import androidx.compose.material3.*
+import androidx.compose.runtime.*
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.unit.dp
+import androidx.lifecycle.lifecycleScope
+import dagger.hilt.android.AndroidEntryPoint
+import kotlinx.coroutines.*
+import kotlinx.coroutines.flow.first
+import to.bitkit.data.PubkyStore
+import to.bitkit.data.SettingsStore
+import to.bitkit.data.keychain.Keychain
+import to.bitkit.repositories.*
+import to.bitkit.services.*
+import to.bitkit.ui.screens.profile.*
+import to.bitkit.ui.theme.AppThemeSurface
+import java.security.SecureRandom
+import javax.inject.Inject
+
+object CleanupProbe {
+ var reads = 0
+ var removals = 0
+ var deletes = 0
+ fun note(s: String) { Log.i("CleanupProbe", s) }
+}
+
+@AndroidEntryPoint
+class CleanupProbeActivity : ComponentActivity() {
+ @Inject lateinit var service: PubkyService
+ @Inject lateinit var sdk: PaykitSdkService
+ @Inject lateinit var repo: PubkyRepo
+ @Inject lateinit var privateRepo: PrivatePaykitRepo
+ @Inject lateinit var pubkyStore: PubkyStore
+ @Inject lateinit var settings: SettingsStore
+ @Inject lateinit var keychain: Keychain
+ @Inject lateinit var wallet: WalletRepo
+ private var status by mutableStateOf("Preparing disposable native identity")
+ private var evidence by mutableStateOf("")
+ private var vm by mutableStateOf<EditProfileViewModel?>(null)
+ private var exited by mutableStateOf(false)
+ private var successToasts = 0 // test hook
+
+ override fun onCreate(savedInstanceState: Bundle?) {
+ super.onCreate(savedInstanceState)
+ setContent {
+ AppThemeSurface {
+ Column(Modifier.fillMaxSize().padding(top = 28.dp)) {
+ Text(status, Modifier.padding(horizontal = 12.dp))
+ Text(evidence, Modifier.padding(horizontal = 12.dp))
+ Row {
+ TextButton(onClick = { lifecycleScope.launch { inspect() } }) { Text("Inspect evidence") }
+ TextButton(onClick = { vm?.dismissDeleteFailureDialog() }) { Text("Cancel failure") }
+ }
+ if (exited) Text("Deletion success navigation")
+ vm?.let { current ->
+ Box(Modifier.weight(1f)) {
+ EditProfileScreen(current, onBackClick = {}, onExitProfile = { exited = true })
+ }
+ }
+ }
+ }
+ }
+ lifecycleScope.launch { to.bitkit.ui.shared.toast.ToastEventBus.events.collect { toast -> if (toast.type == to.bitkit.models.Toast.ToastType.SUCCESS) successToasts++; CleanupProbe.note("toast ${toast.type}: ${toast.title}") } } // test hook
+ lifecycleScope.launch {
+ try {
+ if (!keychain.exists(Keychain.Key.BIP39_MNEMONIC.name)) wallet.createWallet(null).getOrThrow() // test hook: unfunded wallet for normal cold startup
+ service.initialize()
+ if (intent.getBooleanExtra("resume", false)) {
+ repo.initialize().getOrThrow()
+ status = "Restored through normal profile initialization"
+ } else {
+ val secret = ByteArray(32).also { SecureRandom().nextBytes(it) }.joinToString("") { "%02x".format(it) }
+ val home = service.publicKeyFromSecret("00".repeat(32))
+ service.signUp(secret, home, null)
+ service.publishPaykitProfile(com.synonym.paykit.PaykitProfile("Disposable cleanup identity", null, null))
+ repo.initialize().getOrThrow()
+ val peers = listOf("11", "22").map { service.publicKeyFromSecret(it.repeat(32)) }
+ for ((index, peer) in peers.withIndex()) {
+ service.saveContact(peer, "Peer ${index + 1}", restorePrivateConnection = true)
+ }
+ if (intent.getBooleanExtra("retained", false)) sdk.reviewPublishContact(peers.last())
+ repo.loadContacts()
+ pubkyStore.update { it.copy(contactProfileOverrides = repo.contacts.value.associate { c -> c.publicKey to c.toProfileData() }) }
+ settings.update { it.copy(sharesPrivatePaykitEndpoints = true, sharesPublicPaykitEndpoints = true) }
+ status = "Authenticated disposable identity"
+ }
+ CleanupProbe.reads = 0; CleanupProbe.removals = 0; CleanupProbe.deletes = 0
+ vm = EditProfileViewModel(applicationContext, repo, privateRepo)
+ CleanupProbe.note("READY owner=${repo.publicKey.value}")
+ inspect()
+ } catch (e: Exception) {
+ status = "Provisioning failed: ${e.message}"
+ CleanupProbe.note(status)
+ Log.e("CleanupProbe", "setup", e)
+ }
+ }
+ }
+
+ private suspend fun inspect() {
+ val state = settings.data.first()
+ val session = keychain.loadString(Keychain.Key.PAYKIT_SESSION.name) != null
+ val overrides = pubkyStore.data.first().contactProfileOverrides.size
+ evidence = "Session $session; contacts ${repo.contacts.value.size}; overrides $overrides; sharing ${state.sharesPublicPaykitEndpoints}/${state.sharesPrivatePaykitEndpoints}; reads ${CleanupProbe.reads}, removals ${CleanupProbe.removals}, deletes ${CleanupProbe.deletes}; success $exited; success toasts $successToasts"
+ CleanupProbe.note(evidence)
+ }
+}Case 1a: Session true; contacts 2; overrides 2; sharing true/true; reads 0, removals 0, deletes 0; success false; success toasts 0
Case 1b: Session true; contacts 2; overrides 2; sharing true/true; reads 1, removals 1, deletes 0; success false; success toasts 0
Case 1c: Session true; contacts 1; overrides 1; sharing true/true; reads 1, removals 1, deletes 0; success false; success toasts 0
Case 2: Session true; contacts 0; overrides 0; sharing true/true; reads 1, removals 1, deletes 1; success false; success toasts 0
Reviewer model: gpt-6.1-sol
Reasoning effort: high
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: full review of the complete PR diff against its merge base 99b446a (the head of dependency #1688), at 2daec90. No earlier QA baseline existed. Coverage: PubkyRepo.deleteProfile / deleteAllContacts; the error paths in EditProfileViewModel for profile deletion and Disconnect; the callers PrivatePaykitRepo.removePublishedEndpointsForCleanup / closeAndClear, signOut / clearAuthenticatedState, and the PaykitSdkService contact and profile wrappers; the updated unit tests and the new journey. Also checked: the Paykit SDK contracts at paykit-rs v0.1.1-rc1 (f384694) and parity with iOS twin bitkit-ios#1124 at a062618.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
The stricter stop-on-incomplete-cleanup behaviour looks sound for transient failures. Dropping the 404/"missing" tolerance is safe: in the SDK, fetch_paykit_profile returns None when the profile is already gone, so deletePaykitProfile completes normally, and contact_records is a local storage read. Contact keys are compared after normalising the pubky prefix in every place. Returning early when there are no contacts leaves no stale state, because the successful signOut() path calls pubkyStore.reset() and clears the contact list. One problem remains: contacts that the SDK keeps on purpose, because of an active recurring subscription, now block profile deletion permanently without saying why (see the inline finding). The iOS twin has the same behaviour: removeContacts skips subscribed contacts and the best-effort wrapper is gone, so the concern applies to both platforms.
Existing unresolved thread: deferred withdrawal leaves private endpoints withdrawn after a stopped deletion. The author says it is tracked in #1687 / #1693. I did not re-report it here.
Validation: no build or tests were run in this review. The author's CI at this head passed: CI, Lint, E2E. None of these exercises the new cleanup-fault journey.
Device testing: not performed in this review.
Suggested additional test cases
- Android, two wallets with different Pubky identities (A and B), plus a third saved contact C on A. A saves B and C as contacts and holds an active recurring subscription with B, as payer or payee. On A, go to Edit Profile and delete the profile. Expected: deletion is refused with a message naming the active subscription, and C plus the private payment links are still intact. Then tap Retry. Expected: it does not loop on the same generic failure.
Findings
- [MEDIUM] An active subscription with a saved contact now blocks profile deletion permanently, with no explanation — inline at
app/src/main/java/to/bitkit/repositories/PubkyRepo.kt:1116.
Reviewer model: claude-opus-5-5
Reasoning effort: high
| updateContacts { current -> current.filterNot { it.publicKey in removedKeys } } | ||
| } | ||
| markContactsLoaded() | ||
| if (removedKeys != keys) throw PrivatePaykitError.PrivateUnavailable |
There was a problem hiding this comment.
[MEDIUM] An active subscription with a saved contact now blocks profile deletion permanently, with no explanation
Trigger: the user has a saved contact with an ACTIVE_RECURRING payment request that has not ended (either payer or payee role), and confirms Delete Profile.
Mechanism: PaykitSdkService.removeContacts (PaykitSdkService.kt:807-815) silently leaves out subscribed counterparties before calling removeContactsAndBlockPeers. This is a deliberate retention policy: PaykitSdkServiceTest active subscription prevents deletion until it ends asserts it returns an empty list. In this PR, deleteAllContacts compares the returned keys to every record and throws PrivatePaykitError.PrivateUnavailable when they differ. By the time it throws, attemptDeleteProfile has already withdrawn the private payment links (removePublishedEndpointsForCleanup), and the SDK has removed and blocked every other saved contact. Nothing checks for active subscriptions beforehand, and the error carries no subscription reason. The single-contact delete path does report one (PubkyContactError.ActiveSubscription).
Consequence: this path shows the generic dialog "We couldn't delete your profile data. Retry, or disconnect…". Every Retry withdraws the links again and fails the same way until the subscription is cancelled or ends. The user is never told why, and is left with a profile that still exists, its other contacts already removed and blocked, and its links withdrawn. Disconnect is the only way out, and it leaves the profile published.
Expected behavior: contacts kept on purpose because of a subscription should be treated differently from unknown or failed removals. Check for active subscriptions before any destructive step and show an actionable reason (as the single-contact delete does), or exclude these contacts from the incomplete-cleanup stop.
Evidence basis: source analysis at 2daec90 and paykit-rs f384694 (remove_contacts_and_block_peers doc: a skipped record does not stop the others from being removed). Not reproduced on a device. An earlier bot review called this stop intended. This finding is about the consequences above (non-actionable message, a Retry that always fails, destructive steps before the check), not about the stop itself.
Twin: synonymdev/bitkit-ios#1124
Dependency: #1688
This PR stops profile deletion after incomplete payment-link or contact cleanup instead of clearing local evidence and continuing.
Description
Out of Scope
Design
Existing Profile › Profile Edit in the Bitkit handoff. This changes failure handling; it adds no visual design.
Preview
N/A — existing error UI is reused.
QA Notes
Setup
Use this branch's normal released dependencies: Core 0.8.0-rc11 and Paykit 0.1.1-rc1. Component tests use synthetic identities and injected service failures; no real profile, wallet or funds are needed. For J1, provision a disposable authenticated identity and a homeserver fault proxy that can hold/fail private endpoint withdrawal, lose a contact-removal acknowledgement, and return a partial removal with a retained active peer, or fail profile deletion with a 404-like error without confirming the outcome. Those controls are unavailable in the standard journey fixture. The committed
journeys/profile/profile-cleanup-failure.xmldescribes the sequence; do not use a personal profile or reset another worker's wallet.Journeys
profile-cleanup-failure.xml— incomplete cleanup preserves retained evidence and prevents later profile deletion.Manual Tests
1. Enable the J1 cleanup fault → Edit Profile → confirm deletion → failure UI appears without profile deletion or success navigation — authenticated cleanup fault proxy not in Capabilities.
2. Inject a profile-deletion error mentioning 404 → confirm deletion → failure appears with the session and sharing settings retained — authenticated fault proxy not in Capabilities.
Automated Checks
PubkyRepoTest.kt— partial/unknown contact removal and unavailable reads stop before profile deletion/sign-out and preserve remaining data.EditProfileViewModelTest.kt— private cleanup failure stops the flow; local finalization failure emits no success and does not open a deletion Retry dialog.Evidence
Controlled baseline: 221 repository tests included two expected failures for partial/unknown contact cleanup; 19 view-model tests included two expected failures for ignored cleanup outcomes. Final source passed
just compile,just test,just lintandjust format: 4,627 tests across 254 suites, zero failures/errors/skips. Complete reports contain zero Detekt/Android Lint findings. JVM sharing notices and the structured Gradle deprecation match recorded baseline diagnostics. Two consecutive independent final reviews found no outstanding issues in this scoped change.Acknowledged contact removals clear only their requested returned keys. Failed private local finalization reports an error without a deletion Retry dialog; Disconnect also consumes that result. The broader ownership/manual-retry limitation above remains open.
The follow-up at
2daec90e2removes broad profile-deletion error suppression. A controlled pre-fix repository suite executed 223 tests with one expected failure: a 404-like deletion error returned success and signed out. The repaired focused suite passed all 223 cases. Final validation passed compile, 4,628 tests across 254 suites with zero failures/errors/skips, full formatting, Detekt and Android Lint with zero findings. Complete diagnostics were inspected. Two consecutive independent final reviews are clean. The SDK already represents confirmed profile absence as normal completion; arbitrary thrown errors no longer authorize sign-out. Earlier acknowledged contact/endpoint removals are not rolled back.The previous
479c53065hosted head passed 4,627 units and all lint reports (artifact SHA-256/ZIP CRC verified, three SARIF/XML reports with zero findings). Its E2E workflow eventually passed 39 cases, but LNURL and Transfer initially timed out waiting for expected UI and passed the workflow's automatic retry. Full logs were inspected; the initial causes remain unproven. No extra rerun was requested.Gaps
J1 and manual cases 1–2 have not run as complete app journeys: the authenticated fault proxy is not provisioned. Component tests do not establish cross-device CAS or historical incident causation. All three commits in the complete current-base PR range are verified locally and on GitHub (
verified=true,reason=valid). The earlier second commit only renamed unchanged changelog bytes to this PR number. Current-head hosted CI run 40165960501 passed 4,628 units. Lint run 40165960499 passed; complete logs and artifact 12587788708 were inspected with matching SHA-256/ZIP CRC and zero findings across three SARIF/XML reports. The manifest warning matches the actual base and previous head. Current-head E2E run 40165960710 passed 39 cases across seven shards on their first attempts. The complete 6,672-line log was inspected. This is the existing E2E suite, not the new authenticated cleanup fault journey.Models used
gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)