diff --git a/changes/45855-icon-not-found-client-error b/changes/45855-icon-not-found-client-error new file mode 100644 index 0000000000..e37c98f37c --- /dev/null +++ b/changes/45855-icon-not-found-client-error @@ -0,0 +1 @@ +* Fixed software title icon not-found errors (and other 4xx errors) being reported as server-side exceptions in OTEL traces, APM, Sentry, and the Redis-backed debug errors endpoint. diff --git a/server/contexts/ctxerr/ctxerr.go b/server/contexts/ctxerr/ctxerr.go index 9d7c69630d..5c9688c533 100644 --- a/server/contexts/ctxerr/ctxerr.go +++ b/server/contexts/ctxerr/ctxerr.go @@ -379,10 +379,10 @@ func Handle(ctx context.Context, err error) { sentry.CaptureException(cause) } } - } - if eh := FromContext(ctx); eh != nil { - eh.Store(ferr) + if eh := FromContext(ctx); eh != nil { + eh.Store(ferr) + } } } @@ -412,6 +412,23 @@ func isClientError(err error) bool { return clientErr.IsClientError() } + // Mirror the 4xx mappings EncodeError uses at the transport layer so + // these errors are not telemetered as server errors. + var nfErr platform_errors.NotFoundError + if errors.As(err, &nfErr) && nfErr.IsNotFound() { + return true + } + type isExister interface{ IsExists() bool } + var existsErr isExister + if errors.As(err, &existsErr) && existsErr.IsExists() { + return true + } + type isConflicter interface{ IsConflict() bool } + var conflictErr isConflicter + if errors.As(err, &conflictErr) && conflictErr.IsConflict() { + return true + } + // Check for errors with an explicit HTTP status code in the 4xx range type statusCoder interface{ StatusCode() int } var sc statusCoder diff --git a/server/contexts/ctxerr/ctxerr_test.go b/server/contexts/ctxerr/ctxerr_test.go index fb59694b02..03cfeb6e14 100644 --- a/server/contexts/ctxerr/ctxerr_test.go +++ b/server/contexts/ctxerr/ctxerr_test.go @@ -316,6 +316,44 @@ func TestHandle(t *testing.T) { ctx = NewContext(ctx, eh) Handle(ctx, err) }) + + t.Run("does not store client errors in the error handler", func(t *testing.T) { + cases := []struct { + name string + err error + }{ + {"NotFound", Wrap(context.Background(), &mockNotFoundError{notFound: true}, "wrap")}, + {"Exists", Wrap(context.Background(), &mockExistsError{exists: true}, "wrap")}, + {"Conflict", Wrap(context.Background(), &mockConflictError{conflict: true}, "wrap")}, + {"IsClientError", Wrap(context.Background(), &mockClientError{isClient: true}, "wrap")}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + eh := MockHandler{} + eh.StoreImpl = func(serr error) { + t.Fatalf("Store should not be called for client errors, got: %v", serr) + } + ctx = NewContext(ctx, eh) + Handle(ctx, tc.err) + }) + } + }) + + t.Run("stores server errors in the error handler", func(t *testing.T) { + ctx := context.Background() + eh := MockHandler{} + err := Wrap(ctx, errors.New("boom"), "wrap") + called := false + eh.StoreImpl = func(serr error) { + called = true + var ferr *FleetError + require.ErrorAs(t, serr, &ferr) + } + ctx = NewContext(ctx, eh) + Handle(ctx, err) + require.True(t, called, "Store must be called for server errors") + }) } func TestAdditionalMetadata(t *testing.T) { @@ -400,6 +438,24 @@ type mockStatusCoder struct{ code int } func (e *mockStatusCoder) Error() string { return "status error" } func (e *mockStatusCoder) StatusCode() int { return e.code } +// mockNotFoundError implements IsNotFound() for testing. +type mockNotFoundError struct{ notFound bool } + +func (e *mockNotFoundError) Error() string { return "not found" } +func (e *mockNotFoundError) IsNotFound() bool { return e.notFound } + +// mockExistsError implements IsExists() for testing. +type mockExistsError struct{ exists bool } + +func (e *mockExistsError) Error() string { return "already exists" } +func (e *mockExistsError) IsExists() bool { return e.exists } + +// mockConflictError implements IsConflict() for testing. +type mockConflictError struct{ conflict bool } + +func (e *mockConflictError) Error() string { return "conflict" } +func (e *mockConflictError) IsConflict() bool { return e.conflict } + func TestIsClientError(t *testing.T) { tests := []struct { name string @@ -456,6 +512,41 @@ func TestIsClientError(t *testing.T) { err: &mockStatusCoder{code: 500}, expected: false, }, + { + name: "IsNotFound (no IsClientError)", + err: &mockNotFoundError{notFound: true}, + expected: true, + }, + { + name: "IsNotFound wrapped", + err: fmt.Errorf("wrap: %w", &mockNotFoundError{notFound: true}), + expected: true, + }, + { + name: "IsNotFound returning false is not a client error", + err: &mockNotFoundError{notFound: false}, + expected: false, + }, + { + name: "IsExists (no IsClientError)", + err: &mockExistsError{exists: true}, + expected: true, + }, + { + name: "IsExists wrapped", + err: fmt.Errorf("wrap: %w", &mockExistsError{exists: true}), + expected: true, + }, + { + name: "IsConflict (no IsClientError)", + err: &mockConflictError{conflict: true}, + expected: true, + }, + { + name: "IsConflict wrapped", + err: fmt.Errorf("wrap: %w", &mockConflictError{conflict: true}), + expected: true, + }, } for _, tt := range tests {