Distributor push logs and metrics record 500 instead of 499 on client cancellation

[friedrichg]

AI Tool Usage Notice
This report was drafted with AI assistance (code investigation and root-cause tracing across multiple Cortex versions). It has been reviewed and validated before submission.

Describe the bug
The distributor's remote-write push path handles a client-canceled request (client disconnect, or a proxy/load balancer enforcing a timeout) as a server error: it logs at error level and its request-status metrics record 500, instead of 499 (nginx's "Client Closed Request" convention). Note that since the client (or fronting proxy) has already disconnected by the time this happens, the 500 set via http.Error(w, ...) is rarely delivered to anyone — the actual damage is that the distributor's access-log line and status-code metrics are stamped 500, making normal client-side cancellations indistinguishable from real server errors in dashboards and alerting.

Example log line:

POST /api/v1/push (500) 1.5s Response: "context canceled\n" ...

Root cause:

• pkg/distributor/distributor.go: Push (via doBatch → ring.DoBatch → per-ingester send) returns the raw context.Canceled error unwrapped when the client cancels the request context. Other error paths in the same function explicitly wrap errors via httpgrpc.Errorf(code, ...) (e.g. 429 for rate limits, 400 for HA/validation errors, 202 for dedup), but this path does not.
• pkg/util/push/push.go: the HTTP handler calls httpgrpc.HTTPResponseFromError(err) to convert the distributor's error into an HTTP response. Since the bare context.Canceled was never wrapped via httpgrpc.Errorf, it doesn't carry a gRPC status, so ok == false and the handler falls through to the generic fallback: http.Error(w, err.Error(), http.StatusInternalServerError).

For comparison, the query-frontend already handles this correctly, and has since v0.7.0 (commit baae166e33, "Frontend: do not return 500 if the user request cancellation", #2156): pkg/frontend/transport/handler.go defines StatusClientClosedRequest = 499 and explicitly checks context.Canceled, returning 499 instead of 500. This fix was never ported to the distributor's write path. Confirmed this gap exists across all checked versions from v0.7.0 through v1.21.0, and is still present on the current master branch (checked at commit d8815ef22f9352a9ab586b4e3bed9814665cfd90) as of this writing, so it is a long-standing, still-unfixed asymmetry rather than a recent regression.

To Reproduce
Steps to reproduce the behavior:

1. Start a Cortex distributor (any version, including current master)
2. Send a POST /api/v1/push remote-write request and cancel it client-side (or have a proxy in front of the distributor time out) while the distributor is still forwarding the write to ingesters
3. Observe the distributor's access log / response status

Expected behavior
A client-canceled push request should be logged and metriced as HTTP 499, matching the convention already used by the query-frontend, so it is not counted as a server-side error.

Environment:

• Infrastructure: N/A (code-level issue, reproducible in any deployment)
• Deployment tool: N/A

Additional Context
Proposed fix: in pkg/distributor/distributor.go, after doBatch returns an error (or in pkg/util/push/push.go before the generic 500 fallback), add:

if errors.Is(err, context.Canceled) {
    return nil, httpgrpc.Errorf(StatusClientClosedRequest, "%s", err.Error())
}

reusing the existing StatusClientClosedRequest = 499 constant (or the shared one in pkg/util/api/response.go) for consistency with the frontend's handling.

Impact: client-canceled push requests are currently logged and metriced as server-side 5xx errors, which can trigger false-positive SLO/error-rate alerts and pollute error dashboards for the distributor, even though no response is actually delivered to any listener.

[AllenYTF]

I'm working on it.