Skip to content

SLCORE-2387 SLCORE-2394 SLCORE-2395 Trigger local SCA analysis#2088

Open
eray-felek-sonarsource wants to merge 4 commits into
feature/ef/slcore-2289-sca-in-idefrom
feature/ef/2387-poc
Open

SLCORE-2387 SLCORE-2394 SLCORE-2395 Trigger local SCA analysis#2088
eray-felek-sonarsource wants to merge 4 commits into
feature/ef/slcore-2289-sca-in-idefrom
feature/ef/2387-poc

Conversation

@eray-felek-sonarsource
Copy link
Copy Markdown
Contributor

@eray-felek-sonarsource eray-felek-sonarsource commented May 22, 2026


Summary by Gitar

  • SCA Analysis Infrastructure:
    • Added ScaProjectAnalysisService for orchestrating on-demand SCA analysis.
    • Implemented ScaAnalysisContextResolver to consolidate analysis configuration (binding, credentials, endpoint, properties).
    • Added LocalDependencyRiskAnalysisCache to store and merge local scanner findings.
    • Added DependencyRiskMerger and DependencyRiskDtoMapper to reconcile and enrich server-tracked risks with local analysis results.
  • RPC Protocol Updates:
    • Added analyzeProject method to DependencyRiskRpcService for manual trigger of local analysis.
  • Testing Infrastructure:
    • Added unit tests in ScaProjectAnalysisServiceTests and DependencyRiskServiceTests.
    • Added medium integration tests in AnalyzeDependencyRiskProjectMediumTests.

This will update automatically on new commits.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 22, 2026

Agentic Analysis: Early Results

Agentic Analysis and Context Augmentation are available on your project. Here are some issues that could have been prevented. Follow the links to learn how to put them into action.

3 issue(s) found across 2 file(s):

Rule File Line Message
java:S3553 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/DependencyRiskMerger.java 41 Specify a "AnalyzeProjectResponse" parameter instead.
java:S3553 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/DependencyRiskMerger.java 47 Specify a "AnalyzeProjectResponse" parameter instead.
java:S1117 backend/core/src/test/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisServiceTests.java 437 Rename "analyzeProjectOptions" which hides the field declared at line 432.

Analyzed by SonarQube Agentic Analysis in 9.1 s

@hashicorp-vault-sonar-prod
Copy link
Copy Markdown

hashicorp-vault-sonar-prod Bot commented May 22, 2026

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented May 26, 2026

Analyzing CI failures

Code Review ⚠️ Changes requested 1 resolved / 2 findings

Implements local SCA analysis via the new ScaProjectAnalysisService and addresses temporary directory cleanup, but fails to handle null parameters during JSON-RPC deserialization, creating a potential NPE risk.

⚠️ Edge Case: NPE risk when params fields are null from JSON deserialization

📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:125 📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:127 📄 rpc-protocol/src/main/java/org/sonarsource/sonarlint/core/rpc/protocol/backend/sca/AnalyzeDependencyRiskProjectParams.java:28-29

When AnalyzeDependencyRiskProjectParams is deserialized via JSON-RPC (e.g., Gson reflective deserialization), fields excludedPaths and scannerProperties may be null if omitted from the request. At lines 125 and 127, List.copyOf(params.getExcludedPaths()) and Map.copyOf(params.getScannerProperties()) will throw NullPointerException on null input.

Since this is a public RPC API, clients may not always provide these optional-looking fields.

Add null guards before calling List.copyOf / Map.copyOf
.setExcludedPaths(params.getExcludedPaths() == null ? List.of() : List.copyOf(params.getExcludedPaths()))
.setScmExclusionEnabled(params.getScmExclusionEnabled() == null || params.getScmExclusionEnabled())
.setScannerProperties(params.getScannerProperties() == null ? Map.of() : Map.copyOf(params.getScannerProperties()))
✅ 1 resolved
Bug: Temporary work directory is never cleaned up after analysis

📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:108-122
The createWorkDir() method creates a new temporary directory under sca-scanner/work/analyze-project-* on every invocation of analyzeProject, but no cleanup is performed after the analysis completes (neither in a finally block nor via try-with-resources). Over time, repeated analyses will accumulate stale directories consuming disk space.

Consider wrapping the analysis in a try-finally block that deletes the work directory after use (or on failure).

🤖 Prompt for agents
Code Review: Implements local SCA analysis via the new `ScaProjectAnalysisService` and addresses temporary directory cleanup, but fails to handle null parameters during JSON-RPC deserialization, creating a potential NPE risk.

1. ⚠️ Edge Case: NPE risk when params fields are null from JSON deserialization
   Files: backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:125, backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:127, rpc-protocol/src/main/java/org/sonarsource/sonarlint/core/rpc/protocol/backend/sca/AnalyzeDependencyRiskProjectParams.java:28-29

   When `AnalyzeDependencyRiskProjectParams` is deserialized via JSON-RPC (e.g., Gson reflective deserialization), fields `excludedPaths` and `scannerProperties` may be `null` if omitted from the request. At lines 125 and 127, `List.copyOf(params.getExcludedPaths())` and `Map.copyOf(params.getScannerProperties())` will throw `NullPointerException` on null input.
   
   Since this is a public RPC API, clients may not always provide these optional-looking fields.

   Fix (Add null guards before calling List.copyOf / Map.copyOf):
   .setExcludedPaths(params.getExcludedPaths() == null ? List.of() : List.copyOf(params.getExcludedPaths()))
   .setScmExclusionEnabled(params.getScmExclusionEnabled() == null || params.getScmExclusionEnabled())
   .setScannerProperties(params.getScannerProperties() == null ? Map.of() : Map.copyOf(params.getScannerProperties()))

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.
Unblock → Override a blocking verdict and allow merging.

Comment with these commands to change:

Auto-apply Compact Unblock
gitar auto-apply:on         
gitar display:verbose         
gitar unblock         

Was this helpful? React with 👍 / 👎 | Gitar

@eray-felek-sonarsource
Copy link
Copy Markdown
Contributor Author

Code Review ⚠️ Changes requested 3 resolved / 5 findings
Implements local SCA analysis trigger for project dependency risks, but requires handling potential NPEs from null JSON deserialization fields and collection copies.

⚠️ Edge Case: NPE risk when params fields are null from JSON deserialization
📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:125 📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:127 📄 rpc-protocol/src/main/java/org/sonarsource/sonarlint/core/rpc/protocol/backend/sca/AnalyzeDependencyRiskProjectParams.java:28-29

When AnalyzeDependencyRiskProjectParams is deserialized via JSON-RPC (e.g., Gson reflective deserialization), fields excludedPaths and scannerProperties may be null if omitted from the request. At lines 125 and 127, List.copyOf(params.getExcludedPaths()) and Map.copyOf(params.getScannerProperties()) will throw NullPointerException on null input.

Since this is a public RPC API, clients may not always provide these optional-looking fields.

Add null guards before calling List.copyOf / Map.copyOf

.setExcludedPaths(params.getExcludedPaths() == null ? List.of() : List.copyOf(params.getExcludedPaths()))
.setScmExclusionEnabled(params.getScmExclusionEnabled() == null || params.getScmExclusionEnabled())
.setScannerProperties(params.getScannerProperties() == null ? Map.of() : Map.copyOf(params.getScannerProperties()))

⚠️ Edge Case: NPE risk: List.copyOf/Map.copyOf on potentially null params fields
📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:125 📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:127

At lines 125 and 127, List.copyOf(params.getExcludedPaths()) and Map.copyOf(params.getScannerProperties()) will throw NullPointerException if the deserialized params object has null values for these fields. The LSP4J JSON-RPC layer uses Gson which can bypass constructors during deserialization, meaning excludedPaths and scannerProperties may be null even though the full constructor initializes them to List.of()/Map.of(). This is consistent with the previous finding on this file that remains active.

Add null guards before calling List.copyOf/Map.copyOf to handle cases where Gson deserialization leaves fields null.

.setExcludedPaths(params.getExcludedPaths() == null ? List.of() : List.copyOf(params.getExcludedPaths()))
.setScmExclusionEnabled(params.getScmExclusionEnabled() == null || params.getScmExclusionEnabled())
.setScannerProperties(params.getScannerProperties() == null ? Map.of() : Map.copyOf(params.getScannerProperties()))

✅ 3 resolved
✅ Bug: Temporary work directory is never cleaned up after analysis

📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:108-122
The createWorkDir() method creates a new temporary directory under sca-scanner/work/analyze-project-* on every invocation of analyzeProject, but no cleanup is performed after the analysis completes (neither in a finally block nor via try-with-resources). Over time, repeated analyses will accumulate stale directories consuming disk space.
Consider wrapping the analysis in a try-finally block that deletes the work directory after use (or on failure).

✅ Bug: Race condition: rerun request can be lost between threads

📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaAnalysisManager.java:96-101 📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaAnalysisManager.java:109-118 📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaAnalysisManager.java:258-265
In ScaAnalysisManager, there is a race window between onAnalysisCompleted and handleAlreadyRunningAnalysis that can cause an automatic rerun request to be silently lost.
Scenario:

  1. Thread A calls analyzeProject with AUTOMATIC trigger, gets currentAnalysis from runningAnalysis.get()
  2. Thread B (executor) enters onAnalysisCompleted, successfully does compareAndSet(analysis, null), then calls consumeRerunParams() → returns null
  3. Thread A proceeds into handleAlreadyRunningAnalysis, calls currentAnalysis.requestRerun(params) — sets rerunParams on the now-stale analysis
  4. Thread B evaluates rerunParams != null → false (already consumed), skips the rerun
  5. The automatic rerun request is permanently lost

The root cause is that runningAnalysis is cleared (CAS to null) before consumeRerunParams, creating a window where new callers can still call requestRerun on the old analysis object via their stale reference.

✅ Bug: cancelMonitor from RPC framework is unused in analyzeProject

📄 backend/rpc-impl/src/main/java/org/sonarsource/sonarlint/core/rpc/impl/DependencyRiskRpcServiceDelegate.java:55-56 📄 backend/rpc-impl/src/main/java/org/sonarsource/sonarlint/core/rpc/impl/DependencyRiskRpcServiceDelegate.java:51-53
In DependencyRiskRpcServiceDelegate.analyzeProject, the cancelMonitor provided by the RPC framework is ignored — it's not passed to ScaAnalysisManager.analyzeProject(params). This means if the RPC client cancels the request (e.g., IDE closes the request), the ScaAnalysisManager won't know about it. The only way to cancel is via the explicit cancelAnalysis endpoint. While the analysis itself can be cancelled via cancelAnalysis, the blocking result.join() in awaitResult() will keep the RPC thread occupied until the analysis finishes or is explicitly cancelled through the separate endpoint.

🤖 Prompt for agents

Code Review: Implements local SCA analysis trigger for project dependency risks, but requires handling potential NPEs from null JSON deserialization fields and collection copies.

1. ⚠️ Edge Case: NPE risk when params fields are null from JSON deserialization
   Files: backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:125, backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:127, rpc-protocol/src/main/java/org/sonarsource/sonarlint/core/rpc/protocol/backend/sca/AnalyzeDependencyRiskProjectParams.java:28-29

   When `AnalyzeDependencyRiskProjectParams` is deserialized via JSON-RPC (e.g., Gson reflective deserialization), fields `excludedPaths` and `scannerProperties` may be `null` if omitted from the request. At lines 125 and 127, `List.copyOf(params.getExcludedPaths())` and `Map.copyOf(params.getScannerProperties())` will throw `NullPointerException` on null input.
   
   Since this is a public RPC API, clients may not always provide these optional-looking fields.

   Fix (Add null guards before calling List.copyOf / Map.copyOf):
   .setExcludedPaths(params.getExcludedPaths() == null ? List.of() : List.copyOf(params.getExcludedPaths()))
   .setScmExclusionEnabled(params.getScmExclusionEnabled() == null || params.getScmExclusionEnabled())
   .setScannerProperties(params.getScannerProperties() == null ? Map.of() : Map.copyOf(params.getScannerProperties()))

2. ⚠️ Edge Case: NPE risk: List.copyOf/Map.copyOf on potentially null params fields
   Files: backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:125, backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:127

   At lines 125 and 127, `List.copyOf(params.getExcludedPaths())` and `Map.copyOf(params.getScannerProperties())` will throw `NullPointerException` if the deserialized params object has null values for these fields. The LSP4J JSON-RPC layer uses Gson which can bypass constructors during deserialization, meaning `excludedPaths` and `scannerProperties` may be `null` even though the full constructor initializes them to `List.of()`/`Map.of()`. This is consistent with the previous finding on this file that remains active.

   Fix (Add null guards before calling List.copyOf/Map.copyOf to handle cases where Gson deserialization leaves fields null.):
   .setExcludedPaths(params.getExcludedPaths() == null ? List.of() : List.copyOf(params.getExcludedPaths()))
   .setScmExclusionEnabled(params.getScmExclusionEnabled() == null || params.getScmExclusionEnabled())
   .setScannerProperties(params.getScannerProperties() == null ? Map.of() : Map.copyOf(params.getScannerProperties()))

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options
Auto-apply is off → Gitar will not commit updates to this branch.Display: compact → Showing less information.Unblock → Override a blocking verdict and allow merging.

Comment with these commands to change:

Auto-apply Compact Unblock

gitar auto-apply:on         
gitar display:verbose         
gitar unblock         

Was this helpful? React with 👍 / 👎 | Gitar

The 2 remaining issues are addressed, I think this is a false positive

@eray-felek-sonarsource eray-felek-sonarsource marked this pull request as ready for review May 27, 2026 14:51
import static org.assertj.core.api.Assertions.catchThrowable;

@ExtendWith(SystemStubsExtension.class)
class AnalyzeDependencyRiskProjectMediumTests {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the medium tests are very thin, they do not test much

Copy link
Copy Markdown
Contributor Author

@eray-felek-sonarsource eray-felek-sonarsource Jun 1, 2026

Choose a reason for hiding this comment

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

This was also an issue on CLI implementation I think.

We would need real downloading of their CLI to add more meaningful tests which should be covered by this ticket.

WDYT? Should we still add more tests in the scope of this PR? If so please specify which paths you'd like to see covered

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Real downloading would likely be an integration test, here it's a medium test. We could mock interactions with CLI via the harness, similarly to what we do with withSonarQubeConnection

Copy link
Copy Markdown
Contributor Author

@eray-felek-sonarsource eray-felek-sonarsource Jun 1, 2026

Choose a reason for hiding this comment

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

Just to clarify the technical constraint here: the CLI interaction doesn't actually happen in our code. It is entirely encapsulated within the sca-scanner library. The library handles downloading the CLI, spawning it as a separate process, and reading its output. Because only the initial download is an HTTP call, the test harness doesn't have an interception point for the actual execution.

This is why we can't really compare this to withSonarQubeConnection, which works by returning canned responses to HTTP calls made directly by our backend. Stubbing the download step wouldn't solve this either—whatever we serve still gets executed as a real program. Serving anything other than a working executable just reproduces the failure case we already have covered.

To actually test the happy path in a medium test, we would have to choose between two approaches:

Option 1: Provide a fake CLI executable
We could provide a small fake CLI program that prints canned output and let it run normally (via the existing cliOverrideLocation option).

Trade-off: Because it’s a real executable, it is OS-specific (we'd need to handle Windows vs. Linux). This makes it feel much more like an integration test.

Option 2: Inject a fake scanner
We could change our backend dependency injection so tests can replace the real scanner with a fake one that simply returns canned results.

Trade-off: While this avoids the OS-specific execution problem, it requires adding test-only code to the production backend and entirely skips the download, run, and parse steps.

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.

Do you think we should go with one of these options or should we continue with my initial suggestion?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we could have a trade off solution with a very lightweight executable that fakes SCA CLI and creates a fake .tar.gz. It could be used with a system property to override the location of the CLI. This way, we can stub SCA analysis APIs and have a medium tests that can simulate the whole end to end chain.

@eray-felek-sonarsource eray-felek-sonarsource changed the base branch from master to feature/ef/slcore-2289-sca-in-ide June 2, 2026 07:41
* </p>
*/
public class AnalyzeDependencyRiskProjectResponse {
private final List<DependencyRiskDto> dependencyRisks;
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.

The current pattern for SLCore based analysis triggering is that explicit analyze methods DON'T return issues. If in the future we implement automatic analysis, then we'll have to change this response to fit that pattern. Should we already do that?

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.

Same as discussed below

@@ -33,6 +33,13 @@ public interface DependencyRiskRpcService {
@JsonRequest
CompletableFuture<ListAllDependencyRisksResponse> listAll(ListAllParams params);
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.

local scan results are not integrated into this method. What is the end vision for how the frontends would use/combine these methods?

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.

Same goes for the client notification didChangeDependencyRisks

Copy link
Copy Markdown
Contributor Author

@eray-felek-sonarsource eray-felek-sonarsource Jun 2, 2026

Choose a reason for hiding this comment

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

As discussed

We first call analyze project locally
Keep things in cache
After analysis is finished we stream with didChangeDependencyRisks following the general patterns

Copy link
Copy Markdown
Contributor Author

@eray-felek-sonarsource eray-felek-sonarsource Jun 2, 2026

Choose a reason for hiding this comment

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

@georgii-borovinskikh-sonarsource following up on our discussion here, since the local analysis can yield different results depending on the branch we are on, I planning to store the analysis results in cache based on configurationScopeId + branchId, WDYT?

this.dependencyDetails = dependencyDetails;
}

public ReleaseDetailsDto getReleaseDetails() {
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.

What will this be used for on the frontend?

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.

Currently we are not cutting any information that we return to frontend. Do you think this information would never be used? We can let the frontend decide if it's useful or not.

Copy link
Copy Markdown
Contributor Author

@eray-felek-sonarsource eray-felek-sonarsource Jun 2, 2026

Choose a reason for hiding this comment

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

Not sure if we should investigate all the information we return to frontends are useful or not at this stage

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.

We decided we can do this if we have extra time but not essential

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does that mean we don't know what will be implemented on client side?

Comment on lines +26 to +36
public class LocalDependencyRiskAnalysisCache {
private final ConcurrentHashMap<String, AnalyzeProjectResponse> analysesByConfigurationScopeId = new ConcurrentHashMap<>();

public Optional<AnalyzeProjectResponse> get(String configurationScopeId) {
return Optional.ofNullable(analysesByConfigurationScopeId.get(configurationScopeId));
}

public void put(String configurationScopeId, AnalyzeProjectResponse analysis) {
analysesByConfigurationScopeId.put(configurationScopeId, analysis);
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Performance: LocalDependencyRiskAnalysisCache never evicts entries on scope removal

The LocalDependencyRiskAnalysisCache stores AnalyzeProjectResponse objects keyed by configuration scope ID in a ConcurrentHashMap that only grows. When a configuration scope is removed (unbound, closed), the entry is never evicted. Other services in the codebase (e.g., ClientFileSystemService) listen for ConfigurationScopeRemovedEvent to clean up their per-scope state. Without similar cleanup, this cache will leak memory proportional to the number of scopes analyzed over the IDE session lifetime.

Add a remove method, then add a @EventListener for ConfigurationScopeRemovedEvent in DependencyRiskService (or create a dedicated listener) that calls localDependencyRiskAnalysisCache.remove(event.getRemovedConfigurationScopeId()).:

public class LocalDependencyRiskAnalysisCache {
  private final ConcurrentHashMap<String, AnalyzeProjectResponse> analysesByConfigurationScopeId = new ConcurrentHashMap<>();

  public Optional<AnalyzeProjectResponse> get(String configurationScopeId) {
    return Optional.ofNullable(analysesByConfigurationScopeId.get(configurationScopeId));
  }

  public void put(String configurationScopeId, AnalyzeProjectResponse analysis) {
    analysesByConfigurationScopeId.put(configurationScopeId, analysis);
  }

  public void remove(String configurationScopeId) {
    analysesByConfigurationScopeId.remove(configurationScopeId);
  }
}
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Jun 2, 2026

Code Review ⚠️ Changes requested 10 resolved / 11 findings

Implements local SCA analysis orchestration and RPC infrastructure, addressing resource cleanup and race condition issues. The LocalDependencyRiskAnalysisCache requires a mechanism for evicting entries upon scope removal to prevent memory leaks.

⚠️ Performance: LocalDependencyRiskAnalysisCache never evicts entries on scope removal

📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/LocalDependencyRiskAnalysisCache.java:26-36

The LocalDependencyRiskAnalysisCache stores AnalyzeProjectResponse objects keyed by configuration scope ID in a ConcurrentHashMap that only grows. When a configuration scope is removed (unbound, closed), the entry is never evicted. Other services in the codebase (e.g., ClientFileSystemService) listen for ConfigurationScopeRemovedEvent to clean up their per-scope state. Without similar cleanup, this cache will leak memory proportional to the number of scopes analyzed over the IDE session lifetime.

Add a `remove` method, then add a `@EventListener` for `ConfigurationScopeRemovedEvent` in `DependencyRiskService` (or create a dedicated listener) that calls `localDependencyRiskAnalysisCache.remove(event.getRemovedConfigurationScopeId())`.
public class LocalDependencyRiskAnalysisCache {
  private final ConcurrentHashMap<String, AnalyzeProjectResponse> analysesByConfigurationScopeId = new ConcurrentHashMap<>();

  public Optional<AnalyzeProjectResponse> get(String configurationScopeId) {
    return Optional.ofNullable(analysesByConfigurationScopeId.get(configurationScopeId));
  }

  public void put(String configurationScopeId, AnalyzeProjectResponse analysis) {
    analysesByConfigurationScopeId.put(configurationScopeId, analysis);
  }

  public void remove(String configurationScopeId) {
    analysesByConfigurationScopeId.remove(configurationScopeId);
  }
}
✅ 10 resolved
Bug: Temporary work directory is never cleaned up after analysis

📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:108-122
The createWorkDir() method creates a new temporary directory under sca-scanner/work/analyze-project-* on every invocation of analyzeProject, but no cleanup is performed after the analysis completes (neither in a finally block nor via try-with-resources). Over time, repeated analyses will accumulate stale directories consuming disk space.

Consider wrapping the analysis in a try-finally block that deletes the work directory after use (or on failure).

Bug: Race condition: rerun request can be lost between threads

📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaAnalysisManager.java:96-101 📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaAnalysisManager.java:109-118 📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaAnalysisManager.java:258-265
In ScaAnalysisManager, there is a race window between onAnalysisCompleted and handleAlreadyRunningAnalysis that can cause an automatic rerun request to be silently lost.

Scenario:

  1. Thread A calls analyzeProject with AUTOMATIC trigger, gets currentAnalysis from runningAnalysis.get()
  2. Thread B (executor) enters onAnalysisCompleted, successfully does compareAndSet(analysis, null), then calls consumeRerunParams() → returns null
  3. Thread A proceeds into handleAlreadyRunningAnalysis, calls currentAnalysis.requestRerun(params) — sets rerunParams on the now-stale analysis
  4. Thread B evaluates rerunParams != null → false (already consumed), skips the rerun
  5. The automatic rerun request is permanently lost

The root cause is that runningAnalysis is cleared (CAS to null) before consumeRerunParams, creating a window where new callers can still call requestRerun on the old analysis object via their stale reference.

Bug: cancelMonitor from RPC framework is unused in analyzeProject

📄 backend/rpc-impl/src/main/java/org/sonarsource/sonarlint/core/rpc/impl/DependencyRiskRpcServiceDelegate.java:55-56 📄 backend/rpc-impl/src/main/java/org/sonarsource/sonarlint/core/rpc/impl/DependencyRiskRpcServiceDelegate.java:51-53
In DependencyRiskRpcServiceDelegate.analyzeProject, the cancelMonitor provided by the RPC framework is ignored — it's not passed to ScaAnalysisManager.analyzeProject(params). This means if the RPC client cancels the request (e.g., IDE closes the request), the ScaAnalysisManager won't know about it. The only way to cancel is via the explicit cancelAnalysis endpoint. While the analysis itself can be cancelled via cancelAnalysis, the blocking result.join() in awaitResult() will keep the RPC thread occupied until the analysis finishes or is explicitly cancelled through the separate endpoint.

Bug: onCancel interrupt callback is never unregistered

📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:120-134
At line 121, cancelMonitor.onCancel(analysisThread::interrupt) registers a callback that will interrupt the current thread if cancellation fires. However, SonarLintCancelMonitor has no mechanism to unregister callbacks. If the cancel monitor outlives this method call (it's passed in from the RPC layer), a late cancellation could interrupt the thread while it's executing unrelated work. The Thread.interrupted() calls at lines 140/146 only clear the flag if cancellation already occurred — they don't prevent future interrupts from the still-registered callback.

Consider wrapping the callback in a guard that becomes a no-op after the method completes, e.g., using an AtomicBoolean flag checked inside the callback.

Edge Case: Null apiBaseUrl passed to scanner without validation

📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:101 📄 backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/ScaProjectAnalysisService.java:212-221
The method toScaApiBaseUrl (line 212-221) can return null when the SonarCloud endpoint has both apiBaseUrl and baseUrl as null. This null value flows directly into ScaScannerOptionsBuilder.setApiBaseUrl() at line 123. While in practice SonarCloud endpoints likely always have a baseUrl, there's no defensive check like the one for downloadBaseUrl (lines 103-105). If the scanner library doesn't tolerate a null apiBaseUrl, this will result in a confusing error or NPE instead of a clear validation message.

...and 5 more resolved from earlier reviews

🤖 Prompt for agents
Code Review: Implements local SCA analysis orchestration and RPC infrastructure, addressing resource cleanup and race condition issues. The `LocalDependencyRiskAnalysisCache` requires a mechanism for evicting entries upon scope removal to prevent memory leaks.

1. ⚠️ Performance: LocalDependencyRiskAnalysisCache never evicts entries on scope removal
   Files: backend/core/src/main/java/org/sonarsource/sonarlint/core/sca/LocalDependencyRiskAnalysisCache.java:26-36

   The `LocalDependencyRiskAnalysisCache` stores `AnalyzeProjectResponse` objects keyed by configuration scope ID in a `ConcurrentHashMap` that only grows. When a configuration scope is removed (unbound, closed), the entry is never evicted. Other services in the codebase (e.g., `ClientFileSystemService`) listen for `ConfigurationScopeRemovedEvent` to clean up their per-scope state. Without similar cleanup, this cache will leak memory proportional to the number of scopes analyzed over the IDE session lifetime.

   Fix (Add a `remove` method, then add a `@EventListener` for `ConfigurationScopeRemovedEvent` in `DependencyRiskService` (or create a dedicated listener) that calls `localDependencyRiskAnalysisCache.remove(event.getRemovedConfigurationScopeId())`.):
   public class LocalDependencyRiskAnalysisCache {
     private final ConcurrentHashMap<String, AnalyzeProjectResponse> analysesByConfigurationScopeId = new ConcurrentHashMap<>();
   
     public Optional<AnalyzeProjectResponse> get(String configurationScopeId) {
       return Optional.ofNullable(analysesByConfigurationScopeId.get(configurationScopeId));
     }
   
     public void put(String configurationScopeId, AnalyzeProjectResponse analysis) {
       analysesByConfigurationScopeId.put(configurationScopeId, analysis);
     }
   
     public void remove(String configurationScopeId) {
       analysesByConfigurationScopeId.remove(configurationScopeId);
     }
   }

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.
Unblock → Override a blocking verdict and allow merging.

Comment with these commands to change:

Auto-apply Compact Unblock
gitar auto-apply:on         
gitar display:verbose         
gitar unblock         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqube-next
Copy link
Copy Markdown

sonarqube-next Bot commented Jun 2, 2026

Quality Gate failed Quality Gate failed

Failed conditions
2 New issues

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE SonarQube for IDE

Copy link
Copy Markdown
Member

@nquinquenel nquinquenel left a comment

Choose a reason for hiding this comment

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

You'll need to update API_CHANGES as well with all the new APIs

Comment on lines +96 to +99
var delta = dependencyRiskMerger.computeDelta(newRisks);
if (!delta.addedRisks().isEmpty() || !delta.updatedRisks().isEmpty()) {
client.didChangeDependencyRisks(new DidChangeDependencyRisksParams(configurationScopeId, Set.of(), delta.addedRisks(), delta.updatedRisks()));
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure this work as expected, local analysis should compare previous merged snapshot vs new merged snapshot (by id where present, and a stable local key otherwise), not filter by Presence on the new list only

Comment on lines +145 to +149





Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Empty lines to remove

Comment on lines +73 to +80
public AnalyzeDependencyRiskProjectResponse analyzeProject(AnalyzeDependencyRiskProjectParams params, SonarLintCancelMonitor cancelMonitor) {
var configurationScopeId = params.getConfigurationScopeId();
var context = contextResolver.resolve(configurationScopeId);
var localResponse = runLocalAnalysis(configurationScopeId, context);
var mergedRisks = dependencyRiskService.updateLocalAnalysisAndNotify(configurationScopeId, localResponse, cancelMonitor);
var errors = localResponse.errors().stream().map(dependencyRiskDtoMapper::toErrorDto).toList();
return new AnalyzeDependencyRiskProjectResponse(mergedRisks, localResponse.parsedFiles(), errors);
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we may want to check if SCA is supported there

Comment on lines +96 to +99
var downloadBaseUrl = StringUtils.firstNonBlank(System.getProperty(SCA_DOWNLOAD_BASE_URL_PROPERTY), toScaDownloadBaseUrl(endpointParams));
if (downloadBaseUrl == null) {
throw invalidArgument("Missing SCA scanner download base URL. Provide it in the request or with system property '" + SCA_DOWNLOAD_BASE_URL_PROPERTY + "'");
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the message is misleading, you can't provide the download URL via the request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants