Retry websocket connections that drop before completing the handshake - #625
Closed
tellyworth wants to merge 2 commits into
Closed
tellyworth wants to merge 2 commits into
tellyworth wants to merge 2 commits into
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
tellyworth
marked this pull request as ready for review
August 28, 2026 04:02
This was referenced Aug 28, 2026
mokagio
added a commit
that referenced
this pull request
Sep 2, 2026
#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
|
Closing in favor of #628 , which cherry picks the commits here and, being a PR made by a maintainer, run in CI automatically. |
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
2 participants
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.
Fixes a client-side wedge where sync silently stops and never recovers, believed to be the cause of the long-running intermittent sync failure tracked in SIMPL-75.
The bug
Three pieces interlock:
A socket that closes before finishing its handshake never retries.
SPWebSocketInterface didCloseWithCode:only scheduled a retry whenself.openwas YES, butopenis only set inwebSocketDidOpen. A close during connection setup was treated as intentional: the socket was nil'd and nothing was rescheduled. (Intentional closes actually nil the delegate before closing, so they can never reach this handler — every delivered close is unexpected.)Nothing upstream recovers.
Simperium startNetworkManagersearly-returned whenevernetworkManagersStartedwas set — but that flag only means start ran once, not that a socket exists. After the close above, every automatic restart path (reachability regained, OSX wake) became a no-op.The only escape was a full reachability loss (which runs
stopNetworkManagersand resets the flag), an app relaunch, or signing out — which matches the reported symptoms exactly: sync dead for 12–37 hours on perfect networks, immune to logging out, self-healing after a network transition.The fix
didCloseWithCode:now schedules a retry for any delivered close, guarded bynetworkEnabled(mirroringdidFailWithError).startNetworkManagersre-runsstart:per bucket instead of trusting the flag;start:is idempotent for healthy connections.stop:scopes itscancelPreviousPerformRequestsWithTarget:to the reconnect selector instead of cancelling every pending perform on the interface.Tests
First commit adds the tests, second the fix, so the reproduction is verifiable by checking out the first commit:
testSocketClosedBeforeFinishingHandshakeSchedulesReconnection— red before, green aftertestStartNetworkManagersRestartsBucketsEvenWhenAlreadyFlaggedAsStarted— red before, green afterstop:cancels a pending retry, no retry while networking is disabled.Full unit suite: 109 tests, 0 failures (iOS 26.5 simulator).
🤖 Generated with Claude Code
https://claude.ai/code/session_013N7ff8Y5ytsSLQ225boWwG