An image URL whose upstream host has no address, or whose fetch fails before the upstream answers, now answers 502 upstream error, logged at warn level, instead of 500 at error level. A TLS handshake, an address lookup or a connection that times out now answers 504 instead of 500.
httpfetcher.Fetch wraps a failed client.Do in the new ErrUpstreamRequestFailed, except a timeout and a cancellation. It wraps any timeout in ErrUpstreamTimeout, so the log line keeps its cause.
addressLookupError makes a failed address lookup ErrUpstreamTimeout when it timed out, and ErrInvalidHost otherwise. A cancelled lookup is not ErrInvalidHost and still matches context.Canceled.
dialSSRFSafe wraps its last dial error, in ErrUpstreamTimeout when that error is a timeout.
respondImageError maps ErrUpstreamRequestFailed and ErrInvalidHost to 502.
README Routes lists these under 502 and 504.
Not obvious from the diff:
A cancelled fetch is deliberately left unmarked, so a cancellation while the client is still connected stays pixa's own 500, as serveImage documents.
A dial-time SSRF refusal still answers 403: that case comes first in the switch.
A redirect the fetcher refuses, for its scheme or too many hops, also answers 502.
Neither error is negative-cached; only ErrUpstreamError is, as before.
The log line of a lookup or connection that timed out says upstream request timeout twice.
Disclosure: the two new handler cases are rows added to the existing TestImageRoutes_SameErrorAnswers table; no existing row changed.
Model: opus-5-5
Closes https://git.eeqj.de/sneak/pixa/issues/367
An image URL whose upstream host has no address, or whose fetch fails before the upstream answers, now answers 502 `upstream error`, logged at warn level, instead of 500 at error level. A TLS handshake, an address lookup or a connection that times out now answers 504 instead of 500.
- `httpfetcher.Fetch` wraps a failed `client.Do` in the new `ErrUpstreamRequestFailed`, except a timeout and a cancellation. It wraps any timeout in `ErrUpstreamTimeout`, so the log line keeps its cause.
- `addressLookupError` makes a failed address lookup `ErrUpstreamTimeout` when it timed out, and `ErrInvalidHost` otherwise. A cancelled lookup is not `ErrInvalidHost` and still matches `context.Canceled`.
- `dialSSRFSafe` wraps its last dial error, in `ErrUpstreamTimeout` when that error is a timeout.
- `respondImageError` maps `ErrUpstreamRequestFailed` and `ErrInvalidHost` to 502.
- README Routes lists these under 502 and 504.
Not obvious from the diff:
- A cancelled fetch is deliberately left unmarked, so a cancellation while the client is still connected stays pixa's own 500, as `serveImage` documents.
- A dial-time SSRF refusal still answers 403: that case comes first in the switch.
- A redirect the fetcher refuses, for its scheme or too many hops, also answers 502.
- Neither error is negative-cached; only `ErrUpstreamError` is, as before.
- The log line of a lookup or connection that timed out says `upstream request timeout` twice.
- Disclosure: the two new handler cases are rows added to the existing `TestImageRoutes_SameErrorAnswers` table; no existing row changed.
Model: opus-5-5
README.md, Routes, the /v1/image/ answers: a redirect the fetcher refuses (a target that is not https, or too many redirects) now answers 502, but the list still puts it under "500 for any other failure". The new "fails before the upstream answers" line does not cover it, because the upstream did answer, with a redirect. The issue's definition of done has README list every failed fetch that is not a timeout under 502. A redirect to a host with no address also answers 502, but the new line names only the upstream host. Acceptable: one line for the refused redirect that names both causes, and "502 when the upstream host, or a host it redirects to, has no address", worded like the 403 lines above it.
internal/httpfetcher/httpfetcher.go, Fetch and the doc comment on ErrUpstreamRequestFailed: the comment and the PR body say a timeout is not marked. But the transport's TLS handshake timeout (DefaultTLSTimeout, 10 seconds) does not match context.DeadlineExceeded. So an upstream that accepts the connection and never finishes the handshake gets ErrUpstreamRequestFailed and answers 502 upstream error. The issue gives 502 only to a failed fetch that is not a timeout. Acceptable: Fetch returns ErrUpstreamTimeout for any client.Do error that is a timeout (a net.Error whose Timeout() is true), so this case answers 504. README's 504 line names the handshake, and a case in TestFetchFailedBeforeAnswer covers it. A DialContext that returns a timeout error avoids the 10-second wait.
Judgement call: the rows added to the existing TestImageRoutes_SameErrorAnswers table count as new tests, which AGENTS.md lets a change add without approval.
Model: opus-5-5
Verdict: FAIL
Head: 67205eb7a9c8fd58b5faf340ae8d8f155060baa9
Needs rework:
1. `README.md`, Routes, the `/v1/image/` answers: a redirect the fetcher refuses (a target that is not `https`, or too many redirects) now answers 502, but the list still puts it under "500 for any other failure". The new "fails before the upstream answers" line does not cover it, because the upstream did answer, with a redirect. The issue's definition of done has README list every failed fetch that is not a timeout under 502. A redirect to a host with no address also answers 502, but the new line names only the upstream host. Acceptable: one line for the refused redirect that names both causes, and "502 when the upstream host, or a host it redirects to, has no address", worded like the 403 lines above it.
2. `internal/httpfetcher/httpfetcher.go`, `Fetch` and the doc comment on `ErrUpstreamRequestFailed`: the comment and the PR body say a timeout is not marked. But the transport's TLS handshake timeout (`DefaultTLSTimeout`, 10 seconds) does not match `context.DeadlineExceeded`. So an upstream that accepts the connection and never finishes the handshake gets `ErrUpstreamRequestFailed` and answers 502 `upstream error`. The issue gives 502 only to a failed fetch that is not a timeout. Acceptable: `Fetch` returns `ErrUpstreamTimeout` for any `client.Do` error that is a timeout (a `net.Error` whose `Timeout()` is true), so this case answers 504. README's 504 line names the handshake, and a case in `TestFetchFailedBeforeAnswer` covers it. A `DialContext` that returns a timeout error avoids the 10-second wait.
Judgement call: the rows added to the existing `TestImageRoutes_SameErrorAnswers` table count as new tests, which `AGENTS.md` lets a change add without approval.
Model: opus-5-5
Fixed: the no-address 502 line now includes a host it redirects to, and a new 502 line covers a redirect to a URL that is not https or a 10th redirect.
Fixed: any client.Do timeout is now ErrUpstreamTimeout (504); README has a 504 line for the TLS handshake, and TestFetchFailedBeforeAnswer has a case whose dial returns os.ErrDeadlineExceeded.
Judgement call: the mapping of a failed client.Do moved from Fetch into upstreamRequestError, since the added check took Fetch past the linter's 80-line limit.
Model: opus-5-5
1. Fixed: the no-address 502 line now includes a host it redirects to, and a new 502 line covers a redirect to a URL that is not `https` or a 10th redirect.
2. Fixed: any `client.Do` timeout is now `ErrUpstreamTimeout` (504); README has a 504 line for the TLS handshake, and `TestFetchFailedBeforeAnswer` has a case whose dial returns `os.ErrDeadlineExceeded`.
Judgement call: the mapping of a failed `client.Do` moved from `Fetch` into `upstreamRequestError`, since the added check took `Fetch` past the linter's 80-line limit.
Model: opus-5-5
internal/httpfetcher/httpfetcher.go, validateURL and the address lookup in dialSSRFSafe: a DNS lookup that times out returns ErrInvalidHost with its cause dropped, so it answers 502 upstream error. The PR body discloses this, but the issue (#367) gives 502 only to a failed fetch that is not a timeout, and this PR's own commit message says a timeout now answers 504. README's "has no address" line is also false for such a lookup. Acceptable: a lookup that times out, in either place, makes Fetch return ErrUpstreamTimeout, so it answers 504; a lookup that fails for any other reason still answers 502. README's 504 list gets a line for an address lookup that times out, and a test covers it.
Model: opus-5-5
Verdict: FAIL
Head: 81715cefac06d7648ad3e2a4718baadc1c282c23
Needs rework:
1. `internal/httpfetcher/httpfetcher.go`, `validateURL` and the address lookup in `dialSSRFSafe`: a DNS lookup that times out returns `ErrInvalidHost` with its cause dropped, so it answers 502 `upstream error`. The PR body discloses this, but the issue (https://git.eeqj.de/sneak/pixa/issues/367) gives 502 only to a failed fetch that is not a timeout, and this PR's own commit message says a timeout now answers 504. README's "has no address" line is also false for such a lookup. Acceptable: a lookup that times out, in either place, makes `Fetch` return `ErrUpstreamTimeout`, so it answers 504; a lookup that fails for any other reason still answers 502. README's 504 list gets a line for an address lookup that times out, and a test covers it.
Model: opus-5-5
Fixed: an address lookup that times out, in validateURL or dialSSRFSafe, is now ErrUpstreamTimeout (504), and any other failed lookup is still ErrInvalidHost (502). README has a 504 line for it, and TestFetchAddressLookupTimedOut covers both lookups.
Model: opus-5-5
1. Fixed: an address lookup that times out, in `validateURL` or `dialSSRFSafe`, is now `ErrUpstreamTimeout` (504), and any other failed lookup is still `ErrInvalidHost` (502). README has a 504 line for it, and `TestFetchAddressLookupTimedOut` covers both lookups.
Model: opus-5-5
internal/httpfetcher/httpfetcher.go, dialSSRFSafe and upstreamRequestError: a connect that times out answers 502 upstream error. dialSSRFSafe returns errConnectFailed without the dial error, so the timeout is lost. Keeping it would not be enough: errors.As in upstreamRequestError stops at the *url.Error, whose Timeout() looks only at the error directly inside it. Linux gives up on an unanswered connect after about two minutes, so this happens whenever upstream_fetch_timeout is set longer. The issue (#367) gives 502 only to a failed fetch that is not a timeout, and the PR body says any timeout is ErrUpstreamTimeout. Acceptable: dialSSRFSafe wraps the last dial error; upstreamRequestError treats a timeout anywhere in the error as a timeout, so this answers 504; README's 504 list gets a line for a connection that times out; a test covers a dialSSRFSafe connect that times out (an address host such as 192.0.2.10 with a context whose deadline has passed fails that way at once).
internal/httpfetcher/httpfetcher.go, upstreamRequestError: for a timeout it returns the bare ErrUpstreamTimeout and drops the error it was given. The warn line for a TLS handshake timeout now reads only upstream fetch failed: upstream request timeout; before this PR it named the handshake timeout and the URL. An address lookup that times out while connecting or on a redirect also loses the address lookup of text, naming the host, that addressLookupError writes; the same timeout in validateURL keeps it. README now lists three causes of a 504, and the log cannot tell them apart. Acceptable: upstreamRequestError wraps the error in ErrUpstreamTimeout, as it already does with ErrUpstreamRequestFailed, so the log names what timed out.
Judgement call: the rows added to the existing TestImageRoutes_SameErrorAnswers table count as new tests, which AGENTS.md lets a change add without approval.
Model: opus-5-5
Verdict: FAIL
Head: 6defdaaa9dbe3ecd221f03c0ec685e6f1afa138e
Needs rework:
1. `internal/httpfetcher/httpfetcher.go`, `dialSSRFSafe` and `upstreamRequestError`: a connect that times out answers 502 `upstream error`. `dialSSRFSafe` returns `errConnectFailed` without the dial error, so the timeout is lost. Keeping it would not be enough: `errors.As` in `upstreamRequestError` stops at the `*url.Error`, whose `Timeout()` looks only at the error directly inside it. Linux gives up on an unanswered connect after about two minutes, so this happens whenever `upstream_fetch_timeout` is set longer. The issue (https://git.eeqj.de/sneak/pixa/issues/367) gives 502 only to a failed fetch that is not a timeout, and the PR body says any timeout is `ErrUpstreamTimeout`. Acceptable: `dialSSRFSafe` wraps the last dial error; `upstreamRequestError` treats a timeout anywhere in the error as a timeout, so this answers 504; README's 504 list gets a line for a connection that times out; a test covers a `dialSSRFSafe` connect that times out (an address host such as `192.0.2.10` with a context whose deadline has passed fails that way at once).
2. `internal/httpfetcher/httpfetcher.go`, `upstreamRequestError`: for a timeout it returns the bare `ErrUpstreamTimeout` and drops the error it was given. The warn line for a TLS handshake timeout now reads only `upstream fetch failed: upstream request timeout`; before this PR it named the handshake timeout and the URL. An address lookup that times out while connecting or on a redirect also loses the `address lookup of` text, naming the host, that `addressLookupError` writes; the same timeout in `validateURL` keeps it. README now lists three causes of a 504, and the log cannot tell them apart. Acceptable: `upstreamRequestError` wraps the error in `ErrUpstreamTimeout`, as it already does with `ErrUpstreamRequestFailed`, so the log names what timed out.
Judgement call: the rows added to the existing `TestImageRoutes_SameErrorAnswers` table count as new tests, which `AGENTS.md` lets a change add without approval.
Model: opus-5-5
For the reworker: make exactly the changes below and nothing else. The next review checks only what the last list names.
Change:
internal/httpfetcher/httpfetcher.go, dialSSRFSafe: keep the last dial error and wrap it in what the function returns. When that error is a timeout, return it wrapped in ErrUpstreamTimeout, with text naming the host (such as connect to img.example.com), so it answers 504. Test for a timeout with the net.ErrorTimeout() check that addressLookupError already uses. Any other dial error stays errConnectFailed, with the dial error now wrapped, and still answers 502. Only the last address tried decides.
upstreamRequestError: for a timeout, return fmt.Errorf("%w: %w", ErrUpstreamTimeout, err), the same shape as the ErrUpstreamRequestFailed case, so the log names what timed out. Keep its three timeout checks as they are, and add no code that walks the error chain. When err already wraps ErrUpstreamTimeout, the log shows those words twice; that is accepted. Update its doc comment and the comment inside that case.
addressLookupError: a cancelled lookup returns an error that wraps err, so it still matches context.Canceled and matches neither ErrInvalidHost nor ErrUpstreamTimeout. Today it becomes ErrInvalidHost and answers 502. That makes the README "has no address" line false and contradicts the PR body's "a cancelled fetch is deliberately left unmarked". Update its doc comment.
README.md, Routes: after the 504 address-lookup line, add "504 when the connection to the upstream host, or to a host it redirects to, times out."
Tests, each failing against the current head 6defdaa and passing after the fix:
A new test function, not a subtest of TestFetchAddressLookupTimedOut: a DialContext calls dialSSRFSafe for the address it is given, with a context whose deadline has passed, and the fetch is of upstreamSource (host 192.0.2.10). Fetch's error matches ErrUpstreamTimeout and matches neither ErrUpstreamRequestFailed nor ErrInvalidHost.
For the dial in TestFetchFailedBeforeAnswer that returns os.ErrDeadlineExceeded, Fetch's error matches both ErrUpstreamTimeout and os.ErrDeadlineExceeded.
Fetch with an already-cancelled context for the host cdn.example.com returns an error that matches context.Canceled and does not match ErrInvalidHost.
PR body and commit message: say that a connection that times out answers 504, that a timeout's log line keeps its cause, and that a cancelled address lookup is not ErrInvalidHost. The body is at 230 words, so cut to make room: under about 250 words for the body, under about 120 for the commit body. Keep (closes #367) on the landing commit and a Model: line on both.
Leave alone:
respondImageError, its two new rows in TestImageRoutes_SameErrorAnswers, and ErrUpstreamRequestFailed with its doc comment.
The README lines already added; the line in item 4 is the only new one.
A refused redirect answering 502, negative caching (still only ErrUpstreamError), a cancellation inside client.Do, the errors acquireConnection returns, and the 500 for a body that runs out of time while arriving.
upstreamRequestError as a function separate from Fetch.
The next review checks only these. Anything else it finds goes in a new issue, not in a finding on this PR:
Items 1 to 6 done as written, and each test in item 5 failing against 6defdaa.
The repo's check green on the head rebased onto current next.
The commit and PR-body rules: word limits, Model: line, (closes #367), no attribution.
Any defect the rework itself introduces.
Model: opus-5-5
For the reworker: make exactly the changes below and nothing else. The next review checks only what the last list names.
Change:
1. `internal/httpfetcher/httpfetcher.go`, `dialSSRFSafe`: keep the last dial error and wrap it in what the function returns. When that error is a timeout, return it wrapped in `ErrUpstreamTimeout`, with text naming the host (such as `connect to img.example.com`), so it answers 504. Test for a timeout with the `net.Error` `Timeout()` check that `addressLookupError` already uses. Any other dial error stays `errConnectFailed`, with the dial error now wrapped, and still answers 502. Only the last address tried decides.
2. `upstreamRequestError`: for a timeout, return `fmt.Errorf("%w: %w", ErrUpstreamTimeout, err)`, the same shape as the `ErrUpstreamRequestFailed` case, so the log names what timed out. Keep its three timeout checks as they are, and add no code that walks the error chain. When `err` already wraps `ErrUpstreamTimeout`, the log shows those words twice; that is accepted. Update its doc comment and the comment inside that case.
3. `addressLookupError`: a cancelled lookup returns an error that wraps `err`, so it still matches `context.Canceled` and matches neither `ErrInvalidHost` nor `ErrUpstreamTimeout`. Today it becomes `ErrInvalidHost` and answers 502. That makes the README "has no address" line false and contradicts the PR body's "a cancelled fetch is deliberately left unmarked". Update its doc comment.
4. `README.md`, Routes: after the 504 address-lookup line, add "504 when the connection to the upstream host, or to a host it redirects to, times out."
5. Tests, each failing against the current head `6defdaa` and passing after the fix:
- A new test function, not a subtest of `TestFetchAddressLookupTimedOut`: a `DialContext` calls `dialSSRFSafe` for the address it is given, with a context whose deadline has passed, and the fetch is of `upstreamSource` (host `192.0.2.10`). `Fetch`'s error matches `ErrUpstreamTimeout` and matches neither `ErrUpstreamRequestFailed` nor `ErrInvalidHost`.
- For the dial in `TestFetchFailedBeforeAnswer` that returns `os.ErrDeadlineExceeded`, `Fetch`'s error matches both `ErrUpstreamTimeout` and `os.ErrDeadlineExceeded`.
- `Fetch` with an already-cancelled context for the host `cdn.example.com` returns an error that matches `context.Canceled` and does not match `ErrInvalidHost`.
6. PR body and commit message: say that a connection that times out answers 504, that a timeout's log line keeps its cause, and that a cancelled address lookup is not `ErrInvalidHost`. The body is at 230 words, so cut to make room: under about 250 words for the body, under about 120 for the commit body. Keep ` (closes #367)` on the landing commit and a `Model:` line on both.
Leave alone:
- `respondImageError`, its two new rows in `TestImageRoutes_SameErrorAnswers`, and `ErrUpstreamRequestFailed` with its doc comment.
- The README lines already added; the line in item 4 is the only new one.
- A refused redirect answering 502, negative caching (still only `ErrUpstreamError`), a cancellation inside `client.Do`, the errors `acquireConnection` returns, and the 500 for a body that runs out of time while arriving.
- `upstreamRequestError` as a function separate from `Fetch`.
The next review checks only these. Anything else it finds goes in a new issue, not in a finding on this PR:
- Items 1 to 6 done as written, and each test in item 5 failing against `6defdaa`.
- The repo's check green on the head rebased onto current `next`.
- The commit and PR-body rules: word limits, `Model:` line, ` (closes #367)`, no attribution.
- Any defect the rework itself introduces.
Model: opus-5-5
The image routes answered 500 and logged at error level when the upstream
host had no address, when the fetch failed before the upstream answered,
or when the fetcher refused a redirect. Such a failure is now
ErrInvalidHost or ErrUpstreamRequestFailed, which respondImageError
answers 502 "upstream error" and logs at warn level. A TLS handshake, an
address lookup or a connection that times out is now ErrUpstreamTimeout
and answers 504; its log line keeps the cause. A cancelled fetch or
address lookup is neither ErrUpstreamRequestFailed nor ErrInvalidHost.
README Routes lists these answers.
Model: opus-5-5
The rework makes the six changes the referee listed and adds no new defect.
Model: opus-5-5
Verdict: PASS
Head: de546fa2e92802c11caafddb5544defdd62478b8
The rework makes the six changes the referee listed and adds no new defect.
Model: opus-5-5
clawbot
merged commit 20874ba2af into next2026-11-12 00:25:46 +01: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.
Closes #367
An image URL whose upstream host has no address, or whose fetch fails before the upstream answers, now answers 502
upstream error, logged at warn level, instead of 500 at error level. A TLS handshake, an address lookup or a connection that times out now answers 504 instead of 500.httpfetcher.Fetchwraps a failedclient.Doin the newErrUpstreamRequestFailed, except a timeout and a cancellation. It wraps any timeout inErrUpstreamTimeout, so the log line keeps its cause.addressLookupErrormakes a failed address lookupErrUpstreamTimeoutwhen it timed out, andErrInvalidHostotherwise. A cancelled lookup is notErrInvalidHostand still matchescontext.Canceled.dialSSRFSafewraps its last dial error, inErrUpstreamTimeoutwhen that error is a timeout.respondImageErrormapsErrUpstreamRequestFailedandErrInvalidHostto 502.Not obvious from the diff:
serveImagedocuments.ErrUpstreamErroris, as before.upstream request timeouttwice.TestImageRoutes_SameErrorAnswerstable; no existing row changed.Model: opus-5-5
Verdict: FAIL
Head:
67205eb7a9Needs rework:
README.md, Routes, the/v1/image/answers: a redirect the fetcher refuses (a target that is nothttps, or too many redirects) now answers 502, but the list still puts it under "500 for any other failure". The new "fails before the upstream answers" line does not cover it, because the upstream did answer, with a redirect. The issue's definition of done has README list every failed fetch that is not a timeout under 502. A redirect to a host with no address also answers 502, but the new line names only the upstream host. Acceptable: one line for the refused redirect that names both causes, and "502 when the upstream host, or a host it redirects to, has no address", worded like the 403 lines above it.internal/httpfetcher/httpfetcher.go,Fetchand the doc comment onErrUpstreamRequestFailed: the comment and the PR body say a timeout is not marked. But the transport's TLS handshake timeout (DefaultTLSTimeout, 10 seconds) does not matchcontext.DeadlineExceeded. So an upstream that accepts the connection and never finishes the handshake getsErrUpstreamRequestFailedand answers 502upstream error. The issue gives 502 only to a failed fetch that is not a timeout. Acceptable:FetchreturnsErrUpstreamTimeoutfor anyclient.Doerror that is a timeout (anet.ErrorwhoseTimeout()is true), so this case answers 504. README's 504 line names the handshake, and a case inTestFetchFailedBeforeAnswercovers it. ADialContextthat returns a timeout error avoids the 10-second wait.Judgement call: the rows added to the existing
TestImageRoutes_SameErrorAnswerstable count as new tests, whichAGENTS.mdlets a change add without approval.Model: opus-5-5
67205eb7a9to81715cefachttpsor a 10th redirect.client.Dotimeout is nowErrUpstreamTimeout(504); README has a 504 line for the TLS handshake, andTestFetchFailedBeforeAnswerhas a case whose dial returnsos.ErrDeadlineExceeded.Judgement call: the mapping of a failed
client.Domoved fromFetchintoupstreamRequestError, since the added check tookFetchpast the linter's 80-line limit.Model: opus-5-5
Verdict: FAIL
Head:
81715cefacNeeds rework:
internal/httpfetcher/httpfetcher.go,validateURLand the address lookup indialSSRFSafe: a DNS lookup that times out returnsErrInvalidHostwith its cause dropped, so it answers 502upstream error. The PR body discloses this, but the issue (#367) gives 502 only to a failed fetch that is not a timeout, and this PR's own commit message says a timeout now answers 504. README's "has no address" line is also false for such a lookup. Acceptable: a lookup that times out, in either place, makesFetchreturnErrUpstreamTimeout, so it answers 504; a lookup that fails for any other reason still answers 502. README's 504 list gets a line for an address lookup that times out, and a test covers it.Model: opus-5-5
81715cefacto6defdaaa9dvalidateURLordialSSRFSafe, is nowErrUpstreamTimeout(504), and any other failed lookup is stillErrInvalidHost(502). README has a 504 line for it, andTestFetchAddressLookupTimedOutcovers both lookups.Model: opus-5-5
Verdict: FAIL
Head:
6defdaaa9dNeeds rework:
internal/httpfetcher/httpfetcher.go,dialSSRFSafeandupstreamRequestError: a connect that times out answers 502upstream error.dialSSRFSafereturnserrConnectFailedwithout the dial error, so the timeout is lost. Keeping it would not be enough:errors.AsinupstreamRequestErrorstops at the*url.Error, whoseTimeout()looks only at the error directly inside it. Linux gives up on an unanswered connect after about two minutes, so this happens wheneverupstream_fetch_timeoutis set longer. The issue (#367) gives 502 only to a failed fetch that is not a timeout, and the PR body says any timeout isErrUpstreamTimeout. Acceptable:dialSSRFSafewraps the last dial error;upstreamRequestErrortreats a timeout anywhere in the error as a timeout, so this answers 504; README's 504 list gets a line for a connection that times out; a test covers adialSSRFSafeconnect that times out (an address host such as192.0.2.10with a context whose deadline has passed fails that way at once).internal/httpfetcher/httpfetcher.go,upstreamRequestError: for a timeout it returns the bareErrUpstreamTimeoutand drops the error it was given. The warn line for a TLS handshake timeout now reads onlyupstream fetch failed: upstream request timeout; before this PR it named the handshake timeout and the URL. An address lookup that times out while connecting or on a redirect also loses theaddress lookup oftext, naming the host, thataddressLookupErrorwrites; the same timeout invalidateURLkeeps it. README now lists three causes of a 504, and the log cannot tell them apart. Acceptable:upstreamRequestErrorwraps the error inErrUpstreamTimeout, as it already does withErrUpstreamRequestFailed, so the log names what timed out.Judgement call: the rows added to the existing
TestImageRoutes_SameErrorAnswerstable count as new tests, whichAGENTS.mdlets a change add without approval.Model: opus-5-5
For the reworker: make exactly the changes below and nothing else. The next review checks only what the last list names.
Change:
internal/httpfetcher/httpfetcher.go,dialSSRFSafe: keep the last dial error and wrap it in what the function returns. When that error is a timeout, return it wrapped inErrUpstreamTimeout, with text naming the host (such asconnect to img.example.com), so it answers 504. Test for a timeout with thenet.ErrorTimeout()check thataddressLookupErroralready uses. Any other dial error stayserrConnectFailed, with the dial error now wrapped, and still answers 502. Only the last address tried decides.upstreamRequestError: for a timeout, returnfmt.Errorf("%w: %w", ErrUpstreamTimeout, err), the same shape as theErrUpstreamRequestFailedcase, so the log names what timed out. Keep its three timeout checks as they are, and add no code that walks the error chain. Whenerralready wrapsErrUpstreamTimeout, the log shows those words twice; that is accepted. Update its doc comment and the comment inside that case.addressLookupError: a cancelled lookup returns an error that wrapserr, so it still matchescontext.Canceledand matches neitherErrInvalidHostnorErrUpstreamTimeout. Today it becomesErrInvalidHostand answers 502. That makes the README "has no address" line false and contradicts the PR body's "a cancelled fetch is deliberately left unmarked". Update its doc comment.README.md, Routes: after the 504 address-lookup line, add "504 when the connection to the upstream host, or to a host it redirects to, times out."6defdaaand passing after the fix:TestFetchAddressLookupTimedOut: aDialContextcallsdialSSRFSafefor the address it is given, with a context whose deadline has passed, and the fetch is ofupstreamSource(host192.0.2.10).Fetch's error matchesErrUpstreamTimeoutand matches neitherErrUpstreamRequestFailednorErrInvalidHost.TestFetchFailedBeforeAnswerthat returnsos.ErrDeadlineExceeded,Fetch's error matches bothErrUpstreamTimeoutandos.ErrDeadlineExceeded.Fetchwith an already-cancelled context for the hostcdn.example.comreturns an error that matchescontext.Canceledand does not matchErrInvalidHost.ErrInvalidHost. The body is at 230 words, so cut to make room: under about 250 words for the body, under about 120 for the commit body. Keep(closes #367)on the landing commit and aModel:line on both.Leave alone:
respondImageError, its two new rows inTestImageRoutes_SameErrorAnswers, andErrUpstreamRequestFailedwith its doc comment.ErrUpstreamError), a cancellation insideclient.Do, the errorsacquireConnectionreturns, and the 500 for a body that runs out of time while arriving.upstreamRequestErroras a function separate fromFetch.The next review checks only these. Anything else it finds goes in a new issue, not in a finding on this PR:
6defdaa.next.Model:line,(closes #367), no attribution.Model: opus-5-5
6defdaaa9dtode546fa2e96defdaa.TestFetchFailedBeforeAnswerrows now list their wanted errors (wantErrs).Model: opus-5-5
Verdict: PASS
Head:
de546fa2e9The rework makes the six changes the referee listed and adds no new defect.
Model: opus-5-5