feat(pinner): add Close with ErrClosed lifecycle#1150
Open
lidel wants to merge 5 commits into
Open
Conversation
Pinner gains Close() error. Close waits for every in-flight operation, including streaming goroutines from RecursiveKeys, DirectKeys, and InternalPins, to finish. After Close, every other Pinner method fails fast with a new ErrClosed sentinel; streaming methods surface it as the Err of a single entry on the returned channel, which is then closed. dspinner gains the matching implementation: Close is idempotent, admission is serialised with sync.Mutex + sync.WaitGroup, and stream sends select on a shutdown channel so a parked consumer cannot stall Close. The panic-recovery and context guards added in #1146 stay in place as defence in depth for hosts that do not wire Close correctly. Hosts that own the backing datastore (e.g. kubo) should call Close on the pinner before closing the datastore to avoid use-after-close panics in pebble and similar stores.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1150 +/- ##
==========================================
+ Coverage 63.11% 63.24% +0.13%
==========================================
Files 267 267
Lines 26777 26848 +71
==========================================
+ Hits 16899 16979 +80
+ Misses 8158 8152 -6
+ Partials 1720 1717 -3
... and 10 files with indirect coverage changes 🚀 New features to boost your workflow:
|
fix the errClosedChan rationale (buffering is needed because the send is synchronous and has no reader; there is no goroutine to leak) and the InternalPins comment that claimed the call was not tracked by p.wg despite begin() doing wg.Add. - changelog: condense the two close bullets, promote the downstream-impl break to an action-required callout, add #1150 refs - pinner.Pinner.Close / ErrClosed godoc: lead with the load-bearing verb, drop restatement - dspinner: errClosedChan, InternalPins, streamIndex send/recover, begin, Close, SetAutosync comments shortened or corrected
lidel
added a commit
to ipfs/kubo
that referenced
this pull request
May 13, 2026
Close fires context.AfterFunc on every admitted op's derived ctx, so in-flight Pin/Unpin/streams that honor ctx bail out promptly instead of forcing kubo's shutdown to wait or trip the watchdog. - stopCtx + AfterFunc fan-out replaces the chan-based done signal - begin returns the derived ctx; admitted methods thread it through - streamIndex send watches one ctx.Done; Close-driven cancel from snapshotIndex surfaces as ErrClosed on the stream
lidel
added a commit
to ipfs/kubo
that referenced
this pull request
May 13, 2026
ipfs/boxo#1150 was reworked to use context fan-out instead of a done channel. Pinner.Close now cancels every admitted op and waits for them to return, broadening the shutdown contract from "drain streams" to "drain everything". Comments and changelog reworded to match.
gammazero
approved these changes
May 13, 2026
|
|
||
| require.ErrorIs(t, <-pinReturned, context.Canceled) | ||
| require.NoError(t, <-closeReturned) | ||
| } |
Contributor
There was a problem hiding this comment.
Would testing.Synctest be good here too?
drop the 20ms/50ms timing in TestCloseWaitsForInFlightOperation and move it into a synctest bubble (gammazero review). Go 1.26 does not treat sync.RWMutex.Lock waiting behind a durably-held RLock as durably blocked, so swap the non-ctx-aware blocker from the pinner's RWMutex to a tiny ctxIgnoringDAGService whose Add parks on a channel and ignores ctx. Same behavioral assertion (Close must wait on wg when the cancellation fan-out cannot unstick the in-flight op), deterministic and ~50x faster. also drop the duplicated doc paragraph above the test.
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
2 participants
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.
Follow-up to #1146. The Pinner had no lifecycle, so kubo/pebble could close the backing datastore while pinner goroutines were still reading it and hit the use-after-close panic that #1146 papers over with
recover().👉 This PR adds
Close() errorto thePinnerinterface and implements it indspinner.Close fires
context.AfterFuncon every admitted op's derived ctx, so in-flightPin,Unpin,Flush,Update, and streaming goroutines that honor ctx bail out promptly withcontext.Canceled(scalar) orErrClosed(stream) rather than draining to completion. After Close, every other method fails fast withErrClosed; streaming methods deliver it asStreamedPin.Erron a single entry then close the channel. Close is idempotent and goroutine-safe.This means Close returns about as fast as the slowest in-flight op takes to honor its ctx. Hosts with a hard shutdown deadline (e.g. kubo via ipfs/kubo#11329's
shutdown.CloseWithCtx) get bounded shutdown for free, and the panic-recovery from #1146 stays as defense in depth.Breaking change: downstream
Pinnerimplementations must addClose() error. Hosts owning the datastore should callCloseon the pinner before closing the datastore.Kubo PR that uses this: