Skip to content

cache: preserve and count lookup read errors - #2967

Open
aw1cks wants to merge 1 commit into
mozilla:mainfrom
aw1cks-forks:aggregate-cache-read-errors
Open

aw1cks wants to merge 1 commit into
mozilla:mainfrom
aw1cks-forks:aggregate-cache-read-errors

Conversation

@aw1cks

@aw1cks aw1cks commented Oct 28, 2026

Copy link
Copy Markdown

Fixing two gaps in read-error metrics:

  • Read-error cache misses only incremented cache_errors and omitted cache_read_errors
  • Client-side mode failed to propagate some errors:
    • Backend read failures became an ordinary miss
    • IpcStorage also treated invalid cached archives as misses.

BREAKING CHANGE: the storage RPC response format changes, so the client and daemon need matching rebuilt binaries; restart the daemon after upgrading.

@codecov-commenter

codecov-commenter commented Oct 28, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.24812% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.69%. Comparing base (dac6c80) to head (978bee5).

Files with missing lines Patch % Lines
src/server.rs 99.15% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2967      +/-   ##
==========================================
+ Coverage   78.54%   78.69%   +0.14%     
==========================================
  Files          74       74              
  Lines       44039    44154     +115     
==========================================
+ Hits        34591    34745     +154     
+ Misses       9448     9409      -39     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread docs/Architecture.md
compilation increments `Cache read errors` as well as the existing `Cache errors`
and `Cache misses` counters. Ordinary misses do not increment the read-error
counter, and overall lookup timeouts are recorded separately as `Cache timeouts`.
Errors handled inside a multilevel tier walk are not aggregate lookup errors.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i didn't understand this line, could you please clarify or drop it? :)

Comment thread src/cache/ipc_storage.rs
Ok(entry) => Ok(Cache::Hit(entry)),
Err(_) => Ok(Cache::Miss),
}
Ok(Cache::Hit(CacheRead::from(file)?))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

maybe add a with_context with the path like the open above? otherwise the read error log doesn't say which entry is broken

Comment thread src/test/mock_storage.rs
}
async fn get_raw(&self, _key: &str) -> Result<Option<Bytes>> {
let next = self.raw_rx.lock().await.try_next().unwrap();
next.unwrap_or(Ok(None))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

try_next() returns Err when nothing is queued (tx is still alive), so the unwrap above panics and this unwrap_or never applies, no?
maybe just expect(...) like get does

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.

3 participants