From 401a4b5915d14b4edfc524440576cc789e1aa3b3 Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Thu, 30 Jul 2026 16:02:48 -0700 Subject: [PATCH 1/3] feat(symbols): skip uploading symbols LaunchDarkly already has A rebuild that changes one module still re-sent every symbol file, so upload time tracked the size of the whole build rather than what actually changed. `upload` now asks for skip_existing, and any key answered with an empty string is reported as already uploaded and skipped. Only content-addressed symbols-id keys are ever answered that way, so a Version Lane copy still overwrites. A backend that predates the argument rejects the query outright, so the request is retried once without it. That keeps a freshly updated CLI working against an older or self-hosted deployment, at the cost of sending bytes it might not have needed. --no-skip-existing forces a re-upload, for repairing a corrupt stored object. Co-authored-by: Cursor --- cmd/symbols/apple_upload.go | 19 +++++- cmd/symbols/flutter_upload.go | 19 +++++- cmd/symbols/upload.go | 82 +++++++++++++++++++++--- cmd/symbols/upload_dedup_test.go | 104 +++++++++++++++++++++++++++++++ 4 files changed, 211 insertions(+), 13 deletions(-) create mode 100644 cmd/symbols/upload_dedup_test.go diff --git a/cmd/symbols/apple_upload.go b/cmd/symbols/apple_upload.go index a2ac5a22..f4ed86c7 100644 --- a/cmd/symbols/apple_upload.go +++ b/cmd/symbols/apple_upload.go @@ -46,7 +46,10 @@ func (m appleSymbolMap) label() string { // When includeSources is set, each image's referenced source files are packed // into a .srcbundle and uploaded alongside its map so the UI can show source // context around native frames. -func uploadAppleDSYMs(apiKey, projectID, path, backendURL string, includeSources bool) error { +// +// Every key here is addressed by build UUID, so skipExisting applies to all of +// them: an unchanged binary keeps its UUID, so a repeat upload sends nothing. +func uploadAppleDSYMs(apiKey, projectID, path, backendURL string, includeSources, skipExisting bool) error { images, err := findDSYMImages(path) if err != nil { return fmt.Errorf("failed to find dSYM files: %w", err) @@ -68,7 +71,7 @@ func uploadAppleDSYMs(apiKey, projectID, path, backendURL string, includeSources keys[i] = m.Key } - uploadURLs, err := getSymbolUploadUrls(apiKey, projectID, keys, backendURL) + uploadURLs, err := getSymbolUploadUrls(apiKey, projectID, keys, backendURL, skipExisting) if err != nil { return fmt.Errorf("failed to get upload URLs: %w", err) } @@ -78,13 +81,23 @@ func uploadAppleDSYMs(apiKey, projectID, path, backendURL string, includeSources return fmt.Errorf("expected %d upload URLs but received %d", len(maps), len(uploadURLs)) } + skipped := 0 for i, m := range maps { + if alreadyUploaded(uploadURLs[i]) { + fmt.Printf("Skipping %s, already uploaded\n", m.label()) + skipped++ + continue + } if err := uploadBytes(m.Data, uploadURLs[i], m.label()); err != nil { return fmt.Errorf("failed to upload symbol map for %s: %w", m.UUID, err) } } - fmt.Println("Successfully uploaded all symbols") + if skipped > 0 { + fmt.Printf("Successfully uploaded all symbols (%d already present)\n", skipped) + } else { + fmt.Println("Successfully uploaded all symbols") + } return nil } diff --git a/cmd/symbols/flutter_upload.go b/cmd/symbols/flutter_upload.go index 10cdb318..adb0b3ea 100644 --- a/cmd/symbols/flutter_upload.go +++ b/cmd/symbols/flutter_upload.go @@ -44,7 +44,10 @@ type flutterUpload struct { // uploadFlutterSymbols discovers app.*.symbols files under path, compiles each // to a .dartmap, and uploads it to the Id lane (and the Version lane when // appVersion is set). -func uploadFlutterSymbols(apiKey, projectID, path, appVersion, backendURL string) error { +// +// With skipExisting only the Id-lane copy can be skipped: a rebuild under the same +// --app-version must still replace what that version resolves to. +func uploadFlutterSymbols(apiKey, projectID, path, appVersion, backendURL string, skipExisting bool) error { uploads, err := buildFlutterMaps(path, appVersion) if err != nil { return err @@ -55,7 +58,7 @@ func uploadFlutterSymbols(apiKey, projectID, path, appVersion, backendURL string keys[i] = u.Key } - uploadURLs, err := getSymbolUploadUrls(apiKey, projectID, keys, backendURL) + uploadURLs, err := getSymbolUploadUrls(apiKey, projectID, keys, backendURL, skipExisting) if err != nil { return fmt.Errorf("failed to get upload URLs: %w", err) } @@ -64,13 +67,23 @@ func uploadFlutterSymbols(apiKey, projectID, path, appVersion, backendURL string return fmt.Errorf("expected %d upload URLs but received %d", len(uploads), len(uploadURLs)) } + skipped := 0 for i, u := range uploads { + if alreadyUploaded(uploadURLs[i]) { + fmt.Printf("Skipping %s, already uploaded\n", u.Label) + skipped++ + continue + } if err := uploadBytes(u.Data, uploadURLs[i], u.Label); err != nil { return fmt.Errorf("failed to upload symbol map %s: %w", u.Label, err) } } - fmt.Println("Successfully uploaded all symbols") + if skipped > 0 { + fmt.Printf("Successfully uploaded all symbols (%d already present)\n", skipped) + } else { + fmt.Println("Successfully uploaded all symbols") + } return nil } diff --git a/cmd/symbols/upload.go b/cmd/symbols/upload.go index 48a57844..f88aad0a 100644 --- a/cmd/symbols/upload.go +++ b/cmd/symbols/upload.go @@ -39,6 +39,11 @@ const ( // scanned from --source-path, since an R8 mapping records no paths). includeSourcesFlag = "include-sources" + // noSkipExistingFlag forces a re-upload of symbols LaunchDarkly already has. + // Dedup is on by default and only applies to symbols-id (content addressed) + // keys, so this is for repairing a corrupt stored object, not normal use. + noSkipExistingFlag = "no-skip-existing" + // sourcePathFlag is the directory scanned for .java/.kt sources when // --include-sources is used with --type android. It defaults to the current // directory because --path points at the mapping.txt output dir, which holds @@ -97,6 +102,20 @@ const ( ) } ` + + // getSymbolUrlsDedupQuery asks the backend to answer keys it already stores with + // an empty string. A separate document because a backend that predates the + // argument rejects the whole query, which getSymbolUploadUrls falls back from. + getSymbolUrlsDedupQuery = ` + query GetSymbolUploadUrls($api_key: String!, $project_id: String!, $paths: [String!]!, $skip_existing: Boolean) { + get_symbol_upload_urls_ld( + api_key: $api_key + project_id: $project_id + paths: $paths + skip_existing: $skip_existing + ) + } + ` ) // reactNativeUploadSuffixes are the files produced by `react-native bundle`: @@ -191,6 +210,8 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error path := viper.GetString(pathFlag) basePath := viper.GetString(basePathFlag) backendUrl := viper.GetString(backendUrlFlag) + // Dedup is the default; --no-skip-existing forces every byte to be resent. + skipExisting := !viper.GetBool(noSkipExistingFlag) if backendUrl == "" { backendUrl = defaultBackendUrl @@ -200,7 +221,7 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error // symbol maps keyed by build UUID, ignoring the version/symbols-id lanes. if symbolType == typeAppleDSYM { fmt.Printf("Starting to upload %s symbols from %s\n", symbolType, path) - return uploadAppleDSYMs(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, path, backendUrl, viper.GetBool(includeSourcesFlag)) + return uploadAppleDSYMs(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, path, backendUrl, viper.GetBool(includeSourcesFlag), skipExisting) } // Flutter/Dart symbols take a dedicated path too: each app..symbols @@ -208,7 +229,7 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error // Version-lane copy when --app-version is set. if symbolType == typeFlutter { fmt.Printf("Starting to upload %s symbols from %s\n", symbolType, path) - return uploadFlutterSymbols(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, path, appVersion, backendUrl) + return uploadFlutterSymbols(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, path, appVersion, backendUrl, skipExisting) } symbolsIDPrefix := symbolsIDPrefixForType(symbolType) @@ -271,7 +292,7 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error } } - uploadUrls, err := getSymbolUploadUrls(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, s3Keys, backendUrl) + uploadUrls, err := getSymbolUploadUrls(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, s3Keys, backendUrl, skipExisting) if err != nil { return fmt.Errorf("failed to get upload URLs: %w", err) } @@ -282,23 +303,42 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error return fmt.Errorf("expected %d upload URLs but received %d", len(s3Keys), len(uploadUrls)) } + skipped := 0 for i, file := range files { + if alreadyUploaded(uploadUrls[i]) { + fmt.Printf("Skipping %s, already uploaded\n", file.Name) + skipped++ + continue + } if err := uploadFile(file.Path, uploadUrls[i], file.Name); err != nil { return fmt.Errorf("failed to upload file %s: %w", file.Path, err) } } if sourceBundle != nil { // The bundle's key was appended last, so it takes the final URL. - if err := uploadBytes(sourceBundle, uploadUrls[len(files)], androidSourceBundleName); err != nil { + if alreadyUploaded(uploadUrls[len(files)]) { + fmt.Printf("Skipping %s, already uploaded\n", androidSourceBundleName) + skipped++ + } else if err := uploadBytes(sourceBundle, uploadUrls[len(files)], androidSourceBundleName); err != nil { return fmt.Errorf("failed to upload source bundle: %w", err) } } - fmt.Println("Successfully uploaded all symbols") + if skipped > 0 { + fmt.Printf("Successfully uploaded all symbols (%d already present)\n", skipped) + } else { + fmt.Println("Successfully uploaded all symbols") + } return nil } } +// alreadyUploaded reports whether the backend answered a key with "skip" rather +// than a URL. The empty string is the signal, which is why asking for it is opt-in. +func alreadyUploaded(uploadURL string) bool { + return uploadURL == "" +} + // symbolTypeAliases maps user-friendly synonyms to a canonical --type value. // All Apple platforms share the single dSYM-based pipeline, so any Apple // platform acronym resolves to apple-dsym. @@ -480,15 +520,40 @@ func readSymbolsIDFile(filePath string) string { return strings.TrimSpace(string(content)) } -func getSymbolUploadUrls(apiKey, projectID string, paths []string, backendUrl string) ([]string, error) { +// getSymbolUploadUrls returns one upload URL per requested key, in order. +// +// With skipExisting, a key the backend already stores comes back empty and callers +// must skip it; only content-addressed symbols-id keys are answered that way, so a +// Version Lane key still gets a URL and still overwrites. +// +// A backend that predates the argument rejects the query, so this retries once +// without it, keeping an updated CLI working against an older deployment. +func getSymbolUploadUrls(apiKey, projectID string, paths []string, backendUrl string, skipExisting bool) ([]string, error) { + urls, err := requestSymbolUploadUrls(apiKey, projectID, paths, backendUrl, skipExisting) + if err != nil && skipExisting && strings.Contains(err.Error(), skipExistingArgument) { + return requestSymbolUploadUrls(apiKey, projectID, paths, backendUrl, false) + } + return urls, err +} + +// skipExistingArgument names the query argument, in both the request and the +// validation error a backend without it returns. +const skipExistingArgument = "skip_existing" + +func requestSymbolUploadUrls(apiKey, projectID string, paths []string, backendUrl string, skipExisting bool) ([]string, error) { variables := map[string]interface{}{ "api_key": apiKey, "project_id": projectID, "paths": paths, } + query := getSymbolUrlsQuery + if skipExisting { + query = getSymbolUrlsDedupQuery + variables[skipExistingArgument] = true + } reqBody, err := json.Marshal(map[string]interface{}{ - "query": getSymbolUrlsQuery, + "query": query, "variables": variables, }) if err != nil { @@ -589,6 +654,9 @@ func initFlags(cmd *cobra.Command) { cmd.Flags().Bool(includeSourcesFlag, false, fmt.Sprintf("Also upload your source files so the errors page can show source context around native frames (%s and %s). Your source is stored in LaunchDarkly", typeAppleDSYM, typeAndroid)) _ = viper.BindPFlag(includeSourcesFlag, cmd.Flags().Lookup(includeSourcesFlag)) + cmd.Flags().Bool(noSkipExistingFlag, false, "Re-upload symbols even when LaunchDarkly already has them. By default a symbols-id file that is already stored is skipped, since its id is derived from its contents") + _ = viper.BindPFlag(noSkipExistingFlag, cmd.Flags().Lookup(noSkipExistingFlag)) + cmd.Flags().String(sourcePathFlag, defaultPath, fmt.Sprintf("Directory to scan for .java/.kt sources when using --%s with --type %s", includeSourcesFlag, typeAndroid)) _ = viper.BindPFlag(sourcePathFlag, cmd.Flags().Lookup(sourcePathFlag)) } diff --git a/cmd/symbols/upload_dedup_test.go b/cmd/symbols/upload_dedup_test.go new file mode 100644 index 00000000..2b1d593b --- /dev/null +++ b/cmd/symbols/upload_dedup_test.go @@ -0,0 +1,104 @@ +package symbols + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// capturedRequest is one GraphQL request body a test server received. +type capturedRequest struct { + Query string `json:"query"` + Variables map[string]interface{} `json:"variables"` +} + +// dedupTestServer records every request and answers each with the given body. +// Responses are consumed in order so a test can drive the retry path. +func dedupTestServer(t *testing.T, responses ...string) (*httptest.Server, *[]capturedRequest) { + t.Helper() + var got []capturedRequest + i := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + require.NoError(t, err) + var captured capturedRequest + require.NoError(t, json.Unmarshal(body, &captured)) + got = append(got, captured) + + response := responses[len(responses)-1] + if i < len(responses) { + response = responses[i] + } + i++ + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(response)) + })) + t.Cleanup(srv.Close) + return srv, &got +} + +func TestAlreadyUploaded(t *testing.T) { + // The empty string must never be mistaken for a URL to PUT to. + assert.True(t, alreadyUploaded("")) + assert.False(t, alreadyUploaded("https://example.com/presigned")) +} + +func TestGetSymbolUploadUrlsRequestsDedup(t *testing.T) { + srv, got := dedupTestServer(t, `{"data":{"get_symbol_upload_urls_ld":["","https://u/2"]}}`) + + urls, err := getSymbolUploadUrls("key", "proj", []string{"a", "b"}, srv.URL, true) + require.NoError(t, err) + assert.Equal(t, []string{"", "https://u/2"}, urls) + + require.Len(t, *got, 1) + req := (*got)[0] + assert.Contains(t, req.Query, skipExistingArgument, "the dedup document should be sent") + assert.Equal(t, true, req.Variables[skipExistingArgument]) +} + +func TestGetSymbolUploadUrlsOmitsDedupWhenDisabled(t *testing.T) { + srv, got := dedupTestServer(t, `{"data":{"get_symbol_upload_urls_ld":["https://u/1"]}}`) + + urls, err := getSymbolUploadUrls("key", "proj", []string{"a"}, srv.URL, false) + require.NoError(t, err) + assert.Equal(t, []string{"https://u/1"}, urls) + + require.Len(t, *got, 1) + req := (*got)[0] + assert.NotContains(t, req.Query, skipExistingArgument, "--no-skip-existing must not ask for dedup") + assert.NotContains(t, req.Variables, skipExistingArgument) +} + +// A backend that predates the argument rejects the whole query, and an updated CLI +// must still work against it by re-asking without dedup. +func TestGetSymbolUploadUrlsFallsBackWhenArgumentUnknown(t *testing.T) { + srv, got := dedupTestServer(t, + `{"errors":[{"message":"Unknown argument \"skip_existing\" on field \"Query.get_symbol_upload_urls_ld\"."}]}`, + `{"data":{"get_symbol_upload_urls_ld":["https://u/1"]}}`, + ) + + urls, err := getSymbolUploadUrls("key", "proj", []string{"a"}, srv.URL, true) + require.NoError(t, err) + assert.Equal(t, []string{"https://u/1"}, urls) + + require.Len(t, *got, 2, "expected one rejected attempt and one retry") + assert.Contains(t, (*got)[0].Query, skipExistingArgument) + assert.NotContains(t, (*got)[1].Query, skipExistingArgument, "the retry must drop the argument") +} + +// Only the unknown-argument case is retried, so a credential or project error still +// reaches the caller instead of being retried as a plain upload. +func TestGetSymbolUploadUrlsDoesNotRetryOtherErrors(t *testing.T) { + srv, got := dedupTestServer(t, `{"errors":[{"message":"error querying project"}]}`) + + _, err := getSymbolUploadUrls("key", "proj", []string{"a"}, srv.URL, true) + require.Error(t, err) + assert.True(t, strings.Contains(err.Error(), "error querying project")) + assert.Len(t, *got, 1, "a non-argument error must not trigger a retry") +} From 03ea7834315b9999ba2a1e1b2f7f608cbf290016 Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Thu, 30 Jul 2026 16:15:01 -0700 Subject: [PATCH 2/3] feat(symbols): note skipped files on stderr while the default is new Skipping already-stored symbols is a change in what `upload` does by default, and the closing "Successfully uploaded all symbols" reads the same whether bytes were sent or not. Anyone re-uploading to repair a stored object would take a no-op run for a successful one, so a run that skipped anything now says so on stderr and names --no-skip-existing. The notice is transitional; the counts stay on stdout so scripts parsing them are unaffected. Also corrects the source-bundle comments: bundles are keyed by the id of the mapping or image they explain, so they are never skipped (enforced backend-side). Co-authored-by: Cursor --- cmd/symbols/apple_upload.go | 12 +++++----- cmd/symbols/flutter_upload.go | 6 +---- cmd/symbols/upload.go | 31 +++++++++++++++++++++----- cmd/symbols/upload_dedup_test.go | 38 ++++++++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 18 deletions(-) diff --git a/cmd/symbols/apple_upload.go b/cmd/symbols/apple_upload.go index f4ed86c7..91eda7df 100644 --- a/cmd/symbols/apple_upload.go +++ b/cmd/symbols/apple_upload.go @@ -47,8 +47,10 @@ func (m appleSymbolMap) label() string { // into a .srcbundle and uploaded alongside its map so the UI can show source // context around native frames. // -// Every key here is addressed by build UUID, so skipExisting applies to all of -// them: an unchanged binary keeps its UUID, so a repeat upload sends nothing. +// With skipExisting, an unchanged binary keeps its UUID, so re-running this sends +// nothing. Source bundles are exempt (the backend decides): they are keyed by their +// image's UUID rather than their own contents, so sources that were unreadable on the +// machine that uploaded first still need to overwrite. func uploadAppleDSYMs(apiKey, projectID, path, backendURL string, includeSources, skipExisting bool) error { images, err := findDSYMImages(path) if err != nil { @@ -93,11 +95,7 @@ func uploadAppleDSYMs(apiKey, projectID, path, backendURL string, includeSources } } - if skipped > 0 { - fmt.Printf("Successfully uploaded all symbols (%d already present)\n", skipped) - } else { - fmt.Println("Successfully uploaded all symbols") - } + reportUploadSummary(skipped) return nil } diff --git a/cmd/symbols/flutter_upload.go b/cmd/symbols/flutter_upload.go index adb0b3ea..106cd6ab 100644 --- a/cmd/symbols/flutter_upload.go +++ b/cmd/symbols/flutter_upload.go @@ -79,11 +79,7 @@ func uploadFlutterSymbols(apiKey, projectID, path, appVersion, backendURL string } } - if skipped > 0 { - fmt.Printf("Successfully uploaded all symbols (%d already present)\n", skipped) - } else { - fmt.Println("Successfully uploaded all symbols") - } + reportUploadSummary(skipped) return nil } diff --git a/cmd/symbols/upload.go b/cmd/symbols/upload.go index f88aad0a..85961ca2 100644 --- a/cmd/symbols/upload.go +++ b/cmd/symbols/upload.go @@ -273,7 +273,10 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error // Android sources are packed into one bundle uploaded beside the mapping, // on the same lane, so the errors page can show the code around each // retraced frame. Keyed off the first mapping's lane: a build has one - // mapping, and its sources are the app's sources. + // mapping, and its sources are the app's sources. Because that key is the + // mapping's id and not the bundle's, the bundle is never skipped as already + // uploaded (the backend decides) — a source-only change keeps the mapping's + // id, and re-running with --include-sources must replace the stale sources. var sourceBundle []byte if symbolType == typeAndroid && viper.GetBool(includeSourcesFlag) { sourceRoot := viper.GetString(sourcePathFlag) @@ -324,11 +327,7 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error } } - if skipped > 0 { - fmt.Printf("Successfully uploaded all symbols (%d already present)\n", skipped) - } else { - fmt.Println("Successfully uploaded all symbols") - } + reportUploadSummary(skipped) return nil } } @@ -339,6 +338,26 @@ func alreadyUploaded(uploadURL string) bool { return uploadURL == "" } +// reportUploadSummary closes out an upload, and when anything was skipped says so on +// stderr as well. +// +// Skipping is new behavior, and someone re-uploading to repair a stored object would +// otherwise read "Successfully uploaded" from a run that sent nothing. The notice is +// transitional and can come out once a release or two has gone by; the counts stay on +// stdout so scripts parsing them are unaffected. +func reportUploadSummary(skipped int) { + if skipped == 0 { + fmt.Println("Successfully uploaded all symbols") + return + } + + fmt.Printf("Successfully uploaded all symbols (%d already present)\n", skipped) + fmt.Fprintf(os.Stderr, + "Note: %d file(s) were skipped because LaunchDarkly already stores them under the same content-derived id. This is new in this release; re-run with --%s to upload them anyway.\n", + skipped, noSkipExistingFlag, + ) +} + // symbolTypeAliases maps user-friendly synonyms to a canonical --type value. // All Apple platforms share the single dSYM-based pipeline, so any Apple // platform acronym resolves to apple-dsym. diff --git a/cmd/symbols/upload_dedup_test.go b/cmd/symbols/upload_dedup_test.go index 2b1d593b..92993b3e 100644 --- a/cmd/symbols/upload_dedup_test.go +++ b/cmd/symbols/upload_dedup_test.go @@ -5,6 +5,7 @@ import ( "io" "net/http" "net/http/httptest" + "os" "strings" "testing" @@ -49,6 +50,43 @@ func TestAlreadyUploaded(t *testing.T) { assert.False(t, alreadyUploaded("https://example.com/presigned")) } +// A run that skipped everything still prints "Successfully uploaded", so the notice +// is what tells someone re-uploading to repair an object that nothing was sent. +func TestReportUploadSummaryNotesSkippedFiles(t *testing.T) { + stdout, stderr := captureOutput(t, func() { reportUploadSummary(2) }) + assert.Contains(t, stdout, "(2 already present)") + assert.Contains(t, stderr, noSkipExistingFlag, "the notice should name the opt-out") + + stdout, stderr = captureOutput(t, func() { reportUploadSummary(0) }) + assert.Contains(t, stdout, "Successfully uploaded all symbols") + assert.Empty(t, stderr, "nothing was skipped, so there is nothing to warn about") +} + +// captureOutput runs fn with os.Stdout and os.Stderr redirected, returning what each +// received. Both are small enough here to stay inside the pipe buffer. +func captureOutput(t *testing.T, fn func()) (string, string) { + t.Helper() + + outR, outW, err := os.Pipe() + require.NoError(t, err) + errR, errW, err := os.Pipe() + require.NoError(t, err) + + origOut, origErr := os.Stdout, os.Stderr + os.Stdout, os.Stderr = outW, errW + fn() + os.Stdout, os.Stderr = origOut, origErr + + require.NoError(t, outW.Close()) + require.NoError(t, errW.Close()) + + out, err := io.ReadAll(outR) + require.NoError(t, err) + errOut, err := io.ReadAll(errR) + require.NoError(t, err) + return string(out), string(errOut) +} + func TestGetSymbolUploadUrlsRequestsDedup(t *testing.T) { srv, got := dedupTestServer(t, `{"data":{"get_symbol_upload_urls_ld":["","https://u/2"]}}`) From a7418b388163cfbbcea80077415aa1806b7c40b2 Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Thu, 30 Jul 2026 16:46:21 -0700 Subject: [PATCH 3/3] feat(symbols): send a digest for artifacts keyed by another artifact's id Source bundles are keyed by the id of the mapping or image they explain, so the backend can't settle them by existence and they were re-sent on every run with --include-sources. They now travel with the MD5 of what is about to be uploaded, and are skipped when that matches what is stored. The digest costs nothing to compute: both platforms build the bundle in memory before uploading it. Nothing else sends one, so a large mapping is still settled by existence alone rather than being read back off disk to hash. The old-backend retry now triggers on either argument name, since validation names whichever one it reached first. Co-authored-by: Cursor --- cmd/symbols/apple_upload.go | 20 ++++++--- cmd/symbols/flutter_upload.go | 4 +- cmd/symbols/upload.go | 75 ++++++++++++++++++++++--------- cmd/symbols/upload_dedup_test.go | 77 +++++++++++++++++++++++++------- 4 files changed, 133 insertions(+), 43 deletions(-) diff --git a/cmd/symbols/apple_upload.go b/cmd/symbols/apple_upload.go index 91eda7df..ef5f083f 100644 --- a/cmd/symbols/apple_upload.go +++ b/cmd/symbols/apple_upload.go @@ -22,6 +22,10 @@ const appleSymbolsIDPrefix = "_sym/apple/id" // in lookup (maps are keyed by UUID); the backend appends the same extension. const appleSymbolExt = ".dsymmap" +// kindSources marks an appleSymbolMap that carries packed sources rather than a +// symbol map. +const kindSources = "sources" + // appleSymbolMap is one compiled artifact ready to upload: an architecture's // .dsymmap symbol map, or (with --include-sources) its .srcbundle sources. type appleSymbolMap struct { @@ -48,9 +52,9 @@ func (m appleSymbolMap) label() string { // context around native frames. // // With skipExisting, an unchanged binary keeps its UUID, so re-running this sends -// nothing. Source bundles are exempt (the backend decides): they are keyed by their -// image's UUID rather than their own contents, so sources that were unreadable on the -// machine that uploaded first still need to overwrite. +// nothing. Source bundles borrow that UUID rather than being keyed by their own +// contents — sources unreadable on the machine that uploaded first must still be able +// to overwrite — so those are skipped only when their digest matches what is stored. func uploadAppleDSYMs(apiKey, projectID, path, backendURL string, includeSources, skipExisting bool) error { images, err := findDSYMImages(path) if err != nil { @@ -69,11 +73,17 @@ func uploadAppleDSYMs(apiKey, projectID, path, backendURL string, includeSources } keys := make([]string, len(maps)) + digests := make([]string, len(maps)) for i, m := range maps { keys[i] = m.Key + if m.Kind == kindSources { + // Sources are keyed by their image's UUID rather than by their own + // contents, so only a digest can show that re-sending them is a no-op. + digests[i] = contentDigest(m.Data) + } } - uploadURLs, err := getSymbolUploadUrls(apiKey, projectID, keys, backendURL, skipExisting) + uploadURLs, err := getSymbolUploadUrls(apiKey, projectID, keys, digests, backendURL, skipExisting) if err != nil { return fmt.Errorf("failed to get upload URLs: %w", err) } @@ -150,7 +160,7 @@ func buildAppleMaps(images []string, includeSources bool) ([]appleSymbolMap, err UUID: a.UUID, Arch: arch, Data: srcData, - Kind: "sources", + Kind: kindSources, }) fmt.Printf("Built source bundle for %s (%s, %d files, %d bytes)\n", a.UUID, arch, nFiles, len(srcData)) } diff --git a/cmd/symbols/flutter_upload.go b/cmd/symbols/flutter_upload.go index 106cd6ab..ec0fab60 100644 --- a/cmd/symbols/flutter_upload.go +++ b/cmd/symbols/flutter_upload.go @@ -58,7 +58,9 @@ func uploadFlutterSymbols(apiKey, projectID, path, appVersion, backendURL string keys[i] = u.Key } - uploadURLs, err := getSymbolUploadUrls(apiKey, projectID, keys, backendURL, skipExisting) + // No digests: every key here is either the dartmap's own build id or a Version + // Lane copy, which the backend re-presigns so it can overwrite. + uploadURLs, err := getSymbolUploadUrls(apiKey, projectID, keys, nil, backendURL, skipExisting) if err != nil { return fmt.Errorf("failed to get upload URLs: %w", err) } diff --git a/cmd/symbols/upload.go b/cmd/symbols/upload.go index 85961ca2..dce58f72 100644 --- a/cmd/symbols/upload.go +++ b/cmd/symbols/upload.go @@ -2,6 +2,7 @@ package symbols import ( "bytes" + "crypto/md5" "encoding/json" "fmt" "io" @@ -10,6 +11,7 @@ import ( "net/url" "os" "path/filepath" + "slices" "strings" "github.com/spf13/cobra" @@ -104,15 +106,16 @@ const ( ` // getSymbolUrlsDedupQuery asks the backend to answer keys it already stores with - // an empty string. A separate document because a backend that predates the - // argument rejects the whole query, which getSymbolUploadUrls falls back from. + // an empty string. A separate document because a backend that predates these + // arguments rejects the whole query, which getSymbolUploadUrls falls back from. getSymbolUrlsDedupQuery = ` - query GetSymbolUploadUrls($api_key: String!, $project_id: String!, $paths: [String!]!, $skip_existing: Boolean) { + query GetSymbolUploadUrls($api_key: String!, $project_id: String!, $paths: [String!]!, $skip_existing: Boolean, $digests: [String!]) { get_symbol_upload_urls_ld( api_key: $api_key project_id: $project_id paths: $paths skip_existing: $skip_existing + digests: $digests ) } ` @@ -274,9 +277,9 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error // on the same lane, so the errors page can show the code around each // retraced frame. Keyed off the first mapping's lane: a build has one // mapping, and its sources are the app's sources. Because that key is the - // mapping's id and not the bundle's, the bundle is never skipped as already - // uploaded (the backend decides) — a source-only change keeps the mapping's - // id, and re-running with --include-sources must replace the stale sources. + // mapping's id and not the bundle's, an object being there says nothing + // about these sources — a source-only change keeps the mapping's id — so the + // bundle is skipped only when its digest matches what is already stored. var sourceBundle []byte if symbolType == typeAndroid && viper.GetBool(includeSourcesFlag) { sourceRoot := viper.GetString(sourcePathFlag) @@ -295,7 +298,15 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error } } - uploadUrls, err := getSymbolUploadUrls(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, s3Keys, backendUrl, skipExisting) + // Only the source bundle needs a digest: every other key here is derived from + // the artifact's own content, so the backend settles those by existence alone + // and we never have to read a large mapping back off disk to hash it. + digests := make([]string, len(s3Keys)) + if sourceBundle != nil { + digests[len(s3Keys)-1] = contentDigest(sourceBundle) + } + + uploadUrls, err := getSymbolUploadUrls(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, s3Keys, digests, backendUrl, skipExisting) if err != nil { return fmt.Errorf("failed to get upload URLs: %w", err) } @@ -541,25 +552,44 @@ func readSymbolsIDFile(filePath string) string { // getSymbolUploadUrls returns one upload URL per requested key, in order. // -// With skipExisting, a key the backend already stores comes back empty and callers -// must skip it; only content-addressed symbols-id keys are answered that way, so a -// Version Lane key still gets a URL and still overwrites. +// With skipExisting, a key whose bytes the backend already stores comes back empty +// and callers must skip it. digests is parallel to paths and may be nil or hold "" +// for a key the caller has no digest for; it is what lets the backend settle a key +// that isn't derived from its own contents, since there existence proves nothing. // -// A backend that predates the argument rejects the query, so this retries once -// without it, keeping an updated CLI working against an older deployment. -func getSymbolUploadUrls(apiKey, projectID string, paths []string, backendUrl string, skipExisting bool) ([]string, error) { - urls, err := requestSymbolUploadUrls(apiKey, projectID, paths, backendUrl, skipExisting) - if err != nil && skipExisting && strings.Contains(err.Error(), skipExistingArgument) { - return requestSymbolUploadUrls(apiKey, projectID, paths, backendUrl, false) +// A backend that predates these arguments rejects the query, so this retries once +// without them, keeping an updated CLI working against an older deployment. +func getSymbolUploadUrls(apiKey, projectID string, paths, digests []string, backendUrl string, skipExisting bool) ([]string, error) { + urls, err := requestSymbolUploadUrls(apiKey, projectID, paths, digests, backendUrl, skipExisting) + if err != nil && skipExisting && mentionsDedupArgument(err) { + return requestSymbolUploadUrls(apiKey, projectID, paths, nil, backendUrl, false) } return urls, err } -// skipExistingArgument names the query argument, in both the request and the -// validation error a backend without it returns. -const skipExistingArgument = "skip_existing" +// The dedup arguments, named in both the request and the validation error a backend +// without them returns. +const ( + skipExistingArgument = "skip_existing" + digestsArgument = "digests" +) + +// mentionsDedupArgument reports whether err is a backend rejecting an argument it +// doesn't know. Either name means the deployment predates dedup, since both arrived +// together; validation names whichever it reached. +func mentionsDedupArgument(err error) bool { + return strings.Contains(err.Error(), skipExistingArgument) || + strings.Contains(err.Error(), digestsArgument) +} + +// contentDigest is the hex MD5 of bytes about to be uploaded, which is what S3 +// reports as the ETag of an object stored in one part. Sending it lets the backend +// prove that what it already stores under a key is exactly these bytes. +func contentDigest(data []byte) string { + return fmt.Sprintf("%x", md5.Sum(data)) +} -func requestSymbolUploadUrls(apiKey, projectID string, paths []string, backendUrl string, skipExisting bool) ([]string, error) { +func requestSymbolUploadUrls(apiKey, projectID string, paths, digests []string, backendUrl string, skipExisting bool) ([]string, error) { variables := map[string]interface{}{ "api_key": apiKey, "project_id": projectID, @@ -569,6 +599,11 @@ func requestSymbolUploadUrls(apiKey, projectID string, paths []string, backendUr if skipExisting { query = getSymbolUrlsDedupQuery variables[skipExistingArgument] = true + // The backend wants one digest per path or none, so an all-empty slice is + // left off entirely rather than sent as padding. + if slices.ContainsFunc(digests, func(d string) bool { return d != "" }) { + variables[digestsArgument] = digests + } } reqBody, err := json.Marshal(map[string]interface{}{ diff --git a/cmd/symbols/upload_dedup_test.go b/cmd/symbols/upload_dedup_test.go index 92993b3e..596f96b1 100644 --- a/cmd/symbols/upload_dedup_test.go +++ b/cmd/symbols/upload_dedup_test.go @@ -2,6 +2,7 @@ package symbols import ( "encoding/json" + "fmt" "io" "net/http" "net/http/httptest" @@ -90,7 +91,7 @@ func captureOutput(t *testing.T, fn func()) (string, string) { func TestGetSymbolUploadUrlsRequestsDedup(t *testing.T) { srv, got := dedupTestServer(t, `{"data":{"get_symbol_upload_urls_ld":["","https://u/2"]}}`) - urls, err := getSymbolUploadUrls("key", "proj", []string{"a", "b"}, srv.URL, true) + urls, err := getSymbolUploadUrls("key", "proj", []string{"a", "b"}, nil, srv.URL, true) require.NoError(t, err) assert.Equal(t, []string{"", "https://u/2"}, urls) @@ -98,12 +99,40 @@ func TestGetSymbolUploadUrlsRequestsDedup(t *testing.T) { req := (*got)[0] assert.Contains(t, req.Query, skipExistingArgument, "the dedup document should be sent") assert.Equal(t, true, req.Variables[skipExistingArgument]) + assert.NotContains(t, req.Variables, digestsArgument, "no key needed a digest") +} + +// A digest is what lets the backend settle a key that isn't derived from its own +// contents, so it has to arrive aligned with the paths it describes. +func TestGetSymbolUploadUrlsSendsDigests(t *testing.T) { + srv, got := dedupTestServer(t, `{"data":{"get_symbol_upload_urls_ld":["https://u/1",""]}}`) + + digest := contentDigest([]byte("sources")) + _, err := getSymbolUploadUrls("key", "proj", []string{"mapping.txt", "sources.srcbundle"}, []string{"", digest}, srv.URL, true) + require.NoError(t, err) + + require.Len(t, *got, 1) + req := (*got)[0] + assert.Contains(t, req.Query, digestsArgument) + assert.Equal(t, []interface{}{"", digest}, req.Variables[digestsArgument]) +} + +// An all-empty slice is padding, and the backend takes either one digest per path or +// none at all, so it is left off rather than sent. +func TestGetSymbolUploadUrlsOmitsEmptyDigests(t *testing.T) { + srv, got := dedupTestServer(t, `{"data":{"get_symbol_upload_urls_ld":["https://u/1"]}}`) + + _, err := getSymbolUploadUrls("key", "proj", []string{"a"}, []string{""}, srv.URL, true) + require.NoError(t, err) + + require.Len(t, *got, 1) + assert.NotContains(t, (*got)[0].Variables, digestsArgument) } func TestGetSymbolUploadUrlsOmitsDedupWhenDisabled(t *testing.T) { srv, got := dedupTestServer(t, `{"data":{"get_symbol_upload_urls_ld":["https://u/1"]}}`) - urls, err := getSymbolUploadUrls("key", "proj", []string{"a"}, srv.URL, false) + urls, err := getSymbolUploadUrls("key", "proj", []string{"a"}, []string{contentDigest([]byte("a"))}, srv.URL, false) require.NoError(t, err) assert.Equal(t, []string{"https://u/1"}, urls) @@ -111,23 +140,30 @@ func TestGetSymbolUploadUrlsOmitsDedupWhenDisabled(t *testing.T) { req := (*got)[0] assert.NotContains(t, req.Query, skipExistingArgument, "--no-skip-existing must not ask for dedup") assert.NotContains(t, req.Variables, skipExistingArgument) + assert.NotContains(t, req.Variables, digestsArgument) } -// A backend that predates the argument rejects the whole query, and an updated CLI -// must still work against it by re-asking without dedup. +// A backend that predates the arguments rejects the whole query, and an updated CLI +// must still work against it by re-asking without dedup. Validation names whichever +// argument it reached, so either one has to trigger the retry. func TestGetSymbolUploadUrlsFallsBackWhenArgumentUnknown(t *testing.T) { - srv, got := dedupTestServer(t, - `{"errors":[{"message":"Unknown argument \"skip_existing\" on field \"Query.get_symbol_upload_urls_ld\"."}]}`, - `{"data":{"get_symbol_upload_urls_ld":["https://u/1"]}}`, - ) - - urls, err := getSymbolUploadUrls("key", "proj", []string{"a"}, srv.URL, true) - require.NoError(t, err) - assert.Equal(t, []string{"https://u/1"}, urls) - - require.Len(t, *got, 2, "expected one rejected attempt and one retry") - assert.Contains(t, (*got)[0].Query, skipExistingArgument) - assert.NotContains(t, (*got)[1].Query, skipExistingArgument, "the retry must drop the argument") + for _, unknown := range []string{skipExistingArgument, digestsArgument} { + t.Run(unknown, func(t *testing.T) { + srv, got := dedupTestServer(t, + fmt.Sprintf(`{"errors":[{"message":"Unknown argument \"%s\" on field \"Query.get_symbol_upload_urls_ld\"."}]}`, unknown), + `{"data":{"get_symbol_upload_urls_ld":["https://u/1"]}}`, + ) + + urls, err := getSymbolUploadUrls("key", "proj", []string{"a"}, []string{contentDigest([]byte("a"))}, srv.URL, true) + require.NoError(t, err) + assert.Equal(t, []string{"https://u/1"}, urls) + + require.Len(t, *got, 2, "expected one rejected attempt and one retry") + assert.Contains(t, (*got)[0].Query, skipExistingArgument) + assert.NotContains(t, (*got)[1].Query, skipExistingArgument, "the retry must drop the arguments") + assert.NotContains(t, (*got)[1].Variables, digestsArgument) + }) + } } // Only the unknown-argument case is retried, so a credential or project error still @@ -135,8 +171,15 @@ func TestGetSymbolUploadUrlsFallsBackWhenArgumentUnknown(t *testing.T) { func TestGetSymbolUploadUrlsDoesNotRetryOtherErrors(t *testing.T) { srv, got := dedupTestServer(t, `{"errors":[{"message":"error querying project"}]}`) - _, err := getSymbolUploadUrls("key", "proj", []string{"a"}, srv.URL, true) + _, err := getSymbolUploadUrls("key", "proj", []string{"a"}, nil, srv.URL, true) require.Error(t, err) assert.True(t, strings.Contains(err.Error(), "error querying project")) assert.Len(t, *got, 1, "a non-argument error must not trigger a retry") } + +// The digest has to be the one S3 reports as a one-part object's ETag, or the backend +// can never match it against what it stores. +func TestContentDigestMatchesS3ETagForm(t *testing.T) { + assert.Equal(t, "d41d8cd98f00b204e9800998ecf8427e", contentDigest(nil)) + assert.Equal(t, "5d41402abc4b2a76b9719d911017c592", contentDigest([]byte("hello"))) +}