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"); }