Skip to content

fix(tokenizer): never start a vocab load that can only be fetched - #34

Merged
zhanghanduo merged 7 commits into
mainfrom
fix/tokenizer-cold-cache-fetch-gate
Sep 6, 2026
Merged

fix(tokenizer): never start a vocab load that can only be fetched#34
zhanghanduo merged 7 commits into
mainfrom
fix/tokenizer-cold-cache-fetch-gate

Conversation

@zhanghanduo

@zhanghanduo zhanghanduo commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

What

0.8.1 moved import tiktoken to the caller thread and added a 1 s atexit
join. That covers a cache hit, but not a cache miss: get_encoding() can perform
an unbounded network fetch plus BPE parsing, leaving its daemon thread alive when
the interpreter exits. Repeated cold-cache protocol tests reproduced one
post-response double free or corruption (fasttop) / -6 failure in 90 runs.

This PR starts no tokenizer thread unless the requested cl100k_base artifact is
already cached and passes tiktoken's expected SHA-256. Missing, disabled,
unrelated, corrupt, oversized, FIFO, and other non-regular cache
entries fail closed to the callers' CJK-aware heuristics. Operators can warm the
cache during build/setup, or explicitly restore runtime fetching with
AGENT_CORE_TIKTOKEN_FETCH=1 and accept the original exit-time risk.

Safety details

  • The cache key and content hash are pinned for cl100k_base, the only encoding
    requested by AgentCore.
  • Cache files are opened with non-blocking/no-follow flags where supported,
    accepted only when regular, and read at most 4 MiB before hashing. A FIFO or a
    concurrently growing file cannot turn validation into an unbounded read.
  • The first caller claims an encoding before validation; concurrent callers
    return None instead of repeating the filesystem work.
  • A real-tiktoken contract test compares the pinned URL/hash metadata and
    exercises tiktoken's actual cache lookup with network reads stubbed out. CI and
    release jobs run it in an isolated environment using the tokenizer version in
    uv.lock.

Consumer impact (MINOR)

A host with no valid warm cache now remains on approximate token counts instead
of fetching once and eventually becoming exact. The fallback is not a hard upper
bound: measured heuristic/real ratios were 0.84x for Chinese, 0.88x mixed
Chinese/Latin, 1.14x English prose, 0.72x JSON tool arguments, and roughly
0.10-0.125x for emoji-heavy samples. The context guard's 1.5x buffer does not
cover every input. Hosts sizing against a hard provider limit should warm the
cache.

Images that bake TIKTOKEN_CACHE_DIR remain on exact counts. If caching is
disabled with TIKTOKEN_CACHE_DIR="", set it to a writable directory before
running:

python -c "import tiktoken; tiktoken.get_encoding('cl100k_base')"

Tests that intentionally assert heuristic token arithmetic should pin that
estimator; this PR does so for the context-overflow regression test that was
previously sensitive to the host's cache state.

Verification

  • Default environment: 1505 passed, 1 skipped
  • Locked tokenizer extra with a real warm cache: 1506 passed
  • Dedicated pin/cache-lookup contract test: passed
  • Ruff: passed
  • Pyright: 0 errors
  • git diff --check: clean

Version remains 0.9.0: this deliberately changes observable cold-cache
behavior and therefore crosses the repository's 0.MINOR.PATCH compatibility
boundary.

0.8.1 moved `import tiktoken` off the daemon 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` plus a BPE
parse, so the thread is still mid-flight when the join expires and gets
killed anyway — out of the dynamic linker, into `malloc`.

Reproduced by repeating the two serve protocol tests with the cache
guaranteed empty: 1 of 90 `test_serve_subprocess_e2e` runs died with
`double free or corruption (fasttop)` / `-6` after a fully correct
protocol stream, against 0 of 60 warm and 0 of 260 blocked-egress. So
the earlier fix covered only the warm half — the half CI sees, since
warming the cache is the standard mitigation.

So when the cache can serve nothing, start no thread: mark the encoding
unavailable, warn once with the fix, and stay on chars/4. Token counts
go approximate on hosts that never warmed the cache;
AGENT_CORE_TIKTOKEN_FETCH=1 opts back into the fetch and its window.
@zhanghanduo

Copy link
Copy Markdown
Collaborator Author

Verification follow-up — two things the original description did not cover.

1. AgentCore's own venv has no tiktoken, so this repo's suite never exercises the real library; every tokenizer test runs against a fake loader. Checked the three real-library branches by shadow-loading this branch into a consumer venv that does have tiktoken:

arm result
warm cache threads=['tiktoken-init-cl100k_base'] — load runs, unchanged
cold cache threads=[] + the WARNING (50/50 runs)
cold cache + AGENT_CORE_TIKTOKEN_FETCH=1 threads=[...] — old behaviour restored

The crash's precondition is what is gone: in the cold arm no tiktoken-init-* thread exists at all, so finalization has nothing to kill. Worth being explicit that counting clean runs would prove little here — the original failure rate was ~1%.

2. A consumer break, found by running the ApodexHarness suite against this branch with TIKTOKEN_CACHE_DIR pointed at an empty directory. tests/core/runtime/loop/test_tokenizer.py::test_first_call_is_nonblocking_even_if_load_is_slow monkeypatches a deliberately slow fake get_encoding and asserts the encoder eventually lands; the gate looks at the filesystem before sys.modules, so it now refuses a load that would never have touched the network. 1 failed, 8870 passed cold, and fully green warm (8871 passed; the one ERROR in both is a pre-existing env issue in that repo, unrelated). Now documented in the CHANGELOG with the fix for such tests: pin TIKTOKEN_CACHE_DIR at a directory holding any file, or set the opt-in env.

That also makes the point that this is invisible on a warm host — the same reason 0.8.1 looked complete.

zhanghanduo and others added 5 commits September 6, 2026 14:06
… hash

Two things the gate left unsaid.

**Which estimator the fallback is.** The two warnings, the module docstring
and the changelog all called it "chars/4". It is not:
`context_budget.estimate_tokens` counts each CJK character as a token and
divides only the non-CJK remainder by 4 — that replacement of the old flat
`len//4` is the whole subject of `tokens.py`'s docstring. The wording also
pointed the wrong way: "chars/4" reads as "CJK collapses to a quarter", and
a reader who knows the old 4x CJK under-count would conclude a Chinese
workload loses its context guard outright. Measured against cl100k_base:

    pure Chinese         0.84x of the real count
    mixed Chinese/Latin  0.88x
    English prose        1.14x
    JSON tool arguments  0.72x

It does err low on CJK, and lower on argument-heavy JSON, by 12-28% rather
than 4x — the guard's 1.5x buffer is what covers that. The changelog now
carries the numbers, because "approximate" alone leaves a host unable to
judge its own hard gateway limit.

**What proving a local load costs.** The gate SHA-256s the cached vocab
(~1.7 MB for cl100k_base, ~2 ms measured) on the calling thread, once per
encoding per process. This module's whole subject is what may and may not
run there, and 0.8.1 documented the 25 ms import for exactly that reason, so
the hash belongs in the same paragraph — including why tiktoken's own
constructor cannot do the verifying: calling it is the fetch being avoided.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_CACHE_SPECS` pins tiktoken's cache key and expected content hash, and
nothing could falsify either. Every existing test monkeypatches the table
and writes a payload whose hash it just computed, so a typo in either
constant — or a tiktoken release that re-publishes a vocab — would leave the
gate permanently shut with the whole suite green. The failure is silent by
construction: exact token counts quietly become heuristic ones, and the one
WARNING blames the host's cache for a directory that is correctly warmed.

The truth is readable without touching the network:
`tiktoken_ext.openai_public.cl100k_base`'s source carries the blobpath and
the `expected_hash` its loader validates against, and the cache key is
`sha1(blobpath)` per `read_file_cached`. Reading the source rather than
calling the constructor matters — calling it is the fetch this module
exists to avoid. The test loops `_CACHE_SPECS`, so a future encoding is
covered by adding it there. Negative-controlled: corrupting either pinned
constant by one character turns it red.

CI gets tiktoken for this one test through `--with`, deliberately not in
the shared environment. Installing it suite-wide turned
`test_agent_loop_engine.py::test_synthesised_zero_usage_does_not_reset_the_token_estimate`
red, because that test's arithmetic is calibrated on the heuristic: its
4000-char filler is ~1000 heuristic tokens and far fewer real ones. That
hazard now has a changelog paragraph of its own, since any host asserting on
token totals inherits it the moment a cache goes warm.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0.8.2 narrowed the Tier 1 card from 3 source URLs to 1 and told consumers
that a host needing several independent sources per claim "should now raise
`_MINI_CARD_MAX_URLS` deliberately". That knob did not exist: the constant is
module-private and read at import, so the only ways to get the old width back
were monkeypatching a private name or forking the compactor.

`KeepLastNToolResultsCompactor(max_card_urls=...)` threads through to
`_elided_tool_card(..., max_urls=...)`. The default is `_MINI_CARD_MAX_URLS`,
so nothing changes for a caller that does not pass it — the measured 99.2%
first-URL coverage stays the default, and the shim in MiroHarness needs no
edit (a new keyword argument on a public class is backward compatible).
Negative values raise at construction, alongside the existing
`keep_tool_result` guard; 0 is legal and drops the card's source line while
keeping the call and any argument URL.

Tests: 4 cases in tests/test_keep_last_n_compactor.py — the default is still
1, a raised value actually retains that many URLs while staying inside the
body budget, 0 drops the source line, and -1 is rejected.
@zhanghanduo
zhanghanduo merged commit a176407 into main Sep 6, 2026
5 checks passed
@zhanghanduo
zhanghanduo deleted the fix/tokenizer-cold-cache-fetch-gate branch September 6, 2026 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant