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.
This commit is contained in:
Aari
2026-07-19 17:24:31 +08:00
committed by GitHub
parent 4746a2579b
commit bc9c027a54
6 changed files with 324 additions and 13 deletions
+8
View File
@@ -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
+12 -1
View File
@@ -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)
+15 -4
View File
@@ -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:
@@ -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))
+71 -2
View File
@@ -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)
+212 -4
View File
@@ -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()