fix issue RemoveWorker to handle 304 and worker running after being unassigned

Author: suwatch

src/WebJobs.Script.Scaling/AppServiceScaleHandler.cs

@@ -175,7 +175,7 @@ public async Task<bool> PingWorker(string activityId, IWorkerInfo worker)
         /// - FE should return success regardless if worker belongs.
         /// - Any unexpected error will throw.
         /// </summary>
-        public async Task RemoveWorker(string activityId, IWorkerInfo worker)
+        public async Task<bool> RemoveWorker(string activityId, IWorkerInfo worker)
         {
             var stampHostName = GetStampHostName(worker.StampName);
             var details = string.Format("Remove worker request from {0}:{1}", AppServiceSettings.CurrentStampName, AppServiceSettings.WorkerName);
@@ -187,7 +187,13 @@ public async Task RemoveWorker(string activityId, IWorkerInfo worker)
 
             using (var response = await SendAsync(activityId, HttpMethod.Delete, pathAndQuery, worker, details))
             {
+                if (response.StatusCode == HttpStatusCode.NotModified)
+                {
+                    return false;
+                }
+
                 response.EnsureSuccessStatusCode();
+                return true;
             }
         }
 

src/WebJobs.Script.Scaling/IScaleHandler.cs

@@ -20,8 +20,9 @@ public interface IScaleHandler
 
         /// <summary>
         /// ScaleManager requests worker removal
+        /// true or false return value indicates whether worker was indeed removed
         /// </summary>
-        Task RemoveWorker(string activityId, IWorkerInfo worker);
+        Task<bool> RemoveWorker(string activityId, IWorkerInfo worker);
 
         /// <summary>
         /// ScaleManager requests to ping specific worker

src/WebJobs.Script.Scaling/ScaleManager.cs

@@ -20,6 +20,7 @@ public class ScaleManager : IDisposable
         private IWorkerInfo _worker;
         private Timer _workerUpdateTimer;
         private bool _disposed;
+        private bool _pingResult;
 
         private DateTime _pingWorkerUtc = DateTime.MinValue;
         private DateTime _managerCheckUtc = DateTime.MinValue;
@@ -155,18 +156,19 @@ protected virtual async Task ProcessWorkItem(string activityId)
         /// </summary>
         protected virtual async Task PingWorker(string activityId, IWorkerInfo worker)
         {
-            var pingResult = true;
-            if (_pingWorkerUtc < DateTime.UtcNow)
+            // if ping was unsuccessful, keep pinging.  this is to address
+            // the issue where site continue to run on an unassigned worker.
+            if (!_pingResult || _pingWorkerUtc < DateTime.UtcNow)
             {
                 // if PingWorker throws, we will not update the worker status
                 // this worker will be stale and eventually removed.
-                pingResult = await _eventHandler.PingWorker(activityId, worker);
+                _pingResult = await _eventHandler.PingWorker(activityId, worker);
 
                 _pingWorkerUtc = DateTime.UtcNow.Add(_settings.WorkerPingInterval);
             }
 
             // check if worker is valid for the site
-            if (pingResult)
+            if (_pingResult)
             {
                 await _table.AddOrUpdate(worker);
 
@@ -531,11 +533,13 @@ protected virtual async Task<bool> RequestAddWorker(string activityId, IEnumerab
 
         protected virtual async Task RequestRemoveWorker(string activityId, IWorkerInfo manager, IWorkerInfo toRemove)
         {
-            await _eventHandler.RemoveWorker(activityId, toRemove);
-
-            await _table.Delete(toRemove);
+            // remove worker return true if worker is indeed removed
+            if (await _eventHandler.RemoveWorker(activityId, toRemove))
+            {
+                await _table.Delete(toRemove);
 
-            _tracer.TraceRemoveWorker(activityId, toRemove, string.Format("Worker removed by manager ({0})", manager.ToDisplayString()));
+                _tracer.TraceRemoveWorker(activityId, toRemove, string.Format("Worker removed by manager ({0})", manager.ToDisplayString()));
+            }
         }
     }
 }

test/WebJobs.Script.Scaling.Tests/MockScaleHandler.cs

@@ -18,9 +18,9 @@ public virtual Task<string> AddWorker(string activityId, IEnumerable<string> sta
             return Task.FromResult(stampNames.FirstOrDefault());
         }
 
-        public virtual Task RemoveWorker(string activityId, IWorkerInfo workerInfo)
+        public virtual Task<bool> RemoveWorker(string activityId, IWorkerInfo workerInfo)
         {
-            return Task.CompletedTask;
+            return Task.FromResult(true);
         }
 
         public virtual Task<bool> PingWorker(string activityId, IWorkerInfo workerInfo)

test/WebJobs.Script.Scaling.Tests/RequestRemoveWorkerTests.cs

@@ -20,7 +20,7 @@ public async Task BasicTests(IWorkerInfo manager, IWorkerInfo toRemove)
             using (var scaleManager = new MockScaleManager(MockBehavior.Strict))
             {
                 scaleManager.MockScaleHandler.Setup(s => s.RemoveWorker(activityId, toRemove))
-                    .Returns(Task.CompletedTask);
+                    .Returns(Task.FromResult(true));
                 scaleManager.MockWorkerTable.Setup(t => t.Delete(toRemove))
                     .Returns(Task.CompletedTask);
                 scaleManager.MockScaleTracer.Setup(t => t.TraceRemoveWorker(activityId, toRemove, It.Is<string>(s => s.Contains(manager.ToDisplayString()))));