fix(redis): atomic surrogate sets across instances, and Get on hash mappings - #65
Open
silverbackdan wants to merge 2 commits into
Open
silverbackdan wants to merge 2 commits into
silverbackdan wants to merge 2 commits into
Conversation
…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.
This was referenced Sep 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
SetStorer, so Souin'sstoreTagupdates the comma-joinedSURROGATE_<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.SouinAPI.BulkDeletestill reads them withGet(IDX_<key>).GETon a hash fails withWRONGTYPE,Getreturns nothing and startsReconnect(), and while it's reconnecting theDeletecalls that follow are skipped. The variants and the mapping stay cached.AddToSet(smaller window): it reads a legacy string value, thenDELs andSADDs in a separate transaction. Two writers migrating the same tag overwrite each other's members, even within one process.Fix
Get: onWRONGTYPEit reads the hash and returns it as an encodedcore.StorageMapper. That's the format callers expect from a mapping key, and it's whatBulkDeletedecodes. A key that isn't a hash (a surrogate set, say) still returns nothing. Error replies no longer triggerReconnect(), because the connection is fine.AddToSet: the legacy migration,SADDand 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).SetStorer(AddToSetwith the same script,GetSetreading 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
*/caddymodules tagged v0.0.20 still require their base modules at v0.0.19, so a standardxcaddy build --with github.com/darkweak/storages/go-redis/caddyships go-redis v0.0.19. For this fix to reach xcaddy builds, the next release needs themake bump-versioncommit, as v0.0.19 had (#54).Tests
Against the
redis:8.4-alpinefromcompose.test.yml:go-redis:TestRedis_Sets_ConcurrentInstanceshas two storer instances add 40 members concurrently to a set that starts in the legacy string format, over 20 rounds. Onmainit fails on the first round with 21 of 42 members.TestRedis_Get_HashMappingrunsBulkDelete's sequence against a hash mapping. Onmain,Getreturns no entries.redis:TestRedis_Sets,TestRedis_Sets_LegacyStringMigration,TestRedis_WalkSets(ported from go-redis) andTestRedis_Sets_ConcurrentInstances. Onmainthey fail because the storer doesn't implementSetStorer.go test -racepasses forcore,go-redis,redisandotter, with-count=3for the two Redis modules.golangci-lint run --new-from-rev=mainonly reportsexhaustructon&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:
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