Fix permanent shard loss when RevokeShards times out - #3766
Open
Aditya1404Sal wants to merge 2 commits into
Open
Fix permanent shard loss when RevokeShards times out#3766Aditya1404Sal wants to merge 2 commits into
Aditya1404Sal wants to merge 2 commits into
Conversation
✅ Deploy Preview for golemcloud canceled.
|
Aditya1404Sal
force-pushed
the
fix/revoke-shards-background-drain
branch
3 times, most recently
from
August 26, 2026 07:09
9267d90 to
4849499
Compare
Aditya1404Sal
marked this pull request as ready for review
August 26, 2026 08:12
…ound, leave failed-revoke shards unassigned and reconcile the old executor
…ntil each is unloaded, raise the revoke deadline to 60s
Aditya1404Sal
force-pushed
the
fix/revoke-shards-background-drain
branch
from
August 26, 2026 19:58
4849499 to
84e0086
Compare
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.
fixes GOL-474
Problem
Chaos run on golem-dev (2026-08-18): 460 of 1024 shards had no owner for 43 minutes and the cluster never repaired itself.
RevokeShardson the executor removed the shards from its local assignment and then drained the affected agents one at a time, each step paying the agent's teardown (oplog commit + status flush + checkpoint). With enough agents that exceeded the shard manager's 5 s deadline. On failure the shard manager calledRebalance::remove_shards, which dropped the shards from both halves of the plan, so the routing table kept crediting the old executor (which had already dropped them locally) and the loop replayed the identical plan forever.A first version of this PR detached the drain into a background task. That was wrong (thanks @vigoo): returning before the agents are stopped lets an agent keep running on the old executor while the new owner recovers it, and both write the same oplog. Without an epoch fence the synchronous drain is the ordering barrier.
Change
Executor (
grpc/mod.rs) — the drain stays on the RPC, but:Worker::is_loaded()turning false, not the interrupt acknowledgement. The ack is sent fromset_suspendedbefore the invocation loop commits the oplog and flushes status;is_loaded()only flips insidestop_internal_lockedafter that commit and flush, on every exit path (including the ones that never remove the worker fromActiveAgents). Loaded-but-idle agents get no ack receiver but are woken and unload through the same path, so the same barrier covers them;SetShardAssignmentgets the same synchronous drain for the shards it removes (it is the reconcile the shard manager sends after a failed revoke); recovery of gained shards stays detached for bothAssignShardsandSetShardAssignment;InterruptKind::Restart→RetryDecision::Immediateused to re-instantiate on the losing executor), andon_shard_assignment_changedre-checks ownership before each restart.Shard manager
remove_assignment_shards, one-sided) and sends the old pod its authoritativeSetShardAssignmentin the same pass, before the next pass hands the shards to another pod. A failed assignment also queues the pod for reconciliation.revoke_shards_timeout5 s → 60 s. It bounds a drain whose duration is one agent's suspend, not the agent count; withretriesit is a liveness backstop (5 × 60 s is the longest a wedged-but-alive executor can hold one pass before its shards are released).SetShardAssignmentis wrapped inrevoke_shards_timeout(it shared the 5 sassign_shards_timeout).Tests
golem-worker-executor/tests/sharding.rs(new, group2, sequential):revoke_shards_returns_only_after_lost_agents_are_unloaded,revoke_shards_drains_lost_agents_concurrently(per-agent teardown commits delayed through a one-shot test-utils seam inTestOplog::commit; asserts the teardowns overlap),revoke_shards_unloads_idle_agents,set_shard_assignment_drains_lost_agents_before_returning. All four fail against the background drain (the RPC returned in <1 ms with the agents still loaded).golem-shard-manager/tests/shard_management.rs:revoke_timeout_after_executor_applied_it_does_not_strand_shards(the production failure shape; never converges onmain),failed_revoke_reconciles_old_executor_before_reassigning,unreachable_executor_still_releases_its_revoked_shards.Accepted behaviour / not in scope
Set, then its shards are released. That wait is the point.retries_on_unavailableloop retries the 10 s connect timeout inside each of the 5 attempts, so a black-holed pod can hold one pass for several minutes (before: ~25 s, cut off by the 5 s deadline). Left as is to keep this change minimal; lowering the client's connect timeout or its inner retries is a config-only knob if it shows up.claim_dueand processing; the rebalance loop has no backoff.Verification
cargo test -p golem-shard-manager --lib: 51 passed;--test integration(Postgres testcontainer): 15 passed, including the three revoke-failure tests.cargo test -p golem-worker-executor --test integration -- sharding::: 4 passed. Against the previous background drain the same four fail (... was still loaded when revoke_shards returned (after 0.5–0.8 ms)).Clocksagents drain in ~12 ms; 5 idle agents in ~260 ms; with a 200 ms artificial teardown commit per agent the concurrent drain takes ~330 ms (sequential would be ≥ 1 s).api::interruption,api::simulated_crash,wasi::sleep_*): 7 passed.cargo clippy --all-targetsongolem-shard-manager,golem-worker-executor,golem-worker-executor-test-utils: clean;cargo fmt --all -- --check: clean.golem-shard-manager/config/*regenerated from the binary: onlyrevoke_shards_timeout = "1m"changes.integration-tests --test sharding(coordinated_scenario_01_02,service_is_responsive_to_shard_changes) against rebuilt service binaries: 2 passed (81 s).