From cb4cf123c68721b17f541affd7129bc433406ee3 Mon Sep 17 00:00:00 2001 From: "ilblackdragon@gmail.com" Date: Sat, 28 Mar 2026 12:12:41 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20address=20review=20feedback=20=E2=80=94?= =?UTF-8?q?=20transaction=20safety,=20patch=20mode,=20formatting?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Wrap libSQL save_version in a transaction to prevent race condition where concurrent writers could allocate the same version number - Make content optional in memory_write when in patch mode (old_string present) — LLM no longer forced to provide unused content param - Improve metadata update error handling with explicit match arms - Run cargo fmt across all files Co-Authored-By: Claude Opus 4.6 (1M context) --- src/db/libsql/workspace.rs | 94 +++++++++++++++++++++++++++---------- src/tools/builtin/memory.rs | 26 ++++++---- src/workspace/document.rs | 10 +++- src/workspace/hygiene.rs | 16 ++----- 4 files changed, 97 insertions(+), 49 deletions(-) diff --git a/src/db/libsql/workspace.rs b/src/db/libsql/workspace.rs index 15ab6bb6..1d8bc261 100644 --- a/src/db/libsql/workspace.rs +++ b/src/db/libsql/workspace.rs @@ -855,9 +855,10 @@ impl WorkspaceStore for LibSqlBackend { reason: e.to_string(), })?; let now = fmt_ts(&Utc::now()); - let meta_str = serde_json::to_string(metadata).map_err(|e| WorkspaceError::SearchFailed { - reason: format!("Failed to serialize metadata: {e}"), - })?; + let meta_str = + serde_json::to_string(metadata).map_err(|e| WorkspaceError::SearchFailed { + reason: format!("Failed to serialize metadata: {e}"), + })?; conn.execute( "UPDATE memory_documents SET metadata = ?2, updated_at = ?3 WHERE id = ?1", params![id.to_string(), meta_str, now], @@ -899,9 +900,13 @@ impl WorkspaceStore for LibSqlBackend { })?; let mut docs = Vec::new(); - while let Some(row) = rows.next().await.map_err(|e| WorkspaceError::SearchFailed { - reason: format!("Failed to read config document row: {e}"), - })? { + while let Some(row) = rows + .next() + .await + .map_err(|e| WorkspaceError::SearchFailed { + reason: format!("Failed to read config document row: {e}"), + })? + { docs.push(row_to_memory_document(&row)); } Ok(docs) @@ -926,8 +931,17 @@ impl WorkspaceStore for LibSqlBackend { let doc_id = document_id.to_string(); let now = fmt_ts(&Utc::now()); - // Get next version number - let mut rows = conn + // Use a transaction to prevent race conditions: the SELECT and INSERT + // must be atomic so concurrent writers don't allocate the same version. + let tx = conn + .transaction() + .await + .map_err(|e| WorkspaceError::SearchFailed { + reason: format!("Failed to start transaction: {e}"), + })?; + + // Get next version number (inside transaction — serializes writers) + let mut rows = tx .query( "SELECT COALESCE(MAX(version), 0) + 1 FROM memory_document_versions WHERE document_id = ?1", params![doc_id.clone()], @@ -937,27 +951,45 @@ impl WorkspaceStore for LibSqlBackend { reason: format!("Failed to get next version number: {e}"), })?; - let next_version = if let Some(row) = rows.next().await.map_err(|e| WorkspaceError::SearchFailed { - reason: format!("Failed to read version number: {e}"), - })? { + let next_version = if let Some(row) = + rows.next() + .await + .map_err(|e| WorkspaceError::SearchFailed { + reason: format!("Failed to read version number: {e}"), + })? { get_i64(&row, 0) as i32 } else { 1 }; + drop(rows); - conn.execute( + tx.execute( r#" INSERT INTO memory_document_versions (id, document_id, version, content, content_hash, created_at, changed_by) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7) "#, - params![id, doc_id, next_version as i64, content, content_hash, now, changed_by], + params![ + id, + doc_id, + next_version as i64, + content, + content_hash, + now, + changed_by + ], ) .await .map_err(|e| WorkspaceError::SearchFailed { reason: format!("Failed to save version: {e}"), })?; + tx.commit() + .await + .map_err(|e| WorkspaceError::SearchFailed { + reason: format!("Failed to commit version: {e}"), + })?; + Ok(next_version) } @@ -999,12 +1031,16 @@ impl WorkspaceStore for LibSqlBackend { })?; Ok(DocumentVersion { - id: get_text(&row, 0).parse().map_err(|e| WorkspaceError::SearchFailed { - reason: format!("Invalid version UUID: {e}"), - })?, - document_id: get_text(&row, 1).parse().map_err(|e| WorkspaceError::SearchFailed { - reason: format!("Invalid document UUID: {e}"), - })?, + id: get_text(&row, 0) + .parse() + .map_err(|e| WorkspaceError::SearchFailed { + reason: format!("Invalid version UUID: {e}"), + })?, + document_id: get_text(&row, 1) + .parse() + .map_err(|e| WorkspaceError::SearchFailed { + reason: format!("Invalid document UUID: {e}"), + })?, version: get_i64(&row, 2) as i32, content: get_text(&row, 3), content_hash: get_text(&row, 4), @@ -1041,9 +1077,13 @@ impl WorkspaceStore for LibSqlBackend { })?; let mut versions = Vec::new(); - while let Some(row) = rows.next().await.map_err(|e| WorkspaceError::SearchFailed { - reason: format!("Failed to read version row: {e}"), - })? { + while let Some(row) = rows + .next() + .await + .map_err(|e| WorkspaceError::SearchFailed { + reason: format!("Failed to read version row: {e}"), + })? + { versions.push(VersionSummary { version: get_i64(&row, 0) as i32, content_hash: get_text(&row, 1), @@ -1074,9 +1114,13 @@ impl WorkspaceStore for LibSqlBackend { reason: format!("Failed to get latest version number: {e}"), })?; - if let Some(row) = rows.next().await.map_err(|e| WorkspaceError::SearchFailed { - reason: format!("Failed to read version number: {e}"), - })? { + if let Some(row) = rows + .next() + .await + .map_err(|e| WorkspaceError::SearchFailed { + reason: format!("Failed to read version number: {e}"), + })? + { // MAX returns NULL if no rows — libsql returns Null for the value let val = row.get::(0).ok(); match val { diff --git a/src/tools/builtin/memory.rs b/src/tools/builtin/memory.rs index 25ef8633..ecb0db9c 100644 --- a/src/tools/builtin/memory.rs +++ b/src/tools/builtin/memory.rs @@ -265,7 +265,7 @@ impl Tool for MemoryWriteTool { "default": false } }, - "required": ["content"] + "required": [] }) } @@ -276,7 +276,9 @@ impl Tool for MemoryWriteTool { ) -> Result { let start = std::time::Instant::now(); - let content = require_str(¶ms, "content")?; + // In patch mode (old_string present), content is not required. + let is_patch_mode = params.get("old_string").and_then(|v| v.as_str()).is_some(); + let content = params.get("content").and_then(|v| v.as_str()).unwrap_or(""); let target = params .get("target") @@ -316,9 +318,9 @@ impl Tool for MemoryWriteTool { return Ok(ToolOutput::success(output, start.elapsed())); } - if content.trim().is_empty() { + if !is_patch_mode && content.trim().is_empty() { return Err(ToolError::InvalidParameters( - "content cannot be empty".to_string(), + "content cannot be empty (use old_string/new_string for patch mode)".to_string(), )); } @@ -491,14 +493,20 @@ impl Tool for MemoryWriteTool { } // Apply metadata if provided (after write/append, works for all targets). + // We read the document once to get its ID — this is a hot read right + // after the write, so it's effectively free (same DB connection/cache). if let Some(meta) = params.get("metadata") && meta.is_object() { - // Read the document to get its ID - if let Ok(doc) = workspace.read(&resolved_path).await - && let Err(e) = workspace.update_metadata(doc.id, meta).await - { - tracing::warn!(path = %resolved_path, "failed to update metadata: {e}"); + match workspace.read(&resolved_path).await { + Ok(doc) => { + if let Err(e) = workspace.update_metadata(doc.id, meta).await { + tracing::warn!(path = %resolved_path, "failed to update metadata: {e}"); + } + } + Err(e) => { + tracing::warn!(path = %resolved_path, "failed to read doc for metadata update: {e}"); + } } } diff --git a/src/workspace/document.rs b/src/workspace/document.rs index dafaf1e3..b1248daa 100644 --- a/src/workspace/document.rs +++ b/src/workspace/document.rs @@ -540,11 +540,17 @@ mod tests { }); let meta = DocumentMetadata::from_value(&value); assert_eq!(meta.skip_indexing, Some(true)); - assert_eq!(meta.extra.get("custom_field").and_then(|v| v.as_str()), Some("hello")); + assert_eq!( + meta.extra.get("custom_field").and_then(|v| v.as_str()), + Some("hello") + ); // Round-trip preserves the field let back = meta.to_value(); - assert_eq!(back.get("custom_field").and_then(|v| v.as_str()), Some("hello")); + assert_eq!( + back.get("custom_field").and_then(|v| v.as_str()), + Some("hello") + ); } #[test] diff --git a/src/workspace/hygiene.rs b/src/workspace/hygiene.rs index 4a11d561..6a2f76b2 100644 --- a/src/workspace/hygiene.rs +++ b/src/workspace/hygiene.rs @@ -174,11 +174,7 @@ pub async fn run_if_due(workspace: &Workspace, config: &HygieneConfig) -> Hygien match cleanup_directory(workspace, &directory, hygiene.retention_days).await { Ok(deleted) => { if deleted > 0 { - tracing::info!( - directory, - deleted, - "memory hygiene: cleaned directory" - ); + tracing::info!(directory, deleted, "memory hygiene: cleaned directory"); } report.directories_cleaned.push((directory, deleted)); } @@ -454,11 +450,7 @@ mod tests { } /// Helper to seed a .config document with hygiene metadata on a directory. - async fn seed_hygiene_config( - workspace: &Workspace, - directory: &str, - retention_days: u32, - ) { + async fn seed_hygiene_config(workspace: &Workspace, directory: &str, retention_days: u32) { let config_path = format!("{}.config", directory); // Create the .config document with empty content workspace @@ -492,9 +484,7 @@ mod tests { ws.write("daily/2024-01-15.md", "Old log") .await .expect("write log"); - ws.write("daily/.config", "") - .await - .expect("write config"); + ws.write("daily/.config", "").await.expect("write config"); // Run cleanup with 0-day retention (deletes everything old) let deleted = cleanup_directory(&ws, "daily/", 0)