Unescape escaped semicolons in a plain moduleArgs string by LiranAbir · Pull Request #256 · RedisLabsModules/RLTest

LiranAbir · 2026-09-24T13:04:25Z

Problem

fix_modulesArgs splits a moduleArgs string with split_by_semicolon, which both splits on unescaped ; and unescapes the escaped ones. The plain-string branch added in #243 computes that result and then throws it away, reusing the raw input instead:

parts = split_by_semicolon(modulesArgs)
if len(parts) == 1:
    # No semicolons - keep as plain string
    is_plain_str = True
    modulesArgs = [[modulesArgs.strip()]]   # still carries the backslashes

So a value whose separator is an escaped semicolon — the documented way to pass one — reaches the module with the backslashes intact:

	value handed to the module
before #243	`COMPACTION_POLICY max:1m:1d;min:10s:1h`
after #243	`COMPACTION_POLICY max:1m:1d\;min:10s:1h`

Note this only affects a string with no unescaped semicolon. Mixing in one plain ; takes the other branch and is handled correctly, which is why it went unnoticed.

Effect on callers

A module that parses a semicolon-separated value gets an invalid one and fails to load, which stops the server. The caller sees only

Error 111 connecting to localhost:6379. Connection refused.

against the Env it just constructed, and because the configuration is rejected before the server opens its logfile, the retained log is empty and says nothing about the cause.

This is what turns test_globalconfigs red in RedisTimeSeries on every non-cluster phase when its RLTest pin moves onto current master — both failing tests pass COMPACTION_POLICY max:1m:1d\;min:10s:1h\;avg:2h:10d\;avg:3d:100d as a plain string. Tracked as MOD-18913.

Change

Take parts[0] instead of the raw input. The split already produced the unescaped value, and for a single part it is exactly what the pre-#243 code returned, so nothing else in the branch changes — _merge_by_words then merges defaults into an already-unescaped string.

Two unit tests cover it, with and without defaults to merge. Both fail on master and pass here; the existing 15 are unaffected.

17 passed in 0.22s

🤖 Generated with Claude Code

fix_modulesArgs computes the unescaped parts, then discards them and reuses the raw input for the single-part case, so the backslash of an escaped semicolon reaches the module verbatim. A module that takes a semicolon- separated value then gets an invalid one and fails to load, which stops the server: the caller sees only "Connection refused" against the Env it just built, and a config rejected this early leaves an empty logfile behind. Regression from RedisLabsModules#243, which introduced the plain-string branch. The split already returns the unescaped value, so take it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

codecov-commenter · 2026-09-24T13:14:22Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 34.97%. Comparing base (02cfbb9) to head (8dc964e).
⚠️ Report is 22 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #256      +/-   ##
==========================================
+ Coverage   32.46%   34.97%   +2.51%     
==========================================
  Files          17       18       +1     
  Lines        2597     2745     +148     
==========================================
+ Hits          843      960     +117     
- Misses       1754     1785      +31

Flag	Coverage Δ
unittests	`34.97% <100.00%> (+2.51%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gabsow · 2026-09-24T14:40:39Z

+            # of any escaped semicolon, and passing those on to the module is fatal.
            is_plain_str = True
-            modulesArgs = [[modulesArgs.strip()]]
+            modulesArgs = [[parts[0]]]


This can be a breaking chenge for redis serche, no?
@alonre24 ?

I don't think so, I assume that if tests are passing, then it should be sufficient...

gabsow

Approved at 8dc964e after compatibility validation against RediSearch master 811145e9cb94d9894e2dcb9ea5a3309085bcff66.

RLTest argument-parser and environment-spec unit tests: 29 passed.
Baseline RLTest 0.7.29 and this patch discover the same 2,617 Search test IDs.
Focused Search integration tests (test_config, test_multithread, test_query_oom, test_coord_query_args, test_index_list) pass with zero failures in standalone and three-shard OSS cluster on Redis 8.10.0 and current unstable 20bb2cfc. The baseline standalone run also passes.

Validation was local on macOS ARM64 with a fresh Search build. This does not claim a full Linux, sanitizer, TLS, or all-platform nightly run.

alonre24

Approved based on @gabsow succesful run against RediSearch

RedisLabsModules/RLTest#256 merged as 7e9f69ac1, so the escaped-semicolon fix is available upstream and the pin no longer has to reference a fork. Same code either way - 8dc964e was that PR's head - so this changes nothing in CI, it just stops the branch from merging with a personal fork as a dependency.

…#2197) * MOD-18913 - Pin the RLTest fix for escaped semicolons in moduleArgs The plain-string branch of fix_modulesArgs reuses the raw input instead of the unescaped split, so an escaped semicolon reaches the module with its backslash. test_globalconfigs passes COMPACTION_POLICY that way, the module then fails to load, the server never starts, and the test reports only Connection refused against the Env it just built - on gen, slaves, aof and aof_slaves. Fixed upstream in RLTest#256; pinned to that commit until it merges. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Trigger CI * Point the pin at upstream RLTest now that the fix has merged RedisLabsModules/RLTest#256 merged as 7e9f69ac1, so the escaped-semicolon fix is available upstream and the pin no longer has to reference a fork. Same code either way - 8dc964e was that PR's head - so this changes nothing in CI, it just stops the branch from merging with a personal fork as a dependency. * Spell the RLTest pin as a full SHA An abbreviated SHA only resolves while the commit stays reachable from the default branch, because a git fetch of a specific object needs the full hash and an abbreviation is resolved after the clone. It works today - CI on the previous commit passed - but it is a needless dependency on that reachability, and the pin this replaced was a full SHA. Same commit either way. Raised by Cursor Bugbot on 965b2db. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

…ack (#2194) * MOD-18751: waive cluster bus protected mode for the oss-cluster phase redis/redis#15722 added cluster-bus-port-protected-mode, and on the 8.12 line it defaults to enabled: a node started with cluster-enabled and without tls-cluster then refuses to start, because its cluster bus port would be unauthenticated. That is how the oss-cluster phase is built, so every shard dies at startup and each test fails at connect with "Connection refused". The refusal happens while the configuration is validated, before the server opens its logfile, so the retained logs are empty and say nothing about the cause. Waive it for that phase. The bus ports here are bound to localhost on an ephemeral host, which is the condition the directive documents for waiving, and they were equally unauthenticated before the redis change - which only made running them an explicit choice. The option is not passed blind, because a redis that does not have it rejects the directive and fails to start the same way. RLTest takes it as an option rather than guessing, since nothing it can see distinguishes a build that has the option from one that does not: the 8.12 line reports 8.9.241 in version.h, the same as builds from before the change. CLUSTER_BUS_PROTECTED_MODE= omits it for anyone running the cluster phase against an older redis locally. The tls_cluster phase is left alone, since tls-cluster authenticates the bus, and so is the existing-env phase, where RLTest starts no servers. Requires RLTest with the option (RedisLabsModules/RLTest#255). * Keep the waiver inside the oss-cluster phase Setting it on RLTEST_ARGS in the OSS_CLUSTER block leaks into the tls_cluster block below, which is a separate if and inherits the parent shell's value - the tls_cluster phase was getting the option too, which this change had claimed it would not. Set it inside each oss-cluster invocation's own subshell instead, as the other phases do. Found by dry-running tests.sh with NOP=1 and reading the generated RLTest configuration per phase. Verified after: one occurrence, in "tests on OSS cluster" only; the general, slaves, AOF and tls_cluster phases carry none. Note the pre-existing duplicate --cluster_node_timeout in the tls_cluster configuration, which comes from the same leak and is left alone here. * Trigger CI * MOD-18906 - Start the failover test's replicas on a protected cluster bus (#2196) test_topology_events:test_failover spawns its own cluster-node replicas, rather than relying on RLTest, so RLTest's cluster-bus-port-protected-mode waiver does not reach them. Since redis/redis#15722 a cluster node whose bus port is not authenticated by tls-cluster refuses to start unless protection is waived, so those replicas died at startup and the test timed out after 2308s on every oss-cluster leg. Mirror the master's value of the option onto the replica we spawn. The config is absent on older redis, which reports an empty dict and gets nothing added, since passing an unknown config would be fatal there instead. Also bound the wait for the replica to serve. It was the one unbounded wait in the helper, so a replica that never started produced only a test timeout, with the cause nowhere in the output: a config redis rejects is reported before the logfile is opened, leaving an empty log. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * MOD-18913 - Cap redis-py below 6 so the suite stays on RESP2 RLTest's own requirement on the client is uncapped (redis = ">=5.0.0"), where the commit this repo was pinned to until now capped it at "^5.0.0rc2". Moving the pin therefore let pip install redis-py 8.1.0, which negotiates RESP3 by default, and under RESP3 TS.MRANGE ... GROUPBY replies with a map rather than rows: ((filtered_by, withlabels, samples),) = result ValueError: too many values to unpack (expected 3) Pin the client to the 5.x series, which is what the old RLTest pin was silently giving us, so the pin bump does not carry a protocol change with it. Supporting RESP3 in the assertions is the real fix and stays with MOD-18913. * Restore the MOD-18906 replica fix It was reverted by accident in the previous commit, which picked up a stale staged copy of the file alongside the requirements change. * MOD-18913: absorb the RLTest upgrade - unescape moduleArgs semicolons (#2197) * MOD-18913 - Pin the RLTest fix for escaped semicolons in moduleArgs The plain-string branch of fix_modulesArgs reuses the raw input instead of the unescaped split, so an escaped semicolon reaches the module with its backslash. test_globalconfigs passes COMPACTION_POLICY that way, the module then fails to load, the server never starts, and the test reports only Connection refused against the Env it just built - on gen, slaves, aof and aof_slaves. Fixed upstream in RLTest#256; pinned to that commit until it merges. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Trigger CI * Point the pin at upstream RLTest now that the fix has merged RedisLabsModules/RLTest#256 merged as 7e9f69ac1, so the escaped-semicolon fix is available upstream and the pin no longer has to reference a fork. Same code either way - 8dc964e was that PR's head - so this changes nothing in CI, it just stops the branch from merging with a personal fork as a dependency. * Spell the RLTest pin as a full SHA An abbreviated SHA only resolves while the commit stays reachable from the default branch, because a git fetch of a specific object needs the full hash and an abbreviation is resolved after the clone. It works today - CI on the previous commit passed - but it is a needless dependency on that reachability, and the pin this replaced was a full SHA. Same commit either way. Raised by Cursor Bugbot on 965b2db. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

LiranAbir mentioned this pull request Sep 24, 2026

MOD-18913: absorb the RLTest upgrade - unescape moduleArgs semicolons RedisTimeSeries/RedisTimeSeries#2197

Merged

gabsow reviewed Sep 24, 2026

View reviewed changes

gabsow approved these changes Sep 27, 2026

View reviewed changes

alonre24 approved these changes Sep 27, 2026

View reviewed changes

LiranAbir merged commit 7e9f69a into RedisLabsModules:master Sep 27, 2026
9 checks passed

LiranAbir mentioned this pull request Sep 27, 2026

MOD-18751 / MOD-18906 / MOD-18913: bring the oss-cluster flow tests back RedisTimeSeries/RedisTimeSeries#2194

Merged

Navigation Menu

Unescape escaped semicolons in a plain moduleArgs string - #256

Unescape escaped semicolons in a plain moduleArgs string#256
LiranAbir merged 1 commit into
RedisLabsModules:masterfrom
LiranAbir:MOD-18913-fix-modulesargs-escapes

LiranAbir commented Sep 24, 2026

Uh oh!

codecov-commenter commented Sep 24, 2026

Uh oh!

gabsow Sep 24, 2026

Uh oh!

alonre24 Sep 27, 2026

Uh oh!

gabsow left a comment

Uh oh!

alonre24 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

LiranAbir commented Sep 24, 2026

Problem

Effect on callers

Change

Uh oh!

codecov-commenter commented Sep 24, 2026

Codecov Report

Uh oh!

gabsow Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

alonre24 Sep 27, 2026

Choose a reason for hiding this comment

Uh oh!

gabsow left a comment

Choose a reason for hiding this comment

Uh oh!

alonre24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants