fix(executor): bind the gRPC listener before registering with the scheduler - #2225
Merged
andygrove merged 3 commits intoAug 4, 2026
Merged
Conversation
…eduler The executor spawned its tonic server and registered with the scheduler immediately afterwards. Server::serve binds inside the future it returns, so the socket did not exist yet, while the scheduler's registration handler dials that port back via a single connect with no retry. Losing that race gave ECONNREFUSED, failed registration, and killed the executor at startup. Bind the listener on the current task before spawning, and serve from it with serve_with_incoming_shutdown, so registration cannot run before the port accepts connections. tonic ignores the builder's tcp_nodelay and tcp_keepalive when serving from a pre-bound listener, so the new create_grpc_server_incoming helper applies them itself.
andygrove
marked this pull request as ready for review
August 4, 2026 22:14
Member
Author
|
@villebro @akshaychitneni could you review? |
avantgardnerio
approved these changes
Aug 4, 2026
avantgardnerio
left a comment
Contributor
There was a problem hiding this comment.
Thank you for fixing this!
Nit: the body claims test_create_grpc_server_incoming_port_in_use but I don't see that in the code?
Member
Author
|
Good catch, thanks. That test was dropped by a cleanup commit rather than renamed, so it really wasn't there. Restored it in d76c435, and fixed the body which was also still using the old name for the other test. |
This was referenced Aug 6, 2026
andygrove
added a commit
that referenced
this pull request
Aug 6, 2026
…eduler (#2225) (#2236) * fix(executor): bind the gRPC listener before registering with the scheduler The executor spawned its tonic server and registered with the scheduler immediately afterwards. Server::serve binds inside the future it returns, so the socket did not exist yet, while the scheduler's registration handler dials that port back via a single connect with no retry. Losing that race gave ECONNREFUSED, failed registration, and killed the executor at startup. Bind the listener on the current task before spawning, and serve from it with serve_with_incoming_shutdown, so registration cannot run before the port accepts connections. tonic ignores the builder's tcp_nodelay and tcp_keepalive when serving from a pre-bound listener, so the new create_grpc_server_incoming helper applies them itself. * refactor: tighten create_grpc_server_incoming docs and tests * test: restore port-in-use test for create_grpc_server_incoming (cherry picked from commit 984513e)
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.
Which issue does this PR close?
Closes #2224.
Rationale for this change
In push-based scheduling the executor tells the scheduler about itself before its own gRPC server is listening, and the scheduler dials that port back to verify connectivity. When the callback wins the race the executor dies at startup.
The ordering in
executor_server::startupwas:tokio::spawnthe tonic server.Server::servebinds inside the future, so the socket does not exist yet whenspawnreturns — even though the"... Grpc Server listening on ..."line is already in the log.register_executor, immediately.On the scheduler side
ExecutorManager::register_executorcallstest_connectivity, a singleExecutorGrpcClient::connect(...)with no retry and no backoff. If the spawned task has not reached itsbindyet, that connect getsECONNREFUSED, registration returns an error, andexecutor_processtreats a registration error as fatal, so the executor exits instead of joining the cluster.The opposite direction is already tolerant: the executor retries its connection to the scheduler in a loop. Only the scheduler's callback to the executor is one-shot, which is why the executor's own startup ordering has to be right.
It is timing-dependent and so shows up rarely and on loaded machines. The instance that prompted this was CI, where one of two executors lost the race and the HA chaos harness timed out waiting for it:
The consequence outside of tests is the same shape: an executor that is otherwise healthy fails to join, and in a restart loop it can keep failing to join.
What changes are included in this PR?
ballista/core/src/utils.rs— newcreate_grpc_server_incoming(addr, &GrpcServerConfig), which binds the listening socket eagerly and returns tonic'sTcpIncoming. tonic ignores the server builder'stcp_nodelay/tcp_keepalivewhen serving from a pre-bound listener, so the helper applies the same valuescreate_grpc_serversets and keeps the two in one place. The remaining settings (timeout, HTTP/2 keep-alive) still come from the builder as before, so the socket is configured exactly as it was.ballista/executor/src/executor_server.rs—startupbinds via that helper on the current task, before spawning, and the spawned task now serves withserve_with_incoming_shutdown. Registration therefore cannot run before the port is accepting connections. This retires the standing// TODO the executor registration should happen only after the executor grpc server started.The"listening on"log line is now true when it is printed.One incidental improvement falls out of binding eagerly: a port conflict now fails
startupdirectly, with the bind error, rather than being discovered later through the spawned server task.No change to the scheduler's
test_connectivity. The issue also floats giving it a bounded retry; that is worth doing on its own merits for genuinely remote executors, but it is a separate resilience change and is not needed to fix this race, which is entirely local to the executor's startup ordering.New tests in
ballista-core:test_create_grpc_server_incoming_binds_eagerly— connects to the bound address while nothing is serving on it. This is the property the fix depends on, and it fails against a lazily-bound socket.test_create_grpc_server_incoming_port_in_use— binding an address twice is an error rather than a panic, so a port conflict still surfaces as a normal startup failure.Are there any user-facing changes?
No behaviour changes and no API breakage.
create_grpc_server_incomingis a new public function inballista-core, additive alongsidecreate_grpc_server. Note that it must be called from within a Tokio runtime, since the listener registers with the reactor; this is documented on the function.