Distinguish client and internal errors in action responses (#9162)

Annaseli · web-flow · commit 116b57898df9 · 2025-06-12T00:18:30.000+03:00
diff --git a/pkg/actions/action.go b/pkg/actions/action.go
@@ -66,9 +66,6 @@ type MatchSpec struct {
 var (
 	reName   = regexp.MustCompile(`^\w[\w\-. ]+$`)
 	reHookID = regexp.MustCompile(`^[_a-zA-Z][\-_a-zA-Z0-9]{1,255}$`)
-
-	ErrInvalidAction         = errors.New("invalid action")
-	ErrInvalidEventParameter = errors.New("invalid event parameter")
 )
 
 func isEventSupported(event graveler.EventType) bool {
diff --git a/pkg/actions/errors.go b/pkg/actions/errors.go
@@ -0,0 +1,14 @@
+package actions
+
+import "errors"
+
+var (
+	ErrActionFailed          = errors.New("action failed")
+	ErrNotFound              = errors.New("not found")
+	ErrNilValue              = errors.New("nil value")
+	ErrIfExprNotBool         = errors.New("hook 'if' expression should evaluate to a boolean")
+	ErrParamConflict         = errors.New("parameters conflict")
+	ErrUnknownHookType       = errors.New("unknown hook type")
+	ErrInvalidAction         = errors.New("invalid action")
+	ErrInvalidEventParameter = errors.New("invalid event parameter")
+)
diff --git a/pkg/actions/hook.go b/pkg/actions/hook.go
@@ -3,7 +3,6 @@ package actions
 import (
 	"bytes"
 	"context"
-	"errors"
 	"fmt"
 	"net/http"
 
@@ -39,8 +38,6 @@ var hooks = map[HookType]NewHookFunc{
 	HookTypeLua:     NewLuaHook,
 }
 
-var ErrUnknownHookType = errors.New("unknown hook type")
-
 func NewHook(hook ActionHook, action *Action, cfg Config, server *http.Server, serverAddress string, collector stats.Collector) (Hook, error) {
 	f := hooks[hook.Type]
 	if f == nil {
diff --git a/pkg/actions/kv_run_results_iterator.go b/pkg/actions/kv_run_results_iterator.go
@@ -2,14 +2,11 @@ package actions
 
 import (
 	"context"
-	"errors"
 	"fmt"
 
 	"github.com/treeverse/lakefs/pkg/kv"
 )
 
-var ErrParamConflict = errors.New("parameters conflict")
-
 type KVRunResultIterator struct {
 	it    kv.MessageIterator
 	err   error
diff --git a/pkg/actions/service.go b/pkg/actions/service.go
@@ -32,12 +32,6 @@ const (
 	commitPrefix = "commits"
 )
 
-var (
-	ErrNotFound      = errors.New("not found")
-	ErrNilValue      = errors.New("nil value")
-	ErrIfExprNotBool = errors.New("hook 'if' expression should evaluate to a boolean")
-)
-
 type Config struct {
 	Enabled bool
 	Lua     struct {
@@ -366,6 +360,10 @@ func (s *StoreService) runTasks(ctx context.Context, record graveler.HookRecord,
 				var buf bytes.Buffer
 				if task.Err == nil {
 					task.Err = task.Hook.Run(ctx, record, &buf)
+					if task.Err != nil {
+						// Wrap the error to later indicate that the precondition failed due to a client-side hook error.
+						task.Err = fmt.Errorf("%w: %s", ErrActionFailed, task.Err)
+					}
 				}
 				task.EndTime = time.Now().UTC()
 
diff --git a/pkg/api/controller.go b/pkg/api/controller.go
@@ -2934,14 +2934,6 @@ func (c *Controller) handleAPIErrorCallback(ctx context.Context, w http.Response
 
 	log := c.Logger.WithContext(ctx).WithError(err)
 
-	// Handle Hook Errors
-	var hookAbortErr *graveler.HookAbortError
-	if errors.As(err, &hookAbortErr) {
-		log.WithField("run_id", hookAbortErr.RunID).Warn("aborted by hooks")
-		cb(w, r, http.StatusPreconditionFailed, hookAbortErr.Unwrap())
-		return true
-	}
-
 	// order of case is important, more specific errors should be first
 	switch {
 	case errors.Is(err, graveler.ErrLinkAddressInvalid),
@@ -3027,6 +3019,9 @@ func (c *Controller) handleAPIErrorCallback(ctx context.Context, w http.Response
 	case errors.Is(err, authentication.ErrInsufficientPermissions):
 		c.Logger.WithContext(ctx).WithError(err).Info("User verification failed - insufficient permissions")
 		cb(w, r, http.StatusUnauthorized, http.StatusText(http.StatusUnauthorized))
+	case errors.Is(err, actions.ErrActionFailed):
+		log.WithError(err).Debug("Precondition failed, aborted by action failure")
+		cb(w, r, http.StatusPreconditionFailed, err)
 	default:
 		c.Logger.WithContext(ctx).WithError(err).Error("API call returned status internal server error")
 		cb(w, r, http.StatusInternalServerError, err)

Original file line number	Diff line number	Diff line change
`@@ -66,9 +66,6 @@ type MatchSpec struct {`
`66`	`66`	`var (`
`67`	`67`	reName = regexp.MustCompile(`^\w[\w\-. ]+$`)
`68`	`68`	reHookID = regexp.MustCompile(`^[_a-zA-Z][\-_a-zA-Z0-9]{1,255}$`)
`69`		`-`
`70`		`- ErrInvalidAction = errors.New("invalid action")`
`71`		`- ErrInvalidEventParameter = errors.New("invalid event parameter")`
`72`	`69`	`)`
`73`	`70`
`74`	`71`	`func isEventSupported(event graveler.EventType) bool {`