WALM-295: Owner-scoped memory read API (namespaces/memories/agents) - #537
WALM-295: Owner-scoped memory read API (namespaces/memories/agents)#537harrymove-ctrl wants to merge 20 commits into
Conversation
Owner-scoped read API (namespaces/memories/agents) plus per-memory expiry fields, covering both tickets since WALM-296 extends the same memories response WALM-295 creates.
…, namespaces pagination, agents caching, route metrics, auth docs
…S vector races in memory_read tests
ducnmm
left a comment
There was a problem hiding this comment.
Found five issues that should be addressed before merge:
-
High —
/agentsfails on testnet.list_owner_agentsalways calls the JSON-RPC-onlylist_delegate_keys_cached, even though testnet requiresSUI_GRPC_URLand JSON-RPC is explicitly unsupported. Auth can succeed through gRPC, then every cold/expired/agentslookup returns 500. Please implement delegate-key listing throughstate.sui_grpc_client, with JSON-RPC only as a supported fallback. -
High —
/namespacesdoes not support incremental sync. Itsupdated_aftercursor is only the last namespace name (HAVING namespace > $2). If Console has syncedalphaand a later write changesalpha's count/storage, polling with the prior cursor never returns that changed rollup. This conflicts with WALM-295's incremental-listing scope. The cursor needs change/high-watermark semantics, or the API contract and ticket scope need to be revised explicitly. -
High —
/memoriesdoes not return a terminal watermark.next_cursoris generated only whenhas_more. An owner whose result fits in one page receives no checkpoint at all, and after the final page of a multi-page traversal the latest row is likewise not checkpointed. Clients therefore cannot poll incrementally without rereading data. Return the last emitted cursor independently ofhas_more, or add a separate continuation watermark. -
High — migrations 010–012 have a rolling-deploy race. Migration 010 adds nullable
updated_atwithout a default, 011 backfills and commits, and only 012 installs the default plusNOT NULL. An old replica can insert a new NULL row between 011 and 012, causingSET NOT NULLto fail and new replicas to crash-loop. InstallDEFAULT NOW()when the column is introduced, before the backfill, then enforce non-nullability. -
Medium — incremental sync cannot communicate deletion. Forget and expired-blob cleanup hard-delete
vector_entries; laterupdated_afterreads only scan rows that still exist. Console can retain a deleted memory indefinitely. Add tombstones/change events, or explicitly require and support periodic full reconciliation.
CI is green, but current tests do not cover testnet gRPC behavior, rolling deployment, or incremental lifecycle behavior after update/delete.
ducnmm
left a comment
There was a problem hiding this comment.
Found two blocking issues and two pagination-contract issues on the current head (c09bdd05):
-
High — rolling deployment can fail the
updated_at NOT NULLmigration.services/server/migrations/010_memory_read_api_columns.sql:24adds the column nullable without a default. Existing replicas can insert NULL rows while 011 backfills; 012 then fails atservices/server/migrations/012_memory_read_api_updated_at_not_null.sql:13. Set the default before the backfill so concurrent writers cannot create new NULLs. -
High —
/agentsbypasses the configured gRPC client.services/server/src/routes/memory_read.rs:357-363passes only the HTTP client/JSON-RPC URL, andservices/server/src/storage/sui.rs:221-227always uses JSON-RPC. Testnet requires gRPC, so cold-cache/agentsrequests can return 500 despite auth succeeding through the gRPC client. Route this through the configured Sui client path. -
Medium — final memory page drops the sync watermark.
memory_read.rs:249-253returns a cursor only when another row already exists. One-page or completed traversals end withnext_cursor: null, so clients have no(updated_at, id)watermark for future incremental polling. -
Medium — namespace continuation cannot surface updates to earlier-sorting namespaces.
memory_read.rs:93-100usesHAVING namespace > cursorand does not filter/order byMAX(updated_at). Once a namespace sorts before the cursor, later changes to it are invisible to that incremental traversal.
Verification: complete diff and surrounding auth/storage/migrations reviewed; GitHub checks and diff check pass; focused cursor and delegate-key tests pass. DB-backed migration and handler tests could not be run without DATABASE_URL.
|
Additional review findings on current HEAD ( 6. High — 7. Medium — 8. Medium — Namespace response missing 9. Low — Memory items don't expose Regarding existing findings #1–#5 from your earlier reviews: I verified them against the current HEAD and they all still apply. #1 (testnet gRPC) and #4 (migration rolling-deploy race) remain the highest priority blockers before merge. |
…g JSON-RPC-sunset migration
…n statement in 010, before 011's backfill
…laim on deletion visibility
|
Addressed the contract-clarification blockers in 1-3. Durable checkpoint / one-page / final-page / empty-sync behavior — 4. Same- 5. Previously-seen namespace resurfacing on later change — fixed. 6. Deletion propagation — confirmed not implemented, and no longer overclaimed in the docs. Agreed this is a contract change (breaking: namespace ordering + cursor wire format) and should get Console-team sign-off before we consider this mergeable — not just a code review pass. Requested acceptance tests: one-page-then-insert, update-after-checkpoint, same- |
…pdated_after, exclude from recall
Henry's WALM-295 contract review asked whether deletion propagation is
in scope; it wasn't. forget() (and the reactive Walrus-404 cleanup) now
stamps deleted_at + bumps updated_at instead of hard-deleting, so a
client polling GET /v1/owners/{owner}/memories sees status: "deleted"
on its next incremental sync instead of the row silently vanishing.
Namespace rollups still advance their updated_at watermark on a
deletion (so the namespace resurfaces) but exclude deleted rows from
memory_count/storage_used.
Also closes a gap the soft-delete refactor introduced: search_similar,
fetch_plaintext_by_blob_id, and get_blobs_by_namespace had no
deleted_at filter, so a "forgotten" memory's content remained fully
recallable via /api/recall, /api/recall/manual, and /api/ask despite
the read API reporting it as deleted. All three now exclude
soft-deleted rows, matching the filter already applied to
namespace_stats.
Documents the new deletion-visibility contract and the
snapshot_version client contract (any version mismatch -> discard
cursor, full resync) in docs/api/memory-read-api.md, and adds the
empty-incremental-sync regression test flagged as outstanding in the
PR #537 review thread.
Style Guide AuditAudited 1 file(s) against the Sui Documentation Style Guide. 46 violation(s) found. All must be fixed before merge.
|
Re-review — WALM-295Traced all 5 acceptance criteria plus the migration lock-splitting and provenance-plumbing claims against the diff (not the description). Approve — this is genuinely correct, well-reasoned work, but there are two things to close before merge: Verified:
To close before merge:
Depth: targeted (data-layer + auth-reuse changes; migrations warranted the closer read given they touch a live table). |
Summary
Adds the owner-scoped, cursor-paginated read API Console syncs from (WALM-295):
GET /v1/owners/{owner}/namespaces— rollup (memory count, storage used) per namespace, paginated.GET /v1/owners/{owner}/memories— keyset pagination on(owner, updated_at, id), includesagent_id/package_id/statusper memory.GET /v1/owners/{owner}/agents— live on-chain read of the account's delegate keys, short-TTL cached.All three reuse the existing signature-verified auth (
/api/restore's pattern) — the{owner}path segment must match the authenticated identity or the request gets403.Also plumbs
agent_id/package_idinto all 5 real write paths ofinsert_vector(remember, remember-manual/analyze, restore recovery) so the newmemoriesresponse has real provenance data, not just nulls.Migration
New migration set
010–013onvector_entries(addsupdated_at,agent_id,package_id, and a(owner, updated_at, id)index). Split across 4 separate transactions rather than one, so the backfillUPDATEonly needs aROW EXCLUSIVElock (doesn't block reads) instead of holdingACCESS EXCLUSIVEacross the whole backfill.Design doc / plan
docs/superpowers/specs/2026-08-04-memory-read-api-design.mddocs/superpowers/plans/2026-08-04-walm-295-memory-read-api.mddocs/api/memory-read-api.md— Console-facing contract (response shapes + auth headers)Test plan
cargo test --bins— 429 passed, 0 failed, 30 ignoredcargo build --bins/cargo fmt --check— cleandocker compose -f services/server/docker-compose.yml) throughout — migrations apply cleanly, keyset pagination confirmed gap/duplicate-free including a forcedupdated_attie, planner confirmed index usage (no full-table scan)Follow-ups (non-blocking, tracked here for visibility)
AppState.delegate_keys_cache(the new in-process cache backing/agents) has no eviction/cap — grows unbounded, keyed by every distinct owner that calls/agentsover the server's lifetime. Slow memory growth only, not a correctness issue. Needs a periodic sweep or bounded/LRU cache.docs/api/memory-read-api.md's 429 example uses illustrativelayer/retry_after_secondsvalues that don't match the real constants inrate_limit.rs(real layers are snake_case likeaccount_burst, fixed at 60s/300s). Doc-only, no functional impact.Scope note
WALM-296 (per-memory expiry:
end_epoch/expires_at) extends the samememoriesresponse this PR creates and is tracked as a separate follow-on PR — not included here.