Merge pull request #9 from raythurman2386/bugfix/move-race-condition

Fix: Race conditions in move operations and improve installation

Author: raythurman2386

Makefile

@@ -13,6 +13,11 @@ build:
 	$(GO) build -o bin/strawd ./cmd/strawd
 
 install:
+	@if pgrep -x strawd > /dev/null 2>&1; then \
+		echo "Stopping existing strawd instances..."; \
+		pkill -x strawd 2>/dev/null || true; \
+		sleep 1; \
+	fi
 	install -d $(DESTDIR)$(BINDIR)
 	install -m 755 bin/straw $(DESTDIR)$(BINDIR)/
 	install -m 755 bin/strawd $(DESTDIR)$(BINDIR)/

install.sh

@@ -110,6 +110,14 @@ if [ ! -f "straw" ] || [ ! -f "strawd" ]; then
     exit 1
 fi
 
+# Stop any running strawd instances before installing new binary
+echo "Checking for running strawd instances..."
+if pgrep -x strawd > /dev/null 2>&1; then
+    echo "Stopping existing strawd processes..."
+    pkill -x strawd 2>/dev/null || true
+    sleep 1
+fi
+
 # Install binaries
 if [ -w "$INSTALL_DIR" ]; then
     echo "Installing to $INSTALL_DIR..."
@@ -136,6 +144,13 @@ if [ "$OS" = "linux" ] && command -v systemctl >/dev/null 2>&1; then
     SERVICE_DIR="${HOME}/.config/systemd/user"
     mkdir -p "$SERVICE_DIR"
 
+    # Stop existing service before updating binary
+    if systemctl --user is-active --quiet strawd.service 2>/dev/null; then
+        echo "Stopping existing strawd service..."
+        systemctl --user stop strawd.service
+        sleep 1
+    fi
+    
     cat > "$SERVICE_DIR/strawd.service" << EOF
 [Unit]
 Description=Straw File Automation Daemon
@@ -153,9 +168,15 @@ EOF
 
     systemctl --user daemon-reload
     systemctl --user enable strawd.service
-    systemctl --user start strawd.service
 
-    echo "Started strawd service"
+    # Start or restart the service
+    if systemctl --user is-active --quiet strawd.service 2>/dev/null; then
+        systemctl --user restart strawd.service
+        echo "Restarted strawd service"
+    else
+        systemctl --user start strawd.service
+        echo "Started strawd service"
+    fi
 fi
 
 echo ""

internal/actions/actions.go

@@ -43,27 +43,65 @@ func (e *Executor) move(src, destDir string) error {
 		return err
 	}
 
+	srcInfo, err := os.Stat(src)
+	if err != nil {
+		if os.IsNotExist(err) {
+			// Source is already gone — a prior event already moved it.
+			slog.Debug("Source file already removed, skipping move", "src", src)
+			return nil
+		}
+		return err
+	}
+
 	destPath := e.getDestPath(src, destDir)
 
-	// Ensure we aren't moving a file into itself or sensitive areas if we can help it
 	if destPath == src {
 		return fmt.Errorf("source and destination are the same: %s", src)
 	}
 
-	// Attempt standard rename
-	err := os.Rename(src, destPath)
-	if err == nil {
+	// Directories can only be renamed, not copied as a fallback.
+	if srcInfo.IsDir() {
+		return e.renameOrSkip(src, destPath)
+	}
+
+	// Attempt standard rename for files.
+	renameErr := e.renameOrSkip(src, destPath)
+	if renameErr == nil {
 		return nil
 	}
 
-	// If rename fails (e.g. cross-device), fallback to copy + delete
-	slog.Debug("Standard rename failed, attempting copy+delete fallback", "src", src, "dest", destPath, "error", err)
+	// If rename fails (e.g. cross-device), fallback to copy + delete.
+	slog.Debug("Standard rename failed, attempting copy+delete fallback", "src", src, "dest", destPath, "renameError", renameErr)
 
 	if err := e.copyToPath(src, destPath); err != nil {
+		slog.Debug("copyToPath failed", "src", src, "dest", destPath, "error", err, "isNotExist", os.IsNotExist(err))
+		if os.IsNotExist(err) {
+			slog.Debug("Source file disappeared during fallback copy", "src", src)
+			return nil
+		}
 		return fmt.Errorf("fallback copy failed: %w", err)
 	}
 
-	return os.Remove(src)
+	// Clean up the source after a successful copy.
+	if err := os.Remove(src); err != nil && !os.IsNotExist(err) {
+		return fmt.Errorf("failed to remove source after copy: %w", err)
+	}
+
+	return nil
+}
+
+// renameOrSkip attempts os.Rename. If the source has already been removed
+// (e.g. by a concurrent event), it returns nil instead of an error.
+func (e *Executor) renameOrSkip(src, dest string) error {
+	err := os.Rename(src, dest)
+	if err == nil {
+		return nil
+	}
+	if os.IsNotExist(err) {
+		slog.Debug("Source file disappeared before rename completed", "src", src, "dest", dest)
+		return nil
+	}
+	return err
 }
 
 func (e *Executor) copy(src, destDir string) error {

internal/actions/actions_test.go

@@ -90,6 +90,63 @@ func TestExecutor_MoveCrossDevice(t *testing.T) {
 	}
 }
 
+// TestExecutor_MoveSourceAlreadyMoved tests that calling move on a file
+// that was already moved to the destination (e.g. by a duplicate event)
+// succeeds without returning an error.
+func TestExecutor_MoveSourceAlreadyMoved(t *testing.T) {
+	e := NewExecutor()
+	tmpDir, err := os.MkdirTemp("", "move_already_test")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(tmpDir)
+
+	src := filepath.Join(tmpDir, "src.txt")
+	if err := os.WriteFile(src, []byte("hello"), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	destDir := filepath.Join(tmpDir, "out")
+
+	// First move should succeed normally
+	err = e.move(src, destDir)
+	if err != nil {
+		t.Fatalf("first move failed: %v", err)
+	}
+
+	if _, err := os.Stat(filepath.Join(destDir, "src.txt")); os.IsNotExist(err) {
+		t.Fatal("file should exist in destination after first move")
+	}
+
+	// Second move on the same (now missing) source path should not error,
+	// because the file is already at the destination.
+	err = e.move(src, destDir)
+	if err != nil {
+		t.Errorf("second move should succeed (file already at destination), got: %v", err)
+	}
+}
+
+// TestExecutor_MoveSourceGone tests that calling move on a source file
+// that no longer exists is treated as a no-op success, since a prior
+// event already handled it.
+func TestExecutor_MoveSourceGone(t *testing.T) {
+	e := NewExecutor()
+	tmpDir, err := os.MkdirTemp("", "move_gone_test")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(tmpDir)
+
+	src := filepath.Join(tmpDir, "ghost.txt")
+	destDir := filepath.Join(tmpDir, "out")
+
+	// Source never existed — move should treat this as a no-op (already handled)
+	err = e.move(src, destDir)
+	if err != nil {
+		t.Errorf("move should succeed when source does not exist (already handled), got: %v", err)
+	}
+}
+
 func TestExecutor_Shell(t *testing.T) {
 	e := NewExecutor()
 	tmpDir, err := os.MkdirTemp("", "shell_test")

internal/watcher/watcher.go

@@ -5,10 +5,13 @@ import (
 	"path/filepath"
 	"strings"
 	"sync"
+	"time"
 
 	"github.com/fsnotify/fsnotify"
 )
 
+const debounceInterval = 100 * time.Millisecond
+
 type EventType int
 
 const (
@@ -89,18 +92,32 @@ func (w *Watcher) Start() {
 }
 
 func (w *Watcher) loop() {
+	// pending tracks debounced events keyed by file path.
+	// When multiple fsnotify events arrive for the same path within
+	// debounceInterval, only the last event is forwarded.
+	pending := make(map[string]Event)
+	timers := make(map[string]*time.Timer)
+
+	// flush receives paths whose debounce timer has expired,
+	// keeping all map access on the loop goroutine.
+	flush := make(chan string, 100)
+
 	for {
 		select {
 		case event, ok := <-w.fsWatcher.Events:
 			if !ok {
+				// Flush remaining pending events before exiting
+				for path, ev := range pending {
+					w.events <- ev
+					delete(pending, path)
+				}
 				return
 			}
 
-			// Handle new directories
+			// Handle new directories immediately (not debounced)
 			if event.Op&fsnotify.Create == fsnotify.Create {
 				info, err := os.Stat(event.Name)
 				if err == nil && info.IsDir() {
-					// Check if inside a recursive root
 					if w.isRecursive(event.Name) {
 						if err := w.Add(event.Name, true); err != nil {
 							w.errors <- err
@@ -109,16 +126,39 @@ func (w *Watcher) loop() {
 				}
 			}
 
-			// Forward event
-			// We might want to debounce or filter here, but for now just raw events.
-			w.events <- translateEvent(event)
+			// Debounce: coalesce multiple events for the same path.
+			// Each new event resets the timer; only when the timer
+			// expires (no new events for debounceInterval) do we
+			// forward the final event to consumers.
+			translated := translateEvent(event)
+			pending[event.Name] = translated
+
+			if t, exists := timers[event.Name]; exists {
+				t.Reset(debounceInterval)
+			} else {
+				path := event.Name
+				timers[path] = time.AfterFunc(debounceInterval, func() {
+					flush <- path
+				})
+			}
+
+		case path := <-flush:
+			if ev, ok := pending[path]; ok {
+				w.events <- ev
+				delete(pending, path)
+				delete(timers, path)
+			}
 
 		case err, ok := <-w.fsWatcher.Errors:
 			if !ok {
 				return
 			}
 			w.errors <- err
 		case <-w.done:
+			// Stop all pending timers on shutdown
+			for _, t := range timers {
+				t.Stop()
+			}
 			return
 		}
 	}

scripts/install_service.sh

@@ -50,9 +50,25 @@ WantedBy=default.target
 EOF
 
 echo "Installing service..."
+
+# Stop existing service before updating
+if systemctl --user is-active --quiet "$SERVICE_NAME" 2>/dev/null; then
+    echo "Stopping existing strawd service..."
+    systemctl --user stop "$SERVICE_NAME"
+    sleep 1
+fi
+
 systemctl --user daemon-reload
 systemctl --user enable "$SERVICE_NAME"
-systemctl --user restart "$SERVICE_NAME"
+
+# Start or restart the service
+if systemctl --user is-active --quiet "$SERVICE_NAME" 2>/dev/null; then
+    systemctl --user restart "$SERVICE_NAME"
+    echo "Restarted strawd service"
+else
+    systemctl --user start "$SERVICE_NAME"
+    echo "Started strawd service"
+fi
 
 echo "---------------------------------------------------"
 echo "✅ Straw daemon is now running as a user service!"