Skip to content

Commit 9d59e0d

Browse files
authored
fix: stop leaking errors over the operator (#5212)
1 parent 9cce0f4 commit 9d59e0d

4 files changed

Lines changed: 42 additions & 22 deletions

File tree

‎pkg/operator/dagoperator/dag.go‎

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ func isDagConditionEvalErr(err error) bool {
6969
}
7070

7171
type dagEngineError struct {
72-
errorType string
72+
errorType v1contracts.DurableTaskErrorType
7373
errorMessage string
7474
}
7575

@@ -82,17 +82,30 @@ func isDagEngineErr(err error) bool {
8282
return errors.As(err, &e)
8383
}
8484

85-
// these errors can be returned to the client verbatim. everything else is an
86-
// internal failure whose details stay in the engine logs.
87-
func isUserFacingDagErr(err error) bool {
88-
var nonDeterminismErr *repository.NonDeterminismError
85+
func clientErrorMessageForDagErr(err error) (message string, ok bool) {
86+
var childFailedErr *dagChildFailedError
87+
var cancelledErr *dagCancelledError
88+
var conditionEvalErr *dagConditionEvalError
8989
var celFailureErr *operator.DAGStepCELFailureError
90-
91-
return isDagChildFailedErr(err) ||
92-
isDagConditionEvalErr(err) ||
93-
isDagEngineErr(err) ||
94-
errors.As(err, &celFailureErr) ||
95-
errors.As(err, &nonDeterminismErr)
90+
var nonDeterminismErr *repository.NonDeterminismError
91+
var engineErr *dagEngineError
92+
93+
switch {
94+
case errors.As(err, &childFailedErr):
95+
return childFailedErr.Error(), true
96+
case errors.As(err, &cancelledErr):
97+
return cancelledErr.Error(), true
98+
case errors.As(err, &conditionEvalErr):
99+
return conditionEvalErr.Error(), true
100+
case errors.As(err, &celFailureErr):
101+
return celFailureErr.Error(), true
102+
case errors.As(err, &nonDeterminismErr):
103+
return nonDeterminismErr.Error(), true
104+
case errors.As(err, &engineErr) && engineErr.errorType == v1contracts.DurableTaskErrorType_DURABLE_TASK_ERROR_TYPE_NONDETERMINISM:
105+
return engineErr.errorMessage, true
106+
default:
107+
return "", false
108+
}
96109
}
97110

98111
func isDagNonRetryablePgErr(err error) bool {
@@ -678,7 +691,7 @@ func (d *dag) taskConsumer(ctx context.Context, resp *v1contracts.DurableTaskRes
678691

679692
// Dropping this would leave the run waiting forever for an ack or completion that
680693
// will never arrive (e.g. a nondeterminism error means the wait was never committed).
681-
d.err = &dagEngineError{errorType: m.Error.GetErrorType().String(), errorMessage: m.Error.GetErrorMessage()}
694+
d.err = &dagEngineError{errorType: m.Error.GetErrorType(), errorMessage: m.Error.GetErrorMessage()}
682695
}
683696
}
684697

‎pkg/operator/dagoperator/operator.go‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -407,8 +407,9 @@ func (d *DAGOperator) run(action *contracts.AssignedAction) error {
407407

408408
return nil
409409
}
410-
if isDagCancelledErr(dagErr) {
411-
return d.cancelDAG(span, action, dagErr.Error())
410+
var cancelledErr *dagCancelledError
411+
if errors.As(dagErr, &cancelledErr) {
412+
return d.cancelDAG(span, action, cancelledErr.Error())
412413
}
413414
if errors.Is(dagErr, context.Canceled) {
414415
return d.handleRunCancellation(span, action, tasks, dagErr)
@@ -554,22 +555,26 @@ func (d *DAGOperator) handleRunCancellation(span trace.Span, action *contracts.A
554555
}
555556
}
556557

557-
return d.cancelDAG(span, action, err.Error())
558+
d.Logger().Debug().Err(err).
559+
Str("task_run_external_id", action.TaskRunExternalId).
560+
Msg("dag run cancelled")
561+
562+
return d.cancelDAG(span, action, repository.TaskCancelledErrorMessage)
558563
}
559564

560565
func (d *DAGOperator) fail(span trace.Span, action *contracts.AssignedAction, err error, shouldNotRetry bool) error {
561566
span.RecordError(err)
562567
span.SetStatus(telemetry_codes.Error, "dag run failed")
563568
span.SetAttributes(attribute.Bool("dagoperator.should_not_retry", shouldNotRetry))
564569

565-
clientErrorMessage := err.Error()
570+
clientErrorMessage := fmt.Sprintf("internal error while orchestrating the dag run (task run %s)", action.TaskRunExternalId)
566571

567-
if !isUserFacingDagErr(err) {
572+
if userFacingMessage, ok := clientErrorMessageForDagErr(err); ok {
573+
clientErrorMessage = "dag failed: " + userFacingMessage
574+
} else {
568575
d.Logger().Error().Err(err).
569576
Str("task_run_external_id", action.TaskRunExternalId).
570577
Msg("dag orchestration failed with an internal error")
571-
572-
clientErrorMessage = fmt.Sprintf("internal error while orchestrating the dag run (task run %s)", action.TaskRunExternalId)
573578
}
574579

575580
if reportErr := d.SendFailed(action, clientErrorMessage, shouldNotRetry); reportErr != nil {

‎pkg/operator/hostgrpc/host_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -752,7 +752,7 @@ func TestOpenDeliversActionsToHandler(t *testing.T) {
752752
ev := session.sentEvents()[0]
753753
assert.Equal(t, contracts.StepActionEventType_STEP_EVENT_TYPE_FAILED, ev.EventType)
754754
assert.Equal(t, "run-2", ev.TaskRunExternalId)
755-
assert.Equal(t, "no capacity", ev.EventPayload)
755+
assert.Equal(t, refusedActionClientErrorMessage, ev.EventPayload, "the handler's error must not reach the client")
756756
assert.Nil(t, ev.ShouldNotRetry)
757757
require.NotNil(t, ev.RetryCount)
758758
assert.Equal(t, int32(2), *ev.RetryCount)
@@ -1207,7 +1207,7 @@ func TestCancelsBypassBlockedStarts(t *testing.T) {
12071207
refused := fs.sentEvents()[0]
12081208
assert.Equal(t, "run-3", refused.TaskRunExternalId)
12091209
assert.Equal(t, contracts.StepActionEventType_STEP_EVENT_TYPE_FAILED, refused.EventType)
1210-
assert.Contains(t, refused.EventPayload, "start queue")
1210+
assert.Equal(t, refusedActionClientErrorMessage, refused.EventPayload)
12111211
assert.Nil(t, refused.ShouldNotRetry)
12121212

12131213
assert.Len(t, handler.handled(), 2, "the queued start waits for the handler")

‎pkg/operator/hostgrpc/session.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ const (
2222
// refusalReportTimeout bounds the failure report for an action the handler refused.
2323
refusalReportTimeout = 30 * time.Second
2424

25+
refusedActionClientErrorMessage = "the operator could not accept the task; it will be retried according to its retry policy"
26+
2527
// minStartQueueSize is the least number of starts a session queues ahead of its handler.
2628
minStartQueueSize = 64
2729

@@ -281,7 +283,7 @@ func (s *session) refuse(action *contracts.AssignedAction, err error) {
281283
ActionId: action.ActionId,
282284
EventTimestamp: timestamppb.Now(),
283285
EventType: contracts.StepActionEventType_STEP_EVENT_TYPE_FAILED,
284-
EventPayload: err.Error(),
286+
EventPayload: refusedActionClientErrorMessage,
285287
RetryCount: &retryCount,
286288
}); reportErr != nil {
287289
s.l.Error().Err(reportErr).Str("worker_id", s.workerId()).Str("task_run_external_id", action.TaskRunExternalId).Msg("could not report the refused action")

0 commit comments

Comments
 (0)