fix(SDK-1319): harden payroll polling against dropped notifications and inconclusive reads - #2794
mariechatfield wants to merge 15 commits into
Conversation
A proxy rewriting X-Gusto-API-Version can cause the response to be parsed against the wrong schema, silently breaking requests instead of surfacing an error.
usePollingTask now distinguishes a domain-understood terminal outcome
('done', handed to onDone) from a non-retryable read error ('error',
handed to a separate onError) instead of silently retrying every
rejection until the deadline. Each poll consumer (calculation,
submission, print-checks generation) reports the real error to
observability and surfaces the same failure UX a real processing
failure would show, without reusing destructive state resets where the
poll never confirmed whether the underlying operation actually
succeeded.
evaluateSubmissionOutcome fell through to a terminal `{type: 'loaded'}`
whenever a poll run couldn't detect a live submitting->processed
transition -- near-guaranteed for any payroll whose submit-to-success
cycle completes faster than the poll can straddle it. hasSubmittedInSession
then never reset, leaving PayrollOverview stuck on "Submitting payroll..."
even after the server confirmed success.
A run with a baseline only exists because Submit was just clicked, so an
inconclusive read there means the async job may not have started yet --
keep polling instead of giving up, matching the same polling-vs-error
precedent already used for isNonRetryablePollError. The baseline-less
mount-effect safety-net poll is unaffected: it must not treat an
already-settled payroll as a fresh transition.
PayrollOverview's `!payrollData` gate covered a dropped notification (fixed in the prior polling refactor by starting the poll unconditionally at mount) but not the sibling cause of the same symptom: the query itself erroring. usePayrollsGet is non-suspense with retry: false, so an errored read leaves payrollData undefined forever with a clean console. Throw the query's error so the component's existing per-instance error boundary (already used one branch below for payrollNotCalculated) takes over -- scoped to just this mounted component, with a working Try again that resets the query and refetches on the fresh mount that follows.
"We couldn't calculate this payroll" was the only first-person string added on this branch -- nothing else in the SDK's English copy uses "we", closest precedent is Payroll.GrossUpModal's passive "Unable to calculate gross up."
isNonRetryablePollError only matched SDKValidationError, so a 401/403 during polling fell into the retryable branch and spun to the 3-minute deadline before reporting a false failure off the stale pre-failure snapshot -- reproducing the original SDK-1291 root cause (a token expiring shortly after calculate/submit, confirmed via Datadog trace) that this whole usePollingTask refactor was meant to close out. GustoEmbeddedError carries the HTTP status on all its subclasses (APIError as the generic fallback, ForbiddenErrorObject for the specific 403 case), so checking the status there covers both.
b2b46bb to
68aa1ec
Compare
… a poll error evaluateCalculationOutcome's freshness check (isNewCalculation) can produce a false negative on data that is genuinely calculated -- e.g. recalculating an already-calculated payroll where calculatedAt doesn't change tick over tick, the same shape the existing deadline-verification test already covers. Routing a non-retryable read error straight to onError discarded that already-read, merely-misclassified snapshot instead of checking it the way onDeadline already does. Only rescues a positive success; a read that verifies as failed still reports 'error', not 'done', so it doesn't reset hasSeenCalculatingRef the way a confirmed processing failure does. Investigated the same fix for useSubmissionPoll: unreachable there. The Submit button never renders once a payroll is processed, and the API itself rejects a resubmit before the poll's baseline could ever be captured already-terminal, so there's no path to the scenario this rescues against. Left unchanged.
…ly does It only latches onto an already-in-flight submission if the very first read lands on `submitting` -- it's a one-shot render guarantee, not a durable cross-tab watcher.
…the payroll query fails Rethrowing the raw query error handed whatever message the backend/SDK produced straight to the host's error boundary -- unlocalized, and not guaranteed to be end-user-appropriate. Wrap it in a translated Error, matching the sibling `payrollNotCalculated` throw right below it.
The retry button sat bare in a plain Flex column while the sibling Close button used ActionsLayout, so the two rendered at inconsistent widths in the same footer area.
It advanced fake timers straight through two full 190s poll windows (~76 real tick iterations), which was slow enough to occasionally exceed the 5s test timeout on a loaded CI runner. Jumping the mocked clock past the deadline instead means the next scheduled read reports it immediately -- same assertion, without the real iteration count.
| @@ -347,6 +353,9 @@ const Root = ({ | |||
| ) | |||
|
|
|||
| if (!payrollData) { | |||
There was a problem hiding this comment.
quick question on the mount poll here — since it kicks off with baseline: null for any payroll we're just viewing, not only after Submit, if that first read comes back non-retryable (a 401 session expiry, or the schema mismatch we're warning about over in apiVersionMismatchHook), onError fires emitProcessingFailed(undefined) and the partner gets a RUN_PAYROLL_PROCESSING_FAILED for a payroll nobody submitted. is that what we want? feels like it might be the same false-failure thing we're trying to kill, just on the view path — curious how you're thinking about it.
There was a problem hiding this comment.
This branch is what throws a big "idk we couldn't load the payroll" error -- it's not beautiful but there's not a whole lot else we can do if there's nothing we can even display because it didn't load. Good callout that the event we emit doesn't match the UX though... I wonder if I need a new event or if the fact that this will hard-fail the app means we don't strictly need to worry about emitting a dedicated event
| if (!outcome.success) { | ||
| return isNonRetryablePollError(outcome.error) | ||
| ? { status: 'error', error: outcome.error } | ||
| : { status: 'polling' } |
There was a problem hiding this comment.
following on from the mount-poll thing: the success path down around line 88 deliberately lets the baseline-less mount poll off the hook (if (run?.baseline != null) return polling, else loaded), but the error path up here doesn't have that same guard. should the error path get the same baseline check so a mount read error just goes to observability, and only a real post-Submit run surfaces processingFailed?
| // showing, the poll has already reached a terminal state, so the loading UI must clear even | ||
| // though hasSeenCalculatingRef stays true (it still guards prepare separately, below). | ||
| const isCalculatingActive = | ||
| !showProcessingFailedAlert && |
There was a problem hiding this comment.
small one on showProcessingFailedAlert. It only gets reset when someone clicks Calculate again. so if a calc kicks off in another tab after the failure alert is already up, isPolling goes true but isCalculatingActive stays false because !showProcessingFailedAlert wins, and we don't show the loader during that real calculation. Should seeing a fresh calculating status also clear the alert, or is that edge rare enough not to sweat?
| ) | ||
|
|
||
| if (!payrollData) { | ||
| if (isError) { |
There was a problem hiding this comment.
just want to confirm intent here — since the query runs with keepPreviousData, on a sticky error data can still hold the previous page, so !payrollData is false and this throw never actually fires; we'd keep showing stale data with no error surfaced. Is stale-on-error the behavior we're after, or should a hard error break through even when there's placeholder data hanging around?
There was a problem hiding this comment.
I need to double check my react query logic here, but I think payrollData is only ever undefined like this if the query cache has never seen any valid response. So, initial load before the first response, but also if we went straight from initial load -> error responses.
Let me run that past Claude but in theory this error only shows up when we've been trying and we just aren't getting anything we can render
Summary
Payroll polling could get stuck: "Submitting payroll..." or a permanent loading spinner, even after the server had already succeeded. This branch closes out that class of bug for
PayrollConfiguration,PayrollOverview,PrintChecksForm, and their sharedusePollingTaskhook.PayrollOverviewsurfaces a translated error message instead of loading forever when its own query failsX-Gusto-API-Versionresponse header now logs a console warning identifying the likely cause (a proxy rewriting it), instead of failing silently downstreamPrintChecksFailurefound while screenshotting these statesScreenshots
Verified against a real backend (
sdk-app:partner, with temporary response overrides -- see stasheddemoScenarios.ts) that each non-happy-path state shows a real, contained failure instead of hanging or crashing uncontained:X-Gusto-API-Versionheader now logs the new console warning identifying the cause.Test plan
Full suite green. Every fix has a regression test confirmed to fail on the pre-fix code before confirming it passes with the fix.
🤖 Generated with Claude Code