Merge Serving into Ready, health reports the heartbeat; derive the registry prune age from the leadership window

- Cluster.Ready: an active node that is healthy, or has no other active node to take its clients;
  /v1/ready returns it, /v1/health goes back to the raw heartbeat (for monitoring)
- Prune keeps rows as long as a new node holds off for them (lease + hold-off), not a separate 3x TTL
- Document health/ready semantics and the passive-node limitation

cluster/cluster.go

@@ -111,10 +111,10 @@ type Cluster interface {
 	// fail open (never pull ALL nodes): during a full database outage every node reports
 	// unhealthy while the mesh keeps delivering on stale peer caches.
 	Healthy() bool
-	// Serving reports whether clients should keep connecting to this node: false only while it
-	// is not Healthy and another active node could take its clients. The only active node keeps
-	// serving with a stale heartbeat, since pulling it would leave clients nowhere to go.
-	Serving() bool
+	// Ready reports whether a load balancer should send clients here: an active node that is
+	// Healthy, or that is not but has no other active node to take its clients. The only active
+	// node stays ready with a stale heartbeat, since pulling it would leave clients nowhere to go.
+	Ready() bool
 	// Role reports this node's configured role (active, or passive)
 	Role() Role
 	// Close stops the cluster and releases its resources.

cluster/cluster_mesh.go

@@ -206,10 +206,12 @@ func (c *meshCluster) Role() Role {
 	return c.conf.Role
 }
 
-// Serving reports whether clients should keep connecting here (see the Cluster interface). It
-// reads the last peer snapshot rather than probing peers, since health checks call it often.
-func (c *meshCluster) Serving() bool {
-	if c.Healthy() {
+// Ready reports whether a load balancer should send clients here (see the Cluster interface). It
+// reads the last peer snapshot rather than probing peers, since load balancers call it often.
+func (c *meshCluster) Ready() bool {
+	if c.conf.Role != RoleActive {
+		return false
+	} else if c.Healthy() {
 		return true
 	}
 	c.mu.Lock()

cluster/cluster_mesh_test.go

@@ -650,10 +650,10 @@ func TestMesh_MembersOfAnIsolatedNodeAreNotReportedHealthy(t *testing.T) {
 	}
 }
 
-func TestMesh_ServingWithoutAnActivePeer(t *testing.T) {
-	// The only active node (a production server with passive peers beside it) keeps serving when
-	// its heartbeat fails: there is nowhere better for its clients, so a health check must not
-	// pull it from the load balancer
+func TestMesh_ReadyWithoutAnActivePeer(t *testing.T) {
+	// The only active node (a production server with passive peers beside it) stays ready when
+	// its heartbeat fails: there is nowhere better for its clients, so a load balancer must not
+	// pull it
 	schemaDSN := dbtest.CreateTestPostgresSchema(t)
 	pool := openTestPool(t, schemaDSN)
 	registerFakePeerWithRole(t, pool, "node-passive", "http://192.168.1.51:2587", RolePassive)
@@ -667,10 +667,10 @@ func TestMesh_ServingWithoutAnActivePeer(t *testing.T) {
 	})
 	require.Nil(t, pool.Close()) // Heartbeats fail from here on
 	waitFor(t, func() bool { return !mesh.Healthy() })
-	require.True(t, mesh.Serving())
+	require.True(t, mesh.Ready())
 }
 
-func TestMesh_NotServingWhenAnActivePeerCanTakeOver(t *testing.T) {
+func TestMesh_NotReadyWhenAnActivePeerCanTakeOver(t *testing.T) {
 	schemaDSN := dbtest.CreateTestPostgresSchema(t)
 	pool := openTestPool(t, schemaDSN)
 	registerFakePeer(t, pool, "node-active", "http://192.168.1.52:2587")
@@ -682,10 +682,10 @@ func TestMesh_NotServingWhenAnActivePeerCanTakeOver(t *testing.T) {
 		defer mesh.mu.Unlock()
 		return len(mesh.knownPeers) == 1
 	})
-	require.True(t, mesh.Serving())
+	require.True(t, mesh.Ready())
 	require.Nil(t, pool.Close())
 	waitFor(t, func() bool { return !mesh.Healthy() })
-	require.False(t, mesh.Serving())
+	require.False(t, mesh.Ready())
 }
 
 func TestMesh_PassiveNodeWithoutAnActivePeerWarns(t *testing.T) {
@@ -716,3 +716,16 @@ func TestMesh_PassiveNodeWithAnActivePeerDoesNotWarn(t *testing.T) {
 	defer mesh.Close()
 	require.NotContains(t, out.String(), "no active node is live")
 }
+
+func TestMesh_PassiveNodeIsNeverReady(t *testing.T) {
+	// A passive node is healthy, but takes no clients from a load balancer, alone or not
+	schemaDSN := dbtest.CreateTestPostgresSchema(t)
+	pool := openTestPool(t, schemaDSN)
+	conf := newTestMeshConfig("node-a", "http://192.168.1.10:2587")
+	conf.Role = RolePassive
+	mesh, err := newMeshCluster(conf, pool, nil)
+	require.Nil(t, err)
+	defer mesh.Close()
+	require.True(t, mesh.Healthy())
+	require.False(t, mesh.Ready())
+}

cluster/cluster_nop.go

@@ -39,8 +39,8 @@ func (c *nopCluster) Role() Role {
 	return RoleActive // A single node takes all client traffic
 }
 
-func (c *nopCluster) Serving() bool {
-	return true // A single node always serves
+func (c *nopCluster) Ready() bool {
+	return true // A single node takes all client traffic
 }
 
 func (c *nopCluster) Close() error {

cluster/registry/registry.go

@@ -175,12 +175,12 @@ func (r *Registry) Fresh() bool {
 	return !r.refreshedAt.IsZero() && time.Since(r.refreshedAt) < r.ttl
 }
 
-// Prune deletes registry rows whose heartbeat is long expired. Only the leader calls this; the
-// grace period of 3x the TTL avoids deleting rows of nodes that are merely slow to heartbeat.
+// Prune deletes registry rows whose heartbeat is long expired. Only the leader calls this. It
+// keeps recently live rows (see recent): a new node holds off for those, and must still see them.
 func (r *Registry) Prune() error {
 	ctx, cancel := r.opContext()
 	defer cancel()
-	_, err := r.pool.ExecContext(ctx, pruneStaleNodesQuery, int64((3 * r.ttl).Seconds()))
+	_, err := r.pool.ExecContext(ctx, pruneStaleNodesQuery, int64(r.recent().Seconds()))
 	return err
 }
 
@@ -235,6 +235,12 @@ func (r *Registry) holdoff() time.Duration {
 	return holdoffFactor * r.ttl
 }
 
+// recent is how long after its last heartbeat a node may still believe it leads: its lease, plus
+// the hold-off it waited before it led
+func (r *Registry) recent() time.Duration {
+	return r.lease() + r.holdoff()
+}
+
 // Deregister deletes this node's registry row; called on shutdown.
 func (r *Registry) Deregister() error {
 	ctx, cancel := r.opContext()
@@ -276,8 +282,7 @@ func (r *Registry) Refresh() ([]*Peer, error) {
 func (r *Registry) queryLiveNodes() ([]*Peer, string, bool, error) {
 	ctx, cancel := r.opContext()
 	defer cancel()
-	recent := r.lease() + r.holdoff()
-	rows, err := r.pool.QueryContext(ctx, selectLiveNodesQuery, int64(recent.Seconds()))
+	rows, err := r.pool.QueryContext(ctx, selectLiveNodesQuery, int64(r.recent().Seconds()))
 	if err != nil {
 		return nil, "", false, err
 	}

cluster/registry/registry_test.go

@@ -646,3 +646,17 @@ func TestRegistry_RecentlyLiveActivePeerMeansHoldOff(t *testing.T) {
 	require.False(t, a.IsLeader())
 	waitForLeader(t, a)
 }
+
+func TestRegistry_PruneKeepsRecentlyLiveActiveNodes(t *testing.T) {
+	// A crashed leader's row must outlive the window in which a new node still holds off for it,
+	// or Prune would let the next node lead while the old one may still believe it does
+	t.Parallel()
+	pool := openTestPool(t, dbtest.CreateTestPostgresSchema(t))
+	a, err := New(pool, "app1", "http://10.0.0.1:2587", RoleActive, leaderTestTTL)
+	require.Nil(t, err)
+	insertNodeAt(t, pool, "app0", "http://10.0.0.9:2587", time.Now().Add(-a.lease()-a.holdoff()+leaderTestTTL))
+	require.Nil(t, a.Prune())
+	require.Equal(t, 1, countRows(t, pool, "app0"))
+	tick(t, a)
+	require.False(t, a.IsLeader())
+}

docs/config.md

@@ -2236,9 +2236,14 @@ If a non-200 HTTP status code is returned or if the returned `healthy` field is
 {"healthy":true,"role":"active"}
 ```
 
+Without clustering, the server is healthy whenever it answers; the endpoint does not check the database. With clustering
+(experimental, see the `experimental-cluster-*` options), it is unhealthy while its cluster heartbeat, a database write every
+few seconds, has failed for 30 seconds.
+
 For load balancers, use `/v1/ready` instead: it returns 200 only if the server should receive clients. Without
-clustering that is the same as being healthy. With clustering (experimental, see the `experimental-cluster-*` options), a passive node is healthy
-but not ready, so a load balancer checking `/v1/ready` does not send it any clients.
+clustering that is always the case. With clustering, a passive node is never ready, and an unhealthy active node is not ready
+while another active node can take its clients. The only active node stays ready even when it is unhealthy, since there is
+nowhere better for its clients.
 
 ```json
 {"ready":true,"role":"active"}
@@ -2376,7 +2381,7 @@ variable before running the `ntfy` command (e.g. `export NTFY_LISTEN_HTTP=:80`).
 | `experimental-cluster-advertise-url`       | `NTFY_EXPERIMENTAL_CLUSTER_ADVERTISE_URL`       | *URL*                                               | `http://<experimental-cluster-listen>` | Experimental, not ready for production. URL under which other nodes reach this node's cluster listener.                                                                                             |
 | `experimental-cluster-secret`              | `NTFY_EXPERIMENTAL_CLUSTER_SECRET`              | *string*                                            | -                 | Experimental, not ready for production. Shared secret authenticating node-to-node traffic; must match on all nodes.                                                                                                                      |
 | `experimental-cluster-batch-linger`        | `NTFY_EXPERIMENTAL_CLUSTER_BATCH_LINGER`        | *duration*                                          | `500ms`           | Experimental, not ready for production. How long messages wait to form a node-to-node delivery batch; `0` sends immediately.                                                                                                             |
-| `experimental-cluster-role`                | `NTFY_EXPERIMENTAL_CLUSTER_ROLE`                | `active` or `passive`                               | `active`          | Experimental, not ready for production. A `passive` node never leads the cluster's singleton jobs and never closes its subscribers to send them to another node; other active nodes do not count it as a place for theirs. Use it for a node that is not behind the load balancer. |
+| `experimental-cluster-role`                | `NTFY_EXPERIMENTAL_CLUSTER_ROLE`                | `active` or `passive`                               | `active`          | Experimental, not ready for production. A `passive` node never leads the cluster's singleton jobs and never closes its subscribers to send them to another node; other active nodes do not count it as a place for theirs. Use it for a node that is not behind the load balancer (check `/v1/ready`, see [Health checks](#health-checks)). While a passive node is cut off from the database, its clients can miss messages published on other nodes, so give it test clients only. |
 | `database-replica-urls`                    | `NTFY_DATABASE_REPLICA_URLS`                    | *list of strings (connection URLs)*                 | -                 | PostgreSQL read replica connection strings. Non-critical read-only queries are distributed across replicas (round-robin) with automatic fallback to primary. Requires `database-url`.                                                   |
 | `cache-file`                               | `NTFY_CACHE_FILE`                               | *filename*                                          | -                 | If set, messages are cached in a local SQLite database instead of only in-memory. This allows for service restarts without losing messages in support of the since= parameter. See [message cache](#message-cache).                     |
 | `cache-duration`                           | `NTFY_CACHE_DURATION`                           | *duration*                                          | 12h               | Duration for which messages will be buffered before they are deleted. This is required to support the `since=...` and `poll=1` parameter. Set this to `0` to disable the cache entirely.                                                |

docs/releases.md

@@ -2153,6 +2153,7 @@ and the [ntfy Android app](https://github.com/binwiederhier/ntfy-android/release
 **Features:**
 
 * Experimental clustering: add `experimental-cluster-role` to run a `passive` node that never leads and never takes over another node's subscribers, e.g. one that is not behind the load balancer ([#2049](https://github.com/binwiederhier/ntfy/pull/2049))
+* Add `/v1/ready` for load balancers, and the node's role to `/v1/health`; with clustering, a node with no other active node leads right away instead of after a one-minute hold-off ([#2050](https://github.com/binwiederhier/ntfy/pull/2050))
 
 **Bug fixes + maintenance:**
 

server/server.go

@@ -809,25 +809,23 @@ func (s *Server) handleTopicAuth(w http.ResponseWriter, _ *http.Request, _ *visi
 }
 
 func (s *Server) handleHealth(w http.ResponseWriter, _ *http.Request, _ *visitor) error {
-	// Unhealthy = the registry heartbeat went stale, peers stopped forwarding to this node, and
-	// another active node can take its clients; 503 lets status-code LB checks pull it (checkers
-	// must fail open, see cluster.Cluster)
-	healthy := s.cluster.Serving()
+	// Unhealthy = the registry heartbeat went stale and peers stopped forwarding to this node. For
+	// monitoring; load balancers check /v1/ready, which keeps the only active node in
+	healthy := s.cluster.Healthy()
 	if !healthy {
 		w.WriteHeader(http.StatusServiceUnavailable)
 	}
 	return s.writeJSON(w, &apiHealthResponse{Healthy: healthy, Role: s.cluster.Role()})
 }
 
-// handleReady answers whether a load balancer should send clients here: an active node that is
-// serving. A passive node is healthy (see handleHealth) but not ready.
+// handleReady answers whether a load balancer should send clients here (see cluster.Cluster's
+// Ready). A passive node is healthy (see handleHealth) but never ready.
 func (s *Server) handleReady(w http.ResponseWriter, _ *http.Request, _ *visitor) error {
-	role := s.cluster.Role()
-	ready := role == cluster.RoleActive && s.cluster.Serving()
+	ready := s.cluster.Ready()
 	if !ready {
 		w.WriteHeader(http.StatusServiceUnavailable)
 	}
-	return s.writeJSON(w, &apiReadyResponse{Ready: ready, Role: role})
+	return s.writeJSON(w, &apiReadyResponse{Ready: ready, Role: s.cluster.Role()})
 }
 
 // handleMetrics returns Prometheus metrics. This endpoint is only called if enable-metrics is set,

server/server_cluster_test.go

@@ -28,6 +28,7 @@ type fakeCluster struct {
 	announced  []string
 	notLeader  bool
 	notHealthy bool
+	notReady   bool
 	role       cluster.Role // Empty means active
 }
 
@@ -46,8 +47,16 @@ func (b *fakeCluster) BroadcastState(state *cluster.State) {
 	b.announced = append(b.announced, state.AddedTopics...)
 }
 
-func (b *fakeCluster) Serving() bool {
-	return b.Healthy() // The fake has no peers to hand clients to; tests set health directly
+func (b *fakeCluster) Ready() bool {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	return !b.notReady
+}
+
+func (b *fakeCluster) setReady(ready bool) {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	b.notReady = !ready
 }
 
 func (b *fakeCluster) Role() cluster.Role {
@@ -341,7 +350,7 @@ func TestServer_Cluster_FirebaseKeepaliverOnlyOnLeader(t *testing.T) {
 
 func TestServer_Cluster_HealthReflectsCluster(t *testing.T) {
 	// A node whose registry heartbeat went stale no longer receives forwarded messages, so
-	// health checks must pull it from rotation (the fail-open policy lives in the checker)
+	// health reports it (load balancers check /v1/ready, see TestServer_Cluster_Ready)
 	s := newTestServer(t, newTestConfig(t, ""))
 	cl := &fakeCluster{}
 	s.cluster = cl
@@ -485,8 +494,7 @@ func TestServer_Cluster_HealthReportsRole(t *testing.T) {
 }
 
 func TestServer_Cluster_Ready(t *testing.T) {
-	// Readiness is what a load balancer checks: an active node that is serving. A passive (or,
-	// later, draining) node is healthy but not ready.
+	// Readiness is what a load balancer checks; the cluster decides it (see Cluster.Ready)
 	s := newTestServer(t, newTestConfig(t, ""))
 	cl := &fakeCluster{}
 	s.cluster = cl
@@ -494,12 +502,7 @@ func TestServer_Cluster_Ready(t *testing.T) {
 	require.Equal(t, 200, rr.Code)
 	require.JSONEq(t, `{"ready":true,"role":"active"}`, rr.Body.String())
 
-	cl.setHealthy(false) // Another active node can take the clients (see Cluster.Serving)
-	rr = request(t, s, "GET", "/v1/ready", "", nil)
-	require.Equal(t, 503, rr.Code)
-	require.JSONEq(t, `{"ready":false,"role":"active"}`, rr.Body.String())
-
-	cl.setHealthy(true)
+	cl.setReady(false)
 	cl.mu.Lock()
 	cl.role = cluster.RolePassive
 	cl.mu.Unlock()
@@ -508,6 +511,20 @@ func TestServer_Cluster_Ready(t *testing.T) {
 	require.JSONEq(t, `{"ready":false,"role":"passive"}`, rr.Body.String())
 }
 
+func TestServer_Cluster_HealthIsTheHeartbeat(t *testing.T) {
+	// The only active node with a stale heartbeat (database unreachable) is unhealthy, which
+	// monitoring should see, but stays ready, since there is nowhere better for its clients
+	s := newTestServer(t, newTestConfig(t, ""))
+	cl := &fakeCluster{}
+	s.cluster = cl
+	cl.setHealthy(false)
+	rr := request(t, s, "GET", "/v1/health", "", nil)
+	require.Equal(t, 503, rr.Code)
+	require.JSONEq(t, `{"healthy":false,"role":"active"}`, rr.Body.String())
+	rr = request(t, s, "GET", "/v1/ready", "", nil)
+	require.Equal(t, 200, rr.Code)
+}
+
 func TestServer_ReadyWithoutClustering(t *testing.T) {
 	s := newTestServer(t, newTestConfig(t, ""))
 	rr := request(t, s, "GET", "/v1/ready", "", nil)

server/types.go

@@ -1,9 +1,9 @@
 package server
 
 import (
-	"heckel.io/ntfy/v2/cluster"
 	"net/http"
 
+	"heckel.io/ntfy/v2/cluster"
 	"heckel.io/ntfy/v2/model"
 	"heckel.io/ntfy/v2/user"
 	"heckel.io/ntfy/v2/util"