Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File watcher breaks when using Helix editor #2131

Open
LatjoLajbin opened this issue Oct 1, 2024 · 13 comments · May be fixed by #2345
Open

File watcher breaks when using Helix editor #2131

LatjoLajbin opened this issue Oct 1, 2024 · 13 comments · May be fixed by #2345

Comments

@LatjoLajbin
Copy link

Running d2 with filewatching breaks as soon as a save is performed, as helix creates and removes a backup file in case of crashes.

Found a similar report in the discord where the user was prompted to make a github issue, but I'm making this since I couldn't seem to find one.

OS: Windows
Shell: PowerShell
Editor: Helix
Installed via: Scoop (v0.6.7)

Wondering if this is related to the 16 milliseconds of event catching in d2cli/watch.go? It feels weird that it's receiving the file system event REMOVE and then immediately tries to ensure that it's being filewatched.

{11631616-B09F-4312-9FD5-925D3D9FCF3A}

There's a discussion in the helix repo regarding a similar issue in Hugo, where it was solved in Hugo by adding .bck files as file type exceptions.

helix-editor/helix#11715
gohugoio/hugo#12856

@cyborg-ts cyborg-ts added this to D2 Oct 1, 2024
@alixander
Copy link
Collaborator

oo interesting, thanks for the legwork researching this

@alixander
Copy link
Collaborator

i installed helix and tried to reproduce but could not. is there a setting you're using to turn on backup files?

Kapture.2024-10-01.at.12.42.50.mp4

@LatjoLajbin
Copy link
Author

Not really, no. if I read it right, the error shows up in os.Stat(path) [go.watch:356]. Does powershell do something different than zsh when trying to find a file? I don't have wsl setup right now to test with on the same machine...

I only found out about the backup file writing through this issue, but my understanding is that it's a baseline for most editors that safeguards against file corruption if the editor crashes during write.

{0F49669C-98AC-4076-BE37-9B642D9CB28D}

@alixander
Copy link
Collaborator

i think related to #9

I wonder if this issue is circumvented if we just do atomic writes

@alixander
Copy link
Collaborator

Let's see in the next version if this still is an issue: #2141

@LatjoLajbin
Copy link
Author

Following up on this to say that I am still having the same issue. Any idea of something I could try to bypass it?

image

@alixander
Copy link
Collaborator

@LatjoLajbin Thank you for the swift followup. I just have to confirm, d2 version prints 0.6.8?

@LatjoLajbin
Copy link
Author

Yes, confirmed. Also tried building from source before this release but didn't yet step through sequentially or anything

@scajanus
Copy link

Hi, I managed to reproduce this in Windows and Neovim with d2 version 0.6.9. I also found a workaround, that points to a possible solution: It seems like the file watcher updates its pointer to a backup file that is created prior to writing, and this file is deleted after a successful write but the reference is never shifted back to the original file.

I don't know enough about filesystem file reference handling, but this feels like it might be specific to Windows. Also speculating, but if other editors backup before save to a separate file instead of moving the original, this bug would not show up in those.

>d2 --theme=300 --dark-theme=200 -l elk -w ./automation.d2
success: listening on http://127.0.0.1:51063
info: compiling automation.d2...
success: successfully compiled automation.d2 to automation.svg in 415.2489ms
info: broadcasting update to 0 clients
success: GET / 200 457B 0s
success: GET /static/watch.js 200 2,661B 42.1296ms
success: GET /static/watch.css 200 206B 42.1296ms
success: GET /watch 101 0B 511µs
success: GET /static/favicon.ico 200 1,150B 0s
err: failed to watch "automation.d2~": GetFileAttributes: The system cannot find the file specified. (retrying in 2s)
err: failed to watch "automation.d2~": GetFileAttributes: The system cannot find the file specified. (retrying in 4s)
err: failed to watch "automation.d2~": GetFileAttributes: The system cannot find the file specified. (retrying in 8s)
warn: received signal interrupt: shutting down...

Next I tried to manually save the file as automation.d2~, which resulted in a similar error message:

err: failed to watch "automation.dz~": GetFileAttributes: The system cannot find the file specified. (retrying in 2s)

This looked like the way Vim/Neovim cycles through swap files, so I checked if it is related to something similar. This turned out to be true: Disabling the setting "writebackup" (:set nowritebackup, "Make a backup before overwriting a file. The backup is removed after the file was successfully written, unless the 'backup' option is also on.") solves the issue.

>d2 --theme=300 --dark-theme=200 -l elk -w ./automation.d2
success: listening on http://127.0.0.1:51310
info: compiling automation.d2...
success: successfully compiled automation.d2 to automation.svg in 415.0945ms
info: broadcasting update to 0 clients
success: GET / 200 457B 0s
success: GET /static/watch.js 200 2,661B 67.125ms
success: GET /static/watch.css 200 206B 67.125ms
success: GET /watch 101 0B 507.2µs
success: GET /static/favicon.ico 200 1,150B 0s
info: detected change in automation.d2: recompiling...
success: successfully compiled automation.d2 to automation.svg in 404.2346ms
info: broadcasting update to 1 client
success: successfully compiled automation.d2 to automation.svg in 564.7307ms
info: broadcasting update to 1 client

@LatjoLajbin if you could be so kind as to see if Helix has a similar setting to disable write backups and write to the file directly instead, could you comment on this again if that solves the issue for you as well?

@LatjoLajbin
Copy link
Author

@scajanus I might be about to spread some misinformation right now since I'm typing this out on my phone off the top of my head, but I believe I recall this discussion in the Helix repo.

It was requested to add an option for disabling write back-ups, but it was rejected as the integrity of files was paramount as a design choice. As such, the option doesn't exist right now.

I'll find the discussion and link it / modify this post when I'm able to.

@alixander alixander linked a pull request Feb 11, 2025 that will close this issue
@alixander
Copy link
Collaborator

alixander commented Feb 11, 2025

Can someone build from source on this branch and test? #2345

Going the Hugo route of just ignoring backup files, which should fix the issue.

@scajanus
Copy link

scajanus commented Feb 13, 2025

I don't have admin rights on my (client's) Windows laptop, which makes installing software a bit cumbersome. I can still try to setup Go and compile this sometime next week if no one else has easier access to a setup to try it out. Couldn't reproduce it on my Mac with Neovim, so looks like this is specific to Windows.

In Vim/Neovim the backups generally append a tilde character, but it looks like if a file with that name (tilde appended) exists, it has some logic to decide on the new file name (e.g. in my example, the backup was first named automation.d2~, and if that already existed, automation.dz~). Furthermore, it is possible for the user to set a different location and/or suffix to use for the write backups. This question has some context: https://stackoverflow.com/q/12062175

I think the most general solution would to not follow the file around if it is renamed, and just stick with the original filename. Looking at the documentation of https://github.com/fsnotify/fsnotify, they mention:

Watch the parent directory and use Event.Name to filter out files you're not interested in. There is an example of this in cmd/fsnotify/file.go.

I haven't programmed in Go so I didn't look at the MR too deeply -- are you watching the files directly? Looks like to library suggests to watch the whole folder, and simply ignore events that don't exactly match the original argument.

@alixander
Copy link
Collaborator

Thank you @scajanus , yes that's basically what the PR is doing: ignore backup/temp-looking files. Not the most robust as that's a moving target, but good enough for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

3 participants