fix(hindsight): bind retain work to enqueue-time session identity

[yingliang-zhang]

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 / metadata attrs. 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.

Verification

• tests/plugins/memory/test_hindsight_provider.py: 79 passed (+1 new), 1 skipped
• New TestRetainIdentityIsolation::test_queued_retain_keeps_enqueue_time_item_config gates the writer between dequeue and execution, mutates _retain_tags / _observation_scopes / _retain_source mid-flight, asserts the shipped item keeps enqueue-time values. Fails on main, passes at this head.
• 8 pre-existing failures on the untouched base remain unchanged (hindsight-client cannot be pip-installed in the sandbox — ImportError in lazy_deps); verified by re-running the suite on the base commit.

History

Originally authored against the pre-#102117 single-file plugin. Most of that work (enqueue-time snapshot of turns/metadata/lineage/bank_id, pre-enqueue _resolve_retain_target, flush-on-switch under OLD ids) was absorbed by the #102117 rewrite and this PR was closed on that basis — but the item-snapshot gap survived the rewrite, verified by git cherry upstream/main. Re-scoped to exactly that residual.

[teknium1]

Thanks for the focused retain-lifecycle work. The premise is confirmed on current main: plugins/memory/hindsight/__init__.py:1667-1687 defers _build_retain_kwargs() into the writer callback, while that helper reads mutable provider state (:1582-1601) and on_session_switch() later changes the active session state (:1890-1899). A queued old-session retain can therefore be assembled with new-session identity/configuration.

The PR addresses that mechanism directly: _AutomaticRetainBatch freezes the request data before queueing (291c04d24bae:1547-1603), and the FIFO drain deep-copies its private payload for each client operation while committing only the head batch (:1623-1675). The added tests exercise enqueue-time identity, retry isolation, switching, timeout abandonment, and client-future ownership. GitHub reports all required checks passing.

Current main is 220 commits ahead of the PR base, but its compare contains neither touched file, so this appears mechanically salvageable.

Automated hermes-sweeper review.

[yingliang-zhang]

Status update after the #102117 refactor (which decomposed plugins/memory/hindsight/init.py 2440 to 1214 lines and moved machinery to {embedded,settings,setup}.py): I audited which parts of this PR the new main already carries, per sub-fix.

Already on new main (absorbed):

• Enqueue-time session identity freeze: _make_turn_retain_job (new init.py around line 979) snapshots content/metadata/lineage tags/bank_id/retain_async/retain_context at enqueue; document_id/update_mode resolved pre-enqueue (sync_turn around line 1020, flush-on-switch on_session_switch around line 1129). The PR headline — a session switch cannot retarget accepted retain work — holds on new main.
• FIFO writer + sentinel + atexit drain + shutdown gating: _writer_loop around line 524 is a single serialized writer; flush-on-switch rides the same queue behind old-session retains; _shutting_down drops late sync_turn() calls.

Still missing on new main (the PR remains valuable for these):

• _build_retain_kwargs re-reads self._retain_tags and self._observation_scopes at write time (around lines 966-968) — config-level fields escape the enqueue freeze.
• Watermark advances at enqueue (around line 1043), not on commit; a failed retain is logged and dropped — turns silently lost, no ordered retry.
• Reconnect retry re-invokes the same closure over the same item dict — no fresh deep copy per attempt; the recreated-client swap (_run_hindsight_operation around line 501) is unlocked and leaks the old client.
• No future ownership through timeout/shutdown: _run_sync abandons timed-out futures; shutdown() closes the client under in-flight ops after the 10s writer join.
• sync_turn(session_id=...) silently rebinds _session_id inline without the rotation/flush discipline on_session_switch enforces.

Net: PARTIAL. I plan to port the lifecycle half (commit-time watermark, ordered retries, per-attempt deep copy, client ownership, lock serialization) onto the new decomposed structure — the identity half the refactor already independently landed.

[yingliang-zhang]

Status after auditing against current main (post-#102117): the core of this PR has been absorbed upstream.

Absorbed (verified on current main):

• Enqueue-time identity freeze: _make_turn_retain_job snapshots content/metadata/lineage tags/bank_id/retain_async/retain_context at enqueue; document_id/update_mode are resolved pre-enqueue (in sync_turn and the flush-on-switch path), so a queued old-session retain can no longer be assembled with new-session identity — the exact mechanism this PR carried.
• on_session_switch flushes under the OLD ids via the same snapshotting job factory.

Residual (deliberately not ported): the writer-loop lifecycle rework (claim-callback / abandoned-state machine / discard-queued-jobs on abandonment, ~the +500-line core of the original). On current main the writer's shutdown contract is drain-then-stop: the sentinel is queued and the writer completes every queued retain before exiting. The rework would change that to abandon-queued-on-shutdown — a behavior change (pending retains are dropped when the writer is wedged past the 10s join), not a bugfix for the premise this PR was reviewed on, and it trades a bounded shutdown for retain loss. If maintainers want the abandonment semantics for wedged writers, that deserves its own issue/PR scoped to the writer contract; nothing in the current code needs it to be correct.

Closing as absorbed-by-main (core) with the residual documented above. Happy to reopen if the writer-contract change is wanted. Thanks for the original review @teknium1.

[yingliang-zhang]

Reopening — the 2026-09-10 closure rationale was wrong.

I closed this on the belief that the enqueue-time identity freeze had been fully absorbed upstream by the #102117 refactor. git cherry upstream/main <head> disproves that: the core commit is still + (patch-unique, not on main).

What upstream main actually has: _make_turn_retain_job snapshots turns/metadata/lineage/bank_id/retain_async/retain_context at enqueue time. What it does not snapshot: the job body still calls self._build_retain_kwargs(...) at run time, which reads mutable provider state. That is the exact defect this PR fixes (a queued old-session retain can still be assembled with new-session configuration).

I will rebase onto current main (the branch predates the #102117 history rewrite) and push shortly.

[yingliang-zhang]

Ported onto current main (d177b119e9) — head now bf1460bceb, conflict-free (MERGEABLE BLOCKED only on the fork-PR workflow-approval gate; please approve the CI run if it reads action_required).

Re-scoped to what current main is still missing. The #102117 rewrite absorbed most of the original branch (enqueue-time snapshot of turns/metadata/lineage/bank_id, _resolve_retain_target called before the writer runs, flush-on-switch under the OLD ids). What it does not do is snapshot the item: _make_turn_retain_job still calls _build_retain_kwargs() inside the writer job, which re-reads mutable per-session attributes at run time.

Concrete defect on main: with retain_every_n_turns > 1 or any queue backlog, an on_session_switch landing between enqueue and writer drain rewrites _retain_tags / _observation_scopes / _retain_source / the _METADATA_ATTRS fields, so an OLD-session retain ships with the NEW session's tags/scopes. The metadata session_id (snapshotted) then contradicts the lineage tags (session:OLD vs metadata session_id=NEW) — a self-inconsistent document.

Fix: build the complete aretain_batch item at enqueue time; the writer job only ships it.

Regression test (TestRetainIdentityIsolation): gates the writer between dequeue and execution, mutates the three attributes mid-flight, and asserts the shipped item keeps the enqueue-time values. Fails on main (tags == [new-tag, ...]), passes at this head.

Verified: 79 passed in tests/plugins/memory/test_hindsight_provider.py (+1 new); the 8 remaining failures are pre-existing on the untouched base (hindsight-client cannot be pip-installed in the sandbox — ImportError in lazy_deps), not introduced by this change.

[yingliang-zhang]

New head bf1460bceb (re-based onto current main d177b119e9, conflict-free — MERGEABLE).

The rebuilt head needs a workflow approval: the three runs queued behind the fork-PR gate are

• CI: 35315206806
• Nix flake check: 35315206477
• Docker Build, Test, and Publish: 35315206427

all action_required. Could a maintainer approve the workflow run so CI can validate the ported fix?

[teknium1]

Closing: the bundled Hindsight provider this PR patches has moved out of this repo.

Thanks @yingliang-zhang for this contribution. In #119888 (merge 9d799e0531c; removal commit 4cbf862abe4) the in-tree plugins/memory/hindsight/ provider was removed — Hindsight now installs from the plugin catalog and its code lives in vectorize-io/hindsight hindsight-integrations/hermes (maintained by @nicoloboschi). There is no longer any code in this repo for the Hindsight half of this PR to patch, so we are closing every open PR against the bundled provider rather than leaving them stranded.

Triage notes:

• Re-scoped (after the author's own audit/reopen in the thread) to one change: build the complete aretain_batch item inside _make_turn_retain_job at enqueue time so the writer job only ships a frozen item, plus a gated-writer regression test (TestRetainIdentityIsolation). Only plugins/memory/hindsight/init.py and its test file; the bundled provider was removed in hindsight moves to the plugin catalog (auto-migrated on first start / hermes update) #119888 (9d799e0) and the pre-removal tree (4cbf862^:1034) still built the item inside _job, so this never landed. The identical run-time _build_retain_kwargs call is still present at the catalog pin (hindsight-integrations/hermes/init.py:1338-1341, reading self._retain_tags/self._observation_scopes at :1310-1312), so it is a genuine upstream fix candidate.
• Still relevant at the catalog pin (dc750388)? Yes — the same code is at hindsight-integrations/hermes/__init__.py:1339 in the upstream tree. It is listed with your credit in Fixes from Hermes-side PRs worth carrying into hindsight-integrations/hermes vectorize-io/hindsight#4662 so it is not lost; if you want to carry the fix yourself, please open it against vectorize-io/hindsight — it would be welcome there.

If you believe this was closed in error, comment and we will reopen.

(Bulk-closed in the hindsight-move close pass.)