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

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

---------

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

* 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>

---------

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