feat(clouddevice): add opt-in retry for transient socket drops - #1792
naseemkullah wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a retry mechanism to HttpURLConnectionClient for handling retryable network errors on idempotent HTTP methods or requests containing an idempotency key, complete with comprehensive unit tests. The feedback suggests registering a no-op error listener during request cleanup to prevent unhandled process crashes, using optional chaining on error.message to avoid potential TypeErrors, and utilizing the built-in getHeader method for a more efficient and readable idempotency key lookup.
d67865d to
34a0dbf
Compare
Add configurable network-level retries to `HttpURLConnectionClient` for transient
connection failures from stale pooled keep-alive sockets, including `socket hang up`,
`ECONNRESET`, `EPIPE`, and `ETIMEDOUT`.
- Add `maxRetries` to `HttpURLConnectionClientOptions`, defaulting to `0` and capped
at `3` to bound latency and prevent unbounded retry loops.
- Retry only before the `ClientRequest` emits its `response` event.
- Retry `GET`, `HEAD`, `OPTIONS`, `PUT`, and `DELETE` requests according to HTTP
idempotency semantics.
- Require a non-empty `Idempotency-Key` for `POST` and `PATCH` retries.
- Preserve request bodies, headers, and idempotency keys across retries and `308`
redirects.
- Abort failed `ClientRequest` instances without destroying the shared HTTPS agent,
avoiding disruption to concurrent requests.
- Re-export `HttpURLConnectionClientOptions` from the package entry point.
- Add coverage for response-boundary safety, idempotency policy, retry exhaustion,
and the retry cap.
34a0dbf to
8ec38e8
Compare
|
Hey @naseemkullah, still having issues with the connection? Which kind of problem are you experiencing? |
Hey @gcatanese, since enabling keepalive, we've seen a decrease in |
The team is reviewing this, my question is if this PR addresses the problem? Are you able to confirm that? Unfortunately we are not able to release alpha/beta versions, which would really help us to validate these types of bug fixes, but we are considering it for the future. |
This reverts commit 8ec38e8.
Allow CloudDeviceApi and EncryptedCloudDeviceApi requests to recover from transient Node.js keep-alive socket drops (`socket hang up` / `ECONNRESET`). - Add `retries?: number` to `IRequest.Options`, defaulting to 0 and capped at 3. - Extend `ApiException` to expose the underlying transport code, such as `ECONNRESET`. - Serialize and encrypt Nexo payloads once before retrying so exact payload identity (`ServiceID`, `POIID`, `SaleID`, and encrypted `NexoBlob`) is preserved. - Restrict retries to recognized transport socket errors; HTTP 4xx/5xx responses and Nexo business errors are not retried. - Add Cloud Device API tests verifying identical payload retransmission.
naseemkullah
changed the title
Good call @gcatanese as I implemented an app side retry (which is working great btw!) I realized the Nexo protocol has no idempotency key header. I completely revamped this PR. Could re-request Gemini to review since its a different approach, or I can open a new one, let me know. Thanks! |
This branch has not been deployed
2 participants
Allow CloudDeviceApi and EncryptedCloudDeviceApi requests to recover from
transient Node.js keep-alive socket drops (
socket hang up/ECONNRESET).retries?: numbertoIRequest.Options, defaulting to 0 and capped at 3.ApiExceptionto expose the underlying transport code, such asECONNRESET.identity (
ServiceID,POIID,SaleID, and encryptedNexoBlob) is preserved.and Nexo business errors are not retried.