fix: retire superseded Trino candidates before readiness

[benben]

Summary

• An obsolete candidate that never becomes ready remains in CREATING, occupying the pool's only creation slot even after a corrected release or blueprint is published. Existing supersession checks only cover PREPARING.
• Check authoritative desired release/blueprint after Gateway registration read-back and before waiting for readiness. Reuse the existing fenced candidate-failure and resource-absence cleanup path; preserve immutable snapshots and admission replay.
• Track registration attempts for the current authority term. A timed-out request can still commit after a member lookup returns 404; only durable registration adoption or a confirmed higher Gateway fence clears that uncertainty.

Safety and scope

• No timeout-based deletion, database migration, Gateway API change, or live-cluster mutation. No internal deployment or customer details included.
• Registered members still require Gateway retirement authorization. Unknown reads and same-term registration outcomes do not authorize deletion.
• An unresolved registration that never commits can remain safely blocked until a genuine leadership handoff/restart establishes a higher fence. This is not a universal self-healing claim.
• Deploy the new control-plane image to all replicas, then let the normal operator reconcile. No direct database edits or manual pod deletion are required for a never-registered, superseded candidate.

Validation

• Reproduced red tests for unready coordinators, unready workers, and configuration-only supersession; these now pass.
• 232 top-level tests pass across the selected Trino lifecycle, registration replay, Gateway configuration, authority, and supersession suites. Includes delayed registration commit, failed/new-term fencing, capacity retention, and immutable snapshots.
• just lint: 0 issues. Kubernetes-tagged diff-scoped lint: 0 issues. git diff --check: clean.
• An initial broader test filter also selected an unrelated admin PostgreSQL integration test, which could not run because Docker Compose is unavailable locally. The final pure-test selection excludes it.
• Extra whole-repository Kubernetes-tagged lint reports 70 findings in unrelated files; this PR does not change those files.
• Independent adversarial source review completed. No live deployment test or CI watching performed.

[github-actions]

Test Impact Plan

Deterministic summary of how this PR changes tests, CI runners, and coverage-risk signals.

Summary

Area	Added	Changed	Deleted
Test files	0	1	0
E2E/journey files	0	0	0
Workflow files	0	0	0

Signals

• Test cases: +4 / -0
• Assertions: +24 / -1
• Skips or known failures added: 0
• Workflow continue-on-error added: 0
• Workflow path filters added: 0
• Test commands removed from justfile: 0
• E2E/journey retry lines added: 0

Coverage risk: neutral or increased

No coverage-reduction warnings detected.

[bill-ph]

What I checked

I read the PR description and the full diff, then traced the surrounding lifecycle in repo/:

• The new candidateSuperseded extraction against the old inline PREPARING check — same comparison (ReleaseID + parsed-snapshot digest vs o.pool.DesiredReleaseID / DesiredBlueprintDigest), so the refactor is behaviour-preserving, and both fields are always populated by trino_pool_config.go:234 / UpsertTrinoPoolSpec earlier in the same tick, so the "complete desired pool specification" guard should not fire in practice.
• The new CREATING supersession branch's placement: after adoptRegisteredMember (so a committed-but-lost registration is still adopted first) and before the readiness gate (which is the actual fix). CREATING -> FAILED_PREPARING is a legal transition (trinopool/phase.go:42), and FAILED_PREPARING still OccupiesCapacity, so the slot is only released after removeFailedCandidateResources confirms absence — the new test asserts exactly that ordering.
• cleanupFailedCandidate with GatewayIncarnation == "" short-circuits to resource removal without a GetMember, which is precisely why the late-commit race needs the new guard. The guard is sound for the same-term case, and the cross-term case is covered by the fence: AcquireTrinoPoolAuthority strictly increments (configstore/trino_pool.go:189), ensureAuthority acquires once per term, and configureGatewayPool only clears registrationAttempts after configuredPoolMatches confirms the Gateway echoed ControllerEpoch == lease.Epoch — so the clear genuinely implies the older RegisterMember is fenced. I also checked that the clear sits after the errTrinoPoolBackoff return but that the first tick of any term always has attempt.payload == desired, so the clear is never skipped on a term boundary.
• Error isolation: the new blocking error returns progressed=false and is collected by progressInstances, so it does not stop other instances or applyPlan.
• Instance creation always snapshots the current o.config.Blueprint, so repair candidates can't be born superseded and loop.

No P0s. The change looks correct and the tests are unusually thorough for this area (delayed commit, failed fencing, capacity retention, snapshot immutability).

P1

controlplane/trino_pool_progress.go:180 — the "unresolved registration" block has no automatic escape and no distinguishable signal.

Once registrationAttempts[instanceID] is set and the candidate is superseded, registerWhenReady returns early every tick: it never retries RegisterMember, and the entry can only be cleared by adoption or by an epoch change. Run only ends a term on a fence refusal, so a stable leader holds this state indefinitely — the docs acknowledge this ("can therefore require a control-plane restart or normal leadership handoff"), but nothing in the code initiates that handoff. Meanwhile the candidate keeps occupying the creation slot, so a DesiredInstances: 1 pool whose candidate never registered can sit at zero serving members until a human restarts the control plane.

The observable signal is also weak: the plain errors.New propagates through progressInstances -> reconcileOnce -> slog.Warn("Trino pool reconcile failed") plus poolObservation().failure() once per 5s tick forever, indistinguishable from a transient instance failure.

The escape already exists in the design — a new term bumps the epoch, which is exactly the confirmed higher fence this guard waits for. Ending the term after the block has persisted for some number of ticks would self-heal via the normal janitor path (the "never re-acquire" warning in dropAuthority is about fence ping-pong with a valid other leader, which is not this case). At minimum, a dedicated metric/log distinct from generic reconcile failure would make the wedge actionable.

P2

controlplane/trino_pool_progress.go:179 — a definitively refused registration is treated as unresolved.

The entry is recorded before the call and only removed on a durable advance, so an application-level refusal that provably did not commit (a 4xx from the Gateway) wedges the candidate just like a timeout. This file's sibling logic already makes exactly this distinction for the pool config: configureRequestRejected (trino_pool_operator.go:530) classifies POOL_VALIDATION / TENANT_IDENTITY_UNVERIFIED / POOL_APIMODE as "this request did not commit" and treats proxy errors and timeouts as unknown. Applying the same classification to the RegisterMember error would clear the attempt for the knowable cases and shrink the P1 wedge to genuine unknowns.

controlplane/trino_pool_progress.go:85 — PENDING is still not covered.

The fix now covers CREATING and PREPARING, but an instance stuck in PENDING (repeated createResources failures — quota, admission webhook, image pull rejection) holds the same creation slot, and a corrected release or blueprint will not retire it. That is the same bug one phase earlier, and unlike CREATING it has no Gateway-registration ambiguity at all, so the check would be unconditional and safe there. It would also avoid instantiating a full coordinator + worker set for a candidate that the very next tick fails.

controlplane/trino_pool_superseded_test.go:97 — split type declaration.

unreadableCreatingMemberGateway is declared here but its only method lives at line 185, with delayedCreatingRegistrationGateway and a whole test function in between. Moving the declaration next to GetMember (or vice versa) would match how delayedCreatingRegistrationGateway is laid out immediately below.

Inline comments

• controlplane/trino_pool_progress.go:180 — P1: This block has no automatic escape. Once the attempt is recorded and the candidate is superseded, registerWhenReady returns here on every tick and never retries RegisterMember; the entry clears only on adoption or an epoch change, and Run only ends a term on a fence refusal. So a stable leader holds this state indefinitely while the candidate keeps occupying the creation slot — a DesiredInstances: 1 pool can sit at zero serving members until someone restarts the control plane. The docs acknowledge the wait, but nothing initiates the handoff that ends it.

The escape already exists in the design: a new term bumps the epoch, which is precisely the confirmed higher fence this guard is waiting for. Voluntarily ending the term after the block has persisted for N ticks would self-heal through the normal janitor path (the "never re-acquire" warning on dropAuthority is about ping-pong with a valid competing leader, which isn't this case).

Separately, the plain errors.New surfaces as slog.Warn("Trino pool reconcile failed") + poolObservation().failure() once per tick forever, indistinguishable from a transient instance failure. A dedicated signal would make the wedge actionable.

• controlplane/trino_pool_progress.go:179 — P2: The attempt is recorded before the call and cleared only on a durable advance, so a registration the Gateway definitively refused (an application-level 4xx that provably did not commit) wedges the candidate exactly like a timeout.

This file's sibling logic already draws that line for pool config: configureRequestRejected (trino_pool_operator.go:530) treats POOL_VALIDATION / TENANT_IDENTITY_UNVERIFIED / POOL_APIMODE as "did not commit" and leaves proxy errors and timeouts unknown. Classifying the RegisterMember error the same way would clear the attempt for the knowable cases and leave the block for genuine unknowns only.

• controlplane/trino_pool_progress.go:85 — P2: With CREATING now covered, PENDING is the remaining gap. An instance whose createResources keeps failing (quota, admission webhook, image pull rejection) holds the same creation slot, and a corrected release or blueprint won't retire it — the same bug one phase earlier. Unlike CREATING there's no Gateway-registration ambiguity in PENDING, so the check could be unconditional there, and it would also avoid instantiating a full coordinator + worker set for a candidate the next tick immediately fails.
• controlplane/trino_pool_superseded_test.go:97 — P2 (nit): This declaration is ~90 lines from its only method (GetMember, line 185), with delayedCreatingRegistrationGateway and a full test function in between. Worth moving the two together, matching how delayedCreatingRegistrationGateway is laid out just below.

— Robo Bill v2 (opus, high reasoning)