[rust-expert] Rust Code Audit: Anti-patterns and Safety Issues

[agentic-workflows-automation]

Rust Code Audit Summary

Repository: strawgate/fastforward
Commit: 778f46e
Files audited: 18 crates across crates/ directory
Crates analyzed: ffwd-config (library), ffwd-io (library), ffwd-output (library), ffwd-core (library), ffwd-types (library), ffwd (binary), ffwd-transform (library), ffwd-runtime (library), ffwd-bench (binary), ffwd-kani (library), ffwd-lint-attrs (library), ffwd-ebpf-proto (library), ffwd-config-wasm (library), ffwd-test-utils (library), ffwd-arrow (library), ffwd-proto-build (build), ffwd-otap-proto (library)

Issues found: 3 Medium severity, 1 Low severity

---

Medium Severity

1. Generic unsafe SAFETY Comments in Format Processing

File: crates/ffwd-io/src/format.rs (lines 280+)

Problem: The write_cri_message function and related format processing use unsafe blocks for Arrow value_unchecked access with a generic SAFETY comment:

// SAFETY: row < arr.len() is asserted at function entry (debug) / guaranteed by caller
let v = unsafe { arr.as_primitive::<arrow::datatypes::Int64Type>().value_unchecked(row) };

This comment does not name the specific invariant being upheld. The undocumented_unsafe_blocks lint is enabled in workspace lints (lints.rust config), which should trigger a deny on this pattern.

Fix: Add a # Safety rustdoc section to the function documenting:

1. Caller must guarantee row < arr.len()
2. The array must be non-null at the given row
3. For typed fast path: caller must use TypedArrayRef (non-Other variant)

---

2. .expect() in Construction-Time Code That Can Panic in Production

File: crates/ffwd-io/src/enrichment.rs

Lines: ~150 (HostInfoTable::new) and ~290 (build_k8s_batch)

Problem: Both HostInfoTable::new() and build_k8s_batch() use .expect() on RecordBatch::try_new:

// enrichment.rs:150
let batch = RecordBatch::try_new(schema, columns)
    .expect("host_info schema mismatch");

// enrichment.rs:290
RecordBatch::try_new(schema, arrays)
    .expect("k8s batch schema mismatch")

While these are construction-time operations (one-time initialization), .expect() will panic in production if the hardcoded schema is wrong. This contradicts the codebase's otherwise rigorous error handling.

Fix: Replace with proper error propagation:

let batch = RecordBatch::try_new(schema, columns)
    .map_err(|e| InputError::Config(format!("host_info schema invalid: {e}")))?;

Alternatively, since HostInfoTable::new() returns Self (not Result<Self>), consider making these fallible or documenting why the panic is acceptable.

---

3. .expect() with Unenforced Guard in Validation Logic

File: crates/ffwd-config/src/validate.rs (lines 66-68)

Problem:

} else if all_errors.len() == 1 {
    Err(ConfigError::Validation(
        all_errors
            .into_iter()
            .next()
            .expect("guarded by len == 1 check"),
    ))

The comment "guarded by len == 1 check" is a programmer assertion, not a compiler-enforced invariant. If the length check logic changes, this becomes a panic.

Fix: Use .pop() instead, which handles empty vectors safely:

} else if all_errors.len() == 1 {
    Err(ConfigError::Validation(
        all_errors.into_iter().next().unwrap(), // safe: length == 1
    ))

Or restructure with if let Some(err) = all_errors.into_iter().next().

---

Low Priority

4. String Used for TLS File Paths Instead of PathBuf

File: crates/ffwd-io/src/input.rs (lines ~130)

Problem: TlsInputConfig uses Option<String> for file paths:

pub struct TlsInputConfig {
    pub cert_file: Option<String>,
    pub key_file: Option<String>,
    pub client_ca_file: Option<String>,
    pub require_client_auth: bool,
}

Using String for file paths gives no compile-time guarantee about what the value represents and requires conversion at use site.

Fix: Use Option<PathBuf> for file paths:

pub struct TlsInputConfig {
    pub cert_file: Option<PathBuf>,
    pub key_file: Option<PathBuf>,
    pub client_ca_file: Option<PathBuf>,
    pub require_client_auth: bool,
}

---

Positive Findings

The codebase demonstrates exemplary Rust practices:

1. Kani Formal Verification: Extensive Kani proofs for critical functions (verify_write_cri_message_ends_with_newline, etc.) - exceptional for a production project.

2. Thread Safety: Properly uses Arc<RwLock<...>> for shared enrichment data. Journal struct correctly uses PhantomData<Rc<()>> to prevent Send/Sync on FFI handles.

3. Ownership Patterns: Function signatures consistently use &str, &[T], &Path where appropriate. No &String, &Vec<T>, or &PathBuf anti-patterns found.

4. Error Handling: InputError, ConfigError, TransformError are well-structured error types with context. Library crate (ffwd-config) properly uses thiserror.

5. Pattern Matching: Consistent use of if let Some(x) = opt and idiomatic match expressions. No is_some() { unwrap() } anti-patterns.

6. Iterator Usage: No .collect::<Vec<_>>() immediately followed by .iter() patterns found. State reuse patterns (like StreamingClassifier) are well-implemented.

7. FFI Safety: journal_ffi.rs has excellent module-level SAFETY documentation explaining the thread confinement contract.

8. Documentation: Comprehensive module-level comments explain design decisions, zero-copy guarantees, and memory ownership.

9. Test Coverage: Proptest property-based tests for parsers, TLA+ state machine verification, and extensive regression tests.

---

Recommended Actions

1. Add # Safety rustdoc to public unsafe fn in format.rs for Arrow operations
2. Replace .expect() in HostInfoTable::new() and build_k8s_batch() with proper error propagation
3. Replace .expect() in validate.rs with .unwrap() or restructure to avoid the assertion
4. Consider changing TlsInputConfig file path fields from Option<String> to Option<PathBuf>
5. Verify undocumented_unsafe_blocks lint is properly catching the generic SAFETY comments in format.rs (may need #[allow] with explanation if the pattern is intentional for performance)

Generated by Rust Expert · ● 1.6M · ◷

• expires on May 12, 2026, 10:49 PM UTC