feat(provider/keystore): alt datastore wipe#1233
Open
guillaumemichel wants to merge 5 commits intomasterfrom
Open
feat(provider/keystore): alt datastore wipe#1233guillaumemichel wants to merge 5 commits intomasterfrom
guillaumemichel wants to merge 5 commits intomasterfrom
Conversation
guillaumemichel
force-pushed
the
keystore/alt-datastore-wipe
branch
from
97ce04b to
dd1e876
Compare
This was referenced Feb 13, 2026
callers that need a prefix should wrap the datastore with namespace.Wrap before passing it to WithDatastore.
lidel
requested changes
Feb 13, 2026
Comment on lines
522
to
527
| @@ -446,6 +525,23 @@ func (s *ResettableKeystore) Close() error { | |||
| if err = s.ds.Sync(context.Background(), sizeKey); err != nil { | |||
| return fmt.Errorf("error syncing size on close: %w", err) | |||
| } | |||
Member
There was a problem hiding this comment.
hm.. late on my end but if persistSize or Sync fails here, the early return will skip closing the factory-owned datastores at lines 530-543, and leak
perhaps wrap the factory close block in a defer at the top of the default branch, so it runs regardless of the early returns
Collaborator
Author
|
There are 3 namespaces in used in the Updated so that the meta datastore isn't its own dedicated datastore, but has to be supplied to the constructor (to avoid a dedicated datastore file for storing 2 values). |
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
3 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.
Context
The
ResettableKeystorehas a datastore where it stores keys that should be advertised to the DHT swarm. Its reset process takes a chan of the keys that should replace the existing ones, write them to a new datastore, and once it has read all the keys atomically swaps with the datastore previously in use. After the swap, the old datastore is cleared, all keys are individually removed.Some datastore implementations (such as leveldb or pebble) don't reclaim the space straight away and simply add tombstones so that removed keys are collected during compaction at a later point. This leads to high disk usage since the swap process increases disk footprint by the keystore size at each reset operation, until compaction happens (usually on process shutdown).
This problem is tracked in ipfs/kubo#11096
Kubo companion PR ipfs/kubo#11198
Proposed fix
Instead of having a single datastore and use
namespace.Wrap()to create the temporary datastore used for swap, we create and destroy datastores whenever needed. This allows reclaiming disk space as soon as the reset operation is over by removing the corresponding directory.The main change is that the
ResettableKeystorecan be supplied with a datastore factory (createanddestroyfunctions) instead of the datastore.The old behavior remains possible, but it is now possible to use datastore factory for optimized disk footprint.