I maintain a small leader election library for .NET that runs on Consul. Last week I
went back to it after a long time away and found that a node which lost its Consul
session would keep believing it was the leader. Not for a while. Forever.
All 46 unit tests were green.
What the code did
The election loop acquires a KV key with a session attached, over and over:
var acquired = await _consul.KV.Acquire(pair, cancellationToken);
if (acquired.Response && !_isLeader)
{
_isLeader = true;
await RaiseLeadershipAcquiredEvent();
}
else if (!acquired.Response && _isLeader)
{
_isLeader = false;
await RaiseLeadershipLostEvent();
}
Read it and it seems right. Stop holding the lock, Acquire comes back false, flag goes
down, event fires.
That is not what Consul does. If you try to acquire a key with a session that no longer
exists, Consul does not tell you "no". It returns HTTP 500 with the body
invalid session "...", and Consul.NET turns that into a thrown
ConsulRequestException.
So execution never reaches the else if. It lands here:
catch (Exception ex)
{
_logger.LogError(ex, "Error in leader election loop");
await Task.Delay(TimeSpan.FromSeconds(1), cancellationToken);
}
Log, wait a second, retry with the same dead session. Round and round.
_isLeader stays true. OnLeadershipLost never fires. Meanwhile another node acquires
the lock perfectly correctly and starts doing the work. Two nodes now think they lead,
and the first one has no path back, because nothing in that loop ever creates a new
session.
I want to be clear that Consul behaved correctly at every step here. So did the other
node. The only thing wrong was my code's model of what a failed acquisition looks like.
Why the tests said nothing
Every test in the suite mocked IConsulClient. The one covering this exact path looked
like this:
_kvEndpointMock
.Setup(x => x.Acquire(It.IsAny<KVPair>(), It.IsAny<CancellationToken>()))
.ThrowsAsync(new Exception("First attempt failed"));
A bare Exception. Then it asserted that something got logged, which it did.
The mock was never going to catch this, because writing a mock that returns
ConsulRequestException(500, "invalid session") requires already knowing that is what
Consul returns. If I had known, I would have written the branch correctly in the first
place. The mock recorded my belief about the dependency and then confirmed it back to
me.
I found the bug by writing a test that starts a real Consul in a container, takes
leadership, destroys the session from the outside, and asserts the node notices. It went
red on the first run.
That test now has friends. There are four more that stand up three Consul servers with
real Raft and kill the leader, and those found things too.
The part that isn't a bug
Fixing the exception handling took about ten lines. It also fixed nothing important,
because the API was still this:
Task<bool> IsLeaderAsync();
Everyone uses it the same way:
if (await _leaderElection.IsLeaderAsync())
{
await DoTheWork();
}
That boolean describes a moment that has already passed. Between the check and the work,
this instance's session can expire. A GC pause longer than the session TTL will do it.
Another node gets leadership. Both nodes run DoTheWork().
Checking a second time inside the loop does not help. A process that is paused cannot
check anything, which is the whole problem.
There is no implementation of that signature that avoids this. The race is in the shape
of the method.
Kleppmann was right and it applies to you too
Martin Kleppmann wrote
How to do distributed locking
in 2016. It was aimed at Redlock, but the argument has nothing to do with Redis.
Summarised in my words, not his: a client holding a lock gets paused for longer than the
lease. GC, page fault, scheduler, hypervisor, does not matter. The lock expires. Someone
else takes it. The paused client wakes up and writes, still believing it holds a lock it
lost thirty seconds ago.
Read the original. The diagrams do more than any paragraph will.
The uncomfortable conclusion is that the lock service is not the thing that can fix
this. Consul did everything right in my bug. So did etcd and ZooKeeper in the equivalent
version of it. The client is the problem, and the client is asleep.
What actually works is making the resource reject stale writers. For that it needs a
number from the lock that only ever goes up. A fencing token.
What that looks like
Consul hands you one without asking. The ModifyIndex of the lock key comes from the
Raft log index, and Raft indices only grow, so every new leader sees a strictly higher
number than every leader before it.
So the API stops being a boolean and becomes a lease you hold:
await using var lease = await _leases.AcquireLeadershipAsync(stoppingToken);
using var work = CancellationTokenSource.CreateLinkedTokenSource(
lease.LostToken, stoppingToken);
while (!work.IsCancellationRequested)
{
await _store.CloseBatchAsync(lease.FencingToken, work.Token);
await Task.Delay(TimeSpan.FromSeconds(5), work.Token);
}
And then the half that gets skipped, which is where the safety actually comes from:
UPDATE invoice_batches
SET status = 'closed', fencing_token = @token
WHERE id = @id
AND fencing_token <= @token;
var rows = await _db.ExecuteAsync(Sql, new { id, token = fencingToken });
if (rows == 0)
{
throw new FencedOutException(fencingToken);
}
WHERE fencing_token <= @token is the entire idea. Without it you have an advisory lock
and a race. With it, the sleepy node's write bounces off the database, because the
database has already seen a bigger number from someone else.
Notice what it does not do. It does not stop the stale node from trying. It makes the
attempt land nowhere, which turns out to be enough.
Two states
The loop this leads to has two states and no third. Either you are holding a lease and
working under a token that dies when the lease does, or you are not holding one and you
wait.
"Am I the leader?" is not a question you get to ask, because any answer is out of date
by the time you act on it.
That is a smaller library than the one I started with. IsLeaderAsync is gone. So is the
campaign loop, and so are the OnLeadershipAcquired / OnLeadershipLost events, which
had the same problem wearing a nicer outfit: a handler that gets told it is now the
leader has nothing to hand downstream.
What I put in the README instead
There is now a section in it called What this does not guarantee, and it opens with
this:
Two instances never run leader work concurrently. They can.
During the pause window the old leader's cancellation token has not fired and its code
is running. That is true of this library and every other one. What I can promise is
narrower: the old leader's writes carry a lower fencing token than the new leader's, so
a resource that checks will refuse them.
Writing that down was the most useful thing in the whole release. A library that claims
mutual exclusion is lying, and the lie is dangerous exactly because it is comforting. If
I tell you where my guarantee stops, you get to decide what to put on the other side of
the line.
When the resource can't take a token
Sometimes it can't. A third-party API with no conditional write. A filesystem. A shell
command.
Best option is to put something in front of it that can fence. A database row, a
compare-and-set in your coordination store, a conditional write in object storage.
Failing that, make the operation idempotent so running it twice is boring.
Failing that, accept the risk on purpose, work out how long your pause window actually
is, and write down somewhere that you accepted it. That last one is not a cop-out. It is
the difference between a known risk and a surprise.
There is no fourth option where the lock by itself makes it safe.
The library is DLeader.Consul. MIT,
net8/9/10. It is small and I am the only person working on it, so apply the usual
caution you would to any young dependency.
Honestly though, the library is the least interesting part. The two things worth taking
away work anywhere: mocks of an external system will agree with whatever you already
believe about it, and a lock without a fencing token is a suggestion.
Top comments (0)