Repository navigation
fix(redis): atomic surrogate sets across instances, and Get on hash mappings - #65
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.
Applies darkweak/storages#65's go-redis half to v0.0.20, so two Souin instances sharing Redis keep every page under a shared tag and purge it. bin/test/souin-redis-shared-purge.sh fails on the unpatched module.
|
Hi @darkweak, a production data point to support merging this one. We have been running this branch (
Moving from the comma-joined surrogate strings to native sets also cut our Redis network traffic by more than 98%. We would love to switch back to an official tag. Could this be merged and released? As noted in the PR description, the release would need the Thanks to both of you for the work on sets and exact purges (souin#851, souin#868, #63). |
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