fix(swift-sdk): tie shielded completion suppression to a sync generation#3733
fix(swift-sdk): tie shielded completion suppression to a sync generation#3733QuantumExplorer wants to merge 2 commits into
Conversation
Replace the boolean `suppressShieldedCompletionEvents` gate with a lock-guarded monotonic generation counter. The boolean was bypassable: a stop (set flag) followed by a restart (clear flag) in the same @mainactor turn re-opened the gate before the stale, already-enqueued completion task ran, leaking the previous run's completion event into the new run. The FFI completion callback now snapshots the generation on the callback thread before enqueueing onto the main actor; stop/clear bump the generation after the Rust drain returns; the main-actor handler drops any event whose snapshot no longer matches the current generation. Start / sync-now no longer reset anything — new events carry the current generation and pass the guard, while a trailing event from a prior run still carries the older generation and is dropped, even on a same-turn restart. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces a boolean suppression flag with a lock-guarded, nonisolated generation counter; FFI callback threads snapshot the generation and pass it into the main-actor handler, which discards completion events whose snapshot doesn't match the manager's current generation. ChangesShielded Sync Generation Counter
Sequence DiagramsequenceDiagram
participant FFIThread as FFI Callback Thread
participant Manager as PlatformWalletManager
participant MainActor as Main Actor
participant Handler as handleShieldedSyncCompleted
FFIThread->>FFIThread: shieldedSyncCompletedCallback()\nsnapshot shieldedSyncGeneration
FFIThread->>Manager: read nonisolated generation
FFIThread->>MainActor: enqueue handleShieldedSyncCompleted(event, generation)
MainActor->>Handler: handleShieldedSyncCompleted(event, generation)
Handler->>Manager: compare generation vs shieldedSyncGeneration.current()
alt generations match
Handler->>Handler: publish event (set lastShieldedSyncEvent)
else generations mismatch
Handler->>Handler: drop stale event
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "ecc49a140c4da3a71d40898f79b5e0c8c94c7a37aa77f748b15484fc4d2505f1"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The generation-counter approach fixes the stale shielded-sync completion race at the Swift/Rust callback boundary without changing the FFI ownership contract. I did not find a functional or security regression in the implementation, but the PR still needs focused regression coverage for the exact stop/clear versus queued-completion ordering this fix relies on.
🟡 1 suggestion(s)
Suggestion: Add regression coverage for stale queued shielded completion events
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift:68
This fix depends on a precise ordering guarantee: the FFI callback snapshots shieldedSyncGeneration before enqueueing onto @MainActor, then stopShieldedSync or clearShielded bumps the generation before that queued completion is delivered. I verified the implementation follows that pattern, but there is still no Swift test coverage exercising shielded-sync completion delivery or the stop/clear race. A later refactor could reintroduce the original boolean-gate problem while still compiling cleanly. Please add a focused regression test that enqueues a completion under generation N, bumps to N+1 before delivery and asserts the stale event is dropped, then verifies a completion captured under the current generation is published.
Inline posting hit GitHub HTTP 422, so this verified finding is posted as a top-level review body.
Regression coverage for the generation-gated suppression in PlatformWalletManager.handleShieldedSyncCompleted(_:generation:). Asserts a completion captured under a superseded generation is dropped (including on a same-turn stop→restart, where a boolean gate would leak the stale event), a current-generation completion is published, and a late straggler does not clobber an already-published event. Lives in the SwiftExampleAppTests Xcode target — the only test target on this repo that links the real SwiftDashSDK (the SwiftTests SPM package depends on a C mock, not the SDK). Verified locally via xcodebuild test (4 cases pass on the iOS Simulator); not currently CI-enforced since the Swift CI job is build-only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the worktree at a70eb9d2edb4f1e18610368235c4463343d802e6 and confirmed the only delta since 07421abc64ef74739de6d12347371e62234987f9 is the new ShieldedSyncGenerationTests.swift regression suite. The underlying generation-based stale-event guard remains correctly implemented in the production shielded-sync path, and the new tests exercise the same-turn restart and late-straggler cases that previously needed coverage. I did not find any remaining correctness issues or false-positive review findings to carry forward.
2 participants
Issue being fixed or feature implemented
Follow-up to #3603 (shielded send end-to-end), which merged with a
boolean
suppressShieldedCompletionEventsgate guarding the shieldedsync completion event after
stop/clear.A reviewer flagged that the boolean is bypassable: the FFI completion
callback re-dispatches onto the
@MainActor, so a final, already-enqueuedevent can land just after
stop/clearreturns. Becausestartclearedthe flag, a caller that stops (or clears) and restarts shielded sync in
the same actor turn re-opens the gate before the stale, queued
completion task runs — so the previous run's completion leaks into the
new run and is published as if it belonged to it.
What was done?
Replaced the boolean with a lock-guarded monotonic generation counter
(
ShieldedSyncGenerationCounter):thread, before enqueueing the event onto the main actor.
stopShieldedSync/clearShieldedbump the generation after the Rustdrain returns.
handleShieldedSyncCompleted(_:generation:)drops any event whosesnapshot no longer matches the current generation.
startShieldedSync/syncShieldedNowno longer reset anything — a newrun's events carry the current generation and pass the guard, while a
trailing event from a prior, stopped run still carries the older
generation and is dropped, even on a same-turn restart.
This is a pure SDK-layer change (no FFI/Rust signature changes); the
example app's
guard isBoundremains as harmless defense-in-depth.How Has This Been Tested?
Built the unified iOS framework + SwiftExampleApp for the simulator
(
./build_ios.sh --target sim) — build succeeded. The change isbehaviorally identical for the common case (events from the live run
publish normally); the generation guard only differs from the old boolean
on the same-actor-turn stop→restart race, which is the bug being fixed.
Breaking Changes
None.
handleShieldedSyncCompletedis an internal method; its newgeneration:parameter is not part of the public API.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit