From 2eeb90a944dcc6887048d2f631fd350d6f516f47 Mon Sep 17 00:00:00 2001 From: Wes Walker Date: Fri, 9 Dec 2022 01:01:49 +0100 Subject: [PATCH] [1742] Ensure multiple composite annoations are correctly written. This implementation uses a collector pattern to allow embedded ExecutionContexts to stash Issue objects for later processing by a non-embedded ancestor ExecutionContext. Also: - Provide explicit constructor implementations for ExecutionContext - Leverage explicit constructors to solidify immutability of several ExecutionContext class members. - Fixed erroneous call to ExecutionContext.Complete in CompositeActionHandler.cs - Use a consistent timestamp for FinishTime in ExecutionContext::Complete --- src/Runner.Worker/ExecutionContext.cs | 63 +++++++++++++++---- .../Handlers/CompositeActionHandler.cs | 2 +- 2 files changed, 52 insertions(+), 13 deletions(-) diff --git a/src/Runner.Worker/ExecutionContext.cs b/src/Runner.Worker/ExecutionContext.cs index 952ba2c50..465e2ed14 100644 --- a/src/Runner.Worker/ExecutionContext.cs +++ b/src/Runner.Worker/ExecutionContext.cs @@ -124,8 +124,10 @@ namespace GitHub.Runner.Worker private readonly TimelineRecord _record = new(); private readonly Dictionary _detailRecords = new(); + private readonly List _embeddedIssueCollector; private readonly object _loggerLock = new(); private readonly object _matchersLock = new(); + private readonly ExecutionContext _parentExecutionContext; private event OnMatcherChanged _onMatcherChanged; @@ -133,7 +135,6 @@ namespace GitHub.Runner.Worker private IPagingLogger _logger; private IJobServerQueue _jobServerQueue; - private ExecutionContext _parentExecutionContext; private Guid _mainTimelineId; private Guid _detailTimelineId; @@ -147,6 +148,29 @@ namespace GitHub.Runner.Worker private long _totalThrottlingDelayInMilliseconds = 0; private bool _stepTelemetryPublished = false; + public ExecutionContext() + : this(null, false) + { + } + + private ExecutionContext(ExecutionContext parent, bool embedded) + { + if (embedded) + { + ArgUtil.NotNull(parent, nameof(parent)); + } + + _parentExecutionContext = parent; + this.IsEmbedded = embedded; + this.StepTelemetry = new ActionsStepTelemetry + { + IsEmbedded = embedded + }; + + //Embedded Execution Contexts pseudo-inherit their parent's embeddedIssueCollector. + _embeddedIssueCollector = embedded ? parent._embeddedIssueCollector : new(); + } + public Guid Id => _record.Id; public Guid EmbeddedId { get; private set; } public string ScopeName { get; private set; } @@ -159,7 +183,7 @@ namespace GitHub.Runner.Worker public Dictionary JobOutputs { get; private set; } public ActionsEnvironmentReference ActionsEnvironment { get; private set; } - public ActionsStepTelemetry StepTelemetry { get; } = new ActionsStepTelemetry(); + public ActionsStepTelemetry StepTelemetry { get; private init; } public DictionaryContextData ExpressionValues { get; } = new DictionaryContextData(); public IList ExpressionFunctions { get; } = new List(); @@ -184,7 +208,7 @@ namespace GitHub.Runner.Worker // An embedded execution context shares the same record ID, record name, and logger // as its enclosing execution context. - public bool IsEmbedded { get; private set; } + public bool IsEmbedded { get; private init; } public TaskResult? Result { @@ -319,7 +343,7 @@ namespace GitHub.Runner.Worker { Trace.Entering(); - var child = new ExecutionContext(); + var child = new ExecutionContext(this, isEmbedded); child.Initialize(HostContext); child.Global = Global; child.ScopeName = scopeName; @@ -344,7 +368,6 @@ namespace GitHub.Runner.Worker child.ExpressionFunctions.Add(item); } child._cancellationTokenSource = cancellationTokenSource ?? new CancellationTokenSource(); - child._parentExecutionContext = this; child.EchoOnActionCommand = EchoOnActionCommand; if (recordOrder != null) @@ -365,11 +388,9 @@ namespace GitHub.Runner.Worker child._logger.Setup(_mainTimelineId, recordId); } - child.IsEmbedded = isEmbedded; child.StepTelemetry.StepId = recordId; child.StepTelemetry.Stage = stage.ToString(); - child.StepTelemetry.IsEmbedded = isEmbedded; - child.StepTelemetry.StepContextName = child.GetFullyQualifiedContextName(); ; + child.StepTelemetry.StepContextName = child.GetFullyQualifiedContextName(); return child; } @@ -411,13 +432,21 @@ namespace GitHub.Runner.Worker this.Warning($"The job has experienced {TimeSpan.FromMilliseconds(_totalThrottlingDelayInMilliseconds).TotalSeconds} seconds total delay caused by server throttling."); } + DateTime now = DateTime.UtcNow; _record.CurrentOperation = currentOperation ?? _record.CurrentOperation; _record.ResultCode = resultCode ?? _record.ResultCode; - _record.FinishTime = DateTime.UtcNow; + _record.FinishTime = now; _record.PercentComplete = 100; _record.Result = _record.Result ?? TaskResult.Succeeded; _record.State = TimelineRecordState.Completed; + foreach (var issue in _embeddedIssueCollector) + { + // Before our main timeline's final QueueTimelineRecordUpdate, + // inject any issues collected by embedded ExecutionContexts. + AddIssue(issue); + } + _jobServerQueue.QueueTimelineRecordUpdate(_mainTimelineId, _record); // complete all detail timeline records. @@ -425,7 +454,7 @@ namespace GitHub.Runner.Worker { foreach (var record in _detailRecords) { - record.Value.FinishTime = record.Value.FinishTime ?? DateTime.UtcNow; + record.Value.FinishTime = record.Value.FinishTime ?? now; record.Value.PercentComplete = record.Value.PercentComplete ?? 100; record.Value.Result = record.Value.Result ?? TaskResult.Succeeded; record.Value.State = TimelineRecordState.Completed; @@ -614,7 +643,17 @@ namespace GitHub.Runner.Worker _record.NoticeCount++; } - _jobServerQueue.QueueTimelineRecordUpdate(_mainTimelineId, _record); + // Embedded ExecutionContexts (a.k.a. Composite actions) should never upload a timeline record to the server. + // Instead, we store processed issues on a shared (psuedo-inherited) list (belonging to the closest + // non-embedded ancestor ExecutionContext) so that they can be processed when that ancestor completes. + if (this.IsEmbedded) + { + _embeddedIssueCollector.Add(issue); + } + else + { + _jobServerQueue.QueueTimelineRecordUpdate(_mainTimelineId, _record); + } } public void UpdateDetailTimelineRecord(TimelineRecord record) @@ -1085,7 +1124,7 @@ namespace GitHub.Runner.Worker { if (contextData != null && contextData.TryGetValue(PipelineTemplateConstants.Vars, out var varsPipelineContextData) && - varsPipelineContextData != null && + varsPipelineContextData != null && varsPipelineContextData is DictionaryContextData varsContextData) { // Set debug variables only when StepDebug/RunnerDebug variables are not present. diff --git a/src/Runner.Worker/Handlers/CompositeActionHandler.cs b/src/Runner.Worker/Handlers/CompositeActionHandler.cs index 4cc54af41..7728ad886 100644 --- a/src/Runner.Worker/Handlers/CompositeActionHandler.cs +++ b/src/Runner.Worker/Handlers/CompositeActionHandler.cs @@ -294,7 +294,7 @@ namespace GitHub.Runner.Worker.Handlers // Evaluation error Trace.Info("Caught exception from expression for embedded step.env"); step.ExecutionContext.Error(ex); - step.ExecutionContext.Complete(TaskResult.Failed); + SetStepConclusion(step, TaskResult.Failed); } // Register Callback