Skip to content

fail an rpc with no leader instead of hanging forever - #1109

Open
timgit wants to merge 1 commit into
electric-sql:mainfrom
timgit:fix/worker-leader-change-hang
Open

timgit wants to merge 1 commit into
electric-sql:mainfrom
timgit:fix/worker-leader-change-hang

Conversation

@timgit

@timgit timgit commented Sep 16, 2026

Copy link
Copy Markdown

The bug

An rpc is posted to pglite-tab:${tabId}, which only the leader listens on, and only after the
tab-here handshake. BroadcastChannel does not buffer, so a call posted while the tab is
disconnected — between leader-here and the new leader's connected — reaches nobody. Nothing
replies to it, and the leader-change listener it installs cannot reject it either: the leader
change that disconnected the tab was dispatched before that listener existed.

_runExclusiveQuery and _runExclusiveTransaction do exactly that from their finally:

await this.#rpc('_acquireTransactionLock')
try {
  return await fn()
} finally {
  await this.#rpc('_releaseTransactionLock') // nobody is listening yet
}

A statement merely queued on the lock is fine — its _acquireTransactionLock rejects before the
try. A statement that held the lock is not: its own rpc rejects, the finally then awaits a
release nobody will answer, and that await swallows the rejection. The caller hangs forever with no
error. query() and exec() are themselves wrapped in _runExclusiveTransaction, so any statement
can hit this, not just an explicit transaction().

Both locks are affected, and for a plain query() it is the query lock that strands first:
query() wraps _runExclusiveTransaction around #runQuery, which wraps _runExclusiveQuery
around the protocol calls, so the inner _releaseQueryLock is the release that goes unanswered.

A tab promoted to leader escapes by luck: its worker posts leader-now, dispatching a second
leader-change that rejects the stuck release. Only a tab that stays a follower hangs, which takes
three instances to reproduce — presumably why this has gone unnoticed.

The fix

#rpc reports the indeterminate state rather than posting into a channel nobody is listening on:

if (!this.#connected) {
  throw new LeaderChangedError()
}

Callers already handle LeaderChangedError from the queued case. Paths behind _checkReady() are
unaffected in the normal case — they wait for a connection before their first rpc.

Test

tests/targets/web/leader-change.test.web.js, picked up by pnpm test:web and CI through the
existing glob. No second tab needed: navigator.locks is origin scoped and shared with dedicated
workers, so several PGliteWorkers sharing an id contend for one election lock. The test starts a
SELECT pg_sleep(2) on the third instance — the one that stays a follower, since locks are granted
in request order — closes the leader, and asserts the statement settles. Reports hung on main,
rejects with Leader changed, pending operation in indeterminate state with the fix.

The warm-up SELECT 1 before that statement is load bearing. The leader's PGlite boots lazily, and
until it is ready every rpc sits queued on _acquireTransactionLock — the benign case that rejects
before the try. Without the warm-up the statement is still queued 500ms in, and the test passes on
main as well, testing nothing. Checked both directions against the built worker: with the guard
stripped out the test fails with hung, with it in place the statement rejects about 2s in.

Also ran pnpm test:basic (277 passed; utils and exec-protocol fail to load on
@electric-sql/pg-protocol in my workspace, both before and after) and the Chromium web targets
(20 passed, opfs-ahp among them). Firefox and WebKit left to CI.

Notes

Found from pg-boss, where a BEGIN … COMMIT through its PGlite
adapter stalled a maintenance cycle permanently after a leader change.

One judgement call worth your input: an rpc issued while disconnected now fails fast rather than
waiting for the new leader. Right for anything spanning a leader change, but a fresh call that
merely raced the reconnect window is also rejected and has to be retried. Happy to make #rpc wait
for connected instead, though it cannot be transparent: nothing in a call says which leader its
operation belongs to, so #rpc cannot tell a fresh call from one the dead leader started.

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.

1 participant