test(property): exercise resource rename via OpApply (RFC-0041)

[naxty]

What this PR does

Adds property-test coverage for resource rename (RFC-0041, feature merged in #493). Rename is modelled as a kind of update — folded into the existing OpApply path rather than introduced as a separate operation kind.

Where rename runs

Test	Chaos surface	Assertions
TestProperty_RenameViaApply	None (focused)	Full AssertAllInvariants
TestProperty_FullChaos	Full (cancel, ForceReconcile, crash, drift, TTL, failure injection)	Identity-only

Both pass 100 rapid iterations locally. TestProperty_FullChaos runs in ~12 minutes.

Design decisions

Why rename is folded into OpApply, not a separate OpRename

The engine emits OperationUpdate for a label-only change (or Update + property patch for label + property change). Treating rename as a separate property-test operation would force a parallel codepath — own executor, own generator, own assertion path — for what's already an update.

Folded design:

• Operation.RenameSlotIndex / RenameNewLabel are optional fields on OpApply.
• Generator picks (rename Y/N) and (property template) independently. The natural combinations cover the three update shapes:

Generator outcome	Engine sees
No rename, new property template	property-only update
Rename + new property template	label + property update
Rename + same property template (rare)	label-only update

• Executor: after FormaFromPoolResources builds the forma, the rename overlay flips one resource's Label/Alias and calls RecordRename on success. Snapshot revert on cancel/failure picks up CurrentLabel/PreviousLabel via the existing snapshot path.

Why FullChaos asserts identity-only (not full State/Properties)

The harness's predicted State/Properties model has multiple drift modes under chaos that aren't rename-specific:

• OOB cloud delete sandwiched between snapshot and failure-revert.
• Cascade-destroy abort with dependents-detected leaving children in Exists.
• Partial-success applies + cancel orderings.

Each of these fires regardless of rename. Enabling rename simply expands rapid's op space enough to find them faster. Fixing the prediction model in full is a multi-day refactor independent of rename.

Rename's actual guarantees don't depend on State/Properties prediction:

1. KSUID stable across rename.
2. NativeID stable across rename.
3. No duplicate-NativeID inventory rows.
4. No managed inventory row carries a slot's PreviousLabel.
5. For every tracked NativeID, the inventory row's label tracks the slot's CurrentLabel overlay.

StateModel.IdentityOnlyInvariants = true narrows AssertAllInvariants to checks that fall out of inventory observation:

• CheckInvariants — duplicate-NativeID + cloud-vs-inventory consistency.
• CheckRenameInvariants — invariants 4 and 5 above.
• Command completeness + resource invariants.

Skipped (prediction-based):

• CheckModelVsInventory (State + Properties + Type)
• CheckUnmanagedModelVsInventory
• CheckManagedDriftVsInventory

TestProperty_RenameViaApply keeps full AssertAllInvariants so any rename-shaped regression has a deterministic non-chaos test pointing at it.

Harness changes for rename-awareness

Many helpers needed to honour the slot's CurrentLabel overlay:

Helper	Change
LabelForResource, LabelForSlot	Consult CurrentLabel first, fall back to index-derived default
FindExistingResourceWithNativeID	Returns post-rename label
NativeIDsByLabel	Keys by post-rename label
resolveResourceUpdateSlot	Matches by LabelForResource (post-rename)
RecordRename	Retargets pending ManagedDriftedResources entries to the new label
LabelOverrides	Threaded through every FormaFrom*Resources caller
ResourceSnapshot	Captures CurrentLabel + PreviousLabel for failure-revert
correctModelFromCommandOutcome failure branch	Drift-aware: an OOB-delete + failed-apply lands on NotExist, not the stale Exists snapshot

New invariants

• ViolationRenameOldLabelStillPresent — after rename, no managed row carries the previous label for the slot's (Stack, Type).
• ViolationRenameLabelDriftFromNativeID — for every tracked NativeID, the inventory row's label equals the slot's CurrentLabel-aware value.

Both wired into CheckRenameInvariants, run from the resource-invariants pass.

File-by-file

• operations.go — Operation.RenameSlotIndex/RenameNewLabel, ResourceSnapshot.CurrentLabel/PreviousLabel, PropertyTestConfig.EnableRename.
• state_model.go — ExpectedResource.CurrentLabel/PreviousLabel, LabelForSlot/LabelForResource/NativeIDsByLabel/FindExistingResourceWithNativeID overlay-aware, RecordRename retargets drift, LabelOverrides, IdentityOnlyInvariants flag.
• executor.go — rename overlay in executeApply, resolveResourceUpdateSlot uses LabelForResource, drift-aware failure revert, IdentityOnlyInvariants short-circuit in AssertAllInvariants.
• generators.go — 1-in-3 rename roll per OpApply (parent slots only when a pool is active), renameSlotIndexFromIDs helper, renameLabelGen.
• invariants.go — CheckRenameInvariants adds positive per-NativeID label check, new violation kinds.
• property_test.go — TestProperty_RenameViaApply standalone; TestProperty_FullChaos gains EnableRename: true + IdentityOnlyInvariants.
• Makefile — -run pattern includes TestProperty_RenameViaApply in the 50-iteration tier.

Out of scope (follow-ups)

• Renaming child / cross-stack slots. drawRename filters to parent slots only — ParentLabelForStack/CrossStackParentLabelForStack don't thread label overrides, so renaming a parent with children would leave their $res blocks pointing at the old label.
• Tightening the harness's State/Properties prediction so FullChaos can run full asserts with rename on. Multi-day work, independent of rename.
• Explicit 3-way mode selection on OpApply (label-only / property-only / both). Today the modes fall out of (rename roll, property template draw); mode draw could be made explicit if a more uniform distribution is wanted.

Test plan

• go build -tags "integration property" ./tests/blackbox/ — green.
• go vet -tags "integration property" ./tests/blackbox/ — clean.
• TestProperty_RenameViaApply 50 iterations local — pass.
• TestProperty_FullChaos 100 iterations local, EnableRename on — two consecutive passes (~12 min each).
• Full make test-property on CI.

[browdues]

Okay this one's truly a bit sneaky. It kinda quietly guts the whole point of the PR.

• LabelForSlot (state_model.go:210) isn't pool-aware
• It drops straight to the flat resourceLabelForStack scheme, but its sibling LabelForResource already checks m.Pool
• Both rename tests spin up a 2-tree pool (Resource Cnt 10), so the forma gets built with pool labels while LabelForSlot keeps handing back flat ones
• So for parent slot 5 you get res-stack-0-f out of LabelForSlot vs res-stack-0-b in the forma
  • no match
  • performRename never flips.
  • Rename only ever fires on slot 0, and only by luck. Every other parent is dead weight

Good news tho, I think the fix is tiny, just give LabelForSlot the same m.Pool branch LabelForResource already has. One gotcha though: RecordRename (l.243) calls LabelForSlot too, so it needs the same patch alongside or it starts writing the wrong prev label the moment renames actually fire

[browdues]

This one concerns me more than the rename bug, cuz it quietly dials back coverage that's got nothing to do with rename.

• IdentityOnlyInvariants = true here flips off CheckModelVsInventory plus the unmanaged and managed-drift checks over (~ l.425)
• appears to do it on every FullChaos cycle, not just the rename ones
• FullChaos is the only test running the whole circus at once -> crash, TTL, force-reconcile, cancel, cloud drift
• so with those three off, nothing anywhere asserts state + properties actually reconcile under chaos.
• Seems this is a real regression that just cruises through green

The inline note kinda admits the prediction model's off under chaos and the call was to switch the assertion off instead of fix it. Totally get it's more scope, but flipping it off globally trades away a pile of real, unrelated coverage to land the rename

Two ideas:

• scope the skip to just the rename-touched Native IDs
• leave FullChaos alone with full asserts + stand up a separate identity-only rename-under-chaos test

Aside: the doc comment in invariants.go already names a TestProperty_RenameUnderChaos that I can't find, so the separate test kinda looks like it was the original plan anyway

[JeroenSoeters]

Thorough read, in the context of the drift-determinism rewrite (#656) and the patch-prediction fix (#657) that landed after this branch was cut. The design direction is right and worth landing; the current form can't merge without a substantive rebase and three fixes.

What's good

• Folding rename into OpApply is the correct shape. The engine emits an update for a label change, so a separate operation kind would duplicate an executor/generator/assertion path for nothing. The (rename Y/N) × (property template) matrix covering label-only / property-only / both is elegant.
• CurrentLabel/PreviousLabel as slot overlay + LabelForResource override is clean, and threading LabelOverrides through the forma builders (and resolveResourceUpdateSlot matching by LabelForResource) is exactly what later ops on a renamed slot need.
• CheckRenameInvariants (old label gone from inventory; per-NativeID label matches the overlay) are good positive-identity invariants, and running them from the resource-invariants pass so they also fire mid-chaos is right.
• Honest scoping note on parent-slots-only (child renames would leave $res blocks pointing at the old label) — keep that restriction, but please record the child-rename gap explicitly in the PR description as a follow-up.

Blocking

1. IdentityOnlyInvariants must go. It skips CheckModelVsInventory (and the unmanaged checks) in FullChaos because "the harness prediction has drift modes that aren't rename-specific". Those drift modes were real, and they are gone: #656 deleted the drift-tolerance mechanism and made the model deterministic, #657 fixed the patch prediction, and FullChaos now runs the full invariant suite green on CI and locally. Narrowing invariants for prediction uncertainty is the exact pattern #656 removed, and merging it back in would re-open the door. Post-rebase, rename-under-chaos must be asserted with the full model; if that surfaces rename bugs the identity-only mode was masking, that's the suite doing its job.

2. The unmentioned-slot revert misses labels. The failed-RU revert was extended to compare CurrentLabel/PreviousLabel (good), but step 2 of correctModelFromCommandOutcome — slots snapshotted but unmentioned in a failed/canceled command — still gates on res.State == snap.State and never examines labels. A failed apply whose renamed slot produced no resource update (the agent computed nothing to do for it, or died before reaching it) leaves the model's optimistic CurrentLabel pointing at a label the engine never persisted, with no later correction. This is the same shape as the patch-prediction bug fixed in #657: an optimistic prediction with no RU to correct it. The revert must also restore labels for unmentioned slots (or the rename must be recorded from the RU outcome rather than optimistically — see below).

3. The rebase onto current main is semantic, not mechanical. This branch edits code #656 deleted: ManagedDriftedResources, ignoreManagedDriftNativeIDs, CheckManagedDriftVsInventory, the drift retargeting inside RecordRename, and the OOB-delete drift overlay added to correctModelFromCommandOutcome (that overlay is itself the tolerance pattern — drop it wholesale, its problem no longer exists). FindExistingResourceWithNativeID is now FindDriftEligibleResource, TriggerSyncAndWait takes the model, and patch-mode predictions go through ResolvePatchPropertiesForResources — note a rename also changes the Name property (the template's NAME resolves to the new label), which the patch merge treats as a scalar replace, so that composes, but it deserves a test.

Non-blocking

• renameLabelGen can collide (two renames drawing the same renamed-NNNN in one stack). The comment says "the executor catches the new-label-already-exists case at apply time", but I don't see that check in executeApply — either add it or make the generator consult the model's current labels; a deterministic suite shouldn't rely on the agent's conflict behavior being modeled implicitly.
• Consider recording the rename from the update RU at fold time instead of optimistically at submission. The chicken-and-egg (RU carries the new label, slot lookup needs the model to know it) is real, but it could be resolved by matching the RU against the pending rename's new label explicitly — that removes the need for label-aware reverts entirely and fits the model's compute-from-responses discipline.
• IdentityOnlyInvariants living on StateModel is test configuration on model state; if any variant of it survives (it shouldn't), it belongs on the test config.

Happy to re-review after the rebase; the rename invariants themselves I'd like to see land.

[JeroenSoeters]

Addendum to my review of 2026-08-18. A second, adversarial pass over the diff surfaced eight additional issues; I traced and verified each against head caf269c3 before writing them up. The three blockers from the original review stand unchanged; the first six below join them as blocking, the last two are non-blocking.

Blocking (additional)

4. The rename invariants cannot detect a rename implemented as destroy+recreate. The model keeps no pre-rename identity baseline: corrections overwrite the tracked NativeID from the command response on both create and update (executor.go:947, executor.go:967), and KSUIDs are not tracked in the model at all. Walk the failure through: if the engine (wrongly) planned a rename as delete-old/create-new, the delete RU carries the old label and no longer resolves to the slot (resolveResourceUpdateSlot matches on the post-rename LabelForResource, executor.go:790-796), while the create RU at the new label resolves and installs the new NativeID. Final inventory has one row, new label, new NativeID, new KSUID: no duplicate NativeID, no old-label row, and the positive check (invariants.go:866-894) compares against the identity it just adopted. Every added invariant passes. "KSUID and NativeID stable across rename" is the PR's guarantee 1 and 2, and neither is actually asserted. The fix falls out of the non-blocking suggestion in my original review: record the rename from the RU outcome, and capture the pre-rename NativeID (and ideally KSUID from inventory) as the baseline the invariant compares against.

5. The focused test can pass with rename completely broken. A generated rename that ApplyForma rejects, that comes back ChangesRequired=false, or that fails asynchronously is silently accepted: the first two return before RecordRename (executor.go:1174-1196), the third reverts the snapshot. TestProperty_RenameViaApply (property_test.go:204-229) never asserts any rename was accepted and completed. An engine that rejects every alias passes the focused regression test for RFC-0041 with zero renames exercised. In the focused config (no chaos, no failure injection) an accepted apply is the norm, so asserting that a drawn rename was actually accepted is cheap and closes the vacuity.

6. The focused config generates exactly the rename category that breaks subsequent operations. ResourceCount: 10 is divisible by SlotsPerTree (5), so the focused test runs pool-based with parent/child trees. renameSlotIndexFromIDs restricts renames to parent slots (generators.go:552-567), but FormaFromPoolResources applies label overrides only to a resource's own label: child $res references still come from pool.ParentLabelForStack (executor.go:2535) and pool.CrossStackParentLabelForStack (executor.go:2508), neither override-aware. After a successful parent rename, every later forma containing that parent's children references a label that no longer exists. Note the filter is inverted relative to its own rationale: parents are the referenced class, so they are the unsafe rename targets; leaf slots (grandchildren) are the safe ones. Either thread overrides through the parent-label helpers or restrict renames to unreferenced slots.

7. Failure injection silently stops working for renamed resources. NativeIDsByLabel keys by the post-rename label (state_model.go:326-336), but buildPluginOpSequences reconstructs the lookup label from the pool/index default (executor.go:2698-2702) and silently continues on a miss (executor.go:2710-2712). The Create match key has the same mismatch (executor.go:2750 vs. the override in the forma). So for a renamed slot the drawn failure is never programmed, while successfulResourceIDs still predicts it, and the model diverges from the agent until drain corrections repair it. Under FullChaos with rename on, the advertised rename-under-failure surface is largely not injected.

8. Reusing a freed label produces a false rename violation. renameLabelGen draws renamed-<100..9999> with no awareness of labels already used (generators.go:573-576); its comment claims the executor catches new-label-already-exists, but no such check exists in executeApply (the overlay at executor.go:1150-1160 just flips the label; this is the collision I flagged as non-blocking, now with a concrete failure). Sequence: rename A to renamed-100, rename A again to renamed-200 (A's PreviousLabel becomes renamed-100, state_model.go:243-245), rename B to renamed-100. Final state is correct, but the old-label check flags any managed row at that (stack, type, label) tuple with no identity guard (invariants.go:853-862), so B's legitimate row fires ViolationRenameOldLabelStillPresent. Rapid's shrinking makes this worse than the uniform odds suggest: IntRange shrinks toward 100, so suffix collisions get more likely as a failing sequence is minimized. This is a nondeterministic false failure, the exact class #656 just eliminated.

9. Reusing the label of a destroyed renamed slot makes slot resolution map-order dependent. ApplyDestroyed clears state and properties but not the label overlay (state_model.go:767-775). Rename A to renamed-100, destroy A, rename B to renamed-100: two slots now answer to the same current label, and resolveResourceUpdateSlot takes the first match while ranging over the Resources map (executor.go:790-796), whose iteration order is unspecified. B's successful update can be applied to dead slot A, including properties and NativeID, giving non-reproducible model corrections. Clearing the overlay on destroy plus a label-uniqueness guard in the generator (or executor) closes both this and the previous finding.

Non-blocking (additional)

• Single-depth rename history hides an earlier leaked label. RecordRename keeps only the immediately preceding label (state_model.go:244). If a defective rename A→B leaks row A and a later B→C succeeds, the old-label check now only looks for B, and leaked A has a distinct NativeID so the duplicate check stays quiet (invariants.go:842-894). A per-slot set of retired labels instead of a single PreviousLabel would close it; fine as a follow-up if the RU-outcome refactor from point 4 doesn't already restructure this.
• The immediate cancel rollback cannot find renamed snapshots. buildLabelToSnapshotMap keys by pool/index default labels and ignores ResourceSnapshot.CurrentLabel (executor.go:1610-1623), so a canceled RU carrying a renamed label never matches and RevertResources is skipped (executor.go:1572-1577). The later drain pass usually repairs mentioned slots, so the impact is bounded, but the cancel bookkeeping should key by the snapshot's own label.

The through-line: most of this stems from recording the rename optimistically at submission and keying half the harness by index-derived labels. Recording from the RU outcome (original review, non-blocking suggestion, now upgraded: it resolves points 4 and 5 directly) plus auditing every LabelForStack/resourceLabelForStack call site for overlay-awareness (points 6, 7, and the cancel map) would resolve most of these at once rather than one patch per finding.

[JeroenSoeters]

Superseded by #668, which landed the rename coverage semantically rebased over the deterministic harness with the review blockers addressed. Thanks for the original design — the OpApply fold and the label overlay carried through as-is.