Skip to content

[backport] Backport sweep for 9.0 - #4686

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

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

Conversation

@valkeyrie-ops

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

Copy link
Copy Markdown

Backport sweep for 9.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.

Needs attention

These candidates could not be applied automatically and need a maintainer to follow up.

1 candidate(s)
Source PR Title Outcome Reason
#4574 Fix script debugger uaf skipped-conflict target branch lacks conflicted file(s): src/modules/lua/debug_lua.c, src/modules/lua/engine_lua.c

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. Both files are free of conflict markers, src/t_stream.c passes a gcc syntax check, and both Tcl files have balanced braces.

Here's how each conflict was handled:

src/t_stream.c (5 conflicts, all caused by two branch differences: 9.0 uses o-&gt;ptr/c-&gt;argv[n]-&gt;ptr where the source branch uses objectGetVal(), and 9.0 has no XDELEX/XACKDEL support):

  • xgroupCommand CREATE (MKSTREAM block): kept 9.0's s = o-&gt;ptr accessor and took the PR's change of removing the early signalModifiedKey there (it now fires in the if (cg) success path instead).
  • xgroupCommand CREATECONSUMER: kept 9.0's c-&gt;argv[4]-&gt;ptr call convention and added the PR's if (created) signalModifiedKey(...) line.
  • xclaimCommand and xautoclaimCommand consumer creation: kept 9.0's c-&gt;argv[3]-&gt;ptr convention and added the PR's modified = 1 on consumer creation.
  • xdelCommand tail: kept the 9.0 side unchanged. The incoming side was the 9.2 xdelGeneric body with XDELEX/XACKDEL propagation (has_pelmode, acked, streamPropagateAckIDs), none of which exists on 9.0 — I confirmed XDELEX appears nowhere on this branch. The PR's "don't emit xdel for DELREF" tweak only applies to that generic path, so it has no counterpart to backport here.

tests/unit/type/stream.tcl: resolved to the HEAD (9.0) side, i.e. no change. The conflict was one giant block of ~890 lines of XDELEX/XACKDEL tests that exist only on the source branch; the PR's actual change to this file was deleting one of those tests, which doesn't exist on 9.0.

tests/unit/type/stream-cgroups.tcl: auto-merged cleanly with the PR's three new WATCH-dirtying tests (+95 lines). I verified the helpers it uses (valkey_deferring_client, wait_for_blocked_clients_count) exist on the target branch, so no adaptation was needed.

The resolved commit is +123/−3 versus upstream's +125/−42; the smaller deletion count is entirely the XDELEX-related removals that have no counterpart on 9.0.
... summary truncated.

AI-edited files requiring review

Full backport commit diff: commit ae244d5ffcdf.

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: ba73428a-2890-40b5-992a-5790da3549c7

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. Here's a summary of what I did:

Conflict analysis: The single conflict in src/valkeymodule.h was in the block of VALKEYMODULE_API function-pointer declarations (the module API table externs), specifically around the ACL entry declarations. The conflict arose because unstable has a ValkeyModule_ACLCheckPermissions declaration adjacent to the changed lines that doesn't exist on the 9.0 branch — I confirmed with grep that ACLCheckPermissions appears nowhere else in the 9.0 sources (no VM_ACLCheckPermissions implementation in src/module.c), so it was purely conflicting context, not part of the fix.

Resolution: I took the incoming side's actual change — declaring ValkeyModule_ACLAddLogEntry and ValkeyModule_ACLAddLogEntryByUserName as returning int instead of void (matching their int VM_ACLAddLogEntry / int VM_ACLAddLogEntryByUserName implementations at src/module.c:10109 and src/module.c:10123) — while dropping the unstable-only ACLCheckPermissions declaration that doesn't belong on 9.0. The third fix from the PR, ValkeyModule_FreeModuleUser changing to int (src/valkeymodule.h:1860), had already auto-merged cleanly, as did the doc-comment addition in src/module.c noting that VM_FreeModuleUser returns VALKEYMODULE_OK.

Verification: No conflict markers remain in either file, the header passes gcc -fsyntax-only, and the resolved diff (+11/−9 lines across both files) matches the upstream PR's size (+12/−10). I did not stage or commit anything, per the constraints.

AI-edited files requiring review

Full backport commit diff: commit 3c109627a9e2.

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: This is a timing-dependent failure specific to the valgrind environment: the identical test passed in 20+ other jobs of the same run, and the dataset visibly changed between two back-to-back assertions due to replication link churn. No default-branch candidate commit fixes this test or this race, and the failing assertion is the core invariant the test exists to verify, so the only 'fixes' available would weaken the test or paper over a possible product-level race. No safe, attributable deterministic fix exists.

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