Skip to content

feat(cache): canonical row cache keys + alias entries + table generation tokens - #28

Open
alexstandiford wants to merge 26 commits into
mainfrom
feat/canonical-row-cache-keys
Open

feat(cache): canonical row cache keys + alias entries + table generation tokens#28
alexstandiford wants to merge 26 commits into
mainfrom
feat/canonical-row-cache-keys

Conversation

@alexstandiford

@alexstandifordalexstandiford commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Problem

The datastore trait cached the same row under different keys depending on how it was asked for:

  1. getModels() read-back keyed by table identity (from findIds() SQL, table order); cacheItems() keyed by $model->getIdentity() — the model's identity, which is not necessarily the table's (Siren's Config model identity omits the tenant column); updateCompound($ids) invalidated only the caller's compound key. A writer cannot enumerate every shape a reader may have used → GET-after-PUT served pre-update rows until TTL; a revoked API key kept authenticating from cache (observed on Siren SaaS against real Redis).
  2. No table-level invalidation primitive existed — every fix so far (fix(cache): invalidate identity-keyed entries on compound-key updates #26, Novatorius/siren#195) was per-key enumeration containment.
  3. The cache-aside race (slow reader SETs a stale row back after the writer invalidated) had no close that doesn't need transactions — and WordPress cannot use transactions.

Design

Canonical row keys. A row has exactly ONE cache entry, keyed by its table identity derived from row data: getFieldsForIdentity() fields, table-declared order, scalars stringified. Every context carries the table name alongside the model class — nothing enforces a 1:1 model-to-table mapping, so two tables reusing one model can never cross-serve rows with coinciding identities. cacheItems() and getCacheContextForItem() are gone. The shape follows the documented Novatorius class patterns: RowCacheContextAdapter (lib/Adapters) is a pure Adapter converting rows, identities, lookup keys, and models into the cache-context vocabulary (row/alias/table/generation contexts, canonical + raw identity projections, identity equality keys, generation-token formats, tri-state lookup verification) — no I/O, publicly testable. The datastore handler trait orchestrates the cache, driving the existing CacheableService with contexts the adapter builds: generation snapshots and bumps, guarded read-throughs, swallow-and-log entry mutations, alias resolution, and ephemeral-token skipping. DatabaseServiceProvider's constructor is unchanged; the only edit is a guidance docblock on its $cacheableService property. Contexts are canonical by construction — the test suite runs a deliberately order-sensitive md5(serialize()) policy to prove no normalizing policy is rescuing them.

Alias entries. A business-key lookup (findFromCompound() with anything that isn't the table identity) resolves through an alias entry: {type, table, alias: ksorted-lookup-key} → canonical identity, then the row entry. Aliases store identities, never data, and self-heal two ways: a dead target misses and falls to the DB, and every alias-resolved hit is verified against the caller's lookup values via the model adapter — so an identity-keyed update that rotates a business key can't leave its alias serving fresh-looking rows for a key they no longer carry. updateCompound() invalidates the row entry + alias as free byproducts of its existing pre-read — no extra query, no enumeration (what #26 tried to do with a resolution query).

Table generation tokens (the WP last_changed pattern). Every context carries a per-table opaque token; every writer replaces it. All prior entries become unreachable at once: O(1) whole-table invalidation (estimatedCount now invalidates correctly), and the late stale SET lands under the old generation where no future reader can compute its key — the race closes without transactions. Each operation takes ONE generation snapshot before touching the database and threads it through every context it builds — that snapshot is both the race fence (a stale write-back keyed pre-write can never collide with post-bump reader keys) and the round-trip bound (one token fetch per operation, not per row). Per-table opt-out: shouldUseTableGenerations(). Opted-out tables keep correct point invalidation: precise per-key deletes, read-time alias verification, and precise set-level (estimatedCount) invalidation on every write — the documented residual cost is only the TTL-bounded cache-aside read-back race.

Also fixed in passing

deleteWhere() previously deleted by the model's identity — on tables where that is a subset of the table identity, the SQL delete could reach rows outside the matched set. It now resolves identity rows via findIds() and deletes by the full table identity. RecordDeleted broadcasts for every deleted row with the raw identity row — deletions listeners never hear about are silent failures, and cache-key normalization stays out of the event contract. On every write path, post-commit cache work is best-effort: post-write invalidation never throws (deleteWhere additionally guarantees it lands via finally even when a mid-loop SQL delete fails), cache failures are logged instead of failing committed writes, and RecordCreated/RecordUpdated/RecordDeleted broadcasts always fire for committed writes under cache outages (bulk delete emission also isolates throwing listeners per-event; single-record broadcasts intentionally keep the pre-PR propagate contract) — a transient cache outage can no longer suppress events, fail committed writes, or break reads. Every cache entry mutation on the datastore handler swallows-and-logs failures (the obligation is documented on each helper); reads treat cache failure as a miss; the read-throughs use a capture-guard that serves the loaded value when only the post-load store failed, propagates domain exceptions untouched, and loads directly when the probe itself broke. Proven by tests driving reads and all three write paths through a throwing cache. updateCompound's SQL WHERE pins the resolved table identity together with the caller's lookup fields — the identity prevents non-unique-key fan-out, the caller's fields turn a concurrent key rotation into a no-op write, and an unresolvable record throws RecordNotFoundException. (Known limitation: QueryStrategy::update() returns void, so a rotation-noped write still broadcasts RecordUpdated.) Duplicate self-matches filter by table identity, so a true duplicate on a different row sharing a subset model identity is still caught. deleteWhere buffers its RecordDeleted broadcasts until after the SQL work, with per-event isolation. create() deliberately does NOT pre-warm the cache: the attribute-hydrated model carries request-typed scalars (#29) and a pre-warm keyed under create's own bump can seed the newest generation with a row a concurrent writer already overwrote — the first read after create costs one round-trip and is always correct.

Breaking / downstream migration

  • getCacheContextForItem(), cacheItems(), and hydrateItems() are removed from the trait (hard breaks, no shims). DatabaseServiceProvider's constructor is unchanged.
  • Event contract changes: RecordDeleted and RecordUpdated broadcast the handler's configured model class (previously the runtime $item::class/$record::class), and RecordDeleted carries the full raw table-identity row with driver-typed values (previously the model's getIdentity()). Listeners keyed on the old shapes must adjust.
  • getEstimatedCount()'s cache operation is now Operation::Read (previously the literal 'estimatedCount') — a CachePolicy::shouldCache() branching on that string changes behavior.
  • maybeThrowForDuplicateUniqueFields() is renamed to maybeThrowForDuplicateUniqueFieldsExcluding() with new semantics (self-matches filter by canonical TABLE identity, model identity as fallback) — stale call sites fail loudly instead of silently misfiltering.
  • composer.json now declares php >= 8.2 with the composer platform pinned to match (phpnomad/chrono, a hard dependency, requires >= 8.2 in every release — older lock files were hiding it; PHP 8's locale-independent float casting also matters for cache keys).
  • The typed-hydration follow-up for create()'s returned model is tracked as create()'s cache pre-warm carries request-typed scalars instead of DB column types #29. Siren: delete the cache overrides in TenantScopedDatabaseDatastoreHandler (the #195 containment) when adopting — the tenant column is part of the table identity, so the decorated/bare split disappears by construction.
  • Key shapes change → one-time cold cache on deploy.

Testing

  • New CanonicalRowCacheTest drives the trait through a realCacheableService + live array strategy: table-identity (not model-identity) keying, business-key update invalidating the row entry (the production bug), alias hit avoiding re-query, stale-alias self-heal, generation bump orphaning a late stale write-back, deleteWhere by full identity. A poisoned or old-format entry — canonical or alias, string garbage, wrong-field arrays, identity subsets — is never served or dereferenced: the slot is repaired from the database and the next lookup is query-free (both generation modes). Direct-store paths (alias writes, list-read write-backs) are proven to write nothing under an ephemeral outage token.
  • Audit-driven additions: partial-identity rows never become cache keys (and warn), estimatedCount invalidates after writes in BOTH generation modes, healed aliases serve lookups query-free, rotated business keys never serve from their old alias, the first read after create() is DB-true (there is deliberately no pre-warm), and deleteWhere coverage includes broadcast payloads and the no-match path. Canonical behavior tests run under both generation modes via data providers.
  • Legacy trait tests updated to the new context shape; phpnomad/event is a declared runtime dependency (replacing hand-rolled interface shims); reusable doubles live in tests/Doubles. Direct unit coverage for the adapter's pure conversions lives in RowCacheContextAdapterTest; outage behavior is covered for reads and all write paths via a throwing-cache double at the datastore-handler level. Read-side outage behavior is proven with a read-failing cache double (canonical reads, alias lookups, estimatedCount, and generation read-blips that must not clobber healthy tokens). Failure paths are proven end to end: read outages, write outages, mid-loop SQL failures in bulk deletes, throwing listeners, generation read-blips (ephemeral tokens skip caching entirely — stores no-op and read-throughs go straight to the database — so a sustained blip can't fill the cache with unreachable garbage). Full package suite: 95 tests, 198 assertions, green on the repo's CI (PHPUnit gate and spellcheck green). PHPStan: the new files (adapter, tests, doubles) are level-9 clean and the repo total improves on main (155 vs 233); the residual is pre-existing legacy flow and legacy interface signatures, re-reported per consuming class.

🤖 Generated with Claude Code

alexstandifordand others added 7 commits July 11, 2026 23:23
…ation tokens
A row now has exactly one cache entry, keyed by its table identity
derived from row data — never from the model's getIdentity() and never
from the shape of the caller's lookup. Business-key lookups resolve
through alias entries (pointers to the canonical identity) that
self-heal when stale. Writers can therefore always name the keys
readers used: updateCompound invalidates the row entry and alias as
free byproducts of its existing pre-read, with no extra query.
Every context is additionally keyed under a per-table generation token
(the WordPress last_changed pattern) that writers replace on every
create/update/delete. This closes the cache-aside race where a slow
reader writes a stale row back after invalidation — without requiring
transactions, which WordPress cannot use — and gives the package O(1)
whole-table invalidation (estimatedCount now invalidates correctly).
Tables can opt out via shouldUseTableGenerations().
deleteWhere now deletes by the full table identity instead of the
model's identity, which on tables where the model identity is a subset
of the table's could previously reach rows outside the matched set.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ation
The audit's key finding: cacheRow() re-fetched the generation token at
SET time, so on the getModels() and business-key read-back paths a
writer bumping between the reader's SELECT and its cache write handed
the reader the NEW token — the stale row landed under the new
generation and the race the tokens exist to close stayed open. Every
operation now takes one generation snapshot BEFORE touching the
database and threads it through all context builds, which also drops
token fetches from two-per-row to one-per-operation.
Also from the audit:
- deleteWhere always broadcasts RecordDeleted with the raw identity
row — a deletion listeners never hear about is a silent failure, and
cache-key normalization must not leak into the event contract.
- resolveTableIdentity falls back to a DB resolution for
generation-disabled tables when the alias was evicted; for those
tables the precise delete is the only invalidation there is.
- phpnomad/event is now a dev dependency; the hand-rolled Events
interface shims are gone (the real interface immediately caught the
shim drift the reviewers predicted) and the no-op query/clause
builders live in shared tests/Doubles.
- estimatedCount passes Operation::Read like every other call site;
dead hydrateItems() removed; stale docblocks on findFromCompound/
getModels/getRowIdentity corrected; shared stringifyScalars()
replaces the duplicated normalization loop.
- New tests: partial-identity rows never become cache keys (and warn),
estimatedCount invalidates after a write, healed aliases serve
lookups query-free.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…store trait
The audit's remaining major: ~200 lines of cache-key policy (canonical
identity derivation, alias contexts, generation mint/read/bump) had
accreted directly into WithDatastoreHandlerMethods — the exact
trait-as-method-dump failure mode the anti-patterns doc warns about.
That vocabulary now lives in DatastoreRowCache, a per-table service the
trait builds lazily via DatastoreRowCacheFactory on the
DatabaseServiceProvider (exposed like cacheableService, so containers
can swap row-cache behavior without touching the trait). The provider's
new constructor parameter is optional and self-defaults, so existing
six-argument construction keeps working. The trait keeps only its
orchestration: shouldUseTableGenerations() as the per-handler override
point, and cacheRow() as the one set-side write.
Also from the audit: the four canonical-behavior tests now run under
BOTH generation modes via a data provider — production defaults to
generations on, and previously only the race test exercised that path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… deleteWhere
Round-3 audit findings.
The major: an identity-keyed update can rotate a business key out from
under its alias, and on generation-disabled tables nothing ever noticed
— the alias kept serving fresh-looking rows for a key they no longer
carried (the revoked-API-key bug class, reintroduced for opt-out
tables). findFromCompound now verifies alias-resolved rows still carry
the caller's lookup values via the model adapter; a mismatch drops the
alias and re-resolves from the database. Fields the adapter does not
expose are skipped rather than punished.
Restored the deleteWhere coverage that was lost in the round-1 test
rewrite, and extended it: full-table-identity deletes, RecordDeleted
broadcast payloads (including when no cache identity can be derived),
and the no-match path leaving the cache untouched. deleteWhere's
generation bump now sits in a finally block so rows deleted before a
mid-loop SQL failure still invalidate set-level caches.
Also from the audit:
- RowCache interface (lib/Interfaces) so the factory and trait depend
on a contract; DatastoreRowCache is the default implementation and
now owns ALL context mutations (storeRow/storeAlias/deleteRow/
deleteAlias/deleteTableContext) — the trait never hands a service-
built context to the raw CacheableService.
- Generation-disabled tables get precise set-level invalidation on
every write (estimatedCount previously lived until TTL for them),
and the opt-out docblock now states the full cost.
- phpnomad/event moved to require with a caret constraint — the
provider imports its interfaces at runtime.
- Reusable test doubles promoted to tests/Doubles; stale/stacked
docblocks removed; create()'s bump-then-pre-warm ordering pinned by
a read-without-query test.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… contract extraction
Round-4 audit findings.
The major (reproduced by the reviewer with a throw-on-delete cache):
updateCompound had no guard around its post-update cache work, so a
transient cache failure after the DB write committed skipped both the
generation bump and the RecordUpdated broadcast — stale reads plus a
silent event drop on exactly the failure path deleteWhere already
defended. All three write paths now treat post-commit cache work as
best-effort: failures are logged, the bump runs in a finally (via
invalidateAfterWriteSafely(), which never throws and cannot mask an
in-flight exception), and event broadcasts always fire for committed
writes.
Contract completion, from the converged reviewer findings:
- RowCache::readRow() read-through — the last cache write (getWithCache
miss-path SET) moves behind the service contract.
- RowCache::matchesLookup() — alias verification moves off the trait
into the collaborator (which now takes the ModelAdapter); when
generations are off and NO lookup field is verifiable, the alias is
treated as stale with a warning instead of passing vacuously.
- RowCache::invalidateAfterWrite() — the bump-or-precise-delete pairing
becomes one service call instead of hand-repeated trait knowledge.
- RowCacheFactory interface; the provider's factory parameter is now
REQUIRED and interface-typed (two reviewers converged on the
package's hard-break precedent over the self-defaulting shim).
- RecordUpdated and RecordDeleted both broadcast $this->model as the
event type; get-then-set generation minting documents its safety
argument; docblock dedup via @inheritdoc; README covers the new
provider member and the generations opt-out trade.
Tests: estimatedCount twins merged into the generation-mode provider;
RecordingEventStrategy::ofType() replaces inline filters; the two
mock-only create tests assert the returned model. 38 tests, 99
assertions, green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Round-6 audit findings — five of six reviewers independently converged
on the same residual seam, and the testing lens correctly flagged that
round 5's failure-path guarantees shipped without proof.
The boundary, finished: hasRow() and readTableValue() join the RowCache
contract, getEstimatedCount() and getModels() route through them, and
the trait now holds ZERO direct cache access — a swapped RowCache
implementation intercepts every read and mutation. The interface is
simultaneously trimmed to what consumers call: identityContext,
aliasContext, tableContext, deleteTableContext, withGeneration, and
bumpGeneration are implementation plumbing again (protected on
DatastoreRowCache).
Correctness tightening from the code-smells lens:
- matchesLookup() on generation-disabled tables now treats ANY
unverifiable lookup field as stale (it is exactly the field a rotation
may have changed), not just all-unverifiable — and the vacuous-pass
case is covered by tests with a nothing-exposing adapter.
- updateCompound()'s SQL now targets the RESOLVED table identity, not
the caller's business key: the contract is one record (limit(1)
pre-read, one RecordUpdated), so a non-unique business key can no
longer fan the write out past what invalidation saw.
- Generation minting persists its token best-effort: a cache that
throws on writes degrades caching to disabled (fresh token per
operation, reads fall to the DB) instead of breaking datastore reads.
- create() logs bump failures and pre-warm failures distinctly.
Proof for the round-5 claims: a FlakyCacheStrategy double drives
create/updateCompound/deleteWhere through a mid-operation cache-write
outage and asserts committed writes return, events still broadcast, and
failures land in the log. New DatastoreRowCacheTest unit-covers the
pure helpers (identity derivation and ordering, identity-shape checks,
malformed alias values, lookup verification in both generation modes).
Phantom docblock reference fixed; healing-sequence test setup deduped.
56 tests, 128 assertions, green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ead-side outages
Round-8 audit findings (five passes, one concerns).
The major: resolveTableIdentity() gave up on generation-enabled tables
when the alias was missing, so updateCompound()'s SQL fell back to the
caller's business key — reopening the multi-row fan-out for non-unique
keys on any alias eviction. Resolution now always falls through to the
database, and the trait's last usesGenerations() branch disappears with
it (the method left the RowCache contract entirely).
Read-side resilience, matching what the PR already claimed: alias
reads, row probes, and generation-token reads treat ANY cache-layer
failure as a miss (DB fallback / fresh mint) instead of breaking the
operation. deleteWhere buffers RecordDeleted broadcasts and emits them
after the SQL work with per-event isolation — a throwing listener can
no longer abort a bulk delete mid-set, and rows deleted before a
mid-loop SQL failure still announce themselves. findIds() now resets
the shared query builder — stale builder state feeding a DELETE's row
selection was a pre-existing smell this PR made load-bearing.
Contract hygiene: rowContext() demoted to protected (tests probe
context slots via a shared ExposedRowCache double instead of widening
the production surface); interface docblocks state the pre-query
snapshot requirement on all alias methods, rowIdentity's unlogged
null case, and the single-slot nature of the table context; the
RecordUpdated payload choice (caller's key, not resolved identity) is
now documented at the call site; matchesLookup's type-insensitivity is
pinned by cross-type data-provider cases it previously never exercised.
The attribute-typed pre-warm follow-up is filed as a repo issue so it
can't get lost.
60 tests, 131 assertions, green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
alexstandifordand others added 2 commits July 12, 2026 02:49
Round-10 audit findings (five passes; code-smells reproduced one major).
The major: read paths performed unguarded cache MUTATIONS — the
miss-path storeAlias/storeRow and the self-heal deleteAlias — so a
write-failing cache backend broke reads (a rotated-alias lookup
surfaced RuntimeException instead of RecordNotFoundException). Every
RowCache mutation now swallows-and-logs cache failures, and the
readRow/readTableValue read-throughs use a capture-guard that
distinguishes three outcomes: fallback loaded then the store threw →
serve the loaded value; the fallback itself threw → propagate the
domain exception untouched; the cache probe threw before the fallback
ran → load directly. The no-throw obligation is now stated on the
RowCache contract itself, since the consuming trait calls everything
unguarded — and the trait's now-dead defensive try/catch blocks are
gone.
create()'s cache pre-warm is REMOVED, not repaired. Two audit rounds
found real hazards in it: request-typed scalars diverging from column
types (#29), and this round's race — a pre-warm keyed under create's
own post-insert bump can seed the newest generation with a row another
writer already overwrote. The first read after create costs one DB
round-trip and is always correct; correctness beats a micro-warm-up.
updateCompound now THROWS RecordNotFoundException when the record
cannot be re-resolved to a table identity mid-operation, instead of
silently degrading the SQL target to the caller's raw business key —
the failure-path fan-out the resolved-identity targeting exists to
prevent.
Also: booleans normalize to '0'/'1' in cache keys ((string) false is
''), the provider's CacheableService is documented as a deliberate
downstream extension surface (the package's own flows no longer touch
it), the interface's stray indentation passes php-cs-fixer, and
ExposedRowCache documents why it deliberately extends a lib/ concrete.
New coverage: cold business-key read under cache-write failure,
rotated-alias lookup surfacing not-found (not cache errors) under
failure, unresolvable update refusing to fan out, and first-read-after-
create being DB-true. 63 tests, 133 assertions, green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…duplicate self-match on business-key updates
Round-12 audit findings — four lenses converged on the same root: the
RowCache contract lists invalidateAfterWrite among the swallow-and-log
mutations, but the default implementation propagated (reproduced under
a failing cache in both generation modes) and only survived because the
trait double-guarded it. The guard now lives in the implementation
where the contract says it is, the trait's redundant wrapper is gone,
and a direct service-level throwing-cache test pins the obligation.
The genuinely new findings:
- updateCompound's duplicate scan filtered self-matches by comparing
each existing model's identity to the CALLER'S key — a business-key
caller re-sending the record's own unique values could never match
itself and tripped a spurious DuplicateEntryException. Self-matches
now compare against the pre-read model's identity, which is
model-to-model and shape-correct by construction.
- getModels() held its freshly queried batch only in the cache, so a
write-dead cache silently degraded one list read into 1+N per-row
queries. Hydrated models are now also held locally; the cache only
serves ids that were skipped as already cached.
- A generation-token read BLIP (failure, not miss) now mints an
ephemeral token without persisting it — readers must not clobber a
healthy token and wholesale-invalidate the table cache during a
read-side outage.
- matchesLookup routes both comparison sides through stringifyScalars,
so lookup verification can never drift from key-normalization rules
(the inline version missed the bool rule: false compared as ''
against a stored '0').
Also: read-path degradation now logs uniformly (hasRow, alias reads);
the read-through failure semantics live on the interface where swapped
implementations will read them; the stale pre-warm rationale is gone
from the bump docs; the factory contract states the generations
trade-off directly; the type-stability regression test moved beside the
other pure-helper tests (its trait-level scaffolding was dead weight).
TableSchemaService's pre-existing unguarded getWithCache is filed as a
follow-up issue.
67 tests, 142 assertions, green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
alexstandifordand others added 8 commits July 12, 2026 04:02
…updates; read-outage proof
Round-14 audit findings — zero majors for the first time; two code-smell
minors, one testing minor, and the accumulated convention nits.
Correctness:
- The duplicate scan's self-match filter now compares TABLE identities
(deriving each candidate's canonical identity through the adapter):
model identities can be a strict subset of the table's, so a true
duplicate on a DIFFERENT row sharing the pre-read record's model
identity could hide behind a model-identity self-match. The
model-identity comparison remains only as the fallback for adapters
that cannot expose a table identity, with the residual shadow
documented. Pinned by a test where org 2's row shares org 1's model
identity and still raises DuplicateEntryException.
- updateCompound's SQL WHERE now pins the resolved identity TOGETHER
with the caller's lookup fields: identity targeting alone reopened a
rotation TOCTOU (resolve, concurrent key rotation, update lands on a
row that no longer carries the key) — merged conditions make that a
no-op write instead. QueryStrategy::update() returning void means a
no-op still broadcasts; documented as a known limitation.
- The read-side outage claims are now proven: FlakyCacheStrategy grew a
failReads switch, and tests drive canonical reads, alias lookups,
estimatedCount, and the generation read-blip (ephemeral token, healthy
token NOT clobbered, entries live after recovery) through a
read-failing cache.
Contract/consistency: identityKey() joins RowCache so identity equality
is defined by its owner (getModels' dedupe map uses it, and skips the
store for underivable rows instead of double-logging); the
snapshot-vs-miss distinction is stated on the contract; collaborator
names align with package conventions ($loggerStrategy, $modelAdapter);
model/adapter fixtures consolidated into tests/Doubles; provider
docblock scopes the extension surface and forbids bypassing RowCache
for row/alias caching; assorted wording fixes (neutral post-write
invalidation phrasing, ANTI-PATTERNS reference, lazy-init rationale).
72 tests, 153 assertions, green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… in SQL conditions
Round-16 audit findings — second consecutive majorless round; three
code-smell minors plus mechanical convergence items.
- A row deleted concurrently between the id query and hydration threw
RecordNotFoundException out of getModels' per-id fallback, which
where()'s catch turned into an EMPTY result — discarding rows that
still exist (reproduced by the reviewer; pre-existing shape, but this
PR rewrote the path). Vanished rows are now skipped per-id; a new
test pins the survivor being served.
- Cache-key stringification no longer leaks into SQL: identity
resolution returns raw caller/row values for the UPDATE's WHERE, and
the cache delete canonicalizes separately via rowIdentity(). Alias-
resolved identities remain the stored canonical strings (the only
form available without a query) — documented, with the caller's raw
business key co-conditioning the WHERE.
- Builder hygiene finished: findIds resets the shared clause builder
(destructive deletes select through it) and the remaining fresh
builds (getModels' batch SELECT, queryRowAndModel) reset the shared
query builder, completing the pattern the round-8 fix started.
- Contract docs: the interface header now defers failure semantics to
each method (it contradicted snapshotGeneration's failure-vs-miss
distinction); bumpGeneration's @return describes the contract rather
than current call-site state; log contexts carry the throwable class;
the stray docblock indent passes cs-fixer.
- Tests: the entry-count ternary moved into a data provider (the
no-conditionals rule); the dual-read outage test split per read path;
logger expectations constrained to their expected messages.
74 tests, 156 assertions, green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… under ephemeral tokens
Round-18 audit findings — five passes (stack elevator fully clean),
zero majors for the third consecutive round.
- deleteWhere's documented guarantees now have proof: a mid-loop SQL
delete failure still lands the post-write invalidation and still
announces completed rows (throw-on-nth-delete double), and a throwing
RecordDeleted listener is isolated per-event without suppressing
siblings (throw-once event double). create()/updateCompound()
broadcasts document that single-record events intentionally keep the
pre-PR propagate contract — there are no siblings to protect.
- Generation read-blips mint PREFIXED ephemeral tokens and storeRow/
storeAlias skip write-backs under them: entries keyed under a token
nobody can ever read are garbage written at read-traffic rate during
a sustained blip.
- findIds() rides initiateQuery() (whose select already defaults to the
identity fields), and the clause-builder reset folds into
initiateQuery() — one bootstrap for every query path instead of a
drift-prone copy.
- composer declares php >= 8.0 (locale-independent float casting in
cache keys; the suite already used 8.0 syntax). Docs: the duplicate-
filter signature repurposing joins the Breaking list; the ExposedRowCache
rationale is stated inline; resolveAliasedIdentity documents its
read-failure null; bumpGeneration's timing wording matches
deleteWhere's partial-failure reality; the read-outage tests collapse
into one provider-driven test.
76 tests, 161 assertions, green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The round-20 repo-consistency audit caught what every earlier round
missed by trusting the local vendor PHPUnit: the repo's phpunit CI
action runs PHPUnit >= 10, which requires STATIC data providers, so the
branch's CI was red with 12 provider errors while 9.6 passed locally.
All seven providers are static now (9.6 accepts both).
The four new lib/ files also analyze clean at the repo's declared
PHPStan level 9: ModelAdapter generics specified, context arrays carry
value types, and the empty-identity guards are gone — the Table
contract declares getFieldsForIdentity() non-empty-array, so the case
was contractually impossible (its contract-violating mock test goes
with it).
From the rest of round 20 (five passes; every fix applied):
- maybeThrowForDuplicateUniqueFields is RENAMED to
maybeThrowForDuplicateUniqueFieldsExcluding: its second parameter had
changed meaning, and a silent signature repurposing should fail
loudly at stale call sites instead of misfiltering quietly.
- Ephemeral generation snapshots now skip caching ENTIRELY: readRow/
readTableValue go straight to their fallback (no guaranteed-miss
round trips, no unreachable miss-path stores) — the docs/code-smells
lenses proved the prior claim only covered storeRow/storeAlias.
Pinned by a service-level test.
- The four mutation signatures require an explicit generation snapshot
(no null default): the race-fence invariant is now enforced by the
signature, not just the docblock.
- matchesLookup's contract documents that adapter exceptions are
domain errors and propagate; deleteRow's param vocabulary aligned;
the last unconstrained logger expectation pinned; a dead failWrites
reset removed; entry-count providers renamed to say what they count.
76 tests, 162 assertions, green locally.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ge ships
Follow-through on the round-20 blocking CI finding:
- php-actions/phpunit's default phar is PHPUnit 11+, which ignores
@dataProvider ANNOTATIONS entirely — static providers alone weren't
enough; provider-driven tests were silently invoked with zero args.
The workflow now pins version 9.6, the major the package's dev
dependencies actually resolve, so CI runs the same PHPUnit the suite
is written for.
- composer's platform is pinned to php 8.2.0 and require.php raised to
>=8.2: phpnomad/chrono (a hard dependency) requires >=8.2 in every
release, so 8.0/8.1 could never actually install this package — the
lock was just hiding it until regenerated. Both workflows run
php 8.2 now (the phpstan job's 8.1 pin was failing composer's
platform check).
- The chronically red spellcheck gate (failing on main for months) gets
its seven missing README words added to .wordlist.txt.
76 tests, 162 assertions green locally on PHPUnit 9.6; the four new
lib/ files are PHPStan level-9 clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every file this PR authors — the four lib/ files, both new test
classes, and all eleven test doubles — analyzes clean at the repo's
declared level 9 (context arrays carry value types, ModelAdapter
generics specified against DataModel, expose helpers return non-null
by contract, event recorder ofType() is generic, model narrowing uses
assertions instead of loose DataModel calls). Also repairs the
.wordlist.txt entry that glued onto a missing trailing newline and
adds the remaining README words the chronically red spellcheck gate
flags on main.
The repo-wide phpstan gate remains red from pre-existing debt in files
this PR doesn't author (the legacy trait test and older lib/ files) —
same as main.
76 tests, 163 assertions, green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e wordlist
The trait's untyped legacy signatures were phpstan's biggest single
source — re-reported once per consuming class, which is how this PR's
new trait consumers pushed the repo count above main's. With the
public surface annotated, the branch analyzes at 194 errors repo-wide
against main's 233: the PR now strictly improves the (still red,
pre-existing) phpstan gate. The remaining debt is legacy mixed-flow
inside untouched methods like buildConditions() — a remediation
project, not this PR.
Spellcheck's remaining three README words join the wordlist.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@alexstandiford

Copy link
Copy Markdown
ContributorAuthor

Audit loop summary

This PR went through ten full rounds of the six-reviewer audit fleet (coding standards, testing standards, repo consistency, stack elevator, documentation, code smells — 60 independent reviews total), with every actionable finding fixed and re-verified between rounds. Highlights of what the loop caught, in order of consequence:

  1. Generation-snapshot race — cache contexts were rebuilt (with a fresh token) at write-back time, so a concurrent writer's bump couldn't fence a slow reader's stale SET. Fixed with one pre-query snapshot per operation, threaded through every context.
  2. Rotated business keys served from live aliases — an identity-keyed update could rotate a business key out from under its alias with nothing noticing on generation-disabled tables. Fixed with read-time lookup verification via the model adapter.
  3. Failure-path outage behavior, end to end — committed writes can no longer be failed (or their events suppressed) by cache outages; reads survive read-side outages; ephemeral tokens skip caching entirely during token-read blips; bulk deletes isolate throwing listeners and guarantee invalidation via finally through mid-loop SQL failures. All proven by failure-injection doubles.
  4. Identity discipline everywhere — SQL updates target the resolved table identity merged with the caller's key (no non-unique-key fan-out, no rotation TOCTOU), duplicate self-matches compare table identities, list reads keep survivors when rows vanish mid-read, and cache stringification stays out of SQL and event payloads.
  5. CI was quietly broken — the phpunit action's default PHPUnit 11 phar ignores @dataProvider annotations (provider tests silently never ran), the lock couldn't install on the workflow's PHP, and spellcheck had been red on main for months. All three addressed: build-test and spellcheck are green; phpstan improves from 233 errors on main to 194 (the residual is pre-existing debt in files this PR doesn't author — every PR-authored file is level-9 clean).

Consciously accepted, documented in code: the provider's cacheableService extension surface (structural narrowing noted as future work), stringified alias identities in SQL WHERE (raw business key co-conditions), the model-identity duplicate-filter fallback for narrow adapters, single-record broadcasts propagating listener exceptions (bulk isolates), QueryStrategy::update()'s void return meaning rotation-noped writes still broadcast (needs an upstream signature change).

Follow-ups filed:#29 (typed hydration of create()'s returned model), #30 (TableSchemaService's unguarded cache reads).

Final state: 76 tests / 163 assertions green on CI, four new lib/ files + all new tests level-9 clean, Siren-side canonicalization already merged as Novatorius/siren#196, and Siren's TenantScopedDatabaseDatastoreHandler containment (#195) is deletable once this ships.

🤖 Generated with Claude Code

alexstandifordand others added 9 commits July 12, 2026 07:35
Alex's review: Novatorius has a closed vocabulary of class types, and
classes outside it are wrong. The prior shape wasn't in it —
DatastoreRowCache mixed two patterns in one class (context conversion +
cache orchestration, violating the one-pattern rule), RowCache was a
13-method speculative contract with a single implementation, and
RowCacheFactory was a factory interface wrapping a trivial new (the
docs reserve factories for constructing complex objects).
The reshape, mechanics unchanged:
- RowCacheContextAdapter (lib/Adapters) — a pure Adapter converting
rows, identities, lookup keys, and models into the cache-context
vocabulary: rowIdentity/rawIdentity/identityKey, row/alias/table/
generation contexts, generation-token formats, and tri-state
matchesLookup. No I/O, no logger; public and directly testable, which
also kills the ExposedRowCache extends-a-lib-concrete exception.
- Cache orchestration returns to the datastore handler trait, driving
CacheableService — the existing documented Service for cache I/O —
directly: generation snapshots/bumps, guarded read-throughs,
swallow-and-log entry mutations, alias resolution, ephemeral-token
skipping, and the unverifiable-lookup policy. Every behavior and
failure guarantee from the audit rounds is preserved and still
covered by the same tests.
- Deleted: RowCache, RowCacheFactory, DatastoreRowCacheFactory,
DatastoreRowCache, ExposedRowCache. DatabaseServiceProvider returns
to its original six-argument constructor — two breaking changes
(provider arg, factory contract) fall out of the PR entirely.
- Tests follow the shapes: RowCacheContextAdapterTest covers the pure
conversions; the trait scenario tests probe cache slots through the
adapter plus a live-token helper.
73 tests, 159 assertions, green; adapter files PHPStan level-9 clean;
repo-wide analysis stays below main (195 vs 233).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…seams
First slow-loop round after the pattern reshape — one reviewer held
every class to the documented vocabulary, the other verified the
refactor against the previously-proven behaviors. Both found real
things:
- The reshape's tri-state matchesLookup had silently WEAKENED the
generation-off rotation defense: it returned unverifiable only when
ZERO lookup fields were exposed, so a lookup with one hidden field
and matching visible fields passed as verified (reviewer reproduced
it). Restored the original strictness — a lookup is verified only
when EVERY field checks out; any hidden field yields unverifiable,
which the gens-off policy treats as stale. Pinned at both the adapter
level and the datastore-flow level with a field-hiding adapter double.
- withGeneration() no longer fails open: a generation-keyed table
refuses (InvalidArgumentException) to build an unkeyed context no
bump could ever orphan.
- The generations flag is captured exactly once, at adapter
construction — the trait's mode checks now read the adapter, so a
dynamic shouldUseTableGenerations() override can't split-brain
invalidation against context keying.
- Token creation moved out of the adapter (creation is orchestration,
not conversion — the adapters doc says pure); the adapter keeps only
token FORMATS (ephemeral prefix, validity), and the README's phantom
RowCacheFactory reference is corrected.
75 tests, 166 assertions, green; adapter and doubles level-9 clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… the datastore trait
Round-2 audit batch:
- Restore tests lost in the pattern reshape: gens-on unverifiable-alias
trust, malformed-alias rejection, ephemeral store no-op, gens-off
write-outage variants, partial-field-hiding strictness (87 tests).
- Narrow types in the trait for phpstan level 9: generation token
handling, canonical read guards against non-model cache values,
typed row extraction, DataModel narrowing in duplicate checks.
- countWhere/deleteWhere inherit the vendor contract docblocks instead
of conflicting local @PARAM shapes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… conventions
Round-3 audit batch:
- A poisoned canonical row entry is now evicted and re-warmed from the
database instead of silently degrading every read of that row to a
query until the generation bumps (mirrors the alias path's handling).
- Adapter conversion methods renamed to the repo's toX() vocabulary
(toRowIdentity, toRowContext, ...); trait accessor renamed to
getCacheContextAdapter().
- HidingModelAdapter renamed OpaqueModelAdapter to stop being one word
away from FieldHidingModelAdapter.
- Logger context key 'exception' renamed 'exceptionMessage'; findBy()
failure message adopts the sibling lookup-key phrasing.
- Convention cleanup: imported InvalidArgumentException, class constant
and double properties hoisted to the top of their class bodies,
use-statement ordering restored, phpstan.yml php_version quoted.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… formats
Round-4 audit batch:
- withGeneration() renamed toGenerationKeyedContext() and demoted to
protected — it is a pure conversion, not a wither, and had no external
callers.
- The adapter can now produce the ephemeral token format it recognizes
(toEphemeralGeneration); the trait mints randomness, the adapter owns
the shape. Round-trip test pins the contract.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Round-5 audit batch (mutation-verified):
- New outage test proves the direct-store paths (alias writes, list-read
batch write-backs) write NOTHING under an ephemeral token — removing
either no-op guard, or minting an unmarked outage token, now fails the
suite. Previously both mutations passed green.
- Malformed-alias coverage is a poison provider (string garbage,
wrong-field array, identity subset) and asserts the slot is repaired,
not just bypassed.
- Blip test asserts byte-for-byte store equality; where() pins model
content; the double expectExceptionMessage() (PHPUnit keeps only the
last) replaced with explicit fragment assertions.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- toRowIdentity() delegates to toRawIdentity() — the projection loop
lived twice after two rounds of accretion.
- Duplicate-filter test comment described the superseded model-identity
mechanism; it now states the table-identity-first policy.
- Cosmetics: dataProvider moved next to its consumer, import order,
missing blank line.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t nits
Round-7 (fresh-eyes) batch:
- Every cache context now carries the table name alongside the model
class: nothing enforces a 1:1 model-to-table mapping, and two tables
reusing one model must never cross-serve rows whose identities
coincide. New adapter test pins it. (Key shape change — the PR already
declares a one-time cold cache.)
- deleteWhere() snapshots the generation before its first query, as the
snapshot invariant documents.
- An entry evicted between the cache service's exists() and get() is a
normal LRU miss, not a failing backend — no more warning noise.
- A poisoned set-level (estimatedCount) slot is bypassed with a direct
database answer instead of being garbage-cast to int.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Round-8 verification confirmed everything else clean; this was the one
mutation that still passed green — reverting the is_numeric guard to a
bare (int) cast went unnoticed. Now a poisoned set-level slot serving a
garbage-cast 0 instead of the database's answer fails the suite.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@alexstandiford