Skip to content

fix(hindsight): acknowledge append retains after success - #64599

Open
Tosko4 wants to merge 2 commits into
NousResearch:mainfrom
Tosko4:fix/hindsight-append-retain-ack
Open

fix(hindsight): acknowledge append retains after success#64599
Tosko4 wants to merge 2 commits into
NousResearch:mainfrom
Tosko4:fix/hindsight-append-retain-ack

Conversation

@Tosko4

@Tosko4 Tosko4 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Hindsight append-mode retains currently advance the turn watermark as soon as a background write is queued. If that write fails, the writer logs the exception and drops the job, while the advanced watermark prevents the failed turns from being included in the next retain. That silently loses conversation turns from Hindsight.

This PR gives append retains acknowledgement-on-success semantics:

  • queued append jobs capture a target turn boundary;
  • turns are removed from the in-memory append buffer only after aretain_batch succeeds;
  • a failed prefix remains pending and is included in the next append boundary;
  • FIFO jobs resolve only the still-unacknowledged part of their own target, preventing loss, duplication, or reordering while writes are in flight;
  • session switches replace the active append state while queued old-session jobs continue to reference only the old state.

sync_turn() remains non-blocking, successful append retains still bound the buffer, and overwrite/legacy retention behavior is unchanged.

Related Issue

Related to #62977.

That PR identifies the unbounded append buffer, but its enqueue-time clear still makes queued turns unrecoverable when the background Hindsight write fails. This PR addresses the missing delivery/acknowledgement semantics while preserving the bounded-memory goal.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Added per-session append-retain state with lock-protected pending-turn accounting in plugins/memory/hindsight/__init__.py.
  • Changed append queue jobs to acknowledge and prune only the prefix confirmed by a successful aretain_batch call.
  • Kept failed append prefixes available for a later boundary or old-session switch flush.
  • Isolated queued old-session work from newly switched session state.
  • Synchronized switch flush publication and state rotation with shutdown ownership.
  • Reset append target accounting when a provider is reinitialized.
  • Added deterministic regression coverage for failed retries, successful and failed in-flight writes, exact batch partitioning, post-ACK compaction, reinitialization, and session-switch/shutdown races.
  • Updated the hermetic session-switch test provider to initialize the new append state.

How to Test

  1. Run the Hindsight provider and session-switch suites:

    python -m pytest \
      tests/agent/test_memory_session_switch.py \
      tests/plugins/memory/test_hindsight_provider.py -q

    Result: 137 passed.

  2. Run the append acknowledgement regressions directly:

    python -m pytest tests/plugins/memory/test_hindsight_provider.py \
      -q -k 'TestAppendRetainAcknowledgements'

    Result: 9 passed. The delivery-failure, concurrent shutdown, and reinitialization regressions were observed failing against the corresponding pre-fix behavior before their implementations were added.

  3. Attempt the repository test suite:

    scripts/run_tests.sh tests

    The run was interrupted by the host's /tmp tmpfs user quota (OSError: [Errno 122] Disk quota exceeded) after the relevant Hindsight files had passed. Re-running the affected files with TMPDIR on the root filesystem eliminated most failures; the remaining unrelated files either hardcode /tmp/hermes_test or depend on the runner's original temp environment. The focused Hindsight/session-switch suite above is green; CI remains the authoritative repository-wide gate.

  4. Run static and compatibility checks:

    ruff check \
      plugins/memory/hindsight/__init__.py \
      tests/agent/test_memory_session_switch.py \
      tests/plugins/memory/test_hindsight_provider.py
    python3 -m compileall -q plugins/memory/hindsight
    python3 scripts/check-windows-footguns.py \
      plugins/memory/hindsight/__init__.py \
      tests/agent/test_memory_session_switch.py \
      tests/plugins/memory/test_hindsight_provider.py
    git diff --check

    Result: all checks passed. Scoped ty reports the same existing diagnostic categories and counts as clean main; this change introduces no new type diagnostics.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Ubuntu Linux, Python 3.11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A; no user-facing configuration or API changed
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide; the diff uses only cross-platform Python threading/queue primitives and passes the repository footgun check
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

Not applicable; this is a background memory-provider correctness fix with deterministic regression tests.

@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers P3 Low — cosmetic, nice to have labels Jul 14, 2026
@Tosko4

Tosko4 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

CI is green except for Python tests / Run tests slice 2/8, which failed twice on the timing-sensitive tests/tools/test_async_delegation.py::test_interrupt_all_signals_running_children assertion (expected "interrupted", got "completed").

This PR only changes the Hindsight provider and its session-switch/provider tests. The failing delegation test passes locally on both this PR head and the current main head (df5700ebe). I do not have permission to rerun upstream Actions; could a maintainer please rerun the failed job?

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused acknowledgement fix. The current PR base still advances the append watermark at queue time (plugins/memory/hindsight/__init__.py:1638-1696), while the writer logs and drops a later failed job (plugins/memory/hindsight/__init__.py:1107-1128), so the reported loss path is real.

The new prefix state acknowledges only after aretain_batch() returns (plugins/memory/hindsight/__init__.py:1613-1673) and isolates old session state during rotation (plugins/memory/hindsight/__init__.py:1908-2015). The regression suite covers retry after failure, in-flight ordering, session switch, shutdown, reinitialization, and compaction (tests/plugins/memory/test_hindsight_provider.py:1497-1771). No verified correctness or design-fit issue found.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/memory Memory subsystem: store, providers, sync, background reviews labels Jul 16, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

The 26 listed PRs address or reference a memory-lifecycle complex spanning stale flush-agent writes, provider shutdown reachability, Hindsight final-batch loss, and append-write acknowledgement. The direct provider fixes add session-end flushing for buffered Hindsight turns, while #64599 independently prevents failed queued append writes from being acknowledged and discarded prematurely.

Related pull requests

Duplicates

#2675 and #2676 were superseded by #2687; #3045 by #3297; #12917 by #16207; #15481 by #16571; and #31856 by #57378. #16697, #28845, #36219, #36988, #55046, and #55936 are competing Hindsight session-end flushes, with #55046 the recorded best existing fix and the others duplicate or partial variants.

Suggested consolidation

Keep #64599 open with the salvage path validated by its keep_open review: preserve acknowledgement-after-success, failed-prefix retry, FIFO target resolution, old-session isolation, and the supplied regression coverage. Separately keep #55046 open as the recorded best session-end-flush track; close #28845, #36219, and #55936 as duplicate/partial variants of #55046 only with the review deviations above recorded, while #8331 and #29857 remain separate author-action tracks for their contributor-reviewed fixes.

Cross-PR triage: Reviewed 26 pull requests and 6 issues in this complex. Each diff was read against this issue; Assessment working set: 372 kB of PR diffs, 95 kB of issue/PR text, 29 kB of discussion (43 comments), 35 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@alt-glitch alt-glitch removed the sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages label Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/memory Memory subsystem: store, providers, sync, background reviews comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants