fix(operator): choose the drive mode from the generation manifest, not the attached row count

[aparajon]

Why this matters

A deployment-keyed apply receives its operations one dispatch at a time, and the first dispatch records the generation manifest — every operation key the dispatcher will send. The claim loop, however, chose the drive mode from the attached row count alone. A keyed apply with one attached operation looked single-operation, took the legacy parent-lease drive, and that drive's finalizer terminalized the parent apply the moment its own work settled. Every later sibling dispatch was then refused against the terminal apply, failing the rest of the generation one retry at a time. A generation whose first operation is a sub-second CREATE TABLE loses this race deterministically: the whole fan-out degrades to one table per apply attempt.

Before — the drive mode races the fan-out; the first drive closes the apply:

dispatcher                                data plane
──────────                                ──────────
op#1 ──────────────────────────────────►  apply created, manifest = N keys
                                          claim: len(ops) == 1 → parent-lease drive
                                          op#1 completes → finalizer stamps
                                          parent COMPLETED   ← manifest ignored
op#2..#N (seconds later) ──────────────►  refused: apply is terminal

After — the manifest picks the drive mode; the projection owns the verdict:

dispatcher                                data plane
──────────                                ──────────
op#1 ──────────────────────────────────►  apply created, manifest = N keys
                                          claim: manifest expects unattached keys
                                          → operation-lease drive; projection CAS
                                            holds parent RUNNING (keys missing)
op#2..#N ──────────────────────────────►  attach ✓ → drive ✓ ...
                                          last declared key settles
                                          → projection derives COMPLETED

What it does

• The claim loop's drive-mode decision now consults the generation manifest via Apply.MissingExpectedOperationKeys: an apply whose manifest declares operation keys with no attached row drives under the operation lease only, so the parent applies row is moved solely by the operator's projection CAS — which already holds the whole-generation verdict until every declared key has attached and finished. Applies without a manifest, fully attached manifests, and single-key retry dispatches keep the legacy parent-lease drive byte-for-byte.
• The stranded-parent repair arm now recognizes the manifest hold: a keyed apply whose attached operations have all settled while the manifest still expects siblings matches the repair claim once its heartbeat goes stale, and the arm previously warned that the target was blocked and consumed the driver's tick. It now identifies the hold from the projection result, logs it as a healthy rollout awaiting dispatches, and releases the tick to claim real operation work.
• The projection's active-applies gauge ownership follows the drive mode: operation-lease-only drives suppress the parent-level gauge, so the projection that terminalizes the parent releases it for those applies — including a keyed apply whose lone attached operation fails the generation permanently. A named predicate (projectionOwnsActiveAppliesGauge) replaces the attached-row-count check.
• An integration test drives the race end to end: the lone attached operation completes, the apply stays open, a late sibling attaches to the still-active apply, and the apply reaches its whole-generation verdict with exactly one terminal summary. Each drive also probes that a direct parent applies write is refused under the operation-only lease, so the test fails on the real harm — a prematurely terminal parent and a refused sibling attach — if the drive-mode decision regresses.

How it moves us toward the northstar

Large declarative schema changes fan out as one keyed generation per deployment; the generation's outcome must be decided by what the dispatcher declared, not by which operation happened to finish first. This makes the manifest the drive-mode authority end to end — creation refuses undeclared keys, the projection gates the verdict, and now the claim path can no longer bypass either.

Opened by Claude (Fable 5).

[Copilot]

Pull request overview

This PR fixes an operator race for deployment-keyed applies by choosing the drive mode based on the apply’s generation manifest (declared expected operation keys), rather than the currently attached apply_operation row count, ensuring the parent apply cannot be terminalized while sibling operations are still expected to arrive.

Changes:

• Add Apply.ManifestDeclaresUnattachedWork to express “manifest expects siblings that haven’t attached yet.”
• Update the operator claim/drive path to consult the parent apply’s manifest before selecting the legacy parent-lease vs operation-lease drive mode.
• Add an integration test that reproduces the “first op finishes before siblings attach” race and verifies the apply remains open until all manifest keys attach and finish.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/storage/types.go	Adds a named predicate for detecting manifest-declared but unattached work.
pkg/storage/types_test.go	Adds unit coverage for the new manifest predicate behavior.
pkg/api/operator.go	Updates drive-mode selection to consult the generation manifest before choosing the legacy path.
pkg/api/operator_manifest_drive_integration_test.go	Adds an end-to-end integration test exercising late sibling attachment and whole-generation completion behavior.
pkg/api/handlers_test.go	Fixes the fake apply store to materialize the returned apply ID so reads by ID can succeed.

---

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

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1101, ac9a5ea.

Verdict: 8 findings — 0 blocking, 6 non-blocking, 2 suggestions. The drive-mode truth table is sound and the no-manifest legacy path is provably unregressed; the weak spots are the PR body's claims and the observability around the new hold.

Non-blocking

1. The PR body claims a named predicate that does not exist. "Adds Apply.ManifestDeclaresUnattachedWork, the named predicate for 'siblings are still on their way'" — the identifier appears nowhere in the head; operator.go:778 calls the pre-existing apply.MissingExpectedOperationKeys(ops) inline instead. Either add the helper (AGENTS.md § Name compound predicates is presumably why it was intended) or drop the bullet — as written the body describes a diff that wasn't pushed.

2. The new test proves a mechanism proxy, not the harm the PR claims to fix. Reverting the whole operator.go hunk leaves five of the test's six assertions passing — apply still RUNNING, CompletedAt nil, no terminal summary, sibling still attaches, apply still completes — and fails only assert.Zero(recoveredCount()) at line 67. Cause: the matrix fake never writes the parent applies row unless probeParentWrite is set, and line 48 doesn't set it — adding that one field on top of the revert reproduces the real harm verbatim (expected: "running" actual: "completed", then attach operation commerce/-80/orders … apply is not active). Coverage of the changed path is gated, which is why this isn't blocking, but PR claim 4 and AGENTS.md § Tests must prove documented behavior need the one-line change.

3. Every healthy keyed rollout now trips the stranded-parent repair arm once a minute. With the lone operation completed and the sibling not yet dispatched, the parent dwells RUNNING with all attached rows terminal and a stale updated_at — exactly FindNextApplyForOperationProjection's predicate (applies.go:1753-1767). The projection no-ops, so operator.go:1426 emits WARN "its target stays blocked until … an operator reconciles it" about a healthy in-flight rollout and consumes the driver's tick. Both halves pre-exist on main and were reachable for ≥2 attached ops; this PR makes it the standard shape — teach the projection arm about the manifest hold.

4. The active_applies gauge leaks on the newly routed path. operator.go:2507 gates the projection's -1 on len(ops) > 1, and the operation-lease-only drive suppresses the engine's own -1 (suppressParentApplyWrites returns before local_apply_sequential.go:709) — so a single-attached keyed apply that fails permanently (failure isn't manifest-gated) terminalizes with nobody decrementing. Reachable pre-PR only via the task-less branch, so this widens it, but the contract comment two lines above ("Single-operation applies keep decrementing in their direct drive") is now false and would mislead a maintainer.

5. Nothing bounds the manifest hold. If a declared sibling never attaches, the projection rewrites every verdict back to RUNNING forever; the operator's only expiry pass is ExpireRetryable (failed_retryable only), the stranded reaper works the other direction (parent settled → children), and checkNoActiveApplyForTargets then refuses every future apply for that target until an operator issues a stop. Pre-existing on main and arguably the right trade (the pre-PR alternative was a silently wrong terminal apply), and RecordApplyManifestHold makes it observable — but a keyed generation now has no automated recovery from a lost dispatch.

6. Banned comment form. operator_manifest_drive_integration_test.go:25 closes with "Without the manifest-aware mode decision the first drive would terminalize the parent…" — the exact shape AGENTS.md bans twice (§ No bug references in code: "// without this fix, X breaks"; § Explain scenario tests: "describe the behavior as if it always existed"). It's the only instance in pkg/; the preceding sentences already state the invariant positively, so delete it. The production comment at operator.go:757-762 is fine — it explains why the manifest is consulted without naming a fix.

General suggestions

7. The parent apply is loaded, used once, and thrown away. operator.go:763's Get feeds only the manifest check; the manifest branch then calls driveClaimedMultiOperation, which re-reads the identical row at operator.go:1037, and the legacy branch re-acquires via ClaimApplyByID. Threading the snapshot down is not the fix (line 1037's read must be fresh — two statements later it gates the terminal-parent check), so this is one extra PK SELECT per claimed single-op drive ahead of a full engine drive: real but negligible.

8. The new Info log can't answer "which shard hasn't dispatched?". operator.go:785 logs missing_operation_count where the pre-existing gate at operator.go:2390 logs missing_operation_keys; the head commit shows the swap was deliberate (a large generation would bloat the entry), so this is a preference, not a defect. It's also the only op-scoped log in the file built on apply.LogAttrs() that doesn't hand-add apply_operation_id — harmless, since apply_id + operation_deployment + operation_key is the row's full natural key.

The one thing that could have broken, verified

Whether the new branch regresses the drive mode for ordinary non-keyed applies — the legacy parent-lease path carries the engine's parent writes, the per-driver terminal observer and the check-gate summary, so silently diverting it would break every single-deployment apply. Proved safe by enumeration plus mutation: the legacy path is reachable only when len(ops) == 1 and the manifest is empty or is exactly that op's key, and mutating the predicate to always-true fails the pre-existing TestOperator_UnclaimableParentReleasesOperationLease with four assertions (it pins the legacy drive through parent-claim-refusal and lease-release semantics, not through recoveredCount). There is no cell where siblings are still expected but the legacy drive is taken, and none where a non-keyed apply is diverted.

Verified correct

• TOCTOU between ListByApply and the new Applies().Get errs safe in every interleaving: a sibling attaching between the two reads only adds missing keys, which routes toward the operation lease.
• The manifest is write-once: the only writer is the insert path (sqlstore/applies.go:583), no UPDATE sets ExpectedOperationKeys, so the claim loop can never observe a pre-manifest keyed apply.
• The "attached key not in the manifest" cell is unreachable — AllowsOperationKey refuses undeclared attaches and pkg/tern/local_client.go:2657 refuses a dispatch whose manifest omits its own key.
• No incomplete sweep in pkg/tern: multiOperation = len(siblings) > 1 computes identically pre- and post-PR, and CutoverPolicy is never set for manifest applies, so the barrier/cutover decision is untouched.
• Mutating the branch to fall through (log but don't delegate) or to len(missing) == 0 is caught by the new test — the changed path is gated even if the harm isn't asserted.
• The handlers_test.go one-liner is load-bearing, not incidental: reverting s.apply.ID = applyID fires the new parent-nil branch and fails two unit tests.
• The two new metric reasons reuse the vocabulary already on main (operator.go:1042/1049) rather than adding an instrument, per AGENTS.md § Adding New Metrics.
• Ordering is safe: placing the manifest check before claimedOperationHasTasks changes nothing — the task-less branch already routes to the operation lease.

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

[aparajon]

🤖 Thanks for the thorough review — all eight findings triaged; four landed as code/body fixes in 9b72312 (new commits only, since the PR is approved), and origin/main was merged in rather than rebasing for the same reason.

1. PR body claims a helper that doesn't exist — fixed the body. The check reads Apply.MissingExpectedOperationKeys directly; a ManifestDeclaresUnattachedWork wrapper would just hide the count the drive-mode log wants, so the body now names the real helper instead of resurrecting the wrapper.
2. Test proves a proxy, not the harm — the manifest test now sets probeParentWrite on both drives and asserts both parent-write probes are refused with ErrApplyLeaseLost. Verified by reverting the drive-mode hunk: the test now fails with the real harm — expected: "running" actual: "completed" on the held parent and a refused sibling attach — not just the recovered-count proxy.
3. Healthy keyed rollouts trip the stranded-parent repair arm — the projection result now carries ManifestHeld, set when the derive gated a whole-generation verdict back to running. The repair arm checks it before the WARN: a held rollout logs an Info stating it needs no repair and returns the tick to the operation claim (which is where the awaited dispatches get driven). The claim still refreshes the heartbeat, so a held apply is reconsidered at most once per staleness window. The check has to live after the derive rather than in the claim predicate: missing manifest keys alone don't identify a hold, because a failed generation with missing keys must still be terminalized (failure is deliberately not manifest-gated), and only the derive knows which verdict was gated.
4. active_applies leak on a single-attached keyed failure — the gauge gate is now the named predicate projectionOwnsActiveAppliesGauge (len(ops) > 1 || missing manifest keys), matching the drive-mode decision that suppressed the engine's own decrement, and the contract comment now states the ownership rule. Unit tests cover both the keyed single-op failure (decrements) and the fully attached keyed single-op (left to the direct drive).
5. Nothing bounds the manifest hold — acknowledged, and deliberate: the data plane can't distinguish a delayed dispatch from a dead one, so a data-plane timeout would race real fan-outs. The follow-up PR gives the control plane the recovery path: when the dispatching apply terminalizes with undispatched manifest keys, it sends the existing durable cancel to the correlated remote apply (via the deployment↔remote-apply correlation that just merged). Until then schemabot.apply_manifest_hold is the watch signal and operator cancel is the manual out.
6. Banned comment form in the test doc — deleted the sentence; the preceding sentences already state the invariant positively.
7. Parent apply loaded and thrown away — agreed it's negligible and agreed threading it forward isn't the fix: driveClaimedMultiOperation must re-read under its own claim anyway, and handing it a pre-claim snapshot would invite exactly the staleness this PR exists to prevent. Left as is.
8. Drive-mode log logs the count, not the keys — deliberate: the missing-key set on a large generation is hundreds of entries and this Info fires per dispatch; the count answers "how far along is the fan-out" while the projection's hold log (Debug) carries the full key set when needed. apply_operation_id is intentionally absent for the same reason as elsewhere — the numeric row ID isn't a triage handle; the log carries operation_key + operation_deployment, which is how operators address an operation.

This comment was written by Claude Code (Fable 5) on Armand's behalf.