Try using a unique temp file for vdocs

[juliasilge]

Maybe addresses a category of problem some users are experiencing in Positron:

• R crashing when editing long quarto files posit-dev/positron#3792
• execution of code in quarto cell sometimes works, sometimes does not posit-dev/positron#3945
• R exiting with error code 0 posit-dev/positron#4437
• https://github.com/posit-dev/positron-beta/discussions/227

No one on the team has been able to observe this problem directly but it is clearly real. @DavisVaughan came up with the hypothesis that maybe the problem is that the vdoc tempfiles are getting overwritten by different requests. Quoted from Slack:

whenever an LSP feature is fired that requires a virtual doc, quarto takes the TextDocument, turns it into virtual doc form (with comments in place of prose and code block fences), and writes it to a temp file
then that temp file is used by the lsp, and it is deleted on the next lsp request
here's where i think the problem may be. every single one of these temp files for R gets the same name, .intellisense.r

since we are in async world here is what may be happening:

• a hover request starts with a particular position, we make the virtual doc, .intellisense.r (hover), but then hit an await before we complete it
• maybe some text edits happen
• a completion request immediately fires, we start handling that. to make its virtual doc we overwrite the original one
• we get back to the hover request after the await, but now the position doesn't line up with the contents of the .intellisense.r document
• ark finally handles it and things blow up

This PR uses UUIDs to make a whole bunch of different tempfiles, one for each request, rather than one file that could get deleted before the request can be handled. This is maybe overkill but we can have folks get the .vsix to try it out.

[cscheid]

Oof, that's a nasty failure case, thanks for finding it!

I like the uuid solution a lot, and this is good to merge as far as I'm concerned.

One question for our collective pondering: how concerned are we about creating large numbers of temporary files here? If we're getting multiple virtualDocUriFromTempFile calls that race each other, should we also be concerned about leaking files? Like I said, I think this is good to merge as is, because at worst it trades a crashing bug for a resource leak and that's a win. But maybe we can fix that too?

[juliasilge]

Looks like success 🤞 reported in posit-dev/positron#3792 (comment). I've pointed the other folks here to get the .vsix as well so let's see if folks are able to try it out.

[juliasilge]

For the long term, is it possible to use in-memory virtual documents? Like with a true LSP backend? Do you have context for why the extension is using temp files in this way?

In the short term, I definitely would advocate for merging something along the lines of this PR, pending another person or two confirming that this fixes their problem, and releasing the extension ASAP. This has been probably the most pernicious crash type Positron users have run into since we went public!

[juliasilge]

Looks like another report of success here: posit-dev/positron#3945 (comment)

[cscheid]

Do you have context for why the extension is using temp files in this way?

I don't. I wasn't involved in writing that code, so I'm reading it for the first time and figuring it out.

It looks like virtualDocUri in apps/vscode/src/vdoc/vdoc.ts calls virtualDocUriFromTempFile if virtualDoc.language.type === "tempfile", where EmbeddedLanguage.type is "content" | "tempfile". In apps/vscode/src/vdoc/languages.ts, there's an additional hint at kEmbededLanguages (sic) that mentions languages requiring creating a temp file. The last four languages declared have {type: "content"} and a comment about "working with text document content provider", so I assume the other ones didn't work for some reason and requires the tempfile route.

[DavisVaughan]

should we also be concerned about leaking files

We definitely leak files like crazy now, since we never clean up .intellisense.*.R now and we make one at each LSP call (so, A LOT)

Still super nice to know what the fix is, we just need a way to clean up when the server is done processing the request (which i think isn't straightforward).

[DavisVaughan]

Oh wait, I'm dumb. These are all LSP requests. Meaning we get a reply back from the server when its done. Duh! So when we get the reply we just clean up the temp file. So if we want to keep the tempfile approach I think we can make this work without leaking files.

We are even doing this already! In this local branch the tempfile we create has a cleanup hook attached that deletes it

quarto/apps/vscode/src/vdoc/vdoc-tempfile.ts

Line 51 in 97cc659

cleanup: async () => await deleteDocument(doc)

And we do call this cleanup hook in a number of places. Here is an example:

quarto/apps/vscode/src/lsp/client.ts

Line 237 in 97cc659

await vdocUri.cleanup();

But what I really like is this withVirtualDocUri() helper that keeps you from footgunning yourself

quarto/apps/vscode/src/vdoc/vdoc.ts

Lines 124 to 132 in 97cc659

	export async function withVirtualDocUri<T>(virtualDocUri: VirtualDocUri, f: (uri: Uri) => Promise<T>) {
	try {
	return await f(virtualDocUri.uri);
	} finally {
	if (virtualDocUri.cleanup) {
	virtualDocUri.cleanup();
	}
	}
	}

We should consider using this everywhere we use a virtual doc right now, and consider taking it a step further as just withVirtualDoc() to include the whole virtual doc creation / teardown, rather than just the URI part.

[DavisVaughan]

Regarding why we do the tempfile approach. It is possible that some LSP requests require workspace context. So by putting the tempfile in the workspace alongside the qmd (where other R files might also exist), then the LSP has the capabilities to process the request on the tempfile with the context of other R files around it

(this is a somewhat educated guess from reading the sources)

[lionel-]

(Sorry I pushed a commit here by mistake, I force-pushed it away)

[lionel-]

Some more observations:

As long as a document is opened (in the sense of vscode.workspace.openTextDocument()), all changes are tracked by VSCode and corresponding events emitted in the correct order. If a document is closed, VSCode stops tracking changes. In both cases (document changed and document closed) code assistance requests should be cancelled.

Cancellation works via a cancellation token that must be checked manually by assistance providers. We currently fail to do that, and instead propagate the assistance request to LSP backends. These backends then are at risk of operating on outdated documents and provide incorrect results or even crash.

The workaround taken in this PR is reasonable. It creates a unique document for each request and any further change causes a new unique document to be created. This guarantees that requests operate on an unchanging source and prevents races. That may be the safest way we can implement the bridge between chunks and assistance providers. And if the provider returns results for outdated documents, VSCode will ignore them. So as long as it doesn't cause performance issues, we probably want to keep doing that.

However this approach only hides the problem. We should also make an effort to cancel propagation of assistance requests to LSPs, if only to avoid unnecessary traffic with the backends. Here is an example with the completion provider: 99be124

We should consider using this everywhere we use a virtual doc right now, and consider taking it a step further as just withVirtualDoc() to include the whole virtual doc creation / teardown, rather than just the URI part.

I was looking at this too, should be easy enough: 0eaea03

Regarding why we do the tempfile approach. It is possible that some LSP requests require workspace context. So by putting the tempfile in the workspace alongside the qmd (where other R files might also exist), then the LSP has the capabilities to process the request on the tempfile with the context of other R files around it

IIUC this is the "local" code path, currently used for formatting and jump-to-definition. This stores the files alongside the qmd files. But here we are working with the non-local path that stores files in a temp folder. I think files from a virtual document provider would be treated in the same way, and be part of the workspace just like these tempfiles?

[lionel-]

@cscheid If you have time would you mind doing a final review? If not no worries, we've closely looked at it with @DavisVaughan already.

We're aiming to merge before Wednesday end of day so this can get in the next Positron release.

The main thing that we changed in addition to Julia's fix is to make sure that temporary files are now cleaned up consistently and we don't leak any. In particular, we opted for removing the "same content" optimisation so that we don't have to reuse the same tempfile for multiple LSP requests, which would complicate their lifetime/ownership management.

We feel like there is room to do better in the future but for now we wanted to make the simplest possible change.

[lionel-]

This was an optimisation for when multiple LSP requests are performed in a row but the underlying file hasn't changed. We removed it to simplify the lifetime management of the tempfiles. We now always create a new tempfile for each request and each caller is in charge of cleaning up the file (we double-checked that all callers currently do clean them up).

[lionel-]

New constructor for a vdoc-uri with a cleanup function.

[lionel-]

Now cleaning up the tempfiles in this code path too.

Previously it would only be deleted manually when a change was detected in the contents.

[juliasilge]

Another report of success here: posit-dev/positron#4437 (reply in thread)

[juliasilge]

Carlos is OOO so let's go with the work as is, thoroughly checked out by Lionel and Davis and confirmed by our couple of Positron users who were having trouble. ✅