fix(mail): IMAP-acct invariant, test isolation, mail-poller healthcheck
Some checks failed
Deploy / docs (push) Has been skipped
Infra CI / notebooks (push) Failing after 16s
Infra CI / api (push) Successful in 32s
Deploy / report (push) Successful in 14s
CI / lint (push) Successful in 28s
Deploy / notebooks (push) Has been skipped
Deploy / zotero (push) Has been skipped
Deploy / api (push) Has been skipped
Deploy / mc (push) Has been skipped
Infra CI / zotero (push) Successful in 14s
Infra CI / docs (push) Successful in 15s
Infra CI / mc (push) Successful in 12s
CI / test (push) Successful in 12m27s
Harden / build-scan-report (push) Failing after 42s
Renovate / renovate (push) Successful in 16s
Package Supply Chain / pkg-supply-chain (push) Failing after 49s
Some checks failed
Deploy / docs (push) Has been skipped
Infra CI / notebooks (push) Failing after 16s
Infra CI / api (push) Successful in 32s
Deploy / report (push) Successful in 14s
CI / lint (push) Successful in 28s
Deploy / notebooks (push) Has been skipped
Deploy / zotero (push) Has been skipped
Deploy / api (push) Has been skipped
Deploy / mc (push) Has been skipped
Infra CI / zotero (push) Successful in 14s
Infra CI / docs (push) Successful in 15s
Infra CI / mc (push) Successful in 12s
CI / test (push) Successful in 12m27s
Harden / build-scan-report (push) Failing after 42s
Renovate / renovate (push) Successful in 16s
Package Supply Chain / pkg-supply-chain (push) Failing after 49s
Three pre-existing bugs surfaced by the postmaster@mail.fhirworx.io DMARC setup; all fixed together because they interact. 1. `seed_mailboxes` and `rotate_creds` only created the IMAP mailbox when the SMTP-creds `password` path failed (i.e. first-time provision). If creds existed but imap-acct didn't — observed for postmaster@mail.fhirworx.io after a partial earlier run — auth would succeed and delivered mail would have nowhere to land, silently dropping e.g. DMARC aggregate reports. Both functions now list `maddy imap-acct` once per droplet visit and ensure each address has its IMAP store, independently of the SMTP store. Added `_maddy_imap_accts` + `_ensure_imap_acct` helpers. 2. `tests/mail/test_droplet_exercise.py::TestDown::test_noop_when_none` called real `down(confirm=True)` while mocking only `_do_client`. `down()` then ran `(DROPLET_JSON, CREDS_JSON).unlink(missing_ok= True)` on the live state files. The pre-commit pytest hook destroyed production credentials.json + droplet.json this way. Added `tests/mail/conftest.py` with an autouse fixture that monkeypatches STATE_DIR / CREDS_JSON / DROPLET_JSON / GIT_MAILER_ENV to a tmp_path for every test in `tests/mail/`. Belt-and-suspenders: future tests in this dir can't reach the real filesystem even if they forget to patch. 3. `mail-poller` inherited a `HEALTHCHECK` from the `api` image probing `http://localhost:8000/health`, but the container runs only a polling loop with no HTTP server, so it stayed permanently `unhealthy`. Compose now overrides the inherited healthcheck. The poll loop touches `/tmp/heartbeat` after each iteration; the healthcheck verifies it's been touched within `2 * MAIL_POLL_INTERVAL` seconds. `start_period: 120s` covers the first cold poll. Existing seed/rotate tests updated to expect the extra ssh calls introduced by the imap-acct check; added one new test covering the "imap-acct already present" path.
This commit is contained in:
13
compose.yml
13
compose.yml
@@ -547,8 +547,21 @@ services:
|
|||||||
command: >
|
command: >
|
||||||
sh -c 'while true; do
|
sh -c 'while true; do
|
||||||
uv run --no-sync stack bib ingest-mail || true;
|
uv run --no-sync stack bib ingest-mail || true;
|
||||||
|
touch /tmp/heartbeat;
|
||||||
sleep $${MAIL_POLL_INTERVAL};
|
sleep $${MAIL_POLL_INTERVAL};
|
||||||
done'
|
done'
|
||||||
|
# Override the inherited HEALTHCHECK from the api image (which
|
||||||
|
# probes http://localhost:8000/health — wrong for this loop-only
|
||||||
|
# container). Mark healthy when the poll loop has completed an
|
||||||
|
# iteration within the last 2 * MAIL_POLL_INTERVAL seconds.
|
||||||
|
healthcheck:
|
||||||
|
test:
|
||||||
|
- "CMD-SHELL"
|
||||||
|
- "test -f /tmp/heartbeat && test $$(( $$(date +%s) - $$(stat -c %Y /tmp/heartbeat) )) -lt $$(( $${MAIL_POLL_INTERVAL:-600} * 2 ))"
|
||||||
|
interval: 60s
|
||||||
|
timeout: 5s
|
||||||
|
retries: 3
|
||||||
|
start_period: 120s
|
||||||
labels:
|
labels:
|
||||||
- "promtail=true"
|
- "promtail=true"
|
||||||
security_opt:
|
security_opt:
|
||||||
|
|||||||
@@ -421,6 +421,28 @@ def _addr_to_key(addr: str) -> str:
|
|||||||
return addr
|
return addr
|
||||||
|
|
||||||
|
|
||||||
|
def _maddy_imap_accts(ip: str) -> set[str]:
|
||||||
|
"""Set of existing IMAP account addresses on the droplet."""
|
||||||
|
r = _ssh(ip, "docker", "exec", "mail", "maddy", "imap-acct", "list")
|
||||||
|
return {line.strip() for line in r.stdout.splitlines() if line.strip()}
|
||||||
|
|
||||||
|
|
||||||
|
def _ensure_imap_acct(ip: str, addr: str, existing: set[str]) -> None:
|
||||||
|
"""Create the IMAP mailbox for ``addr`` if it doesn't already exist.
|
||||||
|
|
||||||
|
Maddy keeps SMTP credentials (creds) and IMAP storage (imap-acct)
|
||||||
|
in separate stores. Having one without the other means auth
|
||||||
|
succeeds but delivered mail has nowhere to land — which silently
|
||||||
|
discards e.g. DMARC aggregate reports. Always pair them.
|
||||||
|
|
||||||
|
``existing`` is mutated so callers can list once and reuse the set.
|
||||||
|
"""
|
||||||
|
if addr in existing:
|
||||||
|
return
|
||||||
|
_ssh(ip, "docker", "exec", "mail", "maddy", "imap-acct", "create", addr)
|
||||||
|
existing.add(addr)
|
||||||
|
|
||||||
|
|
||||||
def rotate_creds(user_or_addr: str) -> str:
|
def rotate_creds(user_or_addr: str) -> str:
|
||||||
"""Mint new password and push to droplet. ``user_or_addr`` may be
|
"""Mint new password and push to droplet. ``user_or_addr`` may be
|
||||||
``user`` (assumed at apex) or full ``user@host``.
|
``user`` (assumed at apex) or full ``user@host``.
|
||||||
@@ -438,12 +460,13 @@ def rotate_creds(user_or_addr: str) -> str:
|
|||||||
meta = discover_droplet(_do_client())
|
meta = discover_droplet(_do_client())
|
||||||
if not meta:
|
if not meta:
|
||||||
raise RuntimeError("no mail droplet exists")
|
raise RuntimeError("no mail droplet exists")
|
||||||
|
ip = meta["public_ip"]
|
||||||
new_pw = _gen_password()
|
new_pw = _gen_password()
|
||||||
|
|
||||||
# Try password change first (account exists). If the account
|
# Try password change first (account exists). If the account
|
||||||
# doesn't exist yet, create it + the matching IMAP account.
|
# doesn't exist yet, create it.
|
||||||
r = _ssh(
|
r = _ssh(
|
||||||
meta["public_ip"],
|
ip,
|
||||||
"docker",
|
"docker",
|
||||||
"exec",
|
"exec",
|
||||||
"mail",
|
"mail",
|
||||||
@@ -457,7 +480,7 @@ def rotate_creds(user_or_addr: str) -> str:
|
|||||||
)
|
)
|
||||||
if r.returncode != 0:
|
if r.returncode != 0:
|
||||||
_ssh(
|
_ssh(
|
||||||
meta["public_ip"],
|
ip,
|
||||||
"docker",
|
"docker",
|
||||||
"exec",
|
"exec",
|
||||||
"mail",
|
"mail",
|
||||||
@@ -468,16 +491,10 @@ def rotate_creds(user_or_addr: str) -> str:
|
|||||||
new_pw,
|
new_pw,
|
||||||
addr,
|
addr,
|
||||||
)
|
)
|
||||||
_ssh(
|
# IMAP mailbox may be missing even when SMTP creds exist (saw this
|
||||||
meta["public_ip"],
|
# in production with postmaster@mail.fhirworx.io after a partial
|
||||||
"docker",
|
# earlier provision). Ensure it independently.
|
||||||
"exec",
|
_ensure_imap_acct(ip, addr, _maddy_imap_accts(ip))
|
||||||
"mail",
|
|
||||||
"maddy",
|
|
||||||
"imap-acct",
|
|
||||||
"create",
|
|
||||||
addr,
|
|
||||||
)
|
|
||||||
creds = _load_json(CREDS_JSON)
|
creds = _load_json(CREDS_JSON)
|
||||||
# Save under both forms so lookup works whether the caller passes
|
# Save under both forms so lookup works whether the caller passes
|
||||||
# the bare key or the full address.
|
# the bare key or the full address.
|
||||||
@@ -490,24 +507,26 @@ def rotate_creds(user_or_addr: str) -> str:
|
|||||||
|
|
||||||
|
|
||||||
def seed_mailboxes(addrs: tuple[str, ...] = DEFAULT_MAILBOXES) -> None:
|
def seed_mailboxes(addrs: tuple[str, ...] = DEFAULT_MAILBOXES) -> None:
|
||||||
"""Ensure each address exists on droplet with cache-matching password.
|
"""Ensure each address exists on droplet with cache-matching password
|
||||||
|
AND has a matching IMAP mailbox.
|
||||||
|
|
||||||
Idempotent: when an address has a cached password, push it to the
|
Idempotent: when an address has a cached password, push it to the
|
||||||
droplet (no-op if matches, replace if differs, create if missing).
|
droplet (no-op if matches, replace if differs, create if missing).
|
||||||
When no cached password exists, mint and push a fresh one.
|
When no cached password exists, mint and push a fresh one. Either
|
||||||
|
way, the IMAP mailbox is verified to exist (independently of the
|
||||||
|
SMTP creds store — they can drift).
|
||||||
"""
|
"""
|
||||||
meta = discover_droplet(_do_client())
|
meta = discover_droplet(_do_client())
|
||||||
if not meta:
|
if not meta:
|
||||||
raise RuntimeError("no mail droplet exists")
|
raise RuntimeError("no mail droplet exists")
|
||||||
ip = meta["public_ip"]
|
ip = meta["public_ip"]
|
||||||
creds = _load_json(CREDS_JSON)
|
creds = _load_json(CREDS_JSON)
|
||||||
|
imap_accts = _maddy_imap_accts(ip)
|
||||||
|
|
||||||
for addr in addrs:
|
for addr in addrs:
|
||||||
key = _addr_to_key(addr)
|
key = _addr_to_key(addr)
|
||||||
pw = creds.get(key)
|
pw = creds.get(key)
|
||||||
if pw:
|
if pw:
|
||||||
# `creds password` updates an existing account; falls
|
|
||||||
# through to create+imap-acct for first-time provision.
|
|
||||||
r = _ssh(
|
r = _ssh(
|
||||||
ip,
|
ip,
|
||||||
"docker",
|
"docker",
|
||||||
@@ -522,6 +541,7 @@ def seed_mailboxes(addrs: tuple[str, ...] = DEFAULT_MAILBOXES) -> None:
|
|||||||
check=False,
|
check=False,
|
||||||
)
|
)
|
||||||
if r.returncode == 0:
|
if r.returncode == 0:
|
||||||
|
_ensure_imap_acct(ip, addr, imap_accts)
|
||||||
ok(f"mailbox {addr}: cached password in sync with droplet")
|
ok(f"mailbox {addr}: cached password in sync with droplet")
|
||||||
continue
|
continue
|
||||||
_ssh(
|
_ssh(
|
||||||
@@ -536,10 +556,11 @@ def seed_mailboxes(addrs: tuple[str, ...] = DEFAULT_MAILBOXES) -> None:
|
|||||||
pw,
|
pw,
|
||||||
addr,
|
addr,
|
||||||
)
|
)
|
||||||
_ssh(ip, "docker", "exec", "mail", "maddy", "imap-acct", "create", addr)
|
_ensure_imap_acct(ip, addr, imap_accts)
|
||||||
ok(f"mailbox {addr}: created on droplet from cached password")
|
ok(f"mailbox {addr}: created on droplet from cached password")
|
||||||
else:
|
else:
|
||||||
rotate_creds(addr)
|
rotate_creds(addr)
|
||||||
|
imap_accts.add(addr)
|
||||||
|
|
||||||
|
|
||||||
def write_git_mailer_env() -> bool:
|
def write_git_mailer_env() -> bool:
|
||||||
|
|||||||
23
tests/mail/conftest.py
Normal file
23
tests/mail/conftest.py
Normal file
@@ -0,0 +1,23 @@
|
|||||||
|
"""Isolate every test in tests/mail/ from real mail state on disk.
|
||||||
|
|
||||||
|
Without this, a test that exercises real code paths (e.g. `down()` with only
|
||||||
|
the DigitalOcean client mocked) will run `unlink(missing_ok=True)` on the
|
||||||
|
production `CREDS_JSON` and `DROPLET_JSON`. The pre-commit pytest hook
|
||||||
|
silently destroyed live mail state this way in May 2026.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture(autouse=True)
|
||||||
|
def _isolate_mail_state(tmp_path, monkeypatch):
|
||||||
|
state_dir = tmp_path / "mail"
|
||||||
|
state_dir.mkdir()
|
||||||
|
git_dir = tmp_path / "git"
|
||||||
|
git_dir.mkdir()
|
||||||
|
monkeypatch.setattr("mail.droplet.STATE_DIR", state_dir)
|
||||||
|
monkeypatch.setattr("mail.droplet.CREDS_JSON", state_dir / "credentials.json")
|
||||||
|
monkeypatch.setattr("mail.droplet.DROPLET_JSON", state_dir / "droplet.json")
|
||||||
|
monkeypatch.setattr("mail.droplet.GIT_MAILER_ENV", git_dir / "mailer.env")
|
||||||
@@ -240,10 +240,31 @@ class TestSeedMailboxes:
|
|||||||
def test_cached_sync(self, mc_save, mc_ssh, mc_load, mc_discover, mc_client):
|
def test_cached_sync(self, mc_save, mc_ssh, mc_load, mc_discover, mc_client):
|
||||||
mc_discover.return_value = {"public_ip": "1.2.3.4"}
|
mc_discover.return_value = {"public_ip": "1.2.3.4"}
|
||||||
mc_load.return_value = {"postmaster": "pw1"}
|
mc_load.return_value = {"postmaster": "pw1"}
|
||||||
mc_ssh.return_value = MagicMock(returncode=0)
|
# stdout="" so _maddy_imap_accts returns empty set; addr is then
|
||||||
|
# ensured (one extra ssh call for imap-acct create).
|
||||||
|
mc_ssh.return_value = MagicMock(returncode=0, stdout="")
|
||||||
|
|
||||||
seed_mailboxes(addrs=(f"postmaster@{DOMAIN}",))
|
seed_mailboxes(addrs=(f"postmaster@{DOMAIN}",))
|
||||||
assert mc_ssh.call_count == 1
|
# imap-acct list + creds password + imap-acct create
|
||||||
|
assert mc_ssh.call_count == 3
|
||||||
|
|
||||||
|
@patch("mail.droplet._do_client")
|
||||||
|
@patch("mail.droplet.discover_droplet")
|
||||||
|
@patch("mail.droplet._load_json")
|
||||||
|
@patch("mail.droplet._ssh")
|
||||||
|
@patch("mail.droplet._save_json")
|
||||||
|
def test_cached_sync_imap_present(
|
||||||
|
self, mc_save, mc_ssh, mc_load, mc_discover, mc_client
|
||||||
|
):
|
||||||
|
"""When IMAP acct already exists, no create-call is issued."""
|
||||||
|
mc_discover.return_value = {"public_ip": "1.2.3.4"}
|
||||||
|
mc_load.return_value = {"postmaster": "pw1"}
|
||||||
|
addr = f"postmaster@{DOMAIN}"
|
||||||
|
mc_ssh.return_value = MagicMock(returncode=0, stdout=f"{addr}\n")
|
||||||
|
|
||||||
|
seed_mailboxes(addrs=(addr,))
|
||||||
|
# imap-acct list + creds password — no create needed
|
||||||
|
assert mc_ssh.call_count == 2
|
||||||
|
|
||||||
@patch("mail.droplet._do_client")
|
@patch("mail.droplet._do_client")
|
||||||
@patch("mail.droplet.discover_droplet")
|
@patch("mail.droplet.discover_droplet")
|
||||||
@@ -255,17 +276,20 @@ class TestSeedMailboxes:
|
|||||||
):
|
):
|
||||||
mc_discover.return_value = {"public_ip": "1.2.3.4"}
|
mc_discover.return_value = {"public_ip": "1.2.3.4"}
|
||||||
mc_load.return_value = {"postmaster": "pw1"}
|
mc_load.return_value = {"postmaster": "pw1"}
|
||||||
mc_ssh.return_value = MagicMock(returncode=1)
|
mc_ssh.return_value = MagicMock(returncode=1, stdout="")
|
||||||
|
|
||||||
seed_mailboxes(addrs=(f"postmaster@{DOMAIN}",))
|
seed_mailboxes(addrs=(f"postmaster@{DOMAIN}",))
|
||||||
assert mc_ssh.call_count == 3
|
# imap-acct list + creds password (fail) + creds create + imap-acct create
|
||||||
|
assert mc_ssh.call_count == 4
|
||||||
|
|
||||||
@patch("mail.droplet._do_client")
|
@patch("mail.droplet._do_client")
|
||||||
@patch("mail.droplet.discover_droplet")
|
@patch("mail.droplet.discover_droplet")
|
||||||
@patch("mail.droplet._load_json", return_value={})
|
@patch("mail.droplet._load_json", return_value={})
|
||||||
|
@patch("mail.droplet._ssh")
|
||||||
@patch("mail.droplet.rotate_creds")
|
@patch("mail.droplet.rotate_creds")
|
||||||
def test_no_cache_rotates(self, mc_rotate, mc_load, mc_discover, mc_client):
|
def test_no_cache_rotates(self, mc_rotate, mc_ssh, mc_load, mc_discover, mc_client):
|
||||||
mc_discover.return_value = {"public_ip": "1.2.3.4"}
|
mc_discover.return_value = {"public_ip": "1.2.3.4"}
|
||||||
|
mc_ssh.return_value = MagicMock(returncode=0, stdout="")
|
||||||
seed_mailboxes(addrs=(f"postmaster@{DOMAIN}",))
|
seed_mailboxes(addrs=(f"postmaster@{DOMAIN}",))
|
||||||
mc_rotate.assert_called_once()
|
mc_rotate.assert_called_once()
|
||||||
|
|
||||||
@@ -347,10 +371,12 @@ class TestRotateCredsBody:
|
|||||||
self, mc_chmod, mc_save, mc_load, mc_ssh, mc_discover, mc_client
|
self, mc_chmod, mc_save, mc_load, mc_ssh, mc_discover, mc_client
|
||||||
):
|
):
|
||||||
mc_discover.return_value = {"public_ip": "1.2.3.4"}
|
mc_discover.return_value = {"public_ip": "1.2.3.4"}
|
||||||
mc_ssh.return_value = MagicMock(returncode=0)
|
# creds password ok + imap-acct list (empty stdout → set()) +
|
||||||
|
# imap-acct create.
|
||||||
|
mc_ssh.return_value = MagicMock(returncode=0, stdout="")
|
||||||
pw = rotate_creds("git")
|
pw = rotate_creds("git")
|
||||||
assert len(pw) == 24
|
assert len(pw) == 24
|
||||||
assert mc_ssh.call_count == 1
|
assert mc_ssh.call_count == 3
|
||||||
|
|
||||||
@patch("mail.droplet._do_client")
|
@patch("mail.droplet._do_client")
|
||||||
@patch("mail.droplet.discover_droplet")
|
@patch("mail.droplet.discover_droplet")
|
||||||
@@ -362,14 +388,17 @@ class TestRotateCredsBody:
|
|||||||
self, mc_chmod, mc_save, mc_load, mc_ssh, mc_discover, mc_client
|
self, mc_chmod, mc_save, mc_load, mc_ssh, mc_discover, mc_client
|
||||||
):
|
):
|
||||||
mc_discover.return_value = {"public_ip": "1.2.3.4"}
|
mc_discover.return_value = {"public_ip": "1.2.3.4"}
|
||||||
|
# creds password (fail) + creds create + imap-acct list (empty)
|
||||||
|
# + imap-acct create.
|
||||||
mc_ssh.side_effect = [
|
mc_ssh.side_effect = [
|
||||||
MagicMock(returncode=1),
|
MagicMock(returncode=1, stdout=""),
|
||||||
MagicMock(returncode=0),
|
MagicMock(returncode=0, stdout=""),
|
||||||
MagicMock(returncode=0),
|
MagicMock(returncode=0, stdout=""),
|
||||||
|
MagicMock(returncode=0, stdout=""),
|
||||||
]
|
]
|
||||||
pw = rotate_creds("user@sub.example.com")
|
pw = rotate_creds("user@sub.example.com")
|
||||||
assert len(pw) == 24
|
assert len(pw) == 24
|
||||||
assert mc_ssh.call_count == 3
|
assert mc_ssh.call_count == 4
|
||||||
|
|
||||||
|
|
||||||
class TestProvision:
|
class TestProvision:
|
||||||
|
|||||||
Reference in New Issue
Block a user