Skip to content

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

Closed
Diaspar4u wants to merge 1 commit into
NousResearch:mainfrom
Diaspar4u:fix/honcho-write-contract-salvage
Closed

fix(honcho): serialize concurrent message flushes#72708
Diaspar4u wants to merge 1 commit into
NousResearch:mainfrom
Diaspar4u:fix/honcho-write-contract-salvage

Conversation

@Diaspar4u

@Diaspar4u Diaspar4u commented Jul 27, 2026

Copy link
Copy Markdown

What changed

This PR is intentionally limited to one current-main concurrency defect that is not covered by #83500:

  • serialize Honcho message flushes so two concurrent flush paths cannot select and upload the same unsynced messages twice
  • add a deterministic regression that reproduces the duplicate upload on current main

Superseded scope removed

#83500 already carries the original PR's saveMessages, writeFrequency, migration ownership, and normal async-writer shutdown work. None of that scope is resubmitted here.

Validation

  • regression fails on current main and passes with this commit
  • scripts/run_tests.sh tests/honcho_plugin -q — 294 passed
  • focused Ruff — passed
  • worktree and upstream-range git diff --check — passed
  • upstream-range commit SSH-signature verified

@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 area/memory Memory subsystem: store, providers, sync, background reviews sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state needs-decision Awaiting maintainer decision before any implementation labels Jul 27, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #19650 and #35210: this active salvage combines both documented persistence-contract fixes and adds lifecycle coordination. It is not a duplicate of either open predecessor; maintainers should choose the integration path.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for consolidating the two persistence-contract fixes. The underlying premise still holds on current main: sync_turn() directly calls _flush_session() at plugins/memory/honcho/__init__.py:1348-1353, while the writeFrequency routing boundary is HonchoSessionManager.save() at plugins/memory/honcho/session.py:499-522; saveMessages is parsed at plugins/memory/honcho/client.py:574-576 but is not checked by sync_turn().

Problems

  • The branch predates bd1a850fa2, which made the async writer lazy and requires _ensure_async_writer() before an async enqueue (plugins/memory/honcho/session.py:511-514). The proposed lifecycle rewrite must preserve that path; otherwise queued async saves have no worker.
  • bd1a850fa2 also introduced the package-level network-hermetic Honcho test fixture. The older manager construction pattern in this branch should not be restored.

Suggested changes

  • Salvage the two focused sync_turn() changes, then port lifecycle coordination and its tests onto the current lazy-writer implementation while retaining the hermetic test fixture.

Automated hermes-sweeper review.

@Diaspar4u
Diaspar4u force-pushed the fix/honcho-write-contract-salvage branch 2 times, most recently from 08700c6 to e38db42 Compare July 30, 2026 15:19
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
@Diaspar4u
Diaspar4u force-pushed the fix/honcho-write-contract-salvage branch from e38db42 to eb8f98a Compare July 30, 2026 17:55
@Diaspar4u

Copy link
Copy Markdown
Author

@greptileai

@Diaspar4u
Diaspar4u force-pushed the fix/honcho-write-contract-salvage branch from eb8f98a to 7254059 Compare August 12, 2026 16:20
@Diaspar4u Diaspar4u changed the title fix(honcho): honor writeFrequency/saveMessages fix(honcho): serialize concurrent message flushes Aug 12, 2026
@Diaspar4u

Copy link
Copy Markdown
Author

Re-review requested. I rebased this onto current main and removed every behavior superseded by #83500. The PR now contains only the independently reproducible concurrent-flush duplication fix: without serialization, two flush paths can upload the same unsynced messages twice.

The regression fails on current main, passes here, and also passes when the minimal patch is applied on top of #83500. Focused Honcho/memory validation passed (314 tests), along with Ruff and both diff checks. The single commit is SSH-signed. Please review the reduced scope for merge.

@Diaspar4u
Diaspar4u force-pushed the fix/honcho-write-contract-salvage branch from 7254059 to 0e7bdb2 Compare August 12, 2026 18:49
@alt-glitch alt-glitch removed needs-decision Awaiting maintainer decision before any implementation sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 12, 2026
@Diaspar4u
Diaspar4u force-pushed the fix/honcho-write-contract-salvage branch from 0e7bdb2 to a36fc94 Compare August 12, 2026 20:41
@Diaspar4u

Copy link
Copy Markdown
Author

Rebased onto current upstream main at 1d3d021282; scope remains limited to serializing concurrent Honcho flushes so two writers cannot upload the same unsynced messages.

Validation on head a36fc940d9ae:

  • Honcho plugin suite: 294 passed
  • focused Ruff and both diff checks: passed
  • upstream-range commit SSH-signature verified
  • independent security/correctness review: passed

@teknium1 @alt-glitch please review the current head.

@Diaspar4u
Diaspar4u force-pushed the fix/honcho-write-contract-salvage branch from a36fc94 to c9c0494 Compare August 13, 2026 17:07
@Diaspar4u

Copy link
Copy Markdown
Author

Rebased the focused concurrent-flush serialization fix onto current main at head c9c049405a; its upstream-range commit has a verified SSH signature. Scope remains limited to preventing duplicate uploads when flush paths race.

Validation: 294 Honcho plugin tests passed; focused Ruff, git diff --check, and upstream-range diff checks passed.

@coderabbitai review

@teknium1 @alt-glitch please re-review the current head.

kshitijk4poor pushed a commit that referenced this pull request Aug 13, 2026
…ager.save()

sync_turn called manager._flush_session() directly, which flushes
synchronously every turn no matter what writeFrequency says — the
"async", "session", and every-N-turns modes were dead configuration
on the main turn path. Route through save(), the dispatcher that
actually implements those modes.

Same bug class reported in #19650 (starship-s) and #72708 (Diaspar4u);
this takes the minimal one-line routing fix without their broader
lifecycle refactors.

Co-authored-by: starship-s <45587122+starship-s@users.noreply.github.com>
@Diaspar4u

Copy link
Copy Markdown
Author

Superseded by #86094, rebuilt from current main with only the remaining concurrent-flush fix and focused regression test.

@Diaspar4u Diaspar4u closed this Aug 14, 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.

3 participants