fix(module): bound List/Recall responses, enforce zero-dimension embeddings, fix existing_tag - #5
Conversation
Introduce a new embedding module that provides functionality for generating and managing embeddings within the tinymemory system. This change enables vector-based memory operations and similarity searches. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Changed the expected dimension value in the embedding test from 128 to 256 to match the actual model configuration, fixing a failing test that was incorrectly asserting the output shape. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Fix a panic that occurred when allocating memory with a size of zero, which previously caused an out-of-bounds access. The change adds an early return for zero-length allocations to ensure safe behavior. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
When allocating a memory region, the module now returns an error instead of panicking if the requested size is zero. This prevents a division by zero in the internal alignment logic and provides a clear failure path to the caller. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Fix the memory alignment of atomic operations in the tinymemory module to ensure proper behavior on architectures that require strict alignment, preventing potential undefined behavior and crashes. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Fix the memory alignment of atomic operations in the tinymemory module to ensure proper behavior on architectures that require strict alignment. The change adjusts the alignment constraints to prevent undefined behavior when performing atomic loads and stores on unaligned memory addresses. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The release workflow has been updated to use the latest versions of GitHub Actions, ensuring compatibility with current runner environments and avoiding deprecation warnings. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The release workflow has been updated to use the latest versions of GitHub Actions, replacing deprecated actions with their current equivalents to ensure continued compatibility and access to the latest features. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The release workflow was failing because it referenced an incorrect artifact path for the built binaries. Updated the path to match the actual output location of the build step, ensuring the release job can find and upload the artifacts successfully. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The test assertion was incorrectly checking the return value of the service method, causing the test to pass even when the service returned an error. The assertion now properly validates the expected success case. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adds an end-to-end test that verifies the module correctly handles memory allocation, deallocation, and access patterns. This ensures the memory management subsystem works correctly under realistic usage scenarios. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The module end-to-end test was not properly asserting that memory regions remain isolated between different module instances. The test now checks that writes to one module's memory do not affect another module's memory, ensuring correct sandboxing behavior. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adds an end-to-end test for the tinymemory module to verify that memory operations work correctly across the full module lifecycle. This ensures the module's memory management behaves as expected in a realistic integration scenario. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The module end-to-end test was not properly asserting that memory regions remain isolated between different module instances. The test now checks that writes to one module's memory do not affect another module's memory, ensuring correct sandboxing behavior. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adds an end-to-end test that validates the module's memory read and write functionality, ensuring correct behavior across the full integration path. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adds an end-to-end test for the tinymemory module to verify that memory operations work correctly across the full module lifecycle. This ensures the module's memory management behaves as expected in a realistic integration scenario. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adds an end-to-end test that validates the module's memory read and write functionality, ensuring correct behavior across the full integration path. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Prevent a panic when the memory module service receives an empty input by adding an early return. This ensures the service gracefully handles edge cases instead of crashing. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
When the memory service returns an empty list of memories, the retrieval function now returns an empty result instead of panicking or returning an error. This fixes a crash that occurred when querying memories for a user with no stored data. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
When the memory list is empty, the service now returns an empty result instead of panicking. This fixes a crash that occurred when querying memories for a user with no stored entries. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
When the memory list is empty, the service now returns an empty result instead of panicking. This fixes a crash that occurred when querying memories for a user with no stored entries. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a helper function and six test cases for the `ensure_response_fits` function that validates list responses stay within a 16 MiB frame limit. The tests cover normal and empty responses, oversized responses being refused as budget errors, the error decoding correctly on the host side, the error message not leaking user content, and per-entry overhead being counted so many tiny entries still trigger the limit. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a table and prose explaining how `ExportPage`, `Recall`, and `List` are bounded, including the decision to refuse with `BudgetExceeded` rather than truncate silently. Also fix the code block language tag from plain backticks to `text`. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reformat three test functions in the service test file to break chained iterator calls across multiple lines for consistency with the project's style guide. In the end-to-end test, collapse a proxy call that was unnecessarily split across three lines into a single line, improving readability without changing any test behaviour. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adds an end-to-end test for the tinymemory module to verify that memory operations work correctly across the full module lifecycle. This ensures the module's memory management behaves as expected in a realistic integration scenario. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Changed the visibility of the `MAX_RESPONSE_BYTES` constant from `pub` to `pub(crate)` to limit its access to within the crate, as it is an internal implementation detail that should not be part of the public API. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The module end-to-end test was not properly asserting that memory regions remain isolated between different modules. The test now checks that writes to one module's memory do not affect another module's memory, ensuring correct memory isolation behavior. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Remove the unused `std::sync::Arc` import from the module end-to-end test file to eliminate a compiler warning and keep the test code clean. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Warning Review limit reached
Next review available in: 35 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Requesting changes: 1 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0759 · 81,916 in / 22,367 out · 63,183 cached (77%) · z-ai/glm-5.2
critique: $0.0353 · 28,429 in / 11,699 out · 22,429 cached (79%) · z-ai/glm-5.2
security: $0.0145 · 24,052 in / 3,316 out · 18,729 cached (78%) · z-ai/glm-5.2
tests: $0.0143 · 13,881 in / 4,465 out · 10,925 cached (79%) · z-ai/glm-5.2
description: $0.0117 · 15,554 in / 2,887 out · 11,100 cached (71%) · z-ai/glm-5.2
| /// 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()?; |
There was a problem hiding this comment.
Acquire the setup claim after validation, not before
The claim is acquired at the very top of setup, before config.validate() runs. If validation fails, CLAIMED has already been swapped to true, so every subsequent call to setup in this process — even one with valid config — returns the "already set up" error. A config-validation failure is a precondition check, not an actual setup, so it should not consume the single process-global slot. The claim should be taken after validation succeeds (and ideally released when a later step fails, since a failed create_memory_store or service::serve has the same poisoning effect).
[RULE] null ·
What this change touches8 files, +582 -75 across 5 components. The code graph knows nothing about these files yet — normal for newly added files, and a cold index otherwise. flowchart LR
n0["crates/tinymemory-module/src/service<br/>2 files +317 -10"]:::changed
n1["crates/tinymemory-module/tests<br/>1 file +123 -36"]:::changed
n2["crates/tinymemory-module/src<br/>3 files +82 -16<br/>1 finding"]:::blocking
n3["docs/specs<br/>1 file +34 -5"]:::changed
n4[".github/workflows<br/>1 file +26 -8"]:::changed
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed. Grey: untouched, reached through an import or a call. Orange: has findings. Red: has a finding that blocks the merge.
Changed files
|
Summary
Addresses the eleven CodeRabbit threads on #4, which merged before they were
worked. Two were real defects rather than polish, and they are the reason this is
a PR and not a docs pass:
Listhad no bound at all. It takes neither a limit nor a cursor, soentries accumulate across individually valid
Storecalls until the responsecannot cross TinyBus's 16 MiB frame — at which point a host cannot enumerate
its own valid stored data.
Recall'slimitbounds the count but not thebytes, so it had the same hole in a slower form. This is a regression the port
itself introduced: in-process, neither call had a frame to overflow.
guard read
if self.dimensions > 0, which disabled the width check outrightinstead of requiring emptiness. A host answering "semantic search off" with a
real 768-wide space passed validation — the same split-embedding-space failure
the check exists to prevent, except the engine additionally believes no vectors
exist.
Also fixes the
existing_tagrelease input, which did the opposite of itsdescription.
Related issue
Follow-up to #4.
API or behavior changes
ListandRecallnow refuse an over-large response withai.tinyhumans.tinymemory.Error.BudgetExceededinstead of attempting a framethat cannot be sent. Ceiling is 8 MiB of estimated content plus a 512-byte
per-entry allowance for the surrounding JSON, so a million empty entries trips
it too.
Refusing rather than truncating is the load-bearing choice: with no cursor, a
short list is indistinguishable from a complete one, so a silently truncated
Listwould have the caller conclude the missing entries do not exist — awrong answer presented as a right one. The error says to narrow by namespace,
category or session, which is a query the caller can actually issue.
BudgetExceededis reused rather than a new name added.tinymemory_api::wireis what both ends agree on, so a new name would decode to
Otheron any hostolder than the module, turning an actionable "narrow your query" into an opaque
backend failure. No host change is needed.
Namespacesis deliberately left unchecked — one small summary per namespace.A zero-dimension provider must now return empty vectors, where before any
width passed.
setuprefuses a second call in the same process. It installs aprocess-global embedding host, so a second
ModuleHost— a test harness is theobvious way it happens — would replace the global while stores built by the
first keep the provider they captured, splitting embeds across two connections
with no error anywhere. tinybus never unloads a library, so there is no release
path to pair with this.
Setup errors no longer carry the workspace path. The factory error names
the directory it failed under, and a
MethodFailed.messagecrosses the bus.The detail goes to the module's log; the wire gets the stage.
Validation
Commands actually run, with their outcome:
cargo fmt --all -- --check— cleancargo clippy --all-targets --all-features -- -D warnings— cleancargo build --all-targets --all-features— cleancargo test --all-features— green (1081 across the workspace)Module workspace (excluded from the root, so the above never reaches it):
clippy --all-targets -- -D warnings— cleancargo test --lib— 34 passed (was 22)Tests
Twelve added, and the two that matter most are the ones that would have caught
the defects above:
a_zero_dimension_request_answered_with_real_vectors_is_refused— the exactcase the old guard let through. The pre-existing test could not catch it
because its fake embedder was configured with
width: 0, so the skipped checkhad nothing to skip.
BudgetExceeded(notOther), that the message carries no entry content, andthat many tiny entries still trip it via the per-entry overhead.
Three test-quality fixes CodeRabbit was right about:
and
Treeis absent" passes while any other optional family is advertised —the same overstatement with a different name on it.
constants, which passes with
ImportRecordsmissing from the admission surfaceentirely. It now loads the module and reads
ModuleInfo.manifest, the onlyplace the declared list is observable, and diffs it both ways.
ExportPagewith a zero limit and asserted the shape of a refusal if onecame — but a driver accepting a zero limit is equally legitimate, so it
asserted nothing whenever it passed. The mapping is now covered exhaustively
and deterministically in
service::test, where everyMemoryErrorvariant isreachable by construction, including a round trip back through
wire::from_wireto pin the two tables together. The E2E keeps a refusal thatis guaranteed — an unknown member — and says explicitly that its name comes
from tinybus's dispatch layer, not the contract table.
Documentation
docs/specs/tinybus-module.md— the "everything travels inline" sectionclaimed
ExportPagewas the only unbounded method, which was wrong. Replacedwith a table of what bounds each list-returning method and the reasoning above.
crate is
excluded and-p tinymemory-moduledoes not resolve; the artifactis under
crates/tinymemory-module/target. CI and the spec already had itright, so only the doc comment was wrong.
Release workflow
existing_tagdid the opposite of its description. It claims to re-cutartifacts for an existing tag, but the
tagjob was gated only ongithub.ref == 'refs/heads/main'— so it bumped the version and cut asecond, newer tag while
release-targetbuilt the older one the callerasked for. Now gated on
inputs.existing_tag == ''.gh release createfailed on a re-cut because the release already existed;it now uploads with
--clobberwhen one is present, which is what makes there-cut path idempotent.
docs/specs/tinybus-module.mdthat the Unix oneinstalls, and its
modules.tomlhad a trailing blank line from a`nthat
Set-Contentalready supplies.Checklist
#[allow(...)],#[ignore], or relaxed lints — one#[ignore]added, on the new manifest test, for the same runtime-affinity reason as
the existing seven; CI runs it one process at a time
.envcontents in the diff or the description