Skip to content

[backport] Backport sweep for 8.0 - #4685

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

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

Conversation

@valkeyrie-ops

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

Copy link
Copy Markdown

Backport sweep for 8.0

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

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 and no markers remain. The files stay listed as "UU" in git status only because I did not run git add, per the constraints.

What I did:

src/t_stream.c (+28/-3): The PR adds signalModifiedKey() calls to stream commands that mutate consumer-group/PEL metadata. All nine conflicts were caused by two unrelated drifts on the newer branch: an o-&gt;ptrobjectGetVal(o) accessor refactor, and reply-ordering changes (upstream moved addReply after the notify calls, plus a new XDELEX/XACKDEL propagation block in xdelCommand). Neither exists on 8.0 — I verified objectGetVal and XDELEX appear nowhere on this branch — so I kept 8.0's -&gt;ptr accessors and reply ordering, and merged in only the PR's additions:

  • xgroupCommand: moved the MKSTREAM signalModifiedKey from stream creation to successful group creation, and added signals to SETID, DESTROY, CREATECONSUMER, and DELCONSUMER.
  • xsetidCommand and xackCommand: added the missing signal calls.
  • xclaimCommand / xautoclaimCommand: kept the incoming modified flag tracking (set on consumer creation, NACK removal, claims, and last-id propagation) with a single signalModifiedKey per command. The int modified = 0; declarations and the xreadCommand modified_stream changes had already auto-merged cleanly.
  • xdelCommand: kept HEAD's version unchanged — the PR's edit there only reordered the xdel event for XDELEX DELREF, a command 8.0 doesn't have.

tests/unit/type/stream.tcl (no net change): The conflict was a ~900-line block of XACKDEL/XDELEX tests that exist only on the newer branch, with an empty HEAD side. The PR's actual change to this file was deleting one of those XDELEX tests, which isn't on 8.0, so I dropped the incoming block entirely and the file matches HEAD.

tests/unit/type/stream-cgroups.tcl (auto-merged, +95): Three new WATCH-dirtying tests. I verified the helpers they use (valkey_deferring_client, wait_for_blocked_clients_count) already exis
... summary truncated.

AI-edited files requiring review

Full backport commit diff: commit 2fcb42aafaed.

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: ac84642a-7571-4f48-9dfa-07d60472be8d

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 in src/valkeymodule.h is resolved and the tree is clean of conflict markers. Here's a summary of what I found and did:

The conflict: The source PR changes three function-pointer declarations in the module API table from void to int return type (ValkeyModule_FreeModuleUser, ValkeyModule_ACLAddLogEntry, ValkeyModule_ACLAddLogEntryByUserName) so they match their int-returning implementations in module.c. The cherry-pick conflicted because the upstream diff's context includes a ValkeyModule_ACLCheckPermissions declaration that sits between the ACL check functions and the ACL log functions on unstable — that API doesn't exist on the 8.0 branch.

The resolution: I kept the incoming side's two int (*ValkeyModule_ACLAddLogEntry...) declarations (the actual PR change) and dropped the ACLCheckPermissions declaration, since it's not part of this PR and doesn't exist anywhere on 8.0 (verified by grep against HEAD). The ValkeyModule_FreeModuleUser void→int change at valkeymodule.h:1536 had already auto-merged cleanly, as had the doc-comment addition in module.c.

Verification:

  • No conflict markers remain in src/valkeymodule.h.
  • The implementations on 8.0 already return int (VM_FreeModuleUser at module.c:9596, VM_ACLAddLogEntry at module.c:9796, VM_ACLAddLogEntryByUserName at module.c:9810), so the new declarations match.
  • The resolved diff is 12 insertions / 10 deletions across the same two files — identical in size to the upstream PR.
  • Only the two files from the cherry-pick were touched; nothing was staged or committed.

AI-edited files requiring review

Full backport commit diff: commit 0149f76d1047.

Please review these AI resolutions for correctness before merging.

@valkeyrie-ops

valkeyrie-ops Bot commented Sep 18, 2026

Copy link
Copy Markdown
Author

Automatic follow-up for the current backport head.

I did not push a fix: The failure is not deterministic: the identical test at the identical commit passed in ~29 sibling jobs of the same CI run (typically in ~20s), and the other failing jobs in this run failed on entirely different cluster tests (slot-ownership.tcl, replica-migration), the classic scatter pattern of flaky cluster-convergence tests. No default-branch candidate commit touches manual-takeover.tcl or this convergence path (FAILOVER_AUTH_NACK is a later unrelated feature, confirmed absent from this checkout; the takeover path uses no election votes). The only 'fixes' available would be inflating timeouts or weakening the recovery assertion, which the rules forbid; therefore no safe, attributable fix exists.

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-almalinux9-tls-module
  • test-sanitizer-address (clang)
  • test-sanitizer-undefined (gcc)

@valkeyrie-ops

valkeyrie-ops Bot commented Sep 18, 2026

Copy link
Copy Markdown
Author

Automatic follow-up for the current backport head.

I did not push a fix: This is a timing-dependent race exposing a real product bug: FAILOVER_AUTH_ACKs carry no election epoch, so a single voter's ACKs from two consecutive elections are double-counted (src/cluster_legacy.c:3628-3638). The upstream mitigation direction (FAILOVER_AUTH_NACK epoch echo, 5c24e5d) depends on the NACK feature, which is confirmed absent from this 8.0 branch (no FAILOVER_AUTH_NACK anywhere in src/), so it does not port cleanly, and no self-contained test-scaffolding fix exists that would not weaken what the test verifies.

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-ubuntu-jemalloc-fortify (Cluster should eventually be up again in tests/unit/cluster/manual-takeover.tcl)

@valkeyrie-ops

valkeyrie-ops Bot commented Sep 18, 2026

Copy link
Copy Markdown
Author

Automatic follow-up for the current backport head.

I did not push a fix: This is a timing-dependent/flaky failure: whether keys get evicted during init_test depends on how quickly the server releases the prior test's client buffer memory, and the helper has no wait/synchronization. It is not a product bug and not deterministic scaffolding breakage. None of the discovered default-branch commits touch tests/unit/maxmemory.tcl or this eviction path, so there is no upstream fix to port, and authoring a fix would mean designing new flakiness mitigation (e.g. waiting for used_memory to settle), which falls outside a safe mechanical fix per the rules.

Looked at the failure from this run.

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.

2 participants