feat(lsp): older and delisted versions of lsp are automatically removed #6409

Hweinstock · 2025-01-21T19:36:03Z

Problem

Installed LSP artifacts remain forever. There is no cleanup effort made.

Solution

Implement the following heuristic:

Delete all currently installed delisted versions.
Delete all versions that remain, except the most recent 2.

Included in this change is a new utility function partition. partition is like filter, but it produces both the positive and negative result in two separate sublists. See the tests for a simple example.

Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
Your code changes must meet the guidelines in CONTRIBUTING.md.
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

github-actions · 2025-01-21T19:36:18Z

This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
- Note: beta or "experiment" features that have active users should announce fixes in the changelog.
- If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

Hweinstock · 2025-01-21T19:54:16Z

/runIntegrationTests

jpinkney-aws · 2025-01-22T13:11:57Z

packages/core/src/amazonq/lsp/workspaceInstaller.ts

        }
    }
+
+    private async getDownloadedVersions(downloadDirectory: string): Promise<string[]> {


Can you pull this into a common location so the other language server can use it: https://github.com/aws/aws-toolkit-vscode/blob/feature/amazonqLSP/packages/amazonq/src/lsp/lspInstaller.ts

I moved it to its own util.ts file to avoid circular dependency. Lmk if there is a better place.

packages/core/src/amazonq/lsp/workspaceInstaller.ts

packages/core/src/shared/utilities/tsUtils.ts

jpinkney-aws · 2025-01-22T13:16:24Z

packages/core/src/amazonq/lsp/workspaceInstaller.ts

+        }>
+    ) {
+        this.versionRange = options?.versionRange ?? new Range(supportedLspServerVersions)
+        this.shouldCleanUp = options?.cleanUp ?? true


is there any case where we would make this false?

I was using this to get greater control in the integ tests, but considering those tests aren't even running, it likely makes sense to remove this bloat for now.

packages/core/src/test/amazonq/lsp/workspaceinstaller.test.ts

jpinkney-aws · 2025-01-22T13:19:21Z

packages/core/src/test/amazonq/lsp/workspaceinstaller.test.ts

+            assert.ok(result.includes('1.1.1'))
+        })
+
+        it('deletes delisted versions', async function () {


Maybe a couple more tests to make sure if we have < 2 versions that everything behaves correctly? I think in that case we would want to keep the latest 2 just in case one gets delisted and we can fall back to the other

I added 3 new cases.

if there are less than 2 remaining versions (after filtering for delisted), we should keep all of them.

if there are less than 2 versions overall, we should keep all of the non-delisted ones.

if all versions present are delisted, we still shouldn't download it.

Do you think that covers the edge cases with < 2 versions?

packages/core/src/test/amazonq/lsp/workspaceinstaller.test.ts

jpinkney-aws · 2025-01-22T13:21:51Z

packages/core/src/testInteg/amazonq/lsp/workspaceInstaller.test.ts

+        if (!onMac) {
+            this.skip()
+        }
+        await fs.delete(LanguageServerResolver.defaultDir, { force: true, recursive: true })


is there a way we can mock this path somehow we aren't nuking our language servers when we run this test

I was trying to run them as e2e as possible, but I think for these integ tests the setup will take more work to get right. Right now, I deleted them, and we can re-add in a followup if it seems important.

justinmk3 · 2025-01-23T20:02:08Z

packages/core/src/amazonq/lsp/util.ts

@@ -0,0 +1,10 @@
+/*!


These various fetch/download/lsp utils seem potentially applicable to both Toolkit and Q in the future. How much of these util modules are Q-specific? Can most of them live in /core/ ?

All the logic for downloading/fetching etc is in core. The only thing that should be in the q lsp folder is the codewhisperer language server resolver and the codewhisperer language server activation

oh right this is in core but the amazonq part of core... if it's not q-specific then it could live in shared.

jpinkney-aws · 2025-01-24T13:30:03Z

packages/core/src/amazonq/lsp/workspaceInstaller.ts

        }
    }
+
+    private isDelisted(manifestVersions: LspVersion[], targetVersion: string): boolean {


I think these are still missing from this file as well: https://github.com/aws/aws-toolkit-vscode/blob/feature/amazonqLSP/packages/amazonq/src/lsp/lspInstaller.ts#L21. Since this only does the cleanup for the workspace context language server and not the codewhisperer one

Moved it to a general location in core to avoid duplicating.

Hweinstock · 2025-01-24T19:30:39Z

non-web failing tests addressed in #6429

packages/core/src/shared/lsp/utils/cleanup.ts

justinmk3 · 2025-01-24T20:36:04Z

packages/core/src/shared/lsp/utils/cleanup.ts

+        isDelisted(manifestVersions, v)
+    )
+    for (const v of delistedVersions) {
+        await fs.delete(path.join(downloadDirectory, v), { force: true, recursive: true })


I guess force=true avoids any potential failures. So we don't need to use .catch() to continue the for-loop on failure.

justinmk3

Not a blocker for this PR, but it does look lilke the cleanup module + tests should live in shared/languageServer/ (which should be renamed to shared/lsp/)

Hweinstock · 2025-01-24T21:32:06Z

Updated it so that cleanup code lives in packages/core/src/shared/lsp/utils/cleanup.ts. and tests in packages/core/src/test/shared/lsp/utils/cleanup.test.ts.

Hweinstock added 3 commits January 21, 2025 14:21

implement heuristic

9cd4544

clean up handling on optional args

e7bf0af

add tests for utility method

90d23b1

fetch versions from manifest file

2c19cf4

Hweinstock added 2 commits January 21, 2025 15:07

skip integ tests on non-mac

1c366ef

skip beforeAll hook on non-mac

b00ec0d

Hweinstock marked this pull request as ready for review January 21, 2025 21:51

Hweinstock requested review from a team as code owners January 21, 2025 21:51

jpinkney-aws reviewed Jan 22, 2025

View reviewed changes

Hweinstock added 6 commits January 22, 2025 10:00

pull out helper function so that it can be used by other modules

4198611

handle case where remaining versions is 2 or less

32d2ba3

add comment to partition helper function

a7a89c1

remove special logic for integ tests

ee7343b

increase test coverage for cases when < 2 versions

604d2e1

rename file

78aae8f

justinmk3 reviewed Jan 23, 2025

View reviewed changes

jpinkney-aws reviewed Jan 24, 2025

View reviewed changes

Hweinstock added 5 commits January 24, 2025 12:34

Merge branch 'feature/amazonqLSP' into lsp/cleanUp

3b9878f

fix: generalize cleanup to apply to other installer

3b693c6

refactor: avoid exporting unnecessarily

d5b972e

refactor: reduce noise in changes

bf36a48

refactor: move cleanup work to more general location

88db689

jpinkney-aws approved these changes Jan 24, 2025

View reviewed changes

justinmk3 reviewed Jan 24, 2025

View reviewed changes

packages/core/src/shared/lsp/utils/cleanup.ts Outdated Show resolved Hide resolved

justinmk3 reviewed Jan 24, 2025

View reviewed changes

justinmk3 approved these changes Jan 24, 2025

View reviewed changes

Hweinstock added 2 commits January 24, 2025 16:24

refactor: rename cleanup function and move test file

bace7ca

Merge branch 'feature/amazonqLSP' into lsp/cleanup

5e47cb5

Hweinstock force-pushed the lsp/cleanUp branch from 5a69e8d to 5e47cb5 Compare January 24, 2025 21:30

fix: migrate old name

da20782

jpinkney-aws merged commit 7488e0a into aws:feature/amazonqLSP Jan 28, 2025
17 checks passed

Hweinstock deleted the lsp/cleanUp branch January 28, 2025 14:41

feat(lsp): older and delisted versions of lsp are automatically removed #6409

feat(lsp): older and delisted versions of lsp are automatically removed #6409

Uh oh!

Conversation

Hweinstock commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Uh oh!

github-actions bot commented Jan 21, 2025

Uh oh!

Hweinstock commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpinkney-aws Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hweinstock commented Jan 24, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk3 left a comment

Choose a reason for hiding this comment

Uh oh!

Hweinstock commented Jan 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Hweinstock commented Jan 21, 2025 •

edited

Loading

Hweinstock commented Jan 21, 2025 •

edited

Loading

jpinkney-aws Jan 24, 2025 •

edited

Loading