Skip to content

fix(redis): atomic surrogate sets across instances, and Get on hash mappings - #65

Open
silverbackdan wants to merge 2 commits into
darkweak:mainfrom
silverbackdan:fix/redis-atomic-surrogate-sets
Open

silverbackdan wants to merge 2 commits into
darkweak:mainfrom
silverbackdan:fix/redis-atomic-surrogate-sets

Conversation

@silverbackdan

Copy link
Copy Markdown

Fixes darkweak/souin#870.

Problem

With a Redis store shared by several Souin instances, some cached responses end up unpurgeable by tag. There are two causes, depending on the module and version.

  • redis (rueidis), and go-redis v0.0.19: neither implements SetStorer, so Souin's storeTag updates the comma-joined SURROGATE_<tag> string with a GET then a SET. It serialises that with a mutex that covers one process only, so instances sharing Redis overwrite each other's additions.
  • go-redis v0.0.20: since feat(go-redis): store mappings as one hash field per variant #63, mappings are hashes, but Souin's SouinAPI.BulkDelete still reads them with Get(IDX_<key>). GET on a hash fails with WRONGTYPE, Get returns nothing and starts Reconnect(), and while it's reconnecting the Delete calls that follow are skipped. The variants and the mapping stay cached.
  • go-redis AddToSet (smaller window): it reads a legacy string value, then DELs and SADDs in a separate transaction. Two writers migrating the same tag overwrite each other's members, even within one process.

Fix

  • go-redis Get: on WRONGTYPE it reads the hash and returns it as an encoded core.StorageMapper. That's the format callers expect from a mapping key, and it's what BulkDelete decodes. A key that isn't a hash (a surrogate set, say) still returns nothing. Error replies no longer trigger Reconnect(), because the connection is fine.
  • go-redis AddToSet: the legacy migration, SADD and lifetime extension now run as one Lua script. Behaviour is unchanged: legacy members are kept, a positive duration extends the lifetime but never shortens it, and a non-positive one leaves it alone. The lifetime is passed in milliseconds (PTTL/PEXPIRE).
  • redis (rueidis): implements SetStorer (AddToSet with the same script, GetSet reading legacy strings or sets, WalkSets). Souin v1.7.9 already uses it when the storer supports it (perf(surrogate): store tags via native sets when the storer supports it souin#851).

No interface changes. The script touches a single key, so it works on Redis Cluster.

Release note

The */caddy modules tagged v0.0.20 still require their base modules at v0.0.19, so a standard xcaddy build --with github.com/darkweak/storages/go-redis/caddy ships go-redis v0.0.19. For this fix to reach xcaddy builds, the next release needs the make bump-version commit, as v0.0.19 had (#54).

Tests

Against the redis:8.4-alpine from compose.test.yml:

  • go-redis: TestRedis_Sets_ConcurrentInstances has two storer instances add 40 members concurrently to a set that starts in the legacy string format, over 20 rounds. On main it fails on the first round with 21 of 42 members. TestRedis_Get_HashMapping runs BulkDelete's sequence against a hash mapping. On main, Get returns no entries.
  • redis: TestRedis_Sets, TestRedis_Sets_LegacyStringMigration, TestRedis_WalkSets (ported from go-redis) and TestRedis_Sets_ConcurrentInstances. On main they fail because the storer doesn't implement SetStorer.
  • go test -race passes for core, go-redis, redis and otter, with -count=3 for the two Redis modules.
  • golangci-lint run --new-from-rev=main only reports exhaustruct on &core.KeyIndex{} and on the test clients' options literals. Those follow the existing code, which uses the same idioms.

End to end: two FrankenPHP instances with Souin v1.7.9 sharing one Redis, concurrent first requests for pages sharing one tag, then a purge of that tag through one instance. Pages still cached afterwards:

60 pages ×3 10 pages ×3 200 pages
go-redis v0.0.19 3, 7, 5 0, 0, 2 –
go-redis v0.0.20 23, 47, 15 3, 3, 6 –
rueidis v0.0.19 8, 2, 10 0, 0, 0 –
go-redis, this branch 0, 0, 0 0, 0, 0 0
rueidis, this branch 0, 0, 0 0, 0, 0 0

On this branch every page is indexed in every run. Purges sent through either instance, or through a freshly restarted one, remove what the other stored, and purging a tag leaves tags that contain it indexed (darkweak/souin#867).

Not covered: Redis Cluster, and upgrading a live fleet where old and new binaries run side by side.

🤖 Generated with Claude Code

…gh Get

AddToSet read a legacy string value, then deleted and rewrote the key
in a separate transaction. Two writers migrating the same tag, from one
process or from several instances sharing Redis, overwrote each other's
members. The migration, addition and lifetime update now run as one
script.

Get on a mapping key stored as a hash (since darkweak#63) failed with WRONGTYPE,
returned nothing and started a reconnection. Souin's purge reads
mappings through Get, so it found no variants, and the deletions it sent
next were skipped while reconnecting. Get now returns the hash as an
encoded StorageMapper, and error replies no longer trigger a reconnection.
Without SetStorer, Souin stores surrogate tags in rueidis as one
comma-joined string, updated by GET then SET under an in-process mutex.
Instances sharing Redis overwrite each other's additions, and the lost
cache keys can no longer be purged by tag. Use native sets, with the
same atomic migration script as go-redis.
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.

Shared Redis store: concurrent instances lose surrogate-key index entries, and go-redis v0.0.20 purges skip deletions

1 participant