Skip to content

Only publish added peers after they were persisted - #1150

Open
ajaysehwal wants to merge 1 commit into
lightningdevkit:mainfrom
ajaysehwal:fix-peer-store-unpersisted-peers
Open

ajaysehwal wants to merge 1 commit into
lightningdevkit:mainfrom
ajaysehwal:fix-peer-store-unpersisted-peers

Conversation

@ajaysehwal

@ajaysehwal ajaysehwal commented Oct 11, 2026 •

Copy link
Copy Markdown

Fixes #1142.

PeerStore::add_peer inserted the peer into the in-memory map before awaiting the write. If the write failed, a retry for the same peer hit the existing-entry fast path and returned Ok(()) without writing again. The peer was then reported as stored (e.g., in peer listings), although it would vanish after a restart, losing the automatic reconnection information.

We now stage the updated snapshot, persist it, and only then publish it to the in-memory map, mirroring what remove_peer already does. The in-memory set therefore only ever contains durable entries, so a retry after a failed write persists again.

  • Mutations stay serialized through the existing mutation_lock.
  • If the future is dropped before the publish step, memory is left untouched.
  • If a write committed without a success acknowledgement, memory is left untouched and the retry rewrites the same snapshot, which is idempotent.
  • Unrelated peers are preserved as the snapshot is built from the full current set.

We also reworded the docs of list_peers and get_peer: as changes are now only published after they were persisted, they no longer claim that callers may observe changes that are still being persisted.

Testing

Added regression tests in peer_store.rs:

  • add_peer_is_retried_after_persist_failure: the first write fails and the peer isn't visible, the retry persists it, and a PeerStore restored from the persisted bytes contains the peer.
  • add_peer_does_not_mutate_memory_if_persist_fails: a failing store leaves the in-memory set empty.
  • failed_add_peer_keeps_other_peers_and_retry_persists_all: a failed add leaves existing peers intact, and after the retry the restored store contains both the existing and the new peer.
  • add_peer_retry_is_idempotent_after_unacknowledged_commit: if a write reached the store but wasn't acknowledged, the peer isn't published, and the retry succeeds without duplicating it.

All four fail on the previous implementation and pass with this change. The full lib test suite passes.

AI disclosure

This change was prepared with the assistance of Claude Code.

@ldk-reviews-bot

ldk-reviews-bot commented Oct 11, 2026 •

Copy link
Copy Markdown

I've assigned @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

`PeerStore::add_peer` inserted the peer into the in-memory map before
awaiting the write. If the write failed, a retry for the same peer hit
the existing-entry fast path and returned `Ok(())` without writing, so
the peer was reported as stored although it would vanish after a restart.

Stage the updated snapshot, persist it, and only then publish it to the
in-memory map, mirroring `remove_peer`. The in-memory set now only ever
contains durable entries, so a retry after a failed write persists again.

Adds regression tests for a failed first write followed by a retry
(including restoring the peer from the persisted bytes) and for the
failed write leaving memory untouched.

AI-assisted: this change was prepared with Claude Code.
@ldk-reviews-bot
ldk-reviews-bot requested a review from tnull October 11, 2026 16:24
@ajaysehwal
ajaysehwal force-pushed the fix-peer-store-unpersisted-peers branch from f115e53 to 7efe262 Compare October 11, 2026 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Do not report unpersisted peers as successfully stored

2 participants