Detection for heredoc (#2738)

Co-authored-by: Cory Miller <13227161+cory-miller@users.noreply.github.com>

src/Runner.Common/Constants.cs

@@ -158,7 +158,6 @@ namespace GitHub.Runner.Common
                 public static readonly string LogTemplateErrorsAsDebugMessages = "DistributedTask.LogTemplateErrorsAsDebugMessages";
                 public static readonly string UseContainerPathForTemplate = "DistributedTask.UseContainerPathForTemplate";
                 public static readonly string AllowRunnerContainerHooks = "DistributedTask.AllowRunnerContainerHooks";
-                public static readonly string AllowRunnerStallDetect = "DistributedTask.AllowRunnerStallDetect";
             }
 
             public static readonly string InternalTelemetryIssueDataKey = "_internal_telemetry";

src/Runner.Worker/FeatureManager.cs

@@ -11,10 +11,5 @@ namespace GitHub.Runner.Worker
             var isContainerHooksPathSet = !string.IsNullOrEmpty(Environment.GetEnvironmentVariable(Constants.Hooks.ContainerHooksPath));
             return isContainerHookFeatureFlagSet && isContainerHooksPathSet;
         }
-        public static bool IsStallDetectEnabled(Variables variables)
-        {
-            var isStallDetectFeatureFlagSet = variables?.GetBoolean(Constants.Runner.Features.AllowRunnerStallDetect) ?? false;
-            return isStallDetectFeatureFlagSet;
-        }
     }
 }

src/Runner.Worker/FileCommandManager.cs

@@ -322,16 +322,28 @@ namespace GitHub.Runner.Worker
                     var equalsIndex = line.IndexOf("=", StringComparison.Ordinal);
                     var heredocIndex = line.IndexOf("<<", StringComparison.Ordinal);
 
-                    // Heredoc style NAME<<EOF (where EOF is typically randomly-generated Base64 and may include an '=' character)
+                    // Heredoc style NAME=<<EOF (where EOF is typically randomly-generated Base64 and may include an '=' character)
                     // see https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#multiline-strings
-                    if (heredocIndex >= 0 && (equalsIndex < 0 || heredocIndex < equalsIndex))
+                    // "key=<<EOF is also considered heredoc where the key contains an =.
+                    // Also we detect heredoc if whitespace chars exist between = and <<. (ex: "key      <<EOF")
+                    var isHeredoc = heredocIndex >= 0 &&
+                    (
+                        equalsIndex < 0 ||
+                        heredocIndex < equalsIndex ||
+                        (
+                            heredocIndex > equalsIndex &&
+                            OnlyContainsWhiteSpaceBetweenPositions(line, equalsIndex, heredocIndex)
+                        ) 
+                    );
+                    
+                    if (isHeredoc)
                     {
                         var split = line.Split(new[] { "<<" }, 2, StringSplitOptions.None);
                         if (string.IsNullOrEmpty(split[0]) || string.IsNullOrEmpty(split[1]))
                         {
                             throw new Exception($"Invalid format '{line}'. Name must not be empty and delimiter must not be empty");
                         }
-                        key = split[0];
+                        key = split[0].Trim().TrimEnd('=');
                         var delimiter = split[1];
                         var startIndex = index; // Start index of the value (inclusive)
                         var endIndex = index;   // End index of the value (exclusive)
@@ -352,8 +364,8 @@ namespace GitHub.Runner.Worker
 
                         output = endIndex > startIndex ? text.Substring(startIndex, endIndex - startIndex) : string.Empty;
                     }
-                    // Normal style NAME=VALUE
-                    else if (equalsIndex >= 0 && heredocIndex < 0)
+                    // Normal style NAME=VALUE, can have << in the value.
+                    else if (equalsIndex >= 0)
                     {
                         var split = line.Split(new[] { '=' }, 2, StringSplitOptions.None);
                         if (string.IsNullOrEmpty(line))
@@ -383,6 +395,32 @@ namespace GitHub.Runner.Worker
             return GetEnumerator();
         }
 
+        // accepts a string, two indexes, and returns true if only whitespace chars exist between those two indexes (non-inclusive)
+        private static bool OnlyContainsWhiteSpaceBetweenPositions(string str, int pos1, int pos2)
+        {
+            // Check if the provided positions are valid
+            if (pos1 < 0 || pos2 < 0 || pos1 >= str.Length || pos2 >= str.Length)
+            {
+                throw new ArgumentOutOfRangeException("OnlyContainsWhiteSpaceBetweenPositions: Positions should be within the string length.");
+            }
+
+            // Ensure pos1 is always the smaller position
+            if (pos2 < pos1)
+            {
+                throw new ArgumentException("OnlyContainsWhiteSpaceBetweenPositions: pos1 must be less than or equal to pos2.");
+            }
+
+            for (int i = pos1 + 1; i < pos2; i++)
+            {
+                if (!char.IsWhiteSpace(str[i]))
+                {
+                    return false;
+                }
+            }
+
+            return true;
+        }
+
         private static string ReadLine(
                 string text,
                 ref int index)

src/Runner.Worker/Handlers/ContainerActionHandler.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
@@ -240,11 +240,9 @@ namespace GitHub.Runner.Worker.Handlers
             }
             else
             {
-                StallManager stallManager = FeatureManager.IsStallDetectEnabled(ExecutionContext.Global.Variables) ? new StallManager(ExecutionContext) : null;
-                using (OutputManager stdoutManager = new OutputManager(ExecutionContext, ActionCommandManager, container, stallManager),
-                             stderrManager = new OutputManager(ExecutionContext, ActionCommandManager, container, stallManager))
+                using (var stdoutManager = new OutputManager(ExecutionContext, ActionCommandManager, container))
+                using (var stderrManager = new OutputManager(ExecutionContext, ActionCommandManager, container))
                 {
-                    stallManager?.Initialize();
                     var runExitCode = await dockerManager.DockerRun(ExecutionContext, container, stdoutManager.OnDataReceived, stderrManager.OnDataReceived);
                     ExecutionContext.Debug($"Docker Action run completed with exit code {runExitCode}");
                     if (runExitCode != 0)

src/Runner.Worker/Handlers/NodeScriptActionHandler.cs

@@ -159,15 +159,12 @@ namespace GitHub.Runner.Worker.Handlers
                 ExecutionContext.Global.Variables.Set("Node12ActionsWarnings", StringUtil.ConvertToJson(warningActions));
             }
 
-            StallManager stallManager = FeatureManager.IsStallDetectEnabled(ExecutionContext.Global.Variables) ? new StallManager(ExecutionContext) : null;
-
-            using (OutputManager stdoutManager = new OutputManager(ExecutionContext, ActionCommandManager, null, stallManager),
-                                 stderrManager = new OutputManager(ExecutionContext, ActionCommandManager, null, stallManager))
+            using (var stdoutManager = new OutputManager(ExecutionContext, ActionCommandManager))
+            using (var stderrManager = new OutputManager(ExecutionContext, ActionCommandManager))
             {
                 StepHost.OutputDataReceived += stdoutManager.OnDataReceived;
                 StepHost.ErrorDataReceived += stderrManager.OnDataReceived;
 
-                stallManager?.Initialize();
                 // Execute the process. Exit code 0 should always be returned.
                 // A non-zero exit code indicates infrastructural failure.
                 // Task failure should be communicated over STDOUT using ## commands.

src/Runner.Worker/Handlers/OutputManager.cs

@@ -26,14 +26,12 @@ namespace GitHub.Runner.Worker.Handlers
         private IssueMatcher[] _matchers = Array.Empty<IssueMatcher>();
         // Mapping that indicates whether a directory belongs to the workflow repository
         private readonly Dictionary<string, string> _directoryMap = new();
-        private StallManager _stallManager;
 
-        public OutputManager(IExecutionContext executionContext, IActionCommandManager commandManager, ContainerInfo container = null, StallManager stallManager = null)
+        public OutputManager(IExecutionContext executionContext, IActionCommandManager commandManager, ContainerInfo container = null)
         {
             _executionContext = executionContext;
             _commandManager = commandManager;
             _container = container ?? executionContext.Global.Container;
-            _stallManager = stallManager;
 
             // Recursion failsafe (test override)
             var failsafeString = Environment.GetEnvironmentVariable("RUNNER_TEST_GET_REPOSITORY_PATH_FAILSAFE");
@@ -78,10 +76,6 @@ namespace GitHub.Runner.Worker.Handlers
 
         public void OnDataReceived(object sender, ProcessDataReceivedEventArgs e)
         {
-            if (_stallManager != null)
-            {
-                _stallManager.OnDataReceived(sender, e);
-            }
             var line = e.Data;
 
             // ## commands

src/Runner.Worker/Handlers/RunnerPluginHandler.cs

@@ -43,14 +43,11 @@ namespace GitHub.Runner.Worker.Handlers
 
             // Make sure only particular task get run as runner plugin.
             var runnerPlugin = HostContext.GetService<IRunnerPluginManager>();
-            StallManager stallManager = FeatureManager.IsStallDetectEnabled(ExecutionContext.Global.Variables) ? new StallManager(ExecutionContext) : null;
-
-            using (OutputManager outputManager = new OutputManager(ExecutionContext, ActionCommandManager, null, stallManager))
+            using (var outputManager = new OutputManager(ExecutionContext, ActionCommandManager))
             {
                 ActionCommandManager.EnablePluginInternalCommand();
                 try
                 {
-                    stallManager?.Initialize();
                     await runnerPlugin.RunPluginActionAsync(ExecutionContext, plugin, Inputs, Environment, RuntimeVariables, outputManager.OnDataReceived);
                 }
                 finally

src/Runner.Worker/Handlers/ScriptHandler.cs

@@ -321,15 +321,13 @@ namespace GitHub.Runner.Worker.Handlers
             ExecutionContext.Debug($"{fileName} {arguments}");
 
             Inputs.TryGetValue("standardInInput", out var standardInInput);
-            StallManager stallManager = FeatureManager.IsStallDetectEnabled(ExecutionContext.Global.Variables) ? new StallManager(ExecutionContext) : null;
-            using (OutputManager stdoutManager = new OutputManager(ExecutionContext, ActionCommandManager, null, stallManager),
-                                 stderrManager = new OutputManager(ExecutionContext, ActionCommandManager, null, stallManager))
+            using (var stdoutManager = new OutputManager(ExecutionContext, ActionCommandManager))
+            using (var stderrManager = new OutputManager(ExecutionContext, ActionCommandManager))
             {
                 StepHost.OutputDataReceived += stdoutManager.OnDataReceived;
                 StepHost.ErrorDataReceived += stderrManager.OnDataReceived;
 
                 // Execute
-                stallManager?.Initialize();
                 int exitCode = await StepHost.ExecuteAsync(ExecutionContext,
                                             workingDirectory: StepHost.ResolvePathForStepHost(ExecutionContext, workingDirectory),
                                             fileName: fileName,

src/Runner.Worker/Handlers/StallManager.cs

@@ -1,70 +0,0 @@
-using System;
-using System.Timers;
-using GitHub.Runner.Common;
-using GitHub.Runner.Sdk;
-
-namespace GitHub.Runner.Worker.Handlers
-{
-    [ServiceLocator(Default = typeof(TimerAdapter))]
-    public interface ITimer
-    {
-        void Start();
-        void Stop();
-        double Interval { get; set; }
-        event ElapsedEventHandler Elapsed;
-        bool AutoReset { get; set; }
-        void Dispose();
-    }
-
-    public class TimerAdapter : Timer, ITimer { }
-
-    public sealed class StallManager : IDisposable
-    {
-        public static TimeSpan DefaultStallInterval = TimeSpan.FromMinutes(30);
-
-        private readonly IExecutionContext _executionContext;
-        private readonly double _interval;
-
-        private ITimer _timer { get; set; }
-        private int _intervalsElapsedWhileStalled = 0;
-
-        public StallManager(IExecutionContext executionContext, double interval, ITimer timer)
-        {
-            _executionContext = executionContext;
-            _interval = interval;
-            _timer = timer;
-
-            _timer.Interval = _interval;
-            _timer.Elapsed += TriggerWarning;
-        }
-        public StallManager(IExecutionContext executionContext, double interval) : this(executionContext, interval, new TimerAdapter()) { }
-        public StallManager(IExecutionContext executionContext) : this(executionContext, StallManager.DefaultStallInterval.TotalMilliseconds) { }
-
-        public void Initialize()
-        {
-            this.OnDataReceived(null, null);
-        }
-
-        public void Dispose()
-        {
-            try
-            {
-                _timer.Dispose();
-            }
-            catch { }
-        }
-
-        public void OnDataReceived(object sender, ProcessDataReceivedEventArgs e)
-        {
-            _intervalsElapsedWhileStalled = 0;
-            _timer.Stop();
-            _timer.Start();
-        }
-
-        private void TriggerWarning(object source, ElapsedEventArgs e)
-        {
-            _intervalsElapsedWhileStalled++;
-            _executionContext.Warning($"No output has been detected in the last {TimeSpan.FromMilliseconds(_intervalsElapsedWhileStalled * _interval).TotalMinutes} minutes and the process has not yet exited. This step may have stalled and might require some investigation.");
-        }
-    }
-}

src/Test/L0/Worker/FileCommandTestBase.cs

@@ -238,15 +238,26 @@ namespace GitHub.Runner.Common.Tests.Worker
                     "MY_KEY_4<<EOF",
                     "EOF EOF",
                     "EOF",
+                    "MY_KEY_5=abc << def",
+                    "MY_KEY_6=    <<EOF",
+                    "white space test",
+                    "EOF",
+                    "MY_KEY_7 <<=EOF=",
+                    "abc",
+                    "=EOF=",
+                    string.Empty
                 };
                 TestUtil.WriteContent(stateFile, content);
                 _fileCmdExtension.ProcessCommand(_executionContext.Object, stateFile, null);
                 Assert.Equal(0, _issues.Count);
-                Assert.Equal(4, _store.Count);
+                Assert.Equal(7, _store.Count);
                 Assert.Equal($"hello{BREAK}{BREAK}three{BREAK}", _store["MY_KEY_1"]);
                 Assert.Equal($"hello=two", _store["MY_KEY_2"]);
                 Assert.Equal($" EOF", _store["MY_KEY_3"]);
                 Assert.Equal($"EOF EOF", _store["MY_KEY_4"]);
+                Assert.Equal($"abc << def", _store["MY_KEY_5"]);
+                Assert.Equal($"white space test", _store["MY_KEY_6"]);
+                Assert.Equal($"abc", _store["MY_KEY_7"]);
             }
         }
 

src/Test/L0/Worker/OutputManagerL0.cs

@@ -1014,8 +1014,7 @@ namespace GitHub.Runner.Common.Tests.Worker
                     return false;
                 });
 
-            StallManager stallManager = new StallManager(_executionContext.Object);
-            _outputManager = new OutputManager(_executionContext.Object, _commandManager.Object, stepContainer, stallManager);
+            _outputManager = new OutputManager(_executionContext.Object, _commandManager.Object, stepContainer);
             return hostContext;
         }
 

src/Test/L0/Worker/StallManagerL0.cs

@@ -1,156 +0,0 @@
-using System;
-using System.Timers;
-using System.Collections.Generic;
-using System.Runtime.CompilerServices;
-using GitHub.Runner.Worker;
-using GitHub.Runner.Worker.Container;
-using GitHub.Runner.Worker.Handlers;
-using Moq;
-using Xunit;
-using DTWebApi = GitHub.DistributedTask.WebApi;
-using GitHub.Runner.Common.Util;
-using GitHub.DistributedTask.WebApi;
-using System.Diagnostics;
-
-namespace GitHub.Runner.Common.Tests.Worker
-{
-
-    public class MockTimer : ITimer
-    {
-        public bool _started = false;
-        public bool _stopped = false;
-        public bool _reset = false;
-        public double Interval { get; set; }
-        public event ElapsedEventHandler Elapsed;
-        public bool AutoReset { get; set; }
-
-        public MockTimer()
-        {
-            Interval = 1;
-        }
-
-        public void Dispose() { }
-
-        public void Start()
-        {
-            _started = true;
-            if (_stopped)
-            {
-                _stopped = false;
-                _reset = true;
-            }
-        }
-        public void Stop()
-        {
-            _reset = false;
-            _started = false;
-            _stopped = true;
-        }
-
-        public void TimeElapsed()
-        {
-            this.Elapsed.Invoke(this, new EventArgs() as ElapsedEventArgs);
-        }
-    }
-
-    public sealed class StallManagerL0
-    {
-        private Mock<IExecutionContext> _executionContext;
-        private List<Tuple<DTWebApi.Issue, string>> _issues;
-        private Variables _variables;
-
-        private TestHostContext Setup(
-            [CallerMemberName] string name = "",
-            ContainerInfo jobContainer = null,
-            ContainerInfo stepContainer = null)
-        {
-            var hostContext = new TestHostContext(this, name);
-            _executionContext = new Mock<IExecutionContext>();
-            _issues = new List<Tuple<DTWebApi.Issue, string>>();
-
-            // Variables to test for secret scrubbing & FF options
-            _variables = new Variables(hostContext, new Dictionary<string, VariableValue>
-                {
-                    { "DistributedTask.AllowRunnerStallDetect", new VariableValue("true", true) },
-                });
-
-            _executionContext.Setup(x => x.Global)
-                .Returns(new GlobalContext
-                {
-                    Container = jobContainer,
-                    Variables = _variables,
-                    WriteDebug = true,
-                });
-
-            _executionContext.Setup(x => x.AddIssue(It.IsAny<DTWebApi.Issue>(), It.IsAny<ExecutionContextLogOptions>()))
-                .Callback((DTWebApi.Issue issue, ExecutionContextLogOptions logOptions) =>
-                {
-                    var resolvedMessage = issue.Message;
-                    if (logOptions.WriteToLog && !string.IsNullOrEmpty(logOptions.LogMessageOverride))
-                    {
-                        resolvedMessage = logOptions.LogMessageOverride;
-                    }
-                    _issues.Add(new(issue, resolvedMessage));
-                });
-
-            return hostContext;
-        }
-
-        [Fact]
-        [Trait("Level", "L0")]
-        [Trait("Category", "Worker")]
-        public void OutputWarningMessageOnTimeElapsed()
-        {
-            MockTimer timer = new MockTimer();
-            using (Setup())
-            using (StallManager manager = new StallManager(_executionContext.Object, TimeSpan.FromMinutes(10).TotalMilliseconds, timer))
-            {
-
-                timer.TimeElapsed();
-
-                Assert.Equal(1, _issues.Count);
-                Assert.Equal("No output has been detected in the last 10 minutes and the process has not yet exited. This step may have stalled and might require some investigation.", _issues[0].Item1.Message);
-                Assert.Equal(DTWebApi.IssueType.Warning, _issues[0].Item1.Type);
-            }
-        }
-
-        [Fact]
-        [Trait("Level", "L0")]
-        [Trait("Category", "Worker")]
-        public void ValidateTimerResetOnNewMessage()
-        {
-
-            MockTimer timer = new MockTimer();
-            using (Setup())
-            using (StallManager manager = new StallManager(_executionContext.Object, TimeSpan.FromMinutes(10).TotalMilliseconds, timer))
-            {
-
-                // Trigger 2 elapsed
-                timer.TimeElapsed();
-                timer.TimeElapsed();
-
-                // Should have triggered 2 warnings
-                Assert.Equal(2, _issues.Count);
-                Assert.Equal("No output has been detected in the last 10 minutes and the process has not yet exited. This step may have stalled and might require some investigation.", _issues[0].Item1.Message);
-                Assert.Equal("No output has been detected in the last 20 minutes and the process has not yet exited. This step may have stalled and might require some investigation.", _issues[1].Item1.Message);
-                Assert.Equal(DTWebApi.IssueType.Warning, _issues[0].Item1.Type);
-                Assert.Equal(DTWebApi.IssueType.Warning, _issues[1].Item1.Type);
-
-                // Should reset timer
-                manager.OnDataReceived(null, null);
-
-                Assert.True(timer._reset);
-                Assert.Equal(2, _issues.Count);
-
-                // Trigger another elapsed interval
-                timer.TimeElapsed();
-
-                // Timer should have reset and one new warning should have been added
-                Assert.Equal(3, _issues.Count);
-                Assert.Equal("No output has been detected in the last 10 minutes and the process has not yet exited. This step may have stalled and might require some investigation.", _issues[2].Item1.Message);
-                Assert.Equal(DTWebApi.IssueType.Warning, _issues[2].Item1.Type);
-
-            }
-        }
-    }
-}