Don't do empty background sync triggers (#5436)

Author: mathewc

src/WebJobs.Script.WebHost/FunctionsSyncService.cs

@@ -74,7 +74,7 @@ private async void OnSyncTimerTick(object state)
                 if (!cancellationToken.IsCancellationRequested && ShouldSyncTriggers)
                 {
                     _logger.LogDebug("Initiating background SyncTriggers operation");
-                    await _functionsSyncManager.TrySyncTriggersAsync(checkHash: true);
+                    await _functionsSyncManager.TrySyncTriggersAsync(isBackgroundSync: true);
                 }
             }
             catch (Exception exc) when (!exc.IsFatal())

src/WebJobs.Script.WebHost/Management/FunctionsSyncManager.cs

@@ -79,7 +79,7 @@ internal bool ArmCacheEnabled
             }
         }
 
-        public async Task<SyncTriggersResult> TrySyncTriggersAsync(bool checkHash = false)
+        public async Task<SyncTriggersResult> TrySyncTriggersAsync(bool isBackgroundSync = false)
         {
             var result = new SyncTriggersResult
             {
@@ -99,27 +99,36 @@ public async Task<SyncTriggersResult> TrySyncTriggersAsync(bool checkHash = fals
                 await _syncSemaphore.WaitAsync();
 
                 var hashBlob = await GetHashBlobAsync();
-                if (checkHash && hashBlob == null)
+                if (isBackgroundSync && hashBlob == null)
                 {
-                    // short circuit before doing any work in cases where
-                    // we're asked to check/update hash but don't have
+                    // short circuit before doing any work in background sync
+                    // cases where we need to check/update hash but don't have
                     // storage access
                     return result;
                 }
 
-                string json = await GetSyncTriggersPayload();
+                var payload = await GetSyncTriggersPayload();
+                if (isBackgroundSync && payload.Count == 0)
+                {
+                    // We don't do background sync for empty triggers.
+                    // We've seen error cases where a site temporarily gets into a situation
+                    // where it's site content is empty. Doing the empty sync can cause the app
+                    // to go idle when it shouldn't.
+                    _logger.LogDebug("No functions found. Skipping Sync operation.");
+                    return result;
+                }
 
                 bool shouldSyncTriggers = true;
                 string newHash = null;
-                if (checkHash)
+                if (isBackgroundSync)
                 {
-                    newHash = await CheckHashAsync(hashBlob, json);
+                    newHash = await CheckHashAsync(hashBlob, payload.Content);
                     shouldSyncTriggers = newHash != null;
                 }
 
                 if (shouldSyncTriggers)
                 {
-                    var (success, error) = await SetTriggersAsync(json);
+                    var (success, error) = await SetTriggersAsync(payload.Content);
                     if (success && newHash != null)
                     {
                         await UpdateHashAsync(hashBlob, newHash);
@@ -254,19 +263,24 @@ internal async Task<CloudBlockBlob> GetHashBlobAsync()
             return _hashBlob;
         }
 
-        public async Task<string> GetSyncTriggersPayload()
+        public async Task<SyncTriggersPayload> GetSyncTriggersPayload()
         {
             var hostOptions = _applicationHostOptions.CurrentValue.ToHostOptions();
             var functionsMetadata = _functionMetadataProvider.GetFunctionMetadata();
 
             // trigger information used by the ScaleController
             var triggers = await GetFunctionTriggers(functionsMetadata, hostOptions);
             var triggersArray = new JArray(triggers);
+            int count = triggersArray.Count;
 
             if (!ArmCacheEnabled)
             {
                 // extended format is disabled - just return triggers
-                return JsonConvert.SerializeObject(triggersArray);
+                return new SyncTriggersPayload
+                {
+                    Content = JsonConvert.SerializeObject(triggersArray),
+                    Count = count
+                };
             }
 
             // Add triggers to the payload
@@ -326,10 +340,18 @@ public async Task<string> GetSyncTriggersPayload()
                 // limit. If we're over limit, revert to the minimal triggers
                 // format.
                 _logger.LogWarning($"SyncTriggers payload of length '{json.Length}' exceeds max length of '{ScriptConstants.MaxTriggersStringLength}'. Reverting to minimal format.");
-                return JsonConvert.SerializeObject(triggersArray);
+                return new SyncTriggersPayload
+                {
+                    Content = JsonConvert.SerializeObject(triggersArray),
+                    Count = count
+                };
             }
 
-            return json;
+            return new SyncTriggersPayload
+            {
+                Content = json,
+                Count = count
+            };
         }
 
         internal async Task<IEnumerable<JObject>> GetFunctionTriggers(IEnumerable<FunctionMetadata> functionsMetadata, ScriptJobHostOptions hostOptions)
@@ -586,6 +608,13 @@ public void Dispose()
             _syncSemaphore.Dispose();
         }
 
+        public class SyncTriggersPayload
+        {
+            public string Content { get; set; }
+
+            public int Count { get; set; }
+        }
+
         private class DurableConfig
         {
             public string HubName { get; set; }

src/WebJobs.Script.WebHost/Management/IFunctionsSyncManager.cs

@@ -10,9 +10,8 @@ public interface IFunctionsSyncManager
         /// <summary>
         /// Sync function triggers with Antares infrastructure.
         /// </summary>
-        /// <param name="checkHash">Indicates whether the last sync hash should be checked
-        /// to conditonally perform the sync.</param>
+        /// <param name="isBackgroundSync">Indicates whether this is a background sync operation.</param>
         /// <returns>The <see cref="SyncTriggersResult"/> for the request.</returns>
-        Task<SyncTriggersResult> TrySyncTriggersAsync(bool checkHash = false);
+        Task<SyncTriggersResult> TrySyncTriggersAsync(bool isBackgroundSync = false);
     }
 }

test/WebJobs.Script.Tests.Integration/Management/FunctionsSyncManagerTests.cs

@@ -2,7 +2,6 @@
 // Licensed under the MIT License. See License.txt in the project root for license information.
 
 using System;
-using System.Collections;
 using System.Collections.Generic;
 using System.IO;
 using System.IO.Abstractions;
@@ -13,9 +12,7 @@
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.Azure.WebJobs.Host.Executors;
-using Microsoft.Azure.WebJobs.Script.BindingExtensions;
 using Microsoft.Azure.WebJobs.Script.Config;
-using Microsoft.Azure.WebJobs.Script.Models;
 using Microsoft.Azure.WebJobs.Script.WebHost;
 using Microsoft.Azure.WebJobs.Script.WebHost.Management;
 using Microsoft.Azure.WebJobs.Script.Workers.Rpc;
@@ -26,7 +23,6 @@
 using Moq;
 using Newtonsoft.Json;
 using Newtonsoft.Json.Linq;
-using NuGet.Packaging.Signing;
 using Xunit;
 
 namespace Microsoft.Azure.WebJobs.Script.Tests.Managment
@@ -48,6 +44,7 @@ public class FunctionsSyncManagerTests : IDisposable
         private readonly Mock<IEnvironment> _mockEnvironment;
         private readonly HostNameProvider _hostNameProvider;
         private string _function1;
+        private bool _emptyContent;
 
         public FunctionsSyncManagerTests()
         {
@@ -284,14 +281,32 @@ private void VerifyResultWithCacheOff()
         }
 
         [Fact]
-        public async Task TrySyncTriggers_CheckHash_PostsExpectedContent()
+        public async Task TrySyncTriggers_BackgroundSync_DoesNotPostsEmptyContent()
+        {
+            _emptyContent = true;
+
+            using (var env = new TestScopedEnvironmentVariable(_vars))
+            {
+                var syncResult = await _functionsSyncManager.TrySyncTriggersAsync(isBackgroundSync: true);
+                Assert.Equal(0, _mockHttpHandler.RequestCount);
+                Assert.Equal(0, _contentBuilder.Length);
+                Assert.True(syncResult.Success);
+                Assert.Null(syncResult.Error);
+
+                var logs = _loggerProvider.GetAllLogMessages().Select(p => p.FormattedMessage).ToArray();
+                Assert.Equal("No functions found. Skipping Sync operation.", logs.Single());
+            }
+        }
+
+        [Fact]
+        public async Task TrySyncTriggers_BackgroundSync_PostsExpectedContent()
         {
             using (var env = new TestScopedEnvironmentVariable(_vars))
             {
                 var hashBlob = await _functionsSyncManager.GetHashBlobAsync();
                 await hashBlob.DeleteIfExistsAsync();
 
-                var syncResult = await _functionsSyncManager.TrySyncTriggersAsync(checkHash: true);
+                var syncResult = await _functionsSyncManager.TrySyncTriggersAsync(isBackgroundSync: true);
                 Assert.True(syncResult.Success);
                 Assert.Null(syncResult.Error);
                 Assert.Equal(1, _mockHttpHandler.RequestCount);
@@ -318,7 +333,7 @@ public async Task TrySyncTriggers_CheckHash_PostsExpectedContent()
                 _loggerProvider.ClearAllLogMessages();
                 ResetMockFileSystem();
                 _mockHttpHandler.Reset();
-                syncResult = await _functionsSyncManager.TrySyncTriggersAsync(checkHash: true);
+                syncResult = await _functionsSyncManager.TrySyncTriggersAsync(isBackgroundSync: true);
                 Assert.Equal(0, _mockHttpHandler.RequestCount);
                 Assert.Equal(0, _contentBuilder.Length);
                 Assert.True(syncResult.Success);
@@ -331,23 +346,23 @@ public async Task TrySyncTriggers_CheckHash_PostsExpectedContent()
                 // simulate a function change resulting in a new hash value
                 ResetMockFileSystem("{}");
                 _mockHttpHandler.Reset();
-                syncResult = await _functionsSyncManager.TrySyncTriggersAsync(checkHash: true);
+                syncResult = await _functionsSyncManager.TrySyncTriggersAsync(isBackgroundSync: true);
                 Assert.Equal(1, _mockHttpHandler.RequestCount);
                 Assert.True(syncResult.Success);
                 Assert.Null(syncResult.Error);
             }
         }
 
         [Fact]
-        public async Task TrySyncTriggers_CheckHash_SetTriggersFailure_HashNotUpdated()
+        public async Task TrySyncTriggers_BackgroundSync_SetTriggersFailure_HashNotUpdated()
         {
             using (var env = new TestScopedEnvironmentVariable(_vars))
             {
                 var hashBlob = await _functionsSyncManager.GetHashBlobAsync();
                 await hashBlob.DeleteIfExistsAsync();
 
                 _mockHttpHandler.MockStatusCode = HttpStatusCode.InternalServerError;
-                var syncResult = await _functionsSyncManager.TrySyncTriggersAsync(checkHash: true);
+                var syncResult = await _functionsSyncManager.TrySyncTriggersAsync(isBackgroundSync: true);
                 Assert.False(syncResult.Success);
                 string expectedErrorMessage = "SyncTriggers call failed (StatusCode=InternalServerError).";
                 Assert.Equal(expectedErrorMessage, syncResult.Error);
@@ -674,12 +689,22 @@ private IFileSystem CreateFileSystem(ScriptApplicationHostOptions hostOptions, s
             dirBase.Setup(d => d.Exists(rootPath)).Returns(true);
             dirBase.Setup(d => d.Exists(Path.Combine(rootPath, "bin"))).Returns(true);
             dirBase.Setup(d => d.EnumerateDirectories(rootPath))
-                .Returns(new[]
+                .Returns(() =>
                 {
-                    Path.Combine(rootPath, "bin"),
-                    Path.Combine(rootPath, "function1"),
-                    Path.Combine(rootPath, "function2"),
-                    Path.Combine(rootPath, "function3")
+                    if (_emptyContent)
+                    {
+                        return new string[0];
+                    }
+                    else
+                    {
+                        return new[]
+                        {
+                            Path.Combine(rootPath, "bin"),
+                            Path.Combine(rootPath, "function1"),
+                            Path.Combine(rootPath, "function2"),
+                            Path.Combine(rootPath, "function3")
+                        };
+                    }
                 });
 
             _function1 = @"{