Ignore regression update when the work item is in some states (#3532)

chkeita · chkeita · commit 7b79dd61bd54 · 2023-10-03T11:51:41.000-07:00
* Ignore regression update when the work item is in some states

* format

* formatting

* don't hide messages in the poison queue

* fix typo

* update regression logic
update test_template to support regression

* build fix

* mypy fix

* build fix

* move regression ignore state under ADODuplicateTemplate

* replace extend with append

* update set_tcp_keepalive

* mke mypy happy

* copy ADODuplicateTemplate.OnDuplicate.RegressionIgnoreStates
diff --git a/docs/webhook_events.md b/docs/webhook_events.md
@@ -2033,6 +2033,10 @@ If webhook is set to have Event Grid message format then the payload will look a
                 },
                 "original_crash_test_result": {
                     "$ref": "#/definitions/CrashTestResult"
+                },
+                "report_url": {
+                    "title": "Report Url",
+                    "type": "string"
                 }
             },
             "required": [
@@ -6427,6 +6431,10 @@ If webhook is set to have Event Grid message format then the payload will look a
                 },
                 "original_crash_test_result": {
                     "$ref": "#/definitions/CrashTestResult"
+                },
+                "report_url": {
+                    "title": "Report Url",
+                    "type": "string"
                 }
             },
             "required": [
diff --git a/src/ApiService/ApiService/Functions/QueueFileChanges.cs b/src/ApiService/ApiService/Functions/QueueFileChanges.cs
@@ -128,20 +128,22 @@ private async Async.Task RequeueMessage(string msg, TimeSpan? visibilityTimeout
             newCustomDequeueCount = json["data"]!["customDequeueCount"]!.GetValue<int>();
         }
 
-        var queueName = QueueFileChangesQueueName;
         if (newCustomDequeueCount > MAX_DEQUEUE_COUNT) {
             _log.LogWarning("Message retried more than {MAX_DEQUEUE_COUNT} times with no success: {msg}", MAX_DEQUEUE_COUNT, msg);
-            queueName = QueueFileChangesPoisonQueueName;
+            await _context.Queue.QueueObject(
+                QueueFileChangesPoisonQueueName,
+                json,
+                StorageType.Config)
+                .IgnoreResult();
+        } else {
+            json!["data"]!["customDequeueCount"] = newCustomDequeueCount + 1;
+            await _context.Queue.QueueObject(
+                QueueFileChangesQueueName,
+                json,
+                StorageType.Config,
+                visibilityTimeout ?? CalculateExponentialBackoff(newCustomDequeueCount))
+                .IgnoreResult();
         }
-
-        json!["data"]!["customDequeueCount"] = newCustomDequeueCount + 1;
-
-        await _context.Queue.QueueObject(
-            queueName,
-            json,
-            StorageType.Config,
-            visibilityTimeout ?? CalculateExponentialBackoff(newCustomDequeueCount))
-            .IgnoreResult();
     }
 
     // Possible return values:
diff --git a/src/ApiService/ApiService/OneFuzzTypes/Model.cs b/src/ApiService/ApiService/OneFuzzTypes/Model.cs
@@ -678,7 +678,8 @@ public record ADODuplicateTemplate(
     Dictionary<string, string> SetState,
     Dictionary<string, string> AdoFields,
     string? Comment = null,
-    List<Dictionary<string, string>>? Unless = null
+    List<Dictionary<string, string>>? Unless = null,
+    List<string>? RegressionIgnoreStates = null
 );
 
 public record AdoTemplate(
@@ -707,7 +708,7 @@ public record RenderedAdoTemplate(
     ADODuplicateTemplate OnDuplicate,
     Dictionary<string, string>? AdoDuplicateFields = null,
     string? Comment = null
-    ) : AdoTemplate(BaseUrl, AuthToken, Project, Type, UniqueFields, AdoFields, OnDuplicate, AdoDuplicateFields, Comment);
+    ) : AdoTemplate(BaseUrl, AuthToken, Project, Type, UniqueFields, AdoFields, OnDuplicate, AdoDuplicateFields, Comment) { }
 
 public record TeamsTemplate(SecretData<string> Url) : NotificationTemplate {
     public Task<OneFuzzResultVoid> Validate() {
diff --git a/src/ApiService/ApiService/OneFuzzTypes/Requests.cs b/src/ApiService/ApiService/OneFuzzTypes/Requests.cs
@@ -131,7 +131,7 @@ public record NotificationSearch(
 
 
 public record NotificationTest(
-    [property: Required] Report Report,
+    [property: Required] IReport Report,
     [property: Required] Notification Notification
 ) : BaseRequest;
 
diff --git a/src/ApiService/ApiService/onefuzzlib/Reports.cs b/src/ApiService/ApiService/onefuzzlib/Reports.cs
@@ -1,7 +1,10 @@
 ﻿using System.Text.Json;
+using System.Text.Json.Serialization;
 using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
 using Microsoft.OneFuzz.Service.OneFuzzLib.Orm;
+
+
 namespace Microsoft.OneFuzz.Service;
 
 public interface IReports {
@@ -85,6 +88,7 @@ public static IReport ParseReportOrRegression(string content, Uri reportUrl) {
     }
 }
 
+[JsonConverter(typeof(ReportConverter))]
 public interface IReport {
     Uri? ReportUrl {
         init;
@@ -95,3 +99,19 @@ public string FileName() {
         return string.Concat(segments);
     }
 };
+
+public class ReportConverter : JsonConverter<IReport> {
+
+    public override IReport? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) {
+        using var templateJson = JsonDocument.ParseValue(ref reader);
+
+        if (templateJson.RootElement.TryGetProperty("crash_test_result", out _)) {
+            return templateJson.Deserialize<RegressionReport>(options);
+        }
+        return templateJson.Deserialize<Report>(options);
+    }
+
+    public override void Write(Utf8JsonWriter writer, IReport value, JsonSerializerOptions options) {
+        throw new NotImplementedException();
+    }
+}
diff --git a/src/ApiService/ApiService/onefuzzlib/notifications/Ado.cs b/src/ApiService/ApiService/onefuzzlib/notifications/Ado.cs
@@ -19,6 +19,7 @@ public class Ado : NotificationsBase, IAdo {
     // https://github.com/MicrosoftDocs/azure-devops-docs/issues/5890#issuecomment-539632059
     private const int MAX_SYSTEM_TITLE_LENGTH = 128;
     private const string TITLE_FIELD = "System.Title";
+    private static List<string> DEFAULT_REGRESSION_IGNORE_STATES = new() { "New", "Commited", "Active" };
 
     public Ado(ILogger<Ado> logTracer, IOnefuzzContext context) : base(logTracer, context) {
     }
@@ -56,7 +57,7 @@ public async Async.Task<OneFuzzResultVoid> NotifyAdo(AdoTemplate config, Contain
         _logTracer.LogEvent(adoEventType);
 
         try {
-            await ProcessNotification(_context, container, filename, config, report, _logTracer, notificationInfo);
+            await ProcessNotification(_context, container, filename, config, report, _logTracer, notificationInfo, isRegression: reportable is RegressionReport);
         } catch (Exception e)
               when (e is VssUnauthorizedException || e is VssAuthenticationException || e is VssServiceException) {
             if (config.AdoFields.TryGetValue("System.AssignedTo", out var assignedTo)) {
@@ -298,7 +299,7 @@ private static async Async.Task<Dictionary<string, WorkItemField2>> GetValidFiel
             .ToDictionary(field => field.ReferenceName.ToLowerInvariant());
     }
 
-    private static async Async.Task ProcessNotification(IOnefuzzContext context, Container container, string filename, AdoTemplate config, Report report, ILogger logTracer, IList<(string, string)> notificationInfo, Renderer? renderer = null) {
+    private static async Async.Task ProcessNotification(IOnefuzzContext context, Container container, string filename, AdoTemplate config, Report report, ILogger logTracer, IList<(string, string)> notificationInfo, Renderer? renderer = null, bool isRegression = false) {
         if (!config.AdoFields.TryGetValue(TITLE_FIELD, out var issueTitle)) {
             issueTitle = "{{ report.crash_site }} - {{ report.executable }}";
         }
@@ -311,7 +312,7 @@ private static async Async.Task ProcessNotification(IOnefuzzContext context, Con
 
         var renderedConfig = RenderAdoTemplate(logTracer, renderer, config, instanceUrl);
         var ado = new AdoConnector(renderedConfig, project!, client, instanceUrl, logTracer, await GetValidFields(client, project));
-        await ado.Process(notificationInfo);
+        await ado.Process(notificationInfo, isRegression);
     }
 
     public static RenderedAdoTemplate RenderAdoTemplate(ILogger logTracer, Renderer renderer, AdoTemplate original, Uri instanceUrl) {
@@ -352,7 +353,8 @@ public static RenderedAdoTemplate RenderAdoTemplate(ILogger logTracer, Renderer
             original.OnDuplicate.SetState,
             onDuplicateAdoFields,
             original.OnDuplicate.Comment != null ? Render(renderer, original.OnDuplicate.Comment, instanceUrl, logTracer) : null,
-            onDuplicateUnless
+            onDuplicateUnless,
+            original.OnDuplicate.RegressionIgnoreStates
         );
 
         return new RenderedAdoTemplate(
@@ -598,7 +600,7 @@ private async Async.Task<WorkItem> CreateNew() {
             return (taskType, document);
         }
 
-        public async Async.Task Process(IList<(string, string)> notificationInfo) {
+        public async Async.Task Process(IList<(string, string)> notificationInfo, bool isRegression) {
             var updated = false;
             WorkItem? oldestWorkItem = null;
             await foreach (var workItem in ExistingWorkItems(notificationInfo)) {
@@ -612,6 +614,13 @@ public async Async.Task Process(IList<(string, string)> notificationInfo) {
                     continue;
                 }
 
+                var regressionStatesToIgnore = _config.OnDuplicate.RegressionIgnoreStates != null ? _config.OnDuplicate.RegressionIgnoreStates : DEFAULT_REGRESSION_IGNORE_STATES;
+                if (isRegression) {
+                    var state = (string)workItem.Fields["System.State"];
+                    if (regressionStatesToIgnore.Contains(state, StringComparer.InvariantCultureIgnoreCase))
+                        continue;
+                }
+
                 using (_logTracer.BeginScope("Non-duplicate work item")) {
                     _logTracer.AddTags(new List<(string, string)> { ("NonDuplicateWorkItemId", $"{workItem.Id}") });
                     _logTracer.LogInformation("Found matching non-duplicate work item");
@@ -621,30 +630,32 @@ public async Async.Task Process(IList<(string, string)> notificationInfo) {
                 updated = true;
             }
 
-            if (!updated) {
-                if (oldestWorkItem != null) {
-                    // We have matching work items but all are duplicates
-                    _logTracer.AddTags(notificationInfo);
-                    _logTracer.LogInformation($"All matching work items were duplicates, re-opening the oldest one");
-                    var stateChanged = await UpdateExisting(oldestWorkItem, notificationInfo);
-                    if (stateChanged) {
-                        // add a comment if we re-opened the bug
-                        _ = await _client.AddCommentAsync(
-                            new CommentCreate() {
-                                Text =
-                                    "This work item was re-opened because OneFuzz could only find related work items that are marked as duplicate."
-                            },
-                            _project,
-                            (int)oldestWorkItem.Id!);
-                    }
-                } else {
-                    // We never saw a work item like this before, it must be new
-                    var entry = await CreateNew();
-                    var adoEventType = "AdoNewItem";
-                    _logTracer.AddTags(notificationInfo);
-                    _logTracer.AddTag("WorkItemId", entry.Id.HasValue ? entry.Id.Value.ToString() : "");
-                    _logTracer.LogEvent(adoEventType);
+            if (updated || isRegression) {
+                return;
+            }
+
+            if (oldestWorkItem != null) {
+                // We have matching work items but all are duplicates
+                _logTracer.AddTags(notificationInfo);
+                _logTracer.LogInformation($"All matching work items were duplicates, re-opening the oldest one");
+                var stateChanged = await UpdateExisting(oldestWorkItem, notificationInfo);
+                if (stateChanged) {
+                    // add a comment if we re-opened the bug
+                    _ = await _client.AddCommentAsync(
+                        new CommentCreate() {
+                            Text =
+                                "This work item was re-opened because OneFuzz could only find related work items that are marked as duplicate."
+                        },
+                        _project,
+                        (int)oldestWorkItem.Id!);
                 }
+            } else {
+                // We never saw a work item like this before, it must be new
+                var entry = await CreateNew();
+                var adoEventType = "AdoNewItem";
+                _logTracer.AddTags(notificationInfo);
+                _logTracer.AddTag("WorkItemId", entry.Id.HasValue ? entry.Id.Value.ToString() : "");
+                _logTracer.LogEvent(adoEventType);
             }
         }
 
diff --git a/src/cli/onefuzz/cli.py b/src/cli/onefuzz/cli.py
@@ -28,6 +28,7 @@
     Type,
     TypeVar,
     Union,
+    cast,
 )
 from uuid import UUID
 
@@ -551,8 +552,16 @@ def set_tcp_keepalive() -> None:
     # Azure Load Balancer default timeout (4 minutes)
     #
     # https://urllib3.readthedocs.io/en/stable/reference/urllib3.connection.html?highlight=keep-alive#:~:text=For%20example%2C%20if,socket.SO_KEEPALIVE%2C%201)%2C%0A%5D
-    if value not in urllib3.connection.HTTPConnection.default_socket_options:
-        urllib3.connection.HTTPConnection.default_socket_options.extend((value,))
+
+    default_socket_options = cast(
+        List[Tuple[int, int, int]],
+        urllib3.connection.HTTPConnection.default_socket_options,
+    )
+
+    if value not in default_socket_options:
+        default_socket_options + [
+            value,
+        ]
 
 
 def execute_api(api: Any, api_types: List[Any], version: str) -> int:
diff --git a/src/cli/onefuzz/debug.py b/src/cli/onefuzz/debug.py
@@ -21,7 +21,15 @@
 from azure.storage.blob import ContainerClient
 from onefuzztypes import models, requests, responses
 from onefuzztypes.enums import ContainerType, TaskType
-from onefuzztypes.models import BlobRef, Job, NodeAssignment, Report, Task, TaskConfig
+from onefuzztypes.models import (
+    BlobRef,
+    Job,
+    NodeAssignment,
+    RegressionReport,
+    Report,
+    Task,
+    TaskConfig,
+)
 from onefuzztypes.primitives import Container, Directory, PoolName
 from onefuzztypes.responses import TemplateValidationResponse
 
@@ -633,35 +641,50 @@ def test_template(
         self,
         notificationConfig: models.NotificationConfig,
         task_id: Optional[UUID] = None,
-        report: Optional[Report] = None,
+        report: Optional[str] = None,
     ) -> responses.NotificationTestResponse:
         """Test a notification template"""
 
+        the_report: Union[Report, RegressionReport, None] = None
+
+        if report is not None:
+            try:
+                the_report = RegressionReport.parse_raw(report)
+                print("testing regression report")
+            except Exception:
+                the_report = Report.parse_raw(report)
+                print("testing normal report")
+
         if task_id is not None:
             task = self.onefuzz.tasks.get(task_id)
-            if report is None:
+            if the_report is None:
                 input_blob_ref = BlobRef(
                     account="dummy-storage-account",
                     container="test-notification-crashes",
                     name="fake-crash-sample",
                 )
-                report = self._create_report(
+                the_report = self._create_report(
                     task.job_id, task.task_id, "fake_target.exe", input_blob_ref
                 )
+            elif isinstance(the_report, RegressionReport):
+                if the_report.crash_test_result.crash_report is None:
+                    raise Exception("invalid regression report: no crash report")
+                the_report.crash_test_result.crash_report.task_id = task.task_id
+                the_report.crash_test_result.crash_report.job_id = task.job_id
             else:
-                report.task_id = task.task_id
-                report.job_id = task.job_id
-        elif report is None:
+                the_report.task_id = task.task_id
+                the_report.job_id = task.job_id
+        elif the_report is None:
             raise Exception("must specify either task_id or report")
 
-        report.report_url = "https://dummy-container.blob.core.windows.net/dummy-reports/dummy-report.json"
+        the_report.report_url = "https://dummy-container.blob.core.windows.net/dummy-reports/dummy-report.json"
 
         endpoint = Endpoint(self.onefuzz)
         return endpoint._req_model(
             "POST",
             responses.NotificationTestResponse,
             data=requests.NotificationTest(
-                report=report,
+                report=the_report,
                 notification=models.Notification(
                     container=Container("test-notification-reports"),
                     notification_id=uuid.uuid4(),
diff --git a/src/pytypes/onefuzztypes/models.py b/src/pytypes/onefuzztypes/models.py
@@ -256,13 +256,15 @@ class CrashTestResult(BaseModel):
 class RegressionReport(BaseModel):
     crash_test_result: CrashTestResult
     original_crash_test_result: Optional[CrashTestResult]
+    report_url: Optional[str]
 
 
 class ADODuplicateTemplate(BaseModel):
     increment: List[str]
     comment: Optional[str]
     set_state: Dict[str, str]
     ado_fields: Dict[str, str]
+    regression_ignore_states: Optional[List[str]]
 
 
 class ADOTemplate(BaseModel):
diff --git a/src/pytypes/onefuzztypes/requests.py b/src/pytypes/onefuzztypes/requests.py
@@ -3,7 +3,7 @@
 # Copyright (c) Microsoft Corporation.
 # Licensed under the MIT License.
 
-from typing import Any, Dict, List, Optional
+from typing import Any, Dict, List, Optional, Union
 from uuid import UUID
 
 from pydantic import AnyHttpUrl, BaseModel, Field, root_validator
@@ -26,6 +26,7 @@
     AutoScaleConfig,
     InstanceConfig,
     NotificationConfig,
+    RegressionReport,
     Report,
     TemplateRenderContext,
 )
@@ -280,7 +281,7 @@ class EventsGet(BaseModel):
 
 
 class NotificationTest(BaseModel):
-    report: Report
+    report: Union[Report, RegressionReport]
     notification: models.Notification
 
 
