Skip to content

feat(server): Redis backend for the idempotency store#1855

Merged
bokelley merged 5 commits into
mainfrom
bokelley/redis-backends
May 19, 2026
Merged

feat(server): Redis backend for the idempotency store#1855
bokelley merged 5 commits into
mainfrom
bokelley/redis-backends

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

Adds redisBackend alongside memoryBackend and pgBackend. Redis is a natural substrate for AdCP v3's idempotency cache — SET … NX EX maps directly to the putIfAbsent claim semantic, and key expiry is enforced by the engine without a sweeper job.

import { createClient } from 'redis';
import { createIdempotencyStore, redisBackend, createAdcpServer } from '@adcp/sdk/server';

const redis = createClient({ url: process.env.REDIS_URL });
redis.on('error', err => console.error('redis error', err));
await redis.connect();

createAdcpServer({
  idempotency: createIdempotencyStore({ backend: redisBackend(redis), ttlSeconds: 86400 }),
  // ...
});

Design decisions worth reviewing

  • Client shape: RedisClientType<any,any,any> | RedisLikeClient union. Node-redis users pass createClient(...) straight in — no as unknown as casts. The narrow RedisLikeClient interface (4 methods) stays available for ioredis/Upstash/test doubles, with a worked ioredis adapter example in the JSDoc. RedisClientType is imported via import type (erased at emit), so redis stays a true optional peer dep with zero runtime coupling.
  • expired vs miss parity. Postgres rows linger past expires_at until cleanup; Redis evicts at the second TTL hits. The backend holds keys alive expiredGraceSeconds (default 120s — store's default 60s clock skew + 60s margin) past expiresAt so the store layer can still return IDEMPOTENCY_EXPIRED within the skew window. Buyer-observable semantics match the Postgres backend.
  • Reclaim semantics. A crashed in-flight claim is naturally reclaimable on retry — Redis auto-deletes expired keys, so SET … NX EX just succeeds. No WHERE expires_at < NOW() dance like pg needs.
  • keyPrefix default "adcp:idem:". Fine for a dedicated Redis or a dedicated db index. The JSDoc explicitly calls out the same-default-different-deployments collision risk and recommends per-deployment prefixes when sharing a Redis db (the principal segment is per-tenant, not per-deployment).
  • ttlFor throws on past expiresAt rather than silently clamping to EX 1 (which would let the entry vanish in 1s and let the next caller re-execute the side effect).
  • Error hygiene. Probe and corrupt-value errors attach the underlying detail to Error.cause; public messages are generic to avoid leaking infra shape (ECONNREFUSED 10.0.x.x, WRONGPASS …) or scoped keys (which contain the principal) into adopter-facing error bodies.
  • clearAll intentionally omitted. Same rationale as pg — a shared Redis is a production resource; an accidental compliance-reset FLUSHDB would nuke unrelated keys. Tests run against a dedicated db index (REDIS_URL=…/15) and call FLUSHDB themselves.

Pre-PR review

Ran in parallel through DX, ad-tech-protocol, code-reviewer, and security-reviewer agents. Folded in all fix-before-merge findings:

  • DX caught that the original RedisLikeClient-only signature didn't accept a real createClient() result (node-redis's set is an overloaded RedisCommandSignature, not structurally assignable). Fixed via the union type above; verified with an out-of-tree typecheck smoke.
  • Code review caught the silent Math.max(1, …) clamp in ttlFor and a missing reclaim-expired test. Both fixed.
  • Security caught two error-message leak vectors (probe + corrupt-value). Both fixed with Error.cause.
  • Protocol expert flagged the cross-deployment keyPrefix collision risk. JSDoc tightened.

Two security findings deferred as pre-existing protocol-store issues (not Redis-introduced regressions): cached responses can contain credentials if a handler returns them (#3), and cache-fill DoS via unbounded distinct keys (#4). Will file follow-up issues.

Test plan

  • tsc --noEmit clean
  • npm run build:lib clean
  • npm run format:check clean
  • 17 new Redis integration tests pass against local Redis (REDIS_URL=redis://127.0.0.1:6379/15)
  • 93/93 idempotency tests pass overall (17 Redis + 76 pre-existing)
  • DX smoke: out-of-tree redisBackend(createClient(...)) typechecks without cast; deliberate type error (passing a string) is correctly rejected by tsc
  • CI green
  • Manual review of the expiredGraceSeconds = 120 default (tied to the store's default 60s clockSkewSeconds + 60s margin — if anyone ever raises the store-level skew, this default needs to keep up; JSDoc says so)

🤖 Generated with Claude Code

Adds redisBackend alongside memoryBackend and pgBackend.
SET … NX EX maps directly to the putIfAbsent claim semantic and key
expiry is enforced by the engine — no sweeper job needed.

- Accepts RedisClientType<any,any,any> | RedisLikeClient. Pass
  createClient(...) from `redis` straight in (no cast); ioredis/Upstash
  implement the 4-method RedisLikeClient interface (worked example in
  JSDoc). RedisClientType is import-type-only so `redis` stays a true
  optional peer dep with zero runtime coupling.
- Holds entries alive expiredGraceSeconds (default 120s) past expiresAt
  so the store layer can still distinguish IDEMPOTENCY_EXPIRED from a
  fresh miss within the clock-skew window — same buyer-observable
  semantics as the Postgres backend.
- keyPrefix defaults to "adcp:idem:"; JSDoc calls out the
  same-default-different-deployments collision risk and recommends
  per-deployment prefixes when sharing a Redis db.
- ttlFor throws on a past expiresAt rather than silently clamping to
  EX 1 (which would let the entry vanish in 1s and let the next caller
  re-execute the side effect).
- Probe and corrupt-value errors carry the underlying detail on
  Error.cause; public messages are generic to avoid leaking infra
  shape or scoped keys (which contain the principal).
- clearAll intentionally omitted — same rationale as pgBackend.

17 integration tests in test/lib/idempotency-redis.test.js (skipped
without REDIS_URL): probe, get/put round-trip, putIfAbsent claim +
concurrent race + expired-entry reclaim, ttlFor-rejects-past, keyPrefix
isolation, corrupt-value surfacing with Error.cause + no key leak,
store-level conflict/expired round-trips, JSON unicode/nesting.

redis ^4.6.0 || ^5.0.0 added as optional peer dep (same shape as pg).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a process-once console.warn at backend construction when the default
keyPrefix ("adcp:idem:") is paired with a node-redis client we can
confidently identify as being on db 0 — the strong signal of a
shared/non-dedicated Redis where the default prefix is most likely to
collide with another AdCP deployment. The principal segment is
per-tenant, not per-deployment, so it can't carry deployment-level
isolation on its own.

Behavior:
- Best-effort introspection via detectNodeRedisDbIndex(client.options).
- Stays silent for escape-hatch clients (ioredis, Upstash, test doubles)
  where we can't read the db index — prefer false-negative over noise.
- Stays silent when keyPrefix is set explicitly, even to the default
  value (treats explicit-pass as a deliberate informed choice).
- Stays silent when suppressDefaultPrefixWarning: true is passed.
- Fires at most once per process across all backend instances.

11 new tests in the existing test file run without REDIS_URL — they
exercise the introspection / once-warn / suppression / escape-hatch
matrix against stub client objects, no live Redis needed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley

Copy link
Copy Markdown
Contributor Author

Folded in feedback from review.

Added (commit 1beaa5a): one-time console.warn at redisBackend construction when the default keyPrefix is used against a node-redis client we can confidently identify as being on db 0. Best-effort introspection via client.options.database or parsed from options.url. Stays silent for escape-hatch clients (ioredis, Upstash, test doubles) where we can't read the db index, stays silent when keyPrefix is set explicitly (even to the default value), and stays silent with suppressDefaultPrefixWarning: true. Fires at most once per process across all backend instances. 11 new tests cover the matrix, no live Redis needed for them.

Filed as follow-up issues (deferred from this PR's security review — both are pre-existing protocol-store issues, not Redis-introduced regressions):

ReplayStore + AdcpStateStore on Redis: ReplayStore is the near-mechanical port — same access pattern (set-with-TTL + check existence), reusable structure. AdcpStateStore needs more careful work for putIfMatch semantics on WATCH/MULTI/EXEC — the silent-data-corruption failure mode (lost writes) makes it categorically different from idempotency's clean "duplicate claim returns cached." Will file as separate issues once this lands.

…'s published .d.ts resolves

The Redis idempotency backend types its public surface as
`RedisClientType<any,any,any> | RedisLikeClient` so adopters can pass
`createClient(...)` from `redis` without `as unknown as` casts at every
call site. That DX win requires the published `.d.ts` to keep
`import type { RedisClientType } from 'redis'`, which the adopter
type-checker must resolve.

`scripts/check-adopter-types.ts` packs the SDK into a clean adopter
project and runs `tsc --noEmit` against the published types. Until
this commit, that scaffold installed `@types/express` and
`@opentelemetry/api` (transitive type references from the server
bundle) but NOT `redis` — so the new Redis backend's `import type`
failed with TS2307 in CI.

The fix follows the same precedent as `@types/express`: adopters who
use the Redis backend will have `redis` installed, and the adopter
check exists to validate the adopter experience, not to enforce
zero-peer-dep type resolution. Comment in the script now spells out
why we install `redis` here when `pg` is NOT installed (pg types via
project-local `PgQueryable`; Redis types via the union for DX).

Confirmed locally: `npm run check:adopter-types` now PASSes on this
branch.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley added a commit that referenced this pull request May 19, 2026
…e footgun

Two follow-ups to the initial PR push:

1. scripts/check-adopter-types.ts now installs `redis` so the new
   backends' published .d.ts (which references `RedisClientType` via
   the `RedisBackendClient` union) resolve in the adopter scaffold.
   Same precedent as `@types/express` already installed there. Without
   this, CI's adopter-types step fails with TS2307. Identical fix
   landed on PR #1855 (Redis idempotency); duplicated here so this
   branch self-contains.

2. JSDoc on ReplayRedisLikeClient names the ioredis.zscore
   string-vs-number footgun explicitly: ioredis returns
   Promise<string | null>; without Number() coercion in the shim, the
   `score > now` compare in has() becomes a string-vs-number that's
   silently wrong near unix-second boundaries (lexicographic
   ordering). The example already had Number(); now there's prose
   above it that names why. DX review surfaced this.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley bokelley merged commit bad9e22 into main May 19, 2026
10 checks passed
@github-actions github-actions Bot mentioned this pull request May 19, 2026
damianbedrock pushed a commit to damianbedrock/adcp-client that referenced this pull request May 23, 2026
Adds two more Redis-backed stores alongside the existing memory and
Postgres variants. Same shape as the idempotency Redis backend:
RedisClientType | <NarrowInterface> union so node-redis users pass
createClient(...) straight in without casts, `import type` keeps
`redis` an optional peer dep with zero runtime coupling.

redisCtxMetadataStore (server/ctx-metadata/backends/redis.ts) —
factory function matching the pg backend pattern. Stores one Redis
key per (account_id, kind, id) with the JSON payload
{ value, resource?, expiresAt? }. Entries with no expiresAt have no
Redis TTL (ctx-metadata lifetimes can be months; silent eviction
produces "package not found" errors that look like publisher bugs).
When expiresAt is set, EX = expiresAt - now + expiredGraceSeconds
(60s default) keeps the value visible to the store layer's own
expiry check within the clock-skew window. bulkGet uses MGET for a
single round trip per batch. clearAll intentionally omitted (same
rationale as idempotency).

RedisReplayStore (signing/redis-replay-store.ts) — class-based to
match PostgresReplayStore. One Redis sorted set per (keyid, scope),
scored by expiresAt, members are nonces. A single Lua script does
the atomic ZREMRANGEBYSCORE -inf now → ZSCORE → ZCARD → ZADD +
PEXPIREAT sequence, guaranteeing the three-way
'ok' | 'replayed' | 'rate_abuse' precedence (replay > rate_abuse >
ok) is observed by every caller exactly once even under heavy
concurrency. Has a probe() for boot-time readiness (not part of the
base ReplayStore interface). No sweeper needed — expired nonces drop
at every insert, abandoned sets evict via PEXPIREAT.

Both backends share a default-prefix warn at
src/lib/utils/redis-default-prefix-warn.ts — one-time console.warn
when the default keyPrefix meets a node-redis client we can
confidently see on db 0. Fires at most once per process across every
backend instance, stays silent for escape-hatch clients (ioredis,
Upstash) where we can't introspect db index, stays silent when
keyPrefix is explicit or suppressDefaultPrefixWarning: true. The
idempotency Redis backend (adcontextprotocol#1855) has the same logic inline; a
follow-up will dedupe onto this shared helper after both PRs land.

redis ^4.6.0 || ^5.0.0 added as optional peer dep (same shape as pg).
Out-of-tree DX smoke confirms node-redis createClient() result passes
to both backends without casts.

Tests (skipped when REDIS_URL not set):
- 11 warn-suite tests run unconditionally (pure introspection)
- 16 ctx-metadata live + 14 replay live integration tests verify
  Lua atomicity, cap precedence, expired reclaim, keyPrefix
  isolation, corrupt-value Error.cause hygiene, MGET single-round-
  trip.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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