Skip to content

fix(honcho): serialize concurrent message flushes - #86094

Open
Diaspar4u wants to merge 3 commits into
NousResearch:mainfrom
Diaspar4u:fix/honcho-concurrent-flushes
Open

fix(honcho): serialize concurrent message flushes#86094
Diaspar4u wants to merge 3 commits into
NousResearch:mainfrom
Diaspar4u:fix/honcho-concurrent-flushes

Conversation

@Diaspar4u

@Diaspar4u Diaspar4u commented Aug 14, 2026

Copy link
Copy Markdown

What does this PR do?

Serializes Honcho message flushes at the session-manager boundary. Without serialization, two concurrent flush paths can select the same unsynced messages before either path marks them synced, causing duplicate uploads.

The lock covers message selection, upload, synced-state updates, and the cache write. This PR intentionally changes no write-frequency, shutdown, configuration, or migration behavior.

Related Issue

Supersedes #72708. Its earlier broader scope is already implemented on main; this PR resubmits only the remaining concurrent-flush defect.

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

  • Add a per-remote-session reentrant lock around HonchoSessionManager._flush_session().
  • Add deterministic regressions for same-session duplicate prevention, unrelated-session concurrency, and same-thread re-entry.

How to Test

  1. Run scripts/run_tests.sh tests/honcho_plugin/test_async_memory.py -k concurrent_flushes_do_not_duplicate_messages -q.
  2. Run scripts/run_tests.sh tests/honcho_plugin -q.
  3. Run ruff check plugins/memory/honcho/session.py tests/honcho_plugin/test_async_memory.py.

The focused regression fails on unmodified main at the second concurrent upload and passes with this patch. The Honcho plugin suite passes 333 tests.

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 macOS

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

N/A — covered by the deterministic concurrency regression test.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 14, 2026
@Diaspar4u

Copy link
Copy Markdown
Author

Current head 760a756d5802cbafd0d83a8b144220b78103a648 is mergeable against current main and contains only the remaining concurrent-flush fix plus its deterministic regression.

Validation on this exact head:

  • focused regression: fails on unmodified main, passes here
  • Honcho plugin suite: 331 passed
  • focused Ruff: passed

The fork workflows are currently action_required for this head. @teknium1 @alt-glitch, please approve Actions for this exact SHA and review the reduced PR. I will keep the head frozen while CI runs.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(honcho): serialize concurrent message flushes

  1. plugins/memory/honcho/session.py:_flush_session — the lock is manager-global, so it serializes flushes across all sessions, and it is held across the network upload (add_messages). A slow Honcho API call blocks unrelated sessions' flushes and any shutdown path that flushes. If per-session isolation is wanted, a lock per honcho-session-id (a small dict of locks) would preserve the fix while limiting the blast radius; if global serialization is intentional, a comment stating so would prevent a future "optimization" from reintroducing the race.

  2. threading.Lock is non-reentrant. If any code path can reach a nested _flush_session while the lock is already held (e.g. a sync-marking failure handler, or a shutdown flush that re-enters through another method), it would deadlock. The new test covers two concurrent external callers but not re-entrancy; a quick audit of _flush_session_locked's body for nested flush calls would close the question.

  3. Only _flush_session is guarded. If other entry points upload unsynced messages without going through it (e.g. shutdown() draining the async queue directly, or a sync_turn fast path), duplicates could still occur outside the lock. Confirming all upload paths route through the lock would fully close the gap the fix targets.

  4. The ObservableFlushLock test (waiting on first_upload_started before starting the second thread, then asserting lock_entry_count == 2) is deterministic as written — good coverage of the serialization contract.

@Diaspar4u

Diaspar4u commented Aug 16, 2026

Copy link
Copy Markdown
Author

Addressed all four points on current head 02b8f92:

  1. Fixed the lock scope. Flush exclusion is now keyed by remote Honcho session ID, so duplicate uploads for one session remain serialized while unrelated sessions can upload concurrently.
  2. Closed the re-entrancy risk. Each per-session lock is an RLock. The current locked body has no nested flush call, and a dedicated regression now proves same-thread re-entry cannot deadlock.
  3. Audited every upload path. Turn/interval writes, the async writer and retry, flush_all() cache draining, async-queue draining, and shutdown all route through _flush_session; the only add_messages call remains inside the locked implementation.
  4. Retained and expanded the deterministic coverage. The original same-session duplicate-prevention test remains, with new tests proving independent sessions overlap and same-session re-entry is safe.

Validation: 161 focused Honcho session/async/auth tests passed; focused Ruff, attribution audit, both diff checks, and all three upstream-range SSH signature checks passed.

@teknium1 @alt-glitch please re-review the current head. Actions on exact head 02b8f92 are awaiting write-access approval; please approve the workflow runs.

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

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have 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.

3 participants