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
23 changes: 23 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,29 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed — zombie/CPU hardening wave 1 (PR #TBD)

- **Busy-poll spin idle-disengages** (`--io-busy-poll-us` / vendored monoio
legacy driver): an idle server no longer burns the spin budget on every
park forever (measured ~2.4s CPU per 3s wall on an idle 4-shard server at
200µs). The driver tracks the last real event per thread and skips the
spin window after `MOON_SPIN_IDLE_DISENGAGE_US` (default 10ms; 0 = old
always-spin) of quiet; the next event re-arms it via the normal wake path,
so steady traffic — the GCE p=1 win path — never disengages.
- **Shard-thread panics abort the whole process** instead of leaving an
N-1-shard server silently answering with a dead shard's keys gone; the
shutdown join loop logs instead of swallowing panics. New `DEBUG PANIC`
subcommand (Redis parity) as the crash-handling test aid.
- **SIGTERM shutdown regression matrix**: shards × held-connections ×
busy-poll × 16-writer AOF write-storm (7 cases, both runtimes, 3
platforms). The documented SIGTERM+SO_REUSEPORT bench hang did NOT
reproduce at HEAD in any composition; the matrix stays as the guard and
the detached monoio accept-task change is deferred until a reproducer
exists.
- **`wait_ready` test harness hardening**: readiness probes retry with a
fresh connection on mid-startup connection resets (CI flake: per-shard
SO_REUSEPORT listeners accept-then-reset during init).

### Added — int8 symmetric ADC for SQ8 vector search (task #13)

- New per-candidate integer dot-product path for SQ8 asymmetric distance
Expand Down
17 changes: 17 additions & 0 deletions src/command/server_admin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ pub fn debug(db: &mut Database, args: &[Frame]) -> Frame {
match classify_debug(args) {
Ok(DebugCall::Object(rest)) => debug_object(db, rest),
Ok(DebugCall::Sleep(rest)) => debug_sleep(rest),
Ok(DebugCall::Panic) => debug_panic(),
Ok(DebugCall::Help) => debug_help(),
Err(e) => e,
}
Expand All @@ -168,6 +169,7 @@ pub fn debug_readonly(db: &Database, args: &[Frame], now_ms: u64) -> Frame {
match classify_debug(args) {
Ok(DebugCall::Object(rest)) => debug_object_readonly(db, rest, now_ms),
Ok(DebugCall::Sleep(rest)) => debug_sleep(rest),
Ok(DebugCall::Panic) => debug_panic(),
Ok(DebugCall::Help) => debug_help(),
Err(e) => e,
}
Expand All @@ -176,6 +178,7 @@ pub fn debug_readonly(db: &Database, args: &[Frame], now_ms: u64) -> Frame {
enum DebugCall<'a> {
Object(&'a [Frame]),
Sleep(&'a [Frame]),
Panic,
Help,
}

Expand All @@ -191,6 +194,8 @@ fn classify_debug(args: &[Frame]) -> Result<DebugCall<'_>, Frame> {
Ok(DebugCall::Object(&args[1..]))
} else if sub.eq_ignore_ascii_case(b"SLEEP") {
Ok(DebugCall::Sleep(&args[1..]))
} else if sub.eq_ignore_ascii_case(b"PANIC") {
Ok(DebugCall::Panic)
} else if sub.eq_ignore_ascii_case(b"HELP") {
Ok(DebugCall::Help)
} else {
Expand All @@ -211,6 +216,10 @@ fn debug_help() -> Frame {
Frame::BulkString(Bytes::from_static(
b" Stall this shard for <seconds> (float, capped at 30).",
)),
Frame::BulkString(Bytes::from_static(b"DEBUG PANIC")),
Frame::BulkString(Bytes::from_static(
b" Panic this shard thread (crash-handling test aid, as in Redis).",
)),
Frame::BulkString(Bytes::from_static(b"DEBUG HELP")),
Frame::BulkString(Bytes::from_static(b" Return subcommand help.")),
])
Expand Down Expand Up @@ -256,6 +265,14 @@ fn debug_object_reply(entry: &Entry) -> Frame {
Frame::SimpleString(Bytes::from(body))
}

/// `DEBUG PANIC` — deliberately panic the executing shard thread (Redis
/// parity: a crash-handling test aid). The process-level panic policy
/// (fail-fast abort, installed in main) is what a client observes; the
/// return type exists only for the signature.
fn debug_panic() -> Frame {
panic!("DEBUG PANIC requested by client");
}
Comment on lines +268 to +274

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. Remote abort via debug panic 🐞 Bug ⛨ Security

DEBUG PANIC unconditionally panics on the shard thread, and the new global panic hook aborts the
whole process for any shard-* panic, making it a client-triggerable full server outage when
DEBUG is permitted. Under the default unrestricted ACL user, DEBUG is allowed, so this is
reachable in default configurations.
Agent Prompt
## Issue description
`DEBUG PANIC` currently allows any client who can run `DEBUG` to crash the server (panic on shard thread → panic hook aborts process). This is an operational DoS risk in default/unrestricted ACL configurations.

## Issue Context
- `DEBUG PANIC` panics unconditionally.
- `main` installs a panic hook that aborts the whole process for `shard-*` thread panics.
- Default ACL user is unrestricted (AllAllowed / +@all), and `@all` includes `debug`.

## Fix Focus Areas
- Add an explicit runtime config/CLI flag (default **disabled**) required to enable `DEBUG PANIC`, and return an error when disabled.
- Alternatively (or additionally), compile-gate it behind a feature (e.g. `cfg(feature = "debug-panic")`) used only by tests.
- Update tests that rely on `DEBUG PANIC` to pass the enable flag / feature.

### Code references
- src/command/server_admin.rs[268-274]
- src/main.rs[96-116]
- src/acl/table.rs[44-62]
- src/acl/rules.rs[221-257]

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


fn debug_sleep(args: &[Frame]) -> Frame {
if args.len() != 1 {
return err_wrong_args("DEBUG SLEEP");
Expand Down
30 changes: 29 additions & 1 deletion src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,29 @@ fn main() -> anyhow::Result<()> {
)
.init();

// Fail-fast on shard-thread panics. Shard threads are joined only at
// shutdown, so an unwinding panic during normal operation would leave an
// N-1-shard server listening and answering while every key owned by the
// dead shard is silently gone — worse than a crash for a database. The
// default hook runs first so the panic message + backtrace still print.
// (Guarded by thread-name prefix: panics on connection/test/aux threads
// keep their normal unwind behavior.)
{
let default_hook = std::panic::take_hook();
std::panic::set_hook(Box::new(move |info| {
default_hook(info);
let thread = std::thread::current();
let name = thread.name().unwrap_or("");
if name.starts_with("shard-") {
eprintln!(
"FATAL: shard thread '{name}' panicked; aborting the whole \
process rather than serving with a dead shard"
);
std::process::abort();
}
}));
}

// ── moon.conf integration: build merged argv before clap parse ──────────
// `merge_conf_argv` scans argv for a positional conf path (argv[1] not
// starting with `-`) or `--config FILE`. When found it loads and tokenises
Expand Down Expand Up @@ -1684,7 +1707,12 @@ fn main() -> anyhow::Result<()> {
}
cancel_token.cancel();
for handle in shard_handles {
let _ = handle.join();
// A shard panic normally aborts via the panic hook above; if one
// still surfaces here (hook replaced, non-unwind edge), say so
// instead of silently swallowing it.
if let Err(e) = handle.join() {
tracing::error!("shard thread panicked during shutdown: {e:?}");
}
}

info!("Server shut down");
Expand Down
179 changes: 179 additions & 0 deletions tests/busy_poll_idle.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
//! `--io-busy-poll-us` must not burn CPU on an IDLE server.
//!
//! RSS/CPU/OOM review item 2 (CPU lens): the poll-mode park spins its full
//! budget on EVERY park even with zero connections and zero work — an idle
//! server burns ~budget/park-interval CPU per shard thread, forever (the
//! "zombie CPU eater" when such a server is orphaned). The spin exists to
//! delete wake latency UNDER TRAFFIC; after an idle window it must disengage
//! and fall back to plain blocking parks, re-arming on the next real event.
//!
//! RED before the fix: an idle 4-shard server with `--io-busy-poll-us 200`
//! accumulates CPU time at roughly 0.8 cores (200µs spin per ~1ms timer
//! park × 4 shard threads); the 3s idle window below measures well over
//! 500ms of CPU. GREEN after: background ticks only, well under the bound.
//!
//! Linux-only (reads /proc/<pid>/stat). The busy-poll CLI flag forces the
//! epoll/kqueue LegacyDriver, so this exercises the patched spin path on
//! both io_uring-capable and MOON_NO_URING environments.
//!
//! monoio-only: under the tokio runtime `--io-busy-poll-us` is an explicit
//! no-op (main.rs warns and skips the spin wiring), so there is no spin to
//! measure — and a debug-build tokio server's unrelated idle-tick cost
//! (~0.9s/3s at 4 shards on CI) would fail the bound spuriously.

#![cfg(all(unix, target_os = "linux", feature = "runtime-monoio"))]
#![allow(clippy::unwrap_used)]

use std::io::{Read, Write};
use std::net::TcpStream;
use std::process::{Child, Command};
use std::time::{Duration, Instant};

fn moon_binary() -> std::path::PathBuf {
std::path::PathBuf::from(env!("CARGO_BIN_EXE_moon"))
}
Comment on lines +32 to +34

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

2. moon_binary() ignores moon_bin 📘 Rule violation ▣ Testability

New integration tests spawn the server binary via env!("CARGO_BIN_EXE_moon") instead of requiring
an explicit MOON_BIN setting. This violates the test harness rule and can cause CI/local runs to
use unintended binaries or paths.
Agent Prompt
## Issue description
Integration tests added in this PR start the server without requiring `MOON_BIN` to be explicitly set.

## Issue Context
The compliance requirement is that integration tests must explicitly use `MOON_BIN` for server binaries (and fail fast if it is missing) rather than relying on build-output discovery like `env!("CARGO_BIN_EXE_moon")`.

## Fix Focus Areas
- tests/busy_poll_idle.rs[27-29]
- tests/shard_panic_abort.rs[21-23]

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


fn free_port() -> u16 {
let l = std::net::TcpListener::bind("127.0.0.1:0").expect("bind :0");
let p = l.local_addr().expect("local_addr").port();
drop(l);
p
}

struct ServerGuard(Child);

impl Drop for ServerGuard {
fn drop(&mut self) {
let _ = self.0.kill();
let _ = self.0.wait();
}
}

fn wait_for_ready(port: u16, deadline: Duration) -> bool {
let start = Instant::now();
while start.elapsed() < deadline {
let addr = format!("127.0.0.1:{port}");
if let Ok(mut s) =
TcpStream::connect_timeout(&addr.parse().expect("addr"), Duration::from_millis(200))
{
s.set_read_timeout(Some(Duration::from_millis(500))).ok();
s.set_write_timeout(Some(Duration::from_millis(500))).ok();
if s.write_all(b"PING\r\n").is_ok() {
let mut buf = [0u8; 16];
if let Ok(n) = s.read(&mut buf) {
if buf[..n].windows(4).any(|w| w == b"PONG") {
return true;
}
}
}
}
std::thread::sleep(Duration::from_millis(100));
}
false
}

/// Process CPU time (user + system) in seconds from /proc/<pid>/stat.
fn cpu_seconds(pid: u32) -> f64 {
let stat = std::fs::read_to_string(format!("/proc/{pid}/stat")).expect("read /proc stat");
// comm can contain spaces — split after the closing paren.
let after = stat.rsplit_once(')').expect("stat paren").1;
let fields: Vec<&str> = after.split_whitespace().collect();
// After ')': field[0]=state ... utime is the 12th, stime the 13th.
let utime: u64 = fields[11].parse().expect("utime");
let stime: u64 = fields[12].parse().expect("stime");
// SAFETY: sysconf(_SC_CLK_TCK) reads a static kernel constant; it takes
// no pointers and touches no shared state.
let hz = unsafe { libc::sysconf(libc::_SC_CLK_TCK) } as f64;
(utime + stime) as f64 / hz
Comment on lines +84 to +87

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. libc::sysconf unsafe unapproved 📘 Rule violation ≡ Correctness

The new test introduces an unsafe call to libc::sysconf that is not listed under the repo’s
pre-approved unsafe patterns, so it must be avoided or explicitly approved per policy. Keeping
unapproved unsafe blocks increases the UB audit surface and violates the unsafe governance
requirements.
Agent Prompt
## Issue description
A new `unsafe { libc::sysconf(libc::_SC_CLK_TCK) }` call was added in an integration test. The unsafe policy requires new unsafe usage to match an approved pattern or be removed in favor of a safe alternative.

## Issue Context
`tests/busy_poll_idle.rs` computes CPU seconds from `/proc/<pid>/stat` and needs the system clock tick rate (HZ). The repo already depends on `nix` on Linux, which provides a safe `sysconf` wrapper.

## Fix Focus Areas
- tests/busy_poll_idle.rs[70-83]

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

}

#[test]
fn busy_poll_idle_server_does_not_burn_cpu() {
let dir = tempfile::tempdir().expect("tempdir");
let port = free_port();
let child = Command::new(moon_binary())
.args([
"--port",
&port.to_string(),
"--dir",
&dir.path().to_string_lossy(),
"--shards",
"4",
"--appendonly",
"no",
"--io-busy-poll-us",
"200",
])
.stdout(std::fs::File::create(dir.path().join("moon.stdout.log")).expect("stdout log"))
.stderr(std::fs::File::create(dir.path().join("moon.stderr.log")).expect("stderr log"))
.spawn()
.expect("spawn moon");
let guard = ServerGuard(child);
let pid = guard.0.id();

assert!(
wait_for_ready(port, Duration::from_secs(15)),
"server never ready"
);

// Settle past startup (recovery scan, first ticks) AND past the idle
// disengage window, then measure a fully idle interval.
std::thread::sleep(Duration::from_secs(1));
let before = cpu_seconds(pid);
std::thread::sleep(Duration::from_secs(3));
let burned = cpu_seconds(pid) - before;

// RED (spin always engaged): ~200µs per ~1ms park × 4 shards ≈ 2.4s here.
// GREEN (idle disengage): background ticks only — comfortably < 0.5s
// even on a loaded CI runner.
assert!(
burned < 0.5,
"idle 4-shard server with --io-busy-poll-us 200 burned {burned:.2}s \
CPU in 3s — spin never disengages when idle"
);
}

/// Traffic after an idle period must still be served (the disengaged spin
/// re-arms via the normal wake path — correctness, not latency, asserted).
#[test]
fn busy_poll_recovers_after_idle() {
let dir = tempfile::tempdir().expect("tempdir");
let port = free_port();
let child = Command::new(moon_binary())
.args([
"--port",
&port.to_string(),
"--dir",
&dir.path().to_string_lossy(),
"--shards",
"2",
"--appendonly",
"no",
"--io-busy-poll-us",
"40",
])
.stdout(std::fs::File::create(dir.path().join("moon.stdout.log")).expect("stdout log"))
.stderr(std::fs::File::create(dir.path().join("moon.stderr.log")).expect("stderr log"))
.spawn()
.expect("spawn moon");
let guard = ServerGuard(child);

assert!(
wait_for_ready(port, Duration::from_secs(15)),
"server never ready"
);
// Idle well past any disengage window, then talk to every shard's path.
std::thread::sleep(Duration::from_secs(2));
let addr = format!("127.0.0.1:{port}");
let mut s = TcpStream::connect_timeout(&addr.parse().expect("addr"), Duration::from_secs(2))
.expect("post-idle connect");
s.set_read_timeout(Some(Duration::from_secs(5))).ok();
for i in 0..100 {
s.write_all(format!("SET pk:{i} v{i}\r\nGET pk:{i}\r\n").as_bytes())
.expect("post-idle write");
let mut buf = [0u8; 128];
let n = s.read(&mut buf).expect("post-idle read");
assert!(n > 0, "no reply after idle re-engage (op {i})");
}
drop(guard);
}
Loading
Loading