feat: Create object candidates for sourcelink downloads (#1507)

This integrates sourcelink downloads into the object
candidate machinery. Previously, if a download from
a sourcelink failed, there would be no indication that
anything had gone wrong. Now, whenever we try to
download a source file from a sourcelink, we create
a "sourcelink" candidate and add it to the module's
candidates. Since the object candidate system was not
set up for adding candidates after the fact, this
requires some tinkering.

CHANGELOG.md

@@ -2,6 +2,11 @@
 
 ## Unreleased
 
+### Various fixes & improvements
+
+- In native symbolication, trying to download a source file from a
+  sourcelink now creates a candidate ([#1507](https://github.com/getsentry/symbolicator/pull/1510)).
+
 ### Dependencies
 
 - Bump Native SDK from v0.7.7 to v0.7.8 ([#1510](https://github.com/getsentry/symbolicator/pull/1510))

crates/symbolicator-native/src/symbolication/module_lookup.rs

@@ -7,8 +7,8 @@ use sentry::{Hub, SentryFutureExt};
 use symbolic::debuginfo::ObjectDebugSession;
 use symbolicator_service::caching::{CacheEntry, CacheError};
 use symbolicator_service::objects::{
-    AllObjectCandidates, FindObject, FindResult, ObjectFeatures, ObjectHandle, ObjectPurpose,
-    ObjectsActor,
+    AllObjectCandidates, FindObject, FindResult, ObjectCandidate, ObjectFeatures, ObjectHandle,
+    ObjectPurpose, ObjectsActor,
 };
 use symbolicator_service::source_context::get_context_lines;
 use symbolicator_service::types::{ObjectFileStatus, RawObjectInfo, Scope};
@@ -389,12 +389,14 @@ impl ModuleLookup {
     }
 
     /// Update the frame with source context, if available.
-    /// Returns an `Url` in case the source code has to be fetched.
+    ///
+    /// Return a triple of scope, URL, and module index
+    /// in case the source code has to be fetched.
     pub(crate) fn try_set_source_context(
         &self,
         debug_sessions: &DebugSessions<'_>,
         frame: &mut RawFrame,
-    ) -> Option<(Scope, url::Url)> {
+    ) -> Option<(Scope, url::Url, usize)> {
         let abs_path = frame.abs_path.as_ref()?;
 
         // Short-circuit here before accessing the source. Line number is required to resolve the context.
@@ -425,7 +427,7 @@ impl ModuleLookup {
             None
         } else {
             // Let caller know this source code may be resolved from a remote URL.
-            filtered_url.map(|url| (session.0.clone(), url))
+            filtered_url.map(|url| (session.0.clone(), url, entry.module_index))
         }
     }
 
@@ -472,6 +474,17 @@ impl ModuleLookup {
                 .find(|entry| entry.module_index == this_module_index),
         }
     }
+
+    /// Adds an `ObjectCandidate` to the module with index `idx`.
+    ///
+    /// If there is no module at that index, this is a no-op.
+    pub fn add_candidate(&mut self, idx: usize, candidate: &ObjectCandidate) {
+        let Some(entry) = self.modules.get_mut(idx) else {
+            return;
+        };
+
+        entry.object_info.candidates.merge_one(candidate);
+    }
 }
 
 #[cfg(test)]

crates/symbolicator-native/src/symbolication/source_context.rs

@@ -1,9 +1,13 @@
 use std::collections::HashMap;
 
 use futures::future;
+use symbolicator_service::caching::{CacheEntry, CacheError};
+use symbolicator_service::objects::{
+    ObjectCandidate, ObjectDownloadInfo, ObjectFeatures, ObjectUseInfo,
+};
 use symbolicator_service::types::{Scope, ScrapingConfig};
 use symbolicator_service::utils::http::is_valid_origin;
-use symbolicator_sources::HttpRemoteFile;
+use symbolicator_sources::{HttpRemoteFile, RemoteFileUri, SourceId};
 
 use crate::interface::{CompleteStacktrace, RawFrame};
 
@@ -21,56 +25,104 @@ impl SymbolicationActor {
             .fetch_sources(self.objects.clone(), stacktraces)
             .await;
 
-        // Map collected source contexts to frames and collect URLs for remote source links.
-        let mut remote_sources: HashMap<(Scope, url::Url), Vec<&mut RawFrame>> = HashMap::new();
+        // Map collected source contexts to the index of the module
+        // and the list of frames they belong to and collect URLs
+        // for remote source links.
+        let mut remote_sources: HashMap<(Scope, url::Url), (usize, Vec<&mut RawFrame>)> =
+            HashMap::new();
         {
             let debug_sessions = module_lookup.prepare_debug_sessions();
 
             for trace in stacktraces {
                 for frame in &mut trace.frames {
-                    let Some(url) =
+                    let Some((scope, url, module_idx)) =
                         module_lookup.try_set_source_context(&debug_sessions, &mut frame.raw)
                     else {
                         continue;
                     };
-                    if let Some(vec) = remote_sources.get_mut(&url) {
-                        vec.push(&mut frame.raw)
-                    } else {
-                        remote_sources.insert(url, vec![&mut frame.raw]);
-                    }
+                    let (_idx, frames) = remote_sources
+                        .entry((scope, url))
+                        .or_insert((module_idx, vec![]));
+                    frames.push(&mut frame.raw);
                 }
             }
         }
 
         // Download remote sources and update contexts.
         if !remote_sources.is_empty() {
             let cache = self.sourcefiles_cache.as_ref();
-            let futures =
-                remote_sources
-                    .into_iter()
-                    .map(|((source_scope, url), frames)| async move {
-                        let mut remote_file =
-                            HttpRemoteFile::from_url(url.clone(), scraping.verify_ssl);
+            let futures = remote_sources.into_iter().map(
+                |((source_scope, url), (module_idx, frames))| async move {
+                    let mut remote_file =
+                        HttpRemoteFile::from_url(url.clone(), scraping.verify_ssl);
 
-                        if scraping.enabled && is_valid_origin(&url, &scraping.allowed_origins) {
-                            remote_file.headers.extend(
-                                scraping
-                                    .headers
-                                    .iter()
-                                    .map(|(key, value)| (key.clone(), value.clone())),
-                            );
-                        }
+                    if scraping.enabled && is_valid_origin(&url, &scraping.allowed_origins) {
+                        remote_file.headers.extend(
+                            scraping
+                                .headers
+                                .iter()
+                                .map(|(key, value)| (key.clone(), value.clone())),
+                        );
+                    }
+
+                    let uri = remote_file.uri();
+                    let res = cache
+                        .fetch_file(&source_scope, remote_file.into(), true)
+                        .await;
 
-                        if let Ok(source) = cache
-                            .fetch_file(&source_scope, remote_file.into(), true)
-                            .await
-                        {
-                            for frame in frames {
-                                ModuleLookup::set_source_context(&source, frame);
-                            }
+                    if let Ok(source) = res.as_ref() {
+                        for frame in frames {
+                            ModuleLookup::set_source_context(source, frame);
                         }
-                    });
-            future::join_all(futures).await;
+                    }
+
+                    (module_idx, Self::object_candidate_for_sourcelink(uri, res))
+                },
+            );
+
+            let candidates = future::join_all(futures).await;
+
+            // Merge the candidates resulting from source downloads into the
+            // respective objects.
+            for (module_idx, candidate) in candidates {
+                module_lookup.add_candidate(module_idx, &candidate);
+            }
+        }
+    }
+
+    // Creates an `ObjectCandidate` based on trying to download a
+    // source file from a link.
+    fn object_candidate_for_sourcelink<T>(
+        location: RemoteFileUri,
+        res: CacheEntry<T>,
+    ) -> ObjectCandidate {
+        let source = SourceId::new("sourcelink");
+        let unwind = ObjectUseInfo::None;
+        let debug = ObjectUseInfo::None;
+        let download = match res {
+            Ok(_) => ObjectDownloadInfo::Ok {
+                features: ObjectFeatures {
+                    has_sources: true,
+                    ..Default::default()
+                },
+            },
+            // Same logic as in `create_candidate_info`.
+            Err(error) => match error {
+                CacheError::NotFound => ObjectDownloadInfo::NotFound,
+                CacheError::PermissionDenied(details) => ObjectDownloadInfo::NoPerm { details },
+                CacheError::Malformed(_) => ObjectDownloadInfo::Malformed,
+                err => ObjectDownloadInfo::Error {
+                    details: err.to_string(),
+                },
+            },
+        };
+
+        ObjectCandidate {
+            source,
+            location,
+            download,
+            unwind,
+            debug,
         }
     }
 }

crates/symbolicator-native/tests/integration/snapshots/integration__public_sources__nuget_source.snap

@@ -1,6 +1,5 @@
 ---
-source: crates/symbolicator-service/tests/integration/public_sources.rs
-assertion_line: 48
+source: crates/symbolicator-native/tests/integration/public_sources.rs
 expression: response.unwrap()
 ---
 stacktraces:
@@ -56,4 +55,12 @@ modules:
             has_sources: true
         debug:
           status: ok
-
+      - source: sourcelink
+        location: "https://raw.githubusercontent.com/mattjohnsonpint/TimeZoneConverter/dab355a1e878bfbfd4c659ddb568ca69961d579e/src/TimeZoneConverter/TZConvert.cs"
+        download:
+          status: ok
+          features:
+            has_debug_info: false
+            has_unwind_info: false
+            has_symbols: false
+            has_sources: true

crates/symbolicator-native/tests/integration/snapshots/integration__symbolication__dotnet_only_source_links.snap

@@ -1,6 +1,5 @@
 ---
-source: crates/symbolicator-service/tests/integration/symbolication.rs
-assertion_line: 226
+source: crates/symbolicator-native/tests/integration/symbolication.rs
 expression: response.unwrap()
 ---
 stacktraces:
@@ -54,4 +53,12 @@ modules:
         location: "http://localhost:<port>/symbols/source-links.pdb/0C380A12822140698565BEE6B3AC196Effffffff/source-links.src.zip"
         download:
           status: notfound
-
+      - source: sourcelink
+        location: "https://raw.githubusercontent.com/getsentry/sentry-dotnet/b31b62192e6934ea04396456461f430e143cf4f9/samples/Sentry.Samples.Console.Basic/Program.cs"
+        download:
+          status: ok
+          features:
+            has_debug_info: false
+            has_unwind_info: false
+            has_symbols: false
+            has_sources: true

crates/symbolicator-native/tests/integration/snapshots/integration__symbolication__dotnet_source_links.snap

@@ -1,6 +1,5 @@
 ---
-source: crates/symbolicator-service/tests/integration/symbolication.rs
-assertion_line: 201
+source: crates/symbolicator-native/tests/integration/symbolication.rs
 expression: response.unwrap()
 ---
 stacktraces:
@@ -60,4 +59,12 @@ modules:
         location: "http://localhost:<port>/symbols/source-links.pdb/37E9E8A61A8E404EB93C6902E277FF55ffffffff/source-links.src.zip"
         download:
           status: notfound
-
+      - source: sourcelink
+        location: "https://raw.githubusercontent.com/dotnet/runtime/d099f075e45d2aa6007a22b71b45a08758559f80/src/libraries/Common/src/System/ThrowHelper.cs"
+        download:
+          status: ok
+          features:
+            has_debug_info: false
+            has_unwind_info: false
+            has_symbols: false
+            has_sources: true

crates/symbolicator-service/src/objects/candidates.rs

@@ -222,25 +222,36 @@ impl AllObjectCandidates {
     /// `other` if they are not [`ObjectUseInfo::None`].
     pub fn merge(&mut self, other: &AllObjectCandidates) {
         for other_info in &other.0 {
-            let key = (&other_info.source, &other_info.location);
-            let found_pos = self
-                .0
-                .binary_search_by_key(&key, |candidate| (&candidate.source, &candidate.location));
-            match found_pos {
-                Ok(index) => {
-                    if let Some(info) = self.0.get_mut(index) {
-                        info.download = other_info.download.clone();
-                        if other_info.unwind != ObjectUseInfo::None {
-                            info.unwind = other_info.unwind.clone();
-                        }
-                        if other_info.debug != ObjectUseInfo::None {
-                            info.debug = other_info.debug.clone();
-                        }
+            self.merge_one(other_info);
+        }
+    }
+
+    /// Merges another candidate into this collection.
+    ///
+    /// If `candidate` already existed in the collection all data which is present in both
+    /// will be overwritten by the data in `candidate`.  Practically that means
+    /// [`ObjectCandidate::download`] will be overwritten by `candidate` and for
+    /// [`ObjectCandidate::unwind`] and [`ObjectCandidate::debug`] it will be overwritten by
+    /// `candidate` if they are not [`ObjectUseInfo::None`].
+    pub fn merge_one(&mut self, candidate: &ObjectCandidate) {
+        let key = (&candidate.source, &candidate.location);
+        let found_pos = self
+            .0
+            .binary_search_by_key(&key, |candidate| (&candidate.source, &candidate.location));
+        match found_pos {
+            Ok(index) => {
+                if let Some(info) = self.0.get_mut(index) {
+                    info.download = candidate.download.clone();
+                    if candidate.unwind != ObjectUseInfo::None {
+                        info.unwind = candidate.unwind.clone();
+                    }
+                    if candidate.debug != ObjectUseInfo::None {
+                        info.debug = candidate.debug.clone();
                     }
                 }
-                Err(index) => {
-                    self.0.insert(index, other_info.clone());
-                }
+            }
+            Err(index) => {
+                self.0.insert(index, candidate.clone());
             }
         }
     }

crates/symbolicator-sources/src/sources/http.rs

@@ -72,7 +72,8 @@ impl HttpRemoteFile {
         HttpRemoteFile::new(source, location)
     }
 
-    pub(crate) fn uri(&self) -> RemoteFileUri {
+    /// Returns a [`RemoteFileUri`] for the file.
+    pub fn uri(&self) -> RemoteFileUri {
         match self.url() {
             Ok(url) => url.as_ref().into(),
             Err(_) => "".into(),