[Bugfix] Deadlock on invalid query to api/v2/search/tags

CHANGELOG.md

@@ -57,6 +57,7 @@ Additionally the `compaction_tenant_backoff_total` metric has been renamed to `c
 * [BUGFIX] Fix incorrect results in TraceQL compare() for spans with array attributes [#5519](https://github.com/grafana/tempo/pull/5519) (@ruslan-mikhailov)
 * [BUGFIX] Fix cache collision for incomplete query in SearchTagValuesV2 [#5549](https://github.com/grafana/tempo/pull/5549) (@ruslan-mikhailov)
 * [BUGFIX] Fix for structural operator with empty left-hand spanset [#5578](https://github.com/grafana/tempo/pull/5578) (@ruslan-mikhailov)
+* [BUGFIX] Deadlock on invalid query to api/v2/search/tags (SearchTagsV2) [#5607](https://github.com/grafana/tempo/pull/5607) (@ruslan-mikhailov)
 
 # v2.8.2
 

integration/e2e/api/api_test.go

@@ -102,6 +102,24 @@ func TestSearchTagsV2(t *testing.T) {
 				},
 			},
 		},
+		{
+			name:  "invalid query",
+			query: ` { a="test" } `,
+			scope: "none",
+			// same results as no filtering
+			expected: searchTagsV2Response{
+				Scopes: []ScopedTags{
+					{
+						Name: "span",
+						Tags: []string{firstBatch.SpanAttr, secondBatch.SpanAttr},
+					},
+					{
+						Name: "resource",
+						Tags: []string{firstBatch.resourceAttr, secondBatch.resourceAttr, "service.name"},
+					},
+				},
+			},
+		},
 		{
 			name:  "first batch - resource",
 			query: fmt.Sprintf(`{ name="%s" }`, firstBatch.name),

modules/ingester/instance_search.go

@@ -269,10 +269,12 @@ func (i *instance) SearchTagsV2(ctx context.Context, req *tempopb.SearchTagsRequ
 		}, fetcher)
 	}
 
-	i.headBlockMtx.RLock()
-	span.AddEvent("acquired headblock mtx")
-	err = searchBlock(ctx, i.headBlock, "headBlock")
-	i.headBlockMtx.RUnlock()
+	func() { // anon function to defer unlock right after search
+		i.headBlockMtx.RLock()
+		defer i.headBlockMtx.RUnlock()
+		span.AddEvent("acquired headblock mtx")
+		err = searchBlock(ctx, i.headBlock, "headBlock")
+	}()
 	if err != nil {
 		return nil, fmt.Errorf("unexpected error searching head block (%s): %w", i.headBlock.BlockMeta().BlockID, err)
 	}
@@ -544,6 +546,8 @@ func (i *instance) SearchTagValuesV2(ctx context.Context, req *tempopb.SearchTag
 				anyErr.Store(fmt.Errorf("unexpected error searching head block (%s): %w", i.headBlock.BlockMeta().BlockID, err))
 			}
 		}()
+	} else {
+		i.headBlockMtx.RUnlock()
 	}
 
 	i.blocksMtx.RLock()

pkg/traceql/engine.go

@@ -224,7 +224,9 @@ func (e *Engine) ExecuteTagNames(
 		Scope:      scope,
 	}
 
-	span.SetAttributes(attribute.String("pipeline", rootExpr.Pipeline.String()))
+	if rootExpr != nil {
+		span.SetAttributes(attribute.String("pipeline", rootExpr.Pipeline.String()))
+	}
 	span.SetAttributes(attribute.String("autocompleteReq", fmt.Sprint(autocompleteReq)))
 
 	return fetcher.Fetch(ctx, autocompleteReq, cb)

pkg/traceql/engine_test.go

@@ -562,6 +562,29 @@ func TestExamplesInEngine(t *testing.T) {
 	}
 }
 
+func TestExecuteTagNames_InvalidQuery(t *testing.T) {
+	e := NewEngine()
+
+	invalidQuery := "{ invalid syntax }"
+	err := e.ExecuteTagNames(
+		context.Background(),
+		AttributeScopeSpan,
+		invalidQuery,
+		func(string, AttributeScope) bool {
+			return false
+		},
+		&MockTagNamesFetcher{},
+	)
+
+	require.NoError(t, err)
+}
+
+type MockTagNamesFetcher struct{}
+
+func (m *MockTagNamesFetcher) Fetch(context.Context, FetchTagsRequest, FetchTagsCallback) error {
+	return nil
+}
+
 func TestExecuteTagValues(t *testing.T) {
 	// TODO: This test is stupid, it's using the traceql engine to execute the query
 	//  and doesn't actually test the ExecuteTagValues function