fix: move shard split ownership into shard controller

[mattisonchao]

Motivation

Shard splitting currently has two independent owners: the coordinator runtime tracks a standalone split controller, while the parent shard controller owns elections, ensemble changes, and deletion. The runtime lock does not serialize with the parent controller event loop, so both paths can clone and write the same shard metadata. A split-phase write based on a stale snapshot can overwrite a newer term, leader, or ensemble transition.

We also found that replacing a namespace snapshot during a split could discard a concurrent update to another shard. The standalone split controller duplicates lifecycle and recovery ownership, and review exposed related failure paths: incomplete split metadata could leave a lifecycle running, runtime callbacks could create child controllers during shutdown, and closing an aborted child controller while holding the runtime lock could block callback progress. The parent shard controller is the correct ownership boundary for the split lifecycle and its metadata changes.

Modifications

• add a SplitAction queue and serialize split initiation with the parent shard controller event loop
• move split lifecycle ownership to the parent controller context and resume persisted split state there
• marshal split metadata mutations through the same event loop as elections, ensemble changes, and deletion
• add an atomic UpdateSplitShardStatus(namespace, shard, split, children) metadata operation without changing UpdateNamespaceStatus
• keep parent split metadata as the single persisted lifecycle-phase source and use UpdateShardStatus for phase transitions
• merge split state into the latest parent metadata so concurrent parent and other-shard changes are preserved
• validate split action identity, configuration, and complete creation metadata
• reject new split requests after runtime shutdown begins
• abort pre-fence splits on operation timeout, but preserve persisted split metadata during controller shutdown for recovery
• close split state only after its owning event loop stops, and ignore split callbacks after runtime shutdown begins
• remove aborted child controllers under the runtime lock, then close them after releasing it
• add regression coverage for ordering, stale snapshots, incomplete metadata, concurrent parent changes, lock scope, timeout cleanup, restart state, request admission, and concurrent shutdown

Testing

• make lint
• go test ./oxiad/...
• go test -race ./oxiad/coordinator/runtime ./oxiad/coordinator/runtime/controller/shard -count=1
• git diff --check

[Copilot]

Pull request overview

This PR moves shard split initiation and lifecycle ownership into the parent shard controller’s event loop, aiming to eliminate stale-snapshot metadata writes and serialize split metadata mutations with elections/ensemble changes/deletion in the coordinator runtime.

Changes:

• Introduces a shard-controller–owned split action path (SplitAction + controller queue) and a new Splitting state machine that runs under the shard controller’s ownership.
• Refactors metadata writes to use Metadata.UpdateNamespaceStatus(update func(*NamespaceStatus) bool) bool so callers merge changes into the latest snapshot under the metadata write lock.
• Adds/updates regression tests to validate split ordering/serialization, metadata update safety, and shutdown/resume behavior.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
oxiad/coordinator/runtime/runtime.go	Removes standalone split controller ownership; delegates split initiation to shard controllers and creates child controllers on SplitStarted.
oxiad/coordinator/runtime/controller/shard/shard_splitter.go	Adds Splitter to validate + persist initial split metadata via UpdateNamespaceStatus.
oxiad/coordinator/runtime/controller/shard/shard_splitter_test.go	Adds unit tests for splitter correctness and split action serialization.
oxiad/coordinator/runtime/controller/shard/shard_controller.go	Adds split action queueing + split metadata update queue; resumes persisted splits under controller ownership.
oxiad/coordinator/runtime/controller/shard/shard_controller_test.go	Updates controller test helper to pass splitter config.
oxiad/coordinator/runtime/controller/shard/shard_controller_split.go	Refactors split controller into Splitting state machine owned by shard controller.
oxiad/coordinator/runtime/controller/shard/shard_controller_split_test.go	Updates split lifecycle tests to run via shard controller context; adds new regression tests.
oxiad/coordinator/runtime/controller/interfaces.go	Extends ShardSplitEventListener with SplitStarted.
oxiad/coordinator/runtime/balancer/state/grouping_test.go	Updates test comment to reflect new split ownership model.
oxiad/coordinator/runtime/balancer/scheduler_test.go	Updates mock metadata to support functional UpdateNamespaceStatus.
oxiad/coordinator/runtime/autosplit/monitor_test.go	Updates namespace status update call to new UpdateNamespaceStatus API.
oxiad/coordinator/runtime/action/split.go	Adds SplitAction and SplitResult for split initiation through controller queue.
oxiad/coordinator/runtime/action/action.go	Adds Split to the action type enum.
oxiad/coordinator/reconciler/namespace_reconciler_test.go	Updates mock metadata to new UpdateNamespaceStatus signature/behavior.
oxiad/coordinator/metadata/metadata.go	Changes UpdateNamespaceStatus to a functional, lock-serialized update API with retry semantics and return value.

Suppressed comments (1)

oxiad/coordinator/runtime/controller/shard/shard_controller_split.go:109

• Splitting.Start() can resume an in-progress split solely based on persisted metadata, but it doesn't guard that required dependencies are configured (notably eventListener and executeMetadataUpdate). If a controller is constructed without split config (e.g., in tests or future callers) and encounters persisted split metadata, the state machine can later nil-deref when emitting events or mutating metadata. Add a fast-fail guard before starting.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (1)

oxiad/coordinator/runtime/runtime.go:634

• When the parent shard exists in metadata but its controller is missing, this returns the same "shard not found" error as the earlier metadata check (line 625). That makes troubleshooting ambiguous; this branch is specifically a missing controller, not missing shard metadata.

	c.RLock()
	sc, exists := c.shardControllers[parentShardId]
	c.RUnlock()
	if !exists {
		return 0, 0, errors.Errorf("shard %d not found in namespace %q", parentShardId, namespace)
	}

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (2)

oxiad/coordinator/metadata/metadata.go:523

• applyShardSplitPhase validates that the parent has two child shard IDs and that each child has non-nil Split metadata, but it does not validate that each child's Split.ParentShardId matches parentShard. That can allow UpdateShardSplitPhase to advance phases even when the parent/child split relationship is inconsistent (e.g., corrupted or mismatched metadata), which risks masking the inconsistency and driving the state machine forward on the wrong shards.

		childMetadata, exists := namespaceStatus.Shards[childShard]
		if !exists || childMetadata.Split == nil {
			return false, fmt.Errorf("split metadata for shard %d not found while updating split phase", childShard)
		}
		shardMetadata = append(shardMetadata, childMetadata)

SHARD_SPLITTING_PLAN.md:3

• Line has trailing whitespace (Status: Draft␠␠), which is typically flagged by git diff --check and can cause CI/style gates to fail. Remove the trailing spaces (a Markdown hard line break doesn’t seem needed here since the next line is already a new paragraph).

Status: Draft  

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (1)

oxiad/coordinator/runtime/runtime.go:733

• SplitAborted holds the runtime mutex while calling sc.Close() for child shard controllers. Close() can block (it waits for the controller goroutine to stop), so doing it under the runtime lock can stall other coordinator operations and increases deadlock risk if any shutdown path tries to acquire the runtime lock while waiting on a controller to close.

Consider removing the child controllers from the map and recomputing assignments while holding the lock, then releasing the lock before actually closing the controllers.

	for _, childId := range []int64{leftChild, rightChild} {
		if sc, exists := c.shardControllers[childId]; exists {
			_ = sc.Close()
			delete(c.shardControllers, childId)
		}

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (1)

oxiad/coordinator/runtime/controller/shard/shard_controller_split.go:160

• runSplitStateMachine() currently calls abort() on any ctx error (including context.Canceled). During controller shutdown, enqueueSplitMetadataUpdate short-circuits on ctx.Done(), so abort() can end up notifying SplitAborted without being able to delete child shard status / clear parent split metadata, leaving metadata inconsistent with runtime.SplitAborted’s assumptions.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[mattisonchao]

@copilot review

[mattisonchao]

@copilot review

[mattisonchao]

@copilot review

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[fenos]

@copilot review