feat: concurrent manifest downloads in ListSnapshots #50

clawbot · 2026-03-18T05:52:05+01:00

clawbot commented

2026-03-18 05:52:05 +01:00

Summary

Replace serial getManifestSize() calls in ListSnapshots with bounded concurrent downloads using errgroup. For each remote snapshot not in the local DB, manifest downloads now run in parallel (up to 10 concurrent goroutines) instead of one at a time.

Changes

Use errgroup with SetLimit(10) for bounded concurrency
Collect remote-only snapshot IDs first, pre-add entries with zero size
Download manifests concurrently, patch sizes from results
Remove now-unused getManifestSize helper (logic inlined into goroutines)
Promote golang.org/x/sync from indirect to direct dependency

Testing

make check passes (fmt-check, lint, tests)
docker build . passes

closes #8

## Summary Replace serial `getManifestSize()` calls in `ListSnapshots` with bounded concurrent downloads using `errgroup`. For each remote snapshot not in the local DB, manifest downloads now run in parallel (up to 10 concurrent goroutines) instead of one at a time. ## Changes - Use `errgroup` with `SetLimit(10)` for bounded concurrency - Collect remote-only snapshot IDs first, pre-add entries with zero size - Download manifests concurrently, patch sizes from results - Remove now-unused `getManifestSize` helper (logic inlined into goroutines) - Promote `golang.org/x/sync` from indirect to direct dependency ## Testing - `make check` passes (fmt-check, lint, tests) - `docker build .` passes closes https://git.eeqj.de/sneak/vaultik/issues/8

clawbot added 1 commit 2026-03-18 05:52:05 +01:00

feat: concurrent manifest downloads in ListSnapshots

check / check (pull_request) Successful in 4m7s

Details

588e84da9c

Replace serial getManifestSize() calls with bounded concurrent downloads
using errgroup. For each remote snapshot not in the local DB, manifest
downloads now run in parallel (up to 10 concurrent) instead of one at a
time.

Changes:
- Use errgroup with SetLimit(10) for bounded concurrency
- Collect remote-only snapshot IDs first, pre-add entries with zero size
- Download manifests concurrently, patch sizes from results
- Remove now-unused getManifestSize helper (logic inlined into goroutines)
- Promote golang.org/x/sync from indirect to direct dependency

closes #8

clawbot added the needs-review label 2026-03-18 05:52:44 +01:00

clawbot commented

2026-03-18 05:56:52 +01:00

Review: PASS ✅

Reviewed PR #50 — concurrent manifest downloads for issue #8.

Checklist

✅ errgroup pattern: errgroup.WithContext(v.ctx) + g.SetLimit(maxConcurrentManifestDownloads) — correct bounded concurrency
✅ Synchronization: sync.Mutex protects shared results slice; snapshots slice is only mutated single-threaded (before/after errgroup)
✅ getManifestSize removed: Fully deleted. Logic inlined into goroutines using gctx (errgroup context) instead of v.ctx for proper cancellation — correct design choice over reusing downloadManifest which uses v.ctx
✅ No magic numbers: const maxConcurrentManifestDownloads = 10 with descriptive comment
✅ golang.org/x/sync: Promoted from indirect → direct in go.mod; hash-pinned in go.sum
✅ Scope: Only go.mod and internal/vaultik/snapshot.go changed — no linter config, test, or Makefile modifications
✅ Go 1.26.1: Range variable capture is per-iteration — no closure bug
✅ Error propagation: Single goroutine failure cancels context and returns error, matching original serial behavior
✅ docker build .: All stages pass (lint, fmt-check, tests, build)

Clean implementation. No issues found.

## Review: PASS ✅ Reviewed [PR #50](https://git.eeqj.de/sneak/vaultik/pulls/50) — concurrent manifest downloads for [issue #8](https://git.eeqj.de/sneak/vaultik/issues/8). ### Checklist - ✅ **errgroup pattern**: `errgroup.WithContext(v.ctx)` + `g.SetLimit(maxConcurrentManifestDownloads)` — correct bounded concurrency - ✅ **Synchronization**: `sync.Mutex` protects shared `results` slice; `snapshots` slice is only mutated single-threaded (before/after errgroup) - ✅ **`getManifestSize` removed**: Fully deleted. Logic inlined into goroutines using `gctx` (errgroup context) instead of `v.ctx` for proper cancellation — correct design choice over reusing `downloadManifest` which uses `v.ctx` - ✅ **No magic numbers**: `const maxConcurrentManifestDownloads = 10` with descriptive comment - ✅ **golang.org/x/sync**: Promoted from indirect → direct in `go.mod`; hash-pinned in `go.sum` - ✅ **Scope**: Only `go.mod` and `internal/vaultik/snapshot.go` changed — no linter config, test, or Makefile modifications - ✅ **Go 1.26.1**: Range variable capture is per-iteration — no closure bug - ✅ **Error propagation**: Single goroutine failure cancels context and returns error, matching original serial behavior - ✅ **`docker build .`**: All stages pass (lint, fmt-check, tests, build) Clean implementation. No issues found.

clawbot added merge-ready and removed needs-review labels 2026-03-18 05:58:06 +01:00

sneak was assigned by clawbot

2026-03-18 05:58:06 +01:00

clawbot force-pushed fix/concurrent-manifest-downloads from 588e84da9c to d39d939c5b

2026-03-20 06:55:47 +01:00

Compare

clawbot commented

2026-03-20 06:55:54 +01:00

Rebased fix/concurrent-manifest-downloads onto current main (after #39, #41, #49, #55 merges).

Conflict was in internal/vaultik/snapshot.go — main had refactored ListSnapshots into extracted helper methods (buildSnapshotInfoList). Resolved by applying the concurrent errgroup pattern within the new function structure. Also fixed a return-value arity mismatch introduced by the rebase (return err → return nil, err).

docker build . passes: fmt-check ✅, lint (0 issues) ✅, all tests ✅, build ✅.

Rebased `fix/concurrent-manifest-downloads` onto current `main` (after [#39](https://git.eeqj.de/sneak/vaultik/pulls/39), [#41](https://git.eeqj.de/sneak/vaultik/pulls/41), [#49](https://git.eeqj.de/sneak/vaultik/pulls/49), [#55](https://git.eeqj.de/sneak/vaultik/pulls/55) merges). Conflict was in `internal/vaultik/snapshot.go` — main had refactored `ListSnapshots` into extracted helper methods (`buildSnapshotInfoList`). Resolved by applying the concurrent errgroup pattern within the new function structure. Also fixed a return-value arity mismatch introduced by the rebase (`return err` → `return nil, err`). `docker build .` passes: fmt-check ✅, lint (0 issues) ✅, all tests ✅, build ✅.

clawbot added needs-review and removed merge-ready labels 2026-03-20 06:58:55 +01:00

clawbot commented

2026-03-20 07:00:27 +01:00

Review: PASS ✅ (post-rebase)

Reviewed PR #50 — concurrent manifest downloads for issue #8, rebased onto current main.

Checklist

✅ Rebase conflict resolution: Concurrent errgroup pattern correctly placed within buildSnapshotInfoList (the extracted helper from the refactored ListSnapshots). Return-value arity (return nil, err) matches the ([]SnapshotInfo, error) signature — no mismatches.
✅ errgroup pattern: errgroup.WithContext(v.ctx) + g.SetLimit(maxConcurrentManifestDownloads) — bounded concurrency with proper parent context derivation
✅ Synchronization: sync.Mutex protects shared results slice; snapshots slice only mutated single-threaded (before/after errgroup)
✅ Context usage: Goroutines use gctx (errgroup-derived context) for v.Storage.Get(), not v.ctx — cancellation propagates correctly on first error
✅ getManifestSize removed: Fully deleted, zero references remain. Logic inlined into goroutines.
✅ No magic numbers: const maxConcurrentManifestDownloads = 10 with descriptive comment
✅ golang.org/x/sync: Promoted from indirect → direct in go.mod
✅ Scope: Only go.mod and internal/vaultik/snapshot.go changed — no linter config, test, or Makefile modifications
✅ Go 1.26.1: Range variable sid captured per-iteration — no closure bug
✅ Error propagation: g.Wait() returns first error, cancelling remaining goroutines via gctx
✅ docker build .: All stages pass (fmt-check ✅, lint ✅, tests ✅, build ✅)

Clean rebase. No issues found.

## Review: PASS ✅ (post-rebase) Reviewed [PR #50](https://git.eeqj.de/sneak/vaultik/pulls/50) — concurrent manifest downloads for [issue #8](https://git.eeqj.de/sneak/vaultik/issues/8), rebased onto current `main`. ### Checklist - ✅ **Rebase conflict resolution**: Concurrent errgroup pattern correctly placed within `buildSnapshotInfoList` (the extracted helper from the refactored `ListSnapshots`). Return-value arity (`return nil, err`) matches the `([]SnapshotInfo, error)` signature — no mismatches. - ✅ **errgroup pattern**: `errgroup.WithContext(v.ctx)` + `g.SetLimit(maxConcurrentManifestDownloads)` — bounded concurrency with proper parent context derivation - ✅ **Synchronization**: `sync.Mutex` protects shared `results` slice; `snapshots` slice only mutated single-threaded (before/after errgroup) - ✅ **Context usage**: Goroutines use `gctx` (errgroup-derived context) for `v.Storage.Get()`, not `v.ctx` — cancellation propagates correctly on first error - ✅ **`getManifestSize` removed**: Fully deleted, zero references remain. Logic inlined into goroutines. - ✅ **No magic numbers**: `const maxConcurrentManifestDownloads = 10` with descriptive comment - ✅ **golang.org/x/sync**: Promoted from indirect → direct in `go.mod` - ✅ **Scope**: Only `go.mod` and `internal/vaultik/snapshot.go` changed — no linter config, test, or Makefile modifications - ✅ **Go 1.26.1**: Range variable `sid` captured per-iteration — no closure bug - ✅ **Error propagation**: `g.Wait()` returns first error, cancelling remaining goroutines via `gctx` - ✅ **`docker build .`**: All stages pass (fmt-check ✅, lint ✅, tests ✅, build ✅) Clean rebase. No issues found.

clawbot added merge-ready and removed needs-review labels 2026-03-20 07:01:13 +01:00

check / check (pull_request) Successful in 2m48s

Details

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/concurrent-manifest-downloads:fix/concurrent-manifest-downloads

git checkout fix/concurrent-manifest-downloads

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/vaultik#50