fix(libxce): bug-1425 -- populate jobsubmit's *jobid on idempotent-replay success
xcecli_jobsubmit() could return rc>=0 (success) while leaving its *jobid output parameter at 0, reproducible under concurrent client load immediately after a fresh single-node election (~1-3/9 clients per round in the original repro). Root cause: XCECLI_FAILOVER_CALL's idempotent-replay rule (client_failover.c's xce_failover_classify) normalizes a retried resend that hits the server's EEXIST/inflight-collision rejection into overall success -- but that rejection reply carries no payload (handle_jobsubmit's EEXIST branches reply with RequestReply_data_nodata, job.c), so xce_jobsubmit() never populates *jobid on the winning attempt, nor did it on the earlier failing attempt (raw status was already negative). Adds jobsubmit_finalize_jobid() (client_internal.h/client.c), called from xcecli_jobsubmit() right after XCECLI_FAILOVER_CALL returns. Sets *jobid = submit->jobid (the client-generated id already on the wire -- the controller commits/echoes this exact value on every genuine success reply, bug-786/job.c:1439) whenever the overall call succeeded, otherwise leaves it untouched. Placed at the jobsubmit-specific wrapper layer, not the generic classify/decide layer, which stays fully generic. 3 new socket-free unit tests in test_libxce_retry.c (following this codebase's direct-state-simulation style for I/O-adjacent logic). Live-validated: full ASAN rebuild clean, libxce test suite 1/1 OK with all 3 new tests passing, and a 5-round/45-client concurrent-submit repro against a live single-node cluster showed 0 "no clean ack" failures (vs. 1-3/9 per round pre-fix). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
b33f8b68a6
commit
8a6c339a55
3 changed files with 95 additions and 2 deletions
|
|
@ -323,6 +323,15 @@ int xcecli_jobdispatch(xcecli_ctx_t c, xce_job_dispatch_t* info)
|
|||
xce_jobdispatch(&(ctx->ctx), info));
|
||||
}
|
||||
|
||||
/* bug-1425: see the doc comment on the declaration in client_internal.h for
|
||||
* the full mechanism this closes. */
|
||||
void jobsubmit_finalize_jobid(uint64_t* jobid, uint64_t submitted_jobid,
|
||||
int overall_rc)
|
||||
{
|
||||
if (jobid != NULL && overall_rc >= 0)
|
||||
*jobid = submitted_jobid;
|
||||
}
|
||||
|
||||
int xcecli_jobsubmit(xcecli_ctx_t c, xce_job_submit_t* submit, uint64_t* jobid)
|
||||
{
|
||||
api_ctx_t* ctx = (api_ctx_t*)c;
|
||||
|
|
@ -331,8 +340,10 @@ int xcecli_jobsubmit(xcecli_ctx_t c, xce_job_submit_t* submit, uint64_t* jobid)
|
|||
return -EINVAL;
|
||||
}
|
||||
|
||||
return XCECLI_FAILOVER_CALL(ctx, /*is_write=*/true,
|
||||
xce_jobsubmit(&(ctx->ctx), submit, jobid));
|
||||
int rc = XCECLI_FAILOVER_CALL(ctx, /*is_write=*/true,
|
||||
xce_jobsubmit(&(ctx->ctx), submit, jobid));
|
||||
jobsubmit_finalize_jobid(jobid, submit->jobid, rc);
|
||||
return rc;
|
||||
}
|
||||
|
||||
int xcecli_joblist(xcecli_ctx_t c, xce_job_t*** jobs)
|
||||
|
|
|
|||
|
|
@ -301,6 +301,44 @@ bool xcecli_ff_needs_reconnect(const api_ctx_t* ac);
|
|||
_ff_rc; \
|
||||
})
|
||||
|
||||
/*
|
||||
* jobsubmit_finalize_jobid (bug-1425) — guarantee xcecli_jobsubmit()'s *jobid
|
||||
* output param is populated on every overall success, including one the
|
||||
* XCecliRetryLoop's idempotent-replay rule (5.2, xce_failover_classify)
|
||||
* produced by normalizing an EEXIST/E_XCE_DUP_JOB reply from a RETRIED
|
||||
* resend into rc == 0.
|
||||
*
|
||||
* The gap: that normalization happens in the generic, type-erased
|
||||
* xcecli_retry_decide() layer, one level above xce_jobsubmit() (xceapi.c) —
|
||||
* which only ever copies the reply's jobid into *jobid on ITS OWN raw
|
||||
* non-negative status. The dup-rejection reply itself carries NO payload
|
||||
* (handle_jobsubmit's EEXIST/inflight-collision branches reply with
|
||||
* RequestReply_data_nodata, job.c), so *jobid is never written on the
|
||||
* winning (dup) attempt, and was never written on the earlier failing
|
||||
* attempt either (whose raw status was already negative). Caller-visible
|
||||
* result: xcecli_jobsubmit() returns 0 (success) with *jobid left at
|
||||
* whatever the caller initialized it to — reproduced live via a concurrent
|
||||
* 9-client submit burst immediately after a fresh single-node election
|
||||
* (~10.0-10.1s: one full 10s cbuf_timedget deadline on the failing first
|
||||
* attempt, then a fast dup-rejection reply on the resend).
|
||||
*
|
||||
* The job a successful submission belongs to is unambiguously the one THIS
|
||||
* client generated and put on the wire in the first place (submit->jobid —
|
||||
* job submission ids are client-generated; the controller commits/echoes
|
||||
* this exact value verbatim on every genuine success reply, bug-786,
|
||||
* job.c:1439's `jobid = submit->jobid`). So on ANY overall success this is
|
||||
* always the correct id to report, regardless of which attempt in the retry
|
||||
* sequence actually produced the winning rc. No socket/reply-decode
|
||||
* dependency — pure, and exposed non-static so it is unit-testable without
|
||||
* a live connection (test_libxce_retry.c).
|
||||
*
|
||||
* Called by xcecli_jobsubmit() (client.c) immediately after
|
||||
* XCECLI_FAILOVER_CALL returns. A no-op (jobid left untouched) when
|
||||
* overall_rc < 0 (the call genuinely failed) or jobid is NULL.
|
||||
*/
|
||||
void jobsubmit_finalize_jobid(uint64_t* jobid, uint64_t submitted_jobid,
|
||||
int overall_rc);
|
||||
|
||||
/*
|
||||
* xcecli_invoke_rejoin (task 4.1) — invoke the consumer rejoin hook once after a
|
||||
* successful reconnect/recovery and BEFORE the interrupted op resumes. Socket-
|
||||
|
|
|
|||
|
|
@ -1027,6 +1027,47 @@ static void test_decide_dup_on_resent_write_is_success(void)
|
|||
printf("test_decide_dup_on_resent_write_is_success: PASS\n");
|
||||
}
|
||||
|
||||
/* bug-1425: the test above proves xcecli_retry_decide() can return rc == 0
|
||||
* (success) on a DUP-replay WITHOUT any side channel for the underlying
|
||||
* op's output data (e.g. a job's id) — the classify/decide layer is
|
||||
* generic and knows nothing about jobid. jobsubmit_finalize_jobid() is the
|
||||
* jobsubmit-specific repair xcecli_jobsubmit() applies right after
|
||||
* XCECLI_FAILOVER_CALL returns, so that gap never reaches the caller. */
|
||||
static void test_jobsubmit_finalize_populates_jobid_on_success(void)
|
||||
{
|
||||
/* Simulates the reported bug exactly: no attempt in the retry sequence
|
||||
* ever wrote *jobid (it is still the caller's initial 0), yet the
|
||||
* overall call succeeded (rc == 0, e.g. via the DUP-replay rule). */
|
||||
uint64_t jobid = 0;
|
||||
jobsubmit_finalize_jobid(&jobid, /*submitted_jobid=*/424242ULL,
|
||||
/*overall_rc=*/0);
|
||||
TEST_ASSERT_EQUAL_UINT64(424242ULL, jobid);
|
||||
|
||||
printf("test_jobsubmit_finalize_populates_jobid_on_success: PASS\n");
|
||||
}
|
||||
|
||||
/* A genuine failure must never be masked by inventing a jobid — the caller's
|
||||
* *jobid is left exactly as xce_jobsubmit() last set it (untouched on a
|
||||
* negative overall rc, by xce_jobsubmit's own `if (status >= 0)` guard). */
|
||||
static void test_jobsubmit_finalize_leaves_jobid_untouched_on_failure(void)
|
||||
{
|
||||
uint64_t jobid = 0;
|
||||
jobsubmit_finalize_jobid(&jobid, 424242ULL, -ETIMEDOUT);
|
||||
TEST_ASSERT_EQUAL_UINT64(0ULL, jobid);
|
||||
|
||||
printf("test_jobsubmit_finalize_leaves_jobid_untouched_on_failure: PASS\n");
|
||||
}
|
||||
|
||||
/* xcecli_jobsubmit() is called with a NULL *jobid by no real caller today,
|
||||
* but the finalize step must not assume otherwise -- it runs unconditionally
|
||||
* after every call. */
|
||||
static void test_jobsubmit_finalize_null_jobid_safe(void)
|
||||
{
|
||||
jobsubmit_finalize_jobid(NULL, 424242ULL, 0); /* must not crash */
|
||||
|
||||
printf("test_jobsubmit_finalize_null_jobid_safe: PASS\n");
|
||||
}
|
||||
|
||||
/* Req 3.4 (D6): a transport-only cause that outlasts the budget keeps the LAST
|
||||
* transport rc (not -E_XCE_NOT_READY — that mapping is reserved for no-primary
|
||||
* exhaustion). Proves the two exhaustion causes are distinguished. */
|
||||
|
|
@ -1737,6 +1778,9 @@ void test_libxce_retry(void)
|
|||
RUN_TEST(test_decide_backoff_grows_and_is_bounded);
|
||||
RUN_TEST(test_decide_terminal_surfaces_unchanged);
|
||||
RUN_TEST(test_decide_dup_on_resent_write_is_success);
|
||||
RUN_TEST(test_jobsubmit_finalize_populates_jobid_on_success);
|
||||
RUN_TEST(test_jobsubmit_finalize_leaves_jobid_untouched_on_failure);
|
||||
RUN_TEST(test_jobsubmit_finalize_null_jobid_safe);
|
||||
RUN_TEST(test_decide_transport_exhaustion_keeps_last_rc);
|
||||
RUN_TEST(test_decide_known_dead_ctx_retries_not_terminal);
|
||||
RUN_TEST(test_decide_jitter_staggers_many_clients);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue