Repository navigation
Take the server-zd lease back after a step down with open calls - #1182
Merged
Merged
Conversation
A step down now ends the FlowService and WorkerTaskService calls still open at the stopping serving period with UNAVAILABLE right after the lease release. Their service clones, and so their local store handles, drop at once, and AdminService calls keep running. Before the stepped-down process reopens the store or competes for the lease again, it waits up to 60 s for the old store to close instead of continuing after 9 s. A store still held then means a stuck task: the process answers the step down with Internal and exits without taking the lease, so a restart releases the store. The ActiveNodeAdministration contract states this exception to the step down duration bound. Process tests step down the only process, cold and hot, while a 15 s hard-delete Attribute read is open, and check that the read ends and the process takes the lease back. See issue #1174.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
When a serving period starts, the standby-phase gRPC instance keeps accepting connections for 250 ms before it retires. Its FlowService and WorkerTaskService gates were never activated, so calls on those connections answered UNAVAILABLE "the node is starting" after the process already reported SERVING, as an integration test saw 223 ms after startup. The period now activates its public services on the standby instance's gates before the swap, and the serving instance shares those gates. Every call either instance accepts after the activation is served, and AdminService stays as it was on both instances. A process test calls GetServerInfo across a start over new and kept connections and checks that no call sent after the first served one answers "the node is starting".
A process test holds a transactional InvokeRPC at its Worker across a step down and answers it during the commit drain. The call must keep its answer, so ending calls at the start of the stop fails the test. Unit tests drive the wait for the stepped-down store on paused time with a store that stays open: the wait reports it still open after the documented 4 s plus 60 s, and cancellation ends it early. The decision after the wait is now a function: a store still open answers the step down Internal and ends the process with an error, without competing for the lease. The error log moves there from the wait. After a call, the gate also refuses the answer of a period that ended, so a later period's lease never lets it through. Comments, the README, and the operations guide in English and Simplified Chinese now say which calls end at the release, that PollWorkerTask streams already started run until their task ends, and that a stuck store ends the step down with INTERNAL and an exit after about 64 s.
A step down whose old local store stays open answers Internal only when no other process took over during the wait. When one did, the call already answered that it was taken over, and the process just exits 1. The operations guide in English and Simplified Chinese, the README, and the ActiveNodeAdministration contract now say so.
Sharing the gates let the retiring standby instance serve public calls, but that instance closes each connection after its 3 s age and 1 s grace, and its retirement aborts it 4 s after GOAWAY. A Worker stream registered there was cut with Unknown within 4 s, and its task result was lost; a long poll there was cut the same way. A serving instance now starts with its period's public services already active, and the standby instance it replaces never serves a public call. That instance sends GOAWAY at once, since it owes no step down answer, and a call that still reaches it answers UNAVAILABLE "this connection is closing; reconnect and retry the call". After a step down, the old serving instance answers UNAVAILABLE "this server process stepped down; retry the call" instead of "the node is starting". Process tests keep a Worker stream registered across a start open for 6 s and check that no public call sent after a take-back serves answers "the node is starting". The operations guide now says that a task result that arrives after the release is not recorded.
The replaced standby instance now sends GOAWAY at the takeover, so the step down test's describe calls can race that GOAWAY, and its step down nearly always goes through the serving instance. That instance then serves for only milliseconds before it steps down, which the readiness router can miss. The describe loop retries a call that raced the GOAWAY. When the step down went through the serving instance, the test records that serving period with the node group: the slot's standby write check restarts at its next confirmed standby, and the EpochStart bound allows the period the router missed.
On Linux a describe that races the replaced standby instance's GOAWAY fails with Internal "h2 protocol error" rather than Cancelled, which the step down test's describe loop did not retry. The loop now retries any call whose error comes from the transport, since the server never processed such a call, and still fails on any other status.
tonic's accept loop does not prioritize its stop signal, so a standby instance that a serving instance replaced could still accept a new connection and then close it at once with GOAWAY. A client's first call on that connection failed with Internal "h2 protocol error ... GoAway", as a public API test saw when it connected right after the dev server started. A stepped-down serving instance likewise took new connections until 250 ms after the take-back. Each instance now accepts through a gate that a newer instance closes before it starts: the old instance stops accepting at once and keeps its open connections, which then get the graceful two-phase GOAWAY of their retirement. A connection that the old instance accepted while it stopped is handed to its successor rather than served and closed. A stepped-down instance stops accepting without GOAWAY, so the step down answer still reaches its caller. The startup and take-back process tests now also check that every call on a connection opened after the process served is served.
This was referenced Nov 2, 2026
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.
Summary
Internal, unless another process already took over.InstanceListener,HandedOverConnections), so no new connection meets a GOAWAY before its first call.ActiveNodeAdministrationcontract, the server-zd README, and the operations guide (English and Simplified Chinese) describe the new behavior.Why
#1174: after a step down, a process that took its own lease back could exit with
Internalinstead of serving.WouldBlock).Reproduced with a 15 s hard-delete Attribute read held across the step down, and no other standby:
Internal9.1 s after the call, and the process exited.This PR's first CI run also exposed a startup race from #1168. After the swap, the standby-phase instance kept accepting connections for 250 ms, but its public gates were never activated. A server integration test got "the node is starting" 223 ms after readiness reported SERVING.
A first fix shared the gates between instances. It let a retiring instance cut Worker streams, and a later fix that served public calls only on serving instances made a health Check on a just-accepted connection fail with Internal "h2 protocol error ... GoAway": tonic's accept loop does not prioritize its stop signal, so a replaced instance could still accept a connection and close it at once. The accept gate fixes that.
Design decisions
ServerErrorvariant. The exit usesServingTaskFailed.LocalStoreCloseWaiter::for_directory_for_test, enabled through dexzd-server's dev-dependencies.Known gap, not changed here
Testing
standby_failover::step_down::{cold,hot}_take_back_ends_a_read_held_across_the_step_downInternal/WouldBlock, and the hot test's held read returned OK.step_down_keeps_the_answer_of_a_call_that_ends_before_the_release: a transactional InvokeRPC that its Worker answers during the commit drain keeps its OK answer. Moving the call cancellation to the start of the stop fails this test.startup_callsno_public_call_answers_starting_once_the_process_servesandno_public_call_answers_starting_once_a_take_back_servessend GetServerInfo over new and kept connections across a start or a take-back. No call sent after the first served call may answer "the node is starting", and every call on a connection opened after that point must be served.a_worker_stream_kept_across_the_start_stays_open: a Worker stream is accepted only on an instance where it stays open.public_api_authentication::flow_and_worker_calls_need_valid_credentials, which hit the GoAway failure, passed 20/20 locally.node_lifecycle::local_store_close_tests, with paused time and a store that stays open:Internaland ends the process with an error.step_down_with_calls_in_flight_leaves_a_hot_standbynow proves through the period-end message that its read was still open at the release.NodeGroup::note_serving_period), so the harness EpochStart count allows it.standby_failoverthree rounds 39/39,startup_calls5/5. Earlier heads also ran the fullserve_processsuite and e2e 46/46.cargo fmt --check,make clippy,make rust-rules-check,make copyright-check, andcd docs && npm run check.Fixes #1174