Avoid duplicate daemon start on handoff timeout

Author: wesm

cmd/roborev/main.go

@@ -2880,6 +2880,13 @@ func restartDaemonAfterUpdate(binDir string, noRestart bool) {
 				fmt.Println("OK")
 				return
 			}
+			// A replacement PID already exists; avoid manually starting
+			// another daemon instance while handoff is still warming up.
+			fmt.Println(
+				"warning: daemon handoff detected but replacement is not ready;" +
+					" restart it manually",
+			)
+			return
 		}
 		// Treat as unresolved and continue to kill fallback.
 		exited = false
@@ -2906,15 +2913,13 @@ func restartDaemonAfterUpdate(binDir string, noRestart bool) {
 				fmt.Println("OK")
 				return
 			}
-			// In uncertain stop-failure paths, avoid starting another daemon
-			// when a handoff PID already exists but is not ready yet.
-			if stopFailed {
-				fmt.Println(
-					"warning: daemon handoff detected but replacement is not ready;" +
-						" restart it manually",
-				)
-				return
-			}
+			// A replacement PID already exists; avoid manually starting
+			// another daemon instance while handoff is still warming up.
+			fmt.Println(
+				"warning: daemon handoff detected but replacement is not ready;" +
+					" restart it manually",
+			)
+			return
 		}
 		if !exitedAfterKill {
 			fmt.Printf(

cmd/roborev/main_test.go

@@ -1345,17 +1345,69 @@ func TestRestartDaemonAfterUpdateManagerHandoffUnresponsiveUsesRuntimePID(t *tes
 		restartDaemonAfterUpdate("/tmp/bin", false)
 	})
 
-	if s.killCalls != 1 {
-		t.Fatalf("expected kill fallback called once for unresolved handoff, got %d", s.killCalls)
+	if s.killCalls != 0 {
+		t.Fatalf("expected kill fallback not called when handoff PID already exists, got %d", s.killCalls)
 	}
-	if s.startCalls != 1 {
-		t.Fatalf("expected manual start after unresolved handoff, got %d", s.startCalls)
+	if s.startCalls != 0 {
+		t.Fatalf("expected startUpdatedDaemon not called for unready handoff, got %d", s.startCalls)
 	}
 	if strings.Contains(output, "Restarting daemon... OK") {
 		t.Fatalf("unexpected handoff success output: %q", output)
 	}
-	if !strings.Contains(output, "warning: daemon did not become ready after restart; restart it manually") {
-		t.Fatalf("expected not-ready warning after manual start, got %q", output)
+	if !strings.Contains(output, "warning: daemon handoff detected but replacement is not ready; restart it manually") {
+		t.Fatalf("expected not-ready handoff warning, got %q", output)
+	}
+}
+
+func TestRestartDaemonAfterUpdateManagerHandoffAfterKillNotReadyWarnsNoStart(t *testing.T) {
+	s := stubRestartVars(t)
+
+	var handoffSeen bool
+	getAnyRunningDaemon = func() (*daemon.RuntimeInfo, error) {
+		if s.killCalls == 0 {
+			// Initial probe + first wait loop see only the old daemon,
+			// forcing timeout and kill fallback.
+			return &daemon.RuntimeInfo{PID: 100, Addr: "127.0.0.1:7373"}, nil
+		}
+		if !handoffSeen {
+			// After kill fallback, handoff PID appears once.
+			handoffSeen = true
+			return &daemon.RuntimeInfo{PID: 500, Addr: "127.0.0.1:7373"}, nil
+		}
+		// Replacement remains unresponsive during readiness polling.
+		return nil, os.ErrNotExist
+	}
+
+	listAllRuntimes = func() ([]*daemon.RuntimeInfo, error) {
+		if s.killCalls == 0 {
+			return []*daemon.RuntimeInfo{
+				{PID: 100, Addr: "127.0.0.1:7373"},
+			}, nil
+		}
+		return []*daemon.RuntimeInfo{
+			{PID: 500, Addr: "127.0.0.1:7373"},
+		}, nil
+	}
+
+	isPIDAliveForUpdate = func(pid int) bool {
+		return pid == 500
+	}
+
+	output := captureStdout(t, func() {
+		restartDaemonAfterUpdate("/tmp/bin", false)
+	})
+
+	if s.killCalls != 1 {
+		t.Fatalf("expected kill fallback called once, got %d", s.killCalls)
+	}
+	if s.startCalls != 0 {
+		t.Fatalf("expected startUpdatedDaemon not called for unready handoff, got %d", s.startCalls)
+	}
+	if !strings.Contains(output, "warning: daemon handoff detected but replacement is not ready; restart it manually") {
+		t.Fatalf("expected not-ready handoff warning, got %q", output)
+	}
+	if strings.Contains(output, "Restarting daemon... OK") {
+		t.Fatalf("unexpected success output: %q", output)
 	}
 }
 

cmd/roborev/processcheck_common.go

@@ -14,6 +14,14 @@ const (
 	updatePIDNotRoborev
 )
 
+var nonRunDaemonSubcommandsForUpdate = map[string]struct{}{
+	"status":  {},
+	"start":   {},
+	"stop":    {},
+	"restart": {},
+	"logs":    {},
+}
+
 func normalizeCommandLineForUpdate(s string) string {
 	// Strip NUL bytes first (common with UTF-16LE output and /proc cmdline).
 	s = strings.ReplaceAll(s, "\x00", " ")
@@ -117,6 +125,10 @@ func isRoborevDaemonCommandForUpdate(cmdStr string) bool {
 	fields := strings.Fields(cmdLower)
 	foundDaemon := false
 	for _, field := range fields {
+		field = trimCommandTokenQuotesForUpdate(field)
+		if field == "" {
+			continue
+		}
 		if !foundDaemon {
 			if field == "daemon" ||
 				strings.HasSuffix(field, "/daemon") ||
@@ -125,32 +137,16 @@ func isRoborevDaemonCommandForUpdate(cmdStr string) bool {
 			}
 			continue
 		}
-		if strings.HasPrefix(field, "-") {
-			continue
+		if field == "run" {
+			return true
 		}
-		if looksLikeFlagValueForUpdate(field) {
-			continue
+		if _, isNonRunSubcommand := nonRunDaemonSubcommandsForUpdate[field]; isNonRunSubcommand {
+			return false
 		}
-		return field == "run"
 	}
 	return false
 }
 
-func looksLikeFlagValueForUpdate(token string) bool {
-	if strings.ContainsAny(token, "/\\") {
-		return true
-	}
-	if strings.Contains(token, ":") {
-		return true
-	}
-	if strings.Contains(token, "=") {
-		return true
-	}
-	if len(token) > 0 && token[0] >= '0' && token[0] <= '9' {
-		return true
-	}
-	if strings.Contains(token, ".") {
-		return true
-	}
-	return false
+func trimCommandTokenQuotesForUpdate(token string) string {
+	return strings.Trim(token, `"'`)
 }

cmd/roborev/processcheck_common_test.go

@@ -22,6 +22,16 @@ func TestIsRoborevDaemonCommandForUpdate(t *testing.T) {
 			cmdLine: `C:\Tools\roborev.exe daemon run --port 7373`,
 			want:    true,
 		},
+		{
+			name:    "quoted windows args daemon run",
+			cmdLine: `"C:\Tools\roborev.exe" "daemon" "run" "--port" "7373"`,
+			want:    true,
+		},
+		{
+			name:    "plain flag value before run",
+			cmdLine: "roborev daemon --profile prod run",
+			want:    true,
+		},
 		{
 			name:    "daemon status command",
 			cmdLine: "/usr/local/bin/roborev daemon status",