diff --git a/cmd/symbols/apple_upload.go b/cmd/symbols/apple_upload.go index a2ac5a22..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 { @@ -46,7 +50,12 @@ 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 { +// +// With skipExisting, an unchanged binary keeps its UUID, so re-running this sends +// 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 { return fmt.Errorf("failed to find dSYM files: %w", err) @@ -64,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) + uploadURLs, err := getSymbolUploadUrls(apiKey, projectID, keys, digests, backendURL, skipExisting) if err != nil { return fmt.Errorf("failed to get upload URLs: %w", err) } @@ -78,13 +93,19 @@ 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") + reportUploadSummary(skipped) return nil } @@ -139,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 10cdb318..ec0fab60 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,9 @@ func uploadFlutterSymbols(apiKey, projectID, path, appVersion, backendURL string keys[i] = u.Key } - uploadURLs, err := getSymbolUploadUrls(apiKey, projectID, keys, backendURL) + // 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) } @@ -64,13 +69,19 @@ 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") + reportUploadSummary(skipped) return nil } diff --git a/cmd/symbols/upload.go b/cmd/symbols/upload.go index 48a57844..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" @@ -39,6 +41,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 +104,21 @@ const ( ) } ` + + // getSymbolUrlsDedupQuery asks the backend to answer keys it already stores with + // 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, $digests: [String!]) { + get_symbol_upload_urls_ld( + api_key: $api_key + project_id: $project_id + paths: $paths + skip_existing: $skip_existing + digests: $digests + ) + } + ` ) // reactNativeUploadSuffixes are the files produced by `react-native bundle`: @@ -191,6 +213,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 +224,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 +232,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) @@ -252,7 +276,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, 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) @@ -271,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) + // 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) } @@ -282,23 +317,58 @@ 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") + reportUploadSummary(skipped) 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 == "" +} + +// 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. @@ -480,15 +550,64 @@ 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 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 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 +} + +// 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, digests []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 + // 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{}{ - "query": getSymbolUrlsQuery, + "query": query, "variables": variables, }) if err != nil { @@ -589,6 +708,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..596f96b1 --- /dev/null +++ b/cmd/symbols/upload_dedup_test.go @@ -0,0 +1,185 @@ +package symbols + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "os" + "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")) +} + +// 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"]}}`) + + urls, err := getSymbolUploadUrls("key", "proj", []string{"a", "b"}, nil, 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]) + 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"}, []string{contentDigest([]byte("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) + assert.NotContains(t, req.Variables, digestsArgument) +} + +// 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) { + 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 +// 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"}, 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"))) +}