From 737066ed07463cfcb6eedb8fc7ace886fb2cf9f9 Mon Sep 17 00:00:00 2001 From: David Herberth Date: Wed, 6 May 2026 09:29:21 +0200 Subject: [PATCH 1/4] use a cache error --- .../src/symbolication/attachments.rs | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/crates/symbolicator-native/src/symbolication/attachments.rs b/crates/symbolicator-native/src/symbolication/attachments.rs index e206e4106..5d5f5cdf6 100644 --- a/crates/symbolicator-native/src/symbolication/attachments.rs +++ b/crates/symbolicator-native/src/symbolication/attachments.rs @@ -1,17 +1,16 @@ use std::fs::File; -use std::pin::pin; use futures::TryStreamExt; +use symbolicator_service::caching::CacheError; use symbolicator_service::download::DownloadService; use tokio::io::{AsyncSeekExt, AsyncWriteExt, BufWriter}; -use tokio_util::io::StreamReader; use crate::interface::AttachmentFile; pub async fn download_attachment( download_svc: &DownloadService, file: AttachmentFile, -) -> anyhow::Result { +) -> Result { let (storage_url, storage_token) = match file { AttachmentFile::Local(file) => return Ok(file), AttachmentFile::Remote { @@ -29,17 +28,13 @@ pub async fn download_attachment( if let Some(token) = storage_token { request = request.bearer_auth(token); } - let stream = request - .send() - .await? - .error_for_status()? - .bytes_stream() - .map_err(std::io::Error::other); - let mut reader = pin!(StreamReader::new(stream)); + let mut stream = request.send().await?.error_for_status()?.bytes_stream(); let file = tempfile::tempfile()?; let mut writer = BufWriter::new(tokio::fs::File::from_std(file)); - tokio::io::copy(&mut reader, &mut writer).await?; + while let Some(chunk) = stream.try_next().await? { + writer.write_all(&chunk).await?; + } writer.flush().await?; let mut file = writer.into_inner(); file.sync_data().await?; From c267f1fddfd8fd84216ec5c447ee4cd93fa7ae9e Mon Sep 17 00:00:00 2001 From: David Herberth Date: Wed, 6 May 2026 09:31:57 +0200 Subject: [PATCH 2/4] use retry --- .../src/symbolication/attachments.rs | 45 ++++++++++--------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/crates/symbolicator-native/src/symbolication/attachments.rs b/crates/symbolicator-native/src/symbolication/attachments.rs index 5d5f5cdf6..e76fe6f47 100644 --- a/crates/symbolicator-native/src/symbolication/attachments.rs +++ b/crates/symbolicator-native/src/symbolication/attachments.rs @@ -2,7 +2,7 @@ use std::fs::File; use futures::TryStreamExt; use symbolicator_service::caching::CacheError; -use symbolicator_service::download::DownloadService; +use symbolicator_service::download::{self, DownloadService}; use tokio::io::{AsyncSeekExt, AsyncWriteExt, BufWriter}; use crate::interface::AttachmentFile; @@ -19,27 +19,30 @@ pub async fn download_attachment( } => (storage_url, storage_token), }; - // TODO: maybe its worth using the actual `DownloadService` instead of straight going to the `trusted_client`. - // Doing so would in theory allow us to have retries and error report, as well as being able to - // download files in multiple chunks concurrently, but I don’t think our `objecstore` server currently - // supports range requests, and those would also mess with streaming decompression. - // Not to mention that using the `DownloadService` is not that straight forward. - let mut request = download_svc.trusted_client.get(storage_url); - if let Some(token) = storage_token { - request = request.bearer_auth(token); - } - let mut stream = request.send().await?.error_for_status()?.bytes_stream(); + download::retry(|| async { + // TODO: maybe its worth using the actual `DownloadService` instead of straight going to the `trusted_client`. + // Doing so would in theory allow us to have retries and error report, as well as being able to + // download files in multiple chunks concurrently, but I don’t think our `objecstore` server currently + // supports range requests, and those would also mess with streaming decompression. + // Not to mention that using the `DownloadService` is not that straight forward. + let mut request = download_svc.trusted_client.get(&storage_url); + if let Some(token) = storage_token.as_ref() { + request = request.bearer_auth(token); + } + let mut stream = request.send().await?.error_for_status()?.bytes_stream(); - let file = tempfile::tempfile()?; - let mut writer = BufWriter::new(tokio::fs::File::from_std(file)); - while let Some(chunk) = stream.try_next().await? { - writer.write_all(&chunk).await?; - } - writer.flush().await?; - let mut file = writer.into_inner(); - file.sync_data().await?; + let file = tempfile::tempfile()?; + let mut writer = BufWriter::new(tokio::fs::File::from_std(file)); + while let Some(chunk) = stream.try_next().await? { + writer.write_all(&chunk).await?; + } + writer.flush().await?; + let mut file = writer.into_inner(); + file.sync_data().await?; - file.rewind().await?; + file.rewind().await?; - Ok(file.into_std().await) + Ok(file.into_std().await) + }) + .await } From fa4304d513e15cf3210e8b5032863c0e30076039 Mon Sep 17 00:00:00 2001 From: David Herberth Date: Wed, 6 May 2026 10:14:52 +0200 Subject: [PATCH 3/4] generic error handler --- .../src/symbolication/attachments.rs | 9 ++++++++- crates/symbolicator-service/src/download/mod.rs | 15 +++++++++++---- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/crates/symbolicator-native/src/symbolication/attachments.rs b/crates/symbolicator-native/src/symbolication/attachments.rs index e76fe6f47..cfcfd30b5 100644 --- a/crates/symbolicator-native/src/symbolication/attachments.rs +++ b/crates/symbolicator-native/src/symbolication/attachments.rs @@ -29,7 +29,14 @@ pub async fn download_attachment( if let Some(token) = storage_token.as_ref() { request = request.bearer_auth(token); } - let mut stream = request.send().await?.error_for_status()?.bytes_stream(); + let response = request.send().await?; + if !response.status().is_success() { + return Err( + download::GenericErrorHandler::handle_response(&storage_url, response).await, + ); + } + + let mut stream = response.bytes_stream(); let file = tempfile::tempfile()?; let mut writer = BufWriter::new(tokio::fs::File::from_std(file)); diff --git a/crates/symbolicator-service/src/download/mod.rs b/crates/symbolicator-service/src/download/mod.rs index 8d33f7620..e73838c18 100644 --- a/crates/symbolicator-service/src/download/mod.rs +++ b/crates/symbolicator-service/src/download/mod.rs @@ -633,14 +633,15 @@ async fn do_download_reqwest( /// /// This error handler uses the HTTP status code to infer the [`CacheError`], /// this works for any HTTP request, but does not consider API specific responses. -struct GenericErrorHandler; +pub struct GenericErrorHandler; -impl ErrorHandler for GenericErrorHandler { - async fn handle(&self, source: &str, response: SymResponse<'_>) -> CacheError { +impl GenericErrorHandler { + /// Converts an unsuccessful HTTP response to a [`CacheError`]. + pub async fn handle_response(source: &str, response: reqwest::Response) -> CacheError { let status = response.status(); debug_assert!(!status.is_success()); - if let Ok(details) = response.response.text().await { + if let Ok(details) = response.text().await { ::sentry::configure_scope(|scope| { scope.set_extra( "reqwest_response_body", @@ -677,6 +678,12 @@ impl ErrorHandler for GenericErrorHandler { } } +impl ErrorHandler for GenericErrorHandler { + async fn handle(&self, source: &str, response: SymResponse<'_>) -> CacheError { + Self::handle_response(source, response.response).await + } +} + /// A HTTP request Symbolicator wants to make. struct SymRequest<'a> { source_name: &'a str, From 22e80ef58728bfee06e343166950b0fe891e7591 Mon Sep 17 00:00:00 2001 From: David Herberth Date: Wed, 6 May 2026 10:56:04 +0200 Subject: [PATCH 4/4] review, move comment block --- .../src/symbolication/attachments.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/symbolicator-native/src/symbolication/attachments.rs b/crates/symbolicator-native/src/symbolication/attachments.rs index cfcfd30b5..8a29e79b7 100644 --- a/crates/symbolicator-native/src/symbolication/attachments.rs +++ b/crates/symbolicator-native/src/symbolication/attachments.rs @@ -19,12 +19,12 @@ pub async fn download_attachment( } => (storage_url, storage_token), }; + // TODO: maybe its worth using the actual `DownloadService` instead of straight going to the `trusted_client`. + // Doing so would in theory allow us to have retries and error report, as well as being able to + // download files in multiple chunks concurrently, but I don’t think our `objecstore` server currently + // supports range requests, and those would also mess with streaming decompression. + // Not to mention that using the `DownloadService` is not that straight forward. download::retry(|| async { - // TODO: maybe its worth using the actual `DownloadService` instead of straight going to the `trusted_client`. - // Doing so would in theory allow us to have retries and error report, as well as being able to - // download files in multiple chunks concurrently, but I don’t think our `objecstore` server currently - // supports range requests, and those would also mess with streaming decompression. - // Not to mention that using the `DownloadService` is not that straight forward. let mut request = download_svc.trusted_client.get(&storage_url); if let Some(token) = storage_token.as_ref() { request = request.bearer_auth(token);