diff --git a/src/lib/client.c b/src/lib/client.c index 01aec83d..4caeea0d 100644 --- a/src/lib/client.c +++ b/src/lib/client.c @@ -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) diff --git a/src/lib/client_internal.h b/src/lib/client_internal.h index 6484a58d..3341ef55 100644 --- a/src/lib/client_internal.h +++ b/src/lib/client_internal.h @@ -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- diff --git a/src/lib/testsuite/test_libxce_retry.c b/src/lib/testsuite/test_libxce_retry.c index e637817b..41933716 100644 --- a/src/lib/testsuite/test_libxce_retry.c +++ b/src/lib/testsuite/test_libxce_retry.c @@ -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);