feat: @adcp/client 5.0 + PostgresStateStore for cross-machine sessions#2263
Merged
Conversation
Bump @adcp/client 4.30.1 → 5.0.0 and back training-agent session state with the SDK's PostgresStateStore. Solves cross-machine persistence: the training agent's in-memory Map didn't survive the 2 Fly.io machines, so property lists / media buys / collection lists created on one machine couldn't be read on the other. - state.ts rewritten: async getSession(), AsyncLocalStorage per-request cache, serialize/deserialize SessionState to JSONB, flushDirtySessions() only writes sessions whose serialized shape actually changed. - Dispatcher wraps handler execution in runWithSessionContext and flushes on clean return — thrown exceptions discard half-mutated state. - All 48 tool handlers made async with await getSession(). Logic unchanged. - getAllSessions() cross-session fallback removed (was a no-op after the refactor and masked legitimate MEDIA_BUY_NOT_FOUND cases). - sessionKeyFromArgs caps brand.domain (253 chars, RFC-1035) and account_id (128 chars, alphanumeric+._-) — prevents unbounded adcp_state primary-key bloat from attacker-supplied values. - deserializeSession filters __proto__/constructor/prototype keys. - lastGetProductsContext.products dropped from persistence (deterministic from catalog); proposals still persist for the draft→finalize lifecycle. - 5 MB cap per session payload to bound JSONB row size. - Migration 397_adcp_state_store.sql via getAdcpStateMigration(). - TaskResult discriminated-union absorption in addie/mcp/member-tools.ts. - Cross-machine test added: serverA create → serverB read via the store. Tests: 298 training-agent + 587 tests/ suite, all passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
main has moved past 397 and 407; bump the new migration to 408. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Skip flushDirtySessions() when a handler threw mid-request. The dispatcher's catch block now calls markHandlerFailed() so the outer flush is a no-op; partial mutations are not persisted. Previously the finally-style flush ran even on thrown-error paths. - Remove unsafe `products: undefined as unknown as Product[]` cast in deserializeSession. `lastGetProductsContext.products` is now `Product[] | undefined` in SessionState; every consumer already handles the absent case via fallback-to-catalog. - Lowercase brand domain in session key derivation so Acme.com and acme.com share a session (DNS is case-insensitive). Prevents a user with mixed-case input from writing into a different session than a user with lowercase input for the same brand. Tests: - New: does not flush when handler failed mid-request (seeds state, triggers failed-mutation, verifies original state preserved). - New: lowercases brand domain so DNS casing does not fork sessions. - All 300 training-agent tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 17, 2026
Closed
Closed
…h signal
Three correctness bugs in the flush/snapshot machinery:
1. lastAccessedAt defeated the diff check. getSession() updates
lastAccessedAt on every load; serializeSession includes it; so
the flush-time serialization always differed from the load-time
snapshot. Every read wrote. Fix: stableStringify drops
lastAccessedAt from the comparison.
2. Snapshot used raw storedShape, not round-trip. If deserialize
then serialize normalized anything (optional keys, ISO precision),
the first flush of every session looked dirty. Fix: snapshot
serializeSession(session) after load, so the comparison is
against a canonical form.
3. Size cap silently dropped writes. A handler that pushed the
session past 5 MB returned 200 OK to the caller, and the flush
logged a warning and moved on — the data was gone. Fix:
flushDirtySessions throws on oversize. The MCP transport layer
surfaces the failure; operators notice.
Plus a structural fix:
4. Dispatcher return signal replaces markHandlerFailed flag. The
flag approach was fragile — a handler that caught internally
could still flush partial state. Now dispatchCallTool returns
{ result, flushable } and the outer wrapper unambiguously
decides. Thrown handlers set flushable=false; validation /
structured-error paths set flushable=true.
5. clearSessions() no longer duck-types clearCollection — uses
instanceof PostgresStateStore. Tests-only code; future stores
outside the two known implementations are a no-op (documented).
6. sessionKeyFromArgs emits a debug log when brand.domain is
rejected and collapses to open:default — makes malformed callers
visible without changing behaviour.
Tests:
- read-only access does not flush (monkey-patches store.put,
asserts zero writes after a pure-read request)
- snapshot matches round-trip serialization (same test setup,
load without mutation, verify no flush)
- dispatcher skips flush when handler throws (via real MCP
simulateCallTool path, not the removed markHandlerFailed helper)
302/302 training-agent tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bumps
@adcp/client4.30.1 → 5.0.0 and backs training-agent session state with the SDK's newPostgresStateStore. Solves cross-machine persistence: property lists / media buys / collection lists created on one Fly machine are now visible on the other.This unblocks
#2235,#2236, and#2237end-to-end (my prior PR fixed the handler bugs; persistence was the remaining gap). Also applies theTaskResultdiscriminated-union breaking change from 5.0.What changed
server/src/training-agent/state.tsrewritten:getSession(key)is now async and loads fromadcp_state(JSONB). Per-requestAsyncLocalStoragecache for multi-handler-call sharing.flushDirtySessions()writes only sessions whose serialized shape actually changed.runWithSessionContextand flushes only when the handler returns successfully (viaflushableflag in the dispatcher return value). Thrown exceptions skip the flush, discarding half-mutated state.asyncwithawait getSession(...). Logic unchanged.getAllSessions()cross-session fallback removed — was a no-op after the refactor and was masking legitimateMEDIA_BUY_NOT_FOUNDresponses.sessionKeyFromArgsvalidates input: capsbrand.domainat 253 chars / RFC-1035 charset andaccount_idat 128 chars / alphanumeric+._-. Lowercases domain so DNS casing doesn't fork sessions. Prevents unbounded primary-key bloat from caller-supplied values.deserializeSessionfilters__proto__/constructor/prototypemap keys.lastGetProductsContext.productsdropped from persistence (deterministic from catalog);proposalsstill persist for the draft→finalize flow.SessionState.lastGetProductsContext.productsis now properlyProduct[] | undefined.flushDirtySessionsthrows (no longer silently drops). Defense-in-depth on top of per-collection mutation limits.408_adcp_state_store.sqlviagetAdcpStateMigration().member-tools.ts: 4TaskResultFailuresites simplified (5.0 discriminated union guaranteeserror: string).serverAcreates →serverBreads via the shared store.Reviews
Ran code-reviewer, security-reviewer, and adtech-product-expert across two rounds; addressed Must Fix and Should Fix items:
Round 1:
__proto__pollution vector → key filtergetAllSessionssilent cross-session → removedsetStateStoreproduction guard addedRound 2:
lastAccessedAtdefeated the diff check (every read wrote) →stableStringifydrops it from the comparisonserializeSession(session)after loadflushDirtySessionsthrows on oversizemarkHandlerFailedwas fragile → dispatcher now returns{ result, flushable }so the outer wrapper unambiguously decides; internal handler catches don't silently flush partial stateclearSessionsduck-typedclearCollection→ now usesinstanceof PostgresStateStoreKnown limitation documented: concurrent requests against the same session key use last-writer-wins (acceptable for the sequential-storyboard sandbox; production sellers should use per-entity collections via
createAdcpServer).Test plan
tests/suite passnpm run typecheckclean@adcp/client@5.0storyboards againsttest-agent.adcontextprotocol.organd close Training agent: property_list and collection_list in package targeting accepted but not persisted #2235, Training agent: collection list CRUD is half-implemented (get/update/targeting fail) #2236Out of scope
createAdcpServerdomain-grouped migration — documented as a future refactor. This PR keeps the hand-rolled handler architecture and swaps only the persistence layer, which is the minimum change to unblock the storyboards.🤖 Generated with Claude Code