Skip to content

refactor: batch paykit contact cleanup#1099

Open
ben-kaufman wants to merge 4 commits into
codex/paykit-incoming-payment-requests-rc39from
codex/batch-paykit-contact-cleanup
Open

refactor: batch paykit contact cleanup#1099
ben-kaufman wants to merge 4 commits into
codex/paykit-incoming-payment-requests-rc39from
codex/batch-paykit-contact-cleanup

Conversation

@ben-kaufman

Copy link
Copy Markdown
Contributor

Description

This PR builds on #1098 to address the Paykit cleanup performance follow-ups from iOS #620:

  • Reads the SDK linked-peer list once per multi-contact publication or cleanup pass instead of once per contact.
  • Clears receiver-scoped payment lists as before, then drains outbound private messages and checks pending status once for the whole successful batch.
  • Preserves per-contact failure isolation: contacts with failed or still-pending cleanup retain their cached state and retry marker, while successful contacts are cleared.
  • Adds regression coverage for one linked-peer snapshot, one batched drain, and partial cleanup failure.

No user-facing behavior changes are intended.

Preview

N/A — repository-only refactor.

QA Notes

Manual Tests

N/A

Automated Checks

  • PrivatePaykitRepoTest.kt: all 37 focused tests passed, including the three new batching and failure-isolation tests.
  • ./gradlew compileDevDebugKotlin passed.
  • ./gradlew testDevDebugUnitTest passed.
  • ./gradlew detekt passed with only pre-existing findings outside this change.
  • git diff --check passed.

@greptile-apps

greptile-apps Bot commented Jul 22, 2026

Copy link
Copy Markdown

Greptile Summary

This PR batches private Paykit contact publication and cleanup work. The main changes are:

  • Reads linked peers once per multi-contact pass.
  • Clears payment lists before one shared message drain.
  • Preserves cached state for contacts with reported cleanup failures.
  • Adds tests for batching and partial failure handling.

Confidence Score: 4/5

The batched cleanup path can skip remote cleanup or block unrelated contacts, so these cases should be fixed before merging.

  • Invalid keys can be treated as successfully cleaned and lose their retry marker.
  • One linked-peer lookup failure now prevents cleanup for the whole batch.
  • A peer change during the pass can leave remote state without matching cached cleanup state.

app/src/main/java/to/bitkit/repositories/PrivatePaykitRepo.kt

Important Files Changed

Filename Overview
app/src/main/java/to/bitkit/repositories/PrivatePaykitRepo.kt Adds shared linked-peer snapshots, batched cleanup and draining, and per-contact cache reconciliation.
app/src/test/java/to/bitkit/repositories/PrivatePaykitRepoTest.kt Adds valid tests for one linked-peer read, one shared drain, and partial cleanup failure.

Reviews (1): Last reviewed commit: "refactor: batch paykit contact cleanup" | Re-trigger Greptile

Comment on lines +1211 to +1212
val publicKeys = publicKeys.mapNotNull(::normalizedPublicKey).distinct()
if (publicKeys.isEmpty()) return@runSuspendCatching Unit

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Invalid Key Reports Cleanup Success

When a pending deleted-contact key cannot be normalized, mapNotNull drops it and the empty batch returns success. retryPendingDeletedContactEndpointRemoval then removes the cached contact and retry marker even though its remote payment lists were never cleared, so that cleanup is not attempted again.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in e56f9dc. Invalid keys now return InvalidPublicKey instead of being dropped from an empty batch, so the retry marker and cached state are preserved for another cleanup attempt. Added a regression test for this case.

val publicKeys = publicKeys.mapNotNull(::normalizedPublicKey).distinct()
if (publicKeys.isEmpty()) return@runSuspendCatching Unit

val linkedReceiverPaths = linkedReceiverPathsByPublicKey()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Shared Snapshot Breaks Failure Isolation

When this linkedPeers() call throws, the batch exits before clearing any contact. The previous no-argument flow caught each contact's failure and continued with later contacts, so one transient SDK failure now blocks cleanup progress for every contact in the pass.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in e56f9dc. The shared snapshot remains the fast path, but if it fails we retry peer inspection per contact. One contact's failure no longer blocks later contacts, and its locally cached published paths are still cleared while its state remains pending. Added regression coverage for the partial-failure case.

Comment on lines +1214 to +1215
val linkedReceiverPaths = linkedReceiverPathsByPublicKey()
val preparation = clearPrivatePaymentLists(publicKeys, linkedReceiverPaths)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Snapshot Misses Newly Linked Paths

If a receiver path becomes linked after this batch-wide snapshot but before its contact is processed, that path is omitted from receiverPathsForCleanup. The contact can still be marked successful and lose its cache and retry marker while the remote payment list on the missed path remains published.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think this can leave an untracked payment list. Cleanup unions the snapshot with publishedPrivatePaymentReceiverPaths; queued publications are recorded in that local cache before delivery, and publication/cleanup run on the serialized dispatcher. A path becoming linked by itself does not publish a payment list, so any newly linked path with a list to clear is already covered by the cached published path.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're right. The two invariants together close the gap I was concerned about:

  1. receiverPathsForCleanup unions the linked-peer snapshot with publishedPrivatePaymentReceiverPaths — the local cache independently covers every path that was ever published.
  2. The local cache is written before delivery, and publication and cleanup are both serialized on serializedDispatcher, preventing any interleaving that could leave an untracked path.

A newly linked path without a prior publication has nothing in the remote list to clear, so missing it from the snapshot is harmless. My original comment was incorrect — sorry for the noise.

@jvsena42 jvsena42 added this to the 2.5.0 milestone Jul 22, 2026
@ben-kaufman
ben-kaufman force-pushed the codex/paykit-incoming-payment-requests-rc39 branch from 6959e91 to 9bae4d5 Compare July 22, 2026 13:14
@ben-kaufman
ben-kaufman force-pushed the codex/batch-paykit-contact-cleanup branch from e56f9dc to 9cf060e Compare July 22, 2026 13:14
@ben-kaufman
ben-kaufman force-pushed the codex/paykit-incoming-payment-requests-rc39 branch 2 times, most recently from b27d8b5 to eda7df2 Compare July 22, 2026 15:34
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.

2 participants