CT-2332 | Fix false-success during transport when websockets are turned on - #403
Open
codingLogan wants to merge 7 commits into
Open
codingLogan wants to merge 7 commits into
codingLogan wants to merge 7 commits into
Conversation
codingLogan
force-pushed
the
lr/CT-2332-transport-hardening
branch
from
September 24, 2026 22:46
230b029 to
3cc9383
Compare
…he finished job Firefly sets an OAuth member to CONNECTED on the redirect before any job exists. Over websockets that update can arrive after the widget has started its own job; runJobSchedule$ took it as "done" and, unable to load a job, assumed the job it had started finished, showing Success! while the real job ended IMPEDED (CT-2332). The update names the job the member had before runJob was called: null for a first job, the previous job's guid for a returning member. runJobSchedule$ now records that guid when it calls runJob and keeps observing while a CONNECTED idle update still names it. `undefined` is left alone because hosts are not required to send the field. After a 409 the widget started nothing, so only the null case is detectable there. Verified in SAND with websockets on: 50/50 NoDDA runs pass (was ~30% failing). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
codingLogan
force-pushed
the
lr/CT-2332-transport-hardening
branch
from
September 24, 2026 23:51
3cc9383 to
0a582a8
Compare
…fter a 409 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The repo is moving internal and the backend always sends the field (a guid or null), so the gate no longer distinguishes undefined from null. Drops the test that pinned that distinction and the doc sentence that told hosts they could omit the field. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
codingLogan
marked this pull request as ready for review
September 25, 2026 02:32
codingLogan
requested review from
Jameson13B,
ash-wright123,
mwclemy and
wesrisenmay-mx
as code owners
September 25, 2026 02:32
…runJobSchedule unit tests The unit file fed hand-built polling states straight into runJobSchedule$, so it asserted assumptions about what the poller emits rather than what the rendered widget does. Every case now runs through Connecting with the real store, hook and transport, driven by the fake API and a Subject in place of brokaw: the first-time member, the returning member, and now the 409 path where firefly's redirect job is the one observed. All three fail against the previous runner. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
| * CT-2332: firefly sets the member CONNECTED on the OAuth redirect before any job exists, | ||
| * and over websockets that frame can arrive after the widget has started its job. | ||
| */ | ||
| describe('<Connecting /> after OAuth over websockets', () => { |
Collaborator
There was a problem hiding this comment.
These tests feel pretty difficult to comprehend
Collaborator
Author
There was a problem hiding this comment.
Yeah, they are complex because they deal with observables firing based on network events, which are hard to mock correctly. This isn't something that can be really simplified, unfortunately. I did add tests, and uncommented failing e2e tests in the GitLab side to cover more of this though.
Collaborator
Author
There was a problem hiding this comment.
I'll have good ol' Botly see if we can simplify how it reads
…hedule The three websocket scenarios (first-time member, returning member, 409 path) now live in src/utilities/__tests__/runJobSchedule-test.tsx so the coverage sits beside the code it proves. They still render the real <Connecting /> with the real store, hook and transport; only the API and brokaw are faked. The fake backend and render harness they share with ConnectingOAuthJobs-test move to src/utilities/test/connectingOAuthHarness.tsx. ConnectingOAuthJobs-test keeps its original eight tests unchanged and no longer needs the fadeOut mock. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…n/then The Subject plumbing and settle calls moved into the harness behind a fake brokaw (`createFakeBrokaw().memberUpdated(member)`) and a `runJobCalled()` step on the rendered widget. Member fixtures are named for what they represent (connectedWithNoJob, runningJob, finishedJob, impededWithNoEligibleAccounts) and each test narrates its steps. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rovider in the harness The harness no longer hand-builds a redux store or nests its own ApiProvider; it passes preloadedState and apiValue to src/utilities/testingLibrary's render, like the rest of the suite. Exports are trimmed to what the two test files use. Test behavior is unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
| @@ -0,0 +1,147 @@ | |||
| import { waitFor } from 'src/utilities/testingLibrary' | |||
Collaborator
There was a problem hiding this comment.
This feels a lot better. Can we follow our new pattern and put this directly next to the file instead of in a tests folder?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently there is a false-success condition that happens during OAuth, when it really ends in a No DDA scenario
is_being_aggregatedis false. Because it gets this signal, it thinks the job is done and exits successfullyBefore (sometimes)
The end of the OAuth Verification flow with No DDA ends in "Success" - Not true though 😞
After (consistent now)
The end of the Verification flow with No DDA ends in the "No Eligible Accounts screen" - 😄 Correct message for the user
Cause
If the websockets are fast and the systems send the RPC messages in the problematic order, it falls into this problem. It is a race-condition type of behavior. This branch fixes the problem.