Make a plugin toggle take effect without a manual restart

Enabling a plugin could not make its routes live. Flask refuses
register_blueprint once the app has served a request, and each gunicorn worker
holds its own app object, so registering in the worker that served the toggle
would leave the others answering 404 for the same URL.

Worse, the window before the restart was a window in which workers disagreed
about reality. Four defects, all of which answered 200 while going wrong:

- Enabling a not-yet-installed plugin ALWAYS 500'd on a running app:
  install_plugin ran the migrations, updated the registry, and only then called
  register_blueprint and hit Flask's assertion. The toggle half-applied and
  reported failure.
- Enabling one plugin could hang the entire site. @jwt_required does SELECT
  users, so the request's transaction sits open holding a shared metadata lock;
  the plugin's migration runs on its own Alembic connection and needs an
  EXCLUSIVE lock on users for a foreign key. Each waits on the other, and MySQL
  grants metadata locks in request order, so every later login queued behind the
  pending exclusive. Observed twice as a total outage from one plugin toggle.
- Each worker rewrote the WHOLE registry file from a snapshot taken at boot, so
  concurrent enables erased each other: seven enables persisted three, with
  HTTP 200 for all seven.
- Dependency checks read that same stale snapshot, refusing a plugin whose
  dependency another worker had just enabled.

Routes still require the process to restart; what changes is who initiates it.
Each worker fingerprints instance/plugins.json at boot and exits gracefully when
it changes, and the supervisor replaces it. That is the primitive gunicorn's own
--reload uses. Recycles are staggered per worker so the pool rolls rather than
dropping at once, and the exit is scoped to gunicorn: on Windows sys.exit() in a
daemon thread ends the thread rather than the process, so IIS keeps the manual
restart until that path is proven on the Windows VM.

The registry now merges under an exclusive lock and replaces atomically, saves
only the entry being changed, and re-reads before any decision is made from it.

ADR-020 records the decision; ADR-013 is amended where it said the UI must tell
the operator to restart. The restart requirement and the honesty requirement
both stand - only the manual step is removed, and only where it is proven.

(cherry picked from commit 9d6342b3ab)

shopdb/plugins/__init__.py

@@ -39,6 +39,14 @@ class PluginManager:
     def __init__(self):
         self.registry: Optional[PluginRegistry] = None
         self.loader: Optional[PluginLoader] = None
+        # Plugins installed while the app was already serving, whose blueprint
+        # and CLI registration had to wait for a restart. The loader counts them
+        # as loaded (it instantiated them), so this is the only record that
+        # their ROUTES are not live yet.
+        self._deferred_registration: set = set()
+        # Plugins whose routes this process registered at startup. See
+        # _load_enabled_plugins.
+        self._booted_with: set = set()
         self.migration_manager: Optional[PluginMigrationManager] = None
         self.verifier = None
         self._app: Optional[Flask] = None
@@ -100,6 +108,15 @@ class PluginManager:
     def _load_enabled_plugins(self) -> None:
         """Load and register all enabled plugins."""
         plugins = self.loader.load_enabled_plugins(self._app, self._db)
+        # What THIS process actually registered routes for, recorded at boot.
+        # Everything else is per-process and cannot answer "does the fleet need
+        # a restart": under gunicorn each worker has its own manager, its own
+        # loader and its own copy of the registry, so a plugin enabled through
+        # one worker is invisible to the other three. Comparing this snapshot
+        # against plugins.json re-read from disk is correct in every worker,
+        # because a plugin enabled after boot is absent from every worker's
+        # snapshot no matter which one served the enable.
+        self._booted_with = set(plugins.keys())
 
         for name, plugin in plugins.items():
             self._register_plugin_components(plugin)
@@ -157,6 +174,7 @@ class PluginManager:
                 results[name] = 'no-migrations'
                 continue
             try:
+                self._release_session_locks()
                 ok = self.migration_manager.run_plugin_migrations(name)
                 results[name] = 'ok' if ok else 'failed'
             except Exception as ex:
@@ -252,8 +270,72 @@ class PluginManager:
         return {key for key, entry in self.get_declared_plugin_settings().items()
                 if entry.get('public')}
 
+    def _release_session_locks(self) -> None:
+        """Close this request's transaction before any plugin DDL runs.
+
+        SELF-DEADLOCK, AND WHY IT TAKES THE WHOLE SITE DOWN. Enabling a plugin
+        over HTTP arrives inside a request that has already authenticated, and
+        @jwt_required does a SELECT on users - so the request's session is
+        sitting in an open transaction holding a SHARED_READ metadata lock on
+        users. The plugin's migrations then run through Alembic, which builds
+        its OWN connection; a plugin table with a foreign key to users (
+        endpointlocks, policyactionholds, endpointlogcollections and others)
+        needs an EXCLUSIVE metadata lock on it. That second connection waits on
+        the first, the first cannot commit because the request is blocked on the
+        second, and it never resolves.
+
+        It does not stop at the one request. MySQL grants metadata locks in
+        request order, so every later SELECT on users queues BEHIND the pending
+        exclusive even though those reads are compatible with the shared lock
+        already held. Every login on the site then hangs: observed as a total
+        outage from one plugin toggle.
+
+        Committing first ends the transaction and drops the lock, so the DDL
+        takes its exclusive lock immediately. The session stays usable - the
+        next statement simply begins a new transaction.
+        """
+        if self._db is None:
+            return
+        try:
+            self._db.session.commit()
+        except Exception:
+            try:
+                self._db.session.rollback()
+            except Exception:
+                pass
+
     def _register_plugin_components(self, plugin: BasePlugin) -> None:
-        """Register plugin's blueprint, models, CLI commands, etc."""
+        """Register plugin's blueprint, models, CLI commands, etc.
+
+        NOT POSSIBLE ON A LIVE APP, and that is fine. Flask refuses
+        register_blueprint once the app has served its first request, raising
+        AssertionError. Installing a plugin at runtime - which is exactly what
+        enabling one from the setup wizard or the plugins page does - therefore
+        used to run the plugin's migrations, update the registry, and THEN blow
+        up with a 500, leaving the operator with an error over a change that had
+        in fact been recorded.
+
+        Routes only ever take effect on restart anyway; the UI has always said
+        so. So on a live app, skip the parts that mutate app setup and let the
+        restart register them properly. The registry entry, the migrations, the
+        permissions and the settings - everything that belongs in the database -
+        still happen here.
+        """
+        # Flask renamed this: 2.x exposed `got_first_request` publicly, 3.x
+        # keeps it as `_got_first_request`. Check both, because defaulting to
+        # False on the wrong name silently disables this guard - which is how
+        # the 500 survived the first attempt at this fix.
+        served = getattr(self._app, 'got_first_request', None)
+        if served is None:
+            served = getattr(self._app, '_got_first_request', False)
+        if served:
+            self._deferred_registration.add(plugin.meta.name)
+            logger.info(
+                "%s installed while the app is serving; its blueprint and CLI "
+                "commands register on the next restart (Flask does not allow "
+                "either after the first request).", plugin.meta.name)
+            return
+
         # Register blueprint
         blueprint = plugin.get_blueprint()
         if blueprint:
@@ -274,6 +356,40 @@ class PluginManager:
         for cmd in plugin.get_cli_commands():
             self._app.cli.add_command(cmd)
 
+    def get_booted_with(self) -> set:
+        """Plugins this process registered routes for at startup."""
+        return set(self._booted_with)
+
+    def enabled_on_disk(self) -> set:
+        """Enabled plugins per plugins.json, re-read now.
+
+        The in-memory registry is per-worker and goes stale the moment another
+        worker enables something, so a restart check must go back to the file.
+        """
+        import json
+        try:
+            path = self.registry.state_file
+            with open(path) as handle:
+                data = json.load(handle)
+            return {name for name, state in (data.get('plugins') or {}).items()
+                    if state.get('enabled')}
+        except FileNotFoundError:
+            return set()
+        except Exception:
+            # Unreadable or corrupt: fall back to this worker's view rather
+            # than claiming nothing is enabled.
+            return set(self.registry.get_enabled_plugins())
+
+    def get_deferred_registrations(self) -> set:
+        """Plugins enabled in this process whose routes await a restart.
+
+        get_all_loaded() is not a substitute: install_plugin instantiates the
+        plugin through the loader, so a plugin whose blueprint was skipped still
+        reports as loaded. Without this, a health check says no restart is
+        needed while every one of that plugin's routes answers 404.
+        """
+        return set(self._deferred_registration)
+
     def discover_available(self) -> List[Dict]:
         """
         Get list of all available plugins (installed or not).
@@ -322,6 +438,10 @@ class PluginManager:
                         'config_schema': config_schema,
                         'provisioning_note': provisioning_note,
                         'default_enabled': manifest.get('default_enabled', True),
+                        # Declared per plugin in its manifest, not held as a
+                        # list in the wizard, so adding a plugin cannot leave
+                        # the recommendation out of date in another file.
+                        'recommended': bool(manifest.get('recommended', False)),
                     })
                 except Exception as e:
                     logger.warning(f"Error inspecting plugin {name}: {e}")
@@ -362,6 +482,7 @@ class PluginManager:
 
         # Run migrations
         if run_migrations:
+            self._release_session_locks()
             success = self.migration_manager.run_plugin_migrations(name)
             if not success:
                 logger.error(f"Failed to run migrations for {name}")
@@ -549,6 +670,10 @@ class PluginManager:
             logger.error(f"Invalid plugin name: {name}")
             return False
 
+        # Decisions below read the registry, so start from what is on disk
+        # rather than this worker's boot snapshot.
+        self.registry.refresh()
+
         # Auto-install if available but not installed (UX: enabling should just work)
         if not self.registry.is_installed(name):
             logger.info(f"Plugin {name} not installed, installing first")
@@ -601,6 +726,7 @@ class PluginManager:
 
     def disable_plugin(self, name: str) -> bool:
         """Disable an enabled plugin."""
+        self.registry.refresh()
         if not self.registry.is_enabled(name):
             logger.info(f"Plugin {name} is already disabled")
             return True