Skip to content

feat(storage): migrate Mech adapter to SDK CAS - #27

Open
dundas wants to merge 10 commits into
mainfrom
feat/mech-storage-sdk-cas
Open

feat(storage): migrate Mech adapter to SDK CAS#27
dundas wants to merge 10 commits into
mainfrom
feat/mech-storage-sdk-cas

Conversation

@dundas

@dundas dundas commented Aug 16, 2026

Copy link
Copy Markdown
Owner

Summary:

  • migrate the tracked Mech Storage adapter to SDK v0.3.2 CAS and list primitives
  • fence inbox pull, ack, and nack with opaque per-claim lease tokens
  • fix the CLI nack payload and document the updated contract

Validation:

  • memory-backed server suite: 180 pass, 2 skipped
  • SDK adapter suite: pass
  • CLI build and tests: 39 pass, 4 live integration skips
  • secret scan: zero findings
  • independent AI reviews: clean after fixes

Deployment gates:
Production rollout is intentionally not included. It requires a valid non-production Mech app and reviewed retention/delete contract, plus Fly deployment authentication. No credentials are included in this PR.

dundas added 10 commits August 16, 2026 16:39
- Use the supported Mech Storage SDK with CAS, pagination, and tombstones

- Add atomic claims and lease-token fencing across HTTP and CLI

Implements tasks 2.0 through 4.0 from tasks/tasks-0005-prd-mech-storage-sdk-cas-migration.md
Covers stale lease-token acknowledgement before requeue.
Use CAS for single-use key burn and paginate key lookups.
@claude

claude Bot commented Aug 16, 2026

Copy link
Copy Markdown

Review: Mech adapter migration to SDK CAS + lease-token fencing

Solid piece of work overall — the CAS-based conditional-transition pattern (transitionDocument) is used consistently for claim/ack/nack/requeue, the concurrency tests (concurrent pulls conditionally claim..., stale or missing lease tokens cannot mutate..., key-burn race, group-member race) actually exercise the races they claim to fix, and the docs (AGENT-GUIDE.md, API-REFERENCE.md, CLI-REFERENCE.md, ERROR-CODES.md) were updated alongside the behavior change per the repo's contribution guidance. A few things worth a look before this ships:

1. claimNextMessage scans the entire admp_messages collection on every pull (perf)

src/storage/mech.jsclaimNextMessage():

const candidates = (await this.listDocuments('admp_messages'))
  .filter(message => message.to_agent_id === agentId && message.status === 'queued' ...)

listDocuments() paginates through the whole collection (100/page) with no server-side filter by to_agent_id or status. Since pull is the hottest path in the protocol (long-polling clients hit it repeatedly), this makes claim latency scale with total system-wide message volume, not per-agent inbox size — a regression relative to memory.js, which keeps a per-agent index (this.inboxes.get(agentId)) for O(inbox size) lookups. Notably, the same file does build a proper index elsewhere (admp_api_key_hashes for getIssuedKeyByHash), so the pattern to fix this already exists in the codebase — an equivalent to_agent_id-scoped index/collection for queued messages would avoid the full scan on the primary hot path.

2. createDocument masks non-conflict errors as "already exists"

src/storage/mech.jscreateDocument():

const existing = await this.nosql.cas.getDocument(collection, documentKey);
if (existing.ok && existing.document.data?._deleted) { ... }
const error = new Error(`Mech document already exists: ${collection}/${documentKey}`);
error.code = result.code;
throw error;

Any createDocument failure that isn't a revivable tombstone — auth failure, validation error, transient SDK/network error — gets reported with the message "already exists" (only error.code carries the real result.code). Since this path backs createAgent, createMessage, createTenant, createGroup, createIssuedKey, createOutboxMessage, and createRoundTable, a real auth/config problem at startup or under load will surface as confusing "document already exists" errors in logs, which will slow down triage.

3. Public repo now ships a private-registry dependency

.npmrc adds @mech:registry=https://registry.mechdna.net/api/packages/mech/npm/ and package.json/bun.lock pin @mech/storage-sdk@0.3.2 from that same registry. mech.js was previously .gitignore'd with the comment "Proprietary storage backend" — this PR removes that exclusion and commits the adapter plus the private registry reference directly into the public repo. Two things worth confirming explicitly before merge, given CLAUDE.md's instruction to avoid "private environment details" in this public repository:

  • Is @mech/storage-sdk licensed for redistribution/reference from a public OSS repo?
  • Does registry.mechdna.net allow anonymous/unauthenticated npm install/bun install? If not, external contributors and any public CI (not just this repo's own deploy pipeline) will fail to install dependencies out of the box.

Minor notes

  • LEASE_TOKEN_MODE=compat (in inbox.service.js#resolveLeaseToken) intentionally bypasses the new fencing by trusting the currently-stored lease_token when the caller omits one — this is a documented, opt-in migration escape hatch and defaults to required, so it's fine, just flagging that it's a real bypass and should stay off in production per the .env.example comment.
  • transitionDocument's retry backoff caps at Math.min(2 ** attempt, 25)ms over only 3 attempts by default — quite tight headroom under real contention, especially since claimNextMessage calls it once per candidate in a loop. Worth keeping an eye on CAS-conflict rates in production as the PRD's own risk table anticipates.

Nice to have

  • No coverage yet for the LEASE_TOKEN_MODE=compat fallback path itself, only the required (default) behavior.

Nothing above looks blocking on correctness — the fencing/CAS logic itself checks out against the tests — but #1 and #2 seem worth addressing (or explicitly deferring with a tracked follow-up, similar to the existing TODO on the 1000-row list cap) before this goes to production traffic.

@claude

claude Bot commented Aug 16, 2026

Copy link
Copy Markdown

Review: feat(storage): migrate Mech adapter to SDK CAS

Solid piece of work overall — the CAS-based concurrency model is well thought out (compare-and-swap document updates, atomic claimNextMessage, single-use key burning, opaque per-claim lease tokens), and it is backed by targeted concurrency tests in both src/server.test.js and src/storage/mech.test.js (concurrent pulls, stale-token fencing, parallel group-member CAS retries). A few things worth a look before merge:

Possible issues / behavior changes

  1. src/storage/mech.js — every list operation does a full, unfiltered collection scan. The request() helper listPath regex (^\/nosql\/documents\?collection_name=([^&]+)) strips off &limit=1000 and routes to the fully-paginating listDocuments(), which walks the entire collection regardless of the caller intended limit. That means claimNextMessage — called on every single pull — fetches and client-side-filters all messages for all agents, not just the target agent inbox. Same pattern applies to getInbox, listAgents, getAgentByDid, expireLeases, expireMessages, cleanupExpiredMessages, purgeExpiredEphemeralMessages, getStats, etc. This is a real hot-path scalability concern once the messages/agents collections grow, worth tracking even if it is an accepted trade-off from the PRD (FR-5 talks about paginating for correctness, not scoped querying).

Related: the TODO comment above purgeExpiredEphemeralMessages (around src/storage/mech.js line 1343) claims a "limit=1000 cap" that "will not process messages beyond the 1000th in a single sweep" — but that is no longer accurate given the full-pagination behavior described above. Worth updating or removing so it does not mislead future readers.

  1. Ack/nack lease-expiry semantics tightened, not just fenced. Both MemoryStorage.updateLeasedMessage and MechStorage.updateLeasedMessage require lease_until > Date.now() at mutation time, not just a matching token plus status === "leased". Previously, a legitimate-but-late ack (for example slow processing that crosses the visibility timeout, but no other worker has reclaimed the message yet) would still succeed. Now it fails with 409 LEASE_FENCED purely due to wall-clock timing, even with zero contention. This is arguably the more correct fencing behavior and is deliberately covered by a test, but it is a client-visible behavior change worth calling out explicitly in the docs beyond "stale or reclaimed" (docs/API-REFERENCE.md / llms.txt currently describe LEASE_FENCED only in terms of reclaim/staleness, not simple expiry).

  2. Inconsistent error code ordering between ack and nack (src/services/inbox.service.js). ack() calls resolveLeaseToken() unconditionally before touching storage, so a missing token surfaces 409 LEASE_TOKEN_REQUIRED regardless of message status. nack() checks message.status !== "leased" first and throws LEASE_FENCED before ever looking at whether a token was supplied, so a nack with a missing token against an already-acked or queued message reports LEASE_FENCED instead of LEASE_TOKEN_REQUIRED. Minor, but it means the same client mistake (forgetting --lease-token) surfaces different, less actionable error codes depending on which endpoint is called.

  3. Previously-proprietary adapter is now committed to the public repo. .gitignore used to explicitly exclude src/storage/mech.js with the comment "Proprietary storage backend," and src/storage/index.js used to say it was "injected at deploy time via the agentdispatch-deploy overlay" and "not included in the public repository." This PR removes that exclusion and commits the full 1114-line adapter. That may well be an intentional, sanctioned change (no secrets appear to be hard-coded, MECH_API_KEY/MECH_APP_ID are read from env), but given CLAUDE.md guidance about this being a public repo and not committing private implementation details, it would be good to get explicit confirmation that open-sourcing this adapter (including the storage.mechdna.net default host) is intended rather than an incidental side effect of the SDK migration.

Test coverage

Good coverage on the server/storage side for the new concurrency/fencing behavior. The CLI side (cli/src/commands/ack.ts, cli/src/commands/nack.ts) adds a --lease-token required option and changes printMessage() envelope/body resolution (cli/src/output.ts), but neither cli/src/output.test.ts nor cli/test/integration.test.ts was updated to exercise these paths. Worth adding at least a unit test for printMessage rendering lease_token and the envelope.envelope?.body ?? envelope.body fallback, since that is a plain logic change that is easy to regress silently.

Nice touches worth calling out

resolveLeaseToken LEASE_TOKEN_MODE=compat escape hatch is a sensible, opt-in, well-documented way to stage the breaking CLI/API change without a hard cutover. burnSingleUseKey and getIssuedKeyByHash hash-index-with-fallback are nice, focused fixes bundled into the migration. The .env.example and docs updates (AGENT-GUIDE.md, API-REFERENCE.md, ERROR-CODES.md, llms.txt) are thorough and match the implemented status codes/error codes.

Nothing here looks blocking on its own, but it would be worth getting explicit sign-off on point 4 (making mech.js public) before merging, and a decision on whether point 2 stricter expiry semantics is intended production behavior.

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.

1 participant