From f78fc3e700bee529559a022f6b0542932a2a1fb5 Mon Sep 17 00:00:00 2001 From: tellyworth Date: Fri, 28 Aug 2026 13:47:34 +1000 Subject: [PATCH 1/2] Add failing tests for websocket reconnection wedge 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 Claude-Session: https://claude.ai/code/session_013N7ff8Y5ytsSLQ225boWwG --- SimperiumTests/MockWebSocketInterface.h | 3 + SimperiumTests/MockWebSocketInterface.m | 18 ++++ SimperiumTests/SPWebSocketInterfaceTests.m | 112 +++++++++++++++++++++ 3 files changed, 133 insertions(+) diff --git a/SimperiumTests/MockWebSocketInterface.h b/SimperiumTests/MockWebSocketInterface.h index d672884c..5adf8c3e 100644 --- a/SimperiumTests/MockWebSocketInterface.h +++ b/SimperiumTests/MockWebSocketInterface.h @@ -19,4 +19,7 @@ - (NSSet*)mockSentMessages; - (void)mockReceiveMessage:(NSString*)message; +- (NSArray*)mockStartedBucketNames; +- (void)mockClearStartedBucketNames; + @end diff --git a/SimperiumTests/MockWebSocketInterface.m b/SimperiumTests/MockWebSocketInterface.m index 4fdc74d2..9efce652 100644 --- a/SimperiumTests/MockWebSocketInterface.m +++ b/SimperiumTests/MockWebSocketInterface.m @@ -30,6 +30,7 @@ - (void)startChannels; @interface MockWebSocketInterface() @property (nonatomic, strong, readwrite) NSMutableSet* mutableSentMessages; +@property (nonatomic, strong, readwrite) NSMutableArray* mutableStartedBucketNames; @end @@ -52,6 +53,14 @@ - (void)mockReceiveMessage:(NSString*)message { [super webSocket:nil didReceiveMessage:message]; } +- (NSArray*)mockStartedBucketNames { + return self.mutableStartedBucketNames; +} + +- (void)mockClearStartedBucketNames { + [self.mutableStartedBucketNames removeAllObjects]; +} + #pragma mark ==================================================================================== #pragma mark Overriden Methods @@ -68,6 +77,15 @@ - (void)openWebSocket { // Do not open a SPRWebSocket instance } +- (void)start:(SPBucket*)bucket { + if (self.mutableStartedBucketNames == nil) { + self.mutableStartedBucketNames = [NSMutableArray array]; + } + + [self.mutableStartedBucketNames addObject:bucket.name]; + [super start:bucket]; +} + - (BOOL)open { // The "WebSocket" is always open, for unit testing purposes return YES; diff --git a/SimperiumTests/SPWebSocketInterfaceTests.m b/SimperiumTests/SPWebSocketInterfaceTests.m index bf9776af..8c5198fc 100644 --- a/SimperiumTests/SPWebSocketInterfaceTests.m +++ b/SimperiumTests/SPWebSocketInterfaceTests.m @@ -10,12 +10,59 @@ #import "XCTestCase+Simperium.h" #import "MockSimperium.h" #import "MockWebSocketInterface.h" +#import "Simperium+Internals.h" #import "SPLogger.h" #import "JSONKit+Simperium.h" #import "Config.h" +#pragma mark ==================================================================================== +#pragma mark Constants +#pragma mark ==================================================================================== + +static NSTimeInterval const SPReconnectionDelay = 2.5; + + +#pragma mark ==================================================================================== +#pragma mark SPWebSocketInterface: Exposing Private Methods +#pragma mark ==================================================================================== + +@interface SPWebSocketInterface () +@property (nonatomic, assign, readwrite) BOOL open; +- (instancetype)initWithSimperium:(Simperium *)s; +- (void)openWebSocket; +- (void)webSocket:(SPWebSocket *)webSocket didCloseWithCode:(NSInteger)code reason:(NSString *)reason wasClean:(BOOL)wasClean; +@end + + +#pragma mark ==================================================================================== +#pragma mark Simperium: Exposing Private Methods +#pragma mark ==================================================================================== + +@interface Simperium () +- (void)startNetworkManagers; +@end + + +#pragma mark ==================================================================================== +#pragma mark CountingWebSocketInterface +#pragma mark ==================================================================================== + +// Counts reconnection attempts without ever touching the network +@interface CountingWebSocketInterface : SPWebSocketInterface +@property (nonatomic, assign, readwrite) NSUInteger openAttempts; +@end + +@implementation CountingWebSocketInterface + +- (void)openWebSocket { + self.openAttempts++; +} + +@end + + #pragma mark ==================================================================================== #pragma mark SPWebSocketInterfaceTests #pragma mark ==================================================================================== @@ -109,4 +156,69 @@ - (void)testRemoteIndexRequest { XCTAssertTrue(responseSent, @"Index Request-Response wasn't sent!!"); } +- (void)testSocketClosedBeforeFinishingHandshakeSchedulesReconnection { + // A close that arrives before webSocketDidOpen (open == NO) used to be treated as an + // intentional close, wedging the interface: no socket, no retry, and nothing upstream + // notices. See SIMPL-75. + MockSimperium* s = [MockSimperium mockSimperium]; + CountingWebSocketInterface* interface = [[CountingWebSocketInterface alloc] initWithSimperium:s]; + + interface.open = NO; + [interface webSocket:nil didCloseWithCode:1006 reason:@"connection dropped mid-handshake" wasClean:NO]; + + [self waitFor:SPReconnectionDelay]; + XCTAssertTrue(interface.openAttempts > 0, @"A close during connection setup must schedule a reconnection"); +} + +- (void)testSocketClosedAfterOpeningSchedulesReconnection { + MockSimperium* s = [MockSimperium mockSimperium]; + CountingWebSocketInterface* interface = [[CountingWebSocketInterface alloc] initWithSimperium:s]; + + interface.open = YES; + [interface webSocket:nil didCloseWithCode:1006 reason:@"connection dropped" wasClean:NO]; + + [self waitFor:SPReconnectionDelay]; + XCTAssertTrue(interface.openAttempts > 0, @"An unexpected close must schedule a reconnection"); +} + +- (void)testSocketClosedWhileNetworkDisabledDoesNotReconnect { + MockSimperium* s = [MockSimperium mockSimperium]; + CountingWebSocketInterface* interface = [[CountingWebSocketInterface alloc] initWithSimperium:s]; + s.networkEnabled = NO; + + interface.open = NO; + [interface webSocket:nil didCloseWithCode:1006 reason:@"connection dropped" wasClean:NO]; + + [self waitFor:SPReconnectionDelay]; + XCTAssertTrue(interface.openAttempts == 0, @"No reconnection should be attempted while networking is disabled"); +} + +- (void)testStopCancelsPendingReconnection { + MockSimperium* s = [MockSimperium mockSimperium]; + SPBucket* bucket = [s bucketForName:NSStringFromClass([Config class])]; + CountingWebSocketInterface* interface = [[CountingWebSocketInterface alloc] initWithSimperium:s]; + + interface.open = YES; + [interface webSocket:nil didCloseWithCode:1006 reason:@"connection dropped" wasClean:NO]; + [interface stop:bucket]; + + [self waitFor:SPReconnectionDelay]; + XCTAssertTrue(interface.openAttempts == 0, @"An intentional stop must cancel any pending reconnection"); +} + +- (void)testStartNetworkManagersRestartsBucketsEvenWhenAlreadyFlaggedAsStarted { + // The networkManagersStarted flag only records that start was called once; the websocket + // may have been dropped since. Restarting must reach the network interface regardless, + // since SPWebSocketInterface's start: is idempotent for healthy connections. + MockSimperium* s = [MockSimperium mockSimperium]; + [s bucketForName:NSStringFromClass([Config class])]; + XCTAssertTrue(s.networkManagersStarted, @"Expected network managers to be started after authentication"); + + [s.mockWebSocketInterface mockClearStartedBucketNames]; + [s startNetworkManagers]; + + XCTAssertTrue(s.mockWebSocketInterface.mockStartedBucketNames.count > 0, + @"Restarting network managers must restart the buckets' network interface"); +} + @end From 8b27277ffa81779cea2dfcb11c6a0c0264fb04c5 Mon Sep 17 00:00:00 2001 From: tellyworth Date: Fri, 28 Aug 2026 13:52:52 +1000 Subject: [PATCH 2/2] Retry websocket connections that drop before completing the handshake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_013N7ff8Y5ytsSLQ225boWwG --- Simperium/SPWebSocketInterface.m | 9 +++++---- Simperium/Simperium.m | 10 +++++++--- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/Simperium/SPWebSocketInterface.m b/Simperium/SPWebSocketInterface.m index 7dbdb259..b8bae278 100644 --- a/Simperium/SPWebSocketInterface.m +++ b/Simperium/SPWebSocketInterface.m @@ -237,7 +237,7 @@ - (void)stop:(SPBucket *)bucket { self.webSocket = nil; // Prevent any pending retries - [NSObject cancelPreviousPerformRequestsWithTarget:self]; + [NSObject cancelPreviousPerformRequestsWithTarget:self selector:@selector(openWebSocket) object:nil]; } - (void)reset:(SPBucket *)bucket completion:(SPNetworkInterfaceResetCompletion)completion { @@ -388,12 +388,13 @@ - (void)webSocket:(SPWebSocket *)webSocket didReceiveMessage:(id)message { } - (void)webSocket:(SPWebSocket *)webSocket didCloseWithCode:(NSInteger)code reason:(NSString *)reason wasClean:(BOOL)wasClean { - if (self.open) { - // Closed unexpectedly, retry + // Any close that reaches us is unexpected: an intentional stop nils the delegate before closing. + // That includes a close arriving before webSocketDidOpen (self.open == NO) — failing to retry + // there left the interface with no socket and nothing scheduled to rebuild it. + if (self.simperium.networkEnabled) { [self performSelector:@selector(openWebSocket) withObject:nil afterDelay:2]; SPLogVerbose(@"Simperium connection closed (will retry): %ld, %@", (long)code, reason); } else { - // Closed on purpose SPLogInfo(@"Simperium connection closed"); } diff --git a/Simperium/Simperium.m b/Simperium/Simperium.m index e0918229..5fe808e0 100644 --- a/Simperium/Simperium.m +++ b/Simperium/Simperium.m @@ -204,12 +204,16 @@ - (SPBucket *)bucketForName:(NSString *)name { #pragma mark ==================================================================================== - (void)startNetworkManagers { - if (!self.networkEnabled || self.networkManagersStarted || !self.appID) { + if (!self.networkEnabled || !self.appID) { return; } - + + // Note: networkManagersStarted only records that this ran once; the websocket may have been + // dropped since (e.g. it closed before completing its handshake). Don't early-return on it: + // the network interface's start: is idempotent for healthy connections, and re-running it is + // the only automatic path that rebuilds a dead one. SPLogInfo(@"Simperium starting network managers..."); - + // If this gets executed before a logout is complete, make sure this gets logged if (self.logoutInProgress) { SPLogError(@"Simperium Error: there is a pending logout operation that hasn't been fulfilled");