Improved cluster replica tracking and management

[carlhoerberg]

Summary

• Track replicas using individual etcd keys instead of comma-separated ISR list
• Show all known replicas in Nodes page with Connected/In-sync status columns
• Add ability to forget disconnected replicas via UI and CLI
• Generate sequential cluster node IDs using atomic etcd transactions

Changes

Use individual etcd keys for ISR tracking

Instead of maintaining a comma-separated list at a single "isr" key, each replica now has its own key at {prefix}/replica/{id}/insync with value "1" for in-sync and "0" for out-of-sync.

Show all known replicas in nodes page

• Add known_replicas method to Clustering::Server that queries etcd
• Merge etcd replicas with connected followers in NodesController
• Add Connected and In-sync columns to followers table
• Show leader role in replicas table, sorted first

Add forget button for disconnected replicas

• Allow removing disconnected replicas via Nodes page UI or DELETE /api/nodes/:id
• Prevent forgetting leader node
• Add lavinmqctl forget_cluster_node <id> CLI command

Generate sequential cluster node IDs

• New nodes get sequential IDs (1, 2, 3, ...) instead of random IDs
• Uses atomic etcd transactions to ensure IDs don't collide
• Handles manually created nodes by checking for existing replica keys

Test plan

• Start a 3-node cluster and verify nodes get IDs 1, 2, 3
• Verify Nodes page shows all replicas with correct Connected/In-sync status
• Stop a follower and verify it shows as disconnected
• Use Forget button to remove disconnected replica
• Verify lavinmqctl forget_cluster_node works
• Restart forgotten node and verify it gets a new sequential ID

🤖 Generated with Claude Code

[claude]

Claude finished @carlhoerberg's task —— View job

---

Code Review Complete

• Read and analyze changed files for bugs, security issues, performance problems, and Crystal anti-patterns

Issues Found

1. Missing Error Handling in Etcd Operations

File: src/lavinmq/clustering/server.cr:149-161

• The known_replicas method calls @etcd.get_prefix(prefix) but doesn't handle potential etcd communication failures
• Impact: Network failures or etcd outages could cause unhandled exceptions
• Recommendation: Add exception handling around etcd operations

2. Potential Integer Overflow in Sequential ID Generation

File: src/lavinmq/clustering/controller.cr:81-89

• The next_cluster_node_id method increments id indefinitely without bounds checking
• Impact: In theory, could overflow after Int32::MAX iterations (though unlikely in practice)
• Recommendation: Add reasonable upper bound check or use Int64

3. Missing Validation in Node Forgetting API

File: src/lavinmq/http/controller/nodes.cr:163-184

• The delete "/api/nodes/:id" endpoint converts string to Int32 but only checks for nil, not parsing errors
• Invalid base-36 strings could cause exceptions
• Recommendation: Add proper error handling for malformed ID strings

4. Race Condition in Follower Connection Handling

File: src/lavinmq/clustering/server.cr:194-202

• When a follower with existing ID connects, the old follower is removed from @followers array without synchronization with etcd state updates
• Impact: Brief window where etcd may still show old follower as in-sync while new follower is connecting
• Recommendation: Ensure etcd state is updated atomically with follower replacement

5. Performance: Inefficient String Operations

File: src/lavinmq/clustering/server.cr:152-158

• Multiple string splits and repeated array operations when parsing etcd keys
• Impact: Performance degradation with many replicas
• Recommendation: Use more efficient parsing or cache parsed results

No Issues Found For:

• Security vulnerabilities
• Major Crystal anti-patterns
• Missing memory management issues

[carlhoerberg]

Fixes #1425

[snichme]

I looked at the code in Clustering::Server but haven't worked with that code so cannot give a good review there. Will run the code locally as well.

Possible to add some more specs, feel like there is a lot of logic added but not much specs around that

[snichme]

Does this mean that we change the response? Meaning the keys for each follower is not the same? If so this is a breaking change. We could handle it like that or add merged_replicas as additional data in the response like so

Suggested change

	followers: merged_replicas,
	followers: @amqp_server.followers,
	merged_replicas: merged_replicas

[snichme]

How can a connected follower not be in etcd?

[snichme]

We should rescue Etcd::Error and re-raise with something more specific here to improve the error handling.

[snichme]

If we are insync vid legacy ISR key, shouldn't we write that we are insync in with the new key as well?

[claude]

PR Review: Sequential cluster node IDs and per-replica insync tracking

1. Fiber and connection leak in wait_to_be_insync (controller.cr:159-186)

Two fibers are spawned to concurrently watch the new per-replica key and the legacy comma-separated ISR key. When one detects the insync state, it closes the channel and breaks out of its watch loop. However, the other fiber remains blocked inside @etcd.watch(), which holds an open streaming TCP connection to etcd. There is no cancellation mechanism, so this leaked fiber and its HTTP connection persist for the lifetime of the process.

Consider sharing a cancellation signal (e.g., closing the underlying socket or using an Atomic flag checked inside the watch callback) so the losing fiber can also exit.

2. Etcd network calls in hot path under lock (server.cr:287-289)

each_follower is invoked by append, replace_file, and delete_file -- the message publishing and delivery hot paths. The new code calls mark_out_of_sync for each entry in @disconnected_followers while holding @lock. Each mark_out_of_sync call makes 2 HTTP round-trips to etcd (one put for the replica key, one put for the legacy ISR update). With N disconnected followers, this is 2N synchronous network calls blocking all message replication under the mutex.

The old code performed at most 1 etcd call (update_isr) in the same position.

Suggestion: drain @disconnected_followers into a local variable inside the lock, then perform the etcd calls outside the lock, or batch the updates.

3. known_replicas key parsing breaks if prefix contains slashes (server.cr:153-157)

The code splits the full etcd key by / with a limit of 4 and assumes parts[2] is the replica ID. The clustering_etcd_prefix is user-configurable (default "lavinmq"). If it contains a slash (e.g., "my/prefix"), the key "my/prefix/replica/1a/insync" splits to ["my", "prefix", "replica", "1a/insync"]. Then parts[3] is "1a/insync" (not "insync"), so no replicas are ever matched.

Fix: strip the known prefix from the key before parsing the remainder.