From 7f2ba8585b1b3cd387c364899779f4259e125c9b Mon Sep 17 00:00:00 2001 From: Logan Rasmussen Date: Tue, 15 Sep 2026 08:47:05 -0600 Subject: [PATCH 1/5] docs: plan for replacing the Connecting job patches with a bounded reconcile loop --- .../CT-2495-connecting-reconcile-loop-plan.md | 130 ++++++++++++++++++ 1 file changed, 130 insertions(+) create mode 100644 docs/plans/CT-2495-connecting-reconcile-loop-plan.md diff --git a/docs/plans/CT-2495-connecting-reconcile-loop-plan.md b/docs/plans/CT-2495-connecting-reconcile-loop-plan.md new file mode 100644 index 0000000000..ccfb5d30ec --- /dev/null +++ b/docs/plans/CT-2495-connecting-reconcile-loop-plan.md @@ -0,0 +1,130 @@ +# CT-2495: Connecting stall — why refactor to a reconcile loop instead of patching + +**Branch:** `lr/CT-2495-connecting-stall` (builds on `ct/CT-2495-connecting-stall`) +**Status:** Proposal / in progress + +## The problem, precisely + +`Connecting` was designed on one assumption: **the widget is the only thing that +starts jobs on a member.** The job schedule is a local plan (`ACTIVE → PENDING → +PENDING`) and the run effect is one-shot per `activeJob`: start it, poll until +done, mark done, move on. + +OAuth breaks that assumption in two ways: + +1. The member in redux is a **pre-redirect snapshot** — stale `is_being_aggregated`, + `most_recent_job_guid: null`. +2. **Firefly starts its own job on the redirect** (when background aggregation is + off), so the server's reality diverges from the local plan. + +Symptoms: `GET /jobs/null` 404 kills the stream; a Firefly job of a different type +gets treated as the scheduled one; two ACTIVE jobs and nothing picks up the second. +Result: the widget sits on "Connecting" forever. + +## What the first branch (`ct/CT-2495-connecting-stall`) does + +- Fixes (1) correctly: refresh the member on mount, guard against a null job guid. +- Patches (2) with three interacting mechanisms bolted onto the one-shot effect: + - `isForeignJob` detection (finished job type ≠ active job type) + - a **second nested `pollMember`** to wait for a foreign job to actually finish + - an `activeJobAttempt` counter added to the effect deps to force a re-run + +Each is reasonable alone. Together they make the hardest effect in the codebase +harder, and they introduce one real defect: **the re-run has no upper bound.** + +### The unbounded-retry defect + +- **409 loop:** re-run → `runJob` 409s → poll → member is CONNECTED+idle so polling + stops immediately → same "foreign" job loaded → schedule unchanged → `attempt++` + → repeat. Hits `runJob` every ~3s forever if the backend keeps 409ing. +- **`/jobs` outage loop:** `loadJob` fails → job is `null` → treated as foreign → + schedule unchanged → `attempt++` → **`runJob` starts a brand-new real job** → + repeat. Previously this stalled; now it spawns jobs. + +## The alternative: a bounded reconcile loop + +Stop distinguishing "our job" from "their job". The server is the source of truth; +the schedule is a checklist that is **reconciled** after every observed completion. + +``` +loop (bounded by schedule.length + 2): + member = refresh() + if member.is_being_aggregated: + member = pollUntilIdle(member) # who started it doesn't matter + job = loadJob(member.most_recent_job_guid) + schedule = reconcile(schedule, job) # mark matching type DONE + continue + if member is CHALLENGED / error: + exit → jobComplete routes to MFA / error step + next = firstNotDone(schedule) + if !next: exit (all done) + try runJob(next.type) + catch 409: continue # someone else started one; observe it +``` + +Why it is better: + +| Concern | Patch approach | Reconcile loop | +| ----------------------------------- | --------------------------- | ----------------------------------------------------------------------- | +| "Foreign job" | Special-cased in two places | Not a concept — every job is reconciled | +| 409 vs. "Firefly job still running" | Two different code paths | Same path: observe, reconcile, continue | +| Termination | `attempt++`, unbounded | Structural: each iteration marks DONE, starts a job, or exits; hard cap | +| Testability | Only via full React render | Pure RxJS function + the existing integration tests | +| Early data release | Second nested poll | One parameter on the poll: "poll to idle if more jobs remain" | + +`JobSchedule.onJobFinished` as rewritten in the first branch is _already_ +reconciliation (mark matching DONE, only promote if nothing ACTIVE). The loop just +removes the branching around it. + +## Why do it now instead of "patch now, refactor later" + +Deploys are expensive. The thing that usually makes "refactor now" risky is the lack +of a safety net, and the first branch already built it: + +- `ConnectingOAuthJobs-test.tsx` — real store, real schedule, real poller, fake + backend, across the exact stall scenarios. +- `Connecting-test.tsx` — pre-existing non-OAuth behavior and postMessage contracts. +- `JobSchedule-test.js` / `Connect-test.js` — reconciliation semantics. + +If the loop passes all of those unchanged **and** a new "perpetual 409 terminates" +test, we ship one deploy with a stronger guarantee than patch + cap would give. + +## Does this need server work? + +**No.** The loop uses the same three endpoints (`loadMemberByGuid`, `loadJob`, +`runJob`) and the same 409 semantics the current code already handles. + +Server-side changes are a _separate, later_ simplification that would let the +client stop guessing entirely (any one of these): + +1. Make `runJob` idempotent — return the running job (200) instead of 409. +2. Include the redirect-created `job_guid` in the OAuth-state response so the widget + can adopt it explicitly. +3. Have Firefly's redirect job honor the widget's configured mode/products. + +(1) is the smallest change with the biggest payoff. If we get it, most of the loop +collapses to "poll, reconcile, run next". + +## Scope of the refactor + +| Keep as-is | Replace | +| -------------------------------------------------------------------------- | ------------------------------------------- | +| Init effect (refresh → use cases → loadJob → `initializeJobSchedule`) | The run effect's body | +| `loadMostRecentJob` helper | `isForeignJob` + nested second `pollMember` | +| `JobSchedule.onJobFinished` | `activeJobAttempt` state + effect dep | +| `handleMemberPoll`, `initialDataReadySentRef`, postMessages/analytics | — | +| `jobComplete` reducer, `ProgressBar` (still one dispatch per observed job) | — | + +New: `src/utilities/runJobSchedule.js` — a pure `runJobSchedule$()` observable that +emits `{ member, job }` per observed completion and completes when the schedule is +satisfied or a terminal member state is reached. `Connecting.js`'s run effect +becomes "subscribe, dispatch `jobComplete`". + +Explicitly **not** touched: the init effect, the reducer, poller message strings, the +timeout postMessage. + +## Approach + +TDD, integration-first: add failing tests to `ConnectingOAuthJobs-test.tsx` for the +cases the patch approach gets wrong (perpetual 409, `/jobs` outage), then implement +the loop until the whole suite is green. From 32dfcfd75af3a96ac215cd3a053d3c4a6e8450de Mon Sep 17 00:00:00 2001 From: Logan Rasmussen Date: Tue, 15 Sep 2026 08:47:06 -0600 Subject: [PATCH 2/5] refactor: run the Connecting job schedule as a bounded reconcile loop Replace the isForeignJob / nested-poll / activeJobAttempt trio in Connecting with runJobSchedule$, a pure observable that treats the backend as the source of truth: observe whatever is running, reconcile it against the schedule, start the next job, repeat. 409s and Firefly's redirect job go through the same path. The loop is capped at jobs.length + 3 iterations and errors with JobScheduleExhaustedError, which Connecting throws to the host error boundary. Previously a perpetual 409 (or an unloadable job) re-ran runJob every poll interval forever. Integration tests added for: a foreign job followed by every scheduled job, early data release still applying when nothing else is scheduled, and bounded termination under a backend that always rejects the job. --- src/utilities/runJobSchedule.js | 189 ++++++++++++++++++ src/views/connecting/Connecting.js | 118 +++-------- .../__tests__/ConnectingOAuthJobs-test.tsx | 110 +++++++++- 3 files changed, 320 insertions(+), 97 deletions(-) create mode 100644 src/utilities/runJobSchedule.js diff --git a/src/utilities/runJobSchedule.js b/src/utilities/runJobSchedule.js new file mode 100644 index 0000000000..9fad4a4bb1 --- /dev/null +++ b/src/utilities/runJobSchedule.js @@ -0,0 +1,189 @@ +import { concat, defer, EMPTY, of, throwError } from 'rxjs' +import { catchError, filter, map, mergeMap, retry, take, tap } from 'rxjs/operators' + +import * as JobSchedule from 'src/utilities/JobSchedule' +import { JOB_STATUSES } from 'src/const/consts' +import { ReadableStatuses } from 'src/const/Statuses' + +/** + * How many extra loop iterations we allow beyond one per scheduled job. Each + * extra iteration is a job we did not start (Firefly's redirect job, a 409 race) + * that we observed and reconciled against the schedule. + */ +export const EXTRA_ITERATIONS_ALLOWED = 3 + +export class JobScheduleExhaustedError extends Error { + constructor(iterations, schedule) { + const remaining = schedule.jobs + .filter((job) => job.status !== JOB_STATUSES.DONE) + .map((job) => job.type) + .join(', ') + + super(`Gave up running the job schedule after ${iterations} attempts; remaining: ${remaining}`) + this.name = 'JobScheduleExhaustedError' + this.schedule = schedule + } +} + +const isSafeConflictError = (error) => error?.response?.status === 409 + +const isConnectedWithoutError = (member) => + member?.connection_status === ReadableStatuses.CONNECTED && !member?.error?.error_code + +/** + * Work out which job just finished, in order of trust: + * - the job we loaded fresh off the polled member + * - the job the poller itself loaded (may lack job_type over websockets) + * - if we started the job ourselves and could not load it, assume it was ours + */ +const resolveFinishedJob = (loadedJob, polledJob, startedType) => { + if (loadedJob?.job_type !== undefined) return loadedJob + if (polledJob?.job_type !== undefined) return polledJob + if (startedType !== null && startedType !== undefined) { + return { ...(loadedJob || polledJob || {}), job_type: startedType } + } + + return loadedJob ?? polledJob ?? null +} + +/** + * Drive a job schedule to completion against the backend, treating the backend + * as the source of truth for what is actually running. + * + * The widget is not the only thing that starts jobs on a member: Firefly starts + * one on the OAuth redirect, and a race can produce a 409 from `runJob`. Rather + * than special-casing "foreign" jobs, every loop iteration does the same thing: + * + * 1. If the member is being aggregated (by anyone), poll until that job is + * observable, then mark its type DONE in the schedule if it matches. + * 2. Otherwise start the schedule's active job. A 409 just means someone beat + * us to it; go back to 1 and observe. + * 3. Stop when every job is DONE, when the member leaves the CONNECTED state + * (MFA / error – the caller routes on it), or when we hit the iteration cap. + * + * Emits `{ member, job }` once per observed job completion so the caller can + * dispatch `jobComplete` and keep the progress UI in sync. Errors with + * `JobScheduleExhaustedError` if the cap is hit, or with whatever `runJob` + * rejected with for non-409 failures. + * + * @param {Object} deps + * @param {Object} deps.api needs runJob and loadJob + * @param {Function} deps.pollMember from usePollMember() + * @param {Object} deps.member the member to run jobs against (fresh) + * @param {Object} deps.schedule an initialized JobSchedule + * @param {Object} deps.config connect config passed through to runJob + * @param {Function} [deps.onPoll] called with every polling state (UI messaging, timeout) + * @return {Observable<{ member: Object, job: Object|null }>} + */ +export const runJobSchedule$ = ({ + api, + pollMember, + member, + schedule, + config, + onPoll = () => {}, +}) => { + const maxIterations = schedule.jobs.length + EXTRA_ITERATIONS_ALLOWED + + const loadJob = (memberToLoad) => { + if (!memberToLoad?.most_recent_job_guid) return of(null) + + return defer(() => api.loadJob(memberToLoad.most_recent_job_guid)).pipe( + // Sometimes this is too fast in sand and it 404s. Long standing backend problem. + retry(1), + catchError(() => of(null)), + ) + } + + /** + * Poll until the member polling logic says the UI may move on, then decide + * whether the *schedule* may move on. Early data release stops polling while + * the job is still running; that is only acceptable when there is nothing + * 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) => + pollMember(memberGuid).pipe( + tap(onPoll), + filter((pollingState) => pollingState.pollingIsDone), + take(1), + map((pollingState) => pollingState.currentResponse), + mergeMap((polledResponse) => + loadJob(polledResponse.member).pipe( + map((job) => ({ + member: polledResponse.member, + job: resolveFinishedJob(job, polledResponse.job, startedType), + })), + ), + ), + mergeMap(({ member: polledMember, job }) => { + const hasMoreWork = !JobSchedule.areAllJobsDone( + JobSchedule.onJobFinished(currentSchedule, job), + ) + const stillRunning = + isConnectedWithoutError(polledMember) && polledMember.is_being_aggregated === true + + if (!hasMoreWork || !stillRunning) return of({ member: polledMember, job }) + + return pollMember(memberGuid).pipe( + tap(onPoll), + map((pollingState) => pollingState.currentResponse?.member), + filter((m) => m?.is_being_aggregated === false), + take(1), + map((idleMember) => ({ member: idleMember, job })), + ) + }), + ) + + /** + * Observe whatever is running, emit its result, and – if the member is still + * healthy – reconcile the schedule and go around again. + */ + const observeThenContinue = (memberGuid, currentSchedule, iteration, startedType) => + observeRunningJob(memberGuid, currentSchedule, startedType).pipe( + mergeMap(({ member: observedMember, job }) => { + const emitted = of({ member: observedMember, job }) + + // MFA, error, denied... the caller routes away from Connecting. + if (!isConnectedWithoutError(observedMember)) return emitted + + const nextSchedule = JobSchedule.onJobFinished(currentSchedule, job) + + return concat(emitted, iterate(observedMember, nextSchedule, iteration + 1)) + }), + ) + + const iterate = (currentMember, currentSchedule, iteration) => + defer(() => { + if (JobSchedule.areAllJobsDone(currentSchedule)) return EMPTY + + if (iteration > maxIterations) { + return throwError(() => new JobScheduleExhaustedError(iteration - 1, currentSchedule)) + } + + // Something is already running (Firefly's job, or one we just started). + if (currentMember.is_being_aggregated !== false) { + return observeThenContinue(currentMember.guid, currentSchedule, iteration, null) + } + + const activeJob = JobSchedule.getActiveJob(currentSchedule) + + return defer(() => api.runJob(activeJob.type, currentMember.guid, config, true)).pipe( + map(() => activeJob.type), + catchError((error) => { + // 409: a job is already running for this member (usually the one + // Firefly created on the OAuth redirect). Observe it like any other. + if (isSafeConflictError(error)) return of(null) + + return throwError(() => error) + }), + // Whether the job started or conflicted, the next step is the same: + // watch the member until whatever is running finishes. + mergeMap((startedType) => + observeThenContinue(currentMember.guid, currentSchedule, iteration, startedType), + ), + ) + }) + + return iterate(member, schedule, 1) +} diff --git a/src/views/connecting/Connecting.js b/src/views/connecting/Connecting.js index 723ab73448..8c3e45cabf 100644 --- a/src/views/connecting/Connecting.js +++ b/src/views/connecting/Connecting.js @@ -1,17 +1,7 @@ import React, { useEffect, useState, useRef, useContext, useMemo } from 'react' import PropTypes from 'prop-types' import { defer, of } from 'rxjs' -import { - filter, - take, - pluck, - tap, - mergeMap, - concatMap, - catchError, - map, - retry, -} from 'rxjs/operators' +import { mergeMap, catchError, map, retry } from 'rxjs/operators' import { useSelector, useDispatch } from 'react-redux' import { Text } from '@mxenabled/mxui' @@ -23,6 +13,7 @@ import { CONNECTING_MESSAGES } from 'src/utilities/pollers' import { STEPS } from 'src/const/Connect' import { ProgressBar } from 'src/views/connecting/progress/ProgressBar' import * as JobSchedule from 'src/utilities/JobSchedule' +import { runJobSchedule$ } from 'src/utilities/runJobSchedule' import { AriaLive } from 'src/components/AriaLive' import { PoweredByFooter } from 'src/components/PoweredByFooter' import useAnalyticsPath from 'src/hooks/useAnalyticsPath' @@ -84,12 +75,10 @@ export const Connecting = (props) => { const [message, setMessage] = useState(CONNECTING_MESSAGES.STARTING) const [timedOut, setTimedOut] = useState(false) const [connectingError, setConnectingError] = useState(null) - const [activeJobAttempt, setActiveJobAttempt] = useState(0) const initialDataReadySentRef = useRef(false) const pollMember = usePollMember() - const activeJob = JobSchedule.getActiveJob(jobSchedule) const needsToInitializeJobSchedule = jobSchedule.isInitialized === false function handleMemberPoll(pollingState) { @@ -248,79 +237,29 @@ export const Connecting = (props) => { }, [needsToInitializeJobSchedule]) /** - * If the member is not aggregating, start a job, otherwise, poll the - * member until it's done aggregating. + * Once the schedule is initialized, run it to completion. runJobSchedule$ + * owns the start-job / poll / reconcile loop (including jobs we did not + * start, 409 conflicts and the iteration cap); this effect only translates + * what it observes into redux and UI transitions. + * + * It deliberately runs once per initialization rather than once per active + * job: the loop tracks the schedule itself and redux is kept in step through + * jobComplete, which applies the same JobSchedule.onJobFinished. */ useEffect(() => { - // If we still need to initialize the job schedule, do nothing - if (needsToInitializeJobSchedule || !activeJob) return () => {} + if (needsToInitializeJobSchedule || !JobSchedule.getActiveJob(jobSchedule)) return () => {} pollingStartedAtRef.current = Date.now() - const connectMember$ = defer(() => { - const needsJobStarted = currentMember.is_being_aggregated === false - - const startJob$ = defer(() => - api.runJob(activeJob?.type, currentMember.guid, connectConfig, true), - ).pipe( - mergeMap(() => api.loadMemberByGuid(currentMember.guid, clientLocale)), - - catchError((error) => { - // A 409 means a job is already running for this member (for OAuth - // members that is usually the job firefly created on the redirect). - // That is fine: we poll the member by guid below and look the finished - // job up on the polled member, so the stale copy we hold is harmless. - const isSafeConflictError = error?.response?.status === 409 - if (isSafeConflictError) { - return of(currentMember) - } - - // Prevent the Connecting component from trying to continue - // when a bad error occurs. - setConnectingError(error) - throw error - }), - ) - - // If the current member is not being aggregated, start a job - // otherwise, just go with the member we have now - return needsJobStarted ? startJob$ : of(currentMember) - }) - .pipe( - concatMap((member) => - pollMember(member.guid).pipe( - tap((pollingState) => handleMemberPoll(pollingState)), - filter((pollingState) => pollingState.pollingIsDone), - pluck('currentResponse'), - take(1), - mergeMap((polledResponse) => - loadMostRecentJob(polledResponse.member).pipe( - map((job) => ({ - member: polledResponse.member, - job: job ?? polledResponse.job ?? null, - })), - ), - ), - mergeMap(({ member, job }) => { - const isForeignJob = job ? job.job_type !== activeJob.type : true - const isStillRunning = - member.connection_status === ReadableStatuses.CONNECTED && - member.is_being_aggregated === true - - if (!isForeignJob || !isStillRunning) return of({ member, job }) - - return pollMember(member.guid).pipe( - tap((pollingState) => handleMemberPoll(pollingState)), - map((pollingState) => pollingState.currentResponse?.member), - filter((polledMember) => polledMember?.is_being_aggregated === false), - take(1), - map((idleMember) => ({ member: idleMember, job })), - ) - }), - ), - ), - ) - .subscribe(({ member, job }) => { + const schedule$ = runJobSchedule$({ + api, + pollMember, + member: currentMember, + schedule: jobSchedule, + config: connectConfig, + onPoll: handleMemberPoll, + }).subscribe({ + next: ({ member, job }) => { if (onUpsertMember) { onUpsertMember(member) } @@ -334,21 +273,18 @@ export const Connecting = (props) => { return } - const isForeignJob = job ? job.job_type !== activeJob.type : true - const memberIsConnected = member.connection_status === ReadableStatuses.CONNECTED - dispatch(jobComplete(member, job, connectConfig.mode)) - - if (isForeignJob && memberIsConnected) { - setActiveJobAttempt((attempt) => attempt + 1) - } - }) + }, + // Non-409 runJob failures and an exhausted schedule both end up here. + // Throwing from render hands off to the host's error boundary. + error: (error) => setConnectingError(error), + }) return () => { pollingStartedAtRef.current = null - connectMember$.unsubscribe() + schedule$.unsubscribe() } - }, [needsToInitializeJobSchedule, activeJob, activeJobAttempt]) + }, [needsToInitializeJobSchedule]) /** * We removed the timeout step, but customer's relied on the timeout value in diff --git a/src/views/connecting/__tests__/ConnectingOAuthJobs-test.tsx b/src/views/connecting/__tests__/ConnectingOAuthJobs-test.tsx index e7d38b9a05..5ddd00f697 100644 --- a/src/views/connecting/__tests__/ConnectingOAuthJobs-test.tsx +++ b/src/views/connecting/__tests__/ConnectingOAuthJobs-test.tsx @@ -7,6 +7,7 @@ import { POST_MESSAGES } from 'src/const/postMessages' import { ReadableStatuses } from 'src/const/Statuses' import { JOB_TYPES } from 'src/const/consts' import { VERIFY_MODE } from 'src/const/Connect' +import { EXTRA_ITERATIONS_ALLOWED } from 'src/utilities/runJobSchedule' /** * CT-2495: after OAuth the widget lands on Connecting holding the member it @@ -126,11 +127,35 @@ const createFakeBackend = ({ pollsUntilDone = 2, earlyDataRelease = false } = {} return backend } +/** + * Connecting throws `connectingError` during render so the host's error + * boundary can take over. Tests need a boundary of their own to observe that. + */ +class TestErrorBoundary extends React.Component< + { onError: (error: Error) => void; children: React.ReactNode }, + { hasError: boolean } +> { + state = { hasError: false } + + componentDidCatch(error: Error) { + this.props.onError(error) + } + + static getDerivedStateFromError() { + return { hasError: true } + } + + render() { + return this.state.hasError ?
: this.props.children + } +} + const renderConnecting = ( backend: ReturnType, connectConfig: Record, ) => { const onPostMessage = vi.fn() + const onError = vi.fn() const api = { loadMemberByGuid: backend.loadMemberByGuid, loadJob: backend.loadJob, @@ -138,15 +163,17 @@ const renderConnecting = ( } as unknown as ApiContextTypes render( - - - - - , + + + + + + + , { store: createStore() }, ) - return { onPostMessage } + return { onPostMessage, onError } } const expectMemberConnected = (onPostMessage: ReturnType) => @@ -246,6 +273,77 @@ describe(' after OAuth', () => { ).toHaveLength(1) }) + it('runs every scheduled job after a foreign job that was already running', async () => { + const backend = createFakeBackend() + // Firefly kicked off a plain aggregation on the redirect; the widget wants + // verification + identity. All three must run, in order, exactly once. + backend.startJob(REDIRECT_JOB_GUID, JOB_TYPES.AGGREGATION) + + const { onPostMessage } = renderConnecting(backend, { + mode: VERIFY_MODE, + include_identity: true, + }) + + await expectMemberConnected(onPostMessage) + + expect(backend.runJob.mock.calls.map((call) => call[0])).toEqual([ + JOB_TYPES.VERIFICATION, + JOB_TYPES.IDENTIFICATION, + ]) + }) + + it('still releases the user early when the running job satisfies the schedule', async () => { + // Early data release is a product feature: once the job reports its data is + // ready we hand off before aggregation finishes. Waiting to idle must only + // happen when there is more scheduled work to do. + const backend = createFakeBackend({ pollsUntilDone: 1000, earlyDataRelease: true }) + backend.startJob(REDIRECT_JOB_GUID, JOB_TYPES.VERIFICATION) + + const { onPostMessage } = renderConnecting(backend, { mode: VERIFY_MODE }) + + await expectMemberConnected(onPostMessage) + + expect(backend.member.is_being_aggregated).toBe(true) + expect(backend.runJob).not.toHaveBeenCalled() + }) + + it('gives up with an error instead of retrying forever when the backend keeps rejecting the job', async () => { + const backend = createFakeBackend() + // Contradictory backend: the member says it is idle, with a finished foreign + // job, but every attempt to start our job is rejected as a conflict. + backend.jobs[REDIRECT_JOB_GUID] = { guid: REDIRECT_JOB_GUID, job_type: JOB_TYPES.AGGREGATION } + backend.member = { + ...staleOAuthMember, + connection_status: ReadableStatuses.CONNECTED, + is_being_aggregated: false, + most_recent_job_guid: REDIRECT_JOB_GUID, + } + backend.runJob.mockImplementation(async () => { + throw new HttpError(409) + }) + + // React logs caught errors loudly; the throw is the behavior under test. + vi.spyOn(console, 'error').mockImplementation(() => {}) + + const { onPostMessage, onError } = renderConnecting(backend, { mode: VERIFY_MODE }) + + await waitFor(() => expect(onError).toHaveBeenCalled(), { timeout: 5000 }) + expect(onError.mock.calls[0][0].name).toBe('JobScheduleExhaustedError') + + // One attempt per scheduled job plus a small allowance for jobs we did not + // start. With a 10ms poll interval the old code made hundreds of calls here. + const scheduledJobs = 1 + expect(backend.runJob).toHaveBeenCalledTimes(scheduledJobs + EXTRA_ITERATIONS_ALLOWED) + expect(onPostMessage).not.toHaveBeenCalledWith( + POST_MESSAGES.MEMBER_CONNECTED, + expect.anything(), + ) + + const callsAtError = backend.runJob.mock.calls.length + await new Promise((resolve) => setTimeout(resolve, 200)) + expect(backend.runJob.mock.calls.length).toBe(callsAtError) + }) + it('still finishes when the completed job cannot be loaded', async () => { const backend = createFakeBackend() backend.startJob(REDIRECT_JOB_GUID, JOB_TYPES.VERIFICATION) From 69f113d44708a7225dd2c703228b43b06cb00fb4 Mon Sep 17 00:00:00 2001 From: Logan Rasmussen Date: Tue, 15 Sep 2026 08:47:59 -0600 Subject: [PATCH 3/5] docs: CT-2495 reconcile loop summary, risks and next steps --- ...-2495-connecting-reconcile-loop-summary.md | 140 ++++++++++++++++++ 1 file changed, 140 insertions(+) create mode 100644 docs/plans/CT-2495-connecting-reconcile-loop-summary.md diff --git a/docs/plans/CT-2495-connecting-reconcile-loop-summary.md b/docs/plans/CT-2495-connecting-reconcile-loop-summary.md new file mode 100644 index 0000000000..57ce4337fd --- /dev/null +++ b/docs/plans/CT-2495-connecting-reconcile-loop-summary.md @@ -0,0 +1,140 @@ +# CT-2495: Connecting reconcile loop — what changed, risks, next steps + +**Branch:** `lr/CT-2495-connecting-stall` (2 commits on top of `ct/CT-2495-connecting-stall`) +**Companion doc:** [`CT-2495-connecting-reconcile-loop-plan.md`](./CT-2495-connecting-reconcile-loop-plan.md) — the _why_ +**Test status:** full suite green — 125 files / 917 tests + +## TL;DR + +The original branch fixed the OAuth stall but did it by bolting three mechanisms +onto a one-shot effect, and the combination could retry `runJob` forever. This +branch keeps every behavioral fix from the original, replaces those three +mechanisms with one bounded loop in a pure module, and adds an integration test +that proves the loop terminates under a backend that never cooperates. + +No server work required. + +## Net changes + +| File | Change | +| ------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `src/utilities/runJobSchedule.js` | **New.** `runJobSchedule$()` — a pure RxJS observable that drives a job schedule to completion. Observe whatever is running → reconcile it against the schedule → start the next job → repeat. Caps at `jobs.length + 3` iterations and errors with `JobScheduleExhaustedError`. | +| `src/views/connecting/Connecting.js` | Run effect shrinks from ~100 lines of nested `mergeMap`s to "subscribe to `runJobSchedule$`, dispatch `jobComplete`, or set `connectingError`". Deleted: `isForeignJob` (two copies), the nested second `pollMember`, the `activeJobAttempt` state + effect dep, and the now-unused `loadMemberByGuid` after `runJob`. Effect now runs once per schedule init, not once per active job. | +| `src/views/connecting/__tests__/ConnectingOAuthJobs-test.tsx` | +3 integration tests (real store, real schedule, real poller, fake backend) and a small error boundary so the hard-error path is observable. | +| `docs/plans/…-plan.md`, `…-summary.md` | This documentation. | + +**Unchanged from the original branch** (still in place, still tested): the +member refresh on mount, the use-case sync, the `most_recent_job_guid: null` +guard, `JobSchedule.onJobFinished` as reconciliation, the reducer change, the +once-only `connect/initialDataReady`, and every postMessage/analytics contract. + +## How it was built (TDD) + +1. Wrote three integration tests against the _original_ branch: + - foreign job already running → every scheduled job still runs, in order, once + - early data release still hands off early when nothing else is scheduled + - **backend always 409s → give up with an error instead of looping** +2. Ran them: the first two passed (characterization), the third **timed out at 5s** + — the original code never terminates. +3. Wrote `runJobSchedule$`, rewired `Connecting.js`, ran the suite: all green, + including the 5 original OAuth scenarios and the 13 pre-existing Connecting + tests, without modifying any of them. + +The termination test asserts an exact call count (`1 + EXTRA_ITERATIONS_ALLOWED`) +and that no further `runJob` calls happen after the error surfaces. + +## Behavior that is _better_ than the original branch + +- **Bounded.** Perpetual 409 → 4 `runJob` calls then `JobScheduleExhaustedError` + thrown to the host error boundary. Original: one call per poll interval, forever. +- **Job we started but can't load afterward is still credited.** If `runJob` + succeeded for type X and the follow-up `loadJob` fails, the loop marks X done + instead of treating it as foreign and starting X again. Original branch would + re-run it (and under a `/jobs` outage, keep re-running it). +- **One fewer request per job.** The `loadMemberByGuid` right after `runJob` only + fed a guid we already had; polling loads the member anyway. +- **Testable without React.** The loop is a function of `api`, `pollMember`, + `member`, `schedule`, `config`. We chose integration tests for this PR, but unit + tests are now possible if anyone wants them. + +## Risks to weigh + +### 🟠 Cap exhaustion is a hard error + +When the loop gives up, `Connecting` throws and the host error boundary takes over. +That is the _honest_ outcome (we could not run the jobs the customer configured), +and it is what non-409 `runJob` failures already did. But it is a new way to reach +the error screen. Alternatives considered and rejected: + +- _Pretend done and send `memberConnected`_ — lies to the consumer about which + products ran. +- _Keep retrying with backoff_ — exactly the unbounded behavior we are removing. + +If product prefers a softer landing, the place to change is the `error:` handler +in `Connecting.js`; the loop itself does not need to change. + +### 🟠 The loop and redux each hold a copy of the schedule + +`runJobSchedule$` tracks its own schedule to decide what to run next; redux's copy +(driving `ProgressBar`) is updated through `jobComplete`. Both apply the same +`JobSchedule.onJobFinished` to the same `(member, job)`, and the loop only +continues when the reducer would also continue (CONNECTED, no error code), so they +cannot drift in practice. Worth knowing when reading the code. + +### 🟡 Effect deps changed from `[init, activeJob, attempt]` to `[init]` + +Deliberate: the loop owns progression now. If someone later adds a feature that +mutates `jobSchedule` in redux from _outside_ Connecting while it is mounted, the +running loop will not see it. There is no such code path today. + +### 🟡 Websocket transport still yields jobs without `job_type` + +`MemberUpdateTransport` synthesizes `{ guid, async_account_data_ready }` from +socket events. `resolveFinishedJob` handles this (prefers the freshly loaded job, +then the started type), but a websocket-mode integration test does not exist. +Pre-existing gap, not introduced here. + +### 🟡 `/jobs` outage during polling still stalls (pre-existing) + +The polling transport treats a failed `loadJob` as a failed poll, so under a full +`/jobs` outage `pollingIsDone` never becomes true. This branch neither fixes nor +worsens it; the original branch had the same limit. + +### ✅ Reviewed and considered fine + +- `handleMemberPoll` still receives every polling state (via `onPoll`), so the + 60s timeout postMessage and `memberStatusUpdate` behave as before — the existing + fake-timer tests cover this. +- `onUpsertMember` still fires once per observed job with the polled member. +- Error-status fade-out before `jobComplete` is preserved. + +## Suggested review path + +1. `docs/plans/CT-2495-connecting-reconcile-loop-plan.md` — 5 minute read on why. +2. `src/utilities/runJobSchedule.js` — read `iterate` → `observeThenContinue` → + `observeRunningJob` top to bottom; it mirrors the pseudocode in the plan. +3. `Connecting.js` diff — mostly deletion. +4. `ConnectingOAuthJobs-test.tsx` — the fake backend at the top is the mental + model; each test is a scenario against it. + +## Next steps + +**Before merge** + +- [ ] Product/UX sign-off on "exhausted schedule → error screen" (see first risk). +- [ ] Decide whether `EXTRA_ITERATIONS_ALLOWED = 3` is the right allowance. It + means "up to three jobs we did not start" per Connecting session. + +**Soon after** + +- [ ] Add a websocket-mode scenario to `ConnectingOAuthJobs-test.tsx`. +- [ ] Harden `MemberUpdateTransport` so a failed `loadJob` degrades to + `job: undefined` instead of failing the poll (fixes the `/jobs` outage stall). + +**Server conversation (removes the guessing entirely)** + +- [ ] Ask Firefly for **idempotent `runJob`**: return the running job (200) when one + of the requested type already exists, instead of 409. With that in place the + loop collapses to "poll, reconcile, run next" and the 409 branch disappears. +- [ ] Fallback options: include the redirect-created `job_guid` in the OAuth-state + response, or have the redirect job honor the widget's configured mode/products. From d787bac1efbdc7c6afc6581b6053ddb84ec15ea0 Mon Sep 17 00:00:00 2001 From: Logan Rasmussen Date: Tue, 15 Sep 2026 10:22:07 -0600 Subject: [PATCH 4/5] chore: drop planning docs from the repo --- .../CT-2495-connecting-reconcile-loop-plan.md | 130 ---------------- ...-2495-connecting-reconcile-loop-summary.md | 140 ------------------ 2 files changed, 270 deletions(-) delete mode 100644 docs/plans/CT-2495-connecting-reconcile-loop-plan.md delete mode 100644 docs/plans/CT-2495-connecting-reconcile-loop-summary.md diff --git a/docs/plans/CT-2495-connecting-reconcile-loop-plan.md b/docs/plans/CT-2495-connecting-reconcile-loop-plan.md deleted file mode 100644 index ccfb5d30ec..0000000000 --- a/docs/plans/CT-2495-connecting-reconcile-loop-plan.md +++ /dev/null @@ -1,130 +0,0 @@ -# CT-2495: Connecting stall — why refactor to a reconcile loop instead of patching - -**Branch:** `lr/CT-2495-connecting-stall` (builds on `ct/CT-2495-connecting-stall`) -**Status:** Proposal / in progress - -## The problem, precisely - -`Connecting` was designed on one assumption: **the widget is the only thing that -starts jobs on a member.** The job schedule is a local plan (`ACTIVE → PENDING → -PENDING`) and the run effect is one-shot per `activeJob`: start it, poll until -done, mark done, move on. - -OAuth breaks that assumption in two ways: - -1. The member in redux is a **pre-redirect snapshot** — stale `is_being_aggregated`, - `most_recent_job_guid: null`. -2. **Firefly starts its own job on the redirect** (when background aggregation is - off), so the server's reality diverges from the local plan. - -Symptoms: `GET /jobs/null` 404 kills the stream; a Firefly job of a different type -gets treated as the scheduled one; two ACTIVE jobs and nothing picks up the second. -Result: the widget sits on "Connecting" forever. - -## What the first branch (`ct/CT-2495-connecting-stall`) does - -- Fixes (1) correctly: refresh the member on mount, guard against a null job guid. -- Patches (2) with three interacting mechanisms bolted onto the one-shot effect: - - `isForeignJob` detection (finished job type ≠ active job type) - - a **second nested `pollMember`** to wait for a foreign job to actually finish - - an `activeJobAttempt` counter added to the effect deps to force a re-run - -Each is reasonable alone. Together they make the hardest effect in the codebase -harder, and they introduce one real defect: **the re-run has no upper bound.** - -### The unbounded-retry defect - -- **409 loop:** re-run → `runJob` 409s → poll → member is CONNECTED+idle so polling - stops immediately → same "foreign" job loaded → schedule unchanged → `attempt++` - → repeat. Hits `runJob` every ~3s forever if the backend keeps 409ing. -- **`/jobs` outage loop:** `loadJob` fails → job is `null` → treated as foreign → - schedule unchanged → `attempt++` → **`runJob` starts a brand-new real job** → - repeat. Previously this stalled; now it spawns jobs. - -## The alternative: a bounded reconcile loop - -Stop distinguishing "our job" from "their job". The server is the source of truth; -the schedule is a checklist that is **reconciled** after every observed completion. - -``` -loop (bounded by schedule.length + 2): - member = refresh() - if member.is_being_aggregated: - member = pollUntilIdle(member) # who started it doesn't matter - job = loadJob(member.most_recent_job_guid) - schedule = reconcile(schedule, job) # mark matching type DONE - continue - if member is CHALLENGED / error: - exit → jobComplete routes to MFA / error step - next = firstNotDone(schedule) - if !next: exit (all done) - try runJob(next.type) - catch 409: continue # someone else started one; observe it -``` - -Why it is better: - -| Concern | Patch approach | Reconcile loop | -| ----------------------------------- | --------------------------- | ----------------------------------------------------------------------- | -| "Foreign job" | Special-cased in two places | Not a concept — every job is reconciled | -| 409 vs. "Firefly job still running" | Two different code paths | Same path: observe, reconcile, continue | -| Termination | `attempt++`, unbounded | Structural: each iteration marks DONE, starts a job, or exits; hard cap | -| Testability | Only via full React render | Pure RxJS function + the existing integration tests | -| Early data release | Second nested poll | One parameter on the poll: "poll to idle if more jobs remain" | - -`JobSchedule.onJobFinished` as rewritten in the first branch is _already_ -reconciliation (mark matching DONE, only promote if nothing ACTIVE). The loop just -removes the branching around it. - -## Why do it now instead of "patch now, refactor later" - -Deploys are expensive. The thing that usually makes "refactor now" risky is the lack -of a safety net, and the first branch already built it: - -- `ConnectingOAuthJobs-test.tsx` — real store, real schedule, real poller, fake - backend, across the exact stall scenarios. -- `Connecting-test.tsx` — pre-existing non-OAuth behavior and postMessage contracts. -- `JobSchedule-test.js` / `Connect-test.js` — reconciliation semantics. - -If the loop passes all of those unchanged **and** a new "perpetual 409 terminates" -test, we ship one deploy with a stronger guarantee than patch + cap would give. - -## Does this need server work? - -**No.** The loop uses the same three endpoints (`loadMemberByGuid`, `loadJob`, -`runJob`) and the same 409 semantics the current code already handles. - -Server-side changes are a _separate, later_ simplification that would let the -client stop guessing entirely (any one of these): - -1. Make `runJob` idempotent — return the running job (200) instead of 409. -2. Include the redirect-created `job_guid` in the OAuth-state response so the widget - can adopt it explicitly. -3. Have Firefly's redirect job honor the widget's configured mode/products. - -(1) is the smallest change with the biggest payoff. If we get it, most of the loop -collapses to "poll, reconcile, run next". - -## Scope of the refactor - -| Keep as-is | Replace | -| -------------------------------------------------------------------------- | ------------------------------------------- | -| Init effect (refresh → use cases → loadJob → `initializeJobSchedule`) | The run effect's body | -| `loadMostRecentJob` helper | `isForeignJob` + nested second `pollMember` | -| `JobSchedule.onJobFinished` | `activeJobAttempt` state + effect dep | -| `handleMemberPoll`, `initialDataReadySentRef`, postMessages/analytics | — | -| `jobComplete` reducer, `ProgressBar` (still one dispatch per observed job) | — | - -New: `src/utilities/runJobSchedule.js` — a pure `runJobSchedule$()` observable that -emits `{ member, job }` per observed completion and completes when the schedule is -satisfied or a terminal member state is reached. `Connecting.js`'s run effect -becomes "subscribe, dispatch `jobComplete`". - -Explicitly **not** touched: the init effect, the reducer, poller message strings, the -timeout postMessage. - -## Approach - -TDD, integration-first: add failing tests to `ConnectingOAuthJobs-test.tsx` for the -cases the patch approach gets wrong (perpetual 409, `/jobs` outage), then implement -the loop until the whole suite is green. diff --git a/docs/plans/CT-2495-connecting-reconcile-loop-summary.md b/docs/plans/CT-2495-connecting-reconcile-loop-summary.md deleted file mode 100644 index 57ce4337fd..0000000000 --- a/docs/plans/CT-2495-connecting-reconcile-loop-summary.md +++ /dev/null @@ -1,140 +0,0 @@ -# CT-2495: Connecting reconcile loop — what changed, risks, next steps - -**Branch:** `lr/CT-2495-connecting-stall` (2 commits on top of `ct/CT-2495-connecting-stall`) -**Companion doc:** [`CT-2495-connecting-reconcile-loop-plan.md`](./CT-2495-connecting-reconcile-loop-plan.md) — the _why_ -**Test status:** full suite green — 125 files / 917 tests - -## TL;DR - -The original branch fixed the OAuth stall but did it by bolting three mechanisms -onto a one-shot effect, and the combination could retry `runJob` forever. This -branch keeps every behavioral fix from the original, replaces those three -mechanisms with one bounded loop in a pure module, and adds an integration test -that proves the loop terminates under a backend that never cooperates. - -No server work required. - -## Net changes - -| File | Change | -| ------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `src/utilities/runJobSchedule.js` | **New.** `runJobSchedule$()` — a pure RxJS observable that drives a job schedule to completion. Observe whatever is running → reconcile it against the schedule → start the next job → repeat. Caps at `jobs.length + 3` iterations and errors with `JobScheduleExhaustedError`. | -| `src/views/connecting/Connecting.js` | Run effect shrinks from ~100 lines of nested `mergeMap`s to "subscribe to `runJobSchedule$`, dispatch `jobComplete`, or set `connectingError`". Deleted: `isForeignJob` (two copies), the nested second `pollMember`, the `activeJobAttempt` state + effect dep, and the now-unused `loadMemberByGuid` after `runJob`. Effect now runs once per schedule init, not once per active job. | -| `src/views/connecting/__tests__/ConnectingOAuthJobs-test.tsx` | +3 integration tests (real store, real schedule, real poller, fake backend) and a small error boundary so the hard-error path is observable. | -| `docs/plans/…-plan.md`, `…-summary.md` | This documentation. | - -**Unchanged from the original branch** (still in place, still tested): the -member refresh on mount, the use-case sync, the `most_recent_job_guid: null` -guard, `JobSchedule.onJobFinished` as reconciliation, the reducer change, the -once-only `connect/initialDataReady`, and every postMessage/analytics contract. - -## How it was built (TDD) - -1. Wrote three integration tests against the _original_ branch: - - foreign job already running → every scheduled job still runs, in order, once - - early data release still hands off early when nothing else is scheduled - - **backend always 409s → give up with an error instead of looping** -2. Ran them: the first two passed (characterization), the third **timed out at 5s** - — the original code never terminates. -3. Wrote `runJobSchedule$`, rewired `Connecting.js`, ran the suite: all green, - including the 5 original OAuth scenarios and the 13 pre-existing Connecting - tests, without modifying any of them. - -The termination test asserts an exact call count (`1 + EXTRA_ITERATIONS_ALLOWED`) -and that no further `runJob` calls happen after the error surfaces. - -## Behavior that is _better_ than the original branch - -- **Bounded.** Perpetual 409 → 4 `runJob` calls then `JobScheduleExhaustedError` - thrown to the host error boundary. Original: one call per poll interval, forever. -- **Job we started but can't load afterward is still credited.** If `runJob` - succeeded for type X and the follow-up `loadJob` fails, the loop marks X done - instead of treating it as foreign and starting X again. Original branch would - re-run it (and under a `/jobs` outage, keep re-running it). -- **One fewer request per job.** The `loadMemberByGuid` right after `runJob` only - fed a guid we already had; polling loads the member anyway. -- **Testable without React.** The loop is a function of `api`, `pollMember`, - `member`, `schedule`, `config`. We chose integration tests for this PR, but unit - tests are now possible if anyone wants them. - -## Risks to weigh - -### 🟠 Cap exhaustion is a hard error - -When the loop gives up, `Connecting` throws and the host error boundary takes over. -That is the _honest_ outcome (we could not run the jobs the customer configured), -and it is what non-409 `runJob` failures already did. But it is a new way to reach -the error screen. Alternatives considered and rejected: - -- _Pretend done and send `memberConnected`_ — lies to the consumer about which - products ran. -- _Keep retrying with backoff_ — exactly the unbounded behavior we are removing. - -If product prefers a softer landing, the place to change is the `error:` handler -in `Connecting.js`; the loop itself does not need to change. - -### 🟠 The loop and redux each hold a copy of the schedule - -`runJobSchedule$` tracks its own schedule to decide what to run next; redux's copy -(driving `ProgressBar`) is updated through `jobComplete`. Both apply the same -`JobSchedule.onJobFinished` to the same `(member, job)`, and the loop only -continues when the reducer would also continue (CONNECTED, no error code), so they -cannot drift in practice. Worth knowing when reading the code. - -### 🟡 Effect deps changed from `[init, activeJob, attempt]` to `[init]` - -Deliberate: the loop owns progression now. If someone later adds a feature that -mutates `jobSchedule` in redux from _outside_ Connecting while it is mounted, the -running loop will not see it. There is no such code path today. - -### 🟡 Websocket transport still yields jobs without `job_type` - -`MemberUpdateTransport` synthesizes `{ guid, async_account_data_ready }` from -socket events. `resolveFinishedJob` handles this (prefers the freshly loaded job, -then the started type), but a websocket-mode integration test does not exist. -Pre-existing gap, not introduced here. - -### 🟡 `/jobs` outage during polling still stalls (pre-existing) - -The polling transport treats a failed `loadJob` as a failed poll, so under a full -`/jobs` outage `pollingIsDone` never becomes true. This branch neither fixes nor -worsens it; the original branch had the same limit. - -### ✅ Reviewed and considered fine - -- `handleMemberPoll` still receives every polling state (via `onPoll`), so the - 60s timeout postMessage and `memberStatusUpdate` behave as before — the existing - fake-timer tests cover this. -- `onUpsertMember` still fires once per observed job with the polled member. -- Error-status fade-out before `jobComplete` is preserved. - -## Suggested review path - -1. `docs/plans/CT-2495-connecting-reconcile-loop-plan.md` — 5 minute read on why. -2. `src/utilities/runJobSchedule.js` — read `iterate` → `observeThenContinue` → - `observeRunningJob` top to bottom; it mirrors the pseudocode in the plan. -3. `Connecting.js` diff — mostly deletion. -4. `ConnectingOAuthJobs-test.tsx` — the fake backend at the top is the mental - model; each test is a scenario against it. - -## Next steps - -**Before merge** - -- [ ] Product/UX sign-off on "exhausted schedule → error screen" (see first risk). -- [ ] Decide whether `EXTRA_ITERATIONS_ALLOWED = 3` is the right allowance. It - means "up to three jobs we did not start" per Connecting session. - -**Soon after** - -- [ ] Add a websocket-mode scenario to `ConnectingOAuthJobs-test.tsx`. -- [ ] Harden `MemberUpdateTransport` so a failed `loadJob` degrades to - `job: undefined` instead of failing the poll (fixes the `/jobs` outage stall). - -**Server conversation (removes the guessing entirely)** - -- [ ] Ask Firefly for **idempotent `runJob`**: return the running job (200) when one - of the requested type already exists, instead of 409. With that in place the - loop collapses to "poll, reconcile, run next" and the 409 branch disappears. -- [ ] Fallback options: include the redirect-created `job_guid` in the OAuth-state - response, or have the redirect job honor the widget's configured mode/products. From 40dfbdd586e3f8c81006ac52e2aaca453901ce8d Mon Sep 17 00:00:00 2001 From: Logan Rasmussen Date: Tue, 15 Sep 2026 10:22:59 -0600 Subject: [PATCH 5/5] refactor: trim comments that restate the code in the job schedule loop --- src/utilities/runJobSchedule.js | 12 ++---------- src/views/connecting/Connecting.js | 15 +++++---------- .../__tests__/ConnectingOAuthJobs-test.tsx | 5 ++--- 3 files changed, 9 insertions(+), 23 deletions(-) diff --git a/src/utilities/runJobSchedule.js b/src/utilities/runJobSchedule.js index 9fad4a4bb1..9b83d5a385 100644 --- a/src/utilities/runJobSchedule.js +++ b/src/utilities/runJobSchedule.js @@ -135,16 +135,11 @@ export const runJobSchedule$ = ({ }), ) - /** - * Observe whatever is running, emit its result, and – if the member is still - * healthy – reconcile the schedule and go around again. - */ const observeThenContinue = (memberGuid, currentSchedule, iteration, startedType) => observeRunningJob(memberGuid, currentSchedule, startedType).pipe( mergeMap(({ member: observedMember, job }) => { const emitted = of({ member: observedMember, job }) - // MFA, error, denied... the caller routes away from Connecting. if (!isConnectedWithoutError(observedMember)) return emitted const nextSchedule = JobSchedule.onJobFinished(currentSchedule, job) @@ -161,7 +156,6 @@ export const runJobSchedule$ = ({ return throwError(() => new JobScheduleExhaustedError(iteration - 1, currentSchedule)) } - // Something is already running (Firefly's job, or one we just started). if (currentMember.is_being_aggregated !== false) { return observeThenContinue(currentMember.guid, currentSchedule, iteration, null) } @@ -171,14 +165,12 @@ export const runJobSchedule$ = ({ return defer(() => api.runJob(activeJob.type, currentMember.guid, config, true)).pipe( map(() => activeJob.type), catchError((error) => { - // 409: a job is already running for this member (usually the one - // Firefly created on the OAuth redirect). Observe it like any other. + // 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) return throwError(() => error) }), - // Whether the job started or conflicted, the next step is the same: - // watch the member until whatever is running finishes. mergeMap((startedType) => observeThenContinue(currentMember.guid, currentSchedule, iteration, startedType), ), diff --git a/src/views/connecting/Connecting.js b/src/views/connecting/Connecting.js index 8c3e45cabf..8263c7a973 100644 --- a/src/views/connecting/Connecting.js +++ b/src/views/connecting/Connecting.js @@ -237,14 +237,10 @@ export const Connecting = (props) => { }, [needsToInitializeJobSchedule]) /** - * Once the schedule is initialized, run it to completion. runJobSchedule$ - * owns the start-job / poll / reconcile loop (including jobs we did not - * start, 409 conflicts and the iteration cap); this effect only translates - * what it observes into redux and UI transitions. - * - * It deliberately runs once per initialization rather than once per active - * job: the loop tracks the schedule itself and redux is kept in step through - * jobComplete, which applies the same JobSchedule.onJobFinished. + * Runs once per schedule initialization rather than once per active job: + * runJobSchedule$ tracks the schedule itself (including jobs Firefly started + * and 409 races) and redux is kept in step through jobComplete, which applies + * the same JobSchedule.onJobFinished. */ useEffect(() => { if (needsToInitializeJobSchedule || !JobSchedule.getActiveJob(jobSchedule)) return () => {} @@ -275,8 +271,7 @@ export const Connecting = (props) => { dispatch(jobComplete(member, job, connectConfig.mode)) }, - // Non-409 runJob failures and an exhausted schedule both end up here. - // Throwing from render hands off to the host's error boundary. + // Thrown from render below so the host's error boundary takes over. error: (error) => setConnectingError(error), }) diff --git a/src/views/connecting/__tests__/ConnectingOAuthJobs-test.tsx b/src/views/connecting/__tests__/ConnectingOAuthJobs-test.tsx index 5ddd00f697..7589204f9b 100644 --- a/src/views/connecting/__tests__/ConnectingOAuthJobs-test.tsx +++ b/src/views/connecting/__tests__/ConnectingOAuthJobs-test.tsx @@ -309,8 +309,8 @@ describe(' after OAuth', () => { it('gives up with an error instead of retrying forever when the backend keeps rejecting the job', async () => { const backend = createFakeBackend() - // Contradictory backend: the member says it is idle, with a finished foreign - // job, but every attempt to start our job is rejected as a conflict. + // The member reports idle but every runJob is rejected as a conflict, so no + // iteration can ever make progress. backend.jobs[REDIRECT_JOB_GUID] = { guid: REDIRECT_JOB_GUID, job_type: JOB_TYPES.AGGREGATION } backend.member = { ...staleOAuthMember, @@ -322,7 +322,6 @@ describe(' after OAuth', () => { throw new HttpError(409) }) - // React logs caught errors loudly; the throw is the behavior under test. vi.spyOn(console, 'error').mockImplementation(() => {}) const { onPostMessage, onError } = renderConnecting(backend, { mode: VERIFY_MODE })