fix(memory): Qdrant memory async/runtime — recall re-entry, async drop, honest gated tests (#348/#349/#358) - #370
Open
gnanirahulnutakki wants to merge 1 commit into
Open
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…p, honest gated tests (#348/#349/#358) The durable/hybrid Qdrant memory backend owns a nested multi-thread Tokio runtime and bridges the synchronous MemoryRuntime trait onto it via block_on. Three defects stemmed from that bridge: - #348 (High): hybrid recall panicked the turn. The sync trait `search_scoped` did `qdrant.block_on(search_for_subject)` (level-1), and inside that awaited future `search_filtered` called the *sync* `dead_chains` / `search_vectors` / `fetch_record`, each re-bridging via `block_on` (level-2). At level-2 the ambient runtime IS the owned multi-thread `rt`, so the flavor-only guard took `block_in_place(|| rt.block_on(..))` and re-entered the same runtime → "Cannot start a runtime from within a runtime". The write path had the same latent nesting. - #349 (High): `rt: tokio::runtime::Runtime` was owned inline with no `Drop`. Dropping the Arc from an async context (server shutdown, end of a turn) panicked: "Cannot drop a runtime in a context where blocking is not allowed". - #358 (Medium): the gated Qdrant tests returned early and counted as `passed` (not `ignored`) when the env gate was unset, so a green suite masked the durable-path panic. Fixes: - runtime.rs: give every inherent op an async core (record_async, search_vectors_async, search_text_async, fetch_record_async, dead_chains, scroll_records_async, get_record_async); each sync method is now a thin single `block_on(..async..)` wrapper, so one logical call bridges exactly once and never nests. hybrid.rs awaits those cores directly. - block_on_runtime: compare runtime *identity* (Handle::id), not just flavor — if the ambient runtime is our own `rt`, drive on a dedicated scoped thread instead of re-entering. A backstop even though the refactor removes the nest. - Store `rt: Option<Runtime>` and add `impl Drop` calling `shutdown_background()` (returns immediately, safe from any context). - Every gated Qdrant test is `#[ignore]`d (reports `ignored`, never a masked `passed`) and actually runs under `--ignored` against a live Qdrant. A new ci.yml job boots qdrant/qdrant:v1.18.0 and runs the curated `--ignored` set. Verification: crate + e2e default suites green (Qdrant scenarios reported ignored); against a live Qdrant the previously-skipped persistence (#349 async drop) and hybrid-full-pipeline (#348 fused-runtime recall) scenarios pass, as do the crate insert/invalidate/restart and hybrid recall tests; new unit test block_on_runtime_survives_reentry_on_its_own_runtime reproduces #348 and passes. Checkpoint: architect/sessions/qdrant-memory-async-runtime-2026-07-22/journal.md Signed-off-by: GR <gnanirn@gmail.com>
gnanirahulnutakki
force-pushed
the
fix/qdrant-memory-async-runtime
branch
from
July 23, 2026 18:29
74dfce3 to
12af399
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #348, #349, #358 — three defects in the durable/hybrid Qdrant memory backend that share one root: it owns a nested multi-thread Tokio runtime and bridges the synchronous
MemoryRuntimetrait onto it viablock_on.Root causes (verified in-tree)
#348 (High) — hybrid recall panics the turn (nested
block_on).HybridMemoryRetriever::search_scoped(sync trait) didqdrant.block_on(search_for_subject)(level‑1). Inside that awaited future,search_filteredcalled the synchronousdead_chains/search_vectors/fetch_record, each of which re‑bridged viablock_on(level‑2). At level‑2,Handle::try_current()returns the same owned multi‑threadrt, so the flavor‑only guard (runtime_flavor() == MultiThread) tookblock_in_place(|| rt.block_on(..))and re‑entered the same runtime →Cannot start a runtime from within a runtime. The write path (record→ syncqdrant.record) had the identical latent nesting.#349 (High) — durable backend panics when dropped from async.
rt: tokio::runtime::Runtimewas owned inline with noDrop. Dropping theArcfrom an async context (server shutdown, or end of a turn) panics:Cannot drop a runtime in a context where blocking is not allowed.#358 (Medium) — gated tests report
passedwithout running.Both e2e scenarios (and the crate's own
integration.rs/hybrid_integration.rs) used areturn Noneenv‑gate with no#[ignore], so the default suite printed green while the durable/hybrid paths never ran — which is exactly why #349 was invisible in CI.Fix
runtime.rs— single‑bridge async cores. Each inherent op now has an async core (record_async,search_vectors_async,search_text_async,fetch_record_async,dead_chains(now async),scroll_records_async,get_record_async) holding the real logic; every sync method is a thinblock_on(..async..)wrapper over one core. A single logical call therefore bridges exactly once and never nests.hybrid.rsawaits the async cores directly (search_filtered,record,fetch_live).block_on_runtimenow compares runtime identity (Handle::id()), not just flavor: if the ambient runtime is our ownrt, it drives on a dedicated scoped thread instead of re‑entering. Defense‑in‑depth even though the refactor removes the nesting.Drop([High] Qdrant durable-memory backend panics when dropped from an async context #349).rt: Option<Runtime>+impl Drop→shutdown_background(), which signals the workers and returns immediately, so the drop is safe from any context.#[ignore]d — it reports asignored, never a maskedpassed— and actually executes under--ignoredagainst a live Qdrant. A newci.ymljob (qdrant-integration) bootsqdrant/qdrant:v1.18.0as a service and runs the curated--ignoredset.Public API is additive only (new
*_asyncmethods; no sync signature changed) —ardur-serverandardur-admincompile unchanged.Verification
Default suites (no Qdrant):
ardur-memory-qdrantlib+config green;integration/hybrid_integrationnow reportignored(were silentlypassed);cargo test -p ardur-e2e-testsgreen with both Qdrant scenarios reportedignored.clippy -D warnings+fmtclean.Against a live Qdrant (
qdrant/qdrant:v1.18.0, gRPC6334):scenario_qdrant_memory_persistencescenario_hybrid_memory_full_pipelineintegration::{insert_then_query, invalidate_preserves_history_and_past, survives_simulated_restart}hybrid_integration::{record_writes_to_both, search_respects_top_k, recall_excludes_invalidated_memory}runtime::tests::block_on_runtime_survives_reentry_on_its_own_runtime(unit)Out of scope / follow‑up
snapshot_into_receipt_records_eventfails against live Qdrant: qdrant-client's default 5s request deadline is too short for gRPC snapshot creation (REST snapshot is ~1s). This is a separate, pre‑existing snapshot‑timeout bug unrelated to #348/#349 — excluded from the CI sweep via--skipand left for a dedicated follow‑up. The two*semantic*hybrid tests additionally need the BGE model download and are--skipped in CI (documented to run by name locally).Do not merge — leaving for peer review.