Repository navigation
Conversation
|
Confirming this in production with Observed impact before the fix: manual Why it matters beyond latency: with
Validation of this PR's approach locally: applied the Preferring this over #4671: forwarding the flag alone still blocks the tool call on the HTTP round-trip and doesn't register the op for the prefetch visibility barrier. |
1ffd59d to
95d84dd
Compare
|
Rebased onto The initial generated-files failure was unrelated Oracle documentation drift from the base, already fixed upstream in Validation:
Thanks for the production validation above. The implementation still queues the HTTP acknowledgement off the tool-call path and tracks server-side operations for the existing prefetch barrier; GitHub reports this head cleanly mergeable. Ready for maintainer review. |
|
Added a before/after backend integration run to the PR description. This used the actual Hermes provider registry, real Hindsight client over loopback HTTP, a separately launched API/worker, and real pg0 PostgreSQL, not FakeClient or stubbed Hermes modules. With a controlled HTTP hold followed by an extraction hold, the base ( Scope matters: the model/embedding/reranker services were the repository's deterministic HTTP test providers, and the latency numbers are controlled-delay observations, not a production benchmark. The visibility probe explicitly used |
|
Coordination note after the September 30 stacking update on #4671: I checked its current diff. The manual-retain portion now forwards the configured #4670 overlaps on flag forwarding, but additionally:
The before/after real API + PostgreSQL verification is in the PR description and runtime comment, including its deterministic-model and raw-fact-recall limits. This is not a claim that the default observation-only recall filter waits for subsequent consolidation. These overlapping manual-retain hunks should be reconciled rather than merged twice unchanged. If the smaller flag-forwarding change is preferred first, the writer-queue/receipt-tracking portion remains a separate follow-up; flag forwarding alone does not cover those two boundaries. Please retain those distinctions when deciding whether this PR is superseded. |
|
Follow-up real-model evidence for
The async response is an enqueue acknowledgement, not a storage-completion claim. Its queue drained in 43.55 ms with the server receipt still pending; prefetch waited for a completed receipt before recalling the newly stored fact. Retain plus consolidation settled after 24728.31 ms in that case. All four cases independently listed and recalled the fixture's TCP port 48173 as a world fact, and recalled it as an observation after consolidation. Immediate observation-only visibility is not promised by the retain barrier. These are one-shot behavioral checks, not a quality/performance benchmark; neural reranking, a full interactive agent turn and restart durability remain outside scope. The PR description now separates this live-model evidence from the September 27 controlled-delay test. No production code changes were needed. API/PostgreSQL processes and temporary test data were cleaned up; credentials and workspace endpoint are omitted. |
|
Thanks for this! We're handling this fix in another change, so I'm closing this one. We've also stopped accepting pull requests from outside the team. If you run into anything else, please open an issue with steps to reproduce (see CONTRIBUTING.md). |
Summary
With the default
retain_async=true, an explicit Hermeshindsight_retaincall still runs inline and omits the client'sretain_asyncargument. A busy backend can therefore block the agent until the retain timeout, even after accepting the memory.Route asynchronous manual retains through the existing single-writer queue, passing the configured flag at call level and tracking returned operation IDs for the next prefetch's visibility barrier. Capture the item and destination before enqueueing. The tool now reports "Memory queued for storage.";
retain_async=falsestill waits and returns backend errors directly.This carries forward my NousResearch/hermes-agent#61512 after the bundled provider moved here. Addresses the
_tool_retainasync entry in #4662 and NousResearch/hermes-agent#61442; #4662 contains other independent fixes and should stay open. The operation tracking and acknowledgement distinction also incorporate feedback from @russellbrenner on the original PR.Validation
ruff.tomlpass for the Hermes integration;git diff --checkpasses.Actual backend verification (2026-09-27)
Ran a separate native
hindsight-apiprocess and real pg0 PostgreSQL, using the API and client built from this checkout. Loaded the external plugin through the actual Hermes memory-provider registry from Hermesbabdfff62940150e55a93273f426f3d3ddddd651; no fake Hermes modules or FakeClient. This targeted integration used an isolated Python 3.11.15 environment, not a full Hermes CLI installation.The provider talked over loopback HTTP through a forwarding proxy to the real API. The proxy held the retain request for one second; the upstream system-test model service then held fact extraction for one second. These controlled holds verify blocking behavior, not production throughput.
asyncccfe85b485ba22b0f9fcFor the patched async case, the real server receipt was tracked after the local queue drained; background prefetch stayed blocked and sent no recall request while extraction was held. After release, prefetch returned the stored fact, the operation tracking set drained, and an independent client both listed and recalled the persisted memory. All four cases stored and recalled their expected fact. API, PostgreSQL and temporary banks/homes were cleaned up.
Scope of the September 27 run: LLM, embeddings and reranking used the repository's deterministic system-test provider over HTTP. No real model API key was available at that time, so that run does not validate model quality, production latency, cloud service behavior or long-term durability across database restarts. The read-after-write probe explicitly configured
recall_types=["world"]; it does not promise immediate visibility of separately consolidated observations under the default observation-only recall filter. No production implementation change was needed after this verification.Live model verification (2026-09-30)
Repeated the before/after test with real Bailian
qwen3.7-plusextraction/consolidation andqwen3.7-text-embedding(1024 dimensions). This used the actual Hermes registry/provider, Hindsight client, native API/worker and pg0 PostgreSQL, with the same pinned plugin revisions above. No synthetic model responses or artificial HTTP/model delays. Neural reranking was disabled via the productionrrfpassthrough provider.All four final cases passed. The fixture says that CedarLamp's staging service uses TCP port 48173. An independent client listed the stored facts, recalled that port with
types=["world"], and later recalled it withtypes=["observation"]after consolidation settled.For the patched async case, the queue drained in 43.55 ms and the real server operation was still
pending. The wire trace confirmed that prefetch issued recall only after observing acompletedreceipt; it returned the newly extracted port fact. The tool's 1.55 ms response means queued, not persisted: retain plus consolidation settled after 24728.31 ms in that run. The sync path still waited for actual retain completion.Limits: single-fixture, single-run behavior checks, not a model-quality or latency benchmark. Immediate prefetch uses world facts; observation recall is tested after consolidation, not guaranteed by the retain receipt barrier. Neural reranking, a full interactive Hermes agent turn and restart durability remain untested. Only the standalone verification harness needed setup corrections (worker slot reservation and the client's
fact_typefield); production code did not change. Test processes and temporary banks/homes were cleaned up. No credentials or workspace endpoint are included here.After merge, the Hermes catalog SHA needs updating to distribute this fix.