fix: clear extension caches by pattern where Redis can, as intended

[netomi]

First part of #1767 — see the analysis there.

The fast path never ran

if (cache instanceof RedisCacheWriter redisCache) {
    redisCache.clear(CACHE_EXTENSION_JSON, extensionJsonCacheKey.generateWildcard(extension).getBytes());
    return;
}

cache comes from cacheManager.getCache(...), and with ovsx.redis.enabled=true that is a RedisCache. From spring-data-redis 4.0.6, the version we build against:

public class RedisCache extends AbstractValueAdaptingCache     ← not a RedisCacheWriter
public RedisCacheWriter RedisCache.getNativeCache()            ← the writer is behind this
public void RedisCache.clear(java.lang.String)                 ← what this wanted

So every Redis deployment has been taking the fallback instead: an extension with 200 versions costs (3 aliases + 200 versions) × 13 target platforms = 2639 evictions of keys that mostly do not exist, each a round trip, on the request thread. That is what makes deleting a review slow.

Even if the branch had been reachable it would have matched nothing: the pattern was built here without the prefix RedisCacheManager stores keys under. RedisCache.clear(String) is the right API — its bytecode runs the pattern through createAndConvertCacheKey, which applies that prefix, before calling clean.

A key format the prefix can match exactly

Review found that foo.bar* also swept foo.bar2, and scoping it to foo.bar-* only narrowed that: a name may contain a - and so may a version, so nothing separated the keys of bar from those of bar-baz.

The keys were borrowing NamingUtil.toFileFormat, which exists for naming files — the .vsix names, the rename-downloads migration — and is not free to change. The caches get their own format, ending the extension id with a character a name cannot contain:

foo.bar:1.0.0@linux-x64        prefix "foo.bar:"
foo.bar-baz:1.0.0              prefix "foo.bar-baz:"

: is outside [\w\-\+\$~]+, the names ExtensionValidator accepts, so a prefix now matches one extension and no other.

Names are encoded, not trusted. That character set is what the validator enforces now; rows older than it, or mirrored from elsewhere, were never held to it. A prefix also becomes a Redis glob, where * ? [ ] \ are syntax — a stored name like bar* would widen the pattern back over its siblings. Every byte outside [a-z0-9_+$~-] is percent-encoded, so the pattern stays literal whatever the stored name is, and a valid name is byte-identical.

Clearing without blocking Redis

Pattern clearing is no longer occasional — it is what every eviction does. A cache writer built from the connection factory alone uses BatchStrategies.keys(), which runs KEYS over the whole keyspace in one command and blocks the server until it finishes. The manager is built on a writer with BatchStrategies.scan(256) instead, which walks the keyspace with a cursor and deletes in batches.

Deploying it

Nothing persisted changes; this is a cache key format. Entries in the old format become unreachable and expire on their own.

One thing to know for a rolling deployment with a shared Redis: while both formats are in flight, a change committed by a pod on one format does not invalidate what a pod on the other has cached, so either can briefly serve stale JSON. Clearing the caches once after the rollout — the Caches page in the admin dashboard — closes that window. This is deliberately not handled in code: a transitional dual-format eviction would be a second clear per eviction that exists only to be removed a release later.

Testing

CacheServiceEvictionTest pins which way an eviction goes and what the pattern may sweep:

• one clear("foo.bar:*") and no key-by-key eviction where the cache is a RedisCache, the ≥2600 guesses where it is not;
• every key shape the fallback would evict is matched — universal, target-platform, alias, semver pre-release — while foo.bar2, foo.bar-baz and another namespace are not;
• a name carrying the terminator, and names carrying glob syntax (*, ?, [, \, ., space), are encoded rather than widening the pattern;
• a valid name is left exactly as it is.

RedisCacheManagerScanTest drives a cache built by CacheConfig itself over a mocked connection: scan is used and keys never is, and the captured ScanOptions carry extension.json::foo.bar:* — which also shows the cache-name prefix being applied by RedisCache.clear(String), as described above. Both fail against the previous one-line configuration.

The glob is matched in-process, so these are about the pattern we generate rather than what a particular Redis does with it.

Confirmed not vacuous — against the current main, three of the original four fail.

Full server suite green (1434 tests); pre-commit hooks pass.

Note for the reviewer

This touches CacheService, which #2215 also touches (it adds CDN purge calls to the same methods). The two are independent; whichever lands second needs a small textual merge.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Redis patterns could evict caches belonging to extension IDs with shared prefixes.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR improves extension cache eviction by using Redis pattern clearing while retaining key-by-key fallback behavior.

Changes:

• Adds Redis-aware pattern eviction.
• Removes the obsolete null-version guard.
• Adds tests for Redis and fallback paths.

File summaries

File	Summary
server/src/test/java/org/eclipse/openvsx/cache/CacheServiceEvictionTest.java	Tests Redis pattern clearing and fallback eviction behavior.
server/src/main/java/org/eclipse/openvsx/cache/CacheService.java	Implements pattern-based eviction; patterns may be too broad and require delimiter-aware matching plus regression coverage.

Review details

Suppressed comments (1)

server/src/main/java/org/eclipse/openvsx/cache/CacheService.java:180

• The same unbounded foo.bar* pattern is used for the latest-version Redis cache, so an extension such as foo.bar2 can have its latest-version entries deleted when foo.bar is evicted. Include the - delimiter in this wildcard as well (for example foo.bar-*) and cover the prefix-collision case.

        if (clearByPattern(cache, latestExtensionVersionCacheKey.generateWildcard(extension))) {

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Critical issues remain with legacy-key compatibility and Redis glob metacharacter escaping.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unicode key collisions and blocking Redis keyspace scans remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

server/src/main/java/org/eclipse/openvsx/cache/ExtensionJsonCacheKeyGenerator.java:90

• Lowercasing the whole string before encoding breaks the promised isolation for legacy/mirrored names. Unicode case mappings can collapse an invalid name into valid ASCII—for example, Java maps K to k—so foo.K and foo.k receive the same cache prefix. That can serve one extension's cached value for the other and makes either wildcard clear remove both. Lowercase only ASCII A-Z, and percent-encode every other unsafe byte from the original string.

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Balanced