diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b2112a8..20d0c28 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,6 +23,10 @@ jobs: # a product worded from it vanishes on adoption. See the script's docstring. - run: python3 scripts/check_unconsumed_fields.py - run: uv run pytest -q + # The gate pins tiktoken's own cache key and content hash, and only a real + # tiktoken can falsify them. Scope that optional dependency to this contract + # test; the isolated run uses the tokenizer version pinned in uv.lock. + - run: uv run --isolated --frozen --extra dev --extra tokenizer pytest -q tests/test_tokenizer_nonblocking.py::test_pinned_cache_metadata_matches_tiktokens_own_declaration - run: uv build # Two products pin an AgentCore revision. If published code changes without a diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index c3d6f94..aea034f 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -34,6 +34,10 @@ jobs: - run: uv run ruff check agent_core tests scripts - run: uv run pyright agent_core - run: uv run pytest -q + # The gate pins tiktoken's own cache key and content hash, and only a real + # tiktoken can falsify them. Scope that optional dependency to this contract + # test; the isolated run uses the tokenizer version pinned in uv.lock. + - run: uv run --isolated --frozen --extra dev --extra tokenizer pytest -q tests/test_tokenizer_nonblocking.py::test_pinned_cache_metadata_matches_tiktokens_own_declaration - run: uv build # A PyPI version number can never be reused, not even after deleting the diff --git a/CHANGELOG.md b/CHANGELOG.md index 53c2e9f..92e8e6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,115 @@ the GitHub Release body, so a release with no entry here fails. Versioning follows [docs/versioning.md](docs/versioning.md). +## [0.9.0] - 2026-09-06 + +### Fixed + +- The tiktoken encoder init no longer starts a background load that may need the + network. The requested `cl100k_base` cache artifact must exist and pass + tiktoken's expected SHA-256; an empty directory, another encoding's cache, or a + corrupt target file therefore cannot reopen the unsafe fetch path. Otherwise + the encoding is marked unavailable, one WARNING says how to fix it, and callers + stay on their CJK-aware heuristics. Cache validation rejects non-regular files, + caps the synchronous read at 4 MiB, and is claimed once per encoding so + concurrent callers do not repeat it. No thread is started, so finalization has + nothing to kill. + + 0.8.1 moved `import tiktoken` to the caller thread and added a 1 s `atexit` + join. That budget covers a cache *hit* (~140 ms) and nothing else: on a miss + `get_encoding` does an unbounded `requests.get` (tiktoken's `load.py` passes no + timeout) plus a BPE parse, so the thread is still mid-flight when the budget + expires and gets `pthread_exit`ed anyway — out of the dynamic linker now, but + plausibly inside `malloc`. **The 0.8.1 fix covered only the warm half**, which + is exactly the half every CI job sees, because warming the cache is the standard + mitigation. + + Reproduced 2026-09-06 in ApodexHarness, repeating the two `serve` protocol + tests with the cache guaranteed empty (`TMPDIR` redirected to a fresh directory + each iteration, `TIKTOKEN_CACHE_DIR` unset): + + | arm | runs | negative exit codes | + |---|---|---| + | cold cache, `test_serve_subprocess_e2e` | 90 | **1** (`-6`) | + | cold cache, `test_stateless_across_invocations` | 30 | 0 | + | warm cache, both tests | 60 | 0 | + | warm cache, exit micro-probe | 200 | 0 | + | cold cache + unroutable proxy, micro-probe and both tests | 260 | 0 | + + The death: `double free or corruption (fasttop)`, `serve exited -6`, after a + fully correct protocol stream — stdout was complete and valid, the process + simply did not survive its own exit. Two details worth keeping. The failure is + ~1%, so a passing run proves nothing and only repetition at a fixed cache state + measures anything. And blocking egress instead of emptying the cache does *not* + reproduce it: a thread parked in a TCP connect allocates nothing and dies + harmlessly, so the dangerous window is a fetch that is *succeeding* and parsing + — the opposite of what the wedge history would suggest. + +### Changed + +- `KeepLastNToolResultsCompactor(max_card_urls=...)` now exposes the Tier 1 mini + card's per-card URL budget, which 0.8.2 narrowed from 3 to 1 as a module + constant. The default is unchanged, so behaviour is identical unless a host + passes the argument. A host that genuinely needs several independent sources per + claim raises it (~120 chars per extra URL per card); 0 drops the card's source + line entirely, keeping the call and any argument URL. 0.8.2's entry told hosts + to raise the constant, which was not something a consumer could do. + +- **Consumer impact: a host that never warmed the tiktoken cache now gets + approximate token counts instead of exact ones**, where it previously got a + one-time runtime fetch that populated the cache for later processes. Measured + against cl100k_base, the heuristic lands at 0.84x of the real count on pure + Chinese, 0.88x on mixed Chinese/Latin, 1.14x on English prose, and 0.72x on + JSON tool arguments. Those are measurements, not an error bound: emoji-heavy + samples measured only 0.10-0.125x, and the context guard's 1.5x buffer does not + cover that case. A host sizing a request against a hard gateway limit must warm + the cache rather than rely on the estimate. Anything built from + `docker/stateful-agent.Dockerfile` (or any image baking `TIKTOKEN_CACHE_DIR`) is + unaffected — that cache is warm, the load runs, counts stay exact. Fresh + checkouts, CI jobs without the warm-up step, and images built without it change + behaviour. + + Two ways back to exact counts, in preference order: warm the cache once as a + build or setup step (`python -c "import tiktoken; + tiktoken.get_encoding('cl100k_base')"`), or set + `AGENT_CORE_TIKTOKEN_FETCH=1` to allow the runtime fetch and accept the + exit-time window it reopens. If `TIKTOKEN_CACHE_DIR=""` currently disables + caching, set it to a writable directory before running the warm-up command. + + **A consumer test that fakes `tiktoken` now takes the gate branch** if the real + cache directory happens to be empty, because the gate looks at the filesystem + before it looks at `sys.modules` — it cannot tell that a monkeypatched module + will never fetch anything. Found by running ApodexHarness's suite against this + branch with `TIKTOKEN_CACHE_DIR` pointed at an empty directory: + `test_first_call_is_nonblocking_even_if_load_is_slow` installs a deliberately + slow fake `get_encoding` and asserts the encoder eventually lands, which it now + never does. Such a test should set `AGENT_CORE_TIKTOKEN_FETCH=1`; a placeholder + cache file is deliberately insufficient because production must not mistake it + for a usable vocabulary. Note the same suite is fully green with a valid warm + cache, so this is invisible until a host runs cold. + + The gate pins tiktoken's cache key and expected content hash for + `cl100k_base`, the only encoding AgentCore requests. Other encoding names fail + closed unless runtime fetching is explicitly enabled. If a future tiktoken + release changes its artifact, exact counts remain disabled rather than silently + reopening a network-capable daemon thread; update the pinned metadata as part of + that dependency upgrade. Those two pins are now checked against tiktoken's own + declaration (`tiktoken_ext.openai_public`, read without calling the constructor + that fetches) by `test_pinned_cache_metadata_matches_tiktokens_own_declaration`. + The test also exercises tiktoken's real cache lookup with network access stubbed + out, so a cache-key algorithm change fails the build. CI runs it with the + lockfile's tokenizer extra in an isolated environment, for the reason in the + next paragraph. + + **A host test calibrated against the heuristic changes answer once the cache is + warm**, because the cache state now decides which estimator runs. Found here: + installing tiktoken across this repo's own suite turned + `test_agent_loop_engine.py`'s context-guard arithmetic red, since a 4000-char + filler string is ~1000 heuristic tokens but far fewer real ones. That test now + pins the heuristic explicitly. Consumer tests that assert on token totals should + do the same rather than inherit whichever estimator the machine's cache happens + to select. + ## [0.8.2] - 2026-09-06 ### Changed @@ -16,7 +125,9 @@ Versioning follows [docs/versioning.md](docs/versioning.md). gets `[Called: ]` alone instead of a 120-char argument preview. **Consumer impact:** a host that needs several independent sources per claim - should now raise `_MINI_CARD_MAX_URLS` deliberately rather than inherit 3. + loses the 2nd and 3rd URL it used to inherit; the first URL alone covered 99.2% + of carded results that had any. 0.9.0 turns this into a constructor argument + (`max_card_urls`) for hosts that want the old width back. Nothing else changes: cards still carry the call, still carry a source when one exists, and the placeholder/footer contract is untouched. diff --git a/agent_core/runtime/loop/compact.py b/agent_core/runtime/loop/compact.py index fd570a9..3c3d680 100644 --- a/agent_core/runtime/loop/compact.py +++ b/agent_core/runtime/loop/compact.py @@ -169,8 +169,9 @@ def tool_names_by_call_id(messages: list[Message]) -> dict[str, str]: # first URL alone covers 769/775 (99.2%) of the carded results that had any URL. # Dropping to 1 took total retention to 8.9% and left that 99.2% unchanged, i.e. # the extra two URLs per card were spending ~120 chars each on a percentage with -# no reader. A host that needs several independent sources per claim should raise -# this deliberately rather than inherit it. +# no reader. A host that needs several independent sources per claim raises it +# through ``KeepLastNToolResultsCompactor(max_card_urls=...)`` rather than +# inheriting this default. _MINI_CARD_MAX_URLS = 1 _WHITESPACE_RE = re.compile(r"\s+") @@ -234,6 +235,7 @@ def _elided_tool_card( args_preview: str, args_source_url: str, content: str, + max_urls: int = _MINI_CARD_MAX_URLS, ) -> str: """Render the card lines that stand in for a discarded tool body. @@ -269,7 +271,7 @@ def _elided_tool_card( urls: list[str] = [] for url in dict.fromkeys(URL_RE.findall(content)): - if len(urls) >= _MINI_CARD_MAX_URLS: + if len(urls) >= max_urls: break # A web_fetch card would otherwise print its own url twice. if url in args_preview: @@ -628,7 +630,7 @@ class KeepLastNToolResultsCompactor: Keeps the last ``keep_tool_result`` tool results verbatim and replaces the content of every earlier one with :data:`OMITTED_TOOL_RESULT_PLACEHOLDER` followed by a bounded card naming the call (tool + arguments preview) and up - to :data:`_MINI_CARD_MAX_URLS` source URLs found in the discarded body (a + to ``max_card_urls`` source URLs found in the discarded body (a result with no source anywhere gets the tool name alone), then the recovery pointer when the body was spilled. The card is free — both fields already exist in the history and in the body — and it is what keeps a @@ -663,9 +665,12 @@ def __init__( protect_tool_names: frozenset[str] = frozenset(), spill: Callable[[str, str], str | None] | None = None, recovery_footer: Callable[[str], str] = default_recovery_footer, + max_card_urls: int = _MINI_CARD_MAX_URLS, ) -> None: if keep_tool_result < -1: raise ValueError(f"keep_tool_result must be >= -1 (got {keep_tool_result})") + if max_card_urls < 0: + raise ValueError(f"max_card_urls must be >= 0 (got {max_card_urls})") self._keep = keep_tool_result # Tool names whose results are NEVER blanked regardless of age (e.g. # agent-team fan-in: collect_reports / assign_task / submit_report). @@ -675,6 +680,13 @@ def __init__( self._protect = frozenset(protect_tool_names) self._spill = spill self._recovery_footer = recovery_footer + # How many source URLs a card may carry. The default of 1 is measured + # (the first URL alone covers 99.2% of carded results that had any), so + # raising it costs ~120 chars per extra URL per card for a completeness + # nothing downstream was reading. A host that genuinely needs several + # independent sources per claim sets it deliberately; 0 drops the body's + # URL line entirely, keeping only the call and any argument URL. + self._max_card_urls = max_card_urls def compact( self, @@ -732,6 +744,7 @@ def compact( id_to_args.get(call_id, ""), id_to_arg_url.get(call_id, ""), content, + self._max_card_urls, ) if card: placeholder += "\n" + card diff --git a/agent_core/runtime/loop/tokenizer.py b/agent_core/runtime/loop/tokenizer.py index 8c7bea0..887e04b 100644 --- a/agent_core/runtime/loop/tokenizer.py +++ b/agent_core/runtime/loop/tokenizer.py @@ -14,8 +14,9 @@ local file read — see ``docker/stateful-agent.Dockerfile``. 2. **This module**: the first request for an encoding kicks the (potentially network-fetching) init onto a daemon thread and returns - ``None``; callers fall back to a chars/4 heuristic until the encoder - lands. The loop thread NEVER blocks on tiktoken, cache-baked or not. + ``None``; callers fall back to their CJK-aware heuristics until the + encoder lands. The loop thread NEVER blocks on tiktoken, cache-baked + or not. What is deliberately NOT on that daemon thread is ``import tiktoken``. ``tiktoken._tiktoken`` is a Rust extension, so the import ``dlopen``s a @@ -37,12 +38,46 @@ ``providers/nonblocking_stream.py``. CPython runs ``atexit`` callbacks before it starts killing daemon threads, so that is the last point at which the load can be drained cleanly. + +That join has a 1 s budget, which covers a cache *hit* (~140 ms) and +nothing else. On a cache **miss** ``get_encoding`` does an unbounded +``requests.get`` (tiktoken's ``load.py`` passes no timeout) plus a BPE +parse, so it is still mid-flight when the budget expires and the thread +is killed anyway — no longer inside the dynamic linker, but plausibly +inside ``malloc``. Reproduced 2026-09-06 on +``test_serve_subprocess_e2e``: with the vocab cache guaranteed empty, +1 of 90 runs died with ``double free or corruption (fasttop)`` and +``-6`` after a fully correct protocol stream, while 60/60 warm runs and +400 warm/blocked-egress micro-probes were clean. So the previous fix +covered only the warm half. + +Hence the third layer: no thread is started unless the requested vocab's +exact cache artifact exists and passes the hash tiktoken itself expects. +An unrelated or corrupt cache file cannot accidentally reopen the fetch +path. There is then nothing to kill, at the cost of approximate token +counts on a host that never warmed the cache. A host that wants the +fetch anyway sets ``AGENT_CORE_TIKTOKEN_FETCH=1`` and accepts the window. + +Proving the load is local costs a SHA-256 over the cached vocab — ~1.7 MB +for ``cl100k_base``, single-digit milliseconds — and it runs on the +calling thread, like the import above it. The file is opened non-blocking, +must be a regular file, and the read is capped at 4 MiB before its hash is +trusted. The first caller claims the encoding before validation, so +concurrent callers return ``None`` rather than repeating the read. Same +reasoning: bounded local validation is not what the network defense above +exists for. Verifying with tiktoken's own constructor instead would mean +*calling the thing that may fetch*, which is the operation being avoided. """ from __future__ import annotations import atexit +import contextlib +import hashlib import logging +import os +import stat +import tempfile import threading import time from typing import Any @@ -55,7 +90,7 @@ # Per-encoding cache. State machine for a given name: # absent (==_MISSING) → never requested -# None → background init in flight; use the heuristic for now +# None → validation/import/background init in flight; heuristic for now # False → tiktoken unavailable / bad name; terminal, heuristic forever # → ready _encoders: dict[str, Any] = {} @@ -73,6 +108,96 @@ # dangerous in the first place. _JOIN_TIMEOUT_S = 1.0 +# Opt back in to the unbounded network fetch (and its exit-time window). +_FETCH_ENV = "AGENT_CORE_TIKTOKEN_FETCH" +_TRUTHY = frozenset({"1", "true", "yes", "on"}) + +# cl100k_base is ~1.7 MB. Bound the synchronous validation read so a corrupt, +# replaced, or concurrently growing cache file cannot turn this non-blocking +# accessor into an unbounded caller-thread read. +_MAX_CACHE_BYTES = 4 * 1024 * 1024 + +# tiktoken caches the downloaded bytes under ``sha1(blobpath)`` and validates +# them against the constructor's expected SHA-256 before parsing. All AgentCore +# callers currently request cl100k_base. Keeping both values here lets us prove +# that this exact load is local without invoking tiktoken's constructor (which +# is the operation that may fetch). If tiktoken changes either value, this gate +# fails closed until the table is updated. +_CACHE_SPECS: dict[str, tuple[str, str]] = { + "cl100k_base": ( + "9b5ad71b2ce5302211f9c61530b329a4922fc6a4", + "223921b76ee99bde995b7ff738513eef100fb51d18c93597a113bcffe865b2a7", + ), +} + +# The "no valid target cache, staying on the heuristic" warning is worth once. +_warned_uncached = False + + +def _cache_dir() -> str: + """Where tiktoken would look for a cached vocab. + + Mirrors ``tiktoken.load.read_file_cached``: ``TIKTOKEN_CACHE_DIR``, + else ``DATA_GYM_CACHE_DIR``, else ``/data-gym-cache``. An empty + string is tiktoken's way of disabling the cache entirely. + """ + for var in ("TIKTOKEN_CACHE_DIR", "DATA_GYM_CACHE_DIR"): + value = os.environ.get(var) + if value is not None: + return value + return os.path.join(tempfile.gettempdir(), "data-gym-cache") + + +def _cache_problem(name: str) -> str | None: + """Why ``name`` cannot be proved to load locally, or ``None`` if it can.""" + directory = _cache_dir() + if not directory: + return "disabled" + + spec = _CACHE_SPECS.get(name) + if spec is None: + return "unverified encoding" + + cache_key, expected_hash = spec + path = os.path.join(directory, cache_key) + flags = os.O_RDONLY | getattr(os, "O_NONBLOCK", 0) | getattr(os, "O_CLOEXEC", 0) + flags |= getattr(os, "O_NOFOLLOW", 0) + try: + fd = os.open(path, flags) + except FileNotFoundError: + return "missing" + except OSError: + return "unreadable" + + try: + info = os.fstat(fd) + if not stat.S_ISREG(info.st_mode): + return "not a regular file" + if info.st_size > _MAX_CACHE_BYTES: + return "oversized" + + digest = hashlib.sha256() + remaining = _MAX_CACHE_BYTES + 1 + while remaining: + chunk = os.read(fd, min(1024 * 1024, remaining)) + if not chunk: + break + digest.update(chunk) + remaining -= len(chunk) + if remaining == 0: + return "oversized" + actual_hash = digest.hexdigest() + except OSError: + return "unreadable" + finally: + with contextlib.suppress(OSError): + os.close(fd) + return None if actual_hash == expected_hash else "invalid" + + +def _network_fetch_allowed() -> bool: + return os.environ.get(_FETCH_ENV, "").strip().lower() in _TRUTHY + def _join_pending() -> None: """atexit: drain in-flight inits before the interpreter kills them.""" @@ -110,14 +235,57 @@ def get_encoding_nonblocking(name: str = "cl100k_base") -> Any | None: local dlopen, no network), schedules the encoder init on a daemon thread and returns ``None``; later calls return the encoder once it has loaded, or ``None`` while it is still loading. Returns ``None`` - permanently when tiktoken is unavailable — callers MUST fall back to - a heuristic on ``None``. + permanently when tiktoken is unavailable, and permanently when the + requested vocab has no valid cache artifact so the init could issue + an unbounded network fetch (``AGENT_CORE_TIKTOKEN_FETCH=1`` opts + back in) — callers MUST fall back to a heuristic on ``None``. """ - global _atexit_registered + global _atexit_registered, _warned_uncached + + # Claim the name before any filesystem work. Concurrent first callers return + # immediately on the ``None`` state instead of hashing the same artifact. + with _lock: + enc = _encoders.get(name, _MISSING) + if enc is not _MISSING: + return enc or None # None (initializing) and False (failed) collapse to None + _encoders[name] = None - enc = _encoders.get(name, _MISSING) - if enc is not _MISSING: - return enc or None # None (loading) and False (failed) both collapse to None + # A load that can only be served over the network is not worth a + # thread: it cannot finish inside the exit-time join budget, and + # being killed mid-fetch is what corrupts the heap. + fetch_allowed = _network_fetch_allowed() + cache_problem = None if fetch_allowed else _cache_problem(name) + if cache_problem is not None: + with _lock: + _encoders[name] = False + should_warn = not _warned_uncached + _warned_uncached = True + if should_warn: + directory = _cache_dir() + if cache_problem == "disabled": + logger.warning( + "tiktoken caching is disabled by TIKTOKEN_CACHE_DIR=''; " + "token counts stay approximate (CJK-aware heuristic). Set " + "TIKTOKEN_CACHE_DIR to a writable directory, warm %s there, " + "or set %s=1 to fetch it at runtime.", + name, + _FETCH_ENV, + ) + else: + logger.warning( + "tiktoken vocab cache %r has no valid %s artifact (%s); " + "token counts stay approximate (CJK-aware heuristic). Warm it " + "once with " + "`python -c 'import tiktoken; tiktoken.get_encoding(\"%s\")'` " + "after ensuring TIKTOKEN_CACHE_DIR points to a readable and " + "writable cache; set %s=1 to fetch it at runtime instead.", + directory, + name, + cache_problem, + name, + _FETCH_ENV, + ) + return None # On the caller thread, deliberately — never on the daemon thread. try: @@ -129,9 +297,6 @@ def get_encoding_nonblocking(name: str = "cl100k_base") -> Any | None: return None with _lock: - if _encoders.get(name, _MISSING) is not _MISSING: # claimed while we imported - return _encoders[name] or None - _encoders[name] = None # mark loading so concurrent callers don't re-spawn thread = threading.Thread( target=_load, args=(name, tiktoken), diff --git a/pyproject.toml b/pyproject.toml index 07a3639..065d0c9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "apodex-agent-core" -version = "0.8.2" +version = "0.9.0" description = "Shared, product-neutral runtime primitives for Apodex agents" readme = "README.md" license = "Apache-2.0" diff --git a/tests/test_agent_loop_engine.py b/tests/test_agent_loop_engine.py index dff3cda..017f4c8 100644 --- a/tests/test_agent_loop_engine.py +++ b/tests/test_agent_loop_engine.py @@ -688,7 +688,9 @@ async def test_loop_config_cap_overrides_the_policy_default() -> None: @pytest.mark.asyncio -async def test_synthesised_zero_usage_does_not_reset_the_token_estimate() -> None: +async def test_synthesised_zero_usage_does_not_reset_the_token_estimate( + monkeypatch: pytest.MonkeyPatch, +) -> None: """A gateway that omits usage must not silently disable the overflow guard. The zero-filled fallback exists for cost attribution only; letting its @@ -704,6 +706,12 @@ async def test_synthesised_zero_usage_does_not_reset_the_token_estimate() -> Non turn 2, zeros : 0 + 1506 + 1000 = 2506 < 3000 (guard misses) turn 2, preserved : 950 + 1506 + 1000 = 3456 >= 3000 (guard fires) """ + # This test's arithmetic deliberately targets the fallback estimator. Pin + # that dependency so a warm host cache cannot switch it to exact tiktoken + # counts and change the premise being tested. + from agent_core.runtime.loop import context_budget + + monkeypatch.setattr(context_budget, "_get_tokenizer", lambda: None) class SizedTool: name = "echo" diff --git a/tests/test_keep_last_n_compactor.py b/tests/test_keep_last_n_compactor.py index e4c85a1..b6ec812 100644 --- a/tests/test_keep_last_n_compactor.py +++ b/tests/test_keep_last_n_compactor.py @@ -9,6 +9,8 @@ from __future__ import annotations +import pytest + from agent_core.messages import system_msg, tool_msg from agent_core.runtime.loop.compact import ( _LEGACY_OMITTED_TOOL_RESULT_PLACEHOLDERS, @@ -367,3 +369,33 @@ def test_at_most_one_url_even_in_a_url_heavy_body(): body = " ".join(f"https://example.com/r{i}" for i in range(50)) + "x" * 2_000 content = _blanked(_one_call("web_search", '{"query": "q"}', body)) assert content.count("https://example.com/r") == _MINI_CARD_MAX_URLS + + +# --- the max_card_urls knob ------------------------------------------------ + + +def test_default_is_one_url_so_hosts_inherit_the_measured_budget(): + assert _MINI_CARD_MAX_URLS == 1 + body = " ".join(f"https://example.com/r{i}" for i in range(5)) + " " + "x" * 2_000 + assert _blanked(_one_call("web_search", '{"query": "q"}', body)).count("https://") == 1 + + +def test_a_host_that_needs_several_sources_raises_max_card_urls(): + """The knob the CHANGELOG points at: more URLs per card, on request.""" + body = " ".join(f"https://example.com/r{i}" for i in range(5)) + " " + "x" * 2_000 + content = _blanked(_one_call("web_search", '{"query": "q"}', body), max_card_urls=3) + assert content.count("https://example.com/r") == 3 + # Still bounded by the body budget, not by the raised count alone. + assert len(_card_of(content)) <= _MINI_CARD_BODY_MAX_CHARS + + +def test_zero_max_card_urls_drops_the_source_line_but_keeps_the_call(): + body = "see https://example.com/r0 " + "x" * 2_000 + content = _blanked(_one_call("web_search", '{"query": "q"}', body), max_card_urls=0) + assert "[Source URLs]" not in content + assert "[Called: web_search(" in content + + +def test_negative_max_card_urls_is_rejected_at_construction(): + with pytest.raises(ValueError, match="max_card_urls must be >= 0"): + KeepLastNToolResultsCompactor(keep_tool_result=0, max_card_urls=-1) diff --git a/tests/test_tokenizer_nonblocking.py b/tests/test_tokenizer_nonblocking.py index e460343..cbe86d3 100644 --- a/tests/test_tokenizer_nonblocking.py +++ b/tests/test_tokenizer_nonblocking.py @@ -8,13 +8,28 @@ caller thread and only ``get_encoding`` on the daemon thread — these tests pin which thread runs which, and that the exit hook drains the load. + +The load itself is only started when the vocab cache can serve it. A +cache miss means an unbounded fetch that outlives the exit-time join, so +the thread gets killed mid-``malloc`` instead — reproduced as +``double free or corruption (fasttop)`` / ``-6``. The gate around that, +and its ``AGENT_CORE_TIKTOKEN_FETCH`` escape hatch, are pinned below. +Every test therefore has to be explicit about the cache state; the +fixture points ``TIKTOKEN_CACHE_DIR`` at a warm directory so the host's +own ``/tmp`` cannot decide which branch runs. """ from __future__ import annotations +import hashlib +import importlib import importlib.util +import inspect +import os +import re import sys import threading +import time import types from typing import Any @@ -54,11 +69,31 @@ def _get_encoding(self, name: str) -> object: @pytest.fixture -def clean_tokenizer(): +def warm_cache_dir(tmp_path, monkeypatch: pytest.MonkeyPatch): + """A valid target vocab cache, so the fetch gate lets loads run.""" + cache = tmp_path / "data-gym-cache" + cache.mkdir() + cache_key, _ = tokenizer._CACHE_SPECS["cl100k_base"] + payload = b"ranks" + monkeypatch.setitem( + tokenizer._CACHE_SPECS, + "cl100k_base", + (cache_key, hashlib.sha256(payload).hexdigest()), + ) + (cache / cache_key).write_bytes(payload) + monkeypatch.setenv("TIKTOKEN_CACHE_DIR", str(cache)) + monkeypatch.delenv("AGENT_CORE_TIKTOKEN_FETCH", raising=False) + return cache + + +@pytest.fixture +def clean_tokenizer(warm_cache_dir): """Reset the module cache and unhook any real/fake tiktoken.""" saved_module = sys.modules.pop("tiktoken", None) saved_meta = list(sys.meta_path) saved_atexit_registered = tokenizer._atexit_registered + saved_warned = tokenizer._warned_uncached + tokenizer._warned_uncached = False # the warning is once-per-process tokenizer._encoders.clear() tokenizer._threads.clear() tokenizer._atexit_registered = True # don't leak a real atexit hook per test @@ -69,6 +104,7 @@ def clean_tokenizer(): tokenizer._encoders.clear() tokenizer._threads.clear() tokenizer._atexit_registered = saved_atexit_registered + tokenizer._warned_uncached = saved_warned sys.meta_path[:] = saved_meta sys.modules.pop("tiktoken", None) if saved_module is not None: @@ -171,10 +207,21 @@ def find_spec(self, fullname: str, path: Any = None, target: Any = None) -> Any: assert tokenizer.get_encoding_nonblocking("cl100k_base") is None -def test_concurrent_callers_spawn_one_thread(clean_tokenizer) -> None: +def test_concurrent_callers_validate_once_and_spawn_one_thread( + clean_tokenizer, monkeypatch: pytest.MonkeyPatch +) -> None: gate = threading.Event() loader = _RecordingLoader(gate=gate) sys.meta_path.insert(0, loader) + real_cache_problem = tokenizer._cache_problem + cache_checks = 0 + + def counted_cache_problem(name: str) -> str | None: + nonlocal cache_checks + cache_checks += 1 + return real_cache_problem(name) + + monkeypatch.setattr(tokenizer, "_cache_problem", counted_cache_problem) try: start = threading.Barrier(4) @@ -189,5 +236,232 @@ def call() -> None: t.join(timeout=5.0) assert len(tokenizer._threads) == 1 + assert cache_checks == 1 finally: gate.set() + + +# -- the cold-cache fetch gate ------------------------------------------------ + + +def test_empty_cache_dir_stays_on_the_heuristic_and_spawns_no_thread( + clean_tokenizer, tmp_path, monkeypatch: pytest.MonkeyPatch, caplog +) -> None: + """No thread means nothing for finalization to kill mid-fetch.""" + loader = _RecordingLoader() + sys.meta_path.insert(0, loader) + empty = tmp_path / "empty" + empty.mkdir() + monkeypatch.setenv("TIKTOKEN_CACHE_DIR", str(empty)) + + with caplog.at_level("WARNING", logger=tokenizer.logger.name): + assert tokenizer.get_encoding_nonblocking("cl100k_base") is None + + assert tokenizer._threads == {} + assert loader.import_thread is None # not even imported + assert tokenizer._encoders["cl100k_base"] is False # terminal + assert tokenizer.get_encoding_nonblocking("cl100k_base") is None + assert "TIKTOKEN_CACHE_DIR" in caplog.text + assert "AGENT_CORE_TIKTOKEN_FETCH" in caplog.text + + +@pytest.mark.parametrize( + "cache_dir", + ["", "does/not/exist"], + ids=["caching-disabled", "missing-directory"], +) +def test_cache_problem_without_a_usable_cache( + clean_tokenizer, tmp_path, monkeypatch: pytest.MonkeyPatch, cache_dir: str +) -> None: + target = "" if cache_dir == "" else str(tmp_path / cache_dir) + monkeypatch.setenv("TIKTOKEN_CACHE_DIR", target) + assert tokenizer._cache_problem("cl100k_base") is not None + + +def test_populated_cache_lets_the_load_run(clean_tokenizer) -> None: + assert tokenizer._cache_problem("cl100k_base") is None + + loader = _RecordingLoader() + sys.meta_path.insert(0, loader) + assert tokenizer.get_encoding_nonblocking("cl100k_base") is None + tokenizer._join_pending() + assert tokenizer.get_encoding_nonblocking("cl100k_base") is loader.encoder + + +def test_unrelated_cache_file_spawns_no_thread( + clean_tokenizer, warm_cache_dir +) -> None: + cache_key, _ = tokenizer._CACHE_SPECS["cl100k_base"] + (warm_cache_dir / cache_key).unlink() + (warm_cache_dir / "another-encoding").write_bytes(b"valid for something else") + loader = _RecordingLoader() + sys.meta_path.insert(0, loader) + + assert tokenizer._cache_problem("cl100k_base") is not None + assert tokenizer.get_encoding_nonblocking("cl100k_base") is None + assert tokenizer._threads == {} + assert loader.import_thread is None + + +def test_corrupt_target_cache_file_spawns_no_thread( + clean_tokenizer, warm_cache_dir +) -> None: + cache_key, _ = tokenizer._CACHE_SPECS["cl100k_base"] + (warm_cache_dir / cache_key).write_bytes(b"corrupt") + loader = _RecordingLoader() + sys.meta_path.insert(0, loader) + + assert tokenizer._cache_problem("cl100k_base") is not None + assert tokenizer.get_encoding_nonblocking("cl100k_base") is None + assert tokenizer._threads == {} + assert loader.import_thread is None + + +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="FIFO probe requires POSIX") +def test_fifo_at_target_cache_path_does_not_block_or_spawn_a_thread( + clean_tokenizer, warm_cache_dir +) -> None: + cache_key, _ = tokenizer._CACHE_SPECS["cl100k_base"] + target = warm_cache_dir / cache_key + target.unlink() + os.mkfifo(target) + loader = _RecordingLoader() + sys.meta_path.insert(0, loader) + + started = time.monotonic() + assert tokenizer.get_encoding_nonblocking("cl100k_base") is None + + assert time.monotonic() - started < 0.5 + assert tokenizer._threads == {} + assert loader.import_thread is None + assert tokenizer._encoders["cl100k_base"] is False + + +def test_oversized_target_cache_file_spawns_no_thread( + clean_tokenizer, warm_cache_dir +) -> None: + cache_key, _ = tokenizer._CACHE_SPECS["cl100k_base"] + with (warm_cache_dir / cache_key).open("wb") as cache_file: + cache_file.truncate(tokenizer._MAX_CACHE_BYTES + 1) + loader = _RecordingLoader() + sys.meta_path.insert(0, loader) + + assert tokenizer.get_encoding_nonblocking("cl100k_base") is None + assert tokenizer._threads == {} + assert loader.import_thread is None + + +def test_unknown_encoding_fails_closed(clean_tokenizer) -> None: + loader = _RecordingLoader() + sys.meta_path.insert(0, loader) + + assert tokenizer._cache_problem("future_encoding") is not None + assert tokenizer.get_encoding_nonblocking("future_encoding") is None + assert tokenizer._threads == {} + assert loader.import_thread is None + + +def test_data_gym_cache_dir_is_the_second_choice( + clean_tokenizer, warm_cache_dir, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.delenv("TIKTOKEN_CACHE_DIR", raising=False) + monkeypatch.setenv("DATA_GYM_CACHE_DIR", str(warm_cache_dir)) + assert tokenizer._cache_dir() == str(warm_cache_dir) + assert tokenizer._cache_problem("cl100k_base") is None + + +def test_disabled_cache_warning_requires_a_writable_directory( + clean_tokenizer, monkeypatch: pytest.MonkeyPatch, caplog +) -> None: + monkeypatch.setenv("TIKTOKEN_CACHE_DIR", "") + + with caplog.at_level("WARNING", logger=tokenizer.logger.name): + assert tokenizer.get_encoding_nonblocking("cl100k_base") is None + + assert "caching is disabled" in caplog.text + assert "writable directory" in caplog.text + + +@pytest.mark.parametrize("value", ["1", "true", "YES", " on "]) +def test_explicit_opt_in_restores_the_network_fetch( + clean_tokenizer, tmp_path, monkeypatch: pytest.MonkeyPatch, value: str +) -> None: + loader = _RecordingLoader() + sys.meta_path.insert(0, loader) + monkeypatch.setenv("TIKTOKEN_CACHE_DIR", str(tmp_path / "gone")) + monkeypatch.setenv("AGENT_CORE_TIKTOKEN_FETCH", value) + + assert tokenizer.get_encoding_nonblocking("cl100k_base") is None + assert loader.import_thread == threading.current_thread().name + tokenizer._join_pending() + assert tokenizer._encoders["cl100k_base"] is loader.encoder + + +@pytest.mark.parametrize("value", ["", "0", "false", "no"]) +def test_unrecognised_opt_in_values_keep_the_gate_closed( + clean_tokenizer, tmp_path, monkeypatch: pytest.MonkeyPatch, value: str +) -> None: + monkeypatch.setenv("TIKTOKEN_CACHE_DIR", str(tmp_path / "gone")) + monkeypatch.setenv("AGENT_CORE_TIKTOKEN_FETCH", value) + assert tokenizer._network_fetch_allowed() is False + + +def test_pinned_cache_metadata_matches_tiktokens_own_declaration( + tmp_path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The pinned cache key and content hash must be tiktoken's actual ones. + + ``_CACHE_SPECS`` is otherwise unfalsifiable. Every test above monkeypatches + it and writes a payload whose hash it just computed, so a typo in either + constant — or a tiktoken release that re-publishes a vocab — leaves the gate + permanently closed with nothing going red. That failure is silent by + construction: exact token counts become heuristic ones and the one WARNING + blames the host's cache for a directory that is in fact correctly warmed. + + Read the truth out of ``tiktoken_ext.openai_public`` *without* calling the + constructor, because calling it is the fetch this whole module exists to + avoid. The cache key is ``sha1(blobpath)`` (tiktoken's ``read_file_cached``) + and the content hash is the ``expected_hash`` its loader validates against. + """ + if importlib.util.find_spec("tiktoken") is None: + pytest.skip("tiktoken is the optional `tokenizer` extra; install it to check these pins") + # Once tiktoken itself is present, a moved/removed declaration module must + # fail this test rather than quietly turn the dedicated CI check into a skip. + pub = importlib.import_module("tiktoken_ext.openai_public") + load = importlib.import_module("tiktoken.load") + for name, (cache_key, expected_hash) in tokenizer._CACHE_SPECS.items(): + constructor = getattr(pub, name, None) + assert constructor is not None, ( + f"tiktoken no longer defines a {name!r} constructor, so _CACHE_SPECS pins a " + "vocab upstream does not publish under that name" + ) + source = inspect.getsource(constructor) + blobpath = re.search(r'"(https://\S+?\.tiktoken)"', source) + declared = re.search(r'expected_hash\s*=\s*"([0-9a-f]{64})"', source) + assert blobpath and declared, ( + f"cannot read {name!r}'s blobpath and expected_hash out of tiktoken's source; " + "its shape changed, so verify _CACHE_SPECS by hand and repair this test" + ) + assert hashlib.sha1(blobpath.group(1).encode()).hexdigest() == cache_key, ( + f"{name!r} cache key is stale: tiktoken caches {blobpath.group(1)} under a " + "different name now, so the gate can never find a warm cache" + ) + assert declared.group(1) == expected_hash, ( + f"{name!r} content hash is stale: tiktoken expects {declared.group(1)}, so a " + "correctly warmed cache reads as invalid and exact counts stay off" + ) + + # Exercise tiktoken's real cache lookup without allowing a network read. + # This catches a change away from sha1(blobpath), which comparing the + # constructor literals alone cannot detect. + cache = tmp_path / name + cache.mkdir() + payload = b"cache-key-probe" + (cache / cache_key).write_bytes(payload) + monkeypatch.setenv("TIKTOKEN_CACHE_DIR", str(cache)) + + def reject_network(path: str) -> bytes: + pytest.fail(f"tiktoken ignored the pinned cache key and tried to read {path}") + + monkeypatch.setattr(load, "read_file", reject_network) + assert load.read_file_cached(blobpath.group(1), expected_hash=None) == payload diff --git a/uv.lock b/uv.lock index be1b34b..74c0d0e 100644 --- a/uv.lock +++ b/uv.lock @@ -50,7 +50,7 @@ wheels = [ [[package]] name = "apodex-agent-core" -version = "0.8.2" +version = "0.9.0" source = { editable = "." } dependencies = [ { name = "anthropic", extra = ["bedrock"] },