Repository navigation
Count each chord header task only once in the Redis backend - #10919
Conversation
With `acks_late`, a chord header task can run twice: its worker can be killed before the ack, or the broker can deliver the message again. The Redis backend counts each run. In unordered mode a second run always counts twice, and in ordered mode it does when it returns a different value. The body then runs too soon, with two results for one header and none for another, and nothing is logged. Save each part with a Lua script. It saves the result only if this task id and header index are not yet in a `<group>.d` set, then adds them, and reads the count in the same step. The body is still sent whenever the count is complete, so a header that runs again after the last part was saved still sends it. When the chord fails, its `.s` key is now deleted, so a later copy of a header can't fail it again. Older workers still write the same `.j` key, so a chord with parts from old and new workers still ends. The Redis user now needs the right to run scripts. The docs say so. Fixes celery#10910
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10919 +/- ##
=======================================
Coverage 90.72% 90.72%
=======================================
Files 153 153
Lines 21531 21533 +2
Branches 2653 2651 -2
=======================================
+ Hits 19533 19535 +2
Misses 1685 1685
Partials 313 313
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
MehrazRumman
left a comment
MehrazRumman
left a comment
There was a problem hiding this comment.
Checked at 9fbb85e. t/unit/backends/test_redis.py: 180 passed, 1 skipped. 17 of them fail on main: the new ones, plus the existing ones changed to the script.
The unit tests replace the script with a Python copy (register_script in the fake Redis), so the Lua itself only runs in the integration test. To check the real script, I ran RedisBackend.on_chord_part_return against fakeredis with Lua support (fakeredis[lua], which runs it with lupa). The scenario: a 3-part chord, header 0 runs twice with another result, then the other parts, then a copy of the last part:
| body sent before the last part | body calls | |
|---|---|---|
| main, ordered / unordered | [10, 99, 20] |
[[10, 99, 20]] |
| this PR, ordered / unordered | no | [[10, 20, 30]] |
this PR, group_index=None (scored as +inf), both modes |
no | [[10, 20, 30]] |
- On main the body runs early, with two results for header 0 and none for header 2, which is #10910. Here it runs once, with one result per header.
- The copy of the last part after the chord completed doesn't send the body again.
- A copy of a header part after the chord failed doesn't fail it a second time.
- With
result_chord_expires=600, the new.dkey gets the same 600s TTL as.jand.s.
The design choices make sense:
- Keying the check on
task_id:group_indexhandles a header that reuses a frozen signature. - Sending the body each time the count is complete keeps the "worker lost after saving the last part" case working.
- Parts from older workers still count during a rolling upgrade.
redis-py's Script loads the script again on NOSCRIPT (e.g. after a Redis restart or a Sentinel failover), so the cached register_script is fine.
Smaller points
- The release notes should mention the ACL change, since it can stop chords on a deployment that restricts Redis commands. The note in
redis.rstcovers the setup, but people upgrading are more likely to read the changelog. It's also worth a line that Redis-compatible services or proxies have to allowEVALSHA/SCRIPT LOAD. - Optional: since the unit tests only run the Python copy of the script, a test that runs the Lua through
fakeredis[lua]would catch a mismatch between the two without a Redis server. That would add a test dependency, so I'm fine with leaving it to the integration test if maintainers prefer.
Looks good to me.
Chords now save header results with a Lua script. If a Redis user's ACL lists the commands it can run, it needs four more. The Redis docs name them, but people read the changelog when they upgrade. The entry and the docs also say that a proxy or a service that works like Redis has to support the script commands.
The other unit tests use a Python copy of the chord part script, and they pass even when the Lua is wrong. The new tests run the Lua itself with fakeredis. In the main test, a header task runs twice, with and without a group index. Two more tests check a task id at two header indexes and the key TTLs. fakeredis runs Lua with lupa, which has no wheel for CPython 3.15 yet. The tests skip when either package is missing.
|
Thanks for running the Lua with I also added the
|
The note about the Lua script and the Redis commands it needs is now a versionchanged 5.7 note. It also says what changed: a chord counts each header task only once, even when the task runs again.
Note: Before submitting this pull request, please review our contributing
guidelines.
Description
Fixes #10910.
With
acks_late, a chord header task can run twice. The Redis backend adds each run to the chord's count. In the default ordered mode, a second run with the same result merges with the first. But a run with a new result counts again, and so does any second run in unordered mode. The body can then run before the other header tasks finish, with two results for one header and none for another.This saves each part with a small Lua script. In one step, it records the result only if this task id and header index aren't recorded yet, and then reads the count.
What changes:
.skey is now deleted, so a copy that arrives later can't fail it again. The saved results stay, as before..jkey, so chords with parts from old and new workers still finish. Parts from older workers aren't checked for copies.EVALSHA,SCRIPT LOAD, and theSISMEMBERandSADDcalls inside the script. If the user lacks them, header tasks get a Redis permission error as they store their results, so neither the body nor the error callbacks will run. I added a note about this to the Redis docs.Tests cover a repeated header (new value,
None, failure), a part from an older worker, a reused frozen signature, a rerun of the last part, and a copy after the chord failed. An integration test runs the script against Redis in both modes. The crash script in #10910 gives the body one result per header on this branch.