What
advanced.concurrency controls two unrelated resources. In internal/uploader/transfer.go it bounds the network upload pool:
g, ctx := errgroup.WithContext(ctx)
g.SetLimit(cfg.Concurrency)
and in internal/uploader/sync.go the same value is handed to the local hashing pool:
if err := hashPlanFiles(ctx, p.files, cfg.Concurrency, cached); err != nil {
which uses it as g.SetLimit(concurrency) over hashFile calls (internal/uploader/planner.go).
docs/configuration.md documents this honestly ("Also bounds the sync hashing worker pool"), so it is not hidden. It is still the wrong coupling.
Why it matters
The two pools are limited by different things and want different values:
- Uploads are bounded by the server: its per-connection limits, its disk, the link's bandwidth-delay product, and how much parallelism it tolerates before it starts refusing or throttling. The reason a user lowers this value is almost always "my server does not like many parallel writes".
- Hashing is bounded by the runner: CPU cores and local disk throughput. It never touches the server at all.
So the user with the fragile shared-hosting server sets concurrency: 1, which is the correct instruction for the upload pool, and as a side effect makes a sync of a large tree read and SHA-256 every local file strictly one at a time. On a 4-core runner that is roughly a 4x slowdown of a phase that has nothing to do with the reason they set the value. It lands hardest on exactly the sync-with-many-files case that #30 (parallel hashing) was added to fix.
The reverse is true too: a user with a fast server who raises concurrency to 32 to fill a fat pipe now runs 32 concurrent file reads on the runner's disk, which for spinning or throttled storage is worse than 4.
Suggested direction
Decouple the hashing pool from the transfer pool. The lazy version is the right one here: hashing does not need its own config knob, it needs a sensible internal default.
hashPlanFiles(ctx, p.files, runtime.GOMAXPROCS(0), cached)
That is one line, removes the surprise, and needs no new configuration surface (which matches the "most new knobs belong in the simpler category, and preferably nowhere" principle in CLAUDE.md).
If a knob turns out to be genuinely needed later, advanced.hash_concurrency is the natural name, but it should not be added speculatively.
The line in docs/configuration.md that documents the current coupling would be removed in the same change.
What
advanced.concurrencycontrols two unrelated resources. Ininternal/uploader/transfer.goit bounds the network upload pool:and in
internal/uploader/sync.gothe same value is handed to the local hashing pool:which uses it as
g.SetLimit(concurrency)overhashFilecalls (internal/uploader/planner.go).docs/configuration.mddocuments this honestly ("Also bounds the sync hashing worker pool"), so it is not hidden. It is still the wrong coupling.Why it matters
The two pools are limited by different things and want different values:
So the user with the fragile shared-hosting server sets
concurrency: 1, which is the correct instruction for the upload pool, and as a side effect makes a sync of a large tree read and SHA-256 every local file strictly one at a time. On a 4-core runner that is roughly a 4x slowdown of a phase that has nothing to do with the reason they set the value. It lands hardest on exactly the sync-with-many-files case that #30 (parallel hashing) was added to fix.The reverse is true too: a user with a fast server who raises
concurrencyto 32 to fill a fat pipe now runs 32 concurrent file reads on the runner's disk, which for spinning or throttled storage is worse than 4.Suggested direction
Decouple the hashing pool from the transfer pool. The lazy version is the right one here: hashing does not need its own config knob, it needs a sensible internal default.
That is one line, removes the surprise, and needs no new configuration surface (which matches the "most new knobs belong in the simpler category, and preferably nowhere" principle in CLAUDE.md).
If a knob turns out to be genuinely needed later,
advanced.hash_concurrencyis the natural name, but it should not be added speculatively.The line in
docs/configuration.mdthat documents the current coupling would be removed in the same change.