From 40c235157836a3bb6efa9851823ce073c80a2212 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Mon, 13 Jul 2026 07:18:52 +0000 Subject: [PATCH] fix(client): bug-1436 count-aware failover macro for xce_get-family calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit xcecli_joblist/reslist/workerlist/resval/jobinfo return a non-negative ROW COUNT on success (rc = getreply.nrows), not a 0/status code. They were wrapped in XCECLI_FAILOVER_CALL, which unconditionally classifies the raw return value against fixed constants (E_XCE_* and POSIX errno magnitudes accepted in either sign) — a row count that happens to equal one of those constants (e.g. exactly 111 jobs == ECONNREFUSED) was silently misclassified as a transport failure, forcing a spurious xce_failover_rotate() on every call with that exact count. In /xce-test-ha's arithmetic progression (3 + 3*round) this hit at round 36, burning the retry budget on self-inflicted churn until it coincided with a genuine transient and surfaced a real ECONNRESET to the caller. Add XCECLI_FAILOVER_CALL_COUNT: only enters the retry/classify loop when EXPR's raw result is negative; any non-negative result (0 or a genuine row count of any magnitude) is unconditionally success. Switch the five xce_get-family wrappers to it. Verified via 40-round sequential and 40-round chaotic HA soak past round 36, zero committed-job loss. --- src/lib/client.c | 34 +++++++++++++++++------- src/lib/client_internal.h | 55 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 10 deletions(-) diff --git a/src/lib/client.c b/src/lib/client.c index 164353a4..976ff89d 100644 --- a/src/lib/client.c +++ b/src/lib/client.c @@ -283,8 +283,9 @@ int xcecli_reslist(xcecli_ctx_t c, xce_resource_t*** res) return -EINVAL; } - return XCECLI_FAILOVER_CALL(ctx, /*is_write=*/false, - xce_reslist(&(ctx->ctx), res)); + /* xce_reslist returns a non-negative ROW COUNT on success, not a 0/status + * code — must go through the count-aware macro (bug-1436). */ + return XCECLI_FAILOVER_CALL_COUNT(ctx, xce_reslist(&(ctx->ctx), res)); } int xcecli_join(xcecli_ctx_t c, int role, hostattrs_t* attrs) @@ -305,8 +306,9 @@ int xcecli_workerlist(xcecli_ctx_t c, xce_worker_t*** workers) return -EINVAL; } - return XCECLI_FAILOVER_CALL(ctx, /*is_write=*/false, - xce_workerlist(&(ctx->ctx), workers)); + /* xce_workerlist returns a non-negative ROW COUNT on success, not a + * 0/status code — must go through the count-aware macro (bug-1436). */ + return XCECLI_FAILOVER_CALL_COUNT(ctx, xce_workerlist(&(ctx->ctx), workers)); } int xcecli_jobdispatch(xcecli_ctx_t c, xce_job_dispatch_t* info) @@ -341,8 +343,14 @@ int xcecli_joblist(xcecli_ctx_t c, xce_job_t*** jobs) return -EINVAL; } - return XCECLI_FAILOVER_CALL(ctx, /*is_write=*/false, - xce_joblist(&(ctx->ctx), jobs)); + /* xce_joblist returns a non-negative ROW COUNT on success, not a 0/status + * code — a count that happens to equal a retriable errno's magnitude + * (e.g. exactly 111 jobs == ECONNREFUSED) would be misclassified as a + * transport failure by the generic macro, forcing a spurious reconnect + * storm that eventually surfaces a real "Connection reset by peer" once + * it coincides with a genuine transient (bug-1436). Must go through the + * count-aware macro instead. */ + return XCECLI_FAILOVER_CALL_COUNT(ctx, xce_joblist(&(ctx->ctx), jobs)); } int xcecli_clusterconf(xcecli_ctx_t c, xce_clusterconf_t** conf) @@ -583,8 +591,9 @@ int xcecli_resval(xcecli_ctx_t c, char* host, char* res, xce_resval_t*** val) return -EINVAL; } - return XCECLI_FAILOVER_CALL(ctx, /*is_write=*/false, - xce_resval(&(ctx->ctx), host, res, val)); + /* xce_resval returns a non-negative ROW COUNT on success, not a 0/status + * code — must go through the count-aware macro (bug-1436). */ + return XCECLI_FAILOVER_CALL_COUNT(ctx, xce_resval(&(ctx->ctx), host, res, val)); } int xcecli_notify(xcecli_ctx_t c, xce_notify_data_t* data) @@ -913,7 +922,12 @@ int xcecli_jobinfo(xcecli_ctx_t c, uint64_t jobid, xce_job_t** job) return -EINVAL; } - rc = XCECLI_FAILOVER_CALL(ctx, /*is_write=*/false, - xce_jobinfo(&(ctx->ctx), jobid, job)); + /* xce_jobinfo returns 1 (found) / 0 (not found) / negative (error) — a + * fixed non-zero success value, not a 0/status code. 1 never happens to + * collide with a recognized retriable magnitude today, but it is the same + * class of bug as bug-1436 (xcecli_joblist) — go through the count-aware + * macro for correctness and consistency rather than relying on that + * coincidence. */ + rc = XCECLI_FAILOVER_CALL_COUNT(ctx, xce_jobinfo(&(ctx->ctx), jobid, job)); return rc; } diff --git a/src/lib/client_internal.h b/src/lib/client_internal.h index 36398ac5..6484a58d 100644 --- a/src/lib/client_internal.h +++ b/src/lib/client_internal.h @@ -171,6 +171,61 @@ bool xcecli_retry_next(api_ctx_t* ac, int* rc, bool is_write, _fo_rc; \ }) +/* + * XCECLI_FAILOVER_CALL_COUNT(ac, EXPR) — sibling of XCECLI_FAILOVER_CALL for + * xce_get-family calls (bug-1436). + * + * xce_get_with_condition (request.c) — and every xcecli_* wrapper built on it + * (joblist, reslist, workerlist, resval, jobinfo) — returns a NON-NEGATIVE ROW + * COUNT on success (`rc = getreply.nrows`), not a fixed 0/status code; a + * negative return is the ONLY failure signal. XCECLI_FAILOVER_CALL assumes + * every wrapped EXPR follows the "0 = success" status-code convention and + * unconditionally feeds the raw return value through xce_failover_classify(), + * which recognizes retriable/terminal outcomes by comparing MAGNITUDE against + * fixed constants (E_XCE_* >= 10001, and POSIX errno values it deliberately + * accepts in either sign — ECONNRESET=104, ETIMEDOUT=110, ECONNREFUSED=111, + * client_failover.c fo_abs()). A joblist/reslist/workerlist/resval reply whose + * row count happens to equal one of those constants (e.g. exactly 111 jobs == + * ECONNREFUSED) is silently misclassified as a transport failure on EVERY + * call with that exact count — forcing a spurious xce_failover_rotate() to the + * NEXT configured master even though the read fully succeeded. Since + * get-class ops are primary-only, landing on a standby correctly (but + * pointlessly) fails with E_XCE_STANDBY and rotates again; the cycle repeats + * for every subsequent same-sized call in the same batch, burning the finite + * max_retries budget (20) on self-inflicted churn until it coincides with a + * genuine transient (e.g. a peer mid-restart after a just-completed election) + * and exhausts the budget while rotating, surfacing a real + * ECONNRESET/ECONNREFUSED to the caller — bug-1436 ("xce_joblist: Connection + * reset by peer" immediately after a successful wait-primary; reproduced live + * at exactly 111 tracked jobs, the first count in the /xce-test-ha soak's + * arithmetic progression — 3 + 3*round — that lands on 111 == ECONNREFUSED). + * + * Fix: only enter the retry/classify loop when EXPR's raw result is NEGATIVE. + * A non-negative result (0 or a genuine row count, of ANY magnitude) is + * unconditionally success and returned as-is — it can never be misread as a + * status code. A dead ctx (XCE_CTX_ERR) is still caught correctly: the per-op + * XCE_CTX_RUN guard in xceapi.c makes EXPR itself return a negative -EINVAL in + * that state, which enters the loop and is reclassified by + * xcecli_retry_decide's dead-connection branch (checked before magnitude + * classification, regardless of the stale *rc it was invoked with). + * + * is_write is hardcoded false: every xce_get-family op is a read. + */ +#define XCECLI_FAILOVER_CALL_COUNT(ac, EXPR) \ + __extension__({ \ + api_ctx_t* _fc_ac = (ac); \ + xcecli_retry_t _fc_st = XCECLI_RETRY_INIT(_fc_ac); \ + int _fc_rc; \ + mylock_lock(&_fc_ac->failover.lock); \ + do { \ + _fc_rc = (EXPR); \ + } while ((_fc_rc < 0) && \ + xcecli_retry_next(_fc_ac, &_fc_rc, /*is_write=*/false, \ + &_fc_st)); \ + mylock_unlock(&_fc_ac->failover.lock); \ + _fc_rc; \ + }) + /* * xcecli_ctx_recover (task 3.2) — repair the inner xce_ctx_t after a successful * reconnect so the next xce_* attempt can proceed. Socket-free (no I/O, no