From 5e398ce4a3171b2c6a4a763fb9d042c26681b3d6 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 12:19:18 +0100 Subject: [PATCH 1/5] docs: legacy-redis-warning design (#868) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../2026-11-07-legacy-redis-warning-design.md | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 docs/superpowers/specs/2026-11-07-legacy-redis-warning-design.md diff --git a/docs/superpowers/specs/2026-11-07-legacy-redis-warning-design.md b/docs/superpowers/specs/2026-11-07-legacy-redis-warning-design.md new file mode 100644 index 00000000..ce9f8e2f --- /dev/null +++ b/docs/superpowers/specs/2026-11-07-legacy-redis-warning-design.md @@ -0,0 +1,68 @@ +# Warn about the legacy broker variables only when SM_REDIS_URL can replace them (#868) + +## Goal & scope + +`warn_on_legacy_redis_vars` tells every deployment that sets +`SM_BG_TASKS_BROKER_URL` / `SM_BG_TASKS_RESULT_BACKEND` to use `SM_REDIS_URL` +instead. `SM_REDIS_URL` seeds *both* fields with one Redis URL, so the advice +only works when the value is a Redis URL. A production host that runs tasks +in-process must set `memory://` + `cache+memory://` (the localhost refusal +blocks the defaults, and no single URL is valid for both), and gets two +warnings on every boot that it cannot act on. + +In scope: warn only for a value `SM_REDIS_URL` could express; tests; docs; +CHANGELOG. +Out of scope: changing the production localhost refusal or eager-mode +defaults. + +## Approach + +Chosen: a legacy variable is warned about only when its value uses a Redis +scheme: `redis`, `rediss`, `redis+socket` or `sentinel`. Celery and kombu accept +each of these as both the broker and the result backend, so `SM_REDIS_URL` can +carry it. Any other scheme (`memory://`, `cache+memory://`, `amqp://`, `rpc://`, +`sqs://`, `db+…`) stays quiet. Those variables are the only way to express such +a value, so they are not really deprecated for it. + +Rejected: +- Skip the localhost refusal when `task_always_eager` is on, or default to + in-process transports. That is a behaviour change to a production guard, and + `broker_url` is read by more than the task runner (the worker, beat, the + health check). It doesn't fit a warning fix. +- Silence only `memory://` and `cache+memory://`. That still nags RabbitMQ or + SQS hosts with advice they cannot follow. + +## Design + +`modules/background_tasks/background_tasks/core/settings_env_checks.py`: +- add `_REDIS_SCHEMES = frozenset({"redis", "rediss", "redis+socket", "sentinel"})`; +- in `warn_on_legacy_redis_vars`, skip a variable whose value's scheme (the text + before `://`, lowercased) is not in that set; +- extend the docstring with the reason. + +## Decisions made + +- The check is per variable. A host with a Redis broker and an `rpc://` backend + still gets the warning for the broker, which it can move to `SM_REDIS_URL`. +- No new setting and no new log line. +- Docs: a sentence in `docs/reference/env-vars.md` § Background tasks saying the + two variables stay the way to set non-Redis transports (the in-process pair + for a production host with no Redis), and warn only for Redis URLs. +- CHANGELOG `## [Unreleased]` → Fixed, citing #868. + +## Testing + +`modules/background_tasks/tests/core/settings/test_background_tasks_redis_url.py`: +- the in-process pair (`memory://` + `cache+memory://`) under + `SM_ENVIRONMENT=production` boots and logs no deprecation warning; +- an `amqp://` broker logs no warning; +- `rediss://` and `sentinel://` values still warn (parametrized); +- the existing tests (`redis://` legacy values warn, `SM_REDIS_URL` alone is + quiet) stay green. + +No browser QA: settings-only change (`--skip-browser`). + +## Risks + +Low. A deployment using `redis://` keeps the warning. Only hosts that cannot +follow the advice stop seeing it. From 88d9d3f739dba976140791c952247567287f52ed Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 12:20:45 +0100 Subject: [PATCH 2/5] fix(background_tasks): warn about the legacy broker variables only for Redis URLs (#868) SM_REDIS_URL seeds both the broker and the result backend with one URL, so it can replace SM_BG_TASKS_BROKER_URL / SM_BG_TASKS_RESULT_BACKEND only when they hold a Redis URL. In-process (memory:// + cache+memory://), AMQP, SQS and rpc:// values are only expressible through those variables and no longer warn. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 1 + docs/reference/env-vars.md | 2 + .../plans/2026-11-07-legacy-redis-warning.md | 16 ++++++ .../core/settings_env_checks.py | 12 +++- .../test_background_tasks_redis_url.py | 57 +++++++++++++++++++ 5 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 docs/superpowers/plans/2026-11-07-legacy-redis-warning.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 59790d54..23aeb2ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ All notable changes to this project are documented in this file. The format is b ### Fixed +- `background_tasks`: `SM_BG_TASKS_BROKER_URL` and `SM_BG_TASKS_RESULT_BACKEND` are reported as deprecated only when they hold a Redis URL (`redis`, `rediss`, `redis+socket`, `sentinel`), the only values `SM_REDIS_URL` can replace. A production host that runs tasks in-process with `memory://` + `cache+memory://`, or uses an AMQP, SQS or `rpc://` transport, no longer logs a warning on every boot that it cannot act on (#868). - `SecurityHeadersMiddleware` no longer overwrites a `Content-Security-Policy` that a response sets. The app policy is sent beside it as a second header and browsers enforce both, so a page can tighten the policy (for example `frame-ancestors 'none'` on a public page) but never relax it. Before, only a policy containing `sandbox` survived and any other was dropped silently. **Behaviour change:** a response that set a looser policy, relying on it being replaced, now has both policies enforced: remove that header, or make it a policy the page can live with alongside the app policy (#870). - `alembic check` and SQLite autogenerate no longer report the `file_storage` browse index (`sa.desc(sa.column("created_at"))` instead of `sa.text("created_at DESC")`) or `users_access_token.expires_at` (now `AwareDateTime`) as changed, so the host's `alembic check` on SQLite is clean. `make ci-doctor` now runs `alembic check` against its throwaway SQLite database, and a module index written with `sa.text("col DESC")` is no longer reported as changed on SQLite (#869). - `background_tasks`: the Redis invalidation listener now opens the connection a publish uses (one `PING` after every successful subscribe), so the first invalidation after boot or after a Redis outage no longer spends part of its 1 s budget connecting. Under load that connect was cancelled and the message was lost; other workers kept the stale value until its TTL (#880). diff --git a/docs/reference/env-vars.md b/docs/reference/env-vars.md index 934adfa2..6400fab9 100644 --- a/docs/reference/env-vars.md +++ b/docs/reference/env-vars.md @@ -101,6 +101,8 @@ Three fields are still read from the environment, because they have to work *bef | `SM_BG_TASKS_RESULT_BACKEND` | `redis://localhost:6379/1` | Same reasoning as the broker. | | `SM_BG_TASKS_TASK_ALWAYS_EAGER` | `false` | Read at module-import time, so test suites that never run the FastAPI lifespan can still flip it. Runs tasks synchronously in the calling process. | +For a Redis broker and result backend, set `SM_REDIS_URL` instead: the two per-field variables log a deprecation warning when they hold a Redis URL. They stay the way to set any other transport, such as `memory://` + `cache+memory://` for a production host that runs tasks in-process with no Redis, or an AMQP broker, and those values log no warning. + ## File storage module File storage config is now **DB-backed** — edit it in the admin UI at `/admin/settings/` under Files. The key fields and defaults: diff --git a/docs/superpowers/plans/2026-11-07-legacy-redis-warning.md b/docs/superpowers/plans/2026-11-07-legacy-redis-warning.md new file mode 100644 index 00000000..9e54bacf --- /dev/null +++ b/docs/superpowers/plans/2026-11-07-legacy-redis-warning.md @@ -0,0 +1,16 @@ +# Plan: warn about the legacy broker variables only for Redis URLs (#868) + +Design: `docs/superpowers/specs/2026-11-07-legacy-redis-warning-design.md`. + +1. **Tests first** (`modules/background_tasks/tests/core/settings/test_background_tasks_redis_url.py`): + in-process pair under production is quiet; an `amqp://` broker is quiet; + `rediss`, `sentinel` and an upper-case `REDIS` scheme still warn; the check + is per variable. Done when the three quiet/per-variable tests fail on main. +2. **Fix** (`background_tasks/core/settings_env_checks.py`): `_REDIS_SCHEMES` + and a scheme check in `warn_on_legacy_redis_vars`. Done when the settings + tests pass. +3. **Docs**: a paragraph under the Background tasks table in + `docs/reference/env-vars.md`; CHANGELOG Fixed entry citing #868. +4. **Verify**: `make lint`, `make test-py`. + +All tasks are sequential and small; none is hard. diff --git a/modules/background_tasks/background_tasks/core/settings_env_checks.py b/modules/background_tasks/background_tasks/core/settings_env_checks.py index 57de91f4..aab1386f 100644 --- a/modules/background_tasks/background_tasks/core/settings_env_checks.py +++ b/modules/background_tasks/background_tasks/core/settings_env_checks.py @@ -15,6 +15,10 @@ logger = logging.getLogger("background_tasks.settings") +# Schemes Celery accepts as both broker and result backend, so one +# SM_REDIS_URL can carry them. +_REDIS_SCHEMES = frozenset({"redis", "rediss", "redis+socket", "sentinel"}) + def warn_on_legacy_redis_vars(broker_url: str, result_backend: str) -> None: """Nudge deployments off the per-field names onto ``SM_REDIS_URL``. @@ -25,12 +29,18 @@ def warn_on_legacy_redis_vars(broker_url: str, result_backend: str) -> None: than in the default factory because ``env_prefix`` means the env source answers these names before any default is consulted, so the factory never sees them. + + Only a Redis URL is warned about (#868): for anything else, such as the + in-process ``memory://`` + ``cache+memory://`` pair or an AMQP broker, + ``SM_REDIS_URL`` can't express the value and these vars are the way to + set it. """ for value, legacy_var in ( (broker_url, f"{ENV_PREFIX}BROKER_URL"), (result_backend, f"{ENV_PREFIX}RESULT_BACKEND"), ): - if os.environ.get(legacy_var) == value: + scheme = value.partition("://")[0].lower() + if os.environ.get(legacy_var) == value and scheme in _REDIS_SCHEMES: logger.warning( "%s is deprecated; use %s instead (one URL seeds both the " "broker and the result backend).", diff --git a/modules/background_tasks/tests/core/settings/test_background_tasks_redis_url.py b/modules/background_tasks/tests/core/settings/test_background_tasks_redis_url.py index 48f83850..bb4ca97b 100644 --- a/modules/background_tasks/tests/core/settings/test_background_tasks_redis_url.py +++ b/modules/background_tasks/tests/core/settings/test_background_tasks_redis_url.py @@ -145,3 +145,60 @@ def test_legacy_var_still_beats_redis_url_and_the_db(monkeypatch: pytest.MonkeyP assert settings.broker_url == "redis://legacy:6379/0" assert settings.result_backend == "redis://cache:6379/2" + + +def test_in_process_transports_boot_production_without_a_warning( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """#868: a production host with no Redis runs tasks in-process. No single + SM_REDIS_URL can name both halves, so the per-field vars are the only way + to say it and must not be called deprecated.""" + monkeypatch.setenv("SM_ENVIRONMENT", "production") + monkeypatch.setenv("SM_BG_TASKS_BROKER_URL", "memory://") + monkeypatch.setenv("SM_BG_TASKS_RESULT_BACKEND", "cache+memory://") + + with caplog.at_level(logging.WARNING): + settings = BackgroundTasksSettings() + + assert settings.broker_url == "memory://" + assert settings.result_backend == "cache+memory://" + assert "deprecated" not in caplog.text.lower() + + +def test_non_redis_broker_logs_no_warning( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + monkeypatch.setenv("SM_BG_TASKS_BROKER_URL", "amqp://guest@rabbit:5672//") + + with caplog.at_level(logging.WARNING): + BackgroundTasksSettings() + + assert "deprecated" not in caplog.text.lower() + + +@pytest.mark.parametrize( + "url", ["rediss://cache:6380/0", "sentinel://sentinel:26379/0", "REDIS://cache:6379/0"] +) +def test_every_redis_scheme_still_warns( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture, url: str +) -> None: + monkeypatch.setenv("SM_BG_TASKS_BROKER_URL", url) + + with caplog.at_level(logging.WARNING): + BackgroundTasksSettings() + + assert "SM_BG_TASKS_BROKER_URL is deprecated" in caplog.text + + +def test_warning_is_per_variable( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """A Redis broker can move to SM_REDIS_URL even when the backend can't.""" + monkeypatch.setenv("SM_BG_TASKS_BROKER_URL", "redis://old:6379/4") + monkeypatch.setenv("SM_BG_TASKS_RESULT_BACKEND", "rpc://") + + with caplog.at_level(logging.WARNING): + BackgroundTasksSettings() + + assert "SM_BG_TASKS_BROKER_URL is deprecated" in caplog.text + assert "SM_BG_TASKS_RESULT_BACKEND" not in caplog.text From 9d988e1b5d429eb04f3c673ab18eca3df0f93210 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 12:29:55 +0100 Subject: [PATCH 3/5] fix: address code review findings (round 1, pass 1) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- docs/framework/invalidation.md | 2 +- .../background_tasks/background_tasks/core/settings.py | 10 ++++++---- .../background_tasks/core/settings_env_checks.py | 3 ++- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/docs/framework/invalidation.md b/docs/framework/invalidation.md index c834fc97..05c35c41 100644 --- a/docs/framework/invalidation.md +++ b/docs/framework/invalidation.md @@ -73,7 +73,7 @@ A transport failure never propagates. The caller has already committed; turning ## Operating it -`background_tasks` installs the transport during `on_startup`, on its own `broker_url` — which resolves as `SM_BG_TASKS_BROKER_URL` (deprecated) → `SM_REDIS_URL` → stored override → the localhost default. So it always follows Celery's broker, by construction rather than by convention. +`background_tasks` installs the transport during `on_startup`, on its own `broker_url` — which resolves as `SM_BG_TASKS_BROKER_URL` (deprecated for a Redis URL) → `SM_REDIS_URL` → stored override → the localhost default. So it always follows Celery's broker, by construction rather than by convention. | Setting | Default | What it does | |---|---|---| diff --git a/modules/background_tasks/background_tasks/core/settings.py b/modules/background_tasks/background_tasks/core/settings.py index 74ce1fa9..18a8359c 100644 --- a/modules/background_tasks/background_tasks/core/settings.py +++ b/modules/background_tasks/background_tasks/core/settings.py @@ -18,10 +18,12 @@ ``SM_BG_TASKS_BROKER_URL`` and ``SM_BG_TASKS_RESULT_BACKEND`` still work and take precedence over ``SM_REDIS_URL`` — a deployment that set them meant them — -but log a deprecation warning. Being real env reads, they also beat a stored -value (env → DB → default, GH #486). They are answered by pydantic's own env source -now that ``env_prefix`` is set (GH #283), which is also what stops every other -field being readable from its bare, unprefixed name. +but log a deprecation warning when they hold a Redis URL (any other transport, +such as ``memory://``, is theirs to set and logs nothing, GH #868). Being real +env reads, they also beat a stored value (env → DB → default, GH #486). They +are answered by pydantic's own env source now that ``env_prefix`` is set +(GH #283), which is also what stops every other field being readable from its +bare, unprefixed name. The other env read is ``SM_ENVIRONMENT``, consulted by the ``@model_validator`` to refuse a localhost broker in production — that's a diff --git a/modules/background_tasks/background_tasks/core/settings_env_checks.py b/modules/background_tasks/background_tasks/core/settings_env_checks.py index aab1386f..1ff0d63c 100644 --- a/modules/background_tasks/background_tasks/core/settings_env_checks.py +++ b/modules/background_tasks/background_tasks/core/settings_env_checks.py @@ -72,7 +72,8 @@ def forbid_localhost_broker_in_production(broker_url: str, result_backend: str) f"Set {ENV_REDIS_URL} on the container to the Redis service host " "(e.g. redis://redis:6379/0) — one URL seeds both the broker and " f"the result backend. ({env_names} still override it per field, " - "but are deprecated.) These are read before the DB-backed " + "but are deprecated for a Redis URL; any other transport has no " + "other way to be set.) These are read before the DB-backed " "settings exist, so an environment variable is what satisfies " "this at boot." ) From cbf5ffe0492390fde2deb17fcbf82f1511676bad Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 12:33:41 +0100 Subject: [PATCH 4/5] fix: address code review findings (round 1, pass 2) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 2 +- docs/reference/env-vars.md | 2 +- .../background_tasks/core/settings_env_checks.py | 9 ++++----- .../core/settings/test_background_tasks_redis_url.py | 8 +++++++- 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23aeb2ab..fb8d7b37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ All notable changes to this project are documented in this file. The format is b ### Fixed -- `background_tasks`: `SM_BG_TASKS_BROKER_URL` and `SM_BG_TASKS_RESULT_BACKEND` are reported as deprecated only when they hold a Redis URL (`redis`, `rediss`, `redis+socket`, `sentinel`), the only values `SM_REDIS_URL` can replace. A production host that runs tasks in-process with `memory://` + `cache+memory://`, or uses an AMQP, SQS or `rpc://` transport, no longer logs a warning on every boot that it cannot act on (#868). +- `background_tasks`: `SM_BG_TASKS_BROKER_URL` and `SM_BG_TASKS_RESULT_BACKEND` are reported as deprecated only when they hold a Redis URL (`redis`, `rediss`, `redis+socket`, `sentinel`), the only values `SM_REDIS_URL` can replace. A production host that runs tasks in-process with `memory://` + `cache+memory://`, or uses an AMQP, SQS or `rpc://` transport, no longer logs a warning on every boot that it cannot act on, and the production localhost-broker error no longer calls those vars deprecated for non-Redis values (#868). - `SecurityHeadersMiddleware` no longer overwrites a `Content-Security-Policy` that a response sets. The app policy is sent beside it as a second header and browsers enforce both, so a page can tighten the policy (for example `frame-ancestors 'none'` on a public page) but never relax it. Before, only a policy containing `sandbox` survived and any other was dropped silently. **Behaviour change:** a response that set a looser policy, relying on it being replaced, now has both policies enforced: remove that header, or make it a policy the page can live with alongside the app policy (#870). - `alembic check` and SQLite autogenerate no longer report the `file_storage` browse index (`sa.desc(sa.column("created_at"))` instead of `sa.text("created_at DESC")`) or `users_access_token.expires_at` (now `AwareDateTime`) as changed, so the host's `alembic check` on SQLite is clean. `make ci-doctor` now runs `alembic check` against its throwaway SQLite database, and a module index written with `sa.text("col DESC")` is no longer reported as changed on SQLite (#869). - `background_tasks`: the Redis invalidation listener now opens the connection a publish uses (one `PING` after every successful subscribe), so the first invalidation after boot or after a Redis outage no longer spends part of its 1 s budget connecting. Under load that connect was cancelled and the message was lost; other workers kept the stale value until its TTL (#880). diff --git a/docs/reference/env-vars.md b/docs/reference/env-vars.md index 6400fab9..437bd362 100644 --- a/docs/reference/env-vars.md +++ b/docs/reference/env-vars.md @@ -101,7 +101,7 @@ Three fields are still read from the environment, because they have to work *bef | `SM_BG_TASKS_RESULT_BACKEND` | `redis://localhost:6379/1` | Same reasoning as the broker. | | `SM_BG_TASKS_TASK_ALWAYS_EAGER` | `false` | Read at module-import time, so test suites that never run the FastAPI lifespan can still flip it. Runs tasks synchronously in the calling process. | -For a Redis broker and result backend, set `SM_REDIS_URL` instead: the two per-field variables log a deprecation warning when they hold a Redis URL. They stay the way to set any other transport, such as `memory://` + `cache+memory://` for a production host that runs tasks in-process with no Redis, or an AMQP broker, and those values log no warning. +For a Redis broker and result backend, set `SM_REDIS_URL` instead: the two per-field variables log a deprecation warning when they hold a Redis URL (`redis://`, `rediss://`, `redis+socket://` or `sentinel://`). They stay the way to set any other transport, such as `memory://` + `cache+memory://` for a production host that runs tasks in-process with no Redis, or an AMQP broker, and those values log no warning. ## File storage module diff --git a/modules/background_tasks/background_tasks/core/settings_env_checks.py b/modules/background_tasks/background_tasks/core/settings_env_checks.py index 1ff0d63c..ef225a84 100644 --- a/modules/background_tasks/background_tasks/core/settings_env_checks.py +++ b/modules/background_tasks/background_tasks/core/settings_env_checks.py @@ -71,9 +71,8 @@ def forbid_localhost_broker_in_production(broker_url: str, result_backend: str) f"{names} must not point at localhost when SM_ENVIRONMENT={env!r}. " f"Set {ENV_REDIS_URL} on the container to the Redis service host " "(e.g. redis://redis:6379/0) — one URL seeds both the broker and " - f"the result backend. ({env_names} still override it per field, " - "but are deprecated for a Redis URL; any other transport has no " - "other way to be set.) These are read before the DB-backed " - "settings exist, so an environment variable is what satisfies " - "this at boot." + f"the result backend. ({env_names} still override it per field; " + "they are deprecated for a Redis URL, but stay the way to set any " + "other transport.) Environment variables are read before the " + "DB-backed settings exist, so one is what satisfies this at boot." ) diff --git a/modules/background_tasks/tests/core/settings/test_background_tasks_redis_url.py b/modules/background_tasks/tests/core/settings/test_background_tasks_redis_url.py index bb4ca97b..18bafa0c 100644 --- a/modules/background_tasks/tests/core/settings/test_background_tasks_redis_url.py +++ b/modules/background_tasks/tests/core/settings/test_background_tasks_redis_url.py @@ -177,7 +177,13 @@ def test_non_redis_broker_logs_no_warning( @pytest.mark.parametrize( - "url", ["rediss://cache:6380/0", "sentinel://sentinel:26379/0", "REDIS://cache:6379/0"] + "url", + [ + "rediss://cache:6380/0", + "sentinel://sentinel:26379/0", + "redis+socket:///run/redis.sock", + "REDIS://cache:6379/0", + ], ) def test_every_redis_scheme_still_warns( monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture, url: str From d1dffcffb77d4d80516de530cf8186121e599dc7 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 12:36:30 +0100 Subject: [PATCH 5/5] fix: address code review findings (round 1, pass 3) SM_REDIS_URL is also read through redis-py from_url (rate limiter, leases, invalidation transport), which rejects Celery's redis+socket and sentinel schemes, so only redis/rediss values are told to move to it. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 2 +- docs/reference/env-vars.md | 2 +- .../plans/2026-11-07-legacy-redis-warning.md | 8 ++++--- .../2026-11-07-legacy-redis-warning-design.md | 24 ++++++++++++------- .../core/settings_env_checks.py | 13 ++++++---- .../test_background_tasks_redis_url.py | 16 ++++++++++--- 6 files changed, 44 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb8d7b37..c4c5631d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ All notable changes to this project are documented in this file. The format is b ### Fixed -- `background_tasks`: `SM_BG_TASKS_BROKER_URL` and `SM_BG_TASKS_RESULT_BACKEND` are reported as deprecated only when they hold a Redis URL (`redis`, `rediss`, `redis+socket`, `sentinel`), the only values `SM_REDIS_URL` can replace. A production host that runs tasks in-process with `memory://` + `cache+memory://`, or uses an AMQP, SQS or `rpc://` transport, no longer logs a warning on every boot that it cannot act on, and the production localhost-broker error no longer calls those vars deprecated for non-Redis values (#868). +- `background_tasks`: `SM_BG_TASKS_BROKER_URL` and `SM_BG_TASKS_RESULT_BACKEND` are reported as deprecated only when they hold a Redis URL (`redis`, `rediss`), the only values `SM_REDIS_URL` can replace. A production host that runs tasks in-process with `memory://` + `cache+memory://`, or uses an AMQP, SQS or `rpc://` transport, no longer logs a warning on every boot that it cannot act on, and the production localhost-broker error no longer calls those vars deprecated for non-Redis values (#868). - `SecurityHeadersMiddleware` no longer overwrites a `Content-Security-Policy` that a response sets. The app policy is sent beside it as a second header and browsers enforce both, so a page can tighten the policy (for example `frame-ancestors 'none'` on a public page) but never relax it. Before, only a policy containing `sandbox` survived and any other was dropped silently. **Behaviour change:** a response that set a looser policy, relying on it being replaced, now has both policies enforced: remove that header, or make it a policy the page can live with alongside the app policy (#870). - `alembic check` and SQLite autogenerate no longer report the `file_storage` browse index (`sa.desc(sa.column("created_at"))` instead of `sa.text("created_at DESC")`) or `users_access_token.expires_at` (now `AwareDateTime`) as changed, so the host's `alembic check` on SQLite is clean. `make ci-doctor` now runs `alembic check` against its throwaway SQLite database, and a module index written with `sa.text("col DESC")` is no longer reported as changed on SQLite (#869). - `background_tasks`: the Redis invalidation listener now opens the connection a publish uses (one `PING` after every successful subscribe), so the first invalidation after boot or after a Redis outage no longer spends part of its 1 s budget connecting. Under load that connect was cancelled and the message was lost; other workers kept the stale value until its TTL (#880). diff --git a/docs/reference/env-vars.md b/docs/reference/env-vars.md index 437bd362..f0ca2984 100644 --- a/docs/reference/env-vars.md +++ b/docs/reference/env-vars.md @@ -101,7 +101,7 @@ Three fields are still read from the environment, because they have to work *bef | `SM_BG_TASKS_RESULT_BACKEND` | `redis://localhost:6379/1` | Same reasoning as the broker. | | `SM_BG_TASKS_TASK_ALWAYS_EAGER` | `false` | Read at module-import time, so test suites that never run the FastAPI lifespan can still flip it. Runs tasks synchronously in the calling process. | -For a Redis broker and result backend, set `SM_REDIS_URL` instead: the two per-field variables log a deprecation warning when they hold a Redis URL (`redis://`, `rediss://`, `redis+socket://` or `sentinel://`). They stay the way to set any other transport, such as `memory://` + `cache+memory://` for a production host that runs tasks in-process with no Redis, or an AMQP broker, and those values log no warning. +For a Redis broker and result backend, set `SM_REDIS_URL` instead: the two per-field variables log a deprecation warning when they hold a Redis URL (`redis://` or `rediss://`). They stay the way to set any other transport, such as `memory://` + `cache+memory://` for a production host that runs tasks in-process with no Redis, or an AMQP broker, and those values log no warning. ## File storage module diff --git a/docs/superpowers/plans/2026-11-07-legacy-redis-warning.md b/docs/superpowers/plans/2026-11-07-legacy-redis-warning.md index 9e54bacf..61ade4f6 100644 --- a/docs/superpowers/plans/2026-11-07-legacy-redis-warning.md +++ b/docs/superpowers/plans/2026-11-07-legacy-redis-warning.md @@ -4,13 +4,15 @@ Design: `docs/superpowers/specs/2026-11-07-legacy-redis-warning-design.md`. 1. **Tests first** (`modules/background_tasks/tests/core/settings/test_background_tasks_redis_url.py`): in-process pair under production is quiet; an `amqp://` broker is quiet; - `rediss`, `sentinel` and an upper-case `REDIS` scheme still warn; the check + `rediss` and an upper-case `REDIS` scheme still warn, `sentinel` does not; the check is per variable. Done when the three quiet/per-variable tests fail on main. 2. **Fix** (`background_tasks/core/settings_env_checks.py`): `_REDIS_SCHEMES` - and a scheme check in `warn_on_legacy_redis_vars`. Done when the settings + and a scheme check in `warn_on_legacy_redis_vars`; reword the production + localhost error and the `settings.py` docstring to match. Done when the settings tests pass. 3. **Docs**: a paragraph under the Background tasks table in - `docs/reference/env-vars.md`; CHANGELOG Fixed entry citing #868. + `docs/reference/env-vars.md`, the deprecation note in + `docs/framework/invalidation.md`; CHANGELOG Fixed entry citing #868. 4. **Verify**: `make lint`, `make test-py`. All tasks are sequential and small; none is hard. diff --git a/docs/superpowers/specs/2026-11-07-legacy-redis-warning-design.md b/docs/superpowers/specs/2026-11-07-legacy-redis-warning-design.md index ce9f8e2f..3de970ce 100644 --- a/docs/superpowers/specs/2026-11-07-legacy-redis-warning-design.md +++ b/docs/superpowers/specs/2026-11-07-legacy-redis-warning-design.md @@ -18,10 +18,12 @@ defaults. ## Approach Chosen: a legacy variable is warned about only when its value uses a Redis -scheme: `redis`, `rediss`, `redis+socket` or `sentinel`. Celery and kombu accept -each of these as both the broker and the result backend, so `SM_REDIS_URL` can -carry it. Any other scheme (`memory://`, `cache+memory://`, `amqp://`, `rpc://`, -`sqs://`, `db+…`) stays quiet. Those variables are the only way to express such +scheme: `redis` or `rediss`. `SM_REDIS_URL` is also read by the rate limiter, +leases and the invalidation transport through redis-py's `from_url`, so it can +carry only a scheme both Celery and redis-py accept. Celery's `redis+socket` and +`sentinel` fail in redis-py (found in review, round 1 pass 3; the approved design +listed them). Any other scheme (`memory://`, `cache+memory://`, `amqp://`, +`rpc://`, `sqs://`, `db+…`) stays quiet. Those variables are the only way to express such a value, so they are not really deprecated for it. Rejected: @@ -35,17 +37,22 @@ Rejected: ## Design `modules/background_tasks/background_tasks/core/settings_env_checks.py`: -- add `_REDIS_SCHEMES = frozenset({"redis", "rediss", "redis+socket", "sentinel"})`; +- add `_REDIS_SCHEMES = frozenset({"redis", "rediss"})`; - in `warn_on_legacy_redis_vars`, skip a variable whose value's scheme (the text before `://`, lowercased) is not in that set; -- extend the docstring with the reason. +- extend the docstring with the reason; +- reword the production localhost error so it calls the variables deprecated + only for a Redis URL, and say they stay the way to set any other transport. + +Also reworded to match: the `settings.py` module docstring and the +`SM_BG_TASKS_BROKER_URL (deprecated)` mention in `docs/framework/invalidation.md`. ## Decisions made - The check is per variable. A host with a Redis broker and an `rpc://` backend still gets the warning for the broker, which it can move to `SM_REDIS_URL`. - No new setting and no new log line. -- Docs: a sentence in `docs/reference/env-vars.md` § Background tasks saying the +- Docs: a paragraph in `docs/reference/env-vars.md` § Background tasks saying the two variables stay the way to set non-Redis transports (the in-process pair for a production host with no Redis), and warn only for Redis URLs. - CHANGELOG `## [Unreleased]` → Fixed, citing #868. @@ -56,7 +63,8 @@ Rejected: - the in-process pair (`memory://` + `cache+memory://`) under `SM_ENVIRONMENT=production` boots and logs no deprecation warning; - an `amqp://` broker logs no warning; -- `rediss://` and `sentinel://` values still warn (parametrized); +- `rediss://` and an upper-case `REDIS://` still warn; `sentinel://` and + `redis+socket://` do not (parametrized); - the existing tests (`redis://` legacy values warn, `SM_REDIS_URL` alone is quiet) stay green. diff --git a/modules/background_tasks/background_tasks/core/settings_env_checks.py b/modules/background_tasks/background_tasks/core/settings_env_checks.py index ef225a84..7c38a686 100644 --- a/modules/background_tasks/background_tasks/core/settings_env_checks.py +++ b/modules/background_tasks/background_tasks/core/settings_env_checks.py @@ -15,9 +15,11 @@ logger = logging.getLogger("background_tasks.settings") -# Schemes Celery accepts as both broker and result backend, so one -# SM_REDIS_URL can carry them. -_REDIS_SCHEMES = frozenset({"redis", "rediss", "redis+socket", "sentinel"}) +# Schemes one SM_REDIS_URL can carry everywhere it is read: Celery's broker and +# result backend, and redis-py's ``from_url`` behind the rate limiter, leases and +# the invalidation transport. Celery's ``redis+socket`` and ``sentinel`` fail +# there, so those values stay on the per-field vars without a warning. +_REDIS_SCHEMES = frozenset({"redis", "rediss"}) def warn_on_legacy_redis_vars(broker_url: str, result_backend: str) -> None: @@ -39,8 +41,9 @@ def warn_on_legacy_redis_vars(broker_url: str, result_backend: str) -> None: (broker_url, f"{ENV_PREFIX}BROKER_URL"), (result_backend, f"{ENV_PREFIX}RESULT_BACKEND"), ): - scheme = value.partition("://")[0].lower() - if os.environ.get(legacy_var) == value and scheme in _REDIS_SCHEMES: + if os.environ.get(legacy_var) != value: + continue + if value.partition("://")[0].lower() in _REDIS_SCHEMES: logger.warning( "%s is deprecated; use %s instead (one URL seeds both the " "broker and the result backend).", diff --git a/modules/background_tasks/tests/core/settings/test_background_tasks_redis_url.py b/modules/background_tasks/tests/core/settings/test_background_tasks_redis_url.py index 18bafa0c..cf47d2d8 100644 --- a/modules/background_tasks/tests/core/settings/test_background_tasks_redis_url.py +++ b/modules/background_tasks/tests/core/settings/test_background_tasks_redis_url.py @@ -180,12 +180,10 @@ def test_non_redis_broker_logs_no_warning( "url", [ "rediss://cache:6380/0", - "sentinel://sentinel:26379/0", - "redis+socket:///run/redis.sock", "REDIS://cache:6379/0", ], ) -def test_every_redis_scheme_still_warns( +def test_redis_schemes_still_warn( monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture, url: str ) -> None: monkeypatch.setenv("SM_BG_TASKS_BROKER_URL", url) @@ -196,6 +194,18 @@ def test_every_redis_scheme_still_warns( assert "SM_BG_TASKS_BROKER_URL is deprecated" in caplog.text +@pytest.mark.parametrize("url", ["sentinel://sentinel:26379/0", "redis+socket:///run/redis.sock"]) +def test_schemes_redis_url_cannot_carry_log_no_warning( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture, url: str +) -> None: + monkeypatch.setenv("SM_BG_TASKS_BROKER_URL", url) + + with caplog.at_level(logging.WARNING): + BackgroundTasksSettings() + + assert "deprecated" not in caplog.text.lower() + + def test_warning_is_per_variable( monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture ) -> None: