diff --git a/docs/PROJECT-MAP.md b/docs/PROJECT-MAP.md index 9f8306c5..f80658c2 100644 --- a/docs/PROJECT-MAP.md +++ b/docs/PROJECT-MAP.md @@ -91,6 +91,6 @@ Manifest-less directories under `plugins/` are core frontend surface and always ## Size -- test functions defined: **1839** (parametrised cases collect higher) +- test functions defined: **1847** (parametrised cases collect higher) - documented API paths: **376** (`docs/openapi.json`, regenerate with `scripts/gen_openapi.py`) diff --git a/docs/api-inventory.json b/docs/api-inventory.json index 25e6b13b..a1febda3 100644 --- a/docs/api-inventory.json +++ b/docs/api-inventory.json @@ -4319,7 +4319,7 @@ "path": "/api/shopdbbackups/finalize-restore", "auth": "admin", "params": "none", - "purpose": "Run the post-restore schema upgrade against the just-loaded database. A restore that loaded an older set is NOT complete until this succeeds", + "purpose": "Queue the post-restore schema upgrade for the in-container finalizer, and retry it if a previous attempt failed. Does NOT run the upgrade in the request: that watcher runs it out of band, so a restore completes whether or not the browser that started it is still open, and Alembic cannot be killed by the worker timeout mid-chain", "example": "curl -X POST -H \"Authorization: Bearer $TOKEN\" http://localhost:5001/api/shopdbbackups/finalize-restore" }, { diff --git a/docs/openapi.json b/docs/openapi.json index 537904df..9541caa0 100644 --- a/docs/openapi.json +++ b/docs/openapi.json @@ -25016,8 +25016,8 @@ "tags": [ "core-shopdbbackups" ], - "summary": "Run the post-restore schema upgrade against the just-loaded database. A restore that loaded an older set is NOT...", - "description": "Run the post-restore schema upgrade against the just-loaded database. A restore that loaded an older set is NOT complete until this succeeds\n\n**Auth:** admin\n\n**Params:** none\n\n**Example:**\n```\ncurl -X POST -H \"Authorization: Bearer $TOKEN\" http://localhost:5001/api/shopdbbackups/finalize-restore\n```", + "summary": "Queue the post-restore schema upgrade for the in-container finalizer, and retry it if a previous attempt failed. Does...", + "description": "Queue the post-restore schema upgrade for the in-container finalizer, and retry it if a previous attempt failed. Does NOT run the upgrade in the request: that watcher runs it out of band, so a restore completes whether or not the browser that started it is still open, and Alembic cannot be killed by the worker timeout mid-chain\n\n**Auth:** admin\n\n**Params:** none\n\n**Example:**\n```\ncurl -X POST -H \"Authorization: Bearer $TOKEN\" http://localhost:5001/api/shopdbbackups/finalize-restore\n```", "security": [ { "bearerAuth": [] diff --git a/scripts/backup/Dockerfile.backup b/scripts/backup/Dockerfile.backup index 89562e00..8c5215b0 100644 --- a/scripts/backup/Dockerfile.backup +++ b/scripts/backup/Dockerfile.backup @@ -15,13 +15,22 @@ FROM mysql:8.4 # needs no outbound calls and no embedded cert. The cert is gitignored (*.pem) # and must be staged at scripts/backup/zscaler-root-ca.pem before building. # -# The trailing * makes the COPY optional: a plain COPY of a gitignored file -# fails the build outright, so nobody outside a TLS-intercepting network - -# another site, or our own CI - could build this image at all. Absent cert, -# the glob matches nothing, the layer is a no-op and the fetches below go out -# over the public trust store. -COPY scripts/backup/zscaler-root-ca.pem* /etc/pki/ca-trust/source/anchors/ -RUN update-ca-trust +# Copied via the directory rather than by name, because the cert is OPTIONAL: it +# is gitignored, so a plain `COPY scripts/backup/zscaler-root-ca.pem` fails the +# build outright and nobody outside a TLS-intercepting network - another site, +# or our own CI - could build this image at all. A trailing glob does not fix +# that either: BuildKit fails a COPY whose pattern matches nothing, which is +# precisely the case being handled. Copying a directory that always exists and +# testing for the file inside it is the form that works both ways. +COPY scripts/backup/ /tmp/buildctx/ +RUN if [ -f /tmp/buildctx/zscaler-root-ca.pem ]; then \ + echo "staging corporate root CA"; \ + cp /tmp/buildctx/zscaler-root-ca.pem /etc/pki/ca-trust/source/anchors/; \ + update-ca-trust; \ + else \ + echo "no corporate root CA staged - using the public trust store"; \ + fi \ + && rm -rf /tmp/buildctx # restic from the distro is often stale; pull the pinned release binary so the # snapshot format and retention flags are predictable across sites. diff --git a/scripts/backup/backup.sh b/scripts/backup/backup.sh old mode 100644 new mode 100755 diff --git a/scripts/backup/restore.sh b/scripts/backup/restore.sh old mode 100644 new mode 100755 index 9e75ad57..6cdae2ec --- a/scripts/backup/restore.sh +++ b/scripts/backup/restore.sh @@ -61,7 +61,7 @@ DBDUMP="${SETDIR}/db.sql.gz" if [ ! -f "${DBDUMP}" ]; then log "local dump not found; restoring set from restic" command -v restic >/dev/null 2>&1 || die "restic not available and no local dump" - restic restore latest --target / --include "${SETDIR}" \ + restic restore latest --tag "${SETNAME}" --target / --include "${SETDIR}" \ || die "restic restore of ${SETNAME} failed" fi [ -f "${DBDUMP}" ] || die "no db.sql.gz for set ${SETNAME}" @@ -101,12 +101,84 @@ if ! grep -rqE "^revision = ['\"]${SET_DATAREV}['\"]" /app/migrations/versions 2 fi log "data revision ${SET_DATAREV} is known to this code - restore may proceed" +# Safety set FIRST. Everything below this point is destructive and none of it +# is transactional: the dump carries DROP TABLE per table, and the instance tree +# is replaced wholesale. Without a copy taken here, a restore of the wrong set - +# or a failure part-way through the load - leaves nothing to go back to. This is +# a local copy on the same volume, so it is cheap and it is NOT a substitute for +# a real backup; it exists to make THIS operation reversible. +SAFETY_STAMP="$(date -u +%Y-%m-%dT%H%M%SZ)" +SAFETY_DIR="${BACKUPS_ROOT}/pre-restore-${SAFETY_STAMP}" +status_write restore safety "${SETNAME}" "Copying the current state aside" +log "taking a pre-restore safety copy into ${SAFETY_DIR}" +mkdir -p "${SAFETY_DIR}" || die "could not create ${SAFETY_DIR}" + +mysqldump \ + --host="${MYSQL_HOST}" \ + --user="${MYSQL_USER}" \ + --password="${MYSQL_PASSWORD}" \ + --single-transaction \ + --routines \ + --triggers \ + --no-tablespaces \ + --databases "${MYSQL_DATABASE}" \ + | gzip -c > "${SAFETY_DIR}/db.sql.gz" \ + || die "pre-restore dump failed - refusing to touch the live database" + +if [ -d /app/instance ]; then + tar -czf "${SAFETY_DIR}/instance.tar.gz" -C /app instance \ + || die "pre-restore instance copy failed - refusing to touch the live files" +fi + +# Named so an operator finding this directory months later knows what it is and +# how to use it, without reading this script. +cat > "${SAFETY_DIR}/README.txt" </dev/null 2>&1 && [ -n "${RESTIC_REPOSITORY:-}" ]; then status_write restore files "${SETNAME}" "Restoring instance files" - log "restoring instance files from restic" - restic restore latest --target / --include /app/instance \ - || die "restic restore of instance files failed" + # Scoped to THIS set's snapshot. A bare 'latest' restores whatever was + # snapshotted most recently, so restoring anything other than the newest set + # paired that set's DATABASE with the NEWEST snapshot's uploads and + # plugins.json - a mismatch nothing downstream could detect. + log "restoring instance files from the ${SETNAME} snapshot" + STAGE="$(mktemp -d)" + restic restore latest --tag "${SETNAME}" --target "${STAGE}" \ + --include /app/instance \ + || die "restic restore of instance files for ${SETNAME} failed" + + # Replace the CONTENTS of the instance tree rather than restoring over it, + # so files created after the snapshot are gone afterwards - a restore has to + # put the site back to the snapshot, not merge into it. Done as an explicit + # staged swap rather than 'restic restore --delete --target /', because that + # aims a delete pass at the container root and the blast radius of getting + # its filter wrong is the whole filesystem. /app/instance is a mount, so the + # directory itself has to survive; only what is inside it is replaced. + if [ -d "${STAGE}/app/instance" ]; then + find /app/instance -mindepth 1 -delete \ + || die "could not clear the instance tree before restoring it" + cp -a "${STAGE}/app/instance/." /app/instance/ \ + || die "could not lay down the restored instance tree" + else + log "WARN snapshot for ${SETNAME} carried no /app/instance - leaving files alone" + fi + rm -rf "${STAGE}" fi status_write restore database "${SETNAME}" "Loading database dump" diff --git a/scripts/backup/simulate-restore.sh b/scripts/backup/simulate-restore.sh old mode 100644 new mode 100755 diff --git a/scripts/backup/status.sh b/scripts/backup/status.sh old mode 100644 new mode 100755 diff --git a/scripts/backup/test-restore-datarev.sh b/scripts/backup/test-restore-datarev.sh old mode 100644 new mode 100755 diff --git a/scripts/backup/watch.sh b/scripts/backup/watch.sh old mode 100644 new mode 100755 diff --git a/shopdb/__init__.py b/shopdb/__init__.py index f9e4a1c7..245c46dd 100644 --- a/shopdb/__init__.py +++ b/shopdb/__init__.py @@ -209,6 +209,22 @@ def create_app(config_name: str = None) -> Flask: # Register core blueprints register_blueprints(app) + # Finish any restore that is waiting on its schema upgrade. The backup + # sidecar has no Flask, so it loads the dump and leaves a sentinel; this + # container owns the app context that can run the migrations. Started here + # rather than left to a browser poll, because a restore must complete + # whether or not anyone is still watching it. + # + # Skipped unless a backups store actually exists, which keeps it off every + # deployment that does not run the backup overlay - and out of the test + # suite, which builds this app thousands of times. + if not app.config.get('TESTING'): + from .core.api.shopdbbackups import ( + _backups_configured, start_restore_finalizer) + with app.app_context(): + if _backups_configured(): + start_restore_finalizer(app) + # Personal API token auth shim: recognize `Bearer shopdb_pat_...` before # any JWT decode and mint a request-scoped JWT for the token's owner. from .utils.apitoken_auth import install_apitoken_auth diff --git a/shopdb/core/api/shopdbbackups.py b/shopdb/core/api/shopdbbackups.py index f885b56b..fe19bb44 100644 --- a/shopdb/core/api/shopdbbackups.py +++ b/shopdb/core/api/shopdbbackups.py @@ -17,6 +17,8 @@ load rows that reference a column the live schema has not applied yet. import json import os +import threading +import time import subprocess from datetime import datetime, timezone @@ -182,6 +184,106 @@ def _append_history(kind, outcome, setname, message): pass +# The post-restore upgrade is claimed by renaming the sentinel. rename(2) is +# atomic, so exactly one claimant wins even with several gunicorn workers and a +# browser poll all racing - which matters because the loser would otherwise run +# Alembic concurrently against the same database. +UPGRADE_SENTINEL = '.upgrade-requested' +UPGRADE_CLAIM = '.upgrade-inprogress' +UPGRADE_FAILED = '.upgrade-failed' + + +def _claim_upgrade(): + """Take exclusive ownership of a pending upgrade, or return None. + + Returns the set name on success. Losing the race is not an error: it means + another worker is already finalizing the same restore. + """ + root = _backups_root() + sentinel = os.path.join(root, UPGRADE_SENTINEL) + claim = os.path.join(root, UPGRADE_CLAIM) + try: + os.rename(sentinel, claim) + except OSError: + return None + try: + with open(claim, 'r', encoding='utf-8') as handle: + return handle.read().strip() or 'unknown' + except OSError: + return 'unknown' + + +def _finalize_claimed(setname): + """Run the upgrade for a claimed restore and record how it ended. + + A failure parks the claim as .upgrade-failed rather than putting it back: + re-arming it here would retry a migration that just failed, every poll, + forever. The operator re-arms it explicitly through the finalize endpoint. + """ + root = _backups_root() + claim = os.path.join(root, UPGRADE_CLAIM) + _write_status('restore', 'upgrading', setname, + 'Upgrading restored schema to code') + try: + _run_schema_upgrade() + except Exception as error: # noqa: BLE001 - report any upgrade failure + message = 'Schema upgrade failed: ' + str(error) + try: + os.replace(claim, os.path.join(root, UPGRADE_FAILED)) + except OSError: + pass + _write_status('restore', 'failed', setname, message) + _append_history('restore', 'failed', setname, message) + current_app.logger.error('Post-restore schema upgrade failed: %s', error) + return False + try: + os.remove(claim) + except OSError: + pass + _write_status('restore', 'done', setname, 'Restore complete') + _append_history('restore', 'success', setname, 'Restore complete') + current_app.logger.info('Restore finalized: %s', setname) + return True + + +def start_restore_finalizer(app): + """Watch for a restore awaiting its schema upgrade, and run it. + + WHY THIS EXISTS: the restore itself runs in the Flask-less backup sidecar, + which loads the dump and drops a sentinel because it cannot run + 'flask db upgrade'. Until this watcher, the ONLY thing that acted on that + sentinel was the browser poll loop of the modal that queued the restore - + so closing the tab, or letting a token expire, during the minutes a load + takes left the data at the old revision under new code, with every query on + a newer column failing and nothing retrying. + + Runs in the api container because that is where the app context and the + engine live, and out of band rather than in a request because Alembic can + outrun the gunicorn worker timeout and get killed mid-chain. + """ + interval = max(1, int(os.environ.get('SHOPDB_FINALIZE_POLL_SECONDS', '10'))) + + def _loop(): + while True: + time.sleep(interval) + try: + with app.app_context(): + if not _backups_configured(): + continue + setname = _claim_upgrade() + if setname: + _finalize_claimed(setname) + except Exception: # noqa: BLE001 - a watcher must not die + try: + app.logger.exception('Restore finalizer pass failed') + except Exception: # noqa: BLE001 + pass + + thread = threading.Thread(target=_loop, name='restore-finalizer', daemon=True) + thread.start() + return thread + + def _run_schema_upgrade(): """Bring the just-restored database up to the running code, in-process. @@ -469,55 +571,53 @@ def restore_backup(snapshotid): @shopdbbackups_bp.route('/finalize-restore', methods=['POST']) @require_role('admin') def finalize_restore(): - """Run the post-restore schema upgrade in the app context. + """Make sure a restore awaiting its schema upgrade is queued to be finalized. - The restore itself runs in the Flask-less backup sidecar, which loads the - dump and then drops an .upgrade-requested sentinel because it cannot run - 'flask db upgrade' itself. This endpoint is the api-side half: it brings the - just-restored DATA up to the running CODE with the exact idempotent commands - the deploy path uses, then stamps the final 'done'. Without it a restored - dump that sits at an older revision leaves every newer-migration table - throwing 1054 Unknown column. + This used to RUN the upgrade inline, which made two things true that should + not have been: a restore only completed while a browser tab stayed open on + it, and the upgrade ran inside a request, where Alembic can outrun the + gunicorn worker timeout and be killed part-way through the chain, leaving a + half-applied revision the next attempt trips over. - Idempotent and safe to call when nothing is pending: if there is no - .upgrade-requested sentinel it reports there was nothing to finalize. + The work now belongs to the finalizer watcher in this container + (start_restore_finalizer). This endpoint only ensures something is pending + for it to claim, so it is safe to call repeatedly, and it is how an operator + retries an upgrade that failed. """ if not _backups_configured(): return _unavailable_response() root = _backups_root() - sentinel = os.path.join(root, '.upgrade-requested') - try: - with open(sentinel, 'r', encoding='utf-8') as handle: - setname = handle.read().strip() - except OSError: + sentinel = os.path.join(root, UPGRADE_SENTINEL) + claim = os.path.join(root, UPGRADE_CLAIM) + failed = os.path.join(root, UPGRADE_FAILED) + + if os.path.exists(claim): return success_response( - data={'finalized': False}, - message='No restore is awaiting schema upgrade.') + data={'queued': True, 'inprogress': True}, + message='The schema upgrade is already running.') - _write_status('restore', 'upgrading', setname, - 'Upgrading restored schema to code') - try: - _run_schema_upgrade() - except Exception as error: # noqa: BLE001 - report any upgrade failure - _write_status('restore', 'failed', setname, - 'Schema upgrade failed: ' + str(error)) - _append_history('restore', 'failed', setname, - 'Schema upgrade failed: ' + str(error)) - return error_response(ErrorCodes.INTERNAL_ERROR, - 'Post-restore schema upgrade failed: ' + str(error), - http_code=500) + if os.path.exists(sentinel): + return success_response( + data={'queued': True, 'inprogress': False}, + message='The schema upgrade is queued and will run shortly.') + + # A previous attempt failed and parked itself. Re-arming is the retry. + if os.path.exists(failed): + try: + os.replace(failed, sentinel) + except OSError as error: + return error_response(ErrorCodes.INTERNAL_ERROR, + 'Could not re-queue the schema upgrade: ' + str(error), + http_code=500) + AuditLog.log('restore-finalize-retried', 'Backup') + return success_response( + data={'queued': True, 'inprogress': False, 'retried': True}, + message='The failed schema upgrade has been queued to run again.') - try: - os.remove(sentinel) - except OSError: - pass - _write_status('restore', 'done', setname, 'Restore complete') - _append_history('restore', 'success', setname, 'Restore complete') - AuditLog.log('restore-finalized', 'Backup', entityname=setname) return success_response( - data={'finalized': True, 'snapshotid': setname}, - message='Restore finalized: schema upgraded to the running code.') + data={'queued': False}, + message='No restore is awaiting schema upgrade.') @shopdbbackups_bp.route('/status', methods=['GET']) diff --git a/tests/test_core/test_shopdbbackups_finalize.py b/tests/test_core/test_shopdbbackups_finalize.py new file mode 100644 index 00000000..3e614bd8 --- /dev/null +++ b/tests/test_core/test_shopdbbackups_finalize.py @@ -0,0 +1,142 @@ +"""The post-restore schema upgrade: who runs it, and what happens when it fails. + +A restore loads the dump in the Flask-less backup sidecar and leaves a sentinel, +because only the api container can run the migrations. These tests pin the two +properties that failure taught us to care about: the upgrade must not depend on +a browser staying open, and it must never run twice at once. +""" +import os + +import pytest + +from shopdb.core.api import shopdbbackups + + +@pytest.fixture +def backups_root(tmp_path, monkeypatch): + """Point the backup routes at a real, empty directory.""" + root = tmp_path / 'backups' + root.mkdir() + monkeypatch.setenv(shopdbbackups.BACKUPS_ROOT_ENV, str(root)) + return root + + +def _arm(root, setname='wj-2026-09-06T120000Z'): + (root / shopdbbackups.UPGRADE_SENTINEL).write_text(setname, encoding='utf-8') + return setname + + +def test_only_one_claimant_wins_a_pending_upgrade(app, backups_root): + """Two workers racing the same sentinel must not both run Alembic.""" + setname = _arm(backups_root) + with app.app_context(): + first = shopdbbackups._claim_upgrade() + second = shopdbbackups._claim_upgrade() + + assert first == setname + assert second is None, 'a second worker claimed an upgrade already in flight' + assert not (backups_root / shopdbbackups.UPGRADE_SENTINEL).exists() + assert (backups_root / shopdbbackups.UPGRADE_CLAIM).exists() + + +def test_claiming_nothing_is_not_an_error(app, backups_root): + with app.app_context(): + assert shopdbbackups._claim_upgrade() is None + + +def test_a_successful_finalize_clears_the_claim_and_says_done(app, backups_root, + monkeypatch): + setname = _arm(backups_root) + monkeypatch.setattr(shopdbbackups, '_run_schema_upgrade', lambda: None) + + with app.app_context(): + claimed = shopdbbackups._claim_upgrade() + assert shopdbbackups._finalize_claimed(claimed) is True + + assert not (backups_root / shopdbbackups.UPGRADE_CLAIM).exists() + with app.app_context(): + status = shopdbbackups._read_status('restore') + assert status and status['phase'] == 'done' + assert status['setname'] == setname + + +def test_a_failed_upgrade_parks_itself_instead_of_retrying_forever(app, + backups_root, + monkeypatch): + """A failing migration must not be re-armed automatically. + + Putting the sentinel back would run the same broken chain on every poll for + as long as the container lives. + """ + _arm(backups_root) + + def _boom(): + raise RuntimeError('Can\'t locate revision identified by abc123') + + monkeypatch.setattr(shopdbbackups, '_run_schema_upgrade', _boom) + + with app.app_context(): + claimed = shopdbbackups._claim_upgrade() + assert shopdbbackups._finalize_claimed(claimed) is False + + assert (backups_root / shopdbbackups.UPGRADE_FAILED).exists() + assert not (backups_root / shopdbbackups.UPGRADE_SENTINEL).exists(), \ + 'a failed upgrade re-armed itself and will retry forever' + assert not (backups_root / shopdbbackups.UPGRADE_CLAIM).exists() + with app.app_context(): + status = shopdbbackups._read_status('restore') + assert status and status['phase'] == 'failed' + + +def test_the_endpoint_does_not_run_the_upgrade_itself(app, client, auth_headers, + backups_root, monkeypatch): + """Alembic in a request can outrun the worker timeout and be killed mid-chain.""" + _arm(backups_root) + ran = [] + monkeypatch.setattr(shopdbbackups, '_run_schema_upgrade', + lambda: ran.append(True)) + + response = client.post('/api/shopdbbackups/finalize-restore', + headers=auth_headers) + + assert response.status_code == 200 + assert ran == [], 'the request ran the schema upgrade inline' + assert response.get_json()['data']['queued'] is True + assert (backups_root / shopdbbackups.UPGRADE_SENTINEL).exists(), \ + 'the endpoint consumed the sentinel the watcher needs to claim' + + +def test_the_endpoint_re_arms_a_failed_upgrade(app, client, auth_headers, + backups_root): + """Retrying a failed upgrade is what this endpoint is for now.""" + (backups_root / shopdbbackups.UPGRADE_FAILED).write_text('wj-set', + encoding='utf-8') + + response = client.post('/api/shopdbbackups/finalize-restore', + headers=auth_headers) + + assert response.status_code == 200 + assert response.get_json()['data']['retried'] is True + assert (backups_root / shopdbbackups.UPGRADE_SENTINEL).read_text() == 'wj-set' + assert not (backups_root / shopdbbackups.UPGRADE_FAILED).exists() + + +def test_nothing_pending_reports_nothing_pending(app, client, auth_headers, + backups_root): + response = client.post('/api/shopdbbackups/finalize-restore', + headers=auth_headers) + + assert response.status_code == 200 + assert response.get_json()['data']['queued'] is False + + +def test_an_unconfigured_deployment_refuses(app, client, auth_headers, + tmp_path, monkeypatch): + """No store behind the feature means say so, not 500 on a missing path.""" + monkeypatch.setenv(shopdbbackups.BACKUPS_ROOT_ENV, + str(tmp_path / 'nothing-here')) + + response = client.post('/api/shopdbbackups/finalize-restore', + headers=auth_headers) + + assert response.status_code == 404