fix: address second-round review — transactional delete, overflow, error logging

- C1: Wrap PostgreSQL delete_user() in a transaction so partial cleanup
  can't leave users in a half-deleted state
- M2: Add job_events to delete cleanup (both backends) — FK to
  agent_jobs without CASCADE would cause FK violation
- H1/M4: Cap expires_in_days to 36500 before i64 cast (tokens + secrets)
- H2: Validate target user exists before creating admin token to prevent
  orphan tokens on libSQL
- H3: Log DB errors in DbAuthenticator::authenticate() instead of
  silently swallowing them as 401

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

Cargo.lock

@@ -7027,6 +7027,7 @@ dependencies = [
  "futures-util",
  "http 1.4.0",
  "http-body 1.0.1",
+ "http-body-util",
  "iri-string",
  "pin-project-lite",
  "tower 0.5.3",

src/channels/web/auth.rs

@@ -172,7 +172,14 @@ impl DbAuthenticator {
         }
 
         // Cache miss or expired — query DB
-        let (token_record, user_record) = self.store.authenticate_token(&hash).await.ok()??;
+        let (token_record, user_record) = match self.store.authenticate_token(&hash).await {
+            Ok(Some(pair)) => pair,
+            Ok(None) => return None,
+            Err(e) => {
+                tracing::warn!(error = %e, "DB auth lookup failed");
+                return None;
+            }
+        };
 
         let identity = UserIdentity {
             user_id: user_record.id.clone(),

src/channels/web/handlers/secrets.rs

@@ -49,6 +49,7 @@ pub async fn secrets_put_handler(
     let expires_at = body
         .get("expires_in_days")
         .and_then(|v| v.as_u64())
+        .map(|d| d.min(36500))
         .map(|days| chrono::Utc::now() + chrono::Duration::days(days as i64));
 
     let mut params = CreateSecretParams::new(name.clone(), value);

src/channels/web/handlers/tokens.rs

@@ -34,7 +34,10 @@ pub async fn tokens_create_handler(
         ))?
         .to_string();
 
-    let expires_in_days = body.get("expires_in_days").and_then(|v| v.as_u64());
+    let expires_in_days = body
+        .get("expires_in_days")
+        .and_then(|v| v.as_u64())
+        .map(|d| d.min(36500));
 
     let expires_at =
         expires_in_days.map(|days| chrono::Utc::now() + chrono::Duration::days(days as i64));
@@ -57,6 +60,18 @@ pub async fn tokens_create_handler(
         .filter(|_| user.role == "admin")
         .unwrap_or(&user.user_id);
 
+    // Verify the target user exists to prevent orphan tokens.
+    if target_user != user.user_id {
+        store
+            .get_user(target_user)
+            .await
+            .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?
+            .ok_or((
+                StatusCode::BAD_REQUEST,
+                format!("Target user '{target_user}' not found"),
+            ))?;
+    }
+
     let record = store
         .create_api_token(target_user, &name, &hash, token_prefix, expires_at)
         .await

src/channels/web/server.rs

@@ -642,7 +642,9 @@ pub async fn start_server(
                 axum::http::Response::builder()
                     .status(axum::http::StatusCode::INTERNAL_SERVER_ERROR)
                     .header("content-type", "text/plain")
-                    .body(axum::body::Body::from(format!("Internal Server Error: {detail}")))
+                    .body(axum::body::Body::from(format!(
+                        "Internal Server Error: {detail}"
+                    )))
                     .unwrap_or_else(|_| {
                         axum::http::Response::new(axum::body::Body::from("Internal Server Error"))
                     })

src/db/libsql/users.rs

@@ -403,7 +403,6 @@ impl UserStore for LibSqlBackend {
             "wasm_tools",
             "routines",
             "memory_documents",
-            "agent_jobs",
             "conversations",
             "api_tokens",
         ] {
@@ -414,6 +413,16 @@ impl UserStore for LibSqlBackend {
             .await
             .map_err(|e| DatabaseError::Query(e.to_string()))?;
         }
+        // job_events references agent_jobs(id) without CASCADE — delete via subquery.
+        conn.execute(
+            "DELETE FROM job_events WHERE job_id IN (SELECT id FROM agent_jobs WHERE user_id = ?1)",
+            params![id],
+        )
+        .await
+        .map_err(|e| DatabaseError::Query(e.to_string()))?;
+        conn.execute("DELETE FROM agent_jobs WHERE user_id = ?1", params![id])
+            .await
+            .map_err(|e| DatabaseError::Query(e.to_string()))?;
         // Nullify self-referencing created_by before deleting the user
         conn.execute(
             "UPDATE users SET created_by = NULL WHERE created_by = ?1",

src/history/store.rs

@@ -2540,12 +2540,18 @@ impl Store {
     /// Delete a user and all their data across all user-scoped tables.
     /// Returns false if the user doesn't exist.
     pub async fn delete_user(&self, id: &str) -> Result<bool, DatabaseError> {
-        let conn = self.conn().await?;
+        let mut conn = self.conn().await?;
+        let tx = conn
+            .transaction()
+            .await
+            .map_err(|e| DatabaseError::Query(e.to_string()))?;
         // Delete from child tables first to avoid FK violations.
-        // agent_jobs cascades to job_actions, llm_calls, estimation_snapshots
-        // conversations cascades to conversation_messages
-        // memory_documents cascades to memory_chunks
-        // routines cascades to routine_runs
+        // job_events must come before agent_jobs (FK without CASCADE).
+        // agent_jobs cascades to job_actions, llm_calls, estimation_snapshots.
+        // conversations cascades to conversation_messages.
+        // memory_documents cascades to memory_chunks.
+        // routines cascades to routine_runs.
+        // api_tokens cascade automatically via FK on users.
         for table in &[
             "settings",
             "heartbeat_state",
@@ -2556,22 +2562,37 @@ impl Store {
             "wasm_tools",
             "routines",
             "memory_documents",
-            "agent_jobs",
             "conversations",
         ] {
-            conn.execute(&format!("DELETE FROM {} WHERE user_id = $1", table), &[&id])
-                .await?;
+            tx.execute(&format!("DELETE FROM {table} WHERE user_id = $1"), &[&id])
+                .await
+                .map_err(|e| DatabaseError::Query(e.to_string()))?;
         }
+        // job_events references agent_jobs(id) without CASCADE — delete via subquery.
+        tx.execute(
+            "DELETE FROM job_events WHERE job_id IN (SELECT id FROM agent_jobs WHERE user_id = $1)",
+            &[&id],
+        )
+        .await
+        .map_err(|e| DatabaseError::Query(e.to_string()))?;
+        tx.execute("DELETE FROM agent_jobs WHERE user_id = $1", &[&id])
+            .await
+            .map_err(|e| DatabaseError::Query(e.to_string()))?;
         // Nullify self-referencing created_by before deleting the user
-        conn.execute(
+        tx.execute(
             "UPDATE users SET created_by = NULL WHERE created_by = $1",
             &[&id],
         )
-        .await?;
+        .await
+        .map_err(|e| DatabaseError::Query(e.to_string()))?;
         // api_tokens cascade automatically via FK
-        let result = conn
+        let result = tx
             .execute("DELETE FROM users WHERE id = $1", &[&id])
-            .await?;
+            .await
+            .map_err(|e| DatabaseError::Query(e.to_string()))?;
+        tx.commit()
+            .await
+            .map_err(|e| DatabaseError::Query(e.to_string()))?;
         Ok(result > 0)
     }