Skip to content

fix(memory): defer state mutation in add_provider until schema load succeeds - #13224

Closed
Magicray1217 wants to merge 1 commit into
NousResearch:mainfrom
Magicray1217:fix/issue-9948-memory-provider-add
Closed

Magicray1217 wants to merge 1 commit into
NousResearch:mainfrom
Magicray1217:fix/issue-9948-memory-provider-add

Conversation

@Magicray1217

Copy link
Copy Markdown

Problem\n\nMemoryManager.add_provider() mutates internal state (_has_external = True and appends provider to _providers) BEFORE provider.get_tool_schemas() succeeds. If get_tool_schemas() raises, the manager is left in a poisoned half-registered state.\n\n## Fix\n\n- Call get_tool_schemas() first in a try/except\n- Only mutate _has_external and _providers after successful schema load\n- Reuse the loaded schemas list instead of calling get_tool_schemas() redundantly\n\nFixes #9948

…ucceeds. Fixes NousResearch#9948

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists tool/memory Memory tool and memory providers labels Apr 22, 2026
@alt-glitch

Copy link
Copy Markdown

Related to #10051 and #9997 — all three fix the same #9948 bug (add_provider state mutation before schema load). Maintainers should pick one.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for isolating the registration-order failure.

Problems

  • The change has no regression test. Current coverage exercises successful registration and second-external rejection in tests/agent/test_memory_provider.py:139-171, but does not verify that a raising get_tool_schemas() leaves registration state clean.
  • The patch predates current registration validation: agent/memory_manager.py:400-434 now normalizes schemas and excludes reserved core-tool names. A salvage should retain those checks while caching schemas before the mutations at agent/memory_manager.py:396-398.

Suggested changes

  • Add a provider whose get_tool_schemas() raises; assert no provider/tool mapping remains and a subsequent external provider can register successfully.
  • Move the schema-first/cached-list behavior into the current registration loop without dropping normalization or reserved-name filtering.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users area/memory Memory subsystem: store, providers, sync, background reviews labels Jul 12, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Salvaged onto current main as #109131 with your commit authorship preserved where the diff still applied (the file moved/was restructured since April, so some of it is a hand-port with credit in the commit message). Once #109131 merges this PR will be closed with a link to the landed SHA. Thanks for the fix.

@teknium1

Copy link
Copy Markdown
Collaborator

Landed via #109131 (merge befa573) with your commit cherry-picked so authorship is preserved — thank you @Magicray1217. Closing this PR as superseded by the merged salvage; the fix is on main now.

@teknium1 teknium1 closed this Sep 12, 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 P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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