Skip to content

Comments

fix(pinner): restore indirect pin detection and add context cancellation#1039

Merged
lidel merged 3 commits intomainfrom
feat/add-context-checks-indirect-pins
Sep 19, 2025
Merged

fix(pinner): restore indirect pin detection and add context cancellation#1039
lidel merged 3 commits intomainfrom
feat/add-context-checks-indirect-pins

Conversation

@lidel
Copy link
Member

@lidel lidel commented Sep 18, 2025

Follow-up for #1035 (forgot to push local changes):

While at it, some cleanup:

- add context checks at start of each recursive pin traversal
- extract traverseIndirectPins helper to eliminate duplicate code
- reduce code duplication by ~40 lines while maintaining functionality
- add tests for context cancellation behavior

this fixes a pre-existing issue where indirect pin traversal never
checked for context cancellation, causing operations to continue even
after cancellation was requested. especially important for large pin
sets with many recursive pins.
Recursively pinned roots should not show as indirect.
Objects can be both directly and indirectly pinned.

Restores behavior from before PR #1035.
@codecov
Copy link

codecov bot commented Sep 18, 2025

Codecov Report

❌ Patch coverage is 78.43137% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.74%. Comparing base (0cd6062) to head (f73b218).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pinning/pinner/dspinner/pin.go 78.43% 7 Missing and 4 partials ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1039      +/-   ##
==========================================
+ Coverage   60.67%   60.74%   +0.07%     
==========================================
  Files         268      268              
  Lines       33560    33597      +37     
==========================================
+ Hits        20364    20410      +46     
+ Misses      11520    11515       -5     
+ Partials     1676     1672       -4     
Files with missing lines Coverage Δ
pinning/pinner/dspinner/pin.go 61.09% <78.43%> (+1.77%) ⬆️

... and 12 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

explain why recursive pins are excluded from indirect results while
direct pins are not. this asymmetry is intentional and preserved for
compatibility with established ipfs behavior.
@lidel lidel marked this pull request as ready for review September 18, 2025 23:49
@lidel lidel requested a review from a team as a code owner September 18, 2025 23:49
@lidel
Copy link
Member Author

lidel commented Sep 18, 2025

I've documented the backward-compatibility criteria and confirmed this PR passes legacy sharness tests in:

I'm going to merge this since #1035 is already in the main branch, we need this PR as well for Kubo 0.38.0-rc1 (ipfs/kubo#10884).

We may want to look at this in the future, maybe the legacy behavior can/should be changed, but I don't want to make any extra waves for 0.38 -- keeping things as-is where possible for now.

@lidel lidel mentioned this pull request Sep 18, 2025
49 tasks
@lidel lidel enabled auto-merge (squash) September 19, 2025 00:01
@lidel lidel disabled auto-merge September 19, 2025 00:01
@lidel lidel merged commit 031df82 into main Sep 19, 2025
14 checks passed
@lidel lidel deleted the feat/add-context-checks-indirect-pins branch September 19, 2025 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant