From 01815467aac8ae3b5efb6784023d11d23ca4abf6 Mon Sep 17 00:00:00 2001 From: Aling33 <152494711+Aling33@users.noreply.github.com> Date: Mon, 26 Oct 2026 08:31:47 +0800 Subject: [PATCH] fix(projects): delete read-only documents when purging trash (#7499) * fix(projects): delete read-only documents when purging trash A permanently deleted project document could never be purged on Windows: the read-only file attribute makes `Path.unlink` fail with `PermissionError` (`WinError 5`), where POSIX needs only a writable parent. A restored archive or a mode-preserving sync sets that attribute, and because the purge re-raises, the row rolls back and the trashed document keeps coming back with the same error on every retry. Clear the attribute and retry the unlink once, mirroring the retry that `config/paths.py::_clear_read_only_tree` already performs for thread directories. Any other unlink error still propagates, so the row stays retryable. * fix(projects): harden trash purge permission retries --------- Co-authored-by: Aling33 Co-authored-by: Willem Jiang --- README.md | 4 + .../harness/deerflow/projects/trash.py | 37 +++- backend/tests/AGENTS.md | 8 + backend/tests/test_project_trash.py | 168 ++++++++++++++++++ 4 files changed, 210 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 8ceaeebcf6..963e8c0021 100644 --- a/README.md +++ b/README.md @@ -3534,6 +3534,10 @@ origin project, or to a project you pick when the origin is gone or archived; if the target already holds an identical active file, the entries merge. Deleting a project moves its entire shelf to trash in the same step. +Permanently deleting read-only documents also works on Windows. A file that is +already missing does not block deletion; other filesystem errors keep the entry +in Trash for retry. Permissions on symlink targets are preserved. + ## Scheduled Tasks DeerFlow now includes a first-class scheduled-task MVP in the workspace. diff --git a/backend/packages/harness/deerflow/projects/trash.py b/backend/packages/harness/deerflow/projects/trash.py index fb0f93c841..d976c765b2 100644 --- a/backend/packages/harness/deerflow/projects/trash.py +++ b/backend/packages/harness/deerflow/projects/trash.py @@ -5,9 +5,10 @@ projects-root-relative, so no file ever moves, §10.6); the only filesystem work on the restore path is the post-commit merge cleanup of the discarded namespace, best-effort with the sweep as backstop. Purge unlinks the original and ``derived/converted.md`` inside the repository's continuously -row-locked transaction — ``FileNotFoundError`` counts as already removed, -any other unlink error rolls the row deletion back and keeps the trashed -row retryable. The retention sweep runs lazily on the trash listing and +row-locked transaction — ``FileNotFoundError`` counts as already removed, a +read-only file has its attribute cleared and is unlinked once more, and any +other unlink error rolls the row deletion back and keeps the trashed row +retryable. The retention sweep runs lazily on the trash listing and once at gateway startup (no daemon, §15.9): it invokes the same guarded purge for expired rows, then reconciles storage (``.staging`` and unreferenced files older than 24 hours only) and detects — never deletes — @@ -22,6 +23,7 @@ from __future__ import annotations import logging import os import shutil +import stat from collections.abc import Awaitable, Callable from dataclasses import dataclass, field from datetime import UTC, datetime, timedelta @@ -51,6 +53,30 @@ def _rmdir_if_empty(directory: Path) -> None: pass +def _unlink_document_file(path: Path) -> None: + """Unlink one document file, clearing a Windows read-only bit and retrying. + + Windows refuses ``unlink`` on a read-only file (``WinError 5``) where POSIX + needs only a writable parent, so a restored archive or a mode-preserving + sync leaves the trashed document unpurgeable. ``FileNotFoundError`` counts + as already removed (§8.3), including during the retry. Attribute changes + skip symlinks so their targets keep their permissions. Any other error + propagates so the purge transaction rolls back and keeps the row retryable. + """ + try: + path.unlink() + except FileNotFoundError: + return + except PermissionError: + try: + mode = path.lstat().st_mode + if not stat.S_ISLNK(mode): + path.chmod(mode | stat.S_IWRITE) + path.unlink() + except FileNotFoundError: + return + + def _unlink_document_files(paths: Paths, *, user_id: str, row: dict) -> None: """Worker-thread: unlink a row's original + derived companion, then rmdir empty parents (best-effort). ``FileNotFoundError`` counts as already @@ -60,10 +86,7 @@ def _unlink_document_files(paths: Paths, *, user_id: str, row: dict) -> None: original = original_file_path(paths, user_id=user_id, row=row) derived = converted_markdown_path(paths, user_id=user_id, row=row) for path in (original, derived): - try: - path.unlink() - except FileNotFoundError: - pass + _unlink_document_file(path) namespace = paths.project_document_path(user_id, row["stored_relpath"]) documents_dir = namespace.parents[2] for directory in (original.parent, derived.parent, namespace, namespace.parent, namespace.parent.parent): diff --git a/backend/tests/AGENTS.md b/backend/tests/AGENTS.md index 598886cb32..0b3c7d2f48 100644 --- a/backend/tests/AGENTS.md +++ b/backend/tests/AGENTS.md @@ -172,6 +172,14 @@ hide unpinned calls behind unrelated settings. Channel JSON state coverage likewise checks each writer and Discord reader call within its owning class/method; unrelated encoding literals must not mask a regression. +## Project trash purge + +Purge regressions use real original/companion files and the repository transaction. +Inject initial and persistent unlink failures to exercise Windows retry semantics +on every platform; assert rollback and a later successful purge. Plant real links +with `support.symlinks.symlink_or_skip`, preserve target bytes/modes, and cover +dangling links and removal before retry. Keep filesystem work off the event loop. + ## PostgreSQL batch fixtures Batch fixtures use `support.postgres.asyncpg_test_url` to map libpq `sslmode` diff --git a/backend/tests/test_project_trash.py b/backend/tests/test_project_trash.py index 677aebb3cc..f5499c0136 100644 --- a/backend/tests/test_project_trash.py +++ b/backend/tests/test_project_trash.py @@ -12,11 +12,13 @@ from __future__ import annotations import asyncio import logging import os +import stat from datetime import UTC, datetime, timedelta from pathlib import Path from types import SimpleNamespace import pytest +from support.symlinks import symlink_or_skip from deerflow.persistence.engine import close_engine, get_session_factory, init_engine from deerflow.persistence.projects import ProjectDocumentRepository, ProjectRepository @@ -74,6 +76,17 @@ def _age(path: Path, *, hours: float) -> None: os.utime(path, (ts, ts)) +async def _trashed_document_with_companion(env): + project = await env.projects.create(name="P", user_id=_USER) + row = await _add(env, project["id"], name="notes.txt", data=b"hello") + original = original_file_path(env.paths, user_id=_USER, row=row) + derived = converted_markdown_path(env.paths, user_id=_USER, row=row) + derived.parent.mkdir(parents=True, exist_ok=True) + derived.write_text("# converted", encoding="utf-8") + await _trash(env, row) + return row, original, derived + + class TestPurgeFiles: async def test_purge_unlinks_namespace_files_and_rmdirs_empty_parents(self, env): project = await env.projects.create(name="P", user_id=_USER) @@ -111,6 +124,36 @@ class TestPurgeFiles: assert purged is True assert await env.docs.get(row["id"], include_trashed=True, user_id=_USER) is None + async def test_purge_removes_read_only_document_files(self, env): + """A read-only document still purges. + + Windows refuses ``unlink`` on a read-only file (``WinError 5``) where + POSIX needs only a writable parent, so a restored archive or a + mode-preserving sync must not leave a trashed row unpurgeable. Windows + fails here before the fix; POSIX passes either way. + """ + project = await env.projects.create(name="P", user_id=_USER) + row = await _add(env, project["id"], name="notes.txt", data=b"hello") + derived = converted_markdown_path(env.paths, user_id=_USER, row=row) + derived.parent.mkdir(parents=True, exist_ok=True) + derived.write_text("# converted") + original = original_file_path(env.paths, user_id=_USER, row=row) + await _trash(env, row) + protected = (original, derived) + for path in protected: + path.chmod(0o444) + try: + purged = await env.docs.purge(row["id"], remove_files=make_purge_file_remover(env.paths, user_id=_USER), user_id=_USER) + + assert purged is True + assert not original.exists() + assert not derived.exists() + assert await env.docs.get(row["id"], include_trashed=True, user_id=_USER) is None + finally: + for path in protected: + if path.exists(): + path.chmod(0o644) + async def test_partial_unlink_failure_retains_row_and_retry_completes(self, env, monkeypatch): """§13 purge ordering: original unlinked, derived unlink raises — the row stays trashed (retryable); the retry tolerates the missing @@ -147,6 +190,131 @@ class TestPurgeFiles: assert not derived.exists() assert await env.docs.get(row["id"], include_trashed=True, user_id=_USER) is None + async def test_permission_retry_clears_read_only_on_original_and_companion(self, env, monkeypatch): + row, original, derived = await _trashed_document_with_companion(env) + attempts = dict.fromkeys((original, derived), 0) + real_unlink = Path.unlink + + def windows_unlink(self, *args, **kwargs): + if self in attempts: + attempts[self] += 1 + if attempts[self] == 1: + raise PermissionError("simulated Windows read-only file") + assert self.lstat().st_mode & stat.S_IWRITE, "retry did not clear the read-only bit" + return real_unlink(self, *args, **kwargs) + + for path in attempts: + path.chmod(0o444) + monkeypatch.setattr(Path, "unlink", windows_unlink) + try: + assert await env.docs.purge(row["id"], remove_files=make_purge_file_remover(env.paths, user_id=_USER), user_id=_USER) is True + assert attempts == {original: 2, derived: 2} + assert not original.exists() + assert not derived.exists() + assert await env.docs.get(row["id"], include_trashed=True, user_id=_USER) is None + finally: + for path in attempts: + if path.exists(): + path.chmod(0o644) + + @pytest.mark.parametrize("retry_error_type", [PermissionError, OSError]) + async def test_failed_permission_retry_retains_row_and_retry_completes(self, env, monkeypatch, retry_error_type): + row, original, derived = await _trashed_document_with_companion(env) + attempts = 0 + retry_error = retry_error_type("retry still blocked") + real_unlink = Path.unlink + + def blocked_unlink(self, *args, **kwargs): + nonlocal attempts + if self == derived: + attempts += 1 + if attempts == 1: + raise PermissionError("first unlink blocked") + if attempts == 2: + raise retry_error + return real_unlink(self, *args, **kwargs) + + derived.chmod(0o444) + monkeypatch.setattr(Path, "unlink", blocked_unlink) + remover = make_purge_file_remover(env.paths, user_id=_USER) + try: + with pytest.raises(retry_error_type) as caught: + await env.docs.purge(row["id"], remove_files=remover, user_id=_USER) + assert caught.value is retry_error + assert attempts == 2 + assert not original.exists() + assert derived.exists() + retained = await env.docs.get(row["id"], include_trashed=True, user_id=_USER) + assert retained is not None and retained["trashed_at"] + + assert await env.docs.purge(row["id"], remove_files=remover, user_id=_USER) is True + assert attempts == 3 + assert not derived.exists() + assert await env.docs.get(row["id"], include_trashed=True, user_id=_USER) is None + finally: + if derived.exists(): + derived.chmod(0o644) + + async def test_file_removed_during_permission_retry_still_purges(self, env, monkeypatch): + row, original, derived = await _trashed_document_with_companion(env) + attempts = 0 + real_unlink = Path.unlink + + def concurrently_removed(self, *args, **kwargs): + nonlocal attempts + if self == original: + attempts += 1 + if attempts == 1: + raise PermissionError("first unlink blocked") + real_unlink(self, *args, **kwargs) + raise FileNotFoundError("peer removed file before retry") + return real_unlink(self, *args, **kwargs) + + monkeypatch.setattr(Path, "unlink", concurrently_removed) + assert await env.docs.purge(row["id"], remove_files=make_purge_file_remover(env.paths, user_id=_USER), user_id=_USER) is True + assert attempts == 2 + assert not original.exists() + assert not derived.exists() + assert await env.docs.get(row["id"], include_trashed=True, user_id=_USER) is None + + @pytest.mark.parametrize("target_exists", [True, False], ids=["read-only-target", "dangling-link"]) + async def test_permission_retry_preserves_symlink_target(self, env, monkeypatch, tmp_path, target_exists): + row, original, derived = await _trashed_document_with_companion(env) + target = tmp_path / "outside-document.txt" + attempts = 0 + real_unlink = Path.unlink + + def permission_once(self, *args, **kwargs): + nonlocal attempts + if self == original: + attempts += 1 + if attempts == 1: + raise PermissionError("first symlink unlink blocked") + return real_unlink(self, *args, **kwargs) + + try: + if target_exists: + target.write_bytes(b"outside document storage") + target.chmod(0o444) + target_mode = target.stat().st_mode + original.unlink() + symlink_or_skip(original, target) + monkeypatch.setattr(Path, "unlink", permission_once) + + assert await env.docs.purge(row["id"], remove_files=make_purge_file_remover(env.paths, user_id=_USER), user_id=_USER) is True + assert attempts == 2 + assert not original.is_symlink() + assert not derived.exists() + assert await env.docs.get(row["id"], include_trashed=True, user_id=_USER) is None + if target_exists: + assert target.read_bytes() == b"outside document storage" + assert target.stat().st_mode == target_mode + else: + assert not target.exists() + finally: + if target.exists(): + target.chmod(0o644) + class TestRestoreService: async def test_restore_repoints_without_moving_files(self, env):