diff --git a/adapters/remote/Cargo.toml b/adapters/remote/Cargo.toml index 18ec568e..ea30747d 100644 --- a/adapters/remote/Cargo.toml +++ b/adapters/remote/Cargo.toml @@ -20,6 +20,9 @@ anyhow = "1" # cap rather than buffered whole, because the endpoint is operator-supplied # and a broken or hostile one must not be able to OOM the host. reqwest = { version = "0.12", default-features = false, features = ["json", "multipart", "rustls-tls", "stream"] } +# Only the timer, for the read-retry backoff (issue #18 §U5) — reqwest already +# requires a tokio runtime, so this adds no new runtime assumption. +tokio = { version = "1", default-features = false, features = ["time"] } # Remote records are translated through a private, lossless envelope. serde = { version = "1", features = ["derive"] } # Streaming a capped body needs a Stream combinator. diff --git a/adapters/remote/src/cognee.rs b/adapters/remote/src/cognee.rs index 4bcede41..f91f6b33 100644 --- a/adapters/remote/src/cognee.rs +++ b/adapters/remote/src/cognee.rs @@ -8,7 +8,7 @@ use tinymemory_api::recall::RecallOpts; use tinymemory_api::traits::Memory; use tinymemory_api::types::MemoryTaint; -use crate::common::{stable_id, Dialect, HttpClient, RemoteMemory, StoredEntry}; +use crate::common::{stable_id, Attempts, Dialect, HttpClient, RemoteMemory, StoredEntry}; /// Stable driver id used by configuration and status output. pub use tinymemory_api::drivers::COGNEE_DRIVER_ID; @@ -20,6 +20,26 @@ pub struct CogneeMemory { } impl CogneeMemory { + /// Rebuilds the HTTP transport with a different per-request deadline + /// (issue #18 follow-up U5). The default is 60s with a 10s connect + /// deadline — right for interactive calls; a bulk migration or a tight + /// liveness probe may want its own budget. + /// + /// # Errors + /// + /// Fails only if the underlying HTTP client cannot be rebuilt — a + /// configuration-time failure, before any request is made. + pub fn with_request_timeout(mut self, timeout: std::time::Duration) -> anyhow::Result { + let client = self + .inner + .dialect_mut() + .client + .clone() + .with_timeout(timeout)?; + self.inner.dialect_mut().client = client; + Ok(self) + } + /// Connect to a self-hosted Cognee server. /// /// `access_token` is sent as a bearer token. Local deployments with @@ -152,6 +172,13 @@ impl Memory for CogneeMemory { async fn health_check(&self) -> bool { self.inner.health_check().await } + /// Forwarded explicitly: this wrapper delegates method-by-method, so the + /// defaulted `None` would otherwise shadow `RemoteMemory`'s typed probe — + /// which is exactly what the first cut shipped, making §U4's deep health + /// unreachable through every public type (the #68 review's Major 1). + async fn health_probe(&self) -> Option { + self.inner.health_probe().await + } } #[derive(Debug)] @@ -181,7 +208,12 @@ impl CogneeDialect { async fn datasets(&self) -> anyhow::Result> { let response: Value = self .client - .json(Method::GET, "api/v1/datasets/", None) + .json( + Method::GET, + "api/v1/datasets/", + None, + Attempts::RetryTransient, + ) .await?; Ok(response .as_array() @@ -205,6 +237,7 @@ impl CogneeDialect { Method::GET, &format!("api/v1/datasets/{}/data", dataset.id), None, + Attempts::RetryTransient, ) .await?; let mut entries = Vec::new(); @@ -225,6 +258,7 @@ impl CogneeDialect { .text( Method::GET, &format!("api/v1/datasets/{}/data/{id}/raw", dataset.id), + Attempts::RetryTransient, ) .await?; let mut entry: StoredEntry = @@ -356,6 +390,7 @@ impl Dialect for CogneeDialect { "only_context": true, "session_id": opts.session_id })), + Attempts::RetryTransient, ) .await?; let mut entries = Vec::new(); @@ -402,9 +437,17 @@ impl Dialect for CogneeDialect { Ok(true) } - /// Checks Cognee's aggregate health endpoint. - async fn health(&self) -> bool { - self.client.healthy("health").await + /// Probes Cognee's aggregate health endpoint, typed. + async fn health(&self) -> anyhow::Result<()> { + self.client.probe("health").await + } + + /// Context-only recall carries no score field — see the trait doc for + /// what this means for `min_score` (documented-inert, not everything- + /// dropping; the first cut's over-fetch pulled 3x the data and discarded + /// all of it). + fn scores_recall(&self) -> bool { + false } } diff --git a/adapters/remote/src/cognee_test.rs b/adapters/remote/src/cognee_test.rs index ad803781..5afcc3e4 100644 --- a/adapters/remote/src/cognee_test.rs +++ b/adapters/remote/src/cognee_test.rs @@ -207,6 +207,27 @@ async fn native_cognee_round_trips_the_tinymemory_contract() { .len(), 1 ); + // #68 review Major 2: Cognee's context-only recall is scoreless, so the + // strict filter would have dropped 100% of every thresholded result. + // The dialect declares scores_recall() = false and min_score is + // documented-inert: the hit survives. + assert_eq!( + driver + .recall( + "graph", + 3, + &OwnedRecallOpts { + namespace: Some("project".into()), + min_score: Some(0.5), + ..OwnedRecallOpts::default() + }, + None + ) + .await + .expect("recall with a threshold the backend cannot score") + .len(), + 1 + ); assert!(driver.forget("project", "key").await.expect("forget")); assert!(!driver.forget("project", "key").await.expect("forget again")); assert!(driver.health().await.is_usable()); diff --git a/adapters/remote/src/common.rs b/adapters/remote/src/common.rs index a9aa53d7..f9ed7d12 100644 --- a/adapters/remote/src/common.rs +++ b/adapters/remote/src/common.rs @@ -8,6 +8,7 @@ use reqwest::header::{HeaderValue, AUTHORIZATION}; use reqwest::{Method, RequestBuilder, StatusCode, Url}; use serde::{de::DeserializeOwned, Deserialize, Serialize}; use sha2::{Digest, Sha256}; +use tinymemory_api::error::MemoryError; use tinymemory_api::traits::Memory; use tinymemory_api::types::{ MemoryCategory, MemoryEntry, MemoryTaint, NamespaceSummary, RecallOpts, @@ -119,6 +120,29 @@ fn credential_header(value: &str) -> anyhow::Result { Ok(header) } +/// The caller's statement of a request's idempotence — every `json`/`text` +/// call site must choose, which is what makes the read/write retry split +/// CHECKABLE instead of conventional (#68 review, Major 4: the first cut's +/// split lived only in a comment, and wrapping the write helper in the retry +/// path failed nothing). +/// +/// `RetryTransient` is only sound when repeating the request cannot double- +/// apply anything: reads, searches, list walks — POST included when the POST +/// is a query. `Once` is for everything whose repetition has a cost. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum Attempts { + /// Retry up to three times on the typed transient classes. + RetryTransient, + /// One attempt, whatever the failure. + // + // No current caller: the audit behind #68 found every existing + // `json`/`text` call is a read, which is exactly why the marker exists — + // the FIRST write-shaped caller must pick this variant instead of + // silently inheriting retry. Deliberately present before its first use. + #[allow(dead_code)] + Once, +} + impl HttpClient { /// Builds a client that optionally authenticates with a bearer token. pub(crate) fn bearer(endpoint: &str, credential: Option<&str>) -> anyhow::Result { @@ -155,14 +179,35 @@ impl HttpClient { endpoint.set_path(&path); } Ok(Self { - inner: reqwest::Client::builder() - .timeout(std::time::Duration::from_secs(60)) - .build()?, + inner: Self::build_inner(std::time::Duration::from_secs(60))?, endpoint, auth, }) } + /// One place builds the reqwest client, so the two timeouts stay paired: + /// the per-request deadline, and a connect deadline that keeps a + /// black-holed endpoint from consuming the whole request budget before + /// the first byte. + fn build_inner(timeout: std::time::Duration) -> anyhow::Result { + Ok(reqwest::Client::builder() + .timeout(timeout) + .connect_timeout(std::time::Duration::from_secs(10).min(timeout)) + .build()?) + } + + /// Rebuilds this client with a different per-request deadline (issue #18 + /// follow-up U5). The 60s default suits interactive calls; a bulk + /// migration or a health probe may want its own budget. + /// + /// Note the effective worst-case wall time of a retrying READ is ~3x + /// this value plus 750ms of backoff — the transient-retry policy runs up + /// to three attempts, each with its own deadline. + pub(crate) fn with_timeout(mut self, timeout: std::time::Duration) -> anyhow::Result { + self.inner = Self::build_inner(timeout)?; + Ok(self) + } + /// Resolves a relative API path and attaches the configured authentication. fn request(&self, method: Method, path: &str) -> anyhow::Result { let url = self @@ -208,11 +253,22 @@ impl HttpClient { parts.join(": ") }; let class = classify_transport(error.is_timeout(), error.is_connect(), &chain); - if chain.is_empty() { - anyhow::anyhow!("memory API request to {host}: {class}") + let described = class.describe(); + let message = if chain.is_empty() { + format!("memory API request to {host}: {described}") } else { - anyhow::anyhow!("memory API request to {host}: {class} ({chain})") - } + format!("memory API request to {host}: {described} ({chain})") + }; + // §A4: the typed error rides as the anyhow payload, so + // `tinymemory_api::mandatory::engine_error` can downcast it back out + // at the contract boundary instead of flattening it into `Other`. + anyhow::Error::new(match class { + TransportClass::Timeout => MemoryError::Timeout(message), + TransportClass::Dns | TransportClass::Tls | TransportClass::Connect => { + MemoryError::Unreachable(message) + } + TransportClass::Other => return anyhow::anyhow!("{message}"), + }) } /// The error for a non-success status, written for the operator reading a @@ -238,7 +294,9 @@ impl HttpClient { } format!(" — {shown}") }; - match status.as_u16() { + // §A4: every bucket mints a typed [`MemoryError`] carried as the + // anyhow payload — same prose as before, now matchable downstream. + anyhow::Error::new(match status.as_u16() { 401 | 403 => { let hint = match &self.auth { Auth::ApiKey(_) | Auth::Token(_) => "check the API key", @@ -247,12 +305,75 @@ impl HttpClient { "the endpoint requires credentials this client was not configured with" } }; - anyhow::anyhow!( + MemoryError::Unauthorized(format!( "memory API {path} on {host}: the configured credential was rejected \ (HTTP {status}) — {hint}{detail}" - ) + )) + } + 404 => MemoryError::NotFound(format!( + "memory API {path} on {host} returned HTTP 404{detail}" + )), + // A validation refusal: the backend understood the request and + // rejected its CONTENT. Without this arm a real validating + // backend (all three vendors validate; only the in-tree doubles + // accept everything) could never produce the `Invalid` the + // tightened conformance refusal-assertion demands (#68 review, + // Major 5). + 400 | 422 => MemoryError::Invalid(format!( + "memory API {path} on {host} returned HTTP {status}{detail}" + )), + // The answered-but-cannot-serve class: rate limiting and the + // gateway trio. Distinct from `Backend` so a retry policy can key + // on it without parsing prose. + 429 | 502 | 503 | 504 => MemoryError::Unavailable(format!( + "memory API {path} on {host} returned HTTP {status}{detail}" + )), + _ => MemoryError::Backend(format!( + "memory API {path} on {host} returned HTTP {status}{detail}" + )), + }) + } + + /// Whether an error is worth one more attempt on a READ path: the typed + /// transient classes only (issue #18 follow-up U5). Keyed on the §A4 + /// variants rather than message substrings — the fragility the + /// composio sync client's needle-matching retry shows the cost of. + /// `Unauthorized`, `Invalid`, `NotFound`, `Backend` never retry: the + /// answer will not change. + fn retryable(error: &anyhow::Error) -> bool { + matches!( + error.downcast_ref::(), + Some( + MemoryError::Timeout(_) | MemoryError::Unreachable(_) | MemoryError::Unavailable(_) + ) + ) + } + + /// Runs a read-path attempt up to three times with 250ms·2ⁿ backoff. + /// + /// READ paths only — `json` and `text` below, whose calls are all list, + /// search and raw-fetch operations across the three adapters. The write + /// paths (`empty`, `multipart`) are deliberately not routed through + /// here: a `Timeout` on a write leaves whether the backend applied it + /// unknown, and Cognee's multipart upsert and Mem0's add are not + /// idempotent. + async fn with_read_retry(&self, attempt: F) -> anyhow::Result + where + F: Fn() -> Fut, + Fut: std::future::Future>, + { + const MAX_ATTEMPTS: u32 = 3; + let mut tried = 0; + loop { + tried += 1; + match attempt().await { + Ok(value) => return Ok(value), + Err(error) if tried < MAX_ATTEMPTS && Self::retryable(&error) => { + let backoff = std::time::Duration::from_millis(250) * 2_u32.pow(tried - 1); + tokio::time::sleep(backoff).await; + } + Err(error) => return Err(error), } - _ => anyhow::anyhow!("memory API {path} on {host} returned HTTP {status}{detail}"), } } @@ -261,6 +382,22 @@ impl HttpClient { method: Method, path: &str, body: Option<&serde_json::Value>, + attempts: Attempts, + ) -> anyhow::Result { + if matches!(attempts, Attempts::Once) { + return self.json_attempt(method, path, body).await; + } + self.with_read_retry(|| self.json_attempt(method.clone(), path, body)) + .await + } + + /// One send of a JSON request — the body `json` retries (or not, per its + /// `Attempts` marker). + async fn json_attempt( + &self, + method: Method, + path: &str, + body: Option<&serde_json::Value>, ) -> anyhow::Result { let mut request = self.request(method, path)?; if let Some(body) = body { @@ -281,19 +418,30 @@ impl HttpClient { } /// Sends a request and returns a successful response body as text. - pub(crate) async fn text(&self, method: Method, path: &str) -> anyhow::Result { - let response = self - .request(method, path)? - .send() - .await - .map_err(|error| self.transport_error(error))?; - let status = response.status(); - if !status.is_success() { - let body = response.text().await.unwrap_or_default(); - return Err(self.status_error(path, status, &body)); + pub(crate) async fn text( + &self, + method: Method, + path: &str, + attempts: Attempts, + ) -> anyhow::Result { + let attempt = || async { + let response = self + .request(method.clone(), path)? + .send() + .await + .map_err(|error| self.transport_error(error))?; + let status = response.status(); + if !status.is_success() { + let body = response.text().await.unwrap_or_default(); + return Err(self.status_error(path, status, &body)); + } + let body = read_capped(response, path).await?; + String::from_utf8(body).context("memory API response was not valid UTF-8") + }; + if matches!(attempts, Attempts::Once) { + return attempt().await; } - let body = read_capped(response, path).await?; - String::from_utf8(body).context("memory API response was not valid UTF-8") + self.with_read_retry(attempt).await } /// Sends a request whose successful response body is not needed. @@ -324,15 +472,22 @@ impl HttpClient { self.request(method, path) } - /// Reports whether a GET endpoint responds successfully. - pub(crate) async fn healthy(&self, path: &str) -> bool { - let Ok(request) = self.request(Method::GET, path) else { - return false; - }; - request + /// Probes a GET endpoint and reports WHY it failed, typed (issue #18 + /// follow-up U4). The boolean `healthy` below discards status, body and + /// transport class; this keeps them, so a health surface can distinguish + /// "credential rejected" from "unreachable" from "answered 500". + pub(crate) async fn probe(&self, path: &str) -> anyhow::Result<()> { + let response = self + .request(Method::GET, path)? .send() .await - .is_ok_and(|response| response.status().is_success()) + .map_err(|error| self.transport_error(error))?; + let status = response.status(); + if !status.is_success() { + let body = response.text().await.unwrap_or_default(); + return Err(self.status_error(path, status, &body)); + } + Ok(()) } } @@ -444,8 +599,21 @@ pub(crate) trait Dialect: Send + Sync + std::fmt::Debug { ) -> anyhow::Result>; /// Deletes one exact logical record and reports whether it existed. async fn delete(&self, namespace: &str, key: &str) -> anyhow::Result; - /// Checks whether the backend is available for requests. - async fn health(&self) -> bool; + /// Probes whether the backend is available, reporting WHY not, typed + /// (`Ok(())` = serving; the error carries a §A4 `MemoryError` payload). + async fn health(&self) -> anyhow::Result<()>; + /// Whether this backend's recall responses carry a similarity score. + /// + /// Decides the `min_score` tier in [`RemoteMemory::recall`]: a scoring + /// backend gets the strict filter (an unscored hit cannot clear a + /// threshold), while a backend that STRUCTURALLY cannot score — Cognee's + /// context-only recall has no score field at all — keeps its hits, because + /// dropping 100% of every result is not honesty, it is a different lie + /// (the #68 review's Major 2). The inertness on scoreless backends is + /// deliberate and documented rather than silent: this flag is where. + fn scores_recall(&self) -> bool { + true + } } #[derive(Debug)] @@ -455,6 +623,12 @@ pub(crate) struct RemoteMemory { } impl RemoteMemory { + /// Mutable access for the adapters' builder-style configuration + /// (`with_request_timeout` on each public type). + pub(crate) fn dialect_mut(&mut self) -> &mut D { + &mut self.dialect + } + /// Wraps a backend dialect with shared filtering and conversion behavior. pub(crate) fn new(dialect: D) -> Self { Self { dialect } @@ -522,7 +696,12 @@ impl Memory for RemoteMemory { let mut entries = self.dialect.search(query, limit, opts.clone()).await?; entries.retain(|entry| matches_filters(entry, &opts)); if let Some(minimum) = min_score { - entries.retain(|entry| entry.score.is_none_or(|score| score >= minimum)); + if self.dialect.scores_recall() { + entries.retain(|entry| clears_min_score(entry.score, minimum)); + } + // else: the backend cannot score (see `Dialect::scores_recall`) — + // the threshold is documented-inert rather than silently + // everything-dropping. } entries.truncate(limit); Ok(entries @@ -598,10 +777,65 @@ impl Memory for RemoteMemory { /// Delegates availability checking to the backend dialect. async fn health_check(&self) -> bool { - self.dialect.health().await + self.dialect.health().await.is_ok() + } + + /// The typed answer behind `health_check` (issue #18 §U4): the probe's + /// §A4 class decides the health state. `Unavailable` — the backend + /// answered that it cannot serve right now (429 / gateway trio) — maps to + /// `Degraded`: alive, impaired, worth saying so instead of "down". + /// Everything else that fails maps to `Down` with the probe's own reason + /// (which names host and class, never a credential). + async fn health_probe(&self) -> Option { + use tinymemory_api::health::MemoryHealth; + Some(match self.dialect.health().await { + Ok(()) => MemoryHealth::Ready, + Err(error) => { + let reason = health_reason(&error); + match error.downcast_ref::() { + Some(MemoryError::Unavailable(_)) => MemoryHealth::degraded(reason), + _ => MemoryHealth::down(reason), + } + } + }) } } +/// A health `reason` from a probe failure, REDACTED for the status surface. +/// +/// `MemoryHealth`'s contract: the reason is logged and rendered in operator +/// status and must never carry credentials or content. `status_error` +/// interpolates up to 300 chars of the backend's OWN error body — which a +/// vendor is free to fill with the rejected key (#68 review). Every message +/// this crate builds puts that detail after a spaced em-dash, so the reason +/// keeps each chain segment's head and drops the tails. The full untruncated +/// error still flows to the CALLER of the failing operation; only the +/// standing status string is trimmed. Walking `chain()` (not just the top) +/// keeps Mem0's both-probes-failed context instead of losing it to a +/// consuming downcast. +fn health_reason(error: &anyhow::Error) -> String { + error + .chain() + .map(|cause| { + let text = cause.to_string(); + match text.split_once(" — ") { + Some((head, _)) => format!("{head} — detail withheld from status; see logs"), + None => text, + } + }) + .collect::>() + .join("; ") +} + +/// Honesty over leniency (issue #18 §U6): an entry with NO score cannot be +/// shown to clear a threshold the caller asked for, so it drops. The old +/// `is_none_or` let unscored hits pass, which made `min_score` silently inert +/// against any backend that omits score numbers — a caller asking for ≥0.8 +/// got unranked everything and never learned the filter did nothing. +fn clears_min_score(score: Option, minimum: f64) -> bool { + score.is_some_and(|value| value >= minimum) +} + /// Applies TinyMemory recall filters that a backend may not support natively. fn matches_filters(entry: &StoredEntry, opts: &RecallOpts<'_>) -> bool { opts.namespace.is_none_or(|value| entry.namespace == value) @@ -614,6 +848,39 @@ fn matches_filters(entry: &StoredEntry, opts: &RecallOpts<'_>) -> bool { .is_none_or(|value| entry.session_id.as_deref() == Some(value)) } +/// The class of a transport failure — a real enum rather than a prose string +/// (issue #18 §A4), so [`HttpClient::transport_error`] can mint a typed +/// [`MemoryError`] and a retry policy can key on the class instead of +/// substring-matching a rendered message. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum TransportClass { + Timeout, + Dns, + Tls, + Connect, + /// The request did not complete for a reason the chain does not name — + /// deliberately NOT mapped onto a typed variant, because claiming + /// "unreachable" for (say) a mid-body disconnect would be a guess. + Other, +} + +impl TransportClass { + /// The operator-facing prose for this class — exactly the strings the + /// pre-§A4 version returned, so log lines do not change spelling. + fn describe(self) -> &'static str { + match self { + Self::Timeout => "timed out", + Self::Dns => "the host could not be resolved — check the URL", + Self::Tls => { + "TLS failed — the endpoint answered on the port but could not establish a \ + secure connection; check that the URL is the engine's real API host" + } + Self::Connect => "could not connect — check the URL and that the service is reachable", + Self::Other => "the request did not complete", + } + } +} + /// Name the class of a transport failure from what the error chain says. /// /// Pure so the ORDER is testable, which is the whole reason it exists as its @@ -621,15 +888,15 @@ fn matches_filters(entry: &StoredEntry, opts: &RecallOpts<'_>) -> bool { /// naive `if is_connect()` first collapses every class into "could not /// connect". That is exactly what the first version of this did, and it took /// a live run against a real broken endpoint to notice. -fn classify_transport(is_timeout: bool, is_connect: bool, chain: &str) -> &'static str { +fn classify_transport(is_timeout: bool, is_connect: bool, chain: &str) -> TransportClass { let lower = chain.to_ascii_lowercase(); if is_timeout { - "timed out" + TransportClass::Timeout } else if lower.contains("dns") || lower.contains("name or service") || lower.contains("failed to lookup") { - "the host could not be resolved — check the URL" + TransportClass::Dns } else if lower.contains("tls") || lower.contains("handshake") || lower.contains("certificate") @@ -637,12 +904,11 @@ fn classify_transport(is_timeout: bool, is_connect: bool, chain: &str) -> &'stat || lower.contains("invalid peer") || lower.contains("unknown issuer") { - "TLS failed — the endpoint answered on the port but could not establish a \ - secure connection; check that the URL is the engine's real API host" + TransportClass::Tls } else if is_connect { - "could not connect — check the URL and that the service is reachable" + TransportClass::Connect } else { - "the request did not complete" + TransportClass::Other } } @@ -714,7 +980,9 @@ mod credential_header_tests { #[cfg(test)] mod transport_tests { - use super::classify_transport; + #![allow(clippy::expect_used, clippy::panic)] + + use super::{classify_transport, TransportClass}; /// The verbatim chain a rustls handshake abort produces. Cognee's hosted /// endpoint answered TCP and then sent this; `reqwest` reports it as a @@ -728,7 +996,8 @@ mod transport_tests { true, // reqwest really does set is_connect for this "client error (Connect): received fatal alert: InternalError", ); - assert!(class.starts_with("TLS failed"), "got: {class}"); + assert_eq!(class, TransportClass::Tls); + assert!(class.describe().starts_with("TLS failed")); } /// DNS failures are also CONNECT errors; the specific class must win. @@ -739,7 +1008,8 @@ mod transport_tests { true, "client error (Connect): dns error: failed to lookup address information", ); - assert!(class.contains("could not be resolved"), "got: {class}"); + assert_eq!(class, TransportClass::Dns); + assert!(class.describe().contains("could not be resolved")); } #[test] @@ -749,7 +1019,8 @@ mod transport_tests { true, "client error (Connect): tcp connect error: Connection refused (os error 61)", ); - assert!(class.starts_with("could not connect"), "got: {class}"); + assert_eq!(class, TransportClass::Connect); + assert!(class.describe().starts_with("could not connect")); } /// A timeout outranks everything: it is the one class reqwest states @@ -757,12 +1028,64 @@ mod transport_tests { #[test] fn a_timeout_wins_over_every_chain_hint() { let class = classify_transport(true, true, "dns error: something tls certificate"); - assert_eq!(class, "timed out"); + assert_eq!(class, TransportClass::Timeout); + assert_eq!(class.describe(), "timed out"); } #[test] fn an_unrecognised_chain_degrades_without_claiming_a_cause() { let class = classify_transport(false, false, "body error: incomplete message"); - assert_eq!(class, "the request did not complete"); + assert_eq!(class, TransportClass::Other); + assert_eq!(class.describe(), "the request did not complete"); + } + + /// §A4: the typed payload rides the anyhow error and downcasts back out — + /// the property `engine_error` relies on at the contract boundary. + #[test] + fn typed_variants_survive_the_anyhow_round_trip() { + use tinymemory_api::error::MemoryError; + let carried = anyhow::Error::new(MemoryError::Unauthorized("key rejected".into())); + match carried.downcast::() { + Ok(MemoryError::Unauthorized(msg)) => assert_eq!(msg, "key rejected"), + other => panic!("lost the typed payload: {other:?}"), + } + } + + /// §U6: a threshold means a threshold. An unscored hit does not clear + /// one, an exactly-equal score does, and no-threshold callers see the + /// old behavior untouched (the filter never runs). + #[test] + fn min_score_is_honest_about_unscored_hits() { + use super::clears_min_score; + assert!(!clears_min_score(None, 0.1)); + assert!(clears_min_score(Some(0.8), 0.8)); + assert!(!clears_min_score(Some(0.79), 0.8)); + } + + /// The retry gate keys on the §A4 class, never the prose: transient + /// classes retry, deterministic answers do not. + #[test] + fn retry_gate_is_typed_and_conservative() { + use super::HttpClient; + use tinymemory_api::error::MemoryError; + let transient = [ + MemoryError::Timeout("t".into()), + MemoryError::Unreachable("u".into()), + MemoryError::Unavailable("503".into()), + ]; + for error in transient { + assert!(HttpClient::retryable(&anyhow::Error::new(error))); + } + let settled = [ + MemoryError::Unauthorized("401".into()), + MemoryError::Invalid("bad".into()), + MemoryError::NotFound("gone".into()), + MemoryError::Backend("500".into()), + ]; + for error in settled { + assert!(!HttpClient::retryable(&anyhow::Error::new(error))); + } + // Opaque errors never retry: without a class, a retry is a guess. + assert!(!HttpClient::retryable(&anyhow::anyhow!("mystery"))); } } diff --git a/adapters/remote/src/failure_test.rs b/adapters/remote/src/failure_test.rs index d1a2a667..ed524a4c 100644 --- a/adapters/remote/src/failure_test.rs +++ b/adapters/remote/src/failure_test.rs @@ -5,10 +5,10 @@ //! drive a backend that answers correctly, which is the half that was never in //! doubt. //! -//! The assertion that matters is not which error comes back — the contract's -//! error type is still `anyhow` under `MemoryError::Other` here, and §A4 is what -//! makes "unsupported" distinguishable from "failed". It is that a failure comes -//! back **at all**. +//! Since §A4 landed, the assertion is two-fold: a failure comes back **at +//! all**, and — where the class is knowable — it comes back **typed**: a 401 +//! downcasts to `Unauthorized`, a 500 to `Backend`, a dead port to +//! `Unreachable`, so a caller can act on the class instead of parsing prose. //! //! A read that answers `Ok(None)` when the backend returned 500 is saying "this //! memory does not exist" when the truth is "I could not ask". A caller cannot @@ -26,6 +26,7 @@ use axum::http::StatusCode; use axum::routing::{any, get}; use axum::Router; +use tinymemory_api::error::MemoryError; use tinymemory_api::provider::MemoryProvider; use tinymemory_api::recall::RecallOpts; use tinymemory_api::traits::Memory; @@ -50,6 +51,26 @@ async fn failing(status: StatusCode) -> String { serve(Router::new().fallback(any(move || async move { status }))).await } +/// A backend that fails every route with `status` and a body carrying a +/// fake secret, counting requests — for the redaction and retry-count +/// assertions. +async fn failing_with_body( + status: StatusCode, + body: &'static str, +) -> (String, std::sync::Arc) { + let hits = std::sync::Arc::new(std::sync::atomic::AtomicU32::new(0)); + let counter = hits.clone(); + let endpoint = serve(Router::new().fallback(any(move || { + let counter = counter.clone(); + async move { + counter.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + (status, body) + } + }))) + .await; + (endpoint, hits) +} + /// A backend that answers every route with `200 OK` and a body that is not the /// JSON the adapter expects. /// @@ -94,10 +115,13 @@ async fn a_backend_failure_on_write_is_reported_rather_than_swallowed() { MemoryTaint::Internal, ) .await; + let error = result.expect_err("a 500 on write must surface"); assert!( - result.is_err(), - "{name}: a 500 on write must not report success — a caller that \ - believes the write landed has no reason to retry it" + matches!( + error.downcast_ref::(), + Some(MemoryError::Backend(_)) + ), + "{name}: a 500 must arrive typed as Backend, got: {error}" ); } } @@ -133,9 +157,13 @@ async fn an_unauthorized_backend_is_not_reported_as_an_empty_store() { let endpoint = failing(StatusCode::UNAUTHORIZED).await; for (name, memory) in adapters(&endpoint) { let listed = memory.list(None, None, None).await; + let error = listed.expect_err("a 401 must not present as an empty result set"); assert!( - listed.is_err(), - "{name}: a 401 must not present as an empty result set" + matches!( + error.downcast_ref::(), + Some(MemoryError::Unauthorized(_)) + ), + "{name}: a 401 must arrive typed as Unauthorized, got: {error}" ); let recalled = memory.recall("anything", 10, RecallOpts::default()).await; @@ -282,3 +310,142 @@ async fn a_paginated_export_terminates_instead_of_looping() { ); } } + +/// #68 review Major 1: deep health must be reachable through the PUBLIC +/// adapter types — the first cut implemented it on the inner composition and +/// every hand-delegating wrapper shadowed it with the trait default's `None`. +/// A 401 is `Down` naming the credential class; a 503 is `Degraded` (answered, +/// cannot serve). And per the review's redaction minor: the backend's error +/// body — which a vendor is free to fill with the rejected key — must NOT +/// reach the standing status reason. +#[tokio::test] +async fn public_adapters_probe_typed_health_with_redacted_reasons() { + let (unauthorized, _) = failing_with_body( + StatusCode::UNAUTHORIZED, + r#"{"detail":"bad key sk-SECRET123"}"#, + ) + .await; + for (name, memory) in adapters(&unauthorized) { + let health = memory + .health_probe() + .await + .unwrap_or_else(|| panic!("{name}: the public type must forward health_probe")); + assert_eq!(health.as_str(), "down", "{name}: a 401 is Down"); + let reason = health.reason().unwrap_or_default(); + assert!( + reason.contains("credential"), + "{name}: the reason names the class: {reason}" + ); + assert!( + !reason.contains("sk-SECRET123"), + "{name}: the backend's body must not reach the status surface: {reason}" + ); + } + + let (throttled, _) = failing_with_body(StatusCode::SERVICE_UNAVAILABLE, "busy").await; + for (name, memory) in adapters(&throttled) { + let health = memory + .health_probe() + .await + .unwrap_or_else(|| panic!("{name}: the public type must forward health_probe")); + assert_eq!( + health.as_str(), + "degraded", + "{name}: answered-but-cannot-serve is Degraded, not Down" + ); + } +} + +/// #68 review Major 5: a backend-side validation refusal (HTTP 400) must +/// arrive as `Invalid` — the class the tightened conformance refusal +/// assertion demands — never as `Backend`. +#[tokio::test] +async fn a_400_refusal_is_invalid_not_backend() { + let (endpoint, _) = failing_with_body( + StatusCode::BAD_REQUEST, + r#"{"error":"content must not be empty"}"#, + ) + .await; + for (name, memory) in adapters(&endpoint) { + let error = memory + .store_with_taint( + "ns", + "k", + "content", + MemoryCategory::Core, + None, + MemoryTaint::Internal, + ) + .await + .expect_err("a 400 must surface"); + assert!( + matches!( + error.downcast_ref::(), + Some(MemoryError::Invalid(_)) + ), + "{name}: a 400 must be Invalid, got: {error}" + ); + } +} + +/// #68 review Major 4: the retry split is now a per-call statement. A 503 on +/// a retrying READ is attempted three times; the same 503 on a WRITE path +/// (`empty` — no marker, no retry machinery at all) is attempted once. The +/// counter is the proof, not a comment. +#[tokio::test] +async fn transient_failures_retry_reads_three_times_and_writes_once() { + // Reads: every route 503s; the read path retries to its cap. + let (endpoint, hits) = failing_with_body(StatusCode::SERVICE_UNAVAILABLE, "busy").await; + let memory = SupermemoryMemory::api(&endpoint, "key").expect("client"); + let _ = memory.list(None, None, None).await; + assert_eq!( + hits.load(std::sync::atomic::Ordering::SeqCst), + 3, + "a transient read failure retries to the cap" + ); + + // Writes: the LIST half of upsert succeeds (empty page — nothing to + // update), so the create POST is the only thing that can fail. It must + // reach the backend exactly once: the write path has no retry machinery + // at all, and this counter — not a comment — is what pins the split + // (#68 review, Major 4). + let writes = std::sync::Arc::new(std::sync::atomic::AtomicU32::new(0)); + let counter = writes.clone(); + let app = Router::new() + .route( + "/v4/memories/list", + axum::routing::post(|| async { axum::Json(serde_json::json!({"memories": []})) }), + ) + .fallback(any(move || { + let counter = counter.clone(); + async move { + counter.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + StatusCode::SERVICE_UNAVAILABLE + } + })); + let write_endpoint = serve(app).await; + let memory = SupermemoryMemory::api(&write_endpoint, "key").expect("client"); + let error = memory + .store_with_taint( + "ns", + "k", + "v", + MemoryCategory::Core, + None, + MemoryTaint::Internal, + ) + .await + .expect_err("the 503 write must surface"); + assert!( + matches!( + error.downcast_ref::(), + Some(MemoryError::Unavailable(_)) + ), + "and typed: {error}" + ); + assert_eq!( + writes.load(std::sync::atomic::Ordering::SeqCst), + 1, + "a transient WRITE failure is attempted exactly once" + ); +} diff --git a/adapters/remote/src/mem0.rs b/adapters/remote/src/mem0.rs index e61dacd2..8cfc839f 100644 --- a/adapters/remote/src/mem0.rs +++ b/adapters/remote/src/mem0.rs @@ -13,7 +13,7 @@ use tinymemory_api::recall::RecallOpts; use tinymemory_api::traits::Memory; use tinymemory_api::types::MemoryTaint; -use crate::common::{category, Dialect, HttpClient, RemoteMemory, StoredEntry}; +use crate::common::{category, Attempts, Dialect, HttpClient, RemoteMemory, StoredEntry}; /// Stable driver id used by configuration and status output. pub use tinymemory_api::drivers::MEM0_DRIVER_ID; @@ -68,6 +68,26 @@ pub struct Mem0Memory { } impl Mem0Memory { + /// Rebuilds the HTTP transport with a different per-request deadline + /// (issue #18 follow-up U5). The default is 60s with a 10s connect + /// deadline — right for interactive calls; a bulk migration or a tight + /// liveness probe may want its own budget. + /// + /// # Errors + /// + /// Fails only if the underlying HTTP client cannot be rebuilt — a + /// configuration-time failure, before any request is made. + pub fn with_request_timeout(mut self, timeout: std::time::Duration) -> anyhow::Result { + let client = self + .inner + .dialect_mut() + .client + .clone() + .with_timeout(timeout)?; + self.inner.dialect_mut().client = client; + Ok(self) + } + /// Connect to a Mem0 REST server. /// /// `api_key` is sent as `X-API-Key`. Pass `None` only when the server is @@ -199,6 +219,13 @@ impl Memory for Mem0Memory { async fn health_check(&self) -> bool { self.inner.health_check().await } + /// Forwarded explicitly: this wrapper delegates method-by-method, so the + /// defaulted `None` would otherwise shadow `RemoteMemory`'s typed probe — + /// which is exactly what the first cut shipped, making §U4's deep health + /// unreachable through every public type (the #68 review's Major 1). + async fn health_probe(&self) -> Option { + self.inner.health_probe().await + } } #[derive(Debug)] @@ -242,7 +269,12 @@ impl Mem0Dialect { let top_k = Self::LISTING_TOP_K; let response: Value = self .client - .json(Method::GET, &format!("memories?top_k={top_k}"), None) + .json( + Method::GET, + &format!("memories?top_k={top_k}"), + None, + Attempts::RetryTransient, + ) .await?; let results = response .get("results") @@ -282,6 +314,7 @@ impl Mem0Dialect { Method::POST, &format!("v3/memories/?page={page}&page_size={CLOUD_PAGE_SIZE}"), Some(&json!({"filters": {"agent_id": CLOUD_AGENT_ID}})), + Attempts::RetryTransient, ) .await?; let results = response @@ -470,6 +503,7 @@ impl Dialect for Mem0Dialect { Value::Object(filters), opts.min_score, )), + Attempts::RetryTransient, ) .await? } @@ -489,6 +523,7 @@ impl Dialect for Mem0Dialect { Method::POST, "v3/memories/search/", Some(&Self::search_body(query, limit, filters, opts.min_score)), + Attempts::RetryTransient, ) .await? } @@ -518,9 +553,19 @@ impl Dialect for Mem0Dialect { Ok(true) } - /// Accepts either Mem0's health endpoint or its redirected root page. - async fn health(&self) -> bool { - self.client.healthy("api/health").await || self.client.healthy("").await + /// Probes Mem0's health endpoint, falling back to its redirected root + /// page — and unlike the old boolean `||`, a double failure reports BOTH + /// answers, so "the health route 404s but the root is throttled" stops + /// reading as a bare false. + async fn health(&self) -> anyhow::Result<()> { + let primary = match self.client.probe("api/health").await { + Ok(()) => return Ok(()), + Err(error) => error, + }; + self.client + .probe("") + .await + .map_err(|root| root.context(format!("health endpoint also failed: {primary}"))) } } diff --git a/adapters/remote/src/supermemory.rs b/adapters/remote/src/supermemory.rs index 633fa76e..f2eab3a4 100644 --- a/adapters/remote/src/supermemory.rs +++ b/adapters/remote/src/supermemory.rs @@ -7,7 +7,9 @@ use tinymemory_api::recall::RecallOpts; use tinymemory_api::traits::Memory; use tinymemory_api::types::MemoryTaint; -use crate::common::{category, stable_id, Dialect, HttpClient, RemoteMemory, StoredEntry}; +use crate::common::{ + category, stable_id, Attempts, Dialect, HttpClient, RemoteMemory, StoredEntry, +}; /// Stable driver id used by configuration and status output. pub use tinymemory_api::drivers::SUPERMEMORY_DRIVER_ID; @@ -22,6 +24,26 @@ pub struct SupermemoryMemory { } impl SupermemoryMemory { + /// Rebuilds the HTTP transport with a different per-request deadline + /// (issue #18 follow-up U5). The default is 60s with a 10s connect + /// deadline — right for interactive calls; a bulk migration or a tight + /// liveness probe may want its own budget. + /// + /// # Errors + /// + /// Fails only if the underlying HTTP client cannot be rebuilt — a + /// configuration-time failure, before any request is made. + pub fn with_request_timeout(mut self, timeout: std::time::Duration) -> anyhow::Result { + let client = self + .inner + .dialect_mut() + .client + .clone() + .with_timeout(timeout)?; + self.inner.dialect_mut().client = client; + Ok(self) + } + /// Connect to a Supermemory server using its bearer API key. /// /// # Errors @@ -143,6 +165,13 @@ impl Memory for SupermemoryMemory { async fn health_check(&self) -> bool { self.inner.health_check().await } + /// Forwarded explicitly: this wrapper delegates method-by-method, so the + /// defaulted `None` would otherwise shadow `RemoteMemory`'s typed probe — + /// which is exactly what the first cut shipped, making §U4's deep health + /// unreachable through every public type (the #68 review's Major 1). + async fn health_probe(&self) -> Option { + self.inner.health_probe().await + } } #[derive(Debug)] @@ -198,7 +227,14 @@ impl SupermemoryDialect { .get("tinymemory_session_id") .and_then(Value::as_str) .map(str::to_owned), - score: value.get("similarity").and_then(Value::as_f64), + // Both spellings: the adapter's own double and this crate's + // upstream-mirrored conformance double disagree ("similarity" vs + // "score"), and losing the number silently turns min_score into a + // drop-everything filter (#68 review, Major 2). + score: value + .get("similarity") + .or_else(|| value.get("score")) + .and_then(Value::as_f64), taint: metadata .get("tinymemory_taint") .and_then(Value::as_str) @@ -211,7 +247,12 @@ impl SupermemoryDialect { async fn memories(&self) -> anyhow::Result> { let tags: Value = self .client - .json(Method::GET, "v3/container-tags/list", None) + .json( + Method::GET, + "v3/container-tags/list", + None, + Attempts::RetryTransient, + ) .await?; let container_tags = tags .as_array() @@ -252,6 +293,7 @@ impl SupermemoryDialect { "order": "desc", "containerTags": [container_tag] })), + Attempts::RetryTransient, ) .await?; let memories = response @@ -368,7 +410,12 @@ impl Dialect for SupermemoryDialect { } let response: Value = self .client - .json(Method::POST, "v4/search", Some(&body)) + .json( + Method::POST, + "v4/search", + Some(&body), + Attempts::RetryTransient, + ) .await?; Ok(response .get("results") @@ -398,9 +445,14 @@ impl Dialect for SupermemoryDialect { Ok(true) } - /// Checks the local server root, its stable availability endpoint. - async fn health(&self) -> bool { - self.client.healthy("").await + /// Probes `v3/container-tags/list` — the cheapest endpoint that proves + /// BOTH the credential and the data plane (issue #18 §U4). The old + /// root-page probe answered 200 to an unauthenticated client against a + /// live server, so a wrong key looked healthy until the first real call. + async fn health(&self) -> anyhow::Result<()> { + // Status-only: a 200 from this authenticated route is the proof; the + // body's shape is the data path's concern, not the probe's. + self.client.probe("v3/container-tags/list").await } } diff --git a/adapters/remote/src/supermemory_test.rs b/adapters/remote/src/supermemory_test.rs index d3d37ac6..d72a6b2f 100644 --- a/adapters/remote/src/supermemory_test.rs +++ b/adapters/remote/src/supermemory_test.rs @@ -108,8 +108,11 @@ async fn capture_auth(State(state): State>>, headers: HeaderMap #[tokio::test] async fn supermemory_supports_provided_and_self_hosted_apis() { let captured = Arc::new(Mutex::new(Value::Null)); + // The health probe now proves auth + data plane via the container-tags + // list (§U4), so that is where the capture sits — a bare root `/` no + // longer receives the probe. let app = Router::new() - .route("/", get(capture_auth)) + .route("/v3/container-tags/list", get(capture_auth)) .with_state(captured.clone()); let listener = tokio::net::TcpListener::bind("127.0.0.1:0") .await @@ -221,6 +224,40 @@ async fn native_supermemory_round_trips_the_tinymemory_contract() { state.0.lock().expect("state lock").last_search_tag, Some(expected_tag) ); + // #68 review Major 2: min_score connected to the double's OWN response + // shape — the first cut's strictness was only ever tested against + // synthetic Option values, which is how a decode/emit field mismatch + // dropped every hit. Below the double's similarity (0.95): survives. + // Above it: drops. Semantics AND the decode, in one pair. + let scored = |min: f64| { + let driver = &driver; + async move { + driver + .recall( + "Rust", + 1, + &OwnedRecallOpts { + namespace: Some("project".into()), + min_score: Some(min), + ..OwnedRecallOpts::default() + }, + None, + ) + .await + .expect("recall") + .len() + } + }; + assert_eq!( + scored(0.1).await, + 1, + "a scored hit above the threshold survives" + ); + assert_eq!( + scored(0.99).await, + 0, + "a scored hit below the threshold drops" + ); assert!(driver.forget("project", "decision").await.expect("forget")); assert!(driver.health().await.is_usable()); } diff --git a/adapters/tinycortex/src/memory.rs b/adapters/tinycortex/src/memory.rs index 32359736..8bd055e5 100644 --- a/adapters/tinycortex/src/memory.rs +++ b/adapters/tinycortex/src/memory.rs @@ -11,6 +11,7 @@ use std::sync::Arc; use async_trait::async_trait; +use tinymemory_api::error::MemoryError; use tinymemory_api::recall::OwnedRecallOpts; use tinymemory_api::traits::Memory; use tinymemory_api::types::{ @@ -60,6 +61,18 @@ impl Memory for TinycortexMemory { category: MemoryCategory, session_id: Option<&str>, ) -> anyhow::Result<()> { + // §A4: the engine refuses empty content with an opaque anyhow + // (vendor/tinycortex, outside this repo's reach), which the mandatory + // composition can only flatten to `Other` — indistinguishable from a + // backend failure. Enforce the same documented rule HERE, typed, so + // the refusal arrives as the `Invalid` the conformance suite now + // requires. Not message-sniffing: this mirrors the engine's stated + // contract, it does not parse its prose. + if content.trim().is_empty() { + return Err(anyhow::Error::new(MemoryError::Invalid( + "memory content cannot be empty".to_string(), + ))); + } self.inner .store(namespace, key, content, category, session_id) .await @@ -79,6 +92,12 @@ impl Memory for TinycortexMemory { session_id: Option<&str>, taint: MemoryTaint, ) -> anyhow::Result<()> { + // Same typed refusal as `store` — see the comment there. + if content.trim().is_empty() { + return Err(anyhow::Error::new(MemoryError::Invalid( + "memory content cannot be empty".to_string(), + ))); + } self.inner .store_with_taint(namespace, key, content, category, session_id, taint) .await diff --git a/api/src/error.rs b/api/src/error.rs index c9e64056..b7e76a02 100644 --- a/api/src/error.rs +++ b/api/src/error.rs @@ -22,7 +22,14 @@ use thiserror::Error; use crate::capabilities::Capability; /// Errors surfaced by the memory engine. +/// +/// `#[non_exhaustive]` (issue #18 §A4): downstream hosts must keep a wildcard +/// arm, so the *next* class this enum learns to name is an upgrade for them, +/// not a breakage. Inside this workspace `wire::wire_name` still matches +/// exhaustively — a new variant is a compile error there, never a silent +/// fallthrough onto `OTHER`. #[derive(Debug, Error)] +#[non_exhaustive] pub enum MemoryError { /// A requested record / source / node was not found. #[error("not found: {0}")] @@ -73,6 +80,31 @@ pub enum MemoryError { /// driver reported. capability: String, }, + /// The backend rejected the configured credential, or required one that + /// was never configured (HTTP 401 / 403). Retrying cannot help; fixing + /// the key can. The message names the host and the auth scheme's hint, + /// never the credential itself. + #[error("unauthorized: {0}")] + Unauthorized(String), + /// The backend could not be reached at all — connection refused, DNS + /// resolution failed, or the TLS handshake broke. The request never + /// arrived, so nothing was applied. + #[error("unreachable: {0}")] + Unreachable(String), + /// The call exceeded its deadline. Whether the backend applied the work + /// is unknown — which is why write paths must never blindly retry this. + #[error("timed out: {0}")] + Timeout(String), + /// The backend answered that it cannot serve right now (HTTP 429, 502, + /// 503, 504): the retryable class, distinct from [`MemoryError::Backend`] + /// so a retry policy can key on it without parsing prose. + #[error("unavailable: {0}")] + Unavailable(String), + /// The backend answered, and the answer was a failure this contract has + /// no more specific name for (an unexpected 4xx/5xx with its status and + /// bounded body in the message). + #[error("backend failed: {0}")] + Backend(String), /// Catch-all wrapping an opaque lower-level error. #[error(transparent)] Other(#[from] anyhow::Error), diff --git a/api/src/mandatory/mod.rs b/api/src/mandatory/mod.rs index 578fc9da..f427268c 100644 --- a/api/src/mandatory/mod.rs +++ b/api/src/mandatory/mod.rs @@ -77,7 +77,15 @@ pub const SCOPE_UNAPPLIED: &str = /// actually known. #[must_use] pub fn engine_error(error: anyhow::Error) -> MemoryError { - MemoryError::Other(error) + // §A4: an adapter that already knows the failure's class attaches a typed + // MemoryError as the anyhow payload (adapters/remote/src/common.rs does + // for transport and HTTP-status failures). Recover it here instead of + // flattening everything to Other — this one line is what makes + // "unauthorized" distinguishable from "unreachable" across every + // `Memory`-backed provider without touching the trait's signatures. + error + .downcast::() + .unwrap_or_else(MemoryError::Other) } /// `MemoryCore::list` for the all-namespaces case. diff --git a/api/src/mandatory/provider.rs b/api/src/mandatory/provider.rs index 5eee27f8..2c4f46ed 100644 --- a/api/src/mandatory/provider.rs +++ b/api/src/mandatory/provider.rs @@ -188,6 +188,12 @@ impl MemoryProvider for MemoryTraitProvider { } async fn health(&self) -> MemoryHealth { + // A backend that can say more than a boolean does (issue #18 §U4): + // the remote adapters report the typed probe outcome — credential + // rejected, unreachable, throttled — instead of one frozen string. + if let Some(health) = self.memory.health_probe().await { + return health; + } if self.memory.health_check().await { MemoryHealth::Ready } else { diff --git a/api/src/traits.rs b/api/src/traits.rs index c96e0b9b..bf488658 100644 --- a/api/src/traits.rs +++ b/api/src/traits.rs @@ -154,4 +154,17 @@ pub trait Memory: Send + Sync { /// rather than `Err`, so it is safe to call from a liveness probe without /// error-handling boilerplate. async fn health_check(&self) -> bool; + + /// Rich health, when the backend can say more than a boolean (issue #18 + /// follow-up U4). + /// + /// `None` — the default every existing implementation inherits — means + /// "this backend only knows the boolean"; callers fall back to + /// [`Memory::health_check`]. `Some(health)` carries the typed answer: + /// `Down`/`Degraded` with a reason naming the failure class (credential + /// rejected, unreachable, throttled), never a credential or a payload — + /// the reason string reaches operator-facing status surfaces. + async fn health_probe(&self) -> Option { + None + } } diff --git a/api/src/wire.rs b/api/src/wire.rs index 0cba4efe..02a6e61f 100644 --- a/api/src/wire.rs +++ b/api/src/wire.rs @@ -66,6 +66,16 @@ pub const SERDE: &str = "ai.tinyhumans.tinymemory.Error.Serde"; pub const UNSUPPORTED: &str = "ai.tinyhumans.tinymemory.Error.Unsupported"; /// An opaque lower-level failure. pub const OTHER: &str = "ai.tinyhumans.tinymemory.Error.Other"; +/// The backend rejected or required a credential (§A4). +pub const UNAUTHORIZED: &str = "ai.tinyhumans.tinymemory.Error.Unauthorized"; +/// The backend could not be reached (connect / DNS / TLS) (§A4). +pub const UNREACHABLE: &str = "ai.tinyhumans.tinymemory.Error.Unreachable"; +/// The call exceeded its deadline (§A4). +pub const TIMEOUT: &str = "ai.tinyhumans.tinymemory.Error.Timeout"; +/// The backend answered that it cannot serve right now (§A4). +pub const UNAVAILABLE: &str = "ai.tinyhumans.tinymemory.Error.Unavailable"; +/// The backend answered with an otherwise-unclassified failure (§A4). +pub const BACKEND: &str = "ai.tinyhumans.tinymemory.Error.Backend"; /// The wire name for `error`. /// @@ -82,6 +92,11 @@ pub fn wire_name(error: &MemoryError) -> &'static str { MemoryError::Io(_) => IO, MemoryError::Serde(_) => SERDE, MemoryError::Unsupported { .. } => UNSUPPORTED, + MemoryError::Unauthorized(_) => UNAUTHORIZED, + MemoryError::Unreachable(_) => UNREACHABLE, + MemoryError::Timeout(_) => TIMEOUT, + MemoryError::Unavailable(_) => UNAVAILABLE, + MemoryError::Backend(_) => BACKEND, MemoryError::Other(_) => OTHER, } } @@ -97,7 +112,12 @@ pub fn wire_message(error: &MemoryError) -> String { MemoryError::NotFound(message) | MemoryError::Invalid(message) | MemoryError::BudgetExceeded(message) - | MemoryError::PathEscape(message) => message.clone(), + | MemoryError::PathEscape(message) + | MemoryError::Unauthorized(message) + | MemoryError::Unreachable(message) + | MemoryError::Timeout(message) + | MemoryError::Unavailable(message) + | MemoryError::Backend(message) => message.clone(), MemoryError::Unsupported { capability } => capability.clone(), // No inner string to lift: these carry a foreign error type, so the // rendered form is all there is. @@ -124,6 +144,11 @@ pub fn from_wire(name: &str, message: &str) -> MemoryError { IO => MemoryError::Other(anyhow::anyhow!("io error: {message}")), SERDE => MemoryError::Other(anyhow::anyhow!("serde error: {message}")), UNSUPPORTED => MemoryError::unsupported_raw(message), + UNAUTHORIZED => MemoryError::Unauthorized(message.to_string()), + UNREACHABLE => MemoryError::Unreachable(message.to_string()), + TIMEOUT => MemoryError::Timeout(message.to_string()), + UNAVAILABLE => MemoryError::Unavailable(message.to_string()), + BACKEND => MemoryError::Backend(message.to_string()), _ => MemoryError::Other(anyhow::anyhow!("{message}")), } } diff --git a/api/src/wire_tests.rs b/api/src/wire_tests.rs index 0653dc6d..22f00855 100644 --- a/api/src/wire_tests.rs +++ b/api/src/wire_tests.rs @@ -102,3 +102,31 @@ fn an_unknown_capability_name_off_the_wire_survives() { other => panic!("expected Unsupported, got {other:?}"), } } + +/// §A4: the five typed classes round-trip the wire with their messages, and +/// an OLD host that has never heard the new names degrades them to `Other` +/// (the `_ =>` fallback) rather than mislabeling them. +#[test] +fn a4_variants_round_trip_and_degrade_gracefully() { + let cases = [ + MemoryError::Unauthorized("401 from host".into()), + MemoryError::Unreachable("dns failed".into()), + MemoryError::Timeout("deadline".into()), + MemoryError::Unavailable("503".into()), + MemoryError::Backend("500 detail".into()), + ]; + for error in cases { + let name = wire_name(&error); + let message = wire_message(&error); + let rebuilt = from_wire(name, &message); + assert_eq!(wire_name(&rebuilt), name, "round trip changed the class"); + assert_eq!( + wire_message(&rebuilt), + message, + "round trip changed the message" + ); + } + // The unknown-name fallback IS the compatibility story. + let degraded = from_wire("ai.tinyhumans.tinymemory.Error.SomethingNewer", "detail"); + assert!(matches!(degraded, MemoryError::Other(_))); +} diff --git a/conformance/src/suite/mod.rs b/conformance/src/suite/mod.rs index 64754dd1..ff271916 100644 --- a/conformance/src/suite/mod.rs +++ b/conformance/src/suite/mod.rs @@ -24,6 +24,7 @@ use std::sync::Arc; use tinymemory_api::capabilities::Capability; +use tinymemory_api::error::MemoryError; use tinymemory_api::provider::{audit_provider, ExportRecord, MemoryProvider}; use tinymemory_api::recall::OwnedRecallOpts; use tinymemory_api::types::{MemoryCategory, MemoryTaint}; @@ -608,14 +609,12 @@ pub async fn assert_awkward_content_round_trips(provider: &dyn MemoryProvider) { // engine uses that to refuse empty content. What a driver may *not* do // is accept a value and hand back something else. // - // The refusal is not yet required to be `Invalid` specifically. The - // engine's own error is flattened through `anyhow` before the mandatory - // composition sees it, so a validation refusal currently arrives as - // `Other` and is indistinguishable from a backend failure. That is the - // gap §A4 closes; when it does, this should tighten to require - // `MemoryError::Invalid` so a genuine backend failure stops passing - // here. - if provider + // §A4 landed: a refusal must now be `Invalid` — the caller's input + // was rejected — never a transport/backend class wearing a refusal's + // clothes. A driver that answers `Unauthorized` or `Backend` here is + // not refusing the shape, it is failing, and failure must fail the + // suite instead of reading as a documented refusal. + match provider .store( &ns, key, @@ -625,9 +624,12 @@ pub async fn assert_awkward_content_round_trips(provider: &dyn MemoryProvider) { MemoryTaint::Internal, ) .await - .is_err() { - continue; + Ok(()) => {} + Err(MemoryError::Invalid(_)) => continue, + Err(other) => panic!( + "{who}: refusing `{key}` must be Invalid (a validation refusal); got: {other}" + ), } accepted.push(key); if let Some(got) = provider diff --git a/core/src/store/namespace_store/events.rs b/core/src/store/namespace_store/events.rs index c03bf076..2772f621 100644 --- a/core/src/store/namespace_store/events.rs +++ b/core/src/store/namespace_store/events.rs @@ -465,9 +465,10 @@ fn vec_to_bytes(v: &[f32]) -> Vec { } fn bytes_to_vec(bytes: &[u8]) -> Vec { - bytes - .chunks_exact(4) - .map(|chunk| f32::from_le_bytes([chunk[0], chunk[1], chunk[2], chunk[3]])) + let (chunks, _remainder) = bytes.as_chunks::<4>(); + chunks + .iter() + .map(|chunk| f32::from_le_bytes(*chunk)) .collect() } diff --git a/core/src/store/namespace_store/helpers.rs b/core/src/store/namespace_store/helpers.rs index 11d184bd..9db6317a 100644 --- a/core/src/store/namespace_store/helpers.rs +++ b/core/src/store/namespace_store/helpers.rs @@ -56,12 +56,10 @@ impl UnifiedMemory { } pub(crate) fn bytes_to_vec(bytes: &[u8]) -> Vec { - bytes - .chunks_exact(4) - .map(|chunk| { - let arr: [u8; 4] = chunk.try_into().unwrap_or([0; 4]); - f32::from_le_bytes(arr) - }) + let (chunks, _remainder) = bytes.as_chunks::<4>(); + chunks + .iter() + .map(|chunk| f32::from_le_bytes(*chunk)) .collect() } diff --git a/core/src/store/namespace_store/segments.rs b/core/src/store/namespace_store/segments.rs index 3af779b1..de118bbf 100644 --- a/core/src/store/namespace_store/segments.rs +++ b/core/src/store/namespace_store/segments.rs @@ -600,9 +600,10 @@ fn vec_to_bytes(v: &[f32]) -> Vec { } fn bytes_to_vec(bytes: &[u8]) -> Vec { - bytes - .chunks_exact(4) - .map(|chunk| f32::from_le_bytes([chunk[0], chunk[1], chunk[2], chunk[3]])) + let (chunks, _remainder) = bytes.as_chunks::<4>(); + chunks + .iter() + .map(|chunk| f32::from_le_bytes(*chunk)) .collect() } diff --git a/core/src/tree/score/embed/mod.rs b/core/src/tree/score/embed/mod.rs index 2181e62b..8afa40ba 100644 --- a/core/src/tree/score/embed/mod.rs +++ b/core/src/tree/score/embed/mod.rs @@ -313,10 +313,8 @@ pub fn unpack_embedding(b: &[u8]) -> Result> { b.len() ); } - let floats: Vec = b - .chunks_exact(4) - .map(|c| f32::from_le_bytes([c[0], c[1], c[2], c[3]])) - .collect(); + let (chunks, _remainder) = b.as_chunks::<4>(); + let floats: Vec = chunks.iter().map(|c| f32::from_le_bytes(*c)).collect(); if floats.len() != EMBEDDING_DIM { anyhow::bail!( "embedding blob length {} floats, expected {}", diff --git a/crates/tinymemory-module/Cargo.lock b/crates/tinymemory-module/Cargo.lock index ee5c912b..d7035717 100644 --- a/crates/tinymemory-module/Cargo.lock +++ b/crates/tinymemory-module/Cargo.lock @@ -144,7 +144,7 @@ checksum = "d524456ba66e72eb8b115ff89e01e497f8e6d11d78b70b1aa13c0fbd97540a81" dependencies = [ "cfg-if", "cpufeatures 0.3.0", - "rand_core 0.10.1", + "rand_core", ] [[package]] @@ -263,7 +263,16 @@ version = "5.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "44c45a9d03d6676652bcb5e724c7e988de1acad23a711b5217ab9cbecbec2225" dependencies = [ - "dirs-sys", + "dirs-sys 0.4.1", +] + +[[package]] +name = "dirs" +version = "6.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c3e8aa94d75141228480295a7d0e7feb620b1a5ad9f12bc40be62411e38cce4e" +dependencies = [ + "dirs-sys 0.5.0", ] [[package]] @@ -274,10 +283,22 @@ checksum = "520f05a5cbd335fae5a99ff7a6ab8627577660ee5cfd6a94a6a929b52ff0321c" dependencies = [ "libc", "option-ext", - "redox_users", + "redox_users 0.4.6", "windows-sys 0.48.0", ] +[[package]] +name = "dirs-sys" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e01a3366d27ee9890022452ee61b2b63a67e6f13f58900b651ff5665f0bb1fab" +dependencies = [ + "libc", + "option-ext", + "redox_users 0.5.2", + "windows-sys 0.61.2", +] + [[package]] name = "displaydoc" version = "0.2.7" @@ -491,7 +512,7 @@ dependencies = [ "js-sys", "libc", "r-efi", - "rand_core 0.10.1", + "rand_core", "wasm-bindgen", ] @@ -998,15 +1019,6 @@ dependencies = [ "zerovec", ] -[[package]] -name = "ppv-lite86" -version = "0.2.21" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "85eae3c4ed2f50dcfe72643da4befc30deadb458a9b590d720cde2f2b1e97da9" -dependencies = [ - "zerocopy", -] - [[package]] name = "proc-macro2" version = "1.0.107" @@ -1045,7 +1057,7 @@ dependencies = [ "bytes", "getrandom 0.4.3", "lru-slab", - "rand 0.10.2", + "rand", "rand_pcg", "ring", "rustc-hash", @@ -1087,17 +1099,6 @@ version = "6.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f8dcc9c7d52a811697d2151c701e0d08956f92b0e24136cf4cf27b57a6a0d9bf" -[[package]] -name = "rand" -version = "0.8.7" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "22f6172bdec972074665ed81ed53b71da00bfc44b65a753cfde883ec4c702a1a" -dependencies = [ - "libc", - "rand_chacha", - "rand_core 0.6.4", -] - [[package]] name = "rand" version = "0.10.2" @@ -1106,26 +1107,7 @@ checksum = "c7f5fa3a058cd35567ef9bfa5e75732bee0f9e4c55fa90477bef2dfcdbc4be80" dependencies = [ "chacha20", "getrandom 0.4.3", - "rand_core 0.10.1", -] - -[[package]] -name = "rand_chacha" -version = "0.3.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e6c10a63a0fa32252be49d21e7709d4d4baf8d231c2dbce1eaa8141b9b127d88" -dependencies = [ - "ppv-lite86", - "rand_core 0.6.4", -] - -[[package]] -name = "rand_core" -version = "0.6.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ec0be4795e2f6a28069bec0b5ff3e2ac9bafc99e6a9a7dc3547996c5c816922c" -dependencies = [ - "getrandom 0.2.17", + "rand_core", ] [[package]] @@ -1140,7 +1122,7 @@ version = "0.10.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "caa0f4137e1c0a72f4c651489402276c8e8e1cf081f3b0ba156d2cbeef09e86a" dependencies = [ - "rand_core 0.10.1", + "rand_core", ] [[package]] @@ -1163,6 +1145,17 @@ dependencies = [ "thiserror 1.0.69", ] +[[package]] +name = "redox_users" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a4e608c6638b9c18977b00b475ac1f28d14e84b27d8d42f70e0bf1e3dec127ac" +dependencies = [ + "getrandom 0.2.17", + "libredox", + "thiserror 2.0.20", +] + [[package]] name = "ref-cast" version = "1.0.26" @@ -1744,13 +1737,13 @@ dependencies = [ "anyhow", "async-trait", "chrono", - "dirs", + "dirs 5.0.1", "futures", "git2", "hex", "log", "parking_lot", - "rand 0.10.2", + "rand", "regex", "reqwest", "rusqlite", @@ -1810,11 +1803,11 @@ dependencies = [ "anyhow", "async-trait", "chrono", - "dirs", + "dirs 6.0.0", "futures", "log", "parking_lot", - "rand 0.8.7", + "rand", "regex", "reqwest", "rusqlite", @@ -1873,7 +1866,7 @@ dependencies = [ "serde_json", "tinymemory-api", "tokio", - "toml 0.9.12+spec-1.1.0", + "toml 1.1.4+spec-1.1.0", "tracing", "uuid", "walkdir", @@ -1995,21 +1988,6 @@ dependencies = [ "toml_edit", ] -[[package]] -name = "toml" -version = "0.9.12+spec-1.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cf92845e79fc2e2def6a5d828f0801e29a2f8acc037becc5ab08595c7d5e9863" -dependencies = [ - "indexmap", - "serde_core", - "serde_spanned 1.1.1", - "toml_datetime 0.7.5+spec-1.1.0", - "toml_parser", - "toml_writer", - "winnow 0.7.15", -] - [[package]] name = "toml" version = "1.1.4+spec-1.1.0" @@ -2034,15 +2012,6 @@ dependencies = [ "serde", ] -[[package]] -name = "toml_datetime" -version = "0.7.5+spec-1.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "92e1cfed4a3038bc5a127e35a2d360f145e1f4b971b551a2ba5fd7aedf7e1347" -dependencies = [ - "serde_core", -] - [[package]] name = "toml_datetime" version = "1.1.1+spec-1.1.0" @@ -2656,26 +2625,6 @@ dependencies = [ "synstructure", ] -[[package]] -name = "zerocopy" -version = "0.8.56" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "556764e583adb45a9f8d413c2a147fa7e8d821e48e12b14fd560b607998b75eb" -dependencies = [ - "zerocopy-derive", -] - -[[package]] -name = "zerocopy-derive" -version = "0.8.56" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f2ab42fc20575779bd240faa45f94a74256f755c0fa9e89f0ede20d91d0cdfc1" -dependencies = [ - "proc-macro2", - "quote", - "syn 2.0.119", -] - [[package]] name = "zerofrom" version = "0.1.8" diff --git a/crates/tinymemory-module/src/embedding_test.rs b/crates/tinymemory-module/src/embedding_test.rs index 3236a8f5..f90ada33 100644 --- a/crates/tinymemory-module/src/embedding_test.rs +++ b/crates/tinymemory-module/src/embedding_test.rs @@ -27,7 +27,11 @@ struct FakeHostEmbedder { #[tinybus::interface(name = "ai.tinyhumans.tinymemory.EmbeddingHost")] impl FakeHostEmbedder { - #[allow(clippy::unused_async, reason = "the interface macro requires async")] + #[allow( + clippy::unused_async, + clippy::unused_async_trait_impl, + reason = "the interface macro requires async" + )] async fn embed( &self, _model: String, diff --git a/crates/tinymemory-module/src/service/mod.rs b/crates/tinymemory-module/src/service/mod.rs index eba50abb..afee3d8c 100644 --- a/crates/tinymemory-module/src/service/mod.rs +++ b/crates/tinymemory-module/src/service/mod.rs @@ -256,6 +256,7 @@ impl MemoryService { /// The bound driver's stable identifier. #[allow( clippy::unused_async, + clippy::unused_async_trait_impl, reason = "tinybus::interface requires every method to be `async fn`" )] async fn driver_id(&self) -> BusResult { @@ -269,6 +270,7 @@ impl MemoryService { /// change afterwards. #[allow( clippy::unused_async, + clippy::unused_async_trait_impl, reason = "tinybus::interface requires every method to be `async fn`" )] async fn capabilities(&self) -> BusResult { diff --git a/crates/tinymemory-module/tests/module_e2e.rs b/crates/tinymemory-module/tests/module_e2e.rs index 7c24f776..49af10e2 100644 --- a/crates/tinymemory-module/tests/module_e2e.rs +++ b/crates/tinymemory-module/tests/module_e2e.rs @@ -69,7 +69,11 @@ struct HostEmbedder; #[tinybus::interface(name = "ai.tinyhumans.tinymemory.EmbeddingHost")] impl HostEmbedder { - #[allow(clippy::unused_async, reason = "the interface macro requires async")] + #[allow( + clippy::unused_async, + clippy::unused_async_trait_impl, + reason = "the interface macro requires async" + )] async fn embed( &self, _model: String, diff --git a/integration/remote-engines/README.md b/integration/remote-engines/README.md index c7600001..3b8256af 100644 --- a/integration/remote-engines/README.md +++ b/integration/remote-engines/README.md @@ -33,11 +33,14 @@ cargo run -p tinymemory-remote --example conformance -- \ supermemory https://api.supermemory.ai "$SUPERMEMORY_API_KEY" cargo run -p tinymemory-remote --example conformance -- \ - cognee-api https://api.cognee.ai "$COGNEE_API_KEY" + cognee-api "https://tenant-.aws.cognee.ai" "$COGNEE_API_KEY" ``` -For tenant-specific Cognee deployments, replace the shared endpoint with the -tenant URL issued by Cognee. The command writes a unique conformance namespace, +Cognee Cloud has **no shared endpoint** — `api.cognee.ai` resolves in DNS but +nothing listens there (see the constructor note in +`adapters/remote/src/cognee.rs`), which is why this crate exports no default +Cognee endpoint constant. The tenant URL printed beside your API key on the +Cognee dashboard is the only address that exists; substitute it above. The command writes a unique conformance namespace, verifies Core, Recall, and Portability, and deletes its test record before exiting.