fix: replace telegram PID-file singleton lock with flock

The PID-file lock probed liveness with signals and could kill arbitrary
processes on non-Linux systems if an attacker planted a victim PID in
~/.odek/telegram.pid. Replace it with an advisory flock on
~/.odek/telegram.lock via the existing internal/flock module.

- Update cmd/odek/telegram.go acquireLock/release to use flock.Lock.
- Remove PID read/kill/write logic and the instanceLock struct.
- Clean up legacy telegram.pid file on successful lock acquisition.
- Add regression tests for lock-file creation, legacy PID cleanup, and
  no-kill behaviour.
- Update docs/TELEGRAM.md, docs/SECURITY.md, and AGENTS.md.

Author: jkyberneees

AGENTS.md

@@ -135,6 +135,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
 - **Project config sensitive-field rejection** (`internal/config/loader.go`) — `./odek.json` is untrusted, so `base_url`, `api_key`, `system`, and the `dangerous` section set there are ignored (with stderr warnings). These can only be configured from operator-controlled sources: `~/.odek/config.json`, `ODEK_*` env vars, or CLI flags.
 - **MCP subprocess environment sanitisation** (`internal/mcpclient/client.go`) — MCP server children receive only a minimal allowlist of safe environment variables plus explicit `env` overrides. Keys matching secret patterns (`*_API_KEY`, `*_TOKEN`, `*_SECRET`, `*_PASSWORD`, etc.) are stripped, preventing a compromised or malicious MCP server from reading parent secrets.
 - **Schedule atomic-write hardening** (`internal/schedule/store.go` + `internal/fsatomic`) — schedule file writes now use `fsatomic.WriteFile`, which creates a random temp file with `O_EXCL`, fsyncs data and directory, and renames over the target. A swapped-in symlink is replaced rather than followed, closing the symlink-override attack on `schedules.json` / `schedule-state.json`.
+- **Telegram singleton flock lock** (`cmd/odek/telegram.go` + `internal/flock`) — the Telegram bot now uses an advisory `flock` on `~/.odek/telegram.lock` instead of a PID file probed with signals. This removes the non-Linux path where a planted PID could cause odek to kill an arbitrary process.
 - **Telegram photo caption wrapping** (`cmd/odek/telegram.go`) — photo captions cross the Telegram trust boundary, so they are wrapped as untrusted content both when passed to the local vision model and when injected into the main agent's user message.
 - **Secret redaction** (`internal/redact/redact.go`) — 20+ patterns: OpenAI, Anthropic, GitHub PAT, AWS, PEM, JWT, Vault, Google OAuth, SendGrid, Discord, DB URLs, etc.
 

cmd/odek/telegram.go

@@ -19,6 +19,7 @@ import (
 
 	"github.com/BackendStack21/odek"
 	"github.com/BackendStack21/odek/internal/config"
+	"github.com/BackendStack21/odek/internal/flock"
 	"github.com/BackendStack21/odek/internal/llm"
 	"github.com/BackendStack21/odek/internal/loop"
 	"github.com/BackendStack21/odek/internal/memory"
@@ -111,9 +112,9 @@ func handleRestartCommand(chatID, userID int64, adminChats, adminUsers []int64)
 	return "🔄 *Restarting...*\n\nThe bot will restart momentarily. This may take a few seconds.", true
 }
 
-// instanceLockRef holds the current PID file lock, accessible from
-// gracefulRestart so it can release the lock before os.Exit(0).
-var instanceLockRef *instanceLock
+// instanceLockRef holds the current Telegram singleton lock release function,
+// accessible from gracefulRestart so it can release the lock before os.Exit(0).
+var instanceLockRef func()
 
 // getChatMutex returns the per-chat mutex for the given chat ID.
 func getChatMutex(chatID int64) *sync.Mutex {
@@ -142,15 +143,15 @@ func resetChatForNew(chatID int64, sessionManager *telegram.SessionManager, hand
 
 // telegramCmd is the entry point for "odek telegram".
 func telegramCmd(args []string) error {
-	// 0. Acquire singleton lock — kill any stale previous instance.
-	lock, err := acquireLock()
+	// 0. Acquire singleton lock — wait for any previous instance to exit.
+	lockRelease, err := acquireLock()
 	if err != nil {
 		return fmt.Errorf("telegram: %w", err)
 	}
-	instanceLockRef = lock
+	instanceLockRef = lockRelease
 	defer func() {
 		instanceLockRef = nil
-		lock.release()
+		lockRelease()
 	}()
 
 	// 1. Load config from all sources (file → env).
@@ -1010,9 +1011,9 @@ func gracefulRestart(bot *telegram.Bot) {
 	//
 	// Since the child is an independent process already running via
 	// os.StartProcess, the cleanest path is to exit right here.
-	// Release the PID file lock before exit so the child gets a clean slate.
+	// Release the singleton lock before exit so the child gets a clean slate.
 	if instanceLockRef != nil {
-		instanceLockRef.release()
+		instanceLockRef()
 	}
 	// Close the embedded scheduler's MCP connections before exiting — os.Exit
 	// skips deferred cleanup, so without this the MCP child processes (e.g.
@@ -1983,89 +1984,53 @@ func truncateToolArgs(data string, maxLen int) string {
 // ── Singleton Lock ─────────────────────────────────────────────────────
 //
 // Prevents two bot instances from polling Telegram simultaneously (which
-// causes 409 Conflict errors). Uses a PID file at ~/.odek/telegram.pid.
+// causes 409 Conflict errors). Uses an advisory file lock on
+// ~/.odek/telegram.lock via the internal/flock module.
+//
+// Why not a PID file? A PID file is probed with signals and, on macOS and
+// other non-Linux POSIX systems, can easily be made to kill an unrelated
+// process whose PID was planted by an attacker. flock is advisory, portable,
+// and the OS automatically releases the lock when the holding process exits.
 //
 // LIFECYCLE
 //
 // Normal startup (no previous instance):
 //
-//	acquireLock() → PID file doesn't exist → writes own PID → OK
+//	acquireLock() → flock succeeds → OK
 //
 // Competing startup (existing instance still alive):
 //
-//	acquireLock() → reads PID file → kills old process (SIGTERM→5s→SIGKILL)
-//	               → old process dies → writes own PID → OK
+//	acquireLock() → blocks on flock until the old process exits → OK
 //
 // Restart (child starts after parent's os.Exit(0)):
 //
-//	acquireLock() → reads PID file → finds old (dead) PID
-//	               → syscall.Kill(pid, 0) fails (process gone)
-//	               → writes own PID → OK
-//
-// Note: During restart, the parent's deferred lock.release() never runs
-// (os.Exit(0) skips defers). The stale PID file is harmless — the child's
-// acquireLock simply finds a dead PID and overwrites it.
-
-type instanceLock struct {
-	pidFile string
-}
+//	acquireLock() → old process released the lock via os.Exit → OK
 
-// acquireLock reads any existing PID file, kills the old process if still
-// alive, then writes the current PID. Returns the lock for deferred release.
-func acquireLock() (*instanceLock, error) {
+// acquireLock acquires an exclusive advisory lock on ~/.odek/telegram.lock
+// using the internal/flock module. The returned release function must be
+// called on shutdown. Any legacy telegram.pid file is removed on success.
+func acquireLock() (func(), error) {
 	home, err := os.UserHomeDir()
 	if err != nil {
 		return nil, fmt.Errorf("home dir: %w", err)
 	}
-	pidFile := filepath.Join(home, ".odek", "telegram.pid")
+	lockFile := filepath.Join(home, ".odek", "telegram.lock")
 
 	// Ensure parent dir exists.
-	if err := os.MkdirAll(filepath.Dir(pidFile), 0755); err != nil {
-		return nil, fmt.Errorf("mkdir pid: %w", err)
-	}
-
-	// Read stale PID and kill it if still alive.
-	if data, err := os.ReadFile(pidFile); err == nil {
-		oldPID := strings.TrimSpace(string(data))
-		if pid, _ := strconv.Atoi(oldPID); pid > 1 {
-			// Primary liveness check: cross-platform (signal 0 = probe only).
-			if err := syscall.Kill(pid, 0); err == nil {
-				// Process is alive. On Linux, verify it's an odek telegram
-				// process before killing — skip identity check elsewhere since
-				// /proc is Linux-only and the PID file is odek-specific anyway.
-				shouldKill := true
-				if cmdline, err := os.ReadFile(filepath.Join("/proc", oldPID, "cmdline")); err == nil {
-					shouldKill = strings.Contains(string(cmdline), "odek") &&
-						strings.Contains(string(cmdline), "telegram")
-				}
-				if shouldKill {
-					fmt.Fprintf(os.Stderr, "odek telegram: killing stale instance (PID %d)\n", pid)
-					syscall.Kill(pid, syscall.SIGTERM)
-					// Wait up to 5s for graceful shutdown.
-					for i := 0; i < 50; i++ {
-						time.Sleep(100 * time.Millisecond)
-						if err := syscall.Kill(pid, 0); err != nil {
-							break // process gone
-						}
-					}
-					// Force kill if still alive.
-					syscall.Kill(pid, syscall.SIGKILL)
-				}
-			}
-		}
+	if err := os.MkdirAll(filepath.Dir(lockFile), 0755); err != nil {
+		return nil, fmt.Errorf("mkdir lock: %w", err)
 	}
 
-	// Write our PID.
-	if err := os.WriteFile(pidFile, []byte(strconv.Itoa(os.Getpid())+"\n"), 0644); err != nil {
-		return nil, fmt.Errorf("write pid: %w", err)
+	release, err := flock.Lock(lockFile)
+	if err != nil {
+		return nil, fmt.Errorf("acquire singleton lock: %w", err)
 	}
 
-	return &instanceLock{pidFile: pidFile}, nil
-}
+	// Clean up the legacy PID file if present; it is no longer used.
+	pidFile := filepath.Join(home, ".odek", "telegram.pid")
+	os.Remove(pidFile)
 
-// release removes the PID file on clean shutdown.
-func (l *instanceLock) release() {
-	os.Remove(l.pidFile)
+	return release, nil
 }
 
 // ── send_message helpers ──────────────────────────────────────────────

cmd/odek/telegram_test.go

@@ -836,3 +836,71 @@ func TestHandleRestartCommand_AuthorizationAndCooldown(t *testing.T) {
 		t.Fatalf("cooldown should block restart, got reply=%q triggered=%v", reply, triggered)
 	}
 }
+
+// ── singleton lock tests ────────────────────────────────────────────────
+
+func TestAcquireLock_CreatesLockFile(t *testing.T) {
+	dir := t.TempDir()
+	t.Setenv("HOME", dir)
+
+	release, err := acquireLock()
+	if err != nil {
+		t.Fatalf("acquireLock: %v", err)
+	}
+	defer release()
+
+	lockFile := filepath.Join(dir, ".odek", "telegram.lock")
+	info, err := os.Stat(lockFile)
+	if err != nil {
+		t.Fatalf("stat lock file: %v", err)
+	}
+	if perm := info.Mode().Perm(); perm != 0600 {
+		t.Errorf("lock file mode = %04o, want 0600", perm)
+	}
+}
+
+func TestAcquireLock_RemovesLegacyPIDFile(t *testing.T) {
+	dir := t.TempDir()
+	t.Setenv("HOME", dir)
+
+	pidFile := filepath.Join(dir, ".odek", "telegram.pid")
+	if err := os.MkdirAll(filepath.Dir(pidFile), 0755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(pidFile, []byte("12345\n"), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	release, err := acquireLock()
+	if err != nil {
+		t.Fatalf("acquireLock: %v", err)
+	}
+	defer release()
+
+	if _, err := os.Stat(pidFile); !os.IsNotExist(err) {
+		t.Errorf("legacy PID file was not removed")
+	}
+}
+
+func TestAcquireLock_DoesNotKillLegacyPID(t *testing.T) {
+	dir := t.TempDir()
+	t.Setenv("HOME", dir)
+
+	pidFile := filepath.Join(dir, ".odek", "telegram.pid")
+	if err := os.MkdirAll(filepath.Dir(pidFile), 0755); err != nil {
+		t.Fatal(err)
+	}
+	// Old PID-file logic would have killed this process. The flock-based lock
+	// must not act on the PID file contents at all.
+	if err := os.WriteFile(pidFile, []byte(fmt.Sprintf("%d\n", os.Getpid())), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	release, err := acquireLock()
+	if err != nil {
+		t.Fatalf("acquireLock: %v", err)
+	}
+	defer release()
+
+	// If we reach here, the current process is still alive.
+}

docs/SECURITY.md

@@ -316,6 +316,10 @@ MCP server subprocesses no longer inherit the full odek process environment. The
 
 Schedule persistence (`schedules.json` and `schedule-state.json`) now writes through `internal/fsatomic.WriteFile`. It creates a uniquely-named temp file with `O_EXCL` (so a pre-created symlink cannot be opened), fsyncs the data and parent directory, and atomically renames over the target. This means a swapped-in symlink is replaced rather than followed, closing the symlink-override attack where an attacker points `schedules.json.tmp` or `schedule-state.json.tmp` at sensitive files.
 
+### 21. Telegram singleton lock uses flock instead of PID file
+
+The Telegram bot previously used a PID file at `~/.odek/telegram.pid` to enforce a single polling instance. On Linux it verified `/proc/<pid>/cmdline`, but on macOS and other POSIX systems it would kill whatever process the planted PID belonged to. The implementation now uses an advisory `flock` on `~/.odek/telegram.lock` via `internal/flock`. A second instance simply blocks until the first releases the lock, and the OS releases the lock automatically if the holder crashes, eliminating the arbitrary-process-kill vector.
+
 ### YOLO mode
 
 ```json

docs/TELEGRAM.md

@@ -415,7 +415,7 @@ The package defines Telegram API types used throughout:
 
 ### Singleton Lock
 
-The bot writes its PID to `~/.odek/telegram.pid` on startup. If a stale PID file exists from a previous instance, the new process kills it (SIGTERM → 5s grace → SIGKILL) before taking over. This prevents 409 Conflict errors from dual polling.
+The bot acquires an advisory file lock on `~/.odek/telegram.lock` on startup. If another instance is already running, the new process blocks on the lock until the old process exits, then takes over automatically. This prevents 409 Conflict errors from dual polling without trusting or killing PID values, which could otherwise be planted to target unrelated processes.
 
 ### Graceful Restart
 
@@ -445,9 +445,9 @@ During restart:
 
 3. **New messages are rejected** — any message arriving while restart is in progress gets "⏳ Bot is restarting — please try again in a few seconds." The message is not lost (it remains in the Telegram server).
 
-4. **Bounded drain** — the process waits up to 15 seconds for all agent goroutines to finish. If a task is stuck (e.g., a long HTTP call that ignores context), the child process takes over and the parent is killed by the singleton lock.
+4. **Bounded drain** — the process waits up to 15 seconds for all agent goroutines to finish. If a task is stuck (e.g., a long HTTP call that ignores context), the child process takes over after the parent releases the singleton lock.
 
-5. **PID file cleanup** — before `os.Exit(0)`, the PID file lock is explicitly released so the child process starts with no stale lock file.
+5. **Lock release** — before `os.Exit(0)`, the singleton lock is explicitly released so the child process can acquire it immediately.
 
 6. **Post-restart notification** — when the new instance starts, it reads the restart marker file and sends "🔄 Bot restarted" to each chat that was active during the restart.
 
@@ -458,12 +458,13 @@ The actual process handoff uses the same spawn+exit mechanism:
 ```
 SIGHUP → gracefulRestart() → writeRestartMarker() → spawnChild() → os.Exit(0)
                                                                    ↓
-                                                child acquireLock() kills parent
+                                                parent releases singleton lock
+                                                child acquireLock() succeeds
                                                 child gets fresh HTTP/2 connections
                                                 child starts polling Telegram
 ```
 
-The child process inherits environment variables and command-line arguments. `acquireLock` ensures the old process is dead before the new one starts polling. The restart marker at `~/.odek/restart.json` carries the list of chat IDs that had active agent runs.
+The child process inherits environment variables and command-line arguments. `acquireLock` waits for the parent to release the lock, then the child starts polling. The restart marker at `~/.odek/restart.json` carries the list of chat IDs that had active agent runs.
 
 This avoids binary overwrite races, stale HTTP/2 connections, and session context loops that plagued `syscall.Exec`. The restart marker (`~/.odek/restart.json`) enables the new instance to notify users that a restart occurred.