Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions csharp/autobuilder/Semmle.Autobuild.CSharp/DotNetRule.cs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public BuildScript Analyse(IAutobuilder<CSharpAutobuildOptions> builder, bool au
{
// When a custom .NET CLI has been installed, `dotnet --info` has already been executed
// to verify the installation.
var ret = dotNetPath is null ? GetInfoCommand(builder.Actions, dotNetPath, environment) : BuildScript.Success;
var ret = dotNetPath is null ? DotNet.InfoScript(builder.Actions, DotNetCommand(builder.Actions, dotNetPath), environment, builder.Logger) : BuildScript.Success;
foreach (var projectOrSolution in builder.ProjectsOrSolutionsToBuild)
{
var cleanCommand = GetCleanCommand(builder.Actions, dotNetPath, environment);
Expand Down Expand Up @@ -111,14 +111,6 @@ public static BuildScript WithDotNet(IAutobuilder<AutobuildOptionsShared> builde
private static string DotNetCommand(IBuildActions actions, string? dotNetPath) =>
dotNetPath is not null ? actions.PathCombine(dotNetPath, "dotnet") : "dotnet";

private static BuildScript GetInfoCommand(IBuildActions actions, string? dotNetPath, IDictionary<string, string>? environment)
{
var info = new CommandBuilder(actions, null, environment).
RunCommand(DotNetCommand(actions, dotNetPath)).
Argument("--info");
return info.Script;
}

private static CommandBuilder GetCleanCommand(IBuildActions actions, string? dotNetPath, IDictionary<string, string>? environment)
{
var clean = new CommandBuilder(actions, null, environment).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using System.Collections.ObjectModel;
using System.IO;
using System.Linq;
using System.Threading;
using Newtonsoft.Json.Linq;

using Semmle.Util;
Expand Down Expand Up @@ -36,12 +37,29 @@ private DotNet(ILogger logger, string? dotNetPath, TemporaryDirectory tempWorkin

public static IDotNet Make(ILogger logger, string? dotNetPath, TemporaryDirectory tempWorkingDirectory, DependabotProxy? dependabotProxy) => new DotNet(logger, dotNetPath, tempWorkingDirectory, dependabotProxy);

private static void HandleRetryExitCode143(string dotnet, int attempt, ILogger logger)
{
logger.LogWarning($"Running '{dotnet} --info' failed with exit code 143. Retrying...");
var sleep = Math.Pow(2, attempt) * 1000;
Thread.Sleep((int)sleep);
}

private void Info()
{
var res = dotnetCliInvoker.RunCommand("--info", silent: false);
if (!res)
// Allow up to three retry attempts to run `dotnet --info`, to mitigate transient issues
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says "Allow up to three retry attempts" but the loop runs 4 times (attempt = 0, 1, 2, 3). This means there are up to 3 retries after the initial attempt, for a total of 4 attempts. Consider updating the comment to clarify this is "up to 4 attempts (with 3 retries)" or adjust the loop to match the comment.

Suggested change
// Allow up to three retry attempts to run `dotnet --info`, to mitigate transient issues
// Allow up to four attempts (with up to three retries) to run `dotnet --info`, to mitigate transient issues

Copilot uses AI. Check for mistakes.
for (int attempt = 0; attempt < 4; attempt++)
{
throw new Exception($"{dotnetCliInvoker.Exec} --info failed.");
var exitCode = dotnetCliInvoker.RunCommandExitCode("--info", silent: false);
switch (exitCode)
{
case 0:
return;
case 143 when attempt < 3:
HandleRetryExitCode143(dotnetCliInvoker.Exec, attempt, logger);
break;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be a continue?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The break statement is in the scope of the switch statement to prevent fall-through (it doesn't break out of the loop). As there are no statements after the loop, it should be equivalent to use continue here. Do you prefer that?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, it is for the switch. Perhaps continue is better to avoid confusion.

default:
throw new Exception($"{dotnetCliInvoker.Exec} --info failed with exit code {exitCode}.");
}
}
}

Expand Down Expand Up @@ -193,6 +211,34 @@ private static BuildScript DownloadDotNet(IBuildActions actions, ILogger logger,
return BuildScript.Failure;
}

/// <summary>
/// Returns a script for running `dotnet --info`, with retries on exit code 143.
/// </summary>
public static BuildScript InfoScript(IBuildActions actions, string dotnet, IDictionary<string, string>? environment, ILogger logger)
{
var info = new CommandBuilder(actions, null, environment).
RunCommand(dotnet).
Argument("--info");
var script = info.Script;
for (int attempt = 0; attempt < 4; attempt++)
{
script = BuildScript.Bind(script, ret =>
{
switch (ret)
{
case 0:
return BuildScript.Success;
case 143 when attempt < 3:
HandleRetryExitCode143(dotnet, attempt, logger);
return info.Script;
default:
return BuildScript.Failure;
}
});
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The loop variable attempt is captured in the closure passed to BuildScript.Bind, which will be executed later. Due to closure semantics in C#, the value of attempt will be its final value (3) when the script runs, not the value from each iteration. This means:

  1. The condition case 143 when attempt < 3 will always be true on the 4th iteration (when attempt == 3)
  2. The sleep duration calculation in HandleRetryExitCode will always use attempt=3, resulting in a sleep of 8 seconds on every retry instead of exponential backoff (1, 2, 4 seconds)

To fix this, capture the loop variable in a local variable:

for (int i = 0; i < 4; i++)
{
    var attempt = i; // Capture in local variable
    script = BuildScript.Bind(script, ret =>
    {
        switch (ret)
        {
            case 0:
                return BuildScript.Success;
            case 143 when attempt < 3:
                HandleRetryExitCode143(dotnet, attempt, logger);
                return info.Script;
            default:
                return BuildScript.Failure;
        }
    });
}
Suggested change
script = BuildScript.Bind(script, ret =>
{
switch (ret)
{
case 0:
return BuildScript.Success;
case 143 when attempt < 3:
HandleRetryExitCode143(dotnet, attempt, logger);
return info.Script;
default:
return BuildScript.Failure;
}
});
var attemptCopy = attempt; // Capture in local variable
script = BuildScript.Bind(script, ret =>
{
switch (ret)
{
case 0:
return BuildScript.Success;
case 143 when attemptCopy < 3:
HandleRetryExitCode143(dotnet, attemptCopy, logger);
return info.Script;
default:
return BuildScript.Failure;
}
});

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn, you got me there CoPilot!!

}
return script;
}

/// <summary>
/// Returns a script for downloading specific .NET SDK versions, if the
/// versions are not already installed.
Expand Down Expand Up @@ -292,9 +338,7 @@ BuildScript GetInstall(string pwsh) =>
};
}

var dotnetInfo = new CommandBuilder(actions, environment: MinimalEnvironment).
RunCommand(actions.PathCombine(path, "dotnet")).
Argument("--info").Script;
var dotnetInfo = InfoScript(actions, actions.PathCombine(path, "dotnet"), MinimalEnvironment.ToDictionary(), logger);

Func<string, BuildScript> getInstallAndVerify = version =>
// run `dotnet --info` after install, to check that it executes successfully
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,21 @@ private ProcessStartInfo MakeDotnetStartInfo(string args, string? workingDirecto
return startInfo;
}

private bool RunCommandAux(string args, string? workingDirectory, out IList<string> output, bool silent)
private int RunCommandExitCodeAux(string args, string? workingDirectory, out IList<string> output, out string dirLog, bool silent)
{
var dirLog = string.IsNullOrWhiteSpace(workingDirectory) ? "" : $" in {workingDirectory}";
dirLog = string.IsNullOrWhiteSpace(workingDirectory) ? "" : $" in {workingDirectory}";
var pi = MakeDotnetStartInfo(args, workingDirectory);
var threadId = Environment.CurrentManagedThreadId;
void onOut(string s) => logger.Log(silent ? Severity.Debug : Severity.Info, s, threadId);
void onError(string s) => logger.LogError(s, threadId);
logger.LogInfo($"Running '{Exec} {args}'{dirLog}");
var exitCode = pi.ReadOutput(out output, onOut, onError);
return exitCode;
}

private bool RunCommandAux(string args, string? workingDirectory, out IList<string> output, bool silent)
{
var exitCode = RunCommandExitCodeAux(args, workingDirectory, out output, out var dirLog, silent);
if (exitCode != 0)
{
logger.LogError($"Command '{Exec} {args}'{dirLog} failed with exit code {exitCode}");
Expand All @@ -77,6 +83,9 @@ private bool RunCommandAux(string args, string? workingDirectory, out IList<stri
public bool RunCommand(string args, bool silent = true) =>
RunCommandAux(args, null, out _, silent);

public int RunCommandExitCode(string args, bool silent = true) =>
RunCommandExitCodeAux(args, null, out _, out _, silent);

public bool RunCommand(string args, out IList<string> output, bool silent = true) =>
RunCommandAux(args, null, out output, silent);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ internal interface IDotNetCliInvoker
/// </summary>
bool RunCommand(string args, bool silent = true);

/// <summary>
/// Execute `dotnet <paramref name="args"/>` and return the exit code.
/// If `silent` is true the output of the command is logged as `debug` otherwise as `info`.
/// </summary>
int RunCommandExitCode(string args, bool silent = true);

/// <summary>
/// Execute `dotnet <paramref name="args"/>` and return true if the command succeeded, otherwise false.
/// The output of the command is returned in `output`.
Expand Down
11 changes: 9 additions & 2 deletions csharp/extractor/Semmle.Extraction.Tests/DotNet.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ internal class DotNetCliInvokerStub : IDotNetCliInvoker
private string lastArgs = "";
public string WorkingDirectory { get; private set; } = "";
public bool Success { get; set; } = true;
public int ExitCode { get; set; } = 0;

public DotNetCliInvokerStub(IList<string> output)
{
Expand All @@ -26,6 +27,12 @@ public bool RunCommand(string args, bool silent)
return Success;
}

public int RunCommandExitCode(string args, bool silent)
{
lastArgs = args;
return ExitCode;
}

public bool RunCommand(string args, out IList<string> output, bool silent)
{
lastArgs = args;
Expand Down Expand Up @@ -83,7 +90,7 @@ public void TestDotnetInfo()
public void TestDotnetInfoFailure()
{
// Setup
var dotnetCliInvoker = new DotNetCliInvokerStub(new List<string>()) { Success = false };
var dotnetCliInvoker = new DotNetCliInvokerStub(new List<string>()) { ExitCode = 1 };

// Execute
try
Expand All @@ -94,7 +101,7 @@ public void TestDotnetInfoFailure()
// Verify
catch (Exception e)
{
Assert.Equal("dotnet --info failed.", e.Message);
Assert.Equal("dotnet --info failed with exit code 1.", e.Message);
return;
}
Assert.Fail("Expected exception");
Expand Down
2 changes: 0 additions & 2 deletions csharp/ql/integration-tests/all-platforms/dotnet_10/test.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import pytest

@pytest.mark.skip(reason=".NET 10 info command crashes")
def test1(codeql, csharp):
codeql.database.create()

@pytest.mark.skip(reason=".NET 10 info command crashes")
def test2(codeql, csharp):
codeql.database.create(build_mode="none")
Loading