From 673c23188cbd9b50cb1e9ff9a9d23bd1655cbb5e Mon Sep 17 00:00:00 2001
From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com>
Date: Sat, 10 Oct 2026 19:15:33 -0600
Subject: [PATCH] Request sessions keep objects after commit; guard dict()
 serializers (#811)

A RequestSession no longer expires loaded objects at commit, so a handler serializes what it
committed without a reload query.  An AsyncSession cannot run that reload at all, so this is
groundwork for moving request handling to async.  Background sessions (get_db_session) keep
SQLAlchemy's default: they change data outside the ORM (raw SQL, triggers) and rely on the reload.

Collection, CollectionItem and Channel dict() now run inside the serializer guard.  Fixes it found:
- Channel.dict() no longer loads the deferred yt-dlp catalog (info_json), which the channel
  handler loaded only to discard.
- get_channel eager-loads what Channel.dict() reads (populate_existing replaces a refresh query).
- create_channel / get_or_create_download link through relationships, so a loaded
  Collection.downloads stays current.
- Setting a Repo to Never removes the deleted Download from the loaded collection; the response
  reported the old frequency.
- The downloader no longer calls channel.dict() to preload a Channel; its callers read it inside
  the session.

Tests run each request with expiry off, as production does, and turn it back on for the background
work that shares the test Session.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
---
 modules/archive/test/test_api.py        |  4 +++-
 modules/repos/lib.py                    |  2 ++
 modules/videos/channel/api.py           |  1 -
 modules/videos/channel/lib.py           | 27 ++++++++++++-------------
 modules/videos/channel/test/test_api.py |  2 ++
 modules/videos/channel/test/test_lib.py |  4 ++--
 modules/videos/downloader.py            |  8 --------
 modules/videos/models.py                |  4 ++++
 wrolpi/collections/models.py            |  5 ++++-
 wrolpi/common.py                        |  6 +++++-
 wrolpi/conftest.py                      | 23 +++++++++++++++++++--
 wrolpi/db.py                            |  8 +++++++-
 wrolpi/test/test_db.py                  | 19 +++++++++++++++++
 13 files changed, 82 insertions(+), 31 deletions(-)

diff --git a/modules/archive/test/test_api.py b/modules/archive/test/test_api.py
index fb728fa8..c7dad355 100644
--- a/modules/archive/test/test_api.py
+++ b/modules/archive/test/test_api.py
@@ -604,6 +604,8 @@ async def test_get_archive_loads_history_eagerly(test_session, archive_factory,
     test_session.commit()
 
     async def get(archive):
+        url = f'/api/archive/{archive.file_group_id}?skip_viewed=true'
+        test_session.expire_all()  # Each request starts from the same Session state.
         statements = []
 
         def count(conn, cursor, statement, *_):
@@ -611,7 +613,7 @@ async def test_get_archive_loads_history_eagerly(test_session, archive_factory,
 
         event.listen(Engine, 'before_cursor_execute', count)
         try:
-            request, response = await async_client.get(f'/api/archive/{archive.file_group_id}?skip_viewed=true')
+            request, response = await async_client.get(url)
         finally:
             event.remove(Engine, 'before_cursor_execute', count)
         assert response.status_code == HTTPStatus.OK, response.json
diff --git a/modules/repos/lib.py b/modules/repos/lib.py
index f6d6b663..ecbfa40a 100644
--- a/modules/repos/lib.py
+++ b/modules/repos/lib.py
@@ -738,6 +738,8 @@ def _set_repository_download(session: Session, repo: Repository, frequency: int,
     (`frequency` is NEVER) has none; with `clone`, it is cloned once if its clone is missing."""
     if not frequency:
         if download := repo.download:
+            # A deleted object stays in loaded collections; remove it so the Repo reads as never updated.
+            repo.collection.downloads.remove(download)
             session.delete(download)
         if clone and not (repo.directory / '.git').exists():
             request_repository_update(session, repo)
diff --git a/modules/videos/channel/api.py b/modules/videos/channel/api.py
index 50a52023..b1c1ff7b 100644
--- a/modules/videos/channel/api.py
+++ b/modules/videos/channel/api.py
@@ -37,7 +37,6 @@ async def get_channels(request: Request):
 def channel_get(request: Request, channel_id: int = None):
     session = request.ctx.session
     channel = lib.get_channel(session, channel_id=channel_id, return_dict=True)
-    channel.pop('info_json')
     return json_response({'channel': channel})
 
 
diff --git a/modules/videos/channel/lib.py b/modules/videos/channel/lib.py
index c514c5da..dad68f6a 100644
--- a/modules/videos/channel/lib.py
+++ b/modules/videos/channel/lib.py
@@ -3,7 +3,7 @@ from pathlib import Path
 from typing import List, Optional, Union
 
 from sqlalchemy import or_, func, desc
-from sqlalchemy.orm import Query, Session, joinedload
+from sqlalchemy.orm import Query, Session
 
 from wrolpi import flags
 from wrolpi.collections import Collection
@@ -71,9 +71,11 @@ def get_channel(session: Session, *, channel_id: int = None, source_id: str = No
     """
     channel: COD = None  # noqa
     # Try to find the channel by the most reliable methods first.
-    # Always eagerly load Collection to ensure name/directory/tag_id are available
+    # Load what `Channel.dict()` reads: the Collection (name/directory/tag) and its Downloads.  Overwrite any
+    # copy already in the Session so the Channel reflects the database.
+    query = session.query(Channel).options(*Channel.json_options()).populate_existing()
     if channel_id:
-        channel = session.query(Channel).options(joinedload(Channel.collection)).filter_by(id=channel_id).one_or_none()
+        channel = query.filter_by(id=channel_id).one_or_none()
     if not channel and directory:
         directory = Path(directory)
         if not directory.is_absolute():
@@ -85,31 +87,27 @@ def get_channel(session: Session, *, channel_id: int = None, source_id: str = No
         media_dir = get_media_directory()
         current = directory
         while current != media_dir and current != current.parent:
-            channel = session.query(Channel).options(joinedload(Channel.collection)).join(Collection).filter(
-                Collection.directory == str(current)).one_or_none()
+            channel = query.join(Collection).filter(Collection.directory == str(current)).one_or_none()
             if channel:
                 break
             current = current.parent
     if not channel and source_id:
-        channel = session.query(Channel).options(joinedload(Channel.collection)).filter_by(
-            source_id=source_id).one_or_none()
+        channel = query.filter_by(source_id=source_id).one_or_none()
     if not channel and url:
-        channel = session.query(Channel).options(joinedload(Channel.collection)).filter_by(url=url).one_or_none()
+        channel = query.filter_by(url=url).one_or_none()
     if not channel and url:
         # Try to extract YouTube channel ID from the URL and match by source_id.
         from modules.videos.common import get_youtube_channel_id
         yt_channel_id = get_youtube_channel_id(url)
         if yt_channel_id:
-            channel = session.query(Channel).options(joinedload(Channel.collection)).filter_by(
-                source_id=yt_channel_id).one_or_none()
+            channel = query.filter_by(source_id=yt_channel_id).one_or_none()
     if not channel and name:
-        channel = session.query(Channel).join(Collection).filter(Collection.name == name).one_or_none()
+        channel = query.join(Collection).filter(Collection.name == name).one_or_none()
 
     if not channel:
         raise UnknownChannel(f'No channel matches {channel_id=} {source_id=} {url=} {directory=}')
 
     logger.debug(f'Found {channel=} using {channel_id=} {source_id=} {url=} {directory=}')
-    session.refresh(channel)
     if return_dict:
         statistics = channel.get_statistics()
         channel = channel.dict()
@@ -179,12 +177,13 @@ def create_channel(session: Session, data: schema.ChannelPostRequest, return_dic
         name=data.name,
         kind='channel',
         directory=directory,
+        downloads=[],  # A new Collection has none; nothing to load.
     )
     session.add(collection)
     session.flush([collection])
 
     # Create Channel linked to Collection
-    channel = Channel(collection_id=collection.id)
+    channel = Channel(collection=collection)
     session.add(channel)
     session.flush([channel])
     # Apply the changes now that we've OK'd them
@@ -199,7 +198,7 @@ def create_channel(session: Session, data: schema.ChannelPostRequest, return_dic
 
 def delete_channel(session: Session, *, channel_id: int):
     check_channels_config_imported()
-    channel = Channel.find_by_id(session, channel_id)
+    channel = get_channel(session, channel_id=channel_id, return_dict=False)
 
     channel_dict = channel.dict()
     channel.delete_with_videos()
diff --git a/modules/videos/channel/test/test_api.py b/modules/videos/channel/test/test_api.py
index 578a5a85..c9652875 100644
--- a/modules/videos/channel/test/test_api.py
+++ b/modules/videos/channel/test/test_api.py
@@ -317,6 +317,8 @@ async def test_channel_by_id(test_session, async_client, simple_channel, simple_
     channel = response.json['channel']
     # needs_reorganization should be included for UI to display reorganization status
     assert 'needs_reorganization' in channel
+    # The yt-dlp catalog is large and deferred; it is never loaded for a response.
+    assert 'info_json' not in channel
 
 
 @pytest.mark.asyncio
diff --git a/modules/videos/channel/test/test_lib.py b/modules/videos/channel/test/test_lib.py
index 4797f380..987d22d1 100644
--- a/modules/videos/channel/test/test_lib.py
+++ b/modules/videos/channel/test/test_lib.py
@@ -115,14 +115,14 @@ async def test_channels_no_url(async_client, test_session, test_directory, test_
         directory=str(channel1_directory),
     )
     channel1.url = ''
-    lib.create_channel(test_session, data=channel1)
+    lib.create_channel(test_session, data=channel1, return_dict=False)
 
     channel2 = schema.ChannelPostRequest(
         name='bar',
         directory=str(channel2_directory),
         url=''
     )
-    lib.create_channel(test_session, data=channel2)
+    lib.create_channel(test_session, data=channel2, return_dict=False)
 
     channels = list(test_session.query(Channel))
     assert len(channels) == 2
diff --git a/modules/videos/downloader.py b/modules/videos/downloader.py
index fe4f36ca..d5996d45 100755
--- a/modules/videos/downloader.py
+++ b/modules/videos/downloader.py
@@ -712,7 +712,6 @@ class ChannelDownloader(Downloader, ABC):
         with get_db_session(commit=True) as session:
             channel = get_or_create_channel(session, channel_source_id, channel_url, name, channel_tag_name,
                                             destination=prepared.destination)
-            channel.dict()  # get all attributes while we have the session.
             if channel and channel.id:
                 channel_id = channel.id
                 collection_id = channel.collection_id
@@ -1469,8 +1468,6 @@ def get_or_create_channel(session: Session, source_id: str = None, url: str = No
     """
     try:
         channel = get_channel(session, source_id=source_id, url=url, name=name, return_dict=False)
-        # Get all properties while we have the session.
-        channel.dict()
         return channel
     except UnknownChannel:
         pass
@@ -1481,7 +1478,6 @@ def get_or_create_channel(session: Session, source_id: str = None, url: str = No
     if destination:
         try:
             channel = get_channel(session, directory=destination, return_dict=False)
-            channel.dict()
             return channel
         except UnknownChannel:
             pass
@@ -1498,7 +1494,6 @@ def get_or_create_channel(session: Session, source_id: str = None, url: str = No
     # Reuse the Channel that already owns this directory instead of creating a conflicting one.
     try:
         channel = get_channel(session, directory=channel_directory, return_dict=False)
-        channel.dict()
         return channel
     except UnknownChannel:
         pass
@@ -1519,9 +1514,6 @@ def get_or_create_channel(session: Session, source_id: str = None, url: str = No
             f'Cannot create channel {name!r}: conflicts with an existing channel') from e
     # Create the directory now that the channel is approved.
     channel_directory.mkdir(exist_ok=True)
-    # Get all properties while we have the session.
-    channel.dict()
-
     return channel
 
 
diff --git a/modules/videos/models.py b/modules/videos/models.py
index 20130ade..6c9552c7 100644
--- a/modules/videos/models.py
+++ b/modules/videos/models.py
@@ -632,6 +632,9 @@ class Channel(ModelHelper, Base):
     # Joined: a Channel's name, directory and Tag live on its Collection.
     collection = relationship('Collection', foreign_keys=[collection_id], lazy='joined')
 
+    # The yt-dlp catalog is large and no response reads it.
+    dict_exclude = ('info_json',)
+
     @property
     def downloads(self) -> list:
         """Get recurring downloads from associated Collection.  Once-downloads are excluded because they are not
@@ -966,6 +969,7 @@ class Channel(ModelHelper, Base):
         )
         return d
 
+    @serializer
     def dict(self, with_statistics: bool = False, with_downloads: bool = True) -> dict:
         d = super(Channel, self).dict()
         # Add Collection-delegated properties
diff --git a/wrolpi/collections/models.py b/wrolpi/collections/models.py
index a423e4f0..a18829c9 100644
--- a/wrolpi/collections/models.py
+++ b/wrolpi/collections/models.py
@@ -284,7 +284,8 @@ class Collection(ModelHelper, Base):
             )
         if reset_attempts:
             download.attempts = 0
-        download.collection_id = self.id
+        # Through the relationship, so `self.downloads` stays current if it is loaded.
+        download.collection = self
         return download
 
     @staticmethod
@@ -931,6 +932,7 @@ class Collection(ModelHelper, Base):
             return collection
         raise UnknownCollection(f'Cannot find Collection with directory {path}')
 
+    @serializer
     def dict(self) -> dict:
         """Return dictionary representation."""
         d = super(Collection, self).dict()
@@ -1272,6 +1274,7 @@ class CollectionItem(ModelHelper, Base):
         FileGroup's Tags, or its Zim."""
         return joinedload(CollectionItem.file_group).selectinload(FileGroup.tag_files), joinedload(CollectionItem.zim)
 
+    @serializer
     def dict(self) -> dict:
         """Return dictionary representation."""
         d = super(CollectionItem, self).dict()
diff --git a/wrolpi/common.py b/wrolpi/common.py
index 9d6f1b2e..febbd83b 100644
--- a/wrolpi/common.py
+++ b/wrolpi/common.py
@@ -340,8 +340,12 @@ Base = declarative_base()
 
 class ModelHelper:
 
+    # Columns `dict()` leaves out, such as large deferred columns that no response needs.
+    dict_exclude = ()
+
     def dict(self, *_, **__) -> dict:
-        d = {i.name: getattr(self, i.name) for i in self.__table__.columns}  # noqa
+        d = {i.name: getattr(self, i.name) for i in self.__table__.columns  # noqa
+             if i.name not in self.dict_exclude}
         return d
 
     @classmethod
diff --git a/wrolpi/conftest.py b/wrolpi/conftest.py
index d3990bb5..43f27fd9 100644
--- a/wrolpi/conftest.py
+++ b/wrolpi/conftest.py
@@ -45,7 +45,7 @@ from wrolpi.common import logger, await_background_tasks as await_background_tas
 from wrolpi.common import set_test_media_directory, Base, set_test_config
 from wrolpi.contexts import attach_shared_contexts, initialize_configs_contexts
 from wrolpi.dates import set_test_now
-from wrolpi.db import create_wrolpi_engine, RequestSession
+from wrolpi.db import create_wrolpi_engine, RequestDB, RequestSession, REQUEST_SESSION_OPTIONS
 from wrolpi.downloader import DownloadContext, DownloadManager, DownloadResult, Download, Downloader, \
     downloads_manager_config_context, download_cache_config_context
 from wrolpi.errors import UnrecoverableDownloadError
@@ -90,9 +90,25 @@ def test_session(test_directory) -> Generator[Session, Any, None]:
         """Get the testing db"""
         return test_engine, session
 
+    def fake_get_request_session():
+        """A request shares the testing db, but commits like a `RequestSession`: without expiring.  The default
+        returns once the handler is done (`background_task_listener`, `finish`), for the background code which
+        shares the session."""
+        session.expire_on_commit = REQUEST_SESSION_OPTIONS['expire_on_commit']
+        return test_engine, session
+
+    finish = RequestDB.finish
+
+    def fake_finish(self, ok: bool):
+        try:
+            finish(self, ok)
+        finally:
+            session.expire_on_commit = True
+
     try:
         with mock.patch('wrolpi.db._get_db_session', fake_get_db_session), \
-                mock.patch('wrolpi.db._get_request_session', fake_get_db_session):
+                mock.patch('wrolpi.db._get_request_session', fake_get_request_session), \
+                mock.patch.object(RequestDB, 'finish', fake_finish):
             yield session
     finally:
         session.rollback()
@@ -193,6 +209,9 @@ ROUTES_ATTACHED = False
 @api_app.on_response
 async def background_task_listener(request, response):
     """Wait for all background tasks to finish before returning API response while testing."""
+    if session := getattr(request.ctx, 'session', None):
+        # The handler is done.  Background work shares the test's Session, and expires on commit like production.
+        session.expire_on_commit = True
     await await_switches_()
 
 
diff --git a/wrolpi/db.py b/wrolpi/db.py
index e9a95e7b..cf1ef34b 100644
--- a/wrolpi/db.py
+++ b/wrolpi/db.py
@@ -204,6 +204,12 @@ def create_wrolpi_engine(target: Union[str, pathlib.Path]) -> sqlalchemy.engine.
     return engine
 
 
+# Options for a request's Session.  Committing does not expire loaded objects: the handler serializes them after
+# its commit without a query (see `no_db_access`), and an async Session cannot reload them at all.  A handler that
+# changes data outside the Session (raw SQL, triggers, another connection) must `session.refresh()` what it reads
+# next.  Background Sessions (`get_db_session`) keep SQLAlchemy's default and reload expired objects after a commit.
+REQUEST_SESSION_OPTIONS = dict(expire_on_commit=False)
+
 _engine_lock = threading.Lock()
 _engine: sqlalchemy.engine.Engine = None
 _session_maker: sessionmaker = None
@@ -259,7 +265,7 @@ class RequestSession(Session):
     it.  `close()` releases the connection."""
 
     def __init__(self, engine: sqlalchemy.engine.Engine, **kwargs):
-        super().__init__(**kwargs)
+        super().__init__(**{**REQUEST_SESSION_OPTIONS, **kwargs})
         self._request_engine = engine
         self._request_connection = None
 
diff --git a/wrolpi/test/test_db.py b/wrolpi/test/test_db.py
index c6ac2edf..c859da12 100644
--- a/wrolpi/test/test_db.py
+++ b/wrolpi/test/test_db.py
@@ -262,6 +262,25 @@ def test_request_session_keeps_one_connection_until_closed(test_session):
         session.close()
 
 
+def test_only_request_sessions_keep_objects_after_commit(test_session):
+    """A request serializes what it committed without a query (an async Session could not query at all).  Background
+    Sessions reload objects after a commit, because they change data outside the Session (raw SQL, triggers)."""
+    test_session.add(Tag(name='one', color='#000000'))
+    test_session.commit()
+    with production_like_sessions(test_session) as maker:
+        request_session = RequestSession(maker.kw['bind'])
+        tag = request_session.query(Tag).one()
+        request_session.commit()
+        with no_db_access('reading after a commit'):
+            assert tag.name == 'one'
+        request_session.close()
+
+        with get_db_session() as session:
+            tag = session.query(Tag).one()
+            session.commit()
+            assert sqlalchemy.inspect(tag).expired
+
+
 @pytest.mark.allow_unexpected_queries
 def test_no_db_access_refuses_queries_during_tests(test_session):
     """Serializing must not query: the data must already be loaded."""