Skip to content

Bound SignatureCache growth with a key-length cap and a bounded FIFO miss cache (APP-5431) - #15181

Merged
acarl005 merged 7 commits into
masterfrom
factory/app-5431-signature-cache-memory
Aug 18, 2026
Merged

Bound SignatureCache growth with a key-length cap and a bounded FIFO miss cache (APP-5431)#15181
acarl005 merged 7 commits into
masterfrom
factory/app-5431-signature-cache-memory

Conversation

@warp-agent-staging

@warp-agent-staging warp-agent-staging Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Description

SignatureCache::get (crates/warp_completer/src/signatures/legacy/registry.rs) caches command lookups in memo_map::MemoMap, which is append-only by design (no eviction, size cap, or TTL — see its own doc comment).

The completer looks up an arbitrary token taken from the terminal input line on every keystroke/paste (Input::generate_autosuggestion_asyncclassify_commandparse_commandCommandRegistry::signature_from_tokens/signature_with_alias_expansionSignatureCache::get). A correction to the linked Sentry analysis: the cache key is not the full buffer text, it's a single token (tokens.first(), or a later token via maybe_load_replacement_signature). The leak had two independent causes:

  1. A single pathologically large token (e.g. pasting a large blob of text into the terminal input line) got lowercased, cloned, and permanently retained.
  2. Every distinct token that ever missed the cache (not just huge ones) also got a permanent None entry — so the cache also grew without bound purely from the count of distinct short tokens seen over a session.

Linear: https://linear.app/warpdotdev/issue/APP-5431/unbounded-memory-growth-warp-completer-signaturecache-memomap-caches
Sentry: https://warpdotdev.sentry.io/issues/7259255054/

Fix

SignatureCache is split into two structures with different bounds and different eviction guarantees:

  • signatures: MemoMap<String, Signature> — unchanged in shape. Only lookups that actually resolve to a signature are ever inserted. It stays append-only and keeps handing back Option<&Signature> borrowed from inside the map, which is what lets SignatureAtTokenIndex hold a &'a Signature across the parser with no API change. This bounds it by the number of distinct command names that can ever resolve to something — the fixed embedded corpus (1,167 top-level commands as of this writing) plus any explicitly register_signature-ed names (a handful of Warp CLI channel names) — rather than by every distinct token a user has ever typed or pasted.
  • misses: MissCache — new. MissCache lives in its own module, crates/warp_completer/src/signatures/legacy/miss_cache.rs, beside registry.rs. It's a bounded, RwLock-guarded FIFO set (capacity MAX_CACHED_MISSES = 256, defined in that module) holding only the lowercased tokens that recently failed to resolve, no signatures. Because a lookup against it only ever returns a bool, no reference escapes it, so eviction is sound — unlike signatures, there's no outstanding borrow an eviction could invalidate. Internally it's a VecDeque (insertion order, for FIFO eviction) plus a HashSet (for O(1) membership checks), both behind one RwLock.
  • Why a RwLock, not the Mutex from the prior revision: CommandRegistry::global_instance() is a single Arc shared across every terminal session/pane, each generating completions on its own background-thread-pool task, so this genuinely needs cross-thread synchronization — it's not effectively single-threaded. But the negative cache is inherently approximate: a false negative (a miss it forgot) costs one extra, cheap lookup_fn call, never a wrong answer — a much weaker bar than an LRU's guarantees require. Dropping strict LRU recency for plain FIFO eviction is what unlocks the RwLock: a hit against contains is now a pure read (shared read-lock, no mutation), and the write lock is only taken for a genuinely new miss. An LRU would need a write lock on every hit too (to move the entry to the front), which is exactly the shape of lock a RwLock doesn't help with — that's why a plain RwLock wasn't an option before this trade. The cost: a miss that's looked up again while the cache is full doesn't get a second life; it's evicted on schedule regardless. This also removes the lru crate as a dependency.
  • Capacity (256) is picked to comfortably absorb a burst of misses from a single interaction — e.g. a user retrying a mistyped command letter-by-letter, where each partial token is a distinct miss — without holding onto misses for the life of a long session.
  • For tokens longer than MAX_CACHEABLE_COMMAND_LEN (255 bytes — the common filesystem NAME_MAX), get now just returns None outright, before the lowercase allocation and before touching either cache. The previous revision instead fell back to a linear scan of signatures for this case, to keep an oversized register_signature-ed name retrievable. That scan is now gone: walking the full embedded corpus (recursively, including subcommands) plus the Warp CLI's own clap-derived command tree — the only other source of register_signature-ed names in production — the longest name found is 59 characters (a compound dig/dog subcommand name), well under the cap. test_all_known_signature_names_are_within_the_length_cap establishes and locks in that invariant; an artificially oversized register_signature-ed name is now unresolvable via lookup, a deliberate, tested tradeoff rather than a case that occurs today.

What the negative cache buys, honestly stated: a miss is already cheap to redo — lookup_fn probes the embedded, already-compiled signature corpus (rust-embed stores file contents statically and does a binary search — no filesystem I/O, no JSON parsing on a miss; JSON parsing only happens once we know there's a hit). So misses is a bounded convenience that saves a handful of redundant lookups for a token retried in quick succession (e.g. while a user is correcting a typo), not a fix for an expensive-recompute problem.

Alternatives considered and rejected

  • A unified LRU over signatures itself, returning Option<Arc<Signature>> instead of Option<&Signature>. An earlier design for this revision, since it would let one bounded structure serve both hits and misses. Rejected by the requester: it changes SignatureCache::get's (and thus CommandRegistry::signature/signature_from_tokens/etc.'s) return type, which rev-locks callers like SignatureAtTokenIndex into holding an owned handle instead of a borrow — a bigger, more invasive change than this leak warrants.
  • Cloning Signature on every lookup to allow eviction from a single LRU. Would turn a cheap reference return into a clone on every completion keystroke, a hot-path regression.
  • A Mutex<lru::LruCache<..>> for the negative cache. The design in the immediately prior revision of this PR. Rejected by the requester in favor of the RwLock-guarded FIFO described above, once we'd confirmed the registry is genuinely multi-threaded (ruling out thread-local state) and that the cache's approximate nature makes strict LRU recency unnecessary.
  • A fixed-size array of atomics holding token hashes, with no key strings retained. Considered for the negative cache but rejected: without the key, a hash collision would make a real, resolvable command permanently look like a known miss to that slot, silently dropping its completions. The HashSet-based design pays a little more memory for a set of ~256 short strings to avoid that failure mode entirely.
  • Keying on the parsed command-name token only. This is effectively what already happens — the cache is already keyed on a single token, not the raw buffer (see the correction above).
  • Rejecting long tokens outright instead of falling back to a scan. This is what the current revision does, now that the corpus-scan test establishes it's safe; see "Fix" above.

Other unbounded/user-input-keyed caches in the completer pipeline

Swept the rest of warp_completer for the same shape (an append-only or ever-growing cache keyed on user input):

  • CommandRegistry::dynamic_completion_data (CaseInsensitiveHashMap) is built once from a fixed HashMap at CommandRegistry::new and never written after construction — confirmed not a second leak.
  • No other MemoMap, HashMap, or similar cache keyed on raw user/terminal input was found elsewhere in crates/warp_completer.

Linked Issue

  • N/A — tracked in Linear (APP-5431), not a GitHub issue.

Testing

MissCache now has its own module and companion test file, matching how registry.rs/registry_tests.rs are wired:

  • crates/warp_completer/src/signatures/legacy/miss_cache.rs — the MissCache type, MissCacheEntries, and MAX_CACHED_MISSES.
  • crates/warp_completer/src/signatures/legacy/miss_cache_tests.rs — direct unit tests against MissCache (no CommandRegistry involved): test_contains_reflects_recorded_misses, test_inserting_an_existing_entry_is_a_no_op, test_stops_growing_at_capacity, test_evicts_in_fifo_order_regardless_of_lookups (the FIFO-vs-LRU regression test, re-touching the oldest entry before overflow and asserting it's evicted anyway).

Unit tests remaining in crates/warp_completer/src/signatures/legacy/registry_tests.rs (these exercise SignatureCache/CommandRegistry behavior; the two oversized-token tests touch misses only to confirm the length guard never reaches it):

  • test_all_known_signature_names_are_within_the_length_cap — walks every embedded signature (recursively, including subcommands) plus the Warp CLI's clap-derived command tree, asserting every name is within MAX_CACHEABLE_COMMAND_LEN. This is the invariant the oversized-token fast path in get depends on; if a future signature ever violates it, this test fails instead of that name silently becoming unresolvable.
  • test_oversized_command_is_not_cached_and_resolves_to_none — an oversized, unregistered token resolves to None without growing either the positive or the negative cache.
  • test_oversized_later_token_does_not_bypass_the_length_guard — an oversized later token (e.g. the argument to sudo, resolved via maybe_load_replacement_signature) takes the same length-guarded path as an oversized top-level token, via both signature_from_tokens and signature_with_alias_expansion.
  • test_misses_are_never_cached_in_the_positive_cache — a normal-length command that never resolves to a signature never adds an entry to signatures, however many times it's looked up.
  • test_ordinary_commands_still_resolve_and_are_cached — registered commands still resolve case-insensitively; repeated lookups hit the positive cache rather than growing it.
  • test_registered_signature_longer_than_the_cap_is_unresolvable — a signature explicitly registered with an artificially oversized name is not retrievable via signature(), documenting the accepted tradeoff from removing the fallback scan.
  • test_exe_suffix_is_trimmed_before_the_length_check (Windows-only) — a command name exactly at the cap resolves correctly when looked up with .exe appended, proving the trim happens before the length check.
  • test_registered_commands_unaffected_by_oversized_lookupsCommandRegistry::registered_commands() is unaffected by oversized/negative lookups.

Validation run against crates/warp_completer:

  • ./script/format — clean.

  • cargo clippy -p warp_completer --all-targets --tests -- -D warnings — clean (default features, the legacy code path this PR touches).

  • cargo clippy -p warp_completer --all-targets --all-features --tests -- -D warnings — fails with the same 5 pre-existing collapsible_if/let_and_return errors in completer/engine/argument/v2.rs, re-confirmed to reproduce identically on unmodified master; unrelated to this change.

  • cargo nextest run -p warp_completer — 19 registry tests + 4 miss_cache tests pass. The same 25 pre-existing, unrelated failures reproduce identically on unmodified master (Tried to check FeatureFlag::CloudEnvironments before feature flags were initialized, in completer::describe/completer::engine/completer::suggest tests) — 149/174 total tests pass, matching the pre-existing baseline plus the new tests.

  • cargo check -p warp_completer --features v2 — clean; the v2 registry implementation is a separate, untouched module.

  • cargo check -p warp_terminal -p input_classifier -p warp_tui (dependents) — all build clean. SignatureCache's public-facing return types are unchanged (still Option<&Signature>).

  • Removed lru as a dependency of warp_completer (no longer needed); MissCache is built on std::collections::{HashSet, VecDeque} and std::sync::RwLock only.

  • Could not cross-compile-check the Windows-only test in this Linux sandbox (no Windows target/toolchain available); the ordering it tests (trim .exe before the length check) is unchanged from the previous revision.

  • I have manually tested my changes locally (unit tests + the crate-scoped validation above; this is not a UI change so ./script/run manual testing doesn't apply).

Agent Mode

  • Warp Agent Mode - This PR was created via Warp's AI Agent Mode

CHANGELOG-BUG-FIX: Fixed an unbounded memory leak in the terminal completions engine where large or unusual input (e.g. pasting a large blob of text) could permanently grow memory usage over a session.

Co-Authored-By: Warp agent@warp.dev

)

SignatureCache::get caches every distinct command token it is ever asked
to look up in an append-only memo_map::MemoMap, with no eviction, size
cap, or TTL. Since the completer looks up an arbitrary token from the
terminal input line on every keystroke/paste (see
CommandRegistry::signature_from_tokens and friends), a single
pathologically large token (e.g. a large blob of text pasted into the
terminal input) would be cloned into the cache and retained for the
lifetime of the process.

Fix: skip both the lookup and the cache entirely for tokens longer than
MAX_CACHEABLE_COMMAND_LEN (255, the common filesystem NAME_MAX). No real
command name can exceed this, so this cannot drop a legitimate lookup,
and it stops a single huge token from permanently growing the cache.

A true bounded/LRU cache was rejected: SignatureCache::get returns
Option<&Signature> borrowed from inside the map, and those references are
held across the parser (see SignatureAtTokenIndex), which the MemoMap's
append-only design exists specifically to support. Evicting entries while
references to them may be live would not compile; making it compile by
cloning Signature on every lookup would regress a hot path.

Co-Authored-By: Warp <agent@warp.dev>
@cla-bot cla-bot Bot added the cla-signed label Aug 15, 2026
@warp-agent-staging warp-agent-staging Bot added factory:wilson area:performance:memory Memory usage, allocation, leaks, and memory-bound performance. labels Aug 15, 2026
@warp-agent-staging
warp-agent-staging Bot requested a review from acarl005 August 15, 2026 12:29
warp-agent-staging Bot and others added 2 commits August 15, 2026 12:53
Address three review findings on the initial APP-5431 fix:

1. The length cap alone didn't bound the cache: every distinct <=255-byte
   miss still permanently stored a None entry. SignatureCache now only
   caches successful lookups (MemoMap<String, Signature>); a miss is
   never memoized. lookup_fn only probes the embedded, already-compiled
   signature corpus (a cheap binary search, per rust-embed >=8.0's
   "store file contents statically and use binary search" design) --
   JSON parsing only happens on a hit -- so redoing a miss on every
   keystroke is cheap, unlike memoizing it forever.

2. An explicitly insert()-ed signature whose name exceeds the cap would
   become unretrievable through get()/signature() while still being
   listed by registered_commands(). get() now falls back to a cheap,
   allocation-free linear scan (length-prechecked, so still cheap even
   for a huge token) for oversized tokens, so such a signature stays
   retrievable.

3. Added a regression test that exercises the fix for finding 2, and
   rewrote the .exe-trimming test to use a name exactly at the cap so it
   actually exercises trim-then-check ordering (the previous version
   passed regardless of ordering).

registered_commands() no longer needs to filter out negative-cache
entries, since every cache entry is now a real signature by construction.

Co-Authored-By: Warp <agent@warp.dev>
…only map (APP-5431)

The requester asked to keep the key-length cap from the previous revision,
but bound the cache with a true LRU instead of relying on "the set of
resolvable command names is finite". A follow-up from the requester then
changed direction away from an `Arc<Signature>`-based unified LRU (which
would have required `SignatureCache::get` to hand back an owned handle
instead of `Option<&Signature>`) towards keeping `signatures` untouched
and adding a second, bounded structure for misses only:

- `signatures: MemoMap<String, Signature>` is unchanged -- still
  append-only, still handing back `Option<&Signature>` borrowed from
  inside the map, so `SignatureAtTokenIndex`'s `&'a Signature` keeps
  working with no API change. It stays bounded by the number of distinct
  command names that can ever resolve to something (the embedded corpus
  of 1,167 top-level commands, plus explicitly `register_signature`-ed
  names), not by every token a user has ever typed.
- `misses: Mutex<LruCache<String, ()>>` is new: a bounded LRU set of
  tokens that recently failed to resolve. Because `get` only ever
  returns a `bool` from this set (no reference escapes), evicting from
  it is sound -- unlike `signatures`, there's no outstanding borrow that
  eviction could invalidate. Capacity is `MAX_CACHED_MISSES = 256`.
- The existing `MAX_CACHEABLE_COMMAND_LEN` length cap now also gates the
  negative cache: an oversized token skips both `signatures` and
  `misses` entirely, so the leak this PR exists to fix can't just move
  into the new negative cache.

Uses the `lru` crate, which was already vendored transitively (via
`ratatui-core`/`tantivy`) at 0.18.0, rather than adding a new dependency
version.

Co-Authored-By: Warp <agent@warp.dev>
@warp-agent-staging warp-agent-staging Bot changed the title Bound SignatureCache key length to fix unbounded memory growth (APP-5431) Bound SignatureCache growth with a key-length cap and a bounded LRU miss cache (APP-5431) Aug 18, 2026
warp-agent-staging Bot and others added 2 commits August 18, 2026 01:15
- Add a regression test proving an oversized *later* token (e.g. the
  argument to `sudo`, resolved via `maybe_load_replacement_signature`)
  takes the same length-guarded path as an oversized top-level token.
  `self.signatures` there is `CommandRegistry`'s `SignatureCache` field,
  not the raw `MemoMap`, so it already routed through the guarded
  `SignatureCache::get` -- this locks that guarantee in with a test
  covering both `signature_from_tokens` and `signature_with_alias_expansion`.
- Fix `test_negative_cache_evicts_in_lru_order`, which previously only
  re-touched every entry except the oldest and then asserted the oldest
  was evicted -- indistinguishable from FIFO eviction. It now re-touches
  only the oldest entry before overflowing and asserts that entry
  survives while the next-oldest (now genuinely least-recently-used) is
  evicted instead, which a FIFO or arbitrary-eviction cache would fail.
- Fix the stale `register_signature` doc comment, which still described
  the old "cached `None` blocks registration" behavior from before
  misses were split into their own bounded structure.

Co-Authored-By: Warp <agent@warp.dev>
… a FIFO RwLock set

Two follow-ups from the requester:

1. "Don't we know for sure none of our signatures have command names
   longer than MAX_CACHEABLE_COMMAND_LEN?" -- measured it rather than
   assuming: walked every embedded signature (recursively, including
   subcommands) plus the Warp CLI's own clap-derived command tree (the
   only other source of `register_signature`-ed names in production).
   The longest name found is 59 characters (a compound dig/dog
   subcommand name), well under the 255-byte cap. Added
   `test_all_known_signature_names_are_within_the_length_cap`, which
   walks both sources and fails loudly if a future signature ever
   violates this. On the strength of that invariant, `SignatureCache::get`
   now just returns `None` for an oversized token instead of falling
   back to a linear scan -- simpler, and sheds the "compare length
   before case-folding" machinery the scan needed. An explicitly
   `register_signature`-ed signature with an artificially oversized
   name would become unresolvable via lookup; that's a deliberate,
   tested tradeoff, not a real-world regression.

2. "I don't like the Mutex on the cached misses. can we achieve
   interior mutability some other way?" -- first checked whether
   `SignatureCache` is genuinely shared across threads: yes,
   `CommandRegistry::global_instance()` is a single `Arc` shared across
   every terminal session/pane, each generating completions on a
   background thread pool, so this needs real cross-thread
   synchronization, not just single-task interior mutability. But a
   negative cache is inherently approximate -- a false negative just
   costs one extra, cheap `lookup_fn` call, never a wrong answer -- so
   an LRU's strict recency tracking (which needs a write lock on every
   *hit*, to move the entry to the front) is more machinery than the
   requirement calls for. Replaced `Mutex<lru::LruCache<..>>` with a
   hand-rolled `MissCache`: a `RwLock`-guarded FIFO (a `VecDeque` for
   insertion order plus a `HashSet` for O(1) membership checks). A hit
   is now a pure read (shared read-lock, no mutation), and the write
   lock is only taken for a genuinely new miss. This also drops the
   `lru` crate as a dependency entirely. The tradeoff: eviction order is
   now FIFO instead of LRU, so a miss that's looked up again doesn't get
   a second life once the cache is full -- an accepted loss of precision
   for a cache that's explicitly a convenience, not a correctness
   mechanism.

Rewrote `test_negative_cache_evicts_in_lru_order` (which didn't
actually distinguish LRU from FIFO) into
`test_negative_cache_evicts_in_fifo_order`, replaced
`test_registered_signature_longer_than_the_cap_still_resolves` with
`test_registered_signature_longer_than_the_cap_is_unresolvable` to
match the new behavior, and updated the PR-touched doc comments
throughout to describe the current design.

Co-Authored-By: Warp <agent@warp.dev>
@warp-agent-staging warp-agent-staging Bot changed the title Bound SignatureCache growth with a key-length cap and a bounded LRU miss cache (APP-5431) Bound SignatureCache growth with a key-length cap and a bounded FIFO miss cache (APP-5431) Aug 18, 2026
Reviewer request, mechanical only, no behavior change:

- `MissCache` (and `MissCacheEntries`) now live in their own
  `signatures/legacy/miss_cache.rs` module beside `registry.rs`, with a
  companion `miss_cache_tests.rs` wired the same way `registry.rs` wires
  `registry_tests.rs`. `MAX_CACHED_MISSES` moved with it; registry.rs no
  longer references it at all, since `MissCache` now exposes a `Default`
  impl that uses it internally. `MAX_CACHEABLE_COMMAND_LEN` stays in
  `registry.rs`, next to `SignatureCache`.
- The `MissCache`-specific tests (capacity bound, FIFO eviction order)
  moved out of `registry_tests.rs` into `miss_cache_tests.rs`, rewritten
  as direct unit tests against `MissCache` itself rather than driven
  through `CommandRegistry::signature()`. The tests that exercise
  `SignatureCache`/`CommandRegistry` behavior and only touch
  miss-caching indirectly (the two oversized-token tests, which check
  `misses.len()` before/after to confirm the length guard never touches
  it) stayed in `registry_tests.rs`.
- Deleted the doc comments on `MAX_CACHEABLE_COMMAND_LEN` and (now
  moved) `MAX_CACHED_MISSES`. Two comments elsewhere pointed at
  `MAX_CACHEABLE_COMMAND_LEN`'s doc comment specifically; both had that
  parenthetical dropped rather than having the deleted prose relocated
  into them. `MissCache`'s own type-level doc comment (why it's a
  `RwLock`-guarded FIFO) is unaffected -- it's not a const comment, and
  it moved with the type.
- Visibility kept as tight as the split allows: `MissCache` and the
  methods `registry.rs`/its tests need are `pub(super)` (visible within
  `legacy` and its descendants only); `MissCache::new` and the
  `MissCacheEntries` fields stay private to the new module.

Co-Authored-By: Warp <agent@warp.dev>
@acarl005
acarl005 marked this pull request as ready for review August 18, 2026 22:11
@acarl005 acarl005 self-assigned this Aug 18, 2026

@acarl005 acarl005 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trim down those comments as i request

Comment thread crates/warp_completer/src/signatures/legacy/registry.rs Outdated
Comment thread crates/warp_completer/src/signatures/legacy/registry.rs Outdated
Reviewer requests, comments-only, no behavior change:

- Removed every comment (block-above and inline) from the tests this
  PR introduced: in registry_tests.rs, the length-cap invariant test,
  both oversized-token tests, the never-cache-a-miss test, the
  ordinary-commands test, the registered-signature-longer-than-the-cap
  test, the .exe-trim-ordering test, and the registered_commands test;
  in miss_cache_tests.rs, all four MissCache tests. Tests that predate
  this PR are untouched. Where a removed comment was carrying real
  information, the test name already said it (no renames needed); for
  the length-cap invariant test specifically, folded what the comment
  explained -- what a failure means and what to do about it -- into
  the `assert!` message instead.
- Applied two suggested edits from the GitHub review, verbatim: the
  `signatures` field doc comment in registry.rs now ends at "...as the
  map internally is an append-only structure)."; the `misses` field
  doc comment collapses to two lines stating what it is, dropping the
  `MissCache` pointer, the eviction-soundness reasoning, and the
  bounded-convenience explanation.
- On my own initiative (not requested, but the same trend as the two
  review comments): trimmed `MissCache`'s own type doc comment in
  miss_cache.rs from ~15 lines to 3, stating what it is and why it's a
  FIFO behind a `RwLock` rather than an LRU behind a `Mutex`, without
  absorbing any of the text removed from registry.rs.

Co-Authored-By: Warp <agent@warp.dev>
@acarl005
acarl005 enabled auto-merge (squash) August 18, 2026 22:29
@acarl005
acarl005 merged commit 213c9b3 into master Aug 18, 2026
44 of 46 checks passed
@acarl005
acarl005 deleted the factory/app-5431-signature-cache-memory branch August 18, 2026 23:08
iamwavecut pushed a commit to iamwavecut/warp that referenced this pull request Aug 20, 2026
…miss cache (APP-5431) (warpdotdev#15181)

## Description
`SignatureCache::get`
(`crates/warp_completer/src/signatures/legacy/registry.rs`) caches
command lookups in `memo_map::MemoMap`, which is append-only by design
(no eviction, size cap, or TTL — see its own doc comment).

The completer looks up an arbitrary token taken from the terminal input
line on every keystroke/paste (`Input::generate_autosuggestion_async` →
`classify_command` → `parse_command` →
`CommandRegistry::signature_from_tokens`/`signature_with_alias_expansion`
→ `SignatureCache::get`). A correction to the linked Sentry analysis:
the cache key is not the full buffer text, it's a single *token*
(`tokens.first()`, or a later token via
`maybe_load_replacement_signature`). The leak had two independent
causes:
1. A single pathologically large token (e.g. pasting a large blob of
text into the terminal input line) got lowercased, cloned, and
permanently retained.
2. Every distinct token that ever missed the cache (not just huge ones)
also got a permanent `None` entry — so the cache also grew without bound
purely from the *count* of distinct short tokens seen over a session.

Linear:
https://linear.app/warpdotdev/issue/APP-5431/unbounded-memory-growth-warp-completer-signaturecache-memomap-caches
Sentry: https://warpdotdev.sentry.io/issues/7259255054/

### Fix
`SignatureCache` is split into two structures with different bounds and
different eviction guarantees:
- `signatures: MemoMap<String, Signature>` — unchanged in shape. Only
lookups that actually resolve to a signature are ever inserted. It stays
append-only and keeps handing back `Option<&Signature>` borrowed from
inside the map, which is what lets `SignatureAtTokenIndex` hold a `&'a
Signature` across the parser with no API change. This bounds it by the
number of distinct command names that can ever resolve to something —
the fixed embedded corpus (**1,167** top-level commands as of this
writing) plus any explicitly `register_signature`-ed names (a handful of
Warp CLI channel names) — rather than by every distinct token a user has
ever typed or pasted.
- `misses: MissCache` — new. `MissCache` lives in its own module,
`crates/warp_completer/src/signatures/legacy/miss_cache.rs`, beside
`registry.rs`. It's a bounded, `RwLock`-guarded FIFO set (capacity
`MAX_CACHED_MISSES = 256`, defined in that module) holding only the
lowercased tokens that recently failed to resolve, no signatures.
Because a lookup against it only ever returns a `bool`, no reference
escapes it, so eviction is sound — unlike `signatures`, there's no
outstanding borrow an eviction could invalidate. Internally it's a
`VecDeque` (insertion order, for FIFO eviction) plus a `HashSet` (for
O(1) membership checks), both behind one `RwLock`.
- **Why a `RwLock`, not the `Mutex` from the prior revision:**
`CommandRegistry::global_instance()` is a single `Arc` shared across
every terminal session/pane, each generating completions on its own
background-thread-pool task, so this genuinely needs cross-thread
synchronization — it's not effectively single-threaded. But the negative
cache is inherently approximate: a false negative (a miss it forgot)
costs one extra, cheap `lookup_fn` call, never a wrong answer — a much
weaker bar than an LRU's guarantees require. Dropping strict LRU recency
for plain FIFO eviction is what unlocks the `RwLock`: a *hit* against
`contains` is now a pure read (shared read-lock, no mutation), and the
write lock is only taken for a genuinely new miss. An LRU would need a
write lock on every hit too (to move the entry to the front), which is
exactly the shape of lock a `RwLock` doesn't help with — that's why a
plain `RwLock` wasn't an option before this trade. The cost: a miss
that's looked up again while the cache is full doesn't get a second
life; it's evicted on schedule regardless. This also removes the `lru`
crate as a dependency.
- Capacity (`256`) is picked to comfortably absorb a burst of misses
from a single interaction — e.g. a user retrying a mistyped command
letter-by-letter, where each partial token is a distinct miss — without
holding onto misses for the life of a long session.
- For tokens longer than `MAX_CACHEABLE_COMMAND_LEN` (255 bytes — the
common filesystem `NAME_MAX`), `get` now just returns `None` outright,
before the lowercase allocation and before touching either cache. The
previous revision instead fell back to a linear scan of `signatures` for
this case, to keep an oversized `register_signature`-ed name
retrievable. That scan is now gone: walking the full embedded corpus
(recursively, including subcommands) plus the Warp CLI's own
clap-derived command tree — the only other source of
`register_signature`-ed names in production — the longest name found is
**59 characters** (a compound `dig`/`dog` subcommand name), well under
the cap. `test_all_known_signature_names_are_within_the_length_cap`
establishes and locks in that invariant; an artificially oversized
`register_signature`-ed name is now unresolvable via lookup, a
deliberate, tested tradeoff rather than a case that occurs today.

**What the negative cache buys, honestly stated:** a miss is already
cheap to redo — `lookup_fn` probes the embedded, already-compiled
signature corpus (`rust-embed` stores file contents statically and does
a binary search — no filesystem I/O, no JSON parsing on a miss; JSON
parsing only happens once we know there's a hit). So `misses` is a
bounded convenience that saves a handful of redundant lookups for a
token retried in quick succession (e.g. while a user is correcting a
typo), not a fix for an expensive-recompute problem.

### Alternatives considered and rejected
- **A unified LRU over `signatures` itself, returning
`Option<Arc<Signature>>` instead of `Option<&Signature>`.** An earlier
design for this revision, since it would let one bounded structure serve
both hits and misses. Rejected by the requester: it changes
`SignatureCache::get`'s (and thus
`CommandRegistry::signature`/`signature_from_tokens`/etc.'s) return
type, which rev-locks callers like `SignatureAtTokenIndex` into holding
an owned handle instead of a borrow — a bigger, more invasive change
than this leak warrants.
- **Cloning `Signature` on every lookup to allow eviction from a single
LRU.** Would turn a cheap reference return into a clone on every
completion keystroke, a hot-path regression.
- **A `Mutex<lru::LruCache<..>>` for the negative cache.** The design in
the immediately prior revision of this PR. Rejected by the requester in
favor of the `RwLock`-guarded FIFO described above, once we'd confirmed
the registry is genuinely multi-threaded (ruling out thread-local state)
and that the cache's approximate nature makes strict LRU recency
unnecessary.
- **A fixed-size array of atomics holding token hashes, with no key
strings retained.** Considered for the negative cache but rejected:
without the key, a hash collision would make a real, resolvable command
permanently look like a known miss to that slot, silently dropping its
completions. The `HashSet`-based design pays a little more memory for a
set of ~256 short strings to avoid that failure mode entirely.
- **Keying on the parsed command-name token only.** This is effectively
what already happens — the cache is already keyed on a single token, not
the raw buffer (see the correction above).
- **Rejecting long tokens outright instead of falling back to a scan.**
This is what the current revision does, now that the corpus-scan test
establishes it's safe; see "Fix" above.

### Other unbounded/user-input-keyed caches in the completer pipeline
Swept the rest of `warp_completer` for the same shape (an append-only or
ever-growing cache keyed on user input):
- `CommandRegistry::dynamic_completion_data` (`CaseInsensitiveHashMap`)
is built once from a fixed `HashMap` at `CommandRegistry::new` and never
written after construction — confirmed not a second leak.
- No other `MemoMap`, `HashMap`, or similar cache keyed on raw
user/terminal input was found elsewhere in `crates/warp_completer`.

## Linked Issue
- [x] N/A — tracked in Linear (APP-5431), not a GitHub issue.

## Testing
`MissCache` now has its own module and companion test file, matching how
`registry.rs`/`registry_tests.rs` are wired:
- `crates/warp_completer/src/signatures/legacy/miss_cache.rs` — the
`MissCache` type, `MissCacheEntries`, and `MAX_CACHED_MISSES`.
- `crates/warp_completer/src/signatures/legacy/miss_cache_tests.rs` —
direct unit tests against `MissCache` (no `CommandRegistry` involved):
`test_contains_reflects_recorded_misses`,
`test_inserting_an_existing_entry_is_a_no_op`,
`test_stops_growing_at_capacity`,
`test_evicts_in_fifo_order_regardless_of_lookups` (the FIFO-vs-LRU
regression test, re-touching the oldest entry before overflow and
asserting it's evicted anyway).

Unit tests remaining in
`crates/warp_completer/src/signatures/legacy/registry_tests.rs` (these
exercise `SignatureCache`/`CommandRegistry` behavior; the two
oversized-token tests touch `misses` only to confirm the length guard
never reaches it):
- `test_all_known_signature_names_are_within_the_length_cap` — walks
every embedded signature (recursively, including subcommands) plus the
Warp CLI's clap-derived command tree, asserting every name is within
`MAX_CACHEABLE_COMMAND_LEN`. This is the invariant the oversized-token
fast path in `get` depends on; if a future signature ever violates it,
this test fails instead of that name silently becoming unresolvable.
- `test_oversized_command_is_not_cached_and_resolves_to_none` — an
oversized, unregistered token resolves to `None` without growing
*either* the positive or the negative cache.
- `test_oversized_later_token_does_not_bypass_the_length_guard` — an
oversized *later* token (e.g. the argument to `sudo`, resolved via
`maybe_load_replacement_signature`) takes the same length-guarded path
as an oversized top-level token, via both `signature_from_tokens` and
`signature_with_alias_expansion`.
- `test_misses_are_never_cached_in_the_positive_cache` — a normal-length
command that never resolves to a signature never adds an entry to
`signatures`, however many times it's looked up.
- `test_ordinary_commands_still_resolve_and_are_cached` — registered
commands still resolve case-insensitively; repeated lookups hit the
positive cache rather than growing it.
- `test_registered_signature_longer_than_the_cap_is_unresolvable` — a
signature explicitly registered with an artificially oversized name is
*not* retrievable via `signature()`, documenting the accepted tradeoff
from removing the fallback scan.
- `test_exe_suffix_is_trimmed_before_the_length_check` (Windows-only) —
a command name exactly at the cap resolves correctly when looked up with
`.exe` appended, proving the trim happens before the length check.
- `test_registered_commands_unaffected_by_oversized_lookups` —
`CommandRegistry::registered_commands()` is unaffected by
oversized/negative lookups.

Validation run against `crates/warp_completer`:
- `./script/format` — clean.
- `cargo clippy -p warp_completer --all-targets --tests -- -D warnings`
— clean (default features, the legacy code path this PR touches).
- `cargo clippy -p warp_completer --all-targets --all-features --tests
-- -D warnings` — fails with the same 5 pre-existing
`collapsible_if`/`let_and_return` errors in
`completer/engine/argument/v2.rs`, re-confirmed to reproduce identically
on unmodified `master`; unrelated to this change.
- `cargo nextest run -p warp_completer` — 19 `registry` tests + 4
`miss_cache` tests pass. The same 25 pre-existing, unrelated failures
reproduce identically on unmodified `master` (`Tried to check
FeatureFlag::CloudEnvironments before feature flags were initialized`,
in `completer::describe`/`completer::engine`/`completer::suggest` tests)
— 149/174 total tests pass, matching the pre-existing baseline plus the
new tests.
- `cargo check -p warp_completer --features v2` — clean; the v2 registry
implementation is a separate, untouched module.
- `cargo check -p warp_terminal -p input_classifier -p warp_tui`
(dependents) — all build clean. `SignatureCache`'s public-facing return
types are unchanged (still `Option<&Signature>`).
- Removed `lru` as a dependency of `warp_completer` (no longer needed);
`MissCache` is built on `std::collections::{HashSet, VecDeque}` and
`std::sync::RwLock` only.
- Could not cross-compile-check the Windows-only test in this Linux
sandbox (no Windows target/toolchain available); the ordering it tests
(trim `.exe` before the length check) is unchanged from the previous
revision.

- [x] I have manually tested my changes locally (unit tests + the
crate-scoped validation above; this is not a UI change so `./script/run`
manual testing doesn't apply).

## Agent Mode
- [x] Warp Agent Mode - This PR was created via Warp's AI Agent Mode

<!-- warp:pr-description-artifacts start -->
<!-- warp:pr-description-artifacts end -->

CHANGELOG-BUG-FIX: Fixed an unbounded memory leak in the terminal
completions engine where large or unusual input (e.g. pasting a large
blob of text) could permanently grow memory usage over a session.

Co-Authored-By: Warp <agent@warp.dev>

---------

Co-authored-by: warp-agent-staging[bot] <240773466+warp-agent-staging[bot]@users.noreply.github.com>
Co-authored-by: Andy <andy@warp.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:performance:memory Memory usage, allocation, leaks, and memory-bound performance. cla-signed factory:wilson

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant