Refactor/session for path

src/opensak/db/database.py

@@ -20,7 +20,7 @@
 from typing import Generator
 
 from sqlalchemy import create_engine, event, text
-from sqlalchemy.engine import Engine
+from sqlalchemy.engine import Connection, Engine
 from sqlalchemy.orm import Session, sessionmaker
 
 from opensak.backup.premigration import backup_before_migration
@@ -103,39 +103,67 @@ def init_db(db_path: Path | None = None) -> Engine:
             from opensak.config import get_db_path
             db_path = get_db_path()
 
-    new_engine = _make_engine(db_path)
-    new_session = sessionmaker(
-        bind=new_engine,
+    t0 = time.monotonic()
+    # Opening (create_all + migrations) happens before touching the globals —
+    # if the file is not a valid SQLite database _open_engine() raises and the
+    # current engine is unaffected.
+    new_engine = _open_engine(db_path)
+
+    # Only swap the global pointers after everything above succeeded. The
+    # replaced engine is disposed so repeated init_db() calls don't leave one
+    # connection pool per call behind. Sessions still holding a connection
+    # from it keep working; the connection is closed when they return it.
+    old_engine = _engine
+    _engine = new_engine
+    _SessionLocal = _session_factory(new_engine)
+    if old_engine is not None and old_engine is not new_engine:
+        old_engine.dispose()
+    logger.info("init_db: ready (+%.2fs total)", time.monotonic() - t0)
+    return _engine
+
+
+def _session_factory(engine: Engine | Connection) -> sessionmaker:
+    """Session factory with the settings every OpenSAK session uses."""
+    return sessionmaker(
+        bind=engine,
         autoflush=False,
         autocommit=False,
         expire_on_commit=False,  # keep objects usable after session closes
     )
 
-    # Issue #549: if opening this database is about to migrate its schema,
-    # save a one-time copy of it first. This must happen before create_all(),
-    # which already adds any new tables to an old database. Best-effort:
-    # backup_before_migration() never raises and never blocks the migration.
-    if db_path not in _migrated_paths:
-        backup_before_migration(db_path, SCHEMA_VERSION)
-
-    # Create all tables that don't exist yet (safe to call multiple times).
-    # Do this before touching the globals — if the file is not a valid SQLite
-    # database create_all() raises here and the current engine is unaffected.
-    logger.info("init_db: opening %s", db_path)
-    t0 = time.monotonic()
-    Base.metadata.create_all(new_engine)
-    logger.info("init_db: create_all() done (+%.2fs)", time.monotonic() - t0)
 
-    # Kør schema-migrationer for eksisterende databaser (kun én gang per DB-sti)
-    if db_path not in _migrated_paths:
-        _run_migrations(new_engine)
-        _migrated_paths.add(db_path)
+def _open_engine(db_path: Path) -> Engine:
+    """
+    Create an engine for *db_path* and bring its schema up to date
+    (create_all + migrations), without touching the module globals.
 
-    # Only swap the global pointers after everything above succeeded.
-    _engine = new_engine
-    _SessionLocal = new_session
-    logger.info("init_db: ready (+%.2fs total)", time.monotonic() - t0)
-    return _engine
+    Shared by init_db() (the app's active database) and session_for()
+    (a private engine for a background job on another database).
+    """
+    new_engine = _make_engine(db_path)
+    try:
+        # Issue #549: if opening this database is about to migrate its schema,
+        # save a one-time copy of it first. This must happen before
+        # create_all(), which already adds any new tables to an old database.
+        # Best-effort: backup_before_migration() never raises and never
+        # blocks the migration.
+        if db_path not in _migrated_paths:
+            backup_before_migration(db_path, SCHEMA_VERSION)
+
+        # Create all tables that don't exist yet (safe to call multiple times).
+        logger.info("open_db: opening %s", db_path)
+        t0 = time.monotonic()
+        Base.metadata.create_all(new_engine)
+        logger.info("open_db: create_all() done (+%.2fs)", time.monotonic() - t0)
+
+        # Kør schema-migrationer for eksisterende databaser (kun én gang per DB-sti)
+        if db_path not in _migrated_paths:
+            _run_migrations(new_engine)
+            _migrated_paths.add(db_path)
+    except Exception:
+        new_engine.dispose()
+        raise
+    return new_engine
 
 
 # ── Schema migrationer ────────────────────────────────────────────────────────
@@ -1061,8 +1089,63 @@ def get_session() -> Generator[Session, None, None]:
         session.close()
 
 
-def make_session():
-    """Return a bare Session — caller handles commit/rollback/close."""
+@contextmanager
+def session_for(db_path: Path) -> Generator[Session, None, None]:
+    """
+    Like get_session(), but on a private engine for *db_path* — the active
+    database (init_db()'s globals) is left untouched.
+
+    Use this for background jobs that work on a database other than the
+    active one (import into another DB, move/copy caches, PQ e-mail import).
+    Swapping the globals with init_db() from a worker thread instead would
+    let anything on the GUI thread that calls get_session() meanwhile
+    (timers, queued signals) read from or write to the wrong database.
+
+    The schema is brought up to date first, exactly as init_db() would, and
+    the engine is disposed when the block exits.
+
+    Example
+    -------
+        with session_for(other_db) as session:
+            session.add(some_object)
+    """
+    engine = _open_engine(db_path)
+    try:
+        session: Session = _session_factory(engine)()
+        try:
+            yield session
+            session.commit()
+        except Exception:
+            session.rollback()
+            raise
+        finally:
+            session.close()
+    finally:
+        engine.dispose()
+
+
+def _session_on(db_path: Path | None):
+    """get_session() when *db_path* is None or the database the active
+    engine already points at, otherwise session_for(db_path) — so a helper
+    given another database's path never reads or writes the active one."""
+    if db_path is None or (
+        _engine is not None and _engine.url.database
+        and Path(_engine.url.database).resolve() == Path(db_path).resolve()
+    ):
+        return get_session()
+    return session_for(db_path)
+
+
+def make_session(engine: Engine | Connection | None = None):
+    """
+    Return a bare Session — caller handles commit/rollback/close.
+
+    Bound to the active database, or to *engine* when given (e.g. the bind
+    of a session_for() session, so a helper opens its own session on the
+    same database as the session it was handed).
+    """
+    if engine is not None:
+        return _session_factory(engine)()
     if _SessionLocal is None:
         raise RuntimeError("Database not initialised — call init_db() first.")
     return _SessionLocal()
@@ -1126,12 +1209,12 @@ def recalculate_distances(lat: float, lon: float, db_path: Path | None = None) -
     refresh). Uses distance_km_batch() which dispatches to Haversine or
     Vincenty depending on the user's distance_method setting.
 
-    *db_path* is the database the open engine points at, when that may not
-    be the active one — e.g. GsakImportWorker, which switches the engine to
-    each target database in turn. The centre used is then stored in that
-    file's own settings (see distances_up_to_date()); without it it goes to
-    the active database's settings, which while the engine is switched
-    would be the wrong database.
+    *db_path* is the database to work on when that may not be the active
+    one — e.g. GsakImportWorker, which imports into each target database in
+    turn. Its caches are then read and written through a private session
+    (see session_for()) and the centre used is stored in that file's own
+    settings (see distances_up_to_date()); without it both go to the active
+    database.
 
     Returns the number of caches updated.
     """
@@ -1163,7 +1246,7 @@ def _scalar(la2: float, lo2: float) -> float:
                 return (math.degrees(math.atan2(x2, y2)) + 360) % 360
             return [_scalar(la, lo) for la, lo in zip(lats, lons)]
 
-    with get_session() as session:
+    with _session_on(db_path) as session:
         rows = session.execute(
             text("SELECT id, latitude, longitude FROM caches WHERE latitude IS NOT NULL AND longitude IS NOT NULL")
         ).fetchall()
@@ -1255,7 +1338,7 @@ def distances_up_to_date(lat: float, lon: float, db_path: Path | None = None) ->
     # whether it currently has a persisted distance. Checked first (and
     # unconditionally) so an empty/coordinate-less database is always
     # considered up to date — there is nothing to recalculate either way.
-    with get_session() as session:
+    with _session_on(db_path) as session:
         row = session.execute(
             text(
                 "SELECT latitude, longitude, distance FROM caches "
@@ -1304,7 +1387,7 @@ def distances_up_to_date(lat: float, lon: float, db_path: Path | None = None) ->
 
     # Stops at the first hit; only a database that is up to date pays for
     # the full scan.
-    with get_session() as session:
+    with _session_on(db_path) as session:
         missing = session.execute(
             text(
                 "SELECT 1 FROM caches WHERE distance IS NULL "

src/opensak/email/service.py

@@ -75,32 +75,26 @@ def _match_database(pq_name: str) -> "DatabaseInfo | None":
 def _import_zip_bytes(zip_bytes: bytes, target_db_path: Path | None) -> "ImportResult":
     """
     Importér `zip_bytes` i den angivne database (eller den aktuelt
-    aktive, hvis `target_db_path` er None), og gendan bagefter altid
-    den oprindelige aktive database.
+    aktive, hvis `target_db_path` er None).
 
-    Samme mønster som ImportWorker i import_dialog.py — se dens
-    docstring for hvorfor: init_db() skifter en global session-factory,
-    så den skal altid gendannes i en `finally`-blok, uanset resultatet.
+    En anden database end den aktive får sin egen session via
+    session_for() — den globale engine (init_db()) røres ikke, så
+    GUI-tråden kan ikke ramme den forkerte database imens.
     """
-    from opensak.db.database import get_session, init_db
+    from opensak.db.database import get_session, session_for
     from opensak.db.manager import get_db_manager
     from opensak.importer import import_zip
 
-    manager = get_db_manager()
-    original_path = manager.active_path
-    switched = target_db_path is not None and target_db_path != original_path
-    if switched:
-        init_db(db_path=target_db_path)
+    active_path = get_db_manager().active_path
+    other_db = target_db_path is not None and target_db_path != active_path
 
-    try:
-        with tempfile.TemporaryDirectory() as tmpdir:
-            zip_path = Path(tmpdir) / "pq_email.zip"
-            zip_path.write_bytes(zip_bytes)
-            with get_session() as session:
-                return import_zip(zip_path, session)
-    finally:
-        if switched and original_path is not None:
-            init_db(db_path=original_path)
+    with tempfile.TemporaryDirectory() as tmpdir:
+        zip_path = Path(tmpdir) / "pq_email.zip"
+        zip_path.write_bytes(zip_bytes)
+        with (session_for(target_db_path)
+              if other_db and target_db_path is not None
+              else get_session()) as session:
+            return import_zip(zip_path, session)
 
 
 def scan_and_import(

src/opensak/gui/dialogs/gsak_import_dialog.py

@@ -96,46 +96,46 @@ def __init__(self, db3_path: Path, target_db_path: Path | None = None,
         self.replace = replace                # empty the target DB first
 
     def run(self) -> None:
-        from opensak.db.database import get_session, init_db
+        from opensak.db.database import get_session, session_for
         from opensak.db.manager import get_db_manager
         from opensak.importer.gsak_importer import (
             clear_opensak_cache_data, import_gsak_db,
         )
 
-        manager = get_db_manager()
-        original_path = manager.active_path
-        switched = (
+        # Another database than the active one gets a private session —
+        # never swap the app-wide engine from this thread (see session_for).
+        active_path = get_db_manager().active_path
+        target_path = self.target_db_path or active_path
+        other_db = (
             self.target_db_path is not None
-            and self.target_db_path != original_path
+            and self.target_db_path != active_path
         )
-        if switched:
-            init_db(db_path=self.target_db_path)
 
         try:
-            with get_session() as session:
+            with (session_for(self.target_db_path)
+                  if other_db and self.target_db_path is not None
+                  else get_session()) as session:
                 if self.replace:
                     self.cleared.emit(clear_opensak_cache_data(session))
                 result = import_gsak_db(
                     self.db3_path, session,
                     progress_cb=lambda done, total: self.progress.emit(done, total),
                 )
             self.result_ready.emit(result)
-            self._update_distances(self.target_db_path or original_path)
+            self._update_distances(target_path)
         except Exception:
             import traceback
             self.error.emit(traceback.format_exc())
-        finally:
-            if switched and original_path is not None:
-                init_db(db_path=original_path)
 
     @staticmethod
     def _update_distances(db_path: Path | None) -> None:
         """Bring the imported database's distances up to date while still in
         the background, so neither the refresh after the import nor a later
         switch to that database has to recalculate them on the GUI thread
-        (both still check, as a safety net). Runs while the engine points at
-        *db_path*, so its home point and the centre used are read from and
-        stored in that file, not in the active database's settings. A failure
+        (both still check, as a safety net). Works on *db_path* through its own
+        session when that isn't the active database, and its home point and
+        the centre used are read from and stored in that file, not in the
+        active database's settings. A failure
         here only means that check recalculates later — the import itself
         succeeded.
         """