fix(planetscale): hold the cutover when the operator defers it

[aparajon]

Why this matters

--defer-cutover is a decision the operator keeps for themselves: run the schema change now, swap it later, under supervision. On PlanetScale/Vitess that decision was not reaching the backend. Deploy requests were created with auto-cutover on, and the schema swapped seconds after the deploy went ready — the operator found out afterwards.

Nothing on any surface showed it. The deploy request looked ordinary right up to the moment it cut itself over.

What it does

Sends the setting. auto_cutover is a bool tagged json:"auto_cutover,omitempty" on the API client's create request, so false marshals away and "cutover deferred" is byte-identical to "cutover unspecified" — leaving the backend to apply its default. Deploy request creation now builds the JSON body directly rather than through that struct, so false transmits. auto_delete_branch shares the path and carried the same hazard.

Confirms it took. auto_cutover is settled when the deploy request is created, and no later call can change it — so asking is not the same as knowing. Before deploying a deferred change, the engine reads the setting back off the created deploy request and refuses if the backend holds auto-cutover on, or if the setting cannot be read at all. The deploy has not started at that point, so refusing costs a re-run while proceeding costs the gate. A read that does not answer is read again before it is treated as a refusal, so a moment's lag does not throw away a branch, a DDL apply and a validation run.

Verifies a recovered deploy request too. A driver can crash between creating a deploy request and starting it, and the drive that recovers it cannot know how the request it inherited was created. So the same read-back runs before a recovered request is deployed, and a recorded instant decision is declined there when the cutover is held — the stored decision is only ever narrowed. Resume now reads both defer_cutover and defer_deploy from the request, so a restart no longer loses either choice.

Declines instant DDL while a cutover is deferred. Instant DDL executes the deploy and swaps the schema in one step, leaving no pending cutover to park at — so an eligible change would pass straight through the gate even with the setting correct. The row-copy path parks, trading the speed of an eligible change for the gate that was asked for. The timeline states this, since otherwise an eligible change quietly takes the slow path.

Says so honestly on the way through. An eligible change stays eligible while it is being copied, so progress read the instant flag off eligibility and reported a deferred change as applied instantly while it was still copying rows and holding for the operator — the one thing the deferral exists to prevent them being told. Progress now reports what the deployment ran as.

Around the edges

• Deploy-request creation fails closed when no API base URL is configured, and a database that omits the optional api_url falls back to the public endpoint the SDK used before — the same fallback the inventory path already applies, so a working configuration stays working.
• A non-2xx response from the two endpoints this client calls directly returns a typed error carrying the method, path and status. The API's own refusal is flattened and clamped for the PR comment it can land in; the whole response body goes to the server log instead.
• LocalScale stored the setting a deploy request was created with, and whether the deploy ran instantly, but reported neither — so a read-back against it could not succeed. It now serves both from the deployment object the way the API does, which is what makes this exercisable locally and in e2e.

The timeline also names the cutover ownership a deploy request was created with, phrased as the request rather than the outcome, since that is all that is knowable before the read-back.

Verified against the real API

The read-back was exercised against the real PlanetScale API, not only the emulator: auto_cutover is returned nested under the deployment object, which is where this decodes it. A deferred change deployed with this fix read back false — against a baseline of true on every prior deploy request — then held at the gate for a minute and a half instead of cutting itself over 4.5 seconds after going ready, and swapped only on the operator's command.

This PR was written by Armand's AI agent (Claude Opus 5).

[Copilot]

Pull request overview

Fixes PlanetScale/Vitess deferred-cutover behavior so the operator’s --defer-cutover intent is reliably transmitted, verified, and preserved across resumes—preventing unintended automatic cutovers.

Changes:

• Reworks PlanetScale deploy-request creation to use raw JSON so auto_cutover=false / auto_delete_branch=false are actually sent (not dropped by omitempty).
• Adds a read-back verification step for auto_cutover to fail closed if the backend would auto-cutover (or if the value can’t be confirmed).
• Disables instant DDL when cutover is deferred, and persists defer_cutover across resume; adds unit tests for the new behaviors.

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/psclient/client.go	Sends deploy-request creation via raw HTTP JSON and adds a raw read path to confirm auto_cutover from the backend.
pkg/psclient/client_test.go	Adds unit tests covering serialization (including false values), auth header, error surfacing, and read-back parsing rules.
pkg/engine/planetscale/branch.go	Adds timeline event for deploy-request creation, introduces cutover-held verification, and gates instant DDL when cutover is deferred.
pkg/engine/planetscale/branch_test.go	Tests the new event, cutover-held verification behavior, and instant-DDL gating logic.
pkg/engine/planetscale/apply.go	Integrates new event, instant-DDL gating decision, read-back verification, and persists defer_cutover on resume.

---

💡 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 pull/978, 75c58f2.

Verdict: 9 findings — 2 blocking (crash-resume bypasses the new cutover-hold gate; empty api_url env-config now fails every apply), 4 non-blocking, 3 suggestions. The core mechanism — always-transmit auto_cutover=false plus read-back verification — is correct and fails closed on every uncertainty mode; the gaps are at its edges. CI fully green (32/32); merges cleanly with current main.

Blocking

1. The crash-recovery deploy path bypasses verifyCutoverHeld — the new gate is not invariant under driver restart. Three independent finders converged on this. Fresh-apply order is: create DR → persist DeployRequestID (apply.go#L284) → waitForDeployRequestPending (#L291, can run minutes on large diffs) → verify (#L370). A crash in that window resumes via resumeExistingDeployRequest (#L709), where deployRequestNeedsResumeDeploy (#L987: ready ∧ !meta.DeferredDeploy ∧ never deployed) is true for a deferred-cutover apply, and deployDeployRequest fires at #L915 — req.Options["defer_cutover"] is never read in that function (repo grep: only #L271 and #L763) and no read-back happens. Concrete scenario needing no backend misbehavior: a deferred DR created by the pre-upgrade schemabot (whose omitempty dropped auto_cutover=false, so the backend default applied) crashes before deploy and resumes on the new version — it deploys a DR that cuts itself over, the exact outcome this PR exists to prevent, during the upgrade window where unheld DRs are most likely. By the PR's own doc ("a create request that did not arrive as sent leaves no trace… Reading the setting back is the only way to know", branch.go#L565-L568), the same verify belongs before the deploy at #L915. Fix shape: read defer_cutover from req.Options in resumeExistingDeployRequest and call verifyCutoverHeld before deploying (setting is immutable post-creation, so verifying at resume is exactly as sound as verifying fresh).

2. A Vitess env-config that omits the optional api_url now fails every deploy-request creation — a previously working configuration becomes totally unable to apply. api_url is optional (pkg/api/config.go#L953 yaml:"api_url,omitempty"), the server only propagates it when non-empty (service.go#L571-L572), and local_client.go#L264-L266 passes the empty string into NewPSClientWithBaseURL with no fallback. Pre-PR the SDK defaulted to https://api.planetscale.com, so this config worked; post-PR the new hard-fail at client.go#L181 blocks every apply (deferred or not) for that database. The inventory/Etre path already guards this exact case (connection_assembler.go#L187 falls back to DefaultPlanetScaleAPIURL); the env-config path needs the same one-line fallback (in local_client.go or NewPSClientWithBaseURL itself, matching NewPSClient's hardcoded default at client.go#L90). Degrades to non-blocking if every deployed env demonstrably sets api_url — but nothing in the repo enforces that, and the failure mode is total (though loud and fail-closed, not silent).

Non-blocking

3. The same crash window auto-deploys a deferred-deploy apply (pre-existing, same fix). deployRequestNeedsResumeDeploy trusts meta.DeferredDeploy, which is only persisted at apply.go#L388 — after the crash window of finding 1. A --defer-deploy apply crashing between #L284 and #L388 is deployed on resume without the operator trigger. Not introduced by this PR, but consulting req.Options in resumeExistingDeployRequest (the finding-1 fix) covers both flags at once.

4. The real-PlanetScale contract for deployment.auto_cutover on GET is unproven — CI validates the fake against itself. The SDK models auto_cutover on neither response type (confirmed at planetscale-go v0.155.0), so the hand-decoded read-back (client.go#L224) is the only claim the field exists — and LocalScale was taught to report it in this same PR (commit 75c58f2), so every integration/e2e pass proves LocalScale, not PlanetScale. If the real API omits the field, verifyCutoverHeld fails closed and every --defer-cutover apply refuses to deploy — safe direction, but a total feature outage until observed against the real API. Worth one manual GET against real PlanetScale before relying on the feature.

5. The PR body's "remembers the deferral across a restart" claim has no test (AGENTS.md: "Tests must prove documented behavior"). The new unit tests cover the extracted helpers (verifyCutoverHeld, useInstantDDL, the created-event) but nothing drives resumeApply with defer_cutover=true to prove the resumed drive verifies the hold and declines instant DDL — the exact lines (apply.go#L763, #L781-L787) whose absence was the bug this PR fixes. A refactor could drop them again with CI green.

6. verifyCutoverHeld turns a transiently-missing deployment object into a terminal apply failure with no retry. client.go#L230-L231 correctly refuses when the setting is absent, but the codebase itself anticipates nil deployments on a ready DR (apply.go#L326 warns "deploy request has nil deployment") — and Vitess applies never auto-retry engine errors (shouldRetryEngineError is MySQL-only, local_apply_sequential.go#L570), so one transient blip discards the whole branch/DDL/validation run. Fail-closed is right; a small bounded re-read (like waitForDeployRequestPending's polling) before refusing would keep it fail-closed without the needless re-run.

General suggestions

7. deployRequestCreatedEvent asserts an unverified fact on every apply: "created with the cutover held by SchemaBot" with hardcoded "auto_cutover": "false" (branch.go#L548, #L553) — but the read-back only runs on deferred applies, so on ordinary applies the timeline states as fact exactly what the PR's own comments say cannot be known without verifying. Either phrase it as the requested state ("created with auto-cutover disabled (requested)") or run the cheap GET on all paths; and derive the metadata value from the request rather than a literal.

8. doRawJSON error handling, two related notes: non-2xx returns an untyped fmt.Errorf embedding the raw response body (client.go#L257) — that loses the SDK's *ps.Error typing that isRetryablePSError classification keys on, and the raw body flows through apply.ErrorMessage onto PR-facing markdown (AGENTS.md: never render untrusted error text on PR surfaces — matches existing engine practice, so a sanitize-at-render follow-up rather than this PR). Also ThrottleDeployRequest (client.go#L292-L317) still hand-rolls the identical raw-HTTP/auth pattern doRawJSON was extracted for — third copy; migrating it would make the helper's "the one place these calls are made" comment true.

9. Minor hygiene: (a) the deferred-deploy log attr "instant_eligible", useInstant at apply.go#L381 now reports the decision, not eligibility — the PR split those two meanings (#L336 logs true eligibility three lines up); rename to instant_ddl or log both, else a triager reads "PlanetScale said not eligible" for an eligible-but-declined change. (b) The new t.Cleanup callbacks pass t.Context() (server_deploy_integration_test.go#L545, #L575), already cancelled when cleanup runs — AGENTS.md mandates context.Background() in cleanup; the subtest's cancel is a guaranteed no-op that silently leans on the next test's start-of-test sweep. (Follows the file's pre-existing pattern — a file-wide follow-up fix is reasonable.)

The one thing that could have broken, verified

Replacing the SDK's DeployRequests.Create with a hand-rolled HTTP POST for every PlanetScale deploy-request creation (client.go#L176-L203) — not just deferred ones; one mismatch would break all PlanetScale applies. Proved safe by four independent parity checks against planetscale-go v0.155.0 source: URL (byte-identical to the SDK's path.Join("v1/organizations", org, "databases", db, "deploy-requests")), auth (SDK sends Authorization: tokenName:token, exactly what doRawJSON sets), body (field names match the SDK's JSON tags field-for-field; the only semantic delta — false now actually transmits instead of being dropped by omitempty — is the point of the PR, and the omitempty premise was confirmed in the SDK source), and response (decoded into the same ps.DeployRequest the SDK uses). The green E2E Vitess/K8s-Vitess/LocalScale matrices exercise the raw path end-to-end against a container built from this branch. The two edges that couldn't be proven from the repo are findings 2 and 4.

Verified correct

• verifyCutoverHeld fails closed on all three uncertainty modes — API error, auto_cutover=true, and unreported setting each refuse (branch.go#L573-L580, client.go#L230-L231) — each with a dedicated test; the *bool read-back correctly distinguishes "reported false" from "absent"; and the refusal error text states definitively what happened and what to do (re-run), per the no-ambiguous-logs rule.
• No TOCTOU on the verified paths: auto_cutover is immutable after creation (no SDK/API mutation carries it), and verify runs after DR-ready and before deploy on both the fresh (apply.go#L370) and recreate-resume (#L781) paths — including before the deferDeploy early return, so the later operator-triggered deploy needs no re-check.
• Instant DDL can never fire on the crash-resume path: the only pre-deploy persist (#L284) omits IsInstant, so the resume deploy at #L915 runs with the zero-value false — the deferred-cutover hole (finding 1) cannot be amplified by an instant swap.
• useInstantDDL is nil-safe and complete: guards dr.Deployment == nil, requires eligibility, declines only on deferred cutover (branch.go#L597-L602); both former inline computations were replaced and no third site exists; the decline is logged and surfaced as a timeline event — no silent branch.
• A verify refusal cannot loop into an unverified redeploy: Vitess applies mark terminally Failed on engine error (no auto-retry), and a re-run creates a fresh branch/DR — only crash-recovery reaches the unverified deploy, which is finding 1.
• LocalScale read-back fidelity: get/list handlers nest auto_cutover under deployment exactly where the decoder reads it (handlers_deploy.go#L305, #L345); the integration test round-trips both true and false; LocalScale genuinely simulates backend-initiated cutover when auto_cutover=true, so the e2e deferred test (vitess_test.go#L531-L535, parks at WaitingForCutover) is meaningful.
• The one substantive Copilot comment (LocalScale not reporting auto_cutover) was already addressed by head commit 75c58f2, with the author's confirming reply — not re-raised here.
• Commit scopes match repo precedent (fix(localscale), feat(observability) for the timeline event both have direct priors); merge-tree vs current main is clean and the four main commits since the merge-base touch disjoint files (one is semantically complementary — it renders the waiting-for-cutover state this PR makes reachable).
• go build ./... and go test -race ./pkg/psclient/ ./pkg/engine/planetscale/ pass locally at head; error wrapping carries org/database/DR-number identifiers on every new path.

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

[aparajon]

🤖 Replying on Armand's behalf, from his agent. Thank you — this was a genuinely useful pass, and the crash-resume finding was the real one. All nine are addressed on the branch; CI is green at 7d9c2ccf.

1 + 3 — crash-resume bypassed the gate (blocking). Fixed in 49aa1de. resumeExistingDeployRequest now reads both defer_cutover and defer_deploy off req.Options and runs verifyCutoverHeld before deploying a recovered request, and deployRequestNeedsResumeDeploy takes the request's defer_deploy alongside meta.DeferredDeploy, so the window before the metadata flag is persisted no longer auto-deploys a deferred apply. Your reasoning for why verifying at resume is sound — the setting is immutable after creation — is what the comment there now says.

2 — an env config without api_url failed every apply (blocking). Fixed in 4ac37d9: the env-config path falls back to inventory.DefaultPlanetScaleAPIURL, the same guard the inventory path already had.

4 — the real-API contract. Settled live rather than in CI, and now written into the PR body: auto_cutover is returned nested under deployment, which is where this decodes it. A deferred change created with this fix read back false — against a baseline of true on every prior deploy request — then held at the gate for about ninety seconds instead of cutting itself over four and a half seconds after going ready, and swapped only on the operator's command.

5 — the resume claim had no test. 49aa1de adds a resume test that drives the recovered deploy both ways: cutover held and it deploys, auto-cutover on and it refuses with the deploy never started. The instant half you asked for is in 8afc90a, below.

6 — a transiently missing deployment became a terminal failure. Fixed in de68cf9: verifyCutoverHeld re-reads before refusing (three attempts, two seconds apart), and the client now returns distinct sentinels for "no deployment" and "deployment without the setting" so the two are separable in logs. Auto-cutover reported on is an answer and still stops the deploy immediately — only the non-answers are retried.

7 — the created event asserted an unverified fact. Fixed in b1cdee6: the message and the metadata key (requested_auto_cutover) now both say the request rather than the outcome, and the doc explains that the created deploy request echoes back no setting, so this is all that is knowable before the read-back. The value stays a literal because createDeployRequest is the sole caller and always requests a held cutover; what changed is that the name now says which of the two it is.

8 — doRawJSON error handling. Both halves in e3710da. Non-2xx now returns a typed APIError carrying method, path and status; only the API's own message field is rendered onward, flattened and clamped for the markdown it can land in, and the whole body goes to the server log instead. ThrottleDeployRequest is migrated onto the helper, so its "the one place these calls are made" comment is finally true.

9 — hygiene. (a) ea74b03 renames the attr to use_instant and logs true eligibility separately. (b) 9436170 gives the two new cleanup callbacks their own lifetime; the rest of the file's pre-existing pattern is left for the file-wide follow-up you suggested.

Two more turned up while acting on this, both on the same invariant. Progress recovered the instant flag from InstantDDLEligible, but an eligible change whose cutover is deferred is deployed with a row copy and stays eligible the whole time — so the comment and the CLI reported it as applied instantly while it was still copying and holding at the gate, which is the one thing the deferral exists to prevent an operator being told. 3aff6fa reads what the deployment ran as instead, and 0f75a53 teaches LocalScale to report the two separately so the read is exercisable. And on the resume path, 8afc90a declines a recorded instant decision when the cutover is held: instant swaps as the deploy runs, so taking the recovered decision would leave the operator a gate with the swap already behind it. The stored decision is only ever narrowed there.

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