🔧 Fix file path and watcher handling for config and status files#2196
🔧 Fix file path and watcher handling for config and status files#2196
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes file path handling and adds support for fsnotify.Rename events in the file watcher for configuration and status message files. The changes address issues where text editors that use atomic writes (save to temp file, then rename) would cause the file watcher to stop monitoring the files.
Changes:
- Removed
./prefix from file path constants to normalize paths - Added fsnotify.Rename event handling alongside fsnotify.Write events
- Changed error handling to use blank identifier for config reload errors
- Added watcher re-registration after rename events to maintain file monitoring
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _ = config.ReadConfig(event.Name) | ||
| if event.Has(fsnotify.Rename) { | ||
| _ = watcher.Add(event.Name) | ||
| } | ||
| case statusMessagesFileName: | ||
| log.Println("modified file:", event.Name) | ||
| ei.LoadStatusMessages(event.Name) | ||
| if event.Has(fsnotify.Rename) { | ||
| _ = watcher.Add(event.Name) |
There was a problem hiding this comment.
The error from config.ReadConfig(event.Name) is now being silently ignored (changed from checking and logging to _ =), and the subsequent watcher.Add error is also ignored. This is problematic because:
- If
ReadConfigfails, the application will continue running with stale configuration, which could lead to unexpected behavior - If
watcher.Addfails after a rename event, the file will no longer be watched, meaning future changes to the config file won't be detected
Consider either logging these errors or handling them appropriately. At minimum, log when ReadConfig fails so operators know the config reload didn't succeed.
| _ = config.ReadConfig(event.Name) | |
| if event.Has(fsnotify.Rename) { | |
| _ = watcher.Add(event.Name) | |
| } | |
| case statusMessagesFileName: | |
| log.Println("modified file:", event.Name) | |
| ei.LoadStatusMessages(event.Name) | |
| if event.Has(fsnotify.Rename) { | |
| _ = watcher.Add(event.Name) | |
| if err := config.ReadConfig(event.Name); err != nil { | |
| log.Printf("failed to reload config from %s: %v", event.Name, err) | |
| } | |
| if event.Has(fsnotify.Rename) { | |
| if err := watcher.Add(event.Name); err != nil { | |
| log.Printf("failed to re-add watch for config file %s: %v", event.Name, err) | |
| } | |
| } | |
| case statusMessagesFileName: | |
| log.Println("modified file:", event.Name) | |
| ei.LoadStatusMessages(event.Name) | |
| if event.Has(fsnotify.Rename) { | |
| if err := watcher.Add(event.Name); err != nil { | |
| log.Printf("failed to re-add watch for status messages file %s: %v", event.Name, err) | |
| } |
| _ = watcher.Add(event.Name) | ||
| } | ||
| case statusMessagesFileName: | ||
| log.Println("modified file:", event.Name) | ||
| ei.LoadStatusMessages(event.Name) | ||
| if event.Has(fsnotify.Rename) { | ||
| _ = watcher.Add(event.Name) |
There was a problem hiding this comment.
When handling fsnotify.Rename events, simply calling watcher.Add(event.Name) may not work as expected. On many systems, a rename event means the original file was moved or deleted, and event.Name refers to the old file path that no longer exists.
In text editors like vim, nano, or when using atomic writes, the typical pattern is:
- Write to a temporary file
- Rename/move the temp file to the target name (this triggers a RENAME event with the old target as
event.Name) - The watcher stops watching because the original inode is gone
To properly handle this, you should re-add the watch using the constant configFileName (not event.Name), which represents the new file path after the rename. The current implementation tries to watch the old file name, which may not exist anymore.
| _ = watcher.Add(event.Name) | |
| } | |
| case statusMessagesFileName: | |
| log.Println("modified file:", event.Name) | |
| ei.LoadStatusMessages(event.Name) | |
| if event.Has(fsnotify.Rename) { | |
| _ = watcher.Add(event.Name) | |
| _ = watcher.Add(configFileName) | |
| } | |
| case statusMessagesFileName: | |
| log.Println("modified file:", event.Name) | |
| ei.LoadStatusMessages(event.Name) | |
| if event.Has(fsnotify.Rename) { | |
| _ = watcher.Add(statusMessagesFileName) |
| _ = watcher.Add(event.Name) | ||
| } | ||
| case statusMessagesFileName: | ||
| log.Println("modified file:", event.Name) | ||
| ei.LoadStatusMessages(event.Name) | ||
| if event.Has(fsnotify.Rename) { | ||
| _ = watcher.Add(event.Name) |
There was a problem hiding this comment.
Similar to the config file handling, errors from watcher.Add are silently ignored after a rename event. If re-adding the watch fails, the status messages file will no longer be monitored for changes, and the application will continue running without detecting future updates to this file.
At minimum, log when watcher.Add fails so operators are aware that file watching has stopped working.
| _ = watcher.Add(event.Name) | |
| } | |
| case statusMessagesFileName: | |
| log.Println("modified file:", event.Name) | |
| ei.LoadStatusMessages(event.Name) | |
| if event.Has(fsnotify.Rename) { | |
| _ = watcher.Add(event.Name) | |
| if err := watcher.Add(event.Name); err != nil { | |
| log.Printf("failed to re-add watch for config file %q after rename: %v", event.Name, err) | |
| } | |
| } | |
| case statusMessagesFileName: | |
| log.Println("modified file:", event.Name) | |
| ei.LoadStatusMessages(event.Name) | |
| if event.Has(fsnotify.Rename) { | |
| if err := watcher.Add(event.Name); err != nil { | |
| log.Printf("failed to re-add watch for status messages file %q after rename: %v", event.Name, err) | |
| } |
| log.Println("modified file:", event.Name) | ||
| ei.LoadStatusMessages(event.Name) | ||
| if event.Has(fsnotify.Rename) { | ||
| _ = watcher.Add(event.Name) |
There was a problem hiding this comment.
The same issue applies here as with the config file: when handling fsnotify.Rename events, event.Name refers to the old file path that may no longer exist after the rename. You should re-add the watch using statusMessagesFileName constant instead of event.Name to ensure the watcher is monitoring the correct file path after the rename operation completes.
| _ = watcher.Add(event.Name) | |
| _ = watcher.Add(statusMessagesFileName) |
No description provided.