Repository navigation
Stop the chain registry crashing the app on opening (4.2.2 fix for master) - #1327
Open
takemiyamakoto wants to merge 5 commits into
Open
takemiyamakoto wants to merge 5 commits into
takemiyamakoto wants to merge 5 commits into
Conversation
4.2.1 (2026.11.11) can crash right after unlock with "Index out of range" in ChainRegistry.webSocketDidChangeState, on the socket state queue (device crash report from TestFlight, iPhone 16 Pro Max, iOS 27.2.1). Since 4.2.0 the socket engine reports state changes on its own queue, so each new chain socket's first change arrives while the registry is still adding the chain list. The delegate read that list without the registry's lock and trapped when the update replaced it mid-read. 4.0.4 reported the change on the updating thread itself, which hid the missing lock. - ChainRegistry: look the chain up under the lock, on the registry's own serial queue, so the shared socket queue never waits for a chain update. Create the lock with the registry (a lazy var can be created twice), read the list under it in performHotBoot and the package adapter, and guard the chain-types map, which the types sync publishes from its own queue. - NetworkIssuesCenter, ChainsIssuesCenter: keep listeners and issues under a lock and call listeners after releasing it. Screens add listeners while the event queue publishes the reconnecting events above. - EthereumConnectionPool, AccountInfoUpdatingService: create their locks with the object instead of lazily. Tests: a deterministic test parks a chain update and shows the state change waits for it; stress tests race state changes against updates, the types map and both issue centres; a real-socket test drives ConnectionFactory, FailoverChainConnection and WebSocketEngine into the registry; plus a hot-boot test. On 4.2.1 five of them fail (three crash the host) and Thread Sanitizer reports the race at ChainRegistry.swift:1066 through ConnectionPool and FailoverChainConnection, the device crash's frames. With the fix, 149 related tests pass under Thread Sanitizer with no reports, and the new tests pass 20 times in a row. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit f3cb359)
Master's reviewed list for the gate is still the 424-name list that came in with #1305. Nine of its names do not exist, and the selected classes have gained tests since, so the gate cannot pass on master. The launch-crash fix adds five more ChainRegistryTests. Recompute the list for master with the fix. The selected classes hold the same tests here as on the 4.2.2 line, so the list is that line's 483 identities and the gate requires exactly 483. A source scan that reproduces the 4.2.1 (478) and 4.2.2 (483) lists exactly gives the same 483 here. The three files now match the 4.2.2 line (20204c7), which also brings two gate self-test fixes from that line. From 8df1106: the fake xcrun no longer races under pipefail. From 6063ef1: the self-test fails unless it reaches its end. The self-test passes: 6 positive contracts and 50 negative or adversarial cases. Tooling only; nothing here is compiled into the app. (cherry picked from commit 20204c7) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
testSocketStateChangesRacingChainUpdatesAreEachPublishedFromACompleteList treated the last round's node on the last chain as its sign that every round had been applied. StreamableProvider hands each batch to the registry on the global user-initiated queue, which is concurrent, so the rounds can reach the registry's write lock in any order. When an earlier round landed last, the node never matched, and the test waited out its 120 s and failed. In the Release -O Core Data gate on master it failed the first run, then 2 of 10 repeats. A temporary probe showed the last round was not the last batch applied in 6 of 40 runs. Wait instead for one ChainsSetupCompleted per batch (the registry posts one each time it applies a batch). Then check that each chain ends on a node from one of the rounds. What the test checks is unchanged: no socket state change is dropped while chains are replaced. Test only; nothing here is compiled into the app. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This reverts commit 89503c7. The next commit, 2cd1011 from #1326, fixes the cause in the registry instead: chain updates are delivered on its serial queue, so they apply in provider order and the test's wait for the last round's node holds again. Taking that commit as it is keeps ChainRegistry.swift and ChainRegistryTests.swift the same on master and in #1326. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 2cd1011)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
iOS 4.2.1 (2026.11.11) can crash right after unlock with
Index out of rangeinChainRegistry.webSocketDidChangeState, on the socket state queue (TestFlight crash report: iPhone 16 Pro Max, iOS 27.2.1). Since 4.2.0, the socket engine reports state changes on its own queue. The delegate read the chain list without the registry's lock while the startup chain update was replacing it. Master has the same code: the five changed files are byte-identical to 4.2.1's.This brings the fix from the 4.2.2 line (
hotfix/4.2.0-polkaswap-usability) to master, together with #1326's chain update ordering commit. It leaves out the 4.2.2 version bump, so master stays 4.2.0 (2026.11.5).Changes
git cherry-pick -x f3cb35961, applied cleanly)ChainRegistrylooks the chain up under its lock, on its own serial queue. Its lock is no longer lazy.performHotBoot, the package adapter andchainsTypesMapread under locks.NetworkIssuesCenterandChainsIssuesCenterkeep their listeners and issues under a lock.EthereumConnectionPoolandAccountInfoUpdatingServicecreate their locks with the object.ChainRegistryTests.swift.20204c732). Master's reviewed list was still the 424-name list from Consolidate native iOS ownership and wallet recovery #1305, which cannot pass. Recomputed for master plus the fix, it is 483 tests: the same set as the 4.2.2 line.git cherry-pick -x 2cd1011a4, from Consolidate every open iOS PR: 4.2.1 (2026.11.11), redesign and brand, config, subscription and test fixes #1326)subscribeToChainstook chain batches onDispatchQueue.global(qos: .userInitiated), which is concurrent. Two batches could reach the registry's write lock in either order, so an older node update could overwrite a newer one.89503c771).10ff8e1e3reverts it in favour of this commit.Local results
This project has no hosted CI gate. Everything ran locally at
2668d3624(basedce8790cd), with Xcode 27.2 on iOS 27.0 simulators.ChainRegistryTests,IssuesCenterConcurrencyTests,IssuesCenterListenerRemovalTests,IssuesCenterReachabilityTests,ConnectionPoolTests,FailoverChainConnectionTests,EthereumNodeFailoverTests,RootTests(Debug)scripts/ci/run-coredata-release-gate.sh, Release -O)testChosenEthereumNode…, 40 iterations each (Release -O)Notes for review
processingQueuehas no QoS of its own. With2cd1011a4, chain update batches therefore run at the QoS they inherit from the provider's utility queue; onDispatchQueue.global(qos: .userInitiated)they ran at user-initiated. These runs showed no slowdown. The same applies to Consolidate every open iOS PR: 4.2.1 (2026.11.11), redesign and brand, config, subscription and test fixes #1326.testChosenEthereumNodeTakesEffect…, which this PR doesn't change, has 10 s waits. In Core Data gate runs right after a fresh app build, it took 8 to 11 s: it failed twice here and took 8.07 s once on the 4.2.2 line. Otherwise it took under 1.1 s. At this head, the gate run right after the rebuild failed on it, and the rerun from the same build passed it in 0.16 s.Merge order
#1326 is still open. It carries the same fix, the same 483-test gate list and the same ordering commit.
ChainRegistry.swiftandChainRegistryTests.swiftare identical on both branches, and a trial merge of this branch with #1326's head is clean.🤖 Generated with Claude Code