[engine] Bound the global parsed-AST cache

[haileyok]

Summary

parsed_ast_root_cache was an unbounded module-global dict and the lock cache a per-Source defaultdict, neither of which ever evicted — so every distinct (path, contents) parsed over the process lifetime leaked its AST and a Semaphore forever. The per-query UI-API parse path made this unbounded and user-driven.

Related Issues/Tasks

Found via an SML-engine bug hunt. No GitHub issue (N/A).

Changes Made

• Replaced the unbounded dict with a size-bounded LRU cache (_BoundedASTRootCache, stdlib OrderedDict; no new dependency).
• Replaced the per-Source lock defaultdict with a single global parse lock (parsing is non-yielding CPU work that only runs on a cache miss).
• Removed the dead contention traceback.print_stack() debug path tied to the old per-Source locking.
• Added regression tests for LRU eviction and for the module wiring a bounded cache.

Confidence Level

Confidence Level: Claude

Testing

• uv run pytest osprey_worker/src/osprey/engine/ast/tests/test_ast_root_cache.py passes.
• Full engine unit suite passes locally (Docker integration suite not run locally).
• uv run ruff check ., uv run mypy ast/grammar.py, and fawltydeps pass.

Checklist

• Tests pass locally
• uv run ruff check . passes (no unused imports or other lint errors)
• uv tool run fawltydeps --check-unused --pyenv .venv passes (no unused dependencies)
• Updated CHANGELOG.md with my changes, if applicable

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 9145f0d1-8dc7-4f6f-85fd-263426be3e91

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/sml-ast-cache-bound

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[haileyok]

Why the bounded (LRU) cache matters

The cache is keyed on Source contents and never evicted. Source compares by (path, contents), so each distinct contents string becomes a permanent entry holding a full parsed AST for the life of the process. That's fine only if the set of sources is fixed — it isn't:

• Worker rule reloads (EtcdSourcesProvider rebuilds Sources on every etcd update) → each rule edit is new contents → a new permanent entry; old revisions' ASTs stay forever. Slow leak, grows with edits over the process lifetime.
• UI-API query validation (dominant vector): the query path parses a fresh Source (~'Query = ' + user_query) per request, so every distinct ad-hoc query a user types permanently leaks an AST in the long-running Flask process. User-driven and effectively unbounded.

The old defaultdict lock cache was worse in one way: merely reading ast_root_lock_cache[self] inserts a Semaphore, so even sources that fail to parse leak a lock (hence the move to a single global parse lock).

Why LRU specifically: we still want the cache (ast_root is hit many times per source; re-parsing every time would be wasteful), so the fix is to bound it, not remove it. LRU keeps the hot working set resident (the active ruleset stays cached) and evicts the cold tail (old revisions, one-off queries). Eviction is correctness-safe: a miss just re-parses via transform() — deterministic and cheap — so the only cost of evicting something still-needed is a little CPU, never a wrong result. The 4096 cap holds a full ruleset plus several reload generations.

Honest caveat: this is a gradual leak, not a crash. A worker that loads rules once and never reloads was effectively fine under the old code; the bound matters because of reload churn and the per-query UI path, which trend toward OOM in processes meant to run for weeks.