internal/db: Remove node from dqlite cluster on failed join#774
internal/db: Remove node from dqlite cluster on failed join#774claudiubelu wants to merge 1 commit into
Conversation
dqlite.Close() tears down Raft connections but does not remove the node from the dqlite cluster membership. When db.Join() fails after the node has already been added to the cluster (via cli.Add in app.run), the reverter only called Close(), leaving a ghost member in dqlite. This ghost node causes CheckMembershipConsistency to fail (the node exists in dqlite but not in core_cluster_members), which blocks token generation and all future cluster joins. Fix the Join reverter to explicitly ask the dqlite leader to remove us before closing. Also correct a misleading comment in the Bootstrap reverter. Signed-off-by: Claudiu Belu <cbelu@cloudbasesolutions.com>
roosterfish
left a comment
roosterfish
left a comment
There was a problem hiding this comment.
Hey, can you please share some reproducer steps how you could end up in this state?
I see we have an exception to continue starting up the database in case we hit a update.ErrGracefulAbort error. But I would be very interested to see what else could go wrong.
Also could you please add the reproducer as an additional check to test_membership_consistency ?
There was a problem hiding this comment.
Pull request overview
Fixes a cluster-join failure mode where a node that was added to dqlite membership but failed later in db.Join() could be left behind as a “ghost” dqlite member, breaking membership consistency checks and blocking future operations like token generation.
Changes:
- Corrected comments clarifying that
dqlite.Close()tears down Raft connections but does not remove a node from dqlite membership. - Updated the
Join()reverter to attempt removal of the local node from the dqlite cluster before closing, to avoid leaving a ghost member behind.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cli, err := db.dqlite.Leader(removeCtx, dqliteClient.WithConcurrentLeaderConns(1)) | ||
| if err != nil { | ||
| db.log().Warn("Failed to connect to dqlite leader to remove ourselves from the cluster", slog.String("address", db.listenAddr.String()), slog.String("error", err.Error())) | ||
| } else { | ||
| err = cli.Remove(removeCtx, db.dqlite.ID()) |
| removeCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| defer cancel() | ||
|
|
||
| cli, err := db.dqlite.Leader(removeCtx, dqliteClient.WithConcurrentLeaderConns(1)) | ||
| if err != nil { |
|
Marking as draft until you get a chance to get back to it @claudiubelu. |
dqlite.Close()tears down Raft connections but does not remove the node from the dqlite cluster membership. Whendb.Join()fails after the node has already been added to the cluster (via cli.Add in app.run), the reverter only calledClose(), leaving a ghost member in dqlite.This ghost node causes
CheckMembershipConsistencyto fail (the node exists in dqlite but not incore_cluster_members), which blocks token generation and all future cluster joins.Fix the Join reverter to explicitly ask the
dqliteleader to remove us before closing. Also correct a misleading comment in the Bootstrap reverter.