[backport] Backport sweep for 9.0 - #4167
Merged
Merged
Conversation
![valkeyrie-bot[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Replace ~10k individual INCR round-trips with single SET commands using large values. And set shutdown-timeout 0 before shutting down the node. Signed-off-by: Sushil Paneru <sushil.paneru1@gmail.com>
#3770) The weekly CI workflow (running from `unstable`) passes `--failures-output test-failures/valkey.json` to `./runtest` when testing the 7.2 branch. This flag was added to the unstable `daily.yml` on May 7 (commit 199d49a). The 7.2 test runners do not recognize it, print "Wrong argument: --failures-output", and exit 1 — so no tests actually run. The workflow also references `.github/actions/upload-test-failures` which does not exist on 7.2, causing a second failure. ``` $ tclsh tests/test_helper.tcl --failures-output test-failures/valkey.json --list-tests Wrong argument: --failures-output $ echo $? 1 ``` Backports `--failures-output` support from unstable to the 7.2 test infrastructure: - `tests/test_helper.tcl`: accepts the flag and writes structured JSON of failures on completion - `tests/instances.tcl`: accepts the flag in the sentinel/cluster runner - `.github/actions/upload-test-failures/action.yml`: adds the missing composite action Verified the exact CI command succeeds after the fix: ``` $ tclsh tests/test_helper.tcl --io-threads --accurate --failures-output test-failures/valkey.json --verbose --tags network --dump-logs --list-tests (lists tests, exits 0) $ tclsh tests/sentinel/run.tcl --failures-output test-failures/sentinel.json --help (shows help, exits 0) ``` Both commands fail with exit 1 without the fix and succeed with it. --------- Signed-off-by: Sana Nessreddine <sananes@amazon.com> Signed-off-by: Yaron Sananes <yaron.sananes@gmail.com>
valkeyrie-ops
Bot
added
the
ai-resolved-conflicts
Author
AI conflict resolution: source PR #3770Add --failures-output flag to test runners for weekly CI compatibility 1 conflicted file Claude Summary
AI-resolved conflicted files
Full backport commit diff: commit b898b171b4b3.
|
This improves COB memory tracking when using copy avoidance for bulk string replies. This fix addresses underestimation of client memory usage that occurred when reply buffers stored pointers to shared `robj` instead of copying data. IO threads calculate actual reply sizes by calling `sdslen()` on strings before writing, for that we need atomic `tracked_for_cob` flag in payload headers to prevent race conditions and double accounting. See #2396 --------- Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com>
… tests (#3404) These failures seem to be attributed to a race condition in the Aborted test case. `rdb-key-save-delay` 10000 was being set after `$master exec` triggered the full resync. Since repl-diskless-sync-delay 0 was set, the master would immediately start streaming the RDB to the replica once it reconnected. On the ARM runner, when it's fast enough, the entire RDB generation and transfer could complete in ~78ms, before the delay was ever applied. This meant the replica would complete the swap and have 1010 keys instead of the expected 200 and there would be no async_loading window to observe or abort which led to the failures. We saw this in the daily test failure logs ``` 92948:S * RDB memory usage when created 110.85 Mb 92948:S * Done loading RDB, keys loaded: 1010, keys expired: 0 ``` The fix moves `rdb-key-save-delay 10000` to before `$master exec` to guarantee the delay is in effect on the master before the RDB generation begins. Closes #3394, closes #3395. Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
## Problem The test `Test module aof save on server start from empty` in `tests/unit/moduleapi/hooks.tcl` sporadically crashes with `I/O error reading reply`. **Frequency:** 2 out of 15 days (March 26 on `centosstream9-tls-module-no-tls`, April 8 on `fedorarawhide-jemalloc`). **Example failing run:** https://github.com/valkey-io/valkey/actions/runs/24110987718/job/70345236353 ## Root Cause The crash is a **use-after-unload** in the auth test module's blocking authentication thread, NOT a timing issue in the AOF test. The crash log from April 8 shows: ``` 71112:M 00:42:59.710 * Module testacl unloaded 71112:M 00:42:59.711 # crashed by signal: 11, si_code: 1 71112:M 00:42:59.711 # Crashed running the instruction at: 0x7f9dc717384b ``` The sequence: 1. `blocking_auth_cb` spawns a background thread (`AuthBlock_ThreadMain`) that sleeps 500ms 2. Thread wakes, calls `ValkeyModule_UnblockClient()` → main thread processes unblock, decrements `module->blocked_clients` 3. Auth command completes, test calls `r module unload testacl` 4. `moduleUnloadInternal` checks `blocked_clients == 0` if true, proceeds with `dlclose()` 5. **But the background thread is still executing cleanup code** (freeing strings, returning from function) 6. Thread returns into unmapped memory → **SIGSEGV** The `invalidFunctionWasCalled` in the stack trace is the crash handler's safety stub, and the crashing address `0x7f9dc717384b` is in the unmapped auth.so address space. ## Fix Track the background thread ID and `pthread_join()` it in `ValkeyModule_OnUnload` before the module is dlclose'd. This ensures the thread has fully exited before the code is unmapped. The key insight is that `ValkeyModule_UnblockClient()` signals "auth is done" but not "thread is done" — the thread still has cleanup code to execute after that call. `pthread_join()` is the correct synchronization point because it only returns after the thread has fully exited. No mutex is needed since both `blocking_auth_cb` (which creates the thread) and `OnUnload` (which joins it) run on the main event loop thread. Changes to `tests/modules/auth.c`: - Add global `blocking_auth_tid` and `blocking_auth_tid_valid` flag - Set `blocking_auth_tid_valid = 1` after successful `pthread_create` - In `OnUnload`, `pthread_join` the thread if one was created ## Testing Ran `unit/moduleapi/hooks` 100 loops on rpm-distros and ubuntu runners — **all passed**: - **Workflow run:** https://github.com/roshkhatri/valkey/actions/runs/24164276124 - **Config:** `--loops 100 --single unit/moduleapi/hooks` on `almalinux8`, `almalinux9`, `fedoralatest`, `fedorarawhide`, `centosstream9`, `ubuntu-jemalloc`, `ubuntu-arm` - **Result:** 7/7 jobs ✅, zero failures across 700 total test iterations Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
sarthakaggarwal97
pushed a commit
that referenced
this pull request
Jul 17, 2026
…yes (#3548) (#4182) Backport of #3548 to 9.0. Picked up from the "Needs attention" list in sweep #4167, which skipped it because the target branch lacks src/modules/lua/engine_lua.c. **Adaptation for 9.0:** only fix #2 from the original PR (the `lua_insecure_api_current` sync in server.c) applies here. Fix #1 does not exist on 9.0: the Lua engine is not modularized on this branch (src/lua/, not src/modules/lua/), there is no per-engine-context `lua_enable_insecure_api` field hardcoded to 0, and the deprecated-API allowlist gate reads `server.lua_enable_insecure_api` directly in `luaNewIndexAllowList` (script_lua.c). Since `evalInit()` runs in `initServer()` after `loadServerConfig()`, the config-file value is already honored at Lua state initialization. The engine_lua.c hunk is therefore dropped. **Validation on 9.0:** - Full `unit/scripting` suite passes with the fix - Both new tests fail without the fix with the expected symptom: `getfenv()` remains accessible after `CONFIG SET lua-enable-insecure-api no` because `updateLuaEnableInsecureApi()` sees `0 != 0` as false and skips `evalReset()` Signed-off-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Binbin <binloveplay1314@qq.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport sweep for 9.0
Automated cherry-picks from PRs marked "To be backported".
Applied
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.
3 candidate(s)
Generated by valkey-ci-agent using Claude Code.