From 4222ef3ed672edc2c3647cc91cef53fa301fcaa6 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Tue, 15 Sep 2026 22:49:45 +0100 Subject: [PATCH 01/12] feat(snapshot s3): download objects in parallel within a count and byte budget Objects that need content now download concurrently. A slot channel bounds how many are in flight and a weighted semaphore bounds the sum of their listed sizes, so peak temp disk stays around the budget rather than growing with the bucket; an object larger than the whole budget takes all of it and runs alone. Results are written by listing index, and the first failure cancels the shared context so in-flight transfers stop and no further one starts. Defaults are eight objects and 512 MiB, which fits the default Lambda /tmp, and the transfer manager's per-object part concurrency is lowered to three so the connection count stays modest. The root .kosli_ignore is still fetched first, since its rules decide what else to download. --- go.mod | 2 +- internal/aws/aws.go | 124 +++++++++++++-- internal/aws/s3_fingerprint_test.go | 18 +-- internal/aws/s3_parallel_test.go | 238 ++++++++++++++++++++++++++++ 4 files changed, 354 insertions(+), 28 deletions(-) create mode 100644 internal/aws/s3_parallel_test.go diff --git a/go.mod b/go.mod index 71550fa26..37e4bbdab 100644 --- a/go.mod +++ b/go.mod @@ -50,6 +50,7 @@ require ( github.com/zalando/go-keyring v0.2.8 gitlab.com/gitlab-org/api/client-go v1.46.0 golang.org/x/oauth2 v0.37.0 + golang.org/x/sync v0.22.0 golang.org/x/term v0.46.0 google.golang.org/api v0.297.0 google.golang.org/grpc v1.83.2 @@ -249,7 +250,6 @@ require ( golang.org/x/exp v0.0.0-20260410095643-746e56fc9e2f // indirect golang.org/x/mod v0.38.0 // indirect golang.org/x/net v0.58.0 // indirect - golang.org/x/sync v0.22.0 // indirect golang.org/x/sys v0.48.0 // indirect golang.org/x/text v0.41.0 // indirect golang.org/x/time v0.15.0 // indirect diff --git a/internal/aws/aws.go b/internal/aws/aws.go index 8787dffbf..721187cfe 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -27,6 +27,7 @@ import ( "github.com/kosli-dev/cli/internal/digest" "github.com/kosli-dev/cli/internal/filters" "github.com/kosli-dev/cli/internal/logger" + "golang.org/x/sync/semaphore" ) // EcsEnvRequest represents the PUT request body to be sent to kosli from ECS @@ -169,7 +170,13 @@ func defaultNewS3Client(creds *AWSStaticCreds) (S3API, error) { if err != nil { return nil, err } - return &s3Client{S3ListAPI: client, S3DownloadAPI: transfermanager.New(client)}, nil + // Objects download in parallel (see downloadLimits), and the transfer manager + // fetches each object's parts in parallel on top of that. Its default of five + // parts per object times the object concurrency would open more connections + // than helps; three keeps the product modest while still splitting large objects. + return &s3Client{S3ListAPI: client, S3DownloadAPI: transfermanager.New(client, func(o *transfermanager.Options) { + o.Concurrency = 3 + })}, nil } // NewS3ClientFunc is the factory used by GetS3Data to create an S3API client. @@ -480,7 +487,7 @@ func getS3DataFromClient(client S3API, bucket string, includePaths, includeRegex return s3Data, fmt.Errorf("bucket [%s] reported no modification time for any matching object", bucket) } - artifactName, sha256, err := fingerprintS3Objects(client, bucket, objects, logger) + artifactName, sha256, err := fingerprintS3Objects(client, bucket, objects, defaultDownloadLimits, logger) if err != nil { return s3Data, err } @@ -494,8 +501,24 @@ func getS3DataFromClient(client S3API, bucket string, includePaths, includeRegex type s3Object struct { key string lastModified time.Time + size int64 } +// downloadLimits bounds the object downloads in flight at once. +type downloadLimits struct { + // concurrency is the number of objects downloading at the same time. + concurrency int + // bytesInFlight caps the sum of the listed sizes of the objects downloading + // at the same time, and so the temp disk they occupy. An object larger than + // the whole budget downloads alone. + bytesInFlight int64 +} + +// defaultDownloadLimits keeps peak temp disk around half a gigabyte, which fits +// the default Lambda /tmp, and the connection count modest together with the +// transfer manager's per-object part concurrency. +var defaultDownloadLimits = downloadLimits{concurrency: 8, bytesInFlight: 512 << 20} + // listMatchingS3Objects lists the bucket, dropping folder markers and keys the // filters exclude, in the order S3 returns them. func listMatchingS3Objects(client S3ListAPI, bucket string, includePaths []string, includeRegex []*regexp.Regexp, @@ -532,11 +555,14 @@ func listMatchingS3Objects(client S3ListAPI, bucket string, includePaths []strin seen[*object.Key] = true // An object without a timestamp stays in the fingerprint and out of // the snapshot timestamp, as it was before. - var lastModified time.Time + listed := s3Object{key: *object.Key} if object.LastModified != nil { - lastModified = *object.LastModified + listed.lastModified = *object.LastModified } - objects = append(objects, s3Object{key: *object.Key, lastModified: lastModified}) + if object.Size != nil { + listed.size = *object.Size + } + objects = append(objects, listed) } } return objects, nil @@ -550,8 +576,9 @@ func listMatchingS3Objects(client S3ListAPI, bucket string, includePaths []strin // is fingerprinted as that file and named after it, as before. // // A root .kosli_ignore is downloaded first so its rules can be applied, and -// objects the rules exclude are not downloaded at all. -func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3Object, logger *logger.Logger) (string, string, error) { +// objects the rules exclude are not downloaded at all. The remaining objects +// download in parallel within limits; the first failure cancels the rest. +func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3Object, limits downloadLimits, logger *logger.Logger) (string, string, error) { keys := make([]string, len(objects)) for i, object := range objects { keys[i] = object.key @@ -572,7 +599,7 @@ func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3O }() // The manifest starts as paths only; digests are filled in below by index, - // so it stays in listing order. + // so it stays in listing order however the downloads interleave. files := make([]digest.VirtualFile, len(objects)) for i, object := range objects { files[i].Path = paths[object.key] @@ -581,7 +608,7 @@ func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3O // One object is fingerprinted as that file and named after it, as it was // when the objects were laid out on disk. if file, ok := digest.SingleVirtualFile(files); ok { - sha256, err := downloadAndHashS3Object(downloader, tempDir, bucket, objects[0].key, nil, logger) + sha256, err := downloadAndHashS3Object(context.TODO(), downloader, tempDir, bucket, objects[0].key, nil, logger) if err != nil { return "", "", err } @@ -594,7 +621,7 @@ func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3O if paths[key] != digest.IgnoreFileName { continue } - sha256, err := downloadAndHashS3Object(downloader, tempDir, bucket, key, func(file *os.File) error { + sha256, err := downloadAndHashS3Object(context.TODO(), downloader, tempDir, bucket, key, func(file *os.File) error { if _, err := file.Seek(0, io.SeekStart); err != nil { return err } @@ -617,21 +644,25 @@ func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3O return "", "", ignoreRuleError(err) } + var toDownload []int for i, object := range objects { sha256, downloaded := contentSha256[object.key] switch { case downloaded: // the ignore-file pass already hashed this object case needed[files[i].Path]: - sha256, err = downloadAndHashS3Object(downloader, tempDir, bucket, object.key, nil, logger) - if err != nil { - return "", "", err - } + toDownload = append(toDownload, i) default: logger.Debug("object key [%s] is excluded by %s and is not downloaded", object.key, digest.IgnoreFileName) } files[i].Sha256 = sha256 } + // Each download writes its own slot, so the manifest stays in listing order + // however the downloads interleave. + if err := downloadS3ObjectsInParallel(downloader, tempDir, bucket, objects, toDownload, files, limits, logger); err != nil { + return "", "", err + } + sha256, err := digest.VirtualDirSha256(files, rules, logger) if err != nil { return "", "", ignoreRuleError(err) @@ -648,11 +679,72 @@ func ignoreRuleError(err error) error { return err } +// downloadS3ObjectsInParallel fetches the objects at the given indexes and +// writes each content digest into files at the same index. A slot channel +// bounds the number of downloads and a weighted semaphore bounds their listed +// bytes; the first error cancels the shared context so in-flight transfers +// stop and no further one starts. +func downloadS3ObjectsInParallel(downloader S3DownloadAPI, tempDir, bucket string, objects []s3Object, indexes []int, + files []digest.VirtualFile, limits downloadLimits, logger *logger.Logger) error { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + slots := make(chan struct{}, max(limits.concurrency, 1)) + budget := semaphore.NewWeighted(max(limits.bytesInFlight, 1)) + firstErr := make(chan error, 1) + var wg sync.WaitGroup + + for _, i := range indexes { + wg.Add(1) + go func(i int, object s3Object) { + defer wg.Done() + + select { + case slots <- struct{}{}: + case <-ctx.Done(): + return + } + defer func() { <-slots }() + // A slot and a cancellation can be ready together; never start a + // download once another has failed. + if ctx.Err() != nil { + return + } + + // An object larger than the budget takes all of it and so runs alone. + weight := max(min(object.size, limits.bytesInFlight), 1) + if err := budget.Acquire(ctx, weight); err != nil { + return // cancelled while waiting + } + defer budget.Release(weight) + + sha256, err := downloadAndHashS3Object(ctx, downloader, tempDir, bucket, object.key, nil, logger) + if err != nil { + select { + case firstErr <- err: + default: // an earlier failure is already recorded + } + cancel() + return + } + files[i].Sha256 = sha256 + }(i, objects[i]) + } + + wg.Wait() + select { + case err := <-firstErr: + return err + default: + return nil + } +} + // downloadAndHashS3Object fetches one object into a fresh temp file, lets // inspect read it when given, returns the sha256 of its content and removes the // file. The file's name comes from the OS, so nothing about the key reaches the // filesystem. -func downloadAndHashS3Object(downloader S3DownloadAPI, tempDir, bucket, key string, inspect func(*os.File) error, logger *logger.Logger) (string, error) { +func downloadAndHashS3Object(ctx context.Context, downloader S3DownloadAPI, tempDir, bucket, key string, inspect func(*os.File) error, logger *logger.Logger) (string, error) { file, err := os.CreateTemp(tempDir, "object-*") if err != nil { return "", fmt.Errorf("object key [%s]: %w", key, err) @@ -667,7 +759,7 @@ func downloadAndHashS3Object(downloader S3DownloadAPI, tempDir, bucket, key stri } }() - result, err := downloader.DownloadObject(context.TODO(), &transfermanager.DownloadObjectInput{ + result, err := downloader.DownloadObject(ctx, &transfermanager.DownloadObjectInput{ Bucket: aws.String(bucket), Key: aws.String(key), WriterAt: file, diff --git a/internal/aws/s3_fingerprint_test.go b/internal/aws/s3_fingerprint_test.go index 4fa6e8240..bc2e83349 100644 --- a/internal/aws/s3_fingerprint_test.go +++ b/internal/aws/s3_fingerprint_test.go @@ -197,8 +197,10 @@ func (suite *S3FingerprintTestSuite) TestDownloadsExactlyTheContributingObjects( require.Equal(suite.T(), []string{".kosli_ignore", "app.js", "lib/util.js"}, client.downloadedKeys()) } -// Objects are downloaded to files whose names owe nothing to the key, each is -// removed once hashed, and the download directory is gone at the end. +// Objects are downloaded to files whose names owe nothing to the key, every +// file is removed once hashed, and the download directory is gone at the end. +// Downloads overlap, so how many files exist at once is the byte budget's +// concern (see S3ParallelTestSuite), not this test's. func (suite *S3FingerprintTestSuite) TestObjectsNeverLandUnderTheirKeyAndDoNotLinger() { keys := []string{"alpha.bin", "beta/gamma.bin", "delta/epsilon/zeta.bin"} objects := map[string][]byte{} @@ -210,13 +212,6 @@ func (suite *S3FingerprintTestSuite) TestObjectsNeverLandUnderTheirKeyAndDoNotLi require.NotNil(suite.T(), file, "the transfer manager must be handed a real file") require.NotContains(suite.T(), filepath.Base(file.Name()), filepath.Base(key), "the local file name must owe nothing to the key") - client.mu.Lock() - earlier := append([]string{}, client.files[:len(client.files)-1]...) - client.mu.Unlock() - for _, previous := range earlier { - _, err := os.Stat(previous) - require.ErrorIs(suite.T(), err, os.ErrNotExist, "an earlier object's file must be gone before the next download") - } } _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) @@ -253,7 +248,8 @@ func (suite *S3FingerprintTestSuite) TestADownloadErrorNamesTheKey() { _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) require.Error(suite.T(), err) require.ErrorIs(suite.T(), err, os.ErrDeadlineExceeded) - require.Contains(suite.T(), err.Error(), "object key [README.md]") + // Downloads overlap, so either object may be the first to fail. + require.Regexp(suite.T(), `object key \[(README\.md|notes\.txt)\]`, err.Error()) require.NotContains(suite.T(), err.Error(), "--exclude-regex", "a transport failure must not advise dropping the object") } @@ -263,7 +259,7 @@ func (suite *S3FingerprintTestSuite) TestAHashErrorNamesTheKey() { client := &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: map[string][]byte{"README.md": []byte(fakeReadmeBody)}} // Deleting the file between download and hash is the one way to make the // hash fail without touching permissions. - _, err := downloadAndHashS3Object(client, suite.T().TempDir(), fakeS3TestBucketName, "README.md", func(file *os.File) error { + _, err := downloadAndHashS3Object(context.TODO(), client, suite.T().TempDir(), fakeS3TestBucketName, "README.md", func(file *os.File) error { return os.Remove(file.Name()) }, logger.NewStandardLogger()) require.Error(suite.T(), err) diff --git a/internal/aws/s3_parallel_test.go b/internal/aws/s3_parallel_test.go new file mode 100644 index 000000000..25e2facc0 --- /dev/null +++ b/internal/aws/s3_parallel_test.go @@ -0,0 +1,238 @@ +package aws + +import ( + "context" + "errors" + "fmt" + "math/rand" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/feature/s3/transfermanager" + "github.com/kosli-dev/cli/internal/digest" + "github.com/kosli-dev/cli/internal/logger" + "github.com/kosli-dev/cli/internal/utils" + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" +) + +type S3ParallelTestSuite struct { + suite.Suite +} + +// trackingDownloader records how many downloads, and how many listed bytes, +// are in flight at once, and how often each key is fetched. A delay keeps +// downloads overlapping so the bounds are actually exercised. +type trackingDownloader struct { + S3API + sizes map[string]int64 + delay time.Duration + // hook, when set, runs in place of the delegate for that key. + hook func(ctx context.Context, key string) error + + mu sync.Mutex + inFlight int + maxInFlight int + bytesInFlight int64 + maxBytesInFlight int64 + calls map[string]int +} + +func (d *trackingDownloader) DownloadObject(ctx context.Context, params *transfermanager.DownloadObjectInput, optFns ...func(*transfermanager.Options)) (*transfermanager.DownloadObjectOutput, error) { + key := *params.Key + d.mu.Lock() + if d.calls == nil { + d.calls = map[string]int{} + } + d.calls[key]++ + d.inFlight++ + d.bytesInFlight += d.sizes[key] + d.maxInFlight = max(d.maxInFlight, d.inFlight) + d.maxBytesInFlight = max(d.maxBytesInFlight, d.bytesInFlight) + d.mu.Unlock() + defer func() { + d.mu.Lock() + d.inFlight-- + d.bytesInFlight -= d.sizes[key] + d.mu.Unlock() + }() + + time.Sleep(d.delay) + if d.hook != nil { + if err := d.hook(ctx, key); err != nil { + return nil, err + } + } + return d.S3API.DownloadObject(ctx, params, optFns...) +} + +func (d *trackingDownloader) currentInFlight() int { + d.mu.Lock() + defer d.mu.Unlock() + return d.inFlight +} + +// bucketOf builds a fake bucket of n objects of the given size and returns the +// tracking downloader plus the listing fingerprintS3Objects takes. +func bucketOf(n int, size int, delay time.Duration) (*trackingDownloader, []s3Object) { + objects := map[string][]byte{} + listing := make([]s3Object, 0, n) + sizes := map[string]int64{} + for i := 0; i < n; i++ { + key := fmt.Sprintf("dir%d/object-%03d.bin", i%4, i) + body := []byte(fmt.Sprintf("%0*d", size, i)) + objects[key] = body + sizes[key] = int64(len(body)) + listing = append(listing, s3Object{key: key, size: int64(len(body))}) + } + fake := &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: objects} + return &trackingDownloader{S3API: fake, sizes: sizes, delay: delay}, listing +} + +func (suite *S3ParallelTestSuite) TestMakesExactlyOneCallPerObject() { + client, listing := bucketOf(60, 8, 0) + _, parallel, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, downloadLimits{concurrency: 8, bytesInFlight: 1 << 30}, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.Len(suite.T(), client.calls, 60) + for key, n := range client.calls { + require.Equal(suite.T(), 1, n, "key %s", key) + } + + sequential, _ := bucketOf(60, 8, 0) + _, want, err := fingerprintS3Objects(sequential, fakeS3TestBucketName, listing, downloadLimits{concurrency: 1, bytesInFlight: 1 << 30}, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.Equal(suite.T(), want, parallel) +} + +func (suite *S3ParallelTestSuite) TestRespectsTheConcurrencyBound() { + client, listing := bucketOf(40, 8, 5*time.Millisecond) + _, _, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, downloadLimits{concurrency: 4, bytesInFlight: 1 << 30}, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.LessOrEqual(suite.T(), client.maxInFlight, 4) + require.GreaterOrEqual(suite.T(), client.maxInFlight, 2, "downloads must actually overlap for the bound to be tested") +} + +// The byte budget binds before the count bound here: eight slots would allow +// eight 100-byte objects, the budget allows two. +func (suite *S3ParallelTestSuite) TestRespectsTheByteBudget() { + client, listing := bucketOf(20, 100, 5*time.Millisecond) + _, _, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, downloadLimits{concurrency: 8, bytesInFlight: 250}, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.LessOrEqual(suite.T(), client.maxBytesInFlight, int64(250)) + require.LessOrEqual(suite.T(), client.maxInFlight, 2) + require.GreaterOrEqual(suite.T(), client.maxInFlight, 2, "downloads must actually overlap for the budget to be tested") +} + +// An object larger than the whole budget must still download, and runs alone. +func (suite *S3ParallelTestSuite) TestAnObjectLargerThanTheBudgetRunsAlone() { + client, listing := bucketOf(12, 100, 5*time.Millisecond) + big := "big/huge.bin" + body := make([]byte, 1000) + client.S3API.(*FakeS3Client).Objects[big] = body + client.sizes[big] = 1000 + listing = append([]s3Object{{key: big, size: 1000}}, listing...) + + var aloneChecks int + client.hook = func(_ context.Context, key string) error { + if key == big { + suite.mu().Lock() + aloneChecks++ + suite.mu().Unlock() + require.Equal(suite.T(), 1, client.currentInFlight(), "the oversized object must be the only download in flight") + } + return nil + } + + _, _, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, downloadLimits{concurrency: 8, bytesInFlight: 250}, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.Equal(suite.T(), 1, aloneChecks) + require.Equal(suite.T(), 1, client.calls[big]) +} + +var suiteMu sync.Mutex + +func (suite *S3ParallelTestSuite) mu() *sync.Mutex { return &suiteMu } + +// Random per-object delays reorder completion; the fingerprint must equal what +// DirSha256 gives the same tree on disk, every time. +func (suite *S3ParallelTestSuite) TestFingerprintIsIndependentOfCompletionOrder() { + tree := map[string]string{} + for i := 0; i < 30; i++ { + tree[fmt.Sprintf("d%d/sub%d/f%02d.txt", i%3, i%5, i)] = fmt.Sprintf("content %d", i) + } + root := suite.T().TempDir() + objects := map[string][]byte{} + sizes := map[string]int64{} + listing := []s3Object{} + for p, content := range tree { + require.NoError(suite.T(), utils.CreateFileWithContent(filepath.Join(root, filepath.FromSlash(p)), content)) + objects[p] = []byte(content) + sizes[p] = int64(len(content)) + listing = append(listing, s3Object{key: p, size: int64(len(content))}) + } + want, err := digest.DirSha256(root, nil, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + + random := rand.New(rand.NewSource(5)) + for round := 0; round < 3; round++ { + client := &trackingDownloader{S3API: &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: objects}, sizes: sizes} + client.hook = func(_ context.Context, _ string) error { + suite.mu().Lock() + d := time.Duration(random.Intn(4)) * time.Millisecond + suite.mu().Unlock() + time.Sleep(d) + return nil + } + name, got, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, downloadLimits{concurrency: 8, bytesInFlight: 1 << 30}, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.Equal(suite.T(), fakeS3TestBucketName, name) + require.Equal(suite.T(), want, got, "round %d", round) + } +} + +// The first transport error cancels the shared context: an in-flight download +// sees it and returns, and no further download starts. Goroutines race for the +// slots, so the hook decides by arrival rather than by key: the first download +// to arrive fails, every other one blocks until it is cancelled. +func (suite *S3ParallelTestSuite) TestATransportErrorStopsRemainingWork() { + client, listing := bucketOf(10, 8, 0) + errBoom := errors.New("boom") + var arrivals int + var failingKey string + client.hook = func(ctx context.Context, key string) error { + suite.mu().Lock() + arrivals++ + first := arrivals == 1 + if first { + failingKey = key + } + suite.mu().Unlock() + if first { + time.Sleep(5 * time.Millisecond) // let the second slot fill first + return errBoom + } + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(5 * time.Second): + return errors.New("the context was never cancelled") + } + } + + _, _, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, downloadLimits{concurrency: 2, bytesInFlight: 1 << 30}, logger.NewStandardLogger()) + require.Error(suite.T(), err) + require.ErrorIs(suite.T(), err, errBoom) + require.Contains(suite.T(), err.Error(), fmt.Sprintf("object key [%s]", failingKey)) + require.Len(suite.T(), client.calls, 2, "only the two downloads in flight at the failure may have started: %v", client.calls) +} + +func (suite *S3ParallelTestSuite) TestDefaultLimitsAreSane() { + require.GreaterOrEqual(suite.T(), defaultDownloadLimits.concurrency, 2) + require.GreaterOrEqual(suite.T(), defaultDownloadLimits.bytesInFlight, int64(64<<20)) +} + +func TestS3ParallelTestSuite(t *testing.T) { + suite.Run(t, new(S3ParallelTestSuite)) +} From 9a243a337ea7c2d78587251b0fc5a9846924c3e2 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Tue, 15 Sep 2026 22:49:46 +0100 Subject: [PATCH 02/12] feat(snapshot s3): add --download-concurrency and --download-budget flags The parallel download limits were compile-time constants. How much temp disk and how many connections a snapshot may use depends on where it runs, from a default Lambda /tmp of 512 MiB to a fat CI runner, so both are now flags with the previous values as defaults. As with every flag, KOSLI_DOWNLOAD_BUDGET and KOSLI_DOWNLOAD_CONCURRENCY set them from the environment. `--download-budget` reads a size: a bare number is megabytes, matching how Lambda's ephemeral storage is expressed, and a K, M, G or T suffix with an optional B picks the unit, so 512, 512M, 512MB and 0.5G all mean the same. Sizes are binary. Both flags are validated before any request is made, and the budget's default string is pinned to the aws default by a test so the two cannot drift. `aws.DownloadLimits` is exported and threaded from the command through GetS3Data, which tests use to prove the values reach the fan-out. --- cmd/kosli/byteSize.go | 63 +++++++++++++++++ cmd/kosli/byteSize_test.go | 69 +++++++++++++++++++ cmd/kosli/root.go | 2 + cmd/kosli/snapshotS3.go | 40 ++++++++--- cmd/kosli/snapshotS3_test.go | 28 ++++++++ .../testdata/empty-flag-audit-coverage.json | 2 + internal/aws/aws.go | 37 +++++----- internal/aws/aws_test.go | 16 ++--- internal/aws/s3_fingerprint_test.go | 16 ++--- internal/aws/s3_parallel_test.go | 34 ++++++--- 10 files changed, 256 insertions(+), 51 deletions(-) create mode 100644 cmd/kosli/byteSize.go create mode 100644 cmd/kosli/byteSize_test.go diff --git a/cmd/kosli/byteSize.go b/cmd/kosli/byteSize.go new file mode 100644 index 000000000..6dc903440 --- /dev/null +++ b/cmd/kosli/byteSize.go @@ -0,0 +1,63 @@ +package main + +import ( + "errors" + "fmt" + "math" + "strconv" + "strings" + "unicode" +) + +// byteSizeUnits maps a lower-cased unit suffix to its size in bytes. Sizes are +// binary, as Lambda's /tmp and most disk figures are. The trailing "b" or "ib" +// is stripped before lookup, so "M", "MB" and "MiB" all land on the same entry. +var byteSizeUnits = map[string]int64{ + "": 1 << 20, // a bare number is megabytes + "b": 1, + "k": 1 << 10, + "m": 1 << 20, + "g": 1 << 30, + "t": 1 << 40, +} + +// parseByteSize reads a size such as "512", "512M", "8GB" or "1.5G". A bare +// number is megabytes; a unit suffix, case-insensitive and with an optional B, +// selects kilobytes, megabytes, gigabytes or terabytes; "B" alone means bytes. +func parseByteSize(s string) (int64, error) { + s = strings.TrimSpace(s) + if s == "" { + return 0, errors.New("size is empty") + } + + digits := 0 + for digits < len(s) && (s[digits] >= '0' && s[digits] <= '9' || s[digits] == '.') { + digits++ + } + number, unit := s[:digits], strings.TrimSpace(s[digits:]) + if number == "" || strings.ContainsFunc(unit, func(r rune) bool { return !unicode.IsLetter(r) }) { + return 0, fmt.Errorf("%q is not a size: expected a number with an optional K, M, G or T unit, e.g. 512M", s) + } + value, err := strconv.ParseFloat(number, 64) + if err != nil { + return 0, fmt.Errorf("%q is not a size: expected a number with an optional K, M, G or T unit, e.g. 512M", s) + } + + key := strings.ToLower(unit) + if key != "b" { + key = strings.TrimSuffix(strings.TrimSuffix(key, "ib"), "b") + } + multiplier, ok := byteSizeUnits[key] + if !ok { + return 0, fmt.Errorf("unknown unit %q in size %q: use K, M, G or T, optionally followed by B", unit, s) + } + + bytes := value * float64(multiplier) + if bytes >= math.MaxInt64 { + return 0, fmt.Errorf("size %q is too large", s) + } + if bytes < 1 { + return 0, fmt.Errorf("size %q must be at least 1 byte", s) + } + return int64(bytes), nil +} diff --git a/cmd/kosli/byteSize_test.go b/cmd/kosli/byteSize_test.go new file mode 100644 index 000000000..75b69b70e --- /dev/null +++ b/cmd/kosli/byteSize_test.go @@ -0,0 +1,69 @@ +package main + +import ( + "testing" + + "github.com/kosli-dev/cli/internal/aws" + "github.com/stretchr/testify/require" +) + +func TestParseByteSize(t *testing.T) { + const mib = int64(1) << 20 + for _, tc := range []struct { + input string + want int64 + wantErr string + }{ + // A bare number is megabytes, matching how Lambda's ephemeral storage is expressed. + {input: "512", want: 512 * mib}, + {input: "1", want: mib}, + {input: " 64 ", want: 64 * mib}, + // A suffix picks the unit; the trailing B is optional and case does not matter. + {input: "512M", want: 512 * mib}, + {input: "512MB", want: 512 * mib}, + {input: "512mb", want: 512 * mib}, + {input: "512MiB", want: 512 * mib}, + {input: "8G", want: 8 << 30}, + {input: "8GB", want: 8 << 30}, + {input: "8 GB", want: 8 << 30}, + {input: "2T", want: 2 << 40}, + {input: "1024K", want: 1 << 20}, + {input: "4096KB", want: 4 << 20}, + {input: "1000B", want: 1000}, + {input: "1.5G", want: 3 << 29}, + {input: "0.5M", want: 512 << 10}, + // Rejected: nothing to download into, or not a size at all. + {input: "", wantErr: "empty"}, + {input: "0", wantErr: "must be at least 1 byte"}, + {input: "0B", wantErr: "must be at least 1 byte"}, + {input: "-1", wantErr: "not a size"}, + {input: "-512M", wantErr: "not a size"}, + {input: "abc", wantErr: "not a size"}, + {input: "M", wantErr: "not a size"}, + {input: "512X", wantErr: `unknown unit "X"`}, + {input: "512 megabytes", wantErr: `unknown unit "megabytes"`}, + {input: "1e3", wantErr: "not a size"}, + {input: "0x10", wantErr: "not a size"}, + {input: "1,024", wantErr: "not a size"}, + {input: "99999999999T", wantErr: "too large"}, + } { + t.Run(tc.input, func(t *testing.T) { + got, err := parseByteSize(tc.input) + if tc.wantErr != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.wantErr) + return + } + require.NoError(t, err) + require.Equal(t, tc.want, got) + }) + } +} + +// The flag's default is spelled as a string, so pin it to the value the aws +// package uses when no flag is given. +func TestDefaultDownloadBudgetMatchesTheAwsDefault(t *testing.T) { + got, err := parseByteSize(defaultDownloadBudget) + require.NoError(t, err) + require.Equal(t, aws.DefaultDownloadLimits.BytesInFlight, got) +} diff --git a/cmd/kosli/root.go b/cmd/kosli/root.go index 18b17bd3a..24909ef26 100644 --- a/cmd/kosli/root.go +++ b/cmd/kosli/root.go @@ -259,6 +259,8 @@ Paths the list already matches stay excluded whatever is later added there, so k awsSecretKeyFlag = "The AWS secret access key." awsRegionFlag = "The AWS region." bucketNameFlag = "The name of the S3 bucket." + downloadConcurrencyFlag = "[optional] The number of S3 objects to download at the same time when fingerprinting the bucket." + downloadBudgetFlag = "[optional] The maximum total size of the S3 objects downloading at the same time, which caps the temporary disk the snapshot uses. A bare number is megabytes; add K, M, G or T (optionally followed by B) to choose the unit, e.g. 512M or 8G. An object larger than the budget still downloads, on its own. Set TMPDIR to choose where objects are downloaded to." bucketPathsFlag = "[optional] The comma separated list of file and/or directory paths in the S3 bucket to include when fingerprinting. Paths match by literal prefix. Cannot be used together with --exclude or --exclude-regex." bucketPathsRegexFlag = "[optional] The comma separated list of Go regular expressions matched against object keys in the S3 bucket to include when fingerprinting. Cannot be used together with --exclude or --exclude-regex." excludeBucketPathsFlag = "[optional] The comma separated list of file and/or directory paths in the S3 bucket to exclude when fingerprinting. Paths match by literal prefix. Cannot be used together with --include or --include-regex." diff --git a/cmd/kosli/snapshotS3.go b/cmd/kosli/snapshotS3.go index 975c4a2f8..fcd0e448e 100644 --- a/cmd/kosli/snapshotS3.go +++ b/cmd/kosli/snapshotS3.go @@ -1,6 +1,7 @@ package main import ( + "fmt" "io" "net/http" "net/url" @@ -71,12 +72,15 @@ kosli snapshot s3 yourEnvironmentName \ ` type snapshotS3Options struct { - bucket string - includePaths []string - includeRegex []string - excludePaths []string - excludeRegex []string - awsStaticCreds *aws.AWSStaticCreds + bucket string + includePaths []string + includeRegex []string + excludePaths []string + excludeRegex []string + downloadConcurrency int + downloadBudget string + downloadLimits aws.DownloadLimits + awsStaticCreds *aws.AWSStaticCreds } func newSnapshotS3Cmd(out io.Writer) *cobra.Command { @@ -108,7 +112,7 @@ func newSnapshotS3Cmd(out io.Writer) *cobra.Command { } } - return nil + return o.resolveDownloadLimits() }, RunE: func(cmd *cobra.Command, args []string) error { return o.run(args) @@ -120,6 +124,8 @@ func newSnapshotS3Cmd(out io.Writer) *cobra.Command { cmd.Flags().StringSliceVar(&o.includeRegex, "include-regex", []string{}, bucketPathsRegexFlag) cmd.Flags().StringSliceVarP(&o.excludePaths, "exclude", "x", []string{}, excludeBucketPathsFlag) cmd.Flags().StringSliceVar(&o.excludeRegex, "exclude-regex", []string{}, excludeBucketPathsRegexFlag) + cmd.Flags().IntVar(&o.downloadConcurrency, "download-concurrency", aws.DefaultDownloadLimits.Concurrency, downloadConcurrencyFlag) + cmd.Flags().StringVar(&o.downloadBudget, "download-budget", defaultDownloadBudget, downloadBudgetFlag) addAWSAuthFlags(cmd, o.awsStaticCreds) addDryRunFlag(cmd) @@ -143,7 +149,7 @@ func (o *snapshotS3Options) run(args []string) error { return err } - s3Data, err := o.awsStaticCreds.GetS3Data(o.bucket, o.includePaths, o.includeRegex, o.excludePaths, o.excludeRegex, logger) + s3Data, err := o.awsStaticCreds.GetS3Data(o.bucket, o.includePaths, o.includeRegex, o.excludePaths, o.excludeRegex, o.downloadLimits, logger) if err != nil { return err } @@ -164,3 +170,21 @@ func (o *snapshotS3Options) run(args []string) error { } return err } + +// defaultDownloadBudget spells aws.DefaultDownloadLimits.BytesInFlight the way +// the flag reads it. +const defaultDownloadBudget = "512M" + +// resolveDownloadLimits validates the download flags and turns them into the +// limits the aws package takes. +func (o *snapshotS3Options) resolveDownloadLimits() error { + if o.downloadConcurrency < 1 { + return fmt.Errorf("--download-concurrency must be at least 1, got %d", o.downloadConcurrency) + } + budget, err := parseByteSize(o.downloadBudget) + if err != nil { + return fmt.Errorf("invalid --download-budget: %v", err) + } + o.downloadLimits = aws.DownloadLimits{Concurrency: o.downloadConcurrency, BytesInFlight: budget} + return nil +} diff --git a/cmd/kosli/snapshotS3_test.go b/cmd/kosli/snapshotS3_test.go index b83af80dc..4932700f0 100644 --- a/cmd/kosli/snapshotS3_test.go +++ b/cmd/kosli/snapshotS3_test.go @@ -113,6 +113,34 @@ func (suite *SnapshotS3TestSuite) TestSnapshotS3Cmd() { cmd: fmt.Sprintf(`snapshot s3 %s %s --bucket %s --exclude dummy`, suite.envName, suite.defaultKosliArguments, suite.bucketName), golden: "bucket kosli-cli-public was reported to environment snapshot-s3-env\n", }, + { + name: "download limits can be set, with a bare number read as megabytes", + cmd: fmt.Sprintf(`snapshot s3 %s %s --bucket %s --download-concurrency 2 --download-budget 64`, suite.envName, suite.defaultKosliArguments, suite.bucketName), + golden: "bucket kosli-cli-public was reported to environment snapshot-s3-env\n", + }, + { + name: "the download budget takes a unit suffix", + cmd: fmt.Sprintf(`snapshot s3 %s %s --bucket %s --download-budget 2GB`, suite.envName, suite.defaultKosliArguments, suite.bucketName), + golden: "bucket kosli-cli-public was reported to environment snapshot-s3-env\n", + }, + { + wantError: true, + name: "snapshot s3 fails if --download-concurrency is below 1", + cmd: fmt.Sprintf(`snapshot s3 %s %s --bucket %s --download-concurrency 0`, suite.envName, suite.defaultKosliArguments, suite.bucketName), + golden: "Error: --download-concurrency must be at least 1, got 0\n", + }, + { + wantError: true, + name: "snapshot s3 fails if --download-budget is not a size", + cmd: fmt.Sprintf(`snapshot s3 %s %s --bucket %s --download-budget large`, suite.envName, suite.defaultKosliArguments, suite.bucketName), + golden: "Error: invalid --download-budget: \"large\" is not a size: expected a number with an optional K, M, G or T unit, e.g. 512M\n", + }, + { + wantError: true, + name: "snapshot s3 fails if --download-budget is zero", + cmd: fmt.Sprintf(`snapshot s3 %s %s --bucket %s --download-budget 0`, suite.envName, suite.defaultKosliArguments, suite.bucketName), + golden: "Error: invalid --download-budget: size \"0\" must be at least 1 byte\n", + }, } for _, t := range tests { diff --git a/cmd/kosli/testdata/empty-flag-audit-coverage.json b/cmd/kosli/testdata/empty-flag-audit-coverage.json index fcb151ca5..6a40d2f23 100644 --- a/cmd/kosli/testdata/empty-flag-audit-coverage.json +++ b/cmd/kosli/testdata/empty-flag-audit-coverage.json @@ -874,6 +874,8 @@ "aws-region": "string", "aws-secret-key": "string", "bucket": "string", + "download-budget": "string", + "download-concurrency": "int", "dry-run": "bool", "exclude": "stringSlice", "exclude-regex": "stringSlice", diff --git a/internal/aws/aws.go b/internal/aws/aws.go index 721187cfe..5322594ad 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -170,7 +170,7 @@ func defaultNewS3Client(creds *AWSStaticCreds) (S3API, error) { if err != nil { return nil, err } - // Objects download in parallel (see downloadLimits), and the transfer manager + // Objects download in parallel (see DownloadLimits), and the transfer manager // fetches each object's parts in parallel on top of that. Its default of five // parts per object times the object concurrency would open more connections // than helps; three keeps the product modest while still splitting large objects. @@ -448,16 +448,16 @@ func objectMatchesFilter(key string, paths []string, patterns []*regexp.Regexp) // includePaths / excludePaths match object keys by literal prefix. // includeRegex / excludeRegex match object keys by Go regular expression. // Include and exclude filters are mutually exclusive (callers enforce this). -func (staticCreds *AWSStaticCreds) GetS3Data(bucket string, includePaths, includeRegex, excludePaths, excludeRegex []string, logger *logger.Logger) ([]*S3Data, error) { +func (staticCreds *AWSStaticCreds) GetS3Data(bucket string, includePaths, includeRegex, excludePaths, excludeRegex []string, limits DownloadLimits, logger *logger.Logger) ([]*S3Data, error) { client, err := NewS3ClientFunc(staticCreds) if err != nil { return []*S3Data{}, err } - return getS3DataFromClient(client, bucket, includePaths, includeRegex, excludePaths, excludeRegex, logger) + return getS3DataFromClient(client, bucket, includePaths, includeRegex, excludePaths, excludeRegex, limits, logger) } // getS3DataFromClient harvests bucket content using the provided S3API client. -func getS3DataFromClient(client S3API, bucket string, includePaths, includeRegex, excludePaths, excludeRegex []string, logger *logger.Logger) ([]*S3Data, error) { +func getS3DataFromClient(client S3API, bucket string, includePaths, includeRegex, excludePaths, excludeRegex []string, limits DownloadLimits, logger *logger.Logger) ([]*S3Data, error) { s3Data := []*S3Data{} includeRegexCompiled, err := compilePathRegex(includeRegex) @@ -487,7 +487,7 @@ func getS3DataFromClient(client S3API, bucket string, includePaths, includeRegex return s3Data, fmt.Errorf("bucket [%s] reported no modification time for any matching object", bucket) } - artifactName, sha256, err := fingerprintS3Objects(client, bucket, objects, defaultDownloadLimits, logger) + artifactName, sha256, err := fingerprintS3Objects(client, bucket, objects, limits, logger) if err != nil { return s3Data, err } @@ -504,20 +504,21 @@ type s3Object struct { size int64 } -// downloadLimits bounds the object downloads in flight at once. -type downloadLimits struct { - // concurrency is the number of objects downloading at the same time. - concurrency int - // bytesInFlight caps the sum of the listed sizes of the objects downloading +// DownloadLimits bounds the object downloads in flight at once when +// fingerprinting a bucket. +type DownloadLimits struct { + // Concurrency is the number of objects downloading at the same time. + Concurrency int + // BytesInFlight caps the sum of the listed sizes of the objects downloading // at the same time, and so the temp disk they occupy. An object larger than // the whole budget downloads alone. - bytesInFlight int64 + BytesInFlight int64 } -// defaultDownloadLimits keeps peak temp disk around half a gigabyte, which fits +// DefaultDownloadLimits keeps peak temp disk around half a gigabyte, which fits // the default Lambda /tmp, and the connection count modest together with the // transfer manager's per-object part concurrency. -var defaultDownloadLimits = downloadLimits{concurrency: 8, bytesInFlight: 512 << 20} +var DefaultDownloadLimits = DownloadLimits{Concurrency: 8, BytesInFlight: 512 << 20} // listMatchingS3Objects lists the bucket, dropping folder markers and keys the // filters exclude, in the order S3 returns them. @@ -578,7 +579,7 @@ func listMatchingS3Objects(client S3ListAPI, bucket string, includePaths []strin // A root .kosli_ignore is downloaded first so its rules can be applied, and // objects the rules exclude are not downloaded at all. The remaining objects // download in parallel within limits; the first failure cancels the rest. -func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3Object, limits downloadLimits, logger *logger.Logger) (string, string, error) { +func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3Object, limits DownloadLimits, logger *logger.Logger) (string, string, error) { keys := make([]string, len(objects)) for i, object := range objects { keys[i] = object.key @@ -685,12 +686,12 @@ func ignoreRuleError(err error) error { // bytes; the first error cancels the shared context so in-flight transfers // stop and no further one starts. func downloadS3ObjectsInParallel(downloader S3DownloadAPI, tempDir, bucket string, objects []s3Object, indexes []int, - files []digest.VirtualFile, limits downloadLimits, logger *logger.Logger) error { + files []digest.VirtualFile, limits DownloadLimits, logger *logger.Logger) error { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - slots := make(chan struct{}, max(limits.concurrency, 1)) - budget := semaphore.NewWeighted(max(limits.bytesInFlight, 1)) + slots := make(chan struct{}, max(limits.Concurrency, 1)) + budget := semaphore.NewWeighted(max(limits.BytesInFlight, 1)) firstErr := make(chan error, 1) var wg sync.WaitGroup @@ -712,7 +713,7 @@ func downloadS3ObjectsInParallel(downloader S3DownloadAPI, tempDir, bucket strin } // An object larger than the budget takes all of it and so runs alone. - weight := max(min(object.size, limits.bytesInFlight), 1) + weight := max(min(object.size, limits.BytesInFlight), 1) if err := budget.Acquire(ctx, weight); err != nil { return // cancelled while waiting } diff --git a/internal/aws/aws_test.go b/internal/aws/aws_test.go index 6c6412a15..afc3bacaa 100644 --- a/internal/aws/aws_test.go +++ b/internal/aws/aws_test.go @@ -433,7 +433,7 @@ func (suite *AWSTestSuite) TestGetS3Data() { } { suite.Run(t.name, func() { skipIfCredsUnset(suite.T(), t.requireEnvVars, t.creds) - data, err := t.creds.GetS3Data(t.bucketName, t.includePaths, nil, t.excludePaths, nil, logger.NewStandardLogger()) + data, err := t.creds.GetS3Data(t.bucketName, t.includePaths, nil, t.excludePaths, nil, DefaultDownloadLimits, logger.NewStandardLogger()) require.False(suite.T(), (err != nil) != t.wantErr, "GetS3Data() error = %v, wantErr %v", err, t.wantErr) if !t.wantErr { @@ -1178,7 +1178,7 @@ func (suite *AWSTestSuite) TestGetS3DataFromClient() { } data, err := getS3DataFromClient(client, fakeS3TestBucketName, t.includePaths, - t.includeRegex, t.excludePaths, t.excludeRegex, logger.NewStandardLogger()) + t.includeRegex, t.excludePaths, t.excludeRegex, DefaultDownloadLimits, logger.NewStandardLogger()) if t.wantErr { require.Error(suite.T(), err) @@ -1221,7 +1221,7 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientFilterEquivalence() { fingerprint := func(objects map[string][]byte, includePaths, includeRegex, excludePaths, excludeRegex []string) string { client := &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: objects} data, err := getS3DataFromClient(client, fakeS3TestBucketName, includePaths, - includeRegex, excludePaths, excludeRegex, logger.NewStandardLogger()) + includeRegex, excludePaths, excludeRegex, DefaultDownloadLimits, logger.NewStandardLogger()) require.NoError(suite.T(), err) require.Len(suite.T(), data, 1) require.Len(suite.T(), data[0].Digests, 1) @@ -1292,7 +1292,7 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientRejectsKeysWithDotDotSegments( }, } - _, err := getS3DataFromClient(poisoned, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + _, err := getS3DataFromClient(poisoned, fakeS3TestBucketName, nil, nil, nil, nil, DefaultDownloadLimits, logger.NewStandardLogger()) require.Error(suite.T(), err, "a key containing a \"..\" segment must fail the snapshot instead of silently overwriting another object's download") require.Contains(suite.T(), err.Error(), "uploads/user-a/../../protected/release.bin") } @@ -1307,7 +1307,7 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientCollidingKeysAreAnError() { }, } - _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, DefaultDownloadLimits, logger.NewStandardLogger()) require.Error(suite.T(), err) require.Contains(suite.T(), err.Error(), "[a//b]", "the error must name both colliding keys") require.Contains(suite.T(), err.Error(), "[a/b]", "the error must name both colliding keys") @@ -1325,7 +1325,7 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientObjectAndPrefixCollideAreAnErr }, } - _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, DefaultDownloadLimits, logger.NewStandardLogger()) require.Error(suite.T(), err) require.Contains(suite.T(), err.Error(), "object key [a]", "the error must name the object") require.Contains(suite.T(), err.Error(), "object key [a/b]", "the error must name an object under the prefix") @@ -1358,9 +1358,9 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKey }, } - unusualData, err := getS3DataFromClient(unusual, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + unusualData, err := getS3DataFromClient(unusual, fakeS3TestBucketName, nil, nil, nil, nil, DefaultDownloadLimits, logger.NewStandardLogger()) require.NoError(suite.T(), err) - todayData, err := getS3DataFromClient(today, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + todayData, err := getS3DataFromClient(today, fakeS3TestBucketName, nil, nil, nil, nil, DefaultDownloadLimits, logger.NewStandardLogger()) require.NoError(suite.T(), err) require.Len(suite.T(), unusualData, 1) require.Len(suite.T(), todayData, 1) diff --git a/internal/aws/s3_fingerprint_test.go b/internal/aws/s3_fingerprint_test.go index bc2e83349..3ae54c41c 100644 --- a/internal/aws/s3_fingerprint_test.go +++ b/internal/aws/s3_fingerprint_test.go @@ -59,7 +59,7 @@ func (r *recordingDownloader) downloadedKeys() []string { func snapshotFake(t *testing.T, client S3API) (artifactName, fingerprint string) { t.Helper() - data, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + data, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, DefaultDownloadLimits, logger.NewStandardLogger()) require.NoError(t, err) require.Len(t, data, 1) require.Len(t, data[0].Digests, 1) @@ -191,7 +191,7 @@ func (suite *S3FingerprintTestSuite) TestDownloadsExactlyTheContributingObjects( "scratch.tmp": []byte("tmp"), "filtered/out.txt": []byte("out"), }}} - data, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, []string{"filtered/"}, nil, logger.NewStandardLogger()) + data, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, []string{"filtered/"}, nil, DefaultDownloadLimits, logger.NewStandardLogger()) require.NoError(suite.T(), err) require.Len(suite.T(), data, 1) require.Equal(suite.T(), []string{".kosli_ignore", "app.js", "lib/util.js"}, client.downloadedKeys()) @@ -214,7 +214,7 @@ func (suite *S3FingerprintTestSuite) TestObjectsNeverLandUnderTheirKeyAndDoNotLi "the local file name must owe nothing to the key") } - _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, DefaultDownloadLimits, logger.NewStandardLogger()) require.NoError(suite.T(), err) require.Len(suite.T(), client.files, len(keys)) for _, file := range client.files { @@ -233,7 +233,7 @@ func (suite *S3FingerprintTestSuite) TestAMalformedIgnoreRuleFailsTheSnapshot() client := &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: map[string][]byte{ ".kosli_ignore": []byte(rule + "\n"), "app.js": []byte("app"), }} - _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, DefaultDownloadLimits, logger.NewStandardLogger()) require.Error(suite.T(), err) require.Contains(suite.T(), err.Error(), "the bucket's .kosli_ignore holds a rule that cannot be applied") require.Contains(suite.T(), err.Error(), rule) @@ -245,7 +245,7 @@ func (suite *S3FingerprintTestSuite) TestADownloadErrorNamesTheKey() { client := &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: map[string][]byte{ "README.md": []byte(fakeReadmeBody), "notes.txt": []byte(fakeNotesBody), }, DownloadObjectErr: os.ErrDeadlineExceeded} - _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, DefaultDownloadLimits, logger.NewStandardLogger()) require.Error(suite.T(), err) require.ErrorIs(suite.T(), err, os.ErrDeadlineExceeded) // Downloads overlap, so either object may be the first to fail. @@ -276,19 +276,19 @@ func (suite *S3FingerprintTestSuite) TestAListingWithoutModificationTimesDoesNot objects := map[string][]byte{"README.md": []byte(fakeReadmeBody), "notes.txt": []byte(fakeNotesBody)} later := fakeS3LastModified.Add(time.Hour) full, err := getS3DataFromClient(&FakeS3Client{Bucket: fakeS3TestBucketName, Objects: objects, - LastModified: map[string]time.Time{"notes.txt": later}}, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + LastModified: map[string]time.Time{"notes.txt": later}}, fakeS3TestBucketName, nil, nil, nil, nil, DefaultDownloadLimits, logger.NewStandardLogger()) require.NoError(suite.T(), err) partial, err := getS3DataFromClient(&FakeS3Client{Bucket: fakeS3TestBucketName, Objects: objects, LastModified: map[string]time.Time{"notes.txt": later}, NoLastModified: map[string]bool{"README.md": true}}, - fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + fakeS3TestBucketName, nil, nil, nil, nil, DefaultDownloadLimits, logger.NewStandardLogger()) require.NoError(suite.T(), err) require.Equal(suite.T(), full[0].Digests, partial[0].Digests, "the object without a timestamp stays in the fingerprint") require.Equal(suite.T(), later.Unix(), partial[0].LastModifiedTimestamp) _, err = getS3DataFromClient(&FakeS3Client{Bucket: fakeS3TestBucketName, Objects: objects, NoLastModified: map[string]bool{"README.md": true, "notes.txt": true}}, - fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + fakeS3TestBucketName, nil, nil, nil, nil, DefaultDownloadLimits, logger.NewStandardLogger()) require.Error(suite.T(), err) require.Contains(suite.T(), err.Error(), "modification time") } diff --git a/internal/aws/s3_parallel_test.go b/internal/aws/s3_parallel_test.go index 25e2facc0..13b7f8321 100644 --- a/internal/aws/s3_parallel_test.go +++ b/internal/aws/s3_parallel_test.go @@ -93,7 +93,7 @@ func bucketOf(n int, size int, delay time.Duration) (*trackingDownloader, []s3Ob func (suite *S3ParallelTestSuite) TestMakesExactlyOneCallPerObject() { client, listing := bucketOf(60, 8, 0) - _, parallel, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, downloadLimits{concurrency: 8, bytesInFlight: 1 << 30}, logger.NewStandardLogger()) + _, parallel, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, DownloadLimits{Concurrency: 8, BytesInFlight: 1 << 30}, logger.NewStandardLogger()) require.NoError(suite.T(), err) require.Len(suite.T(), client.calls, 60) for key, n := range client.calls { @@ -101,14 +101,14 @@ func (suite *S3ParallelTestSuite) TestMakesExactlyOneCallPerObject() { } sequential, _ := bucketOf(60, 8, 0) - _, want, err := fingerprintS3Objects(sequential, fakeS3TestBucketName, listing, downloadLimits{concurrency: 1, bytesInFlight: 1 << 30}, logger.NewStandardLogger()) + _, want, err := fingerprintS3Objects(sequential, fakeS3TestBucketName, listing, DownloadLimits{Concurrency: 1, BytesInFlight: 1 << 30}, logger.NewStandardLogger()) require.NoError(suite.T(), err) require.Equal(suite.T(), want, parallel) } func (suite *S3ParallelTestSuite) TestRespectsTheConcurrencyBound() { client, listing := bucketOf(40, 8, 5*time.Millisecond) - _, _, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, downloadLimits{concurrency: 4, bytesInFlight: 1 << 30}, logger.NewStandardLogger()) + _, _, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, DownloadLimits{Concurrency: 4, BytesInFlight: 1 << 30}, logger.NewStandardLogger()) require.NoError(suite.T(), err) require.LessOrEqual(suite.T(), client.maxInFlight, 4) require.GreaterOrEqual(suite.T(), client.maxInFlight, 2, "downloads must actually overlap for the bound to be tested") @@ -118,7 +118,7 @@ func (suite *S3ParallelTestSuite) TestRespectsTheConcurrencyBound() { // eight 100-byte objects, the budget allows two. func (suite *S3ParallelTestSuite) TestRespectsTheByteBudget() { client, listing := bucketOf(20, 100, 5*time.Millisecond) - _, _, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, downloadLimits{concurrency: 8, bytesInFlight: 250}, logger.NewStandardLogger()) + _, _, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, DownloadLimits{Concurrency: 8, BytesInFlight: 250}, logger.NewStandardLogger()) require.NoError(suite.T(), err) require.LessOrEqual(suite.T(), client.maxBytesInFlight, int64(250)) require.LessOrEqual(suite.T(), client.maxInFlight, 2) @@ -145,7 +145,7 @@ func (suite *S3ParallelTestSuite) TestAnObjectLargerThanTheBudgetRunsAlone() { return nil } - _, _, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, downloadLimits{concurrency: 8, bytesInFlight: 250}, logger.NewStandardLogger()) + _, _, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, DownloadLimits{Concurrency: 8, BytesInFlight: 250}, logger.NewStandardLogger()) require.NoError(suite.T(), err) require.Equal(suite.T(), 1, aloneChecks) require.Equal(suite.T(), 1, client.calls[big]) @@ -185,7 +185,7 @@ func (suite *S3ParallelTestSuite) TestFingerprintIsIndependentOfCompletionOrder( time.Sleep(d) return nil } - name, got, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, downloadLimits{concurrency: 8, bytesInFlight: 1 << 30}, logger.NewStandardLogger()) + name, got, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, DownloadLimits{Concurrency: 8, BytesInFlight: 1 << 30}, logger.NewStandardLogger()) require.NoError(suite.T(), err) require.Equal(suite.T(), fakeS3TestBucketName, name) require.Equal(suite.T(), want, got, "round %d", round) @@ -221,16 +221,32 @@ func (suite *S3ParallelTestSuite) TestATransportErrorStopsRemainingWork() { } } - _, _, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, downloadLimits{concurrency: 2, bytesInFlight: 1 << 30}, logger.NewStandardLogger()) + _, _, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, DownloadLimits{Concurrency: 2, BytesInFlight: 1 << 30}, logger.NewStandardLogger()) require.Error(suite.T(), err) require.ErrorIs(suite.T(), err, errBoom) require.Contains(suite.T(), err.Error(), fmt.Sprintf("object key [%s]", failingKey)) require.Len(suite.T(), client.calls, 2, "only the two downloads in flight at the failure may have started: %v", client.calls) } +// The limits handed to getS3DataFromClient are the ones the fan-out obeys: the +// same bucket runs one at a time under a limit of one and overlaps under four. +func (suite *S3ParallelTestSuite) TestLimitsReachTheDownloader() { + for _, limit := range []int{1, 4} { + client, _ := bucketOf(12, 8, 3*time.Millisecond) + _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, + DownloadLimits{Concurrency: limit, BytesInFlight: 1 << 30}, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.Len(suite.T(), client.calls, 12) + require.LessOrEqual(suite.T(), client.maxInFlight, limit) + if limit > 1 { + require.GreaterOrEqual(suite.T(), client.maxInFlight, 2) + } + } +} + func (suite *S3ParallelTestSuite) TestDefaultLimitsAreSane() { - require.GreaterOrEqual(suite.T(), defaultDownloadLimits.concurrency, 2) - require.GreaterOrEqual(suite.T(), defaultDownloadLimits.bytesInFlight, int64(64<<20)) + require.GreaterOrEqual(suite.T(), DefaultDownloadLimits.Concurrency, 2) + require.GreaterOrEqual(suite.T(), DefaultDownloadLimits.BytesInFlight, int64(64<<20)) } func TestS3ParallelTestSuite(t *testing.T) { From 64e5446e297e6398a3a0fa80671eb794e6ce9231 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Tue, 15 Sep 2026 22:49:48 +0100 Subject: [PATCH 03/12] refactor(snapshot s3): download with a fixed worker pool instead of a goroutine per object The fan-out started one goroutine per object needing content and parked all but Concurrency of them on a slot channel. Parked goroutines keep their stacks, so memory grew with the bucket: around 2000 goroutines for a 2000-object bucket, hundreds of megabytes at a hundred thousand. Exactly Concurrency workers now pull indexes from a channel fed in listing order. The byte budget, the by-index results and the first-error cancellation are unchanged; the producer stops feeding once the context is cancelled and the workers drain out. A test pins goroutines during a 2000-object run to the concurrency plus a small fixed overhead. --- internal/aws/aws.go | 78 +++++++++++++++++--------------- internal/aws/s3_parallel_test.go | 24 ++++++++++ 2 files changed, 65 insertions(+), 37 deletions(-) diff --git a/internal/aws/aws.go b/internal/aws/aws.go index 5322594ad..7ef968d93 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -681,58 +681,62 @@ func ignoreRuleError(err error) error { } // downloadS3ObjectsInParallel fetches the objects at the given indexes and -// writes each content digest into files at the same index. A slot channel -// bounds the number of downloads and a weighted semaphore bounds their listed -// bytes; the first error cancels the shared context so in-flight transfers -// stop and no further one starts. +// writes each content digest into files at the same index. A fixed pool of +// workers bounds the number of downloads, so memory does not grow with the +// bucket, and a weighted semaphore bounds their listed bytes. The first error +// cancels the shared context: in-flight transfers stop, the producer stops +// feeding, and the workers drain out. func downloadS3ObjectsInParallel(downloader S3DownloadAPI, tempDir, bucket string, objects []s3Object, indexes []int, files []digest.VirtualFile, limits DownloadLimits, logger *logger.Logger) error { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - slots := make(chan struct{}, max(limits.Concurrency, 1)) budget := semaphore.NewWeighted(max(limits.BytesInFlight, 1)) firstErr := make(chan error, 1) - var wg sync.WaitGroup + fail := func(err error) { + select { + case firstErr <- err: + default: // an earlier failure is already recorded + } + cancel() + } - for _, i := range indexes { + work := make(chan int) + var wg sync.WaitGroup + for range max(limits.Concurrency, 1) { wg.Add(1) - go func(i int, object s3Object) { + go func() { defer wg.Done() - - select { - case slots <- struct{}{}: - case <-ctx.Done(): - return - } - defer func() { <-slots }() - // A slot and a cancellation can be ready together; never start a - // download once another has failed. - if ctx.Err() != nil { - return - } - - // An object larger than the budget takes all of it and so runs alone. - weight := max(min(object.size, limits.BytesInFlight), 1) - if err := budget.Acquire(ctx, weight); err != nil { - return // cancelled while waiting - } - defer budget.Release(weight) - - sha256, err := downloadAndHashS3Object(ctx, downloader, tempDir, bucket, object.key, nil, logger) - if err != nil { - select { - case firstErr <- err: - default: // an earlier failure is already recorded + for i := range work { + object := objects[i] + // An object larger than the budget takes all of it and so runs alone. + weight := max(min(object.size, limits.BytesInFlight), 1) + if err := budget.Acquire(ctx, weight); err != nil { + return // cancelled while waiting } - cancel() - return + sha256, err := downloadAndHashS3Object(ctx, downloader, tempDir, bucket, object.key, nil, logger) + budget.Release(weight) + if err != nil { + fail(err) + return + } + files[i].Sha256 = sha256 } - files[i].Sha256 = sha256 - }(i, objects[i]) + }() } + // Feed in listing order; stop as soon as a worker has failed. +feed: + for _, i := range indexes { + select { + case work <- i: + case <-ctx.Done(): + break feed + } + } + close(work) wg.Wait() + select { case err := <-firstErr: return err diff --git a/internal/aws/s3_parallel_test.go b/internal/aws/s3_parallel_test.go index 13b7f8321..80e01102f 100644 --- a/internal/aws/s3_parallel_test.go +++ b/internal/aws/s3_parallel_test.go @@ -6,6 +6,7 @@ import ( "fmt" "math/rand" "path/filepath" + "runtime" "sync" "testing" "time" @@ -244,6 +245,29 @@ func (suite *S3ParallelTestSuite) TestLimitsReachTheDownloader() { } } +// The fan-out must not park one goroutine per object: on a large bucket that +// is hundreds of megabytes of stacks doing nothing. Goroutines in flight stay +// within the concurrency bound plus a small fixed overhead, whatever the size +// of the bucket. +func (suite *S3ParallelTestSuite) TestGoroutinesDoNotScaleWithTheBucket() { + const objects, concurrency = 2000, 4 + client, listing := bucketOf(objects, 8, 0) + before := runtime.NumGoroutine() + var peak int + client.hook = func(_ context.Context, _ string) error { + suite.mu().Lock() + peak = max(peak, runtime.NumGoroutine()) + suite.mu().Unlock() + return nil + } + + _, _, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, DownloadLimits{Concurrency: concurrency, BytesInFlight: 1 << 30}, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.Len(suite.T(), client.calls, objects) + require.LessOrEqual(suite.T(), peak, before+concurrency+8, + "goroutines during the run must be bounded by the concurrency, not the object count") +} + func (suite *S3ParallelTestSuite) TestDefaultLimitsAreSane() { require.GreaterOrEqual(suite.T(), DefaultDownloadLimits.Concurrency, 2) require.GreaterOrEqual(suite.T(), DefaultDownloadLimits.BytesInFlight, int64(64<<20)) From 25037788596ceb262b07710584a040582602a152 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Tue, 15 Sep 2026 22:49:49 +0100 Subject: [PATCH 04/12] docs(adr): accept the record now that both deliveries have landed Parallel downloads and their flags are in, so the sentences describing them as pending state what shipped, and the status moves to Accepted. --- docs/adr/20260911-s3-fingerprint-from-virtual-tree.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md index f5ed44e3e..4ca2ae4e1 100644 --- a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md +++ b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md @@ -1,7 +1,7 @@ --- title: "20260911 - Fingerprint S3 buckets from a virtual tree; object keys never become local paths" description: "Download each object to an anonymous temp file, hash it, delete it, and compute the directory fingerprint from (key, sha256) pairs so that no S3 key is ever used as a filename" -status: "Proposed" +status: "Accepted" date: "2026-09-11" --- @@ -37,7 +37,7 @@ The fingerprint format itself is fixed. An S3 snapshot must match the fingerprin 4. **A root `.kosli_ignore` is honoured virtually.** Its rules are parsed by `digest.ParseIgnoreRules`, the same reading `DirSha256` gives the file, and resolved by `digest/virtualglob.go`, which reproduces `filepathx.Glob`, `filepath.Glob` and `filepath.Walk` step for step over the virtual tree rather than reimplementing what the globs appear to mean. That is what keeps their quirks identical: a literal `**/x` finds a root `x` spelled with a double slash, which the walk's cleaned paths never equal, so a root file `x` survives while a root directory `x` keeps its name and loses its contents; `**/*.log` is rebuilt cleaned and matches outright; and excluding `logs/*` leaves an empty directory whose name is still hashed. Exclusion therefore runs inside the tree walk, not by filtering the file list. The ignore file can never exclude itself, as in `DirSha256`. `digest.FilesNeedingContent` shares that walk so excluded objects are not downloaded at all, and `VirtualDirSha256` refuses a tree that needs a digest it was not given, so a skipped download can never leak into a fingerprint. Equivalence with `DirSha256` on a materialised tree is asserted for every rule shape in `TestVirtualIgnoreTestSuite`. -5. **Downloads may run in parallel**, behind a fixed worker pool and a bytes-in-flight budget derived from listing sizes, with results written by index for determinism and a cancellable context so the first transport error stops in-flight multipart downloads. This is a performance property, not a safety one, and is delivered separately from the change this record describes; see #1167. Until it lands, downloads are sequential, exactly as before, and peak temp disk is one object. +5. **Downloads may run in parallel**, behind a fixed worker pool and a bytes-in-flight budget derived from listing sizes, with results written by index for determinism and a cancellable context so the first transport error stops in-flight multipart downloads. This is a performance property, not a safety one, and was delivered separately from the change this record describes; see #1167. The defaults are eight objects in flight within 512 MB of listed bytes, tunable with `--download-concurrency` and `--download-budget`, so peak temp disk is the budget rather than one object. 6. **The switch is pinned, not argued.** `TestPinnedFingerprints` in `internal/aws` holds fingerprints of representative fake buckets recorded against the key-layout implementation before it was replaced: unusual key shapes, a `.` sorting before a `/`, nested prefixes with folder markers, and a single object with its basename as artifact name. It was green before the switch and must stay green after it, alongside `TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeys` from #1155. @@ -64,4 +64,4 @@ The attest side is unchanged: `kosli attest artifact --artifact-type dir` still - `...`, `.. `, `CON`, colon and backslash keys snapshot again. `..` segments and colliding keys remain errors and now name every key involved. - Excluded and ignored objects are not downloaded. Each object's temp file is removed once hashed, so peak temp disk falls from the whole bucket to one object, and to the configured budget once #1167 lands. The other half of that trade is memory: the old loop streamed the listing a page at a time, while a tree cannot be fingerprinted without holding it, so peak memory rises from one listing page to every object that survives the filters, at a few hundred bytes per object on top of its key across the listing, the key-to-path maps, the manifest and the tree. A million objects is a few hundred megabytes. #1167 sizes its budget against this figure. - #1069 rebases onto the shared layer: metadata mode becomes a sha256 source plugged into the same list, normalise, exclude, tree pipeline, its key rule disappears in favour of rule 2, and its rejection of buckets with a root `.kosli_ignore` becomes a download of that one object. -- Delivery is two pull requests. The first carries this decision with sequential downloads, so the security-relevant review is not mixed with performance work. The second, #1167, adds parallel downloads, the byte budget and the flags that tune them. +- Delivery was two pull requests. The first, #1180, carried this decision with sequential downloads, so the security-relevant review was not mixed with performance work. The second, #1167, added parallel downloads, the byte budget and the flags that tune them. From b2bc926d6cceac6a77635ff84b5c0a6128cc12e1 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Tue, 15 Sep 2026 22:49:51 +0100 Subject: [PATCH 05/12] refactor(snapshot s3): match the SDK's default part concurrency per object The override to three parts kept the connection product modest, but the SDK's five is well within what S3 serves, and object concurrency is the knob operators tune. The value stays pinned here so the product of objects and parts is visible in one place and does not move with an SDK upgrade. --- internal/aws/aws.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/aws/aws.go b/internal/aws/aws.go index 7ef968d93..724814899 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -171,11 +171,11 @@ func defaultNewS3Client(creds *AWSStaticCreds) (S3API, error) { return nil, err } // Objects download in parallel (see DownloadLimits), and the transfer manager - // fetches each object's parts in parallel on top of that. Its default of five - // parts per object times the object concurrency would open more connections - // than helps; three keeps the product modest while still splitting large objects. + // fetches each object's parts in parallel on top of that. The parts figure is + // the SDK's own default, pinned here so the connection product, objects times + // parts, is visible in one place and does not move with an SDK upgrade. return &s3Client{S3ListAPI: client, S3DownloadAPI: transfermanager.New(client, func(o *transfermanager.Options) { - o.Concurrency = 3 + o.Concurrency = 5 })}, nil } From 225124ea4d7865c402b0430a3ef6c1922b84d6f6 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Tue, 15 Sep 2026 22:49:52 +0100 Subject: [PATCH 06/12] refactor(snapshot s3): keep only the comments that carry a fact the code cannot Section labels that restated test rows, doc lines that restated a signature, and duplicated reasoning are gone; the remaining comments state one invariant or reason each. The test hook's comment said it ran in place of the delegate; it runs before it and can only fail it. --- cmd/kosli/byteSize.go | 10 ++++------ cmd/kosli/byteSize_test.go | 6 +----- cmd/kosli/snapshotS3.go | 6 ++---- internal/aws/aws.go | 25 +++++++++---------------- internal/aws/s3_fingerprint_test.go | 6 ++---- internal/aws/s3_parallel_test.go | 27 ++++++++------------------- 6 files changed, 26 insertions(+), 54 deletions(-) diff --git a/cmd/kosli/byteSize.go b/cmd/kosli/byteSize.go index 6dc903440..3d2b35b4c 100644 --- a/cmd/kosli/byteSize.go +++ b/cmd/kosli/byteSize.go @@ -9,9 +9,8 @@ import ( "unicode" ) -// byteSizeUnits maps a lower-cased unit suffix to its size in bytes. Sizes are -// binary, as Lambda's /tmp and most disk figures are. The trailing "b" or "ib" -// is stripped before lookup, so "M", "MB" and "MiB" all land on the same entry. +// byteSizeUnits maps a lower-cased unit, with any trailing "b" or "ib" already +// stripped, to bytes. Units are binary, as disk figures are. var byteSizeUnits = map[string]int64{ "": 1 << 20, // a bare number is megabytes "b": 1, @@ -21,9 +20,8 @@ var byteSizeUnits = map[string]int64{ "t": 1 << 40, } -// parseByteSize reads a size such as "512", "512M", "8GB" or "1.5G". A bare -// number is megabytes; a unit suffix, case-insensitive and with an optional B, -// selects kilobytes, megabytes, gigabytes or terabytes; "B" alone means bytes. +// parseByteSize turns "512", "512M", "8GB" or "1.5G" into bytes: a bare number +// is megabytes, a K, M, G or T suffix takes an optional B, and "B" alone is bytes. func parseByteSize(s string) (int64, error) { s = strings.TrimSpace(s) if s == "" { diff --git a/cmd/kosli/byteSize_test.go b/cmd/kosli/byteSize_test.go index 75b69b70e..0752d739f 100644 --- a/cmd/kosli/byteSize_test.go +++ b/cmd/kosli/byteSize_test.go @@ -14,11 +14,9 @@ func TestParseByteSize(t *testing.T) { want int64 wantErr string }{ - // A bare number is megabytes, matching how Lambda's ephemeral storage is expressed. {input: "512", want: 512 * mib}, {input: "1", want: mib}, {input: " 64 ", want: 64 * mib}, - // A suffix picks the unit; the trailing B is optional and case does not matter. {input: "512M", want: 512 * mib}, {input: "512MB", want: 512 * mib}, {input: "512mb", want: 512 * mib}, @@ -32,7 +30,6 @@ func TestParseByteSize(t *testing.T) { {input: "1000B", want: 1000}, {input: "1.5G", want: 3 << 29}, {input: "0.5M", want: 512 << 10}, - // Rejected: nothing to download into, or not a size at all. {input: "", wantErr: "empty"}, {input: "0", wantErr: "must be at least 1 byte"}, {input: "0B", wantErr: "must be at least 1 byte"}, @@ -60,8 +57,7 @@ func TestParseByteSize(t *testing.T) { } } -// The flag's default is spelled as a string, so pin it to the value the aws -// package uses when no flag is given. +// The flag default is a string, so it can drift from the value it spells. func TestDefaultDownloadBudgetMatchesTheAwsDefault(t *testing.T) { got, err := parseByteSize(defaultDownloadBudget) require.NoError(t, err) diff --git a/cmd/kosli/snapshotS3.go b/cmd/kosli/snapshotS3.go index fcd0e448e..d663ed483 100644 --- a/cmd/kosli/snapshotS3.go +++ b/cmd/kosli/snapshotS3.go @@ -171,12 +171,10 @@ func (o *snapshotS3Options) run(args []string) error { return err } -// defaultDownloadBudget spells aws.DefaultDownloadLimits.BytesInFlight the way -// the flag reads it. +// defaultDownloadBudget is aws.DefaultDownloadLimits.BytesInFlight as the flag +// spells it; a test keeps the two equal. const defaultDownloadBudget = "512M" -// resolveDownloadLimits validates the download flags and turns them into the -// limits the aws package takes. func (o *snapshotS3Options) resolveDownloadLimits() error { if o.downloadConcurrency < 1 { return fmt.Errorf("--download-concurrency must be at least 1, got %d", o.downloadConcurrency) diff --git a/internal/aws/aws.go b/internal/aws/aws.go index 724814899..ffb57d160 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -170,10 +170,8 @@ func defaultNewS3Client(creds *AWSStaticCreds) (S3API, error) { if err != nil { return nil, err } - // Objects download in parallel (see DownloadLimits), and the transfer manager - // fetches each object's parts in parallel on top of that. The parts figure is - // the SDK's own default, pinned here so the connection product, objects times - // parts, is visible in one place and does not move with an SDK upgrade. + // Five parts per object is the SDK's default, pinned so the connection count, + // objects in flight times parts, cannot move with an SDK upgrade. return &s3Client{S3ListAPI: client, S3DownloadAPI: transfermanager.New(client, func(o *transfermanager.Options) { o.Concurrency = 5 })}, nil @@ -515,9 +513,8 @@ type DownloadLimits struct { BytesInFlight int64 } -// DefaultDownloadLimits keeps peak temp disk around half a gigabyte, which fits -// the default Lambda /tmp, and the connection count modest together with the -// transfer manager's per-object part concurrency. +// DefaultDownloadLimits keeps peak temp disk near half a gigabyte, which fits +// Lambda's default /tmp. var DefaultDownloadLimits = DownloadLimits{Concurrency: 8, BytesInFlight: 512 << 20} // listMatchingS3Objects lists the bucket, dropping folder markers and keys the @@ -599,8 +596,7 @@ func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3O } }() - // The manifest starts as paths only; digests are filled in below by index, - // so it stays in listing order however the downloads interleave. + // The manifest starts as paths only; digests are filled in by index below. files := make([]digest.VirtualFile, len(objects)) for i, object := range objects { files[i].Path = paths[object.key] @@ -680,12 +676,10 @@ func ignoreRuleError(err error) error { return err } -// downloadS3ObjectsInParallel fetches the objects at the given indexes and -// writes each content digest into files at the same index. A fixed pool of -// workers bounds the number of downloads, so memory does not grow with the -// bucket, and a weighted semaphore bounds their listed bytes. The first error -// cancels the shared context: in-flight transfers stop, the producer stops -// feeding, and the workers drain out. +// downloadS3ObjectsInParallel fetches the objects at indexes and writes each +// digest into files at the same index. A fixed worker pool bounds downloads and +// goroutines alike, a weighted semaphore bounds their listed bytes, and the +// first error cancels the context so nothing further starts. func downloadS3ObjectsInParallel(downloader S3DownloadAPI, tempDir, bucket string, objects []s3Object, indexes []int, files []digest.VirtualFile, limits DownloadLimits, logger *logger.Logger) error { ctx, cancel := context.WithCancel(context.Background()) @@ -725,7 +719,6 @@ func downloadS3ObjectsInParallel(downloader S3DownloadAPI, tempDir, bucket strin }() } - // Feed in listing order; stop as soon as a worker has failed. feed: for _, i := range indexes { select { diff --git a/internal/aws/s3_fingerprint_test.go b/internal/aws/s3_fingerprint_test.go index 3ae54c41c..e667772e8 100644 --- a/internal/aws/s3_fingerprint_test.go +++ b/internal/aws/s3_fingerprint_test.go @@ -197,10 +197,8 @@ func (suite *S3FingerprintTestSuite) TestDownloadsExactlyTheContributingObjects( require.Equal(suite.T(), []string{".kosli_ignore", "app.js", "lib/util.js"}, client.downloadedKeys()) } -// Objects are downloaded to files whose names owe nothing to the key, every -// file is removed once hashed, and the download directory is gone at the end. -// Downloads overlap, so how many files exist at once is the byte budget's -// concern (see S3ParallelTestSuite), not this test's. +// How many temp files exist at once is the byte budget's concern, tested in +// S3ParallelTestSuite; this test checks only their names and their removal. func (suite *S3FingerprintTestSuite) TestObjectsNeverLandUnderTheirKeyAndDoNotLinger() { keys := []string{"alpha.bin", "beta/gamma.bin", "delta/epsilon/zeta.bin"} objects := map[string][]byte{} diff --git a/internal/aws/s3_parallel_test.go b/internal/aws/s3_parallel_test.go index 80e01102f..ea35fcb0e 100644 --- a/internal/aws/s3_parallel_test.go +++ b/internal/aws/s3_parallel_test.go @@ -23,14 +23,13 @@ type S3ParallelTestSuite struct { suite.Suite } -// trackingDownloader records how many downloads, and how many listed bytes, -// are in flight at once, and how often each key is fetched. A delay keeps -// downloads overlapping so the bounds are actually exercised. +// trackingDownloader records peak downloads and listed bytes in flight, and +// calls per key. delay keeps downloads overlapping so the bounds are exercised. type trackingDownloader struct { S3API sizes map[string]int64 delay time.Duration - // hook, when set, runs in place of the delegate for that key. + // hook runs before the delegate and can fail the download in its place. hook func(ctx context.Context, key string) error mu sync.Mutex @@ -75,8 +74,7 @@ func (d *trackingDownloader) currentInFlight() int { return d.inFlight } -// bucketOf builds a fake bucket of n objects of the given size and returns the -// tracking downloader plus the listing fingerprintS3Objects takes. +// bucketOf fakes n objects of size bytes each, spread over four prefixes. func bucketOf(n int, size int, delay time.Duration) (*trackingDownloader, []s3Object) { objects := map[string][]byte{} listing := make([]s3Object, 0, n) @@ -126,7 +124,6 @@ func (suite *S3ParallelTestSuite) TestRespectsTheByteBudget() { require.GreaterOrEqual(suite.T(), client.maxInFlight, 2, "downloads must actually overlap for the budget to be tested") } -// An object larger than the whole budget must still download, and runs alone. func (suite *S3ParallelTestSuite) TestAnObjectLargerThanTheBudgetRunsAlone() { client, listing := bucketOf(12, 100, 5*time.Millisecond) big := "big/huge.bin" @@ -156,8 +153,6 @@ var suiteMu sync.Mutex func (suite *S3ParallelTestSuite) mu() *sync.Mutex { return &suiteMu } -// Random per-object delays reorder completion; the fingerprint must equal what -// DirSha256 gives the same tree on disk, every time. func (suite *S3ParallelTestSuite) TestFingerprintIsIndependentOfCompletionOrder() { tree := map[string]string{} for i := 0; i < 30; i++ { @@ -193,10 +188,8 @@ func (suite *S3ParallelTestSuite) TestFingerprintIsIndependentOfCompletionOrder( } } -// The first transport error cancels the shared context: an in-flight download -// sees it and returns, and no further download starts. Goroutines race for the -// slots, so the hook decides by arrival rather than by key: the first download -// to arrive fails, every other one blocks until it is cancelled. +// Workers race for the slots, so the hook fails the first arrival rather than a +// fixed key, and blocks every other one until it is cancelled. func (suite *S3ParallelTestSuite) TestATransportErrorStopsRemainingWork() { client, listing := bucketOf(10, 8, 0) errBoom := errors.New("boom") @@ -229,8 +222,6 @@ func (suite *S3ParallelTestSuite) TestATransportErrorStopsRemainingWork() { require.Len(suite.T(), client.calls, 2, "only the two downloads in flight at the failure may have started: %v", client.calls) } -// The limits handed to getS3DataFromClient are the ones the fan-out obeys: the -// same bucket runs one at a time under a limit of one and overlaps under four. func (suite *S3ParallelTestSuite) TestLimitsReachTheDownloader() { for _, limit := range []int{1, 4} { client, _ := bucketOf(12, 8, 3*time.Millisecond) @@ -245,10 +236,8 @@ func (suite *S3ParallelTestSuite) TestLimitsReachTheDownloader() { } } -// The fan-out must not park one goroutine per object: on a large bucket that -// is hundreds of megabytes of stacks doing nothing. Goroutines in flight stay -// within the concurrency bound plus a small fixed overhead, whatever the size -// of the bucket. +// One goroutine per object would be hundreds of megabytes of idle stacks on a +// large bucket. func (suite *S3ParallelTestSuite) TestGoroutinesDoNotScaleWithTheBucket() { const objects, concurrency = 2000, 4 client, listing := bucketOf(objects, 8, 0) From 390e787eaddda2c57473f6c73a4f2468d07ea038 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Tue, 15 Sep 2026 22:49:54 +0100 Subject: [PATCH 07/12] fix(snapshot s3): start no more download workers than objects, and say what concurrency costs in memory The pool started the full concurrency even for three objects or none. Each object in flight can also buffer up to five 8 MiB parts, which the byte budget does not count, so the type docs and flag help now say so. --- cmd/kosli/root.go | 2 +- internal/aws/aws.go | 8 +++++--- internal/aws/s3_parallel_test.go | 18 ++++++++++++++++++ 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/cmd/kosli/root.go b/cmd/kosli/root.go index 24909ef26..151fb8702 100644 --- a/cmd/kosli/root.go +++ b/cmd/kosli/root.go @@ -259,7 +259,7 @@ Paths the list already matches stay excluded whatever is later added there, so k awsSecretKeyFlag = "The AWS secret access key." awsRegionFlag = "The AWS region." bucketNameFlag = "The name of the S3 bucket." - downloadConcurrencyFlag = "[optional] The number of S3 objects to download at the same time when fingerprinting the bucket." + downloadConcurrencyFlag = "[optional] The number of S3 objects to download at the same time when fingerprinting the bucket. Each object in flight may hold up to 40 MB of download buffers in memory, on top of the disk the --download-budget allows." downloadBudgetFlag = "[optional] The maximum total size of the S3 objects downloading at the same time, which caps the temporary disk the snapshot uses. A bare number is megabytes; add K, M, G or T (optionally followed by B) to choose the unit, e.g. 512M or 8G. An object larger than the budget still downloads, on its own. Set TMPDIR to choose where objects are downloaded to." bucketPathsFlag = "[optional] The comma separated list of file and/or directory paths in the S3 bucket to include when fingerprinting. Paths match by literal prefix. Cannot be used together with --exclude or --exclude-regex." bucketPathsRegexFlag = "[optional] The comma separated list of Go regular expressions matched against object keys in the S3 bucket to include when fingerprinting. Cannot be used together with --exclude or --exclude-regex." diff --git a/internal/aws/aws.go b/internal/aws/aws.go index ffb57d160..c2f9d7164 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -505,7 +505,9 @@ type s3Object struct { // DownloadLimits bounds the object downloads in flight at once when // fingerprinting a bucket. type DownloadLimits struct { - // Concurrency is the number of objects downloading at the same time. + // Concurrency is the number of objects downloading at the same time. Each + // one may buffer up to five 8 MiB parts in memory while it writes, so memory + // rises with this figure independently of BytesInFlight. Concurrency int // BytesInFlight caps the sum of the listed sizes of the objects downloading // at the same time, and so the temp disk they occupy. An object larger than @@ -514,7 +516,7 @@ type DownloadLimits struct { } // DefaultDownloadLimits keeps peak temp disk near half a gigabyte, which fits -// Lambda's default /tmp. +// Lambda's default /tmp, and part buffers near 320 MiB of memory. var DefaultDownloadLimits = DownloadLimits{Concurrency: 8, BytesInFlight: 512 << 20} // listMatchingS3Objects lists the bucket, dropping folder markers and keys the @@ -697,7 +699,7 @@ func downloadS3ObjectsInParallel(downloader S3DownloadAPI, tempDir, bucket strin work := make(chan int) var wg sync.WaitGroup - for range max(limits.Concurrency, 1) { + for range min(max(limits.Concurrency, 1), len(indexes)) { wg.Add(1) go func() { defer wg.Done() diff --git a/internal/aws/s3_parallel_test.go b/internal/aws/s3_parallel_test.go index ea35fcb0e..76d4ebe98 100644 --- a/internal/aws/s3_parallel_test.go +++ b/internal/aws/s3_parallel_test.go @@ -257,6 +257,24 @@ func (suite *S3ParallelTestSuite) TestGoroutinesDoNotScaleWithTheBucket() { "goroutines during the run must be bounded by the concurrency, not the object count") } +// A concurrency far above the object count must not start idle workers. +func (suite *S3ParallelTestSuite) TestWorkersAreClampedToTheWork() { + client, listing := bucketOf(3, 8, 0) + before := runtime.NumGoroutine() + var peak int + client.hook = func(_ context.Context, _ string) error { + suite.mu().Lock() + peak = max(peak, runtime.NumGoroutine()) + suite.mu().Unlock() + return nil + } + + _, _, err := fingerprintS3Objects(client, fakeS3TestBucketName, listing, DownloadLimits{Concurrency: 50000, BytesInFlight: 1 << 30}, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.Len(suite.T(), client.calls, 3) + require.LessOrEqual(suite.T(), peak, before+3+8, "workers must be bounded by the objects to download") +} + func (suite *S3ParallelTestSuite) TestDefaultLimitsAreSane() { require.GreaterOrEqual(suite.T(), DefaultDownloadLimits.Concurrency, 2) require.GreaterOrEqual(suite.T(), DefaultDownloadLimits.BytesInFlight, int64(64<<20)) From 03c1247542b177fa1430fb22db6daa9e6bb68eac Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Tue, 15 Sep 2026 22:49:55 +0100 Subject: [PATCH 08/12] fix(snapshot s3): reject "ib" and "bb" as byte-size units Stripping the optional suffix let "5ib" fall through to the bare-number entry as megabytes and "5bb" to bytes. Only one unit letter may precede the suffix. The budget parse error is wrapped rather than flattened. --- cmd/kosli/byteSize.go | 7 ++++++- cmd/kosli/byteSize_test.go | 3 +++ cmd/kosli/snapshotS3.go | 2 +- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/cmd/kosli/byteSize.go b/cmd/kosli/byteSize.go index 3d2b35b4c..85bcbcbf0 100644 --- a/cmd/kosli/byteSize.go +++ b/cmd/kosli/byteSize.go @@ -42,8 +42,13 @@ func parseByteSize(s string) (int64, error) { } key := strings.ToLower(unit) - if key != "b" { + if key != "" && key != "b" { + // Only a single unit letter may precede the optional "b" or "ib", so + // "ib" alone and "bb" are unknown rather than a guess at megabytes or bytes. key = strings.TrimSuffix(strings.TrimSuffix(key, "ib"), "b") + if len(key) != 1 || key == "b" { + return 0, fmt.Errorf("unknown unit %q in size %q: use K, M, G or T, optionally followed by B", unit, s) + } } multiplier, ok := byteSizeUnits[key] if !ok { diff --git a/cmd/kosli/byteSize_test.go b/cmd/kosli/byteSize_test.go index 0752d739f..7dd90ce2a 100644 --- a/cmd/kosli/byteSize_test.go +++ b/cmd/kosli/byteSize_test.go @@ -38,6 +38,9 @@ func TestParseByteSize(t *testing.T) { {input: "abc", wantErr: "not a size"}, {input: "M", wantErr: "not a size"}, {input: "512X", wantErr: `unknown unit "X"`}, + {input: "5ib", wantErr: `unknown unit "ib"`}, + {input: "5bb", wantErr: `unknown unit "bb"`}, + {input: "5KiBB", wantErr: `unknown unit "KiBB"`}, {input: "512 megabytes", wantErr: `unknown unit "megabytes"`}, {input: "1e3", wantErr: "not a size"}, {input: "0x10", wantErr: "not a size"}, diff --git a/cmd/kosli/snapshotS3.go b/cmd/kosli/snapshotS3.go index d663ed483..619ce7ac5 100644 --- a/cmd/kosli/snapshotS3.go +++ b/cmd/kosli/snapshotS3.go @@ -181,7 +181,7 @@ func (o *snapshotS3Options) resolveDownloadLimits() error { } budget, err := parseByteSize(o.downloadBudget) if err != nil { - return fmt.Errorf("invalid --download-budget: %v", err) + return fmt.Errorf("invalid --download-budget: %w", err) } o.downloadLimits = aws.DownloadLimits{Concurrency: o.downloadConcurrency, BytesInFlight: budget} return nil From bb7a69d6d8044e42cd060d6d311f831553e45500 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Tue, 15 Sep 2026 22:49:57 +0100 Subject: [PATCH 09/12] fix: align help text and testing --- cmd/kosli/root.go | 2 +- internal/aws/s3_parallel_test.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cmd/kosli/root.go b/cmd/kosli/root.go index 151fb8702..4e7a6ab6a 100644 --- a/cmd/kosli/root.go +++ b/cmd/kosli/root.go @@ -260,7 +260,7 @@ Paths the list already matches stay excluded whatever is later added there, so k awsRegionFlag = "The AWS region." bucketNameFlag = "The name of the S3 bucket." downloadConcurrencyFlag = "[optional] The number of S3 objects to download at the same time when fingerprinting the bucket. Each object in flight may hold up to 40 MB of download buffers in memory, on top of the disk the --download-budget allows." - downloadBudgetFlag = "[optional] The maximum total size of the S3 objects downloading at the same time, which caps the temporary disk the snapshot uses. A bare number is megabytes; add K, M, G or T (optionally followed by B) to choose the unit, e.g. 512M or 8G. An object larger than the budget still downloads, on its own. Set TMPDIR to choose where objects are downloaded to." + downloadBudgetFlag = "[optional] The maximum total size of the S3 objects downloading at the same time, which caps the temporary disk the snapshot uses. A bare number is megabytes; add K, M, G or T (optionally followed by B) to choose the unit, e.g. 512M or 8G. An object larger than the budget still downloads, on its own. Objects are downloaded to the OS temporary directory." bucketPathsFlag = "[optional] The comma separated list of file and/or directory paths in the S3 bucket to include when fingerprinting. Paths match by literal prefix. Cannot be used together with --exclude or --exclude-regex." bucketPathsRegexFlag = "[optional] The comma separated list of Go regular expressions matched against object keys in the S3 bucket to include when fingerprinting. Cannot be used together with --exclude or --exclude-regex." excludeBucketPathsFlag = "[optional] The comma separated list of file and/or directory paths in the S3 bucket to exclude when fingerprinting. Paths match by literal prefix. Cannot be used together with --include or --include-regex." diff --git a/internal/aws/s3_parallel_test.go b/internal/aws/s3_parallel_test.go index 76d4ebe98..fcd75b6c3 100644 --- a/internal/aws/s3_parallel_test.go +++ b/internal/aws/s3_parallel_test.go @@ -132,13 +132,14 @@ func (suite *S3ParallelTestSuite) TestAnObjectLargerThanTheBudgetRunsAlone() { client.sizes[big] = 1000 listing = append([]s3Object{{key: big, size: 1000}}, listing...) - var aloneChecks int + var aloneChecks, inFlightDuringBig int client.hook = func(_ context.Context, key string) error { if key == big { + seen := client.currentInFlight() suite.mu().Lock() aloneChecks++ + inFlightDuringBig = max(inFlightDuringBig, seen) suite.mu().Unlock() - require.Equal(suite.T(), 1, client.currentInFlight(), "the oversized object must be the only download in flight") } return nil } @@ -147,11 +148,10 @@ func (suite *S3ParallelTestSuite) TestAnObjectLargerThanTheBudgetRunsAlone() { require.NoError(suite.T(), err) require.Equal(suite.T(), 1, aloneChecks) require.Equal(suite.T(), 1, client.calls[big]) + require.Equal(suite.T(), 1, inFlightDuringBig, "the oversized object must be the only download in flight") } -var suiteMu sync.Mutex - -func (suite *S3ParallelTestSuite) mu() *sync.Mutex { return &suiteMu } +func (suite *S3ParallelTestSuite) mu() *sync.Mutex { return &suite.lock } func (suite *S3ParallelTestSuite) TestFingerprintIsIndependentOfCompletionOrder() { tree := map[string]string{} From 105bdd7872fbb73fb3ffe5eaa475d8ec43897d6e Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Tue, 15 Sep 2026 22:49:58 +0100 Subject: [PATCH 10/12] fix: missing testing Mutex --- internal/aws/s3_parallel_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/aws/s3_parallel_test.go b/internal/aws/s3_parallel_test.go index fcd75b6c3..dfb630141 100644 --- a/internal/aws/s3_parallel_test.go +++ b/internal/aws/s3_parallel_test.go @@ -21,6 +21,7 @@ import ( type S3ParallelTestSuite struct { suite.Suite + lock sync.Mutex } // trackingDownloader records peak downloads and listed bytes in flight, and From 6960c083820797ddf86e05ceacac600ad1a4de7a Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Tue, 15 Sep 2026 22:50:00 +0100 Subject: [PATCH 11/12] chore: Update docs/adr/20260911-s3-fingerprint-from-virtual-tree.md --- docs/adr/20260911-s3-fingerprint-from-virtual-tree.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md index 4ca2ae4e1..5487d1b0a 100644 --- a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md +++ b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md @@ -62,6 +62,6 @@ The attest side is unchanged: `kosli attest artifact --artifact-type dir` still - `localPathForS3Key`, `filepath.IsLocal`, the `O_EXCL` open, the `ENOTDIR` branch, `containsSingleFile` and the platform-conditional tests from #1155 are deleted. No code in the S3 path branches on the operating system any more. The codebase does not get smaller, though: the virtual tree, the key rule with its collision reporting, and above all the faithful simulation of `filepathx.Glob` add several hundred lines, most of them owed to reproducing `.kosli_ignore` semantics exactly. That is the price of the compatibility contract, paid once and shared with #1069. - The bucket's ignore file is recognised by the exact key `.kosli_ignore`. On disk, `ignoreFilePathInTree` also accepted a case-folded spelling such as `.KOSLI_IGNORE` where the operator's filesystem folded case, so on macOS or Windows such a bucket had its rules applied; now it does not, its exclusions stop applying and its fingerprint moves to the Linux value. Same correction as the key case above, and release-noted with it. - `...`, `.. `, `CON`, colon and backslash keys snapshot again. `..` segments and colliding keys remain errors and now name every key involved. -- Excluded and ignored objects are not downloaded. Each object's temp file is removed once hashed, so peak temp disk falls from the whole bucket to one object, and to the configured budget once #1167 lands. The other half of that trade is memory: the old loop streamed the listing a page at a time, while a tree cannot be fingerprinted without holding it, so peak memory rises from one listing page to every object that survives the filters, at a few hundred bytes per object on top of its key across the listing, the key-to-path maps, the manifest and the tree. A million objects is a few hundred megabytes. #1167 sizes its budget against this figure. +- Excluded and ignored objects are not downloaded. Each object's temp file is removed once hashed, so peak temp disk falls from the whole bucket to one object, and to the configured budget now that #1167 has landed. The other half of that trade is memory: the old loop streamed the listing a page at a time, while a tree cannot be fingerprinted without holding it, so peak memory rises from one listing page to every object that survives the filters, at a few hundred bytes per object on top of its key across the listing, the key-to-path maps, the manifest and the tree. A million objects is a few hundred megabytes. #1167 sized its budget against this figure. - #1069 rebases onto the shared layer: metadata mode becomes a sha256 source plugged into the same list, normalise, exclude, tree pipeline, its key rule disappears in favour of rule 2, and its rejection of buckets with a root `.kosli_ignore` becomes a download of that one object. - Delivery was two pull requests. The first, #1180, carried this decision with sequential downloads, so the security-relevant review was not mixed with performance work. The second, #1167, added parallel downloads, the byte budget and the flags that tune them. From 08113d11b15634fc2f07db27ece0bef62cbcda6b Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Tue, 15 Sep 2026 22:50:05 +0100 Subject: [PATCH 12/12] fix(snapshot s3): restore the suite lock field and stop asserting from a download goroutine S3ParallelTestSuite.mu() referenced suite.lock without the field existing, failing vet. TestObjectsNeverLandUnderTheirKeyAndDoNotLinger's onDownload hook also called require.* from a worker goroutine, the same shape fixed elsewhere in this package: a failure there calls Goexit on that goroutine instead of failing the test, so the download silently never completes and a later, unrelated 'no content digest' error hides the real one. The hook now only records facts under a mutex; the assertions run after the call returns, on the test goroutine. --- internal/aws/s3_fingerprint_test.go | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/internal/aws/s3_fingerprint_test.go b/internal/aws/s3_fingerprint_test.go index e667772e8..d05066cf8 100644 --- a/internal/aws/s3_fingerprint_test.go +++ b/internal/aws/s3_fingerprint_test.go @@ -5,6 +5,7 @@ import ( "os" "path/filepath" "sort" + "strings" "sync" "testing" "time" @@ -206,14 +207,27 @@ func (suite *S3FingerprintTestSuite) TestObjectsNeverLandUnderTheirKeyAndDoNotLi objects[key] = []byte(key) } client := &recordingDownloader{S3API: &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: objects}} + // Downloads run concurrently, so onDownload fires from worker goroutines; + // require.* must run only on the test goroutine, so record and assert after. + var mu sync.Mutex + var nilFile bool + var keyLikeNames []string client.onDownload = func(key string, file *os.File) { - require.NotNil(suite.T(), file, "the transfer manager must be handed a real file") - require.NotContains(suite.T(), filepath.Base(file.Name()), filepath.Base(key), - "the local file name must owe nothing to the key") + mu.Lock() + defer mu.Unlock() + if file == nil { + nilFile = true + return + } + if strings.Contains(filepath.Base(file.Name()), filepath.Base(key)) { + keyLikeNames = append(keyLikeNames, key) + } } _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, DefaultDownloadLimits, logger.NewStandardLogger()) require.NoError(suite.T(), err) + require.False(suite.T(), nilFile, "the transfer manager must be handed a real file") + require.Empty(suite.T(), keyLikeNames, "the local file name must owe nothing to the key") require.Len(suite.T(), client.files, len(keys)) for _, file := range client.files { _, err := os.Stat(file)