Skip to content

[backport] Backport sweep for 8.1 - #4687

Open
valkeyrie-ops[bot] wants to merge 4 commits into
8.1from
agent/backport/sweep/8.1
Open

valkeyrie-ops[bot] wants to merge 4 commits into
8.1from
agent/backport/sweep/8.1

Conversation

@valkeyrie-ops

@valkeyrie-ops valkeyrie-ops Bot commented Sep 15, 2026

Copy link
Copy Markdown

Backport sweep for 8.1

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

Applied

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

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


Generated by valkey-ci-agent using Claude Code.

Fix missing `signalModifiedKey()` calls for stream commands that mutate
stream metadata rather than stream entries themselves.

Fixes #3429

A few stream commands already mutate consumer-group or PEL state,
increment `server.dirty`, and propagate changes to AOF / replicas, but
they do not always mark the key as modified for `WATCH` and client
tracking invalidation.

Affected command paths:
- `XGROUP CREATE/SETID/DESTROY/CREATECONSUMER/DELCONSUMER`
- `XSETID`
- `XREADGROUP`
- `XACK`
- `XCLAIM`
- `XAUTOCLAIM`

Changes:
- Added `signalModifiedKey()` to `XGROUP
  CREATE/SETID/DESTROY/CREATECONSUMER/DELCONSUMER`
- Added `signalModifiedKey()` to `XSETID`
- Updated `XREADGROUP` to signal key modification when synchronous group
  reads or consumer creation mutate stream metadata
- Updated `XACK`, `XCLAIM`, and `XAUTOCLAIM` to signal key modification
  once per command when they actually mutate PEL or consumer-group state

Additional change:
- Don't emit xdel event for XDELEX DELREF, when removing an orphaned PEL
  ref without deleting the actual stream entry.

---------

Signed-off-by: Tarte <emprimula@gmail.com>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Co-authored-by: Ran Shidlansik <ranshid@amazon.com>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Backport-Source-PR: 3459
@valkeyrie-ops valkeyrie-ops Bot added ai-resolved-conflicts AI is used to resolve conflicts in automated backport PRs. backport Backport PR opened by valkey-ci-agent labels Sep 15, 2026
@valkeyrie-ops

valkeyrie-ops Bot commented Sep 15, 2026

Copy link
Copy Markdown
Author

AI backport resolution: source PR #3459

Fix missing signalModifiedKey calls for stream commands

2 AI-edited files

Claude Summary

All conflicts are resolved, no markers remain, and the file compiles cleanly (make t_stream.o succeeded; I removed the test object file afterward so the tree only contains the three intended file changes).

What I did:

src/t_stream.c — 5 conflict regions, all inside functions the PR touched. Four of them were textual clashes because the 8.1 branch uses direct -&gt;ptr access where unstable uses the newer objectGetVal() macro. I kept 8.1's -&gt;ptr convention (matching the surrounding file) and layered the PR's logic on top:

  • xgroupCommand CREATE/MKSTREAM: kept s = o-&gt;ptr; and preserved the PR's move of signalModifiedKey out of the MKSTREAM block into the if (cg) success branch (that part auto-merged).
  • xgroupCommand CREATECONSUMER: added the PR's if (created) signalModifiedKey(...) while keeping the 8.1-style streamCreateConsumer call.
  • xclaimCommand and xautoclaimCommand consumer creation: added modified = 1; while keeping the 8.1-style calls. The int modified = 0; declarations and all other modified = 1 / if (modified) signalModifiedKey(...) hunks auto-merged cleanly.
  • xdelCommand tail: kept the 8.1 side unchanged. The PR side of that conflict belonged to the XDELEX/XACKDEL feature (acked, has_pelmode, streamPropagateAckIDs, etc.), which does not exist on 8.1 — so the PR's "additional change" about the XDELEX DELREF xdel event is not applicable to this branch.

tests/unit/type/stream.tcl — the single large conflict was the XDELEX/XACKDEL test block that exists upstream but not on 8.1. The PR only deleted one test from inside that block, so I kept the empty HEAD side; the file ends up identical to 8.1 HEAD (net zero change), which is the correct adaptation.

tests/unit/type/stream-cgroups.tcl — auto-merged with the PR's 95 lines of new WATCH-dirtying tests; I verified the helpers they use (valkey_deferring_client, wait_for_blocked_clients_count) exist on 8.1, so no edits were needed.

The resulting di
... summary truncated.

AI-edited files requiring review

Full backport commit diff: commit e66a0ecc820a.

Please review these AI resolutions for correctness before merging.

@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

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: 5b60c68e-aec5-454e-807c-c9a7399e94e4

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

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

Debian 11 Bullseye has reached end of life, and package rotations are
causing apt installs in the build-debian-old job to fail with 404 errors.

Switch the job to Debian 12 Bookworm so it continues testing against the
oldest supported Debian toolchain without relying on EOL package mirrors.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Backport-Source-PR: 4613
#4676)

`ValkeyModule_FreeModuleUser`, `ValkeyModule_ACLAddLogEntry` and
`ValkeyModule_ACLAddLogEntryByUserName` are declared in valkeymodule.h
as returning void, but the implementations (`VM_FreeModuleUser`,
`VM_ACLAddLogEntry`, `VM_ACLAddLogEntryByUserName`) return int, and the
latter two document `VALKEYMODULE_OK` / `VALKEYMODULE_ERR`. Modules
calling them through the API table therefore call an int function via a
void function pointer, which is undefined behavior in C. On x86-64 and
arm64 the discarded return register makes it harmless in practice; on a
target with strict indirect-call signature checking (WebAssembly) the
call traps.

Declare them as int to match the implementation. Existing modules that
ignore the return value are unaffected; modules can now check the result
of the ACL log calls as documented. Also document that `FreeModuleUser`
returns `VALKEYMODULE_OK`.

Found by compiling the server with Emscripten, where
`redis.acl_check_cmd` in a Lua script (which calls `FreeModuleUser`
through the API table) trapped with 'null function or function signature
mismatch'.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Backport-Source-PR: 4676
@valkeyrie-ops

valkeyrie-ops Bot commented Sep 16, 2026

Copy link
Copy Markdown
Author

AI backport resolution: source PR #4676

Fix return types of three module API declarations to match definitions

1 AI-edited file

Claude Summary

The conflict is resolved and verified. Here's a summary:

Conflict cause: The upstream PR #4676 changes three module API function-pointer declarations in src/valkeymodule.h from void to int return types, to match their module.c implementations (VM_FreeModuleUser, VM_ACLAddLogEntry, VM_ACLAddLogEntryByUserName). The cherry-pick conflicted because on unstable those declarations sit next to a ValkeyModule_ACLCheckPermissions declaration that doesn't exist on the 8.1 branch, so the surrounding context differed.

Resolution: In the conflict region at src/valkeymodule.h:1821, I took the incoming int declarations for ValkeyModule_ACLAddLogEntry and ValkeyModule_ACLAddLogEntryByUserName, and dropped the ValkeyModule_ACLCheckPermissions declaration that leaked in from the source branch's context — I confirmed it exists nowhere else on 8.1 (no implementation, no VALKEYMODULE_GET_API entry), so keeping it would have added an API the PR didn't introduce. The ValkeyModule_FreeModuleUser change at line 1801 and the module.c doc-comment change had already merged cleanly.

Verification:

  • No conflict markers remain in either file.
  • The implementations on 8.1 already return int (src/module.c:9800, 10007, 10021), so the declarations now match.
  • gcc -fsyntax-only passes on the header.
  • The final diff is 12 insertions / 10 deletions across the two files — identical to the upstream commit's stat.

I did not run git add or git commit, per instructions.

AI-edited files requiring review

Full backport commit diff: commit 6de553914ab1.

Please review these AI resolutions for correctness before merging.

Pointed out by @zuiderkwast in [PR
#3798](#3798 (comment)).

Valkey remembers that TLS was enabled between builds, but not which
OpenSSL installation was selected. After building with a custom
`OPENSSL_PREFIX`, running a plain `make` later may discover a different
system OpenSSL.

This changes the build flags, triggers an unexpected full rebuild, and
links Valkey against an unintended OpenSSL installation.

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Backport-Source-PR: 4738
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-resolved-conflicts AI is used to resolve conflicts in automated backport PRs. backport Backport PR opened by valkey-ci-agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants