diff --git a/src/Microsoft.ComponentDetection.Detectors/maven/IMavenCommandService.cs b/src/Microsoft.ComponentDetection.Detectors/maven/IMavenCommandService.cs index c6adc8a2e..309cbf40e 100644 --- a/src/Microsoft.ComponentDetection.Detectors/maven/IMavenCommandService.cs +++ b/src/Microsoft.ComponentDetection.Detectors/maven/IMavenCommandService.cs @@ -27,5 +27,6 @@ public interface IMavenCommandService /// If no other detectors are reading the file, it will be safely deleted. /// /// The path to the dependency file that was being read. - void UnregisterFileReader(string dependencyFilePath); + /// The identifier of the detector unregistering the file reader. + void UnregisterFileReader(string dependencyFilePath, string detectorId = null); } diff --git a/src/Microsoft.ComponentDetection.Detectors/maven/MavenCommandService.cs b/src/Microsoft.ComponentDetection.Detectors/maven/MavenCommandService.cs index 9d21971e2..66dd17d0b 100644 --- a/src/Microsoft.ComponentDetection.Detectors/maven/MavenCommandService.cs +++ b/src/Microsoft.ComponentDetection.Detectors/maven/MavenCommandService.cs @@ -69,6 +69,9 @@ public async Task 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) @@ -154,6 +157,7 @@ private async Task GenerateDependenciesFileCoreAsync(ProcessRequ else { this.logger.LogDebug("{DetectorPrefix}: Execution of \"dependency:tree\" on {PomFileLocation} completed successfully", DetectorLogPrefix, pomFile.Location); + return new MavenCliResult(true, null); } } @@ -174,6 +178,10 @@ 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]); } /// @@ -181,98 +189,48 @@ public void RegisterFileReader(string dependencyFilePath) /// If no other detectors are reading the file, it will be safely deleted. /// /// The path to the dependency file that was being read. - public void UnregisterFileReader(string dependencyFilePath) + /// The identifier of the detector unregistering the file reader. + 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); + } } /// /// Attempts to delete a dependency file if no detectors are currently using it. - /// Uses cross-process coordination to prevent race conditions with other instances. /// /// The path to the dependency file to delete. - private void TryDeleteDependencyFileIfNotInUse(string dependencyFilePath) + /// The identifier of the detector requesting the deletion. + 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); } } } diff --git a/src/Microsoft.ComponentDetection.Detectors/maven/MavenWithFallbackDetector.cs b/src/Microsoft.ComponentDetection.Detectors/maven/MavenWithFallbackDetector.cs index 99126d319..f458727a0 100644 --- a/src/Microsoft.ComponentDetection.Detectors/maven/MavenWithFallbackDetector.cs +++ b/src/Microsoft.ComponentDetection.Detectors/maven/MavenWithFallbackDetector.cs @@ -112,11 +112,11 @@ public class MavenWithFallbackDetector : FileComponentDetector, IExperimentalDet private readonly Dictionary documentsLoaded = []; // Track original pom.xml files for potential fallback - private readonly ConcurrentBag originalPomFiles = []; + private readonly ConcurrentQueue originalPomFiles = []; // Track Maven CLI errors for analysis - private readonly ConcurrentBag mavenCliErrors = []; - private readonly ConcurrentBag failedEndpoints = []; + private readonly ConcurrentQueue mavenCliErrors = []; + private readonly ConcurrentQueue failedEndpoints = []; // Telemetry tracking private MavenDetectionMethod usedDetectionMethod = MavenDetectionMethod.None; @@ -185,8 +185,8 @@ protected override async Task> 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) @@ -194,8 +194,8 @@ protected override async Task> OnPrepareDetectionAsy // 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; } } @@ -281,7 +281,7 @@ private async Task> RunMavenCliDetectionAsync( CancellationToken cancellationToken) { var results = new ConcurrentQueue(); - var failedDirectories = new ConcurrentBag(); + var failedDirectories = new ConcurrentQueue(); var cliSuccessCount = 0; var cliFailureCount = 0; @@ -295,7 +295,7 @@ private async Task> 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); @@ -315,13 +315,14 @@ private async Task> 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}"); } } @@ -329,12 +330,12 @@ private async Task> RunMavenCliDetectionAsync( { // 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); } } }, @@ -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."); @@ -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 @@ -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(); diff --git a/src/Microsoft.ComponentDetection.Detectors/maven/MvnCliComponentDetector.cs b/src/Microsoft.ComponentDetection.Detectors/maven/MvnCliComponentDetector.cs index e69d87475..fefc11f28 100644 --- a/src/Microsoft.ComponentDetection.Detectors/maven/MvnCliComponentDetector.cs +++ b/src/Microsoft.ComponentDetection.Detectors/maven/MvnCliComponentDetector.cs @@ -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 @@ -98,17 +97,16 @@ await this.RemoveNestedPomXmls(processRequests).ForEachAsync(processRequest => protected override async Task OnFileFoundAsync(ProcessRequest processRequest, IDictionary 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; diff --git a/test/Microsoft.ComponentDetection.Detectors.Tests/MavenWithFallbackDetectorTests.cs b/test/Microsoft.ComponentDetection.Detectors.Tests/MavenWithFallbackDetectorTests.cs index 208cf95a1..cf9338e90 100644 --- a/test/Microsoft.ComponentDetection.Detectors.Tests/MavenWithFallbackDetectorTests.cs +++ b/test/Microsoft.ComponentDetection.Detectors.Tests/MavenWithFallbackDetectorTests.cs @@ -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)