refactor(storage): claim applies only through the operation ladder

[aparajon]

Why this matters

The storage layer had two byte-identical six-arm claim predicates: the queue-scan claim and the by-ID claim. Two copies of a safety-critical predicate drift — an arm added to one and not the other silently changes which applies a driver can recover. With drivers claiming queued work exclusively through the operation ladder, the queue-scan copy has no production caller left.

What it does

• Removes ApplyStore.FindNextApply (interface + mysqlstore). Drivers claim queued work through the operation ladder (FindNextApplyOperation and friends) and claim one specific apply with ClaimApplyByID, which carries the same claim predicate — pending with child rows, stale-heartbeat reclaim, retryable within budget, pending start control requests — and the same lease rotation.
• Moves the predicate's test coverage onto ClaimApplyByID: six tests ported in place, two re-pinned directly on the by-ID claim (a failed start request keeps a stopped apply stopped until re-requested; a stale waiting_for_cutover apply is reclaimable while a fresh one stays owned), and five removed where a live equivalent already pins the same predicate arm.
• The tern operator integration helpers now claim each dispatched apply by ID, the way api.Service drivers do.

Northstar: one claim predicate, one lease rotation path, exercised by every driver and every test.

Follows #933, which removed the apply-level claim mode this loader served.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR refactors the storage-layer apply claiming flow to eliminate the now-dead queue-scan claim path and ensure all drivers acquire apply leases exclusively through the operation-claim ladder plus a single by-ID claim predicate (ClaimApplyByID). This reduces the risk of safety-critical predicate drift by consolidating claimability rules and lease rotation into one exercised path.

Changes:

• Removed the apply queue-scan claim API (ApplyStore.FindNextApply) and updated callers to claim applies by ID via ClaimApplyByID.
• Updated operator-like tern integration helpers to resolve dispatched applies from storage and claim them by ID, matching the production driver pattern.
• Moved/trimmed tests to pin the unified claim predicate via ClaimApplyByID and updated integration tests to drive specific queued applies by identifier.

Reviewed changes

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

Show a summary per file

File	Description
pkg/tern/local_taskless_completion_integration_test.go	Switches taskless-apply tests from queue-scan claiming to ClaimApplyByID.
pkg/tern/local_resume_scope_integration_test.go	Updates resume-scope test to claim the specific apply by ID and adjusts explanatory comment.
pkg/tern/local_dispatch_shard_integration_test.go	Starts the test operator with a specific apply identifier so it can claim by ID.
pkg/tern/local_control_volume_integration_test.go	Replaces “drive next queued apply” helper usage with “drive this queued apply” by identifier.
pkg/tern/local_control_taskless_integration_test.go	Updates taskless control-flow tests to drive/claim the specific apply via ClaimApplyByID.
pkg/tern/local_control_reconcile_integration_test.go	Updates reconcile-flow tests to synchronously drive the specific queued apply by identifier.
pkg/tern/local_control_completed_engine_integration_test.go	Updates completed-engine control test to drive/claim the specific apply via ClaimApplyByID.
pkg/tern/local_client_integration_test.go	Adds helpers to resolve dispatched applies and updates operator/drive helpers to claim by ID rather than scanning.
pkg/storage/storage.go	Removes FindNextApply from ApplyStore and updates interface docs to reflect ClaimApplyByID.
pkg/storage/README.md	Updates recovery documentation to reference ClaimApplyByID instead of FindNextApply.
pkg/storage/mysqlstore/claimable_states_test.go	Updates comments describing the pending-claim arm now pinned via ClaimApplyByID.
pkg/storage/mysqlstore/apply_operations.go	Updates comments tying operation-claim behavior to the consolidated apply-claim predicate.
pkg/storage/mysqlstore/apply_operations_test.go	Updates parity/coverage comments to reference ClaimApplyByID and renamed apply-level concurrency test.
pkg/storage/mysqlstore/applies.go	Removes the queue-scan claim implementation and retains a single ClaimApplyByID implementation for apply leases.
pkg/storage/mysqlstore/applies_test.go	Ports/removes queue-scan claim tests and re-anchors predicate coverage on ClaimApplyByID.
pkg/api/config.go	Updates operator driver config documentation to no longer reference FindNextApply directly.

---

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

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for pull/934, c4e6889.

Verdict: 6 findings — none blocking: 3 non-blocking (the recovery README now mis-describes how stale work is discovered, stale claim-concept comments including two in files this PR touches, two small coverage narrowings), 3 suggestions (#916 heads-up, a decorative control request in a new test, the remaining ladder mirror as the next dedup target). The core of the PR — the textual merge of FindNextApply and ClaimApplyByID into one function — verified byte-identical in executable code, and the PR body's test accounting (six ported, two re-pinned, five removed with live equivalents) checks out one-for-one. Safe to merge as-is; everything below can follow up.

Non-blocking

1. The storage README's recovery section now says ClaimApplyByID "claims an apply with a stale heartbeat by selecting it" — but the function cannot select (discover) anything, and the section never names the thing that does. README.md#L58 swapped the new name into FindNextApply's old sentence, but the semantics don't transfer: ClaimApplyByID requires the caller to already hold the ID; post-feat(operator): claim work exclusively at the operation level #933/refactor(storage): claim applies only through the operation ladder #934, discovery of a crashed driver's work happens exclusively through the operation ladder (FindNextApplyOperation), and ClaimApplyByID's sole production caller acquires the parent lease after an operation row is claimed (operator.go#L378). The bullet also describes only the stale-heartbeat arm of a four-family predicate. Failure scenario: an operator triaging a stranded apply reads "Heartbeat-Based Recovery", goes looking for ClaimApplyByID's scan loop, and never finds the actual recovery entry point. Fix: one sentence — the ladder discovers, ClaimApplyByID leases the parent.

2. Stale claim-concept comments, two of them in files this PR itself rewrites. In-scope this PR: local_dispatch_shard_integration_test.go#L23 still says "The operator's whole-apply claim must load those tasks" in the header of a test whose helper this diff converts to by-ID claiming; and claimable_states_test.go#L33 keeps the map value "queue claim path" (interpolated into the assertion failure message) even though the comment two lines above was rewritten to name ClaimApplyByID — a future failure message directs the reader to a path grep can no longer find. Carried over from my feat(operator): claim work exclusively at the operation level #933 review and still unfixed here: operator.go#L1104 (also L1141, L1190) labels applyOperationID == 0 "the legacy whole-apply path" when it is now the live stop-reconciliation drive — after this PR deletes the last queue-scan claimer, "legacy" is strictly more wrong and invites deleting a live branch; and e2e/k8s/operator_fixtures_test.go#L63 still justifies a loose assertion by appeal to "apply-level claiming". One sweep commit closes all five.

3. Two deliberate coverage narrowings worth knowing about, both mitigated at the predicate-arm level. (a) The deleted setup-phase test looped over all five PlanetScale setup states; the surviving ClaimApplyByIDClaimsStaleSetupPhase pins only applying_branch_changes (L2429) — the revert-phase twin kept its two-state loop, so the asymmetry looks accidental. All five states flow through the single state IN (...) arm and TestClaimableApplyStates_CoverEveryRegisteredState pins list membership, so the residual risk is small. (b) The deleted stopped-start test's tail — a repeat claim is blocked immediately after a successful stopped→resuming claim — did not survive: both surviving stopped-start tests end on the Resuming assertion (applies_test.go#L3036 and L2993). Today no predicate arm can match a fresh resuming row, and the operation-ladder concurrency test covers the production path, but the just-resumed+still-pending-start row shape is no longer reproduced by any apply-level test. A two-line again re-claim at the end of the re-request test restores it cheaply.

General suggestions

4. Heads-up for the fix(operator): settle applies left behind their own settled operations #916 author (again): its diff adds new doc-comment references to FindNextApply. fix(operator): settle applies left behind their own settled operations #916's FindNextApplyForOperationProjection describes its lease semantics by reference to the function this PR deletes ("FindNextApply re-leases when a heartbeat goes stale…", "rotating the lease onto it like FindNextApply"). No compile break in either merge order — but whichever lands second reintroduces references to a symbol that no longer exists; the right anchor is now ClaimApplyByID. (This is the second feat(operator): claim work exclusively at the operation level #933/refactor(storage): claim applies only through the operation ladder #934-vs-fix(operator): settle applies left behind their own settled operations #916 staleness interaction; a rebase of fix(operator): settle applies left behind their own settled operations #916 would resolve both.)

5. In the new ClaimApplyByIDClaimsStaleWaitingForCutover test, the inserted pending cutover control request is decorative — no predicate arm keys on cutover requests (the only control-request arms are the three ControlOperationStart ones; the claim succeeds via the stale-active arm since waiting_for_cutover is a claimable state). The doc comment ("parked at waiting_for_cutover with a pending cutover request … becomes reclaimable") reads as if the request participates in the mechanism (applies_test.go#L2513). The deleted predecessor had the same decoration, so this is parity — but the re-pin was the moment to either drop the insert or say it's scene-setting. Also cosmetic edit residue from the in-place word swaps: mid-sentence stub lines at applies.go#L1135 ("claimed. It is a") and storage.go#L429.

6. The "one claim predicate" northstar is correctly worded as a northstar — the next dedup target is the operation ladder's hand-mirrored parent gates. This PR reduces full six-arm copies from two to one, but FindNextApplyOperation still mirrors four parent-gate arms by hand (the comments this PR re-anchors to "Mirrors ApplyStore.ClaimApplyByID"), and FindNextApplyForStopReconciliation is a third lease-rotating claimer. The drift risk the PR body motivates now lives entirely in that mirror; worth tracking as the follow-up.

The one thing that could have broken, verified

The diff textually splices two functions into one: it deletes FindNextApply's signature and queue-scan SQL, deletes the old ClaimApplyByID's header and prologue, and leaves a single survivor assembled from parts of both — exactly the shape where an argument-order swap or a lost placeholder silently changes which applies a driver can claim. Verified two ways. First, the survivor against the pre-PR ClaimApplyByID (merge-base f49ce3fd, old lines 1514–1611 vs head applies.go#L1398-1504): every executable line is byte-identical — queryArgs := []any{applyID, state.Apply.Pending} (L1412) with applyID matching WHERE a.id = ? as the first placeholder (L1438), same six arms in the same order, same fmt.Sprintf substitution order, same READ COMMITTED isolation, same persistApplyClaim outcome branches and commit/error wrapping; the only additions are two comment blocks inherited from the deleted function. Second, the PR body's premise: at merge-base the two WHERE predicates were mechanically diffed and are byte-identical modulo the a.id = ? scope and the queue-scan's ORDER BY a.created_at (meaningless for a single-row claim) — so nothing semantic was lost by keeping the by-ID copy. go build ./..., go vet ./..., and vet under the integration and e2e tags all pass in the worktree.

Verified correct

• Every one of the six predicate arms has a direct by-ID test at head: pending+child-rows (applies_test.go:2248/2669/2834), stale-active (:2280, :2423, :2463, :2513), retryable positive with attempt-charge/error-clear/repeat-nil (:2331), retryable freshness + budget refusals (:2368), pending+start (:2855), stopped+start (:2946/:2993/:3036), waiting_for_deploy+start with the full stale-reclaim tail (:2887).
• The five removed tests each have a verified pre-existing equal-or-stronger by-ID equivalent, and the six in-place ports preserve every assertion (the pending-start port keeps caller-sees-pending / persisted-running / repeat-nil; the waiting-for-deploy port keeps the recovery-owner reclaim tail). The new failed-start test can only pass through the stopped+pending-start arm — stopped is not in claimableApplyStates(), so the stale-active arm cannot produce its claim.
• The tern helper redesign is safe: all nine relocated startTestOperator/driveQueuedApply call sites run after Apply() returned an ApplyId, LocalClient.Apply persists the row synchronously before responding (local_client.go:2124), every test dispatches exactly one apply, and the idempotent-dispatch test asserts re-dispatches return the same ApplyId. The renamed concurrency test still proves the SKIP LOCKED single-winner contract (16 drivers, one connection each, one claim).
• Oldest-first queue fairness survives where claiming actually happens — the operation ladder's claim queries all carry ORDER BY created_at, id (apply_operations.go#L135 et al.); the deleted ORDER BY served a query with no callers.
• Removal is complete: repo-wide zero remaining references to FindNextApply (excluding the distinct FindNextApplyOperation/FindNextApplyForStopReconciliation), no test double declares a dead method, and no shared helper (claimableApplyStates, persistApplyClaim, scanApplyInto) was orphaned.
• The stranded-legacy-rows exposure flagged in my feat(operator): claim work exclusively at the operation level #933 review is unchanged by this PR: at merge-base FindNextApply already had zero production callers, so this deletion removes an unreachable escape hatch, not a live recovery path.
• Merge-base is the current main tip; of 38 open PRs only fix(operator): settle applies left behind their own settled operations #916 interacts, and only at comment level (finding 4). Commit scopes and the no-negative-regression-tests convention match AGENTS.md; CI fully green (32/32) on c4e6889.

This review was generated by Claude Code (claude-fable-5).

[aparajon]

🤖 Thanks for the thorough pass — the byte-identical splice verification and the parity cross-check are exactly the assurances this PR needed. Addressed in ac9fcaa (new commit, post-approval):

1. README recovery section: rewritten to separate discovery from leasing — the operation ladder (FindNextApplyOperation) scans for claimable work including stale heartbeats, and ClaimApplyByID leases the parent the caller already identified. All four predicate families are now named instead of just the stale-heartbeat arm.
2. Stale comments: all five swept — the dispatch-shard test header, the "queue claim path" map value (now "ClaimApplyByID pending arm" so the failure message points at a greppable path), the three operator.go "legacy" labels (now correctly the stop-reconciliation drive), and the e2e fixtures comment.
3. Coverage narrowings: (a) the setup-phase test now loops all five PlanetScale setup states as subtests, matching the revert-phase twin — the asymmetry was accidental; (b) the failed-start re-request test now ends with a repeat claim asserting the just-resumed row can't be stolen while its start request is still pending.
4. fix(operator): settle applies left behind their own settled operations #916: agreed — that's for its rebase, which will re-anchor those doc comments to ClaimApplyByID.
5. Decorative cutover request: dropped from the test entirely, and the doc comment now says the claim comes through the stale-active arm (waiting_for_cutover is a claimable dwell state). Both mid-sentence stub lines rewrapped.
6. Ladder mirror: agreed that's where the drift risk now lives — FindNextApplyOperation's hand-mirrored parent gates (and FindNextApplyForStopReconciliation as a third lease-rotating claimer) are now tracked as the next dedup target in the team todos.

This reply was drafted by Claude Code (claude-fable-5) on Armand's behalf.