diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bf18341..3016be3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -54,10 +54,41 @@ jobs: - uses: codecov/codecov-action@fb8b3582c8e4def4969c97caa2f19720cb33a72f # v7 with: use_oidc: true - files: packages/core/coverage/coverage-final.json,packages/redis/coverage/coverage-final.json,packages/memcache/coverage/coverage-final.json,packages/sqlite/coverage/coverage-final.json,packages/nestjs/coverage/coverage-final.json + files: packages/core/coverage/coverage-final.json,packages/redis/coverage/coverage-final.json,packages/memcache/coverage/coverage-final.json,packages/sqlite/coverage/coverage-final.json,packages/nestjs/coverage/coverage-final.json,packages/otel/coverage/coverage-final.json flags: unit fail_ci_if_error: true + # Guards the `engines: >=20` floor the published packages advertise. The + # required `test` job pins Node 22, and the repo's own toolchain cannot run + # any lower (pnpm 11 requires >= 22.13), so this builds on 22 and then loads + # the built output under the oldest and newest supported runtimes the way a + # consumer would — plain `node`, no pnpm. Kept as its own job so the + # required check names stay stable. + compat: + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + node-version: ["20", "24"] + name: compat (node ${{ matrix.node-version }}) + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + - uses: pnpm/action-setup@0977fd99725f1db4007ccb2928dbb4e90d06cc86 # v6 + with: + version: 11.10.0 + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v6 + with: + node-version: "22" + cache: pnpm + - run: pnpm install --frozen-lockfile + - run: pnpm build + - name: Switch to Node ${{ matrix.node-version }} + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v6 + with: + node-version: ${{ matrix.node-version }} + - name: Load the built packages on Node ${{ matrix.node-version }} + run: node scripts/smoke-test.mjs + functional: runs-on: ubuntu-latest strategy: diff --git a/ARCHITECTURE_REVIEW.md b/ARCHITECTURE_REVIEW.md new file mode 100644 index 0000000..be94053 --- /dev/null +++ b/ARCHITECTURE_REVIEW.md @@ -0,0 +1,174 @@ +# Architecture Review — Ziggurat + +**Date:** 2026-08-19 +**Reviewer role:** Senior principal architect, full-implementation review +**Scope:** All six packages (`core`, `redis`, `memcache`, `sqlite`, `nestjs`, `otel`), docs, package manifests, CI, and repo tooling. +**Method:** Every source file read in full. Full build + test suite executed: **12/12 turbo tasks pass** (unit, contract, and integration suites). Claims below that depend on tooling behavior (e.g. Vitest 4) were verified against the installed dependencies, not assumed. +**Relation to prior review:** `CODE_REVIEW.md` (2026-06-11, v0.1.x) — nearly all of its findings are fixed in the current code. A status appendix is at the end. This document reviews the codebase as it stands at v0.2.0. + +> **Resolution status:** H1–H3, M1–M6, and L1–L6 were fixed in the follow-up branch `fix/architecture-review-findings`; the findings below are preserved as written at review time. Still open: **L7** (publishing the adapter contract suite as a test kit) and **L8** (API/perf polish), plus the five architectural observations, which are roadmap items rather than defects. + +--- + +## Executive Summary + +Ziggurat is in very good shape for a v0.2 library. The core abstraction (an ordered stack of `CacheAdapter`s orchestrated by a `CacheManager`) is clean, the stampede coalescing is implemented correctly (in-flight promise registered synchronously before any yield), the event system is genuinely zero-cost when unobserved, and the testing story — a shared 82-case contract suite run against every adapter plus functional tests against real Redis/Memcached backends in CI — is stronger than most published caching libraries. + +The most important problem found is not a crash bug but a **semantic contradiction between the code and its own documentation on backfill TTLs** — the library's central feature. Three docs promise "L1 always uses its own TTL policy" while the code gives backfilled entries the _source layer's remaining TTL_. One of the two is wrong, and the divergence was introduced by a deliberate fix (the TTL-precedence flip from the June review) whose ripple effects on backfill were never reconciled. + +Everything else is medium-or-lower: an injection-token collision waiting to happen in the NestJS package, a broken leftover Vitest workspace file, some sharp edges around empty Redis prefixes and read-repair races, and a handful of doc drift. + +--- + +## What's Working Well + +- **Layered orchestration is properly separated from storage.** `CacheManager` owns cross-layer policy (read-through order, backfill, write fan-out, coalescing, events); adapters own storage semantics. `BaseCacheAdapter` gives adapter authors sane defaults for the extended surface (`has`, `getTtl`, `mget`, `mset`, `mdel`) so a minimal adapter is four methods. +- **Stampede coalescing is correct.** The in-flight promise is created and registered inside the same synchronous continuation as the cache-miss check (`cache-manager.ts:220-244`), so there is no interleaving window between miss detection and registration. Factory errors propagate to all coalesced callers and the map entry is cleaned up in `finally`. +- **Observability design is right.** Events are typed, gated by `hasListeners` so an unobserved manager pays ~nothing, listener exceptions can't break cache operations, and the OTel package is a thin translator over the event stream that depends only on `@opentelemetry/api`. +- **Failure philosophy is coherent and documented.** Reads skip failing layers; writes are `allSettled` best-effort with an opt-in `strictWrites` escape hatch; `wrap()` always returns the factory value even when caching it fails. +- **Testing and CI are a real strength.** Shared contract suite (`core/tests/contract/adapter-contract.test.ts`) reused by every adapter, hermetic-by-default with functional suites behind explicit opt-in, real backends in a CI matrix, CodeQL/OSV/Semgrep/gitleaks, pinned action SHAs, Codecov via OIDC. +- **Security posture in the code itself:** SQL identifier validation for the SQLite table name, glob-escaping of the Redis prefix in SCAN patterns, no dynamic SQL beyond parameterized `IN` lists. +- **The June review was acted on thoroughly** — 10 of 12 findings fully fixed, including subtle ones (Memcached >30-day TTLs, pipeline error surfacing, decorator metadata preservation). + +--- + +## Findings + +### High + +#### H1. Backfill TTL semantics contradict the documentation (core feature) + +**Code:** `packages/core/src/base-cache-adapter.ts:48`, `packages/core/src/cache-manager.ts:90-99` and `:302-317` +**Docs contradicted:** `docs/core-concepts.md:58` and `:189-193`, `docs/advanced-usage.md:30-31`, `README.md:100` + +The manager computes the source entry's remaining TTL and passes it to the target layer as an **explicit** `ttlMs`. Since the June TTL-precedence flip, an explicit TTL beats `defaultTtlMs` (`ttlMs ?? this.defaultTtlMs`), so the target layer's own TTL policy is ignored during backfill — only `maxTtlMs` can cap it. + +The docs say the opposite, in three places: + +- `core-concepts.md:58`: _"If the target adapter has a `defaultTtlMs`, backfill uses that TTL. … This means L1 always uses its own TTL policy, regardless of L2's expiration."_ +- `core-concepts.md:189-193` ("TTL Resolution Order"): _"Adapter's `defaultTtlMs` (if set) — always wins"_ — this section still documents the **pre-flip** precedence for all TTL resolution, contradicting both the code and `api-reference.md` (which is correct). +- `advanced-usage.md:30-31` (the recommended two-layer pattern): _"On a Redis hit, memory is auto-backfilled with memory's own 30s TTL."_ + +**Real-world consequence:** a user who follows the recommended pattern — `MemoryAdapter({ defaultTtlMs: 30_000 })` over `RedisAdapter({ defaultTtlMs: 300_000 })` — gets L1 entries that live up to **5 minutes**, not 30 seconds. The L1 staleness budget silently becomes L2's. The README example only behaves as advertised because it happens to use `maxTtlMs` instead of `defaultTtlMs`. + +**Recommendation:** decide the semantics deliberately, then make code and docs agree. My recommendation is to make backfill honor the target layer's policy: pass the remaining TTL but clamp it, i.e. effective backfill TTL = `min(remainingTtlMs, target.defaultTtlMs ?? ∞, target.maxTtlMs ?? ∞)`. That matches the documented intent ("each layer manages its own TTL"), keeps the safety property that a backfilled entry never outlives the source entry, and keeps `maxTtlMs` meaningful. If instead the current behavior is the intent, all three doc passages plus the `core-concepts.md` TTL-resolution section need rewriting. Either way, add an integration test pinning the chosen behavior — nothing in the current suite catches this divergence, which is how the docs drifted. + +#### H2. NestJS injection token `"CACHE_MANAGER"` collides with `@nestjs/cache-manager` + +**Code:** `packages/nestjs/src/constants.ts:2` + +The token value is the bare string `"CACHE_MANAGER"` — the same string token the official `@nestjs/cache-manager` package uses. `ZigguratModule` also registers itself `global: true` unconditionally. An application using both (a very common state mid-migration, or when a third-party module pulls in Nest's cache) ends up with two global providers competing for one string token; which instance gets injected depends on module resolution order, and the failure mode is a silently wrong object at runtime (`this.cache.wrap is not a function`, or worse, Nest's manager quietly serving where Ziggurat was expected). + +**Recommendation:** change the token _value_ to something owned (`"ZIGGURAT_CACHE_MANAGER"`, or a `Symbol`); the exported constant name can stay `CACHE_MANAGER` so most consumers see no break. Do this before adoption grows — it's a breaking change for anyone who hardcoded the string, and it only gets more expensive. + +#### H3. `vitest.workspace.ts` is dead and broken under Vitest 4 + +**Code:** `vitest.workspace.ts` (repo root) + +The file calls `defineWorkspace` from `vitest/config`. Verified against the installed `vitest@4.1.10`: **that export no longer exists** (workspace files were deprecated in Vitest 3 and removed in 4). CI never notices because tests run per-package through turbo, so the file is simultaneously broken and unused — but any contributor who runs `vitest` from the repo root gets a confusing failure. + +**Recommendation:** delete it, or migrate to the `projects` field in a root `vitest.config.ts` if root-level test invocation is wanted. + +### Medium + +#### M1. Redis adapter's empty default prefix makes `clear()` a database-wide delete + +**Code:** `packages/redis/src/redis-adapter.ts:22`, `:108-119` + +`prefix` defaults to `""`, so `clear()` (and `flushAll()`, which inherits `clear()` via the base class) scans `MATCH *` and pipeline-deletes **every key in the Redis database**, including keys belonging to other applications. The corrupt-payload read-repair path has the same blast-radius property (a foreign, non-JSON key read through the adapter gets deleted). The docs warn about this clearly (`redis-adapter.md`), but a documented footgun with a dangerous _default_ is still a footgun — the safe configuration should be the default, not the recommendation. + +**Recommendation:** either require `prefix` (constructor throws on empty — breaking but honest), or make `clear()` throw on an empty prefix unless an explicit `allowUnprefixedClear: true` opt-in is set. (Memcached's global `flush()` is different: the protocol offers nothing better, and the docs say so loudly. That one is acceptable as-is.) + +#### M2. `wrap()` miss path blocks on writing every layer + +**Code:** `packages/core/src/cache-manager.ts:233` + +After the factory resolves, `wrap()` awaits `setLayers()` — writes to _all_ layers — before resolving. Every coalesced waiter shares that promise, so all of them wait too. This is inconsistent with the library's own latency philosophy: backfill is fire-and-forget by default (`syncBackfill: false`) precisely so a slow layer doesn't tax reads, yet a slow-but-alive Redis adds its full write latency to every `wrap()` miss for every coalesced caller. A dead layer is fine (fast rejection, `allSettled`); a degraded one (100–500 ms writes) directly inflates p99 on the hottest path in the library. + +**Recommendation:** return the factory value as soon as it's known and let the layer writes settle in the background (surfacing failures via `error` events, exactly like backfill), or gate the behavior on an option (`syncWrites`, defaulting to the current behavior for read-your-write safety). Whichever way, document the choice — today it's implicit. + +#### M3. Read-repair deletes race with concurrent writers + +**Code:** `packages/sqlite/src/sqlite-adapter.ts:114-117` and `:166-169`; `packages/redis/src/redis-adapter.ts:37-45`; `packages/memcache/src/memcache-adapter.ts:36-45` + +All three shared-storage adapters do lazy expiry/corruption cleanup as _read, then unconditionally delete by key_. Between the read and the delete, another process can write a fresh entry — which the delete then destroys. SQLite is the most exposed (multi-process WAL usage is an advertised use case, and the fix is trivial: add `AND expires_at IS NOT NULL AND expires_at <= ?` to the cleanup delete). Redis/Memcached need atomicity (Lua script / CAS) to fix properly, which is likely not worth it — but the race should be documented. + +Related: for Redis and Memcached the envelope's `expiresAt` is written with the **writer's** clock and enforced with each **reader's** clock. With clock skew between app instances, a skewed reader can treat valid shared entries as expired and _delete them for everyone_ — turning one machine's clock problem into a fleet-wide cache-miss problem. Redis already enforces TTL server-side via `PSETEX`; consider treating the envelope check as advisory (return a miss without issuing `DEL`) so a skewed reader only harms itself. + +#### M4. `mget` partial-failure semantics differ per adapter + +**Code:** `packages/core/src/base-cache-adapter.ts:81-90` vs `packages/redis/src/redis-adapter.ts:127-165` + +`BaseCacheAdapter.mget` uses `Promise.all` over individual `get()`s — one failing key rejects the whole batch, and the manager then skips that **entire layer** (`cache-manager.ts:272-284`). `RedisAdapter.mget` does the opposite: per-key errors are skipped and a partial map is returned. So the same partial-failure scenario produces different results depending on the adapter, and `api-reference.md` documents only Redis's behavior. The adapter contract (`types.ts:40`) is silent on which is correct. + +**Recommendation:** declare partial-result semantics as the contract (it composes better with the manager's shrinking-set loop), switch the base implementation to `allSettled`, and add a contract-suite case so all adapters are held to it. + +#### M5. `set(key, undefined)` does four different things on four adapters + +- Memory/reference: stored, but reads report a **miss** (node-cache can't distinguish "missing" from "stored undefined"). +- Memory/json: write skipped entirely — documented (`types.ts:200-206`). +- Redis/Memcache: `JSON.stringify({value: undefined, …})` drops the key — future reads are **hits with `value: undefined`**. +- SQLite: `JSON.stringify(undefined)` is `undefined` → better-sqlite3 rejects the bind → **throws**. + +**Recommendation:** pick one rule — "undefined is never stored; the write is a silent no-op" (the memory/json behavior) is the most defensible — apply it in `CacheManager.setLayers`/`wrap` once, centrally, and add a contract test. One central check is cheaper than four adapter fixes. + +#### M6. Published Node support is never tested + +Packages declare `"engines": { "node": ">=20" }` and the README promises Node ≥ 20, but CI runs everything on Node 22 only (`ci.yml`). The root repo requires Node ≥ 22.13 for development. Node 20 consumers are one `Array.fromAsync`-style API away from a runtime break no test would catch. + +**Recommendation:** add a Node version matrix (20 / 22 / 24) to the `validate`+`test` jobs, or raise the published floor to 22 and update README/engines together. + +### Low + +- **L1. Doc drift (small, several spots):** `advanced-usage.md:459` and the `docker-compose.yml` header comment reference `.github/workflows/functional-tests.yml`, which doesn't exist (functional jobs live in `ci.yml`). `memcache-adapter.md` says "`CacheManager.keys()` will exclude keys from Memcache layers" — `CacheManager` has no `keys()`. `redis-adapter.md`'s "Monitor Key Count" tip claims `clear()` uses `KEYS`, contradicting both the code and the same document's earlier (correct) statement that it uses incremental `SCAN`. +- **L2. Dead public API:** `ZIGGURAT_OPTIONS` (`packages/nestjs/src/constants.ts:1`) is exported from the package index but used nowhere — nothing ever provides it. Remove it or wire it up; today it's API surface that promises something that doesn't exist. +- **L3. Codecov upload omits the otel package** — `ci.yml:57` lists coverage files for five packages; `packages/otel/coverage` is missing, so its coverage silently never reaches Codecov. +- **L4. OTel metrics carry no namespace attribute.** Every event includes `namespace`, but the instrumentation drops it — two instrumented managers (e.g. `users` and `products`) are indistinguishable in metrics. Add `cache.namespace` to the attribute sets. +- **L5. `mget` metrics are wrong with duplicate keys** (`cache-manager.ts:263`, `:346`): the ns-key map collapses duplicates, so `missCount = keys.length - result.size` over-counts. Metrics-only; dedupe up front if you care. +- **L6. SQLite adapter never sets `busy_timeout`.** With multiple processes writing through WAL, a concurrent writer gets an immediate `SQLITE_BUSY` throw instead of a brief wait. One `pragma busy_timeout = ` in the constructor (next to the WAL pragma) removes a whole class of spurious layer errors. +- **L7. The contract suite isn't reusable outside the monorepo.** Adapter packages import it via relative path (`../../../core/tests/contract/…`). `custom-adapters.md` invites third parties to build adapters, but they can't run the compliance suite that keeps the first-party adapters honest. Publishing it (e.g. `@ziggurat-cache/adapter-testkit`) would be a differentiating move for an adapter-ecosystem library. +- **L8. Minor API/perf polish:** `del()` is a pure alias of `delete()` — one name is enough this early; `manager.has()` probes layers sequentially with full `get()`s (Redis could answer `EXISTS`/`PTTL` for `has`/`getTtl` at the cost of skipping envelope-expiry checks); listener exceptions are swallowed with no trace even in development (`event-emitter.ts:26-29`) — deliberate and defensible, but an opt-in debug hook would help people wondering why their metrics listener is silent. + +--- + +## Architectural Observations (beyond findings) + +These are not defects — they're the design decisions I'd want on the roadmap discussion for 1.0. + +1. **There is no lifecycle contract.** `CacheAdapter` has no `dispose()`; `MemoryAdapter.close()` exists ad hoc; `CacheManager` has no shutdown; `ZigguratModule` registers no `onApplicationShutdown`. Long-running apps with `checkPeriodMs` timers or injected clients have no orderly teardown path through the library's own abstractions. An optional `dispose?(): Promise` on the adapter contract, a `CacheManager.close()` that fans out to it, and a Nest lifecycle hook would complete the story. +2. **The NestJS integration is single-cache by construction.** One global module, one token, one manager. Real applications typically want several namespaced caches with different layer stacks (`users` in memory+redis, `reports` in sqlite). A `forFeature()` / named-registration API is the natural next step; the current design makes users hand-roll providers. +3. **Stampede protection is per-process only.** Coalescing collapses concurrent misses within one process; N pods still make N factory calls. That's the right v0 scope, but the README's "100 simultaneous requests = 1 database query" is only true on one instance — worth a doc caveat now, and worth roadmap slots for the standard escalation path: TTL jitter → probabilistic early refresh (stale-while-revalidate) → distributed lock via an adapter capability. +4. **The Redis/Memcache envelope duplicates expiry bookkeeping.** `expiresAt` inside the JSON exists so the manager can compute remaining TTL for backfill without an extra `PTTL` round-trip — a reasonable trade — but it's also what creates the clock-skew and read-repair issues in M3. If backfill TTL derivation changes for H1, revisit whether the envelope check should remain load-bearing or become advisory. +5. **`getTtl()` returns the first layer's answer**, which after H1 is resolved may legitimately differ from deeper layers. Fine — but say so in the API reference. + +--- + +## Appendix: Status of the 2026-06-11 Review + +| # | Finding (June) | Status now | +| --- | ------------------------------------ | --------------------------------------------------------------------------------------------------------------------------- | +| 1 | NestJS peer range excluded Nest 11 | **Fixed** (`^10.0.0 \|\| ^11.0.0`) | +| 2 | `flushAll()` used `FLUSHDB` | **Fixed** (prefix-scoped SCAN) — empty-prefix blast radius remains → M1 | +| 3 | Cross-layer value fidelity | **Addressed** (`serialization: "json"` option + docs; reference default kept, documented) | +| 4 | Backfill failures invisible | **Fixed** (settled results feed `emitWriteErrors`) — note the `backfill` event still fires at schedule time, not completion | +| 5 | Unbounded growth (memory/sqlite) | **Fixed** (`checkPeriodMs`, `maxKeys`, `purgeExpired()`) | +| 6 | `defaultTtlMs` overrode explicit TTL | **Fixed in code** — but backfill semantics + `core-concepts.md` were never reconciled → **H1** | +| 7 | Memcached >30-day TTLs | **Fixed** (absolute timestamp) | +| 8 | Redis fractional TTLs | **Fixed** (`Math.ceil`) | +| 9 | Prefix not glob-escaped | **Fixed** (`escapeGlob`) — empty-prefix concern remains → M1 | +| 10 | No `engines` in published packages | **Fixed** — but untested on Node 20 → M6 | +| 11 | `@Cached` dropped metadata | **Fixed** (metadata + name preserved) | +| 12 | All-layer write failures silent | **Fixed** (`strictWrites` + docs) | +| Low | `undefined` semantics divergence | **Still open** → M5 | +| Low | Namespace `:` collisions | **Documented** (`core-concepts.md:52`) — accepted | +| Low | OTel missing m-ops | **Fixed** | + +--- + +## Recommended Priorities + +1. **Resolve H1 now** — decide backfill TTL semantics, align code + three doc passages, and pin the behavior with a test. It's the library's headline feature and currently the docs make a promise the code doesn't keep. +2. **Rename the `CACHE_MANAGER` token value (H2)** while the breaking change is still cheap. +3. **Delete/replace `vitest.workspace.ts` (H3)** and add the Node version matrix (M6) — both are small. +4. **Fold M1–M5 into the 1.0 contract work**: safe-by-default Redis `clear()`, `wrap()` write-latency policy, uniform `mget`/`undefined` semantics in the adapter contract + contract suite, and the SQLite delete guard. +5. **Sweep the doc drift (L1)** in one pass — this codebase's docs are good enough that the few stale spots stand out. diff --git a/README.md b/README.md index e84c7ca..23c4c01 100644 --- a/README.md +++ b/README.md @@ -97,7 +97,7 @@ const cache = new CacheManager({ ], }); -// L1 miss → L2 hit → value returned + L1 backfilled with L1's own TTL +// L1 miss → L2 hit → value returned + L1 backfilled under L1's own TTL policy const product = await cache.wrap(id, async () => api.getProduct(id)); ``` diff --git a/docker-compose.yml b/docker-compose.yml index 5c850cf..c529202 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -9,7 +9,7 @@ # 1. Add a service definition below with `profiles: [backend-name]` # 2. Expose the default port and add a health check # 3. Add the corresponding env var to .env.example -# 4. Add a matrix entry in .github/workflows/functional-tests.yml +# 4. Add a matrix entry to the `functional` job in .github/workflows/ci.yml services: redis: diff --git a/docs/advanced-usage.md b/docs/advanced-usage.md index 0a560bd..ace8b53 100644 --- a/docs/advanced-usage.md +++ b/docs/advanced-usage.md @@ -27,7 +27,7 @@ const userCache = new CacheManager({ **How it works**: - `wrap("42", factory)` → checks memory for `users:42` → checks Redis → calls factory -- On a Redis hit, memory is auto-backfilled with memory's own 30s TTL +- On a Redis hit, memory is auto-backfilled with memory's own 30s TTL (capped by whatever life the Redis entry has left) - On a factory call, memory gets 30s TTL, Redis gets 5min TTL ### Three-Layer (Memory + Redis + Fallback) @@ -334,6 +334,21 @@ const cache = new CacheManager({ layers, strictWrites: true }); `strictWrites` applies to direct `set`/`mset`/`delete`/`mdel` calls. `wrap()` is unaffected: it always returns the value your factory computed, even if caching that value fails — the write error still surfaces via `"error"` events. In a single-layer setup, any write failure means "every layer failed", so it throws. +## Miss latency and `wrapWrites` + +By default `wrap()` resolves only after the computed value has been written to every layer, so a read issued right after it is guaranteed to see the value. The cost is that a slow layer adds its full write latency to every miss — and to every caller coalesced onto that miss. A Redis instance that is degraded rather than down is the case that hurts: it accepts writes, slowly, and each `wrap()` miss waits for them. + +Set `wrapWrites: "background"` to resolve as soon as the factory does and let the layer writes settle afterwards: + +```ts +const cache = new CacheManager({ + layers: [memory, redis], + wrapWrites: "background", // default is "await" +}); +``` + +The trade-off is a brief window where `wrap()` has returned but the value is not cached yet, so a request arriving inside that window recomputes it. Write failures still surface as `"error"` events either way. Keep the default when correctness depends on read-your-write behavior (a `wrap()` immediately followed by a `get()` on the same key); choose `"background"` when miss latency matters more. + ## Value fidelity across layers `MemoryAdapter` stores live references by default, while the Redis, SQLite, and Memcache adapters JSON round-trip values. A `Date` survives an L1 hit but comes back as an ISO string when L2 serves the same key. If you cache rich types in a multi-layer setup, either store plain JSON-safe data or set `new MemoryAdapter({ serialization: "json" })` for consistent shapes (this also prevents callers from mutating cached objects in place). Note that in `json` mode, non-serializable values (functions, circular references) throw at `set()` time and `undefined` is not stored. @@ -456,7 +471,7 @@ This runs functional tests for all configured backends (Redis, Memcached, SQLite ### CI Workflow -Functional tests run automatically in GitHub Actions via `.github/workflows/functional-tests.yml`. Each backend is provisioned as a service container and tested in a separate matrix job, reporting results independently from the hermetic test suite. +Functional tests run automatically in GitHub Actions via the `functional` jobs in `.github/workflows/ci.yml`. Each backend is provisioned as a service container and tested in a separate matrix job, reporting results independently from the hermetic test suite. ### Adding a New Backend @@ -467,5 +482,5 @@ To add functional tests for a new adapter (e.g., Postgres): 3. Add `test:functional` script to the adapter's `package.json` 4. Add a profile to `docker-compose.yml` 5. Add the env var (e.g., `POSTGRES_URL`) to `.env.example` -6. Add a matrix entry in `.github/workflows/functional-tests.yml` +6. Add a matrix entry to the `functional` job in `.github/workflows/ci.yml` 7. Add `test:functional:` to the root `package.json` diff --git a/docs/api-reference.md b/docs/api-reference.md index c098a97..8e99d46 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -18,14 +18,15 @@ new CacheManager(options: CacheManagerOptions) **`CacheManagerOptions`**: -| Property | Type | Default | Description | -| -------------- | ---------------------------------- | -------------------- | ------------------------------------------------------------------------------------------------------------------------------------- | -| `layers` | `CacheAdapter[]` | _(required)_ | Ordered array of cache layers. L1 is index 0 (fastest). Must contain at least one adapter — the constructor throws on an empty array. | -| `namespace` | `string` | _none_ | Prefix prepended to all keys as `namespace:key`. Useful for logical grouping. | -| `syncBackfill` | `boolean` | `false` | When `true`, waits for backfill to complete before returning. | -| `strictWrites` | `boolean` | `false` | When `true`, `set`/`mset`/`delete`/`mdel` throw an `AggregateError` if **every** layer fails the write. `wrap()` is unaffected. | -| `stampede` | `StampedeConfig` | `{ coalesce: true }` | Stampede protection configuration. | -| `events` | `TypedEventEmitter` | _(auto-created)_ | Optional shared event emitter for observability. If omitted, an internal one is created. | +| Property | Type | Default | Description | +| -------------- | ---------------------------------- | -------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------- | +| `layers` | `CacheAdapter[]` | _(required)_ | Ordered array of cache layers. L1 is index 0 (fastest). Must contain at least one adapter — the constructor throws on an empty array. | +| `namespace` | `string` | _none_ | Prefix prepended to all keys as `namespace:key`. Useful for logical grouping. | +| `syncBackfill` | `boolean` | `false` | When `true`, waits for backfill to complete before returning. | +| `wrapWrites` | `"await" \| "background"` | `"await"` | Whether `wrap()` waits for the computed value to reach every layer before resolving. `"background"` trades read-your-write for lower miss latency. | +| `strictWrites` | `boolean` | `false` | When `true`, `set`/`mset`/`delete`/`mdel` throw an `AggregateError` if **every** layer fails the write. `wrap()` is unaffected. | +| `stampede` | `StampedeConfig` | `{ coalesce: true }` | Stampede protection configuration. | +| `events` | `TypedEventEmitter` | _(auto-created)_ | Optional shared event emitter for observability. If omitted, an internal one is created. | **`StampedeConfig`**: @@ -39,6 +40,8 @@ new CacheManager(options: CacheManagerOptions) Queries layers sequentially from L1 to L*n*. Returns the first hit and backfills higher layers. Returns `null` on a complete miss. +Each backfilled layer applies its own `defaultTtlMs`, capped by the source entry's remaining lifetime, so a layer keeps its own staleness budget and a copy never outlives its source. See [Backfill](core-concepts.md#backfill). + ```ts const entry = await cache.get("user:42"); if (entry) { @@ -49,12 +52,14 @@ if (entry) { ##### `set(key: string, value: T, ttlMs?: number): Promise` -Writes the value to **all** layers. TTL is in milliseconds. Omit for no expiration. +Writes the value to **all** layers. TTL is in milliseconds. Omit to fall back to each layer's `defaultTtlMs`, or to no expiration when a layer has none. ```ts await cache.set("user:42", userData, 300_000); ``` +A value of `undefined` is never stored — the write is a no-op on every layer and leaves any existing value under that key untouched. + ##### `delete(key: string): Promise` Removes the key from **all** layers. @@ -109,6 +114,8 @@ const user = await cache.wrap( 4. Otherwise, call the factory, store the result via `set`, and return the value. 5. If the factory throws, the error propagates to all coalesced callers and the in-flight entry is cleaned up. +Step 4 waits for every layer to accept the write before resolving. Set `wrapWrites: "background"` on the manager to resolve as soon as the factory does; the writes then settle in the background and failures surface only as `"error"` events. + ##### `del(key: string): Promise` Alias for `delete`. Convenience method for developers coming from Redis-style APIs. @@ -179,22 +186,22 @@ unsub(); **Available events:** -| Event | Key Fields | Emitted When | -| --------------- | ---------------------------------------------------------------- | ------------------------------------ | -| `hit` | `key`, `layerName`, `layerIndex`, `durationMs` | `get()` finds a value in any layer | -| `miss` | `key`, `durationMs` | `get()` exhausts all layers | -| `set` | `key`, `ttlMs`, `durationMs` | `set()` writes to all layers | -| `delete` | `key`, `durationMs` | `delete()` removes from all layers | -| `error` | `key`, `operation`, `layerName`, `layerIndex`, `error` | Any layer throws during an operation | -| `backfill` | `key`, `sourceLayerName`, `sourceLayerIndex`, `targetLayerNames` | A lower-layer hit triggers backfill | -| `wrap:hit` | `key`, `durationMs` | `wrap()` finds a cached value | -| `wrap:miss` | `key`, `durationMs`, `factoryDurationMs` | `wrap()` calls the factory | -| `wrap:coalesce` | `key` | `wrap()` joins an in-flight request | -| `mget` | `keys`, `hitCount`, `missCount`, `durationMs` | `mget()` completes | -| `mset` | `keyCount`, `durationMs` | `mset()` completes | -| `mdel` | `keyCount`, `durationMs` | `mdel()` completes | - -All events include an optional `namespace` field when the manager has a namespace configured. +| Event | Key Fields | Emitted When | +| --------------- | ---------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `hit` | `key`, `layerName`, `layerIndex`, `durationMs` | `get()` finds a value in any layer | +| `miss` | `key`, `durationMs` | `get()` exhausts all layers | +| `set` | `key`, `ttlMs`, `durationMs` | `set()` writes to all layers | +| `delete` | `key`, `durationMs` | `delete()` removes from all layers | +| `error` | `key`, `operation`, `layerName`, `layerIndex`, `error` | Any layer throws during an operation | +| `backfill` | `key`, `sourceLayerName`, `sourceLayerIndex`, `targetLayerNames` | A backfill is **scheduled** after a lower-layer hit. Emitted before the writes settle; failures arrive separately as `error` events with `operation: "backfill"` | +| `wrap:hit` | `key`, `durationMs` | `wrap()` finds a cached value | +| `wrap:miss` | `key`, `durationMs`, `factoryDurationMs` | `wrap()` calls the factory | +| `wrap:coalesce` | `key` | `wrap()` joins an in-flight request | +| `mget` | `keys`, `hitCount`, `missCount`, `durationMs` | `mget()` completes | +| `mset` | `keyCount`, `durationMs` | `mset()` completes | +| `mdel` | `keyCount`, `durationMs` | `mdel()` completes | + +All events include an optional `namespace` field when the manager has a namespace configured. `mget` reports its `keys`, `hitCount`, and `missCount` over the deduplicated key set, so repeating a key in one call does not inflate the counts. See [Observability](#zigguratolel) for OpenTelemetry integration. @@ -227,6 +234,8 @@ type TtlResult = Abstract class that implements `CacheAdapter` with default implementations for all extended methods. New adapters should extend this class and only implement the 4 core methods: `get`, `set`, `delete`, `clear`. +Extending it also supplies `ttlPolicy` from the TTL options you pass to `super()`, which is how `CacheManager` keeps backfilled entries inside your layer's policy. The default `mget` returns a **partial result** — a key whose `get` throws is omitted rather than rejecting the batch — while `mset`/`mdel` reject so a failed write is reported as a layer failure. + ```ts import { BaseCacheAdapter } from "@ziggurat-cache/core"; ``` @@ -303,6 +312,8 @@ The contract every storage backend must implement. ```ts interface CacheAdapter { readonly name: string; + /** Optional; lets CacheManager keep backfilled copies within this layer's policy. */ + readonly ttlPolicy?: { defaultTtlMs?: number; maxTtlMs?: number }; get(key: string): Promise | null>; set(key: string, value: T, ttlMs?: number): Promise; delete(key: string): Promise; @@ -339,12 +350,13 @@ new RedisAdapter(options: RedisAdapterOptions) **`RedisAdapterOptions`**: -| Property | Type | Default | Description | -| -------------- | ----------------- | ------------ | ---------------------------------------------------------------------------------------------------------------------------------------------- | -| `client` | `Redis` (ioredis) | _(required)_ | A configured ioredis client instance. | -| `prefix` | `string` | `""` | Key prefix for infrastructure-level isolation. All keys are stored as `prefix + key`. | -| `defaultTtlMs` | `number` | _none_ | Fallback TTL applied when no `ttlMs` is passed to `set`/`wrap`. An explicit `ttlMs` always wins. Use `maxTtlMs` to cap all TTLs for the layer. | -| `maxTtlMs` | `number` | _none_ | Upper bound applied to every entry's TTL — explicit TTLs, `defaultTtlMs`, and otherwise-permanent entries are all capped to this. | +| Property | Type | Default | Description | +| ---------------------- | ----------------- | ------------ | ---------------------------------------------------------------------------------------------------------------------------------------------- | +| `client` | `Redis` (ioredis) | _(required)_ | A configured ioredis client instance. | +| `prefix` | `string` | `""` | Key prefix for infrastructure-level isolation. All keys are stored as `prefix + key`. | +| `allowUnprefixedClear` | `boolean` | `false` | Permits `clear()`/`flushAll()` with no `prefix` configured. Without it they throw rather than deleting every key in the database. | +| `defaultTtlMs` | `number` | _none_ | Fallback TTL applied when no `ttlMs` is passed to `set`/`wrap`. An explicit `ttlMs` always wins. Use `maxTtlMs` to cap all TTLs for the layer. | +| `maxTtlMs` | `number` | _none_ | Upper bound applied to every entry's TTL — explicit TTLs, `defaultTtlMs`, and otherwise-permanent entries are all capped to this. | ```ts import Redis from "ioredis"; @@ -366,10 +378,10 @@ const adapter = new RedisAdapter({ Implements the full `CacheAdapter` interface. -- **`get`**: Fetches the key from Redis, parses the JSON, and checks expiration. Returns `null` for missing or expired keys. -- **`set`**: Serializes the value as `{ value, expiresAt }` JSON. Uses `PSETEX` for entries with TTL, `SET` for entries without. +- **`get`**: Fetches the key from Redis, parses the JSON, and checks expiration. Returns `null` for missing, expired, or unparseable keys — reads never delete. +- **`set`**: Serializes the value as `{ value, expiresAt }` JSON. Uses `PSETEX` for entries with TTL, `SET` for entries without. An `undefined` value is skipped. - **`delete`**: Deletes the prefixed key. -- **`clear`**: Scans for all keys matching the prefix pattern and deletes them using a pipeline. Pipeline command failures throw `AggregateError`. +- **`clear`**: Scans for all keys matching the prefix pattern and deletes them using a pipeline. Pipeline command failures throw `AggregateError`. Throws when no `prefix` is configured unless `allowUnprefixedClear` is set. - **`mget`**: Uses a pipeline for batch reads. Per-key read errors are skipped and the successful entries are returned — this means `mget()` may return a partial result map rather than rejecting the entire batch. - **`mset`**: Uses a pipeline for batch writes. Entries with `ttlMs <= 0` are skipped. Pipeline command failures throw `AggregateError`. @@ -521,9 +533,11 @@ instrumentCacheManager(cacheManager: CacheManager, options?: InstrumentationOpti Returns a cleanup function that unsubscribes all listeners. +Every metric carries a `cache.namespace` attribute when the instrumented manager has a namespace, so several managers sharing one meter stay distinguishable. Managers without a namespace record no such attribute. + #### Recorded Metrics -**Counters:** +**Counters:** (`cache.namespace` is added to every row below when the manager has one) | Metric Name | Attributes | Description | | ------------------------------ | -------------------------------- | ---------------------------------------- | diff --git a/docs/core-concepts.md b/docs/core-concepts.md index 71e4d79..a85da71 100644 --- a/docs/core-concepts.md +++ b/docs/core-concepts.md @@ -55,7 +55,14 @@ Namespaces are joined with `:` and not escaped: namespace `"a"` + key `"b:c"` pr When a value is found in a lower layer (e.g., L2), the CacheManager automatically **backfills** all higher layers (e.g., L1) so subsequent reads are served from the fastest layer. -If the target adapter has a `defaultTtlMs`, backfill uses that TTL. Otherwise, it falls back to the remaining TTL from the source entry. This means L1 always uses its own TTL policy, regardless of L2's expiration. +Each target layer applies its own TTL policy to the copy it receives, capped by whatever life the source entry has left: + +- Target has a `defaultTtlMs` → the copy gets `min(defaultTtlMs, source's remaining TTL)`. +- Target has no `defaultTtlMs` → the copy gets the source's remaining TTL. +- Source entry is permanent → the target applies its own policy with nothing to cap it. +- A target's `maxTtlMs` caps the result either way. + +So L1 keeps its own staleness budget rather than inheriting L2's, and a backfilled copy never outlives the entry it was copied from. In a three-layer stack each target is computed independently — an L3 hit can backfill L1 with 10s and L2 with 60s from the same read. ### Backfill Modes @@ -162,7 +169,7 @@ TTL is specified in **milliseconds**. There are two ways to configure it: ### Per-Adapter TTL (Recommended) -Set `defaultTtlMs` on each adapter. This is the recommended approach for multi-layer setups because each layer can have its own expiration policy: +Set `defaultTtlMs` on each adapter — the fallback used when a call passes no TTL of its own. This is the recommended approach for multi-layer setups because each layer can have its own expiration policy: ```ts const cache = new CacheManager({ @@ -176,24 +183,30 @@ const cache = new CacheManager({ await cache.wrap("key", factory); ``` -### Per-Call TTL (Fallback) +### Per-Call TTL -You can also pass TTL directly to `set` or `wrap`. This acts as a fallback — if the adapter has `defaultTtlMs`, the adapter's TTL takes precedence. +You can also pass a TTL directly to `set` or `wrap`. An explicit TTL wins over the adapter's `defaultTtlMs`: ```ts -// Only used if the adapter has no defaultTtlMs +// Overrides defaultTtlMs on every layer await cache.set("key", value, 300_000); await cache.wrap("key", factory, 300_000); ``` ### TTL Resolution Order -1. Adapter's `defaultTtlMs` (if set) — always wins -2. TTL passed via `set`/`wrap` — fallback +1. TTL passed via `set`/`wrap` (if given) — wins +2. Adapter's `defaultTtlMs` — fallback when no TTL was passed 3. No TTL — entry never expires +The adapter's `maxTtlMs`, when set, caps whatever the first two steps produce — including otherwise-permanent entries. Use `defaultTtlMs` for "what a layer does when nobody says otherwise" and `maxTtlMs` for "what a layer will never exceed." + Internally, TTL is stored as an absolute Unix timestamp (`expiresAt`) on the `CacheEntry`. Expired entries are lazily cleaned up on the next `get` call. +### Values that are never stored + +`set(key, undefined)` and `mset` entries whose value is `undefined` are silently skipped by every adapter — no backend can round-trip `undefined`, so storing it would read back as either a miss or a hit carrying `undefined`, depending on the layer. The write is a no-op: `get`, `has`, `getTtl`, and `keys` all report the key as absent, and an existing value under that key is left untouched rather than being overwritten. + ## Observability The `CacheManager` emits typed events for every operation — hits, misses, errors, backfills, stampede coalescing, and more. Events have **zero cost** when no listeners are attached. diff --git a/docs/custom-adapters.md b/docs/custom-adapters.md index c37b1a6..d3aa24a 100644 --- a/docs/custom-adapters.md +++ b/docs/custom-adapters.md @@ -2,17 +2,21 @@ Ziggurat's adapter interface is intentionally minimal. Any storage backend — a database, a file system, a remote API — can be wrapped in a `CacheAdapter` and plugged into the layer stack. -## The `CacheAdapter` Interface +## Extend `BaseCacheAdapter` + +The full `CacheAdapter` interface has twelve members (`has`, `getTtl`, `keys`, `mget`, `mset`, `mdel`, `flushAll`, and a `ttlPolicy` accessor on top of the four core methods). `BaseCacheAdapter` implements all of them in terms of four, so that is what you extend: ```ts -import type { CacheAdapter, CacheEntry } from "@ziggurat-cache/core"; - -interface CacheAdapter { - readonly name: string; - get(key: string): Promise | null>; - set(key: string, value: T, ttlMs?: number): Promise; - delete(key: string): Promise; - clear(): Promise; +import { BaseCacheAdapter } from "@ziggurat-cache/core"; +import type { CacheEntry } from "@ziggurat-cache/core"; + +// You implement these four; BaseCacheAdapter derives the rest. +abstract class BaseCacheAdapter { + abstract readonly name: string; + abstract get(key: string): Promise | null>; + abstract set(key: string, value: T, ttlMs?: number): Promise; + abstract delete(key: string): Promise; + abstract clear(): Promise; } interface CacheEntry { @@ -21,20 +25,29 @@ interface CacheEntry { } ``` +Implementing the bare `CacheAdapter` interface directly is supported, but then all twelve members are yours to write — and a layer with no `ttlPolicy` receives the source entry's remaining lifetime on backfill rather than getting its own policy applied. + ## Implementing an Adapter Here's a complete example of a SQLite adapter: ```ts -import type { CacheAdapter, CacheEntry } from "@ziggurat-cache/core"; +import { BaseCacheAdapter } from "@ziggurat-cache/core"; +import type { AdapterTtlOptions, CacheEntry } from "@ziggurat-cache/core"; import Database from "better-sqlite3"; -export class SqliteAdapter implements CacheAdapter { +export interface SqliteAdapterOptions extends AdapterTtlOptions { + filePath: string; +} + +export class SqliteAdapter extends BaseCacheAdapter { readonly name = "sqlite"; private db: Database.Database; - constructor(filePath: string) { - this.db = new Database(filePath); + constructor(options: SqliteAdapterOptions) { + // Passing TTL options up is what populates `ttlPolicy` and `resolveTtl`. + super(options); + this.db = new Database(options.filePath); this.db.exec(` CREATE TABLE IF NOT EXISTS cache ( key TEXT PRIMARY KEY, @@ -64,7 +77,14 @@ export class SqliteAdapter implements CacheAdapter { } async set(key: string, value: T, ttlMs?: number): Promise { - const expiresAt = ttlMs !== undefined ? Date.now() + ttlMs : null; + // undefined is never stored — every adapter treats it as a no-op write. + if (value === undefined) return; + // resolveTtl applies defaultTtlMs and maxTtlMs from the options you + // passed to super(); an explicit ttlMs wins over defaultTtlMs. + const effectiveTtl = this.resolveTtl(ttlMs); + if (effectiveTtl !== undefined && effectiveTtl <= 0) return; // already expired + const expiresAt = + effectiveTtl !== undefined ? Date.now() + effectiveTtl : null; this.db .prepare( "INSERT OR REPLACE INTO cache (key, value, expires_at) VALUES (?, ?, ?)", @@ -88,7 +108,7 @@ export class SqliteAdapter implements CacheAdapter { The `get` method must return `{ value, expiresAt }` or `null`. The `expiresAt` field is a Unix timestamp in milliseconds, or `null` for entries that never expire. -The CacheManager uses `expiresAt` to calculate the remaining TTL when backfilling higher layers. If you return `null` for `expiresAt`, backfilled entries will have no expiration. +The CacheManager reads `expiresAt` to bound backfills: a copy written into a higher layer gets that layer's own `defaultTtlMs`, capped by your entry's remaining lifetime, so it never outlives the entry it came from. Returning `null` means the higher layer is free to apply its own policy with nothing to cap it. ### 2. Handle Expiration @@ -110,6 +130,14 @@ If your storage is shared with other systems, `clear` should only remove entries All methods return `Promise`. Even if your implementation is synchronous (like the built-in `MemoryAdapter`), the methods must be declared `async` or return resolved promises. +### 6. `undefined` Is Never Stored + +`set(key, undefined)` is a no-op on every built-in adapter: nothing is written, the key reads as absent, and any existing value under it is left alone. Follow the same rule so behavior does not change with layer order. + +### 7. Reads Should Not Delete Other People's Keys + +Cleaning up expired or corrupt entries on read is fine when the rows are unambiguously yours (the SQLite adapter does it, guarded so a concurrent writer is never clobbered). When the backend is shared and scoped only by a key prefix — Redis, Memcached — the built-in adapters report a miss and leave the key alone. The next `set()` replaces it. + ## Using Your Adapter Once implemented, use it like any built-in adapter: @@ -128,24 +156,17 @@ const cache = new CacheManager({ ## Testing with Contract Tests -Ziggurat includes a shared contract test suite that verifies any adapter correctly implements the `CacheAdapter` interface. Use it to validate your custom adapter: - -```ts -import { runAdapterContractTests } from "@ziggurat-cache/core/tests/contract/adapter-contract.test"; -import { SqliteAdapter } from "./sqlite-adapter"; - -runAdapterContractTests("SqliteAdapter", () => new SqliteAdapter(":memory:")); -``` - -The contract tests cover: +Ziggurat validates every built-in adapter against a shared contract suite (`packages/core/tests/contract/adapter-contract.test.ts`), which is how the adapters are kept behaviorally interchangeable. It covers: - `get` returns `null` on a miss - `get` returns a `CacheEntry` with correct `value` and `expiresAt` -- `set` stores and overwrites values -- `set` with TTL sets correct `expiresAt` +- `set` stores and overwrites values, and treats `undefined` as a no-op +- `set` with TTL sets correct `expiresAt`; `ttlMs <= 0` is not stored - TTL expiration removes entries -- `delete` removes a specific key -- `clear` removes all keys +- `delete` removes a specific key, `clear` removes all keys +- `mget`/`mset`/`mdel` batch semantics, including skipping `undefined` values - Various value types (strings, numbers, objects, booleans, null) -If your adapter passes all contract tests, it is compatible with the CacheManager. +> **Note:** the suite is not published on npm yet — it lives in the repository and is imported by relative path from the adapter packages, so it is available to adapters developed inside this repo but not to external ones. Packaging it as a reusable test kit is planned; until then, the list above is the behavior an external adapter should reproduce in its own tests. + +If your adapter satisfies these behaviors, it is compatible with the CacheManager. diff --git a/docs/memcache-adapter.md b/docs/memcache-adapter.md index 31db5bc..93ed451 100644 --- a/docs/memcache-adapter.md +++ b/docs/memcache-adapter.md @@ -50,6 +50,8 @@ TTLs longer than 30 days are automatically sent to memcached as an absolute expi Values are JSON-serialized as `{ value, expiresAt }` strings and stored as Buffers. On retrieval, the Buffer is converted back to a string and parsed. +A payload that fails to parse is reported as a miss. Reads never delete — a read-then-delete would race a concurrent writer, and with an empty `prefix` it could reach keys this adapter does not own. The same applies to the embedded `expiresAt` check: memcached owns the real expiry, so the check is only a clock-skew backstop and a reader with a fast clock will not evict entries other nodes still see. The next `set()` for the key replaces the bad payload. + ## Limitations ### No Key Enumeration @@ -60,7 +62,7 @@ The Memcached protocol does **not support key enumeration**. Calling `keys()` on Error: memcache does not support key enumeration. Override keys() to enable. ``` -This means `CacheManager.keys()` will exclude keys from Memcache layers. If you need key enumeration, consider using Redis or SQLite as your backing store. +`CacheManager` does not expose a `keys()` of its own — key enumeration is an adapter-level operation reached through `getLayers()`, and calling it on this adapter throws. If you need key enumeration, use Redis or SQLite as the backing store for that layer. ### No Namespace-Scoped Clear diff --git a/docs/redis-adapter.md b/docs/redis-adapter.md index 6511e2e..699af5b 100644 --- a/docs/redis-adapter.md +++ b/docs/redis-adapter.md @@ -31,12 +31,13 @@ const cache = new CacheManager({ ### `RedisAdapterOptions` -| Property | Type | Default | Description | -| -------------- | -------- | ------------ | ---------------------------------------------------------------------------------------------------------------------------------------------- | -| `client` | `Redis` | _(required)_ | A configured ioredis client instance. | -| `prefix` | `string` | `""` | String prepended to all keys for infrastructure-level isolation. | -| `defaultTtlMs` | `number` | _none_ | Fallback TTL applied when no `ttlMs` is passed to `set`/`wrap`. An explicit `ttlMs` always wins. Use `maxTtlMs` to cap all TTLs for the layer. | -| `maxTtlMs` | `number` | _none_ | Upper bound applied to every entry's TTL — explicit TTLs, `defaultTtlMs`, and otherwise-permanent entries are all capped to this. | +| Property | Type | Default | Description | +| ---------------------- | --------- | ------------ | ---------------------------------------------------------------------------------------------------------------------------------------------- | +| `client` | `Redis` | _(required)_ | A configured ioredis client instance. | +| `prefix` | `string` | `""` | String prepended to all keys for infrastructure-level isolation. | +| `allowUnprefixedClear` | `boolean` | `false` | Permits `clear()`/`flushAll()` when no `prefix` is set. Without it, those methods refuse to run rather than delete the whole database. | +| `defaultTtlMs` | `number` | _none_ | Fallback TTL applied when no `ttlMs` is passed to `set`/`wrap`. An explicit `ttlMs` always wins. Use `maxTtlMs` to cap all TTLs for the layer. | +| `maxTtlMs` | `number` | _none_ | Upper bound applied to every entry's TTL — explicit TTLs, `defaultTtlMs`, and otherwise-permanent entries are all capped to this. | ### Key Prefixing @@ -58,7 +59,20 @@ When you call `userCache.set("42", userData)`, the actual Redis key is `myapp:us The `clear()` method only deletes keys matching the adapter's prefix, so different prefixes are fully isolated. -`clear()` and `flushAll()` both delete only keys under the adapter's `prefix`, using incremental `SCAN` (never `FLUSHDB`). With an empty prefix this still scans and deletes every key in the database — always configure a `prefix` when the Redis database is shared. +`clear()` and `flushAll()` both delete only keys under the adapter's `prefix`, using incremental `SCAN` (never `FLUSHDB`). + +With an empty prefix there is nothing to scope to, so both methods **throw instead of running** — deleting every key in a possibly shared database is not something to do by accident: + +```ts +const adapter = new RedisAdapter({ client: redis }); // no prefix +await adapter.clear(); // throws + +// Wiping the whole database is a deliberate choice: +const wipe = new RedisAdapter({ client: redis, allowUnprefixedClear: true }); +await wipe.clear(); // scans and deletes everything +``` + +Reads, writes, and single-key deletes are unaffected by this — only the two bulk operations are guarded. ## How Data is Stored @@ -76,7 +90,7 @@ Values are stored as JSON strings in Redis. Each entry is a serialized `CacheEnt - **With TTL**: The adapter uses Redis `PSETEX` (set with millisecond precision expiry). Redis handles expiration natively, and the `expiresAt` timestamp is stored in the JSON for backfill TTL calculations. - **Without TTL**: The adapter uses `SET` with no expiry. The `expiresAt` field is `null`. -Both Redis-native TTL and the `expiresAt` check in `get` are enforced. If a key somehow survives past its `expiresAt` (e.g., clock drift), the adapter catches it on read and deletes the stale entry. +Both Redis-native TTL and the `expiresAt` check in `get` are enforced. Redis owns the real expiry; the `expiresAt` check is a backstop for clock drift, and an entry that fails it is reported as a miss **without being deleted**. A reader whose clock runs fast must not evict entries that other nodes still consider valid. ## Sharing a Redis Client @@ -166,13 +180,13 @@ await cache.set("dates", { }); ``` -`get()` and `mget()` treat an unparseable (corrupt or legacy) cached payload as a miss and delete the offending key, so a single bad entry can't fail a read or a whole batch. Because this deletion happens on read, with an empty `prefix` on a shared database an unparseable foreign key read through the adapter will be deleted — one more reason to always set a `prefix`. +`get()` and `mget()` treat an unparseable (corrupt or legacy) cached payload as a miss, so a single bad entry can't fail a read or a whole batch. Reads never delete: a read-then-delete would race a concurrent writer refreshing the key, and with an empty `prefix` it could reach keys the adapter does not own. The bad payload is replaced by the next `set()` for that key — which `wrap()` does automatically on the miss it just reported. ## Production Tips ### Use a Prefix -Always set a `prefix` in production. This prevents key collisions with other applications or cache instances sharing the same Redis and makes `clear()` safe to call. +Always set a `prefix` in production. This prevents key collisions with other applications or cache instances sharing the same Redis, and it is what makes `clear()` safe to call at all — without one, `clear()` and `flushAll()` refuse to run. ### Handle Redis Failures Gracefully @@ -180,4 +194,4 @@ Ziggurat's CacheManager automatically skips failing layers. If Redis is down, yo ### Monitor Key Count -The `clear()` method uses `KEYS` to find matching prefixed keys. In production Redis instances with millions of keys, prefer periodic TTL-based expiration over calling `clear()`. +The `clear()` method walks the keyspace with incremental `SCAN`, which does not block the server but does cost a full pass. On instances with millions of keys, prefer TTL-based expiration over calling `clear()`. diff --git a/docs/sqlite-adapter.md b/docs/sqlite-adapter.md index f2bfae9..6324514 100644 --- a/docs/sqlite-adapter.md +++ b/docs/sqlite-adapter.md @@ -29,13 +29,14 @@ const cache = new CacheManager({ ## Configuration -| Property | Type | Default | Description | -| -------------- | ------------------- | ------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------- | -| `db` | `Database.Database` | _(required)_ | A better-sqlite3 database instance. | -| `tableName` | `string` | `"ziggurat_cache"` | Name of the cache table. | -| `namespace` | `string` | `""` | Namespace for key isolation within the same table. | -| `defaultTtlMs` | `number` | _none_ | Fallback TTL applied when no `ttlMs` is passed to `set`/`wrap`. An explicit `ttlMs` always wins. Use `maxTtlMs` to cap all TTLs for the layer. | -| `maxTtlMs` | `number` | _none_ | Upper bound applied to every entry's TTL — explicit TTLs, `defaultTtlMs`, and otherwise-permanent entries are all capped to this. | +| Property | Type | Default | Description | +| --------------- | ------------------- | ------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------- | +| `db` | `Database.Database` | _(required)_ | A better-sqlite3 database instance. | +| `tableName` | `string` | `"ziggurat_cache"` | Name of the cache table. | +| `namespace` | `string` | `""` | Namespace for key isolation within the same table. | +| `defaultTtlMs` | `number` | _none_ | Fallback TTL applied when no `ttlMs` is passed to `set`/`wrap`. An explicit `ttlMs` always wins. Use `maxTtlMs` to cap all TTLs for the layer. | +| `maxTtlMs` | `number` | _none_ | Upper bound applied to every entry's TTL — explicit TTLs, `defaultTtlMs`, and otherwise-permanent entries are all capped to this. | +| `busyTimeoutMs` | `number` | `5000` | How long a blocked write waits for a competing writer before failing with `SQLITE_BUSY`. Set `0` to keep SQLite's no-wait default. | ## Schema @@ -55,7 +56,13 @@ CREATE TABLE IF NOT EXISTS ziggurat_cache ( - **Prepared statements** are cached and reused for all operations. - Values are stored as JSON text; `expires_at` is a Unix timestamp in milliseconds. -The adapter sets `journal_mode = WAL` and `synchronous = NORMAL` on the database you pass in. WAL mode persists on the database file — use a dedicated database file for the cache if that matters. +The adapter sets `journal_mode = WAL`, `synchronous = NORMAL`, and `busy_timeout` (5s by default) on the database you pass in. WAL mode persists on the database file — use a dedicated database file for the cache if that matters. + +## Concurrent Access + +WAL mode lets readers and a writer work at the same time, and `busyTimeoutMs` makes a blocked writer wait its turn instead of failing immediately with `SQLITE_BUSY`. + +Reads clean up as they go: a `get` that finds an expired or unparseable row deletes it. Those deletes are conditional on the row still being the one that was read (still expired, or still holding the same corrupt payload), so a writer that refreshed the key between the read and the delete is never clobbered. ## Namespace Isolation diff --git a/packages/core/README.md b/packages/core/README.md index 21349ef..f472f95 100644 --- a/packages/core/README.md +++ b/packages/core/README.md @@ -50,7 +50,7 @@ const cache = new CacheManager({ ], }); -// L1 miss -> L2 hit -> value returned + L1 backfilled +// L1 miss -> L2 hit -> value returned + L1 backfilled under its own 30s policy const product = await cache.wrap(id, async () => api.getProduct(id)); ``` diff --git a/packages/core/src/base-cache-adapter.ts b/packages/core/src/base-cache-adapter.ts index a1763c2..4c49caa 100644 --- a/packages/core/src/base-cache-adapter.ts +++ b/packages/core/src/base-cache-adapter.ts @@ -1,5 +1,6 @@ import type { AdapterTtlOptions, + AdapterTtlPolicy, CacheAdapter, CacheEntry, CacheSetEntry, @@ -8,8 +9,8 @@ import type { export abstract class BaseCacheAdapter implements CacheAdapter { abstract readonly name: string; - private readonly defaultTtlMs?: number; - private readonly maxTtlMs?: number; + /** This layer's own TTL policy; read by CacheManager when backfilling. */ + readonly ttlPolicy: AdapterTtlPolicy; constructor(ttlOptions: AdapterTtlOptions = {}) { BaseCacheAdapter.assertValidTtlOption( @@ -17,8 +18,10 @@ export abstract class BaseCacheAdapter implements CacheAdapter { ttlOptions.defaultTtlMs, ); BaseCacheAdapter.assertValidTtlOption("maxTtlMs", ttlOptions.maxTtlMs); - this.defaultTtlMs = ttlOptions.defaultTtlMs; - this.maxTtlMs = ttlOptions.maxTtlMs; + this.ttlPolicy = Object.freeze({ + defaultTtlMs: ttlOptions.defaultTtlMs, + maxTtlMs: ttlOptions.maxTtlMs, + }); } private static assertValidTtlOption( @@ -45,10 +48,11 @@ export abstract class BaseCacheAdapter implements CacheAdapter { `ttlMs must be a finite number of milliseconds (received ${String(ttlMs)}).`, ); } - const requested = ttlMs ?? this.defaultTtlMs; - if (this.maxTtlMs === undefined) return requested; - if (requested === undefined) return this.maxTtlMs; - return Math.min(requested, this.maxTtlMs); + const { defaultTtlMs, maxTtlMs } = this.ttlPolicy; + const requested = ttlMs ?? defaultTtlMs; + if (maxTtlMs === undefined) return requested; + if (requested === undefined) return maxTtlMs; + return Math.min(requested, maxTtlMs); } abstract get(key: string): Promise | null>; @@ -78,9 +82,16 @@ export abstract class BaseCacheAdapter implements CacheAdapter { ); } + /** + * Reads each key individually. Per-key failures are skipped rather than + * rejecting the whole batch, so callers get a partial result Map — the + * behavior every adapter is held to by the contract suite. Writes keep the + * opposite policy: a failed mset/mdel rejects so the layer is reported as + * having failed the write. + */ async mget(keys: readonly string[]): Promise>> { const result = new Map>(); - await Promise.all( + await Promise.allSettled( keys.map(async (key) => { const entry = await this.get(key); if (entry !== null) result.set(key, entry); diff --git a/packages/core/src/cache-manager.ts b/packages/core/src/cache-manager.ts index b1f5913..91f4bc8 100644 --- a/packages/core/src/cache-manager.ts +++ b/packages/core/src/cache-manager.ts @@ -17,6 +17,7 @@ export class CacheManager { private readonly stampedeConfig: Required; private readonly syncBackfill: boolean; private readonly strictWrites: boolean; + private readonly wrapWrites: "await" | "background"; private readonly inFlightFetches = new Map>(); private readonly events: TypedEventEmitter; @@ -28,6 +29,7 @@ export class CacheManager { this.namespace = options.namespace; this.syncBackfill = options.syncBackfill ?? false; this.strictWrites = options.strictWrites ?? false; + this.wrapWrites = options.wrapWrites ?? "await"; this.stampedeConfig = { coalesce: options.stampede?.coalesce ?? true, }; @@ -49,6 +51,26 @@ export class CacheManager { return this.namespace ? `${this.namespace}:${key}` : key; } + /** + * TTL for a copy backfilled into `layer`: that layer's own defaultTtlMs + * when it declares one, capped by the source entry's remaining lifetime so + * a backfilled copy never outlives the entry it came from. A permanent + * source entry passes no explicit TTL, leaving the target free to apply its + * own policy. Layers that expose no ttlPolicy (custom adapters not built on + * BaseCacheAdapter) receive the remaining lifetime unchanged. + */ + private backfillTtlMs( + layer: CacheAdapter, + expiresAt: number | null, + ): number | undefined { + if (expiresAt === null) return undefined; + const remainingMs = Math.max(0, expiresAt - Date.now()); + const layerDefaultMs = layer.ttlPolicy?.defaultTtlMs; + return layerDefaultMs === undefined + ? remainingMs + : Math.min(remainingMs, layerDefaultMs); + } + async get(key: string): Promise | null> { const nsKey = this.namespacedKey(key); const shouldEmit = @@ -88,14 +110,14 @@ export class CacheManager { } if (i > 0) { const backfillLayers = this.layers.slice(0, i); - const remainingTtlMs = - entry.expiresAt !== null - ? Math.max(0, entry.expiresAt - Date.now()) - : undefined; // backfillLayers === this.layers.slice(0, i), so results[] indices align with emitWriteErrors' this.layers[] indexing const backfillPromise = Promise.allSettled( backfillLayers.map((layer) => - layer.set(nsKey, entry.value, remainingTtlMs), + layer.set( + nsKey, + entry.value, + this.backfillTtlMs(layer, entry.expiresAt), + ), ), ).then((results) => { this.emitWriteErrors(results, key, "backfill"); @@ -230,7 +252,14 @@ export class CacheManager { factoryDurationMs, }); } - await this.setLayers(key, value, ttlMs); + const writes = this.setLayers(key, value, ttlMs); + // setLayers() collects results with allSettled and never rejects, so + // the backgrounded promise cannot surface as an unhandled rejection. + if (this.wrapWrites === "await") { + await writes; + } else { + void writes; + } return value; } finally { if (this.stampedeConfig.coalesce) { @@ -259,8 +288,11 @@ export class CacheManager { this.events.hasListeners("backfill"); const start = shouldEmit ? performance.now() : 0; - const nsKeys = keys.map((k) => this.namespacedKey(k)); - const keyMap = new Map(keys.map((k, i) => [nsKeys[i], k])); + // Deduplicate first: the result Map collapses repeated keys anyway, so + // counting them individually would inflate missCount in the mget event. + const uniqueKeys = [...new Set(keys)]; + const nsKeys = uniqueKeys.map((k) => this.namespacedKey(k)); + const keyMap = new Map(uniqueKeys.map((k, i) => [nsKeys[i], k])); const result = new Map>(); const remaining = new Set(nsKeys); @@ -273,7 +305,7 @@ export class CacheManager { } catch (error) { if (shouldEmit) { this.events.emit("error", { - key: keys.join(","), + key: uniqueKeys.join(","), namespace: this.namespace, operation: "mget", layerName: this.layers[i].name, @@ -301,20 +333,20 @@ export class CacheManager { if (foundInThisLayer.length > 0) { const backfillLayers = this.layers.slice(0, i); - const backfillEntries = foundInThisLayer.map(({ nsKey, entry }) => ({ - key: nsKey, - value: entry.value, - ttlMs: - entry.expiresAt !== null - ? Math.max(0, entry.expiresAt - Date.now()) - : undefined, - })); const backfillKeys = foundInThisLayer .map(({ nsKey }) => keyMap.get(nsKey)) .filter((k): k is string => k !== undefined); // backfillLayers === this.layers.slice(0, i), so results[] indices align with emitWriteErrors' this.layers[] indexing const backfillPromise = Promise.allSettled( - backfillLayers.map((layer) => layer.mset(backfillEntries)), + backfillLayers.map((layer) => + layer.mset( + foundInThisLayer.map(({ nsKey, entry }) => ({ + key: nsKey, + value: entry.value, + ttlMs: this.backfillTtlMs(layer, entry.expiresAt), + })), + ), + ), ).then((results) => { this.emitWriteErrors(results, backfillKeys.join(","), "backfill"); }); @@ -340,10 +372,10 @@ export class CacheManager { if (shouldEmit && this.events.hasListeners("mget")) { this.events.emit("mget", { - keys, + keys: uniqueKeys, namespace: this.namespace, hitCount: result.size, - missCount: keys.length - result.size, + missCount: uniqueKeys.length - result.size, durationMs: performance.now() - start, }); } diff --git a/packages/core/src/memory-adapter.ts b/packages/core/src/memory-adapter.ts index ba14d02..d81b036 100644 --- a/packages/core/src/memory-adapter.ts +++ b/packages/core/src/memory-adapter.ts @@ -2,6 +2,15 @@ import NodeCache from "node-cache"; import type { CacheEntry, MemoryAdapterOptions, TtlResult } from "./types.js"; import { BaseCacheAdapter } from "./base-cache-adapter.js"; +/** + * JSON.stringify is declared as returning string, but returns undefined for + * undefined, functions, and symbols. Stating that honestly keeps the callers' + * skip-the-write checks from looking like dead code. + */ +function stringifyOrUndefined(value: unknown): string | undefined { + return JSON.stringify(value); +} + export class MemoryAdapter extends BaseCacheAdapter { readonly name = "memory"; private readonly cache: NodeCache; @@ -44,14 +53,20 @@ export class MemoryAdapter extends BaseCacheAdapter { // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-parameters, @typescript-eslint/require-await async set(key: string, value: T, ttlMs?: number): Promise { + // undefined is never stored — no backend can round-trip it, so every + // adapter treats the write as a no-op (get/has/keys all report a miss). + if (value === undefined) return; const effectiveTtl = this.resolveTtl(ttlMs); // ttlMs <= 0 means already expired — don't store if (effectiveTtl !== undefined && effectiveTtl <= 0) return; - const stored = - this.serialization === "json" ? JSON.stringify(value) : value; - // JSON mode cannot represent undefined — skip the write entirely so - // has()/keys() stay consistent with get() reporting a miss. - if (this.serialization === "json" && stored === undefined) return; + let stored: unknown = value; + if (this.serialization === "json") { + // Functions and symbols serialize to undefined — skip those writes so + // has()/keys() stay consistent with get() reporting a miss. + const serialized = stringifyOrUndefined(value); + if (serialized === undefined) return; + stored = serialized; + } // node-cache rejects ANY set at capacity, even overwrites of existing // keys; delete first so existing keys can always be refreshed. if (this.maxKeys !== undefined && this.cache.has(key)) { diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 37d9177..673a26b 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -27,8 +27,18 @@ export interface AdapterTtlOptions { maxTtlMs?: number; } +/** + * An adapter's own TTL policy, exposed so CacheManager can keep backfilled + * entries inside the target layer's policy instead of copying the source + * layer's remaining lifetime verbatim. Adapters extending BaseCacheAdapter + * get this for free; adapters that omit it receive the source entry's + * remaining TTL on backfill. + */ +export type AdapterTtlPolicy = Readonly; + export interface CacheAdapter { readonly name: string; + readonly ttlPolicy?: AdapterTtlPolicy; get(key: string): Promise | null>; // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-parameters set(key: string, value: T, ttlMs?: number): Promise; @@ -165,6 +175,21 @@ export interface CacheManagerOptions { syncBackfill?: boolean; stampede?: StampedeConfig; events?: TypedEventEmitter; + /** + * Controls whether wrap() waits for the computed value to be written to + * every layer before resolving. + * + * "await" (default): resolve only after all layer writes settle, so a + * read issued after wrap() resolves is guaranteed to see the value. A slow + * layer adds its full write latency to every wrap() miss — and to every + * caller coalesced onto it. + * + * "background": resolve as soon as the factory does and let the writes + * settle in the background. Lower miss latency at the cost of a brief + * window where the value is not yet cached. Write failures still surface + * via "error" events. + */ + wrapWrites?: "await" | "background"; /** * When true, set/mset/delete/mdel throw an AggregateError if EVERY * layer fails the write. Default false (writes never throw; failures @@ -199,10 +224,10 @@ export interface MemoryAdapterOptions extends AdapterTtlOptions { * (Date, Map) survive here while JSON-based layers flatten them, so * multi-layer reads can return different shapes per layer. * "json": JSON round-trip on every set/get — consistent with the Redis, - * SQLite, and Memcache adapters and immune to caller mutation. Note the - * caveats: non-serializable values (functions, circular references) throw - * at `set()` time, and `undefined` values are not stored at all — reads, - * `has()`, and `keys()` all report a miss for them. + * SQLite, and Memcache adapters and immune to caller mutation. Circular + * references throw at `set()` time, and values JSON cannot represent at all + * (functions, symbols) are skipped like `undefined`: nothing is stored, and + * reads, `has()`, and `keys()` all report a miss. */ serialization?: "reference" | "json"; } diff --git a/packages/core/tests/contract/adapter-contract.test.ts b/packages/core/tests/contract/adapter-contract.test.ts index 5f8c1b9..08768e8 100644 --- a/packages/core/tests/contract/adapter-contract.test.ts +++ b/packages/core/tests/contract/adapter-contract.test.ts @@ -105,6 +105,25 @@ export function runAdapterContractTests( expect(result).not.toBeNull(); expect(result!.value).toBe("value1"); }); + + it("should treat an undefined value as a no-op write", async () => { + // No backend can round-trip undefined, so every adapter agrees: + // the write is skipped and the key reads as absent everywhere. + await expect(adapter.set("key1", undefined)).resolves.toBeUndefined(); + expect(await adapter.get("key1")).toBeNull(); + expect(await adapter.has("key1")).toBe(false); + expect((await adapter.getTtl("key1")).kind).toBe("missing"); + if (supportsKeys) { + expect(await adapter.keys()).not.toContain("key1"); + } + }); + + it("should not clobber an existing value with an undefined write", async () => { + await adapter.set("key1", "value1"); + await adapter.set("key1", undefined); + const result = await adapter.get("key1"); + expect(result!.value).toBe("value1"); + }); }); describe("TTL expiry", () => { @@ -310,6 +329,15 @@ export function runAdapterContractTests( expect((await adapter.get("b"))!.value).toBe(2); }); + it("should skip undefined values without failing the batch", async () => { + await adapter.mset([ + { key: "a", value: undefined }, + { key: "b", value: 2 }, + ]); + expect(await adapter.get("a")).toBeNull(); + expect((await adapter.get("b"))!.value).toBe(2); + }); + it("should be a no-op for empty entry list", async () => { await expect(adapter.mset([])).resolves.not.toThrow(); }); diff --git a/packages/core/tests/integration/multi-layer.test.ts b/packages/core/tests/integration/multi-layer.test.ts index 88692aa..6072716 100644 --- a/packages/core/tests/integration/multi-layer.test.ts +++ b/packages/core/tests/integration/multi-layer.test.ts @@ -170,6 +170,110 @@ describe("Multi-Layer Cache", () => { expect(remainingTtl).toBeGreaterThan(4000); expect(remainingTtl).toBeLessThanOrEqual(5000); }); + + it("backfills with the target layer's own defaultTtlMs, not the source's remaining TTL", async () => { + // The documented promise: each layer keeps its own staleness budget. + // L1 must not inherit L2's much longer lifetime. + const l1Short = new MemoryAdapter({ defaultTtlMs: 30_000 }); + const l2Long = new MemoryAdapter({ defaultTtlMs: 300_000 }); + const cache = new CacheManager({ + layers: [l1Short, l2Long], + syncBackfill: true, + }); + + await l2Long.set("k", "v"); + await cache.get("k"); + + const l1Ttl = await l1Short.getTtl("k"); + expect(l1Ttl.kind).toBe("expiring"); + if (l1Ttl.kind === "expiring") { + expect(l1Ttl.ttlMs).toBeLessThanOrEqual(30_000); + expect(l1Ttl.ttlMs).toBeGreaterThan(29_000); + } + }); + + it("never backfills beyond the source entry's remaining lifetime", async () => { + // The source expires in 2s; L1's 30s default must not outlive it. + const l1Short = new MemoryAdapter({ defaultTtlMs: 30_000 }); + const l2Expiring = new MemoryAdapter(); + const cache = new CacheManager({ + layers: [l1Short, l2Expiring], + syncBackfill: true, + }); + + await l2Expiring.set("k", "v", 2000); + await cache.get("k"); + + const l1Ttl = await l1Short.getTtl("k"); + expect(l1Ttl.kind).toBe("expiring"); + if (l1Ttl.kind === "expiring") { + expect(l1Ttl.ttlMs).toBeLessThanOrEqual(2000); + } + }); + + it("lets the target apply its own policy when the source entry is permanent", async () => { + const l1Short = new MemoryAdapter({ defaultTtlMs: 30_000 }); + const l2Permanent = new MemoryAdapter(); + const cache = new CacheManager({ + layers: [l1Short, l2Permanent], + syncBackfill: true, + }); + + await l2Permanent.set("k", "v"); + await cache.get("k"); + + const l1Ttl = await l1Short.getTtl("k"); + expect(l1Ttl.kind).toBe("expiring"); + if (l1Ttl.kind === "expiring") { + expect(l1Ttl.ttlMs).toBeLessThanOrEqual(30_000); + } + }); + + it("applies the same TTL policy to mget backfill", async () => { + const l1Short = new MemoryAdapter({ defaultTtlMs: 30_000 }); + const l2Long = new MemoryAdapter({ defaultTtlMs: 300_000 }); + const cache = new CacheManager({ + layers: [l1Short, l2Long], + syncBackfill: true, + }); + + await l2Long.mset([ + { key: "a", value: 1 }, + { key: "b", value: 2 }, + ]); + await cache.mget(["a", "b"]); + + for (const key of ["a", "b"]) { + const ttl = await l1Short.getTtl(key); + expect(ttl.kind).toBe("expiring"); + if (ttl.kind === "expiring") { + expect(ttl.ttlMs).toBeLessThanOrEqual(30_000); + } + } + }); + + it("gives each target layer its own backfill TTL", async () => { + const l1Short = new MemoryAdapter({ defaultTtlMs: 10_000 }); + const l2Medium = new MemoryAdapter({ defaultTtlMs: 60_000 }); + const l3Source = new MemoryAdapter({ defaultTtlMs: 600_000 }); + const cache = new CacheManager({ + layers: [l1Short, l2Medium, l3Source], + syncBackfill: true, + }); + + await l3Source.set("k", "v"); + await cache.get("k"); + + const l1Ttl = await l1Short.getTtl("k"); + const l2Ttl = await l2Medium.getTtl("k"); + if (l1Ttl.kind === "expiring" && l2Ttl.kind === "expiring") { + expect(l1Ttl.ttlMs).toBeLessThanOrEqual(10_000); + expect(l2Ttl.ttlMs).toBeGreaterThan(10_000); + expect(l2Ttl.ttlMs).toBeLessThanOrEqual(60_000); + } else { + expect.unreachable("both layers should hold an expiring entry"); + } + }); }); describe("namespace with multi-layer", () => { diff --git a/packages/core/tests/unit/base-cache-adapter.test.ts b/packages/core/tests/unit/base-cache-adapter.test.ts index 2a06268..396ec78 100644 --- a/packages/core/tests/unit/base-cache-adapter.test.ts +++ b/packages/core/tests/unit/base-cache-adapter.test.ts @@ -234,8 +234,10 @@ describe("TTL resolution (defaultTtlMs / maxTtlMs)", () => { describe("BaseCacheAdapter batch error propagation", () => { const adapter = new FailingAdapter(); - it("mget should reject when underlying get calls fail", async () => { - await expect(adapter.mget(["a", "b"])).rejects.toThrow("get failed"); + it("mget should return a partial result rather than rejecting when gets fail", async () => { + // Reads degrade to partial results so one bad key cannot cost the caller + // the whole batch — and, through CacheManager, the whole layer. + await expect(adapter.mget(["a", "b"])).resolves.toEqual(new Map()); }); it("mset should reject when underlying set calls fail", async () => { diff --git a/packages/core/tests/unit/cache-manager.test.ts b/packages/core/tests/unit/cache-manager.test.ts index 36710f7..78048df 100644 --- a/packages/core/tests/unit/cache-manager.test.ts +++ b/packages/core/tests/unit/cache-manager.test.ts @@ -165,6 +165,20 @@ describe("CacheManager (single-layer)", () => { const result = await manager.mget([]); expect(result.size).toBe(0); }); + + it("should deduplicate repeated keys when counting hits and misses", async () => { + await manager.set("a", 1); + const events: { hitCount: number; missCount: number }[] = []; + manager.on("mget", (e) => + events.push({ hitCount: e.hitCount, missCount: e.missCount }), + ); + + await manager.mget(["a", "a", "missing", "missing"]); + + // Two distinct keys were asked for: one hit, one miss. Counting the + // repeats would report a miss that never happened. + expect(events).toEqual([{ hitCount: 1, missCount: 1 }]); + }); }); describe("mset", () => { @@ -908,6 +922,60 @@ describe("strictWrites", () => { }); }); +describe("wrapWrites", () => { + function slowWriteAdapter(delayMs: number): MemoryAdapter { + const a = new MemoryAdapter(); + const realSet = a.set.bind(a); + vi.spyOn(a, "set").mockImplementation(async (key, value, ttlMs) => { + await new Promise((r) => setTimeout(r, delayMs)); + await realSet(key, value, ttlMs); + }); + return a; + } + + it("awaits layer writes by default, so a read after wrap() sees the value", async () => { + const layer = slowWriteAdapter(30); + const cache = new CacheManager({ layers: [layer] }); + + await cache.wrap("k", async () => "v"); + + expect((await layer.get("k"))!.value).toBe("v"); + }); + + it('resolves before the writes settle when wrapWrites is "background"', async () => { + const layer = slowWriteAdapter(30); + const cache = new CacheManager({ + layers: [layer], + wrapWrites: "background", + }); + + expect(await cache.wrap("k", async () => "v")).toBe("v"); + // The write is still in flight — that is the point of the option. + expect(await layer.get("k")).toBeNull(); + + await vi.waitFor(async () => { + expect((await layer.get("k"))!.value).toBe("v"); + }); + }); + + it("still reports background write failures via error events", async () => { + const layer = new MemoryAdapter(); + vi.spyOn(layer, "set").mockRejectedValue(new Error("down")); + const cache = new CacheManager({ + layers: [layer], + wrapWrites: "background", + }); + const errors: unknown[] = []; + cache.on("error", (e) => errors.push(e.error)); + + expect(await cache.wrap("k", async () => "v")).toBe("v"); + + await vi.waitFor(() => { + expect(errors).toHaveLength(1); + }); + }); +}); + describe("constructor validation", () => { it("throws when layers is empty", () => { expect(() => new CacheManager({ layers: [] })).toThrow( diff --git a/packages/memcache/README.md b/packages/memcache/README.md index dbc51f7..94f41fe 100644 --- a/packages/memcache/README.md +++ b/packages/memcache/README.md @@ -36,7 +36,8 @@ const user = await cache.wrap(`user:${id}`, async () => db.users.findById(id)); ```ts interface MemcacheAdapterOptions { client: memjs.Client; // memjs client instance - defaultTtlMs?: number; // Default TTL in milliseconds + defaultTtlMs?: number; // Fallback TTL when a call passes none + maxTtlMs?: number; // Upper bound applied to every entry prefix?: string; // Key prefix (default: none) } ``` diff --git a/packages/memcache/src/memcache-adapter.ts b/packages/memcache/src/memcache-adapter.ts index c42d8ec..e85cb92 100644 --- a/packages/memcache/src/memcache-adapter.ts +++ b/packages/memcache/src/memcache-adapter.ts @@ -34,13 +34,17 @@ export class MemcacheAdapter extends BaseCacheAdapter { try { entry = JSON.parse(result.value.toString()) as CacheEntry; } catch { - // Corrupt/legacy payload — delete and treat as a miss - await this.client.delete(this.prefixedKey(key)); + // Corrupt/legacy payload — treat as a miss. Reads never delete: a + // read-then-delete would race a concurrent writer refreshing the key, + // and with an empty prefix it would reach keys this adapter does not + // own. The next set() overwrites the bad payload anyway. return null; } + // Memcached enforces the real expiry itself; this envelope check is a + // clock-skew backstop only, so it reports a miss without deleting — a + // reader with a fast clock must not evict entries other nodes still see. if (entry.expiresAt !== null && Date.now() >= entry.expiresAt) { - await this.client.delete(this.prefixedKey(key)); return null; } @@ -49,6 +53,9 @@ export class MemcacheAdapter extends BaseCacheAdapter { // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-parameters async set(key: string, value: T, ttlMs?: number): Promise { + // undefined is never stored — JSON cannot round-trip it, and storing the + // envelope without a value would read back as a hit carrying undefined. + if (value === undefined) return; const effectiveTtl = this.resolveTtl(ttlMs); // ttlMs <= 0 means already expired — don't store if (effectiveTtl !== undefined && effectiveTtl <= 0) return; diff --git a/packages/memcache/tests/unit/memcache-adapter.test.ts b/packages/memcache/tests/unit/memcache-adapter.test.ts index 882f84a..c21f68c 100644 --- a/packages/memcache/tests/unit/memcache-adapter.test.ts +++ b/packages/memcache/tests/unit/memcache-adapter.test.ts @@ -69,14 +69,16 @@ describe("MemcacheAdapter", () => { expect(result!.value).toEqual({ foo: "bar" }); }); - it("returns null and deletes the key on corrupt JSON", async () => { + it("returns null on corrupt JSON without deleting the key", async () => { await mockClient.set("bad", "{not json"); const result = await adapter.get("bad"); expect(result).toBeNull(); - expect(mockClient.delete).toHaveBeenCalledWith("bad"); + // Reads must not delete: it would race a concurrent writer, and with an + // empty prefix it would reach keys this adapter does not own. + expect(mockClient.delete).not.toHaveBeenCalled(); }); - it("deletes the prefixed key on corrupt JSON when a prefix is set", async () => { + it("returns null for a corrupt prefixed key without deleting it", async () => { const prefixed = new MemcacheAdapter({ client: mockClient, prefix: "app:", @@ -84,7 +86,7 @@ describe("MemcacheAdapter", () => { await mockClient.set("app:bad", "{not json"); const result = await prefixed.get("bad"); expect(result).toBeNull(); - expect(mockClient.delete).toHaveBeenCalledWith("app:bad"); + expect(mockClient.delete).not.toHaveBeenCalled(); }); }); diff --git a/packages/nestjs/src/constants.ts b/packages/nestjs/src/constants.ts index 0834ab8..0387463 100644 --- a/packages/nestjs/src/constants.ts +++ b/packages/nestjs/src/constants.ts @@ -1,2 +1,9 @@ -export const ZIGGURAT_OPTIONS = "ZIGGURAT_OPTIONS"; -export const CACHE_MANAGER = "CACHE_MANAGER"; +/** + * Injection token for the Ziggurat CacheManager. + * + * The string is deliberately namespaced: `@nestjs/cache-manager` publishes a + * token whose value is the bare string "CACHE_MANAGER", and ZigguratModule + * registers globally, so sharing that value would make injection order decide + * which manager a consumer receives. + */ +export const CACHE_MANAGER = "ZIGGURAT_CACHE_MANAGER"; diff --git a/packages/nestjs/src/index.ts b/packages/nestjs/src/index.ts index 91c3aff..ce2ba25 100644 --- a/packages/nestjs/src/index.ts +++ b/packages/nestjs/src/index.ts @@ -2,4 +2,4 @@ export { ZigguratModule } from "./ziggurat.module.js"; export type { ZigguratModuleAsyncOptions } from "./ziggurat.module.js"; export { Cached } from "./cached.decorator.js"; export type { CachedDecoratorOptions } from "./cached.decorator.js"; -export { CACHE_MANAGER, ZIGGURAT_OPTIONS } from "./constants.js"; +export { CACHE_MANAGER } from "./constants.js"; diff --git a/packages/nestjs/tests/unit/ziggurat-module.test.ts b/packages/nestjs/tests/unit/ziggurat-module.test.ts index ca61861..0b9f1fd 100644 --- a/packages/nestjs/tests/unit/ziggurat-module.test.ts +++ b/packages/nestjs/tests/unit/ziggurat-module.test.ts @@ -7,6 +7,16 @@ import { CACHE_MANAGER } from "../../src/constants.js"; import { CacheManager, MemoryAdapter } from "@ziggurat-cache/core"; describe("ZigguratModule", () => { + describe("CACHE_MANAGER token", () => { + it("should not collide with the @nestjs/cache-manager token value", () => { + // ZigguratModule registers globally, so sharing the bare "CACHE_MANAGER" + // string with @nestjs/cache-manager would let resolution order decide + // which manager a consumer is injected with. + expect(CACHE_MANAGER).not.toBe("CACHE_MANAGER"); + expect(CACHE_MANAGER).toBe("ZIGGURAT_CACHE_MANAGER"); + }); + }); + describe("forRoot", () => { it("should provide CacheManager via CACHE_MANAGER token", async () => { const module = await Test.createTestingModule({ diff --git a/packages/otel/README.md b/packages/otel/README.md index 43c50ed..6598832 100644 --- a/packages/otel/README.md +++ b/packages/otel/README.md @@ -48,9 +48,14 @@ instrumentCacheManager(cache, { | `ziggurat.cache.wrap.hit` | Counter | `wrap()` served from cache | | `ziggurat.cache.wrap.miss` | Counter | `wrap()` called the factory | | `ziggurat.cache.wrap.coalesce` | Counter | Concurrent requests coalesced | +| `ziggurat.cache.mget` | Counter | Batch reads | +| `ziggurat.cache.mset` | Counter | Batch writes | +| `ziggurat.cache.mdel` | Counter | Batch deletes | | `ziggurat.cache.duration` | Histogram | Operation duration in ms (attributes: `cache.operation`, `cache.layer`) | | `ziggurat.cache.wrap.factory_duration` | Histogram | Factory call duration in ms | +Every metric also carries a `cache.namespace` attribute when the instrumented `CacheManager` has a namespace configured, so several managers sharing one meter stay distinguishable. + ## Prometheus Example ```ts diff --git a/packages/otel/src/instrumentation.ts b/packages/otel/src/instrumentation.ts index c007426..27969ed 100644 --- a/packages/otel/src/instrumentation.ts +++ b/packages/otel/src/instrumentation.ts @@ -1,10 +1,24 @@ -import { metrics } from "@opentelemetry/api"; +import { metrics, type Attributes } from "@opentelemetry/api"; import type { CacheManager } from "@ziggurat-cache/core"; export interface InstrumentationOptions { meterName?: string; } +/** + * Merge the event's namespace into an attribute set so metrics from two + * managers over the same meter stay distinguishable. Omitted entirely when + * the manager has no namespace, rather than recorded as an empty string. + */ +function withNamespace( + namespace: string | undefined, + attributes: Attributes = {}, +): Attributes { + return namespace === undefined + ? attributes + : { ...attributes, "cache.namespace": namespace }; +} + export function instrumentCacheManager( cacheManager: CacheManager, options?: InstrumentationOptions, @@ -66,100 +80,131 @@ export function instrumentCacheManager( unsubscribers.push( cacheManager.on("hit", (e) => { - hitCounter.add(1, { + const attributes = withNamespace(e.namespace, { "cache.layer": e.layerName, "cache.operation": "get", }); - durationHistogram.record(e.durationMs, { - "cache.operation": "get", - "cache.layer": e.layerName, - }); + hitCounter.add(1, attributes); + durationHistogram.record(e.durationMs, attributes); }), ); unsubscribers.push( cacheManager.on("miss", (e) => { - missCounter.add(1, { "cache.operation": "get" }); - durationHistogram.record(e.durationMs, { "cache.operation": "get" }); + const attributes = withNamespace(e.namespace, { + "cache.operation": "get", + }); + missCounter.add(1, attributes); + durationHistogram.record(e.durationMs, attributes); }), ); unsubscribers.push( cacheManager.on("set", (e) => { - setCounter.add(1); - durationHistogram.record(e.durationMs, { "cache.operation": "set" }); + setCounter.add(1, withNamespace(e.namespace)); + durationHistogram.record( + e.durationMs, + withNamespace(e.namespace, { "cache.operation": "set" }), + ); }), ); unsubscribers.push( cacheManager.on("delete", (e) => { - deleteCounter.add(1); - durationHistogram.record(e.durationMs, { - "cache.operation": "delete", - }); + deleteCounter.add(1, withNamespace(e.namespace)); + durationHistogram.record( + e.durationMs, + withNamespace(e.namespace, { "cache.operation": "delete" }), + ); }), ); unsubscribers.push( cacheManager.on("error", (e) => { - errorCounter.add(1, { - "cache.layer": e.layerName, - "cache.operation": e.operation, - }); + errorCounter.add( + 1, + withNamespace(e.namespace, { + "cache.layer": e.layerName, + "cache.operation": e.operation, + }), + ); }), ); unsubscribers.push( cacheManager.on("backfill", (e) => { - backfillCounter.add(1, { "cache.source_layer": e.sourceLayerName }); + backfillCounter.add( + 1, + withNamespace(e.namespace, { + "cache.source_layer": e.sourceLayerName, + }), + ); }), ); unsubscribers.push( cacheManager.on("wrap:hit", (e) => { - wrapHitCounter.add(1); - durationHistogram.record(e.durationMs, { "cache.operation": "wrap" }); + wrapHitCounter.add(1, withNamespace(e.namespace)); + durationHistogram.record( + e.durationMs, + withNamespace(e.namespace, { "cache.operation": "wrap" }), + ); }), ); unsubscribers.push( cacheManager.on("wrap:miss", (e) => { - wrapMissCounter.add(1); - durationHistogram.record(e.durationMs, { "cache.operation": "wrap" }); - factoryDurationHistogram.record(e.factoryDurationMs); + wrapMissCounter.add(1, withNamespace(e.namespace)); + durationHistogram.record( + e.durationMs, + withNamespace(e.namespace, { "cache.operation": "wrap" }), + ); + factoryDurationHistogram.record( + e.factoryDurationMs, + withNamespace(e.namespace), + ); }), ); unsubscribers.push( - cacheManager.on("wrap:coalesce", () => { - wrapCoalesceCounter.add(1); + cacheManager.on("wrap:coalesce", (e) => { + wrapCoalesceCounter.add(1, withNamespace(e.namespace)); }), ); unsubscribers.push( cacheManager.on("mget", (e) => { - mgetCounter.add(1); + const attributes = withNamespace(e.namespace, { + "cache.operation": "mget", + }); + mgetCounter.add(1, withNamespace(e.namespace)); if (e.hitCount > 0) { - hitCounter.add(e.hitCount, { "cache.operation": "mget" }); + hitCounter.add(e.hitCount, attributes); } if (e.missCount > 0) { - missCounter.add(e.missCount, { "cache.operation": "mget" }); + missCounter.add(e.missCount, attributes); } - durationHistogram.record(e.durationMs, { "cache.operation": "mget" }); + durationHistogram.record(e.durationMs, attributes); }), ); unsubscribers.push( cacheManager.on("mset", (e) => { - msetCounter.add(1); - durationHistogram.record(e.durationMs, { "cache.operation": "mset" }); + msetCounter.add(1, withNamespace(e.namespace)); + durationHistogram.record( + e.durationMs, + withNamespace(e.namespace, { "cache.operation": "mset" }), + ); }), ); unsubscribers.push( cacheManager.on("mdel", (e) => { - mdelCounter.add(1); - durationHistogram.record(e.durationMs, { "cache.operation": "mdel" }); + mdelCounter.add(1, withNamespace(e.namespace)); + durationHistogram.record( + e.durationMs, + withNamespace(e.namespace, { "cache.operation": "mdel" }), + ); }), ); diff --git a/packages/otel/tests/unit/instrumentation.test.ts b/packages/otel/tests/unit/instrumentation.test.ts index 32ec469..1454b57 100644 --- a/packages/otel/tests/unit/instrumentation.test.ts +++ b/packages/otel/tests/unit/instrumentation.test.ts @@ -375,4 +375,75 @@ describe("instrumentCacheManager", () => { cleanup(); }); + + it("should tag metrics with the manager's namespace", async () => { + const manager = new CacheManager({ + layers: [new MemoryAdapter()], + namespace: "users", + }); + const cleanup = instrumentCacheManager(manager); + + await manager.set("k1", "v1"); + await manager.get("k1"); + + const collected = await collectMetrics(); + for (const name of ["ziggurat.cache.hit", "ziggurat.cache.set"]) { + const metric = findMetric(collected, name); + expect(metric).toBeDefined(); + expect( + metric!.dataPoints.every( + (dp) => dp.attributes["cache.namespace"] === "users", + ), + ).toBe(true); + } + + cleanup(); + }); + + it("should omit the namespace attribute when the manager has none", async () => { + const manager = new CacheManager({ layers: [new MemoryAdapter()] }); + const cleanup = instrumentCacheManager(manager); + + await manager.set("k1", "v1"); + await manager.get("k1"); + + const collected = await collectMetrics(); + const hitMetric = findMetric(collected, "ziggurat.cache.hit"); + expect( + hitMetric!.dataPoints.every( + (dp) => !("cache.namespace" in dp.attributes), + ), + ).toBe(true); + + cleanup(); + }); + + it("should keep two namespaced managers distinguishable on one meter", async () => { + const users = new CacheManager({ + layers: [new MemoryAdapter()], + namespace: "users", + }); + const products = new CacheManager({ + layers: [new MemoryAdapter()], + namespace: "products", + }); + const cleanupUsers = instrumentCacheManager(users); + const cleanupProducts = instrumentCacheManager(products); + + await users.get("absent"); + await products.get("absent"); + await products.get("absent"); + + const collected = await collectMetrics(); + const missMetric = findMetric(collected, "ziggurat.cache.miss"); + const byNamespace = (ns: string) => + missMetric!.dataPoints.find( + (dp) => dp.attributes["cache.namespace"] === ns, + ); + expect(byNamespace("users")!.value).toBe(1); + expect(byNamespace("products")!.value).toBe(2); + + cleanupUsers(); + cleanupProducts(); + }); }); diff --git a/packages/redis/README.md b/packages/redis/README.md index 45ab023..1807d26 100644 --- a/packages/redis/README.md +++ b/packages/redis/README.md @@ -34,8 +34,10 @@ const user = await cache.wrap(`user:${id}`, async () => db.users.findById(id)); ```ts interface RedisAdapterOptions { client: Redis; // ioredis client instance - defaultTtlMs?: number; // Default TTL in milliseconds + defaultTtlMs?: number; // Fallback TTL when a call passes none + maxTtlMs?: number; // Upper bound applied to every entry prefix?: string; // Key prefix (default: none) + allowUnprefixedClear?: boolean; // Permit clear()/flushAll() with no prefix } ``` diff --git a/packages/redis/src/redis-adapter.ts b/packages/redis/src/redis-adapter.ts index bdb0baa..cf02022 100644 --- a/packages/redis/src/redis-adapter.ts +++ b/packages/redis/src/redis-adapter.ts @@ -9,17 +9,26 @@ import type { Redis } from "ioredis"; export interface RedisAdapterOptions extends AdapterTtlOptions { client: Redis; prefix?: string; + /** + * Permit clear()/flushAll() when no `prefix` is configured. Without a + * prefix those methods match every key in the database — including keys + * written by other applications — so they refuse to run unless you opt in + * here. Reads, writes, and deletes of individual keys are unaffected. + */ + allowUnprefixedClear?: boolean; } export class RedisAdapter extends BaseCacheAdapter { readonly name = "redis"; private readonly client: Redis; private readonly prefix: string; + private readonly allowUnprefixedClear: boolean; constructor(options: RedisAdapterOptions) { super(options); this.client = options.client; this.prefix = options.prefix ?? ""; + this.allowUnprefixedClear = options.allowUnprefixedClear ?? false; } private prefixedKey(key: string): string { @@ -34,13 +43,17 @@ export class RedisAdapter extends BaseCacheAdapter { try { entry = JSON.parse(raw) as CacheEntry; } catch { - // Corrupt/legacy payload — delete and treat as a miss - await this.client.del(this.prefixedKey(key)); + // Corrupt/legacy payload — treat as a miss. Reads never delete: a + // read-then-delete would race a concurrent writer refreshing the key, + // and with an empty prefix it would reach keys this adapter does not + // own. The next set() overwrites the bad payload anyway. return null; } + // Redis enforces the real expiry via PSETEX; this envelope check is a + // clock-skew backstop only, so it reports a miss without deleting — a + // reader with a fast clock must not evict entries other nodes still see. if (entry.expiresAt !== null && Date.now() >= entry.expiresAt) { - await this.client.del(this.prefixedKey(key)); return null; } @@ -49,6 +62,9 @@ export class RedisAdapter extends BaseCacheAdapter { // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-parameters async set(key: string, value: T, ttlMs?: number): Promise { + // undefined is never stored — JSON cannot round-trip it, and storing the + // envelope without a value would read back as a hit carrying undefined. + if (value === undefined) return; const effectiveTtl = this.resolveTtl(ttlMs); // ttlMs <= 0 means already expired — don't store if (effectiveTtl !== undefined && effectiveTtl <= 0) return; @@ -73,6 +89,20 @@ export class RedisAdapter extends BaseCacheAdapter { return literal.replace(/[\\*?[\]]/g, "\\$&"); } + /** + * clear()/flushAll() delete every key matching `prefix + "*"`. With an + * empty prefix that is the entire database, so refuse unless the caller + * explicitly opted in via `allowUnprefixedClear`. + */ + private assertClearIsScoped(): void { + if (this.prefix === "" && !this.allowUnprefixedClear) { + throw new Error( + "RedisAdapter.clear()/flushAll() would delete every key in the database because no prefix is configured. " + + "Set a `prefix`, or pass `allowUnprefixedClear: true` if wiping the whole database is intended.", + ); + } + } + private async scanKeys(pattern: string): Promise { const keys: string[] = []; let cursor = "0"; @@ -106,6 +136,7 @@ export class RedisAdapter extends BaseCacheAdapter { } async clear(): Promise { + this.assertClearIsScoped(); const pattern = RedisAdapter.escapeGlob(this.prefix) + "*"; const keys = await this.scanKeys(pattern); if (keys.length > 0) { @@ -137,8 +168,6 @@ export class RedisAdapter extends BaseCacheAdapter { if (!results) return map; - const corruptKeys: string[] = []; - for (let i = 0; i < keys.length; i++) { const [err, raw] = results[i] as [Error | null, string | null]; if (err || raw === null) continue; @@ -147,8 +176,7 @@ export class RedisAdapter extends BaseCacheAdapter { try { entry = JSON.parse(raw) as CacheEntry; } catch { - // Corrupt/legacy payload — treat as a miss and schedule cleanup - corruptKeys.push(prefixedKeys[i]); + // Corrupt/legacy payload — a miss, deleted by nobody. See get(). continue; } if (entry.expiresAt !== null && Date.now() >= entry.expiresAt) { @@ -157,10 +185,6 @@ export class RedisAdapter extends BaseCacheAdapter { map.set(keys[i], entry); } - if (corruptKeys.length > 0) { - await this.client.del(...corruptKeys); - } - return map; } @@ -170,6 +194,8 @@ export class RedisAdapter extends BaseCacheAdapter { const pipeline = this.client.pipeline(); let queued = 0; for (const entry of entries) { + // undefined is never stored — see set(). + if (entry.value === undefined) continue; const effectiveTtl = this.resolveTtl(entry.ttlMs); // ttlMs <= 0 means already expired — don't store if (effectiveTtl !== undefined && effectiveTtl <= 0) continue; diff --git a/packages/redis/tests/contract/redis-contract.test.ts b/packages/redis/tests/contract/redis-contract.test.ts index ed98116..9d6c74b 100644 --- a/packages/redis/tests/contract/redis-contract.test.ts +++ b/packages/redis/tests/contract/redis-contract.test.ts @@ -92,7 +92,10 @@ function createInMemoryRedis(): Redis { } as unknown as Redis; } +// A prefix is required for clear()/flushAll() to be scoped rather than +// database-wide, and the contract suite clears between cases. runAdapterContractTests( "RedisAdapter", - () => new RedisAdapter({ client: createInMemoryRedis() }), + () => + new RedisAdapter({ client: createInMemoryRedis(), prefix: "contract:" }), ); diff --git a/packages/redis/tests/unit/redis-adapter.test.ts b/packages/redis/tests/unit/redis-adapter.test.ts index 05c0212..677560d 100644 --- a/packages/redis/tests/unit/redis-adapter.test.ts +++ b/packages/redis/tests/unit/redis-adapter.test.ts @@ -119,20 +119,24 @@ describe("RedisAdapter", () => { expect(result!.expiresAt).toBe(expiresAt); }); - it("returns null and deletes the key on corrupt JSON", async () => { + it("returns null on corrupt JSON without deleting the key", async () => { await mockRedis.set("bad", "{not json"); const result = await adapter.get("bad"); expect(result).toBeNull(); - expect(mockRedis.del).toHaveBeenCalledWith("bad"); + // Reads must not delete: it would race a concurrent writer, and with an + // empty prefix it would reach keys this adapter does not own. + expect(mockRedis.del).not.toHaveBeenCalled(); }); - it("deletes entries whose embedded expiresAt has passed and returns null", async () => { + it("returns null for an entry whose embedded expiresAt has passed, without deleting it", async () => { await mockRedis.set( "k", JSON.stringify({ value: "v", expiresAt: Date.now() - 1000 }), ); expect(await adapter.get("k")).toBeNull(); - expect(mockRedis.del).toHaveBeenCalledWith("k"); + // The envelope check is a clock-skew backstop; Redis owns the real + // expiry, so a fast-clocked reader must not evict for everyone else. + expect(mockRedis.del).not.toHaveBeenCalled(); }); }); @@ -180,7 +184,29 @@ describe("RedisAdapter", () => { describe("clear", () => { it("should scan for keys matching the prefix and delete them", async () => { - await adapter.clear(); + const prefixed = new RedisAdapter({ client: mockRedis, prefix: "app:" }); + await prefixed.clear(); + expect(mockRedis.scan).toHaveBeenCalled(); + }); + + it("refuses to clear when no prefix is configured", async () => { + await expect(adapter.clear()).rejects.toThrow(/no prefix is configured/); + expect(mockRedis.scan).not.toHaveBeenCalled(); + }); + + it("refuses to flushAll when no prefix is configured", async () => { + await expect(adapter.flushAll()).rejects.toThrow( + /no prefix is configured/, + ); + expect(mockRedis.scan).not.toHaveBeenCalled(); + }); + + it("clears an unprefixed adapter when explicitly opted in", async () => { + const optedIn = new RedisAdapter({ + client: mockRedis, + allowUnprefixedClear: true, + }); + await expect(optedIn.clear()).resolves.toBeUndefined(); expect(mockRedis.scan).toHaveBeenCalled(); }); }); @@ -298,7 +324,7 @@ describe("RedisAdapter", () => { expect(result.has("bad")).toBe(false); }); - it("skips and cleans up a corrupt entry in the middle of a batch", async () => { + it("skips a corrupt entry in the middle of a batch without deleting it", async () => { await adapter.set("a", "v1"); await mockRedis.set("b", "{not json"); await adapter.set("c", "v3"); @@ -306,7 +332,7 @@ describe("RedisAdapter", () => { expect(result.get("a")?.value).toBe("v1"); expect(result.has("b")).toBe(false); expect(result.get("c")?.value).toBe("v3"); - expect(mockRedis.del).toHaveBeenCalledWith("b"); + expect(mockRedis.del).not.toHaveBeenCalled(); }); it("skips entries whose embedded expiresAt has passed", async () => { diff --git a/packages/sqlite/README.md b/packages/sqlite/README.md index edfad12..33854db 100644 --- a/packages/sqlite/README.md +++ b/packages/sqlite/README.md @@ -36,8 +36,11 @@ const user = await cache.wrap(`user:${id}`, async () => api.getUser(id)); ```ts interface SQLiteAdapterOptions { db: Database; // better-sqlite3 database instance - defaultTtlMs?: number; // Default TTL in milliseconds - tableName?: string; // Table name (default: "cache") + defaultTtlMs?: number; // Fallback TTL when a call passes none + maxTtlMs?: number; // Upper bound applied to every entry + tableName?: string; // Table name (default: "ziggurat_cache") + namespace?: string; // Key namespace within the table (default: "") + busyTimeoutMs?: number; // Wait for a competing writer (default: 5000) } ``` diff --git a/packages/sqlite/src/sqlite-adapter.ts b/packages/sqlite/src/sqlite-adapter.ts index aefd560..3e51724 100644 --- a/packages/sqlite/src/sqlite-adapter.ts +++ b/packages/sqlite/src/sqlite-adapter.ts @@ -24,6 +24,13 @@ export interface SQLiteAdapterOptions extends AdapterTtlOptions { db: Database.Database; tableName?: string; namespace?: string; + /** + * Milliseconds a blocked write waits for a competing writer before failing + * with SQLITE_BUSY. Defaults to 5000; set 0 to leave SQLite's default (no + * wait — a concurrent writer fails immediately). Only relevant when several + * connections or processes share the database file. + */ + busyTimeoutMs?: number; } export class SQLiteAdapter extends BaseCacheAdapter { @@ -35,6 +42,8 @@ export class SQLiteAdapter extends BaseCacheAdapter { private readonly stmtGet: Database.Statement; private readonly stmtSet: Database.Statement; private readonly stmtDel: Database.Statement; + private readonly stmtDelExpired: Database.Statement; + private readonly stmtDelCorrupt: Database.Statement; private readonly stmtClear: Database.Statement; private readonly stmtHas: Database.Statement; private readonly stmtGetTtl: Database.Statement; @@ -60,6 +69,12 @@ export class SQLiteAdapter extends BaseCacheAdapter { // Enable WAL mode for better concurrent read performance this.db.pragma("journal_mode = WAL"); this.db.pragma("synchronous = NORMAL"); + // Without this a write that collides with another connection's write + // throws SQLITE_BUSY immediately instead of waiting its turn. + const busyTimeoutMs = options.busyTimeoutMs ?? 5000; + if (busyTimeoutMs > 0) { + this.db.pragma(`busy_timeout = ${String(Math.ceil(busyTimeoutMs))}`); + } // Create table and index if not exists this.db.exec(` @@ -87,6 +102,16 @@ export class SQLiteAdapter extends BaseCacheAdapter { this.stmtDel = this.db.prepare( `DELETE FROM ${this.tableName} WHERE namespace = ? AND key = ?`, ); + // Cleanup-on-read deletes are conditional on the row still being the one + // that was read, so a writer that refreshed the key in between is not + // clobbered. (Safe here in a way it is not for Redis/Memcached: these + // rows are always inside this adapter's own namespace and table.) + this.stmtDelExpired = this.db.prepare( + `DELETE FROM ${this.tableName} WHERE namespace = ? AND key = ? AND expires_at IS NOT NULL AND expires_at <= ?`, + ); + this.stmtDelCorrupt = this.db.prepare( + `DELETE FROM ${this.tableName} WHERE namespace = ? AND key = ? AND value = ?`, + ); this.stmtClear = this.db.prepare( `DELETE FROM ${this.tableName} WHERE namespace = ?`, ); @@ -112,7 +137,7 @@ export class SQLiteAdapter extends BaseCacheAdapter { if (!row) return null; if (row.expires_at !== null && Date.now() >= row.expires_at) { - this.stmtDel.run(this.namespace, key); + this.stmtDelExpired.run(this.namespace, key, Date.now()); return null; } @@ -120,8 +145,8 @@ export class SQLiteAdapter extends BaseCacheAdapter { try { parsed = JSON.parse(row.value) as T; } catch { - // Corrupt/legacy payload — delete and treat as a miss (consistent with Redis/Memcache). - this.stmtDel.run(this.namespace, key); + // Corrupt/legacy payload — drop this exact row and report a miss. + this.stmtDelCorrupt.run(this.namespace, key, row.value); return null; } return { value: parsed, expiresAt: row.expires_at }; @@ -129,6 +154,10 @@ export class SQLiteAdapter extends BaseCacheAdapter { // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-parameters, @typescript-eslint/require-await async set(key: string, value: T, ttlMs?: number): Promise { + // undefined is never stored — JSON.stringify(undefined) is undefined, + // which the driver rejects as a bind parameter. Treat it as a no-op so + // every adapter agrees on what set(key, undefined) means. + if (value === undefined) return; const effectiveTtl = this.resolveTtl(ttlMs); // ttlMs <= 0 means already expired — don't store if (effectiveTtl !== undefined && effectiveTtl <= 0) return; @@ -164,8 +193,8 @@ export class SQLiteAdapter extends BaseCacheAdapter { const remaining = row.expires_at - Date.now(); if (remaining <= 0) { - // Clean up expired entry - this.stmtDel.run(this.namespace, key); + // Clean up expired entry (only if it is still expired — see stmtDelExpired) + this.stmtDelExpired.run(this.namespace, key, Date.now()); return { kind: "missing" }; } @@ -186,7 +215,7 @@ export class SQLiteAdapter extends BaseCacheAdapter { const now = Date.now(); const result = new Map>(); - const corruptKeys: string[] = []; + const corruptRows: Array<{ key: string; value: string }> = []; for (const batch of chunk(keys, MAX_BATCH_PARAMS)) { const placeholders = batch.map(() => "?").join(","); const stmt = this.db.prepare( @@ -204,23 +233,23 @@ export class SQLiteAdapter extends BaseCacheAdapter { try { parsed = JSON.parse(row.value) as T; } catch { - corruptKeys.push(row.key); + corruptRows.push({ key: row.key, value: row.value }); continue; } result.set(row.key, { value: parsed, expiresAt: row.expires_at }); } } - if (corruptKeys.length > 0) { - const deleteCorrupt = this.db.transaction((batches: string[][]) => { - for (const batch of batches) { - const placeholders = batch.map(() => "?").join(","); - const stmt = this.db.prepare( - `DELETE FROM ${this.tableName} WHERE namespace = ? AND key IN (${placeholders})`, - ); - stmt.run(this.namespace, ...batch); - } - }); - deleteCorrupt(chunk(corruptKeys, MAX_BATCH_PARAMS)); + if (corruptRows.length > 0) { + // Delete row-by-row on the exact value read, so a writer that replaced + // the corrupt payload with a good one in the meantime survives. + const deleteCorrupt = this.db.transaction( + (rows: Array<{ key: string; value: string }>) => { + for (const row of rows) { + this.stmtDelCorrupt.run(this.namespace, row.key, row.value); + } + }, + ); + deleteCorrupt(corruptRows); } return result; } @@ -232,6 +261,8 @@ export class SQLiteAdapter extends BaseCacheAdapter { const insertMany = this.db.transaction( (items: readonly CacheSetEntry[]) => { for (const entry of items) { + // undefined is never stored — see set(). + if (entry.value === undefined) continue; const effectiveTtl = this.resolveTtl(entry.ttlMs); // ttlMs <= 0 means already expired — don't store if (effectiveTtl !== undefined && effectiveTtl <= 0) continue; diff --git a/packages/sqlite/tests/unit/sqlite-adapter.test.ts b/packages/sqlite/tests/unit/sqlite-adapter.test.ts index f7b8961..9b90a87 100644 --- a/packages/sqlite/tests/unit/sqlite-adapter.test.ts +++ b/packages/sqlite/tests/unit/sqlite-adapter.test.ts @@ -439,4 +439,89 @@ describe("SQLiteAdapter", () => { } }); }); + + describe("cleanup-on-read races", () => { + // Cleanup deletes are conditional on the row still being the one that was + // read, so a writer landing between the read and the delete survives. + function writeRowDirectly( + key: string, + value: string, + expiresAt: number | null, + ): void { + db.prepare( + "INSERT OR REPLACE INTO ziggurat_cache (namespace, key, value, expires_at) VALUES (?, ?, ?, ?)", + ).run("", key, value, expiresAt); + } + + function rawRow(key: string): { value: string } | undefined { + return db + .prepare( + "SELECT value FROM ziggurat_cache WHERE namespace = ? AND key = ?", + ) + .get("", key) as { value: string } | undefined; + } + + it("removes an expired row on read", async () => { + writeRowDirectly("k", JSON.stringify("v"), Date.now() - 1000); + expect(await adapter.get("k")).toBeNull(); + expect(rawRow("k")).toBeUndefined(); + }); + + it("does not delete a row that was refreshed after the expired read", async () => { + writeRowDirectly("k", JSON.stringify("stale"), Date.now() - 1000); + // Simulate the concurrent writer winning the race: the row is no longer + // expired by the time the cleanup delete runs. + writeRowDirectly("k", JSON.stringify("fresh"), Date.now() + 60_000); + + // A stale in-flight cleanup for the expired read must be a no-op. + await adapter.get("k"); + + const entry = await adapter.get("k"); + expect(entry).not.toBeNull(); + expect(entry!.value).toBe("fresh"); + }); + + it("removes a corrupt row on read", async () => { + writeRowDirectly("k", "{not json", null); + expect(await adapter.get("k")).toBeNull(); + expect(rawRow("k")).toBeUndefined(); + }); + + it("does not delete a corrupt row that was rewritten with valid JSON", async () => { + writeRowDirectly("k", "{not json", null); + writeRowDirectly("k", JSON.stringify("fresh"), null); + + const entry = await adapter.get("k"); + expect(entry).not.toBeNull(); + expect(entry!.value).toBe("fresh"); + expect(rawRow("k")).toBeDefined(); + }); + + it("skips corrupt rows in mget without dropping the good ones", async () => { + await adapter.set("good", "v"); + writeRowDirectly("bad", "{not json", null); + + const result = await adapter.mget(["good", "bad"]); + expect(result.get("good")!.value).toBe("v"); + expect(result.has("bad")).toBe(false); + expect(rawRow("bad")).toBeUndefined(); + expect(rawRow("good")).toBeDefined(); + }); + }); + + describe("busyTimeoutMs", () => { + it("applies a default busy timeout", () => { + const [{ timeout }] = db.pragma("busy_timeout") as [{ timeout: number }]; + expect(timeout).toBe(5000); + }); + + it("honors an explicit busy timeout", () => { + const fresh = new Database(":memory:"); + new SQLiteAdapter({ db: fresh, busyTimeoutMs: 250 }); + const [{ timeout }] = fresh.pragma("busy_timeout") as [ + { timeout: number }, + ]; + expect(timeout).toBe(250); + }); + }); }); diff --git a/scripts/smoke-test.mjs b/scripts/smoke-test.mjs new file mode 100644 index 0000000..e8935b3 --- /dev/null +++ b/scripts/smoke-test.mjs @@ -0,0 +1,88 @@ +/** + * Consumer-perspective smoke test for the built bundles. + * + * The repo's own toolchain needs Node >= 22.13 (pnpm 11 refuses to install on + * anything older), so `pnpm test` cannot run on the Node 20 floor the packages + * advertise in `engines`. This script loads the built output the way a + * consumer does — no pnpm, no vitest, just `node` — which is what lets CI + * verify that floor on the runtime itself. + * + * Run after `pnpm build`: node scripts/smoke-test.mjs + */ +import assert from "node:assert/strict"; +import { createRequire } from "node:module"; +import path from "node:path"; +import { fileURLToPath, pathToFileURL } from "node:url"; + +const require = createRequire(import.meta.url); +const repoRoot = path.join(path.dirname(fileURLToPath(import.meta.url)), ".."); + +// The adapter packages import their backends as types only, so none of them +// pulls a native module at runtime and all six load on any supported Node. +const PACKAGES = ["core", "redis", "memcache", "sqlite", "otel", "nestjs"]; + +const distPath = (pkg, file) => + path.join(repoRoot, "packages", pkg, "dist", file); + +// 1. Both bundles of every package load and export something. +for (const pkg of PACKAGES) { + const esm = await import(pathToFileURL(distPath(pkg, "index.js")).href); + assert.ok(Object.keys(esm).length > 0, `${pkg}: ESM bundle exported nothing`); + const cjs = require(distPath(pkg, "index.cjs")); + assert.ok(Object.keys(cjs).length > 0, `${pkg}: CJS bundle exported nothing`); +} + +// 2. The core API runs, rather than merely parsing. +const { CacheManager, MemoryAdapter } = await import( + pathToFileURL(distPath("core", "index.js")).href +); + +const cache = new CacheManager({ + namespace: "smoke", + layers: [ + new MemoryAdapter({ defaultTtlMs: 5_000 }), // L1: short + new MemoryAdapter({ defaultTtlMs: 60_000 }), // L2: long + ], + syncBackfill: true, +}); +const [l1] = cache.getLayers(); +const nsKey = "smoke:user:1"; + +let factoryCalls = 0; +const results = await Promise.all( + Array.from({ length: 5 }, () => + cache.wrap("user:1", async () => { + factoryCalls++; + return { id: 1, name: "Alice" }; + }), + ), +); +assert.equal( + factoryCalls, + 1, + "concurrent misses should coalesce into one call", +); +for (const value of results) { + assert.deepEqual(value, { id: 1, name: "Alice" }); +} + +// Evict L1 only: the next read hits L2 and backfills L1 under L1's own policy. +await l1.delete(nsKey); +assert.equal(await l1.get(nsKey), null); + +const backfilled = await cache.get("user:1"); +assert.deepEqual(backfilled.value, { id: 1, name: "Alice" }); + +const l1Ttl = await l1.getTtl(nsKey); +assert.equal(l1Ttl.kind, "expiring"); +assert.ok( + l1Ttl.ttlMs <= 5_000, + `backfill should apply L1's own TTL, got ${l1Ttl.ttlMs}ms`, +); + +await cache.delete("user:1"); +assert.equal(await cache.get("user:1"), null); + +console.log( + `ok — ${PACKAGES.length} packages load (ESM + CJS) and @ziggurat-cache/core works on Node ${process.version}`, +); diff --git a/tsconfig.json b/tsconfig.json index 3c24fa6..3a20584 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -3,10 +3,13 @@ "compilerOptions": { "noEmit": true }, - "include": ["vitest.workspace.ts"], + "files": [], "references": [ { "path": "packages/core" }, { "path": "packages/redis" }, - { "path": "packages/nestjs" } + { "path": "packages/memcache" }, + { "path": "packages/sqlite" }, + { "path": "packages/nestjs" }, + { "path": "packages/otel" } ] } diff --git a/vitest.workspace.ts b/vitest.workspace.ts deleted file mode 100644 index 0ecc442..0000000 --- a/vitest.workspace.ts +++ /dev/null @@ -1,3 +0,0 @@ -import { defineWorkspace } from "vitest/config"; - -export default defineWorkspace(["packages/*/vitest.config.ts"]);