Add configurable timeouts to SCM API requests#7536
Conversation
The HTTP client used to access the Git hosting service API (GitHub, GitLab, etc.) hardcoded a 60-second connect timeout and never set a request timeout, so a request that connected but received no response could wait indefinitely. Set a read timeout on every SCM API request and make both timeouts configurable via the NXF_GIT_CONNECT_TIMEOUT and NXF_GIT_READ_TIMEOUT environment variables (default: 60s each). Timeout errors now report the affected URL and the variable to tune. Assisted-by: Claude Code (Fable 5) Signed-off-by: Rob Syme <rob.syme@gmail.com>
A connect timeout means the TCP handshake never completed, therefore no request data reached the server and a retry is always safe. The observed failure mode is a per-connection stall on the network path, where a fresh connection attempt typically succeeds. With a lowered NXF_GIT_CONNECT_TIMEOUT (e.g. 5s) a stalled request now recovers in a few seconds instead of failing the run. Make HttpConnectTimeoutException retryable in the SCM HTTP client retry condition. Read timeouts (the parent HttpTimeoutException) stay fail-fast because the request reached the server. The connect timeout error message now also reports the number of attempts made. Assisted-by: Claude Code (Fable 5) Signed-off-by: Rob Syme <rob.syme@gmail.com>
✅ Deploy Preview for nextflow-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
pditommaso
left a comment
There was a problem hiding this comment.
Thanks @robsyme, this is really nice work — the connect-vs-read retry reasoning is spot on. 🙌
One thought before we merge: could we converge this on the pattern we just landed in nf-seqera instead of adding env vars? Concretely:
Now: swap NXF_GIT_* for an scm.httpClient config scope mirroring seqera.executor.httpClient (io.seqera.config.HttpClientOpts) — same connectTimeout / requestTimeout names and the same zero-handling (requestTimeout = 0 = unbounded, connectTimeout must be > 0). Nice side effect: that model already avoids the 0/0s crash the current timeout0() hits at HttpRequest.timeout(ZERO). And since these env vars aren't released yet, there's nothing to deprecate.
A couple of things we can just reuse:
- Retry's already converged —
RetryConfig implements Retryable.Config, andHxConfig.defaultRetryCondition()already does exactly the "retry connect timeout, not read timeout" rule you hand-rolled inisRetryable. So no new HxClient interface needed — let's lean on what's there. - Small correctness note:
HttpRequest.timeout()also covers the connect phase, so ifrequestTimeout < connectTimeouta connect stall surfaces as the non-retryable exception. Setting the per-request timeout toconnect + requestsidesteps it.
Later (separate PR): we'll then have three near-identical client-config classes (wave, seqera, scm), so I'd like to lift a shared HttpClientOpts into nf-commons — but that's follow-up, no need to block this on it.
WDYT?
|
Thanks Paolo. I agree that there are already a bewildering number of NXF env vars to contend with. That said, the really important client of this change is Platform, which imports this SCM code as a library. It's not totally clear to me if/how configuration options are read in there. I'll have a look tonight. |
The HTTP client used by
nextflow.scm.RepositoryProviderto call the Git hosting service API (GitHub, GitLab, etc.) hardcodes a 60-second connect timeout and sets no per-request timeout. An API call that connects and then receives no response waits indefinitely, and a dropped connection attempt costs a fixed 60 seconds with no retry. In server contexts that embed this code, a single stalled request is enough to produce multi-minute API latency.Changes
Commit 1 makes both timeouts explicit and configurable:
NXF_GIT_CONNECT_TIMEOUTandNXF_GIT_READ_TIMEOUTas duration strings (default:60seach).IOExceptionreporting the affected URL and the variable to tune.Commit 2 makes connect timeouts retryable while read timeouts stay fail-fast:
instanceofcheck targetsHttpConnectTimeoutExceptionspecifically since it extendsHttpTimeoutException.HxClientretry loop, so the error is translated toIOException(now reporting the attempt count) only once all attempts are exhausted.Note the retryability multiplies the connect timeout for a persistently unreachable host: with defaults, 60s x 5 attempts is about 5 minutes before failing. Deployments that want bounded interactive latency should lower the connect timeout, e.g.
NXF_GIT_CONNECT_TIMEOUT=5scaps the worst case at roughly 30 seconds including backoff (tunable further via the existingNXF_RETRY_POLICY_*variables), while a transient stall recovers in about 5-6 seconds on retry. The docs entry spells this out.Tests
isRetryablecovering connect timeout (retryable) vs plainHttpTimeoutException(not retryable), alongside the existingSocketException/UnresolvedAddressExceptioncases.HxConfigretry-condition wiring, including cause traversal and null-cause safety.Complements #7401, #7402 and #7403, which reduce the volume of SCM API calls made during revision resolution.
🤖 Generated with Claude Code