Merge pull request #240 from simplify9/hamza/fix/badresult-retry-matchers

Fix bad-result retries never matching

Author: hamzahalq

SW.Bitween.Api/Resources/RetryPolicies/Create.cs

@@ -20,6 +20,7 @@ public Create(BitweenDbContext dbContext, RequestContext requestContext)
     public async Task<object> Handle(RetryPolicyCreate model)
     {
         _requestContext.EnsureAccess(AccountRole.Admin, AccountRole.Member);
+        RetryGroupValidation.EnsureCanFire(model.Groups);
 
         var entity = new RetryPolicy
         {

SW.Bitween.Api/Resources/RetryPolicies/RetryGroupValidation.cs

@@ -0,0 +1,44 @@
+using System.Collections.Generic;
+using System.Linq;
+using SW.Bitween.Model;
+using SW.PrimitiveTypes;
+
+namespace SW.Bitween.Resources.RetryPolicies;
+
+/// <summary>
+/// Rejects retry groups that could never fire. The evaluator skips such groups silently
+/// (matchers must support the result type being evaluated — see
+/// <see cref="Matcher.Supports"/>), which reads as "retries just don't work", so the
+/// misconfiguration is caught at write time instead.
+/// </summary>
+public static class RetryGroupValidation
+{
+    public static void EnsureCanFire(IEnumerable<RetryGroup> groups)
+    {
+        foreach (var group in groups ?? [])
+        {
+            if ((group.AppliesTo?.Count ?? 0) == 0)
+                throw new SWValidationException("RETRY_GROUP_NO_RESULT_TYPE",
+                    $"Group '{group.Name}' applies to no result type, so it would never be evaluated. " +
+                    "Select Error, Bad result, or both.");
+
+            if ((group.Matchers?.Count ?? 0) == 0)
+                throw new SWValidationException("RETRY_GROUP_NO_MATCHERS",
+                    $"Group '{group.Name}' has no matchers, so it would never match a failure. " +
+                    "Add at least one matcher.");
+
+            foreach (var resultType in group.AppliesTo)
+                if (!group.Matchers.Any(m => m.Supports(resultType)))
+                    throw new SWValidationException("RETRY_GROUP_INCOMPATIBLE_MATCHERS",
+                        $"Group '{group.Name}' applies to {resultType} but none of its matchers can be " +
+                        $"evaluated against {resultType} content. {SupportedMatchersFor(resultType)}");
+        }
+    }
+
+    private static string SupportedMatchersFor(XchangeResultType resultType) => resultType switch
+    {
+        XchangeResultType.Error => "Error supports Contains, Regex and Exception type matchers.",
+        XchangeResultType.BadResult => "Bad result supports Contains, Regex and JSON path matchers.",
+        _ => "Successful results are never retried."
+    };
+}

SW.Bitween.Api/Resources/RetryPolicies/Update.cs

@@ -20,6 +20,7 @@ public Update(BitweenDbContext dbContext, RequestContext requestContext)
     public async Task<object> Handle(int key, RetryPolicyUpdate model)
     {
         _requestContext.EnsureAccess(AccountRole.Admin, AccountRole.Member);
+        RetryGroupValidation.EnsureCanFire(model.Groups);
 
         var entity = await _dbContext.FindAsync<RetryPolicy>(key);
         entity.Name = model.Name;

SW.Bitween.Api/Resources/Subscriptions/Update.cs

@@ -11,6 +11,7 @@
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Http;
 using SW.Bitween.Domain.Accounts;
+using SW.Bitween.Resources.RetryPolicies;
 
 namespace SW.Bitween.Resources.Subscriptions
 {
@@ -58,6 +59,9 @@ public async Task<object> Handle(int key, SubscriptionUpdate model)
                 throw new SWValidationException("RETRY_POLICY_NOT_FOUND",
                     $"Retry policy {model.RetryPolicyId} was not found.");
 
+            if (model.CustomRetryPolicy != null)
+                RetryGroupValidation.EnsureCanFire(model.CustomRetryPolicy.Groups);
+
             entity.SetRetryPolicy(model.RetryPolicyId, model.CustomRetryPolicy);
 
             trail.SetAfter(entity);

SW.Bitween.Sdk/Model/AutoRetry/Matcher.cs

@@ -34,7 +34,7 @@ public enum JsonPathOp
 /// </summary>
 /// <remarks>
 /// For <see cref="XchangeResultType.Error"/> groups the content is the exception stack-trace text.
-/// For <see cref="XchangeResultType.BadResult"/> groups the content is the raw JSON response string.
+/// For <see cref="XchangeResultType.BadResult"/> groups the content is the raw response body.
 /// Matcher implementations are serialised polymorphically via <c>System.Text.Json</c>.
 /// </remarks>
 [JsonPolymorphic(TypeDiscriminatorPropertyName = "type")]
@@ -44,8 +44,13 @@ public enum JsonPathOp
 [JsonDerivedType(typeof(JsonPathMatcher),      typeDiscriminator: "jsonPath")]
 public abstract class Matcher
 {
-    /// <summary>The result type this matcher operates on.</summary>
-    public abstract XchangeResultType ResultType { get; }
+    /// <summary>
+    /// Returns <c>true</c> when this matcher can be evaluated against
+    /// <paramref name="resultType"/> content. The evaluator skips incompatible matchers,
+    /// so a group whose matchers all return <c>false</c> here can never fire for that
+    /// result type.
+    /// </summary>
+    public abstract bool Supports(XchangeResultType resultType);
 
     /// <summary>
     /// Returns <c>true</c> when <paramref name="content"/> satisfies this matcher's condition.
@@ -54,16 +59,17 @@ public abstract class Matcher
     public abstract bool IsMatch(string content);
 }
 
-// ── Error matchers ────────────────────────────────────────────────────────────
+// ── Text matchers (Error and BadResult) ───────────────────────────────────────
 
 /// <summary>
-/// Matches when the exception text contains a literal substring.
-/// Applies to <see cref="XchangeResultType.Error"/> content.
+/// Matches when the failure text contains a literal substring — the exception text for
+/// <see cref="XchangeResultType.Error"/>, the response body for <see cref="XchangeResultType.BadResult"/>.
 /// </summary>
 public class ContainsMatcher : Matcher
 {
     /// <inheritdoc/>
-    public override XchangeResultType ResultType => XchangeResultType.Error;
+    public override bool Supports(XchangeResultType resultType) =>
+        resultType is XchangeResultType.Error or XchangeResultType.BadResult;
 
     /// <summary>The substring to search for.</summary>
     public required string Value { get; init; }
@@ -78,13 +84,14 @@ public override bool IsMatch(string content) =>
 }
 
 /// <summary>
-/// Matches when the exception text satisfies a regular expression.
-/// Applies to <see cref="XchangeResultType.Error"/> content.
+/// Matches when the failure text satisfies a regular expression — the exception text for
+/// <see cref="XchangeResultType.Error"/>, the response body for <see cref="XchangeResultType.BadResult"/>.
 /// </summary>
 public class RegexMatcher : Matcher
 {
     /// <inheritdoc/>
-    public override XchangeResultType ResultType => XchangeResultType.Error;
+    public override bool Supports(XchangeResultType resultType) =>
+        resultType is XchangeResultType.Error or XchangeResultType.BadResult;
 
     /// <summary>.NET-compatible regular expression pattern.</summary>
     public required string Pattern { get; init; }
@@ -106,6 +113,8 @@ public class RegexMatcher : Matcher
     public override bool IsMatch(string content) => Compiled.IsMatch(content);
 }
 
+// ── Error-only matcher ────────────────────────────────────────────────────────
+
 /// <summary>
 /// Matches when the exception text mentions a specific .NET exception type name,
 /// scanning the entire stack-trace including inner exceptions.
@@ -118,7 +127,8 @@ public class RegexMatcher : Matcher
 public class ExceptionTypeMatcher : Matcher
 {
     /// <inheritdoc/>
-    public override XchangeResultType ResultType => XchangeResultType.Error;
+    public override bool Supports(XchangeResultType resultType) =>
+        resultType == XchangeResultType.Error;
 
     /// <summary>
     /// Fully-qualified or short exception type name, e.g. <c>"System.TimeoutException"</c>
@@ -163,7 +173,8 @@ public override bool IsMatch(string content)
 public class JsonPathMatcher : Matcher
 {
     /// <inheritdoc/>
-    public override XchangeResultType ResultType => XchangeResultType.BadResult;
+    public override bool Supports(XchangeResultType resultType) =>
+        resultType == XchangeResultType.BadResult;
 
     /// <summary>JSONPath expression, e.g. <c>"$.error.code"</c> or <c>"$.lines[0].status"</c>.</summary>
     public required string Path { get; init; }

SW.Bitween.Sdk/Model/AutoRetry/RetryPolicyEvaluator.cs

@@ -108,7 +108,7 @@ public RetryDecision Evaluate(
                      .Where(g => g.Enabled && g.AppliesTo.Contains(resultType))
                      .OrderBy(g => g.Priority))
         {
-            var compatibleMatchers = group.Matchers.Where(m => m.ResultType == resultType);
+            var compatibleMatchers = group.Matchers.Where(m => m.Supports(resultType));
             if (compatibleMatchers.Any(m => m.IsMatch(content)))
                 return group;
         }

SW.Bitween.UnitTests/RetryPolicyEvaluatorTests.cs

@@ -246,6 +246,45 @@ public void Evaluator_WrongResultType_GroupSkipped()
         Assert.IsFalse(decision.ShouldRetry);
     }
 
+    [TestMethod]
+    public void Evaluator_ContainsMatcher_MatchesBadResultBody()
+    {
+        var policy = PolicyWith(BadResultGroup("bad", new ContainsMatcher { Value = "INSUFFICIENT_STOCK" }));
+        var ev = new RetryPolicyEvaluator(policy);
+        var decision = ev.Evaluate(XchangeResultType.BadResult, "{\"code\":\"INSUFFICIENT_STOCK\"}", 0);
+        Assert.IsTrue(decision.ShouldRetry);
+        Assert.AreEqual("bad", decision.MatchedGroupName);
+    }
+
+    [TestMethod]
+    public void Evaluator_ContainsMatcher_MatchesNonJsonBadResultBody()
+    {
+        // A 4xx body need not be JSON — text matchers are the only way to reach these.
+        var policy = PolicyWith(BadResultGroup("bad", new ContainsMatcher { Value = "rate limit" }));
+        var ev = new RetryPolicyEvaluator(policy);
+        var decision = ev.Evaluate(XchangeResultType.BadResult, "<html><body>Rate limit exceeded</body></html>", 0);
+        Assert.IsTrue(decision.ShouldRetry);
+    }
+
+    [TestMethod]
+    public void Evaluator_RegexMatcher_MatchesBadResultBody()
+    {
+        var policy = PolicyWith(BadResultGroup("bad", new RegexMatcher { Pattern = @"""status"":\s*""FAILED""" }));
+        var ev = new RetryPolicyEvaluator(policy);
+        var decision = ev.Evaluate(XchangeResultType.BadResult, "{\"status\": \"FAILED\"}", 0);
+        Assert.IsTrue(decision.ShouldRetry);
+    }
+
+    [TestMethod]
+    public void Evaluator_ExceptionTypeMatcher_SkippedForBadResult()
+    {
+        // Exception type names are meaningless against a response body — stays Error-only.
+        var policy = PolicyWith(BadResultGroup("bad", new ExceptionTypeMatcher { Value = "System.TimeoutException" }));
+        var ev = new RetryPolicyEvaluator(policy);
+        var decision = ev.Evaluate(XchangeResultType.BadResult, "System.TimeoutException in body", 0);
+        Assert.IsFalse(decision.ShouldRetry);
+    }
+
     // ─── Evaluator: priority ordering ───────────────────────────────────────────
 
     [TestMethod]