Merge main to trunk (v0.3.0) by lukekim · Pull Request #9 · spiceai/spiceio

lukekim · 2026-04-02T18:53:08Z

Summary

Merge main into trunk using --allow-unrelated-histories -X theirs to bring trunk up to date with all work through v0.3.0.

Includes all changes since trunk diverged:

S3 streaming transfers and sccache integration
SMB2 compounding, small-file fast paths, benchmarks (v0.2.0)
Non-blocking file logging with slog!/serr! macros, timestamps (Add non-blocking file logging and stderr error logging #7)
Version bump to 0.3.0, README updates (Bump version to 0.3.0 for logging release #8)

Test plan

make lint passes (fmt, check, clippy, rustdoc)
cargo test --locked — 200/200 tests pass

- S3 layer: full API router with SigV4 auth, multipart uploads, conditional writes, range reads, ListObjectsV1/V2, CopyObject, multi-object Delete, and stub endpoints for ACL/tagging/versioning - SMB layer: wire protocol client (negotiate, session setup, tree connect, create, read, write, close, query directory, query info) with NTLMv2/SPNEGO authentication - Crypto: macOS CommonCrypto FFI (MD4, MD5, SHA-256, HMAC-MD5, HMAC-SHA256) — zero external crypto dependencies - Makefile with fmt, lint, clippy, test, build targets - Targets macOS 26+, Rust 2024 edition

- add SpioBody for streaming HTTP responses - stream GetObject reads from SMB to HTTP - stream PutObject writes from HTTP to SMB - add sccache integration test script and Makefile target - update docs and dependency metadata

SMB protocol: - Fix SPNEGO DER encoding for payloads >127 bytes - Fix Create/Write request offset calculations (off-by-one) - Fix Create/Write response minimum size checks - Handle STATUS_PENDING by looping for final response - Offer SMB 3.0.0/3.0.2 dialect fallbacks in negotiate - Add NTLMSSP Version field and echo server negotiate flags - Cap max read/write to 64KB to avoid oversized messages SMB3 message signing: - SHA-512 and AES-128-CMAC via CommonCrypto FFI - SP800-108 KDF signing key derivation from session base key - Preauth integrity hash tracking through handshake - Sign all post-auth messages with AES-128-CMAC S3 layer: - Map SMB NotFound → S3 NoSuchKey (404), PermissionDenied → 403 - Streaming PutObject via channel-backed body Test harness: - make test runs sccache integration against SMB share - Cold build populates cache, warm build verifies 100% hit rate

- Rename project, binary, env vars (SPIO_ → SPICEIO_), types (SpioBody → SpiceioBody) - Add 14 AWS CLI tests: ListBuckets, HeadBucket, Put/Get (small/64KB/1MB with integrity), HeadObject, ListObjectsV2, CopyObject, DeleteObject, nested paths, overwrite - Fix QueryDirectory request NameOffset (off-by-one, same as Create/Write) - Test cleanup removes test objects from SMB share

#6) SMB2 compound operations batch multiple SMB operations into single TCP round trips, cutting latency for small file reads (3 RT -> 1 RT), writes (5+ RT -> 2 RT), head/delete (2 RT -> 1 RT), and directory creation (2N RT -> 1 RT). Router fast paths serve small GETs inline via compound Create+Read+Close and small PUTs via compound Create+Write+Close, bypassing the streaming path overhead. Additional changes: - Add --version flag to the binary - Add port-in-use handling to test scripts - Add CI step to verify binary version and kill stale processes - Add extended integration test building spiceai repo through sccache - Add unit tests across all modules (100 total) - Fix parse_http_date misrouting RFC 7231 dates to ISO 8601 parser - Add criterion benchmarks (crypto, protocol, s3) - Optimize epoch_to_iso8601/http_date from 150ns to 18ns (8x) - Add zero-copy decode_read_response_owned for the hot read path - Bump version to 0.2.0, license to Apache 2.0 - Rewrite README with architecture, use cases, and sccache example

Add a lightweight logging system with two macros — slog! (stdout) and serr! (stderr) — that also tee to a log file via SPICEIO_LOG_FILE. The file writer runs on a dedicated OS thread fed by a bounded mpsc channel; try_send ensures logging never blocks the proxy. When no file is configured there is zero overhead (no thread, no channel, no allocation). All log lines are timestamped (ISO-8601 UTC with milliseconds) using gettimeofday — no allocations in the timestamp path. Also adds logging for: - Connection lifecycle (stdout): client TCP accept, SMB TCP connect, negotiate, auth, tree connect - All failure paths (stderr): every SMB wire error, S3 streaming errors, body read errors, access denied, client disconnects

- Bump version to 0.3.0 for the logging release - Add SPICEIO_LOG_FILE to the configuration table - Add non-blocking logging to key highlights - Mention --version flag

Copilot

Pull request overview

Merge main into trunk to bring trunk up to date through v0.3.0, including SMB2 compounding/fast-paths, non-blocking logging, benchmarks, and documentation/versioning updates.

Changes:

Add SMB2 compounding support (Create+Read+Close, Create+Write+Close, Create+Close, batched EnsureDirs) and wire-level protocol helpers (credit charge, close postquery, zero-copy read decode).
Introduce non-blocking logging with slog!/serr! macros and optional file tee via SPICEIO_LOG_FILE, and integrate it across SMB + S3 paths.
Update S3 router behaviors (small-object fast paths, multipart completion flow), add new benches/tests, bump version to 0.3.0, and refresh CI/scripts/docs.

Reviewed changes

Copilot reviewed 22 out of 24 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
`src/smb/protocol.rs`	Protocol parsing/encoding updates, credit-charge helpers, compound constants, new tests.
`src/smb/ops.rs`	Adds compound fast paths for get/put/head/delete/ensure-dirs and temp file helpers.
`src/smb/client.rs`	Adds compound request machinery, per-message signing, more detailed logging, and credit charging.
`src/smb/auth.rs`	Minor doc escaping tweak.
`src/s3/xml.rs`	Optimizes date formatting and adds unit tests.
`src/s3/router.rs`	Adds small-object fast paths, adjusts range handling, multipart completion flow changes, more logging.
`src/s3/multipart.rs`	Refactors constructors and adds store tests.
`src/s3/headers.rs`	Changes range resolution behavior and tightens ISO8601-vs-HTTP-date detection; adds tests.
`src/main.rs`	Moves to local modules, adds `--version`, and introduces exported logging macros + initialization.
`src/log.rs`	New non-blocking logger implementation.
`src/lib.rs`	Exposes logging module and exports logging macros for library consumers/benches/tests.
`src/crypto/ffi.rs`	Micro-optimization in AES-CMAC block processing.
`scripts/test-sccache.sh`	Adds stale-port cleanup and config tweaks for sccache runs.
`scripts/test-sccache-spiceai.sh`	New extended integration test script (optional, local repo-dependent).
`README.md`	Major documentation refresh (features, usage, configuration, dev workflow).
`Makefile`	Adds `test-extended`, removes bench targets from make.
`CLAUDE.md`	Updates documented environment variables and removes bench/checklist sections.
`Cargo.toml`	Bumps version to `0.3.0` and adds `s3_bench` benchmark target.
`Cargo.lock`	Updates dependencies consistent with the merge/version bump.
`benches/s3_bench.rs`	New S3-focused benchmarks.
`benches/protocol_bench.rs`	Restructures protocol benchmarks to match new helpers/paths.
`benches/crypto_bench.rs`	Restructures crypto benchmarks and adds additional coverage.
`.gitignore`	Simplifies ignore rules to `/target`.
`.github/workflows/ci.yml`	Adds binary version check + stale-port cleanup; removes benchmark compilation step.

Comments suppressed due to low confidence (2)

src/s3/router.rs:585

Range handling now assumes range.resolve(file_size) always returns a valid (start,end). With the updated resolve, empty files and unsatisfiable ranges can yield wrapped/invalid values, and content_length = end - start + 1 can overflow. This should return 416 Range Not Satisfiable (with Content-Range: bytes */{file_size}) when the range can’t be satisfied.

    // Determine read range
    let (start, end, is_range) = if let Some(ref range_str) = range_header {
        if let Some(range) = parse_range(range_str) {
            let (s, e) = range.resolve(file_size);
            (s, e, true)
        } else {
            (0, file_size.saturating_sub(1), false)
        }
    } else {
        (0, file_size.saturating_sub(1), false)
    };

    let content_length = end - start + 1;

    // Build response with streaming body

src/s3/router.rs:1049

handle_complete_multipart_upload now calls state.multipart.complete(upload_id) up front, which removes the upload state before assembly succeeds. If any subsequent step fails, the client can’t retry completion or abort/cleanup because the upload has already been consumed. Consider fetching non-destructively first and only removing the upload after the final object write succeeds.

    let upload = match state.multipart.complete(upload_id).await {
        Some(u) => u,
        None => {
            return error_response(
                StatusCode::NOT_FOUND,

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-02T18:59:17Z

+            other => {
+                // Treat unknown as success if zero high-bit, else generic error
+                if other == 0 {
+                    Self::Success
+                } else {


NtStatus::from_u32 now maps every unknown non-zero status to AccessDenied, which discards the actual NTSTATUS value and will misclassify many errors (e.g. sharing violations, invalid parameter, etc.) as access denied. This also makes it impossible to surface the real code in logs/errors. Consider restoring an Unknown(u32) variant (and a raw() accessor) or returning a wrapper type that preserves the original u32 while still offering helpers like is_error().

Copilot · 2026-04-02T18:59:18Z

    pub fn is_error(self) -> bool {
-        let raw = self.raw();
-        raw & 0xC0000000 == 0xC0000000
-    }
-
-    /// Return the raw u32 status code.
-    pub fn raw(self) -> u32 {
-        match self {
-            Self::Success => 0x00000000,
-            Self::MoreProcessingRequired => 0xC0000016,
-            Self::NoSuchFile => 0xC000000F,
-            Self::ObjectNameNotFound => 0xC0000034,
-            Self::ObjectNameCollision => 0xC0000035,
-            Self::AccessDenied => 0xC0000022,
-            Self::EndOfFile => 0xC0000011,
-            Self::NoMoreFiles => 0x80000006,
-            Self::ObjectPathNotFound => 0xC000003A,
-            Self::Unknown(v) => v,
-        }
+        (self as u32) & 0xC0000000 == 0xC0000000
    }


is_error() currently checks severity bits on the enum discriminant (self as u32), but from_u32 no longer preserves unknown raw codes. Even if you keep the enum-only approach, is_error should operate on the original raw status value to avoid incorrect classification and reporting.

Copilot · 2026-04-02T18:59:18Z

    buf.put_u16_le(57); // StructureSize
    buf.put_u8(0); // SecurityFlags
-    buf.put_u8(0x00); // RequestedOplockLevel: SMB2_OPLOCK_LEVEL_NONE
+    buf.put_u8(0x02); // RequestedOplockLevel: SMB2_OPLOCK_LEVEL_NONE = 0, BATCH=0x09, LEASE=0xFF, let's try none


RequestedOplockLevel is set to 0x02 but the comment indicates “NONE = 0”. 0x02 is not “none” in SMB2 and changes caching/locking semantics. If the intent is no oplock, this should be 0x00 (or update both the value and comment to the intended oplock level).

Suggested change

buf.put_u8(0x02); // RequestedOplockLevel: SMB2_OPLOCK_LEVEL_NONE = 0, BATCH=0x09, LEASE=0xFF, let's try none

buf.put_u8(0x00); // RequestedOplockLevel: SMB2_OPLOCK_LEVEL_NONE = 0x00

Copilot · 2026-04-02T18:59:18Z

+    if body.len() < 17 {
+        return None;
+    }
+    let data_offset = (&body[2..3])[0] as usize;


decode_read_response reads data_offset from only one byte (body[2]). In SMB2 READ response, DataOffset is a little-endian u16 at bytes 2..4. This will break if the offset exceeds 255 and is inconsistent with decode_read_response_owned. Parse u16 from body[2..4] here as well.

Suggested change

let data_offset = (&body[2..3])[0] as usize;

let data_offset = u16::from_le_bytes(body[2..4].try_into().unwrap()) as usize;

Copilot · 2026-04-02T18:59:19Z

 pub fn frame_packet(header: &Header, body: &[u8]) -> BytesMut {
    let total = SMB2_HEADER_SIZE + body.len();
-    // NetBIOS session length is 3 bytes (17 + 7 bits), capped at 0x00FFFFFF
-    let netbios_len = (total as u32) & 0x00FF_FFFF;
    let mut buf = BytesMut::with_capacity(4 + total);
-    buf.put_u32(netbios_len); // NetBIOS length (big-endian, no flags)
+    buf.put_u32(total as u32); // NetBIOS length (big-endian, no flags)
    header.encode(&mut buf);


NetBIOS Session Service length is a 24-bit value (high byte contains flags and must typically be 0). Writing total directly as a u32 can set the high byte for payloads > 0x00FF_FFFF (and you currently allow up to 16 MiB). This can produce invalid frames. Mask/validate the length to 24 bits (and ideally error/assert when exceeding 0x00FF_FFFF).

Copilot · 2026-04-02T18:59:19Z

    /// Resolve to absolute byte positions given total file size.
-    /// Returns `None` if the range is not satisfiable (empty file, start >= total, etc.).
-    pub fn resolve(&self, total: u64) -> Option<(u64, u64)> {
-        if total == 0 {
-            return None;
-        }
+    pub fn resolve(&self, total: u64) -> (u64, u64) {
        match (self.start, self.end) {
-            (Some(s), Some(e)) => {
-                if s >= total {
-                    return None;
-                }
-                let end = e.min(total - 1);
-                if s > end {
-                    return None;
-                }
-                Some((s, end))
-            }
-            (Some(s), None) => {
-                if s >= total {
-                    return None;
-                }
-                Some((s, total - 1))
-            }
-            (None, Some(suffix)) => {
-                if suffix == 0 {
-                    return None;
-                }
-                Some((total.saturating_sub(suffix), total - 1))
-            }
-            (None, None) => Some((0, total - 1)),
+            (Some(s), Some(e)) => (s, e.min(total - 1)),
+            (Some(s), None) => (s, total - 1),
+            (None, Some(suffix)) => (total.saturating_sub(suffix), total - 1),
+            (None, None) => (0, total - 1),
        }


RangeSpec::resolve now unconditionally computes total - 1, which underflows for empty objects (total == 0) and no longer detects unsatisfiable ranges (e.g. start >= total, suffix == 0, or start > end). This can lead to wrapped indices and incorrect Content-Length/Content-Range behavior. Consider restoring the previous Option<(u64,u64)> API or returning a Result and handling invalid ranges explicitly.

Copilot · 2026-04-02T18:59:20Z

-            objects.sort_by(|a, b| a.key.cmp(&b.key));
-            common_prefixes.sort();
-
+        Ok((mut objects, common_prefixes)) => {


ListObjects output is no longer sorted lexicographically by key, which breaks S3 listing semantics and can make marker/continuation-token behavior inconsistent across calls. Consider sorting objects by key and common_prefixes before applying pagination/filtering (or ensure the SMB listing is already guaranteed sorted).

Suggested change

Ok((mut objects, common_prefixes)) => {

Ok((mut objects, mut common_prefixes)) => {

// S3 listing semantics require lexicographic ordering before

// marker/start-after/continuation-token filtering and pagination.

objects.sort_by(|a, b| a.key.cmp(&b.key));

common_prefixes.sort();

Copilot · 2026-04-02T18:59:20Z

+        if let Some(part) = upload.parts.get(pn) {
+            match state.share.read_temp(&part.temp_path).await {
+                Ok(data) => final_data.extend_from_slice(&data),
+                Err(e) => {
+                    // Re-register the upload since we consumed it
+                    return io_to_s3_error(&e);
+                }
            }


Missing parts in the completion request are silently ignored (if let Some(part) = ... with no else). This can produce a corrupted final object without returning InvalidPart. The completion flow should validate that every requested PartNumber exists (and likely that ETags match) and fail the request if any part is missing.

Suggested change

if let Some(part) = upload.parts.get(pn) {

match state.share.read_temp(&part.temp_path).await {

Ok(data) => final_data.extend_from_slice(&data),

Err(e) => {

// Re-register the upload since we consumed it

return io_to_s3_error(&e);

}

}

let part = match upload.parts.get(pn) {

Some(part) => part,

None => {

return error_response(

StatusCode::BAD_REQUEST,

"InvalidPart",

&format!("The specified part does not exist. PartNumber: {}", pn),

);

}

};

match state.share.read_temp(&part.temp_path).await {

Ok(data) => final_data.extend_from_slice(&data),

Err(e) => {

// Re-register the upload since we consumed it

return io_to_s3_error(&e);

}

Copilot · 2026-04-02T18:59:20Z

        Ok(ObjectMeta {
            size: data.len() as u64,
-            last_modified: meta.last_modified,
-            etag: meta.etag,
+            last_modified: std::time::SystemTime::now()
+                .duration_since(std::time::UNIX_EPOCH)
+                .unwrap_or_default()
+                .as_secs(),


For large sequential writes, put_object now returns last_modified based on local SystemTime::now(), but head_object/get_object/list_objects derive timestamps from SMB last_write_time. This makes the returned metadata inconsistent with subsequent HEAD/GET responses. Prefer using server-reported metadata (e.g. Close with POSTQUERY_ATTRIB, or a follow-up head) if callers rely on these fields.

Copilot · 2026-04-02T18:59:21Z

+            etag: format!("{:016x}", simple_hash(data)),
            content_type: guess_content_type(key),


For large sequential writes, put_object switches the ETag scheme to simple_hash(data), while other operations derive ETag from SMB last_write_time. This can cause the ETag returned by PUT/Copy/CompleteMultipartUpload to differ from later HEAD/GET ETags for the same object. To preserve consistency, keep a single ETag strategy (ideally based on server metadata) across all code paths.

lukekim added 11 commits March 29, 2026 21:01

Add CLAUDE.md with project guidance for Claude Code

13e868e

Add design principles: reliability, performance, macOS 26+, SMB 3.1.x

cf4696a

Add README with usage, configuration, and build instructions

c5e69a3

Add streaming S3 transfers and sccache test flow

3caccc2

- add SpioBody for streaming HTTP responses - stream GetObject reads from SMB to HTTP - stream PutObject writes from HTTP to SMB - add sccache integration test script and Makefile target - update docs and dependency metadata

Bump version to 0.3.0 and update README for logging (#8)

4e7cab3

- Bump version to 0.3.0 for the logging release - Add SPICEIO_LOG_FILE to the configuration table - Add non-blocking logging to key highlights - Mention --version flag

Merge remote-tracking branch 'upstream/main' into merge-main-to-trunk

2987576

Copilot AI review requested due to automatic review settings April 2, 2026 18:53

lukekim closed this Apr 2, 2026

Copilot started reviewing on behalf of lukekim April 2, 2026 18:53 View session

Copilot AI reviewed Apr 2, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge main to trunk (v0.3.0)#9

Merge main to trunk (v0.3.0)#9
lukekim wants to merge 11 commits into
trunkfrom
merge-main-to-trunk

lukekim commented Apr 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	buf.put_u8(0x02); // RequestedOplockLevel: SMB2_OPLOCK_LEVEL_NONE = 0, BATCH=0x09, LEASE=0xFF, let's try none
	buf.put_u8(0x00); // RequestedOplockLevel: SMB2_OPLOCK_LEVEL_NONE = 0x00

	let data_offset = (&body[2..3])[0] as usize;
	let data_offset = u16::from_le_bytes(body[2..4].try_into().unwrap()) as usize;

		etag: format!("{:016x}", simple_hash(data)),
		content_type: guess_content_type(key),

Conversation

lukekim commented Apr 2, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants