mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-22 22:18:15 +00:00
* perf(sandbox): cache LocalSandbox path-rewrite regexes per instance LocalSandbox re-sorted path_mappings and re-compiled its path-rewrite regex on every tool call: _resolve_paths_in_command (every bash), _resolve_paths_in_content (every write_file), and _reverse_resolve_paths_in_output (every bash output / read_file). The inputs are derived solely from self.path_mappings, which is assigned once in __init__ and never mutated, so the work is identical every call. Compile the patterns once per sandbox via functools.cached_property and reuse them; hoist 'import re' to module scope. Behavior is unchanged — only the per-call sort+escape+compile on the agent's hot path is removed. Fixes #3647. Adds tests covering caching identity, unchanged rewriting, path-segment boundary matching, and the empty-mappings pass-through. * perf(sandbox): also cache resolved local paths and sorted mapping views Beyond the regex compilation, _find_path_mapping, _is_read_only_path, _resolve_path_with_mapping and _reverse_resolve_path re-sorted path_mappings and re-ran Path(local_path).resolve() (a filesystem syscall) on every call. Since path_mappings is immutable, cache the resolved local root per mapping and the two sorted views via cached_property and reuse them. Behavior is unchanged; the reverse-output pattern builder now reuses the same resolved-path cache.
This commit is contained in:
parent
654f5e1c66
commit
21d9ec0db1
@ -2,9 +2,11 @@ import errno
|
||||
import logging
|
||||
import ntpath
|
||||
import os
|
||||
import re
|
||||
import shutil
|
||||
import subprocess
|
||||
from dataclasses import dataclass
|
||||
from functools import cached_property
|
||||
from pathlib import Path
|
||||
from typing import NamedTuple
|
||||
|
||||
@ -83,6 +85,52 @@ class LocalSandbox(Sandbox):
|
||||
# reverse-resolves paths in agent-authored content.
|
||||
self._agent_written_paths: set[str] = set()
|
||||
|
||||
# ``path_mappings`` is set once in ``__init__`` and never mutated, so the
|
||||
# sorted views and compiled path-rewrite patterns below are stable for the
|
||||
# sandbox's lifetime. Caching them avoids re-sorting and re-compiling these
|
||||
# regexes on every bash/read_file/write_file call (the agent's hot path).
|
||||
|
||||
@cached_property
|
||||
def _command_pattern(self) -> re.Pattern[str] | None:
|
||||
"""Compiled matcher for container paths in shell commands (shell-aware boundaries)."""
|
||||
mappings = sorted(self.path_mappings, key=lambda m: len(m.container_path), reverse=True)
|
||||
if not mappings:
|
||||
return None
|
||||
# The lookahead (?=/|$|...) ensures we only match at a path-segment boundary,
|
||||
# preventing /mnt/skills from matching inside /mnt/skills-extra.
|
||||
patterns = [re.escape(m.container_path) + r"(?=/|$|[\s\"';&|<>()])(?:/[^\s\"';&|<>()]*)?" for m in mappings]
|
||||
return re.compile("|".join(f"({p})" for p in patterns))
|
||||
|
||||
@cached_property
|
||||
def _content_pattern(self) -> re.Pattern[str] | None:
|
||||
"""Compiled matcher for container paths in plain file content (text boundaries)."""
|
||||
mappings = sorted(self.path_mappings, key=lambda m: len(m.container_path), reverse=True)
|
||||
if not mappings:
|
||||
return None
|
||||
patterns = [re.escape(m.container_path) + r"(?=/|$|[^\w./-])(?:/[^\s\"';&|<>()]*)?" for m in mappings]
|
||||
return re.compile("|".join(f"({p})" for p in patterns))
|
||||
|
||||
@cached_property
|
||||
def _reverse_output_patterns(self) -> list[re.Pattern[str]]:
|
||||
"""Compiled matchers for local paths in command output (longest local path first)."""
|
||||
return [re.compile(re.escape(self._resolved_local_paths[m]) + r"(?:[/\\][^\s\"';&|<>()]*)?") for m in self._mappings_by_local_specificity]
|
||||
|
||||
@cached_property
|
||||
def _resolved_local_paths(self) -> dict[PathMapping, str]:
|
||||
"""Filesystem-resolved local root per mapping. ``Path.resolve()`` hits the
|
||||
disk, and the mounted directories don't move, so resolve once and reuse."""
|
||||
return {m: str(Path(m.local_path).resolve()) for m in self.path_mappings}
|
||||
|
||||
@cached_property
|
||||
def _mappings_by_container_specificity(self) -> list[PathMapping]:
|
||||
"""Mappings ordered most-specific-container-first (for forward resolution)."""
|
||||
return sorted(self.path_mappings, key=lambda m: len(m.container_path.rstrip("/") or "/"), reverse=True)
|
||||
|
||||
@cached_property
|
||||
def _mappings_by_local_specificity(self) -> list[PathMapping]:
|
||||
"""Mappings ordered longest-local-path-first (for reverse resolution)."""
|
||||
return sorted(self.path_mappings, key=lambda m: len(m.local_path), reverse=True)
|
||||
|
||||
def _is_read_only_path(self, resolved_path: str) -> bool:
|
||||
"""Check if a resolved path is under a read-only mount.
|
||||
|
||||
@ -96,7 +144,7 @@ class LocalSandbox(Sandbox):
|
||||
best_prefix_len = -1
|
||||
|
||||
for mapping in self.path_mappings:
|
||||
local_resolved = str(Path(mapping.local_path).resolve())
|
||||
local_resolved = self._resolved_local_paths[mapping]
|
||||
if resolved == local_resolved or resolved.startswith(local_resolved + os.sep):
|
||||
prefix_len = len(local_resolved)
|
||||
if prefix_len > best_prefix_len:
|
||||
@ -111,7 +159,7 @@ class LocalSandbox(Sandbox):
|
||||
def _find_path_mapping(self, path: str) -> tuple[PathMapping, str] | None:
|
||||
path_str = str(path)
|
||||
|
||||
for mapping in sorted(self.path_mappings, key=lambda m: len(m.container_path.rstrip("/") or "/"), reverse=True):
|
||||
for mapping in self._mappings_by_container_specificity:
|
||||
container_path = mapping.container_path.rstrip("/") or "/"
|
||||
if container_path == "/":
|
||||
if path_str.startswith("/"):
|
||||
@ -141,7 +189,7 @@ class LocalSandbox(Sandbox):
|
||||
return ResolvedPath(path_str, None)
|
||||
|
||||
mapping, relative = mapping_match
|
||||
local_root = Path(mapping.local_path).resolve()
|
||||
local_root = Path(self._resolved_local_paths[mapping])
|
||||
resolved_path = (local_root / relative).resolve() if relative else local_root
|
||||
|
||||
try:
|
||||
@ -171,8 +219,8 @@ class LocalSandbox(Sandbox):
|
||||
path_str = str(Path(normalized_path).resolve())
|
||||
|
||||
# Try each mapping (longest local path first for more specific matches)
|
||||
for mapping in sorted(self.path_mappings, key=lambda m: len(m.local_path), reverse=True):
|
||||
local_path_resolved = str(Path(mapping.local_path).resolve())
|
||||
for mapping in self._mappings_by_local_specificity:
|
||||
local_path_resolved = self._resolved_local_paths[mapping]
|
||||
if path_str == local_path_resolved or path_str.startswith(local_path_resolved + "/"):
|
||||
# Replace the local path prefix with container path
|
||||
relative = path_str[len(local_path_resolved) :].lstrip("/")
|
||||
@ -192,22 +240,10 @@ class LocalSandbox(Sandbox):
|
||||
Returns:
|
||||
Output with local paths resolved to container paths
|
||||
"""
|
||||
import re
|
||||
|
||||
# Sort mappings by local path length (longest first) for correct prefix matching
|
||||
sorted_mappings = sorted(self.path_mappings, key=lambda m: len(m.local_path), reverse=True)
|
||||
|
||||
if not sorted_mappings:
|
||||
return output
|
||||
|
||||
# Create pattern that matches absolute paths
|
||||
# Match paths like /Users/... or other absolute paths
|
||||
# Patterns are compiled once per sandbox (longest local path first for
|
||||
# correct prefix matching) and reused across calls.
|
||||
result = output
|
||||
for mapping in sorted_mappings:
|
||||
# Escape the local path for use in regex
|
||||
escaped_local = re.escape(str(Path(mapping.local_path).resolve()))
|
||||
# Match the local path followed by optional path components with either separator
|
||||
pattern = re.compile(escaped_local + r"(?:[/\\][^\s\"';&|<>()]*)?")
|
||||
for pattern in self._reverse_output_patterns:
|
||||
|
||||
def replace_match(match: re.Match) -> str:
|
||||
matched_path = match.group(0)
|
||||
@ -227,22 +263,10 @@ class LocalSandbox(Sandbox):
|
||||
Returns:
|
||||
Command with container paths resolved to local paths
|
||||
"""
|
||||
import re
|
||||
|
||||
# Sort mappings by length (longest first) for correct prefix matching
|
||||
sorted_mappings = sorted(self.path_mappings, key=lambda m: len(m.container_path), reverse=True)
|
||||
|
||||
# Build regex pattern to match all container paths
|
||||
# Match container path followed by optional path components
|
||||
if not sorted_mappings:
|
||||
pattern = self._command_pattern
|
||||
if pattern is None:
|
||||
return command
|
||||
|
||||
# Create pattern that matches any of the container paths.
|
||||
# The lookahead (?=/|$|...) ensures we only match at a path-segment boundary,
|
||||
# preventing /mnt/skills from matching inside /mnt/skills-extra.
|
||||
patterns = [re.escape(m.container_path) + r"(?=/|$|[\s\"';&|<>()])(?:/[^\s\"';&|<>()]*)?" for m in sorted_mappings]
|
||||
pattern = re.compile("|".join(f"({p})" for p in patterns))
|
||||
|
||||
def replace_match(match: re.Match) -> str:
|
||||
matched_path = match.group(0)
|
||||
return self._resolve_path(matched_path)
|
||||
@ -264,15 +288,10 @@ class LocalSandbox(Sandbox):
|
||||
Returns:
|
||||
Content with container paths resolved to local paths (forward slashes).
|
||||
"""
|
||||
import re
|
||||
|
||||
sorted_mappings = sorted(self.path_mappings, key=lambda m: len(m.container_path), reverse=True)
|
||||
if not sorted_mappings:
|
||||
pattern = self._content_pattern
|
||||
if pattern is None:
|
||||
return content
|
||||
|
||||
patterns = [re.escape(m.container_path) + r"(?=/|$|[^\w./-])(?:/[^\s\"';&|<>()]*)?" for m in sorted_mappings]
|
||||
pattern = re.compile("|".join(f"({p})" for p in patterns))
|
||||
|
||||
def replace_match(match: re.Match) -> str:
|
||||
matched_path = match.group(0)
|
||||
resolved = self._resolve_path(matched_path)
|
||||
|
||||
99
backend/tests/test_local_sandbox_path_regex_cache.py
Normal file
99
backend/tests/test_local_sandbox_path_regex_cache.py
Normal file
@ -0,0 +1,99 @@
|
||||
"""Issue #3647 — LocalSandbox must compile its path-rewrite regexes once per
|
||||
sandbox (cached), not on every bash/read_file/write_file call, while keeping
|
||||
the exact same rewriting behavior.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from pathlib import Path
|
||||
|
||||
from deerflow.sandbox.local.local_sandbox import LocalSandbox, PathMapping
|
||||
|
||||
|
||||
def _make_sandbox(tmp_path: Path) -> LocalSandbox:
|
||||
ws = tmp_path / "workspace"
|
||||
skills = tmp_path / "skills"
|
||||
ws.mkdir()
|
||||
skills.mkdir()
|
||||
return LocalSandbox(
|
||||
id="test",
|
||||
path_mappings=[
|
||||
PathMapping(container_path="/mnt/user-data/workspace", local_path=str(ws)),
|
||||
PathMapping(container_path="/mnt/skills", local_path=str(skills), read_only=True),
|
||||
],
|
||||
)
|
||||
|
||||
|
||||
def test_patterns_are_compiled_once_and_cached(tmp_path):
|
||||
sb = _make_sandbox(tmp_path)
|
||||
# Each cached_property returns the identical object across accesses.
|
||||
assert sb._command_pattern is sb._command_pattern
|
||||
assert sb._content_pattern is sb._content_pattern
|
||||
assert sb._reverse_output_patterns is sb._reverse_output_patterns
|
||||
# Two mappings -> two reverse-output patterns.
|
||||
assert len(sb._reverse_output_patterns) == 2
|
||||
|
||||
|
||||
def test_empty_mappings_yield_no_pattern(tmp_path):
|
||||
sb = LocalSandbox(id="empty", path_mappings=[])
|
||||
assert sb._command_pattern is None
|
||||
assert sb._content_pattern is None
|
||||
assert sb._reverse_output_patterns == []
|
||||
# No mappings -> command/content pass through unchanged.
|
||||
assert sb._resolve_paths_in_command("echo hello") == "echo hello"
|
||||
assert sb._resolve_paths_in_content("plain text") == "plain text"
|
||||
|
||||
|
||||
def test_command_paths_resolved_to_local(tmp_path):
|
||||
sb = _make_sandbox(tmp_path)
|
||||
ws_local = str((tmp_path / "workspace").resolve())
|
||||
out = sb._resolve_paths_in_command("cat /mnt/user-data/workspace/foo.txt")
|
||||
assert out == f"cat {ws_local}/foo.txt"
|
||||
# Calling again uses the cached pattern and produces the same result.
|
||||
assert sb._resolve_paths_in_command("cat /mnt/user-data/workspace/foo.txt") == out
|
||||
|
||||
|
||||
def test_segment_boundary_not_matched_inside_longer_name(tmp_path):
|
||||
sb = _make_sandbox(tmp_path)
|
||||
# "/mnt/skills-extra" must NOT be rewritten by the "/mnt/skills" mapping.
|
||||
out = sb._resolve_paths_in_command("ls /mnt/skills-extra/data")
|
||||
assert out == "ls /mnt/skills-extra/data"
|
||||
|
||||
|
||||
def test_reverse_resolve_output_maps_local_back_to_container(tmp_path):
|
||||
sb = _make_sandbox(tmp_path)
|
||||
ws_local = str((tmp_path / "workspace").resolve())
|
||||
out = sb._reverse_resolve_paths_in_output(f"wrote {ws_local}/foo.txt ok")
|
||||
assert out == "wrote /mnt/user-data/workspace/foo.txt ok"
|
||||
|
||||
|
||||
def test_resolved_paths_and_sorted_views_are_cached(tmp_path):
|
||||
sb = _make_sandbox(tmp_path)
|
||||
# Resolved-local map and sorted views are computed once and reused.
|
||||
assert sb._resolved_local_paths is sb._resolved_local_paths
|
||||
assert sb._mappings_by_container_specificity is sb._mappings_by_container_specificity
|
||||
assert sb._mappings_by_local_specificity is sb._mappings_by_local_specificity
|
||||
# Map covers every mapping with its filesystem-resolved local root.
|
||||
assert set(sb._resolved_local_paths.values()) == {
|
||||
str((tmp_path / "workspace").resolve()),
|
||||
str((tmp_path / "skills").resolve()),
|
||||
}
|
||||
# Most-specific (longest) container path is ordered first.
|
||||
assert sb._mappings_by_container_specificity[0].container_path == "/mnt/user-data/workspace"
|
||||
|
||||
|
||||
def test_forward_resolution_behavior_unchanged(tmp_path):
|
||||
sb = _make_sandbox(tmp_path)
|
||||
ws_local = str((tmp_path / "workspace").resolve())
|
||||
# Container path resolves to the mapped local path.
|
||||
assert sb._resolve_path("/mnt/user-data/workspace/sub/foo.txt") == f"{ws_local}/sub/foo.txt"
|
||||
# An unmapped path is returned unchanged.
|
||||
assert sb._resolve_path("/etc/hosts") == "/etc/hosts"
|
||||
|
||||
|
||||
def test_read_only_mount_detected(tmp_path):
|
||||
sb = _make_sandbox(tmp_path)
|
||||
skills_local = str((tmp_path / "skills").resolve())
|
||||
ws_local = str((tmp_path / "workspace").resolve())
|
||||
assert sb._is_read_only_path(f"{skills_local}/a.md") is True
|
||||
assert sb._is_read_only_path(f"{ws_local}/a.txt") is False
|
||||
Loading…
x
Reference in New Issue
Block a user