{{ message }}
Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Lease failures lack reliable retry and ownership handling, which can remove nodes from discovery or leave cluster joins improperly locked.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds lease keep-alive and recovery handling to the etcd v3 peer-discovery client.
Changes:
- Tracks and revokes node and lock leases
- Re-registers node keys after expired leases
- Extends call timeouts and adds unit coverage
File summaries
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Lease workers and ownership state are not safely handled across disconnections, registration failures, and client restarts.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 7
- Review effort level: Balanced
Zerpet
force-pushed
the
etcd-peer-discovery-lock-lease-keepalive
branch
2 times, most recently
from
September 14, 2026 15:46
d085043 to
2ed95e6
Compare
The etcd v3 peer discovery client granted a lease for the node's registration key and for the peer discovery lock, but never started or maintained a keep-alive for either. Both leases would lapse at their TTL regardless of whether the node was still up, so a node could silently drop out of discovery, or the registration lock could be held by a client that no longer needed it, until the TTL expired. Reconnects made this worse: eetcd_lease renews the node key lease on its own across a reconnect, but nothing re-PUT the node key under a freshly granted lease when the old one had actually lapsed, so a node recovering from an outage longer than the lease TTL stayed missing from discovery even though it had reconnected. This starts and tracks a keep-alive for both leases, revokes a lease once it is no longer needed (a released lock, an unregistered node, or a fresh lease discarded because the key PUT under it failed), and re-PUTs the node key on reconnect when its lease had actually lapsed. Call timeouts for lock/1, unlock/1 and register/1 are widened to bound the extra work this now does before replying.
The test asserted that granting a lease against the unregistered
?MODULE connection name raises, but that only held when the test ran
in isolation. Running the full test target starts system_SUITE first
in the same VM, which registers a real connection under the same
name and leaves eetcd's connection registry in a state where the
same grant call returns {error, eetcd_conn_unavailable} instead of
raising, failing the assertion.
Mock eetcd_lease as the sibling regression tests already do, and
assert on the regrant's actual effect (a new lease id and keep-alive
pid) instead of on an environment-dependent exception.
Zerpet
force-pushed
the
etcd-peer-discovery-lock-lease-keepalive
branch
from
September 17, 2026 11:32
2ed95e6 to
984fd25
Compare
Zerpet
marked this pull request as ready for review
September 17, 2026 11:36
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.
Proposed Changes
The etcd v3 peer discovery client granted a lease for the node's registration key and for the peer discovery lock, but never started or maintained a keep-alive for either. Both leases would lapse at their TTL regardless of whether the node was still up, so a node could silently drop out of discovery, or the registration lock could be held by a client that no longer needed it, until the TTL expired.
Reconnects made this worse: eetcd_lease renews the node key lease on its own across a reconnect, but nothing re-PUT the node key under a freshly granted lease when the old one had actually lapsed, so a node recovering from an outage longer than the lease TTL stayed missing from discovery even though it had reconnected.
This starts and tracks a keep-alive for both leases, revokes a lease once it is no longer needed (a released lock, an unregistered node, or a fresh lease discarded because the key PUT under it failed), and re-PUTs the node key on reconnect when its lease had actually lapsed. Call timeouts for lock/1, unlock/1 and register/1 are widened to bound the extra work this now does before replying.
Types of Changes
What types of changes does your code introduce to this project?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply.You can also fill these out after creating the PR.
This is simply a reminder of what we are going to look for before merging your code.
CONTRIBUTING.mddocumentFurther Comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution
you did and what alternatives you considered, etc.