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

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
 

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)

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)