fix: stop emailing RESOLVED after every successful sync - #68
Conversation
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) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe alert transport now sends an ChangesAlert transition semantics
Priority: ⚪ Pending latest changes Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Daemon
participant alert.raise_
participant FlagFile
participant Mailgun
Daemon->>alert.raise_: report failing cycle
alert.raise_->>FlagFile: store latest failure reason
alert.raise_->>Mailgun: send ALERT
Daemon->>alert.clear: process recovery cycle
alert.clear->>FlagFile: remove active flag
alert.clear->>Mailgun: send RESOLVED
Merge Risk: ⚪ Minimal · up to The alert transition change has no remaining actionable merge risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🟡 Changes recommended
Preserve a pending resolution or retry mechanism so failed recovery notifications are not permanently lost.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates alert recovery notifications to avoid repeated RESOLVED emails while preserving per-failure ALERT notifications. A remaining issue is that recovery notifications may be lost if dispatch fails after the alert flag is deleted.
Changes:
- Makes resolution notifications edge-triggered.
- Adds tests for recovery, repeated failures, and stuck flags.
- Updates configuration documentation.
File summaries
| File | Description |
|---|---|
tests/test_alert.py |
Tests new alert and recovery behavior. |
src/door_sync/alert.py |
Implements edge-triggered resolution emails. |
docs/configuration.rst |
Documents notification semantics. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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) <noreply@anthropic.com>
Problem
With an email transport configured (
alert.transport = "smtp"or"mailgun"), the daemon sent a RESOLVED email after every successful reconcile cycle — not just after recovering from an alert.orchestrator.reconcile()callsalert.clear()at the end of every successful cycle, andclear()dispatched RESOLVED wheneveralert_configwas set, unconditionally.path.unlink(missing_ok=True)swallowed the "no flag was there" case, so nothing distinguished a real recovery from an ordinary healthy cycle.Fix
RESOLVED is now edge-triggered on the alert flag file, which is already the project's
presence = active alertsignal:path.unlink()withoutmissing_ok;FileNotFoundErrormeans no alert was active → return silently, no email.OSErroron removal (stuck flag) logs as before and also returns without mailing — that isn't a transition, and mailing there would reproduce the spam on every subsequent cycle. The retry happens next cycle, and RESOLVED goes out when the removal actually succeeds.ALERT is unchanged. Every failing cycle mails, including consecutive failures on the same condition — a halt that repeats is a sync that is still not happening, and deduping it would drop that signal.
Testing
uv run pytest— 420 passed. Five tests added totests/test_alert.py:[ALERT, RESOLVED][ALERT, ALERT, RESOLVED]uv run ruff check .,uv run ruff format --check ., anduv run pyrefly checkare clean.Deployment note
alert.pyships in the package, so this needs an OTA /.debupdate to reach a device. No config change required — no new keys.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation