Skip to content

Commit be0a214

Browse files
authored
fix(api): recycle WebSocket and resubscribe on same-online network path change (#4202)
* fix(api): recycle WebSocket and resubscribe on same-online network path change Resolve zombie subscription state after iOS recycles the TCP route while NWPathMonitor continues reporting path.status == .satisfied (e.g. during a scenePhase inactive -> active transition on the same Wi-Fi network). Two layered bugs were addressed: 1. WebSocketClient.onNetworkStateChange dropped (.online, .online) transitions through `default: break`. NWPathMonitor's pathUpdateHandler only fires on real path changes, so a second .satisfied emission while already online means the underlying path was swapped and the existing URLSessionWebSocketTask is bound to a stale route. The task's state stays .running but reads/writes silently fail, leaving the cached client zombied. Added an explicit (.online, .online) case that cancels the stale task and re-establishes the connection. Guarded by connection?.state == .running so we don't recycle a torn-down connection. 2. AppSyncRealTimeSubscription.subscribe() early-returned when local state was already .subscribed, so after the WebSocket recycled and resumeExistingSubscriptions() fired, no .start request was resent to the server. The server had already forgotten the subscription, so the subscription was silently broken. Added prepareForResubscribe() which resets local state to .none, and call it from resumeExistingSubscriptions() before re-invoking subscribe(). Fixes #3976 * test(websocket): tolerate spontaneous NWPathMonitor fires in integration test `testWebSocketClient_withRealNetworkMonitor_whenPathChangesWhileOnline_shouldRecycle` was failing deterministically on watchOS CI (watchOS 26.x on Apple Watch Series 10). Reproduced locally on watchOS 26.1 with the same 5-second timeout signature. Root cause: the test used `verifyConnected()` which XCTFails on any event other than `.connected`. On watchOS, the real `NWPathMonitor` inside `AmplifyNetworkMonitor` fires `pathUpdateHandler` spontaneously during startup — often multiple times — which either (a) delivers an `.online` before the WebSocketClient's Combine sink attaches, causing the scan to miss the priming event and never reach `(.online, .online)`, or (b) fires an extra `.satisfied` after the client is connected, triggering a recycle that `verifyConnected` sees as an unexpected `.disconnected`. Rewrote the test to: - Subscribe to the WebSocket publisher before calling `connect()`, so no events can be missed during sink-attach. - Count `.connected` events with a thread-safe counter and expect two (initial connect + post-recycle reconnect) rather than asserting exact event sequences. - Tolerate `.disconnected`/`.error`/`.string`/`.data` events so NWPathMonitor-driven noise doesn't fail the test. Verified on watchOS 26.1 (Apple Watch Series 11 simulator) — passes 8/8 WebSocketClientTests, 1325/1325 AWSPluginsCoreTests. iOS 26.1 still passes 8/8. * docs(tests): apply Given/When/Then doc comments to new regression tests The four regression tests added for issue #3976 were written with plain comment blocks instead of the project-standard Given/When/Then doc comments. Retrofit them to match the convention documented in AGENTS.md. Also strengthen the testing-conventions section in AGENTS.md to mark Given/When/Then doc comments as mandatory for all new or modified tests, so future contributors and automated agents follow the rule. Tests updated: - testWebSocketClient_whenNetworkPathChangesWhileOnline_shouldRecycleConnection - testWebSocketClient_withRealNetworkMonitor_whenPathChangesWhileOnline_shouldRecycle - testAmplifyNetworkMonitor_whenOnlineEmittedTwice_publishesOnlineOnlineTuple - testSubscribe_afterOnlineToOnlinePathChange_shouldRecycleAndResubscribe No behavioral change — doc comments only. All 8 WebSocketClientTests pass on iOS 26.1 and watchOS 26.1. * test(api): address PR #4202 review nits on E2E regression test - Drop the obsolete config-location note from the doc comment; every test in the file has the same requirement, enforced in setUp. - Replace the plain `var subscribedCount = 0` with an AtomicInt helper matching the unit-test pattern. The counter is mutated from a Combine sink closure, which can fire on any scheduler — theoretical data race that AtomicInt closes cleanly.
1 parent a98dca6 commit be0a214

7 files changed

Lines changed: 323 additions & 2 deletions

File tree

AGENTS.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,10 @@ swift test --filter AWSCognitoAuthPluginUnitTests # Specific target
9898
- **Unit tests**: XCTest, defined in Package.swift (19 test targets)
9999
- **Integration tests**: Xcode host app projects under `AmplifyPlugins/<Category>/Tests/<Category>HostApp/`
100100
- **Conventions**: Mock via behavior protocols, use `AmplifyTestCommon` for shared utilities, `AmplifyAsyncTesting` for async helpers
101-
- **Test documentation**: Use Given/When/Then doc comments on all test methods:
101+
- **Test documentation (MANDATORY)**: Every new or modified test method
102+
**must** have a Given/When/Then doc comment. No exceptions — this applies
103+
to unit tests, integration tests, and regression tests alike. Reviewers
104+
should reject PRs that add tests without this structure.
102105
```swift
103106
/// Test description
104107
///

AmplifyPlugins/API/Sources/AWSAPIPlugin/AppSyncRealTimeClient/AppSyncRealTimeClient.swift

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,8 +251,13 @@ actor AppSyncRealTimeClient: AppSyncRealTimeClientProtocol {
251251

252252
private func resumeExistingSubscriptions() {
253253
log.debug("[AppSyncRealTimeClient] Resuming existing subscriptions")
254-
for (id, _) in subscriptions {
254+
for (id, subscription) in subscriptions {
255255
Task { [weak self] in
256+
// Reset local state so subscribe() re-sends .start to the
257+
// server. After a reconnect, the server has no memory of
258+
// prior subscriptions, so stale local .subscribed state must
259+
// not short-circuit the resubscription.
260+
await subscription.prepareForResubscribe()
256261
do {
257262
if let cancellable = try await self?.startSubscription(id) {
258263
await self?.storeInConnectionCancellables(cancellable)

AmplifyPlugins/API/Sources/AWSAPIPlugin/AppSyncRealTimeClient/AppSyncRealTimeSubscription.swift

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,15 @@ actor AppSyncRealTimeSubscription {
8484
state.send(.subscribed)
8585
}
8686

87+
/// Reset local subscription state so `subscribe()` will actually resend
88+
/// the `.start` request after a WebSocket reconnect. The server drops
89+
/// subscription state when the connection dies, so local `.subscribed`
90+
/// state is stale and must not short-circuit the resubscription path.
91+
/// Fix for https://github.com/aws-amplify/amplify-swift/issues/3976
92+
func prepareForResubscribe() {
93+
state.send(.none)
94+
}
95+
8796
func unsubscribe() async throws {
8897
guard state.value == .subscribed else {
8998
log.debug("[AppSyncRealTimeSubscription-\(id)] Subscription should be subscribed to be unsubscribed")

AmplifyPlugins/API/Tests/APIHostApp/AWSAPIPluginFunctionalTests/AppSyncRealTimeClientTests.swift

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,100 @@ class AppSyncRealTimeClientTests: XCTestCase {
169169
withExtendedLifetime(cancellables) { }
170170
}
171171

172+
/// End-to-end regression test for https://github.com/aws-amplify/amplify-swift/issues/3976
173+
/// against a real AppSync backend. Simulates the scenePhase-triggered
174+
/// NWPath recycle by driving two .online states through an injected
175+
/// AmplifyNetworkMonitor and asserts that the subscription is actually
176+
/// re-established (server issues a second start_ack).
177+
///
178+
/// - Given:
179+
/// - An AppSyncRealTimeClient wired to a real AmplifyNetworkMonitor
180+
/// and a real AppSync endpoint from the bundled config.
181+
/// - A live subscription that has already received .subscribed from
182+
/// the server (first start_ack confirmed).
183+
/// - When:
184+
/// - After the WebSocketClient's internal sink has attached (200ms),
185+
/// updateState(.online) is called twice on the network monitor,
186+
/// producing the (.online, .online) tuple from issue #3976.
187+
/// - Then:
188+
/// - The WebSocket is recycled, AppSyncRealTimeClient reconnects,
189+
/// resumeExistingSubscriptions() re-sends `start`, and the server
190+
/// returns a second start_ack — causing a second .subscribed event.
191+
func testSubscribe_afterOnlineToOnlinePathChange_shouldRecycleAndResubscribe() async throws {
192+
var cancellables = Set<AnyCancellable>()
193+
194+
let data = try TestConfigHelper.retrieve(
195+
forResource: GraphQLModelBasedTests.amplifyConfiguration
196+
)
197+
let amplifyConfig = try JSONDecoder().decode(JSONValue.self, from: data)
198+
let (endpoint, apiKey) = (amplifyConfig.api?.plugins?.awsAPIPlugin?.asObject?.values
199+
.map { ($0.endpoint?.stringValue, $0.apiKey?.stringValue) }
200+
.first { $0.0 != nil && $0.1 != nil }
201+
.map { ($0.0!, $0.1!) })!
202+
203+
// Inject a real AmplifyNetworkMonitor we can drive directly.
204+
let networkMonitor = AmplifyNetworkMonitor()
205+
206+
let webSocketClient = WebSocketClient(
207+
url: AppSyncRealTimeClientFactory.appSyncRealTimeEndpoint(URL(string: endpoint)!),
208+
handshakeHttpHeaders: [
209+
URLRequestConstants.Header.webSocketSubprotocols: "graphql-ws",
210+
URLRequestConstants.Header.userAgent: AmplifyAWSServiceConfiguration.userAgentLib + " (intg-test-3976)"
211+
],
212+
interceptor: APIKeyAuthInterceptor(apiKey: apiKey),
213+
networkMonitor: networkMonitor
214+
)
215+
let client = AppSyncRealTimeClient(
216+
endpoint: URL(string: endpoint)!,
217+
requestInterceptor: APIKeyAuthInterceptor(apiKey: apiKey),
218+
webSocketClient: webSocketClient
219+
)
220+
defer { Task { await client.reset() } }
221+
222+
// Wait for WebSocketClient's internal sink to attach to the monitor's
223+
// publisher (it's kicked off via a Task in init). Prime with .online
224+
// AFTER the subscriber is attached so the PassthroughSubject actually
225+
// delivers the event. This gets the scan to (.none, .online) —
226+
// WebSocketClient will ignore it because autoConnect is still false.
227+
try await Task.sleep(nanoseconds: 200_000_000)
228+
await networkMonitor.updateState(.online)
229+
230+
let firstSubscribed = expectation(description: "Initial subscription established")
231+
let resubscribedAfterPathChange = expectation(description: "Subscription re-established after (.online, .online)")
232+
resubscribedAfterPathChange.assertForOverFulfill = false
233+
234+
let id = UUID().uuidString
235+
let subscribedCount = AtomicInt()
236+
let subscription = try await client.subscribe(
237+
id: id,
238+
query: Self.appSyncQuery(with: subscriptionRequest)
239+
).sink { event in
240+
if case .subscribed = event {
241+
let count = subscribedCount.increment()
242+
if count == 1 {
243+
firstSubscribed.fulfill()
244+
} else {
245+
resubscribedAfterPathChange.fulfill()
246+
}
247+
}
248+
}
249+
cancellables.insert(subscription)
250+
251+
try await client.connect()
252+
await fulfillment(of: [firstSubscribed], timeout: 10)
253+
254+
// Simulate the path-recycle: second .online emission produces
255+
// (.online, .online) through the scan — the exact bug tuple.
256+
// In the buggy code, nothing happens; WebSocketClient keeps the
257+
// zombie connection. With the fix, it should tear down and reconnect,
258+
// and AppSyncRealTimeClient.resumeExistingSubscriptions() should
259+
// re-subscribe.
260+
await networkMonitor.updateState(.online)
261+
262+
await fulfillment(of: [resubscribedAfterPathChange], timeout: 15)
263+
withExtendedLifetime(cancellables) { }
264+
}
265+
172266
private func makeOneSubscription(
173267
id: String = UUID().uuidString,
174268
onSubscriptionEvents: ((AppSyncSubscriptionEvent) -> Void)?
@@ -201,3 +295,14 @@ class AppSyncRealTimeClientTests: XCTestCase {
201295
}
202296

203297
}
298+
299+
private final class AtomicInt: @unchecked Sendable {
300+
private var value: Int = 0
301+
private let lock = NSLock()
302+
func increment() -> Int {
303+
lock.lock()
304+
defer { lock.unlock() }
305+
value += 1
306+
return value
307+
}
308+
}

AmplifyPlugins/Core/AWSPluginsCore/WebSocket/WebSocketClient.swift

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,23 @@ extension WebSocketClient {
292292
case (.offline, .online):
293293
log.debug("[WebSocketClient] NetworkMonitor - Device back online")
294294
await createConnectionAndRead()
295+
case (.online, .online):
296+
// NWPathMonitor's pathUpdateHandler only fires on real path
297+
// changes, so a second .satisfied emission while we were already
298+
// online means the underlying path was swapped (e.g., iOS recycled
299+
// the TCP route during a scenePhase transition). The existing
300+
// URLSessionWebSocketTask is now bound to a stale route and any
301+
// further reads/writes will silently fail, leaving the client in
302+
// a zombie state that cached consumers can't recover from.
303+
// Fix for https://github.com/aws-amplify/amplify-swift/issues/3976
304+
guard connection?.state == .running else {
305+
log.debug("[WebSocketClient] NetworkMonitor - Path changed but connection is not running, skipping recycle")
306+
break
307+
}
308+
log.debug("[WebSocketClient] NetworkMonitor - Network path changed while online, recycling connection")
309+
connection?.cancel(with: .invalid, reason: nil)
310+
subject.send(.disconnected(.invalid, nil))
311+
await createConnectionAndRead()
295312
default:
296313
break
297314
}

AmplifyPlugins/Core/AWSPluginsCoreTests/WebSocket/WebSocketClientTests.swift

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,168 @@ class WebSocketClientTests: XCTestCase {
139139
await fulfillment(of: [reconnectExpectation], timeout: timeout)
140140
}
141141

142+
/// Regression test for https://github.com/aws-amplify/amplify-swift/issues/3976.
143+
/// When iOS recycles the TCP route during a scenePhase transition,
144+
/// NWPathMonitor reports .satisfied both before and after, producing
145+
/// (.online, .online) through AmplifyNetworkMonitor's scan. Before the
146+
/// fix, WebSocketClient.onNetworkStateChange hit `default: break` and
147+
/// left the stale URLSessionWebSocketTask in place — a zombie.
148+
///
149+
/// - Given:
150+
/// - A WebSocketClient connected via a MockNetworkMonitor whose scan
151+
/// seed is (.online, .online), so one updateState(.online) produces
152+
/// the bug tuple deterministically.
153+
/// - autoConnectOnNetworkStatusChange is true.
154+
/// - When:
155+
/// - The mock emits .online, producing an (.online, .online) tuple.
156+
/// - Then:
157+
/// - WebSocketClient sends a `.disconnected` event (stale task torn down).
158+
/// - WebSocketClient emits a fresh `.connected` event (new connection).
159+
func testWebSocketClient_whenNetworkPathChangesWhileOnline_shouldRecycleConnection() async throws {
160+
var cancellables = Set<AnyCancellable>()
161+
guard let endpoint = try localWebSocketServer?.start() else {
162+
XCTFail("Local WebSocket server failed to start")
163+
return
164+
}
165+
166+
let mockNetworkMonitor = MockNetworkMonitor()
167+
let webSocketClient = WebSocketClient(url: endpoint, networkMonitor: mockNetworkMonitor)
168+
await verifyConnected(webSocketClient, autoConnectOnNetworkStatusChange: true)
169+
170+
let disconnectExpectation = expectation(description: "Path change should force a disconnect")
171+
let reconnectExpectation = expectation(description: "Path change should trigger a reconnect")
172+
173+
await webSocketClient.publisher.sink { event in
174+
switch event {
175+
case .disconnected:
176+
disconnectExpectation.fulfill()
177+
case .connected:
178+
reconnectExpectation.fulfill()
179+
default:
180+
break
181+
}
182+
}
183+
.store(in: &cancellables)
184+
185+
// Simulate NWPathMonitor firing .satisfied again after a path recycle.
186+
// The scan seed in MockNetworkMonitor is (.online, .online), so sending
187+
// .online produces exactly the (.online, .online) tuple from issue #3976.
188+
await mockNetworkMonitor.updateState(.online)
189+
190+
await fulfillment(
191+
of: [disconnectExpectation, reconnectExpectation],
192+
timeout: timeout,
193+
enforceOrder: true
194+
)
195+
}
196+
197+
/// Integration-level companion to the mock-based test above. Proves the
198+
/// fix works with the real AmplifyNetworkMonitor's scan seed (.none, .none)
199+
/// — i.e., the bug is not an artifact of MockNetworkMonitor's seeding.
200+
/// Drives state through `updateState`, the same seam WebSocketClient
201+
/// itself uses when reporting connectionLost. Tolerates spontaneous
202+
/// NWPathMonitor firings on watchOS by counting `.connected` events
203+
/// instead of using the strict verifyConnected helper.
204+
///
205+
/// - Given:
206+
/// - A WebSocketClient wired to a real AmplifyNetworkMonitor with its
207+
/// natural scan seed (.none, .none).
208+
/// - A publisher sink attached before connect() so no events are lost
209+
/// while the client's internal sink is still attaching.
210+
/// - autoConnectOnNetworkStatusChange is true.
211+
/// - When:
212+
/// - The monitor's updateState(.online) is called twice, producing
213+
/// (.none, .online) then (.online, .online) through the scan.
214+
/// - Then:
215+
/// - A second `.connected` event is observed (initial connect + recycle
216+
/// reconnect), confirming the WebSocket was torn down and rebuilt.
217+
func testWebSocketClient_withRealNetworkMonitor_whenPathChangesWhileOnline_shouldRecycle() async throws {
218+
var cancellables = Set<AnyCancellable>()
219+
guard let endpoint = try localWebSocketServer?.start() else {
220+
XCTFail("Local WebSocket server failed to start")
221+
return
222+
}
223+
224+
let realNetworkMonitor = AmplifyNetworkMonitor()
225+
let webSocketClient = WebSocketClient(url: endpoint, networkMonitor: realNetworkMonitor)
226+
227+
let initialConnect = expectation(description: "Initial WebSocket connect")
228+
let reconnectAfterPathChange = expectation(description: "Reconnect after (.online, .online)")
229+
let connectedCounter = AtomicInt()
230+
231+
await webSocketClient.publisher.sink { event in
232+
if case .connected = event {
233+
let count = connectedCounter.increment()
234+
if count == 1 {
235+
initialConnect.fulfill()
236+
} else if count == 2 {
237+
reconnectAfterPathChange.fulfill()
238+
}
239+
}
240+
// Tolerate .disconnected / .error / .string / .data events,
241+
// which can arrive from NWPathMonitor-driven recycling or from
242+
// LocalWebSocketServer teardown.
243+
}
244+
.store(in: &cancellables)
245+
246+
await webSocketClient.connect(
247+
autoConnectOnNetworkStatusChange: true,
248+
autoRetryOnConnectionFailure: false
249+
)
250+
await fulfillment(of: [initialConnect], timeout: timeout)
251+
252+
// WebSocketClient.init spawns its sink via Task { startNetworkMonitor() };
253+
// by the time initialConnect fulfils, the sink is attached.
254+
// Prime the scan so (previous, next) reaches (.online, .online) on
255+
// the second updateState — first reaches (.none, .online).
256+
await realNetworkMonitor.updateState(.online)
257+
258+
// Second .online emission → scan produces (.online, .online) —
259+
// the exact tuple from issue #3976. With the fix, this triggers a
260+
// recycle that yields a second `.connected`.
261+
await realNetworkMonitor.updateState(.online)
262+
263+
await fulfillment(of: [reconnectAfterPathChange], timeout: timeout)
264+
}
265+
266+
/// Characterizes the input signal that drives issue #3976. Proves that
267+
/// the real AmplifyNetworkMonitor.publisher emits the (.online, .online)
268+
/// tuple when two .online states are sent consecutively — which is what
269+
/// WebSocketClient.onNetworkStateChange receives during a scenePhase-
270+
/// triggered NWPath recycle. Does not exercise the fix; passes both
271+
/// before and after.
272+
///
273+
/// - Given:
274+
/// - A fresh AmplifyNetworkMonitor instance.
275+
/// - A publisher sink that watches for (.online, .online) tuples.
276+
/// - When:
277+
/// - updateState(.online) is called twice consecutively.
278+
/// - Then:
279+
/// - The publisher emits an (.online, .online) tuple via its scan —
280+
/// confirming this is the exact signal WebSocketClient must handle.
281+
func testAmplifyNetworkMonitor_whenOnlineEmittedTwice_publishesOnlineOnlineTuple() async throws {
282+
var cancellables = Set<AnyCancellable>()
283+
let monitor = AmplifyNetworkMonitor()
284+
285+
let expectOnlineOnline = expectation(description: "publisher emits (.online, .online)")
286+
expectOnlineOnline.assertForOverFulfill = false
287+
288+
monitor.publisher.sink { tuple in
289+
if tuple.0 == .online && tuple.1 == .online {
290+
expectOnlineOnline.fulfill()
291+
}
292+
}
293+
.store(in: &cancellables)
294+
295+
// Two consecutive .online emissions must produce an (.online, .online)
296+
// tuple through the scan — the exact input that triggers issue #3976
297+
// in WebSocketClient.onNetworkStateChange.
298+
await monitor.updateState(.online)
299+
await monitor.updateState(.online)
300+
301+
await fulfillment(of: [expectOnlineOnline], timeout: timeout)
302+
}
303+
142304
func testAutoRetry_whenReceiveTransientFailureFromServer() async throws {
143305
var cancellables = Set<AnyCancellable>()
144306
guard let endpoint = try localWebSocketServer?.start() else {
@@ -195,6 +357,17 @@ class WebSocketClientTests: XCTestCase {
195357
}
196358

197359

360+
private final class AtomicInt: @unchecked Sendable {
361+
private var value: Int = 0
362+
private let lock = NSLock()
363+
func increment() -> Int {
364+
lock.lock()
365+
defer { lock.unlock() }
366+
value += 1
367+
return value
368+
}
369+
}
370+
198371
private class MockNetworkMonitor: WebSocketNetworkMonitorProtocol {
199372
typealias State = AmplifyNetworkMonitor.State
200373
let subject = PassthroughSubject<State, Never>()

Package.resolved

Lines changed: 9 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)