Repository navigation
fix: defer automatic Paykit work during payments - #1626
ben-kaufman wants to merge 4 commits into
Conversation
|
Triage: milestone 3.1.0, with #1591 it is stacked on. It refers to issues rather than closing one, so this PR carries the milestone itself. Not a release blocker. |
 [Medium impact] The changes affect Paykit contact updates and recipient discovery around payments. Carry the background priority through automatic identity reads before merging. Findings
SummaryThis PR defers more automatic Paykit work during payment review and sending.
Review was based on code and test inspection; no tests were run. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Automatic contact work] --> B[Check payment hold]
B -->|Held| C[Keep work and retry later]
B -->|Clear| D[Read identity]
D --> E{Read priority}
E -->|Background| F[Check hold after SDK lock]
E -->|Ordered helper| G[Read can bypass a new hold]
F --> H[Prepare publication]
H --> I[Check hold before mutation]
I -->|Held| C
I -->|Clear| J[Publish private endpoints]
Reviews (1) · Last reviewed commit: "chore: rename changelog fragment" · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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 automatic (Background-priority) Paykit contact publication, link preparation and recipient discovery when a payment review or submission becomes active after the work queued but before SDK admission, and keeps the pending keys and retry intent so the work resumes afterward. Reviewed 92148c4 against fix/paykit-proof-latency, full tier, reasoned from the code and CI (Bitkit Android branches are not built on this reviewer's side).
What I checked, and 4 candidates I ruled out
Read in full: the changed regions of PaykitSdkService.kt (checkRefreshAdmission, identityStatus, linkedPeers, privateMessageSchedulingSnapshot, syncPrivatePaymentListsWithReservations, withStateRevisionTracking), PrivatePaykitRepo.kt (scheduleContactPreparation, contactPreparationPriority, publishLocalEndpoints, preparePrivateLinks, drainPendingPrivateMessages, advancePendingPrivateLinks, runPrivateMessageDrainRetries, readPrivateMessageDrainState), PaykitPaymentRequestRepo.refreshEligibleTargets, PaykitSdkOperationLock.
Call sites traced: every caller of the three reads that now check admission; every OperationInProgress rethrow added in PrivatePaykitRepo up to the handler that catches it.
CI: build (unit tests) green, lint and detekt green, Greptile green. e2e shards still running at review time; e2e-tests-local - lnurl_transfer failed in "Create AVD and generate snapshot for caching", an emulator setup step, not the change.
Ruled out
- Explicit and payment-time paths now deferred:
checkRefreshAdmissionthrows only forPriority.Background;prepareSavedContacts,handleOnchainActivity,refreshPrivateEndpointsBeforePaymentand the publicidentityStatus()/linkedPeers()default toOrdered, andrefreshEligibleTargetusesInteractive, so none of them can hit the new throw. - A rethrown
OperationInProgressescaping uncaught: every newthrow itsits under a Background caller whose top-level handler catches it,scheduleContactPreparation'sonFailure(requeues the keys, then waits 1 s) orrunPrivateMessageDrainRetries(resets the backoff and loops).retryScopeis aSupervisorJobwith a logging handler regardless. - Deferred preparation resuming for a reset or deleted profile: the requeue is gated on
generation == preparationGeneration && !isDeletingProfile, andinvalidateContactPreparationclearspendingPreparationKeys; covered bydeferred preparation cannot resume after reset or profile deletion. - Recipient list cleared on deferral:
refreshEligibleTargetsreturns early onOperationInProgressbefore the pruning branch, so existing targets stay; covered by the newPaykitPaymentRequestRepoTest.ktcase.
Merge confidence: 4/5, no findings, but the e2e shards had not finished and the native journeys listed in QA Notes are still unchecked.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 92148c4: no issues found. Reviewed 766f4a8..92148c4, this PR's own two commits on top of #1591. Greptile's open P1 at PrivatePaykitRepo.kt:1032 is the one remaining gap: three Ordered reads in automatic publication aren't checked for review, and it costs latency only.
Checked and clean:
- Every deferral resumes:
- Back, cancel and backgrounding release the hold through the
repeatOnLifecyclecleanup. - An app kill drops the in-memory holds, and startup sync re-queues the contacts.
- The preparation loop re-queues
keys ∩ knownSavedContactKeysonly for the same generation and outside profile deletion.
- Back, cancel and backgrounding release the hold through the
- Pending publication is kept:
retry.prepareEndpointsstays true, and the Lightning refresh isn't acknowledged. - Explicit publication isn't blocked: Ordered and Interactive pass admission. The deferral error is thrown inside
publicationMutex/operationLock, so both are released, and waitingactiveLinkPreparationscomplete with the error instead of hanging. - Locks: the order is
publicationMutex→operationLock, and admission only takessynchronized(paymentReviews). - No payment waits on deferred work: payment-path reads are Ordered or Interactive, and contact resolution happens before the review screens.
- Discovery: a deferred refresh keeps the existing targets and skips pruning, and pickers join with contacts.
- Supervision and cancellation: the batched-drain deferral escapes into a
SupervisorJobscope, andrunSuspendCatchingand the cancellabledelayare used throughout.
Perf-relevant: while Confirm, PIN or HardwareSign is open, each deferred job wakes about once a second for a cheap check with no SDK call. It resumes within about 1 s of review ending.
Device gate (partial): 92148c4 — not run yet. Pending:
link-contact-after-resume.xmlandcontact-payment-sharing.xml;- open Confirm on one device while the other adds or links it, keep it open for 30 s or more, and check nothing publishes;
- then end the review by Back, by paying and by backgrounding, and check the other device gets the updated list within a few seconds;
- control: an explicit link while review is open still publishes at once.
Device results will follow in a separate review.
|
Device gate (partial): Toggle timing: the slow disable here (about 3.5 min) came from a congested SDK queue. On a quiet machine the PR matches master; see partial 3. Still running:
|
|
Updated in 5bd39a8: routine request reloads now retain Background priority when a concurrent action or proof invalidates the first snapshot. The actual repository and SDK regressions fail on the previous head and pass with this change; all 4,423 unit tests and complete Detekt/Lint checks pass. Forced refresh and recovery retain their priority. Final-head device QA remains pending; this does not establish the cause of the observed native delay. |
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict:
The PR moved to 5bd39a8 after this run; these results are for 6cf67cd.
Findings: No new actionable findings in 766f4a8..6cf67cd. I verified the automatic identity-read finding is fixed at 6cf67cd: automatic capability and publication reads retain Background priority through SDK admission and key preparation, and typed deferral reaches the owner that retains pending contacts.
QA for 6cf67cd:
Test 1: request review, PIN cancellation and later request discovery ⏭️ Not driven because the head moved during setup.
The confirmation/PIN waits, later request arrival, background/reopen and one-payment assertions remain unverified in this run.
Test 2: original on-chain retry ✅ Retained prior pass: cancellation preserved the original payment, and fresh approval completed it once.
I relied on Test 3 of the earlier review and recording at 5ced847, carried through the base-head approval at 766f4a8. It covered fee/PIN waits and cancellation, invalid fees, original inputs, the recipient amount and the winning fee. This diff leaves the Ordered recovery implementation unchanged; I did not repeat that device case.
Test 3: link contact after resume ⏭️ Not driven because the head moved during setup.
Save, immediate Home/resume, Request Or Pay readiness and duplicate-contact checks remain unverified in this run.
Test 4: contact payment sharing stays disabled ✅ Teammate verified the toggle remains off after navigation at this head.
I relied on the current-head device report. It verifies disabling sharing, returning Home, reopening Settings and retaining the disabled preference, then re-enabling it. The same report records slow toggle completion without a matched base comparison; I make no latency-improvement claim.
Test 5: automatic publication defers and resumes ⏭️ Not driven because the head moved during setup.
The controlled before-admission hold, absence of publication during Send Confirmation and retained update after closing review remain unverified on a device. The corresponding automated admission and retention checks passed in Test 7.
Test 6: explicit publication and profile invalidation ⏭️ Not driven because the head moved during setup.
Explicit refresh/publication while automatic work is deferred, and prevention of old-profile work after reset, remain unverified on a device. Their repository regressions passed in Test 7.
Test 7: automated admission and retry regressions ✅ The pristine head built and the focused SDK, private-contact and recipient-discovery suites passed.
I ran assembleDevDebug and testDevDebugUnitTest for PaykitSdkServiceTest, PrivatePaykitRepoTest and PaykitPaymentRequestRepoTest. Gradle completion and JUnit XML confirmed execution with no failures, errors or skips. These cover queued capability/identity reads, pre-mutation deferral without dirtying backup state, explicit publication and cleanup, cancellation, profile invalidation, retained scheduling intent and recipient targets. No device media applies.
Coverage: full 766f4a8..6cf67cd review of all seven changed files and reached admission, publication, retry, lifecycle, cancellation, identity/reset and discovery paths. The four new device cases were not started; their changed behavior has source and automated coverage only. No device result from setup is counted as a pass.
Twin: synonymdev/bitkit-ios#1054 fix: defer automatic paykit work during payments at 0077196 matches the admission, retained-work and explicit-cleanup behavior in the source paths compared; no iOS device validation was performed here.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
Device gate (partial 2): deferral check at
Still running:
|
|
Device gate (partial 3): the payments-with-contacts toggle timed on a quiet machine, from tap until the toggle is enabled again, same emulator and wallet.
The PR matches master. The 3.5 min in partial 1 came from the congested SDK queue, not from this PR. The PR's "on" flips about 3 s later than master in both runs; that's small, and two samples can't separate it from noise. Still running: the idle-control deferral check, |
|
Device gate (partial 4): see synonymdev/bitkit-ios#1054 for the iOS-as-A deferral run with this build as B. |
|
Device gate (partial 5):
PIN → broadcast: this run took 91 s, with the payment reads queued 16-19 s right after several minutes of contact relinking. On a quiet machine the PR takes 23-31 s with nothing queued ahead, against 40-96 s on #1591; see partial 6. Still running: |
|
Thanks for capturing the queue times. Those waits are significant; the back-to-back comparison will help distinguish the branch change from differences between runs. We also measured long waits behind automatic Paykit work. #1639 keeps passive proof-display refreshes at Background priority. I’m validating two further changes on both platforms: keeping automatic contact-triggered refreshes at Background priority, and receiving then reading the resulting state under one SDK admission so unrelated maintenance cannot intervene. Explicit payment checks, forced freshness and recovery remain intact. These are not yet a demonstrated fix for your 91-second sample. Our repeated runs still vary, and we’re adding private timing probes to separate SDK queueing, remote storage/delivery and native broadcast time. Please keep the exact build heads and operation queue/work timings with the comparison; I’ll reconcile them with our traces before claiming an improvement. |
|
Device gate (partial 6):
PIN →
On this PR nothing else runs between PIN and broadcast, which is what it sets out to do. The 91 s in partial 5 came from queues on this build right after several minutes of contact relinking, and the quiet runs don't reproduce it. One run first paid the wrong pending request through a script fallback: 3,055 regtest sats to a fixture identity. It's excluded from the table. The device gate is paused here, at the user's request. Still pending:
|
|
Thanks—these quiet runs support the admission change removing competing work from the payment path. They also show that the remaining 23–31 seconds needs separate investigation; removing queue waits alone does not make the flow fast. Our latest Android runs likewise include 23.9 and 25.8 seconds with little queueing. I’m keeping the missed “Home immediately after Save” race and the unfinished comparison cases marked unverified. The next private build adds storage, delivery and native-broadcast phase timings to identify the remaining work, without changing payment checks or retries. |
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: No actionable findings in the delta from 6cf67cd to 5bd39a8.
QA for 5bd39a8:
Two Pixel 7 Android 15 emulators, Bitcoin regtest and staging Pubky. Recordings show the payer on the left and requester on the right, at 2× speed.
Test 1: request review, PIN cancellation and later request discovery ✅ The original request survived cancellation and resume, then paid once.
On the pristine build, the 1,000-sat confirmation and PIN each stayed open for over 30 seconds. Cancelling left Sent activity empty. A later 2,000-sat request arrived without restart. After reopening the original and backgrounding/unlocking, fresh authorization produced one 1,141-sat outgoing including the 141-sat fee. The requester showed “Proof submitted” for 1,000 sats; the later request remained pending.
j1.2x.mp4
Confirmation/PIN waits, cancellation, subsequent discovery, resume and both payment records.
Test 2: original on-chain retry ✅ Retained prior device evidence proves original inputs and fresh approval complete one payment.
I retained Test 3 and its recording from the earlier review, carried through the base-head approval. That case exercised fee/PIN waits and cancellation, invalid fees, original inputs and the approved fee. This delta leaves the Ordered recovery paths unchanged, so I did not repeat it.
Test 3: link contact after resume ✅ Immediate Home after Save retained the contact, and Request became available without saving again.
The fresh contact survived immediate backgrounding and reopening. Request Or Pay became available, Request opened the amount screen, and Contacts contained one entry. It was first observed about 115 seconds after the saved-contact log, with sampled UI checks; this is not an exact linking duration or a latency-improvement claim. Android returns directly to Contact after Save.
j3.2x.mp4
Save, immediate Home, resume, Request availability and the single saved contact.
Test 4: contact payment sharing stays disabled ✅ Retained teammate evidence verifies the disabled preference across navigation.
I relied on the device report at 6cf67cd, which disabled sharing, returned Home, reopened Settings and verified it stayed off before re-enabling it. This delta leaves the preference and withdrawal paths unchanged. The separate matched timing report does not establish a speedup.
Test 5: automatic publication defers and resumes ✅ SDK traces confirmed no automatic mutation during review, then successful publication after Back.
On a debug control build, sharing withdrawal/re-enable created a pending private-list update. A gate held its Background capability read before SDK admission. I opened the 2,000-sat confirmation and released the gate. The trace showed repeated admission deferral and retained contacts for over 30 seconds, with no automatic publication mutation. Closing confirmation let the retained update queue successfully. The hook only supplied the gate and logs; it did not replace SDK results or admission decisions.
j5.2x.mp4
The pending-update setup, held confirmation and return Home; mutation and queue results were checked in the SDK hook trace.
Test 6: explicit publication and profile invalidation ✅ Explicit publication completed during review, and invalidation discarded the deferred generation.
With another automatic update repeatedly deferred, explicit preparation with immediate publication completed while confirmation stayed open. The hook trace confirmed one queued update. Through the debug control, I invoked the actual profile-deletion invalidation and repository-clearing methods: the pending generation was cleared, and no old-generation preparation or publication resumed after closing review and observing for over 30 seconds. This exercised the repository lifecycle boundary; the full profile-deletion UI was not driven.
j6.2x.mp4
Confirmation remained open during the explicit operation and invalidation; completion, queued publication and generation clearing were checked in the hook trace.
Test 7: automated admission and retry regressions ✅ The pristine head built and the focused SDK, private-contact and request suites passed.
I ran assembleDevDebug and the PaykitSdkServiceTest, PrivatePaykitRepoTest and PaykitPaymentRequestRepoTest suites. Gradle completion and JUnit results confirmed execution without failures or skips. These include the new Background checkpoint reload, warm-projection retention and proof filtering on retry, plus admission after identity validation and continued Ordered/Interactive availability. No device media applies.
Review coverage: 6cf67cd…5bd39a8, all four changed files and reached callers. The precise concurrent-proof checkpoint race and request-list identity-validation race were exercised by automated regressions; the native request journey covered their visible flow. The earlier identity-read finding remains fixed and its thread is already resolved.
Twin: synonymdev/bitkit-ios#1054 fix: defer automatic paykit work during payments at 7ed5b69 matches the automatic-publication deferral and explicit-cleanup behavior in the source paths compared. Its request refresh uses a different combined-observation implementation. No iOS device validation was performed here.
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 5bd39a8.
No new actionable code findings.
Background contact publication, recipient discovery, and routine payment-request reloads defer when payment review or submission is already active before SDK admission. Pending contacts and the current discovery targets remain queued and retry afterward. Explicit publication, cleanup, withdrawal, and forced refresh or recovery keep their existing priority. A routine reload whose first snapshot is invalidated by a local action or proof stays at background priority, so the warm request projection is left unchanged until a later successful refresh applies the current proof filter. The earlier Ordered identity-read gap in this thread is fixed on this head: automatic capability and publication identity reads keep Background priority through the SDK lock.
Recipient-discovery deferral matches bitkit-ios#1054 at 7ed5b69: both keep existing targets when admission fails. iOS applies proof filters after one snapshot rather than Android's checkpoint re-read. That was a scoped comparison, not a full iOS review.
Validation: the CI build ran unit tests on this revision and passed, and lint and detekt passed. The new unit tests were inspected and not re-run locally. Device testing: not performed in this review. Earlier device notes apply to 6cf67cd and do not cover this head's request-reload change.
Ready for device testing. Re-run review-refresh.xml on this head.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Delta at 5bd39a8: no issues found. Reviewed 6cf67cd..5bd39a8, one commit with no merges; base #1591 is unchanged at 766f4a8.
- What changed: when a local action or proof lands during a routine refresh, the request re-read now runs at the refresh's own priority instead of Ordered (
PaykitPaymentRequestRepo.kt:592,:608-613). During a review the re-read defers and the current list stays. Forced refreshes and every othersynchronizeLockedcaller keep Ordered. - No stale entry: a paid request can't stay pending while the re-read waits, because accepting it removes it before the payment runs (
:1593). - Lock and backup state: deferral releases the SDK lock and the key, and leaves backup state alone.
Device gate (partial): 5bd39a8 — the earlier results from 6cf67cd hold for contact-payment-sharing.xml (9/9) and link-contact-after-resume.xml (8/8): this delta doesn't touch publication or linking. They no longer hold for review-refresh.xml (17/17), because the delta changes the re-read on that route.
Pending:
- re-run
review-refresh.xml; - re-capture PIN→broadcast in the same run;
link-contact-after-resume.xmlwith iOS first.
This branch has not been deployed
5 participants
Refs #1407, #1419
Stacked on #1591 (
fix/paykit-proof-latency).This PR defers automatic Paykit contact publication, discovery and request refresh when payment review or submission becomes active before SDK admission.
Description
Out of Scope
Design
N/A — no UI changes.
Preview
N/A — no layout changes.
QA Notes
Journeys
Existing regression journeys are unchanged; the final-head native run is pending.
review-refresh.xml— request review, PIN cancellation and subsequent request discovery remain usable.onchain-original-payment-retry.xml— explicit fee approval and recovery authorization retain the original payment; requires its controlled unresolved-payment fixture.link-contact-after-resume.xml— saved contacts finish linking after foreground resume without saving again.contact-payment-sharing.xml— disabling sharing remains effective across navigation.Manual Tests
Automated Checks
PaykitSdkServiceTest.kt— automatic publication defers at queued admission and backup preflight without changing backup state; explicit publication, cleanup and cancellation remain supported.PrivatePaykitRepoTest.kt— deferred publication retains contacts, releases explicit publication and stops after reset or profile deletion.PaykitPaymentRequestRepoTest.kt— deferred recipient discovery retains existing targets; a concurrent proof change defers routine reloads, preserves the warm projection and filters the matching proof on retry while forced refresh stays available.PaykitSdkServiceTest.kt— background capability, identity, peer, scheduling and request reads honor payment and review holds while ordered and interactive reads remain available; a payment starting during identity validation defers the subsequent request list without dirtying backup state.PrivatePaykitRepoTest.kt— typed scheduling deferral retains retry intent without draining messages; a later fresh empty snapshot retires the retry. Queued capability and publication identity reads retain automatic updates when payment begins, while explicit publication remains available.Local validation passed: compilation, 201 affected tests, all 4,423 unit tests, Detekt (zero SARIF findings) and Android Lint (zero XML/SARIF findings). Existing cursor-cleanup and tooling warnings were checked against the previous head; no introduced diagnostics were found.
The two new admission regressions were also run against the unchanged previous head and failed for the intended missing-deferral behavior. Final-head device QA remains pending; this PR makes no measured latency-gain claim.