Skip to content

fix: Make HybridCache::is_hybrid() correctly return false when storage is not configured#1283

Open
uckelman-sf wants to merge 2 commits intofoyer-rs:mainfrom
uckelman-sf:main
Open

fix: Make HybridCache::is_hybrid() correctly return false when storage is not configured#1283
uckelman-sf wants to merge 2 commits intofoyer-rs:mainfrom
uckelman-sf:main

Conversation

@uckelman-sf
Copy link
Copy Markdown

@uckelman-sf uckelman-sf commented Mar 5, 2026

What's changed and what's your intention?

HybridCache::is_hybrid() presently returns true in all circumstances, even when storage is not configured. It is supposed to return false when storage is not configured.

This test, which I added in bd866ee, fails in main currently:

    #[test_log::test(tokio::test)]
    async fn test_is_hybrid_without_disk_cache() {
        let cache = HybridCacheBuilder::<u8, u8>::new()
            .memory(1)
            .storage()
            .build()
            .await
            .unwrap();

        assert!(!cache.is_hybrid());
    }

HybridCache::is_hybrid() is this:

    pub fn is_hybrid(&self) -> bool {
        self.inner.storage.is_enabled()
    }

This is Store::is_enabled():

     pub fn is_enabled(&self) -> bool {
        self.inner.engine.type_id() != TypeId::of::<Arc<NoopEngine<K, V, P>>>()
     }

The problem with is_enabled() is that self.inner.engine is Arc<dyn Engine<K, V, P>> but Any::type_id() does not downcast---which means there are no circumstances where self.inner.engine.type_id() could return something equal to TypeId::of::<Arc<NoopEngine<K, V, P>>>().

The correct thing to do here is upcast the Arc's referent to a &dyn Any and then check if the concrete type is a NoopEngine<K, V, P>:

    pub fn is_enabled(&self) -> bool {
        !(self.inner.engine.as_ref() as &dyn Any).is::<NoopEngine<K, V, P>>()
     }

I've also provided a test for the case where a disk cache is in use, to ensure is_hybrid() returns true there.

Checklist

  • I have written the necessary rustdoc comments
  • I have added the necessary unit tests and integration tests
  • I have passed cargo x (or cargo x --fast instead if the old tests are not modified) in my local environment.

@uckelman-sf uckelman-sf changed the title Make HybridCache::is_hybrid() correctly return false when storage is not configured fix: Make HybridCache::is_hybrid() correctly return false when storage is not configured Mar 5, 2026
test_is_hybrid_without_disk_cache fails because
foyer_storage::store::Store::is_enabled doesn't work.

Signed-off-by: Joel Uckelman <joel.uckelman@levelblue.com>
ever return anything equal to TypeId::of::<Arc<NoopEngine<K, V, P>>>().
The correct thing is to take a ref, upcast that to &dyn Any, and then
call is() on it to check for downcastability.

Store::is_enabled()
Signed-off-by: Joel Uckelman <joel.uckelman@levelblue.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
foyer-storage/src/store.rs 86.43% <100.00%> (ø)
foyer/src/hybrid/cache.rs 85.23% <ø> (+0.48%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

1 participant