From ecf68f14a2e9dbc8085069260d761723ff59d71b Mon Sep 17 00:00:00 2001 From: Ryan Morash Date: Tue, 15 Sep 2026 20:48:27 -0400 Subject: [PATCH 1/2] fix: stop emailing RESOLVED after every successful sync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit alert.clear() runs after every successful reconcile, and it dispatched a RESOLVED email whenever an email transport was configured — regardless of whether an alert had ever been raised. path.unlink(missing_ok=True) swallowed the "no flag was there" case, so nothing distinguished a real recovery from an ordinary healthy cycle, and an operator with SMTP or Mailgun configured got mail every reconcile. Make RESOLVED edge-triggered on the flag file, which is already the project's "presence = active alert" signal: unlink() without missing_ok, and treat FileNotFoundError as "no alert was active, say nothing". A failure to remove an existing flag is not a transition either, so it logs and returns rather than mailing on every subsequent cycle until the path is fixed. ALERT is unchanged: every failing cycle mails, including consecutive failures on the same condition. Deduping it would drop the signal that a sync is still not happening. Co-Authored-By: Claude Opus 5 (1M context) --- docs/configuration.rst | 6 ++- src/door_sync/alert.py | 30 ++++++++++++-- tests/test_alert.py | 93 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 125 insertions(+), 4 deletions(-) diff --git a/docs/configuration.rst b/docs/configuration.rst index 33c0ed9..3d219b7 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -311,7 +311,11 @@ dispatched. The flag file (``ops.alert_flag``) is **always** written regardless of the transport setting — it serves as a simple signal for external monitoring tools. -Optionally, an email alert can be sent via SMTP or Mailgun. +Optionally, an email alert can be sent via SMTP or Mailgun. Every failing +cycle sends an ``ALERT``, including consecutive failures on the same +condition — a halt that repeats is a sync that is still not happening. +``RESOLVED`` is sent once, on the cycle that clears an alert that was actually +active, so a healthy daemon does not mail after every sync. Flag-file only (default) """""""""""""""""""""""" diff --git a/src/door_sync/alert.py b/src/door_sync/alert.py index 93ea84f..7c7a9c5 100644 --- a/src/door_sync/alert.py +++ b/src/door_sync/alert.py @@ -5,6 +5,11 @@ regardless of transport — external monitoring (Nagios, Prometheus textfile collector, etc.) can detect halts without parsing logs. +Every failing cycle sends an ALERT, including consecutive failures on the same +condition. RESOLVED is edge-triggered on the flag: it goes out once, on the +cycle that clears an alert that was actually active, so a healthy daemon does +not mail after every sync. + Email failures are logged at ERROR but never crash a reconcile cycle. """ @@ -36,6 +41,11 @@ def raise_( ) -> None: """Write flag file and, if configured, send an alert email. + Every failing cycle mails, including consecutive failures on the same + condition. A halt that repeats is a sync that is still not happening, and + each one is worth surfacing -- only RESOLVED is edge-triggered, so that a + healthy daemon stays silent (see `clear()`). + Args: reason: Human-readable description of the alert condition. path: Path to the alert flag file. @@ -52,18 +62,32 @@ def clear( path: Path, alert_config: AlertConfig | None = None, ) -> None: - """Remove flag file and, if configured, send a resolved email. + """Remove flag file and, if an alert was active, send a resolved email. + + RESOLVED is edge-triggered on the flag file: it goes out only on the cycle + that actually removes a flag, so a healthy daemon does not mail the + operator once per reconcile. A cycle that finds no flag has nothing to + resolve and says nothing. + + If the flag could not be written when the alert was raised, no RESOLVED + follows that alert -- the unwritable path is already on the logger. Args: path: Path to the alert flag file. alert_config: Email transport settings, or None for flag-file only. """ try: - path.unlink(missing_ok=True) + path.unlink() + except FileNotFoundError: + # No alert was active. Nothing to clear, nobody to notify. + return except OSError as exc: # A flag that cannot be cleared errs toward alarming, which is the safe - # direction, but the operator needs to know why it is stuck. + # direction, but the operator needs to know why it is stuck. No RESOLVED + # either: the flag is still there, so this was not a transition, and a + # stuck flag would otherwise mail on every subsequent cycle. _logger.error("could not clear alert flag %s: %s", path, exc) + return if alert_config is not None: _dispatch(alert_config, subject="RESOLVED", body="Previous alert cleared.") diff --git a/tests/test_alert.py b/tests/test_alert.py index efd71e6..76250be 100644 --- a/tests/test_alert.py +++ b/tests/test_alert.py @@ -317,3 +317,96 @@ def test_clear_survives_unremovable_flag(tmp_path: Path, caplog: pytest.LogCaptu holder.chmod(0o700) assert "could not clear alert flag" in caplog.text + + +# --- RESOLVED is edge-triggered on the flag, not sent every cycle --- + + +def test_clear_without_active_flag_sends_no_email(tmp_path: Path) -> None: + """A healthy cycle must stay silent. + + clear() runs after every successful reconcile, so dispatching RESOLVED + unconditionally mails the operator once per cycle forever. + """ + path = tmp_path / "alert.flag" + cfg = AlertConfig(transport="mailgun", smtp=None, mailgun=_mailgun_config()) + + with patch("door_sync.alert.httpx.post") as mock_post: + alert.clear(path=path, alert_config=cfg) + + mock_post.assert_not_called() + assert not path.exists() + + +def test_clear_sends_resolved_once_then_stays_silent(tmp_path: Path) -> None: + path = tmp_path / "alert.flag" + cfg = AlertConfig(transport="mailgun", smtp=None, mailgun=_mailgun_config()) + + with patch("door_sync.alert.httpx.post") as mock_post: + alert.raise_("safety halt", path=path, alert_config=cfg) + alert.clear(path=path, alert_config=cfg) + alert.clear(path=path, alert_config=cfg) + alert.clear(path=path, alert_config=cfg) + + subjects = [c.kwargs["data"]["subject"] for c in mock_post.call_args_list] + assert subjects == ["[door-sync] ALERT", "[door-sync] RESOLVED"] + + +@pytest.mark.skipif(os.geteuid() == 0, reason="root ignores file permissions") +def test_clear_sends_no_resolved_when_flag_cannot_be_removed(tmp_path: Path) -> None: + """A stuck flag is not a transition, so it must not mail on every cycle.""" + holder = tmp_path / "ro" + holder.mkdir() + flag = holder / "alert.flag" + flag.write_text("stale\n") + cfg = AlertConfig(transport="mailgun", smtp=None, mailgun=_mailgun_config()) + holder.chmod(0o500) + try: + with patch("door_sync.alert.httpx.post") as mock_post: + alert.clear(path=flag, alert_config=cfg) + finally: + holder.chmod(0o700) + + mock_post.assert_not_called() + + +# --- ALERT is per-failure, not per-episode --- + + +def test_raise_mails_every_failing_cycle(tmp_path: Path) -> None: + """Consecutive failures each mail: a repeat halt is a sync still not happening.""" + path = tmp_path / "alert.flag" + cfg = AlertConfig(transport="mailgun", smtp=None, mailgun=_mailgun_config()) + + with patch("door_sync.alert.httpx.post") as mock_post: + alert.raise_("mass deactivation: 12 of 30 active users", path=path, alert_config=cfg) + alert.raise_("mass deactivation: 12 of 30 active users", path=path, alert_config=cfg) + alert.raise_( + "crashed: ConnectionError: controller unreachable", path=path, alert_config=cfg + ) + + assert mock_post.call_count == 3 + bodies = [c.kwargs["data"]["text"] for c in mock_post.call_args_list] + assert bodies[1] == "mass deactivation: 12 of 30 active users" + assert bodies[2] == "crashed: ConnectionError: controller unreachable" + # The flag tracks the most recent reason. + assert path.read_text(encoding="utf-8") == "crashed: ConnectionError: controller unreachable\n" + + +def test_repeated_failures_then_recovery_sends_one_resolved(tmp_path: Path) -> None: + path = tmp_path / "alert.flag" + cfg = AlertConfig(transport="mailgun", smtp=None, mailgun=_mailgun_config()) + + with patch("door_sync.alert.httpx.post") as mock_post: + alert.raise_("safety halt", path=path, alert_config=cfg) + alert.raise_("safety halt", path=path, alert_config=cfg) + alert.clear(path=path, alert_config=cfg) + alert.clear(path=path, alert_config=cfg) + alert.clear(path=path, alert_config=cfg) + + subjects = [c.kwargs["data"]["subject"] for c in mock_post.call_args_list] + assert subjects == [ + "[door-sync] ALERT", + "[door-sync] ALERT", + "[door-sync] RESOLVED", + ] From f2edbc49d0dfdddfbe870eefdcbb1052adef25eb Mon Sep 17 00:00:00 2001 From: Ryan Morash Date: Tue, 15 Sep 2026 20:53:23 -0400 Subject: [PATCH 2/2] fix: name the undelivered notification in send-failure logs Copilot flagged that a RESOLVED whose dispatch fails is never retried: the flag is unlinked first, so every later healthy cycle returns early on FileNotFoundError. The observation is correct, but a pending-resolution state file is the wrong answer for a best-effort courtesy email, and the obvious alternative -- dispatch first, unlink only on success -- is worse: a failed send would leave the flag raised, telling external monitoring the system is still halted after it recovered. The flag, not the email, is the signal monitoring reads, so the recovery is not lost. What was actually missing is the ability to tell which notification got dropped: the failure log named the transport but not the subject. Include it, and record the no-retry reasoning in the clear() docstring so it does not get re-litigated. Co-Authored-By: Claude Opus 5 (1M context) --- src/door_sync/alert.py | 11 +++++++++-- tests/test_alert.py | 24 ++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/door_sync/alert.py b/src/door_sync/alert.py index 7c7a9c5..2ba5d2a 100644 --- a/src/door_sync/alert.py +++ b/src/door_sync/alert.py @@ -72,6 +72,13 @@ def clear( If the flag could not be written when the alert was raised, no RESOLVED follows that alert -- the unwritable path is already on the logger. + A RESOLVED that fails to send is not retried. The flag is already gone by + then, and that -- not the email -- is what external monitoring reads, so + the recovery is not actually lost. Re-raising the flag to force a retry + would tell monitoring the system is still halted, which is worse than a + missed courtesy email. The send failure is logged at ERROR with the + subject, so a dropped notification is traceable. + Args: path: Path to the alert flag file. alert_config: Email transport settings, or None for flag-file only. @@ -141,7 +148,7 @@ def _send_smtp(cfg: SmtpConfig, *, subject: str, body: str) -> None: server.send_message(msg) _logger.info("alert email sent via SMTP: %s", subject) except Exception as exc: - _logger.error("failed to send alert email via SMTP", exc_info=exc) + _logger.error("failed to send alert email via SMTP: %s", subject, exc_info=exc) def _send_mailgun(cfg: MailgunConfig, *, subject: str, body: str) -> None: @@ -162,4 +169,4 @@ def _send_mailgun(cfg: MailgunConfig, *, subject: str, body: str) -> None: resp.raise_for_status() _logger.info("alert email sent via Mailgun: %s", subject) except Exception as exc: - _logger.error("failed to send alert email via Mailgun", exc_info=exc) + _logger.error("failed to send alert email via Mailgun: %s", subject, exc_info=exc) diff --git a/tests/test_alert.py b/tests/test_alert.py index 76250be..b9b95d3 100644 --- a/tests/test_alert.py +++ b/tests/test_alert.py @@ -410,3 +410,27 @@ def test_repeated_failures_then_recovery_sends_one_resolved(tmp_path: Path) -> N "[door-sync] ALERT", "[door-sync] RESOLVED", ] + + +def test_failed_resolved_dispatch_names_the_lost_notification( + tmp_path: Path, + caplog: pytest.LogCaptureFixture, +) -> None: + """A RESOLVED that cannot be delivered is not retried, so the log must name it. + + The flag is already gone, so external monitoring sees the recovery either + way -- but the operator waiting on the all-clear email needs to be able to + tell from the log which notification was dropped. + """ + path = tmp_path / "alert.flag" + path.write_text("reason\n", encoding="utf-8") + cfg = AlertConfig(transport="mailgun", smtp=None, mailgun=_mailgun_config()) + + with ( + patch("door_sync.alert.httpx.post", side_effect=ConnectionError("refused")), + caplog.at_level(logging.ERROR, logger="door_sync.alert"), + ): + alert.clear(path=path, alert_config=cfg) + + assert not path.exists() + assert any("RESOLVED" in r.getMessage() for r in caplog.records)