From 8be03861c9cd3b83adf3f1f8461034556d8a9cd3 Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Tue, 19 Nov 2024 19:43:36 +0000 Subject: [PATCH] core/lifecycle: wind down contributors by channel during failure scenarios At the moment, we wind down the contributor itself. This can lead to situations where if the contributor is a shared channel, another contributor will detach the channel and thus it will never recover. This change fixes this issue by making the comparison on the channel objects themselves. Closes #407 --- src/core/room-lifecycle-manager.ts | 2 +- test/core/room-lifecycle-manager.test.ts | 111 +++++++++++++++++++++++ 2 files changed, 112 insertions(+), 1 deletion(-) diff --git a/src/core/room-lifecycle-manager.ts b/src/core/room-lifecycle-manager.ts index c6d518eb..bc145ca8 100644 --- a/src/core/room-lifecycle-manager.ts +++ b/src/core/room-lifecycle-manager.ts @@ -632,7 +632,7 @@ export class RoomLifecycleManager { this._contributors.map(async (contributor) => { // If its the contributor we want to wait for a conclusion on, then we should not detach it // Unless we're in a failed state, in which case we should detach it - if (contributor === except && this._lifecycle.status !== RoomStatus.Failed) { + if (contributor.channel === except?.channel && this._lifecycle.status !== RoomStatus.Failed) { return; } diff --git a/test/core/room-lifecycle-manager.test.ts b/test/core/room-lifecycle-manager.test.ts index 6b83d56b..86e33b29 100644 --- a/test/core/room-lifecycle-manager.test.ts +++ b/test/core/room-lifecycle-manager.test.ts @@ -18,6 +18,9 @@ vi.mock('ably'); interface MockContributor extends ContributesToRoomLifecycle { emulateStateChange: (change: Ably.ChannelStateChange, update?: boolean) => void; + + // We override the channel so that it can be set in some scenarios + channel: Ably.RealtimeChannel; } const baseError = new Ably.ErrorInfo('error', 500, 50000); @@ -1748,6 +1751,114 @@ describe('room lifecycle manager', () => { ]); }); + it('does not wind down contributors with shared channel', async (context) => { + // Force our status and contributors into attached + const status = new DefaultRoomLifecycle('roomId', makeTestLogger()); + context.firstContributor.emulateStateChange({ + current: AblyChannelState.Attached, + previous: 'initialized', + resumed: false, + reason: baseError, + }); + context.secondContributor.emulateStateChange({ + current: AblyChannelState.Attached, + previous: 'initialized', + resumed: false, + reason: baseError2, + }); + context.thirdContributor.emulateStateChange({ + current: AblyChannelState.Attached, + previous: 'initialized', + resumed: false, + reason: baseError, + }); + // We'll give the second contributor the same channel as the third + context.secondContributor.channel = context.thirdContributor.channel; + status.setStatus({ status: RoomStatus.Attached }); + + // Mock channel detachment results + mockChannelDetachSuccess(context.firstContributor.channel); + mockChannelDetachSuccess(context.secondContributor.channel); + mockChannelDetachSuccess(context.thirdContributor.channel); + + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const monitor = new RoomLifecycleManager( + status, + [context.firstContributor, context.secondContributor, context.thirdContributor], + makeTestLogger(), + 5, + ); + + // Mock channel re-attachment results + mockChannelAttachSuccess(context.firstContributor.channel); + mockChannelAttachFailureThenSuccess(context.secondContributor.channel, AblyChannelState.Suspended, 1001); + + // Observations + let feature2AttachError: Ably.ErrorInfo | undefined; + const observedStatuses: RoomStatus[] = []; + status.onChange((newStatus) => { + if (newStatus.current === RoomStatus.Suspended) { + feature2AttachError = newStatus.error; + } + + observedStatuses.push(newStatus.current); + }); + + // Transition the first contributor to detached + context.firstContributor.emulateStateChange({ + current: AblyChannelState.Suspended, + previous: 'attached', + resumed: false, + reason: baseError, + }); + + // Check that the status is as expected + await waitForRoomLifecycleStatus(status, RoomStatus.Suspended); + + // Now transition the first contributor to attached again + context.firstContributor.emulateStateChange({ + current: AblyChannelState.Attached, + previous: 'suspended', + resumed: false, + reason: baseError, + }); + + // We should be in attached state + await waitForRoomLifecycleStatus(status, RoomStatus.Attached); + + // The first feature will have detach called during the second feature failure sequence + // The second and third features will have detach called twice (once by each), as they share a channel. This + // is as a result of the first feature failing (initial wind down). + expect(context.firstContributor.channel.detach).toBeCalledTimes(1); + expect(context.secondContributor.channel.detach).toBeCalledTimes(2); + expect(context.thirdContributor.channel.detach).toHaveBeenCalledTimes(2); + + // Feature 1 will have attach called twice. First time once it recovers from suspended, second when feature + // 2 recovers. + // Features 2 and 3 will have attach called 3 times in total. Once during the re-attach and failure sequence + // when feature 1 recovers, then twice after feature 2 recovers. + expect(context.firstContributor.channel.attach).toHaveBeenCalledTimes(2); + expect(context.secondContributor.channel.attach).toHaveBeenCalledTimes(3); + expect(context.thirdContributor.channel.attach).toHaveBeenCalledTimes(3); + + // We should have seen feature 1's error come through during the attach sequence as they share a channel + expect(feature2AttachError).toBeErrorInfo({ + code: ErrorCodes.PresenceAttachmentFailed, + cause: { + code: 1001, + }, + }); + + // We should have seen a sequence of statuses + expect(observedStatuses).toEqual([ + RoomStatus.Suspended, + RoomStatus.Attaching, + RoomStatus.Suspended, + RoomStatus.Attaching, + RoomStatus.Attached, + ]); + }); + it('recovers from a suspended channel via many retries', async (context) => { // Force our status and contributors into attached const status = new DefaultRoomLifecycle('roomId', makeTestLogger());