Skip to content

fix: resolve architecture review findings (H1-H3, M1-M6, L1-L6) - #56

Merged
camcima merged 5 commits into
mainfrom
fix/architecture-review-findings
Aug 19, 2026
Merged

fix: resolve architecture review findings (H1-H3, M1-M6, L1-L6)#56
camcima merged 5 commits into
mainfrom
fix/architecture-review-findings

Conversation

@camcima

@camcima camcima commented Aug 19, 2026

Copy link
Copy Markdown
Owner

Fixes the findings from the architecture review in ARCHITECTURE_REVIEW.md (added here): all three high, all six medium, and six of the eight low. Two low findings are deliberately deferred — see the end.

The headline fix is H1: the docs promised that a backfilled layer keeps its own TTL policy, while the code gave it the source layer's remaining lifetime. Following the two-layer pattern the docs recommend — MemoryAdapter({ defaultTtlMs: 30_000 }) over RedisAdapter({ defaultTtlMs: 300_000 }) — L1 entries lived up to 5 minutes instead of 30 seconds. Nothing in the suite covered it, which is how the divergence survived.

What changed

# Finding Fix
H1 Backfill TTL contradicted the docs Backfill applies each target layer's own defaultTtlMs, capped by the source's remaining lifetime. Adapters expose this via a new optional ttlPolicy on CacheAdapter, supplied automatically by BaseCacheAdapter.
H2 Nest token collided with @nestjs/cache-manager Token value is now "ZIGGURAT_CACHE_MANAGER". The exported constant keeps its name.
H3 vitest.workspace.ts broken under Vitest 4 Removed; root tsconfig becomes solution-style and references all six packages instead of three.
M1 Redis clear() wiped the whole database Throws when no prefix is set, unless the new allowUnprefixedClear opts in.
M2 wrap() blocked on every layer write New wrapWrites: "background" resolves as soon as the factory does. Default unchanged.
M3 Read repair raced concurrent writers Redis/Memcached reads report a miss and no longer delete. SQLite keeps cleanup (its rows are unambiguously its own) but the deletes are now conditional on the row still being the one that was read.
M4 mget partial-failure differed per adapter Partial results are the contract; BaseCacheAdapter.mget no longer rejects the batch. Writes still reject.
M5 set(key, undefined) did 4 different things A no-op everywhere, leaving any existing value untouched.
M6 Node 20 claimed but never tested New compat job builds on Node 22, then loads the built bundles on Node 20 and 24 with plain node.
L1–L6 Doc drift, dead ZIGGURAT_OPTIONS export, missing otel coverage upload, unnamespaced otel metrics, duplicate-key mget miscount, missing SQLite busy_timeout All fixed.

custom-adapters.md also taught an example that could not compile — it implemented the bare CacheAdapter interface with four of its twelve members — and pointed at an import path the package doesn't ship. It now builds on BaseCacheAdapter, and the contract-suite section says plainly that the suite isn't published for external adapters yet.

Breaking changes

All are 0.x behavior corrections, each marked with a BREAKING CHANGE: footer:

  1. RedisAdapter.clear()/flushAll() throw without a prefix (opt out with allowUnprefixedClear: true).
  2. Redis/Memcached reads no longer delete expired or corrupt keys.
  3. BaseCacheAdapter.mget returns partial results instead of rejecting.
  4. set(key, undefined) is a no-op rather than storing, throwing, or being stored-but-unreadable.
  5. Backfilled entries honor the target layer's defaultTtlMs.
  6. CACHE_MANAGER token value changed — only affects code that hardcoded the string rather than importing the constant. ZIGGURAT_OPTIONS (exported but never provided by anything) is gone.

Verification

  • 637 unit/contract/integration tests pass (up from 594) — lint, format, and typecheck clean.
  • Node 20 floor confirmed by running scripts/smoke-test.mjs against real Node 20.19.1, 22.22.2, and 24.18.1 locally.
  • Functional suites run against real backends: Redis 111 passed, Memcached 97 passed, SQLite 96 passed, via docker compose.
  • New regression tests cover every behavioral change, including six for H1 alone (per-layer TTL, source-lifetime cap, permanent sources, mget backfill, and independent TTLs per layer in a three-layer stack).
  • The undefined and TTL rules are pinned in the shared contract suite, so all four adapters are held to them.
  • Job names validate and test are unchanged, so the 8 required status checks still resolve; compat is additive.

A note on the compat job

My first attempt at M6 ran the full toolchain on each Node version and failed on Node 20 in 15 seconds — pnpm 11 refuses to install below Node 22.13, and the repo's own engines says the same. The >=20 floor simply cannot be checked that way.

The job now builds on Node 22 and then loads the built bundles under Node 20 and 24 with plain node, which is what a consumer actually does. scripts/smoke-test.mjs loads every package's ESM and CJS bundle — no adapter pulls a native module at runtime, since the backends are type-only imports — and exercises core end to end: coalescing, layered reads, backfill under the target layer's TTL policy, and delete. That turns out to test the published claim more directly than running the dev suite would have.

Deferred

  • L7 — publishing the contract suite as @ziggurat-cache/adapter-testkit. It's a new published package (build config, release wiring, docs) and belongs in its own PR. custom-adapters.md now states the current limitation honestly instead of pointing at a path that doesn't resolve.
  • L8 — API/perf polish (del() alias, native Redis EXISTS/PTTL for has/getTtl, a debug hook for swallowed listener errors). Churn without a correctness payoff; better bundled with the 1.0 API pass.

The five architectural observations in the review (no adapter lifecycle/dispose contract, single-cache-only Nest module, per-process-only stampede protection, envelope expiry bookkeeping, getTtl first-layer semantics) are roadmap items for the 1.0 discussion, not defects, and are untouched here.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MsFZHTnQmAUMjtLUegFRPL

camcima and others added 4 commits August 19, 2026 09:16
Addresses the high/medium behavioral findings from the architecture
review. Each change is pinned by tests, including the backfill TTL
behavior that nothing in the suite previously covered.

Backfill TTL (H1): the manager passed the source entry's remaining
lifetime as an explicit ttlMs, which — since explicit TTLs began winning
over defaultTtlMs — meant a backfilled layer inherited the source's
lifetime instead of applying its own. Following the documented two-layer
pattern, an L1 with a 30s default silently held entries for L2's 5
minutes. Backfill now applies each target layer's own defaultTtlMs,
capped by the source's remaining lifetime, so a layer keeps its
staleness budget and a copy never outlives its source. Adapters expose
this through a new optional `ttlPolicy` on CacheAdapter, supplied
automatically by BaseCacheAdapter.

Redis clear() blast radius (M1): with no prefix configured, clear() and
flushAll() scanned and deleted every key in the database. They now throw
unless the new `allowUnprefixedClear` option opts in.

Read repair (M3): Redis and Memcached reads deleted keys they found
expired or unparseable. That races a concurrent writer, and with an
empty prefix it reaches keys the adapter does not own — a reader with a
fast clock could evict entries the rest of the fleet still sees. Both
now report a miss and leave the key for the next set() to replace.
SQLite keeps its cleanup, since those rows are unambiguously its own,
but the deletes are now conditional on the row still being the one that
was read.

mget partial failure (M4): BaseCacheAdapter.mget rejected the whole
batch if any single key failed, which made CacheManager skip the entire
layer; Redis already returned partial results. Partial results are now
the contract. Writes keep rejecting so a failed mset/mdel is still
reported as a layer failure.

undefined values (M5): set(key, undefined) previously did four different
things across four adapters, including throwing on SQLite. It is now a
no-op everywhere, leaving any existing value untouched.

Also adds `wrapWrites: "background"` so wrap() can resolve without
waiting on slow layer writes (M2), stops mget from counting duplicate
keys as extra misses (L5), and sets a SQLite busy_timeout so a competing
writer waits its turn instead of failing with SQLITE_BUSY (L6).

BREAKING CHANGE: RedisAdapter.clear()/flushAll() now throw when no
prefix is configured; pass allowUnprefixedClear: true to keep wiping the
whole database. Redis and Memcached reads no longer delete expired or
corrupt keys. BaseCacheAdapter.mget returns partial results instead of
rejecting. set(key, undefined) is a no-op on every adapter rather than
storing, throwing, or being stored-but-unreadable. Backfilled entries
now honor the target layer's defaultTtlMs instead of the source layer's
remaining TTL.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MsFZHTnQmAUMjtLUegFRPL
The CACHE_MANAGER token's value was the bare string "CACHE_MANAGER" —
the same value @nestjs/cache-manager uses. Since ZigguratModule
registers globally, an app using both packages had two global providers
competing for one token, and which manager a consumer received depended
on module resolution order. The value is now "ZIGGURAT_CACHE_MANAGER";
the exported constant keeps its name, so code importing the constant is
unaffected.

Drops ZIGGURAT_OPTIONS, which was exported but never provided by
anything — API surface promising something that did not exist.

OTel metrics now carry a cache.namespace attribute when the instrumented
manager has a namespace, so two managers sharing a meter (a "users"
cache and a "products" cache) are no longer indistinguishable.

BREAKING CHANGE: the CACHE_MANAGER token value changed from
"CACHE_MANAGER" to "ZIGGURAT_CACHE_MANAGER". Code that hardcoded the
string instead of importing the constant must be updated. The unused
ZIGGURAT_OPTIONS export has been removed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MsFZHTnQmAUMjtLUegFRPL
vitest.workspace.ts called defineWorkspace, which Vitest 4 no longer
exports (workspace files were deprecated in v3 and removed in v4). CI
never noticed because turbo runs tests per package, so the file was both
broken and unused — it only bit contributors running vitest from the
repo root. The root tsconfig existed largely to typecheck it; it becomes
a solution-style config and now references all six packages instead of
three.

Adds a `compat` job running the suite on Node 20 and 24. The required
`test` job pins Node 22, so nothing exercised the `engines: >=20` floor
that published packages advertise. It is a separate job rather than a
matrix on `test` so the required check names stay stable.

Also adds the otel package to the Codecov file list, which had been
silently omitted.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MsFZHTnQmAUMjtLUegFRPL
The docs promised that a backfilled layer "always uses its own TTL
policy" while the code copied the source layer's remaining lifetime, and
core-concepts.md still described the pre-flip TTL precedence
(defaultTtlMs beating an explicit ttlMs) that api-reference.md already
contradicted. Both now describe what the code does, alongside the new
allowUnprefixedClear, wrapWrites, and busyTimeoutMs options and the
uniform undefined-is-never-stored rule.

Fixes the drift the review turned up: the functional-tests.yml workflow
referenced by advanced-usage.md and docker-compose.yml does not exist
(those jobs live in ci.yml); memcache-adapter.md described a
CacheManager.keys() that has never existed; and redis-adapter.md claimed
clear() uses KEYS while contradicting itself two sections earlier.

custom-adapters.md taught an example that could not compile — it
implemented the bare CacheAdapter interface with four of its twelve
members — and pointed at an import path the package does not ship. It
now builds on BaseCacheAdapter, and the contract-suite section says
plainly that the suite is not yet published for external adapters.

Adds ARCHITECTURE_REVIEW.md, the review these changes respond to, kept
as written with a status note recording what has since been fixed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MsFZHTnQmAUMjtLUegFRPL
Copilot AI lite review requested due to automatic review settings August 19, 2026 13:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (5b6fb81) to head (962275f).

Additional details and impacted files
@@             Coverage Diff             @@
##             main       #56      +/-   ##
===========================================
+ Coverage   99.85%   100.00%   +0.14%     
===========================================
  Files          11        11              
  Lines         668       695      +27     
  Branches      153       167      +14     
===========================================
+ Hits          667       695      +28     
+ Misses          1         0       -1     
Flag Coverage Δ
unit 100.00% <100.00%> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

The compat job I added ran the full toolchain on each Node version, which
fails on Node 20 before reaching any test: pnpm 11 refuses to install
below Node 22.13, and the repo's own `engines` field says the same. So
the >=20 floor the published packages advertise cannot be checked that
way at all.

It builds on Node 22 now and then loads the built bundles under Node 20
and 24 with plain `node` — no pnpm, no vitest — which is what a consumer
actually does. The smoke test loads every package's ESM and CJS bundle
(no adapter pulls a native module at runtime; their backends are
type-only imports) and exercises core end to end: coalescing, layered
reads, backfill under the target layer's TTL policy, and delete.

Verified locally against real Node 20.19.1, 22.22.2, and 24.18.1.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MsFZHTnQmAUMjtLUegFRPL
@camcima
camcima merged commit c744492 into main Aug 19, 2026
13 checks passed
@camcima
camcima deleted the fix/architecture-review-findings branch August 19, 2026 13:27
camcima added a commit that referenced this pull request Aug 19, 2026
The resolution note said L7, L8, and the architectural observations were
still open without saying where to follow them, which is the same kind of
drift the review's own L1 finding was about. Points each at its issue,
and records why two of the observations need no tracker: getTtl()'s
first-layer semantics are documented now, and the envelope-expiry
duplication was largely resolved when #56 made that check advisory.


Claude-Session: https://claude.ai/code/session_01MsFZHTnQmAUMjtLUegFRPL

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants