Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,7 @@ swift test --filter AWSCognitoAuthPluginUnitTests # Specific target
- **Unit tests**: XCTest, defined in Package.swift (19 test targets)
- **Integration tests**: Xcode host app projects under `AmplifyPlugins/<Category>/Tests/<Category>HostApp/`
- **Conventions**: Mock via behavior protocols, use `AmplifyTestCommon` for shared utilities, `AmplifyAsyncTesting` for async helpers
- **Test documentation (MANDATORY)**: Every new or modified test method
**must** have a Given/When/Then doc comment. No exceptions — this applies
to unit tests, integration tests, and regression tests alike. Reviewers
should reject PRs that add tests without this structure.
- **Test documentation**: Use Given/When/Then doc comments on all test methods:
```swift
/// Test description
///
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -251,13 +251,8 @@ actor AppSyncRealTimeClient: AppSyncRealTimeClientProtocol {

private func resumeExistingSubscriptions() {
log.debug("[AppSyncRealTimeClient] Resuming existing subscriptions")
for (id, subscription) in subscriptions {
for (id, _) in subscriptions {
Task { [weak self] in
// Reset local state so subscribe() re-sends .start to the
// server. After a reconnect, the server has no memory of
// prior subscriptions, so stale local .subscribed state must
// not short-circuit the resubscription.
await subscription.prepareForResubscribe()
do {
if let cancellable = try await self?.startSubscription(id) {
await self?.storeInConnectionCancellables(cancellable)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,15 +84,6 @@ actor AppSyncRealTimeSubscription {
state.send(.subscribed)
}

/// Reset local subscription state so `subscribe()` will actually resend
/// the `.start` request after a WebSocket reconnect. The server drops
/// subscription state when the connection dies, so local `.subscribed`
/// state is stale and must not short-circuit the resubscription path.
/// Fix for https://github.com/aws-amplify/amplify-swift/issues/3976
func prepareForResubscribe() {
state.send(.none)
}

func unsubscribe() async throws {
guard state.value == .subscribed else {
log.debug("[AppSyncRealTimeSubscription-\(id)] Subscription should be subscribed to be unsubscribed")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,100 +169,6 @@ class AppSyncRealTimeClientTests: XCTestCase {
withExtendedLifetime(cancellables) { }
}

/// End-to-end regression test for https://github.com/aws-amplify/amplify-swift/issues/3976
/// against a real AppSync backend. Simulates the scenePhase-triggered
/// NWPath recycle by driving two .online states through an injected
/// AmplifyNetworkMonitor and asserts that the subscription is actually
/// re-established (server issues a second start_ack).
///
/// - Given:
/// - An AppSyncRealTimeClient wired to a real AmplifyNetworkMonitor
/// and a real AppSync endpoint from the bundled config.
/// - A live subscription that has already received .subscribed from
/// the server (first start_ack confirmed).
/// - When:
/// - After the WebSocketClient's internal sink has attached (200ms),
/// updateState(.online) is called twice on the network monitor,
/// producing the (.online, .online) tuple from issue #3976.
/// - Then:
/// - The WebSocket is recycled, AppSyncRealTimeClient reconnects,
/// resumeExistingSubscriptions() re-sends `start`, and the server
/// returns a second start_ack — causing a second .subscribed event.
func testSubscribe_afterOnlineToOnlinePathChange_shouldRecycleAndResubscribe() async throws {
var cancellables = Set<AnyCancellable>()

let data = try TestConfigHelper.retrieve(
forResource: GraphQLModelBasedTests.amplifyConfiguration
)
let amplifyConfig = try JSONDecoder().decode(JSONValue.self, from: data)
let (endpoint, apiKey) = (amplifyConfig.api?.plugins?.awsAPIPlugin?.asObject?.values
.map { ($0.endpoint?.stringValue, $0.apiKey?.stringValue) }
.first { $0.0 != nil && $0.1 != nil }
.map { ($0.0!, $0.1!) })!

// Inject a real AmplifyNetworkMonitor we can drive directly.
let networkMonitor = AmplifyNetworkMonitor()

let webSocketClient = WebSocketClient(
url: AppSyncRealTimeClientFactory.appSyncRealTimeEndpoint(URL(string: endpoint)!),
handshakeHttpHeaders: [
URLRequestConstants.Header.webSocketSubprotocols: "graphql-ws",
URLRequestConstants.Header.userAgent: AmplifyAWSServiceConfiguration.userAgentLib + " (intg-test-3976)"
],
interceptor: APIKeyAuthInterceptor(apiKey: apiKey),
networkMonitor: networkMonitor
)
let client = AppSyncRealTimeClient(
endpoint: URL(string: endpoint)!,
requestInterceptor: APIKeyAuthInterceptor(apiKey: apiKey),
webSocketClient: webSocketClient
)
defer { Task { await client.reset() } }

// Wait for WebSocketClient's internal sink to attach to the monitor's
// publisher (it's kicked off via a Task in init). Prime with .online
// AFTER the subscriber is attached so the PassthroughSubject actually
// delivers the event. This gets the scan to (.none, .online) —
// WebSocketClient will ignore it because autoConnect is still false.
try await Task.sleep(nanoseconds: 200_000_000)
await networkMonitor.updateState(.online)

let firstSubscribed = expectation(description: "Initial subscription established")
let resubscribedAfterPathChange = expectation(description: "Subscription re-established after (.online, .online)")
resubscribedAfterPathChange.assertForOverFulfill = false

let id = UUID().uuidString
let subscribedCount = AtomicInt()
let subscription = try await client.subscribe(
id: id,
query: Self.appSyncQuery(with: subscriptionRequest)
).sink { event in
if case .subscribed = event {
let count = subscribedCount.increment()
if count == 1 {
firstSubscribed.fulfill()
} else {
resubscribedAfterPathChange.fulfill()
}
}
}
cancellables.insert(subscription)

try await client.connect()
await fulfillment(of: [firstSubscribed], timeout: 10)

// Simulate the path-recycle: second .online emission produces
// (.online, .online) through the scan — the exact bug tuple.
// In the buggy code, nothing happens; WebSocketClient keeps the
// zombie connection. With the fix, it should tear down and reconnect,
// and AppSyncRealTimeClient.resumeExistingSubscriptions() should
// re-subscribe.
await networkMonitor.updateState(.online)

await fulfillment(of: [resubscribedAfterPathChange], timeout: 15)
withExtendedLifetime(cancellables) { }
}

private func makeOneSubscription(
id: String = UUID().uuidString,
onSubscriptionEvents: ((AppSyncSubscriptionEvent) -> Void)?
Expand Down Expand Up @@ -295,14 +201,3 @@ class AppSyncRealTimeClientTests: XCTestCase {
}

}

private final class AtomicInt: @unchecked Sendable {
private var value: Int = 0
private let lock = NSLock()
func increment() -> Int {
lock.lock()
defer { lock.unlock() }
value += 1
return value
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -292,23 +292,6 @@ extension WebSocketClient {
case (.offline, .online):
log.debug("[WebSocketClient] NetworkMonitor - Device back online")
await createConnectionAndRead()
case (.online, .online):
// NWPathMonitor's pathUpdateHandler only fires on real path
// changes, so a second .satisfied emission while we were already
// online means the underlying path was swapped (e.g., iOS recycled
// the TCP route during a scenePhase transition). The existing
// URLSessionWebSocketTask is now bound to a stale route and any
// further reads/writes will silently fail, leaving the client in
// a zombie state that cached consumers can't recover from.
// Fix for https://github.com/aws-amplify/amplify-swift/issues/3976
guard connection?.state == .running else {
log.debug("[WebSocketClient] NetworkMonitor - Path changed but connection is not running, skipping recycle")
break
}
log.debug("[WebSocketClient] NetworkMonitor - Network path changed while online, recycling connection")
connection?.cancel(with: .invalid, reason: nil)
subject.send(.disconnected(.invalid, nil))
await createConnectionAndRead()
default:
break
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,168 +139,6 @@ class WebSocketClientTests: XCTestCase {
await fulfillment(of: [reconnectExpectation], timeout: timeout)
}

/// Regression test for https://github.com/aws-amplify/amplify-swift/issues/3976.
/// When iOS recycles the TCP route during a scenePhase transition,
/// NWPathMonitor reports .satisfied both before and after, producing
/// (.online, .online) through AmplifyNetworkMonitor's scan. Before the
/// fix, WebSocketClient.onNetworkStateChange hit `default: break` and
/// left the stale URLSessionWebSocketTask in place — a zombie.
///
/// - Given:
/// - A WebSocketClient connected via a MockNetworkMonitor whose scan
/// seed is (.online, .online), so one updateState(.online) produces
/// the bug tuple deterministically.
/// - autoConnectOnNetworkStatusChange is true.
/// - When:
/// - The mock emits .online, producing an (.online, .online) tuple.
/// - Then:
/// - WebSocketClient sends a `.disconnected` event (stale task torn down).
/// - WebSocketClient emits a fresh `.connected` event (new connection).
func testWebSocketClient_whenNetworkPathChangesWhileOnline_shouldRecycleConnection() async throws {
var cancellables = Set<AnyCancellable>()
guard let endpoint = try localWebSocketServer?.start() else {
XCTFail("Local WebSocket server failed to start")
return
}

let mockNetworkMonitor = MockNetworkMonitor()
let webSocketClient = WebSocketClient(url: endpoint, networkMonitor: mockNetworkMonitor)
await verifyConnected(webSocketClient, autoConnectOnNetworkStatusChange: true)

let disconnectExpectation = expectation(description: "Path change should force a disconnect")
let reconnectExpectation = expectation(description: "Path change should trigger a reconnect")

await webSocketClient.publisher.sink { event in
switch event {
case .disconnected:
disconnectExpectation.fulfill()
case .connected:
reconnectExpectation.fulfill()
default:
break
}
}
.store(in: &cancellables)

// Simulate NWPathMonitor firing .satisfied again after a path recycle.
// The scan seed in MockNetworkMonitor is (.online, .online), so sending
// .online produces exactly the (.online, .online) tuple from issue #3976.
await mockNetworkMonitor.updateState(.online)

await fulfillment(
of: [disconnectExpectation, reconnectExpectation],
timeout: timeout,
enforceOrder: true
)
}

/// Integration-level companion to the mock-based test above. Proves the
/// fix works with the real AmplifyNetworkMonitor's scan seed (.none, .none)
/// — i.e., the bug is not an artifact of MockNetworkMonitor's seeding.
/// Drives state through `updateState`, the same seam WebSocketClient
/// itself uses when reporting connectionLost. Tolerates spontaneous
/// NWPathMonitor firings on watchOS by counting `.connected` events
/// instead of using the strict verifyConnected helper.
///
/// - Given:
/// - A WebSocketClient wired to a real AmplifyNetworkMonitor with its
/// natural scan seed (.none, .none).
/// - A publisher sink attached before connect() so no events are lost
/// while the client's internal sink is still attaching.
/// - autoConnectOnNetworkStatusChange is true.
/// - When:
/// - The monitor's updateState(.online) is called twice, producing
/// (.none, .online) then (.online, .online) through the scan.
/// - Then:
/// - A second `.connected` event is observed (initial connect + recycle
/// reconnect), confirming the WebSocket was torn down and rebuilt.
func testWebSocketClient_withRealNetworkMonitor_whenPathChangesWhileOnline_shouldRecycle() async throws {
var cancellables = Set<AnyCancellable>()
guard let endpoint = try localWebSocketServer?.start() else {
XCTFail("Local WebSocket server failed to start")
return
}

let realNetworkMonitor = AmplifyNetworkMonitor()
let webSocketClient = WebSocketClient(url: endpoint, networkMonitor: realNetworkMonitor)

let initialConnect = expectation(description: "Initial WebSocket connect")
let reconnectAfterPathChange = expectation(description: "Reconnect after (.online, .online)")
let connectedCounter = AtomicInt()

await webSocketClient.publisher.sink { event in
if case .connected = event {
let count = connectedCounter.increment()
if count == 1 {
initialConnect.fulfill()
} else if count == 2 {
reconnectAfterPathChange.fulfill()
}
}
// Tolerate .disconnected / .error / .string / .data events,
// which can arrive from NWPathMonitor-driven recycling or from
// LocalWebSocketServer teardown.
}
.store(in: &cancellables)

await webSocketClient.connect(
autoConnectOnNetworkStatusChange: true,
autoRetryOnConnectionFailure: false
)
await fulfillment(of: [initialConnect], timeout: timeout)

// WebSocketClient.init spawns its sink via Task { startNetworkMonitor() };
// by the time initialConnect fulfils, the sink is attached.
// Prime the scan so (previous, next) reaches (.online, .online) on
// the second updateState — first reaches (.none, .online).
await realNetworkMonitor.updateState(.online)

// Second .online emission → scan produces (.online, .online) —
// the exact tuple from issue #3976. With the fix, this triggers a
// recycle that yields a second `.connected`.
await realNetworkMonitor.updateState(.online)

await fulfillment(of: [reconnectAfterPathChange], timeout: timeout)
}

/// Characterizes the input signal that drives issue #3976. Proves that
/// the real AmplifyNetworkMonitor.publisher emits the (.online, .online)
/// tuple when two .online states are sent consecutively — which is what
/// WebSocketClient.onNetworkStateChange receives during a scenePhase-
/// triggered NWPath recycle. Does not exercise the fix; passes both
/// before and after.
///
/// - Given:
/// - A fresh AmplifyNetworkMonitor instance.
/// - A publisher sink that watches for (.online, .online) tuples.
/// - When:
/// - updateState(.online) is called twice consecutively.
/// - Then:
/// - The publisher emits an (.online, .online) tuple via its scan —
/// confirming this is the exact signal WebSocketClient must handle.
func testAmplifyNetworkMonitor_whenOnlineEmittedTwice_publishesOnlineOnlineTuple() async throws {
var cancellables = Set<AnyCancellable>()
let monitor = AmplifyNetworkMonitor()

let expectOnlineOnline = expectation(description: "publisher emits (.online, .online)")
expectOnlineOnline.assertForOverFulfill = false

monitor.publisher.sink { tuple in
if tuple.0 == .online && tuple.1 == .online {
expectOnlineOnline.fulfill()
}
}
.store(in: &cancellables)

// Two consecutive .online emissions must produce an (.online, .online)
// tuple through the scan — the exact input that triggers issue #3976
// in WebSocketClient.onNetworkStateChange.
await monitor.updateState(.online)
await monitor.updateState(.online)

await fulfillment(of: [expectOnlineOnline], timeout: timeout)
}

func testAutoRetry_whenReceiveTransientFailureFromServer() async throws {
var cancellables = Set<AnyCancellable>()
guard let endpoint = try localWebSocketServer?.start() else {
Expand Down Expand Up @@ -357,17 +195,6 @@ class WebSocketClientTests: XCTestCase {
}


private final class AtomicInt: @unchecked Sendable {
private var value: Int = 0
private let lock = NSLock()
func increment() -> Int {
lock.lock()
defer { lock.unlock() }
value += 1
return value
}
}

private class MockNetworkMonitor: WebSocketNetworkMonitorProtocol {
typealias State = AmplifyNetworkMonitor.State
let subject = PassthroughSubject<State, Never>()
Expand Down
Loading
Loading