Retry websocket connections that drop before completing the handshake - #628
Merged
Merged
Conversation
A socket close that arrives before the connection finishes opening is treated as an intentional close: no retry is scheduled, the socket is nil'd, and networkManagersStarted still reads YES, so no automatic path ever rebuilds the connection. Sync silently stops until the app is relaunched or reachability drops completely. These tests document the expected behavior and currently fail: - testSocketClosedBeforeFinishingHandshakeSchedulesReconnection - testStartNetworkManagersRestartsBucketsEvenWhenAlreadyFlaggedAsStarted See SIMPL-75. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013N7ff8Y5ytsSLQ225boWwG
Fixes a permanent sync wedge (SIMPL-75): - SPWebSocketInterface treated a close arriving before webSocketDidOpen as intentional and never retried, even though intentional closes nil the delegate first and can't reach that handler. Any delivered close now schedules a retry, guarded by networkEnabled like didFailWithError. - startNetworkManagers early-returned whenever networkManagersStarted was set, but the flag only records that start ran once — not that a socket exists. After the close above, every automatic restart path (reachability regained, OSX wake) became a no-op, so sync stayed dead until an app relaunch or a full reachability drop. It now re-runs start: for each bucket, which is idempotent for healthy connections. - stop: cancelled every pending perform request on the interface rather than just its own reconnection retry; the cancel is now scoped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013N7ff8Y5ytsSLQ225boWwG
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The behavioral changes are small, directly address the described wedge, and are covered by targeted unit tests that validate the regression and the fix.
Pull request overview
This PR ports the fix for a sync “wedge” where a websocket that drops before completing its handshake would never reconnect, and ensures higher-level restart paths can rebuild a dead websocket. It adds focused unit tests to reproduce (pre-fix) and validate (post-fix) the reconnection and restart behavior.
Changes:
- Retry websocket reconnection on any delivered close (including pre-handshake closes) when networking is enabled.
- Make
startNetworkManagersre-run bucketstart:calls even when previously flagged as started. - Add unit tests + mock instrumentation to verify reconnect scheduling and restart behavior.
File summaries
| File | Description |
|---|---|
| SimperiumTests/SPWebSocketInterfaceTests.m | Adds regression tests for pre-handshake close reconnection and startNetworkManagers restart behavior. |
| SimperiumTests/MockWebSocketInterface.m | Tracks buckets passed to start: to assert startNetworkManagers actually restarts bucket network interfaces. |
| SimperiumTests/MockWebSocketInterface.h | Exposes new mock inspection helpers for started bucket names. |
| Simperium/SPWebSocketInterface.m | Schedules reconnect on any unexpected close when networking is enabled; narrows cancellation in stop: to reconnect selector only. |
| Simperium/Simperium.m | Removes the early return on networkManagersStarted so restarts can rebuild a dropped websocket. |
Review details
- Files reviewed: 5/5 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.
Contributor
Author
|
I reviewed the original PR, #625, and am okay with merging this. It looks odd because I'm the author of this cherry pick PR, but the explanation shall serve. |
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.
@tellyworth's fix from #625, cherry-picked onto current
developso CI runs it.#625 branched before the Buildkite setup landed (#627), so its tests have never run on a build.
Commits and authorship are unchanged, failing tests first and fix second, so the reproduction is still verifiable by checking out the first commit.
#625 carries the full analysis of the wedge and is the place to review the reasoning; this PR exists to put a green build under it. SIMPL-75.
How to test
bundle exec fastlane ios testPosted by Claude Code (Opus 5) on behalf of @mokagio with approval.