fix(pair): let a failed channel open be retried

packages/fxa-settings/src/lib/channels/pairing-channel.test.ts

@@ -70,6 +70,19 @@ describe('PairingChannelClient', () => {
       expect(onConnected).toHaveBeenCalled();
     });
 
+    // A message can land in the same tick the handshake completes, so the
+    // listeners have to be attached by the time `connected` observers run.
+    it('registers channel listeners before dispatching connected', async () => {
+      let listenersAtConnected = 0;
+      client.addEventListener('connected', () => {
+        listenersAtConnected = mockChannel.addEventListener.mock.calls.length;
+      });
+
+      await client.open(SERVER, CHAN, VALID_KEY);
+
+      expect(listenersAtConnected).toBe(3);
+    });
+
     it('throws for missing params', async () => {
       await expect(client.open('', CHAN, VALID_KEY)).rejects.toThrow(
         'Invalid channel server configuration'
@@ -202,7 +215,9 @@ describe('PairingChannelClient', () => {
 
       const onError = jest.fn();
       client.addEventListener('error', onError);
-      await client.open(SERVER, CHAN, VALID_KEY);
+      await expect(client.open(SERVER, CHAN, VALID_KEY)).rejects.toThrow(
+        'connection refused'
+      );
 
       expect(client.isConnected).toBe(false);
       expect(onError).toHaveBeenCalled();
@@ -216,7 +231,7 @@ describe('PairingChannelClient', () => {
       const err = new Error('connection refused');
       PairingChannel.connect.mockRejectedValueOnce(err);
 
-      await client.open(SERVER, CHAN, VALID_KEY);
+      await expect(client.open(SERVER, CHAN, VALID_KEY)).rejects.toThrow(err);
 
       expect(sentryMetrics.captureException).toHaveBeenCalledWith(err);
     });
@@ -233,7 +248,9 @@ describe('PairingChannelClient', () => {
       const onError = jest.fn();
       client.addEventListener('error', onError);
 
-      await client.open(SERVER, CHAN, VALID_KEY);
+      await expect(client.open(SERVER, CHAN, VALID_KEY)).rejects.toThrow(
+        'Connection to remote device closed, please try again'
+      );
 
       expect(client.isConnected).toBe(false);
       const detail = onError.mock.calls[0][0].detail;
@@ -253,11 +270,29 @@ describe('PairingChannelClient', () => {
         new Error('WebSocket unexpectedly closed')
       );
 
-      await client.open(SERVER, CHAN, VALID_KEY);
+      await expect(client.open(SERVER, CHAN, VALID_KEY)).rejects.toThrow(
+        PairingChannelError
+      );
 
       expect(sentryMetrics.captureException).not.toHaveBeenCalled();
     });
 
+    it('allows a retry after a failed open', async () => {
+      const {
+        PairingChannel,
+      } = require('fxa-pairing-channel/dist/FxAccountsPairingChannel.babel.umd.js');
+      PairingChannel.connect.mockRejectedValueOnce(
+        new Error('WebSocket unexpectedly closed')
+      );
+      await expect(client.open(SERVER, CHAN, VALID_KEY)).rejects.toThrow(
+        PairingChannelError
+      );
+
+      await client.open(SERVER, CHAN, VALID_KEY);
+
+      expect(client.isConnected).toBe(true);
+    });
+
     it('rejects concurrent open during in-flight connect', async () => {
       const {
         PairingChannel,

packages/fxa-settings/src/lib/channels/pairing-channel.ts

@@ -252,27 +252,36 @@ export class PairingChannelClient extends EventTarget {
       );
 
       this.channel = channel;
-      this.dispatchEvent(new CustomEvent('connected'));
 
+      // Listeners go on before `connected` so a message that arrives in the
+      // same tick as the handshake completing is not dropped.
       channel.addEventListener('message', this.handleMessage);
       channel.addEventListener('error', this.handleError);
       channel.addEventListener('close', this.handleClose);
+
+      this.dispatchEvent(new CustomEvent('connected'));
     } catch (err) {
       // A dropped socket is the channel server's normal answer for a channel it
       // will not serve, so it carries no signal worth an exception report. The
-      // flow still ends on the failure screen; the console breadcrumb keeps it
-      // visible in the trail of any real error that follows.
-      if (isChannelClosedError(err)) {
+      // console breadcrumb keeps it visible in the trail of any real error
+      // that follows.
+      const closed = isChannelClosedError(err);
+      if (closed) {
         console.warn('Pairing channel closed before it opened', channelId);
-        this.dispatchEvent(
-          new CustomEvent('error', {
-            detail: new PairingChannelError('CONNECTION_CLOSED'),
-          })
-        );
       } else {
         sentryMetrics.captureException(err);
-        this.dispatchEvent(new CustomEvent('error', { detail: err }));
       }
+
+      // Report the closed socket under its own errno so a caller reading the
+      // error sees the same 1006 a mid-flow close produces.
+      const detail = closed
+        ? new PairingChannelError('CONNECTION_CLOSED')
+        : err;
+      this.dispatchEvent(new CustomEvent('error', { detail }));
+
+      // Rejecting is what lets the caller drop this instance; resolving leaves
+      // a client that looks live and blocks every later reopen.
+      throw detail;
     } finally {
       this._opening = false;
     }

packages/fxa-settings/src/models/integrations/pairing-supplicant-integration.test.ts

@@ -429,6 +429,37 @@ describe('PairingSupplicantIntegration', () => {
       expect(integration.state).toBe(SupplicantState.Failed);
       expect(onError).toHaveBeenCalled();
     });
+
+    // The channel server refuses the socket outright for a channel already
+    // consumed, so the Android post-OAuth reload sees the rejection rather
+    // than a close event on a live channel.
+    it('open failure while still Connecting is ignored when completion marker is set', async () => {
+      sessionStorage.setItem(PAIR_COMPLETE_STORAGE_PREFIX + 'c', '1');
+      mockOpen.mockRejectedValueOnce(
+        new Error('Connection to remote device closed, please try again')
+      );
+      const integration = createIntegration();
+      const onError = jest.fn();
+      const onStateChange = jest.fn();
+      integration.onError = onError;
+      integration.onStateChange = onStateChange;
+
+      await integration.openChannel('wss://ch.example.com', 'c', 'k');
+
+      expect(integration.state).toBe(SupplicantState.Connecting);
+      expect(onError).not.toHaveBeenCalled();
+      expect(onStateChange).not.toHaveBeenCalled();
+    });
+
+    it('allows a retry on the same channel after a failed open', async () => {
+      mockOpen.mockRejectedValueOnce(new Error('ws connect failed'));
+      const integration = createIntegration();
+
+      await integration.openChannel('wss://ch.example.com', 'c', 'k');
+      await integration.openChannel('wss://ch.example.com', 'c', 'k');
+
+      expect(mockOpen).toHaveBeenCalledTimes(2);
+    });
   });
 
   describe('isPairing', () => {

packages/fxa-settings/src/models/integrations/pairing-supplicant-integration.ts

@@ -189,7 +189,10 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration {
 
   private setState(state: SupplicantState): void {
     this._state = state;
-    console.info(`Emitting pairing supplicant state change event.`, {id: this._iid, state: this.state});
+    console.info(`Emitting pairing supplicant state change event.`, {
+      id: this._iid,
+      state: this.state,
+    });
     this.onStateChange?.(state);
   }
 
@@ -220,7 +223,7 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration {
     this.onError?.(this._error);
   }
 
-  hasChannel(channelId:string) {
+  hasChannel(channelId: string) {
     return !!this._channel && this._channel.channelId === channelId;
   }
 
@@ -232,9 +235,8 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration {
     channelServerUri: string,
     channelId: string,
     channelKey: string,
-    version:number = 1
+    version: number = 1
   ): Promise<void> {
-
     if (version === 2 && this._channel) {
       if (channelId === this._channel.channelId) {
         console.warn('Pairing channel already open!');
@@ -274,7 +276,12 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration {
     } catch (err: unknown) {
       // Reset _channel so a subsequent openChannel() call can retry
       this._channel = null;
-      this.fail(err);
+      // A consumed channel refusing the socket is the post-OAuth reload, not a
+      // failure. fail() still has to run for the config and already-connected
+      // errors, which reject before open() dispatches any `error` event.
+      if (!this.isPostCompletionReconnect()) {
+        this.fail(err);
+      }
     }
   }
 
@@ -294,8 +301,7 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration {
     // In V2, the the UI flow forces the supplicant to approve first.
     if (this._version === 2) {
       this.setState(SupplicantState.WaitingForAuthority);
-    }
-    else {
+    } else {
       if (this._state === SupplicantState.WaitingForAuthorizations) {
         this.setState(SupplicantState.WaitingForAuthority);
       } else if (this._state === SupplicantState.WaitingForSupplicant) {
@@ -313,11 +319,9 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration {
 
       // Send OAuth request to authority
       if (!this._channel) {
-        throw new Error('Channel no longe exists!')
+        throw new Error('Channel no longer exists!');
       }
-      await this._channel
-        .send('pair:supp:request', oauthParams);
-
+      await this._channel.send('pair:supp:request', oauthParams);
     })().catch((err: unknown) => {
       this.fail(err);
     });
@@ -401,7 +405,7 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration {
     );
   }
 
-  checkClientInfo () {
+  checkClientInfo() {
     // no-op. The supplicant doesn't have client info.
   }
 
@@ -425,7 +429,7 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration {
     }
 
     if (!clientId) {
-      console.warn("Could not resolve a valid clientId!")
+      console.warn('Could not resolve a valid clientId!');
     }
 
     return clientId;
@@ -443,8 +447,7 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration {
   };
 
   private handleChannelError = (event: Event) => {
-
-    console.warn('Channel error event', event)
+    console.warn('Channel error event', event);
 
     if (this.isPostCompletionReconnect()) {
       return;
@@ -488,16 +491,16 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration {
     if (this._version === 2) {
       const data = await firefox.pairOauthStart({});
       if (!data) {
-        throw new Error('Firefox could not provide oauth params.')
+        throw new Error('Firefox could not provide oauth params.');
       }
 
       // This following client id check is a bandaide for now, so we at least
       // have a client id... We fail fast here if we can't figure anything
       // out, since there's not point in proceeding.
       const client_id = this.data.clientId || this.getClientId();
       if (!client_id) {
-        console.warn('Could not determine clientId!')
-        throw new Error("Could not determine clientId! Cannot proceed.")
+        console.warn('Could not determine clientId!');
+        throw new Error('Could not determine clientId! Cannot proceed.');
       }
 
       const scope = [
@@ -515,7 +518,9 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration {
       };
 
       // Fail fast if something wasn't provided.
-      const missing = Object.entries(result).filter(([k,v]) => !v && k !== 'client_id').map(([k]) => k);
+      const missing = Object.entries(result)
+        .filter(([k, v]) => !v && k !== 'client_id')
+        .map(([k]) => k);
       if (missing.length) {
         throw new Error(`Missing required OAuth params: ${missing.join(', ')}`);
       }
@@ -572,7 +577,7 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration {
   }
 
   async destroy(): Promise<void> {
-    console.info('Channel destroy')
+    console.info('Channel destroy');
     this.onStateChange = null;
     this.onError = null;