From 89840935528637a3766f29711654e964fcc9095d Mon Sep 17 00:00:00 2001 From: llb555 <3103086905@qq.com> Date: Sat, 24 Oct 2026 21:56:38 +0800 Subject: [PATCH] fix(uploads): make companion record removal best-effort (#7409) * fix(uploads): make companion record removal best-effort * style(uploads): format the new test and log the markdown-reference removal * fix(uploads): report the orphaned record through the module that owns it --- .../harness/deerflow/uploads/companions.py | 33 ++++++++++- backend/tests/test_upload_companions.py | 59 ++++++++++++++++++- .../test_upload_ingestion_cancellation.py | 4 ++ 3 files changed, 93 insertions(+), 3 deletions(-) diff --git a/backend/packages/harness/deerflow/uploads/companions.py b/backend/packages/harness/deerflow/uploads/companions.py index c82456ff46..c79ccb22ab 100644 --- a/backend/packages/harness/deerflow/uploads/companions.py +++ b/backend/packages/harness/deerflow/uploads/companions.py @@ -11,6 +11,7 @@ import json import logging import os import tempfile +from contextlib import suppress from pathlib import Path from deerflow.utils.atomic_write import cleanup_failed_atomic_write @@ -60,8 +61,31 @@ def register_companion(original: Path, markdown: Path) -> None: raise +def _warn_record_removal_failed(target: Path, *, exc_info: bool) -> None: + """Report a record we could not remove without adding a new failure. + + Diagnostics are best-effort, exactly like ``cleanup_failed_atomic_write``: + a logging handler that raises must not become the caller's error. + """ + with suppress(Exception): + logger.warning("Failed to remove upload companion record for %s", target, exc_info=exc_info) + + def unregister_companion(original: Path) -> None: - _record_path(original.parent, original.name).unlink(missing_ok=True) + """Forget the ownership record for ``original``, best-effort. + + Callers run this on paths whose real outcome is already decided: an upload + that has just been published, or a rejected request being cleaned up. A + record that cannot be removed (a Windows handle still open, a read-only + mount) must not turn that outcome into an error. Dropping the removal is + safe rather than merely convenient: ``resolve_companion`` validates the + recorded source identity, so a leftover record fails validation instead of + resolving to the wrong companion. + """ + try: + _record_path(original.parent, original.name).unlink(missing_ok=True) + except OSError: + _warn_record_removal_failed(original, exc_info=True) def invalidate_overwritten_upload(path: Path) -> None: @@ -77,7 +101,12 @@ def invalidate_overwritten_upload(path: Path) -> None: if record.is_symlink(): continue if json.loads(record.read_text(encoding="utf-8")).get("markdown") == path.name: - record.unlink(missing_ok=True) + try: + record.unlink(missing_ok=True) + except OSError: + # The upload already succeeded; report the orphan instead of + # replacing that outcome, as ``unregister_companion`` does. + _warn_record_removal_failed(record, exc_info=True) except (OSError, ValueError, AttributeError): continue diff --git a/backend/tests/test_upload_companions.py b/backend/tests/test_upload_companions.py index fa18b7b30a..98935d9ac4 100644 --- a/backend/tests/test_upload_companions.py +++ b/backend/tests/test_upload_companions.py @@ -9,7 +9,13 @@ from types import SimpleNamespace import pytest from deerflow.sandbox.local.local_sandbox import LocalSandbox, PathMapping -from deerflow.uploads.companions import companion_names, register_companion, resolve_companion +from deerflow.uploads.companions import ( + companion_names, + invalidate_overwritten_upload, + register_companion, + resolve_companion, + unregister_companion, +) from deerflow.utils.file_outline import extract_outline_for_file @@ -122,3 +128,54 @@ def test_new_conversion_restores_outline_after_source_edit(converted_upload): assert resolve_companion(original) == markdown assert extract_outline_for_file(original)[0] == [{"title": "Replaced B", "line": 1}] + + +def _fail_record_unlink(monkeypatch): + """Make deleting an ownership record fail the way a held handle does.""" + real_unlink = Path.unlink + + def unlink(self, missing_ok=False): + if self.suffix == ".json" and self.parent.name == "upload-companions": + raise PermissionError(13, "The process cannot access the file", str(self)) + return real_unlink(self, missing_ok=missing_ok) + + monkeypatch.setattr(Path, "unlink", unlink) + + +def test_unregister_tolerates_a_record_that_cannot_be_removed(converted_upload, monkeypatch, caplog): + """Removal runs after the caller's outcome is decided, so it cannot raise.""" + original, _ = converted_upload + record = next((original.parent.parent.parent / "upload-companions").glob("*.json")) + _fail_record_unlink(monkeypatch) + + with caplog.at_level("WARNING", logger="deerflow.uploads.companions"): + unregister_companion(original) + + assert "Failed to remove upload companion record" in caplog.text + assert record.exists() + + +def test_overwrite_invalidation_tolerates_a_record_that_cannot_be_removed(converted_upload, monkeypatch, caplog): + """The embedded upload path publishes first, so this must not raise either.""" + original, _ = converted_upload + _fail_record_unlink(monkeypatch) + + with caplog.at_level("WARNING", logger="deerflow.uploads.companions"): + invalidate_overwritten_upload(original) + + assert "Failed to remove upload companion record" in caplog.text + + +def test_record_removal_survives_a_broken_log_handler(converted_upload, monkeypatch): + """Diagnostics are best-effort too: a raising handler cannot fail the caller.""" + from deerflow.uploads import companions as companions_module + + original, _ = converted_upload + _fail_record_unlink(monkeypatch) + + def broken_warning(*_args, **_kwargs): + raise RuntimeError("synthetic logging handler failure") + + monkeypatch.setattr(companions_module.logger, "warning", broken_warning) + + unregister_companion(original) diff --git a/backend/tests/test_upload_ingestion_cancellation.py b/backend/tests/test_upload_ingestion_cancellation.py index a4761c0794..63ad5e3a73 100644 --- a/backend/tests/test_upload_ingestion_cancellation.py +++ b/backend/tests/test_upload_ingestion_cancellation.py @@ -14,6 +14,7 @@ from httpx import ASGITransport, AsyncClient from app.gateway import upload_ingestion from app.gateway.routers import uploads +from deerflow.uploads import companions from deerflow.utils.file_io import run_file_io @@ -427,6 +428,9 @@ def test_rejected_request_preserves_outcome_when_cleanup_fails(local_uploads, mo monkeypatch.setattr(Path, "unlink", unlink_record) monkeypatch.setattr(uploads.logger, "warning", warn) monkeypatch.setattr(upload_ingestion.logger, "warning", warn) + # The ownership record belongs to the companion module, so a failed removal + # is reported there rather than through the upload handler. + monkeypatch.setattr(companions.logger, "warning", warn) error_log = MagicMock() monkeypatch.setattr(uploads.logger, "error", error_log)