refactor(memory): refactor TestMemoryStore to use the unified storage interface#3260
refactor(memory): refactor TestMemoryStore to use the unified storage interface#3260opieter-aws wants to merge 9 commits into
Conversation
github-actions
Bot
added
size/m
area-community
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Assessment: Comment Clean, well-motivated refactor — moving Review themes
Nice work dropping the load-memoization complexity and improving browser-bundle safety with the static |
9ee577e to
6f46a1f
Compare
61e0ba8 to
e7f6484
Compare
Re-review (latest push, merge commit
|
|
Docs haven't yet been released, the TestMemoryStore was released last week. So breaking change seems acceptable to me. |
opieter-aws
added
api/needs-review
| const backend = storage ?? new InMemoryStorage() | ||
| this._storage = NAMESPACED in backend ? backend : namespace(backend, 'memory') | ||
| this._key = `${sanitizeName(name)}.json` | ||
| } |
There was a problem hiding this comment.
Issue: The new NAMESPACED in backend ? backend : namespace(backend, 'memory') guard is the right fix, but its "already-namespaced" arm is untested. The 33-test suite always passes a bare InMemoryStorage/LocalFileStorage, so only the else branch (auto-namespacing) is exercised. The double-prefix regression this guard prevents could silently come back.
Suggestion: Add one test that passes a pre-namespaced view and asserts no double-prefixing, e.g.:
it('does not double-prefix an already-namespaced backend', async () => {
const backend = new LocalFileStorage(dir)
const store = new TestMemoryStore({ name: 'notes', storage: backend.namespace('memory') })
await store.add('hi')
// written under memory/notes.json, NOT memory/memory/notes.json
expect(await backend.read('memory/notes.json')).toBeDefined()
expect(await backend.read('memory/memory/notes.json')).toBeUndefined()
})
Re-review (updated after force-push)Assessment: Comment — the two substantive concerns from my first pass are addressed in code. One follow-up (test coverage) and one process item remain, neither blocking. Status of prior feedback
Housekeeping: my earlier tooling retried and created duplicate inline threads on lines 149/150. I lack permission to delete/edit them, so I've replied marking the extra copies as duplicates — please disregard those. Nice, focused iteration on the namespacing fix. |
|
@strandly-the-agent don't you like me? where's my response? 😢 |
mkmeral
left a comment
mkmeral
left a comment
There was a problem hiding this comment.
why are we making a breaking change? can't we solve this another way?
There was a problem hiding this comment.
🔴 Latest implementation: changes requested — one Python data-loss blocker remains at 541fb5d5.
✅ Exact head 541fb5d5e23118511982b7db279359b3e109298b matches the remote; git diff --check and worktree are clean.
✅ Focused verification: Python 42/42; TypeScript 36/36, no type errors.
✅ The prior sequential two-asyncio.run() failure is fixed: 4/4 records persist. Public persist/path, the persistent default, and prior file locations are restored.
🔴 Simultaneous threads/loops still return two successful add() IDs while persisting one record: repro and fix guidance.
🟡 An invalid explicit path can make nonempty search return [] instead of surfacing the configuration error: details.
API: no exported shape changed, but concurrency/error/fresh-read contracts did. Keep api/needs-review; no API meeting is needed.
Latest delta and observable behavior
Restored / unchanged
- Python and TypeScript retain
persist/path; there is no publicstorageparameter. - Default persistence remains
~/.strands/memory/<sanitized-name>.json; explicitpathstill selects that file;persist=falseremains per-instance ephemeral. - Exports,
search/addsignatures, result types, ranking, deduplication, record fields, and normal data locations are unchanged. - The prior eager-loop-bound lock finding is resolved and regression-tested. Its old inline thread is obsolete/outdated; this review does not re-raise it.
Remaining observable deltas from the internal Storage refactor
- Every token-bearing search and every valid add rereads storage instead of using the prior instance cache, so external edits/deletes/corruption become visible.
persist=falsenow JSON-serializes records, so unsupported Python metadata can fail where the prior in-memory list accepted it.- Backend failures now surface as
StorageErrorand generally name the key/basename rather than the old full-pathOSError/genericErrormessage. - Malformed non-object metadata is rejected eagerly.
- LocalFileStorage retains atomic temp-file replacement for individual writes, but the Python instance read-modify-write transaction is not serialized across simultaneous loops.
CI snapshot
Functional checks completed so far are green across Linux Python 3.10–3.14, most Windows/macOS Python jobs, TypeScript Node 20/22/24 Linux/macOS, lint, code quality, audit, package install, and patch coverage. Some Windows/docs/integration jobs were still running at review time; the aggregate failure was the expected API-label policy gate.
Routed review passes
- Correctness / adversarial: sequential-loop reuse now passes. A deterministic simultaneous two-thread/two-loop barrier repro returns two IDs but retains one record; merge-base serialized this same-instance case.
- API / DevX: public shape/default/location compatibility is restored. The PR body’s “no behavior change” wording is broader than the actual fresh-read, error, validation, serialization, and concurrency deltas.
- Test quality: the new yielding sequential-loop regression is strong. The missing simultaneous-loop barrier test directly hides the remaining blocker; lower-value filename/ephemeral/TS-recovery gaps were suppressed.
- Docs accuracy: prior breaking-change and namespacing feedback is stale and not repeated. Remaining wording nits were suppressed behind the concrete concurrency and invalid-path findings.
- Model context: this remains agent-visible: concurrent synchronous invocations can receive successful memory IDs for records that were overwritten, while ENOTDIR currently looks like “no memories.”
|
We could keep the store backwards compatible with this interface But given that:
I think we can still do a breaking change here. |
|
@mkmeral 👋 sorry for the wait — here's the consolidated public API change breakdown for bar raising. This PR now lands both SDKs (Python + TypeScript) and the surfaces are symmetric. Note: my earlier
|
| Before | After | |
|---|---|---|
persist?: boolean / persist: bool |
✅ (default true) |
❌ removed |
path?: string / path: str |
✅ | ❌ removed |
storage?: Storage |
❌ | ✅ new (optional; defaults to InMemoryStorage) |
persist=false → pass storage: new InMemoryStorage(); path=x → pass storage: new LocalFileStorage(x).
2. Behavior change: default persistence flipped (⚠️ silent data-loss on upgrade)
- Before: persisted to
~/.strands/memory/<name>.jsonby default — durable across restarts. - After: ephemeral by default (
InMemoryStorage) — entries lost on process exit, no error/warning. - The most common call site
TestMemoryStore({ name })keeps compiling but silently stops persisting. This was flagged earlier and is now documented in the docstring; still worth a loud release-note callout.
3. Second, subtler location change (worth explicit bar-raiser attention)
Even after migrating to storage: new LocalFileStorage(), the default directory moves ~/.strands/ (home) → ./.strands/ (cwd) per the new docstrings. So a user doing the "obvious" 1:1 migration to keep persistence will not find their old data (different absolute path + memory/ namespace subdir). This is a second migration gotcha layered on top of #2 — recommend calling it out explicitly in migration notes.
4. Failure-mode / exception-contract changes
- Python
add/search: previously raisedOSError(with target path) on disk read/write failure → now raisesStorageErrorfrom the backend.ValueErrorstill covers malformed blobs, and now additionally validatesmetadatashape. - TypeScript
search/add: contract now documents@throws StorageErrorfor backend read/write failures — a new exception type callers must account for. - Constructor: the old
path must not be emptyvalidation is gone (param removed).
5. New composition behavior — auto-namespacing
- Backend is auto-scoped under a
memory/prefix unless the caller already passed a namespaced view (NAMESPACED in backend), so a pre-namespaced backend won't get double-prefixed (memory/memory/...). Consistent withSessionManager/offloader convention. - Record blob key:
memory/<sanitized-name>.json. On-disk record format unchanged (camelCaseid/content/metadata/createdAt, Z-suffixed ms timestamps), so cross-SDK read compat holds.
6. Concurrency model change (Python, behavioral)
- Switched from
threading.Lock→ per-event-loopasyncio.Lock(rebinds per running loop to support a store reused across the fresh loops a syncAgentspins up). - Dropped the in-memory cache + load memoization: every
search/addreads fresh from the backend. Trade-off: eliminates the cold-load race by construction, at the cost of a backend read per operation. Prior cross-OS-thread guarantees are replaced by per-loop async serialization.
Open questions for bar raising
- Backwards-compat shim vs. clean break. @opieter-aws proposed keeping
persist/pathas@deprecatedaliases. Given (a) docs not yet released and (b) zero GitHub/internal usage, a clean break is defensible — but the deprecation shim is cheap insurance against the two silent behavior changes above (ci: update mypy requirement from <1.0.0,>=0.981 to >=0.981,<2.0.0 #2 + ci: update sphinx-rtd-theme requirement from <2.0.0,>=1.0.0 to >=1.0.0,<4.0.0 #3). Worth a deliberate decision rather than defaulting to either. - Is ephemeral-by-default the right "happy path"? The tenets caution against the obvious path silently doing the surprising thing. For a store literally named to persist "memory," a new user's mental model may expect durability. Confirm ephemeral-default was weighed against a persistent default (or a required explicit
storagearg to force the choice). - Should
storagebe required rather than optional-with-ephemeral-default? Making it required would force callers to consciously choose durability vs. ephemerality and eliminate both silent-data-loss vectors — at the cost of ergonomics for the throwaway/prototype case.
Everything is internally consistent and well-tested across both SDKs; these are design-intent confirmations for the bar-raiser, not correctness gaps.
|
What if |
|
@pgrayy that's the crux of the bar-raising decision, so let me lay out the tradeoff both ways rather than pick a side. Your framing (keep
What that gives up — i.e., "does it need anything other than
There's also a consistency dimension: Possible middle ground worth weighing: keep an ergonomic persistence default (so the common call site doesn't silently lose data) and accept an optional Net: if the team's read is that this store will only ever realistically be local-file or in-memory, your under-the-hood approach is the lower-risk call. If browser/community backends are in scope (the TS browser-safety work suggests they are), exposing |
|
@pgrayy good question to pressure-test the whole approach — I think it splits into two parts: "Does "Keep the same constructor args but build The hybrid worth considering (close to @opieter-aws's earlier sketch, minus the deprecation): accept new TestMemoryStore({ name }) // persists to ~/.strands/memory (unchanged — no data loss)
new TestMemoryStore({ name, persist: false }) // ephemeral
new TestMemoryStore({ name, path: '/data/notes.json' })// custom file
new TestMemoryStore({ name, storage: new S3Storage() })// any backendThat gets backwards-compat + zero silent migration surprises + full extensibility. The price is a wider config surface and a precedence rule to document (what happens if both Net: the pure "constructor-args-only, LocalFileStorage under the hood" option is the simplest but forfeits the browser story that partly motivated the migration; the hybrid keeps that story while erasing the breaking-change concerns. Given docs aren't released and usage is ~zero, the clean |
|
@strandly-the-agent review latest implementation |
opieter-aws
changed the title
Re-review of the latest implementationThis pivot is a clean resolution — nicely done. The store now runs on the unified
One actionable item (Important) — PR metadata is now staleThe code is no longer a breaking change, but the PR still advertises one:
Please update the title (drop the Minor / optional
Assessment: Approve (pending the metadata cleanup). The implementation is correct, backwards-compatible, well-tested, and preserves the guarantees of the original. This ended up in a much better place than a breaking change — good call incorporating the backwards-compatible route. |
| # never does I/O. | ||
| if not persist: | ||
| self._storage: Storage = InMemoryStorage() | ||
| self._key = f"{_sanitize_name(self.name)}.json" |
There was a problem hiding this comment.
Why does the key seem like a file name?
There was a problem hiding this comment.
In the LocalFileStorage the key maps directly to a filename. The .json suffix keeps the on disk file byte-identical to the old implementation.
There was a problem hiding this comment.
But this is for the InMemoryStorage - does that still write to the file system? (I would have expected not)
There was a problem hiding this comment.
Ah missed that, for InMemoryStore indeed this does not write to the file system, it's just an in memory dict. The key just pairs with LocalFileStorage, but it's easy to drop it here if you think it's confusing
| # loop don't each read the same snapshot and clobber one another. Reading inside the critical | ||
| # section guarantees add #N sees add #N-1's write. Serialization is per event loop; adds | ||
| # driven from separate loops/processes against a shared file remain last-write-wins. | ||
| async with self._get_lock(): |
There was a problem hiding this comment.
Should we up level something like this to the Agent or is this really only protecting memory from multiple agents?
From my reading it's the latter but confirming
There was a problem hiding this comment.
It's the latter indeed, the lock only serializes concurrent writes against a single store instance
| records = self._load() | ||
| # The lock serializes the whole read-modify-write cycle so concurrent adds on the same event | ||
| # loop don't each read the same snapshot and clobber one another. Reading inside the critical | ||
| # section guarantees add #N sees add #N-1's write. Serialization is per event loop; adds |
There was a problem hiding this comment.
Serialization is per event loop
What happens if two agents on two different loops attempt to read/write at the same time? Any protection for multiple event loops?
There was a problem hiding this comment.
There's no protection across loops, two agents each using the same test store on their own event loop don’t share a lock. The last write wins.
This is intentional for the test store. With the async storage backend we can't reuse the old locking mechanism and cross-loop coordination would need backend support that the storage interface currently doesn't implement.
I will call this out in the docs to make it explicit.
LMK if you think we need to reconsider this setup.
There was a problem hiding this comment.
This would be a bug right? I'm fine deferring it as a fix, but we're effectively saying that Memory is not intended to be shared by concurrent agents?
There was a problem hiding this comment.
It's a bit more subtle, we're saying that the TestMemoryStore is not intended to be used concurrently across loops. I think it's fine to defer the multi-loop write story to production backends for now, while we add concurrent write mechanism to the Storage object (this is a larger undertaking). But I agree we need to solve this, not just for this use case. I will create an issue with follow up if you agree.
6 participants
Description
TestMemoryStorehand-rolled its own filesystem persistence in both SDKs — path resolution, atomic temp-file writes, plus a records cache with load memoization to paper over cold-load races. The unifiedStorageinterface exists precisely to delete this per-subsystem re-invention, and the storage design doc names the memory store as a consumer to migrate. This moves persistence onto aStoragebackend internally.The public config is unchanged:
persist/pathand the~/.strands/memory/<name>.jsondefault behave exactly as before. Internally the store now resolves a backend from that config —persist: false→InMemoryStorage, otherwise aLocalFileStoragerooted so an explicitpathstill writes to that exact file and the default still lands at the home-dir location. Dropping the bespoke cache also let the read-modify-write inaddread fresh under a lock, closing the cold-load race by construction rather than guarding against it.Related Issues
#3099
#3253
Documentation PR
N/A
Type of Change
Other (please describe): internal refactor — no public API or behavior change
Testing
Ran the
TestMemoryStoreunit suites in both SDKs (Python pytest, TS Node), plus type-check, lint, and format for each; all pass. Exercised each store end to end against both backends, confirming the default home-dir path, an explicitpath,persist: falseephemerality, dedup, and cross-loop reuse from a synchronous agent all behave as before.hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.