Skip to content

feat(preprod): Show analysis URL after mobile-app upload #2675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chromy merged 1 commit into from
Aug 11, 2025
Merged

feat(preprod): Show analysis URL after mobile-app upload #2675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/api/data_types/chunking/mobile_app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ pub struct ChunkedMobileAppRequest<'a> {
pub checksum: Digest,
pub chunks: &'a [Digest],
#[serde(skip_serializing_if = "Option::is_none")]
pub git_sha: Option<&'a str>,
pub head_sha: Option<&'a str>,
#[serde(skip_serializing_if = "Option::is_none")]
pub build_configuration: Option<&'a str>,
}
Expand All @@ -20,4 +20,5 @@ pub struct AssembleMobileAppResponse {
pub state: ChunkedFileState,
pub missing_chunks: Vec<Digest>,
pub detail: Option<String>,
pub artifact_id: Option<String>,
}
4 changes: 2 additions & 2 deletions src/api/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1036,7 +1036,7 @@ impl<'a> AuthenticatedApi<'a> {
project: &str,
checksum: Digest,
chunks: &[Digest],
git_sha: Option<&str>,
head_sha: Option<&str>,
build_configuration: Option<&str>,
) -> ApiResult<AssembleMobileAppResponse> {
let url = format!(
Expand All @@ -1049,7 +1049,7 @@ impl<'a> AuthenticatedApi<'a> {
.with_json_body(&ChunkedMobileAppRequest {
checksum,
chunks,
git_sha,
head_sha,
build_configuration,
})?
.send()?
Expand Down
166 changes: 76 additions & 90 deletions src/commands/mobile_app/upload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,14 @@ use clap::{Arg, ArgAction, ArgMatches, Command};
use indicatif::ProgressStyle;
use itertools::Itertools as _;
use log::{debug, info, warn};
use sha1_smol::Digest;
use symbolic::common::ByteView;
use zip::write::SimpleFileOptions;
use zip::{DateTime, ZipWriter};

use crate::api::{Api, AuthenticatedApi, ChunkUploadCapability};
use crate::api::{Api, AuthenticatedApi, ChunkUploadCapability, ChunkedFileState};
use crate::config::Config;
use crate::utils::args::ArgExt as _;
use crate::utils::chunks::{upload_chunks, Chunk, ASSEMBLE_POLL_INTERVAL};
use crate::utils::chunks::{upload_chunks, Chunk};
use crate::utils::fs::get_sha1_checksums;
#[cfg(all(target_os = "macos", target_arch = "aarch64"))]
use crate::utils::fs::TempDir;
Expand Down Expand Up @@ -129,9 +128,10 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {

let config = Config::current();
let (org, project) = config.get_org_and_project(matches)?;
let base_url = config.get_base_url()?;

let mut uploaded_paths = vec![];
let mut errored_paths = vec![];
let mut uploaded_paths_and_ids = vec![];
let mut errored_paths_and_reasons = vec![];
for (path, zip) in normalized_zips {
info!("Uploading file: {}", path.display());
let bytes = ByteView::open(zip.path())?;
Expand All @@ -143,41 +143,50 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
sha.as_deref(),
build_configuration,
) {
Ok(_) => {
Ok(artifact_id) => {
info!("Successfully uploaded file: {}", path.display());
uploaded_paths.push(path.to_path_buf());
uploaded_paths_and_ids.push((path.to_path_buf(), artifact_id));
}
Err(e) => {
debug!("Failed to upload file at path {}: {}", path.display(), e);
errored_paths.push(path.to_path_buf());
errored_paths_and_reasons.push((path.to_path_buf(), e));
}
}
}

if !errored_paths.is_empty() {
if !errored_paths_and_reasons.is_empty() {
warn!(
"Failed to upload {} file{}:",
errored_paths.len(),
if errored_paths.len() == 1 { "" } else { "s" }
errored_paths_and_reasons.len(),
if errored_paths_and_reasons.len() == 1 {
""
} else {
"s"
}
);
for path in errored_paths {
warn!(" - {}", path.display());
}
}

println!(
"Successfully uploaded {} file{} to Sentry",
uploaded_paths.len(),
if uploaded_paths.len() == 1 { "" } else { "s" }
);
if uploaded_paths.len() < 3 {
for path in &uploaded_paths {
println!(" - {}", path.display());
for (path, reason) in errored_paths_and_reasons {
warn!(" - {} ({})", path.display(), reason);
}
}

if uploaded_paths.is_empty() {
if uploaded_paths_and_ids.is_empty() {
bail!("Failed to upload any files");
} else {
println!(
"Successfully uploaded {} file{} to Sentry",
uploaded_paths_and_ids.len(),
if uploaded_paths_and_ids.len() == 1 {
""
} else {
"s"
}
);
if uploaded_paths_and_ids.len() < 3 {
for (path, artifact_id) in &uploaded_paths_and_ids {
let url = format!("{base_url}/{org}/preprod/{project}/{artifact_id}");
println!(" - {} ({url})", path.display());
}
}
}
Ok(())
}
Expand Down Expand Up @@ -337,14 +346,15 @@ fn normalize_directory(path: &Path) -> Result<TempFile> {
Ok(temp_file)
}

/// Returns artifact id if upload was successful.
fn upload_file(
api: &AuthenticatedApi,
bytes: &[u8],
org: &str,
project: &str,
sha: Option<&str>,
build_configuration: Option<&str>,
) -> Result<()> {
) -> Result<String> {
const SELF_HOSTED_ERROR_HINT: &str = "If you are using a self-hosted Sentry server, \
update to the latest version of Sentry to use the mobile-app upload command.";

Expand All @@ -371,7 +381,7 @@ fn upload_file(
}

let progress_style =
ProgressStyle::default_spinner().template("{spinner} Optimizing bundle for upload...");
ProgressStyle::default_spinner().template("{spinner} Preparing for upload...");
let pb = ProgressBar::new_spinner();
pb.enable_steady_tick(100);
pb.set_style(progress_style);
Expand All @@ -384,76 +394,52 @@ fn upload_file(
.map(|(data, checksum)| Chunk((*checksum, data)))
.collect::<Vec<_>>();

pb.finish_with_duration("Finishing upload");

let response =
api.assemble_mobile_app(org, project, checksum, &checksums, sha, build_configuration)?;
chunks.retain(|Chunk((digest, _))| response.missing_chunks.contains(digest));

if !chunks.is_empty() {
let upload_progress_style = ProgressStyle::default_bar().template(
"{prefix:.dim} Uploading files...\
\n{wide_bar} {bytes}/{total_bytes} ({eta})",
);
upload_chunks(&chunks, &chunk_upload_options, upload_progress_style)?;
} else {
println!("Nothing to upload, all files are on the server");
}

poll_assemble(
api,
checksum,
&checksums,
org,
project,
sha,
build_configuration,
)?;
Ok(())
}

fn poll_assemble(
api: &AuthenticatedApi,
checksum: Digest,
chunks: &[Digest],
org: &str,
project: &str,
sha: Option<&str>,
build_configuration: Option<&str>,
) -> Result<()> {
debug!("Polling assemble for checksum: {}", checksum);

let progress_style = ProgressStyle::default_spinner().template("{spinner} Processing files...");
let pb = ProgressBar::new_spinner();
pb.finish_with_duration("Preparing for upload");

// In the normal case we go through this loop exactly twice:
// 1. state=not_found
// server tells us the we need to send every chunk and we do so
// 2. artifact_id set so we're done (likely state=created)
//
// In the case where all the chunks are already on the server we go
// through only once:
// 1. state=ok, artifact_id set
//
// In the case where something went wrong (which could be on either
// iteration of the loop) we get:
// n. state=err, artifact_id unset
let result = loop {
let response =
api.assemble_mobile_app(org, project, checksum, &checksums, sha, build_configuration)?;
chunks.retain(|Chunk((digest, _))| response.missing_chunks.contains(digest));

if !chunks.is_empty() {
let upload_progress_style = ProgressStyle::default_bar().template(
"{prefix:.dim} Uploading files...\
\n{wide_bar} {bytes}/{total_bytes} ({eta})",
);
upload_chunks(&chunks, &chunk_upload_options, upload_progress_style)?;
}

pb.enable_steady_tick(100);
pb.set_style(progress_style);
// state.is_err() is not the same as this since it also returns
// true for ChunkedFileState::NotFound.
if response.state == ChunkedFileState::Error {
let message = response.detail.as_deref().unwrap_or("unknown error");
bail!("Failed to process uploaded files: {}", message);
}

let response = loop {
let response =
api.assemble_mobile_app(org, project, checksum, chunks, sha, build_configuration)?;
if let Some(artifact_id) = response.artifact_id {
break Ok(artifact_id);
}
Copy link
Copy Markdown

@cursor cursor bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: File Upload Polling Fails Without Artifact ID

The upload_file function can enter an infinite loop if the server returns a successful state (e.g., "ok") but without an artifact_id. The loop lacks a timeout or delay and only exits on error or artifact_id presence. This is a regression from previous polling logic that included a sleep delay, resulting in continuous API calls, high CPU usage, and a hanging CLI.

Fix in Cursor Fix in Web

Copy link
Copy Markdown
Contributor Author

@chromy chromy Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should not occur in practice but I'll:

  • add something to raise an error in this case
  • put some sleep/limit on the loop


if response.state.is_finished() {
break response;
bail!(
"File upload is_finished() but did not succeeded (returning artifact_id) or error"
);
}

std::thread::sleep(ASSEMBLE_POLL_INTERVAL);
};

pb.finish_with_duration("Processing");

if response.state.is_err() {
let message = response.detail.as_deref().unwrap_or("unknown error");
bail!("Failed to process uploaded files: {}", message);
}

if response.state.is_pending() {
info!("File upload complete (processing pending on server)");
} else {
info!("File processing complete");
}

Ok(())
result
}

#[cfg(not(windows))]
Expand Down
3 changes: 1 addition & 2 deletions tests/integration/_cases/mobile_app/mobile_app-upload-apk-all-uploaded.trycmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@
$ sentry-cli mobile-app upload tests/integration/_fixtures/mobile_app/apk.apk
? success
[..]WARN[..]EXPERIMENTAL: The mobile-app subcommand is experimental. The command is subject to breaking changes and may be removed without notice in any release.
Nothing to upload, all files are on the server
Successfully uploaded 1 file to Sentry
- tests/integration/_fixtures/mobile_app/apk.apk
- tests/integration/_fixtures/mobile_app/apk.apk (http[..]/wat-org/preprod/wat-project/42)

```
2 changes: 1 addition & 1 deletion tests/integration/_cases/mobile_app/mobile_app-upload-apk.trycmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ $ sentry-cli mobile-app upload tests/integration/_fixtures/mobile_app/apk.apk --
? success
[..]WARN[..]EXPERIMENTAL: The mobile-app subcommand is experimental. The command is subject to breaking changes and may be removed without notice in any release.
Successfully uploaded 1 file to Sentry
- tests/integration/_fixtures/mobile_app/apk.apk
- tests/integration/_fixtures/mobile_app/apk.apk (http[..]/wat-org/preprod/wat-project/42)

```
2 changes: 1 addition & 1 deletion tests/integration/_cases/mobile_app/mobile_app-upload-ipa.trycmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ $ sentry-cli mobile-app upload tests/integration/_fixtures/mobile_app/ipa.ipa --
? success
[..]WARN[..]EXPERIMENTAL: The mobile-app subcommand is experimental. The command is subject to breaking changes and may be removed without notice in any release.
Successfully uploaded 1 file to Sentry
- tests/integration/_fixtures/mobile_app/ipa.ipa
- tests/integration/_fixtures/mobile_app/ipa.ipa (http[..]/wat-org/preprod/wat-project/some-text-id)

```
12 changes: 7 additions & 5 deletions tests/integration/mobile_app/upload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ fn command_mobile_app_upload_apk_all_uploaded() {
"POST",
"/api/0/projects/wat-org/wat-project/files/preprodartifacts/assemble/",
)
.with_response_body(r#"{"state":"ok","missingChunks":[]}"#),
.with_response_body(r#"{"state":"ok","missingChunks":[],"artifactId":"42"}"#),
)
.register_trycmd_test("mobile_app/mobile_app-upload-apk-all-uploaded.trycmd")
.with_default_token();
Expand Down Expand Up @@ -159,7 +159,7 @@ fn command_mobile_app_upload_apk_chunked() {
"/api/0/projects/wat-org/wat-project/files/preprodartifacts/assemble/",
)
.with_header_matcher("content-type", "application/json")
.with_matcher(r#"{"checksum":"18e40e6e932d0b622d631e887be454cc2003dbb5","chunks":["18e40e6e932d0b622d631e887be454cc2003dbb5"],"git_sha":"test_sha"}"#)
.with_matcher(r#"{"checksum":"18e40e6e932d0b622d631e887be454cc2003dbb5","chunks":["18e40e6e932d0b622d631e887be454cc2003dbb5"],"head_sha":"test_sha"}"#)
.with_response_fn(move |_| {
if is_first_assemble_call.swap(false, Ordering::Relaxed) {
r#"{
Expand All @@ -169,7 +169,8 @@ fn command_mobile_app_upload_apk_chunked() {
} else {
r#"{
"state": "ok",
"missingChunks": []
"missingChunks": [],
"artifactId": "42"
}"#
}
.into()
Expand Down Expand Up @@ -213,7 +214,7 @@ fn command_mobile_app_upload_ipa_chunked() {
"/api/0/projects/wat-org/wat-project/files/preprodartifacts/assemble/",
)
.with_header_matcher("content-type", "application/json")
.with_matcher(r#"{"checksum":"ed9da71e3688261875db21b266da84ffe004a8a4","chunks":["ed9da71e3688261875db21b266da84ffe004a8a4"],"git_sha":"test_sha"}"#)
.with_matcher(r#"{"checksum":"ed9da71e3688261875db21b266da84ffe004a8a4","chunks":["ed9da71e3688261875db21b266da84ffe004a8a4"],"head_sha":"test_sha"}"#)
.with_response_fn(move |_| {
if is_first_assemble_call.swap(false, Ordering::Relaxed) {
r#"{
Expand All @@ -223,7 +224,8 @@ fn command_mobile_app_upload_ipa_chunked() {
} else {
r#"{
"state": "ok",
"missingChunks": []
"missingChunks": [],
"artifactId": "some-text-id"
}"#
}
.into()
Expand Down