Fix Zoekt planner placing two indices for one namespace on one node

🤖 AI-authored change.

Fixes two node-selection defects in Search::Zoekt::PlanningService, each of which can plan two indices for one namespace on the same node — which the unique index on zoekt_indices (zoekt_enabled_namespace_id, zoekt_node_id) forbids. Both return a plan with an empty errors array, so the failure only appears later as PG::UniqueViolation inside ProvisioningService, which rolls back and stamps last_rollout_failed_at. To an operator the namespace simply stops provisioning, with nothing reported by planning.

Before	After
a replica's first index never retired its node, so a later index in the same replica could land back on it	the node is retired when the index is opened; filling the current index first is now an explicit early return
the exclusion set started empty every run, so planning a 1→2 replica delta could not see nodes the namespace's persisted indices already hold	the set is seeded from the namespace's existing indices before the create branch plans anything

• Bug A already fires in production — namespace 8338830 planned 10 indices across only 9 distinct nodes. It needs three indices in one replica to trigger, which is why the existing suite never caught it. Closes #627730 (closed)
• Bug B is dormant at today's default — zoekt_default_number_of_replicas is 1, so EnabledNamespace.with_mismatched_replicas never selects an already-provisioned namespace. Raising that setting makes every provisioned namespace reachable. Closes #627737
• One query for the batch, not one per namespace — the existing-index node lookup is loaded once for the whole planning run (up to 128 namespaces). ~database reviewers: both query shapes and their EXPLAIN (ANALYZE, BUFFERS) plans are at $6053835 — measured on a synthetic table, not a production clone.
• Not in scope: memory-aware placement. This is a prerequisite for it, because that work extends the same find_best_node / @exhausted_node_ids path — see epic 23519 and work item 627279.

Reviewer focus: the two new specs are the contract (ee/spec/services/search/zoekt/planning_service_spec.rb:592-698) — worth running them against master to confirm they fail there. Second, whether the early return in assign_to_node (ee/app/services/search/zoekt/planning_service.rb:203-210) leaves storage accounting unchanged when the current index has room: extend_index (:226-231) versus open_index_on_node (:233-246).

---

This was posted automatically. See a way this automation could be improved? Feel free to mention @johnmason and leave feedback in a comment.

🤖 Mechanism, citations and verification (for agents / deep readers)

Mechanism

Bug A — within a single replica. The old assign_project_to_index added a node to @exhausted_node_ids only when last_index was present, so a replica's first index never retired its node. nodes is sorted by unclaimed space descending in format_nodes and never re-sorted, so that node keeps being re-checked first by find_best_node (ee/app/services/search/zoekt/planning_service.rb:222-224). The fix separates the two decisions that were entangled in one method: assign_to_node (:203-220) extends the current index when it has room (extend_index, :226-231), and otherwise open_index_on_node (:233-246) adds the node to @exhausted_node_ids unconditionally (:242), at the moment the node is committed to.

Bug B — across replicas. replica_plans is per-run state, so on the incremental path (namespace provisioned with one replica, desired count raised to two) only the delta is planned and the nodes held by persisted indices are invisible to find_best_node. Fix: exclude_nodes_holding_existing_indices is called in generate's when :create (:117) and merges the persisted node ids into the exclusion set (:161-163). This is a regression against a closed requirement — work item 593209 §1 asked for anti-affinity across a namespace's replicas and is workflow::complete; that guarantee holds when replicas are planned in the same run, not on the incremental path.

Batching. PlanningService#plan (:18-23) loads (zoekt_enabled_namespace_id, zoekt_node_id) for the whole batch via node_ids_in_use_by_enabled_namespace_id (:45-55) and passes each namespace's node ids into its Plan; Plan keeps the per-namespace lookup as a fallback for a directly constructed Plan (:162). The CodeReuse/ActiveRecord and Database/AvoidUsingPluckWithoutLimit disables are justified inline on the ground that the same unique index bounds the result to one row per (namespace, node) pair.

Specs — ee/spec/services/search/zoekt/planning_service_spec.rb, with Search::Zoekt::Index.global_buffer_factor stubbed to 1.0 so the sizes are raw repository sizes:

• :606-647 — a single replica needing three indices (Bug A). Nodes of 10/6/5 GB, projects of 7/5/2 GB in packing order; asserts three indices on three distinct nodes and empty errors.
• :649-697 — a second replica for a namespace that already has an index (Bug B), deliberately on the incremental path, with occupied_node the emptiest node so the planner would pick it again.
• :700-726 — the batched lookup: one node-id query for two namespaces, none for an empty batch.

Database: queries and plans

The batched lookup replaces a per-namespace one: a planning run covers up to 128 namespaces (SelectionService's max_batch_size), so it issues one query rather than 128. Both shapes are served as an index-only scan by the same unique index u_zoekt_indices_zoekt_enabled_namespace_id_and_zoekt_node_id, which also bounds the row count to one per (namespace, node) pair. 128 passes cost ~81 ms of planning time and ~1,280 buffers, against ~1.4 ms and 514 buffers for the batched form. Both queries and both EXPLAIN (ANALYZE, BUFFERS) plans: $6053835

The figures come from a synthetic 400,000-row table, not a production clone — they establish the plan shape and the direction of the change, and a ~database reviewer's clone remains the authority on absolute cost.

Verification

Head pipeline for 3bff807553a3 — pipeline 2837310590 — is success. The only failed job is pajamas_adoption, which is allow_failure: true. Both predictive child pipelines succeeded (2837465090, 2837463704).

ee/spec/services/search/zoekt/planning_service_spec.rb ran in rspec-ee unit predictive: 39 examples, 0 failures.

What I did not verify

• No local rspec, RuboCop or haml-lint run.
• The red-on-master claim for the new specs is not reproduced here. It was established during authoring; independent evidence means applying the spec file alone to master and running it.
• Bug B is not reproduced end to end, only at the planning layer. Nothing here drives ProvisioningService to an actual PG::UniqueViolation; that failure chain (Index.insert_all! raises, process_replica rescues it as a generic StandardError, the transaction rolls back, update_enabled_namespace stamps last_rollout_failed_at and EnabledNamespace.with_rollout_allowed blocks retries) is read from the code, not observed.
• The production figure for namespace 8338830 (10 indices across 9 nodes) comes from planning data inspected during authoring and was not re-queried.
• No measurement of the storage-packing consequences of the reordering in assign_to_node. The intent is unchanged behaviour when the current index has room, but no before/after comparison of total reserved bytes was run.
• zoekt_default_number_of_replicas > 1 was not exercised, so the claim that raising it makes every provisioned namespace reachable through with_mismatched_replicas is reasoned from the scope definition, not measured.