Refactor and enhance documentation across core modules for clarity and consistency

Author: ekkolon

minikv-core/src/lib.rs

@@ -26,14 +26,14 @@
 //!
 //! # High-Level Architecture
 //!
-//! - `hashing`   — Key-to-path and volume scoring primitives.
-//! - `volumes`   — Replica selection and rebalance detection.
-//! - `record`    — On-disk metadata encoding format.
-//! - `storage`   — Metadata storage abstraction.
-//! - `replication` — HTTP primitives for volume interaction.
-//! - `rebuild`   — Metadata reconstruction from volumes.
-//! - `rebalance` — Migration to ideal replica sets.
-//! - `locking`   — Per-key concurrency control.
+//! - `hashing`     : Key-to-path and volume scoring primitives.
+//! - `volumes`     : Replica selection and rebalance detection.
+//! - `record`      : On-disk metadata encoding format.
+//! - `storage`     : Metadata storage abstraction.
+//! - `replication` : HTTP primitives for volume interaction.
+//! - `rebuild`     : Metadata reconstruction from volumes.
+//! - `rebalance`   : Migration to ideal replica sets.
+//! - `locking`     : Per-key concurrency control.
 //!
 //! This crate is intended to be embedded in a higher-level service.
 

minikv-core/src/record.rs

@@ -304,7 +304,7 @@ mod tests {
 
     #[test]
     fn decode_rejects_short_hash() {
-        // HASH prefix followed by only 10 hex chars — must fail
+        // HASH prefix followed by only 10 hex chars
         let bad = b"HASH0123456789hello";
         assert!(Record::decode(bad).is_err());
     }

minikv-core/src/state.rs

@@ -62,7 +62,7 @@ pub struct AppState {
     /// public-facing address (e.g. `"localhost:8001"`).
     ///
     /// Built once at startup from `--volumes` + `--public-volumes`.
-    /// Empty when `--public-volumes` is not set — Location headers will
+    /// Empty when `--public-volumes` is not set. Location headers will
     /// then use internal addresses unchanged (correct for bare-metal).
     pub vol_rewrite: HashMap<String, String>,
 

minikv/src/cli.rs

@@ -1,10 +1,10 @@
-/// CLI definition for `minikv`.
-///
-/// # Subcommands
-/// - `server`              run the HTTP metadata coordinator
-/// - `rebuild`             reconstruct LevelDB from volume server autoindex
-/// - `rebalance`           move all keys to their ideal volume set
-/// - `print-nginx-config`  emit the nginx volume server config to stdout
+//! CLI definition for `minikv`.
+//!
+//! # Subcommands
+//! - `server`              run the HTTP metadata coordinator
+//! - `rebuild`             reconstruct LevelDB from volume server autoindex
+//! - `rebalance`           move all keys to their ideal volume set
+
 use std::path::PathBuf;
 use std::time::Duration;
 

minikv/src/http/handlers.rs

@@ -1,39 +1,78 @@
-/// HTTP request handlers
-///
-/// All standard and custom methods (UNLINK, REBALANCE) are handled here.
-///
-/// # Method dispatch
-/// ```text
-/// GET / HEAD   → 302 redirect to volume server (after probe)
-/// PUT          → write to replicas
-/// POST ?uploads      → initiate multipart
-/// POST ?uploadId=X   → complete multipart
-/// POST ?delete       → batch delete
-/// DELETE       → hard delete (requires prior UNLINK if protect=true)
-/// UNLINK       → soft delete
-/// REBALANCE    → move key to ideal volumes
-/// GET ?list    → list active keys
-/// GET ?unlinked→ list soft-deleted keys
-/// ```
-use std::sync::Arc;
+//! HTTP request handlers.
+//!
+//! This module exposes the single entry point for all object routes
+//! (`/*key`) and dispatches based on HTTP method and query parameters.
+//!
+//! The handler coordinates:
+//! - per-key locking for mutating operations,
+//! - metadata reads/writes via `AppState`,
+//! - replica selection and rebalance,
+//! - redirect construction (302 or X-Accel-Redirect),
+//! - multipart upload lifecycle,
+//! - soft and hard deletion.
+//!
+//! ## Method semantics
+//!
+//! GET / HEAD  
+//!     Look up metadata, probe volumes, and redirect to a reachable
+//!     replica. Returns 404 if the key is soft-deleted, hard-deleted,
+//!     or unreachable on all volumes (unless a fallback is configured).
+//!
+//! PUT  
+//!     Writes a new object to the ideal replica set. Overwrites are
+//!     rejected. Also handles multipart part uploads.
+//!
+//! POST ?uploads  
+//!     Initiates a multipart upload and returns an upload ID.
+//!
+//! POST ?uploadId=X  
+//!     Completes a multipart upload by concatenating parts and writing
+//!     the final object to replicas.
+//!
+//! POST ?delete  
+//!     Batch delete under a prefix.
+//!
+//! DELETE  
+//!     Hard delete. If `protect=true`, requires a prior UNLINK.
+//!
+//! UNLINK  
+//!     Soft delete. Metadata remains but the key is treated as absent.
+//!
+//! REBALANCE  
+//!     Moves the object to its ideal replica set if needed.
+//!
+//! GET ?list / ?unlinked  
+//!     Query endpoints for active and soft-deleted keys.
+//!
+//! ## Concurrency model
+//!
+//! Mutating operations acquire a per-key lock using `KeyLock`.
+//! Multipart part uploads are locked per `(key, partNumber)`.
+//!
+//! ## Redirect modes
+//!
+//! - In 302 mode, clients are redirected directly to a volume server.
+//! - In X-Accel-Redirect mode, nginx performs an internal redirect and
+//!   serves the object while preserving coordinator-provided headers.
+
+use crate::http::query::handle_query;
+use crate::http::s3::{CompleteMultipartUpload, Delete};
 
 use axum::body::Body;
 use axum::extract::{Path, RawQuery, State};
 use axum::http::{HeaderMap, Method, StatusCode};
 use axum::response::{IntoResponse, Response};
 use bytes::Bytes;
-use rand::seq::SliceRandom;
-use tracing::{debug, info, instrument, warn};
-use uuid::Uuid;
-
-use crate::http::query::handle_query;
-use crate::http::s3::{CompleteMultipartUpload, Delete};
 use minikv_core::hashing::key_to_path;
 use minikv_core::rebalance::rebalance_key;
 use minikv_core::record::Deleted;
 use minikv_core::replication::remote_head;
 use minikv_core::state::AppState;
 use minikv_core::volumes::key_to_volume;
+use rand::seq::SliceRandom;
+use std::sync::Arc;
+use tracing::{debug, info, instrument, warn};
+use uuid::Uuid;
 
 /// Top-level axum handler for all routes (`/*key`).
 ///
@@ -187,7 +226,7 @@ async fn handle_get_head(state: &AppState, key: &[u8], _method: &Method) -> Resp
     // are discarded by nginx unless we use the variable persistence trick:
     //
     //   1. We set X-Content-Type on the coordinator response.
-    //   2. nginx captures it as $upstream_http_x_content_type — this variable
+    //   2. nginx captures it as $upstream_http_x_content_type. This variable
     //      persists across the internal redirect (same ngx_http_request_t).
     //   3. The internal /accel/ location uses:
     //        proxy_hide_header Content-Type;
@@ -203,7 +242,7 @@ async fn handle_get_head(state: &AppState, key: &[u8], _method: &Method) -> Resp
         resp_builder = resp_builder
             .header(axum::http::header::CONTENT_TYPE, ct.as_str())
             // X-Content-Type persists as $upstream_http_x_content_type
-            // across nginx's internal redirect — see nginx-frontend.conf.
+            // across nginx's internal redirect. See nginx-frontend.conf.
             .header("X-Content-Type", ct.as_str());
     }
 

minikv/src/http/mod.rs

@@ -1,3 +1,17 @@
+//! HTTP API wiring for the minikv coordinator.
+//!
+//! This crate provides the Axum router and request entry points that
+//! expose the minikv HTTP interface.
+//!
+//! All object paths are routed through a single catch-all handler
+//! (`/*key`). The handler internally dispatches based on HTTP method
+//! and query parameters, including support for non-standard methods
+//! such as `UNLINK` and `REBALANCE`.
+//!
+//! `build_router` constructs the `Router` and attaches shared
+//! `AppState`, which contains configuration, metadata store access,
+//! locking, and volume topology.
+
 pub mod handlers;
 pub mod query;
 pub mod s3;

minikv/src/http/query.rs

@@ -1,12 +1,27 @@
-/// Query parameter parsing and list/unlinked operations.
-///
-/// Handles GET requests with a query string, which are routing differently
-/// from plain GET (redirect) requests.
-///
-/// Supported operations:
-/// - `?list[&start=X][&limit=N]`       list active keys under prefix
-/// - `?unlinked[&start=X][&limit=N]`   list soft-deleted keys
-/// - `?list-type=2&prefix=X`           S3-style listing
+//! Query handling for list-style operations.
+//!
+//! This module processes GET requests that include a query string.
+//! Plain GET requests (without a query) are handled elsewhere and
+//! result in object redirects.
+//!
+//! Supported query forms:
+//!
+//! - `?list[&start=X][&limit=N]`
+//!     Lists active (non-deleted) keys under the provided prefix.
+//!
+//! - `?unlinked[&start=X][&limit=N]`
+//!     Lists soft-deleted keys under the provided prefix.
+//!
+//! - `?list-type=2&prefix=X`
+//!     Provides an S3-compatible XML listing.
+//!
+//! Listing is prefix-based and backed by a metadata scan (`scan_prefix`).
+//! Results are filtered by deletion state at decode time.
+//!
+//! `start` acts as a cursor (lexicographic lower bound).
+//! `limit` bounds the number of returned keys. A hard cap of
+//! `MAX_KEYS` prevents unbounded responses.
+
 use std::sync::Arc;
 
 use axum::http::StatusCode;
@@ -17,30 +32,46 @@ use tracing::debug;
 use minikv_core::record::Deleted;
 use minikv_core::state::AppState;
 
-/// JSON response for list operations.
+/// JSON response body returned by `?list` and `?unlinked`.
+///
+/// `next` is a continuation cursor. It is empty if no further
+/// results are available.
+///
+/// `keys` contains UTF-8 representations of matching object keys.
 #[derive(Debug, Serialize)]
 pub struct ListResponse {
     pub next: String,
     pub keys: Vec<String>,
 }
 
-/// Query parameters common to list and unlinked operations.
+/// Optional query parameters used by `?list` and `?unlinked`.
+///
+/// `start` is a lexicographic cursor. Keys strictly smaller than
+/// this value are skipped.
+///
+/// `limit` bounds the number of keys returned. If omitted,
+/// all matching keys up to `MAX_KEYS` may be returned.
 #[allow(unused)]
 #[derive(Debug, Deserialize)]
 pub struct ListParams {
     pub start: Option<String>,
     pub limit: Option<usize>,
 }
 
-/// Maximum number of keys returned in a single list response before a
-/// 413 (Payload Too Large) is returned.  Matches Go's hard limit of 1,000,000.
+/// Absolute upper bound on keys returned in a single response.
+///
+/// If this limit is exceeded during iteration, the request
+/// fails with `413 Payload Too Large`.
 const MAX_KEYS: usize = 1_000_000;
 
-/// Handle a GET request that has a non-empty query string.
+/// Entry point for GET requests with a non-empty query string.
 ///
-/// Dispatches to:
-/// - S3-style listing (`?list-type=2`)
-/// - Our own `?list` / `?unlinked` operations
+/// Dispatches based on query parameters:
+///
+/// - `?list-type=2` → S3-compatible XML listing
+/// - `?list` / `?unlinked` → JSON listing
+///
+/// Any other query results in `403 Forbidden`.
 pub async fn handle_query(state: Arc<AppState>, key_prefix: &[u8], raw_query: &str) -> Response {
     // S3-style listing: ?list-type=2&prefix=...
     if raw_query.contains("list-type=2") {
@@ -56,7 +87,15 @@ pub async fn handle_query(state: Arc<AppState>, key_prefix: &[u8], raw_query: &s
     }
 }
 
-/// Handle `?list` and `?unlinked` queries.
+/// Handles `?list` and `?unlinked` queries.
+///
+/// Performs a prefix scan over metadata, applies cursor and limit,
+/// filters by deletion state, and returns a JSON response.
+///
+/// Returns:
+/// - `200 OK` with JSON body on success
+/// - `413 Payload Too Large` if `MAX_KEYS` is exceeded
+/// - `500 Internal Server Error` on metadata or encoding failure
 async fn handle_list(
     state: Arc<AppState>,
     key_prefix: &[u8],
@@ -131,7 +170,13 @@ async fn handle_list(
         .into_response()
 }
 
-/// Handle `?list-type=2` S3-style listing.
+/// Handles `?list-type=2` S3-style listing.
+///
+/// Extends the provided prefix with the S3 `prefix` parameter,
+/// scans metadata, filters out deleted records, and returns
+/// a minimal XML response compatible with S3 clients.
+///
+/// Only active (non-deleted) keys are included.
 async fn handle_s3_list(state: Arc<AppState>, key_prefix: &[u8], raw_query: &str) -> Response {
     // Append the S3 `prefix` parameter to our key prefix.
     let s3_prefix = extract_param(raw_query, "prefix").unwrap_or_default();
@@ -170,7 +215,12 @@ async fn handle_s3_list(state: Arc<AppState>, key_prefix: &[u8], raw_query: &str
         .into_response()
 }
 
-/// Extract a named query parameter from a raw query string.
+/// Extracts a query parameter from a raw query string.
+///
+/// The query string is not URL-decoded. This function performs
+/// a simple `key=value` match split by `&`.
+///
+/// Returns `None` if the parameter is not present.
 fn extract_param(raw_query: &str, name: &str) -> Option<String> {
     for part in raw_query.split('&') {
         if let Some((k, v)) = part.split_once('=')

minikv/src/http/s3.rs

@@ -1,10 +1,11 @@
-/// S3-compatible XML request body parsing.
-///
-/// Handles two S3 API shapes:
-/// - `CompleteMultipartUpload`     finalize a multipart upload.
-/// - `Delete`                      batch-delete multiple objects.
-///
-/// Uses `quick-xml` with serde for zero-copy XML deserialization.
+//! S3-compatible XML request body parsing.
+//!
+//! Handles two S3 API shapes:
+//! - `CompleteMultipartUpload`     finalize a multipart upload.
+//! - `Delete`                      batch-delete multiple objects.
+//!
+//! Uses `quick-xml` with serde for zero-copy XML deserialization.
+
 use serde::Deserialize;
 
 use crate::error::Error;

minikv/tests/integration.rs

@@ -1,13 +1,9 @@
+//! Integration tests
+//!
+//! Each test spins up a real axum server with an in-memory metadata store
+//! and wiremock volume servers, then exercises the full HTTP flow.
+
 use std::collections::{BTreeMap, HashMap};
-/// Integration tests
-///
-/// Each test spins up a real axum server with an in-memory metadata store
-/// and wiremock volume servers, then exercises the full HTTP flow.
-///
-/// # Running
-/// ```sh
-/// cargo test --test integration
-/// ```
 use std::sync::Arc;
 use std::time::Duration;