Repository navigation
fix: defer registry sync during profile deletion - #1538
ben-kaufman wants to merge 1 commit into
Conversation
 [Medium impact] The PR appears safe to merge; no actionable issues were found. SummaryFull profile deletion now skips the intermediate registry update after withdrawing private payment lists. Final public cleanup still attempts the disabled-capability update.
Reviews (1) · Last reviewed commit: "fix: defer registry sync during profile ..." · Reviewed by Greptile |
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. Defers the App Registry sync in full profile deletion to the final public cleanup in signOut, and keeps the private cleanup marker pending until closeAndClear succeeds. Reviewed d689900, full tier.
What I checked, and 4 candidates I ruled out
Read in full: removePublishedEndpointsForCleanup, retryPendingEndpointRemoval, syncPaykitAppAfterCleanup, closeAndClear (PrivatePaykitRepo.kt); attemptDeleteProfile / disconnectProfile (EditProfileViewModel.kt); deleteProfileWithSessionRetry, deleteProfile, signOut, clearLocalState (PubkyRepo.kt); removeBitkitPaymentEndpoints (PubkyService.kt); syncPaykitApp (PublicPaykitRepo.kt, PaykitSdkService.kt); retryPendingPaykitEndpointRemoval (AppViewModel.kt).
Call sites traced: removePublishedEndpointsForCleanup has four callers (EditProfileViewModel delete and disconnect, ProfileViewModel:168, WipeWalletUseCase:91). Only full deletion passes deferAppSync = true; the rest keep the single-argument overload, which behaves as before.
CI: lint, detekt and build-local green. build and the e2e-tests-local shards were still running when I reviewed. On the Bitkit track this box does not build Android branches, so this review comes from reading the code and from CI.
Ruled out
- The deferred sync drops a capability update the registry needed: no. The removed intermediate call is
publicPaykitRepo.syncPaykitApp()with no argument, which resolvesprivateSharingEnabledfromsettings.sharesPrivatePaykitEndpoints. That setting is stilltrueat that point, becausepaykitDisabled()only runs insidedeleteProfile. So the old call republishedprivatePayments = true. The capability actually changes inremoveBitkitPaymentEndpoints(syncPaykitApp(privatePaymentsEnabled = false)), and that call still runs duringsignOut. - The marker stays set after a successful deletion: no.
closeAndClearrunscacheStore.reset(), which bringscleanupPendingback to its defaultfalse. The new repo test asserts this. - A failed deletion strands private payments: no. The marker stays
true, soretryPendingEndpointRemoval(from the maintenance tick andrefreshPrivatePaykitEndpointsIfEnabled) withdraws again, syncs the registry and clears it. That path is unchanged and is covered by the persisted-marker retry test. Until that tick, contact preparation stays paused, which is the behaviour the PR describes. - Public cleanup fails and nothing retries the registry: no. When
removeBitkitPaymentEndpointsfails,signOutcallsclearLocalState(publicPaykitCleanupPending = true).hadPaykitStatestays true there becausedeleteProfilesetspublicPaykitCleanupPendingfirst. This is the same handling as before the change.
Merge confidence: 4/5. No findings, but build and the e2e shards had not finished at review time and the branch was not built here.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Reviewed the full PR diff against its merge base, at d6899001.
No new actionable code findings.
Full profile deletion withdraws private payment lists and leaves the private cleanup marker pending, so the intermediate App Registry sync no longer runs while sharing is still enabled. Final public cleanup still removes public endpoints and publishes the disabled private-payments capability before session revocation. A failed, cancelled, or interrupted deletion keeps that marker for a later retry, and a failed retry retains it. Disconnect, wallet wipe, and disabling private sharing still sync immediately.
Compared with bitkit-ios#979 at b948a469: both defer that sync only for full deletion. iOS keeps the public repair marker and checks again before public reconciliation; Android keeps the private cleanup marker and does not enter retry once deletion is already active. Successful deletion still clears the private marker by resetting the cache after public cleanup.
Validation: inspected PrivatePaykitRepoTest, PubkyServiceTest, and EditProfileViewModelTest; did not execute them. Build and lint passed on this revision.
Device testing: not performed in this review.
Ready for device testing.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at d689900: no findings. I reviewed it as a Paykit endpoint-cleanup change.
Checked:
- The skipped intermediate call published
privatePayments = true, so deferring it loses nothing. The onlyfalsepublish is the final cleanup, which still runs before the session is revoked:PubkyRepo.deleteProfile():1069→signOut():1751-1772→PubkyService.removeBitkitPaymentEndpoints():66-77. The newPubkyServiceTestasserts the ordersyncPublicEndpoints(empty)→syncPaykitApp(false)across failures. - The marker is set at
PrivatePaykitRepo.kt:492and kept through failure and cancellation (:494-499;cancelled profile withdrawal preserves cleanup for retry). It is retried from foreground, network and the 30/60 s maintenance tick, and cleared bycloseAndClear()→cacheStore.reset()on success. - A deferred sync cannot acknowledge an undelivered withdrawal: non-empty
pendingOutboundPrivateCounterpartiesthrowsPrivateUnavailablebefore thedeferAppSyncbranch (:1836-1846). - Retries are gated during deletion (
:399), andendProfileDeletionruns infinallyunderNonCancellable. Back is blocked while busy. - The other callers (
disconnectProfile,ProfileViewModel,WipeWalletUseCase) keepdeferAppSync = false. - This does not change the
policy_error … private_paymentsin synonymdev/bitkit-ios#984 either way.
Performance-relevant: it removes one publishPaykitApp round trip from every full deletion.
Device gate (partial): d689900 — not run yet. Pending: delete-profile.xml with one publishPaykitApp per deletion in the log, and the profile not returning on reopen. The device result will follow.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approving at d689900. The code scan from my earlier review stands, with no findings.
Device gate: d689900 — delete-profile.xml: 7/7 actions pass on Android, using the CI APK and a fresh profile with Paykit on and 0 contacts. During deletion a spinner replaces Delete Profile, and Save and Cancel are disabled with their labels visible. Onboarding appears when it finishes. The profile does not return on reopen or after a force-stop and relaunch (Found no saved paykit session). No crash or ANR. iOS twin synonymdev/bitkit-ios#979 passes the same journey, with exactly one Paykit app publish during deletion.
perf: delete profile ~28.7 s at 0 contacts (19:33:37.8 → 19:34:06.5 UTC). Earlier Android runs measured ~31–35 s on this setup. l10n: no UI text changed.
publishPaykitApp (0 in the window), never a successful one. Confirming exactly one needs a debug log at the call site or homeserver request logs. The approval rests on the code trace and PubkyServiceTest for the order. The 62-contact variant was not run either.
![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: none.
Pair PR synonymdev/bitkit-ios#979 at b948a46 matches: it defers the same sync only for full deletion, and keeps a public repair marker where Android keeps the private one.
QA for d689900:
Tested on two Android 15 emulators on staging: wallet A deletes, wallet B is its linked contact. The build is d689900 plus debug-only log lines at each App Registry publish and cleanup-marker change, two holds and a contact seed (patch below, not pushed). Registry update counts and marker states come from those lines and the app's private Paykit cache.
Test 1: full deletion updates the App Registry once ✅ With one published private list, deletion made one registry update (the final privatePayments=false) where base eb32cdd made two (true, then false), and the profile stayed gone on reopen.
case1.2x.mp4
Base eb32cdd with the same log lines, on wallet B:
base-eb32cdd.2x.mp4
Tap to onboarding took 34 s, with the spinner on Delete Profile and Save and Cancel disabled. The deletions in tests 3 and 4 also made exactly one registry update each.
Test 2: deletion interrupted after the private withdrawal ✅ Killed after a successful private step and before profile removal, the app kept cleanup pending (base clears it at that point), and the first retry that reached the homeserver made one registry update and cleared the marker, with the profile intact.
case2a.2x.mp4
case2b.2x.mp4
The first run had no contacts and relaunched offline: the retries failed and kept the marker until the network came back. The second run withdrew B's published list at the hold and relaunched online: two retries met the write lock the killed process left (shared_state_busy), and the third, about 2 minutes after relaunch, reconciled. A then published its private list for B again.
Test 3: deletion interrupted after profile removal ✅ Killed after the profile was removed and before local state was cleared, the app relaunched with no profile (Profile showed onboarding), and the leftover marker only made background retries fail until a new profile cleared it and published the registry.
case3.2x.mp4
Without a session each retry failed at once (identity_error) on foreground and USDT-availability triggers, with nothing on screen. Setting up the new profile cleared both the private and the public cleanup markers.
Test 4: deletion with 62 contacts ✅ With 62 imported contacts, deletion made one registry update and reached onboarding in 25 s once preparation had settled, and in 68 s when started 8 s after the import.
case4a.2x.mp4
case4b.2x.mp4
The 62 are random keys imported through PubkyRepo.importContacts, so none linked and the private step had nothing to withdraw. The 68 s run is deletion competing with preparation of the just-imported contacts, the latency #1522 tracks; the spinner stayed visible throughout. No uncertain-write wait appeared in the app or Paykit log.
Test 5: turning off contact payments and Disconnect ✅ Turning contact payments off and Disconnect each still updated the registry right after the private withdrawal.
case5.2x.mp4
Disconnect was reached from the Unable to Delete Profile dialog after a deletion attempted offline. That failed deletion kept cleanup pending and the retry on reconnect cleared it; Disconnect's own cleanup then synced the registry before the final privatePayments=false update.
Test 6: undelivered private withdrawal ✅ Right after re-linking with B, the SDK reported the withdrawal to B still pending, and the deferred step failed with PrivateUnavailable, kept the marker and the published-list flag, and made no registry update until a later retry delivered it.
case6.2x.mp4
The pending withdrawal shows only in the app log and private cache; the recording shows the deletion attempt held at that point.
test hook
Not pushed. Logs each App Registry publish with its privatePayments value, each cleanup-marker change and each deletion step; holds before deleteProfileWithSessionRetry or closeAndClear when a flag file exists; seeds N contacts when Edit Profile opens.
diff --git a/app/src/main/java/to/bitkit/repositories/PrivatePaykitRepo.kt b/app/src/main/java/to/bitkit/repositories/PrivatePaykitRepo.kt
index 1afcf97e..50786baf 100644
--- a/app/src/main/java/to/bitkit/repositories/PrivatePaykitRepo.kt
+++ b/app/src/main/java/to/bitkit/repositories/PrivatePaykitRepo.kt
@@ -396,10 +396,12 @@ class PrivatePaykitRepo @Inject constructor(
savedPublicKeys: Collection<String>,
): Result<Unit> = withContext(serializedDispatcher) {
runSuspendCatching {
+ android.util.Log.i("TestHook", "retryPendingEndpointRemoval isDeletingProfile=$isDeletingProfile") // test hook
if (isDeletingProfile) return@runSuspendCatching
val settings = settingsStore.data.first()
val hasDisabledPublications = !settings.sharesPrivatePaykitEndpoints && hasPublishedPrivateEndpoints()
val cleanupPending = isContactSharingCleanupPending()
+ android.util.Log.i("TestHook", "retryPendingEndpointRemoval cleanupPending=$cleanupPending hasDisabledPublications=$hasDisabledPublications") // test hook
if (cleanupPending || hasDisabledPublications) {
if (!cleanupPending) updateContactSharingCleanupPending(true)
removePublishedEndpoints().getOrThrow()
@@ -408,7 +410,9 @@ class PrivatePaykitRepo @Inject constructor(
updateContactSharingCleanupPending(false)
}
retryPendingDeletedContactEndpointRemoval(savedPublicKeys).getOrThrow()
+ android.util.Log.i("TestHook", "retryPendingEndpointRemoval done") // test hook
}.onFailure {
+ android.util.Log.i("TestHook", "retryPendingEndpointRemoval failed: ${it.message}") // test hook
Logger.warn("Failed to retry pending Paykit contact endpoint removal", it, context = TAG)
}
}
@@ -502,6 +506,7 @@ class PrivatePaykitRepo @Inject constructor(
}
suspend fun closeAndClear(): Result<Unit> = withContext(serializedDispatcher) {
+ android.util.Log.i("TestHook", "closeAndClear") // test hook
runSuspendCatching {
invalidateContactPreparation()
publicationMutex.withLock {
@@ -1897,7 +1902,7 @@ class PrivatePaykitRepo @Inject constructor(
}
private suspend fun syncPaykitAppAfterCleanup(): Result<Unit> =
- publicPaykitRepo.syncPaykitApp().onFailure {
+ publicPaykitRepo.syncPaykitApp().also { android.util.Log.i("TestHook", "syncPaykitAppAfterCleanup ok=${it.isSuccess}") }.onFailure { // test hook
runSuspendCatching { settingsStore.update { it.copy(publicPaykitCleanupPending = true) } }
.onFailure(it::addSuppressed)
}
@@ -2114,6 +2119,7 @@ class PrivatePaykitRepo @Inject constructor(
isStillCurrent: (() -> Boolean)? = null,
) {
if (isPending) invalidateContactPreparation()
+ android.util.Log.i("TestHook", "cleanupPending <- $isPending") // test hook
cacheStore.update {
if (isStillCurrent?.invoke() == false) it else it.copy(cleanupPending = isPending)
}
diff --git a/app/src/main/java/to/bitkit/services/PaykitSdkService.kt b/app/src/main/java/to/bitkit/services/PaykitSdkService.kt
index cca86961..854a8aa1 100644
--- a/app/src/main/java/to/bitkit/services/PaykitSdkService.kt
+++ b/app/src/main/java/to/bitkit/services/PaykitSdkService.kt
@@ -806,6 +806,7 @@ class PaykitSdkService @Inject constructor(
operationLock.withLock(priority) {
withStateRevisionTracking { handle ->
val capabilities = appCapabilities(completeSdkCall { handle.identityStatus() })
+ android.util.Log.i("TestHook", "publishPaykitApp site=syncPaykitApp privatePayments=${capabilities.privatePayments && privatePaymentsEnabled} requested=$privatePaymentsEnabled caller=${Throwable().stackTrace.drop(1).filter { it.className.startsWith("to.bitkit") && !it.className.contains("PaykitSdkService") }.take(4).joinToString(" < ") { it.className.substringAfterLast('.') + "." + it.methodName + ":" + it.lineNumber }}") // test hook
completeSdkCall {
handle.publishPaykitApp(
displayName = "Bitkit",
@@ -1448,6 +1449,7 @@ class PaykitSdkService @Inject constructor(
val capabilities = appCapabilities(identityStatus)
if (capabilities.privatePayments) {
val privatePayments = settingsStore.data.first().sharesPrivatePaykitEndpoints
+ android.util.Log.i("TestHook", "publishPaykitApp site=publishAppIfLiveSessionAvailable privatePayments=$privatePayments") // test hook
completeSdkCall {
handle.publishPaykitApp("Bitkit", capabilities.copy(privatePayments = privatePayments))
}
diff --git a/app/src/main/java/to/bitkit/services/PubkyService.kt b/app/src/main/java/to/bitkit/services/PubkyService.kt
index 3ef8918b..21fc626c 100644
--- a/app/src/main/java/to/bitkit/services/PubkyService.kt
+++ b/app/src/main/java/to/bitkit/services/PubkyService.kt
@@ -64,6 +64,7 @@ class PubkyService @Inject constructor(
}
suspend fun removeBitkitPaymentEndpoints() = ServiceQueue.CORE.background {
+ android.util.Log.i("TestHook", "removeBitkitPaymentEndpoints") // test hook
val endpointError = runSuspendCatching {
val report = paykitSdkService.syncPublicEndpoints(emptyList())
if (report.failed.isNotEmpty()) throw AppError("Failed to remove Paykit payment endpoints")
diff --git a/app/src/main/java/to/bitkit/ui/screens/profile/EditProfileViewModel.kt b/app/src/main/java/to/bitkit/ui/screens/profile/EditProfileViewModel.kt
index b8da23ac..ca830890 100644
--- a/app/src/main/java/to/bitkit/ui/screens/profile/EditProfileViewModel.kt
+++ b/app/src/main/java/to/bitkit/ui/screens/profile/EditProfileViewModel.kt
@@ -48,8 +48,28 @@ class EditProfileViewModel @Inject constructor(
init {
loadProfile()
+ seedTestContacts() // test hook
}
+ private fun seedTestContacts() { // test hook
+ val file = java.io.File(context.filesDir, "test-hook-seed-contacts") // test hook
+ if (!file.exists()) return // test hook
+ val keys = file.readLines().map { it.trim() }.filter { it.isNotEmpty() } // test hook
+ file.delete() // test hook
+ viewModelScope.launch { // test hook
+ android.util.Log.i("TestHook", "seed: importing ${keys.size} contacts") // test hook
+ val result = pubkyRepo.importContacts( // test hook
+ keys.mapIndexed { i, k -> // test hook
+ to.bitkit.models.PubkyProfile( // test hook
+ publicKey = k, name = "Seed ${i + 1}", bio = "", imageUrl = null, // test hook
+ links = emptyList(), status = null, // test hook
+ ) // test hook
+ }, // test hook
+ ) // test hook
+ android.util.Log.i("TestHook", "seed: import ok=${result.isSuccess} ${result.exceptionOrNull()?.message}") // test hook
+ } // test hook
+ } // test hook
+
private fun loadProfile() {
viewModelScope.launch {
_uiState.update { it.copy(isLoading = true) }
@@ -269,9 +289,22 @@ class EditProfileViewModel @Inject constructor(
}
try {
privatePaykitRepo.beginProfileDeletion()
- privatePaykitRepo.removePublishedEndpointsForCleanup(TAG, deferAppSync = true)
+ android.util.Log.i("TestHook", "delete: start") // test hook
+ val cleanup = privatePaykitRepo.removePublishedEndpointsForCleanup(TAG, deferAppSync = true) // test hook: keep result
+ android.util.Log.i("TestHook", "delete: private withdrawal done ok=${cleanup.isSuccess}") // test hook
+ if (java.io.File(context.filesDir, "test-hook-hold-delete").exists()) { // test hook
+ android.util.Log.i("TestHook", "delete: HOLD before deleteProfileWithSessionRetry") // test hook
+ while (!java.io.File(context.filesDir, "test-hook-release").exists()) kotlinx.coroutines.delay(500) // test hook
+ android.util.Log.i("TestHook", "delete: released") // test hook
+ } // test hook
val result = pubkyRepo.deleteProfileWithSessionRetry()
+ android.util.Log.i("TestHook", "delete: deleteProfileWithSessionRetry ok=${result.isSuccess}") // test hook
if (result.isSuccess) {
+ if (java.io.File(context.filesDir, "test-hook-hold-clear").exists()) { // test hook
+ android.util.Log.i("TestHook", "delete: HOLD before closeAndClear") // test hook
+ while (!java.io.File(context.filesDir, "test-hook-release").exists()) kotlinx.coroutines.delay(500) // test hook
+ android.util.Log.i("TestHook", "delete: released") // test hook
+ } // test hook
privatePaykitRepo.closeAndClear()
ToastEventBus.send(
type = Toast.ToastType.SUCCESS,Not driven: the zero-contact delete-profile.xml run, validated by @jvsena42 at d689900 (review).
Review: full, eb32cdd...d689900, 8 files. Every changed app line ran in tests 1 to 6; the unit tests, changelog and journey notes were read, not run.
This branch has not been deployed
5 participants
Closes #1539
Related to #1522 and pubky/paykit-rs#197. This is a partial improvement, not a complete fix for deletion latency.
Twin: synonymdev/bitkit-ios#979
This PR avoids an intermediate App Registry update during full profile deletion.
Description
Out of Scope
Design
N/A - no UI changes.
Preview
N/A - no visual changes.
QA Notes
Journeys
delete-profile.xml- record zero-contact deletion time, then repeat the existing 62-contact cases; progress remains visible and reopening Profile does not restore the deleted profile.Manual Tests
Automated Checks
PrivatePaykitRepoTest.kt- private withdrawal, deferred registry reconciliation, persisted-marker retry, cancellation and undelivered withdrawal retention.PubkyServiceTest.kt- final public cleanup attempts the disabled-capability update after endpoint removal, including endpoint and registry failures.EditProfileViewModelTest.kt- only full deletion opts into deferral, before profile removal; failure retains private state.Local validation: 366 focused tests and all 4,256 native tests across 239 suites passed, with zero failures/errors/skips. Compile and dev APK build passed. Full Detekt, including formatting, has exactly the same 1,401 findings as an independently run exact-master baseline (
eb32cdd7); no introduced compiler diagnostics. Five tests added, none removed. Frozen independent correctness, repository-native and polish review found no actionable issue.The controlled repository retry test reuses a persistent-store fixture; it is not an actual process-restart fault test. Author-run device fault-injection and broader populated private-contact acceptance remain incomplete; qualified subsequent reviewer evidence is recorded below. No funds were sent and no safety wait was shortened.
Device timing limitation: the baseline zero-contact flow reached onboarding between 47.29 and 52.54 seconds (coarse accessibility polling). After installing the final candidate without clearing data, the deleted profile stayed absent on restart. Candidate timing was then blocked before deletion by an input-dispatch ANR while typing a new profile name. Its main-thread stack was in Compose keyboard-modifier dispatch, with emulator CPU pressure; the creation code is unchanged, but the exact ANR cause is not established. The failed run was stopped and preserved, not counted as a deletion pass. No Android before/after speedup is claimed.
Reviewer Device Evidence
Ovi review 5554760779 approves exact
d6899001with no findings. The reviewer used two Android 15 emulators on staging, with disclosed uncommitted registry/marker logging, deletion holds and a contact-import fixture. The reviewer reports:privatePayments=falseApp Registry update versus two updates on instrumented baseeb32cdd. The deleted profile remained absent after reopening. Counts came from the inserted logging and private cache, not a parent HTTP trace.shared_state_busy, and a later retry reconciled about two minutes after relaunch. The retained identity then republished its private list.identity_errorretries without a session until creating a new profile cleared both cleanup markers. This is an observed recovery limitation, not evidence that those retries never occur.PrivateUnavailable, retained the pending marker and published-list flag, and made no intermediate registry update before later delivery/retry.These are attributed reviewer results, not author replication. The 34s linked-contact sample and 25s/68s imported-contact samples are not a matched before/after timing benchmark or a maximum-duration guarantee. The zero-contact
delete-profile.xmljourney was carried from jvsena's review 5552784324, which reported 7/7 actions and 28.7s on the CI APK, not rerun by Ovi. The instrumented review did not execute the native unit suite or journey file. Author fault-injection replication and broader populated-contact acceptance remain incomplete; the compound manual checkboxes remain open.