Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions packages/amazonq/test/e2e/lsp/lspInstallerUtil.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
ManifestResolver,
request,
TargetContent,
ToolkitError,
} from 'aws-core-vscode/shared'
import * as semver from 'semver'
import { assertTelemetry } from 'aws-core-vscode/test'
Expand Down Expand Up @@ -201,12 +202,6 @@ export function createLspInstallerTests({
id: lspConfig.id,
manifestLocation: 'remote',
languageServerSetupStage: 'getManifest',
result: 'Failed',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was broken in a different commit. Now the manifest download falls back to the local cache if an etag is found but it still reports the telemetry metric as "remote"

Technically this is a minor bug with reporting telemetry, since it should have been reported as a cache

},
{
id: lspConfig.id,
manifestLocation: 'cache',
languageServerSetupStage: 'getManifest',
result: 'Succeeded',
},
{
Expand Down Expand Up @@ -282,6 +277,34 @@ export function createLspInstallerTests({
const download = await createInstaller(lspConfig).resolve()
assert.ok(download.assetDirectory.endsWith('-rc.0'))
})

it('throws on firewall error', async () => {
// Stub the manifest resolver to return a valid manifest
sandbox.stub(ManifestResolver.prototype, 'resolve').resolves({
manifestSchemaVersion: '0.0.0',
artifactId: 'foo',
artifactDescription: 'foo',
isManifestDeprecated: false,
versions: [createVersion('1.0.0', targetContents)],
})

// Fail all HTTP requests for the language server
sandbox.stub(request, 'fetch').returns({
response: Promise.resolve({
ok: false,
}),
} as any)

// This should now throw a NetworkConnectivityError
await assert.rejects(
async () => await installer.resolve(),
(err: ToolkitError) => {
assert.strictEqual(err.code, 'NetworkConnectivityError')
assert.ok(err.message.includes('Unable to download dependencies'))
return true
}
)
})
})
})
}
1 change: 1 addition & 0 deletions packages/core/src/shared/lsp/baseLspInstaller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export abstract class BaseLspInstaller<T extends ResourcePaths = ResourcePaths,
new Range(supportedVersions, {
includePrerelease: true,
}),
manifestUrl,
this.downloadMessageOverride
).resolve()

Expand Down
45 changes: 36 additions & 9 deletions packages/core/src/shared/lsp/lspResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export class LanguageServerResolver {
private readonly manifest: Manifest,
private readonly lsName: string,
private readonly versionRange: semver.Range,
private readonly manifestUrl: string,
/**
* Custom message to show user when downloading, if undefined it will use the default.
*/
Expand Down Expand Up @@ -90,7 +91,22 @@ export class LanguageServerResolver {

/** Finds an older, cached version of the LSP server bundle. */
private async getFallbackServer(latestVersion: LspVersion): Promise<LspResult> {
const fallbackDirectory = await this.getFallbackDir(latestVersion.serverVersion)
const cachedVersions = await this.getCachedVersions()
if (cachedVersions.length === 0) {
/**
* at this point the latest version doesn't exist locally, lsp download (with retries) failed, and there are no cached fallback versions.
* This _probably_ only happens when the user hit a firewall/proxy issue, since otherwise they would probably have at least
* one other language server locally
*/
throw new ToolkitError(
`Unable to download dependencies from ${this.manifestUrl}. Check your network connectivity or firewall configuration and then try again.`,
{
code: 'NetworkConnectivityError',
Copy link
Contributor

Choose a reason for hiding this comment

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

these error codes auto-populate the reason field in telemetry right? If so, this will be important to track going forward to gauge impact. My suspicion is this only affects a very low % of users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah exactly, that's part of why I wanted to add the code here. It should give us a rough idea of how many people are effected

}
)
}

const fallbackDirectory = await this.getFallbackDir(latestVersion.serverVersion, cachedVersions)
if (!fallbackDirectory) {
throw new ToolkitError('Unable to find a compatible version of the Language Server', {
code: 'IncompatibleVersion',
Expand Down Expand Up @@ -142,13 +158,22 @@ export class LanguageServerResolver {
assetDirectory: cacheDirectory,
}
} else {
await this.cleanupVersion(latestVersion.serverVersion)
throw new ToolkitError('Failed to download server from remote', { code: 'RemoteDownloadFailed' })
}
} finally {
timeout.dispose()
}
}

private async cleanupVersion(version: string) {
// clean up the X.X.X download directory since the download failed
const downloadDirectory = this.getDownloadDirectory(version)
if (await fs.existsDir(downloadDirectory)) {
await fs.delete(downloadDirectory)
}
}

/** Gets the current local ("cached") LSP server bundle. */
private async getLocalServer(
cacheDirectory: string,
Expand Down Expand Up @@ -178,16 +203,9 @@ export class LanguageServerResolver {
/**
* Returns the path to the most compatible cached LSP version that can serve as a fallback
**/
private async getFallbackDir(version: string) {
private async getFallbackDir(version: string, cachedVersions: string[]) {
const compatibleLspVersions = this.compatibleManifestLspVersion()

// determine all folders containing lsp versions in the fallback parent folder
const cachedVersions = (await fs.readdir(this.defaultDownloadFolder()))
.filter(([_, filetype]) => filetype === FileType.Directory)
.map(([pathName, _]) => semver.parse(pathName))
.filter((ver): ver is semver.SemVer => ver !== null)
.map((x) => x.version)

const expectedVersion = semver.parse(version)
if (!expectedVersion) {
return undefined
Expand All @@ -203,6 +221,15 @@ export class LanguageServerResolver {
return fallbackDir.length > 0 ? fallbackDir[0] : undefined
}

private async getCachedVersions() {
// determine all folders containing lsp versions in the parent folder
return (await fs.readdir(this.defaultDownloadFolder()))
.filter(([_, filetype]) => filetype === FileType.Directory)
.map(([pathName, _]) => semver.parse(pathName))
.filter((ver): ver is semver.SemVer => ver !== null)
.map((x) => x.version)
}

/**
* Validate the local cache directory of the given lsp version (matches expected hash)
* If valid return cache directory, else return undefined
Expand Down
1 change: 0 additions & 1 deletion packages/core/src/shared/lsp/manifestResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ export class ManifestResolver {

const localManifest = await this.getLocalManifest(true).catch(() => undefined)
if (localManifest) {
localManifest.location = 'remote'
return localManifest
Copy link
Contributor

@justinmk3 justinmk3 May 7, 2025

Choose a reason for hiding this comment

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

The reason I set location=remote, is because this logic is in the fetchRemoteManifest function, and semantically, this is equivalent to when a "fetch" happens. It's just that fetch was skipped in this case, because the server version is the same.

My intuition was that location=local means a local fallback was used instead of the current remote version. I guess it could be interpreted both ways though.

} else {
// Will emit a `languageServer_setup` result=failed metric...
Expand Down
Loading