Skip to content

Fix maven detector race conditions #1719

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.

Sign up for GitHub

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

Merged
zhenghao104 merged 4 commits into from
Mar 13, 2026
+54 −102
Merged

Fix maven detector race conditions #1719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4a3d989
Fix maven detector race conditions
zhenghao104 Mar 13, 2026
9fbde10
PR comments
zhenghao104 Mar 13, 2026
ed20692
Another PR comment
zhenghao104 Mar 13, 2026
73ee5fe
Cleanup code
zhenghao104 Mar 13, 2026
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
3 changes: 2 additions & 1 deletion src/Microsoft.ComponentDetection.Detectors/maven/IMavenCommandService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,6 @@ public interface IMavenCommandService
/// If no other detectors are reading the file, it will be safely deleted.
/// </summary>
/// <param name="dependencyFilePath">The path to the dependency file that was being read.</param>
void UnregisterFileReader(string dependencyFilePath);
/// <param name="detectorId">The identifier of the detector unregistering the file reader.</param>
void UnregisterFileReader(string dependencyFilePath, string detectorId = null);
}
100 changes: 29 additions & 71 deletions src/Microsoft.ComponentDetection.Detectors/maven/MavenCommandService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ public async Task<MavenCliResult> GenerateDependenciesFileAsync(ProcessRequest p
var pomDir = Path.GetDirectoryName(pomFile.Location);
var depsFilePath = Path.Combine(pomDir, this.BcdeMvnDependencyFileName);

// Register as file reader immediately to prevent premature cleanup
this.RegisterFileReader(depsFilePath);

// Check the cache before acquiring the semaphore to allow fast-path returns
// even when cancellation has been requested.
if (this.completedLocations.TryGetValue(pomFile.Location, out var cachedResult)
Expand Down Expand Up @@ -154,6 +157,7 @@ private async Task<MavenCliResult> GenerateDependenciesFileCoreAsync(ProcessRequ
else
{
this.logger.LogDebug("{DetectorPrefix}: Execution of \"dependency:tree\" on {PomFileLocation} completed successfully", DetectorLogPrefix, pomFile.Location);

return new MavenCliResult(true, null);
}
}
Expand All @@ -174,105 +178,59 @@ public void ParseDependenciesFile(ProcessRequest processRequest)
public void RegisterFileReader(string dependencyFilePath)
{
this.fileReaderCounts.AddOrUpdate(dependencyFilePath, 1, (key, count) => count + 1);
this.logger.LogDebug(
"Registered file reader for {DependencyFilePath}, count: {Count}",
dependencyFilePath,
this.fileReaderCounts[dependencyFilePath]);
}

/// <summary>
/// Unregisters a detector's active reading of a dependency file and attempts cleanup.
/// If no other detectors are reading the file, it will be safely deleted.
/// </summary>
/// <param name="dependencyFilePath">The path to the dependency file that was being read.</param>
public void UnregisterFileReader(string dependencyFilePath)
/// <param name="detectorId">The identifier of the detector unregistering the file reader.</param>
public void UnregisterFileReader(string dependencyFilePath, string detectorId = null)
{
this.fileReaderCounts.AddOrUpdate(dependencyFilePath, 0, (key, count) => Math.Max(0, count - 1));
this.TryDeleteDependencyFileIfNotInUse(dependencyFilePath);
var newCount = this.fileReaderCounts.AddOrUpdate(dependencyFilePath, 0, (key, count) => Math.Max(0, count - 1));
this.logger.LogDebug(
"{DetectorId}: Unregistered file reader for {DependencyFilePath}, count: {Count}",
detectorId ?? "Unknown",
dependencyFilePath,
newCount);

// If no readers remain, attempt cleanup
if (newCount == 0)
{
this.TryDeleteDependencyFileIfNotInUse(dependencyFilePath, detectorId);
}
}

/// <summary>
/// Attempts to delete a dependency file if no detectors are currently using it.
/// Uses cross-process coordination to prevent race conditions with other instances.
/// </summary>
/// <param name="dependencyFilePath">The path to the dependency file to delete.</param>
private void TryDeleteDependencyFileIfNotInUse(string dependencyFilePath)
/// <param name="detectorId">The identifier of the detector requesting the deletion.</param>
private void TryDeleteDependencyFileIfNotInUse(string dependencyFilePath, string detectorId = null)
{
// Check if any local readers are using the file
if (this.fileReaderCounts.TryGetValue(dependencyFilePath, out var count) && count > 0)
{
this.logger.LogDebug("Skipping deletion of {DependencyFilePath} - {Count} local readers still active", dependencyFilePath, count);
return;
}

var coordinationFile = dependencyFilePath + ".deleting";
var detector = detectorId ?? "Unknown";

// Safe to delete - no readers are using the file (count was already verified to be 0)
try
{
// Try to create coordination file atomically with our process ID
var processId = Environment.ProcessId.ToString();

// Use FileMode.CreateNew to ensure atomic creation (fails if file exists)
using (var fs = new FileStream(coordinationFile, FileMode.CreateNew, FileAccess.Write, FileShare.None))
using (var writer = new StreamWriter(fs))
{
writer.Write(processId);
}

this.logger.LogDebug("Created coordination file {CoordinationFile} for process {ProcessId}", coordinationFile, processId);

// Give other processes a chance to create their coordination files if needed
Thread.Sleep(50);

// Verify we still own the coordination (another process might have deleted and recreated it)
if (!File.Exists(coordinationFile))
{
this.logger.LogDebug("Coordination file {CoordinationFile} was deleted by another process", coordinationFile);
return;
}

var coordinationContent = File.ReadAllText(coordinationFile).Trim();
if (coordinationContent != processId)
{
this.logger.LogDebug("Coordination file {CoordinationFile} was taken over by process {OtherProcessId}, aborting deletion", coordinationFile, coordinationContent);
return;
}

// We own the coordination - proceed with deletion
if (File.Exists(dependencyFilePath))
{
File.Delete(dependencyFilePath);
this.logger.LogDebug("Successfully deleted dependency file {DependencyFilePath}", dependencyFilePath);
this.logger.LogDebug("{DetectorId}: Successfully deleted dependency file {DependencyFilePath}", detector, dependencyFilePath);
}
else
{
this.logger.LogDebug("Dependency file {DependencyFilePath} was already deleted", dependencyFilePath);
this.logger.LogDebug("{DetectorId}: Dependency file {DependencyFilePath} was already deleted", detector, dependencyFilePath);
}
}
catch (IOException ex) when (ex.Message.Contains("already exists") || ex.HResult == -2147024816)
{
// Another process is handling deletion (File already exists)
this.logger.LogDebug("Another process is already coordinating deletion of {DependencyFilePath}", dependencyFilePath);
}
catch (Exception ex)
{
this.logger.LogWarning(ex, "Failed to coordinate deletion of dependency file {DependencyFilePath}", dependencyFilePath);
}
finally
{
// Clean up our coordination file
try
{
if (File.Exists(coordinationFile))
{
var coordinationContent = File.ReadAllText(coordinationFile).Trim();
if (coordinationContent == Environment.ProcessId.ToString())
{
File.Delete(coordinationFile);
this.logger.LogDebug("Cleaned up coordination file {CoordinationFile}", coordinationFile);
}
}
}
catch (Exception ex)
{
this.logger.LogDebug(ex, "Failed to clean up coordination file {CoordinationFile}, will be cleaned up later", coordinationFile);
}
this.logger.LogWarning(ex, "{DetectorId}: Failed to delete dependency file {DependencyFilePath}", detector, dependencyFilePath);
}
}
}
37 changes: 16 additions & 21 deletions src/Microsoft.ComponentDetection.Detectors/maven/MavenWithFallbackDetector.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,11 @@ public class MavenWithFallbackDetector : FileComponentDetector, IExperimentalDet
private readonly Dictionary<string, XmlDocument> documentsLoaded = [];

// Track original pom.xml files for potential fallback
private readonly ConcurrentBag<ProcessRequest> originalPomFiles = [];
private readonly ConcurrentQueue<ProcessRequest> originalPomFiles = [];

// Track Maven CLI errors for analysis
private readonly ConcurrentBag<string> mavenCliErrors = [];
private readonly ConcurrentBag<string> failedEndpoints = [];
private readonly ConcurrentQueue<string> mavenCliErrors = [];
private readonly ConcurrentQueue<string> failedEndpoints = [];

// Telemetry tracking
private MavenDetectionMethod usedDetectionMethod = MavenDetectionMethod.None;
Expand Down Expand Up @@ -185,17 +185,17 @@ protected override async Task<IObservable<ProcessRequest>> OnPrepareDetectionAsy
// Timeout occurred (not user cancellation)
this.LogWarning($"OnPrepareDetectionAsync timed out after {PrepareDetectionTimeout.TotalMinutes} minutes. Falling back to static pom.xml parsing.");
this.Telemetry["TimedOut"] = "true";
this.usedDetectionMethod = MavenDetectionMethod.StaticParserOnly;
this.fallbackReason = MavenFallbackReason.OtherMvnCliFailure;
this.usedDetectionMethod = MavenDetectionMethod.Mixed;
return processRequests;
}
catch (Exception ex)
{
// Unexpected error - log and fall back to static parsing
this.LogWarning($"OnPrepareDetectionAsync failed with unexpected error: {ex.Message}. Falling back to static pom.xml parsing.");
this.Telemetry["PrepareDetectionError"] = ex.GetType().Name;
this.usedDetectionMethod = MavenDetectionMethod.StaticParserOnly;
this.fallbackReason = MavenFallbackReason.OtherMvnCliFailure;
this.usedDetectionMethod = MavenDetectionMethod.Mixed;
return processRequests;
}
}
Expand Down Expand Up @@ -281,7 +281,7 @@ private async Task<IObservable<ProcessRequest>> RunMavenCliDetectionAsync(
CancellationToken cancellationToken)
{
var results = new ConcurrentQueue<ProcessRequest>();
var failedDirectories = new ConcurrentBag<string>();
var failedDirectories = new ConcurrentQueue<string>();
var cliSuccessCount = 0;
var cliFailureCount = 0;

Expand All @@ -295,7 +295,7 @@ private async Task<IObservable<ProcessRequest>> RunMavenCliDetectionAsync(
cancellationToken.ThrowIfCancellationRequested();

// Store original pom.xml for telemetry
this.originalPomFiles.Add(processRequest);
this.originalPomFiles.Enqueue(processRequest);

var pomFile = processRequest.ComponentStream;
var pomDir = Path.GetDirectoryName(pomFile.Location);
Expand All @@ -315,26 +315,27 @@ private async Task<IObservable<ProcessRequest>> RunMavenCliDetectionAsync(
// Use existence check to avoid redundant I/O (file will be read during directory scan)
if (this.fileUtilityService.Exists(depsFilePath))
{
// File reader registration is now handled in GenerateDependenciesFileAsync
Interlocked.Increment(ref cliSuccessCount);
}
else
{
// CLI reported success but deps file is missing - treat as failure
Interlocked.Increment(ref cliFailureCount);
failedDirectories.Add(pomDir);
failedDirectories.Enqueue(pomDir);
this.LogWarning($"Maven CLI succeeded but deps file not found: {depsFilePath}");
}
}
else
{
// CLI failed - track directory for nested pom.xml scanning
Interlocked.Increment(ref cliFailureCount);
failedDirectories.Add(pomDir);
failedDirectories.Enqueue(pomDir);

// Capture error output for later analysis
if (!string.IsNullOrWhiteSpace(result.ErrorOutput))
{
this.mavenCliErrors.Add(result.ErrorOutput);
this.mavenCliErrors.Enqueue(result.ErrorOutput);
}
}
},
Expand Down Expand Up @@ -420,13 +421,7 @@ private void DetermineDetectionMethod(int cliSuccessCount, int cliFailureCount)
this.usedDetectionMethod = MavenDetectionMethod.MvnCliOnly;
this.LogDebug("All pom.xml files processed successfully with Maven CLI.");
}
else if (cliSuccessCount == 0 && cliFailureCount > 0)
{
this.usedDetectionMethod = MavenDetectionMethod.StaticParserOnly;
this.LogWarning("Maven CLI failed for all pom.xml files. Using static parsing fallback.");
this.AnalyzeMvnCliFailure();
}
else if (cliSuccessCount > 0 && cliFailureCount > 0)
else if (cliFailureCount > 0)
{
this.usedDetectionMethod = MavenDetectionMethod.Mixed;
this.LogWarning($"Maven CLI failed for {cliFailureCount} pom.xml files. Using mixed detection.");
Expand All @@ -443,16 +438,16 @@ protected override Task OnFileFoundAsync(

if (pattern == this.mavenCommandService.BcdeMvnDependencyFileName)
{
// Process MvnCli result - register as reader and cleanup when done
// Process MvnCli result - file reader already registered at generation step
var depsFilePath = processRequest.ComponentStream.Location;
this.mavenCommandService.RegisterFileReader(depsFilePath);
try
{
this.ProcessMvnCliResult(processRequest);
}
finally
{
this.mavenCommandService.UnregisterFileReader(depsFilePath);
// Unregister file reader after processing to allow cleanup
this.mavenCommandService.UnregisterFileReader(depsFilePath, this.Id);
}
}
else
Expand Down Expand Up @@ -503,7 +498,7 @@ private void AnalyzeMvnCliFailure()
// Extract failed endpoints from error messages
foreach (var endpoint in this.mavenCliErrors.SelectMany(this.ExtractFailedEndpoints))
{
this.failedEndpoints.Add(endpoint);
this.failedEndpoints.Enqueue(endpoint);
}

this.LogAuthErrorGuidance();
Expand Down
12 changes: 5 additions & 7 deletions src/Microsoft.ComponentDetection.Detectors/maven/MvnCliComponentDetector.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,10 @@ await this.RemoveNestedPomXmls(processRequests).ForEachAsync(processRequest =>
{
// The file stream is going to be disposed after the iteration is finished
// so is necessary to read the content and keep it in memory, for further processing.
// Register as reader for coordination with file deletion
this.mavenCommandService.RegisterFileReader(componentStream.Location);
// File reader registration is now handled in GenerateDependenciesFileAsync
using var reader = new StreamReader(componentStream.Stream);
var content = reader.ReadToEnd();
this.mavenCommandService.UnregisterFileReader(componentStream.Location);

return new ProcessRequest
{
ComponentStream = new ComponentStream
Expand All @@ -98,17 +97,16 @@ await this.RemoveNestedPomXmls(processRequests).ForEachAsync(processRequest =>

protected override async Task OnFileFoundAsync(ProcessRequest processRequest, IDictionary<string, string> detectorArgs, CancellationToken cancellationToken = default)
{
// Register as reader to coordinate with other detectors
// File reader already registered at generation step, just process and unregister
var depsFilePath = processRequest.ComponentStream.Location;
this.mavenCommandService.RegisterFileReader(depsFilePath);
try
{
this.mavenCommandService.ParseDependenciesFile(processRequest);
}
finally
{
// Unregister and attempt coordinated cleanup
this.mavenCommandService.UnregisterFileReader(depsFilePath);
// Cleanup coordination with other detectors
this.mavenCommandService.UnregisterFileReader(depsFilePath, this.Id);
}

await Task.CompletedTask;
Expand Down
4 changes: 2 additions & 2 deletions test/Microsoft.ComponentDetection.Detectors.Tests/MavenWithFallbackDetectorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1178,9 +1178,9 @@ public async Task WhenAuthenticationFailsAndParentChildPropertiesUsed_MaintainsC
detectorResult.AdditionalTelemetryDetails.Should().ContainKey("FallbackReason");
detectorResult.AdditionalTelemetryDetails["FallbackReason"].Should().Be("AuthenticationFailure");

// Should have method showing static parser was used
// Should have method showing mixed detection was used (CLI failed but fallback succeeded)
detectorResult.AdditionalTelemetryDetails.Should().ContainKey("DetectionMethod");
detectorResult.AdditionalTelemetryDetails["DetectionMethod"].Should().Be("StaticParserOnly");
detectorResult.AdditionalTelemetryDetails["DetectionMethod"].Should().Be("Mixed");
}

private void SetupMvnCliSuccess(string depsFileContent)
Expand Down
Loading