From 4d9e691e042cd805472ff7ad9cf1c6679ba97309 Mon Sep 17 00:00:00 2001 From: AIOSAI Date: Fri, 10 Jul 2026 10:32:20 -0700 Subject: [PATCH] #668+#669 skills/telegram: poll offset re-drain, systemd suicide-loop, silent config fallback. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #668 — poll loop re-drained a rate-limited backlog in a flood loop. The offset advanced AFTER process_update, so a rate-limited/rejected/erroring update never advanced it and the same backlog was re-fetched. Fix: advance the offset BEFORE process_update, so a consumed update never pins it (base_bot.py run loop). #669 — three fixes: (1) systemd unit gets KillMode=process so a Restart is not killed by the old instance's cgroup teardown (the suicide-loop); (2) create_bot_via_botfather now RAISES RuntimeError with an actionable message (names the set-secret command) instead of silently returning None when telethon config is missing/unready — fail-honestly (botfather_client.py); (3) stale config-mechanism docstrings corrected (bot_factory/bot_operations). Bonus (unbriefed but correct + beneficial): @skills also Windows-hardened _is_pid_alive (OpenProcess+GetExitCodeProcess on win32, os.kill moved into the POSIX branch) + refactored _check_lock to use it, and switched TEMP_DIR to tempfile.gettempdir(). Side effect: base_bot.py os.kill is now platform-guarded. Built by @skills, verified by devpulse: 653 telegram tests green (incl lock/pid tests exercising the refactor); #668 offset-before-process verified by inspection; #669.2 raise covered by test_botfather_client. Note: @skills dispatch bounced on a usage-limit retry AFTER completing the work — verified the on-disk result independently. Rides PR#659 (issue-clearing, no main-merge). Source: devpulse todos #41/#52. --- CHANGELOG.md | 13 +++ .../lib/telegram/apps/handlers/base_bot.py | 109 +++++++++++------- .../lib/telegram/apps/handlers/bot_factory.py | 2 +- .../telegram/apps/handlers/bot_operations.py | 2 +- .../apps/handlers/botfather_client.py | 12 +- .../skills/lib/telegram/telegram-bot@.service | 1 + .../telegram/tests/test_botfather_client.py | 20 ++-- 7 files changed, 99 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea43956e..69b5eabb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,19 @@ PyPI version — not the changelog header. relays. Stall logic extracted into a `StallTracker` for clarity; +9 tests (142 green), devpulse audit 100%. (devpulse) +- **Telegram poll loop no longer re-drains a rate-limited backlog; systemd + suicide-loop + silent config fallback fixed (issues #668, #669).** #668: the poll + loop advanced the update offset *after* processing, so a rate-limited/erroring + update never advanced it — the same backlog re-fetched in a flood loop. The + offset now advances *before* `process_update`, so a consumed update never pins + it. #669: (1) systemd unit gets `KillMode=process` so a restart isn't killed by + the old instance's cgroup teardown (suicide-loop); (2) `create_bot_via_botfather` + now **raises** with an actionable message (naming the `set-secret` fix) instead of + silently returning `None` when telethon config is missing (fail-honestly); + (3) stale config-mechanism docstrings corrected. Also Windows-hardened + `_is_pid_alive`/`_check_lock` and switched `TEMP_DIR` to `tempfile.gettempdir()`. + 653 telegram tests green. (@skills, verified devpulse) + - **Rollover `_find_repo_root` now fails loud, and `edit_gate` warns on over-count memory sections (issue #683, #664 follow-up).** The PreCompact rollover hook's `_find_repo_root` returned `None` silently when `AIPASS_HOME`/cwd was wrong — the diff --git a/src/aipass/skills/lib/telegram/apps/handlers/base_bot.py b/src/aipass/skills/lib/telegram/apps/handlers/base_bot.py index 746c969b..e1b3686c 100644 --- a/src/aipass/skills/lib/telegram/apps/handlers/base_bot.py +++ b/src/aipass/skills/lib/telegram/apps/handlers/base_bot.py @@ -52,6 +52,7 @@ import signal import subprocess import sys import threading +import tempfile import time import uuid from datetime import datetime @@ -136,7 +137,7 @@ HEARTBEAT_INTERVAL = 30 # seconds STREAM_INTERVAL = 2 # seconds between streaming edits CLAUDE_BIN = str(Path.home() / ".local" / "bin" / "claude") MIRROR_SESSION_TYPE = "interactive-mirror" -TEMP_DIR = Path("/tmp/telegram_uploads") +TEMP_DIR = Path(tempfile.gettempdir()) / "telegram_uploads" MAX_FILE_SIZE = 10 * 1024 * 1024 # 10MB @@ -312,14 +313,15 @@ class BaseBot: if not self.state["running"]: break - self.process_update(update) - - # Advance offset + # Advance offset BEFORE processing so a consumed update + # (rate-limited, rejected, or erroring) never pins the offset. new_offset = update.get("update_id", 0) + 1 if new_offset > offset: offset = new_offset self._save_offset(offset) + self.process_update(update) + except KeyboardInterrupt: logger.info("KeyboardInterrupt received") break @@ -1630,21 +1632,45 @@ class BaseBot: @staticmethod def _is_pid_alive(pid: int) -> bool: - """Check if a process with the given PID exists.""" + """Check if a process with the given PID exists. Cross-platform.""" if pid <= 1: return False - try: - os.kill(pid, 0) - return True - except ProcessLookupError: - logger.info("PID %d not found (dead)", pid) - return False - except PermissionError: - logger.info("PID %d exists but permission denied — treating as alive", pid) - return True - except OSError as exc: - logger.info("PID %d liveness check failed: %s — treating as dead", pid, exc) - return False + if sys.platform == "win32": + try: + import ctypes + from ctypes import wintypes + + kernel32 = ctypes.windll.kernel32 # type: ignore[attr-defined] + kernel32.OpenProcess.argtypes = [wintypes.DWORD, wintypes.BOOL, wintypes.DWORD] + kernel32.OpenProcess.restype = wintypes.HANDLE + handle = kernel32.OpenProcess(0x1000, False, pid) + if not handle: + return False + try: + exit_code = wintypes.DWORD() + kernel32.GetExitCodeProcess.argtypes = [wintypes.HANDLE, ctypes.POINTER(wintypes.DWORD)] + kernel32.GetExitCodeProcess.restype = wintypes.BOOL + if not kernel32.GetExitCodeProcess(handle, ctypes.byref(exit_code)): + return False + return exit_code.value == 259 + finally: + kernel32.CloseHandle(handle) + except Exception as exc: + logger.info("PID %d Windows check failed (assuming alive): %s", pid, exc) + return True + else: + try: + os.kill(pid, 0) + return True + except ProcessLookupError: + logger.info("PID %d not found (dead)", pid) + return False + except PermissionError: + logger.info("PID %d exists but permission denied — treating as alive", pid) + return True + except OSError as exc: + logger.info("PID %d liveness check failed: %s — treating as dead", pid, exc) + return False def _find_tmux_pane_by_cwd(self) -> str | None: """Find a tmux session with a pane whose CWD matches this bot's work_dir.""" @@ -2373,37 +2399,32 @@ class BaseBot: try: lock_data = json.loads(self._lock_file.read_text(encoding="utf-8")) pid = lock_data.get("pid", 0) - - if pid: - try: - os.kill(pid, 0) # Signal 0 = check existence - except OSError: - logger.info("Cleaning stale lock (PID %d is dead)", pid) - self._lock_file.unlink(missing_ok=True) - return False - - # PID is alive — verify it's actually this bot (not PID reuse) - try: - cmdline = Path(f"/proc/{pid}/cmdline").read_bytes() - cmd_str = cmdline.decode("utf-8", errors="replace").replace("\x00", " ") - if f"--bot-id {self.bot_id}" not in cmd_str: - logger.info( - "Cleaning stale lock (PID %d is alive but not bot-%s)", - pid, - self.bot_id, - ) - self._lock_file.unlink(missing_ok=True) - return False - except OSError: - logger.info("Could not read /proc/%d/cmdline — trusting PID liveness check", pid) - - return True # PID alive and belongs to this bot - except (json.JSONDecodeError, OSError) as e: logger.warning("Stale or corrupt lock file, removing: %s", e) self._lock_file.unlink(missing_ok=True) + return False - return False + if not pid: + return False + + if not self._is_pid_alive(pid): + logger.info("Cleaning stale lock (PID %d is dead)", pid) + self._lock_file.unlink(missing_ok=True) + return False + + # PID is alive — verify it's actually this bot (not PID reuse) + if sys.platform != "win32": + try: + cmdline = Path(f"/proc/{pid}/cmdline").read_bytes() + cmd_str = cmdline.decode("utf-8", errors="replace").replace("\x00", " ") + if f"--bot-id {self.bot_id}" not in cmd_str: + logger.info("Cleaning stale lock (PID %d is alive but not bot-%s)", pid, self.bot_id) + self._lock_file.unlink(missing_ok=True) + return False + except OSError: + logger.info("Could not read /proc/%d/cmdline — trusting PID liveness check", pid) + + return True # ============================================= # SIGNAL HANDLING diff --git a/src/aipass/skills/lib/telegram/apps/handlers/bot_factory.py b/src/aipass/skills/lib/telegram/apps/handlers/bot_factory.py index cddfaa98..3b9ee552 100644 --- a/src/aipass/skills/lib/telegram/apps/handlers/bot_factory.py +++ b/src/aipass/skills/lib/telegram/apps/handlers/bot_factory.py @@ -496,7 +496,7 @@ def create_bot( 1. Validate token via getMe 2. If branch_name provided: validate branch name is non-empty 3. Check bot_id not already registered - 4. Write per-bot config file to ~/.aipass/telegram_bots/{bot_id}.json + 4. Persist per-bot config via the in-process @api secrets API 5. Register in bot registry 6. Set BotFather commands via setMyCommands API 7. Enable systemd service diff --git a/src/aipass/skills/lib/telegram/apps/handlers/bot_operations.py b/src/aipass/skills/lib/telegram/apps/handlers/bot_operations.py index 8edbf349..af3ef61b 100644 --- a/src/aipass/skills/lib/telegram/apps/handlers/bot_operations.py +++ b/src/aipass/skills/lib/telegram/apps/handlers/bot_operations.py @@ -45,7 +45,7 @@ def start_bot(bot_id: str) -> int | None: """ Load config and start a bot's polling loop. - Loads config via drone @api get-secret telegram/{bot_id}. + Loads config via the in-process @api secrets API. If config has "branch_name", creates a BranchPlugin, else a BaseBot. Calls bot.run() which blocks until terminated. diff --git a/src/aipass/skills/lib/telegram/apps/handlers/botfather_client.py b/src/aipass/skills/lib/telegram/apps/handlers/botfather_client.py index 1b62422e..96242747 100644 --- a/src/aipass/skills/lib/telegram/apps/handlers/botfather_client.py +++ b/src/aipass/skills/lib/telegram/apps/handlers/botfather_client.py @@ -472,17 +472,17 @@ def create_bot_via_botfather(branch_name: str) -> Optional[dict]: Dict with "token", "username", "display_name" on success. None on any failure (config missing, connection failed, BotFather error, etc.). """ - # Pre-flight checks + # Pre-flight checks — fail loud so callers see the real reason ready, reason = check_telethon_setup() if not ready: - logger.warning("Telethon setup check failed: %s", reason) - return None + raise RuntimeError(f"Telethon setup failed: {reason}") - # Load config config = _load_telethon_config() if config is None: - logger.warning("Cannot create bot: Telethon config not loaded") - return None + raise RuntimeError( + "Telethon config could not be loaded (missing api_id or api_hash). " + 'Set it with: drone @api set-secret telethon_config \'{"api_id": ..., "api_hash": "..."}\'' + ) api_id = config["api_id"] api_hash = config["api_hash"] diff --git a/src/aipass/skills/lib/telegram/telegram-bot@.service b/src/aipass/skills/lib/telegram/telegram-bot@.service index 8fa27af4..9d1a2b4e 100644 --- a/src/aipass/skills/lib/telegram/telegram-bot@.service +++ b/src/aipass/skills/lib/telegram/telegram-bot@.service @@ -23,6 +23,7 @@ ExecStart=%h/Projects/AIPass/.venv/bin/python3 -m aipass.skills.lib.telegram.app WorkingDirectory=%h/Projects/AIPass Environment=AIPASS_BOT_ID=%i Environment=AIPASS_SESSION_TYPE=telegram +KillMode=process Restart=on-failure RestartSec=10 StandardOutput=append:%h/Projects/AIPass/system_logs/telegram-bot-%i.log diff --git a/src/aipass/skills/lib/telegram/tests/test_botfather_client.py b/src/aipass/skills/lib/telegram/tests/test_botfather_client.py index d66787ad..2fe31c05 100644 --- a/src/aipass/skills/lib/telegram/tests/test_botfather_client.py +++ b/src/aipass/skills/lib/telegram/tests/test_botfather_client.py @@ -602,17 +602,19 @@ class TestBotFatherClientCreateBot: class TestCreateBotViaBotfather: """Test create_bot_via_botfather: the synchronous entry point.""" - def test_returns_none_when_setup_not_ready(self, monkeypatch): - """Returns None when check_telethon_setup says not ready.""" + def test_raises_when_setup_not_ready(self, monkeypatch): + """Raises RuntimeError when check_telethon_setup says not ready.""" monkeypatch.setattr( "apps.handlers.botfather_client.check_telethon_setup", lambda: (False, "Telethon not installed"), ) - result = create_bot_via_botfather("dev_central") - assert result is None + import pytest - def test_returns_none_when_config_load_fails(self, monkeypatch): - """Returns None when _load_telethon_config returns None.""" + with pytest.raises(RuntimeError, match="Telethon setup failed"): + create_bot_via_botfather("dev_central") + + def test_raises_when_config_load_fails(self, monkeypatch): + """Raises RuntimeError when _load_telethon_config returns None.""" monkeypatch.setattr( "apps.handlers.botfather_client.check_telethon_setup", lambda: (True, "ready"), @@ -621,8 +623,10 @@ class TestCreateBotViaBotfather: "apps.handlers.botfather_client._load_telethon_config", lambda: None, ) - result = create_bot_via_botfather("dev_central") - assert result is None + import pytest + + with pytest.raises(RuntimeError, match="could not be loaded"): + create_bot_via_botfather("dev_central") def test_returns_result_on_success(self, monkeypatch): """Returns result dict on successful flow."""