Fix internal/corrosion/vms.go's scanVMRow (used by ListVMs/ListVMsPag...

[colonelpanik]

Goal

Fix internal/corrosion/vms.go's scanVMRow (used by ListVMs/ListVMsPage) so it actually populates VMRecord.PendingActionID from the pending_action_id column, and extend TestAnyStrandedPending in internal/grpcapi/ha_health_test.go to cover the currently-missing case: a state=pending VM that DOES carry a pending_action_id must NOT be reported as stranded.

Plan

Plan: populate VMRecord.PendingActionID in the ListVMs projection

fix(health): populate PendingActionID in the ListVMs projection

The defect

internal/corrosion/vms.go:277 scanVMRow never sets PendingActionID, and neither
ListVMs (vms.go:248) nor ListVMsPage (vms.go:305) selects the column. Row.String
on an absent column returns "" (internal/corrosion/client.go:1137 → get returns nil →
""), so every VM read through the list path carries PendingActionID == "".

The only production reader of that field off the list path is
internal/grpcapi/ha_health.go:266:

for _, vm := range vms {
    if vm.State == "pending" && vm.PendingActionID == "" {
        return true
    }
}

So today anyStrandedPending degenerates to "any state=pending VM assigned to this host",
and evaluateHADegraded (ha_health.go:250) raises legacy_pending_stranded for it whenever
split_brain_gate_v1 is enforced.

This is a live false positive, not only a test gap. A normal proof-gated
reschedule/failover parks the VM at host_name = <dest>, state = 'pending', pending_action_id = <proof id> in one batch (corrosion.WriteVMRescheduleProof,
internal/corrosion/action_proofs.go:121). From that write until the destination's
reconciler completes the start, the destination node's HA monitor
(RunHAHealthMonitor, ha_health.go:94-105) sets
litevirt_ha_degraded{reason="legacy_pending_stranded"} and publishes an ha.degraded
event, then an ha.recovered when the start lands. The reason string is documented as a
durable operator-repair condition ("refused proof_missing forever",
ha_health.go:20, ha_health.go:244-249); the missing column turns it into routine
transient noise on every failover, and simultaneously makes the genuine stranded-legacy-row
signal indistinguishable from that noise.

internal/health/reconciler.go:736 reads PendingActionID too, but from GetVM, whose
projection is already correct (vms.go:344) — so it is unaffected either way.

Changes

1. internal/corrosion/vms.go — ListVMs projection

Add the column using the exact GetVM convention (vms.go:344):

	sql := `SELECT name, stack_name, host_name, spec, state, state_detail,
		cpu_actual, mem_actual, COALESCE(project, '_default') AS project,
		COALESCE(is_template, 0) AS is_template,
		COALESCE(pending_action_id, '') AS pending_action_id,
		COALESCE(vm_owner_epoch, 0) AS vm_owner_epoch, created_at, updated_at
		FROM vms WHERE deleted_at IS NULL`

COALESCE is redundant against the schema (pending_action_id TEXT NOT NULL DEFAULT '',
schema.go:1103, and the v38 ALTER at schema.go:2428 is likewise NOT NULL DEFAULT ''),
but it matches GetVM and costs nothing. Consistency here is the point: the two projections
should be diffable by eye.

2. internal/corrosion/vms.go — ListVMsPage projection

Same line added to ListVMsPage's SELECT. scanVMRow is shared by both callers, so if
only ListVMs carried the column the shared scanner would silently produce "" for the
paginated path — the same class of bug being fixed, one caller over. PendingActionID has a
consumer that acts on an empty value (anyStrandedPending), so it must be in both
projections.

3. internal/corrosion/vms.go — scanVMRow

Add the mapping, next to IsTemplate:

		PendingActionID: r.String("pending_action_id"),

and replace the doc comment with one that describes the actual per-caller contract. The
scanner reads the union of what its callers select and those projections are not
identical, so the comment must not claim every caller carries every column — it would be
false the moment it was written (ListVMsPage deliberately omits vm_owner_epoch; see Scope):

// scanVMRow maps a row from a VM-list projection to a VMRecord. It reads the UNION of the
// columns its callers select, and an absent column reads as a ZERO VALUE, not an error
// (Row.String/Int64 on a missing column) — so a field is only trustworthy on the paths whose
// SELECT actually carries it:
//
//   - pending_action_id: carried by BOTH ListVMs and ListVMsPage. It must be, because
//     grpcapi's anyStrandedPending treats an empty marker on a pending VM as a stranded
//     transfer — an omission here reads as "markerless" and reports a legitimately-minted
//     transfer as stranded (fixed here; previously omitted by both).
//   - vm_owner_epoch: carried by ListVMs ONLY, so OwnerEpoch reads 0 through ListVMsPage.
//     Deliberate: the dual-run detector's index is built from the unpaginated ListVMs and no
//     ListVMsPage consumer reads OwnerEpoch. Anything that starts reading it on the
//     paginated path must add the column there first.
//   - spec_generation / active_operation_id: read by NEITHER list projection. Use GetVM or
//     ListVMsWithActiveOperation.
//
// Adding a field to this scanner therefore means adding its column to every caller whose
// consumers actually read that field.

This keeps the real hazard documented (silent zero values) without asserting a uniformity
invariant the code does not hold, and it names the one asymmetry a future reader would
otherwise have to rediscover.

Also correct the now-stale VMRecord field comment at vms.go:58-61 — it claims the v41
trio is "Populated by GetVM; the ListVMs projection omits them", which is already untrue of
OwnerEpoch. Narrow it to SpecGeneration/ActiveOperationID, the two that really are
omitted by both list projections, and note that OwnerEpoch rides the unpaginated list read
only. Comment-only.

4. internal/grpcapi/ha_health_test.go — extend TestAnyStrandedPending

Insert the missing negative case between the existing running-VM check and the
vmbad markerless-pending case:

	// A state=pending VM that DOES carry a pending_action_id is a legitimately minted
	// transfer, not stranded — the marker is exactly what startPendingVM validates.
	// This case was unreachable while ListVMs' projection omitted the column: every
	// pending VM read as markerless, so a routine reschedule onto this host flapped
	// legacy_pending_stranded.
	if err := corrosion.InsertVM(ctx, s.db, corrosion.VMRecord{
		Name: "vmok", HostName: "test-host", State: "stopped",
	}, nil, nil); err != nil {
		t.Fatalf("InsertVM: %v", err)
	}
	// Mint the pending transition the way the failover coordinator does: proof row +
	// host/state/pending_action_id stamped in ONE batch.
	if err := corrosion.WriteVMRescheduleProof(ctx, s.db, corrosion.ActionProof{
		ID: "p-ok", Action: corrosion.ActionReschedule, TargetKind: "vm",
		TargetName: "vmok", DestHost: "test-host", Coordinator: "coord-1",
		LeaseHolder: "coord-1", QuorumLive: 3, QuorumNeeded: 2,
	}, "vmok", "test-host"); err != nil {
		t.Fatalf("WriteVMRescheduleProof: %v", err)
	}
	if s.anyStrandedPending(ctx) {
		t.Fatal("a pending VM carrying a pending_action_id is a minted transfer, not stranded")
	}

Two things a reviewer should check about this placement:

• Order is load-bearing. anyStrandedPending is an OR over rows, so the negative case
  must be asserted while no stranded row exists. Asserting it after vmbad would be
  vacuous — the function returns true regardless.
• Leaving vmbad last strengthens the existing assertion for free: it now proves the
  detector still fires with a legitimately-marked pending row already in the table, i.e.
  the predicate is per-row, not "any pending ⇒ true" nor "any marker ⇒ false".

Seeding via WriteVMRescheduleProof rather than a raw UPDATE vms SET pending_action_id=…
(the precedent at internal/health/reconciler_gate_test.go:583) is deliberate: it is the
real producer of the (state='pending', pending_action_id) pair, so the fixture cannot drift
from the production shape, and the test would follow a future change to that write. Proof
replication gating is send-side only (replicator.peerLacksProofSupport), so writing a proof
row into a single-node test client is unaffected. p.OwnerEpoch left empty skips
WriteVMRescheduleProof's epoch CAS, which matters because InsertVM leaves
vm_owner_epoch at 0.

5. internal/corrosion/vms_test.go — pin the projection at its own layer

The grpcapi test proves the consequence; this pins the cause, and covers ListVMsPage,
which no grpcapi test reaches for this field:

// pending_action_id must be in BOTH list projections, because an absent column reads as ""
// rather than erroring and grpcapi's anyStrandedPending treats an empty marker on a pending
// VM as a stranded transfer — so an omission on either path reports a legitimately-minted
// transfer as stranded. (This is a per-column requirement, not a claim that the two
// projections are otherwise identical: vm_owner_epoch is intentionally ListVMs-only. See
// scanVMRow's comment for the full per-column breakdown.)
func TestListVMs_SurfacesPendingActionID(t *testing.T) {
	ctx := context.Background()
	c := apTestClient(t)
	apInsertVM(t, c, "vm1", "h1", "running")
	if err := WriteVMRescheduleProof(ctx, c, apProof("p1", "vm1", "h1"), "vm1", "h1"); err != nil {
		t.Fatalf("WriteVMRescheduleProof: %v", err)
	}
	for _, tc := range []struct {
		name string
		list func() ([]VMRecord, error)
	}{
		{"ListVMs", func() ([]VMRecord, error) { return ListVMs(ctx, c, "", "h1") }},
		{"ListVMsPage", func() ([]VMRecord, error) { return ListVMsPage(ctx, c, "", "h1", "", 10) }},
	} {
		vms, err := tc.list()
		if err != nil || len(vms) != 1 {
			t.Fatalf("%s: err=%v rows=%d; want 1 row", tc.name, err, len(vms))
		}
		if vms[0].State != "pending" {
			t.Fatalf("%s: state=%q; want pending", tc.name, vms[0].State)
		}
		if vms[0].PendingActionID != "p1" {
			t.Fatalf("%s: PendingActionID=%q; want p1", tc.name, vms[0].PendingActionID)
		}
	}
}

apTestClient / apInsertVM / apProof already exist in internal/corrosion/action_proofs_test.go
(same package), so no new helper.

No other files change. No schema change, no proto change, no docs change (see below).

Mutation verification

Per CLAUDE.md, the assertion is worthless until seen failing. Exact sequence:

1. Test-first, unfixed source. Apply only changes 4 and 5:
   • go test ./internal/grpcapi/ -run TestAnyStrandedPending -count=1
     → must FAIL on "a pending VM carrying a pending_action_id is a minted transfer, not stranded".
   • go test ./internal/corrosion/ -run TestListVMs_SurfacesPendingActionID -count=1
     → must FAIL on ListVMs: PendingActionID=""; want p1.
     Capture both failure outputs for the PR body.
2. Apply changes 1-3. Rerun both → PASS.
3. Reverse mutations, one at a time, restoring after each (this is what proves the two
   projections are pinned independently, rather than one test masking the other):
   • Delete PendingActionID: from scanVMRow → both tests red.
   • Remove the COALESCE line from ListVMs only → grpcapi test red; corrosion test red on
     the ListVMs case, green on ListVMsPage.
   • Remove it from ListVMsPage only → corrosion test red on the ListVMsPage case;
     grpcapi test green (anyStrandedPending uses the unpaginated ListVMs). This
     asymmetry is the reason change 5 exists.
4. Full gate (CLAUDE.md "Before you push"):
   go build ./... && go vet ./..., go test ./..., make ci-guards.

Baseline recorded before starting: go build ./..., go vet ./internal/corrosion/ ./internal/grpcapi/,
go test ./internal/grpcapi/ -run TestAnyStrandedPending, and
go test ./internal/corrosion/ -run 'TestListVMs_Filter|TestWriteVMRescheduleProof' all pass
on the unmodified tree — so any red in step 1 is attributable to the new assertions.

Scope held

• pending_action_id only. SpecGeneration / ActiveOperationID stay out of both list
  projections. They have no demonstrated live bug (ListVMsWithActiveOperation,
  internal/corrosion/operations.go:507, is the purpose-built reader) and widening the list
  column set touches ~40 ListVMs call sites' data shape for nothing.

• Pre-existing divergence deliberately left alone — a reviewer will spot it, so stating
  it: ListVMs selects COALESCE(vm_owner_epoch, 0) but ListVMsPage does not, so
  OwnerEpoch reads 0 through the paginated path. Its only caller is the ListVMs RPC
  handler (internal/grpcapi/vm.go:920), which never reads OwnerEpoch, and the dual-run
  detector uses the unpaginated ListVMs (internal/grpcapi/dualrun.go:468) — no live bug,
  so out of scope under the "no unmotivated widening" constraint. Worth a follow-up issue,
  not this commit.

  The consequence for every comment this change writes (scanVMRow in change 3, the test
  doc in change 5, and the justification in change 2): the two list projections are not
  uniform, so each states a per-column requirement — pending_action_id must be on both
  paths because anyStrandedPending acts on an empty value — and never the general rule
  "every caller selects every column that scanVMRow reads." That rule is false in the tree
  this plan produces, since vm_owner_epoch stays ListVMs-only.

  Earlier drafts of this plan asserted that rule in two separate places, and both were wrong.
  Worth naming the trap, because the change invites it: the fix's whole shape is "the
  scanner reads a column no projection selected", which reads as an argument for uniformity —
  but the actual defect is narrower and the honest invariant is per-column. A comment stating
  a false invariant is worse than the terse comment it replaces, because the next person
  adding a field would trust it and assume their column is already carried everywhere.

  The alternative resolution — adding vm_owner_epoch to ListVMsPage so the uniform rule
  becomes true — is a projection widening with no demonstrated bug behind it, i.e. exactly
  what this task's constraints exclude. If a future change gives OwnerEpoch a paginated
  consumer, that is when the column should be added, and scanVMRow's comment says so.

What a reviewer needs to know

• Blast radius is exactly anyStrandedPending. PendingActionID has two production
  readers (ha_health.go:272 via ListVMs, reconciler.go:736 via GetVM); nothing writes
  a VMRecord back from a list read (InsertVM does not touch pending_action_id at all,
  vms.go:167), and the ListVMs RPC maps a fixed pb.VM field set that excludes it
  (vm.go:959). So no proto/UI/REST surface changes and no round-trip risk.
• CI guards are not implicated, but run make ci-guards anyway. stmtshapecheck only
  fingerprints replicated statements — calls to Execute, ExecuteRows,
  ExecuteDeferred, ExecuteBatch, ExecuteBatchGuarded (scripts/ci/stmtshapecheck/main.go:456)
  — and loads packages with Tests: false. A Query SELECT is neither replicated nor
  scanned, so no ledger entry and no schema bump are required for this change, and the
  raw SQL in the new tests is invisible to it. writecheck skips _test.go
  (scripts/ci/writecheck/main.go:79) and only flags discarded returns of its named
  corrosion.<Fn> writers; both new test writes check their error regardless.
• No docs change. legacy_pending_stranded appears only in ha_health.go and Go tests —
  no docs/ reference, and the docs-triangulation guard covers commands / config keys /
  litevirt_* identifiers, none of which move here. (The litevirt_ha_degraded metric name
  and its reason vocabulary are unchanged; only how often one reason fires changes.)
• Operational note for the release description: after this lands, a node that was
  chronically reporting legacy_pending_stranded may go quiet — that is the fix, not a lost
  signal. A node still reporting it is now reporting a genuinely markerless pending row
  (an enforcement-flip legacy row), which is the operator-repair case the reason was built
  for.
• Commit: fix(health) — the observable defect is HA health reporting, even though the
  code change lands in internal/corrosion. fix(corrosion) would be equally defensible;
  pick one, not both.

Deviations from the plan as executed

Two, both minor; the plan's substance held everywhere else (every predicted failure message
matched verbatim, including mutation 3's grpcapi/corrosion asymmetry).

1. Three whitespace-only lines beyond change 3's stated scope. vms.go was ALREADY
   gofmt-dirty at de655ea for one reason: scanVMRow's literal had its post-comment tail
   (OwnerEpoch/CreatedAt/UpdatedAt) padded to the pre-comment group's width, which gofmt
   does not do — a comment line splits an alignment group, so each side aligns independently.
   Adding PendingActionID: makes it the longest key in the pre-comment group, widening that
   side and leaving the tail visibly mis-padded against nothing. gofmt -w internal/corrosion/vms.go
   was therefore run, which re-pads those three lines and makes the file gofmt-clean (many other
   files in the tree are not, and no CI guard checks gofmt — make ci-guards has no fmt step).
   The alternative was leaving a struct literal I had just edited misaligned; the churn is three
   lines, entirely inside the literal the change touches.

2. Mutation-verification step 3, second bullet, is only half-observable. The plan predicts
   that removing the column from ListVMs only leaves the corrosion test "red on the ListVMs
   case, green on ListVMsPage". The second half cannot be seen: TestListVMs_SurfacesPendingActionID
   iterates a plain slice (not t.Run subtests) and each assertion is t.Fatalf, so the run
   aborts at the ListVMs case and never reaches ListVMsPage. Observed output was
   ListVMs: PendingActionID=""; want p1, which is the assertion that mutation targeted. The
   ListVMsPage half is pinned independently by the third mutation, which behaved exactly as
   written — corrosion red with ListVMsPage: PendingActionID=""; want p1, grpcapi green —
   so the property the plan wanted proved (the two projections are pinned separately, neither
   test masking the other) is proved. No test change was made for this: converting the loop to
   subtests to surface both cases per mutation would trade a real t.Fatalf-on-first-failure
   idiom used throughout this package for mutation-run ergonomics only.

Verification run: go build ./..., go vet ./..., go test ./..., and make ci-guards all
pass (ci-guards: no schema growth at v50, no ledger drift, 336 replicated statements all
registered, docs triangulation OK).

Final Codex review

No blocking findings remained.

---

Opened by overseer. The plan and the code each converged to zero blocking findings from Codex before this pull request was created.