From e89148e33e7c232aef368bff05b61ee7e516cd10 Mon Sep 17 00:00:00 2001 From: Lucas Killgore Date: Wed, 23 Oct 2019 12:48:10 -0400 Subject: [PATCH] Don't retry uploads when the http status code response from the server is in the 400's (#131) * Don't retry uploads when the http status code response from the server is in the 400's * Don't retry on fast-fail * Feedback from code review * Always try to attach any uploaded files to the build Don't fast-fail on 'Conflict' * Add dispose * Refactored upload code. Called out specialized 'Conflict' logic. * Added typed exception. --- .../Artifact/FileContainerServer.cs | 157 ++++++++++-------- .../Artifact/PublishArtifact.cs | 25 +-- .../HttpClients/FileContainerHttpClient.cs | 16 ++ 3 files changed, 122 insertions(+), 76 deletions(-) diff --git a/src/Runner.Plugins/Artifact/FileContainerServer.cs b/src/Runner.Plugins/Artifact/FileContainerServer.cs index 037d161d9..2d838bfeb 100644 --- a/src/Runner.Plugins/Artifact/FileContainerServer.cs +++ b/src/Runner.Plugins/Artifact/FileContainerServer.cs @@ -236,15 +236,15 @@ namespace GitHub.Runner.Plugins.Artifact // try upload all files for the first time. UploadResult uploadResult = await ParallelUploadAsync(context, files, maxConcurrentUploads, _uploadCancellationTokenSource.Token); - if (uploadResult.FailedFiles.Count == 0) + if (uploadResult.RetryFiles.Count == 0) { // all files have been upload succeed. - context.Output("File upload succeed."); + context.Output("File upload complete."); return uploadResult.TotalFileSizeUploaded; } else { - context.Output($"{uploadResult.FailedFiles.Count} files failed to upload, retry these files after a minute."); + context.Output($"{uploadResult.RetryFiles.Count} files failed to upload, retry these files after a minute."); } // Delay 1 min then retry failed files. @@ -255,13 +255,13 @@ namespace GitHub.Runner.Plugins.Artifact } // Retry upload all failed files. - context.Output($"Start retry {uploadResult.FailedFiles.Count} failed files upload."); - UploadResult retryUploadResult = await ParallelUploadAsync(context, uploadResult.FailedFiles, maxConcurrentUploads, _uploadCancellationTokenSource.Token); + context.Output($"Start retry {uploadResult.RetryFiles.Count} failed files upload."); + UploadResult retryUploadResult = await ParallelUploadAsync(context, uploadResult.RetryFiles, maxConcurrentUploads, _uploadCancellationTokenSource.Token); - if (retryUploadResult.FailedFiles.Count == 0) + if (retryUploadResult.RetryFiles.Count == 0) { // all files have been upload succeed after retry. - context.Output("File upload succeed after retry."); + context.Output("File upload complete after retry."); return uploadResult.TotalFileSizeUploaded + retryUploadResult.TotalFileSizeUploaded; } else @@ -465,75 +465,61 @@ namespace GitHub.Runner.Plugins.Artifact using (FileStream fs = File.Open(fileToUpload, FileMode.Open, FileAccess.Read, FileShare.Read)) { string itemPath = (_containerPath.TrimEnd('/') + "/" + fileToUpload.Remove(0, _sourceParentDirectory.Length + 1)).Replace('\\', '/'); - uploadTimer.Restart(); - bool catchExceptionDuringUpload = false; - HttpResponseMessage response = null; + bool failAndExit = false; try { - response = await _fileContainerHttpClient.UploadFileAsync(_containerId, itemPath, fs, _projectId, cancellationToken: token, chunkSize: 4 * 1024 * 1024); + uploadTimer.Restart(); + using (HttpResponseMessage response = await _fileContainerHttpClient.UploadFileAsync(_containerId, itemPath, fs, _projectId, cancellationToken: token, chunkSize: 4 * 1024 * 1024)) + { + if (response == null || response.StatusCode != HttpStatusCode.Created) + { + context.Output($"Unable to copy file to server StatusCode={response?.StatusCode}: {response?.ReasonPhrase}. Source file path: {fileToUpload}. Target server path: {itemPath}"); + + if (response?.StatusCode == HttpStatusCode.Conflict) + { + // fail upload task but continue with any other files + context.Error($"Error '{fileToUpload}' has already been uploaded."); + } + else if (_fileContainerHttpClient.IsFastFailResponse(response)) + { + // Fast fail: we received an http status code where we should abandon our efforts + context.Output($"Cannot continue uploading files, so draining upload queue of {_fileUploadQueue.Count} items."); + DrainUploadQueue(context); + failedFiles.Clear(); + failAndExit = true; + throw new UploadFailedException($"Critical failure uploading '{fileToUpload}'"); + } + else + { + context.Debug($"Adding '{fileToUpload}' to retry list."); + failedFiles.Add(fileToUpload); + } + throw new UploadFailedException($"Http failure response '{response?.StatusCode}': '{response?.ReasonPhrase}' while uploading '{fileToUpload}'"); + } + + uploadTimer.Stop(); + context.Debug($"File: '{fileToUpload}' took {uploadTimer.ElapsedMilliseconds} milliseconds to finish upload"); + uploadedSize += fs.Length; + OutputLogForFile(context, fileToUpload, $"Detail upload trace for file: {itemPath}", context.Debug); + } } catch (OperationCanceledException) when (token.IsCancellationRequested) { context.Output($"File upload has been cancelled during upload file: '{fileToUpload}'."); - if (response != null) - { - response.Dispose(); - response = null; - } - throw; } catch (Exception ex) { - catchExceptionDuringUpload = true; context.Output($"Fail to upload '{fileToUpload}' due to '{ex.Message}'."); context.Output(ex.ToString()); - } - uploadTimer.Stop(); - if (catchExceptionDuringUpload || (response != null && response.StatusCode != HttpStatusCode.Created)) - { - if (response != null) + OutputLogForFile(context, fileToUpload, $"Detail upload trace for file that fail to upload: {itemPath}", context.Output); + + if (failAndExit) { - context.Output($"Unable to copy file to server StatusCode={response.StatusCode}: {response.ReasonPhrase}. Source file path: {fileToUpload}. Target server path: {itemPath}"); + context.Debug("Exiting upload."); + throw; } - - // output detail upload trace for the file. - ConcurrentQueue logQueue; - if (_fileUploadTraceLog.TryGetValue(itemPath, out logQueue)) - { - context.Output($"Detail upload trace for file that fail to upload: {itemPath}"); - string message; - while (logQueue.TryDequeue(out message)) - { - context.Output(message); - } - } - - // tracking file that failed to upload. - failedFiles.Add(fileToUpload); - } - else - { - context.Debug($"File: '{fileToUpload}' took {uploadTimer.ElapsedMilliseconds} milliseconds to finish upload"); - uploadedSize += fs.Length; - // debug detail upload trace for the file. - ConcurrentQueue logQueue; - if (_fileUploadTraceLog.TryGetValue(itemPath, out logQueue)) - { - context.Debug($"Detail upload trace for file: {itemPath}"); - string message; - while (logQueue.TryDequeue(out message)) - { - context.Debug(message); - } - } - } - - if (response != null) - { - response.Dispose(); - response = null; } } @@ -590,6 +576,30 @@ namespace GitHub.Runner.Plugins.Artifact } } + private void DrainUploadQueue(RunnerActionPluginExecutionContext context) + { + while (_fileUploadQueue.TryDequeue(out string fileToUpload)) + { + context.Debug($"Clearing upload queue: '{fileToUpload}'"); + Interlocked.Increment(ref _uploadFilesProcessed); + } + } + + private void OutputLogForFile(RunnerActionPluginExecutionContext context, string itemPath, string logDescription, Action log) + { + // output detail upload trace for the file. + ConcurrentQueue logQueue; + if (_fileUploadTraceLog.TryGetValue(itemPath, out logQueue)) + { + log(logDescription); + string message; + while (logQueue.TryDequeue(out message)) + { + log(message); + } + } + } + private void UploadFileTraceReportReceived(object sender, ReportTraceEventArgs e) { ConcurrentQueue logQueue = _fileUploadTraceLog.GetOrAdd(e.File, new ConcurrentQueue()); @@ -607,22 +617,22 @@ namespace GitHub.Runner.Plugins.Artifact { public UploadResult() { - FailedFiles = new List(); + RetryFiles = new List(); TotalFileSizeUploaded = 0; } - public UploadResult(List failedFiles, long totalFileSizeUploaded) + public UploadResult(List retryFiles, long totalFileSizeUploaded) { - FailedFiles = failedFiles; + RetryFiles = retryFiles ?? new List(); TotalFileSizeUploaded = totalFileSizeUploaded; } - public List FailedFiles { get; set; } + public List RetryFiles { get; set; } public long TotalFileSizeUploaded { get; set; } public void AddUploadResult(UploadResult resultToAdd) { - this.FailedFiles.AddRange(resultToAdd.FailedFiles); + this.RetryFiles.AddRange(resultToAdd.RetryFiles); this.TotalFileSizeUploaded += resultToAdd.TotalFileSizeUploaded; } } @@ -657,4 +667,19 @@ namespace GitHub.Runner.Plugins.Artifact this.FailedFiles.AddRange(resultToAdd.FailedFiles); } } + + public class UploadFailedException : Exception + { + public UploadFailedException() + : base() + { } + + public UploadFailedException(string message) + : base(message) + { } + + public UploadFailedException(string message, Exception inner) + : base(message, inner) + { } + } } \ No newline at end of file diff --git a/src/Runner.Plugins/Artifact/PublishArtifact.cs b/src/Runner.Plugins/Artifact/PublishArtifact.cs index e6b87c675..3975c1e91 100644 --- a/src/Runner.Plugins/Artifact/PublishArtifact.cs +++ b/src/Runner.Plugins/Artifact/PublishArtifact.cs @@ -74,17 +74,22 @@ namespace GitHub.Runner.Plugins.Artifact context.Output($"Uploading artifact '{artifactName}' from '{fullPath}' for run #{buildId}"); FileContainerServer fileContainerHelper = new FileContainerServer(context.VssConnection, projectId, containerId, artifactName); - long size = await fileContainerHelper.CopyToContainerAsync(context, fullPath, token); var propertiesDictionary = new Dictionary(StringComparer.OrdinalIgnoreCase); - propertiesDictionary.Add("artifactsize", size.ToString()); - - string fileContainerFullPath = StringUtil.Format($"#/{containerId}/{artifactName}"); - context.Output($"Uploaded '{fullPath}' to server"); - - BuildServer buildHelper = new BuildServer(context.VssConnection); - string jobId = context.Variables.GetValueOrDefault(WellKnownDistributedTaskVariables.JobId).Value ?? string.Empty; - var artifact = await buildHelper.AssociateArtifact(projectId, buildId, jobId, artifactName, ArtifactResourceTypes.Container, fileContainerFullPath, propertiesDictionary, token); - context.Output($"Associated artifact {artifactName} ({artifact.Id}) with run #{buildId}"); + try + { + long size = await fileContainerHelper.CopyToContainerAsync(context, fullPath, token); + propertiesDictionary.Add("artifactsize", size.ToString()); + context.Output($"Uploaded '{size}' bytes from '{fullPath}' to server"); + } + // if any of the results were successful, make sure to attach them to the build + finally + { + string fileContainerFullPath = StringUtil.Format($"#/{containerId}/{artifactName}"); + BuildServer buildHelper = new BuildServer(context.VssConnection); + string jobId = context.Variables.GetValueOrDefault(WellKnownDistributedTaskVariables.JobId).Value ?? string.Empty; + var artifact = await buildHelper.AssociateArtifact(projectId, buildId, jobId, artifactName, ArtifactResourceTypes.Container, fileContainerFullPath, propertiesDictionary, token); + context.Output($"Associated artifact {artifactName} ({artifact.Id}) with run #{buildId}"); + } } } } \ No newline at end of file diff --git a/src/Sdk/WebApi/WebApi/HttpClients/FileContainerHttpClient.cs b/src/Sdk/WebApi/WebApi/HttpClients/FileContainerHttpClient.cs index 4caa5ba8c..57202de4e 100644 --- a/src/Sdk/WebApi/WebApi/HttpClients/FileContainerHttpClient.cs +++ b/src/Sdk/WebApi/WebApi/HttpClients/FileContainerHttpClient.cs @@ -397,6 +397,11 @@ namespace GitHub.Services.FileContainer.Client { break; } + else if (IsFastFailResponse(response)) + { + FileUploadTrace(itemPath, $"Chunk '{currentChunk}' attempt '{attempt}' of file '{itemPath}' received non-success status code {response.StatusCode} for sending request and cannot continue."); + break; + } else { FileUploadTrace(itemPath, $"Chunk '{currentChunk}' attempt '{attempt}' of file '{itemPath}' received non-success status code {response.StatusCode} for sending request."); @@ -538,6 +543,17 @@ namespace GitHub.Services.FileContainer.Client cancellationToken); } + public bool IsFastFailResponse(HttpResponseMessage response) + { + int statusCode = (int)response?.StatusCode; + return statusCode >= 400 && statusCode <= 499; + } + + protected override bool ShouldThrowError(HttpResponseMessage response) + { + return !response.IsSuccessStatusCode && !IsFastFailResponse(response); + } + private async Task ContainerGetRequestAsync( Int64 containerId, String itemPath,