Skip to content

Robustify activation caching: rename navigation, hook lifecycle, membership semantics - #682

Open
Butanium wants to merge 13 commits into
devfrom
claude/pr-681-red-team-j583w4
Open

Robustify activation caching: rename navigation, hook lifecycle, membership semantics#682
Butanium wants to merge 13 commits into
devfrom
claude/pr-681-red-team-j583w4

Conversation

@Butanium

@Butanium Butanium commented Jul 8, 2026

Copy link
Copy Markdown
Member

Summary

Started as a fix for #535 (cache attribute access broken under collapsing/re-mount renames) and grew into a robustness pass over the activation-cache subsystem. Three headline changes:

  1. Cache navigation is structural and shares rename semantics with the envoy treefixes Unused renaming scheme make cache access via renamed name fail #535 and the whole class of string-matching bugs around it.
  2. Cache hooks are actually removed at trace exit — previously they leaked, and the next trace on the same model silently corrupted the old cache.
  3. Membership (in) and lookup ([]) share one resolution semantics — renamed spellings included.

1. Rename navigation (fixes #535)

Cache storage keys are always real module paths (dict-style access, keys(), iteration unchanged). Navigation resolves against a weights-free skeleton of the envoy tree:

  • Envoy._skeleton() snapshots the tree as PathNodes: real path, children by name, aliases resolved to their target nodes via Aliaser.resolve(envoy, alias) — the same function Envoy.__getattr__ uses, so cache and envoy cannot disagree about what an alias means. Nodes hold no modules: the skeleton pickles, travels with remote results, and keeps no weights alive.
  • CacheDict navigation is per-segment tree descent (PathNode.resolve, real children authoritative over aliases). Navigation refuses to descend into subtrees where nothing was cached, and error messages now distinguish "exists on the model but was never cached" from "no such module".
  • Every alias of a module navigates — cache.model.transformer.h[x], cache.model.model.layers[x], cache.model.layers[x], and secondary aliases of collapsing renames ({"model.layers": ["layers", "blocks"]}) all reach the same entry. test_cache_navigation_matches_envoy sweeps the spellings and asserts envoy/cache equivalence.
  • List-valued renames (documented Aliaser syntax) no longer crash tracer.cache(); dot-prefixed alias values ({".transformer.h": ".layers"}, also documented) now work on the envoy side too (Aliaser.build normalizes them).
  • Out-of-bounds indices on (renamed) module lists raise IndexError; index 1 can no longer string-prefix-match h.10. Old pickles (no skeleton fields) fall back to a storage-derived trie, as do CacheDicts constructed without a model.
  • Also fixed along the way: shared-mutable-default state across all CacheDicts in a process; the fixed cache.model root being corrupted by renames like {"model": "foo"}.

2. Hook lifecycle

Persistent cache hooks were never removed: dead mediators are pruned from interleaver.mediators (in __exit__ and check_dangling_mediators) before cancel() runs remove_hooks(), so the handles leaked onto the modules. Consequence: run a second model.trace(...) on the same model and the first trace's cache hooks fire again — entries silently become [Entry, Entry] lists and previously-working .output calls raise AttributeError: 'list' object has no attribute 'output'.

Hooks can't be removed the moment a mediator dies (vLLM re-enters the interleaver context per engine step and cache hooks must keep recording across steps), so pruned mediators' handles are retired to the interleaver and drained in cancel() — the documented single per-trace cleanup path. test_cache_hooks_removed_after_trace asserts hook counts return to baseline and a prior cache survives subsequent traces intact.

3. Membership = lookup

"model.layers.0" in cache was False while cache["model.layers.0"] succeeded. __contains__ now delegates to __getitem__, so in covers renamed spellings and sub-view-relative keys exactly like access does.

Tests

Nine new/extended tests in tests/test_lm.py::TestCache plus one in TestRename (dotted alias values): collapsing renames (incl. list-valued and secondary aliases), root protection (issue #535's original example), envoy/cache navigation equivalence sweep, membership/lookup agreement, state isolation, alias-never-shadows-real-key, old-pickle compatibility, and hook removal across traces.

tests/test_lm.py: 90 passed · test_tiny.py: 18 · test_envoys/test_source/test_iter_edge_cases/test_memory_cleanup/test_multiple_wrappers/test_transform/test_0516_features: 115 passed, 1 xpassed (all on dev base). All three repro snippets from the #535 thread verified. The branch was also adversarially reviewed by an independent agent (A/B behavioral comparison against dev on seven access patterns, pickle/deepcopy round-trips, multi-fire generation, swap renames); its findings are addressed in the later commits.

https://claude.ai/code/session_01GPKVtGmUQWFsS7dMEUsNNe

@Butanium
Butanium marked this pull request as ready for review July 8, 2026 02:54
claude added 5 commits July 8, 2026 02:56
Attribute-style access into a cache (e.g. `cache.model.layers[0].output`)
raised `AttributeError` when a rename re-mounts a nested module at the root
via a dotted rename key (e.g. `{"model.layers": "layers"}`). Two root causes:

- `CacheDict.__getattr__`/int-indexing only matched navigation paths against
  the real storage keys and the lossy inverted leaf-alias map, never the
  authoritative `_alias_paths` (alias-path -> real-key) map. A collapsing
  rename's alias path is not a prefix of any real key, so navigation dead-ended.
- `_add_alias_path` built alias paths with naive substring replacement, which
  also rewrote the fixed root component (`model.model.layers.0` ->
  `foo.foo.layers.0` instead of `model.foo.layers.0`).

Fixes:
- Navigate the `_alias_paths` keyspace as a fallback route in `__getattr__`
  and int-indexing; resolve a landed `_path` back to its real storage key in
  `output`/`inputs`/`input`.
- Protect the root component in `_add_alias_path`.
- Copy underlying storage into sub-views based on raw dict length, so an
  alias-space sub-view (scoped length 0) still propagates storage to children.

Adds regression tests for both the collapsing-rename and root-preservation
cases from the issue.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGecSNRudbfxSX14gsBco5
…ame cache tests

Follow-up to the #535 fix. Out-of-bounds indexing on a renamed/collapsed
modulelist (e.g. `cache.model.layers[999]`) leaked a `KeyError` instead of the
`IndexError` contract honored by non-renamed access, because the bounds check
only consulted the real storage keys. Extend it to the alias-path keyspace.

Broaden the collapsing-rename regression test to also cover `.input` via a
renamed path, out-of-bounds `IndexError`, and navigating into a submodule of a
re-mounted block under a full (no-`modules=`) cache.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGecSNRudbfxSX14gsBco5
…ecedence, segment-wise renames

Red-team follow-ups on the #535 fix:

- CacheDict.__init__: replace shared mutable default args with None
  defaults; every Cache previously shared one _alias_paths dict
  process-wide, leaking alias paths across caches now that navigation
  consults the map.
- Real storage keys are authoritative: __getitem__ checks storage
  before alias translation and _resolved_path() only consults
  _alias_paths for paths absent from storage, so a rename like
  {"a": "b", "b": "a"} can no longer shadow a genuinely cached module.
- _add_alias_path: whole-segment replacement instead of str.replace,
  so {"h": "layers"} no longer corrupts lm_head into lm_layersead and
  {"1": "x"} no longer mangles h.10/h.11/ln_1; handles list-valued
  aliases instead of crashing.
- Whole-segment prefix matching in __getattr__/__getitem__ so index 1
  no longer matches h.10 and returns a sub-view that raises KeyError.
- __getitem__ resolves sub-view-relative aliased keys
  (cache.model["layers.0"]) through the alias map.
- InterleavingTracer.cache: build the alias dict without assuming
  str-valued renames ({"transformer": ["model", "mdl"]} crashed).

Tests: extend collapsing-rename test with dict-style sub-view access
and strided IndexError cases; add list-rename, state-isolation, and
alias-shadowing tests. tests/test_lm.py: 80 passed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GPKVtGmUQWFsS7dMEUsNNe
Annotate the methods touched by the rename-navigation fix so the
str-path / segment-list / sub-view distinctions are visible in the
signatures. No behavior change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GPKVtGmUQWFsS7dMEUsNNe
Rename semantics previously existed twice: the Aliaser resolves aliases
against the live envoy tree, while the cache re-derived aliased paths by
applying the rename dict to path strings (_add_alias_path /
_replace_segments). The reimplementation could drift from real envoy
behavior — substring corruption and the list-valued-rename crash were
both symptoms.

Now Envoy._aliased_paths() walks the tree once at cache creation,
computing every module's user-facing path from the same Aliaser state
and fetch_attr resolution that answer aliased attribute access
(including chained renames and dotted re-mounts). tracer.cache() passes
the map to Cache, and Cache.add registers table entries for cached
modules from it. CacheDict loses _add_alias_path, _replace_segments,
and its _rename attribute; navigation (three routes, real-key
precedence, whole-segment matching) is unchanged and operates on the
same alias-path table as before.

Update the CacheDict unit tests for the new construction, assert
envoy/cache path agreement in the collapsing-rename test, and fix the
cache section of docs/usage/rename-modules.md (its example also wrongly
omitted the cache.model root).

tests/test_lm.py: 80 passed; issue #535 repros verified.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GPKVtGmUQWFsS7dMEUsNNe
@Butanium
Butanium force-pushed the claude/pr-681-red-team-j583w4 branch from 949b40d to 8b8887b Compare July 8, 2026 02:57
@Butanium
Butanium changed the base branch from main to dev July 8, 2026 02:57
claude added 8 commits July 8, 2026 03:19
Replace string-matching cache navigation with structural navigation.
Envoy._skeleton() snapshots the envoy tree as PathNode objects — each
node carries its real path, children by name, and aliases resolved to
their target nodes via the same Aliaser state and fetch_attr resolution
that answer envoy attribute access. Nodes hold no modules, so the
skeleton is picklable and keeps no weights alive.

CacheDict navigation becomes plain tree descent: one resolve() per
segment, real children authoritative over aliases. This deletes the
three-route __getattr__, the alias-path table, _resolved_path, and
Envoy._aliased_paths — the whole string-rewriting/prefix-matching layer
— and fixes what it structurally couldn't express:

- every alias of a module navigates, including secondary aliases of
  collapsing renames ({"model.layers": ["layers", "blocks"]});
- "exists on the model but was never cached" is now distinguishable
  from "no such module" in AttributeError messages;
- whole-segment matching and real-key precedence hold by construction
  instead of by careful string comparisons.

Dict-style access, keys()/items()/iteration scoping, IndexError
semantics, and keys(alias=True) (now computed from the skeleton) are
preserved. CacheDicts constructed without a skeleton derive a bare trie
from their storage keys, so real-path navigation still works.

tests/test_lm.py: 86 passed (adds test_cache_collapsing_list_rename);
issue #535 repros verified.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GPKVtGmUQWFsS7dMEUsNNe
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GPKVtGmUQWFsS7dMEUsNNe
Envoy.__getattr__ and Envoy._skeleton's link pass now call the same
Aliaser.resolve(envoy, alias) — one function decides what an alias
means, so cache navigation cannot disagree with envoy attribute access.

Add test_cache_navigation_matches_envoy: for a chained/collapsing
rename dict, every spelling of a module path (real names, leaf aliases,
re-mounts, and mixes) resolves to the same envoy on the model and the
same entry on the cache.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GPKVtGmUQWFsS7dMEUsNNe
Repo convention: no annotations on untouched code. output/inputs/input
and _scoped_iter are back to their pre-PR bodies, so their added return
types go too.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GPKVtGmUQWFsS7dMEUsNNe
…ias values

From an independent review pass over the branch:

- Rebuild the storage-derived fallback trie when keys were added after
  it was first derived, so skeleton-less CacheDicts don't go stale.
- __setstate__ defaults the skeleton fields, so caches pickled by
  versions without them fall back to the derived trie instead of
  raising AttributeError on any attribute access.
- Aliaser.build normalizes dot-prefixed alias values ({".transformer.h":
  ".layers"} is documented), fixing the one envoy/cache divergence:
  the cache resolved the alias but envoy attribute access didn't.
- Test keys(alias=True) and note _skeleton is meant for the root envoy.

tests/test_lm.py: 89 passed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GPKVtGmUQWFsS7dMEUsNNe
'model.layers.0' in cache was False while cache['model.layers.0']
succeeded. __contains__ now delegates to __getitem__, so membership
covers renamed spellings and sub-view-relative keys exactly like
access does.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GPKVtGmUQWFsS7dMEUsNNe
Dead mediators were pruned from interleaver.mediators (in __exit__ and
check_dangling_mediators) before cancel() could call their
remove_hooks(), so persistent cache hooks survived the trace. The next
trace on the same model re-fired them into the old cache: entries
silently became [Entry, Entry] lists and .output raised
AttributeError.

Hooks cannot be removed the moment a mediator dies — vLLM re-enters
the interleaver context per engine step and cache hooks must keep
recording — so pruned mediators' handles are retired to the
interleaver and drained in cancel(), the single per-trace cleanup
path.

Adds test_cache_hooks_removed_after_trace: hook count returns to
baseline and a prior cache survives a subsequent trace intact.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GPKVtGmUQWFsS7dMEUsNNe
@Butanium Butanium changed the title Fix cache navigation through collapsing/re-mount renames Robustify activation caching: rename navigation, hook lifecycle, membership semantics Jul 8, 2026
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.

2 participants