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
   🐛 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 #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

  vscode/src/vs/workbench/services/workingCopy/common/fileWorkingCopyManager.ts

  Line 389 in 86528d8

  // Resolve target

• 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

    vscode/src/vs/workbench/contrib/notebook/common/notebookEditorModel.ts

    Line 67 in 86528d8

    this._workingCopy?.dispose();

• Save As continues to run, but then it finds the new working copy workingCopy1 is already disposed, and it quits early

  vscode/src/vs/workbench/services/workingCopy/common/storedFileWorkingCopy.ts

  Lines 934 to 942 in 86528d8

  	// We have to protect against being disposed at this point. It could be that the save() operation
  	// was triggerd followed by a dispose() operation right after without waiting. Typically we cannot
  	// be disposed if we are dirty, but if we are not dirty, save() and dispose() can still be triggered
  	// one after the other without waiting for the save() to complete. If we are disposed(), we risk
  	// saving contents to disk that are stale (see https://github.com/microsoft/vscode/issues/50942).
  	// To fix this issue, we will not store the contents to disk when we got disposed.
  	if (this.isDisposed()) {
  	return;
  	}

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 #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:

vscode/src/vs/workbench/services/textmodelResolver/common/textModelResolverService.ts

Lines 124 to 128 in b19017c

	if (model instanceof TextFileEditorModel) {
	// text file models have conditions that prevent them
	// from dispose, so we have to wait until we can dispose
	await this.textFileService.files.canDispose(model);
	}

This is provided by a canDispose method here:

vscode/src/vs/workbench/services/textfile/common/textFileEditorModelManager.ts

Line 545 in b19017c

canDispose(model: TextFileEditorModel): true | Promise<true> {

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

vscode/src/vs/workbench/services/workingCopy/common/storedFileWorkingCopyManager.ts

Line 647 in b19017c

canDispose(workingCopy: IStoredFileWorkingCopy<M>): true | Promise<true> {

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

vscode/src/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.ts

Line 104 in b19017c

protected destroyReferencedObject(_key: string, object: Promise<IResolvedNotebookEditorModel>): void {

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:

vscode/src/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.ts

Lines 77 to 80 in 048d9f5

	// Whenever a notebook model is dirty we automatically reference it so that
	// we can ensure that at least one reference exists. That guarantees that
	// a model with unsaved changes is never disposed.
	let onDirtyAutoReference: IReference<any> | undefined;

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:

vscode/src/vs/workbench/services/workingCopy/common/fileWorkingCopyManager.ts

Lines 467 to 471 in 048d9f5

	if (this.uriIdentityService.extUri.isEqual(source, target) && this.get(source)) {
	targetStoredFileWorkingCopy = await this.stored.resolve(source);
	} else {
	targetStoredFileWorkingCopy = await this.stored.resolve(target);
	}

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 #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:

vscode/src/vs/workbench/services/textfile/common/textfiles.ts

Lines 322 to 327 in ab7c32a

	participate(
	model: ITextFileEditorModel,
	context: { reason: SaveReason },
	progress: IProgress<IProgressStep>,
	token: CancellationToken
	): Promise<void>;

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:

vscode/src/vs/workbench/services/workingCopy/common/workingCopyFileService.ts

Lines 93 to 98 in ab7c32a

	participate(
	workingCopy: IStoredFileWorkingCopy<IStoredFileWorkingCopyModel>,
	context: { reason: SaveReason },
	progress: IProgress<IProgressStep>,
	token: CancellationToken
	): Promise<void>;

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:

vscode/src/vs/workbench/services/textmodelResolver/common/textModelResolverService.ts

Line 25 in ab7c32a

private readonly modelsToDispose = new Set<string>();

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):

vscode/src/vs/workbench/services/workingCopy/common/fileWorkingCopyManager.ts

Lines 467 to 471 in 3a69e15

	if (this.uriIdentityService.extUri.isEqual(source, target) && this.get(source)) {
	targetStoredFileWorkingCopy = await this.stored.resolve(source);
	} else {
	targetStoredFileWorkingCopy = await this.stored.resolve(target);
	}

Restore from move/copy Ops

vscode/src/vs/workbench/services/workingCopy/common/storedFileWorkingCopyManager.ts

Lines 417 to 420 in 3a69e15

	await this.resolve(workingCopyToRestore.target, {
	reload: { async: false }, // enforce a reload
	contents: workingCopyToRestore.snapshot
	});

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.