fix(apple): serialize connection teardown#142
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR fixes a critical race condition in ChangesiOS Connection Teardown Lifecycle Fix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 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 |
hyochan
added
expo-iap
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors connection management in the OpenIapModule to handle concurrency and race conditions using a generation-based locking mechanism. It introduces a connectionLock and connectionGeneration to ensure that initialization and cleanup tasks are synchronized and associated with the correct connection state. Feedback suggests optimizing initConnection by restoring an early check for the initialized state to avoid unnecessary task creation. Additionally, it is recommended to prevent task churn in startMessageListener by verifying if a listener task is already active before restarting it.
|
@coderabbitai review |
|
/gemini review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request refactors the connection management in OpenIapModule to ensure thread safety during concurrent initialization and termination. It introduces a generation-based locking mechanism using NSLock and a connectionGeneration counter to prevent race conditions. A new test case was added to verify that concurrent connection toggles do not cause crashes. The review feedback correctly identifies that the messageListenerTask is not restarted upon reconnection, which would lead to missed billing issue events; a suggestion was provided to include startMessageListener() in the initialization flow.
|
@coderabbitai review |
|
/gemini review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request refactors the OpenIapModule connection management to address race conditions during iOS connection teardown. It introduces thread-safe locking, connection generation tracking, and refined task lifecycle management to ensure reliable initialization and cleanup. The changes include a new regression test for concurrent connection operations and updated documentation for the associated patch releases. A review comment correctly identifies a potential issue where the unfinishedTransactionTask is not cleared, which should be addressed to ensure proper re-processing of unfinished transactions.
There was a problem hiding this comment.
Pull request overview
This PR addresses a crash/race on Apple platforms where endConnection() could run concurrently with an in-flight initConnection(), leading to use-after-free behavior during teardown. It introduces serialized connection lifecycle management in OpenIapModule, adds a stress test for concurrent init/teardown, and documents the related patch releases.
Changes:
- Serialize
OpenIapModuleconnection lifecycle via a lock plus generation tokens; cancel/await init work before teardown. - Track/cancel additional StoreKit background work (unfinished transaction processing) and stabilize StoreKit message listener restoration across reconnects.
- Add an Apple test covering concurrent
initConnection()/endConnection()calls and add release notes for the patch set.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/apple/Sources/OpenIapModule.swift | Adds lock+generation-based serialization for init/end connection; improves teardown and listener/task management. |
| packages/apple/Sources/Helpers/IapState.swift | Adds helper to detect whether subscription billing issue listeners are registered. |
| packages/apple/Tests/OpenIapTests/OpenIapProviderTests.swift | Adds a concurrent init/end stress test to help prevent regressions. |
| packages/docs/src/pages/docs/updates/releases.tsx | Adds release note entry describing the Apple lifecycle race fix and coordinated wrapper releases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai review |
|
/gemini review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request addresses a race condition in the iOS lifecycle where endConnection() could conflict with an in-flight initConnection() by introducing a generation-based locking mechanism using NSLock and UInt64 counters. The changes ensure thread safety and proper task cancellation during connection state transitions, supported by new concurrency tests and updated release documentation. Feedback suggests improving the robustness of the endTask cleanup by capturing resources before asynchronous execution to handle potential deallocation of the shared module. Additionally, the reviewer recommends replacing the polling logic in the initialization loop with a more reactive synchronization primitive, such as a continuation, to improve efficiency.
|
/gemini review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces the OpenIapConnectionLifecycle helper to manage StoreKit connection resources and tasks, effectively resolving race conditions between initConnection and endConnection through generation tracking and locking. The OpenIapModule has been refactored to utilize this manager for synchronized cleanup and task management, supported by a new concurrency test and updated release documentation. Feedback was provided regarding a redundant guard statement in the new lifecycle helper's product manager retrieval logic.
|
@coderabbitai review |
|
/gemini review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new OpenIapConnectionLifecycle helper to manage the lifecycle of StoreKit connections, effectively resolving race conditions between initConnection and endConnection. The OpenIapModule has been refactored to utilize this lifecycle manager, and a new test case verifies the stability of concurrent connection calls. Feedback suggests enhancing error logging during task cancellation, optimizing the retry loop in the initialization process to prevent busy-waiting, and adding redundant state checks after suspension points to further improve concurrency safety.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
packages/docs/src/pages/docs/updates/releases.tsx (1)
29-196: ⚡ Quick winRun
bun audit:docsbefore merging this docs change.The new release-note entry is well-structured, all
target="_blank"anchors carryrel="noopener noreferrer", internal<Link>routes match existing usage patterns, and the six framework package versions are consistent with the preceding patch series.No code issues were found in the added content, but the project's coding guidelines require running
bun audit:docsbefore pushing API/Type documentation edits topackages/docs.As per coding guidelines:
packages/docs/**/*: "Runbun audit:docsbefore pushing API/Type documentation edits."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/docs/src/pages/docs/updates/releases.tsx` around lines 29 - 196, The PR needs to run the project's docs audit and include its results: from the change that adds the release entry with id "apple-2-1-7-framework-ios-connection-teardown-patches", run "bun audit:docs" at the repo root, address any reported issues (fix lint/audit failures or update generated artifacts), stage the resulting changes, and push a new commit so the audit passes before merging.packages/apple/Tests/OpenIapTests/OpenIapProviderTests.swift (1)
105-120: ⚡ Quick winConsider adding a post-chaos re-init assertion to prove the lifecycle is left in a functional state.
The test currently validates crash-absence but not functional correctness after concurrent storms. A post-group
initConnection()assertion would also catch cases where the module gets stuck in a bad generation/task state.✅ Proposed addition
_ = try? await module.endConnection() + + // Verify lifecycle is still usable after concurrent chaos + let reconnected = try await module.initConnection() + XCTAssertTrue(reconnected, "Module should be reconnectable after concurrent init/end stress") + _ = try? await module.endConnection() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apple/Tests/OpenIapTests/OpenIapProviderTests.swift` around lines 105 - 120, The test testConcurrentInitAndEndConnectionDoesNotCrash only checks for crashes; after the withTaskGroup completes, call OpenIapModule.shared.initConnection() once more and assert it succeeds (or does not throw/returns expected value) to verify the module can be re-initialized after concurrent init/end storms; locate the test function and add a final await try? or XCTAssertNoThrow wrapper around OpenIapModule.shared.initConnection() (and optionally a subsequent endConnection() to clean up) to prove the lifecycle is functional.packages/apple/Sources/OpenIapModule.swift (1)
1395-1440: 💤 Low valueBrief
isInitialized == truewindow after cancellation is handled byensureConnection().
setInitialized(true)is called at line 1428 before the finalensureCurrentchecks. If the task is cancelled immediately after, there is a narrow window whereisInitialized == truewhile the endTask cleanup is still pending. This is safe becauseensureConnection()(line 1452) guards all public entry points by first awaiting any in-progressendTaskbefore re-checkingisInitialized. Documenting this invariant explicitly—either here or onensureConnection()—would help future maintainers reason about the design.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apple/Sources/OpenIapModule.swift` around lines 1395 - 1440, performInitConnection sets state.setInitialized(true) before final connection.ensureCurrent checks, which creates a narrow window where isInitialized == true while cancellation/endTask cleanup may still be pending; add a short explicit doc comment describing this invariant and the intended synchronization: explain on connection.ensureCurrent (or the public ensureConnection wrapper) that callers must await any in-progress endTask and re-check isInitialized after ensureCurrent returns, and note that performInitConnection intentionally sets state.setInitialized(true) prior to the final ensureCurrent to allow early visibility while ensureCurrent enforces safety for public entry points; reference performInitConnection, state.setInitialized(true), and connection.ensureCurrent/ensureConnection in the comment so future maintainers can locate the logic.packages/apple/Sources/Helpers/OpenIapConnectionLifecycle.swift (2)
195-209: 💤 Low valueSilent-return vs. throw asymmetry in
startMessageListenerTaskis intentional but undocumented.When
generationisnilandendTask != nil, the method silently returns (the guard falls through) rather than throwingCancellationError. Whengenerationis non-nil and the check fails, it throws. This asymmetry is correct—nil-generation callers usetry?for best-effort start—but a brief comment explaining the nil-generation "best-effort" contract would aid maintainability.📝 Suggested doc-comment
+ /// - Parameter generation: When `nil`, the method is best-effort: it silently + /// returns if an end-task is in progress or a listener is already running. + /// When non-`nil`, it throws `CancellationError` on a generation mismatch. func startMessageListenerTask( generation: UInt64?, makeTask: () -> Task<Void, Never> ) throws {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apple/Sources/Helpers/OpenIapConnectionLifecycle.swift` around lines 195 - 209, The behavior of startMessageListenerTask is asymmetric: when generation is non-nil and mismatches it throws CancellationError, but when generation is nil and endTask != nil it silently returns (best-effort start used by callers who call try?). Add a concise doc-comment above startMessageListenerTask explaining the contract: nil generation means "best-effort" (no throw on existing endTask/messageListenerTask), non-nil generation enforces equality and throws CancellationError on mismatch, and mention callers may use try? for best-effort invocation; reference the parameters generation, makeTask and the fields endTask and messageListenerTask in the comment for clarity.
163-209: ⚡ Quick win
makeTaskfactory closure is invoked while holding theNSLock.All three
start*Taskmethods callmakeTask()insidewithLock. The concrete factories only perform anOpenIapLog.debug(...)call and aTask { ... }allocation — both safe underNSLock(os-log is signal-safe; Task creation just schedules async work). However, the method signatures accept arbitrary() -> Task<…>closures, so the contract "factory must be lock-safe (no blocking, no other lock acquisition)" is implicit. A doc-comment on each method stating this constraint would prevent future callers from inadvertently introducing lock-order inversions.📝 Suggested doc-comment
+ /// - Note: `makeTask` is called while the internal lock is held. It must be + /// non-blocking, must not acquire other locks, and should perform only + /// lightweight work (e.g., allocating a Task). func startTransactionListenerTask( generation: UInt64, makeTask: () -> Task<Void, Error> ) throws {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apple/Sources/Helpers/OpenIapConnectionLifecycle.swift` around lines 163 - 209, The three methods startTransactionListenerTask, startUnfinishedTransactionTask and startMessageListenerTask invoke the makeTask() factory while holding the NSLock inside withLock, which forces callers to provide a lock-safe factory; add a brief doc-comment to each method (referencing startTransactionListenerTask, startUnfinishedTransactionTask, startMessageListenerTask and the makeTask closure) that explicitly states the factory closure must be non-blocking, must not acquire other locks or perform long-running work, and should only create and return a Task (or else move makeTask() invocation outside the withLock block if you want to remove the caller constraint).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/apple/Sources/Helpers/OpenIapConnectionLifecycle.swift`:
- Around line 195-209: The behavior of startMessageListenerTask is asymmetric:
when generation is non-nil and mismatches it throws CancellationError, but when
generation is nil and endTask != nil it silently returns (best-effort start used
by callers who call try?). Add a concise doc-comment above
startMessageListenerTask explaining the contract: nil generation means
"best-effort" (no throw on existing endTask/messageListenerTask), non-nil
generation enforces equality and throws CancellationError on mismatch, and
mention callers may use try? for best-effort invocation; reference the
parameters generation, makeTask and the fields endTask and messageListenerTask
in the comment for clarity.
- Around line 163-209: The three methods startTransactionListenerTask,
startUnfinishedTransactionTask and startMessageListenerTask invoke the
makeTask() factory while holding the NSLock inside withLock, which forces
callers to provide a lock-safe factory; add a brief doc-comment to each method
(referencing startTransactionListenerTask, startUnfinishedTransactionTask,
startMessageListenerTask and the makeTask closure) that explicitly states the
factory closure must be non-blocking, must not acquire other locks or perform
long-running work, and should only create and return a Task (or else move
makeTask() invocation outside the withLock block if you want to remove the
caller constraint).
In `@packages/apple/Sources/OpenIapModule.swift`:
- Around line 1395-1440: performInitConnection sets state.setInitialized(true)
before final connection.ensureCurrent checks, which creates a narrow window
where isInitialized == true while cancellation/endTask cleanup may still be
pending; add a short explicit doc comment describing this invariant and the
intended synchronization: explain on connection.ensureCurrent (or the public
ensureConnection wrapper) that callers must await any in-progress endTask and
re-check isInitialized after ensureCurrent returns, and note that
performInitConnection intentionally sets state.setInitialized(true) prior to the
final ensureCurrent to allow early visibility while ensureCurrent enforces
safety for public entry points; reference performInitConnection,
state.setInitialized(true), and connection.ensureCurrent/ensureConnection in the
comment so future maintainers can locate the logic.
In `@packages/apple/Tests/OpenIapTests/OpenIapProviderTests.swift`:
- Around line 105-120: The test testConcurrentInitAndEndConnectionDoesNotCrash
only checks for crashes; after the withTaskGroup completes, call
OpenIapModule.shared.initConnection() once more and assert it succeeds (or does
not throw/returns expected value) to verify the module can be re-initialized
after concurrent init/end storms; locate the test function and add a final await
try? or XCTAssertNoThrow wrapper around OpenIapModule.shared.initConnection()
(and optionally a subsequent endConnection() to clean up) to prove the lifecycle
is functional.
In `@packages/docs/src/pages/docs/updates/releases.tsx`:
- Around line 29-196: The PR needs to run the project's docs audit and include
its results: from the change that adds the release entry with id
"apple-2-1-7-framework-ios-connection-teardown-patches", run "bun audit:docs" at
the repo root, address any reported issues (fix lint/audit failures or update
generated artifacts), stage the resulting changes, and push a new commit so the
audit passes before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75e31910-8359-47cf-b730-dd84d527ab23
📒 Files selected for processing (5)
packages/apple/Sources/Helpers/IapState.swiftpackages/apple/Sources/Helpers/OpenIapConnectionLifecycle.swiftpackages/apple/Sources/OpenIapModule.swiftpackages/apple/Tests/OpenIapTests/OpenIapProviderTests.swiftpackages/docs/src/pages/docs/updates/releases.tsx
|
/gemini review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new OpenIapConnectionLifecycle helper to manage StoreKit connection states and tasks, specifically addressing race conditions between initialization and teardown. OpenIapModule is refactored to utilize this helper, implementing a generation-based locking mechanism to synchronize concurrent calls and ensure resource cleanup. Feedback was provided regarding the endTask cleanup logic in OpenIapConnectionLifecycle, where using a defer block is recommended to ensure the task handle is cleared even upon cancellation, preventing potential infinite loops in initConnection.
Summary
Closes #140
Test plan
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
Documentation