Skip to content

(fix): keep cache owner registrations in one key per collection - #987

Merged
abnegate merged 2 commits into
feat-query-libfrom
fix/cache-owner-key-leak
Sep 28, 2026
Merged

abnegate merged 2 commits into
feat-query-libfrom
fix/cache-owner-key-leak

Conversation

@abnegate

@abnegate abnegate commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

What

The document cache and the find() query cache no longer leave one permanent Redis key behind per write.

Both invalidation handshakes registered each invalidation under its own <collection key>#owner:<token> key and released it with purge(). On the Redis adapters (utopia-php/cache 4.0.2, same in 5.x), purge() runs LUA_PURGE_BUMP: DEL key followed by HSET key __utopia_gen__ <next>. The key is recreated holding only its generation, and the adapter never sets an expiry, so every write left one key per cache for good.

Registrations now go through a small Cache\Owners class shared by both handshakes:

Step Before After
Register save("#owner:$token", $token) save('#owners', $token, $token); if list('#owners') lacks the token, also save("#owner:$token", $token)
Check load("#owner:$token") load() wherever Owners::find() says the token lives
Release purge("#owner:$token") purge() of that registration: the field, or the token's own key
  • Adapters that store fields (Redis, Multiplexing, RedisCluster) list the field, so the token lives in <collection key>#owners and is released by LUA_PURGE_FIELD, which HDELs the field and bumps the key's generation. The key count is bounded by the number of collections, and the hash only holds registrations still in flight.
  • Adapters that store no fields (Memory, Memcached, Filesystem, Hazelcast, None) always return [] from list(), so every token keeps a key of its own exactly as before. Their purge deletes that key, so nothing leaks, and overlapping invalidations of one collection keep independent registrations. Pool, Sharding and CircuitBreaker pass list() through (checked in utopia-php/cache 4.0.2 and 5.1.1).

The protocol itself is unchanged: "Invalid … cache owner", "Failed to release … cache owner", the fail-closed paths and flush tolerance behave exactly as before; only the storage location of a registration moved. #started, #finished and #epoch were already per collection.

Evidence

Probe against MariaDB 10.11 + Redis 8.2.1, one collection, setQueryCache() installed, a find() after each createDocument():

Before After (200 writes) After (500 writes)
document-cache owner keys 1 → 201 1 → 2 1 → 2
query-cache owner keys 1 → 201 1 → 1 1 → 1

Each leaked key was {"__utopia_gen__":"1"} with TTL -1. After the fix the extra document-cache key is _metadata#owners, not growth.

Why this approach

  • Every key a Redis adapter writes stays forever (no TTL), so the only way to bound the leak is to bound the set of key names; per-token names cannot be.
  • save(..., $ttl) with a key expiry only exists in utopia-php/cache 5.x, and a whole-key purge drops the expiry anyway; the constraint is ^4.0 || ^5.0.
  • A single per-collection "canary" value instead of per-owner registrations would drop the owner checks, which this PR keeps.

Costs

  • One HKEYS of the collection's #owners hash per registration and one per activation on the Redis adapters. The hash only holds in-flight registrations.
  • If a flush lands between a registration's save and its list, that one token falls back to a key of its own, which then stays behind on Redis. Correctness is unaffected: the flush happened before the barrier was written, so the registration is a valid canary either way.

Tests

Each test was seen failing against the code it guards:

Test Fails on
GeneralTests::testCacheInvalidationDoesNotAddRedisKeysPerWrite (e2e, real Redis): the collection's Redis key set is identical before and after 11 writes and a transaction unfixed feat-query-lib
QueryCacheTest / DocumentCacheEpochTest::…DoNotAddKeysToACacheThatKeepsPurgedKeys: same, against RedisLeasableCache, a double that mirrors the Lua purge semantics (a purged key stays, holding its generation) unfixed
QueryCacheTest::testOverlappingInvalidationsSucceedOnACacheWithoutFields, DocumentCacheEpochTest::testOverlappingWritesSucceedOnACacheWithoutFields: a second owner in flight on a field-less cache does not fail the first owner's activation this PR's first revision (1f943b88b)
…RejectsACorruptedOwnerRegistration / …PropagatesAnOwnerReleaseFailure: "Invalid … cache owner" and "Failed to release … cache owner" still fire when a field write is corrupted or a field purge fails unfixed, where no registration was a field, so the fault never reached one

The tests assert key counts and behaviour, not key names or Redis's reserved fields.

Verified locally (on top of #986)

  • composer lint (Pint)
  • composer check (PHPStan, both configs)
  • Unit suite: paratest --functional --processes 4 tests/unit
  • MariaDB e2e lane: paratest --functional --processes 4 tests/e2e/Adapter/MariaDBTest.php

Not verified locally

  • The other e2e lanes (MySQL, Postgres, SQLite, MongoDB, Memory, Pool, Mirror, SharedTables, Schemaless); CI runs them.
  • utopia-php/cache 5.x: the lock pins 4.0.2; the 5.x Lua scripts are identical, but no suite ran against 5.x.
  • Existing *#owner:* keys in deployed Redis instances are not cleaned up by this PR; they are no longer read and can be deleted with SCAN MATCH *#owner:* + DEL (the new #owners key does not match that pattern).

🤖 Generated with Claude Code

The document cache and the find() query cache registered every
invalidation under its own `#owner:<token>` key and released it with
purge(). On the Redis adapters purge() runs LUA_PURGE_BUMP, which DELs
the key and then HSETs its next generation, so the key never goes away;
the adapter sets no expiry either. Each write therefore left one
permanent key per cache: a probe of 200 createDocument() calls against
MariaDB and Redis grew the owner keys from 1 to 201 in each cache.

A collection's registrations are now fields of one `<collection key>#owners`
hash: save($ownersKey, $token, $token) registers, load() with the token
checks, and purge($ownersKey, $token) releases through LUA_PURGE_FIELD,
which HDELs the field and bumps the key's generation. The key count is
bounded by the number of collections, and the hash only holds the
registrations still in flight. The owner checks, fail-closed paths and
flush tolerance are unchanged; only where a registration lives moved.

Adapters that ignore $hash (Memory, Memcached, Filesystem, Hazelcast)
share one slot per collection, so two overlapping invalidations of one
collection there would report the earlier owner as invalid after its
commit and leave the epoch blocked. The Redis adapters, which are the
only Leasable ones, keep fields apart.

Keys matching `*#owner:*` from earlier builds are no longer read.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)
  • main
  • 0.69.x

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: utopia-php/database/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 98205fdf-dc3c-42f4-9a21-0f6068880f07

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[High risk] Refactors how cache invalidations register ownership across collections.

The PR appears safe to merge; no new actionable issue or outstanding previous finding was established.

Summary

The PR consolidates Redis cache-owner registrations into a per-collection key and retains per-token registrations for caches without fields. The follow-up revision adds that fallback and replaces storage-layout assertions with key-growth and fault-injection checks.

Reviews (2) · Last reviewed commit: "(fix): keep separate owner registrations..."

Comment thread src/Database/Traits/Documents.php Outdated
Comment thread tests/unit/Documents/DocumentCacheEpochTest.php Outdated
Comment thread tests/e2e/Adapter/Scopes/GeneralTests.php Outdated
The first revision put every registration in one `#owners` hash per
collection. Memory, Memcached, Filesystem and Hazelcast ignore the
field, so there overlapping invalidations of one collection shared a
single slot, and the earlier owner read the later owner's token and
threw "Invalid document cache owner" after its write had committed.

Owners::register() now checks, right after saving its field, whether
the adapter lists it. The Redis adapters do, so the token stays a field
of the bounded `#owners` hash. Field-less adapters always list nothing,
so the token also gets a key of its own, as before this branch; their
purge deletes it, so nothing leaks there. Owners::find() locates the
registration the same way at activation, so the checks, release and
fail-closed paths read the registration the token actually has.

The regression tests now assert that the key set does not grow with
writes, and inject the owner faults as cache faults, instead of naming
the owners key or Redis's reserved fields.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@abnegate

Copy link
Copy Markdown
Member Author

@greptileai review

@abnegate
abnegate merged commit 30f25fd into feat-query-lib Sep 28, 2026
22 checks passed
@abnegate
abnegate deleted the fix/cache-owner-key-leak branch September 28, 2026 08:28
abnegate added a commit to appwrite/appwrite that referenced this pull request Sep 28, 2026
The train's constraints go back to caret form. For usage ^0.17 is the
range 0.17.* was; for database, abuse and migration ^8.0 and ^3.0 also
admit the later minors of the same major, which their branch aliases
(8.0.x-dev, 3.0.x-dev) and their first tags satisfy without another
constraint change. The four libraries restated their own constraints
the same way, so the lock moves only their references to those heads:
database 30f25fd13b, abuse cf0ed5d129, migration edf6230085 and usage
bf9ab997fa.

The database re-pin brings utopia-php/database#982: on MySQL, from five
joins, each joined table's permission check stays a subquery
(NO_SEMIJOIN); other adapters are unchanged. It also brings
utopia-php/database#986: a write that commits while the cache is being
flushed no longer fails with "Failed to finish document cache
invalidation"; a purge that is really lost still does. And
utopia-php/database#987: the document cache and the find() query cache
register a write's invalidation owner as a field of one key per
collection, so writes no longer leave a Redis key behind each.

Audit moves to the mirror head f8422f9cbf, which carries
monorepo#206's 5.0.0 candidate (7c9c8296cc): its adapters read
attributes and indexes as typed models, and its legacy TYPE_*
constants come from Method. audit keeps its inline alias until audit
5.0.0 is released.

Co-Authored-By: Claude Opus 5.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.

1 participant