Fix deadlock due to infinite retry (#2174)

* Fix deadlock due to infinite retry

* changelog

Author: thampiotr

CHANGELOG.md

@@ -14,7 +14,7 @@ v1.5.1
 
 - Fixed issue with reloading configuration and prometheus metrics duplication in `prometheus.write.queue`. (@mattdurham)
 
-- Fixed a race condition that could lead to a deadlock when using `import` statements, which could lead to a memory leak on `/metrics` endpoint of an Alloy instance. (@thampiotr) 
+- Fixed a few race conditions that could lead to a deadlock when using `import` statements, which could lead to a memory leak on `/metrics` endpoint of an Alloy instance. (@thampiotr)
 
 - Fix a race condition where the ui service was dependent on starting after the remotecfg service, which is not guaranteed. (@dehaansa & @erikbaranowski)
 

internal/runtime/internal/controller/loader.go

@@ -10,6 +10,12 @@ import (
 	"time"
 
 	"github.com/go-kit/log"
+	"github.com/grafana/dskit/backoff"
+	"github.com/hashicorp/go-multierror"
+	"go.opentelemetry.io/otel/attribute"
+	"go.opentelemetry.io/otel/codes"
+	"go.opentelemetry.io/otel/trace"
+
 	"github.com/grafana/alloy/internal/featuregate"
 	"github.com/grafana/alloy/internal/runtime/internal/dag"
 	"github.com/grafana/alloy/internal/runtime/internal/worker"
@@ -19,11 +25,6 @@ import (
 	"github.com/grafana/alloy/syntax/ast"
 	"github.com/grafana/alloy/syntax/diag"
 	"github.com/grafana/alloy/syntax/vm"
-	"github.com/grafana/dskit/backoff"
-	"github.com/hashicorp/go-multierror"
-	"go.opentelemetry.io/otel/attribute"
-	"go.opentelemetry.io/otel/codes"
-	"go.opentelemetry.io/otel/trace"
 )
 
 // The Loader builds and evaluates ComponentNodes from Alloy blocks.
@@ -92,10 +93,11 @@ func NewLoader(opts LoaderOptions) *Loader {
 		componentNodeManager: NewComponentNodeManager(globals, reg),
 
 		// This is a reasonable default which should work for most cases. If a component is completely stuck, we would
-		// retry and log an error every 10 seconds, at most.
+		// retry and log an error every 10 seconds, at most. We give up after some time to prevent lasting deadlocks.
 		backoffConfig: backoff.Config{
 			MinBackoff: 1 * time.Millisecond,
 			MaxBackoff: 10 * time.Second,
+			MaxRetries: 20, // Give up after 20 attempts - it could be a deadlock instead of an overload.
 		},
 
 		graph: &dag.Graph{},
@@ -736,19 +738,31 @@ func (l *Loader) EvaluateDependants(ctx context.Context, updatedNodes []*QueuedN
 				l.concurrentEvalFn(nodeRef, dependantCtx, tracer, parentRef)
 			})
 			if err != nil {
-				level.Error(l.log).Log(
-					"msg", "failed to submit node for evaluation - Alloy is likely overloaded "+
-						"and cannot keep up with evaluating components - will retry",
+				level.Warn(l.log).Log(
+					"msg", "failed to submit node for evaluation - will retry",
 					"err", err,
 					"node_id", n.NodeID(),
 					"originator_id", parent.Node.NodeID(),
 					"retries", retryBackoff.NumRetries(),
 				)
+				// When backing off, release the mut in case the evaluation requires to interact with the loader itself.
+				l.mut.RUnlock()
 				retryBackoff.Wait()
+				l.mut.RLock()
 			} else {
 				break
 			}
 		}
+		if err != nil && !retryBackoff.Ongoing() {
+			level.Error(l.log).Log(
+				"msg", "retry attempts exhausted when submitting node for evaluation to the worker pool - "+
+					"this could be a deadlock, performance bottleneck or severe overload leading to goroutine starvation",
+				"err", err,
+				"node_id", n.NodeID(),
+				"originator_id", parent.Node.NodeID(),
+				"retries", retryBackoff.NumRetries(),
+			)
+		}
 		span.SetAttributes(attribute.Int("retries", retryBackoff.NumRetries()))
 		if err != nil {
 			span.SetStatus(codes.Error, err.Error())