From 9b0c7cbc37f7126bb8818e04bd8f9ad25681fb8b Mon Sep 17 00:00:00 2001 From: Hyeonsang Cho Date: Sun, 18 Oct 2026 11:52:22 +0900 Subject: [PATCH] fix(browser): never replace an existing output file with browser_screenshot (#6913) * fix(browser): never replace an existing output file with browser_screenshot browser_screenshot published straight onto outputs/ through the replacing atomic publisher, so a screenshot saved as report.png silently replaced the user's own report.png, an earlier capture, or another default-named capture taken in the same second (the default stem has second resolution). The sibling web_capture tool already keeps existing files and takes a -1, -2, ... suffix. Stage the complete capture under a hidden sibling and hard-link it onto the first free candidate name. The link is the whole no-overwrite guard: it fails with FileExistsError when the name exists as anything, so no name is checked before it is claimed and a concurrent writer cannot be replaced; the final name never shows a partial file. Staging shares the outputs directory, so the link is same-filesystem, and like Gateway uploads other link failures propagate instead of falling back to replacement. New files keep the publisher's normal creation mode. Hidden per-action progress frames keep the replacing _write_screenshot: their names carry microsecond timestamps in a server-owned directory. * docs(changelog): reference #6913 for the browser_screenshot no-overwrite fix * fix(browser): refuse a staged screenshot swapped before it is linked The sandbox can write outputs/, so it can see the hidden staged capture and replace it with a symlink before the hard link. os.link follows symlinks by default, which would publish a hard link to the host file as the screenshot artifact (the replacing publisher this PR moved away from never followed one). Stage the capture with an exclusively created descriptor and keep its identity from fstat, link without following symlinks where the platform supports it (decided once at import), and require the published name to be that inode; anything else is removed and the capture fails, like Gateway uploads' identity check. A failed staging write still leaves no files behind. * docs(readme): note that browser_screenshot never replaces an output file * fix(browser): keep the staged screenshot open until its link is verified Linux filesystems reuse a freed inode number at once. Closing the staged capture before linking let a file swapped in at the staged name take the same number and pass the samestat check, so the symlink and planted-file swap tests failed on Linux CI while passing on macOS. Hold the staged descriptor through the link and identity check, as Gateway uploads do, and require the published name to be a regular file. * test(browser): race the peer claim against the real publication step The claimed-name test patched _publish_bytes_atomically, which the screenshot path no longer calls, so its hook never fired and the race assertions passed vacuously. Plant the peer file right before os.link or os.replace lands bytes on a visible name, and assert that the claim happened, the capture moved to report-1.png, and the peer file survived. * refactor(capture): share one output-name candidate sequence browser_screenshot hand-copied web_capture's probe bound and suffix order, and the copies had already drifted: Browserless retried a held timestamped name five times, the screenshot tool once. Move the bound, the retry count and the candidate order into utils/output_names.py and have both tools iterate it, so the naming cannot drift again. Browserless behavior is unchanged; its tests now patch the shared module. --------- Co-authored-by: Willem Jiang --- CHANGELOG.md | 14 ++ CHANGELOG_zh.md | 10 + README.md | 3 + backend/packages/harness/deerflow/AGENTS.md | 3 +- .../community/browser_automation/tools.py | 69 ++++- .../deerflow/community/browserless/tools.py | 24 +- .../packages/harness/deerflow/utils/AGENTS.md | 2 + .../harness/deerflow/utils/output_names.py | 28 +++ .../test_browser_screenshot_no_overwrite.py | 237 ++++++++++++++++++ ...st_browserless_capture_name_reservation.py | 9 +- backend/tests/test_output_names.py | 78 ++++++ 11 files changed, 450 insertions(+), 27 deletions(-) create mode 100644 backend/packages/harness/deerflow/utils/output_names.py create mode 100644 backend/tests/test_browser_screenshot_no_overwrite.py create mode 100644 backend/tests/test_output_names.py diff --git a/CHANGELOG.md b/CHANGELOG.md index caa6786652..f259b8e9d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1247,6 +1247,19 @@ This release closes that milestone with **621 merged pull requests**. ### Fixed +- **browser:** `browser_screenshot` no longer replaces a file that already + exists in the thread's outputs directory. A screenshot saved as + `report.png` used to overwrite the user's own `report.png`, an earlier + capture, or another default-named capture taken in the same second; it now + takes the next free name (`report-1.png`, `report-2.png`, ...) like + `web_capture`, and the tool result reports the name actually used. The + complete capture is hard-linked onto the name, so a name taken concurrently + is skipped rather than replaced and the final name never shows a partial + file. Like Gateway uploads, the outputs directory needs hard-link support; + without it the tool reports an error instead of overwriting. A staged + capture swapped before publication, for example for a symlink to a host + file, is never linked into outputs; the tool reports an error. ([#6913]) + - **llm:** A provider error that arrives inside a streaming response that has already returned `200` is now retried like the same error sent as an HTTP status. An OpenAI-compatible gateway that answers `200` early, to keep slow @@ -11785,5 +11798,6 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#6876]: https://github.com/bytedance/deer-flow/pull/6876 [#6897]: https://github.com/bytedance/deer-flow/pull/6897 [#6909]: https://github.com/bytedance/deer-flow/pull/6909 +[#6913]: https://github.com/bytedance/deer-flow/pull/6913 [#6930]: https://github.com/bytedance/deer-flow/pull/6930 [#6991]: https://github.com/bytedance/deer-flow/pull/6991 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index f93cf82cc6..915559b778 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -1002,6 +1002,15 @@ ### 修复 +- **浏览器：** `browser_screenshot` 不再替换线程 outputs 目录中已存在的文件。此前以 + `report.png` 保存的截图会覆盖用户自己的 `report.png`、更早的截图，或同一秒内生成的 + 另一张默认命名截图；现在会像 `web_capture` 一样改用下一个空闲名称（`report-1.png`、 + `report-2.png`……），工具结果会报告实际使用的名称。完整截图通过硬链接发布到目标名称， + 因此并发占用的名称会被跳过而不是被替换，最终名称也不会暴露写了一半的文件。与 Gateway + 上传相同，outputs 目录需要支持硬链接；不支持时工具会报错，而不会覆盖已有文件。发布前 + 被替换的暂存截图（例如换成指向宿主机文件的符号链接）绝不会被链接到 outputs，工具会 + 报错。 ([#6913]) + - **llm：** 流式响应已返回 `200` 之后才在流内报告的 provider 错误，现在会像以 HTTP 状态码返回的同一错误一样重试。为了用 SSE keepalive 维持慢请求而提前返回 `200` 的 OpenAI 兼容网关，会把之后发生的 504 作为 SSE `error` 事件发出；openai SDK 将其抛出为 @@ -9396,5 +9405,6 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#6876]: https://github.com/bytedance/deer-flow/pull/6876 [#6897]: https://github.com/bytedance/deer-flow/pull/6897 [#6909]: https://github.com/bytedance/deer-flow/pull/6909 +[#6913]: https://github.com/bytedance/deer-flow/pull/6913 [#6930]: https://github.com/bytedance/deer-flow/pull/6930 [#6991]: https://github.com/bytedance/deer-flow/pull/6991 diff --git a/README.md b/README.md index 03b035c398..06661b2917 100644 --- a/README.md +++ b/README.md @@ -2707,6 +2707,9 @@ captures from overwriting one another. Failed publication releases the placehold if cleanup fails, Gateway logs a warning with its path and preserves the original publication error or interruption. +`browser_screenshot` never replaces an existing `outputs/` file: a taken name +moves to the next free one, such as `report-1.png`. + Browser dependency auto-detection accepts `name`, `group`, and `use` in any order within a tool entry, with either indented or indentless YAML lists. diff --git a/backend/packages/harness/deerflow/AGENTS.md b/backend/packages/harness/deerflow/AGENTS.md index 8390040b28..56968beebc 100644 --- a/backend/packages/harness/deerflow/AGENTS.md +++ b/backend/packages/harness/deerflow/AGENTS.md @@ -59,7 +59,8 @@ storage and transfer cost bounded relative to lossless PNG. The explicit `browser_screenshot` tool remains PNG because it creates a user-requested artifact. New automatic capture entry points must reuse the shared progress encoding definition in `tools.py` so the byte encoding and `.jpg` suffix cannot -drift. +drift. `browser_screenshot` never replaces an `outputs/` file: it hard-links +onto the first free `-N` name. ### Embedded Client (`packages/harness/deerflow/client.py`) diff --git a/backend/packages/harness/deerflow/community/browser_automation/tools.py b/backend/packages/harness/deerflow/community/browser_automation/tools.py index 034d257f9d..4116e4b2de 100644 --- a/backend/packages/harness/deerflow/community/browser_automation/tools.py +++ b/backend/packages/harness/deerflow/community/browser_automation/tools.py @@ -17,12 +17,16 @@ from __future__ import annotations import asyncio import contextlib +import errno import logging +import os import re +import stat from dataclasses import dataclass from datetime import UTC, datetime from pathlib import Path from typing import Annotated +from uuid import uuid4 from langchain.tools import InjectedToolCallId, tool from langchain_core.messages import ToolMessage @@ -35,6 +39,7 @@ from deerflow.config.paths import VIRTUAL_PATH_PREFIX from deerflow.constants import BROWSER_FRAMES_DIRNAME from deerflow.tools.types import Runtime from deerflow.utils.atomic_write import publish_bytes_atomically as _publish_bytes_atomically +from deerflow.utils.output_names import output_name_candidates from .session import BrowserSession, BrowserSessionManager, PageSnapshot, ScreenshotType, get_browser_session_manager @@ -48,6 +53,8 @@ _OUTPUTS_VIRTUAL_PREFIX = f"{VIRTUAL_PATH_PREFIX}/outputs" _BROWSER_FRAMES_DIRNAME = BROWSER_FRAMES_DIRNAME _FRAMES_VIRTUAL_PREFIX = f"{_OUTPUTS_VIRTUAL_PREFIX}/{_BROWSER_FRAMES_DIRNAME}" _SAFE_FILENAME_RE = re.compile(r"[^A-Za-z0-9._-]+") +# Without no-follow support, os.link may follow a symlink at the staged name. +_LINK_CAN_SKIP_SYMLINKS = os.link in os.supports_follow_symlinks @dataclass(frozen=True) @@ -440,6 +447,64 @@ def _write_screenshot(outputs_path: Path, name: str, content: bytes) -> str: return name +def _publish_screenshot_no_overwrite(outputs_path: Path, name: str, content: bytes) -> str: + """Publish *content* under *name*, or its first free ``-1``, ``-2``, ... variant. + + The outputs directory also holds the user's own deliverables, so a + screenshot never replaces an existing file. The complete capture is staged + under a hidden, exclusively created sibling (``0666 & ~umask``, like + ``Path.write_bytes``), then hard-linked onto each candidate name: the + ``os.link`` is the whole no-overwrite guard, failing with + :class:`FileExistsError` when the name exists as anything (a file, a + symlink, a concurrent capture), so no name is checked before it is claimed. + The final name never shows a partial capture. Staging shares the outputs + directory, so the link is same-filesystem; like Gateway uploads, outputs + need hard-link support and other link failures propagate. + + The sandbox can write ``outputs/`` too, so it could swap the staged name for + a symlink to a host file before the link. The link does not follow symlinks + where supported, and the published name must be the regular-file inode + written here; anything else is removed and the capture fails. The staged + descriptor stays open until that check: an unlinked but open inode cannot be + freed, so a swapped-in file can never reuse its number (Linux filesystems + reuse a freed inode number at once) and pass as the capture. + """ + outputs_path.mkdir(parents=True, exist_ok=True) + staged = outputs_path / f".{name}.{uuid4().hex}.tmp" + fd = os.open(staged, os.O_CREAT | os.O_EXCL | os.O_WRONLY | getattr(os, "O_BINARY", 0), 0o666) + try: + try: + handle = os.fdopen(fd, "wb") + except BaseException: + os.close(fd) + raise + with handle: + handle.write(content) + handle.flush() + os.fsync(handle.fileno()) + staged_stat = os.fstat(handle.fileno()) + for candidate in output_name_candidates(name): + destination = outputs_path / candidate + try: + if _LINK_CAN_SKIP_SYMLINKS: + os.link(staged, destination, follow_symlinks=False) + else: + os.link(staged, destination) + except FileExistsError: + continue + published = os.lstat(destination) + if not stat.S_ISREG(published.st_mode) or not os.path.samestat(published, staged_stat): + destination.unlink(missing_ok=True) + raise OSError(f"Staged browser screenshot changed before publication: {staged.name}") + return candidate + raise FileExistsError(errno.EEXIST, "No free screenshot name", str(outputs_path / name)) + finally: + try: + staged.unlink(missing_ok=True) + except OSError: + logger.warning("Failed to remove staged browser screenshot: %s", staged, exc_info=True) + + @tool("browser_screenshot", parse_docstring=True) async def browser_screenshot_tool( runtime: Runtime, @@ -454,7 +519,7 @@ async def browser_screenshot_tool( stateless page load. Args: - filename: Optional output filename (extension is forced to .png). + filename: Optional output filename (extension is forced to .png). An existing file is never replaced; the screenshot takes the next free name such as report-1.png. full_page: Capture the full scrollable page instead of just the viewport. """ try: @@ -464,7 +529,7 @@ async def browser_screenshot_tool( with _resolve_session(runtime, "browser_screenshot") as session: content = await session.screenshot_bytes(full_page=full_page) name = _safe_screenshot_name(filename) - final_name = await asyncio.to_thread(_write_screenshot, outputs_path, name, content) + final_name = await asyncio.to_thread(_publish_screenshot_no_overwrite, outputs_path, name, content) virtual_path = f"{_OUTPUTS_VIRTUAL_PREFIX}/{final_name}" return Command( update={ diff --git a/backend/packages/harness/deerflow/community/browserless/tools.py b/backend/packages/harness/deerflow/community/browserless/tools.py index 9881d80ccf..0937a858ef 100644 --- a/backend/packages/harness/deerflow/community/browserless/tools.py +++ b/backend/packages/harness/deerflow/community/browserless/tools.py @@ -17,6 +17,7 @@ from deerflow.config import get_app_config from deerflow.config.paths import VIRTUAL_PATH_PREFIX from deerflow.tools.types import Runtime from deerflow.utils.atomic_write import publish_bytes_atomically as _publish_bytes_atomically +from deerflow.utils.output_names import output_name_candidates from deerflow.utils.readability import ReadabilityExtractor from .browserless_client import BrowserlessClient, BrowserlessFetchResult, BrowserlessScreenshotResult @@ -32,11 +33,6 @@ _OUTPUT_FORMAT_TO_EXTENSION = { "webp": "webp", } _SAFE_FILENAME_RE = re.compile(r"[^A-Za-z0-9._-]+") -# Cap collision-suffix probing so a saturated outputs directory cannot spin forever. -_MAX_FILENAME_COLLISION_PROBES = 1000 -# Retry bound for the timestamped fallback: the microseconds stamp can only repeat -# when another writer claimed that exact name within the same microsecond. -_MAX_TIMESTAMP_CLAIM_ATTEMPTS = 5 DEFAULT_BASE_URL = "http://localhost:3032" _UNSET = object() @@ -257,21 +253,9 @@ def _dedupe_output_name(outputs_path: Path, output_name: str) -> str: means another writer is holding them; returning a name anyway would reintroduce the silent overwrite this function exists to prevent. """ - stem = Path(output_name).stem - suffix = Path(output_name).suffix - if _reserve_output_name(outputs_path, output_name): - return output_name - - for index in range(1, _MAX_FILENAME_COLLISION_PROBES + 1): - probe = f"{stem}-{index}{suffix}" - if _reserve_output_name(outputs_path, probe): - return probe - - for _ in range(_MAX_TIMESTAMP_CLAIM_ATTEMPTS): - timestamp = datetime.now(UTC).strftime("%Y%m%d-%H%M%S-%f") - fallback = f"{stem}-{timestamp}{suffix}" - if _reserve_output_name(outputs_path, fallback): - return fallback + for candidate in output_name_candidates(output_name): + if _reserve_output_name(outputs_path, candidate): + return candidate raise RuntimeError(f"could not reserve an output filename for {output_name!r} in {outputs_path}") diff --git a/backend/packages/harness/deerflow/utils/AGENTS.md b/backend/packages/harness/deerflow/utils/AGENTS.md index a67b21d725..88d007d757 100644 --- a/backend/packages/harness/deerflow/utils/AGENTS.md +++ b/backend/packages/harness/deerflow/utils/AGENTS.md @@ -6,6 +6,8 @@ capture/fetch publication. It writes and fsyncs a hidden exclusive sibling befor umask; replacements preserve existing permission bits before publication. Never read the process umask by changing it in this threaded runtime. Behavioral tests for both tool imports live in `tests/test_browser_outputs_atomic.py`. +Both tools take collision names from `output_names.output_name_candidates` +(name, `-1`..`-1000`, five timestamps); claim each atomically, never pre-check. ### Model Retry Hints diff --git a/backend/packages/harness/deerflow/utils/output_names.py b/backend/packages/harness/deerflow/utils/output_names.py new file mode 100644 index 0000000000..973c542a0f --- /dev/null +++ b/backend/packages/harness/deerflow/utils/output_names.py @@ -0,0 +1,28 @@ +"""Collision-free output file names shared by the capture tools.""" + +from __future__ import annotations + +from collections.abc import Iterator +from datetime import UTC, datetime +from pathlib import Path + +# Cap collision-suffix probing so a saturated outputs directory cannot spin forever. +MAX_COLLISION_PROBES = 1000 +# Retry bound for the timestamped fallback: the microseconds stamp can only repeat +# when another writer claimed that exact name within the same microsecond. +MAX_TIMESTAMP_ATTEMPTS = 5 + + +def output_name_candidates(name: str) -> Iterator[str]: + """Yield *name*, then ``stem-1`` .. ``stem-1000``, then timestamped fallbacks. + + Callers claim each candidate atomically and move on when it is taken; the + generator never checks the filesystem, so naming cannot race the claim. + Timestamps are taken lazily, one per fallback attempt. + """ + yield name + stem, suffix = Path(name).stem, Path(name).suffix + for index in range(1, MAX_COLLISION_PROBES + 1): + yield f"{stem}-{index}{suffix}" + for _ in range(MAX_TIMESTAMP_ATTEMPTS): + yield f"{stem}-{datetime.now(UTC).strftime('%Y%m%d-%H%M%S-%f')}{suffix}" diff --git a/backend/tests/test_browser_screenshot_no_overwrite.py b/backend/tests/test_browser_screenshot_no_overwrite.py new file mode 100644 index 0000000000..dec3f0fe90 --- /dev/null +++ b/backend/tests/test_browser_screenshot_no_overwrite.py @@ -0,0 +1,237 @@ +# SPDX-License-Identifier: MIT +"""``browser_screenshot`` must never replace a file already in the outputs directory.""" + +from __future__ import annotations + +import errno +import os +import stat +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from deerflow.community.browser_automation import tools + +_SHOT = b"\x89PNG\r\n\x1a\nscreenshot" + + +async def _screenshot(outputs: Path, filename: str | None = "report.png"): + session = MagicMock() + session.screenshot_bytes = AsyncMock(return_value=_SHOT) + manager = MagicMock() + manager.get_session.return_value = session + runtime = SimpleNamespace(context={"thread_id": "thread-1"}, state={"thread_data": {"outputs_path": str(outputs)}}) + with patch.object(tools, "get_browser_session_manager", return_value=manager), patch.object(tools, "_get_tool_config", return_value={}): + return await tools.browser_screenshot_tool.coroutine(runtime=runtime, tool_call_id="t1", filename=filename) + + +def _artifact(result) -> str: + return result.update["artifacts"][0] + + +@pytest.mark.asyncio +async def test_screenshot_keeps_an_existing_output_and_takes_the_next_name(tmp_path): + (tmp_path / "report.png").write_bytes(b"user report") + + result = await _screenshot(tmp_path) + + assert _artifact(result) == "/mnt/user-data/outputs/report-1.png" + assert "Saved browser screenshot: /mnt/user-data/outputs/report-1.png" in result.update["messages"][0].content + assert (tmp_path / "report.png").read_bytes() == b"user report" + assert (tmp_path / "report-1.png").read_bytes() == _SHOT + assert sorted(path.name for path in tmp_path.iterdir()) == ["report-1.png", "report.png"] + + +@pytest.mark.asyncio +async def test_repeated_screenshots_with_one_filename_keep_every_capture(tmp_path): + names = [_artifact(await _screenshot(tmp_path)).rsplit("/", 1)[1] for _ in range(3)] + + assert names == ["report.png", "report-1.png", "report-2.png"] + assert sorted(path.name for path in tmp_path.iterdir()) == sorted(names) + + +@pytest.mark.asyncio +async def test_a_name_claimed_after_the_choice_is_not_replaced(tmp_path, monkeypatch): + # A peer (another tool call, or the user) can create a name after the + # publisher chose it. Simulate that window at the publication step itself: + # right before a link or replace lands the capture on a visible name, a + # peer claims that name. Only a publication that refuses occupied names at + # that step keeps the peer file. + link, replace = os.link, os.replace + claimed: list[Path] = [] + + def peer_claims(destination) -> None: + destination = Path(destination) + if not claimed and not destination.name.startswith("."): + destination.write_bytes(b"peer output") + claimed.append(destination) + + def link_after_peer(source, destination, *args, **kwargs): + peer_claims(destination) + return link(source, destination, *args, **kwargs) + + def replace_after_peer(source, destination, *args, **kwargs): + peer_claims(destination) + return replace(source, destination, *args, **kwargs) + + monkeypatch.setattr(os, "link", link_after_peer) + monkeypatch.setattr(os, "replace", replace_after_peer) + result = await _screenshot(tmp_path) + + assert claimed == [tmp_path / "report.png"] + assert _artifact(result) == "/mnt/user-data/outputs/report-1.png" + assert (tmp_path / "report.png").read_bytes() == b"peer output" + assert (tmp_path / "report-1.png").read_bytes() == _SHOT + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlink creation") +@pytest.mark.asyncio +async def test_screenshot_does_not_replace_a_symlink_at_the_name(tmp_path): + outputs = tmp_path / "outputs" + outputs.mkdir() + (outputs / "report.png").symlink_to(tmp_path / "missing-target.png") + + result = await _screenshot(outputs) + + assert _artifact(result) == "/mnt/user-data/outputs/report-1.png" + assert (outputs / "report.png").is_symlink() + assert not (tmp_path / "missing-target.png").exists() + + +@pytest.mark.asyncio +async def test_failed_publication_reports_an_error_and_leaves_no_files(tmp_path, monkeypatch): + (tmp_path / "report.png").write_bytes(b"user report") + + def no_hard_links(*args, **kwargs): + raise OSError(errno.EPERM, "hard links unsupported") + + monkeypatch.setattr(os, "link", no_hard_links) + result = await _screenshot(tmp_path) + + assert "artifacts" not in result.update + assert "Error: could not capture screenshot" in result.update["messages"][0].content + assert sorted(path.name for path in tmp_path.iterdir()) == ["report.png"] + assert (tmp_path / "report.png").read_bytes() == b"user report" + + +def _swap_staged_before_link(monkeypatch, replace_staged, watch: Path | None = None) -> list[int]: + """Replace the staged capture right before it is linked, as a sandbox writing ``outputs/`` could. + + Returns the link counts of *watch* observed right after each link call. + """ + link = os.link + link_counts: list[int] = [] + + def swapped_link(source, destination, *args, **kwargs): + source = Path(source) + if source.name.startswith(".") and source.is_file() and not source.is_symlink(): + source.unlink() + replace_staged(source) + try: + return link(source, destination, *args, **kwargs) + finally: + if watch is not None: + link_counts.append(watch.stat().st_nlink) + + monkeypatch.setattr(os, "link", swapped_link) + return link_counts + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlink creation") +@pytest.mark.asyncio +async def test_a_staged_capture_swapped_for_a_symlink_never_publishes_its_target(tmp_path, monkeypatch): + outputs = tmp_path / "outputs" + outputs.mkdir() + secret = tmp_path / "host-secret.txt" + secret.write_bytes(b"host secret") + link_counts = _swap_staged_before_link(monkeypatch, lambda staged: staged.symlink_to(secret), watch=secret) + + result = await _screenshot(outputs) + + assert "artifacts" not in result.update + assert "Error: could not capture screenshot" in result.update["messages"][0].content + assert list(outputs.iterdir()) == [] + # The host file never gains a name in outputs/, not even briefly. + assert link_counts and set(link_counts) == {1} + + +@pytest.mark.asyncio +async def test_a_staged_capture_swapped_for_another_file_is_not_published(tmp_path, monkeypatch): + _swap_staged_before_link(monkeypatch, lambda staged: staged.write_bytes(b"planted")) + + result = await _screenshot(tmp_path) + + assert "artifacts" not in result.update + assert "Error: could not capture screenshot" in result.update["messages"][0].content + assert list(tmp_path.iterdir()) == [] + + +def _open_descriptors_for(target: os.stat_result) -> list[int]: + descriptors = [] + for entry in os.listdir("/dev/fd"): + try: + if os.path.samestat(os.fstat(int(entry)), target): + descriptors.append(int(entry)) + except OSError: + continue + return descriptors + + +@pytest.mark.skipif(not os.path.isdir("/dev/fd"), reason="needs /dev/fd to list open descriptors") +@pytest.mark.asyncio +async def test_the_staged_capture_stays_open_while_it_is_linked(tmp_path, monkeypatch): + # Linux filesystems hand a freed inode number to the next file at once, so + # a capture whose descriptor is closed before the link lets a swapped-in + # file reuse its number and pass the identity check. macOS rarely reuses + # numbers, so the swap tests above only catch that there; pin the guard + # itself: an open descriptor keeps the staged inode allocated. + link = os.link + open_at_link: list[list[int]] = [] + + def observing_link(source, destination, *args, **kwargs): + open_at_link.append(_open_descriptors_for(os.lstat(source))) + return link(source, destination, *args, **kwargs) + + monkeypatch.setattr(os, "link", observing_link) + result = await _screenshot(tmp_path) + + assert _artifact(result) == "/mnt/user-data/outputs/report.png" + assert open_at_link and all(open_at_link) + + +@pytest.mark.asyncio +async def test_a_failed_staging_write_leaves_no_files(tmp_path, monkeypatch): + def fail(*args): + raise OSError(errno.ENOSPC, "disk full") + + monkeypatch.setattr(os, "fsync", fail) + result = await _screenshot(tmp_path) + + assert "Error: could not capture screenshot" in result.update["messages"][0].content + assert list(tmp_path.iterdir()) == [] + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission bits and umask") +@pytest.mark.parametrize("mask", [0o022, 0o077]) +@pytest.mark.asyncio +async def test_screenshot_uses_normal_creation_permissions(tmp_path, mask): + previous_mask = os.umask(mask) + try: + await _screenshot(tmp_path) + finally: + os.umask(previous_mask) + + assert stat.S_IMODE((tmp_path / "report.png").stat().st_mode) == 0o666 & ~mask + + +@pytest.mark.asyncio +async def test_default_names_from_the_same_second_do_not_collide(tmp_path, monkeypatch): + monkeypatch.setattr(tools, "_safe_screenshot_name", lambda filename: "browser-capture-20261015-120000.png") + + first = _artifact(await _screenshot(tmp_path, filename=None)) + second = _artifact(await _screenshot(tmp_path, filename=None)) + + assert first.endswith("/browser-capture-20261015-120000.png") + assert second.endswith("/browser-capture-20261015-120000-1.png") diff --git a/backend/tests/test_browserless_capture_name_reservation.py b/backend/tests/test_browserless_capture_name_reservation.py index 7c73583484..1afd54647d 100644 --- a/backend/tests/test_browserless_capture_name_reservation.py +++ b/backend/tests/test_browserless_capture_name_reservation.py @@ -22,6 +22,7 @@ from datetime import UTC, datetime import pytest tools = importlib.import_module("deerflow.community.browserless.tools") +output_names = importlib.import_module("deerflow.utils.output_names") @pytest.fixture() @@ -152,7 +153,7 @@ def test_interruption_propagates_and_frees_the_name(outputs, monkeypatch): def test_probe_bound_is_inclusive(outputs, monkeypatch): """The last probe index is reachable, so a directory full of ``-1``/``-2`` still gets ``-3``.""" - monkeypatch.setattr(tools, "_MAX_FILENAME_COLLISION_PROBES", 3) + monkeypatch.setattr(output_names, "MAX_COLLISION_PROBES", 3) for name in ("report.png", "report-1.png", "report-2.png"): (outputs / name).write_bytes(b"earlier") @@ -160,7 +161,7 @@ def test_probe_bound_is_inclusive(outputs, monkeypatch): def test_saturated_probe_range_still_reserves_a_fresh_name(outputs, monkeypatch): - monkeypatch.setattr(tools, "_MAX_FILENAME_COLLISION_PROBES", 2) + monkeypatch.setattr(output_names, "MAX_COLLISION_PROBES", 2) taken = ["report.png", "report-1.png", "report-2.png", "report-3.png"] for name in taken: (outputs / name).write_bytes(b"earlier") @@ -185,8 +186,8 @@ def test_exhausted_timestamp_claims_refuse_to_overwrite(outputs, monkeypatch): def now(cls, tz=None): return datetime(2026, 10, 14, 12, 0, 0, 123456, tzinfo=UTC) - monkeypatch.setattr(tools, "_MAX_FILENAME_COLLISION_PROBES", 1) - monkeypatch.setattr(tools, "datetime", _FrozenClock) + monkeypatch.setattr(output_names, "MAX_COLLISION_PROBES", 1) + monkeypatch.setattr(output_names, "datetime", _FrozenClock) held = ["report.png", "report-1.png", "report-20261014-120000-123456.png"] for name in held: (outputs / name).write_bytes(b"earlier") diff --git a/backend/tests/test_output_names.py b/backend/tests/test_output_names.py new file mode 100644 index 0000000000..67e34e78d5 --- /dev/null +++ b/backend/tests/test_output_names.py @@ -0,0 +1,78 @@ +# SPDX-License-Identifier: MIT +"""Both capture tools take output names from one shared candidate sequence.""" + +from __future__ import annotations + +import importlib +from datetime import UTC, datetime +from itertools import islice +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from deerflow.utils import output_names + +browser_automation_tools = importlib.import_module("deerflow.community.browser_automation.tools") +browserless_tools = importlib.import_module("deerflow.community.browserless.tools") + + +class _StepClock(datetime): + """Each call advances one microsecond, so every fallback stamp is distinct.""" + + ticks = 0 + + @classmethod + def now(cls, tz=None): + cls.ticks += 1 + return datetime(2026, 10, 18, 12, 0, 0, cls.ticks, tzinfo=UTC) + + +def test_candidates_keep_the_name_then_number_then_timestamp(monkeypatch): + monkeypatch.setattr(output_names, "MAX_COLLISION_PROBES", 2) + monkeypatch.setattr(_StepClock, "ticks", 0) + monkeypatch.setattr(output_names, "datetime", _StepClock) + + assert list(output_names.output_name_candidates("report.png")) == [ + "report.png", + "report-1.png", + "report-2.png", + *(f"report-20261018-120000-{tick:06d}.png" for tick in range(1, output_names.MAX_TIMESTAMP_ATTEMPTS + 1)), + ] + + +def test_candidates_read_the_clock_only_when_a_fallback_is_reached(monkeypatch): + monkeypatch.setattr(_StepClock, "ticks", 0) + monkeypatch.setattr(output_names, "datetime", _StepClock) + + assert len(list(islice(output_names.output_name_candidates("a.png"), output_names.MAX_COLLISION_PROBES + 1))) == output_names.MAX_COLLISION_PROBES + 1 + assert _StepClock.ticks == 0 + + +def test_both_capture_tools_share_the_candidate_sequence(): + for module in (browser_automation_tools, browserless_tools): + assert module.output_name_candidates is output_names.output_name_candidates + + +@pytest.mark.asyncio +async def test_screenshot_retries_a_held_timestamp_fallback(tmp_path, monkeypatch): + # Browserless already retried a held timestamped name; the screenshot tool + # now shares that bound instead of giving up after one stamp. + monkeypatch.setattr(output_names, "MAX_COLLISION_PROBES", 1) + monkeypatch.setattr(_StepClock, "ticks", 0) + monkeypatch.setattr(output_names, "datetime", _StepClock) + held = ["report.png", "report-1.png", "report-20261018-120000-000001.png"] + for name in held: + (tmp_path / name).write_bytes(b"earlier") + + session = MagicMock() + session.screenshot_bytes = AsyncMock(return_value=b"shot") + manager = MagicMock() + manager.get_session.return_value = session + runtime = SimpleNamespace(context={"thread_id": "thread-1"}, state={"thread_data": {"outputs_path": str(tmp_path)}}) + with patch.object(browser_automation_tools, "get_browser_session_manager", return_value=manager), patch.object(browser_automation_tools, "_get_tool_config", return_value={}): + result = await browser_automation_tools.browser_screenshot_tool.coroutine(runtime=runtime, tool_call_id="t1", filename="report.png") + + assert result.update["artifacts"] == ["/mnt/user-data/outputs/report-20261018-120000-000002.png"] + assert (tmp_path / "report-20261018-120000-000002.png").read_bytes() == b"shot" + assert all((tmp_path / name).read_bytes() == b"earlier" for name in held)