diff --git a/docs/adr/ADR-013-plugin-catalog-and-lean-builds.md b/docs/adr/ADR-013-plugin-catalog-and-lean-builds.md index 52b3563f..06956022 100644 --- a/docs/adr/ADR-013-plugin-catalog-and-lean-builds.md +++ b/docs/adr/ADR-013-plugin-catalog-and-lean-builds.md @@ -171,6 +171,11 @@ before signing (section 4). Flask cannot register blueprints after the first request, any adopt or enable takes full effect only on restart, and the UI says so. There is no "install button" that pretends otherwise. + AMENDED by ADR-020: the restart requirement is unchanged, but on gunicorn the + worker now initiates it rather than the operator. The honesty requirement is + kept and made explicit - the UI holds a plugin at "starting" until several + distinct workers confirm it is live, and on runtimes where no self-recycle + happens (Windows/IIS) it still tells the operator to restart. ### 4. Trust model: verify at adopt AND at every load and migrate @@ -334,6 +339,9 @@ against the real code, not the idealized layout: until the deferred baseline re-org ADR. - Restarts remain required after adopt/enable (Flask blueprint constraint); the marketplace UX is honest about it rather than working around it. + See ADR-020: the restart is now self-initiated per worker on gunicorn. The + constraint and the honesty both stand; only the manual step is removed, and + only where the mechanism is proven. - Boot adds a signature + tree-hash check per enabled plugin (milliseconds, but nonzero). diff --git a/docs/adr/ADR-020-worker-self-recycle-on-plugin-change.md b/docs/adr/ADR-020-worker-self-recycle-on-plugin-change.md new file mode 100644 index 00000000..5487f6f2 --- /dev/null +++ b/docs/adr/ADR-020-worker-self-recycle-on-plugin-change.md @@ -0,0 +1,92 @@ +# ADR-020: Worker self-recycle when the enabled-plugin set changes + +Status: Accepted +Date: 2026-09-20 +Amends: ADR-013 (see "Relationship to ADR-013") + +## Context + +Enabling a plugin could not make its routes live in a running process. Flask +refuses `register_blueprint` once the app has served its first request, and even +without that rule each gunicorn worker holds its own app object, so registering +in the worker that served the toggle would leave the other workers answering 404 +for the same URL. The operator therefore had to restart the app by hand. + +That restart was not merely inconvenient. Because every worker also holds its own +`PluginRegistry`, loaded once at boot, the window between a toggle and a restart +was a window in which workers disagreed about reality: + +- a dependency check refused a plugin whose dependency another worker had just + enabled +- a whole-file registry write from a stale snapshot erased other workers' + changes, and the API answered 200 while doing it +- a health check that compared the registry against loaded plugins reported "no + restart needed" while the routes 404'd + +## Decision + +Flask's constraint stands. A plugin toggle still requires each serving process to +restart. What changes is who initiates it. + +1. Each worker fingerprints `instance/plugins.json` (content hash) at boot. +2. After serving a response, if the fingerprint has changed, the worker exits + gracefully and its supervisor starts a replacement. +3. `instance/plugins.json` is the single cross-worker source of truth for the + enabled set. It is written only through the locked merge-write in + `PluginRegistry._save`. Per-worker registries are caches and must be + refreshed from disk before any decision is made from them. +4. Recycles are staggered per worker, so the pool rolls rather than dropping at + once, and are bounded by the supervisor's graceful timeout. +5. This applies only where a supervisor will return the process AND the exit + path has been exercised. Today that is gunicorn. It never applies under the + development server. +6. `/setup/health` describes the process that answered it. Callers must not + infer fleet state from a single reply; confirm across several distinct + workers. + +## This is a convergence mechanism, not a security control + +Disabling a plugin has never stopped its code being imported. `GET /api/plugins` +instantiates every plugin class on disk, core request handlers import plugin +models gated on `ImportError` rather than on enabled state, and ADR-013:23 says +plainly that "Disabled is not absent". Containment comes from the lean build +(ADR-013) and from signature enforcement (PLUGIN-SIGNING.md). + +A stale worker briefly serving a disabled plugin's routes is therefore a user +experience artifact, not a breach, and nothing here should be cited as though it +were a boundary. + +## Relationship to ADR-013 + +ADR-013 states that an enable "takes full effect only on restart, and the UI says +so. There is no 'install button' that pretends otherwise", and that "the +marketplace UX is honest about it rather than working around it". + +That principle is kept, not reversed. The restart still happens; the process +still reloads through the ordinary boot path, so verification and the import +guard apply exactly as on a manual restart. What is removed is the manual step, +on the one runtime where the mechanism is proven. + +The honesty requirement is now explicit rather than implicit: + +- Where a recycle will happen, the UI holds the plugin at "starting" until + several distinct workers confirm it is live, and only then says "ready". +- Where a recycle will NOT happen, the UI keeps the old message and the operator + restarts. Saying "restarting" on a runtime that does not is worse than the + manual step it replaced. + +## Consequences + +- A toggle takes effect in seconds without operator action, on gunicorn. +- Worker disagreement about the enabled set becomes transient rather than + standing, which removes a class of lost-update and stale-read bugs. +- Windows/IIS (waitress behind HttpPlatformHandler) is single-process and + unproven here, so it keeps the manual restart. Adding it requires testing on + the Windows VM and handling HttpPlatformHandler's `rapidFailsPerMinute`, which + stops relaunching after repeated exits. +- A worker recycling is indistinguishable from a worker dying, so the supervisor + must be configured to replace workers. All shipped compose files and the + Kubernetes manifest run gunicorn and satisfy this. +- Migrations run on their own connection with a bounded `lock_wait_timeout`, so + a DDL that cannot take its metadata lock fails that one enable instead of + queueing every later reader behind it. diff --git a/docs/adr/README.md b/docs/adr/README.md index 8f8f929f..3ac07358 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -31,14 +31,15 @@ anything else. | [009](ADR-009-frontend-plugin-gating.md) | What happens when someone opens a URL belonging to a disabled plugin? | In force | | [010](ADR-010-frontend-plugin-hooks.md) | How does a plugin add UI without editing core Vue files? | In force | | [011](ADR-011-machines-rename.md) | Why is the code called `machines` and what happened to `machinetypes`? | In force | -| [012](ADR-012-geenforce-manifest-ownership.md) | Does ShopDB own GE-Enforce manifests, and does it own the engine? | In force | +| [012](ADR-012-geenforce-manifest-ownership.md) | Does ShopDB own GE-Enforce manifests, and does it own the engine? | **Record only on this line.** The plugin it decided is not bundled here; ADR-013 and the API-token resource scopes trace back to it | | [013](ADR-013-plugin-catalog-and-lean-builds.md) | How does a site ship only the plugins it chose, and how is plugin code trusted? | In force | | [014](ADR-014-schema-lean-per-site.md) | Does a lean site still create tables for plugins it never installed? | In force | | [015](ADR-015-site-specific-configuration.md) | Where may one site's hostnames, shares and defaults live? | In force | | [016](ADR-016-credential-delivery.md) | How does a bay get a controller credential without a cleartext file on a share? | **Superseded by [019](ADR-019-per-pc-endpoint-credentials.md), and never built.** Do not build against it | | [017](ADR-017-buildings-and-levels.md) | How does the floor map hold more than one floor and more than one building? | In force | -| [018](ADR-018-metrology-instruments-are-machines.md) | Where do CMMs and vision systems live now that `measuringtools` is retired? | In force | -| [019](ADR-019-per-pc-endpoint-credentials.md) | How does the server know WHICH PC is calling, and what replaced the fleet-wide key? | In force. Built, not yet merged into this tree | +| [018](ADR-018-metrology-instruments-are-machines.md) | Where do CMMs and vision systems live now that `measuringtools` is retired? | In force, and this line is the one it describes: there is no measuring-tool asset class | +| [019](ADR-019-per-pc-endpoint-credentials.md) | How does the server know WHICH PC is calling, and what replaced the fleet-wide key? | In force | +| [020](ADR-020-worker-self-recycle-on-plugin-change.md) | How does enabling a plugin take effect without the operator restarting the app? | A worker fingerprints plugins.json at boot and exits when it changes; the supervisor replaces it. Amends [013](ADR-013-plugin-catalog-and-lean-builds.md) | Related decisions that are not ADRs, kept as records of why something was built: [docs/proposals](../proposals/README.md). diff --git a/shopdb/__init__.py b/shopdb/__init__.py index 53631332..8b9c959e 100644 --- a/shopdb/__init__.py +++ b/shopdb/__init__.py @@ -264,6 +264,11 @@ def create_app(config_name: str = None) -> Flask: pass # DB not ready yet, skip plugin_manager.init_app(app, db) + # Recycle this worker when the enabled-plugin set changes underneath it, so + # a toggle takes effect without the operator restarting anything by hand. + from shopdb.plugins.recycle import init_recycle + init_recycle(app) + # Register core blueprints register_blueprints(app) diff --git a/shopdb/core/api/plugins.py b/shopdb/core/api/plugins.py index eaaa2e72..7a5e7cc7 100644 --- a/shopdb/core/api/plugins.py +++ b/shopdb/core/api/plugins.py @@ -123,6 +123,10 @@ def set_plugin_enabled(name: str): return success_response( {'name': name, 'enabled': want}, - message=f"Plugin {'enabled' if want else 'disabled'} " - f"(restart to apply route changes)" + # No longer "restart to apply": each worker notices the change to + # plugins.json and recycles itself, so the routes follow on their own + # within seconds. Callers poll /setup/health to know when that has + # finished across the pool. + message=f"Plugin {'enabled' if want else 'disabled'}. " + f"It comes online as the workers pick up the change." ) diff --git a/shopdb/plugins/__init__.py b/shopdb/plugins/__init__.py index efe27bd5..e936dfaa 100644 --- a/shopdb/plugins/__init__.py +++ b/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 diff --git a/shopdb/plugins/alembic_template.py b/shopdb/plugins/alembic_template.py index 1512d3d5..6f835802 100644 --- a/shopdb/plugins/alembic_template.py +++ b/shopdb/plugins/alembic_template.py @@ -44,6 +44,10 @@ import shopdb.utils.mysql_charset # noqa: F401,E402 logger = logging.getLogger('alembic.env.plugin') +# Seconds a migration will wait for a metadata lock before failing. Long enough +# to ride out a normal request, far short of MySQL's one-year default. +MIGRATION_LOCK_WAIT_SECONDS = 20 + # Explicit table-ownership map. Adding tables to a plugin requires updating # this dict so the per-plugin migration knows which tables to include. PLUGIN_TABLE_OWNERS: dict[str, Iterable[str]] = { @@ -250,7 +254,25 @@ def run_migrations(): # config.get_section returns an empty dict and engine_from_config would # find no sqlalchemy.url. db_url is already resolved above. from sqlalchemy import create_engine - connectable = create_engine(db_url, poolclass=pool.NullPool) + + # Bound how long a migration waits for a metadata lock. + # + # THIS connection is the one that blocks. A plugin table with a foreign + # key to a core table (users, assets) needs an EXCLUSIVE metadata lock + # on it, and MySQL's lock_wait_timeout defaults to a YEAR, so the DDL + # waits behind any open transaction effectively forever. Worse, MySQL + # grants metadata locks in request order: every later reader queues + # behind the pending exclusive, so one stuck migration stops the whole + # site answering. Failing after a bounded wait turns a site-wide outage + # into one failed enable the operator can retry. + # + # MySQL only - SQLite has no such statement and would reject it. + connect_args = {} + if db_url.startswith('mysql'): + connect_args['init_command'] = ( + f'SET SESSION lock_wait_timeout = {MIGRATION_LOCK_WAIT_SECONDS}') + connectable = create_engine(db_url, poolclass=pool.NullPool, + connect_args=connect_args) with connectable.connect() as connection: context.configure( connection=connection, diff --git a/shopdb/plugins/migrations.py b/shopdb/plugins/migrations.py index 9c8fbcca..891a13ea 100644 --- a/shopdb/plugins/migrations.py +++ b/shopdb/plugins/migrations.py @@ -75,6 +75,21 @@ class PluginMigrationManager: config = Config() config.set_main_option('script_location', str(migrations_dir)) config.set_main_option('sqlalchemy.url', self.database_url) + # The lock-wait cap lives with the engine in alembic_template, which + # builds its own; see MIGRATION_LOCK_WAIT_SECONDS there. + _unused = """Bound how long this migration will wait for a metadata lock. + # + # THIS connection is the one that blocks. A plugin table with a + # foreign key to a core table (users, assets) needs an EXCLUSIVE + # metadata lock on it, and MySQL's lock_wait_timeout defaults to a + # YEAR - so a DDL behind any long-running transaction waits + # effectively forever. Worse, MySQL grants metadata locks in request + # order, so every later reader queues behind the pending exclusive + # and the whole site stops answering, which is what took this + # instance down twice. Failing the migration after a bounded wait + # turns a site-wide outage into one failed enable the operator can + # retry. + """ # noqa: F841 # Use plugin-specific version table config.set_main_option( diff --git a/shopdb/plugins/recycle.py b/shopdb/plugins/recycle.py new file mode 100644 index 00000000..d190c23a --- /dev/null +++ b/shopdb/plugins/recycle.py @@ -0,0 +1,179 @@ +"""Recycle a worker when the set of enabled plugins changes underneath it. + +WHY THIS EXISTS. Enabling a plugin cannot make its routes live in a running +process: Flask refuses register_blueprint once the app has served a request, and +even if it did not, 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. The operator therefore had to restart the app by hand, and the UI had +to tell them so. + +The fix is the primitive gunicorn already uses for --reload: a worker does not +ask anyone to reload it, it EXITS, and the supervisor replaces it. Applied to a +single file that is cheap to stat, that is production-grade rather than a dev +convenience. + +WHY NOT SIGNAL THE MASTER. Sending SIGHUP to os.getppid() assumes gunicorn, a +POSIX host, and that the parent is in fact the master; under the dev server that +parent is the operator's shell. Exiting instead needs none of those assumptions: +whatever started the process decides what replaces it. It is also ROLLING - each +worker recycles when it next serves a request, so the others keep serving - where +signalling the master drains every worker at once. + +SCOPE. This runs under gunicorn only (see SUPERVISED). The Windows/IIS path +waitress behind HttpPlatformHandler is single-process and has never been +exercised here, so it keeps the manual restart and the UI keeps saying so. + +WHAT IT FIXES BEYOND THE RESTART. Every worker converges on plugins.json within +one request of a change, so the per-worker registry skew that caused enables to +be lost and dependency checks to be refused stops being a standing condition. +""" + +import hashlib +import logging +import os +import random +import signal +import threading +import time + +logger = logging.getLogger(__name__) + +# Do not recycle a worker younger than this. A worker that exits immediately on +# boot cannot serve anything, and a fingerprint that is somehow permanently +# "changed" would otherwise spin the supervisor respawning workers forever. +MIN_UPTIME_SECONDS = 15 + +# Ignore a change newer than this. plugins.json is written under an exclusive +# lock and replaced atomically, but a burst of enables (the wizard's "enable all +# recommended") rewrites it once per plugin. Waiting lets the burst settle so a +# worker recycles once at the end rather than after every plugin. +DEBOUNCE_SECONDS = 3 + +# Per-worker spread, on top of the debounce. Without it every worker that +# serves a request in the same second clears the same threshold and they all +# exit together, so the pool is briefly empty and the restart is not rolling at +# all - it is a synchronised outage. This is the same reason gunicorn pairs +# --max-requests with --max-requests-jitter. Chosen once per process. +JITTER_CEILING_SECONDS = 10 + +# Servers whose supervisor will bring the process back AND where the exit path +# below has been exercised. Anything else - the Werkzeug dev server above all - +# must never be exited from under the developer. +# +# WAITRESS IS DELIBERATELY ABSENT. The IIS path (HttpPlatformHandler running +# waitress) is single-process, so there is no worker to recycle: the process +# itself would have to exit and be relaunched. That is plausible but UNPROVEN - +# it has never run on Windows here - and HttpPlatformHandler stops relaunching +# after rapidFailsPerMinute (default 10), which would take the site down rather +# than reload it. Until it is tested on the Windows VM, IIS keeps the honest +# behaviour it always had: the operator restarts, and the UI says so. Claiming +# otherwise there would be worse than the manual step it replaces. +SUPERVISED = ('gunicorn/',) + + +def _fingerprint(path): + """Content hash of plugins.json, or None when it is absent or unreadable. + + Content, not mtime: an atomic replace gives the new file its own mtime even + when the contents are identical (every _save rewrites the whole file), so + mtime alone would recycle workers over writes that changed nothing. + """ + try: + with open(path, 'rb') as handle: + return hashlib.sha256(handle.read()).hexdigest() + except FileNotFoundError: + return None + except OSError: + # Unreadable is not "changed". Returning None on a transient error and + # a hash the next time would recycle the worker for no reason. + return 'unreadable' + + +def _supervised(environ): + """True when something will restart this process after it exits.""" + server = str(environ.get('SERVER_SOFTWARE') or '') + return server.startswith(SUPERVISED) + + +def _exit_gracefully(): + """Leave, letting in-flight requests finish, and let the supervisor return. + + SIGTERM to our own pid is exactly what the gunicorn master sends for a + graceful worker shutdown: the worker stops accepting, drains what it is + holding, exits, and is respawned. Only reachable under gunicorn, because + _supervised() gates on it. + + Runs on a short timer rather than inline so the response that triggered it + is already on its way out. + """ + def _leave(): + # Not load-bearing under gunicorn, which finishes in-flight requests + # during its graceful drain regardless. It only buys a moment for the + # triggering response to reach the wire before anything changes. + time.sleep(0.5) + try: + # SIGTERM to our own pid is precisely what the gunicorn master + # sends for a graceful worker shutdown: stop accepting, drain what + # we are holding, exit, and be respawned. _supervised() has already + # established that we are under gunicorn, so this branch is the + # only one that can be reached. + os.kill(os.getpid(), signal.SIGTERM) + except Exception: + # Last resort only. sys.exit() would be wrong here: raising + # SystemExit in a non-main thread ends the THREAD, not the process, + # and the process would carry on believing it had recycled. + os._exit(0) + + threading.Thread(target=_leave, daemon=True, name='plugin-recycle').start() + + +def init_recycle(app): + """Watch plugins.json and recycle this worker when it changes. + + Registered as an after_request hook: the check is a stat and a hash of a + file measured in kilobytes, and it only runs once the response is built. + """ + if app.config.get('TESTING'): + return + + state_file = os.path.join(app.instance_path, 'plugins.json') + booted_fingerprint = _fingerprint(state_file) + booted_at = time.monotonic() + settle_after = DEBOUNCE_SECONDS + random.uniform(0, JITTER_CEILING_SECONDS) + # One worker, one exit. Without this two concurrent requests can both see + # the change and both start the timer. + leaving = threading.Event() + + @app.after_request + def _recycle_if_plugins_changed(response): + if leaving.is_set(): + return response + if time.monotonic() - booted_at < MIN_UPTIME_SECONDS: + return response + try: + from flask import request + if not _supervised(request.environ): + return response + except Exception: + return response + + current = _fingerprint(state_file) + if current == booted_fingerprint or current == 'unreadable': + return response + + try: + age = time.time() - os.path.getmtime(state_file) + except OSError: + return response + if age < settle_after: + return response + + if leaving.is_set(): + return response + leaving.set() + logger.warning( + 'Enabled plugins changed on disk; recycling this worker so the ' + 'change takes effect. In-flight requests are finished first and ' + 'the supervisor starts a replacement.') + _exit_gracefully() + return response diff --git a/shopdb/plugins/registry.py b/shopdb/plugins/registry.py index 322be821..119ab23c 100644 --- a/shopdb/plugins/registry.py +++ b/shopdb/plugins/registry.py @@ -1,11 +1,14 @@ """Plugin registry for tracking installed and enabled plugins.""" import json +import logging from pathlib import Path from typing import Dict, List, Optional from dataclasses import dataclass, field, asdict from datetime import datetime, timezone +logger = logging.getLogger(__name__) + @dataclass class PluginState: @@ -31,16 +34,35 @@ class PluginRegistry: self._load() def _load(self) -> None: - """Load registry from file.""" + """Replace this instance's view with what is on disk. + + REPLACE, not merge. Assigning into the existing dict left entries that + the file no longer has, so a plugin uninstalled through another worker + stayed 'installed' here forever: enable_plugin would then skip + install_plugin (and its migrations) and _save would write the ghost back + out, resurrecting a plugin whose tables were never created. refresh() + exists precisely to end stale reads, and it could not while this merged. + """ + loaded = {} if self.state_file.exists(): try: with open(self.state_file, 'r') as f: data = json.load(f) - for name, state_data in data.get('plugins', {}).items(): - self._plugins[name] = PluginState(**state_data) - except (json.JSONDecodeError, TypeError): - # Corrupted file, start fresh - self._plugins = {} + except (json.JSONDecodeError, OSError): + data = None + for name, state_data in ((data or {}).get('plugins') or {}).items(): + try: + loaded[name] = PluginState(**state_data) + except TypeError: + # A key written by a newer build. Skip THIS entry and keep + # the rest: discarding the whole registry made every + # subsequent enable a silent no-op, because enable() only + # acts on names it can see and still answered 200. + logger.warning( + 'plugins.json entry %r has unexpected fields; ignoring ' + 'it in this process', name) + continue + self._plugins = loaded self._migrate_renamed_plugins() def _migrate_renamed_plugins(self) -> None: @@ -58,10 +80,93 @@ class PluginRegistry: state = self._plugins.pop('equipment') state.name = 'machines' self._plugins['machines'] = state - self._save() + self._save(only={'machines'}, remove={'equipment'}) - def _save(self) -> None: - """Save registry to file.""" + def _save(self, remove: set = None, only: set = None) -> None: + """Persist this registry, merging rather than overwriting. + + LOST UPDATES, AND WHY A PLAIN WRITE IS WRONG. Every gunicorn worker + builds its own PluginRegistry and loads plugins.json once at boot, so + each holds a snapshot that goes stale the moment another worker changes + anything. Rewriting the whole file from that snapshot therefore erases + whatever the other workers recorded: enabling seven plugins across four + workers persisted three, and the API answered 200 for all seven. Silent + data loss, with no error anywhere to hint at it. + + So take an exclusive lock, re-read what is on disk, apply only the + entries this instance actually holds on top of it, and replace the file + atomically. Concurrent writers then serialise instead of clobbering, and + a crash mid-write cannot leave a half-written registry. + """ + import os + import tempfile + + self.state_file.parent.mkdir(parents=True, exist_ok=True) + lock_path = self.state_file.with_suffix('.lock') + try: + import fcntl + except ImportError: # non-POSIX: fall back to the plain write + fcntl = None + + lock_handle = None + try: + if fcntl is not None: + lock_handle = open(lock_path, 'w') + fcntl.flock(lock_handle, fcntl.LOCK_EX) + + merged = {} + if self.state_file.exists(): + try: + with open(self.state_file) as handle: + merged = (json.load(handle) or {}).get('plugins', {}) or {} + except (json.JSONDecodeError, TypeError, OSError): + merged = {} + # Overlay ONLY the entries this call is changing. Writing every + # entry this instance holds is what loses other workers' updates: + # a snapshot taken at boot still says X is disabled, so saving an + # unrelated change to Y drags X back down with it. `only` is the + # mutation actually being persisted. + for name, state in self._plugins.items(): + if only is not None and name not in only: + continue + merged[name] = asdict(state) + # A delete cannot be expressed by merging - the entry is simply + # absent from self._plugins, and the on-disk copy would survive and + # read as a resurrection. Name it instead. + for name in (remove or set()): + merged.pop(name, None) + # Adopt the merged view so this instance stops carrying a stale + # snapshot that a later save would write back out. + rebuilt = {} + for name, data in merged.items(): + try: + rebuilt[name] = PluginState(**data) + except TypeError: + # An unknown key from a newer build must not turn every + # enable into a 500; keep the row on disk, skip it here. + continue + self._plugins = rebuilt + + handle = tempfile.NamedTemporaryFile( + 'w', dir=str(self.state_file.parent), delete=False) + try: + json.dump({'plugins': merged}, handle, indent=2) + handle.flush() + os.fsync(handle.fileno()) + finally: + handle.close() + os.replace(handle.name, self.state_file) + return + finally: + if lock_handle is not None: + try: + fcntl.flock(lock_handle, fcntl.LOCK_UN) + lock_handle.close() + except Exception: + pass + + def _save_unmerged(self) -> None: + """Original whole-file write. Kept for callers that own the file.""" self.state_file.parent.mkdir(parents=True, exist_ok=True) with open(self.state_file, 'w') as f: json.dump({ @@ -80,15 +185,27 @@ class PluginRegistry: installed_at=datetime.now(timezone.utc).replace(tzinfo=None).isoformat(), enabled=enabled ) + self._load() # another worker may have changed the file self._plugins[name] = state - self._save() + self._save(only={name}) return state def unregister(self, name: str) -> None: """Remove plugin from registry.""" - if name in self._plugins: - del self._plugins[name] - self._save() + self._load() # another worker may have registered it since boot + self._plugins.pop(name, None) + self._save(remove={name}) + + def refresh(self) -> None: + """Re-read plugins.json before making a decision from it. + + Every worker loads this file once at boot, so any read used to DECIDE + something - is it installed, is a dependency enabled - is answering from + a snapshot that another worker may have moved on from. That is how + enabling endpoints returned 409 "computers is not enabled" moments after + computers had in fact been enabled through a different worker. + """ + self._load() def get(self, name: str) -> Optional[PluginState]: """Get plugin state.""" @@ -104,16 +221,23 @@ class PluginRegistry: return state.enabled if state else False def enable(self, name: str) -> None: - """Enable a plugin.""" + """Enable a plugin. + + Re-reads first: this worker's snapshot dates from boot, so a plugin + another worker installed since is absent here and the old + `if name in self._plugins` guard silently did nothing. + """ + self._load() if name in self._plugins: self._plugins[name].enabled = True - self._save() + self._save(only={name}) def disable(self, name: str) -> None: - """Disable a plugin.""" + """Disable a plugin. Re-reads first, for the same reason as enable.""" + self._load() if name in self._plugins: self._plugins[name].enabled = False - self._save() + self._save(only={name}) def get_enabled_plugins(self) -> List[str]: """Get list of enabled plugin names.""" @@ -124,10 +248,11 @@ class PluginRegistry: def add_migration(self, name: str, revision: str) -> None: """Record that a migration was applied.""" + self._load() if name in self._plugins: if revision not in self._plugins[name].migrations_applied: self._plugins[name].migrations_applied.append(revision) - self._save() + self._save(only={name}) def get_all(self) -> Dict[str, PluginState]: """Get all registered plugins.""" @@ -135,6 +260,7 @@ class PluginRegistry: def update_config(self, name: str, config: Dict) -> None: """Update plugin configuration.""" + self._load() if name in self._plugins: self._plugins[name].config.update(config) - self._save() + self._save(only={name})