From 33c4f28c6a58420b4e065a210ba57d641c19f94b Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sun, 27 Sep 2026 05:48:49 +0000 Subject: [PATCH] fix(controller): a worker join that fails its commit barrier leaves nothing bound (bug-1728) "ERROR incorrect worker idx -1" on every node after a worker-join commit timeout (during a PreVote/election) was the visible end of a real dangling pointer. init_worker/rejoin_worker bind the endpoint to the worker (apply_join_state: w->ep = ep, UNAVAIL cleared) before the commit barrier; on barrier failure handle_worker_join returned without setting priv->u.idx, so when that connection closed handle_endpoint_error saw idx -1, logged the ERROR and never ran handle_worker_leave -- the worker kept w->ep pointing at the freed endpoint, available for dispatch, until it rejoined. - handle_worker_join: on barrier failure, clear w->ep (if still this endpoint) and mark the worker unavailable, as if the join never happened; the worker retries its join. - handle_endpoint_error: an idx -1 worker endpoint is now the legitimate "closed before its join completed" case -- WARN, nothing to clean up. test_worker_join_withheld_acks_replies_error also asserts the worker is unbound and unavailable after the -ETIMEDOUT reply (RED before). Co-Authored-By: Claude Opus 5.5 (1M context) --- src/controller/cluster.c | 7 ++++++- src/controller/join.c | 15 +++++++++++++++ .../testsuite/test_commit_barrier_reply.c | 18 ++++++++++++++++++ 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/controller/cluster.c b/src/controller/cluster.c index 8d4479bf..0b6af62b 100644 --- a/src/controller/cluster.c +++ b/src/controller/cluster.c @@ -73,7 +73,12 @@ static void handle_endpoint_error(endpoint_t* ep) if (priv->u.idx >= 0) { handle_worker_leave(c, priv->u.idx); } else { - ERROR("incorrect worker idx %d", priv->u.idx); + /* bug-1728: a worker connection whose join never completed + * (e.g. its commit barrier timed out during an election) + * has nothing bound to it -- handle_worker_join undoes the + * binding on failure -- so there is nothing to clean up. */ + WARN("worker endpoint %p closed before its join completed", + ep); } break; case HostRole_scheduler: diff --git a/src/controller/join.c b/src/controller/join.c index 3eb25ed1..f81b9d5a 100644 --- a/src/controller/join.c +++ b/src/controller/join.c @@ -130,6 +130,21 @@ static int handle_worker_join(cluster_t* c, endpoint_t* ep, if (rc != 0) { WARN("%s commit barrier failed for %s, rc %d", __func__, from->name, rc); + /* bug-1728: init_worker/rejoin_worker already bound this + * endpoint to the worker (apply_join_state: w->ep = ep, UNAVAIL + * cleared). The join failed and priv->u.idx stays -1, so this + * connection's teardown will never run handle_worker_leave -- + * undo the binding now, as if the join never happened (the + * worker retries), rather than leave w->ep to dangle once the + * endpoint is freed. */ + if (w != NULL) { + lock(w, RTTI_LOCK_WRITE); + if (w->ep == ep) { + w->ep = NULL; + mark_worker_unavail(c, w); + } + unlock(w); + } idx = rc; } } diff --git a/src/controller/testsuite/test_commit_barrier_reply.c b/src/controller/testsuite/test_commit_barrier_reply.c index 48fe83d0..55b5664a 100644 --- a/src/controller/testsuite/test_commit_barrier_reply.c +++ b/src/controller/testsuite/test_commit_barrier_reply.c @@ -873,9 +873,27 @@ static void test_hostctrl_unknown_host_error_reply_immediate(void) static void test_worker_join_withheld_acks_replies_error(void) { + worker_t *w = NULL; + cbrp_setup(300); cbrp_arrange_worker_join("workerA"); cbrp_drive_withheld(handle_join, &g_join_req, 300); + + /* bug-1728: a join whose commit barrier failed must not leave the + * worker record bound to the requesting endpoint. priv->u.idx stays + * -1 on that connection, so its teardown never runs + * handle_worker_leave -- a still-set w->ep would dangle once the + * endpoint is freed. */ + lock(g_hc.workermgr, RTTI_LOCK_READ); + key_find(g_hc.workermgr, (void *)"workerA", (void **)&w); + unlock(g_hc.workermgr); + if (w != NULL) { + TEST_ASSERT_NULL_MESSAGE(w->ep, + "bug-1728: a failed join must not leave w->ep bound to the " + "requesting endpoint"); + TEST_ASSERT_TRUE_MESSAGE(w->host.status & XCE_WORKER_UNAVAIL, + "bug-1728: a worker whose join failed must stay unavailable"); + } cbrp_teardown(); printf("test_worker_join_withheld_acks_replies_error: PASS\n"); }