Skip to content

Comments

fix: classify coalesced cache misses in PageGetExecutor#82

Merged
shikhar merged 11 commits intomainfrom
codex/fix-issue-70-coalesced-miss-status
Feb 20, 2026
Merged

fix: classify coalesced cache misses in PageGetExecutor#82
shikhar merged 11 commits intomainfrom
codex/fix-issue-70-coalesced-miss-status

Conversation

@shikhar
Copy link
Member

@shikhar shikhar commented Feb 20, 2026

Summary

  • Fix coalesced misses being reported as cache hits in PageGetExecutor.
  • Classify request outcomes from foyer entry source.
  • Add explicit coalesced metric: cachey_page_request_total{type="coalesced"}.
  • Split cache-hit metrics into:
    • cache_hit_memory
    • cache_hit_disk
      while keeping aggregate cache_hit.
  • Make page request metric labels type-safe via PageRequestType and export it publicly.
  • Replace helper-only tests with an end-to-end executor coalescing test.

Validation

  • cargo +nightly fmt
  • cargo clippy --all-features --all-targets -- -D warnings --allow deprecated
  • cargo nextest run --lib

Closes #70

@greptile-apps
Copy link

greptile-apps bot commented Feb 20, 2026

Greptile Summary

This PR fixes cache miss classification for coalesced requests by leveraging foyer's entry source instead of request-local tracking. Previously, coalesced requests (concurrent requests for the same uncached page) were incorrectly classified. The fix introduces:

  • Type-safe metrics: PageRequestType enum replaces string literals
  • Source-based classification: Uses foyer::Source (Memory/Disk/Outer) to determine hit vs miss
  • Coalesced tracking: Distinguishes leader misses (fetch closure ran) from coalesced misses (waited for concurrent fetch) via fetched_by_current_request flag
  • Split cache hit metrics: Separates cache_hit_memory and cache_hit_disk while retaining aggregate cache_hit
  • Comprehensive test: Replaces unit tests with e2e test verifying coalescing behavior and metrics

The implementation correctly sets cached_at = 0 for all misses (leader and coalesced) via cache_lookup_outcome_for_source, maintaining the expected semantics for response headers and latency metrics.

Confidence Score: 5/5

  • This PR is safe to merge with high confidence
  • The changes are well-structured with clear separation of concerns. Type safety improvements via the PageRequestType enum prevent metric label typos. The core logic correctly uses foyer's Source to classify cache outcomes, and the implementation properly handles the coalesced case by tracking whether the current request actually ran the fetch closure. The comprehensive e2e test validates the entire flow including request coalescing, metric counting, and cached_at semantics. Code follows Rust best practices with explicit error handling and descriptive naming.
  • No files require special attention

Important Files Changed

Filename Overview
src/service/metrics.rs Added type-safe PageRequestType enum replacing string literals for metric labels
src/service/mod.rs Implemented foyer Source-based cache hit/miss classification with coalesced request tracking and comprehensive e2e test

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[PageGetExecutor.execute] --> B[Increment access metric]
    B --> C[cache.get_or_fetch]
    C --> D{Cache hit or miss?}
    D -->|Miss - no cached entry| E[Run fetch closure]
    E --> F[Set fetched_by_current_request = true]
    F --> G[Download from S3]
    G --> H[Increment download metric]
    H --> I[Return CacheValue with cached_at = now]
    D -->|Hit or coalesced| J[Return existing/coalescing entry]
    I --> K[Check entry.source]
    J --> K
    K --> L{Source type?}
    L -->|Source::Memory| M[CacheHit + CacheHitMemory metrics]
    L -->|Source::Disk| N[CacheHit + CacheHitDisk metrics]
    L -->|Source::Outer + fetched=true| O[Leader miss - no metric]
    L -->|Source::Outer + fetched=false| P[Coalesced metric]
    M --> Q[Keep cached_at timestamp]
    N --> Q
    O --> R[Set cached_at = 0]
    P --> R
    Q --> S[Return page data]
    R --> S
Loading

Last reviewed commit: 4ef682a

@shikhar
Copy link
Member Author

shikhar commented Feb 20, 2026

@greptileai re-review

@shikhar shikhar merged commit 96001d7 into main Feb 20, 2026
5 checks passed
@shikhar shikhar deleted the codex/fix-issue-70-coalesced-miss-status branch February 20, 2026 15:37
@github-actions github-actions bot mentioned this pull request Feb 20, 2026
shikhar pushed a commit that referenced this pull request Feb 20, 2026
## 🤖 New release

* `cachey`: 0.10.2 -> 0.10.3

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

##
[0.10.3](0.10.2...0.10.3)
- 2026-02-20

### Fixed

- *(types)* reject control characters in bucket names
([#84](#84))
- *(object_store)* merge per-request timeout and retry overrides
([#85](#85))
- classify coalesced cache misses in PageGetExecutor
([#82](#82))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

[Detail Bug] Coalesced cache misses are reported as hits in PageGetExecutor

1 participant