Cannot save untitled notebook file with `notebook.formatOnSave.enabled` = true

[amunger]

1. set notebook.formatOnSave.enabled = true
2. Create a new notebook add some text to the cell
3. save the file saved.ipynb :bug: a blank notebook file saved.ipynb is created and the untitled notebook remains open expected: the saved notebook should be open with correct contents.

[rebornix]

After digging into the code a bit, I found a similar issue https://github.com/microsoft/vscode/issues/138850, which was fixed by checking if the stored working copy is created by the right owner to avoid it being disposed unexpected.

Here it happens in a similar fashion:

• Turn on notebook format on save
• Create an untitled editor (untitled 1), reopen in Jupyter Notebook
  • Type and make it dirty
• Save, since it's untitled, it goes through the Save As workflow
  • Users provide new file name/path (test.ipynb)
• A new working copy for test.ipynb (let's call it workingCopy1) created by https://github.com/microsoft/vscode/blob/86528d8df2c142c9cf8ccf80913b88f97c5e97bd/src/vs/workbench/services/workingCopy/common/fileWorkingCopyManager.ts#L389
• Notebook save participants triggered
  • Format on save participant tries to format every cell
  • Every cell will try to resolve its TextDocument
  • Cell content provider will resolve NotebooEditorModel, which will load a working copy, and since we don't have any ref-counting, it loads workingCopy1
  • After format, we dispose our ref for NotebookEditorModel
  • It will then try to dispose workingCopy1 https://github.com/microsoft/vscode/blob/86528d8df2c142c9cf8ccf80913b88f97c5e97bd/src/vs/workbench/contrib/notebook/common/notebookEditorModel.ts#L67
• Save As continues to run, but then it finds the new working copy workingCopy1 is already disposed, and it quits early https://github.com/microsoft/vscode/blob/86528d8df2c142c9cf8ccf80913b88f97c5e97bd/src/vs/workbench/services/workingCopy/common/storedFileWorkingCopy.ts#L934-L942

Since we are not ref-counting working copy objects, and we share the same object for the resource, it can get disposed by the wrong caller (like https://github.com/microsoft/vscode/issues/138850). I'm not sure what's the easy fix for this case though, as we definitely want to dispose the working copy from the editor model (this is actually the most common scenario). @bpasero any suggestion here?

[rebornix]

Note that, there isn't any data loss. The new file is created and saved to disk properly, it just doesn't replace the dirty untitled editor with the new resource because of the early return.

[bpasero]

@rebornix this is a consequence of the notebook model collection behaving slightly different from the text model collection.

The text model collection has an extra safe guard, to never allow to destroy a dirty working copy:

https://github.com/microsoft/vscode/blob/b19017cea80b6157aa8214c984a70022e77526f2/src/vs/workbench/services/textmodelResolver/common/textModelResolverService.ts#L124-L128

This is provided by a canDispose method here:

https://github.com/microsoft/vscode/blob/b19017cea80b6157aa8214c984a70022e77526f2/src/vs/workbench/services/textfile/common/textFileEditorModelManager.ts#L545

We provide the same method for stored file working copies here:

https://github.com/microsoft/vscode/blob/b19017cea80b6157aa8214c984a70022e77526f2/src/vs/workbench/services/workingCopy/common/storedFileWorkingCopyManager.ts#L647

But it needs to be called from the notebook model collection in a similar fashion here:

https://github.com/microsoft/vscode/blob/b19017cea80b6157aa8214c984a70022e77526f2/src/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.ts#L104

The only alternative I can think of is to somehow pass the reference collection down to the file working copy manager so that the ref counting becomes correct, but I would actually suggest to align the behaviour here and do the same as text files do.

[bpasero]

Oh well, I had forgotten about this code:

https://github.com/microsoft/vscode/blob/048d9f51edb1771330466f138d641aa0e1084f10/src/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.ts#L77-L80

So we do have a mechanism in place to auto-reference when the notebook becomes dirty to ensure we never destroy a dirty notebook working copy. I think here in this case the notebook is not dirty and thus gets disposed for good.

The real issue here is that a notebook working copy gets into existence without being ref counted:

https://github.com/microsoft/vscode/blob/048d9f51edb1771330466f138d641aa0e1084f10/src/vs/workbench/services/workingCopy/common/fileWorkingCopyManager.ts#L467-L471

This will only happen from "Save As" invocations, or when saving an untitled notebook. Already back then I was not a big fan of having to do that because it means the lifecycle of the working copy is not connected to the outside world that is ref counting.

I am not sure how to fix this actually...

[bpasero]

I think it boils down to teaching the notebook collection about a +1 on the reference from within the file working copy manager.

[rebornix]

@bpasero thanks for the explanation, I tried to leverage "canDispose" and found it work great https://github.com/microsoft/vscode/pull/180560 . My understanding is while saving+formatting, the notebook model is still dirty so it can't be disposed, but it will only dispose after the StoredFileWorkingCopy finish saving (save participants + fs save).

However I'm not sure how to handle untitled https://github.com/microsoft/vscode/pull/180560/files#diff-9a4a06f8a593491389b6ab180d612e734522b33432a84c30d58485e09dd864f3R87 as canDispose is implemented in IStoredFileWorkingCopyManager but not IUntitledFileWorkingCopyManager.

[bpasero]

Yes, the gist of that code is to prolong the dispose for as long as the notebook is dirty. But I wonder if this bug can still reproduce when you:

• have a non-dirty notebook
• just save to trigger save participants which do not turn the notebook dirty

This would very likely reproduce for files as well but I guess so far we never had to resolve the model again as part of the save participant. Instead we are passing around the model:

https://github.com/microsoft/vscode/blob/ab7c32a5b5275c3fa9552675b6b6035888068fd7/src/vs/workbench/services/textfile/common/textfiles.ts#L322-L327

I wonder if we could do the same for notebooks as a mitigation? In the end the working copy one also passes the working copy around, so no need to resolve it again:

https://github.com/microsoft/vscode/blob/ab7c32a5b5275c3fa9552675b6b6035888068fd7/src/vs/workbench/services/workingCopy/common/workingCopyFileService.ts#L93-L98

The change you did will work for when the notebook is dirty, and I think you already figured out the subtle nuances that are required to track a reference being created while you are waiting for canDispose, specifically the modelsToDispose Set:

https://github.com/microsoft/vscode/blob/ab7c32a5b5275c3fa9552675b6b6035888068fd7/src/vs/workbench/services/textmodelResolver/common/textModelResolverService.ts#L25

If we go down this path I wonder if there should be a common base class that we can reuse for both files and notebooks so that we do not duplicate this logic.

I still think it would be cleaner to pass down the ref-counted-collection to the manager.

As for untitled working copies / files: I think I never had the need to implement canDispose there because there were no issues with the way how untitled copies are managed. Again, I am not super happy with canDispose and would not want to spread it further if we can avoid it. To clarify, the auto-ref from dirty working copies present today in the notebook resolver was explicitly done to avoid this hack and I liked it (I think Jo did it).

[bpasero]

I looked at your change and it is ✅ , meaning its the same as text files now. Maybe we leave it like that for now...

I looked into where we resolve a working copy from within the manager and there are 2 locations:

Save As (this issue): https://github.com/microsoft/vscode/blob/3a69e153f6c68b2b855fb2f1e5bdb798a16a1ee4/src/vs/workbench/services/workingCopy/common/fileWorkingCopyManager.ts#L467-L471

Restore from move/copy Ops https://github.com/microsoft/vscode/blob/3a69e153f6c68b2b855fb2f1e5bdb798a16a1ee4/src/vs/workbench/services/workingCopy/common/storedFileWorkingCopyManager.ts#L417-L420

Even if we had the reference collection being passed down all the way to there, I would not even know when to dispose the reference in these 2 places :-/. Here I have to assume that the components that create the working copy managers own the lifecycle of all working copies.