From 7f4d8b56a3d4aa809c3d5a341c263041745a8483 Mon Sep 17 00:00:00 2001 From: Kacy Fortner Date: Thu, 5 Feb 2026 20:36:52 -0500 Subject: [PATCH] chore: code audit improvements and ci infrastructure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit infrastructure: - add rust-toolchain.toml to pin rust 1.93 with fmt/clippy components - add .cargo/config.toml with release optimizations (lto, strip, codegen-units) - add .github/workflows/ci.yml for automated testing, linting, and security audits - add CONTRIBUTING.md with development workflow and code standards code improvements: - rewrite glob_match() with O(n*m) iterative algorithm instead of O(n²) recursive - add cursor validation in SCAN handler to guard against invalid shard_id - improve documentation for memory overhead constants and LRU sampling - document SCAN cursor encoding format (shard_id << 48 | position) - fix clippy lint (use is_multiple_of instead of % 2 != 0) --- .cargo/config.toml | 31 +++++ .github/workflows/ci.yml | 67 +++++++++ CONTRIBUTING.md | 73 ++++++++++ crates/ember-core/src/keyspace.rs | 168 ++++++++++++++--------- crates/ember-core/src/memory.rs | 9 +- crates/ember-core/src/shard.rs | 7 +- crates/ember-persistence/src/recovery.rs | 4 +- crates/ember-protocol/src/command.rs | 2 +- crates/ember-server/src/connection.rs | 24 +++- rust-toolchain.toml | 3 + 10 files changed, 316 insertions(+), 72 deletions(-) create mode 100644 .cargo/config.toml create mode 100644 .github/workflows/ci.yml create mode 100644 CONTRIBUTING.md create mode 100644 rust-toolchain.toml diff --git a/.cargo/config.toml b/.cargo/config.toml new file mode 100644 index 00000000..826dfba6 --- /dev/null +++ b/.cargo/config.toml @@ -0,0 +1,31 @@ +# cargo configuration for ember +# https://doc.rust-lang.org/cargo/reference/config.html + +[build] +# use all available cores for parallel compilation +jobs = -1 + +[profile.release] +# link-time optimization for smaller, faster binaries +lto = true +# single codegen unit for better optimization (slower compile) +codegen-units = 1 +# strip symbols from release binaries +strip = true +# abort on panic for smaller binary and cleaner crash behavior +panic = "abort" + +[profile.dev] +# faster incremental builds in dev mode +incremental = true + +[profile.test] +# keep debug info in test builds for better backtraces +debug = true + +# alias for common operations +[alias] +b = "build" +c = "check" +t = "test" +r = "run" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 00000000..1d8468c9 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,67 @@ +name: ci + +on: + push: + branches: [main] + pull_request: + branches: [main] + +env: + CARGO_TERM_COLOR: always + RUST_BACKTRACE: 1 + +jobs: + check: + name: check + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + with: + components: rustfmt, clippy + - uses: Swatinem/rust-cache@v2 + - name: fmt + run: cargo fmt --all --check + - name: clippy + run: cargo clippy --workspace -- -D warnings + - name: check + run: cargo check --workspace + + test: + name: test + runs-on: ${{ matrix.os }} + strategy: + matrix: + os: [ubuntu-latest, macos-latest] + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + - uses: Swatinem/rust-cache@v2 + - name: test + run: cargo test --workspace + + build: + name: build + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + - uses: Swatinem/rust-cache@v2 + - name: build release + run: cargo build --release + - name: upload binary + uses: actions/upload-artifact@v4 + with: + name: ember-server + path: target/release/ember-server + + security: + name: security + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + - name: install cargo-audit + run: cargo install cargo-audit + - name: audit + run: cargo audit diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 00000000..c0b2083d --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,73 @@ +# contributing to ember + +thanks for your interest in contributing to ember. this document covers the +development workflow and standards we follow. + +## getting started + +```bash +# clone the repo +git clone https://github.com/kacy/ember +cd ember + +# build and test +make check +``` + +## development workflow + +1. **create a branch** from `main` for your work + - use descriptive names: `feat/add-pubsub`, `fix/memory-leak`, `docs/api-examples` + +2. **make changes** following the code standards below + +3. **run checks** before committing: + ```bash + make check # runs fmt, clippy, and tests + ``` + +4. **commit** with clear messages: + - use lowercase, present tense: `add pubsub support`, `fix memory tracking bug` + - keep commits atomic and focused + +5. **open a pull request** against `main` + - include a summary of changes + - describe what was tested + - note any design considerations + +## code standards + +### style + +- run `cargo fmt` before committing +- run `cargo clippy` with warnings as errors +- no `unwrap()` in library code — use proper error handling +- no `unsafe` without a comment explaining why + +### documentation + +- every public item needs a doc comment +- include examples for complex apis +- document panic conditions and performance characteristics + +### testing + +- add tests for new functionality +- focus on edge cases and error paths +- run the full test suite before submitting + +## project structure + +``` +crates/ +├── ember-server/ # main server binary +├── ember-core/ # sharded engine and data structures +├── ember-protocol/ # resp3 parsing and commands +├── ember-persistence/# aof and snapshots +├── ember-cluster/ # distributed clustering (wip) +└── ember-cli/ # command-line client +``` + +## questions? + +open an issue or start a discussion — we're happy to help. diff --git a/crates/ember-core/src/keyspace.rs b/crates/ember-core/src/keyspace.rs index 62d39351..a9c9eb20 100644 --- a/crates/ember-core/src/keyspace.rs +++ b/crates/ember-core/src/keyspace.rs @@ -62,10 +62,15 @@ pub enum IncrError { impl std::fmt::Display for IncrError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - IncrError::WrongType => write!(f, "WRONGTYPE Operation against a key holding the wrong kind of value"), + IncrError::WrongType => write!( + f, + "WRONGTYPE Operation against a key holding the wrong kind of value" + ), IncrError::NotAnInteger => write!(f, "ERR value is not an integer or out of range"), IncrError::Overflow => write!(f, "ERR increment or decrement would overflow"), - IncrError::OutOfMemory => write!(f, "OOM command not allowed when used memory > 'maxmemory'"), + IncrError::OutOfMemory => { + write!(f, "OOM command not allowed when used memory > 'maxmemory'") + } } } } @@ -179,6 +184,15 @@ pub struct KeyspaceStats { } /// Number of random keys to sample when looking for an eviction candidate. +/// +/// Eviction uses sampling-based approximate LRU — we randomly select this many +/// keys and evict the least-recently-accessed among them. This trades perfect +/// LRU accuracy for O(1) eviction (no sorted structure to maintain). +/// +/// Larger sample sizes give better LRU approximation but cost more per eviction. +/// 16 is a reasonable balance — similar to Redis's default sample size. With +/// 16 samples, we statistically find a good eviction candidate while keeping +/// eviction overhead low even at millions of keys. const EVICTION_SAMPLE_SIZE: usize = 16; /// The core key-value store. @@ -1106,82 +1120,100 @@ impl Default for Keyspace { } } -/// Simple glob-style pattern matching for SCAN's MATCH option. -/// Supports: * (any sequence), ? (any single char), [abc] (char class). +/// Glob-style pattern matching for SCAN's MATCH option. +/// +/// Supports: +/// - `*` matches any sequence of characters (including empty) +/// - `?` matches exactly one character +/// - `[abc]` matches one character from the set +/// - `[^abc]` or `[!abc]` matches one character NOT in the set +/// +/// Uses an iterative two-pointer algorithm with backtracking for O(n*m) +/// worst-case performance, where n is pattern length and m is text length. fn glob_match(pattern: &str, text: &str) -> bool { - let mut pat = pattern.chars().peekable(); - let mut txt = text.chars().peekable(); - - fn match_rec( - pat: &mut std::iter::Peekable, - txt: &mut std::iter::Peekable, - ) -> bool { - loop { - match (pat.peek().copied(), txt.peek().copied()) { - (None, None) => return true, - (None, Some(_)) => return false, - (Some('*'), _) => { - pat.next(); - // try matching * with zero chars, one char, two chars, etc. - let pat_remaining: String = pat.clone().collect(); - let txt_remaining: String = txt.clone().collect(); - for i in 0..=txt_remaining.len() { - let mut p = pat_remaining.chars().peekable(); - let mut t = txt_remaining[i..].chars().peekable(); - if match_rec(&mut p, &mut t) { - return true; - } - } - return false; + let pat: Vec = pattern.chars().collect(); + let txt: Vec = text.chars().collect(); + + let mut pi = 0; // pattern index + let mut ti = 0; // text index + + // backtracking state for the most recent '*' + let mut star_pi: Option = None; + let mut star_ti: usize = 0; + + while ti < txt.len() || pi < pat.len() { + if pi < pat.len() { + match pat[pi] { + '*' => { + // record star position and try matching zero chars first + star_pi = Some(pi); + star_ti = ti; + pi += 1; + continue; } - (Some('?'), Some(_)) => { - pat.next(); - txt.next(); + '?' if ti < txt.len() => { + pi += 1; + ti += 1; + continue; } - (Some('['), Some(tc)) => { - pat.next(); // consume '[' - let mut matched = false; + '[' if ti < txt.len() => { + // parse character class + let tc = txt[ti]; + let mut j = pi + 1; let mut negated = false; - if pat.peek() == Some(&'^') || pat.peek() == Some(&'!') { + let mut matched = false; + + if j < pat.len() && (pat[j] == '^' || pat[j] == '!') { negated = true; - pat.next(); + j += 1; } - while let Some(&c) = pat.peek() { - if c == ']' { - pat.next(); - break; - } - pat.next(); - if c == tc { + + while j < pat.len() && pat[j] != ']' { + if pat[j] == tc { matched = true; } + j += 1; } + if negated { matched = !matched; } - if !matched { - return false; + + if matched && j < pat.len() { + pi = j + 1; // skip past ']' + ti += 1; + continue; } - txt.next(); - } - (Some(pc), Some(tc)) if pc == tc => { - pat.next(); - txt.next(); + // fall through to backtrack } - (Some(_), Some(_)) => return false, - (Some(_), None) => { - // pattern has more chars but text is done - // only ok if remaining pattern is all * - while pat.peek() == Some(&'*') { - pat.next(); - } - return pat.peek().is_none(); + c if ti < txt.len() && c == txt[ti] => { + pi += 1; + ti += 1; + continue; } + _ => {} } } + + // mismatch or end of pattern — try backtracking to last '*' + if let Some(sp) = star_pi { + pi = sp + 1; + star_ti += 1; + ti = star_ti; + if ti > txt.len() { + return false; + } + } else { + return false; + } } - match_rec(&mut pat, &mut txt) + // skip trailing '*' in pattern + while pi < pat.len() && pat[pi] == '*' { + pi += 1; + } + + pi == pat.len() } #[cfg(test)] @@ -2142,7 +2174,11 @@ mod tests { #[test] fn persist_removes_expiry() { let mut ks = Keyspace::new(); - ks.set("key".into(), Bytes::from("val"), Some(Duration::from_secs(60))); + ks.set( + "key".into(), + Bytes::from("val"), + Some(Duration::from_secs(60)), + ); assert!(matches!(ks.ttl("key"), TtlResult::Seconds(_))); assert!(ks.persist("key")); @@ -2166,7 +2202,11 @@ mod tests { #[test] fn pttl_returns_milliseconds() { let mut ks = Keyspace::new(); - ks.set("key".into(), Bytes::from("val"), Some(Duration::from_secs(60))); + ks.set( + "key".into(), + Bytes::from("val"), + Some(Duration::from_secs(60)), + ); match ks.pttl("key") { TtlResult::Milliseconds(ms) => assert!(ms > 59_000 && ms <= 60_000), other => panic!("expected Milliseconds, got {other:?}"), @@ -2207,7 +2247,11 @@ mod tests { #[test] fn pexpire_overwrites_existing_ttl() { let mut ks = Keyspace::new(); - ks.set("key".into(), Bytes::from("val"), Some(Duration::from_secs(60))); + ks.set( + "key".into(), + Bytes::from("val"), + Some(Duration::from_secs(60)), + ); assert!(ks.pexpire("key", 500)); match ks.pttl("key") { TtlResult::Milliseconds(ms) => assert!(ms <= 500), diff --git a/crates/ember-core/src/memory.rs b/crates/ember-core/src/memory.rs index 8439ed03..320c6d79 100644 --- a/crates/ember-core/src/memory.rs +++ b/crates/ember-core/src/memory.rs @@ -10,8 +10,13 @@ use crate::types::Value; /// /// Accounts for: HashMap bucket pointer (8), Entry struct fields /// (Option = 16, last_access Instant = 8, Value enum tag + padding), -/// plus HashMap per-entry bookkeeping. This doesn't need to be exact — -/// it's close enough for eviction triggers and stats reporting. +/// plus HashMap per-entry bookkeeping. +/// +/// This is an approximation measured empirically on x86-64 linux. The exact +/// value varies by platform and compiler version, but precision isn't critical — +/// we use this for eviction triggers and memory reporting, not for correctness. +/// Overestimating is fine (triggers eviction earlier); underestimating could +/// theoretically let memory grow slightly beyond the configured limit. pub(crate) const ENTRY_OVERHEAD: usize = 96; /// Tracks memory usage for a single keyspace. diff --git a/crates/ember-core/src/shard.rs b/crates/ember-core/src/shard.rs index bcbb518b..0939d8a9 100644 --- a/crates/ember-core/src/shard.rs +++ b/crates/ember-core/src/shard.rs @@ -565,7 +565,12 @@ fn dispatch(ks: &mut Keyspace, req: &ShardRequest) -> ShardResponse { /// Returns None for non-mutation requests or failed mutations. fn to_aof_record(req: &ShardRequest, resp: &ShardResponse) -> Option { match (req, resp) { - (ShardRequest::Set { key, value, expire, .. }, ShardResponse::Ok) => { + ( + ShardRequest::Set { + key, value, expire, .. + }, + ShardResponse::Ok, + ) => { let expire_ms = expire.map(|d| d.as_millis() as i64).unwrap_or(-1); Some(AofRecord::Set { key: key.clone(), diff --git a/crates/ember-persistence/src/recovery.rs b/crates/ember-persistence/src/recovery.rs index 6ce3c1b7..a1636c4f 100644 --- a/crates/ember-persistence/src/recovery.rs +++ b/crates/ember-persistence/src/recovery.rs @@ -669,7 +669,9 @@ mod tests { .unwrap(); // also test INCR on a new key writer - .write_record(&AofRecord::Incr { key: "fresh".into() }) + .write_record(&AofRecord::Incr { + key: "fresh".into(), + }) .unwrap(); writer.sync().unwrap(); } diff --git a/crates/ember-protocol/src/command.rs b/crates/ember-protocol/src/command.rs index 44b4cf15..1fccff18 100644 --- a/crates/ember-protocol/src/command.rs +++ b/crates/ember-protocol/src/command.rs @@ -413,7 +413,7 @@ fn parse_mget(args: &[Frame]) -> Result { } fn parse_mset(args: &[Frame]) -> Result { - if args.is_empty() || args.len() % 2 != 0 { + if args.is_empty() || !args.len().is_multiple_of(2) { return Err(ProtocolError::WrongArity("MSET".into())); } let mut pairs = Vec::with_capacity(args.len() / 2); diff --git a/crates/ember-server/src/connection.rs b/crates/ember-server/src/connection.rs index c540d7ce..c50e2d5b 100644 --- a/crates/ember-server/src/connection.rs +++ b/crates/ember-server/src/connection.rs @@ -262,8 +262,7 @@ async fn execute(cmd: Command, engine: &Engine) -> Frame { Command::MSet { pairs } => { // fan out individual SET requests — MSET always succeeds (or OOMs) let keys: Vec = pairs.iter().map(|(k, _)| k.clone()).collect(); - let values: std::collections::HashMap = - pairs.into_iter().collect(); + let values: std::collections::HashMap = pairs.into_iter().collect(); match engine .route_multi(&keys, |k| { @@ -355,8 +354,14 @@ async fn execute(cmd: Command, engine: &Engine) -> Frame { pattern, count, } => { - // cursor format: (shard_id << 48) | position_within_shard - // cursor 0 means start from shard 0, position 0 + // cursor encoding: (shard_id << 48) | position_within_shard + // + // this gives us 16 bits for shard_id (up to 65536 shards) and 48 bits + // for position within each shard. cursor 0 always means "start fresh". + // + // the cursor is opaque to clients — they just pass back whatever we + // returned last time. this lets us iterate across the sharded keyspace + // without clients needing to know the topology. let shard_count = engine.shard_count(); let count = count.unwrap_or(10); @@ -365,6 +370,12 @@ async fn execute(cmd: Command, engine: &Engine) -> Frame { } else { let shard_id = (cursor >> 48) as usize; let position = cursor & 0xFFFF_FFFF_FFFF; + + // guard against invalid cursor (shard_id out of range) + if shard_id >= shard_count { + return Frame::Array(vec![Frame::Bulk(Bytes::from("0")), Frame::Array(vec![])]); + } + (shard_id, position) }; @@ -380,7 +391,10 @@ async fn execute(cmd: Command, engine: &Engine) -> Frame { pattern: pattern.clone(), }; match engine.send_to_shard(current_shard, req).await { - Ok(ShardResponse::Scan { cursor: next_pos, keys }) => { + Ok(ShardResponse::Scan { + cursor: next_pos, + keys, + }) => { all_keys.extend(keys); if next_pos == 0 { // shard exhausted, move to next diff --git a/rust-toolchain.toml b/rust-toolchain.toml new file mode 100644 index 00000000..358641ee --- /dev/null +++ b/rust-toolchain.toml @@ -0,0 +1,3 @@ +[toolchain] +channel = "1.93" +components = ["rustfmt", "clippy"]