diff --git a/docs/plans/20260515-1700-node-associate-foreign-key-fix/plan.md b/docs/plans/20260515-1700-node-associate-foreign-key-fix/plan.md new file mode 100644 index 0000000..6ba9db2 --- /dev/null +++ b/docs/plans/20260515-1700-node-associate-foreign-key-fix/plan.md @@ -0,0 +1,195 @@ +# Node Association Foreign Key Fix + +**Date:** 2026-05-15 +**Status:** Reviewed +**Severity:** High (500 errors on `/api/v1/user/profiles`, broken Nodes/Members pages) + +## Problem + +The node retention policy (`NODE_CLEANUP`) deletes `Node` rows, but orphaned rows remain in dependent tables (`user_profile_nodes`, `event_observers`, `node_tags`). When the API loads user profiles with their adopted nodes via `selectinload`, the `assoc.node` relationship resolves to `None` because the referenced node no longer exists. This causes an `AttributeError` crash: + +``` +AttributeError: 'NoneType' object has no attribute 'public_key' + at api/routes/user_profiles.py:96 — public_key=assoc.node.public_key +``` + +The same issue exists for any code that accesses `.node` on `EventObserver` or `NodeTag` after their referenced node has been deleted. + +## Root Cause + +The `cleanup_inactive_nodes()` function in `collector/cleanup.py` deletes `Node` rows directly via `DELETE FROM nodes WHERE last_seen < cutoff`. Three dependent tables define `ForeignKey("nodes.id", ondelete="CASCADE")`: + +- `user_profile_nodes` (`user_profile_node.py:39`) +- `event_observers` (`event_observer.py:57`) +- `node_tags` (`node_tag.py:32`) + +However, **the cascade never executes** because: + +1. The **async SQLAlchemy engine** (`create_async_engine` in `database.py:115`) does **not** have the `PRAGMA foreign_keys=ON` listener that the sync engine has (`database.py:41-47`). +2. SQLite does not enforce foreign keys or cascades by default. The `PRAGMA` must be enabled per-connection. +3. The collector's cleanup runs via the async engine, so SQLite silently ignores the `ondelete="CASCADE"` constraints, leaving orphaned rows pointing to deleted `nodes.id` values. + +## Affected Code + +| File | Lines | Issue | +|------|-------|-------| +| `common/database.py` | 115 | Async engine created without SQLite FK pragma listener | +| `api/routes/user_profiles.py` | 34-42, 93-101 | `assoc.node.public_key` crashes when `assoc.node is None` | +| `collector/cleanup.py` | 166-225 | Node cleanup doesn't cascade to dependent tables | +| `collector/cli.py` | 517-601 | CLI `cleanup` command only runs event data cleanup, not node cleanup | +| `collector/cli.py` | 537-545 | Docstring says "Node records are never deleted" — misleading once orphan cleanup is added | +| `collector/cli.py` | 717-724 | `truncate` cascade warning omits `user_profile_nodes` and `event_observers` | +| `tests/test_collector/conftest.py` | 35 | Async test fixture has no FK pragma — can't verify cascade | +| `tests/test_api/conftest.py` | 49 | Sync test engine has no FK pragma — can't verify cascade | +| `tests/test_collector/test_cleanup.py` | — | Missing test for `cleanup_inactive_nodes` entirely | + +## Plan + +### Step 1: Fix async SQLite FK enforcement + +**File:** `src/meshcore_hub/common/database.py` + +Add the `PRAGMA foreign_keys=ON` event listener to `self.async_engine` (the `create_async_engine` call), mirroring the existing listener on the sync engine. Use the existing module-scope `event` import — do NOT re-import it. + +```python +# After creating self.async_engine (at database.py:115): +if database_url.startswith("sqlite"): + @event.listens_for(self.async_engine.sync_engine, "connect") + def set_sqlite_pragma_async(dbapi_connection, connection_record): + cursor = dbapi_connection.cursor() + cursor.execute("PRAGMA foreign_keys=ON") + cursor.close() +``` + +This fixes the root cause for all future cleanup operations across all three dependent tables. + +### Step 2: Add null-safety to API routes + +**File:** `src/meshcore_hub/api/routes/user_profiles.py` + +Make `_build_adopted_nodes()` resilient to orphaned associations: + +1. In `_build_adopted_nodes()` (line 34): Skip associations where `assoc.node is None`, log a warning with `profile.id` and `assoc.node_id` for diagnostics. +2. Remove the duplicate inline adoption loop in `list_profiles()` (lines 93-101); refactor it to call `_build_adopted_nodes()` instead. + +This prevents the 500 error even if orphaned rows exist in the database (e.g., from past cleanup runs before this fix). All three callers (`get_my_profile`, `get_profile`, `list_profiles`) are protected. + +### Step 3: Add orphan cleanup function + +**File:** `src/meshcore_hub/collector/cleanup.py` + +Add a new function `cleanup_orphaned_node_relations(db, dry_run=False) -> dict` that: + +1. Finds orphaned rows in all three dependent tables (`user_profile_nodes`, `event_observers`, `node_tags`) where `node_id` does not exist in the `nodes` table (using `LEFT JOIN ... WHERE nodes.id IS NULL` pattern). +2. Deletes those orphaned rows (or counts them in dry-run mode). +3. Returns a dict with per-table counts, e.g. `{"user_profile_nodes": 3, "event_observers": 0, "node_tags": 5}`. + +This repairs existing production databases that already have broken data from past cleanup runs. + +### Step 4: Integrate orphan cleanup into retention cycle + +**File:** `src/meshcore_hub/collector/subscriber.py` + +In the `run_cleanup()` async function within `_start_cleanup_scheduler()` (around line 303): + +1. Remove the inline `async def run_cleanup()` and extract it as a proper method `_run_scheduled_cleanup(db_session)` on the subscriber class. +2. After `cleanup_inactive_nodes()`, call `cleanup_orphaned_node_relations()`. +3. Log the orphan cleanup results. + +This ensures orphans are cleaned up automatically in future runs as a belt-and-suspenders defense even after the PRAGMA fix. + +### Step 5: Wire cleanup into CLI + +**File:** `src/meshcore_hub/collector/cli.py` + +1. Add a `--node-cleanup` option (default: `false`) to the `cleanup` CLI command. When set, also runs `cleanup_inactive_nodes()` and `cleanup_orphaned_node_relations()`. +2. Update the docstring to remove "Node records are never deleted" and document the new option. +3. Add a `--node-cleanup-days` option (default: `30`) for the node inactivity threshold. +4. Display orphan cleanup results in the output. + +### Step 6: Fix test fixtures with FK PRAGMA + +**File:** `tests/test_collector/conftest.py` + +Add `PRAGMA foreign_keys=ON` event listener to the async engine created at line 35. + +**File:** `tests/test_api/conftest.py` + +Add `PRAGMA foreign_keys=ON` event listener to the sync engine created at line 49. + +### Step 7: Fix `truncate` CLI cascade warning + +**File:** `src/meshcore_hub/collector/cli.py` (lines 717-724) + +Add `user_profile_nodes` and `event_observers` to the cascade warning list. + +### Step 8: Tests + +- **`tests/test_collector/test_cleanup.py`**: + - Add test for `cleanup_inactive_nodes()` — create a node with associations in all three dependent tables, delete the node, verify cascade removes all dependent rows. + - Add test for `cleanup_orphaned_node_relations()` — create orphaned rows in all three tables (rows referencing non-existent node_ids), verify the function deletes them. + +- **`tests/test_api/test_user_profiles.py`**: + - Add test verifying `list_profiles` returns 200 (not 500) when orphaned `UserProfileNode` rows exist with a deleted node, and that the orphaned association is excluded from `adopted_nodes` output. + +### Step 9: Documentation + +- **`docs/upgrading.md`**: Add a note about this fix, the automatic orphan cleanup, and a manual repair command for existing deployments. +- **`AGENTS.md`**: No changes needed (no new env vars or config). +- **`SCHEMAS.md`**: No changes needed (FK relationships unchanged). + +## Execution Order + +1. Fix async FK pragma (Step 1) — prevents future orphans +2. Fix test fixtures with FK pragma (Step 6) — unblocks testability +3. Add null-safety (Step 2) — stops the 500 errors immediately +4. Add orphan cleanup function (Step 3) — provides the repair tool +5. Integrate into retention cycle (Step 4) — makes it automatic +6. Wire into CLI (Step 5) — exposes manual control +7. Fix truncate warning (Step 7) +8. Add tests (Step 8) +9. Update docs (Step 9) + +## Risk Assessment + +- **Step 1 (PRAGMA fix):** Low risk. Enables a constraint that was always intended to be active. All `ondelete` clauses use either `CASCADE` or `SET NULL`, which are safe operations. +- **Step 2 (Null-safety):** Zero risk. Purely defensive — only affects display, changes no data. +- **Step 3-4 (Orphan cleanup):** Low risk. Deletes rows that reference non-existent nodes, which are broken data by definition. +- **Step 5 (CLI):** Low risk. The `--node-cleanup` flag defaults to `false` (opt-in), preserving backward compatibility. +- **Step 6 (Test fix):** Zero risk. Only affects test infrastructure. + +## Verification + +After deploying: + +1. Check the API returns 200 for `/api/v1/user/profiles`: + ```bash + curl -s -o /dev/null -w "%{http_code}" http://localhost:8000/api/v1/user/profiles + ``` +2. Run manual cleanup to repair existing orphans: + ```bash + meshcore-hub collector cleanup --dry-run --node-cleanup + meshcore-hub collector cleanup --node-cleanup + ``` +3. Confirm no orphaned rows remain across all three tables: + ```sql + SELECT 'user_profile_nodes' AS tbl, COUNT(*) FROM user_profile_nodes upn + LEFT JOIN nodes n ON n.id = upn.node_id WHERE n.id IS NULL + UNION ALL + SELECT 'event_observers', COUNT(*) FROM event_observers eo + LEFT JOIN nodes n ON n.id = eo.observer_node_id WHERE n.id IS NULL + UNION ALL + SELECT 'node_tags', COUNT(*) FROM node_tags nt + LEFT JOIN nodes n ON n.id = nt.node_id WHERE n.id IS NULL; + ``` + Expected: 0 for all three. + +## Resolved Review Issues + +| Issue | Resolution | +|-------|-----------| +| Orphan cleanup scope (UserProfileNode only vs. all 3 tables) | Cover all 3 tables: `user_profile_nodes`, `event_observers`, `node_tags` | +| Test fixtures lack FK pragma | Add `PRAGMA foreign_keys=ON` to both collector and API test fixtures | +| CLI `cleanup` doesn't run node cleanup | Add `--node-cleanup` option (default off, opt-in) | +| Step 1 code uses shadow-import `from sqlalchemy import event as ...` | Reuse existing module-scope `event` import | +| Missing test for `cleanup_inactive_nodes` | Add test with cascade verification | diff --git a/docs/plans/20260515-1700-node-associate-foreign-key-fix/tasks.md b/docs/plans/20260515-1700-node-associate-foreign-key-fix/tasks.md new file mode 100644 index 0000000..fe3eb51 --- /dev/null +++ b/docs/plans/20260515-1700-node-associate-foreign-key-fix/tasks.md @@ -0,0 +1,50 @@ +# Tasks — Node Association Foreign Key Fix + +Reference: [plan.md](./plan.md) + +## Phase 1: Root Cause Fix + Defensive Hardening + +- [ ] **T1.1** — Add `PRAGMA foreign_keys=ON` event listener to `self.async_engine.sync_engine` in `src/meshcore_hub/common/database.py` (after line 115). Reuse existing module-scope `event` import. Guard with `if database_url.startswith("sqlite"):`. +- [ ] **T1.2** — Add `PRAGMA foreign_keys=ON` event listener to async engine in `tests/test_collector/conftest.py:35`. +- [ ] **T1.3** — Add `PRAGMA foreign_keys=ON` event listener to sync engine in `tests/test_api/conftest.py:49`. +- [ ] **T1.4** — In `src/meshcore_hub/api/routes/user_profiles.py`, add null-guard in `_build_adopted_nodes()` (line 34): skip associations where `assoc.node is None`, log a warning with `assoc.node_id`. +- [ ] **T1.5** — In `src/meshcore_hub/api/routes/user_profiles.py`, refactor the duplicate inline adoption loop in `list_profiles()` (lines 92–101) to call `_build_adopted_nodes()` instead. + +## Phase 2: Orphan Cleanup Function + +- [ ] **T2.1** — Add `cleanup_orphaned_node_relations(db: AsyncSession, dry_run: bool = False) -> dict[str, int]` to `src/meshcore_hub/collector/cleanup.py`. Must cover all three tables (`user_profile_nodes.node_id`, `event_observers.observer_node_id`, `node_tags.node_id`) using `LEFT JOIN nodes ... WHERE nodes.id IS NULL` pattern. Return per-table counts. + +## Phase 3: Integration + +- [ ] **T3.1** — In `src/meshcore_hub/collector/subscriber.py`, add `cleanup_orphaned_node_relations()` call inside the `run_cleanup()` inline function (after `cleanup_inactive_nodes()` at line ~323). Gate it behind the same `if self._node_cleanup_enabled:` block. Log the orphan results. +- [ ] **T3.2** — In `src/meshcore_hub/collector/cli.py`, add `--node-cleanup` flag (default `false`) and `--node-cleanup-days` option (default `30`) to the `cleanup` CLI command. When `--node-cleanup` is set, run `cleanup_inactive_nodes()` and `cleanup_orphaned_node_relations()`. Display results. +- [ ] **T3.3** — Update the `cleanup` command docstring (`cli.py:537–545`): remove "Node records are never deleted", document `--node-cleanup` and `--node-cleanup-days`. +- [ ] **T3.4** — In `src/meshcore_hub/collector/cli.py`, add `user_profile_nodes` and `event_observers` to the truncate cascade warning (lines 717–724). + +## Phase 4: Tests + +- [ ] **T4.1** — Add test for `cleanup_inactive_nodes()` in `tests/test_collector/test_cleanup.py`: create a node with rows in all three dependent tables (`user_profile_nodes`, `event_observers`, `node_tags`), run cleanup, assert cascade deleted all dependent rows. +- [ ] **T4.2** — Add test for `cleanup_orphaned_node_relations()` in `tests/test_collector/test_cleanup.py`: create orphaned rows in all three tables referencing non-existent `node_id`s, run function, assert all orphans deleted and per-table counts correct. +- [ ] **T4.3** — Add test in `tests/test_api/test_user_profiles.py`: create a profile with an adopted node, delete the node (leaving an orphaned `UserProfileNode`), call `GET /profiles`, assert 200 response and orphaned node excluded from `adopted_nodes`. + +## Phase 5: Documentation + +- [ ] **T5.1** — Add upgrade note to `docs/upgrading.md`: describe the async FK pragma fix, automatic orphan cleanup in the retention cycle, and the manual repair command (`meshcore-hub collector cleanup --node-cleanup`). + +## Verification + +After all tasks complete: + +```bash +# Lint and typecheck +source .venv/bin/activate +pre-commit run --all-files + +# Targeted tests +pytest tests/test_collector/test_cleanup.py -v +pytest tests/test_api/test_user_profiles.py -v +pytest tests/test_common/ -v + +# Full suite (only if changes span components) +pytest +``` diff --git a/docs/upgrading.md b/docs/upgrading.md index 15ca588..1cc21b0 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -2,6 +2,33 @@ This guide covers upgrading from a previous MeshCore Hub release to the current version. Check the relevant version section below before upgrading. +## v0.11.0 + +### Async SQLite Foreign Key Fix + +The async SQLAlchemy engine now enables `PRAGMA foreign_keys=ON` for SQLite databases, matching the behavior of the sync engine. Previously, cascade deletes (`ondelete="CASCADE"`) were silently ignored when the collector deleted inactive nodes via the async engine, leaving orphaned rows in `user_profile_nodes`, `event_observers`, and `node_tags`. + +**This is an automatic fix** — no configuration changes are required. The orphaned rows that may have accumulated in existing databases can be cleaned up with: + +```bash +# Dry run to preview +meshcore-hub collector cleanup --node-cleanup --dry-run + +# Live cleanup +meshcore-hub collector cleanup --node-cleanup +``` + +The collector's scheduled cleanup cycle now also runs orphan cleanup automatically after node deletion when `NODE_CLEANUP_ENABLED=true`. + +### CLI Changes + +The `meshcore-hub collector cleanup` command now accepts: + +| Flag | Default | Description | +|------|---------|-------------| +| `--node-cleanup` | `false` | Also delete inactive nodes and orphaned relations | +| `--node-cleanup-days` | `30` | Inactivity threshold for node deletion | + ## v0.10.0 This release introduces OIDC authentication, user profiles with node adoption, removes the Members system, replaces `role=infra` tags with adoption-based infrastructure detection, and replaces the admin tag editor with an inline editor on the node detail page. diff --git a/src/meshcore_hub/api/routes/user_profiles.py b/src/meshcore_hub/api/routes/user_profiles.py index 75d1c5a..b418ef6 100644 --- a/src/meshcore_hub/api/routes/user_profiles.py +++ b/src/meshcore_hub/api/routes/user_profiles.py @@ -32,6 +32,13 @@ def _build_adopted_nodes(profile: UserProfile) -> list[AdoptedNodeRead]: """Extract adopted node list from a profile's eager-loaded associations.""" adopted_nodes = [] for assoc in profile.node_associations: + if assoc.node is None: + logger.warning( + "Orphaned UserProfileNode detected: profile=%s, node_id=%s", + profile.id, + assoc.node_id, + ) + continue adopted_nodes.append( AdoptedNodeRead( public_key=assoc.node.public_key, @@ -89,16 +96,6 @@ async def list_profiles( items = [] for profile in profiles: - adopted_nodes = [] - for assoc in profile.node_associations: - adopted_nodes.append( - AdoptedNodeRead( - public_key=assoc.node.public_key, - name=assoc.node.name, - adv_type=assoc.node.adv_type, - adopted_at=assoc.adopted_at, - ) - ) items.append( UserProfileListItem( id=profile.id, @@ -108,7 +105,7 @@ async def list_profiles( description=profile.description, url=profile.url, node_count=len(profile.node_associations), - adopted_nodes=adopted_nodes, + adopted_nodes=_build_adopted_nodes(profile), ) ) diff --git a/src/meshcore_hub/collector/cleanup.py b/src/meshcore_hub/collector/cleanup.py index e05ac41..c0ffe73 100644 --- a/src/meshcore_hub/collector/cleanup.py +++ b/src/meshcore_hub/collector/cleanup.py @@ -13,11 +13,14 @@ from sqlalchemy.ext.asyncio import AsyncSession from meshcore_hub.common.models import ( Advertisement, EventLog, + EventObserver, Message, Node, + NodeTag, Telemetry, TracePath, ) +from meshcore_hub.common.models.user_profile_node import UserProfileNode logger = logging.getLogger(__name__) @@ -223,3 +226,53 @@ async def cleanup_inactive_nodes( cutoff_date.isoformat(), ) return count + + +async def cleanup_orphaned_node_relations( + db: AsyncSession, + dry_run: bool = False, +) -> dict[str, int]: + """Delete orphaned rows in node-dependent tables. + + Finds and deletes rows in user_profile_nodes, event_observers, + and node_tags that reference node_id values not present in the + nodes table. + + Args: + db: Database session + dry_run: If True, only count records without deleting + + Returns: + Dict mapping table names to deletion counts + """ + counts: dict[str, int] = {} + tables = [ + ("user_profile_nodes", UserProfileNode, UserProfileNode.node_id), + ("event_observers", EventObserver, EventObserver.observer_node_id), + ("node_tags", NodeTag, NodeTag.node_id), + ] + + for table_name, model, fk_column in tables: + subq = select(Node.id).where(Node.id == fk_column) + if dry_run: + count_stmt = select(func.count()).select_from(model).where(~subq.exists()) + result = await db.execute(count_stmt) + count = result.scalar() or 0 + else: + del_stmt = delete(model).where(~subq.exists()) + result = await db.execute(del_stmt) + count = result.rowcount or 0 # type: ignore[attr-defined] + + counts[table_name] = count + if count > 0: + logger.info( + "%s: %s %d orphaned rows", + table_name, + "would delete" if dry_run else "deleted", + count, + ) + + if not dry_run: + await db.commit() + + return counts diff --git a/src/meshcore_hub/collector/cli.py b/src/meshcore_hub/collector/cli.py index a7ec581..7a4eb7b 100644 --- a/src/meshcore_hub/collector/cli.py +++ b/src/meshcore_hub/collector/cli.py @@ -522,6 +522,18 @@ def import_tags_cmd( envvar="DATA_RETENTION_DAYS", help="Number of days to retain data (default: 30)", ) +@click.option( + "--node-cleanup", + is_flag=True, + default=False, + help="Also delete inactive nodes and orphaned relations", +) +@click.option( + "--node-cleanup-days", + type=int, + default=30, + help="Delete nodes not seen for this many days (default: 30)", +) @click.option( "--dry-run", is_flag=True, @@ -532,6 +544,8 @@ def import_tags_cmd( def cleanup_cmd( ctx: click.Context, retention_days: int, + node_cleanup: bool, + node_cleanup_days: int, dry_run: bool, ) -> None: """Manually run data cleanup to delete old events. @@ -543,7 +557,9 @@ def cleanup_cmd( - Trace paths - Event logs - Node records are never deleted - only event data. + Use --node-cleanup to also delete inactive nodes (not seen for + --node-cleanup-days) and any orphaned rows in user_profile_nodes, + event_observers, and node_tags that reference deleted nodes. Use --dry-run to preview what would be deleted without actually deleting anything. @@ -568,7 +584,11 @@ def cleanup_cmd( click.echo("") from meshcore_hub.common.database import DatabaseManager - from meshcore_hub.collector.cleanup import cleanup_old_data + from meshcore_hub.collector.cleanup import ( + cleanup_old_data, + cleanup_inactive_nodes, + cleanup_orphaned_node_relations, + ) # Initialize database db = DatabaseManager(ctx.obj["database_url"]) @@ -591,6 +611,29 @@ def cleanup_cmd( click.echo(f" Event logs: {stats.event_logs_deleted}") click.echo(f" Total: {stats.total_deleted}") + if node_cleanup: + click.echo("") + nodes_deleted = await cleanup_inactive_nodes( + session, + node_cleanup_days, + dry_run=dry_run, + ) + mode = "would be" if dry_run else "were" + click.echo( + f" Inactive nodes {mode} deleted: {nodes_deleted}" + f" (older than {node_cleanup_days} days)" + ) + + orphan_counts = await cleanup_orphaned_node_relations( + session, + dry_run=dry_run, + ) + if any(orphan_counts.values()): + click.echo(" Orphaned relations:") + for table_name, count in orphan_counts.items(): + if count > 0: + click.echo(f" {table_name}: {count}") + if dry_run: click.echo("") click.echo("(Dry run - no data was actually deleted)") @@ -718,6 +761,8 @@ def truncate_cmd( "WARNING: Clearing nodes will also clear all related data due to foreign keys:" ) click.echo(" - node_tags") + click.echo(" - user_profile_nodes") + click.echo(" - event_observers") click.echo(" - advertisements") click.echo(" - messages") click.echo(" - telemetry") diff --git a/src/meshcore_hub/collector/subscriber.py b/src/meshcore_hub/collector/subscriber.py index 5b5c08b..5d13a92 100644 --- a/src/meshcore_hub/collector/subscriber.py +++ b/src/meshcore_hub/collector/subscriber.py @@ -294,6 +294,7 @@ class Subscriber(LetsMeshNormalizer): from meshcore_hub.collector.cleanup import ( cleanup_old_data, cleanup_inactive_nodes, + cleanup_orphaned_node_relations, ) # Get async session and run cleanup @@ -322,6 +323,18 @@ class Subscriber(LetsMeshNormalizer): nodes_deleted, ) + orphan_counts = ( + await cleanup_orphaned_node_relations( + session, + dry_run=False, + ) + ) + if any(orphan_counts.values()): + logger.info( + "Orphan cleanup completed: %s", + orphan_counts, + ) + loop.run_until_complete(run_cleanup()) self._last_cleanup = now diff --git a/src/meshcore_hub/common/database.py b/src/meshcore_hub/common/database.py index 903145c..17b72ae 100644 --- a/src/meshcore_hub/common/database.py +++ b/src/meshcore_hub/common/database.py @@ -113,6 +113,18 @@ class DatabaseManager: # Create async engine for async operations async_url = database_url.replace("sqlite://", "sqlite+aiosqlite://") self.async_engine = create_async_engine(async_url, echo=echo) + + # Enable foreign keys for async SQLite engine + if database_url.startswith("sqlite"): + + @event.listens_for(self.async_engine.sync_engine, "connect") + def set_sqlite_pragma_async( + dbapi_connection: object, connection_record: object + ) -> None: + cursor = dbapi_connection.cursor() # type: ignore[attr-defined] + cursor.execute("PRAGMA foreign_keys=ON") + cursor.close() + from sqlalchemy.ext.asyncio import async_sessionmaker self.async_session_factory = async_sessionmaker( diff --git a/tests/test_api/conftest.py b/tests/test_api/conftest.py index 63879e7..0e85fcc 100644 --- a/tests/test_api/conftest.py +++ b/tests/test_api/conftest.py @@ -8,7 +8,7 @@ from unittest.mock import MagicMock, patch import pytest from fastapi.testclient import TestClient -from sqlalchemy import create_engine +from sqlalchemy import create_engine, event as sa_event from sqlalchemy.orm import sessionmaker from meshcore_hub.api.app import create_app @@ -50,6 +50,13 @@ def api_db_engine(test_db_path): db_url, connect_args={"check_same_thread": False}, ) + + @sa_event.listens_for(engine, "connect") + def set_sqlite_pragma(dbapi_connection: object, connection_record: object) -> None: + cursor = dbapi_connection.cursor() # type: ignore[attr-defined] + cursor.execute("PRAGMA foreign_keys=ON") + cursor.close() + Base.metadata.create_all(engine) yield engine Base.metadata.drop_all(engine) diff --git a/tests/test_api/test_user_profiles.py b/tests/test_api/test_user_profiles.py index e69e764..da0cb69 100644 --- a/tests/test_api/test_user_profiles.py +++ b/tests/test_api/test_user_profiles.py @@ -5,6 +5,7 @@ from unittest.mock import patch import pytest from meshcore_hub.common.models import UserProfile +from meshcore_hub.common.models.user_profile_node import UserProfileNode TEST_USER_ID = "oidc-user-123" OTHER_USER_ID = "oidc-user-456" @@ -81,6 +82,36 @@ class TestListProfiles: assert len(profile["adopted_nodes"]) == 1 assert profile["adopted_nodes"][0]["name"] == sample_node.name + def test_list_profiles_resilient_to_orphaned_adoption( + self, client_no_auth, api_db_session, sample_user_profile, sample_node + ): + """Test that orphaned UserProfileNode rows don't cause 500 errors.""" + from sqlalchemy import text + + adoption = UserProfileNode( + user_profile_id=sample_user_profile.id, + node_id=sample_node.id, + ) + api_db_session.add(adoption) + api_db_session.commit() + + api_db_session.execute(text("PRAGMA foreign_keys=OFF")) + api_db_session.execute( + text("DELETE FROM nodes WHERE id = :id"), + {"id": sample_node.id}, + ) + api_db_session.commit() + api_db_session.execute(text("PRAGMA foreign_keys=ON")) + + response = client_no_auth.get( + "/api/v1/user/profiles", + headers=USER_HEADERS, + ) + assert response.status_code == 200 + data = response.json() + profile = next(p for p in data["items"] if p["id"] == sample_user_profile.id) + assert profile["adopted_nodes"] == [] + class TestGetMyProfile: """Tests for GET /user/profile/me endpoint.""" diff --git a/tests/test_collector/conftest.py b/tests/test_collector/conftest.py index c49ea91..aa64012 100644 --- a/tests/test_collector/conftest.py +++ b/tests/test_collector/conftest.py @@ -1,6 +1,7 @@ """Fixtures for collector component tests.""" import pytest +from sqlalchemy import event as sa_event from sqlalchemy.ext.asyncio import AsyncSession, create_async_engine from sqlalchemy.ext.asyncio import async_sessionmaker @@ -34,6 +35,14 @@ async def async_db_session(): # Create async engine with in-memory database engine = create_async_engine("sqlite+aiosqlite:///:memory:") + @sa_event.listens_for(engine.sync_engine, "connect") + def set_sqlite_pragma_async( + dbapi_connection: object, connection_record: object + ) -> None: + cursor = dbapi_connection.cursor() # type: ignore[attr-defined] + cursor.execute("PRAGMA foreign_keys=ON") + cursor.close() + # Create tables async with engine.begin() as conn: await conn.run_sync(Base.metadata.create_all) diff --git a/tests/test_collector/test_cleanup.py b/tests/test_collector/test_cleanup.py index 11bc508..9ba1b3e 100644 --- a/tests/test_collector/test_cleanup.py +++ b/tests/test_collector/test_cleanup.py @@ -2,17 +2,27 @@ import pytest from datetime import datetime, timedelta, timezone +from sqlalchemy import func, select from sqlalchemy.ext.asyncio import AsyncSession -from meshcore_hub.collector.cleanup import cleanup_old_data, CleanupStats +from meshcore_hub.collector.cleanup import ( + cleanup_inactive_nodes, + cleanup_old_data, + cleanup_orphaned_node_relations, + CleanupStats, +) from meshcore_hub.common.models import ( Advertisement, EventLog, + EventObserver, Message, Node, + NodeTag, Telemetry, TracePath, ) +from meshcore_hub.common.models.user_profile_node import UserProfileNode +from meshcore_hub.common.models.user_profile import UserProfile @pytest.mark.asyncio @@ -248,3 +258,162 @@ async def test_cleanup_stats_repr() -> None: assert "total=21" in repr_str assert "advertisements=10" in repr_str assert "messages=5" in repr_str + + +@pytest.mark.asyncio +async def test_cleanup_inactive_nodes_cascades(async_db_session: AsyncSession) -> None: + """Test that deleting inactive nodes cascades to dependent tables.""" + old_date = datetime.now(timezone.utc) - timedelta(days=60) + + node = Node( + public_key="a" * 64, + name="Stale Node", + last_seen=old_date, + ) + async_db_session.add(node) + await async_db_session.flush() + + profile = UserProfile(user_id="cascade-test-user", name="Test") + async_db_session.add(profile) + await async_db_session.flush() + + adoption = UserProfileNode( + user_profile_id=profile.id, + node_id=node.id, + ) + async_db_session.add(adoption) + + tag = NodeTag(node_id=node.id, key="role", value="gateway") + async_db_session.add(tag) + + observer = EventObserver( + event_type="message", + event_hash="abc123def456abc123def456abc12345", + observer_node_id=node.id, + ) + async_db_session.add(observer) + + await async_db_session.commit() + + deleted = await cleanup_inactive_nodes( + async_db_session, inactivity_days=30, dry_run=False + ) + assert deleted == 1 + + assert await async_db_session.scalar(select(func.count()).select_from(Node)) == 0 + assert ( + await async_db_session.scalar(select(func.count()).select_from(UserProfileNode)) + == 0 + ) + assert await async_db_session.scalar(select(func.count()).select_from(NodeTag)) == 0 + assert ( + await async_db_session.scalar(select(func.count()).select_from(EventObserver)) + == 0 + ) + + assert ( + await async_db_session.scalar(select(func.count()).select_from(UserProfile)) + == 1 + ) + + +@pytest.mark.asyncio +async def test_cleanup_orphaned_node_relations( + async_db_session: AsyncSession, +) -> None: + """Test orphan cleanup deletes rows referencing non-existent nodes.""" + from sqlalchemy import text + + node = Node( + public_key="o" * 64, + name="Temporary Node", + ) + async_db_session.add(node) + await async_db_session.flush() + + profile = UserProfile(user_id="orphan-test-user", name="Test") + async_db_session.add(profile) + await async_db_session.flush() + + adoption = UserProfileNode( + user_profile_id=profile.id, + node_id=node.id, + ) + async_db_session.add(adoption) + + tag = NodeTag(node_id=node.id, key="role", value="gateway") + async_db_session.add(tag) + + observer = EventObserver( + event_type="message", + event_hash="abc123def456abc123def456abc12399", + observer_node_id=node.id, + ) + async_db_session.add(observer) + + await async_db_session.commit() + + await async_db_session.execute(text("PRAGMA foreign_keys=OFF")) + await async_db_session.execute( + text("DELETE FROM nodes WHERE id = :id"), {"id": node.id} + ) + await async_db_session.commit() + await async_db_session.execute(text("PRAGMA foreign_keys=ON")) + + counts = await cleanup_orphaned_node_relations(async_db_session, dry_run=False) + + assert counts["user_profile_nodes"] == 1 + assert counts["event_observers"] == 1 + assert counts["node_tags"] == 1 + + assert ( + await async_db_session.scalar(select(func.count()).select_from(UserProfileNode)) + == 0 + ) + assert await async_db_session.scalar(select(func.count()).select_from(NodeTag)) == 0 + assert ( + await async_db_session.scalar(select(func.count()).select_from(EventObserver)) + == 0 + ) + + +@pytest.mark.asyncio +async def test_cleanup_orphaned_node_relations_dry_run( + async_db_session: AsyncSession, +) -> None: + """Test orphan dry-run counts but does not delete.""" + from sqlalchemy import text + + node = Node( + public_key="p" * 64, + name="Dry Run Node", + ) + async_db_session.add(node) + await async_db_session.flush() + + profile = UserProfile(user_id="orphan-dryrun-user", name="Test") + async_db_session.add(profile) + await async_db_session.flush() + + adoption = UserProfileNode( + user_profile_id=profile.id, + node_id=node.id, + ) + async_db_session.add(adoption) + await async_db_session.commit() + + await async_db_session.execute(text("PRAGMA foreign_keys=OFF")) + await async_db_session.execute( + text("DELETE FROM nodes WHERE id = :id"), {"id": node.id} + ) + await async_db_session.commit() + await async_db_session.execute(text("PRAGMA foreign_keys=ON")) + + counts = await cleanup_orphaned_node_relations(async_db_session, dry_run=True) + + assert counts["user_profile_nodes"] == 1 + + assert ( + await async_db_session.scalar(select(func.count()).select_from(UserProfileNode)) + == 1 + )