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) <noreply@anthropic.com>
This commit is contained in:
parent
7aac018c00
commit
33c4f28c6a
3 changed files with 39 additions and 1 deletions
|
|
@ -73,7 +73,12 @@ static void handle_endpoint_error(endpoint_t* ep)
|
||||||
if (priv->u.idx >= 0) {
|
if (priv->u.idx >= 0) {
|
||||||
handle_worker_leave(c, priv->u.idx);
|
handle_worker_leave(c, priv->u.idx);
|
||||||
} else {
|
} 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;
|
break;
|
||||||
case HostRole_scheduler:
|
case HostRole_scheduler:
|
||||||
|
|
|
||||||
|
|
@ -130,6 +130,21 @@ static int handle_worker_join(cluster_t* c, endpoint_t* ep,
|
||||||
if (rc != 0) {
|
if (rc != 0) {
|
||||||
WARN("%s commit barrier failed for %s, rc %d", __func__,
|
WARN("%s commit barrier failed for %s, rc %d", __func__,
|
||||||
from->name, rc);
|
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;
|
idx = rc;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -873,9 +873,27 @@ static void test_hostctrl_unknown_host_error_reply_immediate(void)
|
||||||
|
|
||||||
static void test_worker_join_withheld_acks_replies_error(void)
|
static void test_worker_join_withheld_acks_replies_error(void)
|
||||||
{
|
{
|
||||||
|
worker_t *w = NULL;
|
||||||
|
|
||||||
cbrp_setup(300);
|
cbrp_setup(300);
|
||||||
cbrp_arrange_worker_join("workerA");
|
cbrp_arrange_worker_join("workerA");
|
||||||
cbrp_drive_withheld(handle_join, &g_join_req, 300);
|
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();
|
cbrp_teardown();
|
||||||
printf("test_worker_join_withheld_acks_replies_error: PASS\n");
|
printf("test_worker_join_withheld_acks_replies_error: PASS\n");
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue