feat: add experimental on-demand pinning#11252
Conversation
Add a background checker that automatically pins content when DHT provider counts fall below a configurable replication target and unpins once enough providers exist again after a grace period.
Gated behind Experimental.OnDemandPinningEnabled.
New CLI commands: ipfs pin ondemand {add,rm,ls}
Safety measures:
- storage budget check (respects StorageMax/GCWatermark)
- idle timeout on recursive DAG fetches (2 min without progress)
- pin partitioning via pin name to distinguish on-demand pins from persitent pins.
|
Please focus the review on: 1. Core logic —
|
…adding v0.42 changelog entry
|
@guillaumemichel Please re-run the checks. I addressed the failing ones for overfull lines , typos, and changelog. |
| "github.com/ipfs/kubo/ondemandpin" | ||
| ) | ||
|
|
||
| const onDemandLiveOptionName = "live" |
There was a problem hiding this comment.
the ondemand ls command shows the locally stored pin records (CID, pinned status, timestamps). With ---live, it additionally queries the DHT in real-time to report how many providers currently exist
There was a problem hiding this comment.
DHT queries are sequential. Doing ipfs pin ondemand ls --live on hundreds of CIDs will take a long time. Still handy for debugging and development.
|
Triage note: we run out of time to review this in 0.41 iteration, but we've added it to 0.42. Thank you for your contribution and patience. 🙏 |
lidel
added
the
need/maintainers-input
gammazero
added
exp/expert
|
Triage: need careful review and testing. |
…ning # Conflicts: # docs/changelogs/v0.42.md # docs/config.md
|
Triage: short on review bandwidth for v0.42, so we'd like to land this in v0.43 instead. I've rebased on latest master and moved the changelog entry to |
|
Hi @lidel , just checking in on this. Please let me know if there’s any additional documentation, context, or testing steps I can provide to help ease the review. |
lidel
left a comment
lidel
left a comment
There was a problem hiding this comment.
Thanks for waiting @ihlec. This deserved a real review a lot sooner than it got one.
Being straight with you: maintainer time on Kubo is a small shared budget this quarter, and this is a lot of code. A new package, new config, new commands, and a loop that pins and unpins on its own. That is not something I can skim, and a feature that deletes pins by itself really shouldn't be skimmed. I kept not finding the hours, and the release ran out first.
So I'm moving it to draft and off v0.43. That is not a rejection. I believe the idea is good and could be added to future Kubo, but pin ownership, the pin and unpin loop, and the name all need a decision before more code goes in. The inline comments say where I would take each one to be less brittle and respect user config. Feel free to push back on any of it, this is a first-pass set of comments, possible I misunderstood something.
| } | ||
| c := rp.RootCid() | ||
|
|
||
| isOurs, err := ondemandpin.PinHasName(req.Context, n.Pinning, c, ondemandpin.OnDemandPinName) |
There was a problem hiding this comment.
ipfs pin ondemand rm decides what to unpin from the pin name (PinHasName with OnDemandPinName), and on-demand is not reserved: ValidatePinName only limits length.
So a user can create their own pin with ipfs pin add --name on-demand, and ipfs pin ondemand rm on that CID will delete it.
The order makes it worse: api.Pin().Rm runs before store.Remove, so for a CID that was never registered the pin is already gone by the time ErrNotRegistered reaches the user.
Could rm read the store first and unpin only when the record exists and PinnedByUs is set? That keys ownership on the store record instead of a free-text name, so nothing has to be reserved; if you would rather keep the name check, namespacing OnDemandPinName as something like kubo:on-demand and rejecting that prefix in ValidatePinName would work too.
| return | ||
| } | ||
|
|
||
| if err := c.pins.Pin(ctx, rec.Cid, OnDemandPinName); err != nil { |
There was a problem hiding this comment.
pinned comes from IsPinned at line 143, before the provider lookup in CountProviders and before Pin fetches the DAG. By the time we pin, it can be minutes old.
If a user runs ipfs pin add --name mydata <cid> in that gap, boxo's doPinRecursive removes the existing recursive pin and re-adds it under the name we pass, so their pin turns into the on-demand pin. Once providers recover and the grace period ends, handleWellReplicated sees OnDemandPinName on it and unpins. The user loses a pin they made themselves.
Could checkRecord re-read the pin state right before it calls Pin? That would not close the race, since the DAG fetch inside Pin still leaves a gap, but it would make the gap much smaller.
| const ( | ||
| DefaultOnDemandPinReplicationTarget = 5 | ||
| DefaultOnDemandPinCheckInterval = 10 * time.Minute | ||
| DefaultOnDemandPinUnpinGracePeriod = 24 * time.Hour |
There was a problem hiding this comment.
DefaultOnDemandPinUnpinGracePeriod is shorter than how long a DHT provider record stays valid (amino.DefaultProvideValidity is 48h), and CountProviders cannot tell a stale record from a live peer.
A provider that goes offline right after the grace timer starts is still returned by the DHT for up to 48h. So this node can stay above ReplicationTarget for the full 24h and unpin while it may be the last one still holding the blocks.
Could DefaultOnDemandPinUnpinGracePeriod be derived from amino.DefaultProvideValidity instead of a hardcoded 24h? With a grace period longer than the record validity, stale records expire before the period ends, the count drops below target, and the timer resets.
config/provide.go already imports the amino package and checks Provide.DHT.Interval against that constant, so the default here would follow upstream if the DHT ever changes it.
| log.Info("on-demand pin checker started") | ||
| defer log.Info("on-demand pin checker stopped") | ||
|
|
||
| ticker := time.NewTicker(c.checkInterval) |
There was a problem hiding this comment.
None of the three OnDemandPinning options are validated.
A CheckInterval of 0s or a negative value reaches time.NewTicker(c.checkInterval) in Run, and time.NewTicker panics on a non-positive interval. The panic happens in the goroutine started by the OnStart hook in core/node/ondemandpin.go, so one config typo takes the daemon down.
A ReplicationTarget of 0 or less makes count < c.replicationTarget never true. The checker then pins nothing and, once the grace period passes, unpins what it already holds.
Could these be checked at config load, next to the ValidateProvideConfig call in core/node/groups.go (ReplicationTarget >= 1, CheckInterval > 0, UnpinGracePeriod > 0), so a bad value fails at startup with a clear error?
| log.Errorw("failed to check pin state, skipping CID", "cid", rec.Cid, "error", err) | ||
| return | ||
| } | ||
| if !rec.PinnedByUs && pinned { |
There was a problem hiding this comment.
🤔PinnedByUs lives in the record, not on the pin, so the two can drift apart.
If the daemon dies between c.pins.Pin (line 183) and c.saveRecord (line 194), or if that save fails (it only logs the error), the pin is on disk while the record still says PinnedByUs=false. From then on this guard reads our own pin as a user pin and skips the CID every cycle. The checker never unpins it once replication recovers, and ipfs pin ondemand ls keeps printing not-pinned for it.
Could the guard ask HasPinWithName(ctx, rec.Cid, OnDemandPinName) instead? handleWellReplicated already calls it before unpinning, and ipfs pin ondemand rm uses PinHasName the same way, so the pin name is already trusted elsewhere.
If the guard trusted it too, PinnedByUs would not be needed at all, and the drift goes away with the field.
| return records, nil | ||
| } | ||
|
|
||
| func (s *Store) Update(ctx context.Context, rec *Record) error { |
There was a problem hiding this comment.
🤔 iiuc Update does a blind put with no check that the record still exists, so the checker can recreate a record the user just deleted.
checkRecord reads the Record, then spends up to checkTimeout (5 minutes) on a DHT walk and a DAG fetch. If ipfs pin ondemand rm <cid> lands in that gap, it deletes the key and reports success, but the in-flight saveRecord writes the key back. The CID shows up in ipfs pin ondemand ls again, and the next cycle re-pins it if it is still under target.
The RWMutex does not catch this, since the checker's read and write are separate locked calls.
Could Update do a Has check and return ErrNotRegistered when the key is gone?
| }, | ||
| } | ||
|
|
||
| type OnDemandLsOutput struct { |
There was a problem hiding this comment.
General UX nits: for a feature that unpins on its own, ls says little about why.
The stored Record only has PinnedByUs, LastAboveTarget and CreatedAt, and --live gives a fresh DHT count, not what the checker last saw or did. So these all print the same not-pinned: count above target, a user pin in the way (checkRecord skips those), hasStorageBudget false (only a warn log), or a pin that failed or ran past checkTimeout.
The unpin time is knowable, above-target-since plus the configured UnpinGracePeriod, but the operator has to open the config and do the math.
Could ondemandpin.Record also keep LastCheckedAt, LastProviderCount and a short LastResult reason, so ls can show them and a computed UnpinAt?
A dry-run mode would help too, so an operator can watch what the checker would do for a week before letting it unpin anything.
| out.LastAboveTarget = rec.LastAboveTarget.Format(time.RFC3339) | ||
| } | ||
|
|
||
| if live && n.Routing != nil { |
There was a problem hiding this comment.
🤔 hm.. iiuc this also is pretty brittle: CountProviders runs once per record inside the loop, on req.Context with no deadline, so ls --live takes as long as all the DHT lookups added up, and one CID that nobody provides can stall the rest of the listing.
checkRecord bounds each check with checkTimeout. A context.WithTimeout per lookup, plus a few running in parallel, would do the same here.
n.Routing != nil guard: with no routing, Providers stays nil, so the text output drops the providers= column and JSON omits the field. The result looks like a plain ls, with no hint that --live did nothing.
Could it return an error there, or explicitly say that provider counts are unavailable? In general, try to write code that avoids hiding the fact silent failures occured, better to be explicit and/or loud.
| DefaultOnDemandPinUnpinGracePeriod = 24 * time.Hour | ||
| ) | ||
|
|
||
| type OnDemandPinning struct { |
There was a problem hiding this comment.
ℹ️ Could we settle the naming before this ships, and borrow ipfs-cluster's words instead of new ones?
"On demand" normally means fetch it when someone asks for it (video on demand). This checker does the opposite: it counts providers and pins when the content is rare. Cluster already has a vocabulary for that idea: replication_factor_min and replication_factor_max in its config, ReplicationFactorMin and ReplicationFactorMax in its API, --replication-min and --replication-max in ipfs-cluster-ctl. The mechanism is not the same (cluster allocates among known peers, this checker observes the DHT), but the user-facing question is: how many copies should exist, and what happens when there are too few.
Concretely: config section ReplicationPinning, Experimental.ReplicationPinningEnabled, CLI ipfs pin replication add|rm|ls, package replicationpin/.
The min/max pair is worth borrowing too, not just the words. Cluster's min is the minimum healthy copies, and its max is the target number of nodes that should pin. The gap between the two is the deadband this checker does not have today (see my note on checkRecord): pin below min, unpin only above max. A single ReplicationTarget cannot express that.
The --help text would carry the same vocabulary. While it is being touched: the tagline says content is pinned "when few DHT providers exist in the routing table", but provider records are not in the routing table, they are found with FindProviders.
This is much easier now than later. The pin name on-demand and the datastore prefix /ondemand-pins/ both end up on disk, so renaming after a release means a migration for anyone who turned the experiment on. ipfs/specs#532 carries the same term in its title.
If the current naming is deliberate, I would rather hear your reasoning than push a rename through.
There was a problem hiding this comment.
When going forward with ReplicationPinning, the user might ask:
- Does a regular pin NOT replicate?
- Whats the difference to normal pinning?
I chose the wording to reflect a "demand to replicate". If a CID is at risk to be lost - only then start pinning.
Keeping the word "pinning" is essential to show the similarities. So that is a strength of both names that we should keep.
ReplicationPinning tells the user what it is doing, but does not tell the user what it is not doing.
I think we need to point out that it is a non-permanent pinning with conditions.
We could go with ConditionalPinning to keep the feature extensible for other conditions. Available disk space is such a condition, so it would be transparent that the replication demand is not the only condition.
On the other hand, I still think OnDemand pinning reflects the goal of the feature very well (The network needs you to jump in and replicate to protect the data from being lost).
""On demand" normally means fetch it when someone asks for it" - In this case the network asks for it.
"Uncle Sam wants you" vs "IPFS demands you to protect your data."
There was a problem hiding this comment.
I am open to follow your intuition here. Feel free to make a decision after considering my argument.
There was a problem hiding this comment.
I don't feel strongly about this tbh, just wanted to ensure the name is not a placeholder.
If you want to keep "on demand" that is fine with me, just make sure docs / --help text on the CLI commands explain that this feature is a decentralized and uncoordinated sibling of "replication factors" in ipfs-cluster (https://ipfscluster.io/documentation/guides/pinning/#replication-factors). (It is useful to tell people what feature is by how it is different (serving different need) from other thing they may be already familiar with.)
| rec.LastAboveTarget = time.Time{} | ||
| log.Infow("pinned", "cid", rec.Cid, "providers", count, "target", c.replicationTarget) | ||
|
|
||
| if err := c.routing.Provide(ctx, rec.Cid, true); err != nil { |
There was a problem hiding this comment.
routing.Provide directly, so it ignores both Provide.Enabled and Provide.Strategy.
The checker is wired with a raw routing.ContentRouting (core/node/ondemandpin.go), not the node's DHTProvider. When an operator sets Provide.Enabled=false, kubo swaps in NoopProvider so nothing is announced, but this call still goes straight to the DHT. A node that was told not to announce content announces anyway. A bug.
It is also a one-shot, with no place in the reprovide schedule. With the default Provide config (Enabled=true, Strategy=all) the reprovider already keeps these pins announced, so the call is mostly redundant. With a strategy that skips pins (for example mfs), it is the only announcement the node ever makes for the DAG, and the record expires after amino.DefaultProvideValidity. Other nodes running this feature then stop seeing this copy and may pin it themselves, which is what the feature is meant to prevent.
Could the checker take DHTProvider and call StartProviding instead? That respects Provide.Enabled, since the noop provider does nothing when providing is off, and it keeps the CID in the reprovide schedule whatever Provide.Strategy sweeps, so the record does not lapse.
If providing is disabled, this feature cannot really do its job, because other nodes have no way to see this copy. Saying that at startup seems kinder than announcing behind the operator's back, breaking the user expectations.
On-demand pinning is not landing in v0.43. Review is open and the design needs changes first, so park the highlight in vFUTURE.md and restore v0.43.md to the upstream skeleton.
3 participants
Summary
Automatically pin content when DHT provider counts fall below a configurable replication target, and unpin once
replication has been above target for a grace period.
Helps keeping critical data around, without wasting storage on overly replicated CIDs.
The feature is described in this draft: ipfs/specs#532
The feature is gated behind
Experimental.OnDemandPinningEnabled.CLI commands for
ipfs pin ondemand:add-- register CIDs for on-demand pinningrm-- deregister and unpinls-- list registered CIDsDesign
pins to avoid accidental deletion. This implementation uses boxo's pin name
field (
"on-demand") for that.StorageMax * StorageGCWatermark.without receiving new blocks (allows large downloads while skipping dead records).
susceptible to Sybil manipulation. Documented as a known limitation.
Configuration Parameters (TBD)
OnDemandPinning.ReplicationTarget5OnDemandPinning.CheckInterval"10m"OnDemandPinning.UnpinGracePeriod"24h"Tests
Unit tests for the checker and store in
ondemandpin/.Visualization of Feature
(out of scope for this CLI PR, but helpful to get the idea)
