From 13c0621b0cadf729289229fe4b2c8b5ceaf3779c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Manso?= Date: Fri, 7 Aug 2026 19:05:51 +0100 Subject: [PATCH] fix(task): stop reporting abandoned requests as server errors (#2384) --- .../backend/internal/common/httpmw/logging.go | 8 +++- apps/backend/internal/task/handlers/errors.go | 27 ++++++++++++++ .../internal/task/handlers/errors_test.go | 37 +++++++++++++++++++ 3 files changed, 71 insertions(+), 1 deletion(-) diff --git a/apps/backend/internal/common/httpmw/logging.go b/apps/backend/internal/common/httpmw/logging.go index e905d3ed41..1d3fddfddf 100644 --- a/apps/backend/internal/common/httpmw/logging.go +++ b/apps/backend/internal/common/httpmw/logging.go @@ -8,6 +8,10 @@ import ( "go.uber.org/zap" ) +// statusClientClosedRequest is nginx's 499, returned by handlers when the +// caller abandoned the request. Go defines no constant for it. +const statusClientClosedRequest = 499 + // RequestLogger logs HTTP request details after the handler completes. func RequestLogger(log *logger.Logger, serverName string) gin.HandlerFunc { return func(c *gin.Context) { @@ -35,7 +39,9 @@ func RequestLogger(log *logger.Logger, serverName string) gin.HandlerFunc { zap.Int("bytes", size), } switch { - case status >= 400 && status != 404: + // 404 and 499 are routine: one is a miss, the other is the caller + // hanging up mid-request. Neither means something went wrong here. + case status >= 400 && status != 404 && status != statusClientClosedRequest: log.Warn("http", fields...) default: log.Debug("http", fields...) diff --git a/apps/backend/internal/task/handlers/errors.go b/apps/backend/internal/task/handlers/errors.go index ab99daf830..2d8ea9367c 100644 --- a/apps/backend/internal/task/handlers/errors.go +++ b/apps/backend/internal/task/handlers/errors.go @@ -1,6 +1,7 @@ package handlers import ( + "context" "errors" "net/http" "strings" @@ -12,7 +13,15 @@ import ( "go.uber.org/zap" ) +// statusClientClosedRequest is nginx's 499. Go defines no constant for it and +// no standard 4xx says "the client hung up", which is what actually happened. +const statusClientClosedRequest = 499 + func handleNotFound(c *gin.Context, log *logger.Logger, err error, fallback string) { + if isClientDisconnect(err) { + abortClientDisconnect(c) + return + } if isNotFound(err) { c.JSON(http.StatusNotFound, gin.H{"error": fallback}) return @@ -31,6 +40,8 @@ func handleNotFound(c *gin.Context, log *logger.Logger, err error, fallback stri func handleSelectedMoveError(c *gin.Context, log *logger.Logger, err error) { switch { + case isClientDisconnect(err): + abortClientDisconnect(c) case isNotFound(err): c.JSON(http.StatusNotFound, gin.H{"error": "task or workflow not found"}) case isMoveConflict(err): @@ -43,6 +54,22 @@ func handleSelectedMoveError(c *gin.Context, log *logger.Logger, err error) { } } +// isClientDisconnect reports a request the caller abandoned. Handlers derive +// their context from c.Request.Context(), which the server cancels when the +// peer goes away, so context.Canceled here means the browser navigated, +// unmounted a component, or aborted an in-flight fetch. Nothing failed +// server-side and nobody is left to read a response. +// +// DeadlineExceeded is deliberately not included: that is our own timeout +// firing, and it stays a logged 500. +func isClientDisconnect(err error) bool { + return err != nil && errors.Is(err, context.Canceled) && !errors.Is(err, context.DeadlineExceeded) +} + +func abortClientDisconnect(c *gin.Context) { + c.AbortWithStatus(statusClientClosedRequest) +} + func isNotFound(err error) bool { if err == nil { return false diff --git a/apps/backend/internal/task/handlers/errors_test.go b/apps/backend/internal/task/handlers/errors_test.go index 7207b4fa04..2a87f76255 100644 --- a/apps/backend/internal/task/handlers/errors_test.go +++ b/apps/backend/internal/task/handlers/errors_test.go @@ -47,6 +47,43 @@ func TestErrorsAreClassifiable(t *testing.T) { } }) + // A browser that navigates away or unmounts a component aborts its + // in-flight fetches. That reaches handlers as a cancelled request context, + // which used to fall through to the 500 branch and log an ERROR with a + // full stacktrace for a response nobody was waiting on any more. + t.Run("client disconnect is not a server error", func(t *testing.T) { + gin.SetMode(gin.TestMode) + handlers := map[string]func(*gin.Context, error){ + "handleNotFound": func(ctx *gin.Context, err error) { + handleNotFound(ctx, newTestLogger(t), err, "task sessions not found") + }, + "handleSelectedMoveError": func(ctx *gin.Context, err error) { + handleSelectedMoveError(ctx, newTestLogger(t), err) + }, + } + for name, handle := range handlers { + t.Run(name, func(t *testing.T) { + rec := httptest.NewRecorder() + ctx, _ := gin.CreateTestContext(rec) + handle(ctx, fmt.Errorf("list task sessions: %w", context.Canceled)) + if rec.Code != statusClientClosedRequest { + t.Fatalf("status=%d, want %d", rec.Code, statusClientClosedRequest) + } + }) + } + }) + + // Our own deadline firing is a real failure and must keep its 500. + t.Run("server timeout stays a server error", func(t *testing.T) { + gin.SetMode(gin.TestMode) + rec := httptest.NewRecorder() + ctx, _ := gin.CreateTestContext(rec) + handleNotFound(ctx, newTestLogger(t), fmt.Errorf("query: %w", context.DeadlineExceeded), "task not found") + if rec.Code != http.StatusInternalServerError { + t.Fatalf("status=%d, want %d", rec.Code, http.StatusInternalServerError) + } + }) + t.Run("WIP limit is a conflict for task creation", func(t *testing.T) { gin.SetMode(gin.TestMode) rec := httptest.NewRecorder()