Harden fetch: client timeout, retry with backoff, url.JoinPath #63

New Issue

clawbot · 2026-08-09T03:38:42+02:00

clawbot commented

2026-08-09 03:38:42 +02:00

Context

Three related defects in internal/cli/fetch.go, all on the same code path:

No client timeout. httpGet() (internal/cli/fetch.go:73-83) builds
the request with http.NewRequestWithContext but then issues it with
http.DefaultClient.Do(req). http.DefaultClient has no Timeout, so a
server that accepts the connection and then stalls hangs the fetch
forever. Note internal/cli/manifest_loader.go:31 already does this
correctly with &http.Client{Timeout: manifestFetchTimeout} — fetch is
the outlier.
No retry. There is no retry or backoff anywhere in fetch.go. A
single transient 5xx or dropped connection aborts a whole multi-file
download that may already be most of the way done. For the stated use
case — mirroring a large tree over flaky HTTP — this is the difference
between the tool being usable and not.
String-concatenation URL building.
internal/cli/fetch.go:143: fileURL := baseURL.String() + encodeFilePath(f.GetPath())
and resolveManifestURL (internal/cli/fetch.go:278-288):
parsed.Path += "index.mf". Both are wrong for base URLs with or without
a trailing slash, with query strings, or with escaped characters.

Definition of done

All HTTP requests in fetch go through an http.Client with an explicit
timeout. The timeout is a named constant and is overridable by the user
via a CLI flag.
Transient failures are retried with exponential backoff and a bounded
attempt count. "Transient" is defined explicitly and narrowly: connection
errors, timeouts, and 5xx / 429 responses. 4xx other than 429 must not
be retried.
Backoff includes jitter and respects Retry-After when the server sends
it.
Retry is cancellable: a ctx cancellation during a backoff sleep returns
promptly rather than sleeping out the interval.
URL construction uses url.JoinPath (or URL.JoinPath) throughout. No
string concatenation onto a URL or a URL.Path anywhere in the file.
Tests, against httptest.Server: a stalled server hits the timeout and
errors rather than hanging; a server that fails twice then succeeds is
retried to success; a 404 is not retried; a base URL with and without a
trailing slash both resolve to the same file URLs; a path needing escaping
round-trips correctly. Retry tests must use short, injected intervals so
the suite stays fast.
make check passes. TODO.md updated in the same commit.

Implementation requirements

Use the standard library. Do not add an HTTP retry dependency for this.
Make the timeout and backoff parameters injectable so tests do not sleep
for real. A test suite that takes seconds to prove backoff works is a
failed implementation of this issue.
Do not retry non-idempotent work, and do not retry a partially-consumed
response body without resetting state — check what downloadFile has
already written before re-attempting, and make sure a retry cannot produce
a truncated or double-written file.
Keep the existing hash and size verification behavior exactly as-is; a
retried download must still be verified.
Preserve the existing progress output semantics.
Commit title must end with (closes #63).

## Context Three related defects in `internal/cli/fetch.go`, all on the same code path: 1. **No client timeout.** `httpGet()` (`internal/cli/fetch.go:73-83`) builds the request with `http.NewRequestWithContext` but then issues it with `http.DefaultClient.Do(req)`. `http.DefaultClient` has no `Timeout`, so a server that accepts the connection and then stalls hangs the fetch forever. Note `internal/cli/manifest_loader.go:31` already does this correctly with `&http.Client{Timeout: manifestFetchTimeout}` — `fetch` is the outlier. 2. **No retry.** There is no retry or backoff anywhere in `fetch.go`. A single transient 5xx or dropped connection aborts a whole multi-file download that may already be most of the way done. For the stated use case — mirroring a large tree over flaky HTTP — this is the difference between the tool being usable and not. 3. **String-concatenation URL building.** `internal/cli/fetch.go:143`: `fileURL := baseURL.String() + encodeFilePath(f.GetPath())` and `resolveManifestURL` (`internal/cli/fetch.go:278-288`): `parsed.Path += "index.mf"`. Both are wrong for base URLs with or without a trailing slash, with query strings, or with escaped characters. ## Definition of done - All HTTP requests in `fetch` go through an `http.Client` with an explicit timeout. The timeout is a named constant and is overridable by the user via a CLI flag. - Transient failures are retried with exponential backoff and a bounded attempt count. "Transient" is defined explicitly and narrowly: connection errors, timeouts, and 5xx / 429 responses. 4xx other than 429 must **not** be retried. - Backoff includes jitter and respects `Retry-After` when the server sends it. - Retry is cancellable: a `ctx` cancellation during a backoff sleep returns promptly rather than sleeping out the interval. - URL construction uses `url.JoinPath` (or `URL.JoinPath`) throughout. No string concatenation onto a URL or a `URL.Path` anywhere in the file. - Tests, against `httptest.Server`: a stalled server hits the timeout and errors rather than hanging; a server that fails twice then succeeds is retried to success; a 404 is not retried; a base URL with and without a trailing slash both resolve to the same file URLs; a path needing escaping round-trips correctly. Retry tests must use short, injected intervals so the suite stays fast. - `make check` passes. `TODO.md` updated in the same commit. ## Implementation requirements - Use the standard library. Do not add an HTTP retry dependency for this. - Make the timeout and backoff parameters injectable so tests do not sleep for real. A test suite that takes seconds to prove backoff works is a failed implementation of this issue. - Do not retry non-idempotent work, and do not retry a partially-consumed response body without resetting state — check what `downloadFile` has already written before re-attempting, and make sure a retry cannot produce a truncated or double-written file. - Keep the existing hash and size verification behavior exactly as-is; a retried download must still be verified. - Preserve the existing progress output semantics. - Commit title must end with ` (closes #63)`.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:38:42 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/mfer#63