-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Automatically cleanup old artifacts of file-based apps #49666
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
78a816e
18dc1ed
715bbbc
ac0be84
0ddedfb
cc6fcd1
8bae948
7e77262
1baabde
d43c35b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,9 +184,12 @@ The subdirectory is created by the SDK CLI with permissions restricting access t | |
Note that it is possible for multiple users to run the same file-based program, however each user's run uses different build artifacts since the base directory is unique per user. | ||
Apart from keeping the source directory clean, such artifact isolation also avoids clashes of build outputs that are not project-scoped, like `project.assets.json`, in the case of multiple entry-point files. | ||
|
||
Artifacts are cleaned periodically by a background task that is started by `dotnet run` and | ||
removes current user's `dotnet run` build outputs that haven't been used in some time. | ||
Artifacts are cleaned periodically (every 2 days) by a background task that is started by `dotnet run` and | ||
removes current user's `dotnet run` build outputs that haven't been used in 30 days. | ||
They are not cleaned immediately because they can be re-used on subsequent runs for better performance. | ||
The automatic cleanup can be disabled by environment variable `DOTNET_CLI_DISABLE_FILE_BASED_APP_ARTIFACTS_AUTOMATIC_CLEANUP=true`, | ||
but other parameters of the automatic cleanup are currently not configurable. | ||
The same cleanup can be performed manually via command `dotnet clean-file-based-app-artifacts`. | ||
Comment on lines
+190
to
+192
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @baronfel please take a look at these new entry point / env names. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We've already discussed the subcommand in another thread above, but yeah, please also take a look at the env var name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you take a look at the proposed unified/structured config model in https://github.com/dotnet/sdk/pull/49761/files#diff-dd9e3f58690e283b389a68dc9f7bf2923a8bec9a198e9a44da1edb549cca7e0bR11 and see where you think the relevant flag/toggle might live? I'm trying to create a unified config model based on IConfiguration from our current crazy explosion of knobs, and using that to try to inform the structure of what our actual desired configuration models might be. To me, this feels like one of two things:
[file_based_apps]
clean = "auto" vs [clean]
cleanup_file_based_apps = "auto" or similar. cc @DamianEdwards for thoughts here too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might want to have more automatic cleanups in the future as well (not just for file-based apps), so perhaps something like |
||
|
||
## Directives for project metadata | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,190 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.CommandLine; | ||
using System.Diagnostics; | ||
using System.Text.Json; | ||
using Microsoft.DotNet.Cli.Commands.Run; | ||
using Microsoft.DotNet.Cli.Utils; | ||
using Microsoft.DotNet.Cli.Utils.Extensions; | ||
|
||
namespace Microsoft.DotNet.Cli.Commands.Clean.FileBasedAppArtifacts; | ||
|
||
internal sealed class CleanFileBasedAppArtifactsCommand(ParseResult parseResult) : CommandBase(parseResult) | ||
{ | ||
public override int Execute() | ||
{ | ||
bool dryRun = _parseResult.GetValue(CleanFileBasedAppArtifactsCommandParser.DryRunOption); | ||
|
||
using var metadataFileStream = OpenMetadataFile(); | ||
|
||
bool anyErrors = false; | ||
int count = 0; | ||
|
||
foreach (var folder in GetFoldersToRemove()) | ||
{ | ||
if (dryRun) | ||
{ | ||
Reporter.Verbose.WriteLine($"Would remove folder: {folder.FullName}"); | ||
count++; | ||
} | ||
else | ||
{ | ||
try | ||
{ | ||
folder.Delete(recursive: true); | ||
Reporter.Verbose.WriteLine($"Removed folder: {folder.FullName}"); | ||
count++; | ||
} | ||
catch (Exception ex) | ||
{ | ||
Reporter.Error.WriteLine(string.Format(CliCommandStrings.CleanFileBasedAppArtifactsErrorRemovingFolder, folder, ex.Message).Red()); | ||
anyErrors = true; | ||
} | ||
} | ||
} | ||
|
||
Reporter.Output.WriteLine( | ||
dryRun | ||
? CliCommandStrings.CleanFileBasedAppArtifactsWouldRemoveFolders | ||
: CliCommandStrings.CleanFileBasedAppArtifactsTotalFoldersRemoved, | ||
count); | ||
|
||
if (!dryRun) | ||
{ | ||
UpdateMetadata(metadataFileStream); | ||
} | ||
|
||
return anyErrors ? 1 : 0; | ||
} | ||
|
||
private IEnumerable<DirectoryInfo> GetFoldersToRemove() | ||
{ | ||
var directory = new DirectoryInfo(VirtualProjectBuildingCommand.GetTempSubdirectory()); | ||
|
||
if (!directory.Exists) | ||
{ | ||
Reporter.Error.WriteLine(string.Format(CliCommandStrings.CleanFileBasedAppArtifactsDirectoryNotFound, directory.FullName).Yellow()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why report a warning here? Won't that mean that installing the CLI then running the command produces a warning? I intuitively expect it would just return nothing because there was nothing to delete and it successfully deleted nothing 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thinking is that if the directory doesn't exist, something might be wrong (e.g., you have some env var set which changes where the directory points to) and it might be good to alert the user. |
||
yield break; | ||
} | ||
|
||
Reporter.Output.WriteLine(CliCommandStrings.CleanFileBasedAppArtifactsScanning, directory.FullName); | ||
|
||
var days = _parseResult.GetValue(CleanFileBasedAppArtifactsCommandParser.DaysOption); | ||
var cutoff = DateTime.UtcNow.AddDays(-days); | ||
|
||
foreach (var subdir in directory.GetDirectories()) | ||
{ | ||
if (subdir.LastWriteTimeUtc < cutoff) | ||
{ | ||
yield return subdir; | ||
} | ||
} | ||
} | ||
|
||
private static FileInfo GetMetadataFile() | ||
{ | ||
return new FileInfo(VirtualProjectBuildingCommand.GetTempSubpath(RunFileArtifactsMetadata.FilePath)); | ||
} | ||
|
||
private FileStream? OpenMetadataFile() | ||
{ | ||
if (!_parseResult.GetValue(CleanFileBasedAppArtifactsCommandParser.AutomaticOption)) | ||
{ | ||
return null; | ||
} | ||
|
||
// Open and lock the metadata file to ensure we are the only automatic cleanup process. | ||
return GetMetadataFile().Open(FileMode.Create, FileAccess.ReadWrite, FileShare.None); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the case of concurrent calls to this method there will be an exception generated here. How does that manifest in terms of user experience? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is only called during the automatic cleanup which runs in the background and its output is not visible anywhere, so the exception won't be seen. |
||
} | ||
|
||
private static void UpdateMetadata(FileStream? metadataFileStream) | ||
{ | ||
if (metadataFileStream is null) | ||
{ | ||
return; | ||
} | ||
|
||
var metadata = new RunFileArtifactsMetadata | ||
{ | ||
LastAutomaticCleanupUtc = DateTime.UtcNow, | ||
}; | ||
JsonSerializer.Serialize(metadataFileStream, metadata, RunFileJsonSerializerContext.Default.RunFileArtifactsMetadata); | ||
} | ||
|
||
/// <summary> | ||
/// Starts a background process to clean up file-based app artifacts if needed. | ||
/// </summary> | ||
public static void StartAutomaticCleanupIfNeeded() | ||
{ | ||
if (ShouldStartAutomaticCleanup()) | ||
{ | ||
Reporter.Verbose.WriteLine("Starting automatic cleanup of file-based app artifacts."); | ||
|
||
var startInfo = new ProcessStartInfo | ||
{ | ||
FileName = new Muxer().MuxerPath, | ||
Arguments = ArgumentEscaper.EscapeAndConcatenateArgArrayForProcessStart( | ||
[ | ||
CleanCommandParser.GetCommand().Name, | ||
CleanFileBasedAppArtifactsCommandParser.Command.Name, | ||
CleanFileBasedAppArtifactsCommandParser.AutomaticOption.Name, | ||
]), | ||
UseShellExecute = false, | ||
RedirectStandardInput = true, | ||
RedirectStandardOutput = true, | ||
RedirectStandardError = true, | ||
CreateNoWindow = true, | ||
}; | ||
|
||
Process.Start(startInfo); | ||
} | ||
} | ||
|
||
private static bool ShouldStartAutomaticCleanup() | ||
{ | ||
if (Env.GetEnvironmentVariableAsBool("DOTNET_CLI_DISABLE_FILE_BASED_APP_ARTIFACTS_AUTOMATIC_CLEANUP", defaultValue: false)) | ||
{ | ||
return false; | ||
} | ||
|
||
FileInfo? metadataFile = null; | ||
try | ||
{ | ||
metadataFile = GetMetadataFile(); | ||
|
||
if (!metadataFile.Exists) | ||
{ | ||
return true; | ||
} | ||
|
||
using var stream = metadataFile.Open(FileMode.Open, FileAccess.Read, FileShare.Read); | ||
var metadata = JsonSerializer.Deserialize(stream, RunFileJsonSerializerContext.Default.RunFileArtifactsMetadata); | ||
|
||
if (metadata?.LastAutomaticCleanupUtc is not { } timestamp) | ||
{ | ||
return true; | ||
} | ||
|
||
// Start automatic cleanup every two days. | ||
return timestamp.AddDays(2) < DateTime.UtcNow; | ||
} | ||
catch (Exception ex) | ||
{ | ||
Reporter.Verbose.WriteLine($"Cannot access artifacts metadata file '{metadataFile?.FullName}': {ex}"); | ||
|
||
// If the file cannot be accessed, automatic cleanup might already be running. | ||
return false; | ||
} | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Metadata stored at the root level of the file-based app artifacts directory. | ||
/// </summary> | ||
internal sealed class RunFileArtifactsMetadata | ||
{ | ||
public const string FilePath = "dotnet-run-file-artifacts-metadata.json"; | ||
|
||
public DateTime? LastAutomaticCleanupUtc { get; init; } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.CommandLine; | ||
|
||
namespace Microsoft.DotNet.Cli.Commands.Clean.FileBasedAppArtifacts; | ||
|
||
internal sealed class CleanFileBasedAppArtifactsCommandParser | ||
{ | ||
public static readonly Option<bool> DryRunOption = new("--dry-run") | ||
{ | ||
Description = CliCommandStrings.CleanFileBasedAppArtifactsDryRun, | ||
Arity = ArgumentArity.Zero, | ||
}; | ||
|
||
public static readonly Option<int> DaysOption = new("--days") | ||
{ | ||
Description = CliCommandStrings.CleanFileBasedAppArtifactsDays, | ||
DefaultValueFactory = _ => 30, | ||
}; | ||
|
||
/// <summary> | ||
/// Specified internally when the command is started automatically in background by <c>dotnet run</c>. | ||
/// Causes <see cref="RunFileArtifactsMetadata.LastAutomaticCleanupUtc"/> to be updated. | ||
/// </summary> | ||
public static readonly Option<bool> AutomaticOption = new("--automatic") | ||
{ | ||
Hidden = true, | ||
}; | ||
|
||
public static Command Command => field ??= ConstructCommand(); | ||
|
||
private static Command ConstructCommand() | ||
{ | ||
Command command = new("file-based-apps", CliCommandStrings.CleanFileBasedAppArtifactsCommandDescription) | ||
{ | ||
Hidden = true, | ||
Options = | ||
{ | ||
DryRunOption, | ||
DaysOption, | ||
AutomaticOption, | ||
}, | ||
}; | ||
|
||
command.SetAction((parseResult) => new CleanFileBasedAppArtifactsCommand(parseResult).Execute()); | ||
return command; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on making this an option (or sub-command?) to the existing
dotnet clean
command instead? e.g.dotnet clean --file-based-app-artifacts
ordotnet clean caches
or some such @baronfelThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dotnet clean --file-based-apps
orclean-file-based-apps
seems good to me. The word artifacts in the context of clean seems redundant.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So does
dotnet clean --file-based-apps
sound good to everyone before I go and change to that?FWIW, I chose to make this a separate command (currently hidden from
dotnet --help
) because it's fundamentally different fromdotnet clean
which uses MSBuild/t:Clean
whereasdotnet clean-file-based-app-artifacts
implements its own logic which also means no flags of the normaldotnet clean
apply. That being said, I'm okay with having this as a flag fordotnet clean
, but I think we might need to check there are no other flags passed.As for dropping "artifacts" from the command name,
dotnet clean-file-based-apps
sounds a bit like your file-based apps will be deleted (not just their artifacts) but on the other hand I also dislike how unwieldy my current command name is.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to hear from @baronfel. I'm curious whether we could make this a sub-command instead, so something like
dotnet clean file-based-apps
. We could still make it hidden but more importantly we'd need to ensure it's not breaking in any way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd very much love a hidden subcommand. It's kinda a gnarly name to have a dedicated option flag, and it's really an implementation detail that we just don't have a super-great home for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with a subcommand is that it also inherits all arguments and options from the parent command. But perhaps that's acceptable given it's a hidden subcommand.