Various fixes for updating dircounts with watch enabled

[joelim-work]

• Fixes fsnotify 1.8.0 crashes lf on symbolic links #1825
• Fixes File size incorrect after copy #1874

[valoq]

I noticed that with this branch the entire watch feature sometimes stops working. It seems quite random, as if there was a race condition but most often it can be observed during move and delete operations. When I delete a file, nothing seems to happen and I need to manually reload to see the file disappear or be moved.

[joelim-work]

I haven't noticed any issues with moving/deleting files so far myself. Moving/deleting files should cause the directory to be reloaded - you can try following the code from app.nav.dirChan to see if you can find the issue.

[valoq]

I tried finding the issue by tracing the path, however the bug appears to be very unreliable.
There are times when no matter what, everything works as expected and then suddenly it does not.
I did find out though that it is not related to the move or update function at all but rather affects the entire watch feature, as if fsnotify stopped working at random times. The bug also only seemed to affect the start directory (usually $HOME) as I was unable to observe it anywhere else. I played around with the recent commits quite a bit and found that I was unable to reproduce the issue if I reverted back to 20f1b20
That makes fbb8c86 the prime suspect and since the issue only appeared in the current directory where lf was started, it may be related to this line since I suspect the current directory to be excluded from the watcher. When I observed the path in a debugger though it always worked as expected and I am also unsure why this commit would result in such a strangely random bug.

[joelim-work]

If the issue isn't strictly related to move/update, then it sounds more likely that the directory didn't get registered into the watcher. Have you tried adding a log.Printf call around the watch.watcher.Add(path) code to see what paths actually get watched? It might also be helpful to log received events too, although you have to be careful that the path where the log file is stored isn't watched, otherwise you end up in an infinite loop (i.e. writing to the log file generates events, which in turn get logged).

Other possibilities is that adding a path can technically fail, see https://pkg.go.dev/github.com/fsnotify/fsnotify#Watcher.Add
There's also a function for just getting a list of all currently watched paths: https://pkg.go.dev/github.com/fsnotify/fsnotify#Watcher.WatchList

[valoq]

I logged all the path added by the watcher now and found a few more things:

1. Every path is actually called by watch.watcher.Add(path)
2. The bug does appear even when the path is added.
3. It is not only the current directory that is affected, but rather the entire watch feature
4. If the bug triggers, it persists throughout the lifetime of the lf process. No file updates are ever displayed in the UI without manual reload, no matter which path is updated.

For testing I do have two versions of lf running:
A) the current version of https://github.com/joelim-work/lf/tree/watch-all
B) the same tree but at this commit 20f1b20, which omits the last 6 commits.

I test for the issue by running both versions next to each other on the same path and adding a file in one of the directories displayed by both versions.

Version A) is randomly affected by the bug, resulting int he entire watch feature not working
Version B) was never observed as affected by the issue even once

I did test Version B) but with fsnotify bumped to v1.8.0 to see if that could be the issue but I did not observe any change.

[joelim-work]

I rebased the branch and force-pushed it to clean up commit history a bit, but the code is still the same.

However I still have no idea what the issue is, because I can't reproduce it on my end. Given that the paths are being watched, I'm surprised it doesn't work for you, since the code here only changes how directories are watched, and not how updates are applied in response to watch events.

I guess there are other possibilities though, for example:

• Watcher.Add being called but resulting in an error
• Updates not being sent out by the watcher
• The modified directory fails to be reloaded
• The directory is reloaded but isn't reflected in the UI

To get to the bottom of this, you'll probably have to log events and trace the code from there, something like below:

diff --git a/watch.go b/watch.go
index d4f70d3..d4e38a0 100644
--- a/watch.go
+++ b/watch.go
@@ -81,6 +81,7 @@ func (watch *watch) loop() {
 	for {
 		select {
 		case ev := <-watch.events:
+			log.Printf("fsnotify event: %v", ev)
 			if ev.Has(fsnotify.Create) {
 				dir := filepath.Dir(ev.Name)
 				watch.addLoad(dir)

[valoq]

Thanks, I may have found the issue with that event logging:

The system I tested this on uses systemd.homed which has the home directory at /home/myuser.homedir and mount the real home to /home/myuser at login. That means the same files exist at both locations. When I logged the events like suggested above and added a file in my home directory, the file was logged as fsnotify event in /home/myuser.homedir twice but never in /home/myuser.
I verified again that this is not an issue related to the fsnotify version, therefore the problem is triggered by something in the last commits.

[joelim-work]

OK I see the problem now. So lf will watch both /home/myuser.homedir and /home/myuser, but it has no way of knowing that they point to the same location because they appear as separate directories. The fact that fsnotify only reports the event for one of the paths is concerning - I would have expected an event for each path that is registered.

I think the reason why the original code happens to 'work' is because the order in which the paths are registered is somewhat consistent, whereas after 539e333 the path is registered when the directory finishes loading (asynchronously from a separate goroutine). But even then, the event will still only be reported for a single path - it's just that in your case it is the path that you were expecting.

If you really want, I can leave out 539e333, but it's sort of just a band-aid fix which hides the real issue in fsnotify.

[valoq]

This construct of mounted home directories is not too uncommon, even outside of systemd. Consequently I would argue to keep lf compatible with this while we report the issue to fsnotify to fix. Once they have released a new version to address this we could merge the changes again.

[joelim-work]

Right I'll will revert 539e333 but keep the diff here just in case I need it again:

Click to expand

diff --git a/app.go b/app.go
index 01ecce3..edd891b 100644
--- a/app.go
+++ b/app.go
@@ -427,7 +427,7 @@ func (app *app) loop() {
 				}
 			}
 
-			app.addWatchPaths()
+			app.watchDir(d)
 
 			app.ui.draw(app.nav)
 		case r := <-app.nav.regChan:
@@ -633,21 +633,17 @@ func (app *app) runShell(s string, args []string, prefix string) {
 	}
 }
 
-func (app *app) addWatchPaths() {
-	if !gOpts.watch || len(app.nav.dirs) == 0 {
+func (app *app) watchDir(dir *dir) {
+	if !gOpts.watch {
 		return
 	}
 
-	paths := make(map[string]bool)
-	for _, dir := range app.nav.dirs {
-		paths[dir.path] = true
-	}
+	app.watch.add(dir.path)
 
-	for _, file := range app.nav.currDir().allFiles {
+	// ensure dircounts are updated for child directories
+	for _, file := range dir.allFiles {
 		if file.IsDir() {
-			paths[file.path] = true
+			app.watch.add(file.path)
 		}
 	}
-
-	app.watch.add(paths)
 }
diff --git a/eval.go b/eval.go
index ecd5a05..e222e44 100644
--- a/eval.go
+++ b/eval.go
@@ -195,7 +195,9 @@ func (e *setExpr) eval(app *app, args []string) {
 		if err == nil {
 			if gOpts.watch {
 				app.watch.start()
-				app.addWatchPaths()
+				for _, dir := range app.nav.dirCache {
+					app.watchDir(dir)
+				}
 			} else {
 				app.watch.stop()
 			}
@@ -582,7 +584,6 @@ func preChdir(app *app) {
 
 func onChdir(app *app) {
 	app.nav.addJumpList()
-	app.addWatchPaths()
 	if cmd, ok := gOpts.cmds["on-cd"]; ok {
 		cmd.eval(app, nil)
 	}
diff --git a/watch.go b/watch.go
index 824bb90..f93da25 100644
--- a/watch.go
+++ b/watch.go
@@ -64,14 +64,12 @@ func (watch *watch) stop() {
 	watch.events = nil
 }
 
-func (watch *watch) add(paths map[string]bool) {
+func (watch *watch) add(path string) {
 	if watch.watcher == nil {
 		return
 	}
 
-	for path := range paths {
-		watch.watcher.Add(path)
-	}
+	watch.watcher.Add(path)
 }
 
 func (watch *watch) loop() {

So to summarize, the changes added will be:

• The change for Fix file size bug when copying files #1876
• Keep all existing watches without removing them
• Bump fsnotify to 1.8.0
• Skip watching /dev

That's the best I can do for now, anything else is pretty much going to run into the issue with mounted directories.

[valoq]

This now seems like a stable solution ready to be merged.