Backfill missing DebugId from CFI Cache (#1305)

We have seen customer minidumps that do not have proper DebugIds in them.
However, there is a valid CodeId, which would allow the executable to be fetched from an external SSQP symbol server.
We transform this executable to a CfiCache, which in theory could have a breakpad `MODULE` entry. Though in practice it does not (yet), as we never output one.

This would allow backfilling the missing DebugID in the minidump with the one found in the executable that we use to generate the CfiCache.

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

@@ -4,10 +4,12 @@ use std::sync::Arc;
 
 use futures::future::BoxFuture;
 use minidump_unwind::SymbolFile;
+use sentry::types::DebugId;
 use tempfile::NamedTempFile;
 
 use symbolic::cfi::CfiCache;
 use symbolic::common::ByteView;
+use symbolic::debuginfo::breakpad::BreakpadModuleRecord;
 use symbolicator_sources::{FileType, ObjectId, ObjectType, SourceConfig};
 
 use crate::caching::{
@@ -22,10 +24,15 @@ use crate::utils::sentry::ConfigureScope;
 use super::caches::versions::CFICACHE_VERSIONS;
 use super::derived::{derive_from_object_handle, DerivedCache};
 
-type CfiItem = (u32, Option<Arc<SymbolFile>>);
+type CfiItem = Option<Arc<(SymbolFile, Option<CfiModuleInfo>)>>;
+
+pub struct CfiModuleInfo {
+    pub debug_id: DebugId,
+    pub debug_file: String,
+}
 
 #[tracing::instrument(skip_all)]
-fn parse_cfi_cache(bytes: ByteView<'static>) -> CacheEntry<CfiItem> {
+fn parse_cfi_cache(bytes: ByteView<'static>) -> CacheEntry<(u32, CfiItem)> {
     let weight = bytes.len().try_into().unwrap_or(u32::MAX);
     // NOTE: we estimate the in-memory structures to be ~8x as heavy in memory as on disk
     let weight = weight.saturating_mul(8);
@@ -39,7 +46,23 @@ fn parse_cfi_cache(bytes: ByteView<'static>) -> CacheEntry<CfiItem> {
     let symbol_file =
         SymbolFile::from_bytes(cfi_cache.as_slice()).map_err(CacheError::from_std_error)?;
 
-    Ok((weight, Some(Arc::new(symbol_file))))
+    let first_line = cfi_cache
+        .as_slice()
+        .split(|b| *b == b'\n')
+        .next()
+        .unwrap_or_default();
+    let cfi_module = BreakpadModuleRecord::parse(first_line)
+        .ok()
+        .and_then(|module| {
+            let debug_id = module.id.parse().ok()?;
+            let debug_file = module.name.into();
+            Some(CfiModuleInfo {
+                debug_id,
+                debug_file,
+            })
+        });
+
+    Ok((weight, Some(Arc::new((symbol_file, cfi_module)))))
 }
 
 #[derive(Clone, Debug)]
@@ -79,7 +102,7 @@ async fn compute_cficache(
 }
 
 impl CacheItemRequest for FetchCfiCacheInternal {
-    type Item = CfiItem;
+    type Item = (u32, CfiItem);
 
     const VERSIONS: CacheVersions = CFICACHE_VERSIONS;
 
@@ -109,7 +132,7 @@ pub struct FetchCfiCache {
     pub scope: Scope,
 }
 
-pub type FetchedCfiCache = DerivedCache<Option<Arc<SymbolFile>>>;
+pub type FetchedCfiCache = DerivedCache<CfiItem>;
 
 impl CfiCacheActor {
     /// Fetches the CFI cache file for a given code module.

crates/symbolicator-service/src/services/symbolication/process_minidump.rs

@@ -20,7 +20,7 @@ use tempfile::TempPath;
 use symbolic::common::{Arch, ByteView, CodeId, DebugId};
 use symbolicator_sources::{ObjectId, ObjectType, SourceConfig};
 
-use crate::services::cficaches::{CfiCacheActor, FetchCfiCache, FetchedCfiCache};
+use crate::services::cficaches::{CfiCacheActor, CfiModuleInfo, FetchCfiCache, FetchedCfiCache};
 use crate::services::minidump::parse_stacktraces_from_minidump;
 use crate::services::module_lookup::object_file_status_from_cache_entry;
 use crate::types::{
@@ -212,7 +212,7 @@ impl SymbolicatorSymbolProvider {
 impl SymbolProvider for SymbolicatorSymbolProvider {
     async fn fill_symbol(
         &self,
-        _module: &(dyn Module + Sync),
+        module: &(dyn Module + Sync),
         _frame: &mut (dyn FrameSymbolizer + Send),
     ) -> Result<(), FillSymbolError> {
         // Always return an error here to signal that we have no useful symbol information to
@@ -221,6 +221,14 @@ impl SymbolProvider for SymbolicatorSymbolProvider {
         // See https://github.com/rust-minidump/rust-minidump/blob/7eed71e4075e0a81696ccc307d6ac68920de5db5/minidump-processor/src/stackwalker/mod.rs#L295.
         //
         // TODO: implement this properly, i.e., use symbolic to actually fill in information.
+
+        // This function is being called for every context frame,
+        // regardless if any stack walking happens.
+        // In contrast, `walk_frame` below will be skipped in case the minidump
+        // does not contain any actionable stack memory that would allow stack walking.
+        // Loading this module here means we will be able to backfill a possibly missing
+        // debug_id/file.
+        self.load_cfi_module(module).await;
         Err(FillSymbolError {})
     }
 
@@ -230,7 +238,7 @@ impl SymbolProvider for SymbolicatorSymbolProvider {
         walker: &mut (dyn FrameWalker + Send),
     ) -> Option<()> {
         let cfi_module = self.load_cfi_module(module).await;
-        cfi_module.cache.ok()??.walk_frame(module, walker)
+        cfi_module.cache.ok()??.0.walk_frame(module, walker)
     }
 
     async fn get_file_path(
@@ -355,6 +363,15 @@ async fn stackwalk(
                     // NOTE: minidump stackwalking is the first thing that happens to a request,
                     // hence the current candidate list is empty.
                     obj_info.candidates = cfi_module.candidates;
+
+                    // if the debug_id/file is empty, it might be possible to
+                    // backfill that using the reference in the executable file.
+                    if let Ok(Some(cfi_item)) = &cfi_module.cache {
+                        if let Some(cfi_module_info) = &cfi_item.1 {
+                            maybe_backfill_debugid(&mut obj_info.raw, cfi_module_info);
+                        }
+                    }
+
                     object_file_status_from_cache_entry(&cfi_module.cache)
                 }
             });
@@ -376,6 +393,15 @@ async fn stackwalk(
     })
 }
 
+fn maybe_backfill_debugid(info: &mut RawObjectInfo, cfi_module: &CfiModuleInfo) {
+    if info.debug_id.is_none() {
+        info.debug_id = Some(cfi_module.debug_id.to_string());
+    }
+    if info.debug_file.is_none() {
+        info.debug_file = Some(cfi_module.debug_file.clone());
+    }
+}
+
 impl SymbolicationActor {
     /// Saves the given `minidump_file` in the diagnostics cache if configured to do so.
     fn maybe_persist_minidump(&self, minidump_file: TempPath) {