fix: don't hang coordinator Close while metadata status writes fail

[merlimat]

Motivation

GrpcServer.Close() closes the runtime before the metadata. The metadata status writers (UpdateShardStatus, DeleteShardStatus, ReserveShardIDs, ...) retry forever on the metadata context (NewBackOff sets MaxElapsedTime: 0), and only coordinatorMetadata.Close() cancels that context. Suppose the status writes keep failing while an election is in flight: after the coordinator lost its leadership (raft VerifyLeader, a configmap conflict), or with an unreachable Kubernetes API. The shard controller's run goroutine then stays in Election.start → UpdateShardStatus. shard.(*controller).Close() waits for it, so runtime.Close() never returns, and neither does Close():

• GrpcServer.Close → runtime.Close → shard.(*controller).Close → sync.(*WaitGroup).Wait
• backoff.RetryNotify → coordinatorMetadata.UpdateShardStatus → Election.start → Election.Start → controller.onElectLeader → controller.handleDataServerFailure → controller.run

With the CLI this only delays shutdown until SIGKILL. An embedder that closes the coordinator after losing the leadership, as the WithOnLeadershipLost option in #1281 documents, hangs instead. The same wait also affects a split controller writing its state, and the reconciler or InitiateSplit (which holds the runtime lock) while they are in ReserveShardIDs.

Changes

• NewGrpcServer creates the server context before the metadata and derives the metadata context from it. The ctxCancel() at the top of Close() now stops the retries before the runtime is closed. The runtime.Close() ordering is unchanged, and the metadata is still closed after the runtime.
• Once canceled, a status write gives up after at most one more attempt, so the writers now report the failure. ReserveShardIDs, UpdateNamespaceStatus, UpdateShardStatus and DeleteShardStatus return an error. CreateNamespaceStatus returns false, and DeleteNamespaceStatus returns an empty status. Before, CreateNamespaceStatus reported a failed write as a created namespace, and the runtime would have started controllers that panic on the missing shard status.
• UpdateNamespaceStatus and UpdateShardStatus also fail with ErrNotFound when the namespace or the shard is gone. Before, they silently did nothing, and UpdateShardStatus re-created a deleted shard, so a nil error now always means the write was persisted.
• Callers stop instead of acting on state that was not persisted:
  • An election does not fence the ensemble with a term it failed to persist, because a later election (possibly by the new leader coordinator) could reuse that term: data servers accept a NewTerm with an equal term. It also does not report a leader it failed to record.
  • The split controller stops with a permanent error and resumes from the persisted state after a restart. Say the coordinator loses its leadership mid-cutover, after the parent fence was persisted. Going past the failed writes would re-elect the children and call SplitComplete, whose DeleteShard (at the persisted, fenced term) deletes the parent while the new leader coordinator still has the split in progress. Retrying in process isn't safe either, since re-entering runCutover after a partial update can reset the split to Bootstrap. A split abort that fails to persist is also left to resume.
  • CreateNamespace and InitiateSplit fail before creating any controllers. A shard deletion whose status removal fails resumes after a restart. The periodic pending-delete update stays best-effort.
• runCutover's final metadata updates move to detachChildren, to keep it under the cyclomatic complexity limit.

Testing

• TestCoordinator_CloseWhileStatusWritesFail (tests/coordinator): 3 data servers and the file metadata provider. Once the shard is up, the test makes cluster-status.json read-only and closes the data servers, so an election gets stuck persisting its new term. Close() must then return. On main it is still blocked when the test gives up. The test is skipped when file permissions are not enforced (root).
• TestController_ElectionDoesNotFenceWithUnpersistedTerm: no NewTerm is sent when the term cannot be persisted. The test fails if the election ignores the write error.
• TestMetadataStatusWritersGiveUpOnceCanceled: once the context is canceled, every writer reports the failure and nothing is persisted. The test fails with the old CreateNamespaceStatus result.
• TestMetadataStatusUpdatesFailWhenTargetIsGone: the status updates return ErrNotFound for a missing namespace or shard, and a deleted shard is not re-created.
• go test -race passes on oxiad/coordinator/... and on the whole tests module.
• golangci-lint v2.13.2: 0 issues on all go.work modules.

Note: Close() still takes about 15s in the new test. The data-server controllers' health retry loops use ConcurrentBackOff, which hides the backoff context from backoff.RetryNotify, so their backoff sleep (10s initially, up to 60s) ignores the cancellation. That is left for a separate change.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved persistence and split-recovery defects can leave metadata inconsistent or allow unsafe progression.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 · 2

Open (6)

• Reject split initiation when the namespace is missing · New
• Treat missing namespace or shard as an election update error · New
• Only apply backoff.Permanent to non-nil update errors · New
• Prevent controllers from starting after metadata deletion · New
• Recover when fencing succeeds but parent persistence fails · New
• Make child cleanup atomic to prevent partial split deletion · New

What changed in this PR

This PR prevents coordinator shutdown hangs by canceling metadata retries and propagating persistence failures through elections, namespace operations, shard deletion, and splits.

Changes:

• Derives metadata operations from the server cancellation context.
• Stops unsafe controller actions after failed persistence.
• Adds shutdown and metadata failure tests.

File	Summary
tests/​coordinator/​coordinator_test.go	Checks status-write errors.
tests/​coordinator/​close_e2e_test.go	Adds shutdown regression coverage.
oxiad/​coordinator/​runtime/​runtime.go	Handles namespace and split persistence failures.
oxiad/​coordinator/​runtime/​controller/​shard/​shard_split_controller.go	Propagates split metadata errors.
oxiad/​coordinator/​runtime/​controller/​shard/​shard_split_controller_test.go	Updates split persistence assertions.
oxiad/​coordinator/​runtime/​controller/​shard/​shard_controller.go	Handles deletion persistence failures.
oxiad/​coordinator/​runtime/​controller/​shard/​shard_controller_test.go	Tests election behavior on failed writes.
oxiad/​coordinator/​runtime/​controller/​shard/​shard_controller_election.go	Requires term and leader persistence.
oxiad/​coordinator/​runtime/​balancer/​scheduler_test.go	Updates metadata mocks.
oxiad/​coordinator/​runtime/​autosplit/​monitor_test.go	Checks update errors.
oxiad/​coordinator/​reconciler/​namespace_reconciler_test.go	Updates metadata mocks and assertions.
oxiad/​coordinator/​metadata/​metadata.go	Returns status-write errors.
oxiad/​coordinator/​metadata/​metadata_test.go	Tests canceled writers.
oxiad/​coordinator/​grpc_server.go	Cancels metadata retries during close.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.