Addressing code review comments

Author: LokeshDogga13

plugins/amazonq/shared/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/lsp/artifacts/ArtifactHelper.kt

@@ -3,7 +3,10 @@
 
 package software.aws.toolkits.jetbrains.services.amazonq.lsp.artifacts
 
+import com.intellij.openapi.actionSystem.AnAction
+import com.intellij.openapi.actionSystem.AnActionEvent
 import com.intellij.util.io.createDirectories
+import com.intellij.util.text.SemVer
 import software.aws.toolkits.core.utils.deleteIfExists
 import software.aws.toolkits.core.utils.error
 import software.aws.toolkits.core.utils.exists
@@ -21,14 +24,14 @@ class ArtifactHelper(private val lspArtifactsPath: Path = DEFAULT_ARTIFACT_PATH)
         private val DEFAULT_ARTIFACT_PATH = getToolkitsCommonCacheRoot().resolve("aws").resolve("toolkits").resolve("language-servers")
         private val logger = getLogger<ArtifactHelper>()
         private const val MAX_DOWNLOAD_ATTEMPTS = 3
-        private val currentAttempt = AtomicInteger(0)
     }
+    private val currentAttempt = AtomicInteger(0)
 
-    fun removeDeListedVersions(deListedVersions: List<ManifestManager.Version>) {
-        val localFolders: List<Path> = getSubFolders(lspArtifactsPath)
+    fun removeDelistedVersions(delistedVersions: List<ManifestManager.Version>) {
+        val localFolders = getSubFolders(lspArtifactsPath)
 
-        deListedVersions.forEach { deListedVersion ->
-            val versionToDelete = deListedVersion.serverVersion ?: return@forEach
+        delistedVersions.forEach { delistedVersion ->
+            val versionToDelete = delistedVersion.serverVersion ?: return@forEach
 
             localFolders
                 .filter { folder -> folder.fileName.toString() == versionToDelete }
@@ -43,7 +46,31 @@ class ArtifactHelper(private val lspArtifactsPath: Path = DEFAULT_ARTIFACT_PATH)
         }
     }
 
-    fun getExistingLSPArtifacts(versions: List<ManifestManager.Version>, target: ManifestManager.VersionTarget?): Boolean {
+    fun deleteOlderLspArtifacts(manifestVersionRanges: ArtifactManager.SupportedManifestVersionRange) {
+        val localFolders = getSubFolders(lspArtifactsPath)
+
+        val validVersions = localFolders
+            .mapNotNull { localFolder ->
+                SemVer.parseFromText(localFolder.fileName.toString())?.let { semVer ->
+                    if (semVer in manifestVersionRanges.startVersion..manifestVersionRanges.endVersion) {
+                        localFolder to semVer
+                    } else null
+                }
+            }
+            .sortedByDescending { (_, semVer) -> semVer }
+
+        // Keep the latest 2 versions, delete others
+        validVersions.drop(2).forEach { (folder, _) ->
+            try {
+                folder.toFile().deleteRecursively()
+                logger.info { "Deleted older LSP artifact: ${folder.fileName}" }
+            } catch (e: Exception) {
+                logger.error(e) { "Failed to delete older LSP artifact: ${folder.fileName}" }
+            }
+        }
+    }
+
+    fun getExistingLspArtifacts(versions: List<ManifestManager.Version>, target: ManifestManager.VersionTarget?): Boolean {
         if (versions.isEmpty() || target?.contents == null) return false
 
         val localLSPPath = lspArtifactsPath.resolve(versions.first().serverVersion.toString())
@@ -52,7 +79,7 @@ class ArtifactHelper(private val lspArtifactsPath: Path = DEFAULT_ARTIFACT_PATH)
         val hasInvalidFiles = target.contents.any { content ->
             content.filename?.let { filename ->
                 val filePath = localLSPPath.resolve(filename)
-                !filePath.exists() || generateMD5Hash(filePath) != content.hashes?.firstOrNull()
+                !filePath.exists() || !validateFileHash(filePath, content.hashes?.firstOrNull())
             } ?: false
         }
 
@@ -64,7 +91,7 @@ class ArtifactHelper(private val lspArtifactsPath: Path = DEFAULT_ARTIFACT_PATH)
                 logger.error(e) { "Failed to delete mismatched LSP artifacts at: $localLSPPath" }
             }
         }
-        return hasInvalidFiles
+        return !hasInvalidFiles
     }
 
     fun tryDownloadLspArtifacts(versions: List<ManifestManager.Version>, target: ManifestManager.VersionTarget?) {
@@ -139,7 +166,8 @@ class ArtifactHelper(private val lspArtifactsPath: Path = DEFAULT_ARTIFACT_PATH)
         }
     }
 
-    private fun validateFileHash(filePath: Path, expectedHash: String): Boolean {
+    private fun validateFileHash(filePath: Path, expectedHash: String?): Boolean {
+        if(expectedHash == null) return false
         val contentHash = generateSHA384Hash(filePath)
         return "sha384:$contentHash" == expectedHash
     }

plugins/amazonq/shared/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/lsp/artifacts/ArtifactManager.kt

@@ -5,8 +5,8 @@ package software.aws.toolkits.jetbrains.services.amazonq.lsp.artifacts
 
 import com.intellij.util.text.SemVer
 import org.assertj.core.util.VisibleForTesting
+import software.aws.toolkits.core.utils.error
 import software.aws.toolkits.core.utils.getLogger
-import software.aws.toolkits.core.utils.info
 import software.aws.toolkits.jetbrains.services.amazonq.project.manifest.ManifestManager
 
 class ArtifactManager {
@@ -53,7 +53,7 @@ class ArtifactManager {
         )
         val lspVersions = getLSPVersionsFromManifestWithSpecifiedRange(manifest)
 
-        this.artifactHelper.removeDeListedVersions(lspVersions.deListedVersions)
+        this.artifactHelper.removeDelistedVersions(lspVersions.deListedVersions)
 
         if (lspVersions.inRangeVersions.isEmpty()) {
             // No versions are found which are in the given range.
@@ -64,12 +64,11 @@ class ArtifactManager {
         val target = getTargetFromLspManifest(lspVersions.inRangeVersions)
 
         // Get Local LSP files and check if we can re-use existing LSP Artifacts
-        if (this.artifactHelper.getExistingLSPArtifacts(lspVersions.inRangeVersions, target)) {
-            return
+        if (!this.artifactHelper.getExistingLspArtifacts(lspVersions.inRangeVersions, target)) {
+            this.artifactHelper.tryDownloadLspArtifacts(lspVersions.inRangeVersions, target)
         }
 
-        this.artifactHelper.tryDownloadLspArtifacts(lspVersions.inRangeVersions, target)
-        logger.info { "Success" }
+        this.artifactHelper.deleteOlderLspArtifacts(manifestVersionRanges)
     }
 
     @VisibleForTesting
@@ -102,8 +101,11 @@ class ArtifactManager {
             target.platform == currentOS && target.arch == currentArchitecture
         }
         if (currentTarget == null) {
+            logger.error {"Failed to obtain target for $currentOS and $currentArchitecture" }
             throw LspException("Target not found in the current Version: ${versions.first().serverVersion}", LspException.ErrorCode.TARGET_NOT_FOUND)
         }
+        logger.info("Target found in the current Version: ${versions.first().serverVersion}")
         return currentTarget
     }
+
 }

plugins/amazonq/shared/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/lsp/artifacts/LspUtils.kt

@@ -33,9 +33,10 @@ fun getCurrentOS(): String = when {
     else -> "linux"
 }
 
-fun getCurrentArchitecture() = when {
-    CpuArch.CURRENT == CpuArch.X86_64 -> "x64"
-    else -> "arm64"
+fun getCurrentArchitecture() = when(CpuArch.CURRENT) {
+    CpuArch.X86_64 -> "x64"
+    CpuArch.ARM64 -> "arm64"
+    else -> "unknown"
 }
 
 fun generateMD5Hash(filePath: Path): String {