fix(handlers): return 499 on client cancel for get-server endpoints

[piyushbag]

Summary

• Extend the fix: client-cancelled GET /v0/servers returns 499 without error log #1335 ListServersError pattern to the get-server-version and get-server-versions handlers
• Map client disconnects to HTTP 499 without error-level logging or a second WriteHeader
• Factor shared cancellation detection into clientClosedRequest and add unit tests for the new helpers

Fixes #1323 (handler-side scope; CDN/RPS follow-up tracked separately on the issue)

Problem

#1335 fixed cancellation handling for GET /v0/servers only. rdimitrov noted that PR only partially addressed #1323. The get-server-version and get-server-versions handlers still logged benign cancellations at error level and returned 500, which can trigger huma superfluous response.WriteHeader warnings.

Test plan

• go test ./internal/api/handlers/v0/... -run 'ListServersError|GetServerDetailsError|GetServerVersionsError'
• go test -race ./internal/api/handlers/v0/... -run 'ListServersError|GetServerDetailsError|GetServerVersionsError'
• go vet ./internal/api/handlers/v0/...

[piyushbag]

Rebased onto current main and fixed the revive error-return lint by returning (bool, error) from clientClosedRequest.

Local: package builds.

[UgaTheDev]

Went through this one with the tests actually running plus a few probes against the head commit (7fe3483, base 3fe10fc). The four new tests pass, and the mechanism holds up better than most attempts at this — a few notes below, none of them blocking.

Things I checked that came out clean:

• It correctly keeps server-side timeouts as 5xx. This is the part that's usually wrong. context.DeadlineExceeded returns 500 both when it's the error value and when it's the context's own error, so a deadline we set ourselves still counts as a server error. The errors.Is(..., context.Canceled) narrowing (rather than a blanket ctx.Err() != nil) is what buys that, and it matches the reasoning already written down in router.go.
• errors.Is survives the database layer. I fed it fmt.Errorf("failed to query server versions: %w", fmt.Errorf("error iterating rows: %w", context.Canceled)) with a live context and got 499. Since postgres.go wraps with %w at every return, that's the shape this will actually see in production.
• No superfluous-WriteHeader problem. I ran an httptest server with a handler that blocks on <-ctx.Done() and had a real client cancel mid-request. The client saw context canceled and the captured server log was empty — huma buffers the response until the handler returns, so there's no write to a closed socket and no http: superfluous response.WriteHeader noise.
• 404 precedence is preserved. The ErrNotFound check still runs ahead of the cancel check in both handlers, so a genuine not-found under a canceled context is still a 404. Easy to get backwards; you didn't.

One thing worth flagging:

The 499 body serializes the underlying error, including anything sensitive in it. clientClosedRequest passes err as the third argument to huma.NewError, and huma renders extra args into the response:

{"status":499,"detail":"Client closed request",
 "errors":[{"message":"error iterating rows: dsn=postgres://user:pw@internal-host/db: context canceled"}]}

That's from a real HTTP round trip, not a unit test. The 500 path four lines up deliberately does the opposite, with a comment saying why:

// Do not pass err here: huma serializes extra error args into the response

and TestListServersError_realFailureDoesNotLeakDetail asserts it. So the helper ends up inconsistent with the convention this same file documents.

To be fair about severity: I don't think it's reachable with a live connection today. grep -rn "WithCancel\|errgroup" internal/service/ internal/database/ comes back empty, so the only source of context.Canceled in the read path is the request context — which means the client has already hung up and never sees the body. It's latent rather than exploitable. But the fix is deleting one argument, and since this helper just went from one call site to three and is the obvious place the next handler will reach for, it seems worth closing here rather than leaving a trap:

return true, huma.NewError(499, "Client closed request")

Two smaller things:

• All four new tests only exercise one of the two branches. Each one cancels the context before calling, so they all pass via errors.Is(ctx.Err(), context.Canceled). The errors.Is(err, context.Canceled) branch — the one that matters for a wrapped error arriving from pgx — isn't covered by any test, new or pre-existing. A single case with context.Background() and a %w-wrapped context.Canceled covers it; I ran that shape and it passes today, so it's purely locking in behavior you already have.
• Four sibling sites still map cancel to 500: status.go:126, status.go:233, status.go:249, and edit.go:78. They read through the same service methods and can see the same canceled context. Clearly outside "get-server endpoints" as scoped, so a follow-up rather than a change request — just noting it so it doesn't get lost now that the helper exists.

One piece of context in case it affects how you weigh this: router.go:90 already remaps a canceled request context to 499 for metrics purposes and skips the error counter for it, independently of what status the handler returns. So the metrics side is arguably already handled on main for these endpoints, and what this PR adds on top is the honest status code to the client and the removal of misleading get server details ... failed 500 log lines. Both are real wins, I just wouldn't want the metrics argument to be the one carrying it.

For completeness on test results: go test ./internal/api/handlers/v0/... -run Error -v is 7/7 green. TestPrometheusHandler in the same package fails for me, but it wants a local Postgres and fails identically on the base commit, so it's unrelated.

Nice, tightly scoped change overall — the deliberate DeadlineExceeded carve-out and the preserved 404 ordering are the details that make this trustworthy. Happy to see it go in with just the NewError argument dropped; the rest is follow-up material.