Persist runtime state (learnings, review history, vector index, webhook dedup) across restarts #19

New issue

Closed

opened 2026-05-01 19:39:01 -07:00 by jwilger · 0 comments

jwilger commented

2026-05-01 19:39:01 -07:00

Owner

Problem

State the gateway accumulates during operation does not survive a restart unless the operator opts in via env vars, and one of the most expensive pieces of state — the symbol embedding index — has no persistent backing wired into the review pipeline at all. Every systemctl restart auto_review (or, in our current dev workflow, every push to main that the watcher picks up) effectively resets the bot's working memory.

Current behaviour

crates/ar-gateway/src/main.rs:174-211 defaults both stores to the in-memory implementations:
- learnings → InMemoryLearningsStore unless AR_LEARNINGS_DB is set
- review_history → InMemoryReviewHistory unless AR_HISTORY_DB is set
- GatewayInfo (main.rs:297-306) reports "in-memory" for both in /info so this is observable, but operators have to read the docs to discover the env-var opt-in.
crates/ar-review/src/context_builder.rs:134 constructs a fresh InMemoryVectorStore::new() on every review — SqliteVectorStore exists in ar-index but is never wired in. Result: we re-walk + re-embed the entire workspace on every PR even though the symbol set rarely changes between reviews of the same repo.
crates/ar-gateway/src/main.rs:329-337 keeps webhook delivery dedup (RecentDeliveries) in an in-memory LRU. After a restart, in-flight redeliveries from Forgejo can trigger duplicate reviews of the same SHA.
crates/ar-orchestrator/src/dispatcher.rs:156 defaults the dispatcher's history to InMemoryReviewHistory, which is the right factory default but means the gateway has to remember to override it.

Why this matters

Incremental review depends on ReviewHistory.last_reviewed. After a restart, every open PR's next webhook becomes a full review again — wasted tokens and duplicate inline comments on lines that haven't changed.
remember/forget chat commands write to LearningsStore. With the in-memory default, anything the bot was asked to remember evaporates on restart.
Symbol embeddings are the costliest call against the local Ollama embedder (especially with the oversized-context bug — see the sibling issue). Re-embedding on every review for state we already computed is the single biggest avoidable cost.
Webhook redelivery dedup resetting on restart is a correctness hazard, not just a cost one.

Proposed direction

Make persistence the default: pick sensible default paths (e.g. $XDG_STATE_HOME/auto_review/{learnings,history,vector}.db), open SQLite there unless the operator explicitly opts out via AR_*_DB=:memory: or similar.
Wire SqliteVectorStore into embed_and_query_symbols (or thread a shared store through BuildReviewContext) so symbol embeddings persist across reviews and restarts. Decide on an invalidation strategy keyed off file mtime or content hash so stale entries don't accumulate.
Add a persistent backing for RecentDeliveries (or move dedup into the review_history table keyed by delivery_id) so post-restart redeliveries are still suppressed.
Surface the chosen backings in /info and structured startup logs so operators can confirm at a glance.

Out of scope

Migration tooling between in-memory and SQLite (greenfield deployments).
Postgres backend — SQLite is the agreed substrate for now.

## Problem State the gateway accumulates during operation does not survive a restart unless the operator opts in via env vars, and one of the most expensive pieces of state — the symbol embedding index — has no persistent backing wired into the review pipeline at all. Every `systemctl restart auto_review` (or, in our current dev workflow, every push to `main` that the watcher picks up) effectively resets the bot's working memory. ## Current behaviour - `crates/ar-gateway/src/main.rs:174-211` defaults both stores to the in-memory implementations: - `learnings` → `InMemoryLearningsStore` unless `AR_LEARNINGS_DB` is set - `review_history` → `InMemoryReviewHistory` unless `AR_HISTORY_DB` is set - `GatewayInfo` (`main.rs:297-306`) reports `"in-memory"` for both in `/info` so this is observable, but operators have to read the docs to discover the env-var opt-in. - `crates/ar-review/src/context_builder.rs:134` constructs a fresh `InMemoryVectorStore::new()` **on every review** — `SqliteVectorStore` exists in `ar-index` but is never wired in. Result: we re-walk + re-embed the entire workspace on every PR even though the symbol set rarely changes between reviews of the same repo. - `crates/ar-gateway/src/main.rs:329-337` keeps webhook delivery dedup (`RecentDeliveries`) in an in-memory LRU. After a restart, in-flight redeliveries from Forgejo can trigger duplicate reviews of the same SHA. - `crates/ar-orchestrator/src/dispatcher.rs:156` defaults the dispatcher's history to `InMemoryReviewHistory`, which is the right factory default but means the gateway has to remember to override it. ## Why this matters - Incremental review depends on `ReviewHistory.last_reviewed`. After a restart, every open PR's next webhook becomes a full review again — wasted tokens and duplicate inline comments on lines that haven't changed. - `remember`/`forget` chat commands write to `LearningsStore`. With the in-memory default, anything the bot was asked to remember evaporates on restart. - Symbol embeddings are the costliest call against the local Ollama embedder (especially with the oversized-context bug — see the sibling issue). Re-embedding on every review for state we already computed is the single biggest avoidable cost. - Webhook redelivery dedup resetting on restart is a correctness hazard, not just a cost one. ## Proposed direction 1. Make persistence the default: pick sensible default paths (e.g. `$XDG_STATE_HOME/auto_review/{learnings,history,vector}.db`), open SQLite there unless the operator explicitly opts out via `AR_*_DB=:memory:` or similar. 2. Wire `SqliteVectorStore` into `embed_and_query_symbols` (or thread a shared store through `BuildReviewContext`) so symbol embeddings persist across reviews and restarts. Decide on an invalidation strategy keyed off file mtime or content hash so stale entries don't accumulate. 3. Add a persistent backing for `RecentDeliveries` (or move dedup into the review_history table keyed by `delivery_id`) so post-restart redeliveries are still suppressed. 4. Surface the chosen backings in `/info` and structured startup logs so operators can confirm at a glance. ## Out of scope - Migration tooling between in-memory and SQLite (greenfield deployments). - Postgres backend — SQLite is the agreed substrate for now.

jwilger referenced this issue from a commit

2026-05-01 23:11:29 -07:00

feat: persist runtime state across restarts (closes #19)

jwilger referenced this issue from a pull request that will close it,

2026-05-01 23:13:40 -07:00

feat: persist runtime state across restarts (closes #19) #25

jwilger referenced this issue from a commit

2026-05-01 23:44:59 -07:00

feat: persist runtime state across restarts (closes #19)

jwilger closed this issue

2026-05-02 06:47:30 -07:00

jwilger referenced this issue from a commit