fix(routerlicious-driver): retry transient network errors in restWrapper

[arafat-java]

How contribute to this repo.

Guidelines for Pull Requests.

Description

Port of a fix validated in a downstream fork (concurrent-editing, commit f71f3c4), adapted to this repo's current restWrapper.ts/restWrapper.spec.ts conventions (which have diverged from the vendored copy — different imports, sinon-based test mocking instead of nock).

The routerlicious driver can run in environments (e.g. AWS Lambda) where the execution context is frozen between invocations. A keep-alive socket pooled by the underlying fetch transport can be closed by the peer (e.g. an ALB) during the freeze; the next request reuses the dead socket and rejects with "socket hang up" / ECONNRESET / EPIPE. request() now retries such transient network errors on a fresh socket, up to 3 attempts (1 initial + 2 retries, 250ms apart), before surfacing the error. A self-signed-certificate failure is excluded from retry since it's permanent, not transient.

Reviewer Guidance

The review process is outlined on this wiki page.

The retry bound (3 attempts) and delay (250ms) were chosen empirically in the downstream fork; happy to discuss if a different value/backoff strategy is preferred. This was authored in an environment without network access to build/test locally — CI will be the first real verification.

[Copilot]

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

[github-actions]

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (212 lines, 2 files), I've queued these reviewers:

• Correctness — logic errors, race conditions, lifecycle issues
• Security — vulnerabilities, secret exposure, injection
• API Compatibility — breaking changes, release tags, type design
• Performance — algorithmic regressions, memory leaks
• Testing — coverage gaps, hollow tests

How this works

• Adjust the reviewer set by ticking/unticking boxes above. Reviewer toggles alone don't trigger anything.

• Tick Start review below to dispatch the review fleet.

• After review finishes, tick Start review again to request another run — it auto-resets after each dispatch.

• This comment updates as new commits land; your reviewer selections are preserved.

• Start review

[github-actions]

🔭 PR Review Fleet Report

Note

This report is generated by an experimental AI review fleet and is provided as a beta feature. Findings are a starting point for discussion, not a gate. Use your own judgement.

Verdict: ❌ Request Changes

0 Alert, 1 Stop, 1 Caution

Findings

Sev	#	Area	File	What	Fix
🛑 Stop	H1	Correctness	packages/drivers/routerlicious-driver/src/restWrapper.ts:215-221	isTransientNetworkError is defined as !isSelfSignedCertError && (isNetworkError || <transientNetworkErrorPattern checks>). Since isNetworkError is simply ["TypeError", "FetchError"].includes(error?.name) (true for virtually every fetch-level failure — DNS failures, connection refused, aborted requests, CORS-blocked requests, invalid URL, response stream aborted mid-transfer, etc.), the transientNetworkErrorPattern (socket hang up|ECONNRESET|EPIPE) match is effectively dead code: any TypeError/FetchError already satisfies the OR via isNetworkError, regardless of whether the failure message actually matches a transient pattern. As a result the new retry loop (up to 3 attempts, 250ms apart) fires for ALL network-level errors, not just the intended 'pooled socket closed by peer' case described in the comment. This is applied uniformly to non-idempotent methods (POST/PATCH/DELETE, e.g. document/summary/blob creation) — if a write request's response fails after the server already processed it (e.g. connection reset mid-response, or any other TypeError raised after the body was transmitted), the wrapper will blindly resubmit the same POST/PATCH/DELETE up to 2 more times, risking duplicate resource creation or duplicate ops on the server. The self-signed-cert case is the only exclusion carved out, showing the authors were aware isNetworkError is overly broad but didn't exclude other non-transient TypeErrors (e.g. invalid URL, CORS failures, aborted signals).	Remove the bare isNetworkError disjunct from isTransientNetworkError and require the transient pattern to actually match (e.g. isTransientNetworkError = !isSelfSignedCertError && typeof errorCode === "string" && transientNetworkErrorPattern.test(errorCode) || transientNetworkErrorPattern.test(error?.message ?? "") || transientNetworkErrorPattern.test(error?.cause?.message ?? "")), so only genuinely transient socket errors are retried. Additionally, consider only auto-retrying idempotent methods (GET) inline in request(), and for non-idempotent methods either avoid the inline retry or ensure retries only happen when it can be established the request was never transmitted to the server.
🚧 Caution	M1	Testing	packages/drivers/routerlicious-driver/src/restWrapper.ts:214,221	The errorCode (error?.code ?? error?.cause?.code) and error?.cause?.message branches of isTransientNetworkError are never exercised by any test — every mock error is a bare TypeError with only a top-level message/code, so error.cause is always undefined. If a future refactor changes how isNetworkError is derived (e.g. tightens it, or the underlying fetch implementation stops throwing plain TypeErrors), these branches would become load-bearing but are currently completely unverified.	Add a test where the mock throws an error with error.name NOT in ["TypeError","FetchError"] (so isNetworkError is false) but with error.cause = { code: "ECONNRESET" } or error.cause = { message: "socket hang up" }, and assert the request is still retried and eventually succeeds/fails as expected — proving the cause-based fallback actually works independent of isNetworkError.

---

View workflow run

[CraigMacomber]

for improved type safety, type errors as unknown not any

[CraigMacomber]

In networking, I generally expect https://en.wikipedia.org/wiki/Exponential_backoff unless there is some known category of issue this timeout is selected to be relevant to. Might be good to document why this specific duration makes sense.

Might also be good to document the consequences of setting this or the retry count higher (I assume it can result in something taking longer before failing, or maybe hiding flakey service issues?).

Anyway, this isn't an area I work in, so I'm no expert here, those are just my thoughts.

[CraigMacomber]

/azp run Build - protocol-definitions,Build - test-tools,server-gitrest,server-gitssh,server-historian,server-routerlicious,Build - client packages,repo-policy-check

[CraigMacomber]

/azp run Build - api-markdown-documenter,Build - benchmark-tool,Build - build-common,Build - build-tools,Build - common-utils,Build - eslint-config-fluid,Build - eslint-plugin-fluid

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[azure-pipelines]

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

[github-actions]

Bundle size comparison

Base commit: ee981100f3fa5fb9b5ea26b9ef62efa7e0691b69
Head commit: c7f9600adc1b332e412a682732c7f52928cd0d0e

⚠️ Comparison unavailable.

The PR's CI build failed — fix the build and the comment will update once the next run succeeds.