From 4855b20140b50a494116b144ecaaa311cf9f4ad2 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 16:40:38 +0000 Subject: [PATCH 1/2] Delegate git process invocation to ktsu.RunCommand [patch] GitRunner now runs git through RunCommand.ExecuteAsync instead of driving a Process by hand. RunCommand owns starting the process, reading both streams, killing the whole tree on cancellation, and giving up on reads once the process is gone. GitRunner keeps what is particular to git: the GIT_* and GIT_CONFIG_* environment, strict UTF-8 decoding, and telling its own timeout apart from the caller giving up. - Standard input is closed through StandardInputMode.Closed (RunCommand 1.9.0). - The environment is built as an overlay. Each inherited GIT_* variable maps to null, which removes it from the child's environment. - A decode failure is still reported as a GitResult, including when it arrives wrapped. - The bounded post-kill drain (DrainTimeout) goes away. RunCommand abandons the reads after a kill instead of waiting for a pipe a grandchild holds open. New tests cover a command that reads standard input, and output that is not valid UTF-8, both alone and mixed with valid text. Fixes ktsu-dev/GitBranchStateCache#27 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01TAvt7dvjcukkH3y6mtUL16 --- Directory.Packages.props | 1 + .../Git/GitRunnerTests.cs | 95 ++++++--- GitBranchStateCache/Git/GitRunner.cs | 187 +++++++----------- .../GitBranchStateCache.csproj | 1 + 4 files changed, 141 insertions(+), 143 deletions(-) diff --git a/Directory.Packages.props b/Directory.Packages.props index fa6684c..99064ea 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -5,6 +5,7 @@ + diff --git a/GitBranchStateCache.Tests/Git/GitRunnerTests.cs b/GitBranchStateCache.Tests/Git/GitRunnerTests.cs index 53c2fbe..37387ef 100644 --- a/GitBranchStateCache.Tests/Git/GitRunnerTests.cs +++ b/GitBranchStateCache.Tests/Git/GitRunnerTests.cs @@ -2,6 +2,7 @@ namespace ktsu.GitBranchStateCache.Tests.Git; +using System.Collections; using System.Diagnostics; using System.Runtime.InteropServices; using ktsu.GitBranchStateCache.Configuration; @@ -144,35 +145,35 @@ private static string OutsideAnyRepository() } [TestMethod] - public void ApplyEnvironment_SetsTheFlagsThatKeepARunPredictable() + public void BuildEnvironment_SetsTheFlagsThatKeepARunPredictable() { - ProcessStartInfo startInfo = new(); - startInfo.Environment["GIT_DIR"] = "/somewhere/inherited"; - - GitRunner.ApplyEnvironment( - startInfo, + Dictionary environment = GitRunner.BuildEnvironment( new GitInvocation { Arguments = ["--version"], Timeout = TimeSpan.FromSeconds(1) }, - new GitBranchStateCacheOptions { MirrorRoot = TempRoot }); + new GitBranchStateCacheOptions { MirrorRoot = TempRoot }, + new Hashtable { ["GIT_DIR"] = "/somewhere/inherited", ["PATH"] = "/usr/bin" }); // GIT_NO_LAZY_FETCH turns a demand for filtered content into a visible error rather than an // enormous unplanned fetch, and no terminal prompt turns a missing credential into a refusal // rather than a process waiting on a terminal that is not there. - Assert.AreEqual("1", startInfo.Environment["GIT_NO_LAZY_FETCH"]); - Assert.AreEqual("0", startInfo.Environment["GIT_TERMINAL_PROMPT"]); - Assert.AreEqual("1", startInfo.Environment["GIT_CONFIG_NOSYSTEM"]); + Assert.AreEqual("1", environment["GIT_NO_LAZY_FETCH"]); + Assert.AreEqual("0", environment["GIT_TERMINAL_PROMPT"]); + Assert.AreEqual("1", environment["GIT_CONFIG_NOSYSTEM"]); + + // An inherited GIT_DIR would point every run at a repository nobody asked for. A null value in + // the overlay is what removes it from the child's environment. + Assert.IsTrue(environment.TryGetValue("GIT_DIR", out string? gitDir)); + Assert.IsNull(gitDir); - // An inherited GIT_DIR would point every run at a repository nobody asked for. - Assert.IsFalse(startInfo.Environment.ContainsKey("GIT_DIR")); + // Everything else is inherited untouched, so it is left out of the overlay. + Assert.IsFalse(environment.ContainsKey("PATH")); } [TestMethod] - public void ApplyEnvironment_WithACredential_ScopesItToTheUpstream() + public void BuildEnvironment_WithACredential_ScopesItToTheUpstream() { const string credential = "Basic dXNlcjp0b2tlbg=="; - ProcessStartInfo startInfo = new(); - GitRunner.ApplyEnvironment( - startInfo, + Dictionary environment = GitRunner.BuildEnvironment( new GitInvocation { Arguments = ["ls-remote", "https://github.com/studio/game.git"], @@ -180,30 +181,68 @@ public void ApplyEnvironment_WithACredential_ScopesItToTheUpstream() Authorization = credential, Timeout = TimeSpan.FromSeconds(1), }, - new GitBranchStateCacheOptions { MirrorRoot = TempRoot }); + new GitBranchStateCacheOptions { MirrorRoot = TempRoot }, + new Hashtable()); // Scoped to the upstream rather than set for all of http, because git matches this // configuration by URL prefix and a redirect leading off the forge would otherwise carry the // caller's credential with it. - Assert.AreEqual("2", startInfo.Environment["GIT_CONFIG_COUNT"]); - Assert.AreEqual("http.https://github.com/.extraHeader", startInfo.Environment["GIT_CONFIG_KEY_1"]); - Assert.AreEqual($"Authorization: {credential}", startInfo.Environment["GIT_CONFIG_VALUE_1"]); + Assert.AreEqual("2", environment["GIT_CONFIG_COUNT"]); + Assert.AreEqual("http.https://github.com/.extraHeader", environment["GIT_CONFIG_KEY_1"]); + Assert.AreEqual($"Authorization: {credential}", environment["GIT_CONFIG_VALUE_1"]); } [TestMethod] - public void ApplyEnvironment_WithoutACredential_SetsNoHeader() + public void BuildEnvironment_WithoutACredential_SetsNoHeader() { - ProcessStartInfo startInfo = new(); - - GitRunner.ApplyEnvironment( - startInfo, + Dictionary environment = GitRunner.BuildEnvironment( new GitInvocation { Arguments = ["--version"], Timeout = TimeSpan.FromSeconds(1) }, - new GitBranchStateCacheOptions { MirrorRoot = TempRoot }); + new GitBranchStateCacheOptions { MirrorRoot = TempRoot }, + new Hashtable()); + + Assert.AreEqual("1", environment["GIT_CONFIG_COUNT"]); + Assert.AreEqual("credential.helper", environment["GIT_CONFIG_KEY_0"]); + } - Assert.AreEqual("1", startInfo.Environment["GIT_CONFIG_COUNT"]); - Assert.AreEqual("credential.helper", startInfo.Environment["GIT_CONFIG_KEY_0"]); + [TestMethod] + public async Task RunAsync_ACommandThatReadsStandardInput_SeesEndOfStreamRatherThanWaiting() + { + // git is never fed anything, so a child that reads standard input has to find it closed. Left + // inherited, it would wait on whatever this service's own standard input is, which under a + // service manager can be a pipe nobody writes to, and the run would end only at its timeout. + GitResult result = await Build(ReaderExecutable()).RunAsync( + new GitInvocation { Arguments = ReaderArguments(), Timeout = TimeSpan.FromSeconds(20) }, + CancellationToken.None); + + Assert.IsFalse(result.TimedOut, "The command waited on standard input until it was killed."); + Assert.IsTrue(result.Succeeded, result.StandardError); + Assert.Contains("eof", result.StandardOutput); } + [TestMethod] + [OSCondition(OperatingSystems.Linux | OperatingSystems.OSX)] + [DataRow("before\\n\\377\\376\\nafter\\n", DisplayName = "invalid bytes among valid text")] + [DataRow("\\377\\376", DisplayName = "invalid bytes alone")] + public async Task RunAsync_OutputThatIsNotUtf8_IsReportedRatherThanReadAsEmptyOrReplaced(string printfFormat) + { + // An undecodable branch or path name must fail loudly. Read leniently it becomes a name that + // matches nothing, and read as nothing it becomes "no branches", both of which look like + // answers. POSIX only, because it needs a shell that can write raw bytes. + GitResult result = await Build("/bin/sh").RunAsync( + new GitInvocation { Arguments = ["-c", $"printf '{printfFormat}'"], Timeout = TimeSpan.FromSeconds(20) }, + CancellationToken.None); + + Assert.IsFalse(result.Succeeded); + Assert.IsFalse(result.TimedOut); + Assert.Contains("not valid UTF-8", result.StandardError); + } + + private static string ReaderExecutable() => OnWindows ? "cmd.exe" : "/bin/sh"; + + private static string[] ReaderArguments() => OnWindows + ? ["/c", "set /p line= & echo eof"] + : ["-c", "if read line; then echo \"read:$line\"; else echo eof; fi"]; + [TestMethod] public async Task RunAsync_ExceedingItsTimeout_ReportsTimedOutAndKillsTheTree() { diff --git a/GitBranchStateCache/Git/GitRunner.cs b/GitBranchStateCache/Git/GitRunner.cs index 7ab4ee4..42bbad7 100644 --- a/GitBranchStateCache/Git/GitRunner.cs +++ b/GitBranchStateCache/Git/GitRunner.cs @@ -2,9 +2,11 @@ namespace ktsu.GitBranchStateCache.Git; -using System.Diagnostics; using System.Text; using ktsu.GitBranchStateCache.Configuration; +using ktsu.RunCommand; +using ktsu.Semantics.Paths; +using ktsu.Semantics.Strings; using Microsoft.Extensions.Options; /// @@ -36,16 +38,6 @@ namespace ktsu.GitBranchStateCache.Git; /// The configured options. public sealed class GitRunner(IOptions options) : IGitRunner { - /// - /// How long the output streams are drained for after a kill before they are given up on. - /// - /// - /// A killed process closes its pipes, so this normally completes immediately. It is bounded - /// because a grandchild that inherited the pipe and outlived the kill would otherwise hold the - /// read open, and this path already runs on a request that is being abandoned. - /// - private static readonly TimeSpan DrainTimeout = TimeSpan.FromSeconds(5); - /// /// Decodes git output strictly, so an undecodable path fails loudly instead of being replaced. /// @@ -59,19 +51,40 @@ public sealed class GitRunner(IOptions options) : IG throwOnInvalidBytes: true); /// + /// + /// Starting, reading and killing the process is 's + /// job: it kills the whole tree on cancellation, because git delegates transport to a helper child + /// that would otherwise be left holding a connection and a pipe, and it stops reading once the + /// process is gone rather than waiting on a pipe a surviving grandchild may hold open. What stays + /// here is what is particular to git: the environment, and telling this service's own timeout + /// apart from the caller giving up. + /// public async Task RunAsync(GitInvocation invocation, CancellationToken cancellationToken) { Ensure.NotNull(invocation); - using Process process = new() { StartInfo = BuildStartInfo(invocation) }; - process.Start(); + GitBranchStateCacheOptions settings = options.Value; - // git is never fed anything, and a child holding an open stdin it is waiting on is a hang - // rather than an error. - process.StandardInput.Close(); + StringBuilder standardOutput = new(); + StringBuilder standardError = new(); + OutputHandler output = new( + chunk => standardOutput.Append(chunk), + chunk => standardError.Append(chunk), + StrictUtf8); - Task standardOutput = process.StandardOutput.ReadToEndAsync(CancellationToken.None); - Task standardError = process.StandardError.ReadToEndAsync(CancellationToken.None); + CommandOptions commandOptions = new() + { + // Resolved here because the option only takes an absolute path, and a relative one has always + // meant relative to this process's current directory. + WorkingDirectory = invocation.WorkingDirectory is null + ? null + : Path.GetFullPath(invocation.WorkingDirectory).As(), + EnvironmentVariables = BuildEnvironment(invocation, settings, Environment.GetEnvironmentVariables()), + + // git is never fed anything, and a child holding an open stdin it is waiting on is a hang + // rather than an error. + StandardInput = StandardInputMode.Closed, + }; using CancellationTokenSource timeout = new(invocation.Timeout); using CancellationTokenSource linked = @@ -79,29 +92,24 @@ public async Task RunAsync(GitInvocation invocation, CancellationToke try { - await process.WaitForExitAsync(linked.Token).ConfigureAwait(false); + int exitCode = await RunCommand.ExecuteAsync( + settings.GitExecutable, + invocation.Arguments, + output, + commandOptions, + linked.Token).ConfigureAwait(false); + + return new GitResult(exitCode, standardOutput.ToString(), standardError.ToString(), TimedOut: false); } catch (OperationCanceledException) { - Kill(process); - await DrainAsync(standardOutput, standardError).ConfigureAwait(false); - // A timeout is this service's own decision and has an answer to report. A cancellation is // the caller giving up, and there is nobody left to report anything to. cancellationToken.ThrowIfCancellationRequested(); return new GitResult(-1, string.Empty, "The git command exceeded its timeout.", TimedOut: true); } - - try - { - return new GitResult( - process.ExitCode, - await standardOutput.ConfigureAwait(false), - await standardError.ConfigureAwait(false), - TimedOut: false); - } - catch (DecoderFallbackException) + catch (Exception failure) when (IsDecodeFailure(failure)) { return new GitResult( -1, @@ -112,118 +120,67 @@ await standardError.ConfigureAwait(false), } /// - /// Kills the process and everything it started. + /// Whether a failure is the strict encoding refusing git's output. /// /// - /// The tree, not just the process: git delegates transport to a helper child, and killing only the - /// parent leaves that helper holding a connection and a pipe. Leaking those is the most likely - /// operational failure of a service shaped like this. + /// The output is read on background tasks, so the decoder's exception can arrive wrapped. /// - private static void Kill(Process process) + private static bool IsDecodeFailure(Exception failure) => failure switch { - try - { - if (!process.HasExited) - { - process.Kill(entireProcessTree: true); - } - } - catch (InvalidOperationException) - { - // The process exited between the check and the kill. Nothing left to do. - } - catch (NotSupportedException) - { - // Killing a tree is unsupported on this platform, and the process is already gone or will - // be reaped when its handle is disposed. - } - } - - private static async Task DrainAsync(Task standardOutput, Task standardError) - { - try - { - await Task.WhenAll(standardOutput, standardError).WaitAsync(DrainTimeout).ConfigureAwait(false); - } - catch (Exception failure) when (failure is TimeoutException or DecoderFallbackException) - { - // The output of a killed command is not reported, so failing to read it changes nothing. - } - } - - private ProcessStartInfo BuildStartInfo(GitInvocation invocation) - { - GitBranchStateCacheOptions settings = options.Value; - - ProcessStartInfo startInfo = new() - { - FileName = settings.GitExecutable, - UseShellExecute = false, - CreateNoWindow = true, - RedirectStandardInput = true, - RedirectStandardOutput = true, - RedirectStandardError = true, - StandardOutputEncoding = StrictUtf8, - StandardErrorEncoding = StrictUtf8, - }; - - if (invocation.WorkingDirectory is not null) - { - startInfo.WorkingDirectory = invocation.WorkingDirectory; - } - - foreach (string argument in invocation.Arguments) - { - startInfo.ArgumentList.Add(argument); - } - - ApplyEnvironment(startInfo, invocation, settings); - return startInfo; - } + DecoderFallbackException => true, + AggregateException aggregate => aggregate.Flatten().InnerExceptions.Any(IsDecodeFailure), + _ => failure.InnerException is not null && IsDecodeFailure(failure.InnerException), + }; /// - /// Applies the environment every run gets, including the caller's credential. + /// Builds the environment every run gets, including the caller's credential, as an overlay on the + /// environment the child would otherwise inherit. /// /// /// Internal so the tests can assert on the environment directly. What it puts where is the whole /// of this class's security posture, and asserting it through the behaviour of a child process /// would only ever cover the parts a child happens to report. /// - /// The process being prepared. /// What is being run. /// The configured options. - internal static void ApplyEnvironment( - ProcessStartInfo startInfo, + /// The environment the child would otherwise inherit. + /// The variables to set, with a null value for each inherited variable to remove. + internal static Dictionary BuildEnvironment( GitInvocation invocation, - GitBranchStateCacheOptions settings) + GitBranchStateCacheOptions settings, + System.Collections.IDictionary inherited) { - foreach (string inherited in startInfo.Environment.Keys - .Where(key => key.StartsWith("GIT_", StringComparison.OrdinalIgnoreCase)) - .ToArray()) + Ensure.NotNull(inherited); + + Dictionary environment = new(StringComparer.OrdinalIgnoreCase); + + foreach (string name in inherited.Keys.OfType() + .Where(key => key.StartsWith("GIT_", StringComparison.OrdinalIgnoreCase))) { - startInfo.Environment.Remove(inherited); + environment[name] = null; } - startInfo.Environment["GIT_CONFIG_NOSYSTEM"] = "1"; - startInfo.Environment["GIT_CONFIG_GLOBAL"] = GlobalConfigPath(settings); + environment["GIT_CONFIG_NOSYSTEM"] = "1"; + environment["GIT_CONFIG_GLOBAL"] = GlobalConfigPath(settings); // No prompting, ever. Without this a missing or refused credential turns a request into a // process waiting on a terminal that is not there, which presents as a hang rather than a 401. - startInfo.Environment["GIT_TERMINAL_PROMPT"] = "0"; - startInfo.Environment["GCM_INTERACTIVE"] = "never"; + environment["GIT_TERMINAL_PROMPT"] = "0"; + environment["GCM_INTERACTIVE"] = "never"; // The mirrors are blobless, and nothing this service runs reads file content. If some future // operation does, this turns it into a visible error during testing rather than an enormous // unplanned fetch in production. - startInfo.Environment["GIT_NO_LAZY_FETCH"] = "1"; + environment["GIT_NO_LAZY_FETCH"] = "1"; - ApplyConfigEnvironment(startInfo, invocation); + ApplyConfigEnvironment(environment, invocation); + return environment; } /// /// Hands git its per-run configuration, including the caller's credential, through the environment. /// - private static void ApplyConfigEnvironment(ProcessStartInfo startInfo, GitInvocation invocation) + private static void ApplyConfigEnvironment(Dictionary environment, GitInvocation invocation) { List> entries = [ @@ -242,12 +199,12 @@ private static void ApplyConfigEnvironment(ProcessStartInfo startInfo, GitInvoca $"Authorization: {authorization}")); } - startInfo.Environment["GIT_CONFIG_COUNT"] = entries.Count.ToString(System.Globalization.CultureInfo.InvariantCulture); + environment["GIT_CONFIG_COUNT"] = entries.Count.ToString(System.Globalization.CultureInfo.InvariantCulture); for (int index = 0; index < entries.Count; index++) { - startInfo.Environment[$"GIT_CONFIG_KEY_{index}"] = entries[index].Key; - startInfo.Environment[$"GIT_CONFIG_VALUE_{index}"] = entries[index].Value; + environment[$"GIT_CONFIG_KEY_{index}"] = entries[index].Key; + environment[$"GIT_CONFIG_VALUE_{index}"] = entries[index].Value; } } diff --git a/GitBranchStateCache/GitBranchStateCache.csproj b/GitBranchStateCache/GitBranchStateCache.csproj index 9c3a722..409469b 100644 --- a/GitBranchStateCache/GitBranchStateCache.csproj +++ b/GitBranchStateCache/GitBranchStateCache.csproj @@ -19,6 +19,7 @@ +