Skip to content

Fix ack#343

Merged
ChrisBr merged 1 commit intomainfrom
cbruckmayer/fix-ack-logic
Jun 30, 2025
Merged

Fix ack#343
ChrisBr merged 1 commit intomainfrom
cbruckmayer/fix-ack-logic

Conversation

@ChrisBr
Copy link
Contributor

@ChrisBr ChrisBr commented Jun 30, 2025

sadd returns the number of added entries so it always returns a truthy value. We have to == 1.

This can result in race conditions for lost tests when another worker marks a lost test as processed but then fails on the original worker.

image

@ChrisBr ChrisBr force-pushed the cbruckmayer/fix-ack-logic branch 4 times, most recently from e43f2a6 to 98b87d0 Compare June 30, 2025 10:18
@ChrisBr ChrisBr requested a review from casperisfine June 30, 2025 10:21
@ChrisBr ChrisBr force-pushed the cbruckmayer/fix-ack-logic branch 2 times, most recently from 0017b91 to 1e68476 Compare June 30, 2025 10:38
redis.call('zrem', zset_key, test)
redis.call('hdel', owners_key, test) -- Doesn't matter if it was reclaimed by another workers
local acknowledged = redis.call('sadd', processed_key, test)
local acknowledged = redis.call('sadd', processed_key, test) == 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the mistake was to think Lua consider 0 as falsey.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right 🤦

@ChrisBr ChrisBr force-pushed the cbruckmayer/fix-ack-logic branch 6 times, most recently from 2234eef to d2e404a Compare June 30, 2025 11:24
@ChrisBr ChrisBr force-pushed the cbruckmayer/fix-ack-logic branch from d2e404a to f9a9894 Compare June 30, 2025 11:38
@ChrisBr ChrisBr merged commit af7e42e into main Jun 30, 2025
13 of 23 checks passed
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.

3 participants