fix(rueidisaside): prevent canceled cache fills from leaking locks

[Makarechi]

Problem

CacheAsideClient.Get can return context.Canceled while a side-effecting lock command is still queued in rueidis. If cleanup uses another multiplexed connection, it can reach Redis first and do nothing; the delayed acquire can then run later and leave a live rueidisid:* placeholder behind.

Blind cleanup is also unsafe when concurrent fills from the same CacheAsideClient share the same client marker ID.

Fix

This change follows the same connection-ordering approach used by rueidislock:

1. PipelineMultiplex is forced to -1 before constructing the rueidis client, so lock mutations for a Redis node use one FIFO pipeline.
2. The complete client marker -> acquire -> loader -> set/delete flow is singleflighted strictly per cache key, including across client-ID changes.
3. Acquire, loader, and final cache write keep the caller's context and deadline.
4. Any ambiguous acquire/fill error runs compare-and-delete cleanup with an independent context.
5. Cleanup completes before the local flight is released, so a same-client follower cannot interleave another fill.
6. No detached fill goroutine, attempt guard, extra Redis key, or new Redis value format is introduced.

flowchart TD
    A["Cache miss"] --> B{"Active local flight for key?"}
    B -- "Yes" --> C["Wait with caller context"]
    C --> A
    B -- "No" --> K["Ensure current client marker"]
    K -- "Error" --> I["Release flight"]
    K -- "Ready" --> D["Acquire lock with caller context"]
    D -- "Key occupied" --> F["Release flight and follow existing value/placeholder"]
    D -- "Canceled or error" --> E["Compare-and-delete cleanup"]
    D -- "Acquired" --> G["Run loader"]
    G --> H["Compare-and-set ready value with caller context"]
    H -- "Success" --> I
    H -- "Canceled or error" --> E
    E --> I

Loading

The FIFO pipeline closes the late-command race:

• if a canceled acquire or final write was already queued, cleanup is queued behind it on the same connection;
• if the canceled command was never queued, compare-and-delete cleanup is harmless;
• per-key singleflight prevents cleanup from deleting another in-process fill that uses the same client marker.

Compatibility

• The placeholder remains the exact existing rueidisid:* heartbeat key, so the Redis data format is unchanged.
• Redis 7 continues to use SET NX GET.
• UseLuaLock=true retains the Redis < 7 compatible acquire script.
• The supplied PipelineMultiplex value is intentionally overridden with -1, matching rueidislock.

Tests

Regression coverage includes:

• canceled acquire after Redis accepted the lock, for Redis 7 and legacy Lua paths;
• local per-key coordination starting before client-marker setup;
• one active acquire per key with independent follower deadlines;
• client-ID changes while a per-key flight is active;
• concurrent fills for different keys;
• cleanup and flight release after loader panic;
• canceled final cache writes;
• ClientBuilder receiving the enforced pipeline option.

Local validation:

• go test . -count=1
• go test -race . -count=1
• go vet ./...
• repeated focused regression tests (-count=50)
• git diff --check

[rueian]

I don't think ignoring the user's context is a good idea, as we should respect the deadline in the user's ctx as much as possible. Can't we just do delkey.Exec(context.Background(), c.client, []string{key}, []string{id}) regardlessly if the returned error from acquireLock or c.client.Do is a context error?

[Makarechi]

Respecting the caller's deadline is a fair concern. The reason I did not run delkey on every context error is that id belongs to the whole CacheAsideClient, not to one acquisition.

For example, two concurrent Get calls on the same missing key can both reach lock acquisition. If A acquires the lock and starts the loader while B returns a context error, delkey(key, id) from B also matches A's lock and removes it. The same race is possible when B is canceled before its command is enqueued and A acquires the lock between B's cache miss and B's cleanup. A's later setkey CAS can then no longer populate the cache, and another client may start a duplicate load.

WithoutCancel is used only for the acquisition round trip so the result tells us whether this specific attempt acquired the lock (RedisNil) before cleanup. I agree that returning by the caller's deadline would be preferable. A safe alternative would be to run the acquire as an in-flight operation, return when ctx.Done() fires, and let the in-flight goroutine wait for the actual Redis result and clean up only if that attempt acquired the lock. Would you prefer that direction?

[rueian]

For example, two concurrent Get calls on the same missing key can both reach lock acquisition. If A acquires the lock and starts the loader while B returns a context error, delkey(key, id) from B also matches A's lock and removes it. The same race is possible when B is canceled before its command is enqueued and A acquires the lock between B's cache miss and B's cleanup. A's later setkey CAS can then no longer populate the cache, and another client may start a duplicate load.

Oh, that makes sense; then can we do singleflight for the acquire-fn-setkey-delkey flow instead?

[rueian]

Or we can do make the id cantains a value from an atomic counter and make sure the delkey only delete the key matching the value.

[Makarechi]

Thanks, both suggestions address the shared client ID race.

A singleflight around the whole acquire -> fn -> setkey/delkey flow, scoped per cache key and client ID generation, seems like the smaller backward-compatible option. Using a per-attempt counter is trickier because the placeholder value currently also names the client heartbeat key: a suffixed value would have no corresponding heartbeat key, and old clients during a rolling upgrade could treat a live owner as dead.

There is one remaining edge case that I do not think singleflight alone covers. With pipeline multiplexing, the canceled acquire and its cleanup can run on different connections. If Do returns a context error while the SET is still queued, delkey may reach Redis first; the SET can then execute later and leave the placeholder behind.

Would you be open to combining the singleflight with a detached acquire worker? Get would return as soon as the caller context is done, while the worker would wait for the actual acquire result. If that attempt acquired the lock, it would run compare-and-delete, and only then release the singleflight. Waiters would keep their own contexts and retry the cache after the flight completes.

This keeps the current Redis format and respects caller deadlines while covering both the shared-ID deletion race and the late-SET cleanup race. If this direction sounds reasonable, I can update the PR and add regression coverage for the canceled leader and live follower cases.

[rueian]

There is one remaining edge case that I do not think singleflight alone covers. With pipeline multiplexing, the canceled acquire and its cleanup can run on different connections. If Do returns a context error while the SET is still queued, delkey may reach Redis first; the SET can then execute later and leave the placeholder behind.

Wouldn't SET later fail because the placeholder has been deleted? What's wrong with this?

Using a per-attempt counter is trickier because the placeholder value currently also names the client heartbeat key: a suffixed value would have no corresponding heartbeat key

Yes, it is trickier, but I think we can overcome that by doing some tricks in Lua.

Would you be open to combining the singleflight with a detached acquire worker?

I'd like to avoid additional goroutines here because they would be costly. I currently prefer making the whole per-key acquire -> fn -> setkey/delkey singleflight since concurrently acquiring the same key from one client is just not optimal. But let me know if it alone can't solve the problem.

[Makarechi]

Implemented in 51940d3.

On the late SET question: after cleanup deletes the placeholder, SET ... NX sees an absent key, so it can succeed when the queued command eventually reaches Redis. That cleanup-before-acquire ordering is the race this revision now covers.

I followed your preferred no-extra-goroutine direction:

• the complete acquire -> fn -> set/delete flow is singleflighted per cache key and client-ID generation;
• every leader registers a unique attempt guard before sending acquire;
• acquire Lua checks that exact guard and uses its remaining TTL for the placeholder;
• the final write checks both the guard and placeholder ownership;
• cleanup revokes the guard and compare-and-deletes the placeholder before releasing the flight.

Therefore, if cleanup reaches Redis first, the delayed acquire/final write sees a revoked guard and does nothing. If acquire reaches Redis first, cleanup removes its placeholder. Cleanup is bounded; an unconfirmed cleanup rotates the local client-ID generation so an old command cannot match the next flight.

The main placeholder remains the exact existing rueidisid:* heartbeat key for rolling-upgrade compatibility. Guard keys are placed in the same Redis Cluster slot as the cache key. Both the Redis 7 and legacy UseLuaLock paths have regression coverage, including delayed acquire, delayed final write, live follower, disconnect generation change, and cleanup failure.

[rueian]

On the late SET question: after cleanup deletes the placeholder, SET ... NX sees an absent key, so it can succeed when the queued command eventually reaches Redis. That cleanup-before-acquire ordering is the race this revision now covers.

I see. I thought you were talking about the setKey previously. Yes, if we do delkey regardlessly, it is possible to reach Redis before the acquireLock or the SET key id NX.

Therefore, if cleanup reaches Redis first, the delayed acquire/final write sees a revoked guard and does nothing. If acquire reaches Redis first, cleanup removes its placeholder. Cleanup is bounded; an unconfirmed cleanup rotates the local client-ID generation so an old command cannot match the next flight.

But I think your latest approach is overkill. We have a couple of ways to make sure delkey and acquireLock or the SET key id NX land into the same connection:

1. Enforce PipelineMultiplex to -1. This is what we do in the rueidislock.
2. Use client.Dedicate/Dedicated for the acquire -> fn -> setkey/delkey flow.
3. Add a new command tag to let the slotfn in the mux.go choose a pipe based on the key hash.

I would prefer 1 for now and explore option 3 later. 2 is less preferable because it can create more connections and has unnecessary overhead.

[Makarechi]

Implemented your preferred option in 1c592a6.

• NewClient now enforces PipelineMultiplex = -1, keeping the acquire and cleanup commands ordered on the same connection.
• The complete acquire -> fn -> setkey/delkey flow remains singleflighted per cache key, so one client does not acquire the same key concurrently.
• I removed the attempt guards, counter, and additional Lua complexity from the previous revision. The Redis 7 path is back to SET NX GET, while the legacy path keeps the existing Lua acquire.
• The caller context is respected by acquire, the loader, and the final write. If cancellation requires cleanup, compare-and-delete runs with an independent context and finishes before the flight is released.

Regression tests cover cancellation cleanup for both Redis 7 and legacy paths, and go test, go test -race, and go vet pass. This should now match option 1; please take another look when convenient.

[rueian]

Even if the client id changed here, why can't we just wait for the flight to finish?

[rueian]

If we jump to retry, we will have a busy retry loop until the existing flight acquires the lock, which is not that good.

[Makarechi]

Good point — we can wait for the existing per-key flight here. Fixed in 81fe858.

The flight map is now keyed strictly by cache key, and beginFlight looks up an active flight before validating the client ID. Active flights are no longer reset on invalidation, so a caller that observed an old or new client ID joins the same in-process flow and waits with its own context instead of repeatedly jumping to retry.

The client-ID check remains only when no flight exists, which prevents a stale caller from becoming a new leader. In that narrow case one retry is still needed to obtain the current ID.

I also updated the Redis 7 and legacy disconnect tests to verify that an ID change does not start a second loader while the old flight is active, and added a unit regression for joining a flight across generations. go test, go test -race, and go vet pass.

[Makarechi]

Follow-up in 1854a98: I removed the remaining stale-ID/no-flight retry as well. The per-key flight is now created before keepalive, so the whole local miss flow starts with strict key-based singleflight. The leader resolves the current client ID inside that flight, while every same-key follower waits with its own context. This removes the retry branch entirely and still keeps different keys independent.

A new regression test blocks client-marker creation and verifies that a follower neither creates another marker nor runs another loader. Full tests, race tests, and go vet pass.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit 81fe858. Configure here.}