mirror of
https://github.com/gotenberg/gotenberg.git
synced 2026-08-17 04:32:15 +01:00
fix(api): classify client-cancelled requests as 499 instead of 500
This commit is contained in:
@@ -86,13 +86,37 @@ func ParseError(err error) (int, string) {
|
|||||||
return http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError)
|
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,
|
// httpErrorHandler is the centralized HTTP error handler. It parses the error,
|
||||||
// returns a response as "text/plain; charset=UTF-8".
|
// returns a response as "text/plain; charset=UTF-8".
|
||||||
func httpErrorHandler() echo.HTTPErrorHandler {
|
func httpErrorHandler() echo.HTTPErrorHandler {
|
||||||
return func(err error, c echo.Context) {
|
return func(err error, c echo.Context) {
|
||||||
logger := c.Get("logger").(*slog.Logger)
|
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)
|
c.Response().Header().Add(echo.HeaderContentType, echo.MIMETextPlainCharsetUTF8)
|
||||||
|
|
||||||
err = c.String(status, message)
|
err = c.String(status, message)
|
||||||
@@ -263,9 +287,15 @@ func telemetryMiddleware(logger *slog.Logger, serverName, correlationIdHeader st
|
|||||||
finishTime := time.Now()
|
finishTime := time.Now()
|
||||||
|
|
||||||
status := c.Response().Status
|
status := c.Response().Status
|
||||||
|
canceled := false
|
||||||
if err != nil {
|
if err != nil {
|
||||||
parsedStatus, _ := ParseError(err)
|
canceled = requestCanceled(c, err)
|
||||||
status = parsedStatus
|
if canceled {
|
||||||
|
status = statusClientClosedRequest
|
||||||
|
} else {
|
||||||
|
parsedStatus, _ := ParseError(err)
|
||||||
|
status = parsedStatus
|
||||||
|
}
|
||||||
|
|
||||||
span.SetAttributes(attribute.String("error", err.Error()))
|
span.SetAttributes(attribute.String("error", err.Error()))
|
||||||
c.Error(err)
|
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_in", c.Request().ContentLength)).
|
||||||
With(slog.Int64("bytes_out", c.Response().Size))
|
With(slog.Int64("bytes_out", c.Response().Size))
|
||||||
|
|
||||||
if err != nil {
|
switch {
|
||||||
accessLogger.ErrorContext(ctx, err.Error())
|
case err == nil:
|
||||||
} else {
|
|
||||||
accessLogger.InfoContext(ctx, "request handled")
|
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{
|
additionalAttributes := []attribute.KeyValue{
|
||||||
|
|||||||
@@ -1,6 +1,10 @@
|
|||||||
package api
|
package api
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"context"
|
||||||
|
"errors"
|
||||||
|
"fmt"
|
||||||
|
"log/slog"
|
||||||
"net/http"
|
"net/http"
|
||||||
"net/http/httptest"
|
"net/http/httptest"
|
||||||
"strings"
|
"strings"
|
||||||
@@ -10,6 +14,70 @@ import (
|
|||||||
"github.com/labstack/echo/v4"
|
"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
|
// TestOutputFilenameMiddleware pins the sanitizing of the
|
||||||
// "Gotenberg-Output-Filename" header. The value reaches archive entry names and
|
// "Gotenberg-Output-Filename" header. The value reaches archive entry names and
|
||||||
// a Content-Disposition header, so a path separator must never survive it.
|
// a Content-Disposition header, so a path separator must never survive it.
|
||||||
|
|||||||
Reference in New Issue
Block a user