fix(lsp): manifest resolver always reports "Failed to download" #7065
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Problem: Ambiguous or misleading log messages. Solution: Refine the logging logic.
justinmk3
force-pushed
the
fixlspinstall
branch
from
07d5546 to
8bdbf20
Compare
justinmk3
changed the title
opieter-aws
approved these changes
Apr 16, 2025
justinmk3
commented
Apr 16, 2025
| const resolved = await tryStageResolvers('getServer', serverResolvers, getServerVersion) | ||
| return resolved | ||
| } finally { | ||
| logger.info(`Finished setting up LSP server`) |
Contributor
Author
There was a problem hiding this comment.
Removed this finally. If an exception was thrown, it didn't really "finish" setup?
justinmk3
commented
Apr 16, 2025
Comment on lines
66
to
+67
| if (!resp.content) { | ||
| throw new ToolkitError( | ||
| `New content was not downloaded; fallback to the locally stored "${this.lsName}" manifest` | ||
| ) | ||
| const msg = `"${this.lsName}" manifest unchanged, skipping download: ${this.manifestURL}` |
Contributor
Author
There was a problem hiding this comment.
!resp.content means "etag didnt change". It doesn't mean "failed to download".
The old code would always report a failed languageServer_setup in the common case (where etag didn't change).
justinmk3
commented
Apr 16, 2025
Comment on lines
+70
to
+72
| const localManifest = await this.getLocalManifest(true).catch(() => undefined) | ||
| if (localManifest) { | ||
| localManifest.location = 'remote' |
Contributor
Author
There was a problem hiding this comment.
The common case is that etag didnt change. So that's handled here by re-using getLocalManifest. ManifestResolver doesn't have a way to tell its caller "I didnt fail but please continue to the next step".
justinmk3
commented
Apr 16, 2025
Comment on lines
-80
to
+88
| logger.info(`Failed to download latest "${this.lsName}" manifest. Falling back to local manifest.`) | ||
| private async getLocalManifest(silent: boolean = false): Promise<Manifest> { |
Contributor
Author
There was a problem hiding this comment.
Always reporting "failed to download" here is misleading because this will happen when "etag didnt change".
justinmk3
force-pushed
the
fixlspinstall
branch
from
8bdbf20 to
4659b80
Compare
justinmk3
commented
Apr 16, 2025
| [this.lsName]: { | ||
| etag, | ||
| // XXX: this stores the entire manifest. vscode warns about our globalStorage size... | ||
| content, |
Contributor
Author
There was a problem hiding this comment.
we are storing the entire manifest in globalStorage :/
justinmk3
force-pushed
the
fixlspinstall
branch
2 times, most recently
from
5e4bfe6 to
3d5fa82
Compare
Problem:
Manifest resolver always reports:
Failed to download latest "…" manifest. Falling back to local manifest.
Solution:
In `fetchRemoteManifest()`, if the ETag indicates no new manifest is
needed, return the local manifest instead of throwing an error
justinmk3
force-pushed
the
fixlspinstall
branch
from
3d5fa82 to
539ff4a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
continues #7043
Problem:
Manifest resolver always reports:
Solution:
fetchRemoteManifest(), if the ETag indicates no new manifest is needed, return the local manifest instead of throwing an errorfeature/xbranches will not be squash-merged at release time.