Skip to content

fix(common/redis): Pipeliner.Do queues every command twice when writeTimeout is set - #4498

Open
paulstadler-mesh wants to merge 1 commit into
dapr:mainfrom
paulstadler-mesh:fix/pipeliner-double-queue
Open

fix(common/redis): Pipeliner.Do queues every command twice when writeTimeout is set#4498
paulstadler-mesh wants to merge 1 commit into
dapr:mainfrom
paulstadler-mesh:fix/pipeliner-double-queue

Conversation

@paulstadler-mesh

Copy link
Copy Markdown

Description

Fixes #4464.

Pipeliner.Do (both v8client.go and v9client.go) is missing a return in its writeTimeout > 0 branch: after queueing the command with a timeout context, execution falls through and queues the same command again with the unbounded context. Every command sent through a TxPipeline then executes twice per Exec whenever the component sets writeTimeout metadata.

Impact

  • State transactions execute every operation twice. StateStore.Multi pipelines the CAS EVAL per operation, so a single transactional write runs the script twice — the version hash field increments by 2 per write, breaking ETag/first-write semantics.
  • Dapr Workflow / actor state stores fail hard on daprd ≥ 1.17.9 (whose workflow engine plants the Lua first-write flag): every second workflow write fails with ERR user_script:14: failed to set key …||workflow||…||metadata. On earlier daprd the duplication is silent — versions inflate and transactional Redis CPU is ~doubled.
  • Encountered in production against Azure Managed Redis: all Dapr Workflow submissions failed once daprd moved to 1.17.9; Redis CPU dropped materially once the duplication was removed. Reproduces identically through built-in state.redis and pluggable components (shared client).

Minimal repro and full analysis in #4464 (e.g. one transactional upsert on a fresh key → GET returns ETag 2).

Fix

Add the missing return (mirrors the existing pattern in DoWrite/DoRead on the same clients).

Testing

New regression test TestPipelinerDoQueuesOnceWithWriteTimeout drives a pipelined INCR through both v8 and v9 clients against miniredis and asserts single execution, with and without writeTimeout (fails on the previous code with counter=2).

Checklist

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation — n/a (no user-facing behavior beyond the bug fix)

…Timeout is set

The writeTimeout branch was missing a return: after queueing the command
with a timeout context it fell through and queued the same command AGAIN
with the unbounded context. Every pipelined command then executed twice
per Exec.

Most visible casualty: the state store's transactional CAS EVAL scripts
(Multi/TxPipeline) — each transactional write incremented the version hash
field by 2, breaking ETag/first-write semantics. With daprd >= 1.17.9
(whose workflow engine plants the first-write flag), Dapr Workflow actor
state saves fail on every second write with:
  ERR user_script:14: failed to set key <app>||...workflow||<id>||metadata
On earlier daprd the duplication is silent (versions inflate, the Lua
escape hatch masks the CAS) and transactional Redis CPU is ~doubled.

Fixes dapr#4464. Regression test drives a pipelined INCR through both clients
and asserts single execution with and without writeTimeout.

Signed-off-by: Paul Stadler <paul.stadler@meshconnect.com>
@paulstadler-mesh

Copy link
Copy Markdown
Author

@mikeee sorry for the direct ping — this one's a 2-line fix (+regression test) for #4464: Pipeliner.Do double-queues every pipelined command when writeTimeout is set, which breaks transactional ETag/first-write semantics (and fails Dapr Workflow state saves on daprd ≥ 1.17.9). We've been running the fix in production for a couple of weeks.

The CI workflows are sitting at awaiting approval (fork PR) — if you could approve the runs, everything else should go green from there. Happy to make any changes needed. Thanks!

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.

Redis: Pipeliner.Do executes every pipelined command twice when writeTimeout is set (breaks transactional ETag/first-write CAS, doubles Lua load)

1 participant