feat(cluster): control plane on the default runtime + formation fixes (v0.9 W0/C-1, #405) - #450
Merged
Merged
Conversation
… (v0.9 W0/C-1, #405) The cluster control plane (bus listener on port+10000, 100ms gossip ticker, failover election) was tokio-only: the monoio startup path — the default build — never spawned it, so `--cluster-enabled` accepted CLUSTER MEET but no peer ever learned anything. Red test first: a 3-node monoio fleet stuck at known_nodes [3, 1, 1] forever. Control plane placement (C-1): - One tokio-native implementation, spawned on a dedicated `cluster-ctl` std thread hosting a current-thread tokio runtime — on BOTH runtimes. Under monoio there is no tokio runtime to share; under tokio, sharing the listener runtime made gossip compete with the accept loop and every connection, which starved the ticker under load (observed as PFAIL detection stalling in the e2e until the thread was dedicated). Not latency-critical, so no shard-thread involvement; O5 aux pinning applies. - The never-called, never-tested monoio duplicates of run_cluster_bus / run_gossip_ticker / run_election_task (and monoio_read_exact) are DELETED per the milestone plan ("do NOT port to monoio: !Send task model, zero payoff") — the tokio variants are the ones all CI has ever exercised. Base tokio features gain "time" + "io-util" (the control plane's only new footprint in the monoio build). - Shared wiring extracted to run_cluster_control_plane() in main.rs; the tokio block's inline duplicate is gone. Formation fixes (pre-existing product bugs, both runtimes — found by the new e2e, which cannot pass without them): - CLUSTER MEET registers the peer under a RANDOM placeholder id ("replaced by handshake") that nothing ever replaced: the handshake merged the real id as a NEW entry and the placeholder lived forever (known_nodes 5 in a 3-node cluster). merge_gossip_into_state now retires same-address/different-id entries when a sender's real identity arrives — also covers a node restarting with a fresh id. - Gossip sections were consumed ONLY for PFAIL/FAIL reports: two nodes MEET-ed into a common seed never learned about each other, so the mesh could not complete. Healthy rumors are now adopted, guarded against self-address rumors (a peer's unresolved placeholder for us) and already-known addresses (the real entry wins). - Rumor-adopted nodes started with pong_recv_ms = 0, which check_failure_states skips — a rumored node that died before first direct contact could NEVER go PFAIL on that observer (reproduced as one survivor permanently not flagging a killed peer). Adoption and the MEET placeholder now stamp a freshness baseline so the staleness clock always runs. Tests: - tests/cluster_formation.rs (new, runs against MOON_BIN, both runtimes): 3-node formation via one seed's MEETs — the load-bearing assertion is nodes 2/3 resolving ALL THREE real node ids (identity convergence, not just known_nodes, which placeholder rumors can satisfy); plus a kill test asserting both survivors flag the victim pfail within the node timeout. Hard FAIL needs quorum ≥ 2 EXTERNAL reporters, unreachable for 2 survivors of 3 masters — full FAIL/election e2e lands with C-3's replica legs. - Unit tests pin the three merge behaviors (placeholder retirement, rumor adoption incl. bus_port + freshness baseline, self/known-addr rumor rejection). - e2e stability: 8/8 consecutive green (4× monoio, 4× tokio) after the fixes; each of the three product bugs was watched failing first. Gates: fmt; clippy -D warnings (default + tokio,jemalloc); macOS full monoio suite; VM Linux lib + e2e on both runtimes. Bench gates waived: no hot-path change — the control plane thread exists only with --cluster-enabled, and the merge fixes run at gossip rate (10 Hz). Refs #405 author: Tin Dang
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe cluster control plane now runs on a dedicated Tokio thread for both runtimes. Gossip convergence handles placeholders and healthy rumors correctly. New end-to-end tests validate three-node formation, bus traffic, and failure detection. ChangesCluster control plane and convergence
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Main as main
participant Control as cluster-ctl
participant Bus as cluster bus
participant Gossip as gossip ticker
participant Nodes as cluster nodes
Main->>Control: start dedicated Tokio runtime
Control->>Bus: run cluster bus
Control->>Gossip: run gossip ticker
Nodes->>Bus: send MEET and gossip traffic
Bus->>Gossip: update cluster state
Gossip->>Nodes: propagate peer rumors
Nodes-->>Gossip: report liveness and failure state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoRun cluster control plane on default runtime and fix 3-node formation
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cluster/gossip.rs (1)
13-13: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep the Tokio feature behind a runtime-independent dependency.
Cargo.tomlonly adds Tokio features underruntime-tokio, butsrc/cluster/gossip.rsandsrc/cluster/bus.rsimporttokio::iounconditionally. Make Tokio available as a base feature for the cluster control-plane module, or gate these imports/exclusions consistently, soruntime-monoiobuilds without failing while the module is selected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cluster/gossip.rs` at line 13, Update the cluster dependency/features configuration and the unconditional Tokio I/O imports used by gossip.rs and bus.rs so runtime-monoio builds successfully when the cluster control-plane module is enabled. Either expose the required tokio::io functionality through the base cluster feature or consistently gate/exclude both imports and their dependent code by runtime, preserving Tokio support.Source: Coding guidelines
🧹 Nitpick comments (3)
src/cluster/gossip.rs (2)
275-296: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider refreshing the sender address on an existing entry.
or_insert_withsetsaddronly at insert time. If a known peer moves to a new address, the entry keeps the stale address, and the gossip ticker keeps probing the oldaddr/bus_port. The retain below removes other entries atsender_addr, so the stale entry survives as the only one and never self-heals.♻️ Proposed refresh of address and bus port
entry.pong_recv_ms = now_ms(); + entry.addr = sender_addr; + entry.bus_port = msg.sender_bus_port; if msg.config_epoch > entry.epoch {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cluster/gossip.rs` around lines 275 - 296, Update the existing-node path around the state.nodes entry lookup to refresh the matched ClusterNode address and bus port from the current sender information, not only when ClusterNode::new inserts it. Preserve the insertion behavior while ensuring known peers that move are subsequently probed at their new address.
336-371: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffRumor adoption has no bound on the node map.
Every gossip section arrives from the cluster bus, which accepts any TCP peer. Each new
(node_id, addr)pair now inserts aClusterNode. A hostile or misconfigured peer can therefore growstate.nodeswithout limit, and every entry is then probed by the ticker rotation.Consider a cap on adopted nodes, or accept rumors only from senders already known through
CLUSTER MEET.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cluster/gossip.rs` around lines 336 - 371, Bound rumor adoption in the unknown-node branch around ClusterNode::new and state.nodes.insert: only insert an adopted node when the node map remains within an explicit configured cap, or require the rumor sender to be an already-known CLUSTER MEET peer. Preserve existing address, port, self-address, and duplicate checks, and skip adoption when the trust or capacity condition is not met.src/main.rs (1)
1903-1931: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueThe
cluster-ctlthread handle is dropped and never joined.
cancel_token.cancel()at shutdown stops the ticker, but the process does not wait for the bus listener and its peer tasks to finish. In-flight gossip and vote handling can be cut mid-write. Keeping theJoinHandleand joining it after the shard joins would make shutdown deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.rs` around lines 1903 - 1931, The cluster control-plane thread spawned in the cluster_state block is not retained or joined during shutdown. Store the std::thread::JoinHandle returned by spawn, then join it after the shard threads have joined and cancellation has completed, preserving the existing run_cluster_control_plane execution and propagating or handling join failures consistently with nearby shutdown joins.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cluster/command.rs`:
- Around line 277-284: Update the placeholder insertion logic in the CLUSTER
MEET handling around ClusterNode::new to check for an existing node with the
same address, rather than only checking state.nodes by the freshly generated
peer_id. Reuse or skip insertion when that address is already present, while
preserving insertion for genuinely new addresses and the existing handshake
behavior.
In `@src/main.rs`:
- Around line 2101-2129: Update the tokio task invoking run_cluster_bus so an
error from that function, including bind failure, cancels the node’s control
plane or otherwise aborts startup instead of only logging and allowing
run_gossip_ticker to continue. Use the existing cancel_token/control-flow
mechanisms around run_cluster_bus and run_gossip_ticker, while preserving the
error log before triggering shutdown.
- Line 2085: Validate the port before computing cluster_port in the surrounding
startup flow: reject any port above 55535 with the existing configuration-error
handling, then perform the addition only for valid values so the result cannot
truncate when cast to u16. Preserve the current cluster-port behavior for ports
within the valid range.
In `@tests/cluster_formation.rs`:
- Around line 23-53: Extend the retry logic in the shared startup flow around
spawn_cluster_node and connect_retry to monitor child liveness while connecting;
when a node exits before accepting either its client port or +10000 cluster-bus
sibling, reserve a fresh port pair, respawn the affected fleet, and retry with
the new ports instead of waiting for the full deadline.
---
Outside diff comments:
In `@src/cluster/gossip.rs`:
- Line 13: Update the cluster dependency/features configuration and the
unconditional Tokio I/O imports used by gossip.rs and bus.rs so runtime-monoio
builds successfully when the cluster control-plane module is enabled. Either
expose the required tokio::io functionality through the base cluster feature or
consistently gate/exclude both imports and their dependent code by runtime,
preserving Tokio support.
---
Nitpick comments:
In `@src/cluster/gossip.rs`:
- Around line 275-296: Update the existing-node path around the state.nodes
entry lookup to refresh the matched ClusterNode address and bus port from the
current sender information, not only when ClusterNode::new inserts it. Preserve
the insertion behavior while ensuring known peers that move are subsequently
probed at their new address.
- Around line 336-371: Bound rumor adoption in the unknown-node branch around
ClusterNode::new and state.nodes.insert: only insert an adopted node when the
node map remains within an explicit configured cap, or require the rumor sender
to be an already-known CLUSTER MEET peer. Preserve existing address, port,
self-address, and duplicate checks, and skip adoption when the trust or capacity
condition is not met.
In `@src/main.rs`:
- Around line 1903-1931: The cluster control-plane thread spawned in the
cluster_state block is not retained or joined during shutdown. Store the
std::thread::JoinHandle returned by spawn, then join it after the shard threads
have joined and cancellation has completed, preserving the existing
run_cluster_control_plane execution and propagating or handling join failures
consistently with nearby shutdown joins.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 063597bd-d029-4aa0-a22f-a5b168b5c5a9
📒 Files selected for processing (8)
CHANGELOG.mdCargo.tomlsrc/cluster/bus.rssrc/cluster/command.rssrc/cluster/failover.rssrc/cluster/gossip.rssrc/main.rstests/cluster_formation.rs
💤 Files with no reviewable changes (2)
- src/cluster/bus.rs
- src/cluster/failover.rs
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…rt refusal, ctl-thread supervision Addresses the CodeRabbit + Qodo findings on #450: - CLUSTER MEET is idempotent by ADDRESS: the placeholder id is fresh random per call, so the old contains_key(&peer_id) check never fired and every repeated MEET for one address stacked another placeholder. Also refuses MEET-ing the node's own advertised address (a self placeholder can never be retired — the handshake's sender==self early-return skips the merge that kills placeholders). - Cluster bus bind failure aborts startup: the listener is now bound in run_cluster_control_plane BEFORE anything spawns, and EADDRINUSE (or any bind error) exits the process — a cluster node without its bus keeps serving clients while being invisible to every peer, the half-alive state both bots flagged. run_cluster_bus takes the bound listener and no longer returns a Result. - --cluster-enabled refuses --port > 55535 at startup (REFUSING TO START, exit 2): the bus port is port + 10000 and previously wrapped silently to a port no peer would ever compute. - cluster-ctl thread supervision: the existing shard-panic abort hook now also matches the cluster-ctl thread, so a control-plane panic aborts the process instead of leaving the node cluster-deaf. With the bind fail-loud change, the only remaining thread-exit paths are graceful shutdown (cancel) and abort. Tests: unit (MEET idempotence ×3 calls, self-MEET error) + e2e occupied_bus_port_aborts_startup (bus sibling held by the harness → child must exit nonzero) + wire-level repeat-MEET / self-MEET asserts in the formation e2e. Gates: fmt; clippy -D warnings ×2 feature sets; cluster unit 54/54; cluster_formation e2e 3/3 on BOTH runtime binaries. Refs #405 author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Aug 14, 2026
CodeRabbit finding on #486, verified against the encoding rather than taken at face value. It is real in both directions: flags | v1 meaning | v2 meaning | consequence ------|------------|-------------------|----------------------------------- 3 | FAIL | replica + PFAIL | v2 reader downgrades a CONFIRMED | | | failure to a mere suspicion 4,5 | (unused) | master/replica | v1 reader tests `== 2 || == 3` | | + FAIL | exactly, so the failure is INVISIBLE `GOSSIP_VERSION` was serialized and then thrown away at [8..10] with the comment "we don't enforce version for forward compat". That is fine while an encoding only ever gains values; it is not fine when values change meaning. Bumps GOSSIP_VERSION 1 -> 2 and translates v1 sections at the wire boundary, so nothing downstream ever sees a v1 discriminant. Unknown/future versions are still parsed as before — the header layout is fixed and unknown health bits already decode as healthy. The reverse direction cannot be fixed from this side: a v1 peer ignores the version too, so it will read our FAIL as healthy no matter what we send. That is accepted rather than worked around, and logged loudly when a v1 peer is seen. No v1 population exists to protect: a multi-node cluster could not form at all before #450, so no released build ever exchanged these bytes in anger. Two tests. One pins the collision itself (`parse_gossip_flags(3)` IS replica+PFAIL) so the translation cannot be "simplified" away later; the other stamps a v1 version into a serialized message and asserts the FAIL rumor survives deserialization as FAIL. Also re-walks the ADD tests->build crossing so the cached scope anchor picks up `tests/cluster_formation.rs`, which the amended §5 scope already listed — the second CodeRabbit finding, and one this task file had flagged against itself. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Aug 14, 2026
CodeRabbit finding on #486, verified against the encoding rather than taken at face value. It is real in both directions: flags | v1 meaning | v2 meaning | consequence ------|------------|-------------------|----------------------------------- 3 | FAIL | replica + PFAIL | v2 reader downgrades a CONFIRMED | | | failure to a mere suspicion 4,5 | (unused) | master/replica | v1 reader tests `== 2 || == 3` | | + FAIL | exactly, so the failure is INVISIBLE `GOSSIP_VERSION` was serialized and then thrown away at [8..10] with the comment "we don't enforce version for forward compat". That is fine while an encoding only ever gains values; it is not fine when values change meaning. Bumps GOSSIP_VERSION 1 -> 2 and translates v1 sections at the wire boundary, so nothing downstream ever sees a v1 discriminant. Unknown/future versions are still parsed as before — the header layout is fixed and unknown health bits already decode as healthy. The reverse direction cannot be fixed from this side: a v1 peer ignores the version too, so it will read our FAIL as healthy no matter what we send. That is accepted rather than worked around, and logged loudly when a v1 peer is seen. No v1 population exists to protect: a multi-node cluster could not form at all before #450, so no released build ever exchanged these bytes in anger. Two tests. One pins the collision itself (`parse_gossip_flags(3)` IS replica+PFAIL) so the translation cannot be "simplified" away later; the other stamps a v1 version into a serialized message and asserts the FAIL rumor survives deserialization as FAIL. Also re-walks the ADD tests->build crossing so the cached scope anchor picks up `tests/cluster_formation.rs`, which the amended §5 scope already listed — the second CodeRabbit finding, and one this task file had flagged against itself. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Aug 14, 2026
…they did not own (#485) (#486) * fix(cluster): peer slot ownership never merged, so nodes served keys they did not own (#485) A multi-node cluster silently accepted writes for slots belonging to other nodes. `CLUSTER ADDSLOTS` does not bump the config epoch, so a hand-built cluster sits at epoch 0 forever; the gossip merge accepted a peer's slot bitmap only at a strictly higher epoch (`0 > 0` is false), so peer ownership was never merged and each node believed the only slots in existence were its own. `route_slot` then found no owner for a peer's slot and fell through to a bootstrap fallback that serves anything unclaimed locally. The failure mode was the worst one available: the write returned `+OK`, landed on the wrong node, and was invisible to the node that owned the slot. No MOVED, no error, nothing the client could detect. Batch 0 — split role from health -------------------------------- `NodeFlags` had mutually exclusive `Master` / `Replica` / `Pfail` / `Fail` variants, so marking a node PFAIL destroyed its role and, for a replica, the `master_id` saying which shard it belonged to. Redis treats the two as orthogonal: a dead master is `role: master, health: fail`, a shape the old type could not express. Split into `NodeRole` + `NodeHealth`. Both axes now travel together in the gossip flags word (bit 0 role, bits 1-2 health), keeping the wire header at its fixed 2130 bytes. Unknown health bits decode as Online so a forward-compatible peer is never treated as failed by accident. The old rumor test `flags == 2 || flags == 3` would have read a suspected *replica* (0b011) as a confirmed failure; rumors are now read through `parse_gossip_flags`. CLUSTER NODES and nodes.conf render both axes in Redis's own spelling, measured against redis-server (3 masters, node-timeout 15000, SHUTDOWN NOSAVE, polled at 100ms): the victim's flags column goes `master` -> `master,fail?` -> `master,fail`, and nodes.conf persists `master,fail`. Moon previously emitted `pfail`, a token Redis never produces and no client parses. Batch 1 — make routing honest ----------------------------- - Gossip merges a peer's bitmap at *equal* epoch. A node is authoritative for its own slots; a strictly lower epoch is still ignored, preserving Redis's highest-epoch-wins tie-break for genuinely conflicting claims. - Direct contact with a node clears suspicion about it. Nothing else ever reset health — `check_failure_states` only ever sets PFAIL — so a recovered node stayed suspected forever and its slots never counted as covered again. - The unclaimed-slot fallback is split by cluster size. A lone node still serves locally, so single-node bootstrap keeps working; a formed cluster returns the new `SlotRoute::Down` -> `CLUSTERDOWN The cluster is down` instead of inventing an answer. Both dispatch gates already funnel non-Local routes through `into_error_frame`, and the inline fast path is fully stood down in cluster mode (deep-review R6), so there is no third-path hole. Tests ----- New suite `tests/cluster_client_bootstrap.rs` (18 tests, written red before the fix). cb1/cb2/cb3 flip green here and cb4 — the single-node fallback the split narrows — stays green. Identical results on both runtime legs. The 14 tests covering CLUSTER SHARDS, MYSHARDID, READONLY/READWRITE, cluster_state and INFO identity are `#[ignore]`d with their owning batch named in the reason string; each is un-ignored by the batch that makes it pass. `cluster_formation.rs::killed_node_is_flagged_by_survivors` asserted the token `pfail`. Its expected literal moved to `fail?`/`fail` — toward the measured oracle — with the assertion's strength unchanged: still both survivors, same deadline, still an exact token match. Recorded in the task's §5 with the measurement, because "a test outside the suite failed and I edited it" is the shape of weakening a test and has to be shown not to be that. Verified: cargo fmt; clippy `--all-targets` clean on both feature legs; lib tests 4620 (monoio) / 3786 (tokio) passing; cluster_formation 3/3 and cluster_client_bootstrap 4 passed / 14 ignored on both runtimes. Refs #485 author: Tin Dang * test(cluster): restore the failed-master premise in the promote-epoch test Batch 0's rename turned `NodeFlags::Fail` into `NodeRole::Master` in this test's setup and dropped the health, so the scenario no longer stated the thing it is about — a failover FROM a failed master. The assertions still passed because `promote_self_to_master` does not gate on health, which is exactly why it was easy to miss. Sets `health = Fail` in the setup and adds the post-condition that the demoted master keeps `role = master` while staying FAIL. That pairing is inexpressible under the old single enum and is the reason for the split, so it deserves a direct assertion rather than only being implied. author: Tin Dang * fix(cluster): an unclaimed slot must say "Hash slot not served", not "The cluster is down" Redis has TWO CLUSTERDOWN messages and a client can tell them apart: CLUSTERDOWN Hash slot not served -- THIS slot has no owner; the cluster may be perfectly healthy otherwise CLUSTERDOWN The cluster is down -- cluster_state is fail `SlotRoute::Down` shipped the second one for the first condition. Conflating them tells an operator the whole cluster is down when a single slot lost its owner. Measured against redis-server: 3 masters, cluster-require-full-coverage no, `CLUSTER DELSLOTS 12182` on the owner, then GET and SET of a key hashing to 12182 on that same node. Both answered `CLUSTERDOWN Hash slot not served`, and cluster_state stayed `ok` — which is what confirms the per-slot reading rather than a cluster-wide one. A peer that had not yet learned of the DELSLOTS still answered `MOVED 12182 127.0.0.1:7403`, so this message is the ex-owner's own reply and not a redirect. Renames the variant to `SlotRoute::SlotNotServed` so the two conditions cannot be confused at the call site again, and adds `cb21` to pin both the read and the write path. The task contract froze the wrong text at v1 — it was derived from the fail-closed clause's wording instead of measured, which is exactly what that contract's header forbids. Recorded as AMENDMENT 1 against the frozen §3 rather than silently edited: the build was written to v1 and had to change to satisfy v2, so this is a correction toward the oracle the contract names as its own authority, not a relaxation to accommodate the build. The fail-closed clause is unchanged and still correct — `The cluster is down` belongs there, and batch 3 still owns it. author: Tin Dang * docs(changelog): name the measured per-slot CLUSTERDOWN text author: Tin Dang * fix(cluster): honour the gossip wire version — v1 and v2 flags collide CodeRabbit finding on #486, verified against the encoding rather than taken at face value. It is real in both directions: flags | v1 meaning | v2 meaning | consequence ------|------------|-------------------|----------------------------------- 3 | FAIL | replica + PFAIL | v2 reader downgrades a CONFIRMED | | | failure to a mere suspicion 4,5 | (unused) | master/replica | v1 reader tests `== 2 || == 3` | | + FAIL | exactly, so the failure is INVISIBLE `GOSSIP_VERSION` was serialized and then thrown away at [8..10] with the comment "we don't enforce version for forward compat". That is fine while an encoding only ever gains values; it is not fine when values change meaning. Bumps GOSSIP_VERSION 1 -> 2 and translates v1 sections at the wire boundary, so nothing downstream ever sees a v1 discriminant. Unknown/future versions are still parsed as before — the header layout is fixed and unknown health bits already decode as healthy. The reverse direction cannot be fixed from this side: a v1 peer ignores the version too, so it will read our FAIL as healthy no matter what we send. That is accepted rather than worked around, and logged loudly when a v1 peer is seen. No v1 population exists to protect: a multi-node cluster could not form at all before #450, so no released build ever exchanged these bytes in anger. Two tests. One pins the collision itself (`parse_gossip_flags(3)` IS replica+PFAIL) so the translation cannot be "simplified" away later; the other stamps a v1 version into a serialized message and asserts the FAIL rumor survives deserialization as FAIL. Also re-walks the ADD tests->build crossing so the cached scope anchor picks up `tests/cluster_formation.rs`, which the amended §5 scope already listed — the second CodeRabbit finding, and one this task file had flagged against itself. author: Tin Dang * test(cluster): pin nodes.conf round-trip for every role x health pairing The flags column is one comma-separated string parsed by substring, and `"fail?"` CONTAINS `"fail"`. Testing them in the wrong order silently reloads every PFAIL node as a confirmed FAIL on restart — a durability-path corruption with no error and no log line. The order is correct today and nothing pinned it. `test_nodes_conf_roundtrip` covers a healthy master with slots, which is precisely the shape that lets the bug through: swapping the two branches leaves it green. Verified the new test is not vacuous by actually swapping the branches — it fails with `master-pfail: health did not survive`, while the pre-existing round-trip test stays green through the same swap. Covers all six role x health pairings, including the ones the old single `NodeFlags` enum could not represent (a failed node that is still a master), and asserts a replica keeps its master_id — a replica that forgets it cannot be grouped into a shard, which is what CLUSTER SHARDS needs in batch 4. author: Tin Dang
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First PR of the v0.9 milestone (Wave 0, C-1 — #405): the cluster control plane (bus listener on
port+10000, 100 ms gossip ticker, failover election) now runs under the default monoio runtime. Red test first: a 3-node monoio fleet was stuck atknown_nodes [3, 1, 1]forever — MEET wrote local state that no peer ever learned about.Control plane placement (C-1)
cluster-ctlstd thread hosting a current-thread tokio runtime — on BOTH runtimes. Under monoio there is no tokio runtime to share; under tokio, sharing the listener runtime made gossip compete with the accept loop and every connection, which starved the ticker under load (observed as PFAIL detection stalling in the new e2e until the thread was dedicated).run_cluster_bus/run_gossip_ticker/run_election_task(+monoio_read_exact) are deleted per the milestone plan ("do NOT port to monoio: !Send task model, zero payoff") — net −232 lines insrc/. Base tokio features gaintime+io-util.run_cluster_control_plane(); the tokio block's inline duplicate removed.Formation fixes (pre-existing product bugs, both runtimes)
Found by the new e2e — a real 3-node cluster had never formed on either runtime (nothing ever tested it end-to-end):
CLUSTER MEETregisters the peer under a random placeholder id; the handshake merged the real id as a NEW entry and the placeholder lived forever (known_nodes5 in a 3-node cluster). Handshake now retires same-address/different-id entries (also covers a node restarting with a fresh id).pong_recv_ms = 0, whichcheck_failure_statesskips — a rumored node that died before first direct contact was permanently unflaggable (reproduced as one survivor never flagging a killed peer). Adoption and the MEET placeholder now stamp a freshness baseline.Verification (C-1 exit: "3-node monoio cluster forms, gossips, elects")
tests/cluster_formation.rs(spawns realMOON_BINbinaries): 3-node formation via one seed's MEETs — the load-bearing criterion is identity convergence (all three real node ids resolved on every node, which placeholder rumors can't satisfy); plus a kill test asserting both survivors flag the victimpfailwithin the node timeout. Hard FAIL needs quorum ≥ 2 external reporters — unreachable for 2 survivors of 3 masters — so full FAIL/election e2e lands with C-3's replica legs.Gates
-D warnings(default +tokio,jemalloc)client_tracking_invalidationclient_tracking_invalidation: multikey DEL/MSET second-key invalidation push intermittently not delivered (~25-40%/run) #448 pre-existing flake, solo-green)--cluster-enabled, and the merge fixes run at gossip rate (10 Hz).Refs #405
Summary by CodeRabbit
New Features
Bug Fixes
Tests