fixes

Author: kcons

src/sentry/workflow_engine/handlers/detector/__init__.py

@@ -4,8 +4,15 @@
     "DetectorHandler",
     "DetectorOccurrence",
     "DetectorStateData",
+    "GroupedDetectorEvaluationResult",
     "StatefulDetectorHandler",
 ]
 
-from .base import DataPacketEvaluationType, DataPacketType, DetectorHandler, DetectorOccurrence
+from .base import (
+    DataPacketEvaluationType,
+    DataPacketType,
+    DetectorHandler,
+    DetectorOccurrence,
+    GroupedDetectorEvaluationResult,
+)
 from .stateful import DetectorStateData, StatefulDetectorHandler

src/sentry/workflow_engine/models/workflow.py

@@ -19,7 +19,7 @@
 from sentry.workflow_engine.models.data_condition import DataCondition, is_slow_condition
 from sentry.workflow_engine.models.data_condition_group import DataConditionGroup
 from sentry.workflow_engine.processors.data_condition_group import TriggerResult
-from sentry.workflow_engine.types import WorkflowEventData
+from sentry.workflow_engine.types import ConditionError, WorkflowEventData
 
 from .json_config import JSONConfigBase
 
@@ -117,7 +117,7 @@ def evaluate_trigger_conditions(
                 "DataConditionGroup does not exist",
                 extra={"id": self.when_condition_group_id},
             )
-            return TriggerResult.FALSE, []
+            return TriggerResult(False, ConditionError(msg="DataConditionGroup does not exist")), []
         group_evaluation, remaining_conditions = process_data_condition_group(
             group, workflow_event_data, when_data_conditions
         )

src/sentry/workflow_engine/processors/data_condition_group.py

@@ -1,6 +1,6 @@
 import dataclasses
 import logging
-from collections.abc import Iterable
+from collections.abc import Callable, Iterable
 from typing import ClassVar, NoReturn, TypeVar
 
 import sentry_sdk
@@ -17,6 +17,13 @@
 T = TypeVar("T")
 
 
+def _find_error(
+    items: list["TriggerResult"], predicate: Callable[["TriggerResult"], bool]
+) -> ConditionError | None:
+    """Helper to find an error from items matching the predicate."""
+    return next((item.error for item in items if predicate(item)), None)
+
+
 @dataclasses.dataclass(frozen=True)
 class TriggerResult:
     """
@@ -39,36 +46,107 @@ class TriggerResult:
     TRUE: ClassVar["TriggerResult"]
     FALSE: ClassVar["TriggerResult"]
 
+    def is_tainted(self) -> bool:
+        """
+        Returns True if this result is less trustworthy due to an error during
+        evaluation.
+        """
+        return self.error is not None
+
     @staticmethod
     def any(items: Iterable["TriggerResult"]) -> "TriggerResult":
+        """
+        Like `any()`, but for TriggerResult. If any inputs had errors that could
+        impact the result, the result will contain an error from one of them.
+        """
         items_list = list(items)
-        for _ in (item for item in items_list if item.triggered and item.error is None):
-            return TriggerResult(triggered=True, error=None)
-        # If we didn't have any untained Trues, the result is tainted.
-        some_error = next((item.error for item in items_list if item.error is not None), None)
-        return TriggerResult(triggered=any(item.triggered for item in items_list), error=some_error)
+        result = any(item.triggered for item in items_list)
+
+        if result:
+            # Result is True. If we have any untainted True, the result is clean.
+            # Only tainted if all Trues are tainted.
+            if any(item.triggered and not item.is_tainted() for item in items_list):
+                return TriggerResult(triggered=True, error=None)
+            # All Trues are tainted
+            return TriggerResult(
+                triggered=True, error=_find_error(items_list, lambda x: x.triggered)
+            )
+        else:
+            # Result is False. Any tainted item could have changed the result.
+            return TriggerResult(
+                triggered=False,
+                error=_find_error(items_list, lambda x: x.is_tainted()),
+            )
 
     @staticmethod
     def all(items: Iterable["TriggerResult"]) -> "TriggerResult":
+        """
+        Like `all()`, but for TriggerResult. If any inputs had errors that could
+        impact the result, the result will contain an error from one of them.
+        """
+        items_list = list(items)
+        result = all(item.triggered for item in items_list)
+
+        if result:
+            # Result is True. Any tainted item could have changed the result.
+            return TriggerResult(
+                triggered=True,
+                error=_find_error(items_list, lambda x: x.is_tainted()),
+            )
+        else:
+            # Result is False. If we have any untainted False, the result is clean.
+            # Only tainted if all Falses are tainted.
+            if any(not item.triggered and not item.is_tainted() for item in items_list):
+                return TriggerResult(triggered=False, error=None)
+            # All Falses are tainted
+            return TriggerResult(
+                triggered=False,
+                error=_find_error(items_list, lambda x: not x.triggered),
+            )
+
+    @staticmethod
+    def none(items: Iterable["TriggerResult"]) -> "TriggerResult":
+        """
+        Like `not any()`, but for TriggerResult. If any inputs had errors that could
+        impact the result, the result will contain an error from one of them.
+        """
         items_list = list(items)
-        some_error = next((item.error for item in items_list if item.error is not None), None)
-        # if anything was tainted, the result is tainted.
-        return TriggerResult(
-            triggered=all(item.triggered for item in items_list),
-            error=some_error,
-        )
+
+        # No items is guaranteed True, no possible error.
+        if not items_list:
+            return TriggerResult(triggered=True, error=None)
+
+        result = all(not item.triggered for item in items_list)
+
+        if result:
+            # Result is True (no conditions triggered)
+            # Any tainted item could have changed the result
+            return TriggerResult(
+                triggered=True,
+                error=_find_error(items_list, lambda x: x.is_tainted()),
+            )
+        else:
+            # Result is False (at least one condition triggered)
+            # If we have any untainted True, the result is clean
+            if any(item.triggered and not item.is_tainted() for item in items_list):
+                return TriggerResult(triggered=False, error=None)
+            # All triggered items are tainted
+            return TriggerResult(
+                triggered=False,
+                error=_find_error(items_list, lambda x: x.triggered),
+            )
 
     def __or__(self, other: "TriggerResult") -> "TriggerResult":
-        return TriggerResult(
-            triggered=self.triggered or other.triggered,
-            error=self.error or other.error,
-        )
+        """
+        OR operation, equivalent to TriggerResult.any([self, other]).
+        """
+        return TriggerResult.any([self, other])
 
     def __and__(self, other: "TriggerResult") -> "TriggerResult":
-        return TriggerResult(
-            triggered=self.triggered and other.triggered,
-            error=self.error or other.error,
-        )
+        """
+        AND operation, equivalent to TriggerResult.all([self, other]).
+        """
+        return TriggerResult.all([self, other])
 
     def __bool__(self) -> NoReturn:
         raise AssertionError("TriggerResult cannot be used as a boolean")
@@ -146,7 +224,9 @@ def evaluate_condition_group_results(
     if logic_type == DataConditionGroup.Type.NONE:
         # if we get to this point, no conditions were met
         # because we would have short-circuited
-        logic_result = TriggerResult.TRUE
+        logic_result = TriggerResult.none(
+            condition_result.logic_result for condition_result in condition_results
+        )
 
     elif logic_type == DataConditionGroup.Type.ANY:
         logic_result = TriggerResult.any(

tests/sentry/workflow_engine/endpoints/test_organization_detector_types.py

@@ -12,13 +12,16 @@
 from sentry.testutils.cases import APITestCase
 from sentry.testutils.silo import region_silo_test
 from sentry.uptime.grouptype import UptimeDomainCheckFailure
-from sentry.workflow_engine.handlers.detector import DetectorHandler, DetectorOccurrence
+from sentry.workflow_engine.handlers.detector import (
+    DetectorHandler,
+    DetectorOccurrence,
+    GroupedDetectorEvaluationResult,
+)
 from sentry.workflow_engine.handlers.detector.base import EventData
 from sentry.workflow_engine.models import DataPacket
 from sentry.workflow_engine.processors.data_condition_group import ProcessedDataConditionGroup
 from sentry.workflow_engine.types import (
     DetectorEvaluationResult,
-    DetectorGroupKey,
     DetectorPriorityLevel,
     DetectorSettings,
 )
@@ -39,10 +42,13 @@ def setUp(self) -> None:
         self.registry_patcher.start()
 
         class MockDetectorHandler(DetectorHandler[dict, bool]):
-            def evaluate(
+            def evaluate_impl(
                 self, data_packet: DataPacket[dict]
-            ) -> dict[DetectorGroupKey, DetectorEvaluationResult]:
-                return {None: DetectorEvaluationResult(None, True, DetectorPriorityLevel.HIGH)}
+            ) -> GroupedDetectorEvaluationResult:
+                return GroupedDetectorEvaluationResult(
+                    result={None: DetectorEvaluationResult(None, True, DetectorPriorityLevel.HIGH)},
+                    tainted=False,
+                )
 
             def extract_value(self, data_packet: DataPacket[dict]) -> bool:
                 return True

tests/sentry/workflow_engine/handlers/detector/test_base.py

@@ -10,6 +10,7 @@
     DataPacketEvaluationType,
     DetectorHandler,
     DetectorOccurrence,
+    GroupedDetectorEvaluationResult,
     StatefulDetectorHandler,
 )
 from sentry.workflow_engine.handlers.detector.stateful import DetectorCounters
@@ -99,10 +100,13 @@ class NoHandlerGroupType(GroupType):
             category_v2 = GroupCategory.METRIC_ALERT.value
 
         class MockDetectorHandler(DetectorHandler[dict, int]):
-            def evaluate(
+            def evaluate_impl(
                 self, data_packet: DataPacket[dict]
-            ) -> dict[DetectorGroupKey, DetectorEvaluationResult]:
-                return {None: DetectorEvaluationResult(None, True, DetectorPriorityLevel.HIGH)}
+            ) -> GroupedDetectorEvaluationResult:
+                return GroupedDetectorEvaluationResult(
+                    result={None: DetectorEvaluationResult(None, True, DetectorPriorityLevel.HIGH)},
+                    tainted=False,
+                )
 
             def extract_value(self, data_packet: DataPacket[dict]) -> int:
                 return data_packet.packet.get("value", 0)
@@ -120,21 +124,24 @@ def extract_dedupe_value(self, data_packet: DataPacket[dict]) -> int:
                 return data_packet.packet.get("dedupe", 0)
 
         class MockDetectorWithUpdateHandler(DetectorHandler[dict, int]):
-            def evaluate(
+            def evaluate_impl(
                 self, data_packet: DataPacket[dict]
-            ) -> dict[DetectorGroupKey, DetectorEvaluationResult]:
+            ) -> GroupedDetectorEvaluationResult:
                 status_change = StatusChangeMessage(
                     "test_update",
                     project_id,
                     DetectorPriorityLevel.HIGH,
                     None,
                 )
 
-                return {
-                    None: DetectorEvaluationResult(
-                        None, True, DetectorPriorityLevel.HIGH, status_change
-                    )
-                }
+                return GroupedDetectorEvaluationResult(
+                    result={
+                        None: DetectorEvaluationResult(
+                            None, True, DetectorPriorityLevel.HIGH, status_change
+                        )
+                    },
+                    tainted=False,
+                )
 
             def create_occurrence(
                 self,

tests/sentry/workflow_engine/models/test_workflow.py

@@ -32,21 +32,21 @@ def test_queryset(self) -> None:
 
     def test_evaluate_trigger_conditions__condition_new_event__True(self) -> None:
         evaluation, _ = self.workflow.evaluate_trigger_conditions(self.event_data)
-        assert evaluation is True
+        assert evaluation.triggered is True
 
     def test_evaluate_trigger_conditions__condition_new_event__False(self) -> None:
         # Update event to have been seen before
         self.group_event.group.times_seen = 5
 
         evaluation, _ = self.workflow.evaluate_trigger_conditions(self.event_data)
-        assert evaluation is False
+        assert evaluation.triggered is False
 
     def test_evaluate_trigger_conditions__no_conditions(self) -> None:
         self.workflow.when_condition_group = None
         self.workflow.save()
 
         evaluation, _ = self.workflow.evaluate_trigger_conditions(self.event_data)
-        assert evaluation is True
+        assert evaluation.triggered is True
 
     def test_evaluate_trigger_conditions__slow_condition(self) -> None:
         # Update group to _all_, since the fast condition is met
@@ -60,7 +60,7 @@ def test_evaluate_trigger_conditions__slow_condition(self) -> None:
             self.event_data
         )
 
-        assert evaluation is True
+        assert evaluation.triggered is True
         assert remaining_conditions == [slow_condition]
 
     def test_full_clean__success(self) -> None: