redis: reconnect and failover fixes, cli argument parsing by kbinetruy · Pull Request #2211 · sipwise/rtpengine

kbinetruy · 2026-10-16T12:27:29Z

When a Redis master fails over or stops answering, rtpengine reconnects from every update while holding r->lock, and sleeps 2 s under it when the node has the wrong role. Writes refused by the server are dropped without a trace, and a deleted call can be written back. In the lab, with --redis alone, all 188 ng requests after a demotion timed out, and a frozen master stopped ng replies for 34.3–35.5 s.

The first four commits of this PR were merged as 20bcdce7, e5ff3ea8, 1c447b50 and 6b3bf829. These six remain:

redis: count and log writes refused by the server
redis: do not sleep under the lock when reconnecting
cli: parse arguments without reading past the str
redis: do not write back a call that has been destroyed
redis: reconnect without holding the lock
redis: use FakeRedis.pm in the older Redis tests

Lab (Redis 7.2, ~21 writes/s)	Master	This branch
Writes refused by the server	not logged	`rediswriteerrors` = server count (6 of 6 runs)
`--redis` alone, failover: failed ng requests	188	0 (3 runs)
`--redis` alone, failover: longest time without an ng reply	18.8 s	0.21–0.22 s
Frozen master, `--redis-cmd-timeout=1000`: longest time without an ng reply	35.5 s	1.30–1.33 s (6 runs)
rtpengine cut from the master (22.6 s): ng p90 / p99 during the cut	619–641 / 1609–1635 ms with commits 1–4	103–108 / 733–933 ms (5 runs)

1. Every reply to a pipelined SET or DEL is checked. An error reply is counted in rediswriteerrors (redis_write_errors_total) and logged at most once a second; the connection is kept. An I/O error or a timeout drops it.

2. One reconnect attempt, with no sleep. After a wrong role, or a failure that took --redis-reconnect-slow or more, operations on that connection are skipped for --redis-reconnect-pause. A refused connection is retried by the next operation, as before.

3. The CLI handlers parse their argument as a str (str_to_X helpers and a new str_to_d) and print it with STR_FORMAT, so the ng cli body is no longer read past its end (set rediscmdtimeout 0 set 7 ms). As with bc08b059, on the TCP CLI too, a numeric value with trailing characters is refused, a non-number is refused by the seven handlers that read it as 0 (set loglevel included), and terminate matches all/own/foreign exactly (terminate a terminated all calls).

4. A call is written only while it is still in the call table, and the deletion decides "never stored" under r->lock, so the "kill calls" timer no longer writes back a call deleted while it waited for the lock.

5. The attempt runs without r->lock. Other operations wait up to --redis-reconnect-slow for it, then are skipped. After the lock was released, a write checks again that the call is live, and a deletion leaves the key to a new call with the same ID. Only the request whose thread makes the attempt, and the first command at the start of an outage, still wait longer.

As today, an update whose reconnect fails is not written and not retried. What changes is that the other requests no longer wait behind the attempt. With plain --redis (452 ng requests per run), master had 213–298 requests per run time out at 10 s (8 runs); this branch had none (16 runs), and the slowest took about 2 s. --redis-reconnect-pause=0 dropped as many writes as the default, and reconnected up to about 40 times a second against a node with the wrong role. The follow-up PR's opt-in repair writes them again.

6. Test-only: the five older Redis tests use FakeRedis.pm, with the same checks; their forked server moves to the module as redis_serve().

Option	Default	Meaning
`--redis-reconnect-pause=MS`	1000	After a wrong-role or slow failure, no new attempt and operations skipped for this long. 0 disables.
`--redis-reconnect-slow=MS`	100	A failed attempt this long or longer counts as slow. Also how long other operations wait for an attempt.

Testing. New daemon tests redis-errors, redis-destroyed and redis-reconnect (fake Redis in FakeRedis.pm), unit test test-str, test-stats extended. Each commit's tests fail on its parent; the converted tests fail on the same redis.c mutants as before. make asan-check with the follow-up PRs on master 3865f896: exit 0 on Debian trixie and Ubuntu noble, 9503 ok, 0 not ok. The patches apply with git am on 3865f896. 25 files, +1583 −804, of which tests +1231 −675.

Question: is a 1000 ms default pause right, or would you rather keep "try at every operation" (--redis-reconnect-pause=0)?

Lab method, per-run figures and test details: https://github.com/kbinetruy/rtpengine/blob/redis-notes/redis-sentinel-notes.md

Call updates (SET) and deletions (DEL) are pipelined, and redis_consume() freed their replies unread: a write refused with READONLY or NOREPLICAS, as during a Sentinel failover, left no trace in rtpengine. Check every reply. An error reply is counted in a new statistic, rediswriteerrors, and logged at most once a second per connection; the connection is kept, since a reconnect would get the same answer. A read error or timeout on the replies drops the connection at once instead of at the next PING. An encoding failure no longer drops the connection. Tested by auto-daemon-tests-redis-errors.pl, with a new fake Redis module, NGCP::Rtpengine::FakeRedis.

On a role mismatch, the reconnect in redis_check_conn() sleeps one second, asks INFO again and sleeps once more: two seconds under r->lock, which every Redis update holds before the ng reply (sipwise#1865). After a failover, with --redis on the old master's address, every update did this, and ng requests timed out. Make one attempt, with no sleep. After a role failure, or a failure that took --redis-reconnect-slow (100 ms) or more, skip Redis operations on that connection for --redis-reconnect-pause (1000 ms), through restore_tick_us as --redis-allowed-errors does. A quick failure (connection refused) is retried at once, as before. As with a failed attempt, a skipped operation is not written; skips are logged once per pause. The startup connection keeps its eleven attempts. Tested by auto-daemon-tests-redis-errors.pl.

cli_handle() takes a str, but most "set" handlers parsed their value with strtol(), strtod() or atoi() on instr->s and printed it with "%s", reading up to the next NUL. The ng "cli" command passes a slice of the request: "set maxsessions 1000" set 10007, "set rediscmdtimeout 0" set 7 ms, and error messages quoted the rest of the request. Parse with the str_to_X helpers, plus a new str_to_d for maxcpu and maxload, and print with STR_FORMAT. As with the str_to_X family, a value with trailing characters ("10x") is refused, and so is a non-number in the seven handlers that read it as 0. "terminate" now matches all, own and foreign exactly (str_cmp()), not as a prefix. Tested by auto-daemon-tests-redis-errors.pl and the new t/test-str.c.

The timer that rewrites never-stored SRTP calls can hold a call while it waits for r->lock. If the call is deleted meanwhile, the deletion, deciding without the lock that the call was never stored, deletes nothing, and the timer then writes it: the call is back at the next restore. Any update racing with a deletion can do the same. Under r->lock, redis_update_onekey() writes a call only while it is still in the call table, and redis_do_delete() decides "never stored", so that a write already past that check is followed by the DEL. Tested by auto-daemon-tests-redis-destroyed.pl.

A reconnect attempt runs under r->lock, which every ng request that writes a call waits for (sipwise#1865). Against a server that drops packets it takes the full connect timeout, so signalling stalls for up to that long at each attempt, for as long as the outage lasts. Make the attempt without the lock: the thread that reconnects sets r->reconnecting and owns r->ctx until it clears the flag under the lock. Other threads wait for it for up to --redis-reconnect-slow, then skip their operation, as during the pause. After the lock was released, redis_update_onekey() checks again that the call is live, and redis_do_delete() leaves the key to a new call with the same ID. The CLI, the DEL after a failed restore and the notification thread leave a connection being made alone. A command on an established connection still holds the lock. Tested by auto-daemon-tests-redis-reconnect.pl.

Ref #2211 "set rediscmdtimeout 0" reconnects the Redis connections, but each of the three calls is guarded by rtpe_redis instead of its own pointer: - with --redis and no notification connection (no --redis-subscribe and no subscribed keyspace), redis_reconnect(rtpe_redis_notify) locks a NULL mutex and rtpengine crashes; - with --redis-write and no --redis, the write connection is never reconnected, so its command timeout is never cleared. Guard each call with its own pointer, as the non-zero branch already does. Tested by auto-daemon-tests-redis-errors.pl, added later in this series ("set rediscmdtimeout 0" over the ng "cli" command with --redis only): without this change rtpengine crashes and never replies. The change is first in the series so that it can be taken on its own. Change-Id: Ia458a6eb0b70a6906ab410fd91087ad5af581f3b

Ref #2211 redis_connect() finds "role:" in the INFO reply and compares what follows with memcmp(): 11 bytes for "role:master", 19 for "role:active-replica" and 10 for "role:slave". memcmp() may read all n bytes, so a reply that ends less than 19 bytes after "role:" (for example a replica's "role:slave\r\n" with nothing after it) is read past its end. Real Redis and KeyDB servers send many more lines after the role, so this does not show in practice, but AddressSanitizer aborts on it (heap-buffer-overflow in the memcmp interceptor) with a minimal reply, such as one from a test server or a proxy. Use strncmp(): hiredis NUL-terminates the reply string, so the compare stops at its end. Tested by auto-daemon-tests-redis-errors.pl, added later in this series, which answers a reconnect's INFO with just "role:slave": an ASan build aborts there without this change. The change is early in the series so that it can be taken on its own. Change-Id: Icdf3d3a23464d861f57406436328d0b166794362

Ref #2211 redis_set_timeout() receives the timeout in microseconds (the --redis-cmd-timeout value times 1000) and logged it unconverted, so --redis-cmd-timeout=1000 was reported as: Setting timeout for Redis commands to 1000000 milliseconds Log the value in milliseconds, as the message says. The value passed to hiredis is unchanged. No functional change. Seen in the log of every connection made with a command timeout set; there is no test, as no test reads this line. Change-Id: Ie4d81e4f8baab1969210a589c98245407b088561

Ref #2211 When json_restore_call() fails, it sends "DEL <call-id>" twice: on the write connection, in its own DB, and on the notification connection, in whatever DB the last keyspace notification selected. For a foreign call (a keyspace notification, or the restore of a subscribed keyspace at startup) that key belongs to the instance that owns the call, which keeps writing it: a standby that fails to restore one record deletes the owner's copy, and the call cannot be taken over any more. For an own call, the DEL on the notification connection can hit a peer's DB. Only delete an unrestorable record when the call is our own, and only on the write connection, in our own DB. A foreign record that can never be restored now stays until its owner rewrites or deletes it, or until it expires (--redis-expires). Seen in a lab with Redis Sentinel, where a subscriber's DEL was refused only because its node had just become a replica. Tests, in auto-daemon-tests-redis-subscribe.pl: a keyspace notification for a foreign call whose record cannot be parsed is followed by no command on either connection, and an own record that cannot be parsed at startup is deleted once, on the write connection only. Without this change both get a DEL on both connections. Change-Id: I5d8a53f3515596cf3fc2c58ea97847476385b876

redis, redis-json and redis-subscribe compared raw bytes on their own sockets, and redis-srtp-index and rollback-redis each carried the same forked server. They now use NGCP::Rtpengine::FakeRedis, and the forked server moves into it as redis_serve(). The checks, their names and their counts are unchanged.

rfuchs · 2026-10-20T16:16:08Z

+static bool redis_call_taken(const str *callid, call_t *not_this) {
+	rwlock_lock_r(&rtpe_callhash_lock);
+	call_t *o = t_hash_table_lookup(rtpe_callhash, callid);
+	rwlock_unlock_r(&rtpe_callhash_lock);
+	return o && o != not_this;
+}


This is just !redis_call_live(...) isn't it?

Also, both of these are inherently racy, as there's no guarantee this doesn't change immediately after rtpe_callhash_lock is released

kbinetruy added 5 commits October 13, 2026 14:58

This was referenced Oct 16, 2026

redis: opt-in Redis Sentinel support and repair of refused writes #2212

Open

redis: optional TLS for the Redis and Redis Sentinel connections #2213

Open

rfuchs reviewed Oct 16, 2026

View reviewed changes

Comment thread perl/NGCP/Rtpengine/FakeRedis.pm

rfuchs reviewed Oct 16, 2026

View reviewed changes

Comment thread daemon/redis.c

rfuchs reviewed Oct 16, 2026

View reviewed changes

Comment thread daemon/cli.c Outdated

kbinetruy force-pushed the redis-failover-fixes branch from 5feabb8 to 3d02382 Compare October 20, 2026 07:28

kbinetruy changed the title ~~redis: nine fixes for a master failover~~ redis: reconnect and failover fixes, cli argument parsing Oct 20, 2026

kbinetruy force-pushed the redis-failover-fixes branch from 3d02382 to 410ad30 Compare October 20, 2026 09:52

rfuchs reviewed Oct 20, 2026

View reviewed changes

Comment thread lib/str.h

rfuchs reviewed Oct 20, 2026

View reviewed changes

Navigation Menu

Repository navigation

redis: reconnect and failover fixes, cli argument parsing - #2211

redis: reconnect and failover fixes, cli argument parsing#2211
kbinetruy wants to merge 6 commits into
sipwise:masterfrom
kbinetruy:redis-failover-fixes

kbinetruy commented Oct 16, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rfuchs Oct 20, 2026

Uh oh!

rfuchs Oct 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

kbinetruy commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rfuchs Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

rfuchs Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kbinetruy commented Oct 16, 2026 •

edited

Loading