Skip to content

fix(hermes): bind retain work to enqueue-time session identity - #4673

Closed
yingliang-zhang wants to merge 10 commits into
vectorize-io:mainfrom
yingliang-zhang:fix/hindsight-retain-session-identity
Closed

yingliang-zhang wants to merge 10 commits into
vectorize-io:mainfrom
yingliang-zhang:fix/hindsight-retain-session-identity

Conversation

@yingliang-zhang

@yingliang-zhang yingliang-zhang commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Problem

_make_turn_retain_job snapshotted the turn payload at enqueue time but still built the aretain_batch item at run time via _build_retain_kwargs(), which reads mutable per-session attributes:

def _job() -> None:
    item = self._build_retain_kwargs(content, ...)   # reads self._retain_tags,
                                                     #      self._observation_scopes,
                                                     #      self._retain_source, _METADATA_ATTRS …

When a session switch lands between enqueue and the writer drain (easy with retain_every_n_turns > 1 or any queue backlog), an OLD-session retain ships with the NEW session's tags / observation scopes / source. The snapshotted metadata session_id then contradicts the lineage tag session:OLD — a self-inconsistent document in the memory bank.

Fix

Build the complete item at enqueue time. The writer job only ships the frozen item:

item = self._build_retain_kwargs(..., tags=..., update_mode=update_mode)
def _job() -> None:
    resp = self._retain_batch(item, ...)

No new surface, no new state: _build_retain_kwargs / _retain_batch are unchanged; only the when moved.

Tests

tests/test_retain_identity_isolation.py — test_queued_retain_keeps_enqueue_time_item_config (port of the original TestRetainIdentityIsolation regression): gates the writer between dequeue and execution, mutates _retain_tags / _observation_scopes / _retain_source mid-flight (exactly what on_session_switch() does to a queued retain), then asserts the shipped item keeps the enqueue-time values. Fails on main, passes at this head.

Verification

Pre-fix (test only, main's _make_turn_retain_job) — RED:

  Full diff:
    [
  -     'old-tag',
  ?      ^^^
  +     'new-tag',
  ?      ^^^
        'session:session-1',
    ]
============================== 1 failed in 0.06s ===============================

Post-fix, uv run pytest tests/test_retain_identity_isolation.py -v (only the touched test file):

collecting ... collected 1 item

tests/test_retain_identity_isolation.py::test_queued_retain_keeps_enqueue_time_item_config PASSED [100%]

============================== 1 passed in 0.04s ===============================

uv run pytest tests/test_provider.py (existing suite exercising the touched path — sync_turn, flush-on-switch, append/overwrite — unchanged): 15 passed in 0.08s.

Provenance

Ported from NousResearch/hermes-agent#64499 (closed in the hindsight-move close pass; the bundled provider moved to hindsight-integrations/hermes in NousResearch/hermes-agent#119888). Original fix, audit trail and regression test by @yingliang-zhang; the mechanism was confirmed by @teknium1's automated review there ("A queued old-session retain can therefore be assembled with new-session identity/configuration"). Tracked in #4662.

Stacked PR (pre-chained 2026-09-30): the diff vs main is cumulative with #4674, #4671, #4672; this PR's own change is snapshotting the retain item at enqueue time so retains carry enqueue-time session identity. Merge top-down: 4674 -> 4671 -> 4672 -> 4673 -> 4675 -> 4676. Identical shared commits merge cleanly in any order (git dedups same-patch-both-sides).

_client is read/written from the retain writer, the prefetch worker and
the turn/tool thread with no lock. _get_client() was check-then-act and
the embedded constructor takes seconds: two threads hitting a cold or
just-nulled client both construct, and the loser's client is orphaned
with an aiohttp session nothing ever closes ("Unclosed client session"
noise). The stale-daemon retry's inline null-and-rebuild widened the
window to seconds and concurrent retries clobbered each other's
replacement.

- _client_lock, a LEAF lock: _get_client() takes it only on the
  construction/retire path; the fast path stays lock-free; never held
  across _run_sync/operation(client) or while taking _prefetch_lock /
  _pending_retain_ops_lock.
- _get_client(*, retire=<client>): the retry retires the EXACT client
  the operation ran with — identity passed as an argument, never shared
  broken-client state a concurrent retry could clobber — and rebuilds
  exactly once under the lock (identity CAS: a sibling's fresh rebuild
  is returned as-is instead of being orphaned).
- shutdown() retires the client under the lock BEFORE closing it
  (_close_client_of(client), parameterised); a concurrent _get_client()
  rebuilds instead of racing the close.

Ported from NousResearch/hermes-agent#117236 (closed in the hindsight-move
close pass). Tracked in vectorize-io#4662.
@yingliang-zhang

Copy link
Copy Markdown
Contributor Author

CI note: verify-generated-files fails here for a pre-existing reason — ./scripts/generate-docs-skill.sh on a clean upstream/main (7a593e8) reproduces the same skills/hindsight-docs/references/developer/oracle.md drift, so the gate is red on every PR including unmodified main. Fixed by #4678 (single-file regeneration); once it merges, this PR's gate goes green with no changes needed here.

@yingliang-zhang
yingliang-zhang force-pushed the fix/hindsight-retain-session-identity branch from 0a0f1cf to 2f874d4 Compare September 23, 2026 14:22
@yingliang-zhang

Copy link
Copy Markdown
Contributor Author

Generated-files sync commit added atop the substantive commit(s): the verify-generated-files gate runs its generators against the PR head (actions/checkout at ref: head.sha), so this branch now carries its own regenerated skills/hindsight-docs/references/developer/oracle.md plus the lint.sh ruff formatting, committed as chore: sync generated files for verify-generated-files. No substantive changes — the review surface is exactly as before.

Conflict resolution (both intents kept): hindsight-integrations/hermes/__init__.py merges the leaf-lock client lifecycle (_client_lock, _get_client(retire=...), _close_client_of(client), shutdown retire-then-close) with main's daemon-based embedded client (_embedded_url, HindsightEmbedded removal). _close_client_of keeps the PR's parameterized identity with main's simplified single-aclose body.

skills/hindsight-docs/references/developer/oracle.md auto-merged to main's regenerated blob (a8d62fc): the PR's vectorize-io#4678 mirror is subsumed by main's generate-docs-skill.sh output from the same source (byte-verified). Checks: 37 hermes tests passed incl. test_client_lifecycle.py; ruff check+format clean per scripts/hooks/lint.sh's hermes path.
The hindsight_retain tool handler called _retain_batch without forwarding
the configured self._retain_async, so tool retains silently dropped the
async/sync choice and aretain_batch fell back to its own server default.
retain_async is a call-level arg (never an item key), so the tool handler
must pass it explicitly. Forward the configured mode, matching the
auto-retain path.

Ported from NousResearch/hermes-agent#60648 (closed in the hindsight-move
close pass). Tracked in vectorize-io#4662.
Ported from NousResearch/hermes-agent#64745 (closed in the hindsight-move
close pass). Tracked in vectorize-io#4662.

The background prefetch worker published its recall into the session slot
unconditionally; a worker outliving on_session_switch's 3s join wrote the
old session's memories into the new session's slot. queue_prefetch also
spawned unbounded threads with the last finisher winning the slot. Workers
now capture a slot generation at spawn, queue_prefetch bumps it and skips
while a prior worker runs, on_session_switch/shutdown bump it to fence late
publishers, and the publish + recall are gated on the current generation.
_make_turn_retain_job snapshotted turns/metadata/lineage/bank_id at
enqueue but still built the aretain_batch item at run time via
_build_retain_kwargs, which re-reads mutable per-session attributes
(_retain_tags, _observation_scopes, _retain_source). A session switch
landing between enqueue and writer drain stamped an OLD-session retain
with the NEW session's tags/scopes/source — a document whose lineage tags
contradicted its own snapshotted metadata session_id.

Build the whole item at enqueue time; the writer job only ships it.

Ported from NousResearch/hermes-agent#64499 (closed in the hindsight-move
close pass). Tracked in vectorize-io#4662.
@nicoloboschi

Copy link
Copy Markdown
Collaborator

Thanks for this! We no longer merge pull requests from outside the team (see CONTRIBUTING.md), so I'm closing this one.

If the problem still affects you, please open an issue with steps to reproduce and we'll take it from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants