Skip to content

fix(gateway): add missing session_key param to _async_flush_memories - #5379

Closed
Mibayy wants to merge 1 commit into
NousResearch:mainfrom
Mibayy:fix/crash-flush-memories-wrong-args
Closed

fix(gateway): add missing session_key param to _async_flush_memories#5379
Mibayy wants to merge 1 commit into
NousResearch:mainfrom
Mibayy:fix/crash-flush-memories-wrong-args

Conversation

@Mibayy

@Mibayy Mibayy commented Apr 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • _async_flush_memories was defined with only self and old_session_id as parameters
  • All 3 call sites pass a second positional argument (session_key / key), causing a TypeError: _async_flush_memories() takes 2 positional arguments but 3 were given at runtime
  • As a result, memories are never flushed on session expiry (line 1285), session reset (line 3141), or session resume/switch (line 4871)

Fix

Added session_key: str = "" as a second parameter to the function signature. The parameter is accepted but not used inside the function body — it is passed by callers for contextual logging at the call site and does not need to be forwarded into _flush_memories_for_session.

One-line diff:

- async def _async_flush_memories(self, old_session_id: str):
+ async def _async_flush_memories(self, old_session_id: str, session_key: str = ""):

Impact

Without this fix, any path that triggers memory flushing silently crashes (the TypeError is caught by surrounding try/except blocks), so accumulated memories are discarded on every session boundary instead of being persisted.

Test plan

  • Confirm no TypeError is raised when sessions expire, reset, or resume
  • Verify memory flush tasks complete successfully via logging output ("Pre-reset memory flush completed")
  • Run existing unit/integration tests for the gateway module

🤖 Generated with Claude Code

Function was defined with only old_session_id but all 3 call sites pass
a second session_key argument, causing TypeError at runtime. Memory is
never flushed on session reset, expiry, or resume.

Fixes: NousResearch#5358-related
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Mibayy

Mibayy commented Apr 6, 2026

Copy link
Copy Markdown
Contributor Author

CI status note

The failing tests in this PR are pre-existing failures on main — identical failures exist in the latest main CI run (run 24015315481):

  • test_code_execution — requires Modal API token not present in CI
  • test_matrix*nio.crypto missing + mock spec incompatibility (unrelated lib issue)
  • test_config_version_unchanged — was already failing on main

None of the failing tests touch the code changed in this PR (gateway/run.py line 735).

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the careful analysis, @Mibayy! This is a well-described and correct fix — unfortunately the code it patches was completely removed from main before this PR was opened.

Commit ea01bdceb (PR #15696, merged April 25 2026) deleted the entire flush_memories subsystem:

  • _async_flush_memories — removed
  • _flush_memories_for_session — removed
  • All three call sites in gateway/run.py — removed

The background memory review loop (firing every 10 user turns) is now the sole memory-persistence path, making the session-boundary flush unnecessary. Because the method and all its callers are gone, the TypeError you identified can no longer occur on current main.

Closing as already superseded on main. This is an automated hermes-sweeper review.

@teknium1 teknium1 closed this Apr 27, 2026
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/gateway Gateway runner, session dispatch, delivery tool/memory Memory tool and memory providers labels Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P1 High — major feature broken, no workaround 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