Skip to content

Keep the recall future Send: the keep predicate must be Sync - #211

Merged
M3gA-Mind merged 1 commit into
tinyhumansai:mainfrom
M3gA-Mind:fix/recall-future-send
Oct 6, 2026
Merged

M3gA-Mind merged 1 commit into
tinyhumansai:mainfrom
M3gA-Mind:fix/recall-future-send

Conversation

@M3gA-Mind

@M3gA-Mind M3gA-Mind commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Recall's future is Send again. #206 (1960218, "a resumed pre-turn leads with the thread in one pack") passes keep: &dyn Fn(&Hit) -> bool through recall's gathering and holds it across an .await. That makes every holistic_recall future !Send, so hosts that run recall on a multi-threaded runtime no longer compile.

openhuman cannot adopt v1.23.3 because of it. Bumping vendor/tinymemory to the v1.23.3 tag (8237ecb) gives:

error[E0277]: `dyn for<'a> Fn(&'a tinymemory_api::Hit) -> bool` cannot be shared between threads safely
note: required because it's used within this `async` fn body
   --> vendor/tinymemory/crates/tinymemory-tools/src/recall/gather.rs:328:51

at four openhuman call sites:

  • memory/lifecycle/hooks.rs:219;
  • agent/tinyagents/memory_summarizer.rs:81 and :86;
  • memory/schemas/handlers.rs:141.

This crate's own CI never needs the future to be Send, so it passed here.

Related issue

Regression from #206. A release (v1.23.4) is needed before openhuman can bump; it is prepared locally against v1.23.3 and waits on it.

API or behavior changes

None. The three changed signatures in recall/gather.rs are private:

  • keep: &dyn Fn(&Hit) -> bool becomes keep: &(dyn Fn(&Hit) -> bool + Sync);
  • every closure passed in already captures only Sync data.

Validation

Local, in a target dir of this worktree's own:

  • cargo fmt --all -- --check: ok
  • cargo clippy --all-targets --all-features -- -D warnings: ok
  • cargo build --all-targets --all-features: ok
  • cargo test --all-features: ok, 1131 passed
  • cargo test: ok, 443 passed. One run first hit the known, intermittent tinymemory-api each_fault_is_caught_by_its_check; this PR does not touch that crate, and the rerun was clean.
  • RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features: ok
  • cargo run -p tinymemory-integrations --example basic: ok
  • The CI "Refuse inline test code" script: ok
  • cargo llvm-cov … --fail-under-lines 80: ok, 94.20% lines
  • cargo hack --feature-powerset --depth 2 --workspace check --all-targets: ok
  • scripts/cortexdb-live.sh on a fresh local CortexDB v0.10.4: ok, 5 + 1 + 3 passed
  • In openhuman: with this same change applied to its vendor/tinymemory checkout (temporarily, then restored), these are all green:
    • cargo check --workspace;
    • cargo clippy --keep-going -p openhuman --tests -- -D warnings;
    • clippy and check on the app manifest;
    • memory:: (160 passed) and flows:: (564 passed).

Revert-check: without the fix, the new test fails to compile ("future cannot be sent between threads safely"); with it, it passes.

Tests

a_recall_future_can_cross_threads (recall/mod_tests.rs) asserts that a holistic_recall future is Send. Every reference recall holds across an .await, the keep predicate included, must then be Sync, so a regression fails to compile in this crate rather than in a host.

Documentation

None needed: no public API or documented behaviour changes.

Checklist

  • The change is focused on one logical change
  • No new #[allow(...)], #[ignore], or relaxed lints
  • No secrets, tokens, or .env contents in the diff or the description

Summary by CodeRabbit

  • Bug Fixes
    • Improved compatibility for recall operations that run in thread-safe or asynchronous contexts. Predicate callbacks used to filter recall results can now be shared safely, and fetch-section recall requests are verified to support sendable async workflows. Existing recall results and filtering behavior are unchanged.

tinyhumansai#206 (1960218) passes `keep: &dyn Fn(&Hit) -> bool` through recall's
gathering and holds it across an .await, so a holistic_recall future is
not Send. Hosts run recall on multi-threaded runtimes (a spawned
pre-turn, an async_trait method), and openhuman no longer compiles
against v1.23.3 (E0277, "cannot be shared between threads safely").

The three private signatures take `&(dyn Fn(&Hit) -> bool + Sync)`, and a
test asserts that a holistic_recall future is Send, so this fails to
compile in this crate rather than in a host.
@tinysweeper

tinysweeper Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Tiny Sweeper review

Tiny Sweeper reviewed this change across 6 lane(s) and found 0 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below.

State: Ready for maintainer review
Priority: none
Reviewed head: 4845ac139ef6
Updated: 1791321267 (Unix time)

Review snapshot

Change surface Files Review signal Count
Production 1 Active findings 0
Tests 1 Noted findings 0
Documentation 0 Resolved findings 0
Configuration 0 Pending checks/questions 0

Completeness: Complete
Test assessment: No supported feature-to-test mapping was available; this does not mean tests are absent or passed.

What changed

The review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below.

Features

None identified with supported citations.

Tests

No supported feature-to-test mapping was produced. Test execution is not inferred.

Findings

No active actionable findings.

Before merge

None.

Agent review details

critique

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: Reviewed 2 files; 0 findings. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._

security

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: Reviewed 2 files; 0 findings. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._

tests

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The change widens the `keep` predicate bound to `+ Sync` at three internal functions so the `recall` future stays `Send` on multi-threaded runtimes, and adds a compile-time test asserting the future is `Send`. The test is a real guard: the type-level assertion fails to compile if a non-`Sync` reference is held across an `.await`, so the behaviour it pins cannot regress silently. One gap remains — the future itself is never driven to completion, so a runtime panic in the body would go unnoticed — but the test as written is deterministic and network-free, satisfying the repo's test rules. Nothing found that should block the merge. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._

commits

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Lane summary: Nothing sensitive found in what this pull request commits.

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The diff does exactly what the description says: it adds `+ Sync` to the three private `keep` predicate signatures in `recall/gather.rs` and adds a compile-time `Send` regression test for `holistic_recall` in `recall/mod_tests.rs`. The title, summary, validation claims, and the "no public API changes" note all match the change; the test follows the repo's naming and placement conventions (`mod_tests.rs`, descriptive name, module-level doc comment on the test). The change looks sound and safe to merge. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._

e2e

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The change threads a `Sync`-bound `keep` predicate through the recall gathering functions so that the `holistic_recall` future can be `Send`, and adds a compile-time unit test asserting that. The change has no external surface — no route, command, flag, message or persisted format is altered, only an internal trait-bound tightening on a private parameter — so it does not need an end-to-end test, and no end-to-end workflow exists in the tree to run one. The behaviour change looks sound and safe to merge. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._
Evidence and run details
  • Models: gpt-5.6-luna, glm-5.3-flash
  • Spend: $0.001026
  • Tokens: 84075 input · 4675 output · 5855 cached · 0 embedding
Head State Pass summary
4845ac139ef6 ready for maintainer review 0 active finding(s), 0 resolved finding(s) (at 1791321267)

tinysweeper 0.1.0

@coderabbitai

coderabbitai Bot commented Oct 6, 2026

Copy link
Copy Markdown

Review in Change Stack →

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 5bde194b-0ff3-4fdf-8d52-23afdad05f2b
📥 Commits

Reviewing files that changed from the base of the PR and between 8237ecb and 4845ac1.

📒 Files selected for processing (2)
  • crates/tinymemory-tools/src/recall/gather.rs
  • crates/tinymemory-tools/src/recall/mod_tests.rs
 ____________________________________________
< Double tap everything. Kill the bugs dead. >
 --------------------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tinysweeper found nothing blocking. Approving.

             $0.0010 · 84,075 in / 4,675 out · 5,855 cached (7%)  · gpt-5.6-luna, glm-5.3-flash
critique:    $0.0004 · 32,334 in / 1,323 out · 4,066 cached (13%) · gpt-5.6-luna
security:    $0.0004 · 34,497 in / 1,157 out · 1,789 cached (5%)  · gpt-5.6-luna
tests:       $0.0000 · 4,444 in  / 145 out   · 0 cached (0%)      · glm-5.3-flash
description: $0.0000 · 4,762 in  / 119 out   · 0 cached (0%)      · glm-5.3-flash
e2e:         $0.0000 · 5,011 in  / 117 out   · 0 cached (0%)      · glm-5.3-flash

@tinysweeper tinysweeper Bot added the priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect. label Oct 6, 2026
@M3gA-Mind
M3gA-Mind merged commit 023c6fb into tinyhumansai:main Oct 6, 2026
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant