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 <noreply@anthropic.com>

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]

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)
 

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