fix(master): bump the cooldown failure count with a compare-and-swap - #270
Merged
Merged
Conversation
Several masters finish a failing task for the same target within seconds of one another because they all swept it at the same instant. The count was bumped with a plain get/put, so two of them could read the same value and both write count+1, losing a failure and delaying the cooldown. On staging three masters failed the same store within 3.4 s and the record showed fewer failures than tasks. Read the key's mod_revision, write the new record in a Txn conditioned on it, and retry with a short jittered backoff on conflict. A 12-way concurrent test now records exactly 12. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
record_failure(#267) bumped the per-target failure count with a plain get → +1 → put. Several masters finish a failing task for the same target within seconds of one another (they all swept it at the same instant), so two could read the same count and both writecount+1— a lost update. Staging showed three masters failing the same store at 10:34:49/:51/:52 with the record under-counting; a broken store then takes an extra window or two to reach the threshold.Fix
get_cooldown_versionedreturns the key'smod_revision;record_failurewrites the new record in aTxnconditioned onCompare::mod_revision == observed, and on conflict revokes its lease, backs off 5–25 ms with jitter, and retries (up to 64 attempts — contention is bounded by the master count, a handful). The success path is unchanged.Verification
New etcd-backed test
concurrent_failure_records_are_not_lost: 12 concurrentrecord_failurecalls on one target, assertsfailures == 12. Under the pre-fix get/put it lost updates; with CAS but a tight 8-retry loop it exhausted retries; with jittered backoff all 12 land. Master suite 28/28 etcd-backed + 42 default, clippy clean.Refs #266, #267, #269.
🤖 Generated with Claude Code