[backport] Backport sweep for 7.2

[valkeyrie-ops]

Backport sweep for 7.2

Automated cherry-picks from PRs marked "To be backported".

Applied

Source PR	Title	Detail
#4395	Fix mem_clients_type_memory leak for cached primary client	conflicts resolved by Claude Code
#4613	Move build-debian-old CI job to Debian Bookworm
#3459	Fix missing signalModifiedKey calls for stream commands	conflicts resolved by Claude Code
#4676	Fix return types of three module API declarations to match definitions	conflicts resolved by Claude Code
#4738	Persist OPENSSL_PREFIX across builds	conflicts resolved by Claude Code

AI resolution details are posted as comments on this PR when available.

---

Generated by valkey-ci-agent using Claude Code.

[valkeyrie-ops]

AI backport resolution: source PR #4395

Fix mem_clients_type_memory leak for cached primary client

2 AI-edited files

Claude Summary

Both conflicts are resolved. Here's a summary of what I found and did:

src/networking.c — resolved to HEAD side (no code change)

The upstream PR changes the rollback guard in freeClient() from if (c->conn) to if (c->last_memory_usage), so a cached primary's memory (whose conn was NULLed by unlinkClient() during caching) is still subtracted from stat_clients_type_memory[].

That buggy if (c->conn) guard doesn't exist on 7.2. It was introduced by commit d064002 ("Fix race condition issues between the main thread and module threads"), which is only on 8.0 and later — no 7.2 ref contains it. On 7.2, freeClient() performs the subtraction unconditionally (src/networking.c:1729-1732, after unlinkClient()), so the leak the PR fixes cannot occur and the PR's intended behavior ("always subtract a previously counted client, regardless of c->conn") is already what 7.2 does. That's also why git presented an empty HEAD side for this conflict.

Taking the upstream block verbatim would have been wrong on three counts: debugServerAssert and server.cached_primary don't exist on 7.2 (it has serverAssert and server.cached_master), and it would double-subtract alongside the existing unconditional rollback, corrupting the stat. So I dropped the conflict block and left freeClient() identical to the 7.2 HEAD.

tests/integration/replication.tcl — kept only the PR's new test

The conflict was at the end of the file: the upstream side carried ~160 lines of unrelated neighboring tests (dual-channel replication, commandlog, NOMASTERLINK) that exist on newer branches but not on 7.2 — they were just diff context, and depend on features 7.2 lacks (dual-channel-replication-enabled, COMMANDLOG). I discarded those and appended only the PR's new test, "Cached primary discard must not leak stat_clients_type_memory", verbatim at the end of the file. It passes on 7.2 code as a regression guard.

I verified every helper the test uses exists on 7.2: `wait_for_syn
... summary truncated.

AI-edited files requiring review

• src/networking.c — view diff
• tests/integration/replication.tcl — view diff

Full backport commit diff: commit c38faddcf164.

Please review these AI resolutions for correctness before merging.

[valkeyrie-ops]

AI backport resolution: source PR #3459

Fix missing signalModifiedKey calls for stream commands

3 AI-edited files

Claude Summary

That output is from the earlier background check that I had already killed and superseded — the "Terminated" line is the hung tclsh -c invocation I stopped with pkill, and the "no such file" error is because that background shell wasn't in the repository directory when it ran the heredoc.

It has no bearing on the resolution: I re-ran the same brace-balance check in the foreground from the repo root afterwards, and it confirmed both tests/unit/type/stream.tcl and tests/unit/type/stream-cgroups.tcl are structurally complete. The conflict resolution stands as summarized — all markers removed, t_stream.c passes the gcc syntax check, and the files are ready to stage.

AI-edited files requiring review

• src/t_stream.c — view diff
• tests/unit/type/stream.tcl — view diff
• tests/unit/type/stream-cgroups.tcl — view diff

Full backport commit diff: commit ed1dc7f37aee.

Please review these AI resolutions for correctness before merging.

[coderabbitai]

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 5e60235b-a755-49a7-be94-da725a0e892d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[valkeyrie-ops]

AI backport resolution: source PR #4676

Fix return types of three module API declarations to match definitions

1 AI-edited file

Claude Summary

Resolved. The conflict in src/valkeymodule.h is fixed and no conflict markers remain in the tree.

What the conflict was: the source PR changes three module API function-pointer declarations from void to int return type (ValkeyModule_FreeModuleUser, ValkeyModule_ACLAddLogEntry, ValkeyModule_ACLAddLogEntryByUserName) to match their int-returning implementations in module.c. The conflict arose only because the upstream branch had reformatted that declaration block (multi-line parameter wrapping) and added a ValkeyModule_ACLCheckPermissions declaration that doesn't exist on 7.2 — ACLCheckPermissions has no implementation anywhere on this branch, so pulling it in would have been wrong.

How I resolved it: I kept the 7.2 branch's single-line declaration style and applied only the three void → int return-type changes (src/valkeymodule.h:1281, 1291-1292). I verified the corresponding implementations on 7.2 (VM_FreeModuleUser at src/module.c:9519, VM_ACLAddLogEntry at 9722, VM_ACLAddLogEntryByUserName at 9733) all already return int, so the declarations now match. The auto-merged module.c change (a doc-comment addition saying FreeModuleUser returns VALKEYMODULE_OK) applied cleanly and needed no adaptation.

Verification: the final diff is 6 insertions / 4 deletions across the two files, in line with the upstream PR's functional change (its larger line count came purely from the reformatted block on the newer branch), and the header passes a gcc -fsyntax-only compile check. I have not run git add or git commit, per instructions.

AI-edited files requiring review

• src/valkeymodule.h — view diff

Full backport commit diff: commit 6b8cd8a5bd80.

Please review these AI resolutions for correctness before merging.

[valkeyrie-ops]

Automatic follow-up for the current backport head.

I did not push a fix: Cannot verify the 'test-almalinux8-jemalloc' job in a controlled environment (job 'test-almalinux8-jemalloc' not found in any workflow, or its environment is unsupported); refusing rather than pushing an unverified fix.

Looked at the failure from this run.

Other checks also failed in that run; re-invoke with the same command to address the next one:

• test-sanitizer-undefined (clang): Verify health as fail for killed node (cluster test) FAILED

[valkeyrie-ops]

Automatic follow-up for the current backport head.

I did not push a fix: This is a timing-dependent flake, not a deterministic breakage: the same test passed in all other jobs of this run, and even the default branch's modern copy of the test (tests/unit/cluster/cluster-shards.tcl) uses the identical 5-second wait, so no upstream fix exists to port. Widening the timeout myself would just mask nondeterministic sanitizer slowness rather than fix mechanical scaffolding, and the failure cannot be deterministically reproduced to verify such a change.

Looked at the failure from this run.

Other checks also failed in that run; re-invoke with the same command to address the next one:

• test-almalinux8-jemalloc (SHUTDOWN NOSAVE can kill a timedout script anyway in tests/unit/scripting.tcl)

[valkeyrie-ops]

AI backport resolution: source PR #4738

Persist OPENSSL_PREFIX across builds

1 AI-edited file

Claude Summary

The conflict in src/Makefile is resolved and the Makefile parses cleanly.

The conflict was inside the persist-settings recipe. The upstream side carried two lines from its context: the new echo OPENSSL_PREFIX=... line (the PR's actual change) and echo BUILD_RDMA=..., which is only context from the newer branch — BUILD_RDMA doesn't exist anywhere else in the 7.2 Makefile, so writing it to .make-settings would be meaningless there. I kept just the OPENSSL_PREFIX line, placed after BUILD_TLS exactly as in the source PR:

	echo BUILD_TLS=$(BUILD_TLS) >> .make-settings
	echo OPENSSL_PREFIX=$(OPENSSL_PREFIX) >> .make-settings
	echo USE_SYSTEMD=$(USE_SYSTEMD) >> .make-settings

OPENSSL_PREFIX is already a real variable on 7.2 (defined around src/Makefile:163 and used at src/Makefile:229-233), so persisting it works as intended. Verification: no conflict markers remain, the resolved diff adds exactly 1 line (matching the upstream 1-line PR), and make -n persist-settings runs without syntax errors and emits the new line. No other files were touched, and nothing was staged or committed.

AI-edited files requiring review

• src/Makefile — view diff

Full backport commit diff: commit 75ed47314bc1.

Please review these AI resolutions for correctness before merging.