From 604d467861deab1e0236a7c99cf24b87583877ca Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:42:27 -0600 Subject: [PATCH] Renaming a directory with Collections waits for a concurrent writer (#863) * Remove a file's Tags Directory links when WROLPi deletes it (#861) * Remove a file's Tags Directory links when WROLPi deletes it When WROLPi deleted a tagged file (a forced delete, duplicate Video cleanup, or a poster conversion), the source was unlinked but its Tags Directory hardlink was not. The link became the only copy, so the tags sync refused to delete it and warned on every sync. The links of a file are now removed before WROLPi removes it, and only if the link is the same file as its source. Co-Authored-By: Claude Opus 5.5 * Remove Tags Directory links on path deletes; keep a valid poster's link The file browser deletes by path (`lib.delete`), which unlinks the sources before any FileGroup is deleted, so its tagged files still left orphaned links. It now removes the links of the files it is about to delete, or of every FileGroup in a deleted directory. A poster is no longer unlinked from the Tags Directory unless it is actually removed: a valid JPEG that is not named .jpg is kept with its link, and a converted poster's links are removed only once the conversion succeeds. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 * Renaming a directory with Collections waits for a concurrent writer POST /api/files/rename on a directory changed Collections on the request's deferred transaction. For a plain directory, that transaction was opened before the FileWorker moved the files and wrote only after, so if anything else wrote meanwhile the Collection update failed ("database is locked"): the request returned 409 with the files already moved, and the nested Collections still pointed at the old directory. Renaming a Collection's own directory failed the same way, before moving anything. rename_directory now changes Collections in a `write` (request.ctx.db.write from the endpoint), which begins as a writer. Files are moved outside it; nested Collections are read inside it, after the move. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- wrolpi/files/api.py | 2 +- wrolpi/files/lib.py | 42 ++++++++++++++++++++++------------- wrolpi/files/test/test_api.py | 30 +++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 16 deletions(-) diff --git a/wrolpi/files/api.py b/wrolpi/files/api.py index c6742710..c0ee369f 100644 --- a/wrolpi/files/api.py +++ b/wrolpi/files/api.py @@ -361,7 +361,7 @@ async def post_rename(request: Request, body: schema.Rename): raise FileConflict(f'File already exists: {new_path}') try: - await lib.rename(request.ctx.session, path, body.new_name, send_events=True) + await lib.rename(request.ctx.session, path, body.new_name, send_events=True, write=request.ctx.db.write) except Exception as e: logger.error(f'Failed to rename {path} to {new_path}', exc_info=e) cause = str(e).split('\n', 1)[0] diff --git a/wrolpi/files/lib.py b/wrolpi/files/lib.py index dce501cb..39df0918 100644 --- a/wrolpi/files/lib.py +++ b/wrolpi/files/lib.py @@ -1,4 +1,5 @@ import asyncio +import contextlib import dataclasses import datetime import functools @@ -17,7 +18,7 @@ import urllib.parse import zipfile from itertools import zip_longest from pathlib import Path -from typing import Callable, List, Optional, Tuple, Union, Dict, Generator, Iterable, Set +from typing import Callable, ContextManager, List, Optional, Tuple, Union, Dict, Generator, Iterable, Set import cachetools.func from sqlalchemy import asc, or_, text as sa_text @@ -1950,11 +1951,16 @@ async def rename_file(path: pathlib.Path, new_name: str) -> pathlib.Path: return new_path -async def rename_directory(session: Session, directory: pathlib.Path, new_name: str, send_events: bool = False) \ - -> pathlib.Path: - """Rename a directory. This is done by moving all files into the new directory, and removing the old directory.""" +async def rename_directory(session: Session, directory: pathlib.Path, new_name: str, send_events: bool = False, + write: Callable[[], ContextManager[Session]] = None) -> pathlib.Path: + """Rename a directory. This is done by moving all files into the new directory, and removing the old directory. + + `session` is used to read. Collections are changed in `write()` (`request.ctx.db.write` in a request), which + begins as a writer; the files are moved outside it. Without `write`, Collections are changed in `session`.""" from wrolpi.files.worker import file_worker + write = write or (lambda: contextlib.nullcontext(session)) + new_directory = directory.with_name(new_name) if new_directory.exists(): raise FileConflict(f'Cannot rename {directory} to {new_directory} because it already exists.') @@ -1966,7 +1972,10 @@ async def rename_directory(session: Session, directory: pathlib.Path, new_name: if collection: # Use Collection's move method which handles files, config saves, and cleanup new_directory.mkdir(parents=True, exist_ok=True) - await collection.move_collection(new_directory, session, send_events=send_events) + # move_collection commits its Download changes before it moves the files, so the write lock is not held + # for the move. + with write() as session: + await collection.move_collection(new_directory, session, send_events=send_events) else: # Regular directory rename - move all paths into the new directory. paths = list(directory.iterdir()) @@ -1980,15 +1989,17 @@ async def rename_directory(session: Session, directory: pathlib.Path, new_name: Events.send_file_move_failed(f'Directory rename has failed: {directory}') raise - # Update nested Collections (files already moved by file_worker above) + # Update nested Collections (files already moved by file_worker above). Read them in the write: the move + # took a while, and a transaction begun before it cannot write after another connection has. directory_str = str(directory) - nested_collections = session.query(Collection).filter( - Collection.directory.like(f'{directory_str}/%') - ).all() - for coll in nested_collections: - relative = coll.directory.relative_to(directory) - new_coll_dir = new_directory / relative - await coll.move_collection(new_coll_dir, session, send_events=send_events, with_files=False) + with write() as session: + nested_collections = session.query(Collection).filter( + Collection.directory.like(f'{directory_str}/%') + ).all() + for coll in nested_collections: + relative = coll.directory.relative_to(directory) + new_coll_dir = new_directory / relative + await coll.move_collection(new_coll_dir, session, send_events=send_events, with_files=False) # Remove the old directory (recursive=True to clean up any empty subdirectories # that remained after files were moved out). @@ -1997,10 +2008,11 @@ async def rename_directory(session: Session, directory: pathlib.Path, new_name: return new_directory -async def rename(session: Session, path: pathlib.Path, new_name: str, send_events: bool = False) -> pathlib.Path: +async def rename(session: Session, path: pathlib.Path, new_name: str, send_events: bool = False, + write: Callable[[], ContextManager[Session]] = None) -> pathlib.Path: """Rename a directory or file. Preserve any tags.""" if path.is_dir(): - return await rename_directory(session, path, new_name, send_events=send_events) + return await rename_directory(session, path, new_name, send_events=send_events, write=write) return await rename_file(path, new_name) diff --git a/wrolpi/files/test/test_api.py b/wrolpi/files/test/test_api.py index 6bd16bf2..5648200e 100644 --- a/wrolpi/files/test/test_api.py +++ b/wrolpi/files/test/test_api.py @@ -8,6 +8,7 @@ from unittest import mock import pytest from modules.videos import Video +from wrolpi.collections.models import Collection from wrolpi.common import get_wrolpi_config from wrolpi.conftest import production_like_sessions, write_lock_held_briefly from wrolpi.files.lib import get_mimetype @@ -1669,3 +1670,32 @@ async def test_file_group_writes_wait_for_a_concurrent_writer(test_session, asyn assert session.query(FileGroup).filter_by(id=file_group.id).one_or_none() is None else: assert session.query(TagFile).count() == (1 if step == 'tag' else 0) + + +@pytest.mark.asyncio +@pytest.mark.parametrize('rename', ['old', 'old/Chan']) +async def test_rename_directory_with_collection_waits_for_a_concurrent_writer( + test_session, test_directory, make_files_structure, async_client, refresh_files, rename): + """Renaming a directory which is (or contains) a Collection's directory moves the files and updates the + Collection, even while something else is writing. + + A plain directory's files are moved first, then the Collections inside it are updated; a deferred + transaction opened before the move could not write after it ("database is locked"), which left the files + moved but the Collection pointing at the old directory.""" + make_files_structure({'old/Chan/notes.txt': 'notes'}) + await refresh_files() + test_session.add(Collection(name='Chan', kind='channel', directory=test_directory / 'old/Chan')) + test_session.commit() + db_file = test_session.get_bind().url.database + new_name = 'new' if rename == 'old' else 'Renamed' + expected = test_directory / ('new/Chan' if rename == 'old' else 'old/Renamed') + + with production_like_sessions(test_session) as maker: + with write_lock_held_briefly(db_file, seconds=3): + _, response = await async_client.post('/api/files/rename', + content=json.dumps(dict(path=rename, new_name=new_name))) + assert response.status_code == HTTPStatus.NO_CONTENT, response.body + + assert (expected / 'notes.txt').is_file() + with maker() as session: + assert session.query(Collection).one().directory == expected