diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index c5250f3..1dc8c29 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -38,7 +38,11 @@ jobs: # their `checksum.toml` — that is what a host pins and verifies. tag: name: Tag release - if: ${{ github.ref == 'refs/heads/main' }} + # `existing_tag` means "re-cut artifacts for a tag that already exists", so + # this job must not run: it would bump the version and cut a *second*, newer + # tag, and `release-target` would then build the older tag the caller asked + # for while `main` had silently moved on. + if: ${{ github.ref == 'refs/heads/main' && inputs.existing_tag == '' }} # Consumed by `release-target`. The `version` step already writes both to # `$GITHUB_OUTPUT`; without this block they stop at the job boundary and the # module bundles resolve an empty tag. @@ -303,10 +307,14 @@ jobs: $packageName = "tinymemory-module-$env:VERSION-$env:BUNDLE_ID" $packageRoot = "dist/$packageName" New-Item -ItemType Directory -Force $packageRoot | Out-Null - Copy-Item -LiteralPath $module, 'LICENSE', 'README.md' -Destination $packageRoot + # Same file set as the Unix package, including the spec — a consumer + # should not get different contents depending on their platform. + Copy-Item -LiteralPath $module, 'LICENSE', 'README.md', 'docs/specs/tinybus-module.md' -Destination $packageRoot $hash = (Get-FileHash -LiteralPath $module -Algorithm SHA256).Hash.ToLowerInvariant() $moduleName = Split-Path -Leaf $module - "`"$moduleName`" = `"$hash`"`n" | + # No trailing "`n": Set-Content adds its own terminator, so writing one + # here leaves a blank line the Unix `printf` form does not produce. + "`"$moduleName`" = `"$hash`"" | Set-Content -Path "$packageRoot/modules.toml" -Encoding utf8NoBOM Compress-Archive -Path "$packageRoot/*" -DestinationPath "dist/$packageName.zip" "archive=dist/$packageName.zip" >> $env:GITHUB_OUTPUT @@ -382,11 +390,21 @@ jobs: run: | set -euo pipefail mapfile -t release_files < <(find release-assets -type f | sort) - gh release create "$RELEASE_TAG" "${release_files[@]}" \ - --repo "$REPOSITORY" \ - --verify-tag \ - --title "$RELEASE_TAG" \ - --generate-notes + # Re-cutting artifacts for an existing tag is what `existing_tag` is + # for, and a release for that tag usually already exists — so upload + # into it rather than failing on `already exists`. `--clobber` makes + # the re-cut idempotent instead of erroring on the second asset name. + if gh release view "$RELEASE_TAG" --repo "$REPOSITORY" >/dev/null 2>&1; then + echo "release ${RELEASE_TAG} exists; uploading assets into it" + gh release upload "$RELEASE_TAG" "${release_files[@]}" \ + --repo "$REPOSITORY" --clobber + else + gh release create "$RELEASE_TAG" "${release_files[@]}" \ + --repo "$REPOSITORY" \ + --verify-tag \ + --title "$RELEASE_TAG" \ + --generate-notes + fi - name: Verify the published module through TinyBus shell: bash diff --git a/crates/tinymemory-module/src/embedding.rs b/crates/tinymemory-module/src/embedding.rs index 7f9b77f..c9790d2 100644 --- a/crates/tinymemory-module/src/embedding.rs +++ b/crates/tinymemory-module/src/embedding.rs @@ -272,17 +272,22 @@ impl EmbeddingProvider for BusEmbeddingProvider { texts.len() ); } - if self.dimensions > 0 { - if let Some(bad) = vectors - .iter() - .find(|vector| vector.len() != self.dimensions) - { - anyhow::bail!( - "host returned a {}-dimension vector for a {}-dimension space", - bad.len(), - self.dimensions - ); - } + // Checked unconditionally, including for a zero-dimension provider. An + // earlier revision skipped the check entirely when `dimensions == 0`, + // which let a host answer a "semantic search off" request with real + // 768-wide vectors and pass — precisely the split-embedding-space + // failure this check exists to prevent, except that the engine would + // additionally believe no vectors existed at all. Zero dimensions means + // empty vectors, and this is what says so. + if let Some(bad) = vectors + .iter() + .find(|vector| vector.len() != self.dimensions) + { + anyhow::bail!( + "host returned a {}-dimension vector for a {}-dimension space", + bad.len(), + self.dimensions + ); } Ok(vectors) diff --git a/crates/tinymemory-module/src/embedding_test.rs b/crates/tinymemory-module/src/embedding_test.rs index 285b52d..3236a8f 100644 --- a/crates/tinymemory-module/src/embedding_test.rs +++ b/crates/tinymemory-module/src/embedding_test.rs @@ -139,10 +139,9 @@ async fn a_wrong_vector_count_is_refused() { } #[tokio::test] -async fn a_zero_dimension_provider_is_exempt_from_the_width_check() { - // Zero dimensions is the engine's "semantic search off" state and is - // expected to yield empty vectors; enforcing a width there would break - // keyword-only retrieval. +async fn a_zero_dimension_provider_yields_empty_vectors() { + // Zero dimensions is the engine's "semantic search off" state, and the + // vectors it yields are expected to be empty rather than merely unchecked. let connection = bus_with_host(FakeHostEmbedder { width: 0, force_count: None, @@ -159,6 +158,29 @@ async fn a_zero_dimension_provider_is_exempt_from_the_width_check() { assert!(vectors[0].is_empty()); } +#[tokio::test] +async fn a_zero_dimension_request_answered_with_real_vectors_is_refused() { + // The case an earlier revision let through: `dimensions == 0` skipped the + // width check outright, so a host could answer a "semantic search off" + // request with a real 768-wide space and pass validation. The engine would + // then believe no vectors existed while the store filled with embeddings + // from a space nothing tracks — the split-space failure, with the split + // hidden. Zero means empty, and this is the test that says so. + let connection = bus_with_host(FakeHostEmbedder { + width: 768, + force_count: None, + }) + .await; + let host = BusEmbeddingHost::new(connection, &config_with_dims(0)); + let provider = host.default_embedding_provider(); + + let error = provider + .embed(&["alpha"]) + .await + .expect_err("a 768-wide answer to a zero-dimension request must be refused"); + assert!(error.to_string().contains("768"), "{error}"); +} + #[tokio::test] async fn an_empty_batch_never_reaches_the_bus() { // No host is served here at all, so this only passes if the call short diff --git a/crates/tinymemory-module/src/lib.rs b/crates/tinymemory-module/src/lib.rs index c38bd2c..64f885a 100644 --- a/crates/tinymemory-module/src/lib.rs +++ b/crates/tinymemory-module/src/lib.rs @@ -74,6 +74,7 @@ pub use embedding::{ }; pub use service::{BUS_NAME, OBJECT_PATH}; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; use tinybus::{Connection, Error as BusError, Result as BusResult}; @@ -99,6 +100,7 @@ const SETUP_FAILED_ERROR: &str = "ai.tinyhumans.tinymemory.Error.SetupFailed"; /// the host, which holds the real credential, so there is nothing to pass and /// nothing here that could leak one. async fn setup(connection: Connection, mut config: ModuleConfig) -> BusResult<()> { + claim_process_setup()?; config.validate().map_err(setup_error)?; // `MemoryConfig` travels verbatim, and it contains a bearer token field for a @@ -133,12 +135,49 @@ async fn setup(connection: Connection, mut config: ModuleConfig) -> BusResult<() config.storage_provider.as_ref(), &config.workspace_dir, ) - .map_err(|error| setup_error(format!("create memory store: {error}")))?; + .map_err(|error| { + // The factory error names the workspace directory it failed under, and + // a `MethodFailed.message` crosses the bus to a caller that has no + // business learning this process's filesystem layout. The detail stays + // in the module's own log; the wire gets the stage only. + log::error!("[tinymemory:module] create memory store failed: {error}"); + setup_error("create memory store") + })?; let provider = tinymemory_tinycortex::provider(Arc::from(memory)); service::serve(&connection, Arc::new(provider)).await } +/// Claim this process's single setup slot. +/// +/// `setup` installs a **process-global** embedding host +/// (`tinymemory_core::embedding_host::set_embedding_host`), so it is not +/// re-entrant the way a per-host resource would be. `ModuleHost` rejects a +/// duplicate module name only within one host, and nothing stops a process from +/// building a second host — a test harness is the obvious way it happens. The +/// second `setup` would replace the global embedder while stores built by the +/// first keep the `BusEmbeddingProvider` they captured, so embeds would be split +/// across two connections with no error anywhere. +/// +/// Refusing the second setup is the honest outcome: one process serves this +/// module once. tinybus never unloads a library, so there is no release path to +/// pair with this and no state to reset. +/// +/// # Errors +/// +/// [`SETUP_FAILED_ERROR`], when this process has already run setup. +fn claim_process_setup() -> BusResult<()> { + static CLAIMED: AtomicBool = AtomicBool::new(false); + + if CLAIMED.swap(true, Ordering::SeqCst) { + return Err(setup_error( + "this module is already set up in this process; it installs a \ + process-global embedding host and cannot be served twice", + )); + } + Ok(()) +} + /// A setup failure, carrying no path and no credential. fn setup_error(message: impl Into) -> BusError { BusError::MethodFailed { diff --git a/crates/tinymemory-module/src/service/mod.rs b/crates/tinymemory-module/src/service/mod.rs index dc14588..d8b2bcf 100644 --- a/crates/tinymemory-module/src/service/mod.rs +++ b/crates/tinymemory-module/src/service/mod.rs @@ -46,10 +46,24 @@ //! So there is no blob store, no chunking and no held output — the apparatus the //! `tinydocs` module needs does not appear in this one. //! -//! The one method that could grow without bound is `ExportPage`, and it is -//! already paged by contract with the caller choosing the page size. A caller -//! that asks for a million records in one page gets a frame-size error, which is -//! the correct answer. +//! Inline does not mean unbounded, though, and the three list-returning methods +//! are not all bounded the same way: +//! +//! - `ExportPage` is paged by contract, with the caller choosing the page size. +//! Asking for a million records in one page gets an error, correctly. +//! - `Recall` takes a `limit`, so the caller bounds the count — but not the +//! bytes, since fifty entries each holding a large document still overflow. +//! - `List` takes **neither**. It has no limit and no cursor, so entries can +//! accumulate across individually valid `Store` calls until the response +//! cannot cross a frame, and the caller has no way to ask for less. +//! +//! So `List` and `Recall` are checked against [`MAX_RESPONSE_BYTES`] and refuse +//! with a named `BudgetExceeded` rather than truncating. Truncating would be the +//! worse failure: with no cursor, a short list is indistinguishable from a +//! complete one, so a caller would conclude the missing entries do not exist. +//! `Namespaces` is left unchecked — it returns one small summary per namespace, +//! and a host with enough namespaces to fill 16 MiB of summaries has a different +//! problem. //! //! # Errors are named, and the names are the contract //! @@ -182,20 +196,28 @@ impl MemoryService { } /// List entries, narrowing by namespace, category and session. + /// + /// Bounded by [`MAX_RESPONSE_BYTES`]: unlike `Recall` and `ExportPage`, this + /// method takes no limit and no cursor, so the caller has no way to ask for + /// less. See [`ensure_response_fits`] for why the answer is a named refusal + /// rather than a truncation. async fn list( &self, namespace: Option, category: Option, session_id: Option, ) -> BusResult> { - self.provider + let entries = self + .provider .list( namespace.as_deref(), category.as_ref(), session_id.as_deref(), ) .await - .map_err(|error| into_bus_error(&error)) + .map_err(|error| into_bus_error(&error))?; + ensure_response_fits(&entries, "List")?; + Ok(entries) } /// Enumerate namespaces with their aggregate counts. @@ -219,10 +241,15 @@ impl MemoryService { opts: OwnedRecallOpts, scope: Option, ) -> BusResult> { - self.provider + let entries = self + .provider .recall(&query, limit, &opts, scope.as_ref()) .await - .map_err(|error| into_bus_error(&error)) + .map_err(|error| into_bus_error(&error))?; + // `limit` bounds the count but not the bytes: a caller asking for 50 + // entries that each hold a large document still overflows a frame. + ensure_response_fits(&entries, "Recall")?; + Ok(entries) } /// Read one page of the export, continuing from `cursor`. @@ -245,6 +272,69 @@ impl MemoryService { } } +/// The response-size ceiling for a method that returns a list of entries. +/// +/// A `TinyBus` frame is JSON capped at 16 MiB. 8 MiB of raw entry content leaves +/// room for the JSON structure around it and for escaping, which can double a +/// pathological string, so a response that passes this check fits with margin. +pub(crate) const MAX_RESPONSE_BYTES: usize = 8 * 1024 * 1024; + +/// Per-entry allowance for the fields that are not `content`. +/// +/// Keys, namespaces, timestamps, category and taint. Deliberately generous: this +/// check exists to stop a response overflowing a frame, and over-estimating +/// refuses slightly early while under-estimating fails at the transport with an +/// error the caller cannot act on. +const PER_ENTRY_OVERHEAD_BYTES: usize = 512; + +/// Refuse a response that would not fit in a frame. +/// +/// # Why a refusal and not a truncation +/// +/// Truncating would be worse than failing. `List` has no cursor, so a caller +/// receiving a short list has no way to tell it apart from a complete one and no +/// way to ask for the rest — it would conclude those entries do not exist. A +/// named error tells the caller to narrow by namespace, category or session, +/// which is a query it can actually issue. +/// +/// # Why `BudgetExceeded` and not a new name +/// +/// The name has to be one both ends already agree on, and +/// [`tinymemory_api::wire`] is the table that makes that true. `BudgetExceeded` +/// is what it means — the result exceeded a size budget — and it round-trips to +/// the host as `MemoryError::BudgetExceeded` with no client change. A new name +/// would decode to `Other` on any host older than the module, turning an +/// actionable "narrow your query" into an opaque backend failure. +/// +/// # Errors +/// +/// [`wire::BUDGET_EXCEEDED`], when the estimate exceeds [`MAX_RESPONSE_BYTES`]. +/// The message names the method and the sizes, never entry content. +fn ensure_response_fits(entries: &[MemoryEntry], method: &str) -> BusResult<()> { + let estimate: usize = entries + .iter() + .map(|entry| entry.content.len().saturating_add(PER_ENTRY_OVERHEAD_BYTES)) + .sum(); + + if estimate > MAX_RESPONSE_BYTES { + log::warn!( + "[tinymemory:module] {method} refused: {} entries estimated at {estimate} bytes \ + exceeds the {MAX_RESPONSE_BYTES} byte response ceiling", + entries.len() + ); + return Err(BusError::MethodFailed { + name: wire::BUDGET_EXCEEDED.to_string(), + message: format!( + "{method} would return {} entries (~{estimate} bytes), over the \ + {MAX_RESPONSE_BYTES} byte response ceiling; narrow the query by \ + namespace, category or session", + entries.len() + ), + }); + } + Ok(()) +} + /// Map a [`MemoryError`] onto a named bus error. /// /// Both the name and the message come from [`tinymemory_api::wire`], which the diff --git a/crates/tinymemory-module/src/service/test.rs b/crates/tinymemory-module/src/service/test.rs index 6f2da8e..573892e 100644 --- a/crates/tinymemory-module/src/service/test.rs +++ b/crates/tinymemory-module/src/service/test.rs @@ -1,2 +1,219 @@ -// Placeholder: the served surface is exercised through the loader E2E in -// `tests/module_e2e.rs`, which drives a real broker and a real module. +//! Tests for the service's error mapping. +//! +//! This is the module's half of the wire contract: every `MemoryError` the +//! engine can raise has to leave as a named bus error the host's client can map +//! back. `tinymemory_api::wire_tests` pins the table itself; what is tested here +//! is that the service actually goes through it. +//! +//! Covered here rather than in the loader E2E deliberately. An E2E can only +//! provoke the errors the engine happens to raise for a given input, which makes +//! it a test of engine internals this port does not own — an earlier revision +//! tried `ExportPage` with a zero limit, and since a driver accepting a zero +//! limit is equally legitimate, the test asserted nothing when it passed. Here +//! every variant is reachable by construction. + +use tinybus::Error as BusError; +use tinymemory_api::error::MemoryError; +use tinymemory_api::wire; + +use super::into_bus_error; + +/// The name and message a mapped error carries on the wire. +fn mapped(error: &MemoryError) -> (String, String) { + match into_bus_error(error) { + BusError::MethodFailed { name, message } => (name, message), + other => panic!("expected MethodFailed, got {other:?}"), + } +} + +#[test] +fn every_variant_leaves_under_its_contract_name() { + // Exhaustive by construction: `wire::wire_name` is a total match over + // `MemoryError`, so a new variant fails to compile there before it can + // silently leave this list. + let cases = [ + (MemoryError::NotFound("k".into()), wire::NOT_FOUND), + (MemoryError::Invalid("bad".into()), wire::INVALID), + ( + MemoryError::BudgetExceeded("too big".into()), + wire::BUDGET_EXCEEDED, + ), + ( + MemoryError::PathEscape("../outside".into()), + wire::PATH_ESCAPE, + ), + (MemoryError::unsupported_raw("tree"), wire::UNSUPPORTED), + ( + MemoryError::Other(anyhow::anyhow!("engine fell over")), + wire::OTHER, + ), + ]; + + for (error, expected) in &cases { + let (name, _) = mapped(error); + assert_eq!(&name, expected, "{error:?} left under the wrong name"); + } +} + +#[test] +fn a_path_escape_never_leaves_as_an_invalid() { + // The security-relevant collapse. `Invalid` tells a caller its input was + // malformed and invites a retry; a sandbox escape is not that, and the host + // re-raises whatever it receives to its own callers. + let (name, _) = mapped(&MemoryError::PathEscape("../../etc".into())); + assert_eq!(name, wire::PATH_ESCAPE); + assert_ne!(name, wire::INVALID); +} + +#[test] +fn a_miss_never_leaves_as_an_invalid() { + // `get`'s contract makes a miss `Ok(None)`, so a `NotFound` that arrived as + // `Invalid` would turn an ordinary absence into a caller-visible failure. + let (name, _) = mapped(&MemoryError::NotFound("absent".into())); + assert_eq!(name, wire::NOT_FOUND); + assert_ne!(name, wire::INVALID); +} + +#[test] +fn the_names_the_service_emits_are_the_ones_the_host_decodes() { + // The drift that matters is silent, so this closes the loop rather than + // trusting the two tables to agree: map out through the service, back + // through the client's decoder, and require the variant to survive. + let originals = [ + MemoryError::NotFound("k".into()), + MemoryError::Invalid("bad".into()), + MemoryError::BudgetExceeded("too big".into()), + MemoryError::PathEscape("../outside".into()), + MemoryError::unsupported_raw("tree"), + ]; + + for original in &originals { + let (name, message) = mapped(original); + let decoded = wire::from_wire(&name, &message); + assert_eq!( + std::mem::discriminant(&decoded), + std::mem::discriminant(original), + "{original:?} did not survive the round trip, arrived as {decoded:?}" + ); + } +} + +#[test] +fn a_message_carries_no_user_content_beyond_what_the_engine_put_there() { + // Not a redaction test — the engine owns its message. This pins that the + // service adds nothing of its own, so the only thing that can leak is what + // the engine already chose to say. + let error = MemoryError::NotFound("some-key".into()); + let (_, message) = mapped(&error); + assert_eq!(message, wire::wire_message(&error)); +} + +/// An entry whose content is `bytes` long. +fn entry_of(bytes: usize) -> tinymemory_api::types::MemoryEntry { + tinymemory_api::types::MemoryEntry { + id: "id".into(), + key: "key".into(), + content: "x".repeat(bytes), + namespace: Some("ns".into()), + category: tinymemory_api::types::MemoryCategory::Core, + timestamp: "2026-01-01T00:00:00Z".into(), + session_id: None, + score: None, + taint: tinymemory_api::types::MemoryTaint::Internal, + } +} + +#[test] +fn an_ordinary_list_response_is_not_refused() { + // The ceiling must not be so tight that normal use trips it. A hundred + // entries of a kilobyte each is an unremarkable namespace. + let entries: Vec<_> = (0..100).map(|_| entry_of(1024)).collect(); + assert!(super::ensure_response_fits(&entries, "List").is_ok()); +} + +#[test] +fn an_empty_list_response_is_not_refused() { + assert!(super::ensure_response_fits(&[], "List").is_ok()); +} + +#[test] +fn a_response_over_the_ceiling_is_refused_as_a_budget_error() { + // `List` takes no limit and no cursor, so entries accumulate across + // individually valid `Store` calls until the response cannot cross a + // 16 MiB frame. Without this check the caller gets a transport failure it + // cannot act on; with it, a named error that says how to narrow the query. + let entries: Vec<_> = (0..2) + .map(|_| entry_of(super::MAX_RESPONSE_BYTES)) + .collect(); + + let error = super::ensure_response_fits(&entries, "List") + .expect_err("a response over the ceiling must be refused"); + match error { + BusError::MethodFailed { name, message } => { + assert_eq!( + name, + wire::BUDGET_EXCEEDED, + "must use a name the host already decodes" + ); + assert!(message.contains("List"), "{message}"); + assert!( + message.contains("narrow"), + "the message must tell the caller what to do: {message}" + ); + } + other => panic!("expected MethodFailed, got {other:?}"), + } +} + +#[test] +fn the_refusal_decodes_host_side_as_a_budget_error() { + // The whole point of reusing an existing name: a new one would decode to + // `Other` on any host older than the module, turning an actionable "narrow + // your query" into an opaque backend failure. + let entries: Vec<_> = (0..2) + .map(|_| entry_of(super::MAX_RESPONSE_BYTES)) + .collect(); + + let BusError::MethodFailed { name, message } = + super::ensure_response_fits(&entries, "List").expect_err("refused") + else { + panic!("expected MethodFailed"); + }; + + let decoded = wire::from_wire(&name, &message); + assert!( + matches!(decoded, MemoryError::BudgetExceeded(_)), + "{decoded:?}" + ); +} + +#[test] +fn the_refusal_message_carries_no_entry_content() { + // Entry content is user memory. The message names sizes and the method, and + // nothing that was stored. + let secret = "correct-horse-battery-staple"; + let mut entries: Vec<_> = (0..2) + .map(|_| entry_of(super::MAX_RESPONSE_BYTES)) + .collect(); + entries[0].content.push_str(secret); + + let BusError::MethodFailed { message, .. } = + super::ensure_response_fits(&entries, "List").expect_err("refused") + else { + panic!("expected MethodFailed"); + }; + assert!(!message.contains(secret), "{message}"); +} + +#[test] +fn the_per_entry_overhead_is_counted_so_many_tiny_entries_still_trip_it() { + // A million empty entries carry no content at all but still cannot cross a + // frame — the JSON structure around each one is the payload. Counting only + // `content.len()` would let this through. + let count = super::MAX_RESPONSE_BYTES / super::PER_ENTRY_OVERHEAD_BYTES + 1; + let entries: Vec<_> = (0..count).map(|_| entry_of(0)).collect(); + assert!( + super::ensure_response_fits(&entries, "List").is_err(), + "entries with no content must still be counted" + ); +} diff --git a/crates/tinymemory-module/tests/module_e2e.rs b/crates/tinymemory-module/tests/module_e2e.rs index bdea3ad..c0d13f4 100644 --- a/crates/tinymemory-module/tests/module_e2e.rs +++ b/crates/tinymemory-module/tests/module_e2e.rs @@ -16,8 +16,12 @@ //! process. Run them one at a time: //! //! ```sh -//! cargo build --release -p tinymemory-module -//! TINYMEMORY_TEST_MODULE=target/release/libtinymemory_module.so \ +//! # Both paths are the module's own workspace, not the repo root: this crate is +//! # `exclude`d from the root workspace (see the root Cargo.toml comment), so +//! # `-p tinymemory-module` does not resolve there and the artifact is written +//! # under `crates/tinymemory-module/target`, not `./target`. +//! cargo build --release --manifest-path crates/tinymemory-module/Cargo.toml +//! TINYMEMORY_TEST_MODULE=$PWD/crates/tinymemory-module/target/release/libtinymemory_module.so \ //! cargo test --manifest-path crates/tinymemory-module/Cargo.toml \ //! --test module_e2e -- --ignored --exact //! ``` @@ -33,8 +37,6 @@ reason = "test code may panic, and the fake embedder derives a vector from a length" )] -use std::sync::Arc; - use tinybus::broker::Broker; use tinybus::module::ModuleHost; use tinybus::transport::memory::MemoryBus; @@ -90,16 +92,23 @@ impl HostEmbedder { } } -/// Load the module, serve the host embedder, and hand back a client connection. +/// Load the module, serve the host embedder, and hand back a client connection +/// together with the admitted `ModuleInfo`. /// /// The returned `ModuleHost` and broker task must be kept alive by the caller: /// dropping the host is what would release the module's transport. -async fn admit_module( +/// +/// The manifest is only observable through a real admission — it is produced by +/// the `cdylib`'s exported `tinybus_module_manifest_v1` and parsed by the host — +/// so a test that wants to inspect the declared surface has to go through here. +/// Most tests do not, and use [`admit_module`] instead. +async fn admit_module_detailed( workspace: &std::path::Path, ) -> ( Connection, ModuleHost, tokio::task::JoinHandle>, + tinybus::module::ModuleInfo, ) { let artifact = std::env::var_os("TINYMEMORY_TEST_MODULE") .expect("TINYMEMORY_TEST_MODULE must point at the built cdylib"); @@ -165,7 +174,19 @@ async fn admit_module( let client = Connection::connect(bus.connect().await.expect("client transport")) .await .expect("client connection"); - (client, modules, broker_task) + (client, modules, broker_task, loaded) +} + +/// Load the module under a fresh workspace and return a client connection to it. +async fn admit_module( + workspace: &std::path::Path, +) -> ( + Connection, + ModuleHost, + tokio::task::JoinHandle>, +) { + let (client, host, task, _info) = admit_module_detailed(workspace).await; + (client, host, task) } fn proxy(connection: &Connection) -> tinybus::Proxy { @@ -188,16 +209,22 @@ async fn the_module_advertises_exactly_the_mandatory_families() { // The adapter deliberately advertises only what it can reach. Advertising // more would make `audit_provider` fail host-side, and would register RPC // methods that answer errors. + // + // Asserted as an exact set rather than as "the mandatory three are present + // and `Tree` is absent": that weaker pair passes while any *other* optional + // family is advertised, which is the same overstatement with a different + // name on it. + assert_eq!( + capabilities, + Capabilities::mandatory(), + "the module must advertise exactly the mandatory families" + ); for mandatory in Capability::MANDATORY { assert!( capabilities.contains(mandatory), "{mandatory:?} must be advertised" ); } - assert!( - !capabilities.contains(Capability::Tree), - "the module must not claim an optional family it cannot serve" - ); let driver_id: String = proxy(&client).call("DriverId", ()).await.expect("DriverId"); assert_eq!(driver_id, "tinycortex"); @@ -365,22 +392,28 @@ async fn a_rejected_request_comes_back_under_its_contract_name() { let workspace = tempfile::tempdir().expect("tempdir"); let (client, _host, _task) = admit_module(workspace.path()).await; - // A zero limit is the clearest driver-rejected input that needs no store - // state to provoke. - let outcome: Result = proxy(&client) - .call("ExportPage", (Option::::None, 0_usize)) - .await; - - if let Err(error) = outcome { - let name = error.wire_name(); - assert!( - name.starts_with("ai.tinyhumans.tinymemory.Error."), - "a refusal must be named from the contract table, got {name}" - ); - } - // A driver that accepts a zero limit is also legitimate — `limit` is a - // request, not a guarantee — so this test asserts the *shape* of a refusal - // when there is one rather than demanding one. + // A method the module does not serve. This is the one refusal that is + // guaranteed regardless of engine behaviour, which is what makes it worth + // asserting here: it proves the served object rejects an unknown member + // rather than hanging or answering something. + // + // Note what this deliberately does *not* claim. The refusal comes from the + // bus's dispatch layer, so its name is tinybus's, not one from the contract + // table — asserting a `ai.tinyhumans.tinymemory.Error.*` name here would be + // asserting the wrong thing. The contract table's own mapping is covered + // exhaustively and deterministically in `service::test`, where every + // `MemoryError` variant is reachable by construction. An earlier revision + // tried to provoke a contract error through `ExportPage` with a zero limit; + // since a driver that accepts a zero limit is equally legitimate, that test + // asserted nothing whenever it passed. + let outcome: Result = proxy(&client).call("NoSuchMethod", ()).await; + + let error = outcome.expect_err("an unknown member must be refused"); + let name = error.wire_name(); + assert!( + !name.is_empty(), + "a refusal must carry a wire name, got {error:?}" + ); } #[tokio::test] @@ -438,16 +471,70 @@ async fn the_module_matches_the_in_process_engine_for_the_same_input() { /// Not `#[ignore]`d: it loads nothing and so is safe alongside the suite. #[test] -fn the_declared_method_list_matches_the_served_interface() { - // The manifest's `methods` list is admission surface. If it drifts from the - // interface's dispatch table, a host can be refused a method the module - // actually serves — or worse, admitted for one it does not. - let arc: Arc<()> = Arc::new(()); - drop(arc); - - // The service type is private, so this asserts the constant surface the - // manifest is written against instead. +fn the_routing_constants_are_the_ones_the_host_dials() { + // Only the constants. What the manifest actually declares is checked against + // a real admission in `the_manifest_declares_every_method_the_module_serves` + // below — these two used to be one test, and the constants alone cannot + // catch a method missing from the manifest. assert_eq!(BUS_NAME, "ai.tinyhumans.tinymemory.Memory"); assert_eq!(OBJECT_PATH, "/ai/tinyhumans/tinymemory/Memory"); assert_eq!(MEMORY_INTERFACE, BUS_NAME); } + +/// Every method the service dispatches, as the manifest must declare it. +/// +/// Kept beside the assertion rather than derived: the `module_export!` macro +/// takes string literals, so there is no constant for a test to share with it. +/// This list is therefore the second opinion — if the two disagree, one of them +/// is wrong and the test says which names differ. +const EXPECTED_METHODS: &[&str] = &[ + "DriverId", + "Capabilities", + "Health", + "Shutdown", + "Store", + "Get", + "Forget", + "List", + "Namespaces", + "Recall", + "ExportPage", + "ImportRecords", +]; + +#[tokio::test] +#[ignore = "drives a real dlopen'ed module; must be the only such test in the process — see the module docs"] +async fn the_manifest_declares_every_method_the_module_serves() { + // The manifest's `methods` list is admission surface: a host can be refused + // a method the module actually serves, or admitted for one it does not. + // + // This inspects the manifest the loaded artifact really exported, which is + // the only way to see it — the list is baked into `tinybus_module_manifest_v1` + // by the macro and parsed by the host during admission. Comparing routing + // constants, as an earlier revision did, passes with `ImportRecords` missing + // from the declaration entirely. + let workspace = tempfile::tempdir().expect("tempdir"); + let (_client, _host, _task, info) = admit_module_detailed(workspace.path()).await; + + let provided = info + .manifest + .provides + .iter() + .find(|interface| interface.version.interface.as_str() == MEMORY_INTERFACE) + .expect("the memory interface must be declared"); + + let declared: std::collections::BTreeSet<&str> = provided + .methods + .iter() + .map(tinybus::MemberName::as_str) + .collect(); + let expected: std::collections::BTreeSet<&str> = EXPECTED_METHODS.iter().copied().collect(); + + assert_eq!( + declared, + expected, + "manifest methods drifted; missing={:?} unexpected={:?}", + expected.difference(&declared).collect::>(), + declared.difference(&expected).collect::>() + ); +} diff --git a/docs/specs/tinybus-module.md b/docs/specs/tinybus-module.md index 68a3544..e648343 100644 --- a/docs/specs/tinybus-module.md +++ b/docs/specs/tinybus-module.md @@ -28,7 +28,7 @@ Everything else the engine uses (`reqwest`, `chrono`, `regex`, `uuid`, --timings` on the host shows a strictly serial chain, each link starting as the previous one ends: -``` +```text tinyagents 12.8 -> 25.4 (12.6s) tinycortex 25.4 -> 35.1 ( 9.7s) tinymemory-core 35.1 -> 40.1 ( 5.0s) @@ -85,16 +85,45 @@ Serving more would advertise capabilities whose accessors return nothing, which `audit_provider` exists to catch, and would make the host register RPC methods that answer errors. -### Everything travels inline +### Everything travels inline, but not unbounded A TinyBus frame is JSON capped at 16 MiB. For a generated document that is a real constraint — a byte array costs ~3.5 bytes per byte — and here it is not: memory entries are text, ~1.1× as JSON. So there is no blob store, no chunking and no held output. The tinydocs module's whole staging apparatus is absent. -`ExportPage` is the only unbounded method and is already paged by contract with -the caller choosing the size. Asking for a million records in one page gets a -frame-size error, which is the correct answer. +Inline is not the same as unbounded, though, and the three list-returning methods +are bounded differently: + +| Method | Caller can bound | Module bounds | +| --- | --- | --- | +| `ExportPage` | count, via `limit` + `cursor` | — paged by contract | +| `Recall` | count, via `limit` | bytes, via `MAX_RESPONSE_BYTES` | +| `List` | **nothing** | bytes, via `MAX_RESPONSE_BYTES` | + +`List` is the one that needed a decision. It takes no limit and no cursor, so +entries accumulate across individually valid `Store` calls until the response +cannot cross a frame — and at that point a host cannot enumerate its own valid +stored data at all. `Recall`'s `limit` bounds the count but not the bytes: fifty +entries each holding a large document overflow just the same. + +Both are therefore checked against an 8 MiB ceiling on estimated content (plus a +512-byte per-entry allowance for the surrounding JSON, so a million empty entries +trip it too) and **refuse** with `BudgetExceeded`. + +Refusing rather than truncating is the load-bearing part. With no cursor, a +short list is indistinguishable from a complete one, so a silently truncated +`List` would have the caller conclude the missing entries do not exist — a wrong +answer presented as a right one. The named error instead says to narrow by +namespace, category or session, which is a query the caller can actually issue. + +`BudgetExceeded` is reused rather than a new name added, because +`tinymemory_api::wire` is what both ends agree on: a new name decodes to `Other` +on any host older than the module, turning an actionable "narrow your query" into +an opaque backend failure. + +`Namespaces` is left unchecked — one small summary per namespace, and a host with +enough namespaces to fill 16 MiB of them has a different problem. ## Errors