Repository navigation
Conversation
jake-bp
force-pushed
the
fix/gc-cancel-key-enumeration
branch
from
October 18, 2026 18:47
e32a2d3 to
0bea69a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GC can return after deleting enough bytes while
AllKeysChanis still enumerating the remaining blockstore. Its producer then blocks on the undrained key channel; with flatfs, the underlying datastore query also stays open. Repeated partial GC passes can accumulate these goroutines, as reported in #272.Give key enumeration a child context and defer its cancellation on every return path. Cancellation lets Boxo leave its producer loop and run
res.Close(), which stops the flatfs query producer. Block reads, deletion, and datastore GC continue using the caller's context.The regression test uses real flatfs with more blocks than both iterator buffers, collects only one block on each of three passes, and observes query closure without draining the iterator. It also checks exact deletion counts and that the caller's context stays active. Targeted tests cover deletion and enumeration errors, zero and negative targets, and parent cancellation.
Validation:
GC returned without closing its blockstore queryand passes after applying the fix.go test -count=1 -timeout=10m -run 'Test(GC|PeriodicGC)' .passes on v1.24.1.go test -count=20 -timeout=10m -run TestGC .passes on v1.24.1.go test -buildvcs=false -count=1 -timeout=10m -run 'Test(GC|PeriodicGC)' .passes on current main (9f5131241e733c7320a7cf50d11e5dcd0054945e, Boxo v0.43.0).go test -mod=readonly -race -count=3 -timeout=15m -run 'Test(GC|PeriodicGC)' .passes.go test -buildvcs=false -timeout=15m ./...on current main fails locally: all three existingTestSeedPeeringcases time out after 120s each, andTestPeriodicGCreports a WindowsTempDir RemoveAllcleanup error (flatfs/.temp: directory is not empty). The new GC regression tests pass. These failures have not been compared against unpatched main.CI=true go test -buildvcs=false -count=1 -timeout=15m ./...passes on current main (34.791s); upstream skipsTestSeedPeeringin CI.Local tests use Windows/amd64 and Go 1.26.1. The separate Linux CI runner provides the race-detector result above.