fix(tools): staleness + tracking-parity fixes for the not-found cache
Review follow-ups on the #25387 salvage: 1. CRITICAL: a cached miss survived out-of-band file creation (terminal command, external process) for the full 60s TTL — breaking the common agent pattern 'check for file -> create it -> read it' (live-repro'd). Serve-side existence guard: one ~free stat before serving a cached miss; if the path now exists the entry is evicted and the real read runs. Also fixes the search-root variant (write under a cached-missing directory). Both mutation-checked. 2. notify_other_tool_call now clears the task's not_found entries too (belt: the dispatcher calls it for every non-read tool). 3. Tracking parity: the record sites no longer early-return. On upstream, error results flow through consecutive-loop detection and dedup bookkeeping; short-circuiting skipped that and broke TestDedupInvalidationTaskResolution when preceded by TestSilentFileMisplacementE2E (bisected: the early return at the read record site was the trigger). Recording is now side-effect-identical to upstream; serving from the cache remains the optimization. Also reuse the already-computed _resolved instead of resolving a second time.
This commit is contained in:
@@ -920,3 +920,77 @@ class TestNotFoundCache:
|
||||
assert _check_not_found_cache("read", "/tmp/ttl-test", tid) is None
|
||||
with ft._read_tracker_lock:
|
||||
assert ("read", "/tmp/ttl-test") not in _read_tracker[tid].get("not_found", {})
|
||||
|
||||
def test_out_of_band_creation_defeats_cached_miss(self, tmp_path):
|
||||
"""CRITICAL staleness contract: a file created AFTER a cached miss —
|
||||
by a terminal command or any external process, NOT write_file_tool —
|
||||
must be served for real on the next read. The agent pattern
|
||||
'check for file → create it → read it' breaks otherwise."""
|
||||
from tools.file_tools import (
|
||||
_check_not_found_cache,
|
||||
_record_not_found,
|
||||
_read_tracker,
|
||||
)
|
||||
|
||||
tid = "neg-cache-oob-read"
|
||||
_read_tracker.pop(tid, None)
|
||||
target = tmp_path / "created-later.txt"
|
||||
|
||||
_record_not_found("read", str(target), tid, '{"error":"File not found: x"}')
|
||||
assert _check_not_found_cache("read", str(target), tid) is not None
|
||||
|
||||
# Out-of-band creation: plain filesystem write, no tool hook fires.
|
||||
target.write_text("real content\n")
|
||||
|
||||
# The cached miss must NOT be served once the path exists…
|
||||
assert _check_not_found_cache("read", str(target), tid) is None, (
|
||||
"stale 'File not found' served after the file was created "
|
||||
"out-of-band — the existence guard regressed"
|
||||
)
|
||||
# …and the entry is evicted, not just skipped.
|
||||
with __import__("tools.file_tools", fromlist=["x"])._read_tracker_lock:
|
||||
assert ("read", str(target)) not in _read_tracker[tid].get("not_found", {})
|
||||
|
||||
def test_out_of_band_creation_defeats_cached_search_miss(self, tmp_path):
|
||||
"""Same contract for search roots: creating a file under a
|
||||
previously-missing directory must defeat the cached 'Path not found'."""
|
||||
from tools.file_tools import (
|
||||
_check_not_found_cache,
|
||||
_record_not_found,
|
||||
_read_tracker,
|
||||
)
|
||||
|
||||
tid = "neg-cache-oob-search"
|
||||
_read_tracker.pop(tid, None)
|
||||
missing_dir = tmp_path / "later-dir"
|
||||
|
||||
_record_not_found("search", str(missing_dir), tid, '{"error":"Path not found: x"}')
|
||||
assert _check_not_found_cache("search", str(missing_dir), tid) is not None
|
||||
|
||||
missing_dir.mkdir()
|
||||
(missing_dir / "x.txt").write_text("hi\n")
|
||||
|
||||
assert _check_not_found_cache("search", str(missing_dir), tid) is None, (
|
||||
"stale 'Path not found' served after the directory was created"
|
||||
)
|
||||
|
||||
def test_notify_other_tool_call_clears_not_found(self):
|
||||
"""Belt-and-suspenders: any non-read tool (terminal etc.) invalidates
|
||||
the task's negative cache via the dispatcher's notify hook."""
|
||||
from tools.file_tools import (
|
||||
_check_not_found_cache,
|
||||
_record_not_found,
|
||||
_read_tracker,
|
||||
notify_other_tool_call,
|
||||
)
|
||||
|
||||
tid = "neg-cache-notify"
|
||||
_read_tracker.pop(tid, None)
|
||||
_record_not_found("read", "/tmp/never-exists-notify", tid, '{"error":"x"}')
|
||||
assert _check_not_found_cache("read", "/tmp/never-exists-notify", tid) is not None
|
||||
|
||||
notify_other_tool_call(tid)
|
||||
|
||||
assert _check_not_found_cache("read", "/tmp/never-exists-notify", tid) is None, (
|
||||
"notify_other_tool_call must clear cached misses"
|
||||
)
|
||||
|
||||
@@ -954,6 +954,7 @@ def _check_not_found_cache(op: str, resolved_str: str, task_id: str) -> str | No
|
||||
|
||||
Eviction: TTL or write_file/patch on the path (see invalidate_for_path).
|
||||
"""
|
||||
import os as _os
|
||||
import time
|
||||
with _read_tracker_lock:
|
||||
task_data = _read_tracker.get(task_id)
|
||||
@@ -969,6 +970,15 @@ def _check_not_found_cache(op: str, resolved_str: str, task_id: str) -> str | No
|
||||
if time.monotonic() - ts > _NOT_FOUND_TTL_SECONDS:
|
||||
nf.pop((op, resolved_str), None)
|
||||
return None
|
||||
# Existence guard: the path may have been created since we cached
|
||||
# the miss — by a terminal command, another agent, or any external
|
||||
# process (write_file/patch invalidate explicitly, but they're not
|
||||
# the only writers). The agent pattern "check file → create it →
|
||||
# read it" is common; serving a stale miss for up to the TTL breaks
|
||||
# it. One stat is ~free next to the subprocess walk we're skipping.
|
||||
if _os.path.exists(resolved_str):
|
||||
nf.pop((op, resolved_str), None)
|
||||
return None
|
||||
return cached_json
|
||||
|
||||
|
||||
@@ -1342,7 +1352,7 @@ def read_file_tool(path: str, offset: int = 1, limit: int = 500, task_id: str =
|
||||
# If we already discovered this path doesn't exist (within TTL),
|
||||
# return the cached error without spawning the subprocess +
|
||||
# similar-files walk. Cleared by write_file/patch on the same path.
|
||||
resolved_str_for_neg = str(_resolve_path_for_task(path, task_id))
|
||||
resolved_str_for_neg = str(_resolved)
|
||||
cached_not_found = _check_not_found_cache("read", resolved_str_for_neg, task_id)
|
||||
if cached_not_found is not None:
|
||||
return cached_not_found
|
||||
@@ -1413,11 +1423,16 @@ def read_file_tool(path: str, offset: int = 1, limit: int = 500, task_id: str =
|
||||
# ── Populate negative-result cache on not-found ───────────────
|
||||
# _suggest_similar_files returns ReadResult(error="File not found: ..").
|
||||
# Cache the JSON we'd return so a retry skips the parent-dir walk.
|
||||
# Deliberately NO early return: on upstream, error results flow
|
||||
# through the tracking block below (consecutive-loop detection,
|
||||
# dedup bookkeeping via the resolved path) and the normal exit —
|
||||
# short-circuiting here changes that behavior (and broke a real
|
||||
# test interaction). Serving from the cache (above) is the
|
||||
# optimization; recording must stay side-effect-identical.
|
||||
_err = result_dict.get("error") or ""
|
||||
if isinstance(_err, str) and _err.startswith("File not found:"):
|
||||
_not_found_json = json.dumps(result_dict, ensure_ascii=False)
|
||||
_record_not_found("read", resolved_str_for_neg, task_id, _not_found_json)
|
||||
return _not_found_json
|
||||
|
||||
# ── Character-count guard ─────────────────────────────────────
|
||||
# We're model-agnostic so we can't count tokens; characters are
|
||||
@@ -1594,6 +1609,15 @@ def notify_other_tool_call(task_id: str = "default"):
|
||||
# progress, so clear per-key dedup hit counters too.
|
||||
if "dedup_hits" in task_data:
|
||||
task_data["dedup_hits"].clear()
|
||||
# Any other tool (terminal, delegate, ...) may have created a
|
||||
# previously-missing path — a cached miss is no longer
|
||||
# trustworthy. The serve-side existence guard in
|
||||
# _check_not_found_cache already covers this, but clearing
|
||||
# here keeps the cache honest and covers exotic cases the
|
||||
# stat can't (e.g. permission flips).
|
||||
nf = task_data.get("not_found")
|
||||
if nf:
|
||||
nf.clear()
|
||||
|
||||
|
||||
def _invalidate_dedup_for_path(filepath: str, task_id: str) -> None:
|
||||
@@ -2085,12 +2109,13 @@ def search_tool(pattern: str, target: str = "content", path: str = ".",
|
||||
"token, cache, or secret-bearing environment files."
|
||||
)
|
||||
|
||||
# Populate negative cache when search root was missing.
|
||||
# Populate negative cache when search root was missing. No early
|
||||
# return — same rationale as the read path: error results keep
|
||||
# flowing through the consecutive-search bookkeeping below.
|
||||
_search_err = result_dict.get("error") or ""
|
||||
if isinstance(_search_err, str) and _search_err.startswith("Path not found:"):
|
||||
_search_nf_json = json.dumps(result_dict, ensure_ascii=False)
|
||||
_record_not_found("search", resolved_search_path, task_id, _search_nf_json)
|
||||
return _search_nf_json
|
||||
|
||||
if count >= 3:
|
||||
result_dict["_warning"] = (
|
||||
|
||||
Reference in New Issue
Block a user