mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
fix(uploads): keep OSError text out of upload, delete and attach 500s (#7217)
* fix(uploads): keep OSError text out of upload, delete and attach 500s Thread upload, upload delete and shelf attach-to-thread put str(exc) in their 500 detail. A staging or unlink OSError names its file, so a permission error, or a disk too full for mkstemp, returned the absolute host path of the thread's uploads directory, user and thread IDs included. Return a fixed detail that still names the caller's file. The existing error log lines keep the full message for operators. * docs(changelog): reference #7217 for the upload 500 detail fix --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
This commit is contained in:
parent
22633f6103
commit
544b8b2112
@ -6916,6 +6916,14 @@ As of 2026-10-21, the milestone includes **829 merged pull requests**.
|
||||
|
||||
### Security
|
||||
|
||||
- **uploads:** Thread uploads (`POST /api/threads/{thread_id}/uploads`), upload
|
||||
deletes (`DELETE /api/threads/{thread_id}/uploads/{filename}`) and shelf
|
||||
attach-to-thread no longer put the raw `OSError` text in their `500` detail. A
|
||||
permission error, or a disk too full to create the staging file, answered with
|
||||
the absolute host path of the thread's uploads directory, including the user
|
||||
and thread IDs. They now return `Failed to upload <name>.` /
|
||||
`Failed to delete <name>.`, and the full error stays in the Gateway log. ([#7217])
|
||||
|
||||
- **agents:** The custom-agent and user-profile routes no longer put the raw
|
||||
exception text in their `500` detail. An unparsable agent `config.yaml` or a
|
||||
filesystem error (`[Errno 13]` and the like) made them answer with the
|
||||
@ -12994,4 +13002,5 @@ with **180 merged pull requests** since the first 2.0 milestone tag.
|
||||
[#7170]: https://github.com/bytedance/deer-flow/pull/7170
|
||||
[#7205]: https://github.com/bytedance/deer-flow/pull/7205
|
||||
[#7207]: https://github.com/bytedance/deer-flow/pull/7207
|
||||
[#7217]: https://github.com/bytedance/deer-flow/pull/7217
|
||||
[#7255]: https://github.com/bytedance/deer-flow/pull/7255
|
||||
|
||||
@ -5256,6 +5256,13 @@
|
||||
|
||||
### 安全
|
||||
|
||||
- **上传:** 线程上传(`POST /api/threads/{thread_id}/uploads`)、删除上传文件
|
||||
(`DELETE /api/threads/{thread_id}/uploads/{filename}`)以及把资料库文档附加到线程,
|
||||
其 `500` 响应的 detail 不再包含原始 `OSError` 文本。此前遇到权限错误,或磁盘已满到
|
||||
无法创建暂存文件时,响应会返回该线程上传目录的宿主机绝对路径(含用户 ID 和线程 ID)。
|
||||
现在它们返回 `Failed to upload <name>.` / `Failed to delete <name>.`,完整错误只记录在
|
||||
Gateway 日志中。([#7217])
|
||||
|
||||
- **agents:** 自定义智能体与用户画像路由的 `500` detail 不再包含原始异常文本。
|
||||
此前无法解析的智能体 `config.yaml` 或文件系统错误(如 `[Errno 13]`)会让响应
|
||||
带出 `users/<user_id>/` 下的宿主机绝对路径;现在返回与智能体导入、导出一致的
|
||||
@ -10371,4 +10378,5 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写,核心包含子
|
||||
[#7170]: https://github.com/bytedance/deer-flow/pull/7170
|
||||
[#7205]: https://github.com/bytedance/deer-flow/pull/7205
|
||||
[#7207]: https://github.com/bytedance/deer-flow/pull/7207
|
||||
[#7217]: https://github.com/bytedance/deer-flow/pull/7217
|
||||
[#7255]: https://github.com/bytedance/deer-flow/pull/7255
|
||||
|
||||
@ -21,6 +21,10 @@ signed-in user, so their unexpected-failure 500s return a fixed detail. Load
|
||||
errors (e.g. a malformed `extensions_config.json`) carry host paths and stay in
|
||||
the server log.
|
||||
|
||||
Thread upload, upload delete and shelf attach-to-thread 500s name the file but
|
||||
not the cause: staging and unlink `OSError`s carry host paths and stay in the
|
||||
server log.
|
||||
|
||||
Skill mutation operation lists return `next_cursor: null` when `has_more` is false, including empty pages after a supplied `after_id`. The next cursor is the final returned operation only when a further page exists.
|
||||
|
||||
`GET /api/agent-tool-groups` projects only configured group names for the
|
||||
|
||||
@ -419,7 +419,7 @@ async def attach_project_document_to_thread(
|
||||
except Exception as exc:
|
||||
logger.error(f"Failed to attach {row['name']} to thread {thread_id}: {exc}")
|
||||
await service.cleanup_written()
|
||||
raise HTTPException(status_code=500, detail=f"Failed to upload {row['name']}: {exc}")
|
||||
raise HTTPException(status_code=500, detail=f"Failed to upload {row['name']}.")
|
||||
# Sync-phase failures (permissions/provider sync) follow ordinary
|
||||
# uploads: they propagate and the host files stay in place.
|
||||
await service.finalize()
|
||||
|
||||
@ -457,7 +457,7 @@ async def upload_files(
|
||||
except Exception as e:
|
||||
logger.error(f"Failed to upload {file.filename}: {e}")
|
||||
await service.cleanup_written()
|
||||
raise HTTPException(status_code=500, detail=f"Failed to upload {file.filename}: {str(e)}")
|
||||
raise HTTPException(status_code=500, detail=f"Failed to upload {file.filename}.")
|
||||
uploaded_files.append(file_info)
|
||||
|
||||
await service.finalize()
|
||||
@ -513,4 +513,4 @@ async def delete_uploaded_file(thread_id: ThreadId, filename: str, request: Requ
|
||||
raise HTTPException(status_code=400, detail=str(e))
|
||||
except Exception as e:
|
||||
logger.error(f"Failed to delete {filename}: {e}")
|
||||
raise HTTPException(status_code=500, detail=f"Failed to delete {filename}: {str(e)}")
|
||||
raise HTTPException(status_code=500, detail=f"Failed to delete {filename}.")
|
||||
|
||||
@ -1335,6 +1335,8 @@ Content-Type: multipart/form-data
|
||||
|
||||
```
|
||||
|
||||
**Error behavior:** an unexpected storage failure answers `500` with `{"detail": "Failed to upload <filename>."}`; the underlying error, which can name host paths, stays in server logs.
|
||||
|
||||
**Name collisions:** filenames are claimed unique against the thread's existing uploads and reserved atomically — a same-name upload never replaces the existing file; it lands as `document_1.pdf` (the response's `filename`/`original_filename` reflect the claimed name). Use the artifacts `PUT` endpoint for sanctioned in-place updates.
|
||||
|
||||
**Supported Document Formats** (converted to Markdown only when automatic conversion is enabled):
|
||||
@ -1381,6 +1383,8 @@ DELETE /api/threads/{thread_id}/uploads/{filename}
|
||||
}
|
||||
```
|
||||
|
||||
**Error behavior:** an unexpected storage failure answers `500` with `{"detail": "Failed to delete <filename>."}`; the underlying error stays in server logs.
|
||||
|
||||
### Thread Cleanup
|
||||
|
||||
Remove DeerFlow-managed local thread files under `.deer-flow/threads/{thread_id}` after the LangGraph thread itself has been deleted.
|
||||
@ -1540,7 +1544,7 @@ POST /api/projects/{project_id}/documents/{document_id}/attach-to-thread/{thread
|
||||
|
||||
Materializes an independent copy of a live shelf document into the target thread's uploads directory through the same ingestion pipeline as an ordinary upload (filename claiming, size checks, optional conversion under `uploads.auto_convert_documents`, sandbox-readable permissions, and sandbox sync for non-mounted providers; a caller denied `sandbox:execute` keeps the host upload without allocating a sandbox). Reading an archived source project's shelf is allowed and does not mutate it; a missing/foreign document, or a target thread the caller cannot write, is `404`. A row whose original bytes are missing or size-mismatched answers `409` `"content_missing"`.
|
||||
|
||||
**Response:** `200 OK` with `{"filename", "size_bytes", "virtual_path", "artifact_url"}` — returned only after ingestion succeeds.
|
||||
**Response:** `200 OK` with `{"filename", "size_bytes", "virtual_path", "artifact_url"}` — returned only after ingestion succeeds. An unexpected ingestion failure answers `500` with `{"detail": "Failed to upload <name>."}`; the underlying error stays in server logs.
|
||||
|
||||
#### Get Document Content
|
||||
|
||||
|
||||
@ -11,6 +11,7 @@ attach-vs-purge staged-copy serialization (§13 concurrency).
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import errno
|
||||
import os
|
||||
import stat
|
||||
import threading
|
||||
@ -550,6 +551,24 @@ class TestAttach:
|
||||
# scope released the partially acquired holder.
|
||||
release.assert_awaited_once()
|
||||
|
||||
def test_attach_keeps_os_error_text_out_of_500(self, tmp_path, caplog):
|
||||
"""A staging OSError names its host path, so it stays in the server log."""
|
||||
app = _build_app(tmp_path)
|
||||
with TestClient(app) as client, patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()):
|
||||
pid = _create_project(client)["id"]
|
||||
doc = _upload(client, pid, "report.txt", b"data").json()["document"]
|
||||
_seed_thread(app, "thread-1")
|
||||
staged_path = _thread_uploads("thread-1") / ".upload-abc123.part"
|
||||
disk_full = OSError(errno.ENOSPC, os.strerror(errno.ENOSPC), str(staged_path))
|
||||
|
||||
with patch.object(uploads, "_prepare_upload_destination", side_effect=disk_full):
|
||||
response = _attach(client, pid, doc["id"], "thread-1")
|
||||
|
||||
assert response.status_code == 500
|
||||
assert response.json() == {"detail": "Failed to upload report.txt."}
|
||||
assert str(staged_path) in caplog.text
|
||||
assert not (_thread_uploads("thread-1") / "report.txt").exists()
|
||||
|
||||
def test_attach_sync_failure_retains_host_file(self, tmp_path):
|
||||
app = _build_app(tmp_path)
|
||||
provider, sandbox = _remote_provider()
|
||||
|
||||
@ -493,7 +493,7 @@ def test_upload_files_removes_the_staged_part_when_the_platform_blocks_an_open_u
|
||||
|
||||
@pytest.mark.parametrize("filename", ["notes.txt", "report.pdf"])
|
||||
@pytest.mark.parametrize("simulate_sharing_violation", [False, True])
|
||||
def test_upload_files_preserves_link_error_and_cleans_staging_after_closing_conversion_fd(tmp_path, monkeypatch, filename, simulate_sharing_violation):
|
||||
def test_upload_files_preserves_link_error_and_cleans_staging_after_closing_conversion_fd(tmp_path, monkeypatch, caplog, filename, simulate_sharing_violation):
|
||||
thread_uploads_dir = tmp_path / "uploads"
|
||||
thread_uploads_dir.mkdir()
|
||||
conversion_fds: set[int] = set()
|
||||
@ -536,7 +536,9 @@ def test_upload_files_preserves_link_error_and_cleans_staging_after_closing_conv
|
||||
asyncio.run(call_unwrapped(uploads.upload_files, "thread-link-error", request=MagicMock(), files=[file], config=SimpleNamespace()))
|
||||
|
||||
assert exc_info.value.status_code == 500
|
||||
assert "original link failure" in exc_info.value.detail
|
||||
assert exc_info.value.detail == f"Failed to upload {filename}."
|
||||
# The logged cause is the link failure, not a later cleanup error.
|
||||
assert "original link failure" in caplog.text
|
||||
assert not blocked_unlinks
|
||||
assert not conversion_fds
|
||||
assert len(duplicated_fds) == (1 if filename == "report.pdf" else 0)
|
||||
@ -1559,6 +1561,27 @@ def test_upload_files_closes_conversion_descriptor_when_private_dir_fails(tmp_pa
|
||||
assert duplicated[0] in closed
|
||||
|
||||
|
||||
def test_upload_files_keeps_os_error_text_out_of_500(tmp_path, caplog):
|
||||
"""A staging OSError names its host path, so it stays in the server log."""
|
||||
thread_uploads_dir = tmp_path / "uploads"
|
||||
thread_uploads_dir.mkdir(parents=True)
|
||||
staged_path = thread_uploads_dir / ".upload-abc123.part"
|
||||
disk_full = OSError(errno.ENOSPC, os.strerror(errno.ENOSPC), str(staged_path))
|
||||
|
||||
with (
|
||||
patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir),
|
||||
patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()),
|
||||
patch.object(uploads, "_prepare_upload_destination", side_effect=disk_full),
|
||||
):
|
||||
file = ChunkedUpload("report.txt", [b"bytes"])
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
asyncio.run(call_unwrapped(uploads.upload_files, "thread-local", request=MagicMock(), files=[file], config=SimpleNamespace()))
|
||||
|
||||
assert exc_info.value.status_code == 500
|
||||
assert exc_info.value.detail == "Failed to upload report.txt."
|
||||
assert str(staged_path) in caplog.text
|
||||
|
||||
|
||||
def test_upload_files_closes_conversion_descriptor_when_ingestion_is_cancelled(tmp_path):
|
||||
"""Cancelling mid-duplication must not strand the descriptor the worker still produces."""
|
||||
thread_uploads_dir = tmp_path / "uploads"
|
||||
@ -1791,6 +1814,25 @@ def test_delete_uploaded_file_rejects_symlink_to_sibling_upload(tmp_path):
|
||||
assert (thread_uploads_dir / "alias.pdf").is_symlink()
|
||||
|
||||
|
||||
def test_delete_uploaded_file_keeps_os_error_text_out_of_500(tmp_path, caplog):
|
||||
"""An unlink OSError names its host path, so it stays in the server log."""
|
||||
thread_uploads_dir = tmp_path / "uploads"
|
||||
thread_uploads_dir.mkdir(parents=True)
|
||||
file_path = thread_uploads_dir / "report.pdf"
|
||||
denied = PermissionError(errno.EACCES, os.strerror(errno.EACCES), str(file_path))
|
||||
|
||||
with (
|
||||
patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir),
|
||||
patch.object(uploads, "delete_file_safe", side_effect=denied),
|
||||
):
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
asyncio.run(call_unwrapped(uploads.delete_uploaded_file, "thread-local", "report.pdf", request=MagicMock()))
|
||||
|
||||
assert exc_info.value.status_code == 500
|
||||
assert exc_info.value.detail == "Failed to delete report.pdf."
|
||||
assert str(file_path) in caplog.text
|
||||
|
||||
|
||||
def test_auto_convert_documents_enabled_defaults_to_false_on_config_errors():
|
||||
class BrokenConfig:
|
||||
def __getattribute__(self, name):
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user