Fix website DB teardown and showing proper DB error page.

DraTeots · DraTeots · commit 608e079a438b · 2026-07-10T17:43:24.000-04:00
diff --git a/python/rcdb/web/__init__.py b/python/rcdb/web/__init__.py
@@ -4,6 +4,7 @@
 from urllib.parse import urlparse
 
 from flask import Flask, render_template, g, request, url_for
+from sqlalchemy.exc import SQLAlchemyError
 from sqlalchemy.orm import subqueryload
 
 import rcdb
@@ -60,8 +61,13 @@ def _connection_hint(conn_str):
 
 app.config.from_object(__name__)
 
-@app.before_request
-def before_request():
+def _resolve_active_database():
+    """Pick the connection string for this request and expose selector state.
+
+    Sets ``g.active_db_name`` / ``g.available_databases`` (used by the navbar DB
+    selector) and returns the connection string to connect to. Does not touch
+    the database -- only reads config and the ``rcdb_database`` cookie.
+    """
     available_dbs = app.config.get("AVAILABLE_DATABASES", {})
 
     if available_dbs:
@@ -90,25 +96,94 @@ def before_request():
             # No default set, use first available
             active_name = next(iter(available_dbs))
 
-        connection_string = available_dbs[active_name]
         g.active_db_name = active_name
         g.available_databases = available_dbs
-    else:
-        # Original single-database behavior
-        connection_string = app.config["SQL_CONNECTION_STRING"]
-        g.active_db_name = None
-        g.available_databases = {}
-
-    g.tdb = rcdb.ConfigurationProvider()
-    g.tdb.connect(connection_string)
+        return available_dbs[active_name]
+
+    # Original single-database behavior
+    g.active_db_name = None
+    g.available_databases = {}
+    return app.config["SQL_CONNECTION_STRING"]
+
+
+@app.before_request
+def before_request():
+    # Never gate static assets on the database -- the graceful DB-error page
+    # below still needs its CSS/JS to load when the DB is unreachable.
+    if request.endpoint == "static":
+        return None
+
     app.jinja_env.globals['datetime_now'] = datetime.now
 
+    # Resolve the selector state first so the navbar (and the DB-error page)
+    # can render even if the connection below fails.
+    connection_string = _resolve_active_database()
+
+    # Connect fresh, per request. A DB access/connection failure (access
+    # denied, too many connections, timeout, host down, ...) must degrade
+    # gracefully instead of surfacing a raw 500 -- and must not poison any
+    # cookie or cached engine, so a later reload recovers once the DB is back.
+    try:
+        g.tdb = rcdb.ConfigurationProvider()
+        g.tdb.connect(connection_string)
+    except SQLAlchemyError:
+        return _render_db_error(connection_string)
+
+    return None
+
+
+def _render_db_error(connection_string):
+    """Log the full failure detail and render the sanitized DB-error page.
+
+    The *log* gets everything an admin needs (which database, the underlying
+    pymysql/SQLAlchemy error code + message, and the stack trace). The *page*
+    gets none of it -- no connection string, host, user, or exception text --
+    only a red "DB connection failure" strip with the top menu (and the DB
+    selector) intact so the visitor can switch to a working database.
+    """
+    logger.exception(
+        "DB connection/access failure for database '%s' (%s); serving graceful "
+        "error page instead of a 500.",
+        getattr(g, "active_db_name", None) or "<default>",
+        _connection_hint(connection_string),
+    )
+
+    # Drop the half-built provider so its engine/pool is released now rather
+    # than lingering as a broken cached connection -- the teardown below only
+    # disposes fully-connected providers.
+    broken = getattr(g, "tdb", None)
+    if broken is not None:
+        try:
+            broken.disconnect()
+        except Exception:
+            logger.debug("Ignoring error while disposing failed DB engine.",
+                         exc_info=True)
+        g.tdb = None
+
+    # 503 Service Unavailable: the failure is transient-friendly (e.g. "too
+    # many connections" recovers), so signal a retryable condition rather than
+    # a 200 or a permanent error. A plain reload after recovery renders fine.
+    return render_template("db_error.html"), 503
+
+
+@app.errorhandler(SQLAlchemyError)
+def handle_db_error(error):
+    """Catch DB errors raised while a view runs (not just at connect time).
+
+    ``before_request`` already handles the common case (``connect()`` exercises
+    the connection via the schema-version check), but a lazily-issued query in
+    a view can still raise. Route those through the same graceful page instead
+    of a 500. ``g.active_db_name`` / ``g.available_databases`` were set by
+    ``before_request`` before the failure, so the selector still renders.
+    """
+    return _render_db_error(getattr(g.get("tdb", None), "connection_string", ""))
+
 
 @app.teardown_request
 def teardown_request(exception):
-    tdb = getattr(g, 'db', None)
-    if tdb:
-        tdb.close()
+    tdb = getattr(g, 'tdb', None)
+    if tdb is not None:
+        tdb.disconnect()
 
 
 @app.errorhandler(404)
diff --git a/python/rcdb/web/templates/db_error.html b/python/rcdb/web/templates/db_error.html
@@ -0,0 +1,20 @@
+{% extends "layouts/base.html" %}
+
+{% block title %}Database unavailable{% endblock %}
+
+{#
+    Graceful page shown when the selected database cannot be reached or used
+    (access denied, too many connections, timeout, host down, ...). The top
+    menu -- including the database selector -- is inherited from base.html so
+    the visitor can switch to a working database and reload. No connection
+    string, host, user, or exception text is exposed here; the full detail is
+    written to the application log instead.
+#}
+
+{% block body %}
+    <div class="alert alert-danger"
+         role="alert"
+         style="text-align: center; font-weight: bold; margin-top: 20px;">
+        DB connection failure
+    </div>
+{% endblock %}
diff --git a/python/tests/test_web_db_failure.py b/python/tests/test_web_db_failure.py
@@ -0,0 +1,119 @@
+"""Web DB-failure handling.
+
+A database selected in the navbar (stored in the ``rcdb_database`` cookie) may
+be unreachable or access-denied server-side (e.g. the MariaDB ``rcdb`` user has
+no GRANT on ``xem2``/``pionct`` -> pymysql ``(1044, "Access denied ...")``).
+
+These tests pin the required behaviour:
+
+* such a failure renders a graceful page (top menu + DB selector + a red
+  "DB connection failure" strip), never a raw 500;
+* no sensitive detail (connection string, host, user, error code/text) leaks
+  into the page, while the full exception is written to the application log;
+* the failure is not sticky -- reloading after the DB recovers, or switching to
+  a working DB, renders normally with no poisoned cookie or cached engine.
+"""
+
+import unittest
+
+from sqlalchemy.exc import OperationalError
+
+import rcdb
+import rcdb.provider
+import rcdb.web
+
+
+class TestWebDbFailure(unittest.TestCase):
+    def setUp(self):
+        self.app = rcdb.web.app
+        self.app.testing = True
+
+        # Save config we mutate so other tests see the app untouched.
+        self._saved = {k: self.app.config.get(k) for k in
+                       ("AVAILABLE_DATABASES", "DEFAULT_DATABASE")}
+        self.app.config["AVAILABLE_DATABASES"] = {
+            "good": "sqlite:///good",
+            "bad": "sqlite:///bad",
+        }
+        self.app.config["DEFAULT_DATABASE"] = "sqlite:///good"
+
+        self.client = self.app.test_client()
+
+        # Replace connect(): raise an access-denied-style OperationalError for the
+        # "bad" DB (while it is "down"); connect a fresh in-memory schema for the
+        # "good" one. ``bad_down`` flips to False to simulate recovery.
+        self.bad_down = True
+        self._orig_connect = rcdb.ConfigurationProvider.connect
+        test = self
+
+        def fake_connect(provider, connection_string="", check_version=True):
+            if "bad" in connection_string and test.bad_down:
+                raise OperationalError(
+                    "SELECT 1", {},
+                    Exception("(1044, \"Access denied for user 'rcdb'@'%' "
+                              "to database 'bad'\")"))
+            test._orig_connect(provider, "sqlite://", check_version=False)
+            rcdb.provider.destroy_all_create_schema(provider)
+
+        rcdb.ConfigurationProvider.connect = fake_connect
+
+    def tearDown(self):
+        rcdb.ConfigurationProvider.connect = self._orig_connect
+        for k, v in self._saved.items():
+            if v is None:
+                self.app.config.pop(k, None)
+            else:
+                self.app.config[k] = v
+
+    def _get(self, path="/", db=None):
+        # Fresh client per call so the cookie jar carries only what we set here
+        # (the Werkzeug test client ignores a raw Cookie header, hence set_cookie).
+        client = self.app.test_client()
+        if db:
+            client.set_cookie("rcdb_database", db)
+        return client.get(path)
+
+    def test_failure_renders_graceful_page_not_500(self):
+        with self.assertLogs("rcdb.web", level="ERROR") as cm:
+            resp = self._get("/", db="bad")
+
+        # Graceful, retryable status -- never a raw 500.
+        self.assertEqual(resp.status_code, 503)
+        self.assertNotEqual(resp.status_code, 500)
+
+        body = resp.get_data(as_text=True)
+        # Red strip + intact top menu incl. the DB selector to switch away.
+        self.assertIn("DB connection failure", body)
+        self.assertIn('id="dbSelector"', body)
+        self.assertIn(">bad<", body)
+        self.assertIn(">good<", body)
+
+        # No sensitive detail leaks into the page ...
+        for secret in ("1044", "Access denied", "sqlite:///bad", "rcdb@",
+                       "OperationalError", "Traceback"):
+            self.assertNotIn(secret, body)
+
+        # ... but the log has the full picture (db name + underlying error).
+        logtext = "\n".join(cm.output)
+        self.assertIn("bad", logtext)
+        self.assertIn("1044", logtext)
+
+    def test_reload_recovers_after_db_comes_back(self):
+        # Same cookie, DB down -> graceful 503.
+        self.assertEqual(self._get("/", db="bad").status_code, 503)
+
+        # DB recovers; a plain reload (identical cookie) must render normally.
+        self.bad_down = False
+        resp = self._get("/", db="bad")
+        self.assertEqual(resp.status_code, 200)
+        self.assertNotIn("DB connection failure", resp.get_data(as_text=True))
+
+    def test_switching_to_working_db_recovers(self):
+        self.assertEqual(self._get("/", db="bad").status_code, 503)
+        resp = self._get("/", db="good")
+        self.assertEqual(resp.status_code, 200)
+        self.assertNotIn("DB connection failure", resp.get_data(as_text=True))
+
+
+if __name__ == "__main__":
+    unittest.main()
