fix: address PR review comments - retry logic, Telegram ok:false handling, and cleanup

- _retry_transient: only retry HTTPError on 5xx and 429 status codes;
  permanent 4xx responses now re-raise immediately instead of burning
  retries on errors that won't recover.
- _edit_status / _answer_callback / _edit_message_remove_keyboard: use
  new _check_telegram_ok helper that raises _TelegramAPIError when
  Telegram returns HTTP 200 with {ok: false}, so application-level
  failures surface and stop being silently swallowed.
- _edit_status: tighten retries=1/backoff=0.5 (was the default 2/1.0) so
  the dispatch hot path can't block ~17s waiting on a flaky edit.
- tests: drop unused 'import time as _time'; expand _retry_transient
  coverage for 5xx/429 retry + 4xx no-retry; add coverage for
  _check_telegram_ok on ok:true and ok:false bodies.

Author: fdsimoes-git

src/telegram_bot.py

@@ -604,17 +604,52 @@ def _send_status(cfg: Config, text: str) -> int | None:
         return None
 
 
+class _TelegramAPIError(Exception):
+    """Telegram returned HTTP 200 with ok:false (application-level error)."""
+
+
+def _check_telegram_ok(r: requests.Response) -> None:
+    """Raise on transport errors AND on Telegram ok=false bodies.
+
+    Telegram returns HTTP 200 with ``{ok: false, error_code, description}``
+    for application-level failures (e.g. "message to edit not found"), so
+    ``raise_for_status()`` alone leaves those silent.
+    """
+    r.raise_for_status()
+    try:
+        body = r.json()
+    except ValueError:
+        return
+    if isinstance(body, dict) and body.get("ok") is False:
+        raise _TelegramAPIError(
+            f"Telegram ok=false: {body.get('error_code')} {body.get('description')}"
+        )
+
+
 def _retry_transient(fn, *, retries: int = 2, backoff: float = 1.0):
-    """Call *fn*; retry up to *retries* times on transient network errors."""
+    """Call *fn*; retry up to *retries* times on transient network errors.
+
+    HTTPError is only retried for 5xx and 429; other 4xx responses are
+    permanent client errors and are re-raised immediately.
+    """
     for attempt in range(1 + retries):
         try:
             return fn()
-        except (requests.exceptions.ConnectionError, requests.exceptions.Timeout, requests.exceptions.HTTPError) as e:
+        except (requests.exceptions.ConnectionError, requests.exceptions.Timeout) as e:
             if attempt < retries:
                 log.warning("Transient Telegram error (attempt %d/%d): %s", attempt + 1, retries + 1, e)
                 time.sleep(backoff)
             else:
                 raise
+        except requests.exceptions.HTTPError as e:
+            status = getattr(e.response, "status_code", None)
+            if status is None or (status < 500 and status != 429):
+                raise
+            if attempt < retries:
+                log.warning("Transient Telegram HTTP %s (attempt %d/%d): %s", status, attempt + 1, retries + 1, e)
+                time.sleep(backoff)
+            else:
+                raise
 
 
 def _edit_status(cfg: Config, msg_id: int, text: str) -> None:
@@ -626,8 +661,10 @@ def _do():
                 json={"chat_id": cfg.telegram_chat_id, "message_id": msg_id, "text": text},
                 timeout=5,
             )
-            r.raise_for_status()
-        _retry_transient(_do)
+            _check_telegram_ok(r)
+        # Tight bounds: this runs on the dispatch hot path, so cap total
+        # latency well below the 30s long-poll cadence.
+        _retry_transient(_do, retries=1, backoff=0.5)
     except Exception as e:  # noqa: BLE001
         log.warning("edit_status failed (non-fatal): %s", e)
 
@@ -702,7 +739,7 @@ def _do():
                 json={"callback_query_id": callback_id, "text": text},
                 timeout=10,
             )
-            r.raise_for_status()
+            _check_telegram_ok(r)
         _retry_transient(_do, retries=1)
     except Exception as e:  # noqa: BLE001
         log.error("answerCallbackQuery failed: %s", e)
@@ -732,7 +769,7 @@ def _do():
                 },
                 timeout=10,
             )
-            r.raise_for_status()
+            _check_telegram_ok(r)
         _retry_transient(_do)
     except Exception as e:  # noqa: BLE001
         log.error("editMessageText failed: %s", e)

tests/test_telegram_bot.py

@@ -824,7 +824,6 @@ def test_retry_transient_retries_on_timeout(monkeypatch):
 
 def test_retry_transient_retries_on_connection_error(monkeypatch):
     """_retry_transient retries on ConnectionError."""
-    import time as _time
     from unittest.mock import MagicMock
     from src.telegram_bot import _retry_transient
 
@@ -857,18 +856,88 @@ def test_retry_transient_raises_after_exhaustion(monkeypatch):
     assert fn.call_count == 2
 
 
-def test_retry_transient_retries_on_http_error(monkeypatch):
-    """_retry_transient retries on HTTPError from raise_for_status()."""
+def _http_error(status: int) -> requests.exceptions.HTTPError:
+    """Build an HTTPError with a response carrying the given status code."""
+    resp = requests.Response()
+    resp.status_code = status
+    return requests.exceptions.HTTPError(f"{status} error", response=resp)
+
+
+def test_retry_transient_retries_on_5xx(monkeypatch):
+    """_retry_transient retries when the HTTPError carries a 5xx status."""
     from unittest.mock import MagicMock
     from src.telegram_bot import _retry_transient
 
-    fn = MagicMock(side_effect=[
-        requests.exceptions.HTTPError("500 Server Error"),
-        "ok",
-    ])
+    fn = MagicMock(side_effect=[_http_error(500), "ok"])
+    import src.telegram_bot as _tb
+    monkeypatch.setattr(_tb.time, "sleep", MagicMock())
+
+    result = _retry_transient(fn, retries=1)
+    assert result == "ok"
+    assert fn.call_count == 2
+
+
+def test_retry_transient_retries_on_429(monkeypatch):
+    """Rate-limit responses are transient — retry them."""
+    from unittest.mock import MagicMock
+    from src.telegram_bot import _retry_transient
+
+    fn = MagicMock(side_effect=[_http_error(429), "ok"])
     import src.telegram_bot as _tb
     monkeypatch.setattr(_tb.time, "sleep", MagicMock())
 
     result = _retry_transient(fn, retries=1)
     assert result == "ok"
     assert fn.call_count == 2
+
+
+def test_retry_transient_does_not_retry_4xx(monkeypatch):
+    """4xx client errors (except 429) are permanent — re-raise immediately."""
+    from unittest.mock import MagicMock
+    from src.telegram_bot import _retry_transient
+
+    fn = MagicMock(side_effect=_http_error(400))
+    import src.telegram_bot as _tb
+    sleep_mock = MagicMock()
+    monkeypatch.setattr(_tb.time, "sleep", sleep_mock)
+
+    with pytest.raises(requests.exceptions.HTTPError):
+        _retry_transient(fn, retries=2)
+    # No retries attempted, no sleep.
+    assert fn.call_count == 1
+    sleep_mock.assert_not_called()
+
+
+def test_retry_transient_does_not_retry_404(monkeypatch):
+    from unittest.mock import MagicMock
+    from src.telegram_bot import _retry_transient
+
+    fn = MagicMock(side_effect=_http_error(404))
+    import src.telegram_bot as _tb
+    monkeypatch.setattr(_tb.time, "sleep", MagicMock())
+
+    with pytest.raises(requests.exceptions.HTTPError):
+        _retry_transient(fn, retries=2)
+    assert fn.call_count == 1
+
+
+def test_check_telegram_ok_raises_on_ok_false():
+    """Telegram returning HTTP 200 with ok:false must raise so the failure
+    surfaces — raise_for_status alone doesn't catch this case."""
+    from src.telegram_bot import _check_telegram_ok, _TelegramAPIError
+
+    r = requests.Response()
+    r.status_code = 200
+    r._content = b'{"ok": false, "error_code": 400, "description": "message to edit not found"}'
+    with pytest.raises(_TelegramAPIError, match="message to edit not found"):
+        _check_telegram_ok(r)
+
+
+def test_check_telegram_ok_passes_on_ok_true():
+    from src.telegram_bot import _check_telegram_ok
+
+    r = requests.Response()
+    r.status_code = 200
+    r._content = b'{"ok": true, "result": {}}'
+    # No exception.
+    _check_telegram_ok(r)