Conversation
#625 changes three behaviours and pins two of them. The narrowing of `stop:`'s `cancelPreviousPerformRequestsWithTarget:` ships with no test that notices its removal: reverting that line to the old broad cancel leaves all seven of the PR's tests green. The socket teardown that runs after a close is only exercised through the retry it enables, and the retry assertions are `openAttempts > 0`, so a regression that scheduled a burst of reconnections would pass. `start:` returning early for an authenticated channel is the reason `startNetworkManagers` can now re-run for every bucket on each restart, so that early return is load-bearing and gets a test of its own. Each test was checked against a targeted mutation of `SPWebSocketInterface.m` and fails only under the mutation it is meant to catch. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are isolated to unit tests and the added assertions directly cover previously unpinned reconnection/teardown behaviors without introducing production-code risk.
Pull request overview
Adds additional unit tests around SPWebSocketInterface reconnection/teardown behavior introduced (or made load‑bearing) by the SIMPL-75 fix in the stacked PR, aiming to pin previously untested reconnection paths and prevent regressions.
Changes:
- Adds a counting test double capable of observing “bystander” delayed performs and outbound auth messages without touching the network.
- Adds tests that pin: (1) exactly-one reconnection scheduling, (2)
stop:canceling only its own pending retry, (3) socket teardown on close (retrying or not), and (4)start:being a no-op for already-authenticated channels.
File summaries
| File | Description |
|---|---|
| SimperiumTests/SPWebSocketInterfaceTests.m | Extends the websocket interface unit tests to cover reconnection scheduling, teardown, and authenticated-channel restart behavior. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The test file's `SPReconnectionDelay = 2.5` was not the reconnection delay — the delay is 2, and 2.5 was the test's wait budget. Two of the tests assert that no reconnection happened once the wait elapses, so a production delay raised past 2.5 would have left them passing without ever giving a retry the chance to fire. `SPWebSocketHeartbeatInterval` and `SPWebSocketTimeoutInterval` are the existing convention for naming these intervals. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`-webSocket:didCloseWithCode:` now treats every close that reaches it as unexpected and schedules a reconnection, so an intentional stop must not let one through. It did not, but only because `SPRWebSocket`'s `close` is asynchronous — the ordering here made the guarantee accidental, and contradicted the comment on the delegate method asserting it. `-[SPWebSocket handleTimeout:]` already detaches first. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`-dealloc` tore the network down, which reaches `-[SPWebSocketChannel stop]` and its main-thread `NSAssert`. Nothing guarantees which thread the last release lands on: `SPLogger` reads its weak delegate on its own queue, so a log message still in flight can hold the final reference to a `Simperium` and free it there. The teardown now takes the buckets as an argument so `-dealloc` can hand them to the main queue, which it could not do while the work needed `self`. This was already failing this branch's build on CI. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mokagio
changed the title
Base automatically changed from
mokagio/simpl-75-websocket-reconnection-fix
to
develop
September 7, 2026 04:13
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.
Stacked on #628, which carries @tellyworth's SIMPL-75 fix.
Note
Below this point, is all AI generated. I usually wrap that info in a
<detaills>block, but I've left it available here because it's useful to know how the code works in this repo that hasn't seen activity in a long time. Presumably, we'll have to keep making changes to it, so this knowledge sharing becomes more valuable, and not just for agents that might review the PRs.The fix changes three behaviours and its tests pin two of them:
stop:'s narrowedcancelPreviousPerformRequestsWithTarget:to the old broad cancel leaves all seven existing tests green.openAttempts > 0, so a regression that scheduled a burst of reconnections would pass.start:returning early for an already-authenticated channel is what makes the new per-bucketstartNetworkManagerssafe to re-run, so it is load-bearing and untested.Each new test was checked against a targeted mutation of
SPWebSocketInterface.mand fails only under the mutation it is meant to catch.Three changes to the code under test came out of writing them:
-[Simperium dealloc]was not safe off the main thread. It tore the network down, reaching-[SPWebSocketChannel stop]and its main-threadNSAssert.SPLoggerreads its weak delegate on its own queue, so a log message in flight can hold the last reference to aSimperiumand free it there. This was already crashing this branch's build before the fix, and the same crash is reachable from the apps. Tracked as SIMPL-76.-[SPWebSocketInterface stop:]closed the socket before detaching it. Now that any close schedules a reconnection, an intentional stop avoided one only becauseSPRWebSocket'scloseis asynchronous.How to test
bundle exec fastlane ios testPosted by Claude Code (Opus 5) on behalf of @mokagio with approval.