Skip to content

fix old sorted set sessions#233

Merged
bsbodden merged 3 commits intomainfrom
fix/remove-working-memory-sessions-zset
Mar 20, 2026
Merged

fix old sorted set sessions#233
bsbodden merged 3 commits intomainfrom
fix/remove-working-memory-sessions-zset

Conversation

@tylerhutcherson
Copy link
Contributor

@tylerhutcherson tylerhutcherson commented Mar 20, 2026

Problem:

Working-memory session metadata was being written to two places on every save/delete:

  • the active Redis Search index over working_memory:* JSON docs
  • the deprecated sessions / sessions:{namespace} sorted sets

Only Redis Search was actually used by list_sessions(). The legacy sorted sets were dead write-only
storage, which caused:

  • unnecessary Redis writes on save/delete
  • wasted Redis memory
  • stale sessions:* entries when working-memory keys expired by TTL
  • ongoing use of a deprecated key path

This happened because sorted sets were originally used for session listing, then Redis Search replaced
that read path, and a later unrelated commit accidentally reintroduced the old ZADD/ZREM writes
without restoring any read dependency on them.

Solution:

  • removed the deprecated sorted-set writes from set_working_memory()
  • removed the deprecated sorted-set deletes from delete_working_memory()
  • kept session listing on the Redis Search index only
  • added an explicit cleanup path for old sessions:* zsets via agent-memory migrate-working-memory
  • did not keep automatic startup cleanup; cleanup is now opt-in through the migration command
  • added regression tests to ensure:
    • working-memory saves do not recreate legacy session zsets
    • legacy zset cleanup works as expected
    • the CLI migration path performs cleanup, while --dry-run does not

Other details:

  • current architecture after this fix is consistent: Redis Search is the only session-listing
    mechanism
  • legacy zset removal is available to operators through migrate-working-memory
  • CLI docs were simplified so the two migration commands are clearer:
    • migrate-memories
    • migrate-working-memory

Note

Medium Risk
Moderate risk because it changes Redis write/delete behavior for working memory sessions and adds a cleanup routine that deletes keys matching sessions/sessions:* when they are zsets. Should be safe if no consumers still rely on those legacy sorted sets, but misclassification or unexpected key usage could remove data.

Overview
Working memory session persistence no longer writes to the deprecated sessions / sessions:* sorted sets. set_working_memory() and delete_working_memory() now only set/delete the working_memory:* JSON key, relying entirely on the search index for session listing.

Adds an opt-in cleanup path for legacy zsets. A new cleanup_deprecated_sessions_zsets() deletes sessions and sessions:* keys only when they are zsets, and agent-memory migrate-working-memory runs this cleanup (skipped on --dry-run) before marking migration complete when no string keys remain.

Docs and tests are updated to reflect the new migrate-working-memory command and to prevent regressions (no zadd/zrem calls, CLI cleanup behavior, and selective zset deletion).

Written by Cursor Bugbot for commit 5b1a7dd. This will update automatically on new commits. Configure here.

@jit-ci
Copy link

jit-ci bot commented Mar 20, 2026

🛡️ Jit Security Scan Results

CRITICAL HIGH MEDIUM

✅ No security findings were detected in this PR


Security scan by Jit

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. No suggestions at this time.

@tylerhutcherson tylerhutcherson added the bug Something isn't working label Mar 20, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes deprecated Redis sorted-set (sessions:*) writes for working-memory session metadata, consolidating session listing to Redis Search only, and adds an explicit opt-in cleanup path via the migrate-working-memory CLI command.

Changes:

  • Removed legacy ZADD/ZREM writes from set_working_memory() and delete_working_memory().
  • Added cleanup_deprecated_sessions_zsets() and wired it into agent-memory migrate-working-memory (skipped on --dry-run).
  • Added regression tests to ensure legacy session zsets are not recreated and that cleanup/migration behavior works as intended.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
agent_memory_server/working_memory.py Removes deprecated zset session indexing writes and adds a cleanup helper for legacy sessions/sessions:* zsets.
agent_memory_server/cli.py Updates migrate-working-memory to optionally clean legacy session zsets (not on dry-run).
tests/test_working_memory_strategies.py Ensures strategy-based working-memory writes do not call deprecated zadd.
tests/test_working_memory.py Adds integration tests for “no legacy zset creation” and for cleanup correctness.
tests/test_cli.py Adds CLI tests verifying cleanup runs (and that dry-run skips it) and command registration includes migrate-working-memory.
docs/cli.md Updates CLI docs for migration commands (with one remaining command-name mismatch noted in comments).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@vishal-bala vishal-bala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Collaborator

@bsbodden bsbodden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@bsbodden bsbodden merged commit 7930c4e into main Mar 20, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants