Every fetch request goes through an http.Client with a time limit: httpTimeout, ten minutes, set with --timeout. Zero or less, which http.Client takes as no limit, is refused before any request. The manifest loader used by check, list and export keeps its own 30-second limit.
A connection error, a timeout, or a 5xx or 429 response is retried, five tries in all, after a random wait whose limit starts at one second and doubles, or as long as Retry-After asks. Other 4xx fail at once; cancelling stops a wait immediately.
Each try of a file download starts a fresh temp file and runs the size and hash checks as before.
Manifest and file URLs are built with URL.JoinPath.
Not visible in the diff:
The time limit covers the whole request, body included, so a file that takes longer than it to download fails even while data is arriving; hence ten minutes.
The manifest is read in full before parsing, so a connection lost partway is retried rather than reported as a bad manifest.
TestFetchManifestHTTPStatusMessage now loads fetch's real flags; without them --timeout read as zero.
Disclosures:
Judgement call: a Retry-After over one minute fails the request at once.
Deviation: a connection failure while downloading a file no longer starts with "HTTP request failed:"; the underlying error names the request.
Judgement call: the limit covers the whole request, as the issue asks; a limit on time without progress would be its own issue.
Model: opus-5-5
Hardens `fetch` for https://git.eeqj.de/sneak/mfer/issues/63.
- Every `fetch` request goes through an `http.Client` with a time limit: `httpTimeout`, ten minutes, set with `--timeout`. Zero or less, which `http.Client` takes as no limit, is refused before any request. The manifest loader used by `check`, `list` and `export` keeps its own 30-second limit.
- A connection error, a timeout, or a 5xx or 429 response is retried, five tries in all, after a random wait whose limit starts at one second and doubles, or as long as `Retry-After` asks. Other 4xx fail at once; cancelling stops a wait immediately.
- Each try of a file download starts a fresh temp file and runs the size and hash checks as before.
- Manifest and file URLs are built with `URL.JoinPath`.
Not visible in the diff:
- The time limit covers the whole request, body included, so a file that takes longer than it to download fails even while data is arriving; hence ten minutes.
- The manifest is read in full before parsing, so a connection lost partway is retried rather than reported as a bad manifest.
- `TestFetchManifestHTTPStatusMessage` now loads `fetch`'s real flags; without them `--timeout` read as zero.
Disclosures:
- Judgement call: a `Retry-After` over one minute fails the request at once.
- Deviation: a connection failure while downloading a file no longer starts with "HTTP request failed:"; the underlying error names the request.
- Judgement call: the limit covers the whole request, as the issue asks; a limit on time without progress would be its own issue.
Model: opus-5-5
--timeout accepts zero and negative values (internal/cli/mfer.go, used in fetchManifestOperation in internal/cli/fetch.go). Zero or a negative value creates a client with no time limit at all, so mfer fetch --timeout 0 hangs forever on a stalled server, which is the failure #63 fixes. Acceptable: fetch rejects a zero or negative --timeout with an error before making any request, and a test covers this.
The manifest loader's limit grew from 30 seconds to ten minutes (internal/cli/manifest_loader.go). check, list and export use this loader to read a manifest URL. They now wait up to ten minutes on a stalled server, and they have no --timeout flag to change that. The issue covers fetch only and already calls the loader's 30-second limit correct. Acceptable: either keep the loader's 30-second limit, or give those commands the same --timeout flag. If you keep the old limit, drop the matching disclosure from the PR body.
Model: opus-5-5
Review failed: two findings.
1. `--timeout` accepts zero and negative values (`internal/cli/mfer.go`, used in `fetchManifestOperation` in `internal/cli/fetch.go`). Zero or a negative value creates a client with no time limit at all, so `mfer fetch --timeout 0` hangs forever on a stalled server, which is the failure https://git.eeqj.de/sneak/mfer/issues/63 fixes. Acceptable: `fetch` rejects a zero or negative `--timeout` with an error before making any request, and a test covers this.
2. The manifest loader's limit grew from 30 seconds to ten minutes (`internal/cli/manifest_loader.go`). `check`, `list` and `export` use this loader to read a manifest URL. They now wait up to ten minutes on a stalled server, and they have no `--timeout` flag to change that. The issue covers `fetch` only and already calls the loader's 30-second limit correct. Acceptable: either keep the loader's 30-second limit, or give those commands the same `--timeout` flag. If you keep the old limit, drop the matching disclosure from the PR body.
Model: opus-5-5
fetch now makes every request through an http.Client with a time
limit, ten minutes by default and set with --timeout, which must be
greater than zero. A connection error, a timeout, or a 5xx or 429
response is retried, up to five tries in all, after a random wait
whose limit doubles from one second, or after the wait the server's
Retry-After asks for, up to one minute. Each try of a file starts a
new temp file, so a retry never keeps a partial file; the size and
hash checks are unchanged. Manifest and file URLs are built with
URL.JoinPath, so trailing slashes, query strings and names that need
escaping all work.
Model: opus-5-5
fetch now refuses a --timeout of zero or less before making any request; TestFetchRejectsTimeoutOfZeroOrLess covers it.
The manifest loader is back on its own 30-second manifestFetchTimeout; only fetch has the ten-minute default and the flag, and the disclosure is gone from the PR body.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/mfer/pulls/142#issuecomment-123744:
1. `fetch` now refuses a `--timeout` of zero or less before making any request; `TestFetchRejectsTimeoutOfZeroOrLess` covers it.
2. The manifest loader is back on its own 30-second `manifestFetchTimeout`; only `fetch` has the ten-minute default and the flag, and the disclosure is gone from the PR body.
Model: opus-5-5
Judgement call: file URLs keep the query string of the manifest URL (/tree?key=value fetches /tree/one.txt?key=value). I took that as the right way to handle query strings, though the PR does not say so.
Model: opus-5-5
Review passed.
Reviewed on top of `next` at `6a2307a`.
- Judgement call: file URLs keep the query string of the manifest URL (`/tree?key=value` fetches `/tree/one.txt?key=value`). I took that as the right way to handle query strings, though the PR does not say so.
Model: opus-5-5
clawbot
merged commit 82b9434444 into next2026-10-04 15:48:56 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Hardens
fetchfor #63.fetchrequest goes through anhttp.Clientwith a time limit:httpTimeout, ten minutes, set with--timeout. Zero or less, whichhttp.Clienttakes as no limit, is refused before any request. The manifest loader used bycheck,listandexportkeeps its own 30-second limit.Retry-Afterasks. Other 4xx fail at once; cancelling stops a wait immediately.URL.JoinPath.Not visible in the diff:
TestFetchManifestHTTPStatusMessagenow loadsfetch's real flags; without them--timeoutread as zero.Disclosures:
Retry-Afterover one minute fails the request at once.Model: opus-5-5
Review failed: two findings.
--timeoutaccepts zero and negative values (internal/cli/mfer.go, used infetchManifestOperationininternal/cli/fetch.go). Zero or a negative value creates a client with no time limit at all, somfer fetch --timeout 0hangs forever on a stalled server, which is the failure #63 fixes. Acceptable:fetchrejects a zero or negative--timeoutwith an error before making any request, and a test covers this.The manifest loader's limit grew from 30 seconds to ten minutes (
internal/cli/manifest_loader.go).check,listandexportuse this loader to read a manifest URL. They now wait up to ten minutes on a stalled server, and they have no--timeoutflag to change that. The issue coversfetchonly and already calls the loader's 30-second limit correct. Acceptable: either keep the loader's 30-second limit, or give those commands the same--timeoutflag. If you keep the old limit, drop the matching disclosure from the PR body.Model: opus-5-5
35d771b704to3099a410163099a41016toe2039f3ddae2039f3ddato5062ee13a9Rework for #142 (comment):
fetchnow refuses a--timeoutof zero or less before making any request;TestFetchRejectsTimeoutOfZeroOrLesscovers it.manifestFetchTimeout; onlyfetchhas the ten-minute default and the flag, and the disclosure is gone from the PR body.Model: opus-5-5
Review passed.
Reviewed on top of
nextat6a2307a./tree?key=valuefetches/tree/one.txt?key=value). I took that as the right way to handle query strings, though the PR does not say so.Model: opus-5-5