diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..c91809d --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,34 @@ +# Changelog + +All notable changes to the `facility360` SDK are documented here. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), +and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +> Entries before 1.2.0 were not tracked in this file; see the git history for details. + +## [1.3.0] — 2026-07-13 +### Fixed +- `getAllBatch` / `getAllAssetsBatch` no longer crash with `Cannot read properties of + undefined (reading 'status')` when a page fetch hits a mid-stream network error + (e.g. `ECONNRESET`). The catch handler previously built an `ApiError` from the + undefined `response`; it now rethrows the real error so the true cause surfaces. +- `ApiError.isAuthorizationError` / `toString` are null-safe against a response-less error. + +### Changed +- **Behavior (affects all `autoRetry` clients — no API change):** the retry policy widened + from 2 retries at a 2ms delay to **6 retries with jittered exponential backoff** + (base 1/2/4/8/16/30s, capped, + up to 25% jitter). A transient network reset now has + time to clear before the retry; jitter de-synchronizes the many concurrent pages of + `getAllBatch` to avoid a thundering-herd re-hammer. Consequence: a request against a + genuinely down endpoint fails in up to ~61s instead of ~6ms. The timeout (`ECONNABORTED`) + one-retry cap is unchanged, and only network/idempotent errors are retried (5xx are still + surfaced, not retried). +- Added a `[facility360] retry N/6 …` log line on each retry so absorbed transient resets + are visible even when `debug` is off. + +## [1.2.0] — 2026-07 +### Added +- Configurable per-request timeouts and `AbortSignal` support on `FamisClient` + (`requestTimeoutMs`, `QueryContext.setTimeout` / `setSignal`). (#108) +- `Filter.contains` with automatic OData value escaping. (#106) diff --git a/errors.ts b/errors.ts index 85c0b2c..65cff7c 100644 --- a/errors.ts +++ b/errors.ts @@ -12,7 +12,10 @@ export class ApiError extends Error { } get isAuthorizationError(): boolean { - return this.resp.status == 401 || this.resp.status === 403; + // Null-safe: a network error (e.g. ECONNRESET) produces an ApiError with no response, + // so `this.resp` may be undefined. Reading `.status` unguarded threw + // "Cannot read properties of undefined (reading 'status')". + return this.resp?.status === 401 || this.resp?.status === 403; } get message(): string { @@ -30,10 +33,10 @@ export class ApiError extends Error { return 'Authorization Failed'; } let str = message; - if (this.resp.request) { - str = `${message} + if (this.resp?.request) { + str = `${message} METHOD: ${this.resp.request.method} -HOST: ${this.resp.config.baseURL} +HOST: ${this.resp.config?.baseURL} URL: ${this.resp.request.path} `; } diff --git a/famis_client.ts b/famis_client.ts index 4f4179b..0c1b175 100644 --- a/famis_client.ts +++ b/famis_client.ts @@ -382,9 +382,19 @@ export class FamisClient { timeout: requestTimeoutMs, }); if (autoRetry) { + const RETRIES = 6; axiosRetry(this.http, { - retries: 2, - retryDelay: () => 2, + retries: RETRIES, + // Exponential backoff WITH JITTER so a transient network reset — e.g. ECONNRESET from a + // shared host / stale keep-alive / sustained load — has time to clear before the retry. + // The previous 2ms delay fired all attempts within ~6ms, so a reset lingering >1s failed + // every attempt and killed the whole paged fetch. Jitter de-synchronizes the many + // concurrent pages of getAllBatch so a shared-host reset doesn't trigger a lockstep + // thundering-herd retry. Budget: base 1/2/4/8/16/30s (capped) + up to +25% jitter. + retryDelay: (retryCount: number) => { + const base = Math.min(1000 * 2 ** (retryCount - 1), 30000); + return base + Math.floor(Math.random() * base * 0.25); + }, // A per-request timeout (ECONNABORTED) fires on a stall; retrying it repeatedly just // multiplies the wait. Allow at most ONE retry on timeout, while leaving the normal // network/idempotent retry behavior (up to `retries`) intact for other failures. @@ -395,6 +405,14 @@ export class FamisClient { } return isNetworkOrIdempotentRequestError(error); }, + // Observability: one concise line per retry so absorbed transient resets are visible in + // logs even when `debug` (full response logging) is off. Proves the retry is doing work. + onRetry: (retryCount: number, error: AxiosError) => { + const tail = (error.config?.url ?? '').split('/').pop(); + console.warn( + `[facility360] retry ${retryCount}/${RETRIES} (${error.code ?? error.response?.status ?? 'ERR'}) ${tail}`, + ); + }, }); } @@ -1899,7 +1917,15 @@ export class FamisClient { callback(resp.data); }) .catch((e: AxiosError) => { - this.throwResponseError(e.response!); + // A network error (e.g. ECONNRESET) has no `response`. Passing `undefined` + // to throwResponseError built an ApiError whose getters then dereferenced + // `this.resp.status` and threw "Cannot read properties of undefined + // (reading 'status')", masking the real cause. Rethrow the real error instead. + if (e.response) { + this.throwResponseError(e.response); + } else { + throw e; + } }); promises.push(req); } diff --git a/package.json b/package.json index 44304a3..9747682 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "facility360", - "version": "1.2.0", + "version": "1.3.0", "description": "A Node based 360Facility client SDK", "main": "dist/index.js", "scripts": { diff --git a/tests/request_timeout.test.ts b/tests/request_timeout.test.ts index 7f33bc8..a80749e 100644 --- a/tests/request_timeout.test.ts +++ b/tests/request_timeout.test.ts @@ -112,18 +112,36 @@ describe('FamisClient request timeouts', () => { expect(attempts).toBe(2); }); - it('still retries a normal network error up to the default retry count', async () => { - const client = makeClient({ autoRetry: true }); - let attempts = 0; - (client.http.defaults as any).adapter = async (config: any) => { - attempts++; - const err: any = new Error('socket hang up'); - err.code = 'ECONNRESET'; // network error (no response) -> retryable - err.config = config; - throw err; - }; - await expect(client.getAllPaged(new QueryContext(), 'users')).rejects.toBeTruthy(); - // Non-timeout network error keeps the default behavior: original + 2 retries = 3 total. - expect(attempts).toBe(3); + it('retries a normal network error up to the raised retry count (original + 6)', async () => { + // Fake timers so the exponential backoff (base 1/2/4/8/16/30s) does not consume real + // wall-clock; we drive the retry chain by flushing microtasks + firing pending timers. + jest.useFakeTimers(); + try { + const client = makeClient({ autoRetry: true }); + let attempts = 0; + (client.http.defaults as any).adapter = async (config: any) => { + attempts++; + const err: any = new Error('socket hang up'); + err.code = 'ECONNRESET'; // network error (no response) -> retryable + err.config = config; + throw err; + }; + const settled = client + .getAllPaged(new QueryContext(), 'users') + .then(() => 'resolved') + .catch(() => 'rejected'); + // Each retry is scheduled via setTimeout(backoff). Flush microtasks so the rejection + // propagates and the next timer is scheduled, then fire it — repeat until exhausted. + for (let i = 0; i < 50; i++) { + await Promise.resolve(); + await Promise.resolve(); + jest.runOnlyPendingTimers(); + } + await expect(settled).resolves.toBe('rejected'); + // retries: 6 -> original request + 6 retries = 7 total. + expect(attempts).toBe(7); + } finally { + jest.useRealTimers(); + } }); });