ref: Refactor fetching/computing derived Cache

crates/symbolicator-service/src/cache.rs

@@ -18,6 +18,8 @@ use tokio::fs::File;
 use tokio::io::{AsyncSeekExt, AsyncWriteExt};
 
 use crate::config::{CacheConfig, Config};
+use crate::services::objects::{FoundObject, ObjectError, ObjectMetaHandle};
+use crate::types::{AllObjectCandidates, CandidateStatus, ObjectFeatures, ObjectUseInfo};
 
 /// Starting content of cache items whose writing failed.
 ///
@@ -292,6 +294,90 @@ pub fn cache_entry_as_cache_status<T>(entry: &CacheEntry<T>) -> CacheStatus {
     }
 }
 
+/// This is the result of fetching a derived cache file.
+///
+/// This has the requested [`CacheEntry`], as well as [`AllObjectCandidates`] that were considered
+/// and the [`ObjectFeatures`] of the primarily used object file.
+#[derive(Clone, Debug)]
+pub struct DerivedCache<T> {
+    pub cache: CacheEntry<T>,
+    pub candidates: AllObjectCandidates,
+    pub features: ObjectFeatures,
+}
+
+/// Derives a [`DerivedCache`] from the provided object handle and derive function.
+///
+/// This function is mainly a wrapper that simplifies error handling and propagation of
+/// [`AllObjectCandidates`] and [`ObjectFeatures`].
+pub async fn derive_from_object_handle<T, Derive, DeriveErr, Fut>(
+    found_object: Result<FoundObject, ObjectError>,
+    candidate_status: CandidateStatus,
+    derive: Derive,
+) -> DerivedCache<T>
+where
+    T: Clone,
+    Derive: FnOnce(Arc<ObjectMetaHandle>) -> Fut,
+    Fut: std::future::Future<Output = Result<Arc<CacheEntry<T>>, Arc<DeriveErr>>>,
+    for<'a> &'a DeriveErr: Into<CacheError>,
+{
+    let (meta, mut candidates) = match found_object {
+        Ok(FoundObject { meta, candidates }) => (meta, candidates),
+        Err(e) => {
+            // NOTE: the only error that can happen here is if the Sentry downloader `list_files`
+            // fails, which we can consider a download error
+            let dynerr: &dyn std::error::Error = &e; // tracing expects a `&dyn Error`
+            tracing::error!(error = dynerr, "Error finding object candidates");
+            return DerivedCache {
+                cache: Err(CacheError::DownloadError(e.to_string())),
+                candidates: Default::default(),
+                features: Default::default(),
+            };
+        }
+    };
+
+    // No handle => NotFound
+    let Some(handle) = meta else {
+        return DerivedCache {
+            cache: Err(CacheError::NotFound),
+            candidates,
+            features: ObjectFeatures::default(),
+        }
+    };
+
+    // Fetch cache file from handle
+    let derived_cache = derive(Arc::clone(&handle)).await;
+
+    match derived_cache {
+        Ok(cache) => {
+            candidates.set_status(
+                candidate_status,
+                handle.source_id(),
+                &handle.uri(),
+                ObjectUseInfo::from_derived_status(
+                    &cache_entry_as_cache_status(&cache),
+                    handle.status(),
+                ),
+            );
+
+            DerivedCache {
+                cache: Arc::try_unwrap(cache).unwrap_or_else(|arc| (*arc).clone()),
+                candidates,
+                features: handle.features(),
+            }
+        }
+
+        // TODO: Internal Err fetching from cache, this should really be merged with `Ok`, as this is
+        // essentially a `Result<Result>` right now.
+        // `Cacher::compute_memoized` should just convert all the internal caching-related errors to
+        // `CacheError` instead of taking a detour through a type parameter.
+        Err(e) => DerivedCache {
+            cache: Err(e.as_ref().into()),
+            candidates,
+            features: ObjectFeatures::default(),
+        },
+    }
+}
+
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct FilesystemSharedCacheConfig {
     pub path: PathBuf,

crates/symbolicator-service/src/services/cficaches.rs

@@ -13,15 +13,14 @@ use symbolic::common::ByteView;
 use symbolicator_sources::{FileType, ObjectId, ObjectType, SourceConfig};
 
 use crate::cache::{
-    cache_entry_as_cache_status, cache_entry_from_cache_status, Cache, CacheEntry, CacheError,
-    CacheStatus, ExpirationTime,
+    cache_entry_from_cache_status, derive_from_object_handle, Cache, CacheEntry, CacheError,
+    CacheStatus, DerivedCache, ExpirationTime,
 };
 use crate::services::cacher::{CacheItemRequest, CacheKey, CacheVersions, Cacher};
 use crate::services::objects::{
-    FindObject, FoundObject, ObjectError, ObjectHandle, ObjectMetaHandle, ObjectPurpose,
-    ObjectsActor,
+    FindObject, ObjectError, ObjectHandle, ObjectMetaHandle, ObjectPurpose, ObjectsActor,
 };
-use crate::types::{AllObjectCandidates, ObjectFeatures, ObjectUseInfo, Scope};
+use crate::types::{CandidateStatus, Scope};
 use crate::utils::futures::{m, measure};
 use crate::utils::sentry::ConfigureScope;
 
@@ -142,38 +141,6 @@ impl CfiCacheActor {
     }
 }
 
-#[derive(Debug)]
-pub struct CfiCacheFile {
-    features: ObjectFeatures,
-    status: CacheStatus,
-    data: ByteView<'static>,
-    candidates: AllObjectCandidates,
-}
-
-impl CfiCacheFile {
-    /// Returns the status of this cache file.
-    pub fn status(&self) -> &CacheStatus {
-        &self.status
-    }
-
-    /// Returns the features of the object file this symcache was constructed from.
-    pub fn features(&self) -> ObjectFeatures {
-        self.features
-    }
-
-    /// Returns the cfi contents as a [`ByteView`].
-    // FIXME(swatinem): symbolic `CfiCache::from_bytes` should actually take a `&[u8]` instead of
-    // an explicit `ByteView`.
-    pub fn data(&self) -> ByteView {
-        self.data.clone()
-    }
-
-    /// Returns all the DIF object candidates.
-    pub fn candidates(&self) -> &AllObjectCandidates {
-        &self.candidates
-    }
-}
-
 #[derive(Clone, Debug)]
 struct FetchCfiCacheInternal {
     request: FetchCfiCache,
@@ -262,22 +229,6 @@ impl CacheItemRequest for FetchCfiCacheInternal {
         cache_entry.and_then(parse_cfi_cache)
     }
 }
-#[derive(Debug, Clone)]
-pub struct FetchCfiCacheResponse {
-    pub cache: CacheEntry<Option<Arc<SymbolFile>>>,
-    pub candidates: AllObjectCandidates,
-    pub features: ObjectFeatures,
-}
-
-impl Default for FetchCfiCacheResponse {
-    fn default() -> Self {
-        Self {
-            cache: Err(CacheError::InternalError),
-            candidates: Default::default(),
-            features: Default::default(),
-        }
-    }
-}
 
 /// Information for fetching the symbols for this cficache
 #[derive(Debug, Clone)]
@@ -288,66 +239,35 @@ pub struct FetchCfiCache {
     pub scope: Scope,
 }
 
+pub type FetchedCfiCache = DerivedCache<Option<Arc<SymbolFile>>>;
+
 impl CfiCacheActor {
     /// Fetches the CFI cache file for a given code module.
     ///
     /// The code object can be identified by a combination of the code-id, debug-id and
     /// debug filename (the basename).  To do this it looks in the existing cache with the
     /// given scope and if it does not yet exist in cached form will fetch the required DIFs
     /// and compute the required CFI cache file.
-    pub async fn fetch(&self, request: FetchCfiCache) -> FetchCfiCacheResponse {
-        let Ok(FoundObject { meta, mut candidates }) = self
+    pub async fn fetch(&self, request: FetchCfiCache) -> FetchedCfiCache {
+        let found_object = self
             .objects
             .find(FindObject {
                 filetypes: FileType::from_object_type(request.object_type),
                 identifier: request.identifier.clone(),
                 sources: request.sources.clone(),
                 scope: request.scope.clone(),
                 purpose: ObjectPurpose::Unwind,
-            }).await else {
-                return FetchCfiCacheResponse::default();
-            };
-
-        match meta {
-            Some(meta_handle) => {
-                match self
-                    .cficaches
-                    .compute_memoized(FetchCfiCacheInternal {
-                        request,
-                        objects_actor: self.objects.clone(),
-                        meta_handle: Arc::clone(&meta_handle),
-                    })
-                    .await
-                {
-                    Ok(symbolfile) => {
-                        candidates.set_unwind(
-                            meta_handle.source_id().clone(),
-                            &meta_handle.uri(),
-                            ObjectUseInfo::from_derived_status(
-                                &cache_entry_as_cache_status(&symbolfile),
-                                meta_handle.status(),
-                            ),
-                        );
-
-                        FetchCfiCacheResponse {
-                            cache: Arc::try_unwrap(symbolfile).unwrap_or_else(|arc| (*arc).clone()),
-                            candidates,
-                            features: meta_handle.features(),
-                        }
-                    }
-                    Err(e) => FetchCfiCacheResponse {
-                        cache: Err(e.as_ref().into()),
-                        candidates,
-                        features: ObjectFeatures::default(),
-                    },
-                }
-            }
-            None => FetchCfiCacheResponse {
-                cache: Err(CacheError::NotFound),
-                candidates,
-                features: ObjectFeatures::default(),
-            },
-        }
+            })
+            .await;
+
+        derive_from_object_handle(found_object, CandidateStatus::Unwind, |meta_handle| {
+            self.cficaches.compute_memoized(FetchCfiCacheInternal {
+                request,
+                objects_actor: self.objects.clone(),
+                meta_handle,
+            })
+        })
+        .await
     }
 }
 

crates/symbolicator-service/src/services/ppdb_caches.rs

@@ -13,18 +13,16 @@ use symbolic::ppdb::{PortablePdbCache, PortablePdbCacheConverter};
 use symbolicator_sources::{FileType, ObjectId, SourceConfig};
 
 use crate::cache::{
-    cache_entry_as_cache_status, cache_entry_from_cache_status, Cache, CacheEntry, CacheError,
-    CacheStatus, ExpirationTime,
+    cache_entry_from_cache_status, derive_from_object_handle, Cache, CacheEntry, CacheError,
+    CacheStatus, DerivedCache, ExpirationTime,
 };
 use crate::services::objects::ObjectError;
-use crate::types::{AllObjectCandidates, ObjectFeatures, ObjectUseInfo, Scope};
+use crate::types::{CandidateStatus, Scope};
 use crate::utils::futures::{m, measure};
 use crate::utils::sentry::ConfigureScope;
 
 use super::cacher::{CacheItemRequest, CacheKey, CacheVersions, Cacher};
-use super::objects::{
-    FindObject, FoundObject, ObjectHandle, ObjectMetaHandle, ObjectPurpose, ObjectsActor,
-};
+use super::objects::{FindObject, ObjectHandle, ObjectMetaHandle, ObjectPurpose, ObjectsActor};
 use super::shared_cache::SharedCacheService;
 
 /// The supported ppdb_cache versions.
@@ -119,23 +117,6 @@ pub struct FetchPortablePdbCache {
     pub scope: Scope,
 }
 
-#[derive(Debug, Clone)]
-pub struct FetchPortablePdbCacheResponse {
-    pub cache: CacheEntry<OwnedPortablePdbCache>,
-    pub candidates: AllObjectCandidates,
-    pub features: ObjectFeatures,
-}
-
-impl Default for FetchPortablePdbCacheResponse {
-    fn default() -> Self {
-        Self {
-            cache: Err(CacheError::InternalError),
-            candidates: Default::default(),
-            features: Default::default(),
-        }
-    }
-}
-
 #[derive(Clone, Debug)]
 pub struct PortablePdbCacheActor {
     ppdb_caches: Arc<Cacher<FetchPortablePdbCacheInternal>>,
@@ -154,8 +135,11 @@ impl PortablePdbCacheActor {
         }
     }
 
-    pub async fn fetch(&self, request: FetchPortablePdbCache) -> FetchPortablePdbCacheResponse {
-        let Ok(FoundObject { meta, mut candidates }) = self
+    pub async fn fetch(
+        &self,
+        request: FetchPortablePdbCache,
+    ) -> DerivedCache<OwnedPortablePdbCache> {
+        let found_object = self
             .objects
             .find(FindObject {
                 filetypes: &[FileType::PortablePdb],
@@ -164,51 +148,16 @@ impl PortablePdbCacheActor {
                 scope: request.scope.clone(),
                 purpose: ObjectPurpose::Debug,
             })
-            .await else {
-                return FetchPortablePdbCacheResponse::default()
-        };
-
-        match meta {
-            Some(handle) => {
-                match self
-                    .ppdb_caches
-                    .compute_memoized(FetchPortablePdbCacheInternal {
-                        request,
-                        objects_actor: self.objects.clone(),
-                        object_meta: Arc::clone(&handle),
-                    })
-                    .await
-                {
-                    Ok(ppdb_cache_file) => {
-                        candidates.set_debug(
-                            handle.source_id(),
-                            &handle.uri(),
-                            ObjectUseInfo::from_derived_status(
-                                &cache_entry_as_cache_status(&ppdb_cache_file),
-                                handle.status(),
-                            ),
-                        );
-
-                        FetchPortablePdbCacheResponse {
-                            cache: Arc::try_unwrap(ppdb_cache_file)
-                                .unwrap_or_else(|arc| (*arc).clone()),
-                            candidates,
-                            features: handle.features(),
-                        }
-                    }
-                    Err(e) => FetchPortablePdbCacheResponse {
-                        cache: Err(e.as_ref().into()),
-                        candidates,
-                        features: ObjectFeatures::default(),
-                    },
-                }
-            }
-            None => FetchPortablePdbCacheResponse {
-                cache: Err(CacheError::NotFound),
-                candidates,
-                features: ObjectFeatures::default(),
-            },
-        }
+            .await;
+        derive_from_object_handle(found_object, CandidateStatus::Debug, |object_meta| {
+            self.ppdb_caches
+                .compute_memoized(FetchPortablePdbCacheInternal {
+                    request,
+                    objects_actor: self.objects.clone(),
+                    object_meta,
+                })
+        })
+        .await
     }
 }