Skip to content

fix: cancel blockstore enumeration when GC returns - #432

Open
jake-bp wants to merge 1 commit into
ipfs:mainfrom
jake-bp:fix/gc-cancel-key-enumeration
Open

jake-bp wants to merge 1 commit into
ipfs:mainfrom
jake-bp:fix/gc-cancel-key-enumeration

Conversation

@jake-bp

@jake-bp jake-bp commented Oct 18, 2026

Copy link
Copy Markdown

GC can return after deleting enough bytes while AllKeysChan is 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:

  • On v1.24.1, the real-flatfs regression fails before the fix with GC returned without closing its blockstore query and 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).
  • Linux/amd64 and Linux/arm64 on the pinned v1.24.1 source: 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 existing TestSeedPeering cases time out after 120s each, and TestPeriodicGC reports a Windows TempDir RemoveAll cleanup error (flatfs/.temp: directory is not empty). The new GC regression tests pass. These failures have not been compared against unpatched main.
  • With upstream CI gating enabled, CI=true go test -buildvcs=false -count=1 -timeout=15m ./... passes on current main (34.791s); upstream skips TestSeedPeering in CI.

Local tests use Windows/amd64 and Go 1.26.1. The separate Linux CI runner provides the race-detector result above.

@jake-bp
jake-bp force-pushed the fix/gc-cancel-key-enumeration branch from e32a2d3 to 0bea69a Compare October 18, 2026 18:47
@jake-bp jake-bp closed this Oct 18, 2026
@jake-bp jake-bp reopened this Oct 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant