Skip to content

feat(agent-memory): Phase 7 — discovery marker#255

Open
jamby77 wants to merge 4 commits into
feature/agent-memory-phase6-consolidatefrom
feature/agent-memory-phase7-discovery
Open

feat(agent-memory): Phase 7 — discovery marker#255
jamby77 wants to merge 4 commits into
feature/agent-memory-phase6-consolidatefrom
feature/agent-memory-phase7-discovery

Conversation

@jamby77

@jamby77 jamby77 commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #254 (Phase 6 — consolidate).

What

Phase 7 of @betterdb/agent-memory: a discovery marker so BetterDB Monitor auto-discovers memory stores on any Valkey it watches, reusing the shared registry protocol from agent-cache.

  • MemoryDiscovery registers a marker on __betterdb:caches keyed by store name: type: 'agent_memory', prefix, version, protocol_version, capabilities: ['recall','consolidate','reinforce'], stats_key: {name}:__mem_stats, plus started_at/pid/hostname.
  • register() sets __betterdb:protocol with NX; heartbeats then run on an unref'd interval, refreshing a TTL'd __betterdb:heartbeat:{name} key and the marker (the NX protocol set is not re-issued per tick — it's a no-op after the first).
  • A cross-type marker collision is defensive only: memory registers under field {name}:mem, distinct from agent-cache's {name}, so the two never collide here. If a foreign-type marker is ever found in this field it emits a visible warning and proceeds last-writer-wins (rather than throwing into a swallowed promise); a same-type marker is overwritten.
  • MemoryStore gains an opt-in discovery option (registers on construct) and close() to stop the heartbeat and delete the marker.

Design / reuse notes

  • The agent-cache DiscoveryManager uses a method-style Valkey client and hard-codes its cache type in the collision check, while MemoryStore is built on a .call-only client. Rather than refactor the merged manager (and its FakeClient tests), this reuses the protocol as the single source of truth — agent-cache now additively re-exports its discovery constants (REGISTRY_KEY, PROTOCOL_KEY, HEARTBEAT_KEY_PREFIX, TTL/interval, PROTOCOL_VERSION, MarkerMetadata) — and implements the register/heartbeat loop against .call, so Monitor reads the memory marker identically to a cache marker.
  • Registration is fire-and-forget (construction stays sync); any rejected registration can't leak as an unhandled rejection, and close() awaits readiness before teardown.
  • Best-effort writes (an onWriteFailed hook is in place for the Phase 9 observability wiring).

Tests

  • discovery.test.ts (8): marker shape/capabilities/stats_key · protocol NX + heartbeat TTL · cross-type collision warns and overwrites (last-writer-wins) · same-type overwrite · stop deletes heartbeat · tickHeartbeat re-writes · interval fires (fake timers) · best-effort on write failure.
  • MemoryStore.discovery.test.ts (2): opt-in registers on construct + close() tears down · disabled → no registry I/O.

@betterdb/agent-cache unchanged behaviorally (253/253 still green after the additive export). agent-memory: 65/65 green · tsc clean · prettier clean.


Note

Low Risk
Best-effort optional discovery metadata on Valkey; no changes to recall/remember/consolidate paths or agent-cache runtime behavior.

Overview
Adds opt-in Valkey discovery for @betterdb/agent-memory so BetterDB Monitor can find memory stores the same way it finds agent caches.

MemoryDiscovery writes an agent_memory marker to the shared __betterdb:caches registry (field {name}:mem, separate from agent-cache’s {name}), sets __betterdb:protocol once with NX, and runs an unref’d heartbeat that refreshes a TTL’d heartbeat key and re-writes the marker. Cross-type collisions warn and last-writer-wins; registry/heartbeat I/O is best-effort (onWriteFailed hook for later observability).

MemoryStore accepts discovery?: boolean | MemoryDiscoveryConfig, registers on construct (fire-and-forget, sync constructor), and close() awaits registration then stops heartbeats and deletes the heartbeat key.

@betterdb/agent-cache only additively re-exports discovery protocol constants (REGISTRY_KEY, PROTOCOL_KEY, heartbeat TTL/interval, MarkerMetadata, etc.)—no behavior change to existing discovery.

New unit tests cover marker shape, protocol/heartbeat behavior, stop/teardown ordering, and MemoryStore wiring when discovery is on vs off.

Reviewed by Cursor Bugbot for commit c83f1e6. Bugbot is set up for automated code reviews on this repo. Configure here.

@jamby77 jamby77 force-pushed the feature/agent-memory-phase6-consolidate branch from 5cdeb4c to c317978 Compare June 18, 2026 06:57
@jamby77 jamby77 force-pushed the feature/agent-memory-phase7-discovery branch from 524f3a8 to e58dbb7 Compare June 18, 2026 06:57
@jamby77 jamby77 force-pushed the feature/agent-memory-phase6-consolidate branch from c317978 to d5a95b9 Compare June 18, 2026 07:19
@jamby77 jamby77 force-pushed the feature/agent-memory-phase7-discovery branch from e58dbb7 to 0d89d67 Compare June 18, 2026 07:19
@jamby77 jamby77 force-pushed the feature/agent-memory-phase6-consolidate branch from d5a95b9 to e27477f Compare June 18, 2026 07:28
@jamby77 jamby77 force-pushed the feature/agent-memory-phase7-discovery branch from 0d89d67 to 73d91d7 Compare June 18, 2026 07:28

@KIvanow KIvanow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Phase 7 is cleanly additive: agent-cache stays behaviorally identical, discovery is opt-in, the {name}:mem field namespacing is a nice touch, and close() awaiting discoveryReady before teardown is the right shape. One thing I would like reworked, plus two minors:

1. The cross-type collision protection is both unreachable for the case the PR cites and silent if it ever fires. The description says it "detects a name collision with a different cache type and throws," but:

  • Memory registers under field {name}:mem while agent-cache registers under {name} (see agent-cache/src/discovery.ts:210). Those are different hash fields, so a memory-vs-cache same-name collision can never land on the same field and checkCollision never sees it. The only way the check fires is if something wrote a non-agent_memory type into {name}:mem, which nothing in the codebase does.
  • Even if it did fire, the throw is swallowed twice: registration is fire-and-forget with ready.catch(() => undefined), and close() does await this.discoveryReady.catch(() => undefined). No caller path observes it, so the store just constructs and runs with the marker silently unwritten. Agent-cache at least logs on collision/overwrite. Please route a collision through onWriteFailed or a visible warning, and align the PR wording with what actually happens.

2. (minor) require('../package.json') runs at module load for every importer of MemoryStore, even with discovery disabled. It is a top-level const, so every consumer pays a disk read on import and inherits a bundler hazard (package.json is not always emitted). Reading it lazily inside createDiscovery scopes the cost to discovery users.

3. (minor) tickHeartbeat re-issues SET PROTOCOL_KEY ... NX on every interval, which is a guaranteed no-op after the first tick and can be dropped from the heartbeat path. The HGET to HSET collision check is also non-atomic (TOCTOU), which is low-stakes for best-effort discovery but worth a comment.

@jamby77 jamby77 force-pushed the feature/agent-memory-phase6-consolidate branch from e27477f to 6864f6b Compare June 19, 2026 09:51
@jamby77 jamby77 force-pushed the feature/agent-memory-phase7-discovery branch from 73d91d7 to 8bc0c13 Compare June 19, 2026 09:51
@jamby77

jamby77 commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator Author

@KIvanow Reworked in 8bc0c13: (1) a cross-type collision now emits a visible console.warn and proceeds last-writer-wins instead of throwing into a swallowed promise — and the PR description is corrected to say the check is purely defensive (memory registers under {name}:mem, distinct from agent-cache's {name}, so the two never collide here); (2) package.json is read lazily inside createDiscovery; (3) dropped the redundant SET protocol NX from every heartbeat tick and noted the best-effort HGET→HSET TOCTOU.

@jamby77 jamby77 requested a review from KIvanow June 19, 2026 09:52

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 8bc0c13. Configure here.

Comment thread packages/agent-memory/src/discovery.ts
@jamby77 jamby77 force-pushed the feature/agent-memory-phase6-consolidate branch from 6864f6b to 3ee71c8 Compare June 19, 2026 10:01
@jamby77 jamby77 force-pushed the feature/agent-memory-phase7-discovery branch from 8bc0c13 to 9f8485a Compare June 19, 2026 10:01
@jamby77 jamby77 force-pushed the feature/agent-memory-phase6-consolidate branch from 3ee71c8 to 1b71d3e Compare June 19, 2026 13:10
@jamby77 jamby77 force-pushed the feature/agent-memory-phase7-discovery branch from 94d2a7d to 66c0809 Compare June 19, 2026 13:10
jamby77 added 4 commits June 19, 2026 16:32
- MemoryDiscovery registers an agent_memory marker on the shared
  __betterdb:caches registry: type 'agent_memory', prefix, version,
  capabilities ['recall','consolidate','reinforce'], stats_key
- Reuses the agent-cache discovery protocol constants (additively
  re-exported) so Monitor reads memory markers identically; writes via
  MemoryStore's .call client (no method-style client needed)
- Heartbeat on an unref'd interval with a TTL'd heartbeat key; best-effort
  writes; name-collision detection against a different cache type
- MemoryStore gains an opt-in discovery option and close() to stop the
  heartbeat and remove the marker
- Re-export discovery constants/MarkerMetadata from @betterdb/agent-cache
…:mem

Register the agent_memory marker under the {name}:mem registry field and
heartbeat key so a memory store and an agent-cache sharing the same name
no longer collide on the same __betterdb:caches field / heartbeat key
(reported on the AgentMemory facade, which discovers both tiers).
…at work

- a cross-type marker collision now emits a visible console.warn and proceeds
  last-writer-wins, instead of throwing into a swallowed registration promise
  (the memory marker lives under {name}:mem, so it never collides with
  agent-cache's {name} field — the check is purely defensive)
- read package.json lazily inside createDiscovery so non-discovery importers
  don't pay a disk read (and avoid the bundler-emit hazard) at module load
- drop the redundant SET protocol NX from every heartbeat tick (no-op after
  register); note the best-effort HGET->HSET TOCTOU
…tes it

stop() cleared the interval and DEL'd the heartbeat but didn't wait for a tick
already running, so a tick that fired just before close() could re-write the
heartbeat/marker after the DEL and make the store look alive post-shutdown.
Track the in-flight tick and await it before deleting.
@jamby77 jamby77 force-pushed the feature/agent-memory-phase7-discovery branch from 66c0809 to c83f1e6 Compare June 19, 2026 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants