Fix execution queue zombies and stop re-queueing failing configs - #698
Open
arthurschreiber wants to merge 1 commit into
Open
Fix execution queue zombies and stop re-queueing failing configs#698arthurschreiber wants to merge 1 commit into
arthurschreiber wants to merge 1 commit into
Conversation
The execution scheduler could wedge for hours (observed: a 33h stall on the production host) because of three related issues in the cron queue: 1. executeElement mutated element.identifier.UUID between retries, then removed the element from the queue by its *current* identifier. Since the queue map is keyed by the identifier the element had when it was inserted, the delete missed and the entry leaked forever with Executing=true. Leaked entries fill the queue, count towards addToQueue's per-config limit, and make it report configs as "already full", starving new work. executeElement now removes the element by its original key on every exit path (extracted removeFromQueue). 2. Failed executions do not count towards MaximumBenchmarkWithSameConfig, so a permanently broken config (e.g. tpcc-olap timing out) was re-queued at full multiplicity on every cron tick and monopolised the benchmark machine. addToQueue now bails once a config has MaximumFailedBenchmarkWithSameConfig failed executions; a new git ref resets the counter. Counting uses the new exec.CountExecutionStatus, which reads the execution table alone (failed runs have no macrobenchmark row, so CountMacroBenchmark cannot see them). 3. cronExecutionQueueWatcher was a sleep-less busy loop pegging a CPU core on the shared web host; it now polls once a second. Co-Authored-By: Claude Opus 4.8 (1M context) <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.
Context
The execution scheduler could wedge for hours — a 33-hour stall was observed on the production host (
benchmark.vitess.io) between Jul 5 06:09 and Jul 6 15:35 UTC, and over a 4.3-day sample 47% of wall-clock time was idle. The root cause is a set of related bugs in the cron execution queue.Changes
1. Fix queue-zombie leak (the stall)
executeElementmutateselement.identifier.UUIDbefore each retry, then removed the element from thequeuemap by its current identifier. Since the map is keyed by the identifier the element had when it was inserted, the delete missed and the entry leaked forever withExecuting=true. Leaked entries fill the queue, count towardsaddToQueue's per-config limit, and make it report configs as"already full"— starving all new work.Verified in production: the midnight cron on Jul 6 logged
"not adding … already full"fortpcc-olap @ f240d1cbeven though the DB had 0 finished and 60 failed runs for it.executeElementnow captures the original key up front and always removes by it (extracted intoremoveFromQueue). The recursive retry was rewritten as an iterative loop so cleanup happens in exactly one place. A regression test (TestRemoveFromQueueAfterUUIDMutation) covers the mutate-then-remove case.2. Stop re-queueing permanently-failing configs
Failed executions do not count towards
MaximumBenchmarkWithSameConfig, so a config that always fails (e.g. a workload that always times out) was re-added at full multiplicity on every cron tick and monopolised the benchmark machine.addToQueuenow bails once a config reachesMaximumFailedBenchmarkWithSameConfig(20 = one full nightly cycle incl. retries) failed executions; a new git ref resets the counter. Counting uses the newexec.CountExecutionStatus, which reads theexecutiontable alone (failed runs have nomacrobenchmarkrow, soCountMacroBenchmarkcan't see them).3. Watcher busy-loop
cronExecutionQueueWatcherwas a sleep-less loop pegging a CPU core on the shared web host; it now polls once a second.Testing
go test ./go/server/... ./go/exec/...(incl. the new regression test) ✅golangci-lint run(v2.12.2, matching CI) — 0 issues ✅🤖 Generated with Claude Code