MOD-17518 Refine cluster-type detection (allow RE+oss bdbs)

[galcohen-redislabs]

Replace the load-time cluster detection (CONFIG GET cluster-enabled + rlec_version parsing) with a runtime GetClusterType() and allow both cluster-aware and -unaware shards in RE cluster bdbs.

[gabsow]

Reviewed this against the two MOD-17518 support packages and the crash artifact from this PR's own CI run. The core of it is right, and the RE signal is better than what I wrote in #122 — probing for the enterprise-only REDISMODULE_NOTIFY_TRIMMED capability is a capability check, where mine parses rlec_version out of INFO. I'd rather have yours. _proxy-filtered whenever the RE bit is set is also exactly the fix: it covers RE_OSS, which is the combination that broke.

Four things I think block it as written, two of them not visible to CI:

1. ClusterType_NONE is a legitimate state and the assert kills standalone OSS. This is what all 6 legs are failing on, from the test-logs-7.2 artifact of run 31736465742:

   16334:M 13 Aug 2026 19:36:38.227 # === ASSERTION FAILED ===
   16334:M 13 Aug 2026 19:36:38.227 # ==> cluster.c:216 'result != ClusterType_NONE' is not true
   

   test_basic_testBasicMR-oss, i.e. plain standalone OSS 7.2, where neither bit can be set.

2. The auth inversion re-creates the hang on RE, with a different error string. Details inline at SendAuthCommandIfNeeded. CI cannot catch this, because RLTest's oss-cluster has a real cluster bus and therefore a genuinely shared internal secret — RE does not.

3. GetClusterType() is recomputed per call site, on two different threads. The command flags are decided at module-load time and the auth mode on the LibMR event loop. If those two evaluations disagree — and there is a documented reason they do — we are back to flags and auth contradicting each other, which is the bug this PR is fixing. Inline.

4. MR_IsClusterInitialize() changing meaning breaks LibMR's own non-cluster usage. Inline.

On the diagnosability side: this whole incident was identified from one log line in a support package, and the new version of that line can no longer express RE + OSS. Worth keeping. Inline.

Happy to fold my #122 into this and close mine — I think the merge is: your TRIMMED capability probe for the RE bit, the load-time CONFIG GET cluster-enabled for the OSS bit, NONE allowed, computed once and cached in clusterCtx, plus the credential ordering and NULL-secret fallback from #122 (f7ff12c). Say the word and I'll push that onto your branch or re-target #122 as a diff against it.

[gabsow]

Blocker — this is what the 6 red legs are. From the test-logs-7.2 artifact of run 31736465742, on test_basic_testBasicMR-oss:

# === ASSERTION FAILED ===
# ==> cluster.c:216 'result != ClusterType_NONE' is not true

A plain standalone OSS server sets neither bit: GetContextFlags() & CLUSTER is 0 when not in cluster mode, and NOTIFY_TRIMMED is enterprise-only (redismodule-rlec.h:55). So NONE is not a can't-happen — it is the single most common deployment LibMR runs in, and asserting on it aborts the server at module load.

NONE wants to be a normal value meaning "neither OSS cluster nor enterprise", which is what the old isOss = true + CurrCluster == NULL state expressed.

[galcohen-redislabs]

refined in later commits

[gabsow]

Confirmed addressed by 9e31ab0 — and it held up in practice: across five databases and a dozen shard starts on a live Enterprise cluster (standalone and cluster-mode, plus a restart cycle) the assert never fired, asserts=0 on every shard log I checked.

The || MR_IsMainThread() form still encodes the invariant implicitly rather than saying it, so a one-line comment ("NONE is only reachable before a topology exists, i.e. only on the main thread") would save the next reader the three-step walk. Non-blocking, and I'm not going to hold the approve on it.

[gabsow]

Compute this once at init and cache it in clusterCtx — recomputing per call site creates three separate problems.

Init/runtime skew. The comment this PR deletes says it outright: RedisModule_GetContextFlags() does not report REDISMODULE_CTX_FLAGS_CLUSTER at module-load time, which is precisely why the CONFIG GET cluster-enabled probe existed. GetClusterType() is called during MR_ClusterInit (log line, command_flags, both command registrations) and again later on the event loop (SendAuthCommandIfNeeded, MR_HelloResponseArrived). If the flag is absent at load and present afterwards, then on an RE_OSS shard the flags are chosen as RE-only while the auth is chosen as OSS — flags and auth disagreeing again, which is the exact shape of the bug being fixed. On a pure OSS-cluster shard the same skew makes it NONE at load, i.e. the assert above.

Off-thread module API access, no GIL. SendAuthCommandIfNeeded and MR_HelloResponseArrived run on the LibMR event loop thread. Both GetContextFlags and GetKeyspaceNotificationFlagsAll are now called from there without RedisModule_ThreadSafeContextLock — note the existing code in this same function takes the lock before GetInternalSecret for that reason. On Enterprise this reaches the cluster plugin, which is main-thread-only; that is the RED-209602 crash pattern (LibMR reading the cluster API off-thread -> plugin panic -> SIGABRT). Previously these sites read a plain int.

Guard the API pointer. RedisModule_GetKeyspaceNotificationFlagsAll is version-gated like every other API in redismodule.h; if it is ever NULL this dereferences it. Cheap to check, and RedisModule_GetInternalSecret is already checked this way two lines down at the call site.

Nit while here: GetClusterType() should be GetClusterType(void), and always_inline on something making two API calls isn't buying anything.

[galcohen-redislabs]

The deleted comment was wrong. Both the flag and the CONFIG GET ... look at the same server.cluster_enabled which is available at OnLoad. I verified it on unstable, 7.x and 8.x.
The RedisModule_...() APIs results are now cached upon OnLoad so no need to lock.
The RedisModule_GetKeyspaceNotificationFlagsAll() API is supported since redis 6.2 and is not going away anytime soon. No need to guard.
Nit fixed.

[gabsow]

You're right and I was leaning on a stale comment rather than checking — if server.cluster_enabled is readable at OnLoad on 7.x/8.x/unstable then the init/runtime skew I described can't happen, and caching at load removes the locking concern too. Withdrawn.

One small thing on the new assert: result != ClusterType_NONE || MR_IsMainThread() makes the invariant thread-dependent, so it reads as "NONE off the main thread is impossible" rather than stating why. It happens to hold — reaching the event loop requires a topology, and a topology means CLUSTERSET, which sets the RE bit — but that's a chain of three non-obvious facts. A one-line comment saying that would save the next reader the walk. Non-blocking.

[galcohen-redislabs]

I think the assert is clear enough without any comment since if A then B is equivalent to not A or B.

[gabsow]

This inverts the credential preference, and I think it re-creates the hang on RE.

For an RE_OSS shard the ClusterType_OSS bit is set, so this branch wins and the CLUSTERSET password is never sent — an RE shard authenticates with the password DMC puts in ADDR <pwd>@<ip>:<port>, and that is the credential RE actually accepts.

The evidence that the internal secret is not usable there is the July support package on MOD-17518: that run took the short-form path, so MR_ClusterGetPassword() returned NULL, this internal-secret AUTH was sent, and the handshake still failed — 3554 x unknown command over ~30 minutes. RE's cluster is synthesized by the cluster plugin rather than gossiped over a cluster bus, so there is no shared secret to authenticate with.

With _proxy-filtered flags the commands are visible on a regular connection, so the failure mode changes rather than disappearing: the connection ends up unauthenticated and HELLO comes back NOAUTH Authentication required instead of unknown command, retried at 1 Hz for the life of the database. Same dead fan-out, new string to grep for.

I hit the mirror image of this in #122 and Bugbot caught it — I had keyed the credential choice off the same flag as the command flags, which made an enterprise binary stop sending internal-secret AUTH at all. What worked was leaving the preference alone and only fixing the contradictory case:

if (commandsAreInternal) internal secret;   /* a password cannot make a connection internal */
else if (n->password)    password;          /* what RE gives us */
else                     internal secret;   /* nothing else available; server may still require auth */

Note this also has to stay consistent with MR_ClusterGetPassword() (line 2050 on master), which drops the password whenever RedisModule_GetInternalSecret exists — on RE_OSS that throws away the one credential RE accepts.

[galcohen-redislabs]

Why do you think the CLUSTERSET short form in the July support package on MOD-17518 didn't carry the password with it?
In the dmc code I see that the short cluster info is using the internal_pass:

        args.push_back("AUTH");
        args.push_back(dmc_bdb_config_get_internal_pass(bdb_config));

so probably what you saw there was the results of missing the _proxy-filtered flag due to bad calculation of the flags.

[gabsow]

Good challenge, and I think you're right — I can't support that claim.

My reasoning was: short form -> MR_ClusterGetPassword() returns NULL when RedisModule_GetInternalSecret exists -> n->password NULL -> internal-secret AUTH. But that only holds for the argc == 1 case. If DMC sends AUTH <internal_pass> then it's argc == 3 and MR_ClusterSet takes password = argv[2] directly, so n->password is set and the password branch runs — same as the long form. I never checked the DMC side; my local Redis-Enterprise checkout is from May, has no short-form CLUSTERSET and no form= logging, so it can't settle it and you're reading current source.

Withdrawing the inference. It actually makes the root cause cleaner: the password branch ran in both packages, so the connection was password-authenticated in both, non-internal in both, and the internal-flagged HELLO was invisible in both. No anomaly left to explain.

What it also means is that neither package tells us anything about whether RE's internal secret is usable. So this branch goes from "known not to work on RE" to "unverified on RE", which is the state I'd want closed before merge rather than after: on a real ASM database cachedClusterType is RE_OSS, the OSS bit is set, so this preferred branch sends AUTH "internal connection" <secret> and never the CLUSTERSET password. If RE doesn't propagate a shared secret across a database's shards, HELLO comes back NOAUTH and we've swapped one 1 Hz retry loop for another. One redis-cli COMMAND INFO timeseries.hello plus a shard log on a live cluster-mode database settles both that and the flag change; happy to run it if you can point me at an env0 cluster.

[gabsow]

Resolved, and against me — I was wrong. Verified on a live cluster-mode Enterprise database with this branch's module.

RE_OSS sets the OSS bit, so this branch is the one that runs and the CLUSTERSET password is never sent, exactly as I described. But it works: bad_hello=0, NOAUTH=0, and the fan-out returns in 0.02 s with correct data, where stock 8.8.3 on the identical database config returns "at least one shard did not reply" after 5 s.

So Redis Enterprise does propagate a usable internal secret across a database's shards. That was the one thing neither support package could show, and I guessed it the wrong way round. Your credential ordering is correct and the ordering I proposed in #122 was solving a problem that doesn't exist. Objection withdrawn.

[gabsow]

Worth fixing while this function is being touched: RedisModule_GetInternalSecret can be present and still return NULL, so this assert is a crash vector, not a can't-happen. A cluster wired together purely by CLUSTERSET never gets a secret — which is what LibMR's own test harness is.

Empirically: an intermediate commit of #122 made this reachable and build (unstable) died on it —

# ==> cluster.c:441 'secret' is not true

f7ff12c in #122 turns it into a TrySendInternalSecretAuth() that returns false when there is no secret so the caller can fall through to the password. Since this PR makes the internal branch the preferred one, the assert becomes reachable in strictly more situations than on master.

[galcohen-redislabs]

But this code block is guarded by checking that the cluster type does include oss, so a password should exist.

[gabsow]

The guard establishes that a password exists, but the assert is on the secret, and those come from different places — RedisModule_GetInternalSecret being non-NULL as an API pointer doesn't mean it returns a secret.

This isn't hypothetical; it's how I found it. An intermediate commit of #122 made this branch reachable and build (unstable) died right here:

# === ASSERTION FAILED ===
# ==> cluster.c:441 'secret' is not true

A cluster wired purely by CLUSTERSET never gets a secret, which is what the LibMR harness is. This PR doesn't hit it because the CLUSTERSET-implies-RE hint keeps the harness off this branch — but the same hint means a real RE_OSS database now prefers this branch, so the exposure is strictly larger than on master, and the failure is a module abort rather than a retry loop.

if (!secret) { fall through to the password } covers it, which is what f7ff12c in #122 does.

[gabsow]

Downgrading this from blocker to optional: on the live Enterprise run the assert did not fire (asserts=0, crashes=0 on every shard), so GetInternalSecret returns a real secret on RE and this branch is safe there in practice.

The NULL state does still exist — it is what killed build (unstable) on #122 (cluster.c:441 'secret' is not true) in a CLUSTERSET-wired cluster with no secret. So if (!secret) → fall through to the password is still worth having as hardening, but it is no longer a condition of my approval and I won't push it further.

[galcohen-redislabs]

Not sure I understand the concern: the guard is that 1. the internal secret api exists, and 2. the redis servers are cluster aware. In this case they will always have a shared secret (negotiated via gossips), so the assert holds.

[gabsow]

This log line is how MOD-17518 was actually found — Detected redis oss (cluster-enabled=yes) in redis-197.log was the single line that identified the misclassification in a 344-shard support package. The new version can't express the state that matters: RE_OSS prints as enterprise / yes, which reads identically to how a plain enterprise shard would print if the OSS bit were spurious.

Since the type is now a proper bitmask, please print the combination and the resulting decision, e.g. Detected redis re+oss (cluster-enabled=yes), internal commands: _proxy-filtered. The next incident of this class will be triaged from exactly this line, and having the registered flags in it too would have turned a package dissection into one grep.

(Also two GetClusterType() calls in one statement — with caching this becomes one read.)

[galcohen-redislabs]

The info we have in the log is good enough. We have both bits so can deduce the command flags.

[gabsow]

Withdrawn — and you were right in the most direct way possible: that log line is what I used to verify the fix. Same shard, same database config, before and after:

stock 8.8.3: <timeseries> Detected redis oss (cluster-enabled=yes)
this branch: <timeseries> Detected redis enterprise (cluster-enabled=yes)

Both bits were legible and that was enough to tell the whole story at a glance. My concern about RE_OSS being indistinguishable was theoretical; in the actual artifact it read fine.

[gabsow]

== ClusterType_OSS excludes RE_OSS, so <module>.REFRESHCLUSTER is no longer registered on an ASM / OSS-cluster-API enterprise database, where post-#98 it was. That is probably what you want — RE drives the topology through DMC and refresh is documented as a user-invoked OSS thing — but it silently removes a command from those databases, so worth confirming it's deliberate, and checking that no automation or test invokes TIMESERIES.REFRESHCLUSTER against an ASM database.

If it is deliberate, the comment just above is now slightly wrong and could say why RE_OSS is excluded.

[galcohen-redislabs]

REFRESHCLUSTER is not sent in RE. This is what the code checks now.

[gabsow]

Closing this — f7890a4 covers it, and the intent is clear now: not sent in RE, registered for non-RE. Verified alongside the rest on a live cluster; the non-RE path is fine.

[gabsow]

This changes what "initialized" means for every non-cluster deployment. Dropping the isOss || term means MR_IsClusterInitialize() is false until a CLUSTERSET or REFRESHCLUSTER creates CurrCluster, which on a standalone server never happens.

The consequence is in mr.c:541: MR_CreateExecution() sets UINITIALIZED_CLUSTER_ERROR for every execution. So on standalone OSS, LibMR goes from "runs the execution locally" to "errors" — which is test_basic_testBasicMR-oss, the same test the assert is currently crashing, so CI can't show you this one until the assert is gone.

RedisTimeSeries happens to be shielded because it gates on IsMRCluster() -> MR_ClusterIsInClusterMode() -> MR_ClusterGetSize() > 1, but LibMR shouldn't depend on every consumer doing that.

If the intent is "we no longer need isOss to fake initialization", then the standalone case needs an explicit representation — that is really what ClusterType_NONE is for.

[galcohen-redislabs]

changed the logic (actually it was wrong then and it is wrong now, but too many tests rely on it, so keeping it like that)

[gabsow]

Good — fe63060 covers the case I was worried about, and I exercised it: a standalone multi-shard database running this branch's module answered TS.MGET WITHLABELS FILTER in 0.01 s with 84 correct rows. So the non-RE always-initialized path works and MR_CreateExecution is not handing back UINITIALIZED_CLUSTER_ERROR.

Agreed on the wider point that the semantics are odd both before and after; not something to untangle in this PR.

[AvivDavid23]

nit: Can we maybe create some functions(i.e IsClusterTypeRE/IsClusterTypeOSS) and use them instead?

[galcohen-redislabs]

I thought about it but the couldn't find a non-confusing naming scheme (i.e., IsClusterTypeRE being true doesn't tell us anything about the OSS-ness of the cluster, so the Is in the name is somewhat misleading). The best I could come up with was this explicit enum, but that is actually backed by the two bits (i.e., when read as a bitmask).

[gabsow]

Holding off on an approve, but only on two things — the flag change itself is right, and I'd rather land this than #122.

1. Guard the NULL secret (one line).

const char *secret = RedisModule_GetInternalSecret(mr_staticCtx, &len);
if (!secret) { /* fall through to n->password */ }

RedisModule_GetInternalSecret being non-NULL as an API pointer doesn't mean it returns a secret — a cluster wired purely by CLUSTERSET never gets one. That's not hypothetical, it's how I found it: an intermediate commit of #122 made this branch reachable and build (unstable) aborted on cluster.c:441 'secret' is not true. This PR keeps the harness off the branch via the CLUSTERSET-implies-RE hint, but the same hint makes a real RE_OSS database prefer it, so the exposure is larger than master's — and the failure mode is a module abort, not a retry loop. The guard is unconditionally correct whatever #2 says.

2. One live check on a cluster-mode RE database.

On an ASM database cachedClusterType is RE_OSS, so the OSS bit is set and SendAuthCommandIfNeeded authenticates with the internal secret and never sends the CLUSTERSET password. That inverts the risk onto the one path this ticket is about: the internal secret's usability on RE is unverified, while the CLUSTERSET password is known-good — it is how shard-to-shard has always authenticated there, and it is what the 340 healthy shards in the MOD-17518 package used. If RE doesn't propagate a shared secret across a database's shards, unknown command simply becomes NOAUTH and the 1 Hz loop is unchanged.

To be clear about my own part in this: my earlier claim that the July package proved the internal secret unusable on RE was wrong — Gal correctly pointed out the short-form CLUSTERSET carries AUTH <internal_pass>, so n->password was set and the password branch ran. That removes my evidence, but it doesn't create evidence the other way; it moves this from known-bad to unverified.

CI can't settle it — green means the OSS paths are healthy, and the RE-hint deliberately keeps the harness off this branch. What settles it, on any live cluster-mode database:

redis-cli -p <shard-port> COMMAND INFO timeseries.hello

_proxy-filtered with no internal confirms the flag half; the shard log then shows whether HELLO completes or turns into NOAUTH. If it turns out the secret isn't usable there, the credential preference needs flipping to password-first on RE_OSS and everything else here stands.

Happy to push the guard onto this branch, or to run the live check if someone can point me at an env0 cluster-mode database. I'll close #122 once this merges — its only remaining content is that guard and the credential ordering.

[gabsow]

Approving — verified on a live cluster-mode Enterprise database, which is the check I was holding out for. And one of my two objections was simply wrong; details below.

env0 aws-cluster-gabsow, RS 100.0.20-7719. Built this branch's pin (e091480) via RTS#2149 on the host, swapped it in for the bundled TS 8.8.3, same database config either side:

database	module	TS.MGET WITHLABELS FILTER
cluster-mode (redis_cluster_enabled, ASM off), stock 8.8.3	80803	5.0 s, "at least one shard did not reply"
cluster-mode, this branch	999999 e796e480	0.02 s, 84 rows correct
standalone, stock 8.8.3	80803	0.01 s correct
standalone, this branch	999999	0.01 s correct — no regression

The classification flips exactly as intended, same shard, same config:

stock:    <timeseries> Detected redis oss (cluster-enabled=yes)        -> 162x ERR unknown command 'timeseries.HELLO'
patched:  <timeseries> Detected redis enterprise (cluster-enabled=yes) -> bad_hello=0

Both patched shards: bad_hello=0 NOAUTH=0 asserts=0 crashes=0, with connected : to both peers.

My auth objection was wrong

I argued this would trade unknown command for NOAUTH, because it prefers the internal secret over the CLUSTERSET password on RE_OSS. It does prefer it — the OSS bit is set there, so the password branch is unreachable — and it works: zero NOAUTH, zero retries, fan-out forms immediately. So RE does propagate a usable internal secret across a database's shards, which neither support package could tell us and which I had guessed the other way. Your credential ordering is fine and my #122 ordering was solving a non-problem.

RedisModule_Assert(secret) also didn't fire, so the secret is non-NULL on RE. I'd still take the one-line if (!secret) fallback as hardening — LibMR's own harness proves the NULL state exists — but it is no longer a blocker, so treat it as optional follow-up rather than a condition of this approval.

Unrelated crash worth its own ticket

During rladmin restart db, one shard took a SIGSEGV (signal: 11, Accessing address: (nil), then a recursive fault in the signal handler). That process was running the old module (version 80803, loaded before my swap), so it is not from this branch — but it wedged the database in active-change-pending. Looks like the short-form-CLUSTERSET crash family rather than anything here.

I'll close #122; its only remaining content was the NULL guard and a credential ordering that this verification shows is unnecessary.