diff --git a/.env.example b/.env.example index d4edc8b..4501953 100644 --- a/.env.example +++ b/.env.example @@ -387,21 +387,20 @@ PROMETHEUS_PORT=9090 # ------------------- # Redis Cache # ------------------- -# Optional Redis cache for API response caching. +# Required Redis cache for API response caching. # Reduces database load for read-heavy endpoints (nodes, messages, dashboard). -# When disabled or unavailable, the API queries the database directly. +# A Redis outage makes /health/ready report not_ready (503) and cached +# endpoints return 503 — Redis is infrastructure, not an optional accelerator. # -# Docker: Redis is included in the "cache" profile (--profile cache). -# REDIS_ENABLED defaults to false everywhere. To enable, set REDIS_ENABLED=true -# and start with --profile cache (or point REDIS_HOST at an external Redis). -# Bare-metal: Install Redis separately and set REDIS_ENABLED=true. +# Docker: the bundled Redis joins the "core" profile via docker-compose.dev.yml +# (zero-effort dev). Production (docker-compose.prod.yml) does NOT start it — +# point REDIS_* at your shared/external instance, or start the bundled one +# with --profile cache (or --profile all). +# Bare-metal: install and run a Redis server locally (localhost defaults). # # For multi-instance setups sharing one Redis, use different REDIS_KEY_PREFIX # values per instance (e.g., hub for prod, hub-stg for staging). -# Enable Redis caching (default: false outside Docker, true in Docker Compose) -# REDIS_ENABLED=false - # Redis server host (use "redis" in Docker Compose) # REDIS_HOST=localhost diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0385911..9870ad5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -97,6 +97,16 @@ jobs: --health-interval 5s --health-timeout 5s --health-retries 10 + redis: + # Pinned to match the bundled redis:8-alpine in docker-compose.yml + image: redis:8 + ports: + - 6379:6379 + options: >- + --health-cmd "redis-cli ping" + --health-interval 5s + --health-timeout 5s + --health-retries 10 steps: - uses: actions/checkout@v7 @@ -114,6 +124,7 @@ jobs: - name: Run tests with pytest env: TEST_POSTGRES_URL: postgresql+psycopg2://meshcorehub:meshcorehub-test@localhost:5432/meshcorehub_test + TEST_REDIS_URL: redis://localhost:6379/0 run: | pytest -nauto --cov=meshcore_hub --cov-report=xml --cov-report=term-missing --junitxml=junit.xml -o junit_family=legacy diff --git a/AGENTS.md b/AGENTS.md index a397e31..7380ec8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -29,7 +29,7 @@ This venv is only for local testing, linting, and migration authoring. Frontend ## Development ```bash -# Build / start / stop the stack (core = collector + api + web + migrate) +# Build / start / stop the stack (core = collector + api + web + migrate + bundled postgres/redis via the dev override) docker compose -f docker-compose.yml -f docker-compose.dev.yml --profile core build docker compose -f docker-compose.yml -f docker-compose.dev.yml --profile core up -d docker compose -f docker-compose.yml -f docker-compose.dev.yml --profile core down @@ -78,7 +78,7 @@ npm run test:frontend # host: vitest unit + component tests ## Tests & Quality -Coverage is **opt-in**; add `--cov=meshcore_hub` (or `make test-cov`) when you want it. The dev loop defaults to no coverage and parallel across CPU cores. Backend tests require PostgreSQL: `make test` / `make test-cov` / `make test-unit` start and stop the throwaway test Postgres automatically (a `postgres:17-alpine` on `127.0.0.1:55432`; the management is skipped when `TEST_POSTGRES_URL` points at your own instance). For direct `pytest` runs, `make test-db-up` first — pytest fails fast with a hint when the database is unreachable. +Coverage is **opt-in**; add `--cov=meshcore_hub` (or `make test-cov`) when you want it. The dev loop defaults to no coverage and parallel across CPU cores. Backend tests require PostgreSQL **and Redis** (both mandatory infrastructure): `make test` / `make test-cov` / `make test-unit` start and stop the throwaway test stack automatically (a `postgres:17-alpine` on `127.0.0.1:55432` and a `redis:8-alpine` on `127.0.0.1:55433`; the management is skipped when `TEST_POSTGRES_URL` points at your own instance). For direct `pytest` runs, `make test-db-up` first — pytest fails fast with a hint when either is unreachable (`TEST_REDIS_URL` overrides the Redis target). ```bash # Canonical: run tests in parallel, no coverage, surface the pass/fail summary @@ -142,7 +142,7 @@ Design notes when extending the suite: ## Database & Ops -The database backend is **PostgreSQL** (the only backend since v0.19). The bundled container is dev-only: it joins the `core` compose profile via `docker-compose.dev.yml` (profiles union across override files), so the documented dev commands start it automatically — production (`docker-compose.prod.yml`) never starts it with `core`/`migrate`, so it cannot shadow a shared `postgres` host on the same network. See `docs/database.md` for the full reference, production provisioning, and schema-per-instance setup. Backend tests also require Postgres: `make test-db-up` starts a throwaway instance on `127.0.0.1:55432` (or point `TEST_POSTGRES_URL` at your own). +The database backend is **PostgreSQL** (the only backend since v0.19) and the cache backend is **Redis** (mandatory since v0.20 — same compose-profile pattern: the bundled containers join the `core` profile only via `docker-compose.dev.yml`, so the documented dev commands start them automatically; production (`docker-compose.prod.yml`) never starts them with `core`, so they cannot shadow shared `postgres`/`redis` hosts on the same network). See `docs/database.md` for the full database reference and `docs/deployment.md` → Redis Caching for the Redis one. Backend tests require both: `make test-db-up` starts throwaway instances on `127.0.0.1:55432` (Postgres) and `127.0.0.1:55433` (Redis) — or point `TEST_POSTGRES_URL` / `TEST_REDIS_URL` at your own. ```bash # --- LOCAL (venv): author a migration against a local schema (NOT the dev DB) @@ -168,7 +168,7 @@ docker compose -f docker-compose.yml -f docker-compose.dev.yml --profile core ex ### Cache invalidation on writes -Every mutation handler (POST/PUT/DELETE) on a user/admin-mutable entity MUST call the matching `invalidate_*` helper from `meshcore_hub.api.cache_invalidation` after `session.commit()` succeeds, so the UI reflects the change on the next page load instead of waiting for the Redis TTL. The helper is a no-op when Redis is disabled and swallows backend errors, so it's always safe to call. +Every mutation handler (POST/PUT/DELETE) on a user/admin-mutable entity MUST call the matching `invalidate_*` helper from `meshcore_hub.api.cache_invalidation` after `session.commit()` succeeds, so the UI reflects the change on the next page load instead of waiting for the Redis TTL. The helper never raises (backend errors log at ERROR after a committed write), so it's always safe to call. Redis is a mandatory backend — only read/store paths hard-fail (503) during an outage. The HTTP-layer cache policy on `/api/v1/*` GETs is `private, no-cache` (i.e. must-revalidate) precisely so this works: the browser always sends `If-None-Match` on navigation, the server answers 304 when Redis is warm and unchanged (cheap — no body) or 200 after an invalidation. Do NOT change this back to `max-age>0` — server-side cache invalidation cannot reach the browser's HTTP cache, so any freshness window would let stale responses survive a mutation until expiry. diff --git a/Makefile b/Makefile index bb2cf18..a77c7c0 100644 --- a/Makefile +++ b/Makefile @@ -36,11 +36,11 @@ restore: alpine sh -c "cd / && tar xzf /backup/$$(basename $(FILE))" # --- Tests --------------------------------------------------------------- -# Backend tests run against PostgreSQL only: `test`, `test-cov`, and -# `test-unit` start the throwaway test DB automatically and stop it again -# afterwards — even when pytest fails (EXIT trap). Set TEST_POSTGRES_URL to -# point at your own instance and the automatic up/down is skipped. -# For direct pytest runs, start it manually with `make test-db-up`. +# Backend tests run against PostgreSQL and Redis: `test`, `test-cov`, and +# `test-unit` start the throwaway test stack automatically and stop it again +# afterwards — even when pytest fails (EXIT trap). Set TEST_POSTGRES_URL / +# TEST_REDIS_URL to point at your own instances and the automatic up/down is +# skipped. For direct pytest runs, start it manually with `make test-db-up`. # Coverage is opt-in (use test-cov). Dev loop runs in parallel across cores. # `test` runs the backend suite then the frontend (vitest) suite. test: @@ -71,10 +71,11 @@ test-unit: test-frontend: npm run test:frontend -# Throwaway PostgreSQL for the backend suite (127.0.0.1:55432, dev-only creds, -# ephemeral tmpfs storage). pytest fails fast with a hint if it is unreachable. -# The project name is pinned so a COMPOSE_PROJECT_NAME set in .env (the dev -# stack) can never make `down` target the dev services. +# Throwaway PostgreSQL (127.0.0.1:55432) + Redis (127.0.0.1:55433) for the +# backend suite (dev-only creds, ephemeral tmpfs storage). Redis is required +# cache infrastructure — pytest fails fast with a hint if either is +# unreachable. The project name is pinned so a COMPOSE_PROJECT_NAME set in +# .env (the dev stack) can never make `down` target the dev services. TEST_DB_COMPOSE = docker compose -f docker-compose.test-db.yml -p meshcore-hub-test-db test-db-up: diff --git a/README.md b/README.md index 652d320..878d36a 100644 --- a/README.md +++ b/README.md @@ -108,9 +108,9 @@ Service profiles: | Profile | Services | Use Case | | ---------- | --------------------------------------------------------------- | ----------------------------------------- | | `all` | postgres, redis, mqtt, observer, migrate, collector, api, web | Everything on one host | -| `core` | postgres (dev override only), migrate, collector, api, web | Central server infrastructure | +| `core` | postgres, redis (dev override only), migrate, collector, api, web | Central server infrastructure | | `mqtt` | meshcore-mqtt-broker | Local MQTT broker (optional) | -| `cache` | redis | Local Redis cache (optional) | +| `cache` | redis | Bundled Redis, standalone opt-in | | `postgres` | postgres | Bundled database, standalone opt-in | | `observer` | packet capture observer | Observes RF traffic and publishes to MQTT | | `seed` | seed | One-time seed data import | @@ -120,6 +120,8 @@ Service profiles: **Database note:** the bundled `postgres` container joins the `core` profile only when using `docker-compose.dev.yml` (profiles merge across override files), so the zero-config dev workflow is unchanged. Production (`docker-compose.prod.yml`) deliberately leaves it out — the bundled container's `postgres` network alias would shadow a shared instance of the same name on the same Docker network. Point the `DATABASE_*` variables at your shared/external instance, or opt in explicitly with `--profile postgres` / `--profile all`. See [docs/database.md](docs/database.md). +**Redis note:** Redis is required infrastructure for the API, and the bundled `redis` container follows the same profile pattern as Postgres: it joins `core` only via `docker-compose.dev.yml` (dev starts it automatically), while `docker-compose.prod.yml` leaves it out so it cannot shadow a shared `redis` host on the same network. Point the `REDIS_*` variables at your shared/external instance in production, or opt into the bundled one with `--profile cache` / `--profile all`. + ### Simple Self-Hosted Setup The quickest way to get started is running the entire stack on a single machine with a connected LoRa radio. diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index 028c317..5690c8c 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -48,6 +48,13 @@ services: seed: pull_policy: build + # Bundled dev Redis: same 'core' profile-union as postgres — Redis is + # required infrastructure, so the documented dev workflow (`--profile + # core`, `make up`) starts it for zero-effort setup. Production uses + # docker-compose.prod.yml, which leaves 'core' out so the bundled + # container never shadows a shared instance named 'redis' on the network. redis: + profiles: + - core ports: - "${REDIS_PORT:-6379}:6379" diff --git a/docker-compose.prod.yml b/docker-compose.prod.yml index 84c3e27..7d27e89 100644 --- a/docker-compose.prod.yml +++ b/docker-compose.prod.yml @@ -10,6 +10,9 @@ # starting the bundled container alongside it would shadow a 'postgres' host # on the same network. Opt in explicitly with `--profile postgres` (or # `--profile all`) if you really want the bundled database on this host. +# The same applies to the bundled 'redis' service (a required dependency of +# the API): point REDIS_* at your shared/external instance, or opt into the +# bundled one explicitly with `--profile cache` (or `--profile all`). # # Prerequisites: # docker network create proxy-net diff --git a/docker-compose.test-db.yml b/docker-compose.test-db.yml index f8d652a..d6b6b6f 100644 --- a/docker-compose.test-db.yml +++ b/docker-compose.test-db.yml @@ -1,5 +1,5 @@ # ========================================================================== -# Throwaway PostgreSQL instance for the backend test suite (pytest). +# Throwaway PostgreSQL + Redis instances for the backend test suite (pytest). # # NOT part of the application stack. Start/stop with: # make test-db-up @@ -11,7 +11,8 @@ # The meshcorehub role owns the meshcorehub_test database (image entrypoint), # which is exactly the privilege the per-xdist-worker test schemas need # (CREATE SCHEMA) — no superuser and no CREATEDB required. The data directory -# is a tmpfs mount, so nothing survives `down`. +# is a tmpfs mount, so nothing survives `down`. Redis (required cache +# infrastructure) is published on 55433 with the same tmpfs treatment. # ========================================================================== name: meshcore-hub-test-db @@ -36,3 +37,16 @@ services: interval: 5s timeout: 5s retries: 10 + + test-redis: + image: redis:8-alpine + container_name: meshcore-hub-test-redis + ports: + - "127.0.0.1:55433:6379" + tmpfs: + - /data + healthcheck: + test: ["CMD", "redis-cli", "ping"] + interval: 5s + timeout: 5s + retries: 10 diff --git a/docker-compose.yml b/docker-compose.yml index e8e70f2..d31c642 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -127,9 +127,13 @@ services: - observer_data:/app/data # ========================================================================== - # Redis Cache - Optional shared cache for API response caching - # Use --profile cache to start with the bundled Redis, or point - # REDIS_HOST at an external Redis instance for multi-instance setups. + # Redis Cache - required cache for API responses (the only cache backend) + # Bundled development Redis: docker-compose.dev.yml unions 'core' into + # these profiles so a default dev `compose up` is zero-effort. Production + # (prod.yml) does NOT — point REDIS_* at your shared/external instance + # instead; starting the bundled container alongside it would shadow a + # 'redis' host on the same network. Start it standalone with the explicit + # 'cache' profile (or 'all') only when the bundled Redis is really wanted. # ========================================================================== redis: image: redis:8-alpine @@ -291,6 +295,11 @@ services: condition: service_completed_successfully collector: condition: service_started + # Healthy when the bundled redis is profile-active (dev override); + # skipped when it is not (production external Redis). + redis: + condition: service_healthy + required: false volumes: - data:/data environment: @@ -321,8 +330,7 @@ services: - METRICS_CACHE_TTL=${METRICS_CACHE_TTL:-60} # Allow unauthenticated /metrics when no API_READ_KEY is set (default: deny) - METRICS_PUBLIC=${METRICS_PUBLIC:-false} - # Redis cache (optional — API works without Redis) - - REDIS_ENABLED=${REDIS_ENABLED:-false} + # Redis cache (required infrastructure — bundled service or external) - REDIS_HOST=redis - REDIS_PORT=6379 - REDIS_PASSWORD=${REDIS_PASSWORD:-} diff --git a/docs/configuration.md b/docs/configuration.md index 6f520c4..ad09fdd 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -50,11 +50,10 @@ The bundled `postgres` container derives its `POSTGRES_USER` / `POSTGRES_PASSWOR ## Caching -Optional Redis-backed caching for API responses. When disabled or unavailable, the API queries the database directly. Operational guidance — Docker `cache` profile, bare-metal Redis, and multi-instance key-prefix isolation — is in [deployment.md → Redis Caching](deployment.md#redis-caching). +Redis-backed caching for API responses — **required infrastructure** (since v0.20). An unreachable Redis makes `/health/ready` report `not_ready` (503) and cached endpoints return `503 cache backend unavailable`. Operational guidance — Docker profiles, bare-metal Redis, and multi-instance key-prefix isolation — is in [deployment.md → Redis Caching](deployment.md#redis-caching). | Variable | Default | Description | | --- | --- | --- | -| `REDIS_ENABLED` | `false` | Enable Redis API response caching | | `REDIS_HOST` | `localhost` (`redis` in Docker) | Redis server host | | `REDIS_PORT` | `6379` | Redis server port | | `REDIS_DB` | `0` | Redis database number | diff --git a/docs/deployment.md b/docs/deployment.md index 4b6921b..1409ac4 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -93,7 +93,7 @@ The API is read-mostly and holds no per-process state — the response cache liv API_WORKERS=4 ``` -Each worker is an independent process sharing one listening socket, so the kernel balances connections across them and CPU-bound work (JSON serialisation, validation) spreads over multiple cores. Workers read their configuration from **environment variables** (CLI flags are not propagated to forked workers), which is how Docker Compose already supplies config. Enabling Redis (`REDIS_ENABLED=true`) is recommended so all workers share one cache. +Each worker is an independent process sharing one listening socket, so the kernel balances connections across them and CPU-bound work (JSON serialisation, validation) spreads over multiple cores. Workers read their configuration from **environment variables** (CLI flags are not propagated to forked workers), which is how Docker Compose already supplies config. Redis (required infrastructure) gives all workers one shared response cache. Pick a worker count around the number of CPU cores available to the container; start with `2`–`4` and measure under realistic load. Workers all talk to the same PostgreSQL database (the bundled container or an external instance — see [database.md](database.md)), so scaling extends across hosts unchanged. @@ -101,16 +101,13 @@ Pick a worker count around the number of CPU cores available to the container; s ## Redis Caching -Optional Redis-backed caching for API responses. When disabled or unavailable, the API queries the database directly. +Redis-backed caching for API responses — **required infrastructure** (since v0.20). An unreachable Redis fails readiness (`/health/ready` → 503) and cached endpoints return `503 cache backend unavailable`; it is not an optional accelerator. -**Docker:** Redis is included in the `cache` profile. Disabled by default — set `REDIS_ENABLED=true` to enable. +**Docker (dev):** `docker-compose.dev.yml` unions the bundled `redis` service into the `core` profile, so the standard dev workflow starts it automatically. Start it standalone with the explicit `cache` profile (or `all`). -```bash -docker compose --profile cache up # Start with bundled Redis -docker compose --profile core up # Start without Redis -``` +**Docker (production):** `docker-compose.prod.yml` deliberately does **not** start the bundled Redis with `core` — point `REDIS_HOST` / `REDIS_PORT` / `REDIS_PASSWORD` at your shared/external instance (starting the bundled container alongside it would shadow a `redis` host on the same network), or opt in explicitly with `--profile cache` / `--profile all`. -**Bare-metal:** Install Redis separately, then set `REDIS_ENABLED=true` and `REDIS_HOST=localhost`. +**Bare-metal:** Install and run a Redis server (the `localhost` defaults point at a local `redis-server`). **Multi-instance:** Use different `REDIS_KEY_PREFIX` values per instance to share one Redis without key collisions. diff --git a/docs/plans/20260830-2057-mandatory-redis/plan.md b/docs/plans/20260830-2057-mandatory-redis/plan.md new file mode 100644 index 0000000..55d20b3 --- /dev/null +++ b/docs/plans/20260830-2057-mandatory-redis/plan.md @@ -0,0 +1,375 @@ +# Make Redis Mandatory (PostgreSQL-parity) + +## Summary + +Redis becomes a required infrastructure dependency for MeshCore Hub, mirroring the +pattern used to make PostgreSQL the only database backend in v0.19. Every +conditional caching code path is removed: the `REDIS_ENABLED` flag (settings, CLI, +`create_app` parameter, compose env), the `NullCache` no-op backend, the +`cache is None` short-circuits in the `@cached` decorator, and the `redis_enabled` +branches in the application lifespan and `/health/ready` endpoint. + +Operational semantics change to "mandatory": a Redis outage is a hard failure. +`/health/ready` returns 503 `not_ready` when Redis is unreachable (same as the +database check), and cached endpoints translate Redis errors into +`HTTPException(503)` instead of silently degrading to direct database queries. +Production environments that run Redis separately are supported through the same +compose-profile-union trick used for Postgres — the bundled `redis` service joins +the `core` profile only via `docker-compose.dev.yml`, while `docker-compose.prod.yml` +leaves it out so `REDIS_HOST`/`REDIS_PASSWORD` can point at a shared/external +instance without host shadowing. + +User-confirmed design decisions (from planning discussion): + +1. **Readiness**: Redis unreachable → `/health/ready` returns 503 `not_ready`. +2. **Outage behavior**: hard-fail — cache GET/SET errors propagate out of `@cached` + as 503 responses ("every cached endpoint fails during a Redis outage"), not + soft-degrade. +3. **Test strategy**: real Redis in the test infra (full Postgres parity) — a + throwaway `test-redis` container next to `test-postgres`, a CI service + container, and per-xdist-worker key prefixes with between-test flushes. +4. **Invalidation semantics**: `invalidate_*` helpers never raise (ERROR log) + after a committed write; only read/store paths hard-fail (confirmed at plan + review). + +## Background & Motivation + +Redis was introduced as an entirely optional response cache +(`docs/plans/20260609-2106-redis-api-cache/plan.md`): `REDIS_ENABLED` defaults to +false, a `NullCache` stands in when disabled, the bundled compose service lives +behind the opt-in `cache` profile, and `RedisCacheBackend` swallows every +operational error (warn-and-degrade). This leaves three parallel code paths +(enabled-real, disabled-null, enabled-but-unreachable-degraded) that every feature +touching the cache must reason about, and it makes "cache invalidation on writes" +(see AGENTS.md → Cache invalidation) best-effort in all deployments. + +Since then the project made PostgreSQL the only database backend +(commit `81dcd3d`, plan `docs/plans/20260829-2312-remove-sqlite-support/`) and +kept the bundled container out of the production `core` profile +(commit `5a8794f fix(compose): keep bundled postgres out of the core profile in +production`). That change established the pattern this plan reuses: + +- dev: `docker-compose.dev.yml` unions `core` into the base service profiles + (profiles merge across compose files) so a default dev `compose up` is + zero-effort; +- prod: `docker-compose.prod.yml` does not, so the bundled container never starts + to shadow a shared host on the same network — operators point `REDIS_*` + variables at their external instance; +- wiring: dependent services use + `depends_on: {condition: service_healthy, required: false}` so the dependency + is enforced when the bundled service is profile-active and skipped when it is + not (see `migrate` → `postgres`). + +Redis is already an unconditional install dependency +(`redis[hiredis]>=5.0.0` in `pyproject.toml`), so this plan only removes runtime +optionalism. + +## Goals + +- Remove every conditional caching code path (`REDIS_ENABLED`, `NullCache`, + `cache is None`, `redis_enabled` branches) — exactly one code path remains. +- Make a Redis outage loud: `/health/ready` → 503 `not_ready`; cached endpoints → + 503 on Redis errors during request handling. +- Support production environments running Redis separately (external/shared + instances) via the Postgres compose pattern; dev `core` stack starts Redis + automatically. +- Stand up real Redis for the backend test suite (throwaway container + CI + service), replacing today's accidental cache bypass (TestClient never runs the + lifespan, so `app.state.redis_cache` is unset in most tests). +- Update all operator-facing docs (`.env.example`, configuration/deployment/README/ + upgrading/AGENTS.md) to describe Redis as required infrastructure. + +## Non-Goals + +- No migration to `redis.asyncio` (the sync client stays, matching the sync + SQLAlchemy session pattern; noted as future work in the original cache plan). +- No changes to cache key layouts, TTL semantics, the `private, no-cache` + Cache-Control policy, or ETag/If-None-Match handling. +- No Redis usage beyond the API response cache (collector and web tiers keep + their current architecture; the collector does not cache). +- No changes to the metrics in-memory TTL cache (`api/metrics.py`). +- No `fakeredis` dependency — tests use a real Redis container. +- No changes to `REDIS_HOST`/`REDIS_PORT`/`REDIS_DB`/`REDIS_PASSWORD`/ + `REDIS_KEY_PREFIX`/`REDIS_CACHE_TTL*` variable names or defaults. + +## Requirements + +### Functional Requirements + +- FR1: Removing `REDIS_ENABLED` from environment/CLI/`create_app()` has no + effect — the app always configures a `RedisCacheBackend` from the remaining + `REDIS_*` settings. +- FR2: `/health/ready` always pings Redis and reports + `"redis": "connected"` or `"redis": "unreachable"`; unreachable ⇒ overall + `"status": "not_ready"` ⇒ HTTP 503 (same contract as the database check). +- FR3: A Redis error during cache lookup or store inside `@cached` produces a + 503 response (`HTTPException`, detail e.g. "cache backend unavailable") — not + a silent cache miss, not a raw 500. +- FR4: Cache invalidation helpers (`invalidate_*`) never raise: they run after + `session.commit()` succeeded, so raising would report failure for a durable + write and invite duplicate client retries. Backend errors there log at ERROR. + (Confirmed at plan review — reads/stores hard-fail, invalidation never + raises.) +- FR5: `docker compose --profile core up` with the dev override starts Redis and + the API waits for it to be healthy before starting. +- FR6: Production (`docker-compose.prod.yml`, `core` profile) does not start the + bundled Redis; pointing `REDIS_HOST` at an external instance is the documented + production setup. The `cache` profile remains for explicitly starting the + bundled Redis standalone. +- FR7: The backend test suite runs against a real Redis; the e2e stack includes + a Redis service and the e2e API container connects to it. + +### Technical Requirements + +- TR1: `common/redis.py`: delete `NullCache`; remove try/except from + `get`/`set`/`delete` so `redis.RedisError` propagates; keep `ping() -> bool` + (swallowing there is correct — it is a health probe interface). +- TR2: `api/cache.py`: both async and sync wrappers read + `request.app.state.redis_cache` without a None branch; `_lookup`/`_store` lose + their warn-and-degrade exception handling; Redis errors are caught at the + wrapper boundary and re-raised as `HTTPException(503)`. +- TR3: `api/app.py`: drop the `redis_enabled` parameter and `app.state` field; + lifespan always constructs `RedisCacheBackend(host, port, db, password, + key_prefix)` from `app.state` and pings at startup (log connected / error); + `create_app_from_env()` stops passing `redis_enabled`; update the + `health_ready` docstring ("database and optional Redis" → mandatory Redis). +- TR4: `api/cache_invalidation.py`: `_cache()` returns the non-Optional backend; + remove the "skipped (no backend)" branch; update module/function docstrings + that describe disabled-mode behaviour. +- TR5: `api/cli.py`: remove `--redis-enabled/--no-redis` and the corresponding + parameter; startup info output always prints the Redis target. +- TR6: `common/config.py` (`APISettings`): remove the `redis_enabled` field. +- TR7: Compose: `docker-compose.yml` api service drops `REDIS_ENABLED` (and the + "optional — API works without Redis" env comment) and gains + `depends_on: redis: {condition: service_healthy, required: false}` alongside + its existing migrate/collector dependencies; + `docker-compose.dev.yml` adds `redis: profiles: [core]` (keeping the port + mapping); `docker-compose.prod.yml` header comment documents external Redis; + `e2e/docker-compose.test.yml` adds a `redis:8-alpine` service (healthcheck) + and `REDIS_HOST=redis` + `depends_on` on the api service. The e2e api + currently sets no `REDIS_*` env at all, so `REDIS_HOST=redis` is mandatory + there (code default `localhost` would fail). Its healthcheck stays the basic + `/health` — the 503 readiness change cannot deadlock stack startup. +- TR8: Test infra: `docker-compose.test-db.yml` gains `test-redis` + (redis:8-alpine, published on `127.0.0.1:55433`, tmpfs, healthcheck — the + Makefile's `test-db-up` runs `up -d --wait` on the whole file, so no target + changes are needed and `--wait` requires the healthcheck); + `tests/conftest.py` gains a session fixture resolving `TEST_REDIS_URL` + (default `redis://localhost:55433/0`) with a ping fail-fast hint mirroring + the Postgres one (`pytest.exit(..., returncode=4)` in `db_url`); + `tests/test_api/conftest.py` module app fixtures wire a real + `RedisCacheBackend` with a per-xdist-worker key prefix (mirroring `db_schema` + naming, e.g. `hub_test_gw0`) plus a function-scoped autouse flush-by-prefix + fixture (the DB is truncated between tests — the cache must be too, or + modules see stale HITs). The flush fixture must save/restore + `app.state.redis_cache` around each test, because some tests swap in their + own `MagicMock`/`_FakeCache` on the shared module-scoped app + (`test_routes.py`, `test_dashboard.py`) and must not permanently displace the + real backend. +- TR9: CI (`.github/workflows/ci.yml`): add a `redis:8` service container and + `TEST_REDIS_URL` env to the backend-test job (Postgres service-container + parity; pinned like `postgres:17` is). +- TR10: No changes to `pyproject.toml` dependencies (`redis[hiredis]` already + mandatory; no `fakeredis`). + +## Implementation Plan + +### Phase 1: Core code path removal + +- `common/redis.py`: delete `NullCache`; unwrap `get`/`set`/`delete` error + handling (keep `ping()` bool semantics and the SCAN-based delete diagnostics). +- `api/cache.py`: remove `if cache is None: return func(...)` from both wrappers; + remove the degrade catches in `_lookup`/`_store`; catch + `redis.exceptions.RedisError` in the wrappers and raise `HTTPException(503)`. +- `api/cache_invalidation.py`: non-Optional `_cache()`, drop the None-skip log + branch, ERROR-level (not WARNING) logging in `_drop`, docstring updates. +- `api/app.py`: unconditional backend construction in `lifespan` + startup ping + log; `/health/ready` always includes redis and flips to `not_ready`/503 on + `ping() == False`; remove `redis_enabled` from `create_app()` signature, + `app.state`, and `create_app_from_env()`. +- `api/cli.py`: remove the enable flag option/param; always echo Redis target. +- `common/config.py`: remove `redis_enabled` from `APISettings`. + +### Phase 2: Compose & infrastructure + +- `docker-compose.yml`: update the redis service header comment (required); + api service — remove `REDIS_ENABLED` env, add the `required: false` healthy + `depends_on` (migrate→postgres pattern). Keep `all`/`cache` profiles. +- `docker-compose.dev.yml`: `redis: profiles: [core]` union, identical shape to + the existing `postgres:` block. +- `docker-compose.prod.yml`: extend the header NOTE to cover Redis alongside + Postgres (bundled service not in `core`; use `REDIS_*` against the shared + instance; `--profile cache` to opt into the bundled one). +- `e2e/docker-compose.test.yml`: add the redis service; api `depends_on` + + `REDIS_HOST=redis`. +- `.env.example`: delete `REDIS_ENABLED`; rewrite the Redis block comments + (required dependency; bundled in dev; external for prod; key-prefix isolation + for multi-instance stays). +- `README.md`: profile table — `core` row gains redis: "postgres, redis (dev + override only), migrate, collector, api, web"; `cache` row loses + "(optional)" wording (mirror the `postgres` row: "Bundled Redis, standalone + opt-in"); extend the "Database note" paragraph (~README.md:121) to cover + Redis with the same profile-union / no-shadowing rationale. + +### Phase 3: Test infrastructure (real Redis) + +- `docker-compose.test-db.yml`: add `test-redis` (see TR8); update file header. +- `Makefile`: no target changes (same compose file drives `test-db-up`/`down`); + update comments mentioning the throwaway DB to mention Redis. +- `tests/conftest.py`: `TEST_REDIS_URL` resolution + ping fail-fast + (mirror the `TEST_POSTGRES_URL` hint). +- `tests/test_api/conftest.py`: real `RedisCacheBackend` wired in + `app_no_auth`/`app_with_auth`/`app_spam` (via `_wire_overrides`) with + worker-unique `key_prefix`; autouse per-test flush-by-prefix fixture. +- `.github/workflows/ci.yml`: redis service container + `TEST_REDIS_URL`. + +### Phase 4: Test updates + +- `tests/test_api/test_cache.py`: + - delete `TestNullCache` and all `NullCache` wiring (~lines 102–117, 318, + 792–808, 1969–1985, 2069–2073 — the "cache_control_ttl emitted even with + NullCache" tests become plain backend-present tests); + - rewrite `TestLifespanRedis`: always-constructs-from-app.state + close-on- + shutdown (drop the disabled/NullCache case); + - rewrite `TestCliRedis` (~lines 1149–1279): `test_redis_enabled_shows_banner` + drops the `--redis-enabled` flag (banner always shows); + `test_redis_disabled_hides_details` is deleted; + `test_redis_params_passed_to_create_app` drops `redis_enabled` from the + expected `create_app()` kwargs (asserted at ~line 1224); + - add hard-fail tests: backend raising `RedisError` on GET/SET ⇒ endpoint + responds 503; + - keep the mocked-`redis.Redis` unit tests in `TestRedisCacheBackend`. +- `tests/test_api/test_app_factory.py`: drop `redis_enabled` assertions; delete + `test_factory_redis_enabled_accepts_truthy_values`; adjust + `test_factory_reads_database_and_redis_from_env`; remove `"REDIS_ENABLED"` + from the `_FACTORY_ENV` cleanup list (line 19). +- Rewrite the existing `TestHealthReadyRedis` (test_cache.py:1101–1148): + `test_health_ready_omits_redis_when_disabled` is deleted (redis is always + reported now); the connected/unreachable tests drop their + `redis_enabled` assignments and mock-swap instead of delete + `app.state.redis_cache`; **`test_health_ready_reports_unreachable` flips + from asserting 200 to asserting 503 + `"status": "not_ready"`** — the + current 200 assertion is exactly the behaviour FR2 removes. +- Tests that already assign `MagicMock()`/`_FakeCache` to + `app.state.redis_cache` (`test_routes.py`, `test_dashboard.py`) continue to + work unchanged. + +### Phase 5: Documentation + +- `docs/upgrading.md`: new `## v0.20.0` section (v0.19.0 is already tagged and + shipped — `git tag --contains 81dcd3d` = v0.19.0): Redis now required, + `REDIS_ENABLED` removed (a leftover value is ignored), production + external-Redis guidance, dev stack now ships Redis in `core`. Do NOT edit the + historical v0.19/v0.12 sections that mention `REDIS_ENABLED` (lines ~416, + 438, 455) — those are release-history records of past behaviour. +- `docs/configuration.md` + `docs/deployment.md`: caching sections rewritten — + required dependency; drop `REDIS_ENABLED` row; bundled-vs-external guidance; + multi-instance `REDIS_KEY_PREFIX` note stays. The deployment.md sweep covers + **all** `REDIS_ENABLED` mentions, including the one outside the caching + section in the multi-worker guidance (~line 96), plus the Docker/bare-metal + ones (~lines 106, 113). +- `AGENTS.md`: update the cache-invalidation convention line ("helper is a no-op + when Redis is disabled" → mandatory backend, never-raise-after-commit + rationale); `core` stack description gains redis; test-db instructions now + cover Postgres + Redis. +- `docs/plans/20260830-2057-mandatory-redis/` (this plan; add `tasks.md` + alongside if the workflow calls for it). + +### Phase 6: Verification + +- `make test-db-up && pytest -nauto --no-cov 2>&1 | grep -iE "passed|failed" | tail -3` +- `pre-commit run --all-files` +- `npx playwright test --config=e2e/playwright.config.ts --list` (collection + check only; the user builds/runs the e2e stack) +- No image builds / `make up` — the user builds and runs the stack manually. + +## References + +- `docs/plans/20260609-2106-redis-api-cache/plan.md` — original (optional) Redis + cache implementation; source of the `NullCache`/`REDIS_ENABLED` pattern being + removed. +- `docs/plans/20260829-2312-remove-sqlite-support/plan.md` — PostgreSQL-mandatory + precedent whose compose profile-union pattern this plan mirrors. +- `docs/plans/20260613-2111-postgres-migration/plan.md` — earlier PostgreSQL + migration background. +- Commit `81dcd3d` — "Remove SQLite support; PostgreSQL is now the only database + backend" (scope model for config/CLI/tests/docs sweep). +- Commit `5a8794f` — "fix(compose): keep bundled postgres out of the core profile + in production" (the dev-union/prod-exclusion mechanism reused here). +- AGENTS.md → "Cache invalidation on writes" (convention text requiring update). +- `docs/deployment.md` → "Redis Caching", `docs/configuration.md` → "Caching", + `.env.example` Redis block (operator docs requiring rewrite). + +## Review + +**Status**: Approved with Changes + +**Reviewed**: 2026-08-30 + +### Resolutions + +- **FR4 invalidation semantics (open question)**: confirmed by user — + `invalidate_*` never raises after a committed write (ERROR log); only + read/store paths hard-fail with 503. FR4 text updated to record the + confirmation. +- **Version heading (open question)**: resolved to `## v0.20.0` — evidence: + `git tag --contains 81dcd3d` = `v0.19.0` and `git describe --tags` = + `v0.19.0`, so v0.19 (the SQLite-removal release) is already tagged/shipped; + the Redis change lands in the next release. +- **CI Redis pinning (open question)**: resolved to `redis:8` — evidence: + ci.yml pins the Postgres service to `postgres:17` with the comment "Pinned + to match the bundled postgres:17-alpine" (ci.yml:87-88); the bundled Redis is + `redis:8-alpine` (docker-compose.yml:135), so `redis:8` follows the same + convention. +- **`cache` compose profile (open question)**: resolved — keep it, mirroring + the retained `postgres` standalone profile; README `cache` row reworded to + "Bundled Redis, standalone opt-in". +- **Gap found: `TestCliRedis`** (test_cache.py:1149–1279) exercises the + `--redis-enabled` flag and asserts "Redis enabled: True/False" banners — + missing from the original Phase 4. Rewrite/delete instructions added. +- **Gap found (re-verification): `TestHealthReadyRedis`** + (test_cache.py:1101–1148) — an existing class the plan only said to + "add" tests for. Its `test_health_ready_reports_unreachable` asserts + **200** today; under FR2 it must assert **503 `not_ready`**, and + `test_health_ready_omits_redis_when_disabled` must be deleted. Rewrite + instructions added to Phase 4. +- **Gap found (re-verification): deployment.md sweep scope.** A + `REDIS_ENABLED=true` recommendation sits in the multi-worker guidance + (docs/deployment.md:96), outside the caching section (~106, ~113) — Phase 5 + now covers all mentions explicitly. +- **Gap found: `_FACTORY_ENV`** (test_app_factory.py:19) lists `REDIS_ENABLED` + in its env-cleanup list — removal added to Phase 4. +- **Gap found: `health_ready` docstring** reads "database and optional Redis" + (app.py:300) — docstring update added to TR3. +- **Gap found: compose env comment** "# Redis cache (optional — API works + without Redis)" on the api service — rewrite added to TR7. +- **Risk found: mock-swapping tests.** Some tests assign a + `MagicMock`/`_FakeCache` to `app.state.redis_cache` on the shared + module-scoped app (test_routes.py:441, test_dashboard.py:1347); with a real + default backend that would permanently displace it. TR8's flush fixture now + saves/restores `app.state.redis_cache` around each test. +- **Verified no conflict: e2e startup.** The e2e api healthcheck probes the + basic `/health` (e2e/docker-compose.test.yml:182), not `/health/ready`, so + the new 503 readiness cannot deadlock stack startup. The e2e api sets no + `REDIS_*` env at all, so `REDIS_HOST=redis` is mandatory there (added to + TR7). +- **Verified: no Makefile target changes.** `test-db-up` runs + `$(TEST_DB_COMPOSE) up -d --wait` on the whole file (Makefile:80-81), so a + new `test-redis` service starts automatically; `--wait` requires its + healthcheck (already in TR8). +- **Verified: fail-fast pattern.** The `db_url` fixture exits via + `pytest.exit(..., returncode=4)` with a `make test-db-up` hint + (tests/conftest.py:122-144); TR8 mirrors it for Redis. +- **Verified no plan conflict.** The remove-sqlite plan schedules the + `database_backend` field / `migrate-to-postgres` removals for v0.20 — the + same release hosts this change; the upgrading.md sections are additive, no + overlap. + +### Remaining Action Items + +- Implement Phases 1–6 (not started); run the Phase 6 verification commands. +- During Phase 5, leave the historical `REDIS_ENABLED` mentions in the + v0.19/v0.12 upgrading.md sections untouched (release-history records). +- When rewriting `TestCliRedis`, note the CLI keeps its + "Redis: host:port/db", "Redis key prefix:", and TTL echoes (cli.py:304-309) + — only the enabled/disabled banner and the flag disappear. diff --git a/docs/plans/20260830-2057-mandatory-redis/tasks.md b/docs/plans/20260830-2057-mandatory-redis/tasks.md new file mode 100644 index 0000000..403986b --- /dev/null +++ b/docs/plans/20260830-2057-mandatory-redis/tasks.md @@ -0,0 +1,103 @@ +# Tasks: Make Redis Mandatory (PostgreSQL-parity) + +> Generated from `plan.md` on 2026-08-30 + +## Group 1: Core Code Path Removal (Phase 1) + +- [x] 1.1 `src/meshcore_hub/common/redis.py`: delete `NullCache` and make errors propagate + - [x] Delete the `NullCache` class (starts at redis.py:25) + - [x] Unwrap try/except in `get`/`set`/`delete` so `redis.RedisError` propagates + - [x] Keep `ping() -> bool` swallowing semantics (health probe interface) and the SCAN-based delete diagnostics +- [x] 1.2 `src/meshcore_hub/api/cache.py`: single code path, 503 on Redis errors + - [x] Remove `if cache is None: return func(...)` from both the async and sync `@cached` wrappers + - [x] Remove warn-and-degrade exception handling from `_lookup`/`_store` + - [x] Catch `redis.exceptions.RedisError` at the wrapper boundary and raise `HTTPException(503, detail="cache backend unavailable")` (FR3) +- [x] 1.3 `src/meshcore_hub/api/cache_invalidation.py`: never-raise invalidation + - [x] Make `_cache()` return the non-Optional backend + - [x] Remove the "skipped (no backend)" branch/log + - [x] Log `_drop` backend errors at ERROR (not WARNING); never raise (FR4) + - [x] Update module/function docstrings describing disabled-mode / `NullCache` / `REDIS_ENABLED` behaviour (cache_invalidation.py:55-56) +- [x] 1.4 `src/meshcore_hub/api/app.py`: unconditional backend + readiness change + - [x] Lifespan (app.py:46-63): always construct `RedisCacheBackend(host, port, db, password, key_prefix)` from `app.state`; delete the `redis_enabled` branch and `NullCache` fallback + - [x] Ping at startup and log connected/error + - [x] Remove `redis_enabled` from `create_app()` signature (app.py:95), docstring (app.py:128), `app.state` assignment (app.py:172), and `create_app_from_env()` (app.py:375) + - [x] `health_ready` (app.py:298-322): always ping; unreachable ⇒ `"redis": "unreachable"` + overall `"status": "not_ready"` ⇒ 503; drop the `redis_enabled` guard (app.py:314-315) + - [x] Update the `health_ready` docstring: "database and optional Redis" → mandatory Redis (app.py:300) +- [x] 1.5 `src/meshcore_hub/api/cli.py`: remove the enable flag + - [x] Remove `--redis-enabled/--no-redis` option + `REDIS_ENABLED` envvar (cli.py:137) + the `redis_enabled` parameter (cli.py:240) and its `create_app` passthrough (cli.py:369) + - [x] Remove the "Redis enabled:" banner line (cli.py:302-303) + - [x] Keep the "Redis: host:port/db", "Redis key prefix:", and TTL echoes (cli.py:304-309) — always printed now +- [x] 1.6 `src/meshcore_hub/common/config.py`: remove `redis_enabled` from `APISettings` (config.py:458) + +## Group 2: Compose & Infrastructure (Phase 2) + +- [x] 2.1 `docker-compose.yml` (base) + - [x] Update the redis service header comment: required dependency, not optional + - [x] api service: remove `REDIS_ENABLED` env (line 325) and rewrite the "# Redis cache (optional — API works without Redis)" comment above it + - [x] api service: add `depends_on: redis: {condition: service_healthy, required: false}` alongside the existing migrate/collector dependencies + - [x] Keep the `all`/`cache` profiles on the redis service unchanged +- [x] 2.2 `docker-compose.dev.yml`: add `redis: profiles: [core]` union block, identical shape to the existing `postgres:` block (lines 44-46); keep the port mapping +- [x] 2.3 `docker-compose.prod.yml`: extend the header NOTE — bundled Redis not in `core`; point `REDIS_*` at the shared/external instance; `--profile cache` opts into the bundled one +- [x] 2.4 `e2e/docker-compose.test.yml`: add Redis to the e2e stack + - [x] Add a `redis:8-alpine` service with healthcheck + - [x] api service: add `REDIS_HOST=redis` (mandatory — the e2e api sets no other `REDIS_*` env and the code default `localhost` would fail) + `depends_on` redis + - [x] Leave the api healthcheck probing basic `/health` (line 182) — the new 503 readiness must not deadlock stack startup +- [x] 2.5 `.env.example`: delete `REDIS_ENABLED`; rewrite the Redis block comments (required dependency; bundled in dev; external for prod; `REDIS_KEY_PREFIX` multi-instance isolation stays) +- [x] 2.6 `README.md` + - [x] Profile table `core` row: "postgres, redis (dev override only), migrate, collector, api, web" (line 111) + - [x] Profile table `cache` row: drop "(optional)" — "Bundled Redis, standalone opt-in" mirroring the `postgres` row (line 113) + - [x] Extend the "Database note" paragraph (~line 121) to cover Redis with the same profile-union / no-shadowing rationale + +## Group 3: Test Infrastructure — Real Redis (Phase 3) + +- [x] 3.1 `docker-compose.test-db.yml`: add `test-redis` service (redis:8-alpine, published on `127.0.0.1:55433`, tmpfs, healthcheck); update the file header comment +- [x] 3.2 `Makefile`: comment-only updates — mention Redis alongside Postgres in the throwaway-DB comments; NO target changes (`test-db-up` runs `up -d --wait` on the whole file, Makefile:80-81, so `test-redis` starts automatically) +- [x] 3.3 `tests/conftest.py`: session `redis_url` fixture + - [x] Resolve `TEST_REDIS_URL` (default `redis://localhost:55433/0`) + - [x] Ping fail-fast mirroring `db_url` (tests/conftest.py:122-144): `pytest.exit(..., returncode=4)` with a `make test-db-up` hint +- [x] 3.4 `tests/test_api/conftest.py`: wire the real backend into the module app fixtures + - [x] In `_wire_overrides` (line 128): construct a real `RedisCacheBackend` with a per-xdist-worker `key_prefix` mirroring `db_schema` naming (e.g. `hub_test_gw0`) + - [x] Applies to `app_no_auth` (151), `app_with_auth` (172), `app_spam` (184) + - [x] Add a function-scoped autouse flush-by-prefix fixture (DB is truncated between tests; cache must be too, or modules see stale HITs) + - [x] The flush fixture must save/restore `app.state.redis_cache` around each test — tests that swap in `MagicMock`/`_FakeCache` (test_routes.py:441, test_dashboard.py:1347) must not permanently displace the real backend +- [x] 3.5 `.github/workflows/ci.yml`: add a `redis:8` service container (pin comment mirroring `postgres:17` at ci.yml:87-88) + `TEST_REDIS_URL` env on the backend-test job + +## Group 4: Test Updates (Phase 4) + +- [x] 4.1 `tests/test_api/test_cache.py`: delete `NullCache` coverage + - [x] Delete `TestNullCache` (lines 102-117) and the `NullCache` import (line 11) + - [x] Remove `NullCache` wiring at lines 318, 792-808, 1969-1985, 2069-2073 — the "cache_control_ttl emitted even with NullCache" tests become plain backend-present tests +- [x] 4.2 Rewrite `TestLifespanRedis` (lines 1005-1074): always-constructs-from-`app.state` + close-on-shutdown; drop the disabled/`NullCache` cases and `redis_enabled` assignments (lines 1012-1057) +- [x] 4.3 Rewrite `TestHealthReadyRedis` (lines 1101-1148) + - [x] Delete `test_health_ready_omits_redis_when_disabled` (redis is always reported now) + - [x] Connected/unreachable tests: drop `redis_enabled` assignments; mock-swap `app.state.redis_cache` instead of deleting it + - [x] `test_health_ready_reports_unreachable`: flip 200 → 503 + `"status": "not_ready"` (current 200 assertion is exactly the behaviour FR2 removes) +- [x] 4.4 Rewrite `TestCliRedis` (lines 1149-1279) + - [x] `test_redis_enabled_shows_banner`: drop `--redis-enabled` flag; banner always shows + - [x] Delete `test_redis_disabled_hides_details` + - [x] `test_redis_params_passed_to_create_app`: drop `redis_enabled` from expected `create_app()` kwargs (asserted ~line 1224); keep host/port/db/password/prefix/TTL assertions +- [x] 4.5 Add hard-fail tests (FR3): backend raising `RedisError` on GET/SET ⇒ endpoint responds 503 +- [x] 4.6 Keep the mocked-`redis.Redis` unit tests in `TestRedisCacheBackend` (line 120) unchanged +- [x] 4.7 `tests/test_api/test_app_factory.py` + - [x] Remove `"REDIS_ENABLED"` from the `_FACTORY_ENV` cleanup list (line 19) + - [x] Adjust `test_factory_reads_database_and_redis_from_env` (line 64): drop `REDIS_ENABLED` setenv + assertion (line 73) + - [x] Drop the false-case assertions (lines 84-90) + - [x] Delete `test_factory_redis_enabled_accepts_truthy_values` (lines 103-107) + +## Group 5: Documentation (Phase 5) + +- [x] 5.1 `docs/upgrading.md`: add a `## v0.20.0` section — Redis now required; `REDIS_ENABLED` removed (leftover values ignored); production external-Redis guidance; dev `core` stack now ships Redis + - [x] Do NOT edit the historical v0.19/v0.12 `REDIS_ENABLED` mentions (lines ~416, 438, 455) — release-history records +- [x] 5.2 `docs/configuration.md`: rewrite the caching section; drop the `REDIS_ENABLED` row (line 57) +- [x] 5.3 `docs/deployment.md`: rewrite all `REDIS_ENABLED` mentions — multi-worker guidance (~line 96), Docker (~106), bare-metal (~113); required-dependency framing; bundled-vs-external guidance; keep the multi-instance `REDIS_KEY_PREFIX` note +- [x] 5.4 `AGENTS.md`: update the cache-invalidation convention ("no-op when Redis is disabled" → mandatory backend, never-raise-after-commit rationale); `core` stack description gains redis; test-db instructions cover Postgres + Redis + +## Group 6: Verification (Phase 6) + +- [x] 6.1 `make test-db-up` — throwaway stack now starts `test-postgres` + `test-redis`; `--wait` proves the Redis healthcheck +- [x] 6.2 `pytest --no-cov tests/test_api/ 2>&1 | grep -iE "passed|failed" | tail -3` — targeted suite first (real Redis wired) +- [x] 6.3 `pytest -nauto --no-cov 2>&1 | grep -iE "passed|failed" | tail -3` — full backend suite +- [x] 6.4 Leftover sweep: `rg -n "REDIS_ENABLED|redis_enabled|NullCache" --glob '!.venv' --glob '!docs/plans/**'` must only match the historical upgrading.md sections (v0.19/v0.12) — everything else is a miss +- [x] 6.5 `pre-commit run --all-files` (includes ruff, frontend-typecheck hooks) +- [x] 6.6 `npx playwright test --config=e2e/playwright.config.ts --list` — collection check only (user builds/runs the e2e stack) +- [x] 6.7 No image builds, no `make up` — the user builds and runs the stack manually diff --git a/docs/upgrading.md b/docs/upgrading.md index a958676..4cb0a46 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -2,6 +2,26 @@ This guide covers upgrading from a previous MeshCore Hub release to the current version. Check the relevant version section below before upgrading. +## v0.20.0 + +### Redis is now required + +**⚠️ Breaking:** Redis is mandatory infrastructure for the API — the optional-cache mode introduced in v0.13 is removed. Every deployment must be able to reach a Redis server; there is no `REDIS_ENABLED` flag anymore (a leftover value in your environment is ignored and can be removed). + +What changed: + +- **`REDIS_ENABLED` is gone.** The API always configures its Redis cache backend from the remaining `REDIS_*` variables (`REDIS_HOST`, `REDIS_PORT`, `REDIS_DB`, `REDIS_PASSWORD`, `REDIS_KEY_PREFIX`, `REDIS_CACHE_TTL*`). Remove `REDIS_ENABLED` from your `.env` / orchestrator config. +- **A Redis outage is a hard failure.** `/health/ready` reports `"redis": "unreachable"` with overall `"status": "not_ready"` (HTTP 503 — the same contract as the database check), and cached endpoints return `503 cache backend unavailable` instead of silently degrading to direct database queries. Cache invalidation after a committed write never fails the request (backend errors log at ERROR). +- **The bundled `redis` container follows the Postgres profile pattern.** It joins the `core` compose profile only via `docker-compose.dev.yml`, so the documented dev workflow starts it automatically. Production (`docker-compose.prod.yml`) deliberately does **not** start it with `core`: point `REDIS_HOST`/`REDIS_PORT`/`REDIS_PASSWORD` at your shared/external Redis instance, or opt into the bundled container explicitly with `--profile cache` (or `--profile all`) — starting it alongside an external instance would shadow a `redis` host on the same network. +- **Bare-metal installs** need a running Redis (e.g. a local `redis-server`; the `localhost` defaults point at it) before starting the API. +- **Tests run against real Redis.** The backend suite requires a reachable Redis (`make test-db-up` starts a throwaway one alongside Postgres; `TEST_REDIS_URL` points at your own). CI uses a `redis:8` service container. + +Upgrading: + +1. Remove `REDIS_ENABLED` from your environment. +2. Ensure a Redis server is reachable from the `api` service (set `REDIS_*` accordingly — in Docker use the `cache` profile or an external instance). +3. Restart the stack and verify `/health/ready` reports `"redis": "connected"`. + ## v0.19.0 ### PostgreSQL-only (SQLite support removed) diff --git a/e2e/docker-compose.test.yml b/e2e/docker-compose.test.yml index c1e4e22..fda1230 100644 --- a/e2e/docker-compose.test.yml +++ b/e2e/docker-compose.test.yml @@ -1,10 +1,10 @@ # MeshCore Hub - Playwright End-to-End Test Stack # # Self-contained, throwaway stack for the Playwright suite in this directory. -# Runs its OWN ephemeral Postgres (schema created by `migrate` via Alembic) and -# NEVER touches the local development database: distinct project name, distinct -# named volumes, no host port for Postgres, and no ${VAR} interpolation (so the -# dev .env is never consulted). +# Runs its OWN ephemeral Postgres and Redis (schema created by `migrate` via +# Alembic) and NEVER touches the local development database: distinct project +# name, distinct named volumes, no host port for Postgres, and no ${VAR} +# interpolation (so the dev .env is never consulted). # # Usage (from the repo root): # make e2e-build # docker compose -f e2e/docker-compose.test.yml build @@ -34,6 +34,21 @@ services: retries: 10 start_period: 5s + # ========================================================================== + # Redis - required cache backend for the API (ephemeral, not published) + # ========================================================================== + redis: + image: redis:8-alpine + container_name: meshcore-test-redis + tmpfs: + - /data + healthcheck: + test: ["CMD", "redis-cli", "ping"] + interval: 5s + timeout: 5s + retries: 10 + start_period: 5s + # ========================================================================== # Database Migrations - create the `meshcorehub` schema + tables (Alembic) # ========================================================================== @@ -153,6 +168,8 @@ services: condition: service_completed_successfully mqtt: condition: service_healthy + redis: + condition: service_healthy ports: - "18000:8000" volumes: @@ -177,6 +194,9 @@ services: - API_PORT=8000 - API_READ_KEY=test-read-key - API_ADMIN_KEY=test-admin-key + # Redis is required — point at the ephemeral service (the code default + # 'localhost' would fail inside the container network). + - REDIS_HOST=redis command: ["api"] healthcheck: test: ["CMD", "python", "-c", "import urllib.request; urllib.request.urlopen('http://localhost:8000/health')"] diff --git a/src/meshcore_hub/api/app.py b/src/meshcore_hub/api/app.py index e54c35d..e3cd362 100644 --- a/src/meshcore_hub/api/app.py +++ b/src/meshcore_hub/api/app.py @@ -42,25 +42,29 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: logger.info(f"Initializing database: {database_url}") _db_manager = DatabaseManager(database_url) - # Initialize Redis cache - redis_enabled = getattr(app.state, "redis_enabled", False) - if redis_enabled: - from meshcore_hub.common.redis import RedisCacheBackend + # Initialize Redis cache (required infrastructure) + from meshcore_hub.common.redis import RedisCacheBackend - redis_cache = RedisCacheBackend( - host=getattr(app.state, "redis_host", "localhost"), - port=getattr(app.state, "redis_port", 6379), - db=getattr(app.state, "redis_db", 0), - password=getattr(app.state, "redis_password", None), - key_prefix=getattr(app.state, "redis_key_prefix", "hub"), + redis_cache = RedisCacheBackend( + host=getattr(app.state, "redis_host", "localhost"), + port=getattr(app.state, "redis_port", 6379), + db=getattr(app.state, "redis_db", 0), + password=getattr(app.state, "redis_password", None), + key_prefix=getattr(app.state, "redis_key_prefix", "hub"), + ) + app.state.redis_cache = redis_cache + if redis_cache.ping(): + logger.info( + "Redis cache connected: %s:%s/%s", + getattr(app.state, "redis_host", "localhost"), + getattr(app.state, "redis_port", 6379), + getattr(app.state, "redis_db", 0), ) - app.state.redis_cache = redis_cache - logger.info("Redis cache enabled") else: - from meshcore_hub.common.redis import NullCache - - app.state.redis_cache = NullCache() - logger.info("Redis cache disabled") + logger.error( + "Redis cache unreachable at startup — /health/ready will report " + "not_ready and cached endpoints will return 503 until it recovers" + ) yield @@ -92,7 +96,6 @@ def create_app( metrics_enabled: bool = True, metrics_cache_ttl: int = 60, metrics_public: bool = False, - redis_enabled: bool = False, redis_host: str = "localhost", redis_port: int = 6379, redis_db: int = 0, @@ -125,7 +128,6 @@ def create_app( metrics_enabled: Enable Prometheus metrics endpoint at /metrics metrics_cache_ttl: Seconds to cache metrics output metrics_public: Allow unauthenticated /metrics when no read key is set - redis_enabled: Enable Redis API response caching redis_host: Redis server host redis_port: Redis server port redis_db: Redis database number @@ -169,7 +171,6 @@ def create_app( app.state.mqtt_ws_path = mqtt_ws_path app.state.metrics_cache_ttl = metrics_cache_ttl app.state.metrics_public = metrics_public - app.state.redis_enabled = redis_enabled app.state.redis_host = redis_host app.state.redis_port = redis_port app.state.redis_db = redis_db @@ -297,10 +298,12 @@ def create_app( @app.get("/health/ready", tags=["Health"]) def health_ready() -> JSONResponse: - """Readiness check including database and optional Redis. + """Readiness check including database and Redis. - Returns 503 when not ready so Docker/Kubernetes readiness probes - (which judge by HTTP status code) pull the instance from rotation. + Redis is required infrastructure: an unreachable backend flips the + overall status to ``not_ready``. Returns 503 when not ready so + Docker/Kubernetes readiness probes (which judge by HTTP status + code) pull the instance from rotation. """ try: db = get_db_manager() @@ -311,12 +314,11 @@ def create_app( result = {"status": "not_ready", "database": type(e).__name__} redis_cache = getattr(app.state, "redis_cache", None) - redis_enabled = getattr(app.state, "redis_enabled", False) - if redis_enabled and redis_cache is not None: - if redis_cache.ping(): - result["redis"] = "connected" - else: - result["redis"] = "unreachable" + if redis_cache is not None and redis_cache.ping(): + result["redis"] = "connected" + else: + result["redis"] = "unreachable" + result["status"] = "not_ready" status_code = 200 if result.get("status") == "ready" else 503 return JSONResponse(content=result, status_code=status_code) @@ -372,7 +374,6 @@ def create_app_from_env() -> FastAPI: metrics_enabled=metrics_enabled, metrics_cache_ttl=metrics_cache_ttl, metrics_public=metrics_public, - redis_enabled=settings.redis_enabled, redis_host=settings.redis_host, redis_port=settings.redis_port, redis_db=settings.redis_db, diff --git a/src/meshcore_hub/api/cache.py b/src/meshcore_hub/api/cache.py index b3f783b..76677dd 100644 --- a/src/meshcore_hub/api/cache.py +++ b/src/meshcore_hub/api/cache.py @@ -8,11 +8,20 @@ import logging from typing import Any, Callable, Optional from urllib.parse import urlencode -from fastapi import Request +from fastapi import HTTPException, Request from fastapi.responses import Response +from redis.exceptions import RedisError logger = logging.getLogger(__name__) +_CACHE_UNAVAILABLE_DETAIL = "cache backend unavailable" + + +def _cache_unavailable(key: str, e: RedisError) -> HTTPException: + """Translate a Redis outage into a 503 (mandatory infrastructure).""" + logger.error("Redis error for %s: %s", key, e) + return HTTPException(status_code=503, detail=_CACHE_UNAVAILABLE_DETAIL) + def sorted_query_string(request: Request) -> str: """Build a deterministic query string from request params, sorted by key. @@ -109,10 +118,7 @@ def _store(cache: Any, cache_key: str, result: Any, ttl: int) -> tuple[Any, str] body, serialized = _serialize_for_cache(result) etag = _compute_etag(serialized) envelope = json.dumps({"body": body, "etag": etag}) - try: - cache.set(cache_key, envelope, ttl) - except Exception as e: - logger.warning("Cache store error for %s: %s", cache_key, e) + cache.set(cache_key, envelope, ttl) return body, etag @@ -124,11 +130,7 @@ def _lookup(cache: Any, cache_key: str, request: Request) -> tuple[Any, str]: hashed on read so they still serve correctly; natural expiry migrates them to the envelope format on the next write. """ - try: - raw = cache.get(cache_key) - except Exception as e: - logger.warning("Redis GET error for %s: %s", cache_key, e) - return _MISS, "" + raw = cache.get(cache_key) if raw is None: logger.debug("Cache MISS: %s", cache_key) @@ -176,24 +178,27 @@ def cached( @functools.wraps(func) async def async_wrapper(*args: Any, **kwargs: Any) -> Any: request = _find_request(kwargs) - cache = getattr(request.app.state, "redis_cache", None) + cache = request.app.state.redis_cache ttl = getattr(request.app.state, ttl_setting, 30) # Always advertise the client-cache TTL so the middleware can - # emit Cache-Control regardless of whether Redis is configured. + # emit Cache-Control for every response. request.state.cache_control_ttl = ttl - if cache is None: - return await func(*args, **kwargs) - cache_key = _build_cache_key(request, endpoint_name, key_builder) - body, etag = _lookup(cache, cache_key, request) + try: + body, etag = _lookup(cache, cache_key, request) + except RedisError as e: + raise _cache_unavailable(cache_key, e) from e if body is _MISS: # MISS: call the handler, store, and return the original # result so FastAPI's response_model handling still sees # the Pydantic model (not the JSON-dumped dict). result = await func(*args, **kwargs) - _, etag = _store(cache, cache_key, result, ttl) + try: + _, etag = _store(cache, cache_key, result, ttl) + except RedisError as e: + raise _cache_unavailable(cache_key, e) from e return_value = result else: # HIT: only have the JSON-deserialized body. @@ -214,19 +219,22 @@ def cached( @functools.wraps(func) def sync_wrapper(*args: Any, **kwargs: Any) -> Any: request = _find_request(kwargs) - cache = getattr(request.app.state, "redis_cache", None) + cache = request.app.state.redis_cache ttl = getattr(request.app.state, ttl_setting, 30) request.state.cache_control_ttl = ttl - if cache is None: - return func(*args, **kwargs) - cache_key = _build_cache_key(request, endpoint_name, key_builder) - body, etag = _lookup(cache, cache_key, request) + try: + body, etag = _lookup(cache, cache_key, request) + except RedisError as e: + raise _cache_unavailable(cache_key, e) from e if body is _MISS: result = func(*args, **kwargs) - _, etag = _store(cache, cache_key, result, ttl) + try: + _, etag = _store(cache, cache_key, result, ttl) + except RedisError as e: + raise _cache_unavailable(cache_key, e) from e return_value = result else: return_value = body diff --git a/src/meshcore_hub/api/cache_invalidation.py b/src/meshcore_hub/api/cache_invalidation.py index 48a770e..bd67e46 100644 --- a/src/meshcore_hub/api/cache_invalidation.py +++ b/src/meshcore_hub/api/cache_invalidation.py @@ -26,13 +26,13 @@ Helpers here delete every namespace an entity touches — including cross-entity embeddings (e.g. adoptions surface inside ``nodes``, ``profiles``, and ``advertisements`` listings). -All helpers are no-ops when ``app.state.redis_cache`` is missing (cache -disabled) and swallow any backend error so a cache outage never breaks a -successful write — matching the resilience pattern in ``RedisCacheBackend``. +All helpers never raise: they run after ``session.commit()`` has succeeded, +so raising would report failure for a durable write and invite duplicate +client retries. Backend errors log at ERROR instead — only the read/store +paths in ``api/cache.py`` hard-fail (503) during a Redis outage. """ import logging -from typing import Optional from fastapi import Request @@ -41,27 +41,23 @@ from meshcore_hub.common.redis import CacheBackend logger = logging.getLogger(__name__) -def _cache(request: Request) -> Optional[CacheBackend]: - """Return the cache backend for this app, or None if caching is disabled.""" - return getattr(request.app.state, "redis_cache", None) +def _cache(request: Request) -> CacheBackend: + """Return the mandatory cache backend for this app.""" + cache: CacheBackend = request.app.state.redis_cache + return cache def _drop(request: Request, prefix: str) -> None: - """Best-effort ``delete(prefix)``; never raises. + """``delete(prefix)``; never raises. - Emits structured log lines so production traces can confirm a mutation - handler actually fired invalidation and see how many Redis keys were - deleted. The ``backend=`` field distinguishes ``RedisCacheBackend`` - (real Redis) from ``NullCache`` (Redis disabled) in one glance — useful - when ``REDIS_ENABLED`` is misconfigured. + Invalidation runs after ``session.commit()`` succeeded, so an exception + here would report failure for a durable write. Backend errors log at + ERROR (read/store paths in ``api/cache.py`` are the ones that + hard-fail). Emits structured log lines so production traces can confirm + a mutation handler actually fired invalidation and see how many Redis + keys were deleted. """ cache = _cache(request) - if cache is None: - logger.debug( - "Cache invalidate skipped (no backend on app.state): prefix=%s", - prefix, - ) - return logger.info( "Cache invalidate start: prefix=%s backend=%s", prefix, @@ -71,7 +67,7 @@ def _drop(request: Request, prefix: str) -> None: cache.delete(prefix) logger.info("Cache invalidate ok: prefix=%s", prefix) except Exception as e: - logger.warning( + logger.error( "Cache invalidate error: prefix=%s error=%s", prefix, e, diff --git a/src/meshcore_hub/api/cli.py b/src/meshcore_hub/api/cli.py index 22b94de..c2907a4 100644 --- a/src/meshcore_hub/api/cli.py +++ b/src/meshcore_hub/api/cli.py @@ -131,12 +131,6 @@ import click "configured (default: deny)" ), ) -@click.option( - "--redis-enabled/--no-redis", - default=False, - envvar="REDIS_ENABLED", - help="Enable Redis API response caching", -) @click.option( "--redis-host", type=str, @@ -237,7 +231,6 @@ def api( metrics_enabled: bool, metrics_cache_ttl: int, metrics_public: bool, - redis_enabled: bool, redis_host: str, redis_port: int, redis_db: int, @@ -299,14 +292,12 @@ def api( click.echo(f"Metrics enabled: {metrics_enabled}") click.echo(f"Metrics cache TTL: {metrics_cache_ttl}s") click.echo(f"Metrics public (no key): {metrics_public}") - click.echo(f"Redis enabled: {redis_enabled}") - if redis_enabled: - click.echo(f"Redis: {redis_host}:{redis_port}/{redis_db}") - click.echo(f"Redis key prefix: {redis_key_prefix}") - click.echo( - f"Redis cache TTL: {redis_cache_ttl}s " - f"(dashboard: {redis_cache_ttl_dashboard}s)" - ) + click.echo(f"Redis: {redis_host}:{redis_port}/{redis_db}") + click.echo(f"Redis key prefix: {redis_key_prefix}") + click.echo( + f"Redis cache TTL: {redis_cache_ttl}s " + f"(dashboard: {redis_cache_ttl_dashboard}s)" + ) click.echo(f"API Cache-Control enabled: {api_cache_control_enabled}") click.echo( f"Spam detection: {settings.spam_detection_enabled} " @@ -326,7 +317,6 @@ def api( # We need to pass app as string for reload to work click.echo("\nStarting in development mode with auto-reload...") click.echo("Note: Using default settings for reload mode.") - click.echo("Note: Redis defaults to disabled in reload mode.") uvicorn.run( "meshcore_hub.api.app:create_app", @@ -366,7 +356,6 @@ def api( metrics_enabled=metrics_enabled, metrics_cache_ttl=metrics_cache_ttl, metrics_public=metrics_public, - redis_enabled=redis_enabled, redis_host=redis_host, redis_port=redis_port, redis_db=redis_db, diff --git a/src/meshcore_hub/common/config.py b/src/meshcore_hub/common/config.py index c2a9f07..74860c4 100644 --- a/src/meshcore_hub/common/config.py +++ b/src/meshcore_hub/common/config.py @@ -454,10 +454,7 @@ class APISettings(CommonSettings): ), ) - # Redis cache - redis_enabled: bool = Field( - default=False, description="Enable Redis API response caching" - ) + # Redis cache (required infrastructure) redis_host: str = Field(default="localhost", description="Redis server host") redis_port: int = Field(default=6379, description="Redis server port") redis_db: int = Field(default=0, description="Redis database number") diff --git a/src/meshcore_hub/common/redis.py b/src/meshcore_hub/common/redis.py index 8024442..f14e3e8 100644 --- a/src/meshcore_hub/common/redis.py +++ b/src/meshcore_hub/common/redis.py @@ -22,26 +22,14 @@ class CacheBackend: raise NotImplementedError -class NullCache(CacheBackend): - """No-op cache backend used when Redis is disabled.""" - - def get(self, key: str) -> Optional[str]: - return None - - def set(self, key: str, value: str, ttl: int) -> None: - pass - - def delete(self, prefix: str) -> None: - # Logged so production traces can distinguish "Redis disabled" from - # "Redis enabled but matched no keys" when diagnosing invalidation. - logger.debug("NullCache delete: prefix=%s", prefix) - - def ping(self) -> bool: - return False - - class RedisCacheBackend(CacheBackend): - """Redis-backed cache using a connection pool.""" + """Redis-backed cache using a connection pool. + + Redis is required infrastructure: ``get``/``set``/``delete`` let + ``redis.RedisError`` propagate so callers fail loudly (the ``@cached`` + decorator translates it into a 503 response). Only ``ping()`` swallows + errors — it is a boolean health-probe interface. + """ def __init__( self, @@ -68,22 +56,15 @@ class RedisCacheBackend(CacheBackend): return f"{self._prefix}:{key}" def get(self, key: str) -> Optional[str]: - try: - full_key = self._full_key(key) - value = self._client.get(full_key) - if value is not None: - return value.decode("utf-8") if isinstance(value, bytes) else value - return None - except Exception as e: - logger.warning("Redis GET error for %s: %s", key, e) - return None + full_key = self._full_key(key) + value = self._client.get(full_key) + if value is not None: + return value.decode("utf-8") if isinstance(value, bytes) else value + return None def set(self, key: str, value: str, ttl: int) -> None: - try: - full_key = self._full_key(key) - self._client.setex(full_key, ttl, value) - except Exception as e: - logger.warning("Redis SET error for %s: %s", key, e) + full_key = self._full_key(key) + self._client.setex(full_key, ttl, value) def delete(self, prefix: str) -> None: # Diagnostic logging: emit one INFO line per call with the prefix, @@ -94,32 +75,22 @@ class RedisCacheBackend(CacheBackend): full_prefix = self._full_key(prefix) total_deleted = 0 iterations = 0 - try: - cursor = 0 - while True: - cursor, keys = self._client.scan( - cursor, match=f"{full_prefix}*", count=100 - ) - iterations += 1 - if keys: - self._client.delete(*keys) - total_deleted += len(keys) - if cursor == 0: - break - logger.info( - "Redis cache delete: prefix=%s full_prefix=%s keys_deleted=%d scan_iterations=%d", - prefix, - full_prefix, - total_deleted, - iterations, - ) - except Exception as e: - logger.warning( - "Redis DELETE error: prefix=%s full_prefix=%s error=%s", - prefix, - full_prefix, - e, - ) + cursor = 0 + while True: + cursor, keys = self._client.scan(cursor, match=f"{full_prefix}*", count=100) + iterations += 1 + if keys: + self._client.delete(*keys) + total_deleted += len(keys) + if cursor == 0: + break + logger.info( + "Redis cache delete: prefix=%s full_prefix=%s keys_deleted=%d scan_iterations=%d", + prefix, + full_prefix, + total_deleted, + iterations, + ) def ping(self) -> bool: try: diff --git a/tests/conftest.py b/tests/conftest.py index 6230ebf..60b458c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -118,6 +118,37 @@ DEFAULT_TEST_POSTGRES_URL = ( "@localhost:55432/meshcorehub_test" ) +# Default points at the throwaway stack's Redis (127.0.0.1:55433). Override +# with TEST_REDIS_URL to use any other Redis instance. +DEFAULT_TEST_REDIS_URL = "redis://localhost:55433/0" + + +@pytest.fixture(scope="session") +def redis_url() -> str: + """Redis URL for this pytest session (Redis is a required dependency). + + Missing or unreachable Redis is a hard exit — mirroring the Postgres + fail-fast in ``db_url`` — with an actionable message pointing at + ``make test-db-up``. + """ + import redis as redis_lib + + url = os.environ.get("TEST_REDIS_URL") or DEFAULT_TEST_REDIS_URL + probe = redis_lib.Redis.from_url(url, socket_timeout=2, socket_connect_timeout=2) + try: + probe.ping() + except Exception as exc: + pytest.exit( + f"Redis test server unreachable at {url!r}: {exc}\n" + "Backend tests require Redis (mandatory cache infrastructure). " + "Start the throwaway stack with: make test-db-up\n" + "(or point TEST_REDIS_URL at your own instance)", + returncode=4, + ) + finally: + probe.close() + return url + @pytest.fixture(scope="session") def db_url(worker_id: str) -> str: diff --git a/tests/test_api/conftest.py b/tests/test_api/conftest.py index 029872f..30877a7 100644 --- a/tests/test_api/conftest.py +++ b/tests/test_api/conftest.py @@ -7,6 +7,7 @@ suite can reuse the same database; they are inherited here. from contextlib import contextmanager from datetime import datetime, timezone +from urllib.parse import urlparse import pytest from fastapi.testclient import TestClient @@ -19,6 +20,7 @@ from meshcore_hub.api.dependencies import ( get_db_manager, ) from meshcore_hub.common.database import DatabaseManager, create_database_engine +from meshcore_hub.common.redis import RedisCacheBackend from meshcore_hub.common.models import ( Advertisement, Base, @@ -73,6 +75,39 @@ def api_db_session(api_db_engine): _truncate_all(api_db_engine) +@pytest.fixture(scope="session") +def api_redis_cache(redis_url: str, worker_id: str): + """Session-scoped real Redis backend with a per-xdist-worker prefix. + + Redis is mandatory infrastructure, so the shared app fixtures exercise + the production backend. The key prefix mirrors ``db_schema`` naming + (``hub_test_gw0``) so parallel xdist workers on one Redis instance never + see each other's keys. + """ + parsed = urlparse(redis_url) + db_path = (parsed.path or "/0").lstrip("/") + return RedisCacheBackend( + host=parsed.hostname or "localhost", + port=parsed.port or 6379, + db=int(db_path) if db_path else 0, + password=parsed.password, + key_prefix=f"hub_test_{worker_id}", + ) + + +@pytest.fixture(autouse=True) +def _flush_redis_cache(api_redis_cache): + """Flush this worker's cache keys before each test (autouse). + + The database is truncated between tests; the cache must be too, or + module-scoped apps sharing one backend would serve stale HITs from + earlier tests. ``delete("")`` SCAN-matches every key under the + worker's ``hub_test_`` prefix — including keys left behind by + a previous pytest run (worker prefixes are stable). + """ + api_redis_cache.delete("") + + @pytest.fixture(scope="session") def mock_mqtt(): """Session-scoped mock MQTT client (no per-test state).""" @@ -125,8 +160,14 @@ def _isolate_db_global(monkeypatch: pytest.MonkeyPatch, mock_db_manager) -> None monkeypatch.setattr(app_module, "_db_manager", mock_db_manager) -def _wire_overrides(app, api_db_engine, mock_mqtt, mock_db_manager) -> None: - """Install the standard DB/MQTT dependency overrides on ``app``.""" +def _wire_overrides( + app, api_db_engine, mock_mqtt, mock_db_manager, redis_cache +) -> None: + """Install the standard DB/MQTT/cache wiring on ``app``. + + ``app.state.redis_cache`` is pinned to the real session backend because + TestClient never runs the lifespan (which would normally construct it). + """ Session = sessionmaker(bind=api_db_engine) def override_get_db_manager(request=None): @@ -145,10 +186,11 @@ def _wire_overrides(app, api_db_engine, mock_mqtt, mock_db_manager) -> None: app.dependency_overrides[get_db_manager] = override_get_db_manager app.dependency_overrides[get_db_session] = override_get_db_session app.dependency_overrides[get_mqtt_client] = override_get_mqtt_client + app.state.redis_cache = redis_cache @pytest.fixture(scope="module") -def app_no_auth(db_url, api_db_engine, mock_mqtt, mock_db_manager): +def app_no_auth(db_url, api_db_engine, mock_mqtt, mock_db_manager, api_redis_cache): """Module-scoped FastAPI app with no authentication. Built once per test module; ``create_app`` is the second-most expensive @@ -164,24 +206,24 @@ def app_no_auth(db_url, api_db_engine, mock_mqtt, mock_db_manager): # app opts in so data-shape tests keep working without a key. metrics_public=True, ) - _wire_overrides(app, api_db_engine, mock_mqtt, mock_db_manager) + _wire_overrides(app, api_db_engine, mock_mqtt, mock_db_manager, api_redis_cache) yield app @pytest.fixture(scope="module") -def app_with_auth(db_url, api_db_engine, mock_mqtt, mock_db_manager): +def app_with_auth(db_url, api_db_engine, mock_mqtt, mock_db_manager, api_redis_cache): """Module-scoped FastAPI app with authentication enabled.""" app = create_app( database_url=db_url, read_key="test-read-key", admin_key="test-admin-key", ) - _wire_overrides(app, api_db_engine, mock_mqtt, mock_db_manager) + _wire_overrides(app, api_db_engine, mock_mqtt, mock_db_manager, api_redis_cache) yield app @pytest.fixture(scope="module") -def app_spam(db_url, api_db_engine, mock_mqtt, mock_db_manager): +def app_spam(db_url, api_db_engine, mock_mqtt, mock_db_manager, api_redis_cache): """Module-scoped app with spam detection enabled (no auth).""" app = create_app( database_url=db_url, @@ -190,25 +232,32 @@ def app_spam(db_url, api_db_engine, mock_mqtt, mock_db_manager): spam_detection_enabled=True, spam_score_threshold=0.6, ) - _wire_overrides(app, api_db_engine, mock_mqtt, mock_db_manager) + _wire_overrides(app, api_db_engine, mock_mqtt, mock_db_manager, api_redis_cache) yield app @pytest.fixture -def client_spam(app_spam) -> TestClient: +def client_spam(app_spam, api_redis_cache) -> TestClient: """Test client with spam detection enabled.""" + # Re-pin the real backend: tests that swap a mock/_FakeCache onto the + # shared module-scoped app's state must not displace it permanently. + app_spam.state.redis_cache = api_redis_cache return TestClient(app_spam, raise_server_exceptions=True) @pytest.fixture -def client_no_auth(app_no_auth) -> TestClient: +def client_no_auth(app_no_auth, api_redis_cache) -> TestClient: """Test client with no authentication.""" + # Re-pin the real backend (see client_spam). + app_no_auth.state.redis_cache = api_redis_cache return TestClient(app_no_auth, raise_server_exceptions=True) @pytest.fixture -def client_with_auth(app_with_auth) -> TestClient: +def client_with_auth(app_with_auth, api_redis_cache) -> TestClient: """Test client with authentication enabled.""" + # Re-pin the real backend (see client_spam). + app_with_auth.state.redis_cache = api_redis_cache return TestClient(app_with_auth, raise_server_exceptions=True) diff --git a/tests/test_api/test_app_factory.py b/tests/test_api/test_app_factory.py index d717d13..ecfc1dd 100644 --- a/tests/test_api/test_app_factory.py +++ b/tests/test_api/test_app_factory.py @@ -16,7 +16,6 @@ _FACTORY_ENV = [ "DATABASE_SCHEMA", "DATABASE_USER", "DATABASE_PASSWORD", - "REDIS_ENABLED", "REDIS_HOST", "REDIS_PORT", "REDIS_CACHE_TTL", @@ -61,7 +60,6 @@ def test_factory_reads_database_and_redis_from_env(clean_env): """Workers must pick up the real DB/Redis config from env, not the hardcoded create_app defaults.""" clean_env.setenv("DATABASE_URL", "postgresql+psycopg2://u:p@workers-test:5432/hub") - clean_env.setenv("REDIS_ENABLED", "true") clean_env.setenv("REDIS_HOST", "redis-test") clean_env.setenv("REDIS_PORT", "6390") clean_env.setenv("MQTT_HOST", "mqtt-test") @@ -70,7 +68,6 @@ def test_factory_reads_database_and_redis_from_env(clean_env): app = create_app_from_env() assert app.state.database_url == "postgresql+psycopg2://u:p@workers-test:5432/hub" - assert app.state.redis_enabled is True assert app.state.redis_host == "redis-test" assert app.state.redis_port == 6390 assert app.state.mqtt_host == "mqtt-test" @@ -81,13 +78,11 @@ def test_factory_assembles_url_from_component_vars(clean_env): """Without an explicit DATABASE_URL, the DATABASE_* components assemble the connection — the app factory never falls back to a default file DB.""" clean_env.delenv("DATABASE_URL") - clean_env.setenv("REDIS_ENABLED", "false") clean_env.setenv("DATABASE_HOST", "pg-test") clean_env.setenv("DATABASE_PASSWORD", "pw") app = create_app_from_env() - assert app.state.redis_enabled is False assert app.state.database_url == ( "postgresql+psycopg2://meshcorehub:pw@pg-test:5432/meshcorehub" ) @@ -100,13 +95,6 @@ def test_factory_without_database_config_raises(clean_env): _ = create_app_from_env() -def test_factory_redis_enabled_accepts_truthy_values(clean_env): - """REDIS_ENABLED / METRICS_ENABLED parse common truthy spellings.""" - clean_env.setenv("REDIS_ENABLED", "1") - app = create_app_from_env() - assert app.state.redis_enabled is True - - def test_factory_metrics_enabled_via_env(clean_env): """METRICS_ENABLED=true mounts the /metrics endpoint.""" clean_env.setenv("METRICS_ENABLED", "true") diff --git a/tests/test_api/test_cache.py b/tests/test_api/test_cache.py index 0e156d4..8d9325d 100644 --- a/tests/test_api/test_cache.py +++ b/tests/test_api/test_cache.py @@ -4,11 +4,12 @@ import json from unittest.mock import AsyncMock, MagicMock, patch import pytest -from fastapi import FastAPI, Request +from fastapi import FastAPI, HTTPException, Request from pydantic import BaseModel +from redis.exceptions import ConnectionError as RedisConnectionError, RedisError from meshcore_hub.api.cache import cached, sorted_query_string -from meshcore_hub.common.redis import NullCache, RedisCacheBackend +from meshcore_hub.common.redis import RedisCacheBackend class _SampleModel(BaseModel): @@ -99,24 +100,6 @@ class TestSortedQueryString: assert sorted_query_string(both) != sorted_query_string(single) -class TestNullCache: - def test_get_returns_none(self): - cache = NullCache() - assert cache.get("any_key") is None - - def test_set_does_not_raise(self): - cache = NullCache() - cache.set("key", "value", 30) - - def test_ping_returns_false(self): - cache = NullCache() - assert cache.ping() is False - - def test_delete_does_not_raise(self): - cache = NullCache() - cache.delete("prefix") - - class TestRedisCacheBackend: def test_get_returns_cached_value(self): with patch("redis.Redis") as mock_redis_cls: @@ -167,24 +150,27 @@ class TestRedisCacheBackend: backend = RedisCacheBackend(key_prefix="hub") assert backend.ping() is False - def test_get_returns_none_on_connection_error(self): + def test_get_raises_on_connection_error(self): + """Redis is mandatory: GET errors propagate (caller decides 503).""" with patch("redis.Redis") as mock_redis_cls: mock_client = MagicMock() mock_redis_cls.return_value = mock_client - mock_client.get.side_effect = Exception("connection error") + mock_client.get.side_effect = RedisConnectionError("connection error") backend = RedisCacheBackend(key_prefix="hub") - assert backend.get("any_key") is None + with pytest.raises(RedisError): + backend.get("any_key") - def test_set_logs_warning_on_error(self): + def test_set_raises_on_error(self): + """Redis is mandatory: SET errors propagate (caller decides 503).""" with patch("redis.Redis") as mock_redis_cls: mock_client = MagicMock() mock_redis_cls.return_value = mock_client - mock_client.setex.side_effect = Exception("timeout") + mock_client.setex.side_effect = RedisError("timeout") backend = RedisCacheBackend(key_prefix="hub") - backend.set("key", "value", 30) - # Should not raise + with pytest.raises(RedisError): + backend.set("key", "value", 30) def test_key_prefix_prepended(self): with patch("redis.Redis") as mock_redis_cls: @@ -231,14 +217,16 @@ class TestRedisCacheBackend: assert mock_client.scan.call_count == 2 assert mock_client.delete.call_count == 2 - def test_delete_handles_exception(self): + def test_delete_raises_on_scan_error(self): + """DELETE errors propagate; ``cache_invalidation._drop`` contains them.""" with patch("redis.Redis") as mock_redis_cls: mock_client = MagicMock() mock_redis_cls.return_value = mock_client - mock_client.scan.side_effect = Exception("scan error") + mock_client.scan.side_effect = RedisError("scan error") backend = RedisCacheBackend(key_prefix="hub") - backend.delete("nodes") + with pytest.raises(RedisError): + backend.delete("nodes") def test_close_calls_client_close(self): with patch("redis.Redis") as mock_redis_cls: @@ -313,44 +301,17 @@ class TestCachedDecorator: assert request.state.cache_status == "MISS" mock_cache.set.assert_called_once() - async def test_null_cache_always_calls_handler(self): - app = FastAPI() - app.state.redis_cache = NullCache() - app.state.redis_cache_ttl = 30 - - call_count = 0 - - @cached("nodes") - async def handler(request: Request): - nonlocal call_count - call_count += 1 - return {"items": [], "total": 0} - - scope = { - "type": "http", - "query_string": b"limit=50", - "headers": [], - "app": app, - } - from starlette.datastructures import State - - request = Request(scope) - request._state = State() - - await handler(request=request) - await handler(request=request) - assert call_count == 2 - - async def test_redis_error_falls_through(self): + async def test_redis_error_on_lookup_returns_503(self): + """Redis is mandatory: a GET error surfaces as 503, not a degrade.""" app = FastAPI() mock_cache = MagicMock() - mock_cache.get.side_effect = Exception("redis down") + mock_cache.get.side_effect = RedisConnectionError("redis down") app.state.redis_cache = mock_cache app.state.redis_cache_ttl = 30 @cached("nodes") async def handler(request: Request): - return {"items": ["fallback"], "total": 1} + return {"items": ["should not be reached"], "total": 1} scope = { "type": "http", @@ -363,8 +324,36 @@ class TestCachedDecorator: request = Request(scope) request._state = State() - result = await handler(request=request) - assert result == {"items": ["fallback"], "total": 1} + with pytest.raises(HTTPException) as exc_info: + await handler(request=request) + assert exc_info.value.status_code == 503 + + def test_sync_wrapper_redis_error_returns_503(self): + """The sync wrapper shares the mandatory-backend contract.""" + app = FastAPI() + mock_cache = MagicMock() + mock_cache.get.side_effect = RedisConnectionError("redis down") + app.state.redis_cache = mock_cache + app.state.redis_cache_ttl = 30 + + @cached("nodes") + def handler(request: Request): + return {"items": ["never"], "total": 1} + + scope = { + "type": "http", + "query_string": b"", + "headers": [], + "app": app, + } + from starlette.datastructures import State + + request = Request(scope) + request._state = State() + + with pytest.raises(HTTPException) as exc_info: + handler(request=request) + assert exc_info.value.status_code == 503 async def test_no_request_raises_error(self): @cached("nodes") @@ -544,11 +533,12 @@ class TestCachedDecorator: envelope = json.loads(set_call[0][1]) assert envelope["body"] == ["a", "b"] - async def test_cache_set_error_falls_through(self): + async def test_redis_error_on_store_returns_503(self): + """Redis is mandatory: a SET error after the handler also 503s.""" app = FastAPI() mock_cache = MagicMock() mock_cache.get.return_value = None - mock_cache.set.side_effect = Exception("set error") + mock_cache.set.side_effect = RedisError("set error") app.state.redis_cache = mock_cache app.state.redis_cache_ttl = 30 @@ -567,29 +557,9 @@ class TestCachedDecorator: request = Request(scope) request._state = State() - result = await handler(request=request) - assert result == {"items": ["ok"]} - - async def test_no_cache_on_app_state_calls_handler(self): - app = FastAPI() - - @cached("nodes") - async def handler(request: Request): - return {"items": ["direct"]} - - scope = { - "type": "http", - "query_string": b"", - "headers": [], - "app": app, - } - from starlette.datastructures import State - - request = Request(scope) - request._state = State() - - result = await handler(request=request) - assert result == {"items": ["direct"]} + with pytest.raises(HTTPException) as exc_info: + await handler(request=request) + assert exc_info.value.status_code == 503 class TestCachedEtag: @@ -789,10 +759,12 @@ class TestCachedEtag: assert response.status_code == 304 async def test_cache_control_ttl_always_set(self): - """Even when Redis is NullCache, request.state.cache_control_ttl - is set so the middleware can emit Cache-Control.""" + """request.state.cache_control_ttl is set on every request (before + any cache interaction) so the middleware can emit Cache-Control.""" app = FastAPI() - app.state.redis_cache = NullCache() + mock_cache = MagicMock() + mock_cache.get.return_value = None + app.state.redis_cache = mock_cache app.state.redis_cache_ttl = 30 @cached("nodes") @@ -805,7 +777,9 @@ class TestCachedEtag: async def test_cache_control_ttl_uses_overridden_setting(self): app = FastAPI() - app.state.redis_cache = NullCache() + mock_cache = MagicMock() + mock_cache.get.return_value = None + app.state.redis_cache = mock_cache app.state.redis_cache_ttl = 30 app.state.redis_cache_ttl_dashboard = 90 @@ -970,13 +944,20 @@ class TestCacheControlMiddleware: monkeypatch.setattr( client_no_auth.app.state, "api_cache_control_enabled", False ) - # Remove any pre-existing cache backend for the duration of the test; - # ``monkeypatch.delattr`` with raising=False is a no-op when the - # attribute is absent. - monkeypatch.delattr(client_no_auth.app.state, "redis_cache", raising=False) response = client_no_auth.get("/api/v1/nodes") assert "cache-control" not in response.headers + def test_cached_get_returns_503_when_redis_down(self, client_no_auth): + """FR3: a Redis outage during request handling yields 503, not a + silent degrade to direct database queries.""" + mock_cache = MagicMock() + mock_cache.get.side_effect = RedisConnectionError("redis down") + client_no_auth.app.state.redis_cache = mock_cache + client_no_auth.app.state.redis_cache_ttl = 30 + response = client_no_auth.get("/api/v1/nodes") + assert response.status_code == 503 + assert response.json()["detail"] == "cache backend unavailable" + def test_kill_switch_preserves_x_cache_header(self, client_no_auth, monkeypatch): """X-Cache is observability, not a client-caching directive, so the kill switch should not suppress it.""" @@ -1003,28 +984,12 @@ class TestCacheControlMiddleware: class TestLifespanRedis: - async def test_lifespan_creates_null_cache_when_disabled(self): + async def test_lifespan_constructs_backend_from_app_state(self): import meshcore_hub.api.app as app_module from meshcore_hub.api.app import lifespan app = FastAPI() app.state.database_url = "postgresql+psycopg2://u:p@testhost:5432/testdb" - app.state.redis_enabled = False - app_module._db_manager = None - - mock_db = MagicMock() - mock_db.adispose = AsyncMock() - with patch.object(app_module, "DatabaseManager", return_value=mock_db): - async with lifespan(app): - assert isinstance(app.state.redis_cache, NullCache) - - async def test_lifespan_creates_redis_cache_when_enabled(self): - import meshcore_hub.api.app as app_module - from meshcore_hub.api.app import lifespan - - app = FastAPI() - app.state.database_url = "postgresql+psycopg2://u:p@testhost:5432/testdb" - app.state.redis_enabled = True app.state.redis_host = "redis-host" app.state.redis_port = 6380 app.state.redis_db = 1 @@ -1037,6 +1002,7 @@ class TestLifespanRedis: with patch.object(app_module, "DatabaseManager", return_value=mock_db): with patch("meshcore_hub.common.redis.RedisCacheBackend") as mock_cls: mock_instance = MagicMock() + mock_instance.ping.return_value = True mock_cls.return_value = mock_instance async with lifespan(app): mock_cls.assert_called_once_with( @@ -1048,15 +1014,40 @@ class TestLifespanRedis: ) assert app.state.redis_cache is mock_instance + async def test_lifespan_logs_error_when_unreachable(self, caplog): + """Startup pings Redis; unreachable logs at ERROR but does not + raise (the readiness endpoint reports not_ready instead).""" + import meshcore_hub.api.app as app_module + from meshcore_hub.api.app import lifespan + + app = FastAPI() + app.state.database_url = "postgresql+psycopg2://u:p@testhost:5432/testdb" + app_module._db_manager = None + + mock_db = MagicMock() + mock_db.adispose = AsyncMock() + with patch.object(app_module, "DatabaseManager", return_value=mock_db): + with patch("meshcore_hub.common.redis.RedisCacheBackend") as mock_cls: + mock_instance = MagicMock() + mock_instance.ping.return_value = False + mock_cls.return_value = mock_instance + with caplog.at_level("ERROR", logger="meshcore_hub.api.app"): + async with lifespan(app): + pass + + assert any("unreachable at startup" in r.message for r in caplog.records), [ + r.message for r in caplog.records + ] + async def test_lifespan_closes_cache_on_shutdown(self): import meshcore_hub.api.app as app_module from meshcore_hub.api.app import lifespan app = FastAPI() app.state.database_url = "postgresql+psycopg2://u:p@testhost:5432/testdb" - app.state.redis_enabled = True mock_cache = MagicMock() + mock_cache.ping.return_value = True mock_db_manager = MagicMock() mock_db_manager.adispose = AsyncMock() app_module._db_manager = None @@ -1091,40 +1082,47 @@ class TestXCacheMiddleware: response = client_no_auth.get("/api/v1/nodes") assert response.headers.get("x-cache") == "MISS" - def test_no_x_cache_header_when_no_cache_status(self, client_no_auth): - if hasattr(client_no_auth.app.state, "redis_cache"): - del client_no_auth.app.state.redis_cache - response = client_no_auth.get("/api/v1/nodes") + def test_no_x_cache_header_when_no_cache_status(self, client_no_auth, sample_node): + # Uncached endpoints never set request.state.cache_status, so the + # middleware must not emit an X-Cache header for them. + response = client_no_auth.get(f"/api/v1/nodes/{sample_node.public_key}") + assert response.status_code == 200 assert "x-cache" not in response.headers class TestHealthReadyRedis: - def test_health_ready_omits_redis_when_disabled(self, client_no_auth): - if hasattr(client_no_auth.app.state, "redis_cache"): - del client_no_auth.app.state.redis_cache - client_no_auth.app.state.redis_enabled = False - response = client_no_auth.get("/health/ready") - assert response.status_code == 200 - data = response.json() - assert "redis" not in data - def test_health_ready_reports_connected(self, client_no_auth): mock_cache = MagicMock() mock_cache.ping.return_value = True client_no_auth.app.state.redis_cache = mock_cache - client_no_auth.app.state.redis_enabled = True response = client_no_auth.get("/health/ready") assert response.status_code == 200 assert response.json()["redis"] == "connected" def test_health_ready_reports_unreachable(self, client_no_auth): + """FR2: Redis is required — unreachable flips overall status to + not_ready and the response to 503 (same contract as the database).""" mock_cache = MagicMock() mock_cache.ping.return_value = False client_no_auth.app.state.redis_cache = mock_cache - client_no_auth.app.state.redis_enabled = True response = client_no_auth.get("/health/ready") - assert response.status_code == 200 - assert response.json()["redis"] == "unreachable" + assert response.status_code == 503 + data = response.json() + assert data["redis"] == "unreachable" + assert data["status"] == "not_ready" + + def test_health_ready_reports_unreachable_when_backend_missing( + self, client_no_auth + ): + """No backend on app.state (lifespan never ran) is unreachable — + mandatory infrastructure has no silent-degrade path.""" + if hasattr(client_no_auth.app.state, "redis_cache"): + del client_no_auth.app.state.redis_cache + response = client_no_auth.get("/health/ready") + assert response.status_code == 503 + data = response.json() + assert data["redis"] == "unreachable" + assert data["status"] == "not_ready" def test_health_ready_returns_503_when_db_down(self, client_no_auth): """Readiness probes judge by status code: DB down must be a 503.""" @@ -1147,7 +1145,8 @@ class TestHealthReadyRedis: class TestCliRedis: - def test_redis_enabled_shows_banner(self): + def test_redis_target_always_echoed(self): + """Redis is required: the CLI always prints the connection target.""" from click.testing import CliRunner from meshcore_hub.api.cli import api @@ -1161,29 +1160,12 @@ class TestCliRedis: ) result = runner.invoke( api, - ["--redis-enabled", "--redis-host", "myredis"], + ["--redis-host", "myredis"], catch_exceptions=False, ) - assert "Redis enabled: True" in result.output assert "Redis: myredis:6379/0" in result.output assert "Redis key prefix: hub" in result.output - def test_redis_disabled_hides_details(self): - from click.testing import CliRunner - - from meshcore_hub.api.cli import api - - runner = CliRunner() - with patch("uvicorn.run"): - with patch("meshcore_hub.common.config.get_api_settings") as mock_settings: - mock_settings.return_value = MagicMock( - data_home="/tmp/test", - effective_database_url="postgresql+psycopg2://u:p@testhost:5432/testdb", - ) - result = runner.invoke(api, catch_exceptions=False) - assert "Redis enabled: False" in result.output - assert "Redis:" not in result.output.replace("Redis enabled: False", "") - def test_redis_params_passed_to_create_app(self): from click.testing import CliRunner @@ -1201,7 +1183,6 @@ class TestCliRedis: runner.invoke( api, [ - "--redis-enabled", "--redis-host", "rhost", "--redis-port", @@ -1221,7 +1202,7 @@ class TestCliRedis: catch_exceptions=False, ) call_kwargs = mock_create_app.call_args[1] - assert call_kwargs["redis_enabled"] is True + assert "redis_enabled" not in call_kwargs assert call_kwargs["redis_host"] == "rhost" assert call_kwargs["redis_port"] == 6380 assert call_kwargs["redis_db"] == 2 @@ -1502,20 +1483,8 @@ class TestCacheInvalidationHelpers: cache.delete.assert_any_call("/api/v1/dashboard") assert cache.delete.call_count == 2 - def test_helpers_are_noop_when_cache_missing(self): - # No redis_cache attribute on state — must not raise. - from meshcore_hub.api import cache_invalidation as inv - - request = _make_request_with_cache(cache=None) - inv.invalidate_channels(request) - inv.invalidate_routes(request) - inv.invalidate_nodes(request) - inv.invalidate_profiles(request) - inv.invalidate_messages(request) - inv.invalidate_advertisements(request) - inv.invalidate_dashboard(request) - def test_helpers_swallow_backend_errors(self): + """FR4: invalidation runs after a committed write and never raises.""" from meshcore_hub.api import cache_invalidation as inv cache = MagicMock() @@ -1933,57 +1902,26 @@ class TestInvalidationLogging: for m in messages ), f"ok line missing or malformed: {messages}" - def test_drop_logs_warning_on_backend_error(self, caplog): + def test_drop_logs_error_on_backend_error(self, caplog): + """FR4: backend errors during invalidation log at ERROR (never raise).""" from meshcore_hub.api.cache_invalidation import invalidate_channels cache = MagicMock() cache.delete.side_effect = Exception("redis down") request = _make_request_with_cache(cache) - with caplog.at_level("WARNING", logger="meshcore_hub.api.cache_invalidation"): + with caplog.at_level("ERROR", logger="meshcore_hub.api.cache_invalidation"): invalidate_channels(request) - # Must not raise; warning must carry prefix + error text. + # Must not raise; the error record must carry prefix + error text. + error_records = [r for r in caplog.records if r.levelname == "ERROR"] assert any( "Cache invalidate error" in r.message and "prefix=/api/v1/channels" in r.message and "redis down" in r.message - for r in caplog.records + for r in error_records ), [r.message for r in caplog.records] - def test_drop_logs_skipped_when_no_backend(self, caplog): - from meshcore_hub.api.cache_invalidation import invalidate_nodes - - # No redis_cache attribute on app.state. - request = _make_request_with_cache(cache=None) - - with caplog.at_level("DEBUG", logger="meshcore_hub.api.cache_invalidation"): - invalidate_nodes(request) - - assert any( - "Cache invalidate skipped" in r.message and "prefix=nodes" in r.message - for r in caplog.records - ), [r.message for r in caplog.records] - - def test_drop_logs_backend_name_distinguishes_nullcache(self, caplog): - """If NullCache is wired in, the start log must say so. - - Catches the 'REDIS_ENABLED is actually false in production' case - in one log line. - """ - from meshcore_hub.api.cache_invalidation import invalidate_routes - - null_cache = NullCache() - request = _make_request_with_cache(null_cache) - - with caplog.at_level("INFO", logger="meshcore_hub.api.cache_invalidation"): - invalidate_routes(request) - - messages = [r.message for r in caplog.records] - assert any( - "backend=NullCache" in m and "prefix=/api/v1/routes" in m for m in messages - ), f"expected backend=NullCache in start log, got: {messages}" - def test_redis_delete_logs_keys_deleted_count(self, caplog): with patch("redis.Redis") as mock_redis_cls: mock_client = MagicMock() @@ -2027,24 +1965,6 @@ class TestInvalidationLogging: assert "prefix=/api/v1/routes" in msg assert "full_prefix=hub:/api/v1/routes" in msg - def test_redis_delete_warning_includes_full_prefix(self, caplog): - with patch("redis.Redis") as mock_redis_cls: - mock_client = MagicMock() - mock_redis_cls.return_value = mock_client - mock_client.scan.side_effect = Exception("scan timeout") - - backend = RedisCacheBackend(key_prefix="hub") - with caplog.at_level("WARNING", logger="meshcore_hub.common.redis"): - backend.delete("nodes") - - warning_records = [r for r in caplog.records if r.levelname == "WARNING"] - assert len(warning_records) == 1 - msg = warning_records[0].message - assert "Redis DELETE error" in msg - assert "prefix=nodes" in msg - assert "full_prefix=hub:nodes" in msg - assert "scan timeout" in msg - def test_redis_delete_multi_page_scan_logs_total_keys(self, caplog): """Multi-page SCAN must accumulate keys_deleted across iterations.""" with patch("redis.Redis") as mock_redis_cls: @@ -2064,12 +1984,3 @@ class TestInvalidationLogging: msg = info_records[0].message assert "keys_deleted=3" in msg assert "scan_iterations=2" in msg - - def test_nullcache_delete_emits_debug_log(self, caplog): - cache = NullCache() - with caplog.at_level("DEBUG", logger="meshcore_hub.common.redis"): - cache.delete("nodes") - assert any( - "NullCache delete" in r.message and "prefix=nodes" in r.message - for r in caplog.records - ), [r.message for r in caplog.records] diff --git a/tests/test_api/test_dashboard.py b/tests/test_api/test_dashboard.py index 19cf6a3..6ce5353 100644 --- a/tests/test_api/test_dashboard.py +++ b/tests/test_api/test_dashboard.py @@ -186,6 +186,14 @@ class TestDashboardStats: """ from meshcore_hub.common.config import CollectorSettings + # Always-miss cache: the second GET below must recompute (the real + # backend would serve a HIT for the identical request after the + # monkeypatched retention changed the expected result). + from unittest.mock import MagicMock + + client_no_auth.app.state.redis_cache = MagicMock() + client_no_auth.app.state.redis_cache.get.return_value = None + now = datetime.now(timezone.utc) api_db_session.add_all( [