Repository navigation
Conversation
Adding a peer inserted it into the in-memory set before the write was awaited. If the write failed, a retry found the peer already present and returned success without writing again, so the peer was reported as stored but was gone after a restart. Removing a peer already persisted first, but did so through separate hand-rolled logic. Route both mutations through a single helper that applies the change to a copy of the peer set, persists it, and only then publishes it to readers. The in-memory set can thereby no longer get ahead of the persisted one, and a retry after a failed write persists again. Co-Authored-By: HAL 9000
Removing a peer rewrote the persisted peer set even if the peer was not stored. With a failing store, callers were thereby handed a persistence error for a removal that had nothing to remove, e.g., when a channel to a peer we never stored was closed. Report whether the removal changed the peer set, so that nothing is written, and nothing can fail, if the peer is unknown. Co-Authored-By: HAL 9000
The number of persisted peers was written as a plain `u16`, so a peer set of more than 65535 entries had its count truncated and most of its peers silently dropped on the next restart. The event queue had the same issue, which was fixed by switching to `CollectionLength`. Do the same for the peer store. As `CollectionLength` encodes counts below `u16::MAX` exactly as before, previously persisted peer sets are still read and smaller ones are still written unchanged. As the count read from disk is thereby no longer bounded to `u16::MAX`, we also stop preallocating for it, so that a corrupted count fails to read rather than exhausting memory. Co-Authored-By: HAL 9000
A peer store write that fails or is cancelled may still have reached the store, e.g., if a remote store timed out after applying it. The persisted peer set could then differ from the in-memory one, and as mutations that don't change the in-memory set skip the write, the difference was only repaired by the next unrelated change. A peer the user removed could thereby be reconnected to after a restart. Remember that the outcome of a write is unknown until one succeeds, and write the peer set on the next mutation regardless of whether it changes anything. Co-Authored-By: HAL 9000
|
👋 I see @jkczyz was un-assigned. |
This was referenced Nov 12, 2026
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
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.
Several recent PRs each fixed one symptom of the peer store's persistence
handling. This addresses them at the store level instead:
add_peerandremove_peernow share one mutation path that persiststhe updated peer set before publishing it to readers, so the in-memory
set can no longer get ahead of the persisted one and a retry after a
failed write persists again.
CollectionLength, as Use CollectionLength for the persisted event queue length #1230 did for theevent queue, and we no longer preallocate based on it.
applied by the store), the next mutation rewrites the peer set even if it
changes nothing.
Supersedes #1150 and the peer-store half of #1215. It also replaces the
PeerStorerewrite in #1175, which can be rebased to only addupsert_peerand its call-site changes on top. #1149 needed the #1150 fix and can now be rebased onto this.Closes #1142.