diff --git a/cli/src/commands/ak/mod.rs b/cli/src/commands/ak/mod.rs index afc5dfbd..309d4d3c 100644 --- a/cli/src/commands/ak/mod.rs +++ b/cli/src/commands/ak/mod.rs @@ -268,7 +268,7 @@ fn run_write( if force { backend .as_ref() - .overwrite(&path, &content) + .overwrite(&path, &content, None) .map_err(|error| error.to_string())?; } else { backend.as_ref().create(&path, &content).map_err(|error| match error { diff --git a/libs/ak/src/error.rs b/libs/ak/src/error.rs index 3fd268ed..01771f7d 100644 --- a/libs/ak/src/error.rs +++ b/libs/ak/src/error.rs @@ -8,6 +8,7 @@ pub enum Error { NotFound(PathBuf), NotADirectory(PathBuf), UnsafePath(PathBuf), + Conflict(PathBuf), Parse(String), } @@ -36,6 +37,11 @@ impl Display for Error { "unsafe path blocked: {}. ak paths must stay inside the store and cannot pass through symlinks", path.display() ), + Self::Conflict(path) => write!( + f, + "conflict: {} was modified by another client. re-read the file and retry", + path.display() + ), Self::Parse(message) => write!(f, "invalid input: {message}"), } } @@ -71,4 +77,14 @@ mod tests { assert!(rendered.contains("path already exists")); assert!(rendered.contains("choose a new path or overwrite intentionally")); } + + #[test] + fn conflict_error_suggests_re_read_and_retry() { + let error = Error::Conflict(PathBuf::from("docs/runbooks/deploy.md")); + let rendered = error.to_string(); + + assert!(rendered.contains("conflict")); + assert!(rendered.contains("modified by another client")); + assert!(rendered.contains("re-read")); + } } diff --git a/libs/ak/src/store.rs b/libs/ak/src/store.rs index e842b9f4..dfd37ab8 100644 --- a/libs/ak/src/store.rs +++ b/libs/ak/src/store.rs @@ -17,13 +17,19 @@ fn map_knowledge_err(path: &str, err: KnowledgeApiError) -> Error { match err { KnowledgeApiError::NotFound { .. } => Error::NotFound(PathBuf::from(path)), KnowledgeApiError::Conflict { .. } => Error::AlreadyExists(PathBuf::from(path)), + KnowledgeApiError::PreconditionFailed { .. } => Error::Conflict(PathBuf::from(path)), other => Error::Parse(other.to_string()), } } pub trait StorageBackend { fn create(&self, path: &str, content: &[u8]) -> Result<(), Error>; - fn overwrite(&self, path: &str, content: &[u8]) -> Result<(), Error>; + fn overwrite( + &self, + path: &str, + content: &[u8], + expected_hash: Option<&str>, + ) -> Result<(), Error>; fn read(&self, path: &str) -> Result, Error>; fn read_prefix(&self, path: &str, max_bytes: usize) -> Result, Error>; fn remove(&self, path: &str) -> Result<(), Error>; @@ -276,7 +282,12 @@ impl StorageBackend for LocalFsBackend { Ok(()) } - fn overwrite(&self, path: &str, content: &[u8]) -> Result<(), Error> { + fn overwrite( + &self, + path: &str, + content: &[u8], + _expected_hash: Option<&str>, + ) -> Result<(), Error> { self.ensure_store()?; let target = self.resolve_path(path)?; self.ensure_no_symlinks_below_root(&target)?; @@ -497,10 +508,18 @@ impl StorageBackend for RemoteBackend { .map_err(|e| map_knowledge_err(path, e)) } - fn overwrite(&self, path: &str, content: &[u8]) -> Result<(), Error> { + fn overwrite( + &self, + path: &str, + content: &[u8], + expected_hash: Option<&str>, + ) -> Result<(), Error> { tokio::task::block_in_place(|| { - tokio::runtime::Handle::current() - .block_on(async { self.client.overwrite_knowledge_file(path, content).await }) + tokio::runtime::Handle::current().block_on(async { + self.client + .overwrite_knowledge_file(path, content, expected_hash) + .await + }) }) .map(|_| ()) .map_err(|e| map_knowledge_err(path, e)) @@ -779,7 +798,7 @@ mod tests { .expect("create initial summary"); backend - .overwrite("summaries/auth.md", b"new") + .overwrite("summaries/auth.md", b"new", None) .expect("overwrite file"); let content = backend diff --git a/libs/api/src/stakpak/knowledge/mod.rs b/libs/api/src/stakpak/knowledge/mod.rs index b6125962..1ad5cb04 100644 --- a/libs/api/src/stakpak/knowledge/mod.rs +++ b/libs/api/src/stakpak/knowledge/mod.rs @@ -44,6 +44,9 @@ pub enum KnowledgeApiError { Forbidden { message: String }, /// Request was rejected by the server (HTTP 400). BadRequest { message: String }, + /// Optimistic lock mismatch — the `If-Match` header did not match the + /// current `content_hash` (HTTP 412). + PreconditionFailed { message: String }, /// Catch-all for any other HTTP error status, plus the raw body. Http { status: StatusCode, message: String }, /// Transport / serialization / IO failure (no HTTP status available). @@ -57,6 +60,7 @@ impl KnowledgeApiError { | Self::Conflict { message } | Self::Forbidden { message } | Self::BadRequest { message } + | Self::PreconditionFailed { message } | Self::Http { message, .. } | Self::Transport { message } => message, } @@ -69,6 +73,7 @@ impl KnowledgeApiError { Self::Conflict { .. } => Some(StatusCode::CONFLICT), Self::Forbidden { .. } => Some(StatusCode::FORBIDDEN), Self::BadRequest { .. } => Some(StatusCode::BAD_REQUEST), + Self::PreconditionFailed { .. } => Some(StatusCode::PRECONDITION_FAILED), Self::Http { status, .. } => Some(*status), Self::Transport { .. } => None, } @@ -82,6 +87,9 @@ impl std::fmt::Display for KnowledgeApiError { Self::Conflict { message } => write!(f, "conflict: {}", message), Self::Forbidden { message } => write!(f, "forbidden: {}", message), Self::BadRequest { message } => write!(f, "bad request: {}", message), + Self::PreconditionFailed { message } => { + write!(f, "precondition failed: {}", message) + } Self::Http { status, message } => write!(f, "http {}: {}", status, message), Self::Transport { message } => write!(f, "transport error: {}", message), } @@ -368,26 +376,84 @@ impl StakpakApiClient { /// Overwrite an existing knowledge file (or create if not exists). /// - /// The cache is not populated here. Any stale local copy will be - /// revalidated on the next read: `If-None-Match` will miss against the - /// new server ETag and the client will refetch + replace the cached - /// body. + /// Always sends an `If-Match` header for optimistic locking. The server + /// compares this value against the file's current `content_hash` and + /// returns `412 Precondition Failed` if they differ, preventing blind + /// clobbers from concurrent clients. + /// + /// The hash is resolved from, in order of priority: + /// 1. An explicit `expected_content_hash` value provided by the caller + /// 2. The on-disk cache (populated by a prior `read_knowledge_file`) + /// + /// If neither source provides a hash, the call fails with + /// `PreconditionFailed` — callers must read the file first. + /// + /// On success, the cache entry for this path is evicted so the next read + /// refetches the updated body. pub async fn overwrite_knowledge_file( &self, path: &str, content: &[u8], + expected_content_hash: Option<&str>, ) -> Result { let normalized_path = normalize_knowledge_path(path)?; let encoded_path = encode_path_segments(&normalized_path); let url = format!("{}/v1/knowledge/{}", self.base_url, encoded_path); - let response = self + + let if_match = match expected_content_hash { + Some(hash) => Some(hash.to_string()), + None => { + let account = self.resolve_cache_account().await; + match account { + Some(account) => { + let cache_target = cache::cached_path(&account, &normalized_path); + match cache_target { + Some(target) => cache::read_cached(&target).await.map(|(_, etag)| etag), + None => None, + } + } + None => None, + } + } + }; + + let if_match = match if_match { + Some(hash) => hash, + None => { + return Err(KnowledgeApiError::PreconditionFailed { + message: format!( + "no content_hash available for {} — read the file before overwriting", + normalized_path + ), + }); + } + }; + + let request = self .client .put(&url) .header(header::CONTENT_TYPE, "application/octet-stream") - .body(content.to_vec()) - .send() - .await?; - self.handle_knowledge_response(response).await + .header(header::IF_MATCH, &if_match); + let response = request.body(content.to_vec()).send().await?; + + if response.status() == StatusCode::PRECONDITION_FAILED { + return Err(KnowledgeApiError::PreconditionFailed { + message: format!( + "content_hash mismatch for {} — the file was modified by another client", + normalized_path + ), + }); + } + + let result = self.handle_knowledge_response(response).await?; + + if let Some(account) = self.resolve_cache_account().await + && let Some(target) = cache::cached_path(&account, &normalized_path) + { + cache::evict_cached(&target).await; + } + + Ok(result) } /// Delete a knowledge file or directory. On success, evicts the matching @@ -466,6 +532,7 @@ impl StakpakApiClient { KnowledgeApiError::Forbidden { message } } StatusCode::BAD_REQUEST => KnowledgeApiError::BadRequest { message }, + StatusCode::PRECONDITION_FAILED => KnowledgeApiError::PreconditionFailed { message }, other => KnowledgeApiError::Http { status: other, message,