CT-2332 | Fix false-success during transport when websockets are turned on

docs/APIDOCUMENTATION.md

@@ -135,6 +135,8 @@
 ##### Notes
 
 > This callback is also used during OAuth flows to synchronize member data when the backend returns a different `inbound_member_guid` than the one used to start the flow (e.g., during non-OAuth to OAuth migrations). When this happens, the widget will fetch the new member record and update its internal state to use the new GUID.
+>
+> `most_recent_job_guid` must change when a new job starts and be `null` before the first one. A `CONNECTED` member whose value is `null`, or unchanged since the widget called `runJob`, keeps the Connecting step waiting.
 
 ##### Responses
 

src/utilities/__tests__/runJobSchedule-test.tsx

@@ -0,0 +1,147 @@
+import { waitFor } from 'src/utilities/testingLibrary'
+import { POST_MESSAGES } from 'src/const/postMessages'
+import { ReadableStatuses } from 'src/const/Statuses'
+import { JOB_TYPES } from 'src/const/consts'
+import { STEPS, VERIFY_MODE } from 'src/const/Connect'
+import { ACTIONABLE_ERROR_CODES } from 'src/views/actionableError/consts'
+import {
+  createFakeBackend,
+  createFakeBrokaw,
+  expectMemberConnected,
+  HttpError,
+  Member,
+  REDIRECT_JOB_GUID,
+  renderConnecting,
+  staleOAuthMember,
+} from 'src/utilities/test/connectingOAuthHarness'
+
+// fadeOut (Velocity) never resolves in jsdom; Connecting's error path dispatches inside its .then.
+vi.mock('src/utilities/Animation', () => ({ fadeOut: vi.fn(() => Promise.resolve()) }))
+
+/**
+ * runJobSchedule$ drives the Connecting step's job schedule. These tests run it through the
+ * real <Connecting /> (real store, hook and transport); only the API and brokaw are faked.
+ *
+ * CT-2332: firefly sets an OAuth member CONNECTED on the redirect before any job exists.
+ * Over websockets, a copy of that member update can reach the widget *after* it has started
+ * its own job. It looks finished (CONNECTED, not aggregating) but names no job, or the job the
+ * member had before. The widget must not mistake it for its job finishing.
+ */
+
+const OUR_JOB_GUID = `JOB-${JOB_TYPES.VERIFICATION}`
+
+// The member as firefly leaves it on the OAuth redirect: CONNECTED, idle, no job yet.
+const connectedWithNoJob: Member = {
+  ...staleOAuthMember,
+  connection_status: ReadableStatuses.CONNECTED,
+}
+
+const runningJob = (jobGuid: string): Member => ({
+  ...connectedWithNoJob,
+  is_being_aggregated: true,
+  most_recent_job_guid: jobGuid,
+})
+
+const finishedJob = (jobGuid: string): Member => ({
+  ...connectedWithNoJob,
+  most_recent_job_guid: jobGuid,
+})
+
+const impededWithNoEligibleAccounts = (jobGuid: string): Member => ({
+  ...staleOAuthMember,
+  connection_status: ReadableStatuses.IMPEDED,
+  most_recent_job_guid: jobGuid,
+  error: { error_code: ACTIONABLE_ERROR_CODES.NO_ELIGIBLE_ACCOUNTS },
+})
+
+const expectNoEligibleAccountsScreen = async (widget: ReturnType<typeof renderConnecting>) => {
+  // Wait for Connecting to route anywhere, then check where. Before the fix it routed to
+  // CONNECTED as soon as the stale update arrived.
+  await waitFor(() => expect(widget.currentStep()).toBeDefined(), { timeout: 4000 })
+  expect(widget.currentStep()).toBe(STEPS.ACTIONABLE_ERROR)
+  expect(widget.onPostMessage).not.toHaveBeenCalledWith(
+    POST_MESSAGES.MEMBER_CONNECTED,
+    expect.anything(),
+  )
+}
+
+describe('runJobSchedule$ through <Connecting /> over websockets', () => {
+  afterEach(() => {
+    vi.restoreAllMocks()
+  })
+
+  it('ignores the late CONNECTED update with no job and shows the real outcome of the job it started', async () => {
+    // Given a first-time OAuth member: no job has ever run on it.
+    const backend = createFakeBackend()
+    const brokaw = createFakeBrokaw()
+    const widget = renderConnecting(
+      backend,
+      { mode: VERIFY_MODE },
+      { webSocket: brokaw.connection },
+    )
+
+    // When the widget starts its verification job...
+    await widget.runJobCalled()
+    // ...and firefly's pre-job update arrives late, looking finished but naming no job...
+    await brokaw.memberUpdated(connectedWithNoJob)
+    // ...then the widget's job actually finishes with no eligible accounts.
+    await brokaw.memberUpdated(impededWithNoEligibleAccounts(OUR_JOB_GUID))
+
+    // Then the widget shows the error, never a success.
+    await expectNoEligibleAccountsScreen(widget)
+  })
+
+  it('ignores the late CONNECTED update that still names a returning member’s previous job', async () => {
+    // Given a returning member whose previous job was also a verification. Attributing that
+    // old job by type would wrongly complete the schedule.
+    const PREVIOUS_JOB_GUID = 'JOB-old'
+    const returningMember = finishedJob(PREVIOUS_JOB_GUID)
+    const backend = createFakeBackend({ member: returningMember })
+    backend.jobs[PREVIOUS_JOB_GUID] = { guid: PREVIOUS_JOB_GUID, job_type: JOB_TYPES.VERIFICATION }
+    const brokaw = createFakeBrokaw()
+    const widget = renderConnecting(
+      backend,
+      { mode: VERIFY_MODE },
+      { webSocket: brokaw.connection, member: returningMember },
+    )
+
+    // When the widget starts a new verification job...
+    await widget.runJobCalled()
+    // ...and firefly's pre-job update arrives late, still naming the previous job...
+    await brokaw.memberUpdated(returningMember)
+    // ...then the new job finishes with no eligible accounts.
+    await brokaw.memberUpdated(impededWithNoEligibleAccounts(OUR_JOB_GUID))
+
+    // Then the widget shows the error, never a success.
+    await expectNoEligibleAccountsScreen(widget)
+  })
+
+  it('observes the job firefly assigned when its own runJob is rejected with a 409', async () => {
+    // Given firefly already started the verification job on the redirect
+    // (disable_background_agg clients), so the widget's own runJob is a duplicate.
+    const backend = createFakeBackend()
+    backend.runJob.mockImplementationOnce(async () => {
+      backend.startJob(REDIRECT_JOB_GUID, JOB_TYPES.VERIFICATION)
+      throw new HttpError(409)
+    })
+    const brokaw = createFakeBrokaw()
+    const widget = renderConnecting(
+      backend,
+      { mode: VERIFY_MODE },
+      { webSocket: brokaw.connection },
+    )
+
+    // When the widget's runJob is rejected...
+    await widget.runJobCalled()
+    // ...the late pre-job update is still ignored...
+    await brokaw.memberUpdated(connectedWithNoJob)
+    // ...and firefly's job is seen running, then finishing.
+    await brokaw.memberUpdated(runningJob(REDIRECT_JOB_GUID))
+    await brokaw.memberUpdated(finishedJob(REDIRECT_JOB_GUID))
+
+    // Then the widget completes against firefly's job without starting another.
+    await expectMemberConnected(widget.onPostMessage)
+    expect(backend.runJob).toHaveBeenCalledTimes(1)
+    expect(backend.loadJob).toHaveBeenCalledWith(REDIRECT_JOB_GUID)
+  })
+})

src/utilities/runJobSchedule.js

@@ -30,6 +30,17 @@ const isSafeConflictError = (error) => error?.response?.status === 409
 const isConnectedWithoutError = (member) =>
   member?.connection_status === ReadableStatuses.CONNECTED && !member?.error?.error_code
 
+const NOT_STARTED_BY_US = { type: null, previousJobGuid: null }
+
+// Firefly sets an OAuth member CONNECTED on the redirect before any job exists, and over
+// websockets that update can arrive after we started ours (CT-2332). It names the job the
+// member had before runJob: none for a first job, the previous job for a returning member.
+const isPreJobUpdate = (member, started) => {
+  const guid = member?.most_recent_job_guid
+
+  return !guid || guid === started.previousJobGuid
+}
+
 /**
  * Work out which job just finished, in order of trust:
  *  - the job we loaded fresh off the polled member
@@ -102,17 +113,26 @@ export const runJobSchedule$ = ({
    * else scheduled after it, otherwise we keep polling until the member is idle
    * so the next job can be started.
    */
-  const observeRunningJob = (memberGuid, currentSchedule, startedType) =>
+  const observeRunningJob = (memberGuid, currentSchedule, started) =>
     pollMember(memberGuid).pipe(
+      // onPoll runs before the gate on purpose: it is where the Connecting timeout lives.
       tap(onPoll),
-      filter((pollingState) => pollingState.pollingIsDone),
+      // Error and MFA states route on the member alone; only CONNECTED needs a real finished job.
+      filter((pollingState) => {
+        const polledMember = pollingState.currentResponse?.member
+
+        return (
+          pollingState.pollingIsDone &&
+          !(isConnectedWithoutError(polledMember) && isPreJobUpdate(polledMember, started))
+        )
+      }),
       take(1),
       map((pollingState) => pollingState.currentResponse),
       mergeMap((polledResponse) =>
         loadJob(polledResponse.member).pipe(
           map((job) => ({
             member: polledResponse.member,
-            job: resolveFinishedJob(job, polledResponse.job, startedType),
+            job: resolveFinishedJob(job, polledResponse.job, started.type),
           })),
         ),
       ),
@@ -135,8 +155,8 @@ export const runJobSchedule$ = ({
       }),
     )
 
-  const observeThenContinue = (memberGuid, currentSchedule, iteration, startedType) =>
-    observeRunningJob(memberGuid, currentSchedule, startedType).pipe(
+  const observeThenContinue = (memberGuid, currentSchedule, iteration, started) =>
+    observeRunningJob(memberGuid, currentSchedule, started).pipe(
       mergeMap(({ member: observedMember, job }) => {
         const emitted = of({ member: observedMember, job })
 
@@ -157,22 +177,27 @@ export const runJobSchedule$ = ({
       }
 
       if (currentMember.is_being_aggregated !== false) {
-        return observeThenContinue(currentMember.guid, currentSchedule, iteration, null)
+        return observeThenContinue(
+          currentMember.guid,
+          currentSchedule,
+          iteration,
+          NOT_STARTED_BY_US,
+        )
       }
 
       const activeJob = JobSchedule.getActiveJob(currentSchedule)
 
       return defer(() => api.runJob(activeJob.type, currentMember.guid, config, true)).pipe(
-        map(() => activeJob.type),
+        map(() => ({ type: activeJob.type, previousJobGuid: currentMember.most_recent_job_guid })),
         catchError((error) => {
           // 409 is usually the job Firefly created on the OAuth redirect.
           // It gets observed and reconciled like any other running job.
-          if (isSafeConflictError(error)) return of(null)
+          if (isSafeConflictError(error)) return of(NOT_STARTED_BY_US)
 
           return throwError(() => error)
         }),
-        mergeMap((startedType) =>
-          observeThenContinue(currentMember.guid, currentSchedule, iteration, startedType),
+        mergeMap((started) =>
+          observeThenContinue(currentMember.guid, currentSchedule, iteration, started),
         ),
       )
     })