worker: retry the connect+auth+registration handshake on transient stalls#1118
Open
MauriceDHanisch wants to merge 1 commit into
Open
worker: retry the connect+auth+registration handshake on transient stalls#1118MauriceDHanisch wants to merge 1 commit into
MauriceDHanisch wants to merge 1 commit into
Conversation
…nt stalls A busy server (mid MILP scheduler solve on its single-threaded executor, or handling a burst of other workers registering) could fail to poll a new worker's connection in time for any leg of the auth handshake or the registration round-trip to complete within its own hardcoded 15s timeout. None of those legs were retried (only the raw TCP connect already was), so one slow attempt killed the whole hq worker start process -- on an autoalloc-managed SLURM pilot, that means the entire allocation is wasted, not just a delayed worker. Reproduced live: a combined registration-burst + real scheduler-load stress test that previously left a second batch of workers permanently unable to register now has all of them connect cleanly (tested up to 30 workers). Retries the whole connect+authenticate+register+await-response sequence (connect_and_register_with_retry, 8 attempts / 10s backoff) rather than raising the fixed 15s timeouts themselves, since there's no fixed stall duration to size a bigger timeout against. Also fixes two latent panics on the registration-response wait (a timeout or early connection close used to abort the process outright instead of returning a proper error). Added DsError::AuthenticationRejected, distinct from the existing transient GenericError, so a deterministic rejection (wrong secret key, incompatible protocol/role) fails fast instead of retrying 8 times for an outcome that will never change -- caught by the existing test_secret.rs tests, which went from ~0.1s to ~70s combined before this fix. Scoped to the worker's own connection to the server only (worker/rpc.rs); client CLI commands and server-side code are untouched.
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.
Summary
connect_and_register, and wraps it inconnect_and_register_with_retry(8 attempts, 10s backoff between attempts).DsError::AuthenticationRejected, a distinct, non-retryable error variant for deterministic auth rejections (wrong secret key, incompatible protocol/role, bad challenge, etc.), and updatesfinish_authentication's rejection sites inauth.rsto construct it explicitly instead of falling through the genericGenericErrorcatch-all. The retry loop treats this variant as immediately fatal; every other error is retried.panic!s in the old inline registration-response-wait code with properErr(...)returns.Motivation
The raw TCP connect step already retried (20x/2s via
connect_to_server), but nothing downstream of it did. On a busy server — mid MILP scheduler solve on its single-threaded executor, or handling a burst of other workers registering concurrently — any leg of the auth handshake or the registration round-trip could miss its own hardcoded 15s timeout simply because the server didn't get scheduled in time to answer, not because anything was actually wrong. That killed the wholehq worker startprocess outright. On an autoalloc-managed SLURM pilot, that means the entire allocation is wasted, not just one delayed worker.Reproduced live with a combined registration-burst + real scheduler-load stress test: previously a second batch of workers was left permanently unable to register; with this change all of them (tested up to 30 workers) connect and register cleanly.
Client-side CLI RPCs (
Connection::initinconnection.rs) are a separate caller ofdo_authenticationand are intentionally untouched — those should keep failing fast rather than retrying.Test plan
cargo check -p hyperqueue -p takocargo test -p tako— 205 passed, 0 failed, ~5s (confirmed the three deterministic-auth-rejection tests intest_secret.rsstay fast — they hit the new non-retryableAuthenticationRejectedpath instead of looping through all 8 retry attempts)