feat(symbolicli): Enable local JS symbolication#1956
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| }); | ||
| found_bundles.insert(path); | ||
| } | ||
| } |
There was a problem hiding this comment.
Lookup ignores repeated query parameters
High Severity
The local /lookup handler deserializes debug_id and url as single optional fields, but lookup_js_artifacts sends multiple debug_id and url query pairs in one request. Axum’s standard Query extractor keeps only one value per name, so additional modules’ bundles are never returned and offline JS symbolication can miss artifacts.
Reviewed by Cursor Bugbot for commit 9f5329a. Configure here.
There was a problem hiding this comment.
True in principle, but in practice the lookup function is only ever called with one debug_id/url.
| out.push(LookupResult::Bundle { | ||
| id: path.to_string_lossy().to_string(), | ||
| url: index.url(&format!("bundles/{path_encoded}")), | ||
| resolved_with: ResolvedWith::Release, |
There was a problem hiding this comment.
Nested bundle download URLs break
Medium Severity
Bundle download URLs encode the full relative zip path (including / as %2F) under /bundles/, while ServeDir serves files from the symbols root. tower-http’s ServeDir does not reliably map such encoded slash paths to nested files, so bundles not at the top level of --symbols can 404 after a successful lookup.
Reviewed by Cursor Bugbot for commit 9f5329a. Configure here.
There was a problem hiding this comment.
Seems to work fine in my tests (including test_existing_bundle).
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
This file is just the event module split out of main.rs, with one small fix noted below.
| Sourcemap(JsModule), | ||
| Object(RawObjectInfo), |
There was a problem hiding this comment.
I switched the order here compared to master. Because of serde(untagged) and the fact that RawObjectInfo matches any type, we need to put other variants first.
| } | ||
| } | ||
|
|
||
| mod event { |
There was a problem hiding this comment.
As mentioned before this module was moved out of this file.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 872a3e1. Configure here.
| let mut archive = ZipArchive::new(archive)?; | ||
|
|
||
| let manifest = archive.by_name("manifest.json")?; | ||
| let manifest: SourceBundleManifest = serde_json::from_reader(manifest)?; |
There was a problem hiding this comment.
Invalid zip aborts entire index
Medium Severity
Building the local artifact index walks every .zip under --symbols and returns an error if any file cannot be opened, parsed as a zip, or read as a bundle with manifest.json. One unrelated or corrupt archive in that tree prevents the local lookup server from starting, blocking all offline JS symbolication for the directory.
Reviewed by Cursor Bugbot for commit 872a3e1. Configure here.
There was a problem hiding this comment.
I believe this is acceptable.
| let listener = tokio::net::TcpListener::from_std(listener).unwrap(); | ||
| axum::serve(listener, router).await.unwrap(); | ||
| }); |
There was a problem hiding this comment.
Bug: The server task spawned in start_server uses .unwrap(). A panic in this task is not propagated, causing silent failures and subsequent connection errors.
Severity: MEDIUM
Suggested Fix
Propagate errors from the spawned task to the caller of start_server. This can be achieved by using a channel (e.g., tokio::sync::oneshot) to communicate the server's startup result back to the main thread. The main thread should wait on this channel to confirm the server has started successfully before proceeding. Replace .unwrap() calls in the task with proper error handling that sends the error over the channel.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: crates/symbolicli/src/js_local_source.rs#L38-L40
Potential issue: The `start_server` function spawns a Tokio task to run a local server
but does not await the task's `JoinHandle`. Inside this spawned task, `unwrap()` is used
after `tokio::net::TcpListener::from_std` and `axum::serve`. If either of these
operations fails, the task will panic. Because the task is not awaited, this panic will
not be propagated to the main thread. The main thread will proceed, assuming the server
is running, and subsequent HTTP requests to the server's URL will fail with connection
errors, making the root cause of the failure difficult to diagnose.
| let manifest = archive.by_name("manifest.json")?; | ||
| let manifest: SourceBundleManifest = serde_json::from_reader(manifest)?; |
There was a problem hiding this comment.
Bug: The offline JS symbol server crashes if the symbols directory contains a .zip file that is not a Sentry artifact bundle, due to a missing manifest.json.
Severity: HIGH
Suggested Fix
Handle the Result from archive.by_name("manifest.json") instead of using the ? operator to propagate errors. If the file is not found, the program should log a warning and continue to the next file in the directory, rather than crashing.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: crates/symbolicli/src/js_local_source.rs#L128-L129
Potential issue: When initializing the local JavaScript symbol server in offline mode,
the code iterates through all `.zip` files in the user-provided symbols directory. It
attempts to read a `manifest.json` from each archive using
`archive.by_name("manifest.json")?`. If a `.zip` file is not a Sentry artifact bundle
and does not contain this manifest, the `?` operator propagates the `FileNotFound`
error. This causes the entire `start_server()` function to fail, crashing the program
instead of skipping the invalid file.
1 participant
This makes it possible to symbolicate JS events locally with
symbolicliby running a pretend Sentry artifact lookup endpoint. Just put some artifact bundles in a directory and calland you should be good. For now it only supports bundles, not individual artifact files.
Closes #1939. Closes SYMBOLI-58.