From 4202e6a7e54d324f57137696cb873073eaa6fcd1 Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Sat, 10 Oct 2026 19:40:55 -0600 Subject: [PATCH] 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 * 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 --------- Co-authored-by: Claude Opus 5.5 --- modules/archive/api.py | 8 ++++---- modules/archive/lib.py | 15 +++++++++++++-- modules/archive/test/test_api.py | 7 +++++-- modules/archive/test/test_lib.py | 23 +++++++++++++++++++++++ wrolpi/files/downloader.py | 4 ++-- wrolpi/files/lib.py | 12 ++++++++++-- wrolpi/files/test/test_lib.py | 18 +++++++++++++++++- 7 files changed, 74 insertions(+), 13 deletions(-) diff --git a/modules/archive/api.py b/modules/archive/api.py index ee4db91b..cd912fa7 100644 --- a/modules/archive/api.py +++ b/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: diff --git a/modules/archive/lib.py b/modules/archive/lib.py index 6c1f11b2..e51a4aa0 100644 --- a/modules/archive/lib.py +++ b/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): diff --git a/modules/archive/test/test_api.py b/modules/archive/test/test_api.py index c7dad355..fb69d3a4 100644 --- a/modules/archive/test/test_api.py +++ b/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 = [ diff --git a/modules/archive/test/test_lib.py b/modules/archive/test/test_lib.py index 9ba1adc0..96eeafb7 100644 --- a/modules/archive/test/test_lib.py +++ b/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 diff --git a/wrolpi/files/downloader.py b/wrolpi/files/downloader.py index 43f90d01..1090fa18 100644 --- a/wrolpi/files/downloader.py +++ b/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( diff --git a/wrolpi/files/lib.py b/wrolpi/files/lib.py index e6d1818d..7a9aca64 100644 --- a/wrolpi/files/lib.py +++ b/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: diff --git a/wrolpi/files/test/test_lib.py b/wrolpi/files/test/test_lib.py index 193ff7b8..7072803e 100644 --- a/wrolpi/files/test/test_lib.py +++ b/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()