[v92-audit] qos-critical: priority client counter drifts on every replica sync, so admission control rejects normal clients below maxclients

[madolson]

server.stat_num_active_priority_clients is incremented in linkClient() and decremented in unlinkClient() from the connection's priority bit read at each of those two moments, but syncCommand() flips that bit in between, so every replica sync permanently loses one from the count. The count feeds admission control, where normal_clients = total_clients - prioritized_clients decides whether a non-priority connection is rejected, so each sync makes the server behave as if one more normal client were connected than really is. With maxclients 20, maxclients-reserved 10 and ten SYNC connections having come and gone, a server holding only 10 clients rejects new normal connections with -ERR max number of clients reached while half of maxclients sits unused. Introduced by #4005 on top of #4076.

Details

Problem

linkClient() counts the connection as prioritized based on the bit at link time (src/networking.c:243):

    if (connIsPriority(c->conn)) {
        server.stat_num_active_priority_clients++;
    }

unlinkClient() decrements based on the bit at unlink time (src/networking.c:2219):

            if (connIsPriority(c->conn)) {
                if (server.stat_num_active_priority_clients > 0) {
                    server.stat_num_active_priority_clients--;
                }
            }

Nothing keeps the two reads in agreement. acceptCommonHandler() sets the bit from priority-subnets before createClient() (src/networking.c:2087), and then syncCommand() sets it to true on any replica link regardless of subnet (src/replication.c:1379):

    if (connSetPriority(c->conn, true) == C_ERR) {

A replica arriving from outside priority-subnets therefore increments nothing and decrements one. cluster_migrateslots.c:862 promotes import job clients the same way. The > 0 guard stops the counter going negative but does not stop it going wrong: it saturates at 0 while real priority connections are still attached.

The count is not just telemetry. hasMaxClientsLimitReached() reads it (src/networking.c:2059-2061):

        long long prioritized_clients = server.stat_num_active_priority_clients;
        long long normal_clients = (total_clients > prioritized_clients) ? (total_clients - prioritized_clients) : 0;
        return normal_clients >= normal_limit;

Undercounting prioritized_clients inflates normal_clients by the same amount, so the reserved-slot check fires early and rejects normal clients that fit inside maxclients.

Reproduction

Minimal, showing the counter disagreeing with CLIENT LIST:

$ valkey-server --port 7405 --priority-subnets "::1/128" --daemonize yes ...

One client from ::1 (matches priority-subnets, so counted), then one connection from 127.0.0.1 that sends SYNC and closes:

step1 (only ::1 client + admin): ['connected_clients:2', 'connected_priority_clients:1']
step2 (after one SYNC connection closed): ['connected_clients:2', 'connected_priority_clients:0']

The ::1 client never left and is still flagged H:

id=3 addr=[::1]:38410 laddr=[::1]:7405 fd=11 name= age=1 idle=1 flags=H capa= db=0 ...
id=4 addr=127.0.0.1:43424 laddr=127.0.0.1:7405 fd=12 name= age=1 idle=0 flags=N capa= db=0 ...

A real replicaof produces the same drift; SYNC is only the shortest way to trigger the promotion.

Impact

maxclients 20, maxclients-reserved 10, priority-subnets ::1/128, eight persistent ::1 clients plus one admin connection. Ten short-lived SYNC connections from 127.0.0.1, then open normal IPv4 clients until refused:

before:                   ['connected_clients:9', 'connected_priority_clients:8']
after 10 SYNC churn:      ['connected_clients:9', 'connected_priority_clients:0']
normal ipv4 clients accepted: 1  first rejection: b'-ERR max number of clients reached\r\n'
final:                    ['connected_clients:10', 'connected_priority_clients:0']

Rejected at 10 of 20 clients. Same setup with the ten SYNC connections removed, on a freshly started server:

CONTROL before:                 ['connected_clients:9', 'connected_priority_clients:8']
CONTROL normal ipv4 accepted: 9 first rejection: b'-ERR max number of clients reached\r\n'
CONTROL final:                  ['connected_clients:18', 'connected_priority_clients:8']

Eight connection slots lost to the drift alone. The drift never recovers on its own; only CONFIG SET priority-subnets, which recomputes the count from scratch at src/networking.c:2006, resets it, and that path has its own problem (#16).

Decisions for a reviewer

Track the counter on the priority transition instead of on link/unlink: have connSetPriority() adjust stat_num_active_priority_clients when the connection is currently linked, and drop the connIsPriority() checks in linkClient()/unlinkClient() in favor of the value the connection had when it was linked. Alternative considered and rejected: making hasMaxClientsLimitReached() walk server.clients to count priority connections. It is correct but turns admission control into an O(maxclients) walk per accept, which is exactly the path that must stay cheap.

Also worth settling: unlinkClient()'s > 0 clamp hides the bug. Once the counter is maintained on transitions it should be a serverAssert, not a clamp.

Testing

Not covered by tests/unit/qos.tcl; it asserts connected_priority_clients only for clients whose priority never changes after accept.

This was generated by AI but verified, with love, by a human.

[madolson]

Duplicate of valkey-io/valkey#4674, with the fix already open as valkey-io/valkey#4681.

That PR's own problem statement describes this exact counter, in the same two functions: "linkClient() previously incremented stat_num_active_priority_clients based on connIsPriority(), but dynamically promoted connections never incremented it, while unlinkClient() decremented unconditionally on disconnect." The upstream issue frames the replication-link demotion as the primary symptom and the counter drift as the second consequence, which is the same defect from the other end.

Closing. The admission-control consequence via hasMaxClientsLimitReached() is worth confirming is covered by #4681's tests before it merges.