fix: normalize error types at api boundary - #211
Draft
duyhungtnn wants to merge 28 commits into
Draft
Conversation
Update DEFAULT_FLUSH_INTERVAL_MILLIS from 10_000 to 30_000 and revise comments to clarify that the minimum flush interval remains 10s while the default is 30s. This ensures the default flush cadence is less frequent than the enforced minimum and makes intent explicit in the code comments.
There was a problem hiding this comment.
Pull request overview
This PR centralizes error normalization at the API boundary so abort/timeout shapes (including Node ABORT_ERR and DOMException AbortError/TimeoutError) are consistently surfaced as BKTBaseError subclasses and correctly classified in metrics and cache processors.
Changes:
- Normalize all API client failures via
toBKTErrorinpostRequestWithRetry, so public API methods only throwBKTBaseErrorsubclasses. - Refactor
toErrorMetricsEventto acceptBKTBaseErrorand dispatch purely viainstanceof, removing string/code heuristics. - Simplify cache processors by removing deadline-detection helpers and relying on the normalized error contract; update/add tests for abort/timeout boundaries.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/pollController.ts | Removes the now-unused isDeadlineExceeded helper after shifting abort/timeout normalization elsewhere. |
| src/objects/metricsEvent.ts | Narrows toErrorMetricsEvent input to BKTBaseError and switches to instanceof-based classification. |
| src/objects/errors.ts | Extends toBKTError to map ABORT_ERR and DOMException abort/timeout names to TimeoutError. |
| src/client.ts | Ensures metrics event conversion uses normalized BKTBaseError at the boundary. |
| src/cache/processor/segmentUsersCacheProcessor.ts | Routes errors through normalization and uses TimeoutError directly (drops deadline helper). |
| src/cache/processor/featureFlagCacheProcessor.ts | Same cache-processor error handling refactor as segment users. |
| src/api/client.ts | Converts the retry wrapper to a single normalization point by catching and throwing toBKTError(...). |
| src/tests/to_bkt_error.ts | Adds unit coverage for ABORT_ERR and DOMException abort/timeout shapes. |
| src/tests/error_to_metrics_event.ts | Updates tests to use BKTBaseError subclasses rather than raw node/DOM errors. |
| src/tests/api_retry.ts | Adds API-boundary tests asserting aborts/timeouts normalize to TimeoutError. |
| src/tests/api_retry_after.ts | Updates retry-after tests to reflect API-boundary normalization for 503s. |
| src/tests/api_failed.ts | Updates integration tests to assert normalized 500s as InternalServerError. |
| src/tests/promise_retriable.ts | Comment-only arrow typography update. |
| src/tests/cache/processor/segementUsersCache/cancellation.ts | Updates cache cancellation mocks to reject with TimeoutError under the new contract. |
| src/tests/cache/processor/featureCache/cancellation.ts | Same cancellation-mock update for feature cache processor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+123
to
+129
| try { | ||
| return await promiseRetriable( | ||
| (s) => this.postRequest<T>(url, chunk, s), | ||
| this.retryPolicy, | ||
| isRetryable, | ||
| signal); | ||
| } catch (e) { |
Comment on lines
6
to
9
| import { APIClient } from '../api/client'; | ||
| import { User } from '../bootstrap'; | ||
| import { InvalidStatusError } from '../objects/errors'; | ||
| import { InvalidStatusError, TimeoutError, UnauthorizedError } from '../objects/errors'; | ||
| import { RetryPolicy } from '../utils/promiseRetriable'; |
Comment on lines
+109
to
+110
| this.pushErrorMetricsEvent(bktError); | ||
| } |
Comment on lines
+104
to
+105
| this.pushErrorMetricsEvent(bktError); | ||
| } |
Comment on lines
+263
to
+265
| if (e instanceof ForbiddenError) { | ||
| logger?.error('An forbidden error occurred. Please check your API Key.'); | ||
| return null; |
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
Abort and timeout errors were misclassified as
UnknownErrorMetricsEvent. WhenAbortSignal.timeout()orAbortController.abort()fires, Node'shttps.requestemitsAbortError { code: 'ABORT_ERR' }— a shape that neithertoErrorMetricsEventnor the cache processors handled.The deeper issue: error conversion was scattered across three places with inconsistent logic.
What changed
Single conversion point —
postRequestWithRetrynow normalizes all errors toBKTBaseErrorviatoBKTErrorbefore throwing. Every public API client method is guaranteed to only throwBKTBaseErrorsubclasses.toBKTError— extended to handleABORT_ERR(Node abort code) and name-based DOMException (AbortError,TimeoutError), both mapping toTimeoutError.toErrorMetricsEvent— signature narrowed from(e: any)to(e: BKTBaseError). Body replaced with pureinstanceofdispatch, removing theisNodeErrorblock and fragilee.namestring checks.Cache processors — removed hand-rolled
isDeadlineExceeded + new TimeoutError()wrapping; now rely on the API client contract and checkinstanceof TimeoutErrordirectly.isDeadlineExceeded— deleted frompollController.ts(no remaining callers).Tests
toBKTErrorcases coveringABORT_ERRand DOMException shapesapi_retry.tsboundary tests: hanging server +AbortSignal.timeout/AbortController.abortboth produceTimeoutErrorerror_to_metrics_event.tsrewritten to useBKTBaseErrorsubclasses as inputsBKTBaseErrorsubclassTimeoutErrorto honour the new contract