Repository navigation
Stop cleanup() expiring the hash of a worker that registers mid-run - #121
Open
ronzyfonzy wants to merge 1 commit into
Open
ronzyfonzy wants to merge 1 commit into
ronzyfonzy wants to merge 1 commit into
Conversation
cleanup() took the allWorkers() snapshot first and listed the worker:<host>:* hashes afterwards. A worker that registered between the two reads was absent from the snapshot but present in the key list, so cleanup() put an expiry on the hash of a live worker. Once it expired the worker kept running with no registration hash, and anything reading the fleet counted it as broken. register() adds the id to the set before it writes the hash, so listing the keys first guarantees every hash seen already has its id in the snapshot. Genuine orphan hashes are still expired. Seen on a fleet where a supervisor starts many workers at once: 15 started together left 14 hashes carrying an expiry, started a second apart left none.
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
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.
The problem
Worker::cleanup()expires the hash of a worker that registers while it is running.It does two reads. It snapshots the workers, and it lists the
worker:<host>:*hashes. Any hash whose id is not in the snapshot is treated as an orphan and gets an expiry. Today the snapshot is taken first and the keys are listed afterwards, so a worker that registers between the two is absent from the snapshot but present in the key list — andcleanup()expires the hash of a worker that is alive and working.When that expiry lands the worker keeps running with no registration hash.
worker:<id>is gone while the id is still in theworkersset, so anything reading the fleet sees a worker it cannot describe.It needs two workers registering close together, so it shows up on a host where a supervisor starts the fleet at once. Starting 15 workers together left 14 hashes carrying an expiry; starting them a second apart left none.
The fix
Swap the order: list the keys first, then snapshot.
register()doessaddbeforehmset, so a hash that exists at the moment the keys are listed already has its id in the set — and therefore in the snapshot taken after. Nothing that is alive can fall through the gap. A hash with no id in the set is still a real orphan and is still expired.One line moves. No behaviour changes other than which of the two reads happens first.
Test
tests/WorkerCleanupTest.phpasserts the order of the two reads, using aRedisstub that records the commands it is given and answers them with empty results. No Redis server, so it runs everywhere the suite already runs.It fails on
masterand passes with the change:Notes
Found in production on 4.x, where it had left a few hundred workers without a registration hash. We are running the equivalent change on a fork and it holds; this is the same fix against
master, with the test rewritten to suit this suite — our own needed a live Redis, which CI here has no service for.I could not run the full suite locally (
composer installwas unavailable in my environment), so CI on this PR is the first complete run. Happy to adjust naming or the test's shape if you would rather it sat somewhere else.