fix(table): bound AllManifests concurrency - #1668
Conversation
zeroshade
left a comment
There was a problem hiding this comment.
The change is correct. I verified the bounding and the error propagation:
- The bound is real and ordering is unaffected.
MakeSequencedChan(table/internal/utils.go:83-99) still re-sequences results byIndexthrough a heap and is untouched by this PR, so capping worker count does not change the order manifests are yielded in. - Error semantics are preserved:
errgroup.WithContextretains the first error and cancels the remaining workers, and the feeder goroutine unblocks ongroupCtx.Done()rather than blocking forever on a fulljobschannel. - No leak on early exit.
outis still allocated with capacityn(table/internal/utils.go:86), so workers can always drain even if the consumer stops iterating, and the existing drain goroutine plus the newdefer cancel()(table/table.go:403) close that loop.
Everything below is optional — nothing here blocks.
| n := len(snapshots) | ||
| workCtx, cancel := context.WithCancel(ctx) | ||
| jobs := make(chan int) | ||
| ch := make(chan list, max(1, min(n, 16))) |
There was a problem hiding this comment.
Non-blocking: because out inside MakeSequencedChan is allocated with capacity n, this cap 16 provides essentially no backpressure — peak memory is still all n manifest lists held at once. Worth a short comment so a future reader doesn't mistake this expression for a memory bound.
| workCtx, cancel := context.WithCancel(ctx) | ||
| jobs := make(chan int) | ||
| ch := make(chan list, max(1, min(n, 16))) | ||
| workers := max(1, min(n, min(runtime.GOMAXPROCS(0), 16))) |
There was a problem hiding this comment.
Two small things here, both optional.
16appears on this line and the one above as an unnamed literal. A named constant would make the intent legible and keep the two in sync.GOMAXPROCS(0)derives a CPU bound for what is purely remote I/O. On a two-core CI runner, a 1000-snapshot history now fetches two-wide, which is likely slower than intended. Consider a bound independent of CPU count — and possibly an option, sinceDelete/Overwritealready expose a concurrency knob (WithOverwriteConcurrency), which leavesAllManifestsas the odd one out.
| workers := max(1, min(n, min(runtime.GOMAXPROCS(0), 16))) | ||
| g, groupCtx := errgroup.WithContext(workCtx) | ||
|
|
||
| for range workers { |
There was a problem hiding this comment.
Worth a follow-up: errgroup.Group.SetLimit(workers) plus a single scheduling goroutine yields the same bound in roughly five lines and removes the hand-rolled jobs channel entirely. The current implementation is correct — this is purely a simplification, so it's fine to defer or skip.
| trackingFS.mu.Lock() | ||
| maxOpen := trackingFS.maxOpen | ||
| trackingFS.mu.Unlock() | ||
| require.Greater(t, maxOpen, 1) |
There was a problem hiding this comment.
Non-blocking, but this is the one assertion I'd soften. maxOpen > 1 requires two goroutines to overlap within the 5 ms sleep, so it can flake on a loaded or effectively single-CPU runner. The upper-bound assertion on the next line is the property this PR actually adds; consider keeping only that, or having the tracking IO rendezvous through a semaphore or WaitGroup so the overlap is deterministic rather than timing-derived. Mutating global GOMAXPROCS is also worth a second look if tests ever run in parallel here.
| } | ||
| } | ||
|
|
||
| func TestAllManifestsLimitsConcurrentReads(t *testing.T) { |
There was a problem hiding this comment.
Suggestions for hardening, none blocking — the bound itself is well covered by this test. Cases that would round it out: a worker error cancels the remaining workers and the first error is what surfaces; an early break out of the range leaks no goroutines (goleak would catch this cheaply); context cancellation propagates to in-flight workers; and the n == 0 / n == 1 edges where max(1, min(n, ...)) collapses.
Summary
Bound the remote work started by AllManifests.
Why
The old implementation started one goroutine and one manifest-list read per snapshot. Long table histories could create a large burst of remote I/O even when the caller stopped early.
What changed
AllManifests now uses a bounded worker pool, stops scheduling after cancellation, preserves snapshot order, and keeps the existing error behavior.
Tests