Repository navigation
fix: retain recovered contact retries - #1455
ben-kaufman wants to merge 1 commit into
Conversation
|
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Clears a retry's missing-peer flag when a fresh link-state read shows the contact linked, so failed intake keeps retrying and readiness is refreshed only inside the original foreground window. Reviewed 25f206a, full tier, reasoned from the code and CI (Bitkit Android branches are not built here).
What I checked, and 4 candidates I ruled out
Read: PrivateMessageDrainRetry, promoteExplicitLinkRetry, runPrivateMessageDrainRetries, drainPrivateMessageRetry, hasPendingPrivateMessageRetry, pendingPrivateMessageDrainKeys, privateMessageDrainState, the link preparation paths that call stopIfUnavailable, and both new tests.
Call sites traced: all six callers of pendingPrivateMessageDrainKeys (the retry loop, drainAndSchedulePrivateLinkRetries, drainPendingEndpointWithdrawals).
CI: build (assembleDevDebug + testDevDebugUnitTest), build-local, lint, detekt and Greptile green; the local e2e matrix was still running and does not exercise contacts.
Ruled out
- Window extended for a recovered peer:
refreshReadinessis set only wheninteractiveUntilis still in the future, andinteractiveUntilis not touched, so an expired retry stays background-only and never callsrefreshEligibleTarget. - Side effect leaking into non-retry callers:
restoreLinkedPeerReadinessreturns the argument unchanged without aPrivateMessageDrainRetryin the coroutine context, sodrainAndSchedulePrivateLinkRetriesand endpoint-withdrawal drains are unaffected. - Flip during the post-drain pending check returning stale
readinessPending: if the last read indrainPrivateMessageRetryrestores the peer, the restoredreceiveLinkedPeersmakes the linked key pending, so the loop keeps going and the next iteration refreshes readiness. - Unverified identity used after recovery: the identity check in
drainPrivateMessageRetrystill runs before any drain; the second new test covers the failed-identity-read path.
Merge confidence: 4/5, the change is covered by the two new PrivatePaykitRepoTest.kt cases and green unit tests, but PrivatePaykitRepo.kt was read around the retry state machine rather than whole.
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full review of the complete PR diff against merge base b2d116d, at 25f206a.
No new actionable code findings.
A scheduling read that shows a missing contact as linked clears that missing result and resumes foreground intake only while the original 20-second window is still open. Identity is still checked before intake, and eligibility is refreshed only after a successful receive inside that window. The unavailable-peer cooldown stays in place. The same recovered-peer contract matches bitkit-ios#911 at 865c91c; that was a source comparison of this flow, not a full iOS review. iOS clears the missing result from the post-drain snapshot, and Android clears it on the scheduling read. Both wait for a verified identity before receiving or signaling readiness.
Validation: CI build on this revision passed testDevDebugUnitTest. I inspected the new PrivatePaykitRepoTest cases and did not re-run them locally. Device testing: not performed. The author manual cases need controlled SDK injection and are outside current journey-runner capabilities.
Suggested additional test cases
- Android unit test with a controlled clock. Setup: an explicit contact retry has already received NotFound, and same-peer outbound is still pending after the 20-second foreground window; then the peer becomes linked and intake fails. Action: let the next drain run. Expected: queued delivery can finish,
refreshEligibleTargetis not called, and the failed intake does not keep a foreground readiness retry alive.
There was a problem hiding this comment.
Approving at 25f206a: no finding.
restoreLinkedPeerReadiness clears the missing mark when the scheduling read shows the peer linked, and restores the readiness refresh only while the retry's original foreground window is still open, so it cannot extend that window. Queued delivery for the peer is kept as before. Both new tests fail intake once after the peer links and expect the retry to continue. This matches the iOS change in synonymdev/bitkit-ios#911.
One thing I looked at and left: the reset happens inside pendingPrivateMessageDrainKeys, which hasPendingPrivateMessageRetry also calls, so a query now updates the retry. Both callers run on the serialized dispatcher inside the retry's own coroutine, so I found no ordering problem.
Device gate: 25f206a — Pixel 9 emulator, dev flavor.
link-contact-after-resume.xmlpassed with a freshly created second identity on a tablet emulator. Save returned at once, Home for five seconds, reopened, then Pay about every 20 seconds. Request or Pay appeared 192 seconds after Save with Request enabled, Request opened the amount screen, and Contacts lists the contact once.- A contact with a valid key and no homeserver, watched for seven minutes: three failed link attempts in the first 90 seconds, all
transport_error, then two more just after the five-minute cooldown. Same as before this change.
NotFound, so the case this PR fixes rests on the unit tests. CI is green at this head.
This PR keeps foreground intake retrying when a previously missing contact becomes linked.
Follow-up to merged #1451, matching the correction in synonymdev/bitkit-ios#911.
Description
Out of Scope
Design
N/A — no UI changes.
Preview
N/A
QA Notes
Journeys
Reviewer background/resume journey passed at
25f206aon a Pixel 9 emulator with a fresh tablet identity. Save returned immediately; after five seconds at Home and resuming, Request/Pay was first observed 192 seconds after Save during checks about 20 seconds apart. Request opened the amount screen and the contact remained listed once.The same review observed transient transport retries for a no-homeserver contact, including retry after the existing cooldown. It did not reach NotFound or missing-to-linked recovery. These are functional observations, not isolated handshake timing or a measured speedup.
Manual Tests
The contacts suite documents the recovered-peer case. These device checks have not been run for this follow-up.
Automated Checks
PrivatePaykitRepoTest.kt— recovered peers retain failed intake after queued delivery or a failed identity read; successful intake refreshes eligibility once and completes the retry.Validation on master
b2d116d9: required compilation and all 3,689 unit tests in 217 suites passed. Independent focused review and the final helper recheck found no actionable issues. Current-head build, lint, Detekt and all seven E2E shards pass. The reviewer-reported device observations above do not replace the two controlled checks or establish a latency improvement.