fix: null-safe ApiError + jittered retry backoff for getAllBatch ECONNRESET (v1.3.0) - #110
Merged
Merged
Conversation
alphahlee
force-pushed
the
sdk/econnreset-batch-params-local
branch
from
July 13, 2026 18:21
0e24605 to
93fa81e
Compare
alphahlee
force-pushed
the
sdk/econnreset-batch-params-local
branch
from
July 13, 2026 20:51
93fa81e to
6bfc124
Compare
…ered retry backoff) — v1.3.0 getAllBatch / getAllAssetsBatch crashed with "Cannot read properties of undefined (reading 'status')" when a page fetch hit a network error (e.g. ECONNRESET): the catch handler passed `e.response` (undefined for a network error) into throwResponseError, whose ApiError getters then dereferenced `this.resp.status`. - famis_client.ts getAllBatch catch: rethrow the real error when there is no HTTP response, instead of fabricating an ApiError from `undefined`. - errors.ts: null-safe `isAuthorizationError` and `message` getters; surface the underlying cause when there is no response. - axios-retry: retries 2 -> 6 with jittered exponential backoff (base 1/2/4/8/16/30s + up to 25% jitter) so a transient reset has time to clear; jitter de-synchronizes the many concurrent pages of getAllBatch (no thundering-herd re-hammer). The ECONNABORTED (timeout) one-retry cap is unchanged. - onRetry logging (`[facility360] retry N/6 ...`) so absorbed resets are visible in logs even with `debug` off. - bump version to 1.3.0 (behavior change: broader retry policy). Backward compatible: no signature/API change for existing callers; the only shared change is the retry policy (more retries + real backoff), a failure-latency change, not a correctness one. request_timeout retry test updated for the new count (fake timers, no real wall-clock cost). Note: the retry-*absorbs*-a-reset path was not exercised in the 2026-07-10 fleet run (0 resets fired locally; the reset appears specific to the Cloud Run -> FAMIS egress path). Confirm post-deploy via the `[facility360] retry` log lines. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
alphahlee
force-pushed
the
sdk/econnreset-batch-params-local
branch
from
July 13, 2026 21:02
6bfc124 to
aa1b87d
Compare
alexisandreason
approved these changes
Jul 13, 2026
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
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.
Problem
getAllBatch/getAllAssetsBatchcrashed withCannot read properties of undefined (reading 'status')when a page fetch hit a network error (e.g. ECONNRESET): the catch handler passede.response(undefined for a network error) intothrowResponseError, whoseApiErrorgetters then dereferencedthis.resp.status. Observed net-failingudst/udstssoassets on deployed connector nightlies.Changes (single commit, rebased onto master)
famis_client.tsgetAllBatchcatch: rethrow the real error when there is no HTTP response, instead of fabricating anApiErrorfromundefined.errors.tsnull-safeisAuthorizationError/toStringgetters (this.resp?.…) so a response-lessApiErrorcan't throw.retries2 → 6 (via aRETRIESconst referenced by theonRetrylog too) with jittered exponential backoff (base 1/2/4/8/16/30s + up to 25% jitter) so a transient reset has time to clear; jitter de-synchronizes concurrentgetAllBatchpages (no thundering-herd). The ECONNABORTED (timeout) one-retry cap is unchanged.[facility360] retry N/6 …) so absorbed resets are visible even withdebugoff.CHANGELOG.md(Keep a Changelog format) with the 1.3.0 entry + a 1.2.0 backfill.autoRetryconsumer's failure latency) rather than a silent patch.Explicitly NOT included
No
{ pageSize, maxConcurrent }opts and no per-fetchgetAllBatchlog line — those were local-testing scaffolding; the connector never uses the opts and signatures stay identical to master. (Corrects the earlier draft of this description, which mentioned them.)Compatibility
No signature/API change. The only change reaching every
autoRetryconsumer is the retry policy — a failure-latency change on a genuinely down endpoint (~6ms → up to ~61s before giving up), not a correctness change.retryConditionstill limits retries to network/idempotent errors + one retry on timeout (no retry-storm on 4xx). NotevalidateStatus: () => trueis unchanged, so transient FAMIS 5xx are still surfaced viathrowResponseError(not axios-retried) — the retry policy targets network errors/timeouts, which is the ECONNRESET case here. Therequest_timeoutretry test was updated for the new count using fake timers (no real wall-clock cost).Validation
Full local-connector fleet nightly (2026-07-10): 0 assets-loader failures across 1,920 loader runs;
udst/udstsso(241k each) completed. 0 ECONNRESETs fired locally — the reset appears specific to the Cloud Run → FAMIS egress path, so the retry-absorbs-a-reset path should be confirmed post-deploy via the[facility360] retrylog lines. All 94 SDK unit tests pass.🤖 Generated with Claude Code