Repository navigation
fix: auto-upgrade local state during install - #407
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).
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds typed monitor control/update, GitHub Releases-based versioning, TLS-aware Redis probes, local extract + rsync uploads, new rolling-upgrade wait step, expanded configs/templates, replaces CI workflow and E2E with multi-language Docker stress harness, and updates docs/install scripts. ChangesCluster Manager and E2E Overhaul
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as eloqctl (Monitor Update)
participant Cmd as CmdExecutor
participant GH as GitHub API
participant DL as DownloadTask
participant LE as LocalExtractTask
participant UP as UploadTask (rsync)
participant SSH as Remote Host
User->>CLI: monitor update --components grafana
CLI->>Cmd: run_update_command(Monitor::Update)
Cmd->>GH: fetch releases/list assets
Cmd->>DL: create/download URLs (+sha256)
DL-->>Cmd: cached/verified artifacts
Cmd->>LE: extract staged dirs
LE-->>Cmd: staged paths
Cmd->>UP: rsync staged dirs to hosts
UP->>SSH: transfer files
SSH-->>UP: ok
Cmd-->>CLI: update summary/status
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
This PR enhances eloqctl installation and upgrade workflows by automatically upgrading local state during install, modernizing release/version resolution via GitHub Releases, and updating the Docker E2E stress-test harness and documentation to match the new workflow.
Changes:
- Run
eloqctl upgradeautomatically duringinstall.shwhen an existing local state install is detected. - Switch version discovery / tarball selection to GitHub Releases (
eloqdata/eloqkv), add local extraction + rsync-based syncing, and improve task stage labeling. - Replace legacy E2E scripts with a multi-SDK stress framework (Python/Go/TypeScript) and update Docker + docs accordingly.
Reviewed changes
Copilot reviewed 57 out of 61 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/README.md | Updated E2E usage docs for new multi-SDK stress workflow. |
| tests/install_control_eloqctl.sh | New helper to install host-built eloqctl into Docker control node. |
| tests/e2e/topology.yaml | E2E topology updated (TLS/minio/version env). |
| tests/e2e/test.sh | Removed legacy scenario-based E2E script. |
| tests/e2e/stress.py | Removed legacy Python-only stress script. |
| tests/e2e/stress_test.sh | Removed legacy stress wrapper script. |
| tests/e2e/cmd_stress_ts/tsconfig.json | New TS stress client build config. |
| tests/e2e/cmd_stress_ts/package.json | New TS stress client dependencies/scripts. |
| tests/e2e/cmd_stress_ts/package-lock.json | Lockfile for TS stress client. |
| tests/e2e/cmd_stress_ts/main.ts | New ioredis-based full-command stress client. |
| tests/e2e/cmd_stress_test.sh | New orchestration script for launch + multi-SDK stress + cleanup. |
| tests/e2e/cmd_stress_py/requirements.txt | Pinned redis-py dependency for Python stress client. |
| tests/e2e/cmd_stress_py/main.py | New redis-py full-command stress client. |
| tests/e2e/cmd_stress_go/go.sum | Go module lock for Go stress client. |
| tests/e2e/cmd_stress_go/go.mod | Go module definition for Go stress client. |
| tests/docker_ha/topology.yaml | Removed unused legacy docker_ha topology file. |
| tests/docker_ha/Dockerfile.stress_ts | New Node-based stress container image. |
| tests/docker_ha/Dockerfile.stress_go | New Go-based stress container image. |
| tests/docker_ha/Dockerfile.stress | New Python-based stress container image. |
| tests/docker_ha/Dockerfile | Expanded node image tooling (debugging + redis tools). |
| tests/docker_ha/docker-compose.yaml | Adds MinIO + control node + stress containers to E2E compose. |
| tests/docker_env.sh | Adds control-node helpers, MinIO bootstrap, topology rendering updates, improved progress output. |
| src/cluster_mgr/src/lib.rs | Exposes new github_release module. |
| src/cluster_mgr/src/github_release.rs | New GitHub Releases parsing + asset matching helpers (with tests). |
| src/cluster_mgr/src/config/mod.rs | Adjusts download cache dir derivation for GitHub-hosted assets. |
| src/cluster_mgr/src/config/deployment.rs | Adds optional event_dispatcher_num to hardware config. |
| src/cluster_mgr/src/config/config_base.rs | Improves client connect string handling via service endpoint mapping. |
| src/cluster_mgr/src/cli/task/upload/upload_task.rs | Generalizes upload task to use rsync for directory syncs. |
| src/cluster_mgr/src/cli/task/upload/upload_task_builder.rs | Adds rsync arg builder; switches upload task inputs to transfer abstraction. |
| src/cluster_mgr/src/cli/task/upload/tx_conf_upload_builder.rs | Initializes new delete_remote field for uploads. |
| src/cluster_mgr/src/cli/task/upload/proxy_upload_builder.rs | Initializes new delete_remote field for uploads. |
| src/cluster_mgr/src/cli/task/upload/monitor_upload_builder.rs | Initializes new delete_remote field for uploads. |
| src/cluster_mgr/src/cli/task/upload/eloq_upload_builder.rs | Moves to local-extract + rsync directory sync approach; refactors upload grouping. |
| src/cluster_mgr/src/cli/task/upload/data_dir_upload_builder.rs | Enables remote deletion when syncing data directories. |
| src/cluster_mgr/src/cli/task/unpack_file_task.rs | Makes make_task_pair public for reuse. |
| src/cluster_mgr/src/cli/task/task_controller.rs | Improves stage labels based on task composition. |
| src/cluster_mgr/src/cli/task/task_base.rs | Adds run_context helper to run an explicit task context. |
| src/cluster_mgr/src/cli/task/rolling_upgrade/steps.rs | Adds local extraction stage; removes now-unused unpack steps. |
| src/cluster_mgr/src/cli/task/monitor_ctl_task.rs | Enables Prometheus lifecycle endpoint and updates unit test. |
| src/cluster_mgr/src/cli/task/mod.rs | Exposes download_task, local_extract_task, monitor_ctl_task. |
| src/cluster_mgr/src/cli/task/local_extract_task.rs | New local tar.gz extraction task for staging release directories. |
| src/cluster_mgr/src/cli/task/group/scale_log_group.rs | Switches log scale to download+extract+rsync logservice dir and script uploads. |
| src/cluster_mgr/src/cli/task/group/scale_group.rs | Switches scale add-node version sync to download+extract+rsync staged TX dir. |
| src/cluster_mgr/src/cli/task/group/deployment_group.rs | Adds local extract stage; removes remote unpack stage in deployment pipeline. |
| src/cluster_mgr/src/cli/task/download_task.rs | Adds optional sha256 validation via GitHub release asset digest and improved cache logic. |
| src/cluster_mgr/src/cli/mod.rs | Extends update CLI with --download-only and --monitor modes. |
| src/cluster_mgr/src/cli/cmd_base.rs | Implements GitHub-based version resolution; adds download-only & monitor-only update flows; enhances status output hints. |
| src/cluster_mgr/config/mono_prometheus.yaml | Enables alertmanager target block by default. |
| src/cluster_mgr/config/alert.rules | Adds new alert rules for ops/connection thresholds. |
| src/cluster_mgr/Cargo.toml | Adds flate2, tar, sha2 dependencies for local extract + digest checks. |
| scripts/test-before-push.sh | Removes local E2E run from pre-push; points to CI for full E2E. |
| README.md | Updates install URLs + documents installer auto-upgrade behavior. |
| install.sh | Detects existing state and runs eloqctl upgrade automatically. |
| doc/upgrade.md | Documents auto migration + GitHub Releases version resolution and download-only/monitor update modes. |
| doc/cluster_mgr.md | Updates command docs for new update/versions behavior. |
| Cargo.lock | Updates lockfile for new Rust dependencies/version bump. |
| .gitignore | Ignores local dev tmp dir + node_modules/pycache/Go stress binary. |
| .github/workflows/test.yml | New consolidated workflow including formatting, clippy, unit tests, and Docker E2E stress run. |
| .github/workflows/quality.yml | Removed older split quality workflow. |
| .githooks/pre-push | Updates messaging to reflect “fast” local checks. |
Files not reviewed (1)
- tests/e2e/cmd_stress_ts/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
src/cluster_mgr/config/alert.rules:134
- The alert description doesn’t match the rule: the expr triggers when connections are
> 100, but the description says exceeded1000. Align the description (and/or threshold) with the expr so on-call operators aren’t misled.
- 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 }}"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+154
to
+166
| let segments = url | ||
| .path_segments() | ||
| .map(|segments| segments.collect_vec()) | ||
| .unwrap_or_default(); | ||
| if segments.len() < 8 | ||
| || segments[0] != "eloqdata" | ||
| || segments[1] != "eloqkv" | ||
| || segments[2] != "releases" | ||
| || segments[3] != "download" | ||
| { | ||
| return Ok(None); | ||
| } | ||
|
|
Comment on lines
+407
to
413
| let staged_dir = LocalExtractTask::staged_dir_for(&tx_download_url, &tx_home); | ||
| if !staged_dir.exists() { | ||
| bail!( | ||
| "Unable to determine TX service image filename from {}", | ||
| effective_tx_image_url | ||
| "TX service staged directory not found: {}", | ||
| staged_dir.display() | ||
| ); | ||
| } |
Comment on lines
+282
to
+287
| if !staged_dir.exists() { | ||
| return Err(anyhow!( | ||
| "Log service staged directory not found: {}", | ||
| staged_dir.display() | ||
| )); | ||
| } |
Comment on lines
+17
to
+37
| "startup-nodes": { type: "string", default: "127.0.0.1:6379,127.0.0.1:6380" }, | ||
| password: { type: "string", default: "testpass" }, | ||
| "cmd-timeout": { type: "string", default: "5000" }, | ||
| "progress-interval": { type: "string", default: "5000" }, | ||
| "key-count": { type: "string", default: "256" }, | ||
| workers: { type: "string", default: "16" }, | ||
| inflight: { type: "string", default: "4" }, | ||
| repeat: { type: "string", default: "10" }, | ||
| duration: { type: "string", default: "0" }, | ||
| "tls-insecure": { type: "string", default: "true" }, | ||
| "read-from-replicas": { type: "string", default: "true" }, | ||
| "command-set": { type: "string", default: "full" }, | ||
| "log-prefix": { type: "string", default: "" }, | ||
| }, | ||
| }); | ||
|
|
||
| const startupNodes = args["startup-nodes"]!.split(",").map(s => s.trim()); | ||
| const password = args.password!; | ||
| const CMD_TIMEOUT = parseInt(args["cmd-timeout"]!) * 1000; | ||
| const PROGRESS_INTERVAL = parseInt(args["progress-interval"]!) * 1000; | ||
| const KEY_COUNT = parseInt(args["key-count"]!); |
Comment on lines
+296
to
+302
| --repeat ${REPEAT} \ | ||
| --progress-interval ${PROGRESS_INTERVAL} \ | ||
| --key-count ${KEY_COUNT} \ | ||
| --cmd-timeout ${CMD_TIMEOUT} \ | ||
| --read-from-replicas 'false' \ | ||
| --tls-insecure '${TLS_ENABLED}'" \ | ||
| 2>&1 | tee "${SCRIPT_DIR}/cmd-stress-ts.log" |
Comment on lines
+9
to
+14
| ```sh | ||
| cd /home/starrysky/workspace/eloqdata-kernel/eloq_waiter | ||
|
|
||
| # Build eloqctl (one time) | ||
| scripts/install-dev.sh | ||
|
|
Comment on lines
+556
to
+564
| ) -> TaskExecutionContext { | ||
| let mut executable = indexmap::IndexMap::new(); | ||
| let mut barrier = vec![]; | ||
|
|
||
| let download = DownloadTask::from_config(config).unwrap_or_default(); | ||
| if !download.is_empty() { | ||
| barrier.push(download.len()); | ||
| executable.extend(download); | ||
| } |
Comment on lines
+118
to
+126
| - 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 }}" | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cluster_mgr/src/cli/task/upload/data_dir_upload_builder.rs (1)
31-35:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
delete_remote: trueagainsttx_srv_homecan wipe unrelated remote files.With directory sync semantics,
--deletewill remove destination entries not present indatafarm. Sincedestistx_srv_home, this can delete non-datafarm content.Suggested fix
UploadFile { source: datafarm.clone(), - dest: txsrv_home.clone(), + dest: format!("{}/datafarm", txsrv_home), extension: "datafarm".to_string(), host: host.to_string(), copy_dir: true, - delete_remote: true, + delete_remote: true, }Or keep
dest: txsrv_homeand setdelete_remote: false.🤖 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/data_dir_upload_builder.rs` around lines 31 - 35, The upload builder is setting delete_remote: true while dest is txsrv_home, which risks removing unrelated files in the remote txsrv_home; update the builder used in data_dir_upload_builder.rs to either change dest to a datafarm-specific subpath (e.g., a path that clearly targets the datafarm directory) or set delete_remote: false so the sync won't prune files outside the datafarm; locate the struct/variable initialization referencing dest: txsrv_home, extension: "datafarm", copy_dir: true and modify the delete_remote field accordingly.src/cluster_mgr/src/config/config_base.rs (1)
429-435:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid panic in
client_connwhen parsinghost:port.
split(':')withparts[1]/parts[0]can panic on malformed input. Usersplit_once(':')and handle invalid entries safely.Suggested 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 = self + .deployment + .tx_service + .tx_host_ports + .first() + .cloned() + .unwrap_or_else(|| "127.0.0.1:6379".to_string()); + let (raw_host, raw_port) = host_port + .rsplit_once(':') + .unwrap_or(("127.0.0.1", "6379")); + let port = raw_port.parse::<u16>().unwrap_or(6379); + let (host, port) = self.service_endpoint(raw_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 429 - 435, The client_conn function currently unwraps parts from host_port.split(':') which can panic on malformed input; update client_conn to use host_port.rsplit_once(':') to safely split into (host, port_str) and parse port_str with parse::<u16>().map_or(DEFAULT_PORT, |p| p) or similar, and handle the None case by treating the whole host_port as host with a default port (e.g., 6379); remove all unwraps—specifically change the logic around deployment.tx_service.tx_host_ports.first().unwrap(), host_port.split, parts[0]/parts[1], and the parse::<u16>().unwrap_or so errors are gracefully handled and a sensible fallback connection string is returned by client_conn and passed into service_endpoint.
🟠 Major comments (19)
.github/workflows/test.yml-46-61 (1)
46-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin mutable GitHub Actions refs, disable checkout token persistence, and sanitize workflow_dispatch inputs
- Pin all
uses:references in.github/workflows/test.yml(actions/checkout@v4,awalsh128/cache-apt-pkgs-action@latest,dtolnay/rust-toolchain@master,swatinem/rust-cache@v2) to full commit SHAs to avoid supply-chain drift.- Add
persist-credentials: falsetoactions/checkout(currently missing), to reduce credential/token exposure surface.- The
workflow_dispatchinputs are interpolated directly into the bash script (workers,duration,repeat,inflight,ELOQKV_VERSION), enabling command/variable injection if inputs contain quotes or shell metacharacters; validate/sanitize to expected formats (e.g., numeric-only) before use.Suggested hardening diff
- - uses: actions/checkout@v4 + - uses: actions/checkout@<full_commit_sha_for_v4> + with: + persist-credentials: false - - name: Install build dependencies - uses: awalsh128/cache-apt-pkgs-action@latest + - name: Install build dependencies + uses: awalsh128/cache-apt-pkgs-action@<full_commit_sha> - - name: Set up Rust - uses: dtolnay/rust-toolchain@master + - name: Set up Rust + uses: dtolnay/rust-toolchain@<full_commit_sha> - - name: Cache Rust build artifacts - uses: swatinem/rust-cache@v2 + - name: Cache Rust build artifacts + uses: swatinem/rust-cache@<full_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 46 - 61, Pin all mutable GitHub Action refs and harden checkout and workflow inputs: replace floating refs (actions/checkout@v4, awalsh128/cache-apt-pkgs-action@latest, dtolnay/rust-toolchain@master, swatinem/rust-cache@v2) with their full commit SHAs, add persist-credentials: false to the actions/checkout step, and validate/sanitize workflow_dispatch inputs (workers, duration, repeat, inflight, ELOQKV_VERSION) before interpolating into bash—e.g., enforce numeric-only for counts/durations and whitelist/escape allowed characters for ELOQKV_VERSION—to prevent supply-chain drift, credential persistence, and command/variable injection..github/workflows/test.yml-82-85 (1)
82-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd strict validation for workflow_dispatch inputs before they’re interpolated into shell commands
workers/duration/repeat/inflightflow fromgithub.event.inputs.*intotests/e2e/cmd_stress_test.sh, where the Go/TS stress paths build abash -c " ... --workers ${WORKERS} ..."command without quoting, allowing input like8; <cmd>to execute on the runner.versionis also interpolated into the sed-based topology renderer without escaping.Suggested validation pattern
- name: Run E2E test run: | if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then workers="${{ github.event.inputs.workers || '8' }}" duration="${{ github.event.inputs.duration || '20' }}" repeat="${{ github.event.inputs.repeat || '8' }}" inflight="${{ github.event.inputs.inflight || '4' }}" + version="${{ github.event.inputs.version || '1.2.2' }}" else workers="8" duration="20" repeat="8" inflight="4" + version="1.2.2" fi + + case "$workers" in ''|*[!0-9]*) echo "invalid workers"; exit 1;; esac + case "$duration" in ''|*[!0-9]*) echo "invalid duration"; exit 1;; esac + case "$repeat" in ''|*[!0-9]*) echo "invalid repeat"; exit 1;; esac + case "$inflight" in ''|*[!0-9]*) echo "invalid inflight"; exit 1;; esac + case "$version" in ''|*[!0-9A-Za-z._-]*) echo "invalid version"; exit 1;; esac export DOCKER_E2E_DIR="$PWD/tests/docker_ha" @@ - export ELOQKV_VERSION="${{ github.event.inputs.version || '1.2.2' }}" + export ELOQKV_VERSION="${version}"🤖 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 - 85, Add strict validation and safer interpolation for workflow_dispatch inputs: ensure workers, duration, repeat, and inflight are validated as positive integers (e.g., regex ^[0-9]+$) in the workflow before exporting them to the environment and before invoking tests/e2e/cmd_stress_test.sh (reject/exit on invalid input), and validate/whitelist the version string (e.g., semver pattern or specific tags) before it’s used by the sed topology renderer; also pass these values safely to shell (quote variables and avoid unquoted bash -c interpolation) or call the script directly with positional args to prevent command injection (references: workflow variables workers/duration/repeat/inflight, tests/e2e/cmd_stress_test.sh, exported env vars WORKERS/DURATION/REPEAT/INFLIGHT, and the version used by the sed topology renderer).src/cluster_mgr/src/cli/cmd_base.rs-898-900 (1)
898-900:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid panic when monitor cluster is supplied only in subcommand args.
expect("monitor cluster is resolved")can panic when parentSubCommand::Monitor.clusterisNonebut childMonitorCommand::Update.clusteris present (a path you already accept viamonitor_cluster). Resolve once viamonitor_cluster(...)and reuse that value here instead ofexpect.Suggested fix
- let stop_cmd = SubCommand::Monitor { - cluster: Some(match cmd { - SubCommand::Update { - cluster: Some(cluster), - .. - } => cluster.clone(), - SubCommand::Monitor { cluster, .. } => { - cluster.clone().expect("monitor cluster is resolved") - } - _ => unreachable!(), - }), + let resolved_cluster = match cmd { + SubCommand::Update { + cluster: Some(cluster), + .. + } => cluster.clone(), + SubCommand::Monitor { cluster, command } => { + Self::monitor_cluster(cluster, command)? + } + _ => unreachable!(), + }; + let stop_cmd = SubCommand::Monitor { + cluster: Some(resolved_cluster.clone()), command: MonitorCommand::Stop { cluster: None, components: vec![], }, }; let start_cmd = SubCommand::Monitor { - cluster: Some(match cmd { - SubCommand::Update { - cluster: Some(cluster), - .. - } => cluster.clone(), - SubCommand::Monitor { cluster, .. } => { - cluster.clone().expect("monitor cluster is resolved") - } - _ => unreachable!(), - }), + cluster: Some(resolved_cluster), command: MonitorCommand::Start { cluster: None, components: vec![], }, };Also applies to: 914-916
🤖 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/cmd_base.rs` around lines 898 - 900, The code currently forces a panic by calling expect("monitor cluster is resolved") on SubCommand::Monitor when the parent enum's cluster is None; instead call the existing monitor_cluster(...) helper once to resolve the cluster (which checks both the parent SubCommand::Monitor.cluster and the child MonitorCommand variants like MonitorCommand::Update) and reuse that returned value here and at the other occurrence (the similar block at the Monitor branch around the later lines). Replace direct clone/expect usage in the SubCommand::Monitor arm with the monitor_cluster(...) result so the logic is centralized and avoids panics.src/cluster_mgr/src/cli/cmd_base.rs-1187-1190 (1)
1187-1190:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
OpenOptionsaccess mode for quiet-mode result filesThe
quiet-modeOpenOptionscreate/truncate handlers opentask-result/operation-resultwithout any access mode flags, which can lead to write failures when the code later writes to the file. Add.write(true)to these blocks:Suggested fix
let f = fs::OpenOptions::new() + .write(true) .create(true) .truncate(true) .open(self.home.join("task-result"))?;Apply to: 1215-1218, 1319-1322, 1383-1386, 2255-2258, 2321-2324 (in addition to 1187-1190).
🤖 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/cmd_base.rs` around lines 1187 - 1190, The OpenOptions calls that create/truncate quiet-mode result files (calls building OpenOptions::new().create(true).truncate(true).open(self.home.join("task-result")) and similar for "operation-result") lack write permission, causing downstream writes to fail; for each such OpenOptions block (the ones opening self.home.join("task-result") and self.home.join("operation-result")), add .write(true) to the builder chain so the file is opened for writing (i.e., change OpenOptions::new().create(true).truncate(true) to OpenOptions::new().create(true).truncate(true).write(true)).src/cluster_mgr/src/cli/task/local_extract_task.rs-144-167 (1)
144-167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRaw binary extraction hardcodes output filename.
unpack_raw_binaryalways writes toALERTMANAGER_WEBHOOK_ADAPTER_BINARYregardless of the actual input file. If this method is called for a different raw binary, the output filename will be incorrect.Consider deriving the output filename from the archive path or passing it as a parameter.
Suggested fix
fn unpack_raw_binary(&self, archive_path: &Path, destination: &Path) -> Result<()> { let file = File::open(archive_path)?; let binary_len = file.metadata()?.len(); self.pg_bar.set_length(binary_len); self.pg_bar.set_position(0); self.pg_bar.set_message(format!( "{} Extracting...", Self::archive_display_name(archive_path) )); let mut reader = self.pg_bar.wrap_read(file); - let output_path = destination.join(ALERTMANAGER_WEBHOOK_ADAPTER_BINARY); + let output_name = archive_path + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or(ALERTMANAGER_WEBHOOK_ADAPTER_BINARY); + let output_path = destination.join(output_name); let mut out_file = File::create(&output_path)?;🤖 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/local_extract_task.rs` around lines 144 - 167, unpack_raw_binary currently hardcodes the output filename to ALERTMANAGER_WEBHOOK_ADAPTER_BINARY causing incorrect filenames for other binaries; change it to derive the output filename from archive_path (e.g., use archive_path.file_name() or file_stem() plus extension) or add an output_name parameter to unpack_raw_binary, then build output_path = destination.join(derived_name) instead of joining ALERTMANAGER_WEBHOOK_ADAPTER_BINARY; update any callers of unpack_raw_binary to pass the desired name if you choose the parameter approach and keep the progress/message text using the derived name.src/cluster_mgr/src/cli/task/unpack_file_task.rs-38-40 (1)
38-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNarrow the
linux.zipspecial-case mapping.Line 38 currently maps any
linux.zipartifact toalertmanager-webhook-adapter. That can send unrelated archives to the wrong unpack target and break rollout when another component also ships a genericlinux.zip.Suggested fix
- if raw_file_name.contains("PrometheusAlert") || raw_file_name == "linux.zip" { + if raw_file_name.contains("PrometheusAlert") { return "alertmanager-webhook-adapter".to_string(); }🤖 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/unpack_file_task.rs` around lines 38 - 40, The current check maps any raw_file_name == "linux.zip" to "alertmanager-webhook-adapter", which is too broad; update the conditional in the function that returns the component name (the block that currently checks raw_file_name.contains("PrometheusAlert") || raw_file_name == "linux.zip") so that the "linux.zip" branch is only taken for alertmanager artifacts (for example require raw_file_name.contains("alertmanager") or match specific names like "alertmanager-linux.zip"/"alertmanager-*.zip" instead of plain "linux.zip"), keeping the existing PrometheusAlert check and returning "alertmanager-webhook-adapter" only when the filename clearly identifies an alertmanager artifact.src/cluster_mgr/src/cli/task/upload/monitor_upload_builder.rs-30-37 (1)
30-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
remove_dir_alltarget to stay under upload root.
staged_dirincludescluster_name; if it contains traversal components, this can delete outside the intended upload workspace. Validate/canonicalize before removal.Also applies to: 55-63
🤖 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/monitor_upload_builder.rs` around lines 30 - 37, The deletion of staged_dir before recreating it is unsafe because staged_dir is built from upload_dir(), config.deployment.cluster_name, MONITOR_DIR and dir_name; canonicalize/normalize and verify that the resolved path is a descendant of the upload root before calling fs::remove_dir_all. Update the logic around staged_dir creation in monitor_upload_builder.rs (the block that constructs staged_dir and the later similar block at the 55-63 region) to canonicalize both upload_dir() and staged_dir (or use join then canonicalize), check that staged_dir.starts_with(canonicalized_upload_root) (or equivalent Path::starts_with) and only then call fs::remove_dir_all(&staged_dir) otherwise return an error; keep the subsequent fs::create_dir_all call unchanged once the safety check passes.src/cluster_mgr/src/cli/task/upload/upload_task_builder.rs-83-85 (1)
83-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid shell injection via unescaped
destin--rsync-path.
format!("mkdir -p {} && rsync", upload_file.dest.trim())is executed by a remote shell. Ifdestcontains shell metacharacters, command execution can be altered.Suggested hardening
- "--rsync-path".to_string(), - format!("mkdir -p {} && rsync", upload_file.dest.trim()), + "--rsync-path".to_string(), + format!("mkdir -p '{}' && rsync", upload_file.dest.trim().replace('\'', "'\\''")),Also reject unsafe destination patterns early (e.g., newline/control chars).
Also applies to: 96-101
🤖 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/upload_task_builder.rs` around lines 83 - 85, The "--rsync-path" argument currently interpolates upload_file.dest directly into a shell command (format!("mkdir -p {} && rsync", upload_file.dest.trim())), which allows shell injection; change this by either (a) performing the mkdir on the remote host separately (e.g., run ssh remote "mkdir -p -- <dest>" without embedding into --rsync-path) or (b) safely shell-quoting/escaping upload_file.dest before embedding (use a proven quoting helper like shlex::quote or implement single-quote escaping and reject inputs with control/newline chars). Update the code that builds "--rsync-path" and any similar occurrences (the other block at lines ~96-101) to validate/deny unsafe destination patterns (newline/control chars) and only pass a safely quoted dest into the rsync-path string or move the mkdir out of the rsync-path altogether.src/cluster_mgr/src/config/monitor.rs-538-601 (1)
538-601:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRoute can point to a missing Alertmanager receiver.
receiver_nameis decided from raw URL presence, but receiver creation depends on parsed tokens. With invalid URLs, route points toalertmanager_webhook_adapterthat is never defined.Suggested fix
- let receiver_name = if self - .alertmanager - .as_ref() - .and_then(|alertmanager| alertmanager.feishu_robot_urls.as_ref()) - .is_some_and(|urls| !urls.is_empty()) - { - "alertmanager_webhook_adapter" - } else { - "null" - }; + let mut receiver_name = "null"; @@ - if !webhook_entries.is_empty() { + if !webhook_entries.is_empty() { + receiver_name = "alertmanager_webhook_adapter"; let mut receiver = serde_yaml::Mapping::new(); @@ - if let Some(route) = alertmanager_config_map + if let Some(route) = alertmanager_config_map .get_mut("route") .and_then(|v| v.as_mapping_mut()) { route.insert( Value::String("receiver".to_string()), Value::String(receiver_name.to_string()), ); }🤖 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/monitor.rs` around lines 538 - 601, The route may reference "alertmanager_webhook_adapter" based on raw feishu_robot_urls even when no valid tokens are parsed, causing a missing receiver; compute the parsed webhook_entries/tokens first (using Self::extract_feishu_robot_token on feishu_robot_urls) and then set receiver_name to "alertmanager_webhook_adapter" only if webhook_entries is non-empty (otherwise "null"), and ensure the route insertion uses that computed receiver_name and that receivers is populated from the same webhook_entries variable (symbols: feishu_robot_urls, extract_feishu_robot_token, webhook_entries, receiver_name, receivers, route).tests/docker_ha/Dockerfile.stress_go-1-16 (1)
1-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun stress container as a non-root user.
This image currently runs as root, which increases risk and can leave root-owned files in the mounted repo.
🤖 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/docker_ha/Dockerfile.stress_go` around lines 1 - 16, The Dockerfile runs as root; add steps to create a non-root user and switch to it before the final CMD: create a group/user (e.g., groupadd/useradd or addgroup/adduser) with a fixed non-root UID/GID (commonly 1000), chown the WORKDIR (/workspace/eloq_waiter) and any needed runtime directories to that user, and add a USER directive to switch to that account so the container executes CMD ["sleep", "infinity"] as non-root; reference the WORKDIR and CMD lines when making these edits and ensure file ownership is updated so mounted repo files aren't left owned by root.tests/docker_ha/Dockerfile.stress-1-21 (1)
1-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun stress container as a non-root user.
This image runs as root by default, which can cause host-mounted workspace permission issues and weakens container isolation.
🤖 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/docker_ha/Dockerfile.stress` around lines 1 - 21, The Dockerfile currently runs as root; modify it to create and switch to a non-root user (e.g., create a user like stressuser), ensure ownership of the WORKDIR (/workspace/eloq_waiter) and any copied files (/tmp/cmd_stress_py_requirements.txt) is set to that user (chown), and add a USER instruction before CMD so the container runs unprivileged; locate the relevant Dockerfile directives (FROM, RUN, COPY, WORKDIR, CMD) and add a RUN that creates the user/group and adjusts ownership, then add USER stressuser.tests/docker_ha/Dockerfile.stress_ts-1-16 (1)
1-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun stress container as a non-root user.
Default root execution is avoidable here and can create permission drift on mounted files.
🤖 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/docker_ha/Dockerfile.stress_ts` around lines 1 - 16, The Dockerfile runs the stress container as root; modify it to create a non-root user (e.g., add a user/group like "eloq" or "appuser"), chown the WORKDIR (/workspace/eloq_waiter) to that user, and switch to that user with a USER instruction before the CMD so the container runs unprivileged; update the RUN sequence that sets up packages to also create the user and set permissions, and ensure WORKDIR and CMD remain correct for the new user.tests/docker_env.sh-82-87 (1)
82-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
compose_down()ignores compose override and can leave test resources behind.You added override support in
compose(), but cleanup bypasses it. WithCOMPOSE_OVERRIDE_FILE,downmay miss override-defined services/networks/volumes.Suggested fix
compose_down() { - if docker compose version >/dev/null 2>&1; then - timeout --kill-after=5s "${CLEANUP_TIMEOUT_SECONDS}s" docker compose -f "${DOCKER_E2E_DIR}/docker-compose.yaml" down -v --remove-orphans >/dev/null 2>&1 || true - else - timeout --kill-after=5s "${CLEANUP_TIMEOUT_SECONDS}s" docker-compose -f "${DOCKER_E2E_DIR}/docker-compose.yaml" down -v --remove-orphans >/dev/null 2>&1 || true - fi + timeout --kill-after=5s "${CLEANUP_TIMEOUT_SECONDS}s" \ + compose down -v --remove-orphans >/dev/null 2>&1 || true }🤖 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/docker_env.sh` around lines 82 - 87, compose_down() currently calls docker compose/docker-compose down with only the base "${DOCKER_E2E_DIR}/docker-compose.yaml", so any COMPOSE_OVERRIDE_FILE services/volumes/networks can be left running; modify compose_down() to mirror compose() by checking if COMPOSE_OVERRIDE_FILE is set/non-empty and, if so, add a second -f "${COMPOSE_OVERRIDE_FILE}" argument to the docker compose (and docker-compose) down invocation, preserving timeout and existing flags (down -v --remove-orphans) and behavior across both branches while keeping CLEANUP_TIMEOUT_SECONDS and DOCKER_E2E_DIR intact.tests/e2e/cmd_stress_py/main.py-56-60 (1)
56-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on invalid numeric args (
key-count,workers,inflight,repeat).Line 316 can raise
ZeroDivisionErrorwhen--key-countis0. Non-positive worker/inflight/repeat values should also be rejected up front.Proposed 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 or args.workers <= 0 or args.inflight <= 0 or args.repeat <= 0: + print( + "FAIL: --key-count, --workers, --inflight, and --repeat must all be > 0", + flush=True, + ) + sys.exit(1)Also applies to: 304-317
🤖 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 56 - 60, After parsing arguments (after args = parser.parse_args()), validate that numeric args key_count, workers, inflight, and repeat are positive integers (>0); if any are non-positive, print a clear FAIL message and sys.exit(1). This prevents the ZeroDivisionError at the calculation using key_count (referenced around line 316) and rejects invalid worker/inflight/repeat inputs up front—implement checks against args.key_count, args.workers, args.inflight, and args.repeat immediately after startup_node validation in main.tests/e2e/cmd_stress_go/main.go-565-578 (1)
565-578:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate CLI bounds before launching workers.
--key-count 0will panic at Line 577 (keyIdx % keyMod). Also,--startup-nodes ""passes the currentlen(addrs)check and fails later with a less clear error.Proposed fix
flag.Parse() + if workers <= 0 || inflight <= 0 || keyCount <= 0 || repeat <= 0 { + logger.Fatalf("invalid args: workers=%d inflight=%d key-count=%d repeat=%d (all must be > 0)", + workers, inflight, keyCount, repeat) + } addrs := strings.Split(startupNodes, ",") - for i := range addrs { - addrs[i] = strings.TrimSpace(addrs[i]) - } - if len(addrs) < 1 { - logger.Fatalf("need at least 1 startup node, got %q", startupNodes) - } + cleaned := make([]string, 0, len(addrs)) + for _, a := range addrs { + a = strings.TrimSpace(a) + if a != "" { + cleaned = append(cleaned, a) + } + } + if len(cleaned) < 1 { + logger.Fatalf("need at least 1 non-empty startup node, got %q", startupNodes) + } + addrs = cleanedAlso applies to: 616-642
🤖 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 565 - 578, The worker loop in stressWorker (and the similar block at lines 616-642) can panic when keyMod is zero and will later fail confusingly if startup nodes are empty; validate CLI bounds before launching workers by: ensure the key count / keyMod argument is > 0 (in the caller or before spawning stressWorker) and bail out with a clear error/log if it is zero, and verify startup nodes (the addrs slice) is non-empty and does not contain only an empty string before using it, returning/ cancelling early with a descriptive error message so the code never reaches the modulo or downstream failure points.tests/e2e/cmd_stress_ts/main.ts-19-20 (1)
19-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefault timeout/progress values are scaled twice.
Defaults are set to
"5000"but then multiplied by1000, so default behavior becomes ~5000 seconds instead of 5 seconds.Proposed fix
- "cmd-timeout": { type: "string", default: "5000" }, - "progress-interval": { type: "string", default: "5000" }, + "cmd-timeout": { type: "string", default: "5" }, + "progress-interval": { type: "string", default: "5" },Also applies to: 35-36
🤖 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 19 - 20, The defaults for the CLI options "cmd-timeout" and "progress-interval" are set to "5000" (strings) but later multiplied by 1000, producing ~5000 seconds instead of 5 seconds; update the default values to "5" (or 5) for both "cmd-timeout" and "progress-interval" in the options declaration in tests/e2e/cmd_stress_ts/main.ts (also apply the same change for the other occurrence at lines ~35-36), or alternatively remove the extra *1000 multiplication where the values are converted, so the final timeout/progress values represent seconds correctly. Ensure the option keys "cmd-timeout" and "progress-interval" and any variables that read them (e.g., cmdTimeout/progressInterval) are consistently treated as numeric seconds.tests/e2e/cmd_stress_ts/main.ts-326-339 (1)
326-339:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAbort early if master discovery fails.
If no master is parsed,
mkClient(masterAddr)receives an empty address and fails later with a less actionable error.Proposed fix
for (const line of nodesInfo.split("\n")) { const fields = line.trim().split(/\s+/); if (fields.length < 3) continue; const addr = fields[1].split("@")[0]; if (fields[2].includes("master") && !masterAddr) masterAddr = addr; if (fields[2].includes("slave") && !replicaAddr) replicaAddr = addr; } + if (!masterAddr) { + throw new Error("no master found in CLUSTER NODES"); + } console.log(P(`master=${masterAddr} replica=${replicaAddr}`));🤖 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 326 - 339, The code parses nodesInfo into masterAddr but doesn't abort if no master is found, causing mkClient(masterAddr) to be called with an empty address; after the loop that sets masterAddr and replicaAddr add an early check that masterAddr is non-empty and if it is, throw an error or call process.exit with a clear message (e.g., "failed to discover master from cluster nodes"); keep the existing mkClient(masterAddr) and replica logic but ensure the check prevents mkClient from getting an empty masterAddr and references the variables masterAddr, replicaAddr, mkClient, and nodesInfo to locate the code to change.tests/e2e/cmd_stress_test.sh-195-200 (1)
195-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
TLS_ENABLEDin topology discovery.
discover_masteralways creates a TLS Redis client. WithTLS_ENABLED=0, this path can fail even when the cluster is healthy.Proposed fix
nodes_info=$(docker compose -f "${DOCKER_E2E_DIR}/docker-compose.yaml" exec -T stress-python python3 -c " import ssl -TLS={'ssl':True,'ssl_cert_reqs':ssl.CERT_NONE,'ssl_check_hostname':False} from redis import Redis -r=Redis(host='${N1}',port=6379,password='${PASSWD}',socket_timeout=5,**TLS) +tls_enabled='${TLS_ENABLED}' +kwargs = {'ssl': True, 'ssl_cert_reqs': ssl.CERT_NONE, 'ssl_check_hostname': False} if tls_enabled == '1' else {} +r=Redis(host='${N1}',port=6379,password='${PASSWD}',socket_timeout=5,**kwargs) print(r.execute_command('CLUSTER','NODES').decode()) r.close() " 2>/dev/null) || { echo "FAIL: cannot connect to cluster for discovery"; return 1; }🤖 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 195 - 200, discover_master currently always builds a TLS Redis client (the TLS dict and passing **TLS to Redis), ignoring the TLS_ENABLED flag; change the logic in discover_master so that it checks TLS_ENABLED and only constructs/passes the TLS-related dict (TLS={'ssl':True,...}) to Redis when TLS_ENABLED is set (non-zero/true), otherwise create the Redis client without TLS kwargs; ensure the code paths that reference TLS, the TLS variable, and the Redis(...) instantiation are guarded by that check so topology discovery works for both TLS_ENABLED=0 and TLS-enabled clusters.tests/e2e/devctl.sh-62-66 (1)
62-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden
control_exec bash -lcinterpolation intests/e2e/devctl.sh
tests/e2e/devctl.shinjects runtime${cluster}/${url}directly into the remotebash -lc "..."string (lines 62-66, 72-75, 82-84). If either value contains quotes/metacharacters, this can break parsing or enable unintended command execution.export_topologyalso interpolates${cluster}into/home/eloq/${cluster}-export.yamlwithout quoting (around lines 91-92).Suggested hardening pattern
- control_exec bash -lc " - eloqctl monitor update --cluster '${cluster}' --component grafana --url '${url}' - " + control_exec bash -lc ' + cluster="$1" + url="$2" + eloqctl monitor update --cluster "$cluster" --component grafana --url "$url" + ' _ "$cluster" "$url"🤖 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/devctl.sh` around lines 62 - 66, The remote shell invocations in tests/e2e/devctl.sh (the control_exec bash -lc "..." block that calls eloqctl stop/remove/launch and the surrounding occurrences) and the export_topology function currently inline ${cluster}/${url} unquoted, which can break or allow injection; fix by shell-escaping or robustly quoting these values before building the remote command (e.g. use printf '%q' or wrap values in single quotes while escaping embedded single quotes) and pass them as environment variables into control_exec rather than direct interpolation, and update export_topology to quote the filename path /home/eloq/${cluster}-export.yaml (escape ${cluster} the same way) so all uses of cluster/url in control_exec and export_topology are safe against quotes and shell metacharacters.
🟡 Minor comments (12)
doc/upgrade.md-60-60 (1)
60-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign monitor-update command syntax with the command reference.
This page uses
--cluster <cluster>, whiledoc/cluster_mgr.mdshows positional<cluster>. Please standardize to one canonical form across docs to avoid copy/paste failures.Also applies to: 66-66
🤖 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 `@doc/upgrade.md` at line 60, The docs use a flag form for the cluster in the monitor-update example but the command reference uses a positional parameter; update the example command(s) to match the canonical positional syntax by replacing occurrences of "--cluster <cluster>" with the positional "<cluster>" in the `eloqctl monitor update` examples (e.g., change `eloqctl monitor update --cluster <cluster> --component grafana` to `eloqctl monitor update <cluster> --component grafana`) so both instances (line 60 and the one at 66) are consistent with doc/cluster_mgr.md.doc/cluster_mgr.md-57-57 (1)
57-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape pipe characters in command cell to keep table valid.
monitor start|stop|status <cluster>should escape|in table content; otherwise the row renders with extra columns.Suggested fix
-| `monitor start|stop|status <cluster>` | Manage Prometheus, Grafana, and node exporter when monitor config is present. | +| `monitor start\|stop\|status <cluster>` | Manage Prometheus, Grafana, and node exporter when monitor config is present. |🤖 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 `@doc/cluster_mgr.md` at line 57, The table row containing the command label "monitor start|stop|status <cluster>" uses unescaped pipe characters which break the Markdown table; update that cell to escape the pipe characters (e.g., replace "|" with "\|") so the cell renders as a single column. Locate the table row that currently contains the literal monitor start|stop|status <cluster> string and change it to use escaped pipes (monitor start\|stop\|status <cluster>) to keep the table valid.src/cluster_mgr/config/alert.rules-127-134 (1)
127-134:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix threshold mismatch in alert description.
Line 134 says “exceeded 1000” but Line 128 triggers at
> 100. This will mislead on-call triage.Suggested fix
- 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 }}"🤖 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 127 - 134, The alert eloqkv_active_connections_too_high has a description that says “exceeded 1000” but the expr triggers at > 100; update the description (and summary if needed) to match the actual threshold (e.g., change “exceeded 1000” to “exceeded 100”) or alternatively change the expr to > 1000 if the intended threshold is 1000; locate the alert by name eloqkv_active_connections_too_high and make the description and any annotations consistent with the expr value.src/cluster_mgr/src/cli/task/group/db_cluster_ctrl_group.rs-279-287 (1)
279-287:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace
expect()with proper error propagation.Using
expect("stop nodes error")can cause a panic if the task generation fails. Since this function returns a tuple (notResult), consider either propagating the error by changing the return type or logging and skipping the failed tasks gracefully.Proposed fix
- let stop_nodes = EloqTxCtlTask::from_config_with_channel( - cmd.clone(), - config, - ServerType::Node, - None, - ) - .expect("stop nodes error"); - barrier.push(stop_nodes.len()); - executable.extend(stop_nodes); + match EloqTxCtlTask::from_config_with_channel( + cmd.clone(), + config, + ServerType::Node, + None, + ) { + Ok(stop_nodes) => { + barrier.push(stop_nodes.len()); + executable.extend(stop_nodes); + } + Err(err) => { + warn!("Failed to generate stop tasks for nodes: {}", err); + } + }🤖 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/group/db_cluster_ctrl_group.rs` around lines 279 - 287, Replace the panicking expect call on EloqTxCtlTask::from_config_with_channel with proper error handling: call EloqTxCtlTask::from_config_with_channel(cmd.clone(), config, ServerType::Node, None) and match its result — on Ok(stop_nodes) proceed with barrier.push(stop_nodes.len()) and executable.extend(stop_nodes); on Err(e) either return Err(e) from the enclosing function (update its signature to a Result) or log the error and skip adding tasks (so the code continues without panicking). Ensure you reference the same symbols (EloqTxCtlTask::from_config_with_channel, stop_nodes, barrier.push, executable.extend) when making the change.src/cluster_mgr/src/cli/task/local_extract_task.rs-96-111 (1)
96-111:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
unwrap()oncache_dir()may panic.Lines 99 and 106 call
url.cache_dir().unwrap()which could panic ifcache_dir()returnsNone. Consider usingok_or_elseto return a proper error.Suggested fix
pub fn cached_archive_path(url: &DownloadUrl) -> PathBuf { match url { DownloadUrl::Local(_) => download_dir().join(url.file_name()), - DownloadUrl::Remote(_) => PathBuf::from(url.cache_dir().unwrap()).join(url.file_name()), + DownloadUrl::Remote(_) => { + let cache = url.cache_dir().unwrap_or_else(|| download_dir().to_string_lossy().to_string()); + PathBuf::from(cache).join(url.file_name()) + } } }Or return
Result<PathBuf>to allow proper error propagation.🤖 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/local_extract_task.rs` around lines 96 - 111, Both cached_archive_path and extract_root_for call url.cache_dir().unwrap() which can panic; change them to handle the None case instead of unwrapping. Update cached_archive_path and extract_root_for to return Result<PathBuf, E> (or propagate an existing error type) and use url.cache_dir().ok_or_else(|| /* construct error */)? to convert None into an Err; then build the path with PathBuf::from(cache_dir) when DownloadUrl::Remote, keeping DownloadUrl::Local using download_dir(), and continue to use Self::archive_stem(&url.file_name()) in extract_root_for.src/cluster_mgr/src/cli/task/group/monitor_ctl_group.rs-69-74 (1)
69-74:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider handling
Nonecluster gracefully instead of panicking.The
expect("monitor cluster is resolved")on Line 71 will panic ifclusterisNone. While the comment indicates an expectation that the caller resolves the cluster beforehand, a panic here could crash the application if this invariant is ever violated by a code path change.Suggested improvement
- let cluster_name = match &cmd_arg { - SubCommand::Monitor { cluster, .. } => { - cluster.clone().expect("monitor cluster is resolved") - } - _ => unreachable!(), - }; + let cluster_name = match &cmd_arg { + SubCommand::Monitor { cluster, .. } => cluster + .clone() + .ok_or_else(|| anyhow::anyhow!("Monitor command requires cluster to be specified"))?, + _ => unreachable!(), + };🤖 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/group/monitor_ctl_group.rs` around lines 69 - 74, The current match on cmd_arg uses SubCommand::Monitor { cluster, .. } and calls cluster.clone().expect("monitor cluster is resolved"), which will panic if cluster is None; change this to handle None gracefully by matching the Option and returning an error or printing a user-friendly message instead of panicking—e.g., when cluster is Some(name) assign to cluster_name, and when None return a Result::Err or call the CLI error/exit path used elsewhere in this module so the function (where cmd_arg is inspected) does not unwind the whole process.src/cluster_mgr/src/cli/task/upload/monitor_upload_builder.rs-305-307 (1)
305-307:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd doc comments for new public API.
pub fn build_for_cmdshould be documented with///to describe intent and expectedcmdvalues.As per coding guidelines, "Document public API with doc comments (///)".
🤖 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/monitor_upload_builder.rs` around lines 305 - 307, Add a doc comment for the public function MonitorInfraConfUploadBuilder::build_for_cmd describing its purpose, the expected values/format of the cmd parameter, and the return (IndexMap<TaskId, TaskInstance>), using triple-slash (///) style comments placed immediately above the fn declaration so it becomes part of the public API documentation.src/cluster_mgr/src/config/deployment.rs-1491-1495 (1)
1491-1495:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
event_dispatcher_numto avoid zero dispatcher threads.
hw.event_dispatcher_num = Some(0)currently writes0to config, which is not a valid runtime setting for dispatcher threads.Suggested fix
let key = "event_dispatcher_num"; let val = set_by_user!(ini.get(SECTION_LOCAL, key), u16); if val.is_none() { - let core_io = hw.event_dispatcher_num.unwrap_or(core_tx.div_ceil(8)); + let core_io = hw + .event_dispatcher_num + .filter(|v| *v > 0) + .unwrap_or_else(|| core_tx.div_ceil(8).max(1)); ini.set(SECTION_LOCAL, key, Some(core_io.to_string())); }🤖 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/deployment.rs` around lines 1491 - 1495, Validate and reject zero for event_dispatcher_num before writing it: when reading hw.event_dispatcher_num (the value used to populate key "event_dispatcher_num" via val and ini.set), treat Some(0) as invalid and fall back to the computed default (core_io = hw.event_dispatcher_num.unwrap_or(core_tx.div_ceil(8)) behavior) instead of writing "0" into ini; i.e., if hw.event_dispatcher_num == Some(0) ignore it and use core_io.to_string() when calling ini.set(SECTION_LOCAL, key, ...).tests/README.md-10-10 (1)
10-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace machine-specific absolute paths in docs.
The hardcoded
/home/starrysky/...paths are not portable and will fail for other contributors. Please switch to repo-relative instructions (for examplecd <repo-root>).Also applies to: 59-59
🤖 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 string "/home/starrysky/workspace/eloqdata-kernel/eloq_waiter" in tests/README.md with a repo-relative instruction such as "cd <repo-root>/eloq_waiter" or simply "cd eloq_waiter"; update the other occurrence noted (line 59) similarly so all documentation uses portable, relative paths instead of hardcoded user home paths.tests/docker_env.sh-141-144 (1)
141-144:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape sed replacement value before injecting
ELOQCTL_FEISHU_ROBOT_URL.Raw URL interpolation can corrupt output when the value contains
&(or delimiter-sensitive chars), which is a realistic webhook URL case.Suggested fix
render_topology_for_control() { local source_topology="$1" local rendered_topology="$2" + local escaped_feishu_url + escaped_feishu_url=$(printf '%s' "${ELOQCTL_FEISHU_ROBOT_URL}" | sed 's/[&|]/\\&/g') sed -e "s|\${ELOQCTL_DOCKER_SSH_KEY}|${ELOQCTL_CONTROL_SSH_KEY}|g" \ -e "s|\${ELOQKV_VERSION}|${ELOQKV_VERSION}|g" \ - -e "s|\${ELOQCTL_FEISHU_ROBOT_URL}|${ELOQCTL_FEISHU_ROBOT_URL}|g" \ + -e "s|\${ELOQCTL_FEISHU_ROBOT_URL}|${escaped_feishu_url}|g" \ "${source_topology}" > "${rendered_topology}" }🤖 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/docker_env.sh` around lines 141 - 144, The sed replacement injects ELOQCTL_FEISHU_ROBOT_URL raw, which will break when the URL contains & or delimiter-sensitive chars; before running sed, create an escaped version (e.g. ELOQCTL_FEISHU_ROBOT_URL_ESC) that escapes ampersands and any chosen sed delimiter characters, then use that escaped variable in the sed -e "s|...|${ELOQCTL_FEISHU_ROBOT_URL_ESC}|g" replacement (keep using source_topology and rendered_topology as targets); this ensures special chars in ELOQCTL_FEISHU_ROBOT_URL do not corrupt the rendered_topology.tests/e2e/devctl.sh-33-39 (1)
33-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
CLUSTER_NAMEoverride is only partially applied.
stop/remove/status/exportusecluster_name(), butlaunchrelies on topology content. IfCLUSTER_NAMEis overridden, commands can target different clusters.Also applies to: 59-66
🤖 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/devctl.sh` around lines 33 - 39, The CLUSTER_NAME override is only applied by cluster_name() but not when rendering topology for launch, causing launch to target a different cluster; update the topology rendering and the launch flow to use the same cluster_name() helper (or read ${CLUSTER_NAME:-${DEFAULT_CLUSTER}}) so all commands (launch, stop, remove, status, export) reference the identical cluster name; locate the topology rendering logic (the function or code that builds the topology used by launch) and replace hardcoded/default usage with a call to cluster_name() or the same expansion so overrides are honored everywhere.tests/install_control_eloqctl.sh-64-68 (1)
64-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
INSTALL_ROOTwhen generating theeloqctlwrapper.The wrapper currently hardcodes
/home/eloq/.eloqctl/bin/cluster_mgr, which can diverge fromCONTROL_ELOQCTL_HOME.Proposed fix
- printf '%s\n' '#!/bin/bash' \ - 'export HOME=/home/eloq' \ - 'export ELOQCTL_HOME=/home/eloq/.eloqctl' \ - 'exec /home/eloq/.eloqctl/bin/cluster_mgr "\$@"' \ + printf '%s\n' '#!/bin/bash' \ + 'export HOME=/home/eloq' \ + 'export ELOQCTL_HOME='"'${INSTALL_ROOT}'" \ + 'exec '"${INSTALL_ROOT}"'/bin/cluster_mgr "\$@"' \ > /usr/local/bin/eloqctl🤖 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/install_control_eloqctl.sh` around lines 64 - 68, The eloqctl wrapper currently hardcodes the path '/home/eloq/.eloqctl/bin/cluster_mgr'; change the printf block that writes /usr/local/bin/eloqctl to use the INSTALL_ROOT (or CONTROL_ELOQCTL_HOME) variable when constructing the exec line so the wrapper uses "${INSTALL_ROOT}/.eloqctl/bin/cluster_mgr" or "${CONTROL_ELOQCTL_HOME}/bin/cluster_mgr" (and preserve the existing exported HOME/ELOQCTL_HOME lines and quoting for "$@") to avoid divergence from CONTROL_ELOQCTL_HOME.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: faa35615-d3e3-4508-9b8d-32b308fe2456
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktests/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 (81)
.githooks/pre-push.github/workflows/quality.yml.github/workflows/test.yml.gitignoreREADME.mddoc/cluster_mgr.mddoc/upgrade.mdinstall.shscripts/test-before-push.shsrc/cluster_mgr/Cargo.tomlsrc/cluster_mgr/config/alert.rulessrc/cluster_mgr/config/feishu.zh.tmplsrc/cluster_mgr/config/grafana_config_v13.inisrc/cluster_mgr/config/mono_alertmanager.yamlsrc/cluster_mgr/config/mono_prometheus.yamlsrc/cluster_mgr/config/runtime_deps_ubuntusrc/cluster_mgr/src/cli/cmd_base.rssrc/cluster_mgr/src/cli/mod.rssrc/cluster_mgr/src/cli/reconcile.rssrc/cluster_mgr/src/cli/task/check_task.rssrc/cluster_mgr/src/cli/task/download_task.rssrc/cluster_mgr/src/cli/task/eloq_tx_ctl_task.rssrc/cluster_mgr/src/cli/task/group/backup_group.rssrc/cluster_mgr/src/cli/task/group/check_group.rssrc/cluster_mgr/src/cli/task/group/db_cluster_ctrl_group.rssrc/cluster_mgr/src/cli/task/group/deployment_group.rssrc/cluster_mgr/src/cli/task/group/launch_group.rssrc/cluster_mgr/src/cli/task/group/monitor_ctl_group.rssrc/cluster_mgr/src/cli/task/group/remove_group.rssrc/cluster_mgr/src/cli/task/group/scale_group.rssrc/cluster_mgr/src/cli/task/group/scale_log_group.rssrc/cluster_mgr/src/cli/task/local_extract_task.rssrc/cluster_mgr/src/cli/task/mod.rssrc/cluster_mgr/src/cli/task/monitor_ctl_task.rssrc/cluster_mgr/src/cli/task/redis_op_task.rssrc/cluster_mgr/src/cli/task/rolling_upgrade/mod.rssrc/cluster_mgr/src/cli/task/rolling_upgrade/steps.rssrc/cluster_mgr/src/cli/task/task_base.rssrc/cluster_mgr/src/cli/task/task_controller.rssrc/cluster_mgr/src/cli/task/task_utils.rssrc/cluster_mgr/src/cli/task/unpack_file_task.rssrc/cluster_mgr/src/cli/task/upload/data_dir_upload_builder.rssrc/cluster_mgr/src/cli/task/upload/eloq_upload_builder.rssrc/cluster_mgr/src/cli/task/upload/mod.rssrc/cluster_mgr/src/cli/task/upload/monitor_upload_builder.rssrc/cluster_mgr/src/cli/task/upload/proxy_upload_builder.rssrc/cluster_mgr/src/cli/task/upload/tx_conf_upload_builder.rssrc/cluster_mgr/src/cli/task/upload/upload_task.rssrc/cluster_mgr/src/cli/task/upload/upload_task_builder.rssrc/cluster_mgr/src/cli/task/wait_replica_ready_task.rssrc/cluster_mgr/src/config/config_base.rssrc/cluster_mgr/src/config/deployment.rssrc/cluster_mgr/src/config/mod.rssrc/cluster_mgr/src/config/monitor.rssrc/cluster_mgr/src/github_release.rssrc/cluster_mgr/src/lib.rssrc/cluster_mgr/src/state/state_mgr.rssrc/rest_api/src/handler.rstests/README.mdtests/docker_env.shtests/docker_ha/Dockerfiletests/docker_ha/Dockerfile.stresstests/docker_ha/Dockerfile.stress_gotests/docker_ha/Dockerfile.stress_tstests/docker_ha/docker-compose.yamltests/docker_ha/topology.yamltests/e2e/cmd_stress_go/cmd_stress_gotests/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/devctl.shtests/e2e/stress.pytests/e2e/stress_test.shtests/e2e/test.shtests/e2e/topology.yamltests/install_control_eloqctl.sh
💤 Files with no reviewable changes (5)
- tests/docker_ha/topology.yaml
- .github/workflows/quality.yml
- tests/e2e/stress.py
- tests/e2e/stress_test.sh
- tests/e2e/test.sh
Comment on lines
+407
to
413
| let staged_dir = LocalExtractTask::staged_dir_for(&tx_download_url, &tx_home); | ||
| if !staged_dir.exists() { | ||
| bail!( | ||
| "Unable to determine TX service image filename from {}", | ||
| effective_tx_image_url | ||
| "TX service staged directory not found: {}", | ||
| staged_dir.display() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Premature directory existence check will always fail on fresh runs.
The staged_dir.exists() check at Line 408 runs during task graph construction, before the DownloadTask and LocalExtractTask have executed. On a fresh run where the archive hasn't been downloaded/extracted yet, this check will fail and bail out even though the preceding tasks would have created the directory.
This check should either be removed (trusting the preceding tasks to succeed), or the verification should be moved into a separate task that executes after extraction.
Suggested fix: Remove the premature check
if !extract_tasks.is_empty() {
barrier.push(extract_tasks.len());
executable.extend(extract_tasks);
}
- let staged_dir = LocalExtractTask::staged_dir_for(&tx_download_url, &tx_home);
- if !staged_dir.exists() {
- bail!(
- "TX service staged directory not found: {}",
- staged_dir.display()
- );
- }
+ let staged_dir = LocalExtractTask::staged_dir_for(&tx_download_url, &tx_home);
info!(The upload task will naturally fail if the source directory doesn't exist after extraction.
📝 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
| let staged_dir = LocalExtractTask::staged_dir_for(&tx_download_url, &tx_home); | |
| if !staged_dir.exists() { | |
| bail!( | |
| "Unable to determine TX service image filename from {}", | |
| effective_tx_image_url | |
| "TX service staged directory not found: {}", | |
| staged_dir.display() | |
| ); | |
| } | |
| let staged_dir = LocalExtractTask::staged_dir_for(&tx_download_url, &tx_home); |
🤖 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/group/scale_group.rs` around lines 407 - 413,
The staged_dir existence check is premature because it runs during graph
construction before DownloadTask/LocalExtractTask execute; remove the call to
staged_dir.exists() and the bail! that references
LocalExtractTask::staged_dir_for(&tx_download_url, &tx_home), and instead rely
on the upload task to fail if the directory is missing at runtime (or, if
stronger verification is required, move that check into a new follow-up
verification task that depends on DownloadTask and LocalExtractTask completing).
Ensure the upload task still depends on DownloadTask and LocalExtractTask so
extraction happens before any upload attempt.
Comment on lines
+280
to
+287
| let staged_dir = | ||
| LocalExtractTask::staged_dir_for(&log_download_url, LOG_SERVICE_HOME); | ||
| if !staged_dir.exists() { | ||
| return Err(anyhow!( | ||
| "Log service staged directory not found: {}", | ||
| staged_dir.display() | ||
| )); | ||
| } |
There was a problem hiding this comment.
Premature directory existence check will always fail on fresh runs.
Same issue as in scale_group.rs: the staged_dir.exists() check at Line 282 runs during task graph construction, before DownloadTask and LocalExtractTask have executed. This will cause the command to fail on systems where the archive hasn't been previously downloaded and extracted.
Suggested fix: Remove the premature check
if !extract_tasks.is_empty() {
barrier.push(extract_tasks.len());
executable.extend(extract_tasks);
}
- let staged_dir =
- LocalExtractTask::staged_dir_for(&log_download_url, LOG_SERVICE_HOME);
- if !staged_dir.exists() {
- return Err(anyhow!(
- "Log service staged directory not found: {}",
- staged_dir.display()
- ));
- }
+ let staged_dir =
+ LocalExtractTask::staged_dir_for(&log_download_url, LOG_SERVICE_HOME);The upload task will fail with a clear error if the source doesn't exist.
📝 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
| let staged_dir = | |
| LocalExtractTask::staged_dir_for(&log_download_url, LOG_SERVICE_HOME); | |
| if !staged_dir.exists() { | |
| return Err(anyhow!( | |
| "Log service staged directory not found: {}", | |
| staged_dir.display() | |
| )); | |
| } | |
| let staged_dir = | |
| LocalExtractTask::staged_dir_for(&log_download_url, LOG_SERVICE_HOME); |
🤖 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/group/scale_log_group.rs` around lines 280 -
287, Remove the premature filesystem existence check that runs during task graph
construction: delete the staged_dir.exists() conditional (the block that returns
Err with "Log service staged directory not found") so that
LocalExtractTask::staged_dir_for(&log_download_url, LOG_SERVICE_HOME) is not
validated at construction time; let the downstream DownloadTask/LocalExtractTask
(and the upload task) perform the actual extraction/download and surface any
errors at runtime instead of failing early during graph build.
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
Verification
Summary by CodeRabbit
Release Notes
New Features
--download-onlymode for rolling cluster upgradesBug Fixes
Documentation