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