diff --git a/pinning/pinner/dspinner/pin.go b/pinning/pinner/dspinner/pin.go index c32a13db2..94c8a3dea 100644 --- a/pinning/pinner/dspinner/pin.go +++ b/pinning/pinner/dspinner/pin.go @@ -646,35 +646,9 @@ func (p *pinner) CheckIfPinnedWithType(ctx context.Context, mode ipfspinner.Mode // Check for indirect pins if toCheck.Len() > 0 { - var walkErr error - visited := cid.NewSet() - err := p.cidRIndex.ForEach(ctx, "", func(key, value string) bool { - var rk cid.Cid - rk, walkErr = cid.Cast([]byte(key)) - if walkErr != nil { - return false - } - walkErr = merkledag.Walk(ctx, merkledag.GetLinksWithDAG(p.dserv), rk, func(c cid.Cid) bool { - if toCheck.Len() == 0 || !visited.Visit(c) { - return false - } - if toCheck.Has(c) { - pinned = append(pinned, ipfspinner.Pinned{Key: c, Mode: ipfspinner.Indirect, Via: rk}) - toCheck.Remove(c) - } - return true - }, merkledag.Concurrent()) - if walkErr != nil { - return false - } - return toCheck.Len() > 0 - }) - if err != nil { + if err := p.traverseIndirectPins(ctx, toCheck, &pinned); err != nil { return nil, err } - if walkErr != nil { - return nil, walkErr - } } // Anything left in toCheck is not pinned @@ -741,48 +715,93 @@ func (p *pinner) checkPinsInIndex(ctx context.Context, mode ipfspinner.Mode, inc return pinned, nil } +// traverseIndirectPins is a helper that traverses all recursive pins to find indirect pins. +// It modifies the pinned slice and toCheck set in place. +func (p *pinner) traverseIndirectPins(ctx context.Context, toCheck *cid.Set, pinned *[]ipfspinner.Pinned) error { + var walkErr error + visited := cid.NewSet() + err := p.cidRIndex.ForEach(ctx, "", func(key, value string) bool { + // Check for context cancellation at the start of each recursive pin + select { + case <-ctx.Done(): + walkErr = ctx.Err() + return false + default: + } + + var rk cid.Cid + rk, walkErr = cid.Cast([]byte(key)) + if walkErr != nil { + return false + } + walkErr = merkledag.Walk(ctx, merkledag.GetLinksWithDAG(p.dserv), rk, func(c cid.Cid) bool { + if toCheck.Len() == 0 || !visited.Visit(c) { + return false + } + if toCheck.Has(c) { + *pinned = append(*pinned, ipfspinner.Pinned{Key: c, Mode: ipfspinner.Indirect, Via: rk}) + toCheck.Remove(c) + } + return true + }, merkledag.Concurrent()) + if walkErr != nil { + return false + } + return toCheck.Len() > 0 + }) + if err != nil { + return err + } + if walkErr != nil { + return walkErr + } + return nil +} + // checkIndirectPins checks if the given cids are pinned indirectly func (p *pinner) checkIndirectPins(ctx context.Context, cids ...cid.Cid) ([]ipfspinner.Pinned, error) { pinned := make([]ipfspinner.Pinned, 0, len(cids)) toCheck := cid.NewSet() - // Check all CIDs for indirect pins, regardless of their direct pin status - // A CID can be both directly pinned AND indirectly pinned through a parent + // Filter out CIDs that are recursively pinned at the root level. + // A recursively pinned CID is not considered indirect because recursive pins + // are comprehensive (include all children), making "recursive" take precedence + // over "indirect". + // + // However, we do NOT filter out direct pins here. Direct pins only pin a + // single block, not its children. Therefore, a CID can legitimately be both: + // - Directly pinned (explicitly pinned as a single block) + // - Indirectly pinned (referenced by another pinned object's DAG) + // This is why the asymmetry between recursive and direct pins is intentional. + // + // NOTE: While this behavior may feel arbitrary, we preserve it for compatibility + // as this is how 'ipfs pin ls' has behaved for nearly a decade. The test + // t0081-repo-pinning.sh in Kubo explicitly expects a CID to be both direct + // and indirect, guarding this established behavior. for _, c := range cids { + cidKey := c.KeyString() + + // Check if recursively pinned + ids, err := p.cidRIndex.Search(ctx, cidKey) + if err != nil { + return nil, err + } + if len(ids) > 0 { + // This CID is recursively pinned at root level, not indirect + pinned = append(pinned, ipfspinner.Pinned{Key: c, Mode: ipfspinner.NotPinned}) + continue + } + + // Still check for indirect even if directly pinned + // A CID can be both direct and indirect toCheck.Add(c) } // Now check for indirect pins by traversing recursive pins if toCheck.Len() > 0 { - var walkErr error - visited := cid.NewSet() - err := p.cidRIndex.ForEach(ctx, "", func(key, value string) bool { - var rk cid.Cid - rk, walkErr = cid.Cast([]byte(key)) - if walkErr != nil { - return false - } - walkErr = merkledag.Walk(ctx, merkledag.GetLinksWithDAG(p.dserv), rk, func(c cid.Cid) bool { - if toCheck.Len() == 0 || !visited.Visit(c) { - return false - } - if toCheck.Has(c) { - pinned = append(pinned, ipfspinner.Pinned{Key: c, Mode: ipfspinner.Indirect, Via: rk}) - toCheck.Remove(c) - } - return true - }, merkledag.Concurrent()) - if walkErr != nil { - return false - } - return toCheck.Len() > 0 - }) - if err != nil { + if err := p.traverseIndirectPins(ctx, toCheck, &pinned); err != nil { return nil, err } - if walkErr != nil { - return nil, walkErr - } } // Anything left in toCheck is not pinned diff --git a/pinning/pinner/dspinner/pin_withtype_test.go b/pinning/pinner/dspinner/pin_withtype_test.go index a53809260..a5da52045 100644 --- a/pinning/pinner/dspinner/pin_withtype_test.go +++ b/pinning/pinner/dspinner/pin_withtype_test.go @@ -2,7 +2,9 @@ package dspinner import ( "context" + "fmt" "testing" + "time" bs "github.com/ipfs/boxo/blockservice" blockstore "github.com/ipfs/boxo/blockstore" @@ -155,4 +157,57 @@ func TestCheckIfPinnedWithType(t *testing.T) { require.Equal(t, pinned1, pinned2) }) + + t.Run("Context cancellation during indirect check with many pins", func(t *testing.T) { + // Create many recursive pins to ensure we can hit the cancellation + nodes := make([]cid.Cid, 50) + for i := 0; i < 50; i++ { + node := mdag.NodeWithData([]byte(fmt.Sprintf("recursive node %d", i))) + err = dserv.Add(ctx, node) + require.NoError(t, err) + nodes[i] = node.Cid() + + p.PinWithMode(ctx, nodes[i], ipfspin.Recursive, fmt.Sprintf("recursive-%d", i)) + } + err = p.Flush(ctx) + require.NoError(t, err) + + // Create a context that we can cancel + cancelCtx, cancel := context.WithCancel(ctx) + + // Start checking in a goroutine + done := make(chan struct{}) + var checkErr error + go func() { + defer close(done) + // Try to check for an indirect pin with many recursive pins to traverse + _, checkErr = p.CheckIfPinnedWithType(cancelCtx, ipfspin.Indirect, withoutNames, ck) + }() + + // Cancel the context quickly + time.Sleep(1 * time.Millisecond) + cancel() + + // Wait for the check to complete + <-done + + // Should get a context cancellation error (or nil if it completed too fast) + // The important thing is that it doesn't hang + if checkErr != nil { + require.Equal(t, context.Canceled, checkErr) + } + }) + + t.Run("Context cancellation in checkIndirectPins", func(t *testing.T) { + // Create a context that we can cancel immediately + cancelCtx, cancel := context.WithCancel(ctx) + cancel() // Cancel immediately + + // Try to check for indirect pins with cancelled context + _, err = p.CheckIfPinnedWithType(cancelCtx, ipfspin.Indirect, withoutNames, ck) + + // Should get a context cancellation error + require.Error(t, err) + require.Equal(t, context.Canceled, err) + }) }