refactor: batch paykit contact cleanup

[ben-kaufman]

Description

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

• Reads the SDK linked-peer list once per multi-contact publication or cleanup pass, with one shared retry instead of a per-contact fallback.
• 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.
• Serializes cleanup with endpoint publication and skips the local state wipe if relevant contact state changes during an awaited SDK operation.

On the error path, a failed link snapshot still clears locally recorded published receiver paths while retaining the contact state and retry marker. A batch-level drain-inspection failure conservatively keeps the whole affected batch pending for retry. No user-facing behavior changes are intended.

Linked Issues/Tasks

• Base PR: feat: support incoming paykit requests #637
• Review follow-up: feat: support paykit receiver paths #620
• Companion Android PR: refactor: batch paykit contact cleanup bitkit-android#1099

Screenshot / Video

N/A — service-only refactor.

QA Notes

Manual Tests

N/A

Automated Checks

• PrivatePaykitServiceTests.swift: all 13 focused tests passed in the iOS simulator, including mixed cleanup success/failure coverage.
• SwiftFormat passed for the changed Swift files.
• git diff --check passed.

[greptile-apps]

Greptile Summary

This PR batches Paykit contact cleanup work to reduce repeated SDK calls. The main changes are:

• Reads and groups linked peers once per publication or cleanup batch.
• Drains outbound private messages once after clearing the batch.
• Tracks cleanup success and cached-state removal per contact.

Confidence Score: 4/5

The batch-wide linked-peer failure path can leave stale endpoints without a cleanup retry.

• Successful batch processing keeps failures isolated by contact.
• A failed shared lookup suppresses cleanup updates and retry keys for every contact.
• Non-immediate publication callers can discard that failure.

Bitkit/Services/PrivatePaykitService+Contacts.swift

Important Files Changed

Filename	Overview
Bitkit/Services/PrivatePaykitService+Contacts.swift	Batches linked-peer reads and message draining while preserving per-contact cleanup state, but the shared lookup failure path can omit cleanup and retries.

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

[jvsena42]

Reviewed the batching refactor. The per-contact failure isolation contract does hold — I traced the partial-failure cases (one path of two fails, drain still pending, link inspection fails) and each ends with the contact in failedPublicKeys, state untouched, retry marker set. SwiftFormat lint passes and omitting a changelog fragment is correct for a refactor: with no user-facing change.

Comments inline. The one I'd want addressed (or explicitly accepted) before merge is the widened actor-reentrancy window on the deferred state wipe; the rest are smaller.

Test coverage: no test changes here. PrivatePaykitServiceTests.swift has 12 tests, none of which touch removePublishedEndpoints or linkedPeers, so the new batching, the failedPublicKeys/successfulPublicKeys split, and the snapshot-failure fallback are all untested. The "all 10 focused tests passed" note in the description is a regression run, not coverage of this change. Given the state-clearing logic is the risky part, a test asserting "contact A succeeds, contact B fails → A cleared + marker cleared, B state retained + marker set" would be worth adding.

Merge order: base is codex/paykit-incoming-payment-requests, so this needs to land behind #637.

[jvsena42]

Actor-reentrancy window for the state wipe is now much wider.

PrivatePaykitService is an actor (reentrant), and removePublishedEndpoints is not run under withPublicationLock — only syncLocalEndpointPublication is. Before this PR each contact's state.contacts[X] wipe happened immediately after that contact's own clear+drain. Now every wipe is deferred to here, after the whole batch's clears and the full drain.

Scenario: removePublishedEndpoints() (the no-arg variant, which unions all known + cached + pending keys) runs while a foreground refreshKnownSavedContactEndpoints is in flight. Contact A is cleared early in the batch; during the many subsequent awaits the concurrent publish calls recordPublishedPrivatePaymentList for A (or an incoming invoice writes localInvoicesByReceiverPath); this loop then wipes cachedResolvedEndpoints / localInvoicesByReceiverPath / publishedPrivatePaymentReceiverPaths for A, desyncing local state from what the SDK actually holds.

The hazard pre-exists, but the batching widens it from one-contact latency to whole-batch latency. Cheapest mitigations: snapshot the relevant state.contacts[publicKey] fields before the clear and skip the wipe if they changed, or run the cleanup pass under withPublicationLock as well.

[ben-kaufman]

Addressed in 073bb5d. Cleanup now uses the same publication lock as endpoint publication, snapshots the cleanup-owned contact fields before SDK awaits, and defers the local wipe while retaining the retry marker if those fields changed during the operation.

[jvsena42]

This fallback re-issues the exact call that just failed.

linkedReceiverPaths(publicKey:) now calls linkedReceiverPathsByPublicKey(), which is the same PaykitSdkService.shared.linkedPeers() that just threw when the snapshot was taken. So when the snapshot fails, this pass makes N+1 linkedPeers() calls instead of the pre-PR N — a regression in exactly the dimension the PR optimizes — and the "retrying per contact" wording in the warning above oversells what the retry can accomplish.

Suggest picking one: on snapshot failure either fail the pass once (set firstError, mark all keys pending, return) or continue with an empty snapshot, and drop the per-contact re-fetch entirely. Same applies to the mirrored block in syncLocalEndpointPublicationLocked.

[ben-kaufman]

Addressed in 073bb5d. The per-contact fallback is gone. Both cleanup and publication now use one shared linked-peer snapshot helper that makes one initial call and at most one batch-wide retry, so the failure path is bounded at two calls rather than N+1.

[jvsena42]

Duplicated snapshot-or-fallback block.

This is the same ~14 lines as the block in removePublishedEndpoints (lines 109–124), differing only in the log's reason. Per the repo's reuse convention, worth extracting into a single helper — e.g. linkedReceiverPaths(for publicKey: String, snapshot: [String: Set<String>]?, reason: String) async -> (paths: Set<String>, error: Error?) — so the two call sites can't drift.

(Same redundant-refetch caveat as the other block applies here too.)

[ben-kaufman]

Addressed in 073bb5d. The duplicated snapshot/fallback blocks are replaced by the shared linkedReceiverPathsSnapshot helper, used by both cleanup and publication.

[jvsena42]

Behavior change worth calling out (the description says "no user-facing behavior changes are intended").

Previously, when receiverPathsForCleanup threw for a contact, the loop continued and made zero clear attempts for it. Now a link-inspection failure yields an empty contactLinkedReceiverPaths and execution falls through to here, so the contact still gets clearPrivatePaymentList called for its locally-published paths.

This is arguably an improvement (partial progress, and the contact is marked failed either way so it retries), but it's a real change in what hits the SDK on the error path. Probably worth a line in the PR description.

[ben-kaufman]

Agreed. I kept the partial-progress behavior intentionally: after both shared link-snapshot attempts fail, locally recorded published paths are still cleared, while the contact state and retry marker are retained. The PR description now calls this error-path behavior out explicitly.

[jvsena42]

Blast radius of a transient drain-check failure grew from one contact to the whole batch.

pendingPrivateMessageDrainKeys returns all the passed keys when linkedPeers() or pendingOutboundPrivateCounterparties() throws (see its two catch blocks). With the check now run once per batch, a single transient SDK hiccup marks every successfully-cleared contact as failed, so none of them get their state cleared or their retry marker cleared.

The retry path covers it, so this isn't a correctness bug — but it's a real reduction in transient-failure granularity that comes with the batching, and it's the kind of thing that shows up as "cleanup never converges" if the SDK is flaky during a full cleanup pass. Worth noting in the description alongside the perf win.

[ben-kaufman]

Agreed on the tradeoff. This remains deliberately conservative because a failed batch-level inspection cannot prove which clears have fully drained. The affected contacts keep their state and retry markers, so the next cleanup pass retries safely. The PR description now calls out this batch-level failure granularity.

[jvsena42]

Failure markers are now written once at the end instead of eagerly.

The old code called markDeletedContactCleanupPending([publicKey]) at the moment each contact failed, so a crash mid-loop still left the marker persisted. Now nothing is written until the batch completes.

Mostly recoverable, since removePublishedEndpoints() and pruneUnsavedContactState re-derive keys from state.contacts (which isn't cleared for failed contacts). The narrow gap is a contact deleted via removeSavedContact that has no local cache state — its only record would be this marker, and a crash before this line loses it. Low severity, but it is a durability regression relative to the eager write.

[ben-kaufman]

Addressed in 073bb5d. Deleted-contact cleanup markers are now persisted synchronously in removeSavedContact and removeSavedContacts before the first remote await, closing the no-cache crash gap. Successful cleanup clears only the corresponding markers.

[jvsena42]

Minor: publicKeys is already deduplicated by normalizedSavedContactKeys at the top of the function, so this Set(publicKeys) is rebuilding a set that could be constructed once above the loop and reused (it's also implicitly the membership source for the loop). Purely cosmetic.

[ben-kaufman]

Addressed in 073bb5d. The normalized keys are converted to publicKeySet once and that set is reused for failed/successful membership calculations.