From b6c131abad296d48ff1ea595b0b4606feb0f5a3f Mon Sep 17 00:00:00 2001 From: rigazilla Date: Mon, 19 Oct 2026 15:59:41 +0200 Subject: [PATCH 1/3] =?UTF-8?q?feat:=20multi-server=20seed=20list=20?= =?UTF-8?q?=E2=80=94=20startup=20+=20runtime=20failover=20(issue=20#12)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fromUri now carries all comma-separated hosts as seed servers, retained in URI order (RemoteCache::seeds_), mirroring the Java client's immutable ClusterInfo.getInitialServers() fallback. Startup failover: connect() tries each seed in order (reusing the pre-built bootstrap connection for seeds_[0]); all unreachable -> one HotRodClientException listing every seed tried. Runtime failover: a new tryConnectSeeds() helper is wired into the fall-through points of selectAnyServer and selectServerForKey (incl. the previous `return connection_` no-owners/empty path). It prefers the live bootstrap connection (fast path, so happy path + BASIC mode do no new I/O), else walks the seeds via orderKeyCandidates and adopts the first reachable seed as the new bootstrap connection. Semantics: - Keyed seed fallback is gated on proxyToNonOwner_ (a seed serving a non-owned key is a proxy; preserves D5). Default true matches Java. - Caller-driven exhaustion != unreachability: excluding every known server still throws (no seed rescue of an excluded node, which can alias a seed under a different address — localhost vs. topology container IP). - connection_ is reassigned on runtime failover and its reads in isConnected()/disconnect() now take stateMutex_ (removes the shared_ptr assignment race); disconnect() closes outside the lock to avoid joining the read loop while it waits on stateMutex_. Tests: +2 startup-failover (FromUriIntegrationTest), +1 runtime-failover (MultiSeedIntegrationTest, 2-node BASIC client, kill connected node). All affected regressions re-run green (RetryView, PingRetry, HashAwareRouting, ConcurrentMultiServer); unit 250/250; -Werror/Release build clean. Rationale, Java comparison, and divergences in docs/DECISIONS.md. Co-Authored-By: Claude Opus 4.8 --- CMakeLists.txt | 20 +- README.md | 7 +- docs/DECISIONS.md | 101 +++++++ docs/STATUS.md | 61 ++++- include/hotrod/HotRodURI.h | 11 +- include/hotrod/RemoteCache.h | 44 ++- src/operations/RemoteCache.cpp | 253 ++++++++++++++---- tests/integration/FromUriIntegrationTest.cpp | 47 ++++ .../integration/MultiSeedIntegrationTest.cpp | 135 ++++++++++ 9 files changed, 606 insertions(+), 73 deletions(-) create mode 100644 tests/integration/MultiSeedIntegrationTest.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index eb62bf2..cb7a64a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -319,6 +319,18 @@ if(GTest_FOUND) # Note: NOT gtest_main - we provide our own main() ) + # Multi-server seed list — runtime failover (issue #12) integration tests (multi-server) + add_executable(multiseed_integration_tests + tests/integration/MultiSeedIntegrationTest.cpp + ) + + target_link_libraries(multiseed_integration_tests + PRIVATE + hotrod-client + GTest::gtest + # Note: NOT gtest_main - we provide our own main() + ) + # SASL/SCRAM authentication integration tests (auth-enabled single server) add_executable(auth_integration_tests tests/integration/AuthIntegrationTest.cpp @@ -356,6 +368,7 @@ if(GTest_FOUND) add_test(NAME ConcurrentMultiServerTests COMMAND concurrent_multiserver_tests) add_test(NAME RetryViewIntegrationTests COMMAND retryview_integration_tests) add_test(NAME PingRetryIntegrationTests COMMAND pingretry_integration_tests) + add_test(NAME MultiSeedIntegrationTests COMMAND multiseed_integration_tests) add_test(NAME AuthIntegrationTests COMMAND auth_integration_tests) # Set working directory so scripts can be found @@ -431,6 +444,10 @@ if(GTest_FOUND) WORKING_DIRECTORY ${CMAKE_BINARY_DIR} LABELS "integration;docker;topology;multi-server;retry" ) + set_tests_properties(MultiSeedIntegrationTests PROPERTIES + WORKING_DIRECTORY ${CMAKE_BINARY_DIR} + LABELS "integration;docker;topology;multi-server;failover" + ) set_tests_properties(AuthIntegrationTests PROPERTIES WORKING_DIRECTORY ${CMAKE_BINARY_DIR} LABELS "integration;docker;auth" @@ -445,7 +462,8 @@ if(GTest_FOUND) ContainsKeyIntegrationTests ErrorHandlingIntegrationTests FromUriIntegrationTests TopologyIntegrationTests ConcurrentClientsTests HashAwareRoutingIntegrationTests ConcurrentMultiServerTests - RetryViewIntegrationTests PingRetryIntegrationTests AuthIntegrationTests + RetryViewIntegrationTests PingRetryIntegrationTests MultiSeedIntegrationTests + AuthIntegrationTests PROPERTIES TIMEOUT 300 ) endif() # NOT WIN32 (integration tests) diff --git a/README.md b/README.md index a43543c..0a44ff5 100644 --- a/README.md +++ b/README.md @@ -197,6 +197,9 @@ any headline count.)_ - ✅ **Hot Rod URI** (`RemoteCache::fromUri("hotrod://...")`) — build a client from a connection string (hosts, credentials, `sasl_mechanism`/`client_intelligence`/ `protocol_version` params); `hotrod://` only for now (TLS and more in issue #10) +- ✅ **Multi-server seed list** (issue #12) — all hosts in the URI become seed + servers with startup *and* runtime failover (ops fall back to the seed list when + the connected server dies or no topology is known, e.g. in BASIC mode) - ✅ **Integration test framework** (GoogleTest + Docker + multi-node clusters) ### Full CRUD with Smart Routing and Async Operations @@ -244,7 +247,9 @@ auto cache = RemoteCache::fromUri( "my-cache"); cache->connect(); -// v1 supports hotrod:// only; the first host seeds the topology, credentials +// Supports hotrod:// only. All hosts become seed servers (issue #12): connect() +// tries each in order, and ops fall back to the seed list when the connected +// server dies or no topology is known (e.g. BASIC mode). Credentials // enable SCRAM, and sasl_mechanism/client_intelligence/protocol_version (Java // "4.0"/"4.1" form) are honoured. Userinfo and query values are percent-decoded // (RFC 3986), so a reserved character in a password is written encoded — e.g. a diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index 9a03c75..9bb5bca 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -903,3 +903,104 @@ was missing from the ctest `TIMEOUT 300` group — the exact hang the 2026-10-12 fix addressed everywhere else. Added `-f -` to its CLI invocation and listed `FromUriIntegrationTests` in the timeout block so this suite can no longer stall the CI job on Infinispan `:latest`. + +## 2026-10-19 — Multi-server seed list: startup + runtime failover (issue #12) + +**What:** `fromUri` now carries *all* comma-separated hosts as seed servers +(`RemoteCache::seeds_`, retained in URI order; mirrors the Java client's immutable +`ClusterInfo.getInitialServers()`). Two failover tiers were added: +- **Startup** — `connect()` tries each seed in order, reusing the pre-built + `connection_` for `seeds_[0]` and building fresh for the rest; the first success + becomes the bootstrap `connection_` (and `host_`/`port_`). If every seed is + unreachable it throws one `HotRodClientException(BeforeSend)` whose message lists + every seed tried (with its error). +- **Runtime** — a new `tryConnectSeeds(exclude, tried)` helper (caller holds + `stateMutex_`) is wired into the fall-through points of both selection methods: + `selectAnyServer` (keyless), the hash-owners-exhausted path of + `selectServerForKey`, and that method's no-hash/no-owners/empty fallback (which + previously just `return connection_`). It prefers the live bootstrap `connection_` + (fast path, so the happy path and BASIC mode do no new I/O), else walks the seeds + via the existing pure `orderKeyCandidates({}, seeds_, exclude∪tried)` and adopts + the first reachable seed as the new bootstrap connection so the next op's fast + path reuses it instead of re-probing a dead one. + +**Why it matters:** v1 of the URI work (issue #9) used only the first host as the +seed and relied on topology discovery for the rest, which loses startup failover +and — in BASIC mode, where no topology is ever learned — any failover at all. + +**Design decisions (discussed before implementing, compared against the Java 16.3 +client):** +- **Honor `proxyToNonOwner` for keyed seed fallback (D5 preserved).** When a key's + owners are all down/excluded *and* `proxyToNonOwner_ == false`, we do **not** fall + back to a seed — a seed serving a non-owned key is a proxy, exactly what that flag + forbids. So keyed seed fallback is gated on `proxyToNonOwner_`. (Java has no such + flag — it always proxies; our default `true` matches Java, the strict mode is a + C++-only opt-in.) Bootstrap cases (empty/no topology, no owners) ignore the flag, + since there are no owners to exhaust. +- **Caller-driven exhaustion ≠ unreachability.** If the topology is non-empty but + the caller excluded *every* known server, we throw rather than rescue via a seed. + Found via `PingRetryIntegrationTest.AllServersExcludedThrowsBeforeSend`: the seed's + address (`localhost:11222`) differs from the topology's internal container address + (`172.18.0.x:11222`) for the *same* node, so an exclusion set built from topology + addresses does not contain the seed — the seed fast path would have wrongly + rescued the excluded ping. Fix: seed fallback fires only when we have no topology + at all, or when we actually attempted ≥1 server and it failed (not when the + candidate list was emptied purely by exclusion). Applied to both selection paths + (an explicit `candidates.empty()` throw in `selectAnyServer`; a `!tried.empty()` + gate on the keyed path). +- **Reassign `connection_` on runtime failover (+ fast path), and lock its reads.** + Pointing `connection_` at the reached seed keeps `isConnected()` truthful and + avoids re-probing the dead bootstrap seed on every op during an outage. Because + `connection_` can now be reassigned from a user/op thread (under `stateMutex_`), + the previously-unlocked reads in `isConnected()`/`disconnect()` now take + `stateMutex_` to remove the shared_ptr assignment race. `disconnect()` copies the + pointer out under the lock and calls `close()` *outside* it (close joins the read + loop, which may be blocked in `handleTopologyUpdate` waiting on the same mutex). + The startup `connect()` reassignment is pre-read-loop/single-threaded. +- **Runtime seed (re)connection is gated on a connect()..disconnect() lifecycle + flag (`active_`).** First cut of `tryConnectSeeds` reconnected whenever the + bootstrap connection was down — which silently re-established a connection for an + op issued *after* `disconnect()` (or before `connect()`), breaking the long- + standing contract that such ops throw. Caught by the full `ctest` sweep: all ten + `*AfterDisconnect` suite tests failed (they `EXPECT_THROW std::runtime_error` + after `disconnect()`; BASIC mode has empty topology, so they hit exactly the new + keyed seed-fallback path). Fix: `active_` is set true at the end of a successful + `connect()` and false in `disconnect()` (both under `stateMutex_`); when false, + `tryConnectSeeds` returns the bootstrap `connection_` unchanged so the op throws + "Not connected" as before. `ReconnectAndPing` (disconnect → connect again) still + works because `connect()` re-sets the flag. + +**Divergences from the Java client (intentional, cross-checked against +`/home/rigazilla/git/infinispan/client/hotrod-client` @ 16.3):** +- **connect() all-seeds-down throws; Java tolerates.** Java's `OperationDispatcher + .start()` eagerly connects to all initial servers but swallows every failure and + defers to the first op. We keep our existing "`connect()` establishes ≥1 + connection or throws" contract — clearer for callers; the thrown message names + every seed for diagnosis. +- **No failed-server timeout memo.** Java keeps a global `connectionFailedServers` + set with `serverFailureTimeout` expiry so it pre-emptively avoids known-dead + servers. We have no equivalent, so during an outage we re-probe dead *topology* + members per op (the dead *bootstrap* seed is avoided by the reassign+fast-path). + Latency-under-failure only, never a wrong result. Left as future work. +- **Privileged `connection_` kept (not folded into the pool).** Java 16.x has one + equal channel per address and no distinguished connection; our bootstrap + `connection_` lives outside `connectionPool_`, which is why the fast-path/locking + dance exists and why `seeds_[0]` can hold a duplicate socket (one as `connection_`, + one as a pool entry) once it also appears in topology. Keeping it was a scope call: + folding it into the pool as just-another-entry (the Java-faithful model, which + would delete the fast-path/locking code) is a larger refactor tracked as future + work. Functionally the layered model matches Java's observable failover behavior. + +**Tests:** +2 startup-failover in `FromUriIntegrationTest` (recover via 2nd seed; +all-seeds-unreachable throws naming each) and +2 `MultiSeedIntegrationTest`: +`RuntimeFailoverAcrossSeedsBasic` (2-node cluster, BASIC client seeded with both, +kill the connected node, next op recovers via the other seed) and +`TopologyAwareStartupFailoverAndHashRouting` (HASH client from a multi-seed URI +with a dead first seed: connect() fails over, then topology discovery + hash +routing work). The pure "all topology members unreachable -> seed" corner is not +staged as an integration test: the seeds are the cluster nodes, so killing every +topology member also kills every seed — the BASIC test isolates the seed tier +instead. All affected +regressions re-run green: RetryView (incl. the D5 proxy-disabled throw), PingRetry +(incl. the all-excluded throw fixed above), HashAwareRouting, ConcurrentMultiServer; +unit 250/250; `-Werror`/Release build clean. diff --git a/docs/STATUS.md b/docs/STATUS.md index 8976fe8..d4494af 100644 --- a/docs/STATUS.md +++ b/docs/STATUS.md @@ -33,13 +33,29 @@ _(Full per-session workflow: [`WORKFLOW.md`](WORKFLOW.md).)_ _The 1–3 concrete things to do next. Keep this short and current._ -0. **Hot Rod URI support COMPLETE (issue #9).** `RemoteCache::fromUri(uri, +0. **Multi-server seed list COMPLETE (issue #12).** All comma-separated hosts in a + `fromUri` connection string become seed servers with **startup + runtime + failover**. `connect()` tries each seed in order until one connects (all + unreachable → one `HotRodClientException` listing every seed tried). At runtime, + when the learned topology is empty (incl. BASIC mode) or every known server is + unreachable, ops fall back to the seed list via `tryConnectSeeds` (prefers the + live bootstrap `connection_`; adopts the reached seed as the new bootstrap). + Caller-driven exhaustion is distinguished from unreachability: excluding *every* + known server still throws (no seed rescue of an excluded node). Keyed seed + fallback is gated on `proxyToNonOwner_` (a seed serving a non-owned key is a + proxy, D5). `connection_` reads in `isConnected()`/`disconnect()` now lock + `stateMutex_` (it can be reassigned by runtime failover). Mirrors the Java + client's `ClusterInfo.getInitialServers()` fallback; divergences (connect() + throws vs. Java-tolerant; no failed-server timeout memo) in + [`DECISIONS.md`](DECISIONS.md) (2026-10-19). **Next: pick a roadmap step — Step + 12 (bulk ops) or benchmark the multiplexing path.** +1. **Hot Rod URI support COMPLETE (issue #9).** `RemoteCache::fromUri(uri, cacheName="")` builds a configured (not-yet-connected) client from a `hotrod://[user:password@]host1[:port1][,host2...][?k=v&...]` connection string; returns a `std::unique_ptr` (RemoteCache owns a mutex, so it is non-movable). Parsing lives in a pure, header-only `HotRodURI::parse()` - (`include/hotrod/HotRodURI.h`, 27 unit tests). v1 scope: `hotrod://` only; - first host is the seed (topology discovery finds the rest); credentials enable + (`include/hotrod/HotRodURI.h`, 27 unit tests). Scope: `hotrod://` only; + all hosts are seed servers (issue #12, above); credentials enable SCRAM; userinfo and query values are percent-decoded (RFC 3986, java.net.URI parity — so an `@`/`/`/`:` in a password written `%40`/`%2F`/`%3A` reaches the client as its real bytes); query whitelist `sasl_mechanism` / @@ -49,8 +65,6 @@ _The 1–3 concrete things to do next. Keep this short and current._ `hotrods://`/TLS, unknown params (`connect_timeout`, `socket_timeout`, `use_ssl`, `token`, …), `protocol_version=AUTO`/3.x. Deferred features tracked in **issue #10**. Rationale in [`DECISIONS.md`](DECISIONS.md) (2026-10-09). - **Next: pick a roadmap step — Step 12 (bulk ops) or benchmark the multiplexing - path.** 1. **SASL/SCRAM authentication COMPLETE — end-to-end for the whole SCRAM family.** `RemoteCache::setAuthentication(username, password, realm="default", serverName="infinispan", mechanism="SCRAM-SHA-256")` (flat setter, matching @@ -201,11 +215,13 @@ pull from here next. Step numbers follow (`src/operations/RemoteCache.cpp:140,182`). - [x] **Hot Rod URI support (`fromUri`)** — **shipped 2026-10-09** (issue #9, 27 unit tests). See *Working and shipped*. +- [x] **Multi-server seed list (issue #12)** — **shipped 2026-10-19** (4 integration + tests). Startup + runtime failover across all URI seeds. See *Working and shipped*. - [ ] **Hot Rod URI — deferred features (issue #10):** `hotrods://`/TLS + cert/SNI/hostname-validation params, `connect_timeout`/`socket_timeout`, - multi-server seed list (startup failover across seeds), non-SCRAM SASL - (PLAIN/OAUTHBEARER `token`), `protocol_version=AUTO`/3.x. Each drops its - "unsupported" throw from `HotRodURI::parse` once the capability lands. + non-SCRAM SASL (PLAIN/OAUTHBEARER `token`), `protocol_version=AUTO`/3.x. Each + drops its "unsupported" throw from `HotRodURI::parse` once the capability lands. + (Multi-server seed list is now shipped — issue #12, above.) - [ ] **TLS/SSL support** (transport encryption). Also unblocks `hotrods://` above. **Longer horizon (Step 14+ advanced features):** transactions (XA), client @@ -225,10 +241,18 @@ multiplexing. See "Working and shipped" below._ - **Hot Rod URI (`RemoteCache::fromUri`)** — build a configured client from a `hotrod://[user:pass@]host[:port][,host...][?k=v]` connection string (issue #9); pure header-only parser `HotRodURI::parse` (`include/hotrod/HotRodURI.h`). - `hotrod://` scheme only, first host seeds the topology, SCRAM via credentials, + `hotrod://` scheme only, SCRAM via credentials, `sasl_mechanism`/`client_intelligence`/`protocol_version` params; userinfo and query values are percent-decoded (java.net.URI parity); unsupported - schemes/params throw. TLS/timeouts/multi-seed/non-SCRAM deferred (issue #10) + schemes/params throw. TLS/timeouts/non-SCRAM deferred (issue #10) +- **Multi-server seed list + failover (`fromUri`, issue #12)** — all + comma-separated hosts become seed servers. **Startup failover:** `connect()` + tries each seed in order; all unreachable → one `HotRodClientException` listing + every seed. **Runtime failover:** when the learned topology is empty (incl. + BASIC mode) or every known server is unreachable, ops fall back to the seed list + (`tryConnectSeeds`) — prefers the live bootstrap `connection_` and adopts the + reached seed as the new bootstrap; keyed fallback gated on `proxyToNonOwner_`. + Excluding every known server still throws (no seed rescue of an excluded node) - Wire primitives (vInt, vLong, strings, byte arrays) - Protocol 4.0 headers (all conditional fields) - **SASL/SCRAM authentication (end-to-end)** — `setAuthentication(...)` runs the @@ -294,8 +318,17 @@ multiplexing. See "Working and shipped" below._ earlier +17 for `ServerSelectionTest` (`orderKeyCandidates` + `unionNodes`, Step 11b slices 1–3; +3 keyless-ordering tests for `selectAnyServer`, Step 11c slice 1) still stands -- Integration: **92/92** passing across 19 suites (`ctest`, spins up Docker +- Integration: **96/96** passing across 20 suites (`ctest`, spins up Docker Infinispan single-server + multi-node clusters), now also green on Linux CI. + +2 for the multi-seed startup failover in `FromUriIntegrationTest` (issue #12: + a dead first seed, live second seed → `connect()` recovers via the second; all + seeds unreachable → `connect()` throws a message naming each seed) and +2 for + `MultiSeedIntegrationTest` (issue #12): `RuntimeFailoverAcrossSeedsBasic` (a + BASIC-mode client seeded with both cluster nodes keeps serving after the + connected node is killed, failing over to the other seed) and + `TopologyAwareStartupFailoverAndHashRouting` (a HASH client built from a + multi-seed URI with a dead first seed fails over at connect(), then learns + topology and hash-routes) — verified live this session. +2 for `FromUriIntegrationTest` (issue #9: a client built via `fromUri` round-trips a value; a `?protocol_version=4.0` query param flows through to a working client) and +1 for `AuthIntegrationTest.FromUriWithCredentialsAuthenticates` (credentials @@ -319,8 +352,10 @@ multiplexing. See "Working and shipped" below._ The interlaced distributed tests (`ConcurrentMultiServerTest`) were fixed to tolerate a GET racing ahead of its PUT — a `nullopt` is expected, only a present-but-wrong value is an error. -- Full run: `ctest --test-dir build --output-on-failure` → 100% pass - (19 ctest tests: 1 unit + 18 integration suites) +- Full run: `ctest --test-dir build --output-on-failure` → **100% pass, 21/21** + (1 unit + 20 integration suites) — verified end-to-end this session after the + multi-seed work (incl. the `active_` lifecycle fix that keeps post-`disconnect()` + ops throwing; see DECISIONS 2026-10-19) - **Concurrency validated (2026-09-25):** the concurrent suites pass 8/8 (incl. `ConcurrentWithFailover`), and a ThreadSanitizer run (`build-tsan/`) reports **no races in production code** — slice-4's `stateMutex_`/`shared_ptr` diff --git a/include/hotrod/HotRodURI.h b/include/hotrod/HotRodURI.h index 9aa2f86..27a7e44 100644 --- a/include/hotrod/HotRodURI.h +++ b/include/hotrod/HotRodURI.h @@ -27,11 +27,13 @@ namespace hotrod { * validation are unit-testable in isolation. RemoteCache::fromUri() consumes it * and applies the flat setters. * - * v1 scope (the rest is deferred to issue #10): + * Scope (the rest is deferred to issue #10): * - scheme: only "hotrod://". "hotrods://" (TLS) throws, since the transport has * no TLS yet. - * - multiple comma-separated servers are parsed into `servers`, but the caller - * uses only the first as the seed (topology discovery finds the rest). + * - multiple comma-separated servers are parsed into `servers`; all of them become + * seed servers with startup + runtime failover (issue #12) — RemoteCache tries + * them in order at connect() and falls back to the list when the learned + * topology is empty or exhausted. `seed()` is retained as the first host. * - query parameters are a typed whitelist; an unknown key or a bad value for a * known key throws. Deferred params (timeouts, TLS, use_ssl, non-SCRAM SASL) * are therefore rejected rather than silently ignored. @@ -44,7 +46,8 @@ struct HotRodURI { std::optional clientIntelligence; std::optional protocolVersion; // Protocol::VERSION_40 / _41 - /** The first server in the list — the seed used by RemoteCache in v1. */ + /** The first server in the list — the bootstrap seed RemoteCache connects to + * first; the full `servers` list is used for startup/runtime failover (#12). */ const ServerAddress& seed() const { return servers.front(); } /** diff --git a/include/hotrod/RemoteCache.h b/include/hotrod/RemoteCache.h index 97422f8..7544e49 100644 --- a/include/hotrod/RemoteCache.h +++ b/include/hotrod/RemoteCache.h @@ -50,9 +50,11 @@ class RemoteCache { * Because RemoteCache is non-movable (it owns a mutex), this returns a * unique_ptr rather than a value. * - * v1 scope (see HotRodURI / issue #10 for what is deferred): only the - * hotrod:// scheme; the first host is used as the seed (topology discovery - * finds the rest); credentials enable SCRAM; the client_intelligence, + * Scope (see HotRodURI / issue #10 for what is still deferred): only the + * hotrod:// scheme; ALL comma-separated hosts become seed servers with + * startup and runtime failover across them (issue #12) — connect() tries each + * in order, and ops fall back to the seed list when the learned topology is + * empty or exhausted; credentials enable SCRAM; the client_intelligence, * protocol_version, and sasl_mechanism query parameters are honoured, any * other parameter (or hotrods:// / TLS) throws. * @@ -493,6 +495,12 @@ class RemoteCache { std::string host_; uint16_t port_; + // Configured seed servers, in URI/constructor order (issue #12). Always holds + // at least one entry. Startup failover (connect()) and runtime failover + // (tryConnectSeeds) try these in order; mirrors the Java client's immutable + // ClusterInfo.getInitialServers() — the configured list is never discarded and + // is the permanent fallback when the learned topology is exhausted. + std::vector seeds_; std::string cacheName_; // shared_ptr (not unique_ptr) so a connection handed to an in-flight // operation stays alive even if a concurrent topology update evicts it from @@ -502,6 +510,11 @@ class RemoteCache { uint8_t protocolVersion_; // Protocol version (default: VERSION_41) ClientIntelligence clientIntelligence_; // Client intelligence level bool proxyToNonOwner_ = true; // Owner->non-owner fallback in routing (D5) + // Lifecycle: true only between a successful connect() and disconnect(). Guards + // runtime seed (re)connection (tryConnectSeeds) so an op before connect() or + // after disconnect() still fails as before instead of silently re-establishing + // a connection via a seed. Guarded by stateMutex_ (issue #12). + bool active_ = false; Authentication auth_; // SASL auth config (disabled by default); applied at connect() // stateMutex_ guards the routing/pool state below (topology_, consistentHash_, @@ -589,6 +602,31 @@ class RemoteCache { */ std::shared_ptr getConnectionForServer(const ServerInfo& server); + /** + * Build a (not-yet-connected) MultiplexedConnection to a seed address, wired + * with this cache's topology-update callback. Used by the constructors and by + * connect()'s startup-failover loop. + */ + std::shared_ptr makeSeedConnection(const ServerAddress& addr); + + /** + * Runtime seed failover (issue #12). Try the configured seed servers in order + * (minus @p exclude and anything already in @p tried), returning the first + * reachable connection, or nullptr if none is reachable. + * + * Fast path: reuses the live bootstrap connection_ when its own target isn't + * excluded/tried, so the happy path (topology empty / BASIC mode) costs no new + * I/O. When a *different* seed is reached, it is adopted as the new bootstrap + * connection_ (and host_/port_ follow) so the next op's fast path reuses it + * instead of re-probing the dead one. + * + * Caller must hold stateMutex_ (invoked from the selection methods); this + * method does not lock. @p tried accumulates every seed this call attempts. + */ + std::shared_ptr tryConnectSeeds( + const std::vector& exclude, + std::vector& tried); + /** * Clean up connections to servers no longer in topology. * Called after topology updates. Caller must hold stateMutex_ diff --git a/src/operations/RemoteCache.cpp b/src/operations/RemoteCache.cpp index 1c52af3..0c2b427 100644 --- a/src/operations/RemoteCache.cpp +++ b/src/operations/RemoteCache.cpp @@ -32,30 +32,29 @@ namespace hotrod } } // namespace - RemoteCache::RemoteCache(const std::string &host, uint16_t port) - : host_(host), port_(port), cacheName_(""), - protocolVersion_(Protocol::VERSION_41), - clientIntelligence_(ClientIntelligence::BASIC) + std::shared_ptr RemoteCache::makeSeedConnection(const ServerAddress &addr) { - // Create MultiplexedConnection with topology callback auto topologyCallback = [this](const TopologyInfo &topo, const std::string &cacheName) { this->handleTopologyUpdate(topo, cacheName); }; - connection_ = std::make_shared(host, port, topologyCallback); + return std::make_shared(addr.host, addr.port, topologyCallback); + } + + RemoteCache::RemoteCache(const std::string &host, uint16_t port) + : host_(host), port_(port), seeds_{{host, port}}, cacheName_(""), + protocolVersion_(Protocol::VERSION_41), + clientIntelligence_(ClientIntelligence::BASIC) + { + connection_ = makeSeedConnection(seeds_.front()); } RemoteCache::RemoteCache(const std::string &host, uint16_t port, const std::string &cacheName) - : host_(host), port_(port), cacheName_(cacheName), + : host_(host), port_(port), seeds_{{host, port}}, cacheName_(cacheName), protocolVersion_(Protocol::VERSION_41), clientIntelligence_(ClientIntelligence::BASIC) { - // Create MultiplexedConnection with topology callback - auto topologyCallback = [this](const TopologyInfo &topo, const std::string &cacheName) - { - this->handleTopologyUpdate(topo, cacheName); - }; - connection_ = std::make_shared(host, port, topologyCallback); + connection_ = makeSeedConnection(seeds_.front()); } std::unique_ptr RemoteCache::fromUri(const std::string &uri, @@ -64,9 +63,12 @@ namespace hotrod // Pure parse + validation (throws on anything malformed or unsupported). const HotRodURI parsed = HotRodURI::parse(uri); - // v1: the first host is the seed; topology discovery finds the rest (#10). + // All hosts become seed servers with startup + runtime failover (issue #12). + // The first is the bootstrap connection; the full list is retained for + // failover (see RemoteCache::seeds_ / connect() / tryConnectSeeds). const ServerAddress &seed = parsed.seed(); auto cache = std::make_unique(seed.host, seed.port, cacheName); + cache->seeds_ = parsed.servers; // legal: fromUri is a RemoteCache member if (parsed.clientIntelligence) { @@ -106,19 +108,66 @@ namespace hotrod void RemoteCache::connect() { - connection_->setProtocolVersion(protocolVersion_); - connection_->setClientIntelligence(clientIntelligence_); - connection_->setAuthentication(auth_); - connection_->connect(); + // Startup failover (issue #12): try each configured seed in order until one + // connects. The pre-built connection_ serves seeds_[0]; the rest are built + // fresh on demand. The first success becomes the bootstrap connection_. + // Runs before any read loop / concurrent op, so the connection_ assignment + // only needs the lock to pair cleanly with the locked reads elsewhere. + std::vector failures; + for (size_t i = 0; i < seeds_.size(); ++i) + { + std::shared_ptr candidate = + (i == 0) ? connection_ : makeSeedConnection(seeds_[i]); + candidate->setProtocolVersion(protocolVersion_); + candidate->setClientIntelligence(clientIntelligence_); + candidate->setAuthentication(auth_); + try + { + candidate->connect(); + } + catch (const std::exception &e) + { + failures.push_back(seeds_[i].host + ":" + std::to_string(seeds_[i].port) + + " (" + e.what() + ")"); + continue; + } + { + std::lock_guard lock(stateMutex_); + connection_ = candidate; + host_ = seeds_[i].host; + port_ = seeds_[i].port; + active_ = true; // enable runtime seed failover (issue #12) + } + return; + } + + // Every seed was unreachable. Report them all so a bad connection string is + // diagnosable (the Java client tolerates this and defers to the first op; we + // keep connect()'s "establish >=1 connection or throw" contract). + std::string msg = "Hot Rod connect failed: all seed servers unreachable:"; + for (const auto &f : failures) + msg += " " + f + ";"; + throw HotRodClientException(msg, FailurePhase::BeforeSend); } void RemoteCache::disconnect() { - connection_->close(); + // Copy the bootstrap connection out under the lock (it can be reassigned by + // runtime failover), then close() OUTSIDE the lock: close() joins the read + // loop, which may be blocked in handleTopologyUpdate waiting on stateMutex_. + std::shared_ptr conn; + { + std::lock_guard lock(stateMutex_); + active_ = false; // stop runtime seed failover; ops after disconnect fail + conn = connection_; + } + if (conn) + conn->close(); } bool RemoteCache::isConnected() const { + std::lock_guard lock(stateMutex_); return connection_ && connection_->isConnected(); } @@ -1010,6 +1059,25 @@ namespace hotrod break; } + // Seed fallback (issue #12): every known server is unreachable → try + // the configured seeds to re-bootstrap. Gated on: + // - proxyToNonOwner_: a seed serving a non-owned key is a proxy, + // exactly what proxyToNonOwner=false forbids (D5); when off we keep + // the ownersExhausted throw instead of falling back to a seed. + // - !tried.empty(): only fall back when we actually attempted and + // failed to reach a server. If every candidate was *excluded* by the + // caller (nothing tried), honor that exhaustion rather than rescuing + // via a seed that may alias an excluded server (see selectAnyServer). + if (proxyToNonOwner_ && !tried.empty()) + { + if (auto conn = tryConnectSeeds(ctx.excludeNodes, tried)) + { + if (triedOut) + *triedOut = tried; + return conn; + } + } + fprintf(stderr, "[WARN] No servers available for segment %d\n", segment); if (triedOut) *triedOut = tried; @@ -1024,8 +1092,22 @@ namespace hotrod } } - // Fallback: use default connection - return connection_; + // Fallback: no hash topology, or no owners for this key (incl. BASIC mode and + // empty topology) → bootstrap via the seed list. tryConnectSeeds prefers the + // live connection_, so the common case stays a no-op reuse (issue #12). + std::vector tried; + if (auto conn = tryConnectSeeds(ctx.excludeNodes, tried)) + { + if (triedOut) + *triedOut = tried; + return conn; + } + if (triedOut) + *triedOut = tried; + throw HotRodClientException("No servers available (seed servers unreachable)", + FailurePhase::BeforeSend, std::nullopt, + unionNodes(ctx.excludeNodes, tried), + /*ownersExhausted=*/false); } std::shared_ptr RemoteCache::selectAnyServer(const RetryContext &ctx, @@ -1039,53 +1121,122 @@ namespace hotrod for (const auto &server : topology_.getServers()) allAddrs.push_back({server.host, server.port}); - // Early ping: no topology received yet → nothing to route over, so use the - // seed connection so the very first ping still works. - if (allAddrs.empty()) - { - if (triedOut) - triedOut->clear(); - return connection_; - } - - // Keyless candidates: every server minus the exclusion set, in topology - // order. Reuses the pure, unit-tested orderKeyCandidates() with an empty - // owner list (no key → no owners); the proxy flag is irrelevant here since - // there are no owners, so all servers come from the allServers pass. - std::vector candidates = - orderKeyCandidates({}, allAddrs, ctx.excludeNodes, true); - std::vector tried; - for (const auto &cand : candidates) + + // Try known topology servers first (none if no topology received yet), minus + // the exclusion set, in topology order. Reuses the pure, unit-tested + // orderKeyCandidates() with an empty owner list (no key → no owners). + if (!allAddrs.empty()) { - tried.push_back(cand); - try + std::vector candidates = + orderKeyCandidates({}, allAddrs, ctx.excludeNodes, true); + if (candidates.empty()) { - std::shared_ptr conn = - getConnectionForServer(ServerInfo(cand.host, cand.port, 0)); + // Topology is known but the caller excluded every server. Honor that — + // do NOT rescue via a seed (a seed can alias an excluded server under a + // different address, e.g. localhost vs. its topology container IP). This + // is caller-driven exhaustion, distinct from "all servers unreachable". if (triedOut) - *triedOut = tried; - return conn; + *triedOut = tried; // empty + throw HotRodClientException("No servers available (all excluded)", + FailurePhase::BeforeSend, std::nullopt, + unionNodes(ctx.excludeNodes, tried), + /*ownersExhausted=*/false); } - catch (const std::exception &e) + for (const auto &cand : candidates) { - fprintf(stderr, "[WARN] Failed to connect to %s:%u: %s\n", - cand.host.c_str(), cand.port, e.what()); - // Continue to next candidate + tried.push_back(cand); + try + { + std::shared_ptr conn = + getConnectionForServer(ServerInfo(cand.host, cand.port, 0)); + if (triedOut) + *triedOut = tried; + return conn; + } + catch (const std::exception &e) + { + fprintf(stderr, "[WARN] Failed to connect to %s:%u: %s\n", + cand.host.c_str(), cand.port, e.what()); + // Continue to next candidate + } } } - // Topology existed but every server was excluded or unreachable. Nothing was - // sent (BeforeSend). ownersExhausted is always false for a keyless op. + // No topology yet (e.g. the very first ping), or every topology server we were + // allowed to try was unreachable → fall back to the configured seed list + // (issue #12). tryConnectSeeds prefers the live connection_, so an early ping + // before any topology still works via the bootstrap connection, as before. + if (auto conn = tryConnectSeeds(ctx.excludeNodes, tried)) + { + if (triedOut) + *triedOut = tried; + return conn; + } + + // Nothing reachable. Nothing was sent (BeforeSend). ownersExhausted is always + // false for a keyless op. fprintf(stderr, "[WARN] No servers available for keyless operation\n"); if (triedOut) *triedOut = tried; - throw HotRodClientException("No servers available in topology", + throw HotRodClientException("No servers available (all topology members and " + "seed servers unreachable)", FailurePhase::BeforeSend, std::nullopt, unionNodes(ctx.excludeNodes, tried), /*ownersExhausted=*/false); } + std::shared_ptr RemoteCache::tryConnectSeeds( + const std::vector &exclude, std::vector &tried) + { + // Caller holds stateMutex_ (see header). Only (re)connect to seeds within the + // connect()..disconnect() window. Outside it (before connect, after + // disconnect) return the bootstrap connection_ unchanged so the op fails as it + // did before this feature — never silently re-establish a connection. + if (!active_) + return connection_; + + // Fast path: reuse the live bootstrap + // connection unless its own target is excluded or already tried this attempt + // — so the happy path (empty topology / BASIC mode) does no new I/O. + const ServerAddress current{host_, port_}; + auto inList = [](const std::vector &v, const ServerAddress &a) { + return std::find(v.begin(), v.end(), a) != v.end(); + }; + if (connection_ && connection_->isConnected() && + !inList(exclude, current) && !inList(tried, current)) + { + return connection_; + } + + // Otherwise walk the configured seeds in order, minus excluded/already-tried + // (reuses the pure, unit-tested ordering/dedup helper). The first reachable + // seed is adopted as the new bootstrap connection_ so the next op's fast path + // reuses it instead of re-probing a dead one (runtime failover, issue #12). + const std::vector seedCands = + orderKeyCandidates({}, seeds_, unionNodes(exclude, tried), true); + for (const auto &seed : seedCands) + { + tried.push_back(seed); + try + { + std::shared_ptr conn = + getConnectionForServer(ServerInfo(seed.host, seed.port, 0)); + connection_ = conn; + host_ = seed.host; + port_ = seed.port; + return conn; + } + catch (const std::exception &e) + { + fprintf(stderr, "[WARN] Seed %s:%u unreachable: %s\n", + seed.host.c_str(), seed.port, e.what()); + // Continue to next seed + } + } + return nullptr; + } + std::shared_ptr RemoteCache::getConnectionForServer(const ServerInfo &server) { // Create connection pool key diff --git a/tests/integration/FromUriIntegrationTest.cpp b/tests/integration/FromUriIntegrationTest.cpp index ebe53a9..81f82b7 100644 --- a/tests/integration/FromUriIntegrationTest.cpp +++ b/tests/integration/FromUriIntegrationTest.cpp @@ -2,6 +2,7 @@ #include #include "hotrod/RemoteCache.h" #include "hotrod/HeaderCodec.h" +#include "hotrod/HotRodClientException.h" #include "InfinispanTestEnvironment.h" #include #include @@ -83,6 +84,52 @@ TEST(FromUriIntegrationTest, ProtocolVersionParamProducesWorkingClient) { cache->disconnect(); } +// Startup failover (issue #12): a dead FIRST seed followed by the live server. +// connect() must try the dead seed, fail, and establish the bootstrap connection +// via the second seed; ops then round-trip. 127.0.0.1:1 is a reserved, unused +// port, so the first attempt is refused fast rather than hanging. +TEST(FromUriIntegrationTest, StartupFailoverToSecondSeed) { + const std::string cacheName = "fromuri_failover"; + createCacheViaCLI(cacheName); + + const std::string uri = "hotrod://127.0.0.1:1," + + InfinispanTestEnvironment::host + ":" + + std::to_string(InfinispanTestEnvironment::port); + auto cache = RemoteCache::fromUri(uri, cacheName); + cache->connect(); + EXPECT_TRUE(cache->isConnected()); + + const ByteArray key{'f', 'o', 'k'}; + const ByteArray value{'f', 'o', 'v'}; + cache->put(key, value).get(); + auto got = cache->get(key).get(); + + ASSERT_TRUE(got.has_value()); + EXPECT_EQ(*got, value); + + cache->disconnect(); +} + +// When every seed is unreachable, connect() throws a BeforeSend +// HotRodClientException whose message names each seed it tried. +TEST(FromUriIntegrationTest, StartupAllSeedsUnreachableThrows) { + auto cache = RemoteCache::fromUri("hotrod://127.0.0.1:1,127.0.0.1:2", "nope"); + + bool threw = false; + try { + cache->connect(); + } catch (const HotRodClientException& e) { + threw = true; + EXPECT_EQ(FailurePhase::BeforeSend, e.phase); + const std::string msg = e.what(); + EXPECT_NE(msg.find("127.0.0.1:1"), std::string::npos) + << "message must name the first seed tried: " << msg; + EXPECT_NE(msg.find("127.0.0.1:2"), std::string::npos) + << "message must name the second seed tried: " << msg; + } + EXPECT_TRUE(threw) << "all seeds unreachable must throw from connect()"; +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); ::testing::AddGlobalTestEnvironment(new InfinispanTestEnvironment()); diff --git a/tests/integration/MultiSeedIntegrationTest.cpp b/tests/integration/MultiSeedIntegrationTest.cpp new file mode 100644 index 0000000..e753190 --- /dev/null +++ b/tests/integration/MultiSeedIntegrationTest.cpp @@ -0,0 +1,135 @@ +#include +#include "TopologyTestFixture.h" +#include "hotrod/RemoteCache.h" +#include "hotrod/HotRodClientException.h" +#include +#include +#include +#include + +using namespace hotrod; +using namespace hotrod::test; + +/** + * Multi-server seed list — runtime failover (issue #12). + * + * Startup failover (a dead first seed at connect()) is covered against the + * single-server fixture in FromUriIntegrationTest. This suite closes the runtime + * half: a client seeded with the whole cluster keeps serving after the server it + * is currently talking to dies, by failing over to another seed. + * + * BASIC intelligence is used deliberately — it never receives a topology update, + * so the connection pool stays empty and the client only ever routes through a + * seed. That isolates the seed-failover path (tryConnectSeeds) from topology + * discovery: the recovery here can only have come from the seed list. + */ +class MultiSeedIntegrationTest : public TopologyTest { +protected: + void SetUp() override { + TopologyTest::SetUp(); + resetCluster(2); // DIST_SYNC numOwners=2 → both nodes own every key + clearTestCache(); + } +}; + +// Seed the client with BOTH cluster nodes in BASIC mode, connect (lands on the +// first seed), store a key, then kill that node. The next op must fail over to +// the second seed and still read the value back (it is replicated to both owners). +TEST_F(MultiSeedIntegrationTest, RuntimeFailoverAcrossSeedsBasic) { + const auto& n1 = MultiServerTestEnvironment::getServer(0); + const auto& n2 = MultiServerTestEnvironment::getServer(1); + + const std::string uri = "hotrod://" + n1.host + ":" + std::to_string(n1.port) + + "," + n2.host + ":" + std::to_string(n2.port); + auto cache = RemoteCache::fromUri(uri, TEST_CACHE); + cache->setClientIntelligence(ClientIntelligence::BASIC); // default; explicit for intent + cache->connect(); + ASSERT_TRUE(cache->isConnected()); + + const std::string keyStr = "multiseed-runtime"; + const std::string valueStr = "multiseed-runtime-value"; + ByteArray key(keyStr.begin(), keyStr.end()); + ByteArray value(valueStr.begin(), valueStr.end()); + + cache->put(key, value).get(); + std::this_thread::sleep_for(std::chrono::seconds(2)); // replicate to the other owner + + // Kill node 1 — the bootstrap seed connect() landed on (seeds are tried in URI + // order, so the first live seed is node 1). + fprintf(stderr, "[TEST] killing seed node 1 (%s:%d)\n", n1.host.c_str(), n1.port); + removeNode(1); + std::this_thread::sleep_for(std::chrono::seconds(3)); + + // The next op must fail over to seed node 2. GET is idempotent, so a short + // retry loop safely absorbs an attempt that races the node going down. + std::optional result; + for (int attempt = 0; attempt < 5 && !result.has_value(); ++attempt) { + try { + result = cache->get(key).get(); + } catch (const std::exception& e) { + fprintf(stderr, "[TEST] attempt %d failed, retrying: %s\n", attempt, e.what()); + std::this_thread::sleep_for(std::chrono::seconds(1)); + } + } + + ASSERT_TRUE(result.has_value()) + << "runtime failover to the second seed must recover the value"; + EXPECT_EQ(value, *result); + + cache->disconnect(); + + // Restore the cluster for any subsequent tests. + addNode(1); + std::this_thread::sleep_for(std::chrono::seconds(3)); +} + +// Topology-aware variant: a HASH_DISTRIBUTION_AWARE client built from a multi-seed +// URI whose FIRST seed is dead must (1) fail over at connect() to a live seed, +// (2) learn the cluster topology from it, and (3) hash-route keyed ops — i.e. the +// multi-seed URI path works end-to-end with topology, not just in BASIC mode. (The +// pure "all topology members unreachable -> seed" corner is not staged here: the +// seeds ARE the cluster nodes, so killing every topology member also kills every +// seed; BASIC-mode RuntimeFailoverAcrossSeedsBasic above isolates the seed tier.) +TEST_F(MultiSeedIntegrationTest, TopologyAwareStartupFailoverAndHashRouting) { + const auto& n1 = MultiServerTestEnvironment::getServer(0); + const auto& n2 = MultiServerTestEnvironment::getServer(1); + + const std::string uri = "hotrod://127.0.0.1:1," + + n1.host + ":" + std::to_string(n1.port) + "," + + n2.host + ":" + std::to_string(n2.port) + + "?client_intelligence=hash_distribution_aware"; + auto cache = RemoteCache::fromUri(uri, TEST_CACHE); + cache->connect(); // must fail over past the dead first seed + ASSERT_TRUE(cache->isConnected()); + + const std::string keyStr = "multiseed-topo"; + const std::string valueStr = "multiseed-topo-value"; + ByteArray key(keyStr.begin(), keyStr.end()); + ByteArray value(valueStr.begin(), valueStr.end()); + + // Drive ops until the client has learned the hash topology from a seed. + bool hashReady = false; + for (int i = 0; i < 20 && !hashReady; ++i) { + cache->put(key, value).get(); + std::this_thread::sleep_for(std::chrono::milliseconds(300)); + hashReady = cache->getConsistentHash().hasHashTopology(); + } + ASSERT_TRUE(hashReady) << "a URI-seeded client must learn hash topology from a seed"; + ASSERT_GE(cache->getTopology().getServers().size(), 2u) + << "topology discovery must find both cluster nodes via the seed"; + + // Hash-routed read round-trips. + auto got = cache->get(key).get(); + ASSERT_TRUE(got.has_value()); + EXPECT_EQ(value, *got); + + cache->disconnect(); +} + +// Main — 2-node cluster; both nodes own every key under DIST_SYNC numOwners=2. +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + MultiServerTestEnvironment::setInitialServerCount(2); + ::testing::AddGlobalTestEnvironment(new MultiServerTestEnvironment()); + return RUN_ALL_TESTS(); +} From 264d79bfc7189e2619f3aa1f79272e179ebf6717 Mon Sep 17 00:00:00 2001 From: rigazilla Date: Mon, 19 Oct 2026 18:05:39 +0200 Subject: [PATCH 2/3] docs: current-state hygiene + adopt PR #14's single-source test-count convention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit STATUS.md trimmed to current-state-only (per AGENTS.md/WORKFLOW.md): - ⏭ Next steps rebuilt as a clean 1–3 (Step 12 / benchmark / cleanup) + a one-line "recently shipped" pointer; dropped the completed-work narrative pile and fixed the broken numbering. - Backlog reduced to open [ ] items only; completed [x] blocks removed (they live in "Working and shipped" + dated DECISIONS entries). - Removed the ~88-line Step-10 implementation plan (shipped work); its schema-bug gotcha was preserved in DECISIONS.md first. - Refreshed stale date stamps (2026-09-16 / 2026-10-09 → 2026-10-19) and stale phrasings ("all four operations"; ops table). Adopt PR #14's convention (imported here; PR #14 closed unmerged as stale): - Test counts live once, in STATUS.md "Test status" (250 unit / 96 integration / 20 suites, read via --gtest_list_tests). README drops all hard counts and points to STATUS for the breakdown. - Imported PR #14's "stop repeating test counts" DECISIONS entry (dated 2026-10-09, placed chronologically) with a provenance note. DECISIONS.md: fixed the one non-date heading (multiplexing landing → 2026-07-02, per commit 5b35248); preserved the replace_if_unmodified nibble-flip schema gotcha. README.md: Current Progress / milestone footer now include multi-server seed list (#12); test-count strings removed per the convention above. Co-Authored-By: Claude Opus 4.8 --- README.md | 24 ++-- docs/DECISIONS.md | 52 ++++++- docs/STATUS.md | 344 +++++----------------------------------------- 3 files changed, 96 insertions(+), 324 deletions(-) diff --git a/README.md b/README.md index 0a44ff5..9484bdc 100644 --- a/README.md +++ b/README.md @@ -115,7 +115,7 @@ ctest -C Release --output-on-failure ## Testing -### Unit Tests (250 tests) +### Unit Tests ```bash cd build ./unit_tests @@ -124,7 +124,7 @@ cd build ctest -R UnitTests --output-on-failure ``` -### Integration Tests (92 tests, 19 suites) +### Integration Tests Requires Docker to run Infinispan server: ```bash cd build @@ -153,8 +153,9 @@ ctest --output-on-failure **Current Progress**: Full CRUD (PING/GET/PUT/REMOVE) + SCRAM auth + topology awareness + hash-aware routing + async multiplexing + metadata/version-based operations (Step 10) + typed error handling and user-decided retry, keyed and -keyless (Step 11) + Hot Rod URI support (`RemoteCache::fromUri`, issue #9) — all -shipped. **Next:** bulk operations (Step 12) or multiplexing benchmarks. See +keyless (Step 11) + Hot Rod URI support (`RemoteCache::fromUri`, issue #9) + +multi-server seed list with startup/runtime failover (issue #12) — all shipped. +**Next:** bulk operations (Step 12) or multiplexing benchmarks. See [docs/STATUS.md](docs/STATUS.md) for live status. Per-step milestone history (frozen at Step 9) is archived at [docs/archive/PROGRESS.md](docs/archive/PROGRESS.md). @@ -163,9 +164,9 @@ _(Step numbers follow [`../hotrod-foundry/ROADMAP.md`](../hotrod-foundry/ROADMAP [`docs/STATUS.md`](docs/STATUS.md) is authoritative for which steps are done — not any headline count.)_ -**Test Results** (verified 2026-10-09): -- Unit Tests: **250/250 passing** ✅ -- Integration Tests: **92/92 passing** ✅ across 19 suites (against live Infinispan via Docker) +**Test Results** — unit and integration suites all pass (integration against live +Infinispan via Docker). See [`docs/STATUS.md`](docs/STATUS.md) for the current +coverage breakdown and counts. ## Features @@ -294,8 +295,8 @@ cpp17-client/ │ ├── hash/ # MurmurHash3 + consistent hashing │ └── operations/ # Hot Rod operations (with failover) ├── tests/ # Test suite -│ ├── unit/ # 223 unit tests -│ └── integration/ # 92 integration tests, 19 suites (multi-node clusters) +│ ├── unit/ # unit tests (gtest) +│ └── integration/ # integration tests (multi-node clusters, Docker) ├── documentation/ # AsciiDoc user guide (index.adoc + topics/) ├── examples/ # Usage examples │ └── quickstart/ # Simple GET/PUT/REMOVE + retry + auth examples @@ -375,5 +376,6 @@ In short, following the hotrod-foundry porting guidelines: **Maintained by**: rigazilla **Status**: Active development -**Current Step**: Error handling + user-decided retry complete (Step 11, keyed & -keyless). Next: bulk operations (Step 12). ✅ +**Current Step**: Step 11 (error handling + user-decided retry, keyed & keyless) +complete; since then Hot Rod URI `fromUri` (#9) and multi-server seed list + +failover (#12) shipped. Next: bulk operations (Step 12). ✅ diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index 9bb5bca..c0a2651 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -75,7 +75,7 @@ again when true async landed. **Status:** ✅ Superseded — the real `MultiplexedConnection` replaced this shim (see next entry). No blocking shim remains in `RemoteCache.cpp`. -## 2026 (multiplexing landing) — MultiplexedConnection design +## 2026-07-02 — MultiplexedConnection design **Decision:** One connection per server multiplexes concurrent requests over a single socket. A dedicated read-loop thread reads responses; a @@ -862,6 +862,41 @@ today, so decoding them is currently a no-op; it is done anyway for Java parity and to be ready for issue #10's `token=` (OAUTHBEARER), whose values can carry `=`/`+`/`/`. +## 2026-10-09 — Stop repeating test counts across docs (CI badge considered, rejected) + +**Problem.** The unit/integration test counts were hand-typed in ~6 places across +`README.md` and `docs/STATUS.md`. Every test change meant editing all of them, and +nothing verified the numbers — one README spot had silently drifted to `223 unit +tests` while the real count had moved on. + +**Considered: a CI-published shields.io badge.** Compute counts from the *built test +binaries* in CI (`scripts/count_tests.sh`, counting via `--gtest_list_tests` so the +number matches what actually runs and needs no Docker) and force-push a shields +endpoint JSON to an orphan `badges` branch for the README to embed. Prototyped, then +**rejected as overkill** for a solo, intermittently-worked repo: it added a CI step +with `contents: write`, an orphan branch, a helper script, and a badge that 404s +until the first post-merge run and lags behind CDN caching — a lot of moving parts +to avoid typing a number. + +**Decision.** Just remove the duplication. `docs/STATUS.md` (the authoritative +current-state doc) carries the totals **once**, in its "Test status" section; +everywhere else describes *what* is covered rather than repeating the totals: +- README drops all hard counts — the `### Unit Tests` / `### Integration Tests` + headings, the directory-tree comments, and the "Test Results" block (which now + points at STATUS.md for the breakdown). +- Per-feature breadcrumbs ("shipped … N unit + M integration tests") stay, as + point-in-time slice notes. + +The one-place count is refreshed by hand when it drifts; `--gtest_list_tests` on the +built binaries (`unit_tests` plus the ctest `integration`-labelled suites) is the +quick way to get the real number without a `grep TEST(`, which overcounts +parameterized/disabled macros. + +**Provenance.** This decision + convention originated in PR #14 +(`feat/test-count-badge`); that PR was closed unmerged and its changes were +re-applied here (it had gone stale/conflicting against `main`). Counts as of this +re-application: **250 unit / 96 integration / 20 suites**. + ## 2026-10-12 — Integration tests hung on CI: Infinispan `:latest` CLI stopped exiting on EOF **Symptom.** Every CI run from Oct 9 on hung: `GetIntegrationTests` started and @@ -1004,3 +1039,18 @@ instead. All affected regressions re-run green: RetryView (incl. the D5 proxy-disabled throw), PingRetry (incl. the all-excluded throw fixed above), HashAwareRouting, ConcurrentMultiServer; unit 250/250; `-Werror`/Release build clean. + +## 2026-10-19 — Schema gotcha preserved from STATUS: replace_if_unmodified nibble flip + +**What:** Recording — in its proper home — a schema bug found 2026-09-20 that had +only ever lived in STATUS.md, moved here during the STATUS backlog cleanup so it is +not lost. `hotrod40.ksy` `replace_if_unmodified_request` packs the expiration +`time_units` nibbles in the OPPOSITE order from `put_request`/`expiration_params` +(lifespan and maxIdle swapped). Java uses one shared `writeExpirationParams` for +both PUT and replaceWithVersion, so PUT's order — **high nibble = lifespan, low +nibble = maxIdle** — is authoritative and is what this client implements +(`replaceWithVersion` 0x09, shipped 2026-09-20). Cross-checked against this client + +Java `ReplaceIfUnmodifiedOperation` + a corrected reading of the schema. +**Why it matters:** if someone later "fixes" the client to match the `.ksy` here, +replaceWithVersion's lifespan/maxIdle would silently swap. The `.ksy` is still wrong +on this point; fix pending upstream in hotrod-dissector. diff --git a/docs/STATUS.md b/docs/STATUS.md index d4494af..57af289 100644 --- a/docs/STATUS.md +++ b/docs/STATUS.md @@ -33,109 +33,16 @@ _(Full per-session workflow: [`WORKFLOW.md`](WORKFLOW.md).)_ _The 1–3 concrete things to do next. Keep this short and current._ -0. **Multi-server seed list COMPLETE (issue #12).** All comma-separated hosts in a - `fromUri` connection string become seed servers with **startup + runtime - failover**. `connect()` tries each seed in order until one connects (all - unreachable → one `HotRodClientException` listing every seed tried). At runtime, - when the learned topology is empty (incl. BASIC mode) or every known server is - unreachable, ops fall back to the seed list via `tryConnectSeeds` (prefers the - live bootstrap `connection_`; adopts the reached seed as the new bootstrap). - Caller-driven exhaustion is distinguished from unreachability: excluding *every* - known server still throws (no seed rescue of an excluded node). Keyed seed - fallback is gated on `proxyToNonOwner_` (a seed serving a non-owned key is a - proxy, D5). `connection_` reads in `isConnected()`/`disconnect()` now lock - `stateMutex_` (it can be reassigned by runtime failover). Mirrors the Java - client's `ClusterInfo.getInitialServers()` fallback; divergences (connect() - throws vs. Java-tolerant; no failed-server timeout memo) in - [`DECISIONS.md`](DECISIONS.md) (2026-10-19). **Next: pick a roadmap step — Step - 12 (bulk ops) or benchmark the multiplexing path.** -1. **Hot Rod URI support COMPLETE (issue #9).** `RemoteCache::fromUri(uri, - cacheName="")` builds a configured (not-yet-connected) client from a - `hotrod://[user:password@]host1[:port1][,host2...][?k=v&...]` connection - string; returns a `std::unique_ptr` (RemoteCache owns a mutex, so - it is non-movable). Parsing lives in a pure, header-only `HotRodURI::parse()` - (`include/hotrod/HotRodURI.h`, 27 unit tests). Scope: `hotrod://` only; - all hosts are seed servers (issue #12, above); credentials enable - SCRAM; userinfo and query values are percent-decoded (RFC 3986, java.net.URI - parity — so an `@`/`/`/`:` in a password written `%40`/`%2F`/`%3A` reaches the - client as its real bytes); query whitelist `sasl_mechanism` / - `client_intelligence` / - `protocol_version` (Java `"4.0"`/`"4.1"` form, case-insensitive intelligence). - Anything unsupported throws a clear `HotRodClientException` (BeforeSend): - `hotrods://`/TLS, unknown params (`connect_timeout`, `socket_timeout`, - `use_ssl`, `token`, …), `protocol_version=AUTO`/3.x. Deferred features tracked - in **issue #10**. Rationale in [`DECISIONS.md`](DECISIONS.md) (2026-10-09). -1. **SASL/SCRAM authentication COMPLETE — end-to-end for the whole SCRAM - family.** `RemoteCache::setAuthentication(username, password, realm="default", - serverName="infinispan", mechanism="SCRAM-SHA-256")` (flat setter, matching - `setClientIntelligence`) enables a SASL handshake on **every** connection the - client opens — the seed connection and each topology-discovered member — run - inside `MultiplexedConnection::connect()` before the read loop starts. The - `SaslAuthenticator` drives the two-turn SCRAM exchange over a testable - `SaslTransport` seam (`ConnectionSaslTransport` in production); `SCRAM` is - generalized across `SCRAM-SHA-1/256/512` (OpenSSL EVP, digest-derived - lengths). It completes on the client side after verifying the server signature - (no confirmation round; ignores the server `completed` flag — mirrors Java - `AuthHandler`). Any non-SCRAM mechanism throws a clear `HotRodClientException` - at connect(). New public headers: `Authentication.h`, `AuthCodec.h`, - `SaslAuthenticator.h`. Documented in `documentation/topics/security.adoc`. - Rationale in [`DECISIONS.md`](DECISIONS.md) (2026-09-25 SASL/SCRAM entry). - **Next: pick a new roadmap step — Step 12 (bulk ops) or benchmark the - multiplexing path.** -2. **Step 11c COMPLETE — keyless-op retry (ping) shipped (slices 1–3).** Keyless - operations now mirror the keyed retry strategy: automatic before-send failover - across all servers, user-decided after-send retry (D2). `ping()` routes through - `selectAnyServer(ctx, triedOut)`, which orders candidates via - `orderKeyCandidates({}, allServers, exclude)` (empty owners → all servers in - topology order), sweeps to the next server when one is unreachable, and — when - no topology is known yet (ping is often the very first op) — falls back to the - seed connection. `RetryView::ping()` forwards the exclusion set, so - `cache.excluding(e).ping()` works exactly like keyed ops; `ownersExhausted` is - always `false` for keyless ops. (1) `pingImpl` + `selectAnyServer` in - `RemoteCache.cpp`, 3 keyless-ordering unit tests; (2) `PingRetryIntegrationTest` - (3 tests) — `excluding()` routes the keyless ping to another server, excluding - all servers throws BeforeSend with `ownersExhausted=false`, and the - catch→`excluding(e).ping()` loop recovers after the first-candidate node is - killed; (3) `examples/quickstart/retry.cpp` gained a keyless `pingWithRetry` - loop + README/ERROR_HANDLING docs. The implementation is generic, so future - non-key ops (server stats/admin) reuse `selectAnyServer`. Rationale in - [`DECISIONS.md`](DECISIONS.md) (2026-09-25 Step 11c entries). **Next: pick a new - roadmap step — Step 12 (bulk ops) or benchmark the multiplexing path.** -2. **Step 11b COMPLETE — user-decided retry shipped (slices 1–6).** (1) `selectServerForKey` routes - over the pure, unit-tested `orderKeyCandidates()` helper (owners → non-owner - fallback, minus an exclusion set), takes a defaulted `RetryContext`, reports - the nodes it tried via an out-param, and computes `ownersExhausted` honestly. - (2) Client-level `proxyToNonOwner` flag (default `true`, D5: - `setProxyToNonOwner`/`getProxyToNonOwner`) gates the owner→non-owner fallback; - when `false`, exhausting owners throws a transient exception with - `ownersExhausted=true` instead of proxying. (3) Op-level retry threading via a - **bound-view** API — base op signatures stay retry-free; `cache.excluding(nodes)` - / `cache.excluding(exception)` returns a `RetryView` that forwards to private - `…Impl(args, const RetryContext&)` methods, and every op (including the plain - first call) unions the nodes it tried into the thrown exception's `triedNodes`, - so a caught exception feeds straight back as the next attempt's exclusion set. - New public headers: `RetryContext.h`, `ServerSelection.h`, `RetryView.h`. - (4) Thread-safety: a single `stateMutex_` guards `topology_` + - `consistentHash_` + `connectionPool_` across the read-loop's topology callback - (`handleTopologyUpdate`) and user/retry threads (`selectServerForKey` → - `getConnectionForServer`); the connection pool holds `shared_ptr` and each op - keeps its connection alive for the operation's duration, so a concurrent - topology update can't free a connection mid-`execute()`. - (5) End-to-end integration coverage: `RetryViewIntegrationTest` (3 tests) - exercises `excluding()` dispatch — routes around an excluded owner, throws - `ownersExhausted` when proxy is disabled and all owners are excluded, and runs - the documented catch→`excluding(e)` retry loop after killing the primary owner. - (6) Retry-loop example `examples/quickstart/retry.cpp` (+ README section) — - `getWithRetry`/`putWithRetry` loops using `isTransient`/`outcomeUncertain`/ - `excluding(e)`, and the clarification that `proxyToNonOwner` does not let a - caller skip the catch block. Retry policy/idempotency stays the user's call. - Full design + progress checklist: - [`ERROR_HANDLING_DESIGN.md`](ERROR_HANDLING_DESIGN.md) §5; rationale in - [`DECISIONS.md`](DECISIONS.md) (2026-09-21, 2026-09-25 entries). -3. **Benchmark the multiplexing path** — the async rewrite targets 5–10× - concurrent throughput; this has not been measured yet. -4. **Small cleanup:** read header "other params" when `paramCount > 0` - (two `TODO`s in `src/operations/RemoteCache.cpp:140,182`). +1. **Step 12 — Bulk operations.** `GET_ALL` (0x2F), `PUT_ALL` (0x2D), + `BULK_GET` (0x1F, iterator-style). See 📋 Backlog below. +2. **Benchmark the multiplexing path** — the async rewrite targets 5–10× + concurrent throughput; never measured. +3. **Small cleanup:** read header "other params" when `paramCount > 0` + (two `TODO`s in `src/operations/RemoteCache.cpp`). + +_Recently shipped (detail in "Working and shipped" + [`DECISIONS.md`](DECISIONS.md)): +multi-server seed list + failover (#12, 2026-10-19), Hot Rod URI `fromUri` (#9), +SASL/SCRAM auth, Step 11 error handling + user-decided retry, Step 10 metadata/CAS._ --- @@ -147,64 +54,6 @@ pull from here next. Step numbers follow (STATUS.md) is authoritative for what's done._ **Remaining roadmap steps:** -- **Step 10 — Metadata operations** (version-based CAS). Deliverables: - - [x] `getWithMetadata` (0x1B/0x1C) — **shipped 2026-09-17** (8 unit + 7 integration tests) - - [x] `removeWithVersion` (0x0D/0x0E) — **shipped 2026-09-19** (7 unit + 5 integration tests). Returns `future` (matches Java `boolean removeWithVersion(K, long)`); added `Codec::writeLong`/`readLong` (fixed 8-byte BE). Parser drains the `*_WITH_PREVIOUS` (0x03/0x04) body defensively though FORCE_RETURN_VALUE isn't wired yet. Byte layout cross-checked against all three legs: this client, Java `RemoveIfUnmodifiedOperation`, and `hotrod40.ksy` (`remove_if_unmodified_request` = `key: lp_bytes` + `entry_version: s8`, big-endian per `meta.endian: be`). - - [x] `replaceWithVersion`/REPLACE_IF_UNMODIFIED (0x09/0x0A) — **shipped 2026-09-20** (5 unit + 5 integration tests). Returns `future`; request reuses PUT's expiration byte (high nibble=lifespan, low=maxIdle) + `Codec::writeLong` for the version. **Schema bug found:** `hotrod40.ksy` `replace_if_unmodified_request` has the expiration nibbles flipped vs `put_request`/`expiration_params`; Java uses one shared `writeExpirationParams` for both, so PUT's order is authoritative. Cross-checked against client + Java `ReplaceIfUnmodifiedOperation` + (corrected reading of) the schema. - - [x] `putIfAbsent` (0x05/0x06), `replace` (0x07/0x08), `containsKey` (0x0F/0x10) — **shipped 2026-09-20** (11 unit + 12 integration tests). `putIfAbsent`/`replace` reuse PUT's request layout (`key` + expiration byte + `value`) and return `future>` — the previous/replaced entry when `previousValue` (FORCE_RETURN_VALUE) is set, `nullopt` otherwise; parsers drain the `*_WITH_PREVIOUS` (0x03/0x04) body defensively. `containsKey` is a key-only request returning `future` from the status (no response body). Cross-checked against Java `PutIfAbsentOperation`/`ReplaceOperation`/`ContainsKeyOperation` and `hotrod40.ksy` (put_if_absent/replace → `put_request`/`put_response`; contains_key → `key_request`, status-only response). - - Java ref: `GetWithMetadataOperation`, `ReplaceIfUnmodifiedOperation`. See the - [step-by-step plan](#plan-metadata-operations-step-10) below. -- [x] **Step 11 — Error handling — COMPLETE (11a + 11b + 11c).** Design agreed - 2026-09-21 → [`ERROR_HANDLING_DESIGN.md`](ERROR_HANDLING_DESIGN.md). Split: - - [x] **11a — Error surfacing** — **shipped 2026-09-25** (11 unit + 3 - integration tests). ERROR response parsing (opcode 0x50) with - length-prefixed message drained in the read loop (keeps the stream in sync - even for orphan responses), surfaced as a typed `HotRodClientException` - (pure-data: `FailurePhase` {BeforeSend/AfterSend/ServerError} + - `serverStatus` + `triedNodes` + `ownersExhausted`) replacing bare - `std::runtime_error`, still catchable as `std::runtime_error`. Two-axis - classification helpers: `isTransient()` (futility — library's call) and - `outcomeUncertain()` (ambiguity — 0x86/AfterSend). `COMMAND_TIMEOUT` (0x86) - is transient **and** outcome-uncertain (intentionally diverges from Java). - - [x] **11b — User-decided retry** — **shipped 2026-09-25**: exclusion-aware - `selectServerForKey`, `proxyToNonOwner` default true, a bound-view retry API - (fork 1.b), thread-safe pool/topology. Retry policy/idempotency is the - user's call, not automatic. **Slices 1–3:** exclusion-aware selection - via the pure `orderKeyCandidates()` helper (`ServerSelection.h`) + - `RetryContext` (`RetryContext.h`); client-level `proxyToNonOwner` flag - (default `true`); op-level retry threading via `RetryView`/`cache.excluding()` - (`RetryView.h`) with `triedNodes` unioned into every thrown exception (pure - `unionNodes()` helper). Base op signatures stay retry-free. **Slice 4 - (thread-safety) done:** `stateMutex_` guards topology/hash/pool across the - read-loop and user/retry threads; pool is `shared_ptr` and each op keeps its - connection alive for its duration (no free mid-`execute()`). **Slice 5 - (integration test):** `RetryViewIntegrationTest` (3 tests) covers `excluding()` - route-around, proxy-disabled owners-exhausted throw, and the catch→retry loop - after killing the primary owner. **Slice 6 (example):** - `examples/quickstart/retry.cpp` shows the retry loop and the `proxyToNonOwner` - clarification. - - [x] **11c — Keyless-op retry (ping)** — **shipped 2026-09-25** (3 unit + 3 - integration tests). Keyless ops mirror the keyed two-tier model: automatic - before-send failover across all servers via `selectAnyServer` (deterministic - topology order, minus an exclusion set; seed-connection fallback when no - topology is known yet), user-decided after-send retry via - `cache.excluding(e).ping()`. `ownersExhausted` is always `false` for keyless - ops; `selectAnyServer` is generic for future non-key ops. - `PingRetryIntegrationTest` (3 tests) + a keyless `pingWithRetry` in the retry - example. - Java ref: `org.infinispan.client.hotrod.exceptions.*`. -- [x] **SASL/SCRAM authentication — COMPLETE.** End-to-end auth for the whole - SCRAM family (`SCRAM-SHA-1/256/512`). `setAuthentication(...)` (flat setter) - runs the AUTH_MECH_LIST + two-turn SCRAM handshake on every connection (seed + - topology-discovered), inside `MultiplexedConnection::connect()` before the read - loop; `SaslAuthenticator` over a testable `SaslTransport` seam; `SCRAM` - generalized across the digest family (OpenSSL EVP). Completes on the client - side after verifying the server signature (no confirmation round). Non-SCRAM - mechanisms throw at connect(). New headers `Authentication.h`, `AuthCodec.h`, - `SaslAuthenticator.h`; docs `documentation/topics/security.adoc`; 19 unit + - 5 integration tests. This supersedes the earlier "SCRAM crypto exists but is - dead code" state. Java ref: `AuthHandler` / `AuthOperation`; Kaitai `hotrod40.ksy`. - [ ] **Step 12 — Bulk operations.** `GET_ALL` (0x2F), `PUT_ALL` (0x2D), `BULK_GET` (0x1F, iterator-style). @@ -213,29 +62,27 @@ pull from here next. Step numbers follow concurrent throughput; unmeasured so far. - [ ] **Code cleanup:** read header "other params" when `paramCount > 0` (`src/operations/RemoteCache.cpp:140,182`). -- [x] **Hot Rod URI support (`fromUri`)** — **shipped 2026-10-09** (issue #9, 27 - unit tests). See *Working and shipped*. -- [x] **Multi-server seed list (issue #12)** — **shipped 2026-10-19** (4 integration - tests). Startup + runtime failover across all URI seeds. See *Working and shipped*. - [ ] **Hot Rod URI — deferred features (issue #10):** `hotrods://`/TLS + cert/SNI/hostname-validation params, `connect_timeout`/`socket_timeout`, non-SCRAM SASL (PLAIN/OAUTHBEARER `token`), `protocol_version=AUTO`/3.x. Each drops its "unsupported" throw from `HotRodURI::parse` once the capability lands. - (Multi-server seed list is now shipped — issue #12, above.) + (Multi-server seed list is already shipped — issue #12; see "Working and shipped".) - [ ] **TLS/SSL support** (transport encryption). Also unblocks `hotrods://` above. **Longer horizon (Step 14+ advanced features):** transactions (XA), client listeners/events, counters, Ickle queries, streaming ops (4.1+), multimap, near caching. Not scheduled. -_Done and not repeated here: foundation/primitives, Protocol 4.0 headers, -SCRAM auth (Step 3), topology (Step 4), consistent hashing + hash-aware -routing, connection pooling (Step 13), full CRUD (Steps 6–9), async -multiplexing. See "Working and shipped" below._ +_Done and not repeated here (see "Working and shipped" below + dated +[`DECISIONS.md`](DECISIONS.md) entries): foundation/primitives, Protocol 4.0 +headers, topology (Step 4), consistent hashing + hash-aware routing, connection +pooling (Step 13), full CRUD (Steps 6–9), async multiplexing, Step 10 +metadata/CAS, Step 11 error handling + user-decided retry, SASL/SCRAM auth, Hot +Rod URI `fromUri` (#9), multi-server seed list + failover (#12)._ --- -## Current state (verified against code, 2026-09-16) +## Current state (verified 2026-10-19) **Working and shipped:** - **Hot Rod URI (`RemoteCache::fromUri`)** — build a configured client from a @@ -290,7 +137,7 @@ multiplexing. See "Working and shipped" below._ the connection stays usable after a server error (no stream desync) - **Async API:** all operations return `std::future` / `std::optional` - **MultiplexedConnection:** true async — dedicated read-loop thread, - `messageId → promise` pending map, `execute()` used by all four operations. + `messageId → promise` pending map, `execute()` used by all operations. _(The July "70% / temporary blocking" docs are obsolete — see archive note.)_ - Multi-server topology test fixtures + Docker-based integration tests - **CI runs integration tests on Linux** (Ubuntu job) as of 2026-09-18 — the @@ -305,57 +152,19 @@ multiplexing. See "Working and shipped" below._ suite verified green against `:latest` = 16.3.0 on 2026-10-12. See [`DECISIONS.md`](DECISIONS.md). -**Test status (verified 2026-10-09):** -- Unit: **250/250** passing (`./build/unit_tests`, <1s) — +27 for the Hot Rod URI - parser (`HotRodURITest`: grammar, multi-host, userinfo, the typed query - whitelist, port trailing-garbage rejection, RFC-3986 percent-decoding of - userinfo/query values + malformed-escape rejection, and explicit rejection of - `hotrods://`/unknown-param/`AUTO`-3.x). The - +19 for the auth work - (`AuthCodecTest` body round-trips + `SaslAuthenticatorTest`, a parameterized - SCRAM-SHA-1/256/512 handshake driven by a fake OpenSSL server, plus - mech-not-offered / unsupported-mech / bad-server-signature failure paths); the - earlier +17 for `ServerSelectionTest` (`orderKeyCandidates` + `unionNodes`, - Step 11b slices 1–3; +3 keyless-ordering tests for `selectAnyServer`, Step 11c - slice 1) still stands -- Integration: **96/96** passing across 20 suites (`ctest`, spins up Docker - Infinispan single-server + multi-node clusters), now also green on Linux CI. - +2 for the multi-seed startup failover in `FromUriIntegrationTest` (issue #12: - a dead first seed, live second seed → `connect()` recovers via the second; all - seeds unreachable → `connect()` throws a message naming each seed) and +2 for - `MultiSeedIntegrationTest` (issue #12): `RuntimeFailoverAcrossSeedsBasic` (a - BASIC-mode client seeded with both cluster nodes keeps serving after the - connected node is killed, failing over to the other seed) and - `TopologyAwareStartupFailoverAndHashRouting` (a HASH client built from a - multi-seed URI with a dead first seed fails over at connect(), then learns - topology and hash-routes) — verified live this session. - +2 for `FromUriIntegrationTest` (issue #9: a client built via `fromUri` round-trips - a value; a `?protocol_version=4.0` query param flows through to a working client) - and +1 for `AuthIntegrationTest.FromUriWithCredentialsAuthenticates` (credentials - carried in the URI authenticate end-to-end) — verified live this session. These - prove the `fromUri` parse→setter→connect→op chain; exhaustive parsing coverage - stays in the pure unit tests. (Hash-aware `client_intelligence` via URI is NOT - integration-tested on the single-server fixture — the Dockerized single node - advertises internal container addresses, so routing tests use the multi-server - suites; see DECISIONS 2026-10-09.) - +3 for `RetryViewIntegrationTest` (Step 11b: `excluding()` routes around an - owner; proxy-disabled owners-exhausted throw; the catch→`excluding(e)` retry - loop recovers after the primary owner is killed) — verified 3/3 this session. - +3 for `PingRetryIntegrationTest` (Step 11c: keyless `excluding().ping()` routes - to another server; excluding all servers throws BeforeSend with - `ownersExhausted=false`; the catch→`excluding(e).ping()` loop recovers after the - first-candidate node is killed) — verified 3/3 this session. - +5 for `AuthIntegrationTest` (SASL/SCRAM against an auth-enabled Infinispan 16 - server started by `scripts/start_infinispan_auth.sh`): SCRAM-SHA-1/256/512 - put/get round-trips (3, parameterized), wrong-password throws at connect(), and - no-credentials-against-a-secured-server fails — verified 5/5 live this session. - The interlaced distributed tests (`ConcurrentMultiServerTest`) were fixed to - tolerate a GET racing ahead of its PUT — a `nullopt` is expected, only a - present-but-wrong value is an error. +**Test status (verified 2026-10-19):** **250 unit** + **96 integration** (20 +suites), all passing. This is the one authoritative count — other docs describe +*what* is covered rather than repeating totals. Counts read from the built test +binaries via `--gtest_list_tests`, so they match what actually runs. +- Unit (`./build/unit_tests`, <1s) — wire primitives/headers, consistent hashing, + server-selection/retry helpers, the SASL/SCRAM handshake, and the Hot Rod URI + parser. +- Integration (`ctest`, spins up Docker Infinispan single-server + multi-node + clusters; also green on Linux CI) — CRUD + metadata/CAS, topology & hash-aware + routing, keyed/keyless retry (`RetryView`/ping), SASL/SCRAM against a secured + server, Hot Rod URI `fromUri`, and multi-server seed failover. - Full run: `ctest --test-dir build --output-on-failure` → **100% pass, 21/21** - (1 unit + 20 integration suites) — verified end-to-end this session after the - multi-seed work (incl. the `active_` lifecycle fix that keeps post-`disconnect()` - ops throwing; see DECISIONS 2026-10-19) + (1 unit + 20 integration suites) — verified end-to-end this session. - **Concurrency validated (2026-09-25):** the concurrent suites pass 8/8 (incl. `ConcurrentWithFailover`), and a ThreadSanitizer run (`build-tsan/`) reports **no races in production code** — slice-4's `stateMutex_`/`shared_ptr` @@ -381,7 +190,7 @@ full list (Steps 10–12, benchmarks, TLS, code TODOs). | User guide (AsciiDoc) | `documentation/index.adoc` + `documentation/topics/` | | Retry API (bound view) | `include/hotrod/RetryView.h`, `RetryContext.h`; routing helpers in `ServerSelection.h` | | Async transport core | `src/transport/MultiplexedConnection.cpp` | -| Operations (PING/GET/PUT/REMOVE) | `src/operations/RemoteCache.cpp` | +| Operations (CRUD, metadata/CAS, ping) | `src/operations/RemoteCache.cpp` | | Codecs | `src/codec/`, `src/hash/`, `src/auth/`, `src/topology/` | | Unit tests | `tests/unit/` | | Integration tests | `tests/integration/` (see `README_TOPOLOGY.md` there) | @@ -393,95 +202,6 @@ full list (Steps 10–12, benchmarks, TLS, code TODOs). --- -## Plan: metadata operations (Step 10) - -Opcodes verified against the official Hot Rod protocol reference -(infinispan.org/docs → hotrod_protocol) **and** the authoritative Kaitai -schema at [`../hotrod-dissector/schemas/hotrod40.ksy`](../../hotrod-dissector/schemas/hotrod40.ksy) -on 2026-09-16. That `.ksy` is the wire-format source of truth for protocol -4.0/4.1 — consult it (not prose docs) when in doubt about a byte layout. - -> **Why the Kaitai schema is trustworthy:** it's an *independent* encoding of -> the same protocol the Java client implements, so the two act as a mutual -> cross-check. If this client, the schema, and the Java client ever disagree on -> a byte layout, one of them has a bug — investigate before shipping. Public -> copy: . -> -> **Known schema bug (found 2026-09-20):** `hotrod40.ksy` -> `replace_if_unmodified_request` packs the expiration `time_units` nibbles in -> the *opposite* order from `put_request`/`expiration_params` (lifespan and -> maxIdle swapped). Java uses one shared `writeExpirationParams` for PUT and -> replaceWithVersion, so PUT's order — **high nibble = lifespan, low = maxIdle** -> — is authoritative and is what this client implements. Fix pending upstream in -> hotrod-dissector. - -### Opcode table (request / response) - -| Operation | Req | Resp | In code? | -|-----------|-----|------|----------| -| put | 0x01 | 0x02 | ✅ | -| get | 0x03 | 0x04 | ✅ | -| putIfAbsent | 0x05 | 0x06 | ✅ | -| replace (only if present) | 0x07 | 0x08 | ✅ | -| **replaceWithVersion** (replaceIfUnmodified) | **0x09** | **0x0A** | ✅ | -| remove | 0x0B | 0x0C | ✅ | -| **removeWithVersion** (removeIfUnmodified) | **0x0D** | **0x0E** | ✅ | -| containsKey | 0x0F | 0x10 | ✅ | -| getWithVersion | 0x11 | 0x12 | ❌ | -| **getWithMetadata** | **0x1B** | **0x1C** | ✅ | - -Opcode constants are defined in `include/hotrod/HeaderCodec.h` (`Opcode` -namespace). All Step 10 opcodes above are present; add new ones there. - -### Recommended order -`getWithMetadata` first (it yields the entry version), then the two -version-based writes that depend on it, then the simpler conditionals: -1. ~~**getWithMetadata** (0x1B/0x1C)~~ ✅ shipped 2026-09-17 -2. ~~**removeWithVersion** (0x0D/0x0E)~~ ✅ shipped 2026-09-19 -3. ~~**replaceWithVersion** (0x09/0x0A)~~ ✅ shipped 2026-09-20 -4. ~~putIfAbsent (0x05/0x06), replace (0x07/0x08), containsKey (0x0F/0x10)~~ ✅ shipped 2026-09-20 — **Step 10 complete** - -### Step-by-step: getWithMetadata (the entry point) - -Mirrors the existing `get()` at `src/operations/RemoteCache.cpp:221`. - -1. **Opcodes** — add to `HeaderCodec.h`: - `GET_WITH_METADATA_REQUEST = 0x1B`, `GET_WITH_METADATA_RESPONSE = 0x1C`. -2. **Request encoding** — identical to GET: Protocol 4.0 header + key as - `lp_bytes` (vInt length + bytes). Reuse the get() request builder. -3. **Response body parser** — this is the new bit. `execute()` extracts the - header status and hands it to the parser as the `status` arg (do **not** - read a status byte from the body — the `.ksy` puts status in the header): - - `status == 0x01 || 0x02` → key not found → return `{}` (→ `nullopt`), - exactly as `get()` does at `RemoteCache.cpp:221` - - on `status == 0x00`: **reuse `Connection::receiveMetadata()`** - (`Connection.cpp:292`) — it reads flag + optional created(s8)/lifespan(vInt) - + optional lastUsed(s8)/maxIdle(vInt) + 8-byte version, byte-for-byte - matching `entry_metadata` in the `.ksy` (lines 498–516) - - read value as `lp_bytes` (`conn->receiveByteArray()`) - - return `EntryWithMetadata{ metadata, value }` -4. **Public method** — add - `std::future> getWithMetadata(const ByteArray& key);` - to `RemoteCache.h`, implement via `connection_->execute(...)` with the - parser lambda (same shape as get()). -5. **Unit test** — new `tests/unit/GetWithMetadataTest.cpp`: encode request, - decode a crafted response with and without infinite-lifespan/maxidle flags, - assert version + timestamps + value. Add target to `CMakeLists.txt` and to - the `unit_tests` sources. -6. **Integration test** — PUT a key, then `getWithMetadata`, assert a non-zero - version and the value round-trips (pattern: `GetIntegrationTest.cpp`). - -**Wire format cheat-sheet (response 0x1C):** -`status(1)` · `flag(1)` [`0x01`=INFINITE_LIFESPAN, `0x02`=INFINITE_MAXIDLE] · -`created(int64)`+`lifespan(vInt)` *(only if lifespan not infinite)* · -`lastUsed(int64)`+`maxIdle(vInt)` *(only if maxIdle not infinite)* · -`version(int64, 8 bytes, always)` · `valueLen(vInt)`+`value` *(on success)*. - -**Then removeWithVersion / replaceWithVersion** carry the 8-byte version *in the -request*: removeWithVersion = key + version; replaceWithVersion = key + lifespan -(vInt) + maxIdle (vInt) + version + value. Responses use status `0x01` = -"not done, key was modified", `0x02` = "key does not exist". - ## How this project's docs work (the process) The full per-session workflow — start-of-session re-orientation, what to update From 82d09c8b740921648d95afa982d395813b20bfd2 Mon Sep 17 00:00:00 2001 From: rigazilla Date: Mon, 19 Oct 2026 18:34:51 +0200 Subject: [PATCH 3/3] test: run all integration suites against :latest; docs: log #9/#12 milestone Align the multi-node cluster fixture with the single-server/auth fixtures: test-configs/docker-compose-cluster.yml now uses quay.io/infinispan/server:latest (was pinned 16.2) for all four nodes, so every integration suite exercises the same (newest) server. The quickstart example stays pinned to 16.2 (it's a demo, not a test). Verified green: the full multi-node suite set (Topology, HashAware, ConcurrentClients, ConcurrentMultiServer, RetryView, PingRetry, MultiSeed) passes 7/7 against :latest = 16.3. Docs: STATUS.md "integration tests pull :latest" bullet now notes the cluster fixture too; DECISIONS.md records the alignment (rationale + risk/mitigation); README requirements line corrected (only the quickstart pins 16.2) and the milestone log gains a 2026-10 entry for Hot Rod URI (#9) + multi-server seed list (#12), which shipped between the Step 11 and Step 12 milestones. Co-Authored-By: Claude Opus 4.8 --- README.md | 5 ++++- docs/DECISIONS.md | 21 +++++++++++++++++++++ docs/STATUS.md | 7 ++++--- test-configs/docker-compose-cluster.yml | 8 ++++---- 4 files changed, 33 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 9484bdc..7c9f027 100644 --- a/README.md +++ b/README.md @@ -64,7 +64,8 @@ See [`examples/quickstart/README.md`](examples/quickstart/README.md) for full do - **Google Test** (for testing) **Runtime Requirements**: -- Infinispan Server 16.0+ (tested with 16.2) +- Infinispan Server 16.0+ (all integration suites run against `:latest` — + currently 16.3; only the quickstart example pins 16.2) - Docker (for integration tests and quickstart example) ### Linux (Fedora/RHEL) @@ -370,6 +371,8 @@ In short, following the hotrod-foundry porting guidelines: - ✅ **v0.8.0** (2026-09-20): Metadata / version-based operations (Step 10) - ✅ **v0.9.0** (2026-09-25): Typed error handling + user-decided retry, keyed & keyless (Step 11) +- ✅ **2026-10** (issue-based): Hot Rod URI `fromUri` (#9) + multi-server seed list + with startup/runtime failover (#12) - 🎯 **v0.10.0** (upcoming): Bulk operations (Step 12) --- diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index c0a2651..6bdae75 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -1054,3 +1054,24 @@ Java `ReplaceIfUnmodifiedOperation` + a corrected reading of the schema. **Why it matters:** if someone later "fixes" the client to match the `.ksy` here, replaceWithVersion's lifespan/maxIdle would silently swap. The `.ksy` is still wrong on this point; fix pending upstream in hotrod-dissector. + +## 2026-10-19 — Align all test fixtures to `:latest` (cluster compose was pinned 16.2) + +**What:** `test-configs/docker-compose-cluster.yml` (the multi-node fixture used by +`start_cluster.sh`/`add_cluster_node.sh` for the topology/hash/retry/concurrent/ +multi-seed suites) pinned `quay.io/infinispan/server:16.2`, while the single-server +and auth fixtures (`scripts/start_infinispan_noauth.sh`, `start_infinispan_auth.sh`) +already used `:latest`. Changed all four cluster node images to `:latest` so every +integration suite exercises the same (newest) server. The quickstart example +(`examples/quickstart/start-server.sh`) intentionally stays pinned to 16.2 — it's a +copy-paste demo, not a test. + +**Why:** the split meant different suites tested different server versions (so a +multi-node regression on a new server could hide until a cluster run happened to use +it). STATUS.md already claimed "integration tests pull `:latest`", which was an +overclaim until now. + +**Risk + mitigation:** `:latest` can drift and break (see the 2026-10-12 CLI-hang +entry) — the `TIMEOUT 300` guard per ctest suite plus `cli.sh -f -` batch mode keep +a server-side change failing fast rather than hanging CI. Verified the full +multi-node suite set green against `:latest` = 16.3.0 on 2026-10-19 after the switch. diff --git a/docs/STATUS.md b/docs/STATUS.md index 57af289..e2a4171 100644 --- a/docs/STATUS.md +++ b/docs/STATUS.md @@ -145,11 +145,12 @@ Rod URI `fromUri` (#9), multi-server seed list + failover (#12)._ Windows and Fedora stay unit-only (Fedora runs inside a container with no Docker daemon; Windows doesn't build the integration tests). Windows/MSVC portability and `-Werror` build parity also landed (Sept 2026). -- **Integration tests pull `quay.io/infinispan/server:latest`** (unpinned). +- **All integration tests pull `quay.io/infinispan/server:latest`** (unpinned) — + single-server, auth, and (as of 2026-10-19) the multi-node cluster fixture + (`test-configs/docker-compose-cluster.yml`), which previously pinned 16.2. Cache setup goes through `cli.sh -c -f -` (batch mode, exits on EOF) and each ctest integration suite has `TIMEOUT 300`, so a server/CLI-version change - that stalls a subprocess fails fast instead of hanging the whole CI job. Full - suite verified green against `:latest` = 16.3.0 on 2026-10-12. See + that stalls a subprocess fails fast instead of hanging the whole CI job. See [`DECISIONS.md`](DECISIONS.md). **Test status (verified 2026-10-19):** **250 unit** + **96 integration** (20 diff --git a/test-configs/docker-compose-cluster.yml b/test-configs/docker-compose-cluster.yml index 249def0..b801d27 100644 --- a/test-configs/docker-compose-cluster.yml +++ b/test-configs/docker-compose-cluster.yml @@ -14,7 +14,7 @@ networks: services: # Node 1: Primary node, always started first ispn-node1: - image: quay.io/infinispan/server:16.2 + image: quay.io/infinispan/server:latest container_name: ispn-node1 hostname: ispn-node1 networks: @@ -35,7 +35,7 @@ services: # Node 2: Second node ispn-node2: - image: quay.io/infinispan/server:16.2 + image: quay.io/infinispan/server:latest container_name: ispn-node2 hostname: ispn-node2 networks: @@ -60,7 +60,7 @@ services: # Node 3: Third node (optional, for 3-node tests) ispn-node3: - image: quay.io/infinispan/server:16.2 + image: quay.io/infinispan/server:latest container_name: ispn-node3 hostname: ispn-node3 networks: @@ -85,7 +85,7 @@ services: # Node 4: Fourth node (optional, for 4-node tests) ispn-node4: - image: quay.io/infinispan/server:16.2 + image: quay.io/infinispan/server:latest container_name: ispn-node4 hostname: ispn-node4 networks: