SLCORE-2243 Add connected mode and on-demand artifact resolvers#1925
Conversation
SummaryThis PR introduces two new artifact resolver implementations to support different plugin distribution modes. OnDemandArtifactResolver handles standalone plugin downloads with signature verification and file-based caching. ConnectedModeArtifactResolver fetches plugins from a SonarQube server when in connected mode, with version-based override logic (e.g., server Secrets plugin overrides embedded version for SQ 10.4+). Both resolvers support asynchronous downloads, emit status events, and manage concurrent operations via executor services. What reviewers should knowStart here: Review OnDemandArtifactResolver first (simpler, self-contained); then ConnectedModeArtifactResolver (more complex, involves server interaction and override rules). Key decisions: (1) Both return DOWNLOADING state immediately if artifact isn't cached, then publish a status update event when download completes—verify event publishing logic matches the expected flow. (2) ConnectedModeArtifactResolver uses a version map (FORCE_OVERRIDES_SINCE_VERSION) to determine when server plugins override embedded ones; check that these version thresholds are correct for your use case. (3) Signature verification happens for OnDemand artifacts; missing or failed signatures cause re-download and logging. (4) Both use ConcurrentHashMap to avoid duplicate concurrent downloads of the same artifact. Tests: 436 lines for ConnectedMode, 216 for OnDemand—look for edge cases around cache hits, download failures, and version overrides.
|
![sonar-review-alpha[bot]](https://avatars.githubusercontent.com/in/3025063?s=60&v=4){kind=small}
| // Schedule downloads for server companions not yet in storage | ||
| fetchServerPluginsSafely(connectionId).ifPresent(plugins -> | ||
| plugins.stream() | ||
| .filter(p -> isCompanionPlugin(p.getKey()) && !result.containsKey(p.getKey())) |
There was a problem hiding this comment.
The filter !result.containsKey(p.getKey()) prevents scheduling a download when a companion is already in storage, but there is no hash check. A companion whose jar exists on disk but has a different hash than the server version is returned as SYNCED and never re-downloaded.
Contrast with resolveFromStorage (line 188–193), which explicitly rejects stale artifacts with !stored.hasSameHash(serverPlugin). The companion pass should apply the same check — use getStoredPluginsByKey() instead of getStoredPluginPathsByKey() in the first pass so the full StoredPlugin (with hash) is available, and only add to result when the hash matches.
- Mark as noise
|
|
||
| private void scheduleCompanionDownload(String connectionId, ServerPlugin plugin) { | ||
| var progressKey = connectionId + ":" + plugin.getKey(); | ||
| if (inProgressDownloadKeys.add(progressKey)) { |
There was a problem hiding this comment.
The lambda body has no catch (Exception) block. downloadPluginSync itself never throws (it catches internally and returns FAILED), but the lines that follow — storageService.connection(connectionId).plugins().getStoredPluginPathsByKey() and isSonarCloud(connectionId) — can throw. If they do, the finally removes the key from inProgressDownloadKeys but no event is published, leaving the companion permanently stuck in DOWNLOADING state.
asyncDownload (line 218) handles this correctly with an explicit catch (Exception e) that calls fireFailedEvent. Apply the same pattern here.
- Mark as noise
| return new ResolvedArtifact(ArtifactState.DOWNLOADING, null, null, null); | ||
| } | ||
|
|
||
| private Optional<ResolvedArtifact> findCachedArtifact(DownloadableArtifact artifact) { |
There was a problem hiding this comment.
Logic duplication: findCachedArtifact (line 95) and getArtifactPathIfAvailable (line 161) implement the exact same two-level cache check:
- look up
cachedArtifactPaths - fall back to
buildPluginPathon disk - verify signature; on failure, warn and
deleteQuietly - populate
cachedArtifactPathson hit
The only difference is the return type (Optional<ResolvedArtifact> vs Optional<Path>). Extract a shared Optional<Path> resolveArtifactPathFromCache(DownloadableArtifact) helper so a future change to the cache strategy or signature-on-failure handling only needs to be made in one place.
- Mark as noise
| result.put(plugin.getKey(), PluginStatus.forCompanion(plugin.getKey(), ArtifactState.DOWNLOADING, null, null)); | ||
| } | ||
|
|
||
| private void scheduleCompanionDownload(String connectionId, ServerPlugin plugin) { |
There was a problem hiding this comment.
Logic duplication: scheduleCompanionDownload (line 156) and scheduleDownload (line 196) both implement the same dedup-and-submit pattern:
var progressKey = connectionId + ":" + key;
if (inProgressDownloadKeys.add(progressKey)) {
downloadExecutor.submit(() -> { try { … } finally { inProgressDownloadKeys.remove(progressKey); } });
}Extract a scheduleIfNotInProgress(String progressKey, Runnable task) helper that wraps the submitted Runnable in the dedup guard and the finally-remove. Both call sites then pass only their specific download logic, and the dedup/cleanup contract lives in one place.
- Mark as noise
There was a problem hiding this comment.
Pull request overview
This PR introduces new artifact resolver implementations intended to support downloading analyzers in both connected mode (from server) and on-demand mode (from a remote URL/cache), along with corresponding unit tests.
Changes:
- Add
ConnectedModeArtifactResolverto resolve/sync plugins from SonarQube/SonarCloud storage/server plugin lists, including companion plugin handling. - Add
OnDemandArtifactResolverto download/version-cache specific artifacts on demand with signature verification and event publishing. - Add unit tests covering download scheduling, status reporting, and success/error paths for both resolvers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
backend/core/src/main/java/org/sonarsource/sonarlint/core/plugin/resolvers/ConnectedModeArtifactResolver.java |
New connected-mode resolver that reads server plugin lists / storage, schedules downloads, and publishes status update events. |
backend/core/src/main/java/org/sonarsource/sonarlint/core/plugin/ondemand/OnDemandArtifactResolver.java |
New on-demand resolver that downloads artifacts into a versioned cache, verifies signatures, and publishes status update events. |
backend/core/src/test/java/org/sonarsource/sonarlint/core/plugin/ConnectedModeArtifactResolverTest.java |
New unit tests validating connected-mode resolver behavior across storage/server/error scenarios. |
backend/core/src/test/java/org/sonarsource/sonarlint/core/plugin/ondemand/OnDemandArtifactResolverTest.java |
New unit tests validating on-demand resolver behavior for async downloads, caching, and event publishing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| public class OnDemandArtifactResolver implements ArtifactResolver, ExtraArtifactResolver { | ||
|
|
||
| private static final SonarLintLogger LOG = SonarLintLogger.get(); | ||
| private static final String CACHE_SUBDIR = "ondemand-plugins"; | ||
|
|
||
| private final Path cacheBaseDirectory; | ||
| private final HttpClientProvider httpClientProvider; | ||
| private final OnDemandPluginSignatureVerifier signatureVerifier; | ||
| private final OnDemandPluginCacheManager cacheManager; | ||
| private final Map<SonarLanguage, DownloadableArtifact> artifactsByLanguage; |
| private ResolvedArtifact scheduleDownload(DownloadableArtifact artifact) { | ||
| if (inProgressArtifactKeys.add(artifact.artifactKey())) { | ||
| downloadExecutor.submit(() -> downloadAndFireEvent(artifact)); | ||
| } | ||
| return new ResolvedArtifact(ArtifactState.DOWNLOADING, null, null, null); | ||
| } |
| import org.sonarsource.sonarlint.core.UserPaths; | ||
| import org.sonarsource.sonarlint.core.commons.Version; | ||
| import org.sonarsource.sonarlint.core.commons.api.SonarLanguage; | ||
| import org.sonarsource.sonarlint.core.commons.log.SonarLintLogTester; | ||
| import org.sonarsource.sonarlint.core.event.PluginStatusUpdateEvent; | ||
| import org.sonarsource.sonarlint.core.http.HttpClient; | ||
| import org.sonarsource.sonarlint.core.http.HttpClientProvider; | ||
| import org.sonarsource.sonarlint.core.plugin.ArtifactSource; | ||
| import org.sonarsource.sonarlint.core.plugin.ArtifactState; | ||
| import org.sonarsource.sonarlint.core.plugin.PluginStatus; | ||
| import org.sonarsource.sonarlint.core.plugin.ResolvedArtifact; | ||
| import org.springframework.context.ApplicationEventPublisher; | ||
|
|
| public class ConnectedModeArtifactResolver implements ArtifactResolver, CompanionPluginResolver { | ||
|
|
||
| private static final SonarLintLogger LOG = SonarLintLogger.get(); | ||
| private static final String LEGACY_TYPESCRIPT_PLUGIN_KEY = "typescript"; | ||
| private static final String PLUGIN_FETCH_ERROR = "Could not fetch server plugin list for connection '{}'"; |
| private ResolvedArtifact scheduleDownload(String connectionId, ServerPlugin serverPlugin, SonarLanguage language) { | ||
| var progressKey = connectionId + ":" + serverPlugin.getKey(); | ||
| if (inProgressDownloadKeys.add(progressKey)) { | ||
| downloadExecutor.submit(() -> asyncDownload(connectionId, serverPlugin, language, progressKey)); | ||
| } | ||
| return new ResolvedArtifact(ArtifactState.DOWNLOADING, null, null, null); | ||
| } |
| private void downloadAndVerify(DownloadableArtifact artifact, Path targetPath) throws IOException { | ||
| Files.createDirectories(targetPath.getParent()); | ||
| var tempFile = targetPath.getParent().resolve(targetPath.getFileName() + ".tmp"); | ||
| try { | ||
| downloadPlugin(artifact, tempFile); | ||
| if (!signatureVerifier.verify(tempFile, artifact.artifactKey())) { | ||
| throw new IOException("Signature verification failed for " + artifact.artifactKey()); | ||
| } | ||
| Files.move(tempFile, targetPath, StandardCopyOption.ATOMIC_MOVE); | ||
| LOG.info("Successfully downloaded {} plugin version {}", artifact.artifactKey(), artifact.version()); | ||
| } finally { | ||
| Files.deleteIfExists(tempFile); | ||
| } |
| private OnDemandArtifactResolver buildResolver(Map<SonarLanguage, DownloadableArtifact> artifactsByLanguage) { | ||
| var userPaths = mock(UserPaths.class); | ||
| when(userPaths.getStorageRoot()).thenReturn(tempDir); | ||
| return new OnDemandArtifactResolver(userPaths, httpClientProvider, artifactsByLanguage, eventPublisher, Executors.newCachedThreadPool()); | ||
| } |
| import org.sonarsource.sonarlint.core.plugin.resolvers.ConnectedModeArtifactResolver; | ||
| import org.sonarsource.sonarlint.core.repository.connection.AbstractConnectionConfiguration; | ||
| import org.sonarsource.sonarlint.core.repository.connection.ConnectionConfigurationRepository; | ||
| import org.sonarsource.sonarlint.core.serverapi.exception.ServerRequestException; | ||
| import org.sonarsource.sonarlint.core.serverapi.plugins.ServerPlugin; | ||
| import org.sonarsource.sonarlint.core.serverconnection.ConnectionStorage; | ||
| import org.sonarsource.sonarlint.core.serverconnection.StoredPlugin; | ||
| import org.sonarsource.sonarlint.core.serverconnection.StoredServerInfo; | ||
| import org.sonarsource.sonarlint.core.serverconnection.storage.PluginsStorage; | ||
| import org.sonarsource.sonarlint.core.serverconnection.storage.ServerInfoStorage; | ||
| import org.sonarsource.sonarlint.core.storage.StorageService; | ||
| import org.springframework.context.ApplicationEventPublisher; | ||
|
|
| private void scheduleCompanionDownload(String connectionId, ServerPlugin plugin) { | ||
| var progressKey = connectionId + ":" + plugin.getKey(); | ||
| if (inProgressDownloadKeys.add(progressKey)) { | ||
| downloadExecutor.submit(() -> { | ||
| try { | ||
| var state = downloadPluginSync(connectionId, plugin); | ||
| var storedPath = state == ArtifactState.SYNCED | ||
| ? storageService.connection(connectionId).plugins().getStoredPluginPathsByKey().get(plugin.getKey()) | ||
| : null; | ||
| var source = isSonarCloud(connectionId) ? ArtifactSource.SONARQUBE_CLOUD : ArtifactSource.SONARQUBE_SERVER; | ||
| eventPublisher.publishEvent(new PluginStatusUpdateEvent(connectionId, | ||
| List.of(PluginStatus.forCompanion(plugin.getKey(), state, source, storedPath)))); | ||
| } finally { | ||
| inProgressDownloadKeys.remove(progressKey); | ||
| } | ||
| }); | ||
| } |
Krosovok
force-pushed
the
feature/vt/SLCORE-2243-resolve-connected-ondemand-artifacts
branch
from
c81873d to
cf5fc28
Compare
There was a problem hiding this comment.
Conclusion: Solid work overall — the async download pattern, cache management, and version-gate logic are well structured. Two bugs need fixing before merge: a race condition in the synchronous extra-artifact path and a missing error handler in companion-plugin downloads.
| .flatMap(this::getArtifactPathIfAvailable); | ||
| } | ||
|
|
||
| private Optional<Path> getArtifactPathIfAvailable(DownloadableArtifact artifact) { |
There was a problem hiding this comment.
Bug — race condition on concurrent extra-artifact downloads: getArtifactPathIfAvailable has no concurrency guard (unlike the async path which uses inProgressArtifactKeys). If two threads call resolve(String artifactKey) for the same key simultaneously, both will call downloadAndCache, which calls downloadAndVerify, which both resolve to the same temp-file path (targetPath.getFileName() + ".tmp"). The two downloads interleave writes to that single temp file via FileUtils.copyInputStreamToFile, producing a corrupted byte stream; signature verification then fails and an IOException is thrown.
Fix: guard with inProgressArtifactKeys the same way scheduleDownload does, or use a per-key lock/computeIfAbsent idiom to serialise concurrent callers.
- Mark as noise
| .flatMap(this::getArtifactPathIfAvailable); | ||
| } | ||
|
|
||
| private Optional<Path> getArtifactPathIfAvailable(DownloadableArtifact artifact) { |
There was a problem hiding this comment.
Logic duplication: The three-step sequence — (1) check cachedArtifactPaths in-memory, (2) probe disk via buildPluginPath, (3) verify signature, update cache, or evict — is duplicated verbatim between findCachedArtifact (lines 95–111) and getArtifactPathIfAvailable (lines 161–175). Any future change to cache-eviction policy or signature-check behaviour must be made in both places.
Extract a shared helper, e.g. Optional<Path> resolveFromCache(DownloadableArtifact artifact), that encapsulates the probe-and-verify logic and returns an Optional<Path>. Both callers can then decide what to do on a miss (schedule async vs. download synchronously).
- Mark as noise
| result.put(plugin.getKey(), PluginStatus.forCompanion(plugin.getKey(), ArtifactState.DOWNLOADING, null, null)); | ||
| } | ||
|
|
||
| private void scheduleCompanionDownload(String connectionId, ServerPlugin plugin) { |
There was a problem hiding this comment.
Bug — uncaught exception leaves companion plugin stuck in DOWNLOADING state: scheduleCompanionDownload submits a task that has a finally block but no catch. If anything after downloadPluginSync throws (e.g. storageService.connection(...).plugins().getStoredPluginPathsByKey() or eventPublisher.publishEvent(...) raise an exception), the task exits silently: the inProgressDownloadKeys entry is cleaned up correctly, but no FAILED status event is fired. The IDE-side listener will never receive an update for this companion plugin and will leave it stuck in DOWNLOADING.
Contrast with asyncDownload (lines 229–237) which has an explicit catch (Exception e) that logs and fires a failed event. Apply the same pattern here:
try {
// existing body
} catch (Exception e) {
LOG.error("Failed to process companion plugin '{}' for connection '{}'", plugin.getKey(), connectionId, e);
eventPublisher.publishEvent(new PluginStatusUpdateEvent(connectionId,
List.of(PluginStatus.forCompanion(plugin.getKey(), ArtifactState.FAILED, null, null))));
} finally {
inProgressDownloadKeys.remove(progressKey);
}- Mark as noise
No description provided.