Repository navigation
test: add multi-SDK e2e command stress tests with dual-client mode - #406
Merged
Merged
Conversation
Add a unified e2e test suite that launches an EloqKV cluster and runs full Redis command coverage stress across Python (redis-py), Go (go-redis/v9), and TypeScript (ioredis) SDKs. Each SDK runs 16 workers split evenly between standalone and cluster clients, covering 104 Redis commands with TLS enabled. The topology verifies master/standby/voter deployment over MinIO-backed storage. Key changes: - cmd_stress_test.sh: single entry point for launch → stress → remove - cmd_stress_py/main.go/main.ts: per-SDK stress clients with dual mode - docker_env.sh: compose override support, configurable version/storage - topology.yaml: version + storage parameterized via env vars - GitHub Actions workflow with version/storage/workers inputs Remove superseded legacy tests (test.sh, stress_test.sh, stress.py, go_cluster_tls_stress, memtier_cluster_tls_stress).
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughReplaces legacy shell E2E scripts with a Docker-based multi-SDK stress harness (Python, Go, TypeScript), updates Docker/MinIO/control-node test infra, adds a GitHub Actions test workflow, rewrites tests docs, and applies multiple cluster_mgr code/config changes for downloads, local extraction, rsync uploads, and monitoring. ChangesE2E Stress Test Framework & Clients
Docker Infrastructure & Test Environment
cluster_mgr code, tasks, and observability
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (1)
scripts/test-before-push.sh (1)
20-21: ⚡ Quick winConsider lighter default stress settings for pre-push.
Running full default stress on every push can slow local iteration; setting smaller defaults here keeps the hook practical while preserving signal.
Suggested change
echo "[4/5] Run Docker E2E suite" -bash tests/e2e/cmd_stress_test.sh +DURATION_SECONDS="${DURATION_SECONDS:-10}" \ +WORKERS="${WORKERS:-4}" \ +bash tests/e2e/cmd_stress_test.sh🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test-before-push.sh` around lines 20 - 21, The pre-push script currently runs the full stress e2e suite by calling tests/e2e/cmd_stress_test.sh, which is too heavy for frequent pushes; change scripts/test-before-push.sh to invoke that script with lighter defaults (for example exporting a small STRESS_ITERATIONS or STRESS_DURATION env var, or passing a --quick/--short flag) so the hook runs a quick smoke/stress subset instead of the full default; update the invocation in scripts/test-before-push.sh (the bash call to tests/e2e/cmd_stress_test.sh) to pass the chosen short-mode parameter and ensure cmd_stress_test.sh honors that flag/env var.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e.yml:
- Around line 95-98: After downloading artifacts referenced by variables TX and
LOG from BASE in the e2e workflow, verify their integrity before caching/using
them by fetching the corresponding checksum file or known-good hash and running
a checksum verification (e.g., sha256) against the downloaded files; update the
.github/workflows/e2e.yml steps that perform curl -o
"$HOME/.eloqctl/download/${TX}" and curl -o "$HOME/.eloqctl/download/${LOG}" to
also curl the checksum file (or embed expected hashes), then run a verification
command (sha256sum -c or shasum -a 256 --check) for ${TX} and ${LOG} and fail
the job if verification fails so tampered or corrupted downloads are rejected.
- Around line 40-41: The checkout step currently uses actions/checkout@v4 and
should disable persisting git credentials; update the Checkout step (the
actions/checkout@v4 usage) to set persist-credentials: false so credentials are
not stored locally (since the workflow has permissions: contents: read).
- Around line 44-55: Replace mutable GitHub Actions refs with immutable commit
SHAs: update each `uses:` entry that currently references tags/branches (e.g.,
actions/checkout@v4, awalsh128/cache-apt-pkgs-action@latest,
dtolnay/rust-toolchain@master, swatinem/rust-cache@v2, actions/cache@v4) to the
corresponding full 40+ character commit SHA for the specific release you want to
lock; ensure you replace every occurrence in the workflow and verify the SHA
references point to the intended commit on the action repositories.
- Around line 72-73: Replace direct GitHub expression interpolation inside shell
commands with environment variables and add validation: move uses of `${{
github.event.inputs.version }}`, `${{ github.event.inputs.storage }}`, `${{
github.event.inputs.duration }}`, and `${{ github.event.inputs.workers }}` into
the workflow's env: block (e.g., set VERSION, STORAGE, DURATION_SECONDS,
WORKERS), then in the run steps reference those env variables
(V="${VERSION:-1.2.2}" S="${STORAGE:-rocksdb}"), and add numeric validation for
DURATION_SECONDS and WORKERS (e.g., test with regex like '^[0-9]+$' and exit
with an error or set a safe default if invalid) before exporting/using them in
the E2E script to prevent shell injection and enforce numeric formats.
In `@tests/docker_env.sh`:
- Around line 206-212: The MinIO alias created by the separate docker run calls
won't persist because each minio/mc container is ephemeral; update the script so
the alias and bucket commands run in the same container context or share a
persistent mc config directory: either run a single docker run invoking a shell
that chains the commands (e.g. run minio/mc with sh -c "mc alias set ... && mc
mb ... && mc mb ...") using the existing variables MINIO_ALIAS, MINIO_ENDPOINT,
MINIO_ROOT_USER, MINIO_ROOT_PASSWORD, MINIO_BUCKET and MINIO_LOG_BUCKET, or
mount a host volume (or set MC_CONFIG_DIR/--config-dir) into the container so
the config persists across separate docker run calls.
In `@tests/docker_ha/Dockerfile.stress`:
- Around line 1-19: The Dockerfile runs the stress container as root; create and
switch to an unprivileged user to improve isolation by adding a non-root user
and group (e.g., "stressuser"), chowning the WORKDIR (/workspace/eloq_waiter) to
that user, and adding a USER instruction before CMD so the container runs as
that user; update any RUN steps that require root privileges to occur before
switching user, and ensure pip install and package installs remain earlier while
file ownership is adjusted for the new user.
In `@tests/docker_ha/Dockerfile.stress_go`:
- Around line 1-16: The Dockerfile runs as root by default (FROM
golang:1.24-bookworm) and should create and switch to a non-root user: add a
system user/group (e.g., adduser or groupadd/useradd with a fixed UID/GID),
create a home directory, chown the workspace path (WORKDIR
/workspace/eloq_waiter) to that user, and set USER to that non-root account
before the final CMD ["sleep","infinity"] so the container executes as non-root.
In `@tests/docker_ha/Dockerfile.stress_ts`:
- Around line 1-16: The Dockerfile currently runs the container as root; create
a non-root user and group (e.g., "stressuser") during the image build, chown the
WORKDIR (/workspace/eloq_waiter) to that user, and switch to it with a USER
instruction before the CMD so the container (CMD ["sleep","infinity"]) executes
as the non-root user; add the user creation and chown steps in the RUN block
that installs packages and append USER stressuser after WORKDIR.
In `@tests/e2e/cmd_stress_go/main.go`:
- Line 528: The flag-defined variable keyCount (set via flag.IntVar in main) can
be zero and will cause a modulo-by-zero panic where keyCount is used (around the
key selection/modulo logic); after flag.Parse() add a guard that validates
keyCount > 0 and handle invalid values (log an error and exit with non-zero
status or return) to prevent the panic and ensure downstream code using keyCount
% n cannot divide by zero.
- Around line 635-644: The preload loop currently ignores errors from Redis
calls (masterClient.HSet, LPush, RPush, SAdd, ZAdd, etc.), so modify each call
to capture its returned error and fail fast on any non-nil error: check the
error value immediately after each call inside the loop and abort the test setup
(e.g., log.Fatalf / panic / os.Exit(1) or propagate the error) so preload
failures cannot be silently ignored; do this consistently for all calls in the
loop including the ZAdd inside the inner for and any other masterClient.* calls.
- Around line 693-694: The current testCtx, cancel := context.WithTimeout(ctx,
duration) cancels immediately when duration <= 0; change the code to choose
between context.WithTimeout and context.WithCancel: if duration > 0 use testCtx,
cancel := context.WithTimeout(ctx, duration) else use testCtx, cancel :=
context.WithCancel(ctx) so a non-positive duration does not abort the stress
run; keep defer cancel() after this selection to ensure cleanup.
In `@tests/e2e/cmd_stress_py/main.py`:
- Around line 32-33: Add a guard after parsing arguments to validate that
args.key_count is a positive integer and fail fast with a clear error if it's <=
0; specifically, after parser.add_argument(...) and parser.parse_args(), check
args.key_count > 0 and call sys.exit("Invalid --key-count: must be a positive
integer") (or raise SystemExit) before any worker-starting code so the
modulo-by-zero in the worker loop (the code using args.key_count) cannot run.
In `@tests/e2e/cmd_stress_test.sh`:
- Around line 105-107: The dependency install command "compose exec -T stress-go
bash -c 'cd tests/e2e/cmd_stress_go && go mod download' 2>&1 || true" is masking
failures; remove the trailing "|| true" so the script fails fast on dependency
errors (same change for the similar command at the other occurrence), and if you
want clearer diagnostics wrap the command to capture and echo stderr/stdout
before exiting non‑zero so the CI log shows the root cause instead of later
opaque failures.
In `@tests/e2e/cmd_stress_ts/main.ts`:
- Around line 271-274: masterAddr may be empty after discovery but the code
immediately calls mkClient(masterAddr), hiding the real failure; add an explicit
validation after discovery: if masterAddr is falsy (empty string/null/undefined)
log or throw a clear error and abort (e.g., process.exit(1) or throw new Error)
before calling mkClient, and only construct replica using replicaAddr and
mkClient if masterAddr is valid (keep the existing replicaAddr !== masterAddr
check).
- Around line 19-21: The CLI options "cmd-timeout" and "progress-interval" are
being converted to milliseconds twice (defaults like "5000" +
parseInt(...)*1000) causing 5,000,000ms effective values; fix by ensuring only
one conversion to ms: either change the defaults to represent seconds (e.g.,
"5") or, preferably, remove the extra *1000 in the parsing logic so that
parseInt(argv["cmd-timeout"]) and parseInt(argv["progress-interval"]) yield the
intended millisecond values; look for the parseInt(...)*1000 expressions that
consume "cmd-timeout" and "progress-interval" and adjust them so each option is
multiplied by 1000 exactly once (or not at all if defaults are ms).
In `@tests/README.md`:
- Line 10: Replace the machine-specific absolute path used in the Quick Start
example (the line containing "cd
/home/starrysky/workspace/eloqdata-kernel/eloq_waiter") with a repo-relative or
generic path (e.g., "cd eloq_waiter" or "cd <repo-root>/eloq_waiter") so
instructions are portable for other contributors; update the README.md Quick
Start section to use a relative path or placeholder and optionally add a note
about assuming the repo root.
- Line 44: Update the markdown fence that shows the directory-layout so it
includes a language identifier: find the fenced code block that begins with ```
and contains the line "tests/" (the directory-layout fence in tests/README.md)
and change the opening fence to ```text so the block becomes ```text ... ```,
which will satisfy markdownlint MD040.
---
Nitpick comments:
In `@scripts/test-before-push.sh`:
- Around line 20-21: The pre-push script currently runs the full stress e2e
suite by calling tests/e2e/cmd_stress_test.sh, which is too heavy for frequent
pushes; change scripts/test-before-push.sh to invoke that script with lighter
defaults (for example exporting a small STRESS_ITERATIONS or STRESS_DURATION env
var, or passing a --quick/--short flag) so the hook runs a quick smoke/stress
subset instead of the full default; update the invocation in
scripts/test-before-push.sh (the bash call to tests/e2e/cmd_stress_test.sh) to
pass the chosen short-mode parameter and ensure cmd_stress_test.sh honors that
flag/env var.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f9c5646-ddb2-44fb-96aa-12ddbc791f4f
⛔ Files ignored due to path filters (2)
tests/e2e/cmd_stress_go/go.sumis excluded by!**/*.sumtests/e2e/cmd_stress_ts/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
.github/workflows/e2e.yml.gitignorescripts/test-before-push.shtests/README.mdtests/docker_env.shtests/docker_ha/Dockerfile.stresstests/docker_ha/Dockerfile.stress_gotests/docker_ha/Dockerfile.stress_tstests/docker_ha/docker-compose.ci.ymltests/docker_ha/topology.yamltests/e2e/cmd_stress_go/go.modtests/e2e/cmd_stress_go/main.gotests/e2e/cmd_stress_py/main.pytests/e2e/cmd_stress_py/requirements.txttests/e2e/cmd_stress_test.shtests/e2e/cmd_stress_ts/main.tstests/e2e/cmd_stress_ts/package.jsontests/e2e/cmd_stress_ts/tsconfig.jsontests/e2e/stress.pytests/e2e/stress_test.shtests/e2e/test.shtests/e2e/topology.yaml
💤 Files with no reviewable changes (4)
- tests/docker_ha/topology.yaml
- tests/e2e/stress_test.sh
- tests/e2e/stress.py
- tests/e2e/test.sh
Comment on lines
+40
to
+41
| - name: Checkout | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and display relevant sections of the workflow
if [ -f .github/workflows/e2e.yml ]; then
echo "=== File: .github/workflows/e2e.yml (lines 1-120) ==="
nl -ba .github/workflows/e2e.yml | sed -n '1,120p'
echo "=== File: .github/workflows/e2e.yml (search for checkout + persist-credentials) ==="
rg -n "actions/checkout@|persist-credentials|permissions:" .github/workflows/e2e.yml || true
echo "=== File: .github/workflows/e2e.yml (remaining lines) ==="
nl -ba .github/workflows/e2e.yml | sed -n '121,240p' || true
else
echo "Missing: .github/workflows/e2e.yml"
exit 1
fiRepository: monographdb/eloq_waiter
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/e2e.yml"
if [ ! -f "$FILE" ]; then
echo "Missing: $FILE"
exit 1
fi
echo "=== File: $FILE (lines 1-200) ==="
awk 'NR>=1 && NR<=200 {printf "%5d: %s\n", NR, $0}' "$FILE"
echo "=== Search: actions/checkout@, persist-credentials, permissions ==="
rg -n "actions/checkout@|persist-credentials|permissions:" "$FILE" || true
echo "=== Show checkout step block context ==="
# Print lines around any checkout usage
rg -n "actions/checkout@" "$FILE" -n --context 10 || trueRepository: monographdb/eloq_waiter
Length of output: 5638
Disable checkout credential persistence for this workflow job.
The workflow already has permissions: contents: read, so actions/checkout@v4 doesn’t need to persist git credentials locally.
Suggested change
- name: Checkout
uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Checkout | |
| uses: actions/checkout@v4 | |
| - name: Checkout | |
| uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 40-41: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 41-41: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e.yml around lines 40 - 41, The checkout step currently
uses actions/checkout@v4 and should disable persisting git credentials; update
the Checkout step (the actions/checkout@v4 usage) to set persist-credentials:
false so credentials are not stored locally (since the workflow has permissions:
contents: read).
Comment on lines
+44
to
+55
| uses: awalsh128/cache-apt-pkgs-action@latest | ||
| with: | ||
| packages: protobuf-compiler pkg-config libssl-dev | ||
| version: e2e-ubuntu24-v1 | ||
|
|
||
| - name: Set up Rust | ||
| uses: dtolnay/rust-toolchain@master | ||
| with: | ||
| toolchain: nightly-2026-04-21 | ||
|
|
||
| - name: Cache Rust build artifacts | ||
| uses: swatinem/rust-cache@v2 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Detect unpinned 'uses:' refs in workflow files (expects 40-char SHA pins).
rg -nP '^\s*uses:\s*[^@\s]+@(?!(?:[0-9a-f]{40})\b)' .github/workflowsRepository: monographdb/eloq_waiter
Length of output: 2144
Pin GitHub Actions in .github/workflows/e2e.yml to immutable commit SHAs (no tags/latest/branches).
Mutable uses: refs in this workflow include:
actions/checkout@v4awalsh128/cache-apt-pkgs-action@latestdtolnay/rust-toolchain@masterswatinem/rust-cache@v2actions/cache@v4(also applies at line 81)
Pin each uses: to a full 40+ char commit SHA.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 44-44: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 50-50: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 55-55: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e.yml around lines 44 - 55, Replace mutable GitHub
Actions refs with immutable commit SHAs: update each `uses:` entry that
currently references tags/branches (e.g., actions/checkout@v4,
awalsh128/cache-apt-pkgs-action@latest, dtolnay/rust-toolchain@master,
swatinem/rust-cache@v2, actions/cache@v4) to the corresponding full 40+
character commit SHA for the specific release you want to lock; ensure you
replace every occurrence in the workflow and verify the SHA references point to
the intended commit on the action repositories.
Comment on lines
+72
to
+73
| V="${VERSION:-${{ github.event.inputs.version || '1.2.2' }}}" | ||
| S="${STORAGE:-${{ github.event.inputs.storage || 'rocksdb' }}}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant sections around the referenced line numbers
sed -n '1,160p' .github/workflows/e2e.yml | nl -ba | sed -n '55,135p'Repository: monographdb/eloq_waiter
Length of output: 110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
awk 'NR>=50 && NR<=140 {printf "%4d\t%s\n", NR, $0}' .github/workflows/e2e.ymlRepository: monographdb/eloq_waiter
Length of output: 3381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
awk 'NR>=1 && NR<=80 {printf "%4d\t%s\n", NR, $0}' .github/workflows/e2e.ymlRepository: monographdb/eloq_waiter
Length of output: 2614
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "github\.event\.inputs\.(version|storage|duration|workers)" .github/workflows/e2e.ymlRepository: monographdb/eloq_waiter
Length of output: 377
Avoid ${{ github.event.inputs.* }} interpolation directly inside run: shell commands; pass via env: and validate.
- In Resolve inputs (
.github/workflows/e2e.ymllines 72-73), workflow_dispatch inputs are unconstrained strings and are injected into shell parsing inside double quotes/parameter expansion, allowing input-crafted shell expansions/quote breaking. - In Run E2E test (lines 109-110),
${{ github.event.inputs.duration }}/${{ github.event.inputs.workers }}are similarly interpolated intoexportstatements with no format checks.
Suggested hardening
- - name: Resolve inputs
+ - name: Resolve inputs
id: vars
+ env:
+ INPUT_VERSION: ${{ github.event.inputs.version || '1.2.2' }}
+ INPUT_STORAGE: ${{ github.event.inputs.storage || 'rocksdb' }}
run: |
- V="${VERSION:-${{ github.event.inputs.version || '1.2.2' }}}"
- S="${STORAGE:-${{ github.event.inputs.storage || 'rocksdb' }}}"
+ V="${VERSION:-$INPUT_VERSION}"
+ S="${STORAGE:-$INPUT_STORAGE}"
+ [[ "$V" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]] || { echo "Invalid version: $V"; exit 1; }
+ [[ "$S" =~ ^(rocksdb|rocks_s3)$ ]] || { echo "Invalid storage: $S"; exit 1; }
echo "version=${V}" >> "$GITHUB_OUTPUT"
echo "storage=${S}" >> "$GITHUB_OUTPUT"Add equivalent env: + numeric validation for DURATION_SECONDS and WORKERS (lines 109-110).
🧰 Tools
🪛 zizmor (1.25.2)
[error] 72-72: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 73-73: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e.yml around lines 72 - 73, Replace direct GitHub
expression interpolation inside shell commands with environment variables and
add validation: move uses of `${{ github.event.inputs.version }}`, `${{
github.event.inputs.storage }}`, `${{ github.event.inputs.duration }}`, and `${{
github.event.inputs.workers }}` into the workflow's env: block (e.g., set
VERSION, STORAGE, DURATION_SECONDS, WORKERS), then in the run steps reference
those env variables (V="${VERSION:-1.2.2}" S="${STORAGE:-rocksdb}"), and add
numeric validation for DURATION_SECONDS and WORKERS (e.g., test with regex like
'^[0-9]+$' and exit with an error or set a safe default if invalid) before
exporting/using them in the E2E script to prevent shell injection and enforce
numeric formats.
Comment on lines
+95
to
+98
| curl -fsSL --retry 3 -o "$HOME/.eloqctl/download/${TX}" "${BASE}/${TX}" | ||
|
|
||
| mkdir -p "$HOME/.eloqctl/download/$(dirname "${LOG}")" | ||
| curl -fsSL --retry 3 -o "$HOME/.eloqctl/download/${LOG}" "${BASE}/${LOG}" |
There was a problem hiding this comment.
Verify downloaded artifacts with checksums before caching/using them.
curl download alone does not protect against tampering/corruption of release tarballs.
Suggested integrity check pattern
curl -fsSL --retry 3 -o "$HOME/.eloqctl/download/${TX}" "${BASE}/${TX}"
+ curl -fsSL --retry 3 -o "$HOME/.eloqctl/download/${TX}.sha256" "${BASE}/${TX}.sha256"
+ (cd "$HOME/.eloqctl/download" && sha256sum -c "${TX}.sha256")
mkdir -p "$HOME/.eloqctl/download/$(dirname "${LOG}")"
curl -fsSL --retry 3 -o "$HOME/.eloqctl/download/${LOG}" "${BASE}/${LOG}"
+ curl -fsSL --retry 3 -o "$HOME/.eloqctl/download/${LOG}.sha256" "${BASE}/${LOG}.sha256"
+ (cd "$HOME/.eloqctl/download" && sha256sum -c "${LOG}.sha256")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| curl -fsSL --retry 3 -o "$HOME/.eloqctl/download/${TX}" "${BASE}/${TX}" | |
| mkdir -p "$HOME/.eloqctl/download/$(dirname "${LOG}")" | |
| curl -fsSL --retry 3 -o "$HOME/.eloqctl/download/${LOG}" "${BASE}/${LOG}" | |
| curl -fsSL --retry 3 -o "$HOME/.eloqctl/download/${TX}" "${BASE}/${TX}" | |
| curl -fsSL --retry 3 -o "$HOME/.eloqctl/download/${TX}.sha256" "${BASE}/${TX}.sha256" | |
| (cd "$HOME/.eloqctl/download" && sha256sum -c "${TX}.sha256") | |
| mkdir -p "$HOME/.eloqctl/download/$(dirname "${LOG}")" | |
| curl -fsSL --retry 3 -o "$HOME/.eloqctl/download/${LOG}" "${BASE}/${LOG}" | |
| curl -fsSL --retry 3 -o "$HOME/.eloqctl/download/${LOG}.sha256" "${BASE}/${LOG}.sha256" | |
| (cd "$HOME/.eloqctl/download" && sha256sum -c "${LOG}.sha256") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e.yml around lines 95 - 98, After downloading artifacts
referenced by variables TX and LOG from BASE in the e2e workflow, verify their
integrity before caching/using them by fetching the corresponding checksum file
or known-good hash and running a checksum verification (e.g., sha256) against
the downloaded files; update the .github/workflows/e2e.yml steps that perform
curl -o "$HOME/.eloqctl/download/${TX}" and curl -o
"$HOME/.eloqctl/download/${LOG}" to also curl the checksum file (or embed
expected hashes), then run a verification command (sha256sum -c or shasum -a 256
--check) for ${TX} and ${LOG} and fail the job if verification fails so tampered
or corrupted downloads are rejected.
Comment on lines
+271
to
+274
| console.log(P(`master=${masterAddr} replica=${replicaAddr}`)); | ||
|
|
||
| const master = mkClient(masterAddr); | ||
| const replica = replicaAddr && replicaAddr !== masterAddr ? mkClient(replicaAddr) : null; |
There was a problem hiding this comment.
Validate discovered master address before creating standalone client.
If cluster discovery fails to find a master, Line 273 still builds a client from an empty address, which obscures the root cause. Add an explicit check and abort with a clear error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/cmd_stress_ts/main.ts` around lines 271 - 274, masterAddr may be
empty after discovery but the code immediately calls mkClient(masterAddr),
hiding the real failure; add an explicit validation after discovery: if
masterAddr is falsy (empty string/null/undefined) log or throw a clear error and
abort (e.g., process.exit(1) or throw new Error) before calling mkClient, and
only construct replica using replicaAddr and mkClient if masterAddr is valid
(keep the existing replicaAddr !== masterAddr check).
| └── docker_env.sh ← shared shell helpers | ||
| ``` | ||
| ```sh | ||
| cd /home/starrysky/workspace/eloqdata-kernel/eloq_waiter |
There was a problem hiding this comment.
Avoid machine-specific absolute paths in Quick Start.
/home/starrysky/... is not portable; prefer repo-relative instructions.
Suggested change
-cd /home/starrysky/workspace/eloqdata-kernel/eloq_waiter
+cd <repo-root>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cd /home/starrysky/workspace/eloqdata-kernel/eloq_waiter | |
| cd <repo-root> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/README.md` at line 10, Replace the machine-specific absolute path used
in the Quick Start example (the line containing "cd
/home/starrysky/workspace/eloqdata-kernel/eloq_waiter") with a repo-relative or
generic path (e.g., "cd eloq_waiter" or "cd <repo-root>/eloq_waiter") so
instructions are portable for other contributors; update the README.md Quick
Start section to use a relative path or placeholder and optionally add a note
about assuming the repo root.
|
|
||
| # Install git pre-push hook (auto-runs test-before-push.sh) | ||
| scripts/install-git-hooks.sh | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the directory-layout fence.
This clears markdownlint MD040 and keeps docs lint-clean.
Suggested change
-```
+```text
tests/
...
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/README.md` at line 44, Update the markdown fence that shows the
directory-layout so it includes a language identifier: find the fenced code
block that begins with ``` and contains the line "tests/" (the directory-layout
fence in tests/README.md) and change the opening fence to ```text so the block
becomes ```text ... ```, which will satisfy markdownlint MD040.
There was a problem hiding this comment.
Pull request overview
Adds a new unified E2E “command stress” suite that deploys an EloqKV cluster in Docker and runs Redis-command coverage + stress workloads across Python, Go, and TypeScript SDKs (including dual standalone/cluster-client modes), plus a GitHub Actions workflow to run it on PRs/pushes.
Changes:
- Introduces
tests/e2e/cmd_stress_test.shand per-SDK stress clients (Python/Go/TS) for multi-command coverage + stress runs. - Updates E2E topology to parameterize version and enable TLS + MinIO/log-service-backed storage.
- Removes legacy E2E/stress scripts and wires the new suite into pre-push + CI.
Reviewed changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/README.md | Updates E2E documentation to the new cmd stress suite and configuration knobs. |
| tests/e2e/topology.yaml | Switches E2E topology to TLS + MinIO/log_service + version parameterization. |
| tests/e2e/test.sh | Removes legacy sequential E2E CLI scenario runner. |
| tests/e2e/stress.py | Removes legacy redis-py connection stress script. |
| tests/e2e/stress_test.sh | Removes legacy stress test wrapper script. |
| tests/e2e/cmd_stress_test.sh | New single entry point: launch → Python → Go → TS → remove. |
| tests/e2e/cmd_stress_py/requirements.txt | Adds redis-py dependency pin/range for Python stress client. |
| tests/e2e/cmd_stress_py/main.py | New Python full-command coverage + stress implementation. |
| tests/e2e/cmd_stress_go/go.mod | New Go module for the Go stress client. |
| tests/e2e/cmd_stress_go/go.sum | Go dependency lockfile for the Go stress client. |
| tests/e2e/cmd_stress_go/main.go | New Go full-command coverage + stress implementation. |
| tests/e2e/cmd_stress_ts/package.json | New TS stress client package definition (ioredis + tsx). |
| tests/e2e/cmd_stress_ts/package-lock.json | Locks Node dependencies for TS stress client. |
| tests/e2e/cmd_stress_ts/tsconfig.json | TS compiler configuration for the stress client. |
| tests/e2e/cmd_stress_ts/main.ts | New TypeScript full-command coverage + stress implementation. |
| tests/docker_ha/topology.yaml | Removes redundant legacy docker_ha topology template. |
| tests/docker_ha/Dockerfile.stress | Adds Python stress container image. |
| tests/docker_ha/Dockerfile.stress_go | Adds Go stress container image. |
| tests/docker_ha/Dockerfile.stress_ts | Adds Node/TS stress container image. |
| tests/docker_ha/docker-compose.ci.yml | Adds CI override intended to strip security options for runner compatibility. |
| tests/docker_env.sh | Expands docker helpers for control-node execution, MinIO readiness/bucket setup, and cache seeding. |
| scripts/test-before-push.sh | Switches pre-push E2E invocation to the new cmd stress suite. |
| .gitignore | Ignores node_modules/pycache/old stress binary artifacts under tests/. |
| .github/workflows/e2e.yml | Adds CI workflow to build eloqctl, cache/download binaries, and run the new E2E suite. |
Files not reviewed (1)
- tests/e2e/cmd_stress_ts/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
tests/e2e/cmd_stress_ts/main.ts:36
- TLS_INSECURE parsing treats any value other than the literal string "false" as true. Since cmd_stress_test.sh passes 0/1,
TLS_ENABLED=0still enables TLS in the TS client. Consider treating "0" as false too (and possibly supporting common variants).
const TLS_INSECURE = args["tls-insecure"] !== "false";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| └── docker_env.sh ← shared shell helpers | ||
| ``` | ||
| ```sh | ||
| cd /home/starrysky/workspace/eloqdata-kernel/eloq_waiter |
Comment on lines
+28
to
+30
| 172.28.10.14: | ||
| cpu: 4 | ||
| memory: 1024 |
Comment on lines
+37
to
+42
| rocksdb: !MINIO | ||
| aws_access_key_id: "minioadmin" | ||
| aws_secret_key: "minioadmin" | ||
| bucket_name: "eloqservice" | ||
| bucket_prefix: "store" | ||
| endpoint: "http://172.28.10.20:9000" |
Comment on lines
+10
to
+14
| ELOQCTL_CONTROL_SSH_KEY="${ELOQCTL_CONTROL_SSH_KEY:-/home/eloq/.ssh/id_ed25519}" | ||
| CONTROL_NODE_SERVICE="${CONTROL_NODE_SERVICE:-eloq-node-4}" | ||
| CONTROL_REPO_ROOT="${CONTROL_REPO_ROOT:-/workspace/eloq_waiter}" | ||
| CONTROL_ELOQCTL_HOME="${CONTROL_ELOQCTL_HOME:-/home/eloq/.eloqctl}" | ||
| CONTROL_ELOQCTL="${CONTROL_ELOQCTL:-${CONTROL_ELOQCTL_HOME}/bin/cluster_mgr}" |
Comment on lines
+192
to
+203
| echo "[docker] Wait for MinIO" | ||
| for _ in $(seq 1 60); do | ||
| if curl -fsS "${MINIO_ENDPOINT}/minio/health/live" >/dev/null 2>&1; then | ||
| break | ||
| fi | ||
| sleep 1 | ||
| done | ||
| curl -fsS "${MINIO_ENDPOINT}/minio/health/live" >/dev/null 2>&1 || { | ||
| echo "FAIL: MinIO is not ready at ${MINIO_ENDPOINT}" | ||
| compose ps || true | ||
| compose logs --no-color --tail=80 minio || true | ||
| exit 1 |
| flag.DurationVar(&duration, "duration", 60*time.Second, "test duration") | ||
| flag.DurationVar(&progressInterval, "progress-interval", 5*time.Second, "progress print interval") | ||
| flag.IntVar(&keyCount, "key-count", 256, "key space size") | ||
| flag.BoolVar(&insecureTLS, "tls-insecure", true, "skip TLS verification") |
Comment on lines
+231
to
+233
| ("DUMP", lambda c, i: c.dump(_k(i))), | ||
| ("RESTORE", lambda c, i: (payload := c.dump(_k(i)), | ||
| c.restore(_kt(i, "rs"), 0, payload, replace=True))), |
Comment on lines
+15
to
+35
| eloq-node-4: | ||
| cap_add: [] | ||
| security_opt: [] | ||
| stress-client: | ||
| cap_add: [] | ||
| security_opt: [] | ||
| stress-client-2: | ||
| cap_add: [] | ||
| security_opt: [] | ||
| stress-client-3: | ||
| cap_add: [] | ||
| security_opt: [] | ||
| stress-client-4: | ||
| cap_add: [] | ||
| security_opt: [] | ||
| stress-go: | ||
| cap_add: [] | ||
| security_opt: [] | ||
| stress-ts: | ||
| cap_add: [] | ||
| security_opt: [] |
Comment on lines
+96
to
+109
| prepare_control_node() { | ||
| compose exec -T "${CONTROL_NODE_SERVICE}" bash -lc " | ||
| set -eu | ||
| install -d -m 700 -o eloq -g eloq /home/eloq/.ssh | ||
| install -m 600 -o eloq -g eloq '${CONTROL_REPO_ROOT}/tests/docker_ha/id_ed25519' '${ELOQCTL_CONTROL_SSH_KEY}' | ||
| install -d -m 755 -o eloq -g eloq '${CONTROL_ELOQCTL_HOME}' | ||
| install -d -m 755 -o eloq -g eloq '${CONTROL_ELOQCTL_HOME}/bin' | ||
| cp '${CONTROL_REPO_ROOT}/target/debug/cluster_mgr' '${CONTROL_ELOQCTL}' | ||
| chmod 755 '${CONTROL_ELOQCTL}' | ||
| rm -f '${CONTROL_ELOQCTL_HOME}/config' | ||
| ln -s '${CONTROL_REPO_ROOT}/src/cluster_mgr/config' '${CONTROL_ELOQCTL_HOME}/config' | ||
| chown -h eloq:eloq '${CONTROL_ELOQCTL_HOME}/config' | ||
| chown -R eloq:eloq '${CONTROL_ELOQCTL_HOME}' | ||
| " |
| await cluster.ping(); | ||
| console.log(P("cluster ping OK")); | ||
|
|
||
| const nodesInfo: string = (await cluster.call("cluster", ["nodes"])) as string; |
There was a problem hiding this comment.
♻️ Duplicate comments (4)
.github/workflows/test.yml (4)
44-56:⚠️ Potential issue | 🟠 MajorGitHub Actions should be pinned to immutable commit SHAs.
Multiple actions use mutable tag references:
- Line 44:
awalsh128/cache-apt-pkgs-action@latest- Line 50:
dtolnay/rust-toolchain@master- Line 56:
swatinem/rust-cache@v2- Line 41:
actions/checkout@v4🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 44 - 56, Replace mutable action refs with immutable commit SHAs: update the usages of "awalsh128/cache-apt-pkgs-action@latest", "dtolnay/rust-toolchain@master", "swatinem/rust-cache@v2", and "actions/checkout@v4" to use the corresponding specific commit SHAs (the full 40-char git commit) instead of tags; locate these exact strings in the workflow and replace each tag with the latest vetted commit SHA for that action repository, then run the workflow or a dry run to verify the pinned SHAs work correctly.
82-88:⚠️ Potential issue | 🔴 CriticalTemplate injection risk: workflow_dispatch inputs interpolated directly into shell commands.
Lines 85-86 inject
github.event.inputs.versionandgithub.event.inputs.storagewithout validation, allowing shell metacharacter injection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 82 - 88, The "Resolve inputs" step (id: vars) unsafely interpolates github.event.inputs.version and github.event.inputs.storage into shell variables V and S, allowing shell metacharacter/template injection; fix by not directly expanding raw inputs in a shell command: validate and sanitize inputs before using them (use a strict semver regex check for version in the step handling V/variable and reject or normalize non-matching values), implement a whitelist for storage values (only allow known tokens like "rocksdb", "sqlite", etc.) and reject/normalize everything else, and preferably set outputs using GitHub Actions expressions or safe escaping instead of raw shell interpolation so V and S are assigned only validated/sanitized values before echoing to GITHUB_OUTPUT.
40-41:⚠️ Potential issue | 🟠 MajorMissing persist-credentials: false on checkout step.
The checkout action should disable credential persistence since the workflow only needs read access.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 40 - 41, The Checkout step using actions/checkout@v4 should disable credential persistence; update the checkout step (the step named "Checkout" that uses actions/checkout@v4) to include persist-credentials: false so credentials are not persisted to the runner when only read access is needed.
91-103:⚠️ Potential issue | 🔴 CriticalTemplate injection risk: duration and workers inputs lack validation.
Lines 99-100 inject
github.event.inputs.durationandgithub.event.inputs.workersdirectly into export statements without numeric validation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 91 - 103, The workflow injects github.event.inputs.duration and github.event.inputs.workers directly into DURATION_SECONDS and WORKERS creating a template-injection risk; validate and sanitize those inputs before exporting by capturing the raw inputs into temporary shell variables, check they match a numeric regex (e.g., only digits) and fall back to the safe defaults ('60' for duration and '16' for workers) if validation fails, then export the validated values as DURATION_SECONDS and WORKERS and continue to call bash tests/e2e/cmd_stress_test.sh; ensure the validation logic is placed before the exports and references the same variable names (DURATION_SECONDS, WORKERS) so the downstream script receives only validated numeric values.
🧹 Nitpick comments (2)
tests/e2e/cmd_stress_py/main.py (2)
386-400: 💤 Low valueQPS calculation shows average, not interval rate.
The variable
lastis never updated inside the loop, and stats are cumulative. This computes average QPS since start rather than the instantaneous rate per interval. If this is intentional, renamelasttostart_timefor clarity; otherwise, track deltas:Option: Interval-based QPS
try: last = time.time() + prev_ops = 0 while not _should_stop(): time.sleep(args.progress_interval) now = time.time() elapsed = now - last + last = now s_ok = sum(v["ok"] for v in standalone_stats.values()) s_fail = sum(v["fail"] for v in standalone_stats.values()) c_ok = sum(v["ok"] for v in cluster_stats.values()) c_fail = sum(v["fail"] for v in cluster_stats.values()) total_ok = s_ok + c_ok total_fail = s_fail + c_fail - qps = (total_ok + total_fail) / max(elapsed, 0.001) + current_ops = total_ok + total_fail + qps = (current_ops - prev_ops) / max(elapsed, 0.001) + prev_ops = current_ops🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/cmd_stress_py/main.py` around lines 386 - 400, The QPS calculation currently uses a never-updated variable last and cumulative stats (standalone_stats, cluster_stats), producing an average since start; either convert to interval-rate by computing elapsed = now - last, using the delta counts (compute s_ok_delta, s_fail_delta, c_ok_delta, c_fail_delta from previous snapshot) to derive qps, then set last = now and update the previous-snapshot counters, or if the average since start is intended, rename last to start_time and remove/reset any code that implies interval sampling (keep using cumulative totals and compute qps = (total_ok+total_fail)/max(now-start_time,0.001)). Choose one approach and update the loop variables accordingly (last/start_time, previous counters, qps calculation, and any printed labels).
274-280: 💤 Low valueString-based exception matching is fragile.
The
"Missing key"substring check will silently break if redis-py changes its error message wording. Consider catching a more specific exception type or using a constant/pattern.For stress test code this is acceptable, but be aware it may need adjustment if the library version changes.
Alternative: Check exception type
except Exception as e: # ECHO can't be routed by cluster client (no key) — skip, not fail - if cmd_name == "ECHO" and "Missing key" in str(e): + from redis.exceptions import RedisClusterException + if cmd_name == "ECHO" and isinstance(e, RedisClusterException): continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/cmd_stress_py/main.py` around lines 274 - 280, In the except block in tests/e2e/cmd_stress_py/main.py (the except Exception as e handling around cmd execution), replace the fragile string match ("Missing key" in str(e)) with a check against a specific Redis exception class or a defined pattern constant: import the appropriate exception type from redis (e.g., redis.exceptions.ResponseError or the cluster-specific exception used by your client) and use isinstance(e, ThatException) to skip ECHO routing errors, or define a single regex constant and match against that; update the conditional that currently references cmd_name == "ECHO" and the string check to use the new exception type or constant instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/workflows/test.yml:
- Around line 44-56: Replace mutable action refs with immutable commit SHAs:
update the usages of "awalsh128/cache-apt-pkgs-action@latest",
"dtolnay/rust-toolchain@master", "swatinem/rust-cache@v2", and
"actions/checkout@v4" to use the corresponding specific commit SHAs (the full
40-char git commit) instead of tags; locate these exact strings in the workflow
and replace each tag with the latest vetted commit SHA for that action
repository, then run the workflow or a dry run to verify the pinned SHAs work
correctly.
- Around line 82-88: The "Resolve inputs" step (id: vars) unsafely interpolates
github.event.inputs.version and github.event.inputs.storage into shell variables
V and S, allowing shell metacharacter/template injection; fix by not directly
expanding raw inputs in a shell command: validate and sanitize inputs before
using them (use a strict semver regex check for version in the step handling
V/variable and reject or normalize non-matching values), implement a whitelist
for storage values (only allow known tokens like "rocksdb", "sqlite", etc.) and
reject/normalize everything else, and preferably set outputs using GitHub
Actions expressions or safe escaping instead of raw shell interpolation so V and
S are assigned only validated/sanitized values before echoing to GITHUB_OUTPUT.
- Around line 40-41: The Checkout step using actions/checkout@v4 should disable
credential persistence; update the checkout step (the step named "Checkout" that
uses actions/checkout@v4) to include persist-credentials: false so credentials
are not persisted to the runner when only read access is needed.
- Around line 91-103: The workflow injects github.event.inputs.duration and
github.event.inputs.workers directly into DURATION_SECONDS and WORKERS creating
a template-injection risk; validate and sanitize those inputs before exporting
by capturing the raw inputs into temporary shell variables, check they match a
numeric regex (e.g., only digits) and fall back to the safe defaults ('60' for
duration and '16' for workers) if validation fails, then export the validated
values as DURATION_SECONDS and WORKERS and continue to call bash
tests/e2e/cmd_stress_test.sh; ensure the validation logic is placed before the
exports and references the same variable names (DURATION_SECONDS, WORKERS) so
the downstream script receives only validated numeric values.
---
Nitpick comments:
In `@tests/e2e/cmd_stress_py/main.py`:
- Around line 386-400: The QPS calculation currently uses a never-updated
variable last and cumulative stats (standalone_stats, cluster_stats), producing
an average since start; either convert to interval-rate by computing elapsed =
now - last, using the delta counts (compute s_ok_delta, s_fail_delta,
c_ok_delta, c_fail_delta from previous snapshot) to derive qps, then set last =
now and update the previous-snapshot counters, or if the average since start is
intended, rename last to start_time and remove/reset any code that implies
interval sampling (keep using cumulative totals and compute qps =
(total_ok+total_fail)/max(now-start_time,0.001)). Choose one approach and update
the loop variables accordingly (last/start_time, previous counters, qps
calculation, and any printed labels).
- Around line 274-280: In the except block in tests/e2e/cmd_stress_py/main.py
(the except Exception as e handling around cmd execution), replace the fragile
string match ("Missing key" in str(e)) with a check against a specific Redis
exception class or a defined pattern constant: import the appropriate exception
type from redis (e.g., redis.exceptions.ResponseError or the cluster-specific
exception used by your client) and use isinstance(e, ThatException) to skip ECHO
routing errors, or define a single regex constant and match against that; update
the conditional that currently references cmd_name == "ECHO" and the string
check to use the new exception type or constant instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2f5de86-e505-4382-a459-c3871da2898a
📒 Files selected for processing (6)
.github/workflows/quality.yml.github/workflows/test.ymltests/docker_ha/Dockerfiletests/docker_ha/docker-compose.yamltests/e2e/cmd_stress_py/main.pytests/e2e/topology.yaml
💤 Files with no reviewable changes (1)
- .github/workflows/quality.yml
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/cluster_mgr/src/cli/task/upload/eloq_upload_builder.rs (1)
81-154: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRemove dead code:
all_files_pathis built but never used.Lines 81-151 populate
all_files_pathwith log start scripts and EloqDss-*.ini file paths, but at line 153 the vector is assigned to_and discarded. This is confusing and wasteful. Either the code should useall_files_pathor the entire block should be removed.If this is intentional (relying on
list_files_by_hostto provide the same files), consider removing the dead code to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cluster_mgr/src/cli/task/upload/eloq_upload_builder.rs` around lines 81 - 154, The block populating the all_files_path Vec (built from config.gen_log_start_script(), DeploymentPackage hosts, and scanning create_upload_cluster_dir for EloqDss-*.ini) is dead because all_files_path is never used (it's assigned to _); remove this entire construction or replace its usage with the intended downstream call (e.g., use list_files_by_host or pass all_files_path into the uploader) so files are actually consumed; locate references to all_files_path, config.gen_log_start_script(), create_upload_cluster_dir, and any upload/listing code to either wire the vector into the upload flow or delete the unused code to avoid confusion.src/cluster_mgr/src/config/config_base.rs (1)
420-427:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential panic if
host_portlacks a colon.Line 424 accesses
parts[1]without verifying thatpartshas at least 2 elements. Whilevalidate_topology()should ensure properhost:portformat, a defensive approach would prevent a panic if validation is bypassed or the data is malformed.Consider using
split_oncefor safer parsing:🛡️ Proposed defensive fix
pub fn client_conn(&self) -> String { let bin = self.deployment.client_bin(); let host_port = self.deployment.tx_service.tx_host_ports.first().unwrap(); - let parts: Vec<&str> = host_port.split(':').collect(); - let port = parts[1].parse::<u16>().unwrap_or(6379); - let (host, port) = self.service_endpoint(parts[0], port); + let (host, port_str) = host_port.rsplit_once(':').unwrap_or((host_port, "6379")); + let port = port_str.parse::<u16>().unwrap_or(6379); + let (host, port) = self.service_endpoint(host, port); format!("{bin} -h {host} -p {port}") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cluster_mgr/src/config/config_base.rs` around lines 420 - 427, In client_conn(), avoid indexing parts[1] which can panic if host_port lacks a colon; instead parse host_port using split_once(':') (or check parts.len() >= 2) to safely extract host and port and fall back to a default port (e.g., 6379) or return an error; then call self.service_endpoint(host, parsed_port) and format the command as before. Update logic around deployment.tx_service.tx_host_ports.first().unwrap() and host_port handling in client_conn so malformed entries don't panic (use safe unwrap_or / matching and handle the None case defensively).tests/README.md (1)
92-92:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix CLI binary name in the health-check command.
Line 92 uses
~/.eloqctl/bin/eloqctl, but CI/setup in this PR installs and uses~/.eloqctl/bin/cluster_mgr.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/README.md` at line 92, The README health-check command uses the wrong CLI binary name; replace the occurrence of "~/.eloqctl/bin/eloqctl status test-e2e --wait 30" with the CI-installed binary name "~/.eloqctl/bin/cluster_mgr status test-e2e --wait 30" so the documented command matches the cluster_mgr installation used by the test/setup scripts.
♻️ Duplicate comments (11)
tests/README.md (2)
44-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifiers to fenced code blocks.
The fences at Lines 44 and 116 should include a language (e.g.,
text) to stay markdownlint-clean.Also applies to: 116-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/README.md` around lines 44 - 67, The README contains fenced code blocks with ASCII directory trees that lack a language identifier; update each fenced block showing the tests/ directory tree (the triple-backtick blocks containing the ASCII art like "tests/ ├── README.md ...") and the other similar fenced block later in the file to include a language label (e.g., change ``` to ```text) so markdownlint no longer flags missing language identifiers.
10-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a repo-relative path in Quick Start.
Line 10 is machine-specific and not portable for other contributors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/README.md` at line 10, Replace the machine-specific absolute path "cd /home/starrysky/workspace/eloqdata-kernel/eloq_waiter" in the Quick Start section with a repo-relative command (for example "cd eloq_waiter" or "cd ./eloq_waiter" or instructions to run from repository root) so the setup is portable for other contributors; update the README.md Quick Start snippet accordingly and keep the intent clear that the command should be run from the repository root or provide an alternative that works cross-platform.tests/e2e/cmd_stress_test.sh (1)
158-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t swallow dependency installation failures in stress containers.
Lines 158 and 179 still end with
|| true, so setup errors are hidden and later failures become noisy/indirect.Suggested fix
- compose exec -T stress-go bash -c \ - 'cd tests/e2e/cmd_stress_go && go mod download' 2>&1 || true + compose exec -T stress-go bash -c \ + 'cd tests/e2e/cmd_stress_go && go mod download' 2>&1 ... - compose exec -T stress-ts bash -c \ - 'cd tests/e2e/cmd_stress_ts && npm install --silent' 2>&1 || true + compose exec -T stress-ts bash -c \ + 'cd tests/e2e/cmd_stress_ts && npm install --silent' 2>&1Also applies to: 179-180
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/cmd_stress_test.sh` around lines 158 - 159, The setup commands in the stress container script are silently swallowing failures by appending "|| true" (e.g., the "'cd tests/e2e/cmd_stress_go && go mod download' 2>&1 || true" invocation and the similar command around lines 179-180); remove the "|| true" from these commands so dependency installation failures propagate as non-zero exit codes, or alternatively capture and explicitly log the error and exit non-zero after the command in the stress setup block (look for the compose exec -T stress-go bash -c sections and the go mod download invocations to apply the change).tests/e2e/cmd_stress_ts/main.ts (2)
32-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
cmd-timeout/progress-intervalunit conversion is still inconsistent.Defaults are
"5000"but Lines 32-33 multiply by 1000, producing 5,000,000ms when defaults are used.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/cmd_stress_ts/main.ts` around lines 32 - 33, The CMD_TIMEOUT and PROGRESS_INTERVAL conversions are multiplying defaults (which are strings like "5000") by 1000 and producing values in microseconds; fix by making units consistent: either change the default values to seconds (e.g. "5" instead of "5000") or stop multiplying by 1000 when parsing. Update the parsing lines that set CMD_TIMEOUT and PROGRESS_INTERVAL to use parseInt(args["cmd-timeout"]!, 10) and parseInt(args["progress-interval"]!, 10) without the extra *1000 if the defaults are already milliseconds, or keep the *1000 and change the default strings to seconds ("5") so both sides agree.
318-319:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate discovered
masterAddrbefore constructing standalone client.Line 318 builds a client even if discovery yields an empty master address, which obscures discovery failure and causes downstream connection noise.
Suggested fix
console.log(P(`master=${masterAddr} replica=${replicaAddr}`)); + if (!masterAddr) { + throw new Error("master discovery failed: empty master address from CLUSTER NODES"); + } const master = mkClient(masterAddr);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/cmd_stress_ts/main.ts` around lines 318 - 319, The code currently calls mkClient(masterAddr) unconditionally even when discovery returned an empty masterAddr; change the logic in the block that creates master and replica so you first validate masterAddr is non-empty/truthy before calling mkClient: if masterAddr is empty, do not call mkClient (set master to null/undefined or return/throw an error per the test's expected behavior) and only construct replica via mkClient(replicaAddr) when replicaAddr is truthy and different from masterAddr; update any downstream assumptions that master is always a client accordingly (symbols: masterAddr, mkClient, replicaAddr, replica).tests/e2e/cmd_stress_py/main.py (1)
52-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
--key-count,--workers, and--repeatas positive integers.Line 301 does
idx % key_mod, so--key-count=0crashes; non-positive workers/repeat can also produce false-pass runs with no meaningful load.Suggested fix
args = parser.parse_args() if not args.startup_node: print("FAIL: at least one --startup-node is required", flush=True) sys.exit(1) +if args.key_count <= 0: + print("FAIL: --key-count must be > 0", flush=True) + sys.exit(1) +if args.workers <= 0: + print("FAIL: --workers must be > 0", flush=True) + sys.exit(1) +if args.repeat <= 0: + print("FAIL: --repeat must be > 0", flush=True) + sys.exit(1)Also applies to: 294-302
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/cmd_stress_py/main.py` around lines 52 - 57, The command-line parsing accepts non-positive integers for key-related flags causing crashes or meaningless runs; after parser.parse_args() validate that args.key_count, args.workers, and args.repeat are positive integers (>0) and abort with a clear error message and non-zero exit if any are <= 0; also ensure the code that computes key_mod (used by idx % key_mod) uses the validated args.key_count to avoid division/modulo by zero. Reference: the parsed args object (args), the flags key_count/workers/repeat, and the key_mod/idx usage in the stress loop..github/workflows/test.yml (2)
36-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin all
uses:actions to immutable SHAs and disable checkout credential persistence.Line 36 still uses
actions/checkout@v4withoutpersist-credentials: false, and Lines 36/39/45/51 use mutable refs. This keeps the workflow exposed to supply-chain drift and credential persistence risk.Suggested hardening
- - uses: actions/checkout@v4 + - uses: actions/checkout@<40-char-commit-sha> + with: + persist-credentials: false ... - uses: awalsh128/cache-apt-pkgs-action@latest + uses: awalsh128/cache-apt-pkgs-action@<40-char-commit-sha> ... - uses: dtolnay/rust-toolchain@master + uses: dtolnay/rust-toolchain@<40-char-commit-sha> ... - uses: swatinem/rust-cache@v2 + uses: swatinem/rust-cache@<40-char-commit-sha>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 36 - 51, Pin all GitHub Actions usages to immutable commit SHAs and disable checkout credential persistence: replace the mutable refs actions/checkout@v4, awalsh128/cache-apt-pkgs-action@latest, dtolnay/rust-toolchain@master, and swatinem/rust-cache@v2 with their corresponding immutable SHA pins, and add persist-credentials: false to the actions/checkout step (the checkout invocation referenced as actions/checkout@v4) to prevent credential persistence; ensure the updated workflow replaces each "uses:" entry with the exact commit SHA and keeps existing inputs (e.g., packages, toolchain, components) unchanged.
79-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid direct workflow input interpolation inside shell script exports.
Lines 79-81 inject
${{ github.event.inputs.* }}directly intorun:content. Move these values intoenv:and validate format before export/use to prevent template-injection paths.Suggested hardening
- name: Run E2E test + env: + INPUT_VERSION: ${{ github.event.inputs.version || '1.2.2' }} + INPUT_DURATION: ${{ github.event.inputs.duration || '60' }} + INPUT_WORKERS: ${{ github.event.inputs.workers || '16' }} run: | + [[ "$INPUT_DURATION" =~ ^[0-9]+$ ]] || { echo "Invalid duration: $INPUT_DURATION"; exit 1; } + [[ "$INPUT_WORKERS" =~ ^[0-9]+$ ]] || { echo "Invalid workers: $INPUT_WORKERS"; exit 1; } export DOCKER_E2E_DIR="$PWD/tests/docker_ha" export ELOQCTL_HOME="$HOME/.eloqctl" export ELOQCTL="$HOME/.eloqctl/bin/cluster_mgr" - export ELOQKV_VERSION="${{ github.event.inputs.version || '1.2.2' }}" - export DURATION_SECONDS="${{ github.event.inputs.duration || '60' }}" - export WORKERS="${{ github.event.inputs.workers || '16' }}" + export ELOQKV_VERSION="$INPUT_VERSION" + export DURATION_SECONDS="$INPUT_DURATION" + export WORKERS="$INPUT_WORKERS"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 79 - 81, The workflow currently injects `${{ github.event.inputs.* }}` directly into shell exports (ELOQKV_VERSION, DURATION_SECONDS, WORKERS) which can enable template-injection; instead move these inputs into the job-level or step-level env: map (set ELOQKV_VERSION, DURATION_SECONDS, WORKERS there using the `${{ }}` expressions) and in the run: script reference them as environment variables (e.g. $ELOQKV_VERSION); before using them validate formats inside the script (validate ELOQKV_VERSION against a safe semver/allowed pattern and ensure DURATION_SECONDS and WORKERS are integers within expected ranges) and fail early if validation fails, then export or use the validated values for downstream commands.tests/e2e/cmd_stress_go/main.go (3)
777-778:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle non-positive
-durationexplicitly instead of always using timeout context.With
context.WithTimeout(ctx, duration)at Line 777,duration<=0cancels immediately; this diverges from the “run until interrupted” semantics used in other SDK stress runners.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/cmd_stress_go/main.go` around lines 777 - 778, The current call testCtx, cancel := context.WithTimeout(ctx, duration) cancels immediately when duration <= 0; change to explicitly handle non-positive durations by creating testCtx and cancel conditionally: declare testCtx (context.Context) and cancel (context.CancelFunc) beforehand, and if duration > 0 use context.WithTimeout(ctx, duration) to set them and defer cancel(), otherwise set testCtx = ctx and leave cancel nil (no defer). Update uses of testCtx accordingly so the runner uses “run until interrupted” semantics when duration is non-positive.
616-617:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
-key-countand-repeatbefore starting workers.
keyIdx % keyModat Line 569 panics whenkeyCount == 0; non-positiverepeatalso makes the stress loop non-representative.Suggested fix
flag.Parse() +if keyCount <= 0 { + logger.Fatalf("invalid -key-count: %d (must be > 0)", keyCount) +} +if repeat <= 0 { + logger.Fatalf("invalid -repeat: %d (must be > 0)", repeat) +}Also applies to: 569-570
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/cmd_stress_go/main.go` around lines 616 - 617, After flag.Parse(), validate that the parsed flags keyCount and repeat are positive (keyCount > 0 and repeat > 0) before launching workers or performing any modulo operations; if they are non-positive, log an error and exit (or set safe defaults). Specifically, guard the place that computes keyIdx % keyMod (variables keyIdx and keyMod) so it never divides by zero by ensuring keyMod (derived from keyCount) is > 0; also ensure the stress loop that depends on repeat uses the validated repeat value. Update main (or the worker-starting code that begins after flag.Parse()) to perform this check and early return/exit on invalid values.
719-728:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on preload write errors.
Lines 719-728 still ignore most preload command errors, which can invalidate both coverage and stress outcomes.
Suggested fix pattern
- masterClient.HSet(ctx, k(i)+"h", mk(i)) + if err := masterClient.HSet(ctx, k(i)+"h", mk(i)).Err(); err != nil { + logger.Fatalf("preload HSET key %d failed: %v", i, err) + }Apply the same pattern to
LPush,RPush,SAdd, and eachZAdd.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/cmd_stress_go/main.go` around lines 719 - 728, The preload writes currently ignore errors for masterClient.LPush, RPush, SAdd and ZAdd calls; update each call to capture its returned error and handle it the same way as the existing HSet check (fail fast/log and return/exit). Specifically, change masterClient.LPush(ctx, k(i)+"l", i), masterClient.RPush(ctx, k(i)+"r", i), masterClient.LPush(ctx, k(i)+"lt", "a","b","c"), masterClient.SAdd(ctx, k(i)+"s", i), masterClient.SAdd(ctx, k(i)+"s2", "shared"), masterClient.ZAdd(ctx, k(i)+"z", redis.Z{...}) and the looped masterClient.ZAdd(...) (for j := ...) to assign err := <call>; if err != nil { /* same error handling as HSet: log/fail/exit */ } so preload failures cause the test to fail fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cluster_mgr/config/alert.rules`:
- Around line 124-125: The alert descriptions for the EloqKV rules are
inconsistent with their firing expressions: update the description text in the
rule where summary is "EloqKV instance OPS too low" and the paired rule (the
other EloqKV OPS alert) so the numeric thresholds shown (currently "10000" and
"1000") match the actual PromQL expressions (e.g., change to "100000" or "100"
as appropriate), and ensure the human-readable "Current value" formatting is
preserved; locate the description strings in alert.rules and replace only the
threshold numbers to match the rule expressions.
In `@tests/e2e/cmd_stress_test.sh`:
- Line 138: The current branch "[ -z \"$MASTER\" ] && discover_master" may let
discover_master fail silently; change it to explicitly propagate failure by
invoking discover_master and then checking that MASTER is non-empty (or checking
discover_master's exit status) before continuing—i.e., call discover_master when
MASTER is empty, then if MASTER is still empty (or discover_master returned
non-zero) print an error and exit non-zero so the Python stress step doesn't run
with an empty MASTER; references: the MASTER variable and the discover_master
function.
In `@tests/e2e/cmd_stress_ts/main.ts`:
- Around line 34-37: Validate the parsed CLI numeric arguments KEY_COUNT,
WORKERS, REPEAT, and DURATION (from args) to ensure they are finite positive
integers (or non-negative if DURATION can be zero); detect NaN or values <= 0
where appropriate and fail fast with a clear error message and process exit or
throw before invoking worker/index logic. Update the parsing sites that set
KEY_COUNT, WORKERS, REPEAT, and DURATION to perform Number.isFinite checks and
integer validation (e.g., Math.floor equality) and centralize validation so
downstream code assumes safe numbers. Ensure error messages name the offending
arg (KEY_COUNT/WORKERS/REPEAT/DURATION) and the invalid value to aid debugging.
---
Outside diff comments:
In `@src/cluster_mgr/src/cli/task/upload/eloq_upload_builder.rs`:
- Around line 81-154: The block populating the all_files_path Vec (built from
config.gen_log_start_script(), DeploymentPackage hosts, and scanning
create_upload_cluster_dir for EloqDss-*.ini) is dead because all_files_path is
never used (it's assigned to _); remove this entire construction or replace its
usage with the intended downstream call (e.g., use list_files_by_host or pass
all_files_path into the uploader) so files are actually consumed; locate
references to all_files_path, config.gen_log_start_script(),
create_upload_cluster_dir, and any upload/listing code to either wire the vector
into the upload flow or delete the unused code to avoid confusion.
In `@src/cluster_mgr/src/config/config_base.rs`:
- Around line 420-427: In client_conn(), avoid indexing parts[1] which can panic
if host_port lacks a colon; instead parse host_port using split_once(':') (or
check parts.len() >= 2) to safely extract host and port and fall back to a
default port (e.g., 6379) or return an error; then call
self.service_endpoint(host, parsed_port) and format the command as before.
Update logic around deployment.tx_service.tx_host_ports.first().unwrap() and
host_port handling in client_conn so malformed entries don't panic (use safe
unwrap_or / matching and handle the None case defensively).
In `@tests/README.md`:
- Line 92: The README health-check command uses the wrong CLI binary name;
replace the occurrence of "~/.eloqctl/bin/eloqctl status test-e2e --wait 30"
with the CI-installed binary name "~/.eloqctl/bin/cluster_mgr status test-e2e
--wait 30" so the documented command matches the cluster_mgr installation used
by the test/setup scripts.
---
Duplicate comments:
In @.github/workflows/test.yml:
- Around line 36-51: Pin all GitHub Actions usages to immutable commit SHAs and
disable checkout credential persistence: replace the mutable refs
actions/checkout@v4, awalsh128/cache-apt-pkgs-action@latest,
dtolnay/rust-toolchain@master, and swatinem/rust-cache@v2 with their
corresponding immutable SHA pins, and add persist-credentials: false to the
actions/checkout step (the checkout invocation referenced as
actions/checkout@v4) to prevent credential persistence; ensure the updated
workflow replaces each "uses:" entry with the exact commit SHA and keeps
existing inputs (e.g., packages, toolchain, components) unchanged.
- Around line 79-81: The workflow currently injects `${{ github.event.inputs.*
}}` directly into shell exports (ELOQKV_VERSION, DURATION_SECONDS, WORKERS)
which can enable template-injection; instead move these inputs into the
job-level or step-level env: map (set ELOQKV_VERSION, DURATION_SECONDS, WORKERS
there using the `${{ }}` expressions) and in the run: script reference them as
environment variables (e.g. $ELOQKV_VERSION); before using them validate formats
inside the script (validate ELOQKV_VERSION against a safe semver/allowed pattern
and ensure DURATION_SECONDS and WORKERS are integers within expected ranges) and
fail early if validation fails, then export or use the validated values for
downstream commands.
In `@tests/e2e/cmd_stress_go/main.go`:
- Around line 777-778: The current call testCtx, cancel :=
context.WithTimeout(ctx, duration) cancels immediately when duration <= 0;
change to explicitly handle non-positive durations by creating testCtx and
cancel conditionally: declare testCtx (context.Context) and cancel
(context.CancelFunc) beforehand, and if duration > 0 use
context.WithTimeout(ctx, duration) to set them and defer cancel(), otherwise set
testCtx = ctx and leave cancel nil (no defer). Update uses of testCtx
accordingly so the runner uses “run until interrupted” semantics when duration
is non-positive.
- Around line 616-617: After flag.Parse(), validate that the parsed flags
keyCount and repeat are positive (keyCount > 0 and repeat > 0) before launching
workers or performing any modulo operations; if they are non-positive, log an
error and exit (or set safe defaults). Specifically, guard the place that
computes keyIdx % keyMod (variables keyIdx and keyMod) so it never divides by
zero by ensuring keyMod (derived from keyCount) is > 0; also ensure the stress
loop that depends on repeat uses the validated repeat value. Update main (or the
worker-starting code that begins after flag.Parse()) to perform this check and
early return/exit on invalid values.
- Around line 719-728: The preload writes currently ignore errors for
masterClient.LPush, RPush, SAdd and ZAdd calls; update each call to capture its
returned error and handle it the same way as the existing HSet check (fail
fast/log and return/exit). Specifically, change masterClient.LPush(ctx,
k(i)+"l", i), masterClient.RPush(ctx, k(i)+"r", i), masterClient.LPush(ctx,
k(i)+"lt", "a","b","c"), masterClient.SAdd(ctx, k(i)+"s", i),
masterClient.SAdd(ctx, k(i)+"s2", "shared"), masterClient.ZAdd(ctx, k(i)+"z",
redis.Z{...}) and the looped masterClient.ZAdd(...) (for j := ...) to assign err
:= <call>; if err != nil { /* same error handling as HSet: log/fail/exit */ } so
preload failures cause the test to fail fast.
In `@tests/e2e/cmd_stress_py/main.py`:
- Around line 52-57: The command-line parsing accepts non-positive integers for
key-related flags causing crashes or meaningless runs; after parser.parse_args()
validate that args.key_count, args.workers, and args.repeat are positive
integers (>0) and abort with a clear error message and non-zero exit if any are
<= 0; also ensure the code that computes key_mod (used by idx % key_mod) uses
the validated args.key_count to avoid division/modulo by zero. Reference: the
parsed args object (args), the flags key_count/workers/repeat, and the
key_mod/idx usage in the stress loop.
In `@tests/e2e/cmd_stress_test.sh`:
- Around line 158-159: The setup commands in the stress container script are
silently swallowing failures by appending "|| true" (e.g., the "'cd
tests/e2e/cmd_stress_go && go mod download' 2>&1 || true" invocation and the
similar command around lines 179-180); remove the "|| true" from these commands
so dependency installation failures propagate as non-zero exit codes, or
alternatively capture and explicitly log the error and exit non-zero after the
command in the stress setup block (look for the compose exec -T stress-go bash
-c sections and the go mod download invocations to apply the change).
In `@tests/e2e/cmd_stress_ts/main.ts`:
- Around line 32-33: The CMD_TIMEOUT and PROGRESS_INTERVAL conversions are
multiplying defaults (which are strings like "5000") by 1000 and producing
values in microseconds; fix by making units consistent: either change the
default values to seconds (e.g. "5" instead of "5000") or stop multiplying by
1000 when parsing. Update the parsing lines that set CMD_TIMEOUT and
PROGRESS_INTERVAL to use parseInt(args["cmd-timeout"]!, 10) and
parseInt(args["progress-interval"]!, 10) without the extra *1000 if the defaults
are already milliseconds, or keep the *1000 and change the default strings to
seconds ("5") so both sides agree.
- Around line 318-319: The code currently calls mkClient(masterAddr)
unconditionally even when discovery returned an empty masterAddr; change the
logic in the block that creates master and replica so you first validate
masterAddr is non-empty/truthy before calling mkClient: if masterAddr is empty,
do not call mkClient (set master to null/undefined or return/throw an error per
the test's expected behavior) and only construct replica via
mkClient(replicaAddr) when replicaAddr is truthy and different from masterAddr;
update any downstream assumptions that master is always a client accordingly
(symbols: masterAddr, mkClient, replicaAddr, replica).
In `@tests/README.md`:
- Around line 44-67: The README contains fenced code blocks with ASCII directory
trees that lack a language identifier; update each fenced block showing the
tests/ directory tree (the triple-backtick blocks containing the ASCII art like
"tests/ ├── README.md ...") and the other similar fenced block later in the file
to include a language label (e.g., change ``` to ```text) so markdownlint no
longer flags missing language identifiers.
- Line 10: Replace the machine-specific absolute path "cd
/home/starrysky/workspace/eloqdata-kernel/eloq_waiter" in the Quick Start
section with a repo-relative command (for example "cd eloq_waiter" or "cd
./eloq_waiter" or instructions to run from repository root) so the setup is
portable for other contributors; update the README.md Quick Start snippet
accordingly and keep the intent clear that the command should be run from the
repository root or provide an alternative that works cross-platform.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6190f3aa-53fa-4a63-a1e5-d6dc990f4fbd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.githooks/pre-push.github/workflows/test.yml.gitignorescripts/test-before-push.shsrc/cluster_mgr/config/alert.rulessrc/cluster_mgr/config/mono_prometheus.yamlsrc/cluster_mgr/src/cli/task/monitor_ctl_task.rssrc/cluster_mgr/src/cli/task/upload/eloq_upload_builder.rssrc/cluster_mgr/src/config/config_base.rssrc/cluster_mgr/src/config/deployment.rstests/README.mdtests/docker_env.shtests/docker_ha/docker-compose.yamltests/e2e/cmd_stress_go/main.gotests/e2e/cmd_stress_py/main.pytests/e2e/cmd_stress_test.shtests/e2e/cmd_stress_ts/main.ts
💤 Files with no reviewable changes (2)
- tests/docker_ha/docker-compose.yaml
- tests/docker_env.sh
✅ Files skipped from review due to trivial changes (2)
- .githooks/pre-push
- .gitignore
Comment on lines
+124
to
+125
| summary: "EloqKV instance OPS too low" | ||
| description: "EloqKV instance {{ $labels.instance }} OPS has been below 10000 for 1 minutes. Current value: {{ printf \"%.2f\" $value }}" |
There was a problem hiding this comment.
Align alert descriptions with actual firing thresholds.
Line 125 and Line 134 describe thresholds (10000, 1000) that do not match the rule expressions (100000, 100). This can mislead on-call diagnosis.
Suggested fix
- alert: eloqkv_instance_ops_too_low
expr: sum by (instance) (rate(redis_command_aggregated_total[1m])) < 100000
for: 1m
labels:
severity: critical
annotations:
summary: "EloqKV instance OPS too low"
- description: "EloqKV instance {{ $labels.instance }} OPS has been below 10000 for 1 minutes. Current value: {{ printf \"%.2f\" $value }}"
+ description: "EloqKV instance {{ $labels.instance }} OPS has been below 100000 for 1 minute. Current value: {{ printf \"%.2f\" $value }}"
@@
- alert: eloqkv_active_connections_too_high
expr: sum by (instance) (redis_connection_count) > 100
for: 1m
labels:
severity: warning
annotations:
summary: "EloqKV active connections too high"
- description: "EloqKV instance {{ $labels.instance }} active connections have exceeded 1000 for 1 minute. Current value: {{ printf \"%.0f\" $value }}"
+ description: "EloqKV instance {{ $labels.instance }} active connections have exceeded 100 for 1 minute. Current value: {{ printf \"%.0f\" $value }}"Also applies to: 133-134
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cluster_mgr/config/alert.rules` around lines 124 - 125, The alert
descriptions for the EloqKV rules are inconsistent with their firing
expressions: update the description text in the rule where summary is "EloqKV
instance OPS too low" and the paired rule (the other EloqKV OPS alert) so the
numeric thresholds shown (currently "10000" and "1000") match the actual PromQL
expressions (e.g., change to "100000" or "100" as appropriate), and ensure the
human-readable "Current value" formatting is preserved; locate the description
strings in alert.rules and replace only the threshold numbers to match the rule
expressions.
Comment on lines
+34
to
+37
| const KEY_COUNT = parseInt(args["key-count"]!); | ||
| const WORKERS = parseInt(args.workers!); | ||
| const REPEAT = parseInt(args.repeat!); | ||
| const DURATION = parseInt(args.duration!); |
There was a problem hiding this comment.
Validate numeric CLI args to avoid invalid stress runs.
KEY_COUNT, WORKERS, and REPEAT are used directly in worker/index logic; non-positive or NaN values can silently produce meaningless results.
Suggested fix
const KEY_COUNT = parseInt(args["key-count"]!);
const WORKERS = parseInt(args.workers!);
const REPEAT = parseInt(args.repeat!);
const DURATION = parseInt(args.duration!);
+if (!Number.isFinite(KEY_COUNT) || KEY_COUNT <= 0) throw new Error("--key-count must be > 0");
+if (!Number.isFinite(WORKERS) || WORKERS <= 0) throw new Error("--workers must be > 0");
+if (!Number.isFinite(REPEAT) || REPEAT <= 0) throw new Error("--repeat must be > 0");Also applies to: 268-269
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/cmd_stress_ts/main.ts` around lines 34 - 37, Validate the parsed
CLI numeric arguments KEY_COUNT, WORKERS, REPEAT, and DURATION (from args) to
ensure they are finite positive integers (or non-negative if DURATION can be
zero); detect NaN or values <= 0 where appropriate and fail fast with a clear
error message and process exit or throw before invoking worker/index logic.
Update the parsing sites that set KEY_COUNT, WORKERS, REPEAT, and DURATION to
perform Number.isFinite checks and integer validation (e.g., Math.floor
equality) and centralize validation so downstream code assumes safe numbers.
Ensure error messages name the offending arg (KEY_COUNT/WORKERS/REPEAT/DURATION)
and the invalid value to aid debugging.
This was referenced May 26, 2026
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
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.
Summary
Add a unified e2e test suite (
cmd_stress_test.sh) that deploys an EloqKV cluster(master + standby + voter, TLS + MinIO) and runs full Redis command coverage stress
across Python, Go, and TypeScript SDKs.
What's new
tests/e2e/cmd_stress_test.sh— launch → stress → remove${ELOQKV_VERSION}can be set at launch timesecurity_opt/cap_addfor runner compatibilityWhat's removed
tests/e2e/test.sh— superseded bycmd_stress_test.shtests/e2e/stress_test.sh+stress.py— legacy connection-stresstests/e2e/go_cluster_tls_stress.sh+ dir — covered by the Go SDK testtests/e2e/memtier_cluster_tls_stress.sh— external tool, not coretests/docker_ha/topology.yaml— redundant with e2e/topology.yamlTest results (local, 16 workers, 10s duration)
Summary by CodeRabbit
Tests
Documentation
Chores
Monitoring