Skip to content

Response cache stats#206

Open
acmel wants to merge 11 commits into
sashiko-dev:mainfrom
acmel:response_cache_stats
Open

Response cache stats#206
acmel wants to merge 11 commits into
sashiko-dev:mainfrom
acmel:response_cache_stats

Conversation

@acmel
Copy link
Copy Markdown
Contributor

@acmel acmel commented May 19, 2026

● Response cache statistics, startup improvements, and bug fixes

This series adds observability into the response cache, fixes several
bugs found during testing, and improves daemon startup behavior.

Response cache statistics (patches 3-6)

Track per-review cache hit/miss/token counts in the database and expose
them through the API, CLI (sashiko show), and web UI. The web UI uses
tap-to-expand panels with hit-rate progress bars — available per-patch
and aggregated at the patchset level. All cache stats display is gated
behind the show_cache_stats setting (default off).

Example sashiko-cli show output with cache stats enabled:

Patchset Details:
ID: 61
Subject: perf session: Add minimum event size and alignment validation
Author: Arnaldo Carvalho de Melo acme@redhat.com
Status: Failed
Date: 2026-05-19 14:48:09

Patches (28):
[1] perf session: Add minimum event size and alignment validation (Failed) [Failed] {cache: 502/543 hits, 23.1M tokens saved}
[2] perf tools: Fix event_contains() macro to verify full field extent (Reviewed) [Issues Found] {cache: 413/807 hits, 15.7M tokens saved}
...
[27] perf kwork: Bounds check work->cpu before indexing cpus_runtime[] (Reviewed) [Reviewed] {cache: 3667/4155 hits, 163.0M tokens saved}
[28] perf test: Add truncated perf.data robustness test (Reviewed) [Issues Found] {cache: 3080/3334 hits, 134.8M tokens saved}

Cache: 64296/78526 hits (81.9%), 2841.6M tokens saved, 647.2M tokens stored
Model: gemini-3.1-pro-preview

In the web interface, these stats are available by tapping the ▶ indicator
next to each patch row for per-patch details, and on the Model: line in
the patchset header for aggregate stats across all patches.

Startup and operational improvements (patches 2, 11, 12)

  • Support user-level config in ~/.config/sashiko/Settings.toml so
    personal credentials and overrides don't live in the repo checkout
  • Move custom remote fetches to a background task so the reviewer starts
    immediately instead of blocking on slow git.kernel.org IPv6 fetches
  • Log response cache open timing, file size, and entry count at startup

Bug fixes (patches 1, 7-10)

  • Fix sashiko show picking a stale "Reviewed" result over a newer
    "Failed" retry after patchset resubmission
  • Fix cancel endpoint rejecting localhost connections on dual-stack
    servers (missing to_canonical() call)
  • Return proper 404 when cancelling a non-existent patchset
  • Stop submitting LLM requests for remaining patches after a patchset
    is cancelled

Docs (patch 0)

  • Extract LLM provider configurations into examples/ directory

I'll work now on adding limits to the cache and hopefully some sort of LRU to keep LLM requests that have tons of hits while making space for new entries by purging seldom used ones and then make the request cache a default feature with a given reasonable size based on the experience we gather by exposing the stats using this patch series publicly, which is something I encourage the admin for https:://sashiko.dev to do when upgrading to the next release.

Saving "I know that this is not something introduced by your patch, but there is this pre-existing problem that I analysed and found problematic, can you postpone submitting this series and fixing just this one more issue, pretty please?" cases in some sort of database that gets exposed in the https://sashiko.dev site in one special session of "please help picking up from this cache of query/response tokens cache of problems" is another idea I plan to work on Real Soon Now. :-)

 @@@@@Cheers,

Signed-off-by: Arnaldo Carvalho de Melo acme@redhat.com

@derekbarbosa
Copy link
Copy Markdown
Collaborator

derekbarbosa commented May 19, 2026

Hi @acmel, thank you for the PR.

Here's gemini's output via the /review-pr skill, vetted through a few passes.

  Below are the findings from the review, followed by an adversarial check.

  🔴 Safety / Bugs
   * src/main.rs:L252: 🔴 Safety: The custom_remotes fetch is moved to a background task using tokio::spawn. While this correctly prevents blocking the startup, it uses a shared
     repo_path. Ensure sashiko::git_ops::ensure_remote handles concurrent access or that the reviewer service doesn't try to use these remotes before they are initialized. Since
     ensure_remote is async, it likely involves process-level locking or atomic git operations, but a race condition where a review starts for a remote that isn't fully fetched yet might
     result in a "Remote not found" error instead of waiting.
       * Fix: The code relies on the reviewer calling ensure_remote itself if it misses. This is a safe fallback. No immediate action required, but monitor for "Failed to ensure custom
         remote" logs.

  🟡 Complexity / Logic / SOLID
   * src/reviewer.rs:L568 & L628: 🟡 Logic: Added cancellation checks in the worker loops. Draining the queue (queue.lock().await.clear()) is effective for stopping further processing.
     However, if multiple worker threads detect cancellation simultaneously, they all attempt to clear the same shared queue.
       * Fix: Use an atomic or a more coordinated "kill switch" (like a CancellationToken) if this causes lock contention, though given the context, the current implementation is
         acceptable for stopping the review process.

Other than that, the db changes look safe. @rgushchin I am not sure how you handle the database migrations for adding new columns but if they are allowed to be null, then it should be fine.

derekbarbosa
derekbarbosa approved these changes May 19, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@derekbarbosa derekbarbosa left a comment •
edited
Loading

Choose a reason for hiding this comment

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

My only qualm is the safety finding in main and potentially the logic bug. Once those get fixed -- LGTM

@acmel
Copy link
Copy Markdown
Contributor Author

acmel commented May 20, 2026

Thanks for the review, Derek!

On the two Gemini findings:

  1. Safety (main.rs background remotes): This is by design — the reviewer calls ensure_remote() itself when it encounters a RemoteTarget baseline, so patchsets targeting a custom remote will wait for the fetch at that point. Other patchsets proceed without delay. The comment in the code documents this.

  2. Logic (reviewer.rs queue drain): Multiple workers calling queue.lock().await.clear() on an already-empty Vec is a no-op — there's no contention issue. Each lock acquisition is instant (just clearing or popping a single element).

On the migration question: yes, all four new columns (cache_hits, cache_misses, cache_tokens_saved, cache_tokens_stored) are added via try_add_column("reviews", ..., "INTEGER") with no NOT NULL constraint, so they default to NULL for existing rows. No backfill needed.

@acmel
Copy link
Copy Markdown
Contributor Author

acmel commented May 20, 2026

I'll force push now with the merge conflicts resolved.

@acmel acmel force-pushed the response_cache_stats branch 2 times, most recently from 19cd4d5 to dccf6e0 Compare May 22, 2026 01:19
acmel and others added 11 commits May 22, 2026 13:03
@acmel @claude
fix(cli,db): show latest review status after patchset resubmission
4a87d70 
find_best_review_for_patch_refs() used a broken status-preference heuristic
that could select a stale "Reviewed" result from a previous run over the most
recent "Failed" retry.  Replace with a simple highest-id comparison so the
most recent review always wins.

rerun_patchset() only reset the patchset-level status but left per-patch
statuses (and apply_error) from the previous run.  Add a step to reset all
patches to "Pending" with apply_error cleared.

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Derek Barbosa <debarbos@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@acmel @claude
feat(settings): support user-level config in ~/.config/sashiko/
cef0d35 
Add layered configuration loading so users can place personal overrides
(credentials, paths, custom settings) in ~/.config/sashiko/Settings.toml
without modifying the repo-level Settings.toml.

Settings are loaded in order (last wins):
  1. ./Settings.toml        — repo-level defaults
  2. ~/.config/sashiko/Settings.toml — user overrides (optional)
  3. SASHIKO__* env vars    — per-invocation overrides

The user config path follows the XDG Base Directory Specification:
$XDG_CONFIG_HOME/sashiko/Settings.toml if set, else ~/.config/sashiko/.

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Derek Barbosa <debarbos@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@acmel @claude
feat(cache,settings): add cache miss/tokens-stored counters and show_…
79eb750 
…cache_stats setting

Extend CacheStats with two new fields: `misses` (requests that missed the
cache and triggered an API call) and `tokens_stored` (total tokens in newly
created cache entries, representing future savings potential).

Add `show_cache_stats` boolean to AiSettings (default false) as the gate for
all cache statistics display in CLI, web UI, and daemon logs.

Add `fmt_tokens()` helper for human-readable token counts (42.1k, 1.5M).

Document the response_cache, response_cache_ttl_days, and show_cache_stats
settings in Settings.toml with commented examples.

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Derek Barbosa <debarbos@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@acmel @claude
feat(db,reviewer): record per-review response cache statistics
1bd7c16 
Add four nullable INTEGER columns to the reviews table via migration:
cache_hits, cache_misses, cache_tokens_saved, cache_tokens_stored.

Extend complete_review() with an optional ReviewCacheStats parameter to
populate these columns.

In process_patch_review(), snapshot CacheStats before run_review_tool()
and compute the delta after it returns.  This gives per-patch cache
hit/miss/tokens data without new synchronization — atomic counters only
grow, so the difference is the per-review contribution.

Gate the patchset-level cache summary log and add per-patch cache log
lines behind the show_cache_stats setting.

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Derek Barbosa <debarbos@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@acmel @claude
feat(api,cli): expose and display response cache statistics
ae23856 
Wire show_cache_stats through the API layer: add it to AppState, pass it
from main.rs, and inject it into patchset detail responses alongside the
existing smtp_enabled and dry_run flags.

Extend the reviews SELECT in get_patchset_details() with cache_hits,
cache_misses, cache_tokens_saved, cache_tokens_stored columns.

In the CLI, extend print_patch_line() with a cache_suffix parameter.
When show_cache_stats is enabled, build per-patch suffixes like
"{cache: 7/7 hits, 42.1k tokens saved}" and display a patchset-level
summary line with hit rate and token totals.

Add fmt_tokens() helper for human-readable token counts (k/M suffixes).

Document the response cache feature and previously undocumented CLI
commands (rerun, cancel, local, --watch) in README.md.

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Derek Barbosa <debarbos@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@acmel @claude
feat(web): show response cache statistics with tap-to-expand
30b0f8c 
When show_cache_stats is enabled in the API response, the web UI now
shows cache statistics via tap/click-to-expand panels that work on
both desktop and mobile browsers.

Per-patch: each patch row in the summary table shows a ▶ indicator
when cache stats are available.  Tapping it expands an animated
stats panel with a 3-column grid (hits, misses, hit rate, tokens
saved, tokens stored, total calls) and a hit-rate progress bar.
The row click still scrolls to the patch detail as before.

Patchset-level: the Model line in the metadata section gains a ▶
indicator that expands to show aggregate cache stats across all
patches in the series.

Uses CSS max-height/opacity transitions for smooth animation.  Adds
fmtTokens() JS helper for human-readable token counts (k/M suffixes).

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Derek Barbosa <debarbos@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@acmel @claude
fix(api): use to_canonical() in cancel_patchset loopback check
386a4e6 
The cancel_patchset endpoint was the only mutation endpoint missing
the .to_canonical() call before .is_loopback(), causing 403 Forbidden
when a CLI on the same machine connects via IPv4 (127.0.0.1) to a
dual-stack server (bound to ::).  The IPv4 address arrives as the
mapped ::ffff:127.0.0.1, which is_loopback() rejects.

The submit, rerun_patch, and rerun_patchset endpoints already use
to_canonical() — this was simply missed on cancel.

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Derek Barbosa <debarbos@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@acmel @claude
fix(api,cli): return 404 when cancelling a non-existent patchset
991d169 
cancel_patchset returned the same "not in a cancellable state"
message whether the patchset didn't exist or existed in a
non-cancellable status.  Change the DB function to return
Option<bool> (None = not found, Some(true) = cancelled,
Some(false) = wrong status) so the API can return 404 and the
CLI can show "Patchset N not found".

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Derek Barbosa <debarbos@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@acmel @claude
fix(reviewer): stop processing patches when patchset is cancelled
5653822 
The reviewer only checked for cancellation after completing all patch
reviews, so a cancelled patchset would keep submitting LLM requests
until every patch was processed.  Add a cancellation check before
each process_patch_review call in both the concurrent and main worker
loops — when cancellation is detected, the shared job queue is drained
so all workers stop promptly.

This does not interrupt an in-flight LLM call (that would require
threading a CancellationToken through the AI provider), but it stops
between patches, which is the useful granularity.

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Derek Barbosa <debarbos@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@acmel @claude
fix(main): don't block reviewer startup on custom remote fetches
bb7b717 
Fetching custom remotes (e.g. pahole) from git.kernel.org over IPv6 can
be sluggish, and the sequential ensure_remote() calls in main() blocked
the Reviewer service from starting until all fetches completed.  This
meant no patchsets were picked up while waiting for a slow git fetch.

Move the Reviewer::new() + start() before the custom remote loop and
spawn the remote fetches in a background task.  The reviewer already
calls ensure_remote() itself when it encounters a RemoteTarget baseline
candidate, so patchsets that need a custom remote will wait for the fetch
at that point — while patchsets targeting other repos proceed without
delay.

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Derek Barbosa <debarbos@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@acmel @claude
feat(cache): log timing, file size and entry count on startup
993c672 
Add an "Opening response cache" message before the DB initialization
begins and a "Response cache ready" summary when it completes, reporting
the number of cached entries, file size (MB/GB), lifetime tokens saved,
and elapsed wall-clock time.  This makes it easy to spot slow cache
opens and provides useful diagnostics at startup.

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Derek Barbosa <debarbos@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@acmel acmel force-pushed the response_cache_stats branch from dccf6e0 to 993c672 Compare May 22, 2026 17:58
@acmel
Copy link
Copy Markdown
Contributor Author

acmel commented May 22, 2026

I rebased it to resolve some conflicts, have been using it for a while. Also have a ai_pricing branch on top and now I'm working on LFU/LRU'ing the response cache contents, with a cache contents page that will show which are the entries most used, to help understand where is that all those hits are coming from :-)

@rgushchin
Copy link
Copy Markdown
Member

rgushchin commented May 22, 2026

Thank for the PR, I’ll try to merge it soon. Sorry, busy with some other stuff. But I still do’t fully understand how this cache works: I don’t think sashiko sends too many identical requests. Am I wrong? Can you provide some examples?

@acmel
Copy link
Copy Markdown
Contributor Author

acmel commented May 22, 2026 via email

@acmel
Copy link
Copy Markdown
Contributor Author

acmel commented May 22, 2026

And here are some lines from the sashiko output, as you can see there it finds hits in the cache for things in the current session (supposedly it submitted it multiple times, perhaps for different patches the same prompt?) and some hits are for the same request submitted on a previous session, for the a previous version of the patchset being submitted, I'll put the branch I have with the cache stats page available in my repo, so you can look at it and see if it makes sense, from your point of view, or if something is wrong.

It all seems to work, at least I'm getting good review that I then address, resubmit, notice that it solved the previous report, get new reports, rinse, repeat.

2026-05-22T20:02:01.965359Z INFO sashiko::ai::cache: Cache hit [0665e4942100] (previous session) — 36.458 tokens saved (total previous session: 214.707.298)
2026-05-22T20:02:02.451964Z INFO sashiko::ai::cache: Cache hit [253d06424ab0] (previous session) — 38.486 tokens saved (total previous session: 214.745.784)
2026-05-22T20:02:02.456869Z INFO sashiko::ai::cache: Cache hit [6669e025b788] (previous session) — 38.880 tokens saved (total previous session: 214.784.664)
2026-05-22T20:02:02.942157Z INFO sashiko::ai::cache: Cache hit [240c333457ba] (previous session) — 39.732 tokens saved (total previous session: 214.824.396)
2026-05-22T20:02:03.431223Z INFO sashiko::ai::cache: Cache hit [eb1f9acf0a74] (previous session) — 40.081 tokens saved (total previous session: 214.864.477)
2026-05-22T20:02:03.921442Z INFO sashiko::ai::cache: Cache hit [224ed09b8e46] (previous session) — 40.809 tokens saved (total previous session: 214.905.286)
2026-05-22T20:02:04.410607Z INFO sashiko::ai::cache: Cache hit [a83844651fd7] (previous session) — 41.135 tokens saved (total previous session: 214.946.421)
2026-05-22T20:04:04.416989Z INFO sashiko::ai::cache: Cache hit [3bf10d89f5b6] (this session) — 42.192 tokens saved (total this session: 244.906.548)
2026-05-22T20:04:04.551570Z INFO sashiko::ai::cache: Cache hit [4f84282a22a5] (this session) — 55.771 tokens saved (total this session: 244.962.319)
2026-05-22T20:04:05.039038Z INFO sashiko::ai::cache: Cache hit [52530fb65ed4] (this session) — 59.471 tokens saved (total this session: 245.021.790)
2026-05-22T20:04:05.044632Z INFO sashiko::ai::cache: Cache hit [d216abc3e482] (this session) — 60.234 tokens saved (total this session: 245.082.024)
2026-05-22T20:04:05.048856Z INFO sashiko::ai::cache: Cache hit [84bc66ff7f4e] (this session) — 61.328 tokens saved (total this session: 245.143.352)
2026-05-22T20:04:05.536817Z INFO sashiko::ai::cache: Cache hit [1946bb82db20] (this session) — 62.273 tokens saved (total this session: 245.205.625)
2026-05-22T20:04:06.024585Z INFO sashiko::reviewer: [review-bin] Stage 6 hit non-retryable error: Max interactions exceeded
2026-05-22T20:04:06.024603Z INFO sashiko::reviewer: [review-bin] AI review failed with exception: Max interactions exceeded
2026-05-22T20:04:06.027374Z INFO sashiko::reviewer: Patch 68/6 cache: 1656 hits, 0 misses, 89.9M tokens saved
2026-05-22T20:04:06.029909Z ERROR sashiko::reviewer: Review tool returned error for ps=68 idx=6: Max interactions exceeded
2026-05-22T20:04:06.032538Z INFO sashiko::git_ops: Removing worktree at "/home/acme/git/sashiko/review_trees/sashiko-worktree-r38CsL"
2026-05-22T20:04:06.820061Z INFO sashiko::reviewer: Patchset 68 cache summary — 8.810 hits (3.761 this session, 5.049 previous), 460.152.046 tokens saved (245.205.625 this session, 214.946.421 previous)

@acmel
Copy link
Copy Markdown
Contributor Author

acmel commented May 22, 2026

An excerpt from a Claude session that is talking with sashiko:

● Patch 15 came back clean (Reviewed). Three left: 6, 25, 26 still in review. No new findings. Will check again in 4 minutes.

✻ Cogitated for 8s

✻ Running scheduled task (May 22 4:56pm)

● Bash(sashiko-cli show 68 2>&1 | head -55)
⎿  Patchset Details:
ID: 68
Subject: perf session: Add minimum event size and alignment validation
… +23 lines (ctrl+o to expand)

● Patch 26 reviewed clean. Two left: 6 and 25 still in review. Almost done. Will check again in 4 minutes.

✻ Cogitated for 8s

✻ Running scheduled task (May 22 5:00pm)

● Bash(sashiko-cli show 68 2>&1 | head -55)
⎿  Patchset Details:
ID: 68
Subject: perf session: Add minimum event size and alignment validation
… +23 lines (ctrl+o to expand)

● No change — patches 6 and 25 still in review. Will check again in 4 minutes.

✻ Baked for 9s

✻ Running scheduled task (May 22 5:04pm)

● Bash(sashiko-cli show 68 2>&1 | head -55)
⎿  Patchset Details:
ID: 68
Subject: perf session: Add minimum event size and alignment validation
… +23 lines (ctrl+o to expand)

● Review is complete — all 27 patches have a final status. No new "Issues Found" since last check (6 and 25 both failed infra).

Final tally:

  • Reviewed clean: 2, 4, 5, 8, 10, 11, 12, 13, 15, 18, 20, 21, 23, 24, 26 (15 patches)
  • Issues Found (all triaged, none actionable in current code): 3, 7, 9, 14, 19 (5 patches)
  • Failed (infra): 1, 6, 16, 17, 22, 25, 27 (7 patches)

One actionable fix was found and already applied: patch 19's lseek pipe mode regression. All other findings were pre-existing, theoretical TOCTOU, or already addressed.

● CronDelete(0d31cf39)
⎿  Cancelled 0d31cf39

● Cron job stopped. Submission 68 review complete — no new actionable issues beyond the patch 19 lseek fix already applied.

✻ Cogitated for 19s

──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── perf-tools-next ──
❯ 
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
? for shortcuts

acme@number:~$

@acmel
Copy link
Copy Markdown
Contributor Author

acmel commented May 22, 2026

cache_lru_eviction branch has the page with the cache contents and the reference counting of response cache entries, etc, look at the bottom of the main page, there is a "Cache" link next to the other stats links.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@derekbarbosa derekbarbosa derekbarbosa approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@acmel @derekbarbosa @rgushchin