Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,20 @@ jobs:
exit 1
fi

supply-chain:
name: Supply Chain Security
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v6
- uses: dtolnay/rust-toolchain@1.94.1
- uses: Swatinem/rust-cache@v2
- name: Install cargo-deny and cargo-audit
run: cargo install cargo-deny cargo-audit
- name: cargo deny check
run: cargo deny check
- name: cargo audit
run: cargo audit

msrv:
name: MSRV (1.94)
runs-on: ubuntu-latest
Expand Down
20 changes: 18 additions & 2 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ jobs:
features: runtime-monoio,jemalloc
binary: moon-linux-monoio

- name: linux-aarch64-tokio
os: ubuntu-latest
target: aarch64-unknown-linux-gnu
features: runtime-tokio,jemalloc
binary: moon-linux-aarch64-tokio
cross: true

- name: macos-tokio
os: macos-latest
target: aarch64-apple-darwin
Expand All @@ -42,9 +49,17 @@ jobs:

- uses: Swatinem/rust-cache@v2

- name: Install cross (aarch64)
if: matrix.cross
run: cargo install cross --locked

- name: Build
run: |
cargo build --release --no-default-features --features ${{ matrix.features }} --target ${{ matrix.target }}
if [ "${{ matrix.cross }}" = "true" ]; then
cross build --release --no-default-features --features ${{ matrix.features }} --target ${{ matrix.target }}
else
cargo build --release --no-default-features --features ${{ matrix.features }} --target ${{ matrix.target }}
fi
cp target/${{ matrix.target }}/release/moon ${{ matrix.binary }}

- name: Upload artifact
Expand Down Expand Up @@ -87,7 +102,7 @@ jobs:
- name: Generate checksums
run: |
cd artifacts
sha256sum moon-linux-tokio moon-linux-monoio moon-macos-tokio moon-sbom.json moon-sbom-tokio.json moon-sbom-monoio.json > SHA256SUMS.txt
sha256sum moon-linux-tokio moon-linux-monoio moon-linux-aarch64-tokio moon-macos-tokio moon-sbom.json moon-sbom-tokio.json moon-sbom-monoio.json > SHA256SUMS.txt
cat SHA256SUMS.txt

- name: Install cosign
Expand All @@ -111,6 +126,7 @@ jobs:
--generate-notes \
artifacts/moon-linux-tokio \
artifacts/moon-linux-monoio \
artifacts/moon-linux-aarch64-tokio \
artifacts/moon-macos-tokio \
artifacts/moon-sbom.json \
artifacts/moon-sbom-tokio.json \
Expand Down
12 changes: 11 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,12 +1,22 @@
# Changelog

All notable changes to this project will be documented in this file.

The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

### Fixed — Wave 0-4 Gap Closure (2026-04-09)

- **ZREVRANGEBYSCORE/ZREVRANGEBYLEX correctness bug:** Fixed double-swap of min/max bounds in `zrange_by_score` and `zrange_by_lex` that caused empty results for finite score ranges (e.g., `ZREVRANGEBYSCORE key 3 1`). Added finite-range test to `test-commands.sh`.
- **INFO command enriched:** Clients section now reports `connected_clients`, Memory section reports `used_memory`/`used_memory_human`/`used_memory_rss` (from `/proc/self/status`), Replication section wired to actual `ReplicationState` (role, connected_slaves, master_replid, master_repl_offset).
- **Tracing spans:** Added `#[instrument]` to connection handlers (single, monoio), replication master (tokio, monoio), HNSW compaction, and AOF rewrite — 6 new spans.
- **Replication lag metric wired:** `moon_replication_lag_bytes` Prometheus gauge now updated from `get_replication_info()`.
- **CI supply chain security:** `cargo deny check` + `cargo audit` added to CI pipeline (deny.toml was previously unenforced).
- **Release pipeline:** aarch64-unknown-linux-gnu build added via `cross` for primary production target.
- **Crash matrix expanded:** BGSAVE and BGREWRITEAOF crash cells added (6/7 coverage).
- **Compatibility tests expanded:** Stream (XADD/XLEN/XRANGE/XTRIM), Lua scripting (EVAL/EVALSHA/SCRIPT), and ACL (WHOAMI/LIST) tests added to `redis_compat.rs`.

### Added — Production Contract (Phase 87, 2026-04-08)

- **`docs/PRODUCTION-CONTRACT.md`** — Moon's v1.0 promises: per-command-class SLOs (provisional until Phase 97), supported platform matrix (Linux aarch64 primary, Linux x86_64 secondary contingent on `PERF-04`, macOS dev-only via OrbStack), durability mode semantics per `appendfsync` × failure-class, availability & replication guarantees, security guarantees, explicit out-of-scope list, and a machine-checkable GA Exit Criteria checklist that every v0.1.3 phase ticks off. This is the contract every downstream hardening phase (88–100) tests against.
Expand Down
3 changes: 1 addition & 2 deletions scripts/test-commands.sh
Original file line number Diff line number Diff line change
Expand Up @@ -492,9 +492,8 @@ if should_run "sorted_set"; then
# ZREVRANGEBYSCORE on clean key (ZPOPMIN/ZPOPMAX above mutated z:k1)
rcli ZADD z:revtest 1 alpha 2 beta 3 gamma >/dev/null 2>&1; mcli ZADD z:revtest 1 alpha 2 beta 3 gamma >/dev/null 2>&1
assert_match "ZRANGEBYSCORE 2" ZRANGEBYSCORE z:revtest 1 3
# ZREVRANGEBYSCORE: known issue — moon returns empty for rev range queries
# TODO: investigate src/command/sorted_set.rs zrevrangebyscore implementation
assert_match "ZREVRANGEBYSCORE" ZREVRANGEBYSCORE z:revtest +inf -inf
assert_match "ZREVRANGEBYSCORE 2" ZREVRANGEBYSCORE z:revtest 3 1
assert_match "ZCOUNT" ZCOUNT z:k1 2 5
assert_match "ZINCRBY" ZINCRBY z:k1 100 b
assert_match "ZREM" ZREM z:k1 e
Expand Down
83 changes: 83 additions & 0 deletions src/admin/metrics_setup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ pub fn is_server_ready() -> bool {
// (admin_port=0), so INFO always returns meaningful stats.
static TOTAL_COMMANDS: AtomicU64 = AtomicU64::new(0);
static TOTAL_CONNECTIONS: AtomicU64 = AtomicU64::new(0);
static CONNECTED_CLIENTS: AtomicU64 = AtomicU64::new(0);

/// Initialize the Prometheus metrics exporter and admin HTTP server.
///
Expand Down Expand Up @@ -340,6 +341,7 @@ pub fn record_command_error(cmd: &str) {
#[inline]
pub fn record_connection_opened() {
TOTAL_CONNECTIONS.fetch_add(1, Ordering::Relaxed);
CONNECTED_CLIENTS.fetch_add(1, Ordering::Relaxed);
if !METRICS_INITIALIZED.load(Ordering::Relaxed) {
return;
}
Expand All @@ -350,12 +352,19 @@ pub fn record_connection_opened() {
/// Record a client disconnection.
#[inline]
pub fn record_connection_closed() {
CONNECTED_CLIENTS.fetch_sub(1, Ordering::Relaxed);
if !METRICS_INITIALIZED.load(Ordering::Relaxed) {
return;
}
gauge!("moon_connected_clients").decrement(1.0);
Comment on lines 354 to 359
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Guard the connected-client count against unmatched closes.

Line 355 unconditionally decrements both the atomic state and the Prometheus gauge. If a close path fires twice or after a partially-opened connection, CONNECTED_CLIENTS wraps to u64::MAX and the gauge can go negative.

Suggested fix
 pub fn record_connection_closed() {
-    CONNECTED_CLIENTS.fetch_sub(1, Ordering::Relaxed);
+    let prev = CONNECTED_CLIENTS
+        .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |n| {
+            Some(n.saturating_sub(1))
+        })
+        .unwrap_or(0);
+    if prev == 0 {
+        return;
+    }
     if !METRICS_INITIALIZED.load(Ordering::Relaxed) {
         return;
     }
     gauge!("moon_connected_clients").decrement(1.0);
 }
📝 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
pub fn record_connection_closed() {
CONNECTED_CLIENTS.fetch_sub(1, Ordering::Relaxed);
if !METRICS_INITIALIZED.load(Ordering::Relaxed) {
return;
}
gauge!("moon_connected_clients").decrement(1.0);
pub fn record_connection_closed() {
let prev = CONNECTED_CLIENTS
.fetch_update(Ordering::Relaxed, Ordering::Relaxed, |n| {
Some(n.saturating_sub(1))
})
.unwrap_or(0);
if prev == 0 {
return;
}
if !METRICS_INITIALIZED.load(Ordering::Relaxed) {
return;
}
gauge!("moon_connected_clients").decrement(1.0);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/admin/metrics_setup.rs` around lines 354 - 359, record_connection_closed
currently unconditionally decrements CONNECTED_CLIENTS and the
"moon_connected_clients" gauge, which can underflow if called when count is
zero; change record_connection_closed to atomically decrement CONNECTED_CLIENTS
only when its current value is > 0 (use fetch_update or a compare_exchange loop
on CONNECTED_CLIENTS) and only call
gauge!("moon_connected_clients").decrement(...) if the atomic decrement actually
occurred and METRICS_INITIALIZED.load(...) is true, ensuring the atomic and
Prometheus gauge stay in sync and never go negative; reference
record_connection_closed, CONNECTED_CLIENTS, METRICS_INITIALIZED, and the
"moon_connected_clients" gauge in the fix.

}

/// Current number of connected clients (for INFO command).
#[inline]
pub fn connected_clients() -> u64 {
CONNECTED_CLIENTS.load(Ordering::Relaxed)
}

// ── Keyspace metrics ────────────────────────────────────────────────────

/// Record keyspace hit/miss.
Expand Down Expand Up @@ -464,6 +473,32 @@ pub fn update_rss_bytes(rss: u64) {
gauge!("moon_rss_bytes").set(rss as f64);
}

// ── Memory helpers ──────────────────────────────────────────────────────

/// Read process RSS from /proc/self/status (Linux only).
/// Returns bytes, or 0 on failure / non-Linux.
#[cfg(target_os = "linux")]
pub fn get_rss_bytes() -> u64 {
if let Ok(status) = std::fs::read_to_string("/proc/self/status") {
for line in status.lines() {
if let Some(rest) = line.strip_prefix("VmRSS:") {
let trimmed = rest.trim();
if let Some(kb_str) = trimmed.strip_suffix(" kB") {
if let Ok(kb) = kb_str.trim().parse::<u64>() {
return kb * 1024;
}
}
}
}
}
0
}

#[cfg(not(target_os = "linux"))]
pub fn get_rss_bytes() -> u64 {
0
}

// ── INFO helpers ────────────────────────────────────────────────────────

/// Total commands processed since server start (for INFO Stats).
Expand Down Expand Up @@ -506,6 +541,54 @@ pub fn get_cpu_usage() -> (f64, f64) {
(0.0, 0.0)
}

// ── Global replication state (for INFO) ────────────────────────────────

static GLOBAL_REPL_STATE: once_cell::sync::OnceCell<
std::sync::Arc<std::sync::RwLock<crate::replication::state::ReplicationState>>,
> = once_cell::sync::OnceCell::new();

/// Register the global replication state for INFO queries.
pub fn set_global_repl_state(
state: std::sync::Arc<std::sync::RwLock<crate::replication::state::ReplicationState>>,
) {
Comment on lines +546 to +553
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check if the file exists and examine the code at the specified lines
if [ -f "src/admin/metrics_setup.rs" ]; then
  echo "=== File exists. Checking lines 546-553 ==="
  sed -n '546,553p' src/admin/metrics_setup.rs
  echo ""
  echo "=== Checking for std::sync::RwLock usage in this file ==="
  grep -n "std::sync::RwLock\|std::sync::Mutex\|std::sync::Arc" src/admin/metrics_setup.rs | head -20
else
  echo "File not found at src/admin/metrics_setup.rs"
fi

Repository: pilotspace/moon

Length of output: 865


🏁 Script executed:

# Get context around lines 546-553 to understand the usage better
if [ -f "src/admin/metrics_setup.rs" ]; then
  echo "=== Context around lines 546-553 ==="
  sed -n '540,560p' src/admin/metrics_setup.rs
fi

Repository: pilotspace/moon

Length of output: 925


🏁 Script executed:

# Check if parking_lot is available in the project dependencies
echo "=== Checking Cargo.toml for parking_lot dependency ==="
if grep -r "parking_lot" Cargo.toml 2>/dev/null; then
  echo "parking_lot found in main Cargo.toml"
else
  echo "parking_lot may be in workspace member Cargo.toml or feature-gated"
  find . -name "Cargo.toml" -type f -exec grep -l "parking_lot" {} \; 2>/dev/null | head -10
fi

Repository: pilotspace/moon

Length of output: 171


Replace std::sync::RwLock with parking_lot::RwLock in the global replication state.

Both the static type (line 547) and the function parameter (line 552) use std::sync::RwLock, which violates the coding guideline: never use std::sync locks. Update both to use parking_lot::RwLock instead. parking_lot is already a project dependency.

Code snippet
static GLOBAL_REPL_STATE: once_cell::sync::OnceCell<
    std::sync::Arc<std::sync::RwLock<crate::replication::state::ReplicationState>>,
> = once_cell::sync::OnceCell::new();

pub fn set_global_repl_state(
    state: std::sync::Arc<std::sync::RwLock<crate::replication::state::ReplicationState>>,
) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/admin/metrics_setup.rs` around lines 546 - 553, The GLOBAL_REPL_STATE
static and the set_global_repl_state function parameter use std::sync::RwLock;
change both to use parking_lot::RwLock instead (keep Arc as std::sync::Arc).
Update the type in the OnceCell declaration for GLOBAL_REPL_STATE and the
parameter type of set_global_repl_state to
std::sync::Arc<parking_lot::RwLock<crate::replication::state::ReplicationState>>
(or fully-qualified parking_lot::RwLock) and adjust any imports if necessary.

let _ = GLOBAL_REPL_STATE.set(state);
}

/// Get replication info for INFO command: (role, connected_slaves, master_repl_offset, repl_id).
/// Also updates the Prometheus replication lag gauge as a side-effect.
pub fn get_replication_info() -> (&'static str, usize, u64, String) {
Comment on lines +546 to +559
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

3. std::sync::rwlock in metrics 📘 Rule violation ☼ Reliability

src/admin/metrics_setup.rs introduces std::sync::RwLock for global replication state, contrary
to the project locking rule requiring parking_lot primitives. This increases risk of poisoning
behavior and violates the lock primitive standardization requirement.
Agent Prompt
## Issue description
A new `std::sync::RwLock` is introduced for `GLOBAL_REPL_STATE` in `src/admin/metrics_setup.rs`, violating the requirement to use `parking_lot` locks.

## Issue Context
Project locking rules standardize on `parking_lot` to avoid poisoning semantics and improve performance/consistency.

## Fix Focus Areas
- src/admin/metrics_setup.rs[535-548]
- src/admin/metrics_setup.rs[549-582]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

if let Some(state) = GLOBAL_REPL_STATE.get() {
if let Ok(guard) = state.read() {
let role = match &guard.role {
crate::replication::state::ReplicationRole::Master => "master",
crate::replication::state::ReplicationRole::Replica { .. } => "slave",
};
let slaves = guard.replicas.len();
let offset = guard.master_repl_offset.load(Ordering::Relaxed);
let repl_id = guard.repl_id.clone();
// Update Prometheus lag gauge: max lag across all replicas.
if !guard.replicas.is_empty() {
let max_lag_bytes = guard
.replicas
.iter()
.map(|r| {
let ack: u64 = r
.ack_offsets
.iter()
.map(|a| a.load(Ordering::Relaxed))
.sum();
offset.saturating_sub(ack)
})
.max()
.unwrap_or(0);
record_replication_lag(max_lag_bytes, 0);
}
Comment on lines +569 to +585
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Reset replication lag to zero when no replicas are connected.

The gauge is only updated inside the non-empty branch, so after the last replica disconnects moon_replication_lag_bytes keeps exporting the previous non-zero lag forever. That makes the new metric operationally misleading.

Suggested fix
-            if !guard.replicas.is_empty() {
-                let max_lag_bytes = guard
-                    .replicas
-                    .iter()
-                    .map(|r| {
-                        let ack: u64 = r
-                            .ack_offsets
-                            .iter()
-                            .map(|a| a.load(Ordering::Relaxed))
-                            .sum();
-                        offset.saturating_sub(ack)
-                    })
-                    .max()
-                    .unwrap_or(0);
-                record_replication_lag(max_lag_bytes, 0);
-            }
+            let max_lag_bytes = guard
+                .replicas
+                .iter()
+                .map(|r| {
+                    let ack: u64 = r
+                        .ack_offsets
+                        .iter()
+                        .map(|a| a.load(Ordering::Relaxed))
+                        .sum();
+                    offset.saturating_sub(ack)
+                })
+                .max()
+                .unwrap_or(0);
+            record_replication_lag(max_lag_bytes, 0);
             return (role, slaves, offset, repl_id);
📝 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
// Update Prometheus lag gauge: max lag across all replicas.
if !guard.replicas.is_empty() {
let max_lag_bytes = guard
.replicas
.iter()
.map(|r| {
let ack: u64 = r
.ack_offsets
.iter()
.map(|a| a.load(Ordering::Relaxed))
.sum();
offset.saturating_sub(ack)
})
.max()
.unwrap_or(0);
record_replication_lag(max_lag_bytes, 0);
}
let max_lag_bytes = guard
.replicas
.iter()
.map(|r| {
let ack: u64 = r
.ack_offsets
.iter()
.map(|a| a.load(Ordering::Relaxed))
.sum();
offset.saturating_sub(ack)
})
.max()
.unwrap_or(0);
record_replication_lag(max_lag_bytes, 0);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/admin/metrics_setup.rs` around lines 569 - 585, The Prometheus gauge is
only updated when guard.replicas is non-empty, so when the last replica
disconnects the previous non-zero value remains exported; modify the logic
around guard.replicas in the function containing this block (the code that
computes max_lag_bytes and calls record_replication_lag) to explicitly set the
metric to zero when replicas.is_empty() (e.g., call record_replication_lag(0, 0)
or equivalent in the empty branch) so moon_replication_lag_bytes is reset to 0
when no replicas are connected.

return (role, slaves, offset, repl_id);
}
}
("master", 0, 0, "0".repeat(40))
}

// ── Global SLOWLOG ─────────────────────────────────────────────────────

/// Global slowlog instance accessible from any handler thread.
Expand Down
52 changes: 43 additions & 9 deletions src/command/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,21 +130,54 @@ pub fn readyz() -> Frame {
}
}

/// Format bytes as human-readable (e.g. "1.23M", "456.78K").
fn format_memory_human(bytes: u64) -> String {
const KB: f64 = 1024.0;
const MB: f64 = 1024.0 * 1024.0;
const GB: f64 = 1024.0 * 1024.0 * 1024.0;
let b = bytes as f64;
if b >= GB {
format!("{:.2}G", b / GB)
} else if b >= MB {
format!("{:.2}M", b / MB)
} else if b >= KB {
format!("{:.2}K", b / KB)
} else {
format!("{bytes}B")
}
Comment on lines +133 to +147
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. format! in format_memory_human 📘 Rule violation ➹ Performance

src/command/connection.rs introduces format!-based string building in the command path, which
performs heap allocations and formatting work in a hot module. This violates the
no-allocation/formatting requirement for code under src/command/.
Agent Prompt
## Issue description
`src/command/connection.rs` adds `format!` calls for INFO/`format_memory_human()`, which violates the hot-path allocation/formatting restrictions for `src/command/`.

## Issue Context
Compliance requires avoiding `format!()`/`to_string()`/expensive allocations in command dispatch paths.

## Fix Focus Areas
- src/command/connection.rs[133-147]
- src/command/connection.rs[171-180]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

}

/// INFO command handler.
///
/// Returns a BulkString with minimal INFO sections.
/// Returns a BulkString with server info sections matching Redis INFO format.
pub fn info(db: &Database, _args: &[Frame]) -> Frame {
let mut sections = String::new();
use std::fmt::Write as _;
let mut sections = String::with_capacity(2048);

sections.push_str("# Server\r\n");
sections.push_str("redis_version:0.1.0\r\n");
sections.push_str("moon:true\r\n");
sections.push_str("\r\n");

sections.push_str("# Clients\r\n");
let _ = write!(
sections,
"connected_clients:{}\r\n",
crate::admin::metrics_setup::connected_clients(),
);
sections.push_str("\r\n");

sections.push_str("# Memory\r\n");
let rss = crate::admin::metrics_setup::get_rss_bytes();
let _ = write!(
sections,
"used_memory:{rss}\r\n\
used_memory_human:{human}\r\n\
used_memory_rss:{rss}\r\n\
used_memory_peak:{rss}\r\n",
Comment on lines +174 to +177
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

5. Info keys prefixed by spaces 🐞 Bug ≡ Correctness

INFO uses multi-line string literals with \ line continuation plus indentation, which embeds
leading spaces into field names (e.g. " used_memory_human"), breaking Redis INFO compatibility and
parsers.
Agent Prompt
## Issue description
INFO output includes leading spaces in keys due to indented `\`-continued string literals, which breaks Redis INFO formatting expectations.

## Issue Context
Rust string literal line continuation `\` removes the newline but preserves any indentation whitespace on the next source line.

## Fix Focus Areas
- src/command/connection.rs[170-190]
- src/command/connection.rs[240-272]

## Suggested fix
Rewrite the format strings so continued lines start immediately (no indentation), e.g.:
- "used_memory:{rss}\r\nused_memory_human:{human}\r\nused_memory_rss:{rss}\r\n..."
Apply the same change to Persistence/Vector/Stats/CPU/Replication sections.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +171 to +177
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

used_memory_peak should not mirror current RSS.

This now reports the current RSS as the peak, so the “peak” value will shrink again after memory is released. INFO consumers usually assume used_memory_peak is monotonic. Please track a real high-water mark in metrics_setup and use that here instead of reusing rss.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/command/connection.rs` around lines 171 - 177, The current code prints
used_memory_peak using the current rss value, which makes the peak
non-monotonic; update metrics tracking in crate::admin::metrics_setup to
maintain a high-water mark (e.g., add or expose a get_rss_peak_bytes or
update_rss_peak function that stores the max observed RSS) and then replace the
peak usage here to call that new accessor instead of reusing rss (reference
get_rss_bytes and the used_memory_peak output string in this block to locate
where to swap rss for the real peak value); ensure the peak is only increased
when a new rss > stored_peak so used_memory_peak remains monotonic.

rss = rss,
human = format_memory_human(rss),
);
sections.push_str("\r\n");

sections.push_str("# Persistence\r\n");
Expand Down Expand Up @@ -196,7 +229,6 @@ pub fn info(db: &Database, _args: &[Frame]) -> Frame {
sections.push_str("\r\n");

sections.push_str("# MoonStore\r\n");
use std::fmt::Write as _;
let _ = write!(
sections,
"disk_offload_enabled:{}\r\n",
Expand Down Expand Up @@ -228,13 +260,15 @@ pub fn info(db: &Database, _args: &[Frame]) -> Frame {
sections.push_str("\r\n");

// # Replication
// NOTE: placeholder — always reports master with 0 replicas.
// TODO: wire to actual ReplicationState when replication is implemented.
sections.push_str("# Replication\r\n");
sections.push_str("role:master\r\n");
sections.push_str("connected_slaves:0\r\n");
sections.push_str("master_replid:0000000000000000000000000000000000000000\r\n");
sections.push_str("master_repl_offset:0\r\n");
let (role, slaves, offset, repl_id) = crate::admin::metrics_setup::get_replication_info();
let _ = write!(
sections,
"role:{role}\r\n\
connected_slaves:{slaves}\r\n\
master_replid:{repl_id}\r\n\
master_repl_offset:{offset}\r\n",
);
sections.push_str("\r\n");

sections.push_str("# Keyspace\r\n");
Expand Down
57 changes: 16 additions & 41 deletions src/command/sorted_set/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,27 +263,15 @@ pub(super) fn zrange_by_score(
limit_offset: Option<i64>,
limit_count: Option<i64>,
) -> Frame {
let (min_bound, max_bound) = if rev {
// With REV, max comes first in args
let max_b = match parse_score_bound(min_arg) {
Ok(b) => b,
Err(e) => return e,
};
let min_b = match parse_score_bound(max_arg) {
Ok(b) => b,
Err(e) => return e,
};
(min_b, max_b)
} else {
let min_b = match parse_score_bound(min_arg) {
Ok(b) => b,
Err(e) => return e,
};
let max_b = match parse_score_bound(max_arg) {
Ok(b) => b,
Err(e) => return e,
};
(min_b, max_b)
// All callers pass (min, max) in semantic order regardless of rev.
// The rev flag only affects iteration direction (entries.reverse below).
let min_bound = match parse_score_bound(min_arg) {
Ok(b) => b,
Err(e) => return e,
};
let max_bound = match parse_score_bound(max_arg) {
Ok(b) => b,
Err(e) => return e,
};

let _ = members; // not directly needed; scores has all data
Expand Down Expand Up @@ -344,26 +332,13 @@ pub(super) fn zrange_by_lex(
limit_offset: Option<i64>,
limit_count: Option<i64>,
) -> Frame {
let (min_bound, max_bound) = if rev {
let max_b = match parse_lex_bound(min_arg) {
Ok(b) => b,
Err(e) => return e,
};
let min_b = match parse_lex_bound(max_arg) {
Ok(b) => b,
Err(e) => return e,
};
(min_b, max_b)
} else {
let min_b = match parse_lex_bound(min_arg) {
Ok(b) => b,
Err(e) => return e,
};
let max_b = match parse_lex_bound(max_arg) {
Ok(b) => b,
Err(e) => return e,
};
(min_b, max_b)
let min_bound = match parse_lex_bound(min_arg) {
Ok(b) => b,
Err(e) => return e,
};
let max_bound = match parse_lex_bound(max_arg) {
Ok(b) => b,
Err(e) => return e,
};

let mut entries: Vec<&Bytes> = Vec::new();
Expand Down
Loading