Network-level fetch failures produce opaque "fetch failed" errors and skip bounded retries #267
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
When a REST call to an issue-source or MR platform (Jira, GitLab, GitHub, Forgejo) fails at the network level — DNS resolution failure, connection refused, TLS error, timeout, etc. — pi-loop surfaces only Node's generic
TypeError: fetch failedmessage, with no indication of what failed to connect or why. This makes misconfiguration (e.g. a wrong/placeholder base URL) very hard to diagnose from the logs alone. It also means these failures are not retried, contradicting AGENTS.md's documented error-handling policy that transient/network errors on the issue source or model get bounded retries.Real-world trigger (observed)
A target repo's
piloop-config.yamlhad a leftover placeholder frompiloop-config.example.yaml:Because YAML config takes precedence over inferring the base URL from the git origin host, every GitLab REST call in the
mrstage targeted the non-resolvablegitlab.example.com. The resulting log gave no clue what was wrong:Confirmed via direct reproduction:
Node's
fetchdiscards the actionable detail intoerror.cause, and pi-loop never reads it.Root cause (traced)
GitLabMrClient.createMr()(and every other REST client built on the sharedmakeAuthenticatedRequesthelper — GitLab/GitHub/Forgejo MR clients, GitLab/GitHub/Forgejo issue clients, the Forgejo merge poller, plus Jira's own structurally-identicalrequest()method inJiraRestClient) callshttpFetch(url, init)insidewithRetry().httpFetch(raw globalfetch) fails at the network level, it throws before anHttpResponseis produced — so theif (!response.ok)branch that maps HTTP status codes to a classified error (MrClientError/IssueSourceError/JiraErrorwith properkind/retriable) never runs.TypeError('fetch failed')propagates unchanged.withRetry'sisRetriablepredicate is always scoped toerror instanceof <DomainError> && error.retriable, so this un-classified rawTypeErrorfails that check and is not retried — it fails after a single attempt.classifyError()(src/orchestrator/helpers/classifyError.ts), which has no explicit case forMrClientError/IssueSourceErrornetwork failures either — see related gap below — so it falls through to the generic per-stage default (agent-failureformr/implement/review/remediate/docs,unknownotherwise), anderror.json'smessageis literally whatevererror.messagewas:"fetch failed".Related gap found during investigation
classifyError()currently has no case forMrClientErrorat all (only forMrStageError,JiraError/IssueSourceError,GitError, config errors). This means even ordinary HTTP-status MR-client errors (401/403/404/429/5xx frommrClientErrorFromStatus) are flattened to genericagent-failureinerror.jsontoday, losing their realkind/retriableclassification. This should be fixed alongside the network-error work since the fix touches the same code path.Affected call sites
All REST clients built on
src/shared/http/authenticatedFetch.ts'smakeAuthenticatedRequest():src/gitlab/services/gitLabMrClient.tssrc/gitlab/services/gitLabIssueClient.tssrc/github/services/gitHubMrClient.tssrc/github/services/gitHubIssueClient.tssrc/forgejo/services/forgejoMrClient.tssrc/forgejo/services/forgejoIssueClient.tssrc/mr/services/forgejoMergePoller.tsPlus the structurally-identical (but separate)
private request()method in:src/jira/services/jiraRestClient.tsNote:
src/mr/services/gitlabMergePoller.tsandsrc/mr/services/githubMergePoller.tsuse CLI executors (glab/gh), notfetch, so they are out of scope for thefetch-specific part of this fix, but see the "Also consider" note below.Proposed fix
1. Classify network-level fetch failures at the source
Add a shared helper (e.g.
src/shared/http/classifyFetchError.ts) that recognizes:TypeErrorwith messagefetch failedand inspectserror.causefor acode(ENOTFOUND,ECONNREFUSED,ECONNRESET,ETIMEDOUT,EAI_AGAIN, cert/TLS error codes, etc.) and/orhostnameDOMException/TimeoutErrorfromAbortSignal.timeout(...)if/when request timeouts are addedProduce a clear, actionable message, e.g.:
2. Add a
networkerror kind end-to-endsrc/shared/http/classifyHttpStatus.ts— add'network'toHttpErrorKind, retriabletrueMrClientError(src/mr/helpers/mrClientError.ts),IssueSourceError(src/issue/helpers/issueSourceError.ts),JiraError(src/jira/helpers/jiraError.ts+src/jira/types/jiraError.ts) — all reuseHttpErrorKind/mirror it, sonetworkflows through automatically once added there; verifyJiraErrorKind(currently its own separate union) is updated toosrc/orchestrator/types/orchestrator.ts— add'network'toErrorKind(error.jsonschema)src/artifacts/helpers/validateErrorReport.ts— add'network'toVALID_KINDSDESIGN.md§3.4 — update theerror.jsonkind enum documentation (currentlyconfig|auth|not-found|rate-limit|transient|agent-failure|git|unknown)3. Wire classification into each request path
In
makeAuthenticatedRequest()(src/shared/http/authenticatedFetch.ts), wrap thehttpFetch(url, init)call itself (not just the!response.okbranch) in atry/catchthat runs the newclassifyFetchErrorhelper and throws the caller'serrorFactory-equivalent for network errors — likely requires extendingAuthenticatedRequestOptionswith anetworkErrorFactory(or reusingerrorFactorywith a sentinel status, e.g.0, mapped tonetworkinclassifyHttpStatus). Apply the same treatment toJiraRestClient's separaterequest()method for consistency.4. Fix
isRetriablepredicates to retry network errorsSince
networkwill beretriable: truein the sharedclassifyHttpStatus/kind policy, the existingisRetriable: (error) => error instanceof MrClientError && error.retriablepredicates (and theIssueSourceError/JiraErrorequivalents) will automatically pick this up once network failures are classified into those same error types — no separate predicate change needed if step 3 is done correctly. Confirm this with a test that a network failure actually retries (per AGENTS.md's bounded-retry policy: 3 attempts, exponential backoff).5. Fix the
classifyError()gap forMrClientError/IssueSourceErrorAdd explicit cases in
src/orchestrator/helpers/classifyError.tsforMrClientError(mirroring the existingJiraError/IssueSourceErrorcase: useerror.kind/error.retriabledirectly) so MR-client failures of any kind (not just network) surface their real classification inerror.jsoninstead of the generic per-stage default.Test plan
classifyFetchError: recognizesENOTFOUND,ECONNREFUSED,ETIMEDOUT,ECONNRESET, unknowncause.code(generic network fallback), non-TypeError/non-fetch errors (pass through unclassified).classifyHttpStatus:networkkind added, retriabletrue; existing status-code cases unaffected.makeAuthenticatedRequest(): a mockhttpFetchthat throws aTypeError('fetch failed')(with acause) is caught, classified, and retried up tomaxAttemptsper the injected retry options; asserts the final thrown error carrieskind: 'network',retriable: true, and an actionable message including the host and cause code.JiraRestClient'srequest(): same network-failure classification + retry behavior asmakeAuthenticatedRequest.classifyError(): new case forMrClientErrormirroring theJiraError/IssueSourceErrortest — assertskind/retriablepass through unchanged (covers bothnetworkand pre-existing HTTP-status kinds, closing the related gap in item 5).validateErrorReport:'network'accepted as a validkind.httpFetchseam, simulating a thrownTypeError('fetch failed')fromGitLabMrClient.createMr(), asserting the surfaced error message names the unreachable host.Acceptance criteria
error.jsonwithkind: "network",retriable: true, and amessagethat names the host and the underlying cause (e.g.ENOTFOUND), not the bare string"fetch failed".MrClientErrorfailures of any kind (not just network) are classified correctly byclassifyError()instead of falling back to the generic per-stageagent-failure/unknowndefault.401/403/404/429/5xx) is unaffected — no regression in fail-fast vs retriable behavior for those cases.DESIGN.md§3.4 anderror.jsonschema/tests are updated to includenetworkas a validkind.Also consider (out of scope for this issue, noted for future work)
src/mr/services/gitlabMergePoller.ts/githubMergePoller.tsshell out toglab/ghCLIs viaexecFile, notfetch— a similarly opaque CLI-invocation failure (e.g.glabbinary missing, orglabitself hitting a DNS failure) would need separate handling incliMergePollerBase.ts'smakeCliRunnercatch block, which currently just wrapserror.messageaskind: 'unknown', retriable: falseregardless of cause.fetchcall (relies on OS-level TCP timeouts, which can be very slow for unreachable-but-routable hosts, e.g. black-holed IPs) — worth a separate issue to addAbortSignal.timeout(...)with a sensible default.Discovered while investigating
Forgejo issue #265 (fixed): a different, unrelated resume-detection bug where the docs stage was needlessly re-run after a no-op completion, which was masking/compounding the diagnosis of this network-error issue in the same run.
pi-loop opened and merged a pull request for this issue: #272