# Runtime Health & Robustness Review

> **Project:** Theseus — Rust axum server, tokio runtime, SQLite, WebSocket
> **Date:** 2025-01-XX
> **Scope:** Production readiness, failure handling, observability, operational safety

---

## Executive Summary

Theseus is a well-structured single-binary attack path visualization tool with solid error handling at the API layer, but **lacks critical runtime resilience patterns** for production deployment. The most significant gaps are: no timeouts on any I/O operations, zero retry logic, no graceful shutdown, and a single-mutex database bottleneck. For a local development/pentest tool this is acceptable; for multi-user or long-running deployments it needs hardening.

**Overall Grade: C+**

---

## 1. Timeout Handling — D

**Finding: No timeouts configured on any I/O path.**

| Component | Timeout | Source |
|-----------|---------|--------|
| `deepseek_chat()` reqwest client | ❌ None (default = no timeout) | `src/lib.rs:563` — `reqwest::Client::new()` |
| `LlmClient` (agent) reqwest client | ❌ None (default = no timeout) | `src/agent/llm.rs:17` — `Client::new()` |
| CLI `build_http_client()` | ❌ None | `src/main.rs:704-706` |
| SQLite connection | ❌ No busy timeout | `src/db.rs:22-23` |
| WebSocket connections | ❌ No idle/ping timeout | `src/routes/ws.rs:19-36` |
| axum server bind | ❌ Panics on failure | `src/lib.rs:885-889` |

**Evidence:**
```rust
// lib.rs:563 — new client per request, no timeout
let client = reqwest::Client::new();

// llm.rs:16-21 — same pattern in agent
client: Client::new(),  // no .timeout() or .connect_timeout()

// main.rs:704-706 — CLI client also bare
Ok(reqwest::Client::builder()
    .default_headers(headers)
    .build()?)  // no timeout configured

// db.rs:22-23 — no busy_timeout pragma
let conn = Connection::open(path)?;
conn.execute_batch("PRAGMA journal_mode=WAL; PRAGMA foreign_keys=ON;");
// Missing: PRAGMA busy_timeout = 5000;
```

**Risk:** A hung DeepSeek API call blocks the request handler indefinitely. Under the `Mutex<Connection>` model, a slow query blocks all other requests. WebSocket connections can accumulate if clients disappear without closing.

**Recommendations:**
1. Set `reqwest::Client::builder().timeout(Duration::from_secs(60)).connect_timeout(Duration::from_secs(10))` on all HTTP clients.
2. Add `PRAGMA busy_timeout = 5000;` to `Database::open()`.
3. Implement WebSocket ping/pong with idle timeout (e.g., 60s no-message → close).
4. Use `tokio::time::timeout()` around the agent LLM loop turn (long-running calls).

---

## 2. Retry & Backoff — F

**Finding: Zero retry logic anywhere in the codebase.**

No matches for `retry`, `backoff`, `exponential`, or `retries` in the source tree.

**Evidence:**
```rust
// lib.rs:578-584 — DeepSeek 429 rate limit → immediate 500, no retry
429 => StatusCode::INTERNAL_SERVER_ERROR, // rate limited

// llm.rs:66 — agent LLM call fails → immediate error propagation
.map_err(|e| LlmError::RequestFailed(e.to_string()))?;
```

Both `deepseek_chat()` and `LlmClient::chat()` treat transient failures (network blip, 429 rate limit, 502/503 gateway errors) identically to permanent failures. The agent loop (`loop.rs:85-89`) propagates LLM errors immediately — one failed turn kills the entire agent run.

**Risk:** DeepSeek API is a third-party service with documented rate limits. Transient failures will cause user-facing errors with no recovery path. The agent subsystem is especially vulnerable since a multi-turn conversation is lost on any single API failure.

**Recommendations:**
1. Add retry with exponential backoff (max 3 attempts) for 429, 502, 503, 504 on `deepseek_chat()`.
2. Add retry in `LlmClient::chat()` with `tokio::time::sleep` backoff.
3. In the agent loop, retry the current turn before failing the entire run.
4. Consider using the `reqwest-retry` + `reqwest-middleware` crates.

---

## 3. Logging & Observability — B+

**Finding: Solid structured logging with appropriate levels. Some gaps.**

**What's good:**
- `tracing_subscriber` initialized with env filter (`theseus_server=debug,tower_http=debug`) — `lib.rs:784-787`
- `TraceLayer` on all HTTP requests with INFO-level spans — `lib.rs:878-881`
- `AppError::log()` uses structured fields with severity mapping — `lib.rs:255-308`:
  - 5xx → `ERROR` with backtrace capture
  - 409 Conflict → `WARN`
  - Other 4xx → `INFO`
- API key presence logged without exposing the key — `lib.rs:798-805`
- CORS configuration logged — `lib.rs:842-858`
- Agent errors logged at route handler — `routes/agent.rs:18,36`
- DeepSeek API errors logged with status and body — `lib.rs:571,578,588`

**Concerns:**
- DeepSeek API error bodies logged at ERROR level (`lib.rs:578`) may contain sensitive data from the API response.
- No request ID or correlation ID for tracing individual requests across logs.
- No metrics endpoint (Prometheus, etc.) for operational monitoring.
- `hint` field in `AppError` could leak internal details to logs (currently safe but fragile).

**Recommendations:**
1. Add request ID middleware (e.g., `tower-http::request_id`) for distributed tracing.
2. Truncate DeepSeek API error bodies before logging.
3. Add a `/metrics` endpoint if deploying beyond local use.
4. Consider log redaction for `hint` field if it ever includes user input.

---

## 4. Concurrency Safety — B-

**Finding: Correct but not scalable database access pattern.**

**What's good:**
- `Mutex<Connection>` with poison recovery — `db.rs:17-18`:
  ```rust
  fn c(&self) -> std::sync::MutexGuard<'_, Connection> {
      self.conn.lock().unwrap_or_else(|e| e.into_inner())
  }
  ```
- WAL mode enabled for better concurrent read performance — `db.rs:23`
- Foreign keys enforced — `db.rs:23`
- `broadcast::channel(256)` for WebSocket fan-out — `lib.rs:795`

**Concerns:**
- Single `std::sync::Mutex` serializes ALL database operations. One slow query (e.g., BFS analysis on large graph) blocks every other request. `tokio::sync::Mutex` would at least not block the tokio runtime thread, but connection pooling (r2d2/deadpool) would be better.
- `broadcast::channel(256)` — if more than 256 messages queue before a slow consumer reads, older messages are silently dropped. No logging when this happens.
- `Box::leak` for `Database` is intentional and fine for a single-process tool, but prevents graceful DB cleanup.

**Evidence:**
```rust
// lib.rs:791-793 — leaked for 'static lifetime
let db = Box::leak(Box::new(
    Database::open(db_path).expect("Failed to open database"),
));
```

**Recommendations:**
1. Consider `deadpool-sqlite` or `r2d2` connection pool instead of single-mutex.
2. If keeping single connection, use `tokio::sync::Mutex` to avoid blocking the runtime.
3. Log when broadcast channel drops messages (use `send()` result).

---

## 5. Configuration Validation — B+

**Finding: Graceful degradation with sensible defaults.**

**What's good:**
- Cascading config priority: CLI args → env vars → `./theseus.toml` → `~/.config/theseus/config.toml` — `lib.rs:353-410`
- Empty/whitespace env vars filtered out — `lib.rs:367,372,383,388`
- Invalid TOML silently falls back to defaults — `lib.rs:364`
- CORS invalid origins fall back to localhost — `lib.rs:841-842`
- Missing API keys result in runtime warnings, not crashes — `lib.rs:797-806`
- Endpoints that require API key return clear error messages — `routes/chat.rs:18`, `routes/report.rs:41`

**Concerns:**
- `Database::open().expect("Failed to open database")` panics on DB open failure — `lib.rs:792`. Should fail fast with a clear message.
- `TcpListener::bind(...).await.unwrap()` panics if port is in use — `lib.rs:885-887`.
- No validation that the configured port is in valid range (though `u16` type constrains this).
- Config struct fields are all `Option` — no required fields validated at startup.

**Evidence:**
```rust
// lib.rs:792 — panic on DB failure
Database::open(db_path).expect("Failed to open database"),

// lib.rs:885-887 — panic on bind failure
let listener = tokio::net::TcpListener::bind(format!("0.0.0.0:{port}"))
    .await
    .unwrap();
```

**Recommendations:**
1. Replace `expect`/`unwrap` on DB open and bind with proper error messages + process exit.
2. Log the resolved configuration at startup (mask sensitive values).
3. Consider validating config fields (e.g., warn if CORS origins contain `*`).

---

## 6. Resource Cleanup — C

**Finding: Adequate for short-lived processes, gaps for long-running.**

**What's good:**
- SQLite WAL mode — clean shutdown recovers automatically
- WebSocket handler breaks on send error or close frame — `ws.rs:25,30`
- Evidence file path traversal rejected — `routes/evidence.rs:174`

**Concerns:**
- No graceful shutdown handler (see §7).
- `reqwest::Client::new()` creates a new client per request in `deepseek_chat()` (`lib.rs:563`). Each call allocates a new connection pool that is never explicitly closed. Should reuse a shared client.
- `LlmClient` creates one client per instance (`llm.rs:17`) — better, but agent creates a new `LlmClient` per request (`routes/agent.rs`).
- Evidence files are orphaned when DB records are deleted — documented in `conventions.md` §10, but no cleanup mechanism exists.
- WebSocket handler does not explicitly close the socket on exit — Relies on drop semantics.
- No connection pool for SQLite — single connection held for process lifetime.

**Evidence:**
```rust
// lib.rs:563 — new client per API call (leaks connection pool)
let client = reqwest::Client::new();

// ws.rs:19-36 — no explicit socket.close() on exit
async fn handle_socket(mut socket: WebSocket, state: AppState) {
    // ... loop exits on error/close, socket dropped without explicit close
}
```

**Recommendations:**
1. Create a shared `reqwest::Client` in `AppState` instead of per-request.
2. Add explicit `socket.close()` before exiting WebSocket handler.
3. Add a periodic cleanup job for orphaned evidence files (or document the manual process).
4. Consider `tokio::select!` with a shutdown signal in the WebSocket handler.

---

## 7. Graceful Shutdown — F

**Finding: No graceful shutdown implementation.**

No matches for `shutdown`, `signal`, `SIGTERM`, `SIGINT`, `ctrl_c`, or `graceful` in the source tree.

**Evidence:**
```rust
// lib.rs:885-889 — bare serve, no signal handling
let listener = tokio::net::TcpListener::bind(format!("0.0.0.0:{port}"))
    .await
    .unwrap();
tracing::info!("Theseus server listening on http://0.0.0.0:{port}");
axum::serve(listener, app).await.unwrap();  // blocks forever, no graceful shutdown
```

**Risk:** On SIGTERM/SIGINT, the process terminates immediately. In-flight requests are dropped. WebSocket clients get abrupt disconnection. SQLite WAL checkpoint may not complete (though WAL auto-recovers on next open). Agent runs in progress are lost.

**Recommendations:**
1. Use `axum::serve(listener, app).with_graceful_shutdown(shutdown_signal())`:
   ```rust
   async fn shutdown_signal() {
       let ctrl_c = tokio::signal::ctrl_c();
       let mut sigterm = tokio::signal::unix::signal(SignalKind::terminate()).unwrap();
       tokio::select! {
           _ = ctrl_c => {},
           _ = sigterm.recv() => {},
       }
       tracing::info!("Shutdown signal received, draining connections...");
   }
   ```
2. Set a shutdown timeout (e.g., 30s) for in-flight requests to complete.
3. Log shutdown initiation and completion.

---

## Summary Table

| Criterion | Grade | Severity | Effort |
|-----------|-------|----------|--------|
| Timeout handling | **D** | 🔴 High | Low |
| Retry & backoff | **F** | 🔴 High | Medium |
| Logging & observability | **B+** | 🟢 Low | Low |
| Concurrency safety | **B-** | 🟡 Medium | High |
| Configuration validation | **B+** | 🟢 Low | Low |
| Resource cleanup | **C** | 🟡 Medium | Medium |
| Graceful shutdown | **F** | 🔴 High | Low |

## Priority Fixes (Quick Wins)

1. **Add HTTP client timeouts** — 5 lines of code, prevents indefinite hangs.
2. **Add graceful shutdown** — ~15 lines, prevents data loss on restart.
3. **Reuse `reqwest::Client`** — move to `AppState`, prevents connection pool churn.
4. **Add SQLite busy timeout** — 1 line (`PRAGMA busy_timeout = 5000`).
5. **Replace panic-on-startup with proper errors** — `expect`/`unwrap` → `map_err` + exit.

## Systemic Improvements (Future)

6. **Retry with backoff for DeepSeek API** — critical for agent reliability.
7. **Connection pooling for SQLite** — `deadpool-sqlite` or `r2d2`.
8. **Request ID middleware** — enables request tracing.
9. **WebSocket ping/pong + idle timeout** — prevents zombie connections.
10. **Metrics endpoint** — enables operational monitoring.
