diff --git a/pkg/modules/api/middlewares.go b/pkg/modules/api/middlewares.go index 73b295c6..ea3d64ec 100644 --- a/pkg/modules/api/middlewares.go +++ b/pkg/modules/api/middlewares.go @@ -86,13 +86,37 @@ func ParseError(err error) (int, string) { return http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError) } +// statusClientClosedRequest is the non-standard 499 status (nginx convention) +// recorded when the client aborts the request before it completes. It keeps +// such outcomes out of the 5xx range in the access log and Prometheus metrics. +const statusClientClosedRequest = 499 + +// requestCanceled reports whether err is the result of the client aborting the +// request rather than a server-side failure. It requires both that err wraps +// [context.Canceled] and that the request context itself was canceled, so a +// context.Canceled originating elsewhere still surfaces as an internal error. +// A server-side timeout is [context.DeadlineExceeded], mapped to 503 by +// [ParseError], and is deliberately not treated as a client abort. +// See https://github.com/gotenberg/gotenberg/issues/1627. +func requestCanceled(c echo.Context, err error) bool { + return errors.Is(err, context.Canceled) && errors.Is(c.Request().Context().Err(), context.Canceled) +} + // httpErrorHandler is the centralized HTTP error handler. It parses the error, // returns a response as "text/plain; charset=UTF-8". func httpErrorHandler() echo.HTTPErrorHandler { return func(err error, c echo.Context) { logger := c.Get("logger").(*slog.Logger) - status, message := ParseError(err) + if requestCanceled(c, err) { + // The client is gone, so writing a body would only fail and add + // noise. Record the status so the access log and metrics classify + // it as a client abort rather than an internal error. + c.Response().WriteHeader(statusClientClosedRequest) + return + } + + status, message := ParseError(err) c.Response().Header().Add(echo.HeaderContentType, echo.MIMETextPlainCharsetUTF8) err = c.String(status, message) @@ -263,9 +287,15 @@ func telemetryMiddleware(logger *slog.Logger, serverName, correlationIdHeader st finishTime := time.Now() status := c.Response().Status + canceled := false if err != nil { - parsedStatus, _ := ParseError(err) - status = parsedStatus + canceled = requestCanceled(c, err) + if canceled { + status = statusClientClosedRequest + } else { + parsedStatus, _ := ParseError(err) + status = parsedStatus + } span.SetAttributes(attribute.String("error", err.Error())) c.Error(err) @@ -293,10 +323,15 @@ func telemetryMiddleware(logger *slog.Logger, serverName, correlationIdHeader st With(slog.Int64("bytes_in", c.Request().ContentLength)). With(slog.Int64("bytes_out", c.Response().Size)) - if err != nil { - accessLogger.ErrorContext(ctx, err.Error()) - } else { + switch { + case err == nil: accessLogger.InfoContext(ctx, "request handled") + case canceled: + // A client abort is expected, not a server failure; keep it + // visible but out of the error stream. + accessLogger.InfoContext(ctx, err.Error()) + default: + accessLogger.ErrorContext(ctx, err.Error()) } additionalAttributes := []attribute.KeyValue{ diff --git a/pkg/modules/api/middlewares_test.go b/pkg/modules/api/middlewares_test.go index b189c44e..f3ccad45 100644 --- a/pkg/modules/api/middlewares_test.go +++ b/pkg/modules/api/middlewares_test.go @@ -1,6 +1,10 @@ package api import ( + "context" + "errors" + "fmt" + "log/slog" "net/http" "net/http/httptest" "strings" @@ -10,6 +14,70 @@ import ( "github.com/labstack/echo/v4" ) +// TestRequestCanceled pins the client-abort discriminator: only a +// context.Canceled that stems from the request context counts, so a server +// timeout or an unrelated cancellation still surfaces as an internal failure. +// See https://github.com/gotenberg/gotenberg/issues/1627. +func TestRequestCanceled(t *testing.T) { + canceled, cancel := context.WithCancel(context.Background()) + cancel() + + timedOut, cancelTimeout := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + defer cancelTimeout() + + for _, tc := range []struct { + name string + reqCtx context.Context + err error + want bool + }{ + {"client abort", canceled, context.Canceled, true}, + {"wrapped client abort", canceled, fmt.Errorf("convert to PDF: %w", context.Canceled), true}, + {"canceled error but live request", context.Background(), context.Canceled, false}, + {"canceled request but unrelated error", canceled, errors.New("boom"), false}, + {"server timeout is not a client abort", timedOut, context.DeadlineExceeded, false}, + {"no error", canceled, nil, false}, + } { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(tc.reqCtx) + c := echo.New().NewContext(req, httptest.NewRecorder()) + if got := requestCanceled(c, tc.err); got != tc.want { + t.Fatalf("requestCanceled = %v, want %v", got, tc.want) + } + }) + } +} + +// TestHttpErrorHandler_ClientClosedRequest ensures a client abort is recorded +// as 499 rather than 500, and that a genuine failure keeps its status. +func TestHttpErrorHandler_ClientClosedRequest(t *testing.T) { + canceled, cancel := context.WithCancel(context.Background()) + cancel() + + for _, tc := range []struct { + name string + reqCtx context.Context + err error + wantStatus int + }{ + {"client abort", canceled, fmt.Errorf("convert to PDF: %w", context.Canceled), statusClientClosedRequest}, + {"internal failure", context.Background(), errors.New("boom"), http.StatusInternalServerError}, + } { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(tc.reqCtx) + rec := httptest.NewRecorder() + c := echo.New().NewContext(req, rec) + c.Set("logger", slog.New(slog.DiscardHandler)) + + httpErrorHandler()(tc.err, c) + + if rec.Code != tc.wantStatus { + t.Fatalf("status = %d, want %d", rec.Code, tc.wantStatus) + } + }) + } +} + // TestOutputFilenameMiddleware pins the sanitizing of the // "Gotenberg-Output-Filename" header. The value reaches archive entry names and // a Content-Disposition header, so a path separator must never survive it.