onDidDeleteFiles misfires when file is already deleted from a different app

[deepak1556]

When testing #85929

Steps to reproduce:

1. use the code example from Deleting multiple files through explorer invokes the event listeners per file #86210 that purposely delays the response from willDeleteFiles handler.

2. Delete a file in the explorer, while the delay from the above handler happens

3. Delete the file from terminal

4. After the willDeleteFiles resolves, it will trigger popup with delete operation failed and should permanently delete the file, and also trigger the onDidDeleteFiles event

When similar thing is performed with the rename handlers, while you try to rename through explorer and delete the file from terminal, the onDidRenameFiles will not resolve, which seems like the behavior that should be followed here as well ?

[bpasero]

@deepak1556

After the willDeleteFiles resolves, it will trigger popup with delete operation failed and should permanently delete the file, and also trigger the onDidDeleteFiles event

This sentence reads weird to me, can you clarify exactly what you expect? Do you mean the event should fire even if the file does not exist?

[deepak1556]

Ah sorry, I was describing what is happening at the moment. When events are delayed in the will* handler, the delete operation still thinks it has to complete the operation on a file that is already deleted and open a dialog asking the user to delete.

I am expecting the event to not fire when the file is already deleted and also it should not trigger a dialog asking to delete something thats already deleted.

[bpasero]

Is this just a test case issue or a real world scenario we want to support eventually?

To summarise:

• all text file service file operations now wait for events to been handled by extensions
• each extension is free to block this operation and alter the file system
• text file service continues operation after that happens

I am not even sure how to solve this issue properly because the state of the file system when the operation started might not be the same anymore after the event has fired. The same applies for create and move.

Open for suggestions.

[bpasero]

For the delete method at least we could send the event and afterwards do an additional check if the file exists assuming that someone might have deleted it already.

[deepak1556]

Is this just a test case issue or a real world scenario we want to support eventually?

Didn't think about a real world scenario for this, aim here was to demonstrate an evil scenario. The current api design seems too easy to make evil extensions that will just mess UI with the file operations in the explorer because of the OnWill* handler. Just want to ensure how to make this harder to achieve.

Avoiding OnWill* event emits for explorer UI operations would solve this, as I see the handler is only allowed to delay the operation and not cancel it any way, why would extensions want to delay the UI behavior ?

[jrieken]

why would extensions want to delay the UI behavior ?

Because they often need to do some async computation before knowing if this event requires followup work. E.g. before renaming a file a language service computes further edits and that's usually async.

I don't think much is wrong here and disagree that this is a sharp knife for evil extensions. Tho, I think is fishy is this

and also trigger the onDidDeleteFiles event

When deleting fails it should not continue. @bpasero is it that the underlying file service handles the error already and that this never throws?

[bpasero]

@jrieken yes, IFileService#del does not throw unless the error is something else than ENOENT. This was like that since the beginning before we had file system providers. Are you suggesting to change that to throw?

[jrieken]

Yeah, that's what I am suggesting. Otherwise I don't know how to prevent the firing of a did-delete-event in the case of failure. Generally, file operations like delete are prone for failure and IMO the caller of such methods should handle those failure (and they are likely the best suited for doing so)

[bpasero]

Well the alternative solution would be what I suggested earlier: the text file service needs to validate the state of files after the various potentially long running events have been fired. E.g.:

• delete: check if file got deleted meanwhile and then ignore operation
• move: same?
• copy: same I guess

[bpasero]

IFileService#del() throws if resource does not exist.

[connor4312]

Maybe I need more repro steps, but still see what I think Deepack is reporting in the initial issue

I am expecting the event to not fire when the file is already deleted and also it should not trigger a dialog asking to delete something thats already deleted.

Please let me if there's a different verification I should try running.

[deepak1556]

@connor4312 after proceeding to Delete Permanently now we get a error message with Unable to delete non-existing file <path> and the onDidDeleteFiles event is no longer fired. That was the fix that landed.

[connor4312]

Ah, okay. Yes I can verify that after hitting "Delete Permanently" we no longer get an error popup