Downloads and uploads: don't read ORM objects after their session is gone (#812)

* File downloads: return plain values from upsert_file

Every file download failed after the file was saved, with DetachedInstanceError reading
`fg.location`.  `upsert_file` returned a FileGroup from a session it had committed, and the
caller read it after the session was gone.  SQLAlchemy 1.3 kept that session alive in a
reference cycle until the next garbage collection, so the read worked by accident; 2.0 frees it
as the `with` block ends.  Failed downloads retry, re-downloading their files.

`upsert_file` now returns `UpsertedFile(file_group_id, location)`, read inside its session.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Archive uploads: return plain values from singlefile_to_archive

Same failure as upsert_file: `singlefile_to_archive` returned an Archive from a committed session,
and the upload handler read `archive.file_group.title/url` and `archive.location` after the
session was freed, so every SingleFile upload raised DetachedInstanceError after the Archive was
saved.  It now returns `UploadedArchive(file_group_id, name, url, location)`, read inside its
session.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

modules/archive/api.py

@@ -181,7 +181,7 @@ async def singlefile_upload_switch_handler(url=None):
         # abort the whole transaction — the loser of the race rolls back its
         # own savepoint, set_tags() re-resolves the name, and finds the row
         # the winner just committed.
-        file_group_id = archive.file_group.id
+        file_group_id = archive.file_group_id
         with get_db_session(commit=True) as session:
             for name in tag_names:
                 if Tag.get_by_name(session, name):
@@ -197,10 +197,10 @@ async def singlefile_upload_switch_handler(url=None):
             fg = session.query(FileGroup).filter_by(id=file_group_id).one()
             fg.set_tags(session, list(tag_names))
 
-    name = archive.file_group.title or archive.file_group.url
-    logger.info(f'Created Archive from upload ({q_size}): {archive}')
+    name = archive.name
+    logger.info(f'Created Archive from upload ({q_size}): {name}')
     Events.send_archive_uploaded(f'Created Archive from upload: {name}', url=archive.location)
-    url = archive.file_group.url
+    url = archive.url
     with get_db_session(commit=True) as session:
         if url and (download := Download.get_by_url(session, url)):
             if (download.is_failed or download.is_deferred) and download.downloader == ArchiveDownloader.name:

modules/archive/lib.py

@@ -1644,7 +1644,16 @@ async def extract_singlefile_artifacts(singlefile: bytes) -> SinglefileArtifacts
     return SinglefileArtifacts(url=url, singlefile=singlefile, readability=readability, screenshot=screenshot)
 
 
-async def singlefile_to_archive(singlefile: bytes, destination: pathlib.Path = None) -> Archive:
+@dataclass(frozen=True)
+class UploadedArchive:
+    """The Archive `singlefile_to_archive` created.  Plain values: its Session is gone once it returns."""
+    file_group_id: int
+    name: str
+    url: Optional[str]
+    location: str
+
+
+async def singlefile_to_archive(singlefile: bytes, destination: pathlib.Path = None) -> UploadedArchive:
     """
     Convert a SingleFile to an Archive.
 
@@ -1662,7 +1671,9 @@ async def singlefile_to_archive(singlefile: bytes, destination: pathlib.Path = N
                                   artifacts.screenshot, destination=destination)
     with get_db_session(commit=True) as session:
         archive = register_archive(session, written)
-    return archive
+        file_group = archive.file_group
+        return UploadedArchive(file_group_id=file_group.id, name=file_group.title or file_group.url,
+                               url=file_group.url, location=archive.location)
 
 
 async def get_statistics(session: Session):

modules/archive/test/test_api.py

@@ -5,6 +5,7 @@ from unittest.mock import patch
 import pytest
 
 from modules.archive import lib, Archive
+from modules.archive.lib import UploadedArchive
 from wrolpi.common import get_relative_to_media_directory
 from wrolpi.test.common import skip_ci, skip_macos
 
@@ -235,7 +236,8 @@ async def test_archive_upload_with_tags(test_session, async_client, singlefile_c
     test_session.commit()
 
     async def fake_singlefile_to_archive(singlefile, destination=None):
-        return archive
+        return UploadedArchive(file_group_id=archive.file_group_id, name=archive.file_group.title,
+                               url=archive.file_group.url, location=archive.location)
 
     singlefile_contents = singlefile_contents_factory(title='tagged', url='https://example.com/tagged')
     forms = [
@@ -265,7 +267,8 @@ async def test_archive_upload_without_tags_unchanged(test_session, async_client,
     test_session.commit()
 
     async def fake_singlefile_to_archive(singlefile, destination=None):
-        return archive
+        return UploadedArchive(file_group_id=archive.file_group_id, name=archive.file_group.title,
+                               url=archive.file_group.url, location=archive.location)
 
     singlefile_contents = singlefile_contents_factory(title='no-tags', url='https://example.com/no-tags')
     forms = [

modules/archive/test/test_lib.py

@@ -1402,3 +1402,26 @@ async def test_get_statistics(test_session, archive_factory):
     assert stats['archives']['archives'] == 4
     # Should count 2 domain collections (example.com and example.org).
     assert stats['domains']['domains'] == 2
+
+
+@pytest.mark.asyncio
+async def test_singlefile_to_archive_result_outlives_its_session(async_client, test_session, test_directory,
+                                                                  singlefile_contents_factory):
+    """`singlefile_to_archive`'s Session is gone once it returns (production frees it), so its result must not
+    need it."""
+    import sqlalchemy.orm
+    from unittest import mock
+
+    singlefile = singlefile_contents_factory(title='upload title', url='https://example.com/upload').encode()
+    artifacts = lib.SinglefileArtifacts(url='https://example.com/upload', singlefile=singlefile,
+                                        readability=None, screenshot=None)
+    with mock.patch('modules.archive.lib.extract_singlefile_artifacts', return_value=artifacts):
+        uploaded = await lib.singlefile_to_archive(singlefile)
+    sqlalchemy.orm.close_all_sessions()
+
+    location, name, url = uploaded.location, uploaded.name, uploaded.url
+    archive = test_session.query(Archive).one()
+    assert location == archive.location
+    assert name == 'upload title'
+    assert url == 'https://example.com/upload'
+    assert uploaded.file_group_id == archive.file_group_id

wrolpi/files/downloader.py

@@ -95,8 +95,8 @@ class FileDownloader(Downloader, ABC):
         output_path = None
         try:
             output_path = await self.download_file(download, prepared.url, prepared.destination, ctx=ctx)
-            fg = await upsert_file(output_path, tag_names=download.tag_names)
-            return ExecutedFile(output_path=output_path, location=fg.location)
+            upserted = await upsert_file(output_path, tag_names=download.tag_names)
+            return ExecutedFile(output_path=output_path, location=upserted.location)
         except IgnoredDirectoryError:
             # `upsert_file` says this is an ignored file, that's fine.
             return ExecutedFile(

wrolpi/files/lib.py

@@ -2066,7 +2066,14 @@ def remove_ignored_directory(directory: Union[pathlib.Path, str]):
     get_wrolpi_config().ignored_directories = kept
 
 
-async def upsert_file(file: pathlib.Path | str, tag_names: List[str] = None) -> FileGroup:
+@dataclasses.dataclass(frozen=True)
+class UpsertedFile:
+    """The FileGroup `upsert_file` created or updated.  Plain values: its Session is gone once it returns."""
+    file_group_id: int
+    location: str
+
+
+async def upsert_file(file: pathlib.Path | str, tag_names: List[str] = None) -> UpsertedFile:
     """Insert/update all files in the provided file's FileGroup."""
     if not file or not file.is_file():
         raise InvalidFile(f'Cannot upsert file that does not exist: {file}')
@@ -2130,11 +2137,12 @@ async def upsert_file(file: pathlib.Path | str, tag_names: List[str] = None) ->
 
         # Commit all changes in one transaction
         session.commit()
+        upserted = UpsertedFile(file_group_id=file_group.id, location=file_group.location)
 
     # Upsert directories after FileGroup is committed
     upsert_directories([], file.parents)
 
-    return file_group
+    return upserted
 
 
 def get_file_location_href(file: pathlib.Path) -> str:

wrolpi/files/test/test_lib.py

@@ -1338,7 +1338,8 @@ async def test_upsert_file_video_with_channel(async_client, test_session, test_d
     info_json_file.write_text(json.dumps({'duration': 5}))
 
     # Upsert the file, it should be modeled.
-    fg: FileGroup = await lib.upsert_file(video_file)
+    upserted = await lib.upsert_file(video_file)
+    fg = test_session.query(FileGroup).filter_by(id=upserted.file_group_id).one()
     assert fg.model == 'video', 'Upserted file should have been modeled.'
     assert fg.my_video_files(), 'Video file was upserted.'
     assert fg.my_json_files(), 'Info json file should have been found near video file.'
@@ -1350,6 +1351,21 @@ async def test_upsert_file_video_with_channel(async_client, test_session, test_d
     assert video.info_json_path, 'Info json file not found near video file.'
 
 
+@pytest.mark.asyncio
+async def test_upsert_file_result_outlives_its_session(test_session, test_directory, video_bytes):
+    """`upsert_file`'s Session is gone once it returns (production frees it), so its result must not need it."""
+    video_file = test_directory / 'video.mp4'
+    video_file.write_bytes(video_bytes)
+
+    upserted = await lib.upsert_file(video_file)
+    sqlalchemy.orm.close_all_sessions()
+
+    location = upserted.location
+    file_group = test_session.query(FileGroup).one()
+    assert location == file_group.location
+    assert upserted.file_group_id == file_group.id
+
+
 @only_macos
 def test_get_real_path_name(test_directory):
     (test_directory / 'foo.txt').touch()