Skip to content

fix: reject stale SDK waiters during wallet reset - #1125

Open
ben-kaufman wants to merge 9 commits into
fix/profile-cleanup-outcomesfrom
fix/paykit-sdk-wipe-queue
Open

ben-kaufman wants to merge 9 commits into
fix/profile-cleanup-outcomesfrom
fix/paykit-sdk-wipe-queue

Conversation

@ben-kaufman

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

Copy link
Copy Markdown
Contributor

Related issue: synonymdev/bitkit-android#1687 (the broader profile recovery work remains open).
Dependency: #1124
Companion: synonymdev/bitkit-android#1693 — includes the corresponding Android queue admission repair.

This PR prevents wallet teardown from waiting indefinitely when an older Paykit operation is admitted before teardown but reaches the SDK queue after teardown has begun waiting for its outer operation.

Description

Recheck the wipe flag and generation in the same critical section that inserts an ordinary or cancellable waiter. A stale operation is rejected before it can queue behind teardown. Existing post-acquisition validation, draining of admitted work and nested wipe ownership remain intact.

Out of Scope

Profile-removal owner/attempt, persistent recovery and strict local cleanup are handled separately by #1143 and synonymdev/bitkit-android#1693. Their remaining acceptance and remote-Disconnect findings stay under synonymdev/bitkit-android#1687; this queue change does not claim to resolve them or establish the historical incident cause.

Design

N/A — no UI changes.

Preview

N/A — queue admission has no new visual surface.

Current parent composition — October 30

Signed additive merge 6dca681120a572a263473140f7657f69050221f5 incorporates actual #1124 parent de683f315d91aab493b17e5d41e1958e5ef5fd13, tree c047a2f5a0a41445faa399731f356d3a40734d58. The original three-file child patch and complete SDK queue helper are unchanged. A fresh independent affected-composition review is clean; unchanged earlier concurrency coverage is reused by exact input identity. All nine live-base commits verify locally and on GitHub (verified=true, reason=valid).

The final combined tree passed 2,960 unit tests and five profile/contact UI executions, with one opt-in encrypted native-fixture skip, zero failures and no retries. All 30 queue tests passed, including the admission-to-insertion regression. All 1,687 tracked entries match. Complete build/test logs, all 2,966 nested test summaries, 3,185 action sections, eight exported diagnostic logs and 30 attachments were inspected; the owner verified 18,089 sealed evidence members. The original XCResult remained byte-identical because the audit used a copy. Actual execution used Xcode 26.1 /17B55, SDK26.1 and iPhone16/iOS26.2 (23C54). Other UI classes and four workflow integration classes were not selected and are not passes.

SwiftFormat0.64 reports 0/827, with107 configured exclusions; translations report zero errors and15 warnings verified against the actual parent. Five QoS warnings, three AppIntents notices and application/simulator diagnostics remain disclosed. Four QoS test/message pairs recur in the sealed parent result; source matches do not establish timing cause, harmlessness or parent runtime equivalence. No source compiler or formatting violation was found.

Current-head native CI and SwiftFormat passed on the exact published tree: 2,960 unit and 39 physical UI passes, five explicit skips, zero failed tests or test reruns, formatting 0/827 with 107 configured exclusions. All 30 queue tests passed. Complete logs, all 3,004 nested executions, diagnostic payloads, 55 images and checksum/CRC-verified archive were inspected; the owner verified 9,401 sealed evidence files. The 286 distinct linker messages match the actual parent, with two AppIntents notices, one recurring keyboard timeout and 48 NaN observations retained. One keypad action needed an automatic second tap; this was not a test rerun. A new shared-keychain -25300 event occurred in a passing test whose producer and test are unchanged from the parent; its cause and introduction remain unproved. Skips, excluded integration classes and migration jobs receive no pass credit. The earlier hosted results below remain revision-scoped. The PR remains ready (draft=false); this queue composition does not establish authenticated profile-removal/reset acceptance or close the separate #1143 findings.

Prior parent composition — October 29, final combined tree

Signed additive merge 53b67b077c8fe7424136ce531cc117119714f806 incorporates actual #1124 parent b14d5a1b93ab274928fda044771d6e12c27d2475, with tree 080091e6db1807f0aa1c08ceac7e12cc3e6c12d9. The complete SDK queue class and original three-file child patch remain unchanged. A fresh independent affected-composition review is clean; unchanged prior concurrency reviews are reused by exact source/patch identity. All 8 live-base commits verify locally and on GitHub (verified=true, reason=valid).

The final combined tree passed 2,884 unit tests and five profile/contact UI executions, with one encrypted native-fixture skip, zero failures and no retries. All 1,672 frozen inputs match. Complete raw/action logs, all 2,890 nested summaries, eight diagnostic logs, 28 attachments and executed products were inspected; the owner verified all 13,745 sealed evidence rows. Actual execution used Xcode 26.1 /17B55, SDK 26.1, iPhone 16/iOS 26.2(23C54). Other UI classes and four workflow integration classes were not selected; they are not passes.

SwiftFormat 0.64 passed 0/824 with101 configured exclusions. Its optional global cache write was denied by the sandbox; checking completed with zero source findings. Translation validation reuses the identical actual-parent locale/validator inputs: zero errors and 15 verified warnings, without claiming a repeated execution. Six Paykit initialization QoS observations/six nested warnings, two AppIntents notices and eight simulator haptic messages remain disclosed; no timing-cause or warning-free claim is made. The exact-class and declaration-through-end-of-file hash boundaries are recorded separately.

At prior head 53b67b07, hosted native tests 40958568558 and SwiftFormat 40958568508 passed on synthetic merge cfe5069d, whose tree exactly equals this head and whose parent is b14d5a1b. Native results are 2,884 unit and 39 UI physical passes, five explicit skips, zero failures or retries; the atomic-insertion regression passed. Hosted formatting is 0/824, with 101 configured exclusions. All 2,928 nested results, complete logs, command-output diagnostics, references and checksum/CRC-verified artifact were inspected, and the owner verified all 9,215 sealed evidence rows. The 571 duplicate-symbol events/286 distinct messages and two AppIntents notices match the actual parent's producers. There are 72 CoreGraphics NaN observations versus 48 in the actual parent, including 24 additional emissions in a passing receive test; their source cause and harmlessness are unproved. No warning-free or causal-fix claim is made. All nine migration jobs and the selected fixture/hardware cases were skipped; four integration classes were excluded. These are not passes. Parent #1123's original hosted background-assertion failures remain recorded separately, with its single authorized retry tracked independently. The PR remains ready (draft=false); named reviewers remain represented without duplicate requests. No authenticated profile-removal/reset journey or full #1143 acceptance is inferred.

Earlier parent composition — October 29

Signed additive merge 35e267d8470bc8201ff1fe2041e29b832916c83e incorporates actual #1124 parent 2df5bad1821193ceb1125536a1de428e583010d9, tree 56f218437a445844a55b1717301dc286148ee7fd. The original three-file queue patch remains unchanged. A fresh affected-composition review is clean; prior independent concurrency reviews are reused only after exact source/patch comparison. All seven live-base commits verify locally and on GitHub (verified=true, reason=valid). One push was made; an initially stale GitHub head response was reconciled read-only.

Final selected native validation passed 2,869 unit tests and 39 UI variant executions, zero failures/retries, with five explicit fixture/hardware skips. This equals 2,879 logical passes and 2,913 physical executions including skips. All 1,665 frozen inputs match. SwiftFormat 0.64.0 reports 0/823 files requiring formatting (100 exclusions); translations have zero errors and 15 warnings verified against the actual parent. The toolchain receipt identifies Xcode 26.1 / 17B55, iOS 26.2 / 23C54, iPhone 16 arm64; the historical folder name is not used to infer the Xcode version.

Complete logs, all nested records, exported diagnostics and static reports were inspected; the owner verified 9,349 sealed evidence files. Twenty-three nested QoS records, 48 CoreGraphics NaN messages and two AppIntents notices remain disclosed. Relevant warning-bearing source/configuration matches the actual parent, but occurrence frequency and timing causes are unproven. Wrapper omissions were reconciled against canonical results and raw/action logs. Four excluded integration classes and five selected skips are not passes. No authenticated reset/profile journey is inferred. Current-head native/UI run 40901460844 and SwiftFormat 40901460644 passed on synthetic merge ac77276f, whose tree exactly equals the published head. Complete logs and checksum/CRC-verified XCResult reconcile 2,869 unit and 39 UI successes, five skips and no retries; formatting is 0/823. All 9,054 sealed evidence files were verified. The 286 unique linker warnings match the actual parent; two AppIntents notices and 48 NaN text observations remain, with no QoS or sanitizer reports and no causal-fix claim. All nine migration jobs were skipped. Ovi and João are already pending; existing reviewer representation is preserved without duplicate requests. Prior hosted results below remain tied to their exact earlier heads. The PR remains ready (draft=false).

Prior composition and validation

Signed additive merge ef1286d817a7db933c1b9b2cf6e1773951ca1f4a incorporates live #1124 a2b218613ecdeb0a45cd9364d723204147b27044. The original three-file queue patch is unchanged; two independent reviews checked its interaction with the parent's new active-subscription guards. All six commits in the exact live-base range verify locally and report verified=true/reason=valid on GitHub. The PR remains ready (draft=false).

The final combined tree 1b944543 passed 2,833 selected unit tests and 39 UI variant executions, with zero failures and three explicitly disabled Trezor skips, on iPhone 16 / iOS 26.2 (23C54), Xcode 16.4. The initial runner selected only the ProfileRecovery UI class; a separate full UI target run closed that selection gap without repeating units. The ProfileRecovery case is counted once in combined coverage. All 1,648 frozen file hashes match. Whole-repository SwiftFormat 0.64.0 passed 0/821 (94 configured exclusions). Translation validation retains zero errors / fifteen verified existing warnings by exact equality of locale, validator and source-usage inputs.

Complete build/test logs, every nested execution summary, attachment and diagnostic reference were inspected. Five unit/targeted-UI and twelve full-UI QoS observations remain in unchanged SDK constructor and LDK status/cache paths, with actual-parent source/configuration comparison. AppIntents and simulator diagnostics are retained; this is not a warning-free or timing-cause claim. Disabled Trezor tests, the unavailable native/integration fixtures and the expired local-fork payment case are not passes. No authenticated subscription-cleanup journey or actual wallet reset is claimed. Current-head hosted native tests and formatting now pass. The native run executed 2,833 unit passes and 39 UI variant passes, five explicit fixture/hardware skips, zero failures or retries. Four whole integration selectors were excluded separately. Synthetic checkout ea656da8 has the exact head tree and actual #1124 parent. Complete logs, every nested result/reference and all uploaded artifact bytes were inspected; 8,945 evidence hashes were verified. Hosted formatting is 0/821, with 94 exclusions. The 286 native duplicate-symbol warnings exactly match the verified actual-base run (280 SQLite, four rustsecp256k1, one Rust exception-personality and one BLAKE3). One raw UIKit keyboard queue timeout occurred during the passing, unchanged testPendingWalletSwitchResumesAfterTags; the same observation was verified at the prior head, but its cause remains unproved. Structured runtime issues report zero. Nine migration jobs were skipped. Prior results below remain revision-scoped; no skipped or excluded case receives pass credit.

QA Notes

Setup

Use the committed dependencies, Xcode and an available iOS simulator. Run xcodebuild test -project Bitkit.xcodeproj -scheme Bitkit -destination 'platform=iOS Simulator,id=<SIMULATOR_UDID>' -only-testing:BitkitTests/PaykitSdkOperationLockTests. The regression controls only task admission and queue order; it does not reset a wallet or use funds.

Journeys

N/A — not drivable; see Manual Tests.

Manual Tests

  • 1. With a controlled admission barrier, hold an ordinary outer owner between SDK admission and insertion, begin teardown and let it await that owner, then release insertion → the stale operation rejects and teardown drains. Repeat the cancellable entry path. Precise scheduler barriers are outside the journey runner's capabilities; use disposable controlled resources without a real wallet reset.

Automated Checks

  • added PaykitSdkOperationLockTests.swift regression — covers both admission paths, rejects stale execution and drains test tasks even after a failed assertion.
  • ran the complete required unit selection and full SwiftFormat lint.

Evidence

The deterministic regression holds both cancellable and ordinary operations between SDK admission and queue insertion, begins teardown waiting for the outer owner, and verifies that the old operation rejects without executing stale SDK work. The original negative control produced two bounded timeouts; the repaired test drains all tasks. The queue implementation and regression remain byte-identical through the current parent composition. This establishes Manual 1 at the lock-component level, not an actual wallet reset.

Prior head 3f887088 passed hosted native tests and formatting: 2,829 unit and 39 UI passes, five explicit fixture/hardware skips, zero failed executions/retries, format 0/821. Complete logs and nested diagnostics were inspected. Its duplicate-symbol warnings and keyboard queue timeout during a passing lifecycle test remain retained without a causal claim. Those are revision-scoped results, not a current-head hosted pass.

Gaps

Manual 1 is proved at the lock-component level with controlled resources. No authenticated network journey or actual wallet reset was performed. The controlled race is not evidence that it caused the historical transfer/contact incidents.

Models used

  • Planning/scoping: gpt-6-astra (reasoning: xhigh)
  • Implementation: gpt-6-astra (reasoning: xhigh)
  • Review: gpt-6-astra (reasoning: xhigh) and scoped diagnostics/source review gpt-6.1-sol (reasoning: high)

@ovitrif ovitrif added this to the 3.1.0 milestone Oct 26, 2026
@ovitrif

ovitrif commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Triage: 3.1.0. It fixes the shipped wallet reset, and 3.1.0 is due 28 Oct.

@talosmachina talosmachina left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No findings. Rechecks the wipe flag and generation inside the same critical section that inserts an ordinary or cancellable waiter, so an operation admitted before a wipe can no longer queue behind the wipe that is draining its outer owner. Reviewed 3a7210b, full tier, stacked on #1124 (diffed against fix/profile-cleanup-outcomes).

What I checked, and 5 candidates I ruled out

Read in full: PaykitSdkOperationLock (Bitkit/Services/PubkyService.swift), the new regression in PaykitSdkOperationLockTests.swift, the withLock(generation:) caller at activateRegisteredIdentity.
Not built here: iOS does not build on Linux, so this is reasoned from the code and CI.
CI: SwiftFormat green; Run Tests still pending at the time of this review.

Ruled out

  • Wipe acquire now discards a result: withWalletWipe calls acquire with no generation, so the reject branch is unreachable for it and the result is always true.
  • withLock(generation:) checks the wrong generation at insert: the insert check uses the admitted generation and the post-acquire validate(expectedGeneration ?? admittedGeneration) still enforces the caller's one, so a stale PubkyRegisteredIdentity is still rejected.
  • Waiters queued before the wipe began: they passed the insert check legitimately, are resumed by release() in order, and fail validate after acquisition as before; nothing leaks the lock because defer { release() } is set before validate.
  • Cancellation ordering in acquireCancellable: Task.isCancelled is still checked first, so a cancelled stale caller gets CancellationError, and an early resume never appends a waiter that onCancel would have to remove.
  • Release build: beforeAcquireForTesting, its init and both call sites are inside #if DEBUG, so the release type keeps its implicit init(), which is what PubkyService uses.

The regression would fail without the fix (the ordinary task queues behind the wipe, finished times out) and still drains, because cancelling the wipe removes its cancellable waiter on outer.

Merge confidence: 4/5, no findings and the change has a regression test, but the unit run had not finished when I reviewed.

Reviewer model: claude-opus-5-5
Reasoning effort: Not exposed (the runtime does not report it)

@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.

No findings.

Checked and clean:

  • A waiter queued before the reset can't run against the next wallet. The wipe's .ordered acquire is a barrier in release() (PubkyService.swift:2070), so every earlier waiter resumes first and fails validate on the bumped generation. The new isWiping / generation checks (:2008, :2030) stop anything from inserting behind the wipe.
  • No legitimate caller after the reset is rejected. The only rejection window between release and endWipe() already exists on the base.
  • Rejected callers get the same wallet_wipe_in_progress PaykitError.Storage that admit() / validate() already produce, and their handlers treat it as transient. No new error reaches the UI.
  • Android has the same acquire recheck bundled into synonymdev/bitkit-android#1693 (PaykitSdkOperationLock.kt:176-182).

Device gate: no device check needed — no UI change, no journey, not performance-relevant; the new unit test is the check. CI: Run Tests is still pending at 3a7210b.

@ovi-reviewer ovi-reviewer Bot 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.

Verdict: ✅ Approve

Findings: None.

QA for 3a7210b: I built the unmodified head with Xcode 27.0 and ran the committed lock tests on an iPhone 17 simulator running iOS 26.5.

Test 1: Manual 1 — stale admission during teardown ✅ XCTest assertions confirm stale rejection and task drainage for both admission paths.

testWipeRejectsPreviouslyAdmittedOuterOwnerBeforeQueueInsertion passed. Its barrier holds an outer owner after admission, starts teardown waiting on that owner, then releases insertion. Both ordinary and cancellable callers receive wallet_wipe_in_progress without executing stale work, and the tasks finish. The complete PaykitSdkOperationLockTests selection also passed, covering existing queue ordering, cancellation, wipe cleanup and fresh work.

testWipeRejectsPreviouslyAdmittedOuterOwnerBeforeQueueInsertion: passed
PaykitSdkOperationLockTests: passed

manual-1.2x.mp4

Simulator recording at 2× speed. It shows the XCTest host launching and exiting; the queue behavior is verified by the XCTest assertions and result, rather than visible UI.

Check 1: build and formatting ✅ The pinned head builds, and changed-file lint passes.

xcodebuild build-for-testing succeeded, followed by test-without-building -only-testing:BitkitTests/PaykitSdkOperationLockTests. SwiftFormat 0.64.0 passed both changed Swift files, and git diff --check passed. The hosted unit test job and formatting job are also green.

Full review of a062618..3a7210b, all three changed files and the reached queue, teardown and caller paths. No UI journey, authenticated network flow or actual wallet reset was driven: Manual 1 is a controlled lock-component regression. Complete profile-removal ownership and recovery remain outside this change, as tracked in bitkit-android#1687 Bind profile deletion cleanup to the original sign-in.

Twin/companion: bitkit-android#1693 fix: bind profile removal to its original owner contains the matching atomic admission/generation recheck; its broader recovery integration exceeds this iOS change.

Reviewer model: gpt-6.1-sol
Reasoning effort: high


Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

@ovitrif ovitrif 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.

utAck

@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 its merge base, at 3a7210b0.

No new actionable code findings.

PaykitSdkOperationLock rechecks the wipe flag and generation under the same lock that inserts an ordinary or cancellable waiter. An operation admitted before teardown is rejected with wallet_wipe_in_progress before it can queue behind a wipe that already holds the SDK lock and is waiting on that operation's outer owner. The rejection returns before the SDK lock is taken. Teardown still acquires with no generation gate, and waiters already queued are still handed the lock and then rejected by the existing post-acquire validate before their operation runs. The reject-before-enqueue contract matches bitkit-android#1693 at 7e2fd14. Android also drains profile removal before taking the SDK lock; that remains separate work under bitkit-android#1687. bitkit-ios#1124 at a0626189 leaves this lock unchanged.

Validation: the new regression was inspected and was not executed in this review. Hosted Run Tests and SwiftFormat succeeded on this head. ovi-reviewer reports that PaykitSdkOperationLockTests passed on an iPhone 17 simulator at this revision. Device testing: not performed in this review. Manual 1 is the controlled admission race and is not a UI journey.

Ready for device testing.

Reviewer model: grok-4.7
Reasoning effort: high

@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: Re-reviewed the full PR at 8592e58b, against base and merge base 7e172c61 (#1124). This is a full reassessment because the comparison base changed since the previous QA review at 3a7210b0. No coverage is inherited.

No new actionable code findings.

PaykitSdkOperationLock now checks, under the same lock that adds a waiter to the queue, whether a wallet reset has started since the operation was admitted. If one has, the ordinary or cancellable operation is rejected with wallet_wipe_in_progress. It can no longer queue behind a reset that is waiting for that operation's outer owner. An operation rejected at this point would always have failed the existing post-lock validation, so no call that used to succeed is newly rejected. The reset's own acquire passes no generation and is never rejected. Work queued before the reset still runs first and still fails validation. The ordinary operation that holds the reset back is not cancelled. In debug builds the new test-only init defaults to nil, so the existing PaykitSdkOperationLock() call still resolves. In release builds that code is compiled out and the implicit init() remains. The new regression test covers both entry paths and cleans up its tasks if an assertion fails.

Android parity: bitkit-android#1693 at 7e2fd14f adds the same check in PaykitSdkOperationLock.acquire. Only this contract was compared. Profile-removal ownership stays out of scope, as the PR states.

Validation: by source analysis only. No Swift toolchain was available here, so nothing was built or run. At this head, hosted Run Tests was still pending; SwiftFormat passed.

Recommended before device testing: wait for hosted Run Tests at 8592e58b to pass. That run includes PaykitSdkOperationLockTests.

Device testing: not performed in this review. Manual case 1 runs at the lock level with controlled barriers. No wallet reset or authenticated journey is needed for this change.

Reviewer model: claude-opus-5-5
Reasoning effort: high

@ovi-reviewer ovi-reviewer Bot 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.

Verdict: ✅ Approve

Findings: None.

QA for 6c0a160:

Test 1: Manual 1 — stale admission during teardown ✅ XCTest assertions confirm stale rejection and task drainage for both admission paths.

On an iPhone 17 simulator with iOS 26.5, testWipeRejectsPreviouslyAdmittedOuterOwnerBeforeQueueInsertion passed for the ordinary and cancellable paths. The complete PaykitSdkOperationLockTests selection passed. The recording shows the test host; the queue behavior is checked by XCTest assertions and its result.

manual1-2x.2x.mp4

Check 1: Combined-tree build ✅ The pinned head builds for the iOS simulator.

The app and test bundle built with Xcode 27.0. git diff --check passed for the delta.

The earlier review and simulator report covered the same controlled race at 3a7210b; the lock and that regression are unchanged, while the surrounding service and dependencies changed. This review covers 3a7210b..6c0a160, including the updated parent stack and queue integration. The controlled case does not drive the parent stack's UI changes, an authenticated network flow, or an actual wallet reset.

Companion bitkit-android#1693 fix: bind profile removal to its original owner has the matching admission and generation recheck; its broader recovery integration is separate.


Reviewed by gpt-6.1-terra-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

@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: Re-reviewed the full PR at 6c0a160 after the merge base changed since the saved assessment. No coverage is inherited. The full diff is against merge base c18e7475 (#1124).

No new actionable code findings.

PaykitSdkOperationLock rechecks the wipe flag and admitted generation in the same critical section that inserts an ordinary or cancellable waiter. An operation admitted before wallet reset, then scheduled after the reset is already waiting on that operation's outer owner, is rejected with the existing wallet_wipe_in_progress storage error before it is linked into the queue. That is the same failure validate reports after a successful acquire, including a caller-supplied generation such as activateRegisteredIdentity. withWalletWipe acquires without a generation, so the reset still takes the lock. Waiters already queued before beginWipe stay ahead of the reset, fail validate, and release. Nested reset work still bypasses the queue through ownsWipe. The debug-only admission hook is nil in production, and the release build keeps the implicit initializer.

Android parity: bitkit-android#1693 at 7e2fd14 performs the same stale-admission check inside PaykitSdkOperationLock.acquire before enqueue. Only that contract was compared. Profile-removal ownership remains the separate work in bitkit-android#1687, as this PR states.

Validation: source analysis of the pinned diff, lock callers (withSdk, withPublicRead, withWalletWipe, AppReset.wipe), and the new regression. No Swift toolchain or simulator was available here, so this review did not build or run tests. Hosted Run Tests and SwiftFormat succeeded on 6c0a160. The regression covers both admission paths and was inspected, not executed locally.

Device testing: not performed in this review. Manual case 1 is the controlled lock race and is not a UI journey. No wallet reset or authenticated journey is required for this change.

Ready for device testing.

Reviewer model: grok-4.7
Reasoning effort: high

@ovi-reviewer ovi-reviewer Bot 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.

Verdict: ✅ Approve

Findings: None.

QA for 3f88708:

Check 1: Retained Manual 1 — stale admission during teardown ✅ The earlier simulator pass remains applicable to the unchanged queue and regression.

I retained the passing report at 6c0a160: XCTest verified rejection and task drainage for both ordinary and cancellable admission paths on an iPhone 17 simulator with iOS 26.5. The lock, regression, teardown and reached Paykit caller paths are byte-identical, and the Paykit pin is unchanged. This case was not rerun.

Check 2: Composed app build ✅ The unmodified head builds for an arm64 iOS simulator.

I built 3f88708 with Xcode 27.0 and the committed dependency pins. The checkout remained clean, and git diff --check passed for the delta. The unit/UI suites and formatting were not rerun in this pass.

Reviewed 6c0a160..3f88708, focusing on this change and its interactions with the merged parent; there were no prior open findings. This was not a separate full audit of inherited Blocktank, notification, payment or translation changes. No new device case, authenticated network journey or actual wallet reset was driven.

Companion: bitkit-android#1693 fix: bind profile removal to its original owner at 7e2fd14 has the matching atomic admission/generation recheck. Its broader profile-removal ownership and recovery integration exceeds this iOS change.


Reviewed by gpt-6.1-sol-high

@ovi-reviewer ovi-reviewer Bot 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.

Verdict: ✅ Approve

Findings: None.

QA for 35e267d:

Check: Stale admission during teardown (Manual 1) ✅ The earlier simulator pass remains applicable to the unchanged queue regression.

I retained the passing simulator report at 6c0a160, which verifies rejection and task drainage for both ordinary and cancellable admission. The complete lock and regression, SDK queue/wipe wrappers and reset orchestration are byte-identical. Paykit is unchanged; the LDK update does not affect this controlled case. The original recording is in that report. This case was not rerun.

Check: Current combined app builds ✅ The unmodified head builds for an arm64 iOS simulator.

I built the app with Xcode 27.0 and the committed dependency pins for an iPhone 17 / iOS 26.5 simulator destination. The checkout remained clean, and git diff --check passed. The unit/UI suites and SwiftFormat were not rerun in this session.

Reviewed 3f88708..35e267d, focusing on the queue fix and its interactions with the updated parent subscription guards, session startup and payment paths. There were no prior open findings. This is not a separate full audit or device validation of inherited allowance, send, hardware or CI changes. No new device case, authenticated network journey or actual wallet reset was driven; Manual 1 remains controlled component evidence.

Companion bitkit-android#1693 fix: bind profile removal to its original owner at 4d67734 has the matching atomic admission/generation recheck. Its broader ownership and recovery work exceeds this queue patch.


Reviewed by gpt-6.1-sol-high

@ovitrif ovitrif modified the milestones: 3.1.0, 3.1.1 Oct 29, 2026
@ovitrif ovitrif modified the milestones: 3.1.1, 3.1.0 Oct 29, 2026
@ovitrif

ovitrif commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator

@ben-kaufman Moving this PR to 3.2.0 per the 3.1.0 cut line (https://synonymworkspace.slack.com/archives/C07BJ7DNPCG/p1793307568629859): the restore and profile-deletion stacks harden behaviour that already shipped in 3.0, so they keep landing on master and ship in 3.2.

@ovitrif ovitrif modified the milestones: 3.1.0, 3.2.0 Oct 29, 2026

@ovi-reviewer ovi-reviewer Bot 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.

Verdict: ✅ Approve

Findings: None.

QA for 53b67b0:

Check: Stale admission during teardown (Manual 1) ✅ The earlier simulator pass remains applicable to the unchanged lock and controlled regression.

I retained the passing simulator report at 6c0a160, which verifies stale rejection and task drainage for ordinary and cancellable admission. The complete PaykitSdkOperationLock class and the regression function are byte-identical; the regression creates its own locks and does not enter the changed service wrappers. Package pins are unchanged since the previous review. The original recording is in that report. This case was not rerun.

Check: Current app and test targets build ✅ The unmodified head builds for an arm64 iOS simulator.

xcodebuild build-for-testing succeeded with Xcode 27.0 for an iPhone 17 / iOS 26.5 destination and the committed package pins. The checkout remained clean, and git diff --check passed. Two AppIntents metadata-extraction warnings remain. No test suite or SwiftFormat execution was repeated in this pass.

Reviewed 35e267d..53b67b0, including the parent refresh-deferral changes and their interaction with queue admission, cancellation and teardown. There were no prior open inline findings. No new device case, authenticated network flow or actual wallet reset was driven. The inherited connection-close navigation and broader parent journeys were inspected in source and are not certified by the retained lock test.

Companion bitkit-android#1693 fix: bind profile removal to its original owner at 716e88e retains the matching atomic wipe/generation recheck before queue insertion. Its broader ownership, recovery and acknowledgement work exceeds this iOS queue patch.


Reviewed by gpt-6.1-sol-high

@ovi-reviewer ovi-reviewer Bot 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.

Verdict: ✅ Approve

Findings: None.

QA for 6dca681:

Check: Stale admission during teardown (Manual 1) ✅ The earlier simulator pass remains applicable to the unchanged lock and controlled regression.

I retained the passing simulator report at 6c0a160, which verifies stale rejection and task drainage for ordinary and cancellable admission. The complete PaykitSdkOperationLock class and testWipeRejectsPreviouslyAdmittedOuterOwnerBeforeQueueInsertion are byte-identical at this head. The regression uses its own locks and controlled resources. Its recording is in the linked report; this case was not rerun.

Check: Current app and test targets build ✅ The unmodified head builds for an arm64 iOS simulator.

xcodebuild build-for-testing succeeded with Xcode 27.0, the committed package pins and an iPhone 17 / iOS 26.5 simulator destination. Both checkouts remained clean, and git diff --check passed. Two AppIntents metadata-extraction warnings remain. Test execution and SwiftFormat were not repeated in this pass.

Reviewed 53b67b0..6dca681, including the new parent receive/observation, publication and backup paths and their interaction with queue admission, cancellation and teardown. There were no prior open inline findings. The inherited sharing UI, public-address rotation, backup compatibility and broader payment journeys received source inspection only; this run adds no device coverage for them. No authenticated network flow or actual wallet reset was driven. Manual 1 remains controlled component evidence.

Companion bitkit-android#1693 fix: bind profile removal to its original owner at 716e88e retains the matching atomic wipe/generation recheck before queue insertion. Its broader ownership, recovery and acknowledgement work exceeds this iOS queue patch.


Reviewed by gpt-6.1-sol-high

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.

5 participants