From 7b870183e00f3d97ea591aadd1347fd985e8dc15 Mon Sep 17 00:00:00 2001 From: "tbxy-agent[bot]" <336722940+tbxy-agent[bot]@users.noreply.github.com> Date: Mon, 5 Oct 2026 18:55:57 +0200 Subject: [PATCH] Defer startup command sync to post_migrate and a management command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AppConfig.ready() ran sync_registered_commands() and sync_declarative_schedules() during django.setup(), explicitly suppressing Django's "Accessing the database during app initialization" warning. With gunicorn --preload and pooled connections (psycopg3 pool), the master opened the pool pre-fork; forked workers inherited a pool whose background threads died at fork and wedged in getconn() — proven staging outage (2026-10-05). ready() now only populates the in-memory registry and connects a post_migrate receiver. Rows materialize when migrate finishes and via the new admin_runner_sync management command (manual runs, worker boot, re-sync after code changes). ADMIN_RUNNER_SYNC_ON_READY=True (default False) restores the legacy ready()-time sync for consumers that cannot run migrations during deploy. --- README.md | 16 ++- docs/getting-started/installation.md | 25 +++- docs/guides/decorator.md | 2 +- docs/guides/scheduling.md | 7 +- examples/unfold_django_q2/README.md | 2 +- .../management/commands/cleanup_books.py | 3 +- .../specs/command-sync-on-startup/spec.md | 55 ++++++-- openspec/specs/declarative-schedules/spec.md | 2 +- src/django_admin_runner/apps.py | 43 ++++-- .../management/__init__.py | 0 .../management/commands/__init__.py | 0 .../management/commands/admin_runner_sync.py | 19 +++ src/django_admin_runner/sync.py | 11 ++ tests/test_app_config.py | 128 ++++++++++++++++++ tests/test_sync_command.py | 56 ++++++++ 15 files changed, 337 insertions(+), 32 deletions(-) create mode 100644 src/django_admin_runner/management/__init__.py create mode 100644 src/django_admin_runner/management/commands/__init__.py create mode 100644 src/django_admin_runner/management/commands/admin_runner_sync.py create mode 100644 tests/test_app_config.py create mode 100644 tests/test_sync_command.py diff --git a/README.md b/README.md index 218dd2f..54406b9 100644 --- a/README.md +++ b/README.md @@ -149,13 +149,27 @@ class Command(BaseCommand): ... ``` -The startup sync materializes declarations as `source=code` schedule rows +The sync materializes declarations as `source=code` schedule rows keyed by `(command_name, name)` (a single declaration defaults its name to the command name; lists require explicit unique names so reordering never rewires schedules). The registry wins for the schedule spec, the database wins for `enabled` — admins can pause a declarative schedule without a deploy. Admin-created schedules are never touched by the sync. +### When the sync runs + +`RegisteredCommand` and `source=code` schedule rows materialize when the +registry is synced — automatically after migrations (`post_migrate`) and on +demand via `python manage.py admin_runner_sync` (manual runs, worker boot, +or re-sync after code changes). `AppConfig.ready()` never touches the +database, so `django.setup()` works without one (image builds running +`collectstatic`) and is safe with gunicorn `--preload` and pooled +connections. + +Consumers that cannot run migrations during deploy can restore the legacy +startup sync with `ADMIN_RUNNER_SYNC_ON_READY = True` — not recommended +with preloaded workers or pooled connections. + ## Development **Requirements:** [uv](https://docs.astral.sh/uv/) and [just](https://github.com/casey/just) diff --git a/docs/getting-started/installation.md b/docs/getting-started/installation.md index 024ee74..4deb78f 100644 --- a/docs/getting-started/installation.md +++ b/docs/getting-started/installation.md @@ -27,7 +27,30 @@ INSTALLED_APPS = [ ] ``` -That's it. The package auto-discovers registered commands on startup via `AppConfig.ready()`. +That's it. The package auto-discovers registered commands on startup via +`AppConfig.ready()` — without touching the database — and syncs them into +the database automatically after migrations. + +## Keeping commands in sync + +Discovered commands and declarative schedules materialize as database rows +when the registry is synced: + +- automatically after migrations (`post_migrate` signal), and +- on demand via `python manage.py admin_runner_sync` — manual runs, worker + boot, or re-sync after code changes. + +`AppConfig.ready()` performs no database access, so `django.setup()` works +without a reachable database (image builds running `collectstatic`) and is +safe with gunicorn `--preload` and pooled connections. + +To restore the legacy behavior of syncing during app startup, set: + +```python +ADMIN_RUNNER_SYNC_ON_READY = True +``` + +Not recommended with preloaded workers or pooled connections. ## Optional extras diff --git a/docs/guides/decorator.md b/docs/guides/decorator.md index fb6b053..03c39b6 100644 --- a/docs/guides/decorator.md +++ b/docs/guides/decorator.md @@ -186,7 +186,7 @@ progress-bar-heavy commands so live output updates more frequently. Declarative schedule(s) for this command — a single `Schedule` instance (e.g. `CronSchedule("15 4 * * *")`) or a sequence of them. A single declaration defaults its name to the command name; sequences must declare -explicit, unique names. The startup sync materializes them as `source=code` +explicit, unique names. The sync materializes them as `source=code` rows: the registry wins for the spec, the database wins for `enabled` (admins can pause without a deploy). See the [Scheduling guide](scheduling.md). diff --git a/docs/guides/scheduling.md b/docs/guides/scheduling.md index 0a81d22..936993c 100644 --- a/docs/guides/scheduling.md +++ b/docs/guides/scheduling.md @@ -38,8 +38,9 @@ lists every schedule with its kind, parameters, next run, and last run. ## Declarative (in code) -Declare schedules directly on the decorator — they are materialized on -startup (in `AppConfig.ready()`) and kept in sync on every deploy: +Declare schedules directly on the decorator — they are materialized +whenever the registry syncs: automatically after migrations +(`post_migrate`) or on demand via `python manage.py admin_runner_sync`: ```python from django_admin_runner import CronSchedule, IntervalSchedule, register_command @@ -64,7 +65,7 @@ command name. Lists must declare explicit, unique names. never rewires schedules. Renaming an entry creates a new schedule; the removed declaration's row and native object are deleted. - **The registry wins for the schedule spec** (kind, expression, kwargs): - edits in code are picked up on the next startup. + edits in code are picked up on the next sync. - **The database wins for `enabled`** — admins can pause a declarative schedule without a deploy; the sync never flips it back on. - Admin-created schedules (`source=admin`) are never touched by the sync. diff --git a/examples/unfold_django_q2/README.md b/examples/unfold_django_q2/README.md index 1a49aea..29a12b4 100644 --- a/examples/unfold_django_q2/README.md +++ b/examples/unfold_django_q2/README.md @@ -61,7 +61,7 @@ interval, and one-off): class Command(BaseCommand): ... ``` - The startup sync materializes it as a `source=code` schedule — the code + The sync materializes it as a `source=code` schedule — the code wins for the schedule spec, the database wins for the enabled flag, so admins can pause it without a deploy. diff --git a/examples/unfold_django_q2/books/management/commands/cleanup_books.py b/examples/unfold_django_q2/books/management/commands/cleanup_books.py index 363e1b3..cdc31e3 100644 --- a/examples/unfold_django_q2/books/management/commands/cleanup_books.py +++ b/examples/unfold_django_q2/books/management/commands/cleanup_books.py @@ -7,7 +7,8 @@ group="Maintenance", exclude_params=["verbosity"], # Declarative schedule: materialized as a `source=code` ScheduledCommand - # row on startup and run nightly at 03:00 by qcluster. + # row by the sync (post-migrate / admin_runner_sync) and run nightly at + # 03:00 by qcluster. schedule=CronSchedule("0 3 * * *", kwargs={"older_than": 90}), ) class Command(BaseCommand): diff --git a/openspec/specs/command-sync-on-startup/spec.md b/openspec/specs/command-sync-on-startup/spec.md index 805c57b..12b3d5e 100644 --- a/openspec/specs/command-sync-on-startup/spec.md +++ b/openspec/specs/command-sync-on-startup/spec.md @@ -1,22 +1,57 @@ -## ADDED Requirements +# command-sync-on-startup Specification -### Requirement: Sync runs automatically on app startup -The system SHALL sync `_registry` entries to `RegisteredCommand` rows during `AppConfig.ready()`, after `autodiscover_commands()` has populated the registry. +## Purpose -#### Scenario: Fresh startup with no existing rows -- **WHEN** the app starts and `_registry` contains commands `cleanup_books` and `import_books`, and no `RegisteredCommand` rows exist +Keep `RegisteredCommand` rows (and declarative `ScheduledCommand` rows) in +sync with the in-memory `_registry` without touching the database during app +initialization, so `django.setup()` works without a reachable database +(image builds running `collectstatic`) and never opens pooled connections +pre-fork (gunicorn `--preload`). + +## Requirements + +### Requirement: AppConfig.ready() performs no database access +The system SHALL NOT execute queries, open database connections, or run the sync during `AppConfig.ready()`. `ready()` SHALL only populate the in-memory registry (`autodiscover_commands()`) and connect the `post_migrate` receiver. + +#### Scenario: Preloaded worker with a pooled connection +- **WHEN** `django.setup()` runs in a gunicorn `--preload` master process using a pooled database connection +- **THEN** no database connection is opened and no queries are executed by `ready()` + +#### Scenario: Image build without a database +- **WHEN** `collectstatic` (or any other `django.setup()` boot) runs with no reachable database +- **THEN** app initialization completes successfully without database access + +### Requirement: Sync runs automatically after migrations +The system SHALL sync `_registry` entries to `RegisteredCommand` rows on Django's `post_migrate` signal, exactly once per `migrate` run, after `autodiscover_commands()` has populated the registry. + +#### Scenario: Fresh migrate with no existing rows +- **WHEN** `migrate` finishes and `_registry` contains commands `cleanup_books` and `import_books`, and no `RegisteredCommand` rows exist - **THEN** two `RegisteredCommand` rows are created with `active=True` and metadata matching their registry entries -#### Scenario: Startup with existing rows -- **WHEN** the app starts and `_registry` contains `cleanup_books` (unchanged), `import_books` (description changed in code), and a new command `export_report` +#### Scenario: Migrate with existing rows +- **WHEN** `migrate` finishes and `_registry` contains `cleanup_books` (unchanged), `import_books` (description changed in code), and a new command `export_report` - **THEN** `cleanup_books` row is unchanged, `import_books` row has its `description` and `updated_at` updated, `export_report` row is created with `active=True` +### Requirement: Sync can be run on demand +The system SHALL provide a management command (`admin_runner_sync`) that runs the full sync — registered commands and declarative schedules — for manual runs, worker boot, or re-sync after code changes. + +#### Scenario: Manual re-sync after code changes +- **WHEN** a new command is registered in code and `admin_runner_sync` runs without migrations +- **THEN** a `RegisteredCommand` row is created for the new command and declarative schedules are materialized + +### Requirement: Legacy ready-time sync is opt-in +The system SHALL support an `ADMIN_RUNNER_SYNC_ON_READY` setting (default `False`) that restores sync during `AppConfig.ready()` for consumers that cannot run migrations or the sync command during deploy. + +#### Scenario: Opt-in enabled +- **WHEN** `ADMIN_RUNNER_SYNC_ON_READY = True` and the app starts +- **THEN** the sync runs during `AppConfig.ready()` as before the setting existed + ### Requirement: New commands are created, existing commands are updated For each entry in `_registry`, the sync SHALL create a `RegisteredCommand` if one does not exist for that `name`, or update the existing row's `group`, `display_name`, `description`, and `app_label` fields if they differ from the registry entry. #### Scenario: Command group changes - **WHEN** a command's `group` is changed in the `@register_command` decorator from `"Maintenance"` to `"Book Operations"` -- **THEN** the `RegisteredCommand` row's `group` field is updated to `"Book Operations"` on next startup +- **THEN** the `RegisteredCommand` row's `group` field is updated to `"Book Operations"` on the next sync ### Requirement: Removed commands are deactivated After processing all `_registry` entries, the sync SHALL set `active=False` on any `RegisteredCommand` whose `name` is not in the current `_registry`. @@ -33,8 +68,8 @@ Running the sync multiple times within the same process SHALL produce the same r - **THEN** no additional rows are created, no rows are modified on the second run (timestamps aside) ### Requirement: Sync is silent on fresh DB -The system SHALL handle the case where the `RegisteredCommand` table does not exist yet (e.g., before `migrate` is run) without raising errors. +The system SHALL handle the case where the `RegisteredCommand` table does not exist yet (e.g. before `migrate` is run) without raising errors. #### Scenario: Migrations not yet applied -- **WHEN** `AppConfig.ready()` runs but the `RegisteredCommand` table does not exist in the database +- **WHEN** the sync runs but the `RegisteredCommand` table does not exist in the database - **THEN** the sync is skipped silently (no exception raised) diff --git a/openspec/specs/declarative-schedules/spec.md b/openspec/specs/declarative-schedules/spec.md index fc0ff2b..e6cbf75 100644 --- a/openspec/specs/declarative-schedules/spec.md +++ b/openspec/specs/declarative-schedules/spec.md @@ -22,7 +22,7 @@ The library SHALL provide frozen `Schedule` value objects with one subclass per - **THEN** the registry records both declarations ### Requirement: Sync materializes declared schedules -The startup command sync SHALL create or update `ScheduledCommand` rows (marked `source=code`) for each declared schedule, materializing native backend schedules via the runner. The registry SHALL win for the schedule spec (kind, expression/interval, kwargs); the database SHALL win for `enabled` so admins can pause a declarative schedule without a deploy. +The sync SHALL create or update `ScheduledCommand` rows (marked `source=code`) for each declared schedule, materializing native backend schedules via the runner. The registry SHALL win for the schedule spec (kind, expression/interval, kwargs); the database SHALL win for `enabled` so admins can pause a declarative schedule without a deploy. #### Scenario: Sync creates declared schedules - **WHEN** a command with two declarations is registered and sync runs diff --git a/src/django_admin_runner/apps.py b/src/django_admin_runner/apps.py index 8a8c080..2a9151a 100644 --- a/src/django_admin_runner/apps.py +++ b/src/django_admin_runner/apps.py @@ -1,6 +1,5 @@ -import warnings - from django.apps import AppConfig +from django.conf import settings class AdminRunnerConfig(AppConfig): @@ -11,16 +10,34 @@ class AdminRunnerConfig(AppConfig): def ready(self) -> None: from .registry import autodiscover_commands + # Import-time registry population only. ready() runs inside + # django.setup() — gunicorn --preload masters, collectstatic during + # image builds, ... — where the database may be unreachable or must + # not be touched (e.g. pooled connections opened pre-fork). Syncing + # happens on post_migrate and via the admin_runner_sync command. autodiscover_commands() - from .sync import sync_declarative_schedules, sync_registered_commands - - with warnings.catch_warnings(): - warnings.filterwarnings( - "ignore", message="Accessing the database during app initialization" - ) - try: - sync_registered_commands() - sync_declarative_schedules() - except Exception: - pass + from django.db.models.signals import post_migrate + + post_migrate.connect( + _sync_on_post_migrate, + dispatch_uid="django_admin_runner.sync_on_post_migrate", + ) + + # Legacy opt-in for consumers that cannot run migrate or the sync + # command during deploy. Default is off: no DB access at ready(). + if getattr(settings, "ADMIN_RUNNER_SYNC_ON_READY", False): + from .sync import sync_all + + sync_all() + + +def _sync_on_post_migrate(sender, *, app_config=None, **kwargs) -> None: + """Run the full sync once per migrate — the signal fires for every app.""" + config = app_config if app_config is not None else sender + if getattr(config, "label", None) != "django_admin_runner": + return + + from .sync import sync_all + + sync_all() diff --git a/src/django_admin_runner/management/__init__.py b/src/django_admin_runner/management/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/src/django_admin_runner/management/commands/__init__.py b/src/django_admin_runner/management/commands/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/src/django_admin_runner/management/commands/admin_runner_sync.py b/src/django_admin_runner/management/commands/admin_runner_sync.py new file mode 100644 index 0000000..0f375e0 --- /dev/null +++ b/src/django_admin_runner/management/commands/admin_runner_sync.py @@ -0,0 +1,19 @@ +from django.core.management.base import BaseCommand + +from django_admin_runner.sync import sync_all + + +class Command(BaseCommand): + help = ( + "Sync @register_command registry entries into RegisteredCommand rows " + "and materialize declarative (source=code) schedules. Runs " + "automatically after migrate; call manually to re-sync after code " + "changes or from worker entrypoints." + ) + + def handle(self, *args, **options): + sync_all() + if options.get("verbosity", 1): + self.stdout.write( + self.style.SUCCESS("django-admin-runner: commands and schedules synced") + ) diff --git a/src/django_admin_runner/sync.py b/src/django_admin_runner/sync.py index a5a2547..2038f2e 100644 --- a/src/django_admin_runner/sync.py +++ b/src/django_admin_runner/sync.py @@ -125,6 +125,17 @@ def sync_declarative_schedules() -> None: row.delete() +def sync_all() -> None: + """Run the full sync: registered commands, then declarative schedules. + + Called from the ``post_migrate`` signal, the ``admin_runner_sync`` + management command, and — when ``ADMIN_RUNNER_SYNC_ON_READY`` is set — + ``AppConfig.ready()``. + """ + sync_registered_commands() + sync_declarative_schedules() + + def _supported_kinds() -> frozenset[str]: """Supported schedule kinds of the active runner (empty set on failure).""" try: diff --git a/tests/test_app_config.py b/tests/test_app_config.py new file mode 100644 index 0000000..b639c81 --- /dev/null +++ b/tests/test_app_config.py @@ -0,0 +1,128 @@ +"""AppConfig.ready() must not touch the database. + +Regression tests for the gunicorn ``--preload`` incident: DB access during +``django.setup()`` opens pooled connections pre-fork, and wedges forked +workers when the pool's background threads die. The sync runs via the +``post_migrate`` signal and the ``admin_runner_sync`` command instead. +""" + +from unittest.mock import patch + +import pytest +from django.apps import apps +from django.core.management import call_command +from django.db import connection +from django.db.backends.signals import connection_created +from django.db.models.signals import post_migrate +from django.test import override_settings +from django.test.utils import CaptureQueriesContext + +from django_admin_runner.models import RegisteredCommand, ScheduledCommand +from django_admin_runner.registry import _registry + + +class TestReadyPerformsNoDatabaseAccess: + @pytest.mark.django_db + def test_ready_executes_no_queries(self): + config = apps.get_app_config("django_admin_runner") + with CaptureQueriesContext(connection) as ctx: + config.ready() + assert len(ctx.captured_queries) == 0 + + def test_ready_opens_no_connection(self): + config = apps.get_app_config("django_admin_runner") + created = [] + + def on_connection_created(sender, connection, **kwargs): + created.append(connection.alias) + + connection_created.connect(on_connection_created, weak=False) + try: + config.ready() + finally: + connection_created.disconnect(on_connection_created) + assert created == [] + + def test_ready_does_not_call_sync(self): + config = apps.get_app_config("django_admin_runner") + with ( + patch("django_admin_runner.sync.sync_registered_commands") as sync_commands, + patch("django_admin_runner.sync.sync_declarative_schedules") as sync_schedules, + ): + config.ready() + sync_commands.assert_not_called() + sync_schedules.assert_not_called() + + def test_sync_on_ready_opt_in(self): + """ADMIN_RUNNER_SYNC_ON_READY=True restores the legacy ready() sync.""" + config = apps.get_app_config("django_admin_runner") + with ( + override_settings(ADMIN_RUNNER_SYNC_ON_READY=True), + patch("django_admin_runner.sync.sync_registered_commands") as sync_commands, + patch("django_admin_runner.sync.sync_declarative_schedules") as sync_schedules, + ): + config.ready() + sync_commands.assert_called_once() + sync_schedules.assert_called_once() + + +@pytest.mark.django_db +class TestPostMigrateSync: + def test_signal_for_this_app_syncs(self): + RegisteredCommand.objects.all().delete() + ScheduledCommand.objects.all().delete() + + config = apps.get_app_config("django_admin_runner") + post_migrate.send( + sender=config, + app_config=config, + using="default", + verbosity=0, + interactive=False, + ) + + assert set(RegisteredCommand.objects.values_list("name", flat=True)) == set(_registry) + assert RegisteredCommand.objects.filter(active=True).count() == len(_registry) + + def test_signal_for_other_apps_ignored(self): + RegisteredCommand.objects.all().delete() + + other = apps.get_app_config("auth") + post_migrate.send( + sender=other, + app_config=other, + using="default", + verbosity=0, + interactive=False, + ) + + assert RegisteredCommand.objects.count() == 0 + + def test_full_signal_round_syncs_once(self): + """post_migrate fires once per installed app — sync must run exactly once.""" + with ( + patch("django_admin_runner.sync.sync_registered_commands") as sync_commands, + patch("django_admin_runner.sync.sync_declarative_schedules") as sync_schedules, + ): + for config in apps.get_app_configs(): + post_migrate.send( + sender=config, + app_config=config, + using="default", + verbosity=0, + interactive=False, + ) + sync_commands.assert_called_once() + sync_schedules.assert_called_once() + + @pytest.mark.django_db(transaction=True) + def test_migrate_command_triggers_sync(self): + """End to end: migrate emits post_migrate, which materializes the rows.""" + RegisteredCommand.objects.all().delete() + ScheduledCommand.objects.all().delete() + + call_command("migrate", verbosity=0, interactive=False) + + assert RegisteredCommand.objects.count() == len(_registry) + for name in _registry: + assert RegisteredCommand.objects.filter(name=name, active=True).exists() diff --git a/tests/test_sync_command.py b/tests/test_sync_command.py new file mode 100644 index 0000000..e50ff1b --- /dev/null +++ b/tests/test_sync_command.py @@ -0,0 +1,56 @@ +"""Tests for the admin_runner_sync management command.""" + +from io import StringIO + +import pytest +from django.core.management import call_command + +from django_admin_runner.models import RegisteredCommand, ScheduledCommand +from django_admin_runner.registry import _registry +from django_admin_runner.schedules import CronSchedule, IntervalSchedule + + +@pytest.fixture(autouse=True) +def _registry_snapshot(): + """Restore registry entries after tests that mutate declarations.""" + snapshot = {name: dict(entry) for name, entry in _registry.items()} + yield + _registry.clear() + _registry.update(snapshot) + + +@pytest.mark.django_db +class TestAdminRunnerSyncCommand: + def test_syncs_registered_commands(self): + RegisteredCommand.objects.all().delete() + ScheduledCommand.objects.all().delete() + + call_command("admin_runner_sync", verbosity=0) + + assert set(RegisteredCommand.objects.values_list("name", flat=True)) == set(_registry) + assert RegisteredCommand.objects.filter(active=True).count() == len(_registry) + + def test_syncs_declarative_schedules(self): + RegisteredCommand.objects.all().delete() + ScheduledCommand.objects.all().delete() + _registry["simple_command"]["schedules"] = [ + CronSchedule("15 4 * * *", name="nightly"), + IntervalSchedule(15, name="often"), + ] + + call_command("admin_runner_sync", verbosity=0) + + rows = ScheduledCommand.objects.filter(source=ScheduledCommand.Source.CODE) + assert rows.get(label="nightly").cron == "15 4 * * *" + assert rows.get(label="often").interval_minutes == 15 + + def test_writes_success_message(self): + out = StringIO() + call_command("admin_runner_sync", stdout=out) + assert "synced" in out.getvalue() + + def test_idempotent(self): + call_command("admin_runner_sync", verbosity=0) + count = RegisteredCommand.objects.count() + call_command("admin_runner_sync", verbosity=0) + assert RegisteredCommand.objects.count() == count