search

eclipse-archived / codewind

The official repository of the Eclipse Codewind project
https://codewind.dev
Eclipse Public License 2.0
114 stars 44 forks source link

Codewind should react to a project being deleted from disk #2278

Closed tetchel closed 4 years ago

tetchel commented 4 years ago

If I delete a project's entire root directory, Codewind doesn't react to it at all until I do a manual build, even if auto-build is on.

tetchel commented 4 years ago

Not sure which area of the sync/auto-build cycle this is part of, so I added both.

tobespc commented 4 years ago

Is the filewatcher spotting the directory has been deleted ?

tetchel commented 4 years ago

Yes it is, if I rm -rf /Users/tim/programs/vscode-codewind-workspace/apps-mpp here are the filewatcherd logs. In particular note 121.188 and the few lines underneath.

[March 2, 15:29:43.189] [113.813] Received event [DELETE] /Users/tim/programs/vscode-codewind-workspace/apps-mpp/target/test-classes/it/dev/appsody/starter/HealthEndpointTest.class
[March 2, 15:29:43.196] [113.820] Received event [DELETE] /Users/tim/programs/vscode-codewind-workspace/apps-mpp/target/test-classes/it/dev/appsody/starter
< 2000 lines of other deleted files under the deleted directory >
[March 2, 15:29:50.564] [121.188] Received event [DELETE] /Users/tim/programs/vscode-codewind-workspace/apps-mpp
[March 2, 15:29:51.575] [122.199] Batch change summary for c27fe0f0-54f6-11ea-a588-231bd32b0ee5 @ 1583180990564: [ -HealthEndpointTest.class -starter -appsody -dev -it -test-classes -com.ibm.websphere.appserver.appClientSupport-1.0_it.properties -com.ibm.websphere.appserver.kernelCore-1.0.properties -com.ibm.websphere.appserver.appClientSupport-1.0_cs.properties -com.ibm.websphere.appserver.appSecurity-2.0_zh.properties  (...) ]
[March 2, 15:29:51.576] [122.200] Calling cwctl project sync with: { [--insecure] [project] [sync] [-p] [/Users/tim/programs/vscode-codewind-workspace/apps-mpp] [-i] [c27fe0f0-54f6-11ea-a588-231bd32b0ee5] [-t] [1583180880036] }
[March 2, 15:29:51.816] [122.440] ! ERROR !  Error running 'project sync' installer command time="2020-03-02T15:29:51-05:00" level=error msg="stat /Users/tim/programs/vscode-codewind-workspace/apps-mpp: no such file or directory"

[March 2, 15:29:51.816] [122.440] ! ERROR !  Stdout:No output
[March 2, 15:29:51.817] [122.441] ! ERROR !  Stderr:time="2020-03-02T15:29:51-05:00" level=error msg="stat /Users/tim/programs/vscode-codewind-workspace/apps-mpp: no such file or directory"

[March 2, 15:29:51.817] [122.441] !!! SEVERE !!!  Unexpected exception from CLI:
[object Object]
[March 2, 15:29:54.682] [125.306] Initiating GET request to http://127.0.0.1:10000/api/v1/projects/watchlist
[March 2, 15:29:54.720] [125.344] GET response received: {"projects":[{"projectID":"c27fe0f0-54f6-11ea-a588-231bd32b0ee5","projectWatchStateId":"cea8b8efd23ef252b702e84c1e2a6894","pathToMonitor":"/Users/tim/programs/vscode-codewind-workspace/apps-mpp","ignoredPaths":["*/.idea/*","*.iml","/.project","/load-test*","*/*.swp","*/*.swx","*/.gitignore","*/node_modules*","*/4913","*/.git/*","*/.DS_Store","*/.dockerignore","*/*~","/.settings","/.classpath","/.options","/.vscode/*"],"refPaths":[],"projectCreationTime":1583180880067},{"projectID":"39791e00-58c2-11ea-99b8-05c1a3bc112f","projectWatchStateId":"32eef779d71ae6c80693ef97e301f5d6","pathToMonitor":"/Users/tim/programs/vscode-codewind-workspace/appsnode","ignoredPaths":["*/.idea/*","*.iml","/.project","/load-test*","*/*.swp","*/*.swx","*/.gitignore","*/node_modules*","*/4913","*/.git/*","*/.DS_Store","*/.dockerignore","*/*~","/.settings","/.classpath","/.options","/.vscode/*","/run-dev","/run-debug","/package-lock.json*","/nodejs_restclient.log","/nodejs_dc.log","/manifest.yml","/idt.js","/cli-config.yml","/README.md","/Jenkinsfile","/.cfignore"],"refPaths":[],"projectCreationTime":1583180874904}]}
[March 2, 15:29:54.721] [125.345] Examining file watch state from GET request
[March 2, 15:29:54.722] [125.346] The project creation time has changed, when both values were non-null. Old: 1583180280889 New: 1583180880067(Mon Mar 02 2020 15:28:00 GMT-0500 (Eastern Standard Time)), for project c27fe0f0-54f6-11ea-a588-231bd32b0ee5
[March 2, 15:29:54.723] [125.347] The project watch state has not changed for project c27fe0f0-54f6-11ea-a588-231bd32b0ee5
[March 2, 15:29:54.723] [125.347] The project creation time has changed, when both values were non-null. Old: 1583180271547 New: 1583180874904(Mon Mar 02 2020 15:27:54 GMT-0500 (Eastern Standard Time)), for project 39791e00-58c2-11ea-99b8-05c1a3bc112f
[March 2, 15:29:54.724] [125.348] The project watch state has not changed for project 39791e00-58c2-11ea-99b8-05c1a3bc112f
[March 2, 15:29:54.724] [125.348] HttpGetStatus - GET request loop complete.
[March 2, 15:30:00.095] [130.719] IDE returned a new security token to filewatcher: eyJhbGciOiJSUzI1NiIsInR5cCIgOiAi
[March 2, 15:30:00.095] [130.719] Initiating GET request to https://codewind-gatekeeper-k73k7t85.codewind-ide-312685-991fa493e65cfa1b406d26eea92b71a6-0000.tor01.containers.appdomain.cloud/api/v1/projects/watchlist
[March 2, 15:30:00.250] [130.874] GET response received: {"projects":[]}
[March 2, 15:30:00.250] [130.874] HttpGetStatus - GET request loop complete.
[March 2, 15:30:20.569] [151.193] dispose() called on WatchedPath for /Users/tim/programs/vscode-codewind-workspace/apps-mpp
jgwest commented 4 years ago

The current filewatcherd behaviours appear to be:

@tobespc FYI the output of the filewatcher from Tim's previous comment indicates that cwctl is not correctly handling this case, eg the following is printed by cwctl:

time="2020-03-02T15:29:51-05:00" level=error msg="stat /Users/tim/programs/vscode-codewind-workspace/apps-mpp: no such file or directory"

It's a bit tough to tell from the log output, but that is from cwctl stdout/stderr, and not part of the FWd log statement. I presume cwctl is assuming that the -p directory param exists.

tobespc commented 4 years ago

if you do delete an entire root directory, what would be the reasoning behind doing that ? Would you be thinking the project should be deleted from codewind (as in unbind) ?

Before I start to put changes in, I want to understand the use case a bit better

tetchel commented 4 years ago

I don't know, but i expected at least something to happen 😅

tetchel commented 4 years ago

I guess the reasons to delete the directory would be by accident, to try and move it to another location on disk, or if I wasn't interested in the project any more but didn't know how / forgot to remove it from Codewind first.

jgwest commented 4 years ago

My expectation is that if you delete the project directory (for whatever reason), it should remove the project from Codewind (eg delete project files inside the container, delete the application containers, stop tracking its state, remove it from the active projects list, etc).

tobespc commented 4 years ago

What should happen (based on current design) is

If this is not the case then this should be a bug

jgwest commented 4 years ago

Presuming that your definition of 'project files deleted from disk' includes the root project directory itself, then based on the behaviour we are seeing, the following are currently not true:

So I will convert this item to a bug.

tetchel commented 4 years ago

@tobespc the filewatcherd PRs have been merged, so I think the only blocker for this is cwctl falling over when the deleted directory doesn't exist

jcockbain commented 4 years ago

/assign

jcockbain commented 4 years ago

It's correct that a blocker for cwctl is if a non-existing local dir is passed as the -p parameter to sync. I'm assuming that's whats being called by the filewatcher/IDE when the root project dir is deleted?

The flow seems to work as expected if the entire contents of project is deleted i.e. rm -rf /workspace/project/*. Where cwctl is erroring is if the local project path (-p parameter) doesn't exist (i.e the whole project has been deleted).

From reading this issue, it seems to me that the call made to cwctl after project root dir deletion is the same as with standard sync. The only difference being the path now doesn't exist? So cwctl will need a way to differentiate between a situation where a user has deleted the project and one in which an incorrect path has been given. Cleaning up the project in the former case and returning an error in the latter.

This could be a check against the locOnDisk of that project. If the given path matches, sync. If not, return an error. Currently, that check is only whether the localPath exists or not.

jcockbain commented 4 years ago

An alternative would be to return a specific error (with the relevant projectID / name) in the situation where sync is attempted with a local path that doesn't exist. The IDEs could then ask the user if they want to remove that project from Codewind, and if they do cwctl project remove can handle clean-up in the normal way.

Something like this (which I have on a branch of cwctl), jim is a project I created then removed its root dir: 

cwctl --json project sync --path ~/Documents/workspace/jim --id cfc97cf0-78df-11ea-9bf1-371838e29e51 --time 1586271449631
{"error":"proj_path","error_description":"Path of project jim doesn't exist locally"}
exit status 1

That's my preferred option personally.

Update: Having discussed this in the Portal scrum, I was made aware this error is being returned to the filewatchers and will not be seen by the IDEs. So this implementation may need a similar message emitted to the UI socket?

tetchel commented 4 years ago

If you send a projectDeletion event over the socket the IDE will delete the project from the codewind view

jgwest commented 4 years ago

It's correct that a blocker for cwctl is if a non-existing local dir is passed as the -p parameter to sync. I'm assuming that's whats being called by the filewatcher/IDE when the root project dir is deleted?

Yep, the filewatcherd (Node/Go/Java) and the IDE (I believe) is calling cwctl with -p pointing to the expected location of the project (which now no longer exists).

Where cwctl is erroring is if the local project path (-p parameter) doesn't exist (i.e the whole project has been deleted).

Correct.

From reading this issue, it seems to me that the call made to cwctl after project root dir deletion is the same as with standard sync. The only difference being the path now doesn't exist?

All correct.

And some of the implementation details depend on how y'all want to handle this scenario: Should we inform the user (via IDE UI socket) and just let the user sort it out? Should we automatically delete the project the from codewind server (and inform the IDE that the project is gone)? etc.

Having discussed this in the Portal scrum, I was made aware this error is being returned to the filewatchers and will not be seen by the IDEs.

Yep, it would usually be the filewatchers that notice this, and they have don't have a direct connection to the IDE. (the filewatchers and the IDE are treated as separate entities, as, for example, the filewatcher that runs in Che [the Go filewatcher] has no IDE. And so the filewatcher architecture is designed to run with very limited connection to the IDE, if any.)

As I said it's up to you all how this is handled, but I had envisioned it something like: 1) Filewatcher detects that project path is gone and calls cwctl project sync (filewatcher will now correctly do this as of latest patches) 2) cwctl project sync notices the path is gone, and issues an HTTP request to the Codewind server informing it that the path directory no longer exists on disk. (whether via the sync api, or a new api) 3) The codewind server receives that request and removes the project from its internal project list (performing an equivalent to a delete/unbind, I guess?) and then informs the IDE of the change via UI socket 4) IDE does any UI cleanup and/or any user informing

(@jcockbain)

jcockbain commented 4 years ago

Thanks @jgwest, @tetchel

Steps 2 and 3 wouldn't be much extra work to implement. The majority of that logic already exists in the cwctl project remove command and the Codewind server /unbind API respectively.

Our concerns are around cwctl clearing up a project behind the scenes, without the user verifying it. With that flow, if sync is called with a filepath not on a user's filesystem, the project would be deleted, correct? I can also add a check that the given path equals the locOnDisk of the project, but that currently isn't checked and limits syncing to the locOnDisk set for the project.

To allow a user to verify, a slightly extended flow could be something like this?

  1. cwctl project sync called with non-existing dir, this calls the Codewind server to emit a UI socket message (different from projectDeletion), stating the local path of the project is missing.

  2. The IDEs present the user with the option to delete the project. If they confirm, cwctl project remove or /unbind API is called as normal.

Or, as you said @jgwest, without a step 3 it could just be left for the IDE user to clear up the project.

tetchel commented 4 years ago

To implement 3. you would have to either add a payload to the projectDeletion event to indicate a prompt is necessary, or add a new message type.

edit: nvm, you addressed that in item 2

also, we'd have to make sure cwctl project remove won't error if the directory has already been deleted.

jcockbain commented 4 years ago

There's a deleteFiles boolean flag on the delete command I think, which should ignore the local directory if set false.

tetchel commented 4 years ago

yeah, i guess the prompt that sets that parameter would be skipped in this scenario, that works.

jcockbain commented 4 years ago

@tetchel is there IDE preference be for a modified payload on the projectDeletion message, or new message type?

tetchel commented 4 years ago

I guess a new message is clearer

hhellyer commented 4 years ago

I'm reviewing James's PR's and can't help feeling this is all bit over complicated. We are essentially adding an API onto PFE that just triggers sending a socket message straight back so the local file-watcher can notify the local IDE that a directory is missing. Sending that message by round tripping it through a REST call and socket message doesn't seem the most straight forward solution.

Given that we are changing both the file-watcher, to spot the directory has been deleted and the IDE plugins to handle a socket message saying the directory has been deleted locally wouldn't it be easier to provide the file-watcher a way to tell the IDE directly?

In the VSCode plugin there is already a callback object it can use to get authentication details, IAuthTokenProvider : https://github.com/eclipse/codewind-vscode/blob/6991172281d5b5a7956e07a3aa018987c314e2c0/dev/src/codewind/connection/RemoteConnection.ts#L158 providing something similar to that mechanism to handle things like this would make sense. This issue is essentially a local filesystem problem, it seems excessive to handle it going via a REST API call. There's not really any need to involve PFE until the user has decided if they want to delete the project or not.

Outside testing this issue is most likely to occur at startup/connection time if folders or workspaces have been moved/deleted so it would make sense to handle locally, early on. (Also as an aside we should verify the editors cope if multiple projects disappear at once, for example if someone deletes a workspace folder holding 5 projects in sub directories how does the IDE plugin show this?)

jgwest commented 4 years ago

I should write this up in full as a design doc, as every time folks encounter the filewatcher they want to connect the IDE to it 😄

But in short:

And another (probably less convincing) point - the filewatcher architecture was designed to support the headless IDE case, and I'd like not to shut the door on that:

So, the high level is that until we get rid of the Go filewatcher one cannot assume an IDE is present, and even after that I would push back on increasing IDE integration for the reasons described above.

hhellyer commented 4 years ago

That helps a little, although now I'm worried about the scenario where we have a standalone file-watcher and the project folder is removed, as you say that's mostly hypothetical at this point though. (What happens to the running filewatcher? How do we make a decision about what to do with the project? How does the file-watcher get shut down? Does it just exit and stop watching that folder?)

I think there's still a pretty high cost, we aren't avoiding altering the filewatcher (there's still a new API it has to call just on PFE instead of the callback) we are still altering all 3 IDEs (to handle the new socket message) and we are also altering PFE but the Che scenario does probably mean we have to do this via PFE and the need to use the VSCode plugin in Che unaltered as much as possible probably means we need to do this.

jcockbain commented 4 years ago

@tetchel - the cwctl and PFE PRs have been merged. So now when cwctl sync is called with a non-existent path matching the project's locOnDisk, it won't error as it did before and a missingLocalDir socket message should be emitted with the body {projectID: <projectid>}.

This issue was discussed yesterday in Portal's scrum and whilst this way around does seem a bit long winded, there wasn't a clear simpler solution suggested.

Happy to discus anything else that needs adding/changing from a PFE and cwctl side once the IDEs get a chance to work with this.

tetchel commented 4 years ago

ok, i'll open an issue for the IDE component.