From 66bf6a88eb7730a8c85f0267de4bb4c8d6b8badf Mon Sep 17 00:00:00 2001 From: Muse Date: Sat, 24 Oct 2026 09:38:09 +0000 Subject: [PATCH] fix(agent): reconcile orphaned sing-box core before start After an agent restart the tracked sing-box handle is lost while the old core survives reparented holding the inbound ports, so every apply died on bind, rollback died the same way, and the agent retried forever. startSingboxLocked now scans /proc for processes whose argv carries run -c (excluding the tracked pid) and stops them SIGTERM-then-SIGKILL before starting, covering the apply, rollback, and unchanged-but-not-running paths. The exact-path match never touches cores on other config files; Linux only. --- internal/agent/singbox.go | 106 +++++++++++++ internal/agent/singbox_orphan_test.go | 213 ++++++++++++++++++++++++++ 2 files changed, 319 insertions(+) create mode 100644 internal/agent/singbox_orphan_test.go diff --git a/internal/agent/singbox.go b/internal/agent/singbox.go index 4bc9052..261ffc1 100644 --- a/internal/agent/singbox.go +++ b/internal/agent/singbox.go @@ -10,6 +10,7 @@ import ( "os/exec" "path/filepath" "runtime" + "strconv" "strings" "sync" "syscall" @@ -442,9 +443,114 @@ func (a *Agent) applySingboxConfigLocked(raw json.RawMessage) error { return nil } +// reconcileSingboxOrphans stops untracked sing-box processes running +// this exact live config file: an agent restart loses the handle +// while the old core survives reparented holding the inbound ports, +// which otherwise makes every apply/rollback die on bind forever. +// The exact-path match is the safety boundary — a core on any other +// config file belongs to another panel/agent/router and is never +// touched. Linux only; the Windows agent line is frozen, so the scan +// is a no-op there. Returns how many orphans were reaped. +func reconcileSingboxOrphans(configPath string, ownPid int) int { + if runtime.GOOS != "linux" { + return 0 + } + self := os.Getpid() + reaped := 0 + entries, err := os.ReadDir("/proc") + if err != nil { + return 0 + } + for _, e := range entries { + pid, err := strconv.Atoi(e.Name()) + if err != nil || pid <= 0 || pid == self || pid == ownPid { + continue + } + if !singboxCmdlineMatchesConfig(pid, configPath) { + continue + } + logger.Infof("sing-box orphan on %s (pid %d) from a lost handle; stopping it before start", configPath, pid) + stopForeignProcess(pid) + if foreignPidAlive(pid) { + logger.Warningf("sing-box orphan pid %d survived SIGKILL; the new core may fail to bind", pid) + continue + } + reaped++ + } + return reaped +} + +// singboxCmdlineMatchesConfig reports whether pid's argv contains +// the consecutive triple `run -c `, the exact invocation +// the agent itself uses. A substring match would be unsafe: another +// config path merely sharing a prefix must never qualify. +func singboxCmdlineMatchesConfig(pid int, configPath string) bool { + data, err := os.ReadFile(fmt.Sprintf("/proc/%d/cmdline", pid)) + if err != nil { + return false + } + args := strings.Split(string(data), "\x00") + for len(args) > 0 && args[len(args)-1] == "" { + args = args[:len(args)-1] + } + for i := 0; i+2 < len(args); i++ { + if args[i] == "run" && args[i+1] == "-c" && args[i+2] == configPath { + return true + } + } + return false +} + +// foreignPidAlive treats a zombie as dead: a reparented orphan's +// parent reaps it eventually, but its ports are already released. +func foreignPidAlive(pid int) bool { + if st, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pid)); err == nil { + if i := strings.LastIndexByte(string(st), ')'); i >= 0 && i+2 < len(st) && st[i+2] == 'Z' { + return false + } + } else { + return false + } + err := syscall.Kill(pid, 0) + return err == nil || err == syscall.EPERM +} + +// stopForeignProcess mirrors singboxProcess.stop: SIGTERM, then +// SIGKILL after the same grace, then a bounded wait for the kill. +func stopForeignProcess(pid int) { + proc, _ := os.FindProcess(pid) + if proc == nil { + return + } + _ = proc.Signal(syscall.SIGTERM) + if waitForeignPidExit(pid, singboxStopGrace) { + return + } + _ = proc.Kill() + waitForeignPidExit(pid, singboxStopForce) +} + +func waitForeignPidExit(pid int, timeout time.Duration) bool { + deadline := time.Now().Add(timeout) + for { + if !foreignPidAlive(pid) { + return true + } + if time.Now().After(deadline) { + return false + } + time.Sleep(25 * time.Millisecond) + } +} + // startSingboxLocked starts the process and verifies it survives the // settle window. func (a *Agent) startSingboxLocked(binPath, configPath string) error { + ownPid := 0 + if a.sbProc != nil { + ownPid = a.sbProc.pid() + } + reconcileSingboxOrphans(configPath, ownPid) if err := a.sbProc.start(binPath, configPath, singboxLogPath()); err != nil { return err } diff --git a/internal/agent/singbox_orphan_test.go b/internal/agent/singbox_orphan_test.go new file mode 100644 index 0000000..92e3e2a --- /dev/null +++ b/internal/agent/singbox_orphan_test.go @@ -0,0 +1,213 @@ +package agent + +import ( + "os" + "os/exec" + "runtime" + "syscall" + "testing" + "time" +) + +// Orphan-core recovery (next-version fix batch item 1): after an +// agent restart the tracked sing-box handle is lost while the old +// core survives reparented holding the inbound ports. The reconcile +// in startSingboxLocked must stop exactly those orphans — processes +// whose argv carries `run -c ` — before starting +// a new core, on the apply path, the rollback path, and the +// unchanged-config-but-not-running path alike. +// +// The dummy below is the test binary re-exec'd: with the marker env +// it sleeps until signalled, and its argv carries a crafted `run -c` +// triple, which is exactly what the reconcile matches on. Without +// the marker it is a no-op so ordinary test runs pass through it. +const orphanDummyEnv = "XUI_ORPHAN_DUMMY" + +func TestOrphanDummyProcess(t *testing.T) { + if os.Getenv(orphanDummyEnv) != "1" { + return + } + time.Sleep(3600 * time.Second) +} + +func spawnOrphanDummy(t *testing.T, cfgPath string) (*exec.Cmd, int) { + t.Helper() + if runtime.GOOS != "linux" { + t.Skip("orphan reconcile is Linux-only") + } + cmd := exec.Command(os.Args[0], "-test.run=^TestOrphanDummyProcess$", "--", "run", "-c", cfgPath) + cmd.Env = append(os.Environ(), orphanDummyEnv+"=1") + if err := cmd.Start(); err != nil { + t.Fatalf("start dummy: %v", err) + } + pid := cmd.Process.Pid + t.Cleanup(func() { + if cmd.ProcessState == nil || !cmd.ProcessState.Exited() { + _ = cmd.Process.Kill() + } + _ = cmd.Wait() + }) + deadline := time.Now().Add(10 * time.Second) + for time.Now().Before(deadline) { + if foreignPidAlive(pid) && singboxCmdlineMatchesConfig(pid, cfgPath) { + return cmd, pid + } + time.Sleep(10 * time.Millisecond) + } + t.Fatalf("dummy pid %d never showed the crafted argv", pid) + return nil, 0 +} + +func waitDummyExit(t *testing.T, cmd *exec.Cmd) bool { + t.Helper() + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + select { + case <-done: + return true + case <-time.After(10 * time.Second): + return false + } +} + +func TestReconcileSingboxOrphansStopsMatchingProcess(t *testing.T) { + cfgPath := "/tmp/orphan-test-live-config.json" + cmd, pid := spawnOrphanDummy(t, cfgPath) + if n := reconcileSingboxOrphans(cfgPath, 0); n != 1 { + t.Fatalf("reaped = %d, want 1", n) + } + if !waitDummyExit(t, cmd) { + t.Fatalf("dummy pid %d still alive after reconcile", pid) + } + if foreignPidAlive(pid) { + t.Fatalf("dummy pid %d still reported alive after reconcile", pid) + } +} + +func TestReconcileSingboxOrphansLeavesOtherConfigAlone(t *testing.T) { + _, pid := spawnOrphanDummy(t, "/tmp/orphan-test-other-config.json") + if n := reconcileSingboxOrphans("/tmp/orphan-test-live-config.json", 0); n != 0 { + t.Fatalf("reaped = %d, want 0", n) + } + if !foreignPidAlive(pid) { + t.Fatalf("dummy on a different config path was killed, want untouched") + } +} + +func TestReconcileSingboxOrphansSkipsTrackedPid(t *testing.T) { + cmd, pid := spawnOrphanDummy(t, "/tmp/orphan-test-live-config.json") + if n := reconcileSingboxOrphans("/tmp/orphan-test-live-config.json", pid); n != 0 { + t.Fatalf("reaped = %d, want 0 for the tracked pid", n) + } + if !foreignPidAlive(pid) { + t.Fatalf("tracked pid %d was killed, want untouched", pid) + } + _ = cmd.Process.Kill() +} + +// dropSingboxHandle simulates the agent-restart half of the incident: +// the tracked handle is gone, but the previously started core keeps +// running. The caller owns killing the leaked fake core afterwards. +func dropSingboxHandle(t *testing.T, a *Agent) int { + t.Helper() + a.sbMu.Lock() + pid := a.sbProc.pid() + a.sbProc = &singboxProcess{} + a.sbMu.Unlock() + if pid <= 0 { + t.Fatal("no tracked core to orphan") + } + t.Cleanup(func() { _ = syscall.Kill(pid, syscall.SIGTERM) }) + return pid +} + +func TestApplySingboxRecoversFromStaleHandleAndOrphan(t *testing.T) { + setupFakeSingbox(t) + a := newApplyTestAgent() + t.Cleanup(a.stopSingbox) + + if err := a.applySingboxConfig([]byte(sbCfgA)); err != nil { + t.Fatalf("apply A: %v", err) + } + pidBefore := dropSingboxHandle(t, a) + cmd, orphanPid := spawnOrphanDummy(t, singboxConfigPath()) + + if err := a.applySingboxConfig([]byte(sbCfgB)); err != nil { + t.Fatalf("apply B with stale handle + orphan: %v", err) + } + if !waitDummyExit(t, cmd) { + t.Fatalf("orphan pid %d survived the apply", orphanPid) + } + a.sbMu.Lock() + running := a.sbProc.isRunning() + pidAfter := a.sbProc.pid() + a.sbMu.Unlock() + if !running || pidAfter <= 0 || pidAfter == pidBefore { + t.Fatalf("no new tracked core after recovery: running=%v pid %d -> %d", running, pidBefore, pidAfter) + } + if got := appliedConfig(t); got != sbCfgB { + t.Fatalf("applied config = %q, want cfgB", got) + } +} + +func TestApplySingboxUnchangedRecoversFromStaleHandleAndOrphan(t *testing.T) { + setupFakeSingbox(t) + a := newApplyTestAgent() + t.Cleanup(a.stopSingbox) + + if err := a.applySingboxConfig([]byte(sbCfgA)); err != nil { + t.Fatalf("apply A: %v", err) + } + dropSingboxHandle(t, a) + cmd, orphanPid := spawnOrphanDummy(t, singboxConfigPath()) + + // Unchanged config + tracked handle says not-running: this used + // to fall into the start-fail/rollback-fail loop in production + // (the orphan held the ports); the reconcile inside the start + // must clear it before the new core binds. + if err := a.applySingboxConfig([]byte(sbCfgA)); err != nil { + t.Fatalf("re-apply A with stale handle + orphan: %v", err) + } + if !waitDummyExit(t, cmd) { + t.Fatalf("orphan pid %d survived the unchanged re-apply", orphanPid) + } + a.sbMu.Lock() + running := a.sbProc.isRunning() + a.sbMu.Unlock() + if !running { + t.Fatal("no tracked core running after unchanged re-apply recovery") + } +} + +func TestApplySingboxRollbackRecoversFromStaleHandleAndOrphan(t *testing.T) { + setupFakeSingbox(t) + a := newApplyTestAgent() + t.Cleanup(a.stopSingbox) + + if err := a.applySingboxConfig([]byte(sbCfgA)); err != nil { + t.Fatalf("apply A: %v", err) + } + dropSingboxHandle(t, a) + cmd, orphanPid := spawnOrphanDummy(t, singboxConfigPath()) + + // A config that dies at startup: the new core fails, so the + // apply rolls back — and the rollback's start must clear the + // orphan too, or it dies on bind the same way and the agent + // loops. The apply itself still reports failure either way. + bad := `{"log":{"level":"info"},"note":"RUNTIMEFAIL","inbounds":[]}` + if err := a.applySingboxConfig([]byte(bad)); err == nil { + t.Fatal("apply of a config that dies at startup must fail") + } + if !waitDummyExit(t, cmd) { + t.Fatalf("orphan pid %d survived the apply/rollback sequence", orphanPid) + } + a.sbMu.Lock() + running := a.sbProc.isRunning() + a.sbMu.Unlock() + if !running { + t.Fatal("rollback must leave a running core") + } + if got := appliedConfig(t); got != sbCfgA { + t.Fatalf("active config after rollback = %q, want cfgA", got) + } +}