feat(snapshot s3): download objects in parallel within a count and byte budget by mbevc1 · Pull Request #1191 · kosli-dev/cli

mbevc1 · 2026-09-15T15:05:03Z

Second half of the virtual-tree fingerprint (#1180). kosli snapshot s3 now downloads the objects that contribute to the fingerprint in parallel instead of one after another. A fixed worker pool bounds the number of objects in flight and a weighted semaphore bounds their listed bytes, so peak temp disk is the budget rather than one object; the first transport error cancels the rest. Each download still writes its digest into its own manifest slot, so the fingerprint is identical however the downloads interleave, and the pinned and attested-directory tests from #1180 hold unchanged.

Two flags tune the limits, both settable as KOSLI_ environment variables like every other flag:

--download-concurrency, default 8 objects in flight.
--download-budget, default 512 MB of listed bytes in flight. A bare number is megabytes; K, M, G and T suffixes with an optional B are binary units, so 8G and 8GB are the same.

The transfer manager's per-object part concurrency is pinned at the SDK's default of 5, so the worst case is 40 connections. The gain is largest for buckets of many small objects, where per-request latency dominates and up to eight requests overlap; large objects were already fetched in parallel parts and gain little. The listing phase is unchanged.

The ADR moves to Accepted and its two sentences describing this work as pending now state what shipped. Still open from the #1180 review and deliberately not in this PR: a prepared-tree handle so the tree and ignore rules are resolved once for both the download set and the digest.

Closes #1167.

Checklist

Docs are autogenerated from CLI help — make sure the help text reflects your changes
Helm chart (charts/k8s-reporter/) updated, if needed. Note: these changes live in a separate PR
Terraform provider and related changes (terraform-provider-kosli, terraform-aws-evidence-reporter, terraform-aws-kosli-reporter) updated, if needed

claude · 2026-09-15T15:05:36Z

Claude finished @mbevc1's task in 2m 6s —— View job

Follow-up review — commits `105bdd78`, `6960c083`, `08113d11`

(BEFORE=cdc12500 isn't reachable from the current branch tip, so this covers the three newest commits per git log --oneline -n 20, matching the fixes the prior review round called for.)

Prior findings resolved by these commits:

suite.lock undefined / package doesn't compile (internal/aws/s3_parallel_test.go) — Resolved in 105bdd78. lock sync.Mutex is now a field on S3ParallelTestSuite, and mu() (s3_parallel_test.go:155) returns &suite.lock correctly. No leftover package-scoped suiteMu, no duplicate field.
ADR forward-reference tense (docs/adr/20260911-s3-fingerprint-from-virtual-tree.md) — Resolved in 6960c083. "once snapshot s3: parallel downloads with a bytes-in-flight budget and --download-* flags #1167 lands" → "now that snapshot s3: parallel downloads with a bytes-in-flight budget and --download-* flags #1167 has landed", "snapshot s3: parallel downloads with a bytes-in-flight budget and --download-* flags #1167 sizes its budget" → "snapshot s3: parallel downloads with a bytes-in-flight budget and --download-* flags #1167 sized its budget", matching the suggested wording.
require from a download-pool worker goroutine (internal/aws/s3_fingerprint_test.go, TestObjectsNeverLandUnderTheirKeyAndDoNotLinger) — Resolved in 08113d11. The onDownload hook now only records nilFile/keyLikeNames under a mutex; both require.False/require.Empty assertions moved to the test body after getS3DataFromClient returns. Same shape as the earlier fix to TestAnObjectLargerThanTheBudgetRunsAlone.

No new bugs, races, or regressions found in the lines these three commits touch. 0 new inline comments.

Findings from earlier rounds not touched by this diff (worker clamping, memory doc comments, context.TODO(), zero-size listing edge case on BytesInFlight) are unchanged and were already resolved or deliberately deferred in prior rounds.

Note: go build/go vet/gh were not available in this sandbox (network-restricted), so the compile fix was verified by manual inspection of the struct and mu() method rather than by running the toolchain.

claude · 2026-09-15T15:23:13Z

Follow-up review — commits `c00225c`..`b2f25f7`

Checked the 5 prior findings against the two new commits (37b20562, b2f25f7c). All threads were already marked resolved on GitHub; confirming against the diff:

Worker pool not clamped to object count (internal/aws/aws.go) — Resolved in 37b20562. The loop now reads for range min(max(limits.Concurrency, 1), len(indexes)), exactly the suggested fix, with TestWorkersAreClampedToTheWork pinning that concurrency 50000 over 3 objects starts no idle workers.
Memory cost of Concurrency undocumented (internal/aws/aws.go) — Resolved in 37b20562. Doc comments on DownloadLimits.Concurrency, DefaultDownloadLimits (~320 MiB figure), and the --download-concurrency help text in root.go now all state the per-object buffer cost. The optional "add a memory ceiling" suggestion was consciously not taken (concurrency is already the bounding knob).
context.TODO() still used for the single-object/.kosli_ignore downloads (internal/aws/aws.go) — Not addressed, and correctly so: this was explicitly flagged as an acceptable follow-up in the original comment, and no commit touches those call sites. Left for a later PR alongside the prepared-tree handle.
%v should be %w for the --download-budget parse error (cmd/kosli/snapshotS3.go) — Resolved in b2f25f7c.
byteSize.go unit whitelist ("5ib"/"5bb" silently misparsed) (cmd/kosli/byteSize.go) — Resolved in b2f25f7c. The fix differs from the suggested diff but is stricter: it requires the stripped unit to be a single letter other than b, which also catches "KiBB". Test cases for 5ib, 5bb, and 5KiBB were added.

No new bugs, security issues, or regressions found in the lines touched by these two commits.

0 new inline comments.

View job run

🤖 Generated with Claude Code

claude · 2026-09-15T17:08:23Z

Follow-up review — commit `0a614262` (align help text and testing)

Diff scope: bb13f0c0..0a614262, touching only cmd/kosli/root.go and internal/aws/s3_parallel_test.go.

Prior findings touched by this commit:

TMPDIR help text is Unix-only (cmd/kosli/root.go) — Resolved. downloadBudgetFlag now says "Objects are downloaded to the OS temporary directory," dropping the Unix-specific TMPDIR claim. Simpler than the suggested wording (doesn't name TMP/TEMP), but no longer platform-inaccurate.
require from a worker goroutine gives misleading failures (internal/aws/s3_parallel_test.go:145) — Resolved, matching the suggested fix exactly: the hook now only records inFlightDuringBig, and the require.Equal(... "must be the only download in flight") assertion moved to the test body after fingerprintS3Objects returns.
Package-scoped suiteMu nit (internal/aws/s3_parallel_test.go) — Partially addressed, and it broke the build. The package-level suiteMu var was removed and mu() now returns &suite.lock as suggested, but the corresponding lock sync.Mutex field was never added to S3ParallelTestSuite. internal/aws does not compile as a result (suite.lock undefined). Posted an inline comment with the missing field.

New issue found (1 inline comment): the build-breaking suite.lock reference above — this needs a follow-up commit before merge.

Findings not touched by this diff (aws.go, byteSize.go, snapshotS3.go) were already confirmed resolved in the prior follow-up review and are unaffected by this commit.

View job run

🤖 Generated with Claude Code

…te 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.

…lags 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.

… 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.

Parallel downloads and their flags are in, so the sentences describing them as pending state what shipped, and the status moves to Accepted.

…bject 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.

…ode 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.

…y 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.

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.

…m 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.

github-actions Bot added documentation Improvements or additions to documentation go Pull requests that update go code feat labels Sep 15, 2026