diff --git a/.kiro/specs/libxce-failover/tasks.md b/.kiro/specs/libxce-failover/tasks.md index 53b7bb3..ea32f78 100644 --- a/.kiro/specs/libxce-failover/tasks.md +++ b/.kiro/specs/libxce-failover/tasks.md @@ -29,7 +29,7 @@ - _Requirements: 2.1, 2.2, 2.3, 4.4, 8.4_ - _Boundary: EndpointRotator_ - _Depends: 1.1_ -- [ ] 2.3 (P) Capture redirect on the reply path and wake the wait on endpoint error +- [x] 2.3 (P) Capture redirect on the reply path and wake the wait on endpoint error - In the reply handler, copy the redirect-hint host/port into the client reply context when a redirect reply carries one, before the reply is freed; leave empty otherwise (advisory hint, no sequence correlation) - On endpoint error or peer reset, release any pending reply wait with a transport error so the in-flight call returns instead of hanging - Keep the status flowing through the existing wait-channel path so the public API layer stays unchanged @@ -103,3 +103,4 @@ - 1.1: design said `XCE_MAXHOST_LEN`; real constant is `XCE_MAXHOSTNAME_LEN` (config.h, =256) — design corrected. `api_ctx_t` now in private `client_internal.h`; `xcecli_reconnect_cb` typedef lives there until task 4.1 adds the public setter. Build: `cd src && nix develop --command bash tools/xce-build.sh lib`. - 1.2 (ADAPTED): RED-against-undefined-symbols would break linking of the shared lib test binary across tasks, so 1.2 builds a GREEN reusable harness (`src/lib/testsuite/failover_harness.h`): fake multi-master config+cursor, an 11-row controller-status taxonomy table (resolves to real errnox.h codes), and an N-pthread runner. Component RED→GREEN tests live in 2.1/2.2/2.3/3.1/3.2. The harness defines `fo_expected_action_t` (FO_ACT_*) mirroring the design `xce_fo_action_t` — 2.1 asserts the real classifier against these rows. - 2.2: EndpointRotator `xce_failover_next_target` (pure, socket-free: hint-first/consume, else cursor advance+wrap over conf->masters[]) + thin `xce_failover_rotate` (close-old/reconnect same endpoint). Live connect path is E2E (6.2). LAYERING: functions taking the private `api_ctx_t` are declared in `src/lib/client_internal.h`, NOT the public `xce/client_failover.h` (which keeps only the pure int-typed classifier) — per src/lib/CLAUDE.md (public headers in src/include/xce/ must not pull private state). +- 2.3: RedirectCapture via non-static seam `xce_ctx_capture_redirect_hint()` called in handle_reply BEFORE the capnp free AND before the `status<0` early return (so negative E_XCE_REDIRECT still captures). Bounded strncpy into redirect_hint_host. ReplyWaitWake needed NO production change — existing `handle_disconnect` (err_handler, xcectx.c) already injects -ECONNRESET into wbuf + sets XCE_CTX_ERR; full err_handler path is covered by E2E (6.2). diff --git a/src/include/xcectx.h b/src/include/xcectx.h index a82d268..424c95e 100644 --- a/src/include/xcectx.h +++ b/src/include/xcectx.h @@ -59,4 +59,19 @@ extern int xce_ctx_set_endpoint(xce_ctx_t*, endpoint_t*); extern void xce_block_tasks(xce_ctx_t*); extern void xce_unblock_tasks(xce_ctx_t*); +/* + * xce_ctx_capture_redirect_hint: copy a redirect address carried on a reply + * into the ctx's redirect_hint_host/port fields as a best-effort failover hint + * (Req 2.2, 8.3). Called by handle_reply() BEFORE the capnp-owned reply (and + * its redirect_host string) is freed. + * + * When `host` is NULL/empty or `port` is 0 there is no usable address: the hint + * is left untouched (it stays empty). Otherwise the host is bounded-copied into + * the fixed-size field (truncated + NUL-terminated, never overflowing) and the + * port is stored. A NULL ctx is a safe no-op. Advisory only — correctness never + * depends on the hint. + */ +extern void xce_ctx_capture_redirect_hint(xce_ctx_t*, const char* host, + uint16_t port); + #endif diff --git a/src/lib/testsuite/test_libxce_retry.c b/src/lib/testsuite/test_libxce_retry.c index 3f6bcc6..2c75d7d 100644 --- a/src/lib/testsuite/test_libxce_retry.c +++ b/src/lib/testsuite/test_libxce_retry.c @@ -617,6 +617,150 @@ static void test_next_target_rejects_bad_args(void) printf("test_next_target_rejects_bad_args: PASS\n"); } +/* ════════════════════════════════════════════════════════════════════════ + * Task 2.3 — RedirectCapture (xce_ctx_capture_redirect_hint) RED→GREEN tests. + * + * handle_reply() copies the redirect address carried on a redirect reply into + * the ctx's redirect_hint_host/port BEFORE the capnp-owned reply is freed. The + * copy logic is factored into a non-static seam, xce_ctx_capture_redirect_hint, + * so it can be unit-tested without a live server. + * + * Requirements: 2.2 (capture redirect addr as a best-effort hint), 8.3 (try the + * hinted address first — capture is the prerequisite). + * ════════════════════════════════════════════════════════════════════════ */ + +/* Build a bare xce_ctx_t with an empty redirect hint, without touching any + * iomgr/ssl/endpoint state (capture is pure ctx-field manipulation). */ +static void fo_ctx_clear_hint(xce_ctx_t *ctx) +{ + memset(ctx, 0, sizeof(*ctx)); + ctx->redirect_hint_host[0] = '\0'; + ctx->redirect_hint_port = 0; +} + +/* Req 2.2, 8.3: a present host+port is copied into the ctx hint fields. */ +static void test_capture_redirect_hint_present(void) +{ + xce_ctx_t ctx; + fo_ctx_clear_hint(&ctx); + + xce_ctx_capture_redirect_hint(&ctx, "10.40.0.5", 9300); + + TEST_ASSERT_EQUAL_STRING("10.40.0.5", ctx.redirect_hint_host); + TEST_ASSERT_EQUAL_INT(9300, ctx.redirect_hint_port); + + printf("test_capture_redirect_hint_present: PASS\n"); +} + +/* Req 2.2: when no address is present (host NULL/empty or port 0) the hint is + * left empty — capture must not invent an address. */ +static void test_capture_redirect_hint_absent_stays_empty(void) +{ + xce_ctx_t ctx; + + /* NULL host → hint untouched */ + fo_ctx_clear_hint(&ctx); + xce_ctx_capture_redirect_hint(&ctx, NULL, 9300); + TEST_ASSERT_EQUAL_STRING("", ctx.redirect_hint_host); + TEST_ASSERT_EQUAL_INT(0, ctx.redirect_hint_port); + + /* empty host → hint untouched */ + fo_ctx_clear_hint(&ctx); + xce_ctx_capture_redirect_hint(&ctx, "", 9300); + TEST_ASSERT_EQUAL_STRING("", ctx.redirect_hint_host); + TEST_ASSERT_EQUAL_INT(0, ctx.redirect_hint_port); + + /* port 0 → hint untouched (no usable target) */ + fo_ctx_clear_hint(&ctx); + xce_ctx_capture_redirect_hint(&ctx, "10.40.0.5", 0); + TEST_ASSERT_EQUAL_STRING("", ctx.redirect_hint_host); + TEST_ASSERT_EQUAL_INT(0, ctx.redirect_hint_port); + + printf("test_capture_redirect_hint_absent_stays_empty: PASS\n"); +} + +/* Req 2.2: an over-long host is bounded-copied (NUL-terminated, no overflow); + * the capnp-owned string the controller may send is not length-checked here. */ +static void test_capture_redirect_hint_truncates_long_host(void) +{ + xce_ctx_t ctx; + fo_ctx_clear_hint(&ctx); + + char longhost[XCE_MAXHOSTNAME_LEN * 2]; + memset(longhost, 'h', sizeof(longhost) - 1); + longhost[sizeof(longhost) - 1] = '\0'; + + xce_ctx_capture_redirect_hint(&ctx, longhost, 9400); + + /* copied, NUL-terminated, and never longer than the field bound */ + TEST_ASSERT_EQUAL_INT(XCE_MAXHOSTNAME_LEN - 1, + (int)strlen(ctx.redirect_hint_host)); + TEST_ASSERT_EQUAL_INT('\0', + ctx.redirect_hint_host[XCE_MAXHOSTNAME_LEN - 1]); + TEST_ASSERT_EQUAL_INT(9400, ctx.redirect_hint_port); + + printf("test_capture_redirect_hint_truncates_long_host: PASS\n"); +} + +/* Guard: a NULL ctx is rejected without crashing. */ +static void test_capture_redirect_hint_null_ctx_safe(void) +{ + /* must not crash */ + xce_ctx_capture_redirect_hint(NULL, "10.40.0.5", 9300); + printf("test_capture_redirect_hint_null_ctx_safe: PASS\n"); +} + +/* ════════════════════════════════════════════════════════════════════════ + * Task 2.3 — ReplyWaitWake RED→GREEN test. + * + * On an endpoint error/peer reset, the existing disconnect path injects a + * transport error into ctx->wbuf so a blocked cbuf_get returns instead of + * hanging. This test proves the wake mechanism end-to-end on a real wbuf: a + * thread blocks in cbuf_get, the "disconnect" injects -ECONNRESET, and the + * blocked waiter wakes with exactly that transport error (Req 4.4). + * ════════════════════════════════════════════════════════════════════════ */ + +typedef struct { + cbuf_t wbuf; + int woke_status; /* what the blocked cbuf_get observed */ + int got; /* cbuf_get return code */ +} fo_wake_ctx_t; + +static void fo_wake_waiter(void *p, int idx) +{ + fo_wake_ctx_t *w = (fo_wake_ctx_t *)p; + if (idx == 0) { + /* waiter: block until the disconnect injects an error */ + w->got = cbuf_get(&w->wbuf, &w->woke_status); + } else { + /* injector: mirror handle_disconnect's wbuf error injection. A short + * spin lets the waiter reach cbuf_get first so we exercise the wake, + * not a pre-filled buffer. */ + for (volatile int i = 0; i < 2000000; i++) { + } + int err = -ECONNRESET; + cbuf_tryput(&w->wbuf, &err); + } +} + +/* Req 4.4: a pending cbuf_get is released with the injected transport error so + * an in-flight xce_* call returns instead of hanging across an endpoint reset. */ +static void test_reply_wait_wake_releases_blocked_get(void) +{ + fo_wake_ctx_t w; + memset(&w, 0, sizeof(w)); + TEST_ASSERT_EQUAL_INT(0, cbuf_init(&w.wbuf, 1, sizeof(int))); + + int rc = fo_run_threads(2, fo_wake_waiter, &w); + cbuf_fini(&w.wbuf); + + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_TRUE(w.got >= 0); /* cbuf_get returned, did not hang */ + TEST_ASSERT_EQUAL_INT(-ECONNRESET, w.woke_status); + + printf("test_reply_wait_wake_releases_blocked_get: PASS\n"); +} + /* ── Suite entry point ──────────────────────────────────────────────── */ void test_libxce_retry(void) @@ -638,5 +782,10 @@ void test_libxce_retry(void) RUN_TEST(test_next_target_rotates_and_wraps); RUN_TEST(test_next_target_hint_then_fallback_rotation); RUN_TEST(test_next_target_rejects_bad_args); + RUN_TEST(test_capture_redirect_hint_present); + RUN_TEST(test_capture_redirect_hint_absent_stays_empty); + RUN_TEST(test_capture_redirect_hint_truncates_long_host); + RUN_TEST(test_capture_redirect_hint_null_ctx_safe); + RUN_TEST(test_reply_wait_wake_releases_blocked_get); UNITY_END(); } diff --git a/src/lib/xcectx.c b/src/lib/xcectx.c index 0d0758a..6978f16 100644 --- a/src/lib/xcectx.c +++ b/src/lib/xcectx.c @@ -27,10 +27,36 @@ static int handle_sub_reply(xce_ctx_t* ctx, xce_reply_t* reply) return 1; } +void xce_ctx_capture_redirect_hint(xce_ctx_t* ctx, const char* host, + uint16_t port) +{ + if (ctx == NULL) { + return; + } + + /* No usable redirect address → leave the hint untouched (empty). The + * capnp-owned host pointer may be NULL; an empty host or a zero port is + * likewise "no address". Advisory only — correctness never depends on it. */ + if ((host == NULL) || (host[0] == '\0') || (port == 0)) { + return; + } + + strncpy(ctx->redirect_hint_host, host, XCE_MAXHOSTNAME_LEN - 1); + ctx->redirect_hint_host[XCE_MAXHOSTNAME_LEN - 1] = '\0'; + ctx->redirect_hint_port = port; +} + static int handle_reply(xce_ctx_t* ctx, xce_reply_t* reply) { int rc = 0; + /* Capture the redirect address (if any) as a best-effort failover hint + * BEFORE the capnp-owned reply — including reply->redirect_host — is freed + * by the caller (Req 2.2, 8.3). The status still flows through the existing + * cbuf_tryput(&ctx->wbuf,&status) path unchanged, so xceapi.c is untouched. */ + xce_ctx_capture_redirect_hint(ctx, reply->redirect_host, + reply->redirect_port); + if (reply->status < 0) { return reply->status; }