Astra review r4: settings-5-surfaces task 8 (tenant change resolves fresh)

Range 5c57c95d..1b603228. Fixed in 3e24809f.

Verdict: REJECT

Reviewed only 5c57c95d..1b603228, against the brief, report, prior reviews, rev 11, and R25–R27. Go cannot run here; this is inspection only. No files modified.

Findings

1. [P2] Ignoring a legacy decision can allow a failed build to wake

Where: internal/host/buildstate.go:672.

Scenario: A same-tenant manual instance has T installed, a readable build state, and an untagged consumed decision recording ReasonFailed, Detail: T. There is no separate RefusalNotUp record, and the swap planner is disabled.

The new attribution check removes that failure decision. DecideHeldWake now returns T with AskRoot=true and Failed=false. However, awaitRootDecision immediately returns when T is already live; it neither requests nor awaits root. The wake proceeds, and pre-exec revalidation also admits T.

The implementer’s report records the corresponding failed assertion in TestAFailedInstalledBuildWithNoBuildStateIsHeldFailed. Automatically tagging fixtures made that test pass, but removed its coverage of the legacy case.

Fix: Make an unattributed held-wake decision actually wait for root’s attributed decision, including when the target is already installed. Add an explicitly untagged failed-decision regression through the wake path with no planner. Do not rely on AskRoot alone.

2. [P2] Equivalent account identities lose same-tenant settings

Where: internal/registry/registry.go:1169.

Scenario: A legacy row has account "", app km, and manual or major settings. Re-adding that same app stamps account bab and supplies empty settings/build-state columns.

RouteAccount treats both account spellings as bab; the consumed file remains same-tenant. The new SQL nevertheless identifies a tenant change and clears both columns. Root can adopt the channel head before reconciliation restores the hold. Restoring settings afterward cannot undo that installation.

Fix: Compare effective account identities consistently with RouteAccount, preserving the columns for equivalent legacy identities. Test "" ↔ bab explicitly, alongside genuine account-only changes with empty incoming settings.

3. [P2] The new pre-exec tenant check fails open on a registry read error

Where: internal/host/buildstate.go:527.

Scenario: B owns the host, A’s consumed file remains, and reg.Get fails. Its error is discarded, leaving rt=nil; the new foreign-file check is skipped. b.write can then merge A’s resolution/completion into B’s durable build state. That object write happens before the registry-copy update, so even a subsequent registry error does not prevent contamination.

The discarded error predates this commit, but the new isolation check depends on that read succeeding.

Fix: Return the registry error before any build-state write. Add an injected read-error regression asserting that the durable object remains unchanged. Keep successful “row absent” handling separate from read failure.

Rulings

• R26: Partly agree; disagree with the held-wake safety claim. Preserving untagged bookkeeping in root, pre-exec, and carry avoids losing file-only resolutions and failed completions. Rejecting tagged foreign files is correct. But treating an untagged decision as absent does not always hold execution until root responds; finding 1 demonstrates the exception.
• R27: Agree with the explicitly accepted migration cost. A genuine effective-account change invalidates the old attribution, and a file-only completion or resolution can consequently be lost. Keeping the build-state watermark limits that exposure. This does not cover finding 2: "" → bab leaves the effective tenant unchanged.

Deferred minors triage

• Raw SQL account comparison: promote to must-fix, finding 2. The consequence can be an unwanted installation, not merely temporarily missing columns.
• Discarded beforeExec registry error: promote to must-fix, finding 3. An unread tenant must not authorize merging hostname-keyed bookkeeping.

The remaining inspected changes are sound:

• For distinct canonical tenants, both SQL columns take incoming values, including empty values. Exact same-tenant preservation and the claim-ownership guard remain intact.
• Root filters foreign data before both TargetInput constructions; NextConsumed independently prevents copying foreign bookkeeping.
• The default-settings fresh write uses publishConsumed and its existing lock and route-hold recheck. It introduces no unfenced write; publication failure stops installation.
• The new tests detect the principal foreign-file and column-carry defects. They do not cover the three findings above. The new default-route scenario also lacks a dedicated move-fence interleaving test, although it uses the existing fenced path.

Out of scope

• Existing preservation of good_version, good_digest, good_demand, serving_version, and other unrelated columns across tenant changes.
• Cleanup of retired hosts’ consumed files.
• Copy approval and remaining branch-wide execution gates. Reported test/build results were not independently executed and do not alter this inspection verdict.