Return the failure from NotifyAdo as result instead of an exception (#…

…3555)

* Return the failure from NotifyAdo as result  instead of an exception

* remove comment

* requeue when notification fails

* fix logic

src/ApiService/ApiService/Functions/NotificationsTest.cs

@@ -30,7 +30,7 @@ public async Async.Task<HttpResponseData> Run([HttpTrigger(AuthorizationLevel.An
         }
 
         var result = await _context.NotificationOperations.TriggerNotification(notificationTest.Notification.Container, notificationTest.Notification,
-            notificationTest.Report, isLastRetryAttempt: true);
+            notificationTest.Report);
         var response = req.CreateResponse(HttpStatusCode.OK);
         await response.WriteAsJsonAsync(new NotificationTestResponse(result.IsOk, result.ErrorV?.ToString()));
         return response;

src/ApiService/ApiService/Functions/QueueFileChanges.cs

@@ -58,23 +58,17 @@ public async Async.Task Run(
         var storageAccount = new ResourceIdentifier(topicElement.GetString()!);
 
         try {
-            // Setting isLastRetryAttempt to false will rethrow any exceptions
-            // With the intention that the azure functions runtime will handle requeing
-            // the message for us. The difference is for the poison queue, we're handling the
-            // requeuing ourselves because azure functions doesn't support retry policies
-            // for queue based functions.
-
-            var result = await FileAdded(storageAccount, fileChangeEvent, isLastRetryAttempt: false);
-            if (!result.IsOk && result.ErrorV.Code == ErrorCode.ADO_WORKITEM_PROCESSING_DISABLED) {
-                await RequeueMessage(msg, TimeSpan.FromDays(1));
+            var result = await FileAdded(storageAccount, fileChangeEvent);
+            if (!result.IsOk) {
+                await RequeueMessage(msg, result.ErrorV.Code == ErrorCode.ADO_WORKITEM_PROCESSING_DISABLED ? TimeSpan.FromDays(1) : null);
             }
         } catch (Exception e) {
             _log.LogError(e, "File Added failed");
             await RequeueMessage(msg);
         }
     }
 
-    private async Async.Task<OneFuzzResultVoid> FileAdded(ResourceIdentifier storageAccount, JsonDocument fileChangeEvent, bool isLastRetryAttempt) {
+    private async Async.Task<OneFuzzResultVoid> FileAdded(ResourceIdentifier storageAccount, JsonDocument fileChangeEvent) {
         var data = fileChangeEvent.RootElement.GetProperty("data");
         var url = data.GetProperty("url").GetString()!;
         var parts = url.Split("/").Skip(3).ToList();
@@ -86,7 +80,7 @@ private async Async.Task<OneFuzzResultVoid> FileAdded(ResourceIdentifier storage
 
         var (_, result) = await (
             ApplyRetentionPolicy(storageAccount, container, path),
-            _notificationOperations.NewFiles(container, path, isLastRetryAttempt));
+            _notificationOperations.NewFiles(container, path));
 
         return result;
     }

src/ApiService/ApiService/OneFuzzTypes/Enums.cs

@@ -52,6 +52,7 @@ public enum ErrorCode {
     ADO_VALIDATION_INVALID_PROJECT = 496,
     INVALID_RETENTION_PERIOD = 497,
     INVALID_CLI_VERSION = 498,
+    TRANSIENT_NOTIFICATION_FAILURE = 499,
     // NB: if you update this enum, also update enums.py
 }
 

src/ApiService/ApiService/TestHooks/NotificationOperationsTestHooks.cs

@@ -29,9 +29,7 @@ public async Task<HttpResponseData> NewFiles([HttpTrigger(AuthorizationLevel.Ano
 
             var container = query["container"];
             var fileName = query["fileName"];
-            var isLastRetryAttempt = UriExtension.GetBool("isLastRetryAttempt", query, true);
-
-            _ = await _notificationOps.NewFiles(Container.Parse(container), fileName, isLastRetryAttempt);
+            _ = await _notificationOps.NewFiles(Container.Parse(container), fileName);
             var resp = req.CreateResponse(HttpStatusCode.OK);
             return resp;
         }

src/ApiService/ApiService/onefuzzlib/NotificationOperations.cs

@@ -5,14 +5,14 @@
 namespace Microsoft.OneFuzz.Service;
 
 public interface INotificationOperations : IOrm<Notification> {
-    Async.Task<OneFuzzResultVoid> NewFiles(Container container, string filename, bool isLastRetryAttempt);
+    Async.Task<OneFuzzResultVoid> NewFiles(Container container, string filename);
     IAsyncEnumerable<Notification> GetNotifications(Container container);
     IAsyncEnumerable<(Task, IEnumerable<Container>)> GetQueueTasks();
     Async.Task<OneFuzzResult<Notification>> Create(Container container, NotificationTemplate config, bool replaceExisting);
     Async.Task<Notification?> GetNotification(Guid notifificationId);
 
     System.Threading.Tasks.Task<OneFuzzResultVoid> TriggerNotification(Container container,
-        Notification notification, IReport? reportOrRegression, bool isLastRetryAttempt = false);
+        Notification notification, IReport? reportOrRegression);
 }
 
 public class NotificationOperations : Orm<Notification>, INotificationOperations {
@@ -21,7 +21,7 @@ public NotificationOperations(ILogger<NotificationOperations> log, IOnefuzzConte
         : base(log, context) {
 
     }
-    public async Async.Task<OneFuzzResultVoid> NewFiles(Container container, string filename, bool isLastRetryAttempt) {
+    public async Async.Task<OneFuzzResultVoid> NewFiles(Container container, string filename) {
         // We don't want to store file added events for the events container because that causes an infinite loop
         if (container == WellKnownContainers.Events) {
             return Result.Ok();
@@ -39,7 +39,7 @@ public async Async.Task<OneFuzzResultVoid> NewFiles(Container container, string
                 }
 
                 done.Add(notification.Config);
-                var notificationResult = await TriggerNotification(container, notification, reportOrRegression, isLastRetryAttempt);
+                var notificationResult = await TriggerNotification(container, notification, reportOrRegression);
                 if (result.IsOk && !notificationResult.IsOk) {
                     result = notificationResult;
                 }
@@ -86,15 +86,15 @@ public async Async.Task<OneFuzzResultVoid> NewFiles(Container container, string
     }
 
     public async System.Threading.Tasks.Task<OneFuzzResultVoid> TriggerNotification(Container container,
-        Notification notification, IReport? reportOrRegression, bool isLastRetryAttempt = false) {
+        Notification notification, IReport? reportOrRegression) {
         switch (notification.Config) {
             case TeamsTemplate teamsTemplate:
                 await _context.Teams.NotifyTeams(teamsTemplate, container, reportOrRegression!,
                     notification.NotificationId);
                 break;
             case AdoTemplate adoTemplate when reportOrRegression is not null:
                 if (await _context.FeatureManagerSnapshot.IsEnabledAsync(FeatureFlagConstants.EnableWorkItemCreation)) {
-                    return await _context.Ado.NotifyAdo(adoTemplate, container, reportOrRegression, isLastRetryAttempt,
+                    return await _context.Ado.NotifyAdo(adoTemplate, container, reportOrRegression,
                         notification.NotificationId);
                 } else {
                     return OneFuzzResultVoid.Error(ErrorCode.ADO_WORKITEM_PROCESSING_DISABLED, "Work item processing is currently disabled");

src/ApiService/ApiService/onefuzzlib/notifications/Ado.cs

@@ -12,7 +12,7 @@
 namespace Microsoft.OneFuzz.Service;
 
 public interface IAdo {
-    public Async.Task<OneFuzzResultVoid> NotifyAdo(AdoTemplate config, Container container, IReport reportable, bool isLastRetryAttempt, Guid notificationId);
+    public Async.Task<OneFuzzResultVoid> NotifyAdo(AdoTemplate config, Container container, IReport reportable, Guid notificationId);
 }
 
 public class Ado : NotificationsBase, IAdo {
@@ -24,7 +24,7 @@ public class Ado : NotificationsBase, IAdo {
     public Ado(ILogger<Ado> logTracer, IOnefuzzContext context) : base(logTracer, context) {
     }
 
-    public async Async.Task<OneFuzzResultVoid> NotifyAdo(AdoTemplate config, Container container, IReport reportable, bool isLastRetryAttempt, Guid notificationId) {
+    public async Async.Task<OneFuzzResultVoid> NotifyAdo(AdoTemplate config, Container container, IReport reportable, Guid notificationId) {
         var filename = reportable.FileName();
         Report? report;
         if (reportable is RegressionReport regressionReport) {
@@ -57,22 +57,29 @@ public async Async.Task<OneFuzzResultVoid> NotifyAdo(AdoTemplate config, Contain
         _logTracer.LogEvent(adoEventType);
 
         try {
-            await ProcessNotification(_context, container, filename, config, report, _logTracer, notificationInfo, isRegression: reportable is RegressionReport);
+            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) {
+            when (e is VssUnauthorizedException || e is VssAuthenticationException || e is VssServiceException) {
             if (config.AdoFields.TryGetValue("System.AssignedTo", out var assignedTo)) {
                 _logTracer.AddTag("assigned_to", assignedTo);
             }
 
-            if (!isLastRetryAttempt && IsTransient(e)) {
-                _logTracer.LogError("transient ADO notification failure {JobId} {TaskId} {Container} {Filename}", report.JobId, report.TaskId, container, filename);
-                throw;
+            if (IsTransient(e)) {
+                _logTracer.LogError("transient ADO notification failure {JobId} {TaskId} {Container} {Filename}",
+                    report.JobId, report.TaskId, container, filename);
+
+                return OneFuzzResultVoid.Error(ErrorCode.TRANSIENT_NOTIFICATION_FAILURE,
+                    $"Failed to process ado notification : exception: {e}");
             } else {
                 _logTracer.LogError(e, "Failed to process ado notification");
                 await LogFailedNotification(report, e, notificationId);
                 return OneFuzzResultVoid.Error(ErrorCode.NOTIFICATION_FAILURE,
                     $"Failed to process ado notification : exception: {e}");
             }
+        } catch (Exception e) {
+            return OneFuzzResultVoid.Error(ErrorCode.NOTIFICATION_FAILURE,
+                    $"Failed to process ado notification : exception: {e}");
         }
         return OneFuzzResultVoid.Ok;
     }

src/pytypes/onefuzztypes/enums.py

@@ -306,6 +306,7 @@ class ErrorCode(Enum):
     ADO_VALIDATION_INVALID_PROJECT = 496
     INVALID_RETENTION_PERIOD = 497
     INVALID_CLI_VERSION = 498
+    TRANSIENT_NOTIFICATION_FAILURE = 499
     # NB: if you update this enum, also update Enums.cs