Fix maven detector race conditions

[zhenghao104]

Fix Maven Detector Race Conditions and Improve Reliability

Overview

This PR addresses critical race conditions and reliability issues in Maven component detection by fixing coordination between MavenWithFallbackDetector and MvnCliComponentDetector, ensuring consistent and accurate component detection results.

Issues Fixed

🐛 Issue 1: Race Condition in File Coordination

Problem: Race condition between MavenWithFallbackDetector and MvnCliComponentDetector where one detector would delete dependency files before the other could process them, causing dramatically inconsistent component counts (e.g., 304 components vs 32 components).

Root Cause: Both detectors competed for the same temporary Maven dependency files without proper coordination, leading to file access conflicts.

Solution:

• Implemented register-at-generation pattern in MavenCommandService.GenerateDependenciesFileAsync()
• Files are registered with RegisterFileReader() immediately upon generation
• Both detectors can safely process the same file before cleanup occurs
• Enhanced logging with detector IDs for better debugging and monitoring
• Proper file lifecycle coordination ensures consistent component detection across both detectors

🔧 Issue 2: Incorrect Static Parser Processing Order

Problem: MavenWithFallbackDetector static parsing fallback wasn't working correctly due to using ConcurrentBag (unordered collection), causing different processing order compared to MavenComponentDetector and inconsistent results.

Solution:

• Changed from ConcurrentBag<ProcessRequest> to ConcurrentQueue<ProcessRequest>
• Ensures deterministic processing order matching MavenComponentDetector
• Provides consistent component detection results during fallback scenarios
• Maintains reliable fallback behavior when Maven CLI fails

📊 Issue 3: Misleading Detection Method Telemetry

Problem: Detection method telemetry was incorrectly labeled as "StaticParserOnly" even when Maven CLI was available and attempted, making monitoring and debugging unclear.

Root Cause: Logic incorrectly classified any CLI failures as static-only mode, regardless of whether CLI was actually available and attempted.

Solution:

• Updated classification logic to properly distinguish detection methods:
  • "MvnCliOnly": When all Maven CLI attempts succeed
  • "Mixed": When Maven CLI is available but some attempts fail (includes authentication failures and partial successes)
  • "StaticParserOnly": Only when Maven CLI is explicitly disabled by user or not available on system
• Updated corresponding unit test expectations to reflect correct behavior

Impact

• ✅ Eliminates race conditions causing inconsistent component detection between detectors
• ✅ Ensures both detectors produce identical results on the same repositories
• ✅ Provides accurate telemetry for monitoring detection method usage and debugging
• ✅ Maintains optimal performance while ensuring correctness and reliability
• ✅ Improves debugging capabilities with enhanced logging and detector identification

Validation

• Unit Tests: All 771 Maven unit tests passing
• Integration Tests: All 6 verification tests passing against real-world repositories
• Manual Testing: Validated on multiple repository types with consistent results between detectors

Files Changed

• src/Microsoft.ComponentDetection.Detectors/maven/MavenCommandService.cs
• src/Microsoft.ComponentDetection.Detectors/maven/IMavenCommandService.cs
• src/Microsoft.ComponentDetection.Detectors/maven/MavenWithFallbackDetector.cs
• src/Microsoft.ComponentDetection.Detectors/maven/MvnCliComponentDetector.cs
• test/Microsoft.ComponentDetection.Detectors.Tests/MavenWithFallbackDetectorTests.cs

Breaking Changes

None. All changes are internal implementation improvements that maintain existing public API contracts.

[Copilot]

Pull request overview

Updates Maven detection to reduce race conditions when generating/consuming bcde.mvndeps across concurrent detectors, and adjusts fallback telemetry to reflect combined CLI + static parsing behavior.

Changes:

• Refactors Maven fallback detector’s concurrency primitives (ConcurrentBag → ConcurrentQueue) and detection-method determination.
• Moves/extends dependency-file “reader coordination” logic via IMavenCommandService (register/unregister now accepts a detector id).
• Updates MavenWithFallback detector test expectations for DetectionMethod telemetry.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
test/Microsoft.ComponentDetection.Detectors.Tests/MavenWithFallbackDetectorTests.cs	Updates expected telemetry (DetectionMethod) in an auth-failure fallback scenario.
src/Microsoft.ComponentDetection.Detectors/maven/MvnCliComponentDetector.cs	Removes explicit reader registration and relies on generation-step coordination; updates unregister call signature.
src/Microsoft.ComponentDetection.Detectors/maven/MavenWithFallbackDetector.cs	Refactors shared state containers, changes detection-method logic, and adds verbose fallback logging.
src/Microsoft.ComponentDetection.Detectors/maven/MavenCommandService.cs	Adds detector-id-aware reader coordination and introduces registration during dependency generation.
src/Microsoft.ComponentDetection.Detectors/maven/IMavenCommandService.cs	Updates interface to include optional detectorId parameters for reader coordination methods.

Comments suppressed due to low confidence (2)

src/Microsoft.ComponentDetection.Detectors/maven/MavenWithFallbackDetector.cs:427

• DetermineDetectionMethod now sets DetectionMethod=Mixed whenever cliFailureCount>0, including the case where cliSuccessCount==0 (i.e., CLI failed for all pom.xml files). That conflicts with the MavenDetectionMethod docs (Mixed == some CLI success + some fallback) and changes telemetry semantics compared to the prior StaticParserOnly behavior. Either restore the StaticParserOnly branch when cliSuccessCount==0, or update the enum/docs/telemetry expectations so the meaning of "Mixed" is consistent.

    private void DetermineDetectionMethod(int cliSuccessCount, int cliFailureCount)
    {
        if (cliFailureCount == 0 && cliSuccessCount > 0)
        {
            this.usedDetectionMethod = MavenDetectionMethod.MvnCliOnly;
            this.LogDebug("All pom.xml files processed successfully with Maven CLI.");
        }
        else if (cliFailureCount > 0)
        {
            this.usedDetectionMethod = MavenDetectionMethod.Mixed;
            this.LogWarning($"Maven CLI failed for {cliFailureCount} pom.xml files. Using mixed detection.");
            this.AnalyzeMvnCliFailure();
        }

src/Microsoft.ComponentDetection.Detectors/maven/MavenCommandService.cs:83

• GenerateDependenciesFileAsync increments the fileReaderCounts via RegisterFileReader(depsFilePath, "GenerationStep") but never decrements it on any return path. This can leave fileReaderCounts entries permanently >0 (especially on CLI failure paths where the deps file is never parsed/unregistered), preventing cleanup and causing unbounded growth across scans (MavenCommandService is a singleton). Consider removing this registration from the generation phase, or ensure it is always paired with an UnregisterFileReader in a finally block (including the early cache-return paths and failure paths).

    public async Task<MavenCliResult> GenerateDependenciesFileAsync(ProcessRequest processRequest, CancellationToken cancellationToken = default)
    {
        var pomFile = processRequest.ComponentStream;
        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, "GenerationStep");

        // 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)
            && cachedResult.Success
            && File.Exists(depsFilePath))
        {
            this.logger.LogDebug("{DetectorPrefix}: Skipping duplicate \"dependency:tree\" for {PomFileLocation}, already generated", DetectorLogPrefix, pomFile.Location);
            return cachedResult;
        }

[github-actions]

👋 Hi! It looks like you modified some files in the Detectors folder.
You may need to bump the detector versions if any of the following scenarios apply:

• The detector detects more or fewer components than before
• The detector generates different parent/child graph relationships than before
• The detector generates different devDependencies values than before

If none of the above scenarios apply, feel free to ignore this comment 🙂

[Copilot]

Pull request overview

This PR aims to eliminate race conditions between Maven detectors (MavenWithFallbackDetector and MvnCliComponentDetector) by coordinating lifecycle/cleanup of generated bcde.mvndeps files, while also making fallback behavior/telemetry more consistent and deterministic.

Changes:

• Moves dependency-file “reader registration” toward generation time and extends unregister logging with detector IDs.
• Switches several unordered collections from ConcurrentBag to ConcurrentQueue to preserve deterministic processing order in fallback paths.
• Updates telemetry classification and adjusts the related unit test expectation.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/Microsoft.ComponentDetection.Detectors/maven/MavenCommandService.cs	Adds file-reader registration/logging and changes cleanup/deletion coordination logic.
src/Microsoft.ComponentDetection.Detectors/maven/IMavenCommandService.cs	Updates UnregisterFileReader signature to optionally include detector ID.
src/Microsoft.ComponentDetection.Detectors/maven/MavenWithFallbackDetector.cs	Uses ordered concurrent collections and updates detection-method classification + cleanup calls.
src/Microsoft.ComponentDetection.Detectors/maven/MvnCliComponentDetector.cs	Removes per-read registration/unregistration and switches to unregister-with-detector-id.
test/Microsoft.ComponentDetection.Detectors.Tests/MavenWithFallbackDetectorTests.cs	Updates expected telemetry DetectionMethod to Mixed for auth-failure fallback scenario.

Comments suppressed due to low confidence (1)

src/Microsoft.ComponentDetection.Detectors/maven/MavenCommandService.cs:83

• GenerateDependenciesFileAsync registers a file reader for the deps file up front, but there are multiple exit paths where that registration is never balanced with an UnregisterFileReader call (e.g., cache hit early-return, CLI failure, cancellation/exception while waiting on the semaphore). This will leak fileReaderCounts entries/counts across scans (detectors are singletons) and can also prevent cleanup or block deletion indefinitely. Consider only registering once you know a deps file exists and will be processed, or ensure registration is always paired with an unregister on non-success/cancellation paths (and avoid registering before the cache fast-path).

    public async Task<MavenCliResult> GenerateDependenciesFileAsync(ProcessRequest processRequest, CancellationToken cancellationToken = default)
    {
        var pomFile = processRequest.ComponentStream;
        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)
            && cachedResult.Success
            && File.Exists(depsFilePath))
        {
            this.logger.LogDebug("{DetectorPrefix}: Skipping duplicate \"dependency:tree\" for {PomFileLocation}, already generated", DetectorLogPrefix, pomFile.Location);
            return cachedResult;
        }

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.0%. Comparing base (db58407) to head (73ee5fe).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@     Coverage Diff      @@
##   main   #1719   +/-   ##
============================
============================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.