Repository navigation
fix: stop automatic profile deletion replay - #1688
ben-kaufman wants to merge 2 commits into
Conversation
|
Triage: 3.1.0. It fixes shipped profile deletion, and 3.1.0 is due 28 Oct. |
 [Medium impact] This narrow change appears safe to merge; no actionable issues were found. SummaryRemoves automatic replay of profile deletion after an error.
Reviews (1) · Last reviewed commit: "fix: stop automatic profile deletion rep..." · 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. Removes the automatic session-refresh-and-redelete after a failed profile deletion, so EditProfileViewModel calls PubkyRepo.deleteProfile() once and an uncertain failure lands on the existing failure dialog. Reviewed 99b446a, full tier, as the delta on fix/restore-identity-background (stacked on #1686).
What I checked, and 3 candidates I ruled out
Read in full: PubkyRepo.deleteProfile and deleteAllContacts, EditProfileViewModel.attemptDeleteProfile, retryDeleteProfile and disconnectProfile, the rewritten PubkyRepoTest cases
Call sites traced: deleteProfileWithSessionRetry and refreshSessionIfPossible have no caller left under app/src/main (grep on the PR head)
CI: build (debug build and unit tests) and lint green; e2e shards still running when reviewed
Ruled out
- An expired session can no longer be refreshed inside deletion, so Retry keeps failing: deliberate and stated in Out of Scope; the failure dialog still offers Retry and Disconnect, and session restore through
initialize()is unchanged (the renamed contact-edit test now goes through it). - Retry re-runs contact cleanup: it did before too;
deleteAllContactsis unchanged by this diff. - The sign-out race test passing for the wrong reason:
deletePaykitProfileis held on a deferred untilsignOut()returns, and the test assertssignInis never called and the identity stays cleared.
Merge confidence: 5/5, build, unit tests and lint green, and PubkyRepoTest covers the single-delete path and the sign-out race.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: no findings.
Checked and clean:
- With the replay gone, a failed deletion surfaces the failure dialog after one attempt. Local identity is cleared only inside
signOut(), after the profile delete and the public-endpoint removal, so an interrupted deletion leaves the user signed in with a retryable state and no orphaned remote data from this path. - v3.0.0 holds no persisted pending-deletion state (
PrivatePaykitRepo.kt:130is an in-memory var), so there's nothing to migrate. - Nothing still references the removed
deleteProfileWithSessionRetry/refreshSessionIfPossible, and startup session recovery stays inresolveSessionInitialization. - The new
PubkyRepoTestcases pin one call per tap and nosignIn. - Twin synonymdev/bitkit-ios#1122 does the same. #1690 edits the same lines and builds on this one.
Device gate (partial): 99b446a — not run yet. profile/delete-profile-uncertain-response.xml needs a homeserver fault proxy. I'll approximate it: airplane mode during the delete, expecting the failure dialog once, exactly one Failed to delete profile and no re-sign-in or second delete; Cancel keeps the profile; Retry online completes; Disconnect from the dialog. A plain successful deletion runs too. Device results will follow in a separate review.
There was a problem hiding this comment.
Approving at 99b446a. My code scan stands with no findings, and the device gate found none.
Device gate: 99b446a — Pixel_Tablet, fresh wallet and disposable identity, staging Pubky. journeys: profile/delete-profile-uncertain-response.xml 6/6 actions held, run as an airplane-mode approximation (airplane mode ~0.45 s after "Yes, Delete"). perf: not relevant; a plain deletion takes 8.9 s from confirm to keys cleared, on the unchanged success path. Figma: n/a; l10n: no UI text changed.
- One failure shows "Unable to Delete Profile" with Disconnect / Retry. Through a 65 s wait there's exactly one
Failed to delete profile, no second contact cleanup, no delete call and no sign-in. Cancel keeps the profile, and a relaunch runs only the normal session restore, with no deletion. - Each Retry makes exactly one attempt (3 failures for 3 taps, none automatic). The online Retry completed and reached onboarding.
- Disconnect offline fails at private endpoint removal and keeps the profile, as on master. Online, it signs out.
- For the record, not a finding: a Retry within ~60 s of an interrupted delete fails with
shared_state_busyuntil the homeserver lock expires. The removed automatic retry would have hit the same lock, and the follow-up sits in #1687.
Skipped — covered by CI at 99b446a: unit tests, lint, detekt, e2e (all green).
![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.
QA for 99b446a: Pixel 7 emulator, Android 16 (API 36), disposable unfunded wallet and local Pubky testnet identity.
Test 1: J1 — lost profile deletion acknowledgement ✅ Failure appears once; dismissal and cold restart do not replay deletion.
For J1, a debug fixture provisioned the disposable identity and two contacts, then I drove the ordinary profile editor with Paykit 0.1.1-rc1. The proxy log confirms that the profile DELETE returned HTTP 204 before its response was withheld beyond the SDK timeout. No SDK result or deletion decision was substituted.
“Unable to Delete Profile” appeared after about 31 seconds. The recording shows no success toast or onboarding navigation. Dismissing the dialog with Android Back retained the editor. The app trace records one contact cleanup, one profile deletion and one failure, with no sign-in call.
I then force-stopped the app and launched it normally. Home appeared, normal stored-session restoration ran, and the trace contained no additional deletion during the 85-second restart observation.
| Failure and dismissal | Cold restart |
|---|---|
j1-repeat-failure.2x.mp4 |
j1-repeat-restart.2x.mp4 |
| Accepted DELETE, timeout and dialog dismissal, at 2× speed. | Normal launch and observation without Retry, at 2× speed. |
Check 1: build and repository checks ✅ The unmodified head passes the build and required checks.
assembleDevDebug, just compile, just test and just lint pass. I inspected the finalized unit reports and the Detekt and Android Lint reports: no test failures or skips, and no lint findings. The journey XML and git diff --check also pass.
I relied on the earlier Android device report at this same head for interrupted requests, manual Retry/Disconnect and successful deletion; I did not repeat those cases.
Full review of ac2a7c6..99b446a, all six changed files. J1 was driven in full. The removed helper call graph and sign-out interleaving were checked in source and unit tests; manual cleanup ownership remains outside this change, tracked in bitkit-android#1687 Bind profile deletion cleanup to the original sign-in.
Twin: bitkit-ios#1122 fix: stop automatic profile deletion replay matches the removal of automatic full-flow replay.
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: Reviewed the full PR diff against its merge base, at 99b446a5 (base and merge base ac2a7c66, fix/restore-identity-background). This is a first full assessment; no baseline coverage is inherited.
No new actionable code findings.
No new actionable code findings. After the change, EditProfileViewModel.attemptDeleteProfile calls PubkyRepo.deleteProfile() once. Any failure, including a lost acknowledgement, now shows the existing failure dialog. Before, the app would sign in again and run contact cleanup and profile deletion a second time. Only that wrapper called the removed refreshSessionIfPossible. Startup re-sign-in in initializeSession / resolveSignedInSession is unchanged. The cost is the trade-off the PR describes: when the homeserver session has expired, deletion now fails until normal session recovery or re-authentication. Manual Retry and ownership of the original sign-in during cleanup stay out of scope, tracked in #1687.
Parity: bitkit-ios#1122 @ ca10ace7 also removes the automatic refresh and second deletion from EditProfileView. Its delete-profile-uncertain-response.xml journey is byte-identical to this one. I compared source only, not runtime behaviour.
Dependency: the head of #1686 has moved past this PR's base, to 0fd5e424. It does not add callers of the removed functions. A local test merge of this head with it was clean, and no references to deleteProfileWithSessionRetry / refreshSessionIfPossible remained.
Validation: no tests or builds were run in this review. The matching CI runs at 99b446a passed (CI with unit tests, Lint, E2E). Those E2E suites do not run J1. The new repository tests check that the error comes back unchanged and that no sign-in follows a failure, including after sign-out. The ViewModel failure test checks that endpoint cleanup runs exactly once. J1 needs a fault-injecting homeserver proxy, which no automated layer has; making that proxy available would allow it to be automated.
Device testing: not performed in this review. Device runs of J1 at 99b446a are reported by other reviewers (jvsena42, ovi-reviewer); they are not this reviewer's executions.
Suggested additional test cases:
- Android, expired session: use a disposable Bitkit-created profile whose homeserver session has been revoked on the server, with the local Pubky secret key still present. Open Edit Profile, then Delete, then confirm. Expected: the deletion request is sent once, the failure dialog appears, and there is no automatic sign-in or second deletion. Then force-stop and relaunch the app, wait for profile initialization, and delete again. Record whether normal startup recovery lets the explicit deletion succeed, or whether re-authentication is needed, as the PR's out-of-scope note describes. Disconnect must still return to profile onboarding.
- Android, Ring-linked identity: use a disposable identity adopted from Pubky Ring and inject the same lost acknowledgement during Unlink pubky. Expected: the unlink failure dialog appears once, no automatic second unlink or sign-in happens, and no success toast is shown.
Reviewer model: claude-opus-5-5
Reasoning effort: high
Twin: synonymdev/bitkit-ios#1122
Dependency: #1686
Description
Out of Scope
Design
N/A — existing progress, failure options and navigation.
Preview
An uncertain deletion failure remains a failure instead of silently repeating contact cleanup and profile deletion.
QA Notes
Setup
Use committed released dependencies and synthetic identities for unit tests. J1 requires a disposable test identity and a homeserver fault proxy that accepts one profile deletion, drops its acknowledgement and retains a redacted request counter. That complete-app fixture is unavailable; do not use a personal profile, reset wallets or use live funds.
Journeys
delete-profile-uncertain-response.xml— lost acknowledgement, honest failure feedback, no automatic deletion/sign-in replay, cancellation and restart without selecting Retry.Automated Checks
just compile,just testandjust lintpassed. All 4,623 tests across 254 reports passed with zero failures/errors/skips. Complete Detekt SARIF and Android Lint XML/SARIF contain zero findings; all Gradle problem diagnostics match the unchanged base.Evidence and gaps
Android's pre-fix repository suite executed 223 tests with two expected counterexamples: a lost deletion response causes a second delete and false success, and the old refresh helper can authenticate an identity after sign-out. Removing the helper removes its sole production entry. The repaired 219-test repository suite passes, including one-delete/error preservation and held deletion failure after sign-out. The remaining manual deletion/finalization ownership issue is separate.
The iOS UI and helper call graph has the same automatic replay, and the patch removes that call path while preserving its existing ownership and failure guards. The full native suite is regression coverage; it is not an executed iOS view/proxy lost-acknowledgement journey. J1 remains unverified on both platforms. No historical contact-failure attribution is claimed.
Models used
gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)