Skip to content

Fix permanent shard loss when RevokeShards times out - #3766

Open
Aditya1404Sal wants to merge 2 commits into
golemcloud:mainfrom
Aditya1404Sal:fix/revoke-shards-background-drain
Open

Fix permanent shard loss when RevokeShards times out#3766
Aditya1404Sal wants to merge 2 commits into
golemcloud:mainfrom
Aditya1404Sal:fix/revoke-shards-background-drain

Conversation

@Aditya1404Sal

@Aditya1404Sal Aditya1404Sal commented Aug 25, 2026

Copy link
Copy Markdown

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.

RevokeShards on 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 called Rebalance::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:

  • every lost agent is signalled at once and awaited together, so the wait is the slowest agent's teardown rather than the sum of all teardowns;
  • the barrier is Worker::is_loaded() turning false, not the interrupt acknowledgement. The ack is sent from set_suspended before the invocation loop commits the oplog and flushes status; is_loaded() only flips inside stop_internal_locked after that commit and flush, on every exit path (including the ones that never remove the worker from ActiveAgents). Loaded-but-idle agents get no ack receiver but are woken and unload through the same path, so the same barrier covers them;
  • the drain runs as a spawned task that the RPC awaits, so a client deadline that drops the request cannot leave agents un-signalled; a retried call converges on whatever is still loaded;
  • it loops until a snapshot finds nothing loaded in a lost shard (agents whose creation was in flight are invisible to the first snapshot);
  • SetShardAssignment gets 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 both AssignShards and SetShardAssignment;
  • the invocation loop unloads an agent whose shard is no longer owned instead of restarting it in place (InterruptKind::RestartRetryDecision::Immediate used to re-instantiate on the losing executor), and on_shard_assignment_changed re-checks ownership before each restart.

Shard manager

  • A failed revoke leaves the shards unassigned (remove_assignment_shards, one-sided) and sends the old pod its authoritative SetShardAssignment in the same pass, before the next pass hands the shards to another pod. A failed assignment also queues the pod for reconciliation.
  • revoke_shards_timeout 5 s → 60 s. It bounds a drain whose duration is one agent's suspend, not the agent count; with retries it is a liveness backstop (5 × 60 s is the longest a wedged-but-alive executor can hold one pass before its shards are released).
  • SetShardAssignment is wrapped in revoke_shards_timeout (it shared the 5 s assign_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 in TestOplog::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 on main), failed_revoke_reconciles_old_executor_before_reassigning, unreachable_executor_still_releases_its_revoked_shards.

Accepted behaviour / not in scope

  • A wedged-but-alive executor (an agent that never reaches an interrupt point, or a very long replay) holds the loop for up to 5 × 60 s on the revoke and again on the same-pass Set, then its shards are released. That wait is the point.
  • An executor that becomes unreachable mid-revoke costs more than before because of the longer deadline: the client's retries_on_unavailable loop 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.
  • After a blown deadline the shard manager still releases the shards: same exposure the eviction path has today; the redesign's epoch fence is the structural fix, and without the release the 43-minute loop comes back.
  • Pre-existing, separate follow-ups: the scheduler does not re-check shard ownership between claim_due and 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)).
  • Drain log on the test executor: 5 busy Clocks agents 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).
  • Interruption-adjacent slices (api::interruption, api::simulated_crash, wasi::sleep_*): 7 passed.
  • cargo clippy --all-targets on golem-shard-manager, golem-worker-executor, golem-worker-executor-test-utils: clean; cargo fmt --all -- --check: clean.
  • golem-shard-manager/config/* regenerated from the binary: only revoke_shards_timeout = "1m" changes.
  • e2e integration-tests --test sharding (coordinated_scenario_01_02, service_is_responsive_to_shard_changes) against rebuilt service binaries: 2 passed (81 s).

@netlify

netlify Bot commented Aug 25, 2026

Copy link
Copy Markdown

Deploy Preview for golemcloud canceled.

Name Link
🔨 Latest commit 84e0086
🔍 Latest deploy log https://app.netlify.com/projects/golemcloud/deploys/6a8f457c7c4b2900081e42ed

@Aditya1404Sal
Aditya1404Sal force-pushed the fix/revoke-shards-background-drain branch 3 times, most recently from 9267d90 to 4849499 Compare August 26, 2026 07:09
@Aditya1404Sal
Aditya1404Sal marked this pull request as ready for review August 26, 2026 08:12
@Aditya1404Sal
Aditya1404Sal requested a review from a team 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
Aditya1404Sal force-pushed the fix/revoke-shards-background-drain branch from 4849499 to 84e0086 Compare August 26, 2026 19:58
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.

1 participant