fix: announce "up" only once redis can serve connections

[Gabriellji]

What does this PR do?

Closes #2

RedisModule_OnLoad() called send_udp_message() directly. Redis loads modules
during start-up, before initListeners(), so the broker advertised its address
over UDP while it was still refusing TCP connections. Discovery has no
serve-ability check — a host is admitted as a cluster member as soon as its
announcement arrives — so clients dialled it and were refused. A Kubernetes
readiness probe does not close this: the address clients dial is the pod IP
carried in the datagram, not a Service endpoint.

The first announcement now runs from a CronLoop hook that unsubscribes itself
before announcing. Redis enters its event loop only after initListeners() and
after loadDataFromDisk(), so the first datagram cannot precede the listener,
and a large RDB/AOF delays the announcement rather than letting it out early.
Unsubscribing first means the hook cannot run twice and leaves no callback
behind once it has done its one job.

Nothing else changes: same per-interface threads, same cadence, same arguments.
global_redis_port and global_redis_tls are resolved in OnLoad before the
hook is registered, and the callback neither re-reads configuration nor derives
TLS state. The announcing port log line moves with the send, so it now reports
the announcement rather than preceding it.

Why not a module timer

A one-shot RedisModule_CreateTimer() reaches the same event-loop boundary and
was the first implementation, but redis refuses MODULE UNLOAD while a module
holds an unfired timer, so an immediate load/unload failed where it previously
succeeded.

A cron hook guarded by a flag rather than unsubscribing was also tried. A
function-local static survives this loader's unload/reload, leaving a reloaded
module permanently silent; a file-scope flag works but keeps a callback firing
for the life of the process. Unsubscribing needs neither.

The one cost: on a runtime MODULE LOAD the announcement waits for the next
cron, about 1000 / hz ms — roughly 100 ms at the default hz. At start-up the
first cron is already due, so a --loadmodule broker is unaffected.

Type of change

• Bug fix
• New feature
• Documentation
• Refactor / internal
• Other:

Checklist

• I have read the Contributing guide.
• Tests added or updated, and the full suite passes locally (npm test).
  This repository has no test suite or package.json, so there is nothing to
  run — what was done instead is under How it was verified below.
• Docs / doc-blocks updated where relevant.

How it was verified

Two harnesses. In both, the unmodified module was run first, on the same
harness, and had to reproduce the defect — otherwise the test cannot observe it
and its result means nothing.

1. Redis built from source with sleep(5) before initListeners(), giving a
   guaranteed interval in which the module has loaded but the server refuses
   connections.
2. The unmodified redis binary, with an LD_PRELOAD shim holding the first
   listen() on the redis port for 5 s, which widens the real window instead of
   patching redis.

Both run a stub Kubernetes API over TLS serving a pod list, so real datagrams
are emitted, and compare the first up against redis's own
Ready to accept connections log line — one clock, millisecond resolution.

harness	before	after
patched redis	4.999 s before ready	0.005 s after ready
unmodified binary	5.00 s before ready	0.005 s after ready

Checked against the unmodified module, all unchanged:

• MODULE UNLOAD immediately after MODULE LOAD still succeeds
• repeated unload/reload announces every cycle at the same rate; the hook fires
  once per load and thread count stays flat, so it neither goes silent nor
  accumulates broadcasters
• SIGTERM during the pre-listener window sends zero up datagrams and exits
  accumulates broadcasters
• SIGTERM during the pre-listener window sends zero up datagrams and exits
  cleanly
• SELECTED_INTERFACES unset announces every interface, a matching prefix
  announces only that one, a non-matching prefix announces nothing
• announcement interval and payload fields unchanged
• down emitted for every announced interface on shutdown; thread and fd counts
  identical
• 25 × MODULE LOAD/MODULE UNLOAD leaves no thread or fd growth; the census
  moves 4 → 6 → 4 threads across a cycle, so it does observe the module's threads

Not covered

TLS-listener mode was not exercised end to end. global_redis_tls is assigned
once in OnLoad before the hook is registered and passed through unchanged, so
no code path makes it differ between the two call sites, and the emitted
payload's mode field was identical in every captured datagram.

Contribution terms (required)

• I have read and agree to the @imqueue Contribution Terms.
  I grant the project owner the right to license my contribution
  commercially, royalty-free, my contribution stays available under
  GPL-3.0, I keep my copyright, and I understand I will receive no fee for
  it. If I did not agree, I would not be submitting this contribution.

[github-actions]

All contributors have signed the @imqueue Contribution Terms. ✅
_{Posted by the CLA Assistant Lite bot.}

[Gabriellji]

I have read the CLA Document and I hereby sign the CLA

[Mikhus]

Thanks for the thorough write-up. The analysis in #2 holds on every point I checked against the Redis source, and the boundary this PR picks is the right one. The mechanism used to reach it is not safe, though, and it cannot be merged as written. Details, evidence and a proposed reshaping below.

What holds up

• Modules load before listeners. In main() the order is moduleLoadFromQueue, then initListeners, then loadDataFromDisk, then aeMain, in 7.2, 7.4, 8.0 and 8.2.
• The cron-loop module event fires only from serverCron inside the event loop. whileBlockedCron never fires it, so loading an RDB or AOF delays the announcement instead of letting it out early. The first serverCron is due one millisecond after initServer, so at start-up it runs on the first loop iteration. Locally the announce line lands one millisecond after Ready to accept connections with this branch, and before it with master.
• The timer objection is correct: moduleUnload checks moduleHoldsTimer before it calls OnUnload, so a module cannot cancel its own pending timer to let an unload through.
• On the consumer side, UDPWorker posts cluster:add and the cluster starts startHost and syncHost immediately, so a refused first dial is exactly what happens. fix(cluster): retry a joining host whose subscription catch-up failed core#32 covers what the library does after that.

The problem: unsubscribing from inside the hook is a use-after-free

RedisModule_SubscribeToServerEvent(ctx, event, NULL) deletes the listener node and frees the listener struct. When the callback returns, moduleFireServerEvent continues with:

el->module->in_hook++;
el->callback(&ctx,el->event,subid,moduledata);
el->module->in_hook--;      /* el was freed by the callback */

That read of el->module is on freed memory. The pattern is identical in 7.2, 7.4, 8.0, 8.2 and current unstable. Redis's own tests/modules/hooks.c never unsubscribes from inside a hook, which is why upstream has not tripped over it.

What I ran, with this branch built from 94b4017:

Redis build	Module	Result
8.0.5 Ubuntu package (links system libjemalloc.so.2)	this branch	segfault at the first cron tick, 4 of 4 runs, also with SELECTED_INTERFACES=127 so only one thread starts
same	master	no crash
same	20-line probe doing only the subscribe/unsubscribe pattern	survives; the corruption is silent
7.4.2 built with SANITIZER=address	this branch	heap-use-after-free
same	master	clean
same	probe	heap-use-after-free

Sanitizer report, trimmed:

ERROR: AddressSanitizer: heap-use-after-free ... READ of size 8 thread T0
    #0 moduleFireServerEvent src/module.c:11832
    #2 serverCron src/server.c:1530
freed by thread T0 here:
    #1 RM_SubscribeToServerEvent src/module.c:11685
    #2 cron_broadcast_once (unicaster.so)
previously allocated by thread T0 here:
    #3 RM_SubscribeToServerEvent src/module.c:11693
    #4 RedisModule_OnLoad (unicaster.so)

The shipped 8.0.5 binary crashes at moduleFireServerEvent+0x2a4, which disassembles to mov (%rbx),%rax followed by subl $0x1,0x48(%rax): the in_hook-- on the freed listener. The likely reuse path there: the whole process shares one jemalloc, BroadcastTask is 24 bytes and lands in the same 32-byte size class as the listener, the thread cache hands the just-freed slot back to malloc(sizeof(BroadcastTask)), and strncpy of the interface IP overwrites the module pointer.

This also explains why the harness in the description passed. The official redis:7.2 and redis:7.4 images bundle a je_-prefixed jemalloc, so the module's own allocations go to glibc and the freed slot stays intact. That makes the bug silent in the published images today, and a crash on every start on a distro package, or after any change in how the server is built. A test that passes on undefined behaviour is not evidence that it is safe.

Proposed reshaping

Keep the cron boundary, never unsubscribe from inside the callback. Guard with a file-scope flag reset in OnLoad and let Redis drop the subscription on unload, which moduleUnregisterCleanup already does:

static int announced = 0;   /* file scope; reset in OnLoad so a reload announces again */

void cron_broadcast_once(RedisModuleCtx *ctx, RedisModuleEvent e,
                         uint64_t subevent, void *data) {
    (void)e; (void)subevent; (void)data;

    if (announced || is_closing) {
        return;
    }

    announced = 1;
    start_broadcasting(ctx);
}

and next to the existing is_closing = 0; in RedisModule_OnLoad, add announced = 0;. The cost is one flag check per cron tick.

On the reason given for rejecting this shape: I could not reproduce a function-local static surviving unload/reload. On the 8.0.5 build above, after MODULE UNLOAD both unicaster.so and libcurl are gone from /proc/<pid>/maps, and a function-local counter in OnLoad reads 1 on every reload. Either way, a file-scope flag reset in OnLoad does not depend on what the loader does.

Smaller items while you are in there:

• The RedisModule_SubscribeToServerEvent call in OnLoad is unchecked. A failure would leave the broker silently invisible; log it at warning like the other invisible cases.
• start_broadcasting(RedisModuleCtx *ctx, void *data) still carries the timer-callback shape from the first attempt. It only needs ctx.
• The promoter module announces from OnLoad in the same way, and promoter is the image's default mode. The same gating belongs there, in imqueue/redis-broker-promoter.
• Once both land, imqueue/redis-broker needs the submodule bumps and a rebuild for the 7.2 and 7.4 matrix.

Reporting the el->module read to upstream Redis is worth doing separately. Caching the module pointer before the callback is a one-line fix there, but this module has to be correct on 7.2 and 7.4 as they ship.

Reproduction

The probe I used, which is the subscribe/unsubscribe pattern and nothing else:

#include "redismodule.h"

static void cron_once(RedisModuleCtx *ctx, RedisModuleEvent e, uint64_t sub, void *data) {
    (void)e; (void)sub; (void)data;
    if (RedisModule_SubscribeToServerEvent(ctx, RedisModuleEvent_CronLoop, NULL) != REDISMODULE_OK) {
        RedisModule_Log(ctx, "warning", "probe: could not remove the startup cron hook");
        return;
    }
    RedisModule_Log(ctx, "notice", "probe: one-shot cron hook fired");
}

int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
    (void)argv; (void)argc;
    static int loads = 0;
    if (RedisModule_Init(ctx, "probe", 1, REDISMODULE_APIVER_1) == REDISMODULE_ERR) return REDISMODULE_ERR;
    loads++;
    RedisModule_Log(ctx, "notice", "probe: OnLoad, function-local static loads=%d", loads);
    RedisModule_SubscribeToServerEvent(ctx, RedisModuleEvent_CronLoop, cron_once);
    return REDISMODULE_OK;
}

# Redis 7.4.2 source tree; SANITIZER=address forces MALLOC=libc
make -C redis-7.4.2/src -j SANITIZER=address redis-server
gcc -fPIC -shared -O2 -o probe.so probe.c -I.
ASAN_OPTIONS=detect_leaks=0 redis-7.4.2/src/redis-server --port 16382 --save "" \
    --enable-module-command yes --loadmodule ./probe.so

The report appears within the first cron tick. Loading this branch's unicaster.so the same way gives the same report with cron_broadcast_once in the free stack. On a distro Redis that links the system jemalloc, redis-server --loadmodule ./unicaster.so is enough: the crash report names moduleFireServerEvent called from serverCron.

[Gabriellji]

@Mikhus Thanks for the thorough review — you're right, and the trace was exactly on point. I checked it independently before changing anything, and it reproduces cleanly.

Why it crashed. RedisModule_SubscribeToServerEvent(ctx, event, NULL) frees the listener struct, but moduleFireServerEvent() still touches it after the callback returns (el->module->in_hook--). So the hook removing itself from inside its own callback was a use-after-free on every redis version. It was invisible in our tests because the shipped alpine image bundles a je_-prefixed jemalloc — redis's heap and the module's malloc never meet, so the freed slot was never reused. On any build where they share an allocator, the slot is reused immediately and redis dies at the first cron tick. Our harness was green on undefined behaviour; a lesson we've taken.

Reproduced (94b4017 → new shape):

• Debian redis 7.0.15, system jemalloc: SIGSEGV 4/4 starts → 4/4 ok
• redis 7.4.2, libc malloc: crash at first cron tick → ok
• redis 7.4.2 with SANITIZER=address: report in moduleFireServerEvent → clean

Follow-up commit, as you proposed:

• hook stays subscribed for the module's life; file-scope announced flag, reset in OnLoad; redis drops the subscription itself on unload
• SubscribeToServerEvent in OnLoad checked, warning on failure (return stays OK, like the no-listener case)
• start_broadcasting(ctx) — the timer-shaped void * dropped
• RedisModule_OnUnload given its real signature, int (RedisModuleCtx *) returning OK — it was void, so redis was reading a garbage return value to decide whether to allow the unload

On the static: both observations turned out correct, on different loaders. glibc really unloads on MODULE UNLOAD, so the counter resets — what you saw. musl, the base of our alpine image, never unloads on dlclose(), so the same probe reads 1, 2, 3, 4 across four loads. That is why the reset in OnLoad is load-bearing: without it the reshaped module announced only on the first load (8 0 0 0 0 datagrams over five cycles); with it, 8 8 8 8 8.

Re-verified against the unmodified module: announce lands after Ready to accept connections (patched redis and the shipped image with a held listen()), immediate unload, 5× reload, SIGTERM in the pre-listener window (0 datagrams), interface filtering, cadence/down/thread+fd counts, 25× load/unload leak. End to end with a real @imqueue/core client discovering the broker over UDP: 14/14 runs incl. two broker swaps, every payload delivered exactly once.

Not in this PR, to keep it to one change: is_closing is read by the broadcaster threads without _Atomic (pre-existing), the same gating in redis-broker-promoter, and the redis-broker submodule bumps once both land. Agreed the el->module read is worth reporting to redis upstream.