Skip to content

fix: retain recovered contact retries - #1455

Open
ben-kaufman wants to merge 1 commit into
masterfrom
codex/android-recovered-peer-retry-20261009
Open

ben-kaufman wants to merge 1 commit into
masterfrom
codex/android-recovered-peer-retry-20261009

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

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

  • Clears the missing-peer result when fresh scheduling reads show the contact linked, restoring readiness work only within its original foreground window.
  • Preserves queued delivery and temporary receive failures so readiness is refreshed after successful intake.

Out of Scope

Design

N/A — no UI changes.

Preview

N/A

QA Notes

Journeys

Reviewer background/resume journey passed at 25f206a on 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

  • regression: retain a missing contact's retry with queued delivery → let the peer become linked, drain delivery, and fail intake once within the foreground window → intake retries and refreshes eligibility only after success — controlled peer-state and receive-failure injection not in Capabilities.
  • Fail identity inspection after the missing-peer response → recover identity and link state within the same foreground window, then fail intake once → intake retries without extending the window or using an unverified identity — controlled identity failure and retry inspection not in Capabilities.

The contacts suite documents the recovered-peer case. These device checks have not been run for this follow-up.

Automated Checks

  • added PrivatePaykitRepoTest.kt — recovered peers retain failed intake after queued delivery or a failed identity read; successful intake refreshes eligibility once and completes the retry.
  • ran both regressions against the unchanged retry implementation — they fail before the correction and pass afterward.
  • ran complete diagnostic comparisons against the merged master baseline — all 20 Detekt/format findings and emitted compiler warning sources are pre-existing; none introduced.

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.

@greptile-apps

greptile-apps Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

Summary

Restores contact readiness retries when a previously unavailable contact becomes linked.

  • Foreground retries resume intake when a missing contact becomes linked.

No actionable issues found. This review inspected code and tests; it did not run tests or device checks.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Read current contact state] --> B{Missing contact now linked?}
  B -- Yes --> C[Clear missing result]
  C --> D{Original foreground window open?}
  D -- Yes --> E[Restore readiness retry]
  D -- No --> F[Keep only remaining delivery work]
  B -- No --> F
  E --> G[Attempt intake]
  G -- Failure --> H[Retry while window remains open]
  H --> A
  G -- Success --> I[Refresh eligibility]
  I --> J[Finish when no work remains]
Loading

Reviews (1) · Last reviewed commit: "fix: retain intake retries for recovered..." · Reviewed by Greptile

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Regtest APK

Built from 25f206a (run).

Download bitkit-dev-debug universal APK (expires in 30 days).

@talosmachina talosmachina left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: refreshReadiness is set only when interactiveUntil is still in the future, and interactiveUntil is not touched, so an expired retry stays background-only and never calls refreshEligibleTarget.
  • Side effect leaking into non-retry callers: restoreLinkedPeerReadiness returns the argument unchanged without a PrivateMessageDrainRetry in the coroutine context, so drainAndSchedulePrivateLinkRetries and endpoint-withdrawal drains are unaffected.
  • Flip during the post-drain pending check returning stale readinessPending: if the last read in drainPrivateMessageRetry restores the peer, the restored receiveLinkedPeers makes the linked key pending, so the loop keeps going and the next iteration refreshes readiness.
  • Unverified identity used after recovery: the identity check in drainPrivateMessageRetry still 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 piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, refreshEligibleTarget is not called, and the failed intake does not keep a foreground readiness retry alive.

@jvsena42 jvsena42 left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.xml passed 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.

⚠️ Not reached on a device: a missing peer that later links. The staging homeserver answers a missing contact with a transport error, not NotFound, so the case this PR fixes rests on the unit tests. CI is green at this head.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants