From bc9c027a542442a309c5869f97f77ed46f7c92f5 Mon Sep 17 00:00:00 2001 From: Aari Date: Sun, 19 Jul 2026 17:24:31 +0800 Subject: [PATCH] fix(uploads): claim the converted markdown filename before writing it (#4288) * fix(uploads): claim the converted markdown filename before writing it * doc(changelog): record the converted-markdown filename collision fix Covers both surfaces that report markdown_file: the gateway uploads route and DeerFlowClient.upload_files. * fix(uploads): release the claimed markdown name when conversion fails Claiming the companion .md name before conversion means a conversion that writes nothing leaves the name reserved for the rest of the request, so a later same-stem upload is renamed against a name nothing occupies. Uploading notes.docx (conversion fails) + notes.md stored the user's file as notes_1.md with no notes.md on disk, where main kept notes.md. Discard the claim when conversion returns None, at both call sites that pre-claim it. Covers both victims: a later same-stem .md upload, and the next convertible's companion. --- CHANGELOG.md | 8 + backend/app/gateway/routers/uploads.py | 13 +- backend/packages/harness/deerflow/client.py | 19 +- .../harness/deerflow/utils/file_conversion.py | 8 +- backend/tests/test_client.py | 73 +++++- backend/tests/test_uploads_router.py | 216 +++++++++++++++++- 6 files changed, 324 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ef5c19bd1..6066a460f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -241,6 +241,13 @@ This section accumulates work toward the **2.1.0** milestone blocking filesystem IO in artifact serving, gateway uploads, and the Discord channel; limit the uploaded-file context manifest; and live-tail malformed Redis reconnect ids. ([#3651], [#3551], [#3935], [#3927], [#3917], [#4012]) +- **uploads:** Claim the converted-Markdown companion filename before writing + it, so two convertible uploads sharing a stem (or a convertible plus a + same-stem `.md` upload) no longer silently clobber each other within one + request. When `uploads.auto_convert_documents` is on, the companion `.md` now + gets a unique name (e.g. `a_1.md`); `POST /threads/{id}/uploads` and + `DeerFlowClient.upload_files` both report the actual name in `markdown_file`. + ([#4288]) - **config:** Coerce null object config sections to their defaults; honor the unified database configuration in the store and sync checkpointer; and have legacy DB backfill create missing `Index` objects on existing tables. ([#3573], @@ -1087,3 +1094,4 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#4246]: https://github.com/bytedance/deer-flow/pull/4246 [#4251]: https://github.com/bytedance/deer-flow/pull/4251 [#4264]: https://github.com/bytedance/deer-flow/pull/4264 +[#4288]: https://github.com/bytedance/deer-flow/pull/4288 diff --git a/backend/app/gateway/routers/uploads.py b/backend/app/gateway/routers/uploads.py index f8452cc76..f9b29fed5 100644 --- a/backend/app/gateway/routers/uploads.py +++ b/backend/app/gateway/routers/uploads.py @@ -379,7 +379,13 @@ async def upload_files( file_ext = file_path.suffix.lower() if auto_convert_documents and file_ext in CONVERTIBLE_EXTENSIONS: - md_path = await convert_file_to_markdown(file_path) + # Reserve the companion .md name in this request's seen set + # before writing so conversion cannot silently truncate another + # uploaded or derived file (same invariant as form-part dedupe). + provisional_md_name = Path(safe_filename).with_suffix(".md").name + unique_md_name = claim_unique_filename(provisional_md_name, seen_filenames) + md_output = file_path.with_name(unique_md_name) + md_path = await convert_file_to_markdown(file_path, output_path=md_output) if md_path: written_paths.append(md_path) md_virtual_path = upload_virtual_path(md_path.name) @@ -391,6 +397,11 @@ async def upload_files( file_info["markdown_path"] = str(sandbox_uploads / md_path.name) file_info["markdown_virtual_path"] = md_virtual_path file_info["markdown_artifact_url"] = upload_artifact_url(thread_id, md_path.name) + else: + # Conversion failed and wrote nothing, so release the claim; + # holding it would rename a later same-stem upload against + # a name nothing occupies. + seen_filenames.discard(unique_md_name) uploaded_files.append(file_info) diff --git a/backend/packages/harness/deerflow/client.py b/backend/packages/harness/deerflow/client.py index c8046c0c7..1046e9afa 100644 --- a/backend/packages/harness/deerflow/client.py +++ b/backend/packages/harness/deerflow/client.py @@ -1412,8 +1412,8 @@ class DeerFlowClient: # creating a new ThreadPoolExecutor per converted file. conversion_pool = concurrent.futures.ThreadPoolExecutor(max_workers=1) - def _convert_in_thread(path: Path): - return asyncio.run(convert_file_to_markdown(path)) + def _convert_in_thread(path: Path, output_path: Path | None = None): + return asyncio.run(convert_file_to_markdown(path, output_path=output_path)) try: for src_path, dest_name in resolved_files: @@ -1431,11 +1431,17 @@ class DeerFlowClient: info["original_filename"] = src_path.name if src_path.suffix.lower() in CONVERTIBLE_EXTENSIONS: + # Reserve companion .md name before convert so two stems + # that collapse to the same .md (or a prior .md upload) + # cannot silently overwrite each other. + provisional_md_name = Path(dest_name).with_suffix(".md").name + unique_md_name = claim_unique_filename(provisional_md_name, seen_names) + md_output = dest.with_name(unique_md_name) try: if conversion_pool is not None: - md_path = conversion_pool.submit(_convert_in_thread, dest).result() + md_path = conversion_pool.submit(_convert_in_thread, dest, md_output).result() else: - md_path = asyncio.run(convert_file_to_markdown(dest)) + md_path = asyncio.run(convert_file_to_markdown(dest, output_path=md_output)) except Exception: logger.warning( "Failed to convert %s to markdown", @@ -1449,6 +1455,11 @@ class DeerFlowClient: info["markdown_path"] = str(uploads_dir / md_path.name) info["markdown_virtual_path"] = upload_virtual_path(md_path.name) info["markdown_artifact_url"] = upload_artifact_url(thread_id, md_path.name) + else: + # Conversion failed and wrote nothing, so release the + # claim; holding it would rename a later same-stem + # upload against a name nothing occupies. + seen_names.discard(unique_md_name) uploaded_files.append(info) finally: diff --git a/backend/packages/harness/deerflow/utils/file_conversion.py b/backend/packages/harness/deerflow/utils/file_conversion.py index 5c5a28235..921ea5ad7 100644 --- a/backend/packages/harness/deerflow/utils/file_conversion.py +++ b/backend/packages/harness/deerflow/utils/file_conversion.py @@ -135,7 +135,7 @@ def _do_convert(file_path: Path, pdf_converter: str) -> str: return _convert_with_markitdown(file_path) -async def convert_file_to_markdown(file_path: Path) -> Path | None: +async def convert_file_to_markdown(file_path: Path, output_path: Path | None = None) -> Path | None: """Convert a supported document file to Markdown. PDF files are handled with a two-converter strategy (see module docstring). @@ -144,6 +144,10 @@ async def convert_file_to_markdown(file_path: Path) -> Path | None: Args: file_path: Path to the file to convert. + output_path: Optional destination for the generated ``.md`` file. + When omitted, writes to ``file_path`` with a ``.md`` suffix. + Callers that track per-request filename uniqueness should pass a + pre-claimed path so companion markdown cannot clobber other uploads. Returns: Path to the generated .md file, or None if conversion failed. @@ -157,7 +161,7 @@ async def convert_file_to_markdown(file_path: Path) -> Path | None: else: text = _do_convert(file_path, pdf_converter) - md_path = file_path.with_suffix(".md") + md_path = output_path if output_path is not None else file_path.with_suffix(".md") md_path.write_text(text, encoding="utf-8") logger.info("Converted %s to markdown: %s (%d chars)", file_path.name, md_path.name, len(text)) diff --git a/backend/tests/test_client.py b/backend/tests/test_client.py index ba401d6af..69238d6e3 100644 --- a/backend/tests/test_client.py +++ b/backend/tests/test_client.py @@ -1760,8 +1760,8 @@ class TestUploads: created_executors = [] real_executor_cls = concurrent.futures.ThreadPoolExecutor - async def fake_convert(path: Path) -> Path: - md_path = path.with_suffix(".md") + async def fake_convert(path: Path, output_path: Path | None = None) -> Path: + md_path = output_path if output_path is not None else path.with_suffix(".md") md_path.write_text(f"converted {path.name}") return md_path @@ -1799,6 +1799,75 @@ class TestUploads: assert result["files"][0]["markdown_file"] == "first.md" assert result["files"][1]["markdown_file"] == "second.md" + def test_upload_files_converted_markdown_uses_unique_names_on_stem_collision(self, client): + """Companion .md from convert must not clobber another same-stem companion.""" + with tempfile.TemporaryDirectory() as tmp: + tmp_path = Path(tmp) + uploads_dir = tmp_path / "uploads" + uploads_dir.mkdir() + + docx = tmp_path / "a.docx" + pdf = tmp_path / "a.pdf" + docx.write_bytes(b"DOCX") + pdf.write_bytes(b"PDF") + + async def fake_convert(path: Path, output_path: Path | None = None) -> Path: + md_path = output_path if output_path is not None else path.with_suffix(".md") + md_path.write_text(f"FROM:{path.name}", encoding="utf-8") + return md_path + + with ( + patch("deerflow.client.get_uploads_dir", return_value=uploads_dir), + patch("deerflow.client.ensure_uploads_dir", return_value=uploads_dir), + patch("deerflow.utils.file_conversion.CONVERTIBLE_EXTENSIONS", {".docx", ".pdf"}), + patch("deerflow.utils.file_conversion.convert_file_to_markdown", side_effect=fake_convert), + ): + result = client.upload_files("thread-1", [docx, pdf]) + + assert result["success"] is True + assert result["files"][0]["markdown_file"] == "a.md" + assert result["files"][1]["markdown_file"] == "a_1.md" + assert (uploads_dir / "a.md").read_text(encoding="utf-8") == "FROM:a.docx" + assert (uploads_dir / "a_1.md").read_text(encoding="utf-8") == "FROM:a.pdf" + + def test_upload_files_failed_conversion_releases_the_claimed_markdown_name(self, client): + """A conversion that writes nothing must not reserve stem.md against a later companion. + + Destination names are claimed upfront, so a same-stem ``.md`` upload + always wins ``a.md``; the only reachable victim of a stale claim is the + next convertible's companion. + """ + with tempfile.TemporaryDirectory() as tmp: + tmp_path = Path(tmp) + uploads_dir = tmp_path / "uploads" + uploads_dir.mkdir() + + docx = tmp_path / "a.docx" + pdf = tmp_path / "a.pdf" + docx.write_bytes(b"DOCX") + pdf.write_bytes(b"PDF") + + async def convert_failing_on_docx(path: Path, output_path: Path | None = None) -> Path | None: + if path.suffix.lower() == ".docx": + return None + md_path = output_path if output_path is not None else path.with_suffix(".md") + md_path.write_text(f"FROM:{path.name}", encoding="utf-8") + return md_path + + with ( + patch("deerflow.client.get_uploads_dir", return_value=uploads_dir), + patch("deerflow.client.ensure_uploads_dir", return_value=uploads_dir), + patch("deerflow.utils.file_conversion.CONVERTIBLE_EXTENSIONS", {".docx", ".pdf"}), + patch("deerflow.utils.file_conversion.convert_file_to_markdown", side_effect=convert_failing_on_docx), + ): + result = client.upload_files("thread-1", [docx, pdf]) + + assert result["success"] is True + assert result["files"][0].get("markdown_file") is None + assert result["files"][1]["markdown_file"] == "a.md" + assert (uploads_dir / "a.md").read_text(encoding="utf-8") == "FROM:a.pdf" + assert not (uploads_dir / "a_1.md").exists() + def test_list_uploads(self, client): with tempfile.TemporaryDirectory() as tmp: uploads_dir = Path(tmp) diff --git a/backend/tests/test_uploads_router.py b/backend/tests/test_uploads_router.py index 23290c6e9..b07c19046 100644 --- a/backend/tests/test_uploads_router.py +++ b/backend/tests/test_uploads_router.py @@ -192,8 +192,8 @@ def test_upload_files_syncs_non_local_sandbox_and_marks_markdown_file(tmp_path): sandbox = MagicMock() provider.get.return_value = sandbox - async def fake_convert(file_path: Path) -> Path: - md_path = file_path.with_suffix(".md") + async def fake_convert(file_path: Path, output_path: Path | None = None) -> Path: + md_path = output_path if output_path is not None else file_path.with_suffix(".md") md_path.write_text("converted", encoding="utf-8") return md_path @@ -231,8 +231,8 @@ def test_upload_files_makes_non_local_files_sandbox_writable(tmp_path): sandbox = MagicMock() provider.get.return_value = sandbox - async def fake_convert(file_path: Path) -> Path: - md_path = file_path.with_suffix(".md") + async def fake_convert(file_path: Path, output_path: Path | None = None) -> Path: + md_path = output_path if output_path is not None else file_path.with_suffix(".md") md_path.write_text("converted", encoding="utf-8") return md_path @@ -815,3 +815,211 @@ def test_upload_files_uses_configured_file_count_limit(tmp_path): asyncio.run(call_unwrapped(uploads.upload_files, "thread-local", request=MagicMock(), files=files, config=cfg)) assert exc_info.value.status_code == 413 + + +def _fake_convert_honoring_output_path(content_by_source: dict[str, str] | None = None): + """Mimic convert_file_to_markdown, including optional output_path.""" + + async def fake_convert(file_path: Path, output_path: Path | None = None) -> Path: + md_path = output_path if output_path is not None else file_path.with_suffix(".md") + if content_by_source is not None and file_path.name in content_by_source: + text = content_by_source[file_path.name] + else: + text = f"converted-from:{file_path.name}" + md_path.write_text(text, encoding="utf-8") + return md_path + + return fake_convert + + +def test_upload_files_converted_markdown_does_not_overwrite_user_markdown(tmp_path): + """Companion .md from auto-convert must not clobber a same-request .md upload. + + Declared invariant (upload_files): filenames within one request must not + silently truncate each other. convert_file_to_markdown used to write + stem.md unconditionally, bypassing claim_unique_filename. + """ + thread_uploads_dir = tmp_path / "uploads" + thread_uploads_dir.mkdir(parents=True) + + with ( + patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir), + patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), + patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()), + patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), + patch.object( + uploads, + "convert_file_to_markdown", + AsyncMock(side_effect=_fake_convert_honoring_output_path({"notes.docx": "FROM_DOCX"})), + ), + ): + result = asyncio.run( + call_unwrapped( + uploads.upload_files, + "thread-local", + request=MagicMock(), + files=[ + UploadFile(filename="notes.md", file=BytesIO(b"USER_MARKDOWN")), + UploadFile(filename="notes.docx", file=BytesIO(b"DOCX")), + ], + config=SimpleNamespace(), + ) + ) + + assert result.success is True + assert [f.filename for f in result.files] == ["notes.md", "notes.docx"] + # User upload preserved + assert (thread_uploads_dir / "notes.md").read_bytes() == b"USER_MARKDOWN" + # Converted companion got a unique name instead of overwriting + assert result.files[1].markdown_file == "notes_1.md" + assert (thread_uploads_dir / "notes_1.md").read_text(encoding="utf-8") == "FROM_DOCX" + assert not (thread_uploads_dir / "notes.md").read_text(encoding="utf-8") == "FROM_DOCX" + + +def test_upload_files_two_convertibles_get_distinct_markdown_companions(tmp_path): + """Two convertible files sharing a stem must not share one .md path.""" + thread_uploads_dir = tmp_path / "uploads" + thread_uploads_dir.mkdir(parents=True) + + with ( + patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir), + patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), + patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()), + patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), + patch.object( + uploads, + "convert_file_to_markdown", + AsyncMock(side_effect=_fake_convert_honoring_output_path({"a.docx": "FROM_DOCX", "a.pdf": "FROM_PDF"})), + ), + ): + result = asyncio.run( + call_unwrapped( + uploads.upload_files, + "thread-local", + request=MagicMock(), + files=[ + UploadFile(filename="a.docx", file=BytesIO(b"DOCX")), + UploadFile(filename="a.pdf", file=BytesIO(b"PDF")), + ], + config=SimpleNamespace(), + ) + ) + + assert result.success is True + assert result.files[0].markdown_file == "a.md" + assert result.files[1].markdown_file == "a_1.md" + assert (thread_uploads_dir / "a.md").read_text(encoding="utf-8") == "FROM_DOCX" + assert (thread_uploads_dir / "a_1.md").read_text(encoding="utf-8") == "FROM_PDF" + # Each response entry points at content that belongs to that source + assert (thread_uploads_dir / result.files[0].markdown_file).read_text(encoding="utf-8") == "FROM_DOCX" + assert (thread_uploads_dir / result.files[1].markdown_file).read_text(encoding="utf-8") == "FROM_PDF" + + +def test_upload_files_user_markdown_after_convertible_is_renamed_not_overwritten(tmp_path): + """If convert claims stem.md first, a later same-request .md is renamed.""" + thread_uploads_dir = tmp_path / "uploads" + thread_uploads_dir.mkdir(parents=True) + + with ( + patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir), + patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), + patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()), + patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), + patch.object( + uploads, + "convert_file_to_markdown", + AsyncMock(side_effect=_fake_convert_honoring_output_path({"notes.docx": "FROM_DOCX"})), + ), + ): + result = asyncio.run( + call_unwrapped( + uploads.upload_files, + "thread-local", + request=MagicMock(), + files=[ + UploadFile(filename="notes.docx", file=BytesIO(b"DOCX")), + UploadFile(filename="notes.md", file=BytesIO(b"USER_MARKDOWN")), + ], + config=SimpleNamespace(), + ) + ) + + assert result.success is True + assert result.files[0].filename == "notes.docx" + assert result.files[0].markdown_file == "notes.md" + assert result.files[1].filename == "notes_1.md" + assert result.files[1].original_filename == "notes.md" + assert (thread_uploads_dir / "notes.md").read_text(encoding="utf-8") == "FROM_DOCX" + assert (thread_uploads_dir / "notes_1.md").read_bytes() == b"USER_MARKDOWN" + + +def test_upload_files_failed_conversion_releases_the_claimed_markdown_name(tmp_path): + """A conversion that writes nothing must not reserve stem.md against later uploads.""" + thread_uploads_dir = tmp_path / "uploads" + thread_uploads_dir.mkdir(parents=True) + + with ( + patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir), + patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), + patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()), + patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), + patch.object(uploads, "convert_file_to_markdown", AsyncMock(return_value=None)), + ): + result = asyncio.run( + call_unwrapped( + uploads.upload_files, + "thread-local", + request=MagicMock(), + files=[ + UploadFile(filename="notes.docx", file=BytesIO(b"DOCX")), + UploadFile(filename="notes.md", file=BytesIO(b"USER_MARKDOWN")), + ], + config=SimpleNamespace(), + ) + ) + + assert result.success is True + assert result.files[0].markdown_file is None + assert result.files[1].filename == "notes.md" + assert result.files[1].original_filename is None + assert (thread_uploads_dir / "notes.md").read_bytes() == b"USER_MARKDOWN" + assert not (thread_uploads_dir / "notes_1.md").exists() + + +def test_upload_files_failed_conversion_does_not_push_the_next_companion_to_suffix(tmp_path): + """The second victim of a stale claim: a later convertible's companion.""" + thread_uploads_dir = tmp_path / "uploads" + thread_uploads_dir.mkdir(parents=True) + + async def convert_failing_on_docx(file_path: Path, output_path: Path | None = None) -> Path | None: + if file_path.suffix.lower() == ".docx": + return None + md_path = output_path if output_path is not None else file_path.with_suffix(".md") + md_path.write_text(f"FROM:{file_path.name}", encoding="utf-8") + return md_path + + with ( + patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir), + patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), + patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()), + patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), + patch.object(uploads, "convert_file_to_markdown", AsyncMock(side_effect=convert_failing_on_docx)), + ): + result = asyncio.run( + call_unwrapped( + uploads.upload_files, + "thread-local", + request=MagicMock(), + files=[ + UploadFile(filename="notes.docx", file=BytesIO(b"DOCX")), + UploadFile(filename="notes.pdf", file=BytesIO(b"PDF")), + ], + config=SimpleNamespace(), + ) + ) + + assert result.success is True + assert result.files[0].markdown_file is None + assert result.files[1].markdown_file == "notes.md" + assert (thread_uploads_dir / "notes.md").read_text(encoding="utf-8") == "FROM:notes.pdf" + assert not (thread_uploads_dir / "notes_1.md").exists()