diff options
Diffstat (limited to 'packages/meshbay-hub')
| -rw-r--r-- | packages/meshbay-hub/src/meshbay_hub/tasks/cleanup.py | 125 | ||||
| -rw-r--r-- | packages/meshbay-hub/tests/test_cleanup_foreign_keys.py | 103 |
2 files changed, 193 insertions, 35 deletions
diff --git a/packages/meshbay-hub/src/meshbay_hub/tasks/cleanup.py b/packages/meshbay-hub/src/meshbay_hub/tasks/cleanup.py index b0ef4d4..559d872 100644 --- a/packages/meshbay-hub/src/meshbay_hub/tasks/cleanup.py +++ b/packages/meshbay-hub/src/meshbay_hub/tasks/cleanup.py @@ -4,7 +4,7 @@ import asyncio import logging from datetime import UTC, datetime, timedelta -from sqlalchemy import delete, select +from sqlalchemy import delete, select, update from sqlalchemy.ext.asyncio import AsyncSession from meshbay_hub.db.models import EmailVerification, Group, GroupInviteLink, IPLog, User @@ -35,9 +35,49 @@ async def purge_expired_verifications(db: AsyncSession) -> int: async def purge_stale_pending_users(db: AsyncSession, expiry_days: int = PENDING_USER_EXPIRY_DAYS) -> int: + """Delete accounts that never verified their address, and what points at them. + + A pending account is not childless: registering writes an `account_create` + IP-log row, a failed sign-in another, and a group owner may have added it to + a group. A bare `DELETE FROM users` therefore violates those foreign keys — + which PostgreSQL enforces and the SQLite the suite runs on does not — so on + the real hub it raised, the stale account stayed, and every later run + raised again. + + The IP log is kept and detached, keeping the name, exactly as + `erase_account` does for a deleted account: it is the legal record. Every + other nullable reference is cleared and every other row deleted, found from + the schema so a table added later is covered. An account that owns a group + is left alone — a pending account cannot create one, so that would be a + state this function did not expect and should not guess about. + """ + from meshbay_hub.db.models import Base, Group + cutoff = datetime.now(UTC) - timedelta(days=expiry_days) - result = await db.execute( - delete(User).where(User.status == "pending", User.created_at < cutoff)) + stale = (await db.execute( + select(User.id, User.username).where( + User.status == "pending", User.created_at < cutoff, + ~User.id.in_(select(Group.admin_id))))).all() + if not stale: + return 0 + ids = [uid for uid, _ in stale] + + for uid, name in stale: + await db.execute(update(IPLog).where(IPLog.user_id == uid) + .values(username=name, user_id=None)) + for table in Base.metadata.sorted_tables: + if table.name in (User.__tablename__, IPLog.__tablename__): + continue + for fk in table.foreign_keys: + if fk.column.table.name != User.__tablename__: + continue + column = fk.parent + if column.nullable: + await db.execute(update(table).where(column.in_(ids)) + .values({column.name: None})) + else: + await db.execute(delete(table).where(column.in_(ids))) + result = await db.execute(delete(User).where(User.id.in_(ids))) await db.commit() return result.rowcount @@ -53,42 +93,57 @@ async def purge_invite_links(db: AsyncSession) -> int: return result.rowcount +async def _purge_login_throttle(db: AsyncSession) -> int: + from meshbay_hub import login_throttle + return await login_throttle.purge_expired(db) + + +async def _purge_mail_quota(db: AsyncSession) -> int: + from meshbay_hub import mail + return await mail.purge_expired_quota(db) + + +# In order, each in its own session and its own try: one step that raises must +# not cost the others their run. They used to share one `try`, so the day +# `purge_stale_pending_users` first hit a foreign key on PostgreSQL, the mail +# counters, the sign-in counters and the invitation links behind it stopped +# being purged at all — silently, since the loop logged one line and slept. +_STEPS = ( + ("IP log entries older than the retention period", purge_old_ip_logs), + ("expired email verifications", purge_expired_verifications), + ("stale pending users", purge_stale_pending_users), + # One row per recipient the hub has written to, and the window is a day: + # without this the table grows for the life of the instance. + ("expired mail counters", _purge_mail_quota), + # Every name anybody types at the sign-in form is a row, real or not. + ("expired sign-in counters", _purge_login_throttle), + ("spent or expired invitation links", purge_invite_links), +) + + +async def run_cleanup(get_session) -> dict[str, int | None]: + """One pass of every step. A step that failed reports None.""" + done: dict[str, int | None] = {} + for what, step in _STEPS: + try: + async with get_session() as db: + n = await step(db) + done[what] = n + if n: + log.info("Purged %d %s", n, what) + except asyncio.CancelledError: + raise + except Exception as e: + done[what] = None + log.error("Cleanup of %s failed: %s", what, e) + return done + + async def cleanup_loop(get_session): """Run cleanup once at startup, then every 24 hours.""" try: while True: - try: - async with get_session() as db: - deleted = await purge_old_ip_logs(db) - if deleted: - log.info("Purged %d IP log entries older than %d days", - deleted, RETENTION_DAYS) - expired = await purge_expired_verifications(db) - if expired: - log.info("Purged %d expired email verifications", expired) - stale = await purge_stale_pending_users(db) - if stale: - log.info("Purged %d stale pending users", stale) - # One row per recipient the hub has written to, and the - # window is a day: without this the table grows for the - # life of the instance and nothing ever reads the old rows. - from meshbay_hub import mail - quota = await mail.purge_expired_quota(db) - if quota: - log.info("Purged %d expired mail counters", quota) - # Every name anybody types at the sign-in form is a row, - # real or not; once its window has passed, nothing reads it. - from meshbay_hub import login_throttle - throttled = await login_throttle.purge_expired(db) - if throttled: - log.info("Purged %d expired sign-in counters", throttled) - links = await purge_invite_links(db) - if links: - log.info("Purged %d spent or expired invitation links", links) - except asyncio.CancelledError: - raise - except Exception as e: - log.error("IP log cleanup failed: %s", e) + await run_cleanup(get_session) await asyncio.sleep(CLEANUP_INTERVAL_HOURS * 3600) except asyncio.CancelledError: return diff --git a/packages/meshbay-hub/tests/test_cleanup_foreign_keys.py b/packages/meshbay-hub/tests/test_cleanup_foreign_keys.py new file mode 100644 index 0000000..b4994b2 --- /dev/null +++ b/packages/meshbay-hub/tests/test_cleanup_foreign_keys.py @@ -0,0 +1,103 @@ +""" +The daily cleanup, against a database that enforces foreign keys. + +PostgreSQL always does; the SQLite the rest of the suite runs on does not unless +asked. So `DELETE FROM users` for an account that still had an IP-log row passed +every test here and raised on the real hub — and because every step shared one +`try`, the purges behind it (mail counters, sign-in counters, invitation links) +stopped running for good. Both halves are held here, on an engine with +`PRAGMA foreign_keys=ON`. +""" + +from datetime import UTC, datetime, timedelta + +import pytest +from meshbay_hub.db.models import ( + Base, + EmailVerification, + Group, + GroupMember, + IPLog, + Notification, + User, +) +from meshbay_hub.tasks import cleanup +from sqlalchemy import event, select +from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine + + +@pytest.fixture +async def strict_db(tmp_path): + engine = create_async_engine(f"sqlite+aiosqlite:///{tmp_path / 'hub.db'}") + + @event.listens_for(engine.sync_engine, "connect") + def _enforce(dbapi_conn, _record): + dbapi_conn.execute("PRAGMA foreign_keys=ON") + + async with engine.begin() as conn: + await conn.run_sync(Base.metadata.create_all) + yield async_sessionmaker(engine, expire_on_commit=False) + await engine.dispose() + + +def _user(name, status, age_days): + return User(username=name, email="x", pw_hash=b"x", pw_salt=b"x", hub_id="h", + status=status, + created_at=datetime.now(UTC) - timedelta(days=age_days)) + + +async def test_a_stale_pending_account_is_purged_with_what_points_at_it(strict_db): + async with strict_db() as db: + owner = _user("groupowner", "active", 30) + stale = _user("neververified", "pending", 8) + db.add_all([owner, stale]) + await db.flush() + group = Group(name="g", admin_id=owner.id) + db.add(group) + await db.flush() + db.add_all([ + IPLog(user_id=stale.id, event="account_create", ip_address="192.0.2.1"), + GroupMember(group_id=group.id, user_id=stale.id), + Notification(user_id=stale.id, kind="group_invite", title="t"), + EmailVerification(email_hash="h", code="1", purpose="registration", + user_id=stale.id, + expires_at=datetime.now(UTC) - timedelta(days=6)), + ]) + await db.commit() + stale_id = stale.id + + async with strict_db() as db: + assert await cleanup.purge_stale_pending_users(db) == 1 + + async with strict_db() as db: + assert await db.get(User, stale_id) is None + log_row = (await db.execute(select(IPLog))).scalar_one() + # The legal record survives, still saying who it was about. + assert log_row.user_id is None and log_row.username == "neververified" + assert (await db.execute(select(GroupMember).where( + GroupMember.user_id == stale_id))).first() is None + + +async def test_a_recent_or_active_account_is_left_alone(strict_db): + async with strict_db() as db: + db.add_all([_user("stillpending", "pending", 2), + _user("activeuser", "active", 30)]) + await db.commit() + async with strict_db() as db: + assert await cleanup.purge_stale_pending_users(db) == 0 + + +async def test_one_failing_step_does_not_stop_the_others(strict_db, monkeypatch): + ran = [] + + async def broken(db): + raise RuntimeError("boom") + + async def later(db): + ran.append("later") + return 0 + + monkeypatch.setattr(cleanup, "_STEPS", (("broken", broken), ("later", later))) + done = await cleanup.run_cleanup(strict_db) + assert done == {"broken": None, "later": 0} + assert ran == ["later"] |